From 18ae2295f99ba9788c964ce3f6b658f7a71a1f07 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Wed, 31 Mar 2021 20:34:12 +0100 Subject: [PATCH 1/2] Pass context through to client calls in controllers and acme issuer Signed-off-by: joshvanl --- pkg/controller/acmechallenges/controller.go | 2 +- pkg/controller/acmechallenges/sync.go | 6 +++--- pkg/controller/acmeorders/sync.go | 20 +++++++++---------- pkg/controller/cainjector/setup.go | 8 +++++--- pkg/controller/cainjector/sources.go | 12 +++++------ .../certificaterequests/acme/acme.go | 6 +++--- pkg/controller/certificaterequests/sync.go | 4 ++-- pkg/controller/clusterissuers/sync.go | 6 +++--- pkg/controller/clusterissuers/sync_test.go | 2 +- pkg/controller/ingress-shim/sync.go | 6 +++--- pkg/controller/issuers/sync.go | 6 +++--- pkg/controller/issuers/sync_test.go | 2 +- pkg/issuer/acme/http/ingress.go | 18 ++++++++--------- pkg/issuer/acme/http/ingress_test.go | 16 +++++++-------- pkg/issuer/acme/http/pod.go | 8 ++++---- pkg/issuer/acme/http/pod_test.go | 10 +++++----- pkg/issuer/acme/http/service.go | 8 ++++---- pkg/issuer/acme/http/service_test.go | 10 +++++----- pkg/issuer/acme/setup.go | 12 +++++------ 19 files changed, 82 insertions(+), 80 deletions(-) diff --git a/pkg/controller/acmechallenges/controller.go b/pkg/controller/acmechallenges/controller.go index 427d2c687..9359a3cb8 100644 --- a/pkg/controller/acmechallenges/controller.go +++ b/pkg/controller/acmechallenges/controller.go @@ -170,7 +170,7 @@ func (c *controller) runScheduler(ctx context.Context) { ch = ch.DeepCopy() ch.Status.Processing = true - _, err := c.cmClient.AcmeV1().Challenges(ch.Namespace).UpdateStatus(context.TODO(), ch, metav1.UpdateOptions{}) + _, err := c.cmClient.AcmeV1().Challenges(ch.Namespace).UpdateStatus(ctx, ch, metav1.UpdateOptions{}) if err != nil { log.Error(err, "error scheduling challenge for processing") return diff --git a/pkg/controller/acmechallenges/sync.go b/pkg/controller/acmechallenges/sync.go index 7eb47f6e4..f099631ad 100644 --- a/pkg/controller/acmechallenges/sync.go +++ b/pkg/controller/acmechallenges/sync.go @@ -71,7 +71,7 @@ func (c *controller) Sync(ctx context.Context, ch *cmacme.Challenge) (err error) if apiequality.Semantic.DeepEqual(oldChal.Status, ch.Status) && len(oldChal.Finalizers) == len(ch.Finalizers) { return } - _, updateErr := c.cmClient.AcmeV1().Challenges(ch.Namespace).UpdateStatus(context.TODO(), ch, metav1.UpdateOptions{}) + _, updateErr := c.cmClient.AcmeV1().Challenges(ch.Namespace).UpdateStatus(ctx, ch, metav1.UpdateOptions{}) if updateErr != nil { err = utilerrors.NewAggregate([]error{err, updateErr}) } @@ -248,14 +248,14 @@ func (c *controller) handleFinalizer(ctx context.Context, ch *cmacme.Challenge) defer func() { // call UpdateStatus first as we may have updated the challenge.status.reason field - ch, updateErr := c.cmClient.AcmeV1().Challenges(ch.Namespace).UpdateStatus(context.TODO(), ch, metav1.UpdateOptions{}) + ch, updateErr := c.cmClient.AcmeV1().Challenges(ch.Namespace).UpdateStatus(ctx, ch, metav1.UpdateOptions{}) if updateErr != nil { err = utilerrors.NewAggregate([]error{err, updateErr}) return } // call Update to remove the metadata.finalizers entry ch.Finalizers = ch.Finalizers[1:] - _, updateErr = c.cmClient.AcmeV1().Challenges(ch.Namespace).Update(context.TODO(), ch, metav1.UpdateOptions{}) + _, updateErr = c.cmClient.AcmeV1().Challenges(ch.Namespace).Update(ctx, ch, metav1.UpdateOptions{}) if updateErr != nil { err = utilerrors.NewAggregate([]error{err, updateErr}) return diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 99fe464de..9cb35e39e 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -52,7 +52,7 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { return } log.V(logf.DebugLevel).Info("updating Order resource status") - _, updateErr := c.cmClient.AcmeV1().Orders(o.Namespace).UpdateStatus(context.TODO(), o, metav1.UpdateOptions{}) + _, updateErr := c.cmClient.AcmeV1().Orders(o.Namespace).UpdateStatus(ctx, o, metav1.UpdateOptions{}) if updateErr != nil { log.Error(err, "failed to update status") err = utilerrors.NewAggregate([]error{err, updateErr}) @@ -100,7 +100,7 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { log.V(logf.DebugLevel).Info("Order has already been completed, cleaning up any owned Challenge resources") // if the Order is valid and the certificate data has been set, clean // up any owned Challenge resources and do nothing - return c.deleteAllChallenges(o) + return c.deleteAllChallenges(ctx, o) } dbg.Info("Computing list of Challenge resources that need to exist to complete this Order") @@ -125,10 +125,10 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { switch { case needToCreateChallenges: log.V(logf.DebugLevel).Info("Creating additional Challenge resources to complete Order") - return c.createRequiredChallenges(o, requiredChallenges) + return c.createRequiredChallenges(ctx, o, requiredChallenges) case needToDeleteChallenges: log.V(logf.DebugLevel).Info("Deleting leftover Challenge resources no longer required by Order") - return c.deleteLeftoverChallenges(o, requiredChallenges) + return c.deleteLeftoverChallenges(ctx, o, requiredChallenges) } // we know that this list only contains the 'required' challenges as we use @@ -339,9 +339,9 @@ func (c *controller) anyRequiredChallengesDoNotExist(requiredChallenges []cmacme return false, nil } -func (c *controller) createRequiredChallenges(o *cmacme.Order, requiredChallenges []cmacme.Challenge) error { +func (c *controller) createRequiredChallenges(ctx context.Context, o *cmacme.Order, requiredChallenges []cmacme.Challenge) error { for _, ch := range requiredChallenges { - _, err := c.cmClient.AcmeV1().Challenges(ch.Namespace).Create(context.TODO(), &ch, metav1.CreateOptions{}) + _, err := c.cmClient.AcmeV1().Challenges(ch.Namespace).Create(ctx, &ch, metav1.CreateOptions{}) if apierrors.IsAlreadyExists(err) { continue } @@ -362,14 +362,14 @@ func (c *controller) anyLeftoverChallengesExist(o *cmacme.Order, requiredChallen return len(leftoverChallenges) > 0, nil } -func (c *controller) deleteLeftoverChallenges(o *cmacme.Order, requiredChallenges []cmacme.Challenge) error { +func (c *controller) deleteLeftoverChallenges(ctx context.Context, o *cmacme.Order, requiredChallenges []cmacme.Challenge) error { leftover, err := c.determineLeftoverChallenges(o, requiredChallenges) if err != nil { return err } for _, ch := range leftover { - if err := c.cmClient.AcmeV1().Challenges(ch.Namespace).Delete(context.TODO(), ch.Name, metav1.DeleteOptions{}); err != nil { + if err := c.cmClient.AcmeV1().Challenges(ch.Namespace).Delete(ctx, ch.Name, metav1.DeleteOptions{}); err != nil { return err } } @@ -377,14 +377,14 @@ func (c *controller) deleteLeftoverChallenges(o *cmacme.Order, requiredChallenge return nil } -func (c *controller) deleteAllChallenges(o *cmacme.Order) error { +func (c *controller) deleteAllChallenges(ctx context.Context, o *cmacme.Order) error { challenges, err := c.listOwnedChallenges(o) if err != nil { return err } for _, ch := range challenges { - if err := c.cmClient.AcmeV1().Challenges(ch.Namespace).Delete(context.TODO(), ch.Name, metav1.DeleteOptions{}); err != nil { + if err := c.cmClient.AcmeV1().Challenges(ch.Namespace).Delete(ctx, ch.Name, metav1.DeleteOptions{}); err != nil { return err } } diff --git a/pkg/controller/cainjector/setup.go b/pkg/controller/cainjector/setup.go index c0722b9ce..298e0dcfa 100644 --- a/pkg/controller/cainjector/setup.go +++ b/pkg/controller/cainjector/setup.go @@ -79,7 +79,7 @@ var ( func registerAllInjectors(ctx context.Context, groupName string, mgr ctrl.Manager, sources []caDataSource, client client.Client, ca cache.Cache) error { controllers := make([]controller.Controller, len(injectorSetups)) for i, setup := range injectorSetups { - controller, err := newGenericInjectionController(groupName, mgr, setup, sources, ca, client) + controller, err := newGenericInjectionController(ctx, groupName, mgr, setup, sources, ca, client) if err != nil { if !meta.IsNoMatchError(err) || !setup.injector.IsAlpha() { return err @@ -123,7 +123,9 @@ func registerAllInjectors(ctx context.Context, groupName string, mgr ctrl.Manage // indexes and event sources. Keep checking new controller-runtime releases for // improvements which might make this easier: // * https://github.com/kubernetes-sigs/controller-runtime/issues/764 -func newGenericInjectionController(groupName string, mgr ctrl.Manager, setup injectorSetup, sources []caDataSource, ca cache.Cache, client client.Client) (controller.Controller, error) { +func newGenericInjectionController(ctx context.Context, groupName string, mgr ctrl.Manager, + setup injectorSetup, sources []caDataSource, ca cache.Cache, + client client.Client) (controller.Controller, error) { log := ctrl.Log.WithName(groupName).WithName(setup.resourceName) typ := setup.injector.NewTarget().AsObject() @@ -148,7 +150,7 @@ func newGenericInjectionController(groupName string, mgr ctrl.Manager, setup inj } for _, s := range sources { - if err := s.ApplyTo(mgr, setup, c, ca); err != nil { + if err := s.ApplyTo(ctx, mgr, setup, c, ca); err != nil { return nil, err } } diff --git a/pkg/controller/cainjector/sources.go b/pkg/controller/cainjector/sources.go index bbf22db96..18ac14e61 100644 --- a/pkg/controller/cainjector/sources.go +++ b/pkg/controller/cainjector/sources.go @@ -55,7 +55,7 @@ type caDataSource interface { ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object) (ca []byte, err error) // ApplyTo applies any required watchers to the given controller. - ApplyTo(mgr ctrl.Manager, setup injectorSetup, controller controller.Controller, ca cache.Cache) error + ApplyTo(ctx context.Context, mgr ctrl.Manager, setup injectorSetup, controller controller.Controller, ca cache.Cache) error } // kubeconfigDataSource reads the ca bundle provided as part of the struct @@ -73,7 +73,7 @@ func (c *kubeconfigDataSource) ReadCA(ctx context.Context, log logr.Logger, meta return c.apiserverCABundle, nil } -func (c *kubeconfigDataSource) ApplyTo(mgr ctrl.Manager, setup injectorSetup, _ controller.Controller, _ cache.Cache) error { +func (c *kubeconfigDataSource) ApplyTo(ctx context.Context, mgr ctrl.Manager, setup injectorSetup, _ controller.Controller, _ cache.Cache) error { cfg := mgr.GetConfig() caBundle, err := dataFromSliceOrFile(cfg.CAData, cfg.CAFile) if err != nil { @@ -142,9 +142,9 @@ func (c *certificateDataSource) ReadCA(ctx context.Context, log logr.Logger, met return caData, nil } -func (c *certificateDataSource) ApplyTo(mgr ctrl.Manager, setup injectorSetup, controller controller.Controller, ca cache.Cache) error { +func (c *certificateDataSource) ApplyTo(ctx context.Context, mgr ctrl.Manager, setup injectorSetup, controller controller.Controller, ca cache.Cache) error { typ := setup.injector.NewTarget().AsObject() - if err := ca.IndexField(context.TODO(), typ, injectFromPath, injectableCAFromIndexer); err != nil { + if err := ca.IndexField(ctx, typ, injectFromPath, injectableCAFromIndexer); err != nil { return err } @@ -219,9 +219,9 @@ func (c *secretDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj return caData, nil } -func (c *secretDataSource) ApplyTo(mgr ctrl.Manager, setup injectorSetup, controller controller.Controller, ca cache.Cache) error { +func (c *secretDataSource) ApplyTo(ctx context.Context, mgr ctrl.Manager, setup injectorSetup, controller controller.Controller, ca cache.Cache) error { typ := setup.injector.NewTarget().AsObject() - if err := ca.IndexField(context.TODO(), typ, injectFromSecretPath, injectableCAFromSecretIndexer); err != nil { + if err := ca.IndexField(ctx, typ, injectFromSecretPath, injectableCAFromSecretIndexer); err != nil { return err } if err := controller.Watch(source.NewKindWithCache(&corev1.Secret{}, ca), diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index 4fa4dfb6e..493b84a04 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -118,7 +118,7 @@ func (a *ACME) Sign(ctx context.Context, cr *v1.CertificateRequest, issuer v1.Ge if k8sErrors.IsNotFound(err) { // Failing to create the order here is most likely network related. // We should backoff and keep trying. - _, err = a.acmeClientV.Orders(expectedOrder.Namespace).Create(context.TODO(), expectedOrder, metav1.CreateOptions{}) + _, err = a.acmeClientV.Orders(expectedOrder.Namespace).Create(ctx, expectedOrder, metav1.CreateOptions{}) if err != nil { message := fmt.Sprintf("Failed create new order resource %s/%s", expectedOrder.Namespace, expectedOrder.Name) @@ -187,12 +187,12 @@ func (a *ACME) Sign(ctx context.Context, cr *v1.CertificateRequest, issuer v1.Ge x509Cert, err := pki.DecodeX509CertificateBytes(order.Status.Certificate) if err != nil { log.Error(err, "failed to decode x509 certificate data on Order resource.") - return nil, a.acmeClientV.Orders(order.Namespace).Delete(context.TODO(), order.Name, metav1.DeleteOptions{}) + return nil, a.acmeClientV.Orders(order.Namespace).Delete(ctx, order.Name, metav1.DeleteOptions{}) } if ok, err := pki.PublicKeyMatchesCertificate(csr.PublicKey, x509Cert); err != nil || !ok { log.Error(err, "The public key in Order.Status.Certificate does not match the public key in CertificateRequest.Spec.Request. Deleting the order.") - return nil, a.acmeClientV.Orders(order.Namespace).Delete(context.TODO(), order.Name, metav1.DeleteOptions{}) + return nil, a.acmeClientV.Orders(order.Namespace).Delete(ctx, order.Name, metav1.DeleteOptions{}) } log.V(logf.InfoLevel).Info("certificate issued") diff --git a/pkg/controller/certificaterequests/sync.go b/pkg/controller/certificaterequests/sync.go index 621d7a0c2..35de40168 100644 --- a/pkg/controller/certificaterequests/sync.go +++ b/pkg/controller/certificaterequests/sync.go @@ -160,7 +160,7 @@ func (c *Controller) updateCertificateRequestStatusAndAnnotations(ctx context.Co // if annotations changed we have to call .Update() and not .UpdateStatus() if !reflect.DeepEqual(old.Annotations, new.Annotations) { log.V(logf.DebugLevel).Info("updating resource due to change in annotations", "diff", pretty.Diff(old.Annotations, new.Annotations)) - return c.cmClient.CertmanagerV1().CertificateRequests(new.Namespace).Update(context.TODO(), new, metav1.UpdateOptions{}) + return c.cmClient.CertmanagerV1().CertificateRequests(new.Namespace).Update(ctx, new, metav1.UpdateOptions{}) } if apiequality.Semantic.DeepEqual(old.Status, new.Status) { @@ -168,5 +168,5 @@ func (c *Controller) updateCertificateRequestStatusAndAnnotations(ctx context.Co } log.V(logf.DebugLevel).Info("updating resource due to change in status", "diff", pretty.Diff(old.Status, new.Status)) - return c.cmClient.CertmanagerV1().CertificateRequests(new.Namespace).UpdateStatus(context.TODO(), new, metav1.UpdateOptions{}) + return c.cmClient.CertmanagerV1().CertificateRequests(new.Namespace).UpdateStatus(ctx, new, metav1.UpdateOptions{}) } diff --git a/pkg/controller/clusterissuers/sync.go b/pkg/controller/clusterissuers/sync.go index e6f9789e8..5309b6be4 100644 --- a/pkg/controller/clusterissuers/sync.go +++ b/pkg/controller/clusterissuers/sync.go @@ -40,7 +40,7 @@ func (c *controller) Sync(ctx context.Context, iss *cmapi.ClusterIssuer) (err er issuerCopy := iss.DeepCopy() defer func() { - if _, saveErr := c.updateIssuerStatus(iss, issuerCopy); saveErr != nil { + if _, saveErr := c.updateIssuerStatus(ctx, iss, issuerCopy); saveErr != nil { err = errors.NewAggregate([]error{saveErr, err}) } }() @@ -64,9 +64,9 @@ func (c *controller) Sync(ctx context.Context, iss *cmapi.ClusterIssuer) (err er return nil } -func (c *controller) updateIssuerStatus(old, new *cmapi.ClusterIssuer) (*cmapi.ClusterIssuer, error) { +func (c *controller) updateIssuerStatus(ctx context.Context, old, new *cmapi.ClusterIssuer) (*cmapi.ClusterIssuer, error) { if apiequality.Semantic.DeepEqual(old.Status, new.Status) { return nil, nil } - return c.cmClient.CertmanagerV1().ClusterIssuers().UpdateStatus(context.TODO(), new, metav1.UpdateOptions{}) + return c.cmClient.CertmanagerV1().ClusterIssuers().UpdateStatus(ctx, new, metav1.UpdateOptions{}) } diff --git a/pkg/controller/clusterissuers/sync_test.go b/pkg/controller/clusterissuers/sync_test.go index 96fc8bf04..f4065aace 100644 --- a/pkg/controller/clusterissuers/sync_test.go +++ b/pkg/controller/clusterissuers/sync_test.go @@ -76,7 +76,7 @@ func TestUpdateIssuerStatus(t *testing.T) { issuerCopy := issuer.DeepCopy() issuerCopy.Status = newStatus - _, err = c.updateIssuerStatus(issuer, issuerCopy) + _, err = c.updateIssuerStatus(context.TODO(), issuer, issuerCopy) assertErrIsNil(t, fatalf, err) actions := filter(fakeClient.Actions()) diff --git a/pkg/controller/ingress-shim/sync.go b/pkg/controller/ingress-shim/sync.go index 6c3aa10cb..f63cb1c11 100644 --- a/pkg/controller/ingress-shim/sync.go +++ b/pkg/controller/ingress-shim/sync.go @@ -74,7 +74,7 @@ func (c *controller) Sync(ctx context.Context, ing *networkingv1beta1.Ingress) e } for _, crt := range newCrts { - _, err := c.cmClient.CertmanagerV1().Certificates(crt.Namespace).Create(context.TODO(), crt, metav1.CreateOptions{}) + _, err := c.cmClient.CertmanagerV1().Certificates(crt.Namespace).Create(ctx, crt, metav1.CreateOptions{}) if err != nil { return err } @@ -82,7 +82,7 @@ func (c *controller) Sync(ctx context.Context, ing *networkingv1beta1.Ingress) e } for _, crt := range updateCrts { - _, err := c.cmClient.CertmanagerV1().Certificates(crt.Namespace).Update(context.TODO(), crt, metav1.UpdateOptions{}) + _, err := c.cmClient.CertmanagerV1().Certificates(crt.Namespace).Update(ctx, crt, metav1.UpdateOptions{}) if err != nil { return err } @@ -95,7 +95,7 @@ func (c *controller) Sync(ctx context.Context, ing *networkingv1beta1.Ingress) e } for _, crt := range unrequiredCrts { - err = c.cmClient.CertmanagerV1().Certificates(crt.Namespace).Delete(context.TODO(), crt.Name, metav1.DeleteOptions{}) + err = c.cmClient.CertmanagerV1().Certificates(crt.Namespace).Delete(ctx, crt.Name, metav1.DeleteOptions{}) if err != nil { return err } diff --git a/pkg/controller/issuers/sync.go b/pkg/controller/issuers/sync.go index fcca82df4..48e01e567 100644 --- a/pkg/controller/issuers/sync.go +++ b/pkg/controller/issuers/sync.go @@ -40,7 +40,7 @@ func (c *controller) Sync(ctx context.Context, iss *cmapi.Issuer) (err error) { issuerCopy := iss.DeepCopy() defer func() { - if _, saveErr := c.updateIssuerStatus(iss, issuerCopy); saveErr != nil { + if _, saveErr := c.updateIssuerStatus(ctx, iss, issuerCopy); saveErr != nil { err = errors.NewAggregate([]error{saveErr, err}) } }() @@ -64,9 +64,9 @@ func (c *controller) Sync(ctx context.Context, iss *cmapi.Issuer) (err error) { return nil } -func (c *controller) updateIssuerStatus(old, new *cmapi.Issuer) (*cmapi.Issuer, error) { +func (c *controller) updateIssuerStatus(ctx context.Context, old, new *cmapi.Issuer) (*cmapi.Issuer, error) { if apiequality.Semantic.DeepEqual(old.Status, new.Status) { return nil, nil } - return c.cmClient.CertmanagerV1().Issuers(new.Namespace).UpdateStatus(context.TODO(), new, metav1.UpdateOptions{}) + return c.cmClient.CertmanagerV1().Issuers(new.Namespace).UpdateStatus(ctx, new, metav1.UpdateOptions{}) } diff --git a/pkg/controller/issuers/sync_test.go b/pkg/controller/issuers/sync_test.go index 809afcf0b..67cdc40c0 100644 --- a/pkg/controller/issuers/sync_test.go +++ b/pkg/controller/issuers/sync_test.go @@ -76,7 +76,7 @@ func TestUpdateIssuerStatus(t *testing.T) { issuerCopy := issuer.DeepCopy() issuerCopy.Status = newStatus - _, err = c.updateIssuerStatus(issuer, issuerCopy) + _, err = c.updateIssuerStatus(context.TODO(), issuer, issuerCopy) assertErrIsNil(t, fatalf, err) actions := filter(cmClient.Actions()) diff --git a/pkg/issuer/acme/http/ingress.go b/pkg/issuer/acme/http/ingress.go index 078ba36bc..5573daf49 100644 --- a/pkg/issuer/acme/http/ingress.go +++ b/pkg/issuer/acme/http/ingress.go @@ -110,7 +110,7 @@ func (s *Solver) ensureIngress(ctx context.Context, ch *cmacme.Challenge, svcNam } log.V(logf.DebugLevel).Info("creating HTTP01 challenge solver ingress") - return s.createIngress(ch, svcName) + return s.createIngress(ctx, ch, svcName) } func ingressServiceName(ing *networkingv1beta1.Ingress) string { @@ -119,7 +119,7 @@ func ingressServiceName(ing *networkingv1beta1.Ingress) string { // createIngress will create a challenge solving ingress for the given certificate, // domain, token and key. -func (s *Solver) createIngress(ch *cmacme.Challenge, svcName string) (*networkingv1beta1.Ingress, error) { +func (s *Solver) createIngress(ctx context.Context, ch *cmacme.Challenge, svcName string) (*networkingv1beta1.Ingress, error) { ing, err := buildIngressResource(ch, svcName) if err != nil { return nil, err @@ -131,7 +131,7 @@ func (s *Solver) createIngress(ch *cmacme.Challenge, svcName string) (*networkin ing = s.mergeIngressObjectMetaWithIngressResourceTemplate(ing, ch.Spec.Solver.HTTP01.Ingress.IngressTemplate) } - return s.Client.NetworkingV1beta1().Ingresses(ch.Namespace).Create(context.TODO(), ing, metav1.CreateOptions{}) + return s.Client.NetworkingV1beta1().Ingresses(ch.Namespace).Create(ctx, ing, metav1.CreateOptions{}) } func buildIngressResource(ch *cmacme.Challenge, svcName string) (*networkingv1beta1.Ingress, error) { @@ -239,11 +239,11 @@ func (s *Solver) addChallengePathToIngress(ctx context.Context, ch *cmacme.Chall return ing, nil } rule.HTTP.Paths[i] = ingPathToAdd - return s.Client.NetworkingV1beta1().Ingresses(ing.Namespace).Update(context.TODO(), ing, metav1.UpdateOptions{}) + return s.Client.NetworkingV1beta1().Ingresses(ing.Namespace).Update(ctx, ing, metav1.UpdateOptions{}) } } rule.HTTP.Paths = append([]networkingv1beta1.HTTPIngressPath{ingPathToAdd}, rule.HTTP.Paths...) - return s.Client.NetworkingV1beta1().Ingresses(ing.Namespace).Update(context.TODO(), ing, metav1.UpdateOptions{}) + return s.Client.NetworkingV1beta1().Ingresses(ing.Namespace).Update(ctx, ing, metav1.UpdateOptions{}) } } @@ -256,7 +256,7 @@ func (s *Solver) addChallengePathToIngress(ctx context.Context, ch *cmacme.Chall }, }, }) - return s.Client.NetworkingV1beta1().Ingresses(ing.Namespace).Update(context.TODO(), ing, metav1.UpdateOptions{}) + return s.Client.NetworkingV1beta1().Ingresses(ing.Namespace).Update(ctx, ing, metav1.UpdateOptions{}) } // cleanupIngresses will remove the rules added by cert-manager to an existing @@ -283,7 +283,7 @@ func (s *Solver) cleanupIngresses(ctx context.Context, ch *cmacme.Challenge) err log := logf.WithRelatedResource(log, ingress).V(logf.DebugLevel) log.V(logf.DebugLevel).Info("deleting ingress resource") - err := s.Client.NetworkingV1beta1().Ingresses(ingress.Namespace).Delete(context.TODO(), ingress.Name, metav1.DeleteOptions{}) + err := s.Client.NetworkingV1beta1().Ingresses(ingress.Namespace).Delete(ctx, ingress.Name, metav1.DeleteOptions{}) if err != nil { log.V(logf.WarnLevel).Info("failed to delete ingress resource", "error", err) errs = append(errs, err) @@ -295,7 +295,7 @@ func (s *Solver) cleanupIngresses(ctx context.Context, ch *cmacme.Challenge) err } // otherwise, we need to remove any cert-manager added rules from the ingress resource - ing, err := s.Client.NetworkingV1beta1().Ingresses(ch.Namespace).Get(context.TODO(), existingIngressName, metav1.GetOptions{}) + ing, err := s.Client.NetworkingV1beta1().Ingresses(ch.Namespace).Get(ctx, existingIngressName, metav1.GetOptions{}) if k8sErrors.IsNotFound(err) { log.Error(err, "named ingress resource not found, skipping cleanup") return nil @@ -338,7 +338,7 @@ func (s *Solver) cleanupIngresses(ctx context.Context, ch *cmacme.Challenge) err ing.Spec.Rules = ingRules - _, err = s.Client.NetworkingV1beta1().Ingresses(ing.Namespace).Update(context.TODO(), ing, metav1.UpdateOptions{}) + _, err = s.Client.NetworkingV1beta1().Ingresses(ing.Namespace).Update(ctx, ing, metav1.UpdateOptions{}) if err != nil { return err } diff --git a/pkg/issuer/acme/http/ingress_test.go b/pkg/issuer/acme/http/ingress_test.go index 46d37b6b4..e0e223b27 100644 --- a/pkg/issuer/acme/http/ingress_test.go +++ b/pkg/issuer/acme/http/ingress_test.go @@ -50,7 +50,7 @@ func TestGetIngressesForChallenge(t *testing.T) { }, }, PreFn: func(t *testing.T, s *solverFixture) { - ing, err := s.Solver.createIngress(s.Challenge, "fakeservice") + ing, err := s.Solver.createIngress(context.TODO(), s.Challenge, "fakeservice") if err != nil { t.Errorf("error preparing test: %v", err) } @@ -83,7 +83,7 @@ func TestGetIngressesForChallenge(t *testing.T) { }, }, PreFn: func(t *testing.T, s *solverFixture) { - ing, err := s.Solver.createIngress(s.Challenge, "fakeservice") + ing, err := s.Solver.createIngress(context.TODO(), s.Challenge, "fakeservice") if err != nil { t.Errorf("error preparing test: %v", err) } @@ -118,7 +118,7 @@ func TestGetIngressesForChallenge(t *testing.T) { PreFn: func(t *testing.T, s *solverFixture) { differentChallenge := s.Challenge.DeepCopy() differentChallenge.Spec.DNSName = "notexample.com" - _, err := s.Solver.createIngress(differentChallenge, "fakeservice") + _, err := s.Solver.createIngress(context.TODO(), differentChallenge, "fakeservice") if err != nil { t.Errorf("error preparing test: %v", err) } @@ -168,7 +168,7 @@ func TestCleanupIngresses(t *testing.T) { }, }, PreFn: func(t *testing.T, s *solverFixture) { - ing, err := s.Solver.createIngress(s.Challenge, "fakeservice") + ing, err := s.Solver.createIngress(context.TODO(), s.Challenge, "fakeservice") if err != nil { t.Errorf("error preparing test: %v", err) } @@ -203,7 +203,7 @@ func TestCleanupIngresses(t *testing.T) { PreFn: func(t *testing.T, s *solverFixture) { differentChallenge := s.Challenge.DeepCopy() differentChallenge.Spec.DNSName = "notexample.com" - ing, err := s.Solver.createIngress(differentChallenge, "fakeservice") + ing, err := s.Solver.createIngress(context.TODO(), differentChallenge, "fakeservice") if err != nil { t.Errorf("error preparing test: %v", err) } @@ -397,7 +397,7 @@ func TestCleanupIngresses(t *testing.T) { s.Builder.FakeKubeClient().PrependReactor("delete", "ingresses", func(action coretesting.Action) (handled bool, ret runtime.Object, err error) { return true, nil, fmt.Errorf("simulated error") }) - ing, err := s.Solver.createIngress(s.Challenge, "fakeservice") + ing, err := s.Solver.createIngress(context.TODO(), s.Challenge, "fakeservice") if err != nil { t.Errorf("error preparing test: %v", err) } @@ -436,7 +436,7 @@ func TestEnsureIngress(t *testing.T) { }, Err: true, PreFn: func(t *testing.T, s *solverFixture) { - _, err := s.Solver.createIngress(s.Challenge, "anotherfakeservice") + _, err := s.Solver.createIngress(context.TODO(), s.Challenge, "anotherfakeservice") if err != nil { t.Errorf("error preparing test: %v", err) } @@ -542,7 +542,7 @@ func TestMergeIngressObjectMetaWithIngressResourceTemplate(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { test.Setup(t) - resp, err := test.Solver.createIngress(test.Challenge, "fakeservice") + resp, err := test.Solver.createIngress(context.TODO(), test.Challenge, "fakeservice") test.Finish(t, resp, err) }) } diff --git a/pkg/issuer/acme/http/pod.go b/pkg/issuer/acme/http/pod.go index 8f545560f..80aca1f53 100644 --- a/pkg/issuer/acme/http/pod.go +++ b/pkg/issuer/acme/http/pod.go @@ -69,7 +69,7 @@ func (s *Solver) ensurePod(ctx context.Context, ch *cmacme.Challenge) (*corev1.P log.V(logf.InfoLevel).Info("creating HTTP01 challenge solver pod") - return s.createPod(ch) + return s.createPod(ctx, ch) } // getPodsForChallenge returns a list of pods that were created to solve @@ -117,7 +117,7 @@ func (s *Solver) cleanupPods(ctx context.Context, ch *cmacme.Challenge) error { log := logf.WithRelatedResource(log, pod).V(logf.DebugLevel) log.V(logf.InfoLevel).Info("deleting pod resource") - err := s.Client.CoreV1().Pods(pod.Namespace).Delete(context.TODO(), pod.Name, metav1.DeleteOptions{}) + err := s.Client.CoreV1().Pods(pod.Namespace).Delete(ctx, pod.Name, metav1.DeleteOptions{}) if err != nil { log.V(logf.WarnLevel).Info("failed to delete pod resource", "error", err) errs = append(errs, err) @@ -131,9 +131,9 @@ func (s *Solver) cleanupPods(ctx context.Context, ch *cmacme.Challenge) error { // createPod will create a challenge solving pod for the given certificate, // domain, token and key. -func (s *Solver) createPod(ch *cmacme.Challenge) (*corev1.Pod, error) { +func (s *Solver) createPod(ctx context.Context, ch *cmacme.Challenge) (*corev1.Pod, error) { return s.Client.CoreV1().Pods(ch.Namespace).Create( - context.TODO(), + ctx, s.buildPod(ch), metav1.CreateOptions{}) } diff --git a/pkg/issuer/acme/http/pod_test.go b/pkg/issuer/acme/http/pod_test.go index 5885950dd..07b932b06 100644 --- a/pkg/issuer/acme/http/pod_test.go +++ b/pkg/issuer/acme/http/pod_test.go @@ -46,7 +46,7 @@ func TestEnsurePod(t *testing.T) { }, }, PreFn: func(t *testing.T, s *solverFixture) { - ing, err := s.Solver.createPod(s.Challenge) + ing, err := s.Solver.createPod(context.TODO(), s.Challenge) if err != nil { t.Errorf("error preparing test: %v", err) } @@ -142,11 +142,11 @@ func TestEnsurePod(t *testing.T) { }, Err: true, PreFn: func(t *testing.T, s *solverFixture) { - _, err := s.Solver.createPod(s.Challenge) + _, err := s.Solver.createPod(context.TODO(), s.Challenge) if err != nil { t.Errorf("error preparing test: %v", err) } - _, err = s.Solver.createPod(s.Challenge) + _, err = s.Solver.createPod(context.TODO(), s.Challenge) if err != nil { t.Errorf("error preparing test: %v", err) } @@ -196,7 +196,7 @@ func TestGetPodsForCertificate(t *testing.T) { }, }, PreFn: func(t *testing.T, s *solverFixture) { - ing, err := s.Solver.createPod(s.Challenge) + ing, err := s.Solver.createPod(context.TODO(), s.Challenge) if err != nil { t.Errorf("error preparing test: %v", err) } @@ -231,7 +231,7 @@ func TestGetPodsForCertificate(t *testing.T) { PreFn: func(t *testing.T, s *solverFixture) { differentChallenge := s.Challenge.DeepCopy() differentChallenge.Spec.DNSName = "notexample.com" - _, err := s.Solver.createPod(differentChallenge) + _, err := s.Solver.createPod(context.TODO(), differentChallenge) if err != nil { t.Errorf("error preparing test: %v", err) } diff --git a/pkg/issuer/acme/http/service.go b/pkg/issuer/acme/http/service.go index c9aa3b2b4..3adf7a450 100644 --- a/pkg/issuer/acme/http/service.go +++ b/pkg/issuer/acme/http/service.go @@ -53,7 +53,7 @@ func (s *Solver) ensureService(ctx context.Context, ch *cmacme.Challenge) (*core } log.V(logf.DebugLevel).Info("creating HTTP01 challenge solver service") - return s.createService(ch) + return s.createService(ctx, ch) } // getServicesForChallenge returns a list of services that were created to solve @@ -91,12 +91,12 @@ func (s *Solver) getServicesForChallenge(ctx context.Context, ch *cmacme.Challen // createService will create the service required to solve this challenge // in the target API server. -func (s *Solver) createService(ch *cmacme.Challenge) (*corev1.Service, error) { +func (s *Solver) createService(ctx context.Context, ch *cmacme.Challenge) (*corev1.Service, error) { svc, err := buildService(ch) if err != nil { return nil, err } - return s.Client.CoreV1().Services(ch.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{}) + return s.Client.CoreV1().Services(ch.Namespace).Create(ctx, svc, metav1.CreateOptions{}) } func buildService(ch *cmacme.Challenge) (*corev1.Service, error) { @@ -148,7 +148,7 @@ func (s *Solver) cleanupServices(ctx context.Context, ch *cmacme.Challenge) erro log := logf.WithRelatedResource(log, service).V(logf.DebugLevel) log.V(logf.DebugLevel).Info("deleting service resource") - err := s.Client.CoreV1().Services(service.Namespace).Delete(context.TODO(), service.Name, metav1.DeleteOptions{}) + err := s.Client.CoreV1().Services(service.Namespace).Delete(ctx, service.Name, metav1.DeleteOptions{}) if err != nil { log.V(logf.WarnLevel).Info("failed to delete pod resource", "error", err) errs = append(errs, err) diff --git a/pkg/issuer/acme/http/service_test.go b/pkg/issuer/acme/http/service_test.go index 69835430c..8fec87ca7 100644 --- a/pkg/issuer/acme/http/service_test.go +++ b/pkg/issuer/acme/http/service_test.go @@ -44,7 +44,7 @@ func TestEnsureService(t *testing.T) { }, }, PreFn: func(t *testing.T, s *solverFixture) { - svc, err := s.Solver.createService(s.Challenge) + svc, err := s.Solver.createService(context.TODO(), s.Challenge) if err != nil { t.Errorf("error preparing test: %v", err) } @@ -139,11 +139,11 @@ func TestEnsureService(t *testing.T) { }, Err: true, PreFn: func(t *testing.T, s *solverFixture) { - _, err := s.Solver.createService(s.Challenge) + _, err := s.Solver.createService(context.TODO(), s.Challenge) if err != nil { t.Errorf("error preparing test: %v", err) } - _, err = s.Solver.createService(s.Challenge) + _, err = s.Solver.createService(context.TODO(), s.Challenge) if err != nil { t.Errorf("error preparing test: %v", err) } @@ -193,7 +193,7 @@ func TestGetServicesForChallenge(t *testing.T) { }, }, PreFn: func(t *testing.T, s *solverFixture) { - ing, err := s.Solver.createService(s.Challenge) + ing, err := s.Solver.createService(context.TODO(), s.Challenge) if err != nil { t.Errorf("error preparing test: %v", err) } @@ -228,7 +228,7 @@ func TestGetServicesForChallenge(t *testing.T) { PreFn: func(t *testing.T, s *solverFixture) { differentChallenge := s.Challenge.DeepCopy() differentChallenge.Spec.DNSName = "invaliddomain" - _, err := s.Solver.createService(differentChallenge) + _, err := s.Solver.createService(context.TODO(), differentChallenge) if err != nil { t.Errorf("error preparing test: %v", err) } diff --git a/pkg/issuer/acme/setup.go b/pkg/issuer/acme/setup.go index 7e3ae3673..a2fc4f5dc 100644 --- a/pkg/issuer/acme/setup.go +++ b/pkg/issuer/acme/setup.go @@ -88,7 +88,7 @@ func (a *Acme) Setup(ctx context.Context) error { switch { case !a.issuer.GetSpec().ACME.DisableAccountKeyGeneration && apierrors.IsNotFound(err): log.V(logf.InfoLevel).Info("generating acme account private key") - pk, err = a.createAccountPrivateKey(privateKeySelector, ns) + pk, err = a.createAccountPrivateKey(ctx, privateKeySelector, ns) if err != nil { s := messageAccountRegistrationFailed + err.Error() apiutil.SetIssuerCondition(a.issuer, a.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorAccountRegistrationFailed, s) @@ -182,7 +182,7 @@ func (a *Acme) Setup(ctx context.Context) error { var eabAccount *acmeapi.ExternalAccountBinding if eabObj := a.issuer.GetSpec().ACME.ExternalAccountBinding; eabObj != nil { - eabKey, err := a.getEABKey(ns) + eabKey, err := a.getEABKey(ctx, ns) switch { // Do not re-try if we fail to get the MAC key as it does not exist at the reference. case apierrors.IsNotFound(err), errors.IsInvalidData(err): @@ -339,9 +339,9 @@ func (a *Acme) registerAccount(ctx context.Context, cl client.Interface, eabAcco return acc, nil } -func (a *Acme) getEABKey(ns string) ([]byte, error) { +func (a *Acme) getEABKey(ctx context.Context, ns string) ([]byte, error) { eab := a.issuer.GetSpec().ACME.ExternalAccountBinding.Key - sec, err := a.secretsClient.Secrets(ns).Get(context.TODO(), eab.Name, metav1.GetOptions{}) + sec, err := a.secretsClient.Secrets(ns).Get(ctx, eab.Name, metav1.GetOptions{}) // Surface IsNotFound API error to not cause re-sync if apierrors.IsNotFound(err) { return nil, err @@ -370,14 +370,14 @@ func (a *Acme) getEABKey(ns string) ([]byte, error) { // createAccountPrivateKey will generate a new RSA private key, and create it // as a secret resource in the apiserver. -func (a *Acme) createAccountPrivateKey(sel cmmeta.SecretKeySelector, ns string) (*rsa.PrivateKey, error) { +func (a *Acme) createAccountPrivateKey(ctx context.Context, sel cmmeta.SecretKeySelector, ns string) (*rsa.PrivateKey, error) { sel = acme.PrivateKeySelector(sel) accountPrivKey, err := pki.GenerateRSAPrivateKey(pki.MinRSAKeySize) if err != nil { return nil, err } - _, err = a.secretsClient.Secrets(ns).Create(context.TODO(), &corev1.Secret{ + _, err = a.secretsClient.Secrets(ns).Create(ctx, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: sel.Name, Namespace: ns, From a072738c42d65c3fa23d7a26d31206543e567605 Mon Sep 17 00:00:00 2001 From: joshvanl Date: Tue, 6 Apr 2021 16:26:18 +0100 Subject: [PATCH 2/2] Move canceled context defer to first in stack for [cluster]issuer controllers Signed-off-by: joshvanl --- pkg/controller/clusterissuers/sync.go | 7 ++++--- pkg/controller/issuers/sync.go | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/controller/clusterissuers/sync.go b/pkg/controller/clusterissuers/sync.go index 5309b6be4..bd1e4a851 100644 --- a/pkg/controller/clusterissuers/sync.go +++ b/pkg/controller/clusterissuers/sync.go @@ -38,6 +38,10 @@ const ( func (c *controller) Sync(ctx context.Context, iss *cmapi.ClusterIssuer) (err error) { log := logf.FromContext(ctx) + // allow a maximum of 10s + ctx, cancel := context.WithTimeout(ctx, time.Second*10) + defer cancel() + issuerCopy := iss.DeepCopy() defer func() { if _, saveErr := c.updateIssuerStatus(ctx, iss, issuerCopy); saveErr != nil { @@ -50,9 +54,6 @@ func (c *controller) Sync(ctx context.Context, iss *cmapi.ClusterIssuer) (err er return err } - // allow a maximum of 10s - ctx, cancel := context.WithTimeout(ctx, time.Second*10) - defer cancel() err = i.Setup(ctx) if err != nil { s := messageErrorInitIssuer + err.Error() diff --git a/pkg/controller/issuers/sync.go b/pkg/controller/issuers/sync.go index 48e01e567..25124b7f1 100644 --- a/pkg/controller/issuers/sync.go +++ b/pkg/controller/issuers/sync.go @@ -38,6 +38,10 @@ const ( func (c *controller) Sync(ctx context.Context, iss *cmapi.Issuer) (err error) { log := logf.FromContext(ctx) + // allow a maximum of 10s + ctx, cancel := context.WithTimeout(ctx, time.Second*10) + defer cancel() + issuerCopy := iss.DeepCopy() defer func() { if _, saveErr := c.updateIssuerStatus(ctx, iss, issuerCopy); saveErr != nil { @@ -50,9 +54,6 @@ func (c *controller) Sync(ctx context.Context, iss *cmapi.Issuer) (err error) { return err } - // allow a maximum of 10s - ctx, cancel := context.WithTimeout(ctx, time.Second*10) - defer cancel() err = i.Setup(ctx) if err != nil { s := messageErrorInitIssuer + err.Error()