⚠️ : (go/v4): Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces #3723

jonas-jonas · 2024-01-11T10:26:20Z

Replaces the usage of the deprecated webhook.Validator and webhook.Defaulter interfaces with their counterparts admission.CustomValidator and admission.CustomDefaulter.

Closes #3721

k8s-ci-robot · 2024-01-11T10:26:30Z

Hi @jonas-jonas. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2024-01-11T10:26:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jonas-jonas
Once this PR has been reviewed and has the lgtm label, please assign camilamacedo86 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

docs/book/src/component-config-tutorial/testdata/project/README.md

jonas-jonas · 2024-01-12T13:00:24Z

As per #3721 (comment), this change only updates the usages in the golang/v4 part and the docs/ files (which were generated this way). Let me know, if the other files should be changed as well.
@camilamacedo86 I updated this branch to be re-based on master (with the new timestamps). Please review :)

camilamacedo86 · 2024-01-12T20:41:18Z

/ok-to-test

camilamacedo86

Hi @jonas-jonas,

As per #3721 (comment), this change only updates the usages in the golang/v4 part and the docs/ files (which were generated this way). Let me know, if the other files should be changed as well.
@camilamacedo86 I updated this branch to be re-based on master (with the new timestamps). Please review :)

It will be solved by running make generate. Make generate will update the docs and samples under testdata. See that the command generate all and in case of the docs we inject / add the code on top after with the code implemented in: https://github.com/kubernetes-sigs/kubebuilder/blob/master/hack/docs/generate_samples.go (example: https://github.com/kubernetes-sigs/kubebuilder/blob/master/hack/docs/internal/cronjob-tutorial/webhook_implementation.go#L50-L51). You need to change the const/scripts as well.

I noticed that your recent pull request is not passing the CI checks. Here's a breakdown of the issues and some guidance on how to address them:

APIDiff / Verify API differences: This check tends to fail when there are potential breaking changes, which sometimes are inevitable, especially with updates in the controller-runtime. While we aim to avoid such changes, they are acceptable under these circumstances, so you can disregard this failure for now.
Testdata Verification: This is failing because make generate hasn't been run. To resolve this, please:
a) Rebase your branch with the master branch.
b) Ensure that you update the scripts in hack/docs accordingly (example: https://github.com/kubernetes-sigs/kubebuilder/blob/master/hack/docs/internal/cronjob-tutorial/webhook_implementation.go#L50-L51)
c) Execute make generate to update the test data.
d) Squash your commits. Remember, we maintain a one commit per PR policy.
E2E Tests (pull-kubebuilder-e2e-k8s-*): These are end-to-end tests failing on your PR. For debugging:
a) Analyze the logs provided here: E2E Test Logs.
b) Run the tests locally for a more in-depth investigation. You can use Golang IDEA to run these in debug mode, which might be helpful. Here's the test suite you'll be looking at: E2E Test Suite.
c) It's important to understand that these test failures indicate that something in your changes might have broken existing functionality, so careful analysis and debugging are crucial here.

Please take the time to address these issues and update your pull request accordingly. If you have any questions or need further assistance, feel free to reach out!

docs/book/src/cronjob-tutorial/webhook-implementation.md

docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go

jonas-jonas · 2024-01-15T10:54:27Z

@camilamacedo86 thanks for the detailed write-up. I started debugging the e2e failure, and will provide the fix in the coming days. I think I got an idea, on what's going on.

jonas-jonas · 2024-01-17T22:23:27Z

I figured out why the e2e tests are failing: the admission.CustomDefaulter/CustomValidator interfaces work by passing in the runtime.Object as a parameter, instead of using the struct itself (like webhook.Defaulter). This, however, means that there is no type safety inside the Default/ValidateX methods anymore. To fix this, Kubebuilder would need to add a type check and cast inside the generated code to make it possible to access the fields of the struct again.

E.g. generate the following:

