integration: Add a new networking integration test suite by akerouanton · Pull Request #46531 · moby/moby

akerouanton · 2023-09-21T14:50:10Z

Slice of Add a new integration test suite checking network behaviors #45850, to make it easier to review

- What I did

This PR introduces a new integration test suite aimed at testing networking features like inter-container communication, network isolation, port mapping, what happens when the userland proxy is enabled or disabled, etc... and how these features interact with daemon-level and network-level parameters.

So far, there's pretty much no tests making sure our networks are working as expected. For instance, there's a few tests for port mapping, but they don't cover all cases (eg. IPv6, disabling the userland proxy, etc...), and a few tests check if a specific iptables rule exists, but that doesn't prevent that specific rule from be wrong in the first place.

As we're planning to refactor how iptables rules are written and change some of them to fix known security issues, we need a way to test all parameter combinations (at least, the important ones). So far, this was done by hand which is painful and time consuming. As such, this new test suite is foundational to upcoming work.

This test suite should help:

Assert networking security boundaries are working properly, and prevent regressions on security patches;
Discover what's missing to make the userland proxy disabled by default;
Give a canonical test plan for new port mappers;

Following tests are implemented in this PR:

Inter-container communication for internal and non-internal bridge networks, over IPv4 and * IPv6 (using ULAs or link-local addresses, SLAAC-assigned or dynamically allocated) ;
Inter-network communication for internal and non-internal bridge networks, over IPv4 and IPv6, is disallowed ;

- How to verify it

CI is green.

- A picture of a cute animal (not mandatory but encouraged)

integration/networking/bridge_test.go

corhere · 2023-09-21T15:36:10Z

integration/networking/bridge_test.go

+	const bridge1Opts = "bridge1"
+	const bridge2Opts = "bridge2"


Nit: define a struct type instead of using maps with a fixed set of keys. Types can be declared inside a function body, BTW.

Types can be declared inside a function body, BTW.

Yeah, I was hesitant on this one; didn't know what approach would be more idiomatic. Will fix that.

thaJeztah

LGTM

thaJeztah · 2023-09-21T20:07:50Z

DOH! Looks like there's some failures though. So the good news; "tests are testing", The bad news.. something's failing;

=== FAIL: amd64.integration.networking TestBridgeICC/IPv4_internal_network (8.64s)
    bridge_test.go:126: assertion failed: expression is false: strings.Contains(res.Stdout.String(), "1 packets transmitted, 1 packets received")
    bridge_test.go:128: Logs from ctr-icc-1-2:
    --- FAIL: TestBridgeICC/IPv4_internal_network (8.64s)

=== FAIL: amd64.integration.networking TestBridgeICC/IPv6_ULA_on_internal_network (8.66s)
    bridge_test.go:126: assertion failed: expression is false: strings.Contains(res.Stdout.String(), "1 packets transmitted, 1 packets received")
    bridge_test.go:128: Logs from ctr-icc-3-2:
    --- FAIL: TestBridgeICC/IPv6_ULA_on_internal_network (8.66s)

☝️ Also wondering if is.Contains(...) instead of string.Contains() would be useful for those (would it print the string it tried to match?)

akerouanton · 2023-10-17T12:25:51Z

integration/networking/bridge_test.go

+				bridge1Opts: []func(*types.NetworkCreate){network.WithInternal()},
+				bridge2Opts: []func(*types.NetworkCreate){network.WithInternal()},
+			},
+			stderr: "sendto: Network is unreachable",


@corhere @thaJeztah Since #46603 has been merged, the error for such case has changed from "no response" to "network is unreachable".

integration/networking/bridge_test.go

corhere · 2023-10-17T15:35:22Z

integration/networking/bridge_test.go

+			assert.Check(t, res.ExitCode == 0)
+			assert.Check(t, res.Stderr.Len() == 0)


Suggested change

assert.Check(t, res.ExitCode == 0)

assert.Check(t, res.Stderr.Len() == 0)

assert.Check(t, is.Equal(res.ExitCode, 0))

assert.Check(t, is.Equal(res.Stderr.Len(), 0))

This is kinda repetitive. Have you considered extracting these assertions to a test helper function?

integration/networking/bridge_test.go

akerouanton · 2023-10-30T10:07:26Z

@corhere I coalesced both TestBridgeICC and TestBridgeIPv6SLAACLLAddress together.

thaJeztah

LGTM, but left one tiny comment

