From f39d6e72379003c57c86f00821e8bc5a36a7a3b5 Mon Sep 17 00:00:00 2001 From: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com> Date: Tue, 4 Feb 2025 16:33:10 -0800 Subject: [PATCH] feat(ws): add WorkspaceCreate model to backend Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com> --- workspaces/backend/README.md | 93 +++++++++--- workspaces/backend/api/app.go | 12 +- .../backend/api/healthcheck_handler_test.go | 2 +- workspaces/backend/api/helpers.go | 46 ++++++ workspaces/backend/api/logging.go | 46 ++++++ workspaces/backend/api/namespaces_handler.go | 12 +- .../backend/api/namespaces_handler_test.go | 6 +- .../api/{errors.go => response_errors.go} | 142 ++++++++++++------ workspaces/backend/api/response_success.go | 41 +++++ workspaces/backend/api/suite_test.go | 9 +- .../backend/api/workspacekinds_handler.go | 35 ++--- .../api/workspacekinds_handler_test.go | 24 +-- workspaces/backend/api/workspaces_handler.go | 138 ++++++++++------- .../backend/api/workspaces_handler_test.go | 139 +++++++++-------- .../backend/internal/helper/validation.go | 94 ++++++++++++ .../internal/models/workspaces/funcs.go | 10 +- .../models/workspaces/funcs_create.go | 69 +++++++++ .../internal/models/workspaces/types.go | 4 +- .../models/workspaces/types_create.go | 91 +++++++++++ .../internal/repositories/namespaces/repo.go | 3 - .../repositories/workspacekinds/repo.go | 1 - .../internal/repositories/workspaces/repo.go | 56 ++++--- 22 files changed, 789 insertions(+), 284 deletions(-) create mode 100644 workspaces/backend/api/logging.go rename workspaces/backend/api/{errors.go => response_errors.go} (60%) create mode 100644 workspaces/backend/api/response_success.go create mode 100644 workspaces/backend/internal/helper/validation.go create mode 100644 workspaces/backend/internal/models/workspaces/funcs_create.go create mode 100644 workspaces/backend/internal/models/workspaces/types_create.go diff --git a/workspaces/backend/README.md b/workspaces/backend/README.md index 25d96513..eb088313 100644 --- a/workspaces/backend/README.md +++ b/workspaces/backend/README.md @@ -7,21 +7,29 @@ The Kubeflow Workspaces Backend is the _backend for frontend_ (BFF) used by the > We greatly appreciate any contributions. # Building and Deploying + TBD # Development + Run the following command to build the BFF: + ```shell make build ``` + After building it, you can run our app with: + ```shell make run ``` + If you want to use a different port: + ```shell make run PORT=8000 ``` + ### Endpoints | URL Pattern | Handler | Action | @@ -43,56 +51,99 @@ make run PORT=8000 | DELETE /api/v1/workspacekinds/{name} | TBD | Delete a WorkspaceKind entity | ### Sample local calls -``` + +Healthcheck: + +```shell # GET /api/v1/healthcheck curl -i localhost:4000/api/v1/healthcheck ``` -``` + +List all Namespaces: + +```shell # GET /api/v1/namespaces curl -i localhost:4000/api/v1/namespaces ``` -``` + +List all Workspaces: + +```shell # GET /api/v1/workspaces/ curl -i localhost:4000/api/v1/workspaces ``` -``` + +List all Workspaces in a Namespace: + +```shell # GET /api/v1/workspaces/{namespace} curl -i localhost:4000/api/v1/workspaces/default ``` -``` + +Create a Workspace: + +```shell # POST /api/v1/workspaces/{namespace} curl -X POST http://localhost:4000/api/v1/workspaces/default \ -H "Content-Type: application/json" \ -d '{ + "data": { "name": "dora", + "kind": "jupyterlab", "paused": false, "defer_updates": false, - "kind": "jupyterlab", - "image_config": "jupyterlab_scipy_190", - "pod_config": "tiny_cpu", - "home_volume": "workspace-home-bella", - "data_volumes": [ - { - "pvc_name": "workspace-data-bella", - "mount_path": "/data/my-data", - "read_only": false + "pod_template": { + "pod_metadata": { + "labels": { + "app": "dora" + }, + "annotations": { + "app": "dora" + } + }, + "volumes": { + "home": "workspace-home-bella", + "data": [ + { + "pvc_name": "workspace-data-bella", + "mount_path": "/data/my-data", + "read_only": false + } + ] + }, + "options": { + "image_config": "jupyterlab_scipy_190", + "pod_config": "tiny_cpu" } - ] - }' -``` + } + } +}' ``` + +Get a Workspace: + +```shell # GET /api/v1/workspaces/{namespace}/{name} curl -i localhost:4000/api/v1/workspaces/default/dora ``` -``` + +Delete a Workspace: + +```shell # DELETE /api/v1/workspaces/{namespace}/{name} -curl -X DELETE localhost:4000/api/v1/workspaces/workspace-test/dora -``` +curl -X DELETE localhost:4000/api/v1/workspaces/default/dora ``` + +List all WorkspaceKinds: + +```shell # GET /api/v1/workspacekinds curl -i localhost:4000/api/v1/workspacekinds ``` -``` + +Get a WorkspaceKind: + +```shell # GET /api/v1/workspacekinds/{name} curl -i localhost:4000/api/v1/workspacekinds/jupyterlab ``` diff --git a/workspaces/backend/api/app.go b/workspaces/backend/api/app.go index efb11b3b..5e5dc233 100644 --- a/workspaces/backend/api/app.go +++ b/workspaces/backend/api/app.go @@ -34,20 +34,20 @@ const ( Version = "1.0.0" PathPrefix = "/api/v1" + NamespacePathParam = "namespace" + ResourceNamePathParam = "name" + // healthcheck HealthCheckPath = PathPrefix + "/healthcheck" // workspaces AllWorkspacesPath = PathPrefix + "/workspaces" - NamespacePathParam = "namespace" - WorkspaceNamePathParam = "name" WorkspacesByNamespacePath = AllWorkspacesPath + "/:" + NamespacePathParam - WorkspacesByNamePath = AllWorkspacesPath + "/:" + NamespacePathParam + "/:" + WorkspaceNamePathParam + WorkspacesByNamePath = AllWorkspacesPath + "/:" + NamespacePathParam + "/:" + ResourceNamePathParam // workspacekinds - AllWorkspaceKindsPath = PathPrefix + "/workspacekinds" - WorkspaceKindNamePathParam = "name" - WorkspaceKindsByNamePath = AllWorkspaceKindsPath + "/:" + WorkspaceNamePathParam + AllWorkspaceKindsPath = PathPrefix + "/workspacekinds" + WorkspaceKindsByNamePath = AllWorkspaceKindsPath + "/:" + ResourceNamePathParam // namespaces AllNamespacesPath = PathPrefix + "/namespaces" diff --git a/workspaces/backend/api/healthcheck_handler_test.go b/workspaces/backend/api/healthcheck_handler_test.go index ec1fe340..ddc1de0a 100644 --- a/workspaces/backend/api/healthcheck_handler_test.go +++ b/workspaces/backend/api/healthcheck_handler_test.go @@ -46,7 +46,7 @@ var _ = Describe("HealthCheck Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) diff --git a/workspaces/backend/api/helpers.go b/workspaces/backend/api/helpers.go index eb5f09c1..ed14d3f8 100644 --- a/workspaces/backend/api/helpers.go +++ b/workspaces/backend/api/helpers.go @@ -18,13 +18,20 @@ package api import ( "encoding/json" + "fmt" + "mime" "net/http" + "strings" ) +// Envelope is the body of all requests and responses that contain data. +// NOTE: error responses use the ErrorEnvelope type type Envelope[D any] struct { + // TODO: make all declarations of Envelope use pointers for D Data D `json:"data"` } +// WriteJSON writes a JSON response with the given status code, data, and headers. func (a *App) WriteJSON(w http.ResponseWriter, status int, data any, headers http.Header) error { js, err := json.MarshalIndent(data, "", "\t") @@ -47,3 +54,42 @@ func (a *App) WriteJSON(w http.ResponseWriter, status int, data any, headers htt return nil } + +// DecodeJSON decodes the JSON request body into the given value. +func (a *App) DecodeJSON(r *http.Request, v any) error { + decoder := json.NewDecoder(r.Body) + decoder.DisallowUnknownFields() + if err := decoder.Decode(v); err != nil { + return fmt.Errorf("error decoding JSON: %w", err) + } + return nil +} + +// ValidateContentType validates the Content-Type header of the request. +// If this method returns false, the request has been handled and the caller should return immediately. +// If this method returns true, the request has the correct Content-Type. +func (a *App) ValidateContentType(w http.ResponseWriter, r *http.Request, expectedMediaType string) bool { + contentType := r.Header.Get("Content-Type") + if contentType == "" { + a.unsupportedMediaTypeResponse(w, r, fmt.Errorf("Content-Type header is missing")) + return false + } + mediaType, _, err := mime.ParseMediaType(contentType) + if err != nil { + a.badRequestResponse(w, r, fmt.Errorf("error parsing Content-Type header: %w", err)) + return false + } + if mediaType != expectedMediaType { + a.unsupportedMediaTypeResponse(w, r, fmt.Errorf("unsupported media type: %s, expected: %s", mediaType, expectedMediaType)) + return false + } + + return true +} + +// LocationGetWorkspace returns the GET location (HTTP path) for a workspace resource. +func (a *App) LocationGetWorkspace(namespace, name string) string { + path := strings.Replace(WorkspacesByNamePath, ":"+NamespacePathParam, namespace, 1) + path = strings.Replace(path, ":"+ResourceNamePathParam, name, 1) + return path +} diff --git a/workspaces/backend/api/logging.go b/workspaces/backend/api/logging.go new file mode 100644 index 00000000..be98d5a1 --- /dev/null +++ b/workspaces/backend/api/logging.go @@ -0,0 +1,46 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package api + +import "net/http" + +// LogError logs an error message with the request details. +func (a *App) LogError(r *http.Request, err error) { + var ( + method = r.Method + uri = r.URL.RequestURI() + ) + a.logger.Error(err.Error(), "method", method, "uri", uri) +} + +// LogWarn logs a warning message with the request details. +func (a *App) LogWarn(r *http.Request, message string) { + var ( + method = r.Method + uri = r.URL.RequestURI() + ) + a.logger.Warn(message, "method", method, "uri", uri) +} + +// LogInfo logs an info message with the request details. +func (a *App) LogInfo(r *http.Request, message string) { + var ( + method = r.Method + uri = r.URL.RequestURI() + ) + a.logger.Info(message, "method", method, "uri", uri) +} diff --git a/workspaces/backend/api/namespaces_handler.go b/workspaces/backend/api/namespaces_handler.go index dc41380e..fb62d7d7 100644 --- a/workspaces/backend/api/namespaces_handler.go +++ b/workspaces/backend/api/namespaces_handler.go @@ -26,7 +26,7 @@ import ( models "github.com/kubeflow/notebooks/workspaces/backend/internal/models/namespaces" ) -type NamespacesEnvelope Envelope[[]models.Namespace] +type NamespaceListEnvelope Envelope[[]models.Namespace] func (a *App) GetNamespacesHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { @@ -48,12 +48,6 @@ func (a *App) GetNamespacesHandler(w http.ResponseWriter, r *http.Request, _ htt return } - namespacesEnvelope := NamespacesEnvelope{ - Data: namespaces, - } - - err = a.WriteJSON(w, http.StatusOK, namespacesEnvelope, nil) - if err != nil { - a.serverErrorResponse(w, r, err) - } + responseEnvelope := &NamespaceListEnvelope{Data: namespaces} + a.dataResponse(w, r, responseEnvelope) } diff --git a/workspaces/backend/api/namespaces_handler_test.go b/workspaces/backend/api/namespaces_handler_test.go index 5f12911e..2acbda55 100644 --- a/workspaces/backend/api/namespaces_handler_test.go +++ b/workspaces/backend/api/namespaces_handler_test.go @@ -93,14 +93,14 @@ var _ = Describe("Namespaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) Expect(err).NotTo(HaveOccurred()) - By("unmarshalling the response JSON to NamespacesEnvelope") - var response NamespacesEnvelope + By("unmarshalling the response JSON to NamespaceListEnvelope") + var response NamespaceListEnvelope err = json.Unmarshal(body, &response) Expect(err).NotTo(HaveOccurred()) diff --git a/workspaces/backend/api/errors.go b/workspaces/backend/api/response_errors.go similarity index 60% rename from workspaces/backend/api/errors.go rename to workspaces/backend/api/response_errors.go index 3c807fcd..cf7e1dd9 100644 --- a/workspaces/backend/api/errors.go +++ b/workspaces/backend/api/response_errors.go @@ -17,45 +17,72 @@ limitations under the License. package api import ( - "encoding/json" "fmt" "net/http" "strconv" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +const ( + errMsgPathParamsInvalid = "path parameters were invalid" + errMsgRequestBodyInvalid = "request body was invalid" + errMsgKubernetesValidation = "kubernetes validation error (note: .cause.validation_errors[] correspond to the internal k8s object, not the request body)" ) +// ErrorEnvelope is the body of all error responses. +type ErrorEnvelope struct { + Error *HTTPError `json:"error"` +} + type HTTPError struct { StatusCode int `json:"-"` ErrorResponse } type ErrorResponse struct { - Code string `json:"code"` - Message string `json:"message"` + Code string `json:"code"` + Message string `json:"message"` + Cause *ErrorCause `json:"cause,omitempty"` } -type ErrorEnvelope struct { - Error *HTTPError `json:"error"` +type ErrorCause struct { + ValidationErrors []ValidationError `json:"validation_errors,omitempty"` } -func (a *App) LogError(r *http.Request, err error) { - var ( - method = r.Method - uri = r.URL.RequestURI() - ) +type ValidationError struct { + Type field.ErrorType `json:"type"` + Field string `json:"field"` + Message string `json:"message"` +} + +// errorResponse writes an error response to the client. +func (a *App) errorResponse(w http.ResponseWriter, r *http.Request, httpError *HTTPError) { + env := ErrorEnvelope{Error: httpError} - a.logger.Error(err.Error(), "method", method, "uri", uri) + err := a.WriteJSON(w, httpError.StatusCode, env, nil) + if err != nil { + a.LogError(r, err) + w.WriteHeader(httpError.StatusCode) + } } -func (a *App) LogWarn(r *http.Request, message string) { - var ( - method = r.Method - uri = r.URL.RequestURI() - ) +// HTTP: 500 +func (a *App) serverErrorResponse(w http.ResponseWriter, r *http.Request, err error) { + a.LogError(r, err) - a.logger.Warn(message, "method", method, "uri", uri) + httpError := &HTTPError{ + StatusCode: http.StatusInternalServerError, + ErrorResponse: ErrorResponse{ + Code: strconv.Itoa(http.StatusInternalServerError), + Message: "the server encountered a problem and could not process your request", + }, + } + a.errorResponse(w, r, httpError) } -//nolint:unused +// HTTP: 400 func (a *App) badRequestResponse(w http.ResponseWriter, r *http.Request, err error) { httpError := &HTTPError{ StatusCode: http.StatusBadRequest, @@ -67,29 +94,33 @@ func (a *App) badRequestResponse(w http.ResponseWriter, r *http.Request, err err a.errorResponse(w, r, httpError) } -func (a *App) errorResponse(w http.ResponseWriter, r *http.Request, httpError *HTTPError) { - env := ErrorEnvelope{Error: httpError} - - err := a.WriteJSON(w, httpError.StatusCode, env, nil) - if err != nil { - a.LogError(r, err) - w.WriteHeader(httpError.StatusCode) +// HTTP: 401 +func (a *App) unauthorizedResponse(w http.ResponseWriter, r *http.Request) { + httpError := &HTTPError{ + StatusCode: http.StatusUnauthorized, + ErrorResponse: ErrorResponse{ + Code: strconv.Itoa(http.StatusUnauthorized), + Message: "authentication is required to access this resource", + }, } + a.errorResponse(w, r, httpError) } -func (a *App) serverErrorResponse(w http.ResponseWriter, r *http.Request, err error) { - a.LogError(r, err) +// HTTP: 403 +func (a *App) forbiddenResponse(w http.ResponseWriter, r *http.Request, msg string) { + a.LogWarn(r, msg) httpError := &HTTPError{ - StatusCode: http.StatusInternalServerError, + StatusCode: http.StatusForbidden, ErrorResponse: ErrorResponse{ - Code: strconv.Itoa(http.StatusInternalServerError), - Message: "the server encountered a problem and could not process your request", + Code: strconv.Itoa(http.StatusForbidden), + Message: "you are not authorized to access this resource", }, } a.errorResponse(w, r, httpError) } +// HTTP: 404 func (a *App) notFoundResponse(w http.ResponseWriter, r *http.Request) { httpError := &HTTPError{ StatusCode: http.StatusNotFound, @@ -101,6 +132,7 @@ func (a *App) notFoundResponse(w http.ResponseWriter, r *http.Request) { a.errorResponse(w, r, httpError) } +// HTTP: 405 func (a *App) methodNotAllowedResponse(w http.ResponseWriter, r *http.Request) { httpError := &HTTPError{ StatusCode: http.StatusMethodNotAllowed, @@ -112,42 +144,60 @@ func (a *App) methodNotAllowedResponse(w http.ResponseWriter, r *http.Request) { a.errorResponse(w, r, httpError) } -func (a *App) unauthorizedResponse(w http.ResponseWriter, r *http.Request) { +// HTTP: 409 +func (a *App) conflictResponse(w http.ResponseWriter, r *http.Request, err error) { httpError := &HTTPError{ - StatusCode: http.StatusUnauthorized, + StatusCode: http.StatusConflict, ErrorResponse: ErrorResponse{ - Code: strconv.Itoa(http.StatusUnauthorized), - Message: "authentication is required to access this resource", + Code: strconv.Itoa(http.StatusConflict), + Message: err.Error(), }, } a.errorResponse(w, r, httpError) } -func (a *App) forbiddenResponse(w http.ResponseWriter, r *http.Request, msg string) { - a.LogWarn(r, msg) - +// HTTP:415 +func (a *App) unsupportedMediaTypeResponse(w http.ResponseWriter, r *http.Request, err error) { httpError := &HTTPError{ - StatusCode: http.StatusForbidden, + StatusCode: http.StatusUnsupportedMediaType, ErrorResponse: ErrorResponse{ - Code: strconv.Itoa(http.StatusForbidden), - Message: "you are not authorized to access this resource", + Code: strconv.Itoa(http.StatusUnsupportedMediaType), + Message: err.Error(), }, } a.errorResponse(w, r, httpError) } -//nolint:unused -func (a *App) failedValidationResponse(w http.ResponseWriter, r *http.Request, errors map[string]string) { - message, err := json.Marshal(errors) - if err != nil { - message = []byte("{}") +// HTTP: 422 +func (a *App) failedValidationResponse(w http.ResponseWriter, r *http.Request, msg string, errs field.ErrorList, k8sCauses []metav1.StatusCause) { + valErrs := make([]ValidationError, len(errs)+len(k8sCauses)) + + // convert field errors to validation errors + for i, err := range errs { + valErrs[i] = ValidationError{ + Type: err.Type, + Field: err.Field, + Message: err.ErrorBody(), + } + } + + // convert k8s causes to validation errors + for i, cause := range k8sCauses { + valErrs[i+len(errs)] = ValidationError{ + Type: field.ErrorType(cause.Type), + Field: cause.Field, + Message: cause.Message, + } } httpError := &HTTPError{ StatusCode: http.StatusUnprocessableEntity, ErrorResponse: ErrorResponse{ Code: strconv.Itoa(http.StatusUnprocessableEntity), - Message: string(message), + Message: msg, + Cause: &ErrorCause{ + ValidationErrors: valErrs, + }, }, } a.errorResponse(w, r, httpError) diff --git a/workspaces/backend/api/response_success.go b/workspaces/backend/api/response_success.go new file mode 100644 index 00000000..0829e688 --- /dev/null +++ b/workspaces/backend/api/response_success.go @@ -0,0 +1,41 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package api + +import "net/http" + +// HTTP: 200 +func (a *App) dataResponse(w http.ResponseWriter, r *http.Request, body any) { + err := a.WriteJSON(w, http.StatusOK, body, nil) + if err != nil { + a.serverErrorResponse(w, r, err) + } +} + +// HTTP: 201 +func (a *App) createdResponse(w http.ResponseWriter, r *http.Request, body any, location string) { + w.Header().Set("Location", location) + err := a.WriteJSON(w, http.StatusCreated, body, nil) + if err != nil { + a.serverErrorResponse(w, r, err) + } +} + +// HTTP: 204 +func (a *App) deletedResponse(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) +} diff --git a/workspaces/backend/api/suite_test.go b/workspaces/backend/api/suite_test.go index 2e947859..faf34962 100644 --- a/workspaces/backend/api/suite_test.go +++ b/workspaces/backend/api/suite_test.go @@ -55,6 +55,8 @@ const ( groupsHeader = "groups-header" adminUser = "notebooks-admin" + + descUnexpectedHTTPStatus = "unexpected HTTP status code, response body: %s" ) var ( @@ -125,13 +127,6 @@ var _ = BeforeSuite(func() { }, })).To(Succeed()) - By("listing the clusterRoles") - clusterRoles := &rbacv1.ClusterRoleList{} - Expect(k8sClient.List(ctx, clusterRoles)).To(Succeed()) - for _, clusterRole := range clusterRoles.Items { - fmt.Printf("ClusterRole: %s\n", clusterRole.Name) - } - By("setting up the controller manager") k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ Scheme: scheme.Scheme, diff --git a/workspaces/backend/api/workspacekinds_handler.go b/workspaces/backend/api/workspacekinds_handler.go index 81439167..14b97995 100644 --- a/workspaces/backend/api/workspacekinds_handler.go +++ b/workspaces/backend/api/workspacekinds_handler.go @@ -18,26 +18,31 @@ package api import ( "errors" - "fmt" "net/http" "github.com/julienschmidt/httprouter" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" "github.com/kubeflow/notebooks/workspaces/backend/internal/auth" + "github.com/kubeflow/notebooks/workspaces/backend/internal/helper" models "github.com/kubeflow/notebooks/workspaces/backend/internal/models/workspacekinds" repository "github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspacekinds" ) -type WorkspaceKindsEnvelope Envelope[[]models.WorkspaceKind] +type WorkspaceKindListEnvelope Envelope[[]models.WorkspaceKind] type WorkspaceKindEnvelope Envelope[models.WorkspaceKind] func (a *App) GetWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - name := ps.ByName("name") - if name == "" { - a.serverErrorResponse(w, r, fmt.Errorf("workspace kind name is missing")) + name := ps.ByName(ResourceNamePathParam) + + // validate path parameters + var valErrs field.ErrorList + valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(ResourceNamePathParam), name)...) + if len(valErrs) > 0 { + a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) return } @@ -65,14 +70,8 @@ func (a *App) GetWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, ps return } - workspaceKindEnvelope := WorkspaceKindEnvelope{ - Data: workspaceKind, - } - - err = a.WriteJSON(w, http.StatusOK, workspaceKindEnvelope, nil) - if err != nil { - a.serverErrorResponse(w, r, err) - } + responseEnvelope := &WorkspaceKindEnvelope{Data: workspaceKind} + a.dataResponse(w, r, responseEnvelope) } func (a *App) GetWorkspaceKindsHandler(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { @@ -94,12 +93,6 @@ func (a *App) GetWorkspaceKindsHandler(w http.ResponseWriter, r *http.Request, _ return } - workspaceKindsEnvelope := WorkspaceKindsEnvelope{ - Data: workspaceKinds, - } - - err = a.WriteJSON(w, http.StatusOK, workspaceKindsEnvelope, nil) - if err != nil { - a.serverErrorResponse(w, r, err) - } + responseEnvelope := &WorkspaceKindListEnvelope{Data: workspaceKinds} + a.dataResponse(w, r, responseEnvelope) } diff --git a/workspaces/backend/api/workspacekinds_handler_test.go b/workspaces/backend/api/workspacekinds_handler_test.go index 03683a62..f37c58c9 100644 --- a/workspaces/backend/api/workspacekinds_handler_test.go +++ b/workspaces/backend/api/workspacekinds_handler_test.go @@ -117,14 +117,14 @@ var _ = Describe("WorkspaceKinds Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) Expect(err).NotTo(HaveOccurred()) - By("unmarshalling the response JSON to WorkspaceKindsEnvelope") - var response WorkspaceKindsEnvelope + By("unmarshalling the response JSON to WorkspaceKindListEnvelope") + var response WorkspaceKindListEnvelope err = json.Unmarshal(body, &response) Expect(err).NotTo(HaveOccurred()) @@ -150,7 +150,7 @@ var _ = Describe("WorkspaceKinds Handler", func() { It("should retrieve a single WorkspaceKind successfully", func() { By("creating the HTTP request") - path := strings.Replace(WorkspaceKindsByNamePath, ":"+WorkspaceKindNamePathParam, workspaceKind1Name, 1) + path := strings.Replace(WorkspaceKindsByNamePath, ":"+ResourceNamePathParam, workspaceKind1Name, 1) req, err := http.NewRequest(http.MethodGet, path, http.NoBody) Expect(err).NotTo(HaveOccurred()) @@ -159,7 +159,7 @@ var _ = Describe("WorkspaceKinds Handler", func() { By("executing GetWorkspaceKindHandler") ps := httprouter.Params{ - httprouter.Param{Key: WorkspaceKindNamePathParam, Value: workspaceKind1Name}, + httprouter.Param{Key: ResourceNamePathParam, Value: workspaceKind1Name}, } rr := httptest.NewRecorder() a.GetWorkspaceKindHandler(rr, req, ps) @@ -167,7 +167,7 @@ var _ = Describe("WorkspaceKinds Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) @@ -215,14 +215,14 @@ var _ = Describe("WorkspaceKinds Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) Expect(err).NotTo(HaveOccurred()) - By("unmarshalling the response JSON to WorkspaceKindsEnvelope") - var response WorkspaceKindsEnvelope + By("unmarshalling the response JSON to WorkspaceKindListEnvelope") + var response WorkspaceKindListEnvelope err = json.Unmarshal(body, &response) Expect(err).NotTo(HaveOccurred()) @@ -234,7 +234,7 @@ var _ = Describe("WorkspaceKinds Handler", func() { missingWorkspaceKindName := "non-existent-workspacekind" By("creating the HTTP request") - path := strings.Replace(WorkspaceKindsByNamePath, ":"+WorkspaceNamePathParam, missingWorkspaceKindName, 1) + path := strings.Replace(WorkspaceKindsByNamePath, ":"+ResourceNamePathParam, missingWorkspaceKindName, 1) req, err := http.NewRequest(http.MethodGet, path, http.NoBody) Expect(err).NotTo(HaveOccurred()) @@ -243,7 +243,7 @@ var _ = Describe("WorkspaceKinds Handler", func() { By("executing GetWorkspaceKindHandler") ps := httprouter.Params{ - httprouter.Param{Key: WorkspaceNamePathParam, Value: missingWorkspaceKindName}, + httprouter.Param{Key: ResourceNamePathParam, Value: missingWorkspaceKindName}, } rr := httptest.NewRecorder() a.GetWorkspaceKindHandler(rr, req, ps) @@ -251,7 +251,7 @@ var _ = Describe("WorkspaceKinds Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusNotFound)) + Expect(rs.StatusCode).To(Equal(http.StatusNotFound), descUnexpectedHTTPStatus, rr.Body.String()) }) }) }) diff --git a/workspaces/backend/api/workspaces_handler.go b/workspaces/backend/api/workspaces_handler.go index 726395ee..889ba009 100644 --- a/workspaces/backend/api/workspaces_handler.go +++ b/workspaces/backend/api/workspaces_handler.go @@ -17,34 +17,38 @@ limitations under the License. package api import ( - "encoding/json" "errors" "fmt" "net/http" "github.com/julienschmidt/httprouter" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" "github.com/kubeflow/notebooks/workspaces/backend/internal/auth" + "github.com/kubeflow/notebooks/workspaces/backend/internal/helper" models "github.com/kubeflow/notebooks/workspaces/backend/internal/models/workspaces" repository "github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspaces" ) -type WorkspacesEnvelope Envelope[[]models.Workspace] +type WorkspaceCreateEnvelope Envelope[*models.WorkspaceCreate] + +type WorkspaceListEnvelope Envelope[[]models.Workspace] type WorkspaceEnvelope Envelope[models.Workspace] func (a *App) GetWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { namespace := ps.ByName(NamespacePathParam) - if namespace == "" { - a.serverErrorResponse(w, r, fmt.Errorf("namespace is nil")) - return - } - - workspaceName := ps.ByName(WorkspaceNamePathParam) - if workspaceName == "" { - a.serverErrorResponse(w, r, fmt.Errorf("workspaceName is nil")) + workspaceName := ps.ByName(ResourceNamePathParam) + + // validate path parameters + var valErrs field.ErrorList + valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(NamespacePathParam), namespace)...) + valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(ResourceNamePathParam), workspaceName)...) + if len(valErrs) > 0 { + a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) return } @@ -75,20 +79,24 @@ func (a *App) GetWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps htt return } - modelRegistryRes := WorkspaceEnvelope{ - Data: workspace, - } - - err = a.WriteJSON(w, http.StatusOK, modelRegistryRes, nil) - if err != nil { - a.serverErrorResponse(w, r, err) - } - + responseEnvelope := &WorkspaceEnvelope{Data: workspace} + a.dataResponse(w, r, responseEnvelope) } func (a *App) GetWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { namespace := ps.ByName(NamespacePathParam) + // validate path parameters + // NOTE: namespace is optional, if not provided, we list all workspaces across all namespaces + var valErrs field.ErrorList + if namespace != "" { + valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(NamespacePathParam), namespace)...) + } + if len(valErrs) > 0 { + a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) + return + } + // =========================== AUTH =========================== authPolicies := []*auth.ResourcePolicy{ auth.NewResourcePolicy( @@ -117,30 +125,48 @@ func (a *App) GetWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps ht return } - modelRegistryRes := WorkspacesEnvelope{ - Data: workspaces, + responseEnvelope := &WorkspaceListEnvelope{Data: workspaces} + a.dataResponse(w, r, responseEnvelope) +} + +func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { + namespace := ps.ByName(NamespacePathParam) + + // validate path parameters + var valErrs field.ErrorList + valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(NamespacePathParam), namespace)...) + if len(valErrs) > 0 { + a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) + return + } + + // validate the Content-Type header + if success := a.ValidateContentType(w, r, "application/json"); !success { + return } - err = a.WriteJSON(w, http.StatusOK, modelRegistryRes, nil) + // decode the request body + bodyEnvelope := &WorkspaceCreateEnvelope{} + err := a.DecodeJSON(r, bodyEnvelope) if err != nil { - a.serverErrorResponse(w, r, err) + a.badRequestResponse(w, r, fmt.Errorf("error decoding request body: %w", err)) + return } -} -func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - namespace := ps.ByName("namespace") - if namespace == "" { - a.serverErrorResponse(w, r, fmt.Errorf("namespace is missing")) + // validate the request body + dataPath := field.NewPath("data") + if bodyEnvelope.Data == nil { + valErrs = field.ErrorList{field.Required(dataPath, "data is required")} + a.failedValidationResponse(w, r, errMsgRequestBodyInvalid, valErrs, nil) return } - - workspaceModel := &models.Workspace{} - if err := json.NewDecoder(r.Body).Decode(workspaceModel); err != nil { - a.serverErrorResponse(w, r, fmt.Errorf("error decoding JSON: %w", err)) + valErrs = bodyEnvelope.Data.Validate(dataPath) + if len(valErrs) > 0 { + a.failedValidationResponse(w, r, errMsgRequestBodyInvalid, valErrs, nil) return } - workspaceModel.Namespace = namespace + workspaceCreate := bodyEnvelope.Data // =========================== AUTH =========================== authPolicies := []*auth.ResourcePolicy{ @@ -149,7 +175,7 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps &kubefloworgv1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{ Namespace: namespace, - Name: workspaceModel.Name, + Name: workspaceCreate.Name, }, }, ), @@ -159,34 +185,38 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps } // ============================================================ - createdWorkspace, err := a.repositories.Workspace.CreateWorkspace(r.Context(), workspaceModel) + createdWorkspace, err := a.repositories.Workspace.CreateWorkspace(r.Context(), workspaceCreate, namespace) if err != nil { + if errors.Is(err, repository.ErrWorkspaceAlreadyExists) { + a.conflictResponse(w, r, err) + return + } + if apierrors.IsInvalid(err) { + causes := helper.StatusCausesFromAPIStatus(err) + a.failedValidationResponse(w, r, errMsgKubernetesValidation, nil, causes) + return + } a.serverErrorResponse(w, r, fmt.Errorf("error creating workspace: %w", err)) return } - // Return created workspace as JSON - workspaceEnvelope := WorkspaceEnvelope{ - Data: createdWorkspace, - } + // calculate the GET location for the created workspace (for the Location header) + location := a.LocationGetWorkspace(namespace, createdWorkspace.Name) - w.Header().Set("Location", r.URL.Path) - err = a.WriteJSON(w, http.StatusCreated, workspaceEnvelope, nil) - if err != nil { - a.serverErrorResponse(w, r, fmt.Errorf("error writing JSON: %w", err)) - } + responseEnvelope := &WorkspaceCreateEnvelope{Data: createdWorkspace} + a.createdResponse(w, r, responseEnvelope, location) } func (a *App) DeleteWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - namespace := ps.ByName("namespace") - if namespace == "" { - a.serverErrorResponse(w, r, fmt.Errorf("namespace is missing")) - return - } - - workspaceName := ps.ByName("name") - if workspaceName == "" { - a.serverErrorResponse(w, r, fmt.Errorf("workspace name is missing")) + namespace := ps.ByName(NamespacePathParam) + workspaceName := ps.ByName(ResourceNamePathParam) + + // validate path parameters + var valErrs field.ErrorList + valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(NamespacePathParam), namespace)...) + valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(ResourceNamePathParam), workspaceName)...) + if len(valErrs) > 0 { + a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) return } @@ -217,5 +247,5 @@ func (a *App) DeleteWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps return } - w.WriteHeader(http.StatusNoContent) + a.deletedResponse(w, r) } diff --git a/workspaces/backend/api/workspaces_handler_test.go b/workspaces/backend/api/workspaces_handler_test.go index fbc80e82..e4fce50e 100644 --- a/workspaces/backend/api/workspaces_handler_test.go +++ b/workspaces/backend/api/workspaces_handler_test.go @@ -172,14 +172,14 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) Expect(err).NotTo(HaveOccurred()) - By("unmarshalling the response JSON to WorkspacesEnvelope") - var response WorkspacesEnvelope + By("unmarshalling the response JSON to WorkspaceListEnvelope") + var response WorkspaceListEnvelope err = json.Unmarshal(body, &response) Expect(err).NotTo(HaveOccurred()) @@ -229,14 +229,14 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) Expect(err).NotTo(HaveOccurred()) - By("unmarshalling the response JSON to WorkspacesEnvelope") - var response WorkspacesEnvelope + By("unmarshalling the response JSON to WorkspaceListEnvelope") + var response WorkspaceListEnvelope err = json.Unmarshal(body, &response) Expect(err).NotTo(HaveOccurred()) @@ -267,7 +267,7 @@ var _ = Describe("Workspaces Handler", 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, ":"+WorkspaceNamePathParam, workspaceName1, 1) + path = strings.Replace(path, ":"+ResourceNamePathParam, workspaceName1, 1) req, err := http.NewRequest(http.MethodGet, path, http.NoBody) Expect(err).NotTo(HaveOccurred()) @@ -277,7 +277,7 @@ var _ = Describe("Workspaces Handler", func() { By("executing GetWorkspaceHandler") ps := httprouter.Params{ httprouter.Param{Key: NamespacePathParam, Value: namespaceName1}, - httprouter.Param{Key: WorkspaceNamePathParam, Value: workspaceName1}, + httprouter.Param{Key: ResourceNamePathParam, Value: workspaceName1}, } rr := httptest.NewRecorder() a.GetWorkspaceHandler(rr, req, ps) @@ -285,7 +285,7 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) @@ -439,14 +439,14 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) Expect(err).NotTo(HaveOccurred()) - By("unmarshalling the response JSON to WorkspacesEnvelope") - var response WorkspacesEnvelope + By("unmarshalling the response JSON to WorkspaceListEnvelope") + var response WorkspaceListEnvelope err = json.Unmarshal(body, &response) Expect(err).NotTo(HaveOccurred()) @@ -492,7 +492,7 @@ var _ = Describe("Workspaces Handler", func() { It("should retrieve a single invalid Workspace successfully", func() { By("creating the HTTP request") path := strings.Replace(WorkspacesByNamePath, ":"+NamespacePathParam, namespaceName1, 1) - path = strings.Replace(path, ":"+WorkspaceNamePathParam, workspaceMissingWskName, 1) + path = strings.Replace(path, ":"+ResourceNamePathParam, workspaceMissingWskName, 1) req, err := http.NewRequest(http.MethodGet, path, http.NoBody) Expect(err).NotTo(HaveOccurred()) @@ -502,7 +502,7 @@ var _ = Describe("Workspaces Handler", func() { By("executing GetWorkspaceHandler") ps := httprouter.Params{ httprouter.Param{Key: NamespacePathParam, Value: namespaceName1}, - httprouter.Param{Key: WorkspaceNamePathParam, Value: workspaceMissingWskName}, + httprouter.Param{Key: ResourceNamePathParam, Value: workspaceMissingWskName}, } rr := httptest.NewRecorder() a.GetWorkspaceHandler(rr, req, ps) @@ -510,7 +510,7 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) @@ -551,14 +551,14 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) Expect(err).NotTo(HaveOccurred()) - By("unmarshalling the response JSON to WorkspacesEnvelope") - var response WorkspacesEnvelope + By("unmarshalling the response JSON to WorkspaceListEnvelope") + var response WorkspaceListEnvelope err = json.Unmarshal(body, &response) Expect(err).NotTo(HaveOccurred()) @@ -587,14 +587,14 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusOK)) + Expect(rs.StatusCode).To(Equal(http.StatusOK), descUnexpectedHTTPStatus, rr.Body.String()) By("reading the HTTP response body") body, err := io.ReadAll(rs.Body) Expect(err).NotTo(HaveOccurred()) - By("unmarshalling the response JSON to WorkspacesEnvelope") - var response WorkspacesEnvelope + By("unmarshalling the response JSON to WorkspaceListEnvelope") + var response WorkspaceListEnvelope err = json.Unmarshal(body, &response) Expect(err).NotTo(HaveOccurred()) @@ -608,7 +608,7 @@ var _ = Describe("Workspaces Handler", func() { By("creating the HTTP request") path := strings.Replace(WorkspacesByNamePath, ":"+NamespacePathParam, missingNamespace, 1) - path = strings.Replace(path, ":"+WorkspaceNamePathParam, missingWorkspaceName, 1) + path = strings.Replace(path, ":"+ResourceNamePathParam, missingWorkspaceName, 1) req, err := http.NewRequest(http.MethodGet, path, http.NoBody) Expect(err).NotTo(HaveOccurred()) @@ -618,7 +618,7 @@ var _ = Describe("Workspaces Handler", func() { By("executing GetWorkspaceHandler") ps := httprouter.Params{ httprouter.Param{Key: NamespacePathParam, Value: missingNamespace}, - httprouter.Param{Key: WorkspaceNamePathParam, Value: missingWorkspaceName}, + httprouter.Param{Key: ResourceNamePathParam, Value: missingWorkspaceName}, } rr := httptest.NewRecorder() a.GetWorkspaceHandler(rr, req, ps) @@ -626,7 +626,7 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusNotFound)) + Expect(rs.StatusCode).To(Equal(http.StatusNotFound), descUnexpectedHTTPStatus, rr.Body.String()) }) }) @@ -688,51 +688,46 @@ var _ = Describe("Workspaces Handler", func() { workspaceKind := &kubefloworgv1beta1.WorkspaceKind{} Expect(k8sClient.Get(ctx, workspaceKindKey, workspaceKind)).To(Succeed()) - By("defining the Workspace to create") - workspace := &kubefloworgv1beta1.Workspace{ - ObjectMeta: metav1.ObjectMeta{ - Name: workspaceName, - Namespace: namespaceNameCrud, - }, - Spec: kubefloworgv1beta1.WorkspaceSpec{ - Kind: workspaceKindName, - Paused: ptr.To(false), - DeferUpdates: ptr.To(false), - PodTemplate: kubefloworgv1beta1.WorkspacePodTemplate{ - PodMetadata: &kubefloworgv1beta1.WorkspacePodMetadata{ - Labels: map[string]string{ - "app": "dora", - }, - Annotations: map[string]string{ - "app": "dora", - }, + By("defining a WorkspaceCreate model") + workspaceCreate := &models.WorkspaceCreate{ + Name: workspaceName, + Kind: workspaceKindName, + Paused: false, + DeferUpdates: false, + PodTemplate: models.PodTemplateMutate{ + PodMetadata: models.PodMetadataMutate{ + Labels: map[string]string{ + "app": "dora", }, - Volumes: kubefloworgv1beta1.WorkspacePodVolumes{ - Home: ptr.To("my-home-pvc"), - Data: []kubefloworgv1beta1.PodVolumeMount{ - { - PVCName: "my-data-pvc", - MountPath: "/data", - ReadOnly: ptr.To(false), - }, - }, + Annotations: map[string]string{ + "app": "dora", }, - Options: kubefloworgv1beta1.WorkspacePodOptions{ - ImageConfig: "jupyterlab_scipy_180", - PodConfig: "tiny_cpu", + }, + Volumes: models.PodVolumesMutate{ + Home: ptr.To("my-home-pvc"), + Data: []models.PodVolumeMount{ + { + PVCName: "my-data-pvc", + MountPath: "/data/1", + ReadOnly: false, + }, }, }, + Options: models.PodTemplateOptionsMutate{ + ImageConfig: "jupyterlab_scipy_180", + PodConfig: "tiny_cpu", + }, }, } - workspaceModel := models.NewWorkspaceModelFromWorkspace(workspace, workspaceKind) + bodyEnvelope := WorkspaceCreateEnvelope{Data: workspaceCreate} - By("marshaling the Workspace model to JSON") - workspaceModelJSON, err := json.Marshal(workspaceModel) + By("marshaling the WorkspaceCreate model to JSON") + bodyEnvelopeJSON, err := json.Marshal(bodyEnvelope) Expect(err).NotTo(HaveOccurred()) By("creating an HTTP request to create the Workspace") path := strings.Replace(WorkspacesByNamespacePath, ":"+NamespacePathParam, namespaceNameCrud, 1) - req, err := http.NewRequest(http.MethodPost, path, strings.NewReader(string(workspaceModelJSON))) + req, err := http.NewRequest(http.MethodPost, path, strings.NewReader(string(bodyEnvelopeJSON))) Expect(err).NotTo(HaveOccurred()) req.Header.Set("Content-Type", "application/json") @@ -752,18 +747,32 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusCreated)) + Expect(rs.StatusCode).To(Equal(http.StatusCreated), descUnexpectedHTTPStatus, rr.Body.String()) By("getting the created Workspace from the Kubernetes API") createdWorkspace := &kubefloworgv1beta1.Workspace{} Expect(k8sClient.Get(ctx, workspaceKey, createdWorkspace)).To(Succeed()) By("ensuring the created Workspace matches the expected Workspace") - Expect(createdWorkspace.Spec).To(BeComparableTo(workspace.Spec)) + Expect(createdWorkspace.ObjectMeta.Name).To(Equal(workspaceName)) + Expect(createdWorkspace.Spec.Kind).To(Equal(workspaceKindName)) + Expect(createdWorkspace.Spec.Paused).To(Equal(&workspaceCreate.Paused)) + Expect(createdWorkspace.Spec.DeferUpdates).To(Equal(&workspaceCreate.DeferUpdates)) + Expect(createdWorkspace.Spec.PodTemplate.PodMetadata.Labels).To(Equal(workspaceCreate.PodTemplate.PodMetadata.Labels)) + Expect(createdWorkspace.Spec.PodTemplate.PodMetadata.Annotations).To(Equal(workspaceCreate.PodTemplate.PodMetadata.Annotations)) + Expect(createdWorkspace.Spec.PodTemplate.Volumes.Home).To(Equal(workspaceCreate.PodTemplate.Volumes.Home)) + expected := []kubefloworgv1beta1.PodVolumeMount{ + { + PVCName: workspaceCreate.PodTemplate.Volumes.Data[0].PVCName, + MountPath: workspaceCreate.PodTemplate.Volumes.Data[0].MountPath, + ReadOnly: &workspaceCreate.PodTemplate.Volumes.Data[0].ReadOnly, + }, + } + Expect(createdWorkspace.Spec.PodTemplate.Volumes.Data).To(Equal(expected)) By("creating an HTTP request to delete the Workspace") path = strings.Replace(WorkspacesByNamePath, ":"+NamespacePathParam, namespaceNameCrud, 1) - path = strings.Replace(path, ":"+WorkspaceNamePathParam, workspaceName, 1) + path = strings.Replace(path, ":"+ResourceNamePathParam, workspaceName, 1) req, err = http.NewRequest(http.MethodDelete, path, http.NoBody) Expect(err).NotTo(HaveOccurred()) @@ -778,7 +787,7 @@ var _ = Describe("Workspaces Handler", func() { Value: namespaceNameCrud, }, httprouter.Param{ - Key: WorkspaceNamePathParam, + Key: ResourceNamePathParam, Value: workspaceName, }, } @@ -787,7 +796,7 @@ var _ = Describe("Workspaces Handler", func() { defer rs.Body.Close() By("verifying the HTTP response status code") - Expect(rs.StatusCode).To(Equal(http.StatusNoContent)) + Expect(rs.StatusCode).To(Equal(http.StatusNoContent), descUnexpectedHTTPStatus, rr.Body.String()) By("ensuring the Workspace has been deleted") deletedWorkspace := &kubefloworgv1beta1.Workspace{} @@ -795,5 +804,9 @@ var _ = Describe("Workspaces Handler", func() { Expect(err).To(HaveOccurred()) Expect(apierrors.IsNotFound(err)).To(BeTrue()) }) + + // TODO: test when fail to create a Workspace when: + // - body payload invalid (missing name/kind, and/or non RCF 1123 name) + // - invalid namespace HTTP path parameter (also test for other API handlers) }) }) diff --git a/workspaces/backend/internal/helper/validation.go b/workspaces/backend/internal/helper/validation.go new file mode 100644 index 00000000..77f22a6d --- /dev/null +++ b/workspaces/backend/internal/helper/validation.go @@ -0,0 +1,94 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package helper + +import ( + "errors" + "strings" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +// StatusCausesFromAPIStatus extracts status causes from a Kubernetes apierrors.APIStatus validation error. +// NOTE: we use this to convert them to our own validation error format. +func StatusCausesFromAPIStatus(err error) []metav1.StatusCause { + // ensure this is an APIStatus error + var statusErr apierrors.APIStatus + if status, ok := err.(apierrors.APIStatus); ok || errors.As(err, &status) { + statusErr = status + } else { + return nil + } + + // only attempt to extract if the status is a validation error + errStatus := statusErr.Status() + if errStatus.Reason != metav1.StatusReasonInvalid { + return nil + } + + return errStatus.Details.Causes +} + +// ValidateFieldIsNotEmpty validates a field is not empty. +func ValidateFieldIsNotEmpty(path *field.Path, value string) field.ErrorList { + var errs field.ErrorList + + if value == "" { + errs = append(errs, field.Required(path, "")) + } + + return errs +} + +// ValidateFieldIsDNS1123Subdomain validates a field contains an RCF 1123 DNS subdomain. +// USED FOR: +// - names of: Workspaces, WorkspaceKinds, Secrets, etc. +func ValidateFieldIsDNS1123Subdomain(path *field.Path, value string) field.ErrorList { + var errs field.ErrorList + + if value == "" { + errs = append(errs, field.Required(path, "")) + } else { + failures := validation.IsDNS1123Subdomain(value) + if len(failures) > 0 { + errs = append(errs, field.Invalid(path, value, strings.Join(failures, "; "))) + } + } + + return errs +} + +// ValidateFieldIsDNS1123Label validates a field contains an RCF 1123 DNS label. +// USED FOR: +// - names of: Namespaces, Services, etc. +func ValidateFieldIsDNS1123Label(path *field.Path, value string) field.ErrorList { + var errs field.ErrorList + + if value == "" { + errs = append(errs, field.Required(path, "")) + } else { + failures := validation.IsDNS1123Label(value) + if len(failures) > 0 { + errs = append(errs, field.Invalid(path, value, strings.Join(failures, "; "))) + } + } + + return errs +} diff --git a/workspaces/backend/internal/models/workspaces/funcs.go b/workspaces/backend/internal/models/workspaces/funcs.go index 55cc5b0a..23288c07 100644 --- a/workspaces/backend/internal/models/workspaces/funcs.go +++ b/workspaces/backend/internal/models/workspaces/funcs.go @@ -82,14 +82,10 @@ func NewWorkspaceModelFromWorkspace(ws *kubefloworgv1beta1.Workspace, wsk *kubef dataVolumes := make([]PodVolumeInfo, len(ws.Spec.PodTemplate.Volumes.Data)) for i := range ws.Spec.PodTemplate.Volumes.Data { volume := ws.Spec.PodTemplate.Volumes.Data[i] - readOnly := false - if volume.ReadOnly != nil { - readOnly = *volume.ReadOnly - } dataVolumes[i] = PodVolumeInfo{ - PvcName: volume.PVCName, + PVCName: volume.PVCName, MountPath: volume.MountPath, - ReadOnly: readOnly, + ReadOnly: ptr.Deref(volume.ReadOnly, false), } } @@ -148,7 +144,7 @@ func buildHomeVolume(ws *kubefloworgv1beta1.Workspace, wsk *kubefloworgv1beta1.W } return &PodVolumeInfo{ - PvcName: *ws.Spec.PodTemplate.Volumes.Home, + PVCName: *ws.Spec.PodTemplate.Volumes.Home, MountPath: homeMountPath, // the home volume is ~always~ read-write ReadOnly: false, diff --git a/workspaces/backend/internal/models/workspaces/funcs_create.go b/workspaces/backend/internal/models/workspaces/funcs_create.go new file mode 100644 index 00000000..ee980c53 --- /dev/null +++ b/workspaces/backend/internal/models/workspaces/funcs_create.go @@ -0,0 +1,69 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package workspaces + +import ( + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "k8s.io/utils/ptr" +) + +// NewWorkspaceCreateModelFromWorkspace creates WorkspaceCreate model from a Workspace object. +func NewWorkspaceCreateModelFromWorkspace(ws *kubefloworgv1beta1.Workspace) *WorkspaceCreate { + podLabels := make(map[string]string) + podAnnotations := make(map[string]string) + if ws.Spec.PodTemplate.PodMetadata != nil { + // NOTE: we copy the maps to avoid creating a reference to the original maps. + for k, v := range ws.Spec.PodTemplate.PodMetadata.Labels { + podLabels[k] = v + } + for k, v := range ws.Spec.PodTemplate.PodMetadata.Annotations { + podAnnotations[k] = v + } + } + + dataVolumes := make([]PodVolumeMount, len(ws.Spec.PodTemplate.Volumes.Data)) + for i, v := range ws.Spec.PodTemplate.Volumes.Data { + dataVolumes[i] = PodVolumeMount{ + PVCName: v.PVCName, + MountPath: v.MountPath, + ReadOnly: ptr.Deref(v.ReadOnly, false), + } + } + + workspaceCreateModel := &WorkspaceCreate{ + Name: ws.Name, + Kind: ws.Spec.Kind, + Paused: ptr.Deref(ws.Spec.Paused, false), + DeferUpdates: ptr.Deref(ws.Spec.DeferUpdates, false), + PodTemplate: PodTemplateMutate{ + PodMetadata: PodMetadataMutate{ + Labels: podLabels, + Annotations: podAnnotations, + }, + Volumes: PodVolumesMutate{ + Home: ws.Spec.PodTemplate.Volumes.Home, + Data: dataVolumes, + }, + Options: PodTemplateOptionsMutate{ + ImageConfig: ws.Spec.PodTemplate.Options.ImageConfig, + PodConfig: ws.Spec.PodTemplate.Options.PodConfig, + }, + }, + } + + return workspaceCreateModel +} diff --git a/workspaces/backend/internal/models/workspaces/types.go b/workspaces/backend/internal/models/workspaces/types.go index e68f8821..b33d8457 100644 --- a/workspaces/backend/internal/models/workspaces/types.go +++ b/workspaces/backend/internal/models/workspaces/types.go @@ -16,6 +16,8 @@ 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 { Name string `json:"name"` Namespace string `json:"namespace"` @@ -68,7 +70,7 @@ type PodVolumes struct { } type PodVolumeInfo struct { - PvcName string `json:"pvc_name"` + PVCName string `json:"pvc_name"` MountPath string `json:"mount_path"` ReadOnly bool `json:"read_only"` } diff --git a/workspaces/backend/internal/models/workspaces/types_create.go b/workspaces/backend/internal/models/workspaces/types_create.go new file mode 100644 index 00000000..bdc60d7a --- /dev/null +++ b/workspaces/backend/internal/models/workspaces/types_create.go @@ -0,0 +1,91 @@ +/* +Copyright 2024. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package workspaces + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/kubeflow/notebooks/workspaces/backend/internal/helper" +) + +// WorkspaceCreate is used to create a new workspace. +type WorkspaceCreate struct { + Name string `json:"name"` + Kind string `json:"kind"` + Paused bool `json:"paused"` + DeferUpdates bool `json:"defer_updates"` + PodTemplate PodTemplateMutate `json:"pod_template"` +} + +type PodTemplateMutate struct { + PodMetadata PodMetadataMutate `json:"pod_metadata"` + Volumes PodVolumesMutate `json:"volumes"` + Options PodTemplateOptionsMutate `json:"options"` +} + +type PodMetadataMutate struct { + Labels map[string]string `json:"labels"` + Annotations map[string]string `json:"annotations"` +} + +type PodVolumesMutate struct { + Home *string `json:"home,omitempty"` + Data []PodVolumeMount `json:"data"` +} + +type PodVolumeMount struct { + PVCName string `json:"pvc_name"` + MountPath string `json:"mount_path"` + ReadOnly bool `json:"read_only,omitempty"` +} + +type PodTemplateOptionsMutate struct { + ImageConfig string `json:"image_config"` + PodConfig string `json:"pod_config"` +} + +// Validate validates the WorkspaceCreate struct. +// NOTE: we only do basic validation, more complex validation is done by the controller when attempting to create the workspace. +func (w *WorkspaceCreate) Validate(prefix *field.Path) []*field.Error { + var errs []*field.Error + + // validate the workspace name + namePath := prefix.Child("name") + errs = append(errs, helper.ValidateFieldIsDNS1123Subdomain(namePath, w.Name)...) + + // validate the workspace kind name + kindPath := prefix.Child("kind") + errs = append(errs, helper.ValidateFieldIsDNS1123Subdomain(kindPath, w.Kind)...) + + // validate the image config + imageConfigPath := prefix.Child("pod_template", "options", "image_config") + errs = append(errs, helper.ValidateFieldIsNotEmpty(imageConfigPath, w.PodTemplate.Options.ImageConfig)...) + + // validate the pod config + podConfigPath := prefix.Child("pod_template", "options", "pod_config") + errs = append(errs, helper.ValidateFieldIsNotEmpty(podConfigPath, w.PodTemplate.Options.PodConfig)...) + + // validate the data volumes + dataVolumesPath := prefix.Child("pod_template", "volumes", "data") + for i, volume := range w.PodTemplate.Volumes.Data { + volumePath := dataVolumesPath.Index(i) + errs = append(errs, helper.ValidateFieldIsNotEmpty(volumePath.Child("pvc_name"), volume.PVCName)...) + errs = append(errs, helper.ValidateFieldIsNotEmpty(volumePath.Child("mount_path"), volume.MountPath)...) + } + + return errs +} diff --git a/workspaces/backend/internal/repositories/namespaces/repo.go b/workspaces/backend/internal/repositories/namespaces/repo.go index 0b390a6a..9cea6321 100644 --- a/workspaces/backend/internal/repositories/namespaces/repo.go +++ b/workspaces/backend/internal/repositories/namespaces/repo.go @@ -36,9 +36,6 @@ func NewNamespaceRepository(cl client.Client) *NamespaceRepository { } func (r *NamespaceRepository) GetNamespaces(ctx context.Context) ([]models.Namespace, error) { - - // TODO(ederign): Implement subject access review here to fetch only - // namespaces that "kubeflow-userid" has access to namespaceList := &v1.NamespaceList{} err := r.client.List(ctx, namespaceList, &client.ListOptions{}) if err != nil { diff --git a/workspaces/backend/internal/repositories/workspacekinds/repo.go b/workspaces/backend/internal/repositories/workspacekinds/repo.go index 386862bd..3796e297 100644 --- a/workspaces/backend/internal/repositories/workspacekinds/repo.go +++ b/workspaces/backend/internal/repositories/workspacekinds/repo.go @@ -55,7 +55,6 @@ func (r *WorkspaceKindRepository) GetWorkspaceKind(ctx context.Context, name str func (r *WorkspaceKindRepository) GetWorkspaceKinds(ctx context.Context) ([]models.WorkspaceKind, error) { workspaceKindList := &kubefloworgv1beta1.WorkspaceKindList{} - err := r.client.List(ctx, workspaceKindList) if err != nil { return nil, err diff --git a/workspaces/backend/internal/repositories/workspaces/repo.go b/workspaces/backend/internal/repositories/workspaces/repo.go index cacfcd00..88134d1a 100644 --- a/workspaces/backend/internal/repositories/workspaces/repo.go +++ b/workspaces/backend/internal/repositories/workspaces/repo.go @@ -30,7 +30,7 @@ import ( ) var ErrWorkspaceNotFound = fmt.Errorf("workspace not found") -var ErrRefWorkspaceKindNotExists = fmt.Errorf("referenced WorkspaceKind does not exist") +var ErrWorkspaceAlreadyExists = fmt.Errorf("workspace already exists") type WorkspaceRepository struct { client client.Client @@ -126,51 +126,41 @@ func (r *WorkspaceRepository) GetAllWorkspaces(ctx context.Context) ([]models.Wo return workspacesModels, nil } -func (r *WorkspaceRepository) CreateWorkspace(ctx context.Context, workspaceModel *models.Workspace) (models.Workspace, error) { - // get workspace kind - // NOTE: if the referenced workspace kind does not exist, - // we throw an error because the api would reject the workspace creation - workspaceKind := &kubefloworgv1beta1.WorkspaceKind{} - workspaceKindName := workspaceModel.WorkspaceKind.Name - if err := r.client.Get(ctx, client.ObjectKey{Name: workspaceKindName}, workspaceKind); err != nil { - if apierrors.IsNotFound(err) { - return models.Workspace{}, fmt.Errorf("%w: %s", ErrRefWorkspaceKindNotExists, workspaceKindName) - } - return models.Workspace{}, err - } - +func (r *WorkspaceRepository) CreateWorkspace(ctx context.Context, workspaceCreate *models.WorkspaceCreate, namespace string) (*models.WorkspaceCreate, error) { // get data volumes from workspace model - dataVolumeMounts := make([]kubefloworgv1beta1.PodVolumeMount, len(workspaceModel.PodTemplate.Volumes.Data)) - for i, dataVolume := range workspaceModel.PodTemplate.Volumes.Data { + dataVolumeMounts := make([]kubefloworgv1beta1.PodVolumeMount, len(workspaceCreate.PodTemplate.Volumes.Data)) + for i, dataVolume := range workspaceCreate.PodTemplate.Volumes.Data { dataVolumeMounts[i] = kubefloworgv1beta1.PodVolumeMount{ - PVCName: dataVolume.PvcName, + PVCName: dataVolume.PVCName, MountPath: dataVolume.MountPath, ReadOnly: ptr.To(dataVolume.ReadOnly), } } // define workspace object from model + workspaceName := workspaceCreate.Name + workspaceKindName := workspaceCreate.Kind workspace := &kubefloworgv1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{ - Name: workspaceModel.Name, - Namespace: workspaceModel.Namespace, + Name: workspaceName, + Namespace: namespace, }, Spec: kubefloworgv1beta1.WorkspaceSpec{ - Paused: &workspaceModel.Paused, - DeferUpdates: &workspaceModel.DeferUpdates, + Paused: &workspaceCreate.Paused, + DeferUpdates: &workspaceCreate.DeferUpdates, Kind: workspaceKindName, PodTemplate: kubefloworgv1beta1.WorkspacePodTemplate{ PodMetadata: &kubefloworgv1beta1.WorkspacePodMetadata{ - Labels: workspaceModel.PodTemplate.PodMetadata.Labels, - Annotations: workspaceModel.PodTemplate.PodMetadata.Annotations, + Labels: workspaceCreate.PodTemplate.PodMetadata.Labels, + Annotations: workspaceCreate.PodTemplate.PodMetadata.Annotations, }, Volumes: kubefloworgv1beta1.WorkspacePodVolumes{ - Home: &workspaceModel.PodTemplate.Volumes.Home.PvcName, + Home: workspaceCreate.PodTemplate.Volumes.Home, Data: dataVolumeMounts, }, Options: kubefloworgv1beta1.WorkspacePodOptions{ - ImageConfig: workspaceModel.PodTemplate.Options.ImageConfig.Current.Id, - PodConfig: workspaceModel.PodTemplate.Options.PodConfig.Current.Id, + ImageConfig: workspaceCreate.PodTemplate.Options.ImageConfig, + PodConfig: workspaceCreate.PodTemplate.Options.PodConfig, }, }, }, @@ -178,11 +168,19 @@ func (r *WorkspaceRepository) CreateWorkspace(ctx context.Context, workspaceMode // create workspace if err := r.client.Create(ctx, workspace); err != nil { - return models.Workspace{}, err + if apierrors.IsAlreadyExists(err) { + return nil, ErrWorkspaceAlreadyExists + } + if apierrors.IsInvalid(err) { + // NOTE: we don't wrap this error so we can unpack it in the caller + // and extract the validation errors returned by the Kubernetes API server + return nil, err + } + return nil, err } - // convert the created workspace to a model - createdWorkspaceModel := models.NewWorkspaceModelFromWorkspace(workspace, workspaceKind) + // convert the created workspace to a WorkspaceCreate model + createdWorkspaceModel := models.NewWorkspaceCreateModelFromWorkspace(workspace) return createdWorkspaceModel, nil }