From 4bf715758fd61dcbd8c4190b5fa97bc786e4ca1f Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 6 Oct 2021 11:46:54 -0400 Subject: [PATCH] Do not rotate impersonation proxy signer CA unless necessary This change fixes a copy paste error that led to the impersonation proxy signer CA being rotated based on the configuration of the rotation of the aggregated API serving certificate. This would lead to occasional "Unauthorized" flakes in our CI environments that rotate the serving certificate at a frequent interval. Updated the certs_expirer controller logs to be more detailed. Updated CA common names to be more specific (this does not update any previously generated CAs). Signed-off-by: Monis Khan --- internal/controller/apicerts/certs_expirer.go | 26 ++++++++++++++----- .../impersonatorconfig/impersonator_config.go | 2 +- .../impersonator_config_test.go | 2 +- .../controllermanager/prepare_controllers.go | 6 ++--- 4 files changed, 25 insertions(+), 11 deletions(-) diff --git a/internal/controller/apicerts/certs_expirer.go b/internal/controller/apicerts/certs_expirer.go index c04d27bc4..dbd486280 100644 --- a/internal/controller/apicerts/certs_expirer.go +++ b/internal/controller/apicerts/certs_expirer.go @@ -14,11 +14,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" - "k8s.io/klog/v2" "go.pinniped.dev/internal/constable" pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/plog" ) type certsExpirerController struct { @@ -74,7 +74,13 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error { return fmt.Errorf("failed to get %s/%s secret: %w", c.namespace, c.certsSecretResourceName, err) } if notFound { - klog.Info("certsExpirerController Sync found that the secret does not exist yet or was deleted") + plog.Info("secret does not exist yet or was deleted", + "controller", ctx.Name, + "namespace", c.namespace, + "name", c.certsSecretResourceName, + "key", c.secretKey, + "renewBefore", c.renewBefore.String(), + ) return nil } @@ -85,7 +91,17 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error { certAge := time.Since(notBefore) renewDelta := certAge - c.renewBefore - klog.Infof("certsExpirerController Sync found a renew delta of %s", renewDelta) + plog.Debug("found renew delta", + "controller", ctx.Name, + "namespace", c.namespace, + "name", c.certsSecretResourceName, + "key", c.secretKey, + "renewBefore", c.renewBefore.String(), + "notBefore", notBefore.String(), + "notAfter", notAfter.String(), + "certAge", certAge.String(), + "renewDelta", renewDelta.String(), + ) if renewDelta >= 0 || time.Now().After(notAfter) { err := c.k8sClient. CoreV1(). @@ -107,9 +123,7 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error { } // 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). +// certificate in the provided secret, or an error. func (c *certsExpirerController) getCertBounds(secret *corev1.Secret) (time.Time, time.Time, error) { certPEM := secret.Data[c.secretKey] if certPEM == nil { diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index c4296877b..e7e25e4f8 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -51,7 +51,7 @@ const ( impersonationProxyPort = 8444 defaultHTTPSPort = 443 approximatelyOneHundredYears = 100 * 365 * 24 * time.Hour - caCommonName = "Pinniped Impersonation Proxy CA" + caCommonName = "Pinniped Impersonation Proxy Serving CA" caCrtKey = "ca.crt" caKeyKey = "ca.key" appLabelKey = "app" diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 968298673..7e17d15e0 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -1055,7 +1055,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { require.NotNil(t, block) caCert, err := x509.ParseCertificate(block.Bytes) require.NoError(t, err) - require.Equal(t, "Pinniped Impersonation Proxy CA", caCert.Subject.CommonName) + require.Equal(t, "Pinniped Impersonation Proxy Serving CA", caCert.Subject.CommonName) require.WithinDuration(t, time.Now().Add(-5*time.Minute), caCert.NotBefore, 10*time.Second) require.WithinDuration(t, time.Now().Add(100*time.Hour*24*365), caCert.NotAfter, 10*time.Second) return createdCertPEM diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index 1ba9cd7c1..6129a9747 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -148,7 +148,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { controllerlib.WithInformer, controllerlib.WithInitialEvent, c.ServingCertDuration, - "Pinniped CA", + "Pinniped Aggregation CA", c.NamesConfig.APIService, ), singletonWorker, @@ -285,7 +285,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { controllerlib.WithInformer, controllerlib.WithInitialEvent, 365*24*time.Hour, // 1 year hard coded value - "Pinniped Impersonation Proxy CA", + "Pinniped Impersonation Proxy Signer CA", "", // optional, means do not give me a serving cert ), singletonWorker, @@ -297,7 +297,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { client.Kubernetes, informers.installationNamespaceK8s.Core().V1().Secrets(), controllerlib.WithInformer, - c.ServingCertRenewBefore, + 365*24*time.Hour-time.Hour, // 1 year minus 1 hour hard coded value (i.e. wait until the last moment to break the signer) apicerts.CACertificateSecretKey, ), singletonWorker,