diff --git a/internal/object-controller/controllers/clusterobjectset_controller.go b/internal/object-controller/controllers/clusterobjectset_controller.go index 92b35db789..fff2ae209d 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller.go +++ b/internal/object-controller/controllers/clusterobjectset_controller.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/workqueue" "k8s.io/utils/clock" + "k8s.io/utils/ptr" "pkg.package-operator.run/boxcutter" "pkg.package-operator.run/boxcutter/machinery" machinerytypes "pkg.package-operator.run/boxcutter/machinery/types" @@ -75,7 +76,7 @@ type trackingCache interface { //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectsets,verbs=get;list;watch;update;patch;create;delete //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectsets/status,verbs=update;patch //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectsets/finalizers,verbs=update -//+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch +//+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;patch func (c *ClusterObjectSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { l := log.FromContext(ctx).WithName("cluster-extension-revision") @@ -148,6 +149,10 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl return c.delete(ctx, cos) } + if err := c.claimReferencedSecrets(ctx, cos); err != nil { + return ctrl.Result{}, fmt.Errorf("claiming referenced Secrets: %w", err) + } + remaining, hasDeadline := durationUntilDeadline(c.Clock, cos) isDeadlineExceeded := hasDeadline && remaining <= 0 @@ -895,14 +900,25 @@ func (e *mutableSecretError) Error() string { return e.msg } // have Immutable set to true. A confirmed mutable Secret is returned as a // *mutableSecretError listing every misconfigured Secret; a failure to read a // Secret is returned as a plain (retryable) error. -func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx context.Context, cos *ocv1.ClusterObjectSet) error { - type secretRef struct { - name string - namespace string - } +// secretRef identifies a referenced Secret by name and namespace. +type secretRef struct { + name string + namespace string +} + +// referencedSecretRefs returns the deduplicated Secrets referenced by the +// ClusterObjectSet's phase objects, in first-seen order. Objects with no Secret +// reference are skipped. +// +// TODO: the ObjectSourceRef.namespace API doc says an empty namespace "defaults to the +// OLM system namespace during ref resolution", but that defaulting is not implemented +// anywhere (there is no system namespace available to this controller). Namespaces are +// used verbatim here to match resolveObjectRef and verifyReferencedSecretsImmutable, so +// ownership targets exactly the Secret resolution reads. If the documented defaulting is +// implemented, apply it here so claim, verification, and resolution stay consistent. +func referencedSecretRefs(cos *ocv1.ClusterObjectSet) []secretRef { seen := make(map[secretRef]struct{}) var refs []secretRef - for _, phase := range cos.Spec.Phases { for _, obj := range phase.Objects { if obj.Ref.Name == "" { @@ -915,10 +931,13 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex } } } + return refs +} +func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx context.Context, cos *ocv1.ClusterObjectSet) error { var mutableSecrets []string var readErr error - for _, ref := range refs { + for _, ref := range referencedSecretRefs(cos) { secret := &corev1.Secret{} key := client.ObjectKey{Name: ref.name, Namespace: ref.namespace} if err := c.Client.Get(ctx, key, secret); err != nil { @@ -953,3 +972,58 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex return nil } + +// claimReferencedSecrets adds a non-controller ownerReference from the ClusterObjectSet +// to each referenced Secret for garbage collection. It is idempotent: if the Secret already +// has an ownerReference with the COS's UID, no change is made (so a pre-existing owner +// reference to this COS is left untouched). Claiming is best-effort: Secrets that cannot be +// read (NotFound or other read errors) are skipped and will be claimed on a later reconcile. +// Only transient Patch failures return an error. +func (c *ClusterObjectSetReconciler) claimReferencedSecrets(ctx context.Context, cos *ocv1.ClusterObjectSet) error { + // Claim each unique referenced Secret. + for _, ref := range referencedSecretRefs(cos) { + secret := &corev1.Secret{} + key := client.ObjectKey{Name: ref.name, Namespace: ref.namespace} + if err := c.Client.Get(ctx, key, secret); err != nil { + // Best-effort: skip any Secret we cannot read (NotFound or transient read error). + // The Secret will be claimed on a later reconcile once readable. Claiming must not + // preempt downstream reconcile logic (e.g., verifyReferencedSecretsImmutable's + // precedence handling for mutable Secrets over read errors). + continue + } + + // Check if already owned by this COS UID (idempotency) + alreadyOwned := false + for _, owner := range secret.OwnerReferences { + if owner.UID == cos.UID { + alreadyOwned = true + break + } + } + if alreadyOwned { + continue + } + + // Add non-controller ownerRef + ownerRef := metav1.OwnerReference{ + APIVersion: ocv1.GroupVersion.String(), + Kind: ocv1.ClusterObjectSetKind, + Name: cos.Name, + UID: cos.UID, + Controller: ptr.To(false), + } + + // Apply a merge patch with an optimistic lock. The merge patch replaces the + // ownerReferences list, so the resourceVersion precondition is required to avoid + // overwriting a concurrent writer's owner reference (e.g. a sibling revision + // claiming the same content-addressed Secret) from a stale read: a conflicting + // write returns a Conflict error and is retried on the next reconcile. + patch := client.MergeFromWithOptions(secret.DeepCopy(), client.MergeFromWithOptimisticLock{}) + secret.OwnerReferences = append(secret.OwnerReferences, ownerRef) + if err := c.Client.Patch(ctx, secret, patch); err != nil { + return fmt.Errorf("patching Secret %s/%s: %w", ref.namespace, ref.name, err) + } + } + + return nil +} diff --git a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go index 61e2d097c7..1789a9cde2 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_internal_test.go @@ -569,3 +569,362 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) { assert.Equal(t, int32(1), secretGetCount.Load(), "secret should be fetched only once despite multiple references") }) } + +func Test_ClusterObjectSetReconciler_claimReferencedSecrets(t *testing.T) { + testScheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(testScheme)) + require.NoError(t, corev1.AddToScheme(testScheme)) + + cosUID := types.UID("test-cos-uid") + cosName := "test-cos" + + t.Run("object-data secret gets a non-controller ownerRef to the COS", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "test-ns", + }, + Type: corev1.SecretType("olm.operatorframework.io/object-data"), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: cosName, + UID: cosUID, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + } + + err := reconciler.claimReferencedSecrets(context.Background(), cos) + require.NoError(t, err) + + // Verify the secret has the expected ownerRef + updatedSecret := &corev1.Secret{} + err = testClient.Get(context.Background(), client.ObjectKey{Name: "test-secret", Namespace: "test-ns"}, updatedSecret) + require.NoError(t, err) + require.Len(t, updatedSecret.OwnerReferences, 1) + assert.Equal(t, ocv1.GroupVersion.String(), updatedSecret.OwnerReferences[0].APIVersion) + assert.Equal(t, ocv1.ClusterObjectSetKind, updatedSecret.OwnerReferences[0].Kind) + assert.Equal(t, cosName, updatedSecret.OwnerReferences[0].Name) + assert.Equal(t, cosUID, updatedSecret.OwnerReferences[0].UID) + assert.False(t, *updatedSecret.OwnerReferences[0].Controller) + assert.Nil(t, updatedSecret.OwnerReferences[0].BlockOwnerDeletion) + }) + + t.Run("idempotent: re-running adds no second ref", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "test-ns", + }, + Type: corev1.SecretType("olm.operatorframework.io/object-data"), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: cosName, + UID: cosUID, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + } + + // Call claim twice + err := reconciler.claimReferencedSecrets(context.Background(), cos) + require.NoError(t, err) + err = reconciler.claimReferencedSecrets(context.Background(), cos) + require.NoError(t, err) + + // Verify still exactly one ownerRef + updatedSecret := &corev1.Secret{} + err = testClient.Get(context.Background(), client.ObjectKey{Name: "test-secret", Namespace: "test-ns"}, updatedSecret) + require.NoError(t, err) + require.Len(t, updatedSecret.OwnerReferences, 1) + assert.Equal(t, cosUID, updatedSecret.OwnerReferences[0].UID) + }) + + // TODO: this case covers coexistence with the operator-controller applier, which currently + // sets a Controller=true ownerRef on content Secrets. Once the applier is updated to drop/convert + // that controller ownerRef (deferred CE-side work), this subtest can be removed. + t.Run("already owned by this UID (pre-existing Controller=true ref) is left untouched", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "test-ns", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: ocv1.GroupVersion.String(), + Kind: ocv1.ClusterObjectSetKind, + Name: cosName, + UID: cosUID, + Controller: ptr.To(true), + }}, + }, + Type: corev1.SecretType("olm.operatorframework.io/object-data"), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: cosName, + UID: cosUID, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + } + + err := reconciler.claimReferencedSecrets(context.Background(), cos) + require.NoError(t, err) + + // Verify still exactly one ownerRef, Controller still true + updatedSecret := &corev1.Secret{} + err = testClient.Get(context.Background(), client.ObjectKey{Name: "test-secret", Namespace: "test-ns"}, updatedSecret) + require.NoError(t, err) + require.Len(t, updatedSecret.OwnerReferences, 1) + assert.Equal(t, cosUID, updatedSecret.OwnerReferences[0].UID) + assert.True(t, *updatedSecret.OwnerReferences[0].Controller, "Controller should still be true") + }) + + t.Run("non-object-data (Opaque) secret is also claimed", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "test-ns", + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: cosName, + UID: cosUID, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + } + + err := reconciler.claimReferencedSecrets(context.Background(), cos) + require.NoError(t, err) + + // Verify the secret has the expected ownerRef + updatedSecret := &corev1.Secret{} + err = testClient.Get(context.Background(), client.ObjectKey{Name: "test-secret", Namespace: "test-ns"}, updatedSecret) + require.NoError(t, err) + require.Len(t, updatedSecret.OwnerReferences, 1) + assert.False(t, *updatedSecret.OwnerReferences[0].Controller) + }) + + t.Run("missing secret is skipped", func(t *testing.T) { + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: cosName, + UID: cosUID, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "missing-secret", + Namespace: "test-ns", + Key: "obj", + }, + }}, + }}, + }, + } + + err := reconciler.claimReferencedSecrets(context.Background(), cos) + require.NoError(t, err, "should not error when secret is missing") + }) + + t.Run("same secret referenced by two objects is claimed once", func(t *testing.T) { + var patchCount atomic.Int32 + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "test-ns", + }, + Type: corev1.SecretType("olm.operatorframework.io/object-data"), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + "obj2": []byte(`{"apiVersion":"v1","kind":"Secret"}`), + }, + } + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*corev1.Secret); ok { + patchCount.Add(1) + } + return c.Patch(ctx, obj, patch, opts...) + }, + }). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: cosName, + UID: cosUID, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{ + {Ref: ocv1.ObjectSourceRef{Name: "test-secret", Namespace: "test-ns", Key: "obj"}}, + {Ref: ocv1.ObjectSourceRef{Name: "test-secret", Namespace: "test-ns", Key: "obj2"}}, + }, + }}, + }, + } + + err := reconciler.claimReferencedSecrets(context.Background(), cos) + require.NoError(t, err) + + // Verify the secret has exactly one ownerRef and was patched only once + updatedSecret := &corev1.Secret{} + err = testClient.Get(context.Background(), client.ObjectKey{Name: "test-secret", Namespace: "test-ns"}, updatedSecret) + require.NoError(t, err) + require.Len(t, updatedSecret.OwnerReferences, 1) + assert.Equal(t, int32(1), patchCount.Load(), "secret should be patched only once despite multiple references") + }) + + t.Run("empty-namespace ref is resolved at ref.Namespace verbatim", func(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "", + }, + Type: corev1.SecretType("olm.operatorframework.io/object-data"), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(secret). + Build() + + reconciler := &ClusterObjectSetReconciler{Client: testClient} + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: cosName, + UID: cosUID, + }, + Spec: ocv1.ClusterObjectSetSpec{ + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "test-secret", + Namespace: "", + Key: "obj", + }, + }}, + }}, + }, + } + + err := reconciler.claimReferencedSecrets(context.Background(), cos) + require.NoError(t, err) + + // Verify the secret has the expected ownerRef + updatedSecret := &corev1.Secret{} + err = testClient.Get(context.Background(), client.ObjectKey{Name: "test-secret", Namespace: ""}, updatedSecret) + require.NoError(t, err) + require.Len(t, updatedSecret.OwnerReferences, 1) + assert.Equal(t, cosUID, updatedSecret.OwnerReferences[0].UID) + }) +} diff --git a/internal/object-controller/controllers/clusterobjectset_controller_test.go b/internal/object-controller/controllers/clusterobjectset_controller_test.go index 51f3c8502d..ebeb1e00ce 100644 --- a/internal/object-controller/controllers/clusterobjectset_controller_test.go +++ b/internal/object-controller/controllers/clusterobjectset_controller_test.go @@ -18,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/utils/clock" clocktesting "k8s.io/utils/clock/testing" + "k8s.io/utils/ptr" "pkg.package-operator.run/boxcutter" "pkg.package-operator.run/boxcutter/machinery" machinerytypes "pkg.package-operator.run/boxcutter/machinery/types" @@ -2348,3 +2349,233 @@ func Test_ClusterObjectSetReconciler_getScopedClient_Errors(t *testing.T) { require.Contains(t, err.Error(), "token getter failed") }) } + +// Test_ClusterObjectSetReconciler_claimReferencedSecrets_Envtest validates the +// claim-write behavior against a real API server, specifically that the ownerRef +// patch is accepted on immutable Secrets, is idempotent, and coexists with +// controller=true ownerRefs. +func Test_ClusterObjectSetReconciler_claimReferencedSecrets_Envtest(t *testing.T) { + scheme := newScheme(t) + require.NoError(t, corev1.AddToScheme(scheme)) + + cl, err := client.New(config, client.Options{Scheme: scheme}) + require.NoError(t, err) + + ctx := t.Context() + cosUID := types.UID("test-cos-uid") + + t.Run("patch is accepted on immutable Secret", func(t *testing.T) { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "cos-claim-test-"}} + require.NoError(t, cl.Create(ctx, ns)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), ns)) + }) + + secretName := "immutable-secret-1" + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: ns.Name, + }, + Type: corev1.SecretType("olm.operatorframework.io/object-data"), + Immutable: ptr.To(true), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + require.NoError(t, cl.Create(ctx, secret)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), secret)) + }) + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-group-1", + UID: cosUID + "-1", + }, + Spec: ocv1.ClusterObjectSetSpec{ + Revision: 1, + Group: "test-group", + CollisionProtection: ocv1.CollisionProtectionPrevent, + LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: secretName, + Namespace: ns.Name, + Key: "obj", + }, + }}, + }}, + }, + } + require.NoError(t, cl.Create(ctx, cos)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), cos)) + }) + + // Build a minimal reconciler and trigger reconcile + reconciler := &controllers.ClusterObjectSetReconciler{ + Client: cl, + } + + // The reconcile may fail at factory/cache usage, but claimReferencedSecrets + // should execute first (it only uses Client). + _, _ = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cos.Name}}) + + // Verify the patch succeeded: exactly one non-controller ownerRef + updatedSecret := &corev1.Secret{} + require.NoError(t, cl.Get(ctx, client.ObjectKey{Name: secretName, Namespace: ns.Name}, updatedSecret)) + require.Len(t, updatedSecret.OwnerReferences, 1) + require.Equal(t, ocv1.GroupVersion.String(), updatedSecret.OwnerReferences[0].APIVersion) + require.Equal(t, ocv1.ClusterObjectSetKind, updatedSecret.OwnerReferences[0].Kind) + require.Equal(t, cos.Name, updatedSecret.OwnerReferences[0].Name) + require.Equal(t, cos.UID, updatedSecret.OwnerReferences[0].UID) + require.False(t, *updatedSecret.OwnerReferences[0].Controller) + }) + + t.Run("idempotent across real Get/Patch roundtrip", func(t *testing.T) { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "cos-claim-test-"}} + require.NoError(t, cl.Create(ctx, ns)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), ns)) + }) + + secretName := "idempotent-secret-2" //nolint:gosec // G101 false positive: a test Secret resource name, not a credential + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: ns.Name, + }, + Type: corev1.SecretType("olm.operatorframework.io/object-data"), + Immutable: ptr.To(true), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + require.NoError(t, cl.Create(ctx, secret)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), secret)) + }) + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-group-2-1", + UID: cosUID + "-2", + }, + Spec: ocv1.ClusterObjectSetSpec{ + Revision: 1, + Group: "test-group-2", + CollisionProtection: ocv1.CollisionProtectionPrevent, + LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: secretName, + Namespace: ns.Name, + Key: "obj", + }, + }}, + }}, + }, + } + require.NoError(t, cl.Create(ctx, cos)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), cos)) + }) + + reconciler := &controllers.ClusterObjectSetReconciler{ + Client: cl, + } + + // First claim + _, _ = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cos.Name}}) + + // Second claim (idempotency check) + _, _ = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cos.Name}}) + + // Still exactly one ownerRef + updatedSecret := &corev1.Secret{} + require.NoError(t, cl.Get(ctx, client.ObjectKey{Name: secretName, Namespace: ns.Name}, updatedSecret)) + require.Len(t, updatedSecret.OwnerReferences, 1) + require.Equal(t, cos.UID, updatedSecret.OwnerReferences[0].UID) + }) + + // TODO: this case covers coexistence with the operator-controller applier, which currently + // sets a Controller=true ownerRef on content Secrets. Once the applier is updated to drop/convert + // that controller ownerRef (deferred CE-side work), this subtest can be removed. + t.Run("coexists with controller=true ownerRef", func(t *testing.T) { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{GenerateName: "cos-claim-test-"}} + require.NoError(t, cl.Create(ctx, ns)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), ns)) + }) + + cos := &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-group-3-1", + UID: cosUID + "-3", + }, + Spec: ocv1.ClusterObjectSetSpec{ + Revision: 1, + Group: "test-group-3", + CollisionProtection: ocv1.CollisionProtectionPrevent, + LifecycleState: ocv1.ClusterObjectSetLifecycleStateActive, + Phases: []ocv1.ClusterObjectSetPhase{{ + Name: "phase1", + Objects: []ocv1.ClusterObjectSetObject{{ + Ref: ocv1.ObjectSourceRef{ + Name: "coexist-secret-3", + Namespace: ns.Name, + Key: "obj", + }, + }}, + }}, + }, + } + require.NoError(t, cl.Create(ctx, cos)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), cos)) + }) + + // Pre-seed Secret with a pre-existing controller=true ownerRef to this COS + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "coexist-secret-3", + Namespace: ns.Name, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: ocv1.GroupVersion.String(), + Kind: ocv1.ClusterObjectSetKind, + Name: cos.Name, + UID: cos.UID, + Controller: ptr.To(true), + }}, + }, + Type: corev1.SecretType("olm.operatorframework.io/object-data"), + Immutable: ptr.To(true), + Data: map[string][]byte{ + "obj": []byte(`{"apiVersion":"v1","kind":"ConfigMap"}`), + }, + } + require.NoError(t, cl.Create(ctx, secret)) + t.Cleanup(func() { + require.NoError(t, cl.Delete(context.Background(), secret)) + }) + + reconciler := &controllers.ClusterObjectSetReconciler{ + Client: cl, + } + + // Claim should recognize the existing ownerRef and leave it unchanged + _, _ = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cos.Name}}) + + // Still exactly one ownerRef, unchanged (controller=true) + updatedSecret := &corev1.Secret{} + require.NoError(t, cl.Get(ctx, client.ObjectKey{Name: "coexist-secret-3", Namespace: ns.Name}, updatedSecret)) + require.Len(t, updatedSecret.OwnerReferences, 1) + require.Equal(t, cos.UID, updatedSecret.OwnerReferences[0].UID) + require.True(t, *updatedSecret.OwnerReferences[0].Controller) + }) +} diff --git a/test/e2e/features/cos_secret_ownership.feature b/test/e2e/features/cos_secret_ownership.feature new file mode 100644 index 0000000000..6827de6db5 --- /dev/null +++ b/test/e2e/features/cos_secret_ownership.feature @@ -0,0 +1,162 @@ +@BoxcutterRuntime +Feature: ClusterObjectSet and ClusterObjectDeployment Secret Ownership and Garbage Collection + + As an OLM user, when I delete a ClusterObjectSet or ClusterObjectDeployment, + I expect its referenced Secrets to be garbage-collected automatically. + + Cleanup is only guaranteed once the ClusterObjectSet has taken ownership of a + referenced Secret (added its owner reference). Until that point, the Secret's + lifecycle is the responsibility of whoever created it. + + Background: + Given OLM is available + And namespace "${TEST_NAMESPACE}" is available + + Scenario: Deleting a standalone ClusterObjectSet garbage-collects its object-data Secrets + When resource is applied + """ + apiVersion: v1 + kind: Secret + metadata: + name: ${COS_NAME}-secret + namespace: ${TEST_NAMESPACE} + immutable: true + type: olm.operatorframework.io/object-data + stringData: + configmap: | + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "test-cm", + "namespace": "${TEST_NAMESPACE}" + }, + "data": { + "key": "value" + } + } + """ + And ClusterObjectSet is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + name: ${COS_NAME} + spec: + group: ${NAME} + lifecycleState: Active + collisionProtection: Prevent + phases: + - name: resources + objects: + - ref: + name: ${COS_NAME}-secret + namespace: ${TEST_NAMESPACE} + key: configmap + revision: 1 + """ + Then ClusterObjectSet "${COS_NAME}" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "${COS_NAME}" referred secrets have a non-controller owner reference to the object set + And ClusterObjectSet "${COS_NAME}" referred secrets are remembered + When resource "clusterobjectset/${COS_NAME}" is removed + Then resource "clusterobjectset/${COS_NAME}" is eventually not found + And the remembered revision secrets are removed + + Scenario: Deleting a ClusterObjectDeployment garbage-collects its revisions' object-data Secrets + When resource is applied + """ + apiVersion: v1 + kind: Secret + metadata: + name: ${NAME}-cod-secret + namespace: ${TEST_NAMESPACE} + immutable: true + type: olm.operatorframework.io/object-data + stringData: + configmap: | + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "test-cm-cod", + "namespace": "${TEST_NAMESPACE}" + }, + "data": { + "key": "value" + } + } + """ + And resource is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME}-cod + spec: + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: + - ref: + name: ${NAME}-cod-secret + namespace: ${TEST_NAMESPACE} + key: configmap + """ + Then ClusterObjectDeployment "${NAME}-cod" reports Progressing as False with Reason NewClusterObjectSetProgressed + And ClusterObjectDeployment "${NAME}-cod" reports Available as True with Reason Available + And ClusterObjectDeployment "${NAME}-cod" revision secrets are remembered + When resource "clusterobjectdeployment/${NAME}-cod" is removed + Then resource "clusterobjectdeployment/${NAME}-cod" is eventually not found + And the remembered revision secrets are removed + + Scenario: A referenced immutable Opaque Secret is adopted and garbage-collected + When resource is applied + """ + apiVersion: v1 + kind: Secret + metadata: + name: ${COS_NAME}-opaque-secret + namespace: ${TEST_NAMESPACE} + immutable: true + type: Opaque + stringData: + configmap: | + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": { + "name": "test-cm-opaque", + "namespace": "${TEST_NAMESPACE}" + }, + "data": { + "key": "value" + } + } + """ + And ClusterObjectSet is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectSet + metadata: + name: ${COS_NAME} + spec: + group: ${NAME} + lifecycleState: Active + collisionProtection: Prevent + phases: + - name: resources + objects: + - ref: + name: ${COS_NAME}-opaque-secret + namespace: ${TEST_NAMESPACE} + key: configmap + revision: 1 + """ + Then ClusterObjectSet "${COS_NAME}" reports Ready as True with Reason AllObjectsReady + And ClusterObjectSet "${COS_NAME}" referred secrets have a non-controller owner reference to the object set + And ClusterObjectSet "${COS_NAME}" referred secrets are remembered + When resource "clusterobjectset/${COS_NAME}" is removed + Then resource "clusterobjectset/${COS_NAME}" is eventually not found + And the remembered revision secrets are removed diff --git a/test/e2e/steps/group_steps.go b/test/e2e/steps/group_steps.go index 685c4914b4..8084e624b8 100644 --- a/test/e2e/steps/group_steps.go +++ b/test/e2e/steps/group_steps.go @@ -180,12 +180,35 @@ func ClusterObjectSetObjectsUseSSAManager(ctx context.Context, revisionName, man func RememberClusterObjectSetSecrets(ctx context.Context, revisionName string) error { sc := scenarioCtx(ctx) revisionName = substituteScenarioVars(revisionName, sc) - names, err := collectReferredSecretNames(ctx, revisionName) + refs, err := collectReferredSecretRefs(ctx, revisionName) if err != nil { return err } - for _, name := range names { - sc.revisionSecrets = append(sc.revisionSecrets, resource{kind: "secret", name: name, namespace: olmNamespace}) + for _, ref := range refs { + sc.revisionSecrets = append(sc.revisionSecrets, resource{kind: "secret", name: ref.name, namespace: ref.namespace}) + } + return nil +} + +// RememberCODRevisionSecrets remembers all secrets from all ClusterObjectSet revisions +// belonging to the named ClusterObjectDeployment's group for later GC verification. +func RememberCODRevisionSecrets(ctx context.Context, codName string) error { + sc := scenarioCtx(ctx) + codName = substituteScenarioVars(codName, sc) + + revisions, err := listClusterObjectSetsInGroup(ctx, codName) + if err != nil { + return fmt.Errorf("listing revisions for COD %q: %w", codName, err) + } + + for _, rev := range revisions { + refs, err := collectReferredSecretRefs(ctx, rev.Name) + if err != nil { + return fmt.Errorf("collecting secret refs for revision %q: %w", rev.Name, err) + } + for _, ref := range refs { + sc.revisionSecrets = append(sc.revisionSecrets, resource{kind: "secret", name: ref.name, namespace: ref.namespace}) + } } return nil } diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 12d5dafa16..9fd05aaeb5 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -129,6 +129,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectDeployment "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+)$`, ClusterObjectDeploymentReportsConditionWithoutMsg) sc.Step(`^(?i)ClusterObjectDeployment "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) and Message includes:$`, ClusterObjectDeploymentReportsConditionWithMessageFragment) + sc.Step(`^(?i)ClusterObjectDeployment "([^"]+)" revision secrets are remembered$`, RememberCODRevisionSecrets) sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) transition between (\d+) and (\d+) minutes since its creation$`, ClusterExtensionReportsConditionTransitionTime) sc.Step(`^(?i)ClusterObjectSet is applied(?:\s+.*)?$`, ResourceIsApplied) sc.Step(`^(?i)ClusterObjectSet with Namespace owner "([^"]*)" is applied$`, ClusterObjectSetWithNamespaceOwnerApplied) @@ -152,6 +153,7 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets are immutable$`, ClusterObjectSetReferredSecretsAreImmutable) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets contain labels$`, ClusterObjectSetReferredSecretsContainLabels) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets are owned by the object set$`, ClusterObjectSetReferredSecretsOwnedByObjectSet) + sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets have a non-controller owner reference to the object set$`, ClusterObjectSetReferredSecretsHaveNonControllerOwnerReference) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" referred secrets have type "([^"]+)"$`, ClusterObjectSetReferredSecretsHaveType) sc.Step(`^(?i)resource "([^"]+)" is installed$`, ResourceAvailable) @@ -1334,6 +1336,7 @@ func ClusterObjectSetReferredSecretsContainLabels(ctx context.Context, revisionN // ClusterObjectSetReferredSecretsOwnedByObjectSet verifies that all referred Secrets for the named // ClusterObjectSet have an ownerReference pointing to the ClusterObjectSet with controller=true. +// This step is for CE-flow scenarios where secrets are in olmNamespace with revision-name labels. func ClusterObjectSetReferredSecretsOwnedByObjectSet(ctx context.Context, revisionName string) error { sc := scenarioCtx(ctx) revisionName = substituteScenarioVars(strings.TrimSpace(revisionName), sc) @@ -1370,6 +1373,48 @@ func ClusterObjectSetReferredSecretsOwnedByObjectSet(ctx context.Context, revisi return nil } +// ClusterObjectSetReferredSecretsHaveNonControllerOwnerReference verifies that all Secrets +// referenced in the named ClusterObjectSet's spec.phases[].objects[].ref have a non-controller +// ownerReference pointing to the ClusterObjectSet. This is for standalone COS scenarios where +// secrets are created in arbitrary namespaces without labels and claimed with Controller=false. +func ClusterObjectSetReferredSecretsHaveNonControllerOwnerReference(ctx context.Context, revisionName string) error { + sc := scenarioCtx(ctx) + revisionName = substituteScenarioVars(strings.TrimSpace(revisionName), sc) + + cosObj, err := getResource("clusterobjectset", revisionName, "") + if err != nil { + return fmt.Errorf("getting ClusterObjectSet %q: %w", revisionName, err) + } + cosUID := cosObj.GetUID() + + refs, err := collectReferredSecretRefs(ctx, revisionName) + if err != nil { + return err + } + + for _, ref := range refs { + secret, err := getResource("secret", ref.name, ref.namespace) + if err != nil { + return fmt.Errorf("getting Secret %s/%s: %w", ref.namespace, ref.name, err) + } + + found := false + for _, ownerRef := range secret.GetOwnerReferences() { + if ownerRef.Kind == ocv1.ClusterObjectSetKind && ownerRef.Name == revisionName && ownerRef.UID == cosUID { + if ownerRef.Controller != nil && *ownerRef.Controller { + return fmt.Errorf("secret %s/%s has ownerReference to ClusterObjectSet with controller=true, expected non-controller", ref.namespace, ref.name) + } + found = true + break + } + } + if !found { + return fmt.Errorf("secret %s/%s does not have ownerReference to ClusterObjectSet %q (uid %s)", ref.namespace, ref.name, revisionName, cosUID) + } + } + return nil +} + // ClusterObjectSetReferredSecretsHaveType verifies that all referred Secrets for the named // ClusterObjectSet have the specified Secret type. func ClusterObjectSetReferredSecretsHaveType(ctx context.Context, revisionName, expectedType string) error { @@ -1392,9 +1437,16 @@ func ClusterObjectSetReferredSecretsHaveType(ctx context.Context, revisionName, return nil } -// collectReferredSecretNames returns the unique set of Secret names referenced by the ClusterObjectSet's phase objects. -func collectReferredSecretNames(ctx context.Context, revisionName string) ([]string, error) { - var names []string +// secretRef represents a Secret reference with its namespace. +type secretRef struct { + name string + namespace string +} + +// collectReferredSecretRefs returns the unique set of Secret name+namespace pairs +// referenced by the ClusterObjectSet's phase objects. Empty namespace defaults to olmNamespace. +func collectReferredSecretRefs(ctx context.Context, revisionName string) ([]secretRef, error) { + var refs []secretRef seen := sets.New[string]() var obj *unstructured.Unstructured @@ -1417,14 +1469,34 @@ func collectReferredSecretNames(ctx context.Context, revisionName string) ([]str continue } name, _, _ := unstructured.NestedString(phaseObj, "ref", "name") - if name != "" && !seen.Has(name) { - seen.Insert(name) - names = append(names, name) + namespace, _, _ := unstructured.NestedString(phaseObj, "ref", "namespace") + if namespace == "" { + namespace = olmNamespace + } + key := namespace + "/" + name + if name != "" && !seen.Has(key) { + seen.Insert(key) + refs = append(refs, secretRef{name: name, namespace: namespace}) } } } - if len(names) == 0 { - return nil, fmt.Errorf("no referred secret names found in ClusterObjectSet %q", revisionName) + if len(refs) == 0 { + return nil, fmt.Errorf("no referred secret refs found in ClusterObjectSet %q", revisionName) + } + return refs, nil +} + +// collectReferredSecretNames returns the unique set of Secret names referenced by the ClusterObjectSet's phase objects. +// Deprecated: use collectReferredSecretRefs for namespace-aware collection. Will be removed after all CE-flow +// scenarios are verified to work with the namespace-aware version (target: v1.2 or later). +func collectReferredSecretNames(ctx context.Context, revisionName string) ([]string, error) { + refs, err := collectReferredSecretRefs(ctx, revisionName) + if err != nil { + return nil, err + } + names := make([]string, len(refs)) + for i, ref := range refs { + names[i] = ref.name } return names, nil }