Skip to content

[virtual-machine] Exclude external VM services from Cilium BPF LB - #2357

Merged
Timofei Larkin (lllamnyp) merged 1 commit into
cozystack:mainfrom
mattia-eleuteri:fix/vm-service-proxy-name
Apr 8, 2026
Merged

Timofei Larkin (lllamnyp) merged 1 commit into
cozystack:mainfrom
mattia-eleuteri:fix/vm-service-proxy-name

Conversation

@mattia-eleuteri

@mattia-eleuteri mattia-eleuteri commented Apr 8, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does

Adds the service.kubernetes.io/service-proxy-name: "cozy-proxy" label to VM LoadBalancer services when external: 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/wholeIP annotation) and MetalLB L2 advertisement is unaffected.

This fixes two issues:

  1. 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. The CiliumClusterwideNetworkPolicy then 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 existing allow-external-communication policy allows it. Pod IP isolation between tenants is preserved.

  2. 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:

  • Cilium 1.18.6: inter-tenant via LB IP ✅, pod IP blocked ✅, external SSH ✅, WholeIP all ports ✅
  • Cilium 1.19.1: same results ✅, no wildcard drops ✅
  • MetalLB L2 advertisement: unaffected ✅
  • Other services without the label: unaffected ✅

Note: The externalPorts / PortList filtering is currently not enforced at the network level (the wholeIP annotation 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).

[virtual-machine] Fix inter-tenant connectivity via public LB IPs and WholeIP on Cilium 1.19+

Summary by CodeRabbit

  • New Features
    • Services now include additional Kubernetes metadata to designate proxy handling when external services are enabled, enabling better service management and routing configuration in external deployments.

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]>
@coderabbitai

coderabbitai Bot commented Apr 8, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d1f29663-6b50-42ec-bb0f-8ac2fe47f539

📥 Commits

Reviewing files that changed from the base of the PR and between 38624b7 and 026b1c7.

📒 Files selected for processing (1)
  • packages/apps/vm-instance/templates/service.yaml

📝 Walkthrough

Walkthrough

The Kubernetes Service template now conditionally adds a service.kubernetes.io/service-proxy-name: "cozy-proxy" label in the metadata.labels section when .Values.external is enabled, without altering any other service configuration, control flow, or behavior.

Changes

Cohort / File(s) Summary
Service Label Enhancement
packages/apps/vm-instance/templates/service.yaml
Added conditional Kubernetes label for external proxy service configuration when .Values.external is enabled.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐰 A label so small, yet purposeful and keen,
For the cozy-proxy, now integrated with sheen.
When external calls ring, the service will know,
To route through the proxy—a smooth, guided flow! 🌟

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: adding a Cilium BPF LB exclusion mechanism for external VM services through a Kubernetes label.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lllamnyp Timofei Larkin (lllamnyp) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What an amazing fix with just a single label! Thanks mattia-eleuteri !

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Apr 8, 2026
@lllamnyp Timofei Larkin (lllamnyp) added the kind/backport Categorizes issue or PR as requiring a backport to the current release line label Apr 8, 2026
@lllamnyp
Timofei Larkin (lllamnyp) merged commit d2361f3 into cozystack:main Apr 8, 2026
13 checks passed
@github-actions

github-actions Bot commented Apr 8, 2026

Copy link
Copy Markdown

Successfully created backport PR for release-1.2:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lexfrei

Copy link
Copy Markdown
Contributor

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!

Andrei Kvapil (kvaps) added a commit that referenced this pull request Apr 8, 2026
… from Cilium BPF LB (#2361)

# Description
Backport of #2357 to `release-1.2`.
Andrei Kvapil (kvaps) added a commit that referenced this pull request Apr 28, 2026
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]>
Andrei Kvapil (kvaps) added a commit that referenced this pull request Apr 28, 2026
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]>
Andrei Kvapil (kvaps) added a commit that referenced this pull request Apr 28, 2026
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/backport Categorizes issue or PR as requiring a backport to the current release line lgtm This PR has been approved by a maintainer size/XS This PR changes 0-9 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants