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 <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
This commit is contained in:
parent
c932740dc5
commit
e267cfcf9d
4 changed files with 44 additions and 34 deletions
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 ""
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue