Skip to content

Commit

Permalink
Split the resource semantic webhooks into separate AdmissionControllers
Browse files Browse the repository at this point in the history
By combining our validation logic into our mutating webhook we were previously allowing for mutating webhooks evaluated after our own to modify our resources into invalid shapes.  There are no guarantees around ordering of mutating webhooks (that I could find), so the only way to remedy this properly is to split apart the two into separate webhook configurations:
 - `defaulting`: which runs during the mutating admission webhook phase
 - `validation`: which runs during the validating admission webhook phase.

The diagram in [this post](https://kubernetes.io/blog/2019/03/21/a-guide-to-kubernetes-admission-controllers/) is very helpful in illustrating the flow of webhooks.

Fixes: #847
  • Loading branch information
mattmoor committed Nov 5, 2019
1 parent cdc2959 commit 3f9bcd4
Show file tree
Hide file tree
Showing 13 changed files with 1,150 additions and 125 deletions.
6 changes: 1 addition & 5 deletions apis/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,7 @@ type Convertible interface {
// 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
}
type Immutable interface{}

// Listable indicates that a particular type can be returned via the returned
// list type by the API server.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package resourcesemantics
package defaulting

import (
"context"
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package resourcesemantics
package defaulting

import (
"bytes"
Expand All @@ -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"
Expand All @@ -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

Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package resourcesemantics
package defaulting

import (
"context"
Expand Down Expand Up @@ -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"
)

Expand All @@ -53,7 +54,7 @@ const (
)

var (
handlers = map[schema.GroupVersionKind]GenericCRD{
handlers = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
{
Group: "pkg.knative.dev",
Version: "v1alpha1",
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package resourcesemantics
package defaulting

import (
"context"
Expand All @@ -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"
Expand Down Expand Up @@ -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 */)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package resourcesemantics
package defaulting

import (
"context"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package resourcesemantics
package defaulting

import (
"context"
Expand Down
30 changes: 30 additions & 0 deletions webhook/resourcesemantics/interface.go
Original file line number Diff line number Diff line change
@@ -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
}
84 changes: 84 additions & 0 deletions webhook/resourcesemantics/validation/controller.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
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 validation

import (
"context"

// Injection stuff
kubeclient "knative.dev/pkg/client/injection/kube/client"
vwhinformer "knative.dev/pkg/client/injection/kube/informers/admissionregistration/v1beta1/validatingwebhookconfiguration"
secretinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/secret"

"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/tools/cache"
"knative.dev/pkg/controller"
"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]resourcesemantics.GenericCRD,
wc func(context.Context) context.Context,
disallowUnknownFields bool,
) *controller.Impl {

client := kubeclient.Get(ctx)
vwhInformer := vwhinformer.Get(ctx)
secretInformer := secretinformer.Get(ctx)
options := webhook.GetOptions(ctx)

wh := &reconciler{
name: name,
path: path,
handlers: handlers,

withContext: wc,
disallowUnknownFields: disallowUnknownFields,
secretName: options.SecretName,

client: client,
vwhlister: vwhInformer.Lister(),
secretlister: secretInformer.Lister(),
}

logger := logging.FromContext(ctx)
c := controller.NewImpl(wh, logger, "ConfigMapWebhook")

// Reconcile when the named ValidatingWebhookConfiguration changes.
vwhInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
FilterFunc: controller.FilterWithName(name),
// It doesn't matter what we enqueue because we will always Reconcile
// the named VWH resource.
Handler: controller.HandleAll(c.Enqueue),
})

// Reconcile when the cert bundle changes.
secretInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
FilterFunc: controller.FilterWithNameAndNamespace(system.Namespace(), wh.secretName),
// It doesn't matter what we enqueue because we will always Reconcile
// the named VWH resource.
Handler: controller.HandleAll(c.Enqueue),
})

return c
}
Loading

0 comments on commit 3f9bcd4

Please sign in to comment.