Repository navigation
feat(vpc): expose allowSubnets per subnet - #4478
Aleksei Sviridkin (lexfrei) merged 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 (8)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe VPC subnet API and Helm chart now support optional ChangesVPC subnet CIDRs
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to The optional subnet CIDRs are represented in the VPC configuration and rendered by the registered chart. No actionable merge-blocking issue remains after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The setting is scoped to the subnet that declares it, but the exposed configuration does not limit how broad an admitted source network can be. Authorization and enforcement of that limit need confirmation. 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 |
github-actions
Bot
added
area/networking
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri LGTM. The field is a plain passthrough, and I could not find a way for it to open anything outside the tenant's own subnet.
The ACLs only ever land on the declaring subnet. In the vendored kube-ovn (v1.15.26), SetLogicalSwitchPrivate rebuilds the ingress ACLs of that one logical switch: a default drop, allow inside the subnet CIDR, allow from the node switch CIDR, and one allow rule per allowSubnets entry. No other switch is touched. Traffic from another tenant still has to reach this VPC's router first, and that needs a static route this tenant writes or a peering both sides declare. So even 0.0.0.0/0 only makes this subnet behave as non-private. The controller also re-enqueues the Subnet when spec.allowSubnets changes, so edits and removals apply without recreating it.
The tests hold the template. Removing the with block fails two cases, and replacing it with hasKey fails the empty-list case. Codegen drift and pre-commit are green on this head, and the branch merges cleanly into current main. The red E2E Tests context is from 2026-09-25, before the fork e2e lane was fixed, so it says nothing about this change.
Two non-blocking notes:
- Nothing checks that the values are CIDRs. kube-ovn's
ValidateSubnetrejects the whole Subnet on anythingnet.ParseCIDRrefuses, for example a bare192.168.1.10. The HelmRelease still reports Ready, so the tenant gets a subnet that quietly stops reconciling.cidrhas the same gap today, so I'm not blocking on it. A CIDR pattern on both fields would be a cheap follow-up. - The field won't reach the docs. The vpc Makefile runs cozyvalues-gen without
-r README.md, so the README table is still the old map form and missesroutesandpeerstoo. The website VPC page is built from that README. This is pre-existing. A VPN gateway setup needsroutesandallowSubnetsexplained together, so it's worth fixing once for all three.
Every VPC subnet is rendered private, so kube-ovn drops any packet whose source lies outside the subnet's own CIDR. A tenant that runs a VPN or router VM on a subnet and points a static route at it gets the forward path from the existing routes value, but traffic arriving from the remote network is then dropped by the private ACL. Subnet is cluster-scoped, so the only workaround so far has been an operator patching spec.allowSubnets by hand: something the tenant cannot do, that no application value records, and that is lost whenever the subnet is recreated. Declaring it per subnet keeps the widening in the tenant's own VPC and scoped to the one subnet that needs it. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <[email protected]>
6af081c to
3c657a4
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri I rebased the branch onto current main so CI can run e2e. The image build failed on the old base because it still downloaded mc from dl.min.io, which main has already fixed. The rebase was clean, and the change is the one I approved in 5343554315.
08c344a
into
cozystack:main
2 participants
What this PR does
Adds an optional
allowSubnetslist to each entry ofsubnetsin the VirtualPrivateCloud chart, rendered intospec.allowSubnetsof the kube-ovnSubnet.Every VPC subnet is rendered
private: true, so kube-ovn drops any packet whose source is outside the subnet's own CIDR. A tenant who runs a VPN or router VM on a subnet already gets the forward path throughroutes(a static route with the VM asnextHopIP), but traffic arriving from the remote network is dropped by the private ACL. Today the only fix is an operator patching the cluster-scopedSubnetby hand: the tenant cannot do it, no application value records it, and it is lost whenever the subnet is recreated.The subnet stays private; the list only widens the ingress ACL of the subnet that declares it, inside the tenant's own VPC. An absent or empty list renders nothing, so existing releases produce the same manifests.
The chart had no helm-unittest suite, so this adds a
testtarget andtests/subnet_test.yaml, covering a subnet without the field, one with it, the field staying scoped to its own subnet, and an empty list. Two of the four cases fail against the template onmain.allowSubnetsis a kube-ovn field. #4034 teaches this chart to render Cozyplane objects when kube-ovn is absent, so whichever of the two lands second has to decide how the other backend treats the list.Screenshots
Downstream repositories
subnetsby hand invpc_schema.goandvpc_model.go, so the new nested field needs a schema and expand/flatten change there; an issue rather than a PR, since it should follow this one merging)Release note
Summary by CodeRabbit