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

Avoid usage of newly deprecated webhook.Validator and webhook.Defaulter interfaces #3721

Open
nathanperkins opened this issue Jan 8, 2024 · 15 comments · May be fixed by #3723
Open

Avoid usage of newly deprecated webhook.Validator and webhook.Defaulter interfaces #3721

nathanperkins opened this issue Jan 8, 2024 · 15 comments · May be fixed by #3723
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@nathanperkins
Copy link

nathanperkins commented Jan 8, 2024

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

@nathanperkins nathanperkins added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 8, 2024
@nathanperkins
Copy link
Author

cc: @alvaroaleman

@alvaroaleman
Copy link
Member

Kubebuilder is a separate project owned by a different set of people.

/transfer kubernetes-sigs/kubebuilder

@k8s-ci-robot
Copy link
Contributor

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

/transfer kubebuilder

@nathanperkins
Copy link
Author

@alvaroaleman thank you for moving this! I thought I made it in kubebuilder but I must have mixed my tabs up.

@camilamacedo86
Copy link
Member

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 !!!

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jan 9, 2024

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.

var _ webhook.Defaulter = &{{ .Resource.Kind }}{}
) 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.

@camilamacedo86 camilamacedo86 added kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jan 9, 2024
@jonas-jonas
Copy link
Contributor

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.

@camilamacedo86
Copy link
Member

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.

@nathanperkins
Copy link
Author

nathanperkins commented Jan 13, 2024

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 kubernetes-sigs/controller-runtime#2596 for context.

@nathanperkins
Copy link
Author

@jonas-jonas, please check the comment above. I will add some comments to the PR as well.

@nathanperkins
Copy link
Author

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

@camilamacedo86

This comment was marked as outdated.

@camilamacedo86
Copy link
Member

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. (move webhooks away from api folder #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 #3215 and can be made as a follow-up. So, let's discuss it in another issue. Is that OK?

c/c @varshaprasad96 ^

@nathanperkins
Copy link
Author

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 #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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
5 participants