fix(cozystack-basics): refuse a direct delete of a tenant namespace by yankawai · Pull Request #5011 · cozystack/cozystack

europrinter (yankawai) · 2026-10-21T01:33:01Z

What this PR does

Fixes #5002. A DELETE of a tenant namespace is refused unless it comes from the identities that remove it on the normal path, and the denial says to delete the Tenant instead.

The new ValidatingAdmissionPolicy in cozystack-basics matches DELETE of core namespaces, and its binding selects namespaces labelled tenant.cozystack.io/tenant-root. The tenant chart puts that label on every tenant namespace below tenant-root (tenant.ancestorTenantLabels, and migration 49 backfilled older ones). tenant-root itself has no such label and stays with cozystack-no-delete-guardrail. The tenant-* probe namespaces of the e2e suites carry only namespace.cozystack.io/host and are not selected either.

Two groups pass. system:serviceaccounts:cozy-fluxcd, because helm-controller and its shards run as flux there, and a Tenant uninstall is helm-controller deleting the namespace after the pre-delete hook. system:serviceaccounts:kube-system, for the garbage collector following the ownerReference of a child namespace once its parent is gone. The namespace controller never sends a DELETE for a namespace, it updates finalize, so it is not affected (pkg/controller/namespace/deletion and pkg/registry/core/namespace/storage at v1.33.12 and v1.34.1). system:masters is not exempt, the same as with the no-delete guardrail.

This is the denial kube-apiserver 1.35 returns.

namespaces "tenant-foo" is forbidden: ValidatingAdmissionPolicy 'cozystack-tenant-namespace-delete-policy' with binding 'cozystack-tenant-namespace-delete-policy' denied request: Namespace tenant-foo belongs to the Tenant foo in tenant-root. Delete the Tenant instead with kubectl delete tenants.apps.cozystack.io foo --namespace tenant-root. Deleting the namespace directly can remove the tenant etcd before the Kubernetes clusters that use it and leave the namespace stuck in Terminating. If the Tenant is already gone, remove the label first with kubectl label namespace tenant-foo tenant.cozystack.io/tenant-root-

There are a few limits.

The bypass is removing the label, like with the guardrail. That also takes the namespace out of the tenant-root egress policy and the dashboard tree, which only matters for a namespace that is about to go.
Migration 49 labelled every tenant-* namespace that existed when it ran, so a non-tenant namespace with such a name is refused too and needs the same label removal.
Because the garbage collector is exempt, a parent namespace whose label was removed and that is deleted with foreground propagation still cascades into its children directly.
A DELETE of a namespace that is already Terminating is answered before admission, so the policy does not see it. It changes nothing either.
A cluster that made helm-controller impersonate another account (--default-service-account, or spec.serviceAccountName on Tenant HelmReleases) would need that account allowed. Cozystack sets neither.
It does not change how Kamaji cleans up once the order is broken, which is the second idea in Tenant namespace deleted directly is stuck Terminating on Kamaji finalizers #5002.

The helper rename in external_tls_policy_test.go (loadPolicy(t, path) and compilePolicy) is byte for byte the one in #2622, so the two merge in either order.

Checked locally.

