CONSOLE-5124: Multi-domain console backend support#16686
Conversation
Add a new AdditionalConsoleBaseAddresses field to the ClusterInfo struct to support hosting the console at multiple URLs on different domains. The console-operator will populate this field in the console ConfigMap when additional componentRoutes are configured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Register --additional-base-addresses CLI flag, parse comma-separated URLs into []*url.URL, and pass them to the Server struct. Add config parsing to read AdditionalConsoleBaseAddresses from the console ConfigMap and set the flag. Add test coverage for config round-trip. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change CSRFVerifier to accept a slice of allowed referer URLs instead of a single URL. The verifier now loops over all configured URLs and accepts the request if any match. This enables CSRF validation when the console is served on multiple domains simultaneously. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add oauth2ConfigForHost() to rewrite the OAuth redirect_uri based on the incoming request's Host header, validated against an allowlist of configured hostnames. This ensures OAuth login/callback redirects return to whichever domain the user accessed. Switch login URLs (LoginURL, LoginSuccessURL, LoginErrorURL) from absolute to relative paths so they resolve correctly on any domain. Wire AllowedRedirectHosts and additional base URLs through the auth options layer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@Leo6Leo: This pull request references CONSOLE-5124 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 story 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. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
WalkthroughAdds multi-domain console support via a new ChangesMulti-domain console support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Leo6Leo 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.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/bridge/main.go (1)
209-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep additional base URL validation aligned with
--base-address.
BaseURLandAdditionalBaseURLsare treated as peers downstream in the CSRF and OAuth setup, but they are validated through different code paths here. Reusing the same URL validator (or a shared helper) would prevent the primary and additional domain inputs from drifting on accepted formats and normalization.🤖 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 `@cmd/bridge/main.go` around lines 209 - 225, Keep the validation for AdditionalBaseURLs aligned with the existing --base-address handling by reusing the same URL validation/normalization logic instead of parsing it separately in main. Update the additional base address loop in cmd/bridge/main.go so it goes through the same helper or validator used for BaseURL, ensuring both paths accept and normalize URLs consistently before appending to additionalBaseURLs.pkg/auth/oauth2/auth_test.go (2)
277-322: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd callback coverage for request-host redirect selection.
The production change updates both login and callback paths, but this test only verifies
LoginFunc. Add a focused callback test so a regression inCallbackFuncusing the default redirect URL during code exchange is caught.🤖 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 `@pkg/auth/oauth2/auth_test.go` around lines 277 - 322, Add a focused test for CallbackFunc request-host redirect selection, since the current coverage only checks LoginFunc. In pkg/auth/oauth2/auth_test.go, extend the existing request-host setup around NewOAuth2Authenticator and mockOIDCProvider to exercise CallbackFunc with a request to console-alt.example.com, and assert it uses the host-derived callback URL instead of the default RedirectURL during code exchange.
171-232: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover configured scheme handling for additional redirect URLs.
These cases only prove host replacement with the primary scheme. Add coverage for a mixed-scheme additional base URL, or assert that such configuration is rejected, so the URL-to-host allowlist contract cannot silently drop scheme information.
🤖 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 `@pkg/auth/oauth2/auth_test.go` around lines 171 - 232, The TestOAuth2ConfigForHost coverage only verifies host replacement against the primary redirect scheme, so the additional redirect URL contract could miss mixed-scheme behavior. Update TestOAuth2ConfigForHost in auth_test.go to either add a case for a configured additional base URL with a different scheme and assert the expected outcome, or assert that such mixed-scheme configuration is rejected by NewOAuth2Authenticator. Use the existing Config, AllowedRedirectHosts, and NewOAuth2Authenticator symbols to keep the test aligned with the redirect URL validation behavior.pkg/auth/csrfverifier/csfr_test.go (1)
65-90: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a non-root base-path boundary case.
The new table only covers
/base paths, so it would not catch/console-evilbeing accepted for a configured/consolebase path. Add a rejected regression case alongside the multi-URL 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 `@pkg/auth/csrfverifier/csfr_test.go` around lines 65 - 90, Add a rejected regression case to TestRefererMultipleURLs that covers a non-root base path boundary, since the current table only exercises "/" paths. Update the CSRFVerifier referer test setup to include a configured URL with a base path like "/console", then assert that a lookalike prefix such as "/console-evil" is rejected while the intended base path remains accepted. Use the existing TestRefererMultipleURLs and CSRFVerifier symbols to place the new case alongside the current multi-URL table.
🤖 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 `@cmd/bridge/config/auth/authoptions.go`:
- Line 281: Remove the dead `oidcClientSecret` assignment in the auth options
setup: it is initialized from `c.ClientSecret` in the var block but then
immediately overwritten before first use, triggering `ineffassign`. Update the
initialization flow in `authoptions.go` so `oidcClientSecret` is assigned only
once and the later redundant reassignment is removed, keeping the value used by
the `oidcClientConfig`/OIDC auth path consistent.
In `@pkg/auth/csrfverifier/csrf.go`:
- Around line 93-103: The Referer path check in csrfverifier’s validation logic
is using a raw prefix match, which can incorrectly accept sibling paths like
/console-evil. Update the path comparison inside the loop in csrf.go to use a
boundary-aware helper for refererURL.Path and u.Path, allowing either an exact
match or a trailing “/” segment match for non-empty paths while preserving the
existing hostname, port, and scheme checks.
In `@pkg/auth/oauth2/auth.go`:
- Around line 69-71: The dynamic OAuth redirect allowlist is currently storing
only hosts, so the code in AllowedRedirectHosts and the redirect selection path
around the OAuth redirect_uri builder can silently reuse the primary RedirectURL
scheme for an additional base URL that was configured with a different scheme.
Update the allowlist representation and the redirect construction logic so the
allowed additional base address preserves scheme plus host (or validate and
reject mixed-scheme additional base URLs before they reach the allowlist), and
ensure the redirect URI generated from RedirectURL uses the scheme associated
with the matched allowed address rather than always inheriting the primary one.
In `@pkg/serverconfig/config.go`:
- Around line 263-264: The addClusterInfo helper currently ignores the error
returned by fs.Set when setting additional-base-addresses, which can hide flag
rename or invalid value failures. Update addClusterInfo to capture and propagate
the fs.Set error, and make SetFlagsFromConfig return that failure instead of
continuing silently. Use the existing addClusterInfo and SetFlagsFromConfig
symbols to route the error back to the caller.
---
Nitpick comments:
In `@cmd/bridge/main.go`:
- Around line 209-225: Keep the validation for AdditionalBaseURLs aligned with
the existing --base-address handling by reusing the same URL
validation/normalization logic instead of parsing it separately in main. Update
the additional base address loop in cmd/bridge/main.go so it goes through the
same helper or validator used for BaseURL, ensuring both paths accept and
normalize URLs consistently before appending to additionalBaseURLs.
In `@pkg/auth/csrfverifier/csfr_test.go`:
- Around line 65-90: Add a rejected regression case to TestRefererMultipleURLs
that covers a non-root base path boundary, since the current table only
exercises "/" paths. Update the CSRFVerifier referer test setup to include a
configured URL with a base path like "/console", then assert that a lookalike
prefix such as "/console-evil" is rejected while the intended base path remains
accepted. Use the existing TestRefererMultipleURLs and CSRFVerifier symbols to
place the new case alongside the current multi-URL table.
In `@pkg/auth/oauth2/auth_test.go`:
- Around line 277-322: Add a focused test for CallbackFunc request-host redirect
selection, since the current coverage only checks LoginFunc. In
pkg/auth/oauth2/auth_test.go, extend the existing request-host setup around
NewOAuth2Authenticator and mockOIDCProvider to exercise CallbackFunc with a
request to console-alt.example.com, and assert it uses the host-derived callback
URL instead of the default RedirectURL during code exchange.
- Around line 171-232: The TestOAuth2ConfigForHost coverage only verifies host
replacement against the primary redirect scheme, so the additional redirect URL
contract could miss mixed-scheme behavior. Update TestOAuth2ConfigForHost in
auth_test.go to either add a case for a configured additional base URL with a
different scheme and assert the expected outcome, or assert that such
mixed-scheme configuration is rejected by NewOAuth2Authenticator. Use the
existing Config, AllowedRedirectHosts, and NewOAuth2Authenticator symbols to
keep the test aligned with the redirect URL validation behavior.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9d966753-aff0-4266-a961-93f7e085cfb4
📒 Files selected for processing (10)
cmd/bridge/config/auth/authoptions.gocmd/bridge/main.gopkg/auth/csrfverifier/csfr_test.gopkg/auth/csrfverifier/csrf.gopkg/auth/oauth2/auth.gopkg/auth/oauth2/auth_test.gopkg/server/server.gopkg/serverconfig/config.gopkg/serverconfig/config_test.gopkg/serverconfig/types.go
Fix inconsistent field alignment in ClusterInfo struct after adding AdditionalConsoleBaseAddresses. Remove pre-existing redundant oidcClientSecret reassignment flagged by CodeRabbit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Leo6Leo: 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. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review again, and resolve the comments that are addressed |
|
✅ Action performedComments resolved. Approval is disabled; enable Full review finished. |
|
Manually verified with multiple domains, things are working as expected. Code review: |
4 participants
Analysis / Root cause:
RFE-3615 / Epic CONSOLE-5121: The OpenShift web console needs to be accessible at multiple URLs on different domains/IngressControllers simultaneously. This enables operators to expose the console on both public and internal networks.
Stories: CONSOLE-5124 (Dynamic auth per hostname), CONSOLE-5125 (Dynamic CSRF).
Solution description:
This PR adds backend support for hosting the console at multiple URLs. The console-operator (separate PR) populates
additionalConsoleBaseAddressesin the console ConfigMap when additionalcomponentRoutesare configured iningress.config.openshift.io/cluster.Commit 1 — Config type: Adds
AdditionalConsoleBaseAddresses []stringto theClusterInfostruct in the server config.Commit 2 — Config wiring & CLI flag: Registers
--additional-base-addressesCLI flag, parses comma-separated URLs, wires through config file parsing, and adds theAdditionalBaseURLsfield to theServerstruct. Includes test coverage for config round-trip.Commit 3 — Multi-URL CSRF (CONSOLE-5125): Changes
CSRFVerifierto accept a slice of allowed referer URLs instead of a single URL. The verifier loops over all configured URLs and accepts the request if any match. Includes 12 new test cases covering primary, secondary, port-based, and rejected scenarios.Commit 4 — Dynamic OAuth redirect (CONSOLE-5124): Adds
oauth2ConfigForHost()which rewrites the OAuthredirect_uribased on the incoming request'sHostheader, validated against an operator-provided allowlist. UpdatesLoginFuncandCallbackFuncto use the dynamic config. Switches login URLs from absolute to relative paths so they resolve correctly on any domain. Includes 8 new test cases.All changes are backward compatible — when no additional base addresses are configured, behavior is identical to before.
Screenshots / screen recording:
Backend-only changes, no UI modifications. The feature was validated end-to-end with a POC on a live AWS cluster including external OIDC (Keycloak) across multiple domains.
Test setup:
No special setup required to run tests. For end-to-end testing, the console-operator must be updated to populate
additionalConsoleBaseAddressesin the console ConfigMap (separate PR).Test cases:
go test ./pkg/serverconfig/...— config parsing round-tripgo test ./pkg/auth/csrfverifier/...— multi-URL CSRF verification (primary, secondary, port-based, rejected)go test ./pkg/auth/oauth2/...— dynamic redirect_uri rewriting, nil allowlist fallback, LoginFunc host selectiongo build ./cmd/bridge/...— clean compilationBrowser conformance:
Backend-only changes; browser testing not applicable for this PR.
Additional info:
This is part of a multi-repo effort:
labelsfield toComponentRouteSpec(PR Enable YAML schema sidebar on create #2845)Summary by CodeRabbit
New Features
additional-base-addressesconfiguration/flag for multi-domain deployments.Bug Fixes
Tests