Split apart defaulting and validation webhooks (#133)

Related: knative/pkg#848

cmd/webhook/main.go

@@ -32,25 +32,53 @@ import (
 	"knative.dev/pkg/webhook/certificates"
 	"knative.dev/pkg/webhook/configmaps"
 	"knative.dev/pkg/webhook/resourcesemantics"
+	"knative.dev/pkg/webhook/resourcesemantics/defaulting"
+	"knative.dev/pkg/webhook/resourcesemantics/validation"
 
 	"knative.dev/sample-controller/pkg/apis/samples/v1alpha1"
 )
 
-func NewResourceAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
-	return resourcesemantics.NewAdmissionController(ctx,
+var types = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
+	// List the types to validate.
+	v1alpha1.SchemeGroupVersion.WithKind("AddressableService"): &v1alpha1.AddressableService{},
+}
+
+func NewDefaultingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
+	return defaulting.NewAdmissionController(ctx,
 
 		// Name of the resource webhook.
-		fmt.Sprintf("resources.webhook.%s.knative.dev", system.Namespace()),
+		fmt.Sprintf("defaulting.webhook.%s.knative.dev", system.Namespace()),
 
 		// The path on which to serve the webhook.
-		"/resource-validation",
+		"/defaulting",
 
 		// The resources to validate and default.
-		map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
-			// List the types to validate.
-			v1alpha1.SchemeGroupVersion.WithKind("AddressableService"): &v1alpha1.AddressableService{},
+		types,
+
+		// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
+		func(ctx context.Context) context.Context {
+			// Here is where you would infuse the context with state
+			// (e.g. attach a store with configmap data)
+			return ctx
 		},
 
+		// Whether to disallow unknown fields.
+		true,
+	)
+}
+
+func NewValidationAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
+	return validation.NewAdmissionController(ctx,
+
+		// Name of the resource webhook.
+		fmt.Sprintf("validation.webhook.%s.knative.dev", system.Namespace()),
+
+		// The path on which to serve the webhook.
+		"/resource-validation",
+
+		// The resources to validate and default.
+		types,
+
 		// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
 		func(ctx context.Context) context.Context {
 			// Here is where you would infuse the context with state
@@ -89,7 +117,8 @@ func main() {
 
 	sharedmain.MainWithContext(ctx, "webhook",
 		certificates.NewController,
-		NewResourceAdmissionController,
+		NewDefaultingAdmissionController,
+		NewValidationAdmissionController,
 		NewConfigValidationController,
 	)
 }

config/500-webhook-configuration.yaml

@@ -15,7 +15,7 @@
 apiVersion: admissionregistration.k8s.io/v1beta1
 kind: MutatingWebhookConfiguration
 metadata:
-  name: resources.webhook.knative-samples.knative.dev
+  name: defaulting.webhook.knative-samples.knative.dev
   labels:
     samples.knative.dev/release: devel
 webhooks:
@@ -26,7 +26,23 @@ webhooks:
       name: webhook
       namespace: knative-samples
   failurePolicy: Fail
-  name: resources.webhook.knative-samples.knative.dev
+  name: defaulting.webhook.knative-samples.knative.dev
+---
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: validation.webhook.knative-samples.knative.dev
+  labels:
+    samples.knative.dev/release: devel
+webhooks:
+- admissionReviewVersions:
+  - v1beta1
+  clientConfig:
+    service:
+      name: webhook
+      namespace: knative-samples
+  failurePolicy: Fail
+  name: validation.webhook.knative-samples.knative.dev
 ---
 apiVersion: admissionregistration.k8s.io/v1beta1
 kind: ValidatingWebhookConfiguration

vendor/knative.dev/pkg/apis/interfaces.go

@@ -44,15 +44,6 @@ type Convertible interface {
 	ConvertDown(ctx context.Context, from Convertible) error
 }
 
-// Immutable indicates that a particular type has fields that should
-// not change after creation.
-// DEPRECATED: Use WithinUpdate / GetBaseline from within Validatable instead.
-type Immutable interface {
-	// CheckImmutableFields checks that the current instance's immutable
-	// fields haven't changed from the provided original.
-	CheckImmutableFields(ctx context.Context, original Immutable) *FieldError
-}
-
 // Listable indicates that a particular type can be returned via the returned
 // list type by the API server.
 type Listable interface {

vendor/knative.dev/pkg/injection/sharedmain/main.go

@@ -168,7 +168,7 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
 		metav1.GetOptions{}); err == nil {
 		cmw.Watch(logging.ConfigMapName(), logging.UpdateLevelFromConfigMap(logger, atomicLevel, component))
 	} else if !apierrors.IsNotFound(err) {
-		logger.Fatalw("Error reading ConfigMap: " + logging.ConfigMapName(), zap.Error(err))
+		logger.Fatalw("Error reading ConfigMap: "+logging.ConfigMapName(), zap.Error(err))
 	}
 
 	// Watch the observability config map
@@ -178,7 +178,7 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
 			metrics.UpdateExporterFromConfigMap(component, logger),
 			profilingHandler.UpdateFromConfigMap)
 	} else if !apierrors.IsNotFound(err) {
-		logger.Fatalw("Error reading ConfigMap: " + metrics.ConfigMapName(), zap.Error(err))
+		logger.Fatalw("Error reading ConfigMap: "+metrics.ConfigMapName(), zap.Error(err))
 	}
 
 	if err := cmw.Start(ctx.Done()); err != nil {

vendor/knative.dev/pkg/test/e2e_flags.go

@@ -25,11 +25,27 @@ import (
 	"os"
 	"os/user"
 	"path"
+	"sync"
+
+	"k8s.io/klog"
+	"knative.dev/pkg/test/logging"
+)
+
+const (
+	// e2eMetricExporter is the name for the metrics exporter logger
+	e2eMetricExporter = "e2e-metrics"
+
+	// The recommended default log level https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md
+	klogDefaultLogLevel = "2"
 )
 
-// Flags holds the command line flags or defaults for settings in the user's environment.
-// See EnvironmentFlags for a list of supported fields.
-var Flags = initializeFlags()
+var (
+	flagsSetupOnce = &sync.Once{}
+
+	// Flags holds the command line flags or defaults for settings in the user's environment.
+	// See EnvironmentFlags for a list of supported fields.
+	Flags = initializeFlags()
+)
 
 // EnvironmentFlags define the flags that are needed to run the e2e tests.
 type EnvironmentFlags struct {
@@ -73,9 +89,42 @@ func initializeFlags() *EnvironmentFlags {
 
 	flag.StringVar(&f.Tag, "tag", "latest", "Provide the version tag for the test images.")
 
+	klog.InitFlags(nil)
+	flag.Set("v", klogDefaultLogLevel)
+	flag.Set("alsologtostderr", "true")
+
 	return &f
 }
 
+func printFlags() {
+	fmt.Print("Test Flags: {")
+	flag.CommandLine.VisitAll(func(f *flag.Flag) {
+		fmt.Printf("'%s': '%s', ", f.Name, f.Value.String())
+	})
+	fmt.Println("}")
+}
+
+// SetupLoggingFlags initializes the logging libraries at runtime
+func SetupLoggingFlags() {
+	flagsSetupOnce.Do(func() {
+		if Flags.LogVerbose {
+			// If klog verbosity is not set to a non-default value (via "-args -v=X"),
+			if flag.CommandLine.Lookup("v").Value.String() == klogDefaultLogLevel {
+				// set up verbosity for klog so round_trippers.go prints:
+				//   URL, request headers, response headers, and partial response body
+				// See levels in vendor/k8s.io/client-go/transport/round_trippers.go:DebugWrappers for other options
+				flag.Set("v", "8")
+			}
+			printFlags()
+		}
+		logging.InitializeLogger(Flags.LogVerbose)
+
+		if Flags.EmitMetrics {
+			logging.InitializeMetricExporter(e2eMetricExporter)
+		}
+	})
+}
+
 // ImagePath is a helper function to prefix image name with repo and suffix with tag
 func ImagePath(name string) string {
 	return fmt.Sprintf("%s/%s:%s", Flags.DockerRepo, name, Flags.Tag)

vendor/knative.dev/pkg/testing/resource.go

@@ -38,7 +38,6 @@ type Resource struct {
 // Check that Resource may be validated and defaulted.
 var _ apis.Validatable = (*Resource)(nil)
 var _ apis.Defaultable = (*Resource)(nil)
-var _ apis.Immutable = (*Resource)(nil)
 var _ apis.Listable = (*Resource)(nil)
 
 // ResourceSpec represents test resource spec.
@@ -105,12 +104,7 @@ func (cs *ResourceSpec) Validate(ctx context.Context) *apis.FieldError {
 	return nil
 }
 
-func (current *Resource) CheckImmutableFields(ctx context.Context, og apis.Immutable) *apis.FieldError {
-	original, ok := og.(*Resource)
-	if !ok {
-		return &apis.FieldError{Message: "The provided original was not a Resource"}
-	}
-
+func (current *Resource) CheckImmutableFields(ctx context.Context, original *Resource) *apis.FieldError {
 	if original.Spec.FieldThatsImmutable != current.Spec.FieldThatsImmutable {
 		return &apis.FieldError{
 			Message: "Immutable field changed",

vendor/knative.dev/pkg/webhook/resourcesemantics/controller.go → vendor/knative.dev/pkg/webhook/resourcesemantics/defaulting/controller.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package resourcesemantics
+package defaulting
 
 import (
 	"context"
@@ -30,13 +30,14 @@ import (
 	"knative.dev/pkg/logging"
 	"knative.dev/pkg/system"
 	"knative.dev/pkg/webhook"
+	"knative.dev/pkg/webhook/resourcesemantics"
 )
 
 // NewAdmissionController constructs a reconciler
 func NewAdmissionController(
 	ctx context.Context,
 	name, path string,
-	handlers map[schema.GroupVersionKind]GenericCRD,
+	handlers map[schema.GroupVersionKind]resourcesemantics.GenericCRD,
 	wc func(context.Context) context.Context,
 	disallowUnknownFields bool,
 ) *controller.Impl {