OCPBUGS-77056: Release lock before WaitForCacheSync in secret monitor

pkg/route/secretmanager/manager.go

@@ -61,7 +61,6 @@ func (m *manager) Queue() workqueue.RateLimitingInterface {
 // Returns an error if the route is already registered with a secret or if adding the secret event handler fails.
 func (m *manager) RegisterRoute(ctx context.Context, namespace, routeName, secretName string, handler cache.ResourceEventHandlerFuncs) error {
 	m.handlersLock.Lock()
-	defer m.handlersLock.Unlock()
 
 	// Generate a unique key for the provided namespace and routeName.
 	key := generateKey(namespace, routeName)
@@ -70,22 +69,44 @@ func (m *manager) RegisterRoute(ctx context.Context, namespace, routeName, secre
 	// Each route (namespace/routeName) should be registered only once with any secret.
 	// Note: inside a namespace multiple different routes can be registered(watch) with a common secret.
 	if _, exists := m.registeredHandlers[key]; exists {
+		m.handlersLock.Unlock()
 		return fmt.Errorf("route already registered with key %s", key)
 	}
 
+	// Because adding the secret event handler can take O(latency) time when it starts an informer
+	// and waits for cache sync, we temporarily release the handlersLock. This permits concurrent
+	// registrations of other routes.
+	// We mark this key as tentatively registered using a nil registration
+	// so that concurrent attempts to register the same route fail immediately.
+	m.registeredHandlers[key] = referencedSecret{
+		secretName:          secretName,
+		handlerRegistration: nil, // placeholder while syncing
+	}
+	m.handlersLock.Unlock()
+
 	// Add a secret event handler for the specified namespace and secret, with the handler functions.
+	// This call releases the monitor lock internally during WaitForCacheSync, allowing concurrent
+	// registrations for different secrets. However, it blocks until the cache is synced so that
+	// GetSecret works immediately after RegisterRoute returns.
 	klog.V(5).Infof("trying to add handler for key %s with secret %s", key, secretName)
+
 	handlerReg, err := m.monitor.AddSecretEventHandler(ctx, namespace, secretName, handler)
+
+	m.handlersLock.Lock()
+
 	if err != nil {
-		return err
+		delete(m.registeredHandlers, key)
+		m.handlersLock.Unlock()
+		return fmt.Errorf("failed to add secret event handler for key %s: %w", key, err)
 	}
 
-	// Store the registration and secretName in the manager's map. Used during UnregisterRoute() and GetSecret().
-	m.registeredHandlers[key] = referencedSecret{
-		secretName:          secretName,
-		handlerRegistration: handlerReg,
+	// Update only if it wasn't unregistered while we were syncing
+	if ref, exists := m.registeredHandlers[key]; exists && ref.secretName == secretName && ref.handlerRegistration == nil {
+		ref.handlerRegistration = handlerReg
+		m.registeredHandlers[key] = ref
+		klog.Infof("secret manager registered route for key %s with secret %s", key, secretName)
 	}
-	klog.Infof("secret manager registered route for key %s with secret %s", key, secretName)
+	m.handlersLock.Unlock()
 
 	return nil
 }
@@ -104,6 +125,10 @@ func (m *manager) UnregisterRoute(namespace, routeName string) error {
 		return fmt.Errorf("no handler registered with key %s", key)
 	}
 
+	if ref.handlerRegistration == nil {
+		return fmt.Errorf("route registration currently in progress for key %s", key)
+	}
+
 	// Remove the corresponding secret event handler from the secret monitor.
 	klog.V(5).Info("trying to remove handler with key", key)
 	err := m.monitor.RemoveSecretEventHandler(ref.handlerRegistration)

pkg/route/secretmanager/manager_test.go

@@ -5,10 +5,12 @@ import (
 	"fmt"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/openshift/library-go/pkg/secret/fake"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/tools/cache"
 )
 
@@ -17,6 +19,20 @@ type routeSecret struct {
 	secretName string
 }
 
+func waitForRegistration(mgr *manager, key string) {
+	err := wait.PollImmediate(5*time.Millisecond, 2*time.Second, func() (bool, error) {
+		mgr.handlersLock.Lock()
+		defer mgr.handlersLock.Unlock()
+		if ref, exists := mgr.registeredHandlers[key]; !exists || ref.handlerRegistration != nil {
+			return true, nil
+		}
+		return false, nil
+	})
+	if err != nil {
+		panic(fmt.Sprintf("timeout waiting for registration of %s", key))
+	}
+}
+
 func TestRegisterRoute(t *testing.T) {
 	namespace := "ns"
 
@@ -96,9 +112,14 @@ func TestRegisterRoute(t *testing.T) {
 					gotErr += 1
 				}
 			}
+			for _, rs := range s.rs {
+				waitForRegistration(mgr, generateKey(namespace, rs.routeName))
+			}
 			if gotErr != s.expectErr {
 				t.Fatalf("expected %d errors, got %d errors", s.expectErr, gotErr)
 			}
+			mgr.handlersLock.Lock()
+			defer mgr.handlersLock.Unlock()
 			if len(s.expectHandlersKeys) != len(mgr.registeredHandlers) {
 				t.Fatalf("expected %d keys: %v, got %d keys: %v", len(s.expectHandlersKeys), s.expectHandlersKeys, len(mgr.registeredHandlers), mgr.registeredHandlers)
 			}
@@ -169,6 +190,9 @@ func TestUnregisterRoute(t *testing.T) {
 					t.Fatalf("failed to register %v: %v", rs, err)
 				}
 			}
+			for _, rs := range s.register {
+				waitForRegistration(mgr, generateKey(namespace, rs.routeName))
+			}
 
 			// unregister
 			mgr.monitor = &s.sm
@@ -182,6 +206,8 @@ func TestUnregisterRoute(t *testing.T) {
 			if gotErr != s.expectErr {
 				t.Fatalf("expected %d errors, got %d errors", s.expectErr, gotErr)
 			}
+			mgr.handlersLock.Lock()
+			defer mgr.handlersLock.Unlock()
 			if len(s.expectHandlersKeys) != len(mgr.registeredHandlers) {
 				t.Fatalf("expected %d keys: %v, got %d keys: %v", len(s.expectHandlersKeys), s.expectHandlersKeys, len(mgr.registeredHandlers), mgr.registeredHandlers)
 			}
@@ -245,6 +271,9 @@ func TestGetSecret(t *testing.T) {
 					t.Fatalf("failed to register %v: %v", rs, err)
 				}
 			}
+			for _, rs := range s.register {
+				waitForRegistration(&mgr, generateKey(namespace, rs.routeName))
+			}
 
 			mgr.monitor = &s.sm
 			gotSec, err := mgr.GetSecret(context.TODO(), namespace, routeName)
@@ -295,6 +324,9 @@ func TestLookupRouteSecret(t *testing.T) {
 					t.Fatalf("failed to register %v: %v", rs, err)
 				}
 			}
+			for _, rs := range s.register {
+				waitForRegistration(mgr, generateKey(namespace, rs.routeName))
+			}
 
 			secret, exist := mgr.LookupRouteSecret(namespace, routeName)
 

pkg/secret/fake/fake_secret_monitor.go

@@ -13,8 +13,17 @@ type SecretMonitor struct {
 	Secret *corev1.Secret
 }
 
+type fakeRegistration struct{}
+
+func (fakeRegistration) HasSynced() bool                                    { return true }
+func (fakeRegistration) GetKey() secret.ObjectKey                           { return secret.ObjectKey{} }
+func (fakeRegistration) GetHandler() cache.ResourceEventHandlerRegistration { return nil }
+
 func (sm *SecretMonitor) AddSecretEventHandler(_ context.Context, _ string, _ string, _ cache.ResourceEventHandler) (secret.SecretEventHandlerRegistration, error) {
-	return nil, sm.Err
+	if sm.Err != nil {
+		return nil, sm.Err
+	}
+	return fakeRegistration{}, nil
 }
 func (sm *SecretMonitor) RemoveSecretEventHandler(_ secret.SecretEventHandlerRegistration) error {
 	return sm.Err

pkg/secret/secret_monitor.go

@@ -93,35 +93,47 @@ func (s *secretMonitor) createSecretInformer(namespace, name string) cache.Share
 }
 
 // addSecretEventHandler adds a secret event handler and starts the informer if not already running.
+//
+// The global write lock is released before waiting for the informer cache to sync, allowing
+// concurrent calls for different secrets to proceed in parallel. This is critical for performance
+// when many routes reference external certificate secrets, as registering N secrets serially
+// (each requiring an API server round-trip to etcd) would take O(N * api_latency) time.
 func (s *secretMonitor) addSecretEventHandler(ctx context.Context, namespace, secretName string, handler cache.ResourceEventHandler, secretInformer cache.SharedInformer) (SecretEventHandlerRegistration, error) {
-	s.lock.Lock()
-	defer s.lock.Unlock()
-
 	if handler == nil {
 		return nil, fmt.Errorf("nil handler is provided")
 	}
 
 	// secret identifier (namespace/secret)
 	key := NewObjectKey(namespace, secretName)
 
+	s.lock.Lock()
 	// Start secret informer if monitor does not exist.
 	m, exists := s.monitors[key]
 	if !exists {
 		m = &monitoredItem{}
 		m.itemMonitor = newSingleItemMonitor(key, secretInformer)
 		m.itemMonitor.StartInformer(ctx)
-
-		// wait for first sync
-		if !cache.WaitForCacheSync(ctx.Done(), m.itemMonitor.HasSynced) {
-			return nil, fmt.Errorf("failed waiting for cache sync")
-		}
-
-		// add item key to monitors map
+		// Register the monitor in the map immediately (before releasing the lock) so that
+		// concurrent registrations for the same secret reuse this informer rather than
+		// starting a duplicate.
 		s.monitors[key] = m
+	}
+	s.lock.Unlock()
 
+	if !exists {
 		klog.Info("secret informer started", " item key ", key)
 	}
 
+	// Wait for the informer cache to sync before adding event handlers.
+	// This ensures GetSecret can retrieve secrets immediately after registration.
+	// The global lock is released above so other secrets can register concurrently.
+	if !cache.WaitForCacheSync(ctx.Done(), m.itemMonitor.HasSynced) {
+		return nil, fmt.Errorf("failed waiting for cache sync")
+	}
+
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
 	// add the event handler
 	registration, err := m.itemMonitor.AddEventHandler(handler)
 	if err != nil {
@@ -194,7 +206,7 @@ func (s *secretMonitor) GetSecret(ctx context.Context, handlerRegistration Secre
 		return nil, fmt.Errorf("secret monitor doesn't exist for key %v", key)
 	}
 
-	// wait for informer store sync, to load secrets
+	// Wait for informer store sync to load secrets.
 	if !cache.WaitForCacheSync(ctx.Done(), handlerRegistration.HasSynced) {
 		return nil, fmt.Errorf("failed waiting for cache sync")
 	}

pkg/secret/secret_monitor_test.go

@@ -262,6 +262,9 @@ func TestGetSecret(t *testing.T) {
 			if s.isNilHandlerReg {
 				h = nil
 			}
+			if h != nil {
+				cache.WaitForCacheSync(context.TODO().Done(), h.HasSynced)
+			}
 			if s.isKeyRemoved {
 				delete(sm.monitors, key)
 			}