Re-apply "OCPBUGS-57049: certrotation: move test case name outside of AutoRegenerateAfterOfflineExpiry" #1910
Conversation
…ide of AutoRegenerateAfterOfflineExpiry""
openshift-ci-robot
added
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. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMoves human-readable test descriptors out of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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 (
|
openshift-ci
bot
added
the
approved
|
/payload-job periodic-ci-openshift-release-master-ci-4.20-upgrade-from-stable-4.19-e2e-aws-ovn-upgrade |
|
@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/542e13b0-843e-11f0-8b8d-c212b527c36b-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/certrotationcontroller/certrotationcontroller.go (1)
369-375: Typo in Description: “kuberentes.default.svc” → “kubernetes.default.svc”.User-facing metadata; please correct.
Apply:
- 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).",
🧹 Nitpick comments (2)
pkg/operator/certrotationcontroller/certrotationcontroller.go (2)
456-458: Inconsistent TestName vs. the rest of the ExternalLoadBalancerServing group.Signer/CA use the “api-int endpoint” test; the serving cert uses a Deployment test. If not intentional, align for consistency.
Potential fix:
- TestName: "[sig-apps] Deployment RollingUpdateDeployment should delete old pods and create new ones [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]", + TestName: "[Conformance][sig-api-machinery][Feature:APIServer] kube-apiserver should be accessible via api-int endpoint [Suite:openshift/conformance/parallel/minimal]",
153-210: Consider de-duplicating repeated TestName strings within each 3-resource group.Define per-group constants and reuse across signer/CA/certkey to avoid drift.
📜 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 (3)
pkg/operator/certrotationcontroller/certrotationcontroller.go (3)
161-163: Separation of TestName from AutoRegenerateAfterOfflineExpiry looks good (applies to all AdditionalAnnotations blocks).This improves clarity and avoids overloading the URL field. No functional impact observed.
161-163: Verified: AdditionalAnnotations.TestName exists in library-go v0.0.0-20250729191057-91376e1b394e
AdditionalAnnotations struct in the vendored library-go includes the TestName field and go.mod points to a version containing it; no further action required.
418-420: Confirm no write contention on loadbalancer-serving CA/signer
Both the targetconfigcontroller (sync) and certrotationcontroller (rotate) intentionally referenceloadbalancer-serving-ca/loadbalancer-serving-signer. Please verify that PR #1901 (release-4.19) removed any conflicting annotation updates between these controllers across all rotation/sync blocks (lines 418–420, 441–443, 486–488, 509–511).
| TestName: "[sig-cli] Kubectl logs logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]", | ||
| AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631", | ||
| }, |
There was a problem hiding this comment.
Fix typo in test name: duplicated “logs”.
TestName has “Kubectl logs logs…”. Drop the duplicate “logs” in all three occurrences.
Apply:
- TestName: "[sig-cli] Kubectl logs logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]",
+ TestName: "[sig-cli] Kubectl logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]",Also applies to: 239-241, 254-256
🤖 Prompt for AI Agents
In pkg/operator/certrotationcontroller/certrotationcontroller.go around lines
220-222 (and also at 239-241 and 254-256), the TestName strings contain a
duplicated word "logs" ("Kubectl logs logs…"); remove the extra "logs" in each
occurrence so the string reads "Kubectl logs should be able to retrieve and
filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal]
[Suite:k8s]". Ensure all three occurrences are updated identically and run
tests/lint to confirm no formatting or string literal issues.
kuberentes -> kubernetes
|
/payload-job periodic-ci-openshift-release-master-ci-4.20-upgrade-from-stable-4.19-e2e-aws-ovn-upgrade |
|
@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/21dea790-849c-11f0-96fc-cb92a003d10c-0 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…rtificate keypair
|
/payload-job periodic-ci-openshift-release-master-ci-4.20-upgrade-from-stable-4.19-e2e-aws-ovn-upgrade |
|
@vrutkovs: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f02965c0-8700-11f0-97ac-51bc5a2351bb-0 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/operator/certrotationcontroller/certrotationcontroller.go (2)
220-222: Fix typo in TestName: duplicated “logs” and extra space before [Conformance].Drop the extra “logs” and collapse the double space in all three places.
- TestName: "[sig-cli] Kubectl logs logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]", + TestName: "[sig-cli] Kubectl logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]",- TestName: "[sig-cli] Kubectl logs logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]", + TestName: "[sig-cli] Kubectl logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]",- TestName: "[sig-cli] Kubectl logs logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]", + TestName: "[sig-cli] Kubectl logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]",Also applies to: 239-241, 254-256
656-658: Resolved: control-plane-node-admin client description fixed.Now accurately describes the client cert purpose and subject (system:control-plane-node-admin).
Also applies to: 715-717, 773-776
🧹 Nitpick comments (2)
pkg/operator/certrotationcontroller/certrotationcontroller.go (2)
418-420: Verify ExternalLoadBalancer TestName targets the external endpoint.
- The strings mention “api-int endpoint”, which is the internal LB. For ExternalLoadBalancer, this likely should be “api endpoint”.
- The cert’s TestName currently points to a Deployment RollingUpdate test, which seems unrelated to LB reachability. Please confirm the intended test mapping.
If “api” is correct for ExternalLoadBalancer, apply:
- TestName: "[Conformance][sig-api-machinery][Feature:APIServer] kube-apiserver should be accessible via api-int endpoint [Suite:openshift/conformance/parallel/minimal]", + TestName: "[Conformance][sig-api-machinery][Feature:APIServer] kube-apiserver should be accessible via api endpoint [Suite:openshift/conformance/parallel/minimal]",- TestName: "[Conformance][sig-api-machinery][Feature:APIServer] kube-apiserver should be accessible via api-int endpoint [Suite:openshift/conformance/parallel/minimal]", + TestName: "[Conformance][sig-api-machinery][Feature:APIServer] kube-apiserver should be accessible via api endpoint [Suite:openshift/conformance/parallel/minimal]",For the cert at Lines 456-458, please update TestName to the correct external API reachability test (or confirm current intent). I can adjust the exact string if you point to the canonical test name in your suite.
Also applies to: 441-443, 456-458
624-626: Broaden kube-control-plane-signer/-ca descriptions to reflect all consumers.The signer/CA are also used by control-plane-node-admin and check-endpoints clients, not only KCM and scheduler. Clarify to avoid confusion.
- Description: "Signer for kube-controller-manager and kube-scheduler client certificates.", + Description: "Signer for kube-controller-manager, kube-scheduler, control-plane node admin, and check-endpoints client certificates.",- Description: "CA for kube-apiserver to recognize the kube-controller-manager and kube-scheduler client certificates.", + Description: "CA for kube-apiserver to recognize kube-controller-manager, kube-scheduler, control-plane node admin, and check-endpoints client certificates.",Also applies to: 641-643, 683-685, 700-702, 741-744, 758-761, 801-803, 818-820
📜 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 (7)
pkg/operator/certrotationcontroller/certrotationcontroller.go (7)
161-163: LGTM: TestName/AutoRegenerate annotations added for aggregator client signer/CA/client.Consistent migration of human-readable test descriptors into TestName. No functional impact.
Also applies to: 178-180, 193-195
283-285: LGTM: LocalhostServing annotations.Appropriate TestName and regeneration link; metadata mirrors signer/CA/cert consistently.
Also applies to: 306-308, 321-323
350-352: LGTM: ServiceNetworkServing annotations and typo fix.“kubernetes.default.svc” spelling corrected and TestName aligns with service network access test.
Also applies to: 370-375, 388-390
486-488: LGTM: InternalLoadBalancer annotations.Uses “api-int endpoint” as expected for internal LB; consistent across signer/CA/cert.
Also applies to: 509-511, 525-526
554-556: LGTM: LocalhostRecoveryServing annotations.Consistent TestName and regeneration link; matches localhost-recovery SNI purpose.
Also applies to: 577-579, 593-595
833-835: LGTM: CheckEndpoints client annotations.TestName aligns with presence of check-endpoints.kubeconfig in kube-apiserver containers.
860-862: LGTM: NodeSystemAdmin annotations.Appropriate linkage to localhost-recovery flow and longer validity rationale maintained.
Also applies to: 879-881, 894-896
| JiraComponent: "kube-apiserver", | ||
| AutoRegenerateAfterOfflineExpiry: "https://github.com/openshift/cluster-kube-apiserver-operator/pull/1631,'[sig-cli] Kubectl logs logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]'", | ||
| Description: "CA for the kubelet to recognize the kube-apiserver client certificate.", | ||
| TestName: "[sig-cli] Kubectl logs logs should be able to retrieve and filter logs [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]", |
There was a problem hiding this comment.
coderabbitai points out duplicate logs in test name, and extra space logs [Conformance]
There was a problem hiding this comment.
Yes, this is actual test name though - it's comprised of suite "Kubectl logs", test group "logs" and test name " should be able to retrieve"
|
@cursor help |
|
Unable to authenticate your request. Please make sure to connect your GitHub account to Cursor. Go to Cursor |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrutkovs, wangke19 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 |
|
/retest-required |
|
/cherry-pick release-4.20 |
|
@vrutkovs: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/verified later @wangke19 |
1 similar comment
|
/verified later @wangke19 |
openshift-ci-robot
added
verified-later
verified
|
@wangke19: This PR has been marked to be verified later by 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: 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-merge-bot
bot
merged commit 5c58090
into
openshift:main
|
@vrutkovs: Jira Issue OCPBUGS-57049: Some pull requests linked via external trackers have merged:
The following pull request, linked via external tracker, has not merged: All associated pull requests must be merged or 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. This PR is marked as verified-later. Jira issue(s) in the title of this PR will require post-merge verification. After testing, it must be manually moved to the 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: new pull request created: #1924 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 kubernetes-sigs/prow repository. |
Reverts #1907
After #1901 has landed in 4.19 now controllers are no longer racing on annotations update
Summary by CodeRabbit