ROSAENG-9174 | feat: enable --component-routes for HCP clusters#3376
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIngress component-route parsing now accepts caller-provided allowed route sets, including HCP routes for Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/edit/ingress/cmd.go (1)
311-419: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestrict
--component-routesto HCP. Moving this block outside the legacy-ingress gate now lets classic clusters that still report legacy support reach the component-routes prompt/path. If this is meant to stay HCP-only, add an explicit legacy check before parsing or prompting.🤖 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/edit/ingress/cmd.go` around lines 311 - 419, Restrict the component-routes parsing and interactive prompt block to clusters without legacy ingress support by adding an explicit !hasLegacyIngressSupport guard around the componentRoutesFlag and interactive.Enabled() paths. Preserve the existing isHypershift route selection and parsing behavior for HCP clusters.
🧹 Nitpick comments (1)
cmd/edit/ingress/cmd.go (1)
374-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated allowed-route selection.
The
allowedRoutes/routesselection (expectedComponentRoutesvsexpectedHcpComponentRoutesbased onisHypershift) is repeated verbatim in both the CLI and interactive branches. Extracting a small helper (e.g.allowedComponentRoutes(isHypershift bool) []string) would remove the duplication.♻️ Proposed refactor
+func allowedComponentRoutes(isHypershift bool) []string { + if isHypershift { + return expectedHcpComponentRoutes + } + return expectedComponentRoutes +} + if cmd.Flags().Changed(componentRoutesFlag) { - allowedRoutes := expectedComponentRoutes - if isHypershift { - allowedRoutes = expectedHcpComponentRoutes - } + allowedRoutes := allowedComponentRoutes(isHypershift) componentRoutes, err = parseComponentRoutesForAllowed(args.componentRoutes, allowedRoutes) ... if confirm.Prompt(false, "Would you like to edit the component routes?") { - routes := expectedComponentRoutes - if isHypershift { - routes = expectedHcpComponentRoutes - } + routes := allowedComponentRoutes(isHypershift)🤖 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/edit/ingress/cmd.go` around lines 374 - 390, Extract the repeated expectedComponentRoutes versus expectedHcpComponentRoutes selection into a helper such as allowedComponentRoutes(isHypershift bool) []string, then use it in both the componentRoutesFlag CLI branch and the interactive branch. Preserve the existing route values and isHypershift behavior.
🤖 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.
Outside diff comments:
In `@cmd/edit/ingress/cmd.go`:
- Around line 311-419: Restrict the component-routes parsing and interactive
prompt block to clusters without legacy ingress support by adding an explicit
!hasLegacyIngressSupport guard around the componentRoutesFlag and
interactive.Enabled() paths. Preserve the existing isHypershift route selection
and parsing behavior for HCP clusters.
---
Nitpick comments:
In `@cmd/edit/ingress/cmd.go`:
- Around line 374-390: Extract the repeated expectedComponentRoutes versus
expectedHcpComponentRoutes selection into a helper such as
allowedComponentRoutes(isHypershift bool) []string, then use it in both the
componentRoutesFlag CLI branch and the interactive branch. Preserve the existing
route values and isHypershift behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: de6a5231-9f6e-4819-8334-145751c3598c
📒 Files selected for processing (3)
cmd/edit/ingress/cmd.gocmd/edit/ingress/cmd_test.gocmd/edit/ingress/flags.go
aaea152 to
39a9017
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/edit/ingress/flags.go`:
- Around line 105-113: Update parseComponentRoutesForAllowed to track component
names already encountered and return an error when a duplicate is parsed, before
assigning its builder into result. Preserve the existing count and allowed-route
validation while preventing duplicate names from overwriting earlier entries.
🪄 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: 0270b2b7-c7b9-4f2b-9d13-b1efeed39315
📒 Files selected for processing (3)
cmd/edit/ingress/cmd.gocmd/edit/ingress/cmd_test.gocmd/edit/ingress/flags.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/edit/ingress/cmd_test.go
- cmd/edit/ingress/cmd.go
39a9017 to
55bc548
Compare
| } | ||
| } | ||
|
|
||
| if cmd.Flags().Changed(componentRoutesFlag) { |
There was a problem hiding this comment.
Moving this block outside the legacy-guard also unblocks classic clusters with legacy ingress support, which were previously blocked by both the V2 flag check and the !hasLegacyIngressSupport guard. Wrap this section in if !hasLegacyIngressSupport || isHypershift to preserve the intended restriction.
There was a problem hiding this comment.
Relating the reviews with Cursor
+1 on the !hasLegacyIngressSupport || isHypershift fix after moving component-routes outside the legacy block. nit: extract canEditComponentRoutes := !hasLegacyIngressSupport || isHypershift and use it for both flag (!canEditComponentRoutes) and interactive paths instead of duplicating inverse logic. tests: HCP parser tests are solid; please add coverage for this guard (legacy classic blocked, HCP allowed) — that’s the regression this PR was at risk of introducing.
#3376 (comment)
@coderabbitai confirm relation with a short answer.
There was a problem hiding this comment.
@amandahla Confirmed — this is the same concern as olucasfreitas' comment. Both target the componentRoutes block in cmd/edit/ingress/cmd.go that got moved outside the !hasLegacyIngressSupport guard; your note just refines that fix by suggesting the canEditComponentRoutes := !hasLegacyIngressSupport || isHypershift extraction (reused for both the flag and interactive paths) plus asking for regression test coverage (classic legacy blocked, HCP allowed).
🧠 Learnings used
Learnt from: amandahla
Repo: openshift/rosa PR: 3333
File: cmd/list/cluster/cmd.go:127-127
Timestamp: 2026-07-07T14:43:34.653Z
Learning: For ROSA command implementations under `cmd/` that need to detect HyperShift/Hypershift enablement, prefer calling the nil-safe helper `pkg/ocm.IsHyperShiftCluster(cluster)` rather than directly checking `cluster.Hypershift().Enabled()` (e.g., when `Hypershift()` could be nil). Keep behavior consistent with existing commands that already reuse this helper (such as the upgrade/list/edit/describe/create/delete OIDC/ingress flows).
|
@reedcort ci/prow/lint is required and currently failing. The make lint checkbox is also unchecked — please fix lint issues before requesting review. |
|
Nitpick — This help text lists oauth as a valid key, but HCP clusters reject it. Consider appending a note like '(HCP clusters support console and downloads only)' so users don't need to discover this via a runtime error. |
|
Re: @coderabbitai's concern about restricting This is valid and still unresolved in the latest commit. The extraction opens component routes to classic clusters with legacy ingress support, which was previously blocked. Adding a |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
sed -n '280,430p' cmd/edit/ingress/cmd.goLength of output: 5858
Your suggested guard is correct and minimal — wrapping the extracted block with if !hasLegacyIngressSupport || isHypershift {
if cmd.Flags().Changed(componentRoutesFlag) {
componentRoutes, err = parseComponentRoutesForAllowed(args.componentRoutes, allowedComponentRoutes(isHypershift))
if err != nil {
r.Reporter.Errorf("An error occurred whilst parsing the supplied component routes: %s", err)
os.Exit(1)
}
} else if interactive.Enabled() {
componentRoutes = map[string]*cmv1.ComponentRouteBuilder{}
if confirm.Prompt(false, "Would you like to edit the component routes?") {
for _, componentRoute := range allowedComponentRoutes(isHypershift) {
...
}
}
}
}This restores parity with the pre-PR behavior for classic clusters while keeping component-routes support for HCP clusters as intended by this PR. ✏️ Learnings added
|
55bc548 to
eba1d50
Compare
eba1d50 to
da1a67f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/edit/ingress/cmd_test.go`:
- Around line 95-106: Update the wrong-count test around
parseComponentRoutesForAllowed to provide only valid HCP component routes, such
as a single console route, and change the expected error to report 2 expected
and 1 supplied. Keep the test focused solely on cardinality validation rather
than invalid component-name handling.
- Around line 52-71: The test case around parseComponentRoutesForAllowed should
explicitly assert that the returned componentRouteBuilder keys are exactly
console and downloads before validating route values. Collect the keys, compare
them against the expected key set, then retain the existing hostname and TLS
secret assertions for each returned route.
🪄 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: 81f40cb0-56f5-41a8-b4c5-2c02041d72a8
📒 Files selected for processing (3)
cmd/edit/ingress/cmd.gocmd/edit/ingress/cmd_test.gocmd/edit/ingress/flags.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/edit/ingress/cmd.go
- cmd/edit/ingress/flags.go
Remove component-routes from the exclusivelyIngressV2Flags list so rosa edit ingress --component-routes works on HCP clusters. Add expectedHcpComponentRoutes with only console and downloads (oauth is not configurable via component routes on HCP). Move componentRoutes handling out of the !hasLegacyIngressSupport block so it runs for all cluster types. Interactive mode prompts for console and downloads only on HCP, all three on classic. Signed-off-by: Cortney Reed <creed@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
da1a67f to
33605ca
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olucasfreitas, reedcort 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
|
@olucasfreitas: Overrode contexts on behalf of olucasfreitas: ci/prow/govulncheck, ci/prow/security 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 kubernetes-sigs/prow repository. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3376 +/- ##
==========================================
+ Coverage 28.52% 28.57% +0.05%
==========================================
Files 333 333
Lines 36691 36747 +56
==========================================
+ Hits 10467 10502 +35
- Misses 25469 25490 +21
Partials 755 755 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@reedcort: 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. |
3 participants
PR Summary
Enable
--component-routesflag and interactive mode for HCP clusters inrosa edit ingress, allowing customers to configure custom console and downloads hostnames with TLS certificates.Detailed Description of the Issue
ROSA HCP customers cannot use
rosa edit ingress --component-routesto set custom hostnames for Console and Downloads routes. The CLI blocks all ingress V2 flags for HCP clusters, includingcomponent-routes. The backend (clusters-service) and HyperShift operator now both support componentRoutes for HCP, but the CLI guard rail prevents customers from using the feature.Related Issues and PRs
Type of Change
Previous Behavior
rosa edit ingress --component-routeson an HCP cluster returned:Behavior After This Change
rosa edit ingress --component-routesworks on HCP clusters for console and downloads routes. OAuth is excluded because it runs on the management cluster and is not configurable via component routes on HCP.Flag mode:
Interactive mode (
-i): prompts for console and downloads only (no oauth).Validation:
oauthas a component name returns:'oauth' is not a valid component name. Expected include [console, downloads]How to Test (Step-by-Step)
Preconditions
hypershift-component-routesfeature toggle enabledopenshift-confignamespace on the hosted clusterTest Steps
rosa edit ingress -c <cluster-name> --component-routes 'console: hostname=console.custom.example.com;tlsSecretRef=console-tls-secret, downloads: hostname=downloads.custom.example.com;tlsSecretRef=downloads-tls-secret' <ingress-id>oc get ingresses.config.openshift.io cluster -o jsonpath='{.spec.componentRoutes}'oc get routes -n openshift-consolerosa edit ingress -c <cluster-name> -i <ingress-id>— should prompt for console and downloads onlyrosa edit ingress -c <cluster-name> --excluded-namespaces 'test' <ingress-id>Expected Results
console-custom,downloads-custom)Proof of the Fix
make test:
Flag mode — set console + downloads:
Propagation verified:
[ {"hostname":"console.custom.example.com","name":"console","namespace":"openshift-console","servingCertKeyPairSecret":{"name":"console-tls-secret"}}, {"hostname":"downloads.custom.example.com","name":"downloads","namespace":"openshift-console","servingCertKeyPairSecret":{"name":"downloads-tls-secret"}} ]Custom routes created:
OAuth rejection:
Hostname without secret — backend 400:
Cleanup via empty values:
Interactive mode — set values (no oauth prompted):
Interactive mode — clear console, keep downloads:
Breaking Changes
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit
New Features
Bug Fixes
Tests