Move hook-specific configuration options out of shared options. (#818)

This builds on https://github.com/knative/pkg/pull/817 and makes further
breaking changes. The options pertinent to each admission controller are
now passed to their respective constructors, which leads to a cleaner
options struct, and better prepares for greater webhook diversity.

webhook/config_validation_controller.go

@@ -38,17 +38,21 @@ import (
 
 // ConfigValidationController implements the AdmissionController for ConfigMaps
 type ConfigValidationController struct {
+	// name of the ValidatingWebhookConfiguration
+	name string
+	// path that the webhook should serve on
+	path         string
 	constructors map[string]reflect.Value
-	options      ControllerOptions
 }
 
 // NewConfigValidationController constructs a ConfigValidationController
 func NewConfigValidationController(
-	constructors configmap.Constructors,
-	opts ControllerOptions) AdmissionController {
+	name, path string,
+	constructors configmap.Constructors) AdmissionController {
 	cfgValidations := &ConfigValidationController{
+		name:         name,
+		path:         path,
 		constructors: make(map[string]reflect.Value),
-		options:      opts,
 	}
 
 	for configName, constructor := range constructors {
@@ -94,7 +98,7 @@ func (ac *ConfigValidationController) Register(ctx context.Context, kubeClient k
 		},
 	}}
 
-	configuredWebhook, err := client.Get(ac.options.ConfigValidationWebhookName, metav1.GetOptions{})
+	configuredWebhook, err := client.Get(ac.name, metav1.GetOptions{})
 	if err != nil {
 		return fmt.Errorf("error retrieving webhook: %v", err)
 	}
@@ -114,7 +118,7 @@ func (ac *ConfigValidationController) Register(ctx context.Context, kubeClient k
 		if webhook.Webhooks[i].ClientConfig.Service == nil {
 			return fmt.Errorf("missing service reference for webhook: %s", wh.Name)
 		}
-		webhook.Webhooks[i].ClientConfig.Service.Path = ptr.String(ac.options.ConfigValidationControllerPath)
+		webhook.Webhooks[i].ClientConfig.Service.Path = ptr.String(ac.path)
 	}
 
 	if ok, err := kmp.SafeEqual(configuredWebhook, webhook); err != nil {

webhook/config_validation_controller_test.go

@@ -39,13 +39,18 @@ import (
 	. "knative.dev/pkg/logging/testing"
 )
 
+const (
+	testConfigValidationName = "configmap.webhook.knative.dev"
+	testConfigValidationPath = "/cm"
+)
+
 var (
 	initialConfigWebhook = &admissionregistrationv1beta1.ValidatingWebhookConfiguration{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "configmap.webhook.knative.dev",
+			Name: testConfigValidationName,
 		},
 		Webhooks: []admissionregistrationv1beta1.ValidatingWebhook{{
-			Name: "configmap.webhook.knative.dev",
+			Name: testConfigValidationName,
 			ClientConfig: admissionregistrationv1beta1.WebhookClientConfig{
 				Service: &admissionregistrationv1beta1.ServiceReference{
 					Namespace: system.Namespace(),
@@ -62,24 +67,24 @@ var (
 	}
 )
 
-func newNonRunningTestConfigValidationController(t *testing.T, options ControllerOptions) (
+func newNonRunningTestConfigValidationController(t *testing.T) (
 	kubeClient *fakekubeclientset.Clientset,
 	ac AdmissionController) {
 	t.Helper()
 	// Create fake clients
 	kubeClient = fakekubeclientset.NewSimpleClientset(initialConfigWebhook)
 
-	ac = NewTestConfigValidationController(options)
+	ac = NewTestConfigValidationController()
 	return
 }
 
-func NewTestConfigValidationController(options ControllerOptions) AdmissionController {
+func NewTestConfigValidationController() AdmissionController {
 	validations := configmap.Constructors{"test-config": newConfigFromConfigMap}
-	return NewConfigValidationController(validations, options)
+	return NewConfigValidationController(testConfigValidationName, testConfigValidationPath, validations)
 }
 
 func TestUpdatingConfigValidationController(t *testing.T) {
-	kubeClient, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	kubeClient, ac := newNonRunningTestConfigValidationController(t)
 
 	createDeployment(kubeClient)
 	err := ac.Register(TestContextWithLogger(t), kubeClient, []byte{})
@@ -98,7 +103,7 @@ func TestUpdatingConfigValidationController(t *testing.T) {
 }
 
 func TestDeleteAllowedForConfigMap(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	req := &admissionv1beta1.AdmissionRequest{
 		Operation: admissionv1beta1.Delete,
@@ -110,7 +115,7 @@ func TestDeleteAllowedForConfigMap(t *testing.T) {
 }
 
 func TestConnectAllowedForConfigMap(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	req := &admissionv1beta1.AdmissionRequest{
 		Operation: admissionv1beta1.Connect,
@@ -123,7 +128,7 @@ func TestConnectAllowedForConfigMap(t *testing.T) {
 }
 
 func TestNonConfigMapKindFails(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	req := &admissionv1beta1.AdmissionRequest{
 		Operation: admissionv1beta1.Create,
@@ -138,7 +143,7 @@ func TestNonConfigMapKindFails(t *testing.T) {
 }
 
 func TestAdmitCreateValidConfigMap(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	r := createValidConfigMap()
 	ctx := apis.WithinCreate(apis.WithUserInfo(
@@ -151,7 +156,7 @@ func TestAdmitCreateValidConfigMap(t *testing.T) {
 }
 
 func TestDenyInvalidCreateConfigMapWithWrongType(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	r := createWrongTypeConfigMap()
 	ctx := apis.WithinCreate(apis.WithUserInfo(
@@ -164,7 +169,7 @@ func TestDenyInvalidCreateConfigMapWithWrongType(t *testing.T) {
 }
 
 func TestDenyInvalidCreateConfigMapOutOfRange(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	r := createWrongValueConfigMap()
 	ctx := apis.WithinCreate(apis.WithUserInfo(
@@ -177,7 +182,7 @@ func TestDenyInvalidCreateConfigMapOutOfRange(t *testing.T) {
 }
 
 func TestAdmitUpdateValidConfigMap(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	r := createValidConfigMap()
 	ctx := apis.WithinCreate(apis.WithUserInfo(
@@ -190,7 +195,7 @@ func TestAdmitUpdateValidConfigMap(t *testing.T) {
 }
 
 func TestDenyInvalidUpdateConfigMapWithWrongType(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	r := createWrongTypeConfigMap()
 	ctx := apis.WithinCreate(apis.WithUserInfo(
@@ -203,7 +208,7 @@ func TestDenyInvalidUpdateConfigMapWithWrongType(t *testing.T) {
 }
 
 func TestDenyInvalidUpdateConfigMapOutOfRange(t *testing.T) {
-	_, ac := newNonRunningTestConfigValidationController(t, newDefaultOptions())
+	_, ac := newNonRunningTestConfigValidationController(t)
 
 	r := createWrongValueConfigMap()
 	ctx := apis.WithinCreate(apis.WithUserInfo(

webhook/resource_admission_controller.go

@@ -42,16 +42,6 @@ import (
 	"knative.dev/pkg/ptr"
 )
 
-// ResourceCallback defines a signature for resource specific (Route, Configuration, etc.)
-// handlers that can validate and mutate an object. If non-nil error is returned, object mutation
-// is denied. Mutations should be appended to the patches operations.
-type ResourceCallback func(patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error
-
-// ResourceDefaulter defines a signature for resource specific (Route, Configuration, etc.)
-// handlers that can set defaults on an object. If non-nil error is returned, object mutation
-// is denied. Mutations should be appended to the patches operations.
-type ResourceDefaulter func(patches *[]jsonpatch.JsonPatchOperation, crd GenericCRD) error
-
 // 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 {
@@ -62,20 +52,24 @@ type GenericCRD interface {
 
 // ResourceAdmissionController implements the AdmissionController for resources
 type ResourceAdmissionController struct {
+	// name of the MutatingWebhookConfiguration
+	name string
+	// path that the webhook should serve on
+	path     string
 	handlers map[schema.GroupVersionKind]GenericCRD
-	options  ControllerOptions
 
 	disallowUnknownFields bool
 }
 
 // NewResourceAdmissionController constructs a ResourceAdmissionController
 func NewResourceAdmissionController(
+	name, path string,
 	handlers map[schema.GroupVersionKind]GenericCRD,
-	opts ControllerOptions,
 	disallowUnknownFields bool) AdmissionController {
 	return &ResourceAdmissionController{
+		name:                  name,
+		path:                  path,
 		handlers:              handlers,
-		options:               opts,
 		disallowUnknownFields: disallowUnknownFields,
 	}
 }
@@ -138,7 +132,7 @@ func (ac *ResourceAdmissionController) Register(ctx context.Context, kubeClient
 		return lhs.Resources[0] < rhs.Resources[0]
 	})
 
-	configuredWebhook, err := client.Get(ac.options.ResourceMutatingWebhookName, metav1.GetOptions{})
+	configuredWebhook, err := client.Get(ac.name, metav1.GetOptions{})
 	if err != nil {
 		return fmt.Errorf("error retrieving webhook: %v", err)
 	}
@@ -158,8 +152,7 @@ func (ac *ResourceAdmissionController) Register(ctx context.Context, kubeClient
 		if webhook.Webhooks[i].ClientConfig.Service == nil {
 			return fmt.Errorf("missing service reference for webhook: %s", wh.Name)
 		}
-		webhook.Webhooks[i].ClientConfig.Service.Path = ptr.String(
-			ac.options.ResourceAdmissionControllerPath)
+		webhook.Webhooks[i].ClientConfig.Service.Path = ptr.String(ac.path)
 	}
 
 	if ok, err := kmp.SafeEqual(configuredWebhook, webhook); err != nil {

webhook/resource_admission_controller_deprecated_test.go

@@ -504,7 +504,7 @@ func TestStrictValidation(t *testing.T) {
 				ctx = apis.DisallowDeprecated(ctx)
 			}
 
-			_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+			_, ac := newNonRunningTestResourceAdmissionController(t)
 			resp := ac.Admit(ctx, tc.req)
 
 			if len(tc.wantErrs) > 0 {
@@ -534,7 +534,7 @@ func TestStrictValidation_Spec_Create(t *testing.T) {
 
 	ctx := apis.DisallowDeprecated(TestContextWithLogger(t))
 
-	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t)
 	resp := ac.Admit(ctx, req)
 
 	expectFailsWith(t, resp, "must not set")
@@ -558,7 +558,7 @@ func TestStrictValidation_Spec_Update(t *testing.T) {
 
 	ctx := apis.DisallowDeprecated(TestContextWithLogger(t))
 
-	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t)
 	resp := ac.Admit(ctx, req)
 
 	expectFailsWith(t, resp, "must not update")

webhook/resource_admission_controller_test.go

@@ -44,6 +44,11 @@ import (
 	. "knative.dev/pkg/testing"
 )
 
+const (
+	testResourceValidationPath = "/foo"
+	testResourceValidationName = "webhook.knative.dev"
+)
+
 var (
 	handlers = map[schema.GroupVersionKind]GenericCRD{
 		{
@@ -87,7 +92,7 @@ var (
 	}
 )
 
-func newNonRunningTestResourceAdmissionController(t *testing.T, options ControllerOptions) (
+func newNonRunningTestResourceAdmissionController(t *testing.T) (
 	kubeClient *fakekubeclientset.Clientset,
 	ac AdmissionController) {
 
@@ -95,12 +100,12 @@ func newNonRunningTestResourceAdmissionController(t *testing.T, options Controll
 	// Create fake clients
 	kubeClient = fakekubeclientset.NewSimpleClientset(initialResourceWebhook)
 
-	ac = NewTestResourceAdmissionController(options)
+	ac = NewTestResourceAdmissionController()
 	return
 }
 
 func TestDeleteAllowed(t *testing.T) {
-	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t)
 
 	req := &admissionv1beta1.AdmissionRequest{
 		Operation: admissionv1beta1.Delete,
@@ -112,7 +117,7 @@ func TestDeleteAllowed(t *testing.T) {
 }
 
 func TestConnectAllowed(t *testing.T) {
-	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t)
 
 	req := &admissionv1beta1.AdmissionRequest{
 		Operation: admissionv1beta1.Connect,
@@ -125,7 +130,7 @@ func TestConnectAllowed(t *testing.T) {
 }
 
 func TestUnknownKindFails(t *testing.T) {
-	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t)
 
 	req := &admissionv1beta1.AdmissionRequest{
 		Operation: admissionv1beta1.Create,
@@ -140,7 +145,7 @@ func TestUnknownKindFails(t *testing.T) {
 }
 
 func TestUnknownVersionFails(t *testing.T) {
-	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t)
 	req := &admissionv1beta1.AdmissionRequest{
 		Operation: admissionv1beta1.Create,
 		Kind: metav1.GroupVersionKind{
@@ -153,7 +158,7 @@ func TestUnknownVersionFails(t *testing.T) {
 }
 
 func TestUnknownFieldFails(t *testing.T) {
-	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t)
 	req := &admissionv1beta1.AdmissionRequest{
 		Operation: admissionv1beta1.Create,
 		Kind: metav1.GroupVersionKind{
@@ -303,7 +308,7 @@ func TestAdmitCreates(t *testing.T) {
 			// Setup the resource.
 			tc.setup(ctx, r)
 
-			_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+			_, ac := newNonRunningTestResourceAdmissionController(t)
 			resp := ac.Admit(ctx, createCreateResource(ctx, r))
 
 			if tc.rejection == "" {
@@ -430,7 +435,7 @@ func TestAdmitUpdates(t *testing.T) {
 				&authenticationv1.UserInfo{Username: user2})
 			tc.mutate(ctx, new)
 
-			_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+			_, ac := newNonRunningTestResourceAdmissionController(t)
 			resp := ac.Admit(ctx, createUpdateResource(ctx, old, new))
 
 			if tc.rejection == "" {
@@ -478,7 +483,7 @@ func TestValidCreateResourceSucceedsWithRoundTripAndDefaultPatch(t *testing.T) {
 	}
 	req.Object.Raw = createInnerDefaultResourceWithoutSpec(t)
 
-	_, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	_, ac := newNonRunningTestResourceAdmissionController(t)
 	resp := ac.Admit(TestContextWithLogger(t), req)
 	expectAllowed(t, resp)
 	expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{
@@ -541,7 +546,7 @@ func createInnerDefaultResourceWithSpecAndStatus(t *testing.T, spec *InnerDefaul
 }
 
 func TestUpdatingResourceController(t *testing.T) {
-	kubeClient, ac := newNonRunningTestResourceAdmissionController(t, newDefaultOptions())
+	kubeClient, ac := newNonRunningTestResourceAdmissionController(t)
 
 	createDeployment(kubeClient)
 	err := ac.Register(TestContextWithLogger(t), kubeClient, []byte{})
@@ -636,6 +641,7 @@ func setUserAnnotation(userC, userU string) jsonpatch.JsonPatchOperation {
 	}
 }
 
-func NewTestResourceAdmissionController(options ControllerOptions) AdmissionController {
-	return NewResourceAdmissionController(handlers, options, true)
+func NewTestResourceAdmissionController() AdmissionController {
+	return NewResourceAdmissionController(testResourceValidationName, testResourceValidationPath,
+		handlers, true)
 }

webhook/webhook.go

@@ -51,14 +51,6 @@ var (
 
 // ControllerOptions contains the configuration for the webhook
 type ControllerOptions struct {
-	// ResourceMutatingWebhookName is the name of the webhook we create to handle
-	// mutations before they get stored in the storage.
-	ResourceMutatingWebhookName string
-
-	// ConfigValidationWebhookName is the name of the webhook we create to handle
-	// mutations before they get stored in the storage.
-	ConfigValidationWebhookName string
-
 	// ServiceName is the service name of the webhook.
 	ServiceName string
 
@@ -88,14 +80,6 @@ type ControllerOptions struct {
 	// StatsReporter reports metrics about the webhook.
 	// This will be automatically initialized by the constructor if left uninitialized.
 	StatsReporter StatsReporter
-
-	// Service path for ResourceAdmissionController webhook
-	// Default is "/" for backward compatibility and is set by the constructor
-	ResourceAdmissionControllerPath string
-
-	// Service path for ConfigValidationController webhook
-	// Default is "/config-validation" and is set by the constructor
-	ConfigValidationControllerPath string
 }
 
 // AdmissionController provides the interface for different admission controllers

webhook/webhook_integration_test.go

@@ -197,7 +197,14 @@ func TestValidResponseForResource(t *testing.T) {
 		t.Fatalf("Failed to marshal admission review: %v", err)
 	}
 
-	req, err := http.NewRequest("GET", fmt.Sprintf("https://%s", serverURL), reqBuf)
+	u, err := url.Parse(fmt.Sprintf("https://%s", serverURL))
+	if err != nil {
+		t.Fatalf("bad url %v", err)
+	}
+
+	u.Path = path.Join(u.Path, testResourceValidationPath)
+
+	req, err := http.NewRequest("GET", u.String(), reqBuf)
 	if err != nil {
 		t.Fatalf("http.NewRequest() = %v", err)
 	}
@@ -283,7 +290,14 @@ func TestValidResponseForResourceWithContextDefault(t *testing.T) {
 		t.Fatalf("Failed to marshal admission review: %v", err)
 	}
 
-	req, err := http.NewRequest("GET", fmt.Sprintf("https://%s", serverURL), reqBuf)
+	u, err := url.Parse(fmt.Sprintf("https://%s", serverURL))
+	if err != nil {
+		t.Fatalf("bad url %v", err)
+	}
+
+	u.Path = path.Join(u.Path, testResourceValidationPath)
+
+	req, err := http.NewRequest("GET", u.String(), reqBuf)
 	if err != nil {
 		t.Fatalf("http.NewRequest() = %v", err)
 	}
@@ -384,7 +398,14 @@ func TestInvalidResponseForResource(t *testing.T) {
 		t.Fatalf("Failed to marshal admission review: %v", err)
 	}
 
-	req, err := http.NewRequest("GET", fmt.Sprintf("https://%s", serverURL), reqBuf)
+	u, err := url.Parse(fmt.Sprintf("https://%s", serverURL))
+	if err != nil {
+		t.Fatalf("bad url %v", err)
+	}
+
+	u.Path = path.Join(u.Path, testResourceValidationPath)
+
+	req, err := http.NewRequest("GET", u.String(), reqBuf)
 	if err != nil {
 		t.Fatalf("http.NewRequest() = %v", err)
 	}
@@ -502,7 +523,7 @@ func TestValidResponseForConfigMap(t *testing.T) {
 		t.Fatalf("bad url %v", err)
 	}
 
-	u.Path = path.Join(u.Path, ac.Options.ConfigValidationControllerPath)
+	u.Path = path.Join(u.Path, testConfigValidationPath)
 	req, err := http.NewRequest("GET", u.String(), reqBuf)
 	if err != nil {
 		t.Fatalf("http.NewRequest() = %v", err)
@@ -581,7 +602,7 @@ func TestInvalidResponseForConfigMap(t *testing.T) {
 		t.Fatalf("bad url %v", err)
 	}
 
-	u.Path = path.Join(u.Path, ac.Options.ConfigValidationControllerPath)
+	u.Path = path.Join(u.Path, testConfigValidationPath)
 	req, err := http.NewRequest("GET", u.String(), reqBuf)
 	if err != nil {
 		t.Fatalf("http.NewRequest() = %v", err)

webhook/webhook_test.go

@@ -39,13 +39,9 @@ import (
 
 func newDefaultOptions() ControllerOptions {
 	return ControllerOptions{
-		ServiceName:                     "webhook",
-		Port:                            443,
-		SecretName:                      "webhook-certs",
-		ResourceMutatingWebhookName:     "webhook.knative.dev",
-		ResourceAdmissionControllerPath: "/",
-		ConfigValidationWebhookName:     "configmap.webhook.knative.dev",
-		ConfigValidationControllerPath:  "/config-validation",
+		ServiceName: "webhook",
+		Port:        443,
+		SecretName:  "webhook-certs",
 	}
 }
 
@@ -180,8 +176,10 @@ func NewTestWebhook(client kubernetes.Interface, options ControllerOptions, logg
 	validations := configmap.Constructors{"test-config": newConfigFromConfigMap}
 
 	admissionControllers := map[string]AdmissionController{
-		options.ResourceAdmissionControllerPath: NewResourceAdmissionController(handlers, options, true),
-		options.ConfigValidationControllerPath:  NewConfigValidationController(validations, options),
+		testResourceValidationPath: NewResourceAdmissionController(
+			testResourceValidationName, testResourceValidationPath, handlers, true),
+		testConfigValidationPath: NewConfigValidationController(
+			testConfigValidationName, testConfigValidationPath, validations),
 	}
 	return New(client, options, admissionControllers, logger, nil)
 }