-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
⚠️ Remove pkg/inject #2134
⚠️ Remove pkg/inject #2134
Conversation
/assign @alvaroaleman @sbueringer |
We are using a standalone webhook server at the moment in CAPI Runtime Extensions: https://github.com/kubernetes-sigs/cluster-api/blob/b6e824c92c4c0d85050b9dc09ba99ca23b238f7a/test/extension/main.go#L141 But we probably should switch to using a manager anyway as it gives us a few more useful features. EDIT: Hm looks like we would probably (?) not actually be affected. We are not using the handlers with |
2493b39
to
6a9b719
Compare
// Note that this assumes and requires a valid TLS | ||
// cert and key at the default locations | ||
// tls.crt and tls.key. | ||
func ExampleServer_StartStandalone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StandaloneServer here makes it more difficult to support the Webhook code long term, looking at grep.app
this code is not used on publicly available code at least. The Standalone seems it was added to use webhooks outside of a Manager, but really lots of things from the manager are then required (like scheme, or client).
Going forward, we should try to make the Manager as slim as possible, and require it explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grep.app only indexes the N most popluar repos, so it is somewhat useless when finding usages in potentially non-popular repos. Trough cs.github.com I can find for example these usages:
- https://cs.github.com/wking/ci-tools/blob/64bde4a5f06d600725ddcc59f8d88410ec4fbe35/cmd/pod-scaler/admission.go?q=StartStandalone+language%3AGo#L39
- https://cs.github.com/openshift/cluster-api/blob/1101b984bc6d4e0c81dcb6d6e4651dd6dfbdbec8/exp/runtime/server/server.go?q=StartStandalone+language%3AGo#L239
- https://cs.github.com/varshavaradarajan/webhook-cr-poc/blob/7da2130be1d295be447196b77d5f4fbebe3af652/cmd/webhook-cr-poc/main.go?q=StartStandalone+language%3AGo#L40
So it does seem to be used.
but really lots of things from the manager are then required (like scheme, or client).
Both of these can be constructed just fine without a manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You commented that you reverted this but its still being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The StartStandalone
part isn't necessary now, the scheme is neither used or injected, webhook.Start
should work here
9c26a1d
to
0d72871
Compare
// Note that this assumes and requires a valid TLS | ||
// cert and key at the default locations | ||
// tls.crt and tls.key. | ||
func ExampleServer_StartStandalone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You commented that you reverted this but its still being removed?
Signed-off-by: Vince Prignano <[email protected]>
0d72871
to
927c09e
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
It seems like this will be a breaking change for implementers of Reconciler without a deprecation window. The documentation previously stated the injection package was deprecated, not the injection mechanism. |
@terinjokes These packages have been deprecated since 0.10 and should have been removed but never did. The release notes will reflect these changes. |
Just providing feedback that deprecation notices in the future should be very clear about what they apply to. The packages are deprecated with a notice, sure, but that could have been intepreted as "don't import this package for these interfaces, just implement them" rather then "the entire documented injection system is going away". |
https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 * Make `*http.Client` configurable and use/share the same client by default kubernetes-sigs/controller-runtime#2122 * Remove dependency injection functions kubernetes-sigs/controller-runtime#2134, kubernetes-sigs/controller-runtime#2120 * Add context to EventHandler(s) kubernetes-sigs/controller-runtime#2139 * `Validator` and `CustomValidator` interfaces have been modified to allow returning warnings kubernetes-sigs/controller-runtime#2014 * operator-framework is also pinned to ecb9be48837 until a new release is cut supporting controller-runtime v0.15.0 Signed-off-by: Lee Yarwood <[email protected]>
https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 * Make `*http.Client` configurable and use/share the same client by default kubernetes-sigs/controller-runtime#2122 * Remove dependency injection functions kubernetes-sigs/controller-runtime#2134, kubernetes-sigs/controller-runtime#2120 * Add context to EventHandler(s) kubernetes-sigs/controller-runtime#2139 * `Validator` and `CustomValidator` interfaces have been modified to allow returning warnings kubernetes-sigs/controller-runtime#2014 * operator-framework is also pinned to ecb9be48837 until a new release is cut supporting controller-runtime v0.15.0 Signed-off-by: Lee Yarwood <[email protected]>
https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0 * Make `*http.Client` configurable and use/share the same client by default kubernetes-sigs/controller-runtime#2122 * Remove dependency injection functions kubernetes-sigs/controller-runtime#2134, kubernetes-sigs/controller-runtime#2120 * Add context to EventHandler(s) kubernetes-sigs/controller-runtime#2139 * `Validator` and `CustomValidator` interfaces have been modified to allow returning warnings kubernetes-sigs/controller-runtime#2014 * operator-framework is also pinned to ecb9be48837 until a new release is cut supporting controller-runtime v0.15.0 Signed-off-by: Lee Yarwood <[email protected]>
Closes #167. In #151 we updated some libraries and we missed the fact that kubernetes-sigs/controller-runtime#2134 removed the `InjectDecoder` call entirely. This PR implements a live test of the `CONNECT` functionality, as well as fixes for the decoder.
The InjectDecoder method is now deprecated and the dependency injection is not performed anymore to inject the decoder. This commit setup the decoder during the first call to the Handle method. See kubernetes-sigs/controller-runtime#2134
This changeset brings in a few more breaking changes to remove the rest of the dependency injection code.
The webhook code is now a bit simplified and more explicit in accepting a *runtime.Scheme. The logger is now also not configured in the Standalone mode. We should revisit in a future PR the webhook standalone configuration, and potentially always require a manager. From a quick look in GitHub projects, it seems most folks don't actually use these standalone webhooks, which makes sense because the Manager makes things easier, although there might be private code that's using them.
The config loader was using InjectScheme before, although it really didn't need to.
Signed-off-by: Vince Prignano [email protected]