Inject sidecar via Mutating Webhook Admission Controller

[jpkrohling]

As hinted by @gregoryfranklin on #60, we should aim to inject the sidecars via Mutating Webhook Admission Controller. I believe I tried this during the PoC and couldn't get it to work, but it would be good to try again and, if not possible, document the reasons why we have it the way it is right now.

[gregoryfranklin]

If it helps, various examples can be found here: https://github.com/kubernetes/kubernetes/tree/master/test/images/webhook

The biggest issue I've personally run into when working with Webhooks is managing the SSL certs. The webhook must listen on port 443 and the apiserver must trust the cert (MutatingWebhookConfiguration allows you to provide a ca cert). Both cert-manager and istio deal with this problem by generating their own ca chain (ie self-signed certs) and having a job that patches the MutatingWebhookConfiguration (see https://github.com/munnerz/apiextensions-ca-helper).

[jpkrohling]

The operator-sdk is finally adding support for admission webhooks, which should allow us to finally work on this in an appropriate manner.

operator-framework/operator-sdk#2729

[JessieAMorris]

Any word on this? We're getting weird interactions with the jaeger sidecar injector and our LimitRange for default resources. Overall the current injector doesn't seem to work very well and we're running into issues with it.

[jpkrohling]

I'll see if we can improve the current injector, to make it more reliable. It would help us immensely if you could describe concretely what you are facing.

The sidecar injection via mutating webhooks has been implemented in the opentelemetry-operator, which was mainly done to test waters and see if we can apply the same technique here. The good news is that it got merged today (open-telemetry/opentelemetry-operator#52), the bad news is that the changes required to get it working were quite big: basically, the operator-sdk 0.19.x is a major rewrite from 0.18.x that we are using, and requires changing pretty much the whole operator's foundations. I'm not sure that's feasible to accomplish in the short-term.

[jpkrohling]

@JessieAMorris are you using the operator v1.20? There were some changes made by @rubenvp8510 that should have made it more stable.

[vassilvk]

Hi @JessieAMorris, @jpkrohling,

I was having similar issues where the destructive sidecar injection performed by the operator made the code running in my pods misbehave, due to race conditions caused by the pods being terminated and recreated shortly after they were created.

I switched from using Jaeger Operator's injection, to a non-destructive webhook-based Jaeger agent injection performed by the following KubeMod ModRule which works alongside a Jaeger Operator.

This rule replaces the automatic Jaeger Operator injection by monitoring for deployments tagged with annotation jaeger-sidecar-inject. When the ModRule is deployed, this annotation works as a drop-in replacement for Jaeger Operator's annotation sidecar.jaegertracing.io/inject pointed to a Jaeger instance.

In my case this ModRule is a short-term solution until Jaeger Operator introduces non-destructive webhook-based injection.
Note that the rule injects a Jaeger agent version 1.18.1, but that could be changed to whatever version your Jaeger Operator deploys.

apiVersion: api.kubemod.io/v1beta1
kind: ModRule
metadata:
  name: jaeger-sidecar-inject
spec:
  type: Patch

  match:
    # Intercept deployments ...
    - select: '$.kind'
      matchValue: Deployment
    # ... tagged with annotation jaeger-sidecar-inject ...
    - select: '$.metadata.annotations["jaeger-sidecar-inject"]'
      matchRegex: '.+'
    # ... but ensure that the jaeger-agent container isn't already injected to avoid adding more containers on UPDATE operations.
    - select: '$.spec.template.spec.containers[*].name'
      matchValue: 'jaeger-agent'
      negate: true

  patch:
    # Add a container to the deployment's existing container list...
    - op: add
      # ... at index -1 which indicates that we want to append the new container to the existing list.
      path: /spec/template/spec/containers/-1

      # Use KubeMod's template evaluation to extract the name of the Jaeger instance from annotation jaeger-sidecar-inject.
      value: |-
        name: jaeger-agent
        args:
        - --jaeger.tags=deployment.name={{ .Target.metadata.name }},pod.namespace={{ .Namespace }},pod.id=${POD_ID:},host.ip=${HOST_IP:}
        - --reporter.grpc.host-port=dns:///{{ index .Target.metadata.annotations "jaeger-sidecar-inject" }}-collector-headless.{{ .Namespace }}:14250
        - --reporter.type=grpc
        env:
          - name: POD_ID
            valueFrom:
              fieldRef:
                apiVersion: v1
                fieldPath: metadata.uid
          - name: HOST_IP
            valueFrom:
              fieldRef:
                apiVersion: v1
                fieldPath: status.hostIP
        image: jaegertracing/jaeger-agent:1.18.1
        imagePullPolicy: IfNotPresent
        ports:
        - containerPort: 5775
          name: zk-compact-trft
          protocol: UDP
        - containerPort: 5778
          name: config-rest
          protocol: TCP
        - containerPort: 6831
          name: jg-compact-trft
          protocol: UDP
        - containerPort: 6832
          name: jg-binary-trft
          protocol: UDP
        - containerPort: 14271
          name: admin-http
          protocol: TCP

[jpkrohling]

That looks really cool. I don't think we can add it to the operator itself (yet?), but it's good to know there's a workaround for people who are experiencing problems with the current auto-injection.

[esnible]

The current injection behavior is scary because the operator restarts pods when the annotation changes.

I like it much better if the pods keep running until they are deleted/recreated. That's the way it works in Istio: change the annotation, then kubectl rollout restart to restart the pods.

This change is related to 6edd202

In Istio, we wanted to be able to run >1 control plane, and assign each namespace to a particular plane. I don't know if that configuration is needed in Jaeger. If so, the sidecar.inject.jaeger/inject can't be just true, but name which Jaeger Operator is in change of a particular namespace.

[frzifus]

I agree with @esnible. Could you create another issue to discuss the behavior?

Iam closing this issue since, the sidecar injection moved to a webhook admission controller in #1828.