CONSOLE-5122: Multi-domain console route, ConfigMap, and OAuth support#1184
CONSOLE-5122: Multi-domain console route, ConfigMap, and OAuth support#1184Leo6Leo wants to merge 5 commits into
Conversation
Add functions to discover additional componentRoutes from the Ingress config targeting the openshift-console namespace, and create Route objects for each. Routes are labeled with console.openshift.io/ additional-route for identification and future garbage collection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add AdditionalConsoleBaseAddresses field to ClusterInfo and wire it through the config builder and DefaultConfigMap. The console backend reads this field to enable multi-URL CSRF verification and dynamic OAuth redirect_uri selection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add SetRedirectURIs to register multiple redirect URIs on the OAuthClient, one per console domain. Update RegisterConsoleToOAuthClient to accept additional hosts via variadic parameter. Existing SetRedirectURI delegates to the new function for backward compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…roller Discover additional componentRoutes from the Ingress config in the main sync loop. Create Route objects for each, pass hostnames to SyncConfigMap for additionalConsoleBaseAddresses, and pass to the OAuth controller for redirect URI registration. Route creation errors set degraded conditions but do not block the primary console. 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-5122 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. |
|
[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 |
WalkthroughAdds support for deriving additional console route hostnames from ingress componentRoutes and propagating them into route creation, console config generation, and OAuth client redirect URIs. ChangesAdditional host propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@Leo6Leo: The following test 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. |
![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)
pkg/console/subresource/oauthclient/oauthclient_test.go (1)
170-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider merging into one table-driven test.
TestRegisterConsoleToOAuthClientWithAdditionalHostsandTestRegisterConsoleToOAuthClientNoAdditionalHostsexercise the same function and could be collapsed into a single table-driven test with ahosts/wantURIscase table, consistent withTestSetRedirectURIsabove.As per coding guidelines, "Most unit tests should use the table-driven test pattern... for scenarios with multiple cases."
🤖 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/console/subresource/oauthclient/oauthclient_test.go` around lines 170 - 197, Merge the two RegisterConsoleToOAuthClient tests into a single table-driven test using cases for presence/absence of additional hosts, matching the style of TestSetRedirectURIs. Reuse the RegisterConsoleToOAuthClient helper in each case, vary the hosts input and expected RedirectURIs, and keep the existing secret assertion within the same case loop.Source: Coding guidelines
pkg/console/subresource/route/route.go (1)
301-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing Godoc on new exported functions.
GetAdditionalComponentRouteSpecsandGetAdditionalRouteHostnamesare new exported functions without doc comments explaining their behavior (filtering rules, ordering, dedup behavior).As per coding guidelines, "Document exported Go functions with a Godoc comment that explains what the function does."
📝 Suggested doc comments
+// GetAdditionalComponentRouteSpecs returns the console-namespaced componentRoutes +// from the ingress config, excluding routes already managed by the operator +// (console, downloads, and their custom variants). func GetAdditionalComponentRouteSpecs(ingressConfig *configv1.Ingress) []configv1.ComponentRouteSpec {+// GetAdditionalRouteHostnames returns the hostnames configured for the +// additional (non-managed) console componentRoutes. func GetAdditionalRouteHostnames(ingressConfig *configv1.Ingress) []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 `@pkg/console/subresource/route/route.go` around lines 301 - 330, Add Godoc comments for the exported functions GetAdditionalComponentRouteSpecs and GetAdditionalRouteHostnames. Describe that GetAdditionalComponentRouteSpecs returns only component routes in the OpenShift console namespace that are not in knownConsoleRouteNames, and that GetAdditionalRouteHostnames derives hostnames from those filtered specs in the same order. Keep the comments concise and place them immediately above each function so they satisfy Go export documentation rules.Source: Coding guidelines
pkg/console/subresource/oauthclient/oauthclient.go (1)
96-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd Godoc for new exported
SetRedirectURIs.As per coding guidelines, "Document exported Go functions with a Godoc comment that explains what the function does."
📝 Suggested doc comment
+// SetRedirectURIs rebuilds the OAuthClient's RedirectURIs from the given +// hosts, generating an "/auth/callback" HTTPS URI for each host. func SetRedirectURIs(client *oauthv1.OAuthClient, hosts []string) *oauthv1.OAuthClient {🤖 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/console/subresource/oauthclient/oauthclient.go` around lines 96 - 102, Add a Godoc comment for the exported SetRedirectURIs function that clearly states what it does: it sets the OAuthClient redirect URIs from the provided hosts by converting each host into an HTTPS auth callback URL. Place the comment immediately above SetRedirectURIs and ensure it begins with the function name so it satisfies Go export documentation guidelines.Source: Coding guidelines
pkg/console/subresource/configmap/configmap_test.go (1)
1290-1309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test case exercises populated
additionalHosts.All table cases pass
nilfor the newadditionalHostsparam, so this test never verifies thatadditionalConsoleBaseAddressesis correctly emitted in the generatedconsole-config.yaml. Consider adding a case with a non-empty slice to cover the new multi-domain code path end-to-end.As per path instructions, "Cover both success and failure paths in unit tests, including edge cases such as empty inputs, boundary values, missing fields, duplicates, and large inputs."
🤖 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/console/subresource/configmap/configmap_test.go` around lines 1290 - 1309, The DefaultConfigMap test coverage is missing a case where additionalHosts is populated, so the new multi-domain path is never exercised. Update the configmap_test.go table-driven tests around DefaultConfigMap to add at least one case with a non-empty additionalHosts slice and assert that additionalConsoleBaseAddresses is rendered in the generated console-config.yaml, using the DefaultConfigMap symbol and the existing test case structure to locate the change.Source: Path instructions
🤖 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 `@pkg/console/operator/sync_v400.go`:
- Around line 145-151: The AdditionalRouteSync status is only reported when
there are route sync errors, so the condition is never cleared after recovery.
Update the AdditionalRouteSync block in sync_v400.go to call
statusHandler.AddConditions with status.HandleProgressingOrDegraded
unconditionally, matching CustomLogoSync and the other sync handlers, and pass a
nil error when routeSyncErrors is empty so any previously set
degraded/progressing state is reset.
- Around line 137-138: Update the route apply path so it threads the caller’s
ctx and a recorder into routesub.ApplyRoute instead of relying on route.go’s
context.TODO() behavior. Adjust ApplyRoute in the route subresource helper to
accept ctx and recorder, then use that ctx for GET/Create/Update and emit apply
events consistently with the other resource helpers. Update the sync_v400 call
site in operator sync logic to pass the existing context and recorder through
this route application flow.
- Around line 136-144: The additional host list is being built only after
ApplyRoute succeeds, which can make additionalHosts diverge from the desired
additional route set when a route sync fails. Update the sync loop in
sync_v400.go so additionalHosts is appended from each additionalSpecs item
unconditionally, while keeping ApplyRoute errors recorded separately in
routeSyncErrors and logged through routesub.ApplyRoute handling.
In `@pkg/console/subresource/route/route.go`:
- Around line 334-343: AdditionalRoute currently copies only the route name,
host, and label, so it drops the configured serving cert from
ComponentRouteSpec. Update AdditionalRoute in route.go to propagate
spec.ServingCertKeyPairSecret onto the returned Route so extra console hostnames
can use the admin-provided TLS cert, while keeping the existing name/host/label
behavior intact.
---
Nitpick comments:
In `@pkg/console/subresource/configmap/configmap_test.go`:
- Around line 1290-1309: The DefaultConfigMap test coverage is missing a case
where additionalHosts is populated, so the new multi-domain path is never
exercised. Update the configmap_test.go table-driven tests around
DefaultConfigMap to add at least one case with a non-empty additionalHosts slice
and assert that additionalConsoleBaseAddresses is rendered in the generated
console-config.yaml, using the DefaultConfigMap symbol and the existing test
case structure to locate the change.
In `@pkg/console/subresource/oauthclient/oauthclient_test.go`:
- Around line 170-197: Merge the two RegisterConsoleToOAuthClient tests into a
single table-driven test using cases for presence/absence of additional hosts,
matching the style of TestSetRedirectURIs. Reuse the
RegisterConsoleToOAuthClient helper in each case, vary the hosts input and
expected RedirectURIs, and keep the existing secret assertion within the same
case loop.
In `@pkg/console/subresource/oauthclient/oauthclient.go`:
- Around line 96-102: Add a Godoc comment for the exported SetRedirectURIs
function that clearly states what it does: it sets the OAuthClient redirect URIs
from the provided hosts by converting each host into an HTTPS auth callback URL.
Place the comment immediately above SetRedirectURIs and ensure it begins with
the function name so it satisfies Go export documentation guidelines.
In `@pkg/console/subresource/route/route.go`:
- Around line 301-330: Add Godoc comments for the exported functions
GetAdditionalComponentRouteSpecs and GetAdditionalRouteHostnames. Describe that
GetAdditionalComponentRouteSpecs returns only component routes in the OpenShift
console namespace that are not in knownConsoleRouteNames, and that
GetAdditionalRouteHostnames derives hostnames from those filtered specs in the
same order. Keep the comments concise and place them immediately above each
function so they satisfy Go export documentation rules.
🪄 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: 9c06bd3f-b243-4a84-90d9-02a5400ff8df
📒 Files selected for processing (11)
pkg/console/controllers/oauthclients/oauthclients.gopkg/console/operator/sync_v400.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/consoleserver/types.gopkg/console/subresource/oauthclient/oauthclient.gopkg/console/subresource/oauthclient/oauthclient_test.gopkg/console/subresource/route/route.gopkg/console/subresource/route/route_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/console(manual) → reviewed against open PR#16686CONSOLE-5124/config-plumbinginstead of the default branch
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Follow Go coding standards and patterns documented in CONVENTIONS.md
Organize imports according to conventions documented in CONVENTIONS.md
Usegofmtto format Go code with standard formatting
Rungo vetchecks on all Go packagesFollow Go coding standards and patterns as documented in CONVENTIONS.md, including proper import organization
Organize Go code following the repository structure: main entry point in
cmd/console/main.go, API constants inpkg/api/, operator command setup inpkg/cmd/operator/, and version command inpkg/cmd/version/
**/*.go: Usegofmtfor formatting Go code
Follow standard Go naming conventions
Group imports in order: standard lib, 3rd party, kube/openshift, internal (marked with comments)
Use meaningful error messages with context in Go code
Set status conditions usingstatus.Handle*functions with type prefixes (*Degraded, *Progressing, *Available, *Upgradeable)
Use typed errors and wrap errors to preserve stack contextFlag MD5, SHA1, DES, RC4, 3DES, Blowfish, and ECB mode cryptographic usage. Also flag custom crypto implementations and non-constant-time comparison of secrets or tokens.
**/*.go: Do not use deprecated Go APIs such asioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll, ornet.DialinDialcallbacks; useos.ReadFile,os.WriteFile,io.ReadAll, andDialContextinstead.
When returning errors in Go, wrap them with%wand include meaningful context instead of returning the raw error or using%v.
Use specific error checks such asapierrors.IsNotFound(err)instead of matching error strings withstrings.Contains(err.Error(), ...).
Propagate the caller’scontext.Contextthrough operations and avoid replacing it withcontext.Background()inside request/controller code.
Usedeferto release acquired resources so cleanup happens on all return paths.
Avoid god functions: keep Go functions to roughly under 100 lines and split code with too many responsibilities into smaller...
Files:
pkg/console/controllers/oauthclients/oauthclients.gopkg/console/subresource/consoleserver/types.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/oauthclient/oauthclient.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/oauthclient/oauthclient_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/operator/sync_v400.gopkg/console/subresource/route/route.go
⚙️ CodeRabbit configuration file
**/*.go: Review Go code following OpenShift operator patterns.
See CONVENTIONS.md for coding standards and patterns.Refer to the following skills based on CODE PATTERNS, not just file paths:
Refer to /controller-review when code contains:
- Controller struct types (e.g.,
type *Controller struct)func New*Controller(factory functionsfactory.New().WithFilteredEventsInformers(pattern.ToController(method callsSync(ctx context.Context, controllerContext factory.SyncContext)methodsoperatorConfig.Spec.ManagementStatechecksstatus.NewStatusHandlerorstatus.Handle*functionsRefer to /sync-handler-review when code contains:
- Main operator sync functions (e.g.,
sync_v400.gocontent)- Sequential resource syncing with early returns
- Incremental reconciliation loops
- Multiple
resourceapply.Apply*()calls in sequence- Dependency ordering of ConfigMaps → Secrets → Service Accounts → RBAC → Services → Deployments → Routes
- Feature gate conditional logic
Refer to /go-quality-review for all Go code to check:
- Deprecated imports:
ioutil.ReadFile,ioutil.WriteFile,ioutil.ReadAll- Deprecated patterns:
DialwithoutDialContext- Error handling: missing
%win fmt.Errorf- Code smells: deep nesting (4+ levels), functions >100 lines
- Magic values: unexplained numbers/strings
- Context propagation:
context.Background()instead of passed ctx- Missing godoc on exported functions
Files:
pkg/console/controllers/oauthclients/oauthclients.gopkg/console/subresource/consoleserver/types.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/oauthclient/oauthclient.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/oauthclient/oauthclient_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/operator/sync_v400.gopkg/console/subresource/route/route.go
{pkg,cmd}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use gofmt for code formatting on pkg and cmd directories
{pkg,cmd}/**/*.go: Format code usinggofmt -w ./pkg ./cmd
Rungo vetchecks on all Go packages in ./pkg and ./cmd
Files:
pkg/console/controllers/oauthclients/oauthclients.gopkg/console/subresource/consoleserver/types.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/oauthclient/oauthclient.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/oauthclient/oauthclient_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/operator/sync_v400.gopkg/console/subresource/route/route.go
pkg/console/controllers/**/*.go
📄 CodeRabbit inference engine (ARCHITECTURE.md)
Place all controller implementations in
pkg/console/controllers/subdirectory, with each controller in its own package (e.g.,clidownloads/,oauthclients/,route/,service/)
Files:
pkg/console/controllers/oauthclients/oauthclients.go
**
⚙️ CodeRabbit configuration file
**: # OpenShift Console Operator - AI Context HubThis file serves as the central AI documentation hub for the OpenShift Console Operator project. AI assistants (Claude, Cursor, Copilot, CodeRabbit, etc.) use this and the linked documents to understand project context.
Go Version and Dependencies
Go Version and Dependencies
- Go version: 1.24.0 (toolchain: go1.24.4)
- Dependency management: Uses
go.modwith vendoring- Build flags: Use
GOFLAGS="-mod=vendor"for builds and tests to ensure vendored dependencies are used- Key dependencies: openshift/api, openshift/library-go, k8s.io client libraries
- Go version: 1.24.0 (toolchain: go1.24.4)
- Dependency management: Uses
go.modwith vendoring- Build flags: Use
GOFLAGS="-mod=vendor"for builds and tests to ensure vendored dependencies are used- Key dependencies: openshift/api, openshift/library-go, k8s.io client libraries
Quick Reference
This Repository
Document Purpose ARCHITECTURE.md System architecture, components, repository structure CONVENTIONS.md Go coding standards, patterns, import organization TESTING.md Testing patterns, commands, debugging README.md Project README with setup instructions Console Repository (openshift/console)
For frontend-related guidelines, see the openshift/console repository:
Document Purpose STYLEGUIDE.md Frontend code style guidelines INTERNATIONALIZATION.md i18n patterns and translation guidelines CONTRIBUTING.md Contribution guidelines for the console project Project Summary
The **console-operator...
Files:
pkg/console/controllers/oauthclients/oauthclients.gopkg/console/subresource/consoleserver/types.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/oauthclient/oauthclient.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/oauthclient/oauthclient_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/operator/sync_v400.gopkg/console/subresource/route/route.go
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}
⚙️ CodeRabbit configuration file
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}: Injection prevention (prodsec-skills):
- SQL: parameterized queries only; no string concatenation
- Command: no shell=True, os.system, or backtick exec with user input
- LDAP/XPath: escape special characters in filters
- Path traversal: canonicalize paths, reject ../
- Deserialization: no pickle/yaml.load()/eval on untrusted data
- Prototype pollution: no recursive merge of untrusted objects
- Validate at trust boundaries with allow-lists, not deny-lists
- Normalize Unicode and anchor regexes (^$); watch for ReDoS
Files:
pkg/console/controllers/oauthclients/oauthclients.gopkg/console/subresource/consoleserver/types.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/oauthclient/oauthclient.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/oauthclient/oauthclient_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/operator/sync_v400.gopkg/console/subresource/route/route.go
pkg/console/subresource/**/*.go
📄 CodeRabbit inference engine (ARCHITECTURE.md)
Use
pkg/console/subresource/packages for resource builders, with separate packages for each resource type (authentication, configmap, deployment, oauthclient, route, secret, etc.)
Files:
pkg/console/subresource/consoleserver/types.gopkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/oauthclient/oauthclient.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/configmap/configmap.gopkg/console/subresource/oauthclient/oauthclient_test.gopkg/console/subresource/consoleserver/config_builder.gopkg/console/subresource/route/route.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow testing patterns and commands documented in TESTING.md
Follow testing patterns and commands as documented in TESTING.md, including running unit tests with 'make test-unit' and checks with 'make check'
**/*_test.go: Use table-driven tests for comprehensive coverage
Usehttptestfor HTTP handler testing in Go
Include proper cleanup functions in tests
Test both success and failure pathsIn Go tests, do not ignore returned errors; check
errand fail the test witht.Fatalfort.Errorfas appropriate.
Files:
pkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/oauthclient/oauthclient_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Review test code for quality and patterns.Refer to /unit-test-review when test is in pkg//*_test.go:**
- Table-driven test structure with test cases
- Use of
go-test/deepfor struct comparisons- Test naming conventions (TestFunctionName)
- Error handling with
wantErrpattern- Edge case coverage (nil, empty, boundary values)
- Proper assertions with helpful error messages
- Test isolation (no shared mutable state)
Refer to /e2e-test-review when test contains:
framework.MustNewClientset(t, nil)or similar e2e framework usagewait.Pollorwait.PollImmediatepatternsretry.RetryOnConflictfor updates- Cleanup via
deferfunctions- Console/operator CR manipulations
- Test assertions on cluster state
Suggest to use /e2e-test-review when:
- PR adds new feature requiring e2e coverage
- Test file is empty or skeleton
- Comments indicate "TODO: add test"
Review for common issues:
- Missing cleanup (defer statements)
- Using
time.Sleepinstead ofwait.Poll- Missing context timeouts
- Vague error messages in assertions
- Tests without table-driven structure when testing multiple cases
- Ignoring errors with
_- Tests without assertions
Files:
pkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/oauthclient/oauthclient_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (.claude/skills/unit-test-review.md)
pkg/**/*_test.go: Most unit tests should use the table-driven test pattern, including atests := []struct{...}table andt.Run(tt.name, ...)subtests for scenarios with multiple cases.
Test function names and subtest case names should be descriptive of the behavior or scenario being tested (for example,TestGetNodeComputeEnvironmentsor"Custom hostname and TLS secret set").
Usegithub.com/go-test/deep(deep.Equal) for struct comparisons instead of==or manual field-by-field checks.
Cover both success and failure paths in unit tests, including edge cases such as empty inputs, boundary values, missing fields, duplicates, and large inputs.
Structure tests using Arrange-Act-Assert so setup, execution, and verification are clearly separated.
When testing error-returning functions, assert error presence correctly and, when relevant, validate the error message substring instead of ignoring the error or discarding it with_.
Prefer dependency injection via interfaces for testability, and keep tests isolated so they do not depend on execution order or shared mutable state.
Extract repeated setup into helper functions when common test fixtures are reused across multiple tests.
Write specific, informative assertions that explain what failed instead of vague or silent failures.
Inline simple test data, but move complex fixtures to helper functions ortestdata/files.
Avoid tests that rely on execution order, share global mutable state, use hardcoded sleeps, omit assertions, or verify implementation details instead of behavior.
Files:
pkg/console/subresource/configmap/configmap_test.gopkg/console/subresource/route/route_test.gopkg/console/subresource/configmap/tech_preview_test.gopkg/console/subresource/oauthclient/oauthclient_test.go
**/*sync*.go
📄 CodeRabbit inference engine (CONVENTIONS.md)
Implement sync loops (
sync_v400) incrementally: start from zero, create/update missing requirements, and return to continue on next loop
Files:
pkg/console/operator/sync_v400.go
**/operator/**/*.go
📄 CodeRabbit inference engine (Custom checks)
When deployment manifests, operator code, or controllers are added/modified, ensure they do not introduce scheduling constraints assuming standard HA topology. Check ControlPlaneTopology for SingleReplica/DualReplica/HighlyAvailableArbiter/External modes before applying constraints. Use required anti-affinity with maxUnavailable >= 1 (not maxUnavailable: 0). Cap replica counts to schedulable nodes. Exclude arbiter nodes on TNA. Avoid master nodeSelectors on HyperShift. Use library-go DeploymentController hooks (WithTopologyAwareReplicasHook, WithTopologyAwareSchedulingHook, WithControlPlaneNodeSelectorHook).
Files:
pkg/console/operator/sync_v400.go
**/sync_v400.go
📄 CodeRabbit inference engine (.claude/skills/sync-handler-review.md)
Incremental sync pattern: each sync loop should stop on the first error and resume from the next step on the next reconciliation instead of collecting and joining all errors.
Files:
pkg/console/operator/sync_v400.go
🔀 Multi-repo context openshift/console
Linked repositories findings
openshift/console (refs/pull/16686/head)
pkg/serverconfig/types.go:73-77definesClusterInfo.AdditionalConsoleBaseAddresses, matching the operator’s newadditionalConsoleBaseAddressesConfigMap field.pkg/serverconfig/config.go:263-264maps that field to the CLI flagadditional-base-addresses, so the backend will actually consume the new values.pkg/auth/oauth2/auth_test.go:210-220, 319-320covers alternate console hostnames and ports in redirect handling, which lines up with the operator’s new multi-host OAuth client registration.
No additional mismatches were visible in this repo from the inspected ref. [::openshift/console::]
🔇 Additional comments (10)
pkg/console/subresource/route/route_test.go (1)
164-283: LGTM!pkg/console/subresource/configmap/configmap.go (1)
52-52: LGTM!Also applies to: 70-70, 111-111
pkg/console/controllers/oauthclients/oauthclients.go (1)
171-172: LGTM!Also applies to: 211-211, 230-230
pkg/console/subresource/oauthclient/oauthclient.go (1)
58-63: LGTM!pkg/console/subresource/oauthclient/oauthclient_test.go (1)
136-168: LGTM!pkg/console/operator/sync_v400.go (1)
152-164: LGTM!Also applies to: 375-375, 450-450
pkg/console/subresource/configmap/tech_preview_test.go (1)
84-101: LGTM!pkg/console/subresource/consoleserver/config_builder.go (2)
86-86: LGTM!Also applies to: 375-379
315-319: 🎯 Functional CorrectnessNo issue:
AdditionalHosts(additionalHosts)is already applied in both config builders. The default and user-defined console-config paths both pass the additional hosts through beforeConfigYAML().> Likely an incorrect or invalid review comment.pkg/console/subresource/consoleserver/types.go (1)
68-79: LGTM!
| for _, spec := range additionalSpecs { | ||
| requiredRoute := routesub.AdditionalRoute(spec) | ||
| if _, _, err := routesub.ApplyRoute(co.routeClient, requiredRoute); err != nil { | ||
| klog.Errorf("failed to sync additional route %s: %v", spec.Name, err) | ||
| routeSyncErrors = append(routeSyncErrors, fmt.Sprintf("%s: %v", spec.Name, err)) | ||
| continue | ||
| } | ||
| additionalHosts = append(additionalHosts, string(spec.Hostname)) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
fd -a 'sync_v400.go|oauthclients.go|routes*.go|configmap*.go' pkg
echo "== search for additional route hostnames and configmap hosts =="
rg -n "GetAdditionalRouteHostnames|additionalConsoleBaseAddresses|additionalHosts|ApplyRoute|AdditionalRoute" pkg/console -S
echo "== inspect target files around matches =="
for f in \
pkg/console/operator/sync_v400.go \
pkg/console/controllers/oauthclients/oauthclients.go \
pkg/console/subresource/route/*.go \
pkg/console/subresource/configmap/*.go
do
if [ -f "$f" ]; then
echo "\n### $f"
sed -n '1,240p' "$f"
fi
doneRepository: openshift/console-operator
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files | rg '^(pkg/console/operator/sync_v400\.go|pkg/console/controllers/oauthclients/oauthclients\.go|pkg/console/subresource/route/.*\.go|pkg/console/subresource/configmap/.*\.go)$'
echo "== search =="
rg -n "GetAdditionalRouteHostnames|additionalConsoleBaseAddresses|additionalHosts|ApplyRoute|AdditionalRoute" pkg/console -SRepository: openshift/console-operator
Length of output: 4492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' pkg/console/controllers/oauthclients/oauthclients.go
printf '\n---\n'
sed -n '1,260p' pkg/console/subresource/route/route.go
printf '\n---\n'
sed -n '1,240p' pkg/console/operator/sync_v400.goRepository: openshift/console-operator
Length of output: 28318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files pkg/console/operator/sync_v400.go pkg/console/controllers/oauthclients/oauthclients.go pkg/console/subresource/route pkg/console/subresource/configmap
echo "== symbols =="
rg -n "GetAdditionalRouteHostnames|additionalConsoleBaseAddresses|additionalHosts|ApplyRoute|AdditionalRoute" pkg/console -S
echo "== oauthclients.go =="
sed -n '1,260p' pkg/console/controllers/oauthclients/oauthclients.go
echo "== sync_v400.go slice =="
sed -n '100,190p' pkg/console/operator/sync_v400.go
echo "== route subresource files =="
for f in $(git ls-files pkg/console/subresource/route/*.go); do
echo "--- $f"
sed -n '1,240p' "$f"
done
echo "== configmap subresource files =="
for f in $(git ls-files pkg/console/subresource/configmap/*.go); do
echo "--- $f"
sed -n '1,240p' "$f"
doneRepository: openshift/console-operator
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate GetAdditionalRouteHostnames =="
rg -n "func GetAdditionalRouteHostnames|GetAdditionalRouteHostnames\(" pkg -S
echo "== inspect function and nearby route host handling =="
files=$(rg -l "GetAdditionalRouteHostnames|additional-base-addresses|AdditionalConsoleBaseAddresses|AdditionalBaseAddresses" pkg -S | head -n 20)
for f in $files; do
echo "--- $f"
sed -n '1,260p' "$f"
doneRepository: openshift/console
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== route.go around GetAdditionalRouteHostnames and AdditionalRoute =="
sed -n '300,380p' pkg/console/subresource/route/route.go
echo
echo "== route_test.go around GetAdditionalRouteHostnames =="
sed -n '230,290p' pkg/console/subresource/route/route_test.go
echo
echo "== configmap consumer around AdditionalHosts =="
sed -n '40,130p' pkg/console/subresource/configmap/configmap.goRepository: openshift/console-operator
Length of output: 7646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,380p' pkg/console/subresource/route/route.go
printf '\n---\n'
sed -n '230,290p' pkg/console/subresource/route/route_test.go
printf '\n---\n'
sed -n '40,130p' pkg/console/subresource/configmap/configmap.goRepository: openshift/console-operator
Length of output: 7481
Keep additionalHosts aligned with the desired additional routes pkg/console/operator/sync_v400.go:136-144
GetAdditionalRouteHostnames() only filters out the built-in console/downloads route names; it still returns every additional hostname from ingress. Because this loop drops hosts whose ApplyRoute fails, additionalConsoleBaseAddresses can diverge from the OAuth redirect set on a transient route error. Build additionalHosts from additionalSpecs unconditionally and keep the route sync errors separate.
🤖 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/console/operator/sync_v400.go` around lines 136 - 144, The additional
host list is being built only after ApplyRoute succeeds, which can make
additionalHosts diverge from the desired additional route set when a route sync
fails. Update the sync loop in sync_v400.go so additionalHosts is appended from
each additionalSpecs item unconditionally, while keeping ApplyRoute errors
recorded separately in routeSyncErrors and logged through routesub.ApplyRoute
handling.
| requiredRoute := routesub.AdditionalRoute(spec) | ||
| if _, _, err := routesub.ApplyRoute(co.routeClient, requiredRoute); err != nil { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate ApplyRoute definition and signature
ast-grep run --pattern 'func ApplyRoute($$$) $$$' --lang go pkg/console/subresource/route
rg -nP '\bfunc ApplyRoute\b' -A 15 pkg/console/subresource/routeRepository: openshift/console-operator
Length of output: 3827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- apply helper ---'
sed -n '1,260p' pkg/console/subresource/route/route.go
echo '--- sync call site ---'
sed -n '110,190p' pkg/console/operator/sync_v400.go
echo '--- any recorder usage with ApplyRoute ---'
rg -n 'ApplyRoute|Recorder|record' pkg/console -g'*.go'Repository: openshift/console-operator
Length of output: 23240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- apply helper ---'
sed -n '1,260p' pkg/console/subresource/route/route.go
echo '--- sync call site ---'
sed -n '110,190p' pkg/console/operator/sync_v400.go
echo '--- any recorder usage with ApplyRoute or route apply events ---'
rg -n 'ApplyRoute|Eventf|Recorder\(' pkg/console -g'*.go'Repository: openshift/console-operator
Length of output: 15090
Thread ctx and the recorder through ApplyRoute.
pkg/console/subresource/route/route.go still uses context.TODO() for GET/Create/Update and doesn’t accept a recorder, so this path ignores cancellation and won’t emit route apply events like the other resource helpers.
🤖 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/console/operator/sync_v400.go` around lines 137 - 138, Update the route
apply path so it threads the caller’s ctx and a recorder into
routesub.ApplyRoute instead of relying on route.go’s context.TODO() behavior.
Adjust ApplyRoute in the route subresource helper to accept ctx and recorder,
then use that ctx for GET/Create/Update and emit apply events consistently with
the other resource helpers. Update the sync_v400 call site in operator sync
logic to pass the existing context and recorder through this route application
flow.
Source: Coding guidelines
| if len(routeSyncErrors) > 0 { | ||
| statusHandler.AddConditions(status.HandleProgressingOrDegraded( | ||
| "AdditionalRouteSync", | ||
| "FailedAdditionalRoutes", | ||
| fmt.Errorf("failed to sync additional routes: %s", strings.Join(routeSyncErrors, "; ")), | ||
| )) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
AdditionalRouteSync condition is never cleared on success.
Every other status call in this function (CustomLogoSync, TechPreviewSync, ConfigMapSync, etc.) invokes status.HandleProgressingOrDegraded(...) unconditionally, passing a nil error on success so the handler clears any previously-set Degraded/Progressing state. Here, AddConditions is only called if len(routeSyncErrors) > 0, so once a transient route failure sets AdditionalRouteSyncDegraded=True, it will never be reset back to false even after the routes start succeeding again — leaving the ClusterOperator permanently degraded for a resolved issue.
🐛 Proposed fix to always report the condition
- if len(routeSyncErrors) > 0 {
- statusHandler.AddConditions(status.HandleProgressingOrDegraded(
- "AdditionalRouteSync",
- "FailedAdditionalRoutes",
- fmt.Errorf("failed to sync additional routes: %s", strings.Join(routeSyncErrors, "; ")),
- ))
- }
+ var additionalRouteSyncErr error
+ if len(routeSyncErrors) > 0 {
+ additionalRouteSyncErr = fmt.Errorf("failed to sync additional routes: %s", strings.Join(routeSyncErrors, "; "))
+ }
+ statusHandler.AddConditions(status.HandleProgressingOrDegraded("AdditionalRouteSync", "FailedAdditionalRoutes", additionalRouteSyncErr))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(routeSyncErrors) > 0 { | |
| statusHandler.AddConditions(status.HandleProgressingOrDegraded( | |
| "AdditionalRouteSync", | |
| "FailedAdditionalRoutes", | |
| fmt.Errorf("failed to sync additional routes: %s", strings.Join(routeSyncErrors, "; ")), | |
| )) | |
| } | |
| var additionalRouteSyncErr error | |
| if len(routeSyncErrors) > 0 { | |
| additionalRouteSyncErr = fmt.Errorf("failed to sync additional routes: %s", strings.Join(routeSyncErrors, "; ")) | |
| } | |
| statusHandler.AddConditions(status.HandleProgressingOrDegraded("AdditionalRouteSync", "FailedAdditionalRoutes", additionalRouteSyncErr)) |
🤖 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/console/operator/sync_v400.go` around lines 145 - 151, The
AdditionalRouteSync status is only reported when there are route sync errors, so
the condition is never cleared after recovery. Update the AdditionalRouteSync
block in sync_v400.go to call statusHandler.AddConditions with
status.HandleProgressingOrDegraded unconditionally, matching CustomLogoSync and
the other sync handlers, and pass a nil error when routeSyncErrors is empty so
any previously set degraded/progressing state is reset.
| func AdditionalRoute(spec configv1.ComponentRouteSpec) *routev1.Route { | ||
| route := resourceread.ReadRouteV1OrDie(bindata.MustAsset("assets/routes/console-route.yaml")) | ||
| route.Name = string(spec.Name) | ||
| route.Spec.Host = string(spec.Hostname) | ||
| if route.Labels == nil { | ||
| route.Labels = make(map[string]string) | ||
| } | ||
| route.Labels[additionalRouteLabel] = "true" | ||
| return route | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## route.go around AdditionalRoute'
nl -ba pkg/console/subresource/route/route.go | sed -n '280,380p'
echo
echo '## search for AdditionalRoute usages and serving cert handling'
rg -n "AdditionalRoute|ServingCertKeyPairSecret|servingCertKeyPairSecret|consumingUser|consumingUsers|additionalRouteLabel|additionalConsoleBaseAddresses|additional-base-addresses" pkg bindata manifests -g '!vendor'
echo
echo '## route asset'
nl -ba bindata/assets/routes/console-route.yaml | sed -n '1,220p'
echo
echo '## route subresource package outline'
ast-grep outline pkg/console/subresource/route/route.go --view expandedRepository: openshift/console-operator
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## pkg/console/subresource/route/route.go around AdditionalRoute'
sed -n '280,380p' pkg/console/subresource/route/route.go
echo
echo '## search for serving cert / consuming user / additional route related code'
rg -n "AdditionalRoute|ServingCertKeyPairSecret|servingCertKeyPairSecret|consumingUser|consumingUsers|additionalRouteLabel|additionalConsoleBaseAddresses|additional-base-addresses" pkg bindata manifests
echo
echo '## bindata/assets/routes/console-route.yaml'
sed -n '1,220p' bindata/assets/routes/console-route.yamlRepository: openshift/console-operator
Length of output: 4783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## route.go top section'
sed -n '1,120p' pkg/console/subresource/route/route.go
echo
echo '## route controller around additional route handling'
sed -n '380,470p' pkg/console/controllers/route/controller.go
echo
echo '## sync_v400 additional route usage'
sed -n '110,165p' pkg/console/operator/sync_v400.go
echo
echo '## route tests'
sed -n '250,290p' pkg/console/subresource/route/route_test.goRepository: openshift/console-operator
Length of output: 9618
Additional routes should carry the configured serving cert. AdditionalRoute only copies the name, host, and label; it drops spec.ServingCertKeyPairSecret, so extra console hostnames can’t use the admin-provided TLS cert and will fail for custom domains. pkg/console/subresource/route/route.go:334-343
🤖 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/console/subresource/route/route.go` around lines 334 - 343,
AdditionalRoute currently copies only the route name, host, and label, so it
drops the configured serving cert from ComponentRouteSpec. Update
AdditionalRoute in route.go to propagate spec.ServingCertKeyPairSecret onto the
returned Route so extra console hostnames can use the admin-provided TLS cert,
while keeping the existing name/host/label behavior intact.
2 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 PR adds operator-side support for discovering, creating, and managing additional console routes from
ingress.config.openshift.io/clustercomponentRoutes.Story: CONSOLE-5122
Companion console backend PR: openshift/console#16686
Solution description:
When a cluster admin adds a
componentRoutetoingress.config.openshift.io/clustertargeting theopenshift-consolenamespace, the operator now:console.openshift.io/additional-route: true)/auth/callbackon the OAuthClientadditionalConsoleBaseAddressesin the console ConfigMap so the console backend can handle multi-URL CSRF and dynamic OAuth redirectsCommit 1 — Route discovery + creation: Adds
GetAdditionalComponentRouteSpecs,GetAdditionalRouteHostnames, andAdditionalRouteto the route subresource. Filters componentRoutes for namespace=openshift-console, excludes known routes (console, downloads, console-custom, downloads-custom). Includes 13 test cases.Commit 2 — ConfigMap support: Adds
AdditionalConsoleBaseAddressesfield toClusterInfo,AdditionalHosts()builder method, and wires throughDefaultConfigMap.Commit 3 — OAuth multi-host: Adds
SetRedirectURIs(plural) for registering multiple redirect URIs. UpdatesRegisterConsoleToOAuthClientto accept additional hosts via variadic parameter. ExistingSetRedirectURIdelegates to the new function. Includes 5 test cases.Commit 4 — Sync loop wiring: Integrates all three concerns into
sync_v400and the OAuth controller. Only successfully-created routes have their hostnames passed to the ConfigMap. Route creation errors are aggregated into a singleAdditionalRouteSyncdegraded condition without blocking the primary console.All changes are backward compatible — when no additional componentRoutes are configured, behavior is identical to before.
Test setup:
Requires the companion console backend PR (openshift/console#16686) for full end-to-end testing. The operator changes can be tested independently by verifying:
additionalConsoleBaseAddressesTest cases:
go test ./pkg/console/subresource/route/...— route discovery, filtering, template creation (13 cases)go test ./pkg/console/subresource/oauthclient/...— multi-host redirect URI registration (5 cases)go test ./pkg/console/subresource/configmap/...— existing tests pass with new parametergo test ./pkg/console/operator/...— operator tests passgo build ./...— clean compilationBrowser conformance:
Operator-only changes; browser testing covered by companion console PR.
Additional info:
This is part of a multi-repo effort:
labelsfield toComponentRouteSpec(PR #2845)Not in scope (separate stories):
servingCertKeyPairSecretfor additional routesSummary by CodeRabbit