LRP: Fix loopback in per packet LB mode by pravk03 · Pull Request #33721 · cilium/cilium

pravk03 · 2024-07-10T23:01:33Z

This PR fixes cases with the local redirect policy datapath behavior with per-packet LB resulting in loopback. The loopback occurs when the request originates from the node local backend and the local redirect service translation in the bpf program at the LXC interface causes the request to be redirected back to itself. This PR extends the fix in #26144 to work for per-packet LB mode.

Fixes: #30558

Fix loopback with LRP when per-packet LB is used.

maintainer-s-little-helper · 2024-07-10T23:01:36Z

Commits 20b9898, 91c5e0c do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2024-07-10T23:05:34Z

Commits 20b9898, 91c5e0c, ccbf71f do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2024-07-10T23:08:03Z

Commits 20b9898, 91c5e0c do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2024-07-10T23:08:39Z

Commits 20b9898, 91c5e0c do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2024-07-10T23:26:59Z

Commits ffd7e89, 3e552c2 do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2024-07-11T17:18:07Z

Commits ffd7e89, 3e552c2, 2261aae do not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

joestringer · 2024-07-11T17:56:16Z

@pravk03 thanks for the fix! Please make sure to sign off your commits per the comment from the bot above before marking PRs ready for review.

maintainer-s-little-helper · 2024-07-11T19:11:30Z

Commit bc606fe does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

joestringer · 2024-07-11T20:02:26Z

/test

ysksuzuki · 2024-07-31T06:21:16Z

/test

ysksuzuki · 2024-08-01T06:39:41Z

ci-l4lb is consistently failing. I think 809ce56 fixes this falure. @pravk03 Could you rebase your branch?

time="2024-07-31T12:04:37Z" level=fatal msg="failed to start: daemon creation failed: CRD Identity allocation mode requires k8s to be configured\nfailed to stop: unable to find controller ipcache-inject-labels" subsys=daemon

Since the bpf helper function bpf_get_netns_cookie() is not exposed to the TC BPF programs, we would need to use ELF substitutions to make the network namespace cookie of an endpoint available to the BPF program attached. In a subsequent patch, we would use this in TC BPF program to detect LRP loopback. Signed-off-by: Praveen Krishna <pkrishn@google.com>

These functions would also be invoked from bpf_lxc.c in the subsequent commit. Signed-off-by: Praveen Krishna <pkrishn@google.com>

Skip service translation logic for a local-redirect service when the packet originates from the node local backend. Skip LB only for socket tuples matching local redirect policy tuples populated in the cilium_skip_lb{4,6} maps. The maps are populated by the userspace based on user deployed LRPs. Signed-off-by: Praveen Krishna <pkrishn@google.com>

Adds test cases to validate lrp loopback fix in per-packet LB mode. Signed-off-by: Praveen Krishna <pkrishn@google.com>

Update the added support for LRP skipRedirectFromBackend with per packet based LB. Signed-off-by: Praveen Krishna <pkrishn@google.com>

pravk03 · 2024-08-02T00:23:59Z

@ysksuzuki rebased.

ysksuzuki · 2024-08-02T05:14:18Z

/test

ysksuzuki · 2024-08-03T02:59:15Z

I will add e2e test before backporting the PR to v1.16.

julianwiedmann

Just a few non-blocking comments, maybe you want to address these in a follow-up PR.

julianwiedmann · 2024-08-04T20:00:52Z

Documentation/network/kubernetes/local-redirect-policy.rst

+The configuration is supported with socket-based load-balancing and per packet based load-balancing, and requires ``SO_NETNS_COOKIE`` feature
 available in Linux kernel version >= 5.8.


Suggested change

The configuration is supported with socket-based load-balancing and per packet based load-balancing, and requires ``SO_NETNS_COOKIE`` feature

available in Linux kernel version >= 5.8.

This configuration requires the ``SO_NETNS_COOKIE`` feature available in Linux kernel version >= 5.8.

If it now works for either variant of load-balancing, let's not have the user worry about unnecessary details :).

julianwiedmann · 2024-08-04T20:02:58Z

bpf/tests/skip_lb_xlate_lrp_per_packet_lb.c

+	l2 = pktgen__push_ethhdr(&builder);
+	if (!l2)
+		return TEST_ERROR;
+	ethhdr__set_macs(l2, (__u8 *)src, (__u8 *)dst);
+	/* Push IPv4 header */
+	l3 = pktgen__push_default_iphdr(&builder);
+	if (!l3)
+		return TEST_ERROR;
+	l3->saddr = V4_BACKEND_IP;
+	l3->daddr = V4_SERVICE_IP;
+	/* Push TCP header */
+	l4 = pktgen__push_default_tcphdr(&builder);
+	if (!l4)
+		return TEST_ERROR;
+	l4->source = tcp_src_one;
+	l4->dest = SERVICE_PORT;


Use pktgen__push_ipv4_tcp_packet() here? (same in the other PKTGEN stage below)

julianwiedmann · 2024-08-04T20:04:47Z

bpf/tests/skip_lb_xlate_lrp_per_packet_lb.c

+	lb_svc_key.address = V4_SERVICE_IP;
+	lb_svc_key.dport = SERVICE_PORT;
+	lb_svc_key.scope = LB_LOOKUP_SCOPE_EXT;
+	/* Create a LRP service with one backend */
+	lb_svc_value.count = 1;
+	lb_svc_value.flags = SVC_FLAG_ROUTABLE;
+	lb_svc_value.flags2 = SVC_FLAG_LOCALREDIRECT;
+	lb_svc_value.rev_nat_index = revnat_id;
+	map_update_elem(&LB4_SERVICES_MAP_V2, &lb_svc_key, &lb_svc_value, BPF_ANY);
+	/* Insert a reverse NAT entry for the above service */
+	revnat_value.address = V4_SERVICE_IP;
+	revnat_value.port = SERVICE_PORT;
+	map_update_elem(&LB4_REVERSE_NAT_MAP, &revnat_id, &revnat_value, BPF_ANY);
+	/* A backend between 1 and .count is chosen, since we have only one backend
+	 * it is always backend_slot 1. Point it to backend_id 124.
+	 */
+	lb_svc_key.backend_slot = 1;
+	lb_svc_value.backend_id = 124;
+	map_update_elem(&LB4_SERVICES_MAP_V2, &lb_svc_key, &lb_svc_value, BPF_ANY);
+
+	/* Add the service in LB4_SKIP_MAP to skip service translation for request originating from the local backend */
+	struct skip_lb4_key key = {
+		.netns_cookie = ENDPOINT_NETNS_COOKIE,
+		.address = V4_SERVICE_IP,
+		.port = SERVICE_PORT,
+	};
+	__u8 val = 0;
+
+	map_update_elem(&LB4_SKIP_MAP, &key, &val, BPF_ANY);
+
+	/* Create backend id 124 which contains the IP and port to send the
+	 * packet to.
+	 */
+	backend.address = V4_BACKEND_IP;
+	backend.port = BACKEND_PORT;
+	backend.proto = IPPROTO_TCP;
+	backend.flags = 0;
+	map_update_elem(&LB4_BACKEND_MAP, &lb_svc_value.backend_id, &backend, BPF_ANY);
+	/* Add an IPCache entry for pod 1 */
+	cache_key.lpm_key.prefixlen = 32;
+	cache_key.family = ENDPOINT_KEY_IPV4;
+	cache_key.ip4 = V4_BACKEND_IP;
+	/* a random sec id for the pod */
+	cache_value.sec_identity = 112233;
+	map_update_elem(&IPCACHE_MAP, &cache_key, &cache_value, BPF_ANY);
+	ep_key.ip4 = V4_BACKEND_IP;
+	ep_key.family = ENDPOINT_KEY_IPV4;
+	map_update_elem(&ENDPOINTS_MAP, &ep_key, &ep_value, BPF_ANY);


I want to believe that we have helpers for most of this in bpf/tests/lib/... now. Except the LB4_SKIP_MAP operations, which would be a good addition?

Thanks. Created #34443.

#33721 refactored the LRP code in bpf_sock, and dropped the HAVE_NETNS_COOKIE guard when calling get_netns_cookie(). When LRP is enabled, this causes the verifier on pre-5.8 kernels (where the helper is not available) to reject the bpf_sock program with "invalid func unknown#122". Re-introduce the check for HAVE_NETNS_COOKIE in the IPv4 path. This matches the IPv6 path. Fixes: 9d8b5f7 ("bpf:move LBx skip map and lookup functions to lb.h") Fixes: #40457 Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

[ upstream commit 5745846 ] #33721 refactored the LRP code in bpf_sock, and dropped the HAVE_NETNS_COOKIE guard when calling get_netns_cookie(). When LRP is enabled, this causes the verifier on pre-5.8 kernels (where the helper is not available) to reject the bpf_sock program with "invalid func unknown#122". Re-introduce the check for HAVE_NETNS_COOKIE in the IPv4 path. This matches the IPv6 path. Fixes: 9d8b5f7 ("bpf:move LBx skip map and lookup functions to lb.h") Fixes: #40457 Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jul 10, 2024

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jul 10, 2024

pravk03 force-pushed the pr/lrp-loopback-fix branch from ccbf71f to 91c5e0c Compare July 10, 2024 23:08

pravk03 force-pushed the pr/lrp-loopback-fix branch from 91c5e0c to d4d203e Compare July 10, 2024 23:08

pravk03 force-pushed the pr/lrp-loopback-fix branch from d4d203e to 3e552c2 Compare July 10, 2024 23:26

borkmann requested review from aditighag and ysksuzuki July 11, 2024 09:46

pravk03 marked this pull request as ready for review July 11, 2024 17:18

pravk03 requested review from a team as code owners July 11, 2024 17:18

pravk03 requested review from christarazi and lmb July 11, 2024 17:18

joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jul 11, 2024

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 11, 2024

joestringer added the needs-backport/1.16 label Jul 11, 2024

pravk03 force-pushed the pr/lrp-loopback-fix branch from 2261aae to 94d8013 Compare July 11, 2024 19:11

pravk03 force-pushed the pr/lrp-loopback-fix branch from 94d8013 to 9f33d2d Compare July 11, 2024 19:13

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jul 11, 2024

viktor-kurchenko approved these changes Jul 29, 2024

View reviewed changes

julianwiedmann added area/lrp Impacts Local Redirect Policy. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Jul 30, 2024

pravk03 added 5 commits August 1, 2024 17:22

bpf:move LBx skip map and lookup functions to lb.h

29bb99c

These functions would also be invoked from bpf_lxc.c in the subsequent commit. Signed-off-by: Praveen Krishna <pkrishn@google.com>

bpf: Add tests for LRP loopback fix

511f95e

Adds test cases to validate lrp loopback fix in per-packet LB mode. Signed-off-by: Praveen Krishna <pkrishn@google.com>

doc:LRP skipRedirectFromBackend with per packet LB

23ca9f7

Update the added support for LRP skipRedirectFromBackend with per packet based LB. Signed-off-by: Praveen Krishna <pkrishn@google.com>

pravk03 force-pushed the pr/lrp-loopback-fix branch from 0efc431 to 23ca9f7 Compare August 2, 2024 00:22

ysksuzuki added the backport/author The backport will be carried out by the author of the PR. label Aug 3, 2024

ysksuzuki approved these changes Aug 3, 2024

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 3, 2024

julianwiedmann reviewed Aug 4, 2024

View reviewed changes

julianwiedmann added this pull request to the merge queue Aug 4, 2024

Merged via the queue into cilium:main with commit 62b6a17 Aug 4, 2024

ysksuzuki mentioned this pull request Sep 19, 2024

lrp: define ENABLE_LOCAL_REDIRECT_POLICY regardless of socketLB setting #34954

Merged

ysksuzuki mentioned this pull request Sep 28, 2024

[v1.16] author backport: LRP fixes #35072

Merged

julianwiedmann added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed needs-backport/1.16 labels Sep 30, 2024

julianwiedmann mentioned this pull request Sep 1, 2025

program cil_sock6_sendmsg: load program: invalid argument #40457

Closed

3 tasks

		The configuration is supported with socket-based load-balancing and per packet based load-balancing, and requires ``SO_NETNS_COOKIE`` feature
		available in Linux kernel version >= 5.8.

	The configuration is supported with socket-based load-balancing and per packet based load-balancing, and requires ``SO_NETNS_COOKIE`` feature
	available in Linux kernel version >= 5.8.
	This configuration requires the ``SO_NETNS_COOKIE`` feature available in Linux kernel version >= 5.8.

Conversation

pravk03 commented Jul 10, 2024

Uh oh!

maintainer-s-little-helper bot commented Jul 10, 2024

Uh oh!

maintainer-s-little-helper bot commented Jul 10, 2024

Uh oh!

maintainer-s-little-helper bot commented Jul 10, 2024

Uh oh!

maintainer-s-little-helper bot commented Jul 10, 2024

Uh oh!

maintainer-s-little-helper bot commented Jul 10, 2024

Uh oh!

maintainer-s-little-helper bot commented Jul 11, 2024

Uh oh!

joestringer commented Jul 11, 2024

Uh oh!

maintainer-s-little-helper bot commented Jul 11, 2024

Uh oh!

joestringer commented Jul 11, 2024

Uh oh!

ysksuzuki commented Jul 31, 2024

Uh oh!

ysksuzuki commented Aug 1, 2024

Uh oh!

pravk03 commented Aug 2, 2024

Uh oh!

ysksuzuki commented Aug 2, 2024

Uh oh!

ysksuzuki commented Aug 3, 2024

Uh oh!

julianwiedmann left a comment

Choose a reason for hiding this comment

Uh oh!

julianwiedmann Aug 4, 2024

Choose a reason for hiding this comment

Uh oh!

pravk03 Aug 20, 2024

Choose a reason for hiding this comment

Uh oh!

julianwiedmann Aug 4, 2024

Choose a reason for hiding this comment

Uh oh!

pravk03 Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

julianwiedmann Aug 4, 2024

Choose a reason for hiding this comment

Uh oh!

pravk03 Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants