Backport of [NET-10985] Fix bug where imagePullSecrets were not set up for Gateways into release/1.4.x (#4373)

hc-github-team-consul-core · nathancoleman · blake · web-flow · commit e7e743af960b · 2024-10-03T10:56:53.000-05:00
[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 <nathan.coleman@hashicorp.com> Co-authored-by: Blake Covarrubias <blake@covarrubi.as>
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 }}
@@ -317,6 +318,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
@@ -332,6 +336,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
@@ -195,12 +195,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{
@@ -219,7 +220,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": {
@@ -271,7 +274,6 @@ func TestUpsert(t *testing.T) {
 						},
 					}, "1", false, false),
 				},
-				serviceAccounts: []*corev1.ServiceAccount{},
 			},
 		},
 		"create a new gateway deployment with managed Service and ACLs": {
@@ -299,13 +301,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),
@@ -330,7 +333,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"}}),
 				},
 			},
 		},
@@ -438,7 +441,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),
@@ -456,12 +459,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),
@@ -486,7 +489,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,
@@ -523,7 +526,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),
@@ -546,12 +549,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),
@@ -570,7 +573,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,
@@ -924,7 +927,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),
@@ -934,7 +937,7 @@ func TestUpsert(t *testing.T) {
 				},
 				services: []*corev1.Service{},
 				serviceAccounts: []*corev1.ServiceAccount{
-					configureServiceAccount(name, namespace, labels, "1"),
+					configureServiceAccount(name, namespace, labels, "1", nil),
 				},
 			},
 		},
@@ -1114,7 +1117,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{
@@ -1210,6 +1213,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 {
@@ -1403,7 +1409,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",
@@ -1456,7 +1462,7 @@ func configureDeployment(name, namespace string, labels map[string]string, repli
 					},
 					NodeSelector:       nodeSelector,
 					Tolerations:        tolerations,
-					ServiceAccountName: serviceAccoutName,
+					ServiceAccountName: serviceAccountName,
 				},
 			},
 		},
@@ -1580,7 +1586,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",
@@ -1601,6 +1607,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
@@ -36,7 +36,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 (g Gatekeeper) 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
@@ -51,6 +51,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
@@ -183,6 +184,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.")
diff --git a/control-plane/subcommand/inject-connect/v1controllers.go b/control-plane/subcommand/inject-connect/v1controllers.go

Original file line number	Diff line number	Diff line change
`@@ -96,7 +96,9 @@ func (g *Gatekeeper) namespacedName(gateway gwv1beta1.Gateway) types.NamespacedN`
`96`	`96`	`}`
`97`	`97`
`98`	`98`	`func (g *Gatekeeper) serviceAccountName(gateway gwv1beta1.Gateway, config common.HelmConfig) string {`
`99`		`- if config.AuthMethod == "" && !config.EnableOpenShift {`
	`99`	`+ // We only create a ServiceAccount if it's needed for RBAC or image pull secrets;`
	`100`	`+ // otherwise, we clean up if one was previously created.`
	`101`	`+ if config.AuthMethod == "" && !config.EnableOpenShift && len(config.ImagePullSecrets) == 0 {`
`100`	`102`	`return ""`
`101`	`103`	`}`
`102`	`104`	`return gateway.Name`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ type initContainerCommandData struct {`
`36`	`36`	`LogJSON bool`
`37`	`37`	`}`
`38`	`38`
`39`		`-// containerInit returns the init container spec for connect-init that polls for the service and the connect proxy service to be registered`
	`39`	`+// initContainer returns the init container spec for connect-init that polls for the service and the connect proxy service to be registered`
`40`	`40`	`// so that it can save the proxy service id to the shared volume and boostrap Envoy with the proxy-id.`
`41`	`41`	`func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace string) (corev1.Container, error) {`
`42`	`42`	`data := initContainerCommandData{`