Enable Cilium Gateway API #923 #924
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces support for a Gateway API addon in the Kubernetes application Helm chart. It adds configuration options, documentation, schema validation, and conditional resource templates for deploying Gateway API CRDs using a new HelmRelease. The Cilium HelmRelease and deletion job templates are updated for dependency and lifecycle management. A new system package for the Gateway API CRDs is also added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes
participant GatewayAPI CRDs HelmRelease
participant Cilium HelmRelease
User->>Helm: Set addons.gatewayAPI.enabled = true
Helm->>Kubernetes: Deploy GatewayAPI CRDs HelmRelease
Helm->>Kubernetes: Deploy Cilium HelmRelease (depends on GatewayAPI CRDs)
Kubernetes->>GatewayAPI CRDs HelmRelease: Install CRDs
Kubernetes->>Cilium HelmRelease: Install Cilium with Gateway API integration
Possibly related PRs
Suggested labels
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/system/gateway-api/Chart.yaml (1)
1-3: Add chart metadata and verify version handling
TheChart.yamlcorrectly definesapiVersion,name, and uses aversion: 0.0.0placeholder for CI-driven updates. Consider adding adescriptionfield to improve chart discoverability:description: Cozystack Gateway API Helm chartAlso ensure your build pipeline replaces the placeholder version as intended to avoid packaging/installation failures.
packages/system/gateway-api/Makefile (1)
1-9: Parameterize the CRD version
Hardcodingv1.2.0in thekubectl kustomizecommand will require manual updates for future Gateway API releases. Introduce aCRD_VERSIONvariable and reference it:-export NAME=gateway-api -export NAMESPACE=cozy-$(NAME) +export NAME := gateway-api +export NAMESPACE := cozy-$(NAME) +CRD_VERSION := v1.2.0 update: 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=$(CRD_VERSION)" > templates/crds-experimental.yaml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/apps/kubernetes/templates/helmreleases/cilium.yaml(2 hunks)packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml(1 hunks)packages/system/gateway-api/Chart.yaml(1 hunks)packages/system/gateway-api/Makefile(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (3)
packages/apps/kubernetes/templates/helmreleases/cilium.yaml (2)
8-11: Enable Gateway API and Envoy features
The newgatewayAPI.enabledandenvoy.enabledflags are correctly added under theciliumvalues. Please verify against the upstream Cilium Helm chart’s values reference to ensure these keys match exactly (case-sensitive) and toggle the intended modules.
53-54: Add HelmRelease dependency for Gateway API
Declaring a dependency on{{ .Release.Name }}-gateway-apiensures Cilium deploys only after the Gateway API is ready, preventing race conditions. This is critical for orchestrating the startup sequence.packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (1)
1-4: Ignore YAMLlint false positive
The reported lint error at line 4 is due to Helm’s Go template syntax, which YAMLlint doesn’t understand. The final rendered manifest is valid YAML once variables are substituted.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 4-4: syntax error: expected , but found ''
(syntax)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (2)
9-18: Pin your chart version to avoid unintended upgrades
Right now this will always track the latest chart in the repo. It’s safer to add aversionfield underspec.chart(e.g.version: 0.0.0or a templated placeholder) so that automated rollouts only happen when you explicitly bump it.spec: interval: 5m releaseName: gateway-api chart: spec: chart: cozy-gateway-api + version: 0.0.0 # pin to the initial version; update via your build pipeline reconcileStrategy: Revision
23-26: Consider isolating the Gateway API in its own namespace
Deploying inkube-systemcan blur the line between system and workload components. You might improve security and manageability by creating a dedicated namespace (e.g.gateway-system) instead of reusingkube-system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (4)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (4)
4-7: Labels are correctly spelled and consistent
Nice catch fixing the previous typo—cozystack.io/repositoryandcozystack.io/target-cluster-nameare now correctly namespaced.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 4-4: syntax error: expected , but found ''
(syntax)
27-31: Review remediation retry strategy
You currently allow unlimited retries (-1) for both install and upgrade remediation. Confirm this is intentional—an endless loop on a bad release can flood your logs. You may want to set a finite retry count or add a backoff.
19-22:❓ Verification inconclusive
Verify the kubeConfig secret and key
You’re referencing a secret named{{ .Release.Name }}-admin-kubeconfigwith keysuper-admin.svc. Please confirm that this secret exists in the cluster and that its data key matches—otherwise the HelmRelease will fail on reconcile.
🏁 Script executed:
#!/bin/bash set -e RELEASE="{{ .Release.Name }}" NS="kube-system" SECRET="${RELEASE}-admin-kubeconfig" # Check secret presence and key kubectl get secret -n $NS $SECRET \ -o jsonpath='{.data.super-admin\.svc}' \ >/dev/null && echo "✔ Secret and key exist" || echo "✗ Missing secret or key"Length of output: 376
Verify
kubeConfigsecret and key
Ensure that the secret{{ .Release.Name }}-admin-kubeconfigexists in thekube-systemnamespace and that it contains asuper-admin.svcdata key—otherwise the HelmRelease will fail to reconcile.• File: packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
Lines 19-22You can confirm this by running in your cluster environment:
kubectl get secret -n kube-system {{ .Release.Name }}-admin-kubeconfig \ -o jsonpath='{.data.super-admin\.svc}'Or inspect the entire secret with:
kubectl get secret -n kube-system {{ .Release.Name }}-admin-kubeconfig -o yaml
1-2:❓ Verification inconclusive
Confirm HelmRelease API version
Ensure that your target cluster has the CRD forHelmReleaseunderhelm.toolkit.fluxcd.io/v2. Some FluxCD installations ship withv2beta1orv2alpha1. If your cluster does not recognizev2, you’ll need to align this to the supported apiVersion.
🏁 Script executed:
#!/bin/bash # Verify that the HelmRelease CRD version exists kubectl api-resources --api-group=helm.toolkit.fluxcd.io | grep -E '^helmreleases\s+v2\b'Length of output: 189
Confirm HelmRelease API version compatibility in gateway-api.yaml
Ensure your cluster supports thehelm.toolkit.fluxcd.io/v2HelmRelease CRD. Many FluxCD installations default tov2beta1orv2alpha1, so ifv2isn’t recognized, you’ll need to update this file to the supported version.To verify on your cluster:
kubectl api-resources --api-group=helm.toolkit.fluxcd.io | grep '^helmreleases'– If you don’t see a
v2entry, adjust lines 1–2 of
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
from:apiVersion: helm.toolkit.fluxcd.io/v2 kind: HelmReleaseto the supported API version (e.g.,
helm.toolkit.fluxcd.io/v2beta1).
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (1)
32-36: Fixed self-referentialdependsOnblock
This block currently creates a dependency on itself, which has no effect. To depend on the Cilium HelmRelease (named{{ .Release.Name }}-cilium), replace it as follows:- dependsOn: - {{- if lookup "helm.toolkit.fluxcd.io/v2" "HelmRelease" .Release.Namespace .Release.Name }} - - name: {{ .Release.Name }} - namespace: {{ .Release.Namespace }} - {{- end }} + dependsOn: + - name: {{ .Release.Name }}-cilium + namespace: {{ .Release.Namespace }}
🧹 Nitpick comments (3)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (3)
1-4: Quote templating for valid YAML and suppress linter false positives
YAML linters (e.g., yamllint) will error on unquoted Go template scalars. Wrap templated values in quotes to maintain valid YAML syntax:metadata: - name: {{ .Release.Name }}-gateway-api + name: "{{ .Release.Name }}-gateway-api"You can apply the same quoting pattern to other templated fields or disable YAMLLint on this file if preferred.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 4-4: syntax error: expected , but found ''
(syntax)
23-24: Namespace selection for installation
Usingkube-systemfor bothtargetNamespaceandstorageNamespacemay introduce resource noise in the core namespace. Consider isolating the Gateway API resources in a dedicated namespace (e.g.,cozy-system-networking) for clearer ownership and easier cleanup.
25-31: Reconsider unlimited retry strategy
Setting bothinstall.remediation.retriesandupgrade.remediation.retriesto-1allows infinite retries, which could mask persistent failures and lead to resource churn. You may want to use a finite retry count (e.g.,3or5) or add a timeout:install: - remediation: - retries: -1 + remediation: + retries: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (3)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (3)
5-7: Labels correctly set
Thecozystack.io/repositoryandcozystack.io/target-cluster-namelabels follow the existing conventions and appear accurate.
9-18: Verify chart source configuration
Theinterval,releaseName, andchart.specblocks look sound. Please confirm that:
- The
HelmRepositorynamedcozystack-systemexists in thecozy-systemnamespace.- The chart
cozy-gateway-apiat the referenced revision is available in that repository.
19-22: Confirm kubeconfig secret exists
You reference a secret named{{ .Release.Name }}-admin-kubeconfigwith keysuper-admin.svc. Ensure this secret is provisioned before reconciliation to avoid deployment failures.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (1)
32-36: Remove self-dependency and fix indentation
This block creates a self-referential dependency (.Release.Name) which has no effect and introduces invalid YAML indentation. To depend on the Cilium HelmRelease instead, update as follows:-spec: - dependsOn: - {{- if lookup "helm.toolkit.fluxcd.io/v2" "HelmRelease" .Release.Namespace .Release.Name }} - - name: {{ .Release.Name }} - namespace: {{ .Release.Namespace }} - {{- end }} +spec: + dependsOn: + {{- if lookup "helm.toolkit.fluxcd.io/v2" "HelmRelease" .Release.Namespace (printf "%s-cilium" .Release.Name) }} + - name: {{ printf "%s-cilium" .Release.Name }} + namespace: {{ .Release.Namespace }} + {{- end }}
🧹 Nitpick comments (3)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (3)
9-10: Ensure dynamic Helm release naming
Currently you use a staticreleaseName: gateway-api. If you deploy multiple clusters or need to avoid name collisions, consider making this dynamic to matchmetadata.name:-spec: - releaseName: gateway-api +spec: + releaseName: {{ printf "%s-gateway-api" .Release.Name }}
11-18: Pin chart version and validate sourceRef
- You’re relying on
reconcileStrategy: Revisionwithout specifying aversion:. Adding a version constraint improves reproducibility:spec: chart: cozy-gateway-api
version: <semver> reconcileStrategy: Revision
- Verify that a
HelmRepositorynamedcozystack-systemexists in thecozy-systemnamespace.
23-31: Consider namespace isolation for Gateway API
Deploying both control-plane and release artifacts intokube-systemcan blur system boundaries. You might:
- Use a dedicated namespace (e.g.,
gateway-system8000 ) fortargetNamespace.- Revert
storageNamespaceto Flux’s default (flux-system) unless you have a specific reason to store Helm artifacts inkube-system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/apps/kubernetes/templates/helmreleases/cilium.yaml(2 hunks)packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml(1 hunks)packages/system/gateway-api/Chart.yaml(1 hunks)packages/system/gateway-api/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/apps/kubernetes/templates/helmreleases/cilium.yaml
- packages/system/gateway-api/Chart.yaml
- packages/system/gateway-api/Makefile
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
[error] 4-4: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (3)
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml (3)
1-3: Verify FluxCD API version compatibility
Ensure thatapiVersion: helm.toolkit.fluxcd.io/v2matches the FluxCD HelmRelease CRD installed in your cluster (e.g., some versions usev2beta1). A mismatch will cause CRD validation failures.
4-4: Ignore YAMLlint false positive on Go templates
The syntax error reported by YAMLlint at this line is due to Go templating delimiters ({{ … }}) and does not affect FluxCD rendering.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 4-4: syntax error: expected , but found ''
(syntax)
5-7: Label correctness
The labelscozystack.io/repository: systemandcozystack.io/target-cluster-name: {{ .Release.Name }}are correctly spelled and namespaced.
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @zdenekjanda, can we add gatewayAPI option into values, to make this component optional?
packages/apps/kubernetes/templates/helmreleases/gateway-api.yaml
Outdated
Show resolved
Hide resolved
|
All merged in, thanks for improvements it looks good now to me. |
Signed-off-by: Zdenek Deu Janda <zdenek.janda@cloudevelops.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
@zdenekjanda many thanks for your contribution! |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml (2)
11-11: Consider using a dynamic releaseName.Hardcoding
releaseName: gateway-api-crdsmay conflict if multiple clusters share the same storage namespace. For consistency withmetadata.name, consider:-releaseName: gateway-api-crds +releaseName: {{ .Release.Name }}-gateway-api-crds
33-37: EnsuredependsOnindentation is correct.The
dependsOn:block must have its list items properly indented in the rendered YAML (dependsOn:at one level, items two spaces deeper). Misalignment can cause the dependency to be ignored.
🛑 Comments failed to post (1)
packages/apps/kubernetes/templates/helmreleases/delete.yaml (1)
33-33:
⚠️ Potential issueMissing RBAC permission for
gateway-api-crds.You added
{{ .Release.Name }}-gateway-api-crdsto the patch command, but the Role’sresourceNameslist does not include it. Without that entry, the pre-delete hook cannot suspend this HelmRelease. Please update the Role to grant patch rights on{{ .Release.Name }}-gateway-api-crds.Suggested diff:
--- a/packages/apps/kubernetes/templates/helmreleases/delete.yaml +++ b/packages/apps/kubernetes/templates/helmreleases/delete.yaml @@ resourceNames: - - {{ .Release.Name }}-cilium + - {{ .Release.Name }}-cilium + - {{ .Release.Name }}-gateway-api-crds - {{ .Release.Name }}-csi📝 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.@@ resourceNames: - - {{ .Release.Name }}-cilium + - {{ .Release.Name }}-cilium + - {{ .Release.Name }}-gateway-api-crds - {{ .Release.Name }}-csi
Implementation of Cilium Gateway API <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added optional Gateway API addon for Kubernetes clusters, controlled by a new configuration flag. - Introduced automated deployment of Gateway API CRDs when the addon is enabled. - **Documentation** - Updated documentation to describe the new Gateway API addon and its configuration. - **Chores** - Added chart metadata and automation files for managing Gateway API CRDs. - Updated chart version to reflect new features. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit ae05d2f) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Implementation of Cilium Gateway API
Summary by CodeRabbit