Address review feedback on ClusterObjectSet group tracking

Scope COS sibling and previous revision lookup by group and controller
owner kind and name, while keeping SSA field ownership group-based.
Clarify standalone API docs and extend admission and owner tests.

Centralize COS index setup, hide runtime watch configuration behind
ClusterExtension setup methods, and mark temporary COS integration
paths for the future ClusterObjectDeployment migration.

Share ClusterExtension revision discovery across status, apply, and Helm
migration, filtering by group and controller owner kind and name.
Exclude unrelated revisions from numbering, retention, and fallback.

Validate with full unit tests including race detection, lint, API lint
diff, and idempotent API and manifest regeneration.

Co-authored-by: Codex <codex@openai.com>
Signed-off-by: Francesco Giudici <fgiudici@redhat.com>

Author: fgiudici

api/v1/clusterobjectset_types.go

@@ -38,8 +38,12 @@ const (
 
 // ClusterObjectSetSpec defines the desired state of ClusterObjectSet.
 type ClusterObjectSetSpec struct {
-	// group identifies the ClusterExtension whose revisions belong together.
-	// All revisions for the same ClusterExtension must use its name as their group.
+	// group is a required, immutable identifier that links related revisions together.
+	// Revisions sharing the same group and controller owner kind and name form an ordered sequence.
+	// Only owner references with controller set to true are considered.
+	// Revisions without a controller owner form a sequence with other such revisions in the same group.
+	// The value must be 1 to 52 characters long, start with a lowercase letter,
+	// contain only lowercase letters, digits or hyphens, and end with a letter or digit.
 	//
 	// +required
 	// +kubebuilder:validation:MinLength=1
@@ -66,11 +70,11 @@ type ClusterObjectSetSpec struct {
 	// +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Archived' && oldSelf == self", message="cannot un-archive"
 	LifecycleState ClusterObjectSetLifecycleState `json:"lifecycleState,omitempty"`
 
-	// revision is a required, immutable sequence number representing a specific revision
-	// of the parent ClusterExtension.
+	// 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 belonging to the same parent ClusterExtension must have a unique revision number.
+	// Each ClusterObjectSet in the same revision sequence must have a unique revision number.
 	// The revision number must always be the previous revision number plus one, or 1 for the first revision.
 	//
 	// +required
@@ -573,12 +577,12 @@ type ObservedPhase struct {
 // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=='Ready')].status`
 // +kubebuilder:printcolumn:name=Age,type=date,JSONPath=`.metadata.creationTimestamp`
 
-// ClusterObjectSet represents an immutable snapshot of Kubernetes objects
-// for a specific version of a ClusterExtension. Each revision contains objects
-// organized into phases that roll out sequentially. The same object can only be managed by a single revision
-// at a time. Ownership of objects is transitioned from one revision to the next as the extension is upgraded
-// or reconfigured. Once the latest revision has rolled out successfully, previous active revisions are archived for
-// posterity.
+// ClusterObjectSet represents an immutable snapshot of Kubernetes objects to
+// apply and manage on the cluster. Each revision contains objects organized into
+// phases that roll out sequentially. The same object can only be managed by a
+// 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.
 type ClusterObjectSet struct {
 	metav1.TypeMeta `json:",inline"`
 

api/v1/clusterobjectset_types_test.go

@@ -18,33 +18,51 @@ func TestClusterObjectSetImmutability(t *testing.T) {
 	ctx := context.Background()
 	i := 0
 	for name, tc := range map[string]struct {
-		spec       ClusterObjectSetSpec
-		updateFunc func(*ClusterObjectSet)
-		allowed    bool
+		spec          ClusterObjectSetSpec
+		updateFunc    func(*ClusterObjectSet)
+		allowed       bool
+		expectedError string
 	}{
 		"group is immutable": {
 			spec: ClusterObjectSetSpec{
+				Group:               "test-group",
 				LifecycleState:      ClusterObjectSetLifecycleStateActive,
 				Revision:            1,
 				CollisionProtection: CollisionProtectionPrevent,
 			},
 			updateFunc: func(cos *ClusterObjectSet) {
 				cos.Spec.Group = "another-group"
 			},
+			expectedError: "group is immutable",
+		},
+		"group cannot be cleared": {
+			spec: ClusterObjectSetSpec{
+				Group:               "test-group",
+				LifecycleState:      ClusterObjectSetLifecycleStateActive,
+				Revision:            1,
+				CollisionProtection: CollisionProtectionPrevent,
+			},
+			updateFunc: func(cos *ClusterObjectSet) {
+				cos.Spec.Group = ""
+			},
+			expectedError: "spec.group: Required value",
 		},
 		"unchanged group permits lifecycle update": {
 			spec: ClusterObjectSetSpec{
+				Group:               "test-group",
 				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",
 				LifecycleState:      ClusterObjectSetLifecycleStateActive,
 				Revision:            1,
 				CollisionProtection: CollisionProtectionPrevent,
@@ -55,6 +73,7 @@ func TestClusterObjectSetImmutability(t *testing.T) {
 		},
 		"phases may be initially empty": {
 			spec: ClusterObjectSetSpec{
+				Group:               "test-group",
 				LifecycleState:      ClusterObjectSetLifecycleStateActive,
 				Revision:            1,
 				CollisionProtection: CollisionProtectionPrevent,
@@ -72,6 +91,7 @@ func TestClusterObjectSetImmutability(t *testing.T) {
 		},
 		"phases may be initially unset": {
 			spec: ClusterObjectSetSpec{
+				Group:               "test-group",
 				LifecycleState:      ClusterObjectSetLifecycleStateActive,
 				Revision:            1,
 				CollisionProtection: CollisionProtectionPrevent,
@@ -88,6 +108,7 @@ func TestClusterObjectSetImmutability(t *testing.T) {
 		},
 		"phases are immutable if not empty": {
 			spec: ClusterObjectSetSpec{
+				Group:               "test-group",
 				LifecycleState:      ClusterObjectSetLifecycleStateActive,
 				Revision:            1,
 				CollisionProtection: CollisionProtectionPrevent,
@@ -109,6 +130,7 @@ func TestClusterObjectSetImmutability(t *testing.T) {
 		},
 		"spec collisionProtection is immutable": {
 			spec: ClusterObjectSetSpec{
+				Group:               "test-group",
 				LifecycleState:      ClusterObjectSetLifecycleStateActive,
 				Revision:            1,
 				CollisionProtection: CollisionProtectionPrevent,
@@ -125,7 +147,6 @@ func TestClusterObjectSetImmutability(t *testing.T) {
 				},
 				Spec: tc.spec,
 			}
-			cos.Spec.Group = "test-group"
 			i = i + 1
 			require.NoError(t, c.Create(ctx, cos))
 			tc.updateFunc(cos)
@@ -136,6 +157,9 @@ func TestClusterObjectSetImmutability(t *testing.T) {
 			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)
+			}
 		})
 	}
 }
@@ -388,54 +412,60 @@ func TestClusterObjectSetValidity(t *testing.T) {
 	}
 }
 
+func TestClusterObjectSetSpecValidation(t *testing.T) {
+	c := newClient(t)
+	t.Run("missing spec", func(t *testing.T) {
+		cos := &unstructured.Unstructured{Object: map[string]any{
+			"apiVersion": GroupVersion.String(),
+			"kind":       ClusterObjectSetKind,
+			"metadata":   map[string]any{"generateName": "spec-validation-"},
+		}}
+		err := c.Create(t.Context(), cos)
+		require.True(t, errors.IsInvalid(err), "%v", err)
+		require.ErrorContains(t, err, "spec: Required")
+	})
+}
+
 func TestClusterObjectSetGroupValidation(t *testing.T) {
 	c := newClient(t)
 	for _, tc := range []struct {
-		name     string
-		group    *string
-		omitSpec bool
-		valid    bool
+		name  string
+		group *string
+		valid bool
 	}{
-		{name: "missing spec", omitSpec: true},
-		{name: "missing group"},
-		{name: "empty group", group: ptr.To("")},
+		{name: "missing group", valid: false},
+		{name: "empty group", group: ptr.To(""), valid: false},
 		{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))},
-		{name: "starts with digit", group: ptr.To("1group")},
-		{name: "starts with hyphen", group: ptr.To("-group")},
-		{name: "ends with hyphen", group: ptr.To("group-")},
-		{name: "uppercase", group: ptr.To("Group")},
-		{name: "underscore", group: ptr.To("my_group")},
-		{name: "dot", group: ptr.To("my.group")},
+		{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},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			cos := &unstructured.Unstructured{Object: map[string]any{
 				"apiVersion": GroupVersion.String(),
 				"kind":       ClusterObjectSetKind,
 				"metadata":   map[string]any{"generateName": "group-validation-"},
 			}}
-			if !tc.omitSpec {
-				spec := map[string]any{
-					"revision": int64(1), "lifecycleState": string(ClusterObjectSetLifecycleStateActive),
-					"collisionProtection": string(CollisionProtectionPrevent),
-				}
-				if tc.group != nil {
-					spec["group"] = *tc.group
-				}
-				cos.Object["spec"] = spec
+			spec := map[string]any{
+				"revision": int64(1), "lifecycleState": string(ClusterObjectSetLifecycleStateActive),
+				"collisionProtection": string(CollisionProtectionPrevent),
 			}
+			if tc.group != nil {
+				spec["group"] = *tc.group
+			}
+			cos.Object["spec"] = spec
 			err := c.Create(t.Context(), cos)
 			if tc.valid {
 				require.NoError(t, err)
 			} else {
 				require.True(t, errors.IsInvalid(err), "%v", err)
-				if tc.omitSpec {
-					require.ErrorContains(t, err, "spec: Required")
-				} else {
-					require.ErrorContains(t, err, "spec.group")
-				}
+				require.ErrorContains(t, err, "spec.group")
 			}
 		})
 	}

applyconfigurations/api/v1/clusterobjectset.go

@@ -45,7 +45,6 @@ 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/object-controller/scheme"
-	"github.com/operator-framework/operator-controller/internal/shared/clusterobjectset"
 	cacheutil "github.com/operator-framework/operator-controller/internal/shared/util/cache"
 	"github.com/operator-framework/operator-controller/internal/shared/util/tlsprofiles"
 	"github.com/operator-framework/operator-controller/internal/shared/version"
@@ -183,8 +182,7 @@ func newManager(cfg *config, restConfig *rest.Config) (manager.Manager, error) {
 	if err != nil {
 		return nil, fmt.Errorf("creating discovery client: %w", err)
 	}
-	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &ocv1.ClusterObjectSet{},
-		clusterobjectset.GroupField, clusterobjectset.ExtractGroup); err != nil {
+	if err := controllers.SetupIndexes(context.Background(), mgr.GetFieldIndexer()); err != nil {
 		return nil, fmt.Errorf("indexing ClusterObjectSet group: %w", err)
 	}
 	factory, err := controllers.NewDefaultRevisionEngineFactory(

applyconfigurations/api/v1/clusterobjectsetspec.go

@@ -48,15 +48,12 @@ import (
 	crcache "sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/certwatcher"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/event"
 	crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
-	crhandler "sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/metrics/filters"
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
-	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
 
@@ -75,7 +72,6 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/certproviders"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/scheme"
-	"github.com/operator-framework/operator-controller/internal/shared/clusterobjectset"
 	sharedcontrollers "github.com/operator-framework/operator-controller/internal/shared/controllers"
 	cacheutil "github.com/operator-framework/operator-controller/internal/shared/util/cache"
 	fsutil "github.com/operator-framework/operator-controller/internal/shared/util/fs"
@@ -425,9 +421,11 @@ func run() error {
 	}
 
 	cl := mgr.GetClient()
+	// TODO(COD): Remove this registration when this manager neither hosts the COS
+	// reconciler nor queries COS revisions directly. COD integration should replace
+	// the direct ClusterExtension-to-COS queries, but the COS reconciler needs the index.
 	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
-		if err := mgr.GetFieldIndexer().IndexField(context.Background(), &ocv1.ClusterObjectSet{},
-			clusterobjectset.GroupField, clusterobjectset.ExtractGroup); err != nil {
+		if err := clusterobjctrl.SetupIndexes(context.Background(), mgr.GetFieldIndexer()); err != nil {
 			return fmt.Errorf("indexing ClusterObjectSet group: %w", err)
 		}
 	}
@@ -484,25 +482,14 @@ func run() error {
 		return err
 	}
 
-	var ctrlBuilderOpts []controllers.ControllerBuilderOption
-	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
-		ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithClusterObjectSetWatch())
-	} else {
-		ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithWatchesRawSource(
-			trackingCache.Source(
-				crhandler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &ocv1.ClusterExtension{}),
-				predicate.ResourceVersionChangedPredicate{},
-				predicate.Funcs{
-					CreateFunc: func(event.TypedCreateEvent[client.Object]) bool { return false },
-				},
-			),
-		))
-	}
-
 	ceReconciler := &controllers.ClusterExtensionReconciler{
 		Client: cl,
 	}
-	_, err = ceReconciler.SetupWithManager(mgr, ctrlBuilderOpts...)
+	if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
+		_, err = ceReconciler.SetupWithManagerForBoxcutter(mgr)
+	} else {
+		_, err = ceReconciler.SetupWithManagerForHelm(mgr, trackingCache)
+	}
 	if err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
 		return err