⚠️ Remove pkg/inject #2134

vincepri · 2023-01-18T17:25:04Z

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 vincepri@redhat.com

vincepri · 2023-01-18T17:25:21Z

/assign @alvaroaleman @sbueringer

sbueringer · 2023-01-19T16:30:13Z

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.

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.
(cc @fabriziopandini @killianmuldoon)

EDIT: Hm looks like we would probably (?) not actually be affected. We are not using the handlers with StandaloneWebhook, just the webhook.Server standalone without a manager.

pkg/webhook/admission/defaulter_custom.go

pkg/webhook/conversion/decoder.go

pkg/webhook/server.go

examples/builtins/mutatingwebhook.go

examples/tokenreview/tokenreview.go

pkg/builder/example_test.go

pkg/webhook/conversion/decoder.go

alvaroaleman · 2023-01-20T19:17:52Z

pkg/webhook/example_test.go

-// Note that this assumes and requires a valid TLS
-// cert and key at the default locations
-// tls.crt and tls.key.
-func ExampleServer_StartStandalone() {


What is the reason for removing this?

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

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.

reverted, thanks!

You commented that you reverted this but its still being removed?

The StartStandalone part isn't necessary now, the scheme is neither used or injected, webhook.Start should work here

examples/builtins/main.go

alvaroaleman · 2023-01-20T20:56:04Z

pkg/webhook/example_test.go

-// Note that this assumes and requires a valid TLS
-// cert and key at the default locations
-// tls.crt and tls.key.
-func ExampleServer_StartStandalone() {


You commented that you reverted this but its still being removed?

Signed-off-by: Vince Prignano <vincepri@redhat.com>

k8s-ci-robot · 2023-01-20T22:52:12Z

[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:

~~OWNERS~~ [alvaroaleman,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

terinjokes · 2023-03-30T12:09:47Z

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.

vincepri · 2023-04-10T14:19:53Z

@terinjokes These packages have been deprecated since 0.10 and should have been removed but never did. The release notes will reflect these changes.

terinjokes · 2023-04-10T14:58:00Z

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 <lyarwood@redhat.com>

kubernetes-sigs/controller-runtime#2134

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 <lyarwood@redhat.com>

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 18, 2023

k8s-ci-robot requested review from joelanford and sbueringer January 18, 2023 17:25

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 18, 2023

k8s-ci-robot assigned alvaroaleman and sbueringer Jan 18, 2023

sbueringer reviewed Jan 19, 2023

View reviewed changes

pkg/webhook/admission/defaulter_custom.go Show resolved Hide resolved

pkg/webhook/conversion/decoder.go Outdated Show resolved Hide resolved

sbueringer reviewed Jan 19, 2023

View reviewed changes

pkg/webhook/server.go Show resolved Hide resolved

vincepri force-pushed the remove-webhook-config-depinject branch 9 times, most recently from 2493b39 to 6a9b719 Compare January 20, 2023 17:38

alvaroaleman reviewed Jan 20, 2023

View reviewed changes

vincepri force-pushed the remove-webhook-config-depinject branch 6 times, most recently from 9c26a1d to 0d72871 Compare January 20, 2023 20:18

alvaroaleman reviewed Jan 20, 2023

View reviewed changes

⚠️ Remove pkg/inject

927c09e

Signed-off-by: Vince Prignano <vincepri@redhat.com>

vincepri force-pushed the remove-webhook-config-depinject branch from 0d72871 to 927c09e Compare January 20, 2023 21:38

alvaroaleman approved these changes Jan 20, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2023

k8s-ci-robot merged commit 35b47fc into kubernetes-sigs:master Jan 20, 2023

tiraboschi mentioned this pull request May 5, 2023

Bump k8s to v0.27.2 kubevirt/hyperconverged-cluster-operator#2333

Merged

11 tasks

This was referenced May 22, 2023

poc: scheduling gates beta verison keptn/lifecycle-toolkit#1249

Closed

Remove Decoder Injector interface from webhook keptn/lifecycle-toolkit#1445

Closed

jaronoff97 mentioned this pull request May 24, 2023

Bump kubernetes to 1.27 operator and target allocator open-telemetry/opentelemetry-operator#1653

Merged

ary1992 mentioned this pull request Jun 16, 2023

Upgrade k8s.io/* to v0.27, sigs.k8s.io/controller-runtime to v0.15 gardener/gardener#7991

Closed

ary1992 added a commit to ary1992/gardener that referenced this pull request Jun 28, 2023

Adapt webhook admission to accept mgr.GetScheme() as first argument.

a166665

kubernetes-sigs/controller-runtime#2134

ary1992 mentioned this pull request Jul 7, 2023

Remove dependency injection functions gardener/gardener#8217

Merged

RainbowMango mentioned this pull request Jul 24, 2023

Bump Kubernetes dependencies to v1.27.3 karmada-io/karmada#3730

Merged

duanhongyi mentioned this pull request Aug 7, 2023

bump sigs.k8s.io/controller-runtime to v0.15.1 drycc-addons/service-catalog#25

Closed

lyarwood mentioned this pull request Aug 9, 2023

chore: Update controller-runtime to v0.15.0 kubevirt/ssp-operator#604

Closed

diranged mentioned this pull request Aug 16, 2023

feat(test): introduce a test that executes the validatingwebhook diranged/oz#168

Merged

Aryan-sharma11 mentioned this pull request Feb 7, 2024

Fix : Pods not being annotated with kubearmor-policy=enabled by Kubearmor controller kubearmor/KubeArmor#1610

Merged

7 tasks

antoninbas mentioned this pull request Mar 27, 2024

admission.Decoder.Decode is panicking after version bump #2695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Remove pkg/inject #2134

⚠️ Remove pkg/inject #2134

vincepri commented Jan 18, 2023

vincepri commented Jan 18, 2023

sbueringer commented Jan 19, 2023 •

edited

alvaroaleman Jan 20, 2023

vincepri Jan 20, 2023

alvaroaleman Jan 20, 2023

vincepri Jan 20, 2023

alvaroaleman Jan 20, 2023

vincepri Jan 20, 2023

alvaroaleman Jan 20, 2023

k8s-ci-robot commented Jan 20, 2023

terinjokes commented Mar 30, 2023

vincepri commented Apr 10, 2023

terinjokes commented Apr 10, 2023

⚠️ Remove pkg/inject #2134

⚠️ Remove pkg/inject #2134

Conversation

vincepri commented Jan 18, 2023

vincepri commented Jan 18, 2023

sbueringer commented Jan 19, 2023 • edited

alvaroaleman Jan 20, 2023

Choose a reason for hiding this comment

vincepri Jan 20, 2023

Choose a reason for hiding this comment

alvaroaleman Jan 20, 2023

Choose a reason for hiding this comment

vincepri Jan 20, 2023

Choose a reason for hiding this comment

alvaroaleman Jan 20, 2023

Choose a reason for hiding this comment

vincepri Jan 20, 2023

Choose a reason for hiding this comment

alvaroaleman Jan 20, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 20, 2023

terinjokes commented Mar 30, 2023

vincepri commented Apr 10, 2023

terinjokes commented Apr 10, 2023

sbueringer commented Jan 19, 2023 •

edited