Ignore kernel-assigned LL addrs when selecting "bip6"#49022
Ignore kernel-assigned LL addrs when selecting "bip6"#49022thaJeztah merged 3 commits intomoby:masterfrom
Conversation
robmry
added
area/networking
robmry
force-pushed
the
ignore_kernel_ll_for_fcidrv6
branch
5 times, most recently
from
21253ae to
b78c41d
Compare
These tests create iptables rules for different addresses on docker0 but, unlike tests that do that for user-defined bridges, those rules aren't removed when the test deletes the network, because the default bridge network can't be deleted. So, use (abuse) the L3Segment code to run the tests in their own network namespace. Signed-off-by: Rob Murray <[email protected]>
Env var DOCKER_TEST_CREATE_DEFAULT_BRIDGE could be set to override the name of the default bridge - without the bridge being user-managed (unlike the '--bridge' daemon option). It was needed by tests looking at docker0 behaviour, using their own instance of the daemon, without breaking the docker0 instance belonging to CI's daemon. Now, those tests run in their own netns using the name docker0. So, remove the unused env var. Signed-off-by: Rob Murray <[email protected]>
robmry
force-pushed
the
ignore_kernel_ll_for_fcidrv6
branch
from
b78c41d to
b4f3ba0
Compare
akerouanton
left a comment
akerouanton
left a comment
There was a problem hiding this comment.
There's an extra debug file that need to go away 😉 Otherwise, LGTM.
| host.Do(t, func() { | ||
| // Run without OTel because there's no routing from this netns for it - which | ||
| // means the daemon doesn't shut down cleanly, causing the test to fail. | ||
| d := daemon.New(t, daemon.WithEnvVars("OTEL_EXPORTER_OTLP_ENDPOINT=")) |
There was a problem hiding this comment.
Maybe we should create a new util function in github.com/docker/docker/testutil/daemon to disable OTel. I think it'll become more common to do that as we're planning to update unit tests to use L3Segments for better test isolation.
There was a problem hiding this comment.
Yes, or maybe we can have the L3Segment stuff set up a default route so that the daemon can work (more) normally in its own netns. It needs more thought.
There was a problem hiding this comment.
Would this be something we could control through the ctx := testutil.StartSpan(baseContext, t) utility? It looks like that checks if Otel is configured in the environment; perhaps that could also take an argument to run t.SetEnv() for the test (and restore it after). Not sure if t.SetEnv would be problematic if we want to run things in parallel though.
(definitely in favour of having an option for this, so that we don't have to know about specific env-vars everywhere)
Commit facb232 aligned the way the default bridge's IPv6 subnet and gateway addresses are selected with IPv4. Part of that involved looking at addresses already on the bridge, along with daemon config options. But, for IPv6, the kernel will assign a link-local address to the bridge. Make sure that address is ignored when selecting "bip6" when it's not explicitly specified. This is made slightly complicated because we allow fixed-cidr-v6 to be a link-local subnet (either the standard "fe80::/64", or any other non-overlapping LL subnet in "fe80::/10"). Following this change, if fixed-cidr-v6 is (or is included by) "fe80::/64", the bridge's kernel-assigned LL address may be used as the network's gateway address - even though it may also get an IPAM-assigned LL address. Signed-off-by: Rob Murray <[email protected]>
robmry
force-pushed
the
ignore_kernel_ll_for_fcidrv6
branch
from
b4f3ba0 to
56eb47c
Compare
|
Unrelated test failures ... |
| host.Do(t, func() { | ||
| // Run without OTel because there's no routing from this netns for it - which | ||
| // means the daemon doesn't shut down cleanly, causing the test to fail. | ||
| d := daemon.New(t, daemon.WithEnvVars("OTEL_EXPORTER_OTLP_ENDPOINT=")) |
There was a problem hiding this comment.
Would this be something we could control through the ctx := testutil.StartSpan(baseContext, t) utility? It looks like that checks if Otel is configured in the environment; perhaps that could also take an argument to run t.SetEnv() for the test (and restore it after). Not sure if t.SetEnv would be problematic if we want to run things in parallel though.
(definitely in favour of having an option for this, so that we don't have to know about specific env-vars everywhere)
| // link-local addresses, unless fixed-cidr-v6 has the standard link-local | ||
| // prefix. (If fixed-cidr-v6 is the standard LL prefix, the kernel-assigned | ||
| // address is likely to be used instead of an IPAM assigned address.) | ||
| if family == netlink.FAMILY_V6 && (fCidrIP == nil || !isStandardLL(fCidrIP)) { |
There was a problem hiding this comment.
Nit (not need to fix); looks like the fCidrIP == nil is redundant here, because isStandardLL already returns false if it's nil
3 participants
- What I did
Noticed that, sometimes (under conditions I've not managed to repro reliably) when enabling IPv6 on the default bridge without specifying a
fixed-cidr-v6,docker0wasn't assigned an address from the daemon's implicit ULA subnet - it only got an IPv4 address and the kernel-assigned link local address.Commit facb232 (#48319 - not shipped) aligned the way the default bridge's IPv6 subnet and gateway addresses are selected with IPv4.
Part of that involved looking at addresses already on the bridge, along with daemon config options. But, for IPv6, the kernel will assign a link-local address to the bridge - and that was confusing things.
- How I did it
Run tests that change docker0 in their own netns
These tests create iptables rules for different addresses on
docker0but, unlike tests that do that for user-defined bridges, those rules aren't removed when the test deletes the network, because the default bridge network can't be deleted.Because of this, adding tests for LL addresses in the third commit in this PR caused
TestAccessPublishedPortFromHostto fail - it uses a custom bridge and wasn't affected by the daemon change in this PR, but NAT rules left behind by the new tests broke it.So, use (abuse) the
L3Segmentcode to run the tests in their own network namespace.A minor drawback is that
TestDaemonDefaultBridgeWithFixedCidrButNoBipcan no longer run in rootless mode, because theL3Segmentcan't be created in rootlesskit's netns. But the test isn't looking at anything specific to rootless mode.Remove test env var DOCKER_TEST_CREATE_DEFAULT_BRIDGE
Env var
DOCKER_TEST_CREATE_DEFAULT_BRIDGEcould be set to override the name of the default bridge - without the bridge being user-managed (unlike the--bridgedaemon option).It was needed by tests looking at docker0 behaviour, using their own instance of the daemon, without breaking the
docker0 instance belonging to CI's daemon. Now, those tests run in their own netns using the name docker0.
So, remove the unused env var.
Ignore kernel-assigned LL addrs when selecting "bip6"
Make sure the kernel-assigned LL address is ignored when selecting "bip6" (when bip6 is not explicitly specified).
This is made slightly complicated because we allow
fixed-cidr-v6to be a link-local subnet (either the standard "fe80::/64", or any other non-overlapping LL subnet in "fe80::/10").Following this change, if fixed-cidr-v6 is (or is included by) "fe80::/64", the bridge's kernel-assigned LL address may be used as the network's gateway address - even though it may also get an IPAM-assigned LL address.
- How to verify it
Updated the integration tests to make sure the bridge always has a kernel-assigned LL address - and added test cases for link-local subnets.
Without the fix, two of the updated tests fail ...
- Description for the changelog