Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix load docker-image re-tagging with prefix #2955

Merged
merged 1 commit into from
Oct 6, 2022
Merged

Fix load docker-image re-tagging with prefix #2955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions pkg/cmd/kind/load/docker-image/docker-image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func runE(logger log.Logger, flags *flagpole, args []string) error {
imageID := imageIDs[i]
processed := false
for _, node := range candidateNodes {
exists, reTagRequired := checkIfImageReTagRequired(node, imageID, imageName, nodeutils.ImageTags)
exists, reTagRequired, imageName := checkIfImageReTagRequired(node, imageID, imageName, nodeutils.ImageTags)
if exists && !reTagRequired {
continue
}
Expand Down Expand Up @@ -241,15 +241,15 @@ func removeDuplicates(slice []string) []string {
}

// checkIfImageExists makes sure we only perform the reverse lookup of the ImageID to tag map
func checkIfImageReTagRequired(node nodes.Node, imageID, imageName string, tagFetcher imageTagFetcher) (exists, reTagRequired bool) {
func checkIfImageReTagRequired(node nodes.Node, imageID, imageName string, tagFetcher imageTagFetcher) (exists, reTagRequired bool, sanitizedImage string) {
tags, err := tagFetcher(node, imageID)
if len(tags) == 0 || err != nil {
exists = false
return
}
exists = true
imageName = sanitizeImage(imageName)
if ok := tags[imageName]; ok {
sanitizedImage = sanitizeImage(imageName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if we should always sanitize the images names?

if ok := tags[sanitizedImage]; ok {
reTagRequired = false
return
}
Expand Down
50 changes: 35 additions & 15 deletions pkg/cmd/kind/load/docker-image/docker-image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,10 @@ func Test_checkIfImageReTagRequired(t *testing.T) {
tags map[string]bool
err error
}
imageID string
imageName string
returnValues []bool
imageID string
imageName string
returnValues []bool
sanitizedImage string
}{
{
name: "image is already present",
Expand All @@ -128,9 +129,10 @@ func Test_checkIfImageReTagRequired(t *testing.T) {
},
nil,
},
imageID: "sha256:fd3fd9ab134a864eeb7b2c073c0d90192546f597c60416b81fc4166cca47f29a",
imageName: "k8s.io/image1:tag1",
returnValues: []bool{true, false},
imageID: "sha256:fd3fd9ab134a864eeb7b2c073c0d90192546f597c60416b81fc4166cca47f29a",
imageName: "k8s.io/image1:tag1",
returnValues: []bool{true, false},
sanitizedImage: "k8s.io/image1:tag1",
},
{
name: "re-tag is required",
Expand All @@ -144,9 +146,26 @@ func Test_checkIfImageReTagRequired(t *testing.T) {
},
nil,
},
imageID: "sha256:fd3fd9ab134a864eeb7b2c073c0d90192546f597c60416b81fc4166cca47f29a",
imageName: "k8s.io/image1:tag2",
returnValues: []bool{true, true},
imageID: "sha256:fd3fd9ab134a864eeb7b2c073c0d90192546f597c60416b81fc4166cca47f29a",
imageName: "k8s.io/image1:tag2",
returnValues: []bool{true, true},
sanitizedImage: "k8s.io/image1:tag2",
},
{
name: "re-tag is required with docker.io prefix",
imageTags: struct {
tags map[string]bool
err error
}{
map[string]bool{
"docker.io/foo/image1:tag1": true,
},
nil,
},
imageID: "sha256:fd3fd9ab134a864eeb7b2c073c0d90192546f597c60416b81fc4166cca47f29a",
imageName: "foo/image1:tag2",
returnValues: []bool{true, true},
sanitizedImage: "docker.io/foo/image1:tag2",
},
{
name: "image tag fetch failed",
Expand All @@ -157,9 +176,10 @@ func Test_checkIfImageReTagRequired(t *testing.T) {
map[string]bool{},
errors.New("some runtime error"),
},
imageID: "sha256:fd3fd9ab134a864eeb7b2c073c0d90192546f597c60416b81fc4166cca47f29a",
imageName: "k8s.io/image1:tag2",
returnValues: []bool{false, false},
imageID: "sha256:fd3fd9ab134a864eeb7b2c073c0d90192546f597c60416b81fc4166cca47f29a",
imageName: "k8s.io/image1:tag2",
returnValues: []bool{false, false},
sanitizedImage: "",
},
}

Expand All @@ -169,11 +189,11 @@ func Test_checkIfImageReTagRequired(t *testing.T) {
// checkIfImageReTagRequired doesn't use the `nodes.Node` type for anything. So
// passing a nil value here should be fine as the other two functions that use the
// nodes.Node has been stubbed out already
exists, reTagRequired := checkIfImageReTagRequired(nil, tc.imageID, tc.imageName, func(n nodes.Node, s string) (map[string]bool, error) {
exists, reTagRequired, sanitizedImage := checkIfImageReTagRequired(nil, tc.imageID, tc.imageName, func(n nodes.Node, s string) (map[string]bool, error) {
return tc.imageTags.tags, tc.imageTags.err
})
if exists != tc.returnValues[0] || reTagRequired != tc.returnValues[1] {
t.Errorf("checkIfImageReTagRequired failed. Expected: %v, got: [%v, %v]", tc.returnValues, exists, reTagRequired)
if exists != tc.returnValues[0] || reTagRequired != tc.returnValues[1] || sanitizedImage != tc.sanitizedImage {
t.Errorf("checkIfImageReTagRequired failed. Expected: [%v,%v,%v], got: [%v, %v, %v]", tc.returnValues[0], tc.returnValues[1], tc.sanitizedImage, exists, reTagRequired, sanitizedImage)
}
})
}
Expand Down