From 16bafb54c851c2b8723f99cfd54223873d8894fb Mon Sep 17 00:00:00 2001 From: Ondra Kupka Date: Tue, 13 Jan 2026 15:38:16 +0100 Subject: [PATCH] pkg/operator/certrotation: Correct logging Right now secret/configmap annotation change logging is happening within utility functions, which are not always used to really apply changes, but only to generate desired state. This causes log entries mentioning changes applied, which are actually not being applied at all. This patch removes the log statements from EnsureTLSMetadataUpdate and indirectly also NewTLSArtifactObjectMeta, moving logging into relevant reconciliation loops, really emitting logs when a change is there. Assisted-by: Claude Code --- pkg/operator/certrotation/annotations.go | 16 -------- pkg/operator/certrotation/cabundle.go | 20 +++++++++- pkg/operator/certrotation/signer.go | 21 ++++++++++- pkg/operator/certrotation/target.go | 20 +++++++++- pkg/operator/csr/cert_controller.go | 48 ++++++++++++++++++++---- 5 files changed, 96 insertions(+), 29 deletions(-) diff --git a/pkg/operator/certrotation/annotations.go b/pkg/operator/certrotation/annotations.go index 3c051c88e6..2f1667669d 100644 --- a/pkg/operator/certrotation/annotations.go +++ b/pkg/operator/certrotation/annotations.go @@ -1,10 +1,8 @@ package certrotation import ( - "github.com/google/go-cmp/cmp" "github.com/openshift/api/annotations" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/klog/v2" ) const ( @@ -48,44 +46,30 @@ func (a AdditionalAnnotations) EnsureTLSMetadataUpdate(meta *metav1.ObjectMeta) meta.Annotations = make(map[string]string) } if len(a.JiraComponent) > 0 && meta.Annotations[annotations.OpenShiftComponent] != a.JiraComponent { - diff := cmp.Diff(meta.Annotations[annotations.OpenShiftComponent], a.JiraComponent) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", annotations.OpenShiftComponent, meta.Namespace, meta.Name, diff) meta.Annotations[annotations.OpenShiftComponent] = a.JiraComponent modified = true } if len(a.Description) > 0 && meta.Annotations[annotations.OpenShiftDescription] != a.Description { - diff := cmp.Diff(meta.Annotations[annotations.OpenShiftDescription], a.Description) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", annotations.OpenShiftDescription, meta.Namespace, meta.Name, diff) meta.Annotations[annotations.OpenShiftDescription] = a.Description modified = true } if len(a.TestName) > 0 && meta.Annotations[CertificateTestNameAnnotation] != a.TestName { - diff := cmp.Diff(meta.Annotations[CertificateTestNameAnnotation], a.TestName) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", CertificateTestNameAnnotation, meta.Name, meta.Namespace, diff) meta.Annotations[CertificateTestNameAnnotation] = a.TestName modified = true } if len(a.AutoRegenerateAfterOfflineExpiry) > 0 && meta.Annotations[CertificateAutoRegenerateAfterOfflineExpiryAnnotation] != a.AutoRegenerateAfterOfflineExpiry { - diff := cmp.Diff(meta.Annotations[CertificateAutoRegenerateAfterOfflineExpiryAnnotation], a.AutoRegenerateAfterOfflineExpiry) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", CertificateAutoRegenerateAfterOfflineExpiryAnnotation, meta.Namespace, meta.Name, diff) meta.Annotations[CertificateAutoRegenerateAfterOfflineExpiryAnnotation] = a.AutoRegenerateAfterOfflineExpiry modified = true } if len(a.NotBefore) > 0 && meta.Annotations[CertificateNotBeforeAnnotation] != a.NotBefore { - diff := cmp.Diff(meta.Annotations[CertificateNotBeforeAnnotation], a.NotBefore) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", CertificateNotBeforeAnnotation, meta.Name, meta.Namespace, diff) meta.Annotations[CertificateNotBeforeAnnotation] = a.NotBefore modified = true } if len(a.NotAfter) > 0 && meta.Annotations[CertificateNotAfterAnnotation] != a.NotAfter { - diff := cmp.Diff(meta.Annotations[CertificateNotAfterAnnotation], a.NotAfter) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", CertificateNotAfterAnnotation, meta.Name, meta.Namespace, diff) meta.Annotations[CertificateNotAfterAnnotation] = a.NotAfter modified = true } if len(a.RefreshPeriod) > 0 && meta.Annotations[CertificateRefreshPeriodAnnotation] != a.RefreshPeriod { - diff := cmp.Diff(meta.Annotations[CertificateRefreshPeriodAnnotation], a.RefreshPeriod) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", CertificateRefreshPeriodAnnotation, meta.Name, meta.Namespace, diff) meta.Annotations[CertificateRefreshPeriodAnnotation] = a.RefreshPeriod modified = true } diff --git a/pkg/operator/certrotation/cabundle.go b/pkg/operator/certrotation/cabundle.go index 447b1e0e31..e38f4f57e4 100644 --- a/pkg/operator/certrotation/cabundle.go +++ b/pkg/operator/certrotation/cabundle.go @@ -8,6 +8,8 @@ import ( "reflect" "sort" + "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -101,7 +103,14 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC if err != nil { return nil, err } - klog.V(2).Infof("Created ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name) + + if klog.V(2).Enabled() { + klog.V(2).Infof("Created ca-bundle.crt configmap %s/%s with:\n%s", caBundleConfigMap.Namespace, caBundleConfigMap.Name, certs.CertificateBundleToString(updatedCerts)) + if diff := cmp.Diff(nil, actualCABundleConfigMap.Annotations); len(diff) > 0 { + klog.V(2).Infof("ConfigMap %s/%s annotations diff: %s", actualCABundleConfigMap.Namespace, actualCABundleConfigMap.Name, diff) + } + } + caBundleConfigMap = actualCABundleConfigMap } else if updateRequired { actualCABundleConfigMap, err := c.Client.ConfigMaps(c.Namespace).Update(ctx, caBundleConfigMap, metav1.UpdateOptions{}) @@ -113,7 +122,14 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC if err != nil { return nil, err } - klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name) + + if klog.V(2).Enabled() { + klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", caBundleConfigMap.Namespace, caBundleConfigMap.Name, certs.CertificateBundleToString(updatedCerts)) + if diff := cmp.Diff(originalCABundleConfigMap.Annotations, actualCABundleConfigMap.Annotations); len(diff) > 0 { + klog.V(2).Infof("ConfigMap %s/%s annotations diff: %s", actualCABundleConfigMap.Namespace, actualCABundleConfigMap.Name, diff) + } + } + caBundleConfigMap = actualCABundleConfigMap } diff --git a/pkg/operator/certrotation/signer.go b/pkg/operator/certrotation/signer.go index c2c8b8368f..89f95ec260 100644 --- a/pkg/operator/certrotation/signer.go +++ b/pkg/operator/certrotation/signer.go @@ -6,6 +6,8 @@ import ( "fmt" "time" + "github.com/google/go-cmp/cmp" + "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" @@ -64,6 +66,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* if err != nil && !apierrors.IsNotFound(err) { return nil, false, err } + signingCertKeyPairSecret := originalSigningCertKeyPairSecret.DeepCopy() if apierrors.IsNotFound(err) { // create an empty one @@ -108,7 +111,14 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* if err != nil { return nil, false, err } - klog.V(2).Infof("Created secret %s/%s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name) + + if klog.V(2).Enabled() { + klog.V(2).Infof("Created secret %s/%s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name) + if diff := cmp.Diff(nil, actualSigningCertKeyPairSecret.Annotations); len(diff) > 0 { + klog.V(2).Infof("Secret %s/%s annotations diff: %s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name, diff) + } + } + signingCertKeyPairSecret = actualSigningCertKeyPairSecret } else if updateRequired { actualSigningCertKeyPairSecret, err := c.Client.Secrets(c.Namespace).Update(ctx, signingCertKeyPairSecret, metav1.UpdateOptions{}) @@ -120,7 +130,14 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* if err != nil { return nil, false, err } - klog.V(2).Infof("Updated secret %s/%s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name) + + if klog.V(2).Enabled() { + klog.V(2).Infof("Updated secret %s/%s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name) + if diff := cmp.Diff(originalSigningCertKeyPairSecret.Annotations, actualSigningCertKeyPairSecret.Annotations); len(diff) > 0 { + klog.V(2).Infof("Secret %s/%s annotations diff: %s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name, diff) + } + } + signingCertKeyPairSecret = actualSigningCertKeyPairSecret } diff --git a/pkg/operator/certrotation/target.go b/pkg/operator/certrotation/target.go index 88cd41189e..a3eb44616a 100644 --- a/pkg/operator/certrotation/target.go +++ b/pkg/operator/certrotation/target.go @@ -7,6 +7,8 @@ import ( "strings" "time" + "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -135,7 +137,14 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont if err != nil { return nil, err } - klog.V(2).Infof("Created secret %s/%s", actualTargetCertKeyPairSecret.Namespace, actualTargetCertKeyPairSecret.Name) + + if klog.V(2).Enabled() { + klog.V(2).Infof("Created secret %s/%s", actualTargetCertKeyPairSecret.Namespace, actualTargetCertKeyPairSecret.Name) + if diff := cmp.Diff(nil, actualTargetCertKeyPairSecret.Annotations); len(diff) > 0 { + klog.V(2).Infof("Secret %s/%s annotations diff: %s", actualTargetCertKeyPairSecret.Namespace, actualTargetCertKeyPairSecret.Name, diff) + } + } + targetCertKeyPairSecret = actualTargetCertKeyPairSecret } else if updateRequired { actualTargetCertKeyPairSecret, err := c.Client.Secrets(c.Namespace).Update(ctx, targetCertKeyPairSecret, metav1.UpdateOptions{}) @@ -147,7 +156,14 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont if err != nil { return nil, err } - klog.V(2).Infof("Updated secret %s/%s", actualTargetCertKeyPairSecret.Namespace, actualTargetCertKeyPairSecret.Name) + + if klog.V(2).Enabled() { + klog.V(2).Infof("Updated secret %s/%s", actualTargetCertKeyPairSecret.Namespace, actualTargetCertKeyPairSecret.Name) + if diff := cmp.Diff(originalTargetCertKeyPairSecret.Annotations, actualTargetCertKeyPairSecret.Annotations); len(diff) > 0 { + klog.V(2).Infof("Secret %s/%s annotations diff: %s", actualTargetCertKeyPairSecret.Namespace, actualTargetCertKeyPairSecret.Name, diff) + } + } + targetCertKeyPairSecret = actualTargetCertKeyPairSecret } diff --git a/pkg/operator/csr/cert_controller.go b/pkg/operator/csr/cert_controller.go index ba4f1b358f..514f7f193e 100644 --- a/pkg/operator/csr/cert_controller.go +++ b/pkg/operator/csr/cert_controller.go @@ -10,6 +10,8 @@ import ( "math/rand" "time" + "github.com/google/go-cmp/cmp" + "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/certrotation" "github.com/openshift/library-go/pkg/operator/events" @@ -151,6 +153,10 @@ func NewClientCertificateController( func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory.SyncContext) error { // get secret containing client certificate secret, err := c.spokeCoreClient.Secrets(c.SecretNamespace).Get(ctx, c.SecretName, metav1.GetOptions{}) + + // Capture original annotations for diff logging. + var originalAnnotations map[string]string + switch { case errors.IsNotFound(err): secret = &corev1.Secret{ @@ -162,6 +168,12 @@ func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory. } case err != nil: return fmt.Errorf("unable to get secret %q: %w", c.SecretNamespace+"/"+c.SecretName, err) + default: + // Copy original annotations before any modifications + originalAnnotations = make(map[string]string, len(secret.Annotations)) + for k, v := range secret.Annotations { + originalAnnotations[k] = v + } } needsMetadataUpdate := c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&secret.ObjectMeta) @@ -188,14 +200,14 @@ func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory. _ = c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&secret.ObjectMeta) // save the changes into secret - if err := c.saveSecret(secret); err != nil { + if err := c.saveSecret(secret, originalAnnotations); err != nil { return err } syncCtx.Recorder().Eventf("ClientCertificateCreated", "A new client certificate for %s is available", c.controllerName) c.reset() return nil } else if needsMetadataUpdate && len(secret.ResourceVersion) > 0 { - if err := c.saveSecret(secret); err != nil { + if err := c.saveSecret(secret, originalAnnotations); err != nil { return err } } @@ -326,14 +338,36 @@ func (c *clientCertificateController) createCSR(ctx context.Context) (string, er return req.Name, nil } -func (c *clientCertificateController) saveSecret(secret *corev1.Secret) error { - var err error +func (c *clientCertificateController) saveSecret(secret *corev1.Secret, oldAnnotations map[string]string) error { if secret.ResourceVersion == "" { - _, err = c.spokeCoreClient.Secrets(c.SecretNamespace).Create(context.Background(), secret, metav1.CreateOptions{}) + actualSecret, err := c.spokeCoreClient.Secrets(c.SecretNamespace).Create(context.Background(), secret, metav1.CreateOptions{}) + if err != nil { + return err + } + + if klog.V(2).Enabled() { + klog.V(2).Infof("Created secret %s/%s", actualSecret.Namespace, actualSecret.Name) + if diff := cmp.Diff(nil, actualSecret.Annotations); len(diff) > 0 { + klog.V(2).Infof("Secret %s/%s annotations diff: %s", actualSecret.Namespace, actualSecret.Name, diff) + } + } + + return nil + } + + actualSecret, err := c.spokeCoreClient.Secrets(c.SecretNamespace).Update(context.Background(), secret, metav1.UpdateOptions{}) + if err != nil { return err } - _, err = c.spokeCoreClient.Secrets(c.SecretNamespace).Update(context.Background(), secret, metav1.UpdateOptions{}) - return err + + if klog.V(2).Enabled() { + klog.V(2).Infof("Updated secret %s/%s", actualSecret.Namespace, actualSecret.Name) + if diff := cmp.Diff(oldAnnotations, actualSecret.Annotations); len(diff) > 0 { + klog.V(2).Infof("Secret %s/%s annotations diff: %s", actualSecret.Namespace, actualSecret.Name, diff) + } + } + + return nil } func (c *clientCertificateController) reset() {