detect network conflict as name is not guaranteed to be unique (#10612)

Signed-off-by: Nicolas De Loof <[email protected]>

pkg/compose/convergence.go

@@ -484,7 +484,7 @@ func (s *composeService) startContainer(ctx context.Context, container moby.Cont
 	return nil
 }
 
-func (s *composeService) createMobyContainer(ctx context.Context,
+func (s *composeService) createMobyContainer(ctx context.Context, //nolint:gocyclo
 	project *types.Project,
 	service types.ServiceConfig,
 	name string,
@@ -513,6 +513,18 @@ func (s *composeService) createMobyContainer(ctx context.Context,
 		}
 		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)
 	if err != nil {
 		return created, err
@@ -536,10 +548,6 @@ func (s *composeService) createMobyContainer(ctx context.Context,
 			Networks: inspectedContainer.NetworkSettings.Networks,
 		},
 	}
-	links, err := s.getLinks(ctx, project.Name, service, number)
-	if err != nil {
-		return created, err
-	}
 	for _, netName := range service.NetworksByPriority() {
 		netwrk := project.Networks[netName]
 		cfg := service.Networks[netName]

pkg/compose/create.go

@@ -143,11 +143,12 @@ func prepareNetworks(project *types.Project) {
 }
 
 func (s *composeService) ensureNetworks(ctx context.Context, networks types.Networks) error {
-	for _, network := range networks {
-		err := s.ensureNetwork(ctx, network)
+	for i, network := range networks {
+		err := s.ensureNetwork(ctx, &network)
 		if err != nil {
 			return err
 		}
+		networks[i] = network
 	}
 	return nil
 }
@@ -1009,98 +1010,168 @@ func getAliases(s types.ServiceConfig, c *types.ServiceNetworkConfig) []string {
 	return aliases
 }
 
-func (s *composeService) ensureNetwork(ctx context.Context, n types.NetworkConfig) error {
-	// NetworkInspect will match on ID prefix, so NetworkList with a name
-	// filter is used to look for an exact match to prevent e.g. a network
-	// named `db` from getting erroneously matched to a network with an ID
-	// like `db9086999caf`
+func (s *composeService) ensureNetwork(ctx context.Context, n *types.NetworkConfig) error {
+	if n.External.External {
+		return s.resolveExternalNetwork(ctx, n)
+	}
+
+	err := s.resolveOrCreateNetwork(ctx, n)
+	if errdefs.IsConflict(err) {
+		// Maybe another execution of `docker compose up|run` created same network
+		// let's retry once
+		return s.resolveOrCreateNetwork(ctx, n)
+	}
+	return err
+}
+
+func (s *composeService) resolveOrCreateNetwork(ctx context.Context, n *types.NetworkConfig) error { //nolint:gocyclo
+	expectedNetworkLabel := n.Labels[api.NetworkLabel]
+	expectedProjectLabel := n.Labels[api.ProjectLabel]
+
+	// First, try to find a unique network matching by name or ID
+	inspect, err := s.apiClient().NetworkInspect(ctx, n.Name, moby.NetworkInspectOptions{})
+	if err == nil {
+		// NetworkInspect will match on ID prefix, so double check we get the expected one
+		// as looking for network named `db` we could erroneously matched network ID `db9086999caf`
+		if inspect.Name == n.Name || inspect.ID == n.Name {
+			if inspect.Labels[api.ProjectLabel] != expectedProjectLabel {
+				logrus.Warnf("a network with name %s exists but was not created by compose.\n"+
+					"Set `external: true` to use an existing network", n.Name)
+			}
+			if inspect.Labels[api.NetworkLabel] != expectedNetworkLabel {
+				return fmt.Errorf("network %s was found but has incorrect label %s set to %q", n.Name, api.NetworkLabel, inspect.Labels[api.NetworkLabel])
+			}
+			return nil
+		}
+	}
+	// ignore other errors. Typically, an ambiguous request by name results in some generic `invalidParameter` error
+
+	// Either not found, or name is ambiguous - use NetworkList to list by name
 	networks, err := s.apiClient().NetworkList(ctx, moby.NetworkListOptions{
 		Filters: filters.NewArgs(filters.Arg("name", n.Name)),
 	})
 	if err != nil {
 		return err
 	}
-	networkNotFound := true
+
+	// NetworkList Matches all or part of a network name, so we have to filter for a strict match
+	networks = utils.Filter(networks, func(net moby.NetworkResource) bool {
+		return net.Name == n.Name
+	})
+
 	for _, net := range networks {
-		if net.Name == n.Name {
-			networkNotFound = false
-			break
+		if net.Labels[api.ProjectLabel] == expectedProjectLabel &&
+			net.Labels[api.NetworkLabel] == expectedNetworkLabel {
+			return nil
 		}
 	}
-	if networkNotFound {
-		if n.External.External {
-			if n.Driver == "overlay" {
-				// Swarm nodes do not register overlay networks that were
-				// created on a different node unless they're in use.
-				// Here we assume `driver` is relevant for a network we don't manage
-				// which is a non-sense, but this is our legacy ¯\(ツ)/¯
-				// networkAttach will later fail anyway if network actually doesn't exists
-				enabled, err := s.isSWarmEnabled(ctx)
-				if err != nil {
-					return err
-				}
-				if enabled {
-					return nil
-				}
-			}
-			return fmt.Errorf("network %s declared as external, but could not be found", n.Name)
-		}
-		var ipam *network.IPAM
-		if n.Ipam.Config != nil {
-			var config []network.IPAMConfig
-			for _, pool := range n.Ipam.Config {
-				config = append(config, network.IPAMConfig{
-					Subnet:     pool.Subnet,
-					IPRange:    pool.IPRange,
-					Gateway:    pool.Gateway,
-					AuxAddress: pool.AuxiliaryAddresses,
-				})
-			}
-			ipam = &network.IPAM{
-				Driver: n.Ipam.Driver,
-				Config: config,
-			}
+
+	// we could have set NetworkList with a projectFilter and networkFilter but not doing so allows to catch this
+	// scenario were a network with same name exists but doesn't have label, and use of `CheckDuplicate: true`
+	// prevents to create another one.
+	if len(networks) > 0 {
+		logrus.Warnf("a network with name %s exists but was not created by compose.\n"+
+			"Set `external: true` to use an existing network", n.Name)
+		return nil
+	}
+
+	var ipam *network.IPAM
+	if n.Ipam.Config != nil {
+		var config []network.IPAMConfig
+		for _, pool := range n.Ipam.Config {
+			config = append(config, network.IPAMConfig{
+				Subnet:     pool.Subnet,
+				IPRange:    pool.IPRange,
+				Gateway:    pool.Gateway,
+				AuxAddress: pool.AuxiliaryAddresses,
+			})
 		}
-		createOpts := moby.NetworkCreate{
-			CheckDuplicate: true,
-			// TODO NameSpace Labels
-			Labels:     n.Labels,
-			Driver:     n.Driver,
-			Options:    n.DriverOpts,
-			Internal:   n.Internal,
-			Attachable: n.Attachable,
-			IPAM:       ipam,
-			EnableIPv6: n.EnableIPv6,
+		ipam = &network.IPAM{
+			Driver: n.Ipam.Driver,
+			Config: config,
 		}
+	}
+	createOpts := moby.NetworkCreate{
+		CheckDuplicate: true,
+		Labels:         n.Labels,
+		Driver:         n.Driver,
+		Options:        n.DriverOpts,
+		Internal:       n.Internal,
+		Attachable:     n.Attachable,
+		IPAM:           ipam,
+		EnableIPv6:     n.EnableIPv6,
+	}
 
-		if n.Ipam.Driver != "" || len(n.Ipam.Config) > 0 {
-			createOpts.IPAM = &network.IPAM{}
-		}
+	if n.Ipam.Driver != "" || len(n.Ipam.Config) > 0 {
+		createOpts.IPAM = &network.IPAM{}
+	}
 
-		if n.Ipam.Driver != "" {
-			createOpts.IPAM.Driver = n.Ipam.Driver
+	if n.Ipam.Driver != "" {
+		createOpts.IPAM.Driver = n.Ipam.Driver
+	}
+
+	for _, ipamConfig := range n.Ipam.Config {
+		config := network.IPAMConfig{
+			Subnet:     ipamConfig.Subnet,
+			IPRange:    ipamConfig.IPRange,
+			Gateway:    ipamConfig.Gateway,
+			AuxAddress: ipamConfig.AuxiliaryAddresses,
 		}
+		createOpts.IPAM.Config = append(createOpts.IPAM.Config, config)
+	}
+	networkEventName := fmt.Sprintf("Network %s", n.Name)
+	w := progress.ContextWriter(ctx)
+	w.Event(progress.CreatingEvent(networkEventName))
+
+	_, err = s.apiClient().NetworkCreate(ctx, n.Name, createOpts)
+	if err != nil {
+		w.Event(progress.ErrorEvent(networkEventName))
+		return errors.Wrapf(err, "failed to create network %s", n.Name)
+	}
+	w.Event(progress.CreatedEvent(networkEventName))
+	return nil
+}
+
+func (s *composeService) resolveExternalNetwork(ctx context.Context, n *types.NetworkConfig) error {
+	// NetworkInspect will match on ID prefix, so NetworkList with a name
+	// filter is used to look for an exact match to prevent e.g. a network
+	// named `db` from getting erroneously matched to a network with an ID
+	// like `db9086999caf`
+	networks, err := s.apiClient().NetworkList(ctx, moby.NetworkListOptions{
+		Filters: filters.NewArgs(filters.Arg("name", n.Name)),
+	})
+	if err != nil {
+		return err
+	}
+
+	// NetworkList API doesn't return the exact name match, so we can retrieve more than one network with a request
+	networks = utils.Filter(networks, func(net moby.NetworkResource) bool {
+		return net.Name == n.Name
+	})
 
-		for _, ipamConfig := range n.Ipam.Config {
-			config := network.IPAMConfig{
-				Subnet:     ipamConfig.Subnet,
-				IPRange:    ipamConfig.IPRange,
-				Gateway:    ipamConfig.Gateway,
-				AuxAddress: ipamConfig.AuxiliaryAddresses,
+	switch len(networks) {
+	case 1:
+		n.Name = networks[0].ID
+		return nil
+	case 0:
+		if n.Driver == "overlay" {
+			// Swarm nodes do not register overlay networks that were
+			// created on a different node unless they're in use.
+			// Here we assume `driver` is relevant for a network we don't manage
+			// which is a non-sense, but this is our legacy ¯\(ツ)/¯
+			// networkAttach will later fail anyway if network actually doesn't exists
+			enabled, err := s.isSWarmEnabled(ctx)
+			if err != nil {
+				return err
+			}
+			if enabled {
+				return nil
 			}
-			createOpts.IPAM.Config = append(createOpts.IPAM.Config, config)
-		}
-		networkEventName := fmt.Sprintf("Network %s", n.Name)
-		w := progress.ContextWriter(ctx)
-		w.Event(progress.CreatingEvent(networkEventName))
-		if _, err := s.apiClient().NetworkCreate(ctx, n.Name, createOpts); err != nil {
-			w.Event(progress.ErrorEvent(networkEventName))
-			return errors.Wrapf(err, "failed to create network %s", n.Name)
 		}
-		w.Event(progress.CreatedEvent(networkEventName))
-		return nil
+		return fmt.Errorf("network %s declared as external, but could not be found", n.Name)
+	default:
+		return fmt.Errorf("multiple networks with name %q were found. Use network ID as `name` to avoid ambiguity", n.Name)
 	}
-	return nil
 }
 
 func (s *composeService) ensureVolume(ctx context.Context, volume types.VolumeConfig, project string) error {

pkg/compose/down.go

@@ -171,32 +171,30 @@ func (s *composeService) ensureImagesDown(ctx context.Context, project *types.Pr
 
 func (s *composeService) ensureNetworksDown(ctx context.Context, project *types.Project, w progress.Writer) []downOp {
 	var ops []downOp
-	for _, n := range project.Networks {
+	for key, n := range project.Networks {
 		if n.External.External {
 			continue
 		}
 		// loop capture variable for op closure
-		networkName := n.Name
+		networkKey := key
+		idOrName := n.Name
 		ops = append(ops, func() error {
-			return s.removeNetwork(ctx, networkName, w)
+			return s.removeNetwork(ctx, networkKey, project.Name, idOrName, w)
 		})
 	}
 	return ops
 }
 
-func (s *composeService) removeNetwork(ctx context.Context, name string, w progress.Writer) error {
-	// networks are guaranteed to have unique IDs but NOT names, so it's
-	// possible to get into a situation where a compose down will fail with
-	// an error along the lines of:
-	// 	failed to remove network test: Error response from daemon: network test is ambiguous (2 matches found based on name)
-	// as a workaround here, the delete is done by ID after doing a list using
-	// the name as a filter (99.9% of the time this will return a single result)
+func (s *composeService) removeNetwork(ctx context.Context, composeNetworkName string, projectName string, name string, w progress.Writer) error {
 	networks, err := s.apiClient().NetworkList(ctx, moby.NetworkListOptions{
-		Filters: filters.NewArgs(filters.Arg("name", name)),
+		Filters: filters.NewArgs(
+			projectFilter(projectName),
+			networkFilter(composeNetworkName)),
 	})
 	if err != nil {
-		return errors.Wrapf(err, fmt.Sprintf("failed to inspect network %s", name))
+		return errors.Wrapf(err, "failed to list networks")
 	}
+
 	if len(networks) == 0 {
 		return nil
 	}

pkg/compose/down_test.go

@@ -60,8 +60,8 @@ func TestDown(t *testing.T) {
 	// cleanup properly if duplicates are inadvertently created
 	api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{Filters: filters.NewArgs(projectFilter(strings.ToLower(testProject)))}).
 		Return([]moby.NetworkResource{
-			{ID: "abc123", Name: "myProject_default"},
-			{ID: "def456", Name: "myProject_default"},
+			{ID: "abc123", Name: "myProject_default", Labels: map[string]string{compose.NetworkLabel: "default"}},
+			{ID: "def456", Name: "myProject_default", Labels: map[string]string{compose.NetworkLabel: "default"}},
 		}, nil)
 
 	stopOptions := containerType.StopOptions{}
@@ -74,7 +74,9 @@ func TestDown(t *testing.T) {
 	api.EXPECT().ContainerRemove(gomock.Any(), "789", moby.ContainerRemoveOptions{Force: true}).Return(nil)
 
 	api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{
-		Filters: filters.NewArgs(filters.Arg("name", "myProject_default")),
+		Filters: filters.NewArgs(
+			projectFilter(strings.ToLower(testProject)),
+			networkFilter("default")),
 	}).Return([]moby.NetworkResource{
 		{ID: "abc123", Name: "myProject_default"},
 		{ID: "def456", Name: "myProject_default"},
@@ -106,7 +108,11 @@ func TestDownRemoveOrphans(t *testing.T) {
 	api.EXPECT().VolumeList(gomock.Any(), filters.NewArgs(projectFilter(strings.ToLower(testProject)))).
 		Return(volume.ListResponse{}, nil)
 	api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{Filters: filters.NewArgs(projectFilter(strings.ToLower(testProject)))}).
-		Return([]moby.NetworkResource{{Name: "myProject_default"}}, nil)
+		Return([]moby.NetworkResource{
+			{
+				Name:   "myProject_default",
+				Labels: map[string]string{compose.NetworkLabel: "default"},
+			}}, nil)
 
 	stopOptions := containerType.StopOptions{}
 	api.EXPECT().ContainerStop(gomock.Any(), "123", stopOptions).Return(nil)
@@ -118,7 +124,10 @@ func TestDownRemoveOrphans(t *testing.T) {
 	api.EXPECT().ContainerRemove(gomock.Any(), "321", moby.ContainerRemoveOptions{Force: true}).Return(nil)
 
 	api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{
-		Filters: filters.NewArgs(filters.Arg("name", "myProject_default")),
+		Filters: filters.NewArgs(
+			networkFilter("default"),
+			projectFilter(strings.ToLower(testProject)),
+		),
 	}).Return([]moby.NetworkResource{{ID: "abc123", Name: "myProject_default"}}, nil)
 	api.EXPECT().NetworkInspect(gomock.Any(), "abc123", gomock.Any()).Return(moby.NetworkResource{ID: "abc123"}, nil)
 	api.EXPECT().NetworkRemove(gomock.Any(), "abc123").Return(nil)

pkg/compose/filters.go

@@ -31,6 +31,10 @@ func serviceFilter(serviceName string) filters.KeyValuePair {
 	return filters.Arg("label", fmt.Sprintf("%s=%s", api.ServiceLabel, serviceName))
 }
 
+func networkFilter(name string) filters.KeyValuePair {
+	return filters.Arg("label", fmt.Sprintf("%s=%s", api.NetworkLabel, name))
+}
+
 func oneOffFilter(b bool) filters.KeyValuePair {
 	v := "False"
 	if b {

pkg/utils/slices.go

@@ -39,3 +39,13 @@ func Remove[T any](origin []T, elements ...T) []T {
 	}
 	return filtered
 }
+
+func Filter[T any](elements []T, predicate func(T) bool) []T {
+	var filtered []T
+	for _, v := range elements {
+		if predicate(v) {
+			filtered = append(filtered, v)
+		}
+	}
+	return filtered
+}