diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index eb7ca4797..322ef6c6d 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -228,7 +228,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation } r.warnLegacyPresets(app) - r.warnRemovedFields(ctx, app) + r.warnRemovedKubernetesFields(ctx, app) r.warnRemovedUserPasswords(ctx, app) // Run the genericapiserver-supplied validating admission chain @@ -590,7 +590,7 @@ func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObje } r.warnLegacyPresets(app) - r.warnRemovedFields(ctx, app) + r.warnRemovedKubernetesFields(ctx, app) r.warnRemovedUserPasswords(ctx, app) r.warnUpgradeIntroducedCollisions(ctx, oldObj, app) @@ -2155,81 +2155,35 @@ func (r *REST) warnLegacyPresets(app *appsv1alpha1.Application) { } } -// kubernetesKind is the Application.Kind of the Kubernetes cluster CR. +// kubernetesKind is the Application.Kind of the Kubernetes cluster CR whose +// Phase 2-removed value keys the warning below reports on. const kubernetesKind = "Kubernetes" -// removedField is a values key whose chart has stopped reading it. Such a key -// is still accepted and stored -- an upgraded release keeps it until re-edited, -// and the migrations deliberately leave it in place rather than rewriting -// values mid-upgrade -- so the write path says so rather than letting the field -// look effective. -type removedField struct { - key string - // replacement says what happens instead, in the second half of the warning. - replacement string - // emptyDefault marks a key that is set only when it carries a value. The - // image overrides defaulted to empty strings, the read path fills defaults - // in, and a client that writes back what it read stores them, so their - // presence says nothing. - emptyDefault bool -} +// removedKubernetesFields are Kubernetes CR value keys that Phase 2 moved to the +// separate KubernetesNodes resource. They are still accepted and stored -- an +// upgraded cluster keeps them in its values until re-edited, and the adoption +// migration deliberately leaves them in place rather than rewriting parent +// values mid-upgrade -- but they no longer render anything on the Kubernetes CR. +var removedKubernetesFields = []string{"nodeGroups", "nodeHealthCheck", "maxNodeProvisionTime"} -var removedFieldsByKind = map[string][]removedField{ - kubernetesKind: { - {key: "nodeGroups", replacement: "worker pools are managed as separate KubernetesNodes resources (see the kubernetes-nodes chart)"}, - {key: "nodeHealthCheck", replacement: "worker pools are managed as separate KubernetesNodes resources (see the kubernetes-nodes chart)"}, - {key: "maxNodeProvisionTime", replacement: "worker pools are managed as separate KubernetesNodes resources (see the kubernetes-nodes chart)"}, - {key: "images", replacement: "the images come from the chart, and an air-gapped install serves them through the registry mirrors configured on its nodes", emptyDefault: true}, - }, - "KubernetesNodes": { - {key: "images", replacement: "the images come from the chart, and an air-gapped install serves them through the registry mirrors configured on its nodes", emptyDefault: true}, - }, - "OpenSearch": { - {key: "images", replacement: "the image follows spec.version", emptyDefault: true}, - }, -} - -// warnRemovedFields emits a client-facing admission warning for each removed -// field still present in the values, so an operator editing one (e.g. bumping -// spec.nodeGroups..minReplicas) is told it has no effect instead of silently -// getting nothing. Non-blocking: the field is preserved as-is. -func (r *REST) warnRemovedFields(ctx context.Context, app *appsv1alpha1.Application) { - fields, ok := removedFieldsByKind[r.kindName] - if !ok || app == nil || app.Spec == nil || len(app.Spec.Raw) == 0 { +// warnRemovedKubernetesFields emits a client-facing admission warning for each +// Phase 2-removed field still present in a Kubernetes CR's values, so an +// operator editing one (e.g. bumping spec.nodeGroups..minReplicas) is told it +// has no effect instead of silently getting nothing. Non-blocking: the field is +// preserved as-is; worker pools are managed as KubernetesNodes resources. +func (r *REST) warnRemovedKubernetesFields(ctx context.Context, app *appsv1alpha1.Application) { + if r.kindName != kubernetesKind || app == nil || app.Spec == nil || len(app.Spec.Raw) == 0 { return } var values map[string]any if err := json.Unmarshal(app.Spec.Raw, &values); err != nil { return } - for _, f := range fields { - value, present := values[f.key] - if !present || (f.emptyDefault && !carriesValue(value)) { - continue + for _, key := range removedKubernetesFields { + if _, present := values[key]; present { + warning.AddWarning(ctx, "", fmt.Sprintf( + "spec.%s is ignored on the Kubernetes resource since Phase 2: worker pools are managed as separate KubernetesNodes resources (see the kubernetes-nodes chart). The field is stored but has no effect.", key)) } - warning.AddWarning(ctx, "", fmt.Sprintf( - "spec.%s is ignored on the %s resource: %s. The field is stored but has no effect.", - f.key, r.kindName, f.replacement)) - } -} - -// carriesValue reports whether a value is more than an empty default: not null, -// not an empty string, and for a map, at least one entry that carries a value. -func carriesValue(value any) bool { - switch v := value.(type) { - case nil: - return false - case string: - return v != "" - case map[string]any: - for _, nested := range v { - if carriesValue(nested) { - return true - } - } - return false - default: - return true } } diff --git a/pkg/registry/apps/application/rest_validation_test.go b/pkg/registry/apps/application/rest_validation_test.go index 9efc192cc..f48f40d26 100644 --- a/pkg/registry/apps/application/rest_validation_test.go +++ b/pkg/registry/apps/application/rest_validation_test.go @@ -443,11 +443,11 @@ type fakeWarningRecorder struct{ warnings []string } func (f *fakeWarningRecorder) AddWarning(_, text string) { f.warnings = append(f.warnings, text) } -// TestWarnRemovedFields covers the inert-field warning: a release that still -// carries a key its chart has stopped reading is accepted (the migrations leave -// those keys in place) but the operator is warned that it no longer takes -// effect. A kind with no removed fields never warns. -func TestWarnRemovedFields(t *testing.T) { +// TestWarnRemovedKubernetesFields covers the Phase 2 inert-field warning: a +// Kubernetes CR that still carries nodeGroups / nodeHealthCheck / +// maxNodeProvisionTime is accepted (the migration leaves them in place) but the +// operator is warned that they no longer take effect. Other kinds never warn. +func TestWarnRemovedKubernetesFields(t *testing.T) { tests := []struct { name string kindName string @@ -478,36 +478,6 @@ func TestWarnRemovedFields(t *testing.T) { specJSON: `{"nodeGroups":{"md0":{}}}`, wantKeys: nil, }, - { - name: "kubernetes with the removed image overrides warns", - kindName: "Kubernetes", - specJSON: `{"images":{"kubectl":"example.test/kubectl:1"}}`, - wantKeys: []string{"images"}, - }, - { - name: "kubernetes with the defaulted empty image overrides does not warn", - kindName: "Kubernetes", - specJSON: `{"images":{"kubectl":"","talosCsrSigner":"","waitForKubeconfig":""}}`, - wantKeys: nil, - }, - { - name: "opensearch with the defaulted empty image override does not warn", - kindName: "OpenSearch", - specJSON: `{"version":"v2","images":{"opensearch":""}}`, - wantKeys: nil, - }, - { - name: "opensearch with the removed image override warns", - kindName: "OpenSearch", - specJSON: `{"version":"v2","images":{"opensearch":"example.test/opensearch:1"}}`, - wantKeys: []string{"images"}, - }, - { - name: "foundationdb legacy version is handled by the chart", - kindName: "FoundationDB", - specJSON: `{"version":"v7.3","cluster":{"version":"7.1.67","redundancyMode":"double"}}`, - wantKeys: nil, - }, } for _, tt := range tests { @@ -517,7 +487,7 @@ func TestWarnRemovedFields(t *testing.T) { r := &REST{kindName: tt.kindName} app := &appsv1alpha1.Application{Spec: &apiextv1.JSON{Raw: []byte(tt.specJSON)}} - r.warnRemovedFields(ctx, app) + r.warnRemovedKubernetesFields(ctx, app) if len(rec.warnings) != len(tt.wantKeys) { t.Fatalf("expected %d warnings, got %d: %v", len(tt.wantKeys), len(rec.warnings), rec.warnings) diff --git a/pkg/registry/apps/application/rest_warn_removed_fields_callsite_test.go b/pkg/registry/apps/application/rest_warn_removed_fields_callsite_test.go index 1872a5477..b83666102 100644 --- a/pkg/registry/apps/application/rest_warn_removed_fields_callsite_test.go +++ b/pkg/registry/apps/application/rest_warn_removed_fields_callsite_test.go @@ -36,8 +36,8 @@ import ( "github.com/cozystack/cozystack/pkg/config" ) -// The inert-field warning is only useful if it is actually wired into -// the write path. TestWarnRemovedFields (rest_validation_test.go) +// The Phase 2 inert-field warning is only useful if it is actually wired into +// the write path. TestWarnRemovedKubernetesFields (rest_validation_test.go) // pins the helper in isolation, but deleting either call site // (rest.go Create / Update) leaves that test green while an operator editing a // removed field silently gets no warning. These two tests drive Create and @@ -99,7 +99,7 @@ func TestCreate_WarnsOnRemovedKubernetesFields(t *testing.T) { ctx := warning.WithWarningRecorder(request.WithNamespace(context.Background(), "tenant-foo"), rec) // Short-circuit right after the warning (createValidation runs immediately - // after warnRemovedFields), so the test does not depend on + // after warnRemovedKubernetesFields), so the test does not depend on // conversion or the fake client's write path. sentinel := errors.New("stop after warning") createValidation := func(_ context.Context, _ runtime.Object) error { return sentinel } @@ -128,7 +128,7 @@ func TestUpdate_WarnsOnRemovedKubernetesFields(t *testing.T) { rec := &fakeWarningRecorder{} ctx := warning.WithWarningRecorder(request.WithNamespace(context.Background(), "tenant-foo"), rec) - // updateValidation runs BEFORE warnRemovedFields, so it must pass + // updateValidation runs BEFORE warnRemovedKubernetesFields, so it must pass // (return nil) for the warning to be reached. The final Update result is // irrelevant — the warning is recorded before any conversion/write — so any // downstream error is ignored; only the recorded warning is asserted.