From fa5e49225b0dbbf35be542e2138a5b9ecc290b9d Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Thu, 3 Oct 2024 12:35:10 -0400 Subject: [PATCH] Backport of [NET-10985] Fix bug where imagePullSecrets were not set up 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 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 | 4 +- .../api-gateway/gatekeeper/gatekeeper.go | 4 +- .../api-gateway/gatekeeper/gatekeeper_test.go | 51 +++++++++++-------- control-plane/api-gateway/gatekeeper/init.go | 2 +- .../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, 103 insertions(+), 39 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 aeafad5b2b..2b8d12a8ef 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 }} @@ -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 index 9b0ab1d7e8..a42e76d347 100644 --- 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 index 6cb7170fc8..53e25512c1 100644 --- 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 index 8c0d842d49..a051696fcb 100644 --- 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 index 76c026c4a6..e8cab6578b 100644 --- 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 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 68b878b9de..f39dd2521f 100644 --- 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.") diff --git a/control-plane/subcommand/inject-connect/v1controllers.go b/control-plane/subcommand/inject-connect/v1controllers.go index 76c7a38b49..5930204437 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" @@ -30,6 +32,23 @@ func (c *Command) configureV1Controllers(ctx context.Context, mgr manager.Manage // 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) @@ -117,6 +136,7 @@ func (c *Command) configureV1Controllers(ctx context.Context, mgr manager.Manage }, ImageDataplane: c.flagConsulDataplaneImage, ImageConsulK8S: c.flagConsulK8sImage, + ImagePullSecrets: cfgFile.ImagePullSecrets, ConsulDestinationNamespace: c.flagConsulDestinationNamespace, NamespaceMirroringPrefix: c.flagK8SNSMirroringPrefix, EnableNamespaces: c.flagEnableNamespaces,