Skip to content

Commit

Permalink
webhook: add an option to recover from panics in handler
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@isitinschi
isitinschi committed Jul 13, 2022
1 parent 15154aa commit b5ab2b2
Show file tree
Hide file tree
Showing 4 changed files with 239 additions and 7 deletions.
15 changes: 11 additions & 4 deletions pkg/builder/webhook.go
View file
Expand Up @@ -39,6 +39,7 @@ type WebhookBuilder struct {
gvk schema.GroupVersionKind
mgr manager.Manager
config *rest.Config
recoverPanic bool
}

// WebhookManagedBy allows inform its manager.Manager.
Expand Down Expand Up @@ -68,6 +69,12 @@ func (blder *WebhookBuilder) WithValidator(validator admission.CustomValidator)
return blder
}

// RecoverPanic indicates whether the panic caused by webhook should be recovered.
func (blder *WebhookBuilder) RecoverPanic() *WebhookBuilder {
blder.recoverPanic = true
return blder
}

// Complete builds the webhook.
func (blder *WebhookBuilder) Complete() error {
// Set the Config
Expand Down Expand Up @@ -124,10 +131,10 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() {

func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook {
if defaulter := blder.withDefaulter; defaulter != nil {
return admission.WithCustomDefaulter(blder.apiType, defaulter)
return admission.WithCustomDefaulter(blder.apiType, defaulter).WithRecoverPanic(blder.recoverPanic)
}
if defaulter, ok := blder.apiType.(admission.Defaulter); ok {
return admission.DefaultingWebhookFor(defaulter)
return admission.DefaultingWebhookFor(defaulter).WithRecoverPanic(blder.recoverPanic)
}
log.Info(
"skip registering a mutating webhook, object does not implement admission.Defaulter or WithDefaulter wasn't called",
Expand All @@ -153,10 +160,10 @@ func (blder *WebhookBuilder) registerValidatingWebhook() {

func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook {
if validator := blder.withValidator; validator != nil {
return admission.WithCustomValidator(blder.apiType, validator)
return admission.WithCustomValidator(blder.apiType, validator).WithRecoverPanic(blder.recoverPanic)
}
if validator, ok := blder.apiType.(admission.Validator); ok {
return admission.ValidatingWebhookFor(validator)
return admission.ValidatingWebhookFor(validator).WithRecoverPanic(blder.recoverPanic)
}
log.Info(
"skip registering a validating webhook, object does not implement admission.Validator or WithValidator wasn't called",
Expand Down
151 changes: 149 additions & 2 deletions pkg/builder/webhook_test.go
View file
Expand Up @@ -134,6 +134,72 @@ func runTests(admissionReviewVersion string) {
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
})

It("should scaffold a defaulting webhook which recovers from panics", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
ExpectWithOffset(1, err).NotTo(HaveOccurred())

By("registering the type in the Scheme")
builder := scheme.Builder{GroupVersion: testDefaulterGVK.GroupVersion()}
builder.Register(&TestDefaulter{}, &TestDefaulterList{})
err = builder.AddToScheme(m.GetScheme())
ExpectWithOffset(1, err).NotTo(HaveOccurred())

err = WebhookManagedBy(m).
For(&TestDefaulter{Panic: true}).
RecoverPanic().
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
svr := m.GetWebhookServer()
ExpectWithOffset(1, svr).NotTo(BeNil())

reader := strings.NewReader(`{
"kind":"AdmissionReview",
"apiVersion":"admission.k8s.io/` + admissionReviewVersion + `",
"request":{
"uid":"07e52e8d-4513-11e9-a716-42010a800270",
"kind":{
"group":"",
"version":"v1",
"kind":"TestDefaulter"
},
"resource":{
"group":"",
"version":"v1",
"resource":"testdefaulter"
},
"namespace":"default",
"operation":"CREATE",
"object":{
"replica":1,
"panic":true
},
"oldObject":null
}
}`)

ctx, cancel := context.WithCancel(context.Background())
cancel()
// TODO: we may want to improve it to make it be able to inject dependencies,
// but not always try to load certs and return not found error.
err = svr.Start(ctx)
if err != nil && !os.IsNotExist(err) {
ExpectWithOffset(1, err).NotTo(HaveOccurred())
}

By("sending a request to a mutating webhook path")
path := generateMutatePath(testDefaulterGVK)
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable fields")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: injected panic [recovered]`))
})

It("should scaffold a defaulting webhook with a custom defaulter", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
Expand Down Expand Up @@ -284,6 +350,73 @@ func runTests(admissionReviewVersion string) {
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`))
})

It("should scaffold a validating webhook which recovers from panics", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
ExpectWithOffset(1, err).NotTo(HaveOccurred())

By("registering the type in the Scheme")
builder := scheme.Builder{GroupVersion: testValidatorGVK.GroupVersion()}
builder.Register(&TestValidator{}, &TestValidatorList{})
err = builder.AddToScheme(m.GetScheme())
ExpectWithOffset(1, err).NotTo(HaveOccurred())

err = WebhookManagedBy(m).
For(&TestValidator{Panic: true}).
RecoverPanic().
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
svr := m.GetWebhookServer()
ExpectWithOffset(1, svr).NotTo(BeNil())

reader := strings.NewReader(`{
"kind":"AdmissionReview",
"apiVersion":"admission.k8s.io/` + admissionReviewVersion + `",
"request":{
"uid":"07e52e8d-4513-11e9-a716-42010a800270",
"kind":{
"group":"",
"version":"v1",
"kind":"TestValidator"
},
"resource":{
"group":"",
"version":"v1",
"resource":"testvalidator"
},
"namespace":"default",
"operation":"CREATE",
"object":{
"replica":2,
"panic":true
}
}
}`)

ctx, cancel := context.WithCancel(context.Background())
cancel()
// TODO: we may want to improve it to make it be able to inject dependencies,
// but not always try to load certs and return not found error.
err = svr.Start(ctx)
if err != nil && !os.IsNotExist(err) {
ExpectWithOffset(1, err).NotTo(HaveOccurred())
}

By("sending a request to a validating webhook path")
path := generateValidatePath(testValidatorGVK)
_, err = reader.Seek(0, 0)
ExpectWithOffset(1, err).NotTo(HaveOccurred())
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable field")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: injected panic [recovered]`))
})

It("should scaffold a validating webhook with a custom validator", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
Expand Down Expand Up @@ -542,7 +675,8 @@ var _ runtime.Object = &TestDefaulter{}
const testDefaulterKind = "TestDefaulter"

type TestDefaulter struct {
Replica int `json:"replica,omitempty"`
Replica int `json:"replica,omitempty"`
Panic bool `json:"panic,omitempty"`
}

var testDefaulterGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: testDefaulterKind}
Expand All @@ -568,6 +702,9 @@ func (*TestDefaulterList) GetObjectKind() schema.ObjectKind { return nil }
func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil }

func (d *TestDefaulter) Default() {
if d.Panic {
panic("injected panic")
}
if d.Replica < 2 {
d.Replica = 2
}
Expand All @@ -579,7 +716,8 @@ var _ runtime.Object = &TestValidator{}
const testValidatorKind = "TestValidator"

type TestValidator struct {
Replica int `json:"replica,omitempty"`
Replica int `json:"replica,omitempty"`
Panic bool `json:"panic,omitempty"`
}

var testValidatorGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: testValidatorKind}
Expand Down Expand Up @@ -607,13 +745,19 @@ func (*TestValidatorList) DeepCopyObject() runtime.Object { return nil }
var _ admission.Validator = &TestValidator{}

func (v *TestValidator) ValidateCreate() error {
if v.Panic {
panic("injected panic")
}
if v.Replica < 0 {
return errors.New("number of replica should be greater than or equal to 0")
}
return nil
}

func (v *TestValidator) ValidateUpdate(old runtime.Object) error {
if v.Panic {
panic("injected panic")
}
if v.Replica < 0 {
return errors.New("number of replica should be greater than or equal to 0")
}
Expand All @@ -626,6 +770,9 @@ func (v *TestValidator) ValidateUpdate(old runtime.Object) error {
}

func (v *TestValidator) ValidateDelete() error {
if v.Panic {
panic("injected panic")
}
if v.Replica > 0 {
return errors.New("number of replica should be less than or equal to 0 to delete")
}
Expand Down
25 changes: 24 additions & 1 deletion pkg/webhook/admission/webhook.go
View file
Expand Up @@ -19,6 +19,7 @@ package admission
import (
"context"
"errors"
"fmt"
"net/http"

"github.com/go-logr/logr"
Expand All @@ -27,6 +28,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/json"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/client-go/kubernetes/scheme"

logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
Expand Down Expand Up @@ -121,6 +123,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
Expand All @@ -138,11 +143,29 @@ func (wh *Webhook) InjectLogger(l logr.Logger) error {
return nil
}

// WithRecoverPanic takes a bool flag which indicates whether the panic caused by webhook should be recovered.
func (wh *Webhook) WithRecoverPanic(recoverPanic bool) *Webhook {
wh.RecoverPanic = recoverPanic
return wh
}

// Handle processes AdmissionRequest.
// 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 wh.RecoverPanic {
if r := recover(); r != nil {
for _, fn := range utilruntime.PanicHandlers {
fn(r)
}
response = Errored(http.StatusInternalServerError, fmt.Errorf("panic: %v [recovered]", r))
return
}
}
}()

resp := wh.Handler.Handle(ctx, req)
if err := resp.Complete(req); err != nil {
wh.log.Error(err, "unable to encode response")
Expand Down
55 changes: 55 additions & 0 deletions pkg/webhook/admission/webhook_test.go
View file
Expand Up @@ -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{})
})
})
})

var _ = Describe("Should be able to write/read admission.Request to/from context", func() {
Expand Down

0 comments on commit b5ab2b2

Please sign in to comment.