From df9a679f908232a54a1fabfe98c79b2ddea7fa93 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Tue, 31 Oct 2017 19:46:53 +0000 Subject: [PATCH 1/2] Update `FindUniqueNetwork` to address network name duplications This fix is part of the effort to address 30242 where issue arise because of the fact that multiple networks may share the same name (within or across local/swarm scopes). The focus of this fix is to allow creation of service when a network in local scope has the same name as the service network. An integration test has been added. This fix fixes 30242. Signed-off-by: Yong Tang Upstream-commit: cafed80cd019a8b40025eaa5e5b37459362607fb Component: engine --- .../api/server/router/network/backend.go | 4 +- .../server/router/network/network_routes.go | 102 ++++++++++++++++-- .../engine/daemon/cluster/executor/backend.go | 4 +- .../cluster/executor/container/adapter.go | 10 +- .../cluster/executor/container/container.go | 15 +-- components/engine/daemon/cluster/networks.go | 2 +- .../engine/daemon/container_operations.go | 35 +++--- components/engine/daemon/network.go | 102 ++++++++++++------ .../integration-cli/docker_cli_swarm_test.go | 73 +++++++++++++ 9 files changed, 270 insertions(+), 77 deletions(-) diff --git a/components/engine/api/server/router/network/backend.go b/components/engine/api/server/router/network/backend.go index a32a0b9c00..8ae6a078b7 100644 --- a/components/engine/api/server/router/network/backend.go +++ b/components/engine/api/server/router/network/backend.go @@ -12,11 +12,11 @@ import ( // Backend is all the methods that need to be implemented // to provide network specific functionality. type Backend interface { - FindNetwork(idName string) (libnetwork.Network, error) + FindUniqueNetwork(idName string) (libnetwork.Network, error) GetNetworks() []libnetwork.Network CreateNetwork(nc types.NetworkCreateRequest) (*types.NetworkCreateResponse, error) ConnectContainerToNetwork(containerName, networkName string, endpointConfig *network.EndpointSettings) error DisconnectContainerFromNetwork(containerName string, networkName string, force bool) error - DeleteNetwork(name string) error + DeleteNetwork(networkID string) error NetworksPrune(ctx context.Context, pruneFilters filters.Args) (*types.NetworksPruneReport, error) } diff --git a/components/engine/api/server/router/network/network_routes.go b/components/engine/api/server/router/network/network_routes.go index a67bb9f0b2..497ce0a502 100644 --- a/components/engine/api/server/router/network/network_routes.go +++ b/components/engine/api/server/router/network/network_routes.go @@ -2,6 +2,7 @@ package network import ( "encoding/json" + "fmt" "net/http" "strconv" "strings" @@ -288,7 +289,12 @@ func (n *networkRouter) postNetworkConnect(ctx context.Context, w http.ResponseW return err } - return n.backend.ConnectContainerToNetwork(connect.Container, vars["id"], connect.EndpointConfig) + // Always make sure there is no ambiguity with respect to the network ID/name + nw, err := n.backend.FindUniqueNetwork(vars["id"]) + if err != nil { + return err + } + return n.backend.ConnectContainerToNetwork(connect.Container, nw.ID(), connect.EndpointConfig) } func (n *networkRouter) postNetworkDisconnect(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { @@ -312,15 +318,19 @@ func (n *networkRouter) deleteNetwork(ctx context.Context, w http.ResponseWriter if err := httputils.ParseForm(r); err != nil { return err } - if _, err := n.cluster.GetNetwork(vars["id"]); err == nil { - if err = n.cluster.RemoveNetwork(vars["id"]); err != nil { + + nw, err := n.findUniqueNetwork(vars["id"]) + if err != nil { + return err + } + if nw.Scope == "swarm" { + if err = n.cluster.RemoveNetwork(nw.ID); err != nil { + return err + } + } else { + if err := n.backend.DeleteNetwork(nw.ID); err != nil { return err } - w.WriteHeader(http.StatusNoContent) - return nil - } - if err := n.backend.DeleteNetwork(vars["id"]); err != nil { - return err } w.WriteHeader(http.StatusNoContent) return nil @@ -518,3 +528,79 @@ func (n *networkRouter) postNetworksPrune(ctx context.Context, w http.ResponseWr } return httputils.WriteJSON(w, http.StatusOK, pruneReport) } + +// findUniqueNetwork will search network across different scopes (both local and swarm). +// NOTE: This findUniqueNetwork is differnt from FindUniqueNetwork from the daemon. +// In case multiple networks have duplicate names, return error. +// First find based on full ID, return immediately once one is found. +// If a network appears both in swarm and local, assume it is in local first +// For full name and partial ID, save the result first, and process later +// in case multiple records was found based on the same term +// TODO (yongtang): should we wrap with version here for backward compatibility? +func (n *networkRouter) findUniqueNetwork(term string) (types.NetworkResource, error) { + listByFullName := map[string]types.NetworkResource{} + listByPartialID := map[string]types.NetworkResource{} + + nw := n.backend.GetNetworks() + for _, network := range nw { + if network.ID() == term { + return *n.buildDetailedNetworkResources(network, false), nil + + } + if network.Name() == term { + // No need to check the ID collision here as we are still in + // local scope and the network ID is unique in this scope. + listByFullName[network.ID()] = *n.buildDetailedNetworkResources(network, false) + } + if strings.HasPrefix(network.ID(), term) { + // No need to check the ID collision here as we are still in + // local scope and the network ID is unique in this scope. + listByPartialID[network.ID()] = *n.buildDetailedNetworkResources(network, false) + } + } + + nr, _ := n.cluster.GetNetworks() + for _, network := range nr { + if network.ID == term { + return network, nil + } + if network.Name == term { + // Check the ID collision as we are in swarm scope here, and + // the map (of the listByFullName) may have already had a + // network with the same ID (from local scope previously) + if _, ok := listByFullName[network.ID]; !ok { + listByFullName[network.ID] = network + } + } + if strings.HasPrefix(network.ID, term) { + // Check the ID collision as we are in swarm scope here, and + // the map (of the listByPartialID) may have already had a + // network with the same ID (from local scope previously) + if _, ok := listByPartialID[network.ID]; !ok { + listByPartialID[network.ID] = network + } + } + } + + // Find based on full name, returns true only if no duplicates + if len(listByFullName) == 1 { + for _, v := range listByFullName { + return v, nil + } + } + if len(listByFullName) > 1 { + return types.NetworkResource{}, fmt.Errorf("network %s is ambiguous (%d matches found based on name)", term, len(listByFullName)) + } + + // Find based on partial ID, returns true only if no duplicates + if len(listByPartialID) == 1 { + for _, v := range listByPartialID { + return v, nil + } + } + if len(listByPartialID) > 1 { + return types.NetworkResource{}, fmt.Errorf("network %s is ambiguous (%d matches found based on ID prefix)", term, len(listByPartialID)) + } + + return types.NetworkResource{}, libnetwork.ErrNoSuchNetwork(term) +} diff --git a/components/engine/daemon/cluster/executor/backend.go b/components/engine/daemon/cluster/executor/backend.go index f6763d15cd..ed8f50467d 100644 --- a/components/engine/daemon/cluster/executor/backend.go +++ b/components/engine/daemon/cluster/executor/backend.go @@ -27,8 +27,8 @@ import ( // Backend defines the executor component for a swarm agent. type Backend interface { CreateManagedNetwork(clustertypes.NetworkCreateRequest) error - DeleteManagedNetwork(name string) error - FindNetwork(idName string) (libnetwork.Network, error) + DeleteManagedNetwork(networkID string) error + FindUniqueNetwork(idName string) (libnetwork.Network, error) SetupIngress(clustertypes.NetworkCreateRequest, string) (<-chan struct{}, error) ReleaseIngress() (<-chan struct{}, error) PullImage(ctx context.Context, image, tag, platform string, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error diff --git a/components/engine/daemon/cluster/executor/container/adapter.go b/components/engine/daemon/cluster/executor/container/adapter.go index 81e740af13..7db47983cb 100644 --- a/components/engine/daemon/cluster/executor/container/adapter.go +++ b/components/engine/daemon/cluster/executor/container/adapter.go @@ -143,8 +143,8 @@ func (c *containerAdapter) pullImage(ctx context.Context) error { } func (c *containerAdapter) createNetworks(ctx context.Context) error { - for _, network := range c.container.networks() { - ncr, err := c.container.networkCreateRequest(network) + for name := range c.container.networksAttachments { + ncr, err := c.container.networkCreateRequest(name) if err != nil { return err } @@ -162,15 +162,15 @@ func (c *containerAdapter) createNetworks(ctx context.Context) error { } func (c *containerAdapter) removeNetworks(ctx context.Context) error { - for _, nid := range c.container.networks() { - if err := c.backend.DeleteManagedNetwork(nid); err != nil { + for name, v := range c.container.networksAttachments { + if err := c.backend.DeleteManagedNetwork(v.Network.ID); err != nil { switch err.(type) { case *libnetwork.ActiveEndpointsError: continue case libnetwork.ErrNoSuchNetwork: continue default: - log.G(ctx).Errorf("network %s remove failed: %v", nid, err) + log.G(ctx).Errorf("network %s remove failed: %v", name, err) return err } } diff --git a/components/engine/daemon/cluster/executor/container/container.go b/components/engine/daemon/cluster/executor/container/container.go index 4f41fb3e23..ad4ade0713 100644 --- a/components/engine/daemon/cluster/executor/container/container.go +++ b/components/engine/daemon/cluster/executor/container/container.go @@ -507,7 +507,7 @@ func getEndpointConfig(na *api.NetworkAttachment, b executorpkg.Backend) *networ DriverOpts: na.DriverAttachmentOpts, } if v, ok := na.Network.Spec.Annotations.Labels["com.docker.swarm.predefined"]; ok && v == "true" { - if ln, err := b.FindNetwork(na.Network.Spec.Annotations.Name); err == nil { + if ln, err := b.FindUniqueNetwork(na.Network.Spec.Annotations.Name); err == nil { n.NetworkID = ln.ID() } } @@ -575,19 +575,6 @@ func (c *containerConfig) serviceConfig() *clustertypes.ServiceConfig { return svcCfg } -// networks returns a list of network names attached to the container. The -// returned name can be used to lookup the corresponding network create -// options. -func (c *containerConfig) networks() []string { - var networks []string - - for name := range c.networksAttachments { - networks = append(networks, name) - } - - return networks -} - func (c *containerConfig) networkCreateRequest(name string) (clustertypes.NetworkCreateRequest, error) { na, ok := c.networksAttachments[name] if !ok { diff --git a/components/engine/daemon/cluster/networks.go b/components/engine/daemon/cluster/networks.go index 04582eb31e..3b01ea4efb 100644 --- a/components/engine/daemon/cluster/networks.go +++ b/components/engine/daemon/cluster/networks.go @@ -292,7 +292,7 @@ func (c *Cluster) populateNetworkID(ctx context.Context, client swarmapi.Control for i, n := range networks { apiNetwork, err := getNetwork(ctx, client, n.Target) if err != nil { - ln, _ := c.config.Backend.FindNetwork(n.Target) + ln, _ := c.config.Backend.FindUniqueNetwork(n.Target) if ln != nil && runconfig.IsPreDefinedNetwork(ln.Name()) { // Need to retrieve the corresponding predefined swarm network // and use its id for the request. diff --git a/components/engine/daemon/container_operations.go b/components/engine/daemon/container_operations.go index fc08f8263d..355d5496ea 100644 --- a/components/engine/daemon/container_operations.go +++ b/components/engine/daemon/container_operations.go @@ -251,8 +251,8 @@ func (daemon *Daemon) updateNetworkSettings(container *container.Container, n li return runconfig.ErrConflictHostNetwork } - for s := range container.NetworkSettings.Networks { - sn, err := daemon.FindNetwork(s) + for s, v := range container.NetworkSettings.Networks { + sn, err := daemon.FindUniqueNetwork(getNetworkID(s, v.EndpointSettings)) if err != nil { continue } @@ -308,8 +308,8 @@ func (daemon *Daemon) updateNetwork(container *container.Container) error { // Find if container is connected to the default bridge network var n libnetwork.Network - for name := range container.NetworkSettings.Networks { - sn, err := daemon.FindNetwork(name) + for name, v := range container.NetworkSettings.Networks { + sn, err := daemon.FindUniqueNetwork(getNetworkID(name, v.EndpointSettings)) if err != nil { continue } @@ -339,7 +339,7 @@ func (daemon *Daemon) updateNetwork(container *container.Container) error { } func (daemon *Daemon) findAndAttachNetwork(container *container.Container, idOrName string, epConfig *networktypes.EndpointSettings) (libnetwork.Network, *networktypes.NetworkingConfig, error) { - n, err := daemon.FindNetwork(idOrName) + n, err := daemon.FindUniqueNetwork(getNetworkID(idOrName, epConfig)) if err != nil { // We should always be able to find the network for a // managed container. @@ -377,16 +377,16 @@ func (daemon *Daemon) findAndAttachNetwork(container *container.Container, idOrN // trigger attachment in the swarm cluster manager. if daemon.clusterProvider != nil { var err error - config, err = daemon.clusterProvider.AttachNetwork(idOrName, container.ID, addresses) + config, err = daemon.clusterProvider.AttachNetwork(getNetworkID(idOrName, epConfig), container.ID, addresses) if err != nil { return nil, nil, err } } - n, err = daemon.FindNetwork(idOrName) + n, err = daemon.FindUniqueNetwork(getNetworkID(idOrName, epConfig)) if err != nil { if daemon.clusterProvider != nil { - if err := daemon.clusterProvider.DetachNetwork(idOrName, container.ID); err != nil { + if err := daemon.clusterProvider.DetachNetwork(getNetworkID(idOrName, epConfig), container.ID); err != nil { logrus.Warnf("Could not rollback attachment for container %s to network %s: %v", container.ID, idOrName, err) } } @@ -437,7 +437,7 @@ func (daemon *Daemon) updateContainerNetworkSettings(container *container.Contai if mode.IsUserDefined() { var err error - n, err = daemon.FindNetwork(networkName) + n, err = daemon.FindUniqueNetwork(networkName) if err == nil { networkName = n.Name() } @@ -797,7 +797,7 @@ func (daemon *Daemon) connectToNetwork(container *container.Container, idOrName // ForceEndpointDelete deletes an endpoint from a network forcefully func (daemon *Daemon) ForceEndpointDelete(name string, networkName string) error { - n, err := daemon.FindNetwork(networkName) + n, err := daemon.FindUniqueNetwork(networkName) if err != nil { return err } @@ -949,7 +949,7 @@ func (daemon *Daemon) releaseNetwork(container *container.Container) { var networks []libnetwork.Network for n, epSettings := range settings { - if nw, err := daemon.FindNetwork(n); err == nil { + if nw, err := daemon.FindUniqueNetwork(getNetworkID(n, epSettings.EndpointSettings)); err == nil { networks = append(networks, nw) } @@ -993,7 +993,7 @@ func (daemon *Daemon) ConnectToNetwork(container *container.Container, idOrName return errRemovalContainer(container.ID) } - n, err := daemon.FindNetwork(idOrName) + n, err := daemon.FindUniqueNetwork(idOrName) if err == nil && n != nil { if err := daemon.updateNetworkConfig(container, n, endpointConfig, true); err != nil { return err @@ -1016,7 +1016,7 @@ func (daemon *Daemon) ConnectToNetwork(container *container.Container, idOrName // DisconnectFromNetwork disconnects container from network n. func (daemon *Daemon) DisconnectFromNetwork(container *container.Container, networkName string, force bool) error { - n, err := daemon.FindNetwork(networkName) + n, err := daemon.FindUniqueNetwork(networkName) container.Lock() defer container.Unlock() @@ -1087,3 +1087,12 @@ func (daemon *Daemon) DeactivateContainerServiceBinding(containerName string) er } return sb.DisableService() } + +func getNetworkID(name string, endpointSettings *networktypes.EndpointSettings) string { + // We only want to prefer NetworkID for user defined networks. + // For systems like bridge, none, etc. the name is preferred (otherwise restart may cause issues) + if containertypes.NetworkMode(name).IsUserDefined() && endpointSettings != nil && endpointSettings.NetworkID != "" { + return endpointSettings.NetworkID + } + return name +} diff --git a/components/engine/daemon/network.go b/components/engine/daemon/network.go index 573901e7a6..9b5a66f36a 100644 --- a/components/engine/daemon/network.go +++ b/components/engine/daemon/network.go @@ -29,31 +29,36 @@ func (daemon *Daemon) NetworkControllerEnabled() bool { return daemon.netController != nil } -// FindNetwork function finds a network for a given string that can represent network name or id -func (daemon *Daemon) FindNetwork(idName string) (libnetwork.Network, error) { - // 1. match by full ID. - n, err := daemon.GetNetworkByID(idName) - if err == nil || !isNoSuchNetworkError(err) { - return n, err +// FindUniqueNetwork returns a network based on: +// 1. Full ID +// 2. Full Name +// 3. Partial ID +// as long as there is no ambiguity +func (daemon *Daemon) FindUniqueNetwork(term string) (libnetwork.Network, error) { + listByFullName := []libnetwork.Network{} + listByPartialID := []libnetwork.Network{} + for _, nw := range daemon.GetNetworks() { + if nw.ID() == term { + return nw, nil + } + if nw.Name() == term { + listByFullName = append(listByFullName, nw) + } + if strings.HasPrefix(nw.ID(), term) { + listByPartialID = append(listByPartialID, nw) + } } - - // 2. match by full name - n, err = daemon.GetNetworkByName(idName) - if err == nil || !isNoSuchNetworkError(err) { - return n, err + switch { + case len(listByFullName) == 1: + return listByFullName[0], nil + case len(listByFullName) > 1: + return nil, fmt.Errorf("network %s is ambiguous (%d matches found based on name)", term, len(listByFullName)) + case len(listByPartialID) == 1: + return listByPartialID[0], nil + case len(listByPartialID) > 1: + return nil, fmt.Errorf("network %s is ambiguous (%d matches found based on ID prefix)", term, len(listByPartialID)) } - - // 3. match by ID prefix - list := daemon.GetNetworksByIDPrefix(idName) - if len(list) == 0 { - // Be very careful to change the error type here, the libnetwork.ErrNoSuchNetwork error is used by the controller - // to retry the creation of the network as managed through the swarm manager - return nil, errors.WithStack(notFound(libnetwork.ErrNoSuchNetwork(idName))) - } - if len(list) > 1 { - return nil, errors.WithStack(invalidIdentifier(idName)) - } - return list[0], nil + return nil, libnetwork.ErrNoSuchNetwork(term) } func isNoSuchNetworkError(err error) bool { @@ -274,7 +279,9 @@ func (daemon *Daemon) createNetwork(create types.NetworkCreateRequest, id string // check if user defined CheckDuplicate, if set true, return err // otherwise prepare a warning message if create.CheckDuplicate { - return nil, libnetwork.NetworkNameError(create.Name) + if !agent || nw.Info().Dynamic() { + return nil, libnetwork.NetworkNameError(create.Name) + } } warning = fmt.Sprintf("Network with name %s (id : %s) already exists", nw.Name(), nw.ID()) } @@ -464,25 +471,56 @@ func (daemon *Daemon) GetNetworkDriverList() []string { } // DeleteManagedNetwork deletes an agent network. +// The requirement of networkID is enforced. func (daemon *Daemon) DeleteManagedNetwork(networkID string) error { - return daemon.deleteNetwork(networkID, true) + n, err := daemon.GetNetworkByID(networkID) + if err != nil { + return err + } + return daemon.deleteNetwork(n, true) } // DeleteNetwork destroys a network unless it's one of docker's predefined networks. func (daemon *Daemon) DeleteNetwork(networkID string) error { - return daemon.deleteNetwork(networkID, false) -} - -func (daemon *Daemon) deleteNetwork(networkID string, dynamic bool) error { - nw, err := daemon.FindNetwork(networkID) + n, err := daemon.GetNetworkByID(networkID) if err != nil { return err } + return daemon.deleteNetwork(n, false) +} - if nw.Info().Ingress() { - return nil +func (daemon *Daemon) deleteLoadBalancerSandbox(n libnetwork.Network) { + controller := daemon.netController + + //The only endpoint left should be the LB endpoint (nw.Name() + "-endpoint") + endpoints := n.Endpoints() + if len(endpoints) == 1 { + sandboxName := n.Name() + "-sbox" + + info := endpoints[0].Info() + if info != nil { + sb := info.Sandbox() + if sb != nil { + if err := sb.DisableService(); err != nil { + logrus.Warnf("Failed to disable service on sandbox %s: %v", sandboxName, err) + //Ignore error and attempt to delete the load balancer endpoint + } + } + } + + if err := endpoints[0].Delete(true); err != nil { + logrus.Warnf("Failed to delete endpoint %s (%s) in %s: %v", endpoints[0].Name(), endpoints[0].ID(), sandboxName, err) + //Ignore error and attempt to delete the sandbox. + } + + if err := controller.SandboxDestroy(sandboxName); err != nil { + logrus.Warnf("Failed to delete %s sandbox: %v", sandboxName, err) + //Ignore error and attempt to delete the network. + } } +} +func (daemon *Daemon) deleteNetwork(nw libnetwork.Network, dynamic bool) error { if runconfig.IsPreDefinedNetwork(nw.Name()) && !dynamic { err := fmt.Errorf("%s is a pre-defined network and cannot be removed", nw.Name()) return notAllowedError{err} diff --git a/components/engine/integration-cli/docker_cli_swarm_test.go b/components/engine/integration-cli/docker_cli_swarm_test.go index 283f576f57..0eff6aacb2 100644 --- a/components/engine/integration-cli/docker_cli_swarm_test.go +++ b/components/engine/integration-cli/docker_cli_swarm_test.go @@ -2137,3 +2137,76 @@ func (s *DockerSwarmSuite) TestSwarmClusterEventsConfig(c *check.C) { // filtered by config waitForEvent(c, d, t1, "-f type=config", "config remove "+id, defaultRetryCount) } + +func (s *DockerSwarmSuite) TestServiceCreateWithDuplicateNetworkNames(c *check.C) { + d := s.AddDaemon(c, true, true) + + name := "foo" + networkCreateRequest := types.NetworkCreateRequest{ + Name: name, + NetworkCreate: types.NetworkCreate{ + CheckDuplicate: false, + Driver: "bridge", + }, + } + + // Create networks with the same name, 2 in local scope and 1 in swarm scope + var n1 types.NetworkCreateResponse + status, body, err := d.SockRequest("POST", "/networks/create", networkCreateRequest) + c.Assert(err, checker.IsNil, check.Commentf(string(body))) + c.Assert(status, checker.Equals, http.StatusCreated, check.Commentf(string(body))) + c.Assert(json.Unmarshal(body, &n1), checker.IsNil) + + var n2 types.NetworkCreateResponse + status, body, err = d.SockRequest("POST", "/networks/create", networkCreateRequest) + c.Assert(err, checker.IsNil, check.Commentf(string(body))) + c.Assert(status, checker.Equals, http.StatusCreated, check.Commentf(string(body))) + c.Assert(json.Unmarshal(body, &n2), checker.IsNil) + + var n3 types.NetworkCreateResponse + // Dupliates with name but with different driver + networkCreateRequest.NetworkCreate.Driver = "overlay" + status, body, err = d.SockRequest("POST", "/networks/create", networkCreateRequest) + c.Assert(err, checker.IsNil, check.Commentf(string(body))) + c.Assert(status, checker.Equals, http.StatusCreated, check.Commentf(string(body))) + c.Assert(json.Unmarshal(body, &n3), checker.IsNil) + + // Create Service with the same name + d.CreateService(c, simpleTestService, func(s *swarm.Service) { + s.Spec.Name = "top" + s.Spec.TaskTemplate.Networks = []swarm.NetworkAttachmentConfig{ + {Target: name}, + } + }) + + // make sure task has been deployed. + waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 1) + + result := icmd.RunCmd(d.Command("ps", "-a", "-q")) + result.Assert(c, icmd.Success) + containers := strings.Split(strings.TrimSpace(result.Stdout()), "\n") + c.Assert(len(containers), checker.Equals, 1) + + result = icmd.RunCmd(d.Command("inspect", "--format", `{{.NetworkSettings.Networks.foo.NetworkID}}`, containers[0])) + result.Assert(c, icmd.Success) + c.Assert(strings.TrimSpace(result.Stdout()), checker.Equals, n3.ID) + + // Remove Service + result = icmd.RunCmd(d.Command("service", "rm", "top")) + result.Assert(c, icmd.Success) + + // make sure task has been destroyed. + waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 0) + + result = icmd.RunCmd(d.Command("network", "rm", n1.ID)) + result.Assert(c, icmd.Success) + c.Assert(strings.TrimSpace(result.Stdout()), checker.Equals, n1.ID) + + result = icmd.RunCmd(d.Command("network", "rm", n2.ID)) + result.Assert(c, icmd.Success) + c.Assert(strings.TrimSpace(result.Stdout()), checker.Equals, n2.ID) + + result = icmd.RunCmd(d.Command("network", "rm", n3.ID)) + result.Assert(c, icmd.Success) + c.Assert(strings.TrimSpace(result.Stdout()), checker.Equals, n3.ID) +} From 7ee813acf5ed1bcd4c8ccec62ccdd38973ff7213 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Tue, 31 Oct 2017 20:05:24 +0000 Subject: [PATCH 2/2] Update and use FindNetwork on Windows. Signed-off-by: Yong Tang Upstream-commit: b249ccb1151a3c2c7e442b6c1a769821afb28fe4 Component: engine --- .../server/router/network/network_routes.go | 4 +- .../daemon/container_operations_windows.go | 2 +- components/engine/daemon/daemon_test.go | 2 +- components/engine/daemon/errors.go | 17 ----- components/engine/daemon/network.go | 11 ++- components/engine/daemon/oci_windows.go | 2 +- .../integration-cli/docker_cli_swarm_test.go | 73 ------------------- .../engine/integration/service/create_test.go | 63 ++++++++++++++++ 8 files changed, 73 insertions(+), 101 deletions(-) diff --git a/components/engine/api/server/router/network/network_routes.go b/components/engine/api/server/router/network/network_routes.go index 497ce0a502..f124813439 100644 --- a/components/engine/api/server/router/network/network_routes.go +++ b/components/engine/api/server/router/network/network_routes.go @@ -530,7 +530,7 @@ func (n *networkRouter) postNetworksPrune(ctx context.Context, w http.ResponseWr } // findUniqueNetwork will search network across different scopes (both local and swarm). -// NOTE: This findUniqueNetwork is differnt from FindUniqueNetwork from the daemon. +// NOTE: This findUniqueNetwork is different from FindUniqueNetwork in the daemon. // In case multiple networks have duplicate names, return error. // First find based on full ID, return immediately once one is found. // If a network appears both in swarm and local, assume it is in local first @@ -547,7 +547,7 @@ func (n *networkRouter) findUniqueNetwork(term string) (types.NetworkResource, e return *n.buildDetailedNetworkResources(network, false), nil } - if network.Name() == term { + if network.Name() == term && !network.Info().Ingress() { // No need to check the ID collision here as we are still in // local scope and the network ID is unique in this scope. listByFullName[network.ID()] = *n.buildDetailedNetworkResources(network, false) diff --git a/components/engine/daemon/container_operations_windows.go b/components/engine/daemon/container_operations_windows.go index 51762a2441..c443d2908d 100644 --- a/components/engine/daemon/container_operations_windows.go +++ b/components/engine/daemon/container_operations_windows.go @@ -170,7 +170,7 @@ func (daemon *Daemon) initializeNetworkingPaths(container *container.Container, if nc.NetworkSettings != nil { for n := range nc.NetworkSettings.Networks { - sn, err := daemon.FindNetwork(n) + sn, err := daemon.FindUniqueNetwork(n) if err != nil { continue } diff --git a/components/engine/daemon/daemon_test.go b/components/engine/daemon/daemon_test.go index b70e00e7d2..44bf3414c0 100644 --- a/components/engine/daemon/daemon_test.go +++ b/components/engine/daemon/daemon_test.go @@ -317,7 +317,7 @@ func TestValidateContainerIsolation(t *testing.T) { func TestFindNetworkErrorType(t *testing.T) { d := Daemon{} - _, err := d.FindNetwork("fakeNet") + _, err := d.FindUniqueNetwork("fakeNet") _, ok := errors.Cause(err).(libnetwork.ErrNoSuchNetwork) if !errdefs.IsNotFound(err) || !ok { assert.Fail(t, "The FindNetwork method MUST always return an error that implements the NotFound interface and is ErrNoSuchNetwork") diff --git a/components/engine/daemon/errors.go b/components/engine/daemon/errors.go index 8e0756acfb..69c09fb83d 100644 --- a/components/engine/daemon/errors.go +++ b/components/engine/daemon/errors.go @@ -226,20 +226,3 @@ func translateContainerdStartErr(cmd string, setExitCode func(int), err error) e // TODO: it would be nice to get some better errors from containerd so we can return better errors here return retErr } - -// TODO: cpuguy83 take care of it once the new library is ready -type errNotFound struct{ error } - -func (errNotFound) NotFound() {} - -func (e errNotFound) Cause() error { - return e.error -} - -// notFound is a helper to create an error of the class with the same name from any error type -func notFound(err error) error { - if err == nil { - return nil - } - return errNotFound{err} -} diff --git a/components/engine/daemon/network.go b/components/engine/daemon/network.go index 9b5a66f36a..cf3d015b80 100644 --- a/components/engine/daemon/network.go +++ b/components/engine/daemon/network.go @@ -61,11 +61,6 @@ func (daemon *Daemon) FindUniqueNetwork(term string) (libnetwork.Network, error) return nil, libnetwork.ErrNoSuchNetwork(term) } -func isNoSuchNetworkError(err error) bool { - _, ok := err.(libnetwork.ErrNoSuchNetwork) - return ok -} - // GetNetworkByID function returns a network whose ID matches the given ID. // It fails with an error if no matching network is found. func (daemon *Daemon) GetNetworkByID(id string) (libnetwork.Network, error) { @@ -109,7 +104,11 @@ func (daemon *Daemon) GetNetworksByIDPrefix(partialID string) []libnetwork.Netwo // getAllNetworks returns a list containing all networks func (daemon *Daemon) getAllNetworks() []libnetwork.Network { - return daemon.netController.Networks() + c := daemon.netController + if c == nil { + return nil + } + return c.Networks() } type ingressJob struct { diff --git a/components/engine/daemon/oci_windows.go b/components/engine/daemon/oci_windows.go index c7c94f327a..d85cd269d9 100644 --- a/components/engine/daemon/oci_windows.go +++ b/components/engine/daemon/oci_windows.go @@ -159,7 +159,7 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) { gwHNSID := "" if c.NetworkSettings != nil { for n := range c.NetworkSettings.Networks { - sn, err := daemon.FindNetwork(n) + sn, err := daemon.FindUniqueNetwork(n) if err != nil { continue } diff --git a/components/engine/integration-cli/docker_cli_swarm_test.go b/components/engine/integration-cli/docker_cli_swarm_test.go index 0eff6aacb2..283f576f57 100644 --- a/components/engine/integration-cli/docker_cli_swarm_test.go +++ b/components/engine/integration-cli/docker_cli_swarm_test.go @@ -2137,76 +2137,3 @@ func (s *DockerSwarmSuite) TestSwarmClusterEventsConfig(c *check.C) { // filtered by config waitForEvent(c, d, t1, "-f type=config", "config remove "+id, defaultRetryCount) } - -func (s *DockerSwarmSuite) TestServiceCreateWithDuplicateNetworkNames(c *check.C) { - d := s.AddDaemon(c, true, true) - - name := "foo" - networkCreateRequest := types.NetworkCreateRequest{ - Name: name, - NetworkCreate: types.NetworkCreate{ - CheckDuplicate: false, - Driver: "bridge", - }, - } - - // Create networks with the same name, 2 in local scope and 1 in swarm scope - var n1 types.NetworkCreateResponse - status, body, err := d.SockRequest("POST", "/networks/create", networkCreateRequest) - c.Assert(err, checker.IsNil, check.Commentf(string(body))) - c.Assert(status, checker.Equals, http.StatusCreated, check.Commentf(string(body))) - c.Assert(json.Unmarshal(body, &n1), checker.IsNil) - - var n2 types.NetworkCreateResponse - status, body, err = d.SockRequest("POST", "/networks/create", networkCreateRequest) - c.Assert(err, checker.IsNil, check.Commentf(string(body))) - c.Assert(status, checker.Equals, http.StatusCreated, check.Commentf(string(body))) - c.Assert(json.Unmarshal(body, &n2), checker.IsNil) - - var n3 types.NetworkCreateResponse - // Dupliates with name but with different driver - networkCreateRequest.NetworkCreate.Driver = "overlay" - status, body, err = d.SockRequest("POST", "/networks/create", networkCreateRequest) - c.Assert(err, checker.IsNil, check.Commentf(string(body))) - c.Assert(status, checker.Equals, http.StatusCreated, check.Commentf(string(body))) - c.Assert(json.Unmarshal(body, &n3), checker.IsNil) - - // Create Service with the same name - d.CreateService(c, simpleTestService, func(s *swarm.Service) { - s.Spec.Name = "top" - s.Spec.TaskTemplate.Networks = []swarm.NetworkAttachmentConfig{ - {Target: name}, - } - }) - - // make sure task has been deployed. - waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 1) - - result := icmd.RunCmd(d.Command("ps", "-a", "-q")) - result.Assert(c, icmd.Success) - containers := strings.Split(strings.TrimSpace(result.Stdout()), "\n") - c.Assert(len(containers), checker.Equals, 1) - - result = icmd.RunCmd(d.Command("inspect", "--format", `{{.NetworkSettings.Networks.foo.NetworkID}}`, containers[0])) - result.Assert(c, icmd.Success) - c.Assert(strings.TrimSpace(result.Stdout()), checker.Equals, n3.ID) - - // Remove Service - result = icmd.RunCmd(d.Command("service", "rm", "top")) - result.Assert(c, icmd.Success) - - // make sure task has been destroyed. - waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 0) - - result = icmd.RunCmd(d.Command("network", "rm", n1.ID)) - result.Assert(c, icmd.Success) - c.Assert(strings.TrimSpace(result.Stdout()), checker.Equals, n1.ID) - - result = icmd.RunCmd(d.Command("network", "rm", n2.ID)) - result.Assert(c, icmd.Success) - c.Assert(strings.TrimSpace(result.Stdout()), checker.Equals, n2.ID) - - result = icmd.RunCmd(d.Command("network", "rm", n3.ID)) - result.Assert(c, icmd.Success) - c.Assert(strings.TrimSpace(result.Stdout()), checker.Equals, n3.ID) -} diff --git a/components/engine/integration/service/create_test.go b/components/engine/integration/service/create_test.go index 6cfb27e820..a4373721f5 100644 --- a/components/engine/integration/service/create_test.go +++ b/components/engine/integration/service/create_test.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/client" "github.com/docker/docker/integration-cli/request" "github.com/gotestyourself/gotestyourself/poll" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/context" ) @@ -80,6 +81,68 @@ func TestCreateServiceMultipleTimes(t *testing.T) { poll.WaitOn(t, networkIsRemoved(client, overlayID), poll.WithTimeout(1*time.Minute), poll.WithDelay(10*time.Second)) } +func TestCreateWithDuplicateNetworkNames(t *testing.T) { + defer setupTest(t)() + d := newSwarm(t) + defer d.Stop(t) + client, err := request.NewClientForHost(d.Sock()) + require.NoError(t, err) + + name := "foo" + networkCreate := types.NetworkCreate{ + CheckDuplicate: false, + Driver: "bridge", + } + + n1, err := client.NetworkCreate(context.Background(), name, networkCreate) + require.NoError(t, err) + + n2, err := client.NetworkCreate(context.Background(), name, networkCreate) + require.NoError(t, err) + + // Dupliates with name but with different driver + networkCreate.Driver = "overlay" + n3, err := client.NetworkCreate(context.Background(), name, networkCreate) + require.NoError(t, err) + + // Create Service with the same name + var instances uint64 = 1 + serviceSpec := swarmServiceSpec("top", instances) + + serviceSpec.TaskTemplate.Networks = append(serviceSpec.TaskTemplate.Networks, swarm.NetworkAttachmentConfig{Target: name}) + + service, err := client.ServiceCreate(context.Background(), serviceSpec, types.ServiceCreateOptions{}) + require.NoError(t, err) + + poll.WaitOn(t, serviceRunningTasksCount(client, service.ID, instances)) + + resp, _, err := client.ServiceInspectWithRaw(context.Background(), service.ID, types.ServiceInspectOptions{}) + require.NoError(t, err) + assert.Equal(t, n3.ID, resp.Spec.TaskTemplate.Networks[0].Target) + + // Remove Service + err = client.ServiceRemove(context.Background(), service.ID) + require.NoError(t, err) + + // Make sure task has been destroyed. + poll.WaitOn(t, serviceIsRemoved(client, service.ID)) + + // Remove networks + err = client.NetworkRemove(context.Background(), n3.ID) + require.NoError(t, err) + + err = client.NetworkRemove(context.Background(), n2.ID) + require.NoError(t, err) + + err = client.NetworkRemove(context.Background(), n1.ID) + require.NoError(t, err) + + // Make sure networks have been destroyed. + poll.WaitOn(t, networkIsRemoved(client, n3.ID), poll.WithTimeout(1*time.Minute), poll.WithDelay(10*time.Second)) + poll.WaitOn(t, networkIsRemoved(client, n2.ID), poll.WithTimeout(1*time.Minute), poll.WithDelay(10*time.Second)) + poll.WaitOn(t, networkIsRemoved(client, n1.ID), poll.WithTimeout(1*time.Minute), poll.WithDelay(10*time.Second)) +} + func swarmServiceSpec(name string, replicas uint64) swarm.ServiceSpec { return swarm.ServiceSpec{ Annotations: swarm.Annotations{