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

Custom path for webhooks #4295

Open
damsien opened this issue Nov 5, 2024 · 3 comments
Open

Custom path for webhooks #4295

damsien opened this issue Nov 5, 2024 · 3 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/blocked triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@damsien
Copy link
Contributor

damsien commented Nov 5, 2024

What do you want to happen?

Hello, I'm coming here to talk about these closed issues #1436 & #1333 that were considered as a bug.

The issue is the following: when I generate the webhook file with kubebuilder create api, an annotation is automatically generated in this form: //+kubebuilder:webhook:path=/validate-xxx-xxx-v1-xxx,mutating=false,.... This annotation can make the developer think that he can actually modify the path (which is not the case as explained here).

The previous issues where blocked mostly because the controller-runtime project didn't implement the custom path feature. However, I recently made a PR that have been merged. This PR adds the ability for the developer to choose to use a custom path for his webhooks.

My idea here is to let the developer choose a custom path if he wants to in his kubebuilder project. This choice will be made only when calling the creation command (kubebuilder create webhook) with a --custom-path flag. By using this flag, kubebuilder will generate an annotation including the custom path of the developer (//+kubebuilder:webhook:path=/my-custom-path,mutating=false,...).

If this feature is accepted, I would like to work on this issue.

Extra Labels

No response

@damsien damsien added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 5, 2024
@camilamacedo86
Copy link
Member

Hi @damsien,

This looks good! From my understanding (please correct me if I’m mistaken):

  • We need to wait for the controller-runtime v0.20 release, which will include your implementation: kubernetes-sigs/controller-runtime#2998.
  • Additionally, it appears that controller-tools need to support a "custom path" since the current implementation has it hardcoded here. So, for Kubebuilder to support "custom paths," we’d need changes in controller-tools as well, correct? Then, once there’s a release with these changes, we can consider how to add this support in Kubebuilder.
  • Lastly, it should work for all types of webhooks (defaulting, mutating, and conversion). We’ll also need to ensure compatibility with both CoreTypes and External types (reference).
  • Furthermore, kubebuilder alpha generate command also need to become to support it.

Therefore, I am adding blocked until we have release versions from controller-runtime and controller-tools with this support. Then, after that if anyone wish to push a PR with proposed changes to add this support please feel free. We can go from there.

@camilamacedo86 camilamacedo86 added triage/needs-information Indicates an issue needs more information in order to work on it. triage/blocked labels Nov 5, 2024
@damsien
Copy link
Contributor Author

damsien commented Nov 6, 2024

controller-tools

I have a few things to say about the controller-tools project:

  • Concerning your second bullet point, it is not hardcoded in the url you sent. Actually, the setLogConstructor is not related to the registration of the webhook (with the webhook path) that happens just after (in the registerWebhooks function).

  • Concerning the controller-tools project, I think that we do not need to change anything since the path that is used to generate the webhook comes from the marker that is generated by kubebuilder as described here.

Moreover, we can see that the controller-tools project respect the "custom path" approach. If we specify a different path in the marker, then the webhook will be generated with the custom path as a value of the .webhooks[].clientConfig.path field. But, controller-runtime (in v0.19) registers the path with the predefined path function. That causes a conflict.

We can find more information on how this Path attribute has been implemented in this PR. Already at that time, this PR explains that there is a discoloration between the controller-runtime and the controller-tools projects on how do they handle the webhook path.

controller-runtime

  • I agree with the fact that we need to wait for the v0.20 of controller-runtime.

If you agree with the fact that we have nothing to change in the controller-tools project (excepts by modifying the comment above the Path attribute), maybe I can start working on PR that implement the solution on kubebuilder and request for a merge when the v0.20 is out?

Do not hesitate to correct me on any statement if I am wrong 😄

@camilamacedo86
Copy link
Member

Hi @damsien

Feel free to push a PR with your proposal for changes when the controller runtime v0.20 is released, and we start to use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. triage/blocked triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
None yet
Development

No branches or pull requests

2 participants