Repository navigation
[virtual-machine] Exclude external VM services from Cilium BPF LB - #2357
Timofei Larkin (lllamnyp) merged 1 commit into
Conversation
Add service.kubernetes.io/service-proxy-name label to LoadBalancer services when external: true. This prevents Cilium from adding the service to its BPF service map, fixing two issues: 1. Inter-tenant connectivity via public LB IPs: Cilium's kube-proxy replacement DNATs traffic to LB IPs before policy evaluation, causing the CiliumClusterwideNetworkPolicy to block legitimate cross-tenant traffic through public IPs. 2. WholeIP broken on Cilium 1.19+ (cozystack#2327): wildcard service drop entries block all ports not declared in the Service spec before traffic reaches cozy-proxy's nftables rules. With this label, Cilium completely ignores the Service. Routing is handled by kube-ovn (via the wholeIP annotation) and MetalLB continues to advertise the IP via L2 unaffected. Tested on Cilium 1.18.6 and 1.19.1: inter-tenant via LB IP works, pod IP isolation preserved, external access unaffected. Signed-off-by: mattia-eleuteri <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Kubernetes Service template now conditionally adds a Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the service.yaml template for the vm-instance package to include the service.kubernetes.io/service-proxy-name: "cozy-proxy" label when the service is configured as external. I have no feedback to provide as there were no review comments to evaluate.
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
What an amazing fix with just a single label! Thanks mattia-eleuteri !
|
Successfully created backport PR for |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. The label placement and indentation are correct, the service.kubernetes.io/service-proxy-name: "cozy-proxy" semantics properly excludes external VM services from Cilium BPF LB (since cozystack's Cilium is not configured with k8s.serviceProxyName), and cozy-proxy is a default platform component that will handle these services. Minimal, well-scoped fix.
|
Thanks for the contribution! Great find and a very clean fix — one label that solves two issues at once. We appreciate your work and look forward to more contributions from you. Welcome aboard! |
Pulls in the per-port filtering and allowICMP support that the companion vm-instance chart fix relies on. cozy-proxy v0.3.0 also tightens the selector to the standard service.kubernetes.io/service-proxy-name=cozy-proxy label and switches the default ingress mode to port-filter; both are already covered by the vm-instance chart (label landed in #2357, wholeIP/allowICMP wired explicitly in the parent PR), so VM workloads upgrade transparently. Out-of-tree consumers using cozy-proxy annotations directly (without the label, or relying on the absent-annotation passthrough default) are called out in the upstream v0.3.0 release notes: https://github.com/cozystack/cozy-proxy/releases/tag/v0.3.0 Signed-off-by: Andrei Kvapil <[email protected]>
Pulls in the per-port filtering and allowICMP support that the companion vm-instance chart fix in #2501 relies on. cozy-proxy v0.3.0 also tightens the selector to the standard service.kubernetes.io/service-proxy-name=cozy-proxy label and switches the default ingress mode to port-filter; both are already covered by the vm-instance chart (label landed in #2357, wholeIP/allowICMP wired explicitly in #2501), so VM workloads upgrade transparently. Out-of-tree consumers using cozy-proxy annotations directly (without the label, or relying on the absent-annotation passthrough default) are called out in the upstream v0.3.0 release notes: https://github.com/cozystack/cozy-proxy/releases/tag/v0.3.0 Signed-off-by: Andrei Kvapil <[email protected]>
Bumps the vendored cozy-proxy chart and image tag from v0.2.0 to v0.3.0 by running `make update` in `packages/system/cozy-proxy`. cozy-proxy v0.3.0 release: https://github.com/cozystack/cozy-proxy/releases/tag/v0.3.0 ## Why cozy-proxy v0.3.0 is the companion of #2501. The chart fix in #2501 finally honors the documented PortList semantics (only `externalPorts` reachable, ICMP preserved by default), but only takes effect once cozy-proxy v0.3.0 is in place — v0.2.0 has no port-aware logic, so the chart fix silently no-ops on it. This PR ships that runtime side. ## Upgrade impact For cozystack users this is a **bug fix, not a breaking change**. cozy-proxy v0.3.0 introduces two contract changes upstream — (1) label-only selector via `service.kubernetes.io/service-proxy-name: cozy-proxy`, and (2) port-filter as the default ingress mode — both of which are absorbed by the vm-instance chart (label landed in #2357, `wholeIP` + `allowICMP` wired in #2501). - **`externalMethod: WholeIP`** (default) — chart renders label + `wholeIP: "true"` in both old and new versions. cozy-proxy keeps passthrough. **No change.** - **`externalMethod: PortList`** — previously the chart always rendered `wholeIP: "true"` regardless of method, and cozy-proxy v0.2.0 had no port-aware logic, so PortList was silently a no-op (all ports reachable). After this bump + #2501, PortList behaves as documented: only ports listed in `externalPorts` reach the VM. Users who configured PortList but reached undeclared ports (relying on the silent no-op) will see those ports become unreachable — that's the documented intent of PortList finally taking effect. ICMP keeps working by default thanks to `externalAllowICMP: true`. - **DaemonSet rollover** — re-init of the nft table during cozy-proxy pod restart is a brief blip on existing flows; same as any DaemonSet upgrade. Out-of-tree consumers using cozy-proxy directly (without the `service-proxy-name` label, or relying on the absent-annotation passthrough default) need to migrate per the upstream v0.3.0 release notes — not a cozystack concern. ## Order of operations This PR is functionally complete on its own (image bump), but the user-facing PortList fix it enables only happens when #2501 also lands. Recommended merge order: 1. #2501 first (chart adapts to the new contract) 2. This PR second (cozy-proxy DaemonSet picks up the new image) The reverse order is also safe — cozy-proxy v0.3.0 with the unmodified vm-instance chart still produces correct results for `externalMethod: WholeIP` (most common case) and keeps PortList silently broken until #2501 lands. ## Test plan - [ ] `helm template packages/system/cozy-proxy` — image tag `v0.3.0` rendered - [ ] `make unit-tests` passes - [ ] After #2501 lands, end-to-end: `externalMethod: PortList` with `externalPorts: [22]` filters all other ports, ping works - [ ] WholeIP-method VM Service unchanged after upgrade ## Companion PRs - #2501 — vm-instance chart wires `wholeIP` / `allowICMP` per `externalMethod` - cozystack/cozy-proxy#13 (merged) — label-only selector + port-filter default - cozy-proxy v0.3.0 release notes — https://github.com/cozystack/cozy-proxy/releases/tag/v0.3.0 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Released version 0.3.0 with updated Helm chart metadata and container image configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Adds the
service.kubernetes.io/service-proxy-name: "cozy-proxy"label to VM LoadBalancer services whenexternal: true. This tells Cilium to completely ignore the service — no BPF entries, no DNAT, no wildcard drops.Routing continues to work through kube-ovn (
networking.cozystack.io/wholeIPannotation) and MetalLB L2 advertisement is unaffected.This fixes two issues:
Inter-tenant connectivity via public LB IPs: With
kube-proxy-replacement: true, Cilium DNATs traffic destined to LB IPs (on ports declared in the Service) to the backend pod IP before evaluating network policies. TheCiliumClusterwideNetworkPolicythen sees a cross-tenant pod-to-pod flow and blocks it. Ports not in the Service pass as "world" traffic and work fine — so does ICMP. With this label, Cilium doesn't DNAT anything, all traffic is classified as "world", and the existingallow-external-communicationpolicy allows it. Pod IP isolation between tenants is preserved.WholeIP broken on Cilium 1.19+ (bug(cozy-proxy): WholeIP 1:1 NAT broken with Cilium 1.19 due to wildcard service drop entries #2327): Cilium 1.19 introduced wildcard service drop entries (
cilium/cilium#40684) that drop all traffic to a LB IP on ports not declared in the Service, before it reaches netfilter/cozy-proxy. With this label, no BPF entries are created at all, so there's nothing to drop.Tested on our lab cluster:
Note: The
externalPorts/ PortList filtering is currently not enforced at the network level (thewholeIPannotation causes kube-ovn to route all ports regardless). This was the case before this change and is a separate concern — per-VM CiliumNetworkPolicies could be re-introduced for that (ref:4cbf562a, #1601).Summary by CodeRabbit