Skip to content

Commit

Permalink
fix review findings
Browse files Browse the repository at this point in the history
  • Loading branch information
sbueringer committed Aug 6, 2024
1 parent 36acd6b commit 3452b25
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 11 deletions.
28 changes: 22 additions & 6 deletions pkg/builder/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ func (blder *WebhookBuilder) WithLogConstructor(logConstructor func(base logr.Lo

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

Expand Down Expand Up @@ -170,10 +170,18 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() {

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

func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook {
if validator := blder.customValidator; validator != nil {
return admission.WithCustomValidator(blder.mgr.GetScheme(), blder.apiType, validator).WithRecoverPanic(blder.recoverPanic)
w := admission.WithCustomValidator(blder.mgr.GetScheme(), blder.apiType, validator)
if blder.recoverPanic != nil {
w = w.WithRecoverPanic(*blder.recoverPanic)
}
return w
}
if validator, ok := blder.apiType.(admission.Validator); ok {
return admission.ValidatingWebhookFor(blder.mgr.GetScheme(), validator).WithRecoverPanic(blder.recoverPanic)
w := admission.ValidatingWebhookFor(blder.mgr.GetScheme(), validator)
if blder.recoverPanic != nil {
w = w.WithRecoverPanic(*blder.recoverPanic)
}
return w
}
log.Info(
"skip registering a validating webhook, object does not implement admission.Validator or WithValidator wasn't called",
Expand Down
5 changes: 2 additions & 3 deletions pkg/builder/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/utils/ptr"

logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
Expand Down Expand Up @@ -161,7 +160,7 @@ func runTests(admissionReviewVersion string) {

err = WebhookManagedBy(m).
For(&TestDefaulter{Panic: true}).
RecoverPanic(nil). // Defaults to true.
// RecoverPanic defaults to true.
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
svr := m.GetWebhookServer()
Expand Down Expand Up @@ -370,7 +369,7 @@ func runTests(admissionReviewVersion string) {

err = WebhookManagedBy(m).
For(&TestValidator{Panic: true}).
RecoverPanic(ptr.To[bool](true)).
RecoverPanic(true).
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
svr := m.GetWebhookServer()
Expand Down
4 changes: 2 additions & 2 deletions pkg/webhook/admission/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ type Webhook struct {

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

Expand Down

0 comments on commit 3452b25

Please sign in to comment.