fix: handling untagged images when tagging and removing (#1511 + #1512) (#1513)

Author: iwilltry42

pkg/cli/images_rm.go

@@ -2,12 +2,9 @@ package cli
 
 import (
 	"fmt"
-	"strings"
 
 	cli "github.com/acorn-io/acorn/pkg/cli/builder"
 	"github.com/acorn-io/acorn/pkg/client"
-	"github.com/acorn-io/acorn/pkg/tags"
-	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/spf13/cobra"
 )
 
@@ -34,18 +31,7 @@ func (a *ImageDelete) Run(cmd *cobra.Command, args []string) error {
 	}
 
 	for _, image := range args {
-		opts := []name.Option{name.WithDefaultRegistry("")}
-
-		if strings.HasPrefix("sha256:", image) || tags.SHAPermissivePrefixPattern.MatchString(image) {
-			opts = append(opts, name.WithDefaultTag(""))
-		}
-
-		// normalize image name (adding :latest if no tag is specified and it's not a digest or potential ID)
-		ref, err := name.ParseReference(image, opts...)
-		if err != nil {
-			return err
-		}
-		deleted, err := c.ImageDelete(cmd.Context(), strings.TrimSuffix(ref.Name(), ":"), &client.ImageDeleteOptions{Force: a.Force})
+		deleted, err := c.ImageDelete(cmd.Context(), image, &client.ImageDeleteOptions{Force: a.Force})
 		if err != nil {
 			return fmt.Errorf("deleting %s: %w", image, err)
 		}

pkg/server/registry/apigroups/acorn/images/storage_test.go

@@ -14,39 +14,84 @@ func TestFindMatchingImage(t *testing.T) {
 		},
 		{
 			Digest: "sha256:987654321",
+			Tags: []string{
+				"foo:latest",
+			},
 		},
 		{
 			Digest: "sha256:123409876",
+			Tags: []string{
+				"foo/bar",
+				"foo/bar:dev",
+			},
 		},
 		{
 			Digest: "sha256:1a6c64d2ccd0bb035f9c8196d3bfe72a7fdbddc4530dfcb3ab2a0ab8afb57eeb",
 			Tags: []string{
 				"foo/bar",
+				"spam/eggs:v1",
 			},
 		},
 	}
 	il := apiv1.ImageList{
 		Items: images,
 	}
 
+	// pass: digest prefix
 	image, ref, err := findImageMatch(il, "12345")
 	if err != nil {
 		t.Fatal(err)
 	}
 	assert.Equal(t, "sha256:12345678", image.Digest)
 	assert.Equal(t, "", ref)
 
+	// err: ambiguous digest prefix
 	_, _, err = findImageMatch(il, "123")
 	assert.Error(t, err)
+	assert.Equal(t, "Image identifier 123 is not unique", err.Error())
 
+	// pass: full digest reference
 	image, ref, err = findImageMatch(il, "foo/bar@sha256:1a6c64d2ccd0bb035f9c8196d3bfe72a7fdbddc4530dfcb3ab2a0ab8afb57eeb")
 	if err != nil {
 		t.Fatal(err)
 	}
 	assert.Equal(t, "sha256:1a6c64d2ccd0bb035f9c8196d3bfe72a7fdbddc4530dfcb3ab2a0ab8afb57eeb", image.Digest)
 	assert.Equal(t, "foo/bar", ref)
 
+	// err: not found by full digest reference
 	_, _, err = findImageMatch(il, "ghcr.io/acorn-io/library/hello-world@sha256:1a6c64d2ccd0bb035f9c8196d3bfe72a7fdbddc4530dfcb3ab2a0ab8afb57eeb")
 	assert.Error(t, err)
 	assert.Equal(t, "images.api.acorn.io \"ghcr.io/acorn-io/library/hello-world@sha256:1a6c64d2ccd0bb035f9c8196d3bfe72a7fdbddc4530dfcb3ab2a0ab8afb57eeb\" not found", err.Error())
+
+	// err: ambiguous reg/repo reference
+	_, _, err = findImageMatch(il, "foo/bar")
+	assert.Error(t, err)
+	assert.Equal(t, "Image identifier foo/bar is not unique", err.Error())
+
+	// pass: full tag reference
+	image, ref, err = findImageMatch(il, "spam/eggs:v1")
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.Equal(t, "sha256:1a6c64d2ccd0bb035f9c8196d3bfe72a7fdbddc4530dfcb3ab2a0ab8afb57eeb", image.Digest)
+	assert.Equal(t, "spam/eggs:v1", ref)
+
+	// pass: repo without tag, defaulting to :latest
+	image, ref, err = findImageMatch(il, "foo")
+	if err != nil {
+		t.Fatal(err)
+	}
+	assert.Equal(t, "sha256:987654321", image.Digest)
+	assert.Equal(t, "foo:latest", ref)
+
+	// err: tiny string that's neither a digest nor an existing tag
+	// (it is a valid image tag, but not an acorn image tag item)
+	_, _, err = findImageMatch(il, "dev")
+	assert.Error(t, err)
+	assert.Equal(t, "images.api.acorn.io \"dev\" not found", err.Error())
+
+	// err: same as above, but with repo part
+	_, _, err = findImageMatch(il, "bar:dev")
+	assert.Error(t, err)
+	assert.Equal(t, "images.api.acorn.io \"bar:dev\" not found", err.Error())
 }

pkg/server/registry/apigroups/acorn/images/strategy.go

@@ -191,29 +191,31 @@ func (s *Strategy) findImage(ctx context.Context, namespace, imageName string) (
 // - tag name (with default): <registry>/<repo> or <repo> -> Will be matched against the default tag (:latest)
 //   - Note: if we get some string here, that matches the SHAPermissivePrefixPattern, it could be both a digest or a name without a tag
 //     so we will try to match it against the default tag (:latest) first and if that fails, we treat it as a digest(-prefix)
-func findImageMatch(images apiv1.ImageList, imageName string) (*apiv1.Image, string, error) {
+func findImageMatch(images apiv1.ImageList, search string) (*apiv1.Image, string, error) {
 	var (
 		repoDigest     name.Digest
 		digest         string
 		digestPrefix   string
 		tagName        string
 		tagNameDefault string
+		canBeMultiple  bool // if true, we will not return on first match
 	)
-	if strings.HasPrefix(imageName, "sha256:") {
-		digest = imageName
-	} else if tags2.SHAPattern.MatchString(imageName) {
-		digest = "sha256:" + imageName
-		tagNameDefault = imageName
-	} else if tags2.SHAPermissivePrefixPattern.MatchString(imageName) {
-		digestPrefix = "sha256:" + imageName
-		tagNameDefault = imageName
+	if strings.HasPrefix(search, "sha256:") {
+		digest = search
+	} else if tags2.SHAPattern.MatchString(search) {
+		digest = "sha256:" + search
+		tagNameDefault = search // this could as well be some name without registry/repo path and tag
+	} else if tags2.SHAPermissivePrefixPattern.MatchString(search) {
+		digestPrefix = "sha256:" + search
+		tagNameDefault = search // this could as well be some name without registry/repo path and tag
 	} else {
-		ref, err := name.ParseReference(imageName, name.WithDefaultRegistry(""), name.WithDefaultTag(""))
+		ref, err := name.ParseReference(search, name.WithDefaultRegistry(""), name.WithDefaultTag(""))
 		if err != nil {
 			return nil, "", err
 		}
 		if ref.Identifier() == "" {
-			tagNameDefault = ref.Name()
+			tagNameDefault = ref.Name() // some name without a tag, so we will try to match it against the default tag (:latest)
+			canBeMultiple = true
 		} else if dig, ok := ref.(name.Digest); ok {
 			repoDigest = dig
 		} else {
@@ -231,28 +233,31 @@ func findImageMatch(images apiv1.ImageList, imageName string) (*apiv1.Image, str
 	}
 
 	var matchedImage apiv1.Image
+	var matchedTag string
 	for _, image := range images.Items {
+		// >>> match by tag name with default tag (:latest)
 		if tagNameDefault != "" {
 			for _, tag := range image.Tags {
 				if tag == tagNameDefault {
-					return &image, "", nil
+					return &image, tag, nil
 				}
 			}
 		}
 
+		// >>> match by digest or digest prefix
 		if image.Digest == digest {
 			return &image, "", nil
 		} else if digestPrefix != "" && strings.HasPrefix(image.Digest, digestPrefix) {
 			if matchedImage.Digest != "" && matchedImage.Digest != image.Digest {
-				reason := fmt.Sprintf("Image identifier %v is not unique", imageName)
-				return nil, "", apierrors.NewBadRequest(reason)
+				return nil, "", apierrors.NewBadRequest(fmt.Sprintf("Image identifier %v is not unique", search))
 			}
 			matchedImage = image
 		}
 
+		// >>> match by repo digest
+		// this returns an image which matches the digest and has at least one tag
+		// which matches the repo part of the repo digest.
 		if repoDigest.Name() != "" && image.Digest == repoDigest.DigestStr() {
-			// Matching by repo digest returns an image which matches the digest and has at least one tag
-			// which matches the repo part of the repo digest.
 			for _, tag := range image.Tags {
 				imageParsedTag, err := name.NewTag(tag, name.WithDefaultRegistry(""))
 				if err != nil {
@@ -264,27 +269,42 @@ func findImageMatch(images apiv1.ImageList, imageName string) (*apiv1.Image, str
 			}
 		}
 
-		for i, tag := range image.Tags {
-			if tag == imageName {
-				return &image, image.Tags[i], nil
+		// >>> match by tag name
+		for _, tag := range image.Tags {
+			if tag == search {
+				if !canBeMultiple {
+					return &image, tag, nil
+				}
+				if matchedImage.Digest != "" && matchedImage.Digest != image.Digest {
+					return nil, "", apierrors.NewBadRequest(fmt.Sprintf("Image identifier %v is not unique", search))
+				}
+				matchedImage = image
+				matchedTag = tag
 			} else if tag != "" {
-				imageParsedTag, err := name.NewTag(tag, name.WithDefaultRegistry(""))
+				imageParsedTag, err := name.NewTag(tag, name.WithDefaultRegistry(""), name.WithDefaultTag("")) // no default here, as we also have repo-only tag items
 				if err != nil {
 					continue
 				}
 				if imageParsedTag.Name() == tagName {
-					return &image, tag, nil
+					if !canBeMultiple {
+						return &image, tag, nil
+					}
+					if matchedImage.Digest != "" && matchedImage.Digest != image.Digest {
+						return nil, "", apierrors.NewBadRequest(fmt.Sprintf("Image identifier %v is not unique", search))
+					}
+					matchedImage = image
+					matchedTag = tag
 				}
 			}
 		}
 	}
 
 	if matchedImage.Digest != "" {
-		return &matchedImage, "", nil
+		return &matchedImage, matchedTag, nil
 	}
 
 	return nil, "", apierrors.NewNotFound(schema.GroupResource{
 		Group:    api.Group,
 		Resource: "images",
-	}, imageName)
+	}, search)
 }

pkg/server/registry/apigroups/acorn/images/tag.go

@@ -95,34 +95,34 @@ func (t *TagStrategy) ImageTag(ctx context.Context, namespace, imageName string,
 		set.Insert(imageRef.Context().Name())
 	} else {
 		if set.Has(imageRef.Context().Name()) {
-			// we're inserting a tag, so we can remove any digest-only entry
+			// we're inserting a tag, so we can remove any repo-only entry
 			set.Delete(imageRef.Context().Name())
 		}
 		set.Insert(imageRef.Name())
-	}
 
-	// remove the tag from any other image
-	hasChanged := false
-	for _, img := range imageList.Items {
-		if img.Name == image.Name {
-			continue
-		}
-		res, err = normalizeTags(img.Tags, false)
-		if err != nil {
-			return nil, err
-		}
-		for i, tag := range res {
-			if set.Has(tag) {
-				img.Tags = append(img.Tags[:i], img.Tags[i+1:]...)
-				hasChanged = true
+		// remove the tag from any other image (not if it's a repo-only reference, e.g. pulled by digest, as those can be duplicated)
+		hasChanged := false
+		for _, img := range imageList.Items {
+			if img.Name == image.Name {
+				continue
 			}
-		}
-		if hasChanged {
-			err = t.client.Update(ctx, &img)
+			res, err = normalizeTags(img.Tags, false)
 			if err != nil {
 				return nil, err
 			}
-			hasChanged = false
+			for i, tag := range res {
+				if set.Has(tag) {
+					img.Tags = append(img.Tags[:i], img.Tags[i+1:]...)
+					hasChanged = true
+				}
+			}
+			if hasChanged {
+				err = t.client.Update(ctx, &img)
+				if err != nil {
+					return nil, err
+				}
+				hasChanged = false
+			}
 		}
 	}