[cilium] Update cilium to 1.19.1 - #2173
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR upgrades the Cilium Helm chart from version 1.18.6 to 1.19.1. It includes version bumps across build and chart files, introduces extensive new configuration options for cloud provider nodeSpecs, standalone DNS proxy, MCS API support, and adds numerous extension points via templated include blocks for customization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Failed to read Trivy output file: ENOENT: no such file or directory, open '/inmem/1254/nsjail-5a79ddfc-e9d7-4279-add5-ec126f9b6b8b/merged/.trivy-output.json' Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
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 primarily focuses on upgrading the Cilium system package to version 1.19.1, bringing a suite of enhancements across various components. It introduces new configuration parameters for cloud provider integrations, advanced networking features like BIG TCP and policy deny responses, and significant improvements to ClusterMesh and Multi-Cluster Services API support. The update also refines security settings, expands observability options with detailed profiling, and modernizes Kubernetes API interactions, including the introduction of a standalone DNS proxy. 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
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 (@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
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the Cilium system package to version 1.19.1. The changes are extensive, reflecting the new features, configuration updates, and deprecations in the new version. The update seems to be handled correctly across the various configuration files, templates, and documentation. I've added a couple of suggestions to improve reproducibility and align with Kubernetes best practices. Overall, the changes look solid.
Note: Security Review did not run due to the size of the PR.
Artem Bortnikov (BROngineer)
force-pushed
the
artem/cilium-v1.19.1
branch
from
March 10, 2026 08:55
6485a1d to
1169407
Compare
Contributor
Author
|
e2e fails due to |
Signed-off-by: Artem Bortnikov <brongineer747@gmail.com>
Andrei Kvapil (kvaps)
force-pushed
the
artem/cilium-v1.19.1
branch
from
March 10, 2026 11:42
1169407 to
748f814
Compare
Artem Bortnikov (BROngineer)
marked this pull request as ready for review
March 10, 2026 11:44
Closed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/system/cilium/charts/cilium/templates/cilium-agent/role.yaml (1)
123-145:⚠️ Potential issue | 🟠 MajorKeep this Role change out of the vendored upstream template.
Like the paired RoleBinding, this is a direct modification under
charts/. That makes the customization fragile across future Cilium bumps. Please apply it via the repo’s vendoring flow/overlay rather than editing the vendored chart source directly.As per coding guidelines "Do NOT directly edit upstream Helm charts in vendored charts directory; use proper chart vendoring mechanisms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cilium/charts/cilium/templates/cilium-agent/role.yaml` around lines 123 - 145, The Role resource "cilium-tlsinterception-secrets" was added directly into the vendored Cilium chart template (role.yaml) which violates the guideline to not edit upstream charts; revert/remove this Role addition from the vendored template and instead create a patch/overlay in the repo's chart vendoring flow (e.g., add a Helm chart overlay/patch or use the repo's vendor overlay mechanism) that injects the Role and its RoleBinding as needed based on the same conditionals (.Values.agent, .Values.preflight.enabled, .Values.serviceAccounts.cilium.create, $readSecretsOnlyFromSecretsNamespace, .Values.tls.secretsNamespace.name), ensuring the change is applied outside the vendored chart source and tracked by the vendor/overlay system.packages/system/cilium/charts/cilium/templates/cilium-agent/rolebinding.yaml (1)
129-149:⚠️ Potential issue | 🟠 MajorAvoid carrying this change as a direct edit in the vendored chart.
This template sits under the vendored
charts/tree, so local RBAC changes here are likely to be overwritten on the next Cilium vendor refresh. Please move this through the chart vendoring/patching mechanism used for system packages instead of patching the upstream template in place.As per coding guidelines "Do NOT directly edit upstream Helm charts in vendored charts directory; use proper chart vendoring mechanisms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cilium/charts/cilium/templates/cilium-agent/rolebinding.yaml` around lines 129 - 149, The change modifies the vendored Helm template that creates the RoleBinding named cilium-tlsinterception-secrets in the cilium-agent rolebinding.yaml (the block using .Values.tls.secretsNamespace.name, .Values.serviceAccounts.cilium.create and .Values.preflight.enabled), which must not be edited in-place; revert this edit in the vendored charts tree and instead implement the RBAC change via the repository's chart vendoring/patch mechanism (create a chart patch/overlay or use the system package’s chart patch workflow to add or modify the RoleBinding template) so the adjustment is applied outside the upstream vendored files and survives future vendor refreshes.packages/system/cilium/charts/cilium/templates/cilium-ingress-service.yaml (1)
57-63:⚠️ Potential issue | 🔴 CriticalMove this template customization to the umbrella chart using proper chart vendoring mechanisms, and add required EndpointSlice labels.
The
cilium-ingress-service.yamlfile is part of the upstream Cilium chart and should not be directly edited in the vendored directory. Per the coding guidelines, use proper chart vendoring mechanisms (e.g., Helm patches or custom chart templates in the umbrella layer).Additionally, the manually managed
EndpointSliceis missing critical labels required for Service association:kubernetes.io/service-nameandendpointslice.kubernetes.io/managed-by. Without the first label, Kubernetes will not associate this slice with the Service. The second label is required by Kubernetes for manually managed slices.Suggested label additions (if keeping template here)
metadata: name: {{ .Values.ingressController.service.name }} namespace: {{ include "cilium.namespace" . }} labels: + kubernetes.io/service-name: {{ .Values.ingressController.service.name }} + endpointslice.kubernetes.io/managed-by: cilium {{- with .Values.commonLabels }} {{- toYaml . | nindent 4 }} {{- end }} {{- if .Values.ingressController.service.labels }} {{- toYaml .Values.ingressController.service.labels | nindent 4 }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cilium/charts/cilium/templates/cilium-ingress-service.yaml` around lines 57 - 63, This vendored change edits the upstream template cilium-ingress-service.yaml's labels block directly; instead move your customization into the umbrella chart via proper chart vendoring/patching (e.g., Helm chart dependency, chart CR/patch, or umbrella templates) rather than modifying the upstream file, and if you must manage an EndpointSlice template add the required labels kubernetes.io/service-name=<service-name> and endpointslice.kubernetes.io/managed-by=manual (or your manager id) so Kubernetes will associate and accept a manually managed EndpointSlice; locate the labels block in cilium-ingress-service.yaml and any EndpointSlice template references to apply the label additions or implement the change as a patch in the umbrella chart.
🤖 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/cilium/charts/cilium/templates/cilium-configmap.yaml`:
- Around line 763-777: The ConfigMap renders duplicate keys for
enable-encryption-strict-mode-egress when both
.Values.encryption.strictMode.enabled and
.Values.encryption.strictMode.egress.enabled are set; update the first legacy
block that uses .Values.encryption.strictMode.* so it only runs when the newer
egress-specific path is not enabled (e.g., wrap the legacy block in an
additional condition like if not .Values.encryption.strictMode.egress.enabled),
keeping the later egress-specific block unchanged; reference the keys/values
enable-encryption-strict-mode-egress, encryption-strict-egress-cidr,
encryption-strict-egress-allow-remote-node-identities and the values paths
.Values.encryption.strictMode.enabled and
.Values.encryption.strictMode.egress.enabled to locate and modify the template.
- Line 1121: The template validates the legacy key before falling back, so
validateDuration receives an empty value and returns "0s", preventing fallback;
change the expression for hubble-export-aggregation-interval to apply default
first (i.e., take .Values.hubble.export.aggregationInterval | default
.Values.hubble.export.static.aggregationInterval) and then pipe that result into
include "validateDuration" so the static fallback is used when the legacy key is
absent; update the cilium-configmap template accordingly via the chart patching
mechanism rather than editing the vendored file directly, referencing the
hubble-export-aggregation-interval key and the include "validateDuration" call.
- Around line 1392-1394: The template emits
operator.unmanagedPodWatcher.selector unquoted causing empty string selectors to
become empty YAML values; fix by creating a patch
(quote-pod-restart-selector.patch) that updates the cilium template to quote
pod-restart-selector: "{{ .Values.operator.unmanagedPodWatcher.selector }}",
place that patch into the cilium patches directory per the repo vendoring
pattern, and modify the cilium Makefile update target to apply the patch (e.g.,
run patch --no-backup-if-mismatch -p3 <
patches/quote-pod-restart-selector.patch) during update so the vendored chart is
corrected without direct edits.
In
`@packages/system/cilium/charts/cilium/templates/cilium-operator/deployment.yaml`:
- Around line 190-193: The deployment still sets hostPort: 9234 for the "health"
container even when operator.hostNetwork is false; update the cilium-operator
deployment template to conditionally include the hostPort field only when
.Values.operator.hostNetwork is true (e.g., wrap or gate the hostPort: 9234 line
with an if .Values.operator.hostNetwork), leaving containerPort: 9234 and
existing probe behavior unchanged so non-hostNetwork pods do not reserve the
host port.
In
`@packages/system/cilium/charts/cilium/templates/cilium-secrets-namespace.yaml`:
- Around line 23-25: The added secretsNamespaceLabels block inside the vendored
Cilium template (cilium-secrets-namespace.yaml) must be removed to avoid local
drift; revert/delete the lines referencing secretsNamespaceLabels in the
templates under charts/cilium/templates and instead add the label customization
via the package’s chart overlay/vendoring mechanism (e.g., add the labels in
your package-level values.yaml or an overlay file that targets the upstream
cilium chart) so the change is preserved across upstream updates; ensure you
reference the same key name secretsNamespaceLabels in the overlay/values file so
the original template can consume the labels without modifying vendored files.
In
`@packages/system/cilium/charts/cilium/templates/clustermesh-apiserver/users-configmap.yaml`:
- Around line 1-3: The new check against .Values.clustermesh.config.enabled in
the top-level conditional of users-configmap.yaml causes the ConfigMap to no
longer render for existing clusters that never set that value; remove
.Values.clustermesh.config.enabled from the combined if-condition in the
template (i.e., revert the conditional to depend only on
.Values.clustermesh.useAPIServer,
.Values.clustermesh.apiserver.kvstoremesh.kvstoreMode == "internal",
identityAllocationCRD and the tls.authMode != "legacy"), or alternatively
replace the check with a safe default such as using the Helm default function
(e.g., default true .Values.clustermesh.config.enabled) so undefined values
preserve prior behavior; update users-configmap.yaml accordingly and ensure the
conditional still references the same symbols (.Values.clustermesh.useAPIServer,
.Values.clustermesh.apiserver.kvstoremesh.kvstoreMode, include
"identityAllocationCRD", and .Values.clustermesh.apiserver.tls.authMode).
In
`@packages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-clusterrole.yaml`:
- Around line 10-13: The annotations block is misindented: the template line
containing "{{- toYaml . | nindent 4 }}" has extra leading spaces causing a
6-space indent; adjust the template so the toYaml/nindent invocation is indented
exactly under "annotations:" (use the same 4-space level as labels) by removing
the extra leading whitespace around the "{{- toYaml . | nindent 4 }}" expression
in the .Values.clustermesh.annotations block so the generated YAML aligns
correctly.
In
`@packages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-clusterrolebinding.yaml`:
- Around line 10-13: The annotations block using .Values.clustermesh.annotations
produces a 6-space indent because the template directive has extra leading
whitespace; fix it by aligning the template line so the toYaml output is
indented exactly 4 spaces under the annotations: key (use the existing toYaml .
| nindent 4 but remove the extra leading spaces before the {{- ... }}), ensuring
the annotations block and the template directive have consistent indentation.
In
`@packages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job.yaml`:
- Line 6: The Job named cilium-coredns-mcsapi-autoconfig (the Helm hook with
annotations post-install,post-upgrade and ttlSecondsAfterFinished) needs a hook
delete policy to avoid AlreadyExists on upgrade; add a metadata.annotations
entry like "helm.sh/hook-delete-policy":
"before-hook-creation,hook-succeeded,hook-failed" (or at minimum
"before-hook-creation,hook-succeeded") so Helm will remove the previous hook Job
before creating the new one and after completion.
In
`@packages/system/cilium/charts/cilium/templates/hubble/tls-helm/metrics-server-secret.yaml`:
- Around line 14-24: You added Cozystack-specific labels/annotations directly
into the vendored Cilium template (the metrics-server-secret.yaml edits
inserting cilium.io/helm-template-non-idempotent and templating
.Values.commonLabels/.Values.hubble.annotations/.Values.nonIdempotentAnnotations);
revert those changes in the vendored file and instead add the custom
labels/annotations in the chart vendoring/umbrella layer (for example by
injecting them via the umbrella chart's values.yaml or a small patch/template in
the umbrella chart that merges into the Cilium release), ensuring you reference
the same keys (commonLabels, hubble.annotations, nonIdempotentAnnotations) so
behavior is preserved without editing the upstream vendored template.
In `@packages/system/cilium/Makefile`:
- Line 13: The helm pull command currently pins the chart to a minor version
("helm pull cilium/cilium --untar --untardir charts --version 1.19") which can
allow pulling different 1.19.x patches; update the command to the exact reviewed
patch release by changing the --version value to 1.19.1 so the vendored chart in
charts/ matches the reviewed PR (i.e., update the helm pull invocation to use
--version 1.19.1).
---
Outside diff comments:
In `@packages/system/cilium/charts/cilium/templates/cilium-agent/role.yaml`:
- Around line 123-145: The Role resource "cilium-tlsinterception-secrets" was
added directly into the vendored Cilium chart template (role.yaml) which
violates the guideline to not edit upstream charts; revert/remove this Role
addition from the vendored template and instead create a patch/overlay in the
repo's chart vendoring flow (e.g., add a Helm chart overlay/patch or use the
repo's vendor overlay mechanism) that injects the Role and its RoleBinding as
needed based on the same conditionals (.Values.agent, .Values.preflight.enabled,
.Values.serviceAccounts.cilium.create, $readSecretsOnlyFromSecretsNamespace,
.Values.tls.secretsNamespace.name), ensuring the change is applied outside the
vendored chart source and tracked by the vendor/overlay system.
In
`@packages/system/cilium/charts/cilium/templates/cilium-agent/rolebinding.yaml`:
- Around line 129-149: The change modifies the vendored Helm template that
creates the RoleBinding named cilium-tlsinterception-secrets in the cilium-agent
rolebinding.yaml (the block using .Values.tls.secretsNamespace.name,
.Values.serviceAccounts.cilium.create and .Values.preflight.enabled), which must
not be edited in-place; revert this edit in the vendored charts tree and instead
implement the RBAC change via the repository's chart vendoring/patch mechanism
(create a chart patch/overlay or use the system package’s chart patch workflow
to add or modify the RoleBinding template) so the adjustment is applied outside
the upstream vendored files and survives future vendor refreshes.
In `@packages/system/cilium/charts/cilium/templates/cilium-ingress-service.yaml`:
- Around line 57-63: This vendored change edits the upstream template
cilium-ingress-service.yaml's labels block directly; instead move your
customization into the umbrella chart via proper chart vendoring/patching (e.g.,
Helm chart dependency, chart CR/patch, or umbrella templates) rather than
modifying the upstream file, and if you must manage an EndpointSlice template
add the required labels kubernetes.io/service-name=<service-name> and
endpointslice.kubernetes.io/managed-by=manual (or your manager id) so Kubernetes
will associate and accept a manually managed EndpointSlice; locate the labels
block in cilium-ingress-service.yaml and any EndpointSlice template references
to apply the label additions or implement the change as a patch in the umbrella
chart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56d1a93b-aa3c-4c42-b379-07ffe0ed4156
📒 Files selected for processing (66)
packages/system/cilium/Makefilepackages/system/cilium/charts/cilium/Chart.yamlpackages/system/cilium/charts/cilium/README.mdpackages/system/cilium/charts/cilium/files/cilium-envoy/configmap/bootstrap-config.yamlpackages/system/cilium/charts/cilium/files/nodeinit/startup.bashpackages/system/cilium/charts/cilium/templates/_extensions.tplpackages/system/cilium/charts/cilium/templates/_helpers.tplpackages/system/cilium/charts/cilium/templates/cilium-agent/clusterrole.yamlpackages/system/cilium/charts/cilium/templates/cilium-agent/daemonset.yamlpackages/system/cilium/charts/cilium/templates/cilium-agent/role.yamlpackages/system/cilium/charts/cilium/templates/cilium-agent/rolebinding.yamlpackages/system/cilium/charts/cilium/templates/cilium-ca-secret.yamlpackages/system/cilium/charts/cilium/templates/cilium-configmap.yamlpackages/system/cilium/charts/cilium/templates/cilium-envoy/daemonset.yamlpackages/system/cilium/charts/cilium/templates/cilium-envoy/service.yamlpackages/system/cilium/charts/cilium/templates/cilium-ingress-service.yamlpackages/system/cilium/charts/cilium/templates/cilium-nodeinit/daemonset.yamlpackages/system/cilium/charts/cilium/templates/cilium-operator/clusterrole.yamlpackages/system/cilium/charts/cilium/templates/cilium-operator/deployment.yamlpackages/system/cilium/charts/cilium/templates/cilium-operator/role.yamlpackages/system/cilium/charts/cilium/templates/cilium-operator/rolebinding.yamlpackages/system/cilium/charts/cilium/templates/cilium-operator/secret.yamlpackages/system/cilium/charts/cilium/templates/cilium-preflight/clusterrole.yamlpackages/system/cilium/charts/cilium/templates/cilium-secrets-namespace.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/clusterrole.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/deployment.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/service.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-cronjob/_job-spec.tplpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-cronjob/cronjob.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-cronjob/job.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-cronjob/role.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-helm/admin-secret.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-helm/local-secret.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-helm/remote-secret.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-helm/server-secret.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-apiserver/users-configmap.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-config/_helpers.tplpackages/system/cilium/charts/cilium/templates/clustermesh-config/clustermesh-secret.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-config/kvstoremesh-secret.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/clusterrole.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/clusterrolebinding.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-clusterrole.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-clusterrolebinding.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-role.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-rolebinding.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-serviceaccount.yamlpackages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job.yamlpackages/system/cilium/charts/cilium/templates/hubble-relay/configmap.yamlpackages/system/cilium/charts/cilium/templates/hubble-ui/clusterrole.yamlpackages/system/cilium/charts/cilium/templates/hubble-ui/deployment.yamlpackages/system/cilium/charts/cilium/templates/hubble/tls-cronjob/_job-spec.tplpackages/system/cilium/charts/cilium/templates/hubble/tls-cronjob/cronjob.yamlpackages/system/cilium/charts/cilium/templates/hubble/tls-cronjob/job.yamlpackages/system/cilium/charts/cilium/templates/hubble/tls-helm/metrics-server-secret.yamlpackages/system/cilium/charts/cilium/templates/hubble/tls-helm/relay-client-secret.yamlpackages/system/cilium/charts/cilium/templates/hubble/tls-helm/relay-server-secret.yamlpackages/system/cilium/charts/cilium/templates/hubble/tls-helm/server-secret.yamlpackages/system/cilium/charts/cilium/templates/hubble/tls-helm/ui-client-certs.yamlpackages/system/cilium/charts/cilium/templates/standalone-dns-proxy/configmap.yamlpackages/system/cilium/charts/cilium/templates/standalone-dns-proxy/daemonset.yamlpackages/system/cilium/charts/cilium/templates/validate.yamlpackages/system/cilium/charts/cilium/values.schema.jsonpackages/system/cilium/charts/cilium/values.yamlpackages/system/cilium/charts/cilium/values.yaml.tmplpackages/system/cilium/images/cilium/Dockerfilepackages/system/cilium/values.yaml
💤 Files with no reviewable changes (4)
- packages/system/cilium/charts/cilium/templates/clustermesh-apiserver/tls-cronjob/role.yaml
- packages/system/cilium/charts/cilium/templates/cilium-agent/clusterrole.yaml
- packages/system/cilium/charts/cilium/templates/cilium-preflight/clusterrole.yaml
- packages/system/cilium/charts/cilium/templates/hubble-ui/clusterrole.yaml
Comment on lines
+763
to
777
| # --- DEPRECATED: Please use encryption.strictMode.egress.enabled instead | ||
| enable-encryption-strict-mode-egress: {{ .Values.encryption.strictMode.enabled | quote }} | ||
| encryption-strict-egress-cidr: {{ .Values.encryption.strictMode.cidr | quote }} | ||
| encryption-strict-egress-allow-remote-node-identities: {{ .Values.encryption.strictMode.allowRemoteNodeIdentities | quote }} | ||
| {{- end }} | ||
|
|
||
| encryption-strict-mode-cidr: {{ .Values.encryption.strictMode.cidr | quote }} | ||
| {{- if .Values.encryption.strictMode.ingress.enabled }} | ||
| enable-encryption-strict-mode-ingress: {{ .Values.encryption.strictMode.ingress.enabled | quote }} | ||
| {{- end }} | ||
|
|
||
| encryption-strict-mode-allow-remote-node-identities: {{ .Values.encryption.strictMode.allowRemoteNodeIdentities | quote }} | ||
| {{- if .Values.encryption.strictMode.egress.enabled }} | ||
| enable-encryption-strict-mode-egress: {{ .Values.encryption.strictMode.egress.enabled | quote }} | ||
| encryption-strict-egress-cidr: {{ .Values.encryption.strictMode.egress.cidr | quote }} | ||
| encryption-strict-egress-allow-remote-node-identities: {{ .Values.encryption.strictMode.egress.allowRemoteNodeIdentities | quote }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the specific lines from the file to verify the code snippet
sed -n '763,777p' packages/system/cilium/charts/cilium/templates/cilium-configmap.yamlRepository: cozystack/cozystack
Length of output: 1008
🏁 Script executed:
# Also get broader context around these lines to understand the template logic
sed -n '755,785p' packages/system/cilium/charts/cilium/templates/cilium-configmap.yamlRepository: cozystack/cozystack
Length of output: 1611
🏁 Script executed:
# Check the file size and structure
wc -l packages/system/cilium/charts/cilium/templates/cilium-configmap.yamlRepository: cozystack/cozystack
Length of output: 136
🌐 Web query:
Cilium 1.19 encryption strictMode deprecation egress upgrade
💡 Result:
In Cilium 1.19, the “encryption strictMode” settings were renamed to be egress-specific, and the old (non-egress) options are deprecated and scheduled for removal in Cilium 1.20. [1]
What’s deprecated (1.19) → what to use instead
Agent flags
--enable-encryption-strict-mode→--enable-encryption-strict-mode-egress[1]--encryption-strict-mode-cidr→--encryption-strict-egress-cidr[1]--encryption-strict-mode-allow-remote-node-identities→--encryption-strict-egress-allow-remote-node-identities[1]
Helm values
encryption.strictMode.enabled→encryption.strictMode.egress.enabled[1][2]encryption.strictMode.cidr→encryption.strictMode.egress.cidr[1][2]encryption.strictMode.allowRemoteNodeIdentities→encryption.strictMode.egress.allowRemoteNodeIdentities[1][2]
Egress strict mode behavior (what it does)
Set:
encryption.strictMode.egress.enabled=trueencryption.strictMode.egress.cidr=<your IPv4 pod CIDR>
This enforces that traffic leaving a node toward that CIDR must be encrypted (otherwise it’s dropped), with limitations like IPv4-only coverage and potential need for allowRemoteNodeIdentities depending on routing/tunneling and CIDR overlap. [3]
Related: ingress strict mode (new separate knob)
There is also encryption.strictMode.ingress.enabled (distinct from egress); it drops unencrypted overlay ingress traffic and is only applicable in specific WireGuard+tunneling setups. [2][3]
Sources: Upgrade guide (1.19) [1], Helm reference (1.19) [2], Transparent Encryption docs (1.19) [3].
Prevent duplicate enable-encryption-strict-mode-egress keys during Cilium 1.19 migration.
Cilium 1.19 deprecates encryption.strictMode.* in favor of encryption.strictMode.egress.*. When both are enabled during migration, lines 763–776 emit the same ConfigMap keys twice, making the rendered output ambiguous.
Add a mutual-exclusion check to ensure only one path is active:
Proposed fix
-{{- if .Values.encryption.strictMode.enabled }}
+{{- if and .Values.encryption.strictMode.enabled .Values.encryption.strictMode.egress.enabled }}
+ {{- fail "Use either encryption.strictMode.* or encryption.strictMode.egress.*, not both." }}
+{{- else if .Values.encryption.strictMode.enabled }}
# --- DEPRECATED: Please use encryption.strictMode.egress.enabled instead
enable-encryption-strict-mode-egress: {{ .Values.encryption.strictMode.enabled | quote }}
encryption-strict-egress-cidr: {{ .Values.encryption.strictMode.cidr | quote }}
encryption-strict-egress-allow-remote-node-identities: {{ .Values.encryption.strictMode.allowRemoteNodeIdentities | quote }}
{{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/cilium/charts/cilium/templates/cilium-configmap.yaml` around
lines 763 - 777, The ConfigMap renders duplicate keys for
enable-encryption-strict-mode-egress when both
.Values.encryption.strictMode.enabled and
.Values.encryption.strictMode.egress.enabled are set; update the first legacy
block that uses .Values.encryption.strictMode.* so it only runs when the newer
egress-specific path is not enabled (e.g., wrap the legacy block in an
additional condition like if not .Values.encryption.strictMode.egress.enabled),
keeping the later egress-specific block unchanged; reference the keys/values
enable-encryption-strict-mode-egress, encryption-strict-egress-cidr,
encryption-strict-egress-allow-remote-node-identities and the values paths
.Values.encryption.strictMode.enabled and
.Values.encryption.strictMode.egress.enabled to locate and modify the template.
| hubble-export-file-max-size-mb: {{ .Values.hubble.export.fileMaxSizeMb | default .Values.hubble.export.static.fileMaxSizeMb | quote }} | ||
| hubble-export-file-max-backups: {{ .Values.hubble.export.fileMaxBackups | default .Values.hubble.export.static.fileMaxBackups | quote }} | ||
| hubble-export-file-compress: {{ .Values.hubble.export.fileCompress | default .Values.hubble.export.static.fileCompress | quote }} | ||
| hubble-export-aggregation-interval: {{ include "validateDuration" .Values.hubble.export.aggregationInterval | default .Values.hubble.export.static.aggregationInterval | quote }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and whether this is a vendored chart
cd packages/system/cilium && find . -name "*.yaml" -o -name "*.yml" | head -30Repository: cozystack/cozystack
Length of output: 1708
🏁 Script executed:
# Check if this is indeed a vendored chart directory structure
ls -la packages/system/cilium/charts/cilium/ | head -20Repository: cozystack/cozystack
Length of output: 823
🏁 Script executed:
# Look for the validateDuration function definition
rg "validateDuration" packages/system/cilium/ -A 5Repository: cozystack/cozystack
Length of output: 10575
🏁 Script executed:
# Check the values.yaml structure
head -200 packages/system/cilium/charts/cilium/values.yaml | tail -100Repository: cozystack/cozystack
Length of output: 3309
🏁 Script executed:
# Search for hubble export configuration in values
rg "hubble:" packages/system/cilium/charts/cilium/values.yaml -A 30 | head -60Repository: cozystack/cozystack
Length of output: 1183
🏁 Script executed:
# Search for hubble export configuration in values.yaml
rg "hubble:" packages/system/cilium/charts/cilium/values.yaml -A 100 | grep -A 50 "export:"Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check Chart.yaml to understand dependencies/vendoring
cat packages/system/cilium/Chart.yamlRepository: cozystack/cozystack
Length of output: 195
🏁 Script executed:
# Read the _helpers.tpl file to see validateDuration completely
cat -n packages/system/cilium/charts/cilium/templates/_helpers.tpl | sed -n '1,50p'Repository: cozystack/cozystack
Length of output: 2065
🏁 Script executed:
# Search more carefully for export configuration
rg "export:" packages/system/cilium/charts/cilium/values.yaml -B 2 -A 30Repository: cozystack/cozystack
Length of output: 1191
🏁 Script executed:
# Check if aggregationInterval exists under hubble.export
rg "aggregationInterval" packages/system/cilium/charts/cilium/values.yaml -B 5 -A 5Repository: cozystack/cozystack
Length of output: 911
🏁 Script executed:
# Check the parent Chart.yaml for dependencies
cat packages/system/cilium/Chart.yaml | head -50Repository: cozystack/cozystack
Length of output: 195
🏁 Script executed:
# Look at the directory structure of packages/system to understand vendoring pattern
ls -la packages/system/cilium/Repository: cozystack/cozystack
Length of output: 748
🏁 Script executed:
# Check if there's a top-level hubble.export.aggregationInterval in values.yaml
rg "aggregationInterval" packages/system/cilium/charts/cilium/ -B 3 -A 1 | grep -E "(aggregationInterval|hubble)" | head -20Repository: cozystack/cozystack
Length of output: 4252
🏁 Script executed:
# Check the actual structure to see all paths with aggregationInterval
rg "aggregationInterval" packages/system/cilium/ -B 2Repository: cozystack/cozystack
Length of output: 5754
🏁 Script executed:
# Verify the umbrella chart structure - see how cozy-cilium references charts/cilium
cat packages/system/cilium/templates/Chart.yaml 2>/dev/null || echo "No templates in umbrella"
ls -la packages/system/cilium/templates/Repository: cozystack/cozystack
Length of output: 253
🏁 Script executed:
# Check if there's any documentation or mechanism for chart vendoring
find packages/system/cilium -name "*.md" -o -name "Makefile" | xargs head -30 2>/dev/nullRepository: cozystack/cozystack
Length of output: 3462
🏁 Script executed:
# Check if this is the actual upstream cilium chart by looking at the version
cat packages/system/cilium/charts/cilium/Chart.yaml | head -20Repository: cozystack/cozystack
Length of output: 1740
🏁 Script executed:
# Look at the other files in the cilium configmap to understand the pattern
sed -n '1100,1130p' packages/system/cilium/charts/cilium/templates/cilium-configmap.yamlRepository: cozystack/cozystack
Length of output: 1983
🏁 Script executed:
# Verify the complete validateDuration function
sed -n '/define "validateDuration"/,/end/p' packages/system/cilium/charts/cilium/templates/_helpers.tplRepository: cozystack/cozystack
Length of output: 190
🏁 Script executed:
# Check git history to see if this file was modified from upstream
cd packages/system/cilium/charts/cilium && git log --oneline templates/cilium-configmap.yaml 2>/dev/null | head -5 || echo "No git history available"Repository: cozystack/cozystack
Length of output: 102
🏁 Script executed:
# Compare similar patterns in the file to understand the intended fallback behavior
grep -n "default .Values.hubble.export" packages/system/cilium/charts/cilium/templates/cilium-configmap.yamlRepository: cozystack/cozystack
Length of output: 670
🏁 Script executed:
# Check if this specific bug exists in the upstream cilium v1.19.1 chart
web_search
Search for cilium helm chart v1.19.1 hubble export aggregationInterval issueRepository: cozystack/cozystack
Length of output: 158
🏁 Script executed:
# Try to see if we can compare versions or check upstream
rg "hubble-export-aggregation-interval" packages/system/cilium/charts/cilium/templates/ -B 2 -A 2Repository: cozystack/cozystack
Length of output: 1028
🏁 Script executed:
# Verify if this line exists unchanged in upstream cilium 1.19.1 by checking if there were any local modifications
cd packages/system/cilium/charts/cilium && git diff HEAD -- templates/cilium-configmap.yaml 2>/dev/null | head -50 || echo "No local diff"Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check if there's a patches directory or overlay mechanism for the vendored chart
find packages/system/cilium -name "*.patch" -o -name "kustomization.yaml" -o -name "*patch*" 2>/dev/nullRepository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check if there's documentation about how to patch vendored charts in this repo
find packages -name "README*" -o -name "CONTRIBUTING*" | xargs grep -l "patch\|vendor\|chart" 2>/dev/null | head -3Repository: cozystack/cozystack
Length of output: 203
🏁 Script executed:
# Check the hack directory for common patterns
ls packages/system/cilium/
cat packages/system/cilium/values.yaml 2>/dev/null | head -5Repository: cozystack/cozystack
Length of output: 254
🏁 Script executed:
# Check if umbrella chart at packages/system/cilium/values.yaml can override nested values
cat packages/system/cilium/values.yaml | grep -A 20 "cilium:"Repository: cozystack/cozystack
Length of output: 510
🏁 Script executed:
# Verify the structure - how overrides work in the umbrella chart
cat packages/system/cilium/Chart.yamlRepository: cozystack/cozystack
Length of output: 195
Apply default before validateDuration to fix the aggregation interval fallback.
Line 1121 validates the legacy key before falling back. Since .Values.hubble.export.aggregationInterval doesn't exist in values.yaml and validateDuration returns "0s" for empty input, the fallback to .Values.hubble.export.static.aggregationInterval never applies. Any non-default static aggregation interval gets rendered as "0s".
Note: This file is part of a vendored upstream chart. Apply the fix through proper chart patching mechanisms rather than direct edits.
Proposed fix
- hubble-export-aggregation-interval: {{ include "validateDuration" .Values.hubble.export.aggregationInterval | default .Values.hubble.export.static.aggregationInterval | quote }}
+ hubble-export-aggregation-interval: {{ include "validateDuration" (.Values.hubble.export.aggregationInterval | default .Values.hubble.export.static.aggregationInterval) | quote }}📝 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.
Suggested change
| hubble-export-aggregation-interval: {{ include "validateDuration" .Values.hubble.export.aggregationInterval | default .Values.hubble.export.static.aggregationInterval | quote }} | |
| hubble-export-aggregation-interval: {{ include "validateDuration" (.Values.hubble.export.aggregationInterval | default .Values.hubble.export.static.aggregationInterval) | quote }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/cilium/charts/cilium/templates/cilium-configmap.yaml` at line
1121, The template validates the legacy key before falling back, so
validateDuration receives an empty value and returns "0s", preventing fallback;
change the expression for hubble-export-aggregation-interval to apply default
first (i.e., take .Values.hubble.export.aggregationInterval | default
.Values.hubble.export.static.aggregationInterval) and then pipe that result into
include "validateDuration" so the static fallback is used when the legacy key is
absent; update the cilium-configmap template accordingly via the chart patching
mechanism rather than editing the vendored file directly, referencing the
hubble-export-aggregation-interval key and the include "validateDuration" call.
Comment on lines
+1392
to
+1394
| {{- if ne .Values.operator.unmanagedPodWatcher.selector nil }} | ||
| pod-restart-selector: {{ .Values.operator.unmanagedPodWatcher.selector }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the content around lines 1392-1394
fd "cilium-configmap.yaml" packages/Repository: cozystack/cozystack
Length of output: 131
🏁 Script executed:
# If found, check the content around the specified lines
if [ -f "packages/system/cilium/charts/cilium/templates/cilium-configmap.yaml" ]; then
echo "=== File exists. Showing lines 1390-1396 ==="
sed -n '1390,1396p' "packages/system/cilium/charts/cilium/templates/cilium-configmap.yaml"
fiRepository: cozystack/cozystack
Length of output: 297
🏁 Script executed:
# Check if this is a vendored chart by looking for upstream references or vendor markers
ls -la packages/system/cilium/charts/cilium/ | head -20Repository: cozystack/cozystack
Length of output: 823
🏁 Script executed:
# Search for pod-restart-selector in the codebase to understand its usage
rg "pod-restart-selector" --max-count 5 -B 2 -A 2Repository: cozystack/cozystack
Length of output: 559
🏁 Script executed:
# Check if this is a vendored chart by looking for version info or upstream references
cat packages/system/cilium/charts/cilium/Chart.yaml | head -20Repository: cozystack/cozystack
Length of output: 1740
🏁 Script executed:
# Check the values.yaml to see how operator.unmanagedPodWatcher.selector is documented
rg "unmanagedPodWatcher" packages/system/cilium/charts/cilium/values.yaml -B 3 -A 3Repository: cozystack/cozystack
Length of output: 395
🏁 Script executed:
# Check if there's a chart vendoring mechanism or upstream reference in the parent directory
ls -la packages/system/cilium/Repository: cozystack/cozystack
Length of output: 748
🏁 Script executed:
# Check the parent Chart.yaml to understand the chart structure
cat packages/system/cilium/Chart.yamlRepository: cozystack/cozystack
Length of output: 195
🏁 Script executed:
# Check the Makefile to see if there's a vendoring mechanism
cat packages/system/cilium/MakefileRepository: cozystack/cozystack
Length of output: 1343
🏁 Script executed:
# Look for the upstream reference mentioned in the review
rg "v1.19.1" packages/system/cilium/Repository: cozystack/cozystack
Length of output: 2323
🏁 Script executed:
# Get more context on the unmanagedPodWatcher.selector definition
rg "unmanagedPodWatcher" packages/system/cilium/charts/cilium/values.yaml -A 10Repository: cozystack/cozystack
Length of output: 527
🏁 Script executed:
# Check if there are any examples of how the project handles fixes to vendored charts
# Look for overrides or patches in the umbrella chart
find packages/system/cilium -name "*.yaml" -type f | head -20Repository: cozystack/cozystack
Length of output: 1539
🏁 Script executed:
# Check if there's a standard pattern for overriding vendored chart templates
ls -la packages/system/cilium/templates/Repository: cozystack/cozystack
Length of output: 228
🏁 Script executed:
# Check if there's a chart override or patching mechanism in the parent chart
cat packages/system/cilium/templates/cni.yamlRepository: cozystack/cozystack
Length of output: 705
🏁 Script executed:
# Verify this is indeed the vendored chart location and check if there's a .git or vendor marker
git log --oneline -1 packages/system/cilium/charts/cilium/Chart.yamlRepository: cozystack/cozystack
Length of output: 102
🏁 Script executed:
# Check if there's a kustomize patch or helm hooks for overriding vendored chart templates
find packages/system/cilium -name "*.patch" -o -name "*.ks.yaml" -o -name "*.overlay" -type fRepository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check the README or documentation about how to handle vendored chart fixes
cat packages/system/cilium/README.md 2>/dev/null || echo "No README found"Repository: cozystack/cozystack
Length of output: 78
🏁 Script executed:
# Verify the semantics: check what happens with empty string in the actual template render
# Look at the full cilium-configmap.yaml context to understand the YAML structure
sed -n '1385,1410p' packages/system/cilium/charts/cilium/templates/cilium-configmap.yamlRepository: cozystack/cozystack
Length of output: 1166
🏁 Script executed:
# Check if there's any documentation or pattern on how to handle patches to vendored charts
find packages/system -name "*.patch" -type f | head -10Repository: cozystack/cozystack
Length of output: 581
🏁 Script executed:
# Check the Makefile in the parent system directory for any patch mechanisms
cat packages/Makefile 2>/dev/null | head -50 || echo "No Makefile in packages/"Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Look at how other packages handle overrides to vendored charts
find packages -type f -name "*.yaml" -path "*/templates/*" | head -20Repository: cozystack/cozystack
Length of output: 1120
🏁 Script executed:
# Check if there's already a patches directory for cilium
ls -la packages/system/cilium/patches/ 2>/dev/null || echo "No patches directory for cilium"Repository: cozystack/cozystack
Length of output: 94
🏁 Script executed:
# Check one of the existing patch files to understand the format
cat packages/system/ingress-nginx/patches/add-metrics2.patchRepository: cozystack/cozystack
Length of output: 1582
🏁 Script executed:
# Verify the exact line number and content in cilium-configmap.yaml
grep -n "pod-restart-selector" packages/system/cilium/charts/cilium/templates/cilium-configmap.yamlRepository: cozystack/cozystack
Length of output: 143
🏁 Script executed:
# Check if there's a mechanism to apply patches to vendored charts
# Look for references in Makefile or CI/CD
find packages/system -name "Makefile" -type f | xargs grep -l "patch" 2>/dev/nullRepository: cozystack/cozystack
Length of output: 303
🏁 Script executed:
# Check the hack/package.mk to understand the build process
cat hack/package.mk | grep -A 10 -B 5 "patch\|vendoring" || echo "No patch references found"Repository: cozystack/cozystack
Length of output: 88
🏁 Script executed:
# Check how ingress-nginx applies patches to understand the pattern
cat packages/system/ingress-nginx/MakefileRepository: cozystack/cozystack
Length of output: 565
🏁 Script executed:
# Verify cilium's Makefile has the update target but no patches yet
cat packages/system/cilium/MakefileRepository: cozystack/cozystack
Length of output: 1343
Apply the quoting fix via a patch file, not by directly editing the vendored chart.
The issue is valid: operator.unmanagedPodWatcher.selector documents an empty string as the way to select all pods, but line 1393 emits the value unquoted, so an empty selector becomes an empty YAML value instead of an empty string. However, per the repository's chart vendoring pattern, this fix must be applied as a patch in the packages/system/cilium/patches/ directory and invoked in the Makefile update target, not by directly editing the vendored chart template.
Suggested approach
Create packages/system/cilium/patches/quote-pod-restart-selector.patch with:
--- a/charts/cilium/templates/cilium-configmap.yaml
+++ b/charts/cilium/templates/cilium-configmap.yaml
@@ -1,2 +1,2 @@
{{- if ne .Values.operator.unmanagedPodWatcher.selector nil }}
- pod-restart-selector: {{ .Values.operator.unmanagedPodWatcher.selector }}
+ pod-restart-selector: {{ .Values.operator.unmanagedPodWatcher.selector | quote }}
{{- end }}Then add to the update target in packages/system/cilium/Makefile:
patch --no-backup-if-mismatch -p3 < patches/quote-pod-restart-selector.patch
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/cilium/charts/cilium/templates/cilium-configmap.yaml` around
lines 1392 - 1394, The template emits operator.unmanagedPodWatcher.selector
unquoted causing empty string selectors to become empty YAML values; fix by
creating a patch (quote-pod-restart-selector.patch) that updates the cilium
template to quote pod-restart-selector: "{{
.Values.operator.unmanagedPodWatcher.selector }}", place that patch into the
cilium patches directory per the repo vendoring pattern, and modify the cilium
Makefile update target to apply the patch (e.g., run patch
--no-backup-if-mismatch -p3 < patches/quote-pod-restart-selector.patch) during
update so the vendored chart is corrected without direct edits.
Comment on lines
+190
to
+193
| - name: health | ||
| containerPort: 9234 | ||
| hostPort: 9234 | ||
| {{- if .Values.operator.prometheus.enabled }} |
Contributor
There was a problem hiding this comment.
Gate the health hostPort on operator.hostNetwork.
When operator.hostNetwork=false, Lines 190-193 still reserve host port 9234. The probes already switch to pod networking in that mode, so this only adds avoidable host-port conflicts and changes the non-hostNetwork behavior.
🛠️ Proposed fix
- name: health
containerPort: 9234
- hostPort: 9234
+ {{- if .Values.operator.hostNetwork }}
+ hostPort: 9234
+ {{- 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.
Suggested change
| - name: health | |
| containerPort: 9234 | |
| hostPort: 9234 | |
| {{- if .Values.operator.prometheus.enabled }} | |
| - name: health | |
| containerPort: 9234 | |
| {{- if .Values.operator.hostNetwork }} | |
| hostPort: 9234 | |
| {{- end }} | |
| {{- if .Values.operator.prometheus.enabled }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/cilium/charts/cilium/templates/cilium-operator/deployment.yaml`
around lines 190 - 193, The deployment still sets hostPort: 9234 for the
"health" container even when operator.hostNetwork is false; update the
cilium-operator deployment template to conditionally include the hostPort field
only when .Values.operator.hostNetwork is true (e.g., wrap or gate the hostPort:
9234 line with an if .Values.operator.hostNetwork), leaving containerPort: 9234
and existing probe behavior unchanged so non-hostNetwork pods do not reserve the
host port.
Comment on lines
+23
to
+25
| {{- with $.Values.secretsNamespaceLabels }} | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
Move this customization out of the vendored Cilium chart.
This template is inside the vendored upstream chart, so adding secretsNamespaceLabels here creates local drift and will be easy to lose on the next Cilium re-vendor/update. Please apply this via the package’s vendoring/overlay mechanism instead of editing charts/cilium/templates/* directly.
As per coding guidelines, packages/system/**: “Use Helm Chart umbrella pattern with vendored upstream charts in charts/ directory” and **/*.yaml: “Do NOT directly edit upstream Helm charts in vendored charts directory; use proper chart vendoring mechanisms”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/cilium/charts/cilium/templates/cilium-secrets-namespace.yaml`
around lines 23 - 25, The added secretsNamespaceLabels block inside the vendored
Cilium template (cilium-secrets-namespace.yaml) must be removed to avoid local
drift; revert/delete the lines referencing secretsNamespaceLabels in the
templates under charts/cilium/templates and instead add the label customization
via the package’s chart overlay/vendoring mechanism (e.g., add the labels in
your package-level values.yaml or an overlay file that targets the upstream
cilium chart) so the change is preserved across upstream updates; ensure you
reference the same key name secretsNamespaceLabels in the overlay/values file so
the original template can consume the labels without modifying vendored files.
Comment on lines
+10
to
+13
| {{- with .Values.clustermesh.annotations }} | ||
| annotations: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
Inconsistent indentation in annotations block.
Line 12 has extra leading whitespace before {{- toYaml . | nindent 4 }}, resulting in 6-space indentation for annotations content while labels use 4-space indentation (line 8). This will produce misaligned YAML output.
🔧 Proposed fix
{{- with .Values.clustermesh.annotations }}
annotations:
- {{- toYaml . | nindent 4 }}
- {{- end }}
+ {{- toYaml . | nindent 4 }}
+ {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-clusterrole.yaml`
around lines 10 - 13, The annotations block is misindented: the template line
containing "{{- toYaml . | nindent 4 }}" has extra leading spaces causing a
6-space indent; adjust the template so the toYaml/nindent invocation is indented
exactly under "annotations:" (use the same 4-space level as labels) by removing
the extra leading whitespace around the "{{- toYaml . | nindent 4 }}" expression
in the .Values.clustermesh.annotations block so the generated YAML aligns
correctly.
Comment on lines
+10
to
+13
| {{- with .Values.clustermesh.annotations }} | ||
| annotations: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
Inconsistent indentation in annotations block.
Same issue as in job-clusterrole.yaml - line 12 has extra leading whitespace producing 6-space indentation for annotations content.
🔧 Proposed fix
{{- with .Values.clustermesh.annotations }}
annotations:
- {{- toYaml . | nindent 4 }}
+ {{- toYaml . | nindent 4 }}
{{- 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.
Suggested change
| {{- with .Values.clustermesh.annotations }} | |
| annotations: | |
| {{- toYaml . | nindent 4 }} | |
| {{- end }} | |
| {{- with .Values.clustermesh.annotations }} | |
| annotations: | |
| {{- toYaml . | nindent 4 }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job-clusterrolebinding.yaml`
around lines 10 - 13, The annotations block using
.Values.clustermesh.annotations produces a 6-space indent because the template
directive has extra leading whitespace; fix it by aligning the template line so
the toYaml output is indented exactly 4 spaces under the annotations: key (use
the existing toYaml . | nindent 4 but remove the extra leading spaces before the
{{- ... }}), ensuring the annotations block and the template directive have
consistent indentation.
| apiVersion: batch/v1 | ||
| kind: Job | ||
| metadata: | ||
| name: cilium-coredns-mcsapi-autoconfig |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the official Helm chart hooks documentation, what happens when a hook resource with a fixed name already exists on a later upgrade, and how does helm.sh/hook-delete-policy: before-hook-creation affect that?
💡 Result:
- If a hook resource has a fixed name and it still exists when you run a later
helm upgrade, Helm will (by default) fail the upgrade because it tries to create the hook resource again and Kubernetes returns an"... already exists"error. [1] - Setting
helm.sh/hook-delete-policy: before-hook-creationmakes Helm delete the previous instance of that hook resource before creating/running the hook again, avoiding the name-collision failure. [1][2]
Sources: [1] Helm “Hooks” docs (official, charts_hooks) [2] Helm “Chart Hooks” docs (hook deletion policies)
Add a delete policy to this fixed-name hook Job.
With the stable name on line 6 and only post-install,post-upgrade annotations, a previous hook resource can still exist when the next upgrade starts. ttlSecondsAfterFinished is asynchronous and does not prevent Helm from immediately attempting to recreate the hook, causing an AlreadyExists error on upgrade.
Suggested fix
annotations:
"helm.sh/hook": post-install,post-upgrade
+ "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded
{{- with .Values.clustermesh.annotations }}
{{- toYaml . | nindent 4 }}
{{- 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.
Suggested change
| name: cilium-coredns-mcsapi-autoconfig | |
| annotations: | |
| "helm.sh/hook": post-install,post-upgrade | |
| "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded | |
| {{- with .Values.clustermesh.annotations }} | |
| {{- toYaml . | nindent 4 }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/cilium/charts/cilium/templates/clustermesh-coredns-mcsapi/job.yaml`
at line 6, The Job named cilium-coredns-mcsapi-autoconfig (the Helm hook with
annotations post-install,post-upgrade and ttlSecondsAfterFinished) needs a hook
delete policy to avoid AlreadyExists on upgrade; add a metadata.annotations
entry like "helm.sh/hook-delete-policy":
"before-hook-creation,hook-succeeded,hook-failed" (or at minimum
"before-hook-creation,hook-succeeded") so Helm will remove the previous hook Job
before creating the new one and after completion.
Comment on lines
+14
to
24
| {{- with .Values.commonLabels }} | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| cilium.io/helm-template-non-idempotent: "true" | ||
|
|
||
| {{- with .Values.hubble.annotations }} | ||
| annotations: | ||
| {{- with .Values.hubble.annotations }} | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| {{- with .Values.nonIdempotentAnnotations }} | ||
| {{- toYaml . | nindent 4 }} |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid patching the vendored Cilium chart in-place.
This file is inside packages/system/cilium/charts/cilium/..., so adding Cozystack-specific labels/annotations here creates local drift that will be overwritten or need to be re-applied on every upstream chart bump. Please move this customization into the chart vendoring/umbrella layer instead of editing the vendored template directly.
As per coding guidelines, **/*.yaml: "Do NOT directly edit upstream Helm charts in vendored charts directory; use proper chart vendoring mechanisms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/system/cilium/charts/cilium/templates/hubble/tls-helm/metrics-server-secret.yaml`
around lines 14 - 24, You added Cozystack-specific labels/annotations directly
into the vendored Cilium template (the metrics-server-secret.yaml edits
inserting cilium.io/helm-template-non-idempotent and templating
.Values.commonLabels/.Values.hubble.annotations/.Values.nonIdempotentAnnotations);
revert those changes in the vendored file and instead add the custom
labels/annotations in the chart vendoring/umbrella layer (for example by
injecting them via the umbrella chart's values.yaml or a small patch/template in
the umbrella chart that merges into the Cilium release), ensuring you reference
the same keys (commonLabels, hubble.annotations, nonIdempotentAnnotations) so
behavior is preserved without editing the upstream vendored template.
| helm repo add cilium https://helm.cilium.io/ | ||
| helm repo update cilium | ||
| helm pull cilium/cilium --untar --untardir charts --version 1.18 | ||
| helm pull cilium/cilium --untar --untardir charts --version 1.19 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Vendoring command:"
sed -n '11,14p' packages/system/cilium/Makefile
echo
echo "Vendored chart version:"
sed -n '/^version:/p' packages/system/cilium/charts/cilium/Chart.yamlRepository: cozystack/cozystack
Length of output: 345
Pin the vendored chart to the reviewed patch release.
Line 13 uses --version 1.19, while this PR vendors 1.19.1. A later make update can pull a different 1.19.x chart into charts/ than the one reviewed here.
Proposed fix
+CILIUM_CHART_VERSION=1.19.1
CILIUM_TAG=$(shell awk '$$1 == "version:" {print $$2}' charts/cilium/Chart.yaml)
@@
- helm pull cilium/cilium --untar --untardir charts --version 1.19
+ helm pull cilium/cilium --untar --untardir charts --version $(CILIUM_CHART_VERSION)📝 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.
Suggested change
| helm pull cilium/cilium --untar --untardir charts --version 1.19 | |
| CILIUM_CHART_VERSION=1.19.1 | |
| CILIUM_TAG=$(shell awk '$$1 == "version:" {print $$2}' charts/cilium/Chart.yaml) | |
| helm pull cilium/cilium --untar --untardir charts --version $(CILIUM_CHART_VERSION) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/system/cilium/Makefile` at line 13, The helm pull command currently
pins the chart to a minor version ("helm pull cilium/cilium --untar --untardir
charts --version 1.19") which can allow pulling different 1.19.x patches; update
the command to the exact reviewed patch release by changing the --version value
to 1.19.1 so the vendored chart in charts/ matches the reviewed PR (i.e., update
the helm pull invocation to use --version 1.19.1).
Andrei Kvapil (kvaps)
approved these changes
Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR updates the cilium system package to the version 1.19.1
Release note
Summary by CodeRabbit
Release Notes
New Features
Improvements