Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from Jul 15, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions pkg/builder/webhook.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// 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) {
if wh.RecoverPanic {
defer func() {
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
Open in desktop
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