Backport of [NET-10985] Fix bug where imagePullSecrets were not set u…

…p for Gateways into release/1.6.x (#4375) * backport of commit 25ba129 * backport of commit 34bbb73 * backport of commit 316985a * backport of commit aa0758f * backport of commit b7c1824 * backport of commit 76415b8 * backport of commit a6b9305 * backport of commit 365b2c2 * backport of commit ac64e83 * backport of commit e6b636d * backport of commit 1e3503c * backport of commit 8260f94 * backport of commit 16fceb9 --------- Co-authored-by: Nathan Coleman <[email protected]>
hashicorp · Sep 27, 2024 · fbe0ae4 · fbe0ae4
1 parent 1d107b4
commit fbe0ae4
Show file tree

Hide file tree

Showing 11 changed files with 105 additions and 41 deletions.
diff --git a/.changelog/4316.txt b/.changelog/4316.txt
@@ -0,0 +1,5 @@
+```release-note:bug
+api-gateway: `global.imagePullSecrets` are now configured on the `ServiceAccount` for `Gateways`.
+
+Note: the referenced image pull Secret(s) must be present in the same namespace the `Gateway` is deployed to.
+```
diff --git a/charts/consul/templates/connect-inject-configmap.yaml b/charts/consul/templates/connect-inject-configmap.yaml
@@ -0,0 +1,18 @@
+{{- if .Values.connectInject.enabled }}
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: {{ template "consul.fullname" . }}-connect-inject-config
+  namespace: {{ .Release.Namespace }}
+  labels:
+    app: {{ template "consul.name" . }}
+    chart: {{ template "consul.chart" . }}
+    heritage: {{ .Release.Service }}
+    release: {{ .Release.Name }}
+    component: connect-injector
+data:
+  config.json: |
+    {
+      "image_pull_secrets": {{ .Values.global.imagePullSecrets | toJson }}
+    }
+{{- end }}
diff --git a/charts/consul/templates/connect-inject-deployment.yaml b/charts/consul/templates/connect-inject-deployment.yaml
@@ -141,6 +141,7 @@ spec:
             - "-ec"
             - |
               exec consul-k8s-control-plane inject-connect \
+                -config-file=/consul/config/config.json \
                 {{- if .Values.global.federation.enabled }}
                 -enable-federation \
                 {{- end }}
@@ -311,6 +312,9 @@ spec:
             successThreshold: 1
             timeoutSeconds: 5
           volumeMounts:
+            - name: config
+              mountPath: /consul/config
+              readOnly: true
           {{- if not (and .Values.global.secretsBackend.vault.enabled .Values.global.secretsBackend.vault.connectInject.tlsCert.secretName) }}
             - name: certs
               mountPath: /etc/connect-injector/certs
@@ -326,6 +330,9 @@ spec:
             {{- toYaml . | nindent 12 }}
           {{- end }}
       volumes:
+        - name: config
+          configMap:
+            name: {{ template "consul.fullname" . }}-connect-inject-config
       {{- if not (and .Values.global.secretsBackend.vault.enabled .Values.global.secretsBackend.vault.connectInject.tlsCert.secretName) }}
         - name: certs
           secret:

diff --git a/control-plane/api-gateway/common/helm_config.go b/control-plane/api-gateway/common/helm_config.go
@@ -19,6 +19,8 @@ type HelmConfig struct {
 	ImageDataplane string
 	// ImageConsulK8S is the Consul Kubernetes Control Plane image to use in gateway deployments.
 	ImageConsulK8S string
+	// ImagePullSecrets reference one or more Secret(s) that contain the credentials to pull images from private image repos.
+	ImagePullSecrets []v1.LocalObjectReference
 	// GlobalImagePullPolicy is the pull policy to use for all images used in gateway deployments.
 	GlobalImagePullPolicy      string
 	ConsulDestinationNamespace string

diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper.go b/control-plane/api-gateway/gatekeeper/gatekeeper.go
@@ -106,7 +106,9 @@ func (g *Gatekeeper) namespacedName(gateway gwv1beta1.Gateway) types.NamespacedN
 }
 
 func (g *Gatekeeper) serviceAccountName(gateway gwv1beta1.Gateway, config common.HelmConfig) string {
-	if config.AuthMethod == "" && !config.EnableOpenShift {
+	// We only create a ServiceAccount if it's needed for RBAC or image pull secrets;
+	// otherwise, we clean up if one was previously created.
+	if config.AuthMethod == "" && !config.EnableOpenShift && len(config.ImagePullSecrets) == 0 {
 		return ""
 	}
 	return gateway.Name

diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go
@@ -197,12 +197,13 @@ func TestUpsert(t *testing.T) {
 				},
 			},
 			helmConfig: common.HelmConfig{
-				ImageDataplane: dataplaneImage,
+				ImageDataplane:   dataplaneImage,
+				ImagePullSecrets: []corev1.LocalObjectReference{{Name: "my-secret"}},
 			},
 			initialResources: resources{},
 			finalResources: resources{
 				deployments: []*appsv1.Deployment{
-					configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"),
+					configureDeployment(name, namespace, labels, 3, nil, nil, name, "1"),
 				},
 				roles: []*rbac.Role{},
 				secrets: []*corev1.Secret{
@@ -224,7 +225,9 @@ func TestUpsert(t *testing.T) {
 						},
 					}, "1", false, false),
 				},
-				serviceAccounts: []*corev1.ServiceAccount{},
+				serviceAccounts: []*corev1.ServiceAccount{
+					configureServiceAccount(name, namespace, labels, "1", []corev1.LocalObjectReference{{Name: "my-secret"}}),
+				},
 			},
 		},
 		"create a new gateway deployment with managed Service": {
@@ -279,7 +282,6 @@ func TestUpsert(t *testing.T) {
 						},
 					}, "1", false, false),
 				},
-				serviceAccounts: []*corev1.ServiceAccount{},
 			},
 		},
 		"create a new gateway deployment with managed Service and ACLs": {
@@ -307,13 +309,14 @@ func TestUpsert(t *testing.T) {
 				},
 			},
 			helmConfig: common.HelmConfig{
-				AuthMethod:     "method",
-				ImageDataplane: dataplaneImage,
+				AuthMethod:       "method",
+				ImageDataplane:   dataplaneImage,
+				ImagePullSecrets: []corev1.LocalObjectReference{{Name: "my-secret"}},
 			},
 			initialResources: resources{},
 			finalResources: resources{
 				deployments: []*appsv1.Deployment{
-					configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"),
+					configureDeployment(name, namespace, labels, 3, nil, nil, name, "1"),
 				},
 				roles: []*rbac.Role{
 					configureRole(name, namespace, labels, "1", false),
@@ -341,7 +344,7 @@ func TestUpsert(t *testing.T) {
 					}, "1", false, false),
 				},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", []corev1.LocalObjectReference{{Name: "my-secret"}}),
 				},
 			},
 		},
@@ -451,7 +454,7 @@ func TestUpsert(t *testing.T) {
 			},
 			initialResources: resources{
 				deployments: []*appsv1.Deployment{
-					configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"),
+					configureDeployment(name, namespace, labels, 3, nil, nil, name, "1"),
 				},
 				roles: []*rbac.Role{
 					configureRole(name, namespace, labels, "1", false),
@@ -472,12 +475,12 @@ func TestUpsert(t *testing.T) {
 					}, "1", true, false),
 				},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 			finalResources: resources{
 				deployments: []*appsv1.Deployment{
-					configureDeployment(name, namespace, labels, 3, nil, nil, "", "2"),
+					configureDeployment(name, namespace, labels, 3, nil, nil, name, "2"),
 				},
 				roles: []*rbac.Role{
 					configureRole(name, namespace, labels, "1", false),
@@ -505,7 +508,7 @@ func TestUpsert(t *testing.T) {
 					}, "2", false, false),
 				},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 			ignoreTimestampOnService: true,
@@ -542,7 +545,7 @@ func TestUpsert(t *testing.T) {
 			},
 			initialResources: resources{
 				deployments: []*appsv1.Deployment{
-					configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"),
+					configureDeployment(name, namespace, labels, 3, nil, nil, name, "1"),
 				},
 				roles: []*rbac.Role{
 					configureRole(name, namespace, labels, "1", false),
@@ -568,12 +571,12 @@ func TestUpsert(t *testing.T) {
 					}, "1", true, false),
 				},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 			finalResources: resources{
 				deployments: []*appsv1.Deployment{
-					configureDeployment(name, namespace, labels, 3, nil, nil, "", "2"),
+					configureDeployment(name, namespace, labels, 3, nil, nil, name, "2"),
 				},
 				roles: []*rbac.Role{
 					configureRole(name, namespace, labels, "1", false),
@@ -595,7 +598,7 @@ func TestUpsert(t *testing.T) {
 					}, "2", false, false),
 				},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 			ignoreTimestampOnService: true,
@@ -955,7 +958,7 @@ func TestUpsert(t *testing.T) {
 			},
 			finalResources: resources{
 				deployments: []*appsv1.Deployment{
-					configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"),
+					configureDeployment(name, namespace, labels, 3, nil, nil, name, "1"),
 				},
 				roles: []*rbac.Role{
 					configureRole(name, namespace, labels, "1", true),
@@ -966,7 +969,7 @@ func TestUpsert(t *testing.T) {
 				secrets:  []*corev1.Secret{},
 				services: []*corev1.Service{},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 		},
@@ -1311,7 +1314,7 @@ func TestDelete(t *testing.T) {
 					}, "1", true, false),
 				},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 			finalResources: resources{
@@ -1377,7 +1380,7 @@ func TestDelete(t *testing.T) {
 					}, "1", true, false),
 				},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 			finalResources: resources{
@@ -1475,6 +1478,9 @@ func validateResourcesExist(t *testing.T, client client.Client, helmConfig commo
 		require.Equal(t, expected.Spec.Template.ObjectMeta.Annotations, actual.Spec.Template.ObjectMeta.Annotations)
 		require.Equal(t, expected.Spec.Template.ObjectMeta.Labels, actual.Spec.Template.Labels)
 
+		// Ensure the service account is assigned
+		require.Equal(t, expected.Spec.Template.Spec.ServiceAccountName, actual.Spec.Template.Spec.ServiceAccountName)
+
 		// Ensure there is an init container
 		hasInitContainer := false
 		for _, container := range actual.Spec.Template.Spec.InitContainers {
@@ -1684,7 +1690,7 @@ func validateResourcesAreDeleted(t *testing.T, k8sClient client.Client, resource
 	return nil
 }
 
-func configureDeployment(name, namespace string, labels map[string]string, replicas int32, nodeSelector map[string]string, tolerations []corev1.Toleration, serviceAccoutName, resourceVersion string) *appsv1.Deployment {
+func configureDeployment(name, namespace string, labels map[string]string, replicas int32, nodeSelector map[string]string, tolerations []corev1.Toleration, serviceAccountName, resourceVersion string) *appsv1.Deployment {
 	return &appsv1.Deployment{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: "apps/v1",
@@ -1737,7 +1743,7 @@ func configureDeployment(name, namespace string, labels map[string]string, repli
 					},
 					NodeSelector:       nodeSelector,
 					Tolerations:        tolerations,
-					ServiceAccountName: serviceAccoutName,
+					ServiceAccountName: serviceAccountName,
 				},
 			},
 		},
@@ -1886,7 +1892,7 @@ func configureService(name, namespace string, labels, annotations map[string]str
 	return &service
 }
 
-func configureServiceAccount(name, namespace string, labels map[string]string, resourceVersion string) *corev1.ServiceAccount {
+func configureServiceAccount(name, namespace string, labels map[string]string, resourceVersion string, pullSecrets []corev1.LocalObjectReference) *corev1.ServiceAccount {
 	return &corev1.ServiceAccount{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: "v1",
@@ -1907,6 +1913,7 @@ func configureServiceAccount(name, namespace string, labels map[string]string, r
 				},
 			},
 		},
+		ImagePullSecrets: pullSecrets,
 	}
 }
 

diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go
@@ -7,12 +7,12 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"k8s.io/utils/ptr"
 	"strconv"
 	"strings"
 	"text/template"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/hashicorp/consul-k8s/control-plane/api-gateway/common"
@@ -36,9 +36,9 @@ type initContainerCommandData struct {
 	LogJSON  bool
 }
 
-// containerInit returns the init container spec for connect-init that polls for the service and the connect proxy service to be registered
+// initContainer returns the init container spec for connect-init that polls for the service and the connect proxy service to be registered
 // so that it can save the proxy service id to the shared volume and boostrap Envoy with the proxy-id.
-func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace string) (corev1.Container, error) {
+func (g *Gatekeeper) initContainer(config common.HelmConfig, name, namespace string) (corev1.Container, error) {
 	data := initContainerCommandData{
 		AuthMethod:         config.AuthMethod,
 		LogLevel:           config.LogLevel,

diff --git a/control-plane/api-gateway/gatekeeper/rolebinding.go b/control-plane/api-gateway/gatekeeper/rolebinding.go
@@ -10,12 +10,13 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 
-	"github.com/hashicorp/consul-k8s/control-plane/api-gateway/common"
-	"github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1"
 	rbac "k8s.io/api/rbac/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
+
+	"github.com/hashicorp/consul-k8s/control-plane/api-gateway/common"
+	"github.com/hashicorp/consul-k8s/control-plane/api/v1alpha1"
 )
 
 func (g *Gatekeeper) upsertRoleBinding(ctx context.Context, gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig, config common.HelmConfig) error {
@@ -65,7 +66,7 @@ func (g *Gatekeeper) deleteRoleBinding(ctx context.Context, gwName types.Namespa
 
 func (g *Gatekeeper) roleBinding(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig, config common.HelmConfig) *rbac.RoleBinding {
 	// Create resources for reference. This avoids bugs if naming patterns change.
-	serviceAccount := g.serviceAccount(gateway)
+	serviceAccount := g.serviceAccount(gateway, config)
 	role := g.role(gateway, gcc, config)
 
 	return &rbac.RoleBinding{

diff --git a/control-plane/api-gateway/gatekeeper/serviceaccount.go b/control-plane/api-gateway/gatekeeper/serviceaccount.go
@@ -7,18 +7,20 @@ import (
 	"context"
 	"errors"
 
-	"github.com/hashicorp/consul-k8s/control-plane/api-gateway/common"
-	"k8s.io/apimachinery/pkg/types"
-	gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
-
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
+	gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
+
+	"github.com/hashicorp/consul-k8s/control-plane/api-gateway/common"
 )
 
 func (g *Gatekeeper) upsertServiceAccount(ctx context.Context, gateway gwv1beta1.Gateway, config common.HelmConfig) error {
-	if config.AuthMethod == "" && !config.EnableOpenShift {
+	// We only create a ServiceAccount if it's needed for RBAC or image pull secrets;
+	// otherwise, we clean up if one was previously created.
+	if config.AuthMethod == "" && !config.EnableOpenShift && len(config.ImagePullSecrets) == 0 {
 		return g.deleteServiceAccount(ctx, types.NamespacedName{Namespace: gateway.Namespace, Name: gateway.Name})
 	}
 
@@ -47,15 +49,12 @@ func (g *Gatekeeper) upsertServiceAccount(ctx context.Context, gateway gwv1beta1
 	}
 
 	// Create the ServiceAccount.
-	serviceAccount = g.serviceAccount(gateway)
+	serviceAccount = g.serviceAccount(gateway, config)
 	if err := ctrl.SetControllerReference(&gateway, serviceAccount, g.Client.Scheme()); err != nil {
 		return err
 	}
-	if err := g.Client.Create(ctx, serviceAccount); err != nil {
-		return err
-	}
 
-	return nil
+	return g.Client.Create(ctx, serviceAccount)
 }
 
 func (g *Gatekeeper) deleteServiceAccount(ctx context.Context, gwName types.NamespacedName) error {
@@ -69,12 +68,13 @@ func (g *Gatekeeper) deleteServiceAccount(ctx context.Context, gwName types.Name
 	return nil
 }
 
-func (g *Gatekeeper) serviceAccount(gateway gwv1beta1.Gateway) *corev1.ServiceAccount {
+func (g *Gatekeeper) serviceAccount(gateway gwv1beta1.Gateway, config common.HelmConfig) *corev1.ServiceAccount {
 	return &corev1.ServiceAccount{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      gateway.Name,
 			Namespace: gateway.Namespace,
 			Labels:    common.LabelsForGateway(&gateway),
 		},
+		ImagePullSecrets: config.ImagePullSecrets,
 	}
 }