Repository navigation
fix(tenant): skip Cilium policies on isp-hosted without Cilium - #4710
Merged
Merged
Conversation
github-actions
Bot
added
area/tenant
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe tenant chart now gates Cilium network policies by bundle type and API availability. Tests cover hosted and non-hosted bundles. The README and security documents describe the limits of tenant isolation when the host does not provide the Cilium policy API. ChangesTenant network policy rendering
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to The change stops hosted installs from failing when the host has no Cilium policy API. Other variants render policies as before. Hosted tenants on such hosts get no Cozystack network isolation, and the docs now say so. No merge-blocking risk was found. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Hosted tenants can now be installed without Cozystack-provided network isolation when the host does not offer Cilium policies. The host must provide any required alternative isolation. There is also an unresolved risk to existing policies if Cilium API discovery changes during an update. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 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 |
The isp-hosted variants install no CNI of their own, so on a host without Cilium the cilium.io/v2 kinds do not exist and the tenant install fails, taking the root tenant and everything that depends on cozystack-basics down with it. The policies now render on isp-hosted only when the host serves both Cilium policy kinds. On every other variant Cozystack installs Cilium itself, so they keep rendering unconditionally: a CRD that is missing at render time there fails the install loudly instead of leaving the tenant without network isolation, because helm-controller does not re-render a release when only API discovery changes. On isp-hosted without Cilium the tenant gets no network isolation from Cozystack; isolating tenants is then up to the host's CNI. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
force-pushed
the
fix/tenant-cilium-policy-guard
branch
from
October 2, 2026 09:11
6fc5251 to
3b5af9d
Compare
Aleksei Sviridkin (lexfrei)
requested a review
from Timur Tukaev (tym83)
as a code owner
October 2, 2026 09:11
github-actions
Bot
added
size/L
myasnikovdaniil
approved these changes
Oct 2, 2026
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
Contributor
There was a problem hiding this comment.
Looks good. Render on the Cilium variants matches main, the guard itself holds up.
Not blocking, but worth an issue or a small PR: the tracingCentral description in values.yaml (and README, schema and ApplicationDefinition generated from it) says tenants sharing the store cannot read each other's traces. What keeps them off vtselect is tenant egress, per the header of packages/system/monitoring/templates/vtraces/vmauth.yaml. On isp-hosted without the Cilium API this PR installs the tenant with tracingCentral still wired and both traces-central policies skipped, so the promise doesn't hold there. Either qualify the description for that case and run make generate, or make the render fail when tracingCentral is set and the policies are skipped. threat-model.md line 67 also states cross-tenant network isolation without pointing at the new non-goal on line 86.
Also not blocking:
- In the issue I suggested an explicit force-require value next to autodetect. Fine to leave out if deliberate, but on hosted there is now no way to ask for a failure when Cilium is missing, and nothing at runtime says the tenant is unisolated. Is the README paragraph the only signal intended?
- Same failure class is left in
packages/system/ingress-nginx/templates/admission-webhook-networkpolicy.yaml. It renders a CiliumNetworkPolicy with no capability check, unlike its sibling egress policy, so a tenant withingress: trueon hosted without Cilium should hit the same no matches for kind error. Going by the render only, not run on a cluster. Worth its own issue.
| - hasDocuments: | ||
| count: 0 | ||
|
|
||
| - it: renders no Cilium objects on isp-hosted when only one Cilium kind is served |
Contributor
There was a problem hiding this comment.
Dropping the CiliumNetworkPolicy conjunct from tenant.ciliumPolicies keeps the suite green, so does widening the hosted list with isp-slim, isp-full-generic or isp-slim-generic. A case serving only CiliumClusterwideNetworkPolicy, and the three other Cilium variants rendering without the API, would catch both. Not blocking.
1 of 12 tasks
Aleksei Sviridkin (lexfrei)
added a commit
that referenced
this pull request
Oct 5, 2026
## What this PR does This adds a daily CI lane that installs the `isp-hosted` platform variant through the `hosted` installer variant. No lane ran either of them before. The PR, nightly and tag lanes all install `isp-full` on Talos, where Cozystack brings its own CNI and storage, and that is how #3376 shipped unnoticed. What `isp-hosted` changes is what it leaves to the host. The system bundle swaps kube-ovn and Cilium for a `noop` networking Package and drops multus, LINSTOR, MetalLB, cozy-proxy, snapshot-controller and the rest of the shared data-plane set. The iaas bundle refuses to render. Migrations are not a difference, `isp-full` enables them too. So the lane runs on kind. kindnet, kube-proxy and local-path come from the cluster, and there is no Cilium, no LINSTOR and no `replicated` or `local` StorageClass. It installs the latest GHCR nightly instead of building anything, at 05:00 UTC after the nightly mirror publishes, and on `workflow_dispatch`. There is no pull request trigger, because a run would install the nightly images and not the PR's. A run passes when every HelmRelease is Ready, `cozystack-platform` and `tenant-root` included. It also checks that the platform emitted the `noop` networking Package and none of LINSTOR, MetalLB, multus or KubeVirt, and that the cluster has no Cilium CRD and no `replicated` or `local` class. The lane reuses the e2e sandbox container for tooling and `collect-report`, attached to the kind network. `hack/isp-hosted-lane_test.bats` pins the wiring: the Package selects `isp-hosted`, the install uses `variant=hosted`, the kind DNS domain matches `networking.clusterDomain`, and kind is pinned by checksum and digest. Six of its seven tests were red before the implementation existed. `hack/sandbox-runner-headroom.bats` gets a named exemption. That check treats any workflow touching `packages/core/testing` as a lane that boots the QEMU guests, and this one boots none. The exemption is checked after the job match, and removing it turns the test red again. The pins come from the kind v0.32.0 release: the binary sha256 `50030de2…` from its `kind-linux-amd64.sha256sum`, and `kindest/node:v1.33.12@sha256:3f5c8443…` from its release notes. v1.33.12 is the Kubernetes version the Talos lanes use, so the variant is the only thing that differs. All three are plain `env` values in the workflow, where a Renovate custom manager can match them. ### Real runs The first real run on this branch, [36961209755](https://github.com/cozystack/cozystack/actions/runs/36961209755), went red on `tenant-root`: ```text tenant-root False Helm install failed for release tenant-root/tenant-root with chart [email protected]+629b61300902: unable to build kubernetes objects from release manifest: resource mapping not found for name: "allow-external-communication" namespace: "tenant-root" from "": no matches for kind "CiliumNetworkPolicy" in version "cilium.io/v2" ``` This is #3387 reproduced live: `packages/apps/tenant` renders Cilium policies on a cluster without Cilium. The other releases went Ready, except `cozystack-basics`, which was still in an upgrade at the last listing. The fix landed in #4710. The run after it, [37089699259](https://github.com/cozystack/cozystack/actions/runs/37089699259), is green on the first attempt. It installed nightly `0.0.0-nightly.20261003.37089458387.1`, built from 309875a, which contains #4710. All four install tests passed, `tenant-root` included, with no `cilium.io` CRD on the cluster. The install took about 9 minutes. ### Notes The OpenAPI suite is not run. Its "Test kinds" case needs an Ingress application, and kind has no LoadBalancer, so it would fail for a reason unrelated to the variant. The red run's job log is 347 MB. cozytest's live trace prints the whole HelmRelease snapshot JSON from `cozy_wait_all_helmreleases_ready` on every 2-second poll, and `gh run view --log` breaks at that size. The isp-full lanes use the same helper, and it is tracked separately. The red run spent about 31 minutes in install, most of it waiting out the 30-minute gate; the green one took about 9 minutes on `oracle-vm-24cpu-96gb-x86-64`. Moving to a smaller runner needs a measured run, so it is a follow-up and not part of this PR. Follow-ups from review, none blocking: - No `vars.NIGHTLY_ENABLED` gate, so with the nightly switched off the lane keeps testing a stale `nightly` tag. A `workflow_run` trigger on a successful mirror would fix that too. - The Package endpoint `cozy-hosted-control-plane` has to agree with `KIND_CLUSTER: cozy-hosted`, and no test checks that yet. - The operator registration check greps a Go literal in `cmd/cozystack-operator/main.go`, so reformatting that table breaks it. - The oras download has no checksum check, same as in `nightly.yaml`. - cozytest stops at the first failing test, so on a red install the "no Cilium" check never runs. Running it before the install would keep that evidence in red runs. Closes #3390 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: The diff adds CI files, test files and one new make target. Nothing under `hack/` is moved or renamed and no existing target changes, so the ccp trigger does not apply. ### Release note ```release-note ci(e2e): add a daily lane that installs the isp-hosted platform variant on kind ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added scheduled and manually triggered end-to-end checks for hosted installations using the latest nightly packages. * Validation covers platform readiness, package deployment, and expected networking and storage behavior. * Test runs collect diagnostic reports and attempt cleanup after failures to support investigation. * Added checks to verify hosted installation settings and ensure prohibited network and storage components are absent. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
The tenant chart no longer fails to install on a hosted cluster without Cilium. The isp-hosted variants install no CNI of their own, but the chart rendered its Cilium policies unconditionally, so
tenant-rootfailed withno matches for kind "CiliumNetworkPolicy" in version "cilium.io/v2". Everything that depends oncozystack-basicsthen never converged. This also unblocks the isp-hosted e2e lane, which fails on exactly this.The guard is keyed on the platform variant. The platform already passes it to every tenant as
_cluster.bundle-name. It covers every Cilium object the chart renders, including the shared-central tracing egress policies.isp-hostedandisp-hosted-slim, the policies render only when the host serves both Cilium policy kinds, since a hosted cluster may run Cilium.The render on Cilium variants is byte-identical to
main. I checked it withhelm templateover all four Cilium variants and an absent_cluster, with optional services on and off, with and without the Cilium API, for root, child and grandchild tenants.I didn't use the capability check alone because it fails open. helm-controller upgrades a deployed release only when the chart or the values digest changes, so a change in API discovery never triggers a re-render. That is
VerifyReleaseininternal/action/verify.goat v1.5.0, the version Cozystack ships. If the Cilium CRDs were not discoverable at install time on a Cilium cluster, a capability-only guard would leave the tenant without policies until the next chart or values change. An upgrade rendered in that window would delete existing ones.On isp-hosted without Cilium, tenants get no network isolation from Cozystack. Isolating them is up to the host. I didn't fall back to standard
NetworkPolicy, because the host CNI may not enforce it. The tenant policies also use Cilium features that don't map one to one: theworldandkube-apiserverentities, clusterwide policies, selectors on ancestor namespace labels. A partial translation would look like isolation without really being one. The tenant README now documents this, including that Cilium added to the host later takes effect only on the tenant's next chart or values change. The threat model and the self-assessment list it under non-goals.The new helm-unittest suite fails on
mainfor the hosted cases without Cilium. Each guard mutation I tried turns at least one case red: no variant check, no capability check,andchanged toor,isp-hosted-slimdropped from the list.I didn't add an explicit "require Cilium" value, since the Cilium variants already behave that way. One thing to keep in mind: the list of hosted variants now lives in the platform bundle and in the tenant helper, so a new hosted variant has to go into both. A platform
_clusterflag saying whether Cozystack manages the CNI would remove that, but it's a platform change.Closes #3387
Screenshots
Not a UI change.
Downstream repositories
The tenant README change reaches the website through the release bot that regenerates the managed-apps pages from each package README.
Release note
Summary by CodeRabbit
isp-hostedandisp-hosted-slimonly when the required Cilium APIs are available. If Cilium becomes available later, policies are applied after a subsequent chart or values change. Tracing policies also require the Cilium APIs.