feat: workspace update api stub

_WORK IN PROGRESS_

Signed-off-by: Andy Stoneberg <[email protected]>

Author: andyatmiami

workspaces/backend/api/workspaces_handler.go

@@ -33,25 +33,23 @@ import (
 	repository "github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspaces"
 )
 
-type WorkspaceCreateEnvelope Envelope[*models.WorkspaceCreate]
-
-type WorkspaceUpdateEnvelope Envelope[*models.WorkspaceUpdate]
+type WorkspaceEnvelope Envelope[*models.WorkspaceUpdate]
 
-type WorkspaceListEnvelope Envelope[[]models.Workspace]
+type WorkspaceCreateEnvelope Envelope[*models.WorkspaceCreate]
 
-type WorkspaceEnvelope Envelope[models.Workspace]
+type WorkspaceListEnvelope Envelope[[]models.WorkspaceListItem]
 
 // GetWorkspaceHandler retrieves a specific workspace by namespace and name.
 //
 //	@Summary		Get workspace
-//	@Description	Returns details of a specific workspace identified by namespace and workspace name.
+//	@Description	Returns the current state of a specific workspace identified by namespace and workspace name, including the revision for optimistic locking. This endpoint is intended for retrieving the workspace state before updating it.
 //	@Tags			workspaces
 //	@ID				getWorkspace
 //	@Accept			json
 //	@Produce		json
 //	@Param			namespace		path		string				true	"Namespace of the workspace"	extensions(x-example=kubeflow-user-example-com)
 //	@Param			workspace_name	path		string				true	"Name of the workspace"			extensions(x-example=my-workspace)
-//	@Success		200				{object}	WorkspaceEnvelope	"Successful operation. Returns the requested workspace details."
+//	@Success		200				{object}	WorkspaceEnvelope	"Successful operation. Returns the requested workspace details with revision."
 //	@Failure		401				{object}	ErrorEnvelope		"Unauthorized. Authentication is required."
 //	@Failure		403				{object}	ErrorEnvelope		"Forbidden. User does not have permission to access the workspace."
 //	@Failure		404				{object}	ErrorEnvelope		"Not Found. Workspace does not exist."
@@ -169,7 +167,7 @@ func (a *App) getWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps ht
 	}
 	// ============================================================
 
