Make Kuma's init container first by default

[johnharris85]

Summary

Currently we append the Kuma init container to an existing list of init containers:

kuma/pkg/plugins/runtime/k8s/webhooks/injector/injector.go

Line 116 in 2d08e3f

pod.Spec.InitContainers = append(pod.Spec.InitContainers, ic)

We should insert ourselves first by default to ensure that nothing 'slips through' before we can redirect traffic.

• This is a change in default functionality, so should probably document it somewhere and call it out in the changelog (in case folks are somehow relying on us not being first).
• Should we make this configurable? (prior art Hashicorp Vault: https://www.vaultproject.io/docs/platform/k8s/injector/annotations#vault-hashicorp-com-agent-init-first)

Worth calling out that if anything starts after our init but before kuma-dp it's going to have no network access (unless the ports are explicitly ignored - traffic.kuma.io/exclude-outbound-ports)

Happy to make this change once we decide on the details above ^^.

[johnharris85]

Was thinking some more about this. We don't specify a reinvocationpolicy (https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#reinvocation-policy) and the default is never, so even if we put something first, another admission controller could absolutely re-order that without us knowing. If we were to set reinvocation to ifneeded then we would need additional logic to determine if we were already injected to be idempotent, I still don't think we're guaranteed to see the final state though unless we also implemented a validating hook to 'ensure' we were first. This probably needs a little more thought / consensus on how we want to move foward.

[github-actions]

This issue was inactive for 30 days it will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant please comment on it promptly or attend the next triage meeting.

[lahabana]

Triage: Yes it makes sense

[github-actions]

This issue was inactive for 30 days it will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant please comment on it promptly or attend the next triage meeting.

[github-actions]

This issue was inactive for 30 days it will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant please comment on it promptly or attend the next triage meeting.

[jakubdyszkiewicz]

Triage: let's introduce config in Kuma CP to indicate if we want to inject by default first or last. Then, let's add an annotation to override this behavior.

[slonka]

This issue is needed to be able to implement this: #2483 - any updates on development here?

[michaelgoodrx]

Hi, I got pointed to this issue from https://kuma-mesh.slack.com/archives/C013ALVE1GU/p1663005053300479.

We have init containers that need the internet to run. Specifically, they need the internet in order to decrypt secrets and generate a config file. So we actually need the kuma init container to go last, because as soon as it runs, there is no internet access until the sidecar starts.

It sounds like there are use cases where the init container should run first and use cases where the container should run last. It probably doesn't make sense to run it "in the middle".

Also, I noticed the thing about the admission controller too, and I have been a little nervous that one day the order of init containers would change and our apps would no longer start. So would definitely be in favor of setting up reinvocation to make sure it is in the "correct" order. RE #3121 (comment)

[lahabana]

I was just thinking this could be configurable through container-patches.

Something like:

apiVersion: kuma.io/v1alpha1
kind: ContainerPatch
metadata:
  name: container-patch-1
  namespace: kuma-system
spec:
   initOrder: first|last|<a specific integer>

or after/before container name

[github-actions]

This issue was inactive for 90 days. It will be reviewed in the next triage meeting and might be closed.
If you think this issue is still relevant, please comment on it or attend the next triage meeting.

[lahabana]

This was done by #5436

[michaelgoodrx]

@lahabana it looks to me like #5436 only changed the order of the sidecar, not the init container.

[curtiscook]

@lahabana it looks to me like #5436 only changed the order of the sidecar, not the init container.

IIRC the initcontainer goes first anyway. #5436 only changed the order, but we need to wait for the sidecar to be ready before the application start. I don't think you need #5857 to get the desired behavior

#5498 will implement the wait functionality with a timeout (once it's complete)

[michaelgoodrx]

IIRC the initcontainer goes first anyway

In the version we are on, it runs last, which is important to us because we have other init containers that need internet access so they have to run before it.

But in my comment, I was pointing out that it looks like you closed this ticket, for changing the init container order, when you actually changed the sidecar order.

[curtiscook]

IIRC the initcontainer goes first anyway

In the version we are on, it runs last, which is important to us because we have other init containers that need internet access so they have to run before it.

But in my comment, I was pointing out that it looks like you closed this ticket, for changing the init container order, when you actually changed the sidecar order.

I didn't close the issue :) but I think there's confusion over this issue. The problem I've been working on addressing is that a container doesn't have DNS access after Init, before sidecar ready. Possible that my issue is separate from this one and we misattributed, but I think my understanding was that this issue was the kuma equivalent of istio/istio#11130

That being said it looks like zekth change to run kuma init as the first init container has been merged so it seems moot?

[michaelgoodrx]

Sorry, I meant "you" as in Kong, I was assuming you worked for them.

Can you link me to the change that affects the init container? If that was merged then I need to open a ticket, because that breaks the mesh for us.

[curtiscook]

Sorry, I meant "you" as in Kong, I was assuming you worked for them.

Can you link me to the change that affects the init container? If that was merged then I need to open a ticket, because that breaks the mesh for us.

Ah, no I'm a community edition user with a problem that I want to fix :)

This should be the initcontainer pr
#5857

[michaelgoodrx]

Thank you!