From 0f747419b6773e0305a5aaebe8b78abb9b4bffad Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Tue, 21 Jun 2022 16:25:42 -0400 Subject: [PATCH 1/2] networks: prevent issues due to duplicate names Within the Docker API/engine, networks have unique IDs, and the name is a friendly label/alias, which notably does NOT have any guarantees around uniqueness (see moby/moby#18864 [^1]). During day-to-day interactive/CLI Compose usage, this is rarely an issue, as Compose itself isn't creating networks concurrently across goroutines. However, if multiple Compose instances are executed simultaneously (e.g. as part of a test suite that runs in parallel), this can easily occur. When it does happen, it's very confusing for users and resolving it via the `docker` CLI is not straightforward either [^2]. There's two primary changes here: * Pass `CheckDuplicates: true` to the Docker API when creating networks to reduce the likelihood of Compose creating duplicates in the first place * On `down`, list networks using a name filter and then remove them all by ID, as the Docker API will return an error if the name alias is used and maps to more than one network Hopefully, this provides a better UX, since the issue should be less likely to occur, and if it does, it can now be resolved via standard Compose workflow commands. [^1]: https://github.com/moby/moby/issues/18864 [^2]: https://stackoverflow.com/questions/63776518/error-2-matches-found-based-on-name-network-nameofservice-default-is-ambiguo Signed-off-by: Milas Bowman --- pkg/compose/create.go | 14 +---------- pkg/compose/down.go | 53 +++++++++++++++++++++++++++++++++++----- pkg/compose/down_test.go | 23 ++++++++++++----- 3 files changed, 65 insertions(+), 25 deletions(-) diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 0c6bf8067..9f33dbc3a 100644 --- a/pkg/compose/create.go +++ b/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 { diff --git a/pkg/compose/down.go b/pkg/compose/down.go index 0130f5a58..b04ac1087 100644 --- a/pkg/compose/down.go +++ b/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,12 +132,8 @@ 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) }) @@ -144,6 +141,50 @@ func (s *composeService) ensureNetworksDown(ctx context.Context, project *types. 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 { diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index c799975cc..0568c72ed 100644 --- a/pkg/compose/down_test.go +++ b/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) { @@ -59,8 +60,16 @@ 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) + // 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(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 +103,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) From 5f89365c8fa44a2263e24803c27260a7ec6e664e Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Wed, 22 Jun 2022 16:34:55 -0400 Subject: [PATCH 2/2] network: make test mock consistent throughout Signed-off-by: Milas Bowman --- pkg/compose/down_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index 0568c72ed..ea9527939 100644 --- a/pkg/compose/down_test.go +++ b/pkg/compose/down_test.go @@ -49,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) @@ -60,8 +66,6 @@ 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) - // 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(filters.Arg("name", "myProject_default")), }).Return([]moby.NetworkResource{