Fixes for ip-masq-agent #26435

qmonnet · 2023-06-23T01:32:18Z

It looks like I didn't test the ip-masq-agent quite enough when adding support for IPv6 - in particular, I didn't test with IPv4 only. Sebastian reported that when IPv6 is off, ip-masq-agent is broken. This PR contains a number of fixes to address that.

Fixes: #26262

Fix some map handling logic as well as some issues with CLI commands related to ip-masq-agent, introduced with IPv6 support

qmonnet · 2023-06-23T01:41:52Z

/test

qmonnet · 2023-06-23T10:00:49Z

/test

brb · 2023-06-23T13:27:54Z

test/k8s/datapath_configuration.go

@@ -361,6 +361,18 @@ var _ = Describe("K8sDatapathConfig", func() {
 			testIPMasqAgent()
 		})

+		It("DirectRouting, IPv4 only", func() {


JFYI, we want to move this to CLI connectivity test in v1.15.

OK thanks, this shouldn't be an issue I think? Or do you have an alternative to suggest?

For this PR it's not an issue. I'm expecting the move to happen early in the v1.15 dev cycle.

bimmlerd

contributing lgtm

qmonnet · 2023-06-23T14:40:09Z

The first two commits from the previous iteration ended up here by mistake (they're from #26454). I removed them, the rest is unchanged.

qmonnet · 2023-06-23T14:40:28Z

/test

learnitall

LGTM on changes in examples/

christarazi

LGTM for ci-structure. It would be good to have a tracking issue for migrating the test out of Ginkgo.

qmonnet · 2023-06-24T09:51:50Z

Ok, I added an item to #26074. Where would be the best location for this test? Basically I want to make sure that Cilium deploys well with the ip-masq-agent on IPv4 only (and that we don't reintroduce this IPv6-map-related warning that would make it unhealthy) and that the feature works as expected. Would connectivity tests be the way to go (as a follow-up)?

pkg/datapath/maps/map.go

lmb · 2023-06-26T09:37:08Z

cilium/cmd/bpf_ipmasq_list.go

@@ -26,7 +26,7 @@ var bpfIPMasqListCmd = &cobra.Command{
 	Run: func(cmd *cobra.Command, args []string) {
 		common.RequireRootPrivilege("cilium bpf ipmasq list")

-		cidrs, err := (&ipmasq.IPMasqBPFMap{}).Dump()
+		cidrs, err := (&ipmasq.IPMasqBPFMap{}).DumpForProtocols(getMasqueradingProtocols())


getMasqueradingProtocols seems like a pretty big hammer to me. Could you instead call IPMasq6Map and IPMasq4Map (ignoring any "file doesn't exist" error) before Dump()?

As discussed offline -- This won't work, because we're trying to Open() the map in DumpWithCallback(), so if we initialise ipmasq.ipMasq6Map by calling ipmasq.IPMasq6Map() in the CLI, we get an error:

root@kind-worker:/home/cilium# cilium bpf ipmasq list error dumping contents of map: loading pinned map /sys/fs/bpf/tc/globals/cilium_ipmasq_v6: no such file or directory

I'll follow your alternate suggestion instead, using the result of ipv4Needed := ipmasq.IPMasq4Map().Open() == nil (and v6) to check whether the maps are open and get the booleans to pass to DumpForProtocols(), instead of going for the API request.

Thanks for the review!

gandro

Looks good to me, thanks for fixing this!

qmonnet · 2023-06-26T11:29:01Z

/test

qmonnet · 2023-06-26T11:31:25Z

I forgot to update my commit log after addressing Lorenz's feedback. I updated and triggered the tests again...
/test

derailed

@qmonnet Nice work! Just a few picks...

daemon/cmd/datapath.go

pkg/datapath/maps/map.go

pkg/maps/ipmasq/ipmasq.go

derailed · 2023-06-26T15:10:44Z

pkg/maps/ipmasq/ipmasq.go

@@ -121,6 +131,10 @@ func (*IPMasqBPFMap) Dump() ([]net.IPNet, error) {
 	return cidrs, nil
 }

+func (*IPMasqBPFMap) Dump() ([]net.IPNet, error) {


Still used? Also no comment on exported method.

Perhaps a larger discussion here but this sounds like prime candidates for generics.
There is very little use for this type as shown below as since this is just a marker type that just keep checking what kind of ips are at play and whether the correct matching configs are set.

Note: in this case we need an instance that is passed in on the fly.

Still used, in the agent. The method didn't have a comment so far and I didn't think of adding one; but I can do, for both.

As for the generics, it makes sense, but have we been using them so far in the code base? This PR is a set of fixes, I'd as well avoid moving things too much or introducing generics in here.

Totally Quentin, I understand. Just a quick note, for potential refacs at some other time.

Fix a bug introduced with the support for IPv6 ip-masq-agent. The macros for IPv6 are not at the right location, and they will be set if IPv4 masquerading is set, even if it is disabled for IPv6. Let's move the code to its right location. Fixes: 7fd9a0d ("ipmasq: Enable IPv6 support for ip-masq-agent") Signed-off-by: Quentin Monnet <quentin@isovalent.com>

When adding support for IPv6 ip-masq-agent, the related maps were not handled correctly in RemoveDisabledMaps(), resulting in maps being kept, even if option.Config.EnableIPv(4|6)Masquerade is disabled. Let's fix the conditions to sweep these maps. Fixes: c5348d2 ("ipmasq: Implement ip-masq-agent support for IPv6") Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Before we introduced support for the IPv6 ip-masq-agent, only IPv4 would support it. Therefore, if the ip-masq-agent agent was enabled in the config, this meant that we had validated that IPv4 masquerading was set, and we could afford creating the related map on the combination of options (IPv4 + ip-masq-agent enabled). Now that we have IPv6 support, having IPv4 and the ip-masq-agent enabled no longer means we're using it for IPv4, given that we may chose to enable masquerading for IPv6 only. So we need to update the conditions in the datapath package, to create these maps only when necessary. Fixes: 7fd9a0d ("ipmasq: Enable IPv6 support for ip-masq-agent") Signed-off-by: Quentin Monnet <quentin@isovalent.com>

If we don't pay attention to what IP protocols should get masqueraded, and try to interact with ip-masq-agent maps without precautions, we can end up in a situation where we try to open a map that does not exist. For example, if the cluster runs with IPv4 only, and we try to add the default prefixes (IPv4 and IPv6) to the ip-masq-agent, then we try to Update() an entry in the IPv6 map for the ip-masq-agent. But this map does not exist (we run with IPv4 only and never created it). So we get a warning in the logs, preventing Cilium to be healthy: loading pinned map /sys/fs/bpf/tc/globals/cilium_ipmasq_v6: no such file or directory Fix this by restricting the operations to the IP versions for which we have masquerading enabled. Fixes: c5348d2 ("ipmasq: Implement ip-masq-agent support for IPv6") Reported-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com>

When adding support for IPv6 ip-masq-agent in Cilium, we made sure to Dump() the contents of the maps only for the map that had been defined (IPv4 and/or IPv6 versions). This works well in the agent, but not from the CLI, which also calls Dump() for "cilium bpf ipmasq list". In that case, the variable referencing the maps have never been set, and we end up exiting from the function without dumping any contents. To fix this, we need the CLI to be able to chose what maps should be needed. The ipmasq package does not expose the maps directly, so we want to pass it the values for option.Config.EnableIPv(4|6)Masquerade instead. These configuration options are not directly available from the CLI. Instead, we check whether the IPv4 and IPv6 ip-masq-agent maps are open, and let the ipmasq package know which maps to dump accordingly. Fixes: c5348d2 ("ipmasq: Implement ip-masq-agent support for IPv6") Reported-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com>

YAML indentation is not correct, and the example file cannot be applied in a straightforward fashion. Let's fix it. Fixes: 2dc829b ("docs: Simplify example how to config ipmasq via ConfigMap") Signed-off-by: Quentin Monnet <quentin@isovalent.com>

In order to detect issues occurring with ip-masq-agent when IPv4 only is enabled, add a new test to deploy the ip-masq-agent without IPv6. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet · 2023-06-26T21:15:37Z

I addressed the feedback from Fernand - I factored a check in daemon/cmd/datapath.go, and added comments for the two exported Dump*() functions - and rebased.

qmonnet · 2023-06-26T21:15:47Z

/test

derailed

@qmonnet LGTM. Thank you for the updates!

qmonnet · 2023-06-26T23:18:00Z

All codeowners are covered by reviews, and CI is green. Let's get these fixes merged

qmonnet mentioned this pull request Jun 16, 2023

Follow-ups for BPF-based IPv6 Masquerading #26074

Open

14 tasks

qmonnet force-pushed the pr/ipmasq-fixes branch 2 times, most recently from bb4044f to 5bd49ed Compare June 23, 2023 01:41

qmonnet force-pushed the pr/ipmasq-fixes branch from 5bd49ed to 033349d Compare June 23, 2023 09:43

qmonnet requested a review from gandro June 23, 2023 12:14

qmonnet marked this pull request as ready for review June 23, 2023 12:14

qmonnet requested review from a team as code owners June 23, 2023 12:14

qmonnet requested review from jschwinger233, christarazi, learnitall, lmb, derailed and bimmlerd June 23, 2023 12:14

borkmann requested a review from brb June 23, 2023 12:32

brb approved these changes Jun 23, 2023

View reviewed changes

bimmlerd approved these changes Jun 23, 2023

View reviewed changes

qmonnet force-pushed the pr/ipmasq-fixes branch from 033349d to cc48eef Compare June 23, 2023 14:39

learnitall approved these changes Jun 23, 2023

View reviewed changes

christarazi approved these changes Jun 23, 2023

View reviewed changes

lmb reviewed Jun 26, 2023

View reviewed changes

gandro approved these changes Jun 26, 2023

View reviewed changes

qmonnet force-pushed the pr/ipmasq-fixes branch from cc48eef to 71d4402 Compare June 26, 2023 11:27

qmonnet requested a review from lmb June 26, 2023 11:28

qmonnet force-pushed the pr/ipmasq-fixes branch from 71d4402 to bb5c1bc Compare June 26, 2023 11:31

lmb approved these changes Jun 26, 2023

View reviewed changes

derailed requested changes Jun 26, 2023

View reviewed changes

qmonnet added 7 commits June 26, 2023 22:12

test: Add e2e test for ip-masq-agent with IPv4 only

482819e

In order to detect issues occurring with ip-masq-agent when IPv4 only is enabled, add a new test to deploy the ip-masq-agent without IPv6. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet force-pushed the pr/ipmasq-fixes branch from bb5c1bc to 482819e Compare June 26, 2023 21:14

qmonnet requested a review from derailed June 26, 2023 21:15

derailed approved these changes Jun 26, 2023

View reviewed changes

qmonnet merged commit fa67780 into cilium:main Jun 26, 2023
65 checks passed

qmonnet deleted the pr/ipmasq-fixes branch June 26, 2023 23:18

joestringer mentioned this pull request Jun 28, 2023

Prepare for release v1.14.0-rc.0 #26544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for ip-masq-agent #26435

Fixes for ip-masq-agent #26435

qmonnet commented Jun 23, 2023

qmonnet commented Jun 23, 2023

qmonnet commented Jun 23, 2023

brb Jun 23, 2023

qmonnet Jun 23, 2023

brb Jun 26, 2023

bimmlerd left a comment

qmonnet commented Jun 23, 2023

qmonnet commented Jun 23, 2023

learnitall left a comment

christarazi left a comment

qmonnet commented Jun 24, 2023

lmb Jun 26, 2023

qmonnet Jun 26, 2023 •

edited

gandro left a comment

qmonnet commented Jun 26, 2023

qmonnet commented Jun 26, 2023 •

edited

derailed left a comment

derailed Jun 26, 2023

qmonnet Jun 26, 2023

derailed Jun 26, 2023

qmonnet commented Jun 26, 2023

qmonnet commented Jun 26, 2023

derailed left a comment

qmonnet commented Jun 26, 2023

Fixes for ip-masq-agent #26435

Fixes for ip-masq-agent #26435

Conversation

qmonnet commented Jun 23, 2023

qmonnet commented Jun 23, 2023

qmonnet commented Jun 23, 2023

brb Jun 23, 2023

Choose a reason for hiding this comment

qmonnet Jun 23, 2023

Choose a reason for hiding this comment

brb Jun 26, 2023

Choose a reason for hiding this comment

bimmlerd left a comment

Choose a reason for hiding this comment

qmonnet commented Jun 23, 2023

qmonnet commented Jun 23, 2023

learnitall left a comment

Choose a reason for hiding this comment

christarazi left a comment

Choose a reason for hiding this comment

qmonnet commented Jun 24, 2023

lmb Jun 26, 2023

Choose a reason for hiding this comment

qmonnet Jun 26, 2023 • edited

Choose a reason for hiding this comment

gandro left a comment

Choose a reason for hiding this comment

qmonnet commented Jun 26, 2023

qmonnet commented Jun 26, 2023 • edited

derailed left a comment

Choose a reason for hiding this comment

derailed Jun 26, 2023

Choose a reason for hiding this comment

qmonnet Jun 26, 2023

Choose a reason for hiding this comment

derailed Jun 26, 2023

Choose a reason for hiding this comment

qmonnet commented Jun 26, 2023

qmonnet commented Jun 26, 2023

derailed left a comment

Choose a reason for hiding this comment

qmonnet commented Jun 26, 2023

qmonnet Jun 26, 2023 •

edited

qmonnet commented Jun 26, 2023 •

edited