fix: derive internal hostname from gatewayClassName instead of hardcoding istio#1241
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
📝 WalkthroughWalkthrough
ChangesGateway-class-aware host + headless controller
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
added
the
review-effort/large
Patryk-Stefanski
force-pushed
the
fix/1240-internal-host-gateway-class
branch
from
408c9ea to
f3abb57
Compare
![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)
docs/design/umbrella-operator-integration/umbrella-operator-integration-design.md (1)
56-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRework the Design section into the required doc structure.
This reads like implementation notes in places (Helm function names, chart-rendering mechanics, etc.), but
docs/design/**/*.mdneeds explicitPrerequisites,Flow,Component Responsibilities,API Changes, andData storagesubsections. Move the function-level detail out of the main narrative and keep the design focused on behavior and boundaries. As per path instructions,docs/design/**/*.mdmust include sections: Problem, Summary, Goals/Non-Goals, Job Stories, Design (with Prerequisites, Flow, Component Responsibilities, API Changes, Data storage subsections), Security Considerations, Relationship to Existing Approaches, Future Considerations, and Execution.🤖 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 `@docs/design/umbrella-operator-integration/umbrella-operator-integration-design.md` around lines 56 - 280, The Design section needs to be restructured to match the required docs template instead of mixing implementation notes into the narrative. Update the document around the “Design” content to include the missing top-level sections and ensure “Design” has explicit “Prerequisites”, “Flow”, “Component Responsibilities”, “API Changes”, and “Data storage” subsections. Move function-level details like renderChart, buildValues, applyManifest, and migration helpers into the appropriate subsection or out of the main flow, and keep the focus on behavior, boundaries, and responsibilities.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 `@api/v1alpha1/mcpgatewayextension_types.go`:
- Around line 294-306: Update the stale CRD doc comment for
MCPGatewayExtensionSpec.PrivateHost, which still hardcodes the old “-istio”
default. Keep the existing InternalHost behavior as-is, but revise the
PrivateHost field description in MCPGatewayExtensionSpec so it matches the
gatewayClassName-based host convention used by InternalHost and no longer
implies Istio-only naming.
---
Nitpick comments:
In
`@docs/design/umbrella-operator-integration/umbrella-operator-integration-design.md`:
- Around line 56-280: The Design section needs to be restructured to match the
required docs template instead of mixing implementation notes into the
narrative. Update the document around the “Design” content to include the
missing top-level sections and ensure “Design” has explicit “Prerequisites”,
“Flow”, “Component Responsibilities”, “API Changes”, and “Data storage”
subsections. Move function-level details like renderChart, buildValues,
applyManifest, and migration helpers into the appropriate subsection or out of
the main flow, and keep the focus on behavior, boundaries, and responsibilities.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36ffd569-8b07-4760-924f-9ad390401455
📒 Files selected for processing (14)
api/v1alpha1/mcpgatewayextension_types.goapi/v1alpha1/mcpgatewayextension_types_test.gobundle/metadata/dependencies.yamlcharts/mcp-gateway/Chart.yamlcharts/mcp-gateway/templates/deployment-controller.yamlcharts/mcp-gateway/templates/networkpolicy-broker-router.yamlcharts/mcp-gateway/templates/networkpolicy-controller.yamlcharts/mcp-gateway/values.yamlcmd/main.godocs/design/umbrella-operator-integration/umbrella-operator-integration-design.mdinternal/controller/broker_router.gointernal/controller/deployment_test.gointernal/controller/mcpgatewayextension_controller.gointernal/controller/setup_indexes_test.go
💤 Files with no reviewable changes (1)
- bundle/metadata/dependencies.yaml
Patryk-Stefanski
force-pushed
the
fix/1240-internal-host-gateway-class
branch
2 times, most recently
from
7928cfc to
e374d4d
Compare
…ding istio InternalHost computed <gateway>-istio.<ns>.svc.cluster.local regardless of gatewayClassName. With openshift-default the Gateway controller names the Service <gateway>-openshift-default, so the computed hostname did not exist and broker session hairpin failed with a DNS lookup error. Pass gatewayClassName through derivePrivateHost and reconcileBrokerRouter so InternalHost uses the actual class name. privateHost override is unaffected. Update stale privateHost field comments and API reference doc to reflect the gatewayClassName-based convention. Regenerate CRDs and bundle manifests. Fixes #1240. Signed-off-by: Patryk Stefanski <pstefans@redhat.com>
Patryk-Stefanski
force-pushed
the
fix/1240-internal-host-gateway-class
branch
from
e374d4d to
dbe8df6
Compare
2 participants
Summary
InternalHosthardcoded-istiowhen computing the hairpin hostname, causing DNS lookup failures whengatewayClassNameis notistio(e.g.openshift-default)gatewayClassNamethroughderivePrivateHostandreconcileBrokerRouterso the correct Service name is derivedopenshift-defaulttest cases to cover the broken scenarioFixes #1240
Test plan
make test-unitpassesgatewayClassName: openshift-defaultand verify broker hairpin resolves the correct hostname without settingprivateHostSummary by CodeRabbit
New Features
Bug Fixes
Chores