bpf: Fix maglev hash with hostServices.hostNamespaceOnly #18336

ysksuzuki · 2021-12-26T08:20:58Z

This PR fixes the bug that Cilium drops packets destined to ClusterIP when configured in combination with　loadBalancer.algorithm=maglev and hostServices.hostNamespaceOnly=true. See the commit message for details.

Fixes: #17474
Fixes: #16966

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

christarazi

Thanks for the PR!

I have a couple of comments below, one for datapath and a minor comment on the CI test. The datapath comment should be validated with the other members of the datapath team (which I've CC'd).

christarazi · 2021-12-27T19:24:20Z

bpf/lib/lb.h

@@ -757,7 +780,8 @@ static __always_inline int lb6_local(const void *map, struct __ctx_buff *ctx,
 				     struct ipv6_ct_tuple *tuple,
 				     const struct lb6_service *svc,
 				     struct ct_state *state,
-				     const bool skip_l3_xlate)
+				     const bool skip_l3_xlate,
+				     const bool use_random_selection)


While this approach presumably works, I think the main concern (for me at least) is that it will cause further strain on the complexity of the BPF progs because of changing the protoype of such a fundamental, common function and all its callers.

Ultimately, the behavior we want is to force bpf_lxc to use the random backend selection AFAIU. In that case, I think there are a couple of alternatives:

Hardcode LB_SELECTION to random only for bpf_lxc at compile-time in the agent. The code for the endpoint-specific BPF prog (bpf_lxc) starts here:

cilium/pkg/datapath/linux/config/config.go

Line 804 in 2581084

