128 external gateway option - #129
Conversation
📝 WalkthroughWalkthroughThis PR adds Kubernetes Gateway API external gateway support to both the flame-hub and flame-node Helm charts. It introduces reusable helper templates that conditionally route HTTPRoutes to either user-provided external gateways or chart-managed defaults, along with updated configuration schemas and conditional Gateway resource rendering. ChangesFlame Hub Gateway API External Gateway Support
Flame Node Gateway API External Gateway Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 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 `@charts/flame-node/templates/_helpers.tpl`:
- Around line 473-475: The helper flame-node.gateway.routeHostname currently
blindly runs regexReplaceAll on .Values.expose.hostname and can emit an empty
string; update it to first check that .Values.expose.hostname is non-empty
(trimmed) and that the extracted hostname passes a DNS-name validation (use
regexMatch with a DNS-safe pattern) before returning the regexReplaceAll result;
if the value is empty or fails the DNS regex, return nothing (so callers won't
render an invalid spec.hostnames entry). Use the existing helper name
flame-node.gateway.routeHostname and the symbols .Values.expose.hostname,
regexReplaceAll and regexMatch to locate and implement this conditional
validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd3c80ff-95e9-4448-8dfe-464cc6dbf2d7
📒 Files selected for processing (17)
charts/flame-hub/templates/_helpers.tplcharts/flame-hub/templates/authup/httproute.yamlcharts/flame-hub/templates/client-ui/httproute.yamlcharts/flame-hub/templates/gateway.yamlcharts/flame-hub/templates/harbor/httproute.yamlcharts/flame-hub/templates/server-core/httproute.yamlcharts/flame-hub/templates/server-messenger/httproute.yamlcharts/flame-hub/templates/server-storage/httproute.yamlcharts/flame-hub/templates/server-telemetry/httproute.yamlcharts/flame-hub/values.yamlcharts/flame-node/templates/_helpers.tplcharts/flame-node/templates/gateway.yamlcharts/flame-node/templates/hub-adapter/httproute.yamlcharts/flame-node/templates/keycloak/httproute.yamlcharts/flame-node/templates/ui/httproute.yamlcharts/flame-node/templates/victoria-logs/httproute.yamlcharts/flame-node/values.yaml
| {{- define "flame-node.gateway.routeHostname" -}} | ||
| {{- regexReplaceAll "^https?://(.*)" .Values.expose.hostname "${1}" -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kubernetes Gateway API v1, can HTTPRoute.spec.hostnames contain an empty string, or must each entry be a valid DNS hostname?
💡 Result:
In Kubernetes Gateway API v1, each entry in HTTPRoute.spec.hostnames must be a valid DNS hostname and cannot be an empty string [1][2]. The Gateway API defines a hostname as a fully qualified domain name of a network host, as defined by RFC 1123 [2]. The field is subject to strict validation, which prohibits empty strings [3]. Specifically, the validation logic enforces a pattern that requires valid DNS labels [2][3]. While the hostnames field is optional—meaning you can omit it entirely to match all hostnames—any individual entry provided within the list must conform to the required DNS format [1][4][2]. IP addresses are also explicitly disallowed in this field [1][2].
Citations:
- 1: https://gateway-api.sigs.k8s.io/api-types/httproute/
- 2: https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/httproute_types.go
- 3: GatewaySpec Listener CEL block empty Hostname if similar Listeners also present kubernetes-sigs/gateway-api#2369
- 4: https://gateway-api.sigs.k8s.io/concepts/hostnames/
🏁 Script executed:
cat -n charts/flame-node/templates/_helpers.tpl | sed -n '470,500p'Repository: PrivateAIM/helm
Length of output: 1372
🏁 Script executed:
# Search for usages of the routeHostname helper
rg "routeHostname" charts/flame-node/templates/ -nRepository: PrivateAIM/helm
Length of output: 745
🏁 Script executed:
# Check one of the HTTPRoute files to see context of helper usage
cat -n charts/flame-node/templates/ui/httproute.yaml | head -30Repository: PrivateAIM/helm
Length of output: 920
🏁 Script executed:
# Look for expose.hostname defaults in values files
find charts/flame-node -name "values*.yaml" -exec grep -l "expose" {} \;Repository: PrivateAIM/helm
Length of output: 123
🏁 Script executed:
# Check the values for expose settings
rg "expose:" charts/flame-node -A 10 -B 2 | head -50Repository: PrivateAIM/helm
Length of output: 1105
Add required hostname validation in the shared route-hostname helper.
This helper can currently return an empty string when expose.hostname is unset (defaults to "" in values.yaml), which then propagates to HTTPRoute spec.hostnames and produces invalid manifests. Per Kubernetes Gateway API v1, each hostname entry must be a valid DNS name and cannot be empty.
Suggested patch
{{- define "flame-node.gateway.routeHostname" -}}
-{{- regexReplaceAll "^https?://(.*)" .Values.expose.hostname "${1}" -}}
+{{- $hostname := required "expose.hostname is required when expose.type=gateway" .Values.expose.hostname -}}
+{{- regexReplaceAll "^https?://(.*)" $hostname "${1}" -}}
{{- end -}}📝 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.
| {{- define "flame-node.gateway.routeHostname" -}} | |
| {{- regexReplaceAll "^https?://(.*)" .Values.expose.hostname "${1}" -}} | |
| {{- end -}} | |
| {{- define "flame-node.gateway.routeHostname" -}} | |
| {{- $hostname := required "expose.hostname is required when expose.type=gateway" .Values.expose.hostname -}} | |
| {{- regexReplaceAll "^https?://(.*)" $hostname "${1}" -}} | |
| {{- end -}} |
🤖 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 `@charts/flame-node/templates/_helpers.tpl` around lines 473 - 475, The helper
flame-node.gateway.routeHostname currently blindly runs regexReplaceAll on
.Values.expose.hostname and can emit an empty string; update it to first check
that .Values.expose.hostname is non-empty (trimmed) and that the extracted
hostname passes a DNS-name validation (use regexMatch with a DNS-safe pattern)
before returning the regexReplaceAll result; if the value is empty or fails the
DNS regex, return nothing (so callers won't render an invalid spec.hostnames
entry). Use the existing helper name flame-node.gateway.routeHostname and the
symbols .Values.expose.hostname, regexReplaceAll and regexMatch to locate and
implement this conditional validation.
maxju
added
deploy/preview
1 participant
#128
Summary by CodeRabbit
New Features
Refactor