fix(oauth): use kubeAPIServerDNSName for LoginURL when configured - #8854
fix(oauth): use kubeAPIServerDNSName for LoginURL when configured#8854chdeshpa-hue wants to merge 1 commit into
Conversation
When a HostedCluster is configured with spec.kubeAPIServerDNSName and custom named certificates, the OAuth token display page showed the raw internal LB IP in --server= instead of the configured DNS name. adaptOAuthConfig was building LoginURL from hcp.Status.ControlPlaneEndpoint.Host without checking hcp.Spec.KubeAPIServerDNSName. The kubeconfig generation path (adaptCustomAdminKubeconfigSecret) already correctly prefers kubeAPIServerDNSName — OAuth config was never updated when that feature was added. Prefer hcp.Spec.KubeAPIServerDNSName as the LoginURL host when set, wrapped with pki.AddBracketsIfIPv6 to match the customExternalURL pattern exactly. The existing OauthLoginURLOverrideAnnotation (IBMCloud) still takes final precedence. The change is inert for all clusters that do not set kubeAPIServerDNSName. Fixes: https://issues.redhat.com/browse/OCPBUGS-86260 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/needs-area
needs-rebase
|
PR needs rebase. 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. |
📝 WalkthroughWalkthrough
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
|
@chdeshpa-hue: The following tests failed, say
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chdeshpa-hue 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 |
openshift-ci
Bot
added
the
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go (1)
35-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
When ... it should ...for the new subtest names.These case descriptions do not follow the repository’s required unit-test wording. As per coding guidelines,
Always use "When ... it should ..." format for describing test cases when creating unit tests.🤖 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 35 - 52, The new table-driven subtest names in the OAuth config test do not follow the required unit-test wording. Update the `name` values in `config_test.go` to use the repository’s `When ... it should ...` format, keeping the same scenarios in the `Test...`/subtest cases that exercise the login URL selection logic.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.
Inline comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go`:
- Around line 28-56: The LoginURL test table in config_test.go does not cover
the new IPv6 bracketing behavior introduced through pki.AddBracketsIfIPv6, so
add a case in the existing table-driven test that uses an IPv6 host and expects
the bracketed https URL. Update the relevant assertions in the login URL test
helper/setup so the new case exercises the same path as the current
kubeAPIServerDNS and annotation scenarios, ensuring the IPv6 formatting is
verified alongside the existing LoginURL logic.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go`:
- Around line 35-52: The new table-driven subtest names in the OAuth config test
do not follow the required unit-test wording. Update the `name` values in
`config_test.go` to use the repository’s `When ... it should ...` format,
keeping the same scenarios in the `Test...`/subtest cases that exercise the
login URL selection logic.
🪄 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: 3f3d33b2-1d3a-4596-9a3a-5c55ffb8cf4d
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go
| tests := []struct { | ||
| name string | ||
| kubeAPIServerDNS string | ||
| loginURLAnnotation string | ||
| expectedLoginURL string | ||
| }{ | ||
| { | ||
| name: "no kubeAPIServerDNSName — uses controlPlaneEndpoint host", | ||
| kubeAPIServerDNS: "", | ||
| expectedLoginURL: fmt.Sprintf("https://%s:%d", lbHost, lbPort), | ||
| }, | ||
| { | ||
| name: "kubeAPIServerDNSName set — uses DNS name", | ||
| kubeAPIServerDNS: dnsName, | ||
| expectedLoginURL: fmt.Sprintf("https://%s:%d", dnsName, lbPort), | ||
| }, | ||
| { | ||
| name: "kubeAPIServerDNSName set but annotation override present — annotation wins", | ||
| kubeAPIServerDNS: dnsName, | ||
| loginURLAnnotation: "https://iam.custom.ibm.com/login", | ||
| expectedLoginURL: "https://iam.custom.ibm.com/login", | ||
| }, | ||
| { | ||
| name: "only annotation override, no DNS name — annotation used", | ||
| kubeAPIServerDNS: "", | ||
| loginURLAnnotation: "https://iam.custom.ibm.com/login", | ||
| expectedLoginURL: "https://iam.custom.ibm.com/login", | ||
| }, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a case for the new IPv6 bracketing path.
The production change now routes LoginURL through pki.AddBracketsIfIPv6(...), but this table only exercises IPv4, DNS, and annotation inputs. A regression back to an unbracketed IPv6 URL would still pass here. As per coding guidelines, Unit test any code changes and additions.
Also applies to: 71-74, 99-99
🤖 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 28 - 56, The LoginURL test table in config_test.go does not cover
the new IPv6 bracketing behavior introduced through pki.AddBracketsIfIPv6, so
add a case in the existing table-driven test that uses an IPv6 host and expects
the bracketed https URL. Update the relevant assertions in the login URL test
helper/setup so the new case exercises the same path as the current
kubeAPIServerDNS and annotation scenarios, ensuring the IPv6 formatting is
verified alongside the existing LoginURL logic.
Source: Coding guidelines
1 participant
Bug
OCPBUGS-86260: Hosted Clusters show internal API IP instead of DNS name in login token page
Problem
When a HostedCluster is configured with
spec.kubeAPIServerDNSName(e.g.,api.ocp4.example.com),the OAuth server login token page shows the raw Load Balancer IP address in the
--serverparameter instead of the configured DNS name.
Actual:
--server=https://10.71.22.118:6443Expected:
--server=https://api.ocp4.example.com:6443Root Cause
adaptOAuthConfiginv2/oauth/config.goconstructs theLoginURLusingcontrolPlaneEndpoint.Host(the LB IP from status) unconditionally, without checkingif
spec.kubeAPIServerDNSNameis set.The kubeconfig generation path (
v2/kas/kubeconfig.go) already handles this correctlyvia
customExternalURL(hcp.Spec.KubeAPIServerDNSName, ...)but the OAuth config pathwas missed during the v2 refactor.
Fix
Check
hcp.Spec.KubeAPIServerDNSNamebefore constructing LoginURL. Precedence:OauthLoginURLOverrideAnnotation(highest — IBMCloud ROKS)Spec.KubeAPIServerDNSName(when set — this fix)Status.ControlPlaneEndpoint.Host(default)Also adds
pki.AddBracketsIfIPv6wrapping for proper IPv6 address formatting (aligningwith the pattern used elsewhere in the codebase).
Testing
Files Changed
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go— fix LoginURL constructioncontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.go— new unit tests (4 cases)Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests