Repository navigation
feat(networking): cozyplane eBPF CNI + cozyplane-kpr networking variant (drops Cilium) - #3149
Timofei Larkin (lllamnyp) wants to merge 10 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true✨ 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 |
f37bd80 to
1f18873
Compare
…ate (#3184) ## What this PR does Closes two coverage gaps found while shadow-observing the API-owner-review gate (#3167) against real PRs in the first hours after merge: 1. **CRD discovery was gated on directory name** (`crds/`, `definitions/`, `manifests/`), so a CRD shipped inside a chart's `templates/` dir — a valid, existing convention when the CRD is wrapped in a `{{- if }}` guard — was invisible to the gate. This was not hypothetical: `#3149` (cozyplane) adds four brand-new CRDs (`VPC`, `VPCBinding`, `VPCPeering`, `Port`) exactly this way, and none of them tripped a review requirement. 2. **Groups served only by an external aggregated apiserver had no representation at all.** The same `#3149` also registers `sdn.cozystack.io` via a `kind: APIService` pointing at a prebuilt image built outside this repository — no CRD, no vendored Go types, nothing to diff. ### Fix - CRD/APIService discovery is now content-based: every `.yaml`/`.yml` file under `packages/` and `internal/` is a candidate, and whether it's kept depends on whether it parses into the right `kind` with a first-party group — not which directory it sits in. - Both parsers now tolerate a Helm directive line (`{{- if }}`, `{{- end }}`, a comment block) sharing a `---`-delimited document with otherwise-valid YAML, rather than dropping the whole document. This matters because the closing `{{- end }}` of a wrapping conditional has no `---` before it, so it lands in the same document as the manifest it closes. - Added a fourth resource source, `SourceAPIService`, for `kind: APIService` manifests registering a first-party group. Like the existing static Go-backed groups, these carry no checked-in schema, so they participate only in new-group / new-resource / removal detection — never a fabricated breaking-change diff against a schema this repo can't see. Verified against the real `feat/cozyplane` branch: the gate now reports all four new CRDs, plus the removal of the old `securitygroups`/`APIService` registration for the same group — a full API-surface migration that was previously invisible end to end. ### Release note ```release-note fix(ci): the API-owner-review gate now discovers CRDs and APIService registrations by content instead of directory name, closing a gap where a CRD or an externally-served aggregated API group could ship without triggering the required review. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Discovery now includes APIService registrations, enabling detection of groups/versions exposed only via aggregated APIService manifests (including merging versions from multiple templates). * **Bug Fixes** * Improved YAML discovery to tolerate mixed/templated multi-document streams: non-CRD/non-APIService documents no longer stop discovery, and embedded template directives are handled more reliably. * File processing is more resilient during snapshot loading (includes a safety cap on YAML size). * **Tests** * Added coverage for APIService discovery, templated neighbor documents, and correct exclusion of third-party manifests. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
85c6dbc to
7f057be
Compare
|
This PR has had no activity for 60 days and was marked |
3c59286 to
7aae3d0
Compare
db3daed to
077f04f
Compare
7a06153 to
312ec1b
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Timofei Larkin (@lllamnyp) NOT LGTM. The pinned arm64 images carry x86-64 binaries, CI is red on the new packages, and the variant switch is accepted on bundle variants where it breaks networking. Reviewed at 312ec1b.
Business context: an opt-in networkingVariant: cozyplane for the Talos system bundle that replaces kube-ovn, Cilium and kube-proxy with the cozyplane eBPF CNI, its aggregated API server and a socket-LB kube-proxy replacement.
The default path is safe. I rendered the platform chart on current main and on main with this diff applied, for all four system variants with default values. The only difference is two new PackageSources (cozystack.cozyplane-apiserver, cozystack.cozyplane-cilium-crds) and the new cozyplane entry in cozystack.networking. No Package changes, so existing clusters are untouched.
Blockers
B1: the arm64 image entries contain amd64 binaries
The platform is going multi-arch, so the arm64 entry has to actually run on arm64. Both indexes list linux/arm64, but only the Debian base layer is arm64. Every binary copied on top is x86-64.
ghcr.io/lllamnyp/cozyplane@sha256:a50c456b… arm64 entry sha256:bb142456…
usr/local/bin/cozyplane-agent ELF 64-bit LSB executable, x86-64
usr/local/bin/cozyplane-apiserver ELF 64-bit LSB executable, x86-64
usr/local/bin/sdn-controller ELF 64-bit LSB executable, x86-64
usr/local/bin/cozyplane-responder ELF 64-bit LSB executable, x86-64
opt/cni/bin/cozyplane, host-local, loopback x86-64
usr/sbin/xtables-nft-multi ARM aarch64 (debian base, correct)
ghcr.io/lllamnyp/cozyplane-kpr@sha256:7417b9ad… arm64 entry sha256:cfe471d8…
usr/bin/cozyplane-kpr ELF 64-bit LSB executable, x86-64
registry.k8s.io/etcd:3.5.16-0 arm64 entry: etcd, etcdctl ARM aarch64 (fine)
The agent binary in the arm64 entry has the same sha256 as the one in the amd64 entry (da363880…), and layers 3 to 10 of the two manifests are byte-identical. The cause is in both upstream Dockerfiles at 7454b02 (the commit main-7454b02 resolves to this digest; kpr's digest is main-60525cd): ARG TARGETARCH=amd64. With a default value, BuildKit does not fill in the platform ARG for that stage (docker/buildx#510), so GOARCH=${TARGETARCH} and the cni-plugins-linux-${TARGETARCH} download are amd64 for both legs. On an arm64 node the agent, controller, apiserver and kpr fail with exec format error. install-cni also copies x86-64 plugins into the host /opt/cni/bin, so the kubelet can't set up any pod network there. Commit 077f04f says "the kpr image is CI-built now, amd64+arm64", and cozyplane-kpr/values.yaml says the same. Both statements are false for these digests.
Fix: in both Dockerfiles, change ARG TARGETARCH=amd64 to a bare ARG TARGETARCH in every stage that reads it, rebuild, and repin. A cheap guard in the upstream release job is file on one binary per leg.
The BPF side is fine. overlay_bpfel.o has only tc programs, and bpf_sock.o has only cgroup/connect*, sendmsg*, recvmsg* and sock_release. Neither has kprobes, fentry or PT_REGS access, so one little-endian object is correct for both architectures. No chart value, nodeSelector or affinity pins kubernetes.io/arch. Both DaemonSets tolerate everything.
B2: CI is red because of the new packages
Unit & controller tests (run 36410226587, attempt 1, 2026-09-28T10:31:34Z) fails in helm-unit-tests:
ERROR: packages/system/cozyplane reported no test suite that ran. Its Makefile
declares a 'test' target, so its recipe is expected to
run helm-unittest over suites under tests/ matching *_test.yaml.
ERROR: Tests failed in the following directories:
- packages/system/cozyplane
- packages/system/cozyplane-apiserver
- packages/system/cozyplane-kpr
The three Makefiles have test: helm lint ., and none of the packages has a tests/ directory. E2E Tests is red only because finalize needs checks, so the in-tree e2e was skipped ("result: skipped"). This does not count as an e2e failure, and it is not e2e coverage either.
The new platform branches have no tests, and packages/core/platform/tests has a suite for every comparable switch (bundles_multus_staging_test.yaml, bundles_networking_encryption_test.yaml). Nothing in that suite or in packages/system/cozystack-api/tests mentions networkingVariant, cozyplane or serveSDN. I'd expect at least these: the default renders kubeovn-cilium, an unknown value fails, cozyplane drops multus, kubeovn-webhook/plunger, securitygroup-controller and the VPC app, serveSDN: false removes only the sdn APIService, and the B3 guard. The new charts need a suite each that renders the Talos overlay, KubePrism host included.
B3: networkingVariant: cozyplane is accepted on variants where it breaks the cluster
The value is validated only inside the isp-full branch (system.yaml:43). Everything else it controls is gated without looking at variant. I rendered variant: isp-full-generic with networkingVariant: cozyplane. It renders without error. cozystack.networking stays on kubeovn-cilium-generic, so Cilium and kube-ovn still run. At the same time the bundle drops kubeovn-webhook, kubeovn-plunger, securitygroup-controller and virtualprivatecloud-application and turns off the sdn APIService in cozystack-api. It also adds cozyplane-cilium-crds next to the real Cilium and cozyplane-apiserver with no cozyplane CNI. isp-hosted and distro-full also get the last three changes.
The same gap applies to gateway.enabled: true on the variant. The default gateway.className is cilium, and nothing serves that class once Cilium is gone.
Fix: fail when networkingVariant != "kubeovn-cilium" and variant != "isp-full", and fail or require a different class when the gateway is enabled on cozyplane. Both need unit tests.
B4: tenant isolation stops being enforced and nothing says so
The tenant chart's isolation is CiliumNetworkPolicy and CiliumClusterwideNetworkPolicy (packages/apps/tenant/templates/networkpolicy.yaml). cozyplane-cilium-crds exists so those objects apply. Nothing on the variant enforces them, so every tenant install reports success while tenant-to-tenant traffic on the default network is open. The same is true for the linstor, monitoring-agents and ingress-nginx policies. The PR body lists this as a known gap, but an operator reads values.yaml, and there the option says only "(no Cilium, no kube-proxy)". For a multi-tenant platform that is the most important property of the choice. Before this leaves draft, the networkingVariant comment and the package docs need to say that tenant network isolation and the host firewall are not enforced on cozyplane.
B5: images come from a personal namespace with no version and no vendoring recipe
The tree already accepts third-party images from a personal namespace. Ouroboros and kuberture do it, so the namespace alone is not the problem. The problem is that nothing ties these pins to a release:
- The refs are digest-only.
docs/agents/image-refs.mdwants tag plus digest, and ouroboros carries0.8.1@sha256:…with a comment on mirroring for air-gapped installs. Upstream has no release tags, only floatingmainandmain-<sha>tags from pushes to main. Soghcr.io/lllamnyp/cozyplane:<version>@sha256:…is not possible yet. - The release job builds with
provenance: false,sbom: false, and does not sign. Only the upstream repo owner can push to that namespace. So the CNI, which runs privileged with hostNetwork on every node, has one person's account as its only supply-chain control. - The three charts are copied by hand. Other vendored packages have an
updatetarget that fetches a pinned upstream ref, and these Makefiles have none.Chart.yamlsays "the real version is stamped by the build/release pipeline", but nothing here stamps it. - The refs sit outside the promote, retag and nightly-mirror flows (third-party class), and no comment says who mirrors them.
Minimum before this leaves draft: an upstream release tag with the image tagged by it, repo:vX.Y.Z@sha256:… in values, and an update target that re-vendors the charts from that tag. The better long-term answer for a default-capable CNI is to build it under ghcr.io/cozystack in the project's release process.
B6: the PR body and commit history don't describe what ships
This repo merges with merge commits, so the body becomes the merge message and all 45 commits land on main. Two sections of the body are wrong against the diff:
- "Bootstrap ordering" describes the bootstrap-CRD takeover. Commit 46f2104 says the takeover is gone and the tenant kinds have no CRDs.
- "removes the old
cozyplane-ciliumvariant andsystem/cilium/values-cozyplane.yaml". Neither exists on main, and the diff removes nothing.
The commits need a rewrite before this leaves draft. Only one of the 45 subjects uses a Conventional Commits type. The rest look like cozyplane: bump to …, cozyplane: vpc-nat reply mark or cozyplane: repair the apiserver template. Eighteen have no body beyond the trailers. Several narrate the process instead of the reason, for example f6ccb5b "it was left out of the bump commit" and cb76b4e "the old 0.3.17 pin had that regression". A few commits split by component, with a WHY each, would do.
Non-blocking
sources/networking.yaml:153points todocs/kube-proxy-replacement.md, which lives upstream, not here. Line 155 says VM-guest ClusterIP does not work yet, while the body says net-0 ClusterIP DNAT is done.- There is no e2e lane for the variant. The in-tree e2e only runs
kubeovn-cilium. The upstreame2eworkflow ran on schedule on 2026-09-14 and 2026-09-21 and failed both times, so the 107/107 kind result has no CI run behind it. A lane isn't required for a draft, but it is needed before anyone can select this. - Size: about 13,450 of the 15,340 added lines are the two Cilium CRDs, vendored verbatim from 1.19.5. About 1,740 are the three vendored cozyplane charts and about 144 are platform and cozystack-api wiring. Reviewers here really own the wiring and the pins. The charts' substance is reviewed upstream, which makes B5 more important.
- The failing
Require API owner review for sizeable API changescheck is process, not a defect.
| # Digest-pinned (no floating tag). Built and published by .github/workflows/ | ||
| # release.yml on every push to main; see docs/packaging.md for how the digest is | ||
| # refreshed and why pinning it does not change it. | ||
| image: ghcr.io/lllamnyp/cozyplane@sha256:a50c456b3ea5aa5043c738a5cad7805707b6fb155b8f7a3d202f3b877590c325 |
There was a problem hiding this comment.
The linux/arm64 entry of this index (sha256:bb142456…) ships x86-64 binaries: agent, controller, apiserver, responder, gateway and the three CNI plugins. Only the Debian base is arm64. Upstream ARG TARGETARCH=amd64 pins GOARCH for both legs. See B1.
| @@ -0,0 +1,29 @@ | |||
| # cozyplane-kpr container image, built from kpr/ in this repository. | |||
| # Digest-pinned (no floating tag); built and pushed by release.yml's kpr-image | |||
| # job for amd64 and arm64, like the cozyplane image. docs/packaging.md has the | |||
There was a problem hiding this comment.
This says amd64 and arm64, but usr/bin/cozyplane-kpr in the arm64 entry sha256:cfe471d8… is x86-64. Same ARG TARGETARCH=amd64 cause in kpr/Dockerfile.
| cozyplane-kpr (socket-LB kube-proxy replacement) — no Cilium, no kube-proxy. */ -}} | ||
| {{- $networkingVariant := .Values.bundles.system.networkingVariant | default "kubeovn-cilium" -}} | ||
| {{- if not (or (eq $networkingVariant "kubeovn-cilium") (eq $networkingVariant "cozyplane")) -}} | ||
| {{- fail (printf "bundles.system.networkingVariant must be \"kubeovn-cilium\" or \"cozyplane\", got %q" $networkingVariant) -}} |
There was a problem hiding this comment.
This check only runs in the isp-full branch. With variant: isp-full-generic, cozyplane renders cleanly and strips the kube-ovn/securitygroup packages and the sdn APIService, while networking stays kubeovn-cilium-generic. See B3.
| # in that repo's chart/cozyplane) and references the published image in | ||
| # values.yaml. There is no image build target here. | ||
|
|
||
| test: |
There was a problem hiding this comment.
The unit-test sweep expects helm-unittest suites under tests/ when a package has a test target, so this is what turns CI red. Same in cozyplane-kpr and cozyplane-apiserver.
312ec1b to
7515d07
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Timofei Larkin (@lllamnyp) NOT LGTM. The arm64 images still carry x86-64 binaries, which was the main point of my last review. The new head only changes the image digest.
On the arm64 entry of ghcr.io/lllamnyp/cozyplane (digest starting sha256:3d87b7ad), crane export --platform linux/arm64 plus file shows cozyplane-apiserver, sdn-controller, cozyplane-gateway, cozyplane-responder and the CNI binaries built for x86-64. cozyplane-kpr (sha256:7417b9ad) was not rebuilt and is the same. The cause is upstream: ARG TARGETARCH=amd64 in Dockerfile:15,28 and kpr/Dockerfile:26 stops BuildKit from filling the platform arg. hack/verify-multiarch.sh only checks the platform list in the index, so CI won't catch it, while cozyplane-kpr/values.yaml:1-3 says the image is built for both.
The other points from that review are still open. There are no chart tests for the cozyplane branches, and the platform suite never sets networkingVariant or serveSDN. networkingVariant is gated on isp-full in bundles/system.yaml:4,42 but not at :183, :206, _helpers.tpl:131 or iaas.yaml:212. Tenant access to sdn.cozystack.io is neither enforced nor documented: the cozyplane-tenant-* roles aggregate into the built-in admin/edit/view, and Cozystack tenants don't get those. Images are pinned only by digest from a personal namespace with no update target, and registry.k8s.io/etcd:3.5.16-0 has no digest.
New in this round:
sdn.cozystack.io/v1alpha1SecurityGroup is served twice with different schemas, by cozystack-api and by cozyplane.docs/security/threat-model.md:57is wrong on this variant.- The apiserver role can get, list and delete any CRD and create or patch any APIService, with no
resourceNames(cozyplane-apiserver/templates/apiserver.yaml:82-91). A compromised pod could delete any CRD or take overv1alpha1.apps.cozystack.io. The controller can create Deployments cluster-wide (controller.yaml:100). - The PR body says etcd runs memory-backed, but the default is
persistent: true(values.yaml:34). The body becomes the merge commit, and 44 of the 45 commit subjects are not Conventional Commits.
Not blocking: the apiserver runs one replica, so the whole API group goes down with it, and the controller runs with --leader-elect=false. There is a small conflict with main in cozystack-api/values.yaml, and the website and ansible-cozystack follow-ups are not ticked.
7515d07 to
03a5713
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Timofei Larkin (@lllamnyp) NOT LGTM. The arm64 image entries still carry x86-64 binaries, the apiserver RBAC is still not scoped, the branch conflicts with main, and the variant ignores the platform's pod and service CIDRs. Reviewed at 03a5713.
The tests, the variant guard and the "not enforced" wording are done. The new suites pass, and the mutations I tried are caught: dropping the isp-full check, setting serveSDN back to true, and changing the KubePrism port each fail a test. The SecurityGroup GVK collision is closed within one cluster, because serveSDN: false removes only the sdn APIService and apiservice_test.yaml pins that.
The default path is still safe. I rendered the platform chart at this head and at its merge base for all four system variants, with only the system bundle and with every bundle on. No Package changed. The only additions are the two new PackageSources and the cozyplane entry in cozystack.networking. The dependsOn graph of the cozyplane render has no cycles and no missing packages. CI on this head is green except the API owner gate, which is process.
Blockers
B1: the arm64 entries are still amd64
This push didn't change the digests. In ghcr.io/lllamnyp/cozyplane@sha256:3d87b7ad…, the linux/arm64 entry sha256:add6c72a0f2d… has the agent, apiserver, gateway, responder, sdn-controller and the three CNI binaries built as ELF x86-64. The arm64 entry of cozyplane-kpr@sha256:7417b9ad… is the same. The vendoring ref COZYPLANE_REF=ac934e14 still has ARG TARGETARCH=amd64 in kpr/Dockerfile:26.
Upstream fixed it about 40 minutes after this head was pushed. e44cb7eda drops the default, and 65bade1c3 repins to cozyplane@sha256:60111aff… and cozyplane-kpr@sha256:41a76cbc…. I checked the arm64 entries of both new digests, and the agent, host-local and cozyplane-kpr there are ARM aarch64. Bumping COZYPLANE_REF to 65bade1c3 or later and running make update in the three packages should be enough. I diffed the vendored charts against ac934e14, and only the Chart.yaml version stanza differs, so the recipe reproduces the tree.
B2: the apiserver and controller RBAC is still cluster-wide
cozyplane-apiserver/templates/apiserver.yaml:83-91 still grants get, create, patch on every APIService and get, list, delete on every CRD, with no resourceNames. cozyplane/templates/controller.yaml:100 still lets the controller create and delete Deployments in any namespace. Upstream 2cb23e62a fixes all three. It scopes get and patch to v1alpha1.sdn.cozystack.io, limits delete to the listed CRD names, keeps create as its own rule (RBAC can't restrict create by name), and moves the Deployment writes into a namespaced Role. The same re-vendor as B1 brings it in.
B3: merge conflicts with main
packages/core/platform/values.yaml, templates/_helpers.tpl and templates/bundles/system.yaml conflict. Since the merge base, main added the isp-slim, isp-slim-generic and isp-hosted-slim variants, dropped distro-full, and moved the common packages to cozystack.platform.package.full.default. The guard at system.yaml:9 only accepts exactly isp-full, so the resolution has to decide what the slim variants do, and the networking-variant suite needs a case for one of them.
B4: the variant ignores networking.podCIDR and networking.serviceCIDR
The platform passes its network settings only to the kube-ovn and Cilium components, and the cozyplane variant installs neither. system.yaml:22-53 builds $networkingComponents for kubeovn and cilium only. So cozyplane always runs with the chart defaults in cozyplane/values.yaml:45-53: clusterCIDR: 10.244.0.0/16, serviceCIDR: 10.96.0.0/16, clusterDNS: 10.96.0.10. Those values feed --cluster-cidr and --internal-cidrs in agent.yaml:167-177 and controller.yaml:159-163.
On a cluster with a non-default pod or service CIDR, the masquerade matches the wrong source range. VPC gateways block the wrong internal ranges, and their one DNS opening points at an address with no DNS behind it. Fix: on the cozyplane variant, set the component's egress.clusterCIDR and egress.serviceCIDR from .Values.networking. clusterDNS has no platform value today, so either derive it from the service CIDR or add one. A platform test with a non-default CIDR should cover it.
B5: the PR body and the commits still don't match what ships
This repo merges with merge commits, so the body becomes the merge message and all 50 commits land on main.
- "Bootstrap ordering" (body lines 30-62) says the CNI serves
sdn.cozystack.ioas bootstrap CRDs and the apiserver takes over from them.system.yaml:227says "There is no takeover: the tenant kinds have no CRDs at all", andcozyplane/tests/talos_render_test.yamlasserts that onlylocal.sdn.cozystack.ioCRDs ship. - The body still says it "removes the old
cozyplane-ciliumvariant andsystem/cilium/values-cozyplane.yaml". Neither exists on main, and the diff removes nothing. - It still says the apiserver's etcd "runs memory-backed".
cozyplane-apiserver/values.yaml:34defaults topersistent: true. - The body is hard-wrapped at about 80 columns. AGENTS.md (Prose Formatting) asks for one line per paragraph.
- 44 of the 50 subjects are not Conventional Commits. Some carry upstream issue numbers as bare refs, like
(#15)in f63c4b1 and 22986e5,(#14 increment 0)in a56ebae and#11in ed2bd4b. On this repo GitHub links them to unrelated cozystack issues. Others use planning labels instead of a reason, like "increment 3 Half A" in 63aca5b or "R5" in 8e79252. A few commits split by component, each with a WHY, would fix this.
Still open, not blocking a draft
- Cozystack tenants still can't reach the variant's tenant API.
cozystack-basics/templates/clusterroles.yaml:36,93,285grants onlysecuritygroupsinsdn.cozystack.io, and here that is cozyplane's kind with a different schema. VPCs, bindings, peerings, gateways and floating IPs come only throughaggregate-to-admin/edit/view, which tenant users are not bound to. The bundle also dropsvirtualprivatecloud-application, so the variant has no tenant-facing VPC app. The work from the closed #4034 would land here or in a follow-up. registry.k8s.io/etcd:3.5.16-0atcozyplane-apiserver/values.yaml:40has no digest, and the cozyplane refs are digest-only. Upstream now cutsvX.Y.Ztags (24d380910), so arepo:vX.Y.Z@sha256:…pin and a tag asCOZYPLANE_REFare possible before this leaves draft.make updatewrites "the real version is stamped by the build/release pipeline" into eachChart.yaml, but nothing stamps it.CHART_VERSIONis hard-coded in the Makefile.cozyplane-cilium-crdsis the only package underpackages/systemwithout a Makefile. Its two CRDs are copied from Cilium 1.19.5 with no recipe, so they will drift from the cilium package without anyone noticing.
packages/system/cozyplane vendors chart/cozyplane from lllamnyp/cozyplane (ref-pinned; make update re-vendors and re-applies the Cozystack-side Chart.yaml version stanza). The CNI slot installs before everything else, so its dependency floor is the kube API alone: the chart ships only the LOCAL group's CRDs (local.sdn.cozystack.io, underlay IPAM) — the tenant kinds are served by the aggregated apiserver, packaged separately. On Talos the agent and resolver bootstrap against KubePrism (localhost:7445), because with no kube-proxy the kubernetes.default ClusterIP is unserved until cozyplane-kpr runs, and kpr needs the agent. The helm-unittest suite pins exactly that platform contract: the KubePrism endpoint, the 00- conflist precedence over a co-installed CNI, and the LOCAL-group-only CRDs. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
cozyplane-kpr imports Cilium's LB control plane and socket-LB as the cluster's only service proxy — a separate component and image from the CNI agent on purpose, since it carries Cilium's cell framework. It dials KubePrism on Talos for the same bootstrap reason the agent does, needs its node name (downward API) to tell node-local endpoints apart — without it no external address is served — and keeps DSR strictly opt-in, because the fleet-wide LB-IP spoof permission DSR needs is an underlay property. The helm-unittest suite pins those three. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
The aggregated apiserver serves sdn.cozystack.io (VPC tenancy, policy) and is a separate package precisely so the CNI slot stays cert-manager-free: this component needs cert-manager Certificates, the etcd-operator and, for its PVC-backed etcd, storage — with no cycle, since linstor depends on the CNI rather than on this. The tenant kinds deliberately have no CRDs: one group served by both a CRD and an APIService collides in the kube-apiserver's OpenAPI merge and the group's schema never serves. The etcd is an etcd-operator v1alpha2 EtcdCluster in certManager TLS mode, PVC-backed on the replicated class by default (its objects are cluster state; a memory-backed member loses them on a full-cluster restart), addressed through the operator's Service because the operator names member pods randomly. The helm-unittest suite pins the EtcdCluster shape and the Service endpoint. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
Many stock components (linstor, tenant, monitoring-agents, ...) embed CiliumNetworkPolicy objects and fail to install without the CRDs. On the Cilium variants the cilium chart provides them; the cozyplane variant runs no Cilium, so this package ships the two policy CRDs (vendored verbatim from Cilium 1.19.5) purely so those manifests apply. Nothing enforces them — the platform docs and threat model say so, and rebuilding tenant isolation on what cozyplane does enforce (native NetworkPolicy, HostFirewall) is a follow-up. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
cozyplane serves sdn.cozystack.io through its own aggregated apiserver; a second APIService for the group from cozystack-api would hijack it and 404 every kind but SecurityGroup. cozystackAPI.serveSDN (default true, so nothing changes off the cozyplane variant) lets the platform turn exactly that APIService off; the suite pins that the toggle removes it and nothing else. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
An opt-in bundles.system.networkingVariant for the isp-full (Talos) bundle: "cozyplane" swaps kube-ovn, Cilium and kube-proxy for the cozyplane eBPF CNI, its aggregated sdn.cozystack.io apiserver and the cozyplane-kpr socket-LB proxy. The default "kubeovn-cilium" renders identically to main. The variant drops the packages that assume the replaced stack — kubeovn-webhook/-plunger, multus, securitygroup-controller, the virtualprivatecloud app — and turns off sdn aggregation in cozystack-api, since cozyplane owns the group. The selector is validated before any branch is taken: accepted off isp-full it would half-apply, stripping the kube-ovn control plane and the sdn APIService while the Cilium networking package keeps running; and the default "cilium" gateway class is refused on cozyplane, where nothing serves it. The platform's networking.podCIDR and serviceCIDR (and clusterDNS, derived from the service CIDR when not set explicitly) are threaded into the cozyplane component, so the egress masquerade and the VPC gateways' internal denies match the cluster rather than the chart defaults. CiliumNetworkPolicy objects apply but are not enforced on this variant; the values comment, the networking source and the threat model say so, and the threat model records the SecurityGroup split: cozyplane's is a first-class tenant instrument for traffic inside and across VPCs, not cozystack-api's projection onto CiliumNetworkPolicy. Tenant isolation rebuilt on native NetworkPolicy + HostFirewall is a follow-up. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
03a5713 to
23c3c54
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Draft opt-in cozyplane variant. The default kubeovn-cilium path is unchanged (231 platform unit tests green, 3 mutations covered, Package set identical). Two confirmed defects in the new variant, an acknowledged default-network isolation gap, and the unmet API-owner gate keep it from merging.
Findings
- [MAJOR]
packages/system/cozyplane/templates/tenant-rbac.yaml:21, tenant VPC grant aggregates into built-in roles no tenant is bound to
- [MAJOR]
packages/system/cozyplane-kpr/templates/daemonset.yaml:28, kpr ClusterRole wildcards all of cilium.io with mutating verbs
- [MINOR]
packages/core/platform/templates/bundles/system.yaml:76, empty podCIDR silently drops masquerade; non-IPv4 serviceCIDR panics illegibly
- [MINOR]
packages/system/cozyplane-apiserver/values.yaml:38, EtcdCluster storageClass replicated is hardcoded and immutable
- [MINOR]
packages/system/cozyplane/templates/controller.yaml:194, controller.replicas settable while --leader-elect=false is hardcoded
- [MINOR]
packages/system/cozyplane/templates/agent.yaml:183, unauthenticated metrics on hostPort 9411 with host firewall dropped
- [MINOR]
packages/system/cozyplane/templates/agent.yaml:22, multi-paragraph prose in # comments ships into rendered manifests
- [MINOR]
packages/system/cozyplane-cilium-crds/Chart.yaml:2, no Makefile / re-vendor recipe for the vendored Cilium CRDs
Operational risks
- Default-network tenant-to-tenant isolation is NOT enforced on the cozyplane variant (CiliumNetworkPolicy ships inert; native-NetworkPolicy enforcement and the RBAC for it are not wired yet). Disclosed in
docs/security/threat-model.md,values.yamland the PR body. Keep the variant out of multi-tenant production until isolation is rebuilt (author-acknowledged follow-up).
Caveats
- Switching
networkingVarianton a live cluster is destructive: the operator's orphan-HR cleanup removes the cilium/kube-ovn releases whileresource-policy: keepPackages linger. Not an in-place operation; add alookup-guard that fails on a variant change, or document it as install-time only. Not verified in Go (no.goin this diff). - CI red "Require API owner review" is a governance gate, not a code defect: self-approval is forbidden and the author is an API owner, so a second owner must approve.
- The two
denypolicy-identity-reachabilitylint hits are false positives: both files arekind: CustomResourceDefinitiononly, everyingressDenyis an OpenAPI schema property name, no policy instance ships. - Multi-arch image legs are unverifiable in a hermetic review; the digests are the fixed refs.
docker manifest inspect ghcr.io/lllamnyp/cozyplane@sha256:60111af...settles it. - Cilium CRDs confirmed CRD-only and inert; a byte-match to upstream 1.19.5 was not fetched.
Recommended follow-ups
- Before leaving draft: move the images off the personal
ghcr.io/lllamnyp/*namespace into the cozystack org registry, digest-pinregistry.k8s.io/etcd:3.5.16-0, and add the variant's e2e lane.
| name: cozyplane-tenant-edit | ||
| labels: | ||
| {{- include "cozyplane.labels" . | nindent 4 }} | ||
| rbac.authorization.k8s.io/aggregate-to-admin: "true" |
There was a problem hiding this comment.
[MAJOR] tenant VPC grant aggregates into built-in roles no tenant is bound to
cozyplane-tenant-edit/-view carry only rbac.authorization.k8s.io/aggregate-to-{admin,edit,view}, but cozystack binds tenants to cozy:tenant:, whose aggregationRule (clusterroles.yaml:225-228 etc.) selects only rbac.cozystack.io/aggregate-to-tenant-. Nothing selects the built-in aggregate-to-* labels, so these roles aggregate into no tenant tier. On the cozyplane variant a tenant-admin gets zero rights on vpcs/vpcbindings/vpcgateways/vpcpeerings/floatingips and no export/peer verbs, and bundles/iaas.yaml:267 drops virtualprivatecloud-application, so VPC self-service is Forbidden for every tenant, only cluster-admin can use the feature. Fix: label these roles rbac.cozystack.io/aggregate-to-tenant-{admin,use,view} (or bind them explicitly to the tenant tiers).
| verbs: [create, patch] | ||
| # Cilium's imported cells reconcile their own CRDs (CiliumNode, LB IP pools, …). | ||
| - apiGroups: [cilium.io] | ||
| resources: ["*"] |
There was a problem hiding this comment.
[MAJOR] kpr ClusterRole wildcards all of cilium.io with mutating verbs
The kpr ClusterRole grants cilium.io: ["*"] with get/list/watch/create/update/patch/delete, so a per-node DaemonSet ServiceAccount can mutate every current and future cilium.io kind cluster-wide (CiliumNetworkPolicy/CCNP included), while the inline comment names only CiliumNode and LB IP pools. The file is vendored from upstream (COZYPLANE_REF=65bade1c3), so narrow it via a patches/ entry or upstream to the kinds actually reconciled. Blast radius is limited on this variant (Cilium is not running), but least-privilege is the bar. What would change my mind: parity with upstream cilium-agent RBAC makes this MINOR.
| {{- $egress := dict "clusterCIDR" .Values.networking.podCIDR "serviceCIDR" .Values.networking.serviceCIDR -}} | ||
| {{- $clusterDNS := dig "clusterDNS" "" .Values.networking -}} | ||
| {{- if not $clusterDNS -}} | ||
| {{- $svcBase := splitList "/" .Values.networking.serviceCIDR | first -}} |
There was a problem hiding this comment.
[MINOR] empty podCIDR silently drops masquerade; non-IPv4 serviceCIDR panics illegibly
clusterDNS is derived by splitting serviceCIDR into octets and printing %s.%s.%s.10. An empty or IPv6-first serviceCIDR fails the render with index out of range instead of naming the field. Worse, an empty networking.podCIDR renders clusterCIDR="", which drops the agent's --cluster-cidr/masquerade and the controller's --gateway-image/--internal-cidrs/--cluster-dns with no error (verified by render: controller keeps only three base args). Not an upgrade regression (merge base rendered the same inputs via kube-ovn), but the unhappy path is silent. Validate the CIDRs and emit a legible render-time fail.
| # StorageClass for the members' PVCs ("" = cluster default). Replicated, so | ||
| # a member's data survives losing the node it was scheduled on. Cozystack's | ||
| # storage layers (linstor, blockstor) both provide a "replicated" class. | ||
| storageClass: "replicated" |
There was a problem hiding this comment.
[MINOR] EtcdCluster storageClass replicated is hardcoded and immutable
The operator-managed EtcdCluster hardcodes storageClass: replicated; EtcdCluster.storageClassName is immutable post-create (etcd-operator-crds). A fresh install where that class is absent (admin creates it after pool setup) or named differently leaves the PVC Pending, the apiserver down on storage deadline, and the Package not Ready, recoverable only by deleting the EtcdCluster, i.e. all sdn objects. Precedent for a hardcoded replicated exists in other system packages, but not with an immutable field holding cluster state. Consider defaulting to the cluster default like extra/etcd, or documenting the hard dependency.
| imagePullPolicy: {{ .Values.imagePullPolicy }} | ||
| command: ["/usr/local/bin/sdn-controller"] | ||
| args: | ||
| - --leader-elect=false |
There was a problem hiding this comment.
[MINOR] controller.replicas settable while --leader-elect=false is hardcoded
--leader-elect=false is hardcoded while controller.replicas (values.yaml:74) is operator-settable and defaults to 1. Raising replicas runs two uncoordinated controllers both writing Services/EndpointSlices/Ports and deleting pods, a silent HA correctness bug. The default is safe; either pin replicas to 1 with a comment, or gate --leader-elect on replicas>1.
| # Per-VPC traffic metrics (#2), Prometheus text at /metrics. | ||
| - name: metrics | ||
| containerPort: 9411 | ||
| hostPort: 9411 |
There was a problem hiding this comment.
[MINOR] unauthenticated metrics on hostPort 9411 with host firewall dropped
The agent exposes metrics on hostPort: 9411 under hostNetwork with no auth. The Cilium HostFirewall (cilium-networkpolicy) that guards the other variants is absent on cozyplane (sources/networking.yaml), so on nodes with a public address the per-VPC traffic counters are readable from outside. Bind the listener to localhost, or document and firewall it.
| - apiGroups: [""] | ||
| resources: ["nodes"] | ||
| verbs: ["get", "list", "watch", "patch"] | ||
| # The plugin (using this SA) reads pod annotations to resolve VPC membership, |
There was a problem hiding this comment.
[MINOR] multi-paragraph prose in # comments ships into rendered manifests
This block (and tenant-rbac.yaml) carries multi-line explanatory prose in # YAML comments. Unlike {{/* */}}, # comments are not stripped at render, so the essay is copied verbatim into every applied manifest and drifts from the code. Move the narrative to the cozystack docs site and leave a one-line pointer. The RBAC-scope comments in controller.yaml are borderline clarifying and can stay.
| @@ -0,0 +1,10 @@ | |||
| apiVersion: v2 | |||
| name: cozyplane-cilium-crds | |||
There was a problem hiding this comment.
[MINOR] no Makefile / re-vendor recipe for the vendored Cilium CRDs
This package ships two Cilium CRDs copied from upstream (1.19.5 per the PR) but has no Makefile, unlike the other three cozyplane packages. There is no recorded re-vendor recipe, so the CRDs will drift from the cilium package's upstream bumps with no signal. Add a Makefile with the pull/pin recipe.
An empty networking.podCIDR rendered an empty egress.clusterCIDR, which silently drops the agent's masquerade and the controller's internal denies; a non-IPv4 serviceCIDR made the clusterDNS octet derivation die with an index error that names no field. Refuse both legibly: the variant requires podCIDR and serviceCIDR, and a service CIDR the derivation cannot read requires an explicit networking.clusterDNS. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
The chart default is the replicated class, written into the EtcdCluster's storageClassName — a field that is immutable post-create. A fresh install without that class leaves the PVCs Pending and the sdn apiserver down, recoverable only by deleting the EtcdCluster and with it every sdn object. The platform now sets the class on the component: local, because a 3-member etcd already replicates at the application level and the local class exists before any replication pool is configured. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
The two policy CRDs were copied by hand with nothing recording where from, so they would drift from the cilium package's upstream bumps with no signal. The Makefile pins the Cilium release (the in-tree files are byte-identical to its upstream CRDs) and make update re-fetches them. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Round 2, now covering the networking substance the first pass could not reach: I read the datapath from lllamnyp/cozyplane@65bade1c3 and mutation-tested the chart wiring. Thanks for the fast turnaround: the 3 fixes you pushed (input validation, etcd SC local, cilium-crds Makefile) close my earlier system.yaml / etcd / vendoring notes. Three blockers remain, two new from the networking review.
Findings
- [MAJOR]
packages/core/platform/templates/bundles/system.yaml:253, inert Cilium policies disable platform self-protection, wider than threat model discloses
- [MAJOR]
packages/system/cozyplane/templates/agent.yaml:171, egress/isolation datapath has no test coverage (mutation-proven)
- [MAJOR]
packages/system/cozyplane/templates/tenant-rbac.yaml:21, tenant VPC grant still never reaches tenants (unchanged since round 1)
- [MINOR]
packages/system/cozyplane/values.yaml:56, default internalCIDRs omits the node/management network
- [MINOR]
packages/core/platform/sources/networking.yaml:166, variant hard-requires Talos/KubePrism with no precondition guard
- [MINOR]
packages/system/cozyplane-kpr/templates/daemonset.yaml:28, kpr ClusterRole wildcards all of cilium.io with mutating verbs (downgraded from round 1 MAJOR)
- [MINOR]
packages/system/cozyplane/templates/controller.yaml:194, controller.replicas settable while --leader-elect=false is hardcoded
- [MINOR]
packages/system/cozyplane/templates/agent.yaml:183, unauthenticated metrics on hostPort 9411 with host firewall dropped
- [MINOR]
packages/system/cozyplane/templates/agent.yaml:22, multi-paragraph prose in # comments ships into rendered manifests
Operational risks
- Default-network tenant isolation is off AND platform-component CNP self-protection is inert (MAJOR 1). Keep the variant out of multi-tenant production until isolation is rebuilt;
ingressDeny-class protections need more than native NetworkPolicy.
Caveats
- Datapath notes from the source read (
lllamnyp/cozyplane@65bade1c3): default-network KubeVirt VMs load-balance across at most 16 Service backends silently (kpr/services.go:60), and SCTP ClusterIP Services are unsupported (kpr/services.go:114), a narrow regression vs the kube-proxy being replaced. Both deserve a docs note. - NIT: the new input-validation guard (
5389123ad) rejects an IPv4-primary dual-stack serviceCIDR like10.96.0.0/16,fd00::/112with its IPv4-only regex; the pre-fix code derived clusterDNS from the leading v4. Legible failure, so minor, but stricter than needed. - Live traffic (ClusterIP/NodePort/LB, pod-to-pod, VM-to-service, DNS, isolation) is NOT verified: swapping the CNI on a running cluster is irreversible and no disposable Talos stand was available, so the datapath is confirmed only by the source read above, not by a run. A fresh 3-node Talos stand is needed to prove packets actually move. This unverified surface is itself a blocking caveat, not a silent pass.
Recommended follow-ups
- Digest-pin
registry.k8s.io/etcd:3.5.16-0; add the variant's e2e lane (there is none in-tree today); before leaving draft move images off the personalghcr.io/lllamnyp/*namespace.
| — host-firewall / default-network NetworkPolicy is a follow-up). On the | ||
| Cilium variants the cilium chart provides the CRDs, so skip it there. */ -}} | ||
| {{- if eq (.Values.bundles.system.networkingVariant | default "kubeovn-cilium") "cozyplane" }} | ||
| {{include "cozystack.platform.package.default" (list "cozystack.cozyplane-cilium-crds" $) }} |
There was a problem hiding this comment.
[MAJOR] inert Cilium policies disable platform self-protection, wider than threat model discloses
Shipping cozyplane-cilium-crds so stock manifests apply means every platform-component CiliumNetworkPolicy/CCNP is inert on this variant, not only tenant-to-tenant. Enumerated carriers: linstor CCNP restrict-drbd-reactor (ingressDeny on DRBD port 9942), ingress-nginx admission-webhook-{networkpolicy,egress-policy}, keda (manager/webhooks/metrics-server), monitoring-agents + kube-state-metrics, talos-log-collector. $ grep -rln 'kind: Cilium.*NetworkPolicy' packages/system | grep -vE 'cozyplane-cilium-crds|/charts/cilium/|cilium-networkpolicy|kubeovn' -> the 10 files above; $ sed -n '3,12p' packages/system/linstor/templates/networkpolicy.yaml -> kind: CiliumClusterwideNetworkPolicy ... ingressDeny ... port 9942. A pod on the flat default network can reach e.g. the linstor DRBD control port. ingressDeny-class rules have no native-NetworkPolicy equivalent, so the rebuild-on-native-NP follow-up cannot restore them. docs/security/threat-model.md should state platform-component protections are unenforced too. (Live reachability belongs to a cozystack-pr-test run on a disposable stand; the enforcement-gone fact is static.)
| # Cluster-egress masquerade: pod traffic leaving the cluster is | ||
| # SNATed to the node address, giving pods an off-cluster return path. | ||
| - "--cluster-cidr={{ .Values.egress.clusterCIDR }}" | ||
| - "--masquerade={{ .Values.egress.masquerade | default "bpf" }}" |
There was a problem hiding this comment.
[MAJOR] egress/isolation datapath has no test coverage (mutation-proven)
14 mutations vs the chart suites. Protected (RED): clusterDNS derivation, CIDR threading, serveSDN, KubePrism port, conflist priority, etcd SC. UNPROTECTED (GREEN when emptied/flipped): --masquerade, whether --cluster-cidr is emitted at all, the composed --internal-cidrs on agent (agent.yaml:177) and controller (controller.yaml:199), MTU (values.yaml:10). Reproduced: $ perl -i -pe 's/--cluster-cidr={{ .Values.egress.clusterCIDR }}/--cluster-cidr=/' agent.yaml && helm unittest packages/system/cozyplane -> Tests: 4 passed, 4 total (still green). Verified vs lllamnyp/cozyplane@65bade1c3:bpf/overlay.c:3304 that empty --cluster-cidr drops all SNAT and empty --internal-cidrs disables the gateway internal denies. These knobs carry the variant's whole egress/floating-isolation guarantee and a template typo ships silently. Pin --cluster-cidr, --masquerade, and the composed --internal-cidrs on agent and controller in cozyplane/tests/talos_render_test.yaml.
| name: cozyplane-tenant-edit | ||
| labels: | ||
| {{- include "cozyplane.labels" . | nindent 4 }} | ||
| rbac.authorization.k8s.io/aggregate-to-admin: "true" |
There was a problem hiding this comment.
[MAJOR] tenant VPC grant still never reaches tenants (unchanged since round 1)
cozyplane-tenant-edit/-view carry only rbac.authorization.k8s.io/aggregate-to-{admin,edit,view}, but cozystack binds tenants to cozy:tenant:, whose aggregationRule selects only rbac.cozystack.io/aggregate-to-tenant-. $ grep -n 'name: cozy:tenant:admin' -A6 clusterroles.yaml -> 225 aggregationRule / 228 rbac.cozystack.io/aggregate-to-tenant-admin; $ grep -n 'name: admin|edit|view' packages/apps/tenant/templates/tenant.yaml -> no output (tenants bind only to cozy:tenant:*). So a tenant-admin gets zero rights on vpcs/vpcbindings/vpcgateways/vpcpeerings/floatingips and no export/peer verbs, and iaas.yaml drops virtualprivatecloud-application here: VPC self-service is Forbidden for every tenant, only cluster-admin can use the feature. Existing tenant securitygroups access is a direct rule in cozy:tenant, the idiom to follow. Fix: label these roles rbac.cozystack.io/aggregate-to-tenant-{admin,use,view}.
| clusterDNS: "10.96.0.10" | ||
| # Additional cluster-internal networks VPC tenants must not reach through | ||
| # their gateways (e.g. the node/management network). | ||
| internalCIDRs: [] |
There was a problem hiding this comment.
[MINOR] default internalCIDRs omits the node/management network
egress.internalCIDRs defaults to [] (its own comment names the omission: the node/management network), and the platform egress dict never sets it. The chart composes --internal-cidrs = clusterCIDR + serviceCIDR + internalCIDRs, so a VPC pod with a FloatingIP egressing on its public IP is not denied toward the node/management network. Default internalCIDRs to include the node CIDR, or document it in threat-model.md as required hardening.
| path: system/cozyplane | ||
| valuesFiles: | ||
| - values.yaml | ||
| - values-talos.yaml |
There was a problem hiding this comment.
[MINOR] variant hard-requires Talos/KubePrism with no precondition guard
sources/networking.yaml always applies values-talos.yaml for cozyplane and cozyplane-kpr, pinning KubePrism localhost:7445. Nothing checks the cluster is Talos. On a non-Talos isp-full cluster the agent/responder/kpr dial localhost:7445, which does not exist, so the apiserver is unreachable and the CNI never goes ready, buried in the bootstrap path. Add a precondition guard or fail loudly.
| verbs: [create, patch] | ||
| # Cilium's imported cells reconcile their own CRDs (CiliumNode, LB IP pools, …). | ||
| - apiGroups: [cilium.io] | ||
| resources: ["*"] |
There was a problem hiding this comment.
[MINOR] kpr ClusterRole wildcards all of cilium.io with mutating verbs (downgraded from round 1 MAJOR)
The kpr ClusterRole grants cilium.io: ["*"] with get/list/watch/create/update/patch/delete while the comment names only CiliumNode and LB IP pools. Downgraded from my round-1 MAJOR: vendored upstream (COZYPLANE_REF=65bade1c3) and Cilium is not running here, so blast radius is small (only the inert CRDs and what kpr itself creates). Still worth narrowing to the kinds actually reconciled, via a patches/ entry or upstream.
| imagePullPolicy: {{ .Values.imagePullPolicy }} | ||
| command: ["/usr/local/bin/sdn-controller"] | ||
| args: | ||
| - --leader-elect=false |
There was a problem hiding this comment.
[MINOR] controller.replicas settable while --leader-elect=false is hardcoded
--leader-elect=false is hardcoded while controller.replicas (values.yaml:74) is operator-settable and defaults to 1. Raising replicas runs two uncoordinated controllers both writing Services/EndpointSlices/Ports and deleting pods, a silent HA correctness bug. Pin replicas to 1 with a comment, or gate --leader-elect on replicas>1.
| # Per-VPC traffic metrics (#2), Prometheus text at /metrics. | ||
| - name: metrics | ||
| containerPort: 9411 | ||
| hostPort: 9411 |
There was a problem hiding this comment.
[MINOR] unauthenticated metrics on hostPort 9411 with host firewall dropped
The agent exposes metrics on hostPort: 9411 under hostNetwork with no auth, and the Cilium HostFirewall (cilium-networkpolicy) that guards the other variants is absent on cozyplane, so on nodes with a public address the per-VPC traffic counters are readable from outside. Bind the listener to localhost, or document and firewall it.
| - apiGroups: [""] | ||
| resources: ["nodes"] | ||
| verbs: ["get", "list", "watch", "patch"] | ||
| # The plugin (using this SA) reads pod annotations to resolve VPC membership, |
There was a problem hiding this comment.
[MINOR] multi-paragraph prose in # comments ships into rendered manifests
This block (and tenant-rbac.yaml) carries multi-line explanatory prose in # YAML comments. Unlike {{/* */}}, # comments are not stripped at render, so the essay is copied verbatim into every applied manifest and drifts from the code. Move the narrative to the cozystack docs site and leave a one-line pointer.
|
Follow-up: I validated the cozyplane datapath on a throwaway 2-node kind cluster (no Talos, no kube-proxy, default CNI disabled), since a static review can't prove packets actually move. Datapath works off-Talos, and a retraction. pod-to-pod (same node and cross node, over the geneve overlay), ClusterIP (load-balanced to a cross-node backend), cluster DNS, and bpf-masquerade egress all pass. So my round-2 note that the variant "hard-requires Talos/KubePrism" is wrong in its strong form: the only Talos coupling is the bootstrap apiserver endpoint, and it is fully overridable via chart values ( New finding: NodePort/LoadBalancer VIPs are silently left unprogrammed on a startup race. On a cold install NodePort did not work until I restarted the kpr DaemonSet. Root cause, confirmed in source at Not tested here (out of kind's reach): VPC isolation / peering / gateway / ServiceVIP (need the sdn apiserver + controller) and KubeVirt-VM datapath (needs nested virtualization). Verdict unchanged. |
|
Follow-up 2: stood up the SDN layer (sdn.cozystack.io apiserver + controller) on the same throwaway kind cluster and exercised the VPC datapath. The VPC model works, verified live. Cross-VPC traffic by VPC address is dropped both ways on the eBPF datapath (not just by an API policy): intra-VPC cross-node allowed, cross-VPC denied, VPCPeering dynamically opens it and deleting the peering re-isolates, and a VPC-scoped ServiceVIP is reachable only from within the VPC or a peered one. Two refinements to my default-network-isolation finding (wording, not the gap itself):
Not exercised: SecurityGroup / HostFirewall, FloatingIP / VPCGateway egress, and the KubeVirt-VM datapath (nested virtualization unavailable in my sandbox). |
Brings in the upstream review fixes (lllamnyp/cozyplane#52-#54): kpr's cilium.io RBAC wildcard is dropped outright — the wired cells watch only Services/EndpointSlices and node-local Pods/Namespaces, proven on a cluster serving no cilium.io group at all; controller leader election is on with its leases Role, so raising replicas can no longer run uncoordinated writers; the agent's per-VPC metrics are authenticated (TokenReview/SubjectAccessReview, secure by default, fail-closed) and bound to the node's primary address — the values file documents that an existing anonymous scrape gets 401 until the shipped cozyplane-metrics-reader ClusterRole is bound to the monitoring ServiceAccount; and the chart comments that shipped inside rendered manifests are trimmed to mechanism statements with doc pointers. The cozyplane image pin moves to the build carrying the agent changes; the kpr binary is unchanged, so its pin stays. Assisted-by: LLM Signed-off-by: Timofei Larkin <[email protected]>
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Timofei Larkin (@lllamnyp) NOT LGTM, only because the branch conflicts with main again. Everything else that blocked in my last review is fixed at 484c0655.
What I checked:
- The arm64 leg of
cozyplane@sha256:880db757...(sha256:70bb1821...) now carries ARM aarch64 binaries. I extracted the layers and ranfileon the agent, apiserver, gateway, responder, sdn-controller and all three CNI plugins.cozyplane-kpr@sha256:41a76cbc...is the build I already checked last time. Both digests match what upstream pins at2608ed75. - The apiserver role names
v1alpha1.sdn.cozystack.ioforget/patchand lists the CRD names fordelete. The Deployment writes moved into a namespaced Role. - The platform threads
podCIDR,serviceCIDRand the derivedclusterDNSinto the component, and the platform suite covers that, the explicit override and both refusals. - The commits and the PR body now describe what ships.
On the PR head, helm unittest passes for platform (235), cozystack-api (16), cozyplane (4), cozyplane-kpr (3) and cozyplane-apiserver (4).
Blocker
packages/core/platform/templates/_helpers.tpl and templates/bundles/system.yaml conflict with current main, and GitHub reports the PR as CONFLICTING. The cause is #4778, which landed after your rebase. It adds the ciliumPool helpers next to the securitygroup-controller block. In system.yaml it also rewrites the end of the kube-ovn/Cilium values block, so kube-ovn excludes the last /21 of the pod CIDR and Cilium allocates from it. Keep those lines inside the kubeovn-cilium path. The /20 or shorter podCIDR check there must not fire on cozyplane, which has no kube-ovn. One test with networkingVariant: cozyplane and a /24 pod CIDR would pin that.
Not blocking
- Red
Unit & controller testson this head (2026-10-07T12:46Z, attempt 1) ishack/bats-strict-setup.batscomplaining abouthack/fork-new-images.bats. That file is not in this diff, and main fixed it in 0ea11b4. A rebase clears it.Require API owner reviewis process. registry.k8s.io/etcd:3.5.16-0incozyplane-apiserver/values.yamlstill has no digest, and eachChart.yamlstill says the version "is stamped by the build/release pipeline" while nothing stamps it.- On IvanHunters' last review: the kpr
cilium.iowildcard, the leader election and the unauthenticated metrics are fixed. Three points are still open. The threat-model note covers only the tenant rows, not the platform components' own CiliumNetworkPolicies. The cozyplane chart suite has no case foregress.*reaching--cluster-cidr/--internal-cidrs.tenant-rbac.yamlstill aggregates only intoadmin/edit/view, and the body defers that to #4508.
What this PR does
Adds an opt-in
bundles.system.networkingVariant: "cozyplane"for theisp-full(Talos) bundle: it replaces kube-ovn, Cilium and kube-proxy with the cozyplane eBPF CNI, its aggregatedsdn.cozystack.ioAPI server, and thecozyplane-kprsocket-LB kube-proxy replacement. The defaultkubeovn-ciliumpath renders identically to main; the selector is refused outright on every other bundle variant (slim variants included), where it would half-apply.Components, one commit each: the vendored CNI chart (
packages/system/cozyplane), the kube-proxy replacement (cozyplane-kpr), the aggregated apiserver with its etcd-operator-managed, PVC-backed etcd (cozyplane-apiserver), the Cilium policy CRDs shipped inert so stock charts install (cozyplane-cilium-crds), acozystackAPI.serveSDNtoggle in cozystack-api, and the platform wiring. The wiring threads the platform'snetworking.podCIDR/serviceCIDR(andclusterDNS, derived from the service CIDR when unset) into the cozyplane component, drops the packages that assume the replaced stack (kubeovn-webhook/-plunger, multus, securitygroup-controller, the virtualprivatecloud app), refuses the defaultciliumgateway class on the variant, and turns off sdn aggregation in cozystack-api.Bootstrap ordering: the CNI installs first with the kube API as its only dependency and ships only the LOCAL group's CRDs (
local.sdn.cozystack.io, underlay IPAM); the tenant kinds (VPC, Port, SecurityGroup, …) have no CRDs and are served only by the aggregated apiserver, which installs later behind cert-manager, etcd-operator and storage. Until it lands,sdn.cozystack.iois simply not served; there is no takeover. On Talos the agent and kpr bootstrap against KubePrism, since with no kube-proxy thekubernetes.defaultClusterIP is unserved until kpr itself runs.The charts are vendored verbatim from
lllamnyp/cozyplaneat a ref pinned in each package's Makefile;make updatere-vendors and re-applies the one Cozystack-side difference (the Chart.yaml version stanza). Image pins are the digests upstream pins for that ref, with multi-arch legs verified there. Each vendored package carries a Cozystack-side helm-unittest suite pinning the platform contract, and the platform suite covers the variant's package set, the guards and the CIDR threading.Known gaps, deliberate in a draft: CiliumNetworkPolicy objects apply but are not enforced on this variant — tenant network isolation on the default network and the Cilium host firewall are not in effect, as stated in the values comment, the networking source and the threat model; rebuilding isolation on what cozyplane does enforce (native NetworkPolicy, its operator-only HostFirewall) is a follow-up. Relatedly, cozystack-api's SecurityGroup (a restricted interface over CiliumNetworkPolicy) is not served on the variant; cozyplane's own SecurityGroup is the first-class tenant instrument for traffic inside and across VPCs. Tenant RBAC for the sdn kinds and the tenant-facing VPC application land in #4508, stacked on this PR. An e2e lane and real-cluster validation of the full variant are follow-ups before this could ever be selectable in production.
Screenshots
Not a UI change.
Downstream repositories
The trigger map fires for cozystack/website (a new platform variant touches the variants page) and arguably terraform-provider-cozystack (the SecurityGroup spec differs per variant). Both follow-ups are deferred while this PR is draft — filing them against an unmerged opt-in variant would be noise; they should be opened when this leaves draft. Boxes left unticked so a human decides.
Release note