From 75b9bd6598c3537fb52064fb064481c9f90b260b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 28 Jun 2021 19:17:11 +0200 Subject: [PATCH 1/8] ingress-shim: untangle logic for "looking for cert owners" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais --- pkg/controller/ingress-shim/BUILD.bazel | 1 - pkg/controller/ingress-shim/checks.go | 48 --------- pkg/controller/ingress-shim/controller.go | 123 ++++++++++++---------- pkg/controller/ingress-shim/sync.go | 2 +- pkg/controller/ingress-shim/sync_test.go | 2 +- 5 files changed, 68 insertions(+), 108 deletions(-) delete mode 100644 pkg/controller/ingress-shim/checks.go diff --git a/pkg/controller/ingress-shim/BUILD.bazel b/pkg/controller/ingress-shim/BUILD.bazel index 496cb84a3..45ebd300b 100644 --- a/pkg/controller/ingress-shim/BUILD.bazel +++ b/pkg/controller/ingress-shim/BUILD.bazel @@ -3,7 +3,6 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = [ - "checks.go", "controller.go", "helper.go", "sync.go", diff --git a/pkg/controller/ingress-shim/checks.go b/pkg/controller/ingress-shim/checks.go deleted file mode 100644 index 96f605b31..000000000 --- a/pkg/controller/ingress-shim/checks.go +++ /dev/null @@ -1,48 +0,0 @@ -/* -Copyright 2020 The cert-manager Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package controller - -import ( - "fmt" - - networkingv1beta1 "k8s.io/api/networking/v1beta1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - - v1 "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" -) - -func (c *controller) ingressesForCertificate(crt *v1.Certificate) ([]*networkingv1beta1.Ingress, error) { - ings, err := c.ingressLister.List(labels.NewSelector()) - - if err != nil { - return nil, fmt.Errorf("error listing certificates: %s", err.Error()) - } - - var affected []*networkingv1beta1.Ingress - for _, ing := range ings { - if crt.Namespace != ing.Namespace { - continue - } - - if metav1.IsControlledBy(crt, ing) { - affected = append(affected, ing) - } - } - - return affected, nil -} diff --git a/pkg/controller/ingress-shim/controller.go b/pkg/controller/ingress-shim/controller.go index a9b84051a..a3d6549f7 100644 --- a/pkg/controller/ingress-shim/controller.go +++ b/pkg/controller/ingress-shim/controller.go @@ -22,6 +22,7 @@ import ( "github.com/go-logr/logr" k8sErrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes" networkinglisters "k8s.io/client-go/listers/networking/v1beta1" @@ -38,7 +39,6 @@ import ( ) const ( - // ControllerName is the name of the ingress-shim controller. ControllerName = "ingress-shim" ) @@ -48,16 +48,11 @@ type defaults struct { } type controller struct { - // maintain a reference to the workqueue for this controller - // so the handleOwnedResource method can enqueue resources - queue workqueue.RateLimitingInterface - - // logger to be used by this controller - log logr.Logger - kClient kubernetes.Interface cmClient clientset.Interface + recorder record.EventRecorder + log logr.Logger ingressLister networkinglisters.IngressLister certificateLister cmlisters.CertificateLister @@ -72,41 +67,35 @@ type controller struct { // It returns the workqueue to be used to enqueue items, a list of // InformerSynced functions that must be synced, or an error. func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitingInterface, []cache.InformerSynced, error) { - // construct a new named logger to be reused throughout the controller + kShared := ctx.KubeSharedInformerFactory + cmShared := ctx.SharedInformerFactory + c.log = logf.FromContext(ctx.RootContext, ControllerName) + queue := workqueue.NewNamedRateLimitingQueue(controllerpkg.DefaultItemBasedRateLimiter(), ControllerName) - // create a queue used to queue up items to be processed - c.queue = workqueue.NewNamedRateLimitingQueue(controllerpkg.DefaultItemBasedRateLimiter(), ControllerName) - - // obtain references to all the informers used by this controller - ingressInformer := ctx.KubeSharedInformerFactory.Networking().V1beta1().Ingresses() - certificatesInformer := ctx.SharedInformerFactory.Certmanager().V1().Certificates() - issuerInformer := ctx.SharedInformerFactory.Certmanager().V1().Issuers() - // build a list of InformerSynced functions that will be returned by the Register method. - // the controller will only begin processing items once all of these informers have synced. mustSync := []cache.InformerSynced{ - ingressInformer.Informer().HasSynced, - certificatesInformer.Informer().HasSynced, - issuerInformer.Informer().HasSynced, + cmShared.Certmanager().V1().Certificates().Informer().HasSynced, + cmShared.Certmanager().V1().Certificates().Informer().HasSynced, + cmShared.Certmanager().V1().Issuers().Informer().HasSynced, } - // set all the references to the listers for used by the Sync function - c.ingressLister = ingressInformer.Lister() - c.certificateLister = certificatesInformer.Lister() - c.issuerLister = issuerInformer.Lister() + c.ingressLister = kShared.Networking().V1beta1().Ingresses().Lister() + c.certificateLister = cmShared.Certmanager().V1().Certificates().Lister() + c.issuerLister = cmShared.Certmanager().V1().Issuers().Lister() - // if scoped to a single namespace - // if we are running in non-namespaced mode (i.e. --namespace=""), we also - // register event handlers and obtain a lister for clusterissuers. + // We don't need to run the ClusterIssuer controller when cert-manager is + // running in non-namespaced mode (i.e. --namespace=""). if ctx.Namespace == "" { - clusterIssuerInformer := ctx.SharedInformerFactory.Certmanager().V1().ClusterIssuers() - mustSync = append(mustSync, clusterIssuerInformer.Informer().HasSynced) - c.clusterIssuerLister = clusterIssuerInformer.Lister() + mustSync = append(mustSync, cmShared.Certmanager().V1().ClusterIssuers().Informer().HasSynced) + c.clusterIssuerLister = cmShared.Certmanager().V1().ClusterIssuers().Lister() } - // register handler functions - ingressInformer.Informer().AddEventHandler(&controllerpkg.QueuingEventHandler{Queue: c.queue}) - certificatesInformer.Informer().AddEventHandler(&controllerpkg.BlockingEventHandler{WorkFunc: c.certificateDeleted}) + kShared.Networking().V1beta1().Ingresses().Informer().AddEventHandler(&controllerpkg.QueuingEventHandler{ + Queue: queue, + }) + cmShared.Certmanager().V1().Certificates().Informer().AddEventHandler(&controllerpkg.BlockingEventHandler{ + WorkFunc: certificateDeleted(queue), + }) c.helper = issuer.NewHelper(c.issuerLister, c.clusterIssuerLister) c.kClient = ctx.Client @@ -119,28 +108,7 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin ctx.DefaultIssuerGroup, } - return c.queue, mustSync, nil -} - -func (c *controller) certificateDeleted(obj interface{}) { - crt, ok := obj.(*cmapi.Certificate) - if !ok { - runtime.HandleError(fmt.Errorf("Object is not a certificate object %#v", obj)) - return - } - ings, err := c.ingressesForCertificate(crt) - if err != nil { - runtime.HandleError(fmt.Errorf("Error looking up ingress observing certificate: %s/%s", crt.Namespace, crt.Name)) - return - } - for _, ing := range ings { - key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(ing) - if err != nil { - runtime.HandleError(err) - continue - } - c.queue.Add(key) - } + return queue, mustSync, nil } func (c *controller) ProcessItem(ctx context.Context, key string) error { @@ -161,7 +129,48 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return err } - return c.Sync(ctx, crt) + return c.sync(ctx, crt) +} + +// Whenever a Certificate gets deleted, we want to reconcile its parent Ingress. +// This parent Ingress is called "controller object". For example, the following +// Certificate is controlled by the Ingress "example": +// +// kind: Certificate +// metadata: +// namespace: cert-that-was-deleted +// ownerReferences: +// - controller: true ← this +// apiVersion: networking.k8s.io/v1beta1 +// kind: Ingress +// name: example +// blockOwnerDeletion: true +// uid: 7d3897c2-ce27-4144-883a-e1b5f89bd65a +func certificateDeleted(queue workqueue.RateLimitingInterface) func(obj interface{}) { + return func(obj interface{}) { + crt, ok := obj.(*cmapi.Certificate) + if !ok { + runtime.HandleError(fmt.Errorf("not a Certificate object: %#v", obj)) + return + } + + ref := metav1.GetControllerOf(crt) + if ref == nil { + // No controller should care about orphans being deleted or + // updated. + return + } + + // We don't check the apiVersion e.g. "networking.k8s.io/v1beta1" + // because there is no chance that another object called "Ingress" be + // the controller of a Certificate. + if ref.Kind != "Ingress" { + return + } + + // Queue items are simple strings of the form "namespace-1/ingress-1". + queue.Add(crt.Namespace + "/" + ref.Name) + } } func init() { diff --git a/pkg/controller/ingress-shim/sync.go b/pkg/controller/ingress-shim/sync.go index a3b18da94..c4d435f73 100644 --- a/pkg/controller/ingress-shim/sync.go +++ b/pkg/controller/ingress-shim/sync.go @@ -46,7 +46,7 @@ const ( var ingressGVK = networkingv1beta1.SchemeGroupVersion.WithKind("Ingress") -func (c *controller) Sync(ctx context.Context, ing *networkingv1beta1.Ingress) error { +func (c *controller) sync(ctx context.Context, ing *networkingv1beta1.Ingress) error { log := logf.WithResource(logf.FromContext(ctx), ing) ctx = logf.NewContext(ctx, log) diff --git a/pkg/controller/ingress-shim/sync_test.go b/pkg/controller/ingress-shim/sync_test.go index 71e59b3a1..2e0a12b66 100644 --- a/pkg/controller/ingress-shim/sync_test.go +++ b/pkg/controller/ingress-shim/sync_test.go @@ -1165,7 +1165,7 @@ func TestSync(t *testing.T) { } b.Start() - err := c.Sync(context.Background(), test.Ingress) + err := c.sync(context.Background(), test.Ingress) // If test.Err == true, err should not be nil and vice versa if test.Err == (err == nil) { From 0b12a5cf5f2f35b7c47f5e73c57c2ebb50d3256a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 9 Jul 2021 17:38:40 +0200 Subject: [PATCH 2/8] ingress-shim: explain why the owner ref does not have a namespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais --- pkg/controller/ingress-shim/controller.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/controller/ingress-shim/controller.go b/pkg/controller/ingress-shim/controller.go index a3d6549f7..5681ae666 100644 --- a/pkg/controller/ingress-shim/controller.go +++ b/pkg/controller/ingress-shim/controller.go @@ -148,14 +148,14 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { // uid: 7d3897c2-ce27-4144-883a-e1b5f89bd65a func certificateDeleted(queue workqueue.RateLimitingInterface) func(obj interface{}) { return func(obj interface{}) { - crt, ok := obj.(*cmapi.Certificate) + cert, ok := obj.(*cmapi.Certificate) if !ok { runtime.HandleError(fmt.Errorf("not a Certificate object: %#v", obj)) return } - ref := metav1.GetControllerOf(crt) - if ref == nil { + ingress := metav1.GetControllerOf(cert) + if ingress == nil { // No controller should care about orphans being deleted or // updated. return @@ -164,12 +164,17 @@ func certificateDeleted(queue workqueue.RateLimitingInterface) func(obj interfac // We don't check the apiVersion e.g. "networking.k8s.io/v1beta1" // because there is no chance that another object called "Ingress" be // the controller of a Certificate. - if ref.Kind != "Ingress" { + if ingress.Kind != "Ingress" { return } - // Queue items are simple strings of the form "namespace-1/ingress-1". - queue.Add(crt.Namespace + "/" + ref.Name) + // Owner references don't know about the namespace of the referenced + // object. That's because owner refs do not support cross-namespace + // references. We also know that the Certificate and its parent Ingress + // must both be on the same namespace. We thus use the Certificate's + // namespace to trigger a resync of the parent Ingress (the string below + // is a "key" of the form "namespace-1/my-ingress"). + queue.Add(cert.Namespace + "/" + ingress.Name) } } From 1cb39d1efeb703a6d2c5f5b0e5e5c4ceeed2463a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 9 Jul 2021 17:42:08 +0200 Subject: [PATCH 3/8] ingress-shim: remove duplicate line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais --- pkg/controller/ingress-shim/controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/controller/ingress-shim/controller.go b/pkg/controller/ingress-shim/controller.go index 5681ae666..2e3d4548d 100644 --- a/pkg/controller/ingress-shim/controller.go +++ b/pkg/controller/ingress-shim/controller.go @@ -74,7 +74,6 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin queue := workqueue.NewNamedRateLimitingQueue(controllerpkg.DefaultItemBasedRateLimiter(), ControllerName) mustSync := []cache.InformerSynced{ - cmShared.Certmanager().V1().Certificates().Informer().HasSynced, cmShared.Certmanager().V1().Certificates().Informer().HasSynced, cmShared.Certmanager().V1().Issuers().Informer().HasSynced, } From 30ad33784d354680c6094ae11a2e0bef546eb88f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 9 Jul 2021 18:22:23 +0200 Subject: [PATCH 4/8] ingress-shim: remove unecessary/verbose comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais --- pkg/controller/ingress-shim/controller.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pkg/controller/ingress-shim/controller.go b/pkg/controller/ingress-shim/controller.go index 2e3d4548d..b2077b0cb 100644 --- a/pkg/controller/ingress-shim/controller.go +++ b/pkg/controller/ingress-shim/controller.go @@ -145,6 +145,8 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { // name: example // blockOwnerDeletion: true // uid: 7d3897c2-ce27-4144-883a-e1b5f89bd65a +// +// Note that the owner reference doesn't know about the Ingress's namespace. func certificateDeleted(queue workqueue.RateLimitingInterface) func(obj interface{}) { return func(obj interface{}) { cert, ok := obj.(*cmapi.Certificate) @@ -167,12 +169,6 @@ func certificateDeleted(queue workqueue.RateLimitingInterface) func(obj interfac return } - // Owner references don't know about the namespace of the referenced - // object. That's because owner refs do not support cross-namespace - // references. We also know that the Certificate and its parent Ingress - // must both be on the same namespace. We thus use the Certificate's - // namespace to trigger a resync of the parent Ingress (the string below - // is a "key" of the form "namespace-1/my-ingress"). queue.Add(cert.Namespace + "/" + ingress.Name) } } From c119b64fdff6efe258968a4c0f0d8c99854f7407 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 9 Jul 2021 18:58:04 +0200 Subject: [PATCH 5/8] ingress-shim: I was syncing on Issuers instead of Ingresses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais --- pkg/controller/ingress-shim/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/ingress-shim/controller.go b/pkg/controller/ingress-shim/controller.go index b2077b0cb..04852db7d 100644 --- a/pkg/controller/ingress-shim/controller.go +++ b/pkg/controller/ingress-shim/controller.go @@ -75,7 +75,7 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin mustSync := []cache.InformerSynced{ cmShared.Certmanager().V1().Certificates().Informer().HasSynced, - cmShared.Certmanager().V1().Issuers().Informer().HasSynced, + kShared.Networking().V1beta1().Ingresses().Informer().HasSynced, } c.ingressLister = kShared.Networking().V1beta1().Ingresses().Lister() From 59051432e36bdae85401c75fb01c587c7f95c7e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 12 Jul 2021 12:38:36 +0200 Subject: [PATCH 6/8] ingress-shim: remove unused issuer and clusterissuer listers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais --- pkg/controller/ingress-shim/BUILD.bazel | 1 - pkg/controller/ingress-shim/controller.go | 19 +++---------------- pkg/controller/ingress-shim/sync_test.go | 23 ++++------------------- 3 files changed, 7 insertions(+), 36 deletions(-) diff --git a/pkg/controller/ingress-shim/BUILD.bazel b/pkg/controller/ingress-shim/BUILD.bazel index 45ebd300b..cce1efc48 100644 --- a/pkg/controller/ingress-shim/BUILD.bazel +++ b/pkg/controller/ingress-shim/BUILD.bazel @@ -17,7 +17,6 @@ go_library( "//pkg/client/clientset/versioned:go_default_library", "//pkg/client/listers/certmanager/v1:go_default_library", "//pkg/controller:go_default_library", - "//pkg/issuer:go_default_library", "//pkg/logs:go_default_library", "@com_github_go_logr_logr//:go_default_library", "@io_k8s_api//core/v1:go_default_library", diff --git a/pkg/controller/ingress-shim/controller.go b/pkg/controller/ingress-shim/controller.go index 04852db7d..14a479d48 100644 --- a/pkg/controller/ingress-shim/controller.go +++ b/pkg/controller/ingress-shim/controller.go @@ -34,7 +34,6 @@ import ( clientset "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" cmlisters "github.com/jetstack/cert-manager/pkg/client/listers/certmanager/v1" controllerpkg "github.com/jetstack/cert-manager/pkg/controller" - "github.com/jetstack/cert-manager/pkg/issuer" logf "github.com/jetstack/cert-manager/pkg/logs" ) @@ -54,12 +53,9 @@ type controller struct { recorder record.EventRecorder log logr.Logger - ingressLister networkinglisters.IngressLister - certificateLister cmlisters.CertificateLister - issuerLister cmlisters.IssuerLister - clusterIssuerLister cmlisters.ClusterIssuerLister + ingressLister networkinglisters.IngressLister + certificateLister cmlisters.CertificateLister - helper issuer.Helper defaults defaults } @@ -74,20 +70,12 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin queue := workqueue.NewNamedRateLimitingQueue(controllerpkg.DefaultItemBasedRateLimiter(), ControllerName) mustSync := []cache.InformerSynced{ - cmShared.Certmanager().V1().Certificates().Informer().HasSynced, kShared.Networking().V1beta1().Ingresses().Informer().HasSynced, + cmShared.Certmanager().V1().Certificates().Informer().HasSynced, } c.ingressLister = kShared.Networking().V1beta1().Ingresses().Lister() c.certificateLister = cmShared.Certmanager().V1().Certificates().Lister() - c.issuerLister = cmShared.Certmanager().V1().Issuers().Lister() - - // We don't need to run the ClusterIssuer controller when cert-manager is - // running in non-namespaced mode (i.e. --namespace=""). - if ctx.Namespace == "" { - mustSync = append(mustSync, cmShared.Certmanager().V1().ClusterIssuers().Informer().HasSynced) - c.clusterIssuerLister = cmShared.Certmanager().V1().ClusterIssuers().Lister() - } kShared.Networking().V1beta1().Ingresses().Informer().AddEventHandler(&controllerpkg.QueuingEventHandler{ Queue: queue, @@ -96,7 +84,6 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin WorkFunc: certificateDeleted(queue), }) - c.helper = issuer.NewHelper(c.issuerLister, c.clusterIssuerLister) c.kClient = ctx.Client c.cmClient = ctx.CMClient c.recorder = ctx.Recorder diff --git a/pkg/controller/ingress-shim/sync_test.go b/pkg/controller/ingress-shim/sync_test.go index 2e0a12b66..022998068 100644 --- a/pkg/controller/ingress-shim/sync_test.go +++ b/pkg/controller/ingress-shim/sync_test.go @@ -19,7 +19,6 @@ package controller import ( "context" "errors" - "fmt" "testing" networkingv1beta1 "k8s.io/api/networking/v1beta1" @@ -1149,19 +1148,16 @@ func TestSync(t *testing.T) { b.Init() defer b.Stop() c := &controller{ - kClient: b.Client, - cmClient: b.CMClient, - recorder: b.Recorder, - issuerLister: b.SharedInformerFactory.Certmanager().V1().Issuers().Lister(), - clusterIssuerLister: b.SharedInformerFactory.Certmanager().V1().ClusterIssuers().Lister(), - certificateLister: b.SharedInformerFactory.Certmanager().V1().Certificates().Lister(), + kClient: b.Client, + cmClient: b.CMClient, + recorder: b.Recorder, + certificateLister: b.SharedInformerFactory.Certmanager().V1().Certificates().Lister(), defaults: defaults{ issuerName: test.DefaultIssuerName, issuerKind: test.DefaultIssuerKind, issuerGroup: test.DefaultIssuerGroup, autoCertificateAnnotations: []string{testAcmeTLSAnnotation}, }, - helper: &fakeHelper{issuer: test.Issuer}, } b.Start() @@ -1188,17 +1184,6 @@ func TestSync(t *testing.T) { } } -type fakeHelper struct { - issuer cmapi.GenericIssuer -} - -func (f *fakeHelper) GetGenericIssuer(ref cmmeta.ObjectReference, ns string) (cmapi.GenericIssuer, error) { - if f.issuer == nil { - return nil, fmt.Errorf("no issuer specified on fake helper") - } - return f.issuer, nil -} - func TestIssuerForIngress(t *testing.T) { type testT struct { Ingress *networkingv1beta1.Ingress From e12173b4c2e78191629a9d3497c36526910e3a20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Mon, 12 Jul 2021 17:23:22 +0200 Subject: [PATCH 7/8] ingress-shim: unit-test certificateDeleted, only call on deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The func certificateDeleted was being called on every possible event (deleted, created, updated). Signed-off-by: Maël Valais --- pkg/controller/ingress-shim/BUILD.bazel | 4 + pkg/controller/ingress-shim/controller.go | 4 +- .../ingress-shim/controller_test.go | 163 ++++++++++++++++++ 3 files changed, 169 insertions(+), 2 deletions(-) create mode 100644 pkg/controller/ingress-shim/controller_test.go diff --git a/pkg/controller/ingress-shim/BUILD.bazel b/pkg/controller/ingress-shim/BUILD.bazel index cce1efc48..429e1f99e 100644 --- a/pkg/controller/ingress-shim/BUILD.bazel +++ b/pkg/controller/ingress-shim/BUILD.bazel @@ -37,6 +37,7 @@ go_library( go_test( name = "go_default_test", srcs = [ + "controller_test.go", "helper_test.go", "sync_test.go", ], @@ -45,13 +46,16 @@ go_test( "//pkg/apis/acme/v1:go_default_library", "//pkg/apis/certmanager/v1:go_default_library", "//pkg/apis/meta/v1:go_default_library", + "//pkg/client/clientset/versioned:go_default_library", "//pkg/controller/test:go_default_library", "//test/unit/gen:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", "@io_k8s_api//networking/v1beta1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_apimachinery//pkg/runtime:go_default_library", "@io_k8s_apimachinery//pkg/types:go_default_library", + "@io_k8s_client_go//kubernetes:go_default_library", "@io_k8s_client_go//testing:go_default_library", ], ) diff --git a/pkg/controller/ingress-shim/controller.go b/pkg/controller/ingress-shim/controller.go index 14a479d48..46dfaab69 100644 --- a/pkg/controller/ingress-shim/controller.go +++ b/pkg/controller/ingress-shim/controller.go @@ -80,8 +80,8 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin kShared.Networking().V1beta1().Ingresses().Informer().AddEventHandler(&controllerpkg.QueuingEventHandler{ Queue: queue, }) - cmShared.Certmanager().V1().Certificates().Informer().AddEventHandler(&controllerpkg.BlockingEventHandler{ - WorkFunc: certificateDeleted(queue), + cmShared.Certmanager().V1().Certificates().Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ + DeleteFunc: certificateDeleted(queue), }) c.kClient = ctx.Client diff --git a/pkg/controller/ingress-shim/controller_test.go b/pkg/controller/ingress-shim/controller_test.go new file mode 100644 index 000000000..a92edfe65 --- /dev/null +++ b/pkg/controller/ingress-shim/controller_test.go @@ -0,0 +1,163 @@ +/* +Copyright 2020 The cert-manager Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "testing" + "time" + + testpkg "github.com/jetstack/cert-manager/pkg/controller/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + networkingv1beta1 "k8s.io/api/networking/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + kclient "k8s.io/client-go/kubernetes" + + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + cmclient "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" +) + +var _ = cmapi.Certificate{} + +func Test_controller_Register(t *testing.T) { + tests := []struct { + name string + existingKObjects []runtime.Object + existingCMObjects []runtime.Object + givenCall func(*testing.T, cmclient.Interface, kclient.Interface) + expectRequeueKey string + }{ + { + name: "ingress should be queued when it is created", + givenCall: func(t *testing.T, _ cmclient.Interface, c kclient.Interface) { + _, err := c.NetworkingV1beta1().Ingresses("namespace-1").Create(context.Background(), &networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "ingress-1", + }}, metav1.CreateOptions{}) + require.NoError(t, err) + }, + expectRequeueKey: "namespace-1/ingress-1", + }, + { + name: "ingress should be queued when it is updated", + existingKObjects: []runtime.Object{&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "ingress-1", + }}}, + givenCall: func(t *testing.T, _ cmclient.Interface, c kclient.Interface) { + _, err := c.NetworkingV1beta1().Ingresses("namespace-1").Update(context.Background(), &networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "ingress-1", + }}, metav1.UpdateOptions{}) + require.NoError(t, err) + }, + expectRequeueKey: "namespace-1/ingress-1", + }, + { + name: "ingress should be queued when it is deleted", + existingKObjects: []runtime.Object{&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "ingress-1", + }}}, + givenCall: func(t *testing.T, _ cmclient.Interface, c kclient.Interface) { + err := c.NetworkingV1beta1().Ingresses("namespace-1").Delete(context.Background(), "ingress-1", metav1.DeleteOptions{}) + require.NoError(t, err) + }, + expectRequeueKey: "namespace-1/ingress-1", + }, + { + name: "ingress should not be queued when its child certificate is added", + givenCall: func(t *testing.T, c cmclient.Interface, _ kclient.Interface) { + _, err := c.CertmanagerV1().Certificates("namespace-1").Create(context.Background(), &cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "cert-1", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "ingress-2", + }}, ingressGVK)}, + }}, metav1.CreateOptions{}) + require.NoError(t, err) + }, + expectRequeueKey: "", + }, + { + name: "ingress should not be queued when its child certificate is updated", + existingCMObjects: []runtime.Object{&cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "cert-1", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "ingress-2", + }}, ingressGVK)}, + }}}, + givenCall: func(t *testing.T, c cmclient.Interface, _ kclient.Interface) { + _, err := c.CertmanagerV1().Certificates("namespace-1").Update(context.Background(), &cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "cert-1", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "ingress-2", + }}, ingressGVK)}, + }}, metav1.UpdateOptions{}) + require.NoError(t, err) + }, + expectRequeueKey: "", + }, + { + name: "ingress should be queued when its child certificate is deleted", + existingCMObjects: []runtime.Object{&cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "cert-1", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ + Namespace: "namespace-1", Name: "ingress-2", + }}, ingressGVK)}, + }}}, + givenCall: func(t *testing.T, c cmclient.Interface, _ kclient.Interface) { + err := c.CertmanagerV1().Certificates("namespace-1").Delete(context.Background(), "cert-1", metav1.DeleteOptions{}) + require.NoError(t, err) + }, + expectRequeueKey: "namespace-1/ingress-2", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + b := &testpkg.Builder{T: t, CertManagerObjects: test.existingCMObjects, KubeObjects: test.existingKObjects} + b.Init() + + // We don't care about the HasSynced functions since we already know + // whether they have been properly "used": if no Ingress or + // Certificate event is received then HasSynced has not been setup + // properly. + queue, _, err := (&controller{}).Register(b.Context) + require.NoError(t, err) + + b.Start() + defer b.Stop() + + test.givenCall(t, b.CMClient, b.Client) + + // We have no way of knowing when the informers will be done adding + // items to the queue due to the "shared informer" architecture: + // Start(stop) does not allow you to wait for the informers to be + // done. To work around that, we do a second queue.Get and expect it + // to be nil. + time.AfterFunc(50*time.Millisecond, queue.ShutDown) + gotKey, _ := queue.Get() + shouldBeNil, done := queue.Get() + assert.True(t, done) + assert.Nil(t, shouldBeNil) + assert.Equal(t, 0, queue.Len()) + if test.expectRequeueKey != "" { + assert.Equal(t, test.expectRequeueKey, gotKey) + } else { + assert.Nil(t, gotKey) + } + }) + } +} From b13b751d635a36f3ac780da3bb9fb55a6ead0424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 13 Jul 2021 13:21:48 +0200 Subject: [PATCH 8/8] PR review with Irbe: re-queue Ingress on "Update" and "Add" of certs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais Co-authored-by: Irbe Krumina --- pkg/controller/ingress-shim/controller.go | 42 ++++++++++++------- .../ingress-shim/controller_test.go | 41 ++++++++++-------- 2 files changed, 52 insertions(+), 31 deletions(-) diff --git a/pkg/controller/ingress-shim/controller.go b/pkg/controller/ingress-shim/controller.go index 46dfaab69..4fd35e5ea 100644 --- a/pkg/controller/ingress-shim/controller.go +++ b/pkg/controller/ingress-shim/controller.go @@ -77,11 +77,27 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin c.ingressLister = kShared.Networking().V1beta1().Ingresses().Lister() c.certificateLister = cmShared.Certmanager().V1().Certificates().Lister() + // We still requeue on "Deleted" for consistency with the rest of the + // controllers, but we don't actually need to. "Deleted" is only emitted + // after the apiserver has removed the object entirely from etcd; if we had + // to do some cleanup, we would use a finalizer, and the cleanup logic would + // be triggered by the "Updated" event when the object gets marked for + // deletion. kShared.Networking().V1beta1().Ingresses().Informer().AddEventHandler(&controllerpkg.QueuingEventHandler{ Queue: queue, }) - cmShared.Certmanager().V1().Certificates().Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - DeleteFunc: certificateDeleted(queue), + + // We still re-queue on "Add" because the workqueue will remove any + // duplicate key, although the Ingress controller already re-queues the + // Ingress after creating the Certificate. + // + // We re-queue on "Update" because we need to check if the Certificate is + // still up to date. + // + // We want to immediately recreate a Certificate when the Certificate is + // deleted. + cmShared.Certmanager().V1().Certificates().Informer().AddEventHandler(&controllerpkg.BlockingEventHandler{ + WorkFunc: certificateHandler(queue), }) c.kClient = ctx.Client @@ -118,23 +134,21 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return c.sync(ctx, crt) } -// Whenever a Certificate gets deleted, we want to reconcile its parent Ingress. -// This parent Ingress is called "controller object". For example, the following -// Certificate is controlled by the Ingress "example": +// Whenever a Certificate gets updated, added or deleted, we want to reconcile +// its parent Ingress. This parent Ingress is called "controller object". For +// example, the following Certificate is controlled by the Ingress "example": // // kind: Certificate -// metadata: -// namespace: cert-that-was-deleted -// ownerReferences: -// - controller: true ← this -// apiVersion: networking.k8s.io/v1beta1 -// kind: Ingress +// metadata: Note that the owner +// namespace: cert-that-was-deleted reference does not +// ownerReferences: have a namespace, +// - controller: true since owner refs +// apiVersion: networking.k8s.io/v1beta1 only work inside +// kind: Ingress the same namespace. // name: example // blockOwnerDeletion: true // uid: 7d3897c2-ce27-4144-883a-e1b5f89bd65a -// -// Note that the owner reference doesn't know about the Ingress's namespace. -func certificateDeleted(queue workqueue.RateLimitingInterface) func(obj interface{}) { +func certificateHandler(queue workqueue.RateLimitingInterface) func(obj interface{}) { return func(obj interface{}) { cert, ok := obj.(*cmapi.Certificate) if !ok { diff --git a/pkg/controller/ingress-shim/controller_test.go b/pkg/controller/ingress-shim/controller_test.go index a92edfe65..2dc6272ec 100644 --- a/pkg/controller/ingress-shim/controller_test.go +++ b/pkg/controller/ingress-shim/controller_test.go @@ -33,8 +33,6 @@ import ( cmclient "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" ) -var _ = cmapi.Certificate{} - func Test_controller_Register(t *testing.T) { tests := []struct { name string @@ -44,7 +42,7 @@ func Test_controller_Register(t *testing.T) { expectRequeueKey string }{ { - name: "ingress should be queued when it is created", + name: "ingress is re-queued when an 'Added' event is received for this ingress", givenCall: func(t *testing.T, _ cmclient.Interface, c kclient.Interface) { _, err := c.NetworkingV1beta1().Ingresses("namespace-1").Create(context.Background(), &networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ Namespace: "namespace-1", Name: "ingress-1", @@ -54,7 +52,7 @@ func Test_controller_Register(t *testing.T) { expectRequeueKey: "namespace-1/ingress-1", }, { - name: "ingress should be queued when it is updated", + name: "ingress is re-queued when an 'Updated' event is received for this ingress", existingKObjects: []runtime.Object{&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ Namespace: "namespace-1", Name: "ingress-1", }}}, @@ -67,7 +65,7 @@ func Test_controller_Register(t *testing.T) { expectRequeueKey: "namespace-1/ingress-1", }, { - name: "ingress should be queued when it is deleted", + name: "ingress is re-queued when a 'Deleted' event is received for this ingress", existingKObjects: []runtime.Object{&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ Namespace: "namespace-1", Name: "ingress-1", }}}, @@ -78,7 +76,7 @@ func Test_controller_Register(t *testing.T) { expectRequeueKey: "namespace-1/ingress-1", }, { - name: "ingress should not be queued when its child certificate is added", + name: "ingress is re-queued when an 'Added' event is received for its child Certificate", givenCall: func(t *testing.T, c cmclient.Interface, _ kclient.Interface) { _, err := c.CertmanagerV1().Certificates("namespace-1").Create(context.Background(), &cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{ Namespace: "namespace-1", Name: "cert-1", @@ -88,10 +86,10 @@ func Test_controller_Register(t *testing.T) { }}, metav1.CreateOptions{}) require.NoError(t, err) }, - expectRequeueKey: "", + expectRequeueKey: "namespace-1/ingress-2", }, { - name: "ingress should not be queued when its child certificate is updated", + name: "ingress is re-queued when an 'Updated' event is received for its child Certificate", existingCMObjects: []runtime.Object{&cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{ Namespace: "namespace-1", Name: "cert-1", OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ @@ -107,10 +105,10 @@ func Test_controller_Register(t *testing.T) { }}, metav1.UpdateOptions{}) require.NoError(t, err) }, - expectRequeueKey: "", + expectRequeueKey: "namespace-1/ingress-2", }, { - name: "ingress should be queued when its child certificate is deleted", + name: "ingress is re-queued when a 'Deleted' event is received for its child Certificate", existingCMObjects: []runtime.Object{&cmapi.Certificate{ObjectMeta: metav1.ObjectMeta{ Namespace: "namespace-1", Name: "cert-1", OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&networkingv1beta1.Ingress{ObjectMeta: metav1.ObjectMeta{ @@ -148,15 +146,24 @@ func Test_controller_Register(t *testing.T) { // done. To work around that, we do a second queue.Get and expect it // to be nil. time.AfterFunc(50*time.Millisecond, queue.ShutDown) - gotKey, _ := queue.Get() - shouldBeNil, done := queue.Get() - assert.True(t, done) - assert.Nil(t, shouldBeNil) - assert.Equal(t, 0, queue.Len()) + + var gotKeys []string + for { + // Get blocks until either (1) a key is returned, or (2) the + // queue is shut down. + gotKey, done := queue.Get() + if done { + break + } + gotKeys = append(gotKeys, gotKey.(string)) + } + assert.Equal(t, 0, queue.Len(), "queue should be empty") + + // We only expect 0 or 1 keys received in the queue. if test.expectRequeueKey != "" { - assert.Equal(t, test.expectRequeueKey, gotKey) + assert.Equal(t, []string{test.expectRequeueKey}, gotKeys) } else { - assert.Nil(t, gotKey) + assert.Nil(t, gotKeys) } }) }