kubebuilder
kubebuilder copied to clipboard
kubernetes-sigs
Avoid usage of newly deprecated webhook.Validator and webhook.Defaulter interfaces
What do you want to happen?
In this controller-runtime issue, it was decided to deprecate the webhook.Validator and webhook.Defaulter interfaces, due to issues they cause with project upgrades by requiring controller-runtime import in API packages. Those functions are planned to be removed later.
We should update the Kubebuilder generated code and the docs to use admission.CustomValidator and admission.CustomDefaulter instead.
Jan 13th edit:
We also need to implement this interface outside of the API packages so that the API packages are not coupled with controller-runtime versions (see the issue above for context). The docs should be updated to warn the reader about the dangers of implementing this on the API types.
Extra Labels
No response
Kubebuilder is a separate project owned by a different set of people.
/transfer kubernetes-sigs/kubebuilder
@alvaroaleman: Something went wrong or the destination repo kubernetes-sigs/kubernetes-sigs/kubebuilder does not exist.
In response to this:
Kubebuilder is a separate project owned by a different set of people.
/transfer kubernetes-sigs/kubebuilder
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.
@alvaroaleman thank you for moving this! I thought I made it in kubebuilder but I must have mixed my tabs up.
Thank you for raising this one. It is very helpful.
It would be amazing if we could track issues to fix those breaking changes and desecrations in controller-runtime always. Thank you for moving it @alvaroaleman !!!
What is required to do here:
1 - Replace Defaulter with CustomDefaulter - https://github.com/search?q=repo%3Akubernetes-sigs%2Fkubebuilder%20%20Defaulter&type=code
2 - Validator with CustomValidator - https://github.com/search?q=repo%3Akubernetes-sigs%2Fkubebuilder++Validator&type=code
The change must be done for the docs/examples and default scaffold for the go/v4 plugin only. We also need to ensure that the example/tutorial works well with the new example and perform all required caveats if necessary to make it work with the interface.
TIP Please, change first the default scaffold under pkg/plugins/golang/v4/ (i.e. https://github.com/kubernetes-sigs/kubebuilder/blob/42c5220993b4b2dcd5b1fedf034f4964807561db/pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook.go#L115) and run make generate then, see that the code used in the tutorial as the samples under testdata will be automatically updated with the latest changes. Then, after that ensure that the documentation and steps are either valid after the changes and supplement it as fit.
Hi, would love to provide a PR for this, if it's not yet taken. Looking at your comment @camilamacedo86 and the code base, it seems like a pretty straight forward change.
However, there haven't been any changes to the generated files since last year, meaning all generated files will be touched (2023 -> 2024). Would be happy to provide a separate PR just updating the timestamps if that better suits the workflow here. Let me know what you prefer.
Hi @jonas-jonas,
Would be happy to provide a separate PR just updating the timestamps
That is a good point. Please feel free just to run make, generate and push a PR if that is not done yet and ping me out. I am more than happy to approve this one.
Also, feel free to push the PR for remove the deprecation and ping me out as well.
Sorry I didn't specify in the original post. We need to implement the interface on some type outside of the API packages and update the docs to warn about the dangers of implementing this in the API packages. The purpose of this change is to reduce coupling between the APIs package and controller-runtime code.
See https://github.com/kubernetes-sigs/controller-runtime/issues/2596 for context.
@jonas-jonas, please check the comment above. I will add some comments to the PR as well.
@camilamacedo86 we probably need somebody from the kubebuilder team to let @jonas-jonas know what is the appropriate place to implement this interface so that it is not part of the API packages which are commonly imported by other projects.
HI @nathanperkins,
Thank you for add your input, please see my comment: https://github.com/kubernetes-sigs/controller-runtime/issues/2596#issuecomment-1890900992
The purpose of this change is to reduce coupling between the API package and controller-runtime code.
I am not sure if I go along with the purpose here since each API should have its own webhook. How can you have a generic webhook interface implementation that is decoupled from the API Kind? Did I misunderstood something here or are you proposing that we have a pkg/webhook?
Hi @nathanperkins,
@camilamacedo86, we probably need somebody from the kubebuilder team to let @jonas-jonas know the appropriate place to implement this interface so that it is not part of the API packages commonly imported by other projects.
I understand that we have two issues/tasks here.
- a) No longer scaffolding the deprecated code. (goal of this task)
- b) You want to decouple the webhooks from the API to allow you to import API/ dir from one project to another. Therefore, you are suggesting changing the layout, for we no longer have the webhooks scaffolded under the API/ directory. (https://github.com/kubernetes-sigs/kubebuilder/issues/3215 <- another task)
Unless I misunderstand something, we do not need to change the layout to no longer use deprecated.
Therefore, the second change/RFE (b) is a breaking change tracked in https://github.com/kubernetes-sigs/kubebuilder/issues/3215 and can be made as a follow-up. So, let's discuss it in another issue. Is that OK?
c/c @varshaprasad96 ^
Unless I misunderstand something, we do not need to change the layout to no longer use deprecated. Therefore, the second change/RFE (b) is a breaking change tracked in https://github.com/kubernetes-sigs/kubebuilder/issues/3215 and can be made as a follow-up. So, let's discuss it in another issue. Is that OK?
Totally reasonable, in that case the PR is fine from my perspective!
The PR for controller-runtime to remove the deprecated interfaces is https://github.com/kubernetes-sigs/controller-runtime/pull/2877. Linking to this issue if more discussion is necessary.
