diff --git a/api/v1/clusterextensionrevision_types.go b/api/v1/clusterextensionrevision_types.go index f3416bf25..389e4a5b3 100644 --- a/api/v1/clusterextensionrevision_types.go +++ b/api/v1/clusterextensionrevision_types.go @@ -105,6 +105,19 @@ type ClusterExtensionRevisionSpec struct { // +optional // ProgressDeadlineMinutes int32 `json:"progressDeadlineMinutes,omitempty"` + + // collisionProtection specifies the default collision protection strategy for all objects + // in this revision. Individual phases or objects can override this value. + // + // When set, this value is used as the default for any phase or object that does not + // explicitly specify its own collisionProtection. + // + // The resolution order is: object > phase > spec + // + // +required + // +kubebuilder:validation:Enum=Prevent;IfNoController;None + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="collisionProtection is immutable" + CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } // ClusterExtensionRevisionLifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -144,6 +157,19 @@ type ClusterExtensionRevisionPhase struct { // +required // +kubebuilder:validation:MaxItems=50 Objects []ClusterExtensionRevisionObject `json:"objects"` + + // collisionProtection specifies the default collision protection strategy for all objects + // in this phase. Individual objects can override this value. + // + // When set, this value is used as the default for any object in this phase that does not + // explicitly specify its own collisionProtection. + // + // When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + // explicitly specify its own collisionProtection. + // + // +optional + // +kubebuilder:validation:Enum=Prevent;IfNoController;None + CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } // ClusterExtensionRevisionObject represents a Kubernetes object to be applied as part @@ -174,7 +200,9 @@ type ClusterExtensionRevisionObject struct { // Use this setting with extreme caution as it may cause multiple controllers to fight over // the same resource, resulting in increased load on the API server and etcd. // - // +required + // When omitted, the value is inherited from the phase, then spec. + // + // +optional // +kubebuilder:validation:Enum=Prevent;IfNoController;None CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } diff --git a/api/v1/clusterextensionrevision_types_test.go b/api/v1/clusterextensionrevision_types_test.go index a6519ec04..75a1a6cc3 100644 --- a/api/v1/clusterextensionrevision_types_test.go +++ b/api/v1/clusterextensionrevision_types_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func TestClusterExtensionRevisionImmutability(t *testing.T) { @@ -21,8 +22,9 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { }{ "revision is immutable": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, }, updateFunc: func(cer *ClusterExtensionRevision) { cer.Spec.Revision = 2 @@ -30,9 +32,10 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { }, "phases may be initially empty": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, - Phases: []ClusterExtensionRevisionPhase{}, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Phases: []ClusterExtensionRevisionPhase{}, }, updateFunc: func(cer *ClusterExtensionRevision) { cer.Spec.Phases = []ClusterExtensionRevisionPhase{ @@ -46,8 +49,9 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { }, "phases may be initially unset": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, }, updateFunc: func(cer *ClusterExtensionRevision) { cer.Spec.Phases = []ClusterExtensionRevisionPhase{ @@ -61,8 +65,9 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { }, "phases are immutable if not empty": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, Phases: []ClusterExtensionRevisionPhase{ { Name: "foo", @@ -79,6 +84,16 @@ func TestClusterExtensionRevisionImmutability(t *testing.T) { } }, }, + "spec collisionProtection is immutable": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + }, + updateFunc: func(cer *ClusterExtensionRevision) { + cer.Spec.CollisionProtection = CollisionProtectionNone + }, + }, } { t.Run(name, func(t *testing.T) { cer := &ClusterExtensionRevision{ @@ -124,8 +139,9 @@ func TestClusterExtensionRevisionValidity(t *testing.T) { }, "revision must be positive": { spec: ClusterExtensionRevisionSpec{ - LifecycleState: ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, }, valid: true, }, @@ -192,6 +208,70 @@ func TestClusterExtensionRevisionValidity(t *testing.T) { }, valid: false, }, + "spec collisionProtection accepts Prevent": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + }, + valid: true, + }, + "spec collisionProtection accepts IfNoController": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionIfNoController, + }, + valid: true, + }, + "spec collisionProtection accepts None": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionNone, + }, + valid: true, + }, + "spec collisionProtection is required": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + }, + valid: false, + }, + "spec collisionProtection rejects invalid values": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtection("Invalid"), + }, + valid: false, + }, + "spec collisionProtection must be set": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + }, + valid: false, + }, + "object collisionProtection is optional": { + spec: ClusterExtensionRevisionSpec{ + LifecycleState: ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, + CollisionProtection: CollisionProtectionPrevent, + Phases: []ClusterExtensionRevisionPhase{ + { + Name: "deploy", + Objects: []ClusterExtensionRevisionObject{ + { + Object: configMap(), + }, + }, + }, + }, + }, + valid: true, + }, } { t.Run(name, func(t *testing.T) { cer := &ClusterExtensionRevision{ @@ -211,3 +291,15 @@ func TestClusterExtensionRevisionValidity(t *testing.T) { }) } } + +func configMap() unstructured.Unstructured { + return unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-cm", + }, + }, + } +} diff --git a/api/v1/validation_test.go b/api/v1/validation_test.go index 16c9447a5..c2f857493 100644 --- a/api/v1/validation_test.go +++ b/api/v1/validation_test.go @@ -36,6 +36,7 @@ func TestValidate(t *testing.T) { } defaultRevisionSpec := func(s *ClusterExtensionRevisionSpec) *ClusterExtensionRevisionSpec { s.Revision = 1 + s.CollisionProtection = CollisionProtectionPrevent return s } c := newClient(t) diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml index 2b431eab9..5ac459dea 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml @@ -56,6 +56,23 @@ spec: spec: description: spec defines the desired state of the ClusterExtensionRevision. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this revision. Individual phases or objects can override this value. + + When set, this value is used as the default for any phase or object that does not + explicitly specify its own collisionProtection. + + The resolution order is: object > phase > spec + enum: + - Prevent + - IfNoController + - None + type: string + x-kubernetes-validations: + - message: collisionProtection is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -102,6 +119,21 @@ spec: ClusterExtensionRevisionPhase represents a group of objects that are applied together. The phase is considered complete only after all objects pass their status probes. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this phase. Individual objects can override this value. + + When set, this value is used as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + + When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + enum: + - Prevent + - IfNoController + - None + type: string name: description: |- name is a required identifier for this phase. @@ -149,6 +181,8 @@ spec: owned by another controller. Use this setting with extreme caution as it may cause multiple controllers to fight over the same resource, resulting in increased load on the API server and etcd. + + When omitted, the value is inherited from the phase, then spec. enum: - Prevent - IfNoController @@ -163,7 +197,6 @@ spec: x-kubernetes-embedded-resource: true x-kubernetes-preserve-unknown-fields: true required: - - collisionProtection - object type: object maxItems: 50 @@ -205,6 +238,7 @@ spec: - message: revision is immutable rule: self == oldSelf required: + - collisionProtection - lifecycleState - revision type: object diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 04e59ca95..cd3579a4e 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -75,8 +75,7 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( sanitizedUnstructured(ctx, &obj) objs = append(objs, ocv1.ClusterExtensionRevisionObject{ - Object: obj, - CollisionProtection: ocv1.CollisionProtectionNone, // allow to adopt objects from previous release + Object: obj, }) } @@ -88,6 +87,7 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( }) rev.Name = fmt.Sprintf("%s-1", ext.Name) rev.Spec.Revision = 1 + rev.Spec.CollisionProtection = ocv1.CollisionProtectionNone // allow to adopt objects from previous release return rev, nil } @@ -147,11 +147,12 @@ func (r *SimpleRevisionGenerator) GenerateRevision( sanitizedUnstructured(ctx, &unstr) objs = append(objs, ocv1.ClusterExtensionRevisionObject{ - Object: unstr, - CollisionProtection: ocv1.CollisionProtectionPrevent, + Object: unstr, }) } - return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil + rev := r.buildClusterExtensionRevision(objs, ext, revisionAnnotations) + rev.Spec.CollisionProtection = ocv1.CollisionProtectionPrevent + return rev, nil } // sanitizedUnstructured takes an unstructured obj, removes status if present, and returns a sanitized copy containing only the allowed metadata entries set below. diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 887dee4a9..854f9517f 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -114,8 +114,9 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ - LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, - Revision: 1, + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + CollisionProtection: ocv1.CollisionProtectionNone, + Revision: 1, Phases: []ocv1.ClusterExtensionRevisionPhase{ { Name: "deploy", @@ -132,7 +133,6 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) }, }, }, - CollisionProtection: ocv1.CollisionProtectionNone, }, { Object: unstructured.Unstructured{ @@ -146,7 +146,6 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) }, }, }, - CollisionProtection: ocv1.CollisionProtectionNone, }, }, }, @@ -215,6 +214,8 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { }, rev.Labels) t.Log("by checking the revision number is 0") require.Equal(t, int64(0), rev.Spec.Revision) + t.Log("by checking the spec-level collisionProtection is set") + require.Equal(t, ocv1.CollisionProtectionPrevent, rev.Spec.CollisionProtection) t.Log("by checking the rendered objects are present in the correct phases") require.Equal(t, []ocv1.ClusterExtensionRevisionPhase{ { @@ -231,7 +232,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { "spec": map[string]interface{}{}, }, }, - CollisionProtection: ocv1.CollisionProtectionPrevent, }, { Object: unstructured.Unstructured{ @@ -260,7 +260,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) { }, }, }, - CollisionProtection: ocv1.CollisionProtectionPrevent, }, }, }, diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 138bcd8eb..eff03e9d9 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -464,10 +464,10 @@ func (c *ClusterExtensionRevisionReconciler) toBoxcutterRevision(ctx context.Con objLabels[labels.OwnerNameKey] = rev.Labels[labels.OwnerNameKey] obj.SetLabels(objLabels) - switch specObj.CollisionProtection { + switch cp := EffectiveCollisionProtection(rev.Spec.CollisionProtection, specPhase.CollisionProtection, specObj.CollisionProtection); cp { case ocv1.CollisionProtectionIfNoController, ocv1.CollisionProtectionNone: opts = append(opts, boxcutter.WithObjectReconcileOptions( - obj, boxcutter.WithCollisionProtection(specObj.CollisionProtection))) + obj, boxcutter.WithCollisionProtection(cp))) } phase.Objects = append(phase.Objects, *obj) @@ -477,6 +477,18 @@ func (c *ClusterExtensionRevisionReconciler) toBoxcutterRevision(ctx context.Con return r, opts, nil } +// EffectiveCollisionProtection resolves the collision protection value using +// the inheritance hierarchy: object > phase > spec > default ("Prevent"). +func EffectiveCollisionProtection(cp ...ocv1.CollisionProtection) ocv1.CollisionProtection { + ecp := ocv1.CollisionProtectionPrevent + for _, c := range cp { + if c != "" { + ecp = c + } + } + return ecp +} + var ( deploymentProbe = &probing.GroupKindSelector{ GroupKind: schema.GroupKind{Group: appsv1.GroupName, Kind: "Deployment"}, diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go index f456976f4..4426e0181 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go @@ -1266,6 +1266,60 @@ func (m *mockTrackingCache) Free(ctx context.Context, user client.Object) error return nil } +func Test_effectiveCollisionProtection(t *testing.T) { + for _, tc := range []struct { + name string + specCP ocv1.CollisionProtection + phaseCP ocv1.CollisionProtection + objectCP ocv1.CollisionProtection + expected ocv1.CollisionProtection + }{ + { + name: "all empty defaults to Prevent", + expected: ocv1.CollisionProtectionPrevent, + }, + { + name: "spec only", + specCP: ocv1.CollisionProtectionNone, + expected: ocv1.CollisionProtectionNone, + }, + { + name: "phase overrides spec", + specCP: ocv1.CollisionProtectionPrevent, + phaseCP: ocv1.CollisionProtectionIfNoController, + expected: ocv1.CollisionProtectionIfNoController, + }, + { + name: "object overrides phase", + specCP: ocv1.CollisionProtectionPrevent, + phaseCP: ocv1.CollisionProtectionIfNoController, + objectCP: ocv1.CollisionProtectionNone, + expected: ocv1.CollisionProtectionNone, + }, + { + name: "object overrides spec", + specCP: ocv1.CollisionProtectionNone, + objectCP: ocv1.CollisionProtectionPrevent, + expected: ocv1.CollisionProtectionPrevent, + }, + { + name: "phase only", + phaseCP: ocv1.CollisionProtectionNone, + expected: ocv1.CollisionProtectionNone, + }, + { + name: "object only", + objectCP: ocv1.CollisionProtectionIfNoController, + expected: ocv1.CollisionProtectionIfNoController, + }, + } { + t.Run(tc.name, func(t *testing.T) { + result := controllers.EffectiveCollisionProtection(tc.specCP, tc.phaseCP, tc.objectCP) + require.Equal(t, tc.expected, result) + }) + } +} + func Test_ClusterExtensionRevisionReconciler_getScopedClient_Errors(t *testing.T) { testScheme := newScheme(t) diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index eb72fb01f..84c91ba89 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -668,6 +668,23 @@ spec: spec: description: spec defines the desired state of the ClusterExtensionRevision. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this revision. Individual phases or objects can override this value. + + When set, this value is used as the default for any phase or object that does not + explicitly specify its own collisionProtection. + + The resolution order is: object > phase > spec + enum: + - Prevent + - IfNoController + - None + type: string + x-kubernetes-validations: + - message: collisionProtection is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -714,6 +731,21 @@ spec: ClusterExtensionRevisionPhase represents a group of objects that are applied together. The phase is considered complete only after all objects pass their status probes. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this phase. Individual objects can override this value. + + When set, this value is used as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + + When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + enum: + - Prevent + - IfNoController + - None + type: string name: description: |- name is a required identifier for this phase. @@ -761,6 +793,8 @@ spec: owned by another controller. Use this setting with extreme caution as it may cause multiple controllers to fight over the same resource, resulting in increased load on the API server and etcd. + + When omitted, the value is inherited from the phase, then spec. enum: - Prevent - IfNoController @@ -775,7 +809,6 @@ spec: x-kubernetes-embedded-resource: true x-kubernetes-preserve-unknown-fields: true required: - - collisionProtection - object type: object maxItems: 50 @@ -817,6 +850,7 @@ spec: - message: revision is immutable rule: self == oldSelf required: + - collisionProtection - lifecycleState - revision type: object diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 6cb9b1848..76f8dfb60 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -629,6 +629,23 @@ spec: spec: description: spec defines the desired state of the ClusterExtensionRevision. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this revision. Individual phases or objects can override this value. + + When set, this value is used as the default for any phase or object that does not + explicitly specify its own collisionProtection. + + The resolution order is: object > phase > spec + enum: + - Prevent + - IfNoController + - None + type: string + x-kubernetes-validations: + - message: collisionProtection is immutable + rule: self == oldSelf lifecycleState: description: |- lifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -675,6 +692,21 @@ spec: ClusterExtensionRevisionPhase represents a group of objects that are applied together. The phase is considered complete only after all objects pass their status probes. properties: + collisionProtection: + description: |- + collisionProtection specifies the default collision protection strategy for all objects + in this phase. Individual objects can override this value. + + When set, this value is used as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + + When omitted, we use .spec.collistionProtection as the default for any object in this phase that does not + explicitly specify its own collisionProtection. + enum: + - Prevent + - IfNoController + - None + type: string name: description: |- name is a required identifier for this phase. @@ -722,6 +754,8 @@ spec: owned by another controller. Use this setting with extreme caution as it may cause multiple controllers to fight over the same resource, resulting in increased load on the API server and etcd. + + When omitted, the value is inherited from the phase, then spec. enum: - Prevent - IfNoController @@ -736,7 +770,6 @@ spec: x-kubernetes-embedded-resource: true x-kubernetes-preserve-unknown-fields: true required: - - collisionProtection - object type: object maxItems: 50 @@ -778,6 +811,7 @@ spec: - message: revision is immutable rule: self == oldSelf required: + - collisionProtection - lifecycleState - revision type: object