OCPBUGS-100179: fix forward controller FilterFunc and finalizer removal

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

pkg/internalregistry/controllers/legacy_image_pull_secret_controller_test.go

@@ -0,0 +1,215 @@
+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/informers"
+	"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)
+			}
+		})
+	}
+}
+
+// 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())
+				}
+			}
+		})
+	}
+}