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

✨ Allow to wire an object mutation handler #2932

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+130 −9
Open

✨ Allow to wire an object mutation handler #2932

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea977b9
Allow to wire a mutation handler
alculquicondor Aug 23, 2024
50c2afb
Simplify design and error on multiple mutation mechanisms
alculquicondor Sep 24, 2024
ed242f4
revert formatting
alculquicondor Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 31 additions & 9 deletions pkg/builder/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
// WebhookBuilder builds a Webhook.
type WebhookBuilder struct {
apiType runtime.Object
mutationHandler admission.Handler
customDefaulter admission.CustomDefaulter
customValidator admission.CustomValidator
gvk schema.GroupVersionKind
Expand Down Expand Up @@ -65,6 +66,12 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder {
return blder
}

// WithMutationHandler takes an admission.Handler, a MutatingWebhook will be wired for it.
func (blder *WebhookBuilder) WithMutationHandler(h admission.Handler) *WebhookBuilder {
Copy link
Member

@alvaroaleman alvaroaleman Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Its super not obvious when and why to use this versus the defaulter and almost guaranteed to lead to questions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we leave it as "more advanced use cases where you want control over the construction of patches"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we could explicitly add the use case of go types that might be in a different version than the CRD.

blder.mutationHandler = h
return blder
}

// WithDefaulter takes an admission.CustomDefaulter interface, a MutatingWebhook will be wired for this type.
func (blder *WebhookBuilder) WithDefaulter(defaulter admission.CustomDefaulter) *WebhookBuilder {
blder.customDefaulter = defaulter
Expand Down Expand Up @@ -140,7 +147,9 @@ func (blder *WebhookBuilder) registerWebhooks() error {
}

// Register webhook(s) for type
blder.registerDefaultingWebhook()
if err := blder.registerDefaultingWebhook(); err != nil {
return err
}
blder.registerValidatingWebhook()

err = blder.registerConversionWebhook()
Expand All @@ -151,8 +160,11 @@ func (blder *WebhookBuilder) registerWebhooks() error {
}

// registerDefaultingWebhook registers a defaulting webhook if necessary.
func (blder *WebhookBuilder) registerDefaultingWebhook() {
mwh := blder.getDefaultingWebhook()
func (blder *WebhookBuilder) registerDefaultingWebhook() error {
mwh, err := blder.getDefaultingWebhook()
if err != nil {
return err
}
if mwh != nil {
mwh.LogConstructor = blder.logConstructor
path := generateMutatePath(blder.gvk)
Expand All @@ -166,17 +178,27 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() {
blder.mgr.GetWebhookServer().Register(path, mwh)
}
}
return nil
}

func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook {
func (blder *WebhookBuilder) getDefaultingWebhook() (*admission.Webhook, error) {
var w *admission.Webhook
if handler := blder.mutationHandler; handler != nil {
w = &admission.Webhook{Handler: handler}
}
if defaulter := blder.customDefaulter; defaulter != nil {
w := admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter)
if blder.recoverPanic != nil {
w = w.WithRecoverPanic(*blder.recoverPanic)
if w != nil {
return nil, errors.New("a WebhookBuilder can only define a MutationHandler or a Defaulter, but not both")
}
return w
w = admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter)
}
return nil
if w == nil {
return nil, nil
}
if blder.recoverPanic != nil {
w = w.WithRecoverPanic(*blder.recoverPanic)
}
return w, nil
}

// registerValidatingWebhook registers a validating webhook if necessary.
Expand Down
103 changes: 102 additions & 1 deletion pkg/builder/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,12 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/onsi/gomega/gbytes"
"gomodules.xyz/jsonpatch/v2"
admissionv1 "k8s.io/api/admission/v1"
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 @@ -217,6 +220,82 @@ func runTests(admissionReviewVersion string) {
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`))
})

It("should scaffold a mutating webhook with a mutator", 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{}).
WithMutationHandler(&TestMutationHandler{}).
WithLogConstructor(func(base logr.Logger, req *admission.Request) logr.Logger {
return admission.DefaultLogConstructor(testingLogger, req)
}).
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":"foo.test.org",
"version":"v1",
"kind":"TestDefaulter"
},
"resource":{
"group":"foo.test.org",
"version":"v1",
"resource":"testdefaulter"
},
"namespace":"default",
"name":"foo",
"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(testDefaulterGVK)
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 fields")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"patch":`))
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`))
EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Mutating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testdefaulter"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`))

By("sending a request to a validating webhook path that doesn't exist")
path = generateValidatePath(testDefaulterGVK)
_, 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.StatusNotFound))
})

It("should scaffold a custom validating webhook", func() {
By("creating a controller manager")
m, err := manager.New(cfg, manager.Options{})
Expand Down Expand Up @@ -570,7 +649,8 @@ func (*TestDefaultValidatorList) DeepCopyObject() runtime.Object { return nil
type TestCustomDefaulter struct{}

func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error {
logf.FromContext(ctx).Info("Defaulting object")
log := logf.FromContext(ctx)
alculquicondor marked this conversation as resolved.
Show resolved Hide resolved
log.Info("Defaulting object")
req, err := admission.RequestFromContext(ctx)
if err != nil {
return fmt.Errorf("expected admission.Request in ctx: %w", err)
Expand All @@ -592,6 +672,27 @@ func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) err

var _ admission.CustomDefaulter = &TestCustomDefaulter{}

// TestMutationHandler
type TestMutationHandler struct{}

func (*TestMutationHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
log := logf.FromContext(ctx)
log.Info("Mutating object")
return admission.Response{
AdmissionResponse: admissionv1.AdmissionResponse{
Allowed: true,
PatchType: ptr.To(admissionv1.PatchTypeJSONPatch),
},
Patches: []jsonpatch.Operation{{
Operation: "replace",
Path: "/replica",
Value: "2",
}},
}
}

var _ admission.Handler = &TestMutationHandler{}

// TestCustomValidator.

type TestCustomValidator struct{}
Expand Down
Loading