From 1bd8a773a783512b93de6b938173959a4886a081 Mon Sep 17 00:00:00 2001 From: Nicolas De loof Date: Wed, 31 May 2023 20:46:23 +0200 Subject: [PATCH] detect network conflict as name is not guaranteed to be unique (#10612) Signed-off-by: Nicolas De Loof --- pkg/compose/convergence.go | 18 ++- pkg/compose/create.go | 231 ++++++++++++++++++++++++------------- pkg/compose/down.go | 22 ++-- pkg/compose/down_test.go | 19 ++- pkg/compose/filters.go | 4 + pkg/utils/slices.go | 10 ++ 6 files changed, 202 insertions(+), 102 deletions(-) diff --git a/pkg/compose/convergence.go b/pkg/compose/convergence.go index ca5ade075..68589ce7e 100644 --- a/pkg/compose/convergence.go +++ b/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] diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 14a3ee8bc..5984d4457 100644 --- a/pkg/compose/create.go +++ b/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,7 +1010,129 @@ func getAliases(s types.ServiceConfig, c *types.ServiceNetworkConfig) []string { return aliases } -func (s *composeService) ensureNetwork(ctx context.Context, n types.NetworkConfig) error { +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 + } + + // 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.Labels[api.ProjectLabel] == expectedProjectLabel && + net.Labels[api.NetworkLabel] == expectedNetworkLabel { + return nil + } + } + + // 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, + }) + } + 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 != "" { + 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 @@ -1020,87 +1143,35 @@ func (s *composeService) ensureNetwork(ctx context.Context, n types.NetworkConfi if err != nil { return err } - networkNotFound := true - for _, net := range networks { - if net.Name == n.Name { - networkNotFound = false - break - } - } - 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, - } - } - 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, - } - if n.Ipam.Driver != "" || len(n.Ipam.Config) > 0 { - createOpts.IPAM = &network.IPAM{} - } + // 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 + }) - 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)) - 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)) + 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 + } + } + 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 { diff --git a/pkg/compose/down.go b/pkg/compose/down.go index 23ea4e43e..3f5910c6e 100644 --- a/pkg/compose/down.go +++ b/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 } diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index 78f5be280..2cdd449d6 100644 --- a/pkg/compose/down_test.go +++ b/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) diff --git a/pkg/compose/filters.go b/pkg/compose/filters.go index 5114a4c81..d6c814977 100644 --- a/pkg/compose/filters.go +++ b/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 { diff --git a/pkg/utils/slices.go b/pkg/utils/slices.go index 6e39c8d37..4459cbd59 100644 --- a/pkg/utils/slices.go +++ 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 +}