⚠️ support delete validation in validator interface

[awesomenix]

• Currently validator interface supports create and update
• Add support for delete as well, which could be a breaking change
• Users need to implement ValidateDelete since interface is updated

[awesomenix]

/assign @mengqiy

[mengqiy]

@awesomenix I'd like to hear your use case more before we decide to make change to the interface.

[mengqiy]

For breaking change, we should use :warning: I have updated the title.

[awesomenix]

@mengqiy In our case we use annotations to validate deletes on certain resource so that none of the deletes are accidental. We used Validating Webhooks which support validating DELETE operations as well (this was easy to override in kubebuilder v1), hence requesting the same in v2 as well.

In short making deletes of resources explicit rather than implicit, since we cant protect deletes with finalizers (once the resource is marked for deletion, it can never be recovered)

BTW love the easy scaffolding of webhooks v2, way less code than the original

[mengqiy]

I'm thinking if we should support CONNECT operation. Trying to find if there is a common use case for it.

[awesomenix]

was thinking if we should add support for CONNECT as well, but wasnt sure of any use case. Honestly i do not know when it will be used, during resource get?

[mengqiy]

@awesomenix
Re. DELETE operation, I saw this KEP.

It seems upstream doesn't support it yet. No objects will be in the AdmissionReview.

[awesomenix]

KEP

Thanks for the reference. I see that 1.15 has the fix kubernetes/kubernetes#76346.

Updating PR to reflect the same.

[mengqiy]

Mostly LG.
IMO we should add some logging showing if a type has implemented the Defaulter and Validator interface in builder package

controller-runtime/pkg/builder/webhook.go

Lines 97 to 132 in 8f633b1

	// registerDefaultingWebhook registers a defaulting webhook if th
	func (blder *WebhookBuilder) registerDefaultingWebhook() {
	if defaulter, isDefaulter := blder.apiType.(admission.Defaulter); isDefaulter {
	mwh := admission.DefaultingWebhookFor(defaulter)
	if mwh != nil {
	path := generateMutatePath(blder.gvk)
	// Checking if the path is already registered.
	// If so, just skip it.
	if !blder.isAlreadyHandled(path) {
	log.Info("Registering a mutating webhook",
	"GVK", blder.gvk,
	"path", path)
	blder.mgr.GetWebhookServer().Register(path, mwh)
	}
	}
	}
	}
	func (blder *WebhookBuilder) registerValidatingWebhook() {
	if validator, isValidator := blder.apiType.(admission.Validator); isValidator {
	vwh := admission.ValidatingWebhookFor(validator)
	if vwh != nil {
	path := generateValidatePath(blder.gvk)
	// Checking if the path is already registered.
	// If so, just skip it.
	if !blder.isAlreadyHandled(path) {
	log.Info("Registering a validating webhook",
	"GVK", blder.gvk,
	"path", path)
	blder.mgr.GetWebhookServer().Register(path, vwh)
	}
	}
	}
	}

.
This can give people hints in debugging when they are using webhook builder, but the type doesn't implement the interfaces.

It probably looks like:

func (blder *WebhookBuilder) registerValidatingWebhook() {
	if validator, isValidator := blder.apiType.(admission.Validator); isValidator {
		log.Info("registering validating webhook, since Validator interface is implemented", "GVK", gvk)
		...
	} else {
		log.Info("webhook.Validator interface is not implemented", "GVK", gvk)
	}
}

Same for registerMutatingWebhook

[awesomenix]

@mengqiy thanks for the feedback. Updated PR.

[mengqiy]

There is no action required in this PR.

[mengqiy]

req.OldObject is an empty object when kube-apiserver is v1.14 or earlier.
obj will be a object with zero values everywhere.
DecodeRaw really should return an error when there is an empty object.
I created #529. We should merge that first.

[mengqiy]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: awesomenix, mengqiy

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

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