From c9ee5a3f479eadb7fcb371c69a8ffebf54c1bbfb Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Thu, 19 Jan 2017 03:04:26 -0800 Subject: [PATCH] Return error in case `docker network inspect` is ambiguous This fix is partially based on comment https://github.com/docker/docker/issues/30242#issuecomment-273517205 Currently, `docker network inspect` relies on `FindNetwork()` which does not take into consideration that multiple networks with the same name might exist. This fix propose to return `docker network inspect` in a similiar fashion like other commands: 1. Lookup full ID 2. Lookup full name 3. Lookup partial ID If multiple networks exist, an error will be returned. NOTE: this fix is not a complete fix for the issue raised in https://github.com/docker/docker/issues/30242#issuecomment-273517205 where SwarmKit is unable to update when multiple networks with the same name exit. To fix that issue requires multiple places when `FindNetwork()` is called. Because of the impact of changing `FindNetwork()`, this fix focus on the issue in `docker network inspect`. A separate PR will be created to address https://github.com/docker/docker/issues/30242#issuecomment-273517205 An integration test has been added. Signed-off-by: Yong Tang Upstream-commit: abf31ee0837ff80638f10b77df9025b6b6470253 Component: engine --- .../api/server/router/network/backend.go | 2 - .../server/router/network/network_routes.go | 80 +++++++++++++++++-- .../integration-cli/docker_cli_swarm_test.go | 75 +++++++++++++++++ 3 files changed, 149 insertions(+), 8 deletions(-) diff --git a/components/engine/api/server/router/network/backend.go b/components/engine/api/server/router/network/backend.go index 0d1dfb0123..000ace6d66 100644 --- a/components/engine/api/server/router/network/backend.go +++ b/components/engine/api/server/router/network/backend.go @@ -11,8 +11,6 @@ import ( // to provide network specific functionality. type Backend interface { FindNetwork(idName string) (libnetwork.Network, error) - GetNetworkByName(idName string) (libnetwork.Network, error) - GetNetworksByID(partialID string) []libnetwork.Network GetNetworks() []libnetwork.Network CreateNetwork(nc types.NetworkCreateRequest) (*types.NetworkCreateResponse, error) ConnectContainerToNetwork(containerName, networkName string, endpointConfig *network.EndpointSettings) error diff --git a/components/engine/api/server/router/network/network_routes.go b/components/engine/api/server/router/network/network_routes.go index f9df7d57b5..970a391f3d 100644 --- a/components/engine/api/server/router/network/network_routes.go +++ b/components/engine/api/server/router/network/network_routes.go @@ -2,7 +2,9 @@ package network import ( "encoding/json" + "fmt" "net/http" + "strings" "golang.org/x/net/context" @@ -82,14 +84,80 @@ func (n *networkRouter) getNetwork(ctx context.Context, w http.ResponseWriter, r return err } - nw, err := n.backend.FindNetwork(vars["id"]) - if err != nil { - if nr, err := n.cluster.GetNetwork(vars["id"]); err == nil { - return httputils.WriteJSON(w, http.StatusOK, nr) + term := vars["id"] + + // In case multiple networks have duplicate names, return error. + // TODO (yongtang): should we wrap with version here for backward compatibility? + + // 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 + listByFullName := map[string]types.NetworkResource{} + listByPartialID := map[string]types.NetworkResource{} + + nw := n.backend.GetNetworks() + for _, network := range nw { + if network.ID() == term { + return httputils.WriteJSON(w, http.StatusOK, *n.buildDetailedNetworkResources(network)) + } + 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) + } + 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) } - return err } - return httputils.WriteJSON(w, http.StatusOK, n.buildDetailedNetworkResources(nw)) + + nr, _ := n.cluster.GetNetworks() + for _, network := range nr { + if network.ID == term { + return httputils.WriteJSON(w, http.StatusOK, network) + } + 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 httputils.WriteJSON(w, http.StatusOK, v) + } + } + if len(listByFullName) > 1 { + return 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 httputils.WriteJSON(w, http.StatusOK, v) + } + } + if len(listByPartialID) > 1 { + return fmt.Errorf("network %s is ambiguous (%d matches found based on ID prefix)", term, len(listByPartialID)) + } + + return libnetwork.ErrNoSuchNetwork(term) } func (n *networkRouter) postNetworkCreate(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { diff --git a/components/engine/integration-cli/docker_cli_swarm_test.go b/components/engine/integration-cli/docker_cli_swarm_test.go index 3a88a00394..1a64731868 100644 --- a/components/engine/integration-cli/docker_cli_swarm_test.go +++ b/components/engine/integration-cli/docker_cli_swarm_test.go @@ -14,6 +14,7 @@ import ( "strings" "time" + "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/integration-cli/checker" "github.com/docker/docker/integration-cli/daemon" @@ -1665,3 +1666,77 @@ func (s *DockerSwarmSuite) TestSwarmReadonlyRootfs(c *check.C) { c.Assert(err, checker.IsNil, check.Commentf(out)) c.Assert(strings.TrimSpace(out), checker.Equals, "true") } + +func (s *DockerSwarmSuite) TestNetworkInspectWithDuplicateNames(c *check.C) { + d := s.AddDaemon(c, true, true) + + name := "foo" + networkCreateRequest := types.NetworkCreateRequest{ + Name: name, + NetworkCreate: types.NetworkCreate{ + CheckDuplicate: false, + Driver: "bridge", + }, + } + + 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) + + // Full ID always works + out, err := d.Cmd("network", "inspect", "--format", "{{.ID}}", n1.ID) + c.Assert(err, checker.IsNil, check.Commentf(out)) + c.Assert(strings.TrimSpace(out), checker.Equals, n1.ID) + + // Name works if it is unique + out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", name) + c.Assert(err, checker.IsNil, check.Commentf(out)) + c.Assert(strings.TrimSpace(out), checker.Equals, n1.ID) + + 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) + + // Full ID always works + out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", n1.ID) + c.Assert(err, checker.IsNil, check.Commentf(out)) + c.Assert(strings.TrimSpace(out), checker.Equals, n1.ID) + + out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", n2.ID) + c.Assert(err, checker.IsNil, check.Commentf(out)) + c.Assert(strings.TrimSpace(out), checker.Equals, n2.ID) + + // Name with duplicates + out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", name) + c.Assert(err, checker.NotNil, check.Commentf(out)) + c.Assert(out, checker.Contains, "network foo is ambiguous (2 matches found based on name)") + + out, err = d.Cmd("network", "rm", n2.ID) + c.Assert(err, checker.IsNil, check.Commentf(out)) + + // 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, &n2), checker.IsNil) + + // Full ID always works + out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", n1.ID) + c.Assert(err, checker.IsNil, check.Commentf(out)) + c.Assert(strings.TrimSpace(out), checker.Equals, n1.ID) + + out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", n2.ID) + c.Assert(err, checker.IsNil, check.Commentf(out)) + c.Assert(strings.TrimSpace(out), checker.Equals, n2.ID) + + // Name with duplicates + out, err = d.Cmd("network", "inspect", "--format", "{{.ID}}", name) + c.Assert(err, checker.NotNil, check.Commentf(out)) + c.Assert(out, checker.Contains, "network foo is ambiguous (2 matches found based on name)") +}