From ee60a9874b66f57dda700fe87bfd8f909ec2a6a1 Mon Sep 17 00:00:00 2001 From: Eric Bottard Date: Tue, 10 Jul 2018 11:56:09 +0200 Subject: [PATCH 1/4] Add validation for bus parameter names Fixes #144 --- pkg/webhook/bus.go | 92 +++++++++++++++++++++++++++++++++++++ pkg/webhook/bus_test.go | 17 +++++++ pkg/webhook/webhook.go | 12 ++++- pkg/webhook/webhook_test.go | 46 +++++++++++++++++++ 4 files changed, 165 insertions(+), 2 deletions(-) create mode 100644 pkg/webhook/bus.go create mode 100644 pkg/webhook/bus_test.go diff --git a/pkg/webhook/bus.go b/pkg/webhook/bus.go new file mode 100644 index 00000000000..242c3312c42 --- /dev/null +++ b/pkg/webhook/bus.go @@ -0,0 +1,92 @@ +/* + * Copyright 2017 the original author or 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 webhook + +import ( + "context" + + "errors" + + "fmt" + "strings" + + "github.com/golang/glog" + "github.com/knative/eventing/pkg/apis/channels/v1alpha1" + "github.com/mattbaird/jsonpatch" + "k8s.io/apimachinery/pkg/util/validation" +) + +var ( + errInvalidBusInput = errors.New("failed to convert input into Bus") +) + +// ValidateBus is Bus resource specific validation and mutation handler +func ValidateBus(ctx context.Context) ResourceCallback { + return func(patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error { + oldBus, newBus, err := unmarshalBuses(ctx, old, new, "ValidateBus") + if err != nil { + return err + } + + return validateBus(oldBus, newBus) + } +} + +func validateBus(old, new *v1alpha1.Bus) error { + if new.Spec.Parameters != nil { + if new.Spec.Parameters.Channel != nil { + for _, p := range *new.Spec.Parameters.Channel { + errs := validation.IsConfigMapKey(p.Name) + if len(errs) > 0 { + return fmt.Errorf("invalid parameter name Spec.Parameters.Channel.%s: %s", p.Name, + strings.Join(errs, ", ")) + } + } + } + if new.Spec.Parameters.Subscription != nil { + for _, p := range *new.Spec.Parameters.Subscription { + errs := validation.IsConfigMapKey(p.Name) + if len(errs) > 0 { + return fmt.Errorf("invalid parameter name Spec.Parameters.Subscription.%s: %s", p.Name, + strings.Join(errs, ", ")) + } + } + } + } + return nil +} + +func unmarshalBuses( + ctx context.Context, old, new GenericCRD, fnName string) (*v1alpha1.Bus, *v1alpha1.Bus, error) { + var oldBus *v1alpha1.Bus + if old != nil { + var ok bool + oldBus, ok = old.(*v1alpha1.Bus) + if !ok { + return nil, nil, errInvalidBusInput + } + } + glog.Infof("%s: OLD Bus is\n%+v", fnName, oldBus) + + newBus, ok := new.(*v1alpha1.Bus) + if !ok { + return nil, nil, errInvalidBusInput + } + glog.Infof("%s: NEW Bus is\n%+v", fnName, newBus) + + return oldBus, newBus, nil +} diff --git a/pkg/webhook/bus_test.go b/pkg/webhook/bus_test.go new file mode 100644 index 00000000000..da58b42be39 --- /dev/null +++ b/pkg/webhook/bus_test.go @@ -0,0 +1,17 @@ +/* + * Copyright 2017 the original author or 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 webhook diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index f8fb6972467..555eea818fb 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -38,7 +38,7 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1" corev1 "k8s.io/api/core/v1" - v1beta1 "k8s.io/api/extensions/v1beta1" + "k8s.io/api/extensions/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -202,6 +202,14 @@ func NewAdmissionController(client kubernetes.Interface, options ControllerOptio client: client, options: options, handlers: map[string]GenericCRDHandler{ + "Bus": { + Factory: &v1alpha1.Bus{}, + Validator: ValidateBus(ctx), + }, + "ClusterBus": { + Factory: &v1alpha1.ClusterBus{}, + Validator: ValidateBus(ctx), + }, "Channel": { Factory: &v1alpha1.Channel{}, Validator: ValidateChannel(ctx), @@ -290,7 +298,7 @@ func (ac *AdmissionController) unregister( func (ac *AdmissionController) register( ctx context.Context, client clientadmissionregistrationv1beta1.MutatingWebhookConfigurationInterface, caCert []byte) error { // nolint: lll - resources := []string{"channels", "subscriptions"} + resources := []string{"buses", "clusterbuses", "channels", "subscriptions"} webhook := &admissionregistrationv1beta1.MutatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index 6d5bbc7fb71..118e09572c2 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -29,6 +29,7 @@ import ( "github.com/mattbaird/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1" + "k8s.io/api/core/v1" extensionsv1beta1 "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" fakekubeclientset "k8s.io/client-go/kubernetes/fake" @@ -127,6 +128,33 @@ func TestUnknownKindFails(t *testing.T) { expectFailsWith(t, ac.admit(testCtx, &req), "unhandled kind") } +func TestInvalidBusParameterNameFails(t *testing.T) { + _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) + req := &admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Create, + Kind: metav1.GroupVersionKind{Kind: "Bus"}, + } + invalidName := "paramètre" + bus := createBus(testBusName, "foobar/dispatcher") + bus.Spec.Parameters.Subscription = &[]v1alpha1.Parameter{{Name: invalidName}} + marshaled, err := json.Marshal(bus) + if err != nil { + t.Fatalf("Failed to marshal bus: %s", err) + } + req.Object.Raw = marshaled + expectFailsWith(t, ac.admit(testCtx, req), "invalid parameter name Spec.Parameters.Subscription.paramètre") + + invalidName = "param/name" + bus = createBus(testBusName, "foobar/dispatcher") + bus.Spec.Parameters.Channel = &[]v1alpha1.Parameter{{Name: invalidName}} + marshaled, err = json.Marshal(bus) + if err != nil { + t.Fatalf("Failed to marshal bus: %s", err) + } + req.Object.Raw = marshaled + expectFailsWith(t, ac.admit(testCtx, req), "invalid parameter name Spec.Parameters.Channel.param/name") +} + func TestInvalidNewChannelNameFails(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) req := &admissionv1beta1.AdmissionRequest{ @@ -319,6 +347,24 @@ func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) { } } +func createBus(busName string, dispatcherImage string) v1alpha1.Bus { + return v1alpha1.Bus{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: busName, + }, + Spec: v1alpha1.BusSpec{ + Dispatcher: v1.Container{ + Image: dispatcherImage, + }, + Parameters: &v1alpha1.BusParameters{ + Channel: &[]v1alpha1.Parameter{}, + Subscription: &[]v1alpha1.Parameter{}, + }, + }, + } +} + func createBaseUpdateChannel() *admissionv1beta1.AdmissionRequest { return &admissionv1beta1.AdmissionRequest{ Operation: admissionv1beta1.Update, From 847588ba730f6605471355d0848a7d68a5fa8f26 Mon Sep 17 00:00:00 2001 From: Eric Bottard Date: Tue, 10 Jul 2018 16:43:52 +0200 Subject: [PATCH 2/4] Add tests for happy path, ClusterBus --- pkg/webhook/bus.go | 26 +++++++------- pkg/webhook/bus_test.go | 17 --------- pkg/webhook/webhook_test.go | 72 +++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 30 deletions(-) delete mode 100644 pkg/webhook/bus_test.go diff --git a/pkg/webhook/bus.go b/pkg/webhook/bus.go index 242c3312c42..a312dfd6d28 100644 --- a/pkg/webhook/bus.go +++ b/pkg/webhook/bus.go @@ -1,11 +1,11 @@ /* - * Copyright 2017 the original author or authors. + * Copyright 2018 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 + * 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, @@ -31,7 +31,7 @@ import ( ) var ( - errInvalidBusInput = errors.New("failed to convert input into Bus") + errInvalidBusInput = errors.New("failed to convert input into Bus or ClusterBus") ) // ValidateBus is Bus resource specific validation and mutation handler @@ -46,10 +46,10 @@ func ValidateBus(ctx context.Context) ResourceCallback { } } -func validateBus(old, new *v1alpha1.Bus) error { - if new.Spec.Parameters != nil { - if new.Spec.Parameters.Channel != nil { - for _, p := range *new.Spec.Parameters.Channel { +func validateBus(old, new v1alpha1.GenericBus) error { + if new.GetSpec().Parameters != nil { + if new.GetSpec().Parameters.Channel != nil { + for _, p := range *new.GetSpec().Parameters.Channel { errs := validation.IsConfigMapKey(p.Name) if len(errs) > 0 { return fmt.Errorf("invalid parameter name Spec.Parameters.Channel.%s: %s", p.Name, @@ -57,8 +57,8 @@ func validateBus(old, new *v1alpha1.Bus) error { } } } - if new.Spec.Parameters.Subscription != nil { - for _, p := range *new.Spec.Parameters.Subscription { + if new.GetSpec().Parameters.Subscription != nil { + for _, p := range *new.GetSpec().Parameters.Subscription { errs := validation.IsConfigMapKey(p.Name) if len(errs) > 0 { return fmt.Errorf("invalid parameter name Spec.Parameters.Subscription.%s: %s", p.Name, @@ -71,18 +71,18 @@ func validateBus(old, new *v1alpha1.Bus) error { } func unmarshalBuses( - ctx context.Context, old, new GenericCRD, fnName string) (*v1alpha1.Bus, *v1alpha1.Bus, error) { - var oldBus *v1alpha1.Bus + ctx context.Context, old, new GenericCRD, fnName string) (v1alpha1.GenericBus, v1alpha1.GenericBus, error) { + var oldBus v1alpha1.GenericBus if old != nil { var ok bool - oldBus, ok = old.(*v1alpha1.Bus) + oldBus, ok = old.(v1alpha1.GenericBus) if !ok { return nil, nil, errInvalidBusInput } } glog.Infof("%s: OLD Bus is\n%+v", fnName, oldBus) - newBus, ok := new.(*v1alpha1.Bus) + newBus, ok := new.(v1alpha1.GenericBus) if !ok { return nil, nil, errInvalidBusInput } diff --git a/pkg/webhook/bus_test.go b/pkg/webhook/bus_test.go deleted file mode 100644 index da58b42be39..00000000000 --- a/pkg/webhook/bus_test.go +++ /dev/null @@ -1,17 +0,0 @@ -/* - * Copyright 2017 the original author or 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 webhook diff --git a/pkg/webhook/webhook_test.go b/pkg/webhook/webhook_test.go index 118e09572c2..c042f6548ae 100644 --- a/pkg/webhook/webhook_test.go +++ b/pkg/webhook/webhook_test.go @@ -128,6 +128,33 @@ func TestUnknownKindFails(t *testing.T) { expectFailsWith(t, ac.admit(testCtx, &req), "unhandled kind") } +func TestValidBusParameterNamePasses(t *testing.T) { + _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) + req := &admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Create, + Kind: metav1.GroupVersionKind{Kind: "Bus"}, + } + validName := "ok_param.name" + bus := createBus(testBusName, "foobar/dispatcher") + bus.Spec.Parameters.Subscription = &[]v1alpha1.Parameter{{Name: validName}} + marshaled, err := json.Marshal(bus) + if err != nil { + t.Fatalf("Failed to marshal bus: %s", err) + } + req.Object.Raw = marshaled + expectAllowed(t, ac.admit(testCtx, req)) + + validName = "simple-name" + bus = createBus(testBusName, "foobar/dispatcher") + bus.Spec.Parameters.Channel = &[]v1alpha1.Parameter{{Name: validName}} + marshaled, err = json.Marshal(bus) + if err != nil { + t.Fatalf("Failed to marshal bus: %s", err) + } + req.Object.Raw = marshaled + expectAllowed(t, ac.admit(testCtx, req)) +} + func TestInvalidBusParameterNameFails(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) req := &admissionv1beta1.AdmissionRequest{ @@ -155,6 +182,33 @@ func TestInvalidBusParameterNameFails(t *testing.T) { expectFailsWith(t, ac.admit(testCtx, req), "invalid parameter name Spec.Parameters.Channel.param/name") } +func TestInvalidClusterBusParameterNameFails(t *testing.T) { + _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) + req := &admissionv1beta1.AdmissionRequest{ + Operation: admissionv1beta1.Create, + Kind: metav1.GroupVersionKind{Kind: "ClusterBus"}, + } + invalidName := "paramètre" + bus := createClusterBus(testBusName, "foobar/dispatcher") + bus.Spec.Parameters.Subscription = &[]v1alpha1.Parameter{{Name: invalidName}} + marshaled, err := json.Marshal(bus) + if err != nil { + t.Fatalf("Failed to marshal bus: %s", err) + } + req.Object.Raw = marshaled + expectFailsWith(t, ac.admit(testCtx, req), "invalid parameter name Spec.Parameters.Subscription.paramètre") + + invalidName = "param/name" + bus = createClusterBus(testBusName, "foobar/dispatcher") + bus.Spec.Parameters.Channel = &[]v1alpha1.Parameter{{Name: invalidName}} + marshaled, err = json.Marshal(bus) + if err != nil { + t.Fatalf("Failed to marshal bus: %s", err) + } + req.Object.Raw = marshaled + expectFailsWith(t, ac.admit(testCtx, req), "invalid parameter name Spec.Parameters.Channel.param/name") +} + func TestInvalidNewChannelNameFails(t *testing.T) { _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) req := &admissionv1beta1.AdmissionRequest{ @@ -365,6 +419,24 @@ func createBus(busName string, dispatcherImage string) v1alpha1.Bus { } } +func createClusterBus(busName string, dispatcherImage string) v1alpha1.ClusterBus { + return v1alpha1.ClusterBus{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: testNamespace, + Name: busName, + }, + Spec: v1alpha1.BusSpec{ + Dispatcher: v1.Container{ + Image: dispatcherImage, + }, + Parameters: &v1alpha1.BusParameters{ + Channel: &[]v1alpha1.Parameter{}, + Subscription: &[]v1alpha1.Parameter{}, + }, + }, + } +} + func createBaseUpdateChannel() *admissionv1beta1.AdmissionRequest { return &admissionv1beta1.AdmissionRequest{ Operation: admissionv1beta1.Update, From 2b698437cd4df909087b5aa53db1485bc600c052 Mon Sep 17 00:00:00 2001 From: Eric Bottard Date: Tue, 10 Jul 2018 17:59:41 +0200 Subject: [PATCH 3/4] Address review comments --- pkg/webhook/bus.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/bus.go b/pkg/webhook/bus.go index a312dfd6d28..042f4c6a269 100644 --- a/pkg/webhook/bus.go +++ b/pkg/webhook/bus.go @@ -18,9 +18,7 @@ package webhook import ( "context" - "errors" - "fmt" "strings" @@ -82,6 +80,9 @@ func unmarshalBuses( } glog.Infof("%s: OLD Bus is\n%+v", fnName, oldBus) + if new == nil { + return nil, nil, errInvalidBusInput + } newBus, ok := new.(v1alpha1.GenericBus) if !ok { return nil, nil, errInvalidBusInput From da099b6f93638abaa0f2b954f6b1a7e7e50b43c4 Mon Sep 17 00:00:00 2001 From: Eric Bottard Date: Tue, 10 Jul 2018 19:27:38 +0200 Subject: [PATCH 4/4] Use dedicated error --- pkg/webhook/bus.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/webhook/bus.go b/pkg/webhook/bus.go index 042f4c6a269..525b7660b82 100644 --- a/pkg/webhook/bus.go +++ b/pkg/webhook/bus.go @@ -30,6 +30,7 @@ import ( var ( errInvalidBusInput = errors.New("failed to convert input into Bus or ClusterBus") + errInternalNilBus = errors.New("unexpected internal error: nil Bus or ClusterBus") ) // ValidateBus is Bus resource specific validation and mutation handler @@ -81,7 +82,7 @@ func unmarshalBuses( glog.Infof("%s: OLD Bus is\n%+v", fnName, oldBus) if new == nil { - return nil, nil, errInvalidBusInput + return nil, nil, errInternalNilBus } newBus, ok := new.(v1alpha1.GenericBus) if !ok {