MGMT-24693: bracket IPv6 in OAuth issuer and callback URLs - #9120
MGMT-24693: bracket IPv6 in OAuth issuer and callback URLs#9120andrej1991 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughOAuth callback URL templates and OAuth metadata URLs now construct host-port values with Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 7❌ Failed checks (7 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andrej1991 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go (1)
41-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required “When ... it should ...” test-name format.
Rename the three new cases to include
it should, for example:When OAuth host is IPv6, it should bracket the address.As per coding guidelines,
**/*_test.go: Always use “When ... it should ...” format for describing test cases when creating unit tests.Proposed rename
- name: "When OAuth host is IPv4, issuer URLs should not use brackets", + name: "When OAuth host is IPv4, it should leave issuer URLs unbracketed", - name: "When OAuth host is IPv6, issuer URLs should bracket the address", + name: "When OAuth host is IPv6, it should bracket the address", - name: "When OAuth host is a hostname, issuer URLs should remain unbracketed", + name: "When OAuth host is a hostname, it should leave issuer URLs unbracketed",🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go` around lines 41 - 79, Rename the three new test cases in the OAuth test table so each follows the required “When ..., it should ...” format: the IPv4 case should state it should not use brackets, the IPv6 case should state it should bracket the address, and the hostname case should state it should remain unbracketed. Keep the test logic and expectations unchanged.Source: Coding guidelines
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
765-767: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd or confirm unit coverage for the callback template.
The supplied tests cover
adaptOauthMetadata, but do not exercise this controller branch. Verify that an existing controller test covers an IPv6OAuthHost; otherwise add one to prevent regressions inOAuthCallbackURLTemplate.As per coding guidelines,
**/*_test.go: Unit test any code changes and additions.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 765 - 767, Add or update a unit test for the controller branch assigning OAuthCallbackURLTemplate, using an IPv6 infraStatus.OAuthHost and verifying the resulting URL brackets the host and includes the OAuth port. Reuse existing controller test setup and confirm the expected callback path and identity-provider placeholder.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 765-767: Add or update a unit test for the controller branch
assigning OAuthCallbackURLTemplate, using an IPv6 infraStatus.OAuthHost and
verifying the resulting URL brackets the host and includes the OAuth port. Reuse
existing controller test setup and confirm the expected callback path and
identity-provider placeholder.
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go`:
- Around line 41-79: Rename the three new test cases in the OAuth test table so
each follows the required “When ..., it should ...” format: the IPv4 case should
state it should not use brackets, the IPv6 case should state it should bracket
the address, and the hostname case should state it should remain unbracketed.
Keep the test logic and expectations unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ec9e07c8-82f1-41f6-af9f-c5c171a31fe8
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9120 +/- ##
==========================================
+ Coverage 44.51% 44.53% +0.01%
==========================================
Files 774 774
Lines 96997 97008 +11
==========================================
+ Hits 43179 43198 +19
+ Misses 50830 50821 -9
- Partials 2988 2989 +1
... and 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Go's url.Parse rejects unbracketed IPv6 host:port strings, which made kube-apiserver CrashLoop on NodePort OAuth metadata and left HostedControlPlane not Ready. Co-authored-by: Cursor <cursoragent@cursor.com>
53fc430 to
3615091
Compare
|
@andrej1991: 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. |
andrej1991
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@andrej1991: This pull request explicitly references no jira issue. 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. |
andrej1991
changed the title
|
@andrej1991: This pull request references MGMT-24693 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 task to target the "5.0.0" version, but no target version was set. 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. |
2 participants
Go's url.Parse rejects unbracketed IPv6 host:port strings, which made kube-apiserver CrashLoop on NodePort OAuth metadata and left HostedControlPlane not Ready.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests