Control plane debug

[zaharidichev]

This pr fixes a problem where control plane component cannot be injected with a debug sidecar. The problem was stemming from the fact that during inject, the control plane components are stripped from all linkerd.io metadata. This later on causes inconsistency when the proxy is injected again and leads to invalid yaml being produced causing an error in the form of:

The Deployment "linkerd-tap" is invalid: spec.template.metadata.labels: Invalid value: map[string]string(nil): selectordoes not match templatelabels``

In addition to that this pr introduces a check that ensures that we can run inject on control plane components only if the --manual flag is present.

Fixes #3396

[grampelberg]

This doesn't appear to be working for emojivoto:

$ kubectl -n emojivoto get deploy -o yaml | bin/go-run cli debug-sidecar inject -
deployment "emoji" skipped
deployment "vote-bot" skipped
deployment "voting" skipped
deployment "web" skipped

I suspect it is because of the mix of auto-injection via annotation and manual. It probably isn't the end of the world if this doesn't work, as you can just use linkerd inject to fix it. I think we'll want to change the command name in that case though. Some brainstorming:

linkerd internal debug-sidecar -
linkerd dev add-debug-sidecar -
linkerd diagnose control-plane-debug -

@olix0r what's your thinking? We're starting to introduce a set of commands like this that are for internal diagnosis.

[zaharidichev]

Most likely, will take a look. Emoji is meshed, right ?

So yes @grampelberg I actually did not realize that when the proxy is autoinjected, it does not actually appear in the yaml that we are processing, so my requirement for injecting a debug sidecar is that there is a proxy container running. This can be changed to also check whether there is "linkerd.io/inject":"enabled" annotation present. I am not sure whether that poses any kind of race conditions. If injection autoinjection is enabled with debug sidecar and we try to do debug-sidecar inject, can we end up stepping on our own toes?

[grampelberg]

Not by default, just pass it through linkerd inject.

[adleong]

Can you explain what the bug is that prevents the debug container from being injected into control plane components and how it is fixed?

I'm confused as to why we need a new linkerd CLI command to inject the debug container into control plane components. What doesn't linkerd inject work for this? Can we fix that rather than introducing a new command?

[grampelberg]

@adleong see #3396

• inject strips out the control plane labels.
• Updating inject to work with control plane components is far more complicated than it seems.
• As this will be used as an internal tool, keeping the scope down to just adding a new command that can only manage control plane components instead of solving the general problem.

[zaharidichev]

@adleong There was a problem where the uninject was removing some annotations from the control plane component when uninjecting and that was causing the buggy behavior. This has been fixed in this commit, right here. Adding a separate command is the result of having a discussion with @grampelberg around introducing a family of commands that only run on the control plane components and are used for more advanced level debugging.

[adleong]

If this is just an internal tool, I'd rather solve this problem with instructions for how to manually edit manifests or something like that rather than adding the technical debt of a new subcommand.

If you're unfamiliar with the complications of the implementation, it's unclear why this would be a separate command and whether the functionality is overlapping.

[zaharidichev]

@adleong I am fine with just scratching the whole separate command part and just fixing the problem that was preventing the debug sidecar injection into the control plane components. I think @grampelberg might also have an opinion on the matter.

[adleong]

I think @grampelberg was just trying to keep the scope narrow here so that this didn't balloon into a huge amount of work. But if you already have a fix that makes linkerd inject work properly for control plane components, that seems like the more correct change to me.

@grampelberg does that sound right to you?

[grampelberg]

What are the implications of adding it to inject? Is there a way that a user could shoot themselves in the foot with it there? The one that comes to mind immediately is inject without --manual on control plane components. That'd be easy to solve with some special code and an error, but I'm wondering if there are others there.

I'd rather solve this problem with instructions for how to manually edit manifests

It has been useful while helping folks debug issues to ask them to add the debug container to a control plane component and the run a command. So far, the instructions for doing it manually have been extremely hit and miss. I'm looking for something that is easy to explain and get the data back from.

[adleong]

If I understand correctly, running linkerd inject on a control plane component without using the --manual flag will always result in weirdness, since this will strip out the manually injected proxy. This is true regardless of the debug container. If someone tries to run linkerd inject on a control plane component without using the --manual flag, we should return an error.

With that established, I don't think we need a new command here.

[zaharidichev]

Alright then, I will remove the new command and we take it from there.

[zaharidichev]

Removed the additional command and left just the fix for the original bug that @grampelberg found. If you decide the command is needed, I can add it back. Up to you :)

[adleong]

I just tried this using this command:

$ k -n linkerd get deploy/linkerd-destination -o yaml | bin/linkerd inject --enable-debug-sidecar - | k apply -f -

However, since the auto-injector skips pods in the control plane namespace, this had the effect of stripping the proxy out of the pod:

linkerd-destination-8b76849f6-k76vt     1/1     Running   0          21s

I think, as we discussed above, running inject on a control plane component without the --manual flag should return an error.

[adleong]

With the manual flag, it works great, however:

k -n linkerd get deploy/linkerd-destination -o yaml | bin/linkerd inject --enable-debug-sidecar --manual - | k apply -f -

[zaharidichev]

@adleong yes, sorry I overlooked that one by mistake

[adleong]

@zaharidichev can you add a bit more context to the description about the bug that's being fixed here. I'm missing the background on why meta should or shouldn't be uninjected from control plane pods.

[adleong]

LGTM

[adleong]

This is pretty confusing so I think it warrants a more extensive comment explaining the context. Control plane components must only ever be manually injected, so they will never have the linkerd.io/inject annotation. Furthermore, since these components are part of linkerd itself, we don't want to strip off the other linkerd.io/* annotations such as linkerd.io/created-by when uninjecting.

Although.... maybe we DO want to strip off linkerd.io/proxy-version? wdyt?

[zaharidichev]

Yes, I will amend that, good call. Wrt to the linkerd.io/proxy-version my view is that there is no need to remove it. I assume if the value of the annotation has changed before the nject, then it will be updated. Isn't that correct?

Also, just to confirm my understanding, from end user perspective, uninject as a command would probably never be used on control plane components by a user, correct. Its only used as part of inject, which in turn is used not to really inject anything, but to update the injected container such as in the case of adding a debug sidecar. I wonder whether I am the only one who finds all that a bit confusing.

[adleong]

I wonder whether I am the only one who finds all that a bit confusing.

I'm pretty sure everyone finds this all very confusing.

[alpeb]

Works great, and it's awesome things settled down on the simplest solution 🏅