OCPBUGS-57049: certrotation: move test case name outside of AutoRegenerateAfterOfflineExpiry #1870
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
approved
vrutkovs
force-pushed
the
tls-details-update-july
branch
from
d6f3f9f to
ae1ed86
Compare
vrutkovs
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-57049, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/valid-bug
vrutkovs
force-pushed
the
tls-details-update-july
branch
from
ae1ed86 to
6f1a79b
Compare
|
/retest-required |
vrutkovs
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/retest |
vrutkovs
changed the title
|
/retest |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
vrutkovs
force-pushed
the
tls-details-update-july
branch
from
cafa594 to
6ca45e9
Compare
|
/retest |
|
/retest-required |
|
/retest-required |
1 similar comment
|
/retest-required |
2 similar comments
vrutkovs
force-pushed
the
tls-details-update-july
branch
from
6ca45e9 to
89d5f4d
Compare
|
/retest-required |
| Name: "aggregator-client-signer", | ||
| AdditionalAnnotations: certrotation.AdditionalAnnotations{ | ||
| JiraComponent: "kube-apiserver", | ||
| AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'[sig-cli] oc adm new-project [apigroup:project.openshift.io][apigroup:authorization.openshift.io] [Suite:openshift/conformance/parallel]'", |
There was a problem hiding this comment.
are there any other changes except moving the test name to the new field ?
There was a problem hiding this comment.
Nope, squashed two commits here (the second commit was fixing issues introduced by the first one, so no need to keep it around)
| AdditionalAnnotations: certrotation.AdditionalAnnotations{ | ||
| JiraComponent: "kube-apiserver", | ||
| AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'[Conformance][sig-api-machinery][Feature:APIServer] local kubeconfig \"control-plane-node.kubeconfig\" should be present in all kube-apiserver containers [Suite:openshift/conformance/parallel/minimal]'", | ||
| Description: "kube-controller-manager and kube-scheduler client certificates.", |
There was a problem hiding this comment.
from the PR description:
This PR also updates conflicting descriptions to kube-control-plane-signer secret to make sure controllers don't hotloop erasing each other changes
does adding a description prevents controllers from erasing changes added by the others controllers?
There was a problem hiding this comment.
Any metadata change needs to make sure that all controllers set the same set of metadata to prevent hotlooping
vrutkovs
force-pushed
the
tls-details-update-july
branch
from
89d5f4d to
51bfa4b
Compare
WalkthroughA new field TestName was added to certrotation.AdditionalAnnotations. Usages in cert rotation configurations were updated so TestName holds a human-readable test name, while AutoRegenerateAfterOfflineExpiry now contains only the PR URL. No certificate rotation logic or control flow changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-57049, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/operator/certrotationcontroller/certrotationcontroller.go (4)
340-365: Fix minor typo in service-network CA description.Description currently says “kuberentes.default.svc”; should be “kubernetes.default.svc”.
Apply this diff:
- Description: "CA for recognizing the kube-apiserver when connecting via the service network (kuberentes.default.svc)." + Description: "CA for recognizing the kube-apiserver when connecting via the service network (kubernetes.default.svc)."Also applies to: 366-382, 383-403
734-753: Correct Description for control-plane-node-admin client secret.The Description for control-plane-node-admin client (Line 773) says “kube-controller-manager and kube-scheduler client certificates.” That appears to be a copy/paste from the control-plane signer, but this secret is for system:control-plane-node-admin. It’s misleading and could confuse operators.
Apply this diff:
- Description: "kube-controller-manager and kube-scheduler client certificates.", + Description: "Client certificate used by the control-plane node admin (system:control-plane-node-admin) to authenticate to the kube-apiserver for local recovery and debugging.",Also applies to: 754-768, 769-790
153-163: Consistency check: TestName vs. PR URL.Across all updated blocks, TestName is used solely for the human-readable test case and AutoRegenerateAfterOfflineExpiry contains only the PR URL (currently PR 1631). This matches the PR goal to avoid repeating the test name in other annotations. If the intent is to reference the PR that originally introduced the auto-regenerate semantics (1631), then keeping 1631 everywhere is correct. If you prefer the “latest touch” provenance, consider changing the URL to this PR (1870) for traceability, but keep it consistent across all writers of the same object.
If you want to prevent accidental drift in the future, consider centralizing these repeated AdditionalAnnotations in small helper functions/constants per signer family, e.g.:
func controlPlaneSignerAnnotations() certrotation.AdditionalAnnotations { return certrotation.AdditionalAnnotations{ JiraComponent: "kube-apiserver", Description: "Signer for kube-controller-manager and kube-scheduler client certificates.", TestName: "[sig-apps] Deployment RollingUpdateDeployment should delete old pods and create new ones [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]", AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631", } }Then reuse it in all places that reference kube-control-plane-signer and its CA to eliminate future drift.
Also applies to: 176-180, 191-195, 220-222, 239-241, 254-256, 283-285, 306-308, 321-323, 350-352, 373-375, 388-390, 418-420, 441-443, 456-458, 486-488, 509-511, 524-526, 554-556, 577-579, 593-595, 624-626, 641-643, 656-658, 683-685, 701-702, 715-717, 742-744, 759-761, 773-776, 801-803, 818-820, 833-835, 860-862, 879-881, 894-896
153-163: Naming in PR description vs. code: TestCase vs. TestName.The PR description mentions a “TestCase” annotation, but the code uses the field name “TestName” in certrotation.AdditionalAnnotations. Assuming library-go added TestName, the code is correct. Consider updating the PR description to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/operator/certrotationcontroller/certrotationcontroller.go(36 hunks)
🔇 Additional comments (11)
pkg/operator/certrotationcontroller/certrotationcontroller.go (11)
153-172: LGTM: Test name moved into AdditionalAnnotations.TestName; URL-only in AutoRegenerateAfterOfflineExpiry.The AggregatorProxyClientCert set (signer/CA/client) now cleanly separates a human-readable TestName from the PR URL. Values are consistent across the three resources. No behavioral changes beyond annotations.
Also applies to: 173-187, 188-209
212-233: LGTM: kube-apiserver-to-kubelet set updated consistently.All three resources now use TestName for the e2e reference and keep AutoRegenerateAfterOfflineExpiry as the original PR URL. No drift spotted among the three blocks.
Also applies to: 234-248, 249-270
273-298: LGTM: localhost-serving set annotations normalized.TestName placement and URL-only AutoRegenerateAfterOfflineExpiry look correct and consistent in signer/CA/client resources.
Also applies to: 299-315, 316-337
408-433: LGTM: external load balancer set annotations normalized.Consistent TestName and URL usage across signer/CA/client. No discrepancies found.
Also applies to: 434-450, 451-473
476-501: LGTM: internal load balancer set annotations normalized.Consistent annotations; mirrors the external LB set appropriately.
Also applies to: 502-518, 519-541
544-569: LGTM: localhost-recovery set annotations normalized.TestName now contains the localhost-recovery.kubeconfig test; URL remains in AutoRegenerateAfterOfflineExpiry. Looks good.
Also applies to: 570-586, 587-610
616-635: LGTM: kube-control-plane-signer (controller-manager client) — descriptions aligned.The signer and its CA use identical Description strings, which is important since multiple reconcilers reference the same signer secret. Good step toward preventing hotloops.
Also applies to: 636-649, 650-670
675-694: LGTM: kube-control-plane-signer (scheduler client) — descriptions aligned.Matches the controller-manager block; no drift in TestName, Description, or URL fields.
Also applies to: 695-709, 710-732
793-812: LGTM: check-endpoints client set normalized.Annotation values are consistent; TestName references the correct conformance test for check-endpoints.
Also applies to: 813-826, 827-849
852-873: LGTM: node-system-admin signer/CA/client annotations normalized.All three use the localhost-recovery-related TestName and consistent descriptions. Good.
Also applies to: 874-887, 888-914
616-626: Verified identical descriptions across reconcilers — no remaining driftI’ve confirmed that the
Description(and otherAdditionalAnnotations) for both thekube-control-plane-signerSecret and itskube-control-plane-signer-caConfigMap are identical across all four reconcilers (KubeControllerManagerClient, KubeSchedulerClient, ControlPlaneNodeAdminClient, CheckEndpointsClient). This alignment ensures the controllers will converge on the same content and prevents the hot-loop “erase each other” behavior.Approving these changes.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, sanchezl, vrutkovs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@vrutkovs: The following tests failed, say
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-merge-bot
bot
merged commit 9649be0
into
openshift:main
|
@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: 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 Issue OCPBUGS-57049 has not been moved to the MODIFIED state. 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. |
4 participants
Move out test name into "TestCase" annotation so that AutoRotate... would contain just the PR URL when it was added and other annotations didn't have to repeat the testcase used.
This PR also updates conflicting descriptions to
kube-control-plane-signersecret to make sure controllers don't hotloop erasing each other changesSummary by CodeRabbit