🐛 Ignore foreign COD revisions in ClusterExtension status

internal/operator-controller/controllers/boxcutter_reconcile_steps.go

@@ -23,6 +23,7 @@ import (
 	"io/fs"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -40,17 +41,37 @@ type BoxcutterRevisionStatesGetter struct {
 }
 
 func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, ext *ocv1.ClusterExtension) (*RevisionStates, error) {
+	rs := &RevisionStates{}
+	cod := &ocv1.ClusterObjectDeployment{}
+	if err := d.Reader.Get(ctx, client.ObjectKey{Name: ext.Name}, cod); err != nil {
+		if apierrors.IsNotFound(err) {
+			// Without a COD, there is no revision state this ClusterExtension should report.
+			return rs, nil
+		}
+		return nil, fmt.Errorf("getting ClusterObjectDeployment %q: %w", ext.Name, err)
+	}
+	if !metav1.IsControlledBy(cod, ext) {
+		// Do not report status for a deployment owned by someone else, even if it
+		// has the name this ClusterExtension expects to use.
+		return rs, nil
+	}
+
 	existingRevisions, err := revisions.ListForGroup(ctx, d.Reader, ext.Name)
 	if err != nil {
 		return nil, fmt.Errorf("listing revisions: %w", err)
 	}
 
-	rs := &RevisionStates{}
 	for _, rev := range existingRevisions {
 		if rev.Spec.LifecycleState == ocv1.ClusterObjectSetLifecycleStateArchived {
 			continue
 		}
 
+		// Group membership is based on the name and does not prove ownership.
+		// Ignore revisions outside this deployment's ownership chain.
+		if !metav1.IsControlledBy(&rev, cod) {
+			continue
+		}
+
 		// TODO: the setting of these annotations (happens in boxcutter applier when we pass in "revisionAnnotations")
 		//   is fairly decoupled from this code where we get the annotations back out. We may want to co-locate
 		//   the set/get logic a bit better to make it more maintainable and less likely to get out of sync.

internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go

@@ -18,28 +18,40 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	apimachineryruntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers"
 	"github.com/operator-framework/operator-controller/internal/testutil"
 )
 
-func TestBoxcutterRevisionStatesGetter_ClassifiesByCompletedAt(t *testing.T) {
+func TestBoxcutterRevisionStatesGetter_ClassifiesAndFiltersRevisions(t *testing.T) {
 	sch := apimachineryruntime.NewScheme()
 	require.NoError(t, ocv1.AddToScheme(sch))
 
 	const extName = "test-ext"
+	ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: extName, UID: "test-ext-uid"}}
+	cod := &ocv1.ClusterObjectDeployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            extName,
+			UID:             "test-cod-uid",
+			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))},
+		},
+	}
 
 	newRevision := func(name string, revision int64, completed bool) *ocv1.ClusterObjectSet {
 		cos := &ocv1.ClusterObjectSet{
 			ObjectMeta: metav1.ObjectMeta{
-				Name: name,
+				Name:            name,
+				OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(cod, ocv1.GroupVersion.WithKind(ocv1.ClusterObjectDeploymentKind))},
 			},
 			Spec: ocv1.ClusterObjectSetSpec{Group: extName, Revision: revision},
 		}
