Consolidate Differences In Runtime YAML of Auto-Inject And Manual-Injected Workloads

[ihcsim]

With #2472, the proxy injector webhook now mutates the pod spec directly. This results in discrepancies between the YAML output generated by the linkerd inject command and that by the webhook.

At runtime, the output of kubectl describe [deploy|sts|job|rc|dameonset] of an auto-injected workload will not have the proxy specs, because the webhook modifies the pod specs, instead of its owner's pod template. The specs of the live pod becomes the only source of truth, which can be confusing for support purposes.

For 2.4, one way to consolidate that this is, as @grampelberg and @alpeb suggested, to devolve the linkerd inject to just adding linkerd annotations.

cc @grampelberg

[grampelberg]

As a project, we've relied on showing users exactly what is happening when they do something. By moving to auto-injecting pods instead of deployments, we've moved away from that a little bit. It feels like there needs to be a preview command that shows the actual YAML for the pods as a preview (linkerd inject --preview?) as well as unifying the two different places that containers can sit.

[alpeb]

linkerd inject --preview sounds like a great idea to me!

A few things to take into account with this approach though:

• The auto injector will always have to be installed (bye bye --proxy-auto-inject :+1: ). This means we can't support k8s versions lower than 1.9 (I think), which should be ok, no? Also maybe we can create a new --pre check to ensure the mutatingwebhook plugin is enabled.
• CLI uninject will now just set the linkerd.io/inject: disabled annotation on the parent workload. That'll trigger the re-creation of the pod and the auto-injector will ignore it. If the pod is an orphan, the same annotation will be set on the pod's meta, but apply might not trigger the UPDATE event in the webhook, so we might have to instruct users to use annotate. This still needs to be double checked.
• To reman consistent, we will no longer inject the proxies into the control plane pods during install; that'll happen as a side effect when they get applied. Is that ok?

[grampelberg]

can't support k8s versions lower than 1.9

I updated support to 1.10+ as part of 2.2.

If the pod is an orphan

Orphan pods are weird, can you update the containers in them? Are they restarted?

To reman consistent, we will no longer inject the proxies into the control plane pods during install; that'll happen as a side effect when they get applied.

Is there a bootstrapping issue we'd run into here? The MWC wouldn't be running and so none of the control plane pods would be injected?

[alpeb]

Orphan pods are weird, can you update the containers in them? Are they restarted?

I've just tested adding an annotation directly to a pod and applying the change. The webhook picks it through a pod UPDATE event, but disregards it because we ignore events on pods that are already injected. So I changed the webhook a bit to not ignore it, and applying again that change gives me this error:

* spec.containers: Forbidden: pod updates may not add or remove containers

So we can't support injection/uninjection of orphan pods through the webhook.

Is there a bootstrapping issue we'd run into here? The MWC wouldn't be running and so none of the control plane pods would be injected?

Right... Let me list some possible workarounds. None sounds ideal, but somebody might have something better to say about them:

