OCPBUGS-57049: Update cert annotations #1971

vrutkovs · 2025-06-10T06:31:17Z

Move testcase name out of auto-regenerate-after-offline-expiry, add
refresh-period.

Follow-up for openshift/origin#29327
Tested in openshift/cluster-kube-apiserver-operator#1768 and openshift/cluster-authentication-operator#742

openshift-ci-robot · 2025-06-10T06:31:23Z

@vrutkovs: This pull request references Jira Issue OCPBUGS-57049, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.20.0) matches configured target version for branch (4.20.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @wangke19

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Move testcase name out of auto-regenerate-after-offline-expiry, add
refresh-period.

Follow-up for openshift/origin#29327

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 openshift-eng/jira-lifecycle-plugin repository.

vrutkovs · 2025-06-10T13:08:55Z

/hold

This makes

certificates.openshift.io/refresh-period: 70080h0m0s

instead of

certificates.openshift.io/refresh-period: 8y

Also 12h0m0s should be trimmed to 12h

vrutkovs · 2025-06-18T06:56:39Z

/hold cancel

Tested in openshift/cluster-kube-apiserver-operator#1768

pkg/operator/certrotation/annotations.go

pkg/operator/certrotation/utils.go

jsafrane · 2025-07-11T13:45:57Z

/approve

sjenning · 2025-07-11T14:19:01Z

/lgtm

pkg/operator/certrotation/annotations.go

p0lyn0mial · 2025-07-23T07:35:39Z

pkg/operator/certrotation/target.go

+
 	annotations.NotBefore = certKeyPair.Certs[0].NotBefore.Format(time.RFC3339)
 	annotations.NotAfter = certKeyPair.Certs[0].NotAfter.Format(time.RFC3339)
+	annotations.RefreshPeriod = durationRound(refresh)


Why is it okay to store an inaccurate duration? Won’t this value be used for debugging purposes?

pkg/operator/certrotation/annotations.go

p0lyn0mial · 2025-07-23T07:53:25Z

pkg/operator/certrotation/annotations.go

 	// NotAfter contains certificate the certificate validity date in RFC3339 format.
 	NotAfter string
+	// RefreshPeriod contains the interval at which the certificate should be refreshed.
+	RefreshPeriod string


In what format will the date be stored? I think this might be useful for tools that process the raw TLS info.

Its using time.Duration format: 8h or 30d or 10y

I think that the duration will be changed by the durationRound function which formats a duration into a human-readable string so machines won't be able to process it (if needed).

where will this field be used ?

Its informational only. We don't limit or verify this that it indeed happens at specified time (however we do ensure that this certificate is being rotated at all with ShortCertRotation featuregate)

I think it would be better if all dates were in the same format for consistency (NotBefore, NotAfter).

This way, the processing tools (the TLS script or cluster-debug-tools) would know what to expect and could reformat them into a more human-readable form if needed during output.

How hard would it be to move durationToString to the TLS registry script?

This isn't a date, it's a duration. Its not the time when next refresh occurs, it would be notAfter+refreshTime.

We can store them as milliseconds, but it won't be human-friendly - and if there is a need for a tool which reads this annotation we might as well convert "5s" into milliseconds too. The format is no longer lossy anyway

or just store duration.String(), at least it would be accepted by https://pkg.go.dev/time#ParseDuration

it would be then possible to convert to a human-readable form on the receiving end, before displaying to end users.

whereas 8y is not accepted by the time.PraseDuration - xref: https://go.dev/play/p/6qztJoDUlJJ

ya, that would store 72h00m00s instead of 3d in annotations, this is why we need a custom Durations.ToBetterString()

pkg/operator/certrotation/annotations.go

p0lyn0mial · 2025-07-23T07:55:22Z

pkg/operator/certrotation/annotations.go

 	// NotAfter contains certificate the certificate validity date in RFC3339 format.
 	NotAfter string
+	// RefreshPeriod contains the interval at which the certificate should be refreshed.
+	RefreshPeriod string


Also atm we store an inaccurate value in that field. Is that OK?

openshift/cluster-kube-apiserver-operator#1768 would set it for cluster-kube-apiserver-operator (see TLS artifacts in openshift/cluster-kube-apiserver-operator#1768):

{ "key": "certificates.openshift.io/refresh-period", "value": "8y" },

It also shows 6h refresh in 4.20 now, as we're using a faster refresh rotation in dev branch

vrutkovs · 2025-07-24T06:57:01Z

See follow-up in #1981

p0lyn0mial · 2025-07-24T07:17:59Z

See follow-up in #1981

LGTM

let's rebase this pr and merge it, and then let's merge the follow-up pr

vrutkovs · 2025-07-24T07:31:21Z

lets

/hold

it for now, as it needs to be tested in CKAO PR first

p0lyn0mial · 2025-07-24T07:56:39Z

pkg/operator/certrotation/target.go

 }

-// setTargetCertKeyPairSecret creates a new cert/key pair and sets them in the secret.  Only one of client, serving, or signer rotation may be specified.
+// setTargetCertKeyPairSecretAndTLSAnnotations creates a new cert/key pair and sets them in the secret.  Only one of client, serving, or signer rotation may be specified.


let's open a new pr for refactoring the target. i can do that. just let me know.

Sure, please do. I'll focus on e2e tests

Move testcase name out of auto-regenerate-after-offline-expiry, add refresh-period

p0lyn0mial · 2025-07-24T10:31:27Z

pkg/operator/certrotation/signer.go

 		}
 		c.EventRecorder.Eventf("SignerUpdateRequired", "%q in %q requires a new signing cert/key pair: %v", c.Name, c.Namespace, reason)
+		c.AdditionalAnnotations.RefreshPeriod = c.Refresh.String()
 		if err = setSigningCertKeyPairSecretAndTLSAnnotations(signingCertKeyPairSecret, c.Validity, c.AdditionalAnnotations); err != nil {


let's pass c.Refresh to setSigningCertKeyPairSecretAndTLSAnnotations and then to setTLSAnnotationsOnSigningCertKeyPairSecret so that annotation assignments are in one place for the signer.

actually, see #1971 (comment)

p0lyn0mial · 2025-07-24T10:32:44Z

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)
+		c.AdditionalAnnotations.RefreshPeriod = c.Refresh.String()
 		if err = setTargetCertKeyPairSecretAndTLSAnnotations(targetCertKeyPairSecret, c.Validity, signingCertKeyPair, c.CertCreator, c.AdditionalAnnotations); err != nil {


i would also pass c.Refresh to setTargetCertKeyPairSecretAndTLSAnnotations and then to setTLSAnnotationsOnTargetCertKeyPairSecret so that annotation assignments are in one place for the target.

actually, see #1971 (comment)

p0lyn0mial · 2025-07-24T10:33:44Z

pkg/operator/certrotation/annotations.go

+	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


do we need a unit test that checks assignment to the new annotation ?

I don't think we do - some certs are not being refreshed and if it's incorrectly set by the controller we'll see this in TLS registry violations

p0lyn0mial · 2025-07-24T10:45:11Z

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)
+		c.AdditionalAnnotations.RefreshPeriod = c.Refresh.String()


should this annotation be also set in line 83 ?

if !c.RefreshOnlyWhenExpired { needsMetadataUpdate := ensureOwnerRefAndTLSAnnotations(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) needsTypeChange := ensureSecretTLSTypeSet(signingCertKeyPairSecret) updateRequired = needsMetadataUpdate || needsTypeChange }

So far, I'd like to set refresh period only when contents are being refreshed. Later on we'll need a new PR which backfills this on existing certificates

What is the reason we don’t want to set the refresh annotation for existing certificates? Wouldn’t it be easier to set it now?

I think we have at least three options:

Update the callers to set the new annotation.

Update the annotation at the beginning of the EnsureSigningCertKeyPair method.

Modify the controller that calls EnsureSigningCertKeyPair to update the annotation.

I think option 2 is the easiest.
Setting the annotation at the beginning of the function ensures it is applied to both new and existing secrets.

I'd prefer a separate PR because this was not tested on existing PRs. If TRT comes and reverts because it breaks upgrades I'd rather have it already applied for new clusters at least.

I agree that 2. seems like the easiest option so far, but I didn't dig deep into this yet

vrutkovs · 2025-07-24T11:27:23Z

/hold cancel

p0lyn0mial · 2025-07-24T12:19:36Z

/lgtm

openshift-ci · 2025-07-24T12:22:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, p0lyn0mial, sjenning, vrutkovs

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/operator/OWNERS~~ [jsafrane,p0lyn0mial]

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

vrutkovs · 2025-07-24T12:24:21Z

/refresh

openshift-ci-robot · 2025-07-24T12:30:09Z

