Skip to content

Commit

Permalink
⚠ Remove deprecated Defaulter and Validator interfaces (#2877)
Browse files Browse the repository at this point in the history 
* remove deprecated Defaulter and Validator interfaces

Signed-off-by: Troy Connor <[email protected]>

* remove log, add test for customValidator/customDefaulter

Signed-off-by: Troy Connor <[email protected]>

* remove deprecated funcs, add test for customDefaulter

Signed-off-by: Troy Connor <[email protected]>

* minimize the diff

Signed-off-by: Troy Connor <[email protected]>

---------

Signed-off-by: Troy Connor <[email protected]>
  • Loading branch information
@troy0820
troy0820 authored Sep 2, 2024
1 parent e6c3d13 commit 896f6de
Show file tree
Hide file tree
Showing 8 changed files with 114 additions and 455 deletions.
20 changes: 0 additions & 20 deletions pkg/builder/webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,16 +176,6 @@ func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook {
}
return w
}
if defaulter, ok := blder.apiType.(admission.Defaulter); ok {
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",
"GVK", blder.gvk)
return nil
}

Expand Down Expand Up @@ -215,16 +205,6 @@ func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook {
}
return w
}
if validator, ok := blder.apiType.(admission.Validator); ok {
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",
"GVK", blder.gvk)
return nil
}

Expand Down
176 changes: 20 additions & 156 deletions pkg/builder/webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func runTests(admissionReviewVersion string) {
close(stop)
})

It("should scaffold a defaulting webhook if the type implements the Defaulter interface", func() {
It("should scaffold a custom defaulting webhook if the type implements the CustomDefaulter interface", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
ExpectWithOffset(1, err).NotTo(HaveOccurred())
Expand All @@ -90,6 +90,7 @@ func runTests(admissionReviewVersion string) {

err = WebhookManagedBy(m).
For(&TestDefaulter{}).
WithDefaulter(&TestCustomDefaulter{}).
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
svr := m.GetWebhookServer()
Expand Down Expand Up @@ -147,7 +148,7 @@ func runTests(admissionReviewVersion string) {
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
})

It("should scaffold a defaulting webhook which recovers from panics", func() {
It("should scaffold a custom defaulting webhook which recovers from panics", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
ExpectWithOffset(1, err).NotTo(HaveOccurred())
Expand All @@ -159,7 +160,9 @@ func runTests(admissionReviewVersion string) {
ExpectWithOffset(1, err).NotTo(HaveOccurred())

err = WebhookManagedBy(m).
For(&TestDefaulter{Panic: true}).
For(&TestDefaulter{}).
WithDefaulter(&TestCustomDefaulter{}).
RecoverPanic(true).
// RecoverPanic defaults to true.
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -285,7 +288,7 @@ func runTests(admissionReviewVersion string) {
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
})

It("should scaffold a validating webhook if the type implements the Validator interface", func() {
It("should scaffold a custom validating webhook if the type implements the CustomValidator interface", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
ExpectWithOffset(1, err).NotTo(HaveOccurred())
Expand All @@ -298,6 +301,7 @@ func runTests(admissionReviewVersion string) {

err = WebhookManagedBy(m).
For(&TestValidator{}).
WithValidator(&TestCustomValidator{}).
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
svr := m.GetWebhookServer()
Expand Down Expand Up @@ -356,7 +360,7 @@ func runTests(admissionReviewVersion string) {
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`))
})

It("should scaffold a validating webhook which recovers from panics", func() {
It("should scaffold a custom validating webhook which recovers from panics", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
ExpectWithOffset(1, err).NotTo(HaveOccurred())
Expand All @@ -368,7 +372,8 @@ func runTests(admissionReviewVersion string) {
ExpectWithOffset(1, err).NotTo(HaveOccurred())

err = WebhookManagedBy(m).
For(&TestValidator{Panic: true}).
For(&TestValidator{}).
WithValidator(&TestCustomValidator{}).
RecoverPanic(true).
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -496,80 +501,7 @@ func runTests(admissionReviewVersion string) {
EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Validating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testvalidator"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`))
})

It("should scaffold defaulting and validating webhooks if the type implements both Defaulter and Validator interfaces", 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: testDefaultValidatorGVK.GroupVersion()}
builder.Register(&TestDefaultValidator{}, &TestDefaultValidatorList{})
err = builder.AddToScheme(m.GetScheme())
ExpectWithOffset(1, err).NotTo(HaveOccurred())

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

reader := strings.NewReader(admissionReviewGV + admissionReviewVersion + `",
"request":{
"uid":"07e52e8d-4513-11e9-a716-42010a800270",
"kind":{
"group":"",
"version":"v1",
"kind":"TestDefaultValidator"
},
"resource":{
"group":"",
"version":"v1",
"resource":"testdefaultvalidator"
},
"namespace":"default",
"operation":"CREATE",
"object":{
"replica":1
},
"oldObject":null
}
}`)

ctx, cancel := context.WithCancel(context.Background())
cancel()
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(testDefaultValidatorGVK)
req := httptest.NewRequest("POST", svcBaseAddr+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":true`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"patch":`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`))

By("sending a request to a validating webhook path")
path = generateValidatePath(testDefaultValidatorGVK)
_, err = reader.Seek(0, 0)
ExpectWithOffset(1, err).NotTo(HaveOccurred())
req = httptest.NewRequest("POST", svcBaseAddr+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":true`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`))
})

It("should scaffold a validating webhook if the type implements the Validator interface to validate deletes", func() {
It("should scaffold a custom validating webhook if the type implements the CustomValidator interface to validate deletes", func() {
By("creating a controller manager")
ctx, cancel := context.WithCancel(context.Background())

Expand All @@ -584,6 +516,7 @@ func runTests(admissionReviewVersion string) {

err = WebhookManagedBy(m).
For(&TestValidator{}).
WithValidator(&TestCustomValidator{}).
Complete()
ExpectWithOffset(1, err).NotTo(HaveOccurred())
svr := m.GetWebhookServer()
Expand Down Expand Up @@ -712,15 +645,6 @@ type TestDefaulterList struct{}
func (*TestDefaulterList) GetObjectKind() schema.ObjectKind { return nil }
func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil }

func (d *TestDefaulter) Default() {
if d.Panic {
panic("fake panic test")
}
if d.Replica < 2 {
d.Replica = 2
}
}

// TestValidator.
var _ runtime.Object = &TestValidator{}

Expand Down Expand Up @@ -753,43 +677,6 @@ type TestValidatorList struct{}
func (*TestValidatorList) GetObjectKind() schema.ObjectKind { return nil }
func (*TestValidatorList) DeepCopyObject() runtime.Object { return nil }

var _ admission.Validator = &TestValidator{}

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

func (v *TestValidator) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
if v.Panic {
panic("fake panic test")
}
if v.Replica < 0 {
return nil, errors.New("number of replica should be greater than or equal to 0")
}
if oldObj, ok := old.(*TestValidator); !ok {
return nil, fmt.Errorf("the old object is expected to be %T", oldObj)
} else if v.Replica < oldObj.Replica {
return nil, fmt.Errorf("new replica %v should not be fewer than old replica %v", v.Replica, oldObj.Replica)
}
return nil, nil
}

func (v *TestValidator) ValidateDelete() (admission.Warnings, error) {
if v.Panic {
panic("fake panic test")
}
if v.Replica > 0 {
return nil, errors.New("number of replica should be less than or equal to 0 to delete")
}
return nil, nil
}

// TestDefaultValidator.
var _ runtime.Object = &TestDefaultValidator{}

Expand Down Expand Up @@ -822,37 +709,7 @@ type TestDefaultValidatorList struct{}
func (*TestDefaultValidatorList) GetObjectKind() schema.ObjectKind { return nil }
func (*TestDefaultValidatorList) DeepCopyObject() runtime.Object { return nil }

func (dv *TestDefaultValidator) Default() {
if dv.Replica < 2 {
dv.Replica = 2
}
}

var _ admission.Validator = &TestDefaultValidator{}

func (dv *TestDefaultValidator) ValidateCreate() (admission.Warnings, error) {
if dv.Replica < 0 {
return nil, errors.New("number of replica should be greater than or equal to 0")
}
return nil, nil
}

func (dv *TestDefaultValidator) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
if dv.Replica < 0 {
return nil, errors.New("number of replica should be greater than or equal to 0")
}
return nil, nil
}

func (dv *TestDefaultValidator) ValidateDelete() (admission.Warnings, error) {
if dv.Replica > 0 {
return nil, errors.New("number of replica should be less than or equal to 0 to delete")
}
return nil, nil
}

// TestCustomDefaulter.

type TestCustomDefaulter struct{}

func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error {
Expand All @@ -866,6 +723,10 @@ func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) err
}

d := obj.(*TestDefaulter) //nolint:ifshort
if d.Panic {
panic("fake panic test")
}

if d.Replica < 2 {
d.Replica = 2
}
Expand All @@ -889,6 +750,9 @@ func (*TestCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Obje
}

v := obj.(*TestValidator) //nolint:ifshort
if v.Panic {
panic("fake panic test")
}
if v.Replica < 0 {
return nil, errors.New("number of replica should be greater than or equal to 0")
}
Expand Down
Loading

0 comments on commit 896f6de

Please sign in to comment.