ROSAENG-9174 | feat(hcp): add component_routes support to HCP default ingress resource#1187
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Hi @reedcort. Thanks for your PR. I'm waiting for a terraform-redhat 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. |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR extends the HCP default ingress Terraform resource to support an optional ChangesComponent Routes Feature
Mock Cleanup
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 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 |
![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)
provider/defaultingress/hcp/resource.go (1)
361-361: 💤 Low valueConsider defensive type assertion for
Elements()iteration.The code casts
vtotypes.Objectwithout a type assertion check. While the schema guaranteesElementTypeisbasetypes.ObjectType, a defensive type assertion would prevent potential panics and provide clearer error handling.🛡️ Suggested defensive type assertion
for k, v := range plan.ComponentRoutes.Elements() { + obj, ok := v.(types.Object) + if !ok { + diags.AddError( + "Invalid component route element type", + fmt.Sprintf("Expected types.Object, got %T for component route %s", v, k), + ) + continue + } componentRouteBuilder := cmv1.NewComponentRoute() - hostname, tlsSecretRef := defaultingress.ExpandComponentRoute(ctx, v.(types.Object), diags) + hostname, tlsSecretRef := defaultingress.ExpandComponentRoute(ctx, obj, diags) componentRouteBuilder.Hostname(hostname)🤖 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 `@provider/defaultingress/hcp/resource.go` at line 361, The loop over Elements() calls defaultingress.ExpandComponentRoute(ctx, v.(types.Object), diags) but uses a direct type assertion that can panic; change it to a defensive check using the comma-ok form (e.g., obj, ok := v.(types.Object)) inside the Elements() iteration and only call ExpandComponentRoute when ok is true; if not ok, append a diagnostic or log an error including the unexpected type and continue to the next element so ExpandComponentRoute and hostname/tlsSecretRef are only used with a valid types.Object.
🤖 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 `@provider/defaultingress/hcp/resource.go`:
- Line 191: DefaultIngressResource.Update/Create currently pass diag.Diagnostics
by value into updateIngress → getDefaultIngressBuilder →
defaultingress.ExpandComponentRoute so Append calls inside ExpandComponentRoute
don't propagate back to resp.Diagnostics; change the signatures of
updateIngress, getDefaultIngressBuilder and defaultingress.ExpandComponentRoute
to accept *diag.Diagnostics (pointer) instead of diag.Diagnostics, update all
call sites to pass &diags (and &resp.Diagnostics where used), and follow the
same pattern used by ValidateStateAndPlanEquals(..., &diags) to ensure appended
diagnostics are visible to the caller.
---
Nitpick comments:
In `@provider/defaultingress/hcp/resource.go`:
- Line 361: The loop over Elements() calls
defaultingress.ExpandComponentRoute(ctx, v.(types.Object), diags) but uses a
direct type assertion that can panic; change it to a defensive check using the
comma-ok form (e.g., obj, ok := v.(types.Object)) inside the Elements()
iteration and only call ExpandComponentRoute when ok is true; if not ok, append
a diagnostic or log an error including the unexpected type and continue to the
next element so ExpandComponentRoute and hostname/tlsSecretRef are only used
with a valid types.Object.
🪄 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: 7d3efed8-5d4d-462c-8e71-98ac0a96fe63
📒 Files selected for processing (5)
docs/resources/hcp_default_ingress.mdprovider/common/mock_clusterclient.goprovider/common/mock_clusterwait.goprovider/defaultingress/hcp/resource.goprovider/defaultingress/hcp/state.go
💤 Files with no reviewable changes (2)
- provider/common/mock_clusterclient.go
- provider/common/mock_clusterwait.go
…ess resource Add component_routes (console, downloads) to the rhcs_hcp_default_ingress resource schema, matching the existing Classic ingress resource pattern. OAuth component routes are not supported on HCP clusters because the OAuth server runs on the management cluster control plane. Signed-off-by: Cortney Reed <creed@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
reedcort
force-pushed
the
ROSAENG-9174-hcp-component-routes
branch
from
0590736 to
826b418
Compare
1 participant
PR Summary
Add
component_routessupport to therhcs_hcp_default_ingressresource, enabling ROSA HCP customers to configure custom hostnames and TLS certificates for Console and Downloads routes — matching feature parity with the Classicrhcs_default_ingressresource.Detailed Description of the Issue
ROSA HCP customers cannot customize hostnames for Console or Downloads routes. This blocks customers migrating from ROSA Classic who rely on custom component routes, and those with strict enterprise domain/compliance requirements (e.g., GovCloud). The Classic default ingress resource already supports
component_routes, but the HCP resource only exposedlistening_method.Related Issues and PRs
Type of Change
Previous Behavior
The
rhcs_hcp_default_ingressresource only supported thelistening_methodattribute. Customers could not configurecomponent_routesfor HCP clusters via Terraform. The field was silently ignored.Behavior After This Change
The
rhcs_hcp_default_ingressresource now supports an optionalcomponent_routesmap attribute for Console and Downloads routes (OAuth is not supported on HCP — the OAuth server runs on the management cluster control plane). Customers can set custom hostnames and reference TLS secrets:Note: This requires the corresponding clusters-service backend change to propagate componentRoutes to the HostedCluster spec. Without that backend change, the API will accept the value but it won't take effect on the cluster.
How to Test (Step-by-Step)
Preconditions
hypershift-ingress-day-2feature toggle enabledopenshift-confignamespace on the clusterTest Steps
rhcs_hcp_default_ingressresource withcomponent_routesfor consoleterraform plan— should show the component_routes as a new attributeterraform apply— should update the ingress via the OCM APIoc get ingresses.config.openshift.io cluster -o jsonpath='{.spec.componentRoutes}'Expected Results
terraform planshows the component_routes diff correctlyterraform applysucceedsProof of the Fix
Breaking Changes
Developer Verification Checklist
[JIRA-TICKET] | [TYPE][(scope)][!]: <MESSAGE>.make install-hookshas been run in this clone.make pre-push-checkspasses.make fmt-checkpasses.make buildpasses.Summary by CodeRabbit
New Features
component_routesfield, enabling configuration of component-specific routes with hostname and TLS secret reference settings.Documentation
component_routesconfiguration option and its available properties.