[NET-10985] Fix bug where imagePullSecrets were not set up for Gateways #4316

nathancoleman · 2024-09-13T19:31:08Z

Note

In order for the pull secrets to work for a Gateway, they must be available in any namespace that a Gateway is deployed to. This is already the case with injected mesh sidecars if you, for example, consume consul-dataplane from a private image registry, so I have not made any special accomadations for Gateways.

Changes proposed in this PR

Plumb global.imagePullSecrets onto the ServiceAccount created for each Gateway

How I've tested this PR

Created a private registry on DockerHub for consul-dataplane, which is used by the gateway's Deployment

docker pull hashicorp/consul-dataplane:1.5.3
docker tag hashicorp/consul-dataplane:1.5.3 <your_dockerhub_username>/consul-dataplane:1.5.3
docker login
docker push <your_dockerhub_username>/consul-dataplane:1.5.3

Created an image pull secret for DockerHub in my K8s cluster
Set global.imageConsulDataplane to the private registry version

Install using this version of the Helm chart and this build of consul-k8s-control-plane

values.yaml

global:
  name: consul
  datacenter: dc1
  imageConsulDataplane: docker.io/<your_dockerhub_username>/consul-dataplane:1.5.3
  imageK8S: consul-k8s-control-plane:local
  imagePullSecrets:
  - name: regcred
  tls:
    enabled: true
    enableAutoEncrypt: true
  acls:
    manageSystemACLs: true
connectInject:
  enabled: true

kind create cluster
make dev-docker && kind load docker-image consul-k8s-control-plane:local
helm upgrade --install consul /path/to/consul-k8s/charts/consul --namespace consul --create-namespace --values ./values.yaml

How I expect reviewers to test this PR

See above

Checklist

Tests added
CHANGELOG entry added

pawellegowski89 · 2024-09-24T09:40:38Z

Looks ok from my side :)

nathancoleman · 2024-09-24T14:00:17Z

@pawellegowski89 One thing that occurred to me while testing this yesterday is that the pull secrets will need to be available in any namespace where you want to deploy a Gateway; however, this is no different from deploying mesh sidecars today if you're deploying with a consul-dataplane from a private image repo, for example.

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.

…ccount

jm96441n · 2024-09-24T18:46:19Z

charts/consul/templates/connect-inject-configmap.yaml

@@ -0,0 +1,18 @@
+{{- if .Values.connectInject.enabled }}


do we need to create this is imagePullSecrets is not set?

It makes sense to gate this for now; however, in the future I am expecting this to be the place we put structured config values like this one that don't fit neatly into a flag value

nvm, I see how we handle if the image pull secrets is empty in the code, always having it results in nicer code paths

jm96441n · 2024-09-24T18:49:44Z

control-plane/api-gateway/gatekeeper/init.go

 // 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) {


was changing this a pointer receiver to keep it consistent with the rest of the methods for the Gatekeeper?

yep, just an inconsistency pointed out by the editor

jm96441n · 2024-09-24T18:49:54Z

control-plane/api-gateway/gatekeeper/init.go

@@ -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


jm96441n · 2024-09-24T18:53:00Z

control-plane/api-gateway/gatekeeper/serviceaccount.go

 )

 func (g *Gatekeeper) upsertServiceAccount(ctx context.Context, gateway gwv1beta1.Gateway, config common.HelmConfig) error {
-	if config.AuthMethod == "" && !config.EnableOpenShift {
+	if config.AuthMethod == "" && !config.EnableOpenShift && len(config.ImagePullSecrets) == 0 {


can we add a comment here about why we can't delete the service account if there are image pull secrets or maybe extract this to a function that can describe it a bit?

added a comment

.changelog/4316.txt

Co-authored-by: Blake Covarrubias <[email protected]>

…yment

pawellegowski89 · 2024-09-26T12:40:33Z

@pawellegowski89 One thing that occurred to me while testing this yesterday is that the pull secrets will need to be available in any namespace where you want to deploy a Gateway; however, this is no different from deploying mesh sidecars today if you're deploying with a consul-dataplane from a private image repo, for example.

You are right @nathancoleman, but that is the responsibility, so to speak, on the preparation of ns, not on the chart consul itself.

…ys (#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 <[email protected]> * 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 <[email protected]>

…p for Gateways into release/1.4.x (#4373) [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]>

…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]>

nathancoleman requested a review from missylbytes September 13, 2024 19:32

nathancoleman added backport/1.3.x backport/1.4.x backport/1.5.x labels Sep 13, 2024

nathancoleman added 8 commits September 24, 2024 14:43

Leave camp cleaner than I found it

34bbb73

Make path to config file configurable

316985a

Add changelog entry

aa0758f

Add note to changelog entry

b7c1824

Ensure ServiceAccount is created if any image pull secrets are provided

76415b8

Add test coverage for image pull secret inclusion on gateway ServiceA…

a6b9305

…ccount

Adjust note in changelog

365b2c2

nathancoleman force-pushed the gateway-image-pull-secrets branch from 5657c7e to 365b2c2 Compare September 24, 2024 18:43

nathancoleman marked this pull request as ready for review September 24, 2024 18:43

nathancoleman requested a review from sarahalsmiller September 24, 2024 18:43

jm96441n reviewed Sep 24, 2024

View reviewed changes

Add a helpful comment explaining when/why we create a ServiceAccount

ac64e83

blake reviewed Sep 24, 2024

View reviewed changes

.changelog/4316.txt Outdated Show resolved Hide resolved

nathancoleman and others added 4 commits September 24, 2024 16:31

Update .changelog/4316.txt

e6b636d

Co-authored-by: Blake Covarrubias <[email protected]>

Return ServiceAccount name when image pull secrets warrant it

1e3503c

Improve unit tests to assert presence of ServiceAccount name on Deplo…

8260f94

…yment

Copy helpful comment added elsewhere

16fceb9

missylbytes approved these changes Sep 26, 2024

View reviewed changes

nathancoleman added the backport/1.6.x Changes are backported to 1.6 label Sep 26, 2024

nathancoleman merged commit e959d33 into main Sep 27, 2024
52 of 53 checks passed

nathancoleman deleted the gateway-image-pull-secrets branch September 27, 2024 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NET-10985] Fix bug where imagePullSecrets were not set up for Gateways #4316

[NET-10985] Fix bug where imagePullSecrets were not set up for Gateways #4316

nathancoleman commented Sep 13, 2024 •

edited

Loading

pawellegowski89 commented Sep 24, 2024

nathancoleman commented Sep 24, 2024 •

edited

Loading

jm96441n Sep 24, 2024

nathancoleman Sep 24, 2024

jm96441n Sep 24, 2024

jm96441n Sep 24, 2024

nathancoleman Sep 24, 2024

jm96441n Sep 24, 2024

jm96441n Sep 24, 2024

nathancoleman Sep 24, 2024

pawellegowski89 commented Sep 26, 2024

[NET-10985] Fix bug where imagePullSecrets were not set up for Gateways #4316

[NET-10985] Fix bug where imagePullSecrets were not set up for Gateways #4316

Conversation

nathancoleman commented Sep 13, 2024 • edited Loading

Changes proposed in this PR

How I've tested this PR

How I expect reviewers to test this PR

Checklist

pawellegowski89 commented Sep 24, 2024

nathancoleman commented Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawellegowski89 commented Sep 26, 2024

nathancoleman commented Sep 13, 2024 •

edited

Loading

nathancoleman commented Sep 24, 2024 •

edited

Loading