feat(k8s): always inject Kuma as the first container

[curtiscook]

Checklist prior to review

• Link to docs PR or issue -- Make Kuma's init container first by default #3121
• Link to UI issue or PR -- n/a
• Is the issue worked on linked? --
• The PR does not hardcode values that might break projects that depend on kuma (e.g. "kumahq" as a image registry) --
• The PR will work for both Linux and Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
• Unit Tests --
• E2E Tests --
• Manual Universal Tests --
• Manual Kubernetes Tests --
• Do you need to update UPGRADE.md? -- no
• Does it need to be backported according to the backporting policy? -- no
• Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

[johnharris85]

Thanks for the PR! This definitely needs to be flagged / configurable and documented as it's a breaking change. We should also document that this only represents a 'best-effort' to put our sidecar first, as another mutating controller could come along and be mischievous :)

[curtiscook]

Thanks for the PR! This definitely needs to be flagged / configurable and documented as it's a breaking change. We should also document that this only represents a 'best-effort' to put our sidecar first, as another mutating controller could come along and be mischievous :)

Agree. This pr still needs

1. configurability
2. testing
3. documentation

Also, the pr itself isn't very useful without a postStart hook on the sidecar, so would probably want to tie that in if possible. Will try to follow up with the cleanup when I get a chance. (Unless someone with more free time wants to take it across the finish line)

[lahabana]

@slonka @johnharris85 Is it really a breaking change? I don't think it is without a postStart as the order of containers doesn't really influence anything (they are all started at once except if there's a postStart).

[slonka]

I thought that we're only considering this feature it with postStart otherwise it's not very useful (I thought that's what we agreed in some slack thread)

[curtiscook]

It only really makes sense with postStart but we could always inject first and then set the config to add a postStart executable

[lahabana]

Right so without postStart it's not especially useful to have it first. So isn't it likely to just have it always first and then make postStart optional/opt-in . So for this PR maybe we should not make this configurable (it should just be first)

[curtiscook]

Right so without postStart it's not especially useful to have it first. So isn't it likely to just have it always first and then make postStart optional/opt-in . So for this PR maybe we should not make this configurable (it should just be first)

This seems sensible and atomic. I might keep the function for testability but drop the boolean in that case.

Originally I was going to incorporate both changes into this pr, but still not familiar with how configurations work in kuma so it's probably better to separate them.

As a side note, I think that allowing the main pod to start without effectively having a network is a critical issue, despite workarounds. This is mostly Kubernetes's fault, but I feel that it's worth handling. Currently the best workaround that I have found is to crash-loop the main pod until you can connect to an outside server i.e. some persistence layer. If you don't do this, Kubernetes seems to assume your node is in a ready state which results in rollouts causing unwanted downtime. Unfortunately the next best workaround is to disable the sidecar proxy, which is even less ideal if you're trying to implement traffic rules with mTLS

[lahabana]

Ok so sounds like you should be able to update your PR to always inject it first (and sign your commit). Then we can help you work on adding the postStart option.

[lahabana]

/format /golden_files

[lahabana]

/format

[lahabana]

  [FAILED] in [It] - /home/runner/work/kuma/kuma/pkg/plugins/runtime/k8s/webhooks/injector/injector_test.go:121 @ 12/09/22 12:29:03.163
  << Timeline

  [FAILED] Expected
      <string>: busybox
  to be equivalent to
      <string>: kuma-sidecar
  In [It] at: /home/runner/work/kuma/kuma/pkg/plugins/runtime/k8s/webhooks/injector/injector_test.go:121 @ 12/09/22 12:29:03.16

Looks like this actually fails

[lahabana]

Your using go 19 that's why all these go embed got updated

[lahabana]

/golden_files

[curtiscook]

It looks like there are test cases under "should inject" that shouldn't inject

  [FAIL] Injector should inject Kuma into a Pod [It] 10. Pod with `kuma.io/sidecar-injection: disabled` annotation

[curtiscook]

I think this last test failure might be a flake, but not familiar enough with your test suite

[lahabana]

Thx for the contrib!

[lahabana]

@curtiscook I added a message in UPGRADE.md because there's a small upgrade path required. Merging as soon as CI passes