CNTRLPLANE-343: certrotationcontroller: use minutes instead of days when FeatureShortCertRotation is enabled by vrutkovs · Pull Request #817 · openshift/cluster-kube-controller-manager-operator

vrutkovs · 2024-08-12T19:53:29Z

No description provided.

openshift-ci · 2024-08-12T19:53:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-bot · 2025-02-18T01:00:25Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-ci-robot · 2025-03-19T08:42:33Z

@vrutkovs: This pull request references CNTRLPLANE-343 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.19.0" version, but no target version was set.

Details

In response to this:

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-03-19T08:42:41Z

/remove-lifecycle stale

vrutkovs · 2025-04-22T11:32:14Z

/retest

vrutkovs · 2025-04-28T07:52:40Z

/retest-required

atiratree · 2025-06-19T11:20:31Z

-		rotationDay = day
-		klog.Warningf("!!! UNSUPPORTED VALUE SET !!!")
-		klog.Warningf("Certificate rotation base set to %q", rotationDay)
+	monthPeriod := time.Hour * 24 * 30


let's remove the defaultRotationDay constant if we do not need it anymore

const is still present

atiratree · 2025-06-19T11:25:31Z

-			Validity:               30 * rotationDay,
-			Refresh:                15 * rotationDay,
+			Validity:               monthPeriod,
+			Refresh:                monthPeriod / 2,


might be useful to have different variables for the CA and the signer

Right, in ckao we have "month", "tenMonth" and "year" periods - so we can combine those.

I think a single refresh period is okay for now, it can be extended later

atiratree · 2025-06-19T11:30:37Z

+		return nil, fmt.Errorf("unable to get FeatureGates: %w", err)
+	}
+
+	if featureGates.Enabled(features.FeatureShortCertRotation) {


I guess we do not support disabling the FG during runtime. Note: FeatureShortCertRotation is DevPreviewNoUpgrade currently and it seems the plan is to use it only for testing?

Maybe we could add a simple comment above to document the use case?

Good idea, added

atiratree · 2025-06-19T11:54:02Z

-		// we need to establish some kind of delay or back pressure to prevent the rollout.  This ensures we don't trigger kas restart
-		// during e2e tests for now.
-		certRotationScale*8,
+		featureGateAccessor,


The Feature is the direct successor of the certRotationScale right?

Are these comments by David obsolete? I suppose it was tuned to work with a specific scale and specific hard-coded value of 8 before.

Can we run into a similar problem with 2h/1h rotation?

Its a replacement for cert rotation scale, yes.

The comments are obsolete - instead of tweaking scales in some operators we define a featuregate observed by every operator. The feature gate also makes your cluster unsupportable (unlike cert scale config via "secret" configmap). And also we run a test which ensures that no certs issued are longer than 8 hrs - so the config is platform wide.

Can we run into a similar problem with 2h/1h rotation?

Its possible - but we have e2e tests which ensure that rollouts happen without disruption

Ok, great to hear that we have it covered and can remove the scale+comment.

atiratree · 2025-06-19T12:01:05Z

+		required.Spec.Containers[i].Env = append(container.Env, corev1.EnvVar{
+			Name:  "OPERATOR_IMAGE_VERSION",
+			Value: operatorImageVersion,
+		})


What do we need the OPERATOR_IMAGE_VERSION env var for?

Operands needs current operator version to make sure observed feature gates are from expected version

Don't we pass the feature gates to operands directly? Or is this something planned? Or do you have a link where this is used?

NewFeatureGateAccessor needs desiredVersion

https://github.com/vrutkovs/cluster-kube-controller-manager-operator/blob/84b4a485b6676ac21a0437821d8c06d5940b794a/pkg/operator/starter.go#L87

which is fetched by calling status.VersionForOperatorFromEnv():

https://github.com/vrutkovs/cluster-kube-controller-manager-operator/blob/84b4a485b6676ac21a0437821d8c06d5940b794a/pkg/operator/starter.go#L82

which reads env var value:

cluster-kube-controller-manager-operator/vendor/github.com/openshift/library-go/pkg/operator/status/version.go

Lines 76 to 78 in 0740142

func VersionForOperatorFromEnv() string {

return os.Getenv(operatorImageVersionEnvVarName)

}

cluster-kube-controller-manager-operator/vendor/github.com/openshift/library-go/pkg/operator/status/version.go

Line 24 in 0740142

operatorImageVersionEnvVarName = "OPERATOR_IMAGE_VERSION"

But this is used only for the operator command (RunOperator) and not for any of the operand containers as far as I can see.

I am not against adding the OPERATOR_IMAGE_VERSION env var if we plan to use it. But if not, I would prefer to keep the KCM pod cleaner/simpler.

Oh, right, we no longer need it (featuregates are now hardcoded in operand sidecar)

Ok, we can add the env later if we find the need.

atiratree · 2025-06-19T16:26:31Z

+
+	if featureGates.Enabled(features.FeatureShortCertRotation) {
+		refreshPeriod = time.Hour * 2
+		klog.Infof("Setting monthPeriod to %v", refreshPeriod)


Suggested change

klog.Infof("Setting monthPeriod to %v", refreshPeriod)

klog.Infof("Setting refreshPeriod to %v", refreshPeriod)

atiratree · 2025-06-19T16:27:01Z

-		rotationDay = day
-		klog.Warningf("!!! UNSUPPORTED VALUE SET !!!")
-		klog.Warningf("Certificate rotation base set to %q", rotationDay)
+	monthPeriod := time.Hour * 24 * 30


const is still present

…CertRotation is enabled

atiratree · 2025-06-20T10:32:32Z

As discussed #817 (comment), it seems we can remove the whole last commit now: a746627

atiratree · 2025-06-20T10:56:45Z

Thanks!

/lgtm

openshift-ci · 2025-06-20T10:57:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, 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:

~~OWNERS~~ [atiratree]

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

openshift-ci-robot · 2025-06-20T12:26:38Z

/retest-required

Remaining retests: 0 against base HEAD 0740142 and 2 for PR HEAD 50e9fe2 in total

vrutkovs · 2025-06-20T16:25:28Z

/test e2e-aws-ovn

openshift-ci-robot · 2025-06-20T18:49:31Z

/retest-required

Remaining retests: 0 against base HEAD 0740142 and 2 for PR HEAD 50e9fe2 in total

openshift-ci · 2025-06-20T21:04:15Z

@vrutkovs: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-scos-e2e-aws-ovn	`50e9fe2`	link	false	`/test okd-scos-e2e-aws-ovn`

Full PR test history. Your PR dashboard.

Details

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-bot · 2025-06-20T22:59:08Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-kube-controller-manager-operator
This PR has been included in build ose-cluster-kube-controller-manager-operator-container-v4.20.0-202506202242.p0.g4c58d4c.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2024

vrutkovs force-pushed the short-rotation-featuregate branch 2 times, most recently from cbc637b to 455f645 Compare August 16, 2024 09:36

vrutkovs mentioned this pull request Sep 3, 2024

Add Short Rotation Period For Certificates openshift/enhancements#1670

Closed

vrutkovs force-pushed the short-rotation-featuregate branch from 455f645 to c5c2087 Compare November 7, 2024 09:07

vrutkovs force-pushed the short-rotation-featuregate branch 3 times, most recently from 79eac26 to 1a59b1d Compare November 19, 2024 13:51

openshift-ci Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 18, 2025

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

vrutkovs force-pushed the short-rotation-featuregate branch from 1a59b1d to 84869ad Compare March 19, 2025 08:41

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

vrutkovs changed the title ~~certrotationcontroller: use minutes instead of days when FeatureShortCertRotation is enabled~~ CNTRLPLANE-343: certrotationcontroller: use minutes instead of days when FeatureShortCertRotation is enabled Mar 19, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 19, 2025

vrutkovs marked this pull request as ready for review March 19, 2025 08:42

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2025

openshift-ci Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 19, 2025

openshift-ci Bot requested review from atiratree and ingvagabund March 19, 2025 08:43

vrutkovs force-pushed the short-rotation-featuregate branch from 84869ad to 4b82d7b Compare March 19, 2025 09:42

vrutkovs changed the title ~~CNTRLPLANE-343: certrotationcontroller: use minutes instead of days when FeatureShortCertRotation is enabled~~ WIP CNTRLPLANE-343: certrotationcontroller: use minutes instead of days when FeatureShortCertRotation is enabled Mar 19, 2025

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2025

vrutkovs changed the title ~~WIP CNTRLPLANE-343: certrotationcontroller: use minutes instead of days when FeatureShortCertRotation is enabled~~ CNTRLPLANE-343: certrotationcontroller: use minutes instead of days when FeatureShortCertRotation is enabled Mar 19, 2025

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2025

vrutkovs force-pushed the short-rotation-featuregate branch 2 times, most recently from 7ce56f6 to 0d18cbc Compare March 21, 2025 09:45

vrutkovs force-pushed the short-rotation-featuregate branch 3 times, most recently from a9cf919 to 8b1f4fc Compare April 22, 2025 08:03

vrutkovs force-pushed the short-rotation-featuregate branch 2 times, most recently from df6e4c4 to 22bfdea Compare May 15, 2025 08:58

Bump library-go

a259355

vrutkovs force-pushed the short-rotation-featuregate branch 2 times, most recently from 63ea291 to 496cfce Compare June 18, 2025 10:28

atiratree reviewed Jun 19, 2025

View reviewed changes

vrutkovs force-pushed the short-rotation-featuregate branch from 15a364e to 84b4a48 Compare June 19, 2025 13:10

atiratree reviewed Jun 19, 2025

View reviewed changes

certrotationcontroller: use minutes instead of days when FeatureShort…

50e9fe2

…CertRotation is enabled

vrutkovs force-pushed the short-rotation-featuregate branch 2 times, most recently from fa01763 to a746627 Compare June 20, 2025 09:42

vrutkovs force-pushed the short-rotation-featuregate branch from a746627 to 50e9fe2 Compare June 20, 2025 10:48

openshift-ci Bot assigned atiratree Jun 20, 2025

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2025

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2025

openshift-merge-bot Bot merged commit 4c58d4c into openshift:master Jun 20, 2025
10 of 11 checks passed

vrutkovs mentioned this pull request Jun 21, 2025

Revert "CNTRLPLANE-343: certrotationcontroller: use minutes instead of days when FeatureShortCertRotation is enabled" #842

Merged

	func VersionForOperatorFromEnv() string {
	return os.Getenv(operatorImageVersionEnvVarName)
	}

	klog.Infof("Setting monthPeriod to %v", refreshPeriod)
	klog.Infof("Setting refreshPeriod to %v", refreshPeriod)

Conversation

vrutkovs commented Aug 12, 2024

Uh oh!

openshift-ci Bot commented Aug 12, 2024

Uh oh!

openshift-bot commented Feb 18, 2025

Uh oh!

openshift-ci-robot commented Mar 19, 2025 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrutkovs commented Mar 19, 2025

Uh oh!

vrutkovs commented Apr 22, 2025

Uh oh!

vrutkovs commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree Jun 19, 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!

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!

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!

vrutkovs Jun 20, 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!

Choose a reason for hiding this comment

Uh oh!

atiratree Jun 19, 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!

atiratree commented Jun 20, 2025

Uh oh!

atiratree commented Jun 20, 2025

Uh oh!

openshift-ci Bot commented Jun 20, 2025

Uh oh!

openshift-ci-robot commented Jun 20, 2025

Uh oh!

vrutkovs commented Jun 20, 2025

Uh oh!

openshift-ci-robot commented Jun 20, 2025

Uh oh!

openshift-ci Bot commented Jun 20, 2025

Uh oh!

Uh oh!

openshift-bot commented Jun 20, 2025

Uh oh!

openshift-ci-robot commented Mar 19, 2025 •

edited by openshift-ci Bot

Loading

atiratree Jun 19, 2025 •

edited

Loading

vrutkovs Jun 20, 2025 •

edited

Loading

atiratree Jun 19, 2025 •

edited

Loading