@@ -51,15 +63,19 @@ func TestBoxcutterRevisionStatesGetter_ClassifiesByCompletedAt(t *testing.T) {
 
 	completed := newRevision("test-ext-1", 1, true)
 	rollingOut := newRevision("test-ext-2", 2, false)
+	archived := newRevision("test-ext-3", 3, true)
+	archived.Spec.LifecycleState = ocv1.ClusterObjectSetLifecycleStateArchived
+	foreign := newRevision("test-ext-4", 4, true)
+	foreign.OwnerReferences[0].UID = "another-cod-uid"
+	unowned := newRevision("test-ext-5", 5, true)
+	unowned.SetOwnerReferences(nil)
 
 	cl := testutil.WithIndexes(t, fake.NewClientBuilder().WithScheme(sch), coscontrollers.SetupIndexes).
-		WithObjects(completed, rollingOut).
+		WithObjects(cod, completed, rollingOut, archived, foreign, unowned).
 		Build()
 
 	getter := &BoxcutterRevisionStatesGetter{Reader: cl}
-	states, err := getter.GetRevisionStates(context.Background(), &ocv1.ClusterExtension{
-		ObjectMeta: metav1.ObjectMeta{Name: extName},
-	})
+	states, err := getter.GetRevisionStates(context.Background(), ext)
 	require.NoError(t, err)
 
 	// A revision with completedAt set is Installed.
@@ -69,4 +85,70 @@ func TestBoxcutterRevisionStatesGetter_ClassifiesByCompletedAt(t *testing.T) {
 	// A revision without completedAt is still RollingOut.
 	require.Len(t, states.RollingOut, 1)
 	require.Equal(t, "test-ext-2", states.RollingOut[0].RevisionName)
+
+	// The foreign, archived, and unowned revisions should not be reported at all.
+	require.NotContains(t, allRevisionNames(states), foreign.Name)
+	require.NotContains(t, allRevisionNames(states), archived.Name)
+	require.NotContains(t, allRevisionNames(states), unowned.Name)
+}
+
+func allRevisionNames(states *RevisionStates) []string {
+	if states == nil {
+		return nil
+	}
+	names := sets.Set[string]{}
+	if states.Installed != nil {
+		names.Insert(states.Installed.RevisionName)
+	}
+	for _, rollingOut := range states.RollingOut {
+		if rollingOut != nil {
+			names.Insert(rollingOut.RevisionName)
+		}
+	}
+	return sets.List[string](names)
+}
+
+func TestBoxcutterRevisionStatesGetter_DoesNotReportUnownedResources(t *testing.T) {
+	sch := apimachineryruntime.NewScheme()
+	require.NoError(t, ocv1.AddToScheme(sch))
+
+	ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-ext-uid"}}
+	unownedCOS := &ocv1.ClusterObjectSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: fmt.Sprintf("%s-1", ext.Name),
+		},
+		Spec:   ocv1.ClusterObjectSetSpec{Group: ext.Name, Revision: 1},
+		Status: ocv1.ClusterObjectSetStatus{CompletedAt: metav1.Now()},
+	}
+	otherCOD := &ocv1.ClusterObjectDeployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: ext.Name,
+			UID:  "test-cod-uid",
+		},
+	}
+	otherCOS := &ocv1.ClusterObjectSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            fmt.Sprintf("%s-1", ext.Name),
+			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(otherCOD, ocv1.GroupVersion.WithKind(ocv1.ClusterObjectDeploymentKind))},
+		},
+		Spec:   ocv1.ClusterObjectSetSpec{Group: ext.Name, Revision: 1},
+		Status: ocv1.ClusterObjectSetStatus{CompletedAt: metav1.Now()},
+	}
+
+	for _, tc := range []struct {
+		name    string
+		objects []client.Object
+	}{
+		{name: "missing deployment", objects: []client.Object{unownedCOS}},
+		{name: "COD/COS not owned by extension", objects: []client.Object{otherCOD, otherCOS}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			cl := fake.NewClientBuilder().WithScheme(sch).WithObjects(tc.objects...).Build()
+			getter := &BoxcutterRevisionStatesGetter{Reader: cl}
+
+			states, err := getter.GetRevisionStates(context.Background(), ext)
+			require.NoError(t, err)
+			require.Equal(t, &RevisionStates{}, states)
+		})
+	}
 }

internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go

