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

🌱 Move API v1beta1 webhooks to a separate package #9047

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

JoelSpeed
Copy link
Contributor

@JoelSpeed JoelSpeed commented Jul 21, 2023

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
Copy link
Contributor

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

@k8s-ci-robot 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 k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 21, 2023
@JoelSpeed JoelSpeed changed the title 🌱 Move APi v1beta1 webhooks to a separate package 🌱 Move API v1beta1 webhooks to a separate package Jul 21, 2023
@JoelSpeed
Copy link
Contributor Author

/test ?

@k8s-ci-robot
Copy link
Contributor

@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
Copy link
Contributor Author

/test pull-cluster-api-test-main

@JoelSpeed 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 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 JoelSpeed force-pushed the move-webhooks-from-api branch 2 times, most recently from f09e167 to 7a79d40 Compare July 26, 2023 15:50
@JoelSpeed JoelSpeed marked this pull request as ready for review July 26, 2023 15:56
@k8s-ci-robot 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
@JoelSpeed JoelSpeed force-pushed the move-webhooks-from-api branch from 7a79d40 to 507cd3f Compare July 27, 2023 10:48
@k8s-ci-robot 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 JoelSpeed force-pushed the move-webhooks-from-api branch from 507cd3f to 79de0a3 Compare August 8, 2023 10:13
@k8s-ci-robot 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 JoelSpeed force-pushed the move-webhooks-from-api branch from 79de0a3 to b501f5f Compare August 8, 2023 10:58
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
webhooks/alias.go Outdated Show resolved Hide resolved
return &machineDeploymentDefaulter{
// MachineDeploymentWebhook creates a new Webhook for MachineDeployments.
func MachineDeploymentWebhook(scheme *runtime.Scheme) Webhook {
return &machineDeployment{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about implementing this the same way as in:

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
	}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@sbueringer sbueringer Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
internal/webhooks/machinedeployment_webhook_test.go Outdated Show resolved Hide resolved
internal/webhooks/machinehealthcheck_webhook.go Outdated Show resolved Hide resolved
internal/webhooks/machine_webhook.go Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Contributor Author

JoelSpeed commented Aug 14, 2023

@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

Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thx, last round of nits

internal/webhooks/machinedeployment.go Outdated Show resolved Hide resolved
internal/webhooks/machinedeployment.go Outdated Show resolved Hide resolved
@JoelSpeed JoelSpeed force-pushed the move-webhooks-from-api branch from 0e63639 to 8193dec Compare August 14, 2023 15:31
@k8s-ci-robot
Copy link
Contributor

@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
Copy link
Member

Thx!

/lgtm

/assign @vincepri @killianmuldoon

@k8s-ci-robot 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
Copy link
Contributor

LGTM label has been added.

Git tree hash: 8f6b2fcd3d8e3d6b70def3e31ea286a7356c7184

@sbueringer
Copy link
Member

/approve

We can follow up if there are further findings

@k8s-ci-robot
Copy link
Contributor

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

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

@k8s-ci-robot 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 k8s-ci-robot merged commit e2f1a48 into kubernetes-sigs:main Aug 17, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Aug 17, 2023
@JoelSpeed JoelSpeed deleted the move-webhooks-from-api branch August 21, 2023 10:50
@g-gaston
Copy link
Contributor

/area api

@k8s-ci-robot k8s-ci-robot added the area/api Issues or PRs related to the APIs label Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Issues or PRs related to the APIs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants