Fix reference ordering in CRI image store
Currently image references end up being stored in a random order due to the way maps are iterated through in Go. This leads to inconsistent identifiers being resolved when a single reference is needed to identify an image and the ordering of the references is used for the selection. Sort references in a consistent and ranked manner, from higher information formats to lower. Note: A `name + tag` reference is considered higher information than a `name + digest` reference since a registry may be used to resolve the digest from a `name + tag` reference. Signed-off-by: Derek McGowan <derek@mcg.dev>
This commit is contained in:
Derek McGowan
parent
edf6484165
commit
0886ceaea2
@ -210,8 +210,8 @@ func (s *store) add(img Image) error {
|
||||
s.images[img.ID] = img
|
||||
return nil
|
||||
}
|
||||
// Or else, merge the references.
|
||||
i.References = util.MergeStringSlices(i.References, img.References)
|
||||
// Or else, merge and sort the references.
|
||||
i.References = sortReferences(util.MergeStringSlices(i.References, img.References))
|
||||
s.images[img.ID] = i
|
||||
return nil
|
||||
}
|
||||
|
||||
@ -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{},
|
||||
},
|
||||
|
||||
75
pkg/cri/store/image/sort.go
Normal file
75
pkg/cri/store/image/sort.go
Normal file
@ -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
|
||||
}
|
||||
84
pkg/cri/store/image/sort_test.go
Normal file
84
pkg/cri/store/image/sort_test.go
Normal file
@ -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
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user