Repository: knative/pkg
Pull request #2583 (2022-08-23 18:03:35)

Make request body available in admission context
Signed-off-by: Andrés Torres <andrest@vmware.com>

<!-- Thanks for sending a pull request! -->

# Changes

<!-- 
Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! 

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic
-->

- :bug: Make request body available in admission context

<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

/kind <kind>

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->
/kind bug

<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

<!-- Please include the 'why' behind your changes if no issue exists -->
The `admissionHandler` is consuming the request body:
https://github.com/knative/pkg/blob/5fbe0c3f016d75ebea3a15ca9727ce52e1abb931/webhook/admission.go#L93

If the `Admit` function of the `AdmissionController` later gets the request using `apis.GetHTTPRequest` and tries to read `Request.Body`, an EOF error will be returned. This PR addresses this issue by making the entire `Request.Body` available again for an additional read operation.

The use case is that I need to parse again the `AdmissionRequest` from `Request.Body` to perform further checks in the validating/mutating phase.

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->
```release-note

```

**Docs**

<!--
:book: If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [knative/docs]: <issue or pr link>
- [Feature Track]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```


linux-foundation-easycla[bot]:  <a href="https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/25767896/142331174/2583/#/?version=2"><img src="https://s3.amazonaws.com/cla-project-logo-prod/cla-not-signed.svg" alt="CLA Not Signed" align="left" height="28" width="328"></a><br/><br /><ul><li><a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/25767896/142331174/2583/#/?version=2' target='_blank'>:x:</a> - login: @elfotografo007 / name: Andrés Torres . The commit (4be31288334b38890eeba9eb0277d14db6e39fb3) is not authorized under a signed CLA. <a href='https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/25767896/142331174/2583/#/?version=2' target='_blank'>Please click here to be authorized</a>. For further assistance with EasyCLA, <a href='https://jira.linuxfoundation.org/servicedesk/customer/portal/4' target='_blank'>please submit a support request ticket</a>.</li></ul>
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2583#" title="Author self-approved">elfotografo007</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign mattmoor for approval by writing `/assign @mattmoor` in a comment. For more information see:[The Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[webhook/OWNERS](https://github.com/knative/pkg/blob/main/webhook/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
knative-prow[bot]: Welcome @elfotografo007! It looks like this is your first PR to knative/pkg 🎉
knative-prow[bot]: Hi @elfotografo007. Thanks for your PR.

I'm waiting for a [knative](https://github.com/orgs/knative/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/knative/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

vaikas: Sweet!
knative-prow-robot: @elfotografo007: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

Patch:
diff --git a/webhook/admission.go b/webhook/admission.go
index 857987ad3..d7e241343 100644
--- a/webhook/admission.go
+++ b/webhook/admission.go
@@ -17,9 +17,11 @@ limitations under the License.
 package webhook
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"strings"
 	"time"
@@ -91,10 +93,12 @@ func admissionHandler(rootLogger *zap.SugaredLogger, stats StatsReporter, c Admi
 		logger.Infof("Webhook ServeHTTP request=%#v", r)
 
 		var review admissionv1.AdmissionReview
-		if err := json.NewDecoder(r.Body).Decode(&review); err != nil {
+		bodyBuffer := bytes.Buffer{}
+		if err := json.NewDecoder(io.TeeReader(r.Body, &bodyBuffer)).Decode(&review); err != nil {
 			http.Error(w, fmt.Sprint("could not decode body:", err), http.StatusBadRequest)
 			return
 		}
+		r.Body = io.NopCloser(&bodyBuffer)
 
 		logger = logger.With(
 			logkey.Kind, review.Request.Kind.String(),
diff --git a/webhook/resourcesemantics/interface.go b/webhook/resourcesemantics/interface.go
index 7f216b101..aa989894d 100644
--- a/webhook/resourcesemantics/interface.go
+++ b/webhook/resourcesemantics/interface.go
@@ -17,6 +17,7 @@ limitations under the License.
 package resourcesemantics
 
 import (
+	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"knative.dev/pkg/apis"
 )
@@ -28,3 +29,22 @@ type GenericCRD interface {
 	apis.Validatable
 	runtime.Object
 }
+
+// VerbLimited defines which Verbs you want to have the webhook invoked on.
+type VerbLimited interface {
+	// SupportedVerbs define which operations (verbs) webhook is called on.
+	SupportedVerbs() []admissionregistrationv1.OperationType
+}
+
+// SubResourceLimited defines which subresources you want to have the webhook
+// invoked on. For example "status", "scale", etc.
+type SubResourceLimited interface {
+	// SupportedSubResources are the subresources that will be registered
+	// for the resource validation.
+	// If you wanted to add for example scale validation for Deployments, you'd
+	// do:
+	// []string{"", "/status", "/scale"}
+	// And to get just the main resource, you would do:
+	// []string{""}
+	SupportedSubResources() []string
+}
diff --git a/webhook/resourcesemantics/validation/controller.go b/webhook/resourcesemantics/validation/controller.go
index 02e47032b..8e8e008e6 100644
--- a/webhook/resourcesemantics/validation/controller.go
+++ b/webhook/resourcesemantics/validation/controller.go
@@ -35,34 +35,21 @@ import (
 	"knative.dev/pkg/webhook/resourcesemantics"
 )
 
-// NewAdmissionController constructs a reconciler
-func NewAdmissionController(
+// NewAdmissionControllerWithConfig constructs a reconciler and registers the
+// provided handlers with specified verbs and SubResources
+func NewAdmissionControllerWithConfig(
 	ctx context.Context,
 	name, path string,
 	handlers map[schema.GroupVersionKind]resourcesemantics.GenericCRD,
 	wc func(context.Context) context.Context,
 	disallowUnknownFields bool,
-	callbacks ...map[schema.GroupVersionKind]Callback,
+	callbacks map[schema.GroupVersionKind]Callback,
 ) *controller.Impl {
-
 	client := kubeclient.Get(ctx)
 	vwhInformer := vwhinformer.Get(ctx)
 	secretInformer := secretinformer.Get(ctx)
 	options := webhook.GetOptions(ctx)
 
-	// This not ideal, we are using a variadic argument to effectively make callbacks optional
-	// This allows this addition to be non-breaking to consumers of /pkg
-	// TODO: once all sub-repos have adopted this, we might move this back to a traditional param.
-	var unwrappedCallbacks map[schema.GroupVersionKind]Callback
-	switch len(callbacks) {
-	case 0:
-		unwrappedCallbacks = map[schema.GroupVersionKind]Callback{}
-	case 1:
-		unwrappedCallbacks = callbacks[0]
-	default:
-		panic("NewAdmissionController may not be called with multiple callback maps")
-	}
-
 	wh := &reconciler{
 		LeaderAwareFuncs: pkgreconciler.LeaderAwareFuncs{
 			// Have this reconciler enqueue our singleton whenever it becomes leader.
@@ -77,7 +64,7 @@ func NewAdmissionController(
 		},
 		path:      path,
 		handlers:  handlers,
-		callbacks: unwrappedCallbacks,
+		callbacks: callbacks,
 
 		withContext:           wc,
 		disallowUnknownFields: disallowUnknownFields,
@@ -109,4 +96,29 @@ func NewAdmissionController(
 	})
 
 	return c
+
+}
+
+// NewAdmissionController constructs a reconciler
+func NewAdmissionController(
+	ctx context.Context,
+	name, path string,
+	handlers map[schema.GroupVersionKind]resourcesemantics.GenericCRD,
+	wc func(context.Context) context.Context,
+	disallowUnknownFields bool,
+	callbacks ...map[schema.GroupVersionKind]Callback,
+) *controller.Impl {
+	// This not ideal, we are using a variadic argument to effectively make callbacks optional
+	// This allows this addition to be non-breaking to consumers of /pkg
+	// TODO: once all sub-repos have adopted this, we might move this back to a traditional param.
+	var unwrappedCallbacks map[schema.GroupVersionKind]Callback
+	switch len(callbacks) {
+	case 0:
+		unwrappedCallbacks = map[schema.GroupVersionKind]Callback{}
+	case 1:
+		unwrappedCallbacks = callbacks[0]
+	default:
+		panic("NewAdmissionController may not be called with multiple callback maps")
+	}
+	return NewAdmissionControllerWithConfig(ctx, name, path, handlers, wc, disallowUnknownFields, unwrappedCallbacks)
 }
diff --git a/webhook/resourcesemantics/validation/reconcile_config.go b/webhook/resourcesemantics/validation/reconcile_config.go
index 0078b61ce..a838351c8 100644
--- a/webhook/resourcesemantics/validation/reconcile_config.go
+++ b/webhook/resourcesemantics/validation/reconcile_config.go
@@ -100,23 +100,54 @@ func (ac *reconciler) reconcileValidatingWebhook(ctx context.Context, caCert []b
 	logger := logging.FromContext(ctx)
 
 	rules := make([]admissionregistrationv1.RuleWithOperations, 0, len(ac.handlers))
-	for gvk := range ac.handlers {
+	for gvk, config := range ac.handlers {
 		plural := strings.ToLower(flect.Pluralize(gvk.Kind))
 
+		// If SupportedVerbs has not been given, provide the legacy defaults
+		// of Create, Update, and Delete
+		supportedVerbs := []admissionregistrationv1.OperationType{
+			admissionregistrationv1.Create,
+			admissionregistrationv1.Update,
+			admissionregistrationv1.Delete,
+		}
+
+		if vl, ok := config.(resourcesemantics.VerbLimited); ok {
+			logging.FromContext(ctx).Debugf("Using custom Verbs")
+			supportedVerbs = vl.SupportedVerbs()
+		}
+		logging.FromContext(ctx).Debugf("Registering verbs: %s", supportedVerbs)
+
+		resources := []string{}
+		// If SupportedSubResources has not been given, provide the legacy
+		// defaults of main resource, and status
+		if srl, ok := config.(resourcesemantics.SubResourceLimited); ok {
+			logging.FromContext(ctx).Debugf("Using custom SubResources")
+			for _, subResource := range srl.SupportedSubResources() {
+				if subResource == "" {
+					// Special case the actual plural if given
+					resources = append(resources, plural)
+				} else {
+					resources = append(resources, plural+subResource)
+				}
+			}
+		} else {
+			resources = append(resources, plural, plural+"/status")
+		}
+		logging.FromContext(ctx).Debugf("Registering SubResources: %s", resources)
 		rules = append(rules, admissionregistrationv1.RuleWithOperations{
-			Operations: []admissionregistrationv1.OperationType{
-				admissionregistrationv1.Create,
-				admissionregistrationv1.Update,
-				admissionregistrationv1.Delete,
-			},
+			Operations: supportedVerbs,
 			Rule: admissionregistrationv1.Rule{
 				APIGroups:   []string{gvk.Group},
 				APIVersions: []string{gvk.Version},
-				Resources:   []string{plural, plural + "/status"},
+				Resources:   resources,
 			},
 		})
 	}
 
+	for _, r := range rules {
+		logging.FromContext(ctx).Debugf("Rule: %+v", r)
+	}
+
 	// Sort the rules by Group, Version, Kind so that things are deterministically ordered.
 	sort.Slice(rules, func(i, j int) bool {
 		lhs, rhs := rules[i], rules[j]
diff --git a/webhook/resourcesemantics/validation/validation_admit.go b/webhook/resourcesemantics/validation/validation_admit.go
index 8ac55e805..2b05a7cd0 100644
--- a/webhook/resourcesemantics/validation/validation_admit.go
+++ b/webhook/resourcesemantics/validation/validation_admit.go
@@ -41,7 +41,6 @@ type Callback struct {
 	function func(ctx context.Context, unstructured *unstructured.Unstructured) error
 
 	// supportedVerbs are the verbs supported for the callback.
-	// The function will only be called on these actions.
 	supportedVerbs map[webhook.Operation]struct{}
 }
 


Test patch:
diff --git a/webhook/admission_integration_test.go b/webhook/admission_integration_test.go
index 8e143c918..caf176097 100644
--- a/webhook/admission_integration_test.go
+++ b/webhook/admission_integration_test.go
@@ -60,6 +60,32 @@ func (fac *fixedAdmissionController) Admit(ctx context.Context, req *admissionv1
 	return fac.response
 }
 
+type readBodyTwiceAdmissionController struct {
+	path     string
+	response *admissionv1.AdmissionResponse
+}
+
+var _ AdmissionController = (*readBodyTwiceAdmissionController)(nil)
+
+func (rbtac *readBodyTwiceAdmissionController) Path() string {
+	return rbtac.path
+}
+
+func (rbtac *readBodyTwiceAdmissionController) Admit(ctx context.Context, req *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse {
+	r := apis.GetHTTPRequest(ctx)
+	if r == nil {
+		panic("nil request!")
+	} else if r.URL.Path != rbtac.path {
+		panic("wrong path!")
+	}
+
+	var review admissionv1.AdmissionReview
+	if err := json.NewDecoder(r.Body).Decode(&review); err != nil {
+		panic("body closed!")
+	}
+	return rbtac.response
+}
+
 func TestAdmissionEmptyRequestBody(t *testing.T) {
 	c := &fixedAdmissionController{
 		path:     "/bazinga",
@@ -547,3 +573,128 @@ func TestAdmissionWarningResponseForResource(t *testing.T) {
 		}
 	}
 }
+
+func TestAdmissionValidResponseForRequestBody(t *testing.T) {
+	ac := &readBodyTwiceAdmissionController{
+		path:     "/bazinga",
+		response: &admissionv1.AdmissionResponse{},
+	}
+	wh, serverURL, ctx, cancel, err := testSetupNoTLS(t, ac)
+	if err != nil {
+		t.Fatal("testSetup() =", err)
+	}
+
+	eg, _ := errgroup.WithContext(ctx)
+	eg.Go(func() error { return wh.Run(ctx.Done()) })
+	defer func() {
+		cancel()
+		if err := eg.Wait(); err != nil {
+			t.Error("Unable to run controller:", err)
+		}
+	}()
+
+	pollErr := waitForServerAvailable(t, serverURL, testTimeout)
+	if pollErr != nil {
+		t.Fatal("waitForServerAvailable() =", err)
+	}
+	client := createNonTLSClient()
+
+	admissionreq := &admissionv1.AdmissionRequest{
+		Operation: admissionv1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+	}
+	testRev := createResource("testrev")
+	marshaled, err := json.Marshal(testRev)
+	if err != nil {
+		t.Fatal("Failed to marshal resource:", err)
+	}
+
+	admissionreq.Resource.Group = "pkg.knative.dev"
+	admissionreq.Object.Raw = marshaled
+	rev := &admissionv1.AdmissionReview{
+		Request: admissionreq,
+	}
+
+	reqBuf := new(bytes.Buffer)
+	err = json.NewEncoder(reqBuf).Encode(&rev)
+	if err != nil {
+		t.Fatal("Failed to marshal admission review:", err)
+	}
+
+	u, err := url.Parse("http://" + serverURL)
+	if err != nil {
+		t.Fatal("bad url", err)
+	}
+
+	u.Path = path.Join(u.Path, ac.Path())
+
+	req, err := http.NewRequest("GET", u.String(), reqBuf)
+	if err != nil {
+		t.Fatal("http.NewRequest() =", err)
+	}
+	req.Header.Add("Content-Type", "application/json")
+
+	doneCh := make(chan struct{})
+	launchedCh := make(chan struct{})
+	go func() {
+		defer close(doneCh)
+
+		close(launchedCh)
+		response, err := client.Do(req)
+		if err != nil {
+			t.Error("Failed to get response", err)
+			return
+		}
+
+		if got, want := response.StatusCode, http.StatusOK; got != want {
+			t.Errorf("Response status code = %v, wanted %v", got, want)
+			return
+		}
+
+		defer response.Body.Close()
+		responseBody, err := ioutil.ReadAll(response.Body)
+		if err != nil {
+			t.Error("Failed to read response body", err)
+			return
+		}
+
+		reviewResponse := admissionv1.AdmissionReview{}
+
+		err = json.NewDecoder(bytes.NewReader(responseBody)).Decode(&reviewResponse)
+		if err != nil {
+			t.Error("Failed to decode response:", err)
+			return
+		}
+
+		if diff := cmp.Diff(rev.TypeMeta, reviewResponse.TypeMeta); diff != "" {
+			t.Errorf("expected the response typeMeta to be the same as the request (-want, +got)\n%s", diff)
+			return
+		}
+	}()
+
+	// Wait for the goroutine to launch.
+	<-launchedCh
+
+	// Check that Admit calls block when they are initiated before informers sync.
+	select {
+	case <-time.After(100 * time.Millisecond):
+	case <-doneCh:
+		t.Fatal("Admit was called before informers had synced.")
+	}
+
+	// Signal the webhook that informers have synced.
+	wh.InformersHaveSynced()
+
+	// Check that after informers have synced that things start completing immediately (including outstanding requests).
+	select {
+	case <-doneCh:
+	case <-time.After(5 * time.Second):
+		t.Error("Timed out waiting on Admit to complete after informers synced.")
+	}
+
+	metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName)
+}
diff --git a/webhook/resourcesemantics/validation/validation_admit_test.go b/webhook/resourcesemantics/validation/validation_admit_test.go
index b63e5788d..1a1bf51eb 100644
--- a/webhook/resourcesemantics/validation/validation_admit_test.go
+++ b/webhook/resourcesemantics/validation/validation_admit_test.go
@@ -685,7 +685,7 @@ func newTestResourceAdmissionController(t *testing.T) webhook.AdmissionControlle
 		SecretName: "webhook-secret",
 	})
 
-	c := NewAdmissionController(
+	c := NewAdmissionControllerWithConfig(
 		ctx, testResourceValidationName, testResourceValidationPath,
 		handlers,
 		func(ctx context.Context) context.Context {