[platform, system, apps] Add Gateway API support with Envoy Gateway#2213
[platform, system, apps] Add Gateway API support with Envoy Gateway#2213sircthulhu wants to merge 31 commits intomainfrom
Conversation
|
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 and Envoy Gateway support: new CRDs, Envoy Gateway charts/package sources, tenant-scoped gateway HelmRelease, platform wiring and values, per-service Gateway/HTTPRoute/TLSRoute Helm templates, and end-to-end Bats tests validating Gateway behavior and connectivity. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
participant LB as LoadBalancer
participant Gateway as Envoy_Gateway
participant Route as HTTPRoute/TLSRoute
participant Service as Backend
end
Client->>LB: HTTP request (80)
LB->>Gateway: forward request
Gateway->>Route: evaluate route rules
Route-->>Gateway: instruct redirect or forward
Gateway-->>Client: 301 redirect to HTTPS
Client->>LB: HTTPS request (443)
LB->>Gateway: TLS passthrough/terminate
Gateway->>Route: select backend
Route->>Service: forward request
Service-->>Gateway: response
Gateway-->>Client: HTTPS response (200/3xx/401/403)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the platform's networking capabilities by integrating the Kubernetes Gateway API. It provides a modern and flexible alternative to traditional Ingress resources, facilitating a smoother migration path to Cilium Gateway API. The changes introduce a structured approach for both system-wide and tenant-specific traffic management, allowing for fine-grained control over routing and load balancing while ensuring backward compatibility with existing Ingress configurations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for the Gateway API across system and tenant components, providing a clear migration path from ingress-nginx. The changes are well-structured, with a central gateway for system services and per-tenant gateways for tenant services, which is a solid design. The use of HTTPRoute and TLSRoute is appropriate for the different services.
I have a few suggestions for improvement:
- My main concern is the use of the deprecated
gateway.networking.k8s.io/v1alpha2API version forTLSRouteresources. I strongly recommend updating this to a stable version likev1orv1beta1for future compatibility. - I've also identified a couple of areas with repetitive code in the Helm templates where using helper templates or loops could improve maintainability.
Overall, this is a great enhancement. Addressing these points will make the implementation more robust and easier to maintain.
| namespaces: | ||
| from: Same | ||
| --- | ||
| apiVersion: gateway.networking.k8s.io/v1alpha2 |
There was a problem hiding this comment.
The apiVersion for TLSRoute is set to gateway.networking.k8s.io/v1alpha2, which is from an old experimental channel of the Gateway API and has been deprecated. It's recommended to use a more recent, stable version. Given that HTTPRoute resources in this PR are using v1, for consistency and future compatibility, you should update TLSRoute to gateway.networking.k8s.io/v1 if your cluster's Gateway API support is at v1.0.0 or newer. If not, v1beta1 would be a better choice than v1alpha2. This applies to all new TLSRoute resources in this pull request (cozystack-api/templates/api-tlsroute.yaml, kubevirt-cdi/templates/cdi-uploadproxy-tlsroute.yaml, and kubevirt/templates/vm-exportproxy-tlsroute.yaml).
apiVersion: gateway.networking.k8s.io/v1| {{- $gateway := $parentNamespace.gateway | default "" }} | ||
| {{- if .Values.gateway }} | ||
| {{- $gateway = $tenantName }} | ||
| {{- end }} |
There was a problem hiding this comment.
This logic for determining the service reference is repeated for etcd, ingress, monitoring, and seaweedfs. To improve maintainability and reduce code duplication, consider creating a shared helper template. For example, you could define a helper in a common _helpers.tpl file:
{{- define "tenant.serviceRef" -}}
{{- $serviceName := .serviceName -}}
{{- $ref := index .parentNamespace $serviceName | default "" -}}
{{- if index .values $serviceName -}}
{{- $ref = .tenantName -}}
{{- end -}}
{{- $ref -}}
{{- end -}}
And then use it like this for each service:
{{- $gateway := include "tenant.serviceRef" (dict "serviceName" "gateway" "tenantName" $tenantName "parentNamespace" $parentNamespace "values" .Values) -}}
This would make the template cleaner and easier to manage.
| {{- if has "dashboard" $exposeServices }} | ||
| - name: https-dashboard | ||
| protocol: HTTPS | ||
| port: 443 | ||
| hostname: {{ index $hostnames "dashboard" | default (printf "dashboard.%s" $rootHost) | quote }} | ||
| tls: | ||
| mode: Terminate | ||
| certificateRefs: | ||
| - name: gateway-dashboard-tls | ||
| allowedRoutes: | ||
| namespaces: | ||
| from: All | ||
| {{- end }} | ||
| {{- if eq $oidcEnabled "true" }} | ||
| - name: https-keycloak | ||
| protocol: HTTPS | ||
| port: 443 | ||
| hostname: {{ index $hostnames "keycloak" | default (printf "keycloak.%s" $rootHost) | quote }} | ||
| tls: | ||
| mode: Terminate | ||
| certificateRefs: | ||
| - name: gateway-keycloak-tls | ||
| allowedRoutes: | ||
| namespaces: | ||
| from: All | ||
| {{- end }} | ||
| {{- if has "api" $exposeServices }} | ||
| - name: tls-passthrough-api | ||
| protocol: TLS | ||
| port: 443 | ||
| hostname: {{ index $hostnames "api" | default (printf "api.%s" $rootHost) | quote }} | ||
| tls: | ||
| mode: Passthrough | ||
| allowedRoutes: | ||
| namespaces: | ||
| from: All | ||
| {{- end }} | ||
| {{- if has "vm-exportproxy" $exposeServices }} | ||
| - name: tls-passthrough-vm-exportproxy | ||
| protocol: TLS | ||
| port: 443 | ||
| hostname: {{ index $hostnames "vm-exportproxy" | default (printf "vm-exportproxy.%s" $rootHost) | quote }} | ||
| tls: | ||
| mode: Passthrough | ||
| allowedRoutes: | ||
| namespaces: | ||
| from: All | ||
| {{- end }} | ||
| {{- if has "cdi-uploadproxy" $exposeServices }} | ||
| - name: tls-passthrough-cdi-uploadproxy | ||
| protocol: TLS | ||
| port: 443 | ||
| hostname: {{ index $hostnames "cdi-uploadproxy" | default (printf "cdi-uploadproxy.%s" $rootHost) | quote }} | ||
| tls: | ||
| mode: Passthrough | ||
| allowedRoutes: | ||
| namespaces: | ||
| from: All | ||
| {{- end }} |
There was a problem hiding this comment.
The listener definitions for different services are very repetitive. To improve maintainability, you could refactor this by defining a list or map of services and their properties, and then looping through them to generate the listener configurations. This would reduce code duplication and make it easier to add or modify services in the future.
sircthulhu
force-pushed
the
feat/gateway-api-httproutes
branch
from
3b366e2 to
8c3ad83
Compare
- Upgrade Gateway API CRDs from v1.2.0 to v1.5.0 (experimental channel) - Add gateway-api-crds as a component in all networking variants, with Cilium depending on it to ensure CRDs are available first - Add gateway.gatewayAPI platform config (enables Cilium gatewayAPI.enabled=true), can be used alongside gateway.ingress - Enable Gateway API support in cert-manager Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
When gateway.gatewayAPI is true, pass gateway-httproute source to the system external-dns package via components values override. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
…ayAPI Previously enableGatewayAPI was hardcoded to true in cert-manager values, causing startup failures in isp-hosted variant where Gateway API CRDs are not installed. Now passed via component override only when gateway.gatewayAPI is enabled. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Add cozystack-gateway system package that creates a shared Gateway resource with per-hostname HTTPS listeners and TLS passthrough listeners for system services. Listeners are dynamically generated from expose-services config. Platform changes: - Add gateway-api flag to _cluster config for component awareness - Add gateway.name/namespace to platform values for parentRef config - Add publishing.hostnames map for per-service hostname overrides - Wire cozystack-gateway package into system bundle (conditional) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Add Gateway API route templates alongside existing Ingress templates
for all system-level services. Routes are conditional on
_cluster.gateway-api being enabled.
HTTPRoute (TLS terminate via central Gateway):
- dashboard: routes to incloud-web-gatekeeper:8000
- keycloak: routes to keycloak-http:80
TLSRoute (TLS passthrough via central Gateway):
- cozystack-api: passthrough to kubernetes:443
- kubevirt vm-exportproxy: passthrough to vm-exportproxy:443
- kubevirt-cdi uploadproxy: passthrough to cdi-uploadproxy:443
All routes reference the central Gateway via parentRefs with
sectionName for listener matching. Hostname resolution follows
convention ({service}.{root-host}) with override support via
_cluster.hostnames map.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Add gateway field to tenant namespace configuration, following the same inheritance pattern as ingress: - tenant-root defaults to gateway: tenant-root - Sub-tenants with gateway: true get their own LB - Sub-tenants without gateway inherit from parent Add infrastructure.labels to central Gateway for shared LB with tenant-root services (cozystack.io/gateway: tenant-root). Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Add per-component Gateway + HTTPRoute/TLSRoute for tenant-level
services. Each component creates its own Gateway with specific
hostname HTTPS listener and cert-manager annotation for automatic
certificate provisioning via HTTP01 or DNS01.
All Gateways within a tenant share a single LoadBalancer IP through
infrastructure.labels (cozystack.io/gateway: {tenant-name}), matching
the current ingress-nginx model of one LB per tenant.
HTTPRoute (per-component Gateway with TLS terminate):
- monitoring/grafana: routes to grafana-service:3000
- monitoring/alerta: routes to alerta:80
- bucket: routes to {bucketName}-ui:8080
- apps/harbor: routes to {release}:80
- extra/bootbox: routes to bootbox:8080
TLSRoute (per-component Gateway with TLS passthrough):
- extra/seaweedfs: passthrough to filer-external:18888
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
- Add HTTP-to-HTTPS redirect HTTPRoute for all Gateways (central and per-tenant) to ensure plain HTTP requests are properly redirected - Add oidc-enabled check to keycloak HTTPRoute condition to match the Gateway listener condition - Use gateway-name from cluster config instead of hardcoded value in central Gateway template - Remove redundant namespace from TLSRoute metadata where it matches the HelmRelease install namespace (kubevirt, kubevirt-cdi) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
v1alpha2 is deprecated in Gateway API v1.5.0 with a removal warning. Update all TLSRoute resources to use gateway.networking.k8s.io/v1. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Replace hardcoded `gatewayClassName: cilium` in all 7 Gateway templates with a configurable value from _cluster.gateway-class-name. Default remains "cilium" for backward compatibility. This enables custom GatewayClass resources with different parameters (e.g. NodePort via CiliumGatewayClassConfig) and maintains vendor-neutrality for non-Cilium Gateway API controllers. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Add cozy-lib.gateway.httpRoute and cozy-lib.gateway.tlsRoute helpers that encapsulate the full Gateway + HTTPRoute/TLSRoute pattern with HTTP-to-HTTPS redirect. This eliminates ~350 lines of duplicated boilerplate across 6 tenant-level components. Add cozy-lib as dependency to bucket, monitoring, harbor, bootbox, seaweedfs, and cozystack-gateway charts. Update PackageSource definitions for cozystack-gateway and bucket-system to include cozy-lib library. Add ns-gateway helper to _cozyconfig.tpl for consistency with existing ns-ingress, ns-etcd, etc. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
When gateway.gatewayAPI is enabled in platform config: - Set Cilium envoy.streamIdleTimeoutDurationSeconds to 86400 (24h) to support long-lived WebSocket (dashboard/noVNC) and large file uploads (bucket, harbor, seaweedfs). Cilium does not support per-route timeouts (GEP-1742), so a global override is needed. - Enable gatewayAPI.enableAlpn for HTTP/2 and gRPC support through Gateway API listeners (required by seaweedfs filer gRPC backend). - Disable Keycloak sticky session route attachment (KC_SPI_STICKY_SESSION_ENCODER_INFINISPAN_SHOULD_ATTACH_ROUTE=false) so Keycloak works without cookie-based session affinity. Infinispan distributed cache handles session replication across nodes. This removes the dependency on nginx cookie affinity annotations. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Match the existing symlink convention used by other charts (e.g. keycloak, tenant) where the cozy-lib symlink target includes a trailing slash: ../../../library/cozy-lib/ Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Revert cozy-lib refactoring to debug platform upgrade issue. Gateway templates restored to inline form. cozy-lib dependency removed from bucket, cozystack-gateway, monitoring, harbor, bootbox, seaweedfs charts. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
…kage
cozystack.platform.package renders `components: {}` when given an
empty dict, which causes the operator post-render to fail with YAML
unmarshal errors. Use package.default (no components) when the dict
is empty, package (with components) only when non-empty.
Fixes cert-manager and external-dns conditional components blocks
that pass an empty dict when gateway.gatewayAPI is false.
Assisted-By: Claude AI
Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
…ingress disabled When ingress is disabled and Gateway API is used, create a CiliumLoadBalancerIPPool from publishing.externalIPs so that Cilium assigns those IPs to the Gateway's LoadBalancer Service. Priority logic: ingress takes externalIPs when enabled; Gateway gets them only when ingress is off. Propagate gateway.ingress as ingress-enabled in _cluster values so the gateway chart can make this decision. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Cover central Gateway, system HTTPRoutes/TLSRoutes, HTTP-to-HTTPS redirect, TLS passthrough to API server, per-tenant Grafana Gateway, and ingress coexistence verification. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
sircthulhu
force-pushed
the
feat/gateway-api-httproutes
branch
from
17f38a2 to
d0cc82b
Compare
sircthulhu
requested review from
IvanHunters,
androndo and
lexfrei
as code owners
dosubot
bot
added
size:XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
packages/system/cozystack-gateway/Chart.yaml (1)
1-3: Consider adding a description field.While not required, adding a
descriptionfield would improve discoverability and help users understand the chart's purpose.📝 Suggested enhancement
apiVersion: v2 name: cozy-cozystack-gateway +description: Central Gateway API Gateway for system-level HTTP and TLS routing version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cozystack-gateway/Chart.yaml` around lines 1 - 3, Add a human-readable description field to Chart.yaml to improve discoverability and explain the chart's purpose; in the Chart.yaml near the existing apiVersion/name/version entries, add a top-level description: "<brief one-line summary of cozy-cozystack-gateway's purpose>" (use clear, concise wording) so consumers see what cozy-cozystack-gateway does when viewing the chart metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/e2e-install-cozystack.bats`:
- Around line 259-267: The curl probes use the IP in the URL so TLS SNI is sent
as the IP (causing hostname-based routing/cert selection to fail); update the
curl invocations that use "https://${gateway_ip}" (the blocks that set http_code
and check Dashboard and Kubernetes API responses) to instead request the
hostname in the URL (e.g., https://dashboard.example.org and
https://api.example.org) and add curl's --resolve option to map each hostname to
${gateway_ip} so the hostname is preserved for SNI while resolving to the
gateway IP; modify both the Dashboard probe (where http_code is set for
dashboard.example.org) and the Kubernetes API probe (and the similar block
around the second occurrence noted) to use --resolve with the appropriate
hostname and ${gateway_ip}.
In `@packages/core/platform/templates/bundles/system.yaml`:
- Around line 173-186: Update the v1.0.0 migration/upgrade notes to document
that external-dns is now opt-in by requiring cozystack.external-dns to be added
to bundles.enabledPackages (which defaults to an empty list), and add a short
operator guidance block instructing anyone upgrading from v0.x to check if they
depend on external-dns and, if so, explicitly add "cozystack.external-dns" to
bundles.enabledPackages in their Platform values before upgrading.
In `@packages/extra/bootbox/templates/matchbox/httproute.yaml`:
- Around line 74-76: The HTTPRoute backendRefs currently references name:
bootbox with port: 8080 which does not match the Service bootbox exposed port
80; update the HTTPRoute backendRefs entry (backendRefs / name: bootbox / port)
in matchbox/httproute.yaml to use port: 80 so it matches the Service definition
in packages/extra/bootbox/templates/matchbox/service.yaml and ensures proper
route resolution.
In `@packages/extra/seaweedfs/templates/tlsroute.yaml`:
- Line 7: The top render condition currently requires (.Values.filer.grpcHost)
which prevents the template from rendering when grpcHost is empty but a fallback
host exists; update the initial if-condition (the line with and (eq $gatewayAPI
"true") (ne $gateway "") (not (eq .Values.topology "Client"))
(.Values.filer.grpcHost)) to instead allow either .Values.filer.grpcHost or the
fallback host (e.g., check for (.Values.filer.grpcHost) OR (. _namespace.host) /
the same host variable used in the fallback at the usages around the
filer.<host> logic on lines referencing the fallback) so the block renders when
a fallback host is available.
- Line 1: Normalize the gateway-api check by converting the value to a string
before comparing and ensure the grpcHost fallback is considered in the
conditional: change the template to set $gatewayAPI := printf "%v" (index
.Values._cluster "gateway-api" | default false) and then compute a $grpcHost
variable using default .Values.filer.grpcHost with the same fallback expression
used later in the template (the explicit fallback hostnames present where
grpcHost is used), and use $grpcHost in the if-condition that currently compares
$gatewayAPI to "true" so boolean/unquoted values and the fallback host are both
respected.
In `@packages/system/cozystack-gateway/templates/gateway.yaml`:
- Around line 111-113: The template loop that emits "cidr: {{ . }}/32" from "{{-
range splitList \",\" $exposeExternalIPs }}" can produce "cidr: /32" for empty
items; update the range to skip empty/whitespace entries by checking the trimmed
value before emitting. Specifically, inside the range over splitList ","
$exposeExternalIPs, wrap the cidr block in a conditional like "if (ne (trim .)
\"\")" (or equivalent trim+not-empty check) so only non-empty values produce
"cidr: {{ . }}/32".
In `@packages/system/gateway-api-crds/Makefile`:
- Line 9: The Makefile now kustomizes Gateway API CRDs to v1.5.0 (the kubectl
kustomize
"github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v1.5.0"
line) but v1.5.0 has breaking changes (TLSRoute and ListenerSet moved from
Experimental->Standard, new validating admission policy
safe-upgrades.gateway.networking.k8s.io, and TLSRoute CEL requiring Kubernetes
>=1.31); before changing the ref, either keep the existing v1.2.0 CRD ref or
update other CRD targets so Standard and Experimental CRDs are applied together,
scan the cluster for resources using TLSRoute v1alpha2 and migrate them to the
new v1 types (or plan removal), ensure target clusters run Kubernetes >=1.31 if
using TLSRoute CEL, and apply CRDs with kubectl apply --server-side=true to
avoid VAP conflicts; also remove or adjust generation of
templates/crds-experimental.yaml if you intend to fully migrate to the Standard
channel so you don't install mixed channels separately which will be blocked by
the new VAP.
In `@packages/system/keycloak/templates/httproute.yaml`:
- Around line 27-29: The HTTPRoute's backendRefs currently points to port 80
which does not exist on the keycloak-http Service (service.yaml exposes port
8080); update the HTTPRoute backendRefs entry for name: keycloak-http to use
port: 8080 (replace the existing port: 80) so the route matches the Service's
exposed port and routing will succeed.
---
Nitpick comments:
In `@packages/system/cozystack-gateway/Chart.yaml`:
- Around line 1-3: Add a human-readable description field to Chart.yaml to
improve discoverability and explain the chart's purpose; in the Chart.yaml near
the existing apiVersion/name/version entries, add a top-level description:
"<brief one-line summary of cozy-cozystack-gateway's purpose>" (use clear,
concise wording) so consumers see what cozy-cozystack-gateway does when viewing
the chart metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3b12f3a-79b0-42fa-9d84-ffce62178120
📒 Files selected for processing (30)
hack/e2e-install-cozystack.batspackages/apps/harbor/templates/httproute.yamlpackages/apps/tenant/README.mdpackages/apps/tenant/templates/namespace.yamlpackages/apps/tenant/values.schema.jsonpackages/apps/tenant/values.yamlpackages/core/platform/sources/cozystack-gateway.yamlpackages/core/platform/sources/networking.yamlpackages/core/platform/templates/apps.yamlpackages/core/platform/templates/bundles/system.yamlpackages/core/platform/values.yamlpackages/extra/bootbox/templates/matchbox/httproute.yamlpackages/extra/seaweedfs/templates/tlsroute.yamlpackages/system/bucket/templates/httproute.yamlpackages/system/cert-manager/values.yamlpackages/system/cozystack-api/templates/api-tlsroute.yamlpackages/system/cozystack-basics/templates/cozystack-values-secret.yamlpackages/system/cozystack-gateway/Chart.yamlpackages/system/cozystack-gateway/templates/gateway.yamlpackages/system/cozystack-gateway/values.yamlpackages/system/dashboard/templates/httproute.yamlpackages/system/gateway-api-crds/Makefilepackages/system/gateway-api-crds/templates/crds-experimental.yamlpackages/system/keycloak/templates/httproute.yamlpackages/system/keycloak/templates/sts.yamlpackages/system/kubevirt-cdi/templates/cdi-uploadproxy-tlsroute.yamlpackages/system/kubevirt/templates/vm-exportproxy-tlsroute.yamlpackages/system/monitoring/templates/alerta/httproute.yamlpackages/system/monitoring/templates/grafana/httproute.yamlpackages/system/tenant-rd/cozyrds/tenant.yaml
| {{- $externalDnsComponents := dict -}} | ||
| {{- if .Values.gateway.gatewayAPI -}} | ||
| {{- $externalDnsValues := dict "external-dns" (dict "sources" (list "service" "ingress" "gateway-httproute")) -}} | ||
| {{- $_ := set $externalDnsComponents "external-dns" (dict "values" $externalDnsValues) -}} | ||
| {{- end -}} | ||
| {{- $disabled := default (list) .Values.bundles.disabledPackages -}} | ||
| {{- $enabled := default (list) .Values.bundles.enabledPackages -}} | ||
| {{- if and (has "cozystack.external-dns" $enabled) (not (has "cozystack.external-dns" $disabled)) -}} | ||
| {{- if gt (len $externalDnsComponents) 0 }} | ||
| {{include "cozystack.platform.package" (list "cozystack.external-dns" "default" $ $externalDnsComponents) }} | ||
| {{- else }} | ||
| {{include "cozystack.platform.package.default" (list "cozystack.external-dns" $) }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if external-dns enablement change is documented in release notes or migration docs
# Search for documentation about this change
rg -i "external-dns" -g "*.md" -g "CHANGELOG*" -g "RELEASE*" -C 3Repository: cozystack/cozystack
Length of output: 50377
🏁 Script executed:
# Check the platform values.yaml file around lines 11-22
head -30 packages/core/platform/values.yaml | cat -nRepository: cozystack/cozystack
Length of output: 1068
🏁 Script executed:
# Search for migration documentation at repo root
find . -maxdepth 2 -type f \( -name "*MIGRATION*" -o -name "*CHANGELOG*" -o -name "*RELEASE*" \) | head -20Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check if there's any documentation about the bundles.enabledPackages change
rg "enabledPackages" -A 2 -B 2 --max-count 20Repository: cozystack/cozystack
Length of output: 3756
🏁 Script executed:
# Search for CHANGELOG, MIGRATION, or release documentation at root level
find . -maxdepth 1 -type f \( -iname "*changelog*" -o -iname "*migration*" -o -iname "*release*" -o -iname "*NEWS*" \)Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check the migration script to understand the enabledPackages context
cat hack/migrate-to-version-1.0.sh | head -100Repository: cozystack/cozystack
Length of output: 4438
🏁 Script executed:
# Look for any documentation about bundles and enabledPackages in the templates
rg "Optional System Packages|enabledPackages" packages/core/platform/templates/bundles/system.yaml -B 5 -A 5Repository: cozystack/cozystack
Length of output: 2147
🏁 Script executed:
# Check if there's a README in packages/core/platform explaining the bundles
head -100 packages/core/platform/README.md 2>/dev/null || echo "No README found"Repository: cozystack/cozystack
Length of output: 78
🏁 Script executed:
# Search for migration documentation in the repository
find . -type f \( -name "*MIGRATION*" -o -name "*UPGRADE*" -o -name "*v1.0*" -o -name "*release*" \) | grep -iE "(md|txt|rst)" | head -20Repository: cozystack/cozystack
Length of output: 526
🏁 Script executed:
# Check if there's documentation about the v1.0 migration or upgrade path
rg -i "migration|upgrade|breaking" --type md | head -30Repository: cozystack/cozystack
Length of output: 5099
🏁 Script executed:
# Look for any documentation that mentions external-dns being optional or breaking change
rg "external-dns" --type md -B 3 -A 3 | head -50Repository: cozystack/cozystack
Length of output: 5408
🏁 Script executed:
# Check what the template_helpers says about optional packages
rg "cozystack.platform.package.optional" packages/core/platform/templates/_helpers.tpl -B 5 -A 10Repository: cozystack/cozystack
Length of output: 1333
🏁 Script executed:
# Check the v1.0.0 changelog to see if external-dns being optional is documented
cat docs/changelogs/v1.0.0.md | head -200Repository: cozystack/cozystack
Length of output: 29028
🏁 Script executed:
# Search for external-dns in all changelog files
rg "external-dns" docs/changelogs/ -B 2 -A 2Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check if there's a breaking changes section in the current or recent changelogs
rg -i "breaking|breaking change" docs/changelogs/ -B 2 -A 2Repository: cozystack/cozystack
Length of output: 16771
Document external-dns enablement requirement in migration guide.
External-dns is now optional and only deployed when explicitly added to bundles.enabledPackages. Since enabledPackages defaults to an empty list (per packages/core/platform/values.yaml:22), existing deployments upgrading from v0.x will lose external-dns unless they explicitly enable it. This behavioral change is not documented in the v1.0.0 "Breaking Changes & Upgrade Notes" section despite being a breaking change for existing clusters.
Add guidance to the migration notes instructing operators upgrading from v0.x to check if external-dns is needed and explicitly add cozystack.external-dns to bundles.enabledPackages in their Platform values if they rely on it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/platform/templates/bundles/system.yaml` around lines 173 - 186,
Update the v1.0.0 migration/upgrade notes to document that external-dns is now
opt-in by requiring cozystack.external-dns to be added to
bundles.enabledPackages (which defaults to an empty list), and add a short
operator guidance block instructing anyone upgrading from v0.x to check if they
depend on external-dns and, if so, explicitly add "cozystack.external-dns" to
bundles.enabledPackages in their Platform values before upgrading.
| backendRefs: | ||
| - name: bootbox | ||
| port: 8080 |
There was a problem hiding this comment.
Fix backendRef port to match the Service’s exposed port.
Line 76 uses port: 8080, but packages/extra/bootbox/templates/matchbox/service.yaml exposes Service bootbox on port: 80 (Line 10). HTTPRoute.backendRefs.port must match the Service port, so this can cause route resolution/routing failure.
🔧 Proposed fix
- name: bootbox
- port: 8080
+ port: 80📝 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.
| backendRefs: | |
| - name: bootbox | |
| port: 8080 | |
| backendRefs: | |
| - name: bootbox | |
| port: 80 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/extra/bootbox/templates/matchbox/httproute.yaml` around lines 74 -
76, The HTTPRoute backendRefs currently references name: bootbox with port: 8080
which does not match the Service bootbox exposed port 80; update the HTTPRoute
backendRefs entry (backendRefs / name: bootbox / port) in
matchbox/httproute.yaml to use port: 80 so it matches the Service definition in
packages/extra/bootbox/templates/matchbox/service.yaml and ensures proper route
resolution.
| @@ -0,0 +1,43 @@ | |||
| {{- $gatewayAPI := (index .Values._cluster "gateway-api") | default "false" }} | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find gateway-api definitions and example values =="
rg -n -C2 'gateway-api' -g '*.yaml' -g '*.yml' -g '*.json'
echo
echo "== Find how _cluster.gateway-api is consumed in templates =="
rg -n -C2 '_cluster|gateway-api|eq \$gatewayAPI "true"' -g '*.yaml' -g '*.tpl'Repository: cozystack/cozystack
Length of output: 50376
🏁 Script executed:
cat -n packages/extra/seaweedfs/templates/tlsroute.yamlRepository: cozystack/cozystack
Length of output: 1582
🏁 Script executed:
# Find values files where gateway-api is configured
find . -name "values*.yaml" -o -name "values*.yml" | head -30 | xargs grep -l "gateway-api" 2>/dev/null | head -10
# Then examine how the value is set (boolean vs string)
echo "=== Checking how gateway-api is provided in values ==="
find . -name "values*.yaml" -o -name "values*.yml" | xargs grep -A2 -B2 "gateway-api:" 2>/dev/null | head -50Repository: cozystack/cozystack
Length of output: 117
🏁 Script executed:
# Search for where gateway.gatewayAPI is defined
echo "=== Looking for gateway.gatewayAPI definition ==="
rg -n "gateway\.gatewayAPI|gatewayAPI:" -A2 -B2 | head -60
# Also search for how _cluster values are passed/injected
echo ""
echo "=== Looking for _cluster value injection ==="
rg -n "\"_cluster\"|_cluster:" -l | head -10
# Check the platform values to understand gateway configuration
echo ""
echo "=== Platform gateway configuration ==="
find . -path "*/platform/*" -name "values*.yaml" | xargs cat 2>/dev/null | grep -A 10 "gateway:" | head -30Repository: cozystack/cozystack
Length of output: 5718
🏁 Script executed:
# Check if other templates have similar grpcHost gates
echo "=== Checking for grpcHost usage in other templates ==="
rg -n "grpcHost" -B2 -A2
# Also check other TLSRoute files to compare their gate conditions
echo ""
echo "=== Comparing gate conditions in similar TLSRoute templates ==="
fd "tlsroute.yaml|httproute.yaml" -x head -10 {} +Repository: cozystack/cozystack
Length of output: 51185
🏁 Script executed:
# Check if seaweedfs ingress.yaml has the same grpcHost gate
echo "=== Checking seaweedfs ingress.yaml for grpcHost gate ==="
sed -n '1,20p' packages/extra/seaweedfs/templates/ingress.yaml
echo ""
echo "=== Checking if other packages with external endpoints use similar gates ==="
rg "grpcHost" packages/extra/seaweedfs/templates/Repository: cozystack/cozystack
Length of output: 1998
Normalize gateway-api type before comparison and make the grpcHost fallback actually reachable.
Line 7 compares $gatewayAPI to the string "true". If _cluster.gateway-api is ever passed as a boolean (not quoted), this comparison silently fails and resources don't render. Additionally, the same line requires (.Values.filer.grpcHost) to be truthy, but lines 22 and 38 provide a fallback hostname—making the gate inconsistent with the template's intent.
🔧 Proposed fix
-{{- $gatewayAPI := (index .Values._cluster "gateway-api") | default "false" }}
+{{- $gatewayAPI := (index .Values._cluster "gateway-api") | default false | toString | lower }}
{{- $gateway := .Values._namespace.gateway | default "" }}
{{- $host := .Values._namespace.host }}
{{- $gatewayClassName := (index .Values._cluster "gateway-class-name") | default "cilium" }}
-{{- if and (eq $gatewayAPI "true") (ne $gateway "") (not (eq .Values.topology "Client")) (.Values.filer.grpcHost) }}
+{{- if and (eq $gatewayAPI "true") (ne $gateway "") (not (eq .Values.topology "Client")) (or .Values.filer.grpcHost $host) }}📝 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.
| {{- $gatewayAPI := (index .Values._cluster "gateway-api") | default "false" }} | |
| {{- $gatewayAPI := (index .Values._cluster "gateway-api") | default false | toString | lower }} |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/extra/seaweedfs/templates/tlsroute.yaml` at line 1, Normalize the
gateway-api check by converting the value to a string before comparing and
ensure the grpcHost fallback is considered in the conditional: change the
template to set $gatewayAPI := printf "%v" (index .Values._cluster "gateway-api"
| default false) and then compute a $grpcHost variable using default
.Values.filer.grpcHost with the same fallback expression used later in the
template (the explicit fallback hostnames present where grpcHost is used), and
use $grpcHost in the if-condition that currently compares $gatewayAPI to "true"
so boolean/unquoted values and the fallback host are both respected.
|
|
||
| {{- $gatewayClassName := (index .Values._cluster "gateway-class-name") | default "cilium" }} | ||
|
|
||
| {{- if and (eq $gatewayAPI "true") (ne $gateway "") (not (eq .Values.topology "Client")) (.Values.filer.grpcHost) }} |
There was a problem hiding this comment.
Render gate blocks the documented/default hostname path.
Line 7 requires .Values.filer.grpcHost, but Lines 22 and 38 already support fallback to filer.<host>. With grpcHost empty and _namespace.host set, this template won’t render at all.
🔧 Proposed fix
{{- $gatewayClassName := (index .Values._cluster "gateway-class-name") | default "cilium" }}
+{{- $filerHost := .Values.filer.grpcHost | default (printf "filer.%s" $host) }}
-{{- if and (eq $gatewayAPI "true") (ne $gateway "") (not (eq .Values.topology "Client")) (.Values.filer.grpcHost) }}
+{{- if and (eq $gatewayAPI "true") (ne $gateway "") (not (eq .Values.topology "Client")) (or .Values.filer.grpcHost $host) }}
@@
- hostname: {{ .Values.filer.grpcHost | default (printf "filer.%s" $host) | quote }}
+ hostname: {{ $filerHost | quote }}
@@
- - {{ .Values.filer.grpcHost | default (printf "filer.%s" $host) | quote }}
+ - {{ $filerHost | quote }}Also applies to: 22-22, 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/extra/seaweedfs/templates/tlsroute.yaml` at line 7, The top render
condition currently requires (.Values.filer.grpcHost) which prevents the
template from rendering when grpcHost is empty but a fallback host exists;
update the initial if-condition (the line with and (eq $gatewayAPI "true") (ne
$gateway "") (not (eq .Values.topology "Client")) (.Values.filer.grpcHost)) to
instead allow either .Values.filer.grpcHost or the fallback host (e.g., check
for (.Values.filer.grpcHost) OR (. _namespace.host) / the same host variable
used in the fallback at the usages around the filer.<host> logic on lines
referencing the fallback) so the block renders when a fallback host is
available.
| {{- range splitList "," $exposeExternalIPs }} | ||
| - cidr: {{ . }}/32 | ||
| {{- end }} |
There was a problem hiding this comment.
Guard against empty IP entries when generating CIDRs.
If expose-external-ips has a trailing comma, this renders an invalid block (cidr: /32). Please skip empty items before emitting blocks.
🔧 Proposed fix
- {{- range splitList "," $exposeExternalIPs }}
- - cidr: {{ . }}/32
- {{- end }}
+ {{- range $ip := splitList "," $exposeExternalIPs }}
+ {{- if $ip }}
+ - cidr: {{ $ip }}/32
+ {{- end }}
+ {{- 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.
| {{- range splitList "," $exposeExternalIPs }} | |
| - cidr: {{ . }}/32 | |
| {{- end }} | |
| {{- range $ip := splitList "," $exposeExternalIPs }} | |
| {{- if $ip }} | |
| - cidr: {{ $ip }}/32 | |
| {{- end }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/cozystack-gateway/templates/gateway.yaml` around lines 111 -
113, The template loop that emits "cidr: {{ . }}/32" from "{{- range splitList
\",\" $exposeExternalIPs }}" can produce "cidr: /32" for empty items; update the
range to skip empty/whitespace entries by checking the trimmed value before
emitting. Specifically, inside the range over splitList "," $exposeExternalIPs,
wrap the cidr block in a conditional like "if (ne (trim .) \"\")" (or equivalent
trim+not-empty check) so only non-empty values produce "cidr: {{ . }}/32".
| rm -rf templates | ||
| mkdir templates | ||
| kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v1.2.0" > templates/crds-experimental.yaml | ||
| kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v1.5.0" > templates/crds-experimental.yaml |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of Kubernetes Gateway API and when was v1.5.0 released?
💡 Result:
The latest version of Kubernetes Gateway API is v1.5.1, released on March 14, 2026. The v1.5.0 version was released on February 26, 2026.
Citations:
- 1: https://github.com/kubernetes-sigs/gateway-api/releases
- 2: https://newreleases.io/project/github/kubernetes-sigs/gateway-api/release/v1.5.0
- 3: https://pkg.go.dev/sigs.k8s.io/gateway-api@v1.5.0
🌐 Web query:
Kubernetes Gateway API v1.5.0 release notes breaking changes changelog
💡 Result:
Kubernetes Gateway API v1.5.0 was released around February 26-27, 2026. Official release notes are available at https://github.com/kubernetes-sigs/gateway-api/releases/tag/v1.5.0. ## Breaking Changes 1. TLSRoute v1alpha2 and XListenerSet removal from Experimental channel: TLSRoute and ListenerSet have graduated to the Standard channel as v1. TLSRoute v1alpha2 is only in Experimental channel in v1.5.0 and will be removed in v1.6. Additionally, TLSRoute's CEL validation requires Kubernetes 1.31+. 2. New Validating Admission Policy (VAP): Introduces safe-upgrades.gateway.networking.k8s.io VAP to prevent: - Downgrading to versions prior to 1.5 after installing v1.5. - Installing Experimental CRDs after Standard CRDs. Workaround: Delete the VAP if needed for specific operations. ## Major Changes Since v1.4.1 (Standard Channel Graduations) - TLSRoute v1 (GEP-2643) - HTTPRoute CORS filter (GEP-1767) - ListenerSet support (GEP-1713) - Certificate selection for Gateway TLS origination (GEP-3155) - ReferenceGrant to v1 ## Experimental - Gateway/HTTPRoute level authentication (GEP-1494) Full changelog: kubernetes-sigs/gateway-api@v1.4.1...v1.5.0 Note: Use server-side apply for installation due to large CRD sizes: kubectl apply --server-side=true -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.5.0/[standard|experimental]-install.yaml. A patch release v1.5.1 followed with minor fixes.
Citations:
- 1: https://github.com/kubernetes-sigs/gateway-api/releases
- 2: https://newreleases.io/project/github/kubernetes-sigs/gateway-api/release/v1.5.0
Address breaking changes in Gateway API v1.5.0 before upgrading.
This updates Gateway API CRDs from v1.2.0 to v1.5.0 (released Feb 26, 2026). While the version is valid, this release includes breaking changes that require attention:
- TLSRoute and ListenerSet: Both graduated from Experimental to Standard channel as v1. TLSRoute v1alpha2 is deprecated and will be removed in v1.6.
- New Validating Admission Policy: A VAP (
safe-upgrades.gateway.networking.k8s.io) now prevents downgrading to pre-1.5 versions and mixing Experimental/Standard CRDs installed separately. - Kubernetes 1.31+ requirement: TLSRoute CEL validation requires Kubernetes 1.31 or later.
Consider whether existing resources depend on the experimental v1alpha2 types and verify cluster compatibility before applying. Use kubectl apply --server-side=true due to large CRD sizes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/gateway-api-crds/Makefile` at line 9, The Makefile now
kustomizes Gateway API CRDs to v1.5.0 (the kubectl kustomize
"github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v1.5.0"
line) but v1.5.0 has breaking changes (TLSRoute and ListenerSet moved from
Experimental->Standard, new validating admission policy
safe-upgrades.gateway.networking.k8s.io, and TLSRoute CEL requiring Kubernetes
>=1.31); before changing the ref, either keep the existing v1.2.0 CRD ref or
update other CRD targets so Standard and Experimental CRDs are applied together,
scan the cluster for resources using TLSRoute v1alpha2 and migrate them to the
new v1 types (or plan removal), ensure target clusters run Kubernetes >=1.31 if
using TLSRoute CEL, and apply CRDs with kubectl apply --server-side=true to
avoid VAP conflicts; also remove or adjust generation of
templates/crds-experimental.yaml if you intend to fully migrate to the Standard
channel so you don't install mixed channels separately which will be blocked by
the new VAP.
Add Envoy Gateway as the Gateway API controller with mergeGateways support. Per-tenant gateway model mirrors ingress-nginx architecture. Per-tenant gateway model: - gateway: true in tenant -> GatewayClass + EnvoyProxy per tenant - mergeGateways: true -> all Gateways per tenant merge into one Service - When externalIPs configured: ClusterIP + externalIPs (same as nginx) - When no externalIPs: standard LoadBalancer - gateway: false -> inherit parent's GatewayClass New packages: - packages/system/envoy-gateway/ (Envoy Gateway controller v1.7.1) - packages/extra/gateway/ (per-tenant GatewayClass + EnvoyProxy) - packages/system/gateway-rd/ (ApplicationDefinition for dashboard UI) - packages/core/platform/sources/envoy-gateway.yaml (standalone) - packages/core/platform/sources/gateway-application.yaml (bundled) - packages/apps/tenant/templates/gateway.yaml (tenant HelmRelease) Platform changes: - Add gateway.gatewayClass field for system services fallback - All components use _namespace.gateway as gatewayClassName - System services fall back to _cluster.gateway when _namespace absent - System services create per-component Gateways Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
sircthulhu
force-pushed
the
feat/gateway-api-httproutes
branch
from
2ccb450 to
e4c7658
Compare
- Use --resolve for correct TLS SNI in curl probes - Update central Gateway test to per-tenant Gateway model - Reference per-component Gateway (dashboard) instead of cozystack-gateway - Remove duplicate tenant patch in Grafana test Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
…ozystack-gateway Add install block to envoy-gateway-system component in gateway-application PackageSource so the operator actually creates a HelmRelease for Envoy Gateway. Without this, the EnvoyProxy CRD was never installed, causing tenant gateway HelmReleases to fail with "no matches for kind EnvoyProxy". Remove the unused cozystack-gateway chart and PackageSource since the PR uses per-component Gateway architecture where each service creates its own Gateway resource, making the central Gateway redundant. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
packages/system/gateway-rd/Makefile (1)
4-4: Optional: add local phony passthrough targets to silence checkmake warnings.Line 4 likely pulls required targets from
package.mk, but checkmake can still warn when scanning this file alone. Consider lightweight passthrough targets.Proposed non-functional Makefile tweak
export NAME=gateway-rd export NAMESPACE=cozy-system include ../../../hack/package.mk + +.PHONY: all clean test +all: + @$(MAKE) -f ../../../hack/package.mk all +clean: + @$(MAKE) -f ../../../hack/package.mk clean +test: + @$(MAKE) -f ../../../hack/package.mk test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/gateway-rd/Makefile` at line 4, The Makefile currently only contains the include line (include ../../../hack/package.mk) which causes checkmake to warn when scanning this file alone; add lightweight local phony passthrough targets (e.g., .PHONY plus targets like build, test, lint, install) that simply forward to the same targets provided by the included package.mk so checkmake sees concrete targets. Ensure the passthrough targets are non-functional stubs that invoke the included makefile's targets (using the standard make invocation) and keep them minimal so behavior doesn't change.packages/system/gateway-rd/templates/cozyrd.yaml (1)
1-1: Narrow the glob to manifest files only.
cozyrds/*may include non-YAML artifacts and produce invalid output. Restricting to YAML files makes rendering safer and more predictable.Suggested diff
-{{- range $path, $_ := .Files.Glob "cozyrds/*" }} +{{- range $path, $_ := .Files.Glob "cozyrds/*.yaml" }} --- {{ $.Files.Get $path }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/gateway-rd/templates/cozyrd.yaml` at line 1, The template currently uses .Files.Glob "cozyrds/*" which can pull non-YAML artifacts; narrow the glob to manifest files only by replacing that pattern with YAML-specific globs (e.g., use .Files.Glob "cozyrds/*.yaml" and also handle "cozyrds/*.yml" or iterate over both patterns) in the same template range expression so only .yaml/.yml files are rendered; update the .Files.Glob call(s) in the template to those YAML-only patterns (and add a second loop if needed) to ensure safe, predictable output.packages/extra/gateway/templates/gateway.yaml (1)
13-14: Consider making replica count configurable.The
replicas: 2is hardcoded here, but based on the relevant context,values.yamlprovides empty_clusterand_namespacedictionaries with no documented interface for replica configuration. Consider exposing this as a configurable value for environments with different availability requirements.♻️ Proposed refactor to make replicas configurable
+{{- $replicas := (index .Values._cluster "envoy-replicas") | default 2 }} {{- $exposeGateway := (index .Values._cluster "gateway") | default "" }} {{- $exposeExternalIPs := (index .Values._cluster "expose-external-ips") | default "" | nospace }}Then use
$replicasinstead of the hardcoded value:envoyDeployment: - replicas: 2 + replicas: {{ $replicas }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extra/gateway/templates/gateway.yaml` around lines 13 - 14, The hardcoded replicas: 2 under envoyDeployment should be made configurable via values.yaml; add a new value (e.g., envoyDeployment.replicas or replicaCount) in values.yaml and replace the literal 2 in gateway.yaml with the Helm template variable (reference the envoyDeployment object and its replicas field) so deployments can be adjusted per-environment; ensure you provide a sensible default in values.yaml and update any README or comments to document the new envoyDeployment.replicas option.packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-poddisruptionbudget.yaml (1)
8-8: Redundantandwith single argument.Line 8 uses
{{- if and .Values.podDisruptionBudget.minAvailable }}butandwith a single argument is redundant.♻️ Proposed fix
- {{- if and .Values.podDisruptionBudget.minAvailable }} + {{- if .Values.podDisruptionBudget.minAvailable }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-poddisruptionbudget.yaml` at line 8, The conditional uses a redundant "and" with a single argument; change the template conditional that currently uses "{{- if and .Values.podDisruptionBudget.minAvailable }}" to a simple "{{- if .Values.podDisruptionBudget.minAvailable }}" so the PodDisruptionBudget conditional correctly evaluates the presence/truthiness of .Values.podDisruptionBudget.minAvailable (update the corresponding if/endif block around that symbol).packages/system/monitoring/templates/alerta/httproute.yaml (1)
3-7: Same pattern issues as Grafana template.This template has the same redundant variable pattern and potential nil access as the Grafana HTTPRoute template:
- Lines 3 and 7 both derive gateway values redundantly
- Line 4 accesses
._namespace.hostwithout default fallbackConsider applying consistent fixes across all monitoring HTTPRoute templates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/monitoring/templates/alerta/httproute.yaml` around lines 3 - 7, The template repeats and risks nil access: remove the redundant declaration of $gatewayClassName and consolidate gateway derivation by using a single variable (e.g., $gateway) for both purposes, and add a safe default when reading .Values._namespace.host (use default "" or fall back to .Values.host) before computing $alertaHost; update references to use the single $gateway variable and ensure $host is defined with a default so .Values._namespace.host cannot be nil.packages/system/monitoring/templates/grafana/httproute.yaml (1)
3-7: Redundant variable and potential nil access.
Lines 3 and 7 assign the same value (
._namespace.gateway) to$gatewayand$gatewayClassNameseparately.Line 4 accesses
.Values._namespace.hostwithout a default fallback. If_namespaceis nil, this could cause a template error.Suggested fix
{{- $gateway := .Values._namespace.gateway | default "" }} -{{- $host := .Values._namespace.host }} +{{- $host := (.Values._namespace).host | default "" }} {{- $grafanaHost := printf "grafana.%s" (.Values.host | default $host) }} - -{{- $gatewayClassName := .Values._namespace.gateway | default "" }}Then use
$gatewaydirectly forgatewayClassNameat line 18.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/monitoring/templates/grafana/httproute.yaml` around lines 3 - 7, The template defines duplicate variables $gateway and $gatewayClassName from .Values._namespace.gateway and reads .Values._namespace.host without a safe default; remove the redundancy by using the single $gateway variable wherever gatewayClassName is needed (e.g., replace uses of $gatewayClassName with $gateway) and protect host access by giving .Values._namespace.host a default (e.g., use a default fallback when assigning $host) to avoid nil access in templates such as in the $host and $grafanaHost assignments.packages/system/dashboard/templates/httproute.yaml (1)
6-7: Redundant variable assignment.
$gatewayand$gatewayClassNameare assigned the same value. Consider using a single variable for clarity.Suggested simplification
{{- $gateway := ((.Values._namespace).gateway) | default (index .Values._cluster "gateway") | default "" }} -{{- $gatewayClassName := $gateway }} +{{- /* $gateway is also used as gatewayClassName */ -}}Then use
$gatewaydirectly at line 20.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/dashboard/templates/httproute.yaml` around lines 6 - 7, The template defines two identical variables ($gateway and $gatewayClassName); remove the redundant $gatewayClassName assignment and update any usages of $gatewayClassName to use $gateway instead (e.g., replace references to $gatewayClassName with $gateway and delete the line setting $gatewayClassName).packages/apps/harbor/templates/httproute.yaml (1)
3-7: Redundant variable assignment.Same pattern as other templates:
$gateway(line 3) and$gatewayClassName(line 7) are assigned identical values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/harbor/templates/httproute.yaml` around lines 3 - 7, The template defines two identical variables $gateway and $gatewayClassName causing redundancy; remove one (prefer keeping a single variable name, e.g., $gatewayClassName or $gateway), update all references in the template to use the chosen variable, and delete the other assignment (the duplicate default assignment of .Values._namespace.gateway) so only one defaulting expression remains (ensure functions or blocks that reference $gateway or $gatewayClassName are updated accordingly).packages/system/envoy-gateway/charts/gateway-helm/templates/certgen.yaml (1)
30-44: Args handling is verbose but functional.The conditional logic to append
--disable-topology-injectorwhen topology injector is disabled works correctly. However, the if/else creates duplicatedcommandblocks.Potential simplification
containers: {{- $args := .Values.certgen.job.args }} {{- if not .Values.topologyInjector.enabled }} {{- $args = append $args "--disable-topology-injector" }} {{- end }} - {{- if $args }} - - args: - {{- toYaml $args | nindent 8 }} - command: + - command: - envoy-gateway - certgen - {{- else }} - - command: - - envoy-gateway - - certgen + {{- with $args }} + args: + {{- toYaml . | nindent 8 }} {{- end }}This puts
argsaftercommandwhich is valid YAML and avoids duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/envoy-gateway/charts/gateway-helm/templates/certgen.yaml` around lines 30 - 44, The template duplicates the `command` block because of the if/else around args; keep `command` rendered once and only conditionally render the `args` block when `$args` is non-empty. In the certgen template, compute `$args` (from `.Values.certgen.job.args` and appending `"--disable-topology-injector"` when `.Values.topologyInjector.enabled` is false) as you already do, then unconditionally emit the `- command: - envoy-gateway - certgen` lines and, if `$args` is set, emit the `- args:` block with `{{- toYaml $args | nindent 8 }}`; remove the current if/else that duplicates `command`.packages/system/envoy-gateway/charts/gateway-helm/templates/namespaced-infra-manager-rbac.yaml (1)
1-35: Clarify relationship with infra-manager-rbac.yaml whenwatch.type == "NamespaceSelector".When
watch.type == "NamespaceSelector"is configured alongsidedeploy.type == "GatewayNamespace", both files create tokenreview ClusterRoles:
infra-manager-rbac.yamlcreates*-cluster-infra-manager(includes both basic + tokenreview rules)- This file creates
*-infra-manager-tokenreview(tokenreview only)While functionally harmless (redundant permissions for the same ServiceAccount), this could be confusing when operators audit RBAC rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/envoy-gateway/charts/gateway-helm/templates/namespaced-infra-manager-rbac.yaml` around lines 1 - 35, When watch.type == "NamespaceSelector" and deploy.type == "GatewayNamespace" the template creates a tokenreview ClusterRole '{{ include "eg.fullname" $ }}-infra-manager-tokenreview' that duplicates the tokenreview rules already present in the other ClusterRole ('*-cluster-infra-manager' from infra-manager-rbac.yaml); update this template to skip creating the tokenreview ClusterRole when $kube.watch.type == "NamespaceSelector" (i.e., add a conditional around the ClusterRole/ClusterRoleBinding blocks checking that not (eq $kube.watch.type "NamespaceSelector")), or alternatively add a clear comment in the template explaining why both exist to avoid audit confusion. Ensure you reference $kube (from .Values.config.envoyGateway.provider.kubernetes) and the ClusterRole name '{{ include "eg.fullname" $ }}-infra-manager-tokenreview' when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps/harbor/templates/httproute.yaml`:
- Around line 1-9: The Ingress template is created unconditionally causing
conflicts when Gateway API is enabled; update the ingress template (the file
that currently renders the Ingress resource) to wrap the whole Ingress manifest
with a guard that checks the same $gatewayAPI value used in httproute.yaml (and
optionally $gateway) so the Ingress is skipped when gateway-api is "true" (e.g.,
use an if that evaluates ne $gatewayAPI "true" around the Ingress manifest);
ensure you reference the same .Values._cluster "gateway-api" lookup used to
populate $gatewayAPI so behavior matches the httproute rendering.
In `@packages/system/dashboard/templates/httproute.yaml`:
- Line 11: The conditional at the top of the template only checks eq $gatewayAPI
"true" and has "dashboard" $exposeServices but omits verifying $gateway is
non-empty; update the if clause to also include (ne $gateway "") so it matches
the Grafana/Alerta templates and prevents rendering an empty gatewayClassName
when $gateway is empty (adjust the conditional that currently references
$gatewayAPI, $exposeServices to also check $gateway).
- Around line 1-11: Ingress is still created unconditionally when "dashboard" is
in expose-services, causing conflicts with the Gateway API httproute which uses
$gatewayAPI; update the dashboard Ingress template to skip creating the Ingress
when Gateway API is enabled by wrapping the Ingress resource block with a
conditional that checks both that "dashboard" is in $exposeServices and that
$gatewayAPI is not "true" (use the same $gatewayAPI and $exposeServices symbols
as in httproute.yaml and the has/and/ne helpers to guard the Ingress).
In `@packages/system/envoy-gateway/charts/gateway-helm/templates/_helpers.tpl`:
- Around line 126-133: The current helper uses a hardcoded fallback "master" for
$imageTag which causes non-deterministic upgrades; change the default to a
pinned value coming from chart values (e.g. replace the literal "master" default
with something like .Values.global.images.ratelimit.tag or prefer a digest field
.Values.global.images.ratelimit.digest when present) and update values.yaml to
expose that pinned tag/digest; adjust the logic around
$repositoryParts/$imageTag to prefer the provided digest/tag from
.Values.global.images.ratelimit before falling back to the chart-level default.
- Around line 69-76: The template currently splits
.Values.deployment.envoyGateway.image.repository with splitn and uses
$repositoryParts._1, which drops namespace segments (e.g.,
"envoyproxy/gateway"); change the logic to preserve the full repository path
when .Values.global.imageRegistry is set by assigning $repositoryName to the
entire .Values.deployment.envoyGateway.image.repository (instead of using
splitn/$repositoryParts._1) and then build the final image string with printf
"%s/%s:%s" using $registryName, the full $repositoryName, and $imageTag.
In
`@packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-hpa.yaml`:
- Line 15: The error message in the Helm template's required check for
maxReplicas contains a typo (`.Values.hps.maxReplicas`); update the required
call message to reference the correct path `.Values.hpa.maxReplicas` so the
check on maxReplicas (the line using required and .Values.hpa.maxReplicas)
reports the correct configuration key.
In
`@packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-service.yaml`:
- Around line 1-30: This change edits the vendored upstream Helm chart's Service
template for the "envoy-gateway" Service (envoy-gateway-service.yaml) and must
be reverted; instead, remove local modifications from the vendored chart and
implement your desired service adjustments by either adding a Service override
in the wrapper chart's values (use .Values.service, .Values.deployment.ports,
.Values.topologyInjector.enabled, and .Values.service.trafficDistribution to
merge/override) or by adding a custom templates/Service definition in the
parent/wrapper chart that defines the Service for "envoy-gateway"; also file an
upstream issue against the Envoy Gateway repo so the change can be considered
upstream.
---
Nitpick comments:
In `@packages/apps/harbor/templates/httproute.yaml`:
- Around line 3-7: The template defines two identical variables $gateway and
$gatewayClassName causing redundancy; remove one (prefer keeping a single
variable name, e.g., $gatewayClassName or $gateway), update all references in
the template to use the chosen variable, and delete the other assignment (the
duplicate default assignment of .Values._namespace.gateway) so only one
defaulting expression remains (ensure functions or blocks that reference
$gateway or $gatewayClassName are updated accordingly).
In `@packages/extra/gateway/templates/gateway.yaml`:
- Around line 13-14: The hardcoded replicas: 2 under envoyDeployment should be
made configurable via values.yaml; add a new value (e.g.,
envoyDeployment.replicas or replicaCount) in values.yaml and replace the literal
2 in gateway.yaml with the Helm template variable (reference the envoyDeployment
object and its replicas field) so deployments can be adjusted per-environment;
ensure you provide a sensible default in values.yaml and update any README or
comments to document the new envoyDeployment.replicas option.
In `@packages/system/dashboard/templates/httproute.yaml`:
- Around line 6-7: The template defines two identical variables ($gateway and
$gatewayClassName); remove the redundant $gatewayClassName assignment and update
any usages of $gatewayClassName to use $gateway instead (e.g., replace
references to $gatewayClassName with $gateway and delete the line setting
$gatewayClassName).
In `@packages/system/envoy-gateway/charts/gateway-helm/templates/certgen.yaml`:
- Around line 30-44: The template duplicates the `command` block because of the
if/else around args; keep `command` rendered once and only conditionally render
the `args` block when `$args` is non-empty. In the certgen template, compute
`$args` (from `.Values.certgen.job.args` and appending
`"--disable-topology-injector"` when `.Values.topologyInjector.enabled` is
false) as you already do, then unconditionally emit the `- command: -
envoy-gateway - certgen` lines and, if `$args` is set, emit the `- args:` block
with `{{- toYaml $args | nindent 8 }}`; remove the current if/else that
duplicates `command`.
In
`@packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-poddisruptionbudget.yaml`:
- Line 8: The conditional uses a redundant "and" with a single argument; change
the template conditional that currently uses "{{- if and
.Values.podDisruptionBudget.minAvailable }}" to a simple "{{- if
.Values.podDisruptionBudget.minAvailable }}" so the PodDisruptionBudget
conditional correctly evaluates the presence/truthiness of
.Values.podDisruptionBudget.minAvailable (update the corresponding if/endif
block around that symbol).
In
`@packages/system/envoy-gateway/charts/gateway-helm/templates/namespaced-infra-manager-rbac.yaml`:
- Around line 1-35: When watch.type == "NamespaceSelector" and deploy.type ==
"GatewayNamespace" the template creates a tokenreview ClusterRole '{{ include
"eg.fullname" $ }}-infra-manager-tokenreview' that duplicates the tokenreview
rules already present in the other ClusterRole ('*-cluster-infra-manager' from
infra-manager-rbac.yaml); update this template to skip creating the tokenreview
ClusterRole when $kube.watch.type == "NamespaceSelector" (i.e., add a
conditional around the ClusterRole/ClusterRoleBinding blocks checking that not
(eq $kube.watch.type "NamespaceSelector")), or alternatively add a clear comment
in the template explaining why both exist to avoid audit confusion. Ensure you
reference $kube (from .Values.config.envoyGateway.provider.kubernetes) and the
ClusterRole name '{{ include "eg.fullname" $ }}-infra-manager-tokenreview' when
making the change.
In `@packages/system/gateway-rd/Makefile`:
- Line 4: The Makefile currently only contains the include line (include
../../../hack/package.mk) which causes checkmake to warn when scanning this file
alone; add lightweight local phony passthrough targets (e.g., .PHONY plus
targets like build, test, lint, install) that simply forward to the same targets
provided by the included package.mk so checkmake sees concrete targets. Ensure
the passthrough targets are non-functional stubs that invoke the included
makefile's targets (using the standard make invocation) and keep them minimal so
behavior doesn't change.
In `@packages/system/gateway-rd/templates/cozyrd.yaml`:
- Line 1: The template currently uses .Files.Glob "cozyrds/*" which can pull
non-YAML artifacts; narrow the glob to manifest files only by replacing that
pattern with YAML-specific globs (e.g., use .Files.Glob "cozyrds/*.yaml" and
also handle "cozyrds/*.yml" or iterate over both patterns) in the same template
range expression so only .yaml/.yml files are rendered; update the .Files.Glob
call(s) in the template to those YAML-only patterns (and add a second loop if
needed) to ensure safe, predictable output.
In `@packages/system/monitoring/templates/alerta/httproute.yaml`:
- Around line 3-7: The template repeats and risks nil access: remove the
redundant declaration of $gatewayClassName and consolidate gateway derivation by
using a single variable (e.g., $gateway) for both purposes, and add a safe
default when reading .Values._namespace.host (use default "" or fall back to
.Values.host) before computing $alertaHost; update references to use the single
$gateway variable and ensure $host is defined with a default so
.Values._namespace.host cannot be nil.
In `@packages/system/monitoring/templates/grafana/httproute.yaml`:
- Around line 3-7: The template defines duplicate variables $gateway and
$gatewayClassName from .Values._namespace.gateway and reads
.Values._namespace.host without a safe default; remove the redundancy by using
the single $gateway variable wherever gatewayClassName is needed (e.g., replace
uses of $gatewayClassName with $gateway) and protect host access by giving
.Values._namespace.host a default (e.g., use a default fallback when assigning
$host) to avoid nil access in templates such as in the $host and $grafanaHost
assignments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e8d5e5d-0469-4c6c-a271-57f6b3d81a9c
⛔ Files ignored due to path filters (9)
packages/extra/gateway/logos/gateway.svgis excluded by!**/*.svgpackages/system/envoy-gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_backends.yamlis excluded by!**/generated/**packages/system/envoy-gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_backendtrafficpolicies.yamlis excluded by!**/generated/**packages/system/envoy-gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_clienttrafficpolicies.yamlis excluded by!**/generated/**packages/system/envoy-gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoyextensionpolicies.yamlis excluded by!**/generated/**packages/system/envoy-gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoypatchpolicies.yamlis excluded by!**/generated/**packages/system/envoy-gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoyproxies.yamlis excluded by!**/generated/**packages/system/envoy-gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_httproutefilters.yamlis excluded by!**/generated/**packages/system/envoy-gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yamlis excluded by!**/generated/**
📒 Files selected for processing (53)
hack/e2e-install-cozystack.batspackages/apps/harbor/templates/httproute.yamlpackages/apps/tenant/templates/gateway.yamlpackages/core/platform/sources/envoy-gateway.yamlpackages/core/platform/sources/gateway-application.yamlpackages/core/platform/templates/apps.yamlpackages/core/platform/templates/bundles/system.yamlpackages/core/platform/values.yamlpackages/extra/bootbox/templates/matchbox/httproute.yamlpackages/extra/gateway/Chart.yamlpackages/extra/gateway/Makefilepackages/extra/gateway/templates/gateway.yamlpackages/extra/gateway/values.schema.jsonpackages/extra/gateway/values.yamlpackages/extra/seaweedfs/templates/tlsroute.yamlpackages/system/bucket/templates/httproute.yamlpackages/system/cozystack-api/templates/api-tlsroute.yamlpackages/system/dashboard/templates/httproute.yamlpackages/system/envoy-gateway/Chart.yamlpackages/system/envoy-gateway/Makefilepackages/system/envoy-gateway/charts/gateway-helm/.helmignorepackages/system/envoy-gateway/charts/gateway-helm/Chart.yamlpackages/system/envoy-gateway/charts/gateway-helm/README.mdpackages/system/envoy-gateway/charts/gateway-helm/crds/gatewayapi-crds.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/NOTES.txtpackages/system/envoy-gateway/charts/gateway-helm/templates/_helpers.tplpackages/system/envoy-gateway/charts/gateway-helm/templates/_rbac.tplpackages/system/envoy-gateway/charts/gateway-helm/templates/certgen-rbac.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/certgen.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-config.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-deployment.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-hpa.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-poddisruptionbudget.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-rbac.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-service.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-serviceaccount.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/envoy-proxy-topology-injector-webhook.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/infra-manager-rbac.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/leader-election-rbac.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/namespace.yamlpackages/system/envoy-gateway/charts/gateway-helm/templates/namespaced-infra-manager-rbac.yamlpackages/system/envoy-gateway/charts/gateway-helm/values.yamlpackages/system/envoy-gateway/values.yamlpackages/system/gateway-rd/Chart.yamlpackages/system/gateway-rd/Makefilepackages/system/gateway-rd/cozyrds/gateway.yamlpackages/system/gateway-rd/templates/cozyrd.yamlpackages/system/gateway-rd/values.yamlpackages/system/keycloak/templates/httproute.yamlpackages/system/kubevirt-cdi/templates/cdi-uploadproxy-tlsroute.yamlpackages/system/kubevirt/templates/vm-exportproxy-tlsroute.yamlpackages/system/monitoring/templates/alerta/httproute.yamlpackages/system/monitoring/templates/grafana/httproute.yaml
✅ Files skipped from review due to trivial changes (14)
- packages/extra/gateway/values.yaml
- packages/system/gateway-rd/values.yaml
- packages/system/envoy-gateway/values.yaml
- packages/extra/gateway/values.schema.json
- packages/system/envoy-gateway/Chart.yaml
- packages/core/platform/sources/envoy-gateway.yaml
- packages/extra/gateway/Chart.yaml
- packages/system/gateway-rd/Chart.yaml
- packages/system/envoy-gateway/charts/gateway-helm/.helmignore
- packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-serviceaccount.yaml
- packages/core/platform/values.yaml
- packages/core/platform/sources/gateway-application.yaml
- packages/system/gateway-rd/cozyrds/gateway.yaml
- packages/system/envoy-gateway/charts/gateway-helm/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/e2e-install-cozystack.bats
- packages/core/platform/templates/bundles/system.yaml
| {{- $gatewayAPI := (index .Values._cluster "gateway-api") | default "false" }} | ||
| {{- $clusterIssuer := (index .Values._cluster "issuer-name") | default "letsencrypt-prod" }} | ||
| {{- $gateway := .Values._namespace.gateway | default "" }} | ||
| {{- $host := .Values._namespace.host }} | ||
| {{- $harborHost := .Values.host | default (printf "%s.%s" .Release.Name $host) }} | ||
|
|
||
| {{- $gatewayClassName := .Values._namespace.gateway | default "" }} | ||
|
|
||
| {{- if and (eq $gatewayAPI "true") (ne $gateway "") }} |
There was a problem hiding this comment.
Routing conflict: Harbor Ingress will still be deployed when Gateway API is enabled.
The existing packages/apps/harbor/templates/ingress.yaml (shown in context snippets) creates an Ingress unconditionally without checking the gateway-api flag. When gateway-api: "true", both the Ingress and these Gateway API resources will be deployed for the same Harbor hostname, causing routing conflicts.
Consider adding a guard to ingress.yaml to skip Ingress creation when Gateway API is enabled:
{{- if ne $gatewayAPI "true" }}🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/apps/harbor/templates/httproute.yaml` around lines 1 - 9, The
Ingress template is created unconditionally causing conflicts when Gateway API
is enabled; update the ingress template (the file that currently renders the
Ingress resource) to wrap the whole Ingress manifest with a guard that checks
the same $gatewayAPI value used in httproute.yaml (and optionally $gateway) so
the Ingress is skipped when gateway-api is "true" (e.g., use an if that
evaluates ne $gatewayAPI "true" around the Ingress manifest); ensure you
reference the same .Values._cluster "gateway-api" lookup used to populate
$gatewayAPI so behavior matches the httproute rendering.
| {{- $gatewayAPI := (index .Values._cluster "gateway-api") | default "false" }} | ||
| {{- $clusterIssuer := (index .Values._cluster "issuer-name") | default "letsencrypt-prod" }} | ||
| {{- $host := index .Values._cluster "root-host" }} | ||
| {{- $hostnames := .Values._cluster.hostnames | default dict }} | ||
| {{- $exposeServices := splitList "," ((index .Values._cluster "expose-services") | default "") }} | ||
| {{- $gateway := ((.Values._namespace).gateway) | default (index .Values._cluster "gateway") | default "" }} | ||
| {{- $gatewayClassName := $gateway }} | ||
|
|
||
| {{- $dashboardHost := index $hostnames "dashboard" | default (printf "dashboard.%s" $host) }} | ||
|
|
||
| {{- if and (eq $gatewayAPI "true") (has "dashboard" $exposeServices) }} |
There was a problem hiding this comment.
Routing conflict: Ingress will still be deployed when Gateway API is enabled.
Looking at packages/system/dashboard/templates/ingress.yaml, the Ingress resource is created unconditionally when dashboard is in expose-services, without checking the gateway-api flag. When gateway-api: "true", both the Ingress and these Gateway API resources will be deployed for dashboard.<host>, causing routing conflicts.
Consider adding a guard to ingress.yaml to skip Ingress creation when Gateway API is enabled:
{{- if and (has "dashboard" $exposeServices) (ne $gatewayAPI "true") }}🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/dashboard/templates/httproute.yaml` around lines 1 - 11,
Ingress is still created unconditionally when "dashboard" is in expose-services,
causing conflicts with the Gateway API httproute which uses $gatewayAPI; update
the dashboard Ingress template to skip creating the Ingress when Gateway API is
enabled by wrapping the Ingress resource block with a conditional that checks
both that "dashboard" is in $exposeServices and that $gatewayAPI is not "true"
(use the same $gatewayAPI and $exposeServices symbols as in httproute.yaml and
the has/and/ne helpers to guard the Ingress).
|
|
||
| {{- $dashboardHost := index $hostnames "dashboard" | default (printf "dashboard.%s" $host) }} | ||
|
|
||
| {{- if and (eq $gatewayAPI "true") (has "dashboard" $exposeServices) }} |
There was a problem hiding this comment.
Inconsistent condition compared to other Gateway API templates.
This template only checks eq $gatewayAPI "true" and has "dashboard" $exposeServices, but doesn't verify that $gateway is non-empty. In contrast, the Grafana and Alerta templates (also in this PR) use (ne $gateway "") as part of their condition.
If $gateway is empty, line 20 will render an empty gatewayClassName, which is invalid. Consider aligning with the other templates:
-{{- if and (eq $gatewayAPI "true") (has "dashboard" $exposeServices) }}
+{{- if and (eq $gatewayAPI "true") (has "dashboard" $exposeServices) (ne $gateway "") }}📝 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.
| {{- if and (eq $gatewayAPI "true") (has "dashboard" $exposeServices) }} | |
| {{- if and (eq $gatewayAPI "true") (has "dashboard" $exposeServices) (ne $gateway "") }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/dashboard/templates/httproute.yaml` at line 11, The
conditional at the top of the template only checks eq $gatewayAPI "true" and has
"dashboard" $exposeServices but omits verifying $gateway is non-empty; update
the if clause to also include (ne $gateway "") so it matches the Grafana/Alerta
templates and prevents rendering an empty gatewayClassName when $gateway is
empty (adjust the conditional that currently references $gatewayAPI,
$exposeServices to also check $gateway).
| {{- if .Values.deployment.envoyGateway.image.repository -}} | ||
| {{/* if global.imageRegistry is defined, it takes precedence always */}} | ||
| {{- if .Values.global.imageRegistry -}} | ||
| {{- $repositoryParts := splitn "/" 2 .Values.deployment.envoyGateway.image.repository -}} | ||
| {{- $registryName := .Values.global.imageRegistry -}} | ||
| {{- $repositoryName := $repositoryParts._1 -}} | ||
| {{- $imageTag := default .Chart.AppVersion .Values.deployment.envoyGateway.image.tag -}} | ||
| {{- printf "%s/%s:%s" $registryName $repositoryName $imageTag -}} |
There was a problem hiding this comment.
Preserve the full repository path when global.imageRegistry is applied.
On Line 72–Line 75, the current splitn "/" 2 + _1 logic drops the first segment unconditionally. For repos like envoyproxy/gateway, this renders as <registry>/gateway:<tag> (namespace lost), which can break pulls.
💡 Suggested fix
{{- if .Values.global.imageRegistry -}}
-{{- $repositoryParts := splitn "/" 2 .Values.deployment.envoyGateway.image.repository -}}
-{{- $registryName := .Values.global.imageRegistry -}}
-{{- $repositoryName := $repositoryParts._1 -}}
+{{- $repo := .Values.deployment.envoyGateway.image.repository -}}
+{{- $repoParts := splitList "/" $repo -}}
+{{- $firstPart := first $repoParts -}}
+{{- $hasRegistryPrefix := or (contains "." $firstPart) (contains ":" $firstPart) (eq $firstPart "localhost") -}}
+{{- $repositoryName := ternary (join "/" (rest $repoParts)) $repo $hasRegistryPrefix -}}
+{{- $registryName := .Values.global.imageRegistry -}}
{{- $imageTag := default .Chart.AppVersion .Values.deployment.envoyGateway.image.tag -}}
{{- printf "%s/%s:%s" $registryName $repositoryName $imageTag -}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/envoy-gateway/charts/gateway-helm/templates/_helpers.tpl`
around lines 69 - 76, The template currently splits
.Values.deployment.envoyGateway.image.repository with splitn and uses
$repositoryParts._1, which drops namespace segments (e.g.,
"envoyproxy/gateway"); change the logic to preserve the full repository path
when .Values.global.imageRegistry is set by assigning $repositoryName to the
entire .Values.deployment.envoyGateway.image.repository (instead of using
splitn/$repositoryParts._1) and then build the final image string with printf
"%s/%s:%s" using $registryName, the full $repositoryName, and $imageTag.
| {{- $imageParts := splitn "/" 2 .Values.global.images.ratelimit.image -}} | ||
| {{/* if global.imageRegistry is defined, it takes precedence always */}} | ||
| {{- $registryName := default $imageParts._0 .Values.global.imageRegistry -}} | ||
| {{- $repositoryTag := $imageParts._1 -}} | ||
| {{- $repositoryParts := splitn ":" 2 $repositoryTag -}} | ||
| {{- $repositoryName := $repositoryParts._0 -}} | ||
| {{- $imageTag := default "master" $repositoryParts._1 -}} | ||
| {{- printf "%s/%s:%s" $registryName $repositoryName $imageTag -}} |
There was a problem hiding this comment.
Avoid floating master as the ratelimit image fallback tag.
Line 132 defaults to master when the image string has no tag. That introduces non-deterministic upgrades and supply-chain drift. Please switch this fallback to a pinned value sourced from chart values (or a digest-based default).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/envoy-gateway/charts/gateway-helm/templates/_helpers.tpl`
around lines 126 - 133, The current helper uses a hardcoded fallback "master"
for $imageTag which causes non-deterministic upgrades; change the default to a
pinned value coming from chart values (e.g. replace the literal "master" default
with something like .Values.global.images.ratelimit.tag or prefer a digest field
.Values.global.images.ratelimit.digest when present) and update values.yaml to
expose that pinned tag/digest; adjust the logic around
$repositoryParts/$imageTag to prefer the provided digest/tag from
.Values.global.images.ratelimit before falling back to the chart-level default.
| {{- if .Values.hpa.minReplicas }} | ||
| minReplicas: {{ .Values.hpa.minReplicas }} | ||
| {{- end }} | ||
| maxReplicas: {{ required ".Values.hps.maxReplicas is required when hpa is enabled" .Values.hpa.maxReplicas }} |
There was a problem hiding this comment.
Typo in error message: hps should be hpa.
The required error message references .Values.hps.maxReplicas but the actual value path is .Values.hpa.maxReplicas. This will confuse users when debugging.
🔧 Proposed fix
- maxReplicas: {{ required ".Values.hps.maxReplicas is required when hpa is enabled" .Values.hpa.maxReplicas }}
+ maxReplicas: {{ required ".Values.hpa.maxReplicas is required when hpa is enabled" .Values.hpa.maxReplicas }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-hpa.yaml`
at line 15, The error message in the Helm template's required check for
maxReplicas contains a typo (`.Values.hps.maxReplicas`); update the required
call message to reference the correct path `.Values.hpa.maxReplicas` so the
check on maxReplicas (the line using required and .Values.hpa.maxReplicas)
reports the correct configuration key.
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: envoy-gateway | ||
| namespace: '{{ .Release.Namespace }}' | ||
| {{- with .Values.service.annotations }} | ||
| annotations: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| labels: | ||
| control-plane: envoy-gateway | ||
| {{- include "eg.labels" . | nindent 4 }} | ||
| spec: | ||
| type: {{ .Values.service.type }} | ||
| {{- if and (eq .Values.service.type "LoadBalancer") .Values.service.loadBalancerIP }} | ||
| loadBalancerIP: {{ .Values.service.loadBalancerIP }} | ||
| {{- end }} | ||
| selector: | ||
| control-plane: envoy-gateway | ||
| {{- include "eg.selectorLabels" . | nindent 4 }} | ||
| ports: | ||
| {{- .Values.deployment.ports | toYaml | nindent 2 -}} | ||
| {{- if .Values.topologyInjector.enabled }} | ||
| - name: webhook | ||
| port: 9443 | ||
| targetPort: 9443 | ||
| {{- end }} | ||
| {{- with .Values.service.trafficDistribution }} | ||
| trafficDistribution: {{ . }} | ||
| {{- end }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if gateway-helm is a vendored chart by looking for Chart.yaml and comparing to upstream
echo "=== Checking for Chart.yaml in gateway-helm ==="
fd -t f "Chart.yaml" packages/system/envoy-gateway/charts/gateway-helm --exec cat {}
echo ""
echo "=== Looking for chart vendoring mechanism (e.g., Chart.lock, dependency files) ==="
fd -t f -e yaml -e lock . packages/system/envoy-gateway --max-depth 2 --exec cat {}Repository: cozystack/cozystack
Length of output: 1052
Remove modifications to the vendored upstream Envoy Gateway Helm chart.
The file packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-service.yaml is located inside the vendored upstream Envoy Gateway Helm chart (as confirmed by the Chart.yaml referencing https://github.com/envoyproxy/gateway). Per coding guidelines, vendored upstream charts in the charts/ directory must not be modified directly.
If service configuration changes are needed:
- Report the issue upstream at https://github.com/envoyproxy/gateway
- Override service values in the wrapper chart (
packages/system/envoy-gateway/Chart.yaml) using values merging - Use a custom
templates/directory in the parent chart for wrapper-specific service definitions
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 7-7: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/envoy-gateway/charts/gateway-helm/templates/envoy-gateway-service.yaml`
around lines 1 - 30, This change edits the vendored upstream Helm chart's
Service template for the "envoy-gateway" Service (envoy-gateway-service.yaml)
and must be reverted; instead, remove local modifications from the vendored
chart and implement your desired service adjustments by either adding a Service
override in the wrapper chart's values (use .Values.service,
.Values.deployment.ports, .Values.topologyInjector.enabled, and
.Values.service.trafficDistribution to merge/override) or by adding a custom
templates/Service definition in the parent/wrapper chart that defines the
Service for "envoy-gateway"; also file an upstream issue against the Envoy
Gateway repo so the change can be considered upstream.
Helm 3 does not process CRDs from subchart crds/ directories in vanilla Helm, while Flux does process them causing conflicts with the existing gateway-api-crds package. Move only Envoy-specific CRDs (gateway.envoyproxy.io) into templates/crds/ so they are installed and updated as regular resources. Remove the subchart crds/ directory to prevent Flux from double-applying standard Gateway API CRDs already provided by the gateway-api-crds package. Update Makefile to copy Envoy CRDs and remove subchart crds/ after helm pull. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
sircthulhu
force-pushed
the
feat/gateway-api-httproutes
branch
from
ccb04ca to
5fcac56
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/envoy-gateway/Makefile`:
- Line 14: The Makefile currently deletes charts/gateway-helm/crds which mutates
the vendored chart; remove the `rm -rf charts/gateway-helm/crds` step and
instead ensure CRDs are copied from the pulled chart into a separate location
used by packaging (e.g., copy charts/gateway-helm/crds -> templates/crds/ or a
build/artifacts dir) so charts/gateway-helm remains an untouched upstream copy;
update the Makefile targets that relied on that removal to use the new copy step
(reference the Makefile target that runs `helm pull` and the path
charts/gateway-helm/crds and templates/crds to locate where to change).
In
`@packages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_backends.yaml`:
- Around line 4-7: The CRD metadata for backends.gateway.envoyproxy.io is
missing the Helm-safe annotation; add the annotation "helm.sh/resource-policy:
keep" under metadata.annotations for the CRD (the block where name:
backends.gateway.envoyproxy.io is defined) to prevent Helm from deleting the CRD
on uninstall, and apply the same annotation to every other copied CRD file in
this templates directory so all CRDs are preserved across Helm uninstall.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdfa7ce6-ea9b-4d6e-aa48-d3b96edffe59
📒 Files selected for processing (9)
packages/system/envoy-gateway/Makefilepackages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_backends.yamlpackages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_backendtrafficpolicies.yamlpackages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_clienttrafficpolicies.yamlpackages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_envoyextensionpolicies.yamlpackages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_envoypatchpolicies.yamlpackages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_envoyproxies.yamlpackages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_httproutefilters.yamlpackages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_securitypolicies.yaml
| rm -rf templates/crds | ||
| mkdir -p templates/crds | ||
| cp charts/gateway-helm/crds/generated/*.yaml templates/crds/ | ||
| rm -rf charts/gateway-helm/crds |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the vendored chart pristine after helm pull.
The last line rewrites the upstream chart inside charts/. Even if we need copied CRDs under templates/crds/, charts/gateway-helm should stay as-pulled so future bumps remain auditable and we do not drift from the repo’s vendoring pattern.
♻️ Proposed fix
cp charts/gateway-helm/crds/generated/*.yaml templates/crds/
- rm -rf charts/gateway-helm/crdsBased on learnings, vendored charts under packages/system/*/charts/ should remain an upstream copy and not be modified directly.
📝 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.
| rm -rf charts/gateway-helm/crds | |
| cp charts/gateway-helm/crds/generated/*.yaml templates/crds/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/envoy-gateway/Makefile` at line 14, The Makefile currently
deletes charts/gateway-helm/crds which mutates the vendored chart; remove the
`rm -rf charts/gateway-helm/crds` step and instead ensure CRDs are copied from
the pulled chart into a separate location used by packaging (e.g., copy
charts/gateway-helm/crds -> templates/crds/ or a build/artifacts dir) so
charts/gateway-helm remains an untouched upstream copy; update the Makefile
targets that relied on that removal to use the new copy step (reference the
Makefile target that runs `helm pull` and the path charts/gateway-helm/crds and
templates/crds to locate where to change).
| metadata: | ||
| annotations: | ||
| controller-gen.kubebuilder.io/version: v0.18.0 | ||
| name: backends.gateway.envoyproxy.io |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Helm v3, when a CustomResourceDefinition is rendered from a chart's templates/directory instead ofcrds/, is it deleted on uninstall, and is metadata.annotations["helm.sh/resource-policy"]="keep" the supported way to prevent that?
💡 Result:
In Helm v3, a CustomResourceDefinition (CRD) rendered from a chart's templates/ directory is treated as a regular templated resource and will be deleted on helm uninstall. CRDs in the crds/ directory are handled specially: they are installed if missing but never deleted by Helm. The annotation metadata.annotations["helm.sh/resource-policy"]="keep" is the supported way to prevent deletion of templated resources (including CRDs from templates/) during helm uninstall, helm upgrade, or helm rollback. Official Helm documentation explicitly describes this usage, and GitHub issues confirm it works for CRDs when applied via the chart template (though manual kubectl annotate may have limitations in some versions). To use it, add to your CRD YAML in templates/: metadata: annotations: helm.sh/resource-policy: keep Note: Resources with this annotation become orphaned after uninstall; Helm no longer manages them.
Citations:
- 1: Moving CRDs from templates/ to crds/ results in CRDs being deleted helm/helm#7279
- 2: https://v3.helm.sh/docs/chart_best_practices/custom_resource_definitions/
- 3: helm3 deleted CRDs with uninstall helm/helm#11564
- 4: https://helm.sh/docs/chart_best_practices/custom_resource_definitions
- 5: Annotation
"helm.sh/resource-policy": keepnot honored when uninstalling helm/helm#10890 - 6: https://helm.sh/docs/howto/charts_tips_and_tricks
Add helm.sh/resource-policy: keep to prevent Helm from deleting the CRD on uninstall.
This CRD is rendered from templates/ rather than the special crds/ directory, so Helm treats it as a normal templated resource. On uninstall, Helm will delete it and cascade-delete every instance of this API. Add the annotation to prevent deletion, or move these CRDs to a dedicated release.
Suggested change
metadata:
annotations:
+ helm.sh/resource-policy: keep
controller-gen.kubebuilder.io/version: v0.18.0Apply this fix to all copied CRDs in this directory.
📝 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.
| metadata: | |
| annotations: | |
| controller-gen.kubebuilder.io/version: v0.18.0 | |
| name: backends.gateway.envoyproxy.io | |
| metadata: | |
| annotations: | |
| helm.sh/resource-policy: keep | |
| controller-gen.kubebuilder.io/version: v0.18.0 | |
| name: backends.gateway.envoyproxy.io |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/envoy-gateway/templates/crds/gateway.envoyproxy.io_backends.yaml`
around lines 4 - 7, The CRD metadata for backends.gateway.envoyproxy.io is
missing the Helm-safe annotation; add the annotation "helm.sh/resource-policy:
keep" under metadata.annotations for the CRD (the block where name:
backends.gateway.envoyproxy.io is defined) to prevent Helm from deleting the CRD
on uninstall, and apply the same annotation to every other copied CRD file in
this templates directory so all CRDs are preserved across Helm uninstall.
…eout in e2e After enabling gatewayAPI on the platform, system HelmReleases need to pick up the new gateway-api value from cozystack-values secret. Force reconcile dashboard and cozystack-api to avoid waiting up to 5 minutes for the next Flux reconciliation cycle. Increase gateway address timeout from 120s to 300s to allow time for Envoy data plane provisioning. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Envoy Gateway subchart hardcodes kubernetesClusterDomain to cluster.local, but cozystack clusters use a configurable domain (e.g. cozy.local). This causes data plane pods to fail connecting to the xDS control plane because the DNS name in the bootstrap config doesn't resolve. Add global.clusterDomain to cozystack-values secret from the platform's networking.clusterDomain setting. Patch the subchart deployment and certgen templates to prefer global.clusterDomain over the hardcoded default. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
When gatewayAPI is enabled, cert-manager needs a gatewayHTTPRoute solver to issue certificates for Gateway listeners. Previously only the Ingress solver was configured, causing ACME challenges to fail when Ingress is disabled. The solver list is now dynamic based on platform settings: - ingress-enabled=true: adds http01.ingress solver - gateway-api=true: adds http01.gatewayHTTPRoute solver - both enabled: both solvers are present - dns01: unchanged, uses Cloudflare solver Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
cert-manager requires a parentRef in the gatewayHTTPRoute solver to know which Gateway should serve the ACME HTTP-01 challenge. Without it, the webhook rejects the ClusterIssuer. Add a shared acme-challenge Gateway in the extra/gateway chart with a wildcard HTTP listener (allowedRoutes from All namespaces). This Gateway merges with other per-component Gateways via mergeGateways and serves ACME challenges for all certificates regardless of namespace. Update the ClusterIssuer template to reference this Gateway when gatewayAPI is enabled. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
…lenge Gateway Per-component Gateways had HTTP listeners with hostname-specific redirect routes that conflicted with cert-manager ACME HTTP-01 challenges. The redirect captured HTTP traffic before the challenge HTTPRoute could serve the token, creating a chicken-and-egg problem. Move the HTTP listener to a single shared acme-challenge Gateway with a wildcard listener (no hostname restriction, allowedRoutes from All). Add a global redirect-to-https HTTPRoute on this Gateway. Per-component Gateways now only have HTTPS listeners. The ACME challenge Exact path match takes priority over the redirect's implicit PathPrefix, so challenges are served correctly while all other HTTP traffic is redirected to HTTPS. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
The keycloak-http service listens on port 8080, not 80. This caused Envoy Gateway to return 500 for all keycloak requests via Gateway API. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Replace blocking flux reconcile --force with non-blocking annotation to avoid 5-minute timeout when dashboard HR has unresolved dependencies. Use fully qualified gateways.gateway.networking.k8s.io resource name to avoid conflict with cozystack Gateway CRD short name. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Per-component Gateways now only have HTTPS listeners which require a valid TLS certificate to become ready. In e2e with example.org domain, ACME challenges can't be completed so certificates are never issued and HTTPS listeners stay not-ready, causing HTTPRoute acceptance to fail. Switch to selfsigned-cluster-issuer in e2e so certificates are issued immediately without ACME verification. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
lexfrei
left a comment
lexfrei
left a comment
There was a problem hiding this comment.
Architecture
The overall design is sound: per-tenant GatewayClass (name = namespace), mergeGateways: true for one LB Service per tenant, Envoy Gateway as controller with Cilium staying as CNI. The dual-stack coexistence with ingress-nginx and the migration path (ingress: true → both → gatewayAPI: true) are well thought out.
However, the execution has several issues — some are security concerns, others are functional bugs from missing conditions or sources. The iterative nature of 31 commits (1 feat + 15 fixes) suggests these were discovered during debugging but not all resolved.
Blocking issues
- Security:
acme-challengeGateway accepts routes from any namespace — allows tenants to intercept HTTP traffic on port 80 - external-dns missing
gateway-tlsroutesource — 5 TLSRoute services (kubernetes-api, vm-exportproxy, cdi-uploadproxy, seaweedfs-filer, openbao) won't get DNS records - Empty
solvers:in ClusterIssuer — reachable when bothingress: falseandgatewayAPI: false - Keycloak sticky session disabled unconditionally — regression for ingress-only deployments
- Envoy Gateway deployed unconditionally — resource overhead and attack surface when
gatewayAPI: false - Alerta/Grafana duplicate HTTP listeners — architectural inconsistency with dashboard/keycloak pattern
| port: 80 | ||
| allowedRoutes: | ||
| namespaces: | ||
| from: All |
There was a problem hiding this comment.
allowedRoutes.namespaces.from: All on a port 80 listener means any namespace can attach HTTPRoutes to this Gateway. With mergeGateways: true, a tenant can create an HTTPRoute with a specific hostname that takes priority over the catch-all redirect-to-https, intercepting HTTP traffic or blocking ACME challenges.
Compare with Harbor's Gateway which correctly uses from: Same.
This should be from: Same or use a label selector. For cert-manager cross-namespace ACME challenges, a ReferenceGrant is the proper mechanism.
| {{include "cozystack.platform.package.optional.default" (list "cozystack.external-dns" $) }} | ||
| {{- $externalDnsComponents := dict -}} | ||
| {{- if .Values.gateway.gatewayAPI -}} | ||
| {{- $externalDnsValues := dict "external-dns" (dict "sources" (list "service" "ingress" "gateway-httproute")) -}} |
There was a problem hiding this comment.
Missing gateway-tlsroute source. Services exposed via TLSRoute won't get DNS records:
kubernetes-api(cozystack-api/templates/api-tlsroute.yaml)vm-exportproxy(kubevirt/templates/vm-exportproxy-tlsroute.yaml)cdi-uploadproxy(kubevirt-cdi/templates/cdi-uploadproxy-tlsroute.yaml)seaweedfs-filer(extra/seaweedfs/templates/tlsroute.yaml)openbao(openbao/charts/openbao/templates/server-tlsroute.yaml)
Add "gateway-tlsroute" to the sources list.
| {{- $_ := set $tenantComponents "tenant" (dict "values" $tenantClusterValues) }} | ||
| {{include "cozystack.platform.package" (list "cozystack.tenant-application" "default" $ $tenantComponents) }} | ||
| {{include "cozystack.platform.package.default" (list "cozystack.ingress-application" $) }} | ||
| {{include "cozystack.platform.package.default" (list "cozystack.gateway-application" $) }} |
There was a problem hiding this comment.
gateway-application (Envoy Gateway controller + CRDs + per-tenant chart) is deployed via package.default — always installed unless explicitly in disabledPackages. This installs Envoy Gateway pods, webhooks, and ~31k lines of CRDs on every cluster, even when gatewayAPI: false.
Compare with how ingress-application is also always deployed — but ingress-nginx is the established default. Envoy Gateway is a new, optional controller.
Consider either package.optional.default or a conditional on gateway.gatewayAPI.
| apiTokenSecretRef: | ||
| name: cloudflare-api-token-secret | ||
| key: api-token | ||
| {{- else }} |
There was a problem hiding this comment.
When solver != "dns01": if both ingressEnabled == "false" (line 19 condition fails) and gatewayAPI == "false" or gateway == "" (line 24 condition fails), the solvers: list is empty. An ACME ClusterIssuer with zero solvers is invalid.
Default values have ingress: true, so this is unlikely but reachable through explicit user configuration. Add a fail with a clear message or a default solver.
| value: https://{{ $ingressHost }} | ||
| - name: JAVA_OPTS_APPEND | ||
| value: "-Djgroups.dns.query=keycloak-headless.cozy-keycloak.svc.{{ $clusterDomain }}" | ||
| - name: KC_SPI_STICKY_SESSION_ENCODER_INFINISPAN_SHOULD_ATTACH_ROUTE |
There was a problem hiding this comment.
This env var was added specifically for Gateway API (commit message: "Disable Keycloak sticky session route attachment... removes dependency on nginx cookie affinity"). But it's set unconditionally, affecting ingress-only deployments too.
With ingress-nginx, Keycloak relies on nginx.ingress.kubernetes.io/affinity: "cookie" for session affinity. Disabling route attachment unconditionally may break multi-replica Keycloak session handling under ingress-nginx.
Gate this on the gateway-api cluster value.
| labels: | ||
| cozystack.io/gateway: {{ $gateway }} | ||
| listeners: | ||
| - name: http |
There was a problem hiding this comment.
This Gateway creates a per-component HTTP listener on port 80 with its own redirect-to-https HTTPRoute. But the architecture already has a shared acme-challenge Gateway (in extra/gateway/templates/gateway.yaml) that handles HTTP→HTTPS redirect for all traffic.
Compare with dashboard and keycloak which correctly have HTTPS-only listeners and rely on the shared redirect. Alerta and Grafana should follow the same pattern.
| labels: | ||
| cozystack.io/gateway: {{ $gateway }} | ||
| listeners: | ||
| - name: http |
There was a problem hiding this comment.
Same issue as Alerta — per-component HTTP listener on port 80 duplicates the shared acme-challenge Gateway's redirect functionality. Should have HTTPS-only listener like dashboard and keycloak.
|
|
||
| # Wait for monitoring to reconcile with gateway config | ||
| if ! kubectl wait hr/monitoring -n tenant-root --timeout=3m --for=condition=ready; then | ||
| flux reconcile hr monitoring -n tenant-root --force |
There was a problem hiding this comment.
flux reconcile requires flux CLI to be installed. Line 219 in the same PR explicitly says: "Use annotation instead of flux reconcile --force to avoid blocking on dependencies" and uses kubectl annotate instead. This line should follow the same pattern for consistency and to avoid an undeclared dependency on flux CLI.
2 participants
What this PR does
Adds Gateway API (HTTPRoute/TLSRoute) support alongside existing Ingress resources for all system and tenant-level components, enabling a migration path from ingress-nginx to Gateway API using Envoy Gateway as the controller.
Why Envoy Gateway instead of Cilium Gateway API
Cilium's built-in Gateway API controller creates a separate LoadBalancer Service per Gateway resource. With
publishing.externalIPs, IPs get spread across services instead of being concentrated on one — breaking the externalIPs use case entirely.Cilium limitations that drove this decision:
mergeGateways— each Gateway = separate LB Service, no way to share IP on same portsXListenerSet(GEP-1713) — cannot delegate listeners to shared GatewaystreamIdleTimeoutDurationSecondsEnvoy Gateway solves all of these. Cilium remains the CNI (eBPF, network policy, LB IPAM).
Architecture
Per-tenant gateway model — mirrors ingress-nginx:
gateway: truein tenant → deploys per-tenant GatewayClass + EnvoyProxymergeGateways: true→ all per-component Gateways in the tenant merge into one LB ServiceCiliumLoadBalancerIPPoolassigns externalIPs to the merged Servicegateway: false→ inherit parent's GatewayClassTLS certificates via cert-manager + Gateway API
ACME HTTP-01 challenges are served through a shared
acme-challengeGateway with a wildcard HTTP listener (allowedRoutes: All). This avoids conflict with HTTP→HTTPS redirects:acme-challengeGateway is the sole HTTP listener on port 80redirect-to-httpsHTTPRoute handles HTTP→HTTPS redirect/.well-known/acme-challenge/TOKEN) takes priority over the redirect (implicit PathPrefix/)http01.ingresswhen ingress enabled,http01.gatewayHTTPRoutewhen gatewayAPI enabled, both when both enabled. dns01 unchanged.Envoy Gateway cluster domain
Envoy Gateway subchart hardcodes
kubernetesClusterDomain: cluster.local, but cozystack uses a configurable domain (e.g.cozy.local). The subchart templates are patched duringmake updateto readglobal.clusterDomain(populated fromnetworking.clusterDomainvia cozystack-values secret) with fallback to the default.Package structure (mirrors ingress-nginx)
packages/system/ingress-nginx/packages/system/envoy-gateway/packages/extra/ingress/packages/extra/gateway/packages/system/ingress-rd/packages/system/gateway-rd/sources/ingress-nginx.yamlsources/envoy-gateway.yamlsources/ingress-application.yamlsources/gateway-application.yamltenant/templates/ingress.yamltenant/templates/gateway.yamlComponents covered
All Gateway API resources are conditional on
gateway.gatewayAPI: true. Existing Ingress templates are untouched — both can coexist simultaneously.Release note
Summary by CodeRabbit