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

Skip fully qualified names when replacing images names #566

Merged
merged 5 commits into from
May 21, 2018
Merged

Skip fully qualified names when replacing images names #566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e53fd8
Skip fully qualified names when replacing images names.
dgageot May 17, 2018
9561061
Simplify tests
dgageot May 18, 2018
72dd9e6
Adding a log for images that are not replaced
dgageot May 18, 2018
a7a584c
Test support for digests
dgageot May 18, 2018
27c326f
Improve image reference parsing
dgageot May 18, 2018
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
60 changes: 49 additions & 11 deletions pkg/skaffold/deploy/kubectl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ import (
"os"
"os/exec"
"path/filepath"
"regexp"
"strings"
"text/template"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
"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"
Expand Down Expand Up @@ -326,20 +326,58 @@ 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
}
} 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 removeTag(image string) string {
re := regexp.MustCompile(":[^/]+$")
return re.ReplaceAllString(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()
}

fullyQualified := false
switch n := r.(type) {
case reference.Tagged:
fullyQualified = n.Tag() != "latest"
case reference.Digested:
fullyQualified = true
}

return &imageReference{
baseName: baseName,
fullyQualified: fullyQualified,
}, nil
}
230 changes: 96 additions & 134 deletions pkg/skaffold/deploy/kubectl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,142 +219,72 @@ 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
manifests := 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: "ignore tag",
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:IGNORED
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
- image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883
name: digest
`)}

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
- image: gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883
name: digest
`)}

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) {
Expand Down Expand Up @@ -412,19 +342,51 @@ 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 {
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 add a test case for digest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be done now!

description string
image string
expectedName string
expectedFullyQualified bool
}{
{
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",
expectedFullyQualified: false,
},
{
description: "tag",
image: "host/user/container:tag",
expectedName: "host/user/container",
expectedFullyQualified: true,
},
{
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,
},
}

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) {
parsed, err := parseReference(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)
testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedName, parsed.baseName)
testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedFullyQualified, parsed.fullyQualified)
})
}
}