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

[jonas-jonas]

trafficstars

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

Closes https://github.com/kubernetes-sigs/kubebuilder/issues/3721

[k8s-ci-robot]

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]

[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

[jonas-jonas]

As per https://github.com/kubernetes-sigs/kubebuilder/issues/3721#issuecomment-1883126875, 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]

/ok-to-test

[jonas-jonas]

@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]

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:

1. Add a _ = res to make the compiler happy (pretty ugly, IMO)
2. add res to the log statement, that is also generated just to "use" the variable inside the method (also not great)
3. Remove the variables in all the test cases that check whether the generated code compiles and don't use the variable (pretty cumbersome)
4. 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]

Hi @jonas-jonas

Regards https://github.com/kubernetes-sigs/kubebuilder/pull/3723#issuecomment-1897130391

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?

[jonas-jonas]

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]

HI @jonas-jonas

https://github.com/kubernetes-sigs/kubebuilder/pull/3723#issuecomment-1898419140

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.

[jonas-jonas]

@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]

Hi @jonas-jonas

Regards : https://github.com/kubernetes-sigs/kubebuilder/pull/3723#issuecomment-1925685343 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]

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]

@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	74b5f8557895a62502886ab38a6c7ebaed4049a5	link	true	/test pull-kubebuilder-e2e-k8s-1-27-10
pull-kubebuilder-e2e-k8s-1-28-6	74b5f8557895a62502886ab38a6c7ebaed4049a5	link	true	/test pull-kubebuilder-e2e-k8s-1-28-6
pull-kubebuilder-e2e-k8s-1-29-0	74b5f8557895a62502886ab38a6c7ebaed4049a5	link	true	/test pull-kubebuilder-e2e-k8s-1-29-0
pull-kubebuilder-e2e-k8s-1-26-6	42bd2b085a2b65cf67bbbf1bcc4f6ace3dd7a308	link	true	/test pull-kubebuilder-e2e-k8s-1-26-6
pull-kubebuilder-e2e-k8s-1-28-0	42bd2b085a2b65cf67bbbf1bcc4f6ace3dd7a308	link	true	/test pull-kubebuilder-e2e-k8s-1-28-0
pull-kubebuilder-e2e-k8s-1-27-3	42bd2b085a2b65cf67bbbf1bcc4f6ace3dd7a308	link	true	/test pull-kubebuilder-e2e-k8s-1-27-3
pull-kubebuilder-e2e-k8s-1-29-2	42bd2b085a2b65cf67bbbf1bcc4f6ace3dd7a308	link	true	/test pull-kubebuilder-e2e-k8s-1-29-2
pull-kubebuilder-e2e-k8s-1-28-7	42bd2b085a2b65cf67bbbf1bcc4f6ace3dd7a308	link	true	/test pull-kubebuilder-e2e-k8s-1-28-7
pull-kubebuilder-e2e-k8s-1-27-11	42bd2b085a2b65cf67bbbf1bcc4f6ace3dd7a308	link	true	/test pull-kubebuilder-e2e-k8s-1-27-11
pull-kubebuilder-e2e-k8s-1-30-0	42bd2b085a2b65cf67bbbf1bcc4f6ace3dd7a308	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]

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

I hope that you do not mind