🌱 Move API v1beta1 webhooks to a separate package #9047

JoelSpeed · 2023-07-21T15:48:03Z

What this PR does / why we need it:

To work towards #9011, we need to remove the reliance on controller-runtime from the API packages.
This updates the webhooks to use the CustomDefaulter and CustomValidator pattern and moves them to match the Cluster and ClusterClass webhooks. This Is a more major lift but removes quite a bit of the code from the API package itself, making it's reliance on controller-runtime a lot smaller.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

k8s-ci-robot · 2023-07-21T15:48:05Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

JoelSpeed · 2023-07-21T16:30:36Z

/test ?

k8s-ci-robot · 2023-07-21T16:30:38Z

@JoelSpeed: The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-test-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-dualstack-and-ipv6-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-scale-main-experimental
/test pull-cluster-api-e2e-workload-upgrade-1-27-latest-main
/test pull-cluster-api-test-mink8s-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-informing-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-verify-main

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JoelSpeed · 2023-07-21T16:31:10Z

/test pull-cluster-api-test-main

sbueringer

Thank you very much for working on this.

Sorry for the long delay, just a few smaller findings

internal/controllers/topology/cluster/desired_state.go

webhooks/alias.go

sbueringer · 2023-08-14T13:06:37Z

internal/webhooks/machinedeployment_webhook.go

-	return &machineDeploymentDefaulter{
+// MachineDeploymentWebhook creates a new Webhook for MachineDeployments.
+func MachineDeploymentWebhook(scheme *runtime.Scheme) Webhook {
+	return &machineDeployment{


WDYT about implementing this the same way as in:

cluster-api/controlplane/kubeadm/internal/webhooks/scale.go

Line 35 in ea0dc4d

v.decoder = admission.NewDecoder(mgr.GetScheme())

?

(i.e. we would drop the constructor, take the scheme from the manager in SetupWebhookWithManager and get rid of the interface, also basically this aligns it to the other webhooks)

I think I implemented the previous version and for some reason I wanted to keep SetupWebhookWithManager on the MachineDeployment object, but now it seems better to be consistent to me

I guess the only risk of that, is that you then may not have a decoder present right? And in all test we would have to set up the decoder and pass through, with this at least we have that guarantee.

I can get a commit up to compare and discuss though

Yup. Seems a similar risk to that the Client is not set in the Cluster & ClusterClass webhooks (similar behavior in all of our controllers)

I think I'm not against ensuring that everything is there via constructors, I would mostly prefer consistency across webhooks (& ideally controllers)

Basically we currently assume that webhooks & controllers are always used ~ like this:

if err := (&controllers.MachineReconciler{ Client: mgr.GetClient(), ... }).SetupWithManager(ctx, mgr, concurrency(machineConcurrency)); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Machine") os.Exit(1) } ... if err := (&webhooks.Cluster{Client: mgr.GetClient()}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Cluster") os.Exit(1) }

Maybe one option is to aim for consistency across webhooks for this PR and (if we want) do a follow-up to figure out if we want to make it safer across the board

I think at least the current layering with internal enforces that folks have to call SetupWebhookWithManager on the exported webhook struct to get an instance of the internal webhook.

So we should be able to rely on that SetupWebhookWithManager is called for the internal webhook struct

(for unit tests we have to do it correctly though :))

internal/webhooks/machinedeployment_webhook.go

internal/webhooks/machinedeployment_webhook_test.go

internal/webhooks/machinehealthcheck_webhook.go

internal/webhooks/machine_webhook.go

JoelSpeed · 2023-08-14T14:51:37Z

@sbueringer I've updated all of your requests in individual commits to make the review easier, LMK if those are good for you and I can squash back down into a single commit

sbueringer

Looks good thx, last round of nits

internal/webhooks/machinedeployment.go

k8s-ci-robot · 2023-08-14T15:33:58Z

@JoelSpeed: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`8193dec`	link	false	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sbueringer · 2023-08-14T16:43:13Z

Thx!

/lgtm

/assign @vincepri @killianmuldoon

k8s-ci-robot · 2023-08-14T16:43:20Z

LGTM label has been added.

Git tree hash: 8f6b2fcd3d8e3d6b70def3e31ea286a7356c7184

sbueringer · 2023-08-17T05:28:18Z

/approve

We can follow up if there are further findings

k8s-ci-robot · 2023-08-17T05:28:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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

g-gaston · 2023-10-23T21:18:08Z

/area api

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 21, 2023

k8s-ci-robot requested review from fabriziopandini and jackfrancis July 21, 2023 15:48

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

JoelSpeed changed the title ~~🌱 Move APi v1beta1 webhooks to a separate package~~ 🌱 Move API v1beta1 webhooks to a separate package Jul 21, 2023

