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

…p for Gateways into release/1.3.x (#4372) [NET-10985] Fix bug where imagePullSecrets were not set up for Gateways (#4316) * Plumb global.imagePullSecrets through to Gateway's ServiceAccount Since pull secrets are a list of structured objects that cannot easily be passed as a flag value to the container, this approach uses a JSON config file that is created as a ConfigMap and then mounted into the connect-injector Pod and parsed on startup. * Leave camp cleaner than I found it * Make path to config file configurable * Add changelog entry * Add note to changelog entry * Ensure ServiceAccount is created if any image pull secrets are provided * Add test coverage for image pull secret inclusion on gateway ServiceAccount * Adjust note in changelog * Add a helpful comment explaining when/why we create a ServiceAccount * Update .changelog/4316.txt * Return ServiceAccount name when image pull secrets warrant it * Improve unit tests to assert presence of ServiceAccount name on Deployment * Copy helpful comment added elsewhere --------- Co-authored-by: Nathan Coleman <[email protected]> Co-authored-by: Blake Covarrubias <[email protected]>
hashicorp · Oct 3, 2024 · fa5e492 · fa5e492
1 parent cafb53d
commit fa5e492
Show file tree

Hide file tree

Showing 11 changed files with 103 additions and 39 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 }}
@@ -314,6 +315,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
@@ -329,6 +333,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
@@ -18,7 +18,9 @@ type HelmConfig struct {
 	// ImageDataplane is the Consul Dataplane image to use in gateway deployments.
 	ImageDataplane string
 	// ImageConsulK8S is the Consul Kubernetes Control Plane image to use in gateway deployments.
-	ImageConsulK8S             string
+	ImageConsulK8S string
+	// ImagePullSecrets reference one or more Secret(s) that contain the credentials to pull images from private image repos.
+	ImagePullSecrets           []v1.LocalObjectReference
 	ConsulDestinationNamespace string
 	NamespaceMirroringPrefix   string
 	EnableNamespaces           bool

diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper.go b/control-plane/api-gateway/gatekeeper/gatekeeper.go
@@ -96,7 +96,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
@@ -187,12 +187,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{},
 				services: []*corev1.Service{
@@ -211,7 +212,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": {
@@ -263,7 +266,6 @@ func TestUpsert(t *testing.T) {
 						},
 					}, "1", false, false),
 				},
-				serviceAccounts: []*corev1.ServiceAccount{},
 			},
 		},
 		"create a new gateway deployment with managed Service and ACLs": {
@@ -291,13 +293,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),
@@ -322,7 +325,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"}}),
 				},
 			},
 		},
@@ -430,7 +433,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),
@@ -448,12 +451,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),
@@ -478,7 +481,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,
@@ -515,7 +518,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),
@@ -538,12 +541,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),
@@ -562,7 +565,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,
@@ -900,7 +903,7 @@ func TestUpsert(t *testing.T) {
 			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", true),
@@ -910,7 +913,7 @@ func TestUpsert(t *testing.T) {
 				},
 				services: []*corev1.Service{},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 		},
@@ -1090,7 +1093,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{
@@ -1182,6 +1185,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 {
@@ -1352,7 +1358,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",
@@ -1405,7 +1411,7 @@ func configureDeployment(name, namespace string, labels map[string]string, repli
 					},
 					NodeSelector:       nodeSelector,
 					Tolerations:        tolerations,
-					ServiceAccountName: serviceAccoutName,
+					ServiceAccountName: serviceAccountName,
 				},
 			},
 		},
@@ -1529,7 +1535,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",
@@ -1550,6 +1556,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
@@ -31,7 +31,7 @@ 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 initContainer(config common.HelmConfig, name, namespace string) (corev1.Container, error) {
 	data := initContainerCommandData{

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,
 	}
 }
diff --git a/control-plane/subcommand/inject-connect/command.go b/control-plane/subcommand/inject-connect/command.go
@@ -50,6 +50,7 @@ type Command struct {
 	flagListen                string
 	flagCertDir               string // Directory with TLS certs for listening (PEM)
 	flagDefaultInject         bool   // True to inject by default
+	flagConfigFile            string // Path to a config file in JSON format
 	flagConsulImage           string // Docker image for Consul
 	flagConsulDataplaneImage  string // Docker image for Envoy
 	flagConsulK8sImage        string // Docker image for consul-k8s
@@ -180,6 +181,7 @@ func init() {
 func (c *Command) init() {
 	c.flagSet = flag.NewFlagSet("", flag.ContinueOnError)
 	c.flagSet.StringVar(&c.flagListen, "listen", ":8080", "Address to bind listener to.")
+	c.flagSet.StringVar(&c.flagConfigFile, "config-file", "", "Path to a JSON config file.")
 	c.flagSet.Var((*flags.FlagMapValue)(&c.flagNodeMeta), "node-meta",
 		"Metadata to set on the node, formatted as key=value. This flag may be specified multiple times to set multiple meta fields.")
 	c.flagSet.BoolVar(&c.flagDefaultInject, "default-inject", true, "Inject by default.")