Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend Proxy Injector To Respond To Update Events #2260

Closed
ihcsim opened this issue Feb 12, 2019 · 7 comments

Comments

@ihcsim
Copy link
Member

commented Feb 12, 2019

Feature Request

What problem are you trying to solve?

At present, the proxy injector only auto-injects the proxy sidecar into newly created deployment resources. The webhook only responds to create events.

How should the problem be solved?

Update the proxy injector webhook configuration.

What do you want to happen? Add any considered drawbacks.

The webhook should be extended to pick up update events too, so that users can utilize the proxy injector to mesh existing resources, without the needs to re-create their resources.

We need to make sure that during a resource update, all user's custom proxy configuration is preserved. Meanwhile, I am not 100% sure how/if we should let the proxy injector performs auto proxy version upgrade.

Any alternatives you've considered?

Is there another way to solve this problem that isn't as good a solution?

How would users interact with this feature?

If you can, explain how users will be able to use this. Maybe some sample CLI
output?

@ihcsim ihcsim changed the title Extend Proxy Injector To Respond To With Update Events Extend Proxy Injector To Respond To Update Events Feb 12, 2019

@ihcsim

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

Following @siggy brief comment, it looks like the proxy injector is already skipping the sidecar injection process, if a proxy is already present. To support the upgrade story, shall we consider updating the healthcheck.HasExistingSidecars() method to check the proxy version, and then perform an auto upgrade if necessary (while preserving any custom configs)? Personally, as a user, I prefer to keep the proxy version upgrade manual, or at least not couple with my kubectl apply or helm update.

@grampelberg grampelberg added this to To do in 2.3 via automation Feb 13, 2019

@grampelberg grampelberg added this to To do in Inject/Install Configuration via automation Feb 14, 2019

@grampelberg grampelberg removed this from To do in 2.3 Feb 14, 2019

@ihcsim ihcsim self-assigned this Feb 15, 2019

@ihcsim ihcsim moved this from To do to In progress in Inject/Install Configuration Feb 19, 2019

@ihcsim

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

@grampelberg @kl I want to get your thoughts on the kind of scenarios we'd like to cover during an update event. After some testing, these happy paths work:

  • if a deployment isn't meshed, but it's annotated with linkerd.io/inject: enabled, then the proxy will be auto-injected
  • if a deployment is already meshed, then no-op on the webhook. All custom proxy configs are preserved.

What about:

  • auto un-inject on a meshed deployment if it either has no annotation at both the namespace and pod level, or it's annotated with linkerd.io/inject: disabled. I'll have to test to see if this is mutable by the webhook.
  • auto version upgrade of proxies if the meshed proxy is at a lower version? I feel like this is going to be part of the larger upgrade story.

This will likely be dependent on the work that @alpeb is doing in #2289.

@grampelberg

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

@ihcsim I've not had adding the annotation work post-create.

  • auto un-inject on a meshed deployment

+1 from me.

  • auto version upgrade of proxies

This feels like the right thing to do, but we should make it explicit instead of implicit. I'd hate for a user to accidentally get a new proxy version they weren't expecting. Perhaps it is another annotation? linkerd.io/upgrade: true ?

@ihcsim

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

I've not had adding the annotation work post-create.

Did you try with kubectl annotate --overwrite? Also fwiw, I'm using kubectl 1.13.0. (I'm gonna do some more testing in a bit.)

Following this AM discussion on auto un-inject. we probably not going to do it until #2289 has landed. There are also some lingering questions around the removal of the namespace-level annotation. Do we interpret that as the user's way of wanting to un-mesh all workloads in that namespace on the next update?

... we should make it explicit instead of implicit. Perhaps it is another annotation?

Absolutely. I assume the annotation will need to be removed once the upgrade is completed.

@grampelberg

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Did you try with kubectl annotate --overwrite?

kubectl apply

@ihcsim

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

@grampelberg and I tested this on his GKE cluster. It works after we updated the mutating webhook configuration to pick update events.

Also, we will limit the scope of this issue to the more common scenario of auto-injecting proxy into un-meshed workload, without the needs to re-deploy. Auto un-inject and proxy version upgrade will be addressed in separate issues.

@klingerf

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

@ihcsim That all makes sense to me. I like your suggestion of only updating workloads that weren't previously injected in the first pass, and worrying about re-inject / upgrade later. And I agree that the reason why adding the annotation doesn't work today is that we're not watching update events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.