go test ./pkg/registry/apps/application/ -run 'TestTenantNamespaceDeletePolicy|TestDatabaseExternalTLSPolicy' -count=1 -v passes 4 tests, the new TestTenantNamespaceDeletePolicy_Decisions with 11 cases. go test ./pkg/registry/... ./pkg/config/... ./pkg/cmd/server/... passes in all 11 packages that have tests.
helm unittest in packages/system/cozystack-basics with helm-unittest 1.1.2 in docker passes 12 suites and 70 tests, against 11 and 66 on main. The new tests on main without the template fail, both Go tests and 4 helm tests in 2 suites.
12 mutations of the template each fail a test. They exempt system:masters, drop either exempt group, select on the no-delete label, add UPDATE, always name tenant-root as the parent, drop the capability gate, label tenant-root, switch to Warn, switch failurePolicy to Ignore, drop the bypass from the message and drop the objectSelector.
The rendered policy, the guardrail and namespaces rendered from the tenant chart were applied to kube-apiserver v1.33.0, v1.35.0 and v1.36.2 from setup-envtest, 16 checks on each. Real and dry-run deletes by a cluster admin, by an OIDC user bound to cluster-admin and by a cozy-system and a tenant ServiceAccount are refused with 403. The flux and garbage collector identities get their delete accepted. tenant-root gets the guardrail's denial, and with the guardrail binding removed a dry-run delete of it goes through, so this policy does not select it. A tenant-* namespace without the label and a tenant namespace after its label is removed are accepted for deletion. With this binding removed, a dry-run delete by the cluster admin goes through. That harness is not part of the PR.
hack/bats-no-negated-assert.bats, hack/bats-no-exit-trap.bats, hack/bats-runner-coverage.bats, hack/helmrelease-readiness.bats and hack/route-hostname-policy-version-coverage.bats pass under hack/cozytest.sh.

The new e2e case in hack/e2e-install-cozystack.bats creates an empty Tenant, checks that a server dry-run delete of its namespace is refused with the message, deletes the Tenant and waits for the namespace to go. It was not run here, there is no Cozystack cluster at hand. Its kubectl steps were run against the envtest apiserver 1.35, the selector and field selector read, the refused dry-run with the exact strings it greps for, and kubectl wait --for=delete on a namespace that is gone. The Chainsaw suites that delete Tenants, the gateway inheritance ones among them, go through the uninstall path as well.

The release-1.6 backport is #5012.

Screenshots

Not a UI change.

Downstream repositories

The trigger map was walked against the diff. It adds a policy and tests to cozystack-basics and an e2e case, and no entry covers that. The website docs do not tell users to delete a tenant namespace directly, and terraform-provider-cozystack deletes Tenants through the apps API.

Release note

fix(cozystack-basics): refuse a direct DELETE of a tenant namespace unless it comes from helm-controller or the garbage collector, and point to deleting the Tenant instead, so a tenant namespace deleted by hand no longer gets stuck Terminating on Kamaji finalizers

A tenant namespace is meant to go away with its Tenant. The tenant chart's pre-delete hook deletes the applications first and the tenant modules such as etcd after them, and helm-controller deletes the namespace at the end. A direct delete hands the namespace to the namespace controller instead. It deletes the pods first and then everything else together, so the tenant etcd and its etcd-ca-tls Secret can be gone before the TenantControlPlanes that use them. Kamaji then cannot reach the datastore to clean up, the TenantControlPlane finalizer never clears and the namespace stays Terminating. The policy refuses DELETE of a namespace labelled tenant.cozystack.io/tenant-root, which the tenant chart puts on every tenant namespace below tenant-root. tenant-root carries no such label and stays with the no-delete guardrail. Two callers are let through. helm-controller and its shards run as the flux ServiceAccount in cozy-fluxcd and delete the namespace during a Tenant uninstall. The garbage collector runs in kube-system and removes a child namespace through its ownerReference once the parent namespace is gone. The namespace controller only finalizes a namespace and never sends a DELETE for it, so it is not affected. Everyone else is refused, system:masters included, as with the no-delete guardrail. The message names the Tenant to delete and the label to remove when the Tenant is already gone. The policy test helpers take the template path now, so the new test compiles the shipped template with the apiserver CEL compiler like the external TLS policy test does. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]>

coderabbitai · 2026-10-21T01:33:19Z

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration

Source mode: Auto
Repository: cozystack/cozystack
Branch: main
File: .coderabbit.yaml
Commit: 56224a1
Layers: Repository YAML
Review profile: CHILL
Plan: Advanced
Run ID: 96b1a339-60c7-44f8-a03b-864774a659ad

Commits

Reviewing files that changed from the base of the PR and between 6bb0fda and d089dec.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

Trigger review

No actionable comments were generated in the recent review.

Walkthrough

The chart adds a validating admission policy that denies direct deletion of tenant namespaces, except for specified platform service-account groups. Helm, Go, and end-to-end tests cover policy rendering, admission decisions, and removal of the namespace through Tenant deletion.

