From 0f749ef7d5e65aba4e8a76aa34b4744685981a91 Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Tue, 26 Mar 2019 21:33:51 -0700 Subject: [PATCH] This creates a way for clients of the webhook to decorate the request context. (#342) Clients of webhook can now decorate the request context with additional metadata via: ``` ac := &AdmissionController{ ... // As before WithContext: func(ctx context.Context) context.Context { // logic to attach stuff to ctx } } ``` This metadata can then be accessed off of the context in methods like `SetDefaults` and `Validate` on types registered as webhook handlers. Fixes: https://github.com/knative/pkg/issues/306 --- testing/resource.go | 19 +++++ webhook/webhook.go | 35 ++++++---- webhook/webhook_integration_test.go | 104 ++++++++++++++++++++++++++++ webhook/webhook_test.go | 46 +++++++++--- 4 files changed, 181 insertions(+), 23 deletions(-) diff --git a/testing/resource.go b/testing/resource.go index 527c184d13..f08b171374 100644 --- a/testing/resource.go +++ b/testing/resource.go @@ -55,6 +55,7 @@ var _ apis.Listable = (*Resource)(nil) // ResourceSpec represents test resource spec. type ResourceSpec struct { FieldWithDefault string `json:"fieldWithDefault,omitempty"` + FieldWithContextDefault string `json:"fieldWithContextDefault,omitempty"` FieldWithValidation string `json:"fieldWithValidation,omitempty"` FieldThatsImmutable string `json:"fieldThatsImmutable,omitempty"` FieldThatsImmutableWithDefault string `json:"fieldThatsImmutableWithDefault,omitempty"` @@ -65,11 +66,29 @@ func (c *Resource) SetDefaults(ctx context.Context) { c.Spec.SetDefaults(ctx) } +type onContextKey struct{} + +// WithValue returns a WithContext for attaching an OnContext with the given value. +func WithValue(ctx context.Context, val string) context.Context { + return context.WithValue(ctx, onContextKey{}, &OnContext{Value: val}) +} + +// OnContext is a struct for holding a value attached to a context. +type OnContext struct { + Value string +} + // SetDefaults sets the defaults on the spec. func (cs *ResourceSpec) SetDefaults(ctx context.Context) { if cs.FieldWithDefault == "" { cs.FieldWithDefault = "I'm a default." } + if cs.FieldWithContextDefault == "" { + oc, ok := ctx.Value(onContextKey{}).(*OnContext) + if ok { + cs.FieldWithContextDefault = oc.Value + } + } if cs.FieldThatsImmutableWithDefault == "" { cs.FieldThatsImmutableWithDefault = "this is another default value" } diff --git a/webhook/webhook.go b/webhook/webhook.go index b75f6b64a8..b07d90781e 100644 --- a/webhook/webhook.go +++ b/webhook/webhook.go @@ -121,9 +121,14 @@ type AdmissionController struct { Handlers map[schema.GroupVersionKind]GenericCRD Logger *zap.SugaredLogger + WithContext func(context.Context) context.Context DisallowUnknownFields bool } +func nop(ctx context.Context) context.Context { + return ctx +} + // GenericCRD is the interface definition that allows us to perform the generic // CRD actions like deciding whether to increment generation and so forth. type GenericCRD interface { @@ -205,32 +210,32 @@ func getOrGenerateKeyCertsFromSecret(ctx context.Context, client kubernetes.Inte // validate checks whether "new" and "old" implement HasImmutableFields and checks them, // it then delegates validation to apis.Validatable on "new". -func validate(old GenericCRD, new GenericCRD) error { +func validate(ctx context.Context, old, new GenericCRD) error { if immutableNew, ok := new.(apis.Immutable); ok && old != nil { // Copy the old object and set defaults so that we don't reject our own // defaulting done earlier in the webhook. old = old.DeepCopyObject().(GenericCRD) // TODO(mattmoor): Plumb through a real context - old.SetDefaults(context.TODO()) + old.SetDefaults(ctx) immutableOld, ok := old.(apis.Immutable) if !ok { return fmt.Errorf("unexpected type mismatch %T vs. %T", old, new) } // TODO(mattmoor): Plumb through a real context - if err := immutableNew.CheckImmutableFields(context.TODO(), immutableOld); err != nil { + if err := immutableNew.CheckImmutableFields(ctx, immutableOld); err != nil { return err } } // Can't just `return new.Validate()` because it doesn't properly nil-check. // TODO(mattmoor): Plumb through a real context - if err := new.Validate(context.TODO()); err != nil { + if err := new.Validate(ctx); err != nil { return err } return nil } -func setAnnotations(patches duck.JSONPatch, new, old GenericCRD, ui *authenticationv1.UserInfo) (duck.JSONPatch, error) { +func setAnnotations(ctx context.Context, patches duck.JSONPatch, new, old GenericCRD, ui *authenticationv1.UserInfo) (duck.JSONPatch, error) { // Nowhere to set the annotations. if new == nil { return patches, nil @@ -246,7 +251,7 @@ func setAnnotations(patches duck.JSONPatch, new, old GenericCRD, ui *authenticat b, a := new.DeepCopyObject().(apis.Annotatable), na // TODO(mattmoor): Plumb through a real context - a.AnnotateUserInfo(context.TODO(), oa, ui) + a.AnnotateUserInfo(ctx, oa, ui) patch, err := duck.CreatePatch(b, a) if err != nil { return nil, err @@ -255,10 +260,10 @@ func setAnnotations(patches duck.JSONPatch, new, old GenericCRD, ui *authenticat } // setDefaults simply leverages apis.Defaultable to set defaults. -func setDefaults(patches duck.JSONPatch, crd GenericCRD) (duck.JSONPatch, error) { +func setDefaults(ctx context.Context, patches duck.JSONPatch, crd GenericCRD) (duck.JSONPatch, error) { before, after := crd.DeepCopyObject(), crd // TODO(mattmoor): Plumb through a real context - after.SetDefaults(context.TODO()) + after.SetDefaults(ctx) patch, err := duck.CreatePatch(before, after) if err != nil { @@ -456,7 +461,13 @@ func (ac *AdmissionController) ServeHTTP(w http.ResponseWriter, r *http.Request) zap.String(logkey.Resource, fmt.Sprint(review.Request.Resource)), zap.String(logkey.SubResource, fmt.Sprint(review.Request.SubResource)), zap.String(logkey.UserInfo, fmt.Sprint(review.Request.UserInfo))) - reviewResponse := ac.admit(logging.WithLogger(r.Context(), logger), review.Request) + ctx := logging.WithLogger(r.Context(), logger) + + if ac.WithContext != nil { + ctx = ac.WithContext(ctx) + } + + reviewResponse := ac.admit(ctx, review.Request) var response admissionv1beta1.AdmissionReview if reviewResponse != nil { response.Response = reviewResponse @@ -561,14 +572,14 @@ func (ac *AdmissionController) mutate(ctx context.Context, req *admissionv1beta1 patches = append(patches, rtp...) } - if patches, err = setDefaults(patches, newObj); err != nil { + if patches, err = setDefaults(ctx, patches, newObj); err != nil { logger.Errorw("Failed the resource specific defaulter", zap.Error(err)) // Return the error message as-is to give the defaulter callback // discretion over (our portion of) the message that the user sees. return nil, err } - if patches, err = setAnnotations(patches, newObj, oldObj, &req.UserInfo); err != nil { + if patches, err = setAnnotations(ctx, patches, newObj, oldObj, &req.UserInfo); err != nil { logger.Errorw("Failed the resource annotator", zap.Error(err)) return nil, perrors.Wrap(err, "error setting annotations") } @@ -577,7 +588,7 @@ func (ac *AdmissionController) mutate(ctx context.Context, req *admissionv1beta1 if newObj == nil { return nil, errMissingNewObject } - if err := validate(oldObj, newObj); err != nil { + if err := validate(ctx, oldObj, newObj); err != nil { logger.Errorw("Failed the resource specific validation", zap.Error(err)) // Return the error message as-is to give the validation callback // discretion over (our portion of) the message that the user sees. diff --git a/webhook/webhook_integration_test.go b/webhook/webhook_integration_test.go index f9315fe233..542f0b6236 100644 --- a/webhook/webhook_integration_test.go +++ b/webhook/webhook_integration_test.go @@ -18,6 +18,7 @@ package webhook import ( "bytes" + "context" "crypto/tls" "encoding/json" "fmt" @@ -27,8 +28,10 @@ import ( "testing" "time" + . "github.com/knative/pkg/testing" "github.com/mattbaird/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" + authenticationv1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -216,6 +219,104 @@ func TestValidResponseForResource(t *testing.T) { } } +func TestValidResponseForResourceWithContextDefault(t *testing.T) { + ac, serverURL, err := testSetup(t) + if err != nil { + t.Fatalf("testSetup() = %v", err) + } + theDefault := "Some default value" + ac.WithContext = func(ctx context.Context) context.Context { + return WithValue(ctx, theDefault) + } + + stopCh := make(chan struct{}) + defer close(stopCh) + + go func() { + err := ac.Run(stopCh) + if err != nil { + t.Fatalf("Unable to run controller: %s", err) + } + }() + + pollErr := waitForServerAvailable(t, serverURL, testTimeout) + if pollErr != nil { + t.Fatalf("waitForServerAvailable() = %v", err) + } + tlsClient, err := createSecureTLSClient(t, ac.Client, &ac.Options) + if err != nil { + t.Fatalf("createSecureTLSClient() = %v", err) + } + + admissionreq := &admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Create, + Kind: metav1.GroupVersionKind{ + Group: "pkg.knative.dev", + Version: "v1alpha1", + Kind: "Resource", + }, + } + testRev := createResource("testrev") + marshaled, err := json.Marshal(testRev) + if err != nil { + t.Fatalf("Failed to marshal resource: %s", err) + } + + admissionreq.Object.Raw = marshaled + rev := &admissionv1beta1.AdmissionReview{ + Request: admissionreq, + } + + reqBuf := new(bytes.Buffer) + err = json.NewEncoder(reqBuf).Encode(&rev) + if err != nil { + t.Fatalf("Failed to marshal admission review: %v", err) + } + + req, err := http.NewRequest("GET", fmt.Sprintf("https://%s", serverURL), reqBuf) + if err != nil { + t.Fatalf("http.NewRequest() = %v", err) + } + req.Header.Add("Content-Type", "application/json") + + response, err := tlsClient.Do(req) + if err != nil { + t.Fatalf("Failed to get response %v", err) + } + + if got, want := response.StatusCode, http.StatusOK; got != want { + t.Errorf("Response status code = %v, wanted %v", got, want) + } + + defer response.Body.Close() + responseBody, err := ioutil.ReadAll(response.Body) + if err != nil { + t.Fatalf("Failed to read response body %v", err) + } + + reviewResponse := admissionv1beta1.AdmissionReview{} + + err = json.NewDecoder(bytes.NewReader(responseBody)).Decode(&reviewResponse) + if err != nil { + t.Fatalf("Failed to decode response: %v", err) + } + + expectPatches(t, reviewResponse.Response.Patch, []jsonpatch.JsonPatchOperation{{ + Operation: "add", + Path: "/spec/fieldThatsImmutableWithDefault", + Value: "this is another default value", + }, { + Operation: "add", + Path: "/spec/fieldWithDefault", + Value: "I'm a default.", + }, { + Operation: "add", + Path: "/spec/fieldWithContextDefault", + Value: theDefault, + }, setUserAnnotation("", ""), + }) +} + func TestInvalidResponseForResource(t *testing.T) { ac, serverURL, err := testSetup(t) if err != nil { @@ -256,6 +357,9 @@ func TestInvalidResponseForResource(t *testing.T) { Version: "v1alpha1", Kind: "Resource", }, + UserInfo: authenticationv1.UserInfo{ + Username: user1, + }, } admissionreq.Object.Raw = marshaled diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go index b5eac85a74..ffa883b8e6 100644 --- a/webhook/webhook_test.go +++ b/webhook/webhook_test.go @@ -179,17 +179,41 @@ func TestValidCreateResourceSucceedsWithDefaultPatch(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(TestContextWithLogger(t), createCreateResource(r)) expectAllowed(t, resp) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - { - Operation: "add", - Path: "/spec/fieldThatsImmutableWithDefault", - Value: "this is another default value", - }, { - Operation: "add", - Path: "/spec/fieldWithDefault", - Value: "I'm a default.", - }, - setUserAnnotation(user1, user1), + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{ + Operation: "add", + Path: "/spec/fieldThatsImmutableWithDefault", + Value: "this is another default value", + }, { + Operation: "add", + Path: "/spec/fieldWithDefault", + Value: "I'm a default.", + }, setUserAnnotation(user1, user1), + }) +} + +func TestValidCreateResourceSucceedsWithDefaultPatchWithContext(t *testing.T) { + r := createResource("a name") + _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) + ctx := TestContextWithLogger(t) + + contextDefault := "I came from context y'all" + ctx = WithValue(ctx, contextDefault) + + resp := ac.admit(ctx, createCreateResource(r)) + expectAllowed(t, resp) + expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{ + Operation: "add", + Path: "/spec/fieldThatsImmutableWithDefault", + Value: "this is another default value", + }, { + Operation: "add", + Path: "/spec/fieldWithDefault", + Value: "I'm a default.", + }, { + Operation: "add", + Path: "/spec/fieldWithContextDefault", + Value: contextDefault, + }, setUserAnnotation(user1, user1), }) }