feat(kubernetes,tenant): copy patch-containerd secret into tenant namespaces - #2927
feat(kubernetes,tenant): copy patch-containerd secret into tenant namespaces#2927Elaugaste (elaugaste) wants to merge 1 commit into
Conversation
…espaces Add a Helm template to the tenant chart that copies the patch-containerd secret from cozy-system into each newly created tenant namespace (when the secret exists). Update the kubernetes chart to consume the secret directly from Release.Namespace instead of maintaining its own copy. Remove the now-redundant copy-patch-containerd.yaml from the kubernetes chart — the secret is provided by the tenant chart. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Elaugaste <elaugaste@gmail.com>
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 refactors the handling of the patch-containerd secret by centralizing its replication within the tenant chart. By moving this logic, the secret is now automatically provisioned in each tenant namespace, allowing the kubernetes chart to reference it locally without needing release-specific naming conventions. Highlights
New Features🧠 You can now enable Memory (public preview) 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. 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 the 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 counterproductive. 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. Footnotes
|
📝 WalkthroughWalkthroughTwo Helm templates are updated to standardize the Changespatch-containerd Secret name standardization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Code Review
This pull request updates the kubernetes and tenant packages to use a static secret name 'patch-containerd' instead of prefixing it with the release name, and adjusts the target namespace for the copied secret. It also wraps the secret copying logic in copy-patch-containerd.yaml with a conditional check to exclude the 'tenant-root' tenant. Feedback includes a request to add the required release-note block to the PR description per repository guidelines, and a suggestion to use the more robust 'toYaml' and 'nindent' functions for serializing secret data instead of a manual loop.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -1,13 +1,16 @@ | |||
| {{- if ne (include "tenant.name" .) "tenant-root" }} | |||
There was a problem hiding this comment.
The PR body is missing the required ```release-note block as specified in the repository guidelines. Please update the PR description to include a valid release-note block to satisfy the CI/CD and release tracking requirements.
References
- PR body must contain a release note block:
release-note ...(link)
There was a problem hiding this comment.
fixed
| data: | ||
| {{- range $key, $value := $sourceSecret.data }} | ||
| {{ printf "%s: %s" $key ($value | quote) | indent 2 }} | ||
| {{ $key }}: {{ $value | quote }} | ||
| {{- end }} |
There was a problem hiding this comment.
Using a manual range loop to serialize map data can be fragile and prone to whitespace/indentation issues. It is more idiomatic and robust to use the toYaml template function combined with nindent.
data:
{{- toYaml $sourceSecret.data | nindent 2 }}References
- Helm template correctness: missing quote, incorrect indentation, wrong scope in with/range. (link)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/tenant/templates/copy-patch-containerd.yaml`:
- Around line 12-14: The `range` loop iterating over the `$sourceSecret.data`
map produces non-deterministic output since map iteration order is random in
Go/Helm, causing the rendered data key order to vary between reconciles and
trigger unnecessary Helm upgrades. Fix this by extracting and sorting the map
keys deterministically before rendering, then iterating over the sorted keys in
a stable order to access and render the corresponding values for each key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 986782b8-2d0b-4cc7-aab2-0ecdc52af90e
📒 Files selected for processing (2)
packages/apps/kubernetes/templates/cluster.yamlpackages/apps/tenant/templates/copy-patch-containerd.yaml
| {{- range $key, $value := $sourceSecret.data }} | ||
| {{ printf "%s: %s" $key ($value | quote) | indent 2 }} | ||
| {{ $key }}: {{ $value | quote }} | ||
| {{- end }} |
There was a problem hiding this comment.
Make Secret data rendering deterministic to avoid reconcile churn.
range over a map is non-deterministic, so the rendered data: key order can flap between reconciles. That can trigger noisy/no-op Helm upgrades in Flux.
Suggested fix
data:
-{{- range $key, $value := $sourceSecret.data }}
- {{ $key }}: {{ $value | quote }}
+{{- range $key := keys $sourceSecret.data | sortAlpha }}
+ {{ $key }}: {{ index $sourceSecret.data $key | quote }}
{{- 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 $key, $value := $sourceSecret.data }} | |
| {{ printf "%s: %s" $key ($value | quote) | indent 2 }} | |
| {{ $key }}: {{ $value | quote }} | |
| {{- end }} | |
| data: | |
| {{- range $key := keys $sourceSecret.data | sortAlpha }} | |
| {{ $key }}: {{ index $sourceSecret.data $key | quote }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/tenant/templates/copy-patch-containerd.yaml` around lines 12 -
14, The `range` loop iterating over the `$sourceSecret.data` map produces
non-deterministic output since map iteration order is random in Go/Helm, causing
the rendered data key order to vary between reconciles and trigger unnecessary
Helm upgrades. Fix this by extracting and sorting the map keys deterministically
before rendering, then iterating over the sorted keys in a stable order to
access and render the corresponding values for each key.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the fix is the right direction (moving the copy to the tenant chart breaks the same-render-pass lookup chicken-and-egg from #2663), but the tenant-root guard drops patch-containerd for Kubernetes clusters created directly in the root tenant, which the old code covered.
Business context: #2663 — the kubernetes chart copied cozy-system/patch-containerd into its own namespace and looked it up in the same Helm render pass; since lookup reads live cluster state, the secret is nil on first install and the registry-mirror certs.d entries never reach the KubeadmConfigTemplate. This moves the copy to the tenant chart so the secret exists before the kubernetes app renders.
Blockers
B1: tenant-root Kubernetes clusters lose patch-containerd
File: packages/apps/tenant/templates/copy-patch-containerd.yaml:1
Issue: the new {{- if ne (include "tenant.name" .) "tenant-root" }} guard creates the secret only in child tenant namespaces. The kubernetes chart's cluster.yaml now looks up a fixed patch-containerd in its release namespace (packages/apps/kubernetes/templates/cluster.yaml:444). The previous template lived in the kubernetes chart and copied into $.Release.Namespace, so it worked for any namespace — including tenant-root.
Evidence: patch-containerd is created by exactly three templates — core/platform/.../containerd-registry-secret.yaml (source, in cozy-system), the removed kubernetes-chart copy, and this new tenant-chart copy. With the guard, nothing creates it in tenant-root. The kubernetes chart is rendered in tenant-root by its own test suite (packages/apps/kubernetes/tests/admin_kubeconfig_wait_test.yaml:5), so a cluster in the root tenant is a real placement. Result: a Kubernetes cluster in tenant-root with registries.mirrors configured no longer gets the certs.d patch — exactly the air-gap case the secret exists for.
Impact: regresses registry-mirror delivery for root-tenant Kubernetes clusters.
Fix: either copy patch-containerd into tenant-root too (drop/narrow the guard — the source is in cozy-system and the destination namespace exists), or, if root-tenant clusters are intentionally unsupported, state that and confirm nothing relies on it.
Non-blocking follow-ups
- The secret is now copied into every non-root tenant namespace, even tenants that never create a Kubernetes cluster. Minor, but it widens where the registry-mirror config lives — worth a line in the PR description.
- Both the tenant-chart copy and the kubernetes consumer use
lookup, whose result is invisible to Flux's digest-based reconcile (a later change tocozy-system/patch-containerdwon't re-render either). Pre-existing, not introduced here, but the design still leans on the source secret being static. helm-unittestcan't exercise this path (lookupreturns nil underhelm template), so the only real coverage is E2E — make sure a tenant cluster with mirrors is exercised there.
| @@ -1,13 +1,16 @@ | |||
| {{- if ne (include "tenant.name" .) "tenant-root" }} | |||
There was a problem hiding this comment.
B1 (blocker): this guard skips tenant-root, so a Kubernetes cluster in the root tenant gets no patch-containerd — cluster.yaml looks it up in its own namespace and nothing else creates it there. The old kubernetes-chart copy covered any namespace. Either copy into tenant-root too, or confirm root-tenant clusters are unsupported.
|
Same story as #2915: this targets code that was removed. #2931 moved tenant workers to Talos via CABPT and deleted Sharing one Secret across a tenant's clusters is still a reasonable ask, but it has to be rebuilt on the machineconfig side rather than rebased. Closing. |
This PR moves the
patch-containerdsecret copying logic from thekuberneteschart to thetenantchart. This ensures that the secret is available in each tenant namespace, allowing thekuberneteschart to consume it directly from the namespace rather than maintaining a separate copy.Release note