Removing tagged images change in behavior

An image name is really just a tag.  When an image has multiple tags, we should be
able to "delete" the one of its tags without harm. In this case, the "delete' is
really a form of Untag (removing the tag from the image).

If an image has multiple tags and the user tries to delete by ID without force, this
should be denied because when you delete by ID there is no distinguishing it like
image tags.

Signed-off-by: baude <bbaude@redhat.com>

Closes: #528
Approved by: mheon

6 files changed, 167 insertions, 25 deletions

diff --git a/cmd/podman/rmi.go b/cmd/podman/rmi.go
index b38686a87..cc902cdbe 100644
--- a/cmd/podman/rmi.go
+++ b/cmd/podman/rmi.go
@@ -75,14 +75,14 @@ func rmiCmd(c *cli.Context) error {
 		return errors.Errorf("no valid images to delete")
 	}
 	for _, img := range imagesToDelete {
-		err := runtime.RemoveImage(img, c.Bool("force"))
+		msg, err := runtime.RemoveImage(img, c.Bool("force"))
 		if err != nil {
 			if lastError != nil {
 				fmt.Fprintln(os.Stderr, lastError)
 			}
 			lastError = err
 		} else {
-			fmt.Println(img.ID())
+			fmt.Println(msg)
 		}
 	}
 	return lastError
diff --git a/libpod/image/image.go b/libpod/image/image.go
index 633a682cc..b6e15e361 100644
--- a/libpod/image/image.go
+++ b/libpod/image/image.go
@@ -685,3 +685,60 @@ func Import(path, reference string, writer io.Writer, signingOptions SigningOpti
 	}
 	return runtime.NewFromLocal(reference)
 }
+
+// MatchRepoTag takes a string and tries to match it against an
+// image's repotags
+func (i *Image) MatchRepoTag(input string) (string, error) {
+	results := make(map[int][]string)
+	var maxCount int
+	// first check if we have an exact match with the input
+	if util.StringInSlice(input, i.Names()) {
+		return input, nil
+	}
+	// next check if we are missing the tag
+	dcImage, err := decompose(input)
+	if err != nil {
+		return "", err
+	}
+	for _, repoName := range i.Names() {
+		count := 0
+		dcRepoName, err := decompose(repoName)
+		if err != nil {
+			return "", err
+		}
+		if dcRepoName.registry == dcImage.registry && dcImage.registry != "" {
+			count++
+		}
+		if dcRepoName.name == dcImage.name && dcImage.name != "" {
+			count++
+		} else if splitString(dcRepoName.name) == splitString(dcImage.name) {
+			count++
+		}
+		if dcRepoName.tag == dcImage.tag {
+			count++
+		}
+		results[count] = append(results[count], repoName)
+		if count > maxCount {
+			maxCount = count
+		}
+	}
+	if maxCount == 0 {
+		return "", errors.Errorf("unable to match user input to any specific repotag")
+	}
+	if len(results[maxCount]) > 1 {
+		return "", errors.Errorf("user input matched multiple repotags for the image")
+	}
+	return results[maxCount][0], nil
+}
+
+// splitString splits input string by / and returns the last array item
+func splitString(input string) string {
+	split := strings.Split(input, "/")
+	return split[len(split)-1]
+}
+
+// InputIsID returns a bool if the user input for an image
+// is the image's partial or full id
+func (i *Image) InputIsID() bool {
+	return strings.HasPrefix(i.ID(), i.InputName)
+}
diff --git a/libpod/image/image_test.go b/libpod/image/image_test.go
index d9e8987a6..5c7cc191d 100644
--- a/libpod/image/image_test.go
+++ b/libpod/image/image_test.go
@@ -136,3 +136,52 @@ func TestImage_New(t *testing.T) {
 	// Shutdown the runtime and remove the temporary storage
 	cleanup(workdir, ir)
 }