@@ -23,45 +23,66 @@ func TestBoxcutterRevisionStatesGetter_GroupIndex(t *testing.T) {
 	cl, err := client.New(config, client.Options{Scheme: scheme})
 	require.NoError(t, err)
 	group := "cache-group"
-	ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: group, UID: "cache-group-uid"}}
-	for _, tc := range []struct {
+	ext := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: group, UID: "cache-group-ext-uid"}}
+	cod := &ocv1.ClusterObjectDeployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:            group,
+			UID:             "cache-group-cod-uid",
+			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))},
+		},
+		Spec: ocv1.ClusterObjectDeploymentSpec{
+			Template: ocv1.ClusterObjectDeploymentTemplate{
+				Spec: ocv1.ClusterObjectDeploymentTemplateSpec{
+					CollisionProtection: ocv1.CollisionProtectionPrevent,
+					Phases:              []ocv1.ClusterObjectSetPhase{{Name: "deploy", Objects: []ocv1.ClusterObjectSetObject{}}},
+				},
+			},
+		},
+	}
+	require.NoError(t, cl.Create(t.Context(), cod))
+	t.Cleanup(func() { require.NoError(t, client.IgnoreNotFound(cl.Delete(context.Background(), cod))) })
+
+	for _, cosDef := range []struct {
 		name      string
 		group     string
 		revision  int64
 		archived  bool
 		succeeded bool
 		ownerName string
 	}{
-		{name: "cache-group-1", group: group, revision: 1, succeeded: true, ownerName: "other-owner"},
-		{name: "cache-group-2", group: group, revision: 2},
+		// Only keep non-archived COSs with the matching group AND ownerName
+		{name: "cache-group-1", group: group, revision: 1, succeeded: true, ownerName: group},
+		{name: "cache-group-2", group: group, revision: 2, ownerName: group},
 		{name: "cache-group-3", group: group, revision: 3, archived: true, succeeded: true, ownerName: group},
+		{name: "cache-group-4", group: group, revision: 4, succeeded: true},
+		{name: "cache-group-5", group: group, revision: 5, ownerName: "different-group"},
 		{name: "other-group-99", group: "other-group", revision: 99, succeeded: true, ownerName: group},
 	} {
 		cos := &ocv1.ClusterObjectSet{
-			ObjectMeta: metav1.ObjectMeta{Name: tc.name},
+			ObjectMeta: metav1.ObjectMeta{Name: cosDef.name},
 			Spec: ocv1.ClusterObjectSetSpec{
-				Group: tc.group, Revision: tc.revision,
+				Group: cosDef.group, Revision: cosDef.revision,
 				LifecycleState:      ocv1.ClusterObjectSetLifecycleStateActive,
 				CollisionProtection: ocv1.CollisionProtectionPrevent,
 			},
 		}
-		if tc.group != group {
+		if cosDef.group != group {
 			cos.Labels = map[string]string{labels.OwnerNameKey: group}
 		}
-		if tc.ownerName != "" {
-			owner := *metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))
-			owner.Name = tc.ownerName
-			if tc.ownerName != ext.Name {
+		if cosDef.ownerName != "" {
+			owner := *metav1.NewControllerRef(cod, ocv1.GroupVersion.WithKind(ocv1.ClusterObjectDeploymentKind))
+			owner.Name = cosDef.ownerName
+			if cosDef.ownerName != cod.Name {
 				owner.UID = "other-owner-uid"
 			}
 			cos.OwnerReferences = []metav1.OwnerReference{owner}
 		}
-		if tc.archived {
+		if cosDef.archived {
 			cos.Spec.LifecycleState = ocv1.ClusterObjectSetLifecycleStateArchived
 		}
 		require.NoError(t, cl.Create(t.Context(), cos))
 		t.Cleanup(func() { require.NoError(t, client.IgnoreNotFound(cl.Delete(context.Background(), cos))) })
-		if tc.succeeded {
+		if cosDef.succeeded {
 			cos.Status.CompletedAt = metav1.Now()
 			require.NoError(t, cl.Status().Update(t.Context(), cos))
 		}