diff --git a/README.md b/README.md index 4c497e1..6a5296e 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,49 @@ The nftables ruleset placed in table `ip cozy_proxy` consists of: `allowICMP: "true"` annotation is set, the pod IP is added to `icmp_allowed_pods` and ICMP toward it is accepted before the drop rule. +### Rule scoping + +The two rewrites are deliberately not scoped alike. Every controller instance +watches every Service, but programs: + +| Object | Scope | Why | +|---|---|---| +| `svc_pod` (`ingress_dnat`), `allowed_ports`, `icmp_allowed_pods` | the node hosting the backend pod | A non-hosting node would rewrite the destination before the packet has even left it. The hosting node then records a conntrack tuple of `(client -> podIP)`, while the reply leaves the pod and gets `saddr` rewritten to the service IP by `egress_snat` at priority `raw`, before conntrack. `(svcIP -> client)` matches nothing, so the reply is not `established` and `port_filter` drops it. | +| `pod_svc` (`egress_snat`) | every node | An intra-cluster client is source-NATed by the CNI to its own node address, which the overlay knows how to reach directly. The backend's reply is then tunnelled straight to that node and never traverses the hosting node's netfilter hooks, so the client's node is the only place left where the pod IP can still be turned back into the service IP. Without the entry there, the reply arrives with the wrong source and the client answers with a RST — a cross-node connection that hangs while the same-node one works. | + +`NODE_NAME` carries the node identity; the chart injects it from +`spec.nodeName`. When it is unset the ingress scope check is disabled and +everything is programmed everywhere, so the binary still runs under a +deployment that does not inject it. + +## L4 LoadBalancer mode (experimental) + +Besides VMs, cozy-proxy can take over the public IP of ordinary `LoadBalancer` +services — several ports, several backends, port translation — so that it is +reachable from every pod the way it is from the Internet. A service opts in with +both: + +```yaml +metadata: + labels: + networking.cozystack.io/lb-proxy: cozy-proxy + annotations: + service.cilium.io/type: ClusterIP # Cilium releases the VIP, keeps the ClusterIP +spec: + type: LoadBalancer + externalTrafficPolicy: Local +``` + +The node from which MetalLB (L2) announces the VIP DNATs new connections to its +local ready backends, masquerading only sources that are node IPs; everything +else addressed to the VIP is dropped. The mode has its own table, `ip +cozy_proxy_l4`, and never touches the VM mode's. It is off by default: run with +`--enable-l4-loadbalancer` (chart value `l4LoadBalancer.enabled`). Phase 1 +handles TCP, IPv4 and `externalTrafficPolicy: Local`. + +See [the RFC](docs/rfc/l4-loadbalancer-mode.md) for the model, the datapath, +the security implications and the rollout plan. + ## Installation Install controller using Helm-chart: diff --git a/charts/cozy-proxy/templates/daemonset.yaml b/charts/cozy-proxy/templates/daemonset.yaml index df774bf..dc75701 100644 --- a/charts/cozy-proxy/templates/daemonset.yaml +++ b/charts/cozy-proxy/templates/daemonset.yaml @@ -21,6 +21,27 @@ spec: - name: cozy-proxy image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" imagePullPolicy: {{ .Values.image.pullPolicy }} + {{- $keepL4Table := and (not .Values.l4LoadBalancer.enabled) (not .Values.l4LoadBalancer.removeTableWhenDisabled) }} + {{- if or .Values.l4LoadBalancer.enabled (not .Values.vmMode.enabled) $keepL4Table }} + args: + {{- if .Values.l4LoadBalancer.enabled }} + - --enable-l4-loadbalancer + {{- end }} + {{- if $keepL4Table }} + - --remove-l4-table-when-disabled=false + {{- end }} + {{- if not .Values.vmMode.enabled }} + - --enable-vm-mode=false + {{- end }} + {{- end }} + env: + # Scopes the ingress rules to the backends hosted on this node; see + # "Rule scoping" in the README for why a non-hosting node must not + # program them. + - name: NODE_NAME + valueFrom: + fieldRef: + fieldPath: spec.nodeName securityContext: privileged: true capabilities: diff --git a/charts/cozy-proxy/templates/role.yaml b/charts/cozy-proxy/templates/role.yaml index 5e80fe1..f01596d 100644 --- a/charts/cozy-proxy/templates/role.yaml +++ b/charts/cozy-proxy/templates/role.yaml @@ -9,4 +9,15 @@ rules: - apiGroups: [""] resources: ["services", "endpoints"] verbs: ["get", "list", "watch"] + {{- if .Values.l4LoadBalancer.enabled }} + - apiGroups: [""] + resources: ["nodes"] + verbs: ["list", "watch"] + - apiGroups: ["discovery.k8s.io"] + resources: ["endpointslices"] + verbs: ["list", "watch"] + - apiGroups: ["metallb.io"] + resources: ["servicel2statuses"] + verbs: ["list", "watch"] + {{- end }} {{- end }} diff --git a/charts/cozy-proxy/values.yaml b/charts/cozy-proxy/values.yaml index e143e92..90e85cf 100644 --- a/charts/cozy-proxy/values.yaml +++ b/charts/cozy-proxy/values.yaml @@ -10,3 +10,21 @@ daemonset: rbac: create: true + +# L4 LoadBalancer mode: takes over from Cilium the public IP of services +# labelled networking.cozystack.io/lb-proxy=cozy-proxy that also carry +# service.cilium.io/type=ClusterIP. See docs/rfc/l4-loadbalancer-mode.md. +l4LoadBalancer: + enabled: false + # With the mode disabled, remove the table an earlier run programmed on the + # next start: this is how the mode is rolled back. Set it to false on a + # release that runs next to a second release running the L4 mode, or each of + # its starts deletes that release's table. + removeTableWhenDisabled: true + +# VM mode, for services labelled service.kubernetes.io/service-proxy-name=cozy-proxy. +# Turn it off only to run the L4 mode as a second release next to a cozy-proxy +# that keeps the VM mode, for instance to try the L4 mode on a live cluster. +# That cozy-proxy then needs l4LoadBalancer.removeTableWhenDisabled=false. +vmMode: + enabled: true diff --git a/docs/rfc/l4-loadbalancer-mode.md b/docs/rfc/l4-loadbalancer-mode.md new file mode 100644 index 0000000..330b040 --- /dev/null +++ b/docs/rfc/l4-loadbalancer-mode.md @@ -0,0 +1,675 @@ +# RFC: L4 LoadBalancer mode + +| | | +|---|---| +| Status | Draft. Phase 1 prototype on branch `feat/l4-loadbalancer` (section 11) | +| Scope | cozy-proxy, plus the Cozystack charts that would opt services in | +| Supersedes | nothing; the existing VM mode is unchanged | + +## 1. Problem + +Cozystack isolates tenants on **egress**: every tenant namespace carries a +`CiliumClusterwideNetworkPolicy` that only lets its pods talk to their own +tenant, a few platform services, and `world`. Ingress policies are wide open to +`world` and `cluster`. + +That model has a blind spot for public IPs. When a pod connects to the public +IP of a `LoadBalancer` service handled by Cilium (kube-proxy replacement), Cilium +translates the destination **inside the client pod**, before the egress policy is +evaluated. The egress verdict is then taken against the backend's identity and +the post-translation port. A pod of tenant A connecting to the public IP of +tenant B's Postgres is therefore judged as "A talking to B's pod" and dropped, +although the same connection from the Internet succeeds. + +So a tenant cannot reach another tenant's public endpoint (or even a parent +tenant's ingress, for nested projects) by its public IP, which users do +expect: a public IP is public. VM services already behave correctly, because +they are handled by cozy-proxy, which Cilium ignores: the packet leaves the +client pod with the public IP as destination, is evaluated as `world`, leaves the +node and comes back through the normal ingress path. + +The other `LoadBalancer` services cannot use cozy-proxy today: + +- the ingress of tenant Kubernetes clusters, created on the infra side by the + kubevirt cloud-controller-manager (CCM); +- tenant `Ingress` applications (ingress-nginx); +- `Postgres` and `MariaDB` with `external: true`. + +cozy-proxy's VM mode is a stateless 1:1 NAT: it rewrites addresses without +touching ports, assumes exactly one backend, and only reads `v1.Endpoints`. +These services need port translation (CCM services target the tenant node +ports), several backends, EndpointSlices (CCM services are selector-less and +their slices are written by `kubevirt-eps-controller`), readiness, and a stateful +NAT so that several backends can share one IP. + +This RFC proposes a second, independent datapath in cozy-proxy — the **L4 mode** +— that takes such services over from Cilium on the ingress path, so that their +public IP is reachable from every tenant, with the client seen as `world`. + +## 2. Model + +### 2.1 Retained model (E3) + +``` + MetalLB L2 announcer of the VIP (node N) + client ──► VIP:port ──► guard ──► DNAT to a *local* ready backend (conntrack) + └─ if the source is a node IP: masquerade + ◄────────── un-NAT by conntrack on the reply ◄── backend +``` + +1. **DNAT, stateful, on the announcer only.** The node MetalLB elected to answer + ARP for the VIP DNATs new connections to one of its *local* ready backends, + with port translation. Replies come back through the same node's conntrack, + which reverses the translation. Services must use + `externalTrafficPolicy: Local`, which is also what makes MetalLB pick an + announcer that has a local backend. +2. **Masquerade only sources that are node IPs.** An in-cluster pod reaching the + VIP from another node is masqueraded by kube-ovn to *its node's* IP before + leaving that node. If the announcer forwarded that packet unchanged, the + backend would answer the node IP directly, OVN would tunnel the reply + straight to the client's node, and that node would see a reply from the pod IP + to a connection it opened towards the VIP: RST. Masquerading those sources on + the announcer pins the reply to the announcer. Node IPs are read from the + `Node` objects (`InternalIP`), so the rule matches exactly the cluster's + nodes, not a CIDR guess — on some installs the LoadBalancer pool sits in the + node subnet. +3. **Every other source is preserved**: an Internet client is seen with its own + IP, a VM client with its public (VM service) IP. +4. **A guard drops everything else addressed to the VIP.** Without it, a packet + for an undeclared port is not translated, the VIP is not a local address any + more (Cilium released it), and the announcer forwards it to its default + gateway, which sends it straight back: a routing loop until the TTL expires. + This was observed on the lab. + +Observed on the lab with this model (3 nodes, kube-ovn + Cilium chaining, +MetalLB L2): + +| Client | Result | Source seen by the backend | +|---|---|---| +| pod on the announcer | OK | `100.64.0.x` (kube-ovn join IP), Cilium identity `world` | +| pod on another node | OK | `100.64.0.x` of the announcer, identity `world` | +| VM on another node (cozy-proxy VM service) | OK | the VM's public IP | +| Internet | OK | the client's real IP | + +### 2.2 Rejected: DNAT on the announcer without masquerade (E1) + +Works for the Internet and for pods on the announcer. A pod on another node gets +a RST: its node masquerades it to the node IP, the backend answers that node IP, +and OVN carries the SYN-ACK in Geneve directly to the client's node without +going back through the announcer's conntrack. Capture on the client node: + +``` +ovn0 In 10.246.4.189.60638 > VIP.80 [S] +bond0.324 Out 10.200.24.12.45576 > VIP.80 [S] +genev_sys P 10.246.4.186.8080 > 10.200.24.12.45576 [S.] <- straight from the pod +ovn0 Out 10.200.24.12.45576 > 10.246.4.186.8080 [R] +``` + +### 2.3 Rejected: DNAT on every node plus masquerade (E2) + +DNAT on every node, masquerading everything that enters from the pod interface, +fixes pod clients and breaks VM clients. A VM's egress is rewritten by +cozy-proxy's VM mode at priority `raw`, before conntrack (`egress_snat`). With a +DNAT and a masquerade on the VM's own node, the reply is de-masqueraded by +conntrack only after the VM mode's stateless `ingress_dnat` (priority `mangle`) +has already run, so the destination is never turned back into the VM's pod IP, +and the VM mode's `port_filter` drops the packet as a new flow on an +undeclared port. + +### 2.4 How Cilium is told to let go + +The service carries the annotation + +```yaml +service.cilium.io/type: ClusterIP +``` + +Cilium then installs only the ClusterIP frontend: it no longer translates the +VIP (nor the NodePort), while in-cluster traffic to the ClusterIP keeps its +current, isolated behavior. MetalLB still allocates and announces the VIP. +Verified on the lab. + +`service.kubernetes.io/service-proxy-name` is **not** used for this mode: it +makes Cilium drop the ClusterIP and the NodePort as well. + +### 2.5 How cozy-proxy recognizes the service + +A label distinct from the VM mode's: + +```yaml +metadata: + labels: + networking.cozystack.io/lb-proxy: cozy-proxy + annotations: + service.cilium.io/type: ClusterIP +``` + +| Label | Annotation | cozy-proxy L4 mode | +|---|---|---| +| absent | any | ignores the service | +| present | absent or another value | ignores it and logs: Cilium still owns the VIP, two owners would race | +| present | `ClusterIP` | manages the service (if supported, see 2.6) | +| present, and `service-proxy-name: cozy-proxy` | any | ignores it: the VM mode owns the service | + +The label is the trust anchor. The annotation alone is not: the kubevirt CCM +copies the tenant Service's annotations onto the infra Service, so a tenant can +set `service.cilium.io/*` annotations on infra objects, but it cannot set labels +there. + +### 2.6 Supported services, by phase + +| | Phase 1 (MVP) | Later | +|---|---|---| +| Protocol | TCP | UDP | +| `externalTrafficPolicy` | `Local` | `Cluster` (see 6) | +| Address family | IPv4 | IPv6 | +| Ports / backends | many / many | | +| Endpoints | EndpointSlices, ready, local | terminating fallback | +| Announcer source | MetalLB L2 `ServiceL2Status` | BGP, other LB providers | + +A labelled and annotated service that is not supported (for example +`externalTrafficPolicy: Cluster`) is **not** programmed, but its VIP is still +added to the guard, so its traffic is dropped instead of looping. A port the +phase does not support (UDP) is likewise left out of the declared ports, so the +guard drops it. + +## 3. Datapath + +### 3.1 Table + +The L4 mode lives in its own table, `ip cozy_proxy_l4`. It never touches the VM +mode's table `ip cozy_proxy`, and deleting it (`nft delete table ip +cozy_proxy_l4`) removes the L4 mode entirely. + +``` +table ip cozy_proxy_l4 { + # Every VIP of a labelled+annotated service, on every node. + set vips { type ipv4_addr } + # (VIP, protocol, port) of every supported service port, on every node. + set vip_ports { type ipv4_addr . inet_proto . inet_service } + # InternalIP of every Node. + set node_ips { type ipv4_addr } + # (backend IP, protocol, backend port) of every local target, announcer only. + set backends { type ipv4_addr . inet_proto . inet_service } + # One map per announced frontend: round-robin slot -> backend, commented + # with the service's namespace/name. nft lists the key type as "type 0" + # (see addBackendMap). + map backends--tcp- { type integer : ipv4_addr . inet_service } + + chain guard { + type filter hook prerouting priority mangle + 10; policy accept; + ct state established,related accept + ip daddr @vips ip daddr . meta l4proto . th dport != @vip_ports drop + } + + chain translate { + type nat hook prerouting priority dstnat - 5; policy accept; + # announcer only, one rule per (VIP, port) + ip daddr tcp dport dnat ip to numgen inc mod map @backends--tcp- + # announcer with no ready local backend for that port + ip daddr tcp dport drop + } + + chain masq { + type nat hook postrouting priority srcnat - 5; policy accept; + ct status dnat ip saddr @node_ips ip daddr . meta l4proto . th dport @backends masquerade fully-random + } +} +``` + +Notes: + +- `guard` is installed on every node. It accepts `established,related` first, so + ICMP errors about a translated flow (PMTU) still reach the NAT and are + translated with it. New packets to a VIP on an undeclared (VIP, protocol, + port) are dropped, which includes ICMP echo: nobody would answer it anyway. +- `translate` only holds rules on the announcer. Elsewhere the packet is left alone + and routed out, to the announcer. +- `masq` only matches flows this node translated (`ct status dnat`) towards one + of its L4 targets, from a node IP, so a node process talking to a backend pod + directly (a kubelet probe) keeps its source. The masquerade picks the address + of the interface towards the backend, `ovn0` on kube-ovn, hence the + `100.64.0.x` source. nft itself would match `ct original ip daddr @vips`; + `github.com/google/nftables` can only emit that with the generic conntrack + key, which makes `nft list` abort on the whole ruleset, hence the match on the + translated destination. +- `nft list` shows the backend maps as `type 0 : ipv4_addr . inet_service`: nft + only names an integer key through a `typeof` annotation, which + `github.com/google/nftables` cannot write. The elements are listed correctly + and the kernel does not look at either. +- `numgen inc` is per node and per rule, which is enough: only one node + translates a given VIP. + +### 3.2 Priorities, and how they interleave with the VM mode + +| Priority | Hook | Table | Chain | Effect | +|---|---|---|---|---| +| -300 (raw) | prerouting | `cozy_proxy` | `egress_snat` | VM mode: stateless source rewrite pod IP -> VM VIP, before conntrack | +| -200 | prerouting | | conntrack | flow lookup / creation | +| -150 (mangle) | prerouting | `cozy_proxy` | `ingress_dnat` | VM mode: stateless destination rewrite VM VIP -> pod IP | +| -140 | prerouting | `cozy_proxy_l4` | `guard` | L4: drop undeclared traffic to L4 VIPs | +| -105 | prerouting | `cozy_proxy_l4` | `translate` | L4: stateful DNAT, first packet only | +| -100 | prerouting | | kube-ovn / iptables nat | unchanged | +| 0 (filter) | prerouting | `cozy_proxy` | `port_filter` | VM mode: per-port filter on VM pod IPs | +| 95 | postrouting | `cozy_proxy_l4` | `masq` | L4: masquerade node sources | + +The two modes act on disjoint destination addresses (VM VIPs vs L4 VIPs), so +they do not see each other's packets, with one exception: a VM client of an L4 +service. Its packet leaves its node with the VM's VIP as source (rewritten at +`raw`, before conntrack), is not masqueraded by the announcer (not a node IP), +and the reply comes back to the VM's node as an ordinary packet to the VM VIP. +That is the lab-validated path. + +The opposite case is **not** supported: an L4 backend that is itself the pod of +a VM-mode service. Its replies would be source-rewritten at `raw` before +conntrack, so the reverse translation could not match, and the VM mode's +`port_filter` would see the L4 DNAT as a new flow. The L4 mode excludes such +backends and logs it. + +### 3.3 Programming model + +The L4 table is small (tens of services) and is **rebuilt as a whole** on every +change, in one netlink transaction: + +``` +add table ip cozy_proxy_l4 # so that the delete below cannot fail +delete table ip cozy_proxy_l4 +add table ip cozy_proxy_l4 { ...full desired state... } +``` + +nftables applies a batch atomically: a packet sees either the old or the new +ruleset, never a mix, and conntrack entries are not affected by rule changes. +There is no incremental diff to get wrong, no stale element to garbage-collect, +and a retry is simply the next full sync. This deliberately differs from the VM +mode, whose per-element updates needed the series of fixes on ENOENT handling, +batch isolation and startup snapshots. + +## 4. Lifecycle + +### 4.1 Inputs + +Every instance watches, cluster-wide: + +| Object | Used for | +|---|---| +| `Service` | selection (2.5), VIP (`status.loadBalancer.ingress[].ip`), ports, eTP | +| `EndpointSlice` (`discovery.k8s.io/v1`) | backends: `conditions.ready` (nil counts as ready), not terminating, `nodeName` = this node, port resolved by port name and protocol | +| `Node` | `node_ips` | +| `ServiceL2Status` (`metallb.io/v1beta1`), label `metallb.io/node=` | whether this node announces the service's VIP | + +`NODE_NAME` is mandatory for the L4 mode (the chart already injects it). Without +it the mode refuses to start rather than program every node. + +### 4.2 Reconciliation + +Level-triggered. Every informer event only marks the state dirty. A single sync +loop recomputes the desired state from the informer caches, coalescing bursts +(250 ms), and applies it as one transaction (3.3) when it changed. A failed pass +is retried after 5 s; a periodic resync (1 min) re-applies the state even +without events, so a table deleted or altered by hand comes back. + +The first sync waits for every informer to be synced. Until then the table left +by the previous instance stays in place and keeps forwarding. + +### 4.3 Conntrack purge + +A conntrack entry outlives the rule that created it. The purge follows +kube-proxy. After every successful sync that withdrew something, and once after +the first sync, the mode deletes the conntrack entries that were translated by +this node (reply source differs from the original destination), towards a VIP +the mode knows (previous or current state), and: + +- **TCP**: whose frontend is gone **on this node** — service removed or no + longer managed, port removed, or this node no longer the announcer (MetalLB + moved the VIP). A TCP backend withdrawn from a frontend that remains keeps its + established connections until they end on their own, or until its pod goes; + new connections no longer reach it. +- **UDP** (and any other protocol): whose backend is no longer programmed nor + draining — gone from the EndpointSlices or not ready — or whose frontend is + gone. Without the purge an active UDP flow would keep being sent to a backend + that left, forever. + +Why TCP is left alone: kube-proxy only purges UDP on endpoint removal, and the +lab showed the cost of doing otherwise. A backend taken out of a Service while +a request was in flight lost its flow at the purge, and the client, idle while +waiting for the answer, hung until its own timeout: the backend's reply matched +no entry any more, and no RST reached the client. Left alone, the request +completes. + +A local UDP endpoint that is terminating is *draining*: it gets no new flow, but +its live ones are kept until it leaves the slices. An endpoint that is merely +not ready is not: its probe says it cannot serve. + +Untranslated flows towards a VIP (a pod on a non-announcer node passing through) +are never touched, nor is anything the VM mode tracks. The purge runs after the +commit, so no new flow can be created towards a backend that was just purged. + +### 4.4 Restart and upgrade of the DaemonSet + +- Stopping an instance leaves the table in the kernel. A DaemonSet rolling + update therefore does not interrupt traffic: the old rules keep forwarding until + the new instance's first sync replaces them atomically. Changes that happen + during the gap are applied with that delay, as with kube-proxy. +- The mode is behind `--enable-l4-loadbalancer` (chart: + `l4LoadBalancer.enabled`), off by default. Disabling it deletes the table at + startup, which is the rollback path for the whole mode. +- An instance with the mode disabled must not delete the table of another + instance on the same node that runs it, as when the L4 mode is tried as a + second release next to the platform's cozy-proxy: the table would only come + back with the owner's next forced resync, up to a minute later, with neither + guard nor translation for the VIPs in between. That instance runs with + `--remove-l4-table-when-disabled=false` (chart: + `l4LoadBalancer.removeTableWhenDisabled`) and leaves the table untouched, as + an instance with the VM mode off leaves `cozy_proxy`. +- Nothing on the L4 side stops the process: the VM mode runs in the same + manager. Without `NODE_NAME`, or while the MetalLB CRD is missing, the mode + logs and stays idle. + +### 4.5 Announcer changes + +When MetalLB moves the VIP (node drain, backend moved), the new announcer +programs the DNAT on its next sync after its `ServiceL2Status` appears, and the +old one withdraws and purges. Connections in flight are lost, as with any L2 +failover; new connections succeed once both the gratuitous ARP and the sync have +happened. + +## 5. Source address, and what it means for security + +| Client | Source seen by the backend | Cilium identity at the backend | +|---|---|---| +| Internet | its real IP | `world` | +| VM with a public IP (cozy-proxy VM mode) | the VM's public IP | `world` | +| pod, or node process, on any node | `100.64.0.x` of the announcer | `world` | + +Consequences: + +- **Reachability**: a pod of any tenant can reach any L4 service by its public + IP, as from the Internet. That is the goal. ClusterIP traffic is unchanged and + stays isolated. +- **CIDR allowlists now apply to in-cluster clients.** A tenant policy such as + `ingressDeny fromCIDRSet 0.0.0.0/0 except ` is evaluated against + `world` sources. With the VM mode today, an in-cluster client is seen as its + node IP (`remote-node`), which such a rule does not match — it is bypassable. + With the L4 mode the source is `world`, so the deny applies. +- **In-cluster clients are indistinguishable from one another.** Every pod and + node process reaching a VIP through a given announcer shares its `100.64.0.x` + address. An allowlist can admit "the cluster" (the join subnet) or not; it + cannot admit one tenant's pods and not another's. Tenants that need that + distinction must use ClusterIP and network policies, or mTLS. +- **ingress-nginx `whitelist-source-range`, Postgres `pg_hba`** and similar + application-level allowlists see the same addresses as the network policies. +- **A tenant cannot switch its own LB to the L4 mode.** The kubevirt CCM + copies every annotation of the tenant Service onto the infra Service, so a + tenant controls `service.cilium.io/type` there — but not the labels: the CCM + sets only its own (`cluster.x-k8s.io/tenant-service-*`, `cluster-name`) and the + platform's `infraLabels`. cozy-proxy requires the + `networking.cozystack.io/lb-proxy` label (2.5), so the annotation alone never + makes it program anything. At worst a tenant copying `service.cilium.io/type: + ClusterIP` makes its own LB dark (Cilium lets go, nobody takes over); an + admission guard on CCM Services (7.1), where the platform runs one, refuses + such a Service in the first place. +- **What the tenant still controls on an L4 CCM Service**: the ports (the CCM + copies them) and `externalTrafficPolicy`. A tenant Service in eTP `Cluster` + that the platform labelled anyway would be refused by cozy-proxy and stay + dark, which is why the label and the annotation must be set per Service, for + eTP `Local` only (7). + +## 6. `externalTrafficPolicy` + +| eTP | Behavior | +|---|---| +| `Local` | Phase 1. DNAT to local ready backends on the announcer; Internet and VM sources preserved. | +| `Cluster` | Not programmed (VIP guarded, traffic dropped) until a later phase. Supporting it requires DNAT to backends on other nodes. Their replies would leave through their own node's gateway and miss the announcer's conntrack, so **every** source would have to be masqueraded, losing the client IP — which is precisely what eTP `Cluster` means, but it changes what tenants see today. | + +In production today, all Postgres and Ingress services and most CCM services use +`Local`. MariaDB (`Cluster` by default in the operator) and a minority of tenant +Kubernetes services (the CCM copies the tenant's policy) use `Cluster`. + +## 7. Opting services in, per type + +All of this is on the Cozystack side; cozy-proxy only reacts to the label and +the annotation. Each change sits behind one platform switch so that it can be +enabled per environment. + +| Type | Where | Change | +|---|---|---| +| Postgres | `packages/apps/postgres/templates/external-svc.yaml` | add the label and the annotation; eTP is already `Local` | +| MariaDB | `packages/apps/mariadb/templates/mariadb.yaml` (`spec.service` / `spec.primaryService`) | add the label and the annotation through the operator's service template, and set `externalTrafficPolicy: Local` (operator default is `Cluster`) | +| Tenant Kubernetes (CCM) | CCM patch, next to the existing ones in `packages/apps/kubernetes/images/kubevirt-cloud-provider/patches`, switched on from `packages/apps/kubernetes/templates/cloud-config.yaml` | The CCM itself sets the label and `service.cilium.io/type: ClusterIP`, overriding any value copied from the tenant, and only when the tenant Service is eTP `Local`. `infraLabels` alone is not enough: it labels every Service of the cluster, eTP `Cluster` ones included, and the annotation would still have to come from the tenant. The CCM only sets labels and annotations at creation, so existing infra Services need a one-shot patch. See 7.1 for the Kyverno guard. | +| Ingress | `packages/extra/ingress/templates/nginx-ingress.yaml` | `controller.service.labels` / `annotations`; eTP is already `Local`. The host ingress (`tenant-root`) with PROXY protocol and ouroboros is last, see Open questions. | +| cozy-proxy | `packages/system/cozy-proxy` | bump the vendored chart, enable the mode, extend RBAC (EndpointSlices, Nodes, ServiceL2Status) | + +### 7.1 Coexistence with an admission guard on CCM Services + +Because the CCM copies every annotation of the tenant Service onto the infra +Service, a platform may run an admission guard against it. The one considered +here is a Kyverno `ClusterPolicy`: on CREATE and UPDATE, it refuses any Service +carrying `cluster.x-k8s.io/tenant-service-name` that has `spec.externalIPs` or +one of a list of MetalLB, external-dns and Cilium annotations, +`service.cilium.io/type` included, so that a tenant cannot drive them through +the CCM's copy. Such a guard, written that way, refuses an L4 CCM Service at +creation, and refuses the one-shot migration patch, which turns a compliant +Service into a violating one. + +It does not refuse the UPDATEs that follow. Kyverno admits any UPDATE of an +object that already violates the rule (`validate.allowExistingViolations`, +`true` by default; verified on the lab with 1.18.2): the CCM's own port updates +pass, but so does an UPDATE that adds another refused key. On the lab, an L4 +CCM Service, violating through its `service.cilium.io/type`, accepted +`service.cilium.io/node` on UPDATE, and another CCM Service already carried a +copied `metallb.io/loadBalancerIPs`. The narrowed rule below closes that hole for +L4 Services: with the label and `ClusterIP`, they are compliant, so every later +UPDATE is validated in full. + +The two are reconciled by narrowing that one key rather than dropping it: + +1. `service.cilium.io/type` leaves the guard's generic deny list, and gets a + rule of its own: it is allowed only with the value `ClusterIP` **and** on a + Service that also carries `networking.cozystack.io/lb-proxy: cozy-proxy`. + Every other `service.cilium.io/*` key stays refused. +2. The label is set by the platform only (the CCM patch or the migration + patch); the CCM copies no tenant label, so a tenant cannot satisfy the + exception by itself. +3. The annotation is set by the CCM patch, overriding the tenant's value, rather + than copied from the tenant. A tenant copying `ClusterIP` onto a Service the + platform did not label is still refused. + +A sketch of the dedicated rule (to be validated against the Kyverno version in +use, 1.18.2 when written): + +```yaml +- name: cilium-type-only-for-l4 + match: + any: + - resources: + kinds: ["Service"] + operations: ["CREATE", "UPDATE"] + selector: + matchExpressions: + - {key: cluster.x-k8s.io/tenant-service-name, operator: Exists} + preconditions: + all: + - key: "{{ request.object.metadata.annotations.\"service.cilium.io/type\" || '' }}" + operator: NotEquals + value: "" + validate: + message: >- + service.cilium.io/type is only allowed as ClusterIP on a LoadBalancer the + platform handed to cozy-proxy (label networking.cozystack.io/lb-proxy). + deny: + conditions: + any: + - key: "{{ request.object.metadata.annotations.\"service.cilium.io/type\" }}" + operator: NotEquals + value: ClusterIP + - key: "{{ request.object.metadata.labels.\"networking.cozystack.io/lb-proxy\" || '' }}" + operator: NotEquals + value: cozy-proxy +``` + +Order of the rollout: the narrowed policy goes in before the first CCM Service +is migrated, otherwise the migration patch is refused once the policy is +enforced, and reported as a violation while it is in `Audit`. + +## 8. Migration and rollback, per service + +Forward, for one service: + +1. Add the label. Nothing happens: cozy-proxy ignores a labelled service without + the annotation. +2. Add the annotation. Cilium withdraws the VIP and cozy-proxy programs it on its + next sync (sub-second). Connections established through Cilium are not + carried over and may be reset. + +Rollback, for one service: **remove the annotation**. Cilium takes the VIP back +and cozy-proxy stops programming it at the same time (it requires both), then +purges its conntrack entries. Removing the label afterwards is cosmetic. + +Removing only the label, on the other hand, leaves the VIP dark (Cilium still +ignores it): the runbook must always remove the annotation. + +Whole mode: remove every annotation, then disable the mode (which deletes the +table). + +For a CCM Service, the label and the annotation are both put on the infra +Service by the platform (7, 7.1), never through the tenant Service; the rollback +removes the annotation there, and the CCM patch must not put it back on its next +update. + +## 9. Rollout plan + +Each step runs for at least a week before the next, with the rollback above +rehearsed on the lab first. + +0. **Lab**: a dedicated test service and a cozy-proxy instance with the mode + enabled only on the lab. Matrix: Internet, pod on the announcer, pod + elsewhere, VM client, multiple backends, backend deletion (purge), announcer + failover, DaemonSet restart, ping and undeclared ports (no loop), CIDR deny + policy. +1. **Postgres and MariaDB** (3 services in production): single port, TCP, + single backend, low connection churn, clients mostly known. MariaDB first + needs eTP `Local`. +2. **Tenant Kubernetes, CCM services with eTP `Local`**: port translation and + several backends (worker VMs). Requires the CCM patch. +3. **Ingress**, tenant ingresses first, the host ingress last. This is where the + risk is: 20 Ingress services carry most of the Internet-facing traffic. + +At each step, compare before and after: reachability from the Internet and from +another tenant, the source IP logged by the backend, MetalLB announcer, and the +`cozy_proxy_l4` table on the announcer. + +## 10. Open questions + +1. **Announcer source.** Phase 1 reads MetalLB's `ServiceL2Status`. Upstream may + prefer an abstraction (an interface with a MetalLB L2 implementation), and + MetalLB BGP mode, where every node advertises, would need a different model. +2. **Pre-programming every node that has a local backend** would make failover + hitless and remove the MetalLB dependency. It differs from the rejected E2 + (it does not masquerade the pod interface), but VM clients on a node holding + a local backend would then be translated on their own node. That path has not + been validated on the lab. +3. **Stale ARP after a failover**: a packet for a declared port reaching a node + that no longer announces the VIP is routed back to the gateway until the + caches expire (bounded by the TTL and MetalLB's gratuitous ARP). Should + non-announcers drop VIP traffic that does not come from a local pod + interface? That needs an interface name (`ovn0`), which is CNI specific. +4. **Host-network clients on the announcer** are not translated (no `output` + hook). They reach the VIP only if the gateway hairpins the packet back. Add a + `nat output` rule? +5. **eTP `Cluster`**: support it with full masquerade, keep refusing it, or have + the charts force `Local`? +6. **Terminating endpoints**: fall back to serving-and-terminating backends when + no ready one is left, as kube-proxy does for eTP `Local`? +7. **Node addresses**: only `InternalIP` today. Are there nodes whose pod egress + is masqueraded to another address (multiple uplinks, `ExternalIP`)? +8. **Host ingress with PROXY protocol and ouroboros**: ouroboros handles hairpin + traffic for the PROXY-protocol host ingress. Its interaction with a VIP that + Cilium no longer owns has to be checked before step 3. +9. **Observability**: metrics (sync duration and errors, programmed services, + purged flows) and Events on the Service when it is refused. Phase 1 only + logs, once per change. +10. **Admission guard on CCM Services (7.1)**: the narrowed rule was tested on + the lab on 2026-10-02 (Kyverno 1.18.2): a real CCM CREATE and port UPDATE, + the migration patch, a tenant copy without the label, another + `service.cilium.io/*` key, and the rollback in both orders. Cozystack does + not ship such a guard today: should it, and in which package? +11. **Where the label lives**: `networking.cozystack.io/lb-proxy: cozy-proxy` is a + proposal; the maintainers may prefer another key. +12. **Purging the TCP flows of a backend that is still alive — decided.** Aligned + on kube-proxy (4.3): on endpoint removal only UDP flows are purged; TCP flows + go with their frontend (service, port, announcement, including a MetalLB + failover). Reason: kube-proxy behaves so, and on the lab an idle client of a + withdrawn but live backend hung without a RST until its own timeout. +13. **Source-port reuse after masquerading node sources (fully-random).** + Measured on the lab: 300 connections per second without keep-alive, for + two minutes, from a pod to a CCM ingress in L4 mode. No error, but 0.06 to + 0.1 % of the connections took over a second, none under Cilium. On the + announcer, the backend VM's SYN-ACK left its veth and never reached `ovn0`: + it was dropped in OVS/OVN, and the client retransmitted. The tuples hit were + `100.64.0.x:`, reused 29 to 50 s after a previous connection on the + same port. + Cause, as far as the code goes: without a flag, the kernel's masquerade + keeps the client's source port whenever *its own* conntrack has the tuple + free (`nf_nat_l4proto_unique_tuple`), and it cannot see OVS's conntrack, + which may still hold the previous connection. Every client of a node + collapses onto one masqueraded address, so the source node's port choices + turn into tuple reuse on the announcer. The suspected drop is the + `ct.inv` match of the kube-ovn ACLs; that part is not confirmed. + Fix in the prototype: `masquerade fully-random` + (`NF_NAT_RANGE_PROTO_RANDOM_FULLY`, a fresh random port per flow; + `random` alone only offsets from a hash). This breaks the correlation with + the source node's choices, but a random port can still land on a tuple OVS + tracks: the residual rate grows with the connection rate and with how long + OVS keeps closed entries. To be measured again under the same load; if it + persists, the next levers are on the OVN side (its conntrack timeouts or + ACLs) or more masquerade addresses per announcer. +14. **`google/nftables` limits**: no `typeof` on sets, and no direction on the + typed conntrack keys (3.1). Both are cosmetic for `nft list` today; fixing + them upstream would let the masquerade match the original destination. + +## 11. Prototype status + +Branch `feat/l4-loadbalancer`, phase 1 only: + +| Area | Where | Tests | +|---|---|---| +| Selection (2.5) | `pkg/l4/select.go` | unit | +| Desired state per node (4.1) | `pkg/l4/state.go` | unit | +| Purge decision (4.3) | `pkg/l4/conntrack.go` | unit | +| nftables table (3) | `pkg/proxy/l4_nft.go` | kernel, in a network namespace: `nft list` against golden files, and real TCP traffic through three namespaces (translation, round-robin, source kept or masqueraded, guard, port without backend, purge, rebuild with an open connection) | +| Conntrack purge | `pkg/proxy/conntrack.go` | kernel, in a network namespace | +| Controller (4.2) | `pkg/controllers/l4_controller.go` | unit, and fake API clients | +| Switch and RBAC | `main.go`, chart `l4LoadBalancer.enabled` | `helm template` | + +The kernel tests need root and skip otherwise; `nft list` comparisons also need +the `nft` binary. They run in a privileged Linux container, for instance: + +``` +docker run --rm --privileged -v "$PWD":/src -w /src golang:1.26 sh -c \ + 'apt-get update -qq && apt-get install -y -qq nftables && go test ./...' +``` + +Not in phase 1: UDP, eTP `Cluster`, IPv6, terminating endpoints, metrics and +Events. + +Lab run on 2026-10-02 (image built from `181b41c`, as a second release with the +VM mode off, next to the platform's cozy-proxy): + +| Case | Result | Source seen by the backend | +|---|---|---| +| Internet, both ports (port translation 80 -> 8080, 81 -> 9090) | OK | the client's public IP | +| pods on the 3 nodes, another tenant | OK | `100.64.0.x` of the announcer | +| VM-mode client on another node, and on the announcer | OK | the VM's public IP | +| round-robin between the announcer's two local backends | OK | | +| undeclared port, ping (tcpdump on the announcer and on another node) | dropped, no packet sent back out | | +| CIDR deny `0.0.0.0/0 except ` on the backend | pods and VM refused, the allowed client passes | | +| backend withdrawn | its flows purged, new connections go to the others (built from `181b41c`; since then TCP flows of a withdrawn backend are kept, see 4.3) | | +| announcer moved by MetalLB | one failed probe out of ~110 (0.5 s interval) | | +| rollout restart of the DaemonSet | open connection kept, 0 failed probes out of 113 | | +| Postgres app, psql from pods, VM and VPN; `pg_stat_activity` | OK | `100.64.0.x`, VM IP, VPN address | +| CNPG switchover | ~7 s unavailable, mostly the server shutting down | | +| rollback (annotation removed) | Cilium serves the VIP again, other tenants refused again | | + +Two bugs were found and fixed: a chain named after an nft keyword (`dnat`), +which nft could not name on the command line, and a misleading logger name. + diff --git a/go.mod b/go.mod index 2be1fb2..f0ad06c 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,8 @@ go 1.26.4 require ( github.com/google/nftables v0.3.0 + github.com/vishvananda/netlink v1.3.1 + github.com/vishvananda/netns v0.0.5 golang.org/x/sys v0.46.0 k8s.io/api v0.36.2 k8s.io/apimachinery v0.36.2 diff --git a/go.sum b/go.sum index 9564083..7057511 100644 --- a/go.sum +++ b/go.sum @@ -120,8 +120,10 @@ github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/ github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= -github.com/vishvananda/netns v0.0.4 h1:Oeaw1EM2JMxD51g9uhtC0D7erkIjgmj8+JZc26m1YX8= -github.com/vishvananda/netns v0.0.4/go.mod h1:SpkAiCQRtJ6TvvxPnOSyH3BMl6unz3xZlaprSwhNNJM= +github.com/vishvananda/netlink v1.3.1 h1:3AEMt62VKqz90r0tmNhog0r/PpWKmrEShJU0wJW6bV0= +github.com/vishvananda/netlink v1.3.1/go.mod h1:ARtKouGSTGchR8aMwmkzC0qiNPrrWO5JS/XMVl45+b4= +github.com/vishvananda/netns v0.0.5 h1:DfiHV+j8bA32MFM7bfEunvT8IAqQ/NzSJHtcmW5zdEY= +github.com/vishvananda/netns v0.0.5/go.mod h1:SpkAiCQRtJ6TvvxPnOSyH3BMl6unz3xZlaprSwhNNJM= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= @@ -142,6 +144,8 @@ golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sys v0.2.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.10.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc= diff --git a/main.go b/main.go index 5d6f9dd..ce9bbf9 100644 --- a/main.go +++ b/main.go @@ -7,6 +7,7 @@ import ( _ "k8s.io/client-go/plugin/pkg/client/auth" "github.com/cozystack/cozy-proxy/pkg/controllers" + "github.com/cozystack/cozy-proxy/pkg/l4" "github.com/cozystack/cozy-proxy/pkg/proxy" "k8s.io/apimachinery/pkg/runtime" @@ -19,7 +20,9 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" ) var ( @@ -34,8 +37,21 @@ func init() { func main() { var probeAddr string var metricsAddr string + var enableL4 bool + var enableVM bool + var removeL4Table bool flag.StringVar(&probeAddr, "health-probe-bind-address", "0", "The address the probe endpoint binds to. Set to \"0\" to disable.") flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metric endpoint binds to. Set to \"0\" to disable.") + flag.BoolVar(&enableL4, "enable-l4-loadbalancer", false, + "Run the L4 LoadBalancer mode for services labelled "+l4.ProxyLabel+"="+l4.ProxyLabelValue+ + " (see docs/rfc/l4-loadbalancer-mode.md).") + flag.BoolVar(&removeL4Table, "remove-l4-table-when-disabled", true, + "With the L4 mode disabled, remove the table left by an earlier run, which is how the mode is rolled back. "+ + "Turn it off on an instance that runs next to another cozy-proxy running the L4 mode: "+ + "each of its starts would delete that instance's table.") + flag.BoolVar(&enableVM, "enable-vm-mode", true, + "Run the VM mode for services labelled service.kubernetes.io/service-proxy-name=cozy-proxy. "+ + "Turn it off only for an instance running the L4 mode next to another cozy-proxy that keeps the VM mode.") opts := zap.Options{ Development: true, } @@ -54,6 +70,10 @@ func main() { os.Exit(1) } + // The L4 mode builds its own clients from an untouched copy: the VM mode + // pins the shared config to the core group below. + l4Cfg := rest.CopyConfig(mgr.GetConfig()) + cfg := mgr.GetConfig() cfg.GroupVersion = &corev1.SchemeGroupVersion cfg.APIPath = "/api" @@ -68,16 +88,35 @@ func main() { os.Exit(1) } - controller := &controllers.ServicesController{ - Clientset: clientset, - Proxy: &proxy.NFTProxyProcessor{}, + // Datapath rules are programmed only for backend pods hosted on this node. + // When NODE_NAME is absent the check is disabled and every service is + // programmed, which is the pre-node-local behavior: degraded, but it keeps + // a new binary working under a chart that does not inject the variable yet. + nodeName := os.Getenv("NODE_NAME") + if nodeName == "" { + log.Info("NODE_NAME is not set, falling back to programming rules for every node's backends; " + + "set it from spec.nodeName to scope rules to this node") } - if err := mgr.Add(controller); err != nil { - log.Error(err, "unable to add endpoints controller to manager") - os.Exit(1) + if enableVM { + controller := &controllers.ServicesController{ + Clientset: clientset, + Proxy: &proxy.NFTProxyProcessor{}, + NodeName: nodeName, + } + + if err := mgr.Add(controller); err != nil { + log.Error(err, "unable to add endpoints controller to manager") + os.Exit(1) + } + } else { + // Another instance owns the cozy_proxy table: leave it alone, the VM + // mode's startup would rebuild its chains and purge its maps. + log.Info("VM mode disabled, the cozy_proxy table is left untouched") } + setupL4(mgr, l4Cfg, enableL4, removeL4Table, nodeName) + if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { log.Error(err, "unable to set up health check") os.Exit(1) @@ -93,3 +132,47 @@ func main() { os.Exit(1) } } + +// setupL4 adds the L4 LoadBalancer mode to the manager. When it is disabled, +// it removes what an earlier run left if removeTable is set, and otherwise +// leaves the table to the instance that owns it. Nothing here may stop the +// process: the VM mode runs in the same manager. +func setupL4(mgr ctrl.Manager, cfg *rest.Config, enabled, removeTable bool, nodeName string) { + datapath := &proxy.NFTL4Datapath{} + if !enabled { + if !removeTable { + log.Info("L4 mode disabled, the " + proxy.L4TableName + " table is left untouched") + return + } + if err := datapath.Teardown(); err != nil { + log.Error(err, "could not remove the L4 table left by an earlier run") + } + return + } + if nodeName == "" { + log.Error(nil, "the L4 LoadBalancer mode needs NODE_NAME to tell which backends are local; not starting it") + return + } + + clientset, err := kubernetes.NewForConfig(cfg) + if err != nil { + log.Error(err, "failed to create the L4 clientset; not starting the L4 mode") + return + } + dyn, err := dynamic.NewForConfig(cfg) + if err != nil { + log.Error(err, "failed to create the L4 dynamic client; not starting the L4 mode") + return + } + if err := mgr.Add(&controllers.L4Controller{ + Clientset: clientset, + Dynamic: dyn, + NodeName: nodeName, + Datapath: datapath, + Conntrack: &proxy.NetlinkConntrack{}, + }); err != nil { + log.Error(err, "unable to add the L4 controller to the manager") + return + } + log.Info("L4 LoadBalancer mode enabled") +} diff --git a/pkg/controllers/l4_controller.go b/pkg/controllers/l4_controller.go new file mode 100644 index 0000000..e56b15f --- /dev/null +++ b/pkg/controllers/l4_controller.go @@ -0,0 +1,318 @@ +package controllers + +import ( + "context" + "fmt" + "net/netip" + "reflect" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/dynamic/dynamicinformer" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/cache" + ctrl "sigs.k8s.io/controller-runtime" + + "github.com/cozystack/cozy-proxy/pkg/l4" + nat "github.com/cozystack/cozy-proxy/pkg/proxy" +) + +// l4log is not derived from the VM controller's logger: named +// "services-controller.l4", its lines read as if the VM mode were doing the +// work, which is confusing when both modes run in one process. +var l4log = ctrl.Log.WithName("l4-controller") + +// serviceL2StatusGVR is MetalLB's record of which node announces a service in +// L2 mode. Each speaker writes one per service it announces, labelled with its +// node. +var serviceL2StatusGVR = schema.GroupVersionResource{ + Group: "metallb.io", + Version: "v1beta1", + Resource: "servicel2statuses", +} + +const metallbNodeLabel = "metallb.io/node" + +const ( + defaultL4MinSyncInterval = 250 * time.Millisecond + defaultL4ResyncInterval = time.Minute + defaultL4RetryInterval = 5 * time.Second +) + +// L4Controller runs the L4 LoadBalancer mode: it keeps this node's L4 table +// in line with the cluster. It shares nothing with ServicesController, which +// runs the VM mode, but the process. +// +// Reconciliation is level-triggered. Events only mark the state dirty; a +// single loop recomputes the whole desired state from the informer caches and +// hands it to the datapath, which replaces its table atomically. A failure is +// simply retried by a later pass. +type L4Controller struct { + Clientset kubernetes.Interface + Dynamic dynamic.Interface + // NodeName is the node this instance programs. Required: without it the + // mode cannot tell which backends are local, and programs nothing. + NodeName string + Datapath nat.L4Datapath + Conntrack nat.ConntrackPurger + + // MinSyncInterval coalesces bursts of events into one pass. + MinSyncInterval time.Duration + // ResyncInterval re-applies the state even without events, so that a + // table removed or altered by hand comes back. + ResyncInterval time.Duration + // RetryInterval is how soon a failed pass is retried. + RetryInterval time.Duration + + dirty chan struct{} + input func() l4.Input + + // applied is the last state the datapath committed; nil until the first + // commit of this process. + applied *l4.State + // purgePending is set when a commit withdrew translations whose + // conntrack entries have not been purged yet, and purgeVIPs collects the + // VIPs those entries may target, removed services included. + purgePending bool + purgeVIPs map[netip.Addr]struct{} + // notices holds what was last logged per service, so a notice is logged + // when it changes rather than on every pass. + notices map[string]string +} + +// Start runs the controller until ctx ends. +// +// It never returns an error: the VM mode runs in the same manager, and a +// failure of the L4 mode must not take it down. Problems are logged and +// retried instead. +func (c *L4Controller) Start(ctx context.Context) error { + if c.NodeName == "" { + l4log.Error(nil, "NODE_NAME is not set; the L4 mode cannot tell which backends are local and stays idle") + <-ctx.Done() + return nil + } + l4log.Info("starting the L4 LoadBalancer mode", "node", c.NodeName) + + factory := informers.NewSharedInformerFactory(c.Clientset, 0) + svcInformer := factory.Core().V1().Services() + sliceInformer := factory.Discovery().V1().EndpointSlices() + nodeInformer := factory.Core().V1().Nodes() + + // Only this node's statuses: that is all the mode needs to know. + dynFactory := dynamicinformer.NewFilteredDynamicSharedInformerFactory(c.Dynamic, 0, metav1.NamespaceAll, + func(o *metav1.ListOptions) { o.LabelSelector = metallbNodeLabel + "=" + c.NodeName }) + l2Informer := dynFactory.ForResource(serviceL2StatusGVR) + + c.dirty = make(chan struct{}, 1) + handler := cache.ResourceEventHandlerFuncs{ + AddFunc: func(interface{}) { c.markDirty() }, + UpdateFunc: func(interface{}, interface{}) { c.markDirty() }, + DeleteFunc: func(interface{}) { c.markDirty() }, + } + for _, inf := range []cache.SharedIndexInformer{ + svcInformer.Informer(), sliceInformer.Informer(), nodeInformer.Informer(), l2Informer.Informer(), + } { + if _, err := inf.AddEventHandler(handler); err != nil { + l4log.Error(err, "could not register an event handler; the L4 mode stays idle") + <-ctx.Done() + return nil + } + } + + factory.Start(ctx.Done()) + dynFactory.Start(ctx.Done()) + + // The table the previous instance left keeps forwarding until the first + // pass, which needs complete caches: programming from a partial view would + // withdraw live services. + synced := []cache.InformerSynced{ + svcInformer.Informer().HasSynced, + sliceInformer.Informer().HasSynced, + nodeInformer.Informer().HasSynced, + l2Informer.Informer().HasSynced, + } + for !cache.WaitForNamedCacheSync("l4", waitOrTimeout(ctx, time.Minute), synced...) { + if ctx.Err() != nil { + return nil + } + l4log.Info("still waiting for the L4 caches to sync; is the MetalLB ServiceL2Status CRD installed?") + } + + svcLister := svcInformer.Lister() + sliceLister := sliceInformer.Lister() + nodeLister := nodeInformer.Lister() + l2Lister := l2Informer.Lister() + c.input = func() l4.Input { + in := l4.Input{NodeName: c.NodeName, Announced: map[string]bool{}} + in.Services, _ = svcLister.List(labels.Everything()) + in.EndpointSlices, _ = sliceLister.List(labels.Everything()) + in.Nodes, _ = nodeLister.List(labels.Everything()) + objs, _ := l2Lister.List(labels.Everything()) + for _, o := range objs { + if key, ok := announcedService(o, c.NodeName); ok { + in.Announced[key] = true + } + } + return in + } + + c.run(ctx) + l4log.Info("stopping the L4 LoadBalancer mode; its table stays in place") + return nil +} + +// run is the sync loop. +func (c *L4Controller) run(ctx context.Context) { + resync := time.NewTicker(orDefault(c.ResyncInterval, defaultL4ResyncInterval)) + defer resync.Stop() + var retry <-chan time.Time + + // The first pass replaces whatever the previous instance left. A forced + // pass stays forced until it succeeds: retried as an ordinary one, it would + // find the desired state unchanged and skip the commit it exists for. + force := true + for { + if err := c.syncWith(c.input(), force); err != nil { + l4log.Error(err, "L4 sync failed, retrying") + retry = time.After(orDefault(c.RetryInterval, defaultL4RetryInterval)) + } else { + retry = nil + force = false + } + + select { + case <-ctx.Done(): + return + case <-c.dirty: + case <-retry: + case <-resync.C: + force = true + } + // Let a burst of events settle into one pass. + select { + case <-ctx.Done(): + return + case <-time.After(orDefault(c.MinSyncInterval, defaultL4MinSyncInterval)): + } + } +} + +func (c *L4Controller) markDirty() { + select { + case c.dirty <- struct{}{}: + default: + } +} + +// syncWith programs the state computed from in, then purges the conntrack +// entries it left stale. An unchanged state is not reprogrammed unless force +// is set. +func (c *L4Controller) syncWith(in l4.Input, force bool) error { + st, notices := l4.Build(in) + c.logNotices(notices) + + unchanged := c.applied != nil && reflect.DeepEqual(*c.applied, st) + if !unchanged || force { + if err := c.Datapath.Sync(st); err != nil { + return fmt.Errorf("programming the L4 table: %w", err) + } + if !unchanged { + l4log.Info("L4 table programmed", "vips", len(st.VIPs), "ports", len(st.Ports), "translated", len(st.Rules)) + } + prev := c.applied + if c.purgePending || l4.PurgeNeeded(prev, st) { + c.purgePending = true + if prev != nil { + if c.purgeVIPs == nil { + c.purgeVIPs = map[netip.Addr]struct{}{} + } + for _, v := range prev.VIPs { + c.purgeVIPs[v] = struct{}{} + } + } + } + c.applied = &st + } + + if !c.purgePending { + return nil + } + // The previous VIPs keep the flows of a service removed since the last + // successful purge within reach. + before := &l4.State{} + for v := range c.purgeVIPs { + before.VIPs = append(before.VIPs, v) + } + n, err := c.Conntrack.Purge(l4.StaleFlows(before, st)) + if err != nil { + return fmt.Errorf("purging stale L4 conntrack entries: %w", err) + } + if n > 0 { + l4log.Info("purged stale L4 conntrack entries", "count", n) + } + c.purgePending = false + c.purgeVIPs = nil + return nil +} + +func (c *L4Controller) logNotices(notices []l4.Notice) { + current := map[string]string{} + for _, n := range notices { + if prev, ok := current[n.Service]; ok { + current[n.Service] = prev + "; " + n.Message + } else { + current[n.Service] = n.Message + } + } + for svc, msg := range current { + if c.notices[svc] != msg { + l4log.Info("service not fully programmed", "service", svc, "reason", msg) + } + } + c.notices = current +} + +// announcedService returns the service a ServiceL2Status says node announces. +func announcedService(obj interface{}, node string) (string, bool) { + u, ok := obj.(*unstructured.Unstructured) + if !ok { + return "", false + } + // The informer already filters on the node label; the status is the + // authoritative field. + if n, _, _ := unstructured.NestedString(u.Object, "status", "node"); n != node { + return "", false + } + ns, _, _ := unstructured.NestedString(u.Object, "status", "serviceNamespace") + name, _, _ := unstructured.NestedString(u.Object, "status", "serviceName") + if ns == "" || name == "" { + return "", false + } + return ns + "/" + name, true +} + +// waitOrTimeout returns a channel closed when ctx ends or after d, whichever +// comes first, so a cache sync that never completes is reported periodically. +func waitOrTimeout(ctx context.Context, d time.Duration) <-chan struct{} { + ch := make(chan struct{}) + go func() { + defer close(ch) + select { + case <-ctx.Done(): + case <-time.After(d): + } + }() + return ch +} + +func orDefault(d, def time.Duration) time.Duration { + if d <= 0 { + return def + } + return d +} diff --git a/pkg/controllers/l4_controller_test.go b/pkg/controllers/l4_controller_test.go new file mode 100644 index 0000000..cc72c3b --- /dev/null +++ b/pkg/controllers/l4_controller_test.go @@ -0,0 +1,433 @@ +package controllers + +import ( + "context" + "errors" + "net/netip" + "reflect" + "sync" + "testing" + "time" + + v1 "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + dynamicfake "k8s.io/client-go/dynamic/fake" + "k8s.io/client-go/kubernetes/fake" + + "github.com/cozystack/cozy-proxy/pkg/l4" +) + +// fakeL4Datapath records the states it is asked to program. +type fakeL4Datapath struct { + mu sync.Mutex + synced []l4.State + failing bool +} + +func (f *fakeL4Datapath) setFailing(v bool) { + f.mu.Lock() + defer f.mu.Unlock() + f.failing = v +} + +func (f *fakeL4Datapath) Sync(st l4.State) error { + f.mu.Lock() + defer f.mu.Unlock() + if f.failing { + return errors.New("commit refused") + } + f.synced = append(f.synced, st) + return nil +} + +func (f *fakeL4Datapath) Teardown() error { return nil } + +func (f *fakeL4Datapath) count() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.synced) +} + +func (f *fakeL4Datapath) last() (l4.State, bool) { + f.mu.Lock() + defer f.mu.Unlock() + if len(f.synced) == 0 { + return l4.State{}, false + } + return f.synced[len(f.synced)-1], true +} + +// fakePurger runs the predicate over a fixed set of flows, as the conntrack +// table would hold them. +type fakePurger struct { + flows []l4.Flow + calls int + purged []l4.Flow + failing bool +} + +func (f *fakePurger) Purge(stale func(l4.Flow) bool) (uint, error) { + f.calls++ + if f.failing { + return 0, errors.New("dump interrupted") + } + var n uint + var left []l4.Flow + for _, fl := range f.flows { + if stale(fl) { + f.purged = append(f.purged, fl) + n++ + continue + } + left = append(left, fl) + } + f.flows = left + return n, nil +} + +const l4Node = "node-a" + +func l4Svc(name, vip string) *v1.Service { + return &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: name, + Labels: map[string]string{l4.ProxyLabel: l4.ProxyLabelValue}, + Annotations: map[string]string{l4.CiliumTypeAnnotation: l4.CiliumTypeClusterIP}, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyLocal, + Ports: []v1.ServicePort{{Name: "pg", Port: 5432, Protocol: v1.ProtocolTCP}}, + }, + Status: v1.ServiceStatus{LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{{IP: vip}}, + }}, + } +} + +func l4Slice(svc string, ips ...string) *discoveryv1.EndpointSlice { + name, port, proto, ready, node := "pg", int32(5432), v1.ProtocolTCP, true, l4Node + s := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: svc + "-1", + Labels: map[string]string{discoveryv1.LabelServiceName: svc}, + }, + AddressType: discoveryv1.AddressTypeIPv4, + Ports: []discoveryv1.EndpointPort{{Name: &name, Port: &port, Protocol: &proto}}, + } + for _, ip := range ips { + s.Endpoints = append(s.Endpoints, discoveryv1.Endpoint{ + Addresses: []string{ip}, + Conditions: discoveryv1.EndpointConditions{Ready: &ready}, + NodeName: &node, + }) + } + return s +} + +func l4Input(slices ...*discoveryv1.EndpointSlice) l4.Input { + return l4.Input{ + NodeName: l4Node, + Services: []*v1.Service{l4Svc("pg", "192.0.2.10")}, + EndpointSlices: slices, + Announced: map[string]bool{"ns/pg": true}, + } +} + +func flowTo(backend string) l4.Flow { + return l4.Flow{ + Protocol: 6, + OrigDst: netip.MustParseAddrPort("192.0.2.10:5432"), + ReplySrc: netip.MustParseAddrPort(backend), + } +} + +// The first sync programs the state Build computes and purges once, since the +// previous instance may have left translations this one no longer programs. +func TestL4SyncProgramsAndPurgesOnFirstSync(t *testing.T) { + // A flow the previous instance translated through a port the service no + // longer has. + oldPort := l4.Flow{ + Protocol: 6, + OrigDst: netip.MustParseAddrPort("192.0.2.10:5433"), + ReplySrc: netip.MustParseAddrPort("10.0.0.1:5433"), + } + dp, ct := &fakeL4Datapath{}, &fakePurger{flows: []l4.Flow{flowTo("10.0.0.1:5432"), flowTo("10.0.0.9:5432"), oldPort}} + c := &L4Controller{NodeName: l4Node, Datapath: dp, Conntrack: ct} + + in := l4Input(l4Slice("pg", "10.0.0.1")) + if err := c.syncWith(in, false); err != nil { + t.Fatalf("sync: %v", err) + } + + want, _ := l4.Build(in) + if got, ok := dp.last(); !ok || !reflect.DeepEqual(got, want) { + t.Errorf("programmed %+v, want %+v", got, want) + } + if ct.calls != 1 { + t.Fatalf("the first sync must purge once, got %d", ct.calls) + } + // A TCP flow to a frontend still served is kept whatever its backend, as + // kube-proxy would. + if len(ct.purged) != 1 || ct.purged[0] != oldPort { + t.Errorf("purged %+v, want only the flow through the port that is gone", ct.purged) + } +} + +// An unchanged state is neither reprogrammed nor purged again. +func TestL4SyncSkipsUnchangedState(t *testing.T) { + dp, ct := &fakeL4Datapath{}, &fakePurger{} + c := &L4Controller{NodeName: l4Node, Datapath: dp, Conntrack: ct} + in := l4Input(l4Slice("pg", "10.0.0.1")) + + for i := 0; i < 3; i++ { + if err := c.syncWith(in, false); err != nil { + t.Fatalf("sync %d: %v", i, err) + } + } + if dp.count() != 1 || ct.calls != 1 { + t.Errorf("an unchanged state must be programmed once, got %d syncs and %d purges", dp.count(), ct.calls) + } + + // The periodic resync re-applies it anyway, so a table removed by hand + // comes back, but has nothing to purge. + if err := c.syncWith(in, true); err != nil { + t.Fatalf("forced sync: %v", err) + } + if dp.count() != 2 || ct.calls != 1 { + t.Errorf("a forced sync must reprogram without purging, got %d syncs and %d purges", dp.count(), ct.calls) + } +} + +// A TCP backend withdrawn from a frontend that remains keeps its connections +// until they end; its flows only go with the frontend. +func TestL4SyncPurgesTCPFlowsWithTheirFrontendOnly(t *testing.T) { + dp, ct := &fakeL4Datapath{}, &fakePurger{} + c := &L4Controller{NodeName: l4Node, Datapath: dp, Conntrack: ct} + + if err := c.syncWith(l4Input(l4Slice("pg", "10.0.0.1", "10.0.0.2")), false); err != nil { + t.Fatalf("sync: %v", err) + } + ct.flows = []l4.Flow{flowTo("10.0.0.1:5432"), flowTo("10.0.0.2:5432")} + + // A backend added: nothing to purge. + if err := c.syncWith(l4Input(l4Slice("pg", "10.0.0.1", "10.0.0.2", "10.0.0.3")), false); err != nil { + t.Fatalf("sync: %v", err) + } + if ct.calls != 1 { + t.Errorf("adding a backend must not purge, got %d purges", ct.calls) + } + + // A backend withdrawn: its connections are left alone. + if err := c.syncWith(l4Input(l4Slice("pg", "10.0.0.1", "10.0.0.3")), false); err != nil { + t.Fatalf("sync: %v", err) + } + if ct.calls != 1 || len(ct.purged) != 0 { + t.Errorf("withdrawing a TCP backend must not purge, got %d purges, purged %+v", ct.calls, ct.purged) + } + + // The node stops announcing the service: every flow through it goes. + gone := l4Input(l4Slice("pg", "10.0.0.1", "10.0.0.3")) + gone.Announced = nil + if err := c.syncWith(gone, false); err != nil { + t.Fatalf("sync: %v", err) + } + if ct.calls != 2 || len(ct.purged) != 2 { + t.Errorf("losing the announcement must purge every flow, got %d purges, purged %+v", ct.calls, ct.purged) + } +} + +// A refused commit leaves the applied state as it was, and the next attempt +// commits even though the desired state did not change in between. +func TestL4SyncRetriesAFailedCommit(t *testing.T) { + dp, ct := &fakeL4Datapath{failing: true}, &fakePurger{} + c := &L4Controller{NodeName: l4Node, Datapath: dp, Conntrack: ct} + in := l4Input(l4Slice("pg", "10.0.0.1")) + + if err := c.syncWith(in, false); err == nil { + t.Fatal("a refused commit must be reported") + } + if ct.calls != 0 { + t.Error("nothing may be purged before the commit succeeded") + } + + dp.failing = false + if err := c.syncWith(in, false); err != nil { + t.Fatalf("sync: %v", err) + } + if dp.count() != 1 || ct.calls != 1 { + t.Errorf("the retry must commit and purge, got %d syncs and %d purges", dp.count(), ct.calls) + } +} + +// A failed purge is retried on the next sync, and still covers the VIP of a +// service that was removed in the meantime. +func TestL4SyncRetriesAFailedPurge(t *testing.T) { + dp, ct := &fakeL4Datapath{}, &fakePurger{} + c := &L4Controller{NodeName: l4Node, Datapath: dp, Conntrack: ct} + + if err := c.syncWith(l4Input(l4Slice("pg", "10.0.0.1")), false); err != nil { + t.Fatalf("sync: %v", err) + } + + // The service goes away, and the purge that follows fails. + ct.flows = []l4.Flow{flowTo("10.0.0.1:5432")} + ct.failing = true + gone := l4.Input{NodeName: l4Node} + if err := c.syncWith(gone, false); err == nil { + t.Fatal("a failed purge must be reported") + } + + ct.failing = false + if err := c.syncWith(gone, false); err != nil { + t.Fatalf("sync: %v", err) + } + if len(ct.purged) != 1 { + t.Errorf("the retried purge must still reach the removed service's flows, purged %+v", ct.purged) + } + if err := c.syncWith(gone, false); err != nil { + t.Fatalf("sync: %v", err) + } + // The first sync, the failed one and its retry; the last sync has + // nothing left to do. + if ct.calls != 3 { + t.Errorf("purges = %d, want 3", ct.calls) + } +} + +var l2StatusGVR = schema.GroupVersionResource{Group: "metallb.io", Version: "v1beta1", Resource: "servicel2statuses"} + +func l2Status(name, node, svcNS, svcName string) *unstructured.Unstructured { + u := &unstructured.Unstructured{} + u.SetAPIVersion("metallb.io/v1beta1") + u.SetKind("ServiceL2Status") + u.SetNamespace("metallb-system") + u.SetName(name) + u.SetLabels(map[string]string{"metallb.io/node": node}) + _ = unstructured.SetNestedField(u.Object, node, "status", "node") + _ = unstructured.SetNestedField(u.Object, svcName, "status", "serviceName") + _ = unstructured.SetNestedField(u.Object, svcNS, "status", "serviceNamespace") + return u +} + +// Wiring: the controller reads Services, EndpointSlices, Nodes and MetalLB's +// ServiceL2Status from the API, and programs what Build makes of them. +func TestL4ControllerProgramsFromTheAPI(t *testing.T) { + node := &v1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: l4Node}, + Status: v1.NodeStatus{Addresses: []v1.NodeAddress{{Type: v1.NodeInternalIP, Address: "10.200.24.11"}}}, + } + cs := fake.NewSimpleClientset(l4Svc("pg", "192.0.2.10"), l4Slice("pg", "10.0.0.1"), node) + dyn := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), + map[schema.GroupVersionResource]string{l2StatusGVR: "ServiceL2StatusList"}, + // Another node's status must not make this node an announcer. + l2Status("l2-other", "node-b", "ns", "pg"), + ) + + dp := &fakeL4Datapath{} + c := &L4Controller{ + Clientset: cs, + Dynamic: dyn, + NodeName: l4Node, + Datapath: dp, + Conntrack: &fakePurger{}, + MinSyncInterval: time.Millisecond, + } + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- c.Start(ctx) }() + defer func() { + cancel() + if err := <-done; err != nil { + t.Errorf("Start returned %v", err) + } + }() + + notAnnounced := func(st l4.State) bool { + return len(st.VIPs) == 1 && len(st.Ports) == 1 && len(st.Rules) == 0 && len(st.NodeIPs) == 1 + } + waitFor(t, dp, "the VIP guarded but not translated", notAnnounced) + + // MetalLB elects this node. + if _, err := dyn.Resource(l2StatusGVR).Namespace("metallb-system").Create(ctx, + l2Status("l2-here", l4Node, "ns", "pg"), metav1.CreateOptions{}); err != nil { + t.Fatalf("creating the ServiceL2Status: %v", err) + } + waitFor(t, dp, "the VIP translated to the local backend", func(st l4.State) bool { + return len(st.Rules) == 1 && len(st.Rules[0].Backends) == 1 && + st.Rules[0].Backends[0].IP == netip.MustParseAddr("10.0.0.1") + }) + + // The backend goes away. + if _, err := cs.DiscoveryV1().EndpointSlices("ns").Update(ctx, l4Slice("pg"), metav1.UpdateOptions{}); err != nil { + t.Fatalf("updating the EndpointSlice: %v", err) + } + waitFor(t, dp, "the port dropped for lack of backend", func(st l4.State) bool { + return len(st.Rules) == 1 && len(st.Rules[0].Backends) == 0 + }) +} + +func waitFor(t *testing.T, dp *fakeL4Datapath, what string, ok func(l4.State) bool) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if st, synced := dp.last(); synced && ok(st) { + return + } + time.Sleep(10 * time.Millisecond) + } + st, _ := dp.last() + t.Fatalf("timed out waiting for %s; last programmed state: %+v", what, st) +} + +// Without a node name the mode cannot tell what is local: it must refuse to +// program anything, and must not take the process down with it — the VM mode +// runs in the same manager. +func TestL4ControllerWithoutNodeNameProgramsNothing(t *testing.T) { + dp := &fakeL4Datapath{} + c := &L4Controller{Datapath: dp, Conntrack: &fakePurger{}} + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + if err := c.Start(ctx); err != nil { + t.Errorf("Start must not fail the manager, got %v", err) + } + if dp.count() != 0 { + t.Errorf("nothing may be programmed without a node name, got %d syncs", dp.count()) + } +} + +// A forced pass — the first one, or the periodic resync that restores a table +// altered by hand — must stay forced until it succeeds. Retried as an ordinary +// pass, it would find the desired state unchanged and skip the commit. +func TestL4RunKeepsForcingUntilAForcedPassSucceeds(t *testing.T) { + in := l4Input(l4Slice("pg", "10.0.0.1")) + st, _ := l4.Build(in) + dp := &fakeL4Datapath{failing: true} + c := &L4Controller{ + NodeName: l4Node, + Datapath: dp, + Conntrack: &fakePurger{}, + MinSyncInterval: time.Millisecond, + RetryInterval: 5 * time.Millisecond, + ResyncInterval: time.Hour, + applied: &st, // what the datapath last committed, as far as we know + dirty: make(chan struct{}, 1), + input: func() l4.Input { return in }, + } + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { c.run(ctx); close(done) }() + defer func() { cancel(); <-done }() + + time.Sleep(30 * time.Millisecond) + dp.setFailing(false) + waitFor(t, dp, "the forced pass committed on retry", func(got l4.State) bool { + return reflect.DeepEqual(got, st) + }) +} diff --git a/pkg/controllers/services_controller.go b/pkg/controllers/services_controller.go index cb00a46..baccb3c 100644 --- a/pkg/controllers/services_controller.go +++ b/pkg/controllers/services_controller.go @@ -3,6 +3,7 @@ package controllers import ( "context" "fmt" + "strings" "sync" "time" @@ -55,6 +56,22 @@ func (sm *ServiceMap) Get(namespace, name string) (*ServiceEndpoints, bool) { return se, ok } +// Snapshot returns the stored Service and Endpoints under the lock. +// +// Get hands back the pointer to the shared ServiceEndpoints, whose Endpoint +// field SetEndpoint rewrites under the lock. Reading that field after Get has +// returned is an unsynchronized read, so any caller outside the informer +// callbacks has to come through here. +func (sm *ServiceMap) Snapshot(namespace, name string) (*v1.Service, *v1.Endpoints, bool) { + sm.mu.Lock() + defer sm.mu.Unlock() + se, ok := sm.serviceMapping[makeKey(namespace, name)] + if !ok || se == nil { + return nil, nil, false + } + return se.Service, se.Endpoint, true +} + // Set stores the ServiceEndpoints under the given namespace and name. func (sm *ServiceMap) Set(namespace, name string, se *ServiceEndpoints) { sm.mu.Lock() @@ -96,6 +113,380 @@ type ServicesController struct { Clientset *kubernetes.Clientset Services *ServiceMap Proxy nat.ProxyProcessor + + // NodeName is the node this instance runs on. It scopes the ingress + // half of the datapath — the destination rewrite and the port filter — + // to backend pods hosted here. Empty disables the check and programs + // everything everywhere. + NodeName string + + // svcStore and epStore are the informer stores. The startup snapshot is + // built from them rather than from Services, because WaitForCacheSync + // returns once the store is populated, not once every initial callback + // has run. Reading the half-filled map made the purge treat a live + // mapping as stale and delete it, leaving the node without it until an + // event happened to re-apply that service — or until the 12-hour resync. + svcStore cache.Store + epStore cache.Store + + // RetryInterval is how often failed datapath writes are re-attempted. + // Zero selects defaultRetryInterval. + RetryInterval time.Duration + + // reconcileMu serializes whole reconciliations, not just the individual + // datapath calls the proxy already serializes. + // + // An informer callback withdraws the rules of a replaced endpoint and then + // applies the new one. Without this lock the retry goroutine can slip + // between the two with the endpoint it snapshotted a moment earlier, and + // its writes land after the update — restoring the mapping of a pod that + // is gone, which points the service IP at a dead backend. + reconcileMu sync.Mutex + + // retryMu guards pendingServices and pendingCleanup. + retryMu sync.Mutex + + // pendingServices holds the keys of services whose datapath programming + // failed. The informers only deliver events, so without a re-attempt a + // transient nftables failure leaves the service unprogrammed until the + // next event — long enough for a public IP to stay dark for minutes. + pendingServices map[string]struct{} + + // pendingWithdrawals holds datapath state that could not be removed. + // + // A failed withdrawal cannot go through pendingServices: by then the + // service is usually gone from Services, so there is nothing to re-derive + // the pair from. It is carried explicitly instead. Leaving it behind is + // not merely untidy — a stale pod_svc entry rewrites the source of + // whatever pod next receives that IP, which on a shared /16 means one + // tenant's egress leaving under another tenant's service IP. + pendingWithdrawals map[string]withdrawal + + // pendingCleanup records that the startup reconciliation failed and has + // to run again. It stays non-fatal, but it no longer waits for the next + // event or the 12-hour informer resync. + pendingCleanup bool +} + +// withdrawal is datapath state waiting to be removed. +type withdrawal struct { + svcIP, podIP string + // egress also withdraws the source rewrite. False when only the ingress + // half has to go, which is the case on a node that stopped hosting the + // backend but still carries the cluster-wide source rewrite. + egress bool +} + +// withdrawalKey identifies a pair independently of which half is pending. +func withdrawalKey(svcIP, podIP string) string { return svcIP + "/" + podIP } + +// markWithdrawal queues datapath state for another removal attempt. +func (c *ServicesController) markWithdrawal(svcIP, podIP string, egress bool) { + c.retryMu.Lock() + defer c.retryMu.Unlock() + if c.pendingWithdrawals == nil { + c.pendingWithdrawals = make(map[string]withdrawal) + } + k := withdrawalKey(svcIP, podIP) + // A full withdrawal supersedes an ingress-only one for the same pair. + if prev, ok := c.pendingWithdrawals[k]; ok && prev.egress { + egress = true + } + c.pendingWithdrawals[k] = withdrawal{svcIP: svcIP, podIP: podIP, egress: egress} +} + +// clearWithdrawal drops a queued removal. +// +// Called when the same pair is programmed again — an endpoint that flapped back +// to the pod IP whose withdrawal failed. Without this the retry would delete +// the rules that were just reinstalled. +func (c *ServicesController) clearWithdrawal(svcIP, podIP string) { + c.retryMu.Lock() + defer c.retryMu.Unlock() + delete(c.pendingWithdrawals, withdrawalKey(svcIP, podIP)) +} + +// defaultRetryInterval is short enough that a failed write is repaired well +// within a human noticing, and long enough that a permanent failure does not +// flood the log. +const defaultRetryInterval = 30 * time.Second + +// markPending queues a service for another programming attempt. +func (c *ServicesController) markPending(namespace, name string) { + c.retryMu.Lock() + defer c.retryMu.Unlock() + if c.pendingServices == nil { + c.pendingServices = make(map[string]struct{}) + } + c.pendingServices[makeKey(namespace, name)] = struct{}{} +} + +// clearPending drops a service from the retry set after a clean pass. +func (c *ServicesController) clearPending(namespace, name string) { + c.retryMu.Lock() + defer c.retryMu.Unlock() + delete(c.pendingServices, makeKey(namespace, name)) +} + +// markCleanupPending queues the startup reconciliation for another attempt. +func (c *ServicesController) markCleanupPending() { + c.retryMu.Lock() + defer c.retryMu.Unlock() + c.pendingCleanup = true +} + +// takePending returns the queued work and clears it. Anything that fails again +// is re-queued by the attempt itself. +func (c *ServicesController) takePending() (services []string, withdrawals []withdrawal, cleanup bool) { + c.retryMu.Lock() + defer c.retryMu.Unlock() + for k := range c.pendingServices { + services = append(services, k) + } + c.pendingServices = nil + for _, w := range c.pendingWithdrawals { + withdrawals = append(withdrawals, w) + } + c.pendingWithdrawals = nil + cleanup = c.pendingCleanup + c.pendingCleanup = false + return services, withdrawals, cleanup +} + +// retryPending re-attempts everything that failed since the last pass. Every +// datapath call is idempotent, so a re-attempt on something already correct is +// a no-op. +func (c *ServicesController) retryPending() { + services, withdrawals, cleanup := c.takePending() + + // Withdrawals first: an apply for the same pair clears its queued removal + // at the moment it succeeds, so it wins either way. + for _, w := range withdrawals { + c.retryWithdrawal(w) + } + + for _, key := range services { + ns, name, ok := splitKey(key) + if !ok { + continue + } + c.retryOne(ns, name, key) + } + + if cleanup { + log.Info("retrying startup cleanup") + if err := c.cleanupRemovedServices(); err != nil { + // takePending cleared the flag, and unlike the per-service and + // withdrawal paths — which re-queue themselves from inside the + // call that failed — nothing here would put it back. Without this + // the reconciliation is retried exactly once. + c.markCleanupPending() + log.Error(err, "cleanup retry failed, queued again") + } + } +} + +// retryWithdrawal re-attempts a removal that failed. The pair is carried +// explicitly because the service it belonged to is usually gone by now. +func (c *ServicesController) retryWithdrawal(w withdrawal) { + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + + log.Info("retrying datapath withdrawal", "svcIP", w.svcIP, "podIP", w.podIP, "egress", w.egress) + if w.egress { + // Re-queues itself on failure. + _ = c.withdrawRules(w.svcIP, w.podIP, "on retry") + return + } + _ = c.withdrawIngressRules(w.svcIP, w.podIP, "on retry") +} + +// retryOne re-applies a single service, reading its state and programming the +// datapath under the reconciliation lock so an informer callback cannot +// interleave and have this attempt restore what it just withdrew. +func (c *ServicesController) retryOne(namespace, name, key string) { + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + + svc, ep, exists := c.Services.Snapshot(namespace, name) + if !exists || !hasValidServiceIP(svc) || !hasValidEndpointIP(ep) { + // The service went away or lost its endpoint; the delete paths have + // already withdrawn its rules. Drop it from the queue. + c.clearPending(namespace, name) + return + } + log.Info("retrying datapath programming", "service", key) + c.applyRules(svc, ep, "on retry") +} + +// runRetryLoop re-attempts failed datapath writes until the context ends. +func (c *ServicesController) runRetryLoop(ctx context.Context) { + interval := c.RetryInterval + if interval <= 0 { + interval = defaultRetryInterval + } + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + c.retryPending() + } + } +} + +// splitKey reverses makeKey. +func splitKey(key string) (namespace, name string, ok bool) { + i := strings.Index(key, "/") + if i <= 0 || i == len(key)-1 { + return "", "", false + } + return key[:i], key[i+1:], true +} + +// endpointNode returns the node hosting the endpoint's first address. +func endpointNode(ep *v1.Endpoints) (string, bool) { + if !hasValidEndpointIP(ep) { + return "", false + } + node := ep.Subsets[0].Addresses[0].NodeName + if node == nil || *node == "" { + return "", false + } + return *node, true +} + +// servesEndpoint reports whether this node hosts ep's backend, and may +// therefore program the ingress half of the datapath. +// +// Only the hosting node may rewrite the destination. Doing it cluster-wide +// makes a non-owning node translate the destination before the packet even +// leaves it, so the owning node records a conntrack tuple the SNATed reply can +// no longer match, and port_filter drops that reply. +// +// This says nothing about the egress half: the source rewrite is programmed on +// every node regardless, see applyRules. +// +// When the node name is unknown (NODE_NAME not injected, or an endpoint +// carrying no NodeName) the ingress rules are programmed anyway, so an older +// chart keeps the previous behavior instead of silently losing the datapath. +func (c *ServicesController) servesEndpoint(ep *v1.Endpoints) bool { + if c.NodeName == "" { + return true + } + node, ok := endpointNode(ep) + if !ok { + return true + } + return node == c.NodeName +} + +// applyRules programs the datapath for a (service, endpoint) pair. The two +// halves have different scopes. Both objects must already have been checked +// with hasValidServiceIP/hasValidEndpointIP. +func (c *ServicesController) applyRules(svc *v1.Service, ep *v1.Endpoints, ctx string) { + svcIP := svc.Status.LoadBalancer.Ingress[0].IP + podIP := ep.Subsets[0].Addresses[0].IP + + // The source rewrite goes on every node, including the ones that do not + // host the backend. When the client is inside the cluster, kube-ovn SNATs + // it to its own node address, which OVN knows how to reach: the backend's + // reply is then handed straight to that node over the Geneve tunnel and + // never traverses the backend node's netfilter hooks. The client's node is + // the last place where the pod IP can still be turned back into the + // service IP the client's conntrack is waiting for. Without it the reply + // arrives with the wrong source and the client answers it with a RST. + // This pair is wanted again; drop any removal still queued for it. + c.clearWithdrawalFor(svc, ep) + + failed := false + if err := c.Proxy.EnsureEgressSNAT(svcIP, podIP); err != nil { + log.Error(err, "failed to ensure egress SNAT "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = true + } + + // The destination rewrite and the port filter stay on the hosting node. + if !c.servesEndpoint(ep) { + if err := c.withdrawIngressRules(svcIP, podIP, ctx+" (backend not on this node)"); err != nil { + failed = true + } + c.recordOutcome(svc, failed) + return + } + + if err := c.Proxy.EnsureIngressDNAT(svcIP, podIP); err != nil { + log.Error(err, "failed to ensure ingress DNAT "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = true + } + if err := c.reconcilePortFilter(svc, svcIP, podIP, ctx); err != nil { + failed = true + } + c.recordOutcome(svc, failed) +} + +// recordOutcome queues the service for another attempt when any part of its +// datapath failed, and clears a previous failure once a pass is clean. +func (c *ServicesController) recordOutcome(svc *v1.Service, failed bool) { + if failed { + c.markPending(svc.Namespace, svc.Name) + return + } + c.clearPending(svc.Namespace, svc.Name) +} + +// clearWithdrawalFor drops a queued removal for a pair that has just been +// programmed again, so the retry does not delete what was reinstalled. +func (c *ServicesController) clearWithdrawalFor(svc *v1.Service, ep *v1.Endpoints) { + c.clearWithdrawal(svc.Status.LoadBalancer.Ingress[0].IP, ep.Subsets[0].Addresses[0].IP) +} + +// withdrawIngressRules removes the ingress half only — destination rewrite and +// port filter — and leaves the source rewrite in place. This is what a node +// that does not host the backend must end up with. +// It returns an error when any part could not be removed, so the caller can +// queue the pair for another attempt. +func (c *ServicesController) withdrawIngressRules(svcIP, podIP, ctx string) error { + failed := c.clearPortFilter(svcIP, podIP, ctx) + if err := c.Proxy.DeleteIngressDNAT(svcIP, podIP); err != nil { + log.Error(err, "failed to delete ingress DNAT "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err + } + if failed != nil { + c.markWithdrawal(svcIP, podIP, false) + } + return failed +} + +// withdrawRules removes every datapath entry for the pair, both halves. Used +// when the pair itself is going away: service deleted, endpoint gone, or a pod +// IP that has been replaced. Absent entries are not an error. +func (c *ServicesController) withdrawRules(svcIP, podIP, ctx string) error { + failed := c.withdrawIngressRules(svcIP, podIP, ctx) + if err := c.Proxy.DeleteEgressSNAT(svcIP, podIP); err != nil { + log.Error(err, "failed to delete egress SNAT "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err + } + if failed != nil { + c.markWithdrawal(svcIP, podIP, true) + } + return failed +} + +// withdrawStaleEndpoint drops the rules of a previous endpoint whose pod IP no +// longer matches the current one, which is what happens when a VM is migrated +// to another node. Without this the old node keeps a mapping for a pod it no +// longer hosts. +func (c *ServicesController) withdrawStaleEndpoint(svc *v1.Service, prev *v1.Endpoints, podIP, ctx string) { + if !hasValidServiceIP(svc) || !hasValidEndpointIP(prev) { + return + } + prevPodIP := prev.Subsets[0].Addresses[0].IP + if prevPodIP == podIP { + return + } + c.withdrawRules(svc.Status.LoadBalancer.Ingress[0].IP, prevPodIP, ctx+" (stale endpoint)") } // Start initializes the NAT, runs the service and endpoint informers, and cleans up removed services. @@ -132,6 +523,8 @@ func (c *ServicesController) Start(ctx context.Context) error { }, ) + c.svcStore = serviceInformer.GetStore() + serviceInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: c.addServiceFunc, DeleteFunc: c.deleteServiceFunc, @@ -174,6 +567,8 @@ func (c *ServicesController) Start(ctx context.Context) error { }, ) + c.epStore = endpointsInformer.GetStore() + endpointsInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: c.addEndpointFunc, DeleteFunc: c.deleteEndpointFunc, @@ -190,12 +585,21 @@ func (c *ServicesController) Start(ctx context.Context) error { } log.Info("endpoints synchronization completed") - // Run cleanup for removed services. + // Run cleanup for removed services. A failure here is logged but does not + // abort: exiting takes the pod down and leaves the node's datapath + // half-programmed, whereas the informers below converge on the next event. log.Info("running cleanup for removed services") if err := c.cleanupRemovedServices(); err != nil { - return fmt.Errorf("failed to cleanup removed services: %w", err) + log.Error(err, "cleanup of removed services failed, queued for retry") + c.markCleanupPending() + } else { + log.Info("cleanup of removed services completed") } - log.Info("cleanup of removed services completed") + + // Re-attempt whatever failed. The informers only deliver events, so + // without this a transient nftables failure leaves a service + // unprogrammed until the next one. + go c.runRetryLoop(ctx) <-ctx.Done() log.Info("shutting down services-controller") @@ -205,6 +609,10 @@ func (c *ServicesController) Start(ctx context.Context) error { // addServiceFunc handles the addition of a service. func (c *ServicesController) addServiceFunc(obj interface{}) { + // Reconciliations are serialized end to end, see reconcileMu. + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + svc, ok := obj.(*v1.Service) if !ok { // Object is not a Service. @@ -231,16 +639,16 @@ func (c *ServicesController) addServiceFunc(obj interface{}) { if err == nil && ep != nil && hasValidEndpointIP(ep) && hasValidServiceIP(svc) { se.Endpoint = ep c.Services.Set(svc.Namespace, svc.Name, se) - svcIP := svc.Status.LoadBalancer.Ingress[0].IP - podIP := ep.Subsets[0].Addresses[0].IP - // Ensure NAT mapping rules are set. - c.Proxy.EnsureRules(svcIP, podIP) - c.reconcilePortFilter(svc, svcIP, podIP, "on svc add") + c.applyRules(svc, ep, "on svc add") } } // deleteServiceFunc handles the deletion of a service. func (c *ServicesController) deleteServiceFunc(obj interface{}) { + // Reconciliations are serialized end to end, see reconcileMu. + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + svc, ok := obj.(*v1.Service) if !ok { // object is not Service @@ -258,13 +666,16 @@ func (c *ServicesController) deleteServiceFunc(obj interface{}) { svcIP := se.Service.Status.LoadBalancer.Ingress[0].IP podIP := se.Endpoint.Subsets[0].Addresses[0].IP - c.clearPortFilter(svcIP, podIP, "on svc deletion") - c.Proxy.DeleteRules(svcIP, podIP) + c.withdrawRules(svcIP, podIP, "on svc deletion") c.Services.Delete(svc.Namespace, svc.Name) } // updateServiceFunc handles service updates. func (c *ServicesController) updateServiceFunc(oldObj, newObj interface{}) { + // Reconciliations are serialized end to end, see reconcileMu. + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + // Cast the object to a Service type. svc, ok := newObj.(*v1.Service) if !ok { @@ -278,8 +689,7 @@ func (c *ServicesController) updateServiceFunc(oldObj, newObj interface{}) { if hasValidServiceIP(se.Service) && hasValidEndpointIP(se.Endpoint) { svcIP := se.Service.Status.LoadBalancer.Ingress[0].IP podIP := se.Endpoint.Subsets[0].Addresses[0].IP - c.clearPortFilter(svcIP, podIP, "on annotation removal") - c.Proxy.DeleteRules(svcIP, podIP) + c.withdrawRules(svcIP, podIP, "on annotation removal") } c.Services.Delete(svc.Namespace, svc.Name) } @@ -292,8 +702,7 @@ func (c *ServicesController) updateServiceFunc(oldObj, newObj interface{}) { if hasValidServiceIP(se.Service) && hasValidEndpointIP(se.Endpoint) { svcIP := se.Service.Status.LoadBalancer.Ingress[0].IP podIP := se.Endpoint.Subsets[0].Addresses[0].IP - c.clearPortFilter(svcIP, podIP, "on svc IP loss") - c.Proxy.DeleteRules(svcIP, podIP) + c.withdrawRules(svcIP, podIP, "on svc IP loss") } c.Services.Delete(svc.Namespace, svc.Name) } @@ -319,9 +728,10 @@ func (c *ServicesController) updateServiceFunc(oldObj, newObj interface{}) { if ep == nil || !hasValidEndpointIP(ep) { if se, exists := c.Services.Get(svc.Namespace, svc.Name); exists && hasValidServiceIP(se.Service) && hasValidEndpointIP(se.Endpoint) { - c.Proxy.DeleteRules( + c.withdrawRules( se.Service.Status.LoadBalancer.Ingress[0].IP, se.Endpoint.Subsets[0].Addresses[0].IP, + "on endpoint loss", ) } c.Services.Set(svc.Namespace, svc.Name, &ServiceEndpoints{Service: svc, Endpoint: nil}) @@ -330,10 +740,10 @@ func (c *ServicesController) updateServiceFunc(oldObj, newObj interface{}) { // At this point, both the Service and Endpoint have valid IPs. // Ensure NAT mapping is up-to-date. - svcIP := svc.Status.LoadBalancer.Ingress[0].IP - podIP := ep.Subsets[0].Addresses[0].IP - c.Proxy.EnsureRules(svcIP, podIP) - c.reconcilePortFilter(svc, svcIP, podIP, "on svc update") + if se, exists := c.Services.Get(svc.Namespace, svc.Name); exists { + c.withdrawStaleEndpoint(svc, se.Endpoint, ep.Subsets[0].Addresses[0].IP, "on svc update") + } + c.applyRules(svc, ep, "on svc update") // Update or add the service mapping with the new endpoint. c.Services.Set(svc.Namespace, svc.Name, &ServiceEndpoints{Service: svc, Endpoint: ep}) @@ -341,6 +751,10 @@ func (c *ServicesController) updateServiceFunc(oldObj, newObj interface{}) { // addEndpointFunc handles the addition of endpoints. func (c *ServicesController) addEndpointFunc(obj interface{}) { + // Reconciliations are serialized end to end, see reconcileMu. + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + // Cast the object to an Endpoints type. ep, ok := obj.(*v1.Endpoints) if !ok { @@ -360,15 +774,17 @@ func (c *ServicesController) addEndpointFunc(obj interface{}) { // If both the Service and the Endpoint have valid IPs, ensure NAT mapping rules. if hasValidServiceIP(se.Service) && hasValidEndpointIP(ep) { - svcIP := se.Service.Status.LoadBalancer.Ingress[0].IP - podIP := ep.Subsets[0].Addresses[0].IP - c.Proxy.EnsureRules(svcIP, podIP) - c.reconcilePortFilter(se.Service, svcIP, podIP, "on endpoint add") + c.withdrawStaleEndpoint(se.Service, se.Endpoint, ep.Subsets[0].Addresses[0].IP, "on endpoint add") + c.applyRules(se.Service, ep, "on endpoint add") } } // deleteEndpointFunc handles endpoint deletions. func (c *ServicesController) deleteEndpointFunc(obj interface{}) { + // Reconciliations are serialized end to end, see reconcileMu. + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + ep, ok := obj.(*v1.Endpoints) if !ok { // object is not Endpoints @@ -385,14 +801,17 @@ func (c *ServicesController) deleteEndpointFunc(obj interface{}) { } svcIP := se.Service.Status.LoadBalancer.Ingress[0].IP podIP := se.Endpoint.Subsets[0].Addresses[0].IP - c.clearPortFilter(svcIP, podIP, "on endpoint delete") - c.Proxy.DeleteRules(svcIP, podIP) + c.withdrawRules(svcIP, podIP, "on endpoint delete") // Set the endpoint to nil. c.Services.SetEndpoint(ep.Namespace, ep.Name, nil) } // updateEndpointFunc handles updates to endpoints. func (c *ServicesController) updateEndpointFunc(oldObj, newObj interface{}) { + // Reconciliations are serialized end to end, see reconcileMu. + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + ep, ok := newObj.(*v1.Endpoints) if !ok { // object is not Endpoints @@ -408,8 +827,7 @@ func (c *ServicesController) updateEndpointFunc(oldObj, newObj interface{}) { if hasValidServiceIP(se.Service) && hasValidEndpointIP(se.Endpoint) { svcIP := se.Service.Status.LoadBalancer.Ingress[0].IP oldPodIP := se.Endpoint.Subsets[0].Addresses[0].IP - c.clearPortFilter(svcIP, oldPodIP, "on endpoint invalidation") - c.Proxy.DeleteRules(svcIP, oldPodIP) + c.withdrawRules(svcIP, oldPodIP, "on endpoint invalidation") } c.Services.SetEndpoint(ep.Namespace, ep.Name, ep) return @@ -420,10 +838,8 @@ func (c *ServicesController) updateEndpointFunc(oldObj, newObj interface{}) { if !hasValidEndpointIP(ep) { return } - svcIP := se.Service.Status.LoadBalancer.Ingress[0].IP - podIP := ep.Subsets[0].Addresses[0].IP - c.Proxy.EnsureRules(svcIP, podIP) - c.reconcilePortFilter(se.Service, svcIP, podIP, "on endpoint update") + c.withdrawStaleEndpoint(se.Service, se.Endpoint, ep.Subsets[0].Addresses[0].IP, "on endpoint update") + c.applyRules(se.Service, ep, "on endpoint update") c.Services.SetEndpoint(ep.Namespace, ep.Name, ep) } @@ -511,64 +927,118 @@ func allowICMP(svc *v1.Service) bool { // reconcilePortFilter applies the port-filter and ICMP-allow state implied by // the service's annotations. Call sites pass the resolved svcIP/podIP and a // short context string that ends up in error logs. -func (c *ServicesController) reconcilePortFilter(svc *v1.Service, svcIP, podIP, ctx string) { +// It returns an error when any part of the port-filter state could not be +// applied, so the caller can queue the service for another attempt. A filtered +// pod with missing allowed_ports drops every packet, so a silent failure here +// is an outage. +func (c *ServicesController) reconcilePortFilter(svc *v1.Service, svcIP, podIP, ctx string) error { + var failed error if wholeIPPassthrough(svc) { if err := c.Proxy.DeletePortFilter(svcIP, podIP); err != nil { log.Error(err, "failed to delete port filter "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err } if err := c.Proxy.DeleteICMPAllow(svcIP, podIP); err != nil { log.Error(err, "failed to delete ICMP allow "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err } - return + return failed } if err := c.Proxy.EnsurePortFilter(svcIP, podIP, svc.Spec.Ports); err != nil { log.Error(err, "failed to ensure port filter "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err } if allowICMP(svc) { if err := c.Proxy.EnsureICMPAllow(svcIP, podIP); err != nil { log.Error(err, "failed to ensure ICMP allow "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err } } else { if err := c.Proxy.DeleteICMPAllow(svcIP, podIP); err != nil { log.Error(err, "failed to delete ICMP allow "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err } } + return failed } // clearPortFilter unconditionally removes both port-filter and ICMP-allow // state for (svcIP, podIP). Used by delete paths. -func (c *ServicesController) clearPortFilter(svcIP, podIP, ctx string) { +func (c *ServicesController) clearPortFilter(svcIP, podIP, ctx string) error { + var failed error if err := c.Proxy.DeletePortFilter(svcIP, podIP); err != nil { log.Error(err, "failed to delete port filter "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err } if err := c.Proxy.DeleteICMPAllow(svcIP, podIP); err != nil { log.Error(err, "failed to delete ICMP allow "+ctx, "svcIP", svcIP, "podIP", podIP) + failed = err } + return failed +} + +// snapshotSource returns the pairs the datapath should hold. +// +// It reads the informer stores when they are available, because they are +// authoritative as soon as WaitForCacheSync returns, whereas Services is +// filled by callbacks that may not have run yet. Falling back to Services +// keeps the controller usable without informers, which is how the unit tests +// drive it. +func (c *ServicesController) snapshotSource() map[string]*ServiceEndpoints { + if c.svcStore == nil || c.epStore == nil { + return c.Services.GetAll() + } + out := make(map[string]*ServiceEndpoints) + for _, obj := range c.svcStore.List() { + svc, ok := obj.(*v1.Service) + if !ok || !isCozyProxyService(svc) || !hasValidServiceIP(svc) { + continue + } + epObj, exists, err := c.epStore.GetByKey(makeKey(svc.Namespace, svc.Name)) + if err != nil || !exists { + continue + } + ep, ok := epObj.(*v1.Endpoints) + if !ok || !hasValidEndpointIP(ep) { + continue + } + out[makeKey(svc.Namespace, svc.Name)] = &ServiceEndpoints{Service: svc, Endpoint: ep} + } + return out } // cleanupRemovedServices performs an initial cleanup for removed services. func (c *ServicesController) cleanupRemovedServices() error { - keepMap := make(map[string]string) - // Get a snapshot of all managed services. - allServices := c.Services.GetAll() + // Reconciliations are serialized end to end, see reconcileMu. + c.reconcileMu.Lock() + defer c.reconcileMu.Unlock() + + // keepEgress holds every managed pair, because the source rewrite is + // programmed cluster-wide. keepIngress holds only the pairs whose backend + // runs here, because the destination rewrite is node-local. The difference + // between the two is what purges entries inherited from a build that + // scoped both maps alike. + keepEgress := make(map[string]string) + keepIngress := make(map[string]string) + allServices := c.snapshotSource() for _, serviceEndpoints := range allServices { - if serviceEndpoints.Service != nil && serviceEndpoints.Endpoint != nil { - var serviceIP, endpointIP string + if serviceEndpoints.Service == nil || serviceEndpoints.Endpoint == nil { + continue + } + if !hasValidServiceIP(serviceEndpoints.Service) || !hasValidEndpointIP(serviceEndpoints.Endpoint) { + continue + } - if len(serviceEndpoints.Service.Status.LoadBalancer.Ingress) > 0 { - serviceIP = serviceEndpoints.Service.Status.LoadBalancer.Ingress[0].IP - } - if len(serviceEndpoints.Endpoint.Subsets) > 0 && len(serviceEndpoints.Endpoint.Subsets[0].Addresses) > 0 { - endpointIP = serviceEndpoints.Endpoint.Subsets[0].Addresses[0].IP - } + serviceIP := serviceEndpoints.Service.Status.LoadBalancer.Ingress[0].IP + endpointIP := serviceEndpoints.Endpoint.Subsets[0].Addresses[0].IP - if serviceIP != "" && endpointIP != "" { - keepMap[serviceIP] = endpointIP - } + keepEgress[serviceIP] = endpointIP + if c.servesEndpoint(serviceEndpoints.Endpoint) { + keepIngress[serviceIP] = endpointIP } } // Call InitialCleanup with the snapshot. - if err := c.Proxy.CleanupRules(keepMap); err != nil { + if err := c.Proxy.CleanupRules(keepEgress, keepIngress); err != nil { return fmt.Errorf("failed to perform initial cleanup: %w", err) } // Build per-svc port filter snapshot for services in non-passthrough mode. @@ -582,7 +1052,7 @@ func (c *ServicesController) cleanupRemovedServices() error { if !hasValidServiceIP(se.Service) || !hasValidEndpointIP(se.Endpoint) { continue } - if wholeIPPassthrough(se.Service) { + if wholeIPPassthrough(se.Service) || !c.servesEndpoint(se.Endpoint) { continue } keepFilters[se.Service.Status.LoadBalancer.Ingress[0].IP] = nat.PortFilterEntry{ @@ -603,7 +1073,7 @@ func (c *ServicesController) cleanupRemovedServices() error { if !hasValidServiceIP(se.Service) || !hasValidEndpointIP(se.Endpoint) { continue } - if wholeIPPassthrough(se.Service) || !allowICMP(se.Service) { + if wholeIPPassthrough(se.Service) || !allowICMP(se.Service) || !c.servesEndpoint(se.Endpoint) { continue } keepICMP[se.Service.Status.LoadBalancer.Ingress[0].IP] = se.Endpoint.Subsets[0].Addresses[0].IP diff --git a/pkg/controllers/services_controller_test.go b/pkg/controllers/services_controller_test.go index 549e4b4..0b734bd 100644 --- a/pkg/controllers/services_controller_test.go +++ b/pkg/controllers/services_controller_test.go @@ -1,12 +1,226 @@ package controllers import ( + "errors" + "fmt" + "sync" "testing" + "time" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "k8s.io/client-go/tools/cache" + + nat "github.com/cozystack/cozy-proxy/pkg/proxy" ) +// epOnNode builds an Endpoints with a single address, optionally pinned to a +// node. Pass an empty node to omit NodeName, as an endpoint with no scheduling +// information would have. +func epOnNode(podIP, node string) *v1.Endpoints { + addr := v1.EndpointAddress{IP: podIP} + if node != "" { + addr.NodeName = &node + } + return &v1.Endpoints{ + Subsets: []v1.EndpointSubset{{Addresses: []v1.EndpointAddress{addr}}}, + } +} + +// lbService builds a LoadBalancer service already assigned the given IP. +func lbService(svcIP string, annot map[string]string) *v1.Service { + return &v1.Service{ + ObjectMeta: metav1.ObjectMeta{Annotations: annot}, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{{IP: svcIP}}, + }, + }, + } +} + +// recordingProxy captures which datapath calls a controller makes. +type recordingProxy struct { + nat.DummyProxyProcessor + mu sync.Mutex + calls []string + keepEgress map[string]string + keepIngress map[string]string + + // failEgress makes EnsureEgressSNAT fail, standing in for a refused + // nftables commit. + failEgress bool + // failPortFilter makes EnsurePortFilter fail. + failPortFilter bool + // failCleanup makes CleanupRules fail. + failCleanup bool + // failDeleteEgress makes DeleteEgressSNAT fail. + failDeleteEgress bool + // failDeleteIngress makes DeleteIngressDNAT fail. + failDeleteIngress bool +} + +func (r *recordingProxy) EnsureEgressSNAT(svcIP, podIP string) error { + r.record("EnsureEgressSNAT") + if r.failEgress { + return errors.New("commit refused") + } + return nil +} + +func (r *recordingProxy) DeleteEgressSNAT(svcIP, podIP string) error { + r.record("DeleteEgressSNAT") + if r.failDeleteEgress { + return errors.New("commit refused") + } + return nil +} + +func (r *recordingProxy) EnsureIngressDNAT(svcIP, podIP string) error { + r.record("EnsureIngressDNAT") + return nil +} + +func (r *recordingProxy) DeleteIngressDNAT(svcIP, podIP string) error { + r.record("DeleteIngressDNAT") + if r.failDeleteIngress { + return errors.New("commit refused") + } + return nil +} + +func (r *recordingProxy) CleanupRules(keepEgress, keepIngress map[string]string) error { + r.record("CleanupRules") + r.keepEgress = keepEgress + r.keepIngress = keepIngress + if r.failCleanup { + return errors.New("commit refused") + } + return nil +} + +func (r *recordingProxy) EnsurePortFilter(svcIP, podIP string, ports []v1.ServicePort) error { + r.record("EnsurePortFilter") + if r.failPortFilter { + return errors.New("commit refused") + } + return nil +} + +func (r *recordingProxy) DeletePortFilter(svcIP, podIP string) error { + r.record("DeletePortFilter") + return nil +} + +// record appends under the lock: the retry loop and the informer callbacks +// both drive the proxy in the concurrency test. +func (r *recordingProxy) record(call string) { + r.mu.Lock() + defer r.mu.Unlock() + r.calls = append(r.calls, call) +} + +func (r *recordingProxy) has(call string) bool { + r.mu.Lock() + defer r.mu.Unlock() + for _, c := range r.calls { + if c == call { + return true + } + } + return false +} + +func TestServesEndpoint(t *testing.T) { + cases := []struct { + name string + nodeName string + ep *v1.Endpoints + expect bool + }{ + {"backend on this node", "node-a", epOnNode("10.0.0.1", "node-a"), true}, + {"backend on another node", "node-a", epOnNode("10.0.0.1", "node-b"), false}, + {"endpoint without NodeName is programmed", "node-a", epOnNode("10.0.0.1", ""), true}, + {"NODE_NAME unset programs everything", "", epOnNode("10.0.0.1", "node-b"), true}, + {"invalid endpoint is programmed", "node-a", &v1.Endpoints{}, true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ctrl := &ServicesController{NodeName: c.nodeName} + if got := ctrl.servesEndpoint(c.ep); got != c.expect { + t.Errorf("servesEndpoint = %v, want %v", got, c.expect) + } + }) + } +} + +// The owning node programs both halves of the datapath. +func TestApplyRulesProgramsBothHalvesOnOwningNode(t *testing.T) { + svc := lbService("192.0.2.10", map[string]string{"networking.cozystack.io/wholeIP": "false"}) + + local := &recordingProxy{} + localCtrl := &ServicesController{Proxy: local, NodeName: "node-a"} + localCtrl.applyRules(svc, epOnNode("10.0.0.1", "node-a"), "test") + + for _, want := range []string{"EnsureEgressSNAT", "EnsureIngressDNAT", "EnsurePortFilter"} { + if !local.has(want) { + t.Errorf("owning node must call %s, got %v", want, local.calls) + } + } +} + +// A non-owning node must not translate the destination — doing so +// desynchronises conntrack on the owning node and gets the reply dropped by +// port_filter — but it must still program the source rewrite. +// +// That rewrite is the only thing that repairs a reply reaching this node +// straight over the overlay, which is what happens whenever the client is +// inside the cluster and kube-ovn SNATs it to this node's own address. +func TestApplyRulesKeepsEgressSNATOnRemoteBackend(t *testing.T) { + svc := lbService("192.0.2.10", map[string]string{"networking.cozystack.io/wholeIP": "false"}) + + remote := &recordingProxy{} + remoteCtrl := &ServicesController{Proxy: remote, NodeName: "node-a"} + remoteCtrl.applyRules(svc, epOnNode("10.0.0.1", "node-b"), "test") + + if !remote.has("EnsureEgressSNAT") { + t.Errorf("non-owning node must still program the source rewrite, got %v", remote.calls) + } + if remote.has("DeleteEgressSNAT") { + t.Errorf("non-owning node must not withdraw the source rewrite, got %v", remote.calls) + } + if remote.has("EnsureIngressDNAT") || remote.has("EnsurePortFilter") { + t.Errorf("non-owning node must not program the ingress half, got %v", remote.calls) + } + if !remote.has("DeleteIngressDNAT") { + t.Errorf("non-owning node must withdraw an inherited destination rewrite, got %v", remote.calls) + } +} + +// A migrated VM leaves its old mapping behind on the node it came from. +func TestWithdrawStaleEndpoint(t *testing.T) { + svc := lbService("192.0.2.10", map[string]string{"networking.cozystack.io/wholeIP": "false"}) + + moved := &recordingProxy{} + movedCtrl := &ServicesController{Proxy: moved, NodeName: "node-a"} + movedCtrl.withdrawStaleEndpoint(svc, epOnNode("10.0.0.1", "node-a"), "10.0.0.2", "test") + // A pod IP that is gone must leave nothing behind, in either half: every + // node carries its source rewrite, so every node has to drop it. + for _, want := range []string{"DeleteIngressDNAT", "DeleteEgressSNAT"} { + if !moved.has(want) { + t.Errorf("changed pod IP must call %s, got %v", want, moved.calls) + } + } + + same := &recordingProxy{} + sameCtrl := &ServicesController{Proxy: same, NodeName: "node-a"} + sameCtrl.withdrawStaleEndpoint(svc, epOnNode("10.0.0.1", "node-a"), "10.0.0.1", "test") + if len(same.calls) != 0 { + t.Errorf("unchanged pod IP must not touch the datapath, got %v", same.calls) + } +} + func svcWith(annot map[string]string) *v1.Service { return &v1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: annot}} } @@ -78,3 +292,393 @@ func TestAllowICMP(t *testing.T) { }) } } + +// The startup snapshot must keep every managed pair in the egress map and only +// the locally hosted ones in the ingress map. Handing the same set to both is +// exactly what leaves an intra-cluster client without a usable reply. +func TestCleanupRemovedServicesScopesKeepMaps(t *testing.T) { + annot := map[string]string{"networking.cozystack.io/wholeIP": "false"} + + ctrl := &ServicesController{Proxy: &recordingProxy{}, NodeName: "node-a"} + ctrl.Services = NewServiceMap() + ctrl.Services.Set("ns", "local", &ServiceEndpoints{ + Service: lbService("192.0.2.10", annot), + Endpoint: epOnNode("10.0.0.1", "node-a"), + }) + ctrl.Services.Set("ns", "remote", &ServiceEndpoints{ + Service: lbService("192.0.2.11", annot), + Endpoint: epOnNode("10.0.0.2", "node-b"), + }) + // A service still waiting for its endpoint contributes to neither map. + ctrl.Services.Set("ns", "pending", &ServiceEndpoints{ + Service: lbService("192.0.2.12", annot), + Endpoint: nil, + }) + + if err := ctrl.cleanupRemovedServices(); err != nil { + t.Fatalf("cleanupRemovedServices: %v", err) + } + rec := ctrl.Proxy.(*recordingProxy) + + wantEgress := map[string]string{"192.0.2.10": "10.0.0.1", "192.0.2.11": "10.0.0.2"} + if len(rec.keepEgress) != len(wantEgress) { + t.Fatalf("keepEgress = %v, want %v", rec.keepEgress, wantEgress) + } + for svc, pod := range wantEgress { + if rec.keepEgress[svc] != pod { + t.Errorf("keepEgress[%s] = %q, want %q", svc, rec.keepEgress[svc], pod) + } + } + + wantIngress := map[string]string{"192.0.2.10": "10.0.0.1"} + if len(rec.keepIngress) != len(wantIngress) { + t.Fatalf("keepIngress = %v, want %v", rec.keepIngress, wantIngress) + } + if rec.keepIngress["192.0.2.10"] != "10.0.0.1" { + t.Errorf("keepIngress = %v, want %v", rec.keepIngress, wantIngress) + } +} + +// A refused commit must not be forgotten: the informers only deliver events, so +// a service left unprogrammed would stay that way until the next one. +func TestFailedProgrammingIsQueuedForRetry(t *testing.T) { + svc := lbService("192.0.2.10", map[string]string{"networking.cozystack.io/wholeIP": "false"}) + svc.Namespace, svc.Name = "ns", "svc1" + ep := epOnNode("10.0.0.1", "node-a") + + px := &recordingProxy{failEgress: true} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + ctrl.applyRules(svc, ep, "test") + + keys, _, _ := ctrl.takePending() + if len(keys) != 1 || keys[0] != "ns/svc1" { + t.Fatalf("failed write must be queued, got %v", keys) + } + + // A clean pass clears it again. + px.failEgress = false + ctrl.applyRules(svc, ep, "test") + keys, _, _ = ctrl.takePending() + if len(keys) != 0 { + t.Errorf("clean pass must clear the queue, got %v", keys) + } +} + +// A port filter that could not be applied is an outage — the pod is filtered +// with no open port — so it must be queued too. +func TestFailedPortFilterIsQueuedForRetry(t *testing.T) { + svc := lbService("192.0.2.10", map[string]string{"networking.cozystack.io/wholeIP": "false"}) + svc.Namespace, svc.Name = "ns", "svc2" + + px := &recordingProxy{failPortFilter: true} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + ctrl.applyRules(svc, epOnNode("10.0.0.1", "node-a"), "test") + + keys, _, _ := ctrl.takePending() + if len(keys) != 1 || keys[0] != "ns/svc2" { + t.Fatalf("failed port filter must be queued, got %v", keys) + } +} + +// The retry pass must re-apply from the stored state. +func TestRetryPendingReappliesDatapath(t *testing.T) { + svc := lbService("192.0.2.10", map[string]string{"networking.cozystack.io/wholeIP": "false"}) + svc.Namespace, svc.Name = "ns", "svc3" + ep := epOnNode("10.0.0.1", "node-a") + + px := &recordingProxy{failEgress: true} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + ctrl.Services = NewServiceMap() + ctrl.Services.Set("ns", "svc3", &ServiceEndpoints{Service: svc, Endpoint: ep}) + ctrl.applyRules(svc, ep, "test") + + px.failEgress = false + px.calls = nil + ctrl.retryPending() + + for _, want := range []string{"EnsureEgressSNAT", "EnsureIngressDNAT", "EnsurePortFilter"} { + if !px.has(want) { + t.Errorf("retry must call %s, got %v", want, px.calls) + } + } + if keys, _, _ := ctrl.takePending(); len(keys) != 0 { + t.Errorf("successful retry must clear the queue, got %v", keys) + } +} + +// A service that disappeared between the failure and the retry must not be +// re-applied; its delete path has already withdrawn the rules. +func TestRetryPendingSkipsVanishedService(t *testing.T) { + px := &recordingProxy{} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + ctrl.Services = NewServiceMap() + ctrl.markPending("ns", "gone") + + ctrl.retryPending() + if len(px.calls) != 0 { + t.Errorf("vanished service must not be re-applied, got %v", px.calls) + } +} + +// A failed startup cleanup must be re-attempted rather than wait for the next +// event or the 12-hour informer resync. +func TestFailedCleanupIsQueuedAndRetried(t *testing.T) { + px := &recordingProxy{failCleanup: true} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + ctrl.Services = NewServiceMap() + + if err := ctrl.cleanupRemovedServices(); err == nil { + t.Fatal("cleanupRemovedServices must surface the failure") + } + ctrl.markCleanupPending() + + px.failCleanup = false + px.calls = nil + ctrl.retryPending() + if !px.has("CleanupRules") { + t.Errorf("cleanup must be retried, got %v", px.calls) + } + if _, _, pending := ctrl.takePending(); pending { + t.Error("successful cleanup retry must clear the flag") + } +} + +func TestSplitKey(t *testing.T) { + cases := []struct { + key, ns, name string + ok bool + }{ + {"ns/name", "ns", "name", true}, + {"ns/sub/name", "ns", "sub/name", true}, + {"noslash", "", "", false}, + {"/name", "", "", false}, + {"ns/", "", "", false}, + {"", "", "", false}, + } + for _, c := range cases { + ns, name, ok := splitKey(c.key) + if ok != c.ok || ns != c.ns || name != c.name { + t.Errorf("splitKey(%q) = (%q,%q,%v), want (%q,%q,%v)", c.key, ns, name, ok, c.ns, c.name, c.ok) + } + } +} + +// The retry goroutine and the informer callbacks reconcile the same stored +// pair. The retry must read it under the map lock and hold the reconciliation +// lock while it programs, or it applies an endpoint the update has already +// withdrawn — pointing the service IP at a pod that is gone. +// +// Run with -race: an unsynchronized read of the shared Endpoint field shows up +// here, which is what reading it through ServiceMap.Get used to do. +func TestRetryIsSerializedAgainstEndpointUpdates(t *testing.T) { + svc := lbService("192.0.2.10", map[string]string{"networking.cozystack.io/wholeIP": "false"}) + svc.Namespace, svc.Name = "ns", "svc" + + ctrl := &ServicesController{Proxy: &recordingProxy{}, NodeName: "node-a"} + ctrl.Services = NewServiceMap() + ctrl.Services.Set("ns", "svc", &ServiceEndpoints{ + Service: svc, + Endpoint: epOnNode("10.0.0.1", "node-a"), + }) + + stop := make(chan struct{}) + var wg sync.WaitGroup + + // Informer side: keep replacing the endpoint, as a migrating VM does. + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; ; i++ { + select { + case <-stop: + return + default: + } + ctrl.Services.SetEndpoint("ns", "svc", epOnNode(fmt.Sprintf("10.0.0.%d", i%250+1), "node-a")) + } + }() + + // Retry side. + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + } + ctrl.markPending("ns", "svc") + ctrl.retryPending() + } + }() + + time.Sleep(150 * time.Millisecond) + close(stop) + wg.Wait() +} + +// A withdrawal that failed cannot go through the service queue: by then the +// service is gone from the map. Left behind, a stale pod_svc entry rewrites the +// source of whatever pod next receives that IP. +func TestFailedWithdrawalIsQueuedAndRetried(t *testing.T) { + px := &recordingProxy{failDeleteEgress: true} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + ctrl.Services = NewServiceMap() + + if err := ctrl.withdrawRules("192.0.2.10", "10.0.0.1", "test"); err == nil { + t.Fatal("withdrawRules must surface the failure") + } + _, pending, _ := ctrl.takePending() + if len(pending) != 1 || pending[0].svcIP != "192.0.2.10" || !pending[0].egress { + t.Fatalf("failed withdrawal must be queued as a full one, got %+v", pending) + } + + // Re-queue it and let the retry succeed. + ctrl.markWithdrawal("192.0.2.10", "10.0.0.1", true) + px.failDeleteEgress = false + px.calls = nil + ctrl.retryPending() + + for _, want := range []string{"DeleteIngressDNAT", "DeleteEgressSNAT"} { + if !px.has(want) { + t.Errorf("retry must call %s, got %v", want, px.calls) + } + } + if _, pending, _ := ctrl.takePending(); len(pending) != 0 { + t.Errorf("successful retry must clear the queue, got %+v", pending) + } +} + +// An endpoint that flaps back to the pod IP whose withdrawal failed must not +// have its freshly installed rules deleted by the queued removal. +func TestReprogrammingClearsQueuedWithdrawal(t *testing.T) { + svc := lbService("192.0.2.10", map[string]string{"networking.cozystack.io/wholeIP": "false"}) + svc.Namespace, svc.Name = "ns", "svc" + ep := epOnNode("10.0.0.1", "node-a") + + px := &recordingProxy{} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + ctrl.Services = NewServiceMap() + ctrl.markWithdrawal("192.0.2.10", "10.0.0.1", true) + + ctrl.applyRules(svc, ep, "test") + + if _, pending, _ := ctrl.takePending(); len(pending) != 0 { + t.Errorf("programming the pair again must drop its queued removal, got %+v", pending) + } +} + +// A node that stops hosting the backend withdraws only the ingress half, and +// must keep the cluster-wide source rewrite. +func TestIngressOnlyWithdrawalIsQueuedWithoutEgress(t *testing.T) { + px := &recordingProxy{failDeleteIngress: true} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + + if err := ctrl.withdrawIngressRules("192.0.2.10", "10.0.0.1", "test"); err == nil { + t.Fatal("withdrawIngressRules must surface the failure") + } + _, pending, _ := ctrl.takePending() + if len(pending) != 1 || pending[0].egress { + t.Fatalf("ingress-only failure must not queue an egress withdrawal, got %+v", pending) + } +} + +// A full withdrawal supersedes an ingress-only one already queued for the pair. +func TestFullWithdrawalSupersedesIngressOnly(t *testing.T) { + ctrl := &ServicesController{Proxy: &recordingProxy{}} + ctrl.markWithdrawal("192.0.2.10", "10.0.0.1", true) + ctrl.markWithdrawal("192.0.2.10", "10.0.0.1", false) + + _, pending, _ := ctrl.takePending() + if len(pending) != 1 || !pending[0].egress { + t.Fatalf("full withdrawal must win, got %+v", pending) + } +} + +// The startup snapshot must come from the informer stores, not from the map the +// callbacks fill. WaitForCacheSync returns once the store is populated, not +// once every initial callback has run, and reading the half-filled map made the +// purge delete a live mapping it then had to wait for an event to restore. +func TestSnapshotSourcePrefersInformerStores(t *testing.T) { + managed := lbService("192.0.2.10", nil) + managed.Namespace, managed.Name = "ns", "managed" + managed.Labels = map[string]string{"service.kubernetes.io/service-proxy-name": "cozy-proxy"} + + unmanaged := lbService("192.0.2.11", nil) + unmanaged.Namespace, unmanaged.Name = "ns", "unmanaged" + + noIP := &v1.Service{ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", Name: "noip", + Labels: map[string]string{"service.kubernetes.io/service-proxy-name": "cozy-proxy"}, + }} + + noEndpoint := lbService("192.0.2.12", nil) + noEndpoint.Namespace, noEndpoint.Name = "ns", "noep" + noEndpoint.Labels = map[string]string{"service.kubernetes.io/service-proxy-name": "cozy-proxy"} + + svcStore := cache.NewStore(cache.MetaNamespaceKeyFunc) + for _, s := range []*v1.Service{managed, unmanaged, noIP, noEndpoint} { + if err := svcStore.Add(s); err != nil { + t.Fatalf("seeding service store: %v", err) + } + } + + ep := epOnNode("10.0.0.1", "node-a") + ep.Namespace, ep.Name = "ns", "managed" + epStore := cache.NewStore(cache.MetaNamespaceKeyFunc) + if err := epStore.Add(ep); err != nil { + t.Fatalf("seeding endpoint store: %v", err) + } + + ctrl := &ServicesController{Proxy: &recordingProxy{}, svcStore: svcStore, epStore: epStore} + // Deliberately empty: this is the map the callbacks had not filled yet. + ctrl.Services = NewServiceMap() + + got := ctrl.snapshotSource() + if len(got) != 1 { + t.Fatalf("snapshot must hold only the managed service with a valid IP and endpoint, got %d: %v", len(got), got) + } + se, ok := got["ns/managed"] + if !ok || se.Service.Name != "managed" || se.Endpoint.Subsets[0].Addresses[0].IP != "10.0.0.1" { + t.Errorf("unexpected snapshot entry: %+v", got) + } +} + +// Without informers the controller still has to work, which is how the unit +// tests drive it. +func TestSnapshotSourceFallsBackToServiceMap(t *testing.T) { + svc := lbService("192.0.2.10", nil) + svc.Namespace, svc.Name = "ns", "svc" + + ctrl := &ServicesController{Proxy: &recordingProxy{}} + ctrl.Services = NewServiceMap() + ctrl.Services.Set("ns", "svc", &ServiceEndpoints{Service: svc, Endpoint: epOnNode("10.0.0.1", "node-a")}) + + if got := ctrl.snapshotSource(); len(got) != 1 { + t.Errorf("fallback must read the service map, got %v", got) + } +} + +// takePending clears the cleanup flag, so a retry that fails again has to put +// it back — otherwise the reconciliation is attempted exactly once and the +// stale state it exists to remove stays for good. +func TestFailedCleanupRetryIsQueuedAgain(t *testing.T) { + px := &recordingProxy{failCleanup: true} + ctrl := &ServicesController{Proxy: px, NodeName: "node-a"} + ctrl.Services = NewServiceMap() + ctrl.markCleanupPending() + + ctrl.retryPending() + if _, _, pending := ctrl.takePending(); !pending { + t.Fatal("a cleanup retry that failed must be queued again") + } + + // And it stops being queued once it succeeds. + ctrl.markCleanupPending() + px.failCleanup = false + ctrl.retryPending() + if _, _, pending := ctrl.takePending(); pending { + t.Error("a successful cleanup retry must clear the flag") + } +} diff --git a/pkg/l4/conntrack.go b/pkg/l4/conntrack.go new file mode 100644 index 0000000..38a2542 --- /dev/null +++ b/pkg/l4/conntrack.go @@ -0,0 +1,145 @@ +package l4 + +import ( + "net/netip" + + v1 "k8s.io/api/core/v1" +) + +// Flow is the part of a conntrack entry the purge decision needs. +type Flow struct { + // Protocol is the IANA protocol number. + Protocol uint8 + // OrigDst is the destination the client used. + OrigDst netip.AddrPort + // ReplySrc is the source of the replies: the backend when this node + // translated the flow, OrigDst otherwise. + ReplySrc netip.AddrPort +} + +// target is one translation the datapath performs. +type target struct { + vip netip.Addr + protocol uint8 + port uint16 + backend netip.AddrPort +} + +// targets returns the translations whose flows must be kept: the ones the +// datapath performs, and the ones towards draining backends, which finish +// their connections. +func targets(st State) map[target]struct{} { + out := map[target]struct{}{} + for _, r := range st.Rules { + for _, bs := range [][]Backend{r.Backends, r.Draining} { + for _, b := range bs { + out[target{r.VIP, protocolNumber(r.Protocol), r.Port, netip.AddrPortFrom(b.IP, b.Port)}] = struct{}{} + } + } + } + return out +} + +// frontend is a (VIP, protocol, port) this node translates. +type frontend struct { + vip netip.Addr + protocol uint8 + port uint16 +} + +func frontends(st State) map[frontend]struct{} { + out := map[frontend]struct{}{} + for _, r := range st.Rules { + out[frontend{r.VIP, protocolNumber(r.Protocol), r.Port}] = struct{}{} + } + return out +} + +// StaleFlows returns the predicate selecting the conntrack entries to delete +// once cur has been committed, coming from prev (nil after a restart). +// +// A conntrack entry outlives the rule that created it. It is stale when this +// node translated it (its replies do not come from the address the client +// used), towards a VIP the L4 mode knows, and: +// +// - for TCP, when its frontend — the service, the port, or this node's +// announcement — is gone. A TCP connection to a backend withdrawn from a +// frontend that still exists is left to end on its own, or with its pod, +// as kube-proxy does. Purging it helps nobody: on the lab an idle client +// then hung until its own timeout, the backend's reply matching no entry +// and no RST reaching the client. +// - for UDP and the rest, when its backend is no longer programmed nor +// draining: without the purge an active UDP flow would keep being sent to +// a backend that left, forever. +// +// Untranslated flows to a VIP — a pod on a node that does not announce it, +// passing through — are left alone, and so is everything addressed elsewhere, +// which keeps the VM mode out of reach. +func StaleFlows(prev *State, cur State) func(Flow) bool { + vips := map[netip.Addr]struct{}{} + for _, st := range []*State{prev, &cur} { + if st == nil { + continue + } + for _, v := range st.VIPs { + vips[v] = struct{}{} + } + for _, r := range st.Rules { + vips[r.VIP] = struct{}{} + } + } + keep := targets(cur) + served := frontends(cur) + + return func(f Flow) bool { + if _, ours := vips[f.OrigDst.Addr()]; !ours { + return false + } + if f.ReplySrc == f.OrigDst { + return false + } + if f.Protocol == protocolNumber(v1.ProtocolTCP) { + _, stillServed := served[frontend{f.OrigDst.Addr(), f.Protocol, f.OrigDst.Port()}] + return !stillServed + } + _, programmed := keep[target{f.OrigDst.Addr(), f.Protocol, f.OrigDst.Port(), f.ReplySrc}] + return !programmed + } +} + +// PurgeNeeded reports whether moving from prev to cur withdrew something whose +// flows StaleFlows would select: a frontend, or a non-TCP backend. After a +// restart (prev nil) the answer is always yes: changes may have happened while +// no instance was watching. +func PurgeNeeded(prev *State, cur State) bool { + if prev == nil { + return true + } + served := frontends(cur) + for fe := range frontends(*prev) { + if _, ok := served[fe]; !ok { + return true + } + } + now := targets(cur) + for t := range targets(*prev) { + if t.protocol == protocolNumber(v1.ProtocolTCP) { + continue + } + if _, ok := now[t]; !ok { + return true + } + } + return false +} + +func protocolNumber(p v1.Protocol) uint8 { + switch p { + case v1.ProtocolUDP: + return 17 + case v1.ProtocolSCTP: + return 132 + default: + return 6 + } +} diff --git a/pkg/l4/conntrack_test.go b/pkg/l4/conntrack_test.go new file mode 100644 index 0000000..dd47437 --- /dev/null +++ b/pkg/l4/conntrack_test.go @@ -0,0 +1,165 @@ +package l4 + +import ( + "net/netip" + "testing" + + v1 "k8s.io/api/core/v1" +) + +const ( + tcp = 6 + udp = 17 +) + +func ap(s string) netip.AddrPort { return netip.MustParseAddrPort(s) } + +// translated is a flow this node DNATed from vip to backend. +func translated(proto uint8, vip, backend string) Flow { + return Flow{Protocol: proto, OrigDst: ap(vip), ReplySrc: ap(backend)} +} + +func stateWith(rules ...Rule) State { + st := State{} + for _, r := range rules { + st.VIPs = append(st.VIPs, r.VIP) + st.Ports = append(st.Ports, r.PortKey) + } + st.VIPs = sortedUnique(st.VIPs, netip.Addr.Compare) + st.Rules = rules + return st +} + +func rule(vip string, port uint16, backends ...Backend) Rule { + return Rule{PortKey: key(vip, port), Service: "ns/svc", Backends: backends} +} + +func udpRule(vip string, port uint16, backends ...Backend) Rule { + r := rule(vip, port, backends...) + r.Protocol = v1.ProtocolUDP + return r +} + +// The purge follows kube-proxy: a withdrawn endpoint loses its UDP flows, but +// its TCP connections are left to end on their own; TCP flows only go with +// their frontend — the service, the port, or this node's announcement. +func TestStaleFlows(t *testing.T) { + prev := stateWith( + rule("192.0.2.10", 80, be("10.0.0.1", 8080), be("10.0.0.2", 8080)), + rule("192.0.2.10", 443, be("10.0.0.1", 8443)), + udpRule("192.0.2.10", 53, be("10.0.0.1", 5353), be("10.0.0.2", 5353)), + rule("192.0.2.20", 5432, be("10.0.0.3", 5432)), + ) + // 10.0.0.2 went away, port 443 was removed from the service, and the + // 192.0.2.20 service is gone altogether. + cur := stateWith( + rule("192.0.2.10", 80, be("10.0.0.1", 8080)), + udpRule("192.0.2.10", 53, be("10.0.0.1", 5353)), + ) + + stale := StaleFlows(&prev, cur) + + cases := []struct { + name string + flow Flow + want bool + }{ + {"backend still programmed", translated(tcp, "192.0.2.10:80", "10.0.0.1:8080"), false}, + // Purging it would leave an idle client hanging without a RST, as + // seen on the lab; the connection ends on its own, or with its pod. + {"TCP backend gone, frontend kept", translated(tcp, "192.0.2.10:80", "10.0.0.2:8080"), false}, + {"UDP backend gone", translated(udp, "192.0.2.10:53", "10.0.0.2:5353"), true}, + {"UDP backend still programmed", translated(udp, "192.0.2.10:53", "10.0.0.1:5353"), false}, + {"port removed", translated(tcp, "192.0.2.10:443", "10.0.0.1:8443"), true}, + {"service removed", translated(tcp, "192.0.2.20:5432", "10.0.0.3:5432"), true}, + {"protocol not programmed", translated(udp, "192.0.2.10:80", "10.0.0.1:8080"), true}, + // A pod on a node that does not announce the VIP goes through + // untranslated; its flow is not ours to cut. + {"untranslated flow to a VIP", translated(tcp, "192.0.2.10:80", "192.0.2.10:80"), false}, + {"flow to another address", translated(tcp, "198.51.100.1:80", "10.0.0.2:8080"), false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := stale(c.flow); got != c.want { + t.Errorf("stale(%+v) = %v, want %v", c.flow, got, c.want) + } + }) + } +} + +// After a restart there is no previous state: whatever this node translated +// towards a frontend it no longer programs must go — for instance after the +// announcement moved to another node while the pod was down. +func TestStaleFlowsWithoutPreviousState(t *testing.T) { + cur := State{ + VIPs: []netip.Addr{addr("192.0.2.10")}, + Ports: []PortKey{key("192.0.2.10", 80)}, + } + stale := StaleFlows(nil, cur) + if !stale(translated(tcp, "192.0.2.10:80", "10.0.0.1:8080")) { + t.Error("a translation this node no longer programs must be purged") + } + announced := stateWith(rule("192.0.2.10", 80, be("10.0.0.1", 8080))) + if StaleFlows(nil, announced)(translated(tcp, "192.0.2.10:80", "10.0.0.9:8080")) { + t.Error("a TCP flow to a frontend still programmed must be kept, whatever its backend") + } + if stale(translated(tcp, "192.0.2.10:80", "192.0.2.10:80")) { + t.Error("an untranslated flow must be kept") + } +} + +func TestPurgeNeeded(t *testing.T) { + base := stateWith(rule("192.0.2.10", 80, be("10.0.0.1", 8080))) + cases := []struct { + name string + prev *State + cur State + want bool + }{ + {"first sync", nil, base, true}, + {"unchanged", &base, base, false}, + {"backend added", &base, stateWith(rule("192.0.2.10", 80, be("10.0.0.1", 8080), be("10.0.0.2", 8080))), false}, + {"service added", &base, stateWith(rule("192.0.2.10", 80, be("10.0.0.1", 8080)), rule("192.0.2.11", 80)), false}, + {"TCP backend replaced", &base, stateWith(rule("192.0.2.10", 80, be("10.0.0.2", 8080))), false}, + {"UDP backend replaced", ptr(stateWith(udpRule("192.0.2.10", 53, be("10.0.0.1", 5353)))), + stateWith(udpRule("192.0.2.10", 53, be("10.0.0.2", 5353))), true}, + {"port removed", &base, stateWith(rule("192.0.2.10", 81, be("10.0.0.1", 8080))), true}, + {"no longer announced", &base, State{VIPs: base.VIPs, Ports: base.Ports}, true}, + {"node IPs only", &base, State{VIPs: base.VIPs, Ports: base.Ports, Rules: base.Rules, NodeIPs: []netip.Addr{addr("10.200.24.11")}}, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := PurgeNeeded(c.prev, c.cur); got != c.want { + t.Errorf("PurgeNeeded = %v, want %v", got, c.want) + } + }) + } +} + +func TestProtocolNumber(t *testing.T) { + if protocolNumber(v1.ProtocolTCP) != tcp || protocolNumber(v1.ProtocolUDP) != udp { + t.Error("IANA numbers expected for TCP and UDP") + } +} + +// A terminating UDP backend keeps its flows until it leaves the EndpointSlice; +// only then are they purged. +func TestDrainingBackendKeepsItsFlows(t *testing.T) { + ready := stateWith(udpRule("192.0.2.10", 53, be("10.0.0.1", 5353), be("10.0.0.2", 5353))) + draining := stateWith(udpRule("192.0.2.10", 53, be("10.0.0.1", 5353))) + draining.Rules[0].Draining = []Backend{be("10.0.0.2", 5353)} + gone := stateWith(udpRule("192.0.2.10", 53, be("10.0.0.1", 5353))) + + if PurgeNeeded(&ready, draining) { + t.Error("a backend that starts draining must not trigger a purge") + } + if StaleFlows(&ready, draining)(translated(udp, "192.0.2.10:53", "10.0.0.2:5353")) { + t.Error("a draining backend's flows must be kept") + } + if !PurgeNeeded(&draining, gone) { + t.Error("a draining backend that leaves the slice must trigger a purge") + } + if !StaleFlows(&draining, gone)(translated(udp, "192.0.2.10:53", "10.0.0.2:5353")) { + t.Error("the flows of a backend gone from the slice must be purged") + } +} diff --git a/pkg/l4/select.go b/pkg/l4/select.go new file mode 100644 index 0000000..195cc63 --- /dev/null +++ b/pkg/l4/select.go @@ -0,0 +1,85 @@ +// Package l4 computes the desired state of cozy-proxy's L4 LoadBalancer mode: +// which services it takes over from Cilium, and what this node must program for +// them. It holds no datapath code, so it can be tested anywhere; the nftables +// side lives in pkg/proxy. +// +// See docs/rfc/l4-loadbalancer-mode.md for the model. +package l4 + +import ( + v1 "k8s.io/api/core/v1" +) + +const ( + // ProxyLabel selects a Service for the L4 mode. It is deliberately not the + // VM mode's service.kubernetes.io/service-proxy-name: that label also makes + // Cilium drop the ClusterIP and the NodePort, while the L4 mode only takes + // over the public IP. + ProxyLabel = "networking.cozystack.io/lb-proxy" + ProxyLabelValue = "cozy-proxy" + + // CiliumTypeAnnotation set to CiliumTypeClusterIP makes Cilium install only + // the ClusterIP frontend, releasing the VIP. The L4 mode requires it: without + // it Cilium keeps translating the VIP and the two would race. + CiliumTypeAnnotation = "service.cilium.io/type" + CiliumTypeClusterIP = "ClusterIP" + + vmProxyNameLabel = "service.kubernetes.io/service-proxy-name" + vmProxyName = "cozy-proxy" +) + +// Selection says how the L4 mode treats a Service. +type Selection int + +const ( + // NotSelected services do not carry the label and are none of our business. + NotSelected Selection = iota + // Ignored services carry the label but must not be touched, because some + // other component still owns the VIP. + Ignored + // Unsupported services are ours — Cilium has released the VIP — but this + // version cannot program them. Their VIP is still guarded, so that its + // traffic is dropped instead of being routed back to the gateway. + Unsupported + // Managed services are programmed. + Managed +) + +func (s Selection) String() string { + switch s { + case NotSelected: + return "NotSelected" + case Ignored: + return "Ignored" + case Unsupported: + return "Unsupported" + case Managed: + return "Managed" + } + return "Selection(?)" +} + +// Classify decides how the L4 mode treats svc. The reason explains anything +// other than Managed or NotSelected, and is meant for the log. +// +// The label is the trust anchor, not the annotation: the kubevirt CCM copies a +// tenant Service's annotations onto the infra Service, so a tenant can set the +// annotation on an infra object, but not the label. +func Classify(svc *v1.Service) (Selection, string) { + if svc == nil || svc.Labels[ProxyLabel] != ProxyLabelValue { + return NotSelected, "" + } + if svc.Labels[vmProxyNameLabel] == vmProxyName { + return Ignored, "also selected by the VM mode, which owns it" + } + if svc.Spec.Type != v1.ServiceTypeLoadBalancer { + return Ignored, "not a LoadBalancer service" + } + if svc.Annotations[CiliumTypeAnnotation] != CiliumTypeClusterIP { + return Ignored, "missing " + CiliumTypeAnnotation + ": " + CiliumTypeClusterIP + ", Cilium still owns the VIP" + } + if svc.Spec.ExternalTrafficPolicy != v1.ServiceExternalTrafficPolicyLocal { + return Unsupported, "externalTrafficPolicy must be Local" + } + return Managed, "" +} diff --git a/pkg/l4/select_test.go b/pkg/l4/select_test.go new file mode 100644 index 0000000..876d82a --- /dev/null +++ b/pkg/l4/select_test.go @@ -0,0 +1,75 @@ +package l4 + +import ( + "testing" + + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// l4Service builds a LoadBalancer service carrying the L4 label and the Cilium +// annotation, with externalTrafficPolicy Local: the shape the L4 mode manages. +func l4Service(ns, name string, vips ...string) *v1.Service { + svc := &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: name, + Labels: map[string]string{ProxyLabel: ProxyLabelValue}, + Annotations: map[string]string{CiliumTypeAnnotation: CiliumTypeClusterIP}, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyLocal, + }, + } + for _, ip := range vips { + svc.Status.LoadBalancer.Ingress = append(svc.Status.LoadBalancer.Ingress, v1.LoadBalancerIngress{IP: ip}) + } + return svc +} + +func TestClassify(t *testing.T) { + cases := []struct { + name string + mutate func(*v1.Service) + want Selection + }{ + {"label, annotation, eTP Local", func(*v1.Service) {}, Managed}, + {"no label", func(s *v1.Service) { delete(s.Labels, ProxyLabel) }, NotSelected}, + {"label with another value", func(s *v1.Service) { s.Labels[ProxyLabel] = "kube-router" }, NotSelected}, + // Cilium still translates the VIP: programming it too would give the + // address two owners. + {"annotation missing", func(s *v1.Service) { delete(s.Annotations, CiliumTypeAnnotation) }, Ignored}, + {"annotation with another value", func(s *v1.Service) { s.Annotations[CiliumTypeAnnotation] = "LoadBalancer" }, Ignored}, + // The VM mode owns the service; it must keep its behavior untouched. + {"also selected by the VM mode", func(s *v1.Service) { + s.Labels["service.kubernetes.io/service-proxy-name"] = "cozy-proxy" + }, Ignored}, + {"not a LoadBalancer", func(s *v1.Service) { s.Spec.Type = v1.ServiceTypeClusterIP }, Ignored}, + // Cilium has let go of the VIP, so it must at least be guarded, but + // phase 1 cannot program it. + {"eTP Cluster", func(s *v1.Service) { + s.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyCluster + }, Unsupported}, + {"eTP unset defaults to Cluster", func(s *v1.Service) { s.Spec.ExternalTrafficPolicy = "" }, Unsupported}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + svc := l4Service("ns", "svc", "192.0.2.10") + c.mutate(svc) + got, reason := Classify(svc) + if got != c.want { + t.Errorf("Classify = %v (%s), want %v", got, reason, c.want) + } + if got != Managed && got != NotSelected && reason == "" { + t.Errorf("a %v service must come with a reason", got) + } + }) + } +} + +func TestClassifyNil(t *testing.T) { + if got, _ := Classify(nil); got != NotSelected { + t.Errorf("Classify(nil) = %v, want NotSelected", got) + } +} diff --git a/pkg/l4/state.go b/pkg/l4/state.go new file mode 100644 index 0000000..6453293 --- /dev/null +++ b/pkg/l4/state.go @@ -0,0 +1,330 @@ +package l4 + +import ( + "cmp" + "fmt" + "net/netip" + "slices" + "strings" + + v1 "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" +) + +// Backend is one translation target: a ready endpoint hosted on this node, at +// the port the EndpointSlice resolved for the service port. +type Backend struct { + IP netip.Addr + Port uint16 +} + +func (b Backend) String() string { return netip.AddrPortFrom(b.IP, b.Port).String() } + +// PortKey is a declared (VIP, protocol, port) frontend. +type PortKey struct { + VIP netip.Addr + Protocol v1.Protocol + Port uint16 +} + +// Rule is a frontend this node translates. An empty Backends means the port is +// announced here but has no local ready backend: its traffic is dropped, since +// leaving it untranslated would route it back to the gateway. +type Rule struct { + PortKey + // Service is the namespace/name of the Service the frontend belongs to. + Service string + Backends []Backend + // Draining are the local endpoints that are terminating. They get no new + // connection, but their live ones are not purged: a backend shutting down + // gracefully must be able to finish them, as it would behind kube-proxy or + // Cilium. A merely not-ready endpoint is not draining: its probe says it + // cannot serve, so its flows are purged like a removed one's. The datapath + // ignores this field. + Draining []Backend +} + +// State is everything one node programs for the L4 mode. Every slice is sorted, +// so two states can be compared with reflect.DeepEqual. +type State struct { + // VIPs are guarded on every node: new traffic to them that is not on a + // declared port is dropped. + VIPs []netip.Addr + // Ports are the declared frontends of every supported service, on every + // node, including the ones this node does not announce: a pod here must be + // able to reach them through the announcer. + Ports []PortKey + // NodeIPs are the sources masqueraded after translation. + NodeIPs []netip.Addr + // Rules are the frontends this node announces and therefore translates. + Rules []Rule +} + +// Notice explains why a service, or part of it, is not programmed. +type Notice struct { + Service string + Message string +} + +// Input is the cluster state Build reads. +type Input struct { + // NodeName is the node this instance programs. + NodeName string + // Services and EndpointSlices hold every object in the cluster, not only + // the L4 ones: the VM mode's slices are needed to exclude its pods. + Services []*v1.Service + EndpointSlices []*discoveryv1.EndpointSlice + Nodes []*v1.Node + // Announced holds the namespace/name of the services whose VIP this node + // announces. + Announced map[string]bool +} + +// Build computes the state this node must program, and the notices worth +// logging. It is a pure function of its input. +func Build(in Input) (State, []Notice) { + b := builder{in: in, slices: groupSlices(in.EndpointSlices), owner: map[PortKey]string{}} + b.vmPods = b.vmModePods() + + services := slices.Clone(in.Services) + slices.SortFunc(services, func(x, y *v1.Service) int { return cmp.Compare(serviceKey(x), serviceKey(y)) }) + for _, svc := range services { + b.addService(svc) + } + + b.st.NodeIPs = nodeIPs(in.Nodes) + b.st.VIPs = sortedUnique(b.st.VIPs, netip.Addr.Compare) + slices.SortFunc(b.st.Ports, comparePortKey) + slices.SortFunc(b.st.Rules, func(x, y Rule) int { return comparePortKey(x.PortKey, y.PortKey) }) + return b.st, b.notices +} + +type builder struct { + in Input + slices map[string][]*discoveryv1.EndpointSlice + vmPods map[netip.Addr]string + owner map[PortKey]string + st State + notices []Notice +} + +// notice records a message once, however many ports or slices raise it. +func (b *builder) notice(service, format string, args ...any) { + n := Notice{Service: service, Message: fmt.Sprintf(format, args...)} + if slices.Contains(b.notices, n) { + return + } + b.notices = append(b.notices, n) +} + +func (b *builder) addService(svc *v1.Service) { + key := serviceKey(svc) + sel, reason := Classify(svc) + switch sel { + case NotSelected: + return + case Ignored: + b.notice(key, "ignored: %s", reason) + return + } + + vips := serviceVIPs(svc) + b.st.VIPs = append(b.st.VIPs, vips...) + if sel == Unsupported { + b.notice(key, "not programmed, its VIP drops all traffic: %s", reason) + return + } + + announced := b.in.Announced[key] + for _, sp := range svc.Spec.Ports { + proto := sp.Protocol + if proto == "" { + proto = v1.ProtocolTCP + } + if proto != v1.ProtocolTCP { + b.notice(key, "port %s/%d not programmed: only TCP is supported", proto, sp.Port) + continue + } + var backends, draining []Backend + if announced { + backends, draining = b.localBackends(key, svc.Namespace, svc.Name, sp.Name, proto) + } + for _, vip := range vips { + pk := PortKey{VIP: vip, Protocol: proto, Port: uint16(sp.Port)} + if prev, taken := b.owner[pk]; taken { + b.notice(key, "port %s %s/%d not programmed: already served by %s", vip, proto, sp.Port, prev) + continue + } + b.owner[pk] = key + b.st.Ports = append(b.st.Ports, pk) + if announced { + b.st.Rules = append(b.st.Rules, Rule{PortKey: pk, Service: key, Backends: backends, Draining: draining}) + } + } + } +} + +// localBackends returns the endpoints of the service port hosted on this node, +// sorted and deduplicated: the ones serving, and the terminating ones. The port +// is matched by name and protocol, the way kube-proxy does, which also +// resolves named target ports. +func (b *builder) localBackends(key, ns, name, portName string, proto v1.Protocol) (ready, draining []Backend) { + for _, s := range b.slices[ns+"/"+name] { + if s.AddressType != discoveryv1.AddressTypeIPv4 { + continue + } + port, ok := slicePort(s, portName, proto) + if !ok { + continue + } + for _, ep := range s.Endpoints { + if ep.NodeName == nil || *ep.NodeName != b.in.NodeName || len(ep.Addresses) == 0 { + continue + } + ip, err := netip.ParseAddr(ep.Addresses[0]) + if err != nil || !ip.Is4() { + continue + } + if owner, vm := b.vmPods[ip]; vm { + b.notice(key, "backend %s excluded: it is the pod of VM-mode service %s", ip, owner) + continue + } + switch { + case serves(ep): + ready = append(ready, Backend{IP: ip, Port: port}) + case terminating(ep): + draining = append(draining, Backend{IP: ip, Port: port}) + } + } + } + ready = sortedUnique(ready, compareBackend) + // An endpoint listed both ways during a transition counts as serving. + draining = slices.DeleteFunc(sortedUnique(draining, compareBackend), func(d Backend) bool { + _, found := slices.BinarySearchFunc(ready, d, compareBackend) + return found + }) + if len(draining) == 0 { + draining = nil + } + return ready, draining +} + +func compareBackend(x, y Backend) int { + return cmp.Or(x.IP.Compare(y.IP), cmp.Compare(x.Port, y.Port)) +} + +// vmModePods returns the pod IPs backing a VM-mode service. The VM mode +// rewrites their replies' source at priority raw, before conntrack, so an L4 +// translation towards them could never be reversed. +func (b *builder) vmModePods() map[netip.Addr]string { + out := map[netip.Addr]string{} + for _, svc := range b.in.Services { + if svc == nil || svc.Labels[vmProxyNameLabel] != vmProxyName { + continue + } + for _, s := range b.slices[serviceKey(svc)] { + for _, ep := range s.Endpoints { + for _, a := range ep.Addresses { + if ip, err := netip.ParseAddr(a); err == nil { + out[ip] = serviceKey(svc) + } + } + } + } + } + return out +} + +// serves reports whether an endpoint may take new connections. A nil Ready is +// to be read as ready, per the EndpointSlice API. A terminating endpoint does +// not serve even when Ready says true, which publishNotReadyAddresses makes it +// do. +func serves(ep discoveryv1.Endpoint) bool { + if terminating(ep) { + return false + } + return ep.Conditions.Ready == nil || *ep.Conditions.Ready +} + +func terminating(ep discoveryv1.Endpoint) bool { + return ep.Conditions.Terminating != nil && *ep.Conditions.Terminating +} + +func slicePort(s *discoveryv1.EndpointSlice, name string, proto v1.Protocol) (uint16, bool) { + for _, p := range s.Ports { + pName := "" + if p.Name != nil { + pName = *p.Name + } + pProto := v1.ProtocolTCP + if p.Protocol != nil { + pProto = *p.Protocol + } + if pName == name && pProto == proto && p.Port != nil && *p.Port > 0 && *p.Port <= 65535 { + return uint16(*p.Port), true + } + } + return 0, false +} + +func serviceVIPs(svc *v1.Service) []netip.Addr { + var out []netip.Addr + for _, ing := range svc.Status.LoadBalancer.Ingress { + if ip, err := netip.ParseAddr(ing.IP); err == nil && ip.Is4() { + out = append(out, ip) + } + } + return out +} + +func nodeIPs(nodes []*v1.Node) []netip.Addr { + var out []netip.Addr + for _, n := range nodes { + if n == nil { + continue + } + for _, a := range n.Status.Addresses { + if a.Type != v1.NodeInternalIP { + continue + } + if ip, err := netip.ParseAddr(a.Address); err == nil && ip.Is4() { + out = append(out, ip) + } + } + } + return sortedUnique(out, netip.Addr.Compare) +} + +func groupSlices(all []*discoveryv1.EndpointSlice) map[string][]*discoveryv1.EndpointSlice { + out := map[string][]*discoveryv1.EndpointSlice{} + for _, s := range all { + if s == nil { + continue + } + svc := s.Labels[discoveryv1.LabelServiceName] + if svc == "" { + continue + } + k := s.Namespace + "/" + svc + out[k] = append(out[k], s) + } + return out +} + +func serviceKey(svc *v1.Service) string { return svc.Namespace + "/" + svc.Name } + +func comparePortKey(x, y PortKey) int { + return cmp.Or( + x.VIP.Compare(y.VIP), + strings.Compare(string(x.Protocol), string(y.Protocol)), + cmp.Compare(x.Port, y.Port), + ) +} + +func sortedUnique[T comparable](s []T, compare func(T, T) int) []T { + if len(s) == 0 { + return nil + } + slices.SortFunc(s, compare) + return slices.Compact(s) +} diff --git a/pkg/l4/state_test.go b/pkg/l4/state_test.go new file mode 100644 index 0000000..5ecf3de --- /dev/null +++ b/pkg/l4/state_test.go @@ -0,0 +1,458 @@ +package l4 + +import ( + "net/netip" + "reflect" + "strings" + "testing" + + v1 "k8s.io/api/core/v1" + discoveryv1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +const thisNode = "node-a" + +func ptr[T any](v T) *T { return &v } + +// endpoint is a ready endpoint on the given node. +func endpoint(ip, node string) discoveryv1.Endpoint { + return discoveryv1.Endpoint{ + Addresses: []string{ip}, + Conditions: discoveryv1.EndpointConditions{Ready: ptr(true)}, + NodeName: ptr(node), + } +} + +// slice builds an IPv4 EndpointSlice for the service ns/svc. +func slice(ns, svc, name string, ports []discoveryv1.EndpointPort, eps ...discoveryv1.Endpoint) *discoveryv1.EndpointSlice { + return &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: name, + Labels: map[string]string{discoveryv1.LabelServiceName: svc}, + }, + AddressType: discoveryv1.AddressTypeIPv4, + Ports: ports, + Endpoints: eps, + } +} + +func epPort(name string, port int32) discoveryv1.EndpointPort { + return discoveryv1.EndpointPort{Name: ptr(name), Port: ptr(port), Protocol: ptr(v1.ProtocolTCP)} +} + +func svcPort(name string, port int32) v1.ServicePort { + return v1.ServicePort{Name: name, Port: port, Protocol: v1.ProtocolTCP} +} + +func node(name string, addrs ...v1.NodeAddress) *v1.Node { + return &v1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: v1.NodeStatus{Addresses: addrs}, + } +} + +func internalIP(ip string) v1.NodeAddress { + return v1.NodeAddress{Type: v1.NodeInternalIP, Address: ip} +} + +func addr(s string) netip.Addr { return netip.MustParseAddr(s) } + +func be(ip string, port uint16) Backend { return Backend{IP: addr(ip), Port: port} } + +func key(vip string, port uint16) PortKey { + return PortKey{VIP: addr(vip), Protocol: v1.ProtocolTCP, Port: port} +} + +// The phase 1 shape: one announced service, several ports, only the ready +// backends hosted here. +func TestBuildAnnouncedServiceUsesLocalReadyBackends(t *testing.T) { + svc := l4Service("ns", "pg", "192.0.2.10") + svc.Spec.Ports = []v1.ServicePort{svcPort("http", 80), svcPort("https", 443)} + + notReady := endpoint("10.0.0.4", thisNode) + notReady.Conditions.Ready = ptr(false) + terminating := endpoint("10.0.0.5", thisNode) + terminating.Conditions.Terminating = ptr(true) + terminating.Conditions.Ready = ptr(false) + unknown := endpoint("10.0.0.6", thisNode) + unknown.Conditions.Ready = nil // nil means ready, per the API + + in := Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + EndpointSlices: []*discoveryv1.EndpointSlice{slice("ns", "pg", "pg-1", + // Target ports are already resolved in the slice, named targets + // included; they need not match the service port. + []discoveryv1.EndpointPort{epPort("http", 8080), epPort("https", 8443)}, + endpoint("10.0.0.2", thisNode), + endpoint("10.0.0.1", thisNode), + endpoint("10.0.0.3", "node-b"), // remote: eTP Local only uses local backends + notReady, + terminating, + unknown, + )}, + Announced: map[string]bool{"ns/pg": true}, + } + + got, notices := Build(in) + + want := State{ + VIPs: []netip.Addr{addr("192.0.2.10")}, + Ports: []PortKey{key("192.0.2.10", 80), key("192.0.2.10", 443)}, + Rules: []Rule{ + {PortKey: key("192.0.2.10", 80), Service: "ns/pg", + Backends: []Backend{be("10.0.0.1", 8080), be("10.0.0.2", 8080), be("10.0.0.6", 8080)}, + Draining: []Backend{be("10.0.0.5", 8080)}}, + {PortKey: key("192.0.2.10", 443), Service: "ns/pg", + Backends: []Backend{be("10.0.0.1", 8443), be("10.0.0.2", 8443), be("10.0.0.6", 8443)}, + Draining: []Backend{be("10.0.0.5", 8443)}}, + }, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("Build =\n%+v\nwant\n%+v", got, want) + } + if len(notices) != 0 { + t.Errorf("no notice expected, got %v", notices) + } +} + +// Only the announcer translates. Every other node still guards the VIP, and +// lets declared ports through so that its pods' packets reach the announcer. +func TestBuildNotAnnouncedServiceIsGuardedOnly(t *testing.T) { + svc := l4Service("ns", "pg", "192.0.2.10") + svc.Spec.Ports = []v1.ServicePort{svcPort("pg", 5432)} + + got, _ := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + EndpointSlices: []*discoveryv1.EndpointSlice{slice("ns", "pg", "pg-1", + []discoveryv1.EndpointPort{epPort("pg", 5432)}, endpoint("10.0.0.1", thisNode))}, + }) + + if len(got.Rules) != 0 { + t.Errorf("a node that does not announce the VIP must not translate it, got %+v", got.Rules) + } + if !reflect.DeepEqual(got.VIPs, []netip.Addr{addr("192.0.2.10")}) { + t.Errorf("VIPs = %v, want the service VIP", got.VIPs) + } + if !reflect.DeepEqual(got.Ports, []PortKey{key("192.0.2.10", 5432)}) { + t.Errorf("Ports = %v, want the declared port", got.Ports) + } +} + +// An announced port without any local ready backend must be dropped, not left +// untranslated: an untranslated packet would be routed back to the gateway. +func TestBuildPortWithoutBackendIsAnEmptyRule(t *testing.T) { + svc := l4Service("ns", "pg", "192.0.2.10") + svc.Spec.Ports = []v1.ServicePort{svcPort("pg", 5432), svcPort("metrics", 9187)} + + got, _ := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + EndpointSlices: []*discoveryv1.EndpointSlice{slice("ns", "pg", "pg-1", + []discoveryv1.EndpointPort{epPort("pg", 5432)}, // no "metrics" port in the slice + endpoint("10.0.0.1", "node-b"))}, // and no local backend at all + Announced: map[string]bool{"ns/pg": true}, + }) + + want := []Rule{ + {PortKey: key("192.0.2.10", 5432), Service: "ns/pg"}, + {PortKey: key("192.0.2.10", 9187), Service: "ns/pg"}, + } + if !reflect.DeepEqual(got.Rules, want) { + t.Errorf("Rules = %+v, want empty rules for both ports %+v", got.Rules, want) + } +} + +// The kubevirt CCM creates selector-less services whose single port has no +// name, and writes their slices itself. +func TestBuildSelectorlessCCMService(t *testing.T) { + svc := l4Service("tenant-k8s", "a6ed08", "192.0.2.20") + svc.Spec.Ports = []v1.ServicePort{{Port: 80, Protocol: v1.ProtocolTCP}} + + s := slice("tenant-k8s", "a6ed08", "a6ed08-x", []discoveryv1.EndpointPort{ + {Name: ptr(""), Port: ptr(int32(31790)), Protocol: ptr(v1.ProtocolTCP)}, + }, endpoint("10.0.80.67", thisNode)) + s.Labels[discoveryv1.LabelManagedBy] = "kubevirt-eps-controller" + + got, _ := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + EndpointSlices: []*discoveryv1.EndpointSlice{s}, + Announced: map[string]bool{"tenant-k8s/a6ed08": true}, + }) + + want := []Rule{{PortKey: key("192.0.2.20", 80), Service: "tenant-k8s/a6ed08", + Backends: []Backend{be("10.0.80.67", 31790)}}} + if !reflect.DeepEqual(got.Rules, want) { + t.Errorf("Rules = %+v, want %+v", got.Rules, want) + } +} + +// Endpoints of one service can transiently appear in two slices. +func TestBuildDeduplicatesBackendsAcrossSlices(t *testing.T) { + svc := l4Service("ns", "web", "192.0.2.10") + svc.Spec.Ports = []v1.ServicePort{svcPort("http", 80)} + ports := []discoveryv1.EndpointPort{epPort("http", 8080)} + + got, _ := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + EndpointSlices: []*discoveryv1.EndpointSlice{ + slice("ns", "web", "web-1", ports, endpoint("10.0.0.1", thisNode)), + slice("ns", "web", "web-2", ports, endpoint("10.0.0.1", thisNode), endpoint("10.0.0.2", thisNode)), + // Same name, other namespace: not this service's slice. + slice("other", "web", "web-3", ports, endpoint("10.0.0.9", thisNode)), + }, + Announced: map[string]bool{"ns/web": true}, + }) + + want := []Backend{be("10.0.0.1", 8080), be("10.0.0.2", 8080)} + if len(got.Rules) != 1 || !reflect.DeepEqual(got.Rules[0].Backends, want) { + t.Errorf("Rules = %+v, want one rule with %v", got.Rules, want) + } +} + +func TestBuildIgnoresNonIPv4AndUnscheduledEndpoints(t *testing.T) { + svc := l4Service("ns", "web", "192.0.2.10", "2001:db8::10") + svc.Spec.Ports = []v1.ServicePort{svcPort("http", 80)} + ports := []discoveryv1.EndpointPort{epPort("http", 8080)} + + v6 := slice("ns", "web", "web-v6", ports, endpoint("2001:db8::1", thisNode)) + v6.AddressType = discoveryv1.AddressTypeIPv6 + noNode := endpoint("10.0.0.2", thisNode) + noNode.NodeName = nil + + got, _ := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + EndpointSlices: []*discoveryv1.EndpointSlice{v6, slice("ns", "web", "web-v4", ports, endpoint("10.0.0.1", thisNode), noNode)}, + Announced: map[string]bool{"ns/web": true}, + }) + + if !reflect.DeepEqual(got.VIPs, []netip.Addr{addr("192.0.2.10")}) { + t.Errorf("VIPs = %v, want only the IPv4 one", got.VIPs) + } + want := []Rule{{PortKey: key("192.0.2.10", 80), Service: "ns/web", Backends: []Backend{be("10.0.0.1", 8080)}}} + if !reflect.DeepEqual(got.Rules, want) { + t.Errorf("Rules = %+v, want %+v", got.Rules, want) + } +} + +// Phase 1 is TCP only. A UDP port is left out of the declared ports, so the +// guard drops it rather than letting it loop. +func TestBuildLeavesUDPPortsOut(t *testing.T) { + svc := l4Service("ns", "dns", "192.0.2.10") + svc.Spec.Ports = []v1.ServicePort{svcPort("tcp", 53), {Name: "udp", Port: 53, Protocol: v1.ProtocolUDP}} + + got, notices := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + EndpointSlices: []*discoveryv1.EndpointSlice{slice("ns", "dns", "dns-1", []discoveryv1.EndpointPort{ + epPort("tcp", 53), + {Name: ptr("udp"), Port: ptr(int32(53)), Protocol: ptr(v1.ProtocolUDP)}, + }, endpoint("10.0.0.1", thisNode))}, + Announced: map[string]bool{"ns/dns": true}, + }) + + if !reflect.DeepEqual(got.Ports, []PortKey{key("192.0.2.10", 53)}) { + t.Errorf("Ports = %v, want only TCP/53", got.Ports) + } + if len(got.Rules) != 1 || got.Rules[0].Protocol != v1.ProtocolTCP { + t.Errorf("Rules = %+v, want only the TCP rule", got.Rules) + } + if !hasNotice(notices, "ns/dns", "UDP") { + t.Errorf("a skipped port must be reported, got %v", notices) + } +} + +// A service Cilium released but phase 1 cannot program is guarded: its VIP +// drops everything. +func TestBuildUnsupportedServiceIsGuardedWithNoPort(t *testing.T) { + svc := l4Service("ns", "maria", "192.0.2.10") + svc.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyCluster + svc.Spec.Ports = []v1.ServicePort{svcPort("mysql", 3306)} + + got, notices := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + Announced: map[string]bool{"ns/maria": true}, + }) + + if !reflect.DeepEqual(got.VIPs, []netip.Addr{addr("192.0.2.10")}) { + t.Errorf("VIPs = %v, want the VIP guarded", got.VIPs) + } + if len(got.Ports) != 0 || len(got.Rules) != 0 { + t.Errorf("an unsupported service must not be opened, got ports %v rules %v", got.Ports, got.Rules) + } + if !hasNotice(notices, "ns/maria", "externalTrafficPolicy") { + t.Errorf("an unsupported service must be reported, got %v", notices) + } +} + +// Services some other component owns are left entirely alone, guard included. +func TestBuildIgnoredAndUnselectedServicesAreUntouched(t *testing.T) { + noAnnotation := l4Service("ns", "half", "192.0.2.10") + delete(noAnnotation.Annotations, CiliumTypeAnnotation) + plain := l4Service("ns", "plain", "192.0.2.11") + delete(plain.Labels, ProxyLabel) + + got, notices := Build(Input{NodeName: thisNode, Services: []*v1.Service{noAnnotation, plain}}) + + if len(got.VIPs) != 0 || len(got.Ports) != 0 || len(got.Rules) != 0 { + t.Errorf("nothing must be programmed, got %+v", got) + } + if !hasNotice(notices, "ns/half", CiliumTypeAnnotation) { + t.Errorf("an ignored labelled service must be reported, got %v", notices) + } + if hasNotice(notices, "ns/plain", "") { + t.Errorf("an unselected service is none of our business, got %v", notices) + } +} + +// A service waiting for its VIP contributes nothing yet. +func TestBuildServiceWithoutVIP(t *testing.T) { + svc := l4Service("ns", "pending") + svc.Spec.Ports = []v1.ServicePort{svcPort("pg", 5432)} + got, _ := Build(Input{NodeName: thisNode, Services: []*v1.Service{svc}, Announced: map[string]bool{"ns/pending": true}}) + if len(got.VIPs) != 0 || len(got.Ports) != 0 || len(got.Rules) != 0 { + t.Errorf("nothing must be programmed before the VIP is allocated, got %+v", got) + } +} + +// Masquerading is restricted to the nodes' own addresses, read from the Node +// objects rather than guessed as a CIDR: the LoadBalancer pool can sit in the +// node subnet, and a VM client's public IP must not be masqueraded. +func TestBuildNodeIPs(t *testing.T) { + got, _ := Build(Input{ + NodeName: thisNode, + Nodes: []*v1.Node{ + node("node-b", internalIP("10.200.24.12"), v1.NodeAddress{Type: v1.NodeHostName, Address: "node-b"}), + node("node-a", internalIP("10.200.24.11"), internalIP("fd00::11"), + v1.NodeAddress{Type: v1.NodeExternalIP, Address: "198.51.100.1"}), + node("node-c", internalIP("10.200.24.12")), // duplicate address + }, + }) + want := []netip.Addr{addr("10.200.24.11"), addr("10.200.24.12")} + if !reflect.DeepEqual(got.NodeIPs, want) { + t.Errorf("NodeIPs = %v, want %v", got.NodeIPs, want) + } +} + +// A VM-mode pod rewrites its replies' source at priority raw, before +// conntrack, so an L4 translation towards it could never be reversed. +func TestBuildExcludesBackendsOwnedByTheVMMode(t *testing.T) { + vm := l4Service("tenant", "vm", "192.0.2.50") + delete(vm.Annotations, CiliumTypeAnnotation) + delete(vm.Labels, ProxyLabel) + vm.Labels[vmProxyNameLabel] = vmProxyName + + web := l4Service("tenant", "web", "192.0.2.10") + web.Spec.Ports = []v1.ServicePort{svcPort("http", 80), svcPort("https", 443)} + ports := []discoveryv1.EndpointPort{epPort("http", 8080), epPort("https", 8443)} + + got, notices := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{vm, web}, + EndpointSlices: []*discoveryv1.EndpointSlice{ + slice("tenant", "vm", "vm-1", nil, endpoint("10.0.0.7", thisNode)), + slice("tenant", "web", "web-1", ports, endpoint("10.0.0.7", thisNode), endpoint("10.0.0.8", thisNode)), + }, + Announced: map[string]bool{"tenant/web": true}, + }) + + want := []Backend{be("10.0.0.8", 8080)} + if len(got.Rules) != 2 || !reflect.DeepEqual(got.Rules[0].Backends, want) { + t.Errorf("Rules = %+v, want only %v on port 80", got.Rules, want) + } + if !hasNotice(notices, "tenant/web", "10.0.0.7") { + t.Errorf("an excluded backend must be reported, got %v", notices) + } + // Once, although both ports raise it. + if len(notices) != 1 { + t.Errorf("notices = %v, want a single one", notices) + } +} + +// MetalLB can share a VIP between services on distinct ports. Two services +// declaring the same port cannot both be served; the first one in key order +// keeps it. +func TestBuildSharedVIP(t *testing.T) { + a := l4Service("ns", "a", "192.0.2.10") + a.Spec.Ports = []v1.ServicePort{svcPort("http", 80)} + b := l4Service("ns", "b", "192.0.2.10") + b.Spec.Ports = []v1.ServicePort{svcPort("http", 80), svcPort("ssh", 22)} + ports := []discoveryv1.EndpointPort{epPort("http", 8080), epPort("ssh", 22)} + + got, notices := Build(Input{ + NodeName: thisNode, + // Deliberately out of order: the result must not depend on it. + Services: []*v1.Service{b, a}, + EndpointSlices: []*discoveryv1.EndpointSlice{ + slice("ns", "a", "a-1", ports, endpoint("10.0.0.1", thisNode)), + slice("ns", "b", "b-1", ports, endpoint("10.0.0.2", thisNode)), + }, + Announced: map[string]bool{"ns/a": true, "ns/b": true}, + }) + + if !reflect.DeepEqual(got.VIPs, []netip.Addr{addr("192.0.2.10")}) { + t.Errorf("VIPs = %v, want the shared VIP once", got.VIPs) + } + want := []Rule{ + {PortKey: key("192.0.2.10", 22), Service: "ns/b", Backends: []Backend{be("10.0.0.2", 22)}}, + {PortKey: key("192.0.2.10", 80), Service: "ns/a", Backends: []Backend{be("10.0.0.1", 8080)}}, + } + if !reflect.DeepEqual(got.Rules, want) { + t.Errorf("Rules = %+v, want %+v", got.Rules, want) + } + if !hasNotice(notices, "ns/b", "ns/a") { + t.Errorf("the conflict must be reported on the losing service, got %v", notices) + } +} + +func hasNotice(notices []Notice, service, substr string) bool { + for _, n := range notices { + if n.Service == service && strings.Contains(n.Message, substr) { + return true + } + } + return false +} + +// A terminating local endpoint gets no new connection, but is kept as +// draining, so its live connections are not purged: a rolling update of an +// Ingress must not reset every client. A merely not-ready one is not: its +// probe says it cannot serve, so its flows are purged like a removed one's. +func TestBuildKeepsTerminatingLocalEndpointsAsDraining(t *testing.T) { + svc := l4Service("ns", "web", "192.0.2.10") + svc.Spec.Ports = []v1.ServicePort{svcPort("http", 80)} + + notReady := endpoint("10.0.0.2", thisNode) + notReady.Conditions.Ready = ptr(false) + // publishNotReadyAddresses keeps Ready true on a terminating endpoint. + terminating := endpoint("10.0.0.3", thisNode) + terminating.Conditions.Terminating = ptr(true) + remote := endpoint("10.0.0.4", "node-b") + remote.Conditions.Ready = ptr(false) + + got, _ := Build(Input{ + NodeName: thisNode, + Services: []*v1.Service{svc}, + EndpointSlices: []*discoveryv1.EndpointSlice{slice("ns", "web", "web-1", + []discoveryv1.EndpointPort{epPort("http", 8080)}, + endpoint("10.0.0.1", thisNode), notReady, terminating, remote)}, + Announced: map[string]bool{"ns/web": true}, + }) + + want := []Rule{{ + PortKey: key("192.0.2.10", 80), + Service: "ns/web", + Backends: []Backend{be("10.0.0.1", 8080)}, + Draining: []Backend{be("10.0.0.3", 8080)}, + }} + if !reflect.DeepEqual(got.Rules, want) { + t.Errorf("Rules = %+v, want %+v", got.Rules, want) + } +} diff --git a/pkg/proxy/conntrack.go b/pkg/proxy/conntrack.go new file mode 100644 index 0000000..5a51c3f --- /dev/null +++ b/pkg/proxy/conntrack.go @@ -0,0 +1,69 @@ +package proxy + +import ( + "fmt" + "net/netip" + + "github.com/vishvananda/netlink" + "golang.org/x/sys/unix" + + "github.com/cozystack/cozy-proxy/pkg/l4" +) + +// NetlinkConntrack deletes conntrack entries over netlink. +type NetlinkConntrack struct { + // Handle, when set, is used instead of the default one. Tests use it to + // work in a scratch network namespace. + Handle *netlink.Handle +} + +// Purge deletes the IPv4 conntrack entries stale selects. The kernel has no +// filter for "translated to this backend", so the table is dumped and matched +// here, which is why the caller only purges when a translation was withdrawn. +func (c *NetlinkConntrack) Purge(stale func(l4.Flow) bool) (uint, error) { + h := c.Handle + if h == nil { + var err error + if h, err = netlink.NewHandle(unix.NETLINK_NETFILTER); err != nil { + return 0, fmt.Errorf("could not open a netfilter netlink handle: %w", err) + } + defer h.Close() + } + n, err := h.ConntrackDeleteFilters(netlink.ConntrackTable, unix.AF_INET, flowFilter(stale)) + if err != nil { + return n, fmt.Errorf("conntrack purge: %w", err) + } + return n, nil +} + +// flowFilter adapts a predicate on l4.Flow to the netlink filter interface. +type flowFilter func(l4.Flow) bool + +func (f flowFilter) MatchConntrackFlow(flow *netlink.ConntrackFlow) bool { + lf, ok := toFlow(flow) + return ok && f(lf) +} + +// toFlow extracts what the purge decision needs: the destination the client +// used, and where the replies come from — the backend, when translated. +func toFlow(f *netlink.ConntrackFlow) (l4.Flow, bool) { + if f == nil { + return l4.Flow{}, false + } + dst, ok := netip.AddrFromSlice(f.Forward.DstIP) + if !ok { + return l4.Flow{}, false + } + src, ok := netip.AddrFromSlice(f.Reverse.SrcIP) + if !ok { + return l4.Flow{}, false + } + return l4.Flow{ + Protocol: f.Forward.Protocol, + OrigDst: netip.AddrPortFrom(dst.Unmap(), f.Forward.DstPort), + ReplySrc: netip.AddrPortFrom(src.Unmap(), f.Reverse.SrcPort), + }, true +} + +// Compile-time assertion that NetlinkConntrack satisfies ConntrackPurger. +var _ ConntrackPurger = (*NetlinkConntrack)(nil) diff --git a/pkg/proxy/conntrack_test.go b/pkg/proxy/conntrack_test.go new file mode 100644 index 0000000..2a65a73 --- /dev/null +++ b/pkg/proxy/conntrack_test.go @@ -0,0 +1,124 @@ +//go:build linux + +package proxy + +import ( + "net" + "net/netip" + "testing" + + "github.com/vishvananda/netlink" + "github.com/vishvananda/netns" + "golang.org/x/sys/unix" + + "github.com/cozystack/cozy-proxy/pkg/l4" +) + +// conntrackIn returns a netlink handle working in ns. +func conntrackIn(t *testing.T, ns netns.NsHandle) *netlink.Handle { + t.Helper() + h, err := netlink.NewHandleAt(ns) + if err != nil { + t.Fatalf("netlink.NewHandleAt: %v", err) + } + t.Cleanup(h.Close) + return h +} + +// createFlow adds a TCP entry translated from client -> origDst to +// replySrc -> client, the way a DNAT leaves it. +func createFlow(t *testing.T, h *netlink.Handle, client, origDst, replySrc string) { + t.Helper() + c := netip.MustParseAddrPort(client) + d := netip.MustParseAddrPort(origDst) + r := netip.MustParseAddrPort(replySrc) + flow := &netlink.ConntrackFlow{ + FamilyType: unix.AF_INET, + Forward: netlink.IPTuple{ + Protocol: unix.IPPROTO_TCP, + SrcIP: net.IP(c.Addr().AsSlice()), SrcPort: c.Port(), + DstIP: net.IP(d.Addr().AsSlice()), DstPort: d.Port(), + }, + Reverse: netlink.IPTuple{ + Protocol: unix.IPPROTO_TCP, + SrcIP: net.IP(r.Addr().AsSlice()), SrcPort: r.Port(), + DstIP: net.IP(c.Addr().AsSlice()), DstPort: c.Port(), + }, + TimeOut: 300, + ProtoInfo: &netlink.ProtoInfoTCP{State: 3}, // TCP_CONNTRACK_ESTABLISHED + } + if err := h.ConntrackCreate(netlink.ConntrackTable, unix.AF_INET, flow); err != nil { + t.Fatalf("ConntrackCreate %s -> %s: %v", client, origDst, err) + } +} + +func listFlows(t *testing.T, h *netlink.Handle) []l4.Flow { + t.Helper() + flows, err := h.ConntrackTableList(netlink.ConntrackTable, unix.AF_INET) + if err != nil { + t.Fatalf("ConntrackTableList: %v", err) + } + var out []l4.Flow + for _, f := range flows { + if lf, ok := toFlow(f); ok { + out = append(out, lf) + } + } + return out +} + +func TestNetlinkConntrackPurgeDeletesSelectedFlowsOnly(t *testing.T) { + ns := scratchNetns(t) + h := conntrackIn(t, ns) + + createFlow(t, h, "198.51.100.7:40001", "192.0.2.10:80", "10.244.0.10:8080") // backend kept + createFlow(t, h, "198.51.100.7:40002", "192.0.2.10:80", "10.244.0.11:8080") // backend gone + createFlow(t, h, "198.51.100.7:40003", "203.0.113.5:443", "10.244.0.11:8443") // same backend IP, other port + + gone := netip.MustParseAddrPort("10.244.0.11:8080") + purger := &NetlinkConntrack{Handle: h} + n, err := purger.Purge(func(f l4.Flow) bool { return f.ReplySrc == gone }) + if err != nil { + t.Fatalf("Purge: %v", err) + } + if n != 1 { + t.Errorf("Purge deleted %d flows, want 1", n) + } + + left := listFlows(t, h) + if len(left) != 2 { + t.Fatalf("flows left = %+v, want 2", left) + } + for _, f := range left { + if f.ReplySrc == gone { + t.Errorf("the selected flow survived: %+v", f) + } + } +} + +// The flow handed to the predicate must carry the translation: the original +// destination and the source of the replies. +func TestToFlow(t *testing.T) { + f := &netlink.ConntrackFlow{ + FamilyType: unix.AF_INET, + Forward: netlink.IPTuple{Protocol: unix.IPPROTO_TCP, + SrcIP: net.ParseIP("198.51.100.7"), SrcPort: 40001, + DstIP: net.ParseIP("192.0.2.10"), DstPort: 80}, + Reverse: netlink.IPTuple{Protocol: unix.IPPROTO_TCP, + SrcIP: net.ParseIP("10.244.0.10"), SrcPort: 8080, + DstIP: net.ParseIP("100.64.0.4"), DstPort: 21653}, + } + got, ok := toFlow(f) + want := l4.Flow{ + Protocol: unix.IPPROTO_TCP, + OrigDst: netip.MustParseAddrPort("192.0.2.10:80"), + ReplySrc: netip.MustParseAddrPort("10.244.0.10:8080"), + } + if !ok || got != want { + t.Errorf("toFlow = %+v, %v, want %+v", got, ok, want) + } + + if _, ok := toFlow(&netlink.ConntrackFlow{}); ok { + t.Error("a flow without addresses must be skipped") + } +} diff --git a/pkg/proxy/dummy.go b/pkg/proxy/dummy.go index 3734c38..1c1aeea 100644 --- a/pkg/proxy/dummy.go +++ b/pkg/proxy/dummy.go @@ -13,18 +13,28 @@ func (d *DummyProxyProcessor) InitRules() error { return nil } -func (d *DummyProxyProcessor) EnsureRules(SvcIP, PodIP string) error { - fmt.Printf("EnsureRules called with SvcIP: %s, PodIP: %s\n", SvcIP, PodIP) +func (d *DummyProxyProcessor) EnsureEgressSNAT(SvcIP, PodIP string) error { + fmt.Printf("EnsureEgressSNAT called with SvcIP: %s, PodIP: %s\n", SvcIP, PodIP) return nil } -func (d *DummyProxyProcessor) DeleteRules(SvcIP, PodIP string) error { - fmt.Printf("DeleteRules called with SvcIP: %s, PodIP: %s\n", SvcIP, PodIP) +func (d *DummyProxyProcessor) DeleteEgressSNAT(SvcIP, PodIP string) error { + fmt.Printf("DeleteEgressSNAT called with SvcIP: %s, PodIP: %s\n", SvcIP, PodIP) return nil } -func (d *DummyProxyProcessor) CleanupRules(KeepMap map[string]string) error { - fmt.Println("CleanupRules called with KeepMap:", KeepMap) +func (d *DummyProxyProcessor) EnsureIngressDNAT(SvcIP, PodIP string) error { + fmt.Printf("EnsureIngressDNAT called with SvcIP: %s, PodIP: %s\n", SvcIP, PodIP) + return nil +} + +func (d *DummyProxyProcessor) DeleteIngressDNAT(SvcIP, PodIP string) error { + fmt.Printf("DeleteIngressDNAT called with SvcIP: %s, PodIP: %s\n", SvcIP, PodIP) + return nil +} + +func (d *DummyProxyProcessor) CleanupRules(keepEgress, keepIngress map[string]string) error { + fmt.Printf("CleanupRules called with keepEgress: %v, keepIngress: %v\n", keepEgress, keepIngress) return nil } diff --git a/pkg/proxy/interface.go b/pkg/proxy/interface.go index 4786120..eddd7b4 100644 --- a/pkg/proxy/interface.go +++ b/pkg/proxy/interface.go @@ -1,12 +1,46 @@ package proxy -import corev1 "k8s.io/api/core/v1" +import ( + corev1 "k8s.io/api/core/v1" + + "github.com/cozystack/cozy-proxy/pkg/l4" +) type ProxyProcessor interface { InitRules() error - EnsureRules(SvcIP, PodIP string) error - DeleteRules(SvcIP, PodIP string) error - CleanupRules(KeepMap map[string]string) error + + // EnsureEgressSNAT programs the pod_svc entry (pod IP → service IP) read + // by the egress_snat chain, so traffic leaving the backend is seen with + // the service IP as its source. + // + // Every node programs it, for every managed service, whether or not it + // hosts the backend. A reply from the backend to an intra-cluster client + // is handed straight to the client's node over the overlay: it never + // traverses the backend node's netfilter hooks, and the client's node is + // then the only place left where the source can still be rewritten. + EnsureEgressSNAT(SvcIP, PodIP string) error + + // DeleteEgressSNAT removes the pod_svc entry for the pair. No-op if absent. + DeleteEgressSNAT(SvcIP, PodIP string) error + + // EnsureIngressDNAT programs the svc_pod entry (service IP → pod IP) read + // by the ingress_dnat chain, so traffic addressed to the service IP is + // delivered to the backend. + // + // Only the node hosting the backend may program it. On any other node the + // rewrite would happen before the packet even leaves, and the hosting node + // would then record a conntrack tuple the reply can no longer match. + EnsureIngressDNAT(SvcIP, PodIP string) error + + // DeleteIngressDNAT removes the svc_pod entry for the pair. No-op if absent. + DeleteIngressDNAT(SvcIP, PodIP string) error + + // CleanupRules reconciles both maps against the desired state. Both + // arguments map service IP → pod IP: keepEgress covers every managed + // service, keepIngress only the backends hosted on this node. Anything + // else is removed, so state inherited from a build with different scoping + // is purged at startup. + CleanupRules(keepEgress, keepIngress map[string]string) error // EnsurePortFilter installs (or replaces) ingress port-filtering rules // for the given pod IP. Only TCP/UDP traffic destined to one of the @@ -46,3 +80,20 @@ type PortFilterEntry struct { PodIP string Ports []corev1.ServicePort } + +// L4Datapath programs the L4 LoadBalancer mode. It is independent of +// ProxyProcessor, which drives the VM mode, and never touches its state. +type L4Datapath interface { + // Sync replaces everything the L4 mode programs with st, atomically. + Sync(st l4.State) error + + // Teardown removes everything the L4 mode programs. No-op if absent. + Teardown() error +} + +// ConntrackPurger deletes conntrack entries. +type ConntrackPurger interface { + // Purge deletes the IPv4 entries stale selects and returns how many + // were deleted. + Purge(stale func(l4.Flow) bool) (uint, error) +} diff --git a/pkg/proxy/l4_datapath_test.go b/pkg/proxy/l4_datapath_test.go new file mode 100644 index 0000000..cb014c4 --- /dev/null +++ b/pkg/proxy/l4_datapath_test.go @@ -0,0 +1,425 @@ +//go:build linux + +package proxy + +import ( + "bufio" + "errors" + "net" + "net/netip" + "os" + "runtime" + "strings" + "testing" + "time" + + "github.com/vishvananda/netlink" + "github.com/vishvananda/netns" + v1 "k8s.io/api/core/v1" + + "github.com/cozystack/cozy-proxy/pkg/l4" +) + +// These tests send real traffic through the L4 table. Three network +// namespaces stand in for a client, the announcer and the node hosting the +// backends: +// +// client 10.0.1.2 ──── 10.0.1.1 router 10.0.2.1 ──── 10.0.2.2, 10.0.2.3 server +// (cozy_proxy_l4) +// +// The VIP 192.0.2.10 is not configured anywhere: like a MetalLB L2 VIP that +// Cilium released, it only exists through the table. + +const ( + testVIP = "192.0.2.10" + backendPort = 8080 + routerToSrv = "10.0.2.1" + clientAddr = "10.0.1.2" + backendA = "10.0.2.2" + backendB = "10.0.2.3" + dialTimeout = 500 * time.Millisecond + serverTimeout = 2 * time.Second +) + +type topology struct { + client, router, server netns.NsHandle +} + +// doIn runs f on a thread switched into ns. Sockets created by f stay in ns. +func doIn(t *testing.T, ns netns.NsHandle, f func() error) error { + t.Helper() + runtime.LockOSThread() + defer runtime.UnlockOSThread() + orig, err := netns.Get() + if err != nil { + t.Fatalf("netns.Get: %v", err) + } + defer func() { _ = orig.Close() }() + if err := netns.Set(ns); err != nil { + t.Fatalf("netns.Set: %v", err) + } + defer func() { + if err := netns.Set(orig); err != nil { + panic(err) // the thread must not go back to the pool in ns + } + }() + return f() +} + +func must(t *testing.T, what string, err error) { + t.Helper() + if err != nil { + t.Fatalf("%s: %v", what, err) + } +} + +func newTopology(t *testing.T) topology { + t.Helper() + tp := topology{client: scratchNetns(t), router: scratchNetns(t), server: scratchNetns(t)} + + h := map[netns.NsHandle]*netlink.Handle{} + for _, ns := range []netns.NsHandle{tp.client, tp.router, tp.server} { + nh, err := netlink.NewHandleAt(ns) + must(t, "netlink.NewHandleAt", err) + t.Cleanup(nh.Close) + h[ns] = nh + lo, err := nh.LinkByName("lo") + must(t, "lo", err) + must(t, "lo up", nh.LinkSetUp(lo)) + } + + // One veth per side, created in the router and moved into its peer. + link := func(peerNS netns.NsHandle, routerEnd, peerEnd, routerIP, peerIPs string) { + must(t, "veth "+routerEnd, h[tp.router].LinkAdd(&netlink.Veth{ + LinkAttrs: netlink.LinkAttrs{Name: routerEnd}, PeerName: peerEnd, + })) + peer, err := h[tp.router].LinkByName(peerEnd) + must(t, "peer "+peerEnd, err) + must(t, "move "+peerEnd, h[tp.router].LinkSetNsFd(peer, int(peerNS))) + + r, err := h[tp.router].LinkByName(routerEnd) + must(t, "router end", err) + must(t, "router addr", h[tp.router].AddrAdd(r, mustAddr(t, routerIP+"/24"))) + must(t, "router up", h[tp.router].LinkSetUp(r)) + + p, err := h[peerNS].LinkByName(peerEnd) + must(t, "peer end", err) + for _, ip := range strings.Split(peerIPs, ",") { + must(t, "peer addr", h[peerNS].AddrAdd(p, mustAddr(t, ip+"/24"))) + } + must(t, "peer up", h[peerNS].LinkSetUp(p)) + must(t, "peer default route", h[peerNS].RouteAdd(&netlink.Route{ + LinkIndex: p.Attrs().Index, Gw: net.ParseIP(routerIP), + })) + } + link(tp.client, "rc", "cr", "10.0.1.1", clientAddr) + link(tp.server, "rs", "sr", routerToSrv, backendA+","+backendB) + + must(t, "ip_forward", doIn(t, tp.router, func() error { + return os.WriteFile("/proc/sys/net/ipv4/ip_forward", []byte("1"), 0o644) + })) + return tp +} + +func mustAddr(t *testing.T, cidr string) *netlink.Addr { + t.Helper() + a, err := netlink.ParseAddr(cidr) + must(t, "ParseAddr "+cidr, err) + return a +} + +// serve answers every connection on the backends with " ", so +// the client learns which backend took it and which source it saw. +func serve(t *testing.T, ns netns.NsHandle) { + t.Helper() + var ln net.Listener + must(t, "listen", doIn(t, ns, func() error { + var err error + ln, err = net.Listen("tcp4", "0.0.0.0:8080") + return err + })) + t.Cleanup(func() { _ = ln.Close() }) + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + _, _ = c.Write([]byte(c.RemoteAddr().String() + " " + c.LocalAddr().String() + "\n")) + _ = c.Close() + } + }() +} + +type answer struct { + seenSource netip.Addr // the client address as the backend saw it + backend netip.Addr // the backend that took the connection +} + +func dial(t *testing.T, ns netns.NsHandle, addr string) (answer, error) { + t.Helper() + var conn net.Conn + err := doIn(t, ns, func() error { + var err error + conn, err = net.DialTimeout("tcp4", addr, dialTimeout) + return err + }) + if err != nil { + return answer{}, err + } + defer func() { _ = conn.Close() }() + _ = conn.SetReadDeadline(time.Now().Add(serverTimeout)) + line, err := bufio.NewReader(conn).ReadString('\n') + if err != nil { + return answer{}, err + } + f := strings.Fields(line) + if len(f) != 2 { + return answer{}, errors.New("malformed answer " + line) + } + return answer{ + seenSource: netip.MustParseAddrPort(f[0]).Addr(), + backend: netip.MustParseAddrPort(f[1]).Addr(), + }, nil +} + +// routerState is what the announcer programs for one service with two local +// backends on port 80 -> 8080. +func routerState(nodeIPs ...string) l4.State { + st := l4.State{ + VIPs: []netip.Addr{a(testVIP)}, + Ports: []l4.PortKey{pk(testVIP, v1.ProtocolTCP, 80)}, + Rules: []l4.Rule{{ + PortKey: pk(testVIP, v1.ProtocolTCP, 80), + Service: "ns/svc", + Backends: []l4.Backend{bk(backendA, backendPort), bk(backendB, backendPort)}, + }}, + } + for _, ip := range nodeIPs { + st.NodeIPs = append(st.NodeIPs, a(ip)) + } + return st +} + +func TestL4DatapathTranslatesAndBalances(t *testing.T) { + tp := newTopology(t) + serve(t, tp.server) + must(t, "Sync", datapathIn(tp.router).Sync(routerState())) + + seen := map[netip.Addr]bool{} + for i := 0; i < 2; i++ { + ans, err := dial(t, tp.client, testVIP+":80") + if err != nil { + t.Fatalf("connection %d to the VIP: %v", i, err) + } + // A client that is not a node keeps its address: this is what lets an + // Internet client, or a VM with its public IP, be seen as itself. + if ans.seenSource != a(clientAddr) { + t.Errorf("backend saw %s, want the client's own address %s", ans.seenSource, clientAddr) + } + seen[ans.backend] = true + } + if !seen[a(backendA)] || !seen[a(backendB)] { + t.Errorf("two connections must reach both backends, got %v", seen) + } +} + +// A client that is a node — a pod masqueraded by its own node — must be +// masqueraded again, so the reply comes back through the announcer. +func TestL4DatapathMasqueradesNodeSources(t *testing.T) { + tp := newTopology(t) + serve(t, tp.server) + must(t, "Sync", datapathIn(tp.router).Sync(routerState(clientAddr))) + + ans, err := dial(t, tp.client, testVIP+":80") + if err != nil { + t.Fatalf("connection to the VIP: %v", err) + } + if ans.seenSource != a(routerToSrv) { + t.Errorf("backend saw %s, want the announcer's address %s", ans.seenSource, routerToSrv) + } +} + +// Without the guard, the announcer routes an undeclared port onwards — on a +// real node back to the gateway, which loops it. Here the router has no route +// for the VIP, so an unguarded packet would come back as an ICMP error, and +// the client would fail fast instead of timing out. +func TestL4DatapathGuardDropsUndeclaredPorts(t *testing.T) { + tp := newTopology(t) + serve(t, tp.server) + dp := datapathIn(tp.router) + + _, err := dial(t, tp.client, testVIP+":81") + if err == nil || isTimeout(err) { + t.Fatalf("without the table, the router must reject the VIP outright, got %v", err) + } + + must(t, "Sync", dp.Sync(routerState())) + _, err = dial(t, tp.client, testVIP+":81") + if !isTimeout(err) { + t.Errorf("an undeclared port must be dropped silently, got %v", err) + } +} + +// An announced port with no local backend is dropped as well. +func TestL4DatapathDropsPortWithoutBackend(t *testing.T) { + tp := newTopology(t) + serve(t, tp.server) + st := routerState() + st.Rules[0].Backends = nil + must(t, "Sync", datapathIn(tp.router).Sync(st)) + + if _, err := dial(t, tp.client, testVIP+":80"); !isTimeout(err) { + t.Errorf("a port without backend must be dropped, got %v", err) + } +} + +// A TCP backend withdrawn from a frontend that remains keeps its flows, as +// behind kube-proxy, while new connections only reach the remaining backend. +// The flows go once the frontend goes — here, the node no longer announcing +// the VIP. +func TestL4DatapathPurgesTCPFlowsWithTheirFrontend(t *testing.T) { + tp := newTopology(t) + serve(t, tp.server) + dp := datapathIn(tp.router) + ct := &NetlinkConntrack{Handle: conntrackIn(t, tp.router)} + + prev := routerState() + must(t, "Sync", dp.Sync(prev)) + for i := 0; i < 2; i++ { + if _, err := dial(t, tp.client, testVIP+":80"); err != nil { + t.Fatalf("connection %d: %v", i, err) + } + } + + withdrawn := netip.AddrPortFrom(a(backendB), backendPort) + flowsTo := func(b netip.AddrPort) int { + n := 0 + for _, f := range listFlows(t, ct.Handle) { + if f.ReplySrc == b { + n++ + } + } + return n + } + if flowsTo(withdrawn) == 0 { + t.Fatal("no flow towards the backend about to be withdrawn: the test would prove nothing") + } + + cur := routerState() + cur.Rules[0].Backends = []l4.Backend{bk(backendA, backendPort)} + must(t, "Sync", dp.Sync(cur)) + if n, err := ct.Purge(l4.StaleFlows(&prev, cur)); err != nil || n != 0 { + t.Fatalf("Purge after a TCP backend withdrawal = %d, %v; want nothing purged", n, err) + } + if flowsTo(withdrawn) == 0 { + t.Error("the withdrawn TCP backend lost its flows; they must end on their own") + } + for i := 0; i < 2; i++ { + ans, err := dial(t, tp.client, testVIP+":80") + if err != nil { + t.Fatalf("connection after the withdrawal: %v", err) + } + if ans.backend != a(backendA) { + t.Errorf("connection reached %s, want only %s", ans.backend, backendA) + } + } + + unannounced := routerState() + unannounced.Rules = nil + must(t, "Sync", dp.Sync(unannounced)) + n, err := ct.Purge(l4.StaleFlows(&cur, unannounced)) + if err != nil { + t.Fatalf("Purge: %v", err) + } + if n == 0 { + t.Error("losing the announcement must purge the translated flows") + } + if left := flowsTo(withdrawn) + flowsTo(netip.AddrPortFrom(a(backendA), backendPort)); left != 0 { + t.Errorf("%d translated flows survived the end of the announcement", left) + } +} + +func isTimeout(err error) bool { + var ne net.Error + return errors.As(err, &ne) && ne.Timeout() +} + +// echo serves a line-echo on port 9090 of the backends, keeping each +// connection open until the client closes it. +func echo(t *testing.T, ns netns.NsHandle) { + t.Helper() + var ln net.Listener + must(t, "listen", doIn(t, ns, func() error { + var err error + ln, err = net.Listen("tcp4", "0.0.0.0:9090") + return err + })) + t.Cleanup(func() { _ = ln.Close() }) + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + go func() { + defer func() { _ = c.Close() }() + r := bufio.NewReader(c) + for { + line, err := r.ReadString('\n') + if err != nil { + return + } + if _, err := c.Write([]byte(line)); err != nil { + return + } + } + }() + } + }() +} + +// Every sync rebuilds the table, and a restarted instance does the same on its +// first pass. A connection established before must keep flowing: its +// translation lives in conntrack, not in the rules. +func TestL4DatapathResyncKeepsEstablishedConnections(t *testing.T) { + tp := newTopology(t) + echo(t, tp.server) + dp := datapathIn(tp.router) + st := l4.State{ + VIPs: []netip.Addr{a(testVIP)}, + Ports: []l4.PortKey{pk(testVIP, v1.ProtocolTCP, 90)}, + Rules: []l4.Rule{{ + PortKey: pk(testVIP, v1.ProtocolTCP, 90), + Service: "ns/echo", + Backends: []l4.Backend{bk(backendA, 9090)}, + }}, + } + must(t, "Sync", dp.Sync(st)) + + var conn net.Conn + must(t, "dial", doIn(t, tp.client, func() error { + var err error + conn, err = net.DialTimeout("tcp4", testVIP+":90", dialTimeout) + return err + })) + defer func() { _ = conn.Close() }() + r := bufio.NewReader(conn) + roundTrip := func(msg string) { + t.Helper() + _ = conn.SetDeadline(time.Now().Add(serverTimeout)) + if _, err := conn.Write([]byte(msg + "\n")); err != nil { + t.Fatalf("write %q: %v", msg, err) + } + got, err := r.ReadString('\n') + if err != nil || got != msg+"\n" { + t.Fatalf("echo of %q = %q, %v", msg, got, err) + } + } + + roundTrip("before") + for i := 0; i < 3; i++ { + must(t, "re-Sync", dp.Sync(st)) + } + roundTrip("after") +} diff --git a/pkg/proxy/l4_nft.go b/pkg/proxy/l4_nft.go new file mode 100644 index 0000000..9260522 --- /dev/null +++ b/pkg/proxy/l4_nft.go @@ -0,0 +1,388 @@ +package proxy + +import ( + "fmt" + "net" + "net/netip" + "sync" + + "github.com/google/nftables" + "github.com/google/nftables/binaryutil" + "github.com/google/nftables/expr" + "github.com/google/nftables/userdata" + "golang.org/x/sys/unix" + v1 "k8s.io/api/core/v1" + + "github.com/cozystack/cozy-proxy/pkg/l4" +) + +// L4TableName is the table of the L4 LoadBalancer mode. It is separate from +// the VM mode's "cozy_proxy" table, which it never touches, so deleting it +// removes the L4 mode entirely. +const L4TableName = "cozy_proxy_l4" + +// Chain priorities of the L4 mode. See docs/rfc/l4-loadbalancer-mode.md for how +// they interleave with the VM mode's chains. +var ( + // After conntrack (-200) and the VM mode's ingress_dnat (-150), before + // the DNAT below. + l4GuardPriority = nftables.ChainPriorityRef(*nftables.ChainPriorityMangle + 10) + // Just before the iptables / kube-ovn nat hooks (-100), which then see the + // translated packet. + l4DNATPriority = nftables.ChainPriorityRef(*nftables.ChainPriorityNATDest - 5) + l4MasqPriority = nftables.ChainPriorityRef(*nftables.ChainPriorityNATSource - 5) +) + +// ipsDstNAT is IPS_DST_NAT from linux/netfilter/nf_conntrack_common.h: the +// conntrack status bit set once a flow has been destination-NATed. +const ipsDstNAT = 1 << 5 + +// NFTL4Datapath programs the L4 mode's table with nftables. +// +// Every Sync rebuilds the table as a whole in a single transaction: the table +// is small, nftables applies a batch atomically — a packet sees the old or the +// new ruleset, never a mix — and conntrack entries are not tied to rules. There +// is no incremental diff to get wrong and no stale element to collect, and a +// retry is just the next Sync. +type NFTL4Datapath struct { + mu sync.Mutex + + // ConnOptions are passed to every netlink connection. Tests use them to + // work in a scratch network namespace. + ConnOptions []nftables.ConnOption +} + +// Sync replaces the L4 table with the given state. +func (d *NFTL4Datapath) Sync(st l4.State) error { + d.mu.Lock() + defer d.mu.Unlock() + + conn, err := nftables.New(d.ConnOptions...) + if err != nil { + return fmt.Errorf("could not create nftables connection: %w", err) + } + + t := &nftables.Table{Family: nftables.TableFamilyIPv4, Name: L4TableName} + // Adding the table first makes the deletion valid when it does not exist + // yet, all within the one transaction. + conn.AddTable(t) + conn.DelTable(t) + t = conn.AddTable(t) + + if err := buildL4Table(conn, t, st); err != nil { + return err + } + if err := conn.Flush(); err != nil { + return fmt.Errorf("failed to commit table %s: %w", L4TableName, err) + } + return nil +} + +// Teardown removes the L4 table, if any. It is how the mode is switched off. +func (d *NFTL4Datapath) Teardown() error { + d.mu.Lock() + defer d.mu.Unlock() + + conn, err := nftables.New(d.ConnOptions...) + if err != nil { + return fmt.Errorf("could not create nftables connection: %w", err) + } + + t := &nftables.Table{Family: nftables.TableFamilyIPv4, Name: L4TableName} + conn.AddTable(t) + conn.DelTable(t) + if err := conn.Flush(); err != nil { + return fmt.Errorf("failed to delete table %s: %w", L4TableName, err) + } + return nil +} + +// buildL4Table queues the sets, maps, chains and rules for st into t. +// +// set vips { type ipv4_addr } all VIPs, every node +// set vip_ports { type ipv4_addr . inet_proto . inet_service } declared ports, every node +// set node_ips { type ipv4_addr } masqueraded sources +// set backends { type ipv4_addr . inet_proto . inet_service } local targets, announcer +// map backends--- { type integer : ipv4_addr . inet_service } +// +// chain guard filter prerouting mangle+10 +// ct state established,related accept +// ip daddr @vips ip daddr . meta l4proto . th dport != @vip_ports drop +// chain translate nat prerouting dstnat-5 (rules on the announcer only) +// ip daddr V tcp dport P dnat ip addr . port to numgen inc mod N map @backends-... +// ip daddr V tcp dport P drop (announced port with no local backend) +// chain masq nat postrouting srcnat-5 +// ct status dnat ip saddr @node_ips ip daddr . meta l4proto . th dport @backends masquerade fully-random +func buildL4Table(conn *nftables.Conn, t *nftables.Table, st l4.State) error { + var ids setIDs + vips := &nftables.Set{Table: t, ID: ids.next(), Name: "vips", KeyType: nftables.TypeIPAddr} + if err := conn.AddSet(vips, addrElements(st.VIPs)); err != nil { + return fmt.Errorf("could not add set %s: %w", vips.Name, err) + } + + portKeyType, err := nftables.ConcatSetType(nftables.TypeIPAddr, nftables.TypeInetProto, nftables.TypeInetService) + if err != nil { + return fmt.Errorf("could not build vip_ports key type: %w", err) + } + vipPorts := &nftables.Set{Table: t, ID: ids.next(), Name: "vip_ports", KeyType: portKeyType, Concatenation: true} + var portElems []nftables.SetElement + for _, pk := range st.Ports { + portElems = append(portElems, nftables.SetElement{ + Key: concatPortKey(net.IP(pk.VIP.AsSlice()), protoByte(pk.Protocol), pk.Port), + }) + } + if err := conn.AddSet(vipPorts, portElems); err != nil { + return fmt.Errorf("could not add set %s: %w", vipPorts.Name, err) + } + + nodeIPs := &nftables.Set{Table: t, ID: ids.next(), Name: "node_ips", KeyType: nftables.TypeIPAddr} + if err := conn.AddSet(nodeIPs, addrElements(st.NodeIPs)); err != nil { + return fmt.Errorf("could not add set %s: %w", nodeIPs.Name, err) + } + + // The translation targets, for the masquerade below to recognize the + // flows this table translated. Matching the flow's original destination + // instead ("ct original ip daddr @vips") is what nft itself would write, + // but github.com/google/nftables v0.3.0 can only emit it with the generic + // conntrack key, which nft then fails to decode: "nft list" aborts on the + // whole ruleset, which is too high a price for every operator on the node. + backends := &nftables.Set{Table: t, ID: ids.next(), Name: "backends", KeyType: portKeyType, Concatenation: true} + var backendElems []nftables.SetElement + seen := map[l4.Backend]bool{} + for _, r := range st.Rules { + for _, b := range r.Backends { + if seen[b] { + continue + } + seen[b] = true + backendElems = append(backendElems, nftables.SetElement{ + Key: concatPortKey(net.IP(b.IP.AsSlice()), protoByte(r.Protocol), b.Port), + }) + } + } + if err := conn.AddSet(backends, backendElems); err != nil { + return fmt.Errorf("could not add set %s: %w", backends.Name, err) + } + + guard := conn.AddChain(&nftables.Chain{ + Name: "guard", + Table: t, + Type: nftables.ChainTypeFilter, + Hooknum: nftables.ChainHookPrerouting, + Priority: l4GuardPriority, + }) + // ICMP errors about a translated flow (PMTU) are "related": they must reach + // the NAT below, which translates them along with their flow. + conn.AddRule(&nftables.Rule{Table: t, Chain: guard, Exprs: ctStateEstablishedRelatedAccept()}) + // Anything else addressed to a VIP and not declared — another port, ICMP + // echo — is dropped. Left alone it would not be translated, and since the + // VIP is not a local address the node would route it back to its gateway, + // which sends it straight back: a loop until the TTL expires. + conn.AddRule(&nftables.Rule{Table: t, Chain: guard, Exprs: []expr.Any{ + &expr.Payload{DestRegister: 1, Base: expr.PayloadBaseNetworkHeader, Offset: 16, Len: 4}, + &expr.Lookup{SourceRegister: 1, SetName: vips.Name, SetID: vips.ID}, + &expr.Payload{DestRegister: unix.NFT_REG32_00, Base: expr.PayloadBaseNetworkHeader, Offset: 16, Len: 4}, + &expr.Meta{Key: expr.MetaKeyL4PROTO, Register: unix.NFT_REG32_01}, + &expr.Payload{DestRegister: unix.NFT_REG32_02, Base: expr.PayloadBaseTransportHeader, Offset: 2, Len: 2}, + &expr.Lookup{SourceRegister: unix.NFT_REG32_00, SetName: vipPorts.Name, SetID: vipPorts.ID, Invert: true}, + &expr.Verdict{Kind: expr.VerdictDrop}, + }}) + + // Not "dnat": nft takes that for its keyword, and an operator could not + // name the chain on the command line without quoting it. + translate := conn.AddChain(&nftables.Chain{ + Name: "translate", + Table: t, + Type: nftables.ChainTypeNAT, + Hooknum: nftables.ChainHookPrerouting, + Priority: l4DNATPriority, + }) + for _, r := range st.Rules { + exprs := matchFrontend(r.PortKey) + if len(r.Backends) == 0 { + // Announced here with no local ready backend: drop rather than let + // the packet loop through the gateway. + exprs = append(exprs, &expr.Verdict{Kind: expr.VerdictDrop}) + } else { + m, err := addBackendMap(conn, t, r, ids.next()) + if err != nil { + return err + } + exprs = append(exprs, + // Round-robin slot, then slot -> (address . port) into + // NFT_REG32_00 and NFT_REG32_01, which the NAT reads. + &expr.Numgen{Register: 1, Modulus: uint32(len(r.Backends)), Type: unix.NFT_NG_INCREMENTAL}, + &expr.Lookup{SourceRegister: 1, DestRegister: 1, IsDestRegSet: true, SetName: m.Name, SetID: m.ID}, + &expr.NAT{ + Type: expr.NATTypeDestNAT, + Family: unix.NFPROTO_IPV4, + RegAddrMin: 1, + RegProtoMin: unix.NFT_REG32_01, + Specified: true, + }, + ) + } + conn.AddRule(&nftables.Rule{ + Table: t, + Chain: translate, + Exprs: exprs, + UserData: userdata.AppendString(nil, userdata.TypeComment, r.Service), + }) + } + + masq := conn.AddChain(&nftables.Chain{ + Name: "masq", + Table: t, + Type: nftables.ChainTypeNAT, + Hooknum: nftables.ChainHookPostrouting, + Priority: l4MasqPriority, + }) + // A pod on another node reaches the VIP masqueraded by its own node. Left + // as is, the backend would answer that node IP directly and the overlay + // would carry the reply to the client's node, bypassing this node's + // conntrack: the client then resets the connection. Masquerading the node + // sources pins the reply here. Every other source — the Internet, a VM's + // public IP — is kept. + // + // "ct status dnat" restricts it to translated flows, so a node process + // talking to a backend pod directly (a kubelet probe) keeps its source. + // + // The source port is drawn fully at random. By default the kernel keeps + // the client's port whenever its own conntrack has the tuple free, but it + // cannot see OVS's: every client of a node collapses onto one masqueraded + // address, and under load the announcer reused a tuple OVS still tracked, + // 29 to 50 s after the previous connection. OVN dropped the backend's + // SYN-ACK and the client retransmitted a second later. + conn.AddRule(&nftables.Rule{Table: t, Chain: masq, Exprs: []expr.Any{ + &expr.Ct{Register: 1, Key: expr.CtKeySTATUS}, + &expr.Bitwise{ + SourceRegister: 1, + DestRegister: 1, + Len: 4, + Mask: binaryutil.NativeEndian.PutUint32(ipsDstNAT), + Xor: binaryutil.NativeEndian.PutUint32(0), + }, + &expr.Cmp{Op: expr.CmpOpNeq, Register: 1, Data: []byte{0, 0, 0, 0}}, + &expr.Payload{DestRegister: 1, Base: expr.PayloadBaseNetworkHeader, Offset: 12, Len: 4}, + &expr.Lookup{SourceRegister: 1, SetName: nodeIPs.Name, SetID: nodeIPs.ID}, + &expr.Payload{DestRegister: unix.NFT_REG32_00, Base: expr.PayloadBaseNetworkHeader, Offset: 16, Len: 4}, + &expr.Meta{Key: expr.MetaKeyL4PROTO, Register: unix.NFT_REG32_01}, + &expr.Payload{DestRegister: unix.NFT_REG32_02, Base: expr.PayloadBaseTransportHeader, Offset: 2, Len: 2}, + &expr.Lookup{SourceRegister: unix.NFT_REG32_00, SetName: backends.Name, SetID: backends.ID}, + &expr.Masq{FullyRandom: true}, + }}) + return nil +} + +// setIDs hands out the set IDs of one transaction, which the rules use to +// reference sets created in the same batch. github.com/google/nftables would +// draw them from a package-level counter guarded only by each connection's own +// lock, so the VM mode queueing its sets on its connection at the same moment +// could give two of ours the same ID, and a rule would then point at the wrong +// set. The kernel only needs them unique within a batch. +type setIDs uint32 + +func (n *setIDs) next() uint32 { + *n++ + return uint32(*n) +} + +// addBackendMap adds the map from round-robin slot to backend for one frontend. +// It is named after the frontend, which is unique, and commented with the +// service it belongs to. +func addBackendMap(conn *nftables.Conn, t *nftables.Table, r l4.Rule, id uint32) (*nftables.Set, error) { + dataType, err := nftables.ConcatSetType(nftables.TypeIPAddr, nftables.TypeInetService) + if err != nil { + return nil, fmt.Errorf("could not build backend map data type: %w", err) + } + m := &nftables.Set{ + Table: t, + ID: id, + Name: fmt.Sprintf("backends-%s-%s-%d", r.VIP, protoName(r.Protocol), r.Port), + Comment: r.Service, + IsMap: true, + KeyType: nftables.TypeInteger, + DataType: dataType, + // numgen writes the slot in host byte order. nft lists the key type as + // "type 0": it only names an integer key through a "typeof" annotation, + // which github.com/google/nftables cannot write. The elements are listed + // correctly, and the kernel does not look at either. + KeyByteOrder: binaryutil.NativeEndian, + } + elems := make([]nftables.SetElement, 0, len(r.Backends)) + for i, b := range r.Backends { + val := make([]byte, 8) // address, then the port padded to 4 bytes + ip := b.IP.As4() + copy(val[0:4], ip[:]) + val[4] = byte(b.Port >> 8) + val[5] = byte(b.Port) + elems = append(elems, nftables.SetElement{Key: binaryutil.NativeEndian.PutUint32(uint32(i)), Val: val}) + } + if err := conn.AddSet(m, elems); err != nil { + return nil, fmt.Errorf("could not add map %s: %w", m.Name, err) + } + return m, nil +} + +// matchFrontend matches "ip daddr VIP dport PORT". +func matchFrontend(pk l4.PortKey) []expr.Any { + vip := pk.VIP.As4() + return []expr.Any{ + &expr.Payload{DestRegister: 1, Base: expr.PayloadBaseNetworkHeader, Offset: 16, Len: 4}, + &expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: vip[:]}, + &expr.Meta{Key: expr.MetaKeyL4PROTO, Register: 1}, + &expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{protoByte(pk.Protocol)}}, + &expr.Payload{DestRegister: 1, Base: expr.PayloadBaseTransportHeader, Offset: 2, Len: 2}, + &expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: binaryutil.BigEndian.PutUint16(pk.Port)}, + } +} + +// ctStateEstablishedRelatedAccept is "ct state established,related accept". +func ctStateEstablishedRelatedAccept() []expr.Any { + return []expr.Any{ + &expr.Ct{Register: 1, SourceRegister: false, Key: expr.CtKeySTATE}, + &expr.Bitwise{ + SourceRegister: 1, + DestRegister: 1, + Len: 4, + Mask: binaryutil.NativeEndian.PutUint32(expr.CtStateBitESTABLISHED | expr.CtStateBitRELATED), + Xor: binaryutil.NativeEndian.PutUint32(0), + }, + &expr.Cmp{Op: expr.CmpOpNeq, Register: 1, Data: []byte{0, 0, 0, 0}}, + &expr.Verdict{Kind: expr.VerdictAccept}, + } +} + +func addrElements(addrs []netip.Addr) []nftables.SetElement { + elems := make([]nftables.SetElement, 0, len(addrs)) + for _, a := range addrs { + ip := a.As4() + elems = append(elems, nftables.SetElement{Key: ip[:]}) + } + return elems +} + +func protoByte(p v1.Protocol) byte { + switch p { + case v1.ProtocolUDP: + return unix.IPPROTO_UDP + case v1.ProtocolSCTP: + return unix.IPPROTO_SCTP + default: + return unix.IPPROTO_TCP + } +} + +func protoName(p v1.Protocol) string { + switch p { + case v1.ProtocolUDP: + return "udp" + case v1.ProtocolSCTP: + return "sctp" + default: + return "tcp" + } +} + +// Compile-time assertion that NFTL4Datapath satisfies L4Datapath. +var _ L4Datapath = (*NFTL4Datapath)(nil) diff --git a/pkg/proxy/l4_nft_test.go b/pkg/proxy/l4_nft_test.go new file mode 100644 index 0000000..59ad557 --- /dev/null +++ b/pkg/proxy/l4_nft_test.go @@ -0,0 +1,297 @@ +//go:build linux + +package proxy + +import ( + "flag" + "net/netip" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/google/nftables" + "github.com/google/nftables/expr" + "github.com/vishvananda/netns" + v1 "k8s.io/api/core/v1" + + "github.com/cozystack/cozy-proxy/pkg/l4" +) + +// These tests program a real kernel, in a scratch network namespace, and +// compare what `nft list` prints with golden files under testdata/l4. They need +// root and skip otherwise; the golden comparison also needs the nft binary. +// +// Regenerate the golden files with: +// +// go test ./pkg/proxy -run TestL4 -update +var updateGolden = flag.Bool("update", false, "rewrite the golden files under testdata") + +// scratchNetns creates a network namespace that lives as long as the test, so +// nothing here ever touches the host's ruleset. +func scratchNetns(t *testing.T) netns.NsHandle { + t.Helper() + if os.Geteuid() != 0 { + t.Skip("needs root to create a network namespace") + } + + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + orig, err := netns.Get() + if err != nil { + t.Fatalf("netns.Get: %v", err) + } + defer func() { _ = orig.Close() }() + ns, err := netns.New() // also switches this thread into it + if err != nil { + t.Skipf("cannot create a network namespace: %v", err) + } + if err := netns.Set(orig); err != nil { + t.Fatalf("netns.Set: %v", err) + } + t.Cleanup(func() { _ = ns.Close() }) + return ns +} + +func datapathIn(ns netns.NsHandle) *NFTL4Datapath { + return &NFTL4Datapath{ConnOptions: []nftables.ConnOption{nftables.WithNetNSFd(int(ns))}} +} + +// inNetns runs the command inside ns. The child inherits the namespace of the +// thread that forks it, which is locked into ns for the duration. +func inNetns(t *testing.T, ns netns.NsHandle, name string, args ...string) (string, error) { + t.Helper() + runtime.LockOSThread() + defer runtime.UnlockOSThread() + orig, err := netns.Get() + if err != nil { + t.Fatalf("netns.Get: %v", err) + } + defer func() { _ = orig.Close() }() + if err := netns.Set(ns); err != nil { + t.Fatalf("netns.Set: %v", err) + } + defer func() { + if err := netns.Set(orig); err != nil { + panic(err) // the thread must not go back to the pool in ns + } + }() + out, err := exec.Command(name, args...).CombinedOutput() + return string(out), err +} + +func nftBinary(t *testing.T) string { + t.Helper() + path, err := exec.LookPath("nft") + if err != nil { + t.Skip("nft binary not available") + } + return path +} + +func listL4Table(t *testing.T, ns netns.NsHandle) string { + t.Helper() + out, err := inNetns(t, ns, nftBinary(t), "list", "table", "ip", L4TableName) + if err != nil { + t.Fatalf("nft list table: %v\n%s", err, out) + } + return out +} + +func checkGolden(t *testing.T, name, got string) { + t.Helper() + path := filepath.Join("testdata", "l4", name+".nft") + if *updateGolden { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(got), 0o644); err != nil { + t.Fatal(err) + } + return + } + want, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading golden file (run with -update to create it): %v", err) + } + if got != string(want) { + t.Errorf("%s differs from the golden file\n--- got\n%s\n--- want\n%s", name, got, want) + } +} + +func a(s string) netip.Addr { return netip.MustParseAddr(s) } + +func pk(vip string, proto v1.Protocol, port uint16) l4.PortKey { + return l4.PortKey{VIP: a(vip), Protocol: proto, Port: port} +} + +func bk(ip string, port uint16) l4.Backend { return l4.Backend{IP: a(ip), Port: port} } + +// announcerState is what the announcer of two services programs: one with two +// ports and two local backends, one announced here with no local backend. +var announcerState = l4.State{ + VIPs: []netip.Addr{a("192.0.2.10"), a("192.0.2.20"), a("192.0.2.30")}, + Ports: []l4.PortKey{ + pk("192.0.2.10", v1.ProtocolTCP, 80), + pk("192.0.2.10", v1.ProtocolTCP, 443), + pk("192.0.2.20", v1.ProtocolTCP, 5432), + }, + NodeIPs: []netip.Addr{a("10.200.24.11"), a("10.200.24.12"), a("10.200.24.13")}, + Rules: []l4.Rule{ + {PortKey: pk("192.0.2.10", v1.ProtocolTCP, 80), Service: "tenant-a/ingress", + Backends: []l4.Backend{bk("10.244.0.10", 8080), bk("10.244.0.11", 8080)}}, + {PortKey: pk("192.0.2.10", v1.ProtocolTCP, 443), Service: "tenant-a/ingress", + Backends: []l4.Backend{bk("10.244.0.10", 8443), bk("10.244.0.11", 8443)}}, + {PortKey: pk("192.0.2.20", v1.ProtocolTCP, 5432), Service: "tenant-b/postgres"}, + }, +} + +// guardOnlyState is the same cluster seen from a node that announces nothing: +// it guards every VIP and translates none. +var guardOnlyState = l4.State{ + VIPs: announcerState.VIPs, + Ports: announcerState.Ports, + NodeIPs: announcerState.NodeIPs, +} + +func TestL4SyncGolden(t *testing.T) { + cases := []struct { + name string + state l4.State + }{ + {"announcer", announcerState}, + {"guard-only", guardOnlyState}, + {"empty", l4.State{}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ns := scratchNetns(t) + if err := datapathIn(ns).Sync(c.state); err != nil { + t.Fatalf("Sync: %v", err) + } + checkGolden(t, c.name, listL4Table(t, ns)) + }) + } +} + +// A sync replaces the table as a whole: nothing from the previous state may +// survive, a backend map in particular. +func TestL4SyncReplacesPreviousState(t *testing.T) { + ns := scratchNetns(t) + dp := datapathIn(ns) + if err := dp.Sync(announcerState); err != nil { + t.Fatalf("first Sync: %v", err) + } + if err := dp.Sync(guardOnlyState); err != nil { + t.Fatalf("second Sync: %v", err) + } + got := listL4Table(t, ns) + if strings.Contains(got, "backends-") || strings.Contains(got, "dnat") && strings.Contains(got, "numgen") { + t.Errorf("state of the first sync survived the second:\n%s", got) + } + checkGolden(t, "guard-only", got) +} + +// Teardown removes the table, is idempotent, and never touches another table +// — the VM mode's in particular. +func TestL4TeardownLeavesOtherTablesAlone(t *testing.T) { + ns := scratchNetns(t) + dp := datapathIn(ns) + + conn, err := nftables.New(nftables.WithNetNSFd(int(ns))) + if err != nil { + t.Fatalf("nftables.New: %v", err) + } + conn.AddTable(&nftables.Table{Family: nftables.TableFamilyIPv4, Name: "cozy_proxy"}) + if err := conn.Flush(); err != nil { + t.Fatalf("creating the VM mode's table: %v", err) + } + + if err := dp.Teardown(); err != nil { + t.Fatalf("Teardown without a table: %v", err) + } + if err := dp.Sync(announcerState); err != nil { + t.Fatalf("Sync: %v", err) + } + if err := dp.Teardown(); err != nil { + t.Fatalf("Teardown: %v", err) + } + + tables, err := conn.ListTablesOfFamily(nftables.TableFamilyIPv4) + if err != nil { + t.Fatalf("ListTables: %v", err) + } + var names []string + for _, tb := range tables { + names = append(names, tb.Name) + } + if len(names) != 1 || names[0] != "cozy_proxy" { + t.Errorf("tables after Teardown = %v, want only cozy_proxy", names) + } +} + +// Operators inspect one chain at a time. A chain named after an nft keyword +// ("dnat") cannot be named on the command line without quoting, which is how +// this was found on the lab. +func TestL4ChainsCanBeListedByName(t *testing.T) { + ns := scratchNetns(t) + if err := datapathIn(ns).Sync(announcerState); err != nil { + t.Fatalf("Sync: %v", err) + } + conn, err := nftables.New(nftables.WithNetNSFd(int(ns))) + if err != nil { + t.Fatalf("nftables.New: %v", err) + } + chains, err := conn.ListChainsOfTableFamily(nftables.TableFamilyIPv4) + if err != nil { + t.Fatalf("ListChains: %v", err) + } + nft := nftBinary(t) + for _, ch := range chains { + if ch.Table.Name != L4TableName { + continue + } + if out, err := inNetns(t, ns, nft, "list", "chain", "ip", L4TableName, ch.Name); err != nil { + t.Errorf("nft list chain %s: %v\n%s", ch.Name, err, out) + } + } +} + +// The masquerade of node sources must pick a fully random source port. By +// default the kernel keeps the client's port whenever its own conntrack has +// the tuple free, and on the lab that made the announcer reuse a +// 100.64.0.x: tuple 29 to 50 s after a previous connection, while OVS +// still tracked it: the backend's SYN-ACK was dropped in OVN and the client +// retransmitted, 0.06 to 0.1 % of connections taking over a second. +func TestL4MasqueradeIsFullyRandom(t *testing.T) { + ns := scratchNetns(t) + if err := datapathIn(ns).Sync(announcerState); err != nil { + t.Fatalf("Sync: %v", err) + } + conn, err := nftables.New(nftables.WithNetNSFd(int(ns))) + if err != nil { + t.Fatalf("nftables.New: %v", err) + } + table := &nftables.Table{Family: nftables.TableFamilyIPv4, Name: L4TableName} + rules, err := conn.GetRules(table, &nftables.Chain{Name: "masq", Table: table}) + if err != nil { + t.Fatalf("GetRules: %v", err) + } + var masqs []*expr.Masq + for _, r := range rules { + for _, e := range r.Exprs { + if m, ok := e.(*expr.Masq); ok { + masqs = append(masqs, m) + } + } + } + if len(masqs) != 1 { + t.Fatalf("want one masquerade in chain masq, got %d", len(masqs)) + } + if !masqs[0].FullyRandom { + t.Errorf("the masquerade keeps the client's source port; want fully-random, got %+v", *masqs[0]) + } +} diff --git a/pkg/proxy/nft.go b/pkg/proxy/nft.go index afe72ac..e66e2f4 100644 --- a/pkg/proxy/nft.go +++ b/pkg/proxy/nft.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net" + "sync" "github.com/google/nftables" "github.com/google/nftables/binaryutil" @@ -18,6 +19,18 @@ var log = ctrl.Log.WithName("nft-proxy-processor") // NFTProxyProcessor implements a NATProcessor using nftables. type NFTProxyProcessor struct { + // mu serializes every queue-then-flush sequence below. + // + // nftables.Conn guards its own message list, but not the interval between + // queueing messages and committing them. Two informer goroutines — plus + // the startup reconciliation — share this connection, so without this + // lock one flush carries the other caller's half-queued messages. A batch + // is a single transaction: one stale deletion in it aborts the other + // caller's addition, which is then never retried because the controller + // only reacts to events. The service stays unprogrammed until the next + // one, which is how a public IP can go dark for minutes after a restart. + mu sync.Mutex + conn *nftables.Conn // Table "cozy_proxy" will contain all objects. @@ -37,6 +50,9 @@ type NFTProxyProcessor struct { // InitRules initializes the nftables configuration in a single table "cozy_proxy". // It flushes the entire ruleset, then re-creates the table with the desired sets, maps, and chains. func (p *NFTProxyProcessor) InitRules() error { + p.mu.Lock() + defer p.mu.Unlock() + log.Info("Initializing nftables NAT configuration") // Create a new connection if needed. @@ -387,244 +403,370 @@ func (p *NFTProxyProcessor) InitRules() error { return nil } -// EnsureRules ensures that a one-to-one mapping exists between svcIP and podIP. -// If a mapping already exists for svcIP with a different podIP, -// the old mapping is removed (from svc_pod, pod_svc, and from the raw pod set) -// before the new mapping is added. -func (p *NFTProxyProcessor) EnsureRules(svcIP, podIP string) error { - log.Info("Ensuring NAT mapping", "svcIP", svcIP, "podIP", podIP) +// EnsureEgressSNAT adds the pod_svc entry (podIP → svcIP) consulted by the +// egress_snat chain. If podIP is already mapped to another service, that stale +// entry is dropped first: a pod IP can only stand for one service IP. +// +// Programmed on every node, see ProxyProcessor.EnsureEgressSNAT. +func (p *NFTProxyProcessor) EnsureEgressSNAT(svcIP, podIP string) error { + p.mu.Lock() + defer p.mu.Unlock() - parsedSvcIP := net.ParseIP(svcIP).To4() - if parsedSvcIP == nil { - return fmt.Errorf("invalid svcIP: %s", svcIP) - } - parsedPodIP := net.ParseIP(podIP).To4() - if parsedPodIP == nil { - return fmt.Errorf("invalid podIP: %s", podIP) - } + log.Info("Ensuring egress SNAT", "svcIP", svcIP, "podIP", podIP) - // --- Remove conflicting mapping for svcIP in svc_pod map --- - // If svcIP already maps to a different pod, remove that mapping and - // delete the old pod from the raw pod set. - svcPodElems, err := p.conn.GetSetElements(p.svcPodMap) + parsedSvcIP, parsedPodIP, err := parsePair(svcIP, podIP) if err != nil { - log.Error(err, "Failed to get svc_pod map elements") - return fmt.Errorf("failed to get svc_pod map elements: %v", err) - } - for _, el := range svcPodElems { - if bytes.Equal(el.Key, parsedSvcIP) { - // Found an existing mapping for svcIP. - if !bytes.Equal(el.Val, parsedPodIP) { - oldPodIP := el.Val - log.Info("Updating mapping for svc", "svcIP", svcIP, "oldPodIP", net.IP(oldPodIP).String(), "newPodIP", podIP) - // Remove the old mapping from svc_pod. - if err := p.conn.SetDeleteElements(p.svcPodMap, []nftables.SetElement{{Key: parsedSvcIP, Val: oldPodIP}}); err != nil { - log.Error(err, "Failed to delete old svc_pod mapping", "svcIP", svcIP, "oldPodIP", net.IP(oldPodIP).String()) - return fmt.Errorf("failed to delete old svc_pod mapping: %v", err) - } - // Remove the corresponding mapping from pod_svc. - if err := p.conn.SetDeleteElements(p.podSvcMap, []nftables.SetElement{{Key: oldPodIP, Val: parsedSvcIP}}); err != nil { - log.Error(err, "Failed to delete corresponding pod_svc mapping", "oldPodIP", net.IP(oldPodIP).String(), "svcIP", svcIP) - return fmt.Errorf("failed to delete corresponding pod_svc mapping: %v", err) - } - } - break // svcIP mapping handled; exit loop. - } + return err } - // --- Remove conflicting mapping for podIP in pod_svc map --- - // If podIP already maps to a different svc, remove that mapping and delete the podIP - // from the raw pod set (since the old mapping is no longer desired). podSvcElems, err := p.conn.GetSetElements(p.podSvcMap) if err != nil { log.Error(err, "Failed to get pod_svc map elements") return fmt.Errorf("failed to get pod_svc map elements: %v", err) } for _, el := range podSvcElems { - if bytes.Equal(el.Key, parsedPodIP) { - // Found an existing mapping for podIP. - if !bytes.Equal(el.Val, parsedSvcIP) { - log.Info("Updating mapping for pod", "podIP", podIP, "oldSvcIP", net.IP(el.Val).String(), "newSvcIP", svcIP) - // Remove the old mapping from pod_svc. - if err := p.conn.SetDeleteElements(p.podSvcMap, []nftables.SetElement{{Key: parsedPodIP, Val: el.Val}}); err != nil { - log.Error(err, "Failed to delete old pod_svc mapping", "podIP", podIP, "oldSvcIP", net.IP(el.Val).String()) - return fmt.Errorf("failed to delete old pod_svc mapping: %v", err) - } - // Remove the corresponding mapping from svc_pod. - if err := p.conn.SetDeleteElements(p.svcPodMap, []nftables.SetElement{{Key: el.Val, Val: parsedPodIP}}); err != nil { - log.Error(err, "Failed to delete corresponding svc_pod mapping", "oldSvcIP", net.IP(el.Val).String(), "podIP", podIP) - return fmt.Errorf("failed to delete corresponding svc_pod mapping: %v", err) - } + if !bytes.Equal(el.Key, parsedPodIP) { + continue + } + if !bytes.Equal(el.Val, parsedSvcIP) { + log.Info("Updating egress SNAT for pod", "podIP", podIP, + "oldSvcIP", net.IP(el.Val).String(), "newSvcIP", svcIP) + if err := p.conn.SetDeleteElements(p.podSvcMap, []nftables.SetElement{{Key: parsedPodIP, Val: el.Val}}); err != nil { + log.Error(err, "Failed to delete stale pod_svc mapping", "podIP", podIP) + return fmt.Errorf("failed to delete stale pod_svc mapping: %v", err) + } + // Commit the removal on its own. Sharing the batch with the + // addition below would let an element that is already gone abort + // the addition too, and nothing retries it. + if err := p.flushTolerateENOENT("EnsureEgressSNAT conflict removal"); err != nil { + log.Error(err, "Failed to commit stale pod_svc removal", "podIP", podIP) + return fmt.Errorf("failed to commit stale pod_svc removal: %v", err) } - break // podIP mapping handled; exit loop. } + break } - // --- Add the new mapping to both maps --- if err := p.conn.SetAddElements(p.podSvcMap, []nftables.SetElement{{Key: parsedPodIP, Val: parsedSvcIP}}); err != nil { log.Error(err, "Failed to add mapping to pod_svc", "podIP", podIP, "svcIP", svcIP) return fmt.Errorf("failed to add mapping to pod_svc: %v", err) } - if err := p.conn.SetAddElements(p.svcPodMap, []nftables.SetElement{{Key: parsedSvcIP, Val: parsedPodIP}}); err != nil { - log.Error(err, "Failed to add mapping to svc_pod", "svcIP", svcIP, "podIP", podIP) - return fmt.Errorf("failed to add mapping to svc_pod: %v", err) - } - log.Info("Added mapping", "svcIP", svcIP, "podIP", podIP) - - // Commit all changes. if err := p.conn.Flush(); err != nil { - log.Error(err, "Failed to commit EnsureNAT changes") - return fmt.Errorf("failed to commit EnsureNAT changes: %v", err) + log.Error(err, "Failed to commit egress SNAT changes") + return fmt.Errorf("failed to commit egress SNAT changes: %v", err) } - log.Info("NAT mapping ensured successfully", "svcIP", svcIP, "podIP", podIP) + log.Info("Egress SNAT ensured successfully", "svcIP", svcIP, "podIP", podIP) return nil } -// DeleteRules removes the mapping for the given svcIP and podIP from both maps -// and commits the removal from NAT translation maps. -func (p *NFTProxyProcessor) DeleteRules(svcIP, podIP string) error { - log.Info("Deleting NAT mapping", "svcIP", svcIP, "podIP", podIP) +// DeleteEgressSNAT removes the pod_svc entry for the pair. An entry that is +// already gone is not an error. +func (p *NFTProxyProcessor) DeleteEgressSNAT(svcIP, podIP string) error { + p.mu.Lock() + defer p.mu.Unlock() - // Parse svcIP and podIP into IPv4 byte slices. - parsedSvcIP := net.ParseIP(svcIP).To4() - if parsedSvcIP == nil { - return fmt.Errorf("invalid svcIP: %s", svcIP) - } - parsedPodIP := net.ParseIP(podIP).To4() - if parsedPodIP == nil { - return fmt.Errorf("invalid podIP: %s", podIP) + log.Info("Deleting egress SNAT", "svcIP", svcIP, "podIP", podIP) + + parsedSvcIP, parsedPodIP, err := parsePair(svcIP, podIP) + if err != nil { + return err } - // Delete mapping from the "pod_svc" map. if err := p.conn.SetDeleteElements(p.podSvcMap, []nftables.SetElement{ {Key: parsedPodIP, Val: parsedSvcIP}, }); err != nil { log.Error(err, "Failed to delete mapping from pod_svc", "podIP", podIP, "svcIP", svcIP) return fmt.Errorf("failed to delete mapping from pod_svc: %v", err) } + if err := p.flushTolerateENOENT("DeleteEgressSNAT"); err != nil { + log.Error(err, "Failed to commit egress SNAT deletion") + return fmt.Errorf("failed to commit egress SNAT deletion: %v", err) + } + log.Info("Egress SNAT deleted successfully", "svcIP", svcIP, "podIP", podIP) + return nil +} + +// EnsureIngressDNAT adds the svc_pod entry (svcIP → podIP) consulted by the +// ingress_dnat chain. If svcIP is already mapped to another pod — a migrated +// VM, a replaced backend — that stale entry is dropped first. +// +// Programmed only by the node hosting the backend, see +// ProxyProcessor.EnsureIngressDNAT. +func (p *NFTProxyProcessor) EnsureIngressDNAT(svcIP, podIP string) error { + p.mu.Lock() + defer p.mu.Unlock() + + log.Info("Ensuring ingress DNAT", "svcIP", svcIP, "podIP", podIP) + + parsedSvcIP, parsedPodIP, err := parsePair(svcIP, podIP) + if err != nil { + return err + } + + svcPodElems, err := p.conn.GetSetElements(p.svcPodMap) + if err != nil { + log.Error(err, "Failed to get svc_pod map elements") + return fmt.Errorf("failed to get svc_pod map elements: %v", err) + } + for _, el := range svcPodElems { + if !bytes.Equal(el.Key, parsedSvcIP) { + continue + } + if !bytes.Equal(el.Val, parsedPodIP) { + log.Info("Updating ingress DNAT for svc", "svcIP", svcIP, + "oldPodIP", net.IP(el.Val).String(), "newPodIP", podIP) + if err := p.conn.SetDeleteElements(p.svcPodMap, []nftables.SetElement{{Key: parsedSvcIP, Val: el.Val}}); err != nil { + log.Error(err, "Failed to delete stale svc_pod mapping", "svcIP", svcIP) + return fmt.Errorf("failed to delete stale svc_pod mapping: %v", err) + } + // Commit the removal on its own, see EnsureEgressSNAT. + if err := p.flushTolerateENOENT("EnsureIngressDNAT conflict removal"); err != nil { + log.Error(err, "Failed to commit stale svc_pod removal", "svcIP", svcIP) + return fmt.Errorf("failed to commit stale svc_pod removal: %v", err) + } + } + break + } + + if err := p.conn.SetAddElements(p.svcPodMap, []nftables.SetElement{{Key: parsedSvcIP, Val: parsedPodIP}}); err != nil { + log.Error(err, "Failed to add mapping to svc_pod", "svcIP", svcIP, "podIP", podIP) + return fmt.Errorf("failed to add mapping to svc_pod: %v", err) + } + if err := p.conn.Flush(); err != nil { + log.Error(err, "Failed to commit ingress DNAT changes") + return fmt.Errorf("failed to commit ingress DNAT changes: %v", err) + } + log.Info("Ingress DNAT ensured successfully", "svcIP", svcIP, "podIP", podIP) + return nil +} + +// DeleteIngressDNAT removes the svc_pod entry for the pair. An entry that is +// already gone is not an error. +func (p *NFTProxyProcessor) DeleteIngressDNAT(svcIP, podIP string) error { + p.mu.Lock() + defer p.mu.Unlock() + + log.Info("Deleting ingress DNAT", "svcIP", svcIP, "podIP", podIP) + + parsedSvcIP, parsedPodIP, err := parsePair(svcIP, podIP) + if err != nil { + return err + } - // Delete mapping from the "svc_pod" map. if err := p.conn.SetDeleteElements(p.svcPodMap, []nftables.SetElement{ {Key: parsedSvcIP, Val: parsedPodIP}, }); err != nil { log.Error(err, "Failed to delete mapping from svc_pod", "svcIP", svcIP, "podIP", podIP) return fmt.Errorf("failed to delete mapping from svc_pod: %v", err) } + if err := p.flushTolerateENOENT("DeleteIngressDNAT"); err != nil { + log.Error(err, "Failed to commit ingress DNAT deletion") + return fmt.Errorf("failed to commit ingress DNAT deletion: %v", err) + } + log.Info("Ingress DNAT deleted successfully", "svcIP", svcIP, "podIP", podIP) + return nil +} - // Commit all changes. - if err := p.conn.Flush(); err != nil { - // Check if the error is ENOENT (no such file or directory) and ignore it. - // This may happen if the elements or even the table were already removed. - if errors.Is(err, unix.ENOENT) { - log.Info("Ignoring ENOENT error during flush in DeleteRules", "error", err) - } else { - log.Error(err, "Failed to commit DeleteNAT changes") - return fmt.Errorf("failed to commit DeleteNAT changes: %v", err) - } +// parsePair validates a (svcIP, podIP) pair and returns both as IPv4 bytes. +func parsePair(svcIP, podIP string) (net.IP, net.IP, error) { + parsedSvcIP := net.ParseIP(svcIP).To4() + if parsedSvcIP == nil { + return nil, nil, fmt.Errorf("invalid svcIP: %s", svcIP) + } + parsedPodIP := net.ParseIP(podIP).To4() + if parsedPodIP == nil { + return nil, nil, fmt.Errorf("invalid podIP: %s", podIP) + } + return parsedSvcIP, parsedPodIP, nil +} + +// flushTolerateENOENT commits the pending batch and treats ENOENT as success. +// +// Deleting a set element that is already gone reports ENOENT, which fails the +// whole flush. Deletions must therefore be committed on their own, so a stale +// element cannot mask a genuine failure among the additions that would +// otherwise share the batch. +// Note on detection: the errno travels as an *netlink.OpError wrapping a +// syscall.Errno, which errors.Is unwraps, and that is the path taken when the +// kernel refuses a batch. github.com/google/nftables v0.3.0 has one branch in +// receiveAckAware that formats a trailing error ack with %v instead of wrapping +// it, and an ENOENT arriving that way reads as an ordinary failure. v0.3.0 is +// the latest release, so the caller is returned an error in that case rather +// than silently continuing — which is the safe direction. +func (p *NFTProxyProcessor) flushTolerateENOENT(op string) error { + err := p.conn.Flush() + if err == nil { + return nil + } + if errors.Is(err, unix.ENOENT) { + log.Info("Ignoring ENOENT on flush — element already gone", "op", op) + return nil + } + return err +} + +// deleteElementsTolerant removes elements from a set, falling back to one +// commit per element when a batched commit reports that one of them is gone. +// +// A flush is a single nftables transaction: one missing element aborts every +// other deletion queued with it. Tolerating that ENOENT on the batch would +// report success while removing nothing, leaving exactly the stale state the +// purge exists to clear — which is how a node can keep entries belonging to a +// scope it no longer programs. Retrying per element skips only the ones that +// really are gone. +func (p *NFTProxyProcessor) deleteElementsTolerant(m *nftables.Set, elems []nftables.SetElement, op string) error { + if len(elems) == 0 { + return nil + } + + if err := p.conn.SetDeleteElements(m, elems); err != nil { + return fmt.Errorf("failed to queue deletions for %s: %v", m.Name, err) + } + err := p.conn.Flush() + if err == nil { + return nil + } + if !errors.Is(err, unix.ENOENT) { + return fmt.Errorf("failed to commit deletions for %s: %v", m.Name, err) } - log.Info("NAT mapping and raw set elements deleted successfully", "svcIP", svcIP, "podIP", podIP) + log.Info("Batched deletion hit ENOENT, retrying element by element", + "op", op, "set", m.Name, "elements", len(elems)) + removed := 0 + for _, el := range elems { + if err := p.conn.SetDeleteElements(m, []nftables.SetElement{el}); err != nil { + return fmt.Errorf("failed to queue deletion for %s: %v", m.Name, err) + } + if err := p.conn.Flush(); err != nil { + if errors.Is(err, unix.ENOENT) { + continue // already gone, nothing to do + } + return fmt.Errorf("failed to commit deletion for %s: %v", m.Name, err) + } + removed++ + } + log.Info("Element-by-element deletion completed", + "op", op, "set", m.Name, "removed", removed, "alreadyGone", len(elems)-removed) return nil } -// CleanupRules receives a keepMap (keys: svcIP, values: podIP) representing the desired state. -// It recovers from an inconsistent state by: -// 1. Removing any mappings in the pod_svc and svc_pod maps that do not match keepMap. -// 2. Adding any missing mappings from keepMap into both maps. -// 3. Cleaning up the raw sets (pod and svc) so that only the desired IPs remain. -func (p *NFTProxyProcessor) CleanupRules(keepMap map[string]string) error { - log.Info("Starting CleanupRules", "keepMap", keepMap) +// CleanupRules reconciles both NAT maps against the desired state. +// +// keepEgress and keepIngress both map service IP → pod IP. They differ in +// scope, which is the whole point: pod_svc must carry every managed service so +// a reply reaching this node over the overlay still gets its source rewritten, +// while svc_pod must carry only the backends hosted here so no other node +// translates a destination before the packet has left. +// +// Anything present in a map but absent from its keep set is removed, which is +// what purges state inherited from a build that scoped the two maps alike. +func (p *NFTProxyProcessor) CleanupRules(keepEgress, keepIngress map[string]string) error { + p.mu.Lock() + defer p.mu.Unlock() - // --- Step 1: Clean up mapping sets --- + log.Info("Starting CleanupRules", "keepEgress", keepEgress, "keepIngress", keepIngress) - // Retrieve current mappings from the pod_svc map. - // Note: pod_svc maps pod IP → svc IP. - podSvcElems, err := p.conn.GetSetElements(p.podSvcMap) + // --- Step 1: collect what has to change in both maps --- + + // pod_svc is keyed by pod IP, so the desired state is keepEgress inverted. + desiredPodSvc := make(map[string]string, len(keepEgress)) // pod → svc + for svc, pod := range keepEgress { + desiredPodSvc[pod] = svc + } + + podSvcDel, podSvcAdd, err := p.diffMap(p.podSvcMap, desiredPodSvc) + if err != nil { + return fmt.Errorf("failed to diff pod_svc: %v", err) + } + svcPodDel, svcPodAdd, err := p.diffMap(p.svcPodMap, keepIngress) if err != nil { - log.Error(err, "Failed to get pod_svc elements") - return fmt.Errorf("failed to get pod_svc elements: %v", err) + return fmt.Errorf("failed to diff svc_pod: %v", err) } - // Build a current mapping in svc->pod direction (for easy comparison with keepMap) - currentMapping := make(map[string]string) // key: svc, value: pod - for _, el := range podSvcElems { - pod := net.IP(el.Key).String() - svc := net.IP(el.Val).String() - currentMapping[svc] = pod - } - - // Prepare slices for elements to delete from both maps. - var toDeletePodSvc []nftables.SetElement - var toDeleteSvcPod []nftables.SetElement - - // For each mapping found in the current configuration, if it does not match the desired state, mark it for deletion. - for svc, pod := range currentMapping { - if expectedPod, ok := keepMap[svc]; !ok || expectedPod != pod { - log.Info("Marking inconsistent mapping for deletion", "svcIP", svc, "podIP", pod) - // Prepare deletion elements. - // pod_svc: key = pod, val = svc. - toDeletePodSvc = append(toDeletePodSvc, nftables.SetElement{ - Key: net.ParseIP(pod).To4(), - Val: net.ParseIP(svc).To4(), - }) - // svc_pod: key = svc, val = pod. - toDeleteSvcPod = append(toDeleteSvcPod, nftables.SetElement{ - Key: net.ParseIP(svc).To4(), - Val: net.ParseIP(pod).To4(), - }) - } - } - - // Delete any inconsistent mappings. - if len(toDeletePodSvc) > 0 { - if err := p.conn.SetDeleteElements(p.podSvcMap, toDeletePodSvc); err != nil { - log.Error(err, "Failed to delete inconsistent mappings from pod_svc") - return fmt.Errorf("failed to delete inconsistent mappings from pod_svc: %v", err) - } - if err := p.conn.SetDeleteElements(p.svcPodMap, toDeleteSvcPod); err != nil { - log.Error(err, "Failed to delete inconsistent mappings from svc_pod") - return fmt.Errorf("failed to delete inconsistent mappings from svc_pod: %v", err) - } - log.Info("Inconsistent mappings removed from both maps") + // --- Step 2: commit the deletions on their own --- + // + // An element that is already gone fails the flush, and a shared batch + // would report that as a cleanup failure, aborting the controller at + // startup. + if len(podSvcDel) > 0 || len(svcPodDel) > 0 { + if err := p.deleteElementsTolerant(p.podSvcMap, podSvcDel, "CleanupRules"); err != nil { + log.Error(err, "Failed to delete stale mappings from pod_svc") + return err + } + if err := p.deleteElementsTolerant(p.svcPodMap, svcPodDel, "CleanupRules"); err != nil { + log.Error(err, "Failed to delete stale mappings from svc_pod") + return err + } + log.Info("Stale mappings removed", "podSvc", len(podSvcDel), "svcPod", len(svcPodDel)) } else { - log.Info("No inconsistent mappings found in maps") + log.Info("No stale mappings found in maps") } - // --- Step 2: Add missing mappings from keepMap --- - - // For every desired mapping in keepMap, ensure it exists in both maps. - for svc, pod := range keepMap { - // Check if the current mapping for svc exists and matches. - if existingPod, ok := currentMapping[svc]; !ok || existingPod != pod { - parsedSvcIP := net.ParseIP(svc).To4() - parsedPodIP := net.ParseIP(pod).To4() - if parsedSvcIP == nil || parsedPodIP == nil { - log.Error(nil, "Invalid IP in keepMap", "svcIP", svc, "podIP", pod) - continue - } - // Add mapping to pod_svc (pod → svc) - if err := p.conn.SetAddElements(p.podSvcMap, []nftables.SetElement{{Key: parsedPodIP, Val: parsedSvcIP}}); err != nil { - log.Error(err, "Failed to add missing mapping to pod_svc", "podIP", pod, "svcIP", svc) - return fmt.Errorf("failed to add missing mapping to pod_svc: %v", err) - } - // Add mapping to svc_pod (svc → pod) - if err := p.conn.SetAddElements(p.svcPodMap, []nftables.SetElement{{Key: parsedSvcIP, Val: parsedPodIP}}); err != nil { - log.Error(err, "Failed to add missing mapping to svc_pod", "svcIP", svc, "podIP", pod) - return fmt.Errorf("failed to add missing mapping to svc_pod: %v", err) - } - log.Info("Added missing mapping", "svcIP", svc, "podIP", pod) + // --- Step 3: add whatever is missing --- + if len(podSvcAdd) > 0 { + if err := p.conn.SetAddElements(p.podSvcMap, podSvcAdd); err != nil { + log.Error(err, "Failed to add missing mappings to pod_svc") + return fmt.Errorf("failed to add missing mappings to pod_svc: %v", err) + } + } + if len(svcPodAdd) > 0 { + if err := p.conn.SetAddElements(p.svcPodMap, svcPodAdd); err != nil { + log.Error(err, "Failed to add missing mappings to svc_pod") + return fmt.Errorf("failed to add missing mappings to svc_pod: %v", err) } } - // --- Final commit --- + // Startup cleanup must not be fatal: aborting here takes the whole + // DaemonSet pod down and leaves the node's datapath half-programmed, while + // the reconcile loop would have converged on the next event anyway. + // Additions are committed strictly: an ENOENT on an addition-only batch + // means the table or the set is gone, so tolerating it would report a + // successful reconciliation with nothing installed. if err := p.conn.Flush(); err != nil { log.Error(err, "Failed to commit cleanup changes") return fmt.Errorf("failed to commit cleanup changes: %v", err) } - log.Info("CleanupRules completed successfully") + log.Info("CleanupRules completed successfully", + "podSvcAdded", len(podSvcAdd), "svcPodAdded", len(svcPodAdd)) return nil } +// diffMap compares one nft map against its desired state and returns the +// elements to delete and to add. Both the map and desired are read in the +// map's own key → value direction. +func (p *NFTProxyProcessor) diffMap(m *nftables.Set, desired map[string]string) (del, add []nftables.SetElement, err error) { + elems, err := p.conn.GetSetElements(m) + if err != nil { + log.Error(err, "Failed to get map elements", "map", m.Name) + return nil, nil, fmt.Errorf("failed to get %s elements: %v", m.Name, err) + } + + current := make(map[string]string, len(elems)) + for _, el := range elems { + key := net.IP(el.Key).String() + val := net.IP(el.Val).String() + current[key] = val + if want, ok := desired[key]; !ok || want != val { + log.Info("Marking stale mapping for deletion", "map", m.Name, "key", key, "value", val) + del = append(del, nftables.SetElement{Key: el.Key, Val: el.Val}) + } + } + + for key, val := range desired { + if cur, ok := current[key]; ok && cur == val { + continue + } + parsedKey := net.ParseIP(key).To4() + parsedVal := net.ParseIP(val).To4() + if parsedKey == nil || parsedVal == nil { + log.Error(nil, "Invalid IP in desired state", "map", m.Name, "key", key, "value", val) + continue + } + log.Info("Adding missing mapping", "map", m.Name, "key", key, "value", val) + add = append(add, nftables.SetElement{Key: parsedKey, Val: parsedVal}) + } + return del, add, nil +} + // EnsurePortFilter installs ingress port filtering rules for the given // (svcIP, podIP) pair. The actual nft set keys are pod IPs, because the // port_filter chain runs at priority filter (0), after ingress_dnat has @@ -632,13 +774,16 @@ func (p *NFTProxyProcessor) CleanupRules(keepMap map[string]string) error { // permitted on the post-DNAT pod IP; all other traffic destined to that // pod IP is dropped. Idempotent. func (p *NFTProxyProcessor) EnsurePortFilter(svcIP, podIP string, ports []corev1.ServicePort) error { + p.mu.Lock() + defer p.mu.Unlock() + // Empty ports list is documented as equivalent to DeletePortFilter: // the caller wants to disable filtering for this pod entirely. Without // this short-circuit we would add the pod to filtered_pods with no // matching allowed_ports entries, which would drop every ingress packet // to the pod IP — the opposite of "no filter". if len(ports) == 0 { - return p.DeletePortFilter(svcIP, podIP) + return p.deletePortFilterLocked(svcIP, podIP) } log.Info("Ensuring port filter", "svcIP", svcIP, "podIP", podIP, "portCount", len(ports)) @@ -648,10 +793,16 @@ func (p *NFTProxyProcessor) EnsurePortFilter(svcIP, podIP string, ports []corev1 } // 1. Remove any pre-existing entries in allowed_ports for podIP so we can - // rebuild the tuple set cleanly (idempotent). + // rebuild the tuple set cleanly (idempotent). Committed on its own: an + // element that is already gone would otherwise abort the additions below + // in the same transaction, leaving the pod in filtered_pods with no + // allowed port — every packet to it dropped. if err := p.removeAllowedPortsForPod(parsedPodIP); err != nil { return err } + if err := p.flushTolerateENOENT("EnsurePortFilter port rebuild"); err != nil { + return fmt.Errorf("failed to commit allowed_ports rebuild for pod %s: %v", podIP, err) + } // 2. Add podIP to filtered_pods (idempotent — Add ignores duplicates). if err := p.conn.SetAddElements(p.filteredPods, []nftables.SetElement{{Key: parsedPodIP}}); err != nil { @@ -691,6 +842,16 @@ func (p *NFTProxyProcessor) EnsurePortFilter(svcIP, podIP string, ports []corev1 // port entries for it. svcIP is used only for logging context. Tolerates // ENOENT for clean idempotency. func (p *NFTProxyProcessor) DeletePortFilter(svcIP, podIP string) error { + p.mu.Lock() + defer p.mu.Unlock() + + return p.deletePortFilterLocked(svcIP, podIP) +} + +// deletePortFilterLocked is DeletePortFilter without the lock, for callers +// that already hold it. sync.Mutex is not reentrant, so EnsurePortFilter must +// come through here rather than calling the exported method. +func (p *NFTProxyProcessor) deletePortFilterLocked(svcIP, podIP string) error { log.Info("Deleting port filter", "svcIP", svcIP, "podIP", podIP) parsedPodIP := net.ParseIP(podIP).To4() if parsedPodIP == nil { @@ -722,6 +883,9 @@ func (p *NFTProxyProcessor) DeletePortFilter(svcIP, podIP string) error { // batches SetAddElements / SetDeleteElements per set, and Flushes exactly // once. func (p *NFTProxyProcessor) CleanupPortFilters(keep map[string]PortFilterEntry) error { + p.mu.Lock() + defer p.mu.Unlock() + log.Info("Starting CleanupPortFilters", "keepCount", len(keep)) // 1. Build desired state in memory. @@ -793,16 +957,14 @@ func (p *NFTProxyProcessor) CleanupPortFilters(keep map[string]PortFilterEntry) } } - // 4. Batch ops. - if len(delPods) > 0 { - if err := p.conn.SetDeleteElements(p.filteredPods, delPods); err != nil { - return fmt.Errorf("failed to delete stale filtered_pods: %v", err) - } + // 4. Batch ops. Deletions are committed first and on their own, so an + // element that is already gone cannot fail the batch carrying the + // additions below. + if err := p.deleteElementsTolerant(p.filteredPods, delPods, "CleanupPortFilters"); err != nil { + return err } - if len(delPorts) > 0 { - if err := p.conn.SetDeleteElements(p.allowedPorts, delPorts); err != nil { - return fmt.Errorf("failed to delete stale allowed_ports: %v", err) - } + if err := p.deleteElementsTolerant(p.allowedPorts, delPorts, "CleanupPortFilters"); err != nil { + return err } if len(addPods) > 0 { if err := p.conn.SetAddElements(p.filteredPods, addPods); err != nil { @@ -815,7 +977,9 @@ func (p *NFTProxyProcessor) CleanupPortFilters(keep map[string]PortFilterEntry) } } - // 5. Single flush. + // 5. Commit the additions strictly: an ENOENT here means the table or the + // set is missing, not that an element was already gone. The caller keeps + // startup non-fatal. if err := p.conn.Flush(); err != nil { return fmt.Errorf("failed to flush CleanupPortFilters: %v", err) } @@ -865,6 +1029,9 @@ func (p *NFTProxyProcessor) removeAllowedPortsForPod(parsedPodIP net.IP) error { // traffic to that pod IP bypasses the port_filter drop rule. svcIP is used // only for logging context. Idempotent. func (p *NFTProxyProcessor) EnsureICMPAllow(svcIP, podIP string) error { + p.mu.Lock() + defer p.mu.Unlock() + log.Info("Ensuring ICMP allow", "svcIP", svcIP, "podIP", podIP) parsedPodIP := net.ParseIP(podIP).To4() if parsedPodIP == nil { @@ -883,6 +1050,9 @@ func (p *NFTProxyProcessor) EnsureICMPAllow(svcIP, podIP string) error { // DeleteICMPAllow removes podIP from icmp_allowed_pods. Tolerates ENOENT for // clean idempotency. svcIP is used only for logging context. func (p *NFTProxyProcessor) DeleteICMPAllow(svcIP, podIP string) error { + p.mu.Lock() + defer p.mu.Unlock() + log.Info("Deleting ICMP allow", "svcIP", svcIP, "podIP", podIP) parsedPodIP := net.ParseIP(podIP).To4() if parsedPodIP == nil { @@ -909,6 +1079,9 @@ func (p *NFTProxyProcessor) DeleteICMPAllow(svcIP, podIP string) error { // must remain in the set. Single-pass diff with one Flush, mirroring // CleanupPortFilters. func (p *NFTProxyProcessor) CleanupICMPAllow(keep map[string]string) error { + p.mu.Lock() + defer p.mu.Unlock() + log.Info("Starting CleanupICMPAllow", "keepCount", len(keep)) desired := make(map[string]bool, len(keep)) @@ -944,10 +1117,8 @@ func (p *NFTProxyProcessor) CleanupICMPAllow(keep map[string]string) error { } } - if len(delPods) > 0 { - if err := p.conn.SetDeleteElements(p.icmpAllowedPods, delPods); err != nil { - return fmt.Errorf("failed to delete stale icmp_allowed_pods: %v", err) - } + if err := p.deleteElementsTolerant(p.icmpAllowedPods, delPods, "CleanupICMPAllow"); err != nil { + return err } if len(addPods) > 0 { if err := p.conn.SetAddElements(p.icmpAllowedPods, addPods); err != nil { @@ -955,6 +1126,7 @@ func (p *NFTProxyProcessor) CleanupICMPAllow(keep map[string]string) error { } } + // Additions committed strictly, see CleanupPortFilters. if err := p.conn.Flush(); err != nil { return fmt.Errorf("failed to flush CleanupICMPAllow: %v", err) } diff --git a/pkg/proxy/testdata/l4/announcer.nft b/pkg/proxy/testdata/l4/announcer.nft new file mode 100644 index 0000000..e2bea9f --- /dev/null +++ b/pkg/proxy/testdata/l4/announcer.nft @@ -0,0 +1,60 @@ +table ip cozy_proxy_l4 { + set vips { + type ipv4_addr + elements = { 192.0.2.10, 192.0.2.20, + 192.0.2.30 } + } + + set vip_ports { + type ipv4_addr . inet_proto . inet_service + elements = { 192.0.2.20 . tcp . 5432, + 192.0.2.10 . tcp . 80, + 192.0.2.10 . tcp . 443 } + } + + set node_ips { + type ipv4_addr + elements = { 10.200.24.11, 10.200.24.12, + 10.200.24.13 } + } + + set backends { + type ipv4_addr . inet_proto . inet_service + elements = { 10.244.0.10 . tcp . 8080, + 10.244.0.11 . tcp . 8080, + 10.244.0.10 . tcp . 8443, + 10.244.0.11 . tcp . 8443 } + } + + map backends-192.0.2.10-tcp-80 { + type 0 : ipv4_addr . inet_service + comment "tenant-a/ingress" + elements = { 0 : 10.244.0.10 . 8080, + 1 : 10.244.0.11 . 8080 } + } + + map backends-192.0.2.10-tcp-443 { + type 0 : ipv4_addr . inet_service + comment "tenant-a/ingress" + elements = { 0 : 10.244.0.10 . 8443, + 1 : 10.244.0.11 . 8443 } + } + + chain guard { + type filter hook prerouting priority mangle + 10; policy accept; + ct state established,related accept + ip daddr @vips ip daddr . meta l4proto . th dport != @vip_ports drop + } + + chain translate { + type nat hook prerouting priority dstnat - 5; policy accept; + ip daddr 192.0.2.10 tcp dport 80 dnat ip to numgen inc mod 2 map @backends-192.0.2.10-tcp-80 comment "tenant-a/ingress" + ip daddr 192.0.2.10 tcp dport 443 dnat ip to numgen inc mod 2 map @backends-192.0.2.10-tcp-443 comment "tenant-a/ingress" + ip daddr 192.0.2.20 tcp dport 5432 drop comment "tenant-b/postgres" + } + + chain masq { + type nat hook postrouting priority srcnat - 5; policy accept; + ct status dnat ip saddr @node_ips ip daddr . meta l4proto . th dport @backends masquerade fully-random + } +} diff --git a/pkg/proxy/testdata/l4/empty.nft b/pkg/proxy/testdata/l4/empty.nft new file mode 100644 index 0000000..0c9dcaa --- /dev/null +++ b/pkg/proxy/testdata/l4/empty.nft @@ -0,0 +1,32 @@ +table ip cozy_proxy_l4 { + set vips { + type ipv4_addr + } + + set vip_ports { + type ipv4_addr . inet_proto . inet_service + } + + set node_ips { + type ipv4_addr + } + + set backends { + type ipv4_addr . inet_proto . inet_service + } + + chain guard { + type filter hook prerouting priority mangle + 10; policy accept; + ct state established,related accept + ip daddr @vips ip daddr . meta l4proto . th dport != @vip_ports drop + } + + chain translate { + type nat hook prerouting priority dstnat - 5; policy accept; + } + + chain masq { + type nat hook postrouting priority srcnat - 5; policy accept; + ct status dnat ip saddr @node_ips ip daddr . meta l4proto . th dport @backends masquerade fully-random + } +} diff --git a/pkg/proxy/testdata/l4/guard-only.nft b/pkg/proxy/testdata/l4/guard-only.nft new file mode 100644 index 0000000..f9bc347 --- /dev/null +++ b/pkg/proxy/testdata/l4/guard-only.nft @@ -0,0 +1,39 @@ +table ip cozy_proxy_l4 { + set vips { + type ipv4_addr + elements = { 192.0.2.10, 192.0.2.20, + 192.0.2.30 } + } + + set vip_ports { + type ipv4_addr . inet_proto . inet_service + elements = { 192.0.2.20 . tcp . 5432, + 192.0.2.10 . tcp . 80, + 192.0.2.10 . tcp . 443 } + } + + set node_ips { + type ipv4_addr + elements = { 10.200.24.11, 10.200.24.12, + 10.200.24.13 } + } + + set backends { + type ipv4_addr . inet_proto . inet_service + } + + chain guard { + type filter hook prerouting priority mangle + 10; policy accept; + ct state established,related accept + ip daddr @vips ip daddr . meta l4proto . th dport != @vip_ports drop + } + + chain translate { + type nat hook prerouting priority dstnat - 5; policy accept; + } + + chain masq { + type nat hook postrouting priority srcnat - 5; policy accept; + ct status dnat ip saddr @node_ips ip daddr . meta l4proto . th dport @backends masquerade fully-random + } +}