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 Jun 30, 2022
1 parent 365ae09 commit bd15155
Show file tree
Hide file tree
Showing 10 changed files with 252 additions and 17 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
}

// WithRecoverPanic takes a bool flag which indicates whether the panic caused by webhook should be recovered.
func (blder *WebhookBuilder) WithRecoverPanic(recoverPanic bool) *WebhookBuilder {
blder.recoverPanic = recoverPanic
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, blder.recoverPanic)
}
if defaulter, ok := blder.apiType.(admission.Defaulter); ok {
return admission.DefaultingWebhookFor(defaulter)
return admission.DefaultingWebhookFor(defaulter, 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, blder.recoverPanic)
}
if validator, ok := blder.apiType.(admission.Validator); ok {
return admission.ValidatingWebhookFor(validator)
return admission.ValidatingWebhookFor(validator, 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}).
WithRecoverPanic(true).
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}).
WithRecoverPanic(true).
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 @@ -540,7 +673,8 @@ func runTests(admissionReviewVersion string) {
var _ runtime.Object = &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: "TestDefaulter"}
Expand All @@ -566,6 +700,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 @@ -575,7 +712,8 @@ func (d *TestDefaulter) Default() {
var _ runtime.Object = &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: "TestValidator"}
Expand Down Expand Up @@ -603,13 +741,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 @@ -622,6 +766,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
5 changes: 3 additions & 2 deletions pkg/webhook/admission/defaulter.go
View file
Expand Up @@ -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 {
return &Webhook{
Handler: &mutatingHandler{defaulter: defaulter},
Handler: &mutatingHandler{defaulter: defaulter},
RecoverPanic: recoverPanic,
}
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/webhook/admission/defaulter_custom.go
View file
Expand Up @@ -33,9 +33,10 @@ type CustomDefaulter interface {
}

// WithCustomDefaulter creates a new Webhook for a CustomDefaulter interface.
func WithCustomDefaulter(obj runtime.Object, defaulter CustomDefaulter) *Webhook {
func WithCustomDefaulter(obj runtime.Object, defaulter CustomDefaulter, recoverPanic bool) *Webhook {
return &Webhook{
Handler: &defaulterForType{object: obj, defaulter: defaulter},
Handler: &defaulterForType{object: obj, defaulter: defaulter},
RecoverPanic: recoverPanic,
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/webhook/admission/defaulter_test.go
View file
Expand Up @@ -16,7 +16,7 @@ var _ = Describe("Defaulter Handler", func() {

It("should return ok if received delete verb in defaulter handler", func() {
obj := &TestDefaulter{}
handler := DefaultingWebhookFor(obj)
handler := DefaultingWebhookFor(obj, false)

resp := handler.Handle(context.TODO(), Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Expand Down
5 changes: 3 additions & 2 deletions pkg/webhook/admission/validator.go
View file
Expand Up @@ -35,9 +35,10 @@ type Validator interface {
}

// ValidatingWebhookFor creates a new Webhook for validating the provided type.
func ValidatingWebhookFor(validator Validator) *Webhook {
func ValidatingWebhookFor(validator Validator, recoverPanic bool) *Webhook {
return &Webhook{
Handler: &validatingHandler{validator: validator},
Handler: &validatingHandler{validator: validator},
RecoverPanic: recoverPanic,
}
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/webhook/admission/validator_custom.go
View file
Expand Up @@ -35,9 +35,10 @@ type CustomValidator interface {
}

// WithCustomValidator creates a new Webhook for validating the provided type.
func WithCustomValidator(obj runtime.Object, validator CustomValidator) *Webhook {
func WithCustomValidator(obj runtime.Object, validator CustomValidator, recoverPanic bool) *Webhook {
return &Webhook{
Handler: &validatorForType{object: obj, validator: validator},
Handler: &validatorForType{object: obj, validator: validator},
RecoverPanic: recoverPanic,
}
}

Expand Down
22 changes: 21 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 @@ -142,7 +147,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")
Expand Down

0 comments on commit bd15155

Please sign in to comment.