-	var workspaces []models.Workspace
+	var workspaces []models.WorkspaceListItem
 	var err error
 	if namespace == "" {
 		workspaces, err = a.repositories.Workspace.GetAllWorkspaces(r.Context())
@@ -195,7 +193,7 @@ func (a *App) getWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps ht
 //	@Produce		json
 //	@Param			namespace	path		string					true	"Namespace for the workspace"	extensions(x-example=kubeflow-user-example-com)
 //	@Param			body		body		WorkspaceCreateEnvelope	true	"Workspace creation configuration"
-//	@Success		201			{object}	WorkspaceEnvelope		"Workspace created successfully"
+//	@Success		201			{object}	WorkspaceCreateEnvelope	"Workspace created successfully"
 //	@Failure		400			{object}	ErrorEnvelope			"Bad Request."
 //	@Failure		401			{object}	ErrorEnvelope			"Unauthorized. Authentication is required."
 //	@Failure		403			{object}	ErrorEnvelope			"Forbidden. User does not have permission to create workspace."
@@ -302,18 +300,18 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps
 //	@ID				updateWorkspace
 //	@Accept			json
 //	@Produce		json
-//	@Param			namespace	path		string					true	"Namespace of the workspace"	extensions(x-example=kubeflow-user-example-com)
-//	@Param			name		path		string					true	"Name of the workspace"			extensions(x-example=my-workspace)
-//	@Param			body		body		WorkspaceCreateEnvelope	true	"Workspace creation configuration"
-//	@Success		200			{object}	WorkspaceEnvelope		"Workspace updated successfully"
-//	@Failure		400			{object}	ErrorEnvelope			"Bad Request."
-//	@Failure		401			{object}	ErrorEnvelope			"Unauthorized. Authentication is required."
-//	@Failure		403			{object}	ErrorEnvelope			"Forbidden. User does not have permission to create workspace."
-//	@Failure		409			{object}	ErrorEnvelope			"Conflict. Current workspace generation is newer than provided."
-//	@Failure		413			{object}	ErrorEnvelope			"Request Entity Too Large. The request body is too large."
-//	@Failure		415			{object}	ErrorEnvelope			"Unsupported Media Type. Content-Type header is not correct."
-//	@Failure		422			{object}	ErrorEnvelope			"Unprocessable Entity. Validation error."
-//	@Failure		500			{object}	ErrorEnvelope			"Internal server error. An unexpected error occurred on the server."
+//	@Param			namespace	path		string				true	"Namespace of the workspace"	extensions(x-example=kubeflow-user-example-com)
+//	@Param			name		path		string				true	"Name of the workspace"			extensions(x-example=my-workspace)
+//	@Param			body		body		WorkspaceEnvelope	true	"Workspace update configuration"
+//	@Success		200			{object}	WorkspaceEnvelope	"Workspace updated successfully"
+//	@Failure		400			{object}	ErrorEnvelope		"Bad Request."
+//	@Failure		401			{object}	ErrorEnvelope		"Unauthorized. Authentication is required."
+//	@Failure		403			{object}	ErrorEnvelope		"Forbidden. User does not have permission to create workspace."
+//	@Failure		409			{object}	ErrorEnvelope		"Conflict. Current workspace generation is newer than provided."
+//	@Failure		413			{object}	ErrorEnvelope		"Request Entity Too Large. The request body is too large."
+//	@Failure		415			{object}	ErrorEnvelope		"Unsupported Media Type. Content-Type header is not correct."
+//	@Failure		422			{object}	ErrorEnvelope		"Unprocessable Entity. Validation error."
+//	@Failure		500			{object}	ErrorEnvelope		"Internal server error. An unexpected error occurred on the server."
 //	@Router			/workspaces/{namespace}/{name} [patch]
 func (a *App) UpdateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
 	namespace := ps.ByName(NamespacePathParam)
@@ -328,25 +326,13 @@ func (a *App) UpdateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps
 		return
 	}
 
-	//
-	// TODO: require the caller to provide the generation base that they are expecting to update.
-	//       either include the generation as a field in the request body, or a header.
-	//
-
-	//
-	// TODO: update the "get" workspace (not list) to return the current generation of the workspace.
-	//       this probably means swapping the GET for Workspace with return a WorkspaceUpdate rather than a Workspace
-	//        - it also means that we will be treating GET as only used for retrieving the current state of the workspace,
-	//          before updating it, whereas LIST is used for getting the details of all workspaces (e.g. the table view in the UI).
-	//
-
 	// validate the Content-Type header
 	if success := a.ValidateContentType(w, r, "application/json"); !success {
 		return
 	}
 
 	// decode the request body
-	bodyEnvelope := &WorkspaceUpdateEnvelope{}
+	bodyEnvelope := &WorkspaceEnvelope{}
 	err := a.DecodeJSON(r, bodyEnvelope)
 	if err != nil {
 		if a.IsMaxBytesError(err) {
@@ -400,6 +386,14 @@ func (a *App) UpdateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps
 			a.notFoundResponse(w, r)
 			return
 		}
+		if errors.Is(err, repository.ErrWorkspaceInvalidRevision) {
+			// Extract the underlying error message for better user feedback
+			var valErrs field.ErrorList
+			revisionPath := field.NewPath("data", "revision")
+			valErrs = append(valErrs, field.Invalid(revisionPath, workspaceUpdate.Revision, err.Error()))
+			a.failedValidationResponse(w, r, errMsgRequestBodyInvalid, valErrs, nil)
+			return
+		}
 		//
 		// TODO: there is still a race condition once we actually try and patch/update the workspace,
 		//       unless we use optimistic locking with the generation field
@@ -419,7 +413,7 @@ func (a *App) UpdateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps
 		return
 	}
 