+
+// TestImage_MatchRepoTag tests the various inputs we need to match
+// against an image's reponames
+func TestImage_MatchRepoTag(t *testing.T) {
+	//Set up
+	workdir, err := mkWorkDir()
+	assert.NoError(t, err)
+
+	so := storage.StoreOptions{
+		RunRoot:   workdir,
+		GraphRoot: workdir,
+	}
+	ir, err := NewImageRuntimeFromOptions(so)
+	assert.NoError(t, err)
+	newImage, err := ir.New("busybox", "", "", os.Stdout, nil, SigningOptions{})
+	assert.NoError(t, err)
+	err = newImage.TagImage("foo:latest")
+	assert.NoError(t, err)
+	err = newImage.TagImage("foo:bar")
+	assert.NoError(t, err)
+
+	// Tests start here.
+	for _, name := range bbNames {
+		repoTag, err := newImage.MatchRepoTag(name)
+		assert.NoError(t, err)
+		assert.Equal(t, "docker.io/library/busybox:latest", repoTag)
+	}
+
+	// Test against tagged images of busybox
+	// foo should resolve to foo:latest
+	repoTag, err := newImage.MatchRepoTag("foo")
+	assert.NoError(t, err)
+	assert.Equal(t, "foo:latest", repoTag)
+
+	// foo:bar should resolve to foo:bar
+	repoTag, err = newImage.MatchRepoTag("foo:bar")
+	assert.NoError(t, err)
+	assert.Equal(t, "foo:bar", repoTag)
+	// Shutdown the runtime and remove the temporary storage
+	cleanup(workdir, ir)
+}
+
+// Test_splitString tests the splitString function in image that
+// takes input and splits on / and returns the last array item
+func Test_splitString(t *testing.T) {
+	assert.Equal(t, splitString("foo/bar"), "bar")
+	assert.Equal(t, splitString("a/foo/bar"), "bar")
+	assert.Equal(t, splitString("bar"), "bar")
+}
diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go
index 248d1c0aa..c830304ae 100644
--- a/libpod/runtime_img.go
+++ b/libpod/runtime_img.go
@@ -834,18 +834,18 @@ func (r *Runtime) UntagImage(image *storage.Image, tag string) (string, error) {
 
 // RemoveImage deletes an image from local storage
 // Images being used by running containers can only be removed if force=true
-func (r *Runtime) RemoveImage(image *image.Image, force bool) error {
+func (r *Runtime) RemoveImage(image *image.Image, force bool) (string, error) {
 	r.lock.Lock()
 	defer r.lock.Unlock()
 
 	if !r.valid {
-		return ErrRuntimeStopped
+		return "", ErrRuntimeStopped
 	}
 
 	// Get all containers, filter to only those using the image, and remove those containers
 	ctrs, err := r.state.AllContainers()
 	if err != nil {
-		return err
+		return "", err
 	}
 	imageCtrs := []*Container{}
 	for _, ctr := range ctrs {
@@ -857,29 +857,33 @@ func (r *Runtime) RemoveImage(image *image.Image, force bool) error {
 		if force {
 			for _, ctr := range imageCtrs {
 				if err := r.removeContainer(ctr, true); err != nil {
-					return errors.Wrapf(err, "error removing image %s: container %s using image could not be removed", image.ID, ctr.ID())
+					return "", errors.Wrapf(err, "error removing image %s: container %s using image could not be removed", image.ID, ctr.ID())
 				}
 			}
 		} else {
-			return fmt.Errorf("could not remove image %s as it is being used by %d containers", image.ID, len(imageCtrs))
+			return "", fmt.Errorf("could not remove image %s as it is being used by %d containers", image.ID, len(imageCtrs))
 		}
 	}
 
-	if len(image.Names()) > 1 && !force {
-		return fmt.Errorf("unable to delete %s (must force) - image is referred to in multiple tags", image.ID)
-	}
-
-	if len(image.Names()) > 1 && force {
-		// If it is forced, we have to untag the image so that it can be deleted
-		if err = r.store.SetNames(image.ID(), image.Names()[:0]); err != nil {
-			return err
+	if len(image.Names()) > 1 && !image.InputIsID() {
+		// If the image has multiple reponames, we do not technically delete
+		// the image. we figure out which repotag the user is trying to refer
+		// to and untag it.
+		repoName, err := image.MatchRepoTag(image.InputName)
+		if err != nil {
+			return "", err
+		}
+		if err := image.UntagImage(repoName); err != nil {
+			return "", err
 		}
+		return fmt.Sprintf("Untagged: %s", repoName), nil
+	} else if len(image.Names()) > 1 && image.InputIsID() && !force {
+		// If the user requests to delete an image by ID and the image has multiple
+		// reponames and no force is applied, we error out.
+		return "", fmt.Errorf("unable to delete %s (must force) - image is referred to in multiple tags", image.ID())
 	}
 
-	if err = image.Remove(force); err != nil {
-		return err
-	}
-	return nil
+	return image.ID(), image.Remove(force)
 }
 
 // GetImage retrieves an image matching the given name or hash from system
diff --git a/test/e2e/push_test.go b/test/e2e/push_test.go
index 028f96424..f783fe418 100644
--- a/test/e2e/push_test.go
+++ b/test/e2e/push_test.go
@@ -35,20 +35,16 @@ var _ = Describe("Podman push", func() {
 
 		session = podmanTest.Podman([]string{"rmi", ALPINE})
 		session.WaitWithDefaultTimeout()
-		Expect(session.ExitCode()).To(Not(Equal(0)))
+		Expect(session.ExitCode()).To(Equal(0))
 
 		session = podmanTest.Podman([]string{"rmi", "busybox:test"})
 		session.WaitWithDefaultTimeout()
-		Expect(session.ExitCode()).To(Not(Equal(0)))
-
-		session = podmanTest.Podman([]string{"rmi", "-f", "busybox:test"})
-		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 	})
 
 	// push to oci-archive, docker-archive, and dir are tested in pull_test.go
 
-	It("podman push to containers/storage", func() {
+	It("podman push to dir", func() {
 		session := podmanTest.Podman([]string{"push", "--remove-signatures", ALPINE, "dir:/tmp/busybox"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
diff --git a/test/e2e/rmi_test.go b/test/e2e/rmi_test.go
index 04346f5ac..67ccc1b95 100644
--- a/test/e2e/rmi_test.go
+++ b/test/e2e/rmi_test.go
@@ -67,4 +67,40 @@ var _ = Describe("Podman rmi", func() {
 
 	})
 
+	It("podman rmi tagged image", func() {
+		setup := podmanTest.Podman([]string{"images", "-q", ALPINE})
+		setup.WaitWithDefaultTimeout()
+		Expect(setup.ExitCode()).To(Equal(0))
+
+		session := podmanTest.Podman([]string{"tag", "alpine", "foo:bar", "foo"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		result := podmanTest.Podman([]string{"images", "-q", "foo"})
+		result.WaitWithDefaultTimeout()
+		Expect(result.ExitCode()).To(Equal(0))
+
+		Expect(result.LineInOuputContains(setup.OutputToString())).To(BeTrue())
+	})
+
+	It("podman rmi image with tags by ID cannot be done without force", func() {
+		setup := podmanTest.Podman([]string{"images", "-q", ALPINE})
+		setup.WaitWithDefaultTimeout()
+		Expect(setup.ExitCode()).To(Equal(0))
+		alpineId := setup.OutputToString()
+
+		session := podmanTest.Podman([]string{"tag", "alpine", "foo:bar", "foo"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		// Trying without --force should fail
+		result := podmanTest.Podman([]string{"rmi", alpineId})
+		result.WaitWithDefaultTimeout()
+		Expect(result.ExitCode()).ToNot(Equal(0))
+
+		// With --force it should work
+		resultForce := podmanTest.Podman([]string{"rmi", "-f", alpineId})
+		resultForce.WaitWithDefaultTimeout()
+		Expect(resultForce.ExitCode()).To(Equal(0))
+	})
 })