Merge pull request #10756 from milas/network-race

up: fix race condition on network connect

pkg/compose/convergence.go

@@ -29,7 +29,6 @@ import (
 	"github.com/containerd/containerd/platforms"
 	moby "github.com/docker/docker/api/types"
 	containerType "github.com/docker/docker/api/types/container"
-	"github.com/docker/docker/api/types/network"
 	specs "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -233,7 +232,13 @@ func (c *convergence) ensureService(ctx context.Context, project *types.Project,
 		name := getContainerName(project.Name, service, number)
 		i := i
 		eg.Go(func() error {
-			container, err := c.service.createContainer(ctx, project, service, name, number, false, true, false)
+			opts := createOptions{
+				AutoRemove:        false,
+				AttachStdin:       false,
+				UseNetworkAliases: true,
+				Labels:            mergeLabels(service.Labels, service.CustomLabels),
+			}
+			container, err := c.service.createContainer(ctx, project, service, name, number, opts)
 			updated[actual+i] = container
 			return err
 		})
@@ -399,12 +404,11 @@ func getScale(config types.ServiceConfig) (int, error) {
 }
 
 func (s *composeService) createContainer(ctx context.Context, project *types.Project, service types.ServiceConfig,
-	name string, number int, autoRemove bool, useNetworkAliases bool, attachStdin bool) (container moby.Container, err error) {
+	name string, number int, opts createOptions) (container moby.Container, err error) {
 	w := progress.ContextWriter(ctx)
 	eventName := "Container " + name
 	w.Event(progress.CreatingEvent(eventName))
-	container, err = s.createMobyContainer(ctx, project, service, name, number, nil,
-		autoRemove, useNetworkAliases, attachStdin, w, mergeLabels(service.Labels, service.CustomLabels))
+	container, err = s.createMobyContainer(ctx, project, service, name, number, nil, opts, w)
 	if err != nil {
 		return
 	}
@@ -429,9 +433,13 @@ func (s *composeService) recreateContainer(ctx context.Context, project *types.P
 	}
 	name := getContainerName(project.Name, service, number)
 	tmpName := fmt.Sprintf("%s_%s", replaced.ID[:12], name)
-	created, err = s.createMobyContainer(ctx, project, service, tmpName, number, inherited,
-		false, true, false, w,
-		mergeLabels(service.Labels, service.CustomLabels).Add(api.ContainerReplaceLabel, replaced.ID))
+	opts := createOptions{
+		AutoRemove:        false,
+		AttachStdin:       false,
+		UseNetworkAliases: true,
+		Labels:            mergeLabels(service.Labels, service.CustomLabels).Add(api.ContainerReplaceLabel, replaced.ID),
+	}
+	created, err = s.createMobyContainer(ctx, project, service, tmpName, number, inherited, opts, w)
 	if err != nil {
 		return created, err
 	}
@@ -484,19 +492,18 @@ func (s *composeService) startContainer(ctx context.Context, container moby.Cont
 	return nil
 }
 
-func (s *composeService) createMobyContainer(ctx context.Context, //nolint:gocyclo
+func (s *composeService) createMobyContainer(ctx context.Context,
 	project *types.Project,
 	service types.ServiceConfig,
 	name string,
 	number int,
 	inherit *moby.Container,
-	autoRemove, useNetworkAliases, attachStdin bool,
+	opts createOptions,
 	w progress.Writer,
-	labels types.Labels,
 ) (moby.Container, error) {
 	var created moby.Container
-	containerConfig, hostConfig, networkingConfig, err := s.getCreateOptions(ctx, project, service, number, inherit,
-		autoRemove, attachStdin, labels)
+	cfgs, err := s.getCreateConfigs(ctx, project, service, number, inherit, opts)
+
 	if err != nil {
 		return created, err
 	}
@@ -514,18 +521,7 @@ func (s *composeService) createMobyContainer(ctx context.Context, //nolint:gocyc
 		plat = &p
 	}
 
-	links, err := s.getLinks(ctx, project.Name, service, number)
-	if err != nil {
-		return created, err
-	}
-	if networkingConfig != nil {
-		for k, s := range networkingConfig.EndpointsConfig {
-			s.Links = links
-			networkingConfig.EndpointsConfig[k] = s
-		}
-	}
-
-	response, err := s.apiClient().ContainerCreate(ctx, containerConfig, hostConfig, networkingConfig, plat, name)
+	response, err := s.apiClient().ContainerCreate(ctx, cfgs.Container, cfgs.Host, cfgs.Network, plat, name)
 	if err != nil {
 		return created, err
 	}
@@ -548,29 +544,19 @@ func (s *composeService) createMobyContainer(ctx context.Context, //nolint:gocyc
 			Networks: inspectedContainer.NetworkSettings.Networks,
 		},
 	}
-	for _, netName := range service.NetworksByPriority() {
-		netwrk := project.Networks[netName]
-		cfg := service.Networks[netName]
-		aliases := []string{getContainerName(project.Name, service, number)}
-		if useNetworkAliases {
-			aliases = append(aliases, service.Name)
-			if cfg != nil {
-				aliases = append(aliases, cfg.Aliases...)
-			}
-		}
-		if val, ok := created.NetworkSettings.Networks[netwrk.Name]; ok {
-			if shortIDAliasExists(created.ID, val.Aliases...) {
-				continue
-			}
-			err = s.apiClient().NetworkDisconnect(ctx, netwrk.Name, created.ID, false)
-			if err != nil {
+
+	// the highest-priority network is the primary and is included in the ContainerCreate API
+	// 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
 			}
 		}
-		err = s.connectContainerToNetwork(ctx, created.ID, netwrk.Name, cfg, links, aliases...)
-		if err != nil {
-			return created, err
-		}
 	}
 
 	err = s.injectSecrets(ctx, project, service, created.ID)
@@ -635,43 +621,6 @@ func (s *composeService) getLinks(ctx context.Context, projectName string, servi
 	return links, nil
 }
 
-func shortIDAliasExists(containerID string, aliases ...string) bool {
-	for _, alias := range aliases {
-		if alias == containerID[:12] {
-			return true
-		}
-	}
-	return false
-}
-
-func (s *composeService) connectContainerToNetwork(ctx context.Context, id string, netwrk string, cfg *types.ServiceNetworkConfig, links []string, aliases ...string) error {
-	var (
-		ipv4Address string
-		ipv6Address string
-		ipam        *network.EndpointIPAMConfig
-	)
-	if cfg != nil {
-		ipv4Address = cfg.Ipv4Address
-		ipv6Address = cfg.Ipv6Address
-		ipam = &network.EndpointIPAMConfig{
-			IPv4Address:  ipv4Address,
-			IPv6Address:  ipv6Address,
-			LinkLocalIPs: cfg.LinkLocalIPs,
-		}
-	}
-	err := s.apiClient().NetworkConnect(ctx, netwrk, id, &network.EndpointSettings{
-		Aliases:           aliases,
-		IPAddress:         ipv4Address,
-		GlobalIPv6Address: ipv6Address,
-		Links:             links,
-		IPAMConfig:        ipam,
-	})
-	if err != nil {
-		return err
-	}
-	return nil
-}
-
 func (s *composeService) isServiceHealthy(ctx context.Context, containers Containers, fallbackRunning bool) (bool, error) {
 	for _, c := range containers {
 		container, err := s.apiClient().ContainerInspect(ctx, c.ID)

pkg/compose/create.go

@@ -48,6 +48,20 @@ import (
 	"github.com/docker/compose/v2/pkg/utils"
 )
 
+type createOptions struct {
+	AutoRemove        bool
+	AttachStdin       bool
+	UseNetworkAliases bool
+	Labels            types.Labels
+}
+
+type createConfigs struct {
+	Container *container.Config
+	Host      *container.HostConfig
+	Network   *network.NetworkingConfig
+	Links     []string
+}
+
 func (s *composeService) Create(ctx context.Context, project *types.Project, options api.CreateOptions) error {
 	return progress.RunWithTitle(ctx, func(ctx context.Context) error {
 		return s.create(ctx, project, options)
@@ -166,18 +180,16 @@ func (s *composeService) ensureProjectVolumes(ctx context.Context, project *type
 	return nil
 }
 
-func (s *composeService) getCreateOptions(ctx context.Context,
+func (s *composeService) getCreateConfigs(ctx context.Context,
 	p *types.Project,
 	service types.ServiceConfig,
 	number int,
 	inherit *moby.Container,
-	autoRemove, attachStdin bool,
-	labels types.Labels,
-) (*container.Config, *container.HostConfig, *network.NetworkingConfig, error) {
-
-	labels, err := s.prepareLabels(labels, service, number)
+	opts createOptions,
+) (createConfigs, error) {
+	labels, err := s.prepareLabels(opts.Labels, service, number)
 	if err != nil {
-		return nil, nil, nil, err
+		return createConfigs{}, err
 	}
 
 	var (
@@ -196,11 +208,6 @@ func (s *composeService) getCreateOptions(ctx context.Context,
 		stdinOpen = service.StdinOpen
 	)
 
-	binds, mounts, err := s.buildContainerVolumes(ctx, *p, service, inherit)
-	if err != nil {
-		return nil, nil, nil, err
-	}
-
 	proxyConfig := types.MappingWithEquals(s.configFile().ParseProxyConfig(s.apiClient().DaemonHost(), nil))
 	env := proxyConfig.OverrideBy(service.Environment)
 
@@ -211,8 +218,8 @@ func (s *composeService) getCreateOptions(ctx context.Context,
 		ExposedPorts:    buildContainerPorts(service),
 		Tty:             tty,
 		OpenStdin:       stdinOpen,
-		StdinOnce:       attachStdin && stdinOpen,
-		AttachStdin:     attachStdin,
+		StdinOnce:       opts.AttachStdin && stdinOpen,
+		AttachStdin:     opts.AttachStdin,
 		AttachStderr:    true,
 		AttachStdout:    true,
 		Cmd:             runCmd,
@@ -228,20 +235,7 @@ func (s *composeService) getCreateOptions(ctx context.Context,
 		StopTimeout:     ToSeconds(service.StopGracePeriod),
 	}
 
-	portBindings := buildContainerPortBindingOptions(service)
-
-	resources := getDeployResources(service)
-
-	if service.NetworkMode == "" {
-		service.NetworkMode = getDefaultNetworkMode(p, service)
-	}
-
-	var networkConfig *network.NetworkingConfig
-	for _, id := range service.NetworksByPriority() {
-		networkConfig = s.createNetworkConfig(p, service, id)
-		break
-	}
-
+	// VOLUMES/MOUNTS/FILESYSTEMS
 	tmpfs := map[string]string{}
 	for _, t := range service.Tmpfs {
 		if arr := strings.SplitN(t, ":", 2); len(arr) > 1 {
@@ -250,39 +244,47 @@ func (s *composeService) getCreateOptions(ctx context.Context,
 			tmpfs[arr[0]] = ""
 		}
 	}
-
-	var logConfig container.LogConfig
-	if service.Logging != nil {
-		logConfig = container.LogConfig{
-			Type:   service.Logging.Driver,
-			Config: service.Logging.Options,
-		}
+	binds, mounts, err := s.buildContainerVolumes(ctx, *p, service, inherit)
+	if err != nil {
+		return createConfigs{}, err
 	}
-
 	var volumesFrom []string
 	for _, v := range service.VolumesFrom {
 		if !strings.HasPrefix(v, "container:") {
-			return nil, nil, nil, fmt.Errorf("invalid volume_from: %s", v)
+			return createConfigs{}, fmt.Errorf("invalid volume_from: %s", v)
 		}
 		volumesFrom = append(volumesFrom, v[len("container:"):])
 	}
 
+	// NETWORKING
 	links, err := s.getLinks(ctx, p.Name, service, number)
 	if err != nil {
-		return nil, nil, nil, err
+		return createConfigs{}, err
 	}
+	networkMode, networkingConfig := defaultNetworkSettings(p, service, number, links, opts.UseNetworkAliases)
+	portBindings := buildContainerPortBindingOptions(service)
 
+	// MISC
+	resources := getDeployResources(service)
+	var logConfig container.LogConfig
+	if service.Logging != nil {
+		logConfig = container.LogConfig{
+			Type:   service.Logging.Driver,
+			Config: service.Logging.Options,
+		}
+	}
 	securityOpts, unconfined, err := parseSecurityOpts(p, service.SecurityOpt)
 	if err != nil {
-		return nil, nil, nil, err
+		return createConfigs{}, err
 	}
+
 	hostConfig := container.HostConfig{
-		AutoRemove:     autoRemove,
+		AutoRemove:     opts.AutoRemove,
 		Binds:          binds,
 		Mounts:         mounts,
 		CapAdd:         strslice.StrSlice(service.CapAdd),
 		CapDrop:        strslice.StrSlice(service.CapDrop),
-		NetworkMode:    container.NetworkMode(service.NetworkMode),
+		NetworkMode:    networkMode,
 		Init:           service.Init,
 		IpcMode:        container.IpcMode(service.Ipc),
 		CgroupnsMode:   container.CgroupnsMode(service.Cgroup),
@@ -317,12 +319,28 @@ func (s *composeService) getCreateOptions(ctx context.Context,
 		hostConfig.ReadonlyPaths = []string{}
 	}
 
-	return &containerConfig, &hostConfig, networkConfig, nil
+	cfgs := createConfigs{
+		Container: &containerConfig,
+		Host:      &hostConfig,
+		Network:   networkingConfig,
+		Links:     links,
+	}
+	return cfgs, nil
+}
+
+func getAliases(project *types.Project, service types.ServiceConfig, serviceIndex int, networkKey string, useNetworkAliases bool) []string {
+	aliases := []string{getContainerName(project.Name, service, serviceIndex)}
+	if useNetworkAliases {
+		aliases = append(aliases, service.Name)
+		if cfg := service.Networks[networkKey]; cfg != nil {
+			aliases = append(aliases, cfg.Aliases...)
+		}
+	}
+	return aliases
 }
 
-func (s *composeService) createNetworkConfig(p *types.Project, service types.ServiceConfig, networkID string) *network.NetworkingConfig {
-	net := p.Networks[networkID]
-	config := service.Networks[networkID]
+func createEndpointSettings(p *types.Project, service types.ServiceConfig, serviceIndex int, networkKey string, links []string, useNetworkAliases bool) *network.EndpointSettings {
+	config := service.Networks[networkKey]
 	var ipam *network.EndpointIPAMConfig
 	var (
 		ipv4Address string
@@ -337,15 +355,12 @@ func (s *composeService) createNetworkConfig(p *types.Project, service types.Ser
 			LinkLocalIPs: config.LinkLocalIPs,
 		}
 	}
-	return &network.NetworkingConfig{
-		EndpointsConfig: map[string]*network.EndpointSettings{
-			net.Name: {
-				Aliases:     getAliases(service, config),
-				IPAddress:   ipv4Address,
-				IPv6Gateway: ipv6Address,
-				IPAMConfig:  ipam,
-			},
-		},
+	return &network.EndpointSettings{
+		Aliases:     getAliases(p, service, serviceIndex, networkKey, useNetworkAliases),
+		Links:       links,
+		IPAddress:   ipv4Address,
+		IPv6Gateway: ipv6Address,
+		IPAMConfig:  ipam,
 	}
 }
 
@@ -404,17 +419,39 @@ func (s *composeService) prepareLabels(labels types.Labels, service types.Servic
 	return labels, nil
 }
 
-func getDefaultNetworkMode(project *types.Project, service types.ServiceConfig) string {
+// defaultNetworkSettings determines the container.NetworkMode and corresponding network.NetworkingConfig (nil if not applicable).
+func defaultNetworkSettings(
+	project *types.Project,
+	service types.ServiceConfig,
+	serviceIndex int,
+	links []string,
+	useNetworkAliases bool,
+) (container.NetworkMode, *network.NetworkingConfig) {
+	if service.NetworkMode != "" {
+		return container.NetworkMode(service.NetworkMode), nil
+	}
+
 	if len(project.Networks) == 0 {
-		return "none"
+		return "none", nil
 	}
 
+	var networkKey string
 	if len(service.Networks) > 0 {
-		name := service.NetworksByPriority()[0]
-		return project.Networks[name].Name
+		networkKey = service.NetworksByPriority()[0]
+	} else {
+		networkKey = "default"
 	}
-
-	return project.Networks["default"].Name
+	mobyNetworkName := project.Networks[networkKey].Name
+	epSettings := createEndpointSettings(project, service, serviceIndex, networkKey, links, useNetworkAliases)
+	networkConfig := &network.NetworkingConfig{
+		EndpointsConfig: map[string]*network.EndpointSettings{
+			mobyNetworkName: epSettings,
+		},
+	}
+	// From the Engine API docs:
+	// > Supported standard values are: bridge, host, none, and container:<name|id>.
+	// > Any other value is taken as a custom network's name to which this container should connect to.
+	return container.NetworkMode(mobyNetworkName), networkConfig
 }
 
 func getRestartPolicy(service types.ServiceConfig) container.RestartPolicy {
@@ -1002,14 +1039,6 @@ func buildTmpfsOptions(tmpfs *types.ServiceVolumeTmpfs) *mount.TmpfsOptions {
 	}
 }
 
-func getAliases(s types.ServiceConfig, c *types.ServiceNetworkConfig) []string {
-	aliases := []string{s.Name}
-	if c != nil {
-		aliases = append(aliases, c.Aliases...)
-	}
-	return aliases
-}
-
 func (s *composeService) ensureNetwork(ctx context.Context, n *types.NetworkConfig) error {
 	if n.External.External {
 		return s.resolveExternalNetwork(ctx, n)

pkg/compose/create_test.go

@@ -22,6 +22,8 @@ import (
 	"sort"
 	"testing"
 
+	"gotest.tools/v3/assert/cmp"
+
 	"github.com/docker/compose/v2/pkg/api"
 
 	composetypes "github.com/compose-spec/compose-go/types"
@@ -203,7 +205,7 @@ func TestBuildContainerMountOptions(t *testing.T) {
 	assert.Equal(t, mounts[2].Target, "\\\\.\\pipe\\docker_engine")
 }
 
-func TestGetDefaultNetworkMode(t *testing.T) {
+func TestDefaultNetworkSettings(t *testing.T) {
 	t.Run("returns the network with the highest priority when service has multiple networks", func(t *testing.T) {
 		service := composetypes.ServiceConfig{
 			Name: "myService",
@@ -231,7 +233,10 @@ func TestGetDefaultNetworkMode(t *testing.T) {
 			}),
 		}
 
-		assert.Equal(t, getDefaultNetworkMode(&project, service), "myProject_myNetwork2")
+		networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true)
+		assert.Equal(t, string(networkMode), "myProject_myNetwork2")
+		assert.Check(t, cmp.Len(networkConfig.EndpointsConfig, 1))
+		assert.Check(t, cmp.Contains(networkConfig.EndpointsConfig, "myProject_myNetwork2"))
 	})
 
 	t.Run("returns default network when service has no networks", func(t *testing.T) {
@@ -256,7 +261,10 @@ func TestGetDefaultNetworkMode(t *testing.T) {
 			}),
 		}
 
-		assert.Equal(t, getDefaultNetworkMode(&project, service), "myProject_default")
+		networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true)
+		assert.Equal(t, string(networkMode), "myProject_default")
+		assert.Check(t, cmp.Len(networkConfig.EndpointsConfig, 1))
+		assert.Check(t, cmp.Contains(networkConfig.EndpointsConfig, "myProject_default"))
 	})
 
 	t.Run("returns none if project has no networks", func(t *testing.T) {
@@ -270,6 +278,28 @@ func TestGetDefaultNetworkMode(t *testing.T) {
 			},
 		}
 
-		assert.Equal(t, getDefaultNetworkMode(&project, service), "none")
+		networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true)
+		assert.Equal(t, string(networkMode), "none")
+		assert.Check(t, cmp.Nil(networkConfig))
+	})
+
+	t.Run("returns defined network mode if explicitly set", func(t *testing.T) {
+		service := composetypes.ServiceConfig{
+			Name:        "myService",
+			NetworkMode: "host",
+		}
+		project := composetypes.Project{
+			Name:     "myProject",
+			Services: []composetypes.ServiceConfig{service},
+			Networks: composetypes.Networks(map[string]composetypes.NetworkConfig{
+				"default": {
+					Name: "myProject_default",
+				},
+			}),
+		}
+
+		networkMode, networkConfig := defaultNetworkSettings(&project, service, 1, nil, true)
+		assert.Equal(t, string(networkMode), "host")
+		assert.Check(t, cmp.Nil(networkConfig))
 	})
 }

pkg/compose/run.go

@@ -99,8 +99,14 @@ func (s *composeService) prepareRun(ctx context.Context, project *types.Project,
 			return "", err
 		}
 	}
-	created, err := s.createContainer(ctx, project, service, service.ContainerName, 1,
-		opts.AutoRemove, opts.UseNetworkAliases, opts.Interactive)
+	createOpts := createOptions{
+		AutoRemove:        opts.AutoRemove,
+		AttachStdin:       opts.Interactive,
+		UseNetworkAliases: opts.UseNetworkAliases,
+		Labels:            mergeLabels(service.Labels, service.CustomLabels),
+	}
+
+	created, err := s.createContainer(ctx, project, service, service.ContainerName, 1, createOpts)
 	if err != nil {
 		return "", err
 	}