From 9a49878d2994169b38da8ccb87c2e4a5d915a248 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 | 13 ++++++--- pkg/operator/certrotation/target.go | 27 ++++++++++++------- 5 files changed, 42 insertions(+), 18 deletions(-) diff --git a/pkg/operator/certrotation/cabundle.go b/pkg/operator/certrotation/cabundle.go index 58c10b7cad..bf8cd95a71 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 modified := false @@ -73,7 +73,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) modified = true 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 36f3cf292d..5ec56273a0 100644 --- a/pkg/operator/certrotation/signer.go +++ b/pkg/operator/certrotation/signer.go @@ -68,6 +68,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 @@ -79,7 +80,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* ), Type: corev1.SecretTypeTLS, } - modified = true + signerExists = false } applyFn := resourceapply.ApplySecret @@ -94,7 +95,10 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* modified = needsMetadataUpdate || needsTypeChange || modified 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 @@ -139,7 +143,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 @@ -156,7 +161,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 879355e1b4..7dda238930 100644 --- a/pkg/operator/certrotation/target.go +++ b/pkg/operator/certrotation/target.go @@ -80,7 +80,7 @@ type TargetCertCreator interface { // NewCertificate creates a new key-cert pair with the given signer. NewCertificate(signer *crypto.CA, validity time.Duration) (*crypto.TLSCertificateConfig, error) // NeedNewTargetCertKeyPair decides whether a new cert-key pair is needed. It returns a non-empty reason if it is the case. - NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string + NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string // SetAnnotations gives an option to override or set additional annotations SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string } @@ -97,6 +97,7 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont // and need to mint one // TODO do the cross signing thing, but this shows the API consumers want and a very simple impl. + secretDoesntExist := false modified := false originalTargetCertKeyPairSecret, err := c.Lister.Secrets(c.Namespace).Get(c.Name) if err != nil && !apierrors.IsNotFound(err) { @@ -114,6 +115,7 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont Type: corev1.SecretTypeTLS, } modified = true + secretDoesntExist = true } applyFn := resourceapply.ApplySecret @@ -127,7 +129,7 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont needsSecretTypeUpdate := ensureSecretTLSTypeSet(targetCertKeyPairSecret) modified = needsMetadataUpdate || needsSecretTypeUpdate || modified - if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired); len(reason) > 0 { + if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, secretDoesntExist); len(reason) > 0 { c.EventRecorder.Eventf("TargetUpdateRequired", "%q in %q requires a new target cert/key pair: %v", c.Name, c.Namespace, reason) if err := setTargetCertKeyPairSecret(targetCertKeyPairSecret, c.Validity, signingCertKeyPair, c.CertCreator, c.AdditionalAnnotations); err != nil { return nil, err @@ -149,7 +151,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, secretDoesntExist bool) string { + if secretDoesntExist { + return "secret doesn't exist" + } + + annotations := secret.Annotations if reason := needNewTargetCertKeyPairForTime(annotations, signer, refresh, refreshOnlyWhenExpired); len(reason) > 0 { return reason } @@ -206,7 +213,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. @@ -266,8 +273,8 @@ func (r *ClientRotation) NewCertificate(signer *crypto.CA, validity time.Duratio return signer.MakeClientCertificateForDuration(r.UserInfo, validity) } -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) +func (r *ClientRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string { + return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist) } func (r *ClientRotation) SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string { @@ -291,8 +298,8 @@ func (r *ServingRotation) RecheckChannel() <-chan struct{} { return r.HostnamesChanged } -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) +func (r *ServingRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string { + reason := needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist) if len(reason) > 0 { return reason } @@ -337,8 +344,8 @@ func (r *SignerRotation) NewCertificate(signer *crypto.CA, validity time.Duratio return crypto.MakeCAConfigForDuration(signerName, validity, signer) } -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) +func (r *SignerRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string { + return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist) } func (r *SignerRotation) SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string {