Use duration and renewBefore to control API cert rotation

These configuration knobs are much more human-understandable than the
previous percentage-based threshold flag.

We now allow users to set the lifetime of the serving cert via a ConfigMap.
Previously this was hardcoded to 1 year.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

internal/controller/apicerts/certs_expirer.go

@@ -28,24 +28,20 @@ type certsExpirerController struct {
 	k8sClient      kubernetes.Interface
 	secretInformer corev1informers.SecretInformer
 
-	// ageThreshold is a percentage (i.e., a real number between 0 and 1,
-	// inclusive) indicating the point in a certificate's lifetime where this
-	// controller will start to try to rotate it.
-	//
-	// Said another way, once ageThreshold % of a certificate's lifetime has
-	// passed, this controller will try to delete it to force a new certificate
-	// to be created.
-	ageThreshold float32
+	// renewBefore is the amount of time after the cert's issuance where
+	// this controller will start to try to rotate it.
+	renewBefore time.Duration
 }
 
 // NewCertsExpirerController returns a controller.Controller that will delete a
-// CA once it gets within some threshold of its expiration time.
+// certificate secret once it gets within some threshold of its expiration time. The
+// deletion forces rotation of the secret with the help of other controllers.
 func NewCertsExpirerController(
 	namespace string,
 	k8sClient kubernetes.Interface,
 	secretInformer corev1informers.SecretInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
-	ageThreshold float32,
+	renewBefore time.Duration,
 ) controller.Controller {
 	return controller.New(
 		controller.Config{
@@ -54,7 +50,7 @@ func NewCertsExpirerController(
 				namespace:      namespace,
 				k8sClient:      k8sClient,
 				secretInformer: secretInformer,
-				ageThreshold:   ageThreshold,
+				renewBefore:    renewBefore,
 			},
 		},
 		withInformer(
@@ -77,20 +73,19 @@ func (c *certsExpirerController) Sync(ctx controller.Context) error {
 		return nil
 	}
 
-	notBefore, notAfter, err := getCABounds(secret)
+	notBefore, notAfter, err := getCertBounds(secret)
 	if err != nil {
-		// If we can't get the CA, then really all we can do is log something, since
-		// if we returned an error then the controller lib would just call us again
-		// and again, which would probably yield the same results.
+		// If we can't read the cert, then really all we can do is log something,
+		// since if we returned an error then the controller lib would just call us
+		// again and again, which would probably yield the same results.
 		klog.Warningf("certsExpirerController Sync() found that the secret is malformed: %s", err.Error())
 		return nil
 	}
 
-	caLifetime := notAfter.Sub(notBefore)
-	caAge := time.Since(notBefore)
-	thresholdDelta := (float32(caAge) / float32(caLifetime)) - c.ageThreshold
-	klog.Infof("certsExpirerController Sync() found a CA age threshold delta of %.9g", thresholdDelta)
-	if thresholdDelta > 0 {
+	certAge := time.Since(notBefore)
+	renewDelta := certAge - c.renewBefore
+	klog.Infof("certsExpirerController Sync() found a renew delta of %s", renewDelta)
+	if renewDelta >= 0 || time.Now().After(notAfter) {
 		err := c.k8sClient.
 			CoreV1().
 			Secrets(c.namespace).
@@ -105,24 +100,25 @@ func (c *certsExpirerController) Sync(ctx controller.Context) error {
 	return nil
 }
 
-// getCABounds returns the NotBefore and NotAfter fields of the CA certificate
-// in the provided secret, or an error. Not that it expects the provided secret
-// to contain the well-known data keys from this package (see certs_manager.go).
-func getCABounds(secret *corev1.Secret) (time.Time, time.Time, error) {
-	caPEM := secret.Data[caCertificateSecretKey]
-	if caPEM == nil {
-		return time.Time{}, time.Time{}, constable.Error("failed to find CA")
+// getCertBounds returns the NotBefore and NotAfter fields of the TLS
+// certificate in the provided secret, or an error. Not that it expects the
+// provided secret to contain the well-known data keys from this package (see
+// certs_manager.go).
+func getCertBounds(secret *corev1.Secret) (time.Time, time.Time, error) {
+	certPEM := secret.Data[tlsCertificateChainSecretKey]
+	if certPEM == nil {
+		return time.Time{}, time.Time{}, constable.Error("failed to find certificate")
 	}
 
-	caBlock, _ := pem.Decode(caPEM)
-	if caBlock == nil {
-		return time.Time{}, time.Time{}, constable.Error("failed to decode CA PEM")
+	certBlock, _ := pem.Decode(certPEM)
+	if certBlock == nil {
+		return time.Time{}, time.Time{}, constable.Error("failed to decode certificate PEM")
 	}
 
-	caCrt, err := x509.ParseCertificate(caBlock.Bytes)
+	cert, err := x509.ParseCertificate(certBlock.Bytes)
 	if err != nil {
-		return time.Time{}, time.Time{}, fmt.Errorf("failed to parse CA: %w", err)
+		return time.Time{}, time.Time{}, fmt.Errorf("failed to parse certificate: %w", err)
 	}
 
-	return caCrt.NotBefore, caCrt.NotAfter, nil
+	return cert.NotBefore, cert.NotAfter, nil
 }

internal/controller/apicerts/certs_expirer_test.go

@@ -96,7 +96,7 @@ func TestExpirerControllerFilters(t *testing.T) {
 				nil, // k8sClient, not needed
 				secretsInformer,
 				withInformer.WithInformer,
-				0, // ageThreshold, not needed
+				0, // renewBefore, not needed
 			)
 
 			unrelated := corev1.Secret{}
@@ -114,7 +114,7 @@ func TestExpirerControllerSync(t *testing.T) {
 
 	tests := []struct {
 		name                string
-		ageThreshold        float32
+		renewBefore         time.Duration
 		fillSecretData      func(*testing.T, map[string][]byte)
 		configKubeAPIClient func(*kubernetesfake.Clientset)
 		wantDelete          bool
@@ -130,47 +130,62 @@ func TestExpirerControllerSync(t *testing.T) {
 			wantDelete:     false,
 		},
 		{
-			name:         "lifetime below threshold",
-			ageThreshold: 0.7,
+			name:        "lifetime below threshold",
+			renewBefore: 7 * time.Hour,
 			fillSecretData: func(t *testing.T, m map[string][]byte) {
-				caPEM, err := testutil.CreateCertificate(
+				certPEM, err := testutil.CreateCertificate(
 					time.Now().Add(-5*time.Hour),
 					time.Now().Add(5*time.Hour),
 				)
 				require.NoError(t, err)
 
-				// See cert_manager.go for this constant.
-				m["caCertificate"] = caPEM
+				// See certs_manager.go for this constant.
+				m["tlsCertificateChain"] = certPEM
 			},
 			wantDelete: false,
 		},
 		{
-			name:         "lifetime above threshold",
-			ageThreshold: 0.3,
+			name:        "lifetime above threshold",
+			renewBefore: 3 * time.Hour,
 			fillSecretData: func(t *testing.T, m map[string][]byte) {
-				caPEM, err := testutil.CreateCertificate(
+				certPEM, err := testutil.CreateCertificate(
 					time.Now().Add(-5*time.Hour),
 					time.Now().Add(5*time.Hour),
 				)
 				require.NoError(t, err)
 
-				// See cert_manager.go for this constant.
-				m["caCertificate"] = caPEM
+				// See certs_manager.go for this constant.
+				m["tlsCertificateChain"] = certPEM
 			},
 			wantDelete: true,
 		},
 		{
-			name:         "delete failure",
-			ageThreshold: 0.3,
+			name:        "cert expired",
+			renewBefore: 3 * time.Hour,
 			fillSecretData: func(t *testing.T, m map[string][]byte) {
-				caPEM, err := testutil.CreateCertificate(
+				certPEM, err := testutil.CreateCertificate(
+					time.Now().Add(-2*time.Hour),
+					time.Now().Add(-1*time.Hour),
+				)
+				require.NoError(t, err)
+
+				// See certs_manager.go for this constant.
+				m["tlsCertificateChain"] = certPEM
+			},
+			wantDelete: true,
+		},
+		{
+			name:        "delete failure",
+			renewBefore: 3 * time.Hour,
+			fillSecretData: func(t *testing.T, m map[string][]byte) {
+				certPEM, err := testutil.CreateCertificate(
 					time.Now().Add(-5*time.Hour),
 					time.Now().Add(5*time.Hour),
 				)
 				require.NoError(t, err)
 
-				// See cert_manager.go for this constant.
-				m["caCertificate"] = caPEM
+				// See certs_manager.go for this constant.
+				m["tlsCertificateChain"] = certPEM
 			},
 			configKubeAPIClient: func(c *kubernetesfake.Clientset) {
 				c.PrependReactor("delete", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) {
@@ -185,8 +200,8 @@ func TestExpirerControllerSync(t *testing.T) {
 				privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
 				require.NoError(t, err)
 
-				// See cert_manager.go for this constant.
-				m["caCertificate"] = x509.MarshalPKCS1PrivateKey(privateKey)
+				// See certs_manager.go for this constant.
+				m["tlsCertificateChain"] = x509.MarshalPKCS1PrivateKey(privateKey)
 			},
 			wantDelete: false,
 		},
@@ -205,7 +220,7 @@ func TestExpirerControllerSync(t *testing.T) {
 			}
 
 			kubeInformerClient := kubernetesfake.NewSimpleClientset()
-			name := "api-serving-cert" // See cert_manager.go.
+			name := "api-serving-cert" // See certs_manager.go.
 			namespace := "some-namespace"
 			if test.fillSecretData != nil {
 				secret := &corev1.Secret{
@@ -231,7 +246,7 @@ func TestExpirerControllerSync(t *testing.T) {
 				kubeAPIClient,
 				kubeInformers.Core().V1().Secrets(),
 				controller.WithInformer,
-				test.ageThreshold,
+				test.renewBefore,
 			)
 
 			// Must start informers before calling TestRunSynchronously().

internal/controller/apicerts/certs_manager.go

@@ -36,6 +36,10 @@ type certsManagerController struct {
 	k8sClient        kubernetes.Interface
 	aggregatorClient aggregatorclient.Interface
 	secretInformer   corev1informers.SecretInformer
+
+	// certDuration is the lifetime of the serving certificate that this
+	// controller will use when issuing the serving certificate.
+	certDuration time.Duration
 }
 
 func NewCertsManagerController(
@@ -45,6 +49,7 @@ func NewCertsManagerController(
 	secretInformer corev1informers.SecretInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 	withInitialEvent pinnipedcontroller.WithInitialEventOptionFunc,
+	certDuration time.Duration,
 ) controller.Controller {
 	return controller.New(
 		controller.Config{
@@ -54,6 +59,7 @@ func NewCertsManagerController(
 				k8sClient:        k8sClient,
 				aggregatorClient: aggregatorClient,
 				secretInformer:   secretInformer,
+				certDuration:     certDuration,
 			},
 		},
 		withInformer(
@@ -95,7 +101,7 @@ func (c *certsManagerController) Sync(ctx controller.Context) error {
 	aggregatedAPIServerTLSCert, err := aggregatedAPIServerCA.Issue(
 		pkix.Name{CommonName: serviceEndpoint},
 		[]string{serviceEndpoint},
-		24*365*time.Hour,
+		c.certDuration,
 	)
 	if err != nil {
 		return fmt.Errorf("could not issue serving certificate: %w", err)

internal/controller/apicerts/certs_manager_test.go

@@ -50,6 +50,7 @@ func TestManagerControllerOptions(t *testing.T) {
 				secretsInformer,
 				observableWithInformerOption.WithInformer,         // make it possible to observe the behavior of the Filters
 				observableWithInitialEventOption.WithInitialEvent, // make it possible to observe the behavior of the initial event
+				0, // certDuration, not needed for this test
 			)
 			secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer)
 		})
@@ -116,6 +117,7 @@ func TestManagerControllerOptions(t *testing.T) {
 func TestManagerControllerSync(t *testing.T) {
 	spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) {
 		const installedInNamespace = "some-namespace"
+		const certDuration = 12345678 * time.Second
 
 		var r *require.Assertions
 
@@ -139,6 +141,7 @@ func TestManagerControllerSync(t *testing.T) {
 				kubeInformers.Core().V1().Secrets(),
 				controller.WithInformer,
 				controller.WithInitialEvent,
+				certDuration,
 			)
 
 			// Set this at the last second to support calling subject.Name().
@@ -221,7 +224,7 @@ func TestManagerControllerSync(t *testing.T) {
 					// Validate the created cert using the CA, and also validate the cert's hostname
 					validCert := testutil.ValidateCertificate(t, actualCACert, actualCertChain)
 					validCert.RequireDNSName("pinniped-api." + installedInNamespace + ".svc")
-					validCert.RequireLifetime(time.Now(), time.Now().Add(24*365*time.Hour), 2*time.Minute)
+					validCert.RequireLifetime(time.Now(), time.Now().Add(certDuration), 2*time.Minute)
 					validCert.RequireMatchesPrivateKey(actualPrivateKey)
 
 					// Make sure we updated the APIService caBundle and left it otherwise unchanged