func (r *CronJob) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
	cronjoblog.Info("validate create", "name", r.Name)
	res := obj.(*CronJob) // <--- THIS IS NEW

	// TODO(user): fill in your validation logic upon object creation.
	return nil, nil
}

The issue with this is, that by default the generated code doesn't compile anymore, since there is an unused variable res inside the function.

To fix this, I could:

Add a _ = res to make the compiler happy (pretty ugly, IMO)
add res to the log statement, that is also generated just to "use" the variable inside the method (also not great)
Remove the variables in all the test cases that check whether the generated code compiles and don't use the variable (pretty cumbersome)
Provide a Kubebuilder wrapper for the admission.CustomDefaulter and admission.CustomValidator that provides the argument with the type of the current struct

IMO 4) would be the cleanest option, as it gives implementors type safe and compilable code that does not contain (unchecked) type casts. But I am not sure if this is even feasible in the current setup and what would be needed for this.

(And I guess, one other option would be to change the interface in the controller-runtime. There is probably a solution using generics that could work here. But, this would likely be a breaking change over there.)

Let me know if you see any other options here.

camilamacedo86

/ok-to-test

camilamacedo86 · 2024-01-18T12:35:03Z

pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook.go


-// Default implements webhook.Defaulter so a webhook will be registered for the type
-func (r *{{ .Resource.Kind }}) Default() {
+// Default implements admission.CustomDefaulter so a webhook will be registered for the type


Are we calling admission.CustomDefaulter for default webhooks?
Or did I misunderstood? Should not it only be used for admission ones?

webhook.CustomDefaulter is an alias for admission.CustomDefaulter. I missed that when I first looked at the required changes, but have now reverted it to use webhook.CustomDefaulter. I hope this answers your question.

camilamacedo86

@varshaprasad96 and @nathanperkins

Could you please, if you have the time, lend a hand with this review? I believe we need to ensure that the changes align accordingly with the types, including defaulting, converting, and so on. Thank you a lot.

camilamacedo86 · 2024-01-18T12:42:49Z

Hi @jonas-jonas

Regards #3723 (comment)

I am not sure if I could follow up but if we need to scaffold res := obj.(*CronJob) // <--- THIS IS NEW we can do. See that we have the Kind in {{ .Resource.Kind }}.

If that does not answer the problem, can you please point out the code that is not working out and how do you need to change it in your POV so that I might help you within.

But it seems that it is passing the e2e tests now; how many options did you use to sort out?

pkg/plugin/util/util.go

jonas-jonas · 2024-01-18T12:48:35Z

I am not sure if I could follow up but if we need to scaffold res := obj.(*CronJob) // <--- THIS IS NEW we can do. See that we have the Kind in {{ .Resource.Kind }}.

No, the problem is, that there would be an unused variable in the generated code by default, which would mean that the Go compiler does not compile the generated code anymore without making adjustments to the code. I outlined potential solutions in the comment above.

I am a bit perplexed, by the passing test data generation, as that fails on my machine, due to unused variables, etc.

camilamacedo86 · 2024-01-18T16:58:17Z

HI @jonas-jonas

#3723 (comment)

The e2e tests are generated by scaffolding the projects. Then, after that, we add the code on top and run the tests.
We have not located this scaffold locally since they were removed after the tests.

I will need to find some time to test it locally for a better understanding.
It might take some time but I will let you know.

pkg/plugin/util/util.go

pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook.go

camilamacedo86 · 2024-02-01T08:06:04Z

docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go

@@ -46,6 +46,8 @@ Then, we set up the webhook with the manager.
 func (r *CronJob) SetupWebhookWithManager(mgr ctrl.Manager) error {
 return ctrl.NewWebhookManagedBy(mgr).
 For(r).
+ WithValidator(r).
+ WithDefaulter(r).


Did you test it out without the WithValidator?
Because looking the controlle-runtime code it seems that the structure that should be passed is one for the Validator? Am I right ? Do we need to use it?

So, before this change, with the deprecated webhook interface For was enough to enable both the validation and defaulting logic, if r implemented either (there are runtime checks here, if I am not mistaken).
For custom validator and custom defaulter, you need to specifically WithValidator or WithDefaulter to enable them.

A test for just the CustomDefaulter is here and for CustomValidator here.

But seems that we are saying here.
The implementation is For the struct r
and with the validator using the same struct.
Should we not have a new type, struct for the validation? (i.e. CronJobCustomValidator)

See: https://github.com/kubernetes-sigs/controller-runtime/blob/4000e996a202917ad7d40f02ed8a2079a9ce25e9/pkg/builder/webhook.go#L70-L74

I understand that you pass an struct representing the Validator and not the object that will be validated.

I believe that's what's been done here, though. This also hasn't changed. It's just, that, instead of doing this check at runtime, using a runtime type assertion, we need to explicitly tell the controller runtime to use this implementation to validate/provide defaults.

I sort of get what you mean, though. However, changing that would be quite a breaking change, and the implications of that change would go over my head.

We need the validator struct
We should not be passing :

WithValidator(r). WithDefaulter(r).

Agree with Camila. CustomValidator and CustomDefaulter should be implemented on a separate struct.

Otherwise there would have been no point to introducing these if we still implement them on the API type.

(the goal is to have CustomValidator/CustomDefaulter outside of the api package to reduce the dependencies to controller-runtime in the API package)

Thank you to confirm my understanding about the requirements for the changes

camilamacedo86 · 2024-02-01T08:09:15Z

testdata/project-v4-multigroup-with-deploy-image/api/v1/lakers_webhook.go

 }

 // TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation.
 //+kubebuilder:webhook:path=/validate-testproject-org-v1-lakers,mutating=false,failurePolicy=fail,sideEffects=None,groups=testproject.org,resources=lakers,verbs=create;update,versions=v1,name=vlakers.kb.io,admissionReviewVersions=v1

-var _ webhook.Validator = &Lakers{}
+var _ admission.CustomValidator = &Lakers{}


Should we really use Lakers{} Or should we create a new struct for the validator?

c/c @nathanperkins ^

To add some context:
The old Validator ran the Validate functions on the deepcopied object:
https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/webhook/admission/validator.go#L105
The new CustomValidator does not do this anymore:
https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/webhook/admission/validator_custom.go#L101

I'd propose to go with a new struct similar to reconciler structs, as there is no benefit in reusing (polluting) the API type.
Let me know if I can support in any way with this.

pkg/plugin/util/testdata/exampleFile.txt

…ulter

jonas-jonas · 2024-02-04T10:22:50Z

@camilamacedo86 sorry for the delays on my side. I've revisited all open conversations and pushed fixes for the remaining issues.

The e2e tests are failing, because the new CustomDefaulter and CustomValidator interfaces no longer provide a type-safe way for accessing the resources:

New signature:

func (r *CronJob) Default(ctx context.Context, obj runtime.Object) error {

(where obj is a deep copy of the r instance with updated values set.)

vs.

func (r *CronJob) Default() {

(where r can be used to access everything that's needed.)

Please advise on how to move forward with this.

camilamacedo86 · 2024-02-06T08:17:40Z

Hi @jonas-jonas

Regards : #3723 (comment)
By looking at the examples in the controller-runtime it seems for me that we should have

func (r *CronJobCustomValidator) Default(ctx context.Context, obj runtime.Object) error {

See: https://github.com/kubernetes-sigs/controller-runtime/blob/4000e996a202917ad7d40f02ed8a2079a9ce25e9/pkg/builder/webhook_test.go#L869-L930

Also, we can log the obj, and we should add some implementations like the one we have in the examples.
We can scaffold ALL that would be required for any case scenario in the default one.

To do this task, it is required to scaffold a project with Kubebuilder, create an API and a webhook, and then try to do a basic implementation and see if that works. So that we can ensure the right way to do so. Did you make that?
Furthermore, It would be great to ensure we also cover it with e2e tests.

k8s-ci-robot · 2024-04-27T19:08:15Z

PR needs rebase.

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.

k8s-ci-robot · 2024-05-16T13:05:18Z

@jonas-jonas: The following tests 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-kubebuilder-e2e-k8s-1-27-10	`74b5f85`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-10`
pull-kubebuilder-e2e-k8s-1-28-6	`74b5f85`	link	true	`/test pull-kubebuilder-e2e-k8s-1-28-6`
pull-kubebuilder-e2e-k8s-1-29-0	`74b5f85`	link	true	`/test pull-kubebuilder-e2e-k8s-1-29-0`
pull-kubebuilder-e2e-k8s-1-26-6	`42bd2b0`	link	true	`/test pull-kubebuilder-e2e-k8s-1-26-6`
pull-kubebuilder-e2e-k8s-1-28-0	`42bd2b0`	link	true	`/test pull-kubebuilder-e2e-k8s-1-28-0`
pull-kubebuilder-e2e-k8s-1-27-3	`42bd2b0`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-3`
pull-kubebuilder-e2e-k8s-1-29-2	`42bd2b0`	link	true	`/test pull-kubebuilder-e2e-k8s-1-29-2`
pull-kubebuilder-e2e-k8s-1-28-7	`42bd2b0`	link	true	`/test pull-kubebuilder-e2e-k8s-1-28-7`
pull-kubebuilder-e2e-k8s-1-27-11	`42bd2b0`	link	true	`/test pull-kubebuilder-e2e-k8s-1-27-11`
pull-kubebuilder-e2e-k8s-1-30-0	`42bd2b0`	link	true	`/test pull-kubebuilder-e2e-k8s-1-30-0`

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-sigs/prow repository. I understand the commands that are listed here.

camilamacedo86 · 2024-08-09T16:32:02Z

Closing in favor https://github.com/kubernetes-sigs/kubebuilder/pull/4060/files

I hope that you do not mind

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-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 11, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 11, 2024

k8s-ci-robot requested review from camilamacedo86 and varshaprasad96 January 11, 2024 10:26

camilamacedo86 reviewed Jan 12, 2024

View reviewed changes

docs/book/src/component-config-tutorial/testdata/project/README.md Outdated Show resolved Hide resolved

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 12, 2024

jonas-jonas marked this pull request as ready for review January 12, 2024 12:57

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 Jan 12, 2024

k8s-ci-robot requested a review from camilamacedo86 January 12, 2024 12:57

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 12, 2024

camilamacedo86 reviewed Jan 14, 2024

View reviewed changes

nathanperkins reviewed Jan 14, 2024

View reviewed changes

docs/book/src/cronjob-tutorial/webhook-implementation.md Show resolved Hide resolved

docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go Outdated Show resolved Hide resolved

jonas-jonas force-pushed the jonas-jonas/fixDeprecatedInterfaces branch from 2796a31 to 7526e3c Compare January 17, 2024 21:54

camilamacedo86 reviewed Jan 18, 2024

View reviewed changes

camilamacedo86 changed the title ~~🐛: Remove use of deprecated webhook.Validator and webhook.Defaulter interfaces~~ ⚠️ Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces Jan 18, 2024

camilamacedo86 changed the title ~~⚠️ Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces~~ ⚠️ : (go/v4): Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces Jan 18, 2024

camilamacedo86 reviewed Jan 18, 2024

View reviewed changes

pkg/plugin/util/util.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jan 22, 2024

View reviewed changes

pkg/plugin/util/util.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Jan 22, 2024

View reviewed changes

pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook.go Show resolved Hide resolved

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

jonas-jonas force-pushed the jonas-jonas/fixDeprecatedInterfaces branch from 7526e3c to 74b5f85 Compare January 27, 2024 10:06

camilamacedo86 reviewed Feb 1, 2024

View reviewed changes

pkg/plugin/util/testdata/exampleFile.txt Outdated Show resolved Hide resolved

jonas-jonas force-pushed the jonas-jonas/fixDeprecatedInterfaces branch from 74b5f85 to 40528f9 Compare February 4, 2024 10:01

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

Use CustomValidator and CustomDefaulter instead of Validator and Defa…

42bd2b0

…ulter

jonas-jonas force-pushed the jonas-jonas/fixDeprecatedInterfaces branch from 40528f9 to 42bd2b0 Compare February 4, 2024 10:15

camilamacedo86 mentioned this pull request Apr 6, 2024

📖 update webhooks for core types to match controller-runtime v0.15 #3399

Closed

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

camilamacedo86 added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label May 24, 2024

camilamacedo86 closed this Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ : (go/v4): Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces #3723

⚠️ : (go/v4): Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces #3723

jonas-jonas commented Jan 11, 2024 •

edited

Loading

k8s-ci-robot commented Jan 11, 2024

k8s-ci-robot commented Jan 11, 2024

jonas-jonas commented Jan 12, 2024

camilamacedo86 commented Jan 12, 2024

camilamacedo86 left a comment

jonas-jonas commented Jan 15, 2024

jonas-jonas commented Jan 17, 2024

camilamacedo86 left a comment

camilamacedo86 Jan 18, 2024

jonas-jonas Feb 4, 2024

camilamacedo86 left a comment

camilamacedo86 commented Jan 18, 2024 •

edited

Loading

jonas-jonas commented Jan 18, 2024 •

edited

Loading

camilamacedo86 commented Jan 18, 2024 •

edited

Loading

camilamacedo86 Feb 1, 2024

jonas-jonas Feb 3, 2024

camilamacedo86 Feb 4, 2024 •

edited

Loading

camilamacedo86 Feb 5, 2024

jonas-jonas Feb 5, 2024

camilamacedo86 Feb 22, 2024

sbueringer Aug 8, 2024 •

edited

Loading

camilamacedo86 Aug 8, 2024

camilamacedo86 Feb 1, 2024

Henni Feb 22, 2024

jonas-jonas commented Feb 4, 2024

camilamacedo86 commented Feb 6, 2024

k8s-ci-robot commented Apr 27, 2024

k8s-ci-robot commented May 16, 2024

camilamacedo86 commented Aug 9, 2024

⚠️ : (go/v4): Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces #3723

⚠️ : (go/v4): Replace usage of deprecated webhook.Validator and webhook.Defaulter interfaces #3723

Conversation

jonas-jonas commented Jan 11, 2024 • edited Loading

k8s-ci-robot commented Jan 11, 2024

k8s-ci-robot commented Jan 11, 2024

jonas-jonas commented Jan 12, 2024

camilamacedo86 commented Jan 12, 2024

camilamacedo86 left a comment

Choose a reason for hiding this comment

jonas-jonas commented Jan 15, 2024

jonas-jonas commented Jan 17, 2024

camilamacedo86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Jan 18, 2024 • edited Loading

jonas-jonas commented Jan 18, 2024 • edited Loading

camilamacedo86 commented Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Feb 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-jonas commented Feb 4, 2024

camilamacedo86 commented Feb 6, 2024

k8s-ci-robot commented Apr 27, 2024

k8s-ci-robot commented May 16, 2024

camilamacedo86 commented Aug 9, 2024

jonas-jonas commented Jan 11, 2024 •

edited

Loading

camilamacedo86 commented Jan 18, 2024 •

edited

Loading

jonas-jonas commented Jan 18, 2024 •

edited

Loading

camilamacedo86 commented Jan 18, 2024 •

edited

Loading

camilamacedo86 Feb 4, 2024 •

edited

Loading

sbueringer Aug 8, 2024 •

edited

Loading