From 85a077eb16bf0e263b8e05925cf4695d4a0968ee Mon Sep 17 00:00:00 2001 From: Vadim Rutkovsky Date: Wed, 17 Apr 2024 15:42:29 +0200 Subject: [PATCH] certrotation: improve error messages * Explicitly log "secret/configmap doesn't exist" events * when secret is refreshed differenciate between refresh on 80% of validity and custom refresh time * when CA bundle is updated log which signer update caused it --- pkg/operator/certrotation/cabundle.go | 12 ++++++++++-- pkg/operator/certrotation/cabundle_test.go | 2 +- .../client_cert_rotation_controller.go | 6 +++++- pkg/operator/certrotation/signer.go | 12 +++++++++--- pkg/operator/certrotation/target.go | 15 ++++++++++----- 5 files changed, 35 insertions(+), 12 deletions(-) diff --git a/pkg/operator/certrotation/cabundle.go b/pkg/operator/certrotation/cabundle.go index 7ec91f7863..5cf888c25e 100644 --- a/pkg/operator/certrotation/cabundle.go +++ b/pkg/operator/certrotation/cabundle.go @@ -41,7 +41,7 @@ type CABundleConfigMap struct { EventRecorder events.Recorder } -func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingCertKeyPair *crypto.CA) ([]*x509.Certificate, error) { +func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingCertKeyPair *crypto.CA, signingCertKeyPairLocation string) ([]*x509.Certificate, error) { // by this point we have current signing cert/key pair. We now need to make sure that the ca-bundle configmap has this cert and // doesn't have any expired certs originalCABundleConfigMap, err := c.Lister.ConfigMaps(c.Namespace).Get(c.Name) @@ -75,7 +75,15 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC return nil, err } if originalCABundleConfigMap == nil || originalCABundleConfigMap.Data == nil || !equality.Semantic.DeepEqual(originalCABundleConfigMap.Data, caBundleConfigMap.Data) { - c.EventRecorder.Eventf("CABundleUpdateRequired", "%q in %q requires a new cert", c.Name, c.Namespace) + reason := "" + if originalCABundleConfigMap == nil { + reason = "configmap doesn't exist" + } else if originalCABundleConfigMap.Data == nil { + reason = "configmap is empty" + } else if !equality.Semantic.DeepEqual(originalCABundleConfigMap.Data, caBundleConfigMap.Data) { + reason = fmt.Sprintf("signer update %s", signingCertKeyPairLocation) + } + c.EventRecorder.Eventf("CABundleUpdateRequired", "%q in %q requires a new cert: %s", c.Name, c.Namespace, reason) LabelAsManagedConfigMap(caBundleConfigMap, CertificateTypeCABundle) actualCABundleConfigMap, modified, err := resourceapply.ApplyConfigMap(ctx, c.Client, c.EventRecorder, caBundleConfigMap) diff --git a/pkg/operator/certrotation/cabundle_test.go b/pkg/operator/certrotation/cabundle_test.go index d9795b2af3..10d7c2aedb 100644 --- a/pkg/operator/certrotation/cabundle_test.go +++ b/pkg/operator/certrotation/cabundle_test.go @@ -236,7 +236,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = c.EnsureConfigMapCABundle(context.TODO(), newCA) + _, err = c.EnsureConfigMapCABundle(context.TODO(), newCA, "signer-secret") switch { case err != nil && len(test.expectedError) == 0: t.Error(err) diff --git a/pkg/operator/certrotation/client_cert_rotation_controller.go b/pkg/operator/certrotation/client_cert_rotation_controller.go index 5159f562a3..71ae47ff1f 100644 --- a/pkg/operator/certrotation/client_cert_rotation_controller.go +++ b/pkg/operator/certrotation/client_cert_rotation_controller.go @@ -121,13 +121,17 @@ func (c CertRotationController) Sync(ctx context.Context, syncCtx factory.SyncCo return syncErr } +func (c CertRotationController) getSigningCertKeyPairLocation() string { + return fmt.Sprintf("%s/%s", c.RotatedSelfSignedCertKeySecret.Namespace, c.RotatedSelfSignedCertKeySecret.Name) +} + func (c CertRotationController) SyncWorker(ctx context.Context) error { signingCertKeyPair, _, err := c.RotatedSigningCASecret.EnsureSigningCertKeyPair(ctx) if err != nil { return err } - cabundleCerts, err := c.CABundleConfigMap.EnsureConfigMapCABundle(ctx, signingCertKeyPair) + cabundleCerts, err := c.CABundleConfigMap.EnsureConfigMapCABundle(ctx, signingCertKeyPair, c.getSigningCertKeyPairLocation()) if err != nil { return err } diff --git a/pkg/operator/certrotation/signer.go b/pkg/operator/certrotation/signer.go index 4cf805bb7b..02a19d7b61 100644 --- a/pkg/operator/certrotation/signer.go +++ b/pkg/operator/certrotation/signer.go @@ -67,6 +67,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* if err != nil && !apierrors.IsNotFound(err) { return nil, false, err } + var signerExists = true signingCertKeyPairSecret := originalSigningCertKeyPairSecret.DeepCopy() if apierrors.IsNotFound(err) { // create an empty one @@ -78,6 +79,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* ), Type: corev1.SecretTypeTLS, } + signerExists = false } applyFn := resourceapply.ApplySecret @@ -96,7 +98,10 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* } signerUpdated := false - if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret.Annotations, c.Refresh, c.RefreshOnlyWhenExpired); needed { + if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret, c.Refresh, c.RefreshOnlyWhenExpired); needed || !signerExists { + if !signerExists { + reason = "secret doesn't exist" + } c.EventRecorder.Eventf("SignerUpdateRequired", "%q in %q requires a new signing cert/key pair: %v", c.Name, c.Namespace, reason) if err := setSigningCertKeyPairSecret(signingCertKeyPairSecret, c.Validity); err != nil { return nil, false, err @@ -136,7 +141,8 @@ func ensureOwnerReference(meta *metav1.ObjectMeta, owner *metav1.OwnerReference) return false } -func needNewSigningCertKeyPair(annotations map[string]string, refresh time.Duration, refreshOnlyWhenExpired bool) (bool, string) { +func needNewSigningCertKeyPair(secret *corev1.Secret, refresh time.Duration, refreshOnlyWhenExpired bool) (bool, string) { + annotations := secret.Annotations notBefore, notAfter, reason := getValidityFromAnnotations(annotations) if len(reason) > 0 { return true, reason @@ -153,7 +159,7 @@ func needNewSigningCertKeyPair(annotations map[string]string, refresh time.Durat validity := notAfter.Sub(notBefore) at80Percent := notAfter.Add(-validity / 5) if time.Now().After(at80Percent) { - return true, fmt.Sprintf("past its latest possible time %v", at80Percent) + return true, fmt.Sprintf("past refresh time (80%% of validity): %v", at80Percent) } developerSpecifiedRefresh := notBefore.Add(refresh) diff --git a/pkg/operator/certrotation/target.go b/pkg/operator/certrotation/target.go index 99bdc93bea..a585fb62a3 100644 --- a/pkg/operator/certrotation/target.go +++ b/pkg/operator/certrotation/target.go @@ -146,7 +146,12 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont return targetCertKeyPairSecret, nil } -func needNewTargetCertKeyPair(annotations map[string]string, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string { +func needNewTargetCertKeyPair(secret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string { + if secret.ResourceVersion == "" { + return "secret doesn't exist" + } + + annotations := secret.Annotations if reason := needNewTargetCertKeyPairForTime(annotations, signer, refresh, refreshOnlyWhenExpired); len(reason) > 0 { return reason } @@ -203,7 +208,7 @@ func needNewTargetCertKeyPairForTime(annotations map[string]string, signer *cryp validity := notAfter.Sub(notBefore) at80Percent := notAfter.Add(-validity / 5) if time.Now().After(at80Percent) { - return fmt.Sprintf("past its latest possible time %v", at80Percent) + return fmt.Sprintf("past refresh time (80%% of validity): %v", at80Percent) } // If Certificate is past its refresh time, we may have action to take. We only do this if the signer is old enough. @@ -264,7 +269,7 @@ func (r *ClientRotation) NewCertificate(signer *crypto.CA, validity time.Duratio } func (r *ClientRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string { - return needNewTargetCertKeyPair(currentCertSecret.Annotations, signer, caBundleCerts, refresh, refreshOnlyWhenExpired) + return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired) } func (r *ClientRotation) SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string { @@ -289,7 +294,7 @@ func (r *ServingRotation) RecheckChannel() <-chan struct{} { } func (r *ServingRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string { - reason := needNewTargetCertKeyPair(currentCertSecret.Annotations, signer, caBundleCerts, refresh, refreshOnlyWhenExpired) + reason := needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired) if len(reason) > 0 { return reason } @@ -335,7 +340,7 @@ func (r *SignerRotation) NewCertificate(signer *crypto.CA, validity time.Duratio } func (r *SignerRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string { - return needNewTargetCertKeyPair(currentCertSecret.Annotations, signer, caBundleCerts, refresh, refreshOnlyWhenExpired) + return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired) } func (r *SignerRotation) SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string {