From 9e53fd85ee4b13f52cbbaf70e39cae98d51c1fa3 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 17 May 2018 21:58:19 +0200 Subject: [PATCH 1/5] Skip fully qualified names when replacing images names. Fix #565 Signed-off-by: David Gageot --- pkg/skaffold/deploy/kubectl.go | 21 +++++--- pkg/skaffold/deploy/kubectl_test.go | 80 ++++++++++++++++++++++++----- 2 files changed, 80 insertions(+), 21 deletions(-) diff --git a/pkg/skaffold/deploy/kubectl.go b/pkg/skaffold/deploy/kubectl.go index 9f0ca875f73..f3885086e5b 100644 --- a/pkg/skaffold/deploy/kubectl.go +++ b/pkg/skaffold/deploy/kubectl.go @@ -327,10 +327,12 @@ func recursiveReplaceImage(i interface{}, replacements map[string]*replacement) case map[interface{}]interface{}: for k, v := range t { if k.(string) == "image" { - name := removeTag(v.(string)) - if img, present := replacements[name]; present { - t[k] = img.tag - img.found = true + name, tag := splitTag(v.(string)) + if tag == "" || tag == "latest" { + if img, present := replacements[name]; present { + t[k] = img.tag + img.found = true + } } } else { recursiveReplaceImage(v, replacements) @@ -339,7 +341,12 @@ func recursiveReplaceImage(i interface{}, replacements map[string]*replacement) } } -func removeTag(image string) string { - re := regexp.MustCompile(":[^/]+$") - return re.ReplaceAllString(image, "") +func splitTag(image string) (string, string) { + re := regexp.MustCompile(`(.*):([^/]+)$`) + matches := re.FindStringSubmatch(image) + + if len(matches) == 3 { + return matches[1], matches[2] + } + return image, "" } diff --git a/pkg/skaffold/deploy/kubectl_test.go b/pkg/skaffold/deploy/kubectl_test.go index 5b27a3f0cd5..0038e354e66 100644 --- a/pkg/skaffold/deploy/kubectl_test.go +++ b/pkg/skaffold/deploy/kubectl_test.go @@ -255,7 +255,7 @@ spec: expected: manifestList{}, }, { - description: "ignore tag", + description: "skipped tagged", builds: []build.Build{{ ImageName: "gcr.io/k8s-skaffold/skaffold-example", Tag: "gcr.io/k8s-skaffold/skaffold-example:TAG", @@ -266,7 +266,7 @@ metadata: name: getting-started spec: containers: - - image: gcr.io/k8s-skaffold/skaffold-example:IGNORED + - image: gcr.io/k8s-skaffold/skaffold-example:v1 name: getting-started`)}, expected: manifestList{[]byte(`apiVersion: v1 kind: Pod @@ -274,10 +274,32 @@ metadata: name: getting-started spec: containers: - - image: gcr.io/k8s-skaffold/skaffold-example:TAG + - image: gcr.io/k8s-skaffold/skaffold-example:v1 name: getting-started`)}, }, { + description: "replace latest", + builds: []build.Build{{ + ImageName: "gcr.io/k8s-skaffold/skaffold-example", + Tag: "gcr.io/k8s-skaffold/skaffold-example:TAG", + }}, + manifests: manifestList{[]byte(`apiVersion: v1 +kind: Pod +metadata: + name: getting-started +spec: + containers: + - image: gcr.io/k8s-skaffold/skaffold-example:latest + name: getting-started`)}, + expected: manifestList{[]byte(`apiVersion: v1 +kind: Pod +metadata: + name: getting-started +spec: + containers: + - image: gcr.io/k8s-skaffold/skaffold-example:TAG + name: getting-started`)}, + }, { description: "service and deployment", builds: []build.Build{{ ImageName: "gcr.io/k8s-skaffold/leeroy-app", @@ -412,19 +434,49 @@ spec: - containerPort: 80`, manifests.String()) } -func TestRemoveTag(t *testing.T) { - removed := removeTag("host:1234/user/container:tag") - if removed != "host:1234/user/container" { - t.Errorf("Tag vas not removed from an image name with port: %s ", removed) +func TestSplitTag(t *testing.T) { + var tests = []struct { + description string + image string + expectedName string + expectedTag string + }{ + { + description: "port and tag", + image: "host:1234/user/container:tag", + expectedName: "host:1234/user/container", + expectedTag: "tag", + }, + { + description: "port", + image: "host:1234/user/container", + expectedName: "host:1234/user/container", + expectedTag: "", + }, + { + description: "tag", + image: "host/user/container:tag", + expectedName: "host/user/container", + expectedTag: "tag", + }, + { + description: "latest", + image: "host/user/container:latest", + expectedName: "host/user/container", + expectedTag: "latest", + }, } - removed = removeTag("host/user/container:tag") - if removed != "host/user/container" { - t.Errorf("Tag vas not removed from an image name with port: %s ", removed) - } + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + name, tag := splitTag(test.image) - removed = removeTag("host:1234/user/container") - if removed != "host:1234/user/container" { - t.Errorf("Image without tag and port is changed during tag removal: %s ", removed) + if name != test.expectedName { + t.Errorf("Expected name: [%s]. Got: [%s]", test.expectedName, name) + } + if tag != test.expectedTag { + t.Errorf("Expected tag: [%s]. Got: [%s]", test.expectedTag, tag) + } + }) } } From 95610616b34a7b9561602977d63cfe65663eea82 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Fri, 18 May 2018 07:08:30 +0200 Subject: [PATCH 2/5] Simplify tests Signed-off-by: David Gageot --- pkg/skaffold/deploy/kubectl_test.go | 194 +++++++--------------------- 1 file changed, 49 insertions(+), 145 deletions(-) diff --git a/pkg/skaffold/deploy/kubectl_test.go b/pkg/skaffold/deploy/kubectl_test.go index 0038e354e66..bfcabd053d7 100644 --- a/pkg/skaffold/deploy/kubectl_test.go +++ b/pkg/skaffold/deploy/kubectl_test.go @@ -219,164 +219,68 @@ func TestKubectlCleanup(t *testing.T) { } func TestReplaceImages(t *testing.T) { - var tests = []struct { - description string - builds []build.Build - manifests manifestList - expected manifestList - shouldErr bool - }{ - { - description: "pod", - builds: []build.Build{{ - ImageName: "gcr.io/k8s-skaffold/skaffold-example", - Tag: "gcr.io/k8s-skaffold/skaffold-example:TAG", - }}, - manifests: manifestList{[]byte(`apiVersion: v1 -kind: Pod -metadata: - name: getting-started -spec: - containers: - - image: gcr.io/k8s-skaffold/skaffold-example - name: getting-started`)}, - expected: manifestList{[]byte(`apiVersion: v1 -kind: Pod -metadata: - name: getting-started -spec: - containers: - - image: gcr.io/k8s-skaffold/skaffold-example:TAG - name: getting-started`)}, - }, - { - description: "empty", - manifests: manifestList{[]byte(""), []byte(" ")}, - expected: manifestList{}, - }, - { - description: "skipped tagged", - builds: []build.Build{{ - ImageName: "gcr.io/k8s-skaffold/skaffold-example", - Tag: "gcr.io/k8s-skaffold/skaffold-example:TAG", - }}, - manifests: manifestList{[]byte(`apiVersion: v1 -kind: Pod -metadata: - name: getting-started -spec: - containers: - - image: gcr.io/k8s-skaffold/skaffold-example:v1 - name: getting-started`)}, - expected: manifestList{[]byte(`apiVersion: v1 -kind: Pod -metadata: - name: getting-started -spec: - containers: - - image: gcr.io/k8s-skaffold/skaffold-example:v1 - name: getting-started`)}, - }, - { - description: "replace latest", - builds: []build.Build{{ - ImageName: "gcr.io/k8s-skaffold/skaffold-example", - Tag: "gcr.io/k8s-skaffold/skaffold-example:TAG", - }}, - manifests: manifestList{[]byte(`apiVersion: v1 + manifests := manifestList{[]byte(` +apiVersion: v1 kind: Pod metadata: name: getting-started spec: containers: - - image: gcr.io/k8s-skaffold/skaffold-example:latest - name: getting-started`)}, - expected: manifestList{[]byte(`apiVersion: v1 + - image: gcr.io/k8s-skaffold/example + name: not-tagged + - image: gcr.io/k8s-skaffold/example:latest + name: latest + - image: gcr.io/k8s-skaffold/example:v1 + name: fully-qualified + - image: skaffold/other + name: other +`)} + + builds := []build.Build{{ + ImageName: "gcr.io/k8s-skaffold/example", + Tag: "gcr.io/k8s-skaffold/example:TAG", + }, { + ImageName: "skaffold/other", + Tag: "skaffold/other:OTHER_TAG", + }} + + expected := manifestList{[]byte(` +apiVersion: v1 kind: Pod metadata: name: getting-started spec: containers: - - image: gcr.io/k8s-skaffold/skaffold-example:TAG - name: getting-started`)}, - }, { - description: "service and deployment", - builds: []build.Build{{ - ImageName: "gcr.io/k8s-skaffold/leeroy-app", - Tag: "gcr.io/k8s-skaffold/leeroy-app:TAG", - }}, - manifests: manifestList{ - []byte(`apiVersion: apps/v1beta2 -kind: Deployment -metadata: - labels: - app: leeroy-app - name: leeroy-app -spec: - selector: - matchLabels: - app: leeroy-app - template: - metadata: - labels: - app: leeroy-app - spec: - containers: - - image: gcr.io/k8s-skaffold/leeroy-app - name: leeroy-app`), - []byte(`apiVersion: v1 -kind: Service -metadata: - labels: - app: leeroy-app - name: leeroy-app -spec: - ports: - - port: 50051 - selector: - app: leeroy-app`)}, - expected: manifestList{ - []byte(`apiVersion: apps/v1beta2 -kind: Deployment -metadata: - labels: - app: leeroy-app - name: leeroy-app -spec: - selector: - matchLabels: - app: leeroy-app - template: - metadata: - labels: - app: leeroy-app - spec: - containers: - - image: gcr.io/k8s-skaffold/leeroy-app:TAG - name: leeroy-app`), - []byte(`apiVersion: v1 -kind: Service -metadata: - labels: - app: leeroy-app - name: leeroy-app -spec: - ports: - - port: 50051 - selector: - app: leeroy-app`)}, - }, - } + - image: gcr.io/k8s-skaffold/example:TAG + name: not-tagged + - image: gcr.io/k8s-skaffold/example:TAG + name: latest + - image: gcr.io/k8s-skaffold/example:v1 + name: fully-qualified + - image: skaffold/other:OTHER_TAG + name: other +`)} + + resultManifest, err := manifests.replaceImages(builds) + + testutil.CheckErrorAndDeepEqual(t, false, err, expected.String(), resultManifest.String()) +} - for _, test := range tests { - t.Run(test.description, func(t *testing.T) { - manifests := manifestList(test.manifests) +func TestReplaceEmptyManifest(t *testing.T) { + manifests := manifestList{[]byte(""), []byte(" ")} + expected := manifestList{} - resultManifest, err := manifests.replaceImages(test.builds) + resultManifest, err := manifests.replaceImages(nil) - testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected.String(), resultManifest.String()) - }) - } + testutil.CheckErrorAndDeepEqual(t, false, err, expected.String(), resultManifest.String()) +} + +func TestReplaceInvalidManifest(t *testing.T) { + manifests := manifestList{[]byte("INVALID")} + + _, err := manifests.replaceImages(nil) + + testutil.CheckError(t, true, err) } func TestGenerateManifest(t *testing.T) { From 72dd9e6d471620721a287bb1a81b1ff296c209b9 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Fri, 18 May 2018 12:42:23 +0200 Subject: [PATCH 3/5] Adding a log for images that are not replaced Signed-off-by: David Gageot --- pkg/skaffold/deploy/kubectl.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/deploy/kubectl.go b/pkg/skaffold/deploy/kubectl.go index f3885086e5b..8340acce3c2 100644 --- a/pkg/skaffold/deploy/kubectl.go +++ b/pkg/skaffold/deploy/kubectl.go @@ -328,10 +328,13 @@ func recursiveReplaceImage(i interface{}, replacements map[string]*replacement) for k, v := range t { if k.(string) == "image" { name, tag := splitTag(v.(string)) - if tag == "" || tag == "latest" { - if img, present := replacements[name]; present { + if img, present := replacements[name]; present { + if tag == "" || tag == "latest" { t[k] = img.tag img.found = true + } else { + // TODO(1.0.0): Remove this warning. + logrus.Infof("Not replacing fully qualified image: %s (see #565)", v) } } } else { From a7a584c9c0e0cbb1c4f3658d9e0e49d1ca869d8b Mon Sep 17 00:00:00 2001 From: David Gageot Date: Fri, 18 May 2018 18:35:41 +0200 Subject: [PATCH 4/5] Test support for digests Signed-off-by: David Gageot --- pkg/skaffold/deploy/kubectl_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/skaffold/deploy/kubectl_test.go b/pkg/skaffold/deploy/kubectl_test.go index bfcabd053d7..0a534b6847d 100644 --- a/pkg/skaffold/deploy/kubectl_test.go +++ b/pkg/skaffold/deploy/kubectl_test.go @@ -234,6 +234,8 @@ spec: name: fully-qualified - image: skaffold/other name: other + - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 + name: digest `)} builds := []build.Build{{ @@ -259,6 +261,8 @@ spec: name: fully-qualified - image: skaffold/other:OTHER_TAG name: other + - image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883 + name: digest `)} resultManifest, err := manifests.replaceImages(builds) From 27c326f337b60f11f16fdf9efde85a54bd9db780 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Fri, 18 May 2018 19:07:59 +0200 Subject: [PATCH 5/5] Improve image reference parsing --- pkg/skaffold/deploy/kubectl.go | 66 ++++++++++++++++++++--------- pkg/skaffold/deploy/kubectl_test.go | 56 ++++++++++++------------ 2 files changed, 76 insertions(+), 46 deletions(-) diff --git a/pkg/skaffold/deploy/kubectl.go b/pkg/skaffold/deploy/kubectl.go index 8340acce3c2..e88282241fd 100644 --- a/pkg/skaffold/deploy/kubectl.go +++ b/pkg/skaffold/deploy/kubectl.go @@ -23,7 +23,6 @@ import ( "os" "os/exec" "path/filepath" - "regexp" "strings" "text/template" @@ -31,6 +30,7 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" + "github.com/docker/distribution/reference" "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/spf13/afero" @@ -326,30 +326,58 @@ func recursiveReplaceImage(i interface{}, replacements map[string]*replacement) } case map[interface{}]interface{}: for k, v := range t { - if k.(string) == "image" { - name, tag := splitTag(v.(string)) - if img, present := replacements[name]; present { - if tag == "" || tag == "latest" { - t[k] = img.tag - img.found = true - } else { - // TODO(1.0.0): Remove this warning. - logrus.Infof("Not replacing fully qualified image: %s (see #565)", v) - } - } - } else { + if k.(string) != "image" { recursiveReplaceImage(v, replacements) + continue + } + + image := v.(string) + parsed, err := parseReference(image) + if err != nil { + logrus.Warnf("Couldn't parse image: %s", v) + continue + } + + if parsed.fullyQualified { + // TODO(1.0.0): Remove this warning. + logrus.Infof("Not replacing fully qualified image: %s (see #565)", v) + continue + } + + if img, present := replacements[parsed.baseName]; present { + t[k] = img.tag + img.found = true } } } } -func splitTag(image string) (string, string) { - re := regexp.MustCompile(`(.*):([^/]+)$`) - matches := re.FindStringSubmatch(image) +type imageReference struct { + baseName string + fullyQualified bool +} + +func parseReference(image string) (*imageReference, error) { + r, err := reference.Parse(image) + if err != nil { + return nil, err + } + + baseName := image + if n, ok := r.(reference.Named); ok { + baseName = n.Name() + } - if len(matches) == 3 { - return matches[1], matches[2] + fullyQualified := false + switch n := r.(type) { + case reference.Tagged: + fullyQualified = n.Tag() != "latest" + case reference.Digested: + fullyQualified = true } - return image, "" + + return &imageReference{ + baseName: baseName, + fullyQualified: fullyQualified, + }, nil } diff --git a/pkg/skaffold/deploy/kubectl_test.go b/pkg/skaffold/deploy/kubectl_test.go index 0a534b6847d..4daac25efb4 100644 --- a/pkg/skaffold/deploy/kubectl_test.go +++ b/pkg/skaffold/deploy/kubectl_test.go @@ -344,47 +344,49 @@ spec: func TestSplitTag(t *testing.T) { var tests = []struct { - description string - image string - expectedName string - expectedTag string + description string + image string + expectedName string + expectedFullyQualified bool }{ { - description: "port and tag", - image: "host:1234/user/container:tag", - expectedName: "host:1234/user/container", - expectedTag: "tag", + description: "port and tag", + image: "host:1234/user/container:tag", + expectedName: "host:1234/user/container", + expectedFullyQualified: true, }, { - description: "port", - image: "host:1234/user/container", - expectedName: "host:1234/user/container", - expectedTag: "", + description: "port", + image: "host:1234/user/container", + expectedName: "host:1234/user/container", + expectedFullyQualified: false, }, { - description: "tag", - image: "host/user/container:tag", - expectedName: "host/user/container", - expectedTag: "tag", + description: "tag", + image: "host/user/container:tag", + expectedName: "host/user/container", + expectedFullyQualified: true, }, { - description: "latest", - image: "host/user/container:latest", - expectedName: "host/user/container", - expectedTag: "latest", + description: "latest", + image: "host/user/container:latest", + expectedName: "host/user/container", + expectedFullyQualified: false, + }, + { + description: "digest", + image: "gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883", + expectedName: "gcr.io/k8s-skaffold/example", + expectedFullyQualified: true, }, } for _, test := range tests { t.Run(test.description, func(t *testing.T) { - name, tag := splitTag(test.image) + parsed, err := parseReference(test.image) - if name != test.expectedName { - t.Errorf("Expected name: [%s]. Got: [%s]", test.expectedName, name) - } - if tag != test.expectedTag { - t.Errorf("Expected tag: [%s]. Got: [%s]", test.expectedTag, tag) - } + testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedName, parsed.baseName) + testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedFullyQualified, parsed.fullyQualified) }) } }