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..6353c00ec 100644 --- a/workspaces/backend/api/workspaces_handler.go +++ b/workspaces/backend/api/workspaces_handler.go @@ -50,10 +50,10 @@ type WorkspaceEnvelope Envelope[models.Workspace] // @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." // @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) @@ -194,12 +194,13 @@ func (a *App) getWorkspacesHandler(w http.ResponseWriter, r *http.Request, ps ht // @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." +// @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) { @@ -295,10 +308,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 +320,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 +349,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/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_create.go b/workspaces/backend/internal/models/workspaces/funcs_write.go similarity index 100% rename from workspaces/backend/internal/models/workspaces/funcs_create.go rename to workspaces/backend/internal/models/workspaces/funcs_write.go 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..59b679aa5 --- /dev/null +++ b/workspaces/backend/internal/models/workspaces/types_write.go @@ -0,0 +1,194 @@ +/* +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 +} + +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 +} + +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/openapi/docs.go b/workspaces/backend/openapi/docs.go index d0b290754..23ff37bb5 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": { @@ -411,7 +411,7 @@ const docTemplate = `{ } }, "400": { - "description": "Bad Request. Invalid request body or namespace format.", + "description": "Bad Request.", "schema": { "$ref": "#/definitions/api.ErrorEnvelope" } @@ -446,6 +446,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": { @@ -527,6 +533,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 +552,7 @@ const docTemplate = `{ } }, "422": { - "description": "Unprocessable Entity. Workspace is not in appropriate state.", + "description": "Unprocessable Entity. Validation error.", "schema": { "$ref": "#/definitions/api.ErrorEnvelope" } @@ -593,12 +605,6 @@ const docTemplate = `{ "$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 +623,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 +672,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 +690,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 +718,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 +754,17 @@ const docTemplate = `{ } } }, + "api.ErrorCauseOrigin": { + "type": "string", + "enum": [ + "INTERNAL", + "KUBERNETES" + ], + "x-enum-varnames": [ + "OriginInternal", + "OriginKubernetes" + ] + }, "api.ErrorEnvelope": { "type": "object", "required": [ @@ -736,12 +784,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 +817,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" + } + ] } } }, diff --git a/workspaces/backend/openapi/swagger.json b/workspaces/backend/openapi/swagger.json index 33669b8a0..390c86246 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": { @@ -409,7 +409,7 @@ } }, "400": { - "description": "Bad Request. Invalid request body or namespace format.", + "description": "Bad Request.", "schema": { "$ref": "#/definitions/api.ErrorEnvelope" } @@ -444,6 +444,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": { @@ -525,6 +531,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 +550,7 @@ } }, "422": { - "description": "Unprocessable Entity. Workspace is not in appropriate state.", + "description": "Unprocessable Entity. Validation error.", "schema": { "$ref": "#/definitions/api.ErrorEnvelope" } @@ -591,12 +603,6 @@ "$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 +621,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 +670,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 +688,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 +716,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 +752,17 @@ } } }, + "api.ErrorCauseOrigin": { + "type": "string", + "enum": [ + "INTERNAL", + "KUBERNETES" + ], + "x-enum-varnames": [ + "OriginInternal", + "OriginKubernetes" + ] + }, "api.ErrorEnvelope": { "type": "object", "required": [ @@ -734,12 +782,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 +815,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" + } + ] } } }, 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