OCPBUGS-85367: controllercmd,certsyncpod: add UserAgentSuffix to distinguish clients#2273
OCPBUGS-85367: controllercmd,certsyncpod: add UserAgentSuffix to distinguish clients#2273tchap wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@tchap: This pull request references Jira Issue OCPBUGS-85367, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR extends the ControllerBuilder pattern to support customizable REST client User-Agent suffixes. A new builder method and configuration field enable controllers to append custom identifiers to their default User-Agent headers. The mechanism is tested and applied in both a generic configuration layer and a specific cert-sync component. ChangesUser-Agent customization support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@tchap: This pull request references Jira Issue OCPBUGS-85367, which is valid. 3 validation(s) were run on this bug
DetailsIn 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: 1
🧹 Nitpick comments (1)
pkg/controller/controllercmd/builder_test.go (1)
271-293: ⚡ Quick winTest duplicates Run()'s logic rather than exercising it.
Lines 277-279 re-implement the exact conditional from
Run(if len(...) > 0 { rest.AddUserAgent(...) }). The test therefore validates thatWithUserAgentSuffixstores the field and thatrest.AddUserAgentbehaves as expected, but it does not cover the production code path inRun. If the suffix application were removed fromRun, this test would still pass.Consider asserting against the field directly (and/or covering
Run's application path), so the test fails ifRunstops applying the suffix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/controllercmd/builder_test.go` around lines 271 - 293, The test is duplicating Run's behavior instead of exercising it; update the test in builder_test.go to assert the controller's userAgentSuffix field directly (access b.userAgentSuffix) to verify WithUserAgentSuffix stored the value, and add a subtest that calls the controller's Run method (or the function that applies the suffix to a rest.Config) to verify Run actually applies the suffix to cfg.UserAgent rather than reimplementing the if len(...) > 0 { rest.AddUserAgent(...) } conditional; reference NewController, WithUserAgentSuffix, Run and the userAgentSuffix field when locating the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/controllercmd/builder.go`:
- Around line 111-113: The field comment for userAgentSuffix incorrectly
references a non-existent method name; update the doc comment so it points to
the actual builder method WithUserAgentSuffix (or remove the reference). Locate
the userAgentSuffix field and change the text "Set via WithUserAgent." to "Set
via WithUserAgentSuffix." (or equivalent accurate wording) so the comment
matches the real API (userAgentSuffix and WithUserAgentSuffix).
---
Nitpick comments:
In `@pkg/controller/controllercmd/builder_test.go`:
- Around line 271-293: The test is duplicating Run's behavior instead of
exercising it; update the test in builder_test.go to assert the controller's
userAgentSuffix field directly (access b.userAgentSuffix) to verify
WithUserAgentSuffix stored the value, and add a subtest that calls the
controller's Run method (or the function that applies the suffix to a
rest.Config) to verify Run actually applies the suffix to cfg.UserAgent rather
than reimplementing the if len(...) > 0 { rest.AddUserAgent(...) } conditional;
reference NewController, WithUserAgentSuffix, Run and the userAgentSuffix field
when locating the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f2134fc8-d6b1-418e-af95-1a33cb87c60e
📒 Files selected for processing (4)
pkg/controller/controllercmd/builder.gopkg/controller/controllercmd/builder_test.gopkg/controller/controllercmd/cmd.gopkg/operator/staticpod/certsyncpod/certsync_cmd.go
|
Overall the approach seems reasonable. It would help me if you could provide a list of the components you'd like to change, or even just one component you'd like to modify. |
There is already a testing PR for KCM-O: openshift/cluster-kube-controller-manager-operator#940 . The Jira issue mentions KAS-O, KCM-O and the scheduler operator, but I haven't checked other components. All these three need a very similar PR as this testing one for KCM-O. Then we need to align the skip list in the API server. And by skip list I mean to extend the logic as in openshift/kubernetes#376 |
Add WithUserAgentSuffix to ControllerBuilder and expose UserAgentSuffix on ControllerCommandConfig so that static pod sidecar controllers using localhost can be identified by the kube-apiserver's pre-readiness request filter. The cert-syncer, which does not use controllercmd, sets its suffix directly in Complete().
|
@tchap: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
3 participants
Add
WithUserAgentSuffixtoControllerBuilderand exposeUserAgentSuffixonControllerCommandConfigso that static pod sidecar controllers using localhost can be identified by the kube-apiserver's pre-readiness request filter.The
cert-syncer, which does not usecontrollercmd, sets its suffix directly inComplete().Note that this does not generate super pretty
UserAgentstrings. When you run the test, you get something likeSo to check for a particular component, you need to do `HasPrefix("cluster-kube-controller-manager-operator") && HasSuffix(/cert-syncer), for example.
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation / Chores