From 7d615ff8e4f89e67e447675427404067a8d23860 Mon Sep 17 00:00:00 2001 From: JoshVanL Date: Thu, 3 Oct 2019 09:47:41 +0100 Subject: [PATCH] Remove getting secret from lister in matches spec func Signed-off-by: JoshVanL --- pkg/controller/certificates/BUILD.bazel | 1 - pkg/controller/certificates/sync.go | 15 +++++++---- pkg/controller/certificates/util.go | 8 ++---- pkg/controller/certificates/util_test.go | 34 +++++++----------------- 4 files changed, 21 insertions(+), 37 deletions(-) diff --git a/pkg/controller/certificates/BUILD.bazel b/pkg/controller/certificates/BUILD.bazel index fc506b3f4..1b23b9c81 100644 --- a/pkg/controller/certificates/BUILD.bazel +++ b/pkg/controller/certificates/BUILD.bazel @@ -55,7 +55,6 @@ go_test( "//pkg/util:go_default_library", "//pkg/util/pki:go_default_library", "//test/unit/gen:go_default_library", - "//test/unit/listers:go_default_library", "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_apimachinery//pkg/runtime:go_default_library", diff --git a/pkg/controller/certificates/sync.go b/pkg/controller/certificates/sync.go index 8f5a41545..eed08beda 100644 --- a/pkg/controller/certificates/sync.go +++ b/pkg/controller/certificates/sync.go @@ -102,7 +102,12 @@ func (c *certificateRequestManager) updateCertificateStatus(ctx context.Context, var matches bool var matchErrs []string if key != nil && cert != nil { - matches, matchErrs = certificateMatchesSpec(crt, key, cert, c.secretLister) + secret, err := c.secretLister.Secrets(crt.Namespace).Get(crt.Spec.SecretName) + if err != nil { + return err + } + + matches, matchErrs = certificateMatchesSpec(crt, key, cert, secret) } isTempCert := isTemporaryCertificate(cert) @@ -255,7 +260,7 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt // a valid partner to the stored certificate. var matchErrs []string dbg.Info("checking if existing certificate stored in Secret resource is not expiring soon and matches certificate spec") - needsIssue, matchErrs, err = c.certificateRequiresIssuance(ctx, crt, existingKey, existingCert) + needsIssue, matchErrs, err = c.certificateRequiresIssuance(ctx, crt, existingKey, existingCert, existingSecret) if err != nil && !errors.IsInvalidData(err) { return err } @@ -330,7 +335,7 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt } // We don't issue a temporary certificate if the existing stored // certificate already 'matches', even if it isn't a temporary certificate. - matches, _ := certificateMatchesSpec(crt, privateKey, existingX509Cert, c.secretLister) + matches, _ := certificateMatchesSpec(crt, privateKey, existingX509Cert, existingSecret) if !matches { log.Info("existing certificate fields do not match certificate spec, issuing temporary certificate") return c.issueTemporaryCertificate(ctx, existingSecret, crt, existingKey) @@ -556,7 +561,7 @@ func (c *certificateRequestManager) issueTemporaryCertificate(ctx context.Contex return nil } -func (c *certificateRequestManager) certificateRequiresIssuance(ctx context.Context, crt *cmapi.Certificate, keyBytes, certBytes []byte) (bool, []string, error) { +func (c *certificateRequestManager) certificateRequiresIssuance(ctx context.Context, crt *cmapi.Certificate, keyBytes, certBytes []byte, secret *corev1.Secret) (bool, []string, error) { key, err := pki.DecodePrivateKeyBytes(keyBytes) if err != nil { return false, nil, err @@ -568,7 +573,7 @@ func (c *certificateRequestManager) certificateRequiresIssuance(ctx context.Cont if isTemporaryCertificate(cert) { return true, nil, nil } - matches, matchErrs := certificateMatchesSpec(crt, key, cert, c.secretLister) + matches, matchErrs := certificateMatchesSpec(crt, key, cert, secret) if !matches { return true, matchErrs, nil } diff --git a/pkg/controller/certificates/util.go b/pkg/controller/certificates/util.go index 1c97fab8a..e5ada1489 100644 --- a/pkg/controller/certificates/util.go +++ b/pkg/controller/certificates/util.go @@ -27,6 +27,7 @@ import ( "time" "github.com/kr/pretty" + corev1 "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" corelisters "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" @@ -75,7 +76,7 @@ func certificateGetter(lister cmlisters.CertificateLister) func(namespace, name var keyFunc = controllerpkg.KeyFunc -func certificateMatchesSpec(crt *v1alpha2.Certificate, key crypto.Signer, cert *x509.Certificate, secretLister corelisters.SecretLister) (bool, []string) { +func certificateMatchesSpec(crt *v1alpha2.Certificate, key crypto.Signer, cert *x509.Certificate, secret *corev1.Secret) (bool, []string) { var errs []string // TODO: add checks for KeySize, KeyAlgorithm fields @@ -116,11 +117,6 @@ func certificateMatchesSpec(crt *v1alpha2.Certificate, key crypto.Signer, cert * errs = append(errs, fmt.Sprintf("IP addresses on TLS certificate not up to date: %q", pki.IPAddressesToString(cert.IPAddresses))) } - // get a copy of the current secret resource - // Note that we already know that it exists, no need to check for errors - // TODO: Refactor so that the secret is passed as argument? - secret, err := secretLister.Secrets(crt.Namespace).Get(crt.Spec.SecretName) - if secret.Annotations == nil { secret.Annotations = make(map[string]string) } diff --git a/pkg/controller/certificates/util_test.go b/pkg/controller/certificates/util_test.go index 12afc2c46..ac18e5b95 100644 --- a/pkg/controller/certificates/util_test.go +++ b/pkg/controller/certificates/util_test.go @@ -27,7 +27,6 @@ import ( cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/test/unit/gen" - "github.com/jetstack/cert-manager/test/unit/listers" ) func TestCertificateMatchesSpec(t *testing.T) { @@ -55,7 +54,7 @@ func TestCertificateMatchesSpec(t *testing.T) { type testT struct { cb cryptoBundle certificate *cmapi.Certificate - fakeLister *listers.FakeSecretLister + secret *corev1.Secret expMatch bool expErrors []string } @@ -64,11 +63,8 @@ func TestCertificateMatchesSpec(t *testing.T) { "if all match then return matched": { cb: exampleBundle, certificate: exampleBundle.certificate, - fakeLister: listers.FakeSecretListerFrom(listers.NewFakeSecretLister(), - listers.SetFakeSecretNamespaceListerGet(secret, nil), - ), - expMatch: true, - expErrors: nil, + expMatch: true, + expErrors: nil, }, "if no common name but DNS and all match then return matched": { @@ -78,9 +74,6 @@ func TestCertificateMatchesSpec(t *testing.T) { certificate: gen.CertificateFrom(exampleBundle.certificate, gen.SetCertificateCommonName(""), ), - fakeLister: listers.FakeSecretListerFrom(listers.NewFakeSecretLister(), - listers.SetFakeSecretNamespaceListerGet(secret, nil), - ), expMatch: true, expErrors: nil, }, @@ -91,11 +84,8 @@ func TestCertificateMatchesSpec(t *testing.T) { gen.SetCertificateCommonName(""), )), certificate: gen.CertificateFrom(exampleBundle.certificate), - fakeLister: listers.FakeSecretListerFrom(listers.NewFakeSecretLister(), - listers.SetFakeSecretNamespaceListerGet(secret, nil), - ), - expMatch: true, - expErrors: nil, + expMatch: true, + expErrors: nil, }, "if common name random string but requested common name in DNS names then match": { @@ -104,11 +94,8 @@ func TestCertificateMatchesSpec(t *testing.T) { gen.SetCertificateCommonName("foobar"), )), certificate: gen.CertificateFrom(exampleBundle.certificate), - fakeLister: listers.FakeSecretListerFrom(listers.NewFakeSecretLister(), - listers.SetFakeSecretNamespaceListerGet(secret, nil), - ), - expMatch: true, - expErrors: nil, + expMatch: true, + expErrors: nil, }, "if common name random string and no request DNS names but request common name then error missing common name": { @@ -117,10 +104,7 @@ func TestCertificateMatchesSpec(t *testing.T) { gen.SetCertificateCommonName("foobar"), )), certificate: gen.CertificateFrom(exampleBundle.certificate), - fakeLister: listers.FakeSecretListerFrom(listers.NewFakeSecretLister(), - listers.SetFakeSecretNamespaceListerGet(secret, nil), - ), - expMatch: false, + expMatch: false, expErrors: []string{ `Common Name on TLS certificate not up to date ("common.name.example.com"): [foobar]`, "DNS names on TLS certificate not up to date: []", @@ -129,7 +113,7 @@ func TestCertificateMatchesSpec(t *testing.T) { } { t.Run(name, func(t *testing.T) { match, errs := certificateMatchesSpec( - test.certificate, test.cb.privateKey, test.cb.cert, test.fakeLister) + test.certificate, test.cb.privateKey, test.cb.cert, secret) if match != test.expMatch { t.Errorf("got unexpected match bool, exp=%t got=%t",