Changes: Tenant namespace deletion guard

Layer / File(s)	Summary
Admission policy and chart coverage `packages/system/cozystack-basics/templates/tenant-namespace-delete-policy.yaml`, `packages/system/cozystack-basics/tests/tenant-namespace-delete-policy_test.yaml`, `packages/system/cozystack-basics/tests/hostname-policies-capability-gate_test.yaml`, `packages/system/cozystack-basics/tests/tenant-root_test.yaml`	The chart adds a validating admission policy and binding for tenant namespace DELETE requests. Helm tests check the rendered resources, API capability gate, and tenant-root label.
Policy decision tests `pkg/registry/apps/application/external_tls_policy_test.go`, `pkg/registry/apps/application/tenant_namespace_delete_policy_test.go`	Go tests check policy binding and decisions for allowed and denied callers. The external TLS test helpers now accept an explicit template path and use generic names.
Tenant uninstall end-to-end test `hack/e2e-install-cozystack.bats`	An end-to-end test checks that direct namespace deletion is denied and that deleting the Tenant removes its namespace.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant E2ETest
  participant KubernetesAPIServer
  participant TenantNamespaceDeletePolicy
  E2ETest->>KubernetesAPIServer: Request direct namespace deletion
  KubernetesAPIServer->>TenantNamespaceDeletePolicy: Evaluate bound policy
  TenantNamespaceDeletePolicy-->>KubernetesAPIServer: Deny and return remediation guidance
  KubernetesAPIServer-->>E2ETest: Namespace deletion rejected
  E2ETest->>KubernetesAPIServer: Delete Tenant
  KubernetesAPIServer-->>E2ETest: Associated namespace is removed

Suggested reviewers: lexfrei

Merge Risk: Minimal · previous review 6bb0f

