Merge pull request #23649 from dmcgowan/image-delete-last-repo-ref

Update rmi logic for canonical references

daemon/image_delete.go

@@ -104,27 +104,34 @@ func (daemon *Daemon) ImageDelete(imageRef string, force, prune bool) ([]types.I
 
 		repoRefs = daemon.referenceStore.References(imgID)
 
-		// If this is a tag reference and all the remaining references
-		// to this image are digest references, delete the remaining
-		// references so that they don't prevent removal of the image.
+		// If a tag reference was removed and the only remaining
+		// references to the same repository are digest references,
+		// then clean up those digest references.
 		if _, isCanonical := parsedRef.(reference.Canonical); !isCanonical {
-			foundTagRef := false
+			foundRepoTagRef := false
 			for _, repoRef := range repoRefs {
-				if _, repoRefIsCanonical := repoRef.(reference.Canonical); !repoRefIsCanonical {
-					foundTagRef = true
+				if _, repoRefIsCanonical := repoRef.(reference.Canonical); !repoRefIsCanonical && parsedRef.Name() == repoRef.Name() {
+					foundRepoTagRef = true
 					break
 				}
 			}
-			if !foundTagRef {
+			if !foundRepoTagRef {
+				// Remove canonical references from same repository
+				remainingRefs := []reference.Named{}
 				for _, repoRef := range repoRefs {
-					if _, err := daemon.removeImageRef(repoRef); err != nil {
-						return records, err
-					}
+					if _, repoRefIsCanonical := repoRef.(reference.Canonical); repoRefIsCanonical && parsedRef.Name() == repoRef.Name() {
+						if _, err := daemon.removeImageRef(repoRef); err != nil {
+							return records, err
+						}
+
+						untaggedRecord := types.ImageDelete{Untagged: repoRef.String()}
+						records = append(records, untaggedRecord)
+					} else {
+						remainingRefs = append(remainingRefs, repoRef)
 
-					untaggedRecord := types.ImageDelete{Untagged: repoRef.String()}
-					records = append(records, untaggedRecord)
+					}
 				}
-				repoRefs = []reference.Named{}
+				repoRefs = remainingRefs
 			}
 		}
 
@@ -135,11 +142,10 @@ func (daemon *Daemon) ImageDelete(imageRef string, force, prune bool) ([]types.I
 
 		removedRepositoryRef = true
 	} else {
-		// If an ID reference was given AND there is exactly one
-		// repository reference to the image then we will want to
-		// remove that reference.
-		// FIXME: Is this the behavior we want?
-		if len(repoRefs) == 1 {
+		// If an ID reference was given AND there is at most one tag
+		// reference to the image AND all references are within one
+		// repository, then remove all references.
+		if isSingleReference(repoRefs) {
 			c := conflictHard
 			if !force {
 				c |= conflictSoft &^ conflictActiveReference
@@ -148,21 +154,48 @@ func (daemon *Daemon) ImageDelete(imageRef string, force, prune bool) ([]types.I
 				return nil, conflict
 			}
 
-			parsedRef, err := daemon.removeImageRef(repoRefs[0])
-			if err != nil {
-				return nil, err
-			}
+			for _, repoRef := range repoRefs {
+				parsedRef, err := daemon.removeImageRef(repoRef)
+				if err != nil {
+					return nil, err
+				}
 
-			untaggedRecord := types.ImageDelete{Untagged: parsedRef.String()}
+				untaggedRecord := types.ImageDelete{Untagged: parsedRef.String()}
 
-			daemon.LogImageEvent(imgID.String(), imgID.String(), "untag")
-			records = append(records, untaggedRecord)
+				daemon.LogImageEvent(imgID.String(), imgID.String(), "untag")
+				records = append(records, untaggedRecord)
+			}
 		}
 	}
 
 	return records, daemon.imageDeleteHelper(imgID, &records, force, prune, removedRepositoryRef)
 }
 
+// isSingleReference returns true when all references are from one repository
+// and there is at most one tag. Returns false for empty input.
+func isSingleReference(repoRefs []reference.Named) bool {
+	if len(repoRefs) <= 1 {
+		return len(repoRefs) == 1
+	}
+	var singleRef reference.Named
+	canonicalRefs := map[string]struct{}{}
+	for _, repoRef := range repoRefs {
+		if _, isCanonical := repoRef.(reference.Canonical); isCanonical {
+			canonicalRefs[repoRef.Name()] = struct{}{}
+		} else if singleRef == nil {
+			singleRef = repoRef
+		} else {
+			return false
+		}
+	}
+	if singleRef == nil {
+		// Just use first canonical ref
+		singleRef = repoRefs[0]
+	}
+	_, ok := canonicalRefs[singleRef.Name()]
+	return len(canonicalRefs) == 1 && ok
+}
+
 // isImageIDPrefix returns whether the given possiblePrefix is a prefix of the
 // given imageID.
 func isImageIDPrefix(imageID, possiblePrefix string) bool {

integration-cli/docker_cli_by_digest_test.go

@@ -380,9 +380,14 @@ func (s *DockerRegistrySuite) TestDeleteImageByIDOnlyPulledByDigest(c *check.C)
 	dockerCmd(c, "pull", imageReference)
 	// just in case...
 
+	dockerCmd(c, "tag", imageReference, repoName+":sometag")
+
 	imageID := inspectField(c, imageReference, "Id")
 
 	dockerCmd(c, "rmi", imageID)
+
+	_, err = inspectFieldWithError(imageID, "Id")
+	c.Assert(err, checker.NotNil, check.Commentf("image should have been deleted"))
 }
 
 func (s *DockerRegistrySuite) TestDeleteImageWithDigestAndTag(c *check.C) {
@@ -412,6 +417,40 @@ func (s *DockerRegistrySuite) TestDeleteImageWithDigestAndTag(c *check.C) {
 	c.Assert(err, checker.NotNil, check.Commentf("image should have been deleted"))
 }
 
+func (s *DockerRegistrySuite) TestDeleteImageWithDigestAndMultiRepoTag(c *check.C) {
+	pushDigest, err := setupImage(c)
+	c.Assert(err, checker.IsNil, check.Commentf("error setting up image"))
+
+	repo2 := fmt.Sprintf("%s/%s", repoName, "repo2")
+
+	// pull from the registry using the <name>@<digest> reference
+	imageReference := fmt.Sprintf("%s@%s", repoName, pushDigest)
+	dockerCmd(c, "pull", imageReference)
+
+	imageID := inspectField(c, imageReference, "Id")
+
+	repoTag := repoName + ":sometag"
+	repoTag2 := repo2 + ":othertag"
+	dockerCmd(c, "tag", imageReference, repoTag)
+	dockerCmd(c, "tag", imageReference, repoTag2)
+
+	dockerCmd(c, "rmi", repoTag)
+
+	// rmi should have deleted repoTag and image reference, but left repoTag2
+	inspectField(c, repoTag2, "Id")
+	_, err = inspectFieldWithError(imageReference, "Id")
+	c.Assert(err, checker.NotNil, check.Commentf("image digest reference should have been removed"))
+
+	_, err = inspectFieldWithError(repoTag, "Id")
+	c.Assert(err, checker.NotNil, check.Commentf("image tag reference should have been removed"))
+
+	dockerCmd(c, "rmi", repoTag2)
+
+	// rmi should have deleted the tag, the digest reference, and the image itself
+	_, err = inspectFieldWithError(imageID, "Id")
+	c.Assert(err, checker.NotNil, check.Commentf("image should have been deleted"))
+}
+
 // TestPullFailsWithAlteredManifest tests that a `docker pull` fails when
 // we have modified a manifest blob and its digest cannot be verified.
 // This is the schema2 version of the test.