From 17728b84371afa9dd36b95c9663686eda8b2783f Mon Sep 17 00:00:00 2001 From: irbekrm Date: Thu, 20 May 2021 20:23:25 +0100 Subject: [PATCH 1/5] Handle cert renewal when renewalBefore slightly less than cert duration correctly Signed-off-by: irbekrm --- .../readiness/readiness_controller.go | 3 +- .../readiness/readiness_controller_test.go | 2 +- .../certificates/trigger/policies/policies.go | 4 +- pkg/controller/certificates/util.go | 35 +++--- pkg/controller/certificates/util_test.go | 117 ++++++++++-------- 5 files changed, 87 insertions(+), 74 deletions(-) diff --git a/pkg/controller/certificates/readiness/readiness_controller.go b/pkg/controller/certificates/readiness/readiness_controller.go index 8dc89295b..2d9c2d6f7 100644 --- a/pkg/controller/certificates/readiness/readiness_controller.go +++ b/pkg/controller/certificates/readiness/readiness_controller.go @@ -169,7 +169,8 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { notBefore := metav1.NewTime(x509cert.NotBefore) notAfter := metav1.NewTime(x509cert.NotAfter) - renewalTime := c.renewalTimeCalculator(x509cert.NotBefore, x509cert.NotAfter, crt) + renewBeforeHint := crt.Spec.RenewBefore + renewalTime := c.renewalTimeCalculator(x509cert.NotBefore, x509cert.NotAfter, renewBeforeHint) //update Certificate's Status crt.Status.NotBefore = ¬Before diff --git a/pkg/controller/certificates/readiness/readiness_controller_test.go b/pkg/controller/certificates/readiness/readiness_controller_test.go index e0d9ce337..14398cddc 100644 --- a/pkg/controller/certificates/readiness/readiness_controller_test.go +++ b/pkg/controller/certificates/readiness/readiness_controller_test.go @@ -45,7 +45,7 @@ func policyEvaluatorBuilder(c cmapi.CertificateCondition) policyEvaluatorFunc { // renewalTimeBuilder returns a fake renewalTimeFunc for ReadinessController. func renewalTimeBuilder(rt *metav1.Time) certificates.RenewalTimeFunc { - return func(notBefore, notAfter time.Time, cert *cmapi.Certificate) *metav1.Time { + return func(notBefore, notAfter time.Time, cert *metav1.Duration) *metav1.Time { return rt } } diff --git a/pkg/controller/certificates/trigger/policies/policies.go b/pkg/controller/certificates/trigger/policies/policies.go index cc3a5c436..c4413376d 100644 --- a/pkg/controller/certificates/trigger/policies/policies.go +++ b/pkg/controller/certificates/trigger/policies/policies.go @@ -213,8 +213,8 @@ func CurrentCertificateNearingExpiry(c clock.Clock, defaultRenewBeforeExpiryDura notBefore := metav1.NewTime(x509cert.NotBefore) notAfter := metav1.NewTime(x509cert.NotAfter) crt := input.Certificate - renewBefore := certificates.RenewBeforeExpiryDuration(notBefore.Time, notAfter.Time, crt.Spec.RenewBefore, defaultRenewBeforeExpiryDuration) - renewalTime := metav1.NewTime(notAfter.Add(-1 * renewBefore)) + renewalTimeCalculator := certificates.RenewalTimeWrapper(defaultRenewBeforeExpiryDuration) + renewalTime := renewalTimeCalculator(notBefore.Time, notAfter.Time, crt.Spec.RenewBefore) renewIn := renewalTime.Time.Sub(c.Now()) if renewIn > 0 { diff --git a/pkg/controller/certificates/util.go b/pkg/controller/certificates/util.go index 2839442e4..187376da9 100644 --- a/pkg/controller/certificates/util.go +++ b/pkg/controller/certificates/util.go @@ -265,30 +265,27 @@ func GenerateLocallySignedTemporaryCertificate(crt *cmapi.Certificate, pkData [] return b, nil } -//RenewalTimeFunc is a custom function type for calculating renewal time of a certificate -type RenewalTimeFunc func(time.Time, time.Time, *cmapi.Certificate) *metav1.Time +//RenewalTimeFunc is a custom function type for calculating renewal time of a certificate. +type RenewalTimeFunc func(time.Time, time.Time, *metav1.Duration) *metav1.Time // RenewalTimeWrapper returns RenewalTimeFunc implementation -// TODO: potentially merge RenewBeforeExpiryDuration into this function and rewrite the tests accordingly func RenewalTimeWrapper(defaultRenewBeforeExpiryDuration time.Duration) RenewalTimeFunc { - return func(notBefore, notAfter time.Time, cert *cmapi.Certificate) *metav1.Time { - renewBefore := RenewBeforeExpiryDuration(notBefore, notAfter, cert.Spec.RenewBefore, defaultRenewBeforeExpiryDuration) + return func(notBefore, notAfter time.Time, renewBeforeHint *metav1.Duration) *metav1.Time { + + // 1. Calculate how long before expiry a cert should be renewed + renewBefore := defaultRenewBeforeExpiryDuration + if renewBeforeHint != nil { + renewBefore = renewBeforeHint.Duration + } + actualDuration := notAfter.Sub(notBefore) + // renewBefore = min(renewBefore, actualDuration/3) + if renewBefore >= (actualDuration / 3) { + renewBefore = actualDuration / 3 + } + + // 2. Calculate when a cert should be renewed rt := metav1.NewTime(notAfter.Add(-1 * renewBefore)) return &rt } } - -// RenewBeforeExpiryDuration will return the amount of time before the given -// NotAfter time that the certificate should be renewed. -func RenewBeforeExpiryDuration(notBefore, notAfter time.Time, specRenewBefore *metav1.Duration, defaultRenewBeforeExpiryDuration time.Duration) time.Duration { - renewBefore := defaultRenewBeforeExpiryDuration - if specRenewBefore != nil { - renewBefore = specRenewBefore.Duration - } - actualDuration := notAfter.Sub(notBefore) - if renewBefore >= actualDuration { - renewBefore = actualDuration / 3 - } - return renewBefore -} diff --git a/pkg/controller/certificates/util_test.go b/pkg/controller/certificates/util_test.go index d06fb6e6f..8c830c281 100644 --- a/pkg/controller/certificates/util_test.go +++ b/pkg/controller/certificates/util_test.go @@ -282,67 +282,82 @@ func selfSignCertificate(t *testing.T, spec cmapi.CertificateSpec) []byte { return pemData } -func TestRenewBeforeExpiryDuration(t *testing.T) { - type testCase struct { - notBefore time.Time - notAfter time.Time - specRenewBefore *metav1.Duration - defaultRenewBeforeExpiryDuration time.Duration - expected time.Duration +func TestRenewalTimeWrapper(t *testing.T) { + type scenario struct { + notBefore time.Time + notAfter time.Time + renewBeforeHint *metav1.Duration + defaultRenewBefore time.Duration + expectedRenewalTime *metav1.Time } now := time.Now() - tests := map[string]testCase{ - "use default": { - notBefore: now, - notAfter: now.Add(time.Hour * 24), - specRenewBefore: nil, - defaultRenewBeforeExpiryDuration: time.Hour, - expected: time.Hour, + tests := map[string]scenario{ + "no renewBeforeHint, defaultRenewBefore < (cert duration / 3)": { + notBefore: now, + notAfter: now.Add(time.Hour * 24), + renewBeforeHint: nil, + defaultRenewBefore: time.Hour, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 23)}, }, - "spec overrides default": { - notBefore: now, - notAfter: now.Add(time.Hour * 24), - specRenewBefore: &metav1.Duration{Duration: time.Hour * 2}, - defaultRenewBeforeExpiryDuration: time.Hour, - expected: time.Hour * 2, + "renewBeforeHint < (cert duration / 3)": { + notBefore: now, + notAfter: now.Add(time.Hour * 24), + renewBeforeHint: &metav1.Duration{Duration: time.Hour * 2}, + defaultRenewBefore: time.Hour, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 22)}, }, - "default larger than actual duration": { - notBefore: now, - notAfter: now.Add(time.Hour * 24), - specRenewBefore: nil, - defaultRenewBeforeExpiryDuration: time.Hour * 24 * 7, - expected: time.Hour * 8, + "no renewBeforeHint, defaultRenewBefore > (cert duration / 3)": { + notBefore: now, + notAfter: now.Add(time.Hour * 24), + renewBeforeHint: nil, + defaultRenewBefore: time.Hour * 24 * 7, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16)}, }, - "spec larger than actual duration": { - notBefore: now, - notAfter: now.Add(time.Hour * 24), - specRenewBefore: &metav1.Duration{Duration: time.Hour * 24 * 7}, - defaultRenewBeforeExpiryDuration: time.Hour, - expected: time.Hour * 8, + "renewBeforeHint > (cert duration / 3)": { + notBefore: now, + notAfter: now.Add(time.Hour * 24), + renewBeforeHint: &metav1.Duration{Duration: time.Hour * 24 * 7}, + defaultRenewBefore: time.Hour, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16)}, }, - "default equal to actual duration": { - notBefore: now, - notAfter: now.Add(time.Hour * 24), - specRenewBefore: nil, - defaultRenewBeforeExpiryDuration: time.Hour * 24, - expected: time.Hour * 8, + "no renewBeforeHint, defaultRenewBefore == cert duration": { + notBefore: now, + notAfter: now.Add(time.Hour * 24), + renewBeforeHint: nil, + defaultRenewBefore: time.Hour * 24, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16)}, }, - "spec equal to actual duration": { - notBefore: now, - notAfter: now.Add(time.Hour * 24), - specRenewBefore: &metav1.Duration{Duration: time.Hour * 24}, - defaultRenewBeforeExpiryDuration: time.Hour, - expected: time.Hour * 8, + "renewBeforeHint == cert duration": { + notBefore: now, + notAfter: now.Add(time.Hour * 24), + renewBeforeHint: &metav1.Duration{Duration: time.Hour * 24}, + defaultRenewBefore: time.Hour, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16)}, + }, + + // The following two test cases would catch the bug reported in + // https://github.com/jetstack/cert-manager/issues/3897 + "cert duration very slightly less than defaultRenewBefore": { + notBefore: now, + notAfter: now.Add(time.Hour * 24), + renewBeforeHint: nil, + defaultRenewBefore: time.Hour*24 + time.Minute, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16)}, + }, + "cert duration very slightly less than renewBeforeHint": { + notBefore: now, + notAfter: now.Add(time.Hour * 24), + renewBeforeHint: nil, + defaultRenewBefore: time.Hour*24 + time.Minute, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16)}, }, } + for n, s := range tests { + t.Run(n, func(t *testing.T) { + f := RenewalTimeWrapper(s.defaultRenewBefore) + renewalTime := f(s.notBefore, s.notAfter, s.renewBeforeHint) + assert.Equal(t, s.expectedRenewalTime, renewalTime) - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - assert.Equal( - t, - tc.expected, - RenewBeforeExpiryDuration(tc.notBefore, tc.notAfter, tc.specRenewBefore, tc.defaultRenewBeforeExpiryDuration), - ) }) } } From e1dff85cade3e4802ca3257b0e7232b552c08e9f Mon Sep 17 00:00:00 2001 From: irbekrm Date: Fri, 21 May 2021 10:24:31 +0100 Subject: [PATCH 2/5] Feedback from code review Signed-off-by: irbekrm --- pkg/controller/certificates/util_test.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/controller/certificates/util_test.go b/pkg/controller/certificates/util_test.go index 8c830c281..be3ed383b 100644 --- a/pkg/controller/certificates/util_test.go +++ b/pkg/controller/certificates/util_test.go @@ -18,6 +18,7 @@ package certificates import ( "crypto" + "fmt" "reflect" "testing" "time" @@ -337,26 +338,26 @@ func TestRenewalTimeWrapper(t *testing.T) { // The following two test cases would catch the bug reported in // https://github.com/jetstack/cert-manager/issues/3897 - "cert duration very slightly less than defaultRenewBefore": { + "cert duration very slightly more than defaultRenewBefore": { notBefore: now, - notAfter: now.Add(time.Hour * 24), + notAfter: now.Add(time.Hour*24 + time.Minute*3), renewBeforeHint: nil, - defaultRenewBefore: time.Hour*24 + time.Minute, - expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16)}, + defaultRenewBefore: time.Hour * 24, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour*16 + time.Minute*2)}, }, - "cert duration very slightly less than renewBeforeHint": { + "cert duration very slightly more than renewBeforeHint": { notBefore: now, - notAfter: now.Add(time.Hour * 24), - renewBeforeHint: nil, - defaultRenewBefore: time.Hour*24 + time.Minute, - expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16)}, + notAfter: now.Add(time.Hour*24 + time.Minute*3), + renewBeforeHint: &metav1.Duration{Duration: time.Hour * 24}, + defaultRenewBefore: time.Hour, + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour*16 + time.Minute*2)}, }, } for n, s := range tests { t.Run(n, func(t *testing.T) { f := RenewalTimeWrapper(s.defaultRenewBefore) renewalTime := f(s.notBefore, s.notAfter, s.renewBeforeHint) - assert.Equal(t, s.expectedRenewalTime, renewalTime) + assert.Equal(t, s.expectedRenewalTime, renewalTime, fmt.Sprintf("Expected renewal time: %v got: %v", s.expectedRenewalTime, renewalTime)) }) } From c67c2c4f47ae7502304c2509cfb839ec5418b872 Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Tue, 4 May 2021 14:26:32 +0100 Subject: [PATCH 3/5] static analysis: pkg/controller fixes the following issues: pkg/controller/acmeorders/util.go:84:6 deadcode `hashChallenge` is unused pkg/controller/certificaterequests/approver/approver.go:72:14 staticcheck SA4021: x = append(y) is equivalent to x = y pkg/controller/certificaterequests/vault/vault_test.go:535:21 errcheck Error return value of `controller.Register` is not checked pkg/controller/certificates/trigger/policies/policies.go:121:26 gosimple S1039: unnecessary use of fmt.Sprintf pkg/controller/clusterissuers/sync_test.go:55:12 errcheck Error return value of `c.Register` is not checked pkg/controller/ingress-shim/sync.go:301:2 gosimple S1005: unnecessary assignment to the blank identifier Signed-off-by: Ashley Davis --- pkg/controller/acmeorders/util.go | 17 ----------------- .../certificaterequests/approver/approver.go | 2 +- .../certificaterequests/vault/vault_test.go | 5 ++++- .../certificates/trigger/policies/policies.go | 2 +- pkg/controller/clusterissuers/sync_test.go | 5 ++++- pkg/controller/ingress-shim/sync.go | 2 +- pkg/controller/test/context_builder.go | 4 ++-- 7 files changed, 13 insertions(+), 24 deletions(-) diff --git a/pkg/controller/acmeorders/util.go b/pkg/controller/acmeorders/util.go index 455fb11e8..429190b0d 100644 --- a/pkg/controller/acmeorders/util.go +++ b/pkg/controller/acmeorders/util.go @@ -18,9 +18,7 @@ package acmeorders import ( "context" - "encoding/json" "fmt" - "hash/fnv" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -81,21 +79,6 @@ func buildChallenge(ctx context.Context, cl acmecl.Interface, issuer cmapi.Gener }, nil } -func hashChallenge(spec cmacme.ChallengeSpec) (uint32, error) { - specBytes, err := json.Marshal(spec) - if err != nil { - return 0, err - } - - hashF := fnv.New32() - _, err = hashF.Write(specBytes) - if err != nil { - return 0, err - } - - return hashF.Sum32(), nil -} - func challengeSpecForAuthorization(ctx context.Context, cl acmecl.Interface, issuer cmapi.GenericIssuer, o *cmacme.Order, authz cmacme.ACMEAuthorization) (*cmacme.ChallengeSpec, error) { log := logf.FromContext(ctx, "challengeSpecForAuthorization") dbg := log.V(logf.DebugLevel) diff --git a/pkg/controller/certificaterequests/approver/approver.go b/pkg/controller/certificaterequests/approver/approver.go index 7569583e1..7347d2445 100644 --- a/pkg/controller/certificaterequests/approver/approver.go +++ b/pkg/controller/certificaterequests/approver/approver.go @@ -69,7 +69,7 @@ func (c *Controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin c.queue = workqueue.NewNamedRateLimitingQueue(controllerpkg.DefaultItemBasedRateLimiter(), ControllerName) certificateRequestInformer := ctx.SharedInformerFactory.Certmanager().V1().CertificateRequests() - mustSync := append([]cache.InformerSynced{certificateRequestInformer.Informer().HasSynced}) + mustSync := []cache.InformerSynced{certificateRequestInformer.Informer().HasSynced} certificateRequestInformer.Informer().AddEventHandler(&controllerpkg.QueuingEventHandler{Queue: c.queue}) c.certificateRequestLister = certificateRequestInformer.Lister() diff --git a/pkg/controller/certificaterequests/vault/vault_test.go b/pkg/controller/certificaterequests/vault/vault_test.go index 6e51caea0..25c8adf16 100644 --- a/pkg/controller/certificaterequests/vault/vault_test.go +++ b/pkg/controller/certificaterequests/vault/vault_test.go @@ -532,7 +532,10 @@ func runTest(t *testing.T, test testT) { } controller := certificaterequests.New(apiutil.IssuerVault, vault) - controller.Register(test.builder.Context) + if _, _, err := controller.Register(test.builder.Context); err != nil { + t.Errorf("failed to register context with controller: %v", err) + } + test.builder.Start() err := controller.Sync(context.Background(), test.certificateRequest) diff --git a/pkg/controller/certificates/trigger/policies/policies.go b/pkg/controller/certificates/trigger/policies/policies.go index c4413376d..6727a4423 100644 --- a/pkg/controller/certificates/trigger/policies/policies.go +++ b/pkg/controller/certificates/trigger/policies/policies.go @@ -118,7 +118,7 @@ func SecretPublicKeysDiffer(input Input) (string, string, bool) { func SecretPrivateKeyMatchesSpec(input Input) (string, string, bool) { if input.Secret.Data == nil || len(input.Secret.Data[corev1.TLSPrivateKeyKey]) == 0 { - return SecretMismatch, fmt.Sprintf("Existing issued Secret does not contain private key data"), true + return SecretMismatch, "Existing issued Secret does not contain private key data", true } pkBytes := input.Secret.Data[corev1.TLSPrivateKeyKey] diff --git a/pkg/controller/clusterissuers/sync_test.go b/pkg/controller/clusterissuers/sync_test.go index f4065aace..95ab254f7 100644 --- a/pkg/controller/clusterissuers/sync_test.go +++ b/pkg/controller/clusterissuers/sync_test.go @@ -52,7 +52,10 @@ func TestUpdateIssuerStatus(t *testing.T) { defer b.Stop() c := &controller{} - c.Register(b.Context) + if _, _, err := c.Register(b.Context); err != nil { + t.Errorf("failed to register context against controller: %v", err) + return + } b.Start() fakeClient := b.FakeCMClient() diff --git a/pkg/controller/ingress-shim/sync.go b/pkg/controller/ingress-shim/sync.go index ecdbee7b2..000bf0a9d 100644 --- a/pkg/controller/ingress-shim/sync.go +++ b/pkg/controller/ingress-shim/sync.go @@ -298,7 +298,7 @@ func setIssuerSpecificConfig(crt *cmapi.Certificate, ing *networkingv1beta1.Ingr } // for ACME issuers - editInPlaceVal, _ := ingAnnotations[cmacme.IngressEditInPlaceAnnotationKey] + editInPlaceVal := ingAnnotations[cmacme.IngressEditInPlaceAnnotationKey] editInPlace := editInPlaceVal == "true" if editInPlace { if crt.Annotations == nil { diff --git a/pkg/controller/test/context_builder.go b/pkg/controller/test/context_builder.go index d3928f340..a9b44f785 100644 --- a/pkg/controller/test/context_builder.go +++ b/pkg/controller/test/context_builder.go @@ -45,8 +45,8 @@ import ( func init() { logs.InitLogs(nil) - flag.Set("alsologtostderr", fmt.Sprintf("%t", true)) - flag.Lookup("v").Value.Set("4") + _ = flag.Set("alsologtostderr", fmt.Sprintf("%t", true)) + _ = flag.Lookup("v").Value.Set("4") } // Builder is a structure used to construct new Contexts for use during tests. From 333af8fd944d47bcc0a700c3e3f4074d25943a85 Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Tue, 4 May 2021 15:05:23 +0100 Subject: [PATCH 4/5] further static check fixes pkg/internal/apis/certmanager/validation/certificate_for_issuer_test.go:34:2 deadcode `defaultTestCrtName` is unused pkg/issuer/acme/dns/rfc2136/provider_test.go:42:23 errcheck Error return value of `server.Shutdown` is not checked pkg/issuer/acme/dns/rfc2136/provider_test.go:77:23 errcheck Error return value of `server.Shutdown` is not checked pkg/issuer/vault/setup.go:37:2 deadcode `messageVaultHealthCheckFailed` is unused pkg/issuer/venafi/client/request.go:143:5 gosimple S1023: redundant break statement pkg/logs/logs.go:68:8 errcheck Error return value of `fs.Set` is not checked the following fixes introduce a panic when the returned error is non-nil, which could be a breaking change but was deemed to be worth it pkg/webhook/server/server.go:58:30 errcheck Error return value is not checked pkg/webhook/server/server.go:59:25 errcheck Error return value is not checked Signed-off-by: Ashley Davis --- pkg/ctl/scheme.go | 4 ++-- .../validation/certificate_for_issuer_test.go | 1 - pkg/issuer/acme/dns/rfc2136/provider_test.go | 12 ++++++++++-- pkg/issuer/vault/setup.go | 1 - pkg/issuer/venafi/client/request.go | 1 - pkg/logs/logs.go | 2 +- pkg/webhook/server/BUILD.bazel | 1 + pkg/webhook/server/server.go | 8 +++++--- 8 files changed, 19 insertions(+), 11 deletions(-) diff --git a/pkg/ctl/scheme.go b/pkg/ctl/scheme.go index 503150b0f..4c2f37a29 100644 --- a/pkg/ctl/scheme.go +++ b/pkg/ctl/scheme.go @@ -58,13 +58,13 @@ func init() { utilruntime.Must(metainternalversion.AddToScheme(Scheme)) // Adds the conversion between internalmeta.List and corev1.List - Scheme.AddConversionFunc((*corev1.List)(nil), (*metainternalversion.List)(nil), func(a, b interface{}, scope conversion.Scope) error { + _ = Scheme.AddConversionFunc((*corev1.List)(nil), (*metainternalversion.List)(nil), func(a, b interface{}, scope conversion.Scope) error { metaList := &metav1.List{} metaList.Items = a.(*corev1.List).Items return metainternalversion.Convert_v1_List_To_internalversion_List(metaList, b.(*metainternalversion.List), scope) }) - Scheme.AddConversionFunc((*metainternalversion.List)(nil), (*corev1.List)(nil), func(a, b interface{}, scope conversion.Scope) error { + _ = Scheme.AddConversionFunc((*metainternalversion.List)(nil), (*corev1.List)(nil), func(a, b interface{}, scope conversion.Scope) error { metaList := &metav1.List{} err := metainternalversion.Convert_internalversion_List_To_v1_List(a.(*metainternalversion.List), metaList, scope) if err != nil { diff --git a/pkg/internal/apis/certmanager/validation/certificate_for_issuer_test.go b/pkg/internal/apis/certmanager/validation/certificate_for_issuer_test.go index 6f739c14c..54d0f2968 100644 --- a/pkg/internal/apis/certmanager/validation/certificate_for_issuer_test.go +++ b/pkg/internal/apis/certmanager/validation/certificate_for_issuer_test.go @@ -31,7 +31,6 @@ import ( const ( defaultTestIssuerName = "test-issuer" - defaultTestCrtName = "test-crt" defaultTestNamespace = gen.DefaultTestNamespace ) diff --git a/pkg/issuer/acme/dns/rfc2136/provider_test.go b/pkg/issuer/acme/dns/rfc2136/provider_test.go index de10a2a3d..613023f34 100644 --- a/pkg/issuer/acme/dns/rfc2136/provider_test.go +++ b/pkg/issuer/acme/dns/rfc2136/provider_test.go @@ -39,7 +39,11 @@ func TestRunSuiteWithTSIG(t *testing.T) { if err := server.Run(ctx); err != nil { t.Fatalf("failed to start test server: %v", err) } - defer server.Shutdown() + defer func() { + if err := server.Shutdown(); err != nil { + t.Errorf("failed to gracefully shut down test server: %v", err) + } + }() var validConfig = cmacme.ACMEIssuerDNS01ProviderRFC2136{ Nameserver: server.ListenAddr(), @@ -74,7 +78,11 @@ func TestRunSuiteNoTSIG(t *testing.T) { if err := server.Run(ctx); err != nil { t.Fatalf("failed to start test server: %v", err) } - defer server.Shutdown() + defer func() { + if err := server.Shutdown(); err != nil { + t.Errorf("failed to gracefully shut down test server: %v", err) + } + }() var validConfig = cmacme.ACMEIssuerDNS01ProviderRFC2136{ Nameserver: server.ListenAddr(), diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go index 2f5fc19e3..235d70e38 100644 --- a/pkg/issuer/vault/setup.go +++ b/pkg/issuer/vault/setup.go @@ -34,7 +34,6 @@ const ( errorVault = "VaultError" messageVaultClientInitFailed = "Failed to initialize Vault client: " - messageVaultHealthCheckFailed = "Failed to call Vault health check: " messageVaultStatusVerificationFailed = "Vault is not initialized or is sealed" messageVaultConfigRequired = "Vault config cannot be empty" messageServerAndPathRequired = "Vault server and path are required fields" diff --git a/pkg/issuer/venafi/client/request.go b/pkg/issuer/venafi/client/request.go index f7272ff3d..be8952e50 100644 --- a/pkg/issuer/venafi/client/request.go +++ b/pkg/issuer/venafi/client/request.go @@ -140,7 +140,6 @@ func convertCustomFieldsToVcert(customFields []api.CustomField) ([]certificate.C switch field.Type { case api.CustomFieldTypePlain, "": fieldType = certificate.CustomFieldPlain - break default: return nil, ErrCustomFieldsType{Type: field.Type} } diff --git a/pkg/logs/logs.go b/pkg/logs/logs.go index c17955c86..194e58b1e 100644 --- a/pkg/logs/logs.go +++ b/pkg/logs/logs.go @@ -65,7 +65,7 @@ func InitLogs(fs *flag.FlagSet) { fs = flag.CommandLine } klog.InitFlags(fs) - fs.Set("logtostderr", "true") + _ = fs.Set("logtostderr", "true") log.SetOutput(GlogWriter{}) log.SetFlags(0) diff --git a/pkg/webhook/server/BUILD.bazel b/pkg/webhook/server/BUILD.bazel index d09b9a4e1..8467ed363 100644 --- a/pkg/webhook/server/BUILD.bazel +++ b/pkg/webhook/server/BUILD.bazel @@ -21,6 +21,7 @@ go_library( "@io_k8s_apimachinery//pkg/runtime:go_default_library", "@io_k8s_apimachinery//pkg/runtime/schema:go_default_library", "@io_k8s_apimachinery//pkg/runtime/serializer/json:go_default_library", + "@io_k8s_apimachinery//pkg/util/runtime:go_default_library", "@io_k8s_component_base//cli/flag:go_default_library", "@io_k8s_sigs_controller_runtime//pkg/log:go_default_library", ], diff --git a/pkg/webhook/server/server.go b/pkg/webhook/server/server.go index 4e9053364..6c46c2d3e 100644 --- a/pkg/webhook/server/server.go +++ b/pkg/webhook/server/server.go @@ -36,6 +36,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer/json" + runtimeutil "k8s.io/apimachinery/pkg/util/runtime" ciphers "k8s.io/component-base/cli/flag" crlog "sigs.k8s.io/controller-runtime/pkg/log" @@ -55,8 +56,9 @@ var ( func init() { apiextensionsinstall.Install(defaultScheme) - admissionv1beta1.AddToScheme(defaultScheme) - admissionv1.AddToScheme(defaultScheme) + + runtimeutil.Must(admissionv1beta1.AddToScheme(defaultScheme)) + runtimeutil.Must(admissionv1.AddToScheme(defaultScheme)) // we need to add the options to empty v1 // TODO fix the server code to avoid this @@ -207,6 +209,7 @@ func (s *Server) Run(stopCh <-chan struct{}) error { s.Log.V(logf.DebugLevel).Info("waiting for server to shutdown") waitForAll(healthzChan, certSourceChan, listenerChan) + s.Log.V(logf.InfoLevel).Info("server shutdown successfully") return err @@ -377,7 +380,6 @@ func (s *Server) handle(inner handleFunc) func(w http.ResponseWriter, req *http. codec := json.NewSerializerWithOptions(json.DefaultMetaFactory, s.scheme(), s.scheme(), json.SerializerOptions{ Pretty: true, }) - codec.Decode(data, nil, nil) obj, _, err := codec.Decode(data, nil, nil) if err != nil { s.Log.Error(err, "failed to decode request body") From 219a620871aff2457457591403021d7e72612198 Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Thu, 20 May 2021 10:09:13 +0100 Subject: [PATCH 5/5] static analysis fixes pkg/ctl/scheme.go:17:1: package comment should be of the form "Package ctl ..." pkg/issuer/acme/dns/acmedns/acmedns.go:43:2: var accountJson should be accountJSON pkg/issuer/acme/dns/acmedns/acmedns.go:50:43: func parameter accountJson should be accountJSON pkg/controller/certificates/trigger/policies/policies.go:57:1: comment on exported type Chain should be of the form "Chain ..." (with optional leading article) pkg/controller/ingress-shim/sync.go:36:2: package "github.com/jetstack/cert-manager/pkg/logs" is being imported more than once (ST1019) pkg/controller/ingress-shim/sync.go:37:2: other import of "github.com/jetstack/cert-manager/pkg/logs" Signed-off-by: Ashley Davis --- .../certificates/trigger/policies/policies.go | 2 +- pkg/controller/ingress-shim/sync.go | 5 ++--- pkg/ctl/scheme.go | 3 ++- pkg/issuer/acme/dns/acmedns/acmedns.go | 10 +++++----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/controller/certificates/trigger/policies/policies.go b/pkg/controller/certificates/trigger/policies/policies.go index 6727a4423..73e0d2016 100644 --- a/pkg/controller/certificates/trigger/policies/policies.go +++ b/pkg/controller/certificates/trigger/policies/policies.go @@ -54,7 +54,7 @@ type Input struct { // in the 'reason' and 'message' return parameters if so. type Func func(Input) (reason, message string, reissue bool) -// A chain of PolicyFuncs to be evaluated in order. +// A Chain of PolicyFuncs to be evaluated in order. type Chain []Func // Evaluate will evaluate the entire policy chain using the provided input. diff --git a/pkg/controller/ingress-shim/sync.go b/pkg/controller/ingress-shim/sync.go index 000bf0a9d..a3b18da94 100644 --- a/pkg/controller/ingress-shim/sync.go +++ b/pkg/controller/ingress-shim/sync.go @@ -33,7 +33,6 @@ import ( cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" - "github.com/jetstack/cert-manager/pkg/logs" logf "github.com/jetstack/cert-manager/pkg/logs" utilerrors "k8s.io/apimachinery/pkg/util/errors" ) @@ -143,7 +142,7 @@ func validateIngressTLSBlock(tlsBlock networkingv1beta1.IngressTLS) []error { func (c *controller) buildCertificates(ctx context.Context, ing *networkingv1beta1.Ingress, issuerName, issuerKind, issuerGroup string) (new, update []*cmapi.Certificate, _ error) { - log := logs.FromContext(ctx) + log := logf.FromContext(ctx) var newCrts []*cmapi.Certificate var updateCrts []*cmapi.Certificate @@ -187,7 +186,7 @@ func (c *controller) buildCertificates(ctx context.Context, ing *networkingv1bet // check if a Certificate for this TLS entry already exists, and if it // does then skip this entry if existingCrt != nil { - log := logs.WithRelatedResource(log, existingCrt) + log := logf.WithRelatedResource(log, existingCrt) log.V(logf.DebugLevel).Info("certificate already exists for ingress resource, ensuring it is up to date") if metav1.GetControllerOf(existingCrt) == nil { diff --git a/pkg/ctl/scheme.go b/pkg/ctl/scheme.go index 4c2f37a29..05f9e0358 100644 --- a/pkg/ctl/scheme.go +++ b/pkg/ctl/scheme.go @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -// This package was created to have a scheme that has the internal cert-manager types, +// Package ctl was created to have a scheme that has the internal cert-manager types, // and their conversion functions as well as the List object type registered, which is needed for ctl command like // `convert` or `create certificaterequest`. + package ctl import ( diff --git a/pkg/issuer/acme/dns/acmedns/acmedns.go b/pkg/issuer/acme/dns/acmedns/acmedns.go index 1b53b3b13..868d3a388 100644 --- a/pkg/issuer/acme/dns/acmedns/acmedns.go +++ b/pkg/issuer/acme/dns/acmedns/acmedns.go @@ -40,19 +40,19 @@ type DNSProvider struct { // Credentials and acme-dns server host are given in environment variables func NewDNSProvider(dns01Nameservers []string) (*DNSProvider, error) { host := os.Getenv("ACME_DNS_HOST") - accountJson := os.Getenv("ACME_DNS_ACCOUNT_JSON") - return NewDNSProviderHostBytes(host, []byte(accountJson), dns01Nameservers) + accountJSON := os.Getenv("ACME_DNS_ACCOUNT_JSON") + return NewDNSProviderHostBytes(host, []byte(accountJSON), dns01Nameservers) } // NewDNSProviderHostBytes returns a DNSProvider instance configured for ACME DNS // acme-dns server host is given in a string // credentials are stored in json in the given string -func NewDNSProviderHostBytes(host string, accountJson []byte, dns01Nameservers []string) (*DNSProvider, error) { +func NewDNSProviderHostBytes(host string, accountJSON []byte, dns01Nameservers []string) (*DNSProvider, error) { client := goacmedns.NewClient(host) var accounts map[string]goacmedns.Account - if err := json.Unmarshal(accountJson, &accounts); err != nil { - return nil, fmt.Errorf("Error unmarshalling accountJson: %s", err) + if err := json.Unmarshal(accountJSON, &accounts); err != nil { + return nil, fmt.Errorf("Error unmarshalling accountJSON: %s", err) } return &DNSProvider{