tumblr / k8s-sidecar-injector

Kubernetes sidecar injection service
Apache License 2.0
345 stars 75 forks source link

Add prependedContainers #51

Open ribbybibby opened 4 years ago

ribbybibby commented 4 years ago

What and why?

Adds a field which allows sidecars to be prepended to the top of the list of the containers. This allows use of this workaround for delaying an application until the sidecars are ready: https://medium.com/@marko.luksa/delaying-application-start-until-sidecar-is-ready-2ec2d21a7b74

Issue: #49

Testing Steps

Please provide adequate testing steps (including screenshots if necessary). Include any test fixtures or sample configurations in your commit.

Reviewers

Required reviewers: @byxorna Request reviews from other people you want to review this PR in the "Reviewers" section on the right.

:warning: this PR must have at least 2 thumbs from the MAINTAINERS.md of the project before merging!

ribbybibby commented 4 years ago

@byxorna @kirooshu @alex-laties

komapa commented 4 years ago

@ribbybibby, I agree that workaround is good until there is a proper solution. What I would recommend as a patch here though is to have a flag for prepend all or append all (current behavior). I think having the prependContainers is confusing and most of the time you do not really care if there are some other containers between your "sidecar" container and the main container, they can start just as normal if they do not have the postStart logic.

So I would vote here for just having a global flag on the injection config for whether to prepend the containers or append (default) and that should solve your use case, right?

ribbybibby commented 4 years ago

Thanks for looking at the PR @kirooshu. Yep, that works for me. I'll make the changes.

ribbybibby commented 4 years ago

@kirooshu I've pushed the suggested changes.

ribbybibby commented 4 years ago

:bow: Thanks for the approve @kirooshu. Is this good to be merged?

ribbybibby commented 4 years ago

Ping - just checking if anyone has had a chance to look at this? @alex-laties @defect