diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 3dcff5fadd..6189c78eff 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -19,8 +19,11 @@ package admission import ( "context" "errors" + "fmt" "net/http" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "github.com/go-logr/logr" jsonpatch "gomodules.xyz/jsonpatch/v2" admissionv1 "k8s.io/api/admission/v1" @@ -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 + // WithContextFunc will allow you to take the http.Request.Context() and // add any additional information such as passing the request path or // headers thus allowing you to read them from within the handler @@ -142,7 +148,22 @@ func (wh *Webhook) InjectLogger(l logr.Logger) error { // If the webhook is mutating type, it delegates the AdmissionRequest to each handler and merge the patches. // If the webhook is validating type, it delegates the AdmissionRequest to each handler and // deny the request if anyone denies. -func (wh *Webhook) Handle(ctx context.Context, req Request) Response { +func (wh *Webhook) Handle(ctx context.Context, req Request) (response Response) { + defer func() { + if r := recover(); r != nil { + if wh.RecoverPanic { + for _, fn := range utilruntime.PanicHandlers { + fn(r) + } + response = Errored(http.StatusInternalServerError, fmt.Errorf("panic: %v [recovered]", r)) + return + } + + wh.log.Info(fmt.Sprintf("Observed a panic in reconciler: %v", r)) + panic(r) + } + }() + resp := wh.Handler.Handle(ctx, req) if err := resp.Complete(req); err != nil { wh.log.Error(err, "unable to encode response") diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index 73b0be1694..607422ee0c 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -192,6 +192,61 @@ var _ = Describe("Admission Webhooks", func() { Expect(handler.dep.decoder).NotTo(BeNil()) }) }) + + Describe("panic recovery", func() { + It("should recover panic if RecoverPanic is true", func() { + panicHandler := func() *Webhook { + handler := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + panic("injected panic") + }, + } + webhook := &Webhook{ + Handler: handler, + RecoverPanic: true, + log: logf.RuntimeLog.WithName("webhook"), + } + + return webhook + } + + By("setting up a webhook with a panicking handler") + webhook := panicHandler() + + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{}) + + By("checking that it errored the request") + Expect(resp.Allowed).To(BeFalse()) + Expect(resp.Result.Code).To(Equal(int32(http.StatusInternalServerError))) + Expect(resp.Result.Message).To(Equal("panic: injected panic [recovered]")) + }) + + It("should not recover panic if RecoverPanic is false by default", func() { + panicHandler := func() *Webhook { + handler := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + panic("injected panic") + }, + } + webhook := &Webhook{ + Handler: handler, + log: logf.RuntimeLog.WithName("webhook"), + } + + return webhook + } + + By("setting up a webhook with a panicking handler") + defer func() { + Expect(recover()).ShouldNot(BeNil()) + }() + webhook := panicHandler() + + By("invoking the webhook") + webhook.Handle(context.Background(), Request{}) + }) + }) }) type stringInjector interface {