• This could be included in the multi-stage install discussion (#2164). We either enforce a first step that installs the proxy injector, or we do all the components injections as a final optional step (alongside things like the CP service profiles generation).
• Have some process in the CP that watches the statuses of the linkerd deployments, and when it detects all is set, then set the linkerd.io/inject: enabled annotation in all of them to trigger the pods restarts and injection.
• Rely on Helm hooks (I know we don't want to enforce Helm, but listing it out for completeness sake...)

[grampelberg]

Man, you're right, none of those options are great. Having a special path for the CP isn't horrible. If we could come up with a good solution, it'd make the helm chart infinitely easier though (as injection wouldn't need to be encoded in the chart itself).

[ihcsim]

CLI uninject will now just set the linkerd.io/inject: disabled annotation on the parent workload.

Or just remove any linkerd.io/inject annotation. When this annotation isn't present, the webhook sees it as an opt-out.

• spec.containers: Forbidden: pod updates may not add or remove containers

This comes as a surprise. Pretty sure we can add the linkerd proxy container to an orphan pod today, via the webhook. In any case, we should be able to update the meta and containers of the pod. (Isn't that how auto-inject works?)

we will no longer inject the proxies into the control plane pods during install

My thought is we can make an exception for install. Bootstrapping of any distributed systems is always weird.

[alpeb]

Or just remove any linkerd.io/inject annotation. When this annotation isn't present, the webhook sees it as an opt-out.

:+1:

• spec.containers: Forbidden: pod updates may not add or remove containers

This comes as a surprise. Pretty sure we can add the linkerd proxy container to an orphan pod today, via the webhook. In any case, we should be able to update the meta and containers of the pod. (Isn't that how auto-inject works?)

Yes, when pods are created. My comment was regarding pods updates; apparently we can't inject/uninject a pod that was already created (we can change the proxy parameters though).

My thought is we can make an exception for install. Bootstrapping of any distributed systems is always weird.

Agreed, sounds like that'll be the most straightforward. That'll mean not being able to get rid of the CLI inject code, or as @grampelberg was afraid of above, put the proxy yamls in the chart templates themselves (parametrized).

[ihcsim]

Yes, when pods are created. My comment was regarding pods updates; apparently we can't inject/uninject a pod that was already created (we can change the proxy parameters though).

Yes, you are right, regarding not being able to remove the proxy container from a single-pod workload. I think it isn't specific to this issue, right? Mind creating an issue for it?

That'll mean not being able to get rid of the CLI inject code

I think we can just decouple the install code from inject, to call pkg/inject.

[alpeb]

Yes, you are right, regarding not being able to remove the proxy container from a single-pod workload. I think it isn't specific to this issue, right? Mind creating an issue for it?

Right. Looks like that's a limitation of the mutating webhook mechanism, so I've added a comment into #2338 and linkerd/linkerd2#2471 so that we document that behavior.

I think we can just decouple the install code from inject, to call pkg/inject.

We still should be able to make lots of simplifications for sure if the only case to cater for was restricted to just the CP components.

[klingerf]

Apologies for chiming in late on this issue. I want to make sure I understand the intent of this change. We're planning on removing the --proxy-auto-inject install flag, in favor of running the proxy injector pod in all control plane installs? And then we're planning on updating the CLI to stop performing injection, in favor of converting flags to annotations that will be processed exclusively by the proxy injector?

I'm frankly a bit wary of dropping support for CLI injection entirely, and forcing all of our users to instead use auto-inject. But it's possible I'm just attached to the old way of doing things.

[grampelberg]

Lemme see if I can pull this all apart a little bit. My proposal:

1. Install the proxy auto-injector by default.
2. Convert linkerd inject from modifying spec.template.spec.containers to modifying spec.template.metadata.annotations. The implication of this are
   • Any spec will show exactly what was applied and not the Linkerd containers.
   • Auto-inject will be modifying the pod itself on creation.
   • linkerd inject will convert command line flags to annotations and add them to the spec.
3. Document how to do a dry run that displays what mutations will actually occur to your pods in production.

I'm frankly a bit wary of dropping support for CLI injection entirely, and forcing all of our users to instead use auto-inject. But it's possible I'm just attached to the old way of doing things.

I'm with you on this one, in particular the ability to do a dry run/preview is key for me. I think this proposal will make it easier for users to reason about what's happening and where.

[ihcsim]

The dry run/preview (of the proxy spec) will be crucial to users who rely on tools like argocd which uses the static YAML as inputs to their deployment pipeline.

[grampelberg]

@ihcsim why do you say that? Adding the annotations should work great for them.

[klingerf]

@grampelberg Your summary matches my own understanding of this issue.

Related to @ihcsim's comment, how does dropping support for CLI inject relate to #2129? The plan in that issue was to allow users to pass injected YAML to kustomize before applying it to their clusters, in order to override pod / container configuration settings that aren't overridable via the linkerd inject command line flags. I don't think we'll be able to support that via annotations only.

[ihcsim]

I was thinking of deployment workflows where the injected YAML manifests are version controlled. Maybe auto-inject will cover most (if not all) of our user base?

[grampelberg]

Oh bugger, that's a good point re kustomize. @ihcsim how does the inject work? I know it is a patch, but what happens if someone has something there already? I'm wondering how k8s would handle partial spec definitions ...

[ihcsim]

Notes from meeting with @klingerf @grampelberg @alpeb:

We will change the CLI inject command's default behaviour to only annotate the workload's original YAML. A new flag, named --advanced, will be added to the inject command to output the full injected YAML, as a "break the glass" measure for more advance workflows.

Actions:

• Make proxy injector a mandatory component of the control plane. This includes removing the --proxy-auto-inject CLI flag.
• Add the new inject --advanced flag to the inject command. The proxy injector will skip the resultant YAML (i.e. auto-inject and config override aren't supported).
• Update CLI inject default behaviour to only annotate workload YAML, and let the proxy injector perform auto-injection.

[olix0r]

A new flag, named --advance, will be added to the inject command to output the full injected YAML, as a "break the glass" measure for more advance workflows.

For my own understanding, where do we expect to implement this?

My main concern is that the inject code won't be able to infer the target namespace, since this can be bound lazily (i.e. omitted from a pod spec and specified via kubectl -n myns apply). We have to be careful to ensure that none of our inject logic assumes it knows the target namespace at inject-time in order for this to work.

[klingerf]

@olix0r The thinking is that the --advanced flag should make linkerd inject work exactly as it does today. Basically, the code that exists now for performing CLI inject will continue to be run when the --advanced flag is present.

[siggy]

Note: this issue is mostly wrapped up via #2710 and #2711 (via PR #2721)

[alpeb]

Fixed via #2721 and #2733