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 containers#3891 was raised to allow partial name matching on `podman volume rm`, which was fixed by PR containers#3896. However, it has been determined in issue containers#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 containers#3896, there has also been PR containers#4832 and PR containers#6736, which extended the partial name removal behavior to API endpoints. This commit also makes these endpoints require exact names. Closes containers#23943 Signed-off-by: Zachary Hanham <[email protected]>
zackattackz · Sep 21, 2024 · 68d4318 · 68d4318
1 parent 186a2b8
commit 68d4318
Show file tree

Hide file tree

Showing 8 changed files with 21 additions and 34 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,7 @@ 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`))
+		Expect(session).Should(ExitWithError(1, "Error: no such volume"))
 	})
 
 	It("podman rm with --all flag", func() {
@@ -76,42 +76,18 @@ var _ = Describe("Podman volume rm", func() {
 		Expect(session.OutputToStringArray()).To(BeEmpty())
 	})
 
-	It("podman volume rm by partial name", func() {
+	It("podman volume remove require exact 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"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).Should(ExitCleanly())
-
-		session = podmanTest.Podman([]string{"volume", "rm", "myv"})
-		session.WaitWithDefaultTimeout()
-		expect := "more than one result for volume name myv: volume already exists"
-		if podmanTest.DatabaseBackend == "boltdb" {
-			// boltdb issues volume name in quotes
-			expect = `more than one result for volume name "myv": volume already exists`
-		}
-		Expect(session).To(ExitWithError(125, expect))
+		Expect(session).Should(ExitWithError(1, "Error: no such volume"))
 
 		session = podmanTest.Podman([]string{"volume", "ls"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(ExitCleanly())
-		Expect(len(session.OutputToStringArray())).To(BeNumerically(">=", 2))
+		Expect(session.OutputToStringArray()).To(HaveLen(1))
 	})
 })