datapath: Change FIB lookups to enable NodePort multihoming

[brb]

Previously, the bpf_fib_lookup() helper was invoked in nodeport.h with
the BPF_FIB_LOOKUP_{OUTPUT,DIRECT} flags set. This commit drops both
flags to make the NodePort BPF multihoming possible.

The former flag instructed the helper to do the FIB lookup from egress
perspective. This required us to set the correct ifindex for the lookup.
In the NodePort BPF code we used DIRECT_ROUTING_DEV_IFINDEX which
corresponds to an iface which is used to communicate between k8s Nodes
(i.e., the iface with k8s Node InternalIP or ExternalIP). However, the
reliance on the DIRECT_ROUTING_DEV_IFINDEX meant that in the case of
more than a single iface used to connect the nodes, only one iface could
be used for the NodePort BPF traffic (forwarded service requests).

To fix this, we can drop the BPF_FIB_LOOKUP_OUTPUT and set the ingress
iface in the FIB lookup parameter. This makes the FIB lookup from the
perspective of the device which received a packet (before forwarding to
egress iface), and it eliminates the need for
DIRECT_ROUTING_DEV_IFINDEX in the NodePort BPF.

The latter flag meant to speed up the FIB lookups. The kernel commit which
introduced it [1] says:

BPF_FIB_LOOKUP_DIRECT to do a lookup that bypasses FIB rules and goes
straight to the table associated with the device (expert setting for
those looking to maximize throughput)

This means that we bypass ip rules. However, cilium-agent for some
setups installs IP rules (e.g., EKS, WireGuard, etc). Therefore, to
avoid any potential pitfals we drop the latter flag so that the IP rules
can be respected.

To measure a potential perf regression, I provisioned [2] scripts on
three AWS EC2 c5a.8xlarge machines (10Gbit, Ubuntu 20.04 LTS, 5.11
kernel). The netperf client was sending requests to the LB node which
was forwarding them to the destination node. The only Helm setting for
Cilium was KPR=strict. The results from three runs each setup are the
following:

---------------------+-----------------+-------------------------
                     | v1.11.1         | v1.11.1 with FIB changes
---------------------+-----------------+-------------------------
TCP_RR MEAN_LATENCY  | 358.74 +- 27.08 | 359.56 +- 30.50
with STDDEV_LATENCY  | 360.28 +- 26.01 | 361.76 +- 26.71
                     | 368.18 +- 28.22 | 365.33 +- 35.83
---------------------+-----------------+-------------------------

As you can see, the increase in latency is negligible. Therefore, this
commit drops the latter flag unconditionally, i.e., without checking
whether there are any non-default IP rules or Cilium is running on a
multi-homing setup.

[1]: torvalds/linux@87f5fc7
[2]: https://github.com/brb/cilium-without-netfilter

Fix #18199

[brb]

/ci-l4lb

[brb]

/test-1.23-net-next

[brb]

Oh wow, net-next everything except the flaky L7 test has passed. Running one more time to gain more confidence.

[brb]

/test

[brb]

/test

[brb]

/test

[brb]

Previously all relevant unquarantined tests have passed (net-next https://jenkins.cilium.io/job/Cilium-PR-K8s-1.23-kernel-net-next/313/ hit the known flakes). Dropped the unquarantine CI commit.

[brb]

/test

[borkmann]

Therefore, to avoid any potential pitfals we drop the latter flag so that the IP rules can be respected.

Given we know exactly when we install ip rules in the agent, I would suggest we should have the agent transparently opt-out of BPF_FIB_LOOKUP_DIRECT when we install them, and otherwise not.

Regarding BPF_FIB_LOOKUP_OUTPUT, should we retain it for hair-pinning LB? If the oif is set, only routes (nexthops really) using that device are considered (see fib_lookup_good_nhc()), which makes a big difference (e.g. XDP). (For egress when we do not know the egress device, set the OUTPUT flag and make sure params->ifindex = 0 to allow the lookup to determine nexthop and device. Wouldn't that work given we know at this point that the pkt is not for local delivery anyway..?)

[brb]

Given we know exactly when we install ip rules in the agent, I would suggest we should have the agent transparently opt-out of BPF_FIB_LOOKUP_DIRECT when we install them, and otherwise not.

I think it's not straight forward to determine whether cilium-agent is going to install IP rules. Also, we don't have any control about IP rules users might install. Not sure whether the perf benefits could offset the complexity in the agent and UX.

(For egress when we do not know the egress device, set the OUTPUT flag and make sure params->ifindex = 0 to allow the lookup to determine nexthop and device. Wouldn't that work given we know at this point that the pkt is not for local delivery anyway..?)

Unfortunately, we cannot set params->ifindex to 0, as then the lookup will fail due to https://github.com/torvalds/linux/blob/v5.16/net/core/filter.c#L5394 (my attempt to drop this check was NACK-ed).

[borkmann]

Ok, lgtm, tiny nit: lets remove the newline above and in v6 part.

[borkmann]

Not sure whether the perf benefits could offset the complexity in the agent and UX.

Ok, fair enough, but agree that if needed we can optimize it later on.