Split the resource semantic webhooks into separate AdmissionControllers

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 {

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",

webhook/resourcesemantics/controller.go → 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 {

webhook/resourcesemantics/resourcesemantics.go → 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)
 

webhook/resourcesemantics/resourcesemantics_test.go → webhook/resourcesemantics/defaulting/defaulting_test.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"
@@ -42,6 +42,7 @@ import (
 	. "knative.dev/pkg/logging/testing"
 	. "knative.dev/pkg/reconciler/testing"
 	. "knative.dev/pkg/testing"
+	"knative.dev/pkg/webhook/resourcesemantics"
 	. "knative.dev/pkg/webhook/testing"
 )
 
@@ -53,7 +54,7 @@ const (
 )
 
 var (
-	handlers = map[schema.GroupVersionKind]GenericCRD{
+	handlers = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
 		{
 			Group:   "pkg.knative.dev",
 			Version: "v1alpha1",
@@ -292,13 +293,6 @@ func TestAdmitCreates(t *testing.T) {
 			Path:      "/metadata/annotations/pkg.knative.dev~1creator",
 			Value:     user1,
 		}},
-	}, {
-		name: "with bad field",
-		setup: func(ctx context.Context, r *Resource) {
-			// Put a bad value in.
-			r.Spec.FieldWithValidation = "not what's expected"
-		},
-		rejection: "invalid value",
 	}}
 
 	for _, tc := range tests {
@@ -399,24 +393,6 @@ func TestAdmitUpdates(t *testing.T) {
 			Path:      "/spec/fieldThatsImmutableWithDefault",
 			Value:     "this is another default value",
 		}},
-	}, {
-		name: "bad mutation (immutable)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-		},
-		mutate: func(ctx context.Context, r *Resource) {
-			r.Spec.FieldThatsImmutableWithDefault = "something different"
-		},
-		rejection: "Immutable field changed",
-	}, {
-		name: "bad mutation (validation)",
-		setup: func(ctx context.Context, r *Resource) {
-			r.SetDefaults(ctx)
-		},
-		mutate: func(ctx context.Context, r *Resource) {
-			r.Spec.FieldWithValidation = "not what's expected"
-		},
-		rejection: "invalid value",
 	}}
 
 	for _, tc := range tests {

webhook/resourcesemantics/table_test.go → webhook/resourcesemantics/defaulting/table_test.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"
@@ -35,6 +35,7 @@ import (
 	"knative.dev/pkg/system"
 	"knative.dev/pkg/webhook"
 	certresources "knative.dev/pkg/webhook/certificates/resources"
+	"knative.dev/pkg/webhook/resourcesemantics"
 
 	. "knative.dev/pkg/reconciler/testing"
 	. "knative.dev/pkg/webhook/testing"
@@ -330,7 +331,7 @@ func TestNew(t *testing.T) {
 	ctx = webhook.WithOptions(ctx, webhook.Options{})
 
 	c := NewAdmissionController(ctx, "foo", "/bar",
-		map[schema.GroupVersionKind]GenericCRD{},
+		map[schema.GroupVersionKind]resourcesemantics.GenericCRD{},
 		func(ctx context.Context) context.Context {
 			return ctx
 		}, true /* disallow unknown field */)

webhook/resourcesemantics/user_info.go → 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"

webhook/resourcesemantics/user_info_test.go → webhook/resourcesemantics/defaulting/user_info_test.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"

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
+}