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 c04d27bc..dbd48628 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 c4296877..e7e25e4f 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 96829867..7e17d15e 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 1ba9cd7c..6129a974 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,