volume rm require exact name

This commit reverts the behavior of allowing volume removal by partial name in favor of requiring the exact name. It does this by favoring usage of `GetVolume` over `LookupVolume` In the past, issue #3891 was raised to allow partial name matching on `podman volume rm`, which was fixed by PR #3896. However, it has been determined in issue #23943 that the pros of allowing removal by partial name (less keys to type) are outweighed by the cons (possibility of unintentionally deleting a volume you didn't mean to). So the behavior will be reverted. Since PR #3896, there has also been PR #4832 and PR #6736, which extended the partial name removal behavior to API endpoints. This commit also makes these endpoints require exact names. Closes #23943 Signed-off-by: Zachary Hanham <[email protected]>
containers · Sep 22, 2024 · 9ae856d · 9ae856d
1 parent 186a2b8
commit 9ae856d
Show file tree

Hide file tree

Showing 9 changed files with 52 additions and 35 deletions.
diff --git a/docs/source/locale/ja/LC_MESSAGES/markdown.po b/docs/source/locale/ja/LC_MESSAGES/markdown.po
@@ -30940,7 +30940,7 @@ msgid ""
 "removed. If a volume is being used by a container, an error is returned "
 "unless the **--force** flag is being used. To remove all volumes, use the"
 " **--all** flag. Volumes can be removed individually by providing their "
-"full name or a unique partial name."
+"full name."
 msgstr ""
 
 #: ../../source/markdown/podman-volume-rm.1.md:21

diff --git a/docs/source/markdown/podman-volume-rm.1.md b/docs/source/markdown/podman-volume-rm.1.md
@@ -11,7 +11,7 @@ podman\-volume\-rm - Remove one or more volumes
 Removes one or more volumes. Only volumes that are not being used are removed.
 If a volume is being used by a container, an error is returned unless the **--force**
 flag is being used. To remove all volumes, use the **--all** flag.
-Volumes can be removed individually by providing their full name or a unique partial name.
+Volumes can be removed individually by providing their full name.
 
 ## OPTIONS
 

