refactor(api): keep application image policy in charts

The charts enforce image selection without extending the API server's
write path. Keep the existing admission behavior while the application
schemas expose the supported fields.

Assisted-by: LLM
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

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.<x>.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.<x>.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
 	}
 }
 

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)

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.