The change adds a guard against direct deletion of tenant namespaces and keeps normal Tenant uninstall working for the platform service accounts. No concrete merge-blocking risk was found. The new end-to-end case has not been run against a real cluster.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (4 skipped: 4…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: preventing direct deletion of tenant namespaces in cozystack-basics.
Linked Issues check		Issue [`#5002`] requests a guard for direct deletion of child tenant namespaces. The PR adds a gated `ValidatingAdmissionPolicy` for core namespace `DELETE` requests and binds it to `tenant.cozystack.io…
Out of Scope Changes check		The changes stay within [`#5002`]. The policy template implements the namespace guard. The tenant-root and capability-gate tests verify related chart behavior. The Go helper rename supports reuse by the…

Full details: Linked Issues check

Explanation

Issue [#5002] requests a guard for direct deletion of child tenant namespaces. The PR adds a gated ValidatingAdmissionPolicy for core namespace DELETE requests and binds it to tenant.cozystack.io/tenant-root. The policy denies ordinary users, including system:masters, and directs them to delete the Tenant or remove the label for an orphaned namespace. It permits the documented Flux and garbage-collector service-account groups for normal uninstall and cascading cleanup. The Go and Helm tests cover matching, selection, decisions, exemptions, and denial messages. The separate Kamaji cleanup idea is not a requirement for this guard.

Full details: Out of Scope Changes check

Explanation

The changes stay within [#5002]. The policy template implements the namespace guard. The tenant-root and capability-gate tests verify related chart behavior. The Go helper rename supports reuse by the new policy tests. The e2e test verifies the guarded delete and normal Tenant uninstall path. No unrelated product behavior or Kamaji cleanup implementation is added.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (4 skipped: 4 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: `3729b470-8f60-4a10-9c71-32500b83e6b7`

📥 Commits

Reviewing files that changed from the base of the PR and between `1f92bcd` and `6bb0fda`.

📒 Files selected for processing: 7

`hack/e2e-install-cozystack.bats`

`packages/system/cozystack-basics/templates/tenant-namespace-delete-policy.yaml`

`packages/system/cozystack-basics/tests/hostname-policies-capability-gate_test.yaml`

`packages/system/cozystack-basics/tests/tenant-namespace-delete-policy_test.yaml`

`packages/system/cozystack-basics/tests/tenant-root_test.yaml`

`pkg/registry/apps/application/external_tls_policy_test.go`

`pkg/registry/apps/application/tenant_namespace_delete_policy_test.go`

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

IvanHunters

Verified the invariant rather than only the guard: enumerated every namespace deleter (no Go code deletes a tenant namespace; helm-controller in cozy-fluxcd and the GC in kube-system are exempt, so legitimate teardown via kubectl delete tenant still works). The direct human DELETE is refused, confirmed with the in-process CEL decision test (11/11) and a mutation that drops the kube-system exemption (the GC case then fails). No blocking issues.

Non-blocking:

tenant-namespace-delete-policy.yaml:69-73: the system:serviceaccounts:kube-system exemption is broad (any SA in kube-system, not just the GC). Acceptable as a footgun guardrail since RBAC still gates who can delete, but worth awareness.
tenant-namespace-delete-policy.yaml:74-84: the denial message parses tenant/parent by splitting on the last -, which is correct only while tenant names stay single-word. Deny fires regardless, so this is message accuracy only.
Migration 49 labels every ^tenant- namespace, so a non-tenant namespace named tenant-* becomes undeletable until the label is removed (the message gives the command). Protection also depends on the tenant-root label being present, with a brief gap for a not-yet-reconciled deep tenant namespace.

Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

Aleksei Sviridkin (lexfrei) · 2026-11-13T11:22:19Z

europrinter (@yankawai) I merged main into your branch (d089dec) so CI runs against the current base. Your commits are unchanged.

Aleksei Sviridkin (lexfrei)

europrinter (@yankawai) LGTM, CI and E2E are green on this head.

europrinter (yankawai) requested review from IvanHunters, Andrei Kvapil (kvaps), Aleksei Sviridkin (lexfrei), Timofei Larkin (lllamnyp), mattia-eleuteri, myasnikovdaniil and Kirill Ilin (sircthulhu) as code owners October 21, 2026 01:33

github-actions Bot added area/platform Issues or PRs related to platform infrastructure (bundle, flux, talos, installer) kind/bug Categorizes issue or PR as related to a bug size/L This PR changes 100-499 lines, ignoring generated files labels Oct 21, 2026

europrinter (yankawai) mentioned this pull request Oct 21, 2026

[Backport release-1.6] fix(cozystack-basics): refuse a direct delete of a tenant namespace #5012

Open

1 of 12 tasks

IvanHunters approved these changes Oct 23, 2026

View reviewed changes

Merge branch 'main' into fix/tenant-namespace-direct-delete-guard

d089dec

Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>

Aleksei Sviridkin (lexfrei) approved these changes Nov 13, 2026

View reviewed changes

Aleksei Sviridkin (lexfrei) merged commit 29e8522 into cozystack:main Nov 13, 2026
63 checks passed

Navigation Menu

Repository navigation

fix(cozystack-basics): refuse a direct delete of a tenant namespace - #5011

fix(cozystack-basics): refuse a direct delete of a tenant namespace#5011
Aleksei Sviridkin (lexfrei) merged 2 commits into
cozystack:mainfrom
yankawai:fix/tenant-namespace-direct-delete-guard

europrinter (yankawai) commented Oct 21, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 21, 2026 •

edited

Loading

Review skipped

Sequence Diagram(s)

Uh oh!

IvanHunters left a comment

Uh oh!

Aleksei Sviridkin (lexfrei) commented Nov 13, 2026

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

europrinter (yankawai) commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Screenshots

Downstream repositories

Release note

Uh oh!

coderabbitai Bot commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Sequence Diagram(s)

Uh oh!

IvanHunters left a comment

Choose a reason for hiding this comment

Uh oh!

Aleksei Sviridkin (lexfrei) commented Nov 13, 2026

Uh oh!

Aleksei Sviridkin (lexfrei) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

europrinter (yankawai) commented Oct 21, 2026 •

edited

Loading

coderabbitai Bot commented Oct 21, 2026 •

edited

Loading