Merge pull request #5250 from dmcgowan/cri-fix-reference-ordering

Fix reference ordering in CRI image store

pkg/cri/store/image/image.go

@@ -210,8 +210,8 @@ func (s *store) add(img Image) error {
 		s.images[img.ID] = img
 		return nil
 	}
-	// Or else, merge the references.
+	// Or else, merge and sort the references.
-	i.References = util.MergeStringSlices(i.References, img.References)
+	i.References = sortReferences(util.MergeStringSlices(i.References, img.References))
 	s.images[img.ID] = i
 	return nil
 }

pkg/cri/store/image/image_test.go

@@ -32,24 +32,24 @@ func TestInternalStore(t *testing.T) {
 		{
 			ID:         "sha256:1123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
 			ChainID:    "test-chain-id-1",
-			References: []string{"ref-1"},
+			References: []string{"containerd.io/ref-1"},
 			Size:       10,
 		},
 		{
 			ID:         "sha256:2123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
 			ChainID:    "test-chain-id-2abcd",
-			References: []string{"ref-2abcd"},
+			References: []string{"containerd.io/ref-2abcd"},
 			Size:       20,
 		},
 		{
 			ID:         "sha256:3123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
-			References: []string{"ref-4a333"},
+			References: []string{"containerd.io/ref-4a333"},
 			ChainID:    "test-chain-id-4a333",
 			Size:       30,
 		},
 		{
 			ID:         "sha256:4123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef",
-			References: []string{"ref-4abcd"},
+			References: []string{"containerd.io/ref-4abcd"},
 			ChainID:    "test-chain-id-4abcd",
 			Size:       40,
 		},
@@ -143,7 +143,7 @@ func TestImageStore(t *testing.T) {
 	image := Image{
 		ID:         id,
 		ChainID:    "test-chain-id-1",
-		References: []string{"ref-1"},
+		References: []string{"containerd.io/ref-1"},
 		Size:       10,
 	}
 	assert := assertlib.New(t)
@@ -159,33 +159,33 @@ func TestImageStore(t *testing.T) {
 		expected []Image
 	}{
 		"nothing should happen if a non-exist ref disappear": {
-			ref:      "ref-2",
+			ref:      "containerd.io/ref-2",
 			image:    nil,
 			expected: []Image{image},
 		},
 		"new ref for an existing image": {
-			ref: "ref-2",
+			ref: "containerd.io/ref-2",
 			image: &Image{
 				ID:         id,
 				ChainID:    "test-chain-id-1",
-				References: []string{"ref-2"},
+				References: []string{"containerd.io/ref-2"},
 				Size:       10,
 			},
 			expected: []Image{
 				{
 					ID:         id,
 					ChainID:    "test-chain-id-1",
-					References: []string{"ref-1", "ref-2"},
+					References: []string{"containerd.io/ref-1", "containerd.io/ref-2"},
 					Size:       10,
 				},
 			},
 		},
 		"new ref for a new image": {
-			ref: "ref-2",
+			ref: "containerd.io/ref-2",
 			image: &Image{
 				ID:         newID,
 				ChainID:    "test-chain-id-2",
-				References: []string{"ref-2"},
+				References: []string{"containerd.io/ref-2"},
 				Size:       20,
 			},
 			expected: []Image{
@@ -193,30 +193,30 @@ func TestImageStore(t *testing.T) {
 				{
 					ID:         newID,
 					ChainID:    "test-chain-id-2",
-					References: []string{"ref-2"},
+					References: []string{"containerd.io/ref-2"},
 					Size:       20,
 				},
 			},
 		},
 		"existing ref point to a new image": {
-			ref: "ref-1",
+			ref: "containerd.io/ref-1",
 			image: &Image{
 				ID:         newID,
 				ChainID:    "test-chain-id-2",
-				References: []string{"ref-1"},
+				References: []string{"containerd.io/ref-1"},
 				Size:       20,
 			},
 			expected: []Image{
 				{
 					ID:         newID,
 					ChainID:    "test-chain-id-2",
-					References: []string{"ref-1"},
+					References: []string{"containerd.io/ref-1"},
 					Size:       20,
 				},
 			},
 		},
 		"existing ref disappear": {
-			ref:      "ref-1",
+			ref:      "containerd.io/ref-1",
 			image:    nil,
 			expected: []Image{},
 		},

pkg/cri/store/image/sort.go

@@ -0,0 +1,75 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package image
+
+import (
+	"sort"
+
+	"github.com/containerd/containerd/reference/docker"
+)
+
+// sortReferences sorts references by refRank then string comparison
+func sortReferences(references []string) []string {
+	var prefs []docker.Reference
+	var bad []string
+
+	for _, ref := range references {
+		pref, err := docker.ParseAnyReference(ref)
+		if err != nil {
+			bad = append(bad, ref)
+		} else {
+			prefs = append(prefs, pref)
+		}
+	}
+	sort.Slice(prefs, func(a, b int) bool {
+		ar := refRank(prefs[a])
+		br := refRank(prefs[b])
+		if ar == br {
+			return prefs[a].String() < prefs[b].String()
+		}
+		return ar < br
+	})
+	sort.Strings(bad)
+	var refs []string
+	for _, pref := range prefs {
+		refs = append(refs, pref.String())
+	}
+	return append(refs, bad...)
+}
+
+// refRank ranks precedence for reference type, preferring higher information references
+// 1. Name + Tag + Digest
+// 2. Name + Tag
+// 3. Name + Digest
+// 4. Name
+// 5. Digest
+// 6. Parse error
+func refRank(ref docker.Reference) uint8 {
+	if _, ok := ref.(docker.Named); ok {
+		if _, ok = ref.(docker.Tagged); ok {
+			if _, ok = ref.(docker.Digested); ok {
+				return 1
+			}
+			return 2
+		}
+		if _, ok = ref.(docker.Digested); ok {
+			return 3
+		}
+		return 4
+	}
+	return 5
+}

pkg/cri/store/image/sort_test.go

@@ -0,0 +1,84 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package image
+
+import (
+	"io"
+	"io/ioutil"
+	"math/rand"
+	"testing"
+
+	"github.com/opencontainers/go-digest"
+)
+
+func TestReferenceSorting(t *testing.T) {
+	digested := func(seed int64) string {
+		b, err := ioutil.ReadAll(io.LimitReader(rand.New(rand.NewSource(seed)), 64))
+		if err != nil {
+			panic(err)
+		}
+		return digest.FromBytes(b).String()
+	}
+	// Add z. prefix to string sort after "sha256:"
+	r1 := func(name, tag string, seed int64) string {
+		return "z.containerd.io/" + name + ":" + tag + "@" + digested(seed)
+	}
+	r2 := func(name, tag string) string {
+		return "z.containerd.io/" + name + ":" + tag
+	}
+	r3 := func(name string, seed int64) string {
+		return "z.containerd.io/" + name + "@" + digested(seed)
+	}
+
+	for i, tc := range []struct {
+		unsorted []string
+		expected []string
+	}{
+		{
+			unsorted: []string{r2("name", "latest"), r3("name", 1), r1("name", "latest", 1)},
+			expected: []string{r1("name", "latest", 1), r2("name", "latest"), r3("name", 1)},
+		},
+		{
+			unsorted: []string{"can't parse this:latest", r3("name", 1), r2("name", "latest")},
+			expected: []string{r2("name", "latest"), r3("name", 1), "can't parse this:latest"},
+		},
+		{
+			unsorted: []string{digested(1), r3("name", 1), r2("name", "latest")},
+			expected: []string{r2("name", "latest"), r3("name", 1), digested(1)},
+		},
+		{
+			unsorted: []string{r2("name", "tag2"), r2("name", "tag3"), r2("name", "tag1")},
+			expected: []string{r2("name", "tag1"), r2("name", "tag2"), r2("name", "tag3")},
+		},
+		{
+			unsorted: []string{r2("name-2", "tag"), r2("name-3", "tag"), r2("name-1", "tag")},
+			expected: []string{r2("name-1", "tag"), r2("name-2", "tag"), r2("name-3", "tag")},
+		},
+	} {
+		sorted := sortReferences(tc.unsorted)
+		if len(sorted) != len(tc.expected) {
+			t.Errorf("[%d]: Mismatched sized, got %d, expected %d", i, len(sorted), len(tc.expected))
+			continue
+		}
+		for j := range sorted {
+			if sorted[j] != tc.expected[j] {
+				t.Errorf("[%d]: Wrong value at %d, got %q, expected %q", i, j, sorted[j], tc.expected[j])
+				break
+			}
+		}
+	}
+}