Skip to content

🐛 Fix custom path for webhooks conflict #3102

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 17, 2025
Merged

🐛 Fix custom path for webhooks conflict #3102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 46 additions & 15 deletions pkg/builder/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,19 @@ import (

// WebhookBuilder builds a Webhook.
type WebhookBuilder struct {
apiType runtime.Object
customDefaulter admission.CustomDefaulter
customDefaulterOpts []admission.DefaulterOption
customValidator admission.CustomValidator
customPath string
gvk schema.GroupVersionKind
mgr manager.Manager
config *rest.Config
recoverPanic *bool
logConstructor func(base logr.Logger, req *admission.Request) logr.Logger
err error
apiType runtime.Object
customDefaulter admission.CustomDefaulter
customDefaulterOpts []admission.DefaulterOption
customValidator admission.CustomValidator
customPath string
customValidatorCustomPath string
customDefaulterCustomPath string
gvk schema.GroupVersionKind
mgr manager.Manager
config *rest.Config
recoverPanic *bool
logConstructor func(base logr.Logger, req *admission.Request) logr.Logger
err error
}

// WebhookManagedBy returns a new webhook builder.
Expand Down Expand Up @@ -96,11 +98,25 @@ func (blder *WebhookBuilder) RecoverPanic(recoverPanic bool) *WebhookBuilder {
}

// WithCustomPath overrides the webhook's default path by the customPath
// Deprecated: WithCustomPath should not be used anymore.
// Please use WithValidatorCustomPath or WithDefaulterCustomPath instead.
func (blder *WebhookBuilder) WithCustomPath(customPath string) *WebhookBuilder {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sbueringer sbueringer Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@sbueringer sbueringer Mar 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

blder.customPath = customPath
return blder
}

// WithValidatorCustomPath overrides the path of the Validator.
func (blder *WebhookBuilder) WithValidatorCustomPath(customPath string) *WebhookBuilder {
blder.customValidatorCustomPath = customPath
return blder
}

// WithDefaulterCustomPath overrides the path of the Defaulter.
func (blder *WebhookBuilder) WithDefaulterCustomPath(customPath string) *WebhookBuilder {
blder.customDefaulterCustomPath = customPath
return blder
}

// Complete builds the webhook.
func (blder *WebhookBuilder) Complete() error {
// Set the Config
Expand Down Expand Up @@ -139,6 +155,10 @@ func (blder *WebhookBuilder) setLogConstructor() {
}
}

func (blder *WebhookBuilder) isThereCustomPathConflict() bool {
return (blder.customPath != "" && blder.customDefaulter != nil && blder.customValidator != nil) || (blder.customPath != "" && blder.customDefaulterCustomPath != "") || (blder.customPath != "" && blder.customValidatorCustomPath != "")
}

func (blder *WebhookBuilder) registerWebhooks() error {
typ, err := blder.getType()
if err != nil {
Expand All @@ -150,6 +170,17 @@ func (blder *WebhookBuilder) registerWebhooks() error {
return err
}

if blder.isThereCustomPathConflict() {
return errors.New("only one of CustomDefaulter or CustomValidator should be set when using WithCustomPath. Otherwise, WithDefaulterCustomPath() and WithValidatorCustomPath() should be used")
}
if blder.customPath != "" {
// isThereCustomPathConflict() already checks for potential conflicts.
// Since we are sure that only one of customDefaulter or customValidator will be used,
// we can set both customDefaulterCustomPath and validatingCustomPath.
blder.customDefaulterCustomPath = blder.customPath
blder.customValidatorCustomPath = blder.customPath
}

// Register webhook(s) for type
err = blder.registerDefaultingWebhook()
if err != nil {
Expand All @@ -174,8 +205,8 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() error {
if mwh != nil {
mwh.LogConstructor = blder.logConstructor
path := generateMutatePath(blder.gvk)
if blder.customPath != "" {
generatedCustomPath, err := generateCustomPath(blder.customPath)
if blder.customDefaulterCustomPath != "" {
generatedCustomPath, err := generateCustomPath(blder.customDefaulterCustomPath)
if err != nil {
return err
}
Expand Down Expand Up @@ -212,8 +243,8 @@ func (blder *WebhookBuilder) registerValidatingWebhook() error {
if vwh != nil {
vwh.LogConstructor = blder.logConstructor
path := generateValidatePath(blder.gvk)
if blder.customPath != "" {
generatedCustomPath, err := generateCustomPath(blder.customPath)
if blder.customValidatorCustomPath != "" {
generatedCustomPath, err := generateCustomPath(blder.customValidatorCustomPath)
if err != nil {
return err
}
Expand Down
Loading
Loading