Linkerd2: autoinject: Inject Pods rather than Deployments

[olix0r]

Currently, the autoinjector service watches deployments and adds proxies to deployments. However, deployments are only created/updated when a user invokes kubectl apply... As we introduce injector config that might be dynamic (i.e. a user may change it at runtime), this means that existing deployments would have no way of using this configuration as the deployments pods are (re-)created.

What if, instead of getting notifications about deployment updates, we watch for _Pod_ creation? I think this will enable the injector to apply configuration on pod creation (i.e. if i delete a pod of an existing deployment, a new pod will be created and get injected anew with config).

cc @grampelberg @ihcsim

[klingerf]

This seems like a good idea, and could be a fix for #1751. It would be worth verifying this assumption:

if i delete a pod of an existing deployment, a new pod will be created and get injected anew with config

We can setup a webhook that watches pods and logs when the webhook is called, to figure out exactly when that happens in a pod lifecycle. I suspect it might only be called once per apply, and subsequent incarnations of the pod will just use the previously applied and modified yaml, instead of re-calling the webhook.

I also wanted to note that some of our inject code relies on the pod's parent object. For example, when I inject a deployment called "hello", the inject code adds the linkerd.io/proxy-deployment: hello label to the pod. This is required for generating prometheus labels. If we're only watching pods, we won't receive the pod's parent object as part of the admission request, and we'll need to come up with a different way to derive the owner from the standalone pod object. We already have code to do this by way of pod owner references, but we should validate that that successfully returns the pod owner for all of our inject use cases:

https://github.com/linkerd/linkerd2/blob/43d29d629e2416c827aff0883812404bf8e99da5/controller/k8s/api.go#L274-L293

[olix0r]

I suspect it might only be called once per apply, and subsequent incarnations of the pod will just use the previously applied and modified yaml, instead of re-calling the webhook.

I'm pretty confident that this _has to_ work: a pod doesn't actually exist until it's instantiated. I.e. if i delete one pod in a deployment, a new pod is _created_ (with a new name), so there can't be a pod resource in the kubernetes API until then... But I certainly agree it's important to prototype this and to figure out what other metadata we have access to in the pod spec we receive from the webhook.

[olix0r]

Another thought: we may need to watch BOTH parent resource types (like Deployment) AND pods: the parent resource to add our own annotations, and the pod to actually do the injection. That is, if the webhook call we get for a single pod lacks that metadata.

[ihcsim]

I suspect it might only be called once per apply, and subsequent incarnations of the pod will just use the previously applied and modified yaml, instead of re-calling the webhook.

If we watch the pods only, I believe the webhook will only receive and mutate the pod YAML in the admission request from the k8s API server, not the parent's spec.template.spec.containers. Hence, auto-injection will always be triggered because the pod YAML is based on the parent's original, unmeshed spec.template.spec.containers.

Another thought: we may need to watch BOTH parent resource types AND pods

Then I am not sure if we are gaining anything by watching the pods.

[grampelberg]

If we watch the pods only, I believe the webhook will only receive and mutate the pod YAML in the admission request from the k8s API server, not the parent's spec.template.spec.containers. Hence, auto-injection will always be triggered because the pod YAML is based on the parent's original, unmeshed spec.template.spec.containers.

This feels like an improvement to me. When someone looks at the YAML in k8s it won't differ nearly as much for something like a Deployment. We'll also natively end up supporting more than just deployments.

Looking at some other solutions, it appears that watching for CREATE on pods is what other folks are doing (and not anything else). See https://github.com/istio/istio/blob/master/install/kubernetes/helm/istio/charts/sidecarInjectorWebhook/templates/mutatingwebhook.yaml.

[olix0r]

Then I am not sure if we are gaining anything by watching the pods.

Consider the case where the control plane is upgraded, and so a new proxy version will be injected. If we only watch, e.g., deployments, then we require that the user update the _deployment object_ to get a new proxy version. By watching pods, upgrading to a new proxy is as simple as "delete the existing pods" and, as the replica set scales the deployment back up, the new pods get the new configuration.

My suggestion for watching both deployments and pods is in response to this comment from @klingerf:

I also wanted to note that some of our inject code relies on the pod's parent object. For example, when I inject a deployment called "hello", the inject code adds the linkerd.io/proxy-deployment: hello label to the pod. This is required for generating prometheus labels.

If we don't know the parent object at pod-inject time and we need to be able to set a label for prometheus, then we may _also_ need to watch/modify the parent objects to set this metadata on the pod spec...

[olix0r]

OK, and it does appear the pods should have enough information for us to discover ownership. From v1.ObjectMeta:

    // List of objects depended by this object. If ALL objects in the list have
    // been deleted, this object will be garbage collected. If this object is managed by a controller,
    // then an entry in this list will point to this controller, with the controller field set to true.
    // There cannot be more than one managing controller.
    // +optional
    // +patchMergeKey=uid
    // +patchStrategy=merge
    OwnerReferences []OwnerReference `json:"ownerReferences,omitempty" patchStrategy:"merge" patchMergeKey:"uid" protobuf:"bytes,13,rep,name=ownerReferences"`

[alpeb]

Please let's wait for #2334 to land before tackling this :pray:

[olix0r]

@alpeb absolutely. i've just been noting things i've learned as i bump against the API... I've no plans to work on this ;)