Split apart defaulting and validation webhooks

Related: knative/pkg#848

Gopkg.toml

@@ -38,7 +38,10 @@ required = [
 
 [[override]]
   name = "knative.dev/pkg"
-  branch = "master"
+  #TODO(mattmoor): DO NOT SUBMIT
+  source = "github.com/mattmoor/pkg"
+  revision = "492ed286f040d9137dd2cee82eb1e25d8cf97523"
+  # branch = "master"
 
 [[override]]
   name = "google.golang.org/genproto"

cmd/webhook/main.go

@@ -29,25 +29,53 @@ import (
 	"knative.dev/pkg/webhook"
 	"knative.dev/pkg/webhook/certificates"
 	"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
@@ -70,6 +98,7 @@ func main() {
 
 	sharedmain.MainWithContext(ctx, "webhook",
 		certificates.NewController,
-		NewResourceAdmissionController,
+		NewDefaultingAdmissionController,
+		NewValidationAdmissionController,
 	)
 }

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: v1
 kind: Secret

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/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 {

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

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package resourcesemantics
+package defaulting
 
 import (
 	"bytes"
@@ -30,7 +30,6 @@ import (
 	"go.uber.org/zap"
 	admissionv1beta1 "k8s.io/api/admission/v1beta1"
 	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
 	admissionlisters "k8s.io/client-go/listers/admissionregistration/v1beta1"
@@ -44,23 +43,16 @@ import (
 	"knative.dev/pkg/system"
 	"knative.dev/pkg/webhook"
 	certresources "knative.dev/pkg/webhook/certificates/resources"
+	"knative.dev/pkg/webhook/resourcesemantics"
 )
 
-// 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 {
-	apis.Defaultable
-	apis.Validatable
-	runtime.Object
-}
-
 var errMissingNewObject = errors.New("the new object may not be nil")
 
 // reconciler implements the AdmissionController for resources
 type reconciler struct {
 	name     string
 	path     string
-	handlers map[schema.GroupVersionKind]GenericCRD
+	handlers map[schema.GroupVersionKind]resourcesemantics.GenericCRD
 
 	withContext func(context.Context) context.Context
 
@@ -217,10 +209,10 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1beta1.Admissio
 	}
 
 	// nil values denote absence of `old` (create) or `new` (delete) objects.
-	var oldObj, newObj GenericCRD
+	var oldObj, newObj resourcesemantics.GenericCRD
 
 	if len(newBytes) != 0 {
-		newObj = handler.DeepCopyObject().(GenericCRD)
+		newObj = handler.DeepCopyObject().(resourcesemantics.GenericCRD)
 		newDecoder := json.NewDecoder(bytes.NewBuffer(newBytes))
 		if ac.disallowUnknownFields {
 			newDecoder.DisallowUnknownFields()
@@ -230,7 +222,7 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1beta1.Admissio
 		}
 	}
 	if len(oldBytes) != 0 {
-		oldObj = handler.DeepCopyObject().(GenericCRD)
+		oldObj = handler.DeepCopyObject().(resourcesemantics.GenericCRD)
 		oldDecoder := json.NewDecoder(bytes.NewBuffer(oldBytes))
 		if ac.disallowUnknownFields {
 			oldDecoder.DisallowUnknownFields()
@@ -258,7 +250,7 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1beta1.Admissio
 	if oldObj != nil {
 		// Copy the old object and set defaults so that we don't reject our own
 		// defaulting done earlier in the webhook.
-		oldObj = oldObj.DeepCopyObject().(GenericCRD)
+		oldObj = oldObj.DeepCopyObject().(resourcesemantics.GenericCRD)
 		oldObj.SetDefaults(ctx)
 
 		s, ok := oldObj.(apis.HasSpec)
@@ -293,17 +285,10 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1beta1.Admissio
 	if newObj == nil {
 		return nil, errMissingNewObject
 	}
-	if err := validate(ctx, 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.
-		return nil, err
-	}
-
 	return json.Marshal(patches)
 }
 
-func (ac *reconciler) setUserInfoAnnotations(ctx context.Context, patches duck.JSONPatch, new GenericCRD, groupName string) (duck.JSONPatch, error) {
+func (ac *reconciler) setUserInfoAnnotations(ctx context.Context, patches duck.JSONPatch, new resourcesemantics.GenericCRD, groupName string) (duck.JSONPatch, error) {
 	if new == nil {
 		return patches, nil
 	}
@@ -341,33 +326,8 @@ func roundTripPatch(bytes []byte, unmarshalled interface{}) (duck.JSONPatch, err
 	return jsonpatch.CreatePatch(bytes, marshaledBytes)
 }
 
-// validate performs validation on the provided "new" CRD.
-// For legacy purposes, this also does apis.Immutable validation,
-// which is deprecated and will be removed in a future release.
-func validate(ctx context.Context, new apis.Validatable) error {
-	if apis.IsInUpdate(ctx) {
-		old := apis.GetBaseline(ctx)
-		if immutableNew, ok := new.(apis.Immutable); ok {
-			immutableOld, ok := old.(apis.Immutable)
-			if !ok {
-				return fmt.Errorf("unexpected type mismatch %T vs. %T", old, new)
-			}
-			if err := immutableNew.CheckImmutableFields(ctx, immutableOld); err != nil {
-				return err
-			}
-		}
-	}
-
-	// Can't just `return new.Validate()` because it doesn't properly nil-check.
-	if err := new.Validate(ctx); err != nil {
-		return err
-	}
-
-	return nil
-}
-
 // setDefaults simply leverages apis.Defaultable to set defaults.
-func setDefaults(ctx context.Context, patches duck.JSONPatch, crd GenericCRD) (duck.JSONPatch, error) {
+func setDefaults(ctx context.Context, patches duck.JSONPatch, crd resourcesemantics.GenericCRD) (duck.JSONPatch, error) {
 	before, after := crd.DeepCopyObject(), crd
 	after.SetDefaults(ctx)
 

vendor/knative.dev/pkg/webhook/resourcesemantics/user_info.go → vendor/knative.dev/pkg/webhook/resourcesemantics/defaulting/user_info.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"

vendor/knative.dev/pkg/webhook/resourcesemantics/interface.go

@@ -0,0 +1,30 @@
+/*
+Copyright 2019 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package resourcesemantics
+
+import (
+	"k8s.io/apimachinery/pkg/runtime"
+	"knative.dev/pkg/apis"
+)
+
+// 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 {
+	apis.Defaultable
+	apis.Validatable
+	runtime.Object
+}