fix(object-controller): retry referenced Secret failures

Read each referenced Secret once per reconciliation. Return read errors,
including missing Secrets, so retries verify immutability before decoding.
Requeue while referenced Secrets remain mutable and log this expected waiting
state at debug level. Deduplicate and verify references in one pass.

Watch metadata changes on ClusterObjectSet-owned Secrets across namespaces
without caching their contents or relying on the pull-secret cache. Cover
missing-Secret recovery and owned-Secret replacement in regression tests.

Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED

Author: fao89

cmd/object-controller/main_test.go

@@ -19,6 +19,7 @@ import (
 	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/object-controller/scheme"
@@ -93,7 +94,7 @@ func TestStandaloneController(t *testing.T) {
 	defer syncCancel()
 	require.True(t, mgr.GetCache().WaitForCacheSync(syncCtx), "manager cache did not synchronize")
 
-	for _, name := range []string{"inline", "secret-ref"} {
+	for _, name := range []string{"inline", "secret-ref", "mutable-secret-ref"} {
 		t.Run(name, func(t *testing.T) {
 			ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "standalone-"}}
 			require.NoError(t, cl.Create(ctx, ns))
@@ -103,12 +104,13 @@ func TestStandaloneController(t *testing.T) {
 				"data":     map[string]any{"hello": "world"},
 			}}
 			obj := ocv1.ClusterObjectSetObject{Object: manifest}
-			if name == "secret-ref" {
+			var secret *corev1.Secret
+			if name != "inline" {
 				data, err := json.Marshal(manifest.Object)
 				require.NoError(t, err)
-				secret := &corev1.Secret{
+				secret = &corev1.Secret{
 					ObjectMeta: metav1.ObjectMeta{Name: "content", Namespace: ns.Name},
-					Immutable:  ptr.To(true), Data: map[string][]byte{"object": data},
+					Immutable:  ptr.To(name != "mutable-secret-ref"), Data: map[string][]byte{"object": data},
 				}
 				require.NoError(t, cl.Create(ctx, secret))
 				obj = ocv1.ClusterObjectSetObject{Ref: ocv1.ObjectSourceRef{Name: secret.Name, Namespace: secret.Namespace, Key: "object"}}
@@ -122,6 +124,39 @@ func TestStandaloneController(t *testing.T) {
 				},
 			}
 			require.NoError(t, cl.Create(ctx, cos))
+			if secret != nil {
+				require.NoError(t, controllerutil.SetControllerReference(cos, secret, scheme.Scheme))
+				require.NoError(t, cl.Update(ctx, secret))
+			}
+			rolloutTimeout := time.Minute
+			if name == "mutable-secret-ref" {
+				require.EventuallyWithT(t, func(collect *assert.CollectT) {
+					if !assert.NoError(collect, cl.Get(ctx, client.ObjectKeyFromObject(cos), cos)) {
+						return
+					}
+					condition := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
+					if assert.NotNil(collect, condition) {
+						assert.Equal(collect, ocv1.ClusterObjectSetReasonBlocked, condition.Reason)
+						assert.Contains(collect, condition.Message, "not immutable")
+					}
+				}, 5*time.Second, 100*time.Millisecond)
+
+				// Let status-triggered reconciliations settle before changing only
+				// the Secret. Recovery must precede the 10-second polling retry.
+				lastVersion, unchangedSince := cos.ResourceVersion, time.Now()
+				require.Eventually(t, func() bool {
+					if err := cl.Get(ctx, client.ObjectKeyFromObject(cos), cos); err != nil {
+						return false
+					}
+					if cos.ResourceVersion != lastVersion {
+						lastVersion, unchangedSince = cos.ResourceVersion, time.Now()
+					}
+					return time.Since(unchangedSince) >= time.Second
+				}, 3*time.Second, 100*time.Millisecond)
+				secret.Immutable = ptr.To(true)
+				require.NoError(t, cl.Update(ctx, secret))
+				rolloutTimeout = 5 * time.Second
+			}
 			require.EventuallyWithT(t, func(collect *assert.CollectT) {
 				if !assert.NoError(collect, cl.Get(ctx, client.ObjectKeyFromObject(cos), cos)) {
 					return
@@ -136,13 +171,52 @@ func TestStandaloneController(t *testing.T) {
 				if assert.NotNil(collect, progressing) {
 					assert.Equal(collect, ocv1.ReasonSucceeded, progressing.Reason)
 				}
-			}, time.Minute, 100*time.Millisecond)
+			}, rolloutTimeout, 100*time.Millisecond)
 			cm := &corev1.ConfigMap{}
 			require.NoError(t, cl.Get(ctx, client.ObjectKey{Name: name, Namespace: ns.Name}, cm))
 			require.Equal(t, "world", cm.Data["hello"])
 			require.NotNil(t, metav1.GetControllerOf(cm))
 			require.Equal(t, cos.UID, metav1.GetControllerOf(cm).UID)
 
+			if secret != nil {
+				// A completed COS does not poll. Replacing an owned source Secret
+				// must trigger content verification, and restoring it must unblock
+				// reconciliation without changing the COS or its managed objects.
+				original := secret.DeepCopy()
+				require.NoError(t, cl.Delete(ctx, secret))
+				secret.ResourceVersion = ""
+				secret.UID = ""
+				changed := manifest.DeepCopy()
+				changed.Object["data"] = map[string]any{"hello": "changed"}
+				secret.Data["object"], err = json.Marshal(changed.Object)
+				require.NoError(t, err)
+				require.NoError(t, cl.Create(ctx, secret))
+				require.EventuallyWithT(t, func(collect *assert.CollectT) {
+					if !assert.NoError(collect, cl.Get(ctx, client.ObjectKeyFromObject(cos), cos)) {
+						return
+					}
+					condition := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
+					if assert.NotNil(collect, condition) {
+						assert.Equal(collect, ocv1.ClusterObjectSetReasonBlocked, condition.Reason)
+						assert.Contains(collect, condition.Message, "resolved content of 1 phase(s) has changed")
+					}
+				}, 30*time.Second, 100*time.Millisecond)
+
+				require.NoError(t, cl.Delete(ctx, secret))
+				original.ResourceVersion = ""
+				original.UID = ""
+				require.NoError(t, cl.Create(ctx, original))
+				require.EventuallyWithT(t, func(collect *assert.CollectT) {
+					if !assert.NoError(collect, cl.Get(ctx, client.ObjectKeyFromObject(cos), cos)) {
+						return
+					}
+					condition := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
+					if assert.NotNil(collect, condition) {
+						assert.Equal(collect, ocv1.ReasonSucceeded, condition.Reason)
+					}
+				}, 30*time.Second, 100*time.Millisecond)
+			}
+
 			// Observe managed-object changes without updating the ClusterObjectSet.
 			originalUID := cm.UID
 			require.NoError(t, cl.Delete(ctx, cm))

cmd/operator-controller/main.go

@@ -685,10 +685,9 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
 		return fmt.Errorf("unable to create revision engine factory: %w", err)
 	}
 
-	cosClient := &secretFallbackClient{
-		Client:          c.mgr.GetClient(),
-		apiReader:       c.mgr.GetAPIReader(),
-		systemNamespace: cfg.systemNamespace,
+	cosClient := &uncachedSecretClient{
+		Client:    c.mgr.GetClient(),
+		apiReader: c.mgr.GetAPIReader(),
 	}
 	if err = (&clusterobjctrl.ClusterObjectSetReconciler{
 		Client:                cosClient,
@@ -762,16 +761,14 @@ func main() {
 	}
 }
 
-// secretFallbackClient wraps a cached client.Client and falls back to direct
-// API reads for Secrets outside the system namespace, where the cache does not watch.
-type secretFallbackClient struct {
+// uncachedSecretClient bypasses the cache for Secret reads.
+type uncachedSecretClient struct {
 	client.Client
-	apiReader       client.Reader
-	systemNamespace string
+	apiReader client.Reader
 }
 
-func (c *secretFallbackClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
-	if _, isSecret := obj.(*corev1.Secret); isSecret && key.Namespace != c.systemNamespace {
+func (c *uncachedSecretClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	if _, isSecret := obj.(*corev1.Secret); isSecret {
 		return c.apiReader.Get(ctx, key, obj, opts...)
 	}
 	return c.Client.Get(ctx, key, obj, opts...)

cmd/operator-controller/secret_client_test.go

@@ -0,0 +1,70 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	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/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	ocv1 "github.com/operator-framework/operator-controller/api/v1"
+)
+
+func TestSecretClientReadsCurrentSecrets(t *testing.T) {
+	testScheme := runtime.NewScheme()
+	require.NoError(t, corev1.AddToScheme(testScheme))
+	for _, namespace := range []string{"olmv1-system", "extension"} {
+		for _, deleted := range []bool{false, true} {
+			name := namespace + "/updated"
+			if deleted {
+				name = namespace + "/deleted"
+			}
+			t.Run(name, func(t *testing.T) {
+				stale := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{Name: "content", Namespace: namespace},
+					Data:       map[string][]byte{"object": []byte("old content")},
+				}
+				cachedClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(stale).Build()
+				apiBuilder := fake.NewClientBuilder().WithScheme(testScheme)
+				if !deleted {
+					current := stale.DeepCopy()
+					current.Data["object"] = []byte("new content")
+					apiBuilder.WithObjects(current)
+				}
+				cl := &uncachedSecretClient{
+					Client: cachedClient, apiReader: apiBuilder.Build(),
+				}
+				secret := &corev1.Secret{}
+				err := cl.Get(t.Context(), client.ObjectKeyFromObject(stale), secret)
+				if deleted {
+					require.True(t, apierrors.IsNotFound(err), "must not return a cached Secret after deletion, got %v", err)
+					return
+				}
+				require.NoError(t, err)
+				require.Equal(t, []byte("new content"), secret.Data["object"])
+			})
+		}
+	}
+}
+
+func TestSecretClientUsesCacheForOtherObjects(t *testing.T) {
+	testScheme := runtime.NewScheme()
+	require.NoError(t, ocv1.AddToScheme(testScheme))
+	cached := &ocv1.ClusterObjectSet{
+		ObjectMeta: metav1.ObjectMeta{Name: "revision"},
+		Spec:       ocv1.ClusterObjectSetSpec{Revision: 1},
+	}
+	current := cached.DeepCopy()
+	current.Spec.Revision = 2
+	cl := &uncachedSecretClient{
+		Client:    fake.NewClientBuilder().WithScheme(testScheme).WithObjects(cached).Build(),
+		apiReader: fake.NewClientBuilder().WithScheme(testScheme).WithObjects(current).Build(),
+	}
+	cos := &ocv1.ClusterObjectSet{}
+	require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(cached), cos))
+	require.Equal(t, int64(1), cos.Spec.Revision)
+}