diff --git a/pkg/route/secretmanager/manager.go b/pkg/route/secretmanager/manager.go index a4a80ee591..3cc96a4fde 100644 --- a/pkg/route/secretmanager/manager.go +++ b/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) diff --git a/pkg/route/secretmanager/manager_test.go b/pkg/route/secretmanager/manager_test.go index 0f5f58c629..6749abc94f 100644 --- a/pkg/route/secretmanager/manager_test.go +++ b/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) diff --git a/pkg/secret/fake/fake_secret_monitor.go b/pkg/secret/fake/fake_secret_monitor.go index 563d73253d..63a8654b43 100644 --- a/pkg/secret/fake/fake_secret_monitor.go +++ b/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 diff --git a/pkg/secret/secret_monitor.go b/pkg/secret/secret_monitor.go index ddccb0484f..7c66899d57 100644 --- a/pkg/secret/secret_monitor.go +++ b/pkg/secret/secret_monitor.go @@ -93,10 +93,12 @@ 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") } @@ -104,24 +106,34 @@ func (s *secretMonitor) addSecretEventHandler(ctx context.Context, namespace, se // 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") } diff --git a/pkg/secret/secret_monitor_test.go b/pkg/secret/secret_monitor_test.go index 7120923508..8936680314 100644 --- a/pkg/secret/secret_monitor_test.go +++ b/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) }