Skip to content

Commit

Permalink
Merge pull request #1900 from isitinschi/webhooks-recover-panics
Browse files Browse the repository at this point in the history 
✨ webhook: add an option to recover from panics in handler
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jul 15, 2022
2 parents b93b5f9 + 037bde6 commit 88234a8
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) {
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
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 88234a8

Please sign in to comment.