From c7545d412aa40cb6798e7228456442cda91f7257 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Sun, 6 Sep 2026 10:48:27 +0200 Subject: [PATCH] fix(api): drop the enum from a status field ScaledDownWorkloadRef.Kind carried an enum constraint, and the API server validates status subresource writes. A kind outside the list would therefore reject the entire status update, not just that entry: conditions, phase and counters would all stop being written, and the symptom would look nothing like the cause. The same constraint on spec.targets is correct and stays. Validate what users write, describe what the controller reports. Verified both directions against a real API server: the status now takes an unlisted kind with the rest of the update intact, and the spec still rejects one. Reinstating the marker makes the new test fail with the whole write rejected, which is the behaviour being guarded against. Closes #66 Signed-off-by: Simon Lauger --- api/v1alpha1/crashlooppolicy_envtest_test.go | 46 +++++++++++++++++++ api/v1alpha1/crashlooppolicy_types.go | 7 ++- ...-operator.lauger.de_crashlooppolicies.yaml | 12 ++--- ...-operator.lauger.de_crashlooppolicies.yaml | 12 ++--- 4 files changed, 63 insertions(+), 14 deletions(-) diff --git a/api/v1alpha1/crashlooppolicy_envtest_test.go b/api/v1alpha1/crashlooppolicy_envtest_test.go index e680dfc..9c2557e 100644 --- a/api/v1alpha1/crashlooppolicy_envtest_test.go +++ b/api/v1alpha1/crashlooppolicy_envtest_test.go @@ -309,3 +309,49 @@ func TestSampleManifestsAreValid(t *testing.T) { t.Fatalf("no sample manifests found in %s", dir) } } + +// TestStatusAcceptsAnyWorkloadKind guards against reintroducing an enum on the +// status field. The API server validates status writes, so a constrained value +// there would reject the whole update, not just the offending entry, and every +// condition and counter would silently stop being written. +func TestStatusAcceptsAnyWorkloadKind(t *testing.T) { + ctx := context.Background() + p := newPolicy("status-kind-test") + if err := k8sClient.Create(ctx, p); err != nil { + t.Fatalf("failed to create policy: %v", err) + } + t.Cleanup(func() { _ = k8sClient.Delete(ctx, p) }) + + p.Status.Phase = CrashLoopPolicyPhaseActive + p.Status.ActiveScaledDown = []ScaledDownWorkloadRef{ + {Kind: "Deployment", Namespace: "default", Name: "known"}, + {Kind: "SomeFutureKind", Namespace: "default", Name: "unknown"}, + } + if err := k8sClient.Status().Update(ctx, p); err != nil { + t.Fatalf("status write rejected for an unlisted kind: %v", err) + } + + fetched := &CrashLoopPolicy{} + if err := k8sClient.Get(ctx, client.ObjectKey{Name: "status-kind-test"}, fetched); err != nil { + t.Fatalf("failed to get policy: %v", err) + } + // The whole update must have landed, not just the recognised entry. + if len(fetched.Status.ActiveScaledDown) != 2 { + t.Errorf("expected both entries to persist, got %d", len(fetched.Status.ActiveScaledDown)) + } + if fetched.Status.Phase != CrashLoopPolicyPhaseActive { + t.Errorf("expected the rest of the status update to land, phase is %q", fetched.Status.Phase) + } +} + +// TestSpecStillRejectsUnknownTarget is the other half: the enum on the spec is +// the one that should stay, so a user typo is caught at write time. +func TestSpecStillRejectsUnknownTarget(t *testing.T) { + ctx := context.Background() + p := newPolicy("spec-target-test") + p.Spec.Targets = []string{"SomeFutureKind"} + if err := k8sClient.Create(ctx, p); err == nil { + t.Error("expected an unknown target kind to be rejected on the spec") + _ = k8sClient.Delete(ctx, p) + } +} diff --git a/api/v1alpha1/crashlooppolicy_types.go b/api/v1alpha1/crashlooppolicy_types.go index 13cd56c..15cf0f4 100644 --- a/api/v1alpha1/crashlooppolicy_types.go +++ b/api/v1alpha1/crashlooppolicy_types.go @@ -74,8 +74,11 @@ type CrashLoopPolicySpec struct { // ScaledDownWorkloadRef identifies a workload currently held at zero replicas // (or suspended, for CronJobs) by this policy. type ScaledDownWorkloadRef struct { - // Kind of the workload: Deployment, StatefulSet or CronJob. - // +kubebuilder:validation:Enum=Deployment;StatefulSet;CronJob + // Kind of the workload, for example Deployment, StatefulSet or CronJob. + // Deliberately not constrained by an enum: this is a status field, and the + // API server validates status writes, so a value outside the list would + // reject the entire status update rather than just this entry. Validate + // what users write, describe what the controller reports. Kind string `json:"kind"` // Namespace of the workload. diff --git a/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml b/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml index e3d698c..38c0786 100644 --- a/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml +++ b/charts/crashloop-operator/crds/crashloop-operator.lauger.de_crashlooppolicies.yaml @@ -237,12 +237,12 @@ spec: (or suspended, for CronJobs) by this policy. properties: kind: - description: 'Kind of the workload: Deployment, StatefulSet - or CronJob.' - enum: - - Deployment - - StatefulSet - - CronJob + description: |- + Kind of the workload, for example Deployment, StatefulSet or CronJob. + Deliberately not constrained by an enum: this is a status field, and the + API server validates status writes, so a value outside the list would + reject the entire status update rather than just this entry. Validate + what users write, describe what the controller reports. type: string name: description: Name of the workload. diff --git a/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml b/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml index e3d698c..38c0786 100644 --- a/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml +++ b/config/crd/bases/crashloop-operator.lauger.de_crashlooppolicies.yaml @@ -237,12 +237,12 @@ spec: (or suspended, for CronJobs) by this policy. properties: kind: - description: 'Kind of the workload: Deployment, StatefulSet - or CronJob.' - enum: - - Deployment - - StatefulSet - - CronJob + description: |- + Kind of the workload, for example Deployment, StatefulSet or CronJob. + Deliberately not constrained by an enum: this is a status field, and the + API server validates status writes, so a value outside the list would + reject the entire status update rather than just this entry. Validate + what users write, describe what the controller reports. type: string name: description: Name of the workload.