🐛 Fix custom path for webhooks conflict

[damsien]

Following this PR #2998. I implemented the custom path for webhooks considering that it will be used by either only the validating or the defaulting. However, the user can actually use the both of them on the same webhook manager.

Until now, when the user was using .WithDefaulter(&TestCustomDefaulter{}), .WithValidator(&TestCustomValidator{}) and .WithCustomPath(customPath) at the same time, an endless loop was launched by constantly calling the other webhook. This bug is caused because they are both register at the same path.

This is the reason why I made this PR. I split the .WithCustomPath() function into two functions .WithDefaultingCustomPath() and .WithValidatingCustomPath() in order to be used in this way:

WebhookManagedBy(mgr).
	For(&TestDefaulter{}).
	WithDefaulter(&TestCustomDefaulter{}).
	WithValidator(&TestCustomValidator{}).
	WithDefaultingCustomPath(defaultingCustomPath).
	WithValidatingCustomPath(validatingCustomPath).
	Complete()

Also, I added two tests:

1. One that tests the example above.
2. And I take advantage of making this PR to add a second one that tests the usage of WithDefaulter() and WithValidator() at the same time (without the custom path implementation) because there were no test about this specific configuration before.

[k8s-ci-robot]

Hi @damsien. 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-sigs/prow repository.

[damsien]

Hello @alvaroaleman , what do you think about this one? I know that it can potentially be a breaking change because we are renaming a function that already exists. I am not sure about the upgrade process that must be done (is it simply a ⚠️ breaking change warning in the release note or a more elaborated procedure?).

Also, I wanted to let you know that it is not yet implemented in the kubebuilder project. I am currently working on the implementation into kubebuilder but I first want to ensure that this bug has been fixed before implementing it.

[alvaroaleman]

Renaming this makes this PR a breaking change for everyone, we should not do that. The two main alternatives I can think of are:

1. Only one of WithCustomDefaulter/WithCustomValidator may be set when using WithCustomPath, otherwise we error out
2. We use indexing, i.E. when both are set, there must also be two WithCustomPath calls and the first gets the first/the second the second

[damsien]

Hello Alvaro,

What about this one:

We keep the WithCustomPath(), WithDefaultingCustomPath() and WithValidatingCustomPath() functions. If the developer uses the WithCustomPath() function, then only one of the WithCustomDefaulter() or WithCustomValidator() should be set, otherwise the developer should use the specific custom path functions.

Maybe it is not a good idea because we have multiple function accomplishing the same purpose. Waiting for you thoughts about it.

[sbueringer]

What about:

1. Enforce only one of WithCustomDefaulter/WithCustomValidator may be set when using WithCustomPath, otherwise we error out
2. Deprecate WithCustomPath
3. Add new options to WithDefaulter / WithValidator that allows to set the path

[sbueringer]

@damsien I meant something slightly diferently :)

Instead of having additional WithValidatingCustomPath & WithDefaultingCustomPath funcs I was suggesting to add options to the existing WithDefaulter / WithValidator functions to set the path (similar to the existing option that already exists for the WithDefaulter funct)

[sbueringer]

Okay nevermind. Just noticed that DefaulterOptions are also passed down to WithCustomDefaulter and there the custom path wouldn't make any sense

[damsien]

Hello @alvaroaleman @sbueringer,

Following our conversation, I add some pre-checks:

1. Enforce only one of WithCustomDefaulter()/WithCustomValidator() when using WithCustomPath()
2. Enforce only one of WithDefaultingCustomPath() or WithCustomPath()
3. Enforce only one of WithValidatingCustomPath() or WithCustomPath()

If at least one of these checks are not good, then we error out when calling Complete().

Also, I added a Deprecated comment on top of the WithCustomPath() function.

What do you think? (I'll squash all my commits when we'll validate the merge)

[sbueringer]

/ok-to-test

[damsien]

Thanks @sbueringer !
I renamed the functions and variables (validating -> validator & defaulting -> defaulter). Did some changes to pass the lint check.

[sbueringer]

Thank you!

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: b286157ec763827d7020ed7d1dedfcb140f5b958

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damsien, sbueringer

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sbueringer]

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