@vrutkovs: Jira Issue OCPBUGS-57049: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/cluster-kube-apiserver-operator#1873 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-57049 has not been moved to the MODIFIED state.

Details

In response to this:

Move testcase name out of auto-regenerate-after-offline-expiry, add
refresh-period.

Follow-up for openshift/origin#29327
Tested in openshift/cluster-kube-apiserver-operator#1768 and openshift/cluster-authentication-operator#742

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Jun 10, 2025

openshift-ci bot requested review from deads2k, jsafrane and wangke19 June 10, 2025 06:31

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2025

vrutkovs force-pushed the cert-annotation-update-jun10 branch from 94c3a48 to 5337ee8 Compare June 17, 2025 16:25

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2025

sjenning reviewed Jul 8, 2025

View reviewed changes

pkg/operator/certrotation/annotations.go Outdated Show resolved Hide resolved

pkg/operator/certrotation/utils.go Outdated Show resolved Hide resolved

vrutkovs force-pushed the cert-annotation-update-jun10 branch 2 times, most recently from d3b66d0 to b8e4d7e Compare July 9, 2025 09:54

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2025

openshift-ci bot assigned sjenning Jul 11, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2025

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2025

vrutkovs force-pushed the cert-annotation-update-jun10 branch from b8e4d7e to 3f5ff62 Compare July 11, 2025 15:45

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2025

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2025

vrutkovs force-pushed the cert-annotation-update-jun10 branch from 3f5ff62 to 56a5a40 Compare July 14, 2025 06:24

p0lyn0mial reviewed Jul 23, 2025

View reviewed changes

vrutkovs force-pushed the cert-annotation-update-jun10 branch 2 times, most recently from 2537111 to 78719bd Compare July 23, 2025 07:47

p0lyn0mial reviewed Jul 23, 2025

View reviewed changes

pkg/operator/certrotation/annotations.go Show resolved Hide resolved

p0lyn0mial reviewed Jul 23, 2025

View reviewed changes

vrutkovs force-pushed the cert-annotation-update-jun10 branch from 208caca to e6d67c1 Compare July 24, 2025 07:30

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2025

p0lyn0mial reviewed Jul 24, 2025

View reviewed changes

vrutkovs force-pushed the cert-annotation-update-jun10 branch from e6d67c1 to 8b6b76a Compare July 24, 2025 08:03

Update cert annotations

6864811

Move testcase name out of auto-regenerate-after-offline-expiry, add refresh-period

vrutkovs force-pushed the cert-annotation-update-jun10 branch from 8b6b76a to cc2229c Compare July 24, 2025 10:25

p0lyn0mial reviewed Jul 24, 2025

View reviewed changes

certrotation: set RefreshPeriod for signers and targets automatically

248e356

vrutkovs force-pushed the cert-annotation-update-jun10 branch from cc2229c to 248e356 Compare July 24, 2025 10:48

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2025

openshift-ci bot assigned p0lyn0mial Jul 24, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2025

openshift-merge-bot bot merged commit 03d85c4 into openshift:master Jul 24, 2025
4 checks passed

This was referenced Jul 24, 2025

OCPBUGS-44842: Set not-before/not-after annotations openshift/cluster-kube-apiserver-operator#1873

Merged

OCPBUGS-44842: Set not-before/not-after annotations openshift/cluster-authentication-operator#776

Open

openshift-ci-robot mentioned this pull request Jul 25, 2025

OCPBUGS-57049: cert inspection: add an option to humanize refresh period annotation #1981

Merged

openshift-ci-robot mentioned this pull request Sep 19, 2025

Re-apply "OCPBUGS-57049: certrotation: move test case name outside of AutoRegenerateAfterOfflineExpiry" openshift/cluster-kube-apiserver-operator#1910

Merged

OCPBUGS-57049: Update cert annotations #1971

OCPBUGS-57049: Update cert annotations #1971

Uh oh!

Conversation

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

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 10, 2025

Uh oh!

vrutkovs commented Jun 10, 2025

Uh oh!

vrutkovs commented Jun 18, 2025

Uh oh!

Uh oh!

Uh oh!

jsafrane commented Jul 11, 2025

Uh oh!

sjenning commented Jul 11, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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 Jul 23, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs commented Jul 24, 2025

Uh oh!

p0lyn0mial commented Jul 24, 2025

Uh oh!

vrutkovs commented Jul 24, 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!

vrutkovs commented Jun 10, 2025 •

edited

Loading

vrutkovs Jul 23, 2025 •

edited

Loading