refresh certs if needed in reconciler (#81)

* refresh certs if needed in reconciler

Signed-off-by: Alex Pana <[email protected]>
Co-authored-by: Rita Zhang <[email protected]>

Author: acpana

pkg/rotator/rotator.go

@@ -35,37 +35,39 @@ import (
 )
 
 const (
-	certName               = "tls.crt"
-	keyName                = "tls.key"
-	caCertName             = "ca.crt"
-	caKeyName              = "ca.key"
-	rotationCheckFrequency = 12 * time.Hour
-	certValidityDuration   = 10 * 365 * 24 * time.Hour
-	lookaheadInterval      = 90 * 24 * time.Hour
+	certName                    = "tls.crt"
+	keyName                     = "tls.key"
+	caCertName                  = "ca.crt"
+	caKeyName                   = "ca.key"
+	rotationCheckFrequency      = 12 * time.Hour
+	defaultCertValidityDuration = 10 * 365 * 24 * time.Hour
+	lookaheadInterval           = 90 * 24 * time.Hour
 )
 
 var crLog = logf.Log.WithName("cert-rotation")
 
-// WebhookType it the type of webhook, either validating/mutating webhook, a CRD conversion webhook, or an extension API server
+// WebhookType it the type of webhook, either validating/mutating webhook, a CRD conversion webhook, or an extension API server.
 type WebhookType int
 
 const (
-	//ValidatingWebhook indicates the webhook is a ValidatingWebhook
+	// ValidatingWebhook indicates the webhook is a ValidatingWebhook.
 	Validating WebhookType = iota
-	//MutingWebhook indicates the webhook is a MutatingWebhook
+	// MutingWebhook indicates the webhook is a MutatingWebhook.
 	Mutating
-	//CRDConversionWebhook indicates the webhook is a conversion webhook
+	// CRDConversionWebhook indicates the webhook is a conversion webhook.
 	CRDConversion
-	//APIServiceWebhook indicates the webhook is an extension API server
+	// APIServiceWebhook indicates the webhook is an extension API server.
 	APIService
-	//ExternalDataProvider indicates the webhook is a Gatekeeper External Data Provider
+	// ExternalDataProvider indicates the webhook is a Gatekeeper External Data Provider.
 	ExternalDataProvider
 )
 
-var _ manager.Runnable = &CertRotator{}
-var _ manager.LeaderElectionRunnable = &CertRotator{}
-var _ manager.Runnable = controllerWrapper{}
-var _ manager.LeaderElectionRunnable = controllerWrapper{}
+var (
+	_ manager.Runnable               = &CertRotator{}
+	_ manager.LeaderElectionRunnable = &CertRotator{}
+	_ manager.Runnable               = controllerWrapper{}
+	_ manager.LeaderElectionRunnable = controllerWrapper{}
+)
 
 type controllerWrapper struct {
 	controller.Controller
@@ -76,9 +78,9 @@ func (cw controllerWrapper) NeedLeaderElection() bool {
 	return cw.needLeaderElection
 }
 
-// WebhookInfo is used by the rotator to receive info about resources to be updated with certificates
+// WebhookInfo is used by the rotator to receive info about resources to be updated with certificates.
 type WebhookInfo struct {
-	//Name is the name of the webhook for a validating or mutating webhook, or the CRD name in case of a CRD conversion webhook
+	// Name is the name of the webhook for a validating or mutating webhook, or the CRD name in case of a CRD conversion webhook
 	Name string
 	Type WebhookType
 }
@@ -114,19 +116,22 @@ func AddRotator(mgr manager.Manager, cr *CertRotator) error {
 	cr.certsNotMounted = make(chan struct{})
 	cr.wasCAInjected = atomic.NewBool(false)
 	cr.caNotInjected = make(chan struct{})
-	if err := mgr.Add(cr); err != nil {
-		return err
+	if !cr.testNoBackgroundRotation {
+		if err := mgr.Add(cr); err != nil {
+			return err
+		}
 	}
 
 	reconciler := &ReconcileWH{
-		cache:              cache,
-		writer:             mgr.GetClient(), // TODO
-		scheme:             mgr.GetScheme(),
-		ctx:                context.Background(),
-		secretKey:          cr.SecretKey,
-		wasCAInjected:      cr.wasCAInjected,
-		webhooks:           cr.Webhooks,
-		needLeaderElection: cr.RequireLeaderElection,
+		cache:                       cache,
+		writer:                      mgr.GetClient(), // TODO
+		scheme:                      mgr.GetScheme(),
+		ctx:                         context.Background(),
+		secretKey:                   cr.SecretKey,
+		wasCAInjected:               cr.wasCAInjected,
+		webhooks:                    cr.Webhooks,
+		needLeaderElection:          cr.RequireLeaderElection,
+		refreshCertIfNeededDelegate: cr.refreshCertIfNeeded,
 	}
 	if err := addController(mgr, reconciler); err != nil {
 		return err
@@ -186,6 +191,12 @@ type CertRotator struct {
 	certsNotMounted chan struct{}
 	wasCAInjected   *atomic.Bool
 	caNotInjected   chan struct{}
+
+	// testNoBackgroundRotation doesn't actually start the rotator in the background.
+	// This should only be used for testing.
+	testNoBackgroundRotation bool
+	// caCertDuration sets how long a CA cert will be valid for.
+	caCertDuration time.Duration
 }
 
 func (cr *CertRotator) NeedLeaderElection() bool {
@@ -204,12 +215,15 @@ func (cr *CertRotator) Start(ctx context.Context) error {
 	if cr.ExtKeyUsages == nil {
 		cr.ExtKeyUsages = &[]x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}
 	}
+	if cr.caCertDuration == time.Duration(0) {
+		cr.caCertDuration = defaultCertValidityDuration
+	}
 
 	// explicitly rotate on the first round so that the certificate
 	// can be bootstrapped, otherwise manager exits before a cert can be written
 	crLog.Info("starting cert rotator controller")
 	defer crLog.Info("stopping cert rotator controller")
-	if err := cr.refreshCertIfNeeded(); err != nil {
+	if _, err := cr.refreshCertIfNeeded(); err != nil {
 		crLog.Error(err, "could not refresh cert on startup")
 		return err
 	}
@@ -224,7 +238,7 @@ tickerLoop:
 	for {
 		select {
 		case <-ticker.C:
-			if err := cr.refreshCertIfNeeded(); err != nil {
+			if _, err := cr.refreshCertIfNeeded(); err != nil {
 				crLog.Error(err, "error rotating certs")
 			}
 		case <-ctx.Done():
@@ -240,8 +254,11 @@ tickerLoop:
 	return nil
 }
 
-// refreshCertIfNeeded returns whether there's any error when refreshing the certs if needed.
-func (cr *CertRotator) refreshCertIfNeeded() error {
+// refreshCertIfNeeded returns true if the CA was rotated
+// and if there's any error when rotating the CA or refreshing the certs.
+func (cr *CertRotator) refreshCertIfNeeded() (bool, error) {
+	var rotatedCA bool
+
 	refreshFn := func() (bool, error) {
 		secret := &corev1.Secret{}
 		if err := cr.reader.Get(context.Background(), cr.SecretKey, secret); err != nil {
@@ -253,6 +270,7 @@ func (cr *CertRotator) refreshCertIfNeeded() error {
 				crLog.Error(err, "could not refresh CA and server certs")
 				return false, nil
 			}
+			rotatedCA = true
 			crLog.Info("server certs refreshed")
 			if cr.RestartOnSecretRefresh {
 				crLog.Info("Secrets have been updated; exiting so pod can be restarted (This behaviour can be changed with the option RestartOnSecretRefresh)")
@@ -283,16 +301,16 @@ func (cr *CertRotator) refreshCertIfNeeded() error {
 		Jitter:   1,
 		Steps:    10,
 	}, refreshFn); err != nil {
-		return err
+		return rotatedCA, err
 	}
-	return nil
+	return rotatedCA, nil
 }
 
 func (cr *CertRotator) refreshCerts(refreshCA bool, secret *corev1.Secret) error {
 	var caArtifacts *KeyPairArtifacts
 	now := time.Now()
 	begin := now.Add(-1 * time.Hour)
-	end := now.Add(certValidityDuration)
+	end := now.Add(cr.caCertDuration)
 	if refreshCA {
 		var err error
 		caArtifacts, err = cr.CreateCACert(begin, end)
@@ -458,7 +476,7 @@ func buildArtifactsFromSecret(secret *corev1.Secret) (*KeyPairArtifacts, error)
 }
 
 // CreateCACert creates the self-signed CA cert and private key that will
-// be used to sign the server certificate
+// be used to sign the server certificate.
 func (cr *CertRotator) CreateCACert(begin, end time.Time) (*KeyPairArtifacts, error) {
 	templ := &x509.Certificate{
 		SerialNumber: big.NewInt(0),
@@ -496,7 +514,7 @@ func (cr *CertRotator) CreateCACert(begin, end time.Time) (*KeyPairArtifacts, er
 }
 
 // CreateCertPEM takes the results of CreateCACert and uses it to create the
-// PEM-encoded public certificate and private key, respectively
+// PEM-encoded public certificate and private key, respectively.
 func (cr *CertRotator) CreateCertPEM(ca *KeyPairArtifacts, begin, end time.Time) ([]byte, []byte, error) {
 	dnsNames := []string{cr.DNSName}
 	dnsNames = append(dnsNames, cr.ExtraDNSNames...)
@@ -527,7 +545,7 @@ func (cr *CertRotator) CreateCertPEM(ca *KeyPairArtifacts, begin, end time.Time)
 	return certPEM, keyPEM, nil
 }
 
-// pemEncode takes a certificate and encodes it as PEM
+// pemEncode takes a certificate and encodes it as PEM.
 func pemEncode(certificateDER []byte, key *rsa.PrivateKey) ([]byte, []byte, error) {
 	certBuf := &bytes.Buffer{}
 	if err := pem.Encode(certBuf, &pem.Block{Type: "CERTIFICATE", Bytes: certificateDER}); err != nil {
@@ -620,7 +638,7 @@ func reconcileSecretAndWebhookMapFunc(webhook WebhookInfo, r *ReconcileWH) func(
 	}
 }
 
-// add adds a new Controller to mgr with r as the reconcile.Reconciler
+// add adds a new Controller to mgr with r as the reconcile.Reconciler.
 func addController(mgr manager.Manager, r *ReconcileWH) error {
 	// Create a new controller
 	c, err := controller.NewUnmanaged("cert-rotator", mgr, controller.Options{Reconciler: r})
@@ -657,20 +675,21 @@ func addController(mgr manager.Manager, r *ReconcileWH) error {
 var _ reconcile.Reconciler = &ReconcileWH{}
 
 // ReconcileWH reconciles a validatingwebhookconfiguration, making sure it
-// has the appropriate CA cert
+// has the appropriate CA cert.
 type ReconcileWH struct {
-	writer             client.Writer
-	cache              cache.Cache
-	scheme             *runtime.Scheme
-	ctx                context.Context
-	secretKey          types.NamespacedName
-	webhooks           []WebhookInfo
-	wasCAInjected      *atomic.Bool
-	needLeaderElection bool
+	writer                      client.Writer
+	cache                       cache.Cache
+	scheme                      *runtime.Scheme
+	ctx                         context.Context
+	secretKey                   types.NamespacedName
+	webhooks                    []WebhookInfo
+	wasCAInjected               *atomic.Bool
+	needLeaderElection          bool
+	refreshCertIfNeededDelegate func() (bool, error)
 }
 
 // Reconcile reads that state of the cluster for a validatingwebhookconfiguration
-// object and makes sure the most recent CA cert is included
+// object and makes sure the most recent CA cert is included.
 func (r *ReconcileWH) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) {
 	if request.NamespacedName != r.secretKey {
 		return reconcile.Result{}, nil
@@ -692,6 +711,19 @@ func (r *ReconcileWH) Reconcile(ctx context.Context, request reconcile.Request)
 	}
 
 	if secret.GetDeletionTimestamp().IsZero() {
+		if r.refreshCertIfNeededDelegate != nil {
+			rotatedCA, err := r.refreshCertIfNeededDelegate()
+			if err != nil {
+				crLog.Error(err, "error rotating certs on secret reconcile")
+				return reconcile.Result{}, err
+			}
+
+			// if we did rotate the CA, the secret is stale so let's return
+			if rotatedCA {
+				return reconcile.Result{}, nil
+			}
+		}
+
 		artifacts, err := buildArtifactsFromSecret(secret)
 		if err != nil {
 			crLog.Error(err, "secret is not well-formed, cannot update webhook configurations")

pkg/rotator/rotator_suite_test.go

@@ -36,7 +36,7 @@ func TestMain(m *testing.M) {
 	os.Exit(code)
 }
 
-// StartTestManager adds recFn
+// StartTestManager adds recFn.
 func StartTestManager(ctx context.Context, mgr manager.Manager, g *gomega.GomegaWithT) *sync.WaitGroup {
 	wg := &sync.WaitGroup{}
 	wg.Add(1)