From 74b258c3bef5fa36387ef314b594065991444a3a Mon Sep 17 00:00:00 2001 From: irbekrm Date: Tue, 31 Jan 2023 12:27:42 +0000 Subject: [PATCH] Code review feedback Signed-off-by: irbekrm --- pkg/controller/cainjector/indexers.go | 37 ++++++++------------------- pkg/controller/cainjector/setup.go | 15 ++++++++++- pkg/controller/cainjector/sources.go | 13 +++++++--- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/pkg/controller/cainjector/indexers.go b/pkg/controller/cainjector/indexers.go index 20317a769..879c57bcb 100644 --- a/pkg/controller/cainjector/indexers.go +++ b/pkg/controller/cainjector/indexers.go @@ -26,8 +26,6 @@ import ( "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" ) @@ -215,32 +213,19 @@ func injectableCAFromSecretIndexer(rawObj client.Object) []string { return []string{secretNameRaw} } -// isInjectable returns predicates that determine whether an object is a +// hasInjectableAnnotation returns predicates that determine whether an object is a // cainjector injectable by looking at whether it has one of the three // annotations used to mark injectables. -func isInjectable() predicate.Funcs { - f := func(o client.Object) bool { - annots := o.GetAnnotations() - if _, ok := annots[cmapi.WantInjectAPIServerCAAnnotation]; ok { - return true - } - if _, ok := annots[cmapi.WantInjectAnnotation]; ok { - return true - } - if _, ok := annots[cmapi.WantInjectFromSecretAnnotation]; ok { - return true - } - return false +func hasInjectableAnnotation(o client.Object) bool { + annots := o.GetAnnotations() + if _, ok := annots[cmapi.WantInjectAPIServerCAAnnotation]; ok { + return true } - return predicate.Funcs{ - UpdateFunc: func(e event.UpdateEvent) bool { - return f(e.ObjectNew) - }, - CreateFunc: func(e event.CreateEvent) bool { - return f(e.Object) - }, - DeleteFunc: func(e event.DeleteEvent) bool { - return f(e.Object) - }, + if _, ok := annots[cmapi.WantInjectAnnotation]; ok { + return true } + if _, ok := annots[cmapi.WantInjectFromSecretAnnotation]; ok { + return true + } + return false } diff --git a/pkg/controller/cainjector/setup.go b/pkg/controller/cainjector/setup.go index 8b08f2d12..4193e0f4e 100644 --- a/pkg/controller/cainjector/setup.go +++ b/pkg/controller/cainjector/setup.go @@ -29,7 +29,9 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/source" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -121,6 +123,17 @@ func RegisterAllInjectors(ctx context.Context, mgr ctrl.Manager, namespace strin err := fmt.Errorf("error making injectable indexable by inject-ca-from-secret annotation: %w", err) return err } + predicates := predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + return hasInjectableAnnotation(e.ObjectNew) + }, + CreateFunc: func(e event.CreateEvent) bool { + return hasInjectableAnnotation(e.Object) + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return hasInjectableAnnotation(e.Object) + }, + } b := ctrl.NewControllerManagedBy(mgr). For(setup.objType, @@ -133,7 +146,7 @@ func RegisterAllInjectors(ctx context.Context, mgr ctrl.Manager, namespace strin // we can use the annotation to filter // injectables is here where we define which // objects' events should trigger a reconcile. - builder.WithPredicates(isInjectable())). + builder.WithPredicates(predicates)). Watches(&source.Kind{Type: new(corev1.Secret)}, handler.EnqueueRequestsFromMapFunc((&secretForInjectableMapper{ Client: mgr.GetClient(), log: log, diff --git a/pkg/controller/cainjector/sources.go b/pkg/controller/cainjector/sources.go index 8f4e1573d..94d727dcc 100644 --- a/pkg/controller/cainjector/sources.go +++ b/pkg/controller/cainjector/sources.go @@ -18,6 +18,7 @@ package cainjector import ( "context" + "errors" "fmt" "github.com/go-logr/logr" @@ -87,7 +88,8 @@ func (c *certificateDataSource) ReadCA(ctx context.Context, log logr.Logger, met certName := splitNamespacedName(certNameRaw) log = log.WithValues("certificate", certName) if certName.Namespace == "" { - log.Error(nil, "invalid certificate name: needs a namespace/ prefix") + err := errors.New("invalid annotation") + log.Error(err, "invalid certificate name: needs a namespace/ prefix") // TODO: should an error be returned here to prevent the caller from proceeding? // don't return an error, requeuing won't help till this is changed return nil, nil @@ -124,7 +126,8 @@ func (c *certificateDataSource) ReadCA(ctx context.Context, log logr.Logger, met // inject the CA data caData, hasCAData := secret.Data[cmmeta.TLSCAKey] if !hasCAData { - log.Error(nil, "certificate has no CA data") + err := errors.New("invalid CA source") + log.Error(err, "certificate has no CA data") // don't requeue, we'll get called when the secret gets updated return nil, nil } @@ -153,7 +156,8 @@ func (c *secretDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj secretName := splitNamespacedName(secretNameRaw) log = log.WithValues("secret", secretName) if secretName.Namespace == "" { - log.Error(nil, "invalid secret source: missing namespace/ prefix") + err := errors.New("invalid annotation") + log.Error(err, "invalid secret source: missing namespace/ prefix") // TODO: should we return error here to prevent the caller from proceeding? // don't return an error, requeuing won't help till this is changed return nil, nil @@ -182,7 +186,8 @@ func (c *secretDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj // inject the CA data caData, hasCAData := secret.Data[cmmeta.TLSCAKey] if !hasCAData { - log.Error(nil, "certificate has no CA data") + err := errors.New("invalid CA source") + log.Error(err, "secret contains no CA data") // don't requeue, we'll get called when the secret gets updated return nil, nil }