From 10b290e6826267a4a0d6e0ee008a3645203bdbe8 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Wed, 28 Jun 2023 11:27:58 -0400 Subject: [PATCH] up: fix race condition on network connect Engine API only allows at most one network to be connected as part of the ContainerCreate API request. Compose will pick the highest priority network. Afterwards, the remaining networks (if any) are connected before the container is actually started. The big change here is that, previously, the highest-priority network was connected in the create, and then disconnected and immediately reconnected along with all the others. This was racy because evidently connecting the container to the network as part of the create isn't synchronous, so sometimes when Compose tried to disconnect it, the API would return an error like: ``` container is not connected to the network ``` To avoid needing to disconnect and immediately reconnect, the network config logic has been refactored to ensure that it sets up the network config correctly the first time. Signed-off-by: Milas Bowman --- pkg/compose/convergence.go | 113 +++++++----------------- pkg/compose/create.go | 173 ++++++++++++++++++++++--------------- pkg/compose/create_test.go | 38 +++++++- pkg/compose/run.go | 10 ++- 4 files changed, 174 insertions(+), 160 deletions(-) diff --git a/pkg/compose/convergence.go b/pkg/compose/convergence.go index 68589ce7e..c009d0825 100644 --- a/pkg/compose/convergence.go +++ b/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) diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 17256b5a7..57e1ca656 100644 --- a/pkg/compose/create.go +++ b/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,7 +244,28 @@ func (s *composeService) getCreateOptions(ctx context.Context, tmpfs[arr[0]] = "" } } + 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 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 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{ @@ -258,31 +273,18 @@ func (s *composeService) getCreateOptions(ctx context.Context, Config: service.Logging.Options, } } - - var volumesFrom []string - for _, v := range service.VolumesFrom { - if !strings.HasPrefix(v, "container:") { - return nil, nil, nil, fmt.Errorf("invalid volume_from: %s", v) - } - volumesFrom = append(volumesFrom, v[len("container:"):]) - } - - links, err := s.getLinks(ctx, p.Name, service, number) - if err != nil { - return nil, nil, nil, err - } - 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 (s *composeService) createNetworkConfig(p *types.Project, service types.ServiceConfig, networkID string) *network.NetworkingConfig { - net := p.Networks[networkID] - config := service.Networks[networkID] +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 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:. + // > 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) diff --git a/pkg/compose/create_test.go b/pkg/compose/create_test.go index c4b84ed11..9939d51fc 100644 --- a/pkg/compose/create_test.go +++ b/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)) }) } diff --git a/pkg/compose/run.go b/pkg/compose/run.go index 22cd913c9..3297eab47 100644 --- a/pkg/compose/run.go +++ b/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 }