From 4b15a4d053ebc3b923868b005c8174b3f9afd09d Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Wed, 21 Oct 2026 19:49:45 +0200 Subject: [PATCH 1/5] Enforce canonical ClusterObjectSet revision names Require ClusterObjectSet names to match - so Kubernetes name uniqueness also guarantees unique revision numbers within a group. Limit revisions to 9999999999 so canonical names fit in label values with groups up to 52 characters. Require callers to set names explicitly and document that generated names are unsupported. Exercise canonical name admission, required fields, revision bounds, and update validation with explicit fixtures and targeted errors. Adapt standalone fixtures and examples, and regenerate experimental artifacts. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectdeployment_types.go | 2 +- api/v1/clusterobjectset_types.go | 14 +- api/v1/clusterobjectset_types_test.go | 430 ++++++++++++------ api/v1/validation_test.go | 48 +- .../api/v1/clusterobjectdeployment.go | 2 +- .../api/v1/clusterobjectset.go | 3 + .../api/v1/clusterobjectsetspec.go | 4 +- cmd/object-controller/main_test.go | 2 +- docs/draft/concepts/clusterobjectsets.md | 30 +- ...peratorframework.io_clusterobjectsets.yaml | 11 +- .../boxcutter_reconcile_steps_test.go | 14 +- manifests/experimental-e2e.yaml | 11 +- manifests/experimental.yaml | 11 +- .../cos_object_collision_and_adoption.feature | 24 +- test/e2e/features/group.feature | 2 +- test/e2e/steps/hooks.go | 2 +- 16 files changed, 396 insertions(+), 214 deletions(-) diff --git a/api/v1/clusterobjectdeployment_types.go b/api/v1/clusterobjectdeployment_types.go index b7956ce46f..c67ec00c9c 100644 --- a/api/v1/clusterobjectdeployment_types.go +++ b/api/v1/clusterobjectdeployment_types.go @@ -133,7 +133,7 @@ type ClusterObjectDeployment struct { metav1.TypeMeta `json:",inline"` // metadata is the standard object's metadata. Names are valid COS groups of at - // most 52 characters; generated child names also include an int64 revision suffix. + // most 52 characters; child names also include a hyphen and up to ten revision digits. // +optional metav1.ObjectMeta `json:"metadata,omitempty"` diff --git a/api/v1/clusterobjectset_types.go b/api/v1/clusterobjectset_types.go index 377aa81633..1b6a7c048f 100644 --- a/api/v1/clusterobjectset_types.go +++ b/api/v1/clusterobjectset_types.go @@ -25,6 +25,11 @@ import ( const ( ClusterObjectSetKind = "ClusterObjectSet" + // ClusterObjectSetMaxRevision is the largest revision whose canonical name fits + // in a 63-character label value with a 52-character group and a separating hyphen. + // Keep the revision validation maximum below in sync with this value. + ClusterObjectSetMaxRevision int64 = 9999999999 + // Condition Types ClusterObjectSetTypeReady = "Ready" @@ -75,11 +80,12 @@ type ClusterObjectSetSpec struct { // revision is a required, immutable sequence number identifying a specific // ClusterObjectSet within a sequence of related revisions. // - // The revision field must be a positive integer. - // Each ClusterObjectSet in the same revision sequence must have a unique revision number. + // The revision field must be an integer from 1 to 9999999999, inclusive. + // Each ClusterObjectSet in the same group must have a unique revision number. // // +required // +kubebuilder:validation:Minimum:=1 + // +kubebuilder:validation:Maximum:=9999999999 // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="revision is immutable" Revision int64 `json:"revision"` @@ -627,6 +633,7 @@ type ObjectCounts struct { // +kubebuilder:object:root=true // +kubebuilder:resource:scope=Cluster // +kubebuilder:subresource:status +// +kubebuilder:validation:XValidation:rule="self.metadata.name == '%s-%d'.format([self.spec.group, self.spec.revision])",message="metadata.name must be -" // +kubebuilder:selectablefield:JSONPath=`.spec.group` // +kubebuilder:printcolumn:name="Group",type=string,JSONPath=`.spec.group` // +kubebuilder:printcolumn:name="Revision",type=integer,JSONPath=`.spec.revision` @@ -644,6 +651,9 @@ type ObjectCounts struct { // single revision at a time. Ownership of objects is transitioned from one revision // to the next as new revisions are rolled out. Once the latest revision has rolled // out successfully, previous active revisions are archived for posterity. +// Set metadata.name explicitly to -, using decimal +// notation without leading zeros. This ensures revision uniqueness within a group. +// Name generation through metadata.generateName is not supported. type ClusterObjectSet struct { metav1.TypeMeta `json:",inline"` diff --git a/api/v1/clusterobjectset_types_test.go b/api/v1/clusterobjectset_types_test.go index f2978a1d9e..afa64038bc 100644 --- a/api/v1/clusterobjectset_types_test.go +++ b/api/v1/clusterobjectset_types_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "math" "strings" "testing" @@ -18,7 +19,6 @@ import ( func TestClusterObjectSetImmutability(t *testing.T) { c := newClient(t) ctx := context.Background() - i := 0 for name, tc := range map[string]struct { spec ClusterObjectSetSpec updateFunc func(*ClusterObjectSet) @@ -27,7 +27,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }{ "group is immutable": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-group-change", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -39,7 +39,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "group cannot be cleared": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-group-cleared", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -51,20 +51,19 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "unchanged group permits lifecycle update": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-lifecycle-update", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, }, updateFunc: func(cos *ClusterObjectSet) { - cos.Spec.Group = "test-group" cos.Spec.LifecycleState = ClusterObjectSetLifecycleStateArchived }, allowed: true, }, "revision is immutable": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-revision-change", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -72,10 +71,11 @@ func TestClusterObjectSetImmutability(t *testing.T) { updateFunc: func(cos *ClusterObjectSet) { cos.Spec.Revision = 2 }, + expectedError: "revision is immutable", }, "phases may be initially empty": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-empty-phases", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -93,7 +93,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "phases may be initially unset": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-unset-phases", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -110,7 +110,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "phases are immutable if not empty": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-populated-phases", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -129,10 +129,11 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, } }, + expectedError: "phases is immutable", }, "spec collisionProtection is immutable": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-collision-protection", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -140,10 +141,11 @@ func TestClusterObjectSetImmutability(t *testing.T) { updateFunc: func(cos *ClusterObjectSet) { cos.Spec.CollisionProtection = CollisionProtectionNone }, + expectedError: "collisionProtection is immutable", }, "progressionProbes may be initially unset": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-unset-probes", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -155,7 +157,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "progressionProbes are immutable once set": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-populated-probes", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -168,7 +170,7 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, "lifecycleState cannot be un-archived": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-unarchive", LifecycleState: ClusterObjectSetLifecycleStateArchived, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -176,10 +178,11 @@ func TestClusterObjectSetImmutability(t *testing.T) { updateFunc: func(cos *ClusterObjectSet) { cos.Spec.LifecycleState = ClusterObjectSetLifecycleStateActive }, + expectedError: "cannot un-archive", }, "lifecycleState can transition from active to archived": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "immutability-archive", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -189,27 +192,37 @@ func TestClusterObjectSetImmutability(t *testing.T) { }, allowed: true, }, + "metadata can be updated": { + spec: ClusterObjectSetSpec{ + Group: "immutability-metadata", + Revision: 1, + LifecycleState: ClusterObjectSetLifecycleStateActive, + CollisionProtection: CollisionProtectionPrevent, + }, + updateFunc: func(cos *ClusterObjectSet) { + cos.Labels = map[string]string{"test": "updated"} + cos.Annotations = map[string]string{"test": "updated"} + }, + allowed: true, + }, } { t.Run(name, func(t *testing.T) { cos := &ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("foo%d", i), + Name: fmt.Sprintf("%s-%d", tc.spec.Group, tc.spec.Revision), }, Spec: tc.spec, } - i = i + 1 require.NoError(t, c.Create(ctx, cos)) tc.updateFunc(cos) err := c.Update(ctx, cos) - if tc.allowed && err != nil { - t.Fatal("expected update to succeed, but got:", err) - } - if !tc.allowed && !errors.IsInvalid(err) { - t.Fatal("expected update to fail due to invalid payload, but got:", err) - } - if tc.expectedError != "" { - require.ErrorContains(t, err, tc.expectedError) + if tc.allowed { + require.NoError(t, err) + return } + require.True(t, errors.IsInvalid(err), "%v", err) + require.NotEmpty(t, tc.expectedError) + require.ErrorContains(t, err, tc.expectedError) }) } } @@ -217,107 +230,109 @@ func TestClusterObjectSetImmutability(t *testing.T) { func TestClusterObjectSetValidity(t *testing.T) { c := newClient(t) ctx := context.Background() - i := 0 for name, tc := range map[string]struct { - spec ClusterObjectSetSpec - valid bool + spec ClusterObjectSetSpec + valid bool + expectedError string }{ - "revision cannot be negative": { - spec: ClusterObjectSetSpec{ - Group: "validity-negative-revision", - LifecycleState: ClusterObjectSetLifecycleStateActive, - Revision: -1, - }, - valid: false, - }, - "revision cannot be zero": { - spec: ClusterObjectSetSpec{ - Group: "validity-zero-revision", - LifecycleState: ClusterObjectSetLifecycleStateActive, - }, - valid: false, - }, - "revision must be positive": { - spec: ClusterObjectSetSpec{ - Group: "validity-positive-revision", - LifecycleState: ClusterObjectSetLifecycleStateActive, - Revision: 1, - CollisionProtection: CollisionProtectionPrevent, - }, - valid: true, - }, "lifecycleState must be set": { spec: ClusterObjectSetSpec{ - Group: "validity-missing-lifecycle", - Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Group: "validity-missing-lifecycle", + Revision: 1, }, - valid: false, + valid: false, + expectedError: "spec.lifecycleState: Required value", }, "phases must have no more than 20 phases": { spec: ClusterObjectSetSpec{ - Group: "validity-too-many-phases", - LifecycleState: ClusterObjectSetLifecycleStateActive, - Revision: 1, - Phases: make([]ClusterObjectSetPhase, 21), + CollisionProtection: CollisionProtectionPrevent, + Group: "validity-too-many-phases", + LifecycleState: ClusterObjectSetLifecycleStateActive, + Revision: 1, + Phases: func() []ClusterObjectSetPhase { + phases := make([]ClusterObjectSetPhase, 21) + for i := range phases { + phases[i] = ClusterObjectSetPhase{Name: fmt.Sprintf("phase-%d", i), Objects: []ClusterObjectSetObject{}} + } + return phases + }(), }, - valid: false, + valid: false, + expectedError: "spec.phases: Too many", }, "phases entries must have no more than 50 objects": { spec: ClusterObjectSetSpec{ - Group: "validity-too-many-objects", - LifecycleState: ClusterObjectSetLifecycleStateActive, - Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Group: "validity-too-many-objects", + LifecycleState: ClusterObjectSetLifecycleStateActive, + Revision: 1, Phases: []ClusterObjectSetPhase{ { - Name: "too-many-objects", - Objects: make([]ClusterObjectSetObject, 51), + Name: "too-many-objects", + Objects: func() []ClusterObjectSetObject { + objects := make([]ClusterObjectSetObject, 51) + for i := range objects { + objects[i] = ClusterObjectSetObject{Object: configMap()} + } + return objects + }(), }, }, }, - valid: false, + valid: false, + expectedError: "spec.phases[0].objects: Too many", }, "phases entry names cannot be empty": { spec: ClusterObjectSetSpec{ - Group: "validity-empty-phase-name", - LifecycleState: ClusterObjectSetLifecycleStateActive, - Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Group: "validity-empty-phase-name", + LifecycleState: ClusterObjectSetLifecycleStateActive, + Revision: 1, Phases: []ClusterObjectSetPhase{ { - Name: "", + Name: "", + Objects: []ClusterObjectSetObject{}, }, }, }, - valid: false, + valid: false, + expectedError: "spec.phases[0].name: Invalid value", }, "phases entry names cannot start with symbols": { spec: ClusterObjectSetSpec{ - Group: "validity-symbol-phase-name", - LifecycleState: ClusterObjectSetLifecycleStateActive, - Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Group: "validity-symbol-phase-name", + LifecycleState: ClusterObjectSetLifecycleStateActive, + Revision: 1, Phases: []ClusterObjectSetPhase{ { - Name: "-invalid", + Name: "-invalid", + Objects: []ClusterObjectSetObject{}, }, }, }, - valid: false, + valid: false, + expectedError: "the value must consist of only lowercase alphanumeric", }, - "phases entry names cannot start with numeric characters": { + "phases entry names can start with numeric characters": { spec: ClusterObjectSetSpec{ - Group: "validity-numeric-phase-name", - LifecycleState: ClusterObjectSetLifecycleStateActive, - Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Group: "validity-numeric-phase-name", + LifecycleState: ClusterObjectSetLifecycleStateActive, + Revision: 1, Phases: []ClusterObjectSetPhase{ { - Name: "1-invalid", + Name: "1-valid", + Objects: []ClusterObjectSetObject{}, }, }, }, - valid: false, + valid: true, }, "phases entry names cannot exceed 63 characters": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "validity-case-10", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -328,11 +343,12 @@ func TestClusterObjectSetValidity(t *testing.T) { }, }, }, - valid: false, + valid: false, + expectedError: "spec.phases[0].name: Too long", }, "phases entry names must be unique": { spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "validity-case-11", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -347,7 +363,8 @@ func TestClusterObjectSetValidity(t *testing.T) { }, }, }, - valid: false, + valid: false, + expectedError: "spec.phases[1]: Duplicate value", }, "spec collisionProtection accepts Prevent": { spec: ClusterObjectSetSpec{ @@ -382,7 +399,8 @@ func TestClusterObjectSetValidity(t *testing.T) { LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, }, - valid: false, + valid: false, + expectedError: "spec.collisionProtection: Required value", }, "spec collisionProtection rejects invalid values": { spec: ClusterObjectSetSpec{ @@ -391,7 +409,8 @@ func TestClusterObjectSetValidity(t *testing.T) { Revision: 1, CollisionProtection: CollisionProtection("Invalid"), }, - valid: false, + valid: false, + expectedError: "spec.collisionProtection: Unsupported value", }, "spec collisionProtection must be set": { spec: ClusterObjectSetSpec{ @@ -399,7 +418,8 @@ func TestClusterObjectSetValidity(t *testing.T) { LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, }, - valid: false, + valid: false, + expectedError: "spec.collisionProtection: Required value", }, "object collisionProtection is optional": { spec: ClusterObjectSetSpec{ @@ -459,20 +479,24 @@ func TestClusterObjectSetValidity(t *testing.T) { valid: true, }, "object ref requires a name": { - spec: refSpec(ObjectSourceRef{Key: "my-key"}), - valid: false, + spec: refSpec("validity-case-21", ObjectSourceRef{Key: "my-key"}), + valid: false, + expectedError: "ref.name: Invalid value", }, "object ref requires a key": { - spec: refSpec(ObjectSourceRef{Name: "my-secret"}), - valid: false, + spec: refSpec("validity-case-22", ObjectSourceRef{Name: "my-secret"}), + valid: false, + expectedError: "ref.key: Invalid value", }, "object ref name cannot exceed 253 characters": { - spec: refSpec(ObjectSourceRef{Name: strings.Repeat("a", 254), Key: "my-key"}), - valid: false, + spec: refSpec("validity-case-23", ObjectSourceRef{Name: strings.Repeat("a", 254), Key: "my-key"}), + valid: false, + expectedError: "ref.name: Too long", }, "object ref namespace cannot exceed 63 characters": { - spec: refSpec(ObjectSourceRef{Name: "my-secret", Key: "my-key", Namespace: strings.Repeat("a", 64)}), - valid: false, + spec: refSpec("validity-case-24", ObjectSourceRef{Name: "my-secret", Key: "my-key", Namespace: strings.Repeat("a", 64)}), + valid: false, + expectedError: "ref.namespace: Too long", }, "object with both object and ref is invalid": { spec: ClusterObjectSetSpec{ @@ -492,7 +516,8 @@ func TestClusterObjectSetValidity(t *testing.T) { }, }, }, - valid: false, + valid: false, + expectedError: "exactly one of the fields in [object ref] must be set", }, "object with neither object nor ref is invalid": { spec: ClusterObjectSetSpec{ @@ -509,17 +534,18 @@ func TestClusterObjectSetValidity(t *testing.T) { }, }, }, - valid: false, + valid: false, + expectedError: "exactly one of the fields in [object ref] must be set", }, "progressionProbe with GroupKind selector and ConditionEqual assertion is valid": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-27", ProgressionProbe{ Selector: validGroupKindSelector(), Assertions: []Assertion{validConditionEqualAssertion()}, }), valid: true, }, "progressionProbe with Label selector and FieldValue assertion is valid": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-28", ProgressionProbe{ Selector: ObjectSelector{ Type: SelectorTypeLabel, Label: metav1.LabelSelector{MatchLabels: map[string]string{"app": "test"}}, @@ -534,21 +560,23 @@ func TestClusterObjectSetValidity(t *testing.T) { valid: true, }, "progressionProbe rejects invalid selector type": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-29", ProgressionProbe{ Selector: ObjectSelector{Type: SelectorType("Invalid")}, Assertions: []Assertion{validConditionEqualAssertion()}, }), - valid: false, + valid: false, + expectedError: "selector.type: Unsupported value", }, "progressionProbe GroupKind selector requires groupKind": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-30", ProgressionProbe{ Selector: ObjectSelector{Type: SelectorTypeGroupKind}, Assertions: []Assertion{validConditionEqualAssertion()}, }), - valid: false, + valid: false, + expectedError: "groupKind is required when type is GroupKind", }, "progressionProbe GroupKind selector forbids label": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-31", ProgressionProbe{ Selector: ObjectSelector{ Type: SelectorTypeGroupKind, GroupKind: metav1.GroupKind{Kind: "ConfigMap"}, @@ -556,17 +584,19 @@ func TestClusterObjectSetValidity(t *testing.T) { }, Assertions: []Assertion{validConditionEqualAssertion()}, }), - valid: false, + valid: false, + expectedError: "label is required when type is Label, and forbidden otherwise", }, "progressionProbe Label selector requires label": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-32", ProgressionProbe{ Selector: ObjectSelector{Type: SelectorTypeLabel}, Assertions: []Assertion{validConditionEqualAssertion()}, }), - valid: false, + valid: false, + expectedError: "label is required when type is Label", }, "progressionProbe Label selector rejects both matchLabels and matchExpressions": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-33", ProgressionProbe{ Selector: ObjectSelector{ Type: SelectorTypeLabel, Label: metav1.LabelSelector{ @@ -578,24 +608,27 @@ func TestClusterObjectSetValidity(t *testing.T) { }, Assertions: []Assertion{validConditionEqualAssertion()}, }), - valid: false, + valid: false, + expectedError: "exactly one of matchLabels or matchExpressions must be set", }, "progressionProbe rejects invalid assertion type": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-34", ProgressionProbe{ Selector: validGroupKindSelector(), Assertions: []Assertion{{Type: ProbeType("Invalid")}}, }), - valid: false, + valid: false, + expectedError: "assertions[0].type: Unsupported value", }, "progressionProbe ConditionEqual assertion requires conditionEqual": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-35", ProgressionProbe{ Selector: validGroupKindSelector(), Assertions: []Assertion{{Type: ProbeTypeConditionEqual}}, }), - valid: false, + valid: false, + expectedError: "conditionEqual is required when type is ConditionEqual", }, "progressionProbe assertion forbids mismatched member": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-36", ProgressionProbe{ Selector: validGroupKindSelector(), Assertions: []Assertion{ { @@ -605,10 +638,11 @@ func TestClusterObjectSetValidity(t *testing.T) { }, }, }), - valid: false, + valid: false, + expectedError: "conditionEqual is required when type is ConditionEqual, and forbidden otherwise", }, "progressionProbe ConditionEqual status must be True or False": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-37", ProgressionProbe{ Selector: validGroupKindSelector(), Assertions: []Assertion{ { @@ -617,10 +651,11 @@ func TestClusterObjectSetValidity(t *testing.T) { }, }, }), - valid: false, + valid: false, + expectedError: "conditionEqual.status: Unsupported value", }, "progressionProbe FieldValue rejects invalid field path": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-38", ProgressionProbe{ Selector: validGroupKindSelector(), Assertions: []Assertion{ { @@ -629,10 +664,11 @@ func TestClusterObjectSetValidity(t *testing.T) { }, }, }), - valid: false, + valid: false, + expectedError: "must contain a valid field path", }, "progressionProbe FieldsEqual rejects invalid field path": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-39", ProgressionProbe{ Selector: validGroupKindSelector(), Assertions: []Assertion{ { @@ -641,13 +677,15 @@ func TestClusterObjectSetValidity(t *testing.T) { }, }, }), - valid: false, + valid: false, + expectedError: "must contain a valid field path", }, "progressionProbe requires at least one assertion": { - spec: probeSpec(ProgressionProbe{ + spec: probeSpec("validity-case-40", ProgressionProbe{ Selector: validGroupKindSelector(), }), - valid: false, + valid: false, + expectedError: "assertions: Required value", }, "progressionProbes must have no more than 20 probes": { spec: func() ClusterObjectSetSpec { @@ -655,26 +693,27 @@ func TestClusterObjectSetValidity(t *testing.T) { for i := range probes { probes[i] = validProbe() } - return probeSpec(probes...) + return probeSpec("validity-case-41", probes...) }(), - valid: false, + valid: false, + expectedError: "spec.progressionProbes: Too many", }, } { t.Run(name, func(t *testing.T) { cos := &ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("cos-validity-%d", i), + Name: fmt.Sprintf("%s-%d", tc.spec.Group, tc.spec.Revision), }, Spec: tc.spec, } - i++ err := c.Create(ctx, cos) - if tc.valid && err != nil { - t.Fatal("expected create to succeed, but got:", err) - } - if !tc.valid && !errors.IsInvalid(err) { - t.Fatal("expected create to fail due to invalid payload, but got:", err) + if tc.valid { + require.NoError(t, err) + return } + require.True(t, errors.IsInvalid(err), "%v", err) + require.NotEmpty(t, tc.expectedError) + require.ErrorContains(t, err, tc.expectedError) }) } } @@ -693,25 +732,113 @@ func TestClusterObjectSetSpecValidation(t *testing.T) { }) } +func TestClusterObjectSetRevisionBounds(t *testing.T) { + c := newClient(t) + for _, tc := range []struct { + testName string + revision *int64 + valid bool + expectedError string + }{ + {testName: "missing", expectedError: "spec.revision: Required value"}, + {testName: "negative", revision: ptr.To(int64(-1)), expectedError: "spec.revision: Invalid value"}, + {testName: "zero", revision: ptr.To(int64(0)), expectedError: "spec.revision: Invalid value"}, + {testName: "minimum", revision: ptr.To(int64(1)), valid: true}, + {testName: "maximum", revision: ptr.To(ClusterObjectSetMaxRevision), valid: true}, + {testName: "above maximum", revision: ptr.To(ClusterObjectSetMaxRevision + 1), expectedError: "spec.revision: Invalid value"}, + {testName: "maximum int64", revision: ptr.To(int64(math.MaxInt64)), expectedError: "spec.revision: Invalid value"}, + } { + t.Run(tc.testName, func(t *testing.T) { + // Both accepted boundaries must fit with a maximum-length group. + group := "bounds-" + strings.Repeat("a", 45) + spec := map[string]any{ + "group": group, + "lifecycleState": string(ClusterObjectSetLifecycleStateActive), + "collisionProtection": string(CollisionProtectionPrevent), + } + name := group + "-1" + if tc.revision != nil { + spec["revision"] = *tc.revision + name = fmt.Sprintf("%s-%d", group, *tc.revision) + } + cos := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": GroupVersion.String(), + "kind": ClusterObjectSetKind, + "metadata": map[string]any{"name": name}, + "spec": spec, + }} + err := c.Create(t.Context(), cos) + if tc.valid { + require.NoError(t, err) + require.Len(t, group, 52) + require.LessOrEqual(t, len(cos.GetName()), 63) + return + } + require.True(t, errors.IsInvalid(err), "%v", err) + require.ErrorContains(t, err, tc.expectedError) + }) + } +} + +func TestClusterObjectSetCanonicalName(t *testing.T) { + c := newClient(t) + for _, tc := range []struct { + testName string + name string + generateName string + group string + revision int64 + valid bool + }{ + {testName: "canonical", name: "canonical-1", group: "canonical", revision: 1, valid: true}, + {testName: "different group", name: "other-group-1", group: "different-group", revision: 1, valid: false}, + {testName: "leading zero", name: "leading-zero-01", group: "leading-zero", revision: 1, valid: false}, + {testName: "wrong revision", name: "wrong-revision-2", group: "wrong-revision", revision: 1, valid: false}, + {testName: "explicit name takes precedence over generateName", name: "explicit-1", generateName: "unrelated-prefix-", group: "explicit", revision: 1, valid: true}, + // The prefix cannot produce the canonical name, regardless of the random suffix. + {testName: "server generated name", generateName: "unrelated-prefix-", group: "generated", revision: 1, valid: false}, + } { + t.Run(tc.testName, func(t *testing.T) { + cos := &ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{Name: tc.name, GenerateName: tc.generateName}, + Spec: ClusterObjectSetSpec{ + Group: tc.group, + Revision: tc.revision, + LifecycleState: ClusterObjectSetLifecycleStateActive, + CollisionProtection: CollisionProtectionPrevent, + }, + } + err := c.Create(t.Context(), cos) + if tc.valid { + require.NoError(t, err) + return + } + require.True(t, errors.IsInvalid(err), "%v", err) + require.ErrorContains(t, err, "metadata.name must be -") + }) + } +} + func TestClusterObjectSetGroupValidation(t *testing.T) { c := newClient(t) for i, tc := range []struct { - name string - group *string - valid bool + name string + group *string + valid bool + expectedError string }{ - {name: "missing group", valid: false}, - {name: "empty group", group: ptr.To(""), valid: false}, + {name: "missing group", expectedError: "spec.group: Required value"}, + {name: "empty group", group: ptr.To(""), expectedError: "spec.group: Invalid value"}, {name: "one character", group: ptr.To("a"), valid: true}, {name: "lowercase hyphens and digits", group: ptr.To("my-group-1"), valid: true}, {name: "maximum length", group: ptr.To(strings.Repeat("a", 52)), valid: true}, - {name: "over maximum length", group: ptr.To(strings.Repeat("a", 53)), valid: false}, - {name: "starts with digit", group: ptr.To("1group"), valid: false}, - {name: "starts with hyphen", group: ptr.To("-group"), valid: false}, - {name: "ends with hyphen", group: ptr.To("group-"), valid: false}, - {name: "uppercase", group: ptr.To("Group"), valid: false}, - {name: "underscore", group: ptr.To("my_group"), valid: false}, - {name: "dot", group: ptr.To("my.group"), valid: false}, + {name: "over maximum length", group: ptr.To(strings.Repeat("a", 53)), expectedError: "spec.group: Too long"}, + {name: "starts with digit", group: ptr.To("1group"), expectedError: "group must start with a lowercase letter"}, + {name: "starts with hyphen", group: ptr.To("-group"), expectedError: "group must start with a lowercase letter"}, + {name: "ends with hyphen", group: ptr.To("group-"), expectedError: "group must start with a lowercase letter"}, + {name: "uppercase", group: ptr.To("Group"), expectedError: "group must start with a lowercase letter"}, + {name: "underscore", group: ptr.To("my_group"), expectedError: "group must start with a lowercase letter"}, + {name: "dot", group: ptr.To("my.group"), expectedError: "group must start with a lowercase letter"}, } { t.Run(tc.name, func(t *testing.T) { objectName := fmt.Sprintf("group-validation-%d", i+1) @@ -724,7 +851,8 @@ func TestClusterObjectSetGroupValidation(t *testing.T) { "metadata": map[string]any{"name": objectName}, }} spec := map[string]any{ - "revision": int64(1), "lifecycleState": string(ClusterObjectSetLifecycleStateActive), + "revision": int64(1), + "lifecycleState": string(ClusterObjectSetLifecycleStateActive), "collisionProtection": string(CollisionProtectionPrevent), } if tc.group != nil { @@ -736,7 +864,7 @@ func TestClusterObjectSetGroupValidation(t *testing.T) { require.NoError(t, err) } else { require.True(t, errors.IsInvalid(err), "%v", err) - require.ErrorContains(t, err, "spec.group") + require.ErrorContains(t, err, tc.expectedError) } }) } @@ -788,9 +916,9 @@ func TestClusterObjectSetGroupFieldSelector(t *testing.T) { // probeSpec returns a minimal valid ClusterObjectSetSpec carrying the given progression probes, // so that each case only varies the probe under test. -func probeSpec(probes ...ProgressionProbe) ClusterObjectSetSpec { +func probeSpec(group string, probes ...ProgressionProbe) ClusterObjectSetSpec { return ClusterObjectSetSpec{ - Group: "test-group", + Group: group, LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, @@ -809,9 +937,9 @@ func validProbe() ProgressionProbe { // refSpec returns a minimal valid ClusterObjectSetSpec whose single phase object is sourced // from the given ref, so that each ObjectSourceRef case only varies the ref under test. -func refSpec(ref ObjectSourceRef) ClusterObjectSetSpec { +func refSpec(group string, ref ObjectSourceRef) ClusterObjectSetSpec { return ClusterObjectSetSpec{ - Group: "test-group", + Group: group, LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, CollisionProtection: CollisionProtectionPrevent, diff --git a/api/v1/validation_test.go b/api/v1/validation_test.go index eb12cc549e..fd947133ef 100644 --- a/api/v1/validation_test.go +++ b/api/v1/validation_test.go @@ -33,12 +33,6 @@ func TestValidate(t *testing.T) { } return s } - defaultRevisionSpec := func(s *ClusterObjectSetSpec) *ClusterObjectSetSpec { - s.Group = "test-group" - s.Revision = 1 - s.CollisionProtection = CollisionProtectionPrevent - return s - } defaultDeploymentSpec := func(s *ClusterObjectDeploymentSpec) *ClusterObjectDeploymentSpec { s.Template.Spec.CollisionProtection = CollisionProtectionPrevent s.Template.Spec.Phases = []ClusterObjectSetPhase{ @@ -99,6 +93,9 @@ func TestValidate(t *testing.T) { "ClusterObjectSet: invalid progress deadline < 10": { args: args{ object: ClusterObjectSetSpec{ + Group: "validation-1", + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, ProgressDeadlineMinutes: 9, }, @@ -108,6 +105,9 @@ func TestValidate(t *testing.T) { "ClusterObjectSet: valid progress deadline = 10": { args: args{ object: ClusterObjectSetSpec{ + Group: "validation-2", + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, ProgressDeadlineMinutes: 10, }, @@ -117,6 +117,9 @@ func TestValidate(t *testing.T) { "ClusterObjectSet: valid progress deadline = 360": { args: args{ object: ClusterObjectSetSpec{ + Group: "validation-3", + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, ProgressDeadlineMinutes: 360, }, @@ -126,6 +129,9 @@ func TestValidate(t *testing.T) { "ClusterObjectSet: valid progress deadline = 720": { args: args{ object: ClusterObjectSetSpec{ + Group: "validation-4", + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, ProgressDeadlineMinutes: 720, }, @@ -135,6 +141,9 @@ func TestValidate(t *testing.T) { "ClusterObjectSet: invalid progress deadline > 720": { args: args{ object: ClusterObjectSetSpec{ + Group: "validation-5", + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, ProgressDeadlineMinutes: 721, }, @@ -144,7 +153,10 @@ func TestValidate(t *testing.T) { "ClusterObjectSet: no progress deadline set": { args: args{ object: ClusterObjectSetSpec{ - LifecycleState: ClusterObjectSetLifecycleStateActive, + Group: "validation-6", + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + LifecycleState: ClusterObjectSetLifecycleStateActive, }, }, want: want{valid: true}, @@ -213,13 +225,10 @@ func TestValidate(t *testing.T) { case ClusterObjectSetSpec: cos := &ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("cos-%d", i), + Name: fmt.Sprintf("%s-%d", s.Group, s.Revision), }, Spec: s, } - if !tc.args.skipDefaulting { - defaultRevisionSpec(&cos.Spec) - } obj = cos case ClusterObjectDeploymentSpec: cod := &ClusterObjectDeployment{ @@ -240,8 +249,9 @@ func TestValidate(t *testing.T) { if tc.want.valid && err != nil { t.Fatal("expected create to succeed, but got:", err) } - if !tc.want.valid && !errors.IsInvalid(err) { - t.Fatal("expected create to fail due to invalid payload, but got:", err) + if !tc.want.valid { + require.True(t, errors.IsInvalid(err), "%v", err) + require.ErrorContains(t, err, "spec.progressDeadlineMinutes") } }) } @@ -251,9 +261,9 @@ func TestClusterObjectSetCompletedAtImmutable(t *testing.T) { c := newClient(t) cos := &ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{Name: "cos-completedat-immutable"}, + ObjectMeta: metav1.ObjectMeta{Name: "cos-completedat-immutable-1"}, Spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "cos-completedat-immutable", Revision: 1, CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, @@ -282,9 +292,9 @@ func TestClusterObjectSetCompletedAtCannotBeRemoved(t *testing.T) { c := newClient(t) cos := &ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{Name: "cos-completedat-noremove"}, + ObjectMeta: metav1.ObjectMeta{Name: "cos-completedat-noremove-1"}, Spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "cos-completedat-noremove", Revision: 1, CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, @@ -308,9 +318,9 @@ func TestClusterObjectSetDigestImmutable(t *testing.T) { c := newClient(t) cos := &ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{Name: "cos-digest-immutable"}, + ObjectMeta: metav1.ObjectMeta{Name: "cos-digest-immutable-1"}, Spec: ClusterObjectSetSpec{ - Group: "test-group", + Group: "cos-digest-immutable", Revision: 1, CollisionProtection: CollisionProtectionPrevent, LifecycleState: ClusterObjectSetLifecycleStateActive, diff --git a/applyconfigurations/api/v1/clusterobjectdeployment.go b/applyconfigurations/api/v1/clusterobjectdeployment.go index e967f68fa4..1acb77a831 100644 --- a/applyconfigurations/api/v1/clusterobjectdeployment.go +++ b/applyconfigurations/api/v1/clusterobjectdeployment.go @@ -35,7 +35,7 @@ import ( type ClusterObjectDeploymentApplyConfiguration struct { metav1.TypeMetaApplyConfiguration `json:",inline"` // metadata is the standard object's metadata. Names are valid COS groups of at - // most 52 characters; generated child names also include an int64 revision suffix. + // most 52 characters; child names also include a hyphen and up to ten revision digits. *metav1.ObjectMetaApplyConfiguration `json:"metadata,omitempty"` // spec defines the desired deployment template. Spec *ClusterObjectDeploymentSpecApplyConfiguration `json:"spec,omitempty"` diff --git a/applyconfigurations/api/v1/clusterobjectset.go b/applyconfigurations/api/v1/clusterobjectset.go index 57a89c5464..d77a178bde 100644 --- a/applyconfigurations/api/v1/clusterobjectset.go +++ b/applyconfigurations/api/v1/clusterobjectset.go @@ -36,6 +36,9 @@ import ( // single revision at a time. Ownership of objects is transitioned from one revision // to the next as new revisions are rolled out. Once the latest revision has rolled // out successfully, previous active revisions are archived for posterity. +// Set metadata.name explicitly to -, using decimal +// notation without leading zeros. This ensures revision uniqueness within a group. +// Name generation through metadata.generateName is not supported. type ClusterObjectSetApplyConfiguration struct { metav1.TypeMetaApplyConfiguration `json:",inline"` // metadata is the standard object's metadata. diff --git a/applyconfigurations/api/v1/clusterobjectsetspec.go b/applyconfigurations/api/v1/clusterobjectsetspec.go index 5dfb42e09b..cdbb97bc22 100644 --- a/applyconfigurations/api/v1/clusterobjectsetspec.go +++ b/applyconfigurations/api/v1/clusterobjectsetspec.go @@ -48,8 +48,8 @@ type ClusterObjectSetSpecApplyConfiguration struct { // revision is a required, immutable sequence number identifying a specific // ClusterObjectSet within a sequence of related revisions. // - // The revision field must be a positive integer. - // Each ClusterObjectSet in the same revision sequence must have a unique revision number. + // The revision field must be an integer from 1 to 9999999999, inclusive. + // Each ClusterObjectSet in the same group must have a unique revision number. Revision *int64 `json:"revision,omitempty"` // phases is an optional, immutable list of phases that group objects to be applied together. // diff --git a/cmd/object-controller/main_test.go b/cmd/object-controller/main_test.go index b82f730028..6fea75f96b 100644 --- a/cmd/object-controller/main_test.go +++ b/cmd/object-controller/main_test.go @@ -114,7 +114,7 @@ func TestStandaloneController(t *testing.T) { obj = ocv1.ClusterObjectSetObject{Ref: ocv1.ObjectSourceRef{Name: secret.Name, Namespace: secret.Namespace, Key: "object"}} } cos := &ocv1.ClusterObjectSet{ - ObjectMeta: metav1.ObjectMeta{Name: name}, + ObjectMeta: metav1.ObjectMeta{Name: name + "-1"}, Spec: ocv1.ClusterObjectSetSpec{ Group: name, LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, Revision: 1, diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index 50f4999aca..dbaabd6592 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -23,6 +23,12 @@ ClusterObjectSets can be used by any controller or system that needs to manage t The `group` value must be 1–52 characters long, start with a lowercase letter, contain only lowercase letters, digits or hyphens, and end with a letter or digit. +Set `metadata.name` explicitly to `-`, with the revision in decimal +and without leading zeros. Revisions range from 1 to 9999999999, inclusive: ten digits plus +the separating hyphen fit within a 63-character name even for a 52-character group. +This naming rule ensures that a group cannot contain duplicate revision numbers. +Name generation through `metadata.generateName` is not supported. + Revisions in the same group can transfer managed objects between them even when they have different controller owners or no owner references. They share the server-side apply field manager `cos-group/`. Different groups remain @@ -255,6 +261,10 @@ When a ClusterExtension is upgraded to a new version: 4. Once the new revision has rolled out successfully, the old revision is archived 5. Older archived revisions are garbage collected, keeping the last 5 for auditing +Once a group reaches revision 9999999999, operator-controller reports an error +instead of allocating another revision, before pruning history or creating new +Secrets or ClusterObjectSets. Reconciliation of unchanged content still works. + ### Relationship with ClusterExtension A `ClusterExtension` owns one or more `ClusterObjectSet` resources. @@ -284,7 +294,7 @@ This example shows a ClusterObjectSet with objects embedded directly in the spec apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: my-app-rev1 + name: my-app-1 spec: group: my-app revision: 1 @@ -365,7 +375,7 @@ Object manifests are stored in immutable Secrets. Secret names are content-addre apiVersion: v1 kind: Secret metadata: - name: my-app-rev1-3a7f1b9c0e2d4f68 + name: my-app-1-3a7f1b9c0e2d4f68 namespace: olmv1-system immutable: true data: @@ -376,7 +386,7 @@ data: apiVersion: v1 kind: Secret metadata: - name: my-app-rev1-8b2e4a6c1d3f5097 + name: my-app-1-8b2e4a6c1d3f5097 namespace: olmv1-system immutable: true data: @@ -392,7 +402,7 @@ The ClusterObjectSet references these Secrets: apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: my-app-rev1 + name: my-app-1 spec: group: my-app revision: 1 @@ -402,23 +412,23 @@ spec: - name: crds objects: - ref: - name: my-app-rev1-3a7f1b9c0e2d4f68 + name: my-app-1-3a7f1b9c0e2d4f68 namespace: olmv1-system key: "K8sTl2xQa0vNpR7mW4dYcJfHbE9gUiAoZX1sDj6wFCy" - name: rbac objects: - ref: - name: my-app-rev1-8b2e4a6c1d3f5097 + name: my-app-1-8b2e4a6c1d3f5097 namespace: olmv1-system key: "Pm5tRqLwXv8sCj3kFdNhYe7bUiA0oZG1x2W9acDfHJy" - ref: - name: my-app-rev1-8b2e4a6c1d3f5097 + name: my-app-1-8b2e4a6c1d3f5097 namespace: olmv1-system key: "Vn4mTsLwXp8rCk3jFdNhYe7bUiA0oZG1x2W9acDfHQy" - name: deploy objects: - ref: - name: my-app-rev1-8b2e4a6c1d3f5097 + name: my-app-1-8b2e4a6c1d3f5097 namespace: olmv1-system key: "Bq9nRtLxYw2sDk4lGePhZf8cVjB1pUiA0oZH3x5WaCdy" ``` @@ -445,6 +455,6 @@ Example output: ``` NAME GROUP READY AGE -my-extension-abc12 my-extension Unknown 2d -my-extension-def34 my-extension True 1h +my-extension-1 my-extension Unknown 2d +my-extension-2 my-extension True 1h ``` diff --git a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml index 35140af55d..bee48a49ee 100644 --- a/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml +++ b/helm/olmv1/base/object-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yaml @@ -53,6 +53,9 @@ spec: single revision at a time. Ownership of objects is transitioned from one revision to the next as new revisions are rolled out. Once the latest revision has rolled out successfully, previous active revisions are archived for posterity. + Set metadata.name explicitly to -, using decimal + notation without leading zeros. This ensures revision uniqueness within a group. + Name generation through metadata.generateName is not supported. properties: apiVersion: description: |- @@ -561,9 +564,10 @@ spec: revision is a required, immutable sequence number identifying a specific ClusterObjectSet within a sequence of related revisions. - The revision field must be a positive integer. - Each ClusterObjectSet in the same revision sequence must have a unique revision number. + The revision field must be an integer from 1 to 9999999999, inclusive. + Each ClusterObjectSet in the same group must have a unique revision number. format: int64 + maximum: 9999999999 minimum: 1 type: integer x-kubernetes-validations: @@ -806,6 +810,9 @@ spec: required: - spec type: object + x-kubernetes-validations: + - message: metadata.name must be - + rule: self.metadata.name == '%s-%d'.format([self.spec.group, self.spec.revision]) selectableFields: - jsonPath: .spec.group served: true diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go index 39bf3c300e..9a57f609f8 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go @@ -32,10 +32,10 @@ func TestBoxcutterRevisionStatesGetter_GroupIndex(t *testing.T) { succeeded bool ownerName string }{ - {name: "cache-installed", group: group, revision: 1, succeeded: true, ownerName: "other-owner"}, - {name: "cache-rolling", group: group, revision: 2}, - {name: "cache-archived", group: group, revision: 3, archived: true, succeeded: true, ownerName: group}, - {name: "cache-other-group", group: "other-group", revision: 99, succeeded: true, ownerName: group}, + {name: "cache-group-1", group: group, revision: 1, succeeded: true, ownerName: "other-owner"}, + {name: "cache-group-2", group: group, revision: 2}, + {name: "cache-group-3", group: group, revision: 3, archived: true, succeeded: true, ownerName: group}, + {name: "other-group-99", group: "other-group", revision: 99, succeeded: true, ownerName: group}, } { cos := &ocv1.ClusterObjectSet{ ObjectMeta: metav1.ObjectMeta{Name: tc.name}, @@ -89,11 +89,11 @@ func TestBoxcutterRevisionStatesGetter_GroupIndex(t *testing.T) { states, err := getter.GetRevisionStates(ctx, ext) require.NoError(t, err) require.NotNil(t, states.Installed) - require.Equal(t, "cache-installed", states.Installed.RevisionName) + require.Equal(t, "cache-group-1", states.Installed.RevisionName) require.Len(t, states.RollingOut, 1) - require.Equal(t, "cache-rolling", states.RollingOut[0].RevisionName) + require.Equal(t, "cache-group-2", states.RollingOut[0].RevisionName) - require.NoError(t, cl.Delete(ctx, &ocv1.ClusterObjectSet{ObjectMeta: metav1.ObjectMeta{Name: "cache-rolling"}})) + require.NoError(t, cl.Delete(ctx, &ocv1.ClusterObjectSet{ObjectMeta: metav1.ObjectMeta{Name: "cache-group-2"}})) require.Eventually(t, func() bool { states, err := getter.GetRevisionStates(ctx, ext) return err == nil && len(states.RollingOut) == 0 diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 647870c3a8..a66bd05ef8 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1965,6 +1965,9 @@ spec: single revision at a time. Ownership of objects is transitioned from one revision to the next as new revisions are rolled out. Once the latest revision has rolled out successfully, previous active revisions are archived for posterity. + Set metadata.name explicitly to -, using decimal + notation without leading zeros. This ensures revision uniqueness within a group. + Name generation through metadata.generateName is not supported. properties: apiVersion: description: |- @@ -2473,9 +2476,10 @@ spec: revision is a required, immutable sequence number identifying a specific ClusterObjectSet within a sequence of related revisions. - The revision field must be a positive integer. - Each ClusterObjectSet in the same revision sequence must have a unique revision number. + The revision field must be an integer from 1 to 9999999999, inclusive. + Each ClusterObjectSet in the same group must have a unique revision number. format: int64 + maximum: 9999999999 minimum: 1 type: integer x-kubernetes-validations: @@ -2718,6 +2722,9 @@ spec: required: - spec type: object + x-kubernetes-validations: + - message: metadata.name must be - + rule: self.metadata.name == '%s-%d'.format([self.spec.group, self.spec.revision]) selectableFields: - jsonPath: .spec.group served: true diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 18c072a5ea..32628fdc66 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1926,6 +1926,9 @@ spec: single revision at a time. Ownership of objects is transitioned from one revision to the next as new revisions are rolled out. Once the latest revision has rolled out successfully, previous active revisions are archived for posterity. + Set metadata.name explicitly to -, using decimal + notation without leading zeros. This ensures revision uniqueness within a group. + Name generation through metadata.generateName is not supported. properties: apiVersion: description: |- @@ -2434,9 +2437,10 @@ spec: revision is a required, immutable sequence number identifying a specific ClusterObjectSet within a sequence of related revisions. - The revision field must be a positive integer. - Each ClusterObjectSet in the same revision sequence must have a unique revision number. + The revision field must be an integer from 1 to 9999999999, inclusive. + Each ClusterObjectSet in the same group must have a unique revision number. format: int64 + maximum: 9999999999 minimum: 1 type: integer x-kubernetes-validations: @@ -2679,6 +2683,9 @@ spec: required: - spec type: object + x-kubernetes-validations: + - message: metadata.name must be - + rule: self.metadata.name == '%s-%d'.format([self.spec.group, self.spec.revision]) selectableFields: - jsonPath: .spec.group served: true diff --git a/test/e2e/features/cos_object_collision_and_adoption.feature b/test/e2e/features/cos_object_collision_and_adoption.feature index ae38d4b0a5..25c13de2aa 100644 --- a/test/e2e/features/cos_object_collision_and_adoption.feature +++ b/test/e2e/features/cos_object_collision_and_adoption.feature @@ -65,7 +65,7 @@ Feature: ClusterObjectSet object collision and adoption apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: owner-${TEST_NAMESPACE} + name: owner-${NAME}-1 spec: lifecycleState: Active group: owner-${NAME} @@ -83,13 +83,13 @@ Feature: ClusterObjectSet object collision and adoption k: owned-by-owner revision: 1 """ - And ClusterObjectSet "owner-${TEST_NAMESPACE}" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "owner-${NAME}-1" reports Ready as True with Reason AllObjectsReady When ClusterObjectSet is applied """ apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: main-${TEST_NAMESPACE} + name: main-${NAME}-1 spec: lifecycleState: Active group: main-${NAME} @@ -107,7 +107,7 @@ Feature: ClusterObjectSet object collision and adoption k: contested revision: 1 """ - Then ClusterObjectSet "main-${TEST_NAMESPACE}" reports Ready as False with Reason ObjectCollision and Message includes: + Then ClusterObjectSet "main-${NAME}-1" reports Ready as False with Reason ObjectCollision and Message includes: """ Cannot take ownership of 1 object(s) in phase "resources" because they are owned by another controller or already exist. """ @@ -119,7 +119,7 @@ Feature: ClusterObjectSet object collision and adoption apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: owner-${TEST_NAMESPACE} + name: owner-${NAME}-1 spec: lifecycleState: Active group: owner-${NAME} @@ -137,13 +137,13 @@ Feature: ClusterObjectSet object collision and adoption k: owned-by-owner revision: 1 """ - And ClusterObjectSet "owner-${TEST_NAMESPACE}" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "owner-${NAME}-1" reports Ready as True with Reason AllObjectsReady When ClusterObjectSet is applied """ apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: main-${TEST_NAMESPACE} + name: main-${NAME}-1 spec: lifecycleState: Active group: main-${NAME} @@ -161,7 +161,7 @@ Feature: ClusterObjectSet object collision and adoption k: contested revision: 1 """ - Then ClusterObjectSet "main-${TEST_NAMESPACE}" reports Ready as False with Reason ObjectCollision and Message includes: + Then ClusterObjectSet "main-${NAME}-1" reports Ready as False with Reason ObjectCollision and Message includes: """ Cannot take ownership of 1 object(s) in phase "resources" because they are owned by another controller or already exist. """ @@ -261,7 +261,7 @@ Feature: ClusterObjectSet object collision and adoption apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: owner-${TEST_NAMESPACE} + name: owner-${NAME}-1 spec: lifecycleState: Active group: owner-${NAME} @@ -279,13 +279,13 @@ Feature: ClusterObjectSet object collision and adoption k: owned-by-owner revision: 1 """ - And ClusterObjectSet "owner-${TEST_NAMESPACE}" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "owner-${NAME}-1" reports Ready as True with Reason AllObjectsReady When ClusterObjectSet is applied """ apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: main-${TEST_NAMESPACE} + name: main-${NAME}-1 spec: lifecycleState: Active group: main-${NAME} @@ -303,7 +303,7 @@ Feature: ClusterObjectSet object collision and adoption k: contested revision: 1 """ - Then ClusterObjectSet "main-${TEST_NAMESPACE}" reports Ready as True with Reason AllObjectsReady + Then ClusterObjectSet "main-${NAME}-1" reports Ready as True with Reason AllObjectsReady And resource "configmap/shared-cm" matches """ data: diff --git a/test/e2e/features/group.feature b/test/e2e/features/group.feature index 39acf94d4f..82d3da2d9e 100644 --- a/test/e2e/features/group.feature +++ b/test/e2e/features/group.feature @@ -178,7 +178,7 @@ Feature: Track ClusterObjectSet revisions by group apiVersion: olm.operatorframework.io/v1 kind: ClusterObjectSet metadata: - name: ${COS_NAME} + name: ${NAME}-other-1 labels: olm.operatorframework.io/owner-name: ${NAME} spec: diff --git a/test/e2e/steps/hooks.go b/test/e2e/steps/hooks.go index 3278a31e0e..fb5980c726 100644 --- a/test/e2e/steps/hooks.go +++ b/test/e2e/steps/hooks.go @@ -220,7 +220,7 @@ func CreateScenarioContext(ctx context.Context, sc *godog.Scenario) (context.Con scenarioName: sc.Name, namespace: fmt.Sprintf("ns-%s", sc.Id), clusterExtensionName: fmt.Sprintf("ce-%s", sc.Id), - clusterObjectSetName: fmt.Sprintf("cos-%s", sc.Id), + clusterObjectSetName: fmt.Sprintf("ce-%s-1", sc.Id), catalogs: make(map[string]string), catalogPackageNames: make(map[string]string), metricsResponse: make(map[string]string), From 6c9a315d274f5f8340303509a1a65308c548324a Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 22 Oct 2026 00:03:59 +0200 Subject: [PATCH 2/5] Remove duplicate collision protection validation case The existing required-field case already covers the same omission and validation error. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectset_types_test.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/api/v1/clusterobjectset_types_test.go b/api/v1/clusterobjectset_types_test.go index afa64038bc..830cfb69b8 100644 --- a/api/v1/clusterobjectset_types_test.go +++ b/api/v1/clusterobjectset_types_test.go @@ -412,15 +412,6 @@ func TestClusterObjectSetValidity(t *testing.T) { valid: false, expectedError: "spec.collisionProtection: Unsupported value", }, - "spec collisionProtection must be set": { - spec: ClusterObjectSetSpec{ - Group: "validity-unset-collision-protection", - LifecycleState: ClusterObjectSetLifecycleStateActive, - Revision: 1, - }, - valid: false, - expectedError: "spec.collisionProtection: Required value", - }, "object collisionProtection is optional": { spec: ClusterObjectSetSpec{ Group: "validity-optional-object-collision-protection", From d01783409f8db152ff634db5a072105edf4384c4 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 22 Oct 2026 01:33:56 +0200 Subject: [PATCH 3/5] Fix empty collision protection validation expectation The required field now serializes its empty string value, which admission rejects through enum validation. Rename the case and expect the specific unsupported-value error. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- api/v1/clusterobjectset_types_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/v1/clusterobjectset_types_test.go b/api/v1/clusterobjectset_types_test.go index 830cfb69b8..232bc1b58e 100644 --- a/api/v1/clusterobjectset_types_test.go +++ b/api/v1/clusterobjectset_types_test.go @@ -393,14 +393,14 @@ func TestClusterObjectSetValidity(t *testing.T) { }, valid: true, }, - "spec collisionProtection is required": { + "spec collisionProtection rejects empty values": { spec: ClusterObjectSetSpec{ - Group: "validity-required-collision-protection", + Group: "validity-empty-collision-protection", LifecycleState: ClusterObjectSetLifecycleStateActive, Revision: 1, }, valid: false, - expectedError: "spec.collisionProtection: Required value", + expectedError: "spec.collisionProtection: Unsupported value", }, "spec collisionProtection rejects invalid values": { spec: ClusterObjectSetSpec{ From 7512c0832d954b71c226f84886d0b63c44102f4c Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Thu, 22 Oct 2026 01:23:22 +0200 Subject: [PATCH 4/5] Guard COD revision allocation at the ClusterObjectSet limit Reject COD allocation at the shared revision limit before incrementing or creating a ClusterObjectSet. Keep unchanged content reconciling at the limit. Cover final allocation, exhausted and oversized revision values, and archived foreign-owned history. Document both COD and CE allocation. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- docs/draft/concepts/clusterobjectsets.md | 8 +- .../clusterobjectdeployment_controller.go | 14 ++- ...jectdeployment_controller_internal_test.go | 8 +- ...clusterobjectdeployment_controller_test.go | 104 +++++++++++++++++- 4 files changed, 124 insertions(+), 10 deletions(-) diff --git a/docs/draft/concepts/clusterobjectsets.md b/docs/draft/concepts/clusterobjectsets.md index dbaabd6592..d568561f8c 100644 --- a/docs/draft/concepts/clusterobjectsets.md +++ b/docs/draft/concepts/clusterobjectsets.md @@ -261,9 +261,11 @@ When a ClusterExtension is upgraded to a new version: 4. Once the new revision has rolled out successfully, the old revision is archived 5. Older archived revisions are garbage collected, keeping the last 5 for auditing -Once a group reaches revision 9999999999, operator-controller reports an error -instead of allocating another revision, before pruning history or creating new -Secrets or ClusterObjectSets. Reconciliation of unchanged content still works. +Once a group reaches revision 9999999999, both the ClusterObjectDeployment +controller and operator-controller report an error instead of allocating another +revision. The check happens before creating a ClusterObjectSet and, for +operator-controller, before pruning history or creating Secrets. Reconciliation +of unchanged content still works. ### Relationship with ClusterExtension diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller.go b/internal/object-controller/controllers/clusterobjectdeployment_controller.go index 545ddb5b84..d89cdedcec 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller.go @@ -128,7 +128,10 @@ func (c *ClusterObjectDeploymentReconciler) listRevisions(ctx context.Context, g // // TODO: revisit AlreadyExists handling under the adoption use-case. func (c *ClusterObjectDeploymentReconciler) createRevision(ctx context.Context, cod *ocv1.ClusterObjectDeployment, revisions []ocv1.ClusterObjectSet, hash string) error { - revision := nextRevisionNumber(revisions) + revision, err := nextRevisionNumber(revisions) + if err != nil { + return fmt.Errorf("allocating revision for group %q: %w", cod.Name, err) + } cos := buildClusterObjectSet(cod, revision, hash) if err := controllerutil.SetControllerReference(cod, cos, c.Scheme); err != nil { return fmt.Errorf("setting controller reference: %w", err) @@ -227,15 +230,18 @@ func templateHash(template ocv1.ClusterObjectDeploymentTemplate) string { // nextRevisionNumber returns the revision number for a new revision: one greater // than the highest revision across all revisions in the group (including archived -// ones, so revision numbers are never reused). -func nextRevisionNumber(revisions []ocv1.ClusterObjectSet) int64 { +// ones, so revision numbers are never reused). It returns an error at the limit. +func nextRevisionNumber(revisions []ocv1.ClusterObjectSet) (int64, error) { var max int64 for i := range revisions { if revisions[i].Spec.Revision > max { max = revisions[i].Spec.Revision } } - return max + 1 + if max >= ocv1.ClusterObjectSetMaxRevision { + return 0, fmt.Errorf("revision limit %d reached", ocv1.ClusterObjectSetMaxRevision) + } + return max + 1, nil } // latestActiveRevision returns the highest-revision ClusterObjectSet that is diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go index 88ad87501f..a0c79797bf 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_internal_test.go @@ -63,12 +63,16 @@ func Test_templateHash_isStableAndSensitive(t *testing.T) { } func Test_nextRevisionNumber(t *testing.T) { - require.Equal(t, int64(1), nextRevisionNumber(nil)) + revision, err := nextRevisionNumber(nil) + require.NoError(t, err) + require.Equal(t, int64(1), revision) revs := []ocv1.ClusterObjectSet{ {Spec: ocv1.ClusterObjectSetSpec{Revision: 1}}, {Spec: ocv1.ClusterObjectSetSpec{Revision: 3, LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived}}, } - require.Equal(t, int64(4), nextRevisionNumber(revs), "must count archived revisions so numbers never repeat") + revision, err = nextRevisionNumber(revs) + require.NoError(t, err) + require.Equal(t, int64(4), revision, "must count archived revisions so numbers never repeat") } func Test_latestActiveRevision(t *testing.T) { diff --git a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go index 9cd92605ff..3942962287 100644 --- a/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectdeployment_controller_test.go @@ -4,6 +4,8 @@ package controllers_test import ( "context" + "fmt" + "math" "testing" "github.com/stretchr/testify/require" @@ -20,6 +22,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/object-controller/controllers" "github.com/operator-framework/operator-controller/internal/shared/labels" + hashutil "github.com/operator-framework/operator-controller/internal/shared/util/hash" "github.com/operator-framework/operator-controller/internal/testutil" ) @@ -49,7 +52,7 @@ func codFixture() *ocv1.ClusterObjectDeployment { } } -func newCODClient(t *testing.T, objs ...client.Object) client.Client { +func newCODClient(t *testing.T, objs ...client.Object) client.WithWatch { t.Helper() return testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(newScheme(t)), controllers.SetupIndexes). WithStatusSubresource(&ocv1.ClusterObjectDeployment{}, &ocv1.ClusterObjectSet{}). @@ -378,3 +381,102 @@ func Test_ClusterObjectDeploymentReconciler_SurfacesAlreadyExists(t *testing.T) Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Name: clusterObjectDeploymentName}}) require.Error(t, err, "an AlreadyExists on create must surface as a reconcile error, not be silently tolerated") } + +func Test_ClusterObjectDeploymentReconciler_RevisionBounds(t *testing.T) { + for _, tc := range []struct { + name string + latest int64 + lifecycleState ocv1.ClusterObjectSetLifecycleState + changed bool + wantErr bool + wantCreates int + }{ + { + name: "allocates final revision after archived foreign-owned history", + latest: ocv1.ClusterObjectSetMaxRevision - 1, + lifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, + changed: false, + wantErr: false, + wantCreates: 1, + }, + { + name: "changed template at maximum creates nothing", + latest: ocv1.ClusterObjectSetMaxRevision, + lifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + changed: true, + wantErr: true, + wantCreates: 0, + }, + { + name: "stored maximum int64 cannot overflow", + latest: math.MaxInt64, + lifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, + changed: false, + wantErr: true, + wantCreates: 0, + }, + { + name: "unchanged template at maximum still reconciles", + latest: ocv1.ClusterObjectSetMaxRevision, + lifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + changed: false, + wantErr: false, + wantCreates: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cod := codFixture() + latest := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-%d", cod.Name, tc.latest), + Labels: map[string]string{ + labels.TemplateHashKey: hashutil.DeepHashObject(cod.Spec.Template), + }, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(&ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-deployment", + UID: "other-uid", + }, + }, ocv1.GroupVersion.WithKind(ocv1.ClusterObjectDeploymentKind)), + }, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Group: cod.Name, + Revision: tc.latest, + LifecycleState: tc.lifecycleState, + CollisionProtection: ocv1.CollisionProtectionPrevent, + Phases: cod.Spec.Template.Spec.Phases, + }, + } + if tc.changed { + cod.Spec.Template.Spec.CollisionProtection = ocv1.CollisionProtectionNone + } + creates := 0 + c := interceptor.NewClient(newCODClient(t, cod, latest), interceptor.Funcs{ + Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + creates++ + return cl.Create(ctx, obj, opts...) + }, + }) + + _, err := reconcileCOD(t, c) + + if tc.wantErr { + require.ErrorContains(t, err, fmt.Sprintf("revision limit %d reached", ocv1.ClusterObjectSetMaxRevision)) + require.ErrorContains(t, err, fmt.Sprintf("group %q", cod.Name)) + } else { + require.NoError(t, err) + } + require.Equal(t, tc.wantCreates, creates) + retained, ok := getCOS(t, c, latest.Name) + require.True(t, ok) + require.Equal(t, latest.Spec, retained.Spec) + if tc.wantCreates == 1 { + created, ok := getCOS(t, c, fmt.Sprintf("%s-%d", cod.Name, ocv1.ClusterObjectSetMaxRevision)) + require.True(t, ok) + require.Equal(t, ocv1.ClusterObjectSetMaxRevision, created.Spec.Revision) + } + }) + } +} From 3ebedbb0a37cebede0caf10eccb9f8cdb90bdc55 Mon Sep 17 00:00:00 2001 From: Francesco Giudici Date: Wed, 21 Oct 2026 19:50:06 +0200 Subject: [PATCH 5/5] Guard CE revision allocation at the ClusterObjectSet limit Reject allocation at the revision limit before incrementing the number, pruning history, or creating revision content. Allow unchanged content to continue reconciling at the limit. Cover the final allowed allocation, owner-independent numbering and exhaustion, unchanged reconciliation, and stored values that would overflow without the guard. Keep this CE implementation separate for the future move of revision allocation into ClusterObjectDeployment. Co-authored-by: Codex Signed-off-by: Francesco Giudici --- .../operator-controller/applier/boxcutter.go | 6 +- .../applier/boxcutter_revision_bounds_test.go | 260 ++++++++++++++++++ 2 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 internal/operator-controller/applier/boxcutter_revision_bounds_test.go diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 4b45ebcff8..0232ed3da7 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -584,7 +584,11 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust // It follows a crash-safe three-step sequence: create Secrets, create COS, patch ownerRefs. func (bc *Boxcutter) createExternalizedRevision(ctx context.Context, ext *ocv1.ClusterExtension, desiredRevision *ocv1ac.ClusterObjectSetApplyConfiguration, existingRevisions []ocv1.ClusterObjectSet) error { prevRevisions := existingRevisions - revisionNumber := latestRevisionNumber(prevRevisions) + 1 + latest := latestRevisionNumber(prevRevisions) + if latest >= ocv1.ClusterObjectSetMaxRevision { + return fmt.Errorf("revision limit %d reached for group %q", ocv1.ClusterObjectSetMaxRevision, ext.Name) + } + revisionNumber := latest + 1 revisionName := fmt.Sprintf("%s-%d", ext.Name, revisionNumber) desiredRevision.WithName(revisionName) diff --git a/internal/operator-controller/applier/boxcutter_revision_bounds_test.go b/internal/operator-controller/applier/boxcutter_revision_bounds_test.go new file mode 100644 index 0000000000..3992cd2706 --- /dev/null +++ b/internal/operator-controller/applier/boxcutter_revision_bounds_test.go @@ -0,0 +1,260 @@ +package applier_test + +import ( + "context" + "fmt" + "math" + "strings" + "testing" + "testing/fstest" + + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" + coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" + "github.com/operator-framework/operator-controller/internal/operator-controller/applier" + "github.com/operator-framework/operator-controller/internal/testutil" + mockapplier "github.com/operator-framework/operator-controller/internal/testutil/mock/applier" +) + +func TestBoxcutter_Apply_RevisionBounds(t *testing.T) { + for _, tc := range []struct { + name string + latest int64 + changed bool + exhausted bool + }{ + { + name: "allocates final allowed revision across owners", + latest: ocv1.ClusterObjectSetMaxRevision - 1, + changed: true, + exhausted: false, + }, + { + name: "exhaustion preserves history and creates nothing", + latest: ocv1.ClusterObjectSetMaxRevision, + changed: true, + exhausted: true, + }, + { + name: "unchanged content at maximum still reconciles", + latest: ocv1.ClusterObjectSetMaxRevision, + changed: false, + exhausted: false, + }, + { + name: "out of range stored revision cannot overflow", + latest: math.MaxInt64, + changed: true, + exhausted: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + fixture := newRevisionBoundsFixture(t, tc.latest, tc.changed) + + completed, status, err := fixture.applier.Apply(t.Context(), fstest.MapFS{}, fixture.extension, nil, nil) + + require.Empty(t, status) + require.Equal(t, 1, fixture.requests.comparisons) + if tc.exhausted { + require.ErrorContains(t, err, fmt.Sprintf("revision limit %d reached for group %q", ocv1.ClusterObjectSetMaxRevision, fixture.extension.Name)) + require.False(t, completed) + } else { + require.NoError(t, err) + require.True(t, completed) + } + + if tc.exhausted || !tc.changed { + require.Zero(t, fixture.requests.newApplies) + require.Zero(t, fixture.requests.creates) + require.Zero(t, fixture.requests.deletes) + require.Zero(t, fixture.requests.patches) + for _, obj := range fixture.seededObjects { + retained := obj.DeepCopyObject().(client.Object) + require.NoError(t, fixture.client.Get(t.Context(), client.ObjectKeyFromObject(obj), retained)) + } + return + } + + require.Equal(t, 1, fixture.requests.newApplies) + require.Positive(t, fixture.requests.creates, "new revision content must be externalized") + var created ocv1.ClusterObjectSet + key := client.ObjectKey{ + Name: fmt.Sprintf("%s-%d", fixture.extension.Name, ocv1.ClusterObjectSetMaxRevision), + } + require.NoError(t, fixture.client.Get(t.Context(), key, &created)) + require.Len(t, created.Name, 63) + require.Equal(t, ocv1.ClusterObjectSetMaxRevision, created.Spec.Revision) + require.Equal(t, fixture.extension.Name, created.Spec.Group) + }) + } +} + +type revisionBoundsFixture struct { + extension *ocv1.ClusterExtension + applier *applier.Boxcutter + client client.Client + seededObjects []client.Object + requests revisionBoundsRequests +} + +type revisionBoundsRequests struct { + comparisons int + newApplies int + creates int + deletes int + patches int +} + +// newRevisionBoundsFixture seeds history and externalized content, and models +// API immutability rejection when changed content requires another revision. +func newRevisionBoundsFixture(t *testing.T, latestRevision int64, changed bool) *revisionBoundsFixture { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + fixture := &revisionBoundsFixture{ + extension: &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: strings.Repeat("a", 52), + UID: "extension-uid", + }, + }, + } + ext := fixture.extension + owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind)) + // Enough archived revisions to trigger retention if the guard runs too late. + for revision := int64(1); revision <= int64(applier.ClusterObjectSetRetentionLimit)+1; revision++ { + fixture.seededObjects = append(fixture.seededObjects, &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-%d", ext.Name, revision), + OwnerReferences: []metav1.OwnerReference{owner}, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, + Revision: revision, + LifecycleState: ocv1.ClusterObjectSetLifecycleStateArchived, + CollisionProtection: ocv1.CollisionProtectionPrevent, + }, + }) + } + latest := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-%d", ext.Name, latestRevision), + UID: "latest-uid", + }, + Spec: ocv1.ClusterObjectSetSpec{ + Group: ext.Name, + Revision: latestRevision, + LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + CollisionProtection: ocv1.CollisionProtectionPrevent, + }, + } + // The latest revision is deliberately owned by someone else (or unowned). + // Neither group-wide numbering nor exhaustion may depend on CE ownership. + if changed { + foreignOwner := owner + foreignOwner.Name = "other-extension" + foreignOwner.UID = "other-uid" + latest.OwnerReferences = []metav1.OwnerReference{foreignOwner} + } + payload := unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "payload", + }, + }, + } + packer := applier.SecretPacker{ + RevisionName: latest.Name, + OwnerName: ext.Name, + SystemNamespace: "olmv1-system", + } + pack, err := packer.Pack([]ocv1.ClusterObjectSetPhase{ + { + Name: "deploy", + Objects: []ocv1.ClusterObjectSetObject{ + { + Object: payload, + }, + }, + }, + }) + require.NoError(t, err) + latest.Spec.Phases = []ocv1.ClusterObjectSetPhase{ + { + Name: "deploy", + Objects: []ocv1.ClusterObjectSetObject{ + { + Ref: pack.Refs[[2]int{0, 0}], + }, + }, + }, + } + fixture.seededObjects = append(fixture.seededObjects, latest) + for i := range pack.Secrets { + pack.Secrets[i].OwnerReferences = []metav1.OwnerReference{ + *metav1.NewControllerRef(latest, ocv1.GroupVersion.WithKind(ocv1.ClusterObjectSetKind)), + } + fixture.seededObjects = append(fixture.seededObjects, &pack.Secrets[i]) + } + + fixture.client = testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(scheme), coscontrollers.SetupIndexes). + WithObjects(fixture.seededObjects...).WithInterceptorFuncs(interceptor.Funcs{ + Apply: func(ctx context.Context, cl client.WithWatch, obj runtime.ApplyConfiguration, opts ...client.ApplyOption) error { + cos := obj.(*ocv1ac.ClusterObjectSetApplyConfiguration) + if *cos.Name == latest.Name { + fixture.requests.comparisons++ + if changed { + return apierrors.NewInvalid(ocv1.GroupVersion.WithKind(ocv1.ClusterObjectSetKind).GroupKind(), latest.Name, + field.ErrorList{field.Invalid(field.NewPath("spec", "phases"), "changed", "phases is immutable")}) + } + } else { + fixture.requests.newApplies++ + } + return cl.Apply(ctx, obj, opts...) + }, + Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + fixture.requests.creates++ + return cl.Create(ctx, obj, opts...) + }, + Delete: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + fixture.requests.deletes++ + return cl.Delete(ctx, obj, opts...) + }, + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + fixture.requests.patches++ + return cl.Patch(ctx, obj, patch, opts...) + }, + }).Build() + generator := mockapplier.NewMockClusterObjectSetGenerator(gomock.NewController(t)) + generator.EXPECT().GenerateRevision(gomock.Any(), gomock.Any(), ext, gomock.Any(), gomock.Any()).Return( + ocv1ac.ClusterObjectSet("").WithSpec(ocv1ac.ClusterObjectSetSpec(). + WithGroup(ext.Name). + WithLifecycleState(ocv1.ClusterObjectSetLifecycleStateActive). + WithCollisionProtection(ocv1.CollisionProtectionPrevent). + WithPhases(ocv1ac.ClusterObjectSetPhase(). + WithName("deploy"). + WithObjects(ocv1ac.ClusterObjectSetObject().WithObject(payload)))), nil) + fixture.applier = &applier.Boxcutter{ + Client: fixture.client, + Scheme: scheme, + RevisionGenerator: generator, + FieldOwner: "test-owner", + SystemNamespace: "olmv1-system", + } + return fixture +}