-	responseEnvelope := &WorkspaceUpdateEnvelope{Data: updatedWorkspace}
+	responseEnvelope := &WorkspaceEnvelope{Data: updatedWorkspace}
 	a.dataResponse(w, r, responseEnvelope)
 }
 

workspaces/backend/api/workspaces_handler_test.go

@@ -197,17 +197,17 @@ var _ = Describe("Workspaces Handler", func() {
 
 			By("ensuring the response contains the expected Workspaces")
 			Expect(response.Data).To(ConsistOf(
-				models.NewWorkspaceModelFromWorkspace(workspace1, workspaceKind),
-				models.NewWorkspaceModelFromWorkspace(workspace2, workspaceKind),
-				models.NewWorkspaceModelFromWorkspace(workspace3, workspaceKind),
+				models.NewWorkspaceListItemFromWorkspace(workspace1, workspaceKind),
+				models.NewWorkspaceListItemFromWorkspace(workspace2, workspaceKind),
+				models.NewWorkspaceListItemFromWorkspace(workspace3, workspaceKind),
 			))
 
-			By("ensuring the response can be marshaled to JSON and back to []Workspace")
+			By("ensuring the response can be marshaled to JSON and back to []WorkspaceListItem")
 			dataJSON, err := json.Marshal(response.Data)
 			Expect(err).NotTo(HaveOccurred(), "failed to marshal data to JSON")
-			var dataObject []models.Workspace
+			var dataObject []models.WorkspaceListItem
 			err = json.Unmarshal(dataJSON, &dataObject)
-			Expect(err).NotTo(HaveOccurred(), "failed to unmarshal JSON to []Workspace")
+			Expect(err).NotTo(HaveOccurred(), "failed to unmarshal JSON to []WorkspaceListItem")
 		})
 
 		It("should retrieve Workspaces from Namespace 1 successfully", func() {
@@ -252,16 +252,16 @@ var _ = Describe("Workspaces Handler", func() {
 
 			By("ensuring the response contains the expected Workspaces")
 			Expect(response.Data).To(ConsistOf(
-				models.NewWorkspaceModelFromWorkspace(workspace1, workspaceKind),
-				models.NewWorkspaceModelFromWorkspace(workspace2, workspaceKind),
+				models.NewWorkspaceListItemFromWorkspace(workspace1, workspaceKind),
+				models.NewWorkspaceListItemFromWorkspace(workspace2, workspaceKind),
 			))
 
-			By("ensuring the response can be marshaled to JSON and back to []Workspace")
+			By("ensuring the response can be marshaled to JSON and back to []WorkspaceListItem")
 			dataJSON, err := json.Marshal(response.Data)
 			Expect(err).NotTo(HaveOccurred(), "failed to marshal data to JSON")
-			var dataObject []models.Workspace
+			var dataObject []models.WorkspaceListItem
 			err = json.Unmarshal(dataJSON, &dataObject)
-			Expect(err).NotTo(HaveOccurred(), "failed to unmarshal JSON to []Workspace")
+			Expect(err).NotTo(HaveOccurred(), "failed to unmarshal JSON to []WorkspaceListItem")
 		})
 
 		It("should retrieve a single Workspace successfully", func() {
@@ -296,23 +296,24 @@ var _ = Describe("Workspaces Handler", func() {
 			err = json.Unmarshal(body, &response)
 			Expect(err).NotTo(HaveOccurred())
 
-			By("getting the WorkspaceKind from the Kubernetes API")
-			workspaceKind := &kubefloworgv1beta1.WorkspaceKind{}
-			Expect(k8sClient.Get(ctx, workspaceKindKey, workspaceKind)).To(Succeed())
-
 			By("getting the Workspace from the Kubernetes API")
 			workspace := &kubefloworgv1beta1.Workspace{}
 			Expect(k8sClient.Get(ctx, workspaceKey1, workspace)).To(Succeed())
 
-			By("ensuring the response matches the expected Workspace")
-			Expect(response.Data).To(BeComparableTo(models.NewWorkspaceModelFromWorkspace(workspace, workspaceKind)))
+			By("ensuring the response matches the expected WorkspaceUpdate")
+			expectedWorkspaceUpdate := models.NewWorkspaceUpdateModelFromWorkspace(workspace)
+			// Normalize Secrets to nil if empty to match JSON unmarshaling behavior (omitempty causes empty slices to become nil)
+			if len(expectedWorkspaceUpdate.PodTemplate.Volumes.Secrets) == 0 {
+				expectedWorkspaceUpdate.PodTemplate.Volumes.Secrets = nil
+			}
+			Expect(response.Data).To(BeComparableTo(expectedWorkspaceUpdate))
 
-			By("ensuring the response can be marshaled to JSON and back to Workspace")
+			By("ensuring the response can be marshaled to JSON and back to WorkspaceUpdate")
 			dataJSON, err := json.Marshal(response.Data)
 			Expect(err).NotTo(HaveOccurred(), "failed to marshal data to JSON")
-			var dataObject models.Workspace
+			var dataObject models.WorkspaceUpdate
 			err = json.Unmarshal(dataJSON, &dataObject)
-			Expect(err).NotTo(HaveOccurred(), "failed to unmarshal JSON to Workspace")
+			Expect(err).NotTo(HaveOccurred(), "failed to unmarshal JSON to WorkspaceUpdate")
 		})
 	})
 
@@ -463,7 +464,7 @@ var _ = Describe("Workspaces Handler", func() {
 			Expect(k8sClient.Get(ctx, workspaceInvalidImageConfigKey, workspaceInvalidImageConfig)).To(Succeed())
 
 			By("ensuring the model for Workspace with missing WorkspaceKind is as expected")
-			workspaceMissingWskModel := models.NewWorkspaceModelFromWorkspace(workspaceMissingWsk, nil)
+			workspaceMissingWskModel := models.NewWorkspaceListItemFromWorkspace(workspaceMissingWsk, nil)
 			Expect(workspaceMissingWskModel.WorkspaceKind.Missing).To(BeTrue())
 			Expect(workspaceMissingWskModel.PodTemplate.Volumes.Home.MountPath).To(Equal(models.UnknownHomeMountPath))
 			Expect(workspaceMissingWskModel.PodTemplate.Options.PodConfig.Current.DisplayName).To(Equal(models.UnknownPodConfig))
@@ -472,12 +473,12 @@ var _ = Describe("Workspaces Handler", func() {
 			Expect(workspaceMissingWskModel.PodTemplate.Options.ImageConfig.Current.Description).To(Equal(models.UnknownImageConfig))
 
 			By("ensuring the model for Workspace with invalid PodConfig is as expected")
-			workspaceInvalidPodConfigModel := models.NewWorkspaceModelFromWorkspace(workspaceInvalidPodConfig, workspaceKind)
+			workspaceInvalidPodConfigModel := models.NewWorkspaceListItemFromWorkspace(workspaceInvalidPodConfig, workspaceKind)
 			Expect(workspaceInvalidPodConfigModel.PodTemplate.Options.PodConfig.Current.DisplayName).To(Equal(models.UnknownPodConfig))
 			Expect(workspaceInvalidPodConfigModel.PodTemplate.Options.PodConfig.Current.Description).To(Equal(models.UnknownPodConfig))
 
 			By("ensuring the model for Workspace with invalid ImageConfig is as expected")
-			workspaceInvalidImageConfigModel := models.NewWorkspaceModelFromWorkspace(workspaceInvalidImageConfig, workspaceKind)
+			workspaceInvalidImageConfigModel := models.NewWorkspaceListItemFromWorkspace(workspaceInvalidImageConfig, workspaceKind)
 			Expect(workspaceInvalidImageConfigModel.PodTemplate.Options.ImageConfig.Current.DisplayName).To(Equal(models.UnknownImageConfig))
 			Expect(workspaceInvalidImageConfigModel.PodTemplate.Options.ImageConfig.Current.Description).To(Equal(models.UnknownImageConfig))
 
@@ -489,7 +490,7 @@ var _ = Describe("Workspaces Handler", func() {
 			))
 		})
 
-		It("should retrieve a single invalid Workspace successfully", func() {
+		It("should retrieve a single Workspace successfully", func() {
 			By("creating the HTTP request")
 			path := strings.Replace(WorkspacesByNamePath, ":"+NamespacePathParam, namespaceName1, 1)
 			path = strings.Replace(path, ":"+ResourceNamePathParam, workspaceMissingWskName, 1)
@@ -525,8 +526,12 @@ var _ = Describe("Workspaces Handler", func() {
 			workspaceMissingWsk := &kubefloworgv1beta1.Workspace{}
 			Expect(k8sClient.Get(ctx, workspaceMissingWskKey, workspaceMissingWsk)).To(Succeed())
 
-			By("ensuring the response matches the expected Workspace")
-			workspaceMissingWskModel := models.NewWorkspaceModelFromWorkspace(workspaceMissingWsk, nil)
+			By("ensuring the response matches the expected WorkspaceUpdate")
+			workspaceMissingWskModel := models.NewWorkspaceUpdateModelFromWorkspace(workspaceMissingWsk)
+			// Normalize Secrets to nil if empty to match JSON unmarshaling behavior (omitempty causes empty slices to become nil)
+			if len(workspaceMissingWskModel.PodTemplate.Volumes.Secrets) == 0 {
+				workspaceMissingWskModel.PodTemplate.Volumes.Secrets = nil
+			}
 			Expect(response.Data).To(BeComparableTo(workspaceMissingWskModel))
 		})
 	})

workspaces/backend/internal/models/workspaces/funcs.go

@@ -29,9 +29,9 @@ const (
 	UnknownPodConfig     = "__UNKNOWN_POD_CONFIG__"
 )
 
-// NewWorkspaceModelFromWorkspace creates a Workspace model from a Workspace and WorkspaceKind object.
+// NewWorkspaceListItemFromWorkspace creates a WorkspaceListItem model from a Workspace and WorkspaceKind object.
 // NOTE: the WorkspaceKind might not exist, so we handle the case where it is nil or has no UID.
-func NewWorkspaceModelFromWorkspace(ws *kubefloworgv1beta1.Workspace, wsk *kubefloworgv1beta1.WorkspaceKind) Workspace {
+func NewWorkspaceListItemFromWorkspace(ws *kubefloworgv1beta1.Workspace, wsk *kubefloworgv1beta1.WorkspaceKind) WorkspaceListItem {
 	// ensure the provided WorkspaceKind matches the Workspace
 	if wskExists(wsk) && ws.Spec.Kind != wsk.Name {
 		panic("provided WorkspaceKind does not match the Workspace")
@@ -98,7 +98,7 @@ func NewWorkspaceModelFromWorkspace(ws *kubefloworgv1beta1.Workspace, wsk *kubef
 		}
 	}
 
-	workspaceModel := Workspace{
+	workspaceModel := WorkspaceListItem{
 		Name:      ws.Name,
 		Namespace: ws.Namespace,
 		WorkspaceKind: WorkspaceKindInfo{

workspaces/backend/internal/models/workspaces/funcs_write.go

@@ -17,6 +17,8 @@ limitations under the License.
 package workspaces
 
 import (
+	"strconv"
+
 	kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1"
 	"k8s.io/utils/ptr"
 )
@@ -44,6 +46,15 @@ func NewWorkspaceCreateModelFromWorkspace(ws *kubefloworgv1beta1.Workspace) *Wor
 		}
 	}
 
+	secretMounts := make([]PodSecretMount, len(ws.Spec.PodTemplate.Volumes.Secrets))
+	for i, s := range ws.Spec.PodTemplate.Volumes.Secrets {
+		secretMounts[i] = PodSecretMount{
+			SecretName:  s.SecretName,
+			MountPath:   s.MountPath,
+			DefaultMode: s.DefaultMode,
+		}
+	}
+
 	workspaceCreateModel := &WorkspaceCreate{
 		Name:         ws.Name,
 		Kind:         ws.Spec.Kind,
@@ -55,8 +66,9 @@ func NewWorkspaceCreateModelFromWorkspace(ws *kubefloworgv1beta1.Workspace) *Wor
 				Annotations: podAnnotations,
 			},
 			Volumes: PodVolumesMutate{
-				Home: ws.Spec.PodTemplate.Volumes.Home,
-				Data: dataVolumes,
+				Home:    ws.Spec.PodTemplate.Volumes.Home,
+				Data:    dataVolumes,
+				Secrets: secretMounts,
 			},
 			Options: PodTemplateOptionsMutate{
 				ImageConfig: ws.Spec.PodTemplate.Options.ImageConfig,
@@ -91,7 +103,17 @@ func NewWorkspaceUpdateModelFromWorkspace(ws *kubefloworgv1beta1.Workspace) *Wor
 		}
 	}
 
+	secretMounts := make([]PodSecretMount, len(ws.Spec.PodTemplate.Volumes.Secrets))
+	for i, s := range ws.Spec.PodTemplate.Volumes.Secrets {
+		secretMounts[i] = PodSecretMount{
+			SecretName:  s.SecretName,
+			MountPath:   s.MountPath,
+			DefaultMode: s.DefaultMode,
+		}
+	}
+
 	workspaceUpdateModel := &WorkspaceUpdate{
+		Revision:     strconv.FormatInt(ws.Generation, 10),
 		Paused:       ptr.Deref(ws.Spec.Paused, false),
 		DeferUpdates: ptr.Deref(ws.Spec.DeferUpdates, false),
 		PodTemplate: PodTemplateMutate{
@@ -100,8 +122,9 @@ func NewWorkspaceUpdateModelFromWorkspace(ws *kubefloworgv1beta1.Workspace) *Wor
 				Annotations: podAnnotations,
 			},
 			Volumes: PodVolumesMutate{
-				Home: ws.Spec.PodTemplate.Volumes.Home,
-				Data: dataVolumes,
+				Home:    ws.Spec.PodTemplate.Volumes.Home,
+				Data:    dataVolumes,
+				Secrets: secretMounts,
 			},
 			Options: PodTemplateOptionsMutate{
 				ImageConfig: ws.Spec.PodTemplate.Options.ImageConfig,

workspaces/backend/internal/models/workspaces/types.go

@@ -16,9 +16,11 @@ limitations under the License.
 
 package workspaces
 
-// Workspace represents a workspace in the system, and is returned by GET and LIST operations.
-// NOTE: this type is not used for CREATE or UPDATE operations, see WorkspaceCreate
-type Workspace struct {
+// WorkspaceListItem represents a workspace in the system, and is returned by LIST operations.
+// NOTE: this type is not used for GET, CREATE or UPDATE operations, see WorkspaceUpdate and WorkspaceCreate
+// TODO: we need to validate which fields should actually be returned in the response
+//   - should only be returning fields relevant to the list view in the UI
+type WorkspaceListItem struct {
 	Name           string            `json:"name"`
 	Namespace      string            `json:"namespace"`
 	WorkspaceKind  WorkspaceKindInfo `json:"workspaceKind"`
@@ -66,6 +68,7 @@ type PodMetadata struct {
 	Annotations map[string]string `json:"annotations"`
 }
 
+// TODO: determine proper use of omitempty for PodVolumes
 type PodVolumes struct {
 	Home    *PodVolumeInfo  `json:"home,omitempty"`
 	Data    []PodVolumeInfo `json:"data"`