Repository: knative/pkg (go)

Issue:
[webhook] allow webhooks to opt out of validating status updates
ie. serving doesn't have validation logic for the `status` block since we expect only our controllers update it. But our webhooks are invoked for status updates

https://github.com/knative/pkg/blob/132b1e71088b721a05f447b18e4459ffe8828169/webhook/resourcesemantics/defaulting/defaulting.go#L148-L152






Pull request:
Fix #2115, allow selective verbs / subresources for webhooks.
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

<!-- Thanks for sending a pull request! -->

# Changes
Currently when we create the validatingwebhookconfiguration, there's couple of wonky things with it:
1. Always includes status as a subresource and there are cases where we do not need it (#2115 )
2. Always includes all the verbs in the configuration even though they will later on get filtered, but the call to webhook is still made.

Also the API is widely used so I wanted to investigate what could be a smallest change using what we currently have to make this configurable.

So, the idea is that we have a way to customize additional validation logic via `Callbacks`, so if we utilize that struct we do not have to change the signature, but can piggyback off of it to provide ways to customize the actual webhook configuration without breaking any existing code.

Anyways, wanted to just put the smallest amount of code to see what folks think. obvs, need to write tests, etc. but just wanted to see what folks think before going too much deeper.

<!-- 
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
-->

-
-
-

<!--
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 <kind>

<!--
*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 -->

**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

```


Patch:
diff --git a/OWNERS_ALIASES b/OWNERS_ALIASES
index 196409e33..ea395ff56 100644
--- a/OWNERS_ALIASES
+++ b/OWNERS_ALIASES
@@ -29,7 +29,6 @@ aliases:
   - snneji
   docs-wg-leads:
   - abrennan89
-  - csantanapr
   - snneji
   docs-writers:
   - abrennan89
@@ -205,9 +204,7 @@ aliases:
   - spencerdillard
   ux-wg-leads:
   - abrennan89
-  - csantanapr
   - snneji
   ux-writers:
   - abrennan89
-  - csantanapr
   - snneji
diff --git a/go.mod b/go.mod
index ef30b683b..b61ae6c93 100644
--- a/go.mod
+++ b/go.mod
@@ -49,7 +49,7 @@ require (
 	k8s.io/code-generator v0.23.9
 	k8s.io/gengo v0.0.0-20220613173612-397b4ae3bce7
 	k8s.io/klog/v2 v2.70.2-0.20220707122935-0990e81f1a8f
-	knative.dev/hack v0.0.0-20220815132133-e9a8475f4329
+	knative.dev/hack v0.0.0-20220823140917-8d1e4ccf9dc3
 	sigs.k8s.io/yaml v1.3.0
 )
 
diff --git a/go.sum b/go.sum
index 6de5574bd..c36434c54 100644
--- a/go.sum
+++ b/go.sum
@@ -1181,8 +1181,8 @@ k8s.io/utils v0.0.0-20210802155522-efc7438f0176/go.mod h1:jPW/WVKK9YHAvNhRxK0md/
 k8s.io/utils v0.0.0-20211116205334-6203023598ed/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
 k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 h1:HNSDgDCrr/6Ly3WEGKZftiE7IY19Vz2GdbOCyI4qqhc=
 k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
-knative.dev/hack v0.0.0-20220815132133-e9a8475f4329 h1:/X969GuiLDMgb+tyx5qZDLxGhENkS3OLPj/VCSW+Ul0=
-knative.dev/hack v0.0.0-20220815132133-e9a8475f4329/go.mod h1:t/azP8I/Cygaw+87O7rkAPrNRjCelmtfSzWzu/9TM7I=
+knative.dev/hack v0.0.0-20220823140917-8d1e4ccf9dc3 h1:umaeMRecA0g5g48L9tnEAkTBIitr9eKWMyJYo9YttAA=
+knative.dev/hack v0.0.0-20220823140917-8d1e4ccf9dc3/go.mod h1:t/azP8I/Cygaw+87O7rkAPrNRjCelmtfSzWzu/9TM7I=
 pgregory.net/rapid v0.3.3 h1:jCjBsY4ln4Atz78QoBWxUEvAHaFyNDQg9+WU62aCn1U=
 pgregory.net/rapid v0.3.3/go.mod h1:UYpPVyjFHzYBGHIxLFoupi8vwk6rXNzRY9OMvVxFIOU=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
diff --git a/vendor/knative.dev/hack/release.sh b/vendor/knative.dev/hack/release.sh
index 292862ee4..6944310ac 100644
--- a/vendor/knative.dev/hack/release.sh
+++ b/vendor/knative.dev/hack/release.sh
@@ -107,7 +107,8 @@ export GITHUB_TOKEN=""
 # Convenience function to run the hub tool.
 # Parameters: $1..$n - arguments to hub.
 function hub_tool() {
-  gorun github.com/github/hub@v2.14.2 "$@"
+  # Pinned to SHA because of https://github.com/github/hub/issues/2517
+  go_run github.com/github/hub/v2@363513a "$@"
 }
 
 # Shortcut to "git push" that handles authentication.
diff --git a/vendor/modules.txt b/vendor/modules.txt
index 065c883af..09569d0f4 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -1088,7 +1088,7 @@ k8s.io/utils/internal/third_party/forked/golang/net
 k8s.io/utils/net
 k8s.io/utils/pointer
 k8s.io/utils/trace
-# knative.dev/hack v0.0.0-20220815132133-e9a8475f4329
+# knative.dev/hack v0.0.0-20220823140917-8d1e4ccf9dc3
 ## explicit; go 1.17
 knative.dev/hack
 # sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6
diff --git a/webhook/admission.go b/webhook/admission.go
index ea5518260..857987ad3 100644
--- a/webhook/admission.go
+++ b/webhook/admission.go
@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"strings"
 	"time"
 
 	"go.uber.org/zap"
@@ -124,6 +125,18 @@ func admissionHandler(rootLogger *zap.SugaredLogger, stats StatsReporter, c Admi
 		if !reviewResponse.Allowed || reviewResponse.PatchType != nil || response.Response == nil {
 			response.Response = reviewResponse
 		}
+
+		// If warnings contain newlines, which they will do by default if
+		// using Knative apis.FieldError, split them based on newlines
+		// and create a new warning. This is because any control characters
+		// in the warnings will cause the warning to be dropped silently.
+		if reviewResponse.Warnings != nil {
+			cleanedWarnings := make([]string, 0, len(reviewResponse.Warnings))
+			for _, w := range reviewResponse.Warnings {
+				cleanedWarnings = append(cleanedWarnings, strings.Split(w, "\n")...)
+			}
+			reviewResponse.Warnings = cleanedWarnings
+		}
 		response.Response.UID = review.Request.UID
 
 		logger = logger.With(
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 66a73c483..8e143c918 100644
--- a/webhook/admission_integration_test.go
+++ b/webhook/admission_integration_test.go
@@ -437,3 +437,113 @@ func TestAdmissionInvalidResponseForResource(t *testing.T) {
 	// Stats should be reported for requests that have admission disallowed
 	metricstest.CheckStatsReported(t, requestCountName, requestLatenciesName)
 }
+
+func TestAdmissionWarningResponseForResource(t *testing.T) {
+	// Test that our single warning below (with newlines) should be turned into
+	// these three warnings
+	expectedWarnings := []string{"everything is not fine.", "like really", "for sure"}
+	ac := &fixedAdmissionController{
+		path:     "/warnmeplease",
+		response: &admissionv1.AdmissionResponse{Warnings: []string{"everything is not fine.\nlike really\nfor sure"}},
+	}
+	wh, serverURL, ctx, cancel, err := testSetup(t, ac)
+	if err != nil {
+		t.Fatal("testSetup() =", err)
+	}
+
+	eg, _ := errgroup.WithContext(ctx)
+	eg.Go(func() error { return wh.Run(ctx.Done()) })
+	wh.InformersHaveSynced()
+	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)
+	}
+	tlsClient, err := createSecureTLSClient(t, kubeclient.Get(ctx), &wh.Options)
+	if err != nil {
+		t.Fatal("createSecureTLSClient() =", err)
+	}
+
+	resource := createResource(testResourceName)
+
+	marshaled, err := json.Marshal(resource)
+	if err != nil {
+		t.Fatal("Failed to marshal resource:", err)
+	}
+
+	admissionreq := &admissionv1.AdmissionRequest{
+		Operation: admissionv1.Create,
+		Kind: metav1.GroupVersionKind{
+			Group:   "pkg.knative.dev",
+			Version: "v1alpha1",
+			Kind:    "Resource",
+		},
+		UserInfo: authenticationv1.UserInfo{
+			Username: user1,
+		},
+	}
+
+	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("https://" + 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")
+
+	response, err := tlsClient.Do(req)
+	if err != nil {
+		t.Fatal("Failed to receive response", err)
+	}
+
+	if got, want := response.StatusCode, http.StatusOK; got != want {
+		t.Errorf("Response status code = %v, wanted %v", got, want)
+	}
+
+	defer response.Body.Close()
+	respBody, err := ioutil.ReadAll(response.Body)
+	if err != nil {
+		t.Fatal("Failed to read response body", err)
+	}
+
+	reviewResponse := admissionv1.AdmissionReview{}
+
+	err = json.NewDecoder(bytes.NewReader(respBody)).Decode(&reviewResponse)
+	if err != nil {
+		t.Fatal("Failed to decode response:", err)
+	}
+
+	warnings := reviewResponse.Response.Warnings
+	if len(warnings) != 3 {
+		t.Errorf("Received unexpected warnings, wanted 3 got: %s", reviewResponse.Response.Warnings)
+	}
+	for i, w := range warnings {
+		if expectedWarnings[i] != w {
+			t.Errorf("Unexpected warning want %s got %s", expectedWarnings[i], w)
+		}
+	}
+}
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 {