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

Update the proxy injector MWC to include reinvocationPolicy #3750

Open
ihcsim opened this issue Nov 21, 2019 · 6 comments
Open

Update the proxy injector MWC to include reinvocationPolicy #3750

ihcsim opened this issue Nov 21, 2019 · 6 comments

Comments

@ihcsim
Copy link
Contributor

ihcsim commented Nov 21, 2019

K8s 1.15 introduces the reinvocationPolicy in the MWC and VWC allowing the webhook to be re-evaluated during a single admission request. This will ensure defaults defined by certain admission controllers like PodSecurityPolicy and LimitRanger are automatically added to the Linkerd proxy and proxy-init containers.

We should update the MWC template to include this property with the value IfNeeded, so that K8s 1.15+ users can take advantage of it.

@ihcsim
Copy link
Contributor Author

ihcsim commented Nov 21, 2019

To ensure backward compatibility, this property should only be added if users are on K8s 1.15+. The logic that copies the primary container's security context to the sidecar container in pkg/inject/inject.go should be retained, to ensure support of PSP continues to work.

@alpeb
Copy link
Member

alpeb commented Nov 22, 2019

@ihcsim LimitRanger and PSP will participate during the reinvocation by default (after k8s 1.15) and will set the defaults on the sidecar, no config changes needed. We want to leave the injector's reinvocationPolicy to its default Never, as that property only affects the injector itself, not the other admission controllers.

@grampelberg
Copy link
Contributor

@alpeb @ihcsim we should close this issue out right?

@ihcsim
Copy link
Contributor Author

ihcsim commented Nov 22, 2019

@alpeb I see what you mean - we will need to set this policy for our MWC if the proxy injector needs to be reinvoked. Closing this issue.

@ihcsim ihcsim closed this as completed Nov 22, 2019
@jroper
Copy link
Contributor

jroper commented Jun 4, 2021

Not sure why this was closed. It would be good if linkerd did support IfNeeded - in Cloudstate, we inject a sidecar that needs to have some ports excluded from the service mesh using the config.linkerd.io/skip-outbound-ports annotation (this sidecar runs Akka, which forms a cluster between other pods of the same deployment, the cluster communication must bypass the service mesh because it by design cares about pod location to implement its sharding, replication and p2p communication patterns, but the service mesh thwarts this). So, if linkerd's webhook runs first, then the Cloudstate webhook runs, these annotations won't be picked up.

@alpeb
Copy link
Member

alpeb commented Jun 4, 2021

@jroper that makes sense. Although currently when the proxy injector runs and detects the pod has already been injected, it'll bail out, so this will require extra modifications besides making reinvocationPolicy configurable. On the other hand, if I'm not mistaking, last time I checked I found custom webhooks were triggered alphabetically so maybe Cloudstate's would run first? (this isn't documented behavior though). Anyways supporting IfNeeded would be more robust; would your team be willing to tackle that change? I'd be glad to provide guidance.

@olix0r olix0r reopened this Jun 4, 2021
alpeb added a commit that referenced this issue Jun 18, 2021
Preliminary for #3750 and #6267

This uses a generic [kubemod](https://github.com/kubemod/kubemod), a
generic mutating webhook, in a new integration test to prove that the
proxy-injector is ignoring changes made by webhooks run after it.

Once we implement reinvocation for the injector, this test should also
be changed to reflect that.
alpeb added a commit that referenced this issue Jun 19, 2021
* Integration test for proxy-injector reinvocation

Preliminary for #3750 and #6267

This uses a generic [kubemod](https://github.com/kubemod/kubemod), a
generic mutating webhook, in a new integration test to prove that the
proxy-injector is ignoring changes made by webhooks run after it.

Once we implement reinvocation for the injector, this test should also
be changed to reflect that.
alpeb added a commit that referenced this issue Jun 22, 2021
Fixes #3750 and partially #6267

As of k8s 1.15 mutating webhooks can be reinvoked whenever another mutating webhook running after the current one mutates the pod being persisted.
Enabling reinvocation for the injector will allow configuring the proxy with annotations generated by such other mutating webhooks.

The implementation consists on adding "remove" statements into the json patch returned by the injector, implemented mostly in the new file `pkg/inject/pod_patch.go` which now holds the `podPatch`, moved from `pkg/inject/inject.go`.

This also updates the "reinvocation" integration test introduced in #6309 to properly verify the reinvocation is happening.

And this also means the "existing proxy sidecar" check is no longer relevant, and has been limited to "existing 3rd party sidecar".

Possible followup: refactor the CLI inject and uninject commands to properly leverage the cleanup done by this patch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants