From 96cb6bf51d806cc69f1978a56fe6c2d01b28d207 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Thu, 29 Oct 2026 11:35:12 -0400 Subject: [PATCH] Ignore foreign COD revisions in ClusterExtension status Only report revisions owned by the ClusterObjectDeployment associated with the extension, so resources from unrelated deployments do not affect its status. Co-Authored-By: GPT-6 Luna --- .../controllers/boxcutter_reconcile_steps.go | 23 ++++- ...boxcutter_reconcile_steps_internal_test.go | 94 +++++++++++++++++-- .../boxcutter_reconcile_steps_test.go | 47 +++++++--- 3 files changed, 144 insertions(+), 20 deletions(-) diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index 0f7154b409..d390214c1d 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/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. diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go index 9403103696..9b7df6cd96 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps_internal_test.go @@ -18,11 +18,14 @@ 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" @@ -30,16 +33,25 @@ import ( "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) + }) + } } diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go index 9a57f609f8..c20a9309de 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps_test.go @@ -23,8 +23,26 @@ 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 @@ -32,36 +50,39 @@ func TestBoxcutterRevisionStatesGetter_GroupIndex(t *testing.T) { 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)) }