fix(cpo): use KubeAPIServerDNSName for OAuth LoginURL when set#8433
fix(cpo): use KubeAPIServerDNSName for OAuth LoginURL when set#8433amasolov wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThe pull request changes adaptOAuthConfig so the OAuth login URL's host is derived from the ControlPlaneEndpoint host but can be overridden by HCP.Spec.KubeAPIServerDNSName when provided; the login URL is then built from that host and the endpoint port. MasterURL and MasterPublicURL assignments remain unchanged. A new test file validates LoginURL, MasterURL, and MasterPublicURL across scenarios: default DNS, KubeAPIServerDNSName override, IP-based control plane endpoints, and precedence of a login URL override annotation. 🚥 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)
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amasolov 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 |
|
/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 37.44% 37.47% +0.03%
==========================================
Files 751 751
Lines 91969 91975 +6
==========================================
+ Hits 34435 34468 +33
+ Misses 54894 54864 -30
- Partials 2640 2643 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amasolov
force-pushed
the
fix/oauth-token-display-fqdn
branch
from
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
| loginHost := controlPlaneEndpoint.Host | ||
| if customDNS := cpContext.HCP.Spec.KubeAPIServerDNSName; len(customDNS) > 0 { | ||
| loginHost = customDNS | ||
| } | ||
| cfg.OAuthConfig.LoginURL = fmt.Sprintf("https://%s:%d", loginHost, controlPlaneEndpoint.Port) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the actual file
find . -path "*/control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go" -type fRepository: openshift/hypershift
Length of output: 139
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f "control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go" ]; then
echo "=== File found, checking lines around 76-80 ==="
sed -n '1,100p' "control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go" | cat -n
else
echo "File not found at expected path. Searching..."
find . -name "config.go" -path "*/oauth/*" 2>/dev/null | head -5
fiRepository: openshift/hypershift
Length of output: 5275
🏁 Script executed:
# Also check imports to understand current dependencies
if [ -f "control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go" ]; then
echo "=== Current imports in the file ==="
sed -n '1,30p' "control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go"
fiRepository: openshift/hypershift
Length of output: 1105
🏁 Script executed:
# Verify Go's net.JoinHostPort behavior with IPv6
python3 << 'EOF'
# Test Go's net.JoinHostPort behavior to confirm it handles IPv6 correctly
test_cases = [
("localhost", "6443", "localhost:6443"),
("example.com", "443", "example.com:443"),
("::1", "6443", "[::1]:6443"), # IPv6 loopback
("2001:db8::1", "8443", "[2001:db8::1]:8443"), # IPv6 address
]
print("Expected net.JoinHostPort behavior with IPv6:")
for host, port, expected in test_cases:
# fmt.Sprintf would produce:
sprintf_result = f"https://{host}:{port}"
print(f"\nHost: {host}, Port: {port}")
print(f" fmt.Sprintf('https://%s:%d', ...): {sprintf_result}")
print(f" net.JoinHostPort(...): {expected}")
print(f" URL would be: https://{expected}")
# Check if it's valid
if host.startswith(":") or ":" in host:
if expected.startswith("["):
print(f" ✓ Properly bracketed for IPv6")
else:
print(f" ✗ NOT bracketed - would be malformed")
EOFRepository: openshift/hypershift
Length of output: 842
🏁 Script executed:
# Check if there are other similar patterns in the file or nearby files
echo "=== Checking for similar fmt.Sprintf patterns in the file ==="
rg 'fmt\.Sprintf.*https.*%s.*%d' control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config.go -nRepository: openshift/hypershift
Length of output: 336
🏁 Script executed:
# Check if net, net/url, or strconv are used elsewhere in the codebase for URL building
echo "=== Searching for net.JoinHostPort usage in codebase ==="
rg 'net\.JoinHostPort' --type go | head -10
echo ""
echo "=== Searching for url.URL pattern usage ==="
rg 'url\.URL.*Scheme.*Host' --type go | head -10Repository: openshift/hypershift
Length of output: 2779
🏁 Script executed:
# Check how controlPlaneEndpoint.Port and cpContext.InfraStatus.OAuthPort are defined/typed
rg 'type.*Endpoint|OAuthPort|OAuthHost' --type go -A 2 | head -30Repository: openshift/hypershift
Length of output: 3306
🏁 Script executed:
# Look for any IPv6 tests or IPv6-related issues in the codebase
rg 'IPv6|\[::' --type go | grep -v vendor | head -10Repository: openshift/hypershift
Length of output: 1480
🏁 Script executed:
# Check the type definition of controlPlaneEndpoint to understand Port type
rg 'ControlPlaneEndpoint|Status.*Endpoint' --type go -A 3 | grep -v vendor | head -20Repository: openshift/hypershift
Length of output: 124
🏁 Script executed:
# Verify the fix will work correctly by checking type conversions needed
# Let's also check if there are any tests that might test IPv6 scenarios
find . -path "*/oauth/*test*.go" -o -path "*/test*oauth*.go" 2>/dev/null | grep -v vendor | head -5Repository: openshift/hypershift
Length of output: 488
🏁 Script executed:
# Check the test file for the config
cat -n ./control-plane-operator/controllers/hostedcontrolplane/v2/oauth/config_test.goRepository: openshift/hypershift
Length of output: 4159
🏁 Script executed:
# Also check the hypershift API to understand the type of ControlPlaneEndpoint
find . -path "*/api/hypershift/v1beta1/*" -name "*.go" -type f 2>/dev/null | grep -v vendor | head -10Repository: openshift/hypershift
Length of output: 524
🏁 Script executed:
# Find APIEndpoint type definition in hypershift API
rg 'type APIEndpoint' --type go -A 5 | grep -v vendorRepository: openshift/hypershift
Length of output: 1115
🏁 Script executed:
# Check the actual behavior with url.URL and net.JoinHostPort to ensure the fix is correct
python3 << 'EOF'
import json
# Test the proposed fix logic
test_cases = [
{"host": "localhost", "port": 6443},
{"host": "api.example.com", "port": 443},
{"host": "::1", "port": 6443},
{"host": "2001:db8::1", "port": 8443},
{"host": "[::1]", "port": 6443}, # already bracketed
]
print("Testing proposed fix logic:\n")
for tc in test_cases:
host = tc["host"]
port = tc["port"]
# Current (broken) approach
sprintf_result = f"https://{host}:{port}"
# Proposed fix approach (simulated)
# In Go: net.JoinHostPort(loginHost, strconv.Itoa(int(controlPlaneEndpoint.Port)))
if host.startswith("["):
# Already bracketed
joined = f"{host}:{port}"
elif ":" in host and not host.startswith("http"):
# IPv6 without brackets
joined = f"[{host}]:{port}"
else:
# IPv4 or hostname
joined = f"{host}:{port}"
url_result = f"https://{joined}"
print(f"Host: {host}, Port: {port}")
print(f" Current fmt.Sprintf: {sprintf_result}")
print(f" Proposed fix: {url_result}")
# Validate URL
if host.startswith("[") or (host.startswith("::") or (":" in host and not host.startswith("http"))):
# IPv6
if "[" in url_result:
print(f" ✓ Valid IPv6 URL")
else:
print(f" ✗ INVALID - IPv6 not bracketed")
else:
print(f" ✓ Valid URL")
print()
EOFRepository: openshift/hypershift
Length of output: 778
🏁 Script executed:
# Verify that the Port field is indeed int32 in the APIEndpoint struct
rg 'type APIEndpoint' --type go -A 10 api/hypershift/v1beta1/hosted_controlplane.goRepository: openshift/hypershift
Length of output: 335
Also fix the same IPv6 URL formatting issue on line 71 for MasterURL.
Both line 71 (masterUrl) and line 80 (LoginURL) format URLs using fmt.Sprintf("https://%s:%d", ...), which produces malformed URLs for IPv6 hosts (missing [] brackets). For example, ::1 becomes https://::1:6443 instead of the valid https://[::1]:6443.
Both should use net.JoinHostPort + url.URL to properly handle IPv6 literals:
💡 Proposed fix
import (
"encoding/json"
"fmt"
+ "net"
+ "net/url"
+ "strconv"
"strings"
@@
- masterUrl := fmt.Sprintf("https://%s:%d", cpContext.InfraStatus.OAuthHost, cpContext.InfraStatus.OAuthPort)
+ masterUrl := (&url.URL{
+ Scheme: "https",
+ Host: net.JoinHostPort(cpContext.InfraStatus.OAuthHost, strconv.Itoa(int(cpContext.InfraStatus.OAuthPort))),
+ }).String()
controlPlaneEndpoint := cpContext.HCP.Status.ControlPlaneEndpoint
cfg.OAuthConfig.MasterURL = masterUrl
cfg.OAuthConfig.MasterPublicURL = masterUrl
@@
- cfg.OAuthConfig.LoginURL = fmt.Sprintf("https://%s:%d", loginHost, controlPlaneEndpoint.Port)
+ cfg.OAuthConfig.LoginURL = (&url.URL{
+ Scheme: "https",
+ Host: net.JoinHostPort(loginHost, strconv.Itoa(int(controlPlaneEndpoint.Port))),
+ }).String()🤖 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.go`
around lines 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).
|
@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. |
2 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