Custom webhook naming (for mutate or validator)

[vsoch]

What do you want to happen?

Hi!

I'm implementing a mutating admission webhook, and I wanted an ability to support more than CRD for it. That doesn't seem to be possible as there is only one apiType here: https://github.com/kubernetes-sigs/controller-runtime/blob/c30c66d67f47feeaf2cf0816e11c6ec0260c6e55/pkg/builder/webhook.go#L56-L62 and so a workaround is to have a flag that goes to main.go that the deploying user can add to determine which (of two, for example) to add. However, the issue there is then the annotation. Even if I fudge it to allow for more than one type (e.g., Pod and Job):

//+kubebuilder:webhook:path=,mutating=true,failurePolicy=fail,sideEffects=None,groups="core;batch",resources=pods;jobs,verbs=create,versions=v1,name=morascache.kb.io,admissionReviewVersions=v1

The name is still going to vary based on what I actually give the builder, e.g., here:

// SetupJobsWebhookWithManager prepares the webhook for jobs
func (r *OrasCache) SetupJobsWebhookWithManager(mgr ctrl.Manager) error {
	injector := &SidecarInjector{Cache: r}
	return ctrl.NewWebhookManagedBy(mgr).
		For(&batchv1.Job{}).
		WithDefaulter(injector).
		Complete()
}

What would solve my problem is an ability to explicitly define the name for the builder, e.g.,

// SetupJobsWebhookWithManager prepares the webhook for jobs
func (r *OrasCache) SetupJobsWebhookWithManager(mgr ctrl.Manager) error {
	injector := &SidecarInjector{Cache: r}
	return ctrl.NewWebhookManagedBy(mgr).
		For(&batchv1.Job{}).
                MutatingName("mutate-sidecar-webhook").
		WithDefaulter(injector).
		Complete()
}

So I am wondering if this would be an accepted feature? I am fairly good with Go and could offer to do a pull request. And if you think there could be support for more than one API type, that could be another interesting discussion. Thanks!

Extra Labels

No response

[camilamacedo86]

Hi @vsoch,

I'm curious about the potential for extending webhook functionality to handle multiple CRDs. I understand that currently, a webhook is typically scoped to a specific Kind. I'm looking to explore scenarios where a single webhook could be used to mutate multiple, distinct Kinds.

However, I have some reservations based on core software engineering principles:

• Cohesion and SRP: A single webhook for multiple Kinds could conflict with the cohesion and Single Responsibility Principle, unless the mutation logic is identical for all involved Kinds.
• Maintainability: Managing multiple Kinds in one webhook might complicate testing and maintenance. Is the trade-off for less code duplication worth the potential overhead?
• Consistency and Extensibility: While Kubernetes favors explicit behavior, an all-in-one webhook could streamline the extension process for new Kinds requiring the same mutations.

Given these considerations, if the mutation logic is similar across different Kinds, would it not be more prudent to encapsulate shared logic in common methods or functions? These could then be invoked by multiple specialized webhooks, each dedicated to a single Kind. This approach would seem to retain the benefits of code reuse without compromising on the clarity and separation of concerns that dedicated webhooks provide.

I'd love to hear your thoughts on this, particularly any use cases you envision where a multi-Kind webhook would be beneficial without impinging on these principles.

Thank you for considering these points, and I look forward to the possibility of discussing this further.

Best regards,

[vsoch]

@camilamacedo86 my webhook is adding a sidecar container to pods. If I do on the level of the pod I can hit pods for multiple different types (Job, Deployment, etc.) But if you imagine a Job Spec that makes 1K pods, it's much more efficient to change the PodSpecTemplate than all 1K pods. But if I change it to watch Job, the user cannot use it with any other object anymore. So I need a way to either dynamically change the single type (not possible right now because of the hard coded annotation, and the automated derivation of the name) or allow for mulitple types (also not possible).

If we can just expose allowing to customize the name (so it's not specific to the type) that would allow for people like myself to at least have the type be a variable, and still one.

[camilamacedo86]

Hi @vsoch,

Interesting, but this one should probably be raised against https://github.com/kubernetes-sigs/controller-runtime as raised in the controller-runtime channel.

[vsoch]

Sure thing, thanks @camilamacedo86

[camilamacedo86]

Hi @vsoch

You might be able to transfer the issue.
I pinged it in the channel and ask if someone could do that.

[vsoch]

okay I'll reopen - if they say no we can close again!

[vsoch]

@camilamacedo86 I found a solution that works with Kubebuilder - I'm going to finish it up and I'll post details here after for others!

[vsoch]

@camilamacedo86 to give you an update (working on this all day!) I got something working (in that it built and deployed in my container) but when I ran the controller-gen locally, I get:

 Error: api/v1alpha1/zz_generated.deepcopy.go:124:9: (*in).DeepCopyInto undefined (type *admission.Decoder has no field or method DeepCopyInto)
# github.com/converged-computing/oras-operator/api/v1alpha1
Error: vet: api/v1alpha1/zz_generated.deepcopy.go:124:9: (*in).DeepCopyInto undefined (type *admission.Decoder has no field or method DeepCopyInto)
make: *** [Makefile:68: vet] Error 1
Error: Process completed with exit code 2.

I tried wrapping the decoder in another struct with the right function, and many other potential workarounds, but no luck so far. Here is one of the current states. https://github.com/converged-computing/oras-operator/pull/11/files

Ideally I can just have the decoder on the struct. I tried moving it to a global variable (didn't work) or creating a scheme for it on the fly (almost worked, but for some reason only job worked) so I'm running out of ideas.

This seems like the issue I'm having kubernetes-sigs/controller-runtime#780

[vsoch]

okay phew, got it working! converged-computing/oras-operator@2bda5f1

I'll do a small summary writeup, but that should be good enough for a developer.

[vsoch]

https://vsoch.github.io/2023/mutating-admission-webhook-multiple-objects/

Thanks for the help! This should work for me for now.