OCPBUGS-81312: fix(cpo): use KubeAPIServerDNSName for OAuth LoginURL when set - #8433
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughadaptOAuthConfig now computes the OAuth LoginURL host from HCP.Status.ControlPlaneEndpoint.Host and, if set, overrides that host with HCP.Spec.KubeAPIServerDNSName; the LoginURL uses that host with the control plane endpoint port. MasterURL and MasterPublicURL continue to be built from the control plane endpoint host/port. A new table-driven test, TestAdaptOAuthConfig, validates LoginURL, MasterURL, and MasterPublicURL across scenarios including default DNS, KubeAPIServerDNSName override, control plane endpoint IPs, precedence of the OauthLoginURL override annotation, and IPv6 bracketing. 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @amasolov. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
area/control-plane-operator
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8433 +/- ##
==========================================
+ Coverage 41.84% 41.87% +0.03%
==========================================
Files 759 759
Lines 94073 94085 +12
==========================================
+ Hits 39361 39400 +39
+ Misses 51956 51926 -30
- Partials 2756 2759 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
833ab6a to
ea9ccf9
Compare
|
/ok-to-test |
![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)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go (1)
27-66: ⚡ Quick winAdd an IPv6 endpoint scenario to lock in
LoginURLformatting behavior.Please include a case with
cpEndpointHost(orkasDNSName) as IPv6 and expected bracketed URL (e.g.https://[2001:db8::1]:6443) to prevent regressions.🤖 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/oauth/config_test.go` around lines 27 - 66, Add a new test case to the existing table in config_test.go that uses an IPv6 address for cpEndpointHost (or kasDNSName) to ensure LoginURL is formatted with brackets; for example set cpEndpointHost to "2001:db8::1", cpEndpointPort to 6443, and expectedLoginURL to "https://[2001:db8::1]:6443" while keeping expectedMasterURL appropriate (e.g. "https://oauth.example.com:443"); ensure the test case also covers interactions with loginURLOverride and KubeAPIServerDNSName precedence if relevant so the code paths in the functions that produce LoginURL (the logic under test in this file) handle IPv6 bracketed host formatting.
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go`:
- Around line 76-80: The MasterURL and cfg.OAuthConfig.LoginURL are being
formatted with fmt.Sprintf("https://%s:%d", ...) which breaks IPv6 literals;
change both constructions to build the host:port with net.JoinHostPort and
construct the full URL via url.URL (e.g., set Scheme to "https", Host to
net.JoinHostPort(loginHost, strconv.Itoa(controlPlaneEndpoint.Port)) and use
url.String()) so IPv6 addresses are correctly wrapped in brackets; update the
places that set masterUrl and cfg.OAuthConfig.LoginURL accordingly (referencing
the masterUrl assignment and cfg.OAuthConfig.LoginURL).
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go`:
- Around line 27-66: Add a new test case to the existing table in config_test.go
that uses an IPv6 address for cpEndpointHost (or kasDNSName) to ensure LoginURL
is formatted with brackets; for example set cpEndpointHost to "2001:db8::1",
cpEndpointPort to 6443, and expectedLoginURL to "https://[2001:db8::1]:6443"
while keeping expectedMasterURL appropriate (e.g.
"https://oauth.example.com:443"); ensure the test case also covers interactions
with loginURLOverride and KubeAPIServerDNSName precedence if relevant so the
code paths in the functions that produce LoginURL (the logic under test in this
file) handle IPv6 bracketed host formatting.
🪄 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: 5d6ff01a-7e2b-40bf-a6b7-c147857082c5
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go
|
/retitle OCPBUGS-81312: fix(cpo): use KubeAPIServerDNSName for OAuth LoginURL when set |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@amasolov: This pull request references Jira Issue OCPBUGS-81312, 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
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-81312, 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. |
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aks
|
|
Scheduling tests matching the |
|
/retest-required |
|
@vsolanki12 can i get a /lgtm on this one if everything looks good to you? thanks |
|
@amasolov , I can't give lgtm from my end. Also, lgtm label is already attached this PR. It needs approve label and the CI jobs needs to be completed. I can see 3 CI jobs are failing. We have to retest to run these jobs as well. Once we have approval we can move this one for verified label. But I would like to know if you have tested the fix or not? |
When KubeAPIServerDNSName is configured on a HostedControlPlane, the OAuth token display page shows the ControlPlaneEndpoint host (typically a LoadBalancer IP) in the oc login command instead of the custom FQDN. The LoginURL in the OAuth server config determines what users see as the --server value on the token display page. Previously it always used ControlPlaneEndpoint.Host, which does not reflect a custom DNS name set via spec.kubeAPIServerDNSName. This change makes the OAuth config prefer KubeAPIServerDNSName for the LoginURL when set, while preserving the existing IBMCloud login URL override annotation as the highest-priority override. Additionally, URL construction now uses net.JoinHostPort with url.URL instead of fmt.Sprintf to correctly handle IPv6 literal addresses (wrapping them in brackets as required by RFC 3986). Signed-off-by: Alexey Masolov <amasolov@redhat.com> Assisted-by: Claude (via Cursor) Co-authored-by: Cursor <cursoragent@cursor.com>
4a0d650 to
7404745
Compare
|
/ok-to-test |
|
Thanks @vsolanki12 . This was tested on a real cluster. Test environment:
Result (from the OAuth configmap generated by the CPO): loginURL correctly uses the custom DNS name instead of the ControlPlaneEndpoint IP. Without this fix, it would have been https://10.0.101.177:30688. masterURL and masterPublicURL correctly continue to use the OAuthHost (infrastructure endpoint), which is expected. I've also rebased on current main. @clebs could I get a /lgtm again? The force-push removed the previous label. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amasolov, muraee 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 |
openshift-ci
Bot
added
the
approved
|
@sdminonne: Only users can be targets for the 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. |
|
/verified later @amasolov |
openshift-ci-robot
added
verified-later
verified
|
@sdminonne: This PR has been marked to be verified later by 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
I have all the evidence I need. The failure is clear:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe sole test failure ( Root CauseAWS EC2 API Rate Limiting (Infrastructure / Flake) The
This failure is completely unrelated to the PR's changes. PR #8433 modifies the control-plane-operator to use The e2e test suite ran 616 tests total: 585 passed, 30 were skipped (platform-specific tests for Azure, KubeVirt, OpenStack, etc.), and only this single infrastructure-level failure occurred. No tests related to OAuth, authentication, or the control-plane-operator failed. Recommendations
Evidence
|
|
/ok-to-test |
|
@amasolov: 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. |
|
@amasolov: Jira Issue OCPBUGS-81312: All pull requests linked via external trackers have merged: This pull request has the 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. |
|
Fix included in release 5.0.0-0.nightly-2026-06-26-152608 |
9 participants
Summary
spec.kubeAPIServerDNSNameis set, the OAuth token display page now shows the custom FQDN inoc login --server=instead of the LoadBalancer IP addressadaptOAuthConfigcovering all LoginURL derivation pathsFixes: https://issues.redhat.com/browse/OCPBUGS-81312
Details
The OAuth server config's
LoginURLdetermines the--server=value shown on the token display page (the "Copy login command" page in the console). Previously,LoginURLalways usedControlPlaneEndpoint.Host, which is derived from the infrastructure LoadBalancer status and resolves to an IP when the LB ingress only has an IP address (common with MetalLB and on-premises environments).When a user configures
spec.kubeAPIServerDNSNameto set a custom FQDN for accessing the API server, this FQDN was correctly used for the custom kubeconfig but was not reflected in the OAuth token display page. Users would seeoc login --server=https://10.x.x.x:6443instead ofoc login --server=https://api.custom.example.com:6443.This change makes
adaptOAuthConfigpreferKubeAPIServerDNSNamefor theLoginURLhost when set, while preserving the existing IBMCloud login URL override annotation (oauth.hypershift.openshift.io/login-url-override) as the highest-priority override.Test plan
go test ./control-plane-operator/controllers/hostedcontrolplane/v2/oauth/... -v)KubeAPIServerDNSNameset that the token display page shows the FQDNKubeAPIServerDNSNameis not setMade with Cursor
Summary by CodeRabbit
New Features
Tests