Merge pull request #364 from SgtCoDFish/backoff-startup

Add retries for issuing initial serving cert

Author: cert-manager-prow[bot]

go.mod

@@ -10,6 +10,7 @@ require (
 	github.com/fsnotify/fsnotify v1.7.0
 	github.com/go-logr/logr v1.4.2
 	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
+	github.com/lestrrat-go/backoff/v2 v2.0.8
 	github.com/onsi/ginkgo/v2 v2.19.0
 	github.com/onsi/gomega v1.33.1
 	github.com/prometheus/client_golang v1.19.1
@@ -80,7 +81,6 @@ require (
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/josharian/intern v1.0.0 // indirect
 	github.com/json-iterator/go v1.1.12 // indirect
-	github.com/lestrrat-go/backoff/v2 v2.0.8 // indirect
 	github.com/lestrrat-go/blackmagic v1.0.2 // indirect
 	github.com/lestrrat-go/httpcc v1.0.1 // indirect
 	github.com/lestrrat-go/iter v1.0.2 // indirect

pkg/certmanager/certmanager.go

@@ -82,12 +82,11 @@ type IssuerChangeNotifier interface {
 	// as updates happen.
 	SubscribeIssuerChange() <-chan *cmmeta.ObjectReference
 
-	// MustWaitForIssuer returns true if there's no "default" issuerRef available
-	// (i.e. no static issuerRef was configured at startup)
+	// HasIssuerConfig returns true if there's a configured active issuer ref.
+	// (i.e. a static issuerRef was configured at startup / runtime issuance config has been successfully acquired)
 	// If this function returns true, InitialIssuer will always return nil and
-	// subscribers must wait for runtime configuration before trying to issue
-	// certificates
-	MustWaitForIssuer() bool
+	// subscribers must wait for runtime configuration before trying to issue certificates
+	HasIssuerConfig() bool
 
 	// InitialIssuer returns the "static" issuer which was configured at startup. Will
 	// always return nil if no such issuer exists.
@@ -475,9 +474,8 @@ func (m *manager) SubscribeIssuerChange() <-chan *cmmeta.ObjectReference {
 	return ch
 }
 
-func (m *manager) MustWaitForIssuer() bool {
-	// if no originalIssuerRef was configured, must wait for runtime configuration
-	return m.originalIssuerRef == nil
+func (m *manager) HasIssuerConfig() bool {
+	return m.activeIssuerRef != nil
 }
 
 func (m *manager) InitialIssuer() *cmmeta.ObjectReference {

pkg/tls/tls.go

@@ -32,6 +32,7 @@ import (
 	cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
 	"github.com/cert-manager/cert-manager/pkg/util/pki"
 	"github.com/go-logr/logr"
+	"github.com/lestrrat-go/backoff/v2"
 	"github.com/prometheus/client_golang/prometheus"
 	"istio.io/istio/pkg/spiffe"
 	pkiutil "istio.io/istio/security/pkg/pki/util"
@@ -164,17 +165,57 @@ func (p *Provider) Start(ctx context.Context) error {
 		}()
 	}
 
-	// If using pure runtime configuration (i.e. no issuerRef provided on startup)
-	// we need to wait for an issuer to be available before we try to fetch the initial
-	// serving certificate
+	var notAfter time.Time
 
-	if p.issuerChangeNotifier.MustWaitForIssuer() {
-		p.waitForInitialIssuer(ctx)
-	}
+	backoffPolicy := backoff.Exponential(
+		backoff.WithMinInterval(time.Second),
+		backoff.WithMaxInterval(time.Second*30),
+		backoff.WithJitterFactor(0.05),
+		backoff.WithMaxRetries(250),
+	)
 
-	notAfter, err := p.fetchCertificate(ctx)
-	if err != nil {
-		return fmt.Errorf("failed to fetch initial serving certificate: %w", err)
+	backoffController := backoffPolicy.Start(ctx)
+
+	for backoff.Continue(backoffController) {
+		// If using pure runtime configuration (i.e. no issuerRef provided on startup) we might need
+		// to wait for an issuer to be available before we try to fetch the initial serving certificate.
+
+		// We also need to wait to be able to actually successfully issue a certificate; if the
+		// user is installing istio-csr and cert-manager at the same time and has already provisioned
+		// a ConfigMap with runtime configuration before installing either, it's very possible that
+		// the issuer the ConfigMap refers to doesn't yet exist, even though the issuer config exists
+
+		if !p.issuerChangeNotifier.HasIssuerConfig() {
+			p.waitForInitialIssuer(ctx)
+		}
+
+		// We have an issuerRef now (after waitForInitialIssuer) but we don't know that it's valid
+		// since the issuer might not have been created yet; so we try, with a timeout, to fetch a
+		// cert and if it fails we'll hit the backoff and try again
+
+		issuanceTimeout := 10 * time.Second
+
+		fetchCtx, cancelFunc := context.WithTimeout(ctx, issuanceTimeout)
+
+		var err error
+		notAfter, err = p.fetchCertificate(fetchCtx)
+		if err != nil {
+			cancelFunc()
+
+			// Some of the functions which block in fetchCertificate don't wrap errors and we won't be able
+			// to reliably confirm DeadlineExceeded was the actual error. But we can try and later the underlying
+			// libraries might improve!
+			if errors.Is(err, context.DeadlineExceeded) {
+				p.log.Info(fmt.Sprintf("initial serving certificate didn't complete in %s; will retry", issuanceTimeout))
+			} else {
+				p.log.Info(fmt.Sprintf("failed to fetch initial serving certificate in %s: %s; will retry", issuanceTimeout, err))
+			}
+
+			continue
+		}
+
+		cancelFunc()
+		break
 	}
 
 	p.log.Info("fetched initial serving certificate")
@@ -212,8 +253,7 @@ func (p *Provider) Start(ctx context.Context) error {
 	}
 }
 
-// waitForInitialIssuer blocks until there's an issuer provided for creating the initial
-// serving cert.
+// waitForInitialIssuer blocks until there's an issuer provided for creating the initial serving cert.
 func (p *Provider) waitForInitialIssuer(ctx context.Context) {
 	for {
 		timer := time.NewTimer(5 * time.Second)
@@ -427,7 +467,8 @@ func (p *Provider) TrustDomain() string {
 	return p.opts.TrustDomain
 }
 
-// All istio-csr's need renewed service serving certificates.
+// All istio-csr pods need up-to-date serving certs to minimise the delay when a non-leader pod
+// takes leadership.
 func (p *Provider) NeedLeaderElection() bool {
 	return false
 }