From a1a559c695bca7cc5b9a93af4fa732fe0c79cd30 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Thu, 6 Jul 2023 16:28:54 -0400 Subject: [PATCH] network: fix random missing network when service has more than one As part of the fix for #10668, the logic was adjusted so that the default (highest-priority) network is used in the `ContainerCreate`, and then the remaining networks are connected via calls to `NetworkConnect` before starting the container. Unfortunately, `ServiceConfig::NetworksByPriority` is neither deterministic nor stable when networks have the same priority. It's non-deterministic because the order of networks from parsing YAML is random, since they are loaded into a Go map (which have random iteration order). Additionally, it's not using a `SortStable` in `compose-go`, so even if the load order was predictable, it still might produce different results. While I look at improving `compose-go` here to prevent this from tripping us up in the future, this fix looks at _all_ networks for a service and ignores the "default" one now. Before, it would always skip the first one in the slice since that _should_ have been the "default". Signed-off-by: Milas Bowman --- pkg/compose/convergence.go | 16 +++++++----- pkg/e2e/networks_test.go | 53 ++++++++++++++------------------------ 2 files changed, 29 insertions(+), 40 deletions(-) diff --git a/pkg/compose/convergence.go b/pkg/compose/convergence.go index c009d0825e..5243e89f9a 100644 --- a/pkg/compose/convergence.go +++ b/pkg/compose/convergence.go @@ -549,13 +549,15 @@ func (s *composeService) createMobyContainer(ctx context.Context, // call via container.NetworkMode & network.NetworkingConfig // any remaining networks are connected one-by-one here after creation (but before start) serviceNetworks := service.NetworksByPriority() - if len(serviceNetworks) > 1 { - for _, networkKey := range serviceNetworks[1:] { - mobyNetworkName := project.Networks[networkKey].Name - epSettings := createEndpointSettings(project, service, number, networkKey, cfgs.Links, opts.UseNetworkAliases) - if err := s.apiClient().NetworkConnect(ctx, mobyNetworkName, created.ID, epSettings); err != nil { - return created, err - } + for _, networkKey := range serviceNetworks { + mobyNetworkName := project.Networks[networkKey].Name + if string(cfgs.Host.NetworkMode) == mobyNetworkName { + // primary network already configured as part of ContainerCreate + continue + } + epSettings := createEndpointSettings(project, service, number, networkKey, cfgs.Links, opts.UseNetworkAliases) + if err := s.apiClient().NetworkConnect(ctx, mobyNetworkName, created.ID, epSettings); err != nil { + return created, err } } diff --git a/pkg/e2e/networks_test.go b/pkg/e2e/networks_test.go index 9264c76cf8..37c4fb9a22 100644 --- a/pkg/e2e/networks_test.go +++ b/pkg/e2e/networks_test.go @@ -29,47 +29,34 @@ import ( func TestNetworks(t *testing.T) { // fixture is shared with TestNetworkModes and is not safe to run concurrently - c := NewCLI(t) - const projectName = "network-e2e" + c := NewCLI(t, WithEnv( + "COMPOSE_PROJECT_NAME="+projectName, + "COMPOSE_FILE=./fixtures/network-test/compose.yaml", + )) - t.Run("ensure we do not reuse previous networks", func(t *testing.T) { - c.RunDockerOrExitError(t, "network", "rm", projectName+"-dbnet") - c.RunDockerOrExitError(t, "network", "rm", "microservices") - }) - - t.Run("up", func(t *testing.T) { - c.RunDockerComposeCmd(t, "-f", "./fixtures/network-test/compose.yaml", "--project-name", projectName, "up", - "-d") - }) + c.RunDockerComposeCmd(t, "down", "-t0", "-v") - t.Run("check running project", func(t *testing.T) { - res := c.RunDockerComposeCmd(t, "-p", projectName, "ps") - res.Assert(t, icmd.Expected{Out: `web`}) + c.RunDockerComposeCmd(t, "up", "-d") - endpoint := "http://localhost:80" - output := HTTPGetWithRetry(t, endpoint+"/words/noun", http.StatusOK, 2*time.Second, 20*time.Second) - assert.Assert(t, strings.Contains(output, `"word":`)) + res := c.RunDockerComposeCmd(t, "ps") + res.Assert(t, icmd.Expected{Out: `web`}) - res = c.RunDockerCmd(t, "network", "ls") - res.Assert(t, icmd.Expected{Out: projectName + "_dbnet"}) - res.Assert(t, icmd.Expected{Out: "microservices"}) - }) + endpoint := "http://localhost:80" + output := HTTPGetWithRetry(t, endpoint+"/words/noun", http.StatusOK, 2*time.Second, 20*time.Second) + assert.Assert(t, strings.Contains(output, `"word":`)) - t.Run("port", func(t *testing.T) { - res := c.RunDockerComposeCmd(t, "--project-name", projectName, "port", "words", "8080") - res.Assert(t, icmd.Expected{Out: `0.0.0.0:8080`}) - }) + res = c.RunDockerCmd(t, "network", "ls") + res.Assert(t, icmd.Expected{Out: projectName + "_dbnet"}) + res.Assert(t, icmd.Expected{Out: "microservices"}) - t.Run("down", func(t *testing.T) { - _ = c.RunDockerComposeCmd(t, "--project-name", projectName, "down") - }) + res = c.RunDockerComposeCmd(t, "port", "words", "8080") + res.Assert(t, icmd.Expected{Out: `0.0.0.0:8080`}) - t.Run("check networks after down", func(t *testing.T) { - res := c.RunDockerCmd(t, "network", "ls") - assert.Assert(t, !strings.Contains(res.Combined(), projectName), res.Combined()) - assert.Assert(t, !strings.Contains(res.Combined(), "microservices"), res.Combined()) - }) + c.RunDockerComposeCmd(t, "down", "-t0", "-v") + res = c.RunDockerCmd(t, "network", "ls") + assert.Assert(t, !strings.Contains(res.Combined(), projectName), res.Combined()) + assert.Assert(t, !strings.Contains(res.Combined(), "microservices"), res.Combined()) } func TestNetworkAliases(t *testing.T) {