fix(tenant): inherit full ancestor label chain from parent namespace - #2912
Conversation
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 addresses a bug in the tenant namespace template where nested tenants failed to inherit the full chain of ancestor labels. By switching from a name-based derivation to reading labels directly from the parent namespace, the fix ensures that all transitive ancestor labels are correctly applied, resolving network policy timeouts for deeply nested tenant structures. 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
|
📝 WalkthroughWalkthroughThe PR adds deterministic ancestor-label generation for tenant namespaces, updates namespace rendering to use it with lookup-safe owner references, adds Helm tests for nested label depth, and introduces a migration that backfills the labels onto existing namespaces. ChangesTenant ancestor labels and namespace rendering
Suggested reviewers: Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 tenant namespace template to inherit ancestor tenant.cozystack.io/* labels directly from the parent namespace's metadata instead of parsing the namespace string. This ensures correct label inheritance for multi-level tenant hierarchies using short names. Feedback on this change suggests simplifying the template by removing the redundant default dict wrapper, as ranging over a nil map is safe in Go templates.
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.
| Reading the existing parent namespace's labels recovers the full | ||
| chain regardless of naming. | ||
| */}} | ||
| {{- range $k, $_ := (default dict $existingNS.metadata.labels) }} |
There was a problem hiding this comment.
In Go templates, ranging over a nil map is completely safe and behaves as a no-op. Therefore, the default dict wrapper is redundant and can be safely omitted to simplify the template.
{{- range $k, $_ := $existingNS.metadata.labels }}
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the lookup-based inheritance has no base case: the tenant-root namespace carries no tenant.cozystack.io/* label, so on a fresh install every direct child of tenant-root now loses the tenant.cozystack.io/tenant-root label that the removed split-based code always emitted. This regresses depth-1 tenants (the common case) while fixing depth-2.
Business context: nested tenants at depth >= 2 were missing transitive ancestor tenant.cozystack.io/* labels — root's direct children use short namespace names (tenant-ktj, not tenant-root-ktj), so the dash-split heuristic could not reconstruct tenant-root — which broke the root-ingress proxy path. The fix replaces the heuristic with forward-propagation of the parent namespace's labels.
Blockers
B1: Direct children of tenant-root lose tenant.cozystack.io/tenant-root on fresh installs
File: packages/apps/tenant/templates/namespace.yaml:90-92
Issue: the new loop copies tenant.cozystack.io/* labels from the parent namespace. For a depth-1 tenant the parent is tenant-root, whose namespace (defined in packages/system/cozystack-basics/templates/tenant-root.yaml) carries platform.cozystack.io/no-delete, namespace.cozystack.io/host and optionally namespace.cozystack.io/gateway — but not tenant.cozystack.io/tenant-root. So a depth-1 child inherits nothing and ends up with only its own self-label.
Evidence: a repo-wide search shows the only emitters of tenant.cozystack.io/<name> are this self-label (skipped for tenant-root by the ne (include "tenant.name" .) "tenant-root" guard) and the split-based code this PR removes. The old code, for a depth-1 tenant (Release.Namespace == "tenant-root"), ran splitList "-" "tenant-root" -> [tenant, root] and emitted tenant.cozystack.io/tenant-root. The new code emits it only if the parent namespace already carries it. The PR's own test plan notes "parent has no tenant.cozystack.io/* labels in fresh install" — that is exactly the regression, not the expected state.
Impact: the tenant-root-egress CiliumClusterwideNetworkPolicy lets root pods (including the root ingress-nginx) egress only to namespaces labeled tenant.cozystack.io/tenant-root. Depth-1 child namespaces losing that label reintroduces the same upstream timed out failure this PR fixes — now at depth 1, which previously worked.
Fix: add tenant.cozystack.io/tenant-root: "" to the tenant-root namespace in packages/system/cozystack-basics/templates/tenant-root.yaml. That establishes the inductive base so the forward-propagation chain holds at every depth (depth-1 inherits {tenant-root}, depth-2 inherits {tenant-ktj, tenant-root}, ...). On upgrade the cozystack-basics chart backfills the label onto the existing tenant-root namespace.
Non-blocking follow-ups
- The fix is forward-only on existing clusters: helm-controller's
lookupresult is not part of the release input digest, so already-created tenant namespaces will not re-render and self-heal. A new descendant created under an already-broken (missing-tenant-root) namespace also inherits the broken set. The manual relabel in "Workaround pre-merge" handles this, but it must be applied from the shallowest broken ancestor downward — worth stating in the PR / migration notes. - The inheritance logic is untested: the top-level
lookup+failabortshelm template/ helm-unittest rendering without a live cluster, so there is no automated coverage. Pre-existing constraint, but the new behavior is now load-bearing for cross-tenant routing.
| Reading the existing parent namespace's labels recovers the full | ||
| chain regardless of naming. | ||
| */}} | ||
| {{- range $k, $_ := (default dict $existingNS.metadata.labels) }} |
There was a problem hiding this comment.
On a fresh install the parent here is tenant-root, whose namespace (cozystack-basics/templates/tenant-root.yaml) carries no tenant.cozystack.io/* label — so a depth-1 child inherits nothing and loses tenant.cozystack.io/tenant-root, which the removed split-based code always emitted. Label the tenant-root namespace with tenant.cozystack.io/tenant-root: "" to give this loop a base case. See B1 in the review summary.
The tenant.cozystack.io/<ancestor> namespace labels drive the <ancestor>-egress CiliumClusterwideNetworkPolicy, which lets an ancestor tenant reach a descendant namespace only when the descendant carries tenant.cozystack.io/<ancestor>. namespace.yaml derived that chain by splitting .Release.Namespace, which only reached one level up: a direct child of tenant-root got tenant-root, but a grandchild (tenant-ktj-htdev under tenant-ktj) got only tenant-ktj and dropped tenant-root entirely. The root ingress controller in tenant-root then could not reach nested tenants, surfacing as upstream timeouts. Derive the chain deterministically from the namespace name instead. Every tenant descends from tenant-root, and tenant.name builds each child as <parent-namespace>-<word>, so every progressive dash-prefix of the name is a real ancestor namespace. This needs no cluster lookup, renders offline (so it is unit-testable), and converges on every reconcile regardless of parent/child HelmRelease ordering. The ownerReference lookup is made non-fatal so the template still renders when the parent namespace lookup is empty; in a real reconcile the parent always exists and the ownerReference is always emitted. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
The tenant chart fix makes new and re-reconciled tenant namespaces self-compute their full tenant.cozystack.io/<ancestor> chain, but existing namespaces whose HelmRelease does not promptly re-reconcile (suspended or failed releases) keep the incomplete labels — and with them the broken <ancestor>-egress reachability that surfaces as upstream timeouts to nested tenants. Backfill the labels directly so clusters heal on upgrade regardless of per-tenant HelmRelease state. Bump targetVersion to 50 so run-migrations.sh iterates the new script (seq CURRENT..49); the missing-file guard otherwise never fires. Best-effort and idempotent: kubectl label --overwrite adds only the listed labels, and per-namespace failures are tolerated so a transient apiserver error cannot abort the platform upgrade. Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
3b5ccd4 to
5996404
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the re-architecture resolves my earlier blocker.
The ancestor chain is now derived deterministically from the namespace name (tenant.ancestorTenantLabels) with tenant.cozystack.io/tenant-root emitted unconditionally, so a depth-1 child no longer loses the tenant-root label — the regression I flagged is gone, and the depth-1 case is pinned by a unit test. The name-derivation is safe against dashes in tenant names because tenant.name rejects any release name that isn't tenant-<single-word>, so every namespace name is a clean dash-join of dash-free ancestors, and the bare tenant prefix is correctly skipped. Migration 49 backfills existing clusters with the same derivation (idempotent --overwrite, best-effort || true, version stamped via the shared helper), addressing the forward-only / no-self-heal gap of the previous approach. The new unit test covers the full contract — depths 1/2/3 plus a negative assertion for a non-ancestor.
Note: the two red build checks are an Oracle registry push denial on the matchbox / platform-migrations images — infra, not this change (the images build; only the push step fails). Needs a green (re)run before merge, separate from the code.
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-2912-to-release-1.5
git worktree add --checkout .worktree/backport-2912-to-release-1.5 backport-2912-to-release-1.5
cd .worktree/backport-2912-to-release-1.5
git reset --hard HEAD^
git cherry-pick -x 59964043e390a7830f79c838cd754d7da0d066d4
git push --force-with-lease |
Backfill tenant.cozystack.io/<ancestor> labels on existing tenant namespaces so clusters heal on upgrade without waiting on each tenant HelmRelease to re-reconcile (#2810/#2171). Backport of migration 49 from #2912, adapted for release-1.5: - migration file 49 -> 45 (release-1.5 is at targetVersion 45; next free slot) - targetVersion 45 -> 46, not main's 50, so the missing-file guard does not fail on the absent 46/47/48/49 - stamp via the inline labeled-ConfigMap apply used by release-1.5 migrations, since lib/cozystack-version.sh does not exist on this branch Best-effort and idempotent: kubectl label --overwrite adds only the listed labels, and per-namespace failures are tolerated so a transient apiserver error cannot abort the platform upgrade. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Backfill tenant.cozystack.io/<ancestor> labels on existing tenant namespaces so clusters heal on upgrade without waiting on each tenant HelmRelease to re-reconcile (#2810/#2171). Backport of migration 49 from #2912, adapted for release-1.5: - migration file 49 -> 46. The existing backport PR #3191 renumbered it to 45, but the SeaweedFS repair migration backported alongside it (#3339 / #3370) claims 45 too, so the two collide. Whichever of those two PRs merges second has to take 46; here the SeaweedFS one keeps 45 because it is what the 1.5.x line is being cut for. - targetVersion 45 -> 47, covering both new migrations. #3191 alone bumps it to 46, and because the two backports agree on that value git merges them without a conflict — leaving targetVersion one short of the highest migration, which run-migrations.sh silently skips (its loop stops at TARGET-1). release-1.5 has no `migrations-target-check` to catch it; that guard arrived on main after v1.5.2 and was never backported. - stamp via the inline labeled-ConfigMap apply used by release-1.5 migrations, since lib/cozystack-version.sh does not exist on this branch Best-effort and idempotent: kubectl label --overwrite adds only the listed labels, and per-namespace failures are tolerated so a transient apiserver error cannot abort the platform upgrade. (cherry picked from commit 50f78ce) Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit aee9fa85b65d96dae1dff7e5044fd26ce710797e)
Summary
packages/apps/tenant/templates/namespace.yamllabels every tenant namespacewith
tenant.cozystack.io/<ancestor>for each of its ancestors. These labelsdrive the
<ancestor>-egressCiliumClusterwideNetworkPolicy(
networkpolicy.yaml): an ancestor tenant's pods may reach a namespace only ifthat namespace carries
tenant.cozystack.io/<ancestor>.The chain was derived by splitting
.Release.Namespace, which only ever reachedone level up. A direct child of
tenant-rootgottenant-root(its parentnamespace is
tenant-root), but a grandchild —tenant-ktj-htdevcreatedunder
tenant-ktj— got onlytenant-ktjand droppedtenant-rootentirely.Without
tenant.cozystack.io/tenant-rooton a nested namespace, the rootingress controller cannot reach the proxy service in a nested
kubernetestenant, surfacing as
upstream timed out (110) while connecting to upstreaminthe nginx logs.
Fix
Derive the ancestor chain deterministically from the namespace name instead of
from any cluster lookup. Every tenant descends from
tenant-root, so that labelis emitted unconditionally; the remaining ancestors are encoded in the namespace
name itself, because
tenant.namebuilds each child as<parent-namespace>-<word>— so every progressive dash-prefix of the name is areal ancestor namespace (
tenant-ktj-htdevyieldstenant-ktjandtenant-ktj-htdev). The logic lives in a newtenant.ancestorTenantLabelshelper:
This needs no cluster state, renders identically offline (so it is
unit-testable), and converges on every reconcile regardless of the order in
which parent and child
HelmReleases reconcile — unlike a parent-namespacelookup, which helm-controller does not re-render on and which isordering-dependent. The
ownerReferencelookupis kept but made non-fatal:during a real reconcile the parent namespace always exists (the tenant
HelmReleaseand itscozystack-valuesSecret live in it), so theownerReferenceis always emitted; when the lookup is empty (offline / unittest) it is simply omitted so the labels still render.
Migration
49backfills the labels on existing clusters so they heal on upgraderegardless of per-tenant
HelmReleasestate (e.g. suspended or failedreleases), and
migrations.targetVersionis bumped to50so the runneriterates it. The migration derives the same chain from each namespace name,
is idempotent (
kubectl label --overwrite) and best-effort (per-namespacefailures are tolerated so a transient apiserver error cannot abort the upgrade).
Test plan
helm template packages/apps/tenantat depth 1/2/3 — emits exactly theexpected ancestor set at each depth: depth-1
{tenant-root, tenant-ktj},depth-2 adds
tenant-ktj-htdev, depth-3 addstenant-ktj-htdev-foo. Nomissing, no stray labels.
helm unittest packages/apps/tenant— newtests/namespace_ancestor_labels_test.yamlpins the chain at depths 1/2/3plus a negative assertion; whole suite green.
bats hack/cozystack-version-stamp.bats— migration49stamps theversion through the shared
stamp_cozystack_versionhelper (all 10 testspass, including the guard that migrations >= 42 must use the helper).
49ancestor derivation verified under/bin/shto match thehelm output byte-for-byte, and dry-runs cleanly under
set -euo pipefail.root -> ktj -> htdev), verifytenant-ktj-htdevhas all threetenant.cozystack.io/*labels.kubernetestenant underhtdevwithaddons.ingressNginx.hosts, verify an external HTTP request reaches thetenant ingress controller via the root-ingress proxy (no
upstream timed out).Closes
tenant.cozystack.io/*labels beyond direct parent #2810Summary by CodeRabbit