diff --git a/workspaces/backend/api/app.go b/workspaces/backend/api/app.go index 0500e70f0..c0c85805b 100644 --- a/workspaces/backend/api/app.go +++ b/workspaces/backend/api/app.go @@ -117,6 +117,7 @@ func (a *App) Routes() http.Handler { router.GET(WorkspacesByNamespacePath, a.GetWorkspacesByNamespaceHandler) router.GET(WorkspacesByNamePath, a.GetWorkspaceHandler) router.POST(WorkspacesByNamespacePath, a.CreateWorkspaceHandler) + router.PATCH(WorkspacesByNamePath, a.UpdateWorkspaceHandler) router.DELETE(WorkspacesByNamePath, a.DeleteWorkspaceHandler) router.POST(PauseWorkspacePath, a.PauseActionWorkspaceHandler) diff --git a/workspaces/backend/api/helpers.go b/workspaces/backend/api/helpers.go index 7583ea70b..b63361314 100644 --- a/workspaces/backend/api/helpers.go +++ b/workspaces/backend/api/helpers.go @@ -20,6 +20,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "mime" "net/http" "strings" @@ -62,8 +63,10 @@ func (a *App) DecodeJSON(r *http.Request, v any) error { decoder := json.NewDecoder(r.Body) decoder.DisallowUnknownFields() if err := decoder.Decode(v); err != nil { - // NOTE: we don't wrap this error so we can unpack it in the caller - if a.IsMaxBytesError(err) { + // NOTE: we don't wrap these errors so we can unpack them in the caller + // io.EOF is only returned when the body is completely empty or contains only whitespace. + // If there's any actual JSON content (even malformed), json.Decoder returns different errors. + if a.IsMaxBytesError(err) || a.IsEOFError(err) { return err } return fmt.Errorf("error decoding JSON: %w", err) @@ -77,6 +80,14 @@ func (a *App) IsMaxBytesError(err error) bool { return errors.As(err, &maxBytesError) } +// IsEOFError checks if the error is an EOF error (empty request body). +// This returns true when the request body is completely empty, which happens when: +// - Content-Length is 0, or +// - The body stream ends immediately without any data (io.EOF) +func (a *App) IsEOFError(err error) bool { + return errors.Is(err, io.EOF) +} + // 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. diff --git a/workspaces/backend/api/response_errors.go b/workspaces/backend/api/response_errors.go index b9aadba33..cb77949ed 100644 --- a/workspaces/backend/api/response_errors.go +++ b/workspaces/backend/api/response_errors.go @@ -29,6 +29,7 @@ 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)" + errMsgKubernetesConflict = "kubernetes conflict error (see .cause.conflict_cause[] for details)" ) // ErrorEnvelope is the body of all error responses. @@ -42,19 +43,66 @@ type HTTPError struct { } type ErrorResponse struct { - Code string `json:"code"` - Message string `json:"message"` - Cause *ErrorCause `json:"cause,omitempty"` + // Code is a string representation of the HTTP status code. + Code string `json:"code"` + + // Message is a human-readable description of the error. + Message string `json:"message"` + + // Cause contains detailed information about the cause of the error. + Cause *ErrorCause `json:"cause,omitempty"` } type ErrorCause struct { + // ConflictCauses contains details about conflict errors that caused the request to fail. + ConflictCauses []ConflictError `json:"conflict_cause,omitempty"` + + // ValidationErrors contains details about validation errors that caused the request to fail. ValidationErrors []ValidationError `json:"validation_errors,omitempty"` } +type ErrorCauseOrigin string + +const ( + // OriginInternal indicates the error originated from the internal application logic. + OriginInternal ErrorCauseOrigin = "INTERNAL" + + // OriginKubernetes indicates the error originated from the Kubernetes API server. + OriginKubernetes ErrorCauseOrigin = "KUBERNETES" +) + +type ConflictError struct { + // Origin indicates where the conflict error originated. + // If value is empty, the origin is unknown. + Origin ErrorCauseOrigin `json:"origin,omitempty"` + + // A human-readable description of the cause of the error. + // This field may be presented as-is to a reader. + Message string `json:"message,omitempty"` +} + type ValidationError struct { - Type field.ErrorType `json:"type"` - Field string `json:"field"` - Message string `json:"message"` + // Origin indicates where the validation error originated. + // If value is empty, the origin is unknown. + Origin ErrorCauseOrigin `json:"origin,omitempty"` + + // A machine-readable description of the cause of the error. + // If value is empty, there is no information available. + Type field.ErrorType `json:"type,omitempty"` + + // The field of the resource that has caused this error, as named by its JSON serialization. + // May include dot and postfix notation for nested attributes. + // Arrays are zero-indexed. + // Fields may appear more than once in an array of causes due to fields having multiple errors. + // + // Examples: + // "name" - the field "name" on the current resource + // "items[0].name" - the field "name" on the first array entry in "items" + Field string `json:"field,omitempty"` + + // A human-readable description of the cause of the error. + // This field may be presented as-is to a reader. + Message string `json:"message,omitempty"` } // errorResponse writes an error response to the client. @@ -145,12 +193,34 @@ func (a *App) methodNotAllowedResponse(w http.ResponseWriter, r *http.Request) { } // HTTP: 409 -func (a *App) conflictResponse(w http.ResponseWriter, r *http.Request, err error) { +func (a *App) conflictResponse(w http.ResponseWriter, r *http.Request, err error, k8sCauses []metav1.StatusCause) { + conflictErrs := make([]ConflictError, len(k8sCauses)) + + // convert k8s causes to conflict errors + for i, cause := range k8sCauses { + conflictErrs[i] = ConflictError{ + Origin: OriginKubernetes, + Message: cause.Message, + } + } + + // if we have k8s causes, use a generic message + // otherwise, use the error message + var msg string + if len(conflictErrs) > 0 { + msg = errMsgKubernetesConflict + } else { + msg = err.Error() + } + httpError := &HTTPError{ StatusCode: http.StatusConflict, ErrorResponse: ErrorResponse{ Code: strconv.Itoa(http.StatusConflict), - Message: err.Error(), + Message: msg, + Cause: &ErrorCause{ + ConflictCauses: conflictErrs, + }, }, } a.errorResponse(w, r, httpError) @@ -187,6 +257,7 @@ func (a *App) failedValidationResponse(w http.ResponseWriter, r *http.Request, m // convert field errors to validation errors for i, err := range errs { valErrs[i] = ValidationError{ + Origin: OriginInternal, Type: err.Type, Field: err.Field, Message: err.ErrorBody(), @@ -196,6 +267,7 @@ func (a *App) failedValidationResponse(w http.ResponseWriter, r *http.Request, m // convert k8s causes to validation errors for i, cause := range k8sCauses { valErrs[i+len(errs)] = ValidationError{ + Origin: OriginKubernetes, Type: field.ErrorType(cause.Type), Field: cause.Field, Message: cause.Message, diff --git a/workspaces/backend/api/response_errors_test.go b/workspaces/backend/api/response_errors_test.go new file mode 100644 index 000000000..5bc9db9bb --- /dev/null +++ b/workspaces/backend/api/response_errors_test.go @@ -0,0 +1,276 @@ +/* +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 ( + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +var _ = Describe("Error Response Functions", func() { + + // Shared constants (used across multiple Describe blocks or in BeforeEach) + const ( + testRequestPath = "/test" + testRequestMethod = "GET" + + // Used in both failedValidationResponse and Error response structure + testFieldName = "name" + testNameRequiredMsg = "name is required" + ) + + var ( + app *App + w *httptest.ResponseRecorder + r *http.Request + ) + + BeforeEach(func() { + app = &App{} + w = httptest.NewRecorder() + r = httptest.NewRequest(testRequestMethod, testRequestPath, http.NoBody) + }) + + Describe("conflictResponse", func() { + const ( + httpStatusCodeConflict = http.StatusConflict + + testGroupResourceGroup = "test" + testGroupResourceResource = "tests" + testResourceName = "test-name" + testConflictMsg = "WorkspaceKind is used by 5 workspace(s)" + ) + + var httpStatusCodeConflictStr = strconv.Itoa(http.StatusConflict) + + type testCase struct { + description string + err error + k8sCauses []metav1.StatusCause + } + + conflictErr := apierrors.NewConflict( + schema.GroupResource{Group: testGroupResourceGroup, Resource: testGroupResourceResource}, + testResourceName, + nil, + ) + + testCases := []testCase{ + { + description: "should return 409 with k8s causes", + err: conflictErr, + k8sCauses: []metav1.StatusCause{{Type: metav1.CauseTypeFieldValueInvalid, Message: testConflictMsg}}, + }, + { + description: "should return 409 with error message when no k8s causes", + err: conflictErr, + k8sCauses: nil, + }, + { + description: "should return 409 with empty conflict causes array", + err: conflictErr, + k8sCauses: []metav1.StatusCause{}, + }, + } + + buildExpectedConflictResponse := func(err error, k8sCauses []metav1.StatusCause, codeStr string) ErrorResponse { + expectedConflictCauses := make([]ConflictError, len(k8sCauses)) + for i, cause := range k8sCauses { + expectedConflictCauses[i] = ConflictError{ + Origin: OriginKubernetes, + Message: cause.Message, + } + } + + // Determine message: if we have k8s causes, use generic message; otherwise use error message + var expectedMessage string + if len(k8sCauses) > 0 { + expectedMessage = errMsgKubernetesConflict + } else { + expectedMessage = err.Error() + } + + // Empty slices become nil after JSON unmarshaling + var expectedConflictCausesForComparison []ConflictError + if len(expectedConflictCauses) > 0 { + expectedConflictCausesForComparison = expectedConflictCauses + } else { + expectedConflictCausesForComparison = nil + } + + return ErrorResponse{ + Code: codeStr, + Message: expectedMessage, + Cause: &ErrorCause{ + ConflictCauses: expectedConflictCausesForComparison, + }, + } + } + + for _, tc := range testCases { + It(tc.description, func() { + app.conflictResponse(w, r, tc.err, tc.k8sCauses) + + Expect(w.Code).To(Equal(httpStatusCodeConflict)) + + var envelope ErrorEnvelope + unmarshalErr := json.Unmarshal(w.Body.Bytes(), &envelope) + Expect(unmarshalErr).NotTo(HaveOccurred()) + Expect(envelope.Error).NotTo(BeNil()) + + expectedErrorResponse := buildExpectedConflictResponse(tc.err, tc.k8sCauses, httpStatusCodeConflictStr) + Expect(envelope.Error.ErrorResponse).To(Equal(expectedErrorResponse)) + }) + } + }) + + Describe("failedValidationResponse", func() { + const ( + httpStatusCodeUnprocessableEntity = http.StatusUnprocessableEntity + + testFieldKind = "kind" + testFieldSpecName = "spec.name" + testFieldSpecKind = "spec.kind" + testInvalidKind = "invalid-kind" + testKindInvalidMsg = "kind is invalid" + ) + + var httpStatusCodeUnprocessableEntityStr = strconv.Itoa(http.StatusUnprocessableEntity) + + type testCase struct { + description string + msg string + valErrs field.ErrorList + k8sCauses []metav1.StatusCause + } + + buildExpectedValidationResponse := func(msg string, valErrs field.ErrorList, k8sCauses []metav1.StatusCause, codeStr string) ErrorResponse { + valErrsConverted := make([]ValidationError, len(valErrs)) + for i, err := range valErrs { + valErrsConverted[i] = ValidationError{ + Origin: OriginInternal, + Type: err.Type, + Field: err.Field, + Message: err.ErrorBody(), + } + } + + k8sCausesConverted := make([]ValidationError, len(k8sCauses)) + for i, cause := range k8sCauses { + k8sCausesConverted[i] = ValidationError{ + Origin: OriginKubernetes, + Type: field.ErrorType(cause.Type), + Field: cause.Field, + Message: cause.Message, + } + } + + expectedValidationErrors := make([]ValidationError, len(valErrsConverted)+len(k8sCausesConverted)) + copy(expectedValidationErrors, valErrsConverted) + copy(expectedValidationErrors[len(valErrsConverted):], k8sCausesConverted) + + var expectedValidationErrorsForComparison []ValidationError + if len(expectedValidationErrors) > 0 { + expectedValidationErrorsForComparison = expectedValidationErrors + } else { + expectedValidationErrorsForComparison = nil + } + + return ErrorResponse{ + Code: codeStr, + Message: msg, + Cause: &ErrorCause{ + ValidationErrors: expectedValidationErrorsForComparison, + }, + } + } + + testCases := []testCase{ + { + description: "should return 422 with internal validation errors", + msg: errMsgRequestBodyInvalid, + valErrs: field.ErrorList{ + field.Required(field.NewPath(testFieldName), ""), + field.Invalid(field.NewPath(testFieldKind), testInvalidKind, testKindInvalidMsg), + }, + k8sCauses: nil, + }, + { + description: "should return 422 with k8s validation errors", + msg: errMsgKubernetesValidation, + valErrs: nil, + k8sCauses: []metav1.StatusCause{ + { + Type: metav1.CauseTypeFieldValueRequired, + Field: testFieldSpecName, + Message: testNameRequiredMsg, + }, + { + Type: metav1.CauseTypeFieldValueInvalid, + Field: testFieldSpecKind, + Message: testKindInvalidMsg, + }, + }, + }, + { + description: "should return 422 with both internal and k8s validation errors", + msg: errMsgRequestBodyInvalid, + valErrs: field.ErrorList{ + field.Required(field.NewPath(testFieldName), ""), + }, + k8sCauses: []metav1.StatusCause{ + { + Type: metav1.CauseTypeFieldValueInvalid, + Field: testFieldSpecKind, + Message: testKindInvalidMsg, + }, + }, + }, + { + description: "should return 422 with empty validation errors", + msg: errMsgRequestBodyInvalid, + valErrs: nil, + k8sCauses: nil, + }, + } + + for _, tc := range testCases { + It(tc.description, func() { + app.failedValidationResponse(w, r, tc.msg, tc.valErrs, tc.k8sCauses) + + Expect(w.Code).To(Equal(httpStatusCodeUnprocessableEntity)) + + var envelope ErrorEnvelope + err := json.Unmarshal(w.Body.Bytes(), &envelope) + Expect(err).NotTo(HaveOccurred()) + Expect(envelope.Error).NotTo(BeNil()) + + expectedErrorResponse := buildExpectedValidationResponse(tc.msg, tc.valErrs, tc.k8sCauses, httpStatusCodeUnprocessableEntityStr) + Expect(envelope.Error.ErrorResponse).To(Equal(expectedErrorResponse)) + }) + } + }) +}) diff --git a/workspaces/backend/api/workspace_actions_handler.go b/workspaces/backend/api/workspace_actions_handler.go index 0b1f54fa9..eea6f5d0b 100644 --- a/workspaces/backend/api/workspace_actions_handler.go +++ b/workspaces/backend/api/workspace_actions_handler.go @@ -50,9 +50,10 @@ type WorkspaceActionPauseEnvelope Envelope[*models.WorkspaceActionPause] // @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." +// @Failure 409 {object} ErrorEnvelope "Conflict. Workspace not in valid state for action." // @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. Workspace is not in appropriate state." +// @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}/{workspaceName}/actions/pause [post] func (a *App) PauseActionWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { @@ -60,8 +61,8 @@ func (a *App) PauseActionWorkspaceHandler(w http.ResponseWriter, r *http.Request workspaceName := ps.ByName(ResourceNamePathParam) var valErrs field.ErrorList - valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(NamespacePathParam), namespace)...) - valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(ResourceNamePathParam), workspaceName)...) + valErrs = append(valErrs, helper.ValidateKubernetesNamespaceName(field.NewPath(NamespacePathParam), namespace)...) + valErrs = append(valErrs, helper.ValidateWorkspaceName(field.NewPath(ResourceNamePathParam), workspaceName)...) if len(valErrs) > 0 { a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) return @@ -78,6 +79,17 @@ func (a *App) PauseActionWorkspaceHandler(w http.ResponseWriter, r *http.Request a.requestEntityTooLargeResponse(w, r, err) return } + if a.IsEOFError(err) { + dataPath := field.NewPath("data") + valErrs := field.ErrorList{field.Required(dataPath, "data is required")} + a.failedValidationResponse(w, r, errMsgRequestBodyInvalid, valErrs, nil) + return + } + // + // TODO: handle UnmarshalTypeError and return 422, + // decode the paths which were failed to decode (included in the error) + // and also do this in the other handlers which decode json + // a.badRequestResponse(w, r, fmt.Errorf("error decoding request body: %w", err)) return } @@ -115,7 +127,16 @@ func (a *App) PauseActionWorkspaceHandler(w http.ResponseWriter, r *http.Request return } if errors.Is(err, repository.ErrWorkspaceInvalidState) { - a.failedValidationResponse(w, r, err.Error(), nil, nil) + // ErrWorkspaceInvalidState from pause action always originates from Kubernetes. + // When the patch fails due to a 'test' operation (e.g., trying to pause an already-paused workspace), + // Kubernetes returns an Invalid error but without StatusCauses. We create a StatusCause here + // so conflictResponse will treat it as a Kubernetes error (by passing k8sCauses) and set origin to KUBERNETES. + causes := []metav1.StatusCause{ + { + Message: err.Error(), + }, + } + a.conflictResponse(w, r, err, causes) return } a.serverErrorResponse(w, r, err) diff --git a/workspaces/backend/api/workspace_actions_handler_test.go b/workspaces/backend/api/workspace_actions_handler_test.go index 57b709585..e82666400 100644 --- a/workspaces/backend/api/workspace_actions_handler_test.go +++ b/workspaces/backend/api/workspace_actions_handler_test.go @@ -283,7 +283,7 @@ var _ = Describe("Workspace Actions Handler", func() { Expect(rs.StatusCode).To(Equal(http.StatusNotFound), descUnexpectedHTTPStatus, rr.Body.String()) }) - It("should return 422 when starting a workspace that is not in Paused state", func() { + It("should return 409 when starting a workspace that is not in Paused state", func() { By("setting the workspace's status state to Unknown and spec.paused to false") workspace := &kubefloworgv1beta1.Workspace{} Expect(k8sClient.Get(ctx, workspaceKey1, workspace)).To(Succeed()) @@ -321,11 +321,11 @@ var _ = Describe("Workspace Actions Handler", func() { rs := rr.Result() defer rs.Body.Close() - By("verifying the HTTP response status code is 422") - Expect(rs.StatusCode).To(Equal(http.StatusUnprocessableEntity), descUnexpectedHTTPStatus, rr.Body.String()) + By("verifying the HTTP response status code is 409") + Expect(rs.StatusCode).To(Equal(http.StatusConflict), descUnexpectedHTTPStatus, rr.Body.String()) }) - It("should return 422 when pausing a workspace that is already paused", func() { + It("should return 409 when pausing a workspace that is already paused", func() { By("setting the workspace's spec.paused to true") workspace := &kubefloworgv1beta1.Workspace{} Expect(k8sClient.Get(ctx, workspaceKey1, workspace)).To(Succeed()) @@ -361,8 +361,8 @@ var _ = Describe("Workspace Actions Handler", func() { rs := rr.Result() defer rs.Body.Close() - By("verifying the HTTP response status code is 422") - Expect(rs.StatusCode).To(Equal(http.StatusUnprocessableEntity), descUnexpectedHTTPStatus, rr.Body.String()) + By("verifying the HTTP response status code is 409") + Expect(rs.StatusCode).To(Equal(http.StatusConflict), descUnexpectedHTTPStatus, rr.Body.String()) }) It("should return 422 when request body is missing data field", func() { diff --git a/workspaces/backend/api/workspacekinds_handler.go b/workspaces/backend/api/workspacekinds_handler.go index b995a4002..08829d8fe 100644 --- a/workspaces/backend/api/workspacekinds_handler.go +++ b/workspaces/backend/api/workspacekinds_handler.go @@ -63,7 +63,7 @@ func (a *App) GetWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, ps // validate path parameters var valErrs field.ErrorList - valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(ResourceNamePathParam), name)...) + valErrs = append(valErrs, helper.ValidateWorkspaceKindName(field.NewPath(ResourceNamePathParam), name)...) if len(valErrs) > 0 { a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) return @@ -172,6 +172,16 @@ func (a *App) CreateWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, workspaceKind := &kubefloworgv1beta1.WorkspaceKind{} err = runtime.DecodeInto(a.StrictYamlSerializer, bodyBytes, workspaceKind) if err != nil { + if err, ok := runtime.AsStrictDecodingError(err); ok { + valErrs := field.ErrorList{} + for _, e := range err.Errors() { + if e != nil { + valErrs = append(valErrs, field.Invalid(nil, nil, e.Error())) + } + } + a.failedValidationResponse(w, r, errMsgRequestBodyInvalid, valErrs, nil) + return + } a.badRequestResponse(w, r, fmt.Errorf("error decoding request body: %w", err)) return } @@ -181,8 +191,9 @@ func (a *App) CreateWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, // comprehensive validation will be done by Kubernetes // NOTE: checking the name field is non-empty also verifies that the workspace kind is not nil/empty var valErrs field.ErrorList + valErrs = append(valErrs, helper.ValidateWorkspaceKindGVK(workspaceKind.APIVersion, workspaceKind.Kind)...) wskNamePath := field.NewPath("metadata", "name") - valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(wskNamePath, workspaceKind.Name)...) + valErrs = append(valErrs, helper.ValidateWorkspaceKindName(wskNamePath, workspaceKind.Name)...) if len(valErrs) > 0 { a.failedValidationResponse(w, r, errMsgRequestBodyInvalid, valErrs, nil) return @@ -207,7 +218,8 @@ func (a *App) CreateWorkspaceKindHandler(w http.ResponseWriter, r *http.Request, createdWorkspaceKind, err := a.repositories.WorkspaceKind.Create(r.Context(), workspaceKind) if err != nil { if errors.Is(err, repository.ErrWorkspaceKindAlreadyExists) { - a.conflictResponse(w, r, err) + causes := helper.StatusCausesFromAPIStatus(err) + a.conflictResponse(w, r, err, causes) return } if apierrors.IsInvalid(err) { diff --git a/workspaces/backend/api/workspacekinds_handler_test.go b/workspaces/backend/api/workspacekinds_handler_test.go index a11e47922..515a9d5a5 100644 --- a/workspaces/backend/api/workspacekinds_handler_test.go +++ b/workspaces/backend/api/workspacekinds_handler_test.go @@ -384,6 +384,7 @@ spec: Expect(response.Error.Cause.ValidationErrors).To(BeComparableTo( []ValidationError{ { + Origin: OriginInternal, Type: field.ErrorTypeRequired, Field: "metadata.name", Message: field.ErrorTypeRequired.String(), @@ -502,6 +503,19 @@ metadata: Expect(response.Error.Cause.ValidationErrors).To(BeComparableTo( []ValidationError{ { + Origin: OriginInternal, + Type: field.ErrorTypeRequired, + Field: "apiVersion", + Message: field.ErrorTypeRequired.String(), + }, + { + Origin: OriginInternal, + Type: field.ErrorTypeRequired, + Field: "kind", + Message: field.ErrorTypeRequired.String(), + }, + { + Origin: OriginInternal, Type: field.ErrorTypeRequired, Field: "metadata.name", Message: field.ErrorTypeRequired.String(), diff --git a/workspaces/backend/api/workspaces_handler.go b/workspaces/backend/api/workspaces_handler.go index a03f7c4ad..f672b20ba 100644 --- a/workspaces/backend/api/workspaces_handler.go +++ b/workspaces/backend/api/workspaces_handler.go @@ -33,27 +33,27 @@ import ( repository "github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspaces" ) -type WorkspaceCreateEnvelope Envelope[*models.WorkspaceCreate] +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." -// @Failure 400 {object} ErrorEnvelope "Bad Request. Invalid namespace or workspace name format." +// @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." +// @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}/{workspace_name} [get] func (a *App) GetWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { @@ -62,8 +62,8 @@ func (a *App) GetWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps htt // 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)...) + valErrs = append(valErrs, helper.ValidateKubernetesNamespaceName(field.NewPath(NamespacePathParam), namespace)...) + valErrs = append(valErrs, helper.ValidateWorkspaceName(field.NewPath(ResourceNamePathParam), workspaceName)...) if len(valErrs) > 0 { a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) return @@ -127,9 +127,9 @@ func (a *App) GetAllWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps // @Produce json // @Param namespace path string true "Namespace to filter workspaces" extensions(x-example=kubeflow-user-example-com) // @Success 200 {object} WorkspaceListEnvelope "Successful operation. Returns a list of workspaces in the specified namespace." -// @Failure 400 {object} ErrorEnvelope "Bad Request. Invalid namespace format." // @Failure 401 {object} ErrorEnvelope "Unauthorized. Authentication is required." // @Failure 403 {object} ErrorEnvelope "Forbidden. User does not have permission to list workspaces." +// @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} [get] func (a *App) GetWorkspacesByNamespaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { @@ -144,7 +144,7 @@ func (a *App) getWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps ht // 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)...) + valErrs = append(valErrs, helper.ValidateKubernetesNamespaceName(field.NewPath(NamespacePathParam), namespace)...) } if len(valErrs) > 0 { a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) @@ -167,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()) @@ -193,13 +193,14 @@ 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" -// @Failure 400 {object} ErrorEnvelope "Bad Request. Invalid request body or namespace format." +// @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." // @Failure 409 {object} ErrorEnvelope "Conflict. Workspace with the same name already exists." // @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} [post] func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { @@ -207,7 +208,7 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps // validate path parameters var valErrs field.ErrorList - valErrs = append(valErrs, helper.ValidateFieldIsDNS1123Subdomain(field.NewPath(NamespacePathParam), namespace)...) + valErrs = append(valErrs, helper.ValidateKubernetesNamespaceName(field.NewPath(NamespacePathParam), namespace)...) if len(valErrs) > 0 { a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) return @@ -226,6 +227,17 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps a.requestEntityTooLargeResponse(w, r, err) return } + if a.IsEOFError(err) { + dataPath := field.NewPath("data") + valErrs := field.ErrorList{field.Required(dataPath, "data is required")} + a.failedValidationResponse(w, r, errMsgRequestBodyInvalid, valErrs, nil) + return + } + // + // TODO: handle UnmarshalTypeError and return 422, + // decode the paths which were failed to decode (included in the error) + // and also do this in the other handlers which decode json + // a.badRequestResponse(w, r, fmt.Errorf("error decoding request body: %w", err)) return } @@ -265,7 +277,8 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps createdWorkspace, err := a.repositories.Workspace.CreateWorkspace(r.Context(), workspaceCreate, namespace) if err != nil { if errors.Is(err, repository.ErrWorkspaceAlreadyExists) { - a.conflictResponse(w, r, err) + causes := helper.StatusCausesFromAPIStatus(err) + a.conflictResponse(w, r, err, causes) return } if apierrors.IsInvalid(err) { @@ -284,6 +297,131 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps a.createdResponse(w, r, responseEnvelope, location) } +// UpdateWorkspaceHandler updates an existing workspace. +// +// @Summary Update workspace +// @Description Updates an existing workspace +// @Tags workspaces +// @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 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) + workspaceName := ps.ByName(ResourceNamePathParam) + + // validate path parameters + var valErrs field.ErrorList + valErrs = append(valErrs, helper.ValidateKubernetesNamespaceName(field.NewPath(NamespacePathParam), namespace)...) + valErrs = append(valErrs, helper.ValidateWorkspaceName(field.NewPath(ResourceNamePathParam), workspaceName)...) + 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 + } + + // decode the request body + bodyEnvelope := &WorkspaceEnvelope{} + err := a.DecodeJSON(r, bodyEnvelope) + if err != nil { + if a.IsMaxBytesError(err) { + a.requestEntityTooLargeResponse(w, r, err) + return + } + // + // TODO: handle UnmarshalTypeError and return 422, + // decode the paths which were failed to decode (included in the error) + // and also do this in the other handlers which decode json + // + a.badRequestResponse(w, r, fmt.Errorf("error decoding request body: %w", err)) + return + } + + // 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 + } + valErrs = bodyEnvelope.Data.Validate(dataPath) + if len(valErrs) > 0 { + a.failedValidationResponse(w, r, errMsgRequestBodyInvalid, valErrs, nil) + return + } + + workspaceUpdate := bodyEnvelope.Data + + // =========================== AUTH =========================== + authPolicies := []*auth.ResourcePolicy{ + auth.NewResourcePolicy( + auth.ResourceVerbUpdate, + &kubefloworgv1beta1.Workspace{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: workspaceName, + }, + }, + ), + } + if success := a.requireAuth(w, r, authPolicies); !success { + return + } + // ============================================================ + + updatedWorkspace, err := a.repositories.Workspace.UpdateWorkspace(r.Context(), workspaceUpdate, namespace, workspaceName) + if err != nil { + if errors.Is(err, repository.ErrWorkspaceNotFound) { + 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 + // (or resourceVersion, with the risk of unexpected 500 errors to the caller). + // + if errors.Is(err, repository.ErrWorkspaceGenerationConflict) { + causes := helper.StatusCausesFromAPIStatus(err) + a.conflictResponse(w, r, err, causes) + return + } + if apierrors.IsInvalid(err) { + causes := helper.StatusCausesFromAPIStatus(err) + a.failedValidationResponse(w, r, errMsgKubernetesValidation, nil, causes) + return + } + a.serverErrorResponse(w, r, fmt.Errorf("error updating workspace: %w", err)) + return + } + + responseEnvelope := &WorkspaceEnvelope{Data: updatedWorkspace} + a.dataResponse(w, r, responseEnvelope) +} + // DeleteWorkspaceHandler deletes a specific workspace by namespace and name. // // @Summary Delete workspace @@ -295,10 +433,10 @@ func (a *App) CreateWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps // @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 204 {object} nil "Workspace deleted successfully" -// @Failure 400 {object} ErrorEnvelope "Bad Request. Invalid namespace or workspace name format." // @Failure 401 {object} ErrorEnvelope "Unauthorized. Authentication is required." // @Failure 403 {object} ErrorEnvelope "Forbidden. User does not have permission to delete the workspace." // @Failure 404 {object} ErrorEnvelope "Not Found. Workspace does not exist." +// @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}/{workspace_name} [delete] func (a *App) DeleteWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { @@ -307,8 +445,8 @@ func (a *App) DeleteWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps // 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)...) + valErrs = append(valErrs, helper.ValidateKubernetesNamespaceName(field.NewPath(NamespacePathParam), namespace)...) + valErrs = append(valErrs, helper.ValidateWorkspaceName(field.NewPath(ResourceNamePathParam), workspaceName)...) if len(valErrs) > 0 { a.failedValidationResponse(w, r, errMsgPathParamsInvalid, valErrs, nil) return @@ -336,6 +474,10 @@ func (a *App) DeleteWorkspaceHandler(w http.ResponseWriter, r *http.Request, ps if errors.Is(err, repository.ErrWorkspaceNotFound) { a.notFoundResponse(w, r) return + } else if apierrors.IsConflict(err) { + causes := helper.StatusCausesFromAPIStatus(err) + a.conflictResponse(w, r, err, causes) + return } a.serverErrorResponse(w, r, err) return diff --git a/workspaces/backend/api/workspaces_handler_test.go b/workspaces/backend/api/workspaces_handler_test.go index 91d56b4b5..1c823fe2b 100644 --- a/workspaces/backend/api/workspaces_handler_test.go +++ b/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)) }) }) diff --git a/workspaces/backend/internal/helper/validation.go b/workspaces/backend/internal/helper/validation.go index 77f22a6d0..a764eeed1 100644 --- a/workspaces/backend/internal/helper/validation.go +++ b/workspaces/backend/internal/helper/validation.go @@ -20,14 +20,18 @@ import ( "errors" "strings" + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" + apivalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "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. +// StatusCausesFromAPIStatus extracts status causes from a Kubernetes apierrors.APIStatus error. +// Supports both Invalid and Conflict errors. +// NOTE: we use this to convert them to our own validation/conflict error format. func StatusCausesFromAPIStatus(err error) []metav1.StatusCause { // ensure this is an APIStatus error var statusErr apierrors.APIStatus @@ -37,9 +41,13 @@ func StatusCausesFromAPIStatus(err error) []metav1.StatusCause { return nil } - // only attempt to extract if the status is a validation error + // only attempt to extract if the status is a validation or conflict error errStatus := statusErr.Status() - if errStatus.Reason != metav1.StatusReasonInvalid { + if errStatus.Reason != metav1.StatusReasonInvalid && errStatus.Reason != metav1.StatusReasonConflict { + return nil + } + + if errStatus.Details == nil { return nil } @@ -75,6 +83,50 @@ func ValidateFieldIsDNS1123Subdomain(path *field.Path, value string) field.Error return errs } +// ValidateWorkspaceName validates a field contains a valid Workspace name. +func ValidateWorkspaceName(path *field.Path, value string) field.ErrorList { + return ValidateFieldIsDNS1123Subdomain(path, value) +} + +// ValidateWorkspaceKindName validates a field contains a valid WorkspaceKind name. +func ValidateWorkspaceKindName(path *field.Path, value string) field.ErrorList { + return ValidateFieldIsDNS1123Subdomain(path, value) +} + +// ValidateWorkspaceKindGVK validates the provided apiVersion and kind are for a WorkspaceKind. +func ValidateWorkspaceKindGVK(apiVersion, kind string) field.ErrorList { + var errs field.ErrorList + + // validate apiVersion + apiVersionPath := field.NewPath("apiVersion") + if apiVersion == "" { + errs = append(errs, field.Required(apiVersionPath, "")) + } else { + expectedApiVersion := kubefloworgv1beta1.GroupVersion.String() + if apiVersion != expectedApiVersion { + errs = append(errs, field.Invalid(apiVersionPath, apiVersion, "invalid apiVersion for a WorkspaceKind")) + } + } + + // validate kind + kindPath := field.NewPath("kind") + if kind == "" { + errs = append(errs, field.Required(kindPath, "")) + } else { + expectedKind := "WorkspaceKind" + if kind != expectedKind { + errs = append(errs, field.Invalid(kindPath, kind, "invalid kind for a WorkspaceKind")) + } + } + + return errs +} + +// ValidateKubernetesSecretName validates a field contains a valid Kubernetes Secret name. +func ValidateKubernetesSecretName(path *field.Path, value string) field.ErrorList { + return ValidateFieldIsDNS1123Subdomain(path, value) +} + // ValidateFieldIsDNS1123Label validates a field contains an RCF 1123 DNS label. // USED FOR: // - names of: Namespaces, Services, etc. @@ -92,3 +144,23 @@ func ValidateFieldIsDNS1123Label(path *field.Path, value string) field.ErrorList return errs } + +// ValidateKubernetesNamespaceName validates a field contains a valid Kubernetes Namespace name. +func ValidateKubernetesNamespaceName(path *field.Path, value string) field.ErrorList { + return ValidateFieldIsDNS1123Label(path, value) +} + +// ValidateKubernetesServicesName validates a field contains a valid Kubernetes Service name. +func ValidateKubernetesServicesName(path *field.Path, value string) field.ErrorList { + return ValidateFieldIsDNS1123Label(path, value) +} + +// ValidateKubernetesAnnotations validates a map of Kubernetes annotations. +func ValidateKubernetesAnnotations(path *field.Path, annotations map[string]string) field.ErrorList { + return apivalidation.ValidateAnnotations(annotations, path) +} + +// ValidateKubernetesLabels validates a map of Kubernetes labels. +func ValidateKubernetesLabels(path *field.Path, labels map[string]string) field.ErrorList { + return v1validation.ValidateLabels(labels, path) +} diff --git a/workspaces/backend/internal/helper/validation_test.go b/workspaces/backend/internal/helper/validation_test.go new file mode 100644 index 000000000..4260670e9 --- /dev/null +++ b/workspaces/backend/internal/helper/validation_test.go @@ -0,0 +1,787 @@ +/* +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 ( + "net/http" + "testing" + + kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +func TestValidation(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Validation Helper Suite") +} + +type testError struct { + message string +} + +func (e *testError) Error() string { + return e.message +} + +var _ = Describe("Validation Helper Functions", func() { + + // Centralized test cases for DNS1123 Subdomain validation + // Used by: ValidateWorkspaceName, ValidateWorkspaceKindName, ValidateKubernetesSecretName + var ( + validDNS1123SubdomainNames = []string{ + "a", // single character + "my-resource", // basic name with dash + "resource-123", // name with numbers + "resource-name-with-many-parts", // long name with multiple dashes + "123-resource", // can start with number + "123", // can be all numbers + "resource.name", // dots ARE allowed in DNS1123 subdomain + "resource.name.with.many.parts", // DNS1123 subdomains can have multiple parts separated by dots + } + invalidDNS1123SubdomainNames = []string{ + "", + "Resource-Name", // uppercase not allowed + "-resource", // cannot start with dash + "resource-", // cannot end with dash + "resource_name", // underscores not allowed + "resource/name", // slashes not allowed in DNS1123 subdomain + } + ) + + // Centralized test cases for DNS1123 Label validation + // Used by: ValidateKubernetesNamespaceName, ValidateKubernetesServicesName + // NOTE: Labels are more restrictive than subdomains - they do NOT allow dots + var ( + validDNS1123LabelNames = []string{ + "a", // single character + "my-resource", // basic name with dash + "resource-123", // name with numbers + "resource-name-with-many-parts", // long name with multiple dashes + "123-resource", // can start with number + "123", // can be all numbers + } + invalidDNS1123LabelNames = []string{ + "", + "Resource-Name", // uppercase not allowed + "-resource", // cannot start with dash + "resource-", // cannot end with dash + "resource.name", // dots not allowed in DNS1123 label + "resource_name", // underscores not allowed in DNS1123 label + "resource/name", // slashes not allowed in DNS1123 label + } + ) + + // Centralized test cases for Kubernetes label/annotation keys + // Both labels and annotations share the same key constraints: + // Format: / + // - The prefix (before /) is optional and must be a DNS-1123 subdomain (dots allowed) + // - The name (after /) is required and must be a DNS-1123 label (with slightly relaxed rules) + // Used by: ValidateKubernetesLabels, ValidateKubernetesAnnotations + var ( + validLabelAnnotationKeys = []string{ + "app", // simple lowercase key (no prefix) + "app.kubernetes.io/name", // standard Kubernetes namespaced prefix with slash + "key.with.dots", // dots are allowed in keys (DNS-1123 subdomain format) + "key_with_underscores", // underscores are allowed in keys + "UPPERCASE", // uppercase is allowed in keys + "123numeric", // can start with number + "mixedCase123", // mixed case and numbers + } + invalidLabelAnnotationKeys = []string{ + "", // empty key not allowed + "-invalid", // cannot start with dash + "invalid-", // cannot end with dash + "app.kubernetes.io/", // cannot end with slash (name part after / is required) + } + ) + + Describe("ValidateFieldIsNotEmpty", func() { + const fieldName = "test" + + type testCase struct { + description string + value string + expectError bool + } + + testCases := []testCase{ + { + description: "should return no errors for non-empty string", + value: "non-empty", + expectError: false, + }, + { + description: "should return required error for empty string", + value: "", + expectError: true, + }, + } + + for _, tc := range testCases { + It(tc.description, func() { + path := field.NewPath(fieldName) + errs := ValidateFieldIsNotEmpty(path, tc.value) + if tc.expectError { + Expect(errs).To(HaveLen(1)) + Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired)) + Expect(errs[0].Field).To(Equal(fieldName)) + } else { + Expect(errs).To(BeEmpty()) + } + }) + } + }) + + Describe("ValidateWorkspaceName", func() { + type testCase struct { + description string + name string + shouldPass bool + } + + testCases := []testCase{} + + for _, name := range validDNS1123SubdomainNames { + testCases = append(testCases, testCase{ + description: "should accept valid workspace names", + name: name, + shouldPass: true, + }) + } + + for _, name := range invalidDNS1123SubdomainNames { + testCases = append(testCases, testCase{ + description: "should reject invalid workspace names", + name: name, + shouldPass: false, + }) + } + + const fieldName = "name" + for _, tc := range testCases { + It(tc.description+" - "+tc.name, func() { + path := field.NewPath(fieldName) + errs := ValidateWorkspaceName(path, tc.name) + if tc.shouldPass { + Expect(errs).To(BeEmpty(), "workspace name %q should be valid", tc.name) + } else { + Expect(errs).To(HaveLen(1), "workspace name %q should return exactly one error", tc.name) + if tc.name == "" { + Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired), "workspace name %q should return Required error type for empty string", tc.name) + } else { + Expect(errs[0].Type).To(Equal(field.ErrorTypeInvalid), "workspace name %q should return Invalid error type", tc.name) + Expect(errs[0].BadValue).To(Equal(tc.name), "workspace name %q error BadValue should match input", tc.name) + } + Expect(errs[0].Field).To(Equal(fieldName), "workspace name %q error should be on %q field", tc.name, fieldName) + } + }) + } + }) + + Describe("ValidateWorkspaceKindName", func() { + type testCase struct { + description string + name string + shouldPass bool + } + + testCases := []testCase{} + + for _, name := range validDNS1123SubdomainNames { + testCases = append(testCases, testCase{ + description: "should accept valid workspace kind names", + name: name, + shouldPass: true, + }) + } + + for _, name := range invalidDNS1123SubdomainNames { + testCases = append(testCases, testCase{ + description: "should reject invalid workspace kind names", + name: name, + shouldPass: false, + }) + } + + const fieldName = "kind" + for _, tc := range testCases { + It(tc.description+" - "+tc.name, func() { + path := field.NewPath(fieldName) + errs := ValidateWorkspaceKindName(path, tc.name) + if tc.shouldPass { + Expect(errs).To(BeEmpty(), "workspace kind name %q should be valid", tc.name) + } else { + Expect(errs).To(HaveLen(1), "workspace kind name %q should return exactly one error", tc.name) + if tc.name == "" { + Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired), "workspace kind name %q should return Required error type for empty string", tc.name) + } else { + Expect(errs[0].Type).To(Equal(field.ErrorTypeInvalid), "workspace kind name %q should return Invalid error type", tc.name) + Expect(errs[0].BadValue).To(Equal(tc.name), "workspace kind name %q error BadValue should match input", tc.name) + } + Expect(errs[0].Field).To(Equal(fieldName), "workspace kind name %q error should be on %q field", tc.name, fieldName) + } + }) + } + }) + + Describe("ValidateWorkspaceKindGVK", func() { + validAPIVersion := kubefloworgv1beta1.GroupVersion.String() + const validKind = "WorkspaceKind" + + type testCase struct { + description string + apiVersion string + kind string + validate func(field.ErrorList) + } + + testCases := []testCase{ + { + description: "should accept valid GVK", + apiVersion: validAPIVersion, + kind: validKind, + validate: func(errs field.ErrorList) { + Expect(errs).To(BeEmpty()) + }, + }, + { + description: "should reject empty apiVersion", + apiVersion: "", + kind: validKind, + validate: func(errs field.ErrorList) { + Expect(errs).To(HaveLen(1)) + Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired)) + Expect(errs[0].Field).To(Equal("apiVersion")) + }, + }, + { + description: "should reject empty kind", + apiVersion: validAPIVersion, + kind: "", + validate: func(errs field.ErrorList) { + Expect(errs).To(HaveLen(1)) + Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired)) + Expect(errs[0].Field).To(Equal("kind")) + }, + }, + { + description: "should reject invalid apiVersion", + apiVersion: "invalid/v1", + kind: validKind, + validate: func(errs field.ErrorList) { + Expect(errs).To(HaveLen(1)) + Expect(errs[0].Type).To(Equal(field.ErrorTypeInvalid)) + Expect(errs[0].Field).To(Equal("apiVersion")) + }, + }, + { + description: "should reject invalid kind", + apiVersion: validAPIVersion, + kind: "InvalidKind", + validate: func(errs field.ErrorList) { + Expect(errs).To(HaveLen(1)) + Expect(errs[0].Type).To(Equal(field.ErrorTypeInvalid)) + Expect(errs[0].Field).To(Equal("kind")) + }, + }, + { + description: "should return multiple errors for both invalid apiVersion and kind", + apiVersion: "invalid/v1", + kind: "InvalidKind", + validate: func(errs field.ErrorList) { + Expect(errs).To(HaveLen(2)) + + errorsByField := make(map[string]*field.Error) + for i := range errs { + errorsByField[errs[i].Field] = errs[i] + } + + apiVersionErr, ok := errorsByField["apiVersion"] + Expect(ok).To(BeTrue(), "should have apiVersion error") + Expect(apiVersionErr.Type).To(Equal(field.ErrorTypeInvalid)) + Expect(apiVersionErr.BadValue).To(Equal("invalid/v1")) + Expect(apiVersionErr.ErrorBody()).To(ContainSubstring("invalid apiVersion for a WorkspaceKind")) + + kindErr, ok := errorsByField["kind"] + Expect(ok).To(BeTrue(), "should have kind error") + Expect(kindErr.Type).To(Equal(field.ErrorTypeInvalid)) + Expect(kindErr.BadValue).To(Equal("InvalidKind")) + Expect(kindErr.ErrorBody()).To(ContainSubstring("invalid kind for a WorkspaceKind")) + }, + }, + } + + for _, tc := range testCases { + It(tc.description, func() { + errs := ValidateWorkspaceKindGVK(tc.apiVersion, tc.kind) + tc.validate(errs) + }) + } + }) + + Describe("ValidateKubernetesNamespaceName", func() { + type testCase struct { + description string + name string + shouldPass bool + } + + testCases := []testCase{} + + for _, name := range validDNS1123LabelNames { + testCases = append(testCases, testCase{ + description: "should accept valid namespace names", + name: name, + shouldPass: true, + }) + } + + for _, name := range invalidDNS1123LabelNames { + testCases = append(testCases, testCase{ + description: "should reject invalid namespace names", + name: name, + shouldPass: false, + }) + } + + const fieldName = "namespace" + for _, tc := range testCases { + It(tc.description+" - "+tc.name, func() { + path := field.NewPath(fieldName) + errs := ValidateKubernetesNamespaceName(path, tc.name) + if tc.shouldPass { + Expect(errs).To(BeEmpty(), "namespace name %q should be valid", tc.name) + } else { + Expect(errs).To(HaveLen(1), "namespace name %q should return exactly one error", tc.name) + if tc.name == "" { + Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired), "namespace name %q should return Required error type for empty string", tc.name) + } else { + Expect(errs[0].Type).To(Equal(field.ErrorTypeInvalid), "namespace name %q should return Invalid error type", tc.name) + Expect(errs[0].BadValue).To(Equal(tc.name), "namespace name %q error BadValue should match input", tc.name) + } + Expect(errs[0].Field).To(Equal(fieldName), "namespace name %q error should be on %q field", tc.name, fieldName) + } + }) + } + }) + + Describe("ValidateKubernetesServicesName", func() { + type testCase struct { + description string + name string + shouldPass bool + } + + testCases := []testCase{} + + for _, name := range validDNS1123LabelNames { + testCases = append(testCases, testCase{ + description: "should accept valid service names", + name: name, + shouldPass: true, + }) + } + + for _, name := range invalidDNS1123LabelNames { + testCases = append(testCases, testCase{ + description: "should reject invalid service names", + name: name, + shouldPass: false, + }) + } + + const fieldName = "service" + for _, tc := range testCases { + It(tc.description+" - "+tc.name, func() { + path := field.NewPath(fieldName) + errs := ValidateKubernetesServicesName(path, tc.name) + if tc.shouldPass { + Expect(errs).To(BeEmpty(), "service name %q should be valid", tc.name) + } else { + Expect(errs).To(HaveLen(1), "service name %q should return exactly one error", tc.name) + if tc.name == "" { + Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired), "service name %q should return Required error type for empty string", tc.name) + } else { + Expect(errs[0].Type).To(Equal(field.ErrorTypeInvalid), "service name %q should return Invalid error type", tc.name) + Expect(errs[0].BadValue).To(Equal(tc.name), "service name %q error BadValue should match input", tc.name) + } + Expect(errs[0].Field).To(Equal(fieldName), "service name %q error should be on %q field", tc.name, fieldName) + } + }) + } + }) + + Describe("ValidateKubernetesSecretName", func() { + type testCase struct { + description string + name string + shouldPass bool + } + + testCases := []testCase{} + + for _, name := range validDNS1123SubdomainNames { + testCases = append(testCases, testCase{ + description: "should accept valid secret names", + name: name, + shouldPass: true, + }) + } + + for _, name := range invalidDNS1123SubdomainNames { + testCases = append(testCases, testCase{ + description: "should reject invalid secret names", + name: name, + shouldPass: false, + }) + } + + const fieldName = "secret" + for _, tc := range testCases { + It(tc.description+" - "+tc.name, func() { + path := field.NewPath(fieldName) + errs := ValidateKubernetesSecretName(path, tc.name) + if tc.shouldPass { + Expect(errs).To(BeEmpty(), "secret name %q should be valid", tc.name) + } else { + Expect(errs).To(HaveLen(1), "secret name %q should return exactly one error", tc.name) + if tc.name == "" { + Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired), "secret name %q should return Required error type for empty string", tc.name) + } else { + Expect(errs[0].Type).To(Equal(field.ErrorTypeInvalid), "secret name %q should return Invalid error type", tc.name) + Expect(errs[0].BadValue).To(Equal(tc.name), "secret name %q error BadValue should match input", tc.name) + } + Expect(errs[0].Field).To(Equal(fieldName), "secret name %q error should be on %q field", tc.name, fieldName) + } + }) + } + }) + + Describe("ValidateKubernetesLabels", func() { + var validLabelValues = []string{ + "", // empty value is allowed + "my-app", // simple lowercase value with dash + "value-with-dashes", // dashes are allowed in values + "value_with_underscores", // underscores are allowed in values + "value.with.dots", // dots are allowed in values + "UPPERCASE", // uppercase is allowed in values + "123numeric", // can start with number + "mixedCase123", // mixed case and numbers + "1.0.0", // version-like format with dots + } + + var invalidLabelValues = []string{ + "value\nwith\nnewlines", // newlines not allowed in label values + "value with spaces", // spaces not allowed in label values + "value/with/slashes", // slashes not allowed in label values + "value\twith\ttabs", // tabs not allowed in label values + } + + const validKey = "valid-key" + const validValue = "valid-value" + + type testCase struct { + description string + labels map[string]string + shouldPass bool + } + + testCases := []testCase{} + + testCases = append(testCases, testCase{ + description: "should accept empty labels map", + labels: map[string]string{}, + shouldPass: true, + }) + + for _, key := range validLabelAnnotationKeys { + testCases = append(testCases, testCase{ + description: "should accept valid label keys", + labels: map[string]string{key: validValue}, + shouldPass: true, + }) + } + + for _, value := range validLabelValues { + testCases = append(testCases, testCase{ + description: "should accept valid label values", + labels: map[string]string{validKey: value}, + shouldPass: true, + }) + } + + for _, key := range invalidLabelAnnotationKeys { + testCases = append(testCases, testCase{ + description: "should reject invalid label keys", + labels: map[string]string{key: validValue}, + shouldPass: false, + }) + } + + for _, value := range invalidLabelValues { + testCases = append(testCases, testCase{ + description: "should reject invalid label values", + labels: map[string]string{validKey: value}, + shouldPass: false, + }) + } + + const fieldName = "labels" + for _, tc := range testCases { + It(tc.description, func() { + path := field.NewPath(fieldName) + errs := ValidateKubernetesLabels(path, tc.labels) + if tc.shouldPass { + Expect(errs).To(BeEmpty(), "labels %v should be valid", tc.labels) + } else { + Expect(errs).NotTo(BeEmpty(), "labels %v should be invalid", tc.labels) + for i := range errs { + Expect(errs[i].Type).To(Equal(field.ErrorTypeInvalid), "labels %v error[%d] should be Invalid type", tc.labels, i) + Expect(errs[i].Field).To(HavePrefix(fieldName), "labels %v error[%d] field should start with %q", tc.labels, i, fieldName) + } + } + }) + } + }) + + Describe("ValidateKubernetesAnnotations", func() { + var validAnnotationValues = []string{ + "", // empty value is allowed + "my-app", // simple lowercase value + "value-with-dashes", // dashes are allowed + "value_with_underscores", // underscores are allowed + "value.with.dots", // dots are allowed + "UPPERCASE", // uppercase is allowed + "123numeric", // can start with number + "value with spaces", // spaces ARE allowed in annotation values (unlike labels) + "value\nwith\nnewlines", // newlines ARE allowed in annotation values (unlike labels) + "value/with/slashes", // slashes ARE allowed in annotation values (unlike labels) + "value\twith\ttabs", // tabs ARE allowed in annotation values (unlike labels) + "JSON-like: {\"key\": \"value\"}", // complex structured data is allowed + } + + // NOTE: There are no invalid annotation values worth testing individually. + // Annotations are extremely permissive - they allow almost any character including + // spaces, newlines, tabs, slashes, and special characters. The only restriction + // is that the total size of all annotations on an object cannot exceed ~256KB + // (262135 bytes). + + const validKey = "valid-key" + const validValue = "valid-value" + + type testCase struct { + description string + annotations map[string]string + shouldPass bool + } + + testCases := []testCase{} + + testCases = append(testCases, testCase{ + description: "should accept empty annotations map", + annotations: map[string]string{}, + shouldPass: true, + }) + + for _, key := range validLabelAnnotationKeys { + testCases = append(testCases, testCase{ + description: "should accept valid annotation keys", + annotations: map[string]string{key: validValue}, + shouldPass: true, + }) + } + + for _, value := range validAnnotationValues { + testCases = append(testCases, testCase{ + description: "should accept valid annotation values", + annotations: map[string]string{validKey: value}, + shouldPass: true, + }) + } + + for _, key := range invalidLabelAnnotationKeys { + testCases = append(testCases, testCase{ + description: "should reject invalid annotation keys", + annotations: map[string]string{key: validValue}, + shouldPass: false, + }) + } + + const fieldName = "annotations" + for _, tc := range testCases { + It(tc.description, func() { + path := field.NewPath(fieldName) + errs := ValidateKubernetesAnnotations(path, tc.annotations) + if tc.shouldPass { + Expect(errs).To(BeEmpty(), "annotations %v should be valid", tc.annotations) + } else { + Expect(errs).NotTo(BeEmpty(), "annotations %v should be invalid", tc.annotations) + for i := range errs { + Expect(errs[i].Type).To(Equal(field.ErrorTypeInvalid), "annotations %v error[%d] should be Invalid type", tc.annotations, i) + Expect(errs[i].Field).To(HavePrefix(fieldName), "annotations %v error[%d] field should start with %q", tc.annotations, i, fieldName) + } + } + }) + } + }) + + Describe("StatusCausesFromAPIStatus", func() { + // Test message constants + const ( + field1RequiredMsg = "field1 is required" + field2InvalidMsg = "field2 is invalid" + workspaceKindInUseMsg = "WorkspaceKind is used by 5 workspace(s)" + operationCannotBeFulfilledMsg = "Operation cannot be fulfilled" + ) + + // Helper validation functions for common test cases + validateNil := func(causes []metav1.StatusCause) { + Expect(causes).To(BeNil()) + } + validateEmpty := func(causes []metav1.StatusCause) { + Expect(causes).To(BeEmpty()) + } + + type testCase struct { + description string + err error + validate func([]metav1.StatusCause) + } + + testCases := []testCase{ + { + description: "should extract causes from Invalid error", + err: apierrors.NewInvalid( + schema.GroupKind{Group: "test", Kind: "Test"}, + "test-name", + field.ErrorList{ + field.Required(field.NewPath("spec", "field1"), field1RequiredMsg), + field.Invalid(field.NewPath("spec", "field2"), "value", field2InvalidMsg), + }, + ), + validate: func(causes []metav1.StatusCause) { + Expect(causes).To(HaveLen(2)) + Expect(causes[0].Field).To(Equal("spec.field1")) + Expect(causes[0].Message).To(ContainSubstring(field1RequiredMsg)) + Expect(causes[1].Field).To(Equal("spec.field2")) + Expect(causes[1].Message).To(ContainSubstring(field2InvalidMsg)) + }, + }, + { + description: "should extract causes from Conflict error (NewApplyConflict)", + err: apierrors.NewApplyConflict( + []metav1.StatusCause{ + { + Type: metav1.CauseTypeFieldValueInvalid, + Message: workspaceKindInUseMsg, + }, + }, + operationCannotBeFulfilledMsg, + ), + validate: func(causes []metav1.StatusCause) { + Expect(causes).To(HaveLen(1)) + Expect(causes[0].Message).To(Equal(workspaceKindInUseMsg)) + }, + }, + { + description: "should extract causes from Conflict error created like webhook (StatusError)", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusConflict, + Reason: metav1.StatusReasonConflict, + Details: &metav1.StatusDetails{ + Group: "kubeflow.org", + Kind: "WorkspaceKind", + Name: "test-workspacekind", + Causes: []metav1.StatusCause{ + { + Message: workspaceKindInUseMsg, + }, + }, + }, + Message: "Operation cannot be fulfilled on WorkspaceKind \"test-workspacekind\"", + }, + }, + validate: func(causes []metav1.StatusCause) { + Expect(causes).To(HaveLen(1)) + Expect(causes[0].Message).To(Equal(workspaceKindInUseMsg)) + }, + }, + { + description: "should return empty for Conflict error without causes", + err: apierrors.NewConflict( + schema.GroupResource{Group: "test", Resource: "tests"}, + "test-name", + nil, + ), + validate: validateEmpty, + }, + { + description: "should return nil for non-APIStatus error", + err: &testError{message: "not an APIStatus error"}, + validate: validateNil, + }, + { + description: "should return nil for APIStatus error with different reason", + err: apierrors.NewNotFound( + schema.GroupResource{Group: "test", Resource: "tests"}, + "test-name", + ), + validate: validateNil, + }, + { + description: "should handle error with nil Details gracefully", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: 422, + Reason: metav1.StatusReasonInvalid, + Details: nil, + }, + }, + validate: validateNil, + }, + { + description: "should handle error with empty Causes", + err: &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: 422, + Reason: metav1.StatusReasonInvalid, + Details: &metav1.StatusDetails{ + Causes: []metav1.StatusCause{}, + }, + }, + }, + validate: validateEmpty, + }, + } + + for _, tc := range testCases { + It(tc.description, func() { + causes := StatusCausesFromAPIStatus(tc.err) + tc.validate(causes) + }) + } + }) +}) diff --git a/workspaces/backend/internal/models/workspaces/funcs.go b/workspaces/backend/internal/models/workspaces/funcs.go index c4cf3caa6..8a51d3b63 100644 --- a/workspaces/backend/internal/models/workspaces/funcs.go +++ b/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{ diff --git a/workspaces/backend/internal/models/workspaces/funcs_create.go b/workspaces/backend/internal/models/workspaces/funcs_create.go deleted file mode 100644 index ee980c534..000000000 --- a/workspaces/backend/internal/models/workspaces/funcs_create.go +++ /dev/null @@ -1,69 +0,0 @@ -/* -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/funcs_write.go b/workspaces/backend/internal/models/workspaces/funcs_write.go new file mode 100644 index 000000000..501daef1b --- /dev/null +++ b/workspaces/backend/internal/models/workspaces/funcs_write.go @@ -0,0 +1,137 @@ +/* +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 ( + "strconv" + + 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), + } + } + + 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, + 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, + Secrets: secretMounts, + }, + Options: PodTemplateOptionsMutate{ + ImageConfig: ws.Spec.PodTemplate.Options.ImageConfig, + PodConfig: ws.Spec.PodTemplate.Options.PodConfig, + }, + }, + } + + return workspaceCreateModel +} + +// NewWorkspaceUpdateModelFromWorkspace creates WorkspaceUpdate model from a Workspace object. +func NewWorkspaceUpdateModelFromWorkspace(ws *kubefloworgv1beta1.Workspace) *WorkspaceUpdate { + 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), + } + } + + 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{ + PodMetadata: PodMetadataMutate{ + Labels: podLabels, + Annotations: podAnnotations, + }, + Volumes: PodVolumesMutate{ + Home: ws.Spec.PodTemplate.Volumes.Home, + Data: dataVolumes, + Secrets: secretMounts, + }, + Options: PodTemplateOptionsMutate{ + ImageConfig: ws.Spec.PodTemplate.Options.ImageConfig, + PodConfig: ws.Spec.PodTemplate.Options.PodConfig, + }, + }, + } + + return workspaceUpdateModel +} diff --git a/workspaces/backend/internal/models/workspaces/types.go b/workspaces/backend/internal/models/workspaces/types.go index 4e8ddaecd..80d3cbe1d 100644 --- a/workspaces/backend/internal/models/workspaces/types.go +++ b/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"` diff --git a/workspaces/backend/internal/models/workspaces/types_create.go b/workspaces/backend/internal/models/workspaces/types_create.go deleted file mode 100644 index 65585f3ce..000000000 --- a/workspaces/backend/internal/models/workspaces/types_create.go +++ /dev/null @@ -1,111 +0,0 @@ -/* -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:"deferUpdates"` - PodTemplate PodTemplateMutate `json:"podTemplate"` -} - -type PodTemplateMutate struct { - PodMetadata PodMetadataMutate `json:"podMetadata"` - 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"` - Secrets []PodSecretMount `json:"secrets,omitempty"` -} - -type PodVolumeMount struct { - PVCName string `json:"pvcName"` - MountPath string `json:"mountPath"` - ReadOnly bool `json:"readOnly,omitempty"` -} - -type PodSecretMount struct { - SecretName string `json:"secretName"` - MountPath string `json:"mountPath"` - DefaultMode int32 `json:"defaultMode,omitempty"` -} - -type PodTemplateOptionsMutate struct { - ImageConfig string `json:"imageConfig"` - PodConfig string `json:"podConfig"` -} - -// 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("podTemplate", "options", "imageConfig") - errs = append(errs, helper.ValidateFieldIsNotEmpty(imageConfigPath, w.PodTemplate.Options.ImageConfig)...) - - // validate the pod config - podConfigPath := prefix.Child("podTemplate", "options", "podConfig") - errs = append(errs, helper.ValidateFieldIsNotEmpty(podConfigPath, w.PodTemplate.Options.PodConfig)...) - - // validate the data volumes - dataVolumesPath := prefix.Child("podTemplate", "volumes", "data") - for i, volume := range w.PodTemplate.Volumes.Data { - volumePath := dataVolumesPath.Index(i) - errs = append(errs, helper.ValidateFieldIsNotEmpty(volumePath.Child("pvcName"), volume.PVCName)...) - errs = append(errs, helper.ValidateFieldIsNotEmpty(volumePath.Child("mountPath"), volume.MountPath)...) - } - - // validate the secrets - secretsPath := prefix.Child("podTemplate", "volumes", "secrets") - for i, secret := range w.PodTemplate.Volumes.Secrets { - secretPath := secretsPath.Index(i) - errs = append(errs, helper.ValidateFieldIsNotEmpty(secretPath.Child("secretName"), secret.SecretName)...) - errs = append(errs, helper.ValidateFieldIsNotEmpty(secretPath.Child("mountPath"), secret.MountPath)...) - if secret.DefaultMode != 0 { - if secret.DefaultMode < 0 || secret.DefaultMode > 511 { - errs = append(errs, field.Invalid(secretPath.Child("defaultMode"), secret.DefaultMode, "defaultMode must be between 0 and 511")) - } - } - } - - return errs -} diff --git a/workspaces/backend/internal/models/workspaces/types_write.go b/workspaces/backend/internal/models/workspaces/types_write.go new file mode 100644 index 000000000..c3059631e --- /dev/null +++ b/workspaces/backend/internal/models/workspaces/types_write.go @@ -0,0 +1,228 @@ +/* +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:"deferUpdates"` + PodTemplate PodTemplateMutate `json:"podTemplate"` +} + +// 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.ValidateWorkspaceName(namePath, w.Name)...) + + // validate the workspace kind name + kindPath := prefix.Child("kind") + errs = append(errs, helper.ValidateWorkspaceKindName(kindPath, w.Kind)...) + + // validate pod template + podTemplatePath := prefix.Child("podTemplate") + errs = append(errs, w.PodTemplate.Validate(podTemplatePath)...) + + return errs +} + +// WorkspaceUpdate is used to update an existing workspace. +// NOTE: we only do basic validation, more complex validation is done by the controller when attempting to update the workspace. +type WorkspaceUpdate struct { + // Revision is an opaque token for optimistic locking. + // Clients receive this value from GET requests and must include it + // in PATCH requests to ensure they are updating the expected version. + // Clients must not parse, interpret, or compare revision values. + Revision string `json:"revision"` + + Paused bool `json:"paused"` // TODO: remove `paused` once we have an "actions" api for pausing workspaces + DeferUpdates bool `json:"deferUpdates"` // TODO: remove `deferUpdates` once the controller is no longer applying redirects + PodTemplate PodTemplateMutate `json:"podTemplate"` +} + +// Validate validates the WorkspaceUpdate struct. +func (w *WorkspaceUpdate) Validate(prefix *field.Path) []*field.Error { + var errs []*field.Error + + // Validate revision is present + revisionPath := prefix.Child("revision") + if w.Revision == "" { + errs = append(errs, field.Required(revisionPath, "revision is required")) + } + + // validate pod template + podTemplatePath := prefix.Child("podTemplate") + errs = append(errs, w.PodTemplate.Validate(podTemplatePath)...) + + return errs +} + +type PodTemplateMutate struct { + PodMetadata PodMetadataMutate `json:"podMetadata"` + Volumes PodVolumesMutate `json:"volumes"` + Options PodTemplateOptionsMutate `json:"options"` +} + +// Validate validates the PodTemplateMutate struct. +func (p *PodTemplateMutate) Validate(prefix *field.Path) []*field.Error { + var errs []*field.Error + + // validate the pod metadata + podMetadataPath := prefix.Child("podMetadata") + errs = append(errs, p.PodMetadata.Validate(podMetadataPath)...) + + // validate the volumes + volumesPath := prefix.Child("volumes") + errs = append(errs, p.Volumes.Validate(volumesPath)...) + + // validate the options + optionsPath := prefix.Child("options") + errs = append(errs, p.Options.Validate(optionsPath)...) + + return errs +} + +// TODO: figure out how we want to handle labels/annotations +// - we do not want users to be able to set labels/annotations +// - but we probably want them to be returned in the response +type PodMetadataMutate struct { + Labels map[string]string `json:"labels"` + Annotations map[string]string `json:"annotations"` +} + +// Validate validates the PodMetadataMutate struct. +func (p *PodMetadataMutate) Validate(prefix *field.Path) []*field.Error { + var errs []*field.Error + + // validate the labels + labelsPath := prefix.Child("labels") + errs = append(errs, helper.ValidateKubernetesLabels(labelsPath, p.Labels)...) + + // validate the annotations + annotationsPath := prefix.Child("annotations") + errs = append(errs, helper.ValidateKubernetesAnnotations(annotationsPath, p.Annotations)...) + + return errs +} + +type PodVolumesMutate struct { + Home *string `json:"home,omitempty"` + Data []PodVolumeMount `json:"data"` + Secrets []PodSecretMount `json:"secrets,omitempty"` +} + +// Validate validates the PodVolumesMutate struct. +func (p *PodVolumesMutate) Validate(prefix *field.Path) []*field.Error { + var errs []*field.Error + + // validate the data volumes + dataVolumesPath := prefix.Child("data") + for i, volume := range p.Data { + volumePath := dataVolumesPath.Index(i) + errs = append(errs, volume.Validate(volumePath)...) + } + + // validate the secrets + secretsPath := prefix.Child("secrets") + for i, secret := range p.Secrets { + secretPath := secretsPath.Index(i) + errs = append(errs, secret.Validate(secretPath)...) + } + + return errs +} + +type PodVolumeMount struct { + PVCName string `json:"pvcName"` + MountPath string `json:"mountPath"` + ReadOnly bool `json:"readOnly,omitempty"` +} + +// Validate validates the PodVolumeMount struct. +func (p *PodVolumeMount) Validate(prefix *field.Path) []*field.Error { + var errs []*field.Error + + // validate the PVC name + pvcNamePath := prefix.Child("pvcName") + errs = append(errs, helper.ValidateFieldIsNotEmpty(pvcNamePath, p.PVCName)...) + + // validate the mount path + mountPath := prefix.Child("mountPath") + errs = append(errs, helper.ValidateFieldIsNotEmpty(mountPath, p.MountPath)...) + + return errs +} + +type PodSecretMount struct { + SecretName string `json:"secretName"` + MountPath string `json:"mountPath"` + DefaultMode int32 `json:"defaultMode,omitempty"` +} + +// Validate validates the PodSecretMount struct. +func (p *PodSecretMount) Validate(prefix *field.Path) []*field.Error { + var errs []*field.Error + + // validate the secret name + secretNamePath := prefix.Child("secretName") + errs = append(errs, helper.ValidateFieldIsNotEmpty(secretNamePath, p.SecretName)...) + + // validate the mount path + mountPath := prefix.Child("mountPath") + errs = append(errs, helper.ValidateFieldIsNotEmpty(mountPath, p.MountPath)...) + + // validate the default mode + defaultModePath := prefix.Child("defaultMode") + if p.DefaultMode != 0 { + if p.DefaultMode < 0 || p.DefaultMode > 511 { + errs = append(errs, field.Invalid(defaultModePath, p.DefaultMode, "defaultMode must be between 0 and 511")) + } + } + + return errs +} + +type PodTemplateOptionsMutate struct { + ImageConfig string `json:"imageConfig"` + PodConfig string `json:"podConfig"` +} + +// Validate validates the PodTemplateOptionsMutate struct. +func (p *PodTemplateOptionsMutate) Validate(prefix *field.Path) []*field.Error { + var errs []*field.Error + + // validate the image config + imageConfigPath := prefix.Child("imageConfig") + errs = append(errs, helper.ValidateFieldIsNotEmpty(imageConfigPath, p.ImageConfig)...) + + // validate the pod config + podConfigPath := prefix.Child("podConfig") + errs = append(errs, helper.ValidateFieldIsNotEmpty(podConfigPath, p.PodConfig)...) + + return errs +} diff --git a/workspaces/backend/internal/repositories/workspaces/repo.go b/workspaces/backend/internal/repositories/workspaces/repo.go index dc4541a27..bf64e7fc2 100644 --- a/workspaces/backend/internal/repositories/workspaces/repo.go +++ b/workspaces/backend/internal/repositories/workspaces/repo.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "strconv" kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -33,9 +34,11 @@ import ( ) var ( - ErrWorkspaceNotFound = fmt.Errorf("workspace not found") - ErrWorkspaceAlreadyExists = fmt.Errorf("workspace already exists") - ErrWorkspaceInvalidState = fmt.Errorf("workspace is in an invalid state for this operation") + ErrWorkspaceNotFound = fmt.Errorf("workspace not found") + ErrWorkspaceAlreadyExists = fmt.Errorf("workspace already exists") + ErrWorkspaceInvalidState = fmt.Errorf("workspace is in an invalid state for this operation") + ErrWorkspaceInvalidRevision = fmt.Errorf("workspace revision must be a positive integer") + ErrWorkspaceGenerationConflict = fmt.Errorf("current workspace generation does not match request") ) type WorkspaceRepository struct { @@ -48,33 +51,23 @@ func NewWorkspaceRepository(cl client.Client) *WorkspaceRepository { } } -func (r *WorkspaceRepository) GetWorkspace(ctx context.Context, namespace string, workspaceName string) (models.Workspace, error) { +func (r *WorkspaceRepository) GetWorkspace(ctx context.Context, namespace string, workspaceName string) (*models.WorkspaceUpdate, error) { // get workspace workspace := &kubefloworgv1beta1.Workspace{} if err := r.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: workspaceName}, workspace); err != nil { if apierrors.IsNotFound(err) { - return models.Workspace{}, ErrWorkspaceNotFound - } - return models.Workspace{}, err - } - - // get workspace kind, if it exists - workspaceKind := &kubefloworgv1beta1.WorkspaceKind{} - workspaceKindName := workspace.Spec.Kind - if err := r.client.Get(ctx, client.ObjectKey{Name: workspaceKindName}, workspaceKind); err != nil { - // ignore error if workspace kind does not exist, as we can still create a model without it - if !apierrors.IsNotFound(err) { - return models.Workspace{}, err + return nil, ErrWorkspaceNotFound } + return nil, err } - // convert workspace to model - workspaceModel := models.NewWorkspaceModelFromWorkspace(workspace, workspaceKind) + // convert workspace to WorkspaceUpdate model + workspaceUpdateModel := models.NewWorkspaceUpdateModelFromWorkspace(workspace) - return workspaceModel, nil + return workspaceUpdateModel, nil } -func (r *WorkspaceRepository) GetWorkspaces(ctx context.Context, namespace string) ([]models.Workspace, error) { +func (r *WorkspaceRepository) GetWorkspaces(ctx context.Context, namespace string) ([]models.WorkspaceListItem, error) { // get all workspaces in the namespace workspaceList := &kubefloworgv1beta1.WorkspaceList{} listOptions := []client.ListOption{ @@ -86,7 +79,7 @@ func (r *WorkspaceRepository) GetWorkspaces(ctx context.Context, namespace strin } // convert workspaces to models - workspacesModels := make([]models.Workspace, len(workspaceList.Items)) + workspacesModels := make([]models.WorkspaceListItem, len(workspaceList.Items)) for i, workspace := range workspaceList.Items { // get workspace kind, if it exists @@ -99,13 +92,13 @@ func (r *WorkspaceRepository) GetWorkspaces(ctx context.Context, namespace strin } } - workspacesModels[i] = models.NewWorkspaceModelFromWorkspace(&workspace, workspaceKind) + workspacesModels[i] = models.NewWorkspaceListItemFromWorkspace(&workspace, workspaceKind) } return workspacesModels, nil } -func (r *WorkspaceRepository) GetAllWorkspaces(ctx context.Context) ([]models.Workspace, error) { +func (r *WorkspaceRepository) GetAllWorkspaces(ctx context.Context) ([]models.WorkspaceListItem, error) { // get all workspaces in the cluster workspaceList := &kubefloworgv1beta1.WorkspaceList{} if err := r.client.List(ctx, workspaceList); err != nil { @@ -113,7 +106,7 @@ func (r *WorkspaceRepository) GetAllWorkspaces(ctx context.Context) ([]models.Wo } // convert workspaces to models - workspacesModels := make([]models.Workspace, len(workspaceList.Items)) + workspacesModels := make([]models.WorkspaceListItem, len(workspaceList.Items)) for i, workspace := range workspaceList.Items { // get workspace kind, if it exists @@ -126,7 +119,7 @@ func (r *WorkspaceRepository) GetAllWorkspaces(ctx context.Context) ([]models.Wo } } - workspacesModels[i] = models.NewWorkspaceModelFromWorkspace(&workspace, workspaceKind) + workspacesModels[i] = models.NewWorkspaceListItemFromWorkspace(&workspace, workspaceKind) } return workspacesModels, nil @@ -202,6 +195,27 @@ func (r *WorkspaceRepository) CreateWorkspace(ctx context.Context, workspaceCrea return createdWorkspaceModel, nil } +func (r *WorkspaceRepository) UpdateWorkspace(ctx context.Context, workspaceUpdate *models.WorkspaceUpdate, namespace, workspaceName string) (*models.WorkspaceUpdate, error) { + _, err := parseWorkspaceRevision(workspaceUpdate.Revision) + if err != nil { + return nil, err + } + + // TODO: implement update logic + // 1. Get current workspace from Kubernetes + // 2. Compare workspaceUpdate.Revision with current workspace.Generation + // 3. Return ErrWorkspaceGenerationConflict if mismatch + // - (?? does this mean we should avoid the cache when getting the current workspace) + // - (?? what information should we return to the caller if conflict is detected) + // 4. Apply updates to workspace.Spec (Paused, DeferUpdates, PodTemplate) + // 5. Update workspace in Kubernetes + // 6. Convert updated workspace to WorkspaceUpdate model and return + // + // TODO: ensure we raise ErrWorkspaceNotFound if the workspace does not exist + + return nil, fmt.Errorf("update workspace not implemented yet") +} + func (r *WorkspaceRepository) DeleteWorkspace(ctx context.Context, namespace, workspaceName string) error { workspace := &kubefloworgv1beta1.Workspace{ ObjectMeta: metav1.ObjectMeta{ @@ -220,6 +234,16 @@ func (r *WorkspaceRepository) DeleteWorkspace(ctx context.Context, namespace, wo return nil } +// parseWorkspaceRevision converts the revision string to the internal generation number. +// For Workspace resources, revision is the string representation of metadata.generation. +func parseWorkspaceRevision(revision string) (int64, error) { + generation, err := strconv.ParseInt(revision, 10, 64) + if err != nil || generation <= 0 { + return 0, ErrWorkspaceInvalidRevision + } + return generation, nil +} + // WorkspacePatchOperation represents a single JSONPatch operation type WorkspacePatchOperation struct { Op string `json:"op"` diff --git a/workspaces/backend/openapi/docs.go b/workspaces/backend/openapi/docs.go index d0b290754..46c0c3575 100644 --- a/workspaces/backend/openapi/docs.go +++ b/workspaces/backend/openapi/docs.go @@ -345,12 +345,6 @@ const docTemplate = `{ "$ref": "#/definitions/api.WorkspaceListEnvelope" } }, - "400": { - "description": "Bad Request. Invalid namespace format.", - "schema": { - "$ref": "#/definitions/api.ErrorEnvelope" - } - }, "401": { "description": "Unauthorized. Authentication is required.", "schema": { @@ -363,6 +357,12 @@ const docTemplate = `{ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "500": { "description": "Internal server error. An unexpected error occurred on the server.", "schema": { @@ -407,11 +407,11 @@ const docTemplate = `{ "201": { "description": "Workspace created successfully", "schema": { - "$ref": "#/definitions/api.WorkspaceEnvelope" + "$ref": "#/definitions/api.WorkspaceCreateEnvelope" } }, "400": { - "description": "Bad Request. Invalid request body or namespace format.", + "description": "Bad Request.", "schema": { "$ref": "#/definitions/api.ErrorEnvelope" } @@ -446,6 +446,111 @@ const docTemplate = `{ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "500": { + "description": "Internal server error. An unexpected error occurred on the server.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + } + } + } + }, + "/workspaces/{namespace}/{name}": { + "patch": { + "description": "Updates an existing workspace", + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "workspaces" + ], + "summary": "Update workspace", + "operationId": "updateWorkspace", + "parameters": [ + { + "type": "string", + "x-example": "kubeflow-user-example-com", + "description": "Namespace of the workspace", + "name": "namespace", + "in": "path", + "required": true + }, + { + "type": "string", + "x-example": "my-workspace", + "description": "Name of the workspace", + "name": "name", + "in": "path", + "required": true + }, + { + "description": "Workspace update configuration", + "name": "body", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/api.WorkspaceEnvelope" + } + } + ], + "responses": { + "200": { + "description": "Workspace updated successfully", + "schema": { + "$ref": "#/definitions/api.WorkspaceEnvelope" + } + }, + "400": { + "description": "Bad Request.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "401": { + "description": "Unauthorized. Authentication is required.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "403": { + "description": "Forbidden. User does not have permission to create workspace.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "409": { + "description": "Conflict. Current workspace generation is newer than provided.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "413": { + "description": "Request Entity Too Large. The request body is too large.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "415": { + "description": "Unsupported Media Type. Content-Type header is not correct.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "500": { "description": "Internal server error. An unexpected error occurred on the server.", "schema": { @@ -527,6 +632,12 @@ const docTemplate = `{ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "409": { + "description": "Conflict. Workspace not in valid state for action.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "413": { "description": "Request Entity Too Large. The request body is too large.", "schema": { @@ -540,7 +651,7 @@ const docTemplate = `{ } }, "422": { - "description": "Unprocessable Entity. Workspace is not in appropriate state.", + "description": "Unprocessable Entity. Validation error.", "schema": { "$ref": "#/definitions/api.ErrorEnvelope" } @@ -556,7 +667,7 @@ const docTemplate = `{ }, "/workspaces/{namespace}/{workspace_name}": { "get": { - "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.", "consumes": [ "application/json" ], @@ -588,17 +699,11 @@ const docTemplate = `{ ], "responses": { "200": { - "description": "Successful operation. Returns the requested workspace details.", + "description": "Successful operation. Returns the requested workspace details with revision.", "schema": { "$ref": "#/definitions/api.WorkspaceEnvelope" } }, - "400": { - "description": "Bad Request. Invalid namespace or workspace name format.", - "schema": { - "$ref": "#/definitions/api.ErrorEnvelope" - } - }, "401": { "description": "Unauthorized. Authentication is required.", "schema": { @@ -617,6 +722,12 @@ const docTemplate = `{ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "500": { "description": "Internal server error. An unexpected error occurred on the server.", "schema": { @@ -660,12 +771,6 @@ const docTemplate = `{ "204": { "description": "Workspace deleted successfully" }, - "400": { - "description": "Bad Request. Invalid namespace or workspace name format.", - "schema": { - "$ref": "#/definitions/api.ErrorEnvelope" - } - }, "401": { "description": "Unauthorized. Authentication is required.", "schema": { @@ -684,6 +789,12 @@ const docTemplate = `{ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "500": { "description": "Internal server error. An unexpected error occurred on the server.", "schema": { @@ -706,10 +817,35 @@ const docTemplate = `{ } } }, + "api.ConflictError": { + "type": "object", + "properties": { + "message": { + "description": "A human-readable description of the cause of the error.\nThis field may be presented as-is to a reader.", + "type": "string" + }, + "origin": { + "description": "Origin indicates where the conflict error originated.\nIf value is empty, the origin is unknown.", + "allOf": [ + { + "$ref": "#/definitions/api.ErrorCauseOrigin" + } + ] + } + } + }, "api.ErrorCause": { "type": "object", "properties": { + "conflict_cause": { + "description": "ConflictCauses contains details about conflict errors that caused the request to fail.", + "type": "array", + "items": { + "$ref": "#/definitions/api.ConflictError" + } + }, "validation_errors": { + "description": "ValidationErrors contains details about validation errors that caused the request to fail.", "type": "array", "items": { "$ref": "#/definitions/api.ValidationError" @@ -717,6 +853,17 @@ const docTemplate = `{ } } }, + "api.ErrorCauseOrigin": { + "type": "string", + "enum": [ + "INTERNAL", + "KUBERNETES" + ], + "x-enum-varnames": [ + "OriginInternal", + "OriginKubernetes" + ] + }, "api.ErrorEnvelope": { "type": "object", "required": [ @@ -736,12 +883,19 @@ const docTemplate = `{ ], "properties": { "cause": { - "$ref": "#/definitions/api.ErrorCause" + "description": "Cause contains detailed information about the cause of the error.", + "allOf": [ + { + "$ref": "#/definitions/api.ErrorCause" + } + ] }, "code": { + "description": "Code is a string representation of the HTTP status code.", "type": "string" }, "message": { + "description": "Message is a human-readable description of the error.", "type": "string" } } @@ -762,20 +916,30 @@ const docTemplate = `{ }, "api.ValidationError": { "type": "object", - "required": [ - "field", - "message", - "type" - ], "properties": { "field": { + "description": "The field of the resource that has caused this error, as named by its JSON serialization.\nMay include dot and postfix notation for nested attributes.\nArrays are zero-indexed.\nFields may appear more than once in an array of causes due to fields having multiple errors.\n\nExamples:\n \"name\" - the field \"name\" on the current resource\n \"items[0].name\" - the field \"name\" on the first array entry in \"items\"", "type": "string" }, "message": { + "description": "A human-readable description of the cause of the error.\nThis field may be presented as-is to a reader.", "type": "string" }, + "origin": { + "description": "Origin indicates where the validation error originated.\nIf value is empty, the origin is unknown.", + "allOf": [ + { + "$ref": "#/definitions/api.ErrorCauseOrigin" + } + ] + }, "type": { - "$ref": "#/definitions/field.ErrorType" + "description": "A machine-readable description of the cause of the error.\nIf value is empty, there is no information available.", + "allOf": [ + { + "$ref": "#/definitions/field.ErrorType" + } + ] } } }, @@ -808,7 +972,7 @@ const docTemplate = `{ ], "properties": { "data": { - "$ref": "#/definitions/workspaces.Workspace" + "$ref": "#/definitions/workspaces.WorkspaceUpdate" } } }, @@ -846,7 +1010,7 @@ const docTemplate = `{ "data": { "type": "array", "items": { - "$ref": "#/definitions/workspaces.Workspace" + "$ref": "#/definitions/workspaces.WorkspaceListItem" } } } @@ -1673,7 +1837,57 @@ const docTemplate = `{ } } }, - "workspaces.Workspace": { + "workspaces.WorkspaceCreate": { + "type": "object", + "required": [ + "deferUpdates", + "kind", + "name", + "paused", + "podTemplate" + ], + "properties": { + "deferUpdates": { + "type": "boolean" + }, + "kind": { + "type": "string" + }, + "name": { + "type": "string" + }, + "paused": { + "type": "boolean" + }, + "podTemplate": { + "$ref": "#/definitions/workspaces.PodTemplateMutate" + } + } + }, + "workspaces.WorkspaceKindInfo": { + "type": "object", + "required": [ + "icon", + "logo", + "missing", + "name" + ], + "properties": { + "icon": { + "$ref": "#/definitions/workspaces.ImageRef" + }, + "logo": { + "$ref": "#/definitions/workspaces.ImageRef" + }, + "missing": { + "type": "boolean" + }, + "name": { + "type": "string" + } + } + }, + "workspaces.WorkspaceListItem": { "type": "object", "required": [ "activity", @@ -1731,56 +1945,6 @@ const docTemplate = `{ } } }, - "workspaces.WorkspaceCreate": { - "type": "object", - "required": [ - "deferUpdates", - "kind", - "name", - "paused", - "podTemplate" - ], - "properties": { - "deferUpdates": { - "type": "boolean" - }, - "kind": { - "type": "string" - }, - "name": { - "type": "string" - }, - "paused": { - "type": "boolean" - }, - "podTemplate": { - "$ref": "#/definitions/workspaces.PodTemplateMutate" - } - } - }, - "workspaces.WorkspaceKindInfo": { - "type": "object", - "required": [ - "icon", - "logo", - "missing", - "name" - ], - "properties": { - "icon": { - "$ref": "#/definitions/workspaces.ImageRef" - }, - "logo": { - "$ref": "#/definitions/workspaces.ImageRef" - }, - "missing": { - "type": "boolean" - }, - "name": { - "type": "string" - } - } - }, "workspaces.WorkspaceState": { "type": "string", "enum": [ @@ -1799,6 +1963,32 @@ const docTemplate = `{ "WorkspaceStateError", "WorkspaceStateUnknown" ] + }, + "workspaces.WorkspaceUpdate": { + "type": "object", + "required": [ + "deferUpdates", + "paused", + "podTemplate", + "revision" + ], + "properties": { + "deferUpdates": { + "description": "TODO: remove ` + "`" + `deferUpdates` + "`" + ` once the controller is no longer applying redirects", + "type": "boolean" + }, + "paused": { + "description": "TODO: remove ` + "`" + `paused` + "`" + ` once we have an \"actions\" api for pausing workspaces", + "type": "boolean" + }, + "podTemplate": { + "$ref": "#/definitions/workspaces.PodTemplateMutate" + }, + "revision": { + "description": "Revision is an opaque token for optimistic locking.\nClients receive this value from GET requests and must include it\nin PATCH requests to ensure they are updating the expected version.\nClients must not parse, interpret, or compare revision values.", + "type": "string" + } + } } } }` diff --git a/workspaces/backend/openapi/swagger.json b/workspaces/backend/openapi/swagger.json index 33669b8a0..6943ac454 100644 --- a/workspaces/backend/openapi/swagger.json +++ b/workspaces/backend/openapi/swagger.json @@ -343,12 +343,6 @@ "$ref": "#/definitions/api.WorkspaceListEnvelope" } }, - "400": { - "description": "Bad Request. Invalid namespace format.", - "schema": { - "$ref": "#/definitions/api.ErrorEnvelope" - } - }, "401": { "description": "Unauthorized. Authentication is required.", "schema": { @@ -361,6 +355,12 @@ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "500": { "description": "Internal server error. An unexpected error occurred on the server.", "schema": { @@ -405,11 +405,11 @@ "201": { "description": "Workspace created successfully", "schema": { - "$ref": "#/definitions/api.WorkspaceEnvelope" + "$ref": "#/definitions/api.WorkspaceCreateEnvelope" } }, "400": { - "description": "Bad Request. Invalid request body or namespace format.", + "description": "Bad Request.", "schema": { "$ref": "#/definitions/api.ErrorEnvelope" } @@ -444,6 +444,111 @@ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "500": { + "description": "Internal server error. An unexpected error occurred on the server.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + } + } + } + }, + "/workspaces/{namespace}/{name}": { + "patch": { + "description": "Updates an existing workspace", + "consumes": [ + "application/json" + ], + "produces": [ + "application/json" + ], + "tags": [ + "workspaces" + ], + "summary": "Update workspace", + "operationId": "updateWorkspace", + "parameters": [ + { + "type": "string", + "x-example": "kubeflow-user-example-com", + "description": "Namespace of the workspace", + "name": "namespace", + "in": "path", + "required": true + }, + { + "type": "string", + "x-example": "my-workspace", + "description": "Name of the workspace", + "name": "name", + "in": "path", + "required": true + }, + { + "description": "Workspace update configuration", + "name": "body", + "in": "body", + "required": true, + "schema": { + "$ref": "#/definitions/api.WorkspaceEnvelope" + } + } + ], + "responses": { + "200": { + "description": "Workspace updated successfully", + "schema": { + "$ref": "#/definitions/api.WorkspaceEnvelope" + } + }, + "400": { + "description": "Bad Request.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "401": { + "description": "Unauthorized. Authentication is required.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "403": { + "description": "Forbidden. User does not have permission to create workspace.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "409": { + "description": "Conflict. Current workspace generation is newer than provided.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "413": { + "description": "Request Entity Too Large. The request body is too large.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "415": { + "description": "Unsupported Media Type. Content-Type header is not correct.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "500": { "description": "Internal server error. An unexpected error occurred on the server.", "schema": { @@ -525,6 +630,12 @@ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "409": { + "description": "Conflict. Workspace not in valid state for action.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "413": { "description": "Request Entity Too Large. The request body is too large.", "schema": { @@ -538,7 +649,7 @@ } }, "422": { - "description": "Unprocessable Entity. Workspace is not in appropriate state.", + "description": "Unprocessable Entity. Validation error.", "schema": { "$ref": "#/definitions/api.ErrorEnvelope" } @@ -554,7 +665,7 @@ }, "/workspaces/{namespace}/{workspace_name}": { "get": { - "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.", "consumes": [ "application/json" ], @@ -586,17 +697,11 @@ ], "responses": { "200": { - "description": "Successful operation. Returns the requested workspace details.", + "description": "Successful operation. Returns the requested workspace details with revision.", "schema": { "$ref": "#/definitions/api.WorkspaceEnvelope" } }, - "400": { - "description": "Bad Request. Invalid namespace or workspace name format.", - "schema": { - "$ref": "#/definitions/api.ErrorEnvelope" - } - }, "401": { "description": "Unauthorized. Authentication is required.", "schema": { @@ -615,6 +720,12 @@ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "500": { "description": "Internal server error. An unexpected error occurred on the server.", "schema": { @@ -658,12 +769,6 @@ "204": { "description": "Workspace deleted successfully" }, - "400": { - "description": "Bad Request. Invalid namespace or workspace name format.", - "schema": { - "$ref": "#/definitions/api.ErrorEnvelope" - } - }, "401": { "description": "Unauthorized. Authentication is required.", "schema": { @@ -682,6 +787,12 @@ "$ref": "#/definitions/api.ErrorEnvelope" } }, + "422": { + "description": "Unprocessable Entity. Validation error.", + "schema": { + "$ref": "#/definitions/api.ErrorEnvelope" + } + }, "500": { "description": "Internal server error. An unexpected error occurred on the server.", "schema": { @@ -704,10 +815,35 @@ } } }, + "api.ConflictError": { + "type": "object", + "properties": { + "message": { + "description": "A human-readable description of the cause of the error.\nThis field may be presented as-is to a reader.", + "type": "string" + }, + "origin": { + "description": "Origin indicates where the conflict error originated.\nIf value is empty, the origin is unknown.", + "allOf": [ + { + "$ref": "#/definitions/api.ErrorCauseOrigin" + } + ] + } + } + }, "api.ErrorCause": { "type": "object", "properties": { + "conflict_cause": { + "description": "ConflictCauses contains details about conflict errors that caused the request to fail.", + "type": "array", + "items": { + "$ref": "#/definitions/api.ConflictError" + } + }, "validation_errors": { + "description": "ValidationErrors contains details about validation errors that caused the request to fail.", "type": "array", "items": { "$ref": "#/definitions/api.ValidationError" @@ -715,6 +851,17 @@ } } }, + "api.ErrorCauseOrigin": { + "type": "string", + "enum": [ + "INTERNAL", + "KUBERNETES" + ], + "x-enum-varnames": [ + "OriginInternal", + "OriginKubernetes" + ] + }, "api.ErrorEnvelope": { "type": "object", "required": [ @@ -734,12 +881,19 @@ ], "properties": { "cause": { - "$ref": "#/definitions/api.ErrorCause" + "description": "Cause contains detailed information about the cause of the error.", + "allOf": [ + { + "$ref": "#/definitions/api.ErrorCause" + } + ] }, "code": { + "description": "Code is a string representation of the HTTP status code.", "type": "string" }, "message": { + "description": "Message is a human-readable description of the error.", "type": "string" } } @@ -760,20 +914,30 @@ }, "api.ValidationError": { "type": "object", - "required": [ - "field", - "message", - "type" - ], "properties": { "field": { + "description": "The field of the resource that has caused this error, as named by its JSON serialization.\nMay include dot and postfix notation for nested attributes.\nArrays are zero-indexed.\nFields may appear more than once in an array of causes due to fields having multiple errors.\n\nExamples:\n \"name\" - the field \"name\" on the current resource\n \"items[0].name\" - the field \"name\" on the first array entry in \"items\"", "type": "string" }, "message": { + "description": "A human-readable description of the cause of the error.\nThis field may be presented as-is to a reader.", "type": "string" }, + "origin": { + "description": "Origin indicates where the validation error originated.\nIf value is empty, the origin is unknown.", + "allOf": [ + { + "$ref": "#/definitions/api.ErrorCauseOrigin" + } + ] + }, "type": { - "$ref": "#/definitions/field.ErrorType" + "description": "A machine-readable description of the cause of the error.\nIf value is empty, there is no information available.", + "allOf": [ + { + "$ref": "#/definitions/field.ErrorType" + } + ] } } }, @@ -806,7 +970,7 @@ ], "properties": { "data": { - "$ref": "#/definitions/workspaces.Workspace" + "$ref": "#/definitions/workspaces.WorkspaceUpdate" } } }, @@ -844,7 +1008,7 @@ "data": { "type": "array", "items": { - "$ref": "#/definitions/workspaces.Workspace" + "$ref": "#/definitions/workspaces.WorkspaceListItem" } } } @@ -1671,7 +1835,57 @@ } } }, - "workspaces.Workspace": { + "workspaces.WorkspaceCreate": { + "type": "object", + "required": [ + "deferUpdates", + "kind", + "name", + "paused", + "podTemplate" + ], + "properties": { + "deferUpdates": { + "type": "boolean" + }, + "kind": { + "type": "string" + }, + "name": { + "type": "string" + }, + "paused": { + "type": "boolean" + }, + "podTemplate": { + "$ref": "#/definitions/workspaces.PodTemplateMutate" + } + } + }, + "workspaces.WorkspaceKindInfo": { + "type": "object", + "required": [ + "icon", + "logo", + "missing", + "name" + ], + "properties": { + "icon": { + "$ref": "#/definitions/workspaces.ImageRef" + }, + "logo": { + "$ref": "#/definitions/workspaces.ImageRef" + }, + "missing": { + "type": "boolean" + }, + "name": { + "type": "string" + } + } + }, + "workspaces.WorkspaceListItem": { "type": "object", "required": [ "activity", @@ -1729,56 +1943,6 @@ } } }, - "workspaces.WorkspaceCreate": { - "type": "object", - "required": [ - "deferUpdates", - "kind", - "name", - "paused", - "podTemplate" - ], - "properties": { - "deferUpdates": { - "type": "boolean" - }, - "kind": { - "type": "string" - }, - "name": { - "type": "string" - }, - "paused": { - "type": "boolean" - }, - "podTemplate": { - "$ref": "#/definitions/workspaces.PodTemplateMutate" - } - } - }, - "workspaces.WorkspaceKindInfo": { - "type": "object", - "required": [ - "icon", - "logo", - "missing", - "name" - ], - "properties": { - "icon": { - "$ref": "#/definitions/workspaces.ImageRef" - }, - "logo": { - "$ref": "#/definitions/workspaces.ImageRef" - }, - "missing": { - "type": "boolean" - }, - "name": { - "type": "string" - } - } - }, "workspaces.WorkspaceState": { "type": "string", "enum": [ @@ -1797,6 +1961,32 @@ "WorkspaceStateError", "WorkspaceStateUnknown" ] + }, + "workspaces.WorkspaceUpdate": { + "type": "object", + "required": [ + "deferUpdates", + "paused", + "podTemplate", + "revision" + ], + "properties": { + "deferUpdates": { + "description": "TODO: remove `deferUpdates` once the controller is no longer applying redirects", + "type": "boolean" + }, + "paused": { + "description": "TODO: remove `paused` once we have an \"actions\" api for pausing workspaces", + "type": "boolean" + }, + "podTemplate": { + "$ref": "#/definitions/workspaces.PodTemplateMutate" + }, + "revision": { + "description": "Revision is an opaque token for optimistic locking.\nClients receive this value from GET requests and must include it\nin PATCH requests to ensure they are updating the expected version.\nClients must not parse, interpret, or compare revision values.", + "type": "string" + } + } } } } \ No newline at end of file diff --git a/workspaces/controller/internal/webhook/workspacekind_webhook.go b/workspaces/controller/internal/webhook/workspacekind_webhook.go index b587e082a..fdd0fe23f 100644 --- a/workspaces/controller/internal/webhook/workspacekind_webhook.go +++ b/workspaces/controller/internal/webhook/workspacekind_webhook.go @@ -20,12 +20,14 @@ import ( "context" "errors" "fmt" + "net/http" "reflect" "sync" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" apivalidation "k8s.io/apimachinery/pkg/api/validation" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" @@ -408,11 +410,28 @@ func (v *WorkspaceKindValidator) ValidateDelete(ctx context.Context, obj runtime // don't allow deletion of WorkspaceKind if it is used by any workspaces if workspaceKind.Status.Workspaces > 0 { - return nil, apierrors.NewConflict( - schema.GroupResource{Group: kubefloworgv1beta1.GroupVersion.Group, Resource: "WorkspaceKind"}, - workspaceKind.Name, - fmt.Errorf("WorkspaceKind is used by %d workspace(s)", workspaceKind.Status.Workspaces), - ) + return nil, &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusConflict, + Reason: metav1.StatusReasonConflict, + Details: &metav1.StatusDetails{ + Group: workspaceKind.GroupVersionKind().Group, + Kind: workspaceKind.GroupVersionKind().Kind, + Name: workspaceKind.Name, + Causes: []metav1.StatusCause{ + { + Message: fmt.Sprintf("WorkspaceKind is used by %d workspace(s)", workspaceKind.Status.Workspaces), + }, + }, + }, + Message: fmt.Sprintf( + "Operation cannot be fulfilled on %s %q", + workspaceKind.GroupVersionKind().GroupKind().String(), + workspaceKind.Name, + ), + }, + } } // don't allow deletion of WorkspaceKind if it has the protection finalizer @@ -420,11 +439,28 @@ func (v *WorkspaceKindValidator) ValidateDelete(ctx context.Context, obj runtime // it is impossible to "un-delete" a resource once it has started terminating // and this is a bad user experience, so we prevent deletion in the first place if controllerutil.ContainsFinalizer(workspaceKind, controller.WorkspaceKindFinalizer) { - return nil, apierrors.NewConflict( - schema.GroupResource{Group: kubefloworgv1beta1.GroupVersion.Group, Resource: "WorkspaceKind"}, - workspaceKind.Name, - errors.New("WorkspaceKind has protection finalizer, indicating one or more workspaces are still using it"), - ) + return nil, &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusConflict, + Reason: metav1.StatusReasonConflict, + Details: &metav1.StatusDetails{ + Group: workspaceKind.GroupVersionKind().Group, + Kind: workspaceKind.GroupVersionKind().Kind, + Name: workspaceKind.Name, + Causes: []metav1.StatusCause{ + { + Message: "WorkspaceKind has protection finalizer, indicating one or more workspaces are still using it", + }, + }, + }, + Message: fmt.Sprintf( + "Operation cannot be fulfilled on %s %q", + workspaceKind.GroupVersionKind().GroupKind().String(), + workspaceKind.Name, + ), + }, + } } return nil, nil