✨ webhook: add an option to recover from panics in handler #1900

isitinschi · 2022-05-13T18:26:08Z

Currently, a panic occcurence in a webhook handler is not recovered and crashes the webhook server.

This change adds an option to Webhook to recover panics similar to how it is handled in Reconciler. It ensures that panics are converted to normal error response.

k8s-ci-robot · 2022-05-13T18:26:16Z

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

isitinschi · 2022-05-13T18:28:28Z

@vincepri WDYT?

christopherhein · 2022-05-13T19:02:40Z

/ok-to-test

FillZpp · 2022-06-29T04:19:44Z

pkg/webhook/admission/webhook.go

 	"net/http"

+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"


Please sort the imports

FillZpp · 2022-06-29T04:28:41Z

pkg/webhook/admission/webhook.go

@@ -121,6 +124,9 @@ type Webhook struct {
 	// and potentially patches to apply to the handler.
 	Handler Handler

+	// RecoverPanic indicates whether the panic caused by webhook should be recovered.
+	RecoverPanic bool


It will be better if we support to set it in pkg/builder/webhook.go.

isitinschi · 2022-06-30T15:26:36Z

/test pull-controller-runtime-test-master

isitinschi · 2022-06-30T15:36:45Z

@FillZpp thank you for the review. I've addressed all your suggestions. PTAL

FillZpp · 2022-07-01T03:13:26Z

pkg/builder/webhook.go

@@ -68,6 +69,12 @@ func (blder *WebhookBuilder) WithValidator(validator admission.CustomValidator)
 	return blder
 }

+// WithRecoverPanic takes a bool flag which indicates whether the panic caused by webhook should be recovered.
+func (blder *WebhookBuilder) WithRecoverPanic(recoverPanic bool) *WebhookBuilder {


How about just defined as func (blder *WebhookBuilder) RecoverPanic() * WebhookBuilder so that recoverPanic should be true if it is called?

FillZpp · 2022-07-01T03:23:44Z

pkg/webhook/admission/defaulter.go

@@ -33,9 +33,10 @@ type Defaulter interface {
 }

 // DefaultingWebhookFor creates a new Webhook for Defaulting the provided type.
-func DefaultingWebhookFor(defaulter Defaulter) *Webhook {
+func DefaultingWebhookFor(defaulter Defaulter, recoverPanic bool) *Webhook {


I have a little concern about these public API change, not sure if there were some ppl directly use them.

So how about a new func (wh *Webhook) WithRecoverPanic*(enable bool) *Webhook and you can use it in builder/webhook like this return admission.ValidatingWebhookFor(validator).WithRecoverPanic(blder. recoverPanic)

isitinschi · 2022-07-01T11:10:28Z

@FillZpp is it better like this?

FillZpp · 2022-07-01T11:41:48Z

/lgtm

/cc @alvaroaleman

isitinschi · 2022-07-08T06:31:05Z

@alvaroaleman Does it look good to you to be merged?

vincepri · 2022-07-08T15:34:58Z

pkg/webhook/admission/webhook.go

+func (wh *Webhook) Handle(ctx context.Context, req Request) (response Response) {
+	defer func() {
+		if r := recover(); r != nil {
+			if wh.RecoverPanic {


Shouldn't this if condition be before the recover()?

We won't be able to process the panic if we don't recover from it first. Panic recovery works the same way in controller.go:

controller-runtime/pkg/internal/controller/controller.go

Lines 104 to 122 in 15154aa

// Reconcile implements reconcile.Reconciler.

func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (_ reconcile.Result, err error) {

defer func() {

if r := recover(); r != nil {

if c.RecoverPanic {

for _, fn := range utilruntime.PanicHandlers {

fn(r)

}

err = fmt.Errorf("panic: %v [recovered]", r)

return

}

log := logf.FromContext(ctx)

log.Info(fmt.Sprintf("Observed a panic in reconciler: %v", r))

panic(r)

}

}()

return c.Do.Reconcile(ctx, req)

}

But we don't want to process it when RecoverPanic is false? What is done in controller.go seems quite pointless to me and IIRC the r won't contain the stacktrace?

Makes sense, let's make RecoverPanic flag more fair and really not process anything if it is set to false. Adjusted my PR accordingly 👍

isitinschi · 2022-07-15T06:55:05Z

@vincepri @alvaroaleman does it look good to you?

alvaroaleman · 2022-07-15T14:17:23Z

pkg/webhook/admission/webhook.go

+	defer func() {
+		if wh.RecoverPanic {


Suggested change

defer func() {

if wh.RecoverPanic {

if wh.RecoverPanic

defer func() {

Currently, a panic occcurence in a webhook handler is not recovered and crashes the webhook server. This change adds an option to Webhook to recover panics similar to how it is handled in Reconciler. It ensures that panics are converted to normal error response.

alvaroaleman · 2022-07-15T19:41:47Z

Thank you!
/lgtm
/approve

k8s-ci-robot · 2022-07-15T19:41:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, isitinschi

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~~ [alvaroaleman]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 13, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 13, 2022

k8s-ci-robot requested a review from droot May 13, 2022 18:26

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 13, 2022

k8s-ci-robot requested a review from gerred May 13, 2022 18:26

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 13, 2022

FillZpp reviewed Jun 29, 2022

View reviewed changes

isitinschi force-pushed the webhooks-recover-panics branch from e17b4ce to bd15155 Compare June 30, 2022 15:17

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 30, 2022

FillZpp reviewed Jul 1, 2022

View reviewed changes

isitinschi force-pushed the webhooks-recover-panics branch 2 times, most recently from 85eab91 to d52d0fe Compare July 1, 2022 10:56

k8s-ci-robot requested a review from alvaroaleman July 1, 2022 11:41

k8s-ci-robot assigned FillZpp Jul 1, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2022

vincepri reviewed Jul 8, 2022

View reviewed changes

isitinschi force-pushed the webhooks-recover-panics branch from d52d0fe to b5ab2b2 Compare July 13, 2022 14:34

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2022

alvaroaleman reviewed Jul 15, 2022

View reviewed changes

isitinschi force-pushed the webhooks-recover-panics branch from b5ab2b2 to 037bde6 Compare July 15, 2022 16:37

k8s-ci-robot assigned alvaroaleman Jul 15, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2022

k8s-ci-robot merged commit 88234a8 into kubernetes-sigs:master Jul 15, 2022

k8s-ci-robot added this to the v0.10.x milestone Jul 15, 2022

isitinschi deleted the webhooks-recover-panics branch July 18, 2022 08:28

sbueringer mentioned this pull request Aug 19, 2022

Tasks for Kubernetes v1.25 kubernetes-sigs/cluster-api#6661

Closed

29 tasks

rfranzke mentioned this pull request Sep 11, 2022

Upgrade k8s.io/* to v0.25, sigs.k8s.io/controller-runtime to v0.13 gardener/gardener#6661

Closed

alvaroaleman mentioned this pull request Dec 3, 2023

✨Webhook: Handle crashes #2606

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ webhook: add an option to recover from panics in handler #1900

✨ webhook: add an option to recover from panics in handler #1900

isitinschi commented May 13, 2022

k8s-ci-robot commented May 13, 2022

isitinschi commented May 13, 2022

christopherhein commented May 13, 2022

FillZpp Jun 29, 2022

FillZpp Jun 29, 2022

isitinschi commented Jun 30, 2022

isitinschi commented Jun 30, 2022

FillZpp Jul 1, 2022

FillZpp Jul 1, 2022

isitinschi commented Jul 1, 2022

FillZpp commented Jul 1, 2022

isitinschi commented Jul 8, 2022

vincepri Jul 8, 2022

isitinschi Jul 8, 2022 •

edited

alvaroaleman Jul 8, 2022

isitinschi Jul 13, 2022

isitinschi commented Jul 15, 2022

alvaroaleman Jul 15, 2022

isitinschi Jul 15, 2022

alvaroaleman commented Jul 15, 2022

k8s-ci-robot commented Jul 15, 2022

		"net/http"

		utilruntime "k8s.io/apimachinery/pkg/util/runtime"

	// Reconcile implements reconcile.Reconciler.
	func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (_ reconcile.Result, err error) {
	defer func() {
	if r := recover(); r != nil {
	if c.RecoverPanic {
	for _, fn := range utilruntime.PanicHandlers {
	fn(r)
	}
	err = fmt.Errorf("panic: %v [recovered]", r)
	return
	}

	log := logf.FromContext(ctx)
	log.Info(fmt.Sprintf("Observed a panic in reconciler: %v", r))
	panic(r)
	}
	}()
	return c.Do.Reconcile(ctx, req)
	}

✨ webhook: add an option to recover from panics in handler #1900

✨ webhook: add an option to recover from panics in handler #1900

Conversation

isitinschi commented May 13, 2022

k8s-ci-robot commented May 13, 2022

isitinschi commented May 13, 2022

christopherhein commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isitinschi commented Jun 30, 2022

isitinschi commented Jun 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isitinschi commented Jul 1, 2022

FillZpp commented Jul 1, 2022

isitinschi commented Jul 8, 2022

Choose a reason for hiding this comment

isitinschi Jul 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isitinschi commented Jul 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Jul 15, 2022

k8s-ci-robot commented Jul 15, 2022

isitinschi Jul 8, 2022 •

edited