Merge pull request docker#9585 from milas/net-ambiguity

networks: prevent issues due to duplicate names

Author: glours

pkg/compose/create.go

@@ -1070,6 +1070,7 @@ func (s *composeService) ensureNetwork(ctx context.Context, n types.NetworkConfi
 			}
 		}
 		createOpts := moby.NetworkCreate{
+			CheckDuplicate: true,
 			// TODO NameSpace Labels
 			Labels:     n.Labels,
 			Driver:     n.Driver,
@@ -1110,19 +1111,6 @@ func (s *composeService) ensureNetwork(ctx context.Context, n types.NetworkConfi
 	return nil
 }
 
-func (s *composeService) removeNetwork(ctx context.Context, network string, w progress.Writer) error {
-	eventName := fmt.Sprintf("Network %s", network)
-	w.Event(progress.RemovingEvent(eventName))
-
-	if err := s.apiClient().NetworkRemove(ctx, network); err != nil {
-		w.Event(progress.ErrorEvent(eventName))
-		return errors.Wrapf(err, fmt.Sprintf("failed to remove network %s", network))
-	}
-
-	w.Event(progress.RemovedEvent(eventName))
-	return nil
-}
-
 func (s *composeService) ensureVolume(ctx context.Context, volume types.VolumeConfig, project string) error {
 	inspected, err := s.apiClient().VolumeInspect(ctx, volume.Name)
 	if err != nil {

pkg/compose/down.go

@@ -23,9 +23,10 @@ import (
 	"time"
 
 	"github.com/compose-spec/compose-go/types"
-	"github.com/docker/cli/cli/registry/client"
 	moby "github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/errdefs"
+	"github.com/pkg/errors"
 	"golang.org/x/sync/errgroup"
 
 	"github.com/docker/compose/v2/pkg/api"
@@ -131,19 +132,59 @@ func (s *composeService) ensureNetworksDown(ctx context.Context, project *types.
 		if n.External.External {
 			continue
 		}
+		// loop capture variable for op closure
 		networkName := n.Name
-		_, err := s.apiClient().NetworkInspect(ctx, networkName, moby.NetworkInspectOptions{})
-		if client.IsNotFound(err) {
-			return nil
-		}
-
 		ops = append(ops, func() error {
 			return s.removeNetwork(ctx, networkName, 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)
+	networks, err := s.apiClient().NetworkList(ctx, moby.NetworkListOptions{
+		Filters: filters.NewArgs(filters.Arg("name", name)),
+	})
+	if err != nil {
+		return errors.Wrapf(err, fmt.Sprintf("failed to inspect network %s", name))
+	}
+	if len(networks) == 0 {
+		return nil
+	}
+
+	eventName := fmt.Sprintf("Network %s", name)
+	w.Event(progress.RemovingEvent(eventName))
+
+	var removed int
+	for _, net := range networks {
+		if err := s.apiClient().NetworkRemove(ctx, net.ID); err != nil {
+			if errdefs.IsNotFound(err) {
+				continue
+			}
+			w.Event(progress.ErrorEvent(eventName))
+			return errors.Wrapf(err, fmt.Sprintf("failed to remove network %s", name))
+		}
+		removed++
+	}
+
+	if removed == 0 {
+		// in practice, it's extremely unlikely for this to ever occur, as it'd
+		// mean the network was present when we queried at the start of this
+		// method but was then deleted by something else in the interim
+		w.Event(progress.NewEvent(eventName, progress.Done, "Warning: No resource found to remove"))
+		return nil
+	}
+
+	w.Event(progress.RemovedEvent(eventName))
+	return nil
+}
+
 func (s *composeService) getServiceImages(options api.DownOptions, project *types.Project) map[string]struct{} {
 	images := map[string]struct{}{}
 	for _, service := range project.Services {

pkg/compose/down_test.go

@@ -21,13 +21,14 @@ import (
 	"strings"
 	"testing"
 
-	compose "github.com/docker/compose/v2/pkg/api"
-	"github.com/docker/compose/v2/pkg/mocks"
 	moby "github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/api/types/volume"
 	"github.com/golang/mock/gomock"
 	"gotest.tools/v3/assert"
+
+	compose "github.com/docker/compose/v2/pkg/api"
+	"github.com/docker/compose/v2/pkg/mocks"
 )
 
 func TestDown(t *testing.T) {
@@ -48,8 +49,14 @@ func TestDown(t *testing.T) {
 		}, nil)
 	api.EXPECT().VolumeList(gomock.Any(), filters.NewArgs(projectFilter(strings.ToLower(testProject)))).
 		Return(volume.VolumeListOKBody{}, nil)
+
+	// network names are not guaranteed to be unique, ensure Compose handles
+	// cleanup properly if duplicates are inadvertently created
 	api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{Filters: filters.NewArgs(projectFilter(strings.ToLower(testProject)))}).
-		Return([]moby.NetworkResource{{Name: "myProject_default"}}, nil)
+		Return([]moby.NetworkResource{
+			{ID: "abc123", Name: "myProject_default"},
+			{ID: "def456", Name: "myProject_default"},
+		}, nil)
 
 	api.EXPECT().ContainerStop(gomock.Any(), "123", nil).Return(nil)
 	api.EXPECT().ContainerStop(gomock.Any(), "456", nil).Return(nil)
@@ -59,8 +66,14 @@ func TestDown(t *testing.T) {
 	api.EXPECT().ContainerRemove(gomock.Any(), "456", moby.ContainerRemoveOptions{Force: true}).Return(nil)
 	api.EXPECT().ContainerRemove(gomock.Any(), "789", moby.ContainerRemoveOptions{Force: true}).Return(nil)
 
-	api.EXPECT().NetworkInspect(gomock.Any(), "myProject_default", moby.NetworkInspectOptions{}).Return(moby.NetworkResource{Name: "myProject_default"}, nil)
-	api.EXPECT().NetworkRemove(gomock.Any(), "myProject_default").Return(nil)
+	api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{
+		Filters: filters.NewArgs(filters.Arg("name", "myProject_default")),
+	}).Return([]moby.NetworkResource{
+		{ID: "abc123", Name: "myProject_default"},
+		{ID: "def456", Name: "myProject_default"},
+	}, nil)
+	api.EXPECT().NetworkRemove(gomock.Any(), "abc123").Return(nil)
+	api.EXPECT().NetworkRemove(gomock.Any(), "def456").Return(nil)
 
 	err := tested.Down(context.Background(), strings.ToLower(testProject), compose.DownOptions{})
 	assert.NilError(t, err)
@@ -94,8 +107,10 @@ func TestDownRemoveOrphans(t *testing.T) {
 	api.EXPECT().ContainerRemove(gomock.Any(), "789", moby.ContainerRemoveOptions{Force: true}).Return(nil)
 	api.EXPECT().ContainerRemove(gomock.Any(), "321", moby.ContainerRemoveOptions{Force: true}).Return(nil)
 
-	api.EXPECT().NetworkInspect(gomock.Any(), "myProject_default", moby.NetworkInspectOptions{}).Return(moby.NetworkResource{Name: "myProject_default"}, nil)
-	api.EXPECT().NetworkRemove(gomock.Any(), "myProject_default").Return(nil)
+	api.EXPECT().NetworkList(gomock.Any(), moby.NetworkListOptions{
+		Filters: filters.NewArgs(filters.Arg("name", "myProject_default")),
+	}).Return([]moby.NetworkResource{{ID: "abc123", Name: "myProject_default"}}, nil)
+	api.EXPECT().NetworkRemove(gomock.Any(), "abc123").Return(nil)
 
 	err := tested.Down(context.Background(), strings.ToLower(testProject), compose.DownOptions{RemoveOrphans: true})
 	assert.NilError(t, err)