Fix image digests (#9869)

* add back values

* refactor helper

* update comments

* tests

* more tests

* cl

* make reusable

* fix lint

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

changelog/v1.18.0-beta14/fix-image-digests.yaml

@@ -0,0 +1,6 @@
+changelog:
+  - type: HELM
+    issueLink: https://github.com/solo-io/gloo/issues/9860
+    resolvesIssue: false
+    description: >-
+      Ensure that image digests are set correctly for all image variants (standard, fips, distroless, fips-distroless).

install/helm/gloo/generate/values.go

@@ -63,15 +63,17 @@ type Rbac struct {
 
 // Common
 type Image struct {
-	Tag        *string `json:"tag,omitempty"  desc:"The image tag for the container."`
-	Repository *string `json:"repository,omitempty"  desc:"The image repository (name) for the container."`
-	Digest     *string `json:"digest,omitempty"  desc:"The hash digest of the container's image, ie. sha256:12345...."`
-	Registry   *string `json:"registry,omitempty" desc:"The image hostname prefix and registry, such as quay.io/solo-io."`
-	PullPolicy *string `json:"pullPolicy,omitempty"  desc:"The image pull policy for the container. For default values, see the Kubernetes docs: https://kubernetes.io/docs/concepts/containers/images/#imagepullpolicy-defaulting"`
-	PullSecret *string `json:"pullSecret,omitempty" desc:"The image pull secret to use for the container, in the same namespace as the container pod."`
-	Variant    *string `json:"variant,omitempty" desc:"Specifies the version of the data-plane containers to deploy. Can take the values 'standard', 'fips', 'distroless', 'fips-distroless'. Defaults to standard. (The 'fips' and 'fips-distroless' variants are an Enterprise-only feature)"`
-	FipsDigest *string `json:"fipsDigest,omitempty"  desc:"[Deprecated] Use 'variant=fips' and 'digest=...' instead. The hash digest of the container's fips image, ie. sha256:12345....  Only consumed if fips=true"`
-	Fips       *bool   `json:"fips,omitempty" desc:"[Deprecated] Use 'variant=fips' instead. If true, deploys a version of the data-plane containers that is built with FIPS-compliant crypto libraries. (Enterprise-only feature)"`
+	Tag                  *string `json:"tag,omitempty"  desc:"The image tag for the container."`
+	Repository           *string `json:"repository,omitempty"  desc:"The image repository (name) for the container."`
+	Digest               *string `json:"digest,omitempty"  desc:"The container image's hash digest (e.g. 'sha256:12345...'), consumed when variant=standard."`
+	FipsDigest           *string `json:"fipsDigest,omitempty"  desc:"The container image's hash digest (e.g. 'sha256:12345...'), consumed when variant=fips. If the image does not have a fips variant, this field will contain the digest for the standard image variant."`
+	DistrolessDigest     *string `json:"distrolessDigest,omitempty"  desc:"The container image's hash digest (e.g. 'sha256:12345...'), consumed when variant=distroless. If the image does not have a distroless variant, this field will contain the digest for the standard image variant."`
+	FipsDistrolessDigest *string `json:"fipsDistrolessDigest,omitempty"  desc:"The container image's hash digest (e.g. 'sha256:12345...'), consumed when variant=fips-distroless. If the image does not have a fips-distroless variant, this field will contain either the fips variant's digest (if supported), else the distroless variant's digest (if supported), else the standard variant's digest."`
+	Registry             *string `json:"registry,omitempty" desc:"The image hostname prefix and registry, such as quay.io/solo-io."`
+	PullPolicy           *string `json:"pullPolicy,omitempty"  desc:"The image pull policy for the container. For default values, see the Kubernetes docs: https://kubernetes.io/docs/concepts/containers/images/#imagepullpolicy-defaulting"`
+	PullSecret           *string `json:"pullSecret,omitempty" desc:"The image pull secret to use for the container, in the same namespace as the container pod."`
+	Variant              *string `json:"variant,omitempty" desc:"Specifies the variant of the control plane and data plane containers to deploy. Can take the values 'standard', 'fips', 'distroless', 'fips-distroless'. Defaults to standard. (The 'fips' and 'fips-distroless' variants are an Enterprise-only feature)"`
+	Fips                 *bool   `json:"fips,omitempty" desc:"[Deprecated] Use 'variant=fips' instead. If true, deploys a version of the control plane and data plane containers that is built with FIPS-compliant crypto libraries. (Enterprise-only feature)"`
 }
 
 type ResourceAllocation struct {

install/helm/gloo/templates/_helpers.tpl

@@ -26,37 +26,62 @@ ClusterRole
 {{- end -}}
 
 {{/*
-Expand the name of a container image by adding the digest, and the -fips / -distroless suffix if configured.
+Construct a container image name from a registry, repository, tag, and digest.
 */}}
 {{- define "gloo.image" -}}
 {{- $image := printf "%s/%s" .registry .repository -}}
-{{- if and .fips .fipsDigest -}}
+
 {{- /*
-In consideration of https://github.com/solo-io/gloo/issues/7326, we want the ability for -fips images to use their own digests,
-rather than falling back (incorrectly) onto the digests of non-fips images
+for fips or fips-distroless variants: add -fips to the image repo (name)
 */ -}}
-{{- $image = printf "%s-fips:%s@%s" $image .tag .fipsDigest -}}
-{{- else -}} {{- /* if and .fips .fipsDigest */ -}}
 {{- if or .fips (has .variant (list "fips" "fips-distroless")) -}}
 {{- $fipsSupportedImages := list "gloo-ee" "extauth-ee" "gloo-ee-envoy-wrapper" "rate-limit-ee" "discovery-ee" "sds-ee" -}}
 {{- if (has .repository $fipsSupportedImages) -}}
 {{- $image = printf "%s-fips" $image -}}
 {{- end -}}{{- /* if (has .repository $fipsSupportedImages) */ -}}
-{{- end -}}{{- /* if .fips */ -}}
+{{- end -}}{{- /* if or .fips (has .variant (list "fips" "fips-distroless")) */ -}}
+
+{{- /*
+add tag, if it exists
+*/ -}}
+{{- if .tag -}}
 {{- $image = printf "%s:%s" $image .tag -}}
-{{- if has .variant (list "distroless" "fips-distroless") -}}
+{{- end -}}{{- /* if .tag */ -}}
+
+{{- /*
+for distroless or fips-distroless variants: add -distroless to the tag
+*/ -}}
+{{- if and .tag (has .variant (list "distroless" "fips-distroless")) -}}
 {{- $distrolessSupportedImages := list "gloo" "gloo-envoy-wrapper" "discovery" "sds" "certgen" "kubectl" "access-logger" "ingress" "gloo-ee" "extauth-ee" "gloo-ee-envoy-wrapper" "rate-limit-ee" "discovery-ee" "sds-ee" "observability-ee" "caching-ee" -}}
 {{- if (has .repository $distrolessSupportedImages) -}}
 {{- $image = printf "%s-distroless" $image -}} {{- /* Add distroless suffix to the tag since it contains the same binaries in a different container */ -}}
 {{- end -}}{{- /* if (has .repository $distrolessSupportedImages) */ -}}
-{{- end -}}{{- /* if .distroless */ -}}
-{{- if .digest -}}
-{{- $image = printf "%s@%s" $image .digest -}}
-{{- end -}}{{- /* if .digest */ -}}
-{{- end -}}{{- /* if and .fips .fipsDigest */ -}}
+{{- end -}}{{- /* if and .tag (has .variant (list "distroless" "fips-distroless")) */ -}}
+
+{{- /*
+add digest for the chosen variant, if it exists
+*/ -}}
+{{- if or .fips (eq .variant "fips") -}}
+  {{- if .fipsDigest -}}
+    {{- $image = printf "%s@%s" $image .fipsDigest -}}
+  {{- end -}}{{- /* if .fipsDigest */ -}}
+{{- else if eq .variant "distroless" -}}
+  {{- if .distrolessDigest -}}
+    {{- $image = printf "%s@%s" $image .distrolessDigest -}}
+  {{- end -}}{{- /* if .distrolessDigest */ -}}
+{{- else if eq .variant "fips-distroless" -}}
+  {{- if .fipsDistrolessDigest -}}
+    {{- $image = printf "%s@%s" $image .fipsDistrolessDigest -}}
+  {{- end -}}{{- /* if .fipsDistrolessDigest */ -}}
+{{- else -}}
+  {{- if .digest -}}{{- /* standard image digest */ -}}
+    {{- $image = printf "%s@%s" $image .digest -}}
+  {{- end -}}{{- /* if .digest */ -}}
+{{- end -}}
 {{ $image }}
 {{- end -}}{{- /* define "gloo.image" */ -}}
 
+
 {{- define "gloo.pullSecret" -}}
 {{- if .pullSecret -}}
 imagePullSecrets:

install/test/helm_suite_test.go

@@ -4,30 +4,28 @@ import (
 	"bytes"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
-	"strings"
 	"testing"
 	"text/template"
 
-	"github.com/pkg/errors"
-	"github.com/solo-io/k8s-utils/installutils/kuberesource"
-	rbacv1 "k8s.io/api/rbac/v1"
-
 	"github.com/ghodss/yaml"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
 	"github.com/solo-io/gloo/pkg/cliutil/helm"
 	"github.com/solo-io/gloo/projects/gloo/cli/pkg/cmd/install"
 	"github.com/solo-io/gloo/projects/gloo/pkg/defaults"
+	"github.com/solo-io/gloo/test/makefile"
 	glootestutils "github.com/solo-io/gloo/test/testutils"
 	soloHelm "github.com/solo-io/go-utils/helmutils"
 	"github.com/solo-io/go-utils/testutils"
+	"github.com/solo-io/k8s-utils/installutils/kuberesource"
 	. "github.com/solo-io/k8s-utils/manifesttestutils"
 	"helm.sh/helm/v3/pkg/action"
 	"helm.sh/helm/v3/pkg/chart/loader"
 	"helm.sh/helm/v3/pkg/release"
 	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 )
@@ -54,10 +52,10 @@ func TestHelm(t *testing.T) {
 }
 
 var _ = BeforeSuite(func() {
-	version = MustGetVersion()
+	version = makefile.MustGetVersion(".", "-C", "../../")
 	pullPolicy = corev1.PullIfNotPresent
 	// generate the values.yaml and Chart.yaml files
-	MustMake(".", "-C", "../../", "generate-helm-files", "-B")
+	makefile.MustMake(".", "-C", "../../", "generate-helm-files", "-B")
 })
 
 type renderTestCase struct {
@@ -78,66 +76,6 @@ func runTests(callback func(testCase renderTestCase)) {
 	}
 }
 
-func MustMake(dir string, args ...string) {
-	makeCmd := exec.Command("make", args...)
-	makeCmd.Dir = dir
-
-	makeCmd.Stdout = GinkgoWriter
-	makeCmd.Stderr = GinkgoWriter
-	err := makeCmd.Run()
-
-	ExpectWithOffset(1, err).NotTo(HaveOccurred())
-}
-
-func MustMakeReturnStdout(dir string, args ...string) string {
-	makeCmd := exec.Command("make", args...)
-	makeCmd.Dir = dir
-
-	var stdout bytes.Buffer
-	makeCmd.Stdout = &stdout
-
-	makeCmd.Stderr = GinkgoWriter
-	err := makeCmd.Run()
-
-	ExpectWithOffset(1, err).NotTo(HaveOccurred())
-
-	return stdout.String()
-}
-
-// MustGetVersion returns the VERSION that will be used to build the chart
-func MustGetVersion() string {
-	output := MustMakeReturnStdout(".", "-C", "../../", "print-VERSION") // use print-VERSION so version matches on forks
-	lines := strings.Split(output, "\n")
-
-	// output from a fork:
-	// <[]string | len:4, cap:4>: [
-	//	"make[1]: Entering directory '/workspace/gloo'",
-	//	"<VERSION>",
-	//	"make[1]: Leaving directory '/workspace/gloo'",
-	//	"",
-	// ]
-
-	// output from the gloo repo:
-	// <[]string | len:2, cap:2>: [
-	//	"<VERSION>",
-	//	"",
-	// ]
-
-	if len(lines) == 4 {
-		// This is being executed from a fork
-		return lines[1]
-	}
-
-	if len(lines) == 2 {
-		// This is being executed from the Gloo repo
-		return lines[0]
-	}
-
-	// Error loudly to prevent subtle failures
-	Fail(fmt.Sprintf("print-VERSION output returned unknown format. %v", lines))
-	return "version-not-found"
-}
-
 type ChartRenderer interface {
 	// returns a TestManifest containing all resources
 	RenderManifest(namespace string, values glootestutils.HelmValues) (TestManifest, error)