JoelSpeed force-pushed the move-webhooks-from-api branch 2 times, most recently from 7a38e98 to 9b409d8 Compare July 26, 2023 14:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2023

JoelSpeed force-pushed the move-webhooks-from-api branch 2 times, most recently from f09e167 to 7a79d40 Compare July 26, 2023 15:50

JoelSpeed marked this pull request as ready for review July 26, 2023 15:56

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2023

k8s-ci-robot requested a review from CecileRobertMichon July 26, 2023 15:56

JoelSpeed force-pushed the move-webhooks-from-api branch from 7a79d40 to 507cd3f Compare July 27, 2023 10:48

JoelSpeed mentioned this pull request Aug 3, 2023

Importing API types pulls in lots of dependencies #9011

Open

25 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2023

JoelSpeed force-pushed the move-webhooks-from-api branch from 507cd3f to 79de0a3 Compare August 8, 2023 10:13

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2023

JoelSpeed force-pushed the move-webhooks-from-api branch from 79de0a3 to b501f5f Compare August 8, 2023 10:58

sbueringer mentioned this pull request Aug 9, 2023

Importing API types pulls in lots of dependencies kubernetes-sigs/cluster-api-provider-vsphere#2159

Closed

sbueringer reviewed Aug 14, 2023

View reviewed changes

internal/webhooks/machinedeployment.go Outdated Show resolved Hide resolved

internal/webhooks/machinedeployment.go Outdated Show resolved Hide resolved

Move webhooks to a separate package

8193dec

JoelSpeed force-pushed the move-webhooks-from-api branch from 0e63639 to 8193dec Compare August 14, 2023 15:31

k8s-ci-robot assigned killianmuldoon, vincepri and sbueringer Aug 14, 2023

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2023

k8s-ci-robot merged commit e2f1a48 into kubernetes-sigs:main Aug 17, 2023
18 of 19 checks passed

k8s-ci-robot added this to the v1.6 milestone Aug 17, 2023

JoelSpeed deleted the move-webhooks-from-api branch August 21, 2023 10:50

k8s-ci-robot added the area/api Issues or PRs related to the APIs label Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Move API v1beta1 webhooks to a separate package #9047

🌱 Move API v1beta1 webhooks to a separate package #9047

JoelSpeed commented Jul 21, 2023 •

edited

k8s-ci-robot commented Jul 21, 2023

JoelSpeed commented Jul 21, 2023

k8s-ci-robot commented Jul 21, 2023

JoelSpeed commented Jul 21, 2023

sbueringer left a comment

sbueringer Aug 14, 2023

sbueringer Aug 14, 2023

JoelSpeed Aug 14, 2023

sbueringer Aug 14, 2023

sbueringer Aug 14, 2023

sbueringer Aug 14, 2023

sbueringer Aug 14, 2023 •

edited

JoelSpeed commented Aug 14, 2023 •

edited

sbueringer left a comment

k8s-ci-robot commented Aug 14, 2023

sbueringer commented Aug 14, 2023

k8s-ci-robot commented Aug 14, 2023

sbueringer commented Aug 17, 2023

k8s-ci-robot commented Aug 17, 2023

g-gaston commented Oct 23, 2023

🌱 Move API v1beta1 webhooks to a separate package #9047

🌱 Move API v1beta1 webhooks to a separate package #9047

Conversation

JoelSpeed commented Jul 21, 2023 • edited

k8s-ci-robot commented Jul 21, 2023

JoelSpeed commented Jul 21, 2023

k8s-ci-robot commented Jul 21, 2023

JoelSpeed commented Jul 21, 2023

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer Aug 14, 2023

Choose a reason for hiding this comment

sbueringer Aug 14, 2023

Choose a reason for hiding this comment

JoelSpeed Aug 14, 2023

Choose a reason for hiding this comment

sbueringer Aug 14, 2023

Choose a reason for hiding this comment

sbueringer Aug 14, 2023

Choose a reason for hiding this comment

sbueringer Aug 14, 2023

Choose a reason for hiding this comment

sbueringer Aug 14, 2023 • edited

Choose a reason for hiding this comment

JoelSpeed commented Aug 14, 2023 • edited

sbueringer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 14, 2023

sbueringer commented Aug 14, 2023

k8s-ci-robot commented Aug 14, 2023

sbueringer commented Aug 17, 2023

k8s-ci-robot commented Aug 17, 2023

g-gaston commented Oct 23, 2023

JoelSpeed commented Jul 21, 2023 •

edited

sbueringer Aug 14, 2023 •

edited

JoelSpeed commented Aug 14, 2023 •

edited