diff --git a/pkg/api/handlers/compat/volumes.go b/pkg/api/handlers/compat/volumes.go
@@ -240,7 +240,7 @@ func RemoveVolume(w http.ResponseWriter, r *http.Request) {
 	 * respectively.
 	 */
 	name := utils.GetName(r)
-	vol, err := runtime.LookupVolume(name)
+	vol, err := runtime.GetVolume(name)
 	if err == nil {
 		// As above, we do not pass `force` from the query parameters here
 		if err := runtime.RemoveVolume(r.Context(), vol, false, query.Timeout); err != nil {

diff --git a/pkg/api/handlers/libpod/volumes.go b/pkg/api/handlers/libpod/volumes.go
@@ -202,7 +202,7 @@ func RemoveVolume(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	name := utils.GetName(r)
-	vol, err := runtime.LookupVolume(name)
+	vol, err := runtime.GetVolume(name)
 	if err != nil {
 		utils.VolumeNotFound(w, name, err)
 		return

diff --git a/pkg/bindings/test/volumes_test.go b/pkg/bindings/test/volumes_test.go
@@ -81,6 +81,12 @@ var _ = Describe("Podman volumes", func() {
 		err = volumes.Remove(connText, vol.Name, nil)
 		Expect(err).ToNot(HaveOccurred())
 
+		// removing partial name should result in 404
+		err = volumes.Remove(connText, vol.Name[:3], nil)
+		code, err = bindings.CheckResponseCode(err)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(code).To(BeNumerically("==", http.StatusNotFound))
+
 		// Removing a volume that is being used without force should be 409
 		vol, err = volumes.Create(connText, entities.VolumeCreateOptions{}, nil)
 		Expect(err).ToNot(HaveOccurred())

diff --git a/pkg/domain/infra/abi/volumes.go b/pkg/domain/infra/abi/volumes.go
@@ -61,7 +61,7 @@ func (ic *ContainerEngine) VolumeRm(ctx context.Context, namesOrIds []string, op
 		}
 	} else {
 		for _, id := range namesOrIds {
-			vol, err := ic.Libpod.LookupVolume(id)
+			vol, err := ic.Libpod.GetVolume(id)
 			if err != nil {
 				if opts.Ignore && errors.Is(err, define.ErrNoSuchVolume) {
 					continue

diff --git a/test/apiv2/30-volumes.at b/test/apiv2/30-volumes.at
@@ -85,8 +85,13 @@ t GET libpod/volumes/nonexistent/json 404 \
     .response=404
 
 ## Remove volumes
+# Should NOT work on partial name
+t DELETE libpod/volumes/foo 404 \
+    .cause="no such volume" \
+    .response=404
+# Remove it for real
 t DELETE libpod/volumes/foo1 204
-#After remove foo1 volume, this volume should not exist
+# After remove foo1 volume, this volume should not exist
 t GET libpod/volumes/foo1/json 404
 # Negative test
 t DELETE libpod/volumes/foo1 404 \

diff --git a/test/e2e/volume_rm_test.go b/test/e2e/volume_rm_test.go
@@ -54,7 +54,11 @@ var _ = Describe("Podman volume rm", func() {
 	It("podman volume remove bogus", func() {
 		session := podmanTest.Podman([]string{"volume", "rm", "bogus"})
 		session.WaitWithDefaultTimeout()
-		Expect(session).Should(ExitWithError(1, `no volume with name "bogus" found: no such volume`))
+		expected := "Error: no such volume"
+		if podmanTest.DatabaseBackend == "boltdb" {
+			expected = "Error: volume with name bogus not found: no such volume"
+		}
+		Expect(session).Should(ExitWithError(1, expected))
 	})
 
 	It("podman rm with --all flag", func() {
@@ -76,42 +80,25 @@ var _ = Describe("Podman volume rm", func() {
 		Expect(session.OutputToStringArray()).To(BeEmpty())
 	})
 
-	It("podman volume rm by partial name", func() {
-		session := podmanTest.Podman([]string{"volume", "create", "myvol"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).Should(ExitCleanly())
-
-		session = podmanTest.Podman([]string{"volume", "rm", "myv"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).Should(ExitCleanly())
-
-		session = podmanTest.Podman([]string{"volume", "ls"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).Should(ExitCleanly())
-		Expect(session.OutputToStringArray()).To(BeEmpty())
-	})
-
-	It("podman volume rm by nonunique partial name", func() {
-		session := podmanTest.Podman([]string{"volume", "create", "myvol1"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).Should(ExitCleanly())
-
-		session = podmanTest.Podman([]string{"volume", "create", "myvol2"})
+	It("podman volume remove require exact name", func() {
+		session := podmanTest.Podman([]string{"volume", "create", defaultVolName})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
 
-		session = podmanTest.Podman([]string{"volume", "rm", "myv"})
+		nameStart := defaultVolName[:3]
+		session = podmanTest.Podman([]string{"volume", "rm", nameStart})
 		session.WaitWithDefaultTimeout()
-		expect := "more than one result for volume name myv: volume already exists"
+		expectedRm := "Error: no such volume"
 		if podmanTest.DatabaseBackend == "boltdb" {
-			// boltdb issues volume name in quotes
-			expect = `more than one result for volume name "myv": volume already exists`
+			expectedRm = fmt.Sprintf("Error: volume with name %s not found: no such volume", nameStart)
 		}
-		Expect(session).To(ExitWithError(125, expect))
+		Expect(session).Should(ExitWithError(1, expectedRm))
 
 		session = podmanTest.Podman([]string{"volume", "ls"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
-		Expect(len(session.OutputToStringArray())).To(BeNumerically(">=", 2))
+		output := session.OutputToStringArray()
+		expectedLs := []string{"DRIVER      VOLUME NAME", fmt.Sprintf("local       %s", defaultVolName)}
+		Expect(output).To(Equal(expectedLs))
 	})
 })
diff --git a/test/system/160-volumes.bats b/test/system/160-volumes.bats
@@ -556,7 +556,11 @@ EOF
 
 @test "podman volume rm --force bogus" {
     run_podman 1 volume rm bogus
-    is "$output" "Error: no volume with name \"bogus\" found: no such volume" "Should print error"
+    expectedoutput="Error: no such volume"
+    if [[ "$CI_DESIRED_DATABASE" = "boltdb" ]]; then
+        expectedoutput="Error: volume with name bogus not found: no such volume"
+    fi
+    is "$output" "$expectedoutput" "Should print error"
     run_podman volume rm --force bogus
     is "$output" "" "Should print no output"
 
@@ -568,6 +572,21 @@ EOF
     assert "$output" = "" "no volumes"
 }
 
+@test "podman volume rm require exact name" {
+    myvolume="testvol"
+    run_podman volume create "$myvolume"
+    run_podman 1 volume rm test
+    expectedoutput="Error: no such volume"
+    if [[ "$CI_DESIRED_DATABASE" = "boltdb" ]]; then
+        expectedoutput="Error: volume with name test not found: no such volume"
+    fi
+    is "$output" "$expectedoutput" "Should print error"
+    run_podman volume ls -q
+    assert "$output" = "$myvolume" "$myvolume still exists"
+    # Clean up
+    run_podman volume rm "$myvolume"
+}
+
 @test "podman ps -f" {
     vol1="/v1_$(random_string)"
     run_podman run -d --rm --volume ${PODMAN_TMPDIR}:$vol1 $IMAGE top