From 418f4d20ae86a25bd1af3dbe8ce36123994075e1 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Fri, 2 Oct 2020 13:22:18 -0400 Subject: [PATCH] Use parent func to indicate when the controller queue is a singleton This prevents unnecessary sync loop runs when the controller is running with a single worker. When the controller is running with more than one worker, it prevents subtle bugs that can cause the controller to go "back in time." Signed-off-by: Monis Khan Signed-off-by: Matt Moyer --- .../webhookcachecleaner.go | 2 +- .../webhookcachefiller/webhookcachefiller.go | 2 +- .../controller/kubecertagent/annotater.go | 4 ++-- internal/controller/kubecertagent/creater.go | 4 ++-- internal/controller/kubecertagent/deleter.go | 4 ++-- internal/controller/kubecertagent/execer.go | 2 +- .../supervisorconfig/jwks_observer.go | 4 ++-- .../supervisorconfig/jwks_writer.go | 2 +- .../oidcproviderconfig_watcher.go | 2 +- .../supervisorconfig/tls_cert_observer.go | 4 ++-- internal/controller/utils.go | 23 +++++++++++++++---- 11 files changed, 34 insertions(+), 19 deletions(-) diff --git a/internal/controller/authenticator/webhookcachecleaner/webhookcachecleaner.go b/internal/controller/authenticator/webhookcachecleaner/webhookcachecleaner.go index 5529af8c..b8d28acf 100644 --- a/internal/controller/authenticator/webhookcachecleaner/webhookcachecleaner.go +++ b/internal/controller/authenticator/webhookcachecleaner/webhookcachecleaner.go @@ -31,7 +31,7 @@ func New(cache *authncache.Cache, webhooks authinformers.WebhookAuthenticatorInf }, controllerlib.WithInformer( webhooks, - pinnipedcontroller.MatchAnythingFilter(), + pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()), controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index f3b5ec50..9dcf0447 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -40,7 +40,7 @@ func New(cache *authncache.Cache, webhooks authinformers.WebhookAuthenticatorInf }, controllerlib.WithInformer( webhooks, - pinnipedcontroller.MatchAnythingFilter(), + pinnipedcontroller.MatchAnythingFilter(nil), // nil parent func is fine because each event is distinct controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/kubecertagent/annotater.go b/internal/controller/kubecertagent/annotater.go index 79219846..eb951d63 100644 --- a/internal/controller/kubecertagent/annotater.go +++ b/internal/controller/kubecertagent/annotater.go @@ -71,12 +71,12 @@ func NewAnnotaterController( }, withInformer( kubeSystemPodInformer, - pinnipedcontroller.SimpleFilter(isControllerManagerPod), + pinnipedcontroller.SimpleFilterWithSingletonQueue(isControllerManagerPod), controllerlib.InformerOption{}, ), withInformer( agentPodInformer, - pinnipedcontroller.SimpleFilter(isAgentPod), + pinnipedcontroller.SimpleFilterWithSingletonQueue(isAgentPod), controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/kubecertagent/creater.go b/internal/controller/kubecertagent/creater.go index 03d2043f..c917a39e 100644 --- a/internal/controller/kubecertagent/creater.go +++ b/internal/controller/kubecertagent/creater.go @@ -65,12 +65,12 @@ func NewCreaterController( }, withInformer( kubeSystemPodInformer, - pinnipedcontroller.SimpleFilter(isControllerManagerPod), + pinnipedcontroller.SimpleFilterWithSingletonQueue(isControllerManagerPod), controllerlib.InformerOption{}, ), withInformer( agentPodInformer, - pinnipedcontroller.SimpleFilter(isAgentPod), + pinnipedcontroller.SimpleFilterWithSingletonQueue(isAgentPod), controllerlib.InformerOption{}, ), // Be sure to run once even to make sure the CI is updated if there are no controller manager diff --git a/internal/controller/kubecertagent/deleter.go b/internal/controller/kubecertagent/deleter.go index 9aa120f1..3953449f 100644 --- a/internal/controller/kubecertagent/deleter.go +++ b/internal/controller/kubecertagent/deleter.go @@ -43,12 +43,12 @@ func NewDeleterController( }, withInformer( kubeSystemPodInformer, - pinnipedcontroller.SimpleFilter(isControllerManagerPod), + pinnipedcontroller.SimpleFilterWithSingletonQueue(isControllerManagerPod), controllerlib.InformerOption{}, ), withInformer( agentPodInformer, - pinnipedcontroller.SimpleFilter(isAgentPod), + pinnipedcontroller.SimpleFilterWithSingletonQueue(isAgentPod), controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/kubecertagent/execer.go b/internal/controller/kubecertagent/execer.go index ec50e49f..a482051e 100644 --- a/internal/controller/kubecertagent/execer.go +++ b/internal/controller/kubecertagent/execer.go @@ -56,7 +56,7 @@ func NewExecerController( }, withInformer( agentPodInformer, - pinnipedcontroller.SimpleFilter(isAgentPod), + pinnipedcontroller.SimpleFilter(isAgentPod, nil), // nil parent func is fine because each event is distinct controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/supervisorconfig/jwks_observer.go b/internal/controller/supervisorconfig/jwks_observer.go index 1ea4c0fc..9f89e0c3 100644 --- a/internal/controller/supervisorconfig/jwks_observer.go +++ b/internal/controller/supervisorconfig/jwks_observer.go @@ -49,12 +49,12 @@ func NewJWKSObserverController( }, withInformer( secretInformer, - pinnipedcontroller.MatchAnythingFilter(), + pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()), controllerlib.InformerOption{}, ), withInformer( oidcProviderInformer, - pinnipedcontroller.MatchAnythingFilter(), + pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()), controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/supervisorconfig/jwks_writer.go b/internal/controller/supervisorconfig/jwks_writer.go index 8af5fb97..16833a0a 100644 --- a/internal/controller/supervisorconfig/jwks_writer.go +++ b/internal/controller/supervisorconfig/jwks_writer.go @@ -110,7 +110,7 @@ func NewJWKSWriterController( // We want to be notified when anything happens to an OPC. withInformer( opcInformer, - pinnipedcontroller.MatchAnythingFilter(), + pinnipedcontroller.MatchAnythingFilter(nil), // nil parent func is fine because each event is distinct controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go index 99b55894..77e6b12b 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go @@ -60,7 +60,7 @@ func NewOIDCProviderWatcherController( }, withInformer( opcInformer, - pinnipedcontroller.MatchAnythingFilter(), + pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()), controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/supervisorconfig/tls_cert_observer.go b/internal/controller/supervisorconfig/tls_cert_observer.go index 7b604f57..cb42275c 100644 --- a/internal/controller/supervisorconfig/tls_cert_observer.go +++ b/internal/controller/supervisorconfig/tls_cert_observer.go @@ -49,12 +49,12 @@ func NewTLSCertObserverController( }, withInformer( secretInformer, - pinnipedcontroller.MatchAnythingFilter(), + pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()), controllerlib.InformerOption{}, ), withInformer( oidcProviderInformer, - pinnipedcontroller.MatchAnythingFilter(), + pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()), controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 8a6cd4fb..354b6a3d 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -12,23 +12,38 @@ import ( func NameAndNamespaceExactMatchFilterFactory(name, namespace string) controllerlib.Filter { return SimpleFilter(func(obj metav1.Object) bool { return obj.GetName() == name && obj.GetNamespace() == namespace - }) + // nil parent func is fine because we only match a key with the given name and namespace + // i.e. it is equivalent to having a SingletonQueue() parent func + }, nil) } // MatchAnythingFilter returns a controllerlib.Filter that allows all objects. -func MatchAnythingFilter() controllerlib.Filter { - return SimpleFilter(func(object metav1.Object) bool { return true }) +func MatchAnythingFilter(parentFunc controllerlib.ParentFunc) controllerlib.Filter { + return SimpleFilter(func(object metav1.Object) bool { return true }, parentFunc) } // SimpleFilter takes a single boolean match function on a metav1.Object and wraps it into a proper controllerlib.Filter. -func SimpleFilter(match func(metav1.Object) bool) controllerlib.Filter { +func SimpleFilter(match func(metav1.Object) bool, parentFunc controllerlib.ParentFunc) controllerlib.Filter { return controllerlib.FilterFuncs{ AddFunc: match, UpdateFunc: func(oldObj, newObj metav1.Object) bool { return match(oldObj) || match(newObj) }, DeleteFunc: match, + ParentFunc: parentFunc, } } +// SingletonQueue returns a parent func that treats all events as the same key. +func SingletonQueue() controllerlib.ParentFunc { + return func(_ metav1.Object) controllerlib.Key { + return controllerlib.Key{} + } +} + +// SimpleFilterWithSingletonQueue returns a Filter based on the given match function that treats all events as the same key. +func SimpleFilterWithSingletonQueue(match func(metav1.Object) bool) controllerlib.Filter { + return SimpleFilter(match, SingletonQueue()) +} + // Same signature as controllerlib.WithInformer(). type WithInformerOptionFunc func( getter controllerlib.InformerGetter,