certrotation: issue more specific update events #1989

vrutkovs · 2025-08-07T06:45:00Z

When secret/configmap is being updated, an event is being emitted. This event should include a more
specific reason why this resource was updated: content change, new annotation etc.

This ensures duplicate events are not being issued by the controller.

Testing this in openshift/cluster-kube-apiserver-operator#1894

openshift-ci · 2025-08-07T06:45:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vrutkovs
Once this PR has been reviewed and has the lgtm label, please assign jsafrane for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/operator/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sanchezl · 2025-08-12T13:01:09Z

pkg/operator/certrotation/cabundle.go

-			needsOwnerUpdate = ensureOwnerReference(&caBundleConfigMap.ObjectMeta, c.Owner)
+		updateRequired = ensureOwnerRefAndTLSAnnotationsForConfigMap(caBundleConfigMap, c.Owner, c.AdditionalAnnotations)
+		if updateRequired {
+			updateDetail = fmt.Sprintf("annotations set to %#v", c.AdditionalAnnotations)


What is only the owner was updated?

EnsureTLSMetadataUpdate was setting annotations, but it was unnecessarily setting needsMetadataUpdate and needsOwnerUpdate.

It reads like: if owner is updated or annotations are updated, event should say "annotations updated" (even if annotations were not updated).

Oh, I see. Okay, I'll issue "owner updated" and "annotations updated" separately

p0lyn0mial · 2025-08-13T07:16:01Z

pkg/operator/certrotation/metadata.go

-	// Existing secret not found - no need to update metadata (will be done by needNewSigningCertKeyPair / NeedNewTargetCertKeyPair)
-	if len(secret.ResourceVersion) == 0 {
-		return false
+func ensureOwnerRefAndTLSAnnotationsForConfigMap(configMap *corev1.ConfigMap, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) string {


why not to have ensureOwnerRefAndTLSAnnotations((meta *metav1.ObjectMeta, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) which could be used for both cm and secrets?

p0lyn0mial · 2025-08-13T07:21:33Z

pkg/operator/certrotation/cabundle.go

 			reason = fmt.Sprintf("signer update %s", signingCertKeyPairLocation)
 		}
 		c.EventRecorder.Eventf("CABundleUpdateRequired", "%q in %q requires a new cert: %s", c.Name, c.Namespace, reason)
+		updateDetail = fmt.Sprintf("content change: %s", reason)


mhm, will this remove the reason set inline 72 ?

Reworked to make it append (adding a comma if necessary)

p0lyn0mial · 2025-08-13T07:21:50Z

pkg/operator/certrotation/cabundle.go

 	}

 	// run Update if metadata needs changing unless running in RefreshOnlyWhenExpired mode
+	updateDetail := ""


nit: updateReason

p0lyn0mial · 2025-08-13T07:22:54Z

pkg/operator/certrotation/metadata.go


-func ensureOwnerRefAndTLSAnnotations(secret *corev1.Secret, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) bool {
-	needsMetadataUpdate := false
+func ensureOwnerRefAndTLSAnnotationsForSecret(secret *corev1.Secret, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) string {


change to ensureOwnerRefAndTLSAnnotations((meta *metav1.ObjectMeta, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations)

Ah, I was not sure if setting annotations would stick. Unit tests show that it works

p0lyn0mial · 2025-08-13T07:23:08Z

pkg/operator/certrotation/signer.go

 	}

 	// run Update if metadata needs changing unless we're in RefreshOnlyWhenExpired mode
+	updateDetail := ""


nit: updateReason

p0lyn0mial · 2025-08-13T07:23:29Z

pkg/operator/certrotation/signer.go

 			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)
+		updateDetail = fmt.Sprintf("signer update: %s", reason)


won't this remove the reason set in line 82?

p0lyn0mial · 2025-08-13T07:23:49Z

pkg/operator/certrotation/target.go


 	if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, creationRequired); len(reason) > 0 {
 		c.EventRecorder.Eventf("TargetUpdateRequired", "%q in %q requires a new target cert/key pair: %v", c.Name, c.Namespace, reason)
+		updateDetail = fmt.Sprintf("content change: %s", reason)


same comment here.

vrutkovs · 2025-08-14T05:35:41Z

Tested in openshift/cluster-kube-apiserver-operator#1894, see Loki output for example events

p0lyn0mial · 2025-08-14T12:33:51Z

pkg/operator/certrotation/cabundle.go

 		}
 		c.EventRecorder.Eventf("CABundleUpdateRequired", "%q in %q requires a new cert: %s", c.Name, c.Namespace, reason)
+		if len(updateReason) > 0 {
+			updateReason += ", "


mhm, how about using strings.Join(updateReasons, ", ") instead ?

p0lyn0mial · 2025-08-14T12:34:11Z

pkg/operator/certrotation/cabundle.go

 	}

 	// run Update if metadata needs changing unless running in RefreshOnlyWhenExpired mode
+	updateReason := ""


then this would become updateReasons := []string{}

p0lyn0mial · 2025-08-14T12:34:19Z

pkg/operator/certrotation/cabundle.go

-		}
-		needsMetadataUpdate := c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&caBundleConfigMap.ObjectMeta)
-		updateRequired = needsOwnerUpdate || needsMetadataUpdate
+		updateReason = ensureOwnerRefAndTLSAnnotations(&caBundleConfigMap.ObjectMeta, c.Owner, c.AdditionalAnnotations)


then we would have to append here

p0lyn0mial · 2025-08-14T12:34:25Z

pkg/operator/certrotation/cabundle.go

+		if len(updateReason) > 0 {
+			updateReason += ", "
+		}
+		updateReason += fmt.Sprintf("content change: %s", reason)


p0lyn0mial · 2025-08-14T12:35:09Z

pkg/operator/certrotation/cabundle.go

 			return nil, nil
 		}
-		resourcehelper.ReportUpdateEvent(c.EventRecorder, actualCABundleConfigMap, err)
+		resourcehelper.ReportUpdateEvent(c.EventRecorder, actualCABundleConfigMap, err, updateReason)


and here we could use strings.Join(updateReasons, ", ")

p0lyn0mial · 2025-08-14T12:35:21Z

pkg/operator/certrotation/cabundle.go

 			return nil, err
 		}
-		klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name)
+		klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s due to %s with:\n%s", caBundleConfigMap.Namespace, caBundleConfigMap.Name, updateReason, certs.CertificateBundleToString(updatedCerts))


strings.Join(updateReasons, ", ")

p0lyn0mial · 2025-08-14T12:36:40Z

pkg/operator/certrotation/metadata.go


-func ensureOwnerRefAndTLSAnnotations(secret *corev1.Secret, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) bool {
-	needsMetadataUpdate := false
+func ensureOwnerRefAndTLSAnnotations(meta *metav1.ObjectMeta, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) string {


this could return []string

Good idea, this simplifies quite a few things (although its rare that an update changes both owners and annotations during normal update)

p0lyn0mial · 2025-08-14T12:38:58Z

pkg/operator/certrotation/metadata.go

-	return additionalAnnotations.EnsureTLSMetadataUpdate(&secret.ObjectMeta) || needsMetadataUpdate
-}
-
-func ensureSecretTLSTypeSet(secret *corev1.Secret) bool {


not needed anymore ?

Yes, no longer required. Last time we touched this was last year in 4.17, and now all new and upgraded clusters have secrets of the new type.

OKay, lets keep it. Since we now emit a more precise event message, we can also confirm this no longer happens

p0lyn0mial · 2025-08-14T12:39:14Z

pkg/operator/certrotation/signer.go

+	updateReason := ""
 	if !c.RefreshOnlyWhenExpired {
-		needsMetadataUpdate := ensureOwnerRefAndTLSAnnotations(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations)
-		needsTypeChange := ensureSecretTLSTypeSet(signingCertKeyPairSecret)


not needed anymore ?

are we sure that all resources have been migrated?
maybe we should keep it for safety ?

p0lyn0mial · 2025-08-14T12:41:33Z

pkg/operator/certrotation/target.go

+	updateReason := ""
 	if !c.RefreshOnlyWhenExpired {
-		needsMetadataUpdate := ensureOwnerRefAndTLSAnnotations(targetCertKeyPairSecret, c.Owner, c.AdditionalAnnotations)
-		needsTypeChange := ensureSecretTLSTypeSet(targetCertKeyPairSecret)


not needed anymore ?

When secret/configmap is being update an event is being emitted. This event should include a more specific reason why this resource was updated: content change, new annotation etc. This ensures duplicate events are not being issued by the controller

openshift-ci · 2025-08-20T12:37:27Z

@vrutkovs: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot requested review from hexfusion and jsafrane August 7, 2025 06:45

vrutkovs force-pushed the certrotation-better-events branch 3 times, most recently from f4c5429 to 45778d8 Compare August 7, 2025 14:41

sanchezl suggested changes Aug 12, 2025

View reviewed changes

p0lyn0mial reviewed Aug 13, 2025

View reviewed changes

vrutkovs force-pushed the certrotation-better-events branch from 891c955 to f363b70 Compare August 13, 2025 11:07

p0lyn0mial reviewed Aug 14, 2025

View reviewed changes

vrutkovs force-pushed the certrotation-better-events branch from f363b70 to d3b7cde Compare August 15, 2025 07:08

vrutkovs added 4 commits August 18, 2025 09:54

certrotation: add ensureConfigmapUpdate alongside ensureSecretUpdate

10e23c1

certrotation: update details specific for annotations and owners

b1a1bf5

certrotation: pass RefreshOnlyWhenExpired in cabundle test

db1a930

vrutkovs force-pushed the certrotation-better-events branch 2 times, most recently from d390383 to db1a930 Compare August 18, 2025 12:58

certrotation: update reasons in getValidityFromAnnotations

576d96f

vrutkovs force-pushed the certrotation-better-events branch from 70763c5 to 576d96f Compare August 20, 2025 12:16

certrotation: issue more specific update events #1989

Are you sure you want to change the base?

certrotation: issue more specific update events #1989

Conversation

vrutkovs commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs commented Aug 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 20, 2025

Uh oh!

Uh oh!

vrutkovs commented Aug 7, 2025 •

edited

Loading

vrutkovs Aug 13, 2025 •

edited

Loading