(CI is currently happy, so once that's addressed we can bring this one in)

thaJeztah · 2023-11-02T13:02:03Z

integration/networking/main_test.go

 func TestMain(m *testing.M) {
 	shutdown := testutil.ConfigureTracing()
-	ctx, span := otel.Tracer("").Start(context.Background(), "integration/network.TestMain")
+	ctx, span := otel.Tracer("").Start(context.Background(), "integration/networking.TestMain")


I guess this was meant to be in the first commit 🙈

This commit introduces a new integration test suite aimed at testing networking features like inter-container communication, network isolation, port mapping, etc... and how they interact with daemon-level and network-level parameters. So far, there's pretty much no tests making sure our networks are well configured: 1. there're a few tests for port mapping, but they don't cover all use cases ; 2. there're a few tests that check if a specific iptables rule exist, but that doesn't prevent that specific iptables rule to be wrong in the first place. As we're planning to refactor how iptables rules are written, and change some of them to fix known security issues, we need a way to test all combinations of parameters. So far, this was done by hand, which is particularly painful and time consuming. As such, this new test suite is foundational to upcoming work. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

Following tests are implemented in this specific commit: - Inter-container communications for internal and non-internal bridge networks, over IPv4 and IPv6. - Inter-container communications using IPv6 link-local addresses for internal and non-internal bridge networks. - Inter-network communications for internal and non-internal bridge networks, over IPv4 and IPv6, are disallowed. Signed-off-by: Albin Kerouanton <albinker@gmail.com>

akerouanton added status/2-code-review area/networking Networking area/testing labels Sep 21, 2023

akerouanton requested a review from corhere September 21, 2023 14:50

corhere approved these changes Sep 21, 2023

View reviewed changes

akerouanton force-pushed the networking-suite-bridge-tests branch from 6ca3ccc to 71857cf Compare September 21, 2023 16:42

thaJeztah approved these changes Sep 21, 2023

View reviewed changes

akerouanton force-pushed the networking-suite-bridge-tests branch 11 times, most recently from e927e0f to e71b692 Compare September 23, 2023 14:10

akerouanton requested a review from tonistiigi as a code owner September 23, 2023 14:10

akerouanton force-pushed the networking-suite-bridge-tests branch 4 times, most recently from d2d07e8 to 65760ae Compare September 25, 2023 08:53

akerouanton requested a review from tianon as a code owner September 25, 2023 11:31

akerouanton force-pushed the networking-suite-bridge-tests branch 2 times, most recently from 1766768 to 9882c65 Compare September 25, 2023 12:47

This was referenced Oct 9, 2023

libnet/d/bridge: Don't set container's gateway when network is internal #46603

Merged

libnet: Don't forward to upstream resolvers on internal nw #46609

Closed

akerouanton force-pushed the networking-suite-bridge-tests branch from 9882c65 to ce61164 Compare October 12, 2023 12:25

akerouanton force-pushed the networking-suite-bridge-tests branch from ce61164 to e6a6c0b Compare October 12, 2023 13:21

akerouanton commented Oct 17, 2023

View reviewed changes

akerouanton requested review from corhere and thaJeztah October 17, 2023 12:25

corhere approved these changes Oct 17, 2023

View reviewed changes

akerouanton force-pushed the networking-suite-bridge-tests branch from e6a6c0b to ebd96ee Compare October 30, 2023 09:58

akerouanton requested a review from corhere October 30, 2023 10:07

akerouanton force-pushed the networking-suite-bridge-tests branch from ebd96ee to ddab566 Compare October 30, 2023 10:40

akerouanton self-assigned this Oct 31, 2023

thaJeztah approved these changes Nov 2, 2023

View reviewed changes

thaJeztah added this to the 25.0.0 milestone Nov 2, 2023

akerouanton added 2 commits November 3, 2023 09:58

akerouanton force-pushed the networking-suite-bridge-tests branch from ddab566 to c1ab6ed Compare November 3, 2023 08:59

thaJeztah merged commit 26c054e into moby:master Nov 3, 2023

akerouanton deleted the networking-suite-bridge-tests branch November 3, 2023 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integration: Add a new networking integration test suite #46531

integration: Add a new networking integration test suite #46531
thaJeztah merged 2 commits intomoby:masterfrom
akerouanton:networking-suite-bridge-tests

akerouanton commented Sep 21, 2023

Uh oh!

Uh oh!

Uh oh!

corhere Sep 21, 2023

Uh oh!

akerouanton Sep 21, 2023

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah commented Sep 21, 2023

Uh oh!

akerouanton Oct 17, 2023

Uh oh!

Uh oh!

corhere Oct 17, 2023

Uh oh!

Uh oh!

akerouanton commented Oct 30, 2023

Uh oh!

thaJeztah left a comment

Uh oh!

thaJeztah Nov 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		assert.Check(t, res.ExitCode == 0)
		assert.Check(t, res.Stderr.Len() == 0)

Conversation

akerouanton commented Sep 21, 2023

Uh oh!

Uh oh!

Uh oh!

corhere Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

akerouanton Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 21, 2023

Uh oh!

akerouanton Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corhere Oct 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akerouanton commented Oct 30, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants