Skip to content

Commit e694b05

Browse files
myasnikovdaniilmyasnikovdaniil
committed
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 <[email protected]>
1 parent 59932e5 commit e694b05

3 files changed

Lines changed: 32 additions & 108 deletions

File tree

‎pkg/registry/apps/application/rest.go‎

Lines changed: 22 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
228228
}
229229

230230
r.warnLegacyPresets(app)
231-
r.warnRemovedFields(ctx, app)
231+
r.warnRemovedKubernetesFields(ctx, app)
232232
r.warnRemovedUserPasswords(ctx, app)
233233

234234
// Run the genericapiserver-supplied validating admission chain
@@ -590,7 +590,7 @@ func (r *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObje
590590
}
591591

592592
r.warnLegacyPresets(app)
593-
r.warnRemovedFields(ctx, app)
593+
r.warnRemovedKubernetesFields(ctx, app)
594594
r.warnRemovedUserPasswords(ctx, app)
595595
r.warnUpgradeIntroducedCollisions(ctx, oldObj, app)
596596

@@ -2155,81 +2155,35 @@ func (r *REST) warnLegacyPresets(app *appsv1alpha1.Application) {
21552155
}
21562156
}
21572157

2158-
// kubernetesKind is the Application.Kind of the Kubernetes cluster CR.
2158+
// kubernetesKind is the Application.Kind of the Kubernetes cluster CR whose
2159+
// Phase 2-removed value keys the warning below reports on.
21592160
const kubernetesKind = "Kubernetes"
21602161

2161-
// removedField is a values key whose chart has stopped reading it. Such a key
2162-
// is still accepted and stored -- an upgraded release keeps it until re-edited,
2163-
// and the migrations deliberately leave it in place rather than rewriting
2164-
// values mid-upgrade -- so the write path says so rather than letting the field
2165-
// look effective.
2166-
type removedField struct {
2167-
key string
2168-
// replacement says what happens instead, in the second half of the warning.
2169-
replacement string
2170-
// emptyDefault marks a key that is set only when it carries a value. The
2171-
// image overrides defaulted to empty strings, the read path fills defaults
2172-
// in, and a client that writes back what it read stores them, so their
2173-
// presence says nothing.
2174-
emptyDefault bool
2175-
}
2176-
2177-
var removedFieldsByKind = map[string][]removedField{
2178-
kubernetesKind: {
2179-
{key: "nodeGroups", replacement: "worker pools are managed as separate KubernetesNodes resources (see the kubernetes-nodes chart)"},
2180-
{key: "nodeHealthCheck", replacement: "worker pools are managed as separate KubernetesNodes resources (see the kubernetes-nodes chart)"},
2181-
{key: "maxNodeProvisionTime", replacement: "worker pools are managed as separate KubernetesNodes resources (see the kubernetes-nodes chart)"},
2182-
{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},
2183-
},
2184-
"KubernetesNodes": {
2185-
{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},
2186-
},
2187-
"OpenSearch": {
2188-
{key: "images", replacement: "the image follows spec.version", emptyDefault: true},
2189-
},
2190-
}
2191-
2192-
// warnRemovedFields emits a client-facing admission warning for each removed
2193-
// field still present in the values, so an operator editing one (e.g. bumping
2194-
// spec.nodeGroups.<x>.minReplicas) is told it has no effect instead of silently
2195-
// getting nothing. Non-blocking: the field is preserved as-is.
2196-
func (r *REST) warnRemovedFields(ctx context.Context, app *appsv1alpha1.Application) {
2197-
fields, ok := removedFieldsByKind[r.kindName]
2198-
if !ok || app == nil || app.Spec == nil || len(app.Spec.Raw) == 0 {
2162+
// removedKubernetesFields are Kubernetes CR value keys that Phase 2 moved to the
2163+
// separate KubernetesNodes resource. They are still accepted and stored -- an
2164+
// upgraded cluster keeps them in its values until re-edited, and the adoption
2165+
// migration deliberately leaves them in place rather than rewriting parent
2166+
// values mid-upgrade -- but they no longer render anything on the Kubernetes CR.
2167+
var removedKubernetesFields = []string{"nodeGroups", "nodeHealthCheck", "maxNodeProvisionTime"}
2168+
2169+
// warnRemovedKubernetesFields emits a client-facing admission warning for each
2170+
// Phase 2-removed field still present in a Kubernetes CR's values, so an
2171+
// operator editing one (e.g. bumping spec.nodeGroups.<x>.minReplicas) is told it
2172+
// has no effect instead of silently getting nothing. Non-blocking: the field is
2173+
// preserved as-is; worker pools are managed as KubernetesNodes resources.
2174+
func (r *REST) warnRemovedKubernetesFields(ctx context.Context, app *appsv1alpha1.Application) {
2175+
if r.kindName != kubernetesKind || app == nil || app.Spec == nil || len(app.Spec.Raw) == 0 {
21992176
return
22002177
}
22012178
var values map[string]any
22022179
if err := json.Unmarshal(app.Spec.Raw, &values); err != nil {
22032180
return
22042181
}
2205-
for _, f := range fields {
2206-
value, present := values[f.key]
2207-
if !present || (f.emptyDefault && !carriesValue(value)) {
2208-
continue
2209-
}
2210-
warning.AddWarning(ctx, "", fmt.Sprintf(
2211-
"spec.%s is ignored on the %s resource: %s. The field is stored but has no effect.",
2212-
f.key, r.kindName, f.replacement))
2213-
}
2214-
}
2215-
2216-
// carriesValue reports whether a value is more than an empty default: not null,
2217-
// not an empty string, and for a map, at least one entry that carries a value.
2218-
func carriesValue(value any) bool {
2219-
switch v := value.(type) {
2220-
case nil:
2221-
return false
2222-
case string:
2223-
return v != ""
2224-
case map[string]any:
2225-
for _, nested := range v {
2226-
if carriesValue(nested) {
2227-
return true
2228-
}
2182+
for _, key := range removedKubernetesFields {
2183+
if _, present := values[key]; present {
2184+
warning.AddWarning(ctx, "", fmt.Sprintf(
2185+
"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))
22292186
}
2230-
return false
2231-
default:
2232-
return true
22332187
}
22342188
}
22352189

‎pkg/registry/apps/application/rest_validation_test.go‎

Lines changed: 6 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -443,11 +443,11 @@ type fakeWarningRecorder struct{ warnings []string }
443443

444444
func (f *fakeWarningRecorder) AddWarning(_, text string) { f.warnings = append(f.warnings, text) }
445445

446-
// TestWarnRemovedFields covers the inert-field warning: a release that still
447-
// carries a key its chart has stopped reading is accepted (the migrations leave
448-
// those keys in place) but the operator is warned that it no longer takes
449-
// effect. A kind with no removed fields never warns.
450-
func TestWarnRemovedFields(t *testing.T) {
446+
// TestWarnRemovedKubernetesFields covers the Phase 2 inert-field warning: a
447+
// Kubernetes CR that still carries nodeGroups / nodeHealthCheck /
448+
// maxNodeProvisionTime is accepted (the migration leaves them in place) but the
449+
// operator is warned that they no longer take effect. Other kinds never warn.
450+
func TestWarnRemovedKubernetesFields(t *testing.T) {
451451
tests := []struct {
452452
name string
453453
kindName string
@@ -478,36 +478,6 @@ func TestWarnRemovedFields(t *testing.T) {
478478
specJSON: `{"nodeGroups":{"md0":{}}}`,
479479
wantKeys: nil,
480480
},
481-
{
482-
name: "kubernetes with the removed image overrides warns",
483-
kindName: "Kubernetes",
484-
specJSON: `{"images":{"kubectl":"example.test/kubectl:1"}}`,
485-
wantKeys: []string{"images"},
486-
},
487-
{
488-
name: "kubernetes with the defaulted empty image overrides does not warn",
489-
kindName: "Kubernetes",
490-
specJSON: `{"images":{"kubectl":"","talosCsrSigner":"","waitForKubeconfig":""}}`,
491-
wantKeys: nil,
492-
},
493-
{
494-
name: "opensearch with the defaulted empty image override does not warn",
495-
kindName: "OpenSearch",
496-
specJSON: `{"version":"v2","images":{"opensearch":""}}`,
497-
wantKeys: nil,
498-
},
499-
{
500-
name: "opensearch with the removed image override warns",
501-
kindName: "OpenSearch",
502-
specJSON: `{"version":"v2","images":{"opensearch":"example.test/opensearch:1"}}`,
503-
wantKeys: []string{"images"},
504-
},
505-
{
506-
name: "foundationdb legacy version is handled by the chart",
507-
kindName: "FoundationDB",
508-
specJSON: `{"version":"v7.3","cluster":{"version":"7.1.67","redundancyMode":"double"}}`,
509-
wantKeys: nil,
510-
},
511481
}
512482

513483
for _, tt := range tests {
@@ -517,7 +487,7 @@ func TestWarnRemovedFields(t *testing.T) {
517487
r := &REST{kindName: tt.kindName}
518488
app := &appsv1alpha1.Application{Spec: &apiextv1.JSON{Raw: []byte(tt.specJSON)}}
519489

520-
r.warnRemovedFields(ctx, app)
490+
r.warnRemovedKubernetesFields(ctx, app)
521491

522492
if len(rec.warnings) != len(tt.wantKeys) {
523493
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‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ import (
3636
"github.com/cozystack/cozystack/pkg/config"
3737
)
3838

39-
// The inert-field warning is only useful if it is actually wired into
40-
// the write path. TestWarnRemovedFields (rest_validation_test.go)
39+
// The Phase 2 inert-field warning is only useful if it is actually wired into
40+
// the write path. TestWarnRemovedKubernetesFields (rest_validation_test.go)
4141
// pins the helper in isolation, but deleting either call site
4242
// (rest.go Create / Update) leaves that test green while an operator editing a
4343
// removed field silently gets no warning. These two tests drive Create and
@@ -99,7 +99,7 @@ func TestCreate_WarnsOnRemovedKubernetesFields(t *testing.T) {
9999
ctx := warning.WithWarningRecorder(request.WithNamespace(context.Background(), "tenant-foo"), rec)
100100

101101
// Short-circuit right after the warning (createValidation runs immediately
102-
// after warnRemovedFields), so the test does not depend on
102+
// after warnRemovedKubernetesFields), so the test does not depend on
103103
// conversion or the fake client's write path.
104104
sentinel := errors.New("stop after warning")
105105
createValidation := func(_ context.Context, _ runtime.Object) error { return sentinel }
@@ -128,7 +128,7 @@ func TestUpdate_WarnsOnRemovedKubernetesFields(t *testing.T) {
128128
rec := &fakeWarningRecorder{}
129129
ctx := warning.WithWarningRecorder(request.WithNamespace(context.Background(), "tenant-foo"), rec)
130130

131-
// updateValidation runs BEFORE warnRemovedFields, so it must pass
131+
// updateValidation runs BEFORE warnRemovedKubernetesFields, so it must pass
132132
// (return nil) for the warning to be reached. The final Update result is
133133
// irrelevant — the warning is recorded before any conversion/write — so any
134134
// downstream error is ignored; only the recorded warning is asserted.

0 commit comments

Comments
 (0)