pkg/operator: consolidate image url logic

Move logic for building image URLs into the operator package.
This improves the consistency for building image URLs from the
combination of default settings, operator CLI args, and config in the
custom resources.

pkg/alertmanager/statefulset.go

@@ -54,15 +54,9 @@ func makeStatefulSet(am *monitoringv1.Alertmanager, old *appsv1.StatefulSet, con
 	// Ideally we would do it before storing but that's currently not possible.
 	// Potentially an update handler on first insertion.
 
-	if am.Spec.BaseImage == "" {
-		am.Spec.BaseImage = config.AlertmanagerDefaultBaseImage
-	}
 	if am.Spec.PortName == "" {
 		am.Spec.PortName = defaultPortName
 	}
-	if am.Spec.Version == "" {
-		am.Spec.Version = operator.DefaultAlertmanagerVersion
-	}
 	if am.Spec.Replicas == nil {
 		am.Spec.Replicas = &minReplicas
 	}
@@ -218,22 +212,16 @@ func makeStatefulSetSpec(a *monitoringv1.Alertmanager, config Config) (*appsv1.S
 	// details see https://github.com/coreos/prometheus-operator/issues/1659
 	a = a.DeepCopy()
 
-	// Version is 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.
-	image := fmt.Sprintf("%s:%s", a.Spec.BaseImage, a.Spec.Version)
-	if a.Spec.Tag != "" {
-		image = fmt.Sprintf("%s:%s", a.Spec.BaseImage, a.Spec.Tag)
-	}
-	if a.Spec.SHA != "" {
-		image = fmt.Sprintf("%s@sha256:%s", a.Spec.BaseImage, a.Spec.SHA)
-	}
-	if a.Spec.Image != nil && *a.Spec.Image != "" {
-		image = *a.Spec.Image
+	amBaseImage := operator.StringValOrDefault(a.Spec.BaseImage, operator.DefaultAlertmanagerBaseImage)
+	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)
+	if a.Spec.Image != nil && strings.TrimSpace(*a.Spec.Image) != "" {
+		amImagePath = *a.Spec.Image
 	}
 
-	version, err := semver.ParseTolerant(a.Spec.Version)
+	version, err := semver.ParseTolerant(amVersion)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to parse alertmanager version")
 	}
@@ -492,7 +480,7 @@ func makeStatefulSetSpec(a *monitoringv1.Alertmanager, config Config) (*appsv1.S
 		{
 			Args:           amArgs,
 			Name:           "alertmanager",
-			Image:          image,
+			Image:          amImagePath,
 			Ports:          ports,
 			VolumeMounts:   amVolumeMounts,
 			LivenessProbe:  livenessProbe,

pkg/operator/defaults.go

@@ -77,5 +77,5 @@ var (
 	}
 	DefaultPrometheusVersion   = PrometheusCompatibilityMatrix[len(PrometheusCompatibilityMatrix)-1]
 	DefaultPrometheusBaseImage = "quay.io/prometheus/prometheus"
-	DefaultPrometheusImage     = DefaultAlertmanagerBaseImage + ":" + DefaultPrometheusVersion
+	DefaultPrometheusImage     = DefaultPrometheusBaseImage + ":" + DefaultPrometheusVersion
 )

pkg/operator/image.go

@@ -0,0 +1,60 @@
+// Copyright 2020 The prometheus-operator Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package operator
+
+import (
+	"fmt"
+	"strings"
+)
+
+// 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 tag != "" {
+		image = fmt.Sprintf("%s:%s", baseImage, tag)
+	}
+	if sha != "" {
+		image = fmt.Sprintf("%s@sha256:%s", baseImage, sha)
+	}
+	return image
+}
+
+// StringValOrDefault returns the default val if the
+// given string is empty/whitespace.
+// Otherwise returns the value of the string..
+func StringValOrDefault(val, defaultVal string) string {
+	if strings.TrimSpace(val) == "" {
+		return defaultVal
+	}
+	return val
+}
+
+// StringPtrValOrDefault returns the default val if the
+// given string pointer is nil points to an empty/whitespace string.
+// Otherwise returns the value of the string.
+func StringPtrValOrDefault(val *string, defaultVal string) string {
+	if val == nil {
+		return defaultVal
+	}
+	return StringValOrDefault(*val, defaultVal)
+}

pkg/operator/image_test.go

