From dc3b458a706a995a186a81e7e2a74e36ae7a60ee Mon Sep 17 00:00:00 2001 From: Evan Mahoney <16479213+emmahone@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:37:31 -0400 Subject: [PATCH 1/2] OCPBUGS-100179: fix forward controller FilterFunc and finalizer removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacyImagePullSecretController has two related defects that together cause dockercfg secrets to remain stuck with an openshift.io/legacy-token finalizer after their namespace's deletionTimestamp is set, leaving namespaces permanently in Terminating. **Defect 1 — FilterFunc silently drops transitioning secrets (new fix)** The informer FilterFunc required the openshift.io/token-secret.name annotation to be present. Secrets that have already transitioned to the "bound" auth type (annotation removed, openshift.io/internal-registry-auth-token.binding: bound) but still carry the legacy-token finalizer were therefore never queued for reconciliation. Once a namespace is deleted and deletionTimestamp is set on these secrets, sync() is never invoked, the finalizer is never cleared, and the namespace hangs indefinitely. Fix: extend the FilterFunc to also pass secrets whose deletionTimestamp is set and that still carry the openshift.io/legacy-token finalizer, regardless of the token-secret.name annotation. The existing sync() deletion path already handles the absent-annotation case correctly (len(t)==0 skips token deletion and proceeds straight to finalizer removal). **Defect 2 — Apply with nil finalizers does not clear the field (cleanup fix)** The deletion path built a filtered finalizers slice and called Apply with it. When openshift.io/legacy-token was the only finalizer, the slice was nil; the applyconfigurations field is tagged omitempty, so nil serialises as absent from the patch body. SSA therefore does not touch the finalizers field and the finalizer persists. Fix: use a JSON Patch (identical to the rollback controller's approach) which directly removes the specific finalizer by index and is not subject to omitempty serialisation. The "test" op before the "remove" ensures safe concurrent writes by failing fast if the cache is stale. Fixes: https://issues.redhat.com/browse/OCPBUGS-100179 --- .../legacy_image_pull_secret_controller.go | 30 +++-- ...egacy_image_pull_secret_controller_test.go | 118 ++++++++++++++++++ 2 files changed, 137 insertions(+), 11 deletions(-) create mode 100644 pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go diff --git a/pkg/internalregistry/controllers/legacy_image_pull_secret_controller.go b/pkg/internalregistry/controllers/legacy_image_pull_secret_controller.go index 783efd5f0..e54399b18 100644 --- a/pkg/internalregistry/controllers/legacy_image_pull_secret_controller.go +++ b/pkg/internalregistry/controllers/legacy_image_pull_secret_controller.go @@ -9,6 +9,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" applycorev1 "k8s.io/client-go/applyconfigurations/core/v1" @@ -46,6 +47,14 @@ func NewLegacyImagePullSecretController(client kubernetes.Interface, secrets inf // not an image pull secret return false } + // A secret being deleted that still carries the finalizer must reach + // sync() even after the token-secret.name annotation has been removed + // (i.e. the secret has transitioned to the "bound" auth type but the + // finalizer was never cleared). Without this, sync() is never invoked + // and namespaces are stuck in Terminating indefinitely. + if !secret.DeletionTimestamp.IsZero() && slices.Contains(secret.Finalizers, "openshift.io/legacy-token") { + return true + } if _, ok = secret.Annotations["openshift.io/token-secret.name"]; !ok { // does not appear to be a legacy managed image pull secret return false @@ -100,17 +109,16 @@ func (c *legacyImagePullSecretController) sync(ctx context.Context, key string) return err } } - // either no token secret was specified, or it was successfully deleted. clear finalizer - var finalizers []string - for _, f := range secret.Finalizers { - if f != "openshift.io/legacy-token" { - finalizers = append(finalizers, f) - } - } - patch := applycorev1.Secret(name, ns). - WithAnnotations(map[string]string{InternalRegistryAuthTokenTypeAnnotation: AuthTokenTypeLegacy}). - WithFinalizers(finalizers...) - _, err = c.client.CoreV1().Secrets(ns).Apply(ctx, patch, metav1.ApplyOptions{FieldManager: legacyTokenSecretControllerFieldManager}) + // either no token secret was specified, or it was successfully deleted. + // Remove the finalizer via JSON Patch (same approach as the rollback + // controller) so that a nil finalizers slice is never serialised with + // omitempty, which would leave the finalizer in place. + index := slices.Index(secret.Finalizers, "openshift.io/legacy-token") + patchData := []byte(fmt.Sprintf(`[`+ + `{"op":"test","path":"/metadata/finalizers/%d","value":"openshift.io/legacy-token"},`+ + `{"op":"remove","path":"/metadata/finalizers/%[1]d"}`+ + `]`, index)) + _, err = c.client.CoreV1().Secrets(ns).Patch(ctx, name, types.JSONPatchType, patchData, metav1.PatchOptions{}) return err } // finalizer has already been removed, nothing to do, delete in progress diff --git a/pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go b/pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go new file mode 100644 index 000000000..104e54663 --- /dev/null +++ b/pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go @@ -0,0 +1,118 @@ +package controllers + +import ( + "context" + "testing" + + "golang.org/x/exp/slices" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + listers "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" +) + +// TestLegacyImagePullSecretControllerSync_DeletionPaths exercises the finalizer-removal +// logic in sync(), including the case (OCPBUGS-100179) where a secret has already +// transitioned out of the "token-secret.name" annotation state but still carries the +// openshift.io/legacy-token finalizer and a deletionTimestamp. +func TestLegacyImagePullSecretControllerSync_DeletionPaths(t *testing.T) { + now := metav1.Now() + + mkSecret := func(opts ...func(*corev1.Secret)) *corev1.Secret { + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns1", + Name: "dockercfg-abc", + }, + Type: corev1.SecretTypeDockercfg, + } + for _, f := range opts { + f(s) + } + return s + } + + withFinalizer := func(s *corev1.Secret) { + s.Finalizers = append(s.Finalizers, "openshift.io/legacy-token") + } + withDeletionTimestamp := func(s *corev1.Secret) { + s.DeletionTimestamp = &now + } + withTokenAnnotation := func(s *corev1.Secret) { + if s.Annotations == nil { + s.Annotations = map[string]string{} + } + s.Annotations["openshift.io/token-secret.name"] = "token-abc" + } + + testCases := []struct { + name string + secret *corev1.Secret // object stored in the API server + cachedSecret *corev1.Secret // object returned by the informer lister (nil → same as secret) + wantFinalizer bool // whether legacy-token should still be present after sync + wantErr bool + }{ + { + // Core fix for OCPBUGS-100179: a secret that has deletionTimestamp set and + // the legacy-token finalizer but lacks the token-secret.name annotation + // (i.e. has already transitioned to the "bound" auth type) must have its + // finalizer removed so that namespace deletion can complete. + name: "stuck secret: deletionTimestamp set, finalizer present, no token-secret.name annotation", + secret: mkSecret(withDeletionTimestamp, withFinalizer), + wantFinalizer: false, + }, + { + name: "normal deletion: deletionTimestamp set, finalizer and annotation both present", + secret: mkSecret(withDeletionTimestamp, withFinalizer, withTokenAnnotation), + wantFinalizer: false, + }, + { + name: "deletion already complete: deletionTimestamp set, finalizer already gone", + secret: mkSecret(withDeletionTimestamp), + wantFinalizer: false, + }, + { + // The JSON Patch "test" op verifies the finalizer is still at the cached index + // before removing it. If the cache is stale (finalizers differ), the patch + // fails and sync() should return an error so the item is requeued. + name: "cache stale: cached finalizer index does not match live object", + secret: mkSecret(withDeletionTimestamp, withFinalizer), + cachedSecret: mkSecret(withDeletionTimestamp, func(s *corev1.Secret) { s.Finalizers = []string{"other", "openshift.io/legacy-token"} }), + wantErr: true, + }, + } + + ctx := context.Background() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if tc.cachedSecret == nil { + tc.cachedSecret = tc.secret + } + client := fake.NewSimpleClientset(tc.secret) + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + if err := indexer.Add(tc.cachedSecret); err != nil { + t.Fatal(err) + } + c := legacyImagePullSecretController{ + client: client, + secrets: listers.NewSecretLister(indexer), + } + err := c.sync(ctx, tc.secret.Namespace+"/"+tc.secret.Name) + if (err != nil) != tc.wantErr { + t.Fatalf("sync() error = %v, wantErr %v", err, tc.wantErr) + } + if tc.wantErr { + return + } + actual, err := client.CoreV1().Secrets(tc.secret.Namespace).Get(ctx, tc.secret.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + hasFinalizer := slices.Contains(actual.Finalizers, "openshift.io/legacy-token") + if hasFinalizer != tc.wantFinalizer { + t.Errorf("openshift.io/legacy-token finalizer present = %v, want %v", hasFinalizer, tc.wantFinalizer) + } + }) + } +} From cf05e429c9fafcd628852b464200774f09726b34 Mon Sep 17 00:00:00 2001 From: Evan Mahoney <16479213+emmahone@users.noreply.github.com> Date: Thu, 6 Aug 2026 14:47:20 -0400 Subject: [PATCH 2/2] test: add FilterFunc coverage via informer event test --- ...egacy_image_pull_secret_controller_test.go | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go b/pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go index 104e54663..133bbef3b 100644 --- a/pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go +++ b/pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go @@ -7,6 +7,7 @@ import ( "golang.org/x/exp/slices" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" @@ -116,3 +117,99 @@ func TestLegacyImagePullSecretControllerSync_DeletionPaths(t *testing.T) { }) } } + +// TestLegacyImagePullSecretControllerFilterFunc verifies that the informer +// FilterFunc correctly enqueues (or ignores) secrets based on the new rule: +// a Dockercfg secret being deleted that still carries the legacy-token +// finalizer must reach the queue even without the token-secret.name annotation. +func TestLegacyImagePullSecretControllerFilterFunc(t *testing.T) { + now := metav1.Now() + + testCases := []struct { + name string + secret *corev1.Secret + wantEnqueued bool + }{ + { + name: "stuck secret: deletionTimestamp, finalizer, no annotation — must be enqueued", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns1", + Name: "dockercfg-stuck", + Finalizers: []string{"openshift.io/legacy-token"}, + DeletionTimestamp: &now, + }, + Type: corev1.SecretTypeDockercfg, + }, + wantEnqueued: true, + }, + { + name: "live secret with annotation — must be enqueued", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns1", + Name: "dockercfg-legacy", + Annotations: map[string]string{"openshift.io/token-secret.name": "token-abc"}, + }, + Type: corev1.SecretTypeDockercfg, + }, + wantEnqueued: true, + }, + { + name: "live secret, finalizer but no annotation, no deletionTimestamp — must not be enqueued", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns1", + Name: "dockercfg-bound", + Finalizers: []string{"openshift.io/legacy-token"}, + }, + Type: corev1.SecretTypeDockercfg, + }, + wantEnqueued: false, + }, + { + name: "wrong secret type — must not be enqueued", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns1", + Name: "opaque-secret", + Annotations: map[string]string{"openshift.io/token-secret.name": "token-abc"}, + }, + Type: corev1.SecretTypeOpaque, + }, + wantEnqueued: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + client := fake.NewSimpleClientset(tc.secret) + factory := informers.NewSharedInformerFactory(client, 0) + secretInformer := factory.Core().V1().Secrets() + + c := NewLegacyImagePullSecretController(client, secretInformer) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + factory.Start(ctx.Done()) + cache.WaitForCacheSync(ctx.Done(), secretInformer.Informer().HasSynced) + + wantKey := tc.secret.Namespace + "/" + tc.secret.Name + if tc.wantEnqueued { + if c.queue.Len() == 0 { + t.Fatalf("expected %q in queue but queue is empty", wantKey) + } + key, _ := c.queue.Get() + defer c.queue.Done(key) + if key != wantKey { + t.Errorf("queue got key %q, want %q", key, wantKey) + } + } else { + if c.queue.Len() != 0 { + t.Errorf("expected empty queue but got %d item(s)", c.queue.Len()) + } + } + }) + } +}