Merge pull request #3392 from lilic/fix-image-tag-version

pkg/operator/image.go: Adjust image path building

go.mod

@@ -5,6 +5,7 @@ go 1.14
 require (
 	github.com/blang/semver v3.5.1+incompatible
 	github.com/brancz/kube-rbac-proxy v0.5.0
+	github.com/docker/distribution v2.7.1+incompatible
 	github.com/evanphx/json-patch v4.5.0+incompatible
 	github.com/ghodss/yaml v1.0.0
 	github.com/go-kit/kit v0.10.0
@@ -16,6 +17,7 @@ require (
 	github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348
 	github.com/mitchellh/hashstructure v0.0.0-20170609045927-2bca23e0e452
 	github.com/oklog/run v1.1.0
+	github.com/opencontainers/go-digest v1.0.0 // indirect
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus-community/prom-label-proxy v0.1.1-0.20200616110844-0fbfa11fa8f3
 	github.com/prometheus/client_golang v1.6.0

go.sum

@@ -170,6 +170,8 @@ github.com/dgrijalva/jwt-go v0.0.0-20160705203006-01aeca54ebda/go.mod h1:E3ru+11
 github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ=
 github.com/dgryski/go-bitstream v0.0.0-20180413035011-3522498ce2c8/go.mod h1:VMaSuZ+SZcx/wljOQKvp5srsbCiKDEb6K2wC4+PiBmQ=
 github.com/dgryski/go-sip13 v0.0.0-20190329191031-25c5027a8c7b/go.mod h1:vAd38F8PWV+bWy6jNmig1y/TA+kYO4g3RSRF0IAv0no=
+github.com/docker/distribution v2.7.1+incompatible h1:a5mlkVzth6W5A4fOsS3D2EO5BUmsJpcB+cRlLU7cSug=
+github.com/docker/distribution v2.7.1+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w=
 github.com/docker/docker v0.7.3-0.20190327010347-be7ac8be2ae0/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
 github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk=
 github.com/docker/go-units v0.4.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk=
@@ -656,6 +658,8 @@ github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1Cpa
 github.com/onsi/gomega v1.7.1 h1:K0jcRCwNQM3vFGh1ppMtDh/+7ApJrjldlX8fA0jDTLQ=
 github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7JYyY=
 github.com/op/go-logging v0.0.0-20160315200505-970db520ece7/go.mod h1:HzydrMdWErDVzsI23lYNej1Htcns9BCg93Dk0bBINWk=
+github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
+github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
 github.com/opentracing-contrib/go-observer v0.0.0-20170622124052-a52f23424492/go.mod h1:Ngi6UdF0k5OKD5t5wlmGhe/EDKPoUM3BXZSSfIuJbis=
 github.com/opentracing-contrib/go-stdlib v0.0.0-20190519235532-cf7a6c988dc9/go.mod h1:PLldrQSroqzH70Xl+1DQcGnefIbqsKR7UDaiux3zV+w=
 github.com/opentracing/basictracer-go v1.0.0/go.mod h1:QfBfYuafItcjQuMwinw9GhYKwFXS9KnPs5lxoYwgW74=

pkg/alertmanager/statefulset.go

@@ -216,7 +216,10 @@ func makeStatefulSetSpec(a *monitoringv1.Alertmanager, config Config) (*appsv1.S
 	amVersion := operator.StringValOrDefault(a.Spec.Version, operator.DefaultAlertmanagerVersion)
 	amTag := operator.StringValOrDefault(a.Spec.Tag, "")
 	amSHA := operator.StringValOrDefault(a.Spec.SHA, "")
-	amImagePath := operator.BuildImagePath(amBaseImage, amVersion, amTag, amSHA)
+	amImagePath, err := operator.BuildImagePath(amBaseImage, amVersion, amTag, amSHA)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build image path")
+	}
 	if a.Spec.Image != nil && strings.TrimSpace(*a.Spec.Image) != "" {
 		amImagePath = *a.Spec.Image
 	}

pkg/operator/image.go

@@ -17,26 +17,38 @@ package operator
 import (
 	"fmt"
 	"strings"
+
+	dockerref "github.com/docker/distribution/reference"
 )
 
 // BuildImagePath builds a container image path based on
 // the given parameters.
-// baseImage and version are used by default.
-// If the tag is specified, we use the tag to identify the container image.
-// If the sha is specified, we use the sha to identify the container image,
-// as it has even stronger immutable guarantees to identify the image.
-func BuildImagePath(baseImage, version, tag, sha string) string {
-	image := baseImage
-	if version != "" {
-		image = fmt.Sprintf("%s:%s", baseImage, version)
+// If the image contains tag or digest then image will be returned.
+// Inspired by kubernetes code handling of image building.
+func BuildImagePath(image, version, tag, sha string) (string, error) {
+	named, err := dockerref.ParseNormalizedNamed(image)
+	if err != nil {
+		return "", fmt.Errorf("couldn't parse image reference %q: %v", image, err)
 	}
-	if tag != "" {
-		image = fmt.Sprintf("%s:%s", baseImage, tag)
+	_, isTagged := named.(dockerref.Tagged)
+	_, isDigested := named.(dockerref.Digested)
+	if isTagged || isDigested {
+		return image, nil
 	}
+
 	if sha != "" {
-		image = fmt.Sprintf("%s@sha256:%s", baseImage, sha)
+		return fmt.Sprintf("%s@sha256:%s", image, sha), nil
+	} else if tag != "" {
+		imageTag, err := dockerref.WithTag(named, tag)
+		if err != nil {
+			return "", err
+		}
+		return imageTag.String(), nil
+	} else if version != "" {
+		return image + ":" + version, nil
 	}
-	return image
+
+	return image, nil
 }
 
 // StringValOrDefault returns the default val if the

pkg/operator/image_test.go

@@ -19,18 +19,18 @@ import (
 )
 
 type ImageSpec struct {
-	BaseImage string
-	Version   string
-	Tag       string
-	SHA       string
+	Image   string
+	Version string
+	Tag     string
+	SHA     string
 }
 
 func TestBuildImagePath(t *testing.T) {
 	defaultImageSpec := &ImageSpec{
-		BaseImage: "foo/bar",
-		Version:   "0.0.1",
+		Image:   "foo.com/bar",
+		Version: "0.0.1",
 	}
-	// imageWithoutVersion := "myrepo/myimage"
+	// imageWithoutVersion := "myrepo/myimage:123"
 	// imageWithVersion := "myhost:9090/myrepo/myimage:0.2"
 	// imageWithTag := "myhost:9090/myrepo/myimage:latest"
 	// imageWithSHA := "foo/bar@sha256:12345"
@@ -44,7 +44,7 @@ func TestBuildImagePath(t *testing.T) {
 		},
 		{
 			spec:     defaultImageSpec,
-			expected: defaultImageSpec.BaseImage + ":" + defaultImageSpec.Version,
+			expected: defaultImageSpec.Image + ":" + defaultImageSpec.Version,
 		},
 		{
 			spec:     &ImageSpec{"myrepo.com/foo", "1.0", "", ""},
@@ -61,7 +61,7 @@ func TestBuildImagePath(t *testing.T) {
 	}
 
 	for i, c := range cases {
-		result := BuildImagePath(c.spec.BaseImage, c.spec.Version, c.spec.Tag, c.spec.SHA)
+		result, _ := BuildImagePath(c.spec.Image, c.spec.Version, c.spec.Tag, c.spec.SHA)
 		if c.expected != result {
 			t.Errorf("expected test case %d to be %q but got %q", i, c.expected, result)
 		}

pkg/prometheus/statefulset.go

@@ -285,8 +285,10 @@ func makeStatefulSetSpec(p monitoringv1.Prometheus, c *Config, ruleConfigMapName
 	if p.Spec.Image != nil && strings.TrimSpace(*p.Spec.Image) != "" {
 		baseImage = *p.Spec.Image
 	}
-	prometheusImagePath := operator.BuildImagePath(baseImage, p.Spec.Version, p.Spec.Tag, p.Spec.SHA)
-
+	prometheusImagePath, err := operator.BuildImagePath(baseImage, p.Spec.Version, p.Spec.Tag, p.Spec.SHA)
+	if err != nil {
+		return nil, err
+	}
 	promArgs := []string{
 		"-web.console.templates=/etc/prometheus/consoles",
 		"-web.console.libraries=/etc/prometheus/console_libraries",
@@ -711,7 +713,10 @@ func makeStatefulSetSpec(p monitoringv1.Prometheus, c *Config, ruleConfigMapName
 		thVersion := operator.StringPtrValOrDefault(p.Spec.Thanos.Version, operator.DefaultThanosVersion)
 		thTag := operator.StringPtrValOrDefault(p.Spec.Thanos.Tag, "")
 		thSHA := operator.StringPtrValOrDefault(p.Spec.Thanos.SHA, "")
-		thanosImage := operator.BuildImagePath(thBaseImage, thVersion, thTag, thSHA)
+		thanosImage, err := operator.BuildImagePath(thBaseImage, thVersion, thTag, thSHA)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to build image path")
+		}
 		// If the image path is set in the custom resource, override other image settings.
 		if p.Spec.Thanos.Image != nil && strings.TrimSpace(*p.Spec.Thanos.Image) != "" {
 			thanosImage = *p.Spec.Thanos.Image

pkg/prometheus/statefulset_test.go

@@ -533,7 +533,7 @@ func TestTagAndShaAndVersion(t *testing.T) {
 		}
 
 		resultImage := sset.Spec.Template.Spec.Containers[0].Image
-		expected := "my-reg/prometheus:latest@sha256:7384a79f4b4991bf8269e7452390249b7c70bcdd10509c8c1c6c6e30e32fb324"
+		expected := "my-reg/prometheus:latest"
 		if resultImage != expected {
 			t.Fatalf("Explicit image should have precedence. Unexpected container image.\n\nExpected: %s\n\nGot: %s", expected, resultImage)
 		}
@@ -623,7 +623,7 @@ func TestTagAndShaAndVersion(t *testing.T) {
 		}
 
 		resultImage := sset.Spec.Template.Spec.Containers[0].Image
-		expected := "my-reg/prometheus:my-unrelated-tag"
+		expected := "docker.io/my-reg/prometheus:my-unrelated-tag"
 		if resultImage != expected {
 			t.Fatalf("Unexpected container image.\n\nExpected: %s\n\nGot: %s", expected, resultImage)
 		}

pkg/thanos/statefulset.go

@@ -153,7 +153,10 @@ func makeStatefulSetSpec(tr *monitoringv1.ThanosRuler, config Config, ruleConfig
 	}
 
 	trBaseImage := operator.StringValOrDefault(config.ThanosDefaultBaseImage, operator.DefaultThanosBaseImage)
-	trImagePath := operator.BuildImagePath(trBaseImage, operator.DefaultThanosVersion, "", "")
+	trImagePath, err := operator.BuildImagePath(trBaseImage, operator.DefaultThanosVersion, "", "")
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to build image path")
+	}
 	if strings.TrimSpace(tr.Spec.Image) != "" {
 		trImagePath = tr.Spec.Image
 	}