Deletion verb interrupted by mutating handler

[AnyISalIn]

Environment

• kubebuilder v3.1.0
• controller-runtime v0.8.3
• kubernetes 1.20.3

How to reproduce

• enable deletion validation for CRD
• enable mutating handler for CRD
• execute kubectl delete <crd> <cr>, its should returns Error from server: admission webhook "xxxxxx" denied the request: there is no content to decode

Possible Problems

when you want to delete a CR, mutaing handler want to Decode(req.Object), however it doesn't work, cause Kubernetes DELETE Admission Request Body only contains OldObject and the deletion verb is broken.

can we skip deletion verb in the mutating handler?

Reference Issues

#1278

[FillZpp]

Yeah, it used to be in req.Object and has been moved to req.OldObject since Kubernetes 1.16 for both validating and mutating DELETE.

I'm not sure should we just skip for deletion request here, or improve it like this:

	reqObj := req.Object
	if req.Operation == admissionv1.Delete {
		reqObj = req.OldObject
	}
	if err := h.decoder.DecodeRaw(reqObj, obj); err != nil {
		return Errored(http.StatusBadRequest, err)
	}

	...
	return PatchResponseFromRaw(reqObj.Raw, marshalled)

I prefer the latter one, for it is a little weird if users enabled DELETE mutation and got nothing when objects deleted.

[AnyISalIn]

@FillZpp Thanks! but I don't know what happens when mutations return some diff patches when sending DELETE verb?

[FillZpp]

@FillZpp Thanks! but I don't know what happens when mutations return some diff patches when sending DELETE verb?

That's the problem. Could you run a test for the scenario? Or I can do this when I'm free in the new year :)

[AnyISalIn]

@FillZpp Thanks! but I don't know what happens when mutations return some diff patches when sending DELETE verb?

That's the problem. Could you run a test for the scenario? Or I can do this when I'm free in the new year :)

Okay, I will try later, Happy New Year.

[AnyISalIn]

@FillZpp Kubernetes seems unable to handle patches on DELETE verb in mutation handler, see from here mutating/dispatcher.go#L315, may we need to skip it?

Error from server (InternalError): Internal error occurred: admission webhook "mmutatingtest.kb.io" attempted to modify the object, which is not supported for this operation

[FillZpp]

Kubernetes seems unable to handle patches on DELETE verb in mutation handler, see from here mutating/dispatcher.go#L315

Alright, if so, I think it is reasonable to skip this verb.

What's more, I'm not sure if we should disable the DELETE verb in controller-tools so that it will not generate it in MutatingWebhookConfiguration or return an error to warn users that DELETE can not be used in mutating webhook?

Any suggestions? @alvaroaleman @vincepri

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[FillZpp]

/remove-lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[FillZpp]

/remove-lifecycle rotten

[alvaroaleman]

The fix was merged, why are we keeping this open?

[FillZpp]

The fix was merged, why are we keeping this open?

Oh sorry, my mistake.

/close

[k8s-ci-robot]

@FillZpp: Closing this issue.

In response to this:

The fix was merged, why are we keeping this open?

Oh sorry, my mistake.

/close

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/test-infra repository.