@@ -0,0 +1,69 @@
+// Copyright 2020 The prometheus-operator Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package operator
+
+import (
+	"testing"
+)
+
+type ImageSpec struct {
+	BaseImage string
+	Version   string
+	Tag       string
+	SHA       string
+}
+
+func TestBuildImagePath(t *testing.T) {
+	defaultImageSpec := &ImageSpec{
+		BaseImage: "foo/bar",
+		Version:   "0.0.1",
+	}
+	// imageWithoutVersion := "myrepo/myimage"
+	// imageWithVersion := "myhost:9090/myrepo/myimage:0.2"
+	// imageWithTag := "myhost:9090/myrepo/myimage:latest"
+	// imageWithSHA := "foo/bar@sha256:12345"
+	cases := []struct {
+		spec     *ImageSpec
+		expected string
+	}{
+		{
+			spec:     &ImageSpec{},
+			expected: "",
+		},
+		{
+			spec:     defaultImageSpec,
+			expected: defaultImageSpec.BaseImage + ":" + defaultImageSpec.Version,
+		},
+		{
+			spec:     &ImageSpec{"myrepo.com/foo", "1.0", "", ""},
+			expected: "myrepo.com/foo:1.0",
+		},
+		{
+			spec:     &ImageSpec{"myrepo.com/foo", "1.0", "latest", ""},
+			expected: "myrepo.com/foo:latest",
+		},
+		{
+			spec:     &ImageSpec{"myrepo.com/foo", "1.0", "latest", "abcd1234"},
+			expected: "myrepo.com/foo@sha256:abcd1234",
+		},
+	}
+
+	for i, c := range cases {
+		result := BuildImagePath(c.spec.BaseImage, 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

@@ -73,30 +73,16 @@ func makeStatefulSet(
 	// details see https://github.com/coreos/prometheus-operator/issues/1659.
 	p = *p.DeepCopy()
 
-	// TODO(fabxc): is this the right point to inject defaults?
-	// Ideally we would do it before storing but that's currently not possible.
-	// Potentially an update handler on first insertion.
-
-	if p.Spec.BaseImage == "" {
-		p.Spec.BaseImage = config.PrometheusDefaultBaseImage
-	}
-	if p.Spec.Version == "" {
-		p.Spec.Version = operator.DefaultPrometheusVersion
-	}
-	if p.Spec.Thanos != nil && p.Spec.Thanos.Version == nil {
-		v := operator.DefaultThanosVersion
-		p.Spec.Thanos.Version = &v
+	promVersion := operator.StringValOrDefault(p.Spec.Version, operator.DefaultPrometheusVersion)
+	parsedVersion, err := semver.ParseTolerant(promVersion)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to parse prometheus version")
 	}
 
 	if p.Spec.PortName == "" {
 		p.Spec.PortName = defaultPortName
 	}
 
-	version, err := semver.ParseTolerant(p.Spec.Version)
-	if err != nil {
-		return nil, errors.Wrap(err, "parse version")
-	}
-
 	if p.Spec.Replicas == nil {
 		p.Spec.Replicas = &minReplicas
 	}
@@ -113,7 +99,7 @@ func makeStatefulSet(
 	}
 	_, memoryRequestFound := p.Spec.Resources.Requests[v1.ResourceMemory]
 	memoryLimit, memoryLimitFound := p.Spec.Resources.Limits[v1.ResourceMemory]
-	if !memoryRequestFound && version.Major == 1 {
+	if !memoryRequestFound && parsedVersion.Major == 1 {
 		defaultMemoryRequest := resource.MustParse("2Gi")
 		compareResult := memoryLimit.Cmp(defaultMemoryRequest)
 		// If limit is given and smaller or equal to 2Gi, then set memory
@@ -126,7 +112,7 @@ func makeStatefulSet(
 		}
 	}
 
-	spec, err := makeStatefulSetSpec(p, config, ruleConfigMapNames)
+	spec, err := makeStatefulSetSpec(p, config, ruleConfigMapNames, parsedVersion)
 	if err != nil {
 		return nil, errors.Wrap(err, "make StatefulSet spec")
 	}
@@ -289,14 +275,17 @@ func makeStatefulSetService(p *monitoringv1.Prometheus, config Config) *v1.Servi
 	return svc
 }
 
-func makeStatefulSetSpec(p monitoringv1.Prometheus, c *Config, ruleConfigMapNames []string) (*appsv1.StatefulSetSpec, error) {
+func makeStatefulSetSpec(p monitoringv1.Prometheus, c *Config, ruleConfigMapNames []string,
+	version semver.Version) (*appsv1.StatefulSetSpec, error) {
 	// Prometheus may take quite long to shut down to checkpoint existing data.
 	// Allow up to 10 minutes for clean termination.
 	terminationGracePeriod := int64(600)
 
-	version, err := semver.ParseTolerant(p.Spec.Version)
-	if err != nil {
-		return nil, errors.Wrap(err, "parse version")
+	baseImage := operator.StringValOrDefault(p.Spec.BaseImage, operator.DefaultPrometheusBaseImage)
+	prometheusImagePath := operator.BuildImagePath(baseImage, p.Spec.Version, p.Spec.Tag, p.Spec.SHA)
+	// An image path specified in the custom resource overrides all other image path settings.
+	if p.Spec.Image != nil && strings.TrimSpace(*p.Spec.Image) != "" {
+		prometheusImagePath = *p.Spec.Image
 	}
 
 	promArgs := []string{
@@ -719,24 +708,16 @@ func makeStatefulSetSpec(p monitoringv1.Prometheus, c *Config, ruleConfigMapName
 
 	disableCompaction := p.Spec.DisableCompaction
 	if p.Spec.Thanos != nil {
-		// Version is 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.
-		thanosBaseImage := c.ThanosDefaultBaseImage
-		if p.Spec.Thanos.BaseImage != nil {
-			thanosBaseImage = *p.Spec.Thanos.BaseImage
-		}
-		thanosImage := fmt.Sprintf("%s:%s", thanosBaseImage, *p.Spec.Thanos.Version)
-		if p.Spec.Thanos.Tag != nil {
-			thanosImage = fmt.Sprintf("%s:%s", thanosBaseImage, *p.Spec.Thanos.Tag)
-		}
-		if p.Spec.Thanos.SHA != nil {
-			thanosImage = fmt.Sprintf("%s@sha256:%s", thanosBaseImage, *p.Spec.Thanos.SHA)
-		}
-		if p.Spec.Thanos.Image != nil && *p.Spec.Thanos.Image != "" {
+		thBaseImage := operator.StringPtrValOrDefault(p.Spec.Thanos.BaseImage, operator.DefaultThanosBaseImage)
+		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)
+		// 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
 		}
+
 		bindAddress := "[$(POD_IP)]"
 		if p.Spec.Thanos.ListenLocal {
 			bindAddress = "127.0.0.1"
@@ -840,21 +821,6 @@ func makeStatefulSetSpec(p monitoringv1.Prometheus, c *Config, ruleConfigMapName
 		promArgs = append(promArgs, "--storage.tsdb.max-block-duration=2h")
 	}
 
-	// Version is 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.
-	prometheusImage := fmt.Sprintf("%s:%s", p.Spec.BaseImage, p.Spec.Version)
-	if p.Spec.Tag != "" {
-		prometheusImage = fmt.Sprintf("%s:%s", p.Spec.BaseImage, p.Spec.Tag)
-	}
-	if p.Spec.SHA != "" {
-		prometheusImage = fmt.Sprintf("%s@sha256:%s", p.Spec.BaseImage, p.Spec.SHA)
-	}
-	if p.Spec.Image != nil && *p.Spec.Image != "" {
-		prometheusImage = *p.Spec.Image
-	}
-
 	prometheusConfigReloaderResources := v1.ResourceRequirements{
 		Limits: v1.ResourceList{}, Requests: v1.ResourceList{}}
 	if c.ConfigReloaderCPU != "0" {
@@ -869,7 +835,7 @@ func makeStatefulSetSpec(p monitoringv1.Prometheus, c *Config, ruleConfigMapName
 	operatorContainers := append([]v1.Container{
 		{
 			Name:                     "prometheus",
-			Image:                    prometheusImage,
+			Image:                    prometheusImagePath,
 			Ports:                    ports,
 			Args:                     promArgs,
 			VolumeMounts:             promVolumeMounts,

pkg/thanos/statefulset.go

@@ -53,12 +53,6 @@ var (
 
 func makeStatefulSet(tr *monitoringv1.ThanosRuler, config Config, ruleConfigMapNames []string, inputHash string) (*appsv1.StatefulSet, error) {
 
-	if tr.Spec.Image == "" {
-		tr.Spec.Image = config.ThanosDefaultBaseImage
-	}
-	if !strings.Contains(tr.Spec.Image, ":") {
-		tr.Spec.Image = tr.Spec.Image + ":" + operator.DefaultThanosVersion
-	}
 	if tr.Spec.Resources.Requests == nil {
 		tr.Spec.Resources.Requests = v1.ResourceList{}
 	}
@@ -158,6 +152,12 @@ func makeStatefulSetSpec(tr *monitoringv1.ThanosRuler, config Config, ruleConfig
 		return nil, errors.New(tr.GetName() + ": thanos ruler requires query config or at least one query endpoint to be specified")
 	}
 
+	trBaseImage := operator.StringValOrDefault(config.ThanosDefaultBaseImage, operator.DefaultThanosBaseImage)
+	trImagePath := operator.BuildImagePath(trBaseImage, operator.DefaultThanosVersion, "", "")
+	if strings.TrimSpace(tr.Spec.Image) != "" {
+		trImagePath = tr.Spec.Image
+	}
+
 	if tr.Spec.EvaluationInterval == "" {
 		tr.Spec.EvaluationInterval = defaultEvaluationInterval
 	}
@@ -386,7 +386,7 @@ func makeStatefulSetSpec(tr *monitoringv1.ThanosRuler, config Config, ruleConfig
 	operatorContainers := append([]v1.Container{
 		{
 			Name:                     "thanos-ruler",
-			Image:                    tr.Spec.Image,
+			Image:                    trImagePath,
 			Args:                     trCLIArgs,
 			Env:                      trEnvVars,
 			VolumeMounts:             trVolumeMounts,