From ab62d270c6efc30c030bdc51ee5432eadc55080e Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 19:38:38 -0800 Subject: [PATCH 01/19] Go vet entire pkg, turn goimports back on This makes sure to run go vet on the examples and the alias file, and turns goimports back on. --- hack/verify.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hack/verify.sh b/hack/verify.sh index 43278b73af..4e9b760495 100755 --- a/hack/verify.sh +++ b/hack/verify.sh @@ -20,7 +20,7 @@ source $(dirname ${BASH_SOURCE})/common.sh header_text "running go vet" -go vet ./pkg/... +go vet ./... # go get is broken for golint. re-enable this once it is fixed. #header_text "running golint" @@ -49,9 +49,9 @@ gometalinter.v2 --disable-all \ --dupl-threshold=400 \ --enable=dupl \ --skip=atomic \ - ./pkg/... + --enable=goimports \ + ./pkg/... ./example/... . # TODO: Enable these as we fix them to make them pass -# --enable=goimports \ # --enable=gosec \ # --enable=maligned \ # --enable=safesql \ From f0ec881b9650f4587cba2a37207c87dbae71d7c9 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 19:48:48 -0800 Subject: [PATCH 02/19] Make everything pass goimports We had goimports turned off. When turned back on, it complained about a bunch of stuff. This fixes that. --- alias.go | 6 ++-- doc.go | 2 +- example/mutatingwebhook.go | 8 ++--- pkg/builder/builder_suite_test.go | 2 +- pkg/builder/example_test.go | 2 +- pkg/client/client_cache.go | 4 +-- pkg/client/example_test.go | 12 +++---- pkg/controller/controller.go | 16 ++++----- pkg/controller/controller_suite_test.go | 2 +- .../controllerutil/controllerutil.go | 14 ++++---- pkg/controller/example_test.go | 8 ++--- pkg/event/event.go | 12 +++---- pkg/handler/enqueue.go | 2 +- pkg/handler/enqueue_owner.go | 2 +- pkg/internal/controller/controller.go | 2 +- pkg/internal/controller/controller_test.go | 12 +++---- pkg/log/log_test.go | 1 - pkg/log/zap/kube_helpers.go | 1 - pkg/manager/example_test.go | 2 +- pkg/manager/internal.go | 2 +- pkg/manager/manager_suite_test.go | 2 +- pkg/metrics/client_go_adapter.go | 34 +++++++++---------- pkg/recorder/example_test.go | 4 +-- pkg/runtime/scheme/scheme.go | 1 - pkg/runtime/signals/signal.go | 2 +- pkg/source/example_test.go | 4 +-- pkg/source/source.go | 2 +- pkg/webhook/server.go | 2 -- 28 files changed, 79 insertions(+), 84 deletions(-) diff --git a/alias.go b/alias.go index 2620fab772..b41f51435b 100644 --- a/alias.go +++ b/alias.go @@ -22,11 +22,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/scheme" - "sigs.k8s.io/controller-runtime/pkg/manager/signals" ) // Builder builds an Application ControllerManagedBy (e.g. Operator) and returns a manager.Manager to start it. @@ -47,7 +47,7 @@ type Manager = manager.Manager // Options are the arguments for creating a new Manager type Options = manager.Options -// Builder builds a new Scheme for mapping go types to Kubernetes GroupVersionKinds. +// SchemeBuilder builds a new Scheme for mapping go types to Kubernetes GroupVersionKinds. type SchemeBuilder = scheme.Builder // GroupVersion contains the "group" and the "version", which uniquely identifies the API. diff --git a/doc.go b/doc.go index 5151792e27..253d6556fc 100644 --- a/doc.go +++ b/doc.go @@ -110,7 +110,7 @@ limitations under the License. // controller-runtime. // // Metrics (pkg/metrics) provided by controller-runtime are registered into a -// controller-runtime-specific Prometheus metrics registery. The manager can +// controller-runtime-specific Prometheus metrics registry. The manager can // serve these by an HTTP endpoint, and additional metrics may be registered to // this Registry as normal. // diff --git a/example/mutatingwebhook.go b/example/mutatingwebhook.go index 98b0323809..f7e0437b14 100644 --- a/example/mutatingwebhook.go +++ b/example/mutatingwebhook.go @@ -73,8 +73,8 @@ func (a *podAnnotator) mutatePodsFn(ctx context.Context, pod *corev1.Pod) error var _ inject.Client = &podAnnotator{} // InjectClient injects the client. -func (v *podAnnotator) InjectClient(c client.Client) error { - v.client = c +func (a *podAnnotator) InjectClient(c client.Client) error { + a.client = c return nil } @@ -83,7 +83,7 @@ func (v *podAnnotator) InjectClient(c client.Client) error { var _ inject.Decoder = &podAnnotator{} // InjectDecoder injects the decoder. -func (v *podAnnotator) InjectDecoder(d types.Decoder) error { - v.decoder = d +func (a *podAnnotator) InjectDecoder(d types.Decoder) error { + a.decoder = d return nil } diff --git a/pkg/builder/builder_suite_test.go b/pkg/builder/builder_suite_test.go index 46ee514ac5..db6a238e15 100644 --- a/pkg/builder/builder_suite_test.go +++ b/pkg/builder/builder_suite_test.go @@ -23,9 +23,9 @@ import ( . "github.com/onsi/gomega" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/envtest" - "sigs.k8s.io/controller-runtime/pkg/metrics" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/metrics" ) func TestSource(t *testing.T) { diff --git a/pkg/builder/example_test.go b/pkg/builder/example_test.go index 25e0facbe4..2197be8af2 100644 --- a/pkg/builder/example_test.go +++ b/pkg/builder/example_test.go @@ -29,8 +29,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/manager/signals" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) // This example creates a simple application ControllerManagedBy that is configured for ReplicaSets and Pods. diff --git a/pkg/client/client_cache.go b/pkg/client/client_cache.go index d6452ab62e..2a1ff05d50 100644 --- a/pkg/client/client_cache.go +++ b/pkg/client/client_cache.go @@ -22,7 +22,7 @@ import ( "sync" "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -141,5 +141,5 @@ type objMeta struct { *resourceMeta // Object contains meta data for the object instance - v1.Object + metav1.Object } diff --git a/pkg/client/example_test.go b/pkg/client/example_test.go index c0ab71301f..9e29f5b3ff 100644 --- a/pkg/client/example_test.go +++ b/pkg/client/example_test.go @@ -22,16 +22,16 @@ import ( "os" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/config" ) var ( - c client.Client + c client.Client someIndexer client.FieldIndexer ) @@ -78,7 +78,7 @@ func ExampleClient_get() { func ExampleClient_create() { // Using a typed object. pod := &corev1.Pod{ - ObjectMeta: v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Namespace: "namespace", Name: "name", }, @@ -177,7 +177,7 @@ func ExampleClient_update() { func ExampleClient_delete() { // Using a typed object. pod := &corev1.Pod{ - ObjectMeta: v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Namespace: "namespace", Name: "name", }, @@ -215,7 +215,7 @@ func ExampleFieldIndexer_secretName() { }) // elsewhere (e.g. in your reconciler) - mySecretName := "someSecret" // derived from the reconcile.Request, for instance + mySecretName := "someSecret" // derived from the reconcile.Request, for instance var podsWithSecrets corev1.PodList _ = c.List(context.Background(), &podsWithSecrets, client.MatchingField("spec.volumes.secret.secretName", mySecretName)) } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 0ad828c477..8d5f8a34cc 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -80,15 +80,15 @@ func New(name string, mgr manager.Manager, options Options) (Controller, error) // Create controller with dependencies set c := &controller.Controller{ - Do: options.Reconciler, - Cache: mgr.GetCache(), - Config: mgr.GetConfig(), - Scheme: mgr.GetScheme(), - Client: mgr.GetClient(), - Recorder: mgr.GetEventRecorderFor(name), - Queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), name), + Do: options.Reconciler, + Cache: mgr.GetCache(), + Config: mgr.GetConfig(), + Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + Recorder: mgr.GetEventRecorderFor(name), + Queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), name), MaxConcurrentReconciles: options.MaxConcurrentReconciles, - Name: name, + Name: name, } // Add the controller as a Manager components diff --git a/pkg/controller/controller_suite_test.go b/pkg/controller/controller_suite_test.go index ec730a2168..30c7fd6efe 100644 --- a/pkg/controller/controller_suite_test.go +++ b/pkg/controller/controller_suite_test.go @@ -24,9 +24,9 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/envtest" - "sigs.k8s.io/controller-runtime/pkg/metrics" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/metrics" ) func TestSource(t *testing.T) { diff --git a/pkg/controller/controllerutil/controllerutil.go b/pkg/controller/controllerutil/controllerutil.go index 3edc98e541..320530ce3f 100644 --- a/pkg/controller/controllerutil/controllerutil.go +++ b/pkg/controller/controllerutil/controllerutil.go @@ -22,7 +22,7 @@ import ( "reflect" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" @@ -33,15 +33,15 @@ import ( // a controller reference is already owned by another controller Object is the // subject and Owner is the reference for the current owner type AlreadyOwnedError struct { - Object v1.Object - Owner v1.OwnerReference + Object metav1.Object + Owner metav1.OwnerReference } func (e *AlreadyOwnedError) Error() string { return fmt.Sprintf("Object %s/%s is already owned by another %s controller %s", e.Object.GetNamespace(), e.Object.GetName(), e.Owner.Kind, e.Owner.Name) } -func newAlreadyOwnedError(Object v1.Object, Owner v1.OwnerReference) *AlreadyOwnedError { +func newAlreadyOwnedError(Object metav1.Object, Owner metav1.OwnerReference) *AlreadyOwnedError { return &AlreadyOwnedError{ Object: Object, Owner: Owner, @@ -53,7 +53,7 @@ func newAlreadyOwnedError(Object v1.Object, Owner v1.OwnerReference) *AlreadyOwn // reconciling the owner object on changes to owned (with a Watch + EnqueueRequestForOwner). // Since only one OwnerReference can be a controller, it returns an error if // there is another OwnerReference with Controller flag set. -func SetControllerReference(owner, object v1.Object, scheme *runtime.Scheme) error { +func SetControllerReference(owner, object metav1.Object, scheme *runtime.Scheme) error { ro, ok := owner.(runtime.Object) if !ok { return fmt.Errorf("is not a %T a runtime.Object, cannot call SetControllerReference", owner) @@ -65,7 +65,7 @@ func SetControllerReference(owner, object v1.Object, scheme *runtime.Scheme) err } // Create a new ref - ref := *v1.NewControllerRef(owner, schema.GroupVersionKind{Group: gvk.Group, Version: gvk.Version, Kind: gvk.Kind}) + ref := *metav1.NewControllerRef(owner, schema.GroupVersionKind{Group: gvk.Group, Version: gvk.Version, Kind: gvk.Kind}) existingRefs := object.GetOwnerReferences() fi := -1 @@ -88,7 +88,7 @@ func SetControllerReference(owner, object v1.Object, scheme *runtime.Scheme) err } // Returns true if a and b point to the same object -func referSameObject(a, b v1.OwnerReference) bool { +func referSameObject(a, b metav1.OwnerReference) bool { aGV, err := schema.ParseGroupVersion(a.APIVersion) if err != nil { return false diff --git a/pkg/controller/example_test.go b/pkg/controller/example_test.go index 6ad7aaa4fa..b6183d7439 100644 --- a/pkg/controller/example_test.go +++ b/pkg/controller/example_test.go @@ -19,15 +19,15 @@ package controller_test import ( "os" - "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/manager" - "sigs.k8s.io/controller-runtime/pkg/reconcile" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/manager/signals" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" ) @@ -70,7 +70,7 @@ func ExampleController() { } // Watch for Pod create / update / delete events and call Reconcile - err = c.Watch(&source.Kind{Type: &v1.Pod{}}, &handler.EnqueueRequestForObject{}) + err = c.Watch(&source.Kind{Type: &corev1.Pod{}}, &handler.EnqueueRequestForObject{}) if err != nil { log.Error(err, "unable to watch pods") os.Exit(1) diff --git a/pkg/event/event.go b/pkg/event/event.go index 4874e2e875..6aa50bf301 100644 --- a/pkg/event/event.go +++ b/pkg/event/event.go @@ -17,7 +17,7 @@ limitations under the License. package event import ( - "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -25,7 +25,7 @@ import ( // by a source.Source and transformed into a reconcile.Request by an handler.EventHandler. type CreateEvent struct { // Meta is the ObjectMeta of the Kubernetes Type that was created - Meta v1.Object + Meta metav1.Object // Object is the object from the event Object runtime.Object @@ -35,13 +35,13 @@ type CreateEvent struct { // by a source.Source and transformed into a reconcile.Request by an handler.EventHandler. type UpdateEvent struct { // MetaOld is the ObjectMeta of the Kubernetes Type that was updated (before the update) - MetaOld v1.Object + MetaOld metav1.Object // ObjectOld is the object from the event ObjectOld runtime.Object // MetaNew is the ObjectMeta of the Kubernetes Type that was updated (after the update) - MetaNew v1.Object + MetaNew metav1.Object // ObjectNew is the object from the event ObjectNew runtime.Object @@ -51,7 +51,7 @@ type UpdateEvent struct { // by a source.Source and transformed into a reconcile.Request by an handler.EventHandler. type DeleteEvent struct { // Meta is the ObjectMeta of the Kubernetes Type that was deleted - Meta v1.Object + Meta metav1.Object // Object is the object from the event Object runtime.Object @@ -66,7 +66,7 @@ type DeleteEvent struct { // handler.EventHandler. type GenericEvent struct { // Meta is the ObjectMeta of a Kubernetes Type this event is for - Meta v1.Object + Meta metav1.Object // Object is the object from the event Object runtime.Object diff --git a/pkg/handler/enqueue.go b/pkg/handler/enqueue.go index 2d0d4a6bda..2464c8b671 100644 --- a/pkg/handler/enqueue.go +++ b/pkg/handler/enqueue.go @@ -20,8 +20,8 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/reconcile" logf "sigs.k8s.io/controller-runtime/pkg/internal/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) var enqueueLog = logf.RuntimeLog.WithName("eventhandler").WithName("EnqueueRequestForObject") diff --git a/pkg/handler/enqueue_owner.go b/pkg/handler/enqueue_owner.go index 563e0c051b..17d512696c 100644 --- a/pkg/handler/enqueue_owner.go +++ b/pkg/handler/enqueue_owner.go @@ -26,9 +26,9 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/event" + logf "sigs.k8s.io/controller-runtime/pkg/internal/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" ) var _ EventHandler = &EnqueueRequestForOwner{} diff --git a/pkg/internal/controller/controller.go b/pkg/internal/controller/controller.go index 18deac0992..cd19332189 100644 --- a/pkg/internal/controller/controller.go +++ b/pkg/internal/controller/controller.go @@ -31,10 +31,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/internal/controller/metrics" + logf "sigs.k8s.io/controller-runtime/pkg/internal/log" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" "sigs.k8s.io/controller-runtime/pkg/source" ) diff --git a/pkg/internal/controller/controller_test.go b/pkg/internal/controller/controller_test.go index 0faa12c3f6..5bfb261ce3 100644 --- a/pkg/internal/controller/controller_test.go +++ b/pkg/internal/controller/controller_test.go @@ -24,7 +24,7 @@ import ( . "github.com/onsi/gomega" "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" - "k8s.io/api/apps/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/workqueue" @@ -62,9 +62,9 @@ var _ = Describe("controller", func() { informers = &informertest.FakeInformers{} ctrl = &Controller{ MaxConcurrentReconciles: 1, - Do: fakeReconcile, - Queue: queue, - Cache: informers, + Do: fakeReconcile, + Queue: queue, + Cache: informers, } ctrl.InjectFunc(func(interface{}) error { return nil }) }) @@ -103,8 +103,8 @@ var _ = Describe("controller", func() { c, err := cache.New(cfg, cache.Options{}) Expect(err).NotTo(HaveOccurred()) - c.GetInformer(&v1.Deployment{}) - c.GetInformer(&v1.ReplicaSet{}) + c.GetInformer(&appsv1.Deployment{}) + c.GetInformer(&appsv1.ReplicaSet{}) ctrl.Cache = c ctrl.WaitForCacheSync = func(<-chan struct{}) bool { return true } diff --git a/pkg/log/log_test.go b/pkg/log/log_test.go index 1ec5f51e0c..fa5ff23bca 100644 --- a/pkg/log/log_test.go +++ b/pkg/log/log_test.go @@ -17,7 +17,6 @@ limitations under the License. package log import ( - "github.com/go-logr/logr" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" diff --git a/pkg/log/zap/kube_helpers.go b/pkg/log/zap/kube_helpers.go index c88ba56cdc..2c0d88386d 100644 --- a/pkg/log/zap/kube_helpers.go +++ b/pkg/log/zap/kube_helpers.go @@ -125,4 +125,3 @@ func (k *KubeAwareEncoder) EncodeEntry(entry zapcore.Entry, fields []zapcore.Fie return k.Encoder.EncodeEntry(entry, fields) } - diff --git a/pkg/manager/example_test.go b/pkg/manager/example_test.go index 44a2e511bd..093e4f1f0e 100644 --- a/pkg/manager/example_test.go +++ b/pkg/manager/example_test.go @@ -20,8 +20,8 @@ import ( "os" "sigs.k8s.io/controller-runtime/pkg/client/config" - "sigs.k8s.io/controller-runtime/pkg/manager" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/manager/signals" ) diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index 716702c5ca..1368372f6e 100644 --- a/pkg/manager/internal.go +++ b/pkg/manager/internal.go @@ -33,10 +33,10 @@ import ( "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/internal/log" "sigs.k8s.io/controller-runtime/pkg/metrics" "sigs.k8s.io/controller-runtime/pkg/recorder" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) diff --git a/pkg/manager/manager_suite_test.go b/pkg/manager/manager_suite_test.go index a1183f66c0..b544fa8ae6 100644 --- a/pkg/manager/manager_suite_test.go +++ b/pkg/manager/manager_suite_test.go @@ -24,9 +24,9 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/envtest" - "sigs.k8s.io/controller-runtime/pkg/metrics" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/metrics" ) func TestSource(t *testing.T) { diff --git a/pkg/metrics/client_go_adapter.go b/pkg/metrics/client_go_adapter.go index d02c56356f..5dfc120397 100644 --- a/pkg/metrics/client_go_adapter.go +++ b/pkg/metrics/client_go_adapter.go @@ -110,35 +110,35 @@ var ( workQueueSubsystem = "workqueue" depth = prometheus.NewGaugeVec(prometheus.GaugeOpts{ - Subsystem: workQueueSubsystem, - Name: "depth", - Help: "Current depth of workqueue", + Subsystem: workQueueSubsystem, + Name: "depth", + Help: "Current depth of workqueue", }, []string{"name"}) adds = prometheus.NewCounterVec(prometheus.CounterOpts{ - Subsystem: workQueueSubsystem, - Name: "adds_total", - Help: "Total number of adds handled by workqueue", + Subsystem: workQueueSubsystem, + Name: "adds_total", + Help: "Total number of adds handled by workqueue", }, []string{"name"}) latency = prometheus.NewHistogramVec(prometheus.HistogramOpts{ - Subsystem: workQueueSubsystem, - Name: "queue_latency_seconds", - Help: "How long in seconds an item stays in workqueue before being requested.", - Buckets: prometheus.ExponentialBuckets(10e-9, 10, 10), + Subsystem: workQueueSubsystem, + Name: "queue_latency_seconds", + Help: "How long in seconds an item stays in workqueue before being requested.", + Buckets: prometheus.ExponentialBuckets(10e-9, 10, 10), }, []string{"name"}) workDuration = prometheus.NewHistogramVec(prometheus.HistogramOpts{ - Subsystem: workQueueSubsystem, - Name: "work_duration_seconds", - Help: "How long in seconds processing an item from workqueue takes.", - Buckets: prometheus.ExponentialBuckets(10e-9, 10, 10), + Subsystem: workQueueSubsystem, + Name: "work_duration_seconds", + Help: "How long in seconds processing an item from workqueue takes.", + Buckets: prometheus.ExponentialBuckets(10e-9, 10, 10), }, []string{"name"}) retries = prometheus.NewCounterVec(prometheus.CounterOpts{ - Subsystem: workQueueSubsystem, - Name: "retries_total", - Help: "Total number of retries handled by workqueue", + Subsystem: workQueueSubsystem, + Name: "retries_total", + Help: "Total number of retries handled by workqueue", }, []string{"name"}) longestRunning = prometheus.NewGaugeVec(prometheus.GaugeOpts{ diff --git a/pkg/recorder/example_test.go b/pkg/recorder/example_test.go index 3066c1c269..c900423d77 100644 --- a/pkg/recorder/example_test.go +++ b/pkg/recorder/example_test.go @@ -27,7 +27,7 @@ import ( var ( recorderProvider recorder.Provider - somePod *corev1.Pod // the object you're reconciling, for example + somePod *corev1.Pod // the object you're reconciling, for example ) func Example_event() { @@ -54,7 +54,7 @@ func Example_pastEventf() { recorder := recorderProvider.GetEventRecorderFor("my-controller") // emit a backdated event (potentially with variable message) - recorder.PastEventf(somePod, metav1.Time{Time: time.Now().Add(-5*time.Minute)}, + recorder.PastEventf(somePod, metav1.Time{Time: time.Now().Add(-5 * time.Minute)}, corev1.EventTypeWarning, "ForgottenCrackers", "Crackers, Gromit! We forgot the crackers!") } diff --git a/pkg/runtime/scheme/scheme.go b/pkg/runtime/scheme/scheme.go index a1d00141b0..889308c1e0 100644 --- a/pkg/runtime/scheme/scheme.go +++ b/pkg/runtime/scheme/scheme.go @@ -20,4 +20,3 @@ limitations under the License. // // Deprecated: use pkg/scheme instead. package scheme - diff --git a/pkg/runtime/signals/signal.go b/pkg/runtime/signals/signal.go index 2b25fe73dd..64bbcef368 100644 --- a/pkg/runtime/signals/signal.go +++ b/pkg/runtime/signals/signal.go @@ -17,7 +17,7 @@ limitations under the License. // Package signals contains libraries for handling signals to gracefully // shutdown the manager in combination with Kubernetes pod graceful termination // policy. -// +// // Deprecated: use pkg/manager/signals instead. package signals diff --git a/pkg/source/example_test.go b/pkg/source/example_test.go index 01a17f89f5..afc40f50d8 100644 --- a/pkg/source/example_test.go +++ b/pkg/source/example_test.go @@ -17,7 +17,7 @@ limitations under the License. package source_test import ( - "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -29,7 +29,7 @@ var ctrl controller.Controller // This example Watches for Pod Events (e.g. Create / Update / Delete) and enqueues a reconcile.Request // with the Name and Namespace of the Pod. func ExampleKind() { - ctrl.Watch(&source.Kind{Type: &v1.Pod{}}, &handler.EnqueueRequestForObject{}) + ctrl.Watch(&source.Kind{Type: &corev1.Pod{}}, &handler.EnqueueRequestForObject{}) } // This example reads GenericEvents from a channel and enqueues a reconcile.Request containing the Name and Namespace diff --git a/pkg/source/source.go b/pkg/source/source.go index 3375616e1d..3d27bc4334 100644 --- a/pkg/source/source.go +++ b/pkg/source/source.go @@ -25,8 +25,8 @@ import ( "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" logf "sigs.k8s.io/controller-runtime/pkg/internal/log" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/source/internal" toolscache "k8s.io/client-go/tools/cache" diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index 38daa91dd1..bd065b71e6 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -187,8 +187,6 @@ func (s *Server) Start(stop <-chan struct{}) error { return nil } -var _ inject.Injector = &Server{} - // InjectFunc injects dependencies into the handlers. func (s *Server) InjectFunc(f inject.Func) error { s.setFields = f From eee0406e8dbb7d8d82bb8384fb9d795b18a743d4 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Wed, 6 Feb 2019 18:52:41 -0800 Subject: [PATCH 03/19] Fix example_test.go to use new list options The example was still using the old list options arguments style (second argument instead of last argument), which made it fail to compile. This also makes sure we run go test on the whole project, instead of just pkg/... --- example_test.go | 2 +- hack/test-all.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/example_test.go b/example_test.go index dde560cf63..be587f5b0b 100644 --- a/example_test.go +++ b/example_test.go @@ -81,7 +81,7 @@ func (a *ReplicaSetReconciler) Reconcile(req controllers.Request) (controllers.R // List the Pods matching the PodTemplate Labels pods := &corev1.PodList{} - err = a.List(context.TODO(), client.InNamespace(req.Namespace).MatchingLabels(rs.Spec.Template.Labels), pods) + err = a.List(context.TODO(), pods, client.InNamespace(req.Namespace), client.MatchingLabels(rs.Spec.Template.Labels)) if err != nil { return controllers.Result{}, err } diff --git a/hack/test-all.sh b/hack/test-all.sh index cb840b1506..df76a1a0f1 100755 --- a/hack/test-all.sh +++ b/hack/test-all.sh @@ -22,7 +22,7 @@ setup_envs header_text "running go test" -go test ./pkg/... -parallel 4 +go test ./... -parallel 4 header_text "running coverage" From 5963ad2c8a487caefa1d99a45b0cb837f4bb385f Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Wed, 6 Feb 2019 18:56:35 -0800 Subject: [PATCH 04/19] Fix admission webhook injection, no mgr, no client The webhook server logic was not handling injection correctly -- it implemented injection for a couple of specific fields, rather than receiving and dealing with an injectfunc. This fixes that. As a side effect, it also removes the handle to manager and client from the webhook server, since those aren't used by the server, and should thus be injected from the manager. --- example/main.go | 3 ++- pkg/webhook/admission/webhook.go | 8 +++++--- pkg/webhook/server.go | 27 +++++++++++++-------------- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/example/main.go b/example/main.go index 55188fc739..7cd95f815e 100644 --- a/example/main.go +++ b/example/main.go @@ -92,7 +92,7 @@ func main() { Build() entryLog.Info("setting up webhook server") - as, err := webhook.NewServer(mgr, webhook.ServerOptions{ + as, err := webhook.NewServer(webhook.ServerOptions{ Port: 9876, CertDir: "/tmp/cert", }) @@ -100,6 +100,7 @@ func main() { entryLog.Error(err, "unable to create a new webhook server") os.Exit(1) } + mgr.Add(as) entryLog.Info("registering webhooks to the webhook server") err = as.Register(mutatingWebhook, validatingWebhook) diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 632160c0cb..f814049f18 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -209,10 +209,12 @@ func (w *Webhook) Validate() error { return nil } -var _ inject.Injector = &Webhook{} - -// InjectFunc injects dependencies into the handlers. +// InjectFunc injects the field setter into the webhook. func (w *Webhook) InjectFunc(f inject.Func) error { + // inject directly into the handlers. It would be more correct + // to do this in a sync.Once in Handle (since we don't have some + // other start/finalize-type method), but it's more efficient to + // do it here, presumably. for _, handler := range w.Handlers { if err := f(handler); err != nil { return err diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index bd065b71e6..4314bc5a08 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -25,7 +25,6 @@ import ( "strconv" "sync" - "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) @@ -62,11 +61,8 @@ type Server struct { // registry maps a path to a http.Handler. registry map[string]http.Handler - // setFields is used to inject dependencies into webhooks - setFields func(i interface{}) error - - // manager is the manager that this webhook server will be registered. - manager manager.Manager + // setFields allows injecting dependencies from an external source + setFields inject.Func once sync.Once } @@ -85,12 +81,11 @@ type Webhook interface { } // NewServer creates a new admission webhook server. -func NewServer(mgr manager.Manager, options ServerOptions) (*Server, error) { +func NewServer(options ServerOptions) (*Server, error) { as := &Server{ sMux: http.NewServeMux(), registry: map[string]http.Handler{}, ServerOptions: options, - manager: mgr, } return as, nil @@ -130,9 +125,7 @@ func (s *Server) Register(webhooks ...Webhook) error { } } - // Lazily add Server to manager. - // Because the all webhook handlers to be in place, so we can inject the things they need. - return s.manager.Add(s) + return nil } // Handle registers a http.Handler for the given pattern. @@ -140,13 +133,19 @@ func (s *Server) Handle(pattern string, handler http.Handler) { s.sMux.Handle(pattern, handler) } -var _ manager.Runnable = &Server{} - // Start runs the server. // It will install the webhook related resources depend on the server configuration. func (s *Server) Start(stop <-chan struct{}) error { s.once.Do(s.setDefault) + // inject fields here as opposed to in Register so that we're certain to have our setFields + // function available. + for _, webhook := range s.registry { + if err := s.setFields(webhook); err != nil { + return err + } + } + // TODO: watch the cert dir. Reload the cert if it changes cert, err := tls.LoadX509KeyPair(path.Join(s.CertDir, certName), path.Join(s.CertDir, keyName)) if err != nil { @@ -187,7 +186,7 @@ func (s *Server) Start(stop <-chan struct{}) error { return nil } -// InjectFunc injects dependencies into the handlers. +// InjectFunc injects the field setter into the server. func (s *Server) InjectFunc(f inject.Func) error { s.setFields = f return nil From 14f4abf388b2159396fa92b5526ff8ba678c1368 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Thu, 7 Feb 2019 00:05:49 -0800 Subject: [PATCH 05/19] Flatten webhook types struct This flattens down the webhook package structure, removing dedicated `types` packages in favor of having things in the relevant places. To do this, the inject interface definitions for admission were also moved to the admission controller location, but they actually make some amount of sense living there. It also renames and restructures a couple of the types for code clarity (e.g. WebhookTypeMutating to MutatingWebhook) or to follow the convention of other types in CR (e.g. Making wrapped types from core Kubernetes nested fields). --- example/mutatingwebhook.go | 10 ++-- example/validatingwebhook.go | 10 ++-- pkg/manager/internal.go | 10 ---- pkg/manager/manager.go | 16 ------ pkg/runtime/inject/inject.go | 15 ------ pkg/webhook/admission/builder/builder.go | 7 ++- pkg/webhook/admission/decode.go | 13 +++-- pkg/webhook/admission/decode_test.go | 7 ++- pkg/webhook/admission/http.go | 19 ++++--- pkg/webhook/admission/http_test.go | 29 ++--------- pkg/webhook/admission/inject.go | 31 +++++++++++ pkg/webhook/admission/response.go | 21 ++++---- pkg/webhook/admission/response_test.go | 13 +++-- pkg/webhook/admission/types.go | 66 ++++++++++++++++++++++++ pkg/webhook/admission/types/types.go | 44 ---------------- pkg/webhook/admission/webhook.go | 53 +++++++++---------- pkg/webhook/admission/webhook_test.go | 48 +++++++++-------- pkg/webhook/types/webhook.go | 28 ---------- 18 files changed, 194 insertions(+), 246 deletions(-) create mode 100644 pkg/webhook/admission/inject.go create mode 100644 pkg/webhook/admission/types.go delete mode 100644 pkg/webhook/admission/types/types.go delete mode 100644 pkg/webhook/types/webhook.go diff --git a/example/mutatingwebhook.go b/example/mutatingwebhook.go index f7e0437b14..945a9f8951 100644 --- a/example/mutatingwebhook.go +++ b/example/mutatingwebhook.go @@ -23,22 +23,20 @@ import ( corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) // podAnnotator annotates Pods type podAnnotator struct { client client.Client - decoder types.Decoder + decoder admission.Decoder } // Implement admission.Handler so the controller can handle admission request. var _ admission.Handler = &podAnnotator{} // podAnnotator adds an annotation to every incoming pods. -func (a *podAnnotator) Handle(ctx context.Context, req types.Request) types.Response { +func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admission.Response { pod := &corev1.Pod{} err := a.decoder.Decode(req, pod) @@ -70,7 +68,6 @@ func (a *podAnnotator) mutatePodsFn(ctx context.Context, pod *corev1.Pod) error // podAnnotator implements inject.Client. // A client will be automatically injected. -var _ inject.Client = &podAnnotator{} // InjectClient injects the client. func (a *podAnnotator) InjectClient(c client.Client) error { @@ -80,10 +77,9 @@ func (a *podAnnotator) InjectClient(c client.Client) error { // podAnnotator implements inject.Decoder. // A decoder will be automatically injected. -var _ inject.Decoder = &podAnnotator{} // InjectDecoder injects the decoder. -func (a *podAnnotator) InjectDecoder(d types.Decoder) error { +func (a *podAnnotator) InjectDecoder(d admission.Decoder) error { a.decoder = d return nil } diff --git a/example/validatingwebhook.go b/example/validatingwebhook.go index 5019cb611c..94b8d9160b 100644 --- a/example/validatingwebhook.go +++ b/example/validatingwebhook.go @@ -23,22 +23,20 @@ import ( corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) // podValidator validates Pods type podValidator struct { client client.Client - decoder types.Decoder + decoder admission.Decoder } // Implement admission.Handler so the controller can handle admission request. var _ admission.Handler = &podValidator{} // podValidator admits a pod iff a specific annotation exists. -func (v *podValidator) Handle(ctx context.Context, req types.Request) types.Response { +func (v *podValidator) Handle(ctx context.Context, req admission.Request) admission.Response { pod := &corev1.Pod{} err := v.decoder.Decode(req, pod) @@ -70,7 +68,6 @@ func (v *podValidator) validatePodsFn(ctx context.Context, pod *corev1.Pod) (boo // podValidator implements inject.Client. // A client will be automatically injected. -var _ inject.Client = &podValidator{} // InjectClient injects the client. func (v *podValidator) InjectClient(c client.Client) error { @@ -80,10 +77,9 @@ func (v *podValidator) InjectClient(c client.Client) error { // podValidator implements inject.Decoder. // A decoder will be automatically injected. -var _ inject.Decoder = &podValidator{} // InjectDecoder injects the decoder. -func (v *podValidator) InjectDecoder(d types.Decoder) error { +func (v *podValidator) InjectDecoder(d admission.Decoder) error { v.decoder = d return nil } diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index 1368372f6e..230b78f89c 100644 --- a/pkg/manager/internal.go +++ b/pkg/manager/internal.go @@ -37,7 +37,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/metrics" "sigs.k8s.io/controller-runtime/pkg/recorder" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) var log = logf.RuntimeLog.WithName("manager") @@ -49,8 +48,6 @@ type controllerManager struct { // scheme is the scheme injected into Controllers, EventHandlers, Sources and Predicates. Defaults // to scheme.scheme. scheme *runtime.Scheme - // admissionDecoder is used to decode an admission.Request. - admissionDecoder types.Decoder // runnables is the set of Controllers that the controllerManager injects deps into and Starts. runnables []Runnable @@ -136,9 +133,6 @@ func (cm *controllerManager) SetFields(i interface{}) error { if _, err := inject.StopChannelInto(cm.internalStop, i); err != nil { return err } - if _, err := inject.DecoderInto(cm.admissionDecoder, i); err != nil { - return err - } if _, err := inject.MapperInto(cm.mapper, i); err != nil { return err } @@ -157,10 +151,6 @@ func (cm *controllerManager) GetScheme() *runtime.Scheme { return cm.scheme } -func (cm *controllerManager) GetAdmissionDecoder() types.Decoder { - return cm.admissionDecoder -} - func (cm *controllerManager) GetFieldIndexer() client.FieldIndexer { return cm.fieldIndexes } diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index ea25e9f509..633091eb78 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -36,8 +36,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/leaderelection" "sigs.k8s.io/controller-runtime/pkg/metrics" "sigs.k8s.io/controller-runtime/pkg/recorder" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) // Manager initializes shared dependencies such as Caches and Clients, and provides them to Runnables. @@ -62,9 +60,6 @@ type Manager interface { // GetScheme returns an initialized Scheme GetScheme() *runtime.Scheme - // GetAdmissionDecoder returns the runtime.Decoder based on the scheme. - GetAdmissionDecoder() types.Decoder - // GetClient returns a client configured with the Config GetClient() client.Client @@ -135,7 +130,6 @@ type Options struct { // Dependency injection for testing newRecorderProvider func(config *rest.Config, scheme *runtime.Scheme, logger logr.Logger) (recorder.Provider, error) newResourceLock func(config *rest.Config, recorderProvider recorder.Provider, options leaderelection.Options) (resourcelock.Interface, error) - newAdmissionDecoder func(scheme *runtime.Scheme) (types.Decoder, error) newMetricsListener func(addr string) (net.Listener, error) } @@ -210,11 +204,6 @@ func New(config *rest.Config, options Options) (Manager, error) { return nil, err } - admissionDecoder, err := options.newAdmissionDecoder(options.Scheme) - if err != nil { - return nil, err - } - // Create the mertics listener. This will throw an error if the metrics bind // address is invalid or already in use. metricsListener, err := options.newMetricsListener(options.MetricsBindAddress) @@ -227,7 +216,6 @@ func New(config *rest.Config, options Options) (Manager, error) { return &controllerManager{ config: config, scheme: options.Scheme, - admissionDecoder: admissionDecoder, errChan: make(chan error), cache: cache, fieldIndexes: cache, @@ -290,10 +278,6 @@ func setOptionsDefaults(options Options) Options { options.newResourceLock = leaderelection.NewResourceLock } - if options.newAdmissionDecoder == nil { - options.newAdmissionDecoder = admission.NewDecoder - } - if options.newMetricsListener == nil { options.newMetricsListener = metrics.NewListener } diff --git a/pkg/runtime/inject/inject.go b/pkg/runtime/inject/inject.go index 7d1fbd4a6d..ec7f30c68d 100644 --- a/pkg/runtime/inject/inject.go +++ b/pkg/runtime/inject/inject.go @@ -22,7 +22,6 @@ import ( "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) // Cache is used by the ControllerManager to inject Cache into Sources, EventHandlers, Predicates, and @@ -70,20 +69,6 @@ func ClientInto(client client.Client, i interface{}) (bool, error) { return false, nil } -// Decoder is used by the ControllerManager to inject decoder into webhook handlers. -type Decoder interface { - InjectDecoder(types.Decoder) error -} - -// DecoderInto will set decoder on i and return the result if it implements Decoder. Returns -// false if i does not implement Decoder. -func DecoderInto(decoder types.Decoder, i interface{}) (bool, error) { - if s, ok := i.(Decoder); ok { - return true, s.InjectDecoder(decoder) - } - return false, nil -} - // Scheme is used by the ControllerManager to inject Scheme into Sources, EventHandlers, Predicates, and // Reconciles type Scheme interface { diff --git a/pkg/webhook/admission/builder/builder.go b/pkg/webhook/admission/builder/builder.go index 4f7e12e20a..23e8c0d14e 100644 --- a/pkg/webhook/admission/builder/builder.go +++ b/pkg/webhook/admission/builder/builder.go @@ -18,7 +18,6 @@ package builder import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "sigs.k8s.io/controller-runtime/pkg/webhook/types" ) // WebhookBuilder builds a webhook based on the provided options. @@ -37,7 +36,7 @@ type WebhookBuilder struct { // t specifies the type of the webhook. // Currently, Mutating and Validating are supported. - t *types.WebhookType + t *admission.WebhookType } // NewWebhookBuilder creates an empty WebhookBuilder. @@ -55,7 +54,7 @@ func (b *WebhookBuilder) Name(name string) *WebhookBuilder { // Mutating sets the type to mutating admission webhook // Only one of Mutating and Validating can be invoked. func (b *WebhookBuilder) Mutating() *WebhookBuilder { - m := types.WebhookTypeMutating + m := admission.MutatingWebhook b.t = &m return b } @@ -63,7 +62,7 @@ func (b *WebhookBuilder) Mutating() *WebhookBuilder { // Validating sets the type to validating admission webhook // Only one of Mutating and Validating can be invoked. func (b *WebhookBuilder) Validating() *WebhookBuilder { - m := types.WebhookTypeValidating + m := admission.ValidatingWebhook b.t = &m return b } diff --git a/pkg/webhook/admission/decode.go b/pkg/webhook/admission/decode.go index 1aea214cfd..5001247e1e 100644 --- a/pkg/webhook/admission/decode.go +++ b/pkg/webhook/admission/decode.go @@ -19,16 +19,15 @@ package admission import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) -// DecodeFunc is a function that implements the Decoder interface. -type DecodeFunc func(types.Request, runtime.Object) error +// DecodeFunc is a function that implements an admission decoder in a single function. +type DecodeFunc func(Request, runtime.Object) error -var _ types.Decoder = DecodeFunc(nil) +var _ Decoder = DecodeFunc(nil) // Decode implements the Decoder interface. -func (f DecodeFunc) Decode(req types.Request, obj runtime.Object) error { +func (f DecodeFunc) Decode(req Request, obj runtime.Object) error { return f(req, obj) } @@ -37,12 +36,12 @@ type decoder struct { } // NewDecoder creates a Decoder given the runtime.Scheme -func NewDecoder(scheme *runtime.Scheme) (types.Decoder, error) { +func NewDecoder(scheme *runtime.Scheme) (Decoder, error) { return decoder{codecs: serializer.NewCodecFactory(scheme)}, nil } // Decode decodes the inlined object in the AdmissionRequest into the passed-in runtime.Object. -func (d decoder) Decode(req types.Request, into runtime.Object) error { +func (d decoder) Decode(req Request, into runtime.Object) error { deserializer := d.codecs.UniversalDeserializer() return runtime.DecodeInto(deserializer, req.AdmissionRequest.Object.Raw, into) } diff --git a/pkg/webhook/admission/decode_test.go b/pkg/webhook/admission/decode_test.go index d0d38c7cb0..c0a9bbfe0f 100644 --- a/pkg/webhook/admission/decode_test.go +++ b/pkg/webhook/admission/decode_test.go @@ -24,11 +24,10 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) var _ = Describe("admission webhook decoder", func() { - var decoder types.Decoder + var decoder Decoder BeforeEach(func(done Done) { var err error decoder, err = NewDecoder(scheme.Scheme) @@ -46,8 +45,8 @@ var _ = Describe("admission webhook decoder", func() { }) Describe("Decode", func() { - req := types.Request{ - AdmissionRequest: &admissionv1beta1.AdmissionRequest{ + req := Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ Object: runtime.RawExtension{ Raw: []byte(`{ "apiVersion": "v1", diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 5b91ad2b50..1e24a2e2ca 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -31,7 +31,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" "sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics" ) @@ -55,7 +54,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { var body []byte var err error - var reviewResponse types.Response + var reviewResponse Response if r.Body != nil { if body, err = ioutil.ReadAll(r.Body); err != nil { log.Error(err, "unable to read the body from the incoming request") @@ -81,7 +80,11 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - ar := v1beta1.AdmissionReview{} + req := Request{} + ar := v1beta1.AdmissionReview{ + // avoid an extra copy + Request: &req.AdmissionRequest, + } if _, _, err := admissionv1beta1schemecodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil { log.Error(err, "unable to decode the request") reviewResponse = ErrorResponse(http.StatusBadRequest, err) @@ -90,13 +93,13 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { } // TODO: add panic-recovery for Handle - reviewResponse = wh.Handle(context.Background(), types.Request{AdmissionRequest: ar.Request}) + reviewResponse = wh.Handle(context.Background(), req) wh.writeResponse(w, reviewResponse) } -func (wh *Webhook) writeResponse(w io.Writer, response types.Response) { - if response.Response.Result.Code != 0 { - if response.Response.Result.Code == http.StatusOK { +func (wh *Webhook) writeResponse(w io.Writer, response Response) { + if response.Result.Code != 0 { + if response.Result.Code == http.StatusOK { metrics.TotalRequests.WithLabelValues(wh.Name, "true").Inc() } else { metrics.TotalRequests.WithLabelValues(wh.Name, "false").Inc() @@ -105,7 +108,7 @@ func (wh *Webhook) writeResponse(w io.Writer, response types.Response) { encoder := json.NewEncoder(w) responseAdmissionReview := v1beta1.AdmissionReview{ - Response: response.Response, + Response: &response.AdmissionResponse, } err := encoder.Encode(responseAdmissionReview) if err != nil { diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index 8d3c0507b4..afe59529b7 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -27,8 +27,6 @@ import ( . "github.com/onsi/gomega" admissionv1beta1 "k8s.io/api/admission/v1beta1" - atypes "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" - "sigs.k8s.io/controller-runtime/pkg/webhook/types" ) var _ = Describe("admission webhook http handler", func() { @@ -77,7 +75,7 @@ var _ = Describe("admission webhook http handler", func() { Body: nopCloser{Reader: bytes.NewBufferString("{")}, } wh := &Webhook{ - Type: types.WebhookTypeMutating, + Type: MutatingWebhook, Handlers: []Handler{}, } expected := []byte( @@ -90,23 +88,6 @@ var _ = Describe("admission webhook http handler", func() { }) }) - Describe("empty body after decoding", func() { - req := &http.Request{ - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: nopCloser{Reader: bytes.NewBuffer(nil)}, - } - wh := &Webhook{ - Type: types.WebhookTypeMutating, - Handlers: []Handler{}, - } - expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"got an empty AdmissionRequest","code":400}}} -`) - It("should return an error with bad-request status code", func() { - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) - }) - }) - Describe("no webhook type", func() { req := &http.Request{ Header: http.Header{"Content-Type": []string{"application/json"}}, @@ -131,7 +112,7 @@ var _ = Describe("admission webhook http handler", func() { } h := &fakeHandler{} wh := &Webhook{ - Type: types.WebhookTypeValidating, + Type: ValidatingWebhook, Handlers: []Handler{h}, } expected := []byte(`{"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}} @@ -152,15 +133,15 @@ func (nopCloser) Close() error { return nil } type fakeHandler struct { invoked bool - fn func(context.Context, atypes.Request) atypes.Response + fn func(context.Context, Request) Response } -func (h *fakeHandler) Handle(ctx context.Context, req atypes.Request) atypes.Response { +func (h *fakeHandler) Handle(ctx context.Context, req Request) Response { h.invoked = true if h.fn != nil { return h.fn(ctx, req) } - return atypes.Response{Response: &admissionv1beta1.AdmissionResponse{ + return Response{AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, }} } diff --git a/pkg/webhook/admission/inject.go b/pkg/webhook/admission/inject.go new file mode 100644 index 0000000000..4127754ae5 --- /dev/null +++ b/pkg/webhook/admission/inject.go @@ -0,0 +1,31 @@ +/* +Copyright 2019 The Kubernetes 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 admission + +// Decoder is used by the ControllerManager to inject decoder into webhook handlers. +type DecoderInjector interface { + InjectDecoder(Decoder) error +} + +// InjectDecoderInto will set decoder on i and return the result if it implements Decoder. Returns +// false if i does not implement Decoder. +func InjectDecoderInto(decoder Decoder, i interface{}) (bool, error) { + if s, ok := i.(DecoderInjector); ok { + return true, s.InjectDecoder(decoder) + } + return false, nil +} diff --git a/pkg/webhook/admission/response.go b/pkg/webhook/admission/response.go index 74f55a4ca3..c76ad3b63c 100644 --- a/pkg/webhook/admission/response.go +++ b/pkg/webhook/admission/response.go @@ -23,13 +23,12 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) // ErrorResponse creates a new Response for error-handling a request. -func ErrorResponse(code int32, err error) types.Response { - return types.Response{ - Response: &admissionv1beta1.AdmissionResponse{ +func ErrorResponse(code int32, err error) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: false, Result: &metav1.Status{ Code: code, @@ -40,14 +39,14 @@ func ErrorResponse(code int32, err error) types.Response { } // ValidationResponse returns a response for admitting a request. -func ValidationResponse(allowed bool, reason string) types.Response { - resp := types.Response{ - Response: &admissionv1beta1.AdmissionResponse{ +func ValidationResponse(allowed bool, reason string) Response { + resp := Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: allowed, }, } if len(reason) > 0 { - resp.Response.Result = &metav1.Status{ + resp.Result = &metav1.Status{ Reason: metav1.StatusReason(reason), } } @@ -57,14 +56,14 @@ func ValidationResponse(allowed bool, reason string) types.Response { // PatchResponseFromRaw takes 2 byte arrays and returns a new response with json patch. // The original object should be passed in as raw bytes to avoid the roundtripping problem // described in https://github.com/kubernetes-sigs/kubebuilder/issues/510. -func PatchResponseFromRaw(original, current []byte) types.Response { +func PatchResponseFromRaw(original, current []byte) Response { patches, err := jsonpatch.CreatePatch(original, current) if err != nil { return ErrorResponse(http.StatusInternalServerError, err) } - return types.Response{ + return Response{ Patches: patches, - Response: &admissionv1beta1.AdmissionResponse{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), }, diff --git a/pkg/webhook/admission/response_test.go b/pkg/webhook/admission/response_test.go index fcbecce942..124db31d3a 100644 --- a/pkg/webhook/admission/response_test.go +++ b/pkg/webhook/admission/response_test.go @@ -26,15 +26,14 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" ) var _ = Describe("admission webhook response", func() { Describe("ErrorResponse", func() { It("should return the response with an error", func() { err := errors.New("this is an error") - expected := types.Response{ - Response: &admissionv1beta1.AdmissionResponse{ + expected := Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: false, Result: &metav1.Status{ Code: http.StatusBadRequest, @@ -49,8 +48,8 @@ var _ = Describe("admission webhook response", func() { Describe("ValidationResponse", func() { It("should return the response with an admission decision", func() { - expected := types.Response{ - Response: &admissionv1beta1.AdmissionResponse{ + expected := Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ Reason: metav1.StatusReason("allow to admit"), @@ -64,9 +63,9 @@ var _ = Describe("admission webhook response", func() { Describe("PatchResponse", func() { It("should return the response with patches", func() { - expected := types.Response{ + expected := Response{ Patches: []jsonpatch.JsonPatchOperation{}, - Response: &admissionv1beta1.AdmissionResponse{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), }, diff --git a/pkg/webhook/admission/types.go b/pkg/webhook/admission/types.go new file mode 100644 index 0000000000..ee24e263cd --- /dev/null +++ b/pkg/webhook/admission/types.go @@ -0,0 +1,66 @@ +/* +Copyright 2018 The Kubernetes 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 admission + +import ( + "github.com/appscode/jsonpatch" + + admissionv1beta1 "k8s.io/api/admission/v1beta1" + "k8s.io/apimachinery/pkg/runtime" +) + +// Request defines the input for an admission handler. +// It contains information to identify the object in +// question (group, version, kind, resource, subresource, +// name, namespace), as well as the operation in question +// (e.g. Get, Create, etc), and the object itself. +type Request struct { + admissionv1beta1.AdmissionRequest +} + +// Response is the output of an admission handler. +// It contains a response indicating if a given +// operation is allowed, as well as a set of patches +// to mutate the object in the case of a mutating admission handler. +type Response struct { + // Patches are the JSON patches for mutating webhooks. + // Using this instead of setting Response.Patch to minimize + // overhead of serialization and deserialization. + Patches []jsonpatch.JsonPatchOperation + // AdmissionResponse is the raw admission response. + // The Patch field in it will be overwritten by the listed patches. + admissionv1beta1.AdmissionResponse +} + +// Decoder is used to decode AdmissionRequest. +type Decoder interface { + // Decode decodes the raw byte object from the AdmissionRequest to the passed-in runtime.Object. + Decode(Request, runtime.Object) error +} + +// WebhookType defines the type of an admission webhook +// (validating or mutating). +type WebhookType int + +const ( + _ WebhookType = iota + // MutatingWebhook represents a webhook that can mutate the object sent to it. + MutatingWebhook + // ValidatingWebhook represents a webhook that can only gate whether or not objects + // sent to it are accepted. + ValidatingWebhook +) diff --git a/pkg/webhook/admission/types/types.go b/pkg/webhook/admission/types/types.go deleted file mode 100644 index 75236779ff..0000000000 --- a/pkg/webhook/admission/types/types.go +++ /dev/null @@ -1,44 +0,0 @@ -/* -Copyright 2018 The Kubernetes 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 types - -import ( - "github.com/appscode/jsonpatch" - - admissionv1beta1 "k8s.io/api/admission/v1beta1" - "k8s.io/apimachinery/pkg/runtime" -) - -// Request is the input of Handler -type Request struct { - AdmissionRequest *admissionv1beta1.AdmissionRequest -} - -// Response is the output of admission.Handler -type Response struct { - // Patches are the JSON patches for mutating webhooks. - // Using this instead of setting Response.Patch to minimize the overhead of serialization and deserialization. - Patches []jsonpatch.JsonPatchOperation - // Response is the admission response. Don't set the Patch field in it. - Response *admissionv1beta1.AdmissionResponse -} - -// Decoder is used to decode AdmissionRequest. -type Decoder interface { - // Decode decodes the raw byte object from the AdmissionRequest to the passed-in runtime.Object. - Decode(Request, runtime.Object) error -} diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index f814049f18..28036b5fe2 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -31,22 +31,20 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" - atypes "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" - "sigs.k8s.io/controller-runtime/pkg/webhook/types" ) // Handler can handle an AdmissionRequest. type Handler interface { - Handle(context.Context, atypes.Request) atypes.Response + Handle(context.Context, Request) Response } // HandlerFunc implements Handler interface using a single function. -type HandlerFunc func(context.Context, atypes.Request) atypes.Response +type HandlerFunc func(context.Context, Request) Response var _ Handler = HandlerFunc(nil) // Handle process the AdmissionRequest by invoking the underlying function. -func (f HandlerFunc) Handle(ctx context.Context, req atypes.Request) atypes.Response { +func (f HandlerFunc) Handle(ctx context.Context, req Request) Response { return f(ctx, req) } @@ -55,7 +53,7 @@ type Webhook struct { // Name is the name of the webhook Name string // Type is the webhook type, i.e. mutating, validating - Type types.WebhookType + Type WebhookType // Path is the path this webhook will serve. Path string // Handlers contains a list of handlers. Each handler may only contains the business logic for its own feature. @@ -88,35 +86,32 @@ var _ Handler = &Webhook{} // If the webhook is mutating type, it delegates the AdmissionRequest to each handler and merge the patches. // If the webhook is validating type, it delegates the AdmissionRequest to each handler and // deny the request if anyone denies. -func (w *Webhook) Handle(ctx context.Context, req atypes.Request) atypes.Response { - if req.AdmissionRequest == nil { - return ErrorResponse(http.StatusBadRequest, errors.New("got an empty AdmissionRequest")) - } - var resp atypes.Response +func (w *Webhook) Handle(ctx context.Context, req Request) Response { + var resp Response switch w.Type { - case types.WebhookTypeMutating: + case MutatingWebhook: resp = w.handleMutating(ctx, req) - case types.WebhookTypeValidating: + case ValidatingWebhook: resp = w.handleValidating(ctx, req) default: return ErrorResponse(http.StatusInternalServerError, errors.New("you must specify your webhook type")) } - resp.Response.UID = req.AdmissionRequest.UID + resp.UID = req.AdmissionRequest.UID return resp } -func (w *Webhook) handleMutating(ctx context.Context, req atypes.Request) atypes.Response { +func (w *Webhook) handleMutating(ctx context.Context, req Request) Response { patches := []jsonpatch.JsonPatchOperation{} for _, handler := range w.Handlers { resp := handler.Handle(ctx, req) - if !resp.Response.Allowed { - setStatusOKInAdmissionResponse(resp.Response) + if !resp.Allowed { + setStatusOKInAdmissionResponse(&resp.AdmissionResponse) return resp } - if resp.Response.PatchType != nil && *resp.Response.PatchType != admissionv1beta1.PatchTypeJSONPatch { + if resp.PatchType != nil && *resp.PatchType != admissionv1beta1.PatchTypeJSONPatch { return ErrorResponse(http.StatusInternalServerError, fmt.Errorf("unexpected patch type returned by the handler: %v, only allow: %v", - resp.Response.PatchType, admissionv1beta1.PatchTypeJSONPatch)) + resp.PatchType, admissionv1beta1.PatchTypeJSONPatch)) } patches = append(patches, resp.Patches...) } @@ -125,8 +120,8 @@ func (w *Webhook) handleMutating(ctx context.Context, req atypes.Request) atypes if err != nil { return ErrorResponse(http.StatusBadRequest, fmt.Errorf("error when marshaling the patch: %v", err)) } - return atypes.Response{ - Response: &admissionv1beta1.AdmissionResponse{ + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ Code: http.StatusOK, @@ -137,16 +132,16 @@ func (w *Webhook) handleMutating(ctx context.Context, req atypes.Request) atypes } } -func (w *Webhook) handleValidating(ctx context.Context, req atypes.Request) atypes.Response { +func (w *Webhook) handleValidating(ctx context.Context, req Request) Response { for _, handler := range w.Handlers { resp := handler.Handle(ctx, req) - if !resp.Response.Allowed { - setStatusOKInAdmissionResponse(resp.Response) + if !resp.Allowed { + setStatusOKInAdmissionResponse(&resp.AdmissionResponse) return resp } } - return atypes.Response{ - Response: &admissionv1beta1.AdmissionResponse{ + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, Result: &metav1.Status{ Code: http.StatusOK, @@ -180,7 +175,7 @@ func (w *Webhook) GetPath() string { } // GetType returns the type of the webhook. -func (w *Webhook) GetType() types.WebhookType { +func (w *Webhook) GetType() WebhookType { w.once.Do(w.setDefaults) return w.Type } @@ -197,8 +192,8 @@ func (w *Webhook) Validate() error { if len(w.Name) == 0 { return errors.New("field Name should not be empty") } - if w.Type != types.WebhookTypeMutating && w.Type != types.WebhookTypeValidating { - return fmt.Errorf("unsupported Type: %v, only WebhookTypeMutating and WebhookTypeValidating are supported", w.Type) + if w.Type != MutatingWebhook && w.Type != ValidatingWebhook { + return fmt.Errorf("unsupported Type: %v, only MutatingWebhook and ValidatingWebhook are supported", w.Type) } if len(w.Path) == 0 { return errors.New("field Path should not be empty") diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index 1f6bddbcb3..9866b03462 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -30,8 +30,6 @@ import ( . "github.com/onsi/gomega" admissionv1beta1 "k8s.io/api/admission/v1beta1" - atypes "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" - "sigs.k8s.io/controller-runtime/pkg/webhook/types" ) var _ = Describe("admission webhook", func() { @@ -48,18 +46,18 @@ var _ = Describe("admission webhook", func() { var wh *Webhook BeforeEach(func(done Done) { alwaysAllow = &fakeHandler{ - fn: func(ctx context.Context, req atypes.Request) atypes.Response { - return atypes.Response{ - Response: &admissionv1beta1.AdmissionResponse{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, }, } }, } alwaysDeny = &fakeHandler{ - fn: func(ctx context.Context, req atypes.Request) atypes.Response { - return atypes.Response{ - Response: &admissionv1beta1.AdmissionResponse{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: false, }, } @@ -75,7 +73,7 @@ var _ = Describe("admission webhook", func() { Context("multiple handlers can be invoked", func() { BeforeEach(func(done Done) { wh = &Webhook{ - Type: types.WebhookTypeValidating, + Type: ValidatingWebhook, Handlers: []Handler{alwaysAllow, alwaysDeny}, } close(done) @@ -94,7 +92,7 @@ var _ = Describe("admission webhook", func() { Context("validating webhook should return if one of the handler denies", func() { BeforeEach(func(done Done) { wh = &Webhook{ - Type: types.WebhookTypeValidating, + Type: ValidatingWebhook, Handlers: []Handler{alwaysDeny, alwaysAllow}, } close(done) @@ -118,8 +116,8 @@ var _ = Describe("admission webhook", func() { Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)}, } patcher1 := &fakeHandler{ - fn: func(ctx context.Context, req atypes.Request) atypes.Response { - return atypes.Response{ + fn: func(ctx context.Context, req Request) Response { + return Response{ Patches: []jsonpatch.JsonPatchOperation{ { Operation: "add", @@ -132,7 +130,7 @@ var _ = Describe("admission webhook", func() { Value: "2", }, }, - Response: &admissionv1beta1.AdmissionResponse{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), }, @@ -140,8 +138,8 @@ var _ = Describe("admission webhook", func() { }, } patcher2 := &fakeHandler{ - fn: func(ctx context.Context, req atypes.Request) atypes.Response { - return atypes.Response{ + fn: func(ctx context.Context, req Request) Response { + return Response{ Patches: []jsonpatch.JsonPatchOperation{ { Operation: "add", @@ -149,7 +147,7 @@ var _ = Describe("admission webhook", func() { Value: "world", }, }, - Response: &admissionv1beta1.AdmissionResponse{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), }, @@ -157,7 +155,7 @@ var _ = Describe("admission webhook", func() { }, } wh := &Webhook{ - Type: types.WebhookTypeMutating, + Type: MutatingWebhook, Handlers: []Handler{patcher1, patcher2}, } expected := []byte( @@ -201,16 +199,16 @@ var _ = Describe("admission webhook", func() { Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)}, } errPatcher := &fakeHandler{ - fn: func(ctx context.Context, req atypes.Request) atypes.Response { - return atypes.Response{ - Response: &admissionv1beta1.AdmissionResponse{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: false, }, } }, } wh := &Webhook{ - Type: types.WebhookTypeMutating, + Type: MutatingWebhook, Handlers: []Handler{errPatcher}, } expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"code":200}}} @@ -226,7 +224,7 @@ var _ = Describe("admission webhook", func() { Describe("webhook validation", func() { Context("valid mutating webhook", func() { wh := &Webhook{ - Type: types.WebhookTypeMutating, + Type: MutatingWebhook, Path: "/mutate-deployments", Handlers: []Handler{&fakeHandler{}}, } @@ -239,7 +237,7 @@ var _ = Describe("admission webhook", func() { Context("valid validating webhook", func() { wh := &Webhook{ - Type: types.WebhookTypeValidating, + Type: ValidatingWebhook, Path: "/validate-deployments", Handlers: []Handler{&fakeHandler{}}, } @@ -257,13 +255,13 @@ var _ = Describe("admission webhook", func() { } It("should fail validation", func() { err := wh.Validate() - Expect(err.Error()).To(ContainSubstring("only WebhookTypeMutating and WebhookTypeValidating are supported")) + Expect(err).To(MatchError("unsupported Type: 0, only MutatingWebhook and ValidatingWebhook are supported")) }) }) Context("missing Handlers", func() { wh := &Webhook{ - Type: types.WebhookTypeValidating, + Type: ValidatingWebhook, Path: "/validate-deployments", Handlers: []Handler{}, } diff --git a/pkg/webhook/types/webhook.go b/pkg/webhook/types/webhook.go deleted file mode 100644 index 2ad1253f21..0000000000 --- a/pkg/webhook/types/webhook.go +++ /dev/null @@ -1,28 +0,0 @@ -/* -Copyright 2018 The Kubernetes 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 types - -// WebhookType defines the type of a webhook -type WebhookType int - -const ( - _ = iota - // WebhookTypeMutating represents mutating type webhook - WebhookTypeMutating WebhookType = iota - // WebhookTypeValidating represents validating type webhook - WebhookTypeValidating -) From 6dbde68e80b15db806a1fa31db1d95efd6696f42 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Thu, 7 Feb 2019 13:06:54 -0800 Subject: [PATCH 06/19] Remove extraneous decoder interface We don't have anywhere that we actually make use of decoder as an interface, so switch to just have a concrete implementation. --- pkg/webhook/admission/decode.go | 20 ++++++-------------- pkg/webhook/admission/decode_test.go | 2 +- pkg/webhook/admission/types.go | 7 ------- 3 files changed, 7 insertions(+), 22 deletions(-) diff --git a/pkg/webhook/admission/decode.go b/pkg/webhook/admission/decode.go index 5001247e1e..621cca5948 100644 --- a/pkg/webhook/admission/decode.go +++ b/pkg/webhook/admission/decode.go @@ -21,27 +21,19 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer" ) -// DecodeFunc is a function that implements an admission decoder in a single function. -type DecodeFunc func(Request, runtime.Object) error - -var _ Decoder = DecodeFunc(nil) - -// Decode implements the Decoder interface. -func (f DecodeFunc) Decode(req Request, obj runtime.Object) error { - return f(req, obj) -} - -type decoder struct { +// Decoder knows how to decode the contents of an admission +// request into a concrete object. +type Decoder struct { codecs serializer.CodecFactory } // NewDecoder creates a Decoder given the runtime.Scheme -func NewDecoder(scheme *runtime.Scheme) (Decoder, error) { - return decoder{codecs: serializer.NewCodecFactory(scheme)}, nil +func NewDecoder(scheme *runtime.Scheme) (*Decoder, error) { + return &Decoder{codecs: serializer.NewCodecFactory(scheme)}, nil } // Decode decodes the inlined object in the AdmissionRequest into the passed-in runtime.Object. -func (d decoder) Decode(req Request, into runtime.Object) error { +func (d *Decoder) Decode(req Request, into runtime.Object) error { deserializer := d.codecs.UniversalDeserializer() return runtime.DecodeInto(deserializer, req.AdmissionRequest.Object.Raw, into) } diff --git a/pkg/webhook/admission/decode_test.go b/pkg/webhook/admission/decode_test.go index c0a9bbfe0f..655d353831 100644 --- a/pkg/webhook/admission/decode_test.go +++ b/pkg/webhook/admission/decode_test.go @@ -27,7 +27,7 @@ import ( ) var _ = Describe("admission webhook decoder", func() { - var decoder Decoder + var decoder *Decoder BeforeEach(func(done Done) { var err error decoder, err = NewDecoder(scheme.Scheme) diff --git a/pkg/webhook/admission/types.go b/pkg/webhook/admission/types.go index ee24e263cd..6d7577af9a 100644 --- a/pkg/webhook/admission/types.go +++ b/pkg/webhook/admission/types.go @@ -20,7 +20,6 @@ import ( "github.com/appscode/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" - "k8s.io/apimachinery/pkg/runtime" ) // Request defines the input for an admission handler. @@ -46,12 +45,6 @@ type Response struct { admissionv1beta1.AdmissionResponse } -// Decoder is used to decode AdmissionRequest. -type Decoder interface { - // Decode decodes the raw byte object from the AdmissionRequest to the passed-in runtime.Object. - Decode(Request, runtime.Object) error -} - // WebhookType defines the type of an admission webhook // (validating or mutating). type WebhookType int From c7969dc2f7b3dad6c5115450caeafabf68d8af17 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Thu, 7 Feb 2019 17:18:42 -0800 Subject: [PATCH 07/19] Extract out multi-handler support, remove builder This extracts out the multi-handler support into separate interfaces. That simplifies the code, makes it easier to test directly, and allows us to drop the now-extraneous Type field. This simultaneously removes the builder (for now) since it doesn't actually make anything simpler. Along the way, we also refactor some common functionality out into a helper on reponse itself as opposed to being run by different bits of the webhook. --- example/main.go | 22 +- pkg/webhook/admission/builder/builder.go | 100 ------- pkg/webhook/admission/builder/doc.go | 58 ---- pkg/webhook/admission/http.go | 2 +- pkg/webhook/admission/http_test.go | 124 ++++----- pkg/webhook/admission/multi.go | 126 +++++++++ pkg/webhook/admission/multi_test.go | 132 +++++++++ pkg/webhook/admission/types.go | 29 -- pkg/webhook/admission/webhook.go | 180 +++++-------- pkg/webhook/admission/webhook_test.go | 326 ++++++++--------------- pkg/webhook/doc.go | 23 +- pkg/webhook/server.go | 17 +- 12 files changed, 505 insertions(+), 634 deletions(-) delete mode 100644 pkg/webhook/admission/builder/builder.go delete mode 100644 pkg/webhook/admission/builder/doc.go create mode 100644 pkg/webhook/admission/multi.go create mode 100644 pkg/webhook/admission/multi_test.go diff --git a/example/main.go b/example/main.go index 7cd95f815e..3f256eeb30 100644 --- a/example/main.go +++ b/example/main.go @@ -32,7 +32,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager/signals" "sigs.k8s.io/controller-runtime/pkg/source" "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission/builder" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) var log = logf.Log.WithName("example-controller") @@ -79,17 +79,15 @@ func main() { // Setup webhooks entryLog.Info("setting up webhooks") - mutatingWebhook := builder.NewWebhookBuilder(). - Path("/mutate-pods"). - Mutating(). - Handlers(&podAnnotator{}). - Build() - - validatingWebhook := builder.NewWebhookBuilder(). - Path("/validate-pods"). - Validating(). - Handlers(&podValidator{}). - Build() + mutatingWebhook := &admission.Webhook{ + Path: "/mutate-pods", + Handler: &podAnnotator{}, + } + + validatingWebhook := &admission.Webhook{ + Path: "/validate-pods", + Handler: &podValidator{}, + } entryLog.Info("setting up webhook server") as, err := webhook.NewServer(webhook.ServerOptions{ diff --git a/pkg/webhook/admission/builder/builder.go b/pkg/webhook/admission/builder/builder.go deleted file mode 100644 index 23e8c0d14e..0000000000 --- a/pkg/webhook/admission/builder/builder.go +++ /dev/null @@ -1,100 +0,0 @@ -/* -Copyright 2018 The Kubernetes 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 builder - -import ( - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" -) - -// WebhookBuilder builds a webhook based on the provided options. -type WebhookBuilder struct { - // name specifies the name of the webhook. It must be unique among all webhooks. - name string - - // path is the URL Path to register this webhook. e.g. "/mutate-pods". - path string - - // handlers handle admission requests. - // A WebhookBuilder may have multiple handlers. - // For example, handlers[0] mutates a pod for feature foo. - // handlers[1] mutates a pod for a different feature bar. - handlers []admission.Handler - - // t specifies the type of the webhook. - // Currently, Mutating and Validating are supported. - t *admission.WebhookType -} - -// NewWebhookBuilder creates an empty WebhookBuilder. -func NewWebhookBuilder() *WebhookBuilder { - return &WebhookBuilder{} -} - -// Name sets the name of the webhook. -// This is optional -func (b *WebhookBuilder) Name(name string) *WebhookBuilder { - b.name = name - return b -} - -// Mutating sets the type to mutating admission webhook -// Only one of Mutating and Validating can be invoked. -func (b *WebhookBuilder) Mutating() *WebhookBuilder { - m := admission.MutatingWebhook - b.t = &m - return b -} - -// Validating sets the type to validating admission webhook -// Only one of Mutating and Validating can be invoked. -func (b *WebhookBuilder) Validating() *WebhookBuilder { - m := admission.ValidatingWebhook - b.t = &m - return b -} - -// Path sets the path for the webhook. -// Path needs to be unique among different webhooks. -// This is required. If not set, it will be built from the type and resource name. -// For example, a webhook that mutates pods has a default path of "/mutate-pods" -// If the defaulting logic can't find a unique path for it, user need to set it manually. -func (b *WebhookBuilder) Path(path string) *WebhookBuilder { - b.path = path - return b -} - -// Handlers sets the handlers of the webhook. -func (b *WebhookBuilder) Handlers(handlers ...admission.Handler) *WebhookBuilder { - b.handlers = handlers - return b -} - -// Build creates the Webhook based on the options provided. -func (b *WebhookBuilder) Build() *admission.Webhook { - if b.t == nil { - b.Mutating() - } - - w := &admission.Webhook{ - Name: b.name, - Type: *b.t, - Path: b.path, - Handlers: b.handlers, - } - - return w -} diff --git a/pkg/webhook/admission/builder/doc.go b/pkg/webhook/admission/builder/doc.go deleted file mode 100644 index 9ef8d65fa0..0000000000 --- a/pkg/webhook/admission/builder/doc.go +++ /dev/null @@ -1,58 +0,0 @@ -/* -Copyright 2018 The Kubernetes 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 builder provides methods to build admission webhooks. - -The following are 2 examples for building mutating webhook and validating webhook. - - webhook1 := NewWebhookBuilder(). - Mutating(). - Path("/mutatepods"). - ForType(&corev1.Pod{}). - Handlers(mutatingHandler11, mutatingHandler12). - Build() - - webhook2 := NewWebhookBuilder(). - Validating(). - Path("/validatepods"). - ForType(&appsv1.Deployment{}). - Handlers(validatingHandler21). - Build() - -Note: To build a webhook for a CRD, you need to ensure the manager uses the scheme that understands your CRD. -This is necessary, because if the scheme doesn't understand your CRD types, the decoder won't be able to decode -the CR object from the admission review request. - -The following snippet shows how to register CRD types with manager's scheme. - - mgr, err := manager.New(cfg, manager.Options{}) - if err != nil { - // handle error - } - // SchemeGroupVersion is group version used to register these objects - SchemeGroupVersion = schema.GroupVersion{Group: "crew.k8s.io", Version: "v1"} - // SchemeBuilder is used to add go types to the GroupVersionKind scheme - SchemeBuilder = &scheme.Builder{GroupVersion: SchemeGroupVersion} - // Register your CRD types. - SchemeBuilder.Register(&Kraken{}, &KrakenList{}) - // Register your CRD types with the manager's scheme. - err = SchemeBuilder.AddToScheme(mgr.GetScheme()) - if err != nil { - // handle error - } -*/ -package builder diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 1e24a2e2ca..29028ccf2b 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -73,7 +73,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { // verify the content type is accurate contentType := r.Header.Get("Content-Type") if contentType != "application/json" { - err = fmt.Errorf("contentType=%s, expect application/json", contentType) + err = fmt.Errorf("contentType=%s, expected application/json", contentType) log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) reviewResponse = ErrorResponse(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index afe59529b7..1e220fff08 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -29,98 +29,66 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" ) -var _ = Describe("admission webhook http handler", func() { - var w *httptest.ResponseRecorder - BeforeEach(func(done Done) { - w = &httptest.ResponseRecorder{ - Body: bytes.NewBuffer(nil), +var _ = Describe("Admission Webhooks", func() { + + Describe("HTTP Handler", func() { + var respRecorder *httptest.ResponseRecorder + BeforeEach(func() { + respRecorder = &httptest.ResponseRecorder{ + Body: bytes.NewBuffer(nil), + } + }) + webhook := &Webhook{ + Handler: nil, } - close(done) - }) - Describe("empty request body", func() { - req := &http.Request{Body: nil} - wh := &Webhook{ - Handlers: []Handler{}, - } + It("should return bad-request when given an empty body", func() { + req := &http.Request{Body: nil} - expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request body is empty","code":400}}} + expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request body is empty","code":400}}} `) - It("should return an error with bad-request status code", func() { - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.Bytes()).To(Equal(expected)) }) - }) - Describe("wrong content type", func() { - req := &http.Request{ - Header: http.Header{"Content-Type": []string{"application/foo"}}, - Body: nopCloser{Reader: bytes.NewBuffer(nil)}, - } - wh := &Webhook{ - Handlers: []Handler{}, - } - expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"contentType=application/foo, expect application/json","code":400}}} -`) - It("should return an error with bad-request status code", func() { - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) + It("should return bad-request when given the wrong content-type", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/foo"}}, + Body: nopCloser{Reader: bytes.NewBuffer(nil)}, + } - }) - }) - - Describe("can't decode body", func() { - req := &http.Request{ - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: nopCloser{Reader: bytes.NewBufferString("{")}, - } - wh := &Webhook{ - Type: MutatingWebhook, - Handlers: []Handler{}, - } - expected := []byte( - `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"couldn't get version/kind; json parse error: unexpected end of JSON input","code":400}}} + expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"contentType=application/foo, expected application/json","code":400}}} `) - It("should return an error with bad-request status code", func() { - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) - + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.Bytes()).To(Equal(expected)) }) - }) - Describe("no webhook type", func() { - req := &http.Request{ - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)}, - } - wh := &Webhook{ - Handlers: []Handler{}, - } - expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"you must specify your webhook type","code":500}}} -`) - It("should return an error with internal-error status code", func() { - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) + It("should return bad-request when given an undecodable body", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: nopCloser{Reader: bytes.NewBufferString("{")}, + } + expected := []byte( + `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"couldn't get version/kind; json parse error: unexpected end of JSON input","code":400}}} +`) + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.Bytes()).To(Equal(expected)) }) - }) - Describe("handler can be invoked", func() { - req := &http.Request{ - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)}, - } - h := &fakeHandler{} - wh := &Webhook{ - Type: ValidatingWebhook, - Handlers: []Handler{h}, - } - expected := []byte(`{"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}} + It("should return the response given by the handler", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)}, + } + webhook := &Webhook{ + Handler: &fakeHandler{}, + } + + expected := []byte(`{"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}} `) - It("should return a response successfully", func() { - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) - Expect(h.invoked).To(BeTrue()) + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.Bytes()).To(Equal(expected)) }) }) }) diff --git a/pkg/webhook/admission/multi.go b/pkg/webhook/admission/multi.go new file mode 100644 index 0000000000..43d98dd4c9 --- /dev/null +++ b/pkg/webhook/admission/multi.go @@ -0,0 +1,126 @@ +/* +Copyright 2018 The Kubernetes 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 admission + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + + "github.com/appscode/jsonpatch" + admissionv1beta1 "k8s.io/api/admission/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" +) + +type multiMutating []Handler + +func (hs multiMutating) Handle(ctx context.Context, req Request) Response { + patches := []jsonpatch.JsonPatchOperation{} + for _, handler := range hs { + resp := handler.Handle(ctx, req) + if !resp.Allowed { + return resp + } + if resp.PatchType != nil && *resp.PatchType != admissionv1beta1.PatchTypeJSONPatch { + return ErrorResponse(http.StatusInternalServerError, + fmt.Errorf("unexpected patch type returned by the handler: %v, only allow: %v", + resp.PatchType, admissionv1beta1.PatchTypeJSONPatch)) + } + patches = append(patches, resp.Patches...) + } + var err error + marshaledPatch, err := json.Marshal(patches) + if err != nil { + return ErrorResponse(http.StatusBadRequest, fmt.Errorf("error when marshaling the patch: %v", err)) + } + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + Result: &metav1.Status{ + Code: http.StatusOK, + }, + Patch: marshaledPatch, + PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), + }, + } +} + +// InjectFunc injects the field setter into the handlers. +func (hs multiMutating) InjectFunc(f inject.Func) error { + // inject directly into the handlers. It would be more correct + // to do this in a sync.Once in Handle (since we don't have some + // other start/finalize-type method), but it's more efficient to + // do it here, presumably. + for _, handler := range hs { + if err := f(handler); err != nil { + return err + } + } + + return nil +} + +// MultiMutatingHandler combines multiple mutating webhook handlers into a single +// mutating webhook handler. Handlers are called in sequential order, and the first +// `allowed: false` response may short-circuit the rest. Users must take care to +// ensure patches are disjoint. +func MultiMutatingHandler(handlers ...Handler) Handler { + return multiMutating(handlers) +} + +type multiValidating []Handler + +func (hs multiValidating) Handle(ctx context.Context, req Request) Response { + for _, handler := range hs { + resp := handler.Handle(ctx, req) + if !resp.Allowed { + return resp + } + } + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + Result: &metav1.Status{ + Code: http.StatusOK, + }, + }, + } +} + +// MultiValidatingHandler combines multiple validating webhook handlers into a single +// validating webhook handler. Handlers are called in sequential order, and the first +// `allowed: false` response may short-circuit the rest. +func MultiValidatingHandler(handlers ...Handler) Handler { + return multiValidating(handlers) +} + +// InjectFunc injects the field setter into the handlers. +func (hs multiValidating) InjectFunc(f inject.Func) error { + // inject directly into the handlers. It would be more correct + // to do this in a sync.Once in Handle (since we don't have some + // other start/finalize-type method), but it's more efficient to + // do it here, presumably. + for _, handler := range hs { + if err := f(handler); err != nil { + return err + } + } + + return nil +} diff --git a/pkg/webhook/admission/multi_test.go b/pkg/webhook/admission/multi_test.go new file mode 100644 index 0000000000..0e744a5cbb --- /dev/null +++ b/pkg/webhook/admission/multi_test.go @@ -0,0 +1,132 @@ +/* +Copyright 2019 The Kubernetes 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 admission + +import ( + "context" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "github.com/appscode/jsonpatch" + admissionv1beta1 "k8s.io/api/admission/v1beta1" +) + +var _ = Describe("Admission Webhooks", func() { + Describe("Multi-handler Webhooks", func() { + alwaysAllow := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + }, + } + }, + } + alwaysDeny := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: false, + }, + } + }, + } + + Context("with validating handlers", func() { + It("should deny the request if any handler denies the request", func() { + By("setting up a handler with accept and deny") + handler := MultiValidatingHandler(alwaysAllow, alwaysDeny) + + By("checking that the handler denies the request") + resp := handler.Handle(context.Background(), Request{}) + Expect(resp.Allowed).To(BeFalse()) + }) + + It("should allow the request if all handlers allow the request", func() { + By("setting up a handler with only accept") + handler := MultiValidatingHandler(alwaysAllow, alwaysAllow) + + By("checking that the handler allows the request") + resp := handler.Handle(context.Background(), Request{}) + Expect(resp.Allowed).To(BeTrue()) + }) + }) + + Context("with mutating handlers", func() { + patcher1 := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + Patches: []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/annotation/new-key", + Value: "new-value", + }, + { + Operation: "replace", + Path: "/spec/replicas", + Value: "2", + }, + }, + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), + }, + } + }, + } + patcher2 := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + Patches: []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/annotation/hello", + Value: "world", + }, + }, + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), + }, + } + }, + } + + It("should not return any patches if the request is denied", func() { + By("setting up a webhook with some patches and a deny") + handler := MultiMutatingHandler(patcher1, patcher2, alwaysDeny) + + By("checking that the handler denies the request and produces no patches") + resp := handler.Handle(context.Background(), Request{}) + Expect(resp.Allowed).To(BeFalse()) + Expect(resp.Patches).To(BeEmpty()) + }) + + It("should produce all patches if the requests are all allowed", func() { + By("setting up a webhook with some patches") + handler := MultiMutatingHandler(patcher1, patcher2, alwaysAllow) + + By("checking that the handler accepts the request and returns all patches") + resp := handler.Handle(context.Background(), Request{}) + Expect(resp.Allowed).To(BeTrue()) + Expect(resp.Patch).To(Equal([]byte(`[{"op":"add","path":"/metadata/annotation/new-key","value":"new-value"},{"op":"replace","path":"/spec/replicas","value":"2"},{"op":"add","path":"/metadata/annotation/hello","value":"world"}]`))) + }) + }) + }) +}) diff --git a/pkg/webhook/admission/types.go b/pkg/webhook/admission/types.go index 6d7577af9a..7a57f498fd 100644 --- a/pkg/webhook/admission/types.go +++ b/pkg/webhook/admission/types.go @@ -16,35 +16,6 @@ limitations under the License. package admission -import ( - "github.com/appscode/jsonpatch" - - admissionv1beta1 "k8s.io/api/admission/v1beta1" -) - -// Request defines the input for an admission handler. -// It contains information to identify the object in -// question (group, version, kind, resource, subresource, -// name, namespace), as well as the operation in question -// (e.g. Get, Create, etc), and the object itself. -type Request struct { - admissionv1beta1.AdmissionRequest -} - -// Response is the output of an admission handler. -// It contains a response indicating if a given -// operation is allowed, as well as a set of patches -// to mutate the object in the case of a mutating admission handler. -type Response struct { - // Patches are the JSON patches for mutating webhooks. - // Using this instead of setting Response.Patch to minimize - // overhead of serialization and deserialization. - Patches []jsonpatch.JsonPatchOperation - // AdmissionResponse is the raw admission response. - // The Patch field in it will be overwritten by the listed patches. - admissionv1beta1.AdmissionResponse -} - // WebhookType defines the type of an admission webhook // (validating or mutating). type WebhookType int diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 28036b5fe2..e322e33dea 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -18,21 +18,79 @@ package admission import ( "context" - "encoding/json" "errors" - "fmt" "net/http" "regexp" "strings" "sync" "github.com/appscode/jsonpatch" - admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/json" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) +var ( + errUnableToEncodeResponse = errors.New("unable to encode response") +) + +// Request defines the input for an admission handler. +// It contains information to identify the object in +// question (group, version, kind, resource, subresource, +// name, namespace), as well as the operation in question +// (e.g. Get, Create, etc), and the object itself. +type Request struct { + admissionv1beta1.AdmissionRequest +} + +// Response is the output of an admission handler. +// It contains a response indicating if a given +// operation is allowed, as well as a set of patches +// to mutate the object in the case of a mutating admission handler. +type Response struct { + // Patches are the JSON patches for mutating webhooks. + // Using this instead of setting Response.Patch to minimize + // overhead of serialization and deserialization. + // Patches set here will override any patches in the response, + // so leave this empty if you want to set the patch response directly. + Patches []jsonpatch.JsonPatchOperation + // AdmissionResponse is the raw admission response. + // The Patch field in it will be overwritten by the listed patches. + admissionv1beta1.AdmissionResponse +} + +// Complete populates any fields that are yet to be set in +// the underlying AdmissionResponse, It mutates the response. +func (r *Response) Complete(req Request) error { + r.UID = req.UID + + // ensure that we have a valid status code + if r.Result == nil { + r.Result = &metav1.Status{} + } + if r.Result.Code == 0 { + r.Result.Code = http.StatusOK + } + // TODO(directxman12): do we need to populate this further, and/or + // is code actually necessary (the same webhook doesn't use it) + + if len(r.Patches) == 0 { + return nil + } + + var err error + r.Patch, err = json.Marshal(r.Patches) + if err != nil { + return err + } + patchType := admissionv1beta1.PatchTypeJSONPatch + r.PatchType = &patchType + + return nil +} + // Handler can handle an AdmissionRequest. type Handler interface { Handle(context.Context, Request) Response @@ -52,16 +110,11 @@ func (f HandlerFunc) Handle(ctx context.Context, req Request) Response { type Webhook struct { // Name is the name of the webhook Name string - // Type is the webhook type, i.e. mutating, validating - Type WebhookType // Path is the path this webhook will serve. Path string - // Handlers contains a list of handlers. Each handler may only contains the business logic for its own feature. - // For example, feature foo and bar can be in the same webhook if all the other configurations are the same. - // The handler will be invoked sequentially as the order in the list. - // Note: if you are using mutating webhook with multiple handlers, it's your responsibility to - // ensure the handlers are not generating conflicting JSON patches. - Handlers []Handler + // Handler actually processes an admission request returning whether it was allowed or denied, + // and potentially patches to apply to the handler. + Handler Handler once sync.Once } @@ -74,11 +127,6 @@ func (w *Webhook) setDefaults() { } } -// Add adds additional handler(s) in the webhook -func (w *Webhook) Add(handlers ...Handler) { - w.Handlers = append(w.Handlers, handlers...) -} - // Webhook implements Handler interface. var _ Handler = &Webhook{} @@ -87,79 +135,13 @@ var _ Handler = &Webhook{} // If the webhook is validating type, it delegates the AdmissionRequest to each handler and // deny the request if anyone denies. func (w *Webhook) Handle(ctx context.Context, req Request) Response { - var resp Response - switch w.Type { - case MutatingWebhook: - resp = w.handleMutating(ctx, req) - case ValidatingWebhook: - resp = w.handleValidating(ctx, req) - default: - return ErrorResponse(http.StatusInternalServerError, errors.New("you must specify your webhook type")) + resp := w.Handler.Handle(ctx, req) + if err := resp.Complete(req); err != nil { + log.Error(err, "unable to encode response") + return ErrorResponse(http.StatusInternalServerError, errUnableToEncodeResponse) } - resp.UID = req.AdmissionRequest.UID - return resp -} -func (w *Webhook) handleMutating(ctx context.Context, req Request) Response { - patches := []jsonpatch.JsonPatchOperation{} - for _, handler := range w.Handlers { - resp := handler.Handle(ctx, req) - if !resp.Allowed { - setStatusOKInAdmissionResponse(&resp.AdmissionResponse) - return resp - } - if resp.PatchType != nil && *resp.PatchType != admissionv1beta1.PatchTypeJSONPatch { - return ErrorResponse(http.StatusInternalServerError, - fmt.Errorf("unexpected patch type returned by the handler: %v, only allow: %v", - resp.PatchType, admissionv1beta1.PatchTypeJSONPatch)) - } - patches = append(patches, resp.Patches...) - } - var err error - marshaledPatch, err := json.Marshal(patches) - if err != nil { - return ErrorResponse(http.StatusBadRequest, fmt.Errorf("error when marshaling the patch: %v", err)) - } - return Response{ - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{ - Code: http.StatusOK, - }, - Patch: marshaledPatch, - PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), - }, - } -} - -func (w *Webhook) handleValidating(ctx context.Context, req Request) Response { - for _, handler := range w.Handlers { - resp := handler.Handle(ctx, req) - if !resp.Allowed { - setStatusOKInAdmissionResponse(&resp.AdmissionResponse) - return resp - } - } - return Response{ - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{ - Code: http.StatusOK, - }, - }, - } -} - -func setStatusOKInAdmissionResponse(resp *admissionv1beta1.AdmissionResponse) { - if resp == nil { - return - } - if resp.Result == nil { - resp.Result = &metav1.Status{} - } - if resp.Result.Code == 0 { - resp.Result.Code = http.StatusOK - } + return resp } // GetName returns the name of the webhook. @@ -174,31 +156,16 @@ func (w *Webhook) GetPath() string { return w.Path } -// GetType returns the type of the webhook. -func (w *Webhook) GetType() WebhookType { - w.once.Do(w.setDefaults) - return w.Type -} - -// Handler returns a http.Handler for the webhook -func (w *Webhook) Handler() http.Handler { - w.once.Do(w.setDefaults) - return w -} - // Validate validates if the webhook is valid. func (w *Webhook) Validate() error { w.once.Do(w.setDefaults) if len(w.Name) == 0 { return errors.New("field Name should not be empty") } - if w.Type != MutatingWebhook && w.Type != ValidatingWebhook { - return fmt.Errorf("unsupported Type: %v, only MutatingWebhook and ValidatingWebhook are supported", w.Type) - } if len(w.Path) == 0 { return errors.New("field Path should not be empty") } - if len(w.Handlers) == 0 { + if w.Handler == nil { return errors.New("field Handler should not be empty") } return nil @@ -210,10 +177,5 @@ func (w *Webhook) InjectFunc(f inject.Func) error { // to do this in a sync.Once in Handle (since we don't have some // other start/finalize-type method), but it's more efficient to // do it here, presumably. - for _, handler := range w.Handlers { - if err := f(handler); err != nil { - return err - } - } - return nil + return f(w.Handler) } diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index 9866b03462..d490a98084 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -17,259 +17,143 @@ limitations under the License. package admission import ( - "bytes" "context" - "encoding/base64" - "encoding/json" - "errors" "net/http" - "net/http/httptest" - "github.com/appscode/jsonpatch" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/appscode/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + machinerytypes "k8s.io/apimachinery/pkg/types" ) -var _ = Describe("admission webhook", func() { - var w *httptest.ResponseRecorder - BeforeEach(func(done Done) { - w = &httptest.ResponseRecorder{ - Body: bytes.NewBuffer(nil), +var _ = Describe("Admission Webhooks", func() { + allowHandler := func() *Webhook { + handler := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + }, + } + }, } - close(done) + webhook := &Webhook{ + Handler: handler, + } + + return webhook + } + + It("should invoke the handler to get a response", func() { + By("setting up a webhook with an allow handler") + webhook := allowHandler() + + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{}) + + By("checking that it allowed the request") + Expect(resp.Allowed).To(BeTrue()) }) - Describe("validating webhook", func() { - var alwaysAllow, alwaysDeny *fakeHandler - var req *http.Request - var wh *Webhook - BeforeEach(func(done Done) { - alwaysAllow = &fakeHandler{ - fn: func(ctx context.Context, req Request) Response { - return Response{ - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - }, - } - }, - } - alwaysDeny = &fakeHandler{ - fn: func(ctx context.Context, req Request) Response { - return Response{ - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: false, - }, - } - }, - } - req = &http.Request{ - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)}, - } - close(done) - }) - Context("multiple handlers can be invoked", func() { - BeforeEach(func(done Done) { - wh = &Webhook{ - Type: ValidatingWebhook, - Handlers: []Handler{alwaysAllow, alwaysDeny}, - } - close(done) - }) + It("should ensure that the response's UID is set to the request's UID", func() { + By("setting up a webhook") + webhook := allowHandler() - It("should deny the request", func() { - expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"code":200}}} -`) - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) - Expect(alwaysAllow.invoked).To(BeTrue()) - Expect(alwaysDeny.invoked).To(BeTrue()) - }) - }) + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{AdmissionRequest: admissionv1beta1.AdmissionRequest{UID: "foobar"}}) + + By("checking that the response share's the request's UID") + Expect(resp.UID).To(Equal(machinerytypes.UID("foobar"))) + }) + + It("should populate the status on a response if one is not provided", func() { + By("setting up a webhook") + webhook := allowHandler() + + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{}) - Context("validating webhook should return if one of the handler denies", func() { - BeforeEach(func(done Done) { - wh = &Webhook{ - Type: ValidatingWebhook, - Handlers: []Handler{alwaysDeny, alwaysAllow}, + By("checking that the response share's the request's UID") + Expect(resp.Result).To(Equal(&metav1.Status{Code: http.StatusOK})) + }) + + It("shouldn't overwrite the status on a response", func() { + By("setting up a webhook that sets a status") + webhook := &Webhook{ + Handler: HandlerFunc(func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + Result: &metav1.Status{Message: "Ground Control to Major Tom"}, + }, } - close(done) - }) + }), + } - It("should deny the request", func() { - expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"code":200}}} -`) - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) - Expect(alwaysDeny.invoked).To(BeTrue()) - Expect(alwaysAllow.invoked).To(BeFalse()) - }) - }) + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{}) + + By("checking that the message is intact") + Expect(resp.Result).NotTo(BeNil()) + Expect(resp.Result.Message).To(Equal("Ground Control to Major Tom")) }) - Describe("mutating webhook", func() { - Context("multiple patch handlers", func() { - req := &http.Request{ - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)}, - } - patcher1 := &fakeHandler{ - fn: func(ctx context.Context, req Request) Response { - return Response{ - Patches: []jsonpatch.JsonPatchOperation{ - { - Operation: "add", - Path: "/metadata/annotation/new-key", - Value: "new-value", - }, - { - Operation: "replace", - Path: "/spec/replicas", - Value: "2", - }, - }, - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), - }, - } - }, - } - patcher2 := &fakeHandler{ - fn: func(ctx context.Context, req Request) Response { - return Response{ - Patches: []jsonpatch.JsonPatchOperation{ - { - Operation: "add", - Path: "/metadata/annotation/hello", - Value: "world", - }, - }, - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), - }, - } - }, - } - wh := &Webhook{ - Type: MutatingWebhook, - Handlers: []Handler{patcher1, patcher2}, - } - expected := []byte( - `{"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200},` + - `"patch":"W3sib3AiOiJhZGQiLCJwYXRoIjoiL21ldGFkYXRhL2Fubm90YXRpb2` + - `4vbmV3LWtleSIsInZhbHVlIjoibmV3LXZhbHVlIn0seyJvcCI6InJlcGxhY2UiLCJwYXRoIjoiL3NwZWMvcmVwbGljYXMiLC` + - `J2YWx1ZSI6IjIifSx7Im9wIjoiYWRkIiwicGF0aCI6Ii9tZXRhZGF0YS9hbm5vdGF0aW9uL2hlbGxvIiwidmFsdWUiOiJ3b3JsZCJ9XQ==",` + - `"patchType":"JSONPatch"}} -`) - patches := []jsonpatch.JsonPatchOperation{ - { - Operation: "add", - Path: "/metadata/annotation/new-key", - Value: "new-value", - }, - { - Operation: "replace", - Path: "/spec/replicas", - Value: "2", - }, - { - Operation: "add", - Path: "/metadata/annotation/hello", - Value: "world", - }, - } - j, _ := json.Marshal(patches) - base64encoded := base64.StdEncoding.EncodeToString(j) - It("should aggregates patches from multiple handlers", func() { - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) - Expect(w.Body.String()).To(ContainSubstring(base64encoded)) - Expect(patcher1.invoked).To(BeTrue()) - Expect(patcher2.invoked).To(BeTrue()) - }) - }) + It("should serialize patch operations into a single jsonpatch blob", func() { + By("setting up a webhook with a patching handler") + webhook := &Webhook{ + Handler: HandlerFunc(func(ctx context.Context, req Request) Response { + return Response{ + Patches: []jsonpatch.Operation{jsonpatch.Operation{Operation: "add", Path: "/a", Value: 2}, jsonpatch.Operation{Operation: "replace", Path: "/b", Value: 4}}, + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), + }, + } + }), + } - Context("patch handler denies the request", func() { - req := &http.Request{ - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: nopCloser{Reader: bytes.NewBufferString(`{"request":{}}`)}, - } - errPatcher := &fakeHandler{ - fn: func(ctx context.Context, req Request) Response { - return Response{ - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: false, - }, - } - }, - } - wh := &Webhook{ - Type: MutatingWebhook, - Handlers: []Handler{errPatcher}, - } - expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"code":200}}} -`) - It("should deny the request", func() { - wh.ServeHTTP(w, req) - Expect(w.Body.Bytes()).To(Equal(expected)) - Expect(errPatcher.invoked).To(BeTrue()) - }) - }) + By("invoking the webhoook") + resp := webhook.Handle(context.Background(), Request{}) + + By("checking that a JSON patch is populated on the response") + patchType := admissionv1beta1.PatchTypeJSONPatch + Expect(resp.PatchType).To(Equal(&patchType)) + Expect(resp.Patch).To(Equal([]byte(`[{"op":"add","path":"/a","value":2},{"op":"replace","path":"/b","value":4}]`))) }) - Describe("webhook validation", func() { - Context("valid mutating webhook", func() { - wh := &Webhook{ - Type: MutatingWebhook, - Path: "/mutate-deployments", - Handlers: []Handler{&fakeHandler{}}, - } - It("should pass validation", func() { - err := wh.Validate() - Expect(err).NotTo(HaveOccurred()) - Expect(wh.Name).To(Equal("mutatedeployments.example.com")) - }) - }) + It("should default a name if none is given", func() { + wh := &Webhook{ + Path: "/somepath", + Handler: &fakeHandler{}, + } - Context("valid validating webhook", func() { + Expect(wh.GetName()).NotTo(BeEmpty()) + }) + + Context("validation", func() { + It("should accept a properly populated webhook", func() { wh := &Webhook{ - Type: ValidatingWebhook, - Path: "/validate-deployments", - Handlers: []Handler{&fakeHandler{}}, + Path: "/somepath", + Handler: &fakeHandler{}, } - It("should pass validation", func() { - err := wh.Validate() - Expect(err).NotTo(HaveOccurred()) - Expect(wh.Name).To(Equal("validatedeployments.example.com")) - }) + Expect(wh.Validate()).To(Succeed()) }) - Context("missing webhook type", func() { + It("should fail when missing a path", func() { wh := &Webhook{ - Path: "/mutate-deployments", - Handlers: []Handler{&fakeHandler{}}, + Handler: &fakeHandler{}, } - It("should fail validation", func() { - err := wh.Validate() - Expect(err).To(MatchError("unsupported Type: 0, only MutatingWebhook and ValidatingWebhook are supported")) - }) + Expect(wh.Validate()).NotTo(Succeed()) }) - Context("missing Handlers", func() { + It("should fail when missing a handler", func() { wh := &Webhook{ - Type: ValidatingWebhook, - Path: "/validate-deployments", - Handlers: []Handler{}, + Path: "/somepath", } - It("should fail validation", func() { - err := wh.Validate() - Expect(err).To(Equal(errors.New("field Handler should not be empty"))) - }) + Expect(wh.Validate()).NotTo(Succeed()) }) - }) }) diff --git a/pkg/webhook/doc.go b/pkg/webhook/doc.go index 6aafff524c..9f40c72a70 100644 --- a/pkg/webhook/doc.go +++ b/pkg/webhook/doc.go @@ -21,25 +21,14 @@ Currently, it only supports admission webhooks. It will support CRD conversion w Build webhooks - // mgr is the manager that runs the server. - webhook1, err := NewWebhookBuilder(). - Name("foo.k8s.io"). - Mutating(). - Path("/mutating-pods"). - Handlers(mutatingHandler1, mutatingHandler2). - Build() - if err != nil { - // handle error + webhook1 := &Webhook{ + Path: "/mutating-pods", + Handler: mutatingHandler, } - webhook2, err := NewWebhookBuilder(). - Name("bar.k8s.io"). - Validating(). - Path("/validating-deployment"). - Handlers(validatingHandler1). - Build() - if err != nil { - // handle error + webhook2 := &Webhook{ + Path: "/validating-pods", + Handler: validatingHandler, } Create a webhook server. diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index 4314bc5a08..381152369a 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -19,6 +19,7 @@ package webhook import ( "context" "crypto/tls" + "fmt" "net" "net/http" "path" @@ -69,12 +70,11 @@ type Server struct { // Webhook defines the basics that a webhook should support. type Webhook interface { + // Webhooks handle HTTP requests. http.Handler // GetPath returns the path that the webhook registered. GetPath() string - // Handler returns a http.Handler for the webhook. - Handler() http.Handler // Validate validates if the webhook itself is valid. // If invalid, a non-nil error will be returned. Validate() error @@ -115,14 +115,13 @@ func (s *Server) Register(webhooks ...Webhook) error { if err != nil { return err } - // Handle actually ensures that no duplicate paths are registered. - s.sMux.Handle(webhook.GetPath(), webhook.Handler()) - s.registry[webhook.GetPath()] = webhooks[i] - - // Inject dependencies to each webhook. - if err := s.setFields(webhooks[i]); err != nil { - return err + // TODO(directxman12): call setfields if we've already started the server + _, found := s.registry[webhook.GetPath()] + if found { + return fmt.Errorf("can't register duplicate path: %v", webhook.GetPath()) } + s.registry[webhook.GetPath()] = webhooks[i] + s.sMux.Handle(webhook.GetPath(), webhook) } return nil From 66be3368b0e400e7668a58bd183722dd4d79756f Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Thu, 7 Feb 2019 17:42:00 -0800 Subject: [PATCH 08/19] Clean up tests to follow standards This cleans up tests to more closely follow CR style, favoring behavior-style tests as opposed to pure unit tests. --- pkg/webhook/admission/decode_test.go | 62 ++++++---- pkg/webhook/admission/multi_test.go | 164 ++++++++++++------------- pkg/webhook/admission/response_test.go | 2 +- 3 files changed, 118 insertions(+), 110 deletions(-) diff --git a/pkg/webhook/admission/decode_test.go b/pkg/webhook/admission/decode_test.go index 655d353831..e41f3aa852 100644 --- a/pkg/webhook/admission/decode_test.go +++ b/pkg/webhook/admission/decode_test.go @@ -22,33 +22,25 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/scheme" ) -var _ = Describe("admission webhook decoder", func() { +var _ = Describe("Admission Webhook Decoder", func() { var decoder *Decoder - BeforeEach(func(done Done) { + BeforeEach(func() { + By("creating a new decoder for a scheme") var err error decoder, err = NewDecoder(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) Expect(decoder).NotTo(BeNil()) - close(done) }) - Describe("NewDecoder", func() { - It("should return a decoder without an error", func() { - decoder, err := NewDecoder(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - Expect(decoder).NotTo(BeNil()) - }) - }) - - Describe("Decode", func() { - req := Request{ - AdmissionRequest: admissionv1beta1.AdmissionRequest{ - Object: runtime.RawExtension{ - Raw: []byte(`{ + req := Request{ + AdmissionRequest: admissionv1beta1.AdmissionRequest{ + Object: runtime.RawExtension{ + Raw: []byte(`{ "apiVersion": "v1", "kind": "Pod", "metadata": { @@ -64,19 +56,35 @@ var _ = Describe("admission webhook decoder", func() { ] } }`), - }, }, - } + }, + } - It("should be able to decode", func() { - err := decoder.Decode(req, &corev1.Pod{}) - Expect(err).NotTo(HaveOccurred()) - }) + It("should decode a valid admission request", func() { + By("extracting the object from the request") + var actualObj corev1.Pod + Expect(decoder.Decode(req, &actualObj)).To(Succeed()) + + By("verifying that all data is present in the object") + Expect(actualObj).To(Equal(corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Image: "bar", Name: "bar"}, + }, + }, + })) + }) - It("should return an error if the GVK mismatch", func() { - err := decoder.Decode(req, &corev1.Node{}) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("unable to decode")) - }) + It("should fail to decode if the object in the request doesn't match the passed-in type", func() { + By("trying to extract a pod into a node") + Expect(decoder.Decode(req, &corev1.Node{})).NotTo(Succeed()) }) }) diff --git a/pkg/webhook/admission/multi_test.go b/pkg/webhook/admission/multi_test.go index 0e744a5cbb..8501e62f62 100644 --- a/pkg/webhook/admission/multi_test.go +++ b/pkg/webhook/admission/multi_test.go @@ -26,107 +26,107 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" ) -var _ = Describe("Admission Webhooks", func() { - Describe("Multi-handler Webhooks", func() { - alwaysAllow := &fakeHandler{ +var _ = Describe("Multi-Handler Admission Webhooks", func() { + alwaysAllow := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + }, + } + }, + } + alwaysDeny := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: false, + }, + } + }, + } + + Context("with validating handlers", func() { + It("should deny the request if any handler denies the request", func() { + By("setting up a handler with accept and deny") + handler := MultiValidatingHandler(alwaysAllow, alwaysDeny) + + By("checking that the handler denies the request") + resp := handler.Handle(context.Background(), Request{}) + Expect(resp.Allowed).To(BeFalse()) + }) + + It("should allow the request if all handlers allow the request", func() { + By("setting up a handler with only accept") + handler := MultiValidatingHandler(alwaysAllow, alwaysAllow) + + By("checking that the handler allows the request") + resp := handler.Handle(context.Background(), Request{}) + Expect(resp.Allowed).To(BeTrue()) + }) + }) + + Context("with mutating handlers", func() { + patcher1 := &fakeHandler{ fn: func(ctx context.Context, req Request) Response { return Response{ + Patches: []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/annotation/new-key", + Value: "new-value", + }, + { + Operation: "replace", + Path: "/spec/replicas", + Value: "2", + }, + }, AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, + Allowed: true, + PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), }, } }, } - alwaysDeny := &fakeHandler{ + patcher2 := &fakeHandler{ fn: func(ctx context.Context, req Request) Response { return Response{ + Patches: []jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/metadata/annotation/hello", + Value: "world", + }, + }, AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: false, + Allowed: true, + PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), }, } }, } - Context("with validating handlers", func() { - It("should deny the request if any handler denies the request", func() { - By("setting up a handler with accept and deny") - handler := MultiValidatingHandler(alwaysAllow, alwaysDeny) - - By("checking that the handler denies the request") - resp := handler.Handle(context.Background(), Request{}) - Expect(resp.Allowed).To(BeFalse()) - }) + It("should not return any patches if the request is denied", func() { + By("setting up a webhook with some patches and a deny") + handler := MultiMutatingHandler(patcher1, patcher2, alwaysDeny) - It("should allow the request if all handlers allow the request", func() { - By("setting up a handler with only accept") - handler := MultiValidatingHandler(alwaysAllow, alwaysAllow) - - By("checking that the handler allows the request") - resp := handler.Handle(context.Background(), Request{}) - Expect(resp.Allowed).To(BeTrue()) - }) + By("checking that the handler denies the request and produces no patches") + resp := handler.Handle(context.Background(), Request{}) + Expect(resp.Allowed).To(BeFalse()) + Expect(resp.Patches).To(BeEmpty()) }) - Context("with mutating handlers", func() { - patcher1 := &fakeHandler{ - fn: func(ctx context.Context, req Request) Response { - return Response{ - Patches: []jsonpatch.JsonPatchOperation{ - { - Operation: "add", - Path: "/metadata/annotation/new-key", - Value: "new-value", - }, - { - Operation: "replace", - Path: "/spec/replicas", - Value: "2", - }, - }, - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), - }, - } - }, - } - patcher2 := &fakeHandler{ - fn: func(ctx context.Context, req Request) Response { - return Response{ - Patches: []jsonpatch.JsonPatchOperation{ - { - Operation: "add", - Path: "/metadata/annotation/hello", - Value: "world", - }, - }, - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), - }, - } - }, - } + It("should produce all patches if the requests are all allowed", func() { + By("setting up a webhook with some patches") + handler := MultiMutatingHandler(patcher1, patcher2, alwaysAllow) - It("should not return any patches if the request is denied", func() { - By("setting up a webhook with some patches and a deny") - handler := MultiMutatingHandler(patcher1, patcher2, alwaysDeny) - - By("checking that the handler denies the request and produces no patches") - resp := handler.Handle(context.Background(), Request{}) - Expect(resp.Allowed).To(BeFalse()) - Expect(resp.Patches).To(BeEmpty()) - }) - - It("should produce all patches if the requests are all allowed", func() { - By("setting up a webhook with some patches") - handler := MultiMutatingHandler(patcher1, patcher2, alwaysAllow) - - By("checking that the handler accepts the request and returns all patches") - resp := handler.Handle(context.Background(), Request{}) - Expect(resp.Allowed).To(BeTrue()) - Expect(resp.Patch).To(Equal([]byte(`[{"op":"add","path":"/metadata/annotation/new-key","value":"new-value"},{"op":"replace","path":"/spec/replicas","value":"2"},{"op":"add","path":"/metadata/annotation/hello","value":"world"}]`))) - }) + By("checking that the handler accepts the request and returns all patches") + resp := handler.Handle(context.Background(), Request{}) + Expect(resp.Allowed).To(BeTrue()) + Expect(resp.Patch).To(Equal([]byte( + `[{"op":"add","path":"/metadata/annotation/new-key","value":"new-value"},` + + `{"op":"replace","path":"/spec/replicas","value":"2"},{"op":"add","path":"/metadata/annotation/hello","value":"world"}]`))) }) }) }) diff --git a/pkg/webhook/admission/response_test.go b/pkg/webhook/admission/response_test.go index 124db31d3a..ac8b4fe7ea 100644 --- a/pkg/webhook/admission/response_test.go +++ b/pkg/webhook/admission/response_test.go @@ -28,7 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -var _ = Describe("admission webhook response", func() { +var _ = Describe("Admission Webhook Response Helpers", func() { Describe("ErrorResponse", func() { It("should return the response with an error", func() { err := errors.New("this is an error") From 9a7de90bd84e93436208e341b9fa430e08e6b443 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Thu, 7 Feb 2019 23:04:58 -0800 Subject: [PATCH 09/19] Clean up webhook server This renames a few ambigously named fields in the webhook server, removes some extraneous fields and methods, and removes the unnecessary constructor for Server. --- example/main.go | 11 ++++--- pkg/webhook/doc.go | 10 +++---- pkg/webhook/server.go | 68 ++++++++++++++++--------------------------- 3 files changed, 34 insertions(+), 55 deletions(-) diff --git a/example/main.go b/example/main.go index 3f256eeb30..1d897b0e5f 100644 --- a/example/main.go +++ b/example/main.go @@ -90,18 +90,17 @@ func main() { } entryLog.Info("setting up webhook server") - as, err := webhook.NewServer(webhook.ServerOptions{ + hookServer := &webhook.Server{ Port: 9876, CertDir: "/tmp/cert", - }) - if err != nil { - entryLog.Error(err, "unable to create a new webhook server") + } + if err := mgr.Add(hookServer); err != nil { + entryLog.Error(err, "unable register webhook server with manager") os.Exit(1) } - mgr.Add(as) entryLog.Info("registering webhooks to the webhook server") - err = as.Register(mutatingWebhook, validatingWebhook) + err = hookServer.Register(mutatingWebhook, validatingWebhook) if err != nil { entryLog.Error(err, "unable to setup the admission server") os.Exit(1) diff --git a/pkg/webhook/doc.go b/pkg/webhook/doc.go index 9f40c72a70..b5e40d1bd5 100644 --- a/pkg/webhook/doc.go +++ b/pkg/webhook/doc.go @@ -33,23 +33,21 @@ Build webhooks Create a webhook server. - as, err := NewServer("baz-admission-server", mgr, ServerOptions{ + hookServer := &Server{ CertDir: "/tmp/cert", - }) - if err != nil { - // handle error } + mgr.Add(hookServer) Register the webhooks in the server. - err = as.Register(webhook1, webhook2) + err = hookServer.Register(webhook1, webhook2) if err != nil { // handle error } Start the server by starting the manager - err := mrg.Start(signals.SetupSignalHandler()) + err := mgr.Start(signals.SetupSignalHandler()) if err != nil { // handle error } diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index 381152369a..df80ad9f12 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -34,9 +34,10 @@ const ( keyName = "tls.key" ) -// ServerOptions are options for configuring an admission webhook server. -type ServerOptions struct { - // Address that the server will listen on. +// Server is an admission webhook server that can serve traffic and +// generates related k8s resources for deploying. +type Server struct { + // Host is the address that the server will listen on. // Defaults to "" - all addresses. Host string @@ -50,22 +51,20 @@ type ServerOptions struct { // If using SecretCertWriter in Provisioner, the server will provision the certificate in a secret, // the user is responsible to mount the secret to the this location for the server to consume. CertDir string -} -// Server is an admission webhook server that can serve traffic and -// generates related k8s resources for deploying. -type Server struct { - // ServerOptions contains options for configuring the admission server. - ServerOptions + // TODO(directxman12): should we make the mux configurable? - sMux *http.ServeMux - // registry maps a path to a http.Handler. - registry map[string]http.Handler + // webhookMux is the multiplexer that handles different webhooks. + webhookMux *http.ServeMux + // webhooks keep track of all registered webhooks for dependency injection, + // and to provide better panic messages on duplicate webhook registration. + webhooks map[string]Webhook // setFields allows injecting dependencies from an external source setFields inject.Func - once sync.Once + // defaultingOnce ensures that the default fields are only ever set once. + defaultingOnce sync.Once } // Webhook defines the basics that a webhook should support. @@ -80,35 +79,23 @@ type Webhook interface { Validate() error } -// NewServer creates a new admission webhook server. -func NewServer(options ServerOptions) (*Server, error) { - as := &Server{ - sMux: http.NewServeMux(), - registry: map[string]http.Handler{}, - ServerOptions: options, - } - - return as, nil -} +// setDefaults does defaulting for the Server. +func (s *Server) setDefaults() { + s.webhooks = map[string]Webhook{} + s.webhookMux = http.NewServeMux() -// setDefault does defaulting for the Server. -func (s *Server) setDefault() { - if s.registry == nil { - s.registry = map[string]http.Handler{} - } - if s.sMux == nil { - s.sMux = http.NewServeMux() - } if s.Port <= 0 { s.Port = 443 } if len(s.CertDir) == 0 { - s.CertDir = path.Join("k8s-webhook-server", "cert") + s.CertDir = path.Join("/tmp", "k8s-webhook-server", "serving-certs") } } // Register validates and registers webhook(s) in the server func (s *Server) Register(webhooks ...Webhook) error { + // TODO(directxman12): is it really worth the ergonomics hit to make this a catchable error? + // In most cases you'll probably just bail immediately anyway. for i, webhook := range webhooks { // validate the webhook before registering it. err := webhook.Validate() @@ -116,30 +103,25 @@ func (s *Server) Register(webhooks ...Webhook) error { return err } // TODO(directxman12): call setfields if we've already started the server - _, found := s.registry[webhook.GetPath()] + _, found := s.webhooks[webhook.GetPath()] if found { return fmt.Errorf("can't register duplicate path: %v", webhook.GetPath()) } - s.registry[webhook.GetPath()] = webhooks[i] - s.sMux.Handle(webhook.GetPath(), webhook) + s.webhooks[webhook.GetPath()] = webhooks[i] + s.webhookMux.Handle(webhook.GetPath(), webhook) } return nil } -// Handle registers a http.Handler for the given pattern. -func (s *Server) Handle(pattern string, handler http.Handler) { - s.sMux.Handle(pattern, handler) -} - // Start runs the server. // It will install the webhook related resources depend on the server configuration. func (s *Server) Start(stop <-chan struct{}) error { - s.once.Do(s.setDefault) + s.defaultingOnce.Do(s.setDefaults) // inject fields here as opposed to in Register so that we're certain to have our setFields // function available. - for _, webhook := range s.registry { + for _, webhook := range s.webhooks { if err := s.setFields(webhook); err != nil { return err } @@ -161,7 +143,7 @@ func (s *Server) Start(stop <-chan struct{}) error { } srv := &http.Server{ - Handler: s.sMux, + Handler: s.webhookMux, } idleConnsClosed := make(chan struct{}) From f46c199f6ab2d874a741b15816027d8db0561a69 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Thu, 7 Feb 2019 23:10:32 -0800 Subject: [PATCH 10/19] Remove the extraneous name field from webhooks It was only being used for registering metrics, and that can be done with the path instead. --- pkg/webhook/admission/http.go | 6 +++--- pkg/webhook/admission/webhook.go | 26 -------------------------- pkg/webhook/admission/webhook_test.go | 9 --------- 3 files changed, 3 insertions(+), 38 deletions(-) diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 29028ccf2b..71d0319abf 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -49,7 +49,7 @@ var _ http.Handler = &Webhook{} func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { startTS := time.Now() - defer metrics.RequestLatency.WithLabelValues(wh.Name).Observe(time.Now().Sub(startTS).Seconds()) + defer metrics.RequestLatency.WithLabelValues(wh.Path).Observe(time.Now().Sub(startTS).Seconds()) var body []byte var err error @@ -100,9 +100,9 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { func (wh *Webhook) writeResponse(w io.Writer, response Response) { if response.Result.Code != 0 { if response.Result.Code == http.StatusOK { - metrics.TotalRequests.WithLabelValues(wh.Name, "true").Inc() + metrics.TotalRequests.WithLabelValues(wh.Path, "true").Inc() } else { - metrics.TotalRequests.WithLabelValues(wh.Name, "false").Inc() + metrics.TotalRequests.WithLabelValues(wh.Path, "false").Inc() } } diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index e322e33dea..ba0097f8ac 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -20,9 +20,6 @@ import ( "context" "errors" "net/http" - "regexp" - "strings" - "sync" "github.com/appscode/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" @@ -108,23 +105,11 @@ func (f HandlerFunc) Handle(ctx context.Context, req Request) Response { // Webhook represents each individual webhook. type Webhook struct { - // Name is the name of the webhook - Name string // Path is the path this webhook will serve. Path string // Handler actually processes an admission request returning whether it was allowed or denied, // and potentially patches to apply to the handler. Handler Handler - - once sync.Once -} - -func (w *Webhook) setDefaults() { - if len(w.Name) == 0 { - reg := regexp.MustCompile("[^a-zA-Z0-9]+") - processedPath := strings.ToLower(reg.ReplaceAllString(w.Path, "")) - w.Name = processedPath + ".example.com" - } } // Webhook implements Handler interface. @@ -144,24 +129,13 @@ func (w *Webhook) Handle(ctx context.Context, req Request) Response { return resp } -// GetName returns the name of the webhook. -func (w *Webhook) GetName() string { - w.once.Do(w.setDefaults) - return w.Name -} - // GetPath returns the path that the webhook registered. func (w *Webhook) GetPath() string { - w.once.Do(w.setDefaults) return w.Path } // Validate validates if the webhook is valid. func (w *Webhook) Validate() error { - w.once.Do(w.setDefaults) - if len(w.Name) == 0 { - return errors.New("field Name should not be empty") - } if len(w.Path) == 0 { return errors.New("field Path should not be empty") } diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index d490a98084..f627b177ff 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -124,15 +124,6 @@ var _ = Describe("Admission Webhooks", func() { Expect(resp.Patch).To(Equal([]byte(`[{"op":"add","path":"/a","value":2},{"op":"replace","path":"/b","value":4}]`))) }) - It("should default a name if none is given", func() { - wh := &Webhook{ - Path: "/somepath", - Handler: &fakeHandler{}, - } - - Expect(wh.GetName()).NotTo(BeEmpty()) - }) - Context("validation", func() { It("should accept a properly populated webhook", func() { wh := &Webhook{ From 027ce71abc1d45ad353e2676c94c4b01e291e887 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Fri, 8 Feb 2019 10:12:03 -0800 Subject: [PATCH 11/19] Remove path from admission, more response helpers This removes the path functionality from admission webhooks, since it's not strictly needed. Relevant metrics are moved up to the webhook server, and path is added to register. This also introduces a couple of helpers that should make code a bit clearer when writing admission webhooks (Allowed, Denied, Patched), and an alias file to avoid importing both webhook and webhook/admission. --- example/main.go | 19 +--- example/mutatingwebhook.go | 18 +-- example/validatingwebhook.go | 27 ++--- pkg/webhook/admission/http.go | 27 +---- pkg/webhook/admission/inject.go | 2 +- pkg/webhook/admission/response.go | 22 ++++ pkg/webhook/admission/response_test.go | 150 ++++++++++++++++++++++--- pkg/webhook/admission/types.go | 30 ----- pkg/webhook/admission/webhook.go | 21 ---- pkg/webhook/admission/webhook_test.go | 32 +----- pkg/webhook/alias.go | 63 +++++++++++ pkg/webhook/server.go | 56 ++++----- 12 files changed, 264 insertions(+), 203 deletions(-) delete mode 100644 pkg/webhook/admission/types.go create mode 100644 pkg/webhook/alias.go diff --git a/example/main.go b/example/main.go index 1d897b0e5f..fdc78fe1be 100644 --- a/example/main.go +++ b/example/main.go @@ -32,7 +32,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager/signals" "sigs.k8s.io/controller-runtime/pkg/source" "sigs.k8s.io/controller-runtime/pkg/webhook" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) var log = logf.Log.WithName("example-controller") @@ -78,17 +77,6 @@ func main() { } // Setup webhooks - entryLog.Info("setting up webhooks") - mutatingWebhook := &admission.Webhook{ - Path: "/mutate-pods", - Handler: &podAnnotator{}, - } - - validatingWebhook := &admission.Webhook{ - Path: "/validate-pods", - Handler: &podValidator{}, - } - entryLog.Info("setting up webhook server") hookServer := &webhook.Server{ Port: 9876, @@ -100,11 +88,8 @@ func main() { } entryLog.Info("registering webhooks to the webhook server") - err = hookServer.Register(mutatingWebhook, validatingWebhook) - if err != nil { - entryLog.Error(err, "unable to setup the admission server") - os.Exit(1) - } + hookServer.Register("/mutate-pods", webhook.Admission{&podAnnotator{}}) + hookServer.Register("/validate-pods", webhook.Admission{&podValidator{}}) entryLog.Info("starting manager") if err := mgr.Start(signals.SetupSignalHandler()); err != nil { diff --git a/example/mutatingwebhook.go b/example/mutatingwebhook.go index 945a9f8951..77b3900894 100644 --- a/example/mutatingwebhook.go +++ b/example/mutatingwebhook.go @@ -32,9 +32,6 @@ type podAnnotator struct { decoder admission.Decoder } -// Implement admission.Handler so the controller can handle admission request. -var _ admission.Handler = &podAnnotator{} - // podAnnotator adds an annotation to every incoming pods. func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admission.Response { pod := &corev1.Pod{} @@ -44,10 +41,10 @@ func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admiss return admission.ErrorResponse(http.StatusBadRequest, err) } - err = a.mutatePodsFn(ctx, pod) - if err != nil { - return admission.ErrorResponse(http.StatusInternalServerError, err) + if pod.Annotations == nil { + pod.Annotations = map[string]string{} } + pod.Annotations["example-mutating-admission-webhook"] = "foo" marshaledPod, err := json.Marshal(pod) if err != nil { @@ -57,15 +54,6 @@ func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admiss return admission.PatchResponseFromRaw(req.AdmissionRequest.Object.Raw, marshaledPod) } -// mutatePodsFn add an annotation to the given pod -func (a *podAnnotator) mutatePodsFn(ctx context.Context, pod *corev1.Pod) error { - if pod.Annotations == nil { - pod.Annotations = map[string]string{} - } - pod.Annotations["example-mutating-admission-webhook"] = "foo" - return nil -} - // podAnnotator implements inject.Client. // A client will be automatically injected. diff --git a/example/validatingwebhook.go b/example/validatingwebhook.go index 94b8d9160b..c52630c6a6 100644 --- a/example/validatingwebhook.go +++ b/example/validatingwebhook.go @@ -32,9 +32,6 @@ type podValidator struct { decoder admission.Decoder } -// Implement admission.Handler so the controller can handle admission request. -var _ admission.Handler = &podValidator{} - // podValidator admits a pod iff a specific annotation exists. func (v *podValidator) Handle(ctx context.Context, req admission.Request) admission.Response { pod := &corev1.Pod{} @@ -44,26 +41,16 @@ func (v *podValidator) Handle(ctx context.Context, req admission.Request) admiss return admission.ErrorResponse(http.StatusBadRequest, err) } - allowed, reason, err := v.validatePodsFn(ctx, pod) - if err != nil { - return admission.ErrorResponse(http.StatusInternalServerError, err) - } - return admission.ValidationResponse(allowed, reason) -} - -func (v *podValidator) validatePodsFn(ctx context.Context, pod *corev1.Pod) (bool, string, error) { key := "example-mutating-admission-webhook" anno, found := pod.Annotations[key] - switch { - case !found: - return found, fmt.Sprintf("failed to find annotation with key: %q", key), nil - case found && anno == "foo": - return found, "", nil - case found && anno != "foo": - return false, - fmt.Sprintf("the value associate with key %q is expected to be %q, but got %q", key, "foo", anno), nil + if !found { + return admission.Denied(fmt.Sprintf("missing annotation %s", key)) + } + if anno != "foo" { + return admission.Denied(fmt.Sprintf("annotation %s did not have value %q", key, "foo")) } - return false, "", nil + + return admission.Allowed("") } // podValidator implements inject.Client. diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 71d0319abf..d678e24778 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -24,33 +24,24 @@ import ( "io" "io/ioutil" "net/http" - "time" "k8s.io/api/admission/v1beta1" admissionv1beta1 "k8s.io/api/admission/v1beta1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - "sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics" ) -var admissionv1beta1scheme = runtime.NewScheme() -var admissionv1beta1schemecodecs = serializer.NewCodecFactory(admissionv1beta1scheme) +var admissionScheme = runtime.NewScheme() +var admissionCodecs = serializer.NewCodecFactory(admissionScheme) func init() { - addToScheme(admissionv1beta1scheme) -} - -func addToScheme(scheme *runtime.Scheme) { - utilruntime.Must(admissionv1beta1.AddToScheme(scheme)) + utilruntime.Must(admissionv1beta1.AddToScheme(admissionScheme)) } var _ http.Handler = &Webhook{} -func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { - startTS := time.Now() - defer metrics.RequestLatency.WithLabelValues(wh.Path).Observe(time.Now().Sub(startTS).Seconds()) - +func (wh Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { var body []byte var err error @@ -85,7 +76,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { // avoid an extra copy Request: &req.AdmissionRequest, } - if _, _, err := admissionv1beta1schemecodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil { + if _, _, err := admissionCodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil { log.Error(err, "unable to decode the request") reviewResponse = ErrorResponse(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) @@ -98,14 +89,6 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { } func (wh *Webhook) writeResponse(w io.Writer, response Response) { - if response.Result.Code != 0 { - if response.Result.Code == http.StatusOK { - metrics.TotalRequests.WithLabelValues(wh.Path, "true").Inc() - } else { - metrics.TotalRequests.WithLabelValues(wh.Path, "false").Inc() - } - } - encoder := json.NewEncoder(w) responseAdmissionReview := v1beta1.AdmissionReview{ Response: &response.AdmissionResponse, diff --git a/pkg/webhook/admission/inject.go b/pkg/webhook/admission/inject.go index 4127754ae5..b56bb6e971 100644 --- a/pkg/webhook/admission/inject.go +++ b/pkg/webhook/admission/inject.go @@ -16,7 +16,7 @@ limitations under the License. package admission -// Decoder is used by the ControllerManager to inject decoder into webhook handlers. +// DecoderInjector is used by the ControllerManager to inject decoder into webhook handlers. type DecoderInjector interface { InjectDecoder(Decoder) error } diff --git a/pkg/webhook/admission/response.go b/pkg/webhook/admission/response.go index c76ad3b63c..0701abc70d 100644 --- a/pkg/webhook/admission/response.go +++ b/pkg/webhook/admission/response.go @@ -25,6 +25,28 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// Allowed constructs a response indicating that the given operation +// is allowed (without any patches). +func Allowed(reason string) Response { + return ValidationResponse(true, reason) +} + +// Denied constructs a response indicating that the given operation +// is not allowed. +func Denied(reason string) Response { + return ValidationResponse(false, reason) +} + +// Patched constructs a response indicating that the given operation is +// allowed, and that the target object should be modified by the given +// JSONPatch operations. +func Patched(reason string, patches ...jsonpatch.JsonPatchOperation) Response { + resp := Allowed(reason) + resp.Patches = patches + + return resp +} + // ErrorResponse creates a new Response for error-handling a request. func ErrorResponse(code int32, err error) Response { return Response{ diff --git a/pkg/webhook/admission/response_test.go b/pkg/webhook/admission/response_test.go index ac8b4fe7ea..f9fe987567 100644 --- a/pkg/webhook/admission/response_test.go +++ b/pkg/webhook/admission/response_test.go @@ -29,8 +29,95 @@ import ( ) var _ = Describe("Admission Webhook Response Helpers", func() { + Describe("Allowed", func() { + It("should return an 'allowed' response", func() { + Expect(Allowed("")).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + }, + }, + )) + }) + + It("should populate a status with a reason when a reason is given", func() { + Expect(Allowed("acceptable")).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + Result: &metav1.Status{ + Reason: "acceptable", + }, + }, + }, + )) + }) + }) + + Describe("Denied", func() { + It("should return a 'not allowed' response", func() { + Expect(Denied("")).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: false, + }, + }, + )) + }) + + It("should populate a status with a reason when a reason is given", func() { + Expect(Denied("UNACCEPTABLE!")).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: false, + Result: &metav1.Status{ + Reason: "UNACCEPTABLE!", + }, + }, + }, + )) + }) + }) + + Describe("Patched", func() { + ops := []jsonpatch.JsonPatchOperation{ + { + Operation: "replace", + Path: "/spec/selector/matchLabels", + Value: map[string]string{"foo": "bar"}, + }, + { + Operation: "delete", + Path: "/spec/replicas", + }, + } + It("should return an 'allowed' response with the given patches", func() { + Expect(Patched("", ops...)).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + }, + Patches: ops, + }, + )) + }) + It("should populate a status with a reason when a reason is given", func() { + Expect(Patched("some changes", ops...)).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + Result: &metav1.Status{ + Reason: "some changes", + }, + }, + Patches: ops, + }, + )) + }) + }) + Describe("ErrorResponse", func() { - It("should return the response with an error", func() { + It("should return a denied response with an error", func() { err := errors.New("this is an error") expected := Response{ AdmissionResponse: admissionv1beta1.AdmissionResponse{ @@ -47,30 +134,65 @@ var _ = Describe("Admission Webhook Response Helpers", func() { }) Describe("ValidationResponse", func() { - It("should return the response with an admission decision", func() { - expected := Response{ - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{ - Reason: metav1.StatusReason("allow to admit"), + It("should populate a status with a reason when a reason is given", func() { + By("checking that a message is populated for 'allowed' responses") + Expect(ValidationResponse(true, "acceptable")).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + Result: &metav1.Status{ + Reason: "acceptable", + }, }, }, - } - resp := ValidationResponse(true, "allow to admit") - Expect(resp).To(Equal(expected)) + )) + + By("checking that a message is populated for 'denied' responses") + Expect(ValidationResponse(false, "UNACCEPTABLE!")).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: false, + Result: &metav1.Status{ + Reason: "UNACCEPTABLE!", + }, + }, + }, + )) + }) + + It("should return an admission decision", func() { + By("checking that it returns a 'denied' response when allowed is false") + Expect(ValidationResponse(true, "")).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: true, + }, + }, + )) + + By("checking that it returns an 'allowed' response when allowed is true") + Expect(ValidationResponse(false, "")).To(Equal( + Response{ + AdmissionResponse: admissionv1beta1.AdmissionResponse{ + Allowed: false, + }, + }, + )) }) }) - Describe("PatchResponse", func() { - It("should return the response with patches", func() { + Describe("PatchResponseFromRaw", func() { + It("should return an 'allowed' response with a patch of the diff between two sets of serialized JSON", func() { expected := Response{ - Patches: []jsonpatch.JsonPatchOperation{}, + Patches: []jsonpatch.JsonPatchOperation{ + {Operation: "replace", Path: "/a", Value: "bar"}, + }, AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: true, PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), }, } - resp := PatchResponseFromRaw([]byte(`{}`), []byte(`{}`)) + resp := PatchResponseFromRaw([]byte(`{"a": "foo"}`), []byte(`{"a": "bar"}`)) Expect(resp).To(Equal(expected)) }) }) diff --git a/pkg/webhook/admission/types.go b/pkg/webhook/admission/types.go deleted file mode 100644 index 7a57f498fd..0000000000 --- a/pkg/webhook/admission/types.go +++ /dev/null @@ -1,30 +0,0 @@ -/* -Copyright 2018 The Kubernetes 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 admission - -// WebhookType defines the type of an admission webhook -// (validating or mutating). -type WebhookType int - -const ( - _ WebhookType = iota - // MutatingWebhook represents a webhook that can mutate the object sent to it. - MutatingWebhook - // ValidatingWebhook represents a webhook that can only gate whether or not objects - // sent to it are accepted. - ValidatingWebhook -) diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index ba0097f8ac..ed5a829df0 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -105,16 +105,11 @@ func (f HandlerFunc) Handle(ctx context.Context, req Request) Response { // Webhook represents each individual webhook. type Webhook struct { - // Path is the path this webhook will serve. - Path string // Handler actually processes an admission request returning whether it was allowed or denied, // and potentially patches to apply to the handler. Handler Handler } -// Webhook implements Handler interface. -var _ Handler = &Webhook{} - // Handle processes AdmissionRequest. // If the webhook is mutating type, it delegates the AdmissionRequest to each handler and merge the patches. // If the webhook is validating type, it delegates the AdmissionRequest to each handler and @@ -129,22 +124,6 @@ func (w *Webhook) Handle(ctx context.Context, req Request) Response { return resp } -// GetPath returns the path that the webhook registered. -func (w *Webhook) GetPath() string { - return w.Path -} - -// Validate validates if the webhook is valid. -func (w *Webhook) Validate() error { - if len(w.Path) == 0 { - return errors.New("field Path should not be empty") - } - if w.Handler == nil { - return errors.New("field Handler should not be empty") - } - return nil -} - // InjectFunc injects the field setter into the webhook. func (w *Webhook) InjectFunc(f inject.Func) error { // inject directly into the handlers. It would be more correct diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index f627b177ff..3501e701b4 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -105,13 +105,7 @@ var _ = Describe("Admission Webhooks", func() { By("setting up a webhook with a patching handler") webhook := &Webhook{ Handler: HandlerFunc(func(ctx context.Context, req Request) Response { - return Response{ - Patches: []jsonpatch.Operation{jsonpatch.Operation{Operation: "add", Path: "/a", Value: 2}, jsonpatch.Operation{Operation: "replace", Path: "/b", Value: 4}}, - AdmissionResponse: admissionv1beta1.AdmissionResponse{ - Allowed: true, - PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), - }, - } + return Patched("", jsonpatch.Operation{Operation: "add", Path: "/a", Value: 2}, jsonpatch.Operation{Operation: "replace", Path: "/b", Value: 4}) }), } @@ -123,28 +117,4 @@ var _ = Describe("Admission Webhooks", func() { Expect(resp.PatchType).To(Equal(&patchType)) Expect(resp.Patch).To(Equal([]byte(`[{"op":"add","path":"/a","value":2},{"op":"replace","path":"/b","value":4}]`))) }) - - Context("validation", func() { - It("should accept a properly populated webhook", func() { - wh := &Webhook{ - Path: "/somepath", - Handler: &fakeHandler{}, - } - Expect(wh.Validate()).To(Succeed()) - }) - - It("should fail when missing a path", func() { - wh := &Webhook{ - Handler: &fakeHandler{}, - } - Expect(wh.Validate()).NotTo(Succeed()) - }) - - It("should fail when missing a handler", func() { - wh := &Webhook{ - Path: "/somepath", - } - Expect(wh.Validate()).NotTo(Succeed()) - }) - }) }) diff --git a/pkg/webhook/alias.go b/pkg/webhook/alias.go new file mode 100644 index 0000000000..9bc7f468b3 --- /dev/null +++ b/pkg/webhook/alias.go @@ -0,0 +1,63 @@ +/* +Copyright 2019 The Kubernetes 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 ( + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// define some aliases for common bits of the webhook functionality + +// AdmissionRequest defines the input for an admission handler. +// It contains information to identify the object in +// question (group, version, kind, resource, subresource, +// name, namespace), as well as the operation in question +// (e.g. Get, Create, etc), and the object itself. +type AdmissionRequest = admission.Request + +// AdmissionResponse is the output of an admission handler. +// It contains a response indicating if a given +// operation is allowed, as well as a set of patches +// to mutate the object in the case of a mutating admission handler. +type AdmissionResponse = admission.Response + +// Admission is webhook suitable for registration with the server +// an admission webhook that validates API operations and potentially +// mutates their contents. +type Admission = admission.Webhook + +// AdmissionHandler knows how to process admission requests, validating them, +// and potentially mutating the objects they contain. +type AdmissionHandler = admission.Handler + +// AdmissionDecoder knows how to decode objects from admission requests. +type AdmissionDecoder = admission.Decoder + +var ( + // Allowed indicates that the admission request should be allowed for the given reason. + Allowed = admission.Allowed + + // Denied indicates that the admission request should be denied for the given reason. + Denied = admission.Denied + + // Patched indicates that the admission request should be allowed for the given reason, + // and that the contained object should be mutated using the given patches. + Patched = admission.Patched + + // Errored indicates that an error occurred in the admission request. + Errored = admission.ErrorResponse +) diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index df80ad9f12..e276dc40a2 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -25,8 +25,10 @@ import ( "path" "strconv" "sync" + "time" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" + "sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics" ) const ( @@ -58,7 +60,7 @@ type Server struct { webhookMux *http.ServeMux // webhooks keep track of all registered webhooks for dependency injection, // and to provide better panic messages on duplicate webhook registration. - webhooks map[string]Webhook + webhooks map[string]http.Handler // setFields allows injecting dependencies from an external source setFields inject.Func @@ -67,21 +69,9 @@ type Server struct { defaultingOnce sync.Once } -// Webhook defines the basics that a webhook should support. -type Webhook interface { - // Webhooks handle HTTP requests. - http.Handler - - // GetPath returns the path that the webhook registered. - GetPath() string - // Validate validates if the webhook itself is valid. - // If invalid, a non-nil error will be returned. - Validate() error -} - // setDefaults does defaulting for the Server. func (s *Server) setDefaults() { - s.webhooks = map[string]Webhook{} + s.webhooks = map[string]http.Handler{} s.webhookMux = http.NewServeMux() if s.Port <= 0 { @@ -92,26 +82,28 @@ func (s *Server) setDefaults() { } } -// Register validates and registers webhook(s) in the server -func (s *Server) Register(webhooks ...Webhook) error { - // TODO(directxman12): is it really worth the ergonomics hit to make this a catchable error? - // In most cases you'll probably just bail immediately anyway. - for i, webhook := range webhooks { - // validate the webhook before registering it. - err := webhook.Validate() - if err != nil { - return err - } - // TODO(directxman12): call setfields if we've already started the server - _, found := s.webhooks[webhook.GetPath()] - if found { - return fmt.Errorf("can't register duplicate path: %v", webhook.GetPath()) - } - s.webhooks[webhook.GetPath()] = webhooks[i] - s.webhookMux.Handle(webhook.GetPath(), webhook) +// Register marks the given webhook as being served at the given path. +// It panics if two hooks are registered on the same path. +func (s *Server) Register(path string, hook http.Handler) { + s.defaultingOnce.Do(s.setDefaults) + _, found := s.webhooks[path] + if found { + panic(fmt.Errorf("can't register duplicate path: %v", path)) } + // TODO(directxman12): call setfields if we've already started the server + s.webhooks[path] = hook + s.webhookMux.Handle(path, instrumentedHook(path, hook)) +} - return nil +// instrumentedHook adds some instrumentation on top of the given webhook. +func instrumentedHook(path string, hookRaw http.Handler) http.Handler { + return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { + startTS := time.Now() + defer func() { metrics.RequestLatency.WithLabelValues(path).Observe(time.Now().Sub(startTS).Seconds()) }() + hookRaw.ServeHTTP(resp, req) + + // TODO(directxman12): add back in metric about total requests broken down by result? + }) } // Start runs the server. From 1859301ea60e60e74314ac819d966305f8bd595a Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 17:29:15 -0800 Subject: [PATCH 12/19] Construct and inject decoder into webhook handlers This ensures that decoders (which are no longer constructed in the manager) are properly injected into webhook handlers. The webhook itself receives a scheme, which it uses to construct a decoder. --- example/main.go | 4 +- example/mutatingwebhook.go | 4 +- example/validatingwebhook.go | 4 +- pkg/webhook/admission/http_test.go | 16 ++++- pkg/webhook/admission/inject.go | 4 +- pkg/webhook/admission/webhook.go | 55 +++++++++++++++- pkg/webhook/admission/webhook_test.go | 91 +++++++++++++++++++++++++++ 7 files changed, 167 insertions(+), 11 deletions(-) diff --git a/example/main.go b/example/main.go index fdc78fe1be..499de9daad 100644 --- a/example/main.go +++ b/example/main.go @@ -88,8 +88,8 @@ func main() { } entryLog.Info("registering webhooks to the webhook server") - hookServer.Register("/mutate-pods", webhook.Admission{&podAnnotator{}}) - hookServer.Register("/validate-pods", webhook.Admission{&podValidator{}}) + hookServer.Register("/mutate-pods", webhook.Admission{Handler: &podAnnotator{}}) + hookServer.Register("/validate-pods", webhook.Admission{Handler: &podValidator{}}) entryLog.Info("starting manager") if err := mgr.Start(signals.SetupSignalHandler()); err != nil { diff --git a/example/mutatingwebhook.go b/example/mutatingwebhook.go index 77b3900894..24d2d4dd6c 100644 --- a/example/mutatingwebhook.go +++ b/example/mutatingwebhook.go @@ -29,7 +29,7 @@ import ( // podAnnotator annotates Pods type podAnnotator struct { client client.Client - decoder admission.Decoder + decoder *admission.Decoder } // podAnnotator adds an annotation to every incoming pods. @@ -67,7 +67,7 @@ func (a *podAnnotator) InjectClient(c client.Client) error { // A decoder will be automatically injected. // InjectDecoder injects the decoder. -func (a *podAnnotator) InjectDecoder(d admission.Decoder) error { +func (a *podAnnotator) InjectDecoder(d *admission.Decoder) error { a.decoder = d return nil } diff --git a/example/validatingwebhook.go b/example/validatingwebhook.go index c52630c6a6..09983b33f2 100644 --- a/example/validatingwebhook.go +++ b/example/validatingwebhook.go @@ -29,7 +29,7 @@ import ( // podValidator validates Pods type podValidator struct { client client.Client - decoder admission.Decoder + decoder *admission.Decoder } // podValidator admits a pod iff a specific annotation exists. @@ -66,7 +66,7 @@ func (v *podValidator) InjectClient(c client.Client) error { // A decoder will be automatically injected. // InjectDecoder injects the decoder. -func (v *podValidator) InjectDecoder(d admission.Decoder) error { +func (v *podValidator) InjectDecoder(d *admission.Decoder) error { v.decoder = d return nil } diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index 1e220fff08..01e97756cf 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -100,8 +100,20 @@ type nopCloser struct { func (nopCloser) Close() error { return nil } type fakeHandler struct { - invoked bool - fn func(context.Context, Request) Response + invoked bool + fn func(context.Context, Request) Response + decoder *Decoder + injectedString string +} + +func (h *fakeHandler) InjectDecoder(d *Decoder) error { + h.decoder = d + return nil +} + +func (h *fakeHandler) InjectString(s string) error { + h.injectedString = s + return nil } func (h *fakeHandler) Handle(ctx context.Context, req Request) Response { diff --git a/pkg/webhook/admission/inject.go b/pkg/webhook/admission/inject.go index b56bb6e971..d5af0d598f 100644 --- a/pkg/webhook/admission/inject.go +++ b/pkg/webhook/admission/inject.go @@ -18,12 +18,12 @@ package admission // DecoderInjector is used by the ControllerManager to inject decoder into webhook handlers. type DecoderInjector interface { - InjectDecoder(Decoder) error + InjectDecoder(*Decoder) error } // InjectDecoderInto will set decoder on i and return the result if it implements Decoder. Returns // false if i does not implement Decoder. -func InjectDecoderInto(decoder Decoder, i interface{}) (bool, error) { +func InjectDecoderInto(decoder *Decoder, i interface{}) (bool, error) { if s, ok := i.(DecoderInjector); ok { return true, s.InjectDecoder(decoder) } diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index ed5a829df0..5a2907c086 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -24,6 +24,7 @@ import ( "github.com/appscode/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/json" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" @@ -108,6 +109,9 @@ type Webhook struct { // Handler actually processes an admission request returning whether it was allowed or denied, // and potentially patches to apply to the handler. Handler Handler + + // scheme is used to construct the Decoder + decoder *Decoder } // Handle processes AdmissionRequest. @@ -124,11 +128,60 @@ func (w *Webhook) Handle(ctx context.Context, req Request) Response { return resp } +// InjectScheme injects a scheme into the webhook, in order to construct a Decoder. +func (w *Webhook) InjectScheme(s *runtime.Scheme) error { + // TODO(directxman12): we should have a better way to pass this down + + var err error + w.decoder, err = NewDecoder(s) + if err != nil { + return err + } + + // inject the decoder here too, just in case the order of calling this is not + // scheme first, then inject func + if w.Handler != nil { + if _, err := InjectDecoderInto(w.GetDecoder(), w.Handler); err != nil { + return err + } + } + + return nil +} + +// GetDecoder returns a decoder to decode the objects embedded in admission requests. +// It may be nil if we haven't received a scheme to use to determine object types yet. +func (w *Webhook) GetDecoder() *Decoder { + return w.decoder +} + // InjectFunc injects the field setter into the webhook. func (w *Webhook) InjectFunc(f inject.Func) error { // inject directly into the handlers. It would be more correct // to do this in a sync.Once in Handle (since we don't have some // other start/finalize-type method), but it's more efficient to // do it here, presumably. - return f(w.Handler) + + // also inject a decoder, and wrap this so that we get a setFields + // that injects a decoder (hopefully things don't ignore the duplicate + // InjectorInto call). + + var setFields inject.Func + setFields = func(target interface{}) error { + if err := f(target); err != nil { + return err + } + + if _, err := inject.InjectorInto(setFields, target); err != nil { + return err + } + + if _, err := InjectDecoderInto(w.GetDecoder(), target); err != nil { + return err + } + + return nil + } + + return setFields(w.Handler) } diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index 3501e701b4..2097414de0 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -26,7 +26,10 @@ import ( "github.com/appscode/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" machinerytypes "k8s.io/apimachinery/pkg/types" + + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) var _ = Describe("Admission Webhooks", func() { @@ -117,4 +120,92 @@ var _ = Describe("Admission Webhooks", func() { Expect(resp.PatchType).To(Equal(&patchType)) Expect(resp.Patch).To(Equal([]byte(`[{"op":"add","path":"/a","value":2},{"op":"replace","path":"/b","value":4}]`))) }) + + Describe("dependency injection", func() { + It("should set dependencies passed in on the handler", func() { + By("setting up a webhook and injecting it with a injection func that injects a string") + setFields := func(target interface{}) error { + inj, ok := target.(stringInjector) + if !ok { + return nil + } + + return inj.InjectString("something") + } + handler := &fakeHandler{} + webhook := &Webhook{ + Handler: handler, + } + Expect(setFields(webhook)).To(Succeed()) + Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) + + By("checking that the string was injected") + Expect(handler.injectedString).To(Equal("something")) + }) + + It("should inject a decoder into the handler", func() { + By("setting up a webhook and injecting it with a injection func that injects a scheme") + setFields := func(target interface{}) error { + if _, err := inject.SchemeInto(runtime.NewScheme(), target); err != nil { + return err + } + return nil + } + handler := &fakeHandler{} + webhook := &Webhook{ + Handler: handler, + } + Expect(setFields(webhook)).To(Succeed()) + Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) + + By("checking that the decoder was injected") + Expect(handler.decoder).NotTo(BeNil()) + }) + + It("should pass a setFields that also injects a decoder into sub-dependencies", func() { + By("setting up a webhook and injecting it with a injection func that injects a scheme") + setFields := func(target interface{}) error { + if _, err := inject.SchemeInto(runtime.NewScheme(), target); err != nil { + return err + } + return nil + } + handler := &handlerWithSubDependencies{ + Handler: HandlerFunc(func(ctx context.Context, req Request) Response { + return Response{} + }), + dep: &subDep{}, + } + webhook := &Webhook{ + Handler: handler, + } + Expect(setFields(webhook)).To(Succeed()) + Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) + + By("checking that setFields sets the decoder as well") + Expect(handler.dep.decoder).NotTo(BeNil()) + }) + }) }) + +type stringInjector interface { + InjectString(s string) error +} + +type handlerWithSubDependencies struct { + Handler + dep *subDep +} + +func (h *handlerWithSubDependencies) InjectFunc(f inject.Func) error { + return f(h.dep) +} + +type subDep struct { + decoder *Decoder +} + +func (d *subDep) InjectDecoder(dec *Decoder) error { + d.decoder = dec + return nil +} From 107c8e98f5779511fb98eff215f0d87d65ee4da6 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 17:42:34 -0800 Subject: [PATCH 13/19] Fix admission for decoding for unstructured Due to a weird interaction between the unstructured decoder (which demands APIVersion and Kind fields) and the API server (which fails to set those fields on the embedded object in admission requests), we can't use the normal decoders, nor can we call json.Unmarshal directly on the unstructured (since it implements an unmarshaller that calls back into the decoder). This detects unstructured objects, and does the right thing for them. --- pkg/webhook/admission/decode.go | 18 ++++++++++++++++++ pkg/webhook/admission/decode_test.go | 13 +++++++++++++ 2 files changed, 31 insertions(+) diff --git a/pkg/webhook/admission/decode.go b/pkg/webhook/admission/decode.go index 621cca5948..ced04ed060 100644 --- a/pkg/webhook/admission/decode.go +++ b/pkg/webhook/admission/decode.go @@ -17,8 +17,10 @@ limitations under the License. package admission import ( + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/util/json" ) // Decoder knows how to decode the contents of an admission @@ -34,6 +36,22 @@ func NewDecoder(scheme *runtime.Scheme) (*Decoder, error) { // Decode decodes the inlined object in the AdmissionRequest into the passed-in runtime.Object. func (d *Decoder) Decode(req Request, into runtime.Object) error { + // NB(directxman12): there's a bug/weird interaction between decoders and + // the API server where the API server doesn't send a GVK on the embedded + // objects, which means the unstructured decoder refuses to decode. It + // also means we can't pass the unstructured directly in, since it'll try + // and call unstructured's special Unmarshal implementation, which calls + // back into that same decoder :-/ + // See kubernetes/kubernetes#74373. + if unstructuredInto, isUnstructured := into.(*unstructured.Unstructured); isUnstructured { + // unmarshal into unstructured's underlying object to avoid calling the decoder + if err := json.Unmarshal(req.Object.Raw, &unstructuredInto.Object); err != nil { + return err + } + + return nil + } + deserializer := d.codecs.UniversalDeserializer() return runtime.DecodeInto(deserializer, req.AdmissionRequest.Object.Raw, into) } diff --git a/pkg/webhook/admission/decode_test.go b/pkg/webhook/admission/decode_test.go index e41f3aa852..efba146d09 100644 --- a/pkg/webhook/admission/decode_test.go +++ b/pkg/webhook/admission/decode_test.go @@ -23,6 +23,7 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/scheme" ) @@ -87,4 +88,16 @@ var _ = Describe("Admission Webhook Decoder", func() { By("trying to extract a pod into a node") Expect(decoder.Decode(req, &corev1.Node{})).NotTo(Succeed()) }) + + It("should be able to decode into an unstructured object", func() { + By("decoding into an unstructured object") + var target unstructured.Unstructured + Expect(decoder.Decode(req, &target)).To(Succeed()) + + By("sanity-checking the metadata on the output object") + Expect(target.Object["metadata"]).To(Equal(map[string]interface{}{ + "name": "foo", + "namespace": "default", + })) + }) }) From e08eee7714798dc28f676ab415c06107c19b6ee6 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 17:57:47 -0800 Subject: [PATCH 14/19] Move webhook examples to an actual Go example file This moves the examples to an actual Go example file, so that we can be sure that they actually compile. --- pkg/webhook/alias.go | 4 +++ pkg/webhook/doc.go | 33 ------------------ pkg/webhook/example_test.go | 67 +++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 33 deletions(-) create mode 100644 pkg/webhook/example_test.go diff --git a/pkg/webhook/alias.go b/pkg/webhook/alias.go index 9bc7f468b3..6639e6bc73 100644 --- a/pkg/webhook/alias.go +++ b/pkg/webhook/alias.go @@ -17,6 +17,7 @@ limitations under the License. package webhook import ( + "github.com/appscode/jsonpatch" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -47,6 +48,9 @@ type AdmissionHandler = admission.Handler // AdmissionDecoder knows how to decode objects from admission requests. type AdmissionDecoder = admission.Decoder +// JSONPatchOp represents a single JSONPatch patch operation. +type JSONPatchOp = jsonpatch.Operation + var ( // Allowed indicates that the admission request should be allowed for the given reason. Allowed = admission.Allowed diff --git a/pkg/webhook/doc.go b/pkg/webhook/doc.go index b5e40d1bd5..2c93f0d995 100644 --- a/pkg/webhook/doc.go +++ b/pkg/webhook/doc.go @@ -18,39 +18,6 @@ limitations under the License. Package webhook provides methods to build and bootstrap a webhook server. Currently, it only supports admission webhooks. It will support CRD conversion webhooks in the near future. - -Build webhooks - - webhook1 := &Webhook{ - Path: "/mutating-pods", - Handler: mutatingHandler, - } - - webhook2 := &Webhook{ - Path: "/validating-pods", - Handler: validatingHandler, - } - -Create a webhook server. - - hookServer := &Server{ - CertDir: "/tmp/cert", - } - mgr.Add(hookServer) - -Register the webhooks in the server. - - err = hookServer.Register(webhook1, webhook2) - if err != nil { - // handle error - } - -Start the server by starting the manager - - err := mgr.Start(signals.SetupSignalHandler()) - if err != nil { - // handle error - } */ package webhook diff --git a/pkg/webhook/example_test.go b/pkg/webhook/example_test.go new file mode 100644 index 0000000000..b2fc77ec1d --- /dev/null +++ b/pkg/webhook/example_test.go @@ -0,0 +1,67 @@ +/* +Copyright 2018 The Kubernetes 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_test + +import ( + "context" + + ctrl "sigs.k8s.io/controller-runtime" + . "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +var ( + mgr ctrl.Manager +) + +func Example() { + // Build webhooks + // These handlers could be also be implementations + // of the AdmissionHandler interface for more complex + // implementations. + mutatingHook := &Admission{ + Handler: admission.HandlerFunc(func(ctx context.Context, req AdmissionRequest) AdmissionResponse { + return Patched("some changes", + JSONPatchOp{Operation: "add", Path: "/metadata/annotations/access", Value: "granted"}, + JSONPatchOp{Operation: "add", Path: "/metadata/annotations/reason", Value: "not so secret"}, + ) + }), + } + + validatingHook := &Admission{ + Handler: admission.HandlerFunc(func(ctx context.Context, req AdmissionRequest) AdmissionResponse { + return Denied("none shall pass!") + }), + } + + // Create a webhook server. + hookServer := &Server{ + Port: 8443, + } + mgr.Add(hookServer) + + // Register the webhooks in the server. + hookServer.Register("/mutating", mutatingHook) + hookServer.Register("/validating", validatingHook) + + // Start the server by starting a previously-set-up manager + err := mgr.Start(ctrl.SetupSignalHandler()) + if err != nil { + // handle error + panic(err) + } +} From 65a93df14bbe8139942d2ac5c714b1b6f4ad0410 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 18:01:43 -0800 Subject: [PATCH 15/19] Remove pkg/internal/admission It's not used anywhere any more. --- hack/test-all.sh | 4 +- pkg/internal/admission/decode.go | 48 ----------- pkg/internal/admission/doc.go | 18 ----- .../admission/example_admissionfunc_test.go | 42 ---------- pkg/internal/admission/example_decode_test.go | 47 ----------- .../admission/example_handlefunc_test.go | 59 -------------- pkg/internal/admission/example_test.go | 43 ---------- pkg/internal/admission/handler.go | 72 ----------------- pkg/internal/admission/http.go | 80 ------------------- pkg/internal/admission/response.go | 48 ----------- pkg/internal/admission/tls.go | 42 ---------- 11 files changed, 2 insertions(+), 501 deletions(-) delete mode 100644 pkg/internal/admission/decode.go delete mode 100644 pkg/internal/admission/doc.go delete mode 100644 pkg/internal/admission/example_admissionfunc_test.go delete mode 100644 pkg/internal/admission/example_decode_test.go delete mode 100644 pkg/internal/admission/example_handlefunc_test.go delete mode 100644 pkg/internal/admission/example_test.go delete mode 100644 pkg/internal/admission/handler.go delete mode 100644 pkg/internal/admission/http.go delete mode 100644 pkg/internal/admission/response.go delete mode 100644 pkg/internal/admission/tls.go diff --git a/hack/test-all.sh b/hack/test-all.sh index df76a1a0f1..594e7eaa05 100755 --- a/hack/test-all.sh +++ b/hack/test-all.sh @@ -28,10 +28,10 @@ header_text "running coverage" # Verify no coverage regressions have been introduced. Remove the exception list from here # once the coverage has been brought back up -if [[ ! $(go test ./pkg/... -coverprofile cover.out -parallel 4 | grep -v "coverage: 100.0% of statements" | grep "controller-runtime/pkg " | grep -v "controller-runtime/pkg \|controller-runtime/pkg/recorder \|pkg/admission/certprovisioner \|pkg/internal/admission \|pkg/cache\|pkg/client \|pkg/event \|pkg/client/config \|pkg/controller/controllertest \|pkg/reconcile/reconciletest \|pkg/test ") ]]; then +if [[ ! $(go test ./pkg/... -coverprofile cover.out -parallel 4 | grep -v "coverage: 100.0% of statements" | grep "controller-runtime/pkg " | grep -v "controller-runtime/pkg \|controller-runtime/pkg/recorder \|pkg/cache\|pkg/client \|pkg/event \|pkg/client/config \|pkg/controller/controllertest \|pkg/reconcile/reconciletest \|pkg/test ") ]]; then echo "ok" else -go test ./pkg/... -coverprofile cover.out -parallel 4 | grep -v "coverage: 100.0% of statements" | grep "controller-runtime/pkg " | grep -v "controller-runtime/pkg \|controller-runtime/pkg/recorder \|pkg/admission/certprovisioner \|pkg/internal/admission \|pkg/cache\|pkg/client \|pkg/event \|pkg/client/config \|pkg/controller/controllertest \|pkg/reconcile/reconciletest \|pkg/test " +go test ./pkg/... -coverprofile cover.out -parallel 4 | grep -v "coverage: 100.0% of statements" | grep "controller-runtime/pkg " | grep -v "controller-runtime/pkg \|controller-runtime/pkg/recorder \|pkg/cache\|pkg/client \|pkg/event \|pkg/client/config \|pkg/controller/controllertest \|pkg/reconcile/reconciletest \|pkg/test " echo "missing test coverage" exit 1 fi diff --git a/pkg/internal/admission/decode.go b/pkg/internal/admission/decode.go deleted file mode 100644 index e3dae2dbbd..0000000000 --- a/pkg/internal/admission/decode.go +++ /dev/null @@ -1,48 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission - -import ( - "fmt" - - "k8s.io/api/admission/v1beta1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/serializer" -) - -var ( - scheme = runtime.NewScheme() - codecs = serializer.NewCodecFactory(scheme) -) - -// Decode reads the Raw data from review and deserializes it into object returning a non-nil response if there was an -// error -func Decode(review v1beta1.AdmissionReview, object runtime.Object, - resourceType metav1.GroupVersionResource) *v1beta1.AdmissionResponse { - if review.Request.Resource != resourceType { - return ErrorResponse(fmt.Errorf("expect resource to be %s", resourceType)) - } - - raw := review.Request.Object.Raw - deserializer := codecs.UniversalDeserializer() - if _, _, err := deserializer.Decode(raw, nil, object); err != nil { - fmt.Printf("%v", err) - return ErrorResponse(err) - } - return nil -} diff --git a/pkg/internal/admission/doc.go b/pkg/internal/admission/doc.go deleted file mode 100644 index 841ed3a332..0000000000 --- a/pkg/internal/admission/doc.go +++ /dev/null @@ -1,18 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission provides libraries for creating admission webhooks. -package admission diff --git a/pkg/internal/admission/example_admissionfunc_test.go b/pkg/internal/admission/example_admissionfunc_test.go deleted file mode 100644 index 4d83ec6325..0000000000 --- a/pkg/internal/admission/example_admissionfunc_test.go +++ /dev/null @@ -1,42 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission_test - -import ( - "fmt" - - "k8s.io/api/admission/v1beta1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/internal/admission" -) - -func ExampleFunc() { - var _ admission.Func = func(review v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { - pod := corev1.Pod{} - resourceType := metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} - if errResp := admission.Decode(review, &pod, resourceType); errResp != nil { - return errResp - } - // Business logic for admission decision - if len(pod.Spec.Containers) != 1 { - return admission.DenyResponse(fmt.Sprintf( - "pod %s/%s may only have 1 container.", pod.Namespace, pod.Name)) - } - return admission.AllowResponse() - } -} diff --git a/pkg/internal/admission/example_decode_test.go b/pkg/internal/admission/example_decode_test.go deleted file mode 100644 index d3ce25d082..0000000000 --- a/pkg/internal/admission/example_decode_test.go +++ /dev/null @@ -1,47 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission_test - -import ( - "fmt" - - "k8s.io/api/admission/v1beta1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/internal/admission" -) - -func ExampleDecode() { - var review v1beta1.AdmissionReview - resourceType := metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} - pod := corev1.Pod{} - if errResp := admission.Decode(review, &pod, resourceType); errResp != nil { - // Send error resp - } -} - -func ExampleErrorResponse() { - admission.ErrorResponse(fmt.Errorf("some error explanation")) -} - -func ExampleDenyResponse() { - admission.DenyResponse(fmt.Sprintf("some deny explanation")) -} - -func ExampleAllowResponse() { - admission.AllowResponse() -} diff --git a/pkg/internal/admission/example_handlefunc_test.go b/pkg/internal/admission/example_handlefunc_test.go deleted file mode 100644 index 3333bc37c6..0000000000 --- a/pkg/internal/admission/example_handlefunc_test.go +++ /dev/null @@ -1,59 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission_test - -import ( - "fmt" - - "k8s.io/api/admission/v1beta1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/internal/admission" -) - -func ExampleHandleFunc() { - resourceType := metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} - admission.HandleFunc("/pod", resourceType, func(review v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { - pod := corev1.Pod{} - if errResp := admission.Decode(review, &pod, resourceType); errResp != nil { - return errResp - } - // Business logic for admission decision - if len(pod.Spec.Containers) != 1 { - return admission.DenyResponse(fmt.Sprintf( - "pod %s/%s may only have 1 container.", pod.Namespace, pod.Name)) - } - return admission.AllowResponse() - }) -} - -func ExampleManager_HandleFunc() { - resourceType := metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} - ah := admission.Manager{} - ah.HandleFunc("/pod", resourceType, func(review v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { - pod := corev1.Pod{} - if errResp := admission.Decode(review, &pod, resourceType); errResp != nil { - return errResp - } - // Business logic for admission decision - if len(pod.Spec.Containers) != 1 { - return admission.DenyResponse(fmt.Sprintf( - "pod %s/%s may only have 1 container.", pod.Namespace, pod.Name)) - } - return admission.AllowResponse() - }) -} diff --git a/pkg/internal/admission/example_test.go b/pkg/internal/admission/example_test.go deleted file mode 100644 index 4fe959bdc3..0000000000 --- a/pkg/internal/admission/example_test.go +++ /dev/null @@ -1,43 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission_test - -import ( - "fmt" - - "k8s.io/api/admission/v1beta1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/internal/admission" -) - -func Example() { - resourceType := metav1.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} - admission.HandleFunc("/pod", resourceType, func(review v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { - pod := corev1.Pod{} - if errResp := admission.Decode(review, &pod, resourceType); errResp != nil { - return errResp - } - // Business logic for admission decision - if len(pod.Spec.Containers) != 1 { - return admission.DenyResponse(fmt.Sprintf( - "pod %s/%s may only have 1 container.", pod.Namespace, pod.Name)) - } - return admission.AllowResponse() - }) - admission.ListenAndServeTLS("") -} diff --git a/pkg/internal/admission/handler.go b/pkg/internal/admission/handler.go deleted file mode 100644 index 933ff43e34..0000000000 --- a/pkg/internal/admission/handler.go +++ /dev/null @@ -1,72 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission - -import ( - "net/http" - - "k8s.io/api/admission/v1beta1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -// Func implements an AdmissionReview operation for a GroupVersionResource -type Func func(review v1beta1.AdmissionReview) *v1beta1.AdmissionResponse - -// HandleEntry -type admissionHandler struct { - GVR metav1.GroupVersionResource - Fn Func -} - -// handle handles an admission request and returns a result -func (ah admissionHandler) handle(review v1beta1.AdmissionReview) *v1beta1.AdmissionResponse { - return ah.handle(review) -} - -// Manager manages admission controllers -type Manager struct { - Entries map[string]admissionHandler - SMux *http.ServeMux -} - -// DefaultAdmissionFns is the default admission control functions registry -var DefaultAdmissionFns = &Manager{ - SMux: http.DefaultServeMux, -} - -// HandleFunc registers fn as an admission control webhook callback for the group,version,resources specified -func (e *Manager) HandleFunc(path string, gvr metav1.GroupVersionResource, fn Func) { - // Register the entry so a Webhook config is created - e.Entries[path] = admissionHandler{gvr, fn} - - // Register the handler path - e.SMux.Handle(path, httpHandler{fn}) -} - -// HandleFunc registers fn as an admission control webhook callback for the group,version,resources specified -func HandleFunc(path string, gvr metav1.GroupVersionResource, fn Func) { - DefaultAdmissionFns.HandleFunc(path, gvr, fn) -} - -// ListenAndServeTLS starts the admission HttpServer. -func ListenAndServeTLS(addr string) error { - server := &http.Server{ - Addr: addr, - TLSConfig: nil, // TODO: Set this - } - return server.ListenAndServeTLS("", "") -} diff --git a/pkg/internal/admission/http.go b/pkg/internal/admission/http.go deleted file mode 100644 index 49ddb4ad16..0000000000 --- a/pkg/internal/admission/http.go +++ /dev/null @@ -1,80 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission - -import ( - "encoding/json" - "io/ioutil" - "net/http" - - "k8s.io/api/admission/v1beta1" - "k8s.io/apimachinery/pkg/runtime" - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" -) - -var ( - // TODO(directxman12): this shouldn't be a global log - log = logf.RuntimeLog.WithName("admission").WithName("http-handler") -) - -func (h httpHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - var body []byte - if r.Body != nil { - if data, err := ioutil.ReadAll(r.Body); err == nil { - body = data - } - } - - // verify the content type is accurate - contentType := r.Header.Get("Content-Type") - if contentType != "application/json" { - log.Error(nil, "invalid content type, expected application/json", "content type", contentType) - return - } - - var reviewResponse *v1beta1.AdmissionResponse - ar := v1beta1.AdmissionReview{} - deserializer := codecs.UniversalDeserializer() - if _, _, err := deserializer.Decode(body, nil, &ar); err != nil { - log.Error(err, "unable to decode request body") - reviewResponse = ErrorResponse(err) - } else { - reviewResponse = h.admit(ar) - } - - response := v1beta1.AdmissionReview{} - if reviewResponse != nil { - response.Response = reviewResponse - response.Response.UID = ar.Request.UID - } - // reset the Object and OldObject, they are not needed in a response. - ar.Request.Object = runtime.RawExtension{} - ar.Request.OldObject = runtime.RawExtension{} - - resp, err := json.Marshal(response) - if err != nil { - log.Error(err, "unable to marshal response") - return - } - if _, err := w.Write(resp); err != nil { - log.Error(err, "unable to write response") - } -} - -type httpHandler struct { - admit Func -} diff --git a/pkg/internal/admission/response.go b/pkg/internal/admission/response.go deleted file mode 100644 index 24dc32d9b3..0000000000 --- a/pkg/internal/admission/response.go +++ /dev/null @@ -1,48 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission - -import ( - "k8s.io/api/admission/v1beta1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -// ErrorResponse creates a new AdmissionResponse for an error handling the request -func ErrorResponse(err error) *v1beta1.AdmissionResponse { - return &v1beta1.AdmissionResponse{ - Result: &metav1.Status{ - Message: err.Error(), - }, - } -} - -// DenyResponse returns a new response for denying a request -func DenyResponse(msg string) *v1beta1.AdmissionResponse { - return &v1beta1.AdmissionResponse{ - Allowed: false, - Result: &metav1.Status{ - Reason: metav1.StatusReason(msg), - }, - } -} - -// AllowResponse returns a new response for admitting a request -func AllowResponse() *v1beta1.AdmissionResponse { - return &v1beta1.AdmissionResponse{ - Allowed: true, - } -} diff --git a/pkg/internal/admission/tls.go b/pkg/internal/admission/tls.go deleted file mode 100644 index ed22d249b2..0000000000 --- a/pkg/internal/admission/tls.go +++ /dev/null @@ -1,42 +0,0 @@ -/* -Copyright 2017 The Kubernetes 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 admission - -//type certs struct { -// Cert []byte -// Key []byte -// CACert []byte -//} - -//// MakeTLSConfig makes a TLS configuration suitable for use with the server -//func makeTLSConfig(certs certs) (*tls.Config, error) { -// caCertPool := x509.NewCertPool() -// caCertPool.AppendCertsFromPEM(certs.CACert) -// //cert, err := tls.X509KeyPair(certs.Cert, certs.Key) -// //if err != nil { -// // return nil, err -// //} -// return &tls.Config{ -// //Certificates: []tls.Certificate{cert}, -// ClientCAs: caCertPool, -// ClientAuth: tls.NoClientCert, -// // Note on GKE there apparently is no client cert sent, so this -// // does not work on GKE. -// // TODO: make this into a configuration option. -// // ClientAuth: tls.RequireAndVerifyClientCert, -// }, nil -//} From 1a4bbe1685b2707289dd494e82f5f6774d6433fb Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 18:06:44 -0800 Subject: [PATCH 16/19] Rename ErrorResponse to Errored It's the same name used in alias.go, and it produces less-verbose code without loss of readability. --- example/mutatingwebhook.go | 4 ++-- example/validatingwebhook.go | 2 +- pkg/webhook/admission/doc.go | 8 ++++---- pkg/webhook/admission/http.go | 10 +++++----- pkg/webhook/admission/multi.go | 4 ++-- pkg/webhook/admission/response.go | 6 +++--- pkg/webhook/admission/response_test.go | 4 ++-- pkg/webhook/admission/webhook.go | 2 +- pkg/webhook/alias.go | 2 +- 9 files changed, 21 insertions(+), 21 deletions(-) diff --git a/example/mutatingwebhook.go b/example/mutatingwebhook.go index 24d2d4dd6c..ec361bf811 100644 --- a/example/mutatingwebhook.go +++ b/example/mutatingwebhook.go @@ -38,7 +38,7 @@ func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admiss err := a.decoder.Decode(req, pod) if err != nil { - return admission.ErrorResponse(http.StatusBadRequest, err) + return admission.Errored(http.StatusBadRequest, err) } if pod.Annotations == nil { @@ -48,7 +48,7 @@ func (a *podAnnotator) Handle(ctx context.Context, req admission.Request) admiss marshaledPod, err := json.Marshal(pod) if err != nil { - return admission.ErrorResponse(http.StatusInternalServerError, err) + return admission.Errored(http.StatusInternalServerError, err) } return admission.PatchResponseFromRaw(req.AdmissionRequest.Object.Raw, marshaledPod) diff --git a/example/validatingwebhook.go b/example/validatingwebhook.go index 09983b33f2..6c02616b28 100644 --- a/example/validatingwebhook.go +++ b/example/validatingwebhook.go @@ -38,7 +38,7 @@ func (v *podValidator) Handle(ctx context.Context, req admission.Request) admiss err := v.decoder.Decode(req, pod) if err != nil { - return admission.ErrorResponse(http.StatusBadRequest, err) + return admission.Errored(http.StatusBadRequest, err) } key := "example-mutating-admission-webhook" diff --git a/pkg/webhook/admission/doc.go b/pkg/webhook/admission/doc.go index 0126aad03a..f33bd11163 100644 --- a/pkg/webhook/admission/doc.go +++ b/pkg/webhook/admission/doc.go @@ -32,13 +32,13 @@ The following snippet is an example implementation of mutating handler. pod := &corev1.Pod{} err := m.decoder.Decode(req, pod) if err != nil { - return admission.ErrorResponse(http.StatusBadRequest, err) + return admission.Errored(http.StatusBadRequest, err) } // Do deepcopy before actually mutate the object. copy := pod.DeepCopy() err = m.mutatePodsFn(ctx, copy) if err != nil { - return admission.ErrorResponse(http.StatusInternalServerError, err) + return admission.Errored(http.StatusInternalServerError, err) } return admission.PatchResponse(pod, copy) } @@ -70,12 +70,12 @@ The following snippet is an example implementation of validating handler. pod := &corev1.Pod{} err := h.decoder.Decode(req, pod) if err != nil { - return admission.ErrorResponse(http.StatusBadRequest, err) + return admission.Errored(http.StatusBadRequest, err) } allowed, reason, err := h.validatePodsFn(ctx, pod) if err != nil { - return admission.ErrorResponse(http.StatusInternalServerError, err) + return admission.Errored(http.StatusInternalServerError, err) } return admission.ValidationResponse(allowed, reason) } diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index d678e24778..2943e445dd 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -49,14 +49,14 @@ func (wh Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if r.Body != nil { if body, err = ioutil.ReadAll(r.Body); err != nil { log.Error(err, "unable to read the body from the incoming request") - reviewResponse = ErrorResponse(http.StatusBadRequest, err) + reviewResponse = Errored(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) return } } else { err = errors.New("request body is empty") log.Error(err, "bad request") - reviewResponse = ErrorResponse(http.StatusBadRequest, err) + reviewResponse = Errored(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) return } @@ -66,7 +66,7 @@ func (wh Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if contentType != "application/json" { err = fmt.Errorf("contentType=%s, expected application/json", contentType) log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) - reviewResponse = ErrorResponse(http.StatusBadRequest, err) + reviewResponse = Errored(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) return } @@ -78,7 +78,7 @@ func (wh Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { } if _, _, err := admissionCodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil { log.Error(err, "unable to decode the request") - reviewResponse = ErrorResponse(http.StatusBadRequest, err) + reviewResponse = Errored(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) return } @@ -96,6 +96,6 @@ func (wh *Webhook) writeResponse(w io.Writer, response Response) { err := encoder.Encode(responseAdmissionReview) if err != nil { log.Error(err, "unable to encode the response") - wh.writeResponse(w, ErrorResponse(http.StatusInternalServerError, err)) + wh.writeResponse(w, Errored(http.StatusInternalServerError, err)) } } diff --git a/pkg/webhook/admission/multi.go b/pkg/webhook/admission/multi.go index 43d98dd4c9..a65be69f68 100644 --- a/pkg/webhook/admission/multi.go +++ b/pkg/webhook/admission/multi.go @@ -38,7 +38,7 @@ func (hs multiMutating) Handle(ctx context.Context, req Request) Response { return resp } if resp.PatchType != nil && *resp.PatchType != admissionv1beta1.PatchTypeJSONPatch { - return ErrorResponse(http.StatusInternalServerError, + return Errored(http.StatusInternalServerError, fmt.Errorf("unexpected patch type returned by the handler: %v, only allow: %v", resp.PatchType, admissionv1beta1.PatchTypeJSONPatch)) } @@ -47,7 +47,7 @@ func (hs multiMutating) Handle(ctx context.Context, req Request) Response { var err error marshaledPatch, err := json.Marshal(patches) if err != nil { - return ErrorResponse(http.StatusBadRequest, fmt.Errorf("error when marshaling the patch: %v", err)) + return Errored(http.StatusBadRequest, fmt.Errorf("error when marshaling the patch: %v", err)) } return Response{ AdmissionResponse: admissionv1beta1.AdmissionResponse{ diff --git a/pkg/webhook/admission/response.go b/pkg/webhook/admission/response.go index 0701abc70d..7f9b544712 100644 --- a/pkg/webhook/admission/response.go +++ b/pkg/webhook/admission/response.go @@ -47,8 +47,8 @@ func Patched(reason string, patches ...jsonpatch.JsonPatchOperation) Response { return resp } -// ErrorResponse creates a new Response for error-handling a request. -func ErrorResponse(code int32, err error) Response { +// Errored creates a new Response for error-handling a request. +func Errored(code int32, err error) Response { return Response{ AdmissionResponse: admissionv1beta1.AdmissionResponse{ Allowed: false, @@ -81,7 +81,7 @@ func ValidationResponse(allowed bool, reason string) Response { func PatchResponseFromRaw(original, current []byte) Response { patches, err := jsonpatch.CreatePatch(original, current) if err != nil { - return ErrorResponse(http.StatusInternalServerError, err) + return Errored(http.StatusInternalServerError, err) } return Response{ Patches: patches, diff --git a/pkg/webhook/admission/response_test.go b/pkg/webhook/admission/response_test.go index f9fe987567..8e2dfde48e 100644 --- a/pkg/webhook/admission/response_test.go +++ b/pkg/webhook/admission/response_test.go @@ -116,7 +116,7 @@ var _ = Describe("Admission Webhook Response Helpers", func() { }) }) - Describe("ErrorResponse", func() { + Describe("Errored", func() { It("should return a denied response with an error", func() { err := errors.New("this is an error") expected := Response{ @@ -128,7 +128,7 @@ var _ = Describe("Admission Webhook Response Helpers", func() { }, }, } - resp := ErrorResponse(http.StatusBadRequest, err) + resp := Errored(http.StatusBadRequest, err) Expect(resp).To(Equal(expected)) }) }) diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 5a2907c086..ff8a41fefc 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -122,7 +122,7 @@ func (w *Webhook) Handle(ctx context.Context, req Request) Response { resp := w.Handler.Handle(ctx, req) if err := resp.Complete(req); err != nil { log.Error(err, "unable to encode response") - return ErrorResponse(http.StatusInternalServerError, errUnableToEncodeResponse) + return Errored(http.StatusInternalServerError, errUnableToEncodeResponse) } return resp diff --git a/pkg/webhook/alias.go b/pkg/webhook/alias.go index 6639e6bc73..d721878b7f 100644 --- a/pkg/webhook/alias.go +++ b/pkg/webhook/alias.go @@ -63,5 +63,5 @@ var ( Patched = admission.Patched // Errored indicates that an error occurred in the admission request. - Errored = admission.ErrorResponse + Errored = admission.Errored ) From a866ee9125fa6de0c12edf7183a0982f382b9e67 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 18:12:22 -0800 Subject: [PATCH 17/19] Remove inline admission examples The were incorrect (would not have compiled), are mostly duplicates of examples/*webhook.go, and can't be easily checked for being up-to-date. --- pkg/webhook/admission/doc.go | 75 +----------------------------------- 1 file changed, 1 insertion(+), 74 deletions(-) diff --git a/pkg/webhook/admission/doc.go b/pkg/webhook/admission/doc.go index f33bd11163..0b274dd02b 100644 --- a/pkg/webhook/admission/doc.go +++ b/pkg/webhook/admission/doc.go @@ -17,80 +17,7 @@ limitations under the License. /* Package admission provides implementation for admission webhook and methods to implement admission webhook handlers. -The following snippet is an example implementation of mutating handler. - - type Mutator struct { - client client.Client - decoder types.Decoder - } - - func (m *Mutator) mutatePodsFn(ctx context.Context, pod *corev1.Pod) error { - // your logic to mutate the passed-in pod. - } - - func (m *Mutator) Handle(ctx context.Context, req types.Request) types.Response { - pod := &corev1.Pod{} - err := m.decoder.Decode(req, pod) - if err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - // Do deepcopy before actually mutate the object. - copy := pod.DeepCopy() - err = m.mutatePodsFn(ctx, copy) - if err != nil { - return admission.Errored(http.StatusInternalServerError, err) - } - return admission.PatchResponse(pod, copy) - } - - // InjectClient is called by the Manager and provides a client.Client to the Mutator instance. - func (m *Mutator) InjectClient(c client.Client) error { - h.client = c - return nil - } - - // InjectDecoder is called by the Manager and provides a types.Decoder to the Mutator instance. - func (m *Mutator) InjectDecoder(d types.Decoder) error { - h.decoder = d - return nil - } - -The following snippet is an example implementation of validating handler. - - type Handler struct { - client client.Client - decoder types.Decoder - } - - func (v *Validator) validatePodsFn(ctx context.Context, pod *corev1.Pod) (bool, string, error) { - // your business logic - } - - func (v *Validator) Handle(ctx context.Context, req types.Request) types.Response { - pod := &corev1.Pod{} - err := h.decoder.Decode(req, pod) - if err != nil { - return admission.Errored(http.StatusBadRequest, err) - } - - allowed, reason, err := h.validatePodsFn(ctx, pod) - if err != nil { - return admission.Errored(http.StatusInternalServerError, err) - } - return admission.ValidationResponse(allowed, reason) - } - - // InjectClient is called by the Manager and provides a client.Client to the Validator instance. - func (v *Validator) InjectClient(c client.Client) error { - h.client = c - return nil - } - - // InjectDecoder is called by the Manager and provides a types.Decoder to the Validator instance. - func (v *Validator) InjectDecoder(d types.Decoder) error { - h.decoder = d - return nil - } +See examples/mutatingwebhook.go and examples/validatingwebhook.go for examples of admission webhooks. */ package admission From 34a415363d71550eac72abc0cac180e2dc089de3 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 18:34:19 -0800 Subject: [PATCH 18/19] Add per-webhook logging This gives each webhook a log, so that serving errors can be tied to a particuluar webhook. --- example/main.go | 4 ++-- pkg/runtime/inject/inject.go | 17 +++++++++++++++++ pkg/webhook/admission/http.go | 12 ++++++------ pkg/webhook/admission/http_test.go | 10 +++++++--- pkg/webhook/admission/webhook.go | 13 +++++++++++-- pkg/webhook/server.go | 10 +++++++++- 6 files changed, 52 insertions(+), 14 deletions(-) diff --git a/example/main.go b/example/main.go index 499de9daad..aa33c94831 100644 --- a/example/main.go +++ b/example/main.go @@ -88,8 +88,8 @@ func main() { } entryLog.Info("registering webhooks to the webhook server") - hookServer.Register("/mutate-pods", webhook.Admission{Handler: &podAnnotator{}}) - hookServer.Register("/validate-pods", webhook.Admission{Handler: &podValidator{}}) + hookServer.Register("/mutate-pods", &webhook.Admission{Handler: &podAnnotator{}}) + hookServer.Register("/validate-pods", &webhook.Admission{Handler: &podValidator{}}) entryLog.Info("starting manager") if err := mgr.Start(signals.SetupSignalHandler()); err != nil { diff --git a/pkg/runtime/inject/inject.go b/pkg/runtime/inject/inject.go index ec7f30c68d..c006fdb53d 100644 --- a/pkg/runtime/inject/inject.go +++ b/pkg/runtime/inject/inject.go @@ -17,9 +17,11 @@ limitations under the License. package inject import ( + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -129,3 +131,18 @@ func InjectorInto(f Func, i interface{}) (bool, error) { } return false, nil } + +// Logger is used to inject Loggers into components that need them +// and don't otherwise have opinions. +type Logger interface { + InjectLogger(l logr.Logger) error +} + +// LoggerInto will set the logger on the given object if it implements inject.Logger, +// returning true if a InjectLogger was called, and false otherwise. +func LoggerInto(l logr.Logger, i interface{}) (bool, error) { + if injectable, wantsLogger := i.(Logger); wantsLogger { + return true, injectable.InjectLogger(l) + } + return false, nil +} diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 2943e445dd..fa60302adc 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -41,21 +41,21 @@ func init() { var _ http.Handler = &Webhook{} -func (wh Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { +func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { var body []byte var err error var reviewResponse Response if r.Body != nil { if body, err = ioutil.ReadAll(r.Body); err != nil { - log.Error(err, "unable to read the body from the incoming request") + wh.log.Error(err, "unable to read the body from the incoming request") reviewResponse = Errored(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) return } } else { err = errors.New("request body is empty") - log.Error(err, "bad request") + wh.log.Error(err, "bad request") reviewResponse = Errored(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) return @@ -65,7 +65,7 @@ func (wh Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { contentType := r.Header.Get("Content-Type") if contentType != "application/json" { err = fmt.Errorf("contentType=%s, expected application/json", contentType) - log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) + wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) reviewResponse = Errored(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) return @@ -77,7 +77,7 @@ func (wh Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { Request: &req.AdmissionRequest, } if _, _, err := admissionCodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil { - log.Error(err, "unable to decode the request") + wh.log.Error(err, "unable to decode the request") reviewResponse = Errored(http.StatusBadRequest, err) wh.writeResponse(w, reviewResponse) return @@ -95,7 +95,7 @@ func (wh *Webhook) writeResponse(w io.Writer, response Response) { } err := encoder.Encode(responseAdmissionReview) if err != nil { - log.Error(err, "unable to encode the response") + wh.log.Error(err, "unable to encode the response") wh.writeResponse(w, Errored(http.StatusInternalServerError, err)) } } diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index 01e97756cf..975115695f 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -26,6 +26,8 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" + admissionv1beta1 "k8s.io/api/admission/v1beta1" ) @@ -33,14 +35,16 @@ var _ = Describe("Admission Webhooks", func() { Describe("HTTP Handler", func() { var respRecorder *httptest.ResponseRecorder + webhook := &Webhook{ + Handler: nil, + } BeforeEach(func() { respRecorder = &httptest.ResponseRecorder{ Body: bytes.NewBuffer(nil), } + _, err := inject.LoggerInto(log.WithName("test-webhook"), webhook) + Expect(err).NotTo(HaveOccurred()) }) - webhook := &Webhook{ - Handler: nil, - } It("should return bad-request when given an empty body", func() { req := &http.Request{Body: nil} diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index ff8a41fefc..2f6cd9a88f 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -22,6 +22,7 @@ import ( "net/http" "github.com/appscode/jsonpatch" + "github.com/go-logr/logr" admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -110,8 +111,16 @@ type Webhook struct { // and potentially patches to apply to the handler. Handler Handler - // scheme is used to construct the Decoder + // decoder is constructed on receiving a scheme and passed down to then handler decoder *Decoder + + log logr.Logger +} + +// InjectLogger gets a handle to a logging instance, hopefully with more info about this particular webhook. +func (w *Webhook) InjectLogger(l logr.Logger) error { + w.log = l + return nil } // Handle processes AdmissionRequest. @@ -121,7 +130,7 @@ type Webhook struct { func (w *Webhook) Handle(ctx context.Context, req Request) Response { resp := w.Handler.Handle(ctx, req) if err := resp.Complete(req); err != nil { - log.Error(err, "unable to encode response") + w.log.Error(err, "unable to encode response") return Errored(http.StatusInternalServerError, errUnableToEncodeResponse) } diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index e276dc40a2..25b2eb43e1 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -111,12 +111,20 @@ func instrumentedHook(path string, hookRaw http.Handler) http.Handler { func (s *Server) Start(stop <-chan struct{}) error { s.defaultingOnce.Do(s.setDefaults) + baseHookLog := log.WithName("webhooks") // inject fields here as opposed to in Register so that we're certain to have our setFields // function available. - for _, webhook := range s.webhooks { + for hookPath, webhook := range s.webhooks { if err := s.setFields(webhook); err != nil { return err } + + // NB(directxman12): we don't propagate this further by wrapping setFields because it's + // unclear if this is how we want to deal with log propagation. In this specific instance, + // we want to be able to pass a logger to webhooks because they don't know their own path. + if _, err := inject.LoggerInto(baseHookLog.WithValues("webhook", hookPath), webhook); err != nil { + return err + } } // TODO: watch the cert dir. Reload the cert if it changes From 286b0b40babfe4f91950251ab992174f23ab783d Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Tue, 19 Feb 2019 20:27:43 -0800 Subject: [PATCH 19/19] Ensure the right deps are in Gopkg.log Nothing was missing from the tree (due to transitive deps), just missing from being required by Gopkg.lock. --- Gopkg.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Gopkg.lock b/Gopkg.lock index 897580722e..967d96f23c 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -962,6 +962,7 @@ "k8s.io/apimachinery/pkg/runtime/serializer", "k8s.io/apimachinery/pkg/selection", "k8s.io/apimachinery/pkg/types", + "k8s.io/apimachinery/pkg/util/json", "k8s.io/apimachinery/pkg/util/runtime", "k8s.io/apimachinery/pkg/util/sets", "k8s.io/apimachinery/pkg/util/uuid",