From e267cfcf9dd30aef98f2fa0a4a479db5d0b305a9 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Wed, 11 Feb 2026 13:29:42 +0300 Subject: [PATCH] fix(api): address review feedback for validation consistency - Return field.ErrorList from validateNameLength for consistent apierrors.NewInvalid error shape (was NewBadRequest) - Add klog warning when YAML parsing fails in parseRootHostFromSecret - Fix maxHelmReleaseName comment to accurately describe Helm convention - Add note that root-host changes require API server restart - Replace interface{} with any throughout openapi.go and rest.go - Remove trailing blank line in const block Co-Authored-By: Claude Signed-off-by: Aleksei Sviridkin --- pkg/cmd/server/openapi.go | 31 ++++++++-------- pkg/cmd/server/start.go | 1 + pkg/registry/apps/application/rest.go | 36 ++++++++++++------- .../apps/application/rest_validation_test.go | 10 +++--- 4 files changed, 44 insertions(+), 34 deletions(-) diff --git a/pkg/cmd/server/openapi.go b/pkg/cmd/server/openapi.go index b52aa989..616709ca 100644 --- a/pkg/cmd/server/openapi.go +++ b/pkg/cmd/server/openapi.go @@ -19,7 +19,6 @@ const ( baseListRef = apiPrefix + ".ApplicationList" baseStatusRef = apiPrefix + ".ApplicationStatus" smp = "application/strategic-merge-patch+json" - ) // deepCopySchema clones *spec.Schema via JSON-marshal/unmarshal. @@ -102,9 +101,9 @@ func cloneKindSchemas(kind string, base, baseStatus, baseList *spec.Schema, v3 b // GVK-extensions setGVK := func(s *spec.Schema, k string) { - s.Extensions = map[string]interface{}{ - "x-kubernetes-group-version-kind": []interface{}{ - map[string]interface{}{"group": "apps.cozystack.io", "version": "v1alpha1", "kind": k}, + s.Extensions = map[string]any{ + "x-kubernetes-group-version-kind": []any{ + map[string]any{"group": "apps.cozystack.io", "version": "v1alpha1", "kind": k}, }, } } @@ -133,35 +132,35 @@ func cloneKindSchemas(kind string, base, baseStatus, baseList *spec.Schema, v3 b } // rewriteDocRefs rewrites all $ref in the OpenAPI document -func rewriteDocRefs(doc interface{}) ([]byte, error) { +func rewriteDocRefs(doc any) ([]byte, error) { raw, err := json.Marshal(doc) if err != nil { return nil, fmt.Errorf("failed to marshal OpenAPI document: %w", err) } - var any interface{} - if err := json.Unmarshal(raw, &any); err != nil { + var parsed any + if err := json.Unmarshal(raw, &parsed); err != nil { return nil, err } - walkAndRewriteRefs(any, "") - return json.Marshal(any) + walkAndRewriteRefs(parsed, "") + return json.Marshal(parsed) } // walkAndRewriteRefs walks arbitrary JSON (map/array) and // - when encountering x-kubernetes-group-version-kind, extracts kind, // updating the currentKind context; // - rewrites all $ref inside the current context from Application* → kind*. -func walkAndRewriteRefs(node interface{}, currentKind string) { +func walkAndRewriteRefs(node any, currentKind string) { switch n := node.(type) { - case map[string]interface{}: + case map[string]any: if gvk, ok := n["x-kubernetes-group-version-kind"]; ok { switch g := gvk.(type) { - case map[string]interface{}: + case map[string]any: if k, ok := g["kind"].(string); ok { currentKind = k } - case []interface{}: + case []any: if len(g) > 0 { - if mm, ok := g[0].(map[string]interface{}); ok { + if mm, ok := g[0].(map[string]any); ok { if k, ok := mm["kind"].(string); ok { currentKind = k } @@ -178,7 +177,7 @@ func walkAndRewriteRefs(node interface{}, currentKind string) { } walkAndRewriteRefs(v, currentKind) } - case []interface{}: + case []any: for _, v := range n { walkAndRewriteRefs(v, currentKind) } @@ -293,7 +292,7 @@ func sanitizeForV2(s *spec.Schema) { if hasIntAndStringAnyOf(s.AnyOf) { s.Type = spec.StringOrArray{"string"} if s.Extensions == nil { - s.Extensions = map[string]interface{}{} + s.Extensions = map[string]any{} } s.Extensions["x-kubernetes-int-or-string"] = true } diff --git a/pkg/cmd/server/start.go b/pkg/cmd/server/start.go index bf0d63bd..09c9485c 100644 --- a/pkg/cmd/server/start.go +++ b/pkg/cmd/server/start.go @@ -220,6 +220,7 @@ func parseRootHostFromSecret(secret *corev1.Secret) string { } `json:"_cluster"` } if err := yaml.Unmarshal(valuesYAML, &values); err != nil { + klog.Warningf("failed to parse values.yaml from cozystack-values secret: %v", err) return "" } diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index 42734774..26e8429f 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -162,8 +162,8 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation } // Validate name length against Helm release and label limits - if err := r.validateNameLength(app.Name); err != nil { - return nil, apierrors.NewBadRequest(err.Error()) + if nameLenErrs := r.validateNameLength(app.Name); len(nameLenErrs) > 0 { + return nil, apierrors.NewInvalid(r.gvk.GroupKind(), app.Name, nameLenErrs) } // Validate that values don't contain reserved keys (starting with "_") @@ -1014,7 +1014,7 @@ func filterInternalKeys(values *apiextv1.JSON) *apiextv1.JSON { if values == nil || len(values.Raw) == 0 { return values } - var data map[string]interface{} + var data map[string]any if err := json.Unmarshal(values.Raw, &data); err != nil { return values } @@ -1035,7 +1035,7 @@ func validateNoInternalKeys(values *apiextv1.JSON) error { if values == nil || len(values.Raw) == 0 { return nil } - var data map[string]interface{} + var data map[string]any if err := json.Unmarshal(values.Raw, &data); err != nil { return err } @@ -1047,7 +1047,8 @@ func validateNoInternalKeys(values *apiextv1.JSON) error { return nil } -// maxHelmReleaseName is the maximum length of a Helm release name (DNS-1123 subdomain). +// maxHelmReleaseName is the Helm release name limit. Helm reserves room for +// chart-generated resource suffixes within the 63-char DNS-1035 label limit. const maxHelmReleaseName = 53 // maxK8sLabelValue is the maximum length of a Kubernetes label value. @@ -1057,7 +1058,13 @@ const maxK8sLabelValue = 63 // For all apps: prefix + name must fit within the Helm release name limit (53 chars). // For Tenants: additionally checks that the host label (name + "." + rootHost) fits // within the Kubernetes label value limit (63 chars). -func (r *REST) validateNameLength(name string) error { +// +// Note: rootHost is read once at API server startup from the cozystack-values Secret. +// Changing root-host requires an API server restart for validation to reflect the new value. +func (r *REST) validateNameLength(name string) field.ErrorList { + fldPath := field.NewPath("metadata").Child("name") + allErrs := field.ErrorList{} + helmMax := maxHelmReleaseName - len(r.releaseConfig.Prefix) maxLen := helmMax @@ -1069,15 +1076,18 @@ func (r *REST) validateNameLength(name string) error { } if maxLen <= 0 { - return fmt.Errorf("configuration error: no valid name length possible (release prefix %q, root host %q)", - r.releaseConfig.Prefix, r.rootHost) + allErrs = append(allErrs, field.Invalid(fldPath, name, + fmt.Sprintf("configuration error: no valid name length possible (release prefix %q, root host %q)", + r.releaseConfig.Prefix, r.rootHost))) + return allErrs } if len(name) > maxLen { - return fmt.Errorf("name %q is too long: maximum %d characters allowed (release prefix %q, root host %q)", - name, maxLen, r.releaseConfig.Prefix, r.rootHost) + allErrs = append(allErrs, field.Invalid(fldPath, name, + fmt.Sprintf("must be no more than %d characters (release prefix %q, root host %q)", + maxLen, r.releaseConfig.Prefix, r.rootHost))) } - return nil + return allErrs } // convertHelmReleaseToApplication implements the actual conversion logic @@ -1233,7 +1243,7 @@ func (r *REST) buildTableFromApplications(apps []appsv1alpha1.Application) metav for i := range apps { app := &apps[i] row := metav1.TableRow{ - Cells: []interface{}{app.GetName(), getReadyStatus(app.Status.Conditions), computeAge(app.GetCreationTimestamp().Time, now), getVersion(app.Status.Version)}, + Cells: []any{app.GetName(), getReadyStatus(app.Status.Conditions), computeAge(app.GetCreationTimestamp().Time, now), getVersion(app.Status.Version)}, Object: runtime.RawExtension{Object: app}, } table.Rows = append(table.Rows, row) @@ -1257,7 +1267,7 @@ func (r *REST) buildTableFromApplication(app appsv1alpha1.Application) metav1.Ta a := app row := metav1.TableRow{ - Cells: []interface{}{app.GetName(), getReadyStatus(app.Status.Conditions), computeAge(app.GetCreationTimestamp().Time, now), getVersion(app.Status.Version)}, + Cells: []any{app.GetName(), getReadyStatus(app.Status.Conditions), computeAge(app.GetCreationTimestamp().Time, now), getVersion(app.Status.Version)}, Object: runtime.RawExtension{Object: &a}, } table.Rows = append(table.Rows, row) diff --git a/pkg/registry/apps/application/rest_validation_test.go b/pkg/registry/apps/application/rest_validation_test.go index 8c93cf37..ee6d66da 100644 --- a/pkg/registry/apps/application/rest_validation_test.go +++ b/pkg/registry/apps/application/rest_validation_test.go @@ -172,13 +172,13 @@ func TestValidateNameLength(t *testing.T) { rootHost: tt.rootHost, } - err := r.validateNameLength(tt.appName) + errs := r.validateNameLength(tt.appName) - if tt.wantError && err == nil { - t.Errorf("expected error for name %q (len=%d), got nil", tt.appName, len(tt.appName)) + if tt.wantError && len(errs) == 0 { + t.Errorf("expected error for name %q (len=%d), got none", tt.appName, len(tt.appName)) } - if !tt.wantError && err != nil { - t.Errorf("unexpected error for name %q (len=%d): %v", tt.appName, len(tt.appName), err) + if !tt.wantError && len(errs) > 0 { + t.Errorf("unexpected error for name %q (len=%d): %v", tt.appName, len(tt.appName), errs) } }) }