From 53d447c5d5c85d5595d5170411189c88a135a789 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Tue, 13 Dec 2016 17:20:52 -0800 Subject: [PATCH] Fix volume Create to check against canonical driver name Previously, it was comparing against the driver name passed in by the caller. This could lead to subtle issues when using plugins, like "plugin" vs. "plugin:latest". Also, remove "conflict:" prefix to improve the error message. Signed-off-by: Aaron Lehmann --- ...ocker_cli_external_volume_driver_unix_test.go | 16 ++++++++++++++++ integration-cli/docker_cli_volume_test.go | 15 --------------- volume/store/errors.go | 2 +- volume/store/store.go | 13 +++++++++++-- 4 files changed, 28 insertions(+), 18 deletions(-) diff --git a/integration-cli/docker_cli_external_volume_driver_unix_test.go b/integration-cli/docker_cli_external_volume_driver_unix_test.go index 466d376d95..9c8ced9d79 100644 --- a/integration-cli/docker_cli_external_volume_driver_unix_test.go +++ b/integration-cli/docker_cli_external_volume_driver_unix_test.go @@ -284,6 +284,22 @@ func (s *DockerExternalVolumeSuite) TearDownSuite(c *check.C) { c.Assert(err, checker.IsNil) } +func (s *DockerExternalVolumeSuite) TestVolumeCLICreateOptionConflict(c *check.C) { + dockerCmd(c, "volume", "create", "test") + + out, _, err := dockerCmdWithError("volume", "create", "test", "--driver", volumePluginName) + c.Assert(err, check.NotNil, check.Commentf("volume create exception name already in use with another driver")) + c.Assert(out, checker.Contains, "A volume named test already exists") + + out, _ = dockerCmd(c, "volume", "inspect", "--format={{ .Driver }}", "test") + _, _, err = dockerCmdWithError("volume", "create", "test", "--driver", strings.TrimSpace(out)) + c.Assert(err, check.IsNil) + + // make sure hidden --name option conflicts with positional arg name + out, _, err = dockerCmdWithError("volume", "create", "--name", "test2", "test2") + c.Assert(err, check.NotNil, check.Commentf("Conflicting options: either specify --name or provide positional arg, not both")) +} + func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverNamed(c *check.C) { s.d.StartWithBusybox(c) diff --git a/integration-cli/docker_cli_volume_test.go b/integration-cli/docker_cli_volume_test.go index 0dace5d495..772b2d6359 100644 --- a/integration-cli/docker_cli_volume_test.go +++ b/integration-cli/docker_cli_volume_test.go @@ -29,21 +29,6 @@ func (s *DockerSuite) TestVolumeCLICreate(c *check.C) { c.Assert(name, check.Equals, "test2") } -func (s *DockerSuite) TestVolumeCLICreateOptionConflict(c *check.C) { - dockerCmd(c, "volume", "create", "test") - out, _, err := dockerCmdWithError("volume", "create", "test", "--driver", "nosuchdriver") - c.Assert(err, check.NotNil, check.Commentf("volume create exception name already in use with another driver")) - c.Assert(out, checker.Contains, "A volume named test already exists") - - out, _ = dockerCmd(c, "volume", "inspect", "--format={{ .Driver }}", "test") - _, _, err = dockerCmdWithError("volume", "create", "test", "--driver", strings.TrimSpace(out)) - c.Assert(err, check.IsNil) - - // make sure hidden --name option conflicts with positional arg name - out, _, err = dockerCmdWithError("volume", "create", "--name", "test2", "test2") - c.Assert(err, check.NotNil, check.Commentf("Conflicting options: either specify --name or provide positional arg, not both")) -} - func (s *DockerSuite) TestVolumeCLIInspect(c *check.C) { c.Assert( exec.Command(dockerBinary, "volume", "inspect", "doesntexist").Run(), diff --git a/volume/store/errors.go b/volume/store/errors.go index db7c3ce243..980175f29c 100644 --- a/volume/store/errors.go +++ b/volume/store/errors.go @@ -14,7 +14,7 @@ var ( // errInvalidName is a typed error returned when creating a volume with a name that is not valid on the platform errInvalidName = errors.New("volume name is not valid on this platform") // errNameConflict is a typed error returned on create when a volume exists with the given name, but for a different driver - errNameConflict = errors.New("conflict: volume name must be unique") + errNameConflict = errors.New("volume name must be unique") ) // OpErr is the error type returned by functions in the store package. It describes diff --git a/volume/store/store.go b/volume/store/store.go index 9b896703f6..b126800868 100644 --- a/volume/store/store.go +++ b/volume/store/store.go @@ -309,8 +309,17 @@ func (s *VolumeStore) checkConflict(name, driverName string) (volume.Volume, err vDriverName := v.DriverName() var conflict bool - if driverName != "" && vDriverName != driverName { - conflict = true + if driverName != "" { + // Retrieve canonical driver name to avoid inconsistencies (for example + // "plugin" vs. "plugin:latest") + vd, err := volumedrivers.GetDriver(driverName) + if err != nil { + return nil, err + } + + if vDriverName != vd.Name() { + conflict = true + } } // let's check if the found volume ref