From 7a3c426899308f8fc6e27d5496a6a9a3b8b5cdd5 Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Thu, 20 Jan 2022 09:55:14 -0500 Subject: [PATCH 1/4] Fix image name parsing with name and digest Image names may contain both tag name and digest. For example `nginx:1.21.5@sha256:7826426c9d8d310c62fc68bcd5e8dde70cb39d4fbbd30eda3b1bd03e35fbde29`. Kustomizations with image transforms will not match these image because the image parser assumes either a tag or digest, but not both. For a real life example of kuberenetes deployments that might need to perform these types of transforms is from the [tekton-pipelines](https://github.com/tektoncd/pipeline) project (see the release.yaml). --- api/filters/imagetag/imagetag_test.go | 29 +++++++++++++++++++++ api/image/image.go | 37 ++++++++++++++++----------- api/image/image_test.go | 36 +++++++++++++++++++++++--- 3 files changed, 84 insertions(+), 18 deletions(-) diff --git a/api/filters/imagetag/imagetag_test.go b/api/filters/imagetag/imagetag_test.go index 96d68366a9..8c20a472ab 100644 --- a/api/filters/imagetag/imagetag_test.go +++ b/api/filters/imagetag/imagetag_test.go @@ -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 { diff --git a/api/image/image.go b/api/image/image.go index 059999062d..ec8cf0eb24 100644 --- a/api/image/image.go +++ b/api/image/image.go @@ -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) } @@ -24,24 +24,31 @@ func IsImageMatched(s, t string) bool { func Split(imageName string) (name string, tag string) { // 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] diff --git a/api/image/image_test.go b/api/image/image_test.go index c3526490e0..86c968e1a6 100644 --- a/api/image/image_test.go +++ b/api/image/image_test.go @@ -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", @@ -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", + }, + { + 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", + }, + { + testName: "with domain and port", + value: "foo.com:443/nginx:1.2.3", + name: "foo.com:443/nginx", + tag: ":1.2.3", }, } From 727093f990e52ddb4e6b3e74a4f37f9649b859c8 Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Sat, 22 Jan 2022 12:39:31 -0500 Subject: [PATCH 2/4] Return digest property from image name parser image.Split now returns 3 fields: name, tag, and digest. The tag and digest fields no longer include their respective delimiters (`:` and `@`). --- api/filters/imagetag/imagetag_test.go | 31 ++++++++++++++++ api/filters/imagetag/updater.go | 19 ++++++++-- api/image/image.go | 53 ++++++++++++++++----------- api/image/image_test.go | 27 +++++++++++--- 4 files changed, 98 insertions(+), 32 deletions(-) diff --git a/api/filters/imagetag/imagetag_test.go b/api/filters/imagetag/imagetag_test.go index 8c20a472ab..feba8035e7 100644 --- a/api/filters/imagetag/imagetag_test.go +++ b/api/filters/imagetag/imagetag_test.go @@ -687,6 +687,37 @@ spec: }, }, }, + "new 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.3.0@sha256:xyz +`, + filter: Filter{ + ImageTag: types.Image{ + Name: "nginx", + NewName: "apache", + NewTag: "1.3.0", + Digest: "sha256:xyz", + }, + }, + fsSlice: []types.FieldSpec{ + { + Path: "spec/image", + }, + }, + }, } for tn, tc := range testCases { diff --git a/api/filters/imagetag/updater.go b/api/filters/imagetag/updater.go index 1c3637cdec..55ec48aa77 100644 --- a/api/filters/imagetag/updater.go +++ b/api/filters/imagetag/updater.go @@ -28,14 +28,25 @@ func (u imageTagUpdater) Filter(rn *yaml.RNode) (*yaml.RNode, error) { return rn, nil } - name, tag := image.Split(value) + name, tag, digest := image.Split(value) if u.ImageTag.NewName != "" { name = u.ImageTag.NewName } - if u.ImageTag.NewTag != "" { - tag = ":" + u.ImageTag.NewTag + // not overriding tag/digest component, keep original + if u.ImageTag.NewTag == "" && u.ImageTag.Digest == "" { + if tag != "" { + tag = ":" + tag + } + if digest != "" { + tag += "@" + digest + } } - if u.ImageTag.Digest != "" { + // overriding tag or digest will replace both original tag and digest values + if u.ImageTag.NewTag != "" && u.ImageTag.Digest != "" { + tag = ":" + u.ImageTag.NewTag + "@" + u.ImageTag.Digest + } else if u.ImageTag.NewTag != "" { + tag = ":" + u.ImageTag.NewTag + } else if u.ImageTag.Digest != "" { tag = "@" + u.ImageTag.Digest } diff --git a/api/image/image.go b/api/image/image.go index ec8cf0eb24..4a88050b48 100644 --- a/api/image/image.go +++ b/api/image/image.go @@ -20,38 +20,47 @@ func IsImageMatched(s, t string) bool { // Split separates and returns the name and tag parts // from the image string using either colon `:` or at `@` separators. -// Note that the returned tag keeps its separator. -func Split(imageName string) (name string, tag string) { +// image reference pattern: [[host[:port]/]component/]component[:tag][@digest] +func Split(imageName string) (name string, tag string, digest string) { // check if image name contains a domain // if domain is present, ignore domain and check for `:` searchName := imageName slashIndex := strings.Index(imageName, "/") if slashIndex > 0 { searchName = imageName[slashIndex:] + } else { + slashIndex = 0 } - 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 { - i = strings.LastIndex(searchName, ":") - if i > 0 && slashIndex > 0 { - i = slashIndex + i - } + id := strings.Index(searchName, "@") + ic := strings.Index(searchName, ":") + + // no tag or digest + if ic < 0 && id < 0 { + return imageName, "", "" + } + + // digest only + if id >= 0 && (id < ic || ic < 0) { + id += slashIndex + name = imageName[:id] + digest = strings.TrimPrefix(imageName[id:], "@") + return name, "", digest } - if i < 0 { - return imageName, "" + // tag and digest + if id >= 0 && ic >= 0 { + id += slashIndex + ic += slashIndex + name = imageName[:ic] + tag = strings.TrimPrefix(imageName[ic:id], ":") + digest = strings.TrimPrefix(imageName[id:], "@") + return name, tag, digest } - name = imageName[:i] - tag = imageName[i:] - return + // tag only + ic += slashIndex + name = imageName[:ic] + tag = strings.TrimPrefix(imageName[ic:], ":") + return name, tag, "" } diff --git a/api/image/image_test.go b/api/image/image_test.go index 86c968e1a6..1154fd7809 100644 --- a/api/image/image_test.go +++ b/api/image/image_test.go @@ -61,50 +61,65 @@ func TestSplit(t *testing.T) { value string name string tag string + digest string }{ { testName: "no tag", value: "nginx", name: "nginx", tag: "", + digest: "", }, { testName: "with tag", value: "nginx:1.2.3", name: "nginx", - tag: ":1.2.3", + tag: "1.2.3", + digest: "", }, { testName: "with digest", value: "nginx@sha256:12345", name: "nginx", - tag: "@sha256:12345", + tag: "", + digest: "sha256:12345", }, { testName: "with tag and digest", value: "nginx:1.2.3@sha256:12345", name: "nginx", - tag: ":1.2.3@sha256:12345", + tag: "1.2.3", + digest: "sha256:12345", }, { testName: "with domain", value: "docker.io/nginx:1.2.3", name: "docker.io/nginx", - tag: ":1.2.3", + tag: "1.2.3", + digest: "", }, { testName: "with domain and port", value: "foo.com:443/nginx:1.2.3", name: "foo.com:443/nginx", - tag: ":1.2.3", + tag: "1.2.3", + digest: "", + }, + { + testName: "with domain, port, and digest", + value: "foo.com:443/nginx:1.2.3@sha256:12345", + name: "foo.com:443/nginx", + tag: "1.2.3", + digest: "sha256:12345", }, } for _, tc := range testCases { t.Run(tc.testName, func(t *testing.T) { - name, tag := Split(tc.value) + name, tag, digest := Split(tc.value) assert.Equal(t, tc.name, name) assert.Equal(t, tc.tag, tag) + assert.Equal(t, tc.digest, digest) }) } } From 1bac439e0fe5ad2a5a8901c41d22afa5ff178e62 Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Tue, 25 Jan 2022 19:35:31 -0500 Subject: [PATCH 3/4] Fix merge file indentation --- api/filters/imagetag/imagetag_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/filters/imagetag/imagetag_test.go b/api/filters/imagetag/imagetag_test.go index fbfefd9e7e..f534dd16c1 100644 --- a/api/filters/imagetag/imagetag_test.go +++ b/api/filters/imagetag/imagetag_test.go @@ -679,7 +679,7 @@ spec: }, }, }, - "mutation tracker": { + "mutation tracker": { input: ` group: apps apiVersion: v1 From 11d585e6d1dbfedbb2cee8271d47a0bbdbb8bf1f Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Tue, 25 Jan 2022 21:27:58 -0500 Subject: [PATCH 4/4] Refactor imagetag updater string builder --- api/filters/imagetag/imagetag_test.go | 34 +++++++++++++++++++++++++-- api/filters/imagetag/updater.go | 29 +++++++++++++---------- api/image/image_test.go | 2 +- 3 files changed, 49 insertions(+), 16 deletions(-) diff --git a/api/filters/imagetag/imagetag_test.go b/api/filters/imagetag/imagetag_test.go index f534dd16c1..752202b4d8 100644 --- a/api/filters/imagetag/imagetag_test.go +++ b/api/filters/imagetag/imagetag_test.go @@ -767,7 +767,7 @@ spec: }, }, }, - "image with tag and digest": { + "image with tag and digest new name": { input: ` apiVersion: example.com/v1 kind: Foo @@ -796,7 +796,37 @@ spec: }, }, }, - "new tag and digest": { + "image with tag and digest new name new tag": { + 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.3.0 +`, + filter: Filter{ + ImageTag: types.Image{ + Name: "nginx", + NewName: "apache", + NewTag: "1.3.0", + }, + }, + fsSlice: []types.FieldSpec{ + { + Path: "spec/image", + }, + }, + }, + "image with tag and digest new name new tag and digest": { input: ` apiVersion: example.com/v1 kind: Foo diff --git a/api/filters/imagetag/updater.go b/api/filters/imagetag/updater.go index df3b4ded0a..45c6c748da 100644 --- a/api/filters/imagetag/updater.go +++ b/api/filters/imagetag/updater.go @@ -34,25 +34,28 @@ func (u imageTagUpdater) SetImageValue(rn *yaml.RNode) error { if u.ImageTag.NewName != "" { name = u.ImageTag.NewName } - // not overriding tag/digest component, keep original - if u.ImageTag.NewTag == "" && u.ImageTag.Digest == "" { - if tag != "" { - tag = ":" + tag - } - if digest != "" { - tag += "@" + digest - } - } + // overriding tag or digest will replace both original tag and digest values if u.ImageTag.NewTag != "" && u.ImageTag.Digest != "" { - tag = ":" + u.ImageTag.NewTag + "@" + u.ImageTag.Digest + tag = u.ImageTag.NewTag + digest = u.ImageTag.Digest } else if u.ImageTag.NewTag != "" { - tag = ":" + u.ImageTag.NewTag + tag = u.ImageTag.NewTag + digest = "" } else if u.ImageTag.Digest != "" { - tag = "@" + u.ImageTag.Digest + tag = "" + digest = u.ImageTag.Digest + } + + // build final image name + if tag != "" { + name += ":" + tag + } + if digest != "" { + name += "@" + digest } - return u.trackableSetter.SetScalar(name + tag)(rn) + return u.trackableSetter.SetScalar(name)(rn) } func (u imageTagUpdater) Filter(rn *yaml.RNode) (*yaml.RNode, error) { diff --git a/api/image/image_test.go b/api/image/image_test.go index 1154fd7809..9c3cd3b0a4 100644 --- a/api/image/image_test.go +++ b/api/image/image_test.go @@ -106,7 +106,7 @@ func TestSplit(t *testing.T) { digest: "", }, { - testName: "with domain, port, and digest", + testName: "with domain, port, tag and digest", value: "foo.com:443/nginx:1.2.3@sha256:12345", name: "foo.com:443/nginx", tag: "1.2.3",