func (h *HeaderfileWriter) WriteEndpointConfig(w io.Writer, e datapath.EndpointConfiguration) error {

. From my quick glance at the code, that seems like the right place given that LB_SELECTION is set inside the node variant WriteNodeConfig().

Provide a bpc_lxc-specific lb{4,6}_xlate function which will always use random backend selection, rather than rely on LB_SELECTION and is only called from the bpf_lxc code.

In both scenarios, we only pay the "cost" once, rather than at each packet (albeit almost negligible; optimization will likely help), and more importantly, reduce program complexity (no conditionals in BPF code) for future changes.

IMO, I think the first option seems cleaner to me, as it's simpler Go code mangling vs. C code changes which very likely will end up duplicating some code (I presume).

That's my 2 cents, but would love to hear what the others from the datapath team think (@cilium/bpf @brb).

Thank you for your feedback!

The first option makes sense to me too because it's simple, as you mentioned, while my current approach changes the common function and makes things more complex.

How about this one? I could confirm that the e2e tests with this branch have passed in my environment.
ysksuzuki@0d2dbc4

With this change, LB_SELECTION, initially defined in globally scoped node_config.h will be overridden in ep_config.h, and then bpf_lxc will stick to LB_SELECTION_RANDOM.

christarazi · 2021-12-27T19:30:55Z

test/k8sT/Services.go

+				ExpectAllPodsTerminated(kubectl)
+			})
+
+			It("Checks ClusterIP connectivity in combination with hostServices.hostNamespaceOnly and maglev", func() {


Usually the It block text shouldn't overlap with the Context text. The context text can be describing what the test is and the It text can be describing what the assertion is.

So, I'd probably do something like:

Context("hostServices.hostNamespaceOnly and Maglev enabled") ... It("Checks ClusterIP connectivity")

brb

Couldn't you just #define LB_SELECTION LB_SELECTION_RANDOM in bpf_lxc.c before including lb.h?

ysksuzuki · 2022-01-06T11:09:34Z

Couldn't you just #define LB_SELECTION LB_SELECTION_RANDOM in bpf_lxc.c before including lb.h?

Yes, I could confirm that the e2e tests have passed with this commit that defines LB_SELECTION_RANDOM in bpf_lxc.c. Now I finally understand what you were saying.

christarazi · 2022-01-06T17:16:47Z

Thanks Martynas. I think it would be good to add the motivation / why in the commit msg and to also add a comment explaining briefly why this is necessary inside the code itself (above the undef).

brb · 2022-01-12T08:35:43Z

test/k8sT/Services.go

@@ -566,6 +566,41 @@ var _ = SkipDescribeIf(helpers.RunsOn54Kernel, "K8sServicesTest", func() {

 			monitorRes.ExpectContains(clusterIP, "Service VIP not seen in monitor trace, indicating socket lb still in effect")
 		})
+
+		Context("hostServices.hostNamespaceOnly and Maglev enabled", func() {


I'd suggest to avoid adding additional test case which redeploys cilium, and thus increases the test suite run time. Instead, you could reuse the "Checks connectivity when skipping socket lb in pod ns" test case.

bpf/bpf_lxc.c

ysksuzuki · 2022-01-16T09:35:49Z

I needed #18493 to pass the K8sServiceTest in my environment. Will rebase the branch after it's merged.

ysksuzuki · 2022-01-16T09:44:45Z

This test is failing in my environment even with the master branch.

Summarizing 1 Failure:

[Fail] K8sPolicyTest Multi-node policy test validates fromEntities policies [It] Validates fromEntities all policy
/home/cybozu/go/src/github.com/cilium/cilium/test/k8sT/Policies.go:1686

Ran 89 of 411 Specs in 7176.412 seconds
FAIL! -- 88 Passed | 1 Failed | 0 Pending | 322 Skipped
--- FAIL: TestTest (7176.53s)
FAIL

brb

Getting closer to merging 🚀

brb · 2022-01-17T10:06:08Z

bpf/bpf_lxc.c

@@ -30,6 +30,14 @@
 #include "lib/nat46.h"
 #include "lib/identity.h"
 #include "lib/policy.h"
+
+/* Override LB_SELECTION initially defined in node_config.h to force bpf_lxc to use the random backend selection
+ * algorithm for in-cluster traffic. It will fail with the Maglev hash algorithm because Cilium doesn't provision


Nit: "It will fail" => "Otherwise, it will fail".

pkg/service/service.go

brb · 2022-01-17T10:07:35Z

test/k8sT/Services.go

@@ -459,6 +459,7 @@ var _ = SkipDescribeIf(helpers.RunsOn54Kernel, "K8sServicesTest", func() {
 			BeforeAll(func() {
 				DeployCiliumOptionsAndDNS(kubectl, ciliumFilename, map[string]string{
 					"bpf.lbExternalClusterIP": "true",
+					"loadBalancer.algorithm":  "maglev",


This needs a comment why we enable Maglev in this test case.

brb · 2022-01-17T10:07:43Z

test/k8sT/Services.go

@@ -492,6 +493,7 @@ var _ = SkipDescribeIf(helpers.RunsOn54Kernel, "K8sServicesTest", func() {
 		BeforeAll(func() {
 			DeployCiliumOptionsAndDNS(kubectl, ciliumFilename, map[string]string{
 				"hostServices.hostNamespaceOnly": "true",
+				"loadBalancer.algorithm":         "maglev",


This fixes the bug that Cilium drops packets destined to ClusterIP. Cilium tries to resolve ClusterIP to Pod IP using Maglev hash when hostServices.hostNamespaceOnly is enabled. However, it doesn't provision the Maglev LUT for ClusterIP. So it drops the packet. This commit fixes this problem by forcing bpf_lxc to use the random backend selection for ClusterIP. Also, Cilium will populate the Maglev LUT for ClusterIP if bpf.lbExternalClusterIP is set to true so that the external ingress traffic connecting ClusterIP will be properly handled. Fixes: cilium#17474 Signed-off-by: Yusuke Suzuki <yusuke-suzuki@cybozu.co.jp>

brb

Thanks!

ysksuzuki requested a review from a team December 26, 2021 08:20

ysksuzuki requested a review from a team as a code owner December 26, 2021 08:20

ysksuzuki requested a review from a team December 26, 2021 08:20

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 26, 2021

ysksuzuki requested review from jibi and christarazi December 26, 2021 08:21

maintainer-s-little-helper bot assigned jibi and christarazi Dec 26, 2021

christarazi requested a review from brb December 27, 2021 19:06

maintainer-s-little-helper bot assigned brb Dec 27, 2021

christarazi requested changes Dec 27, 2021

View reviewed changes

brb requested changes Jan 3, 2022

View reviewed changes

ysksuzuki force-pushed the fix-maglev-with-hostservice branch from 43fe453 to 31e711c Compare January 7, 2022 02:39

ysksuzuki requested review from brb and christarazi January 7, 2022 04:32

christarazi approved these changes Jan 7, 2022

View reviewed changes

maintainer-s-little-helper bot unassigned christarazi Jan 7, 2022

brb requested changes Jan 12, 2022

View reviewed changes

borkmann reviewed Jan 12, 2022

View reviewed changes

bpf/bpf_lxc.c Show resolved Hide resolved

ysksuzuki force-pushed the fix-maglev-with-hostservice branch from fec4f28 to 556fb7f Compare January 15, 2022 13:11

YutaroHayakawa mentioned this pull request Jan 17, 2022

bpf: Move to-stack trace after host firewall #18484

Merged

6 tasks

brb requested changes Jan 17, 2022

View reviewed changes

brb mentioned this pull request Jan 18, 2022

IPv6 cluster IP not routable from external to the cluster #18494

Closed

2 tasks

ysksuzuki force-pushed the fix-maglev-with-hostservice branch 2 times, most recently from 9b382ac to 2769f6f Compare January 28, 2022 02:34

ysksuzuki requested a review from brb January 28, 2022 03:04

gpl mentioned this pull request Feb 3, 2022

bpf.lbExternalClusterIP=true doesn't work with Maglev when requested from outside cluster #16966

Closed

brb approved these changes Feb 8, 2022

View reviewed changes

maintainer-s-little-helper bot unassigned brb Feb 8, 2022

brb added release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Feb 8, 2022

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 Feb 8, 2022

brb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 8, 2022

ysksuzuki requested a review from borkmann February 8, 2022 12:04

maintainer-s-little-helper bot assigned borkmann Feb 8, 2022

joamaki merged commit fc99c0c into cilium:master Feb 9, 2022

brb added the needs-backport/1.11 label Mar 17, 2022

qmonnet mentioned this pull request Mar 30, 2022

v1.11 backports 2022-03-30 #19277

Merged

18 tasks

qmonnet added backport-pending/1.11 and removed needs-backport/1.11 labels Mar 30, 2022

ysksuzuki mentioned this pull request Apr 1, 2022

Hostport doesn’t work when using Cilium with enabling maglev #19260

Closed

2 tasks

pchaigno added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Apr 4, 2022

joestringer mentioned this pull request Apr 15, 2022

Prepare for release v1.11.4 #19462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpf: Fix maglev hash with hostServices.hostNamespaceOnly #18336

bpf: Fix maglev hash with hostServices.hostNamespaceOnly #18336

ysksuzuki commented Dec 26, 2021 •

edited by brb

christarazi left a comment

christarazi Dec 27, 2021

ysksuzuki Dec 28, 2021 •

edited

christarazi Dec 27, 2021

brb left a comment

ysksuzuki commented Jan 6, 2022

christarazi commented Jan 6, 2022

brb Jan 12, 2022

ysksuzuki commented Jan 16, 2022 •

edited

ysksuzuki commented Jan 16, 2022

brb left a comment

brb Jan 17, 2022

brb Jan 17, 2022

brb Jan 17, 2022

brb left a comment

Navigation Menu

bpf: Fix maglev hash with hostServices.hostNamespaceOnly #18336

bpf: Fix maglev hash with hostServices.hostNamespaceOnly #18336

Conversation

ysksuzuki commented Dec 26, 2021 • edited by brb

christarazi left a comment

Choose a reason for hiding this comment

christarazi Dec 27, 2021

Choose a reason for hiding this comment

ysksuzuki Dec 28, 2021 • edited

Choose a reason for hiding this comment

christarazi Dec 27, 2021

Choose a reason for hiding this comment

brb left a comment

Choose a reason for hiding this comment

ysksuzuki commented Jan 6, 2022

christarazi commented Jan 6, 2022

brb Jan 12, 2022

Choose a reason for hiding this comment

ysksuzuki commented Jan 16, 2022 • edited

ysksuzuki commented Jan 16, 2022

brb left a comment

Choose a reason for hiding this comment

brb Jan 17, 2022

Choose a reason for hiding this comment

brb Jan 17, 2022

Choose a reason for hiding this comment

brb Jan 17, 2022

Choose a reason for hiding this comment

brb left a comment

Choose a reason for hiding this comment

ysksuzuki commented Dec 26, 2021 •

edited by brb

ysksuzuki Dec 28, 2021 •

edited

ysksuzuki commented Jan 16, 2022 •

edited