OCPBUGS-100140: bracket IPv6 in OAuth issuer and callback URLs - #9120
OCPBUGS-100140: bracket IPv6 in OAuth issuer and callback URLs#9120andrej1991 wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughOAuth callback templates, OAuth metadata endpoints, OAuth client redirect URIs, ignition endpoints, and API server URLs now use Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
![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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9120 +/- ##
==========================================
+ Coverage 44.51% 44.66% +0.14%
==========================================
Files 774 775 +1
Lines 96997 97114 +117
==========================================
+ Hits 43179 43373 +194
+ Misses 50830 50744 -86
- Partials 2988 2997 +9
... and 14 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
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. |
csrwng
added
the
approved
|
/cc |
andrej1991
changed the title
openshift-ci-robot
added
the
jira/invalid-bug
|
@andrej1991: This pull request references Jira Issue OCPBUGS-100140, 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@andrej1991: This pull request references Jira Issue OCPBUGS-100140, which is valid. The bug has been moved to the POST state. 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. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/lgtm |
|
Scheduling tests matching the |
|
/retest-required |
Nirshal
left a comment
Nirshal
left a comment
There was a problem hiding this comment.
Missing fix in HCCO OAuth clients
Nice fix! We're aware this wasn't part of the original issue scope, but it looks like ReconcileBrowserClient and ReconcileChallengingClient in control-plane-operator/hostedclusterconfigoperator/controllers/resources/oauth/reconcile.go (lines 25, 32) have the same fmt.Sprintf("https://%s:%d/...", ...) pattern that will produce invalid redirect URIs for IPv6 OAuth hosts. The externalHost value ultimately comes from InfraStatus.OAuthHost via the --oauth-address flag.
Would you mind extending the fix to cover these two call sites as well? It would be great to have all the IPv6 bracketing handled in one PR. Thanks!
| wantErr: true, | ||
| errSubstr: "failed to unmarshal oauth metadata", | ||
| }, | ||
| { |
There was a problem hiding this comment.
nit: the project convention for test names is "When ..., it should ...". For example:
"When OAuth host is IPv6, it should bracket the address""When OAuth host is IPv4, it should leave issuer URLs unbracketed"
| } | ||
| if util.HCPOAuthEnabled(hostedControlPlane) { | ||
| hostedControlPlane.Status.OAuthCallbackURLTemplate = fmt.Sprintf("https://%s:%d/oauth2callback/[identity-provider-name]", infraStatus.OAuthHost, infraStatus.OAuthPort) | ||
| // JoinHostPort brackets IPv6 literals so url.Parse accepts the callback template. |
There was a problem hiding this comment.
nit: JoinHostPort already conveys the intent — the comment could be dropped per project conventions (no comments on self-explanatory code).
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci
Bot
added
area/hypershift-operator
|
@andrej1991: This pull request references Jira Issue OCPBUGS-100140, 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. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: andrej1991, Nirshal The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Line 1118: Add test coverage for both reconciler paths using a raw IPv6
serviceStrategy.NodePort.Address, verifying the generated IgnitionEndpoint is
correctly bracketed by net.JoinHostPort. Cover the hostedcluster_controller.go
site at lines 1118-1118 and reconcile_legacy.go site at lines 799-799; update
tests only, with no direct production-code change required.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cc0c86e7-cfec-4aa8-bdfa-09f6607aa7f4
📒 Files selected for processing (8)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/oauth/reconcile.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/oauth/reconcile_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/internal/platform/agent/agent_test.gohypershift-operator/controllers/hostedcluster/reconcile_legacy.gosupport/globalconfig/infrastructure.gosupport/globalconfig/infrastructure_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/oauth_test.go
| } | ||
| if err == nil && serviceFirstNodePortAvailable(ignitionService) { | ||
| hcluster.Status.IgnitionEndpoint = fmt.Sprintf("%s:%d", serviceStrategy.NodePort.Address, ignitionService.Spec.Ports[0].NodePort) | ||
| hcluster.Status.IgnitionEndpoint = net.JoinHostPort(serviceStrategy.NodePort.Address, strconv.Itoa(int(ignitionService.Spec.Ports[0].NodePort))) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- hostedcluster_controller.go around line 1118 ---'
sed -n '1088,1135p' hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
echo
echo '--- reconcile_legacy.go around line 799 ---'
sed -n '770,825p' hypershift-operator/controllers/hostedcluster/reconcile_legacy.go
echo
echo '--- search tests for IgnitionEndpoint / IPv6 / NodePort Address ---'
rg -n "IgnitionEndpoint|NodePort.Address|JoinHostPort|IPv6|bracket" hypershift-operator/controllers/hostedcluster -g '*_test.go'Repository: openshift/hypershift
Length of output: 10823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "2001:|IPv6|JoinHostPort|IgnitionEndpoint|NodePort.Address" hypershift-operator/controllers/hostedcluster -g '*_test.go' -A 4 -B 4Repository: openshift/hypershift
Length of output: 38286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files hypershift-operator/controllers/hostedcluster/*_test.goRepository: openshift/hypershift
Length of output: 894
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- NodePort-related tests in hostedcluster_controller_test.go ---'
rg -n "NodePort|ignition service|ignition proxy service|IgnitionEndpoint|JoinHostPort|Address" hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go -A 6 -B 6
echo
echo '--- Legacy reconcile test search in hostedcluster controller tests ---'
rg -n "legacy|reconcile_legacy|IgnitionEndpoint" hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go -A 3 -B 3Repository: openshift/hypershift
Length of output: 28937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- exact NodePort.Address usages in tests ---'
rg -n "NodePort\.Address|IgnitionEndpoint.*JoinHostPort|JoinHostPort\(serviceStrategy\.NodePort\.Address|serviceStrategy\.NodePort\.Address" hypershift-operator -g '*_test.go' -A 4 -B 4
echo
echo '--- any reconcile_legacy test file or legacy-path-specific test references ---'
rg -n "reconcile_legacy|legacy path|IgnitionEndpoint" hypershift-operator -g '*_test.go' -A 2 -B 2Repository: openshift/hypershift
Length of output: 205
Add raw IPv6 NodePort coverage for both reconciler paths. The current tests don’t exercise net.JoinHostPort(serviceStrategy.NodePort.Address, ...) with a raw IPv6 address, so an unbracketed endpoint regression would still slip through.
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go#L1118-L1118hypershift-operator/controllers/hostedcluster/reconcile_legacy.go#L799-L799
📍 Affects 2 files
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go#L1118-L1118(this comment)hypershift-operator/controllers/hostedcluster/reconcile_legacy.go#L799-L799
🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go` at
line 1118, Add test coverage for both reconciler paths using a raw IPv6
serviceStrategy.NodePort.Address, verifying the generated IgnitionEndpoint is
correctly bracketed by net.JoinHostPort. Cover the hostedcluster_controller.go
site at lines 1118-1118 and reconcile_legacy.go site at lines 799-799; update
tests only, with no direct production-code change required.
Source: Coding guidelines
Test Resultse2e-aws
e2e-aks
|
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-aks |
|
/test e2e-aks-4-22 |
|
@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. |
|
/verified later |
|
@gamli75: 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. |
7 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