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.

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

Fix image name parsing with tag and digest #4406

Merged
merged 5 commits into from Feb 16, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 29 additions & 0 deletions api/filters/imagetag/imagetag_test.go
Expand Up @@ -658,6 +658,35 @@ spec:
},
},
},
"image with tag and digest": {
input: `
apiVersion: example.com/v1
kind: Foo
metadata:
name: instance
spec:
image: nginx:1.2.1@sha256:46d5b90a7f4e9996351ad893a26bcbd27216676ad4d5316088ce351fb2c2c3dd
`,
expectedOutput: `
apiVersion: example.com/v1
kind: Foo
metadata:
name: instance
spec:
image: apache:1.2.1@sha256:46d5b90a7f4e9996351ad893a26bcbd27216676ad4d5316088ce351fb2c2c3dd
`,
filter: Filter{
ImageTag: types.Image{
Name: "nginx",
NewName: "apache",
},
},
fsSlice: []types.FieldSpec{
{
Path: "spec/image",
},
},
},
}

for tn, tc := range testCases {
Expand Down
37 changes: 22 additions & 15 deletions api/image/image.go
Expand Up @@ -14,7 +14,7 @@ func IsImageMatched(s, t string) bool {
// Tag values are limited to [a-zA-Z0-9_.{}-].
// Some tools like Bazel rules_k8s allow tag patterns with {} characters.
// More info: https://github.com/bazelbuild/rules_k8s/pull/423
pattern, _ := regexp.Compile("^" + t + "(@sha256)?(:[a-zA-Z0-9_.{}-]*)?$")
pattern, _ := regexp.Compile("^" + t + "(:[a-zA-Z0-9_.{}-]*)?(@sha256:[a-zA-Z0-9_.{}-]*)?$")
return pattern.MatchString(s)
}

Expand All @@ -24,24 +24,31 @@ func IsImageMatched(s, t string) bool {
func Split(imageName string) (name string, tag string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this method to return three things - a name, tag, and digest? Below in one of the tests I see tag: :1.2.3@sha256:12345 which doesn't seem correct. We should differentiate the tag and digest where possible to help support cases where there are both.

// check if image name contains a domain
// if domain is present, ignore domain and check for `:`
ic := -1
if slashIndex := strings.Index(imageName, "/"); slashIndex < 0 {
ic = strings.LastIndex(imageName, ":")
searchName := imageName
slashIndex := strings.Index(imageName, "/")
if slashIndex > 0 {
searchName = imageName[slashIndex:]
}

i := strings.LastIndex(imageName, "@")
if i > 0 {
ic := strings.Index(searchName[:i], ":")
if ic > 0 {
if slashIndex > 0 {
i = slashIndex + ic
} else {
i = ic
}
}
} else {
lastIc := strings.LastIndex(imageName[slashIndex:], ":")
// set ic only if `:` is present
if lastIc > 0 {
ic = slashIndex + lastIc
i = strings.LastIndex(searchName, ":")
if i > 0 && slashIndex > 0 {
i = slashIndex + i
}
}
ia := strings.LastIndex(imageName, "@")
if ic < 0 && ia < 0 {
return imageName, ""
}

i := ic
if ia > 0 {
i = ia
if i < 0 {
return imageName, ""
}

name = imageName[:i]
Expand Down
36 changes: 33 additions & 3 deletions api/image/image_test.go
Expand Up @@ -23,11 +23,23 @@ func TestIsImageMatched(t *testing.T) {
isMatched: true,
},
{
testName: "name is match",
testName: "name is match with tag",
value: "nginx:12345",
name: "nginx",
isMatched: true,
},
{
testName: "name is match with digest",
value: "nginx@sha256:xyz",
name: "nginx",
isMatched: true,
},
{
testName: "name is match with tag and digest",
value: "nginx:12345@sha256:xyz",
name: "nginx",
isMatched: true,
},
{
testName: "name is not a match",
value: "apache:12345",
Expand Down Expand Up @@ -64,9 +76,27 @@ func TestSplit(t *testing.T) {
},
{
testName: "with digest",
value: "nginx@12345",
value: "nginx@sha256:12345",
name: "nginx",
tag: "@12345",
tag: "@sha256:12345",
Copy link
Contributor

Choose a reason for hiding this comment

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

Aligned with the above suggestion to change the Split function to return the tag and digest separately, I'd like to see this test look like the following:

			testName: "with digest",
			value:    "nginx@sha256:12345",
			name:     "nginx",
			digest:   "sha256:12345"

},
{
testName: "with tag and digest",
value: "nginx:1.2.3@sha256:12345",
name: "nginx",
tag: ":1.2.3@sha256:12345",
},
{
testName: "with domain",
value: "docker.io/nginx:1.2.3",
name: "docker.io/nginx",
tag: ":1.2.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it doesn't make sense to include the leading colon in the split. The tag is "1.2.3". Could you change that?

},
{
testName: "with domain and port",
value: "foo.com:443/nginx:1.2.3",
name: "foo.com:443/nginx",
tag: ":1.2.3",
},
}

Expand Down