Fix issue for debug containers when using custom Docker registry #3873

javaducky · 2019-12-30T18:35:48Z

Subject
Fixes bug where override of Docker registry was not being applied to debug containers (#3851)

Problem
Overrides for Docker registry are not being applied to debug containers and provide no means to correct the image.

Solution
This update expands the data.proxy configuration section within the Linkerd ConfigMap to maintain the overridden image name for debug containers at install-time similar to handling of the proxy and proxyInit images.

This change also enables the further override option of the registry for debug containers at inject-time given utilization of the --registry CLI option.

Validation
Several new unit tests have been created to confirm functionality. In addition, the following workflows were run through:

Standard Workflow with Custom Registry

This workflow installs Linkerd control plane based upon a custom registry, then injecting the debug sidecar into a service.

Start with a k8s instance having no Linkerd installation
Build all images locally using bin/docker-build
Create custom tags (using same version) for generated images, e.g. docker tag gcr.io/linkerd-io/debug:git-a4ebecb6 javaducky.com/linkerd-io/debug:git-a4ebecb6
Install Linkerd with registry override bin/linkerd install --registry=javaducky.com/linkerd-io | kubectl apply -f -
Once Linkerd has been fully initialized, you should be able to confirm that the linkerd-config ConfigMap now contains the debug image name, pull policy, and version within the data.proxy section
Request injection of the debug image into an available container. I used the Emojivoto voting service as described in https://linkerd.io/2/tasks/using-the-debug-container/ as kubectl -n emojivoto get deploy/voting -o yaml | bin/linkerd inject --enable-debug-sidecar - | kubectl apply -f -
Once the deployment creates a new pod for the service, inspection should show that the container now includes the "linkerd-debug" container name based on the applicable override image seen previously within the ConfigMap
Debugging can also be verified by viewing debug container logs as kubectl -n emojivoto logs deploy/voting linkerd-debug -f
Modifying the config.linkerd.io/enable-debug-sidecar annotation, setting to “false”, should show that the pod will be recreated no longer running the debug container.

Overriding the Custom Registry Override at Injection

This builds upon the “Standard Workflow with Custom Registry” by overriding the Docker registry utilized for the debug container at the time of injection.

“Clean” the Emojivoto voting service by removing any Linkerd annotations from the deployment
Request injection similar to before, except provide the --registry option as in kubectl -n emojivoto get deploy/voting -o yaml | bin/linkerd inject --enable-debug-sidecar --registry=gcr.io/linkerd-io - | kubectl apply -f -
Inspection of the deployment config should now show the override annotation for config.linkerd.io/debug-image having the debug container from the new registry. Viewing the running pod should show that the linkerd-debug container was injected and running the correct image. Of note, the proxy and proxy-init images are still running the “original” override images.
As before, modifying the config.linkerd.io/enable-debug-sidecar annotation setting to “false”, should show that the pod will be recreated no longer running the debug container.

Standard Workflow with Default Registry

This workflow is the typical workflow which utilizes the standard Linkerd image registry.

Uninstall the Linkerd control plane using bin/linkerd install --ignore-cluster | kubectl delete -f - as described at https://linkerd.io/2/tasks/uninstall/
Clean the Emojivoto environment using curl -sL https://run.linkerd.io/emojivoto.yml | kubectl delete -f - then reinstall using curl -sL https://run.linkerd.io/emojivoto.yml | kubectl apply -f -
Perform standard Linkerd installation as bin/linkerd install | kubectl apply -f -
Once Linkerd has been fully initialized, you should be able to confirm that the linkerd-config ConfigMap references the default debug image of gcr.io/linkerd-io/debug within the data.proxy section
Request injection of the debug image into an available container as kubectl -n emojivoto get deploy/voting -o yaml | bin/linkerd inject --enable-debug-sidecar - | kubectl apply -f -
Debugging can also be verified by viewing debug container logs as kubectl -n emojivoto logs deploy/voting linkerd-debug -f
Modifying the config.linkerd.io/enable-debug-sidecar annotation, setting to “false”, should show that the pod will be recreated no longer running the debug container.

Overriding the Default Registry at Injection

This workflow builds upon the “Standard Workflow with Default Registry” by overriding the Docker registry utilized for the debug container at the time of injection.

“Clean” the Emojivoto voting service by removing any Linkerd annotations from the deployment
Request injection similar to before, except provide the --registry option as in kubectl -n emojivoto get deploy/voting -o yaml | bin/linkerd inject --enable-debug-sidecar --registry=javaducky.com/linkerd-io - | kubectl apply -f -
Inspection of the deployment config should now show the override annotation for config.linkerd.io/debug-image having the debug container from the new registry. Viewing the running pod should show that the linkerd-debug container was injected and running the correct image. Of note, the proxy and proxy-init images are still running the “original” override images.
As before, modifying the config.linkerd.io/enable-debug-sidecar annotation setting to “false”, should show that the pod will be recreated no longer running the debug container.

Fixes issue #3851

Signed-off-by: Paul Balogh javaducky@gmail.com

alpeb · 2020-01-02T19:24:04Z

Thanks @javaducky, looks pretty exhaustive! 🎉
I've just started to review this. When running the full integration suite I found there's an error when trying to upgrade from the latest stable (2.6.1) version. In cli/cmd/inject.go when you try doing

configs.Debug.DebugImage.PullPolicy = options.imagePullPolicy

It causes a panic, presumably because configs.Debug is empty in 2.6.1, so you need to account for that case.
FYI the deep integration tests aren't run automatically in github for PRs, but you can run full suite in your machine as explained here.

javaducky · 2020-01-02T20:15:12Z

Awesome, thank you! I'll get that fixed and make sure to run those tests locally. Lots of angles with this ;)

javaducky · 2020-01-03T18:41:18Z

@alpeb I've corrected the upgrade issue and have confirmed that the integration tests are able to fully pass. I apologize for not having run the full suite previously.

alpeb · 2020-01-07T14:14:19Z

Thanks @javaducky . Do you mind merging against master before we resume reviewing?

javaducky · 2020-01-08T05:44:26Z

@alpeb, this should be good again given the rebase to latest master.

adleong

This is great work, @javaducky!

Just as a thought, what do you think about putting the debug sidecar image configuration in the Proxy config object rather than creating a new Debug config object? This is what we have done for the proxy-init container and I think it's reasonable to treat the Proxy config object as configuration for all injected sidecar containers.

javaducky · 2020-01-09T01:16:14Z

@adleong thanks, I'm definitely having fun with this project! As for your suggestion, including the debug configuration options in the data.proxy section was one of the original thoughts that had been discussed in Slack (conversations start around December 27). I only received feedback on that from @alpeb given the holidays, so I went with it.

Having the separation from the proxy felt a bit cleaner to me, but I'm totally fine with including it in the data.proxy section if that's preferred. Not sure if any others might be interested in taking a quick look at the options I had posted originally. Option 3 is what is currently implemented, Option 2 seems to match what you're suggesting.

As I mentioned, I'm fine either way and it's not a huge pain to change if one option is deemed preferable over another. Other than including @alpeb again, I'm unsure of who else should be asked to weigh their opinion. @grampelberg I'm guessing?

Please let know!

adleong · 2020-01-09T22:53:03Z

I chatted briefly with @alpeb and we think it makes sense to put this in the proxy config (i.e. option 2).

javaducky · 2020-01-09T23:01:43Z

Cool...not a problem! I should have those changes turned around pretty quick.

javaducky · 2020-01-10T07:02:42Z

@adleong and @alpeb . I've now updated the code to no longer use the custom data.debug configuration section any longer. Now all of the debug container information will be included in the data.proxy section. I've rebased the changes against the latest master.

javaducky · 2020-01-10T14:03:53Z

Updated "Validation" section of PR given most recent usage change.

charts/linkerd2/README.md

cli/cmd/inject.go

alpeb

Thanks @javaducky , this is even more solid 👍 Also thanks for the going the extra mile with the tests 😃
I've just got a couple of minor comments before approving 😉

charts/linkerd2/README.md

cli/cmd/upgrade.go

cli/cmd/inject.go

javaducky · 2020-01-14T13:35:09Z

@alpeb fyi, the tests are currently failing; presumably until https://github.com/linkerd/linkerd2/pull/3923/files is merged into master and rebased into this branch. I'll let you know once it should be safe to continue review.

I'm currently building with these changes and will confirm they correct things.

cli/cmd/inject.go

…debug containers (linkerd#3851) This fix creates the following a new "debug" configuration section. This allows for more consistent usage as with proxyImage and proxyInitImage. Registry flag does not set registry for debug container linkerd#3851 Signed-off-by: Paul Balogh <javaducky@gmail.com>

…issue Signed-off-by: Paul Balogh <javaducky@gmail.com>

…n of configs Signed-off-by: Paul Balogh <javaducky@gmail.com>

Signed-off-by: Paul Balogh <javaducky@gmail.com>

…a new data.debug section Signed-off-by: Paul Balogh <javaducky@gmail.com>

Signed-off-by: Paul Balogh <javaducky@gmail.com>

…on inject Signed-off-by: Paul Balogh <javaducky@gmail.com>

…ride and correction to README Signed-off-by: Paul Balogh <javaducky@gmail.com>

Signed-off-by: Paul Balogh <javaducky@gmail.com>

…date for README Signed-off-by: Paul Balogh <javaducky@gmail.com>

javaducky · 2020-01-15T22:54:07Z

@adleong @alpeb this should be ready for you again. 🤞

adleong

I've spot checked the inject output and run integration tests on this branch and everything looks good to me!

Thanks for your hard work on this, @javaducky!

cpretzer

Tests and spot check ran successfully for me as well.

LGTM!

alpeb

Looks good to me 👍 🥇
It is of note that grpc_server.go (and its test file) were amended for the Config() endpoint to return the install flags as well. That ended up not being used after all, but I think that's still a valuable fix to leave in 👍

javaducky · 2020-01-16T15:50:42Z

Yeah, I forgot that I left that in after removing the lookup of install flags.

Pothulapati

Had a look at the code, LGTM!

Thank you so much for working on this 💯

javaducky force-pushed the debug-registry-override branch 2 times, most recently from 68c5a25 to 5c5efc9 Compare December 30, 2019 19:27

javaducky force-pushed the debug-registry-override branch from 906dbb3 to 4392950 Compare January 3, 2020 16:00

javaducky mentioned this pull request Jan 3, 2020

Respect registry override during inject #3879

Merged

grampelberg assigned alpeb Jan 6, 2020

javaducky force-pushed the debug-registry-override branch 3 times, most recently from 5e5fbc8 to 079cd17 Compare January 8, 2020 05:27

adleong reviewed Jan 8, 2020

View reviewed changes

javaducky force-pushed the debug-registry-override branch from fbd1aa7 to c0a0954 Compare January 10, 2020 06:55

adleong reviewed Jan 10, 2020

View reviewed changes

charts/linkerd2/README.md Outdated Show resolved Hide resolved

adleong reviewed Jan 10, 2020

View reviewed changes

cli/cmd/inject.go Outdated Show resolved Hide resolved

cli/cmd/inject.go Outdated Show resolved Hide resolved

javaducky force-pushed the debug-registry-override branch from c0a0954 to e1efc88 Compare January 11, 2020 02:34

alpeb reviewed Jan 13, 2020

View reviewed changes

charts/linkerd2/README.md Outdated Show resolved Hide resolved

cli/cmd/upgrade.go Show resolved Hide resolved

cli/cmd/inject.go Outdated Show resolved Hide resolved

javaducky force-pushed the debug-registry-override branch 2 times, most recently from 4dc63ac to f62f4cb Compare January 14, 2020 04:17

javaducky force-pushed the debug-registry-override branch from f62f4cb to 4f08a49 Compare January 14, 2020 18:38

javaducky commented Jan 15, 2020

View reviewed changes

cli/cmd/inject.go Show resolved Hide resolved

adleong reviewed Jan 15, 2020

View reviewed changes

cli/cmd/inject.go Outdated Show resolved Hide resolved

cli/cmd/inject.go Show resolved Hide resolved

javaducky added 10 commits January 15, 2020 16:11

Post-rebase updates; updated new testcases and addressed scopelinter …

5e6e3b4

…issue Signed-off-by: Paul Balogh <javaducky@gmail.com>

Correcting upgrade issue from prior releases having no "debug" sectio…

2deef12

…n of configs Signed-off-by: Paul Balogh <javaducky@gmail.com>

Post-rebase update; bump version of prometheus to match updated tests

a426214

Signed-off-by: Paul Balogh <javaducky@gmail.com>

Review update; moving debug configurations to data.proxy rather than …

25071ed

…a new data.debug section Signed-off-by: Paul Balogh <javaducky@gmail.com>

Post-rebase updates to fix tests

36fbf6d

Signed-off-by: Paul Balogh <javaducky@gmail.com>

Review updates; update versions in README, cleanup registry override …

a56b265

…on inject Signed-off-by: Paul Balogh <javaducky@gmail.com>

Review updates; defensive coding for debug container on registry over…

4a56955

…ride and correction to README Signed-off-by: Paul Balogh <javaducky@gmail.com>

Review updates; simplify registry override

a69af6e

Signed-off-by: Paul Balogh <javaducky@gmail.com>

Update unit test to appease linter

d30e6b6

Signed-off-by: Paul Balogh <javaducky@gmail.com>

javaducky force-pushed the debug-registry-override branch from d81da22 to dac07b9 Compare January 15, 2020 22:46

Review update; improving the overwriteRegistry method; post-rebase up…

08005f7

…date for README Signed-off-by: Paul Balogh <javaducky@gmail.com>

javaducky force-pushed the debug-registry-override branch from dac07b9 to 08005f7 Compare January 15, 2020 22:47

adleong approved these changes Jan 16, 2020

View reviewed changes

cpretzer approved these changes Jan 16, 2020

View reviewed changes

alpeb approved these changes Jan 16, 2020

View reviewed changes

javaducky requested a review from Pothulapati January 17, 2020 16:45

Pothulapati approved these changes Jan 17, 2020

View reviewed changes

adleong merged commit dabee12 into linkerd:master Jan 17, 2020

javaducky deleted the debug-registry-override branch January 17, 2020 20:36

javaducky mentioned this pull request Jan 19, 2020

Update annotation documentation given changes from #3873 linkerd/website#637

Merged

adleong mentioned this pull request Jan 23, 2020

Registry flag does not set registry for debug container #3851

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue for debug containers when using custom Docker registry #3873

Fix issue for debug containers when using custom Docker registry #3873

javaducky commented Dec 30, 2019 •

edited

Loading

alpeb commented Jan 2, 2020

javaducky commented Jan 2, 2020

javaducky commented Jan 3, 2020

alpeb commented Jan 7, 2020

javaducky commented Jan 8, 2020

adleong left a comment

javaducky commented Jan 9, 2020

adleong commented Jan 9, 2020

javaducky commented Jan 9, 2020

javaducky commented Jan 10, 2020

javaducky commented Jan 10, 2020

alpeb left a comment

javaducky commented Jan 14, 2020 •

edited

Loading

javaducky commented Jan 15, 2020

adleong left a comment

cpretzer left a comment

alpeb left a comment

javaducky commented Jan 16, 2020

Pothulapati left a comment

Fix issue for debug containers when using custom Docker registry #3873

Fix issue for debug containers when using custom Docker registry #3873

Conversation

javaducky commented Dec 30, 2019 • edited Loading

Standard Workflow with Custom Registry

Overriding the Custom Registry Override at Injection

Standard Workflow with Default Registry

Overriding the Default Registry at Injection

alpeb commented Jan 2, 2020

javaducky commented Jan 2, 2020

javaducky commented Jan 3, 2020

alpeb commented Jan 7, 2020

javaducky commented Jan 8, 2020

adleong left a comment

Choose a reason for hiding this comment

javaducky commented Jan 9, 2020

adleong commented Jan 9, 2020

javaducky commented Jan 9, 2020

javaducky commented Jan 10, 2020

javaducky commented Jan 10, 2020

alpeb left a comment

Choose a reason for hiding this comment

javaducky commented Jan 14, 2020 • edited Loading

javaducky commented Jan 15, 2020

adleong left a comment

Choose a reason for hiding this comment

cpretzer left a comment

Choose a reason for hiding this comment

alpeb left a comment

Choose a reason for hiding this comment

javaducky commented Jan 16, 2020

Pothulapati left a comment

Choose a reason for hiding this comment

javaducky commented Dec 30, 2019 •

edited

Loading

javaducky commented Jan 14, 2020 •

edited

Loading