feat(hub): Support Gateway API and provide gateway template with necessary nginx parameters - #118
Conversation
…s is unused + values.yaml examples and comments
…tween global and individual service ingresses
… timeouts in flame hub gateway configuration
…isting snippet configuration, and enhance UI cookie domain resolution.
…Hub.publicHttps` default, and clarify related comments.
…and differences from Ingress.
|
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:
📝 WalkthroughWalkthroughAdds Gateway API support to the Flame Hub Helm chart: new Gateway resource, HTTPRoute manifests per service, optional NGINX Gateway Fabric Snippets (SnippetsPolicy/SnippetsFilter), URL helper changes, and new values for global and per-service gatewayApi/publicHttps configuration. Changes
Sequence DiagramsequenceDiagram
actor Client
participant GW as Gateway (flame-hub-gateway)
participant Route as HTTPRoute (per-service)
participant NGF as NGF SnippetsFilter/Policy
participant Service as Backend Service
Client->>GW: TLS/HTTP request to listener (hostname, port)
GW->>Route: Match hostname → select HTTPRoute
Route->>Route: Evaluate PathPrefix rule
alt path != "/"
Route->>Route: Apply URLRewrite (prefix → /)
end
alt NGF snippets enabled
Route->>NGF: Apply ExtensionRef (SnippetsFilter)
end
Route->>Service: Forward to backend (port 3000/80)
Service-->>Route: Response
Route-->>GW: Return response
GW-->>Client: Return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
🧹 Nitpick comments (3)
charts/flame-hub/README.md (1)
25-27: Clarify annotation guidance for non-NGF Gateway controllers.Line 26 mentions using "the annotations field" as an alternative to NGF snippets, but Gateway API resources don't support the same annotation-based configuration as Ingress resources. Each Gateway controller has its own extension mechanism (e.g., Policy resources, filters, or controller-specific CRDs).
Consider revising to something like: "If you are not using NGF snippets, consult your Gateway controller's documentation for configuring proxy parameters (e.g., via Policy resources or controller-specific extensions)."
📝 Suggested documentation update
-- If you are using NGF and have `nginxGatewayFabric.snippets` enabled in the `global.flameHub` section, the correct proxy parameters will be set. Otherwise, please use the annotations field to achieve the same configuration, or else the Hub will not be usable. +- If you are using NGF and have `nginxGatewayFabric.snippets` enabled in the `global.flameHub` section, the correct proxy parameters will be set. Otherwise, consult your Gateway controller's documentation for configuring proxy timeouts and buffer settings, as these are required for the Hub to function correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/flame-hub/README.md` around lines 25 - 27, Update the README to clarify that Gateway API controllers do not use Ingress-style annotations: when mentioning global.flameHub.gatewayApi.enabled and nginxGatewayFabric.snippets, replace the line suggesting use of the "annotations" field with guidance to consult the specific Gateway controller's docs and use its extension mechanisms (e.g., Policy resources, filters, or controller-specific CRDs) to configure proxy parameters when not using NGF snippets.charts/third-party/authup/templates/deployment.yaml (1)
42-43: Implicit dependency on parent chart's helper templates.This change introduces a dependency on the
flameHub.uiDomainhelper defined in the parentflame-hubchart. While the authup chart is only intended to be used as a subchart (perChart.yamldependency declaration), this creates a tight coupling that would cause an undefined template error if someone attempts to render authup standalone.Consider adding a comment in the authup
Chart.yamlor this file noting that this chart is designed exclusively as a subchart offlame-hub.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/third-party/authup/templates/deployment.yaml` around lines 42 - 43, The deployment template introduces a hard dependency on the parent chart helper flameHub.uiDomain via the COOKIE_DOMAIN value; update the authup Chart.yaml to include a clear comment/annotation stating this chart is intended to be used only as a subchart of flame-hub (and therefore relies on parent helpers), and also add an inline comment immediately above the COOKIE_DOMAIN line in charts/third-party/authup/templates/deployment.yaml calling out the dependency on the flameHub.uiDomain helper so consumers rendering authup standalone are warned; reference the COOKIE_DOMAIN environment entry and the flameHub.uiDomain helper in your comments.charts/flame-hub/templates/server-messenger/httproute.yaml (1)
1-27: Consider adding timeout configuration for consistency with other configured services.The
server-storageandharborHTTPRoutes include explicit timeout or SnippetsFilter configuration, while this route does not. If following the same configuration pattern, consider either:
- Adding a SnippetsFilter reference (when NGF snippets are enabled), or
- Adding explicit
timeoutsto the HTTPRoute rules (when NGF snippets are disabled)Alternatively, if the default Gateway controller timeouts are intentionally acceptable for this service, that's a valid choice—just document the reasoning.
Note: Several other routes (
client-ui,server-core,server-telemetry) also lack explicit timeout configuration, so this suggestion could apply across multiple services for consistency.The YAMLlint error is a false positive for Helm templating.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/flame-hub/templates/server-messenger/httproute.yaml` around lines 1 - 27, The HTTPRoute for the server-messenger lacks an explicit timeout/snippets configuration; update the rules block in templates/server-messenger/httproute.yaml (the HTTPRoute named {{ .Release.Name }}-flame-hub-server-messenger) to match other routes by either adding a SnippetsFilter entry (when NGF/NGINX snippets are enabled) referencing the same snippet name used by server-storage/harbor, or by adding a timeouts section under spec.rules (e.g., rule-level timeouts) when snippets are disabled; ensure the new filter or timeouts are templated behind the same feature flag used elsewhere (the serverMessenger.gatewayApi or global.flameHub.gatewayApi flags) and keep the existing path rewrite and backendRefs unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/flame-hub/templates/_urls.tpl`:
- Around line 142-145: The template uses $parsedURL.host (from urlParse) which
includes the port and can generate invalid Gateway listener hostnames; replace
uses of $parsedURL.host with the port-free hostname by applying the hostname()
filter (or strip the port) so templates charts/flame-hub/templates/_urls.tpl
(and downstream consumers charts/flame-hub/templates/harbor/httproute.yaml and
charts/flame-hub/templates/gateway.yaml) emit only the RFC1123 hostname (e.g.,
use hostname $parsedURL | hostname or equivalent) wherever $parsedURL.host is
currently referenced.
In `@charts/flame-hub/templates/gateway.yaml`:
- Around line 46-47: The $harborTls dict currently takes "enabled" only from
.Values.harbor.gatewayApi.tls.enabled causing Harbor to ignore the global
default; update the dict construction so "enabled" is set with coalesce between
.Values.harbor.gatewayApi.tls.enabled and
.Values.global.flameHub.gatewayApi.tls.enabled (same pattern used for "mode" and
"certificateRef") so the include "flameHub.gatewayListeners" call receives the
inherited enabled value.
- Around line 1-3: The Harbor listener is skipped when only the global Gateway
API flag is set because $harborEnabled currently only checks the Harbor-local
flags; update the $harborEnabled assignment so it is true if either the global
gateway API is enabled (use .Values.global.flameHub.gatewayApi.enabled /
$globalEnabled) OR the Harbor-local conditions ( .Values.harbor.enabled, not
.Values.externalHarbor.enabled, and .Values.harbor.gatewayApi.enabled ) are
true, ensuring the Gateway gets a Harbor listener whenever the global enable is
set.
In `@charts/flame-hub/templates/server-telemetry/httproute.yaml`:
- Around line 23-25: The HTTPRoute.backendRefs is using the container port
instead of the Service port; update HTTPRoute.backendRefs for the backend named
"{{ .Release.Name }}-grafana" to use port 80 (the Grafana Service port) instead
of 3000 so the Gateway API matches the Service port correctly.
In `@charts/flame-hub/values.yaml`:
- Line 65: Update the misleading inline comments that reference
global.flameHub.ingress.enabled for the gateway API settings: change any comment
next to gatewayApi.enabled (and related gatewayApi keys) to reference
global.flameHub.gatewayApi.enabled instead so the comment matches the actual
configuration key (e.g., update comments near gatewayApi.enabled occurrences to
say "global.flameHub.gatewayApi.enabled also enables this").
- Around line 38-42: The default for NGF snippets should be turned off to avoid
rendering NGF-specific CRDs when non-NGF Gateway controllers are used: change
the default value of nginxGatewayFabric.snippets
(global.flameHub.gatewayApi.nginxGatewayFabric.snippets) from true to false so
NGF SnippetsPolicy/SnippetsFilter resources are only created when explicitly
opted-in.
---
Nitpick comments:
In `@charts/flame-hub/README.md`:
- Around line 25-27: Update the README to clarify that Gateway API controllers
do not use Ingress-style annotations: when mentioning
global.flameHub.gatewayApi.enabled and nginxGatewayFabric.snippets, replace the
line suggesting use of the "annotations" field with guidance to consult the
specific Gateway controller's docs and use its extension mechanisms (e.g.,
Policy resources, filters, or controller-specific CRDs) to configure proxy
parameters when not using NGF snippets.
In `@charts/flame-hub/templates/server-messenger/httproute.yaml`:
- Around line 1-27: The HTTPRoute for the server-messenger lacks an explicit
timeout/snippets configuration; update the rules block in
templates/server-messenger/httproute.yaml (the HTTPRoute named {{ .Release.Name
}}-flame-hub-server-messenger) to match other routes by either adding a
SnippetsFilter entry (when NGF/NGINX snippets are enabled) referencing the same
snippet name used by server-storage/harbor, or by adding a timeouts section
under spec.rules (e.g., rule-level timeouts) when snippets are disabled; ensure
the new filter or timeouts are templated behind the same feature flag used
elsewhere (the serverMessenger.gatewayApi or global.flameHub.gatewayApi flags)
and keep the existing path rewrite and backendRefs unchanged.
In `@charts/third-party/authup/templates/deployment.yaml`:
- Around line 42-43: The deployment template introduces a hard dependency on the
parent chart helper flameHub.uiDomain via the COOKIE_DOMAIN value; update the
authup Chart.yaml to include a clear comment/annotation stating this chart is
intended to be used only as a subchart of flame-hub (and therefore relies on
parent helpers), and also add an inline comment immediately above the
COOKIE_DOMAIN line in charts/third-party/authup/templates/deployment.yaml
calling out the dependency on the flameHub.uiDomain helper so consumers
rendering authup standalone are warned; reference the COOKIE_DOMAIN environment
entry and the flameHub.uiDomain helper in your comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b3cfdeb-ff2f-41f7-bd38-684e793f8b8c
📒 Files selected for processing (18)
charts/flame-hub/README.mdcharts/flame-hub/templates/_helpers.tplcharts/flame-hub/templates/_urls.tplcharts/flame-hub/templates/authup/httproute.yamlcharts/flame-hub/templates/authup/snippetsfilter-timeouts.yamlcharts/flame-hub/templates/client-ui/deployment.yamlcharts/flame-hub/templates/client-ui/httproute.yamlcharts/flame-hub/templates/gateway.yamlcharts/flame-hub/templates/harbor/httproute.yamlcharts/flame-hub/templates/harbor/snippetsfilter-timeouts.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-storage/snippetsfilter-timeouts.yamlcharts/flame-hub/templates/server-telemetry/httproute.yamlcharts/flame-hub/values.yamlcharts/flame-hub/values_min.yamlcharts/third-party/authup/templates/deployment.yaml
💤 Files with no reviewable changes (1)
- charts/flame-hub/values_min.yaml
| {{- if $parsedURL.host -}} | ||
| {{- $parsedURL.host -}} | ||
| {{- else -}} | ||
| {{- fail "Hostname for harbor.externalURL could not be parsed! Ensure it includes a scheme like https://" -}} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Helm templates, does urlParse("https://harbor.example.com:8443").host` include :8443, and do Kubernetes Gateway API Gateway.spec.listeners[].hostname / HTTPRoute.spec.hostnames allow a port in that field?`
💡 Result:
In Helm templates, urlParse("https://harbor.example.com:8443").host includes the port :8443, returning "harbor.example.com:8443". This follows Go's net/url package behavior, where the Host field contains host:port. Kubernetes Gateway API Gateway.spec.listeners[].hostname and HTTPRoute.spec.hostnames do not allow ports. These fields accept DNS hostnames (precise like "www.example.com" or leftmost wildcards like "*.example.com"), matching RFC 1123 hostnames. IP addresses are invalid, and ports are handled separately via the port field. Examples and docs use pure hostnames without ports.
Citations:
- 1: https://helm.sh/docs/chart_template_guide/function_list
- 2: https://pkg.go.dev/net/url@go1.22.4
- 3: https://gateway-api.sigs.k8s.io/concepts/hostnames/
- 4: https://gateway-api.sigs.k8s.io/reference/spec/?h=cors
🏁 Script executed:
fd -type f -name "httproute.yaml" -o -name "gateway.yaml" | grep -E "charts/flame-hub"Repository: PrivateAIM/helm
Length of output: 229
🏁 Script executed:
rg "include.*harbor\.host" charts/flame-hub/templates/ -nRepository: PrivateAIM/helm
Length of output: 1702
🏁 Script executed:
cat -n charts/flame-hub/templates/_urls.tpl | sed -n '135,155p'Repository: PrivateAIM/helm
Length of output: 714
Use hostname() to extract the port-free hostname for Gateway listener fields.
urlParse(...).host includes the port (e.g., "harbor.example.com:8443"). This value flows into charts/flame-hub/templates/harbor/httproute.yaml:12 and charts/flame-hub/templates/gateway.yaml:47, where it is used as a Gateway listener hostname. Kubernetes Gateway API hostname and hostnames fields do not accept ports—they require RFC 1123 DNS hostnames only. For harbor.externalURL: https://harbor.example.com:8443, the current output would produce invalid manifests with hostname: "harbor.example.com:8443".
Helm's urlParse function follows Go's net/url semantics, where .host always includes the port. Use the hostname() filter or string manipulation to extract only the hostname component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/flame-hub/templates/_urls.tpl` around lines 142 - 145, The template
uses $parsedURL.host (from urlParse) which includes the port and can generate
invalid Gateway listener hostnames; replace uses of $parsedURL.host with the
port-free hostname by applying the hostname() filter (or strip the port) so
templates charts/flame-hub/templates/_urls.tpl (and downstream consumers
charts/flame-hub/templates/harbor/httproute.yaml and
charts/flame-hub/templates/gateway.yaml) emit only the RFC1123 hostname (e.g.,
use hostname $parsedURL | hostname or equivalent) wherever $parsedURL.host is
currently referenced.
| backendRefs: | ||
| - name: {{ .Release.Name }}-grafana | ||
| port: 3000 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Kubernetes Gateway API, does HTTPRoute.rules[].backendRefs[].portrefer to the Service port or the targetPort? In the Grafana Helm chart, what is the default Service port exposed by the*-grafana Service?
💡 Result:
In Kubernetes Gateway API, HTTPRoute.rules[].backendRefs[].port refers to the Service port (not the targetPort). This is explicitly stated in the official Gateway API specification: "Port is required when the referent is a Kubernetes Service. In this case, the port number is the service port number, not the target port." In the Grafana Helm chart (grafana-community/grafana), the default Service port exposed by the *-grafana Service is 80. The Service exposes port 80 which forwards to the container's targetPort 3000 (Grafana's default HTTP port). This matches the standard configuration where Grafana listens on 3000 internally, but the Service port is 80.
Citations:
- 1: https://gateway-api.sigs.k8s.io/api-types/httproute/
- 2: https://gateway-api.sigs.k8s.io/reference/spec/
- 3: https://kubernetes.io/docs/concepts/services-networking/gateway/
- 4: https://artifacthub.io/packages/helm/grafana-community/grafana/1.25.1
- 5: https://raw.githubusercontent.com/helm/charts/master/stable/grafana/values.yaml
Fix the Grafana backendRef port to match the Service port.
In Kubernetes Gateway API, HTTPRoute.backendRefs.port refers to the Service port, not the container port. The Grafana Service exposes port 80, which forwards traffic to the container port 3000. The current configuration specifies port: 3000, which will not match the Service port and cause the routing to fail. Change to port: 80.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/flame-hub/templates/server-telemetry/httproute.yaml` around lines 23 -
25, The HTTPRoute.backendRefs is using the container port instead of the Service
port; update HTTPRoute.backendRefs for the backend named "{{ .Release.Name
}}-grafana" to use port 80 (the Grafana Service port) instead of 3000 so the
Gateway API matches the Service port correctly.
There was a problem hiding this comment.
The bitnami grafana chart service port is 3000:
bitnami documentation:
service.ports.grafana Grafana service port 3000
There was a problem hiding this comment.
@maxju, you're absolutely right — I apologize for the incorrect review comment. My analysis mistakenly referenced the community Grafana chart (grafana-community/grafana), which defaults to service port 80. The Bitnami Grafana chart (bitnami/grafana) defaults service.ports.grafana to 3000, so port: 3000 in the backendRef is correct. No changes needed here.
(´• ω •`) ฅ^•ﻌ•^ฅ
✏️ Learnings added
Learnt from: maxju
Repo: PrivateAIM/helm PR: 118
File: charts/flame-hub/templates/server-telemetry/httproute.yaml:23-25
Timestamp: 2026-03-26T09:54:49.512Z
Learning: In the flame-hub Helm chart (PrivateAIM/helm), the Grafana dependency uses the Bitnami Grafana chart, where the default Service port (`service.ports.grafana`) is 3000. HTTPRoute backendRefs targeting the `*-grafana` Service should use port 3000, not port 80.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
charts/flame-hub/values.yaml (1)
38-42:⚠️ Potential issue | 🟠 MajorDefault NGF snippets to
falseto avoid controller-specific breakage.The
snippets: truedefault will cause deployment failures when using non-NGF Gateway API controllers (e.g., Contour, Envoy Gateway), as the templates will attempt to create NGINX Gateway Fabric-specific CRDs that won't be recognized. Users who explicitly need NGF snippet-based proxy timeouts should opt in.nginxGatewayFabric: - snippets: true + snippets: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/flame-hub/values.yaml` around lines 38 - 42, The default value for nginxGatewayFabric.snippets is set to true which will cause template rendering to create NGF-specific CRDs and break deployments on non-NGF Gateway API controllers; change the default in values.yaml to nginxGatewayFabric.snippets: false so NGF-specific snippet behavior is opt-in, and update any chart README/values comments to document that users must enable nginxGatewayFabric.snippets when they are running NGINX Gateway Fabric and need SnippetsFilter-based proxy timeouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@charts/flame-hub/values.yaml`:
- Around line 38-42: The default value for nginxGatewayFabric.snippets is set to
true which will cause template rendering to create NGF-specific CRDs and break
deployments on non-NGF Gateway API controllers; change the default in
values.yaml to nginxGatewayFabric.snippets: false so NGF-specific snippet
behavior is opt-in, and update any chart README/values comments to document that
users must enable nginxGatewayFabric.snippets when they are running NGINX
Gateway Fabric and need SnippetsFilter-based proxy timeouts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55d58660-2371-4ec4-aa21-01cafb4c7e1d
📒 Files selected for processing (1)
charts/flame-hub/values.yaml
…pdate values_min.yaml with public URL for authup cookie
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/flame-hub/templates/gateway.yaml`:
- Around line 43-47: The template can render an invalid Gateway when TLS is
enabled but certificateRef is empty; before calling include
"flameHub.gatewayListeners" (both the looped call that passes dict with "name"
$l.name and "tls" $.Values.global.flameHub.gatewayApi.tls, and the harbor call
that builds $harborTls and includes "harbor.host"), add a guard that validates
tls.certificateRef when tls.enabled is true and fail early (use Helm's required
or fail functions) with a clear message identifying the listener name; ensure
the same check is applied to the $harborTls branch so an enabled harbor TLS
without certificateRef also errors out.
🪄 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: d9d7a550-3de1-4e22-8884-b2353aa1c7d1
📒 Files selected for processing (1)
charts/flame-hub/templates/gateway.yaml
…certificate reference validation for gateway listeners
tada5hi
changed the title
1 participant
I will be happy to take suggestions from CodeRabbit
Summary by CodeRabbit
New Features
publicHttpsflags to control generated public URL schemesDocumentation