From e959d33ad2b2f08e045256a2fbc5164ef0fec258 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Fri, 27 Sep 2024 12:51:33 -0400 Subject: [PATCH] [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 Co-authored-by: Blake Covarrubias * 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: Blake Covarrubias --- .changelog/4316.txt | 5 ++ .../templates/connect-inject-configmap.yaml | 18 +++++++ .../templates/connect-inject-deployment.yaml | 7 +++ .../api-gateway/common/helm_config.go | 2 + .../api-gateway/gatekeeper/gatekeeper.go | 4 +- .../api-gateway/gatekeeper/gatekeeper_test.go | 53 +++++++++++-------- control-plane/api-gateway/gatekeeper/init.go | 6 +-- .../api-gateway/gatekeeper/rolebinding.go | 7 +-- .../api-gateway/gatekeeper/serviceaccount.go | 22 ++++---- .../subcommand/inject-connect/command.go | 2 + .../inject-connect/v1controllers.go | 20 +++++++ 11 files changed, 105 insertions(+), 41 deletions(-) create mode 100644 .changelog/4316.txt create mode 100644 charts/consul/templates/connect-inject-configmap.yaml diff --git a/.changelog/4316.txt b/.changelog/4316.txt new file mode 100644 index 0000000000..5397ebd093 --- /dev/null +++ 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 new file mode 100644 index 0000000000..98a7dae45f --- /dev/null +++ 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 index 725c26df10..8eebcfe42f 100644 --- 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 index d551757c5b..175cef56ba 100644 --- 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 index 538444303f..79766219dc 100644 --- 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 index f6342ec725..0c60aa610b 100644 --- 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 index 6502b7be8f..e8a17dc8ea 100644 --- 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 index 1a60e752c8..f315b78402 100644 --- 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 index d1c5c9883a..64dc0b75dd 100644 --- 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 index 68addfc7b2..125c46ea2a 100644 --- a/control-plane/subcommand/inject-connect/command.go +++ b/control-plane/subcommand/inject-connect/command.go @@ -48,6 +48,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 @@ -174,6 +175,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 index fa6e36d507..dd6b3be739 100644 --- a/control-plane/subcommand/inject-connect/v1controllers.go +++ b/control-plane/subcommand/inject-connect/v1controllers.go @@ -5,10 +5,12 @@ package connectinject import ( "context" + "encoding/json" "fmt" "os" "github.com/hashicorp/consul-server-connection-manager/discovery" + v1 "k8s.io/api/core/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -31,6 +33,23 @@ func (c *Command) configureControllers(ctx context.Context, mgr manager.Manager, // Create Consul API config object. consulConfig := c.consul.ConsulClientConfig() + type FileConfig struct { + ImagePullSecrets []v1.LocalObjectReference `json:"image_pull_secrets"` + } + + var cfgFile FileConfig + if c.flagConfigFile != "" { + if file, err := os.ReadFile(c.flagConfigFile); err != nil { + setupLog.Info("Failed to read specified -config-file", "file", c.flagConfigFile, "error", err) + } else { + if err := json.Unmarshal(file, &cfgFile); err != nil { + setupLog.Error(err, "Config file present but could not be deserialized, will use defaults", "file", c.flagConfigFile) + } else { + setupLog.Info("Config file present and deserialized", "file", c.flagConfigFile, "config", cfgFile) + } + } + } + // Convert allow/deny lists to sets. allowK8sNamespaces := flags.ToSet(c.flagAllowK8sNamespacesList) denyK8sNamespaces := flags.ToSet(c.flagDenyK8sNamespacesList) @@ -118,6 +137,7 @@ func (c *Command) configureControllers(ctx context.Context, mgr manager.Manager, }, ImageDataplane: c.flagConsulDataplaneImage, ImageConsulK8S: c.flagConsulK8sImage, + ImagePullSecrets: cfgFile.ImagePullSecrets, GlobalImagePullPolicy: c.flagGlobalImagePullPolicy, ConsulDestinationNamespace: c.flagConsulDestinationNamespace, NamespaceMirroringPrefix: c.flagK8SNSMirroringPrefix,