From b2b1ef14ec0b783e3308fb49dc1f375138a5be63 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 29 May 2019 21:30:45 -0700 Subject: [PATCH 1/5] split admissionregistration.v1beta1/Webhook into MutatingWebhook and ValidatingWebhook Kubernetes-commit: 55ecc45455f191c404e355097bf1beae9c42f895 --- .../configuration/mutating_webhook_manager.go | 17 +- .../mutating_webhook_manager_test.go | 13 +- .../validating_webhook_manager.go | 19 +-- .../validating_webhook_manager_test.go | 13 +- pkg/admission/plugin/webhook/accessors.go | 139 ++++++++++++++++ .../plugin/webhook/generic/interfaces.go | 7 +- .../plugin/webhook/generic/webhook.go | 27 ++-- .../plugin/webhook/generic/webhook_test.go | 31 ++-- .../plugin/webhook/mutating/dispatcher.go | 35 ++-- .../plugin/webhook/mutating/plugin_test.go | 6 +- .../plugin/webhook/namespace/matcher.go | 6 +- .../plugin/webhook/namespace/matcher_test.go | 5 +- .../plugin/webhook/testing/testcase.go | 153 ++++++++++++------ .../plugin/webhook/util/client_config.go | 39 ++--- .../plugin/webhook/validating/dispatcher.go | 37 ++--- .../plugin/webhook/validating/plugin_test.go | 4 +- 16 files changed, 382 insertions(+), 169 deletions(-) create mode 100644 pkg/admission/plugin/webhook/accessors.go diff --git a/pkg/admission/configuration/mutating_webhook_manager.go b/pkg/admission/configuration/mutating_webhook_manager.go index 4b2256e11..bacf61722 100644 --- a/pkg/admission/configuration/mutating_webhook_manager.go +++ b/pkg/admission/configuration/mutating_webhook_manager.go @@ -24,6 +24,7 @@ import ( "k8s.io/api/admissionregistration/v1beta1" "k8s.io/apimachinery/pkg/labels" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" "k8s.io/client-go/informers" admissionregistrationlisters "k8s.io/client-go/listers/admissionregistration/v1beta1" @@ -48,7 +49,7 @@ func NewMutatingWebhookConfigurationManager(f informers.SharedInformerFactory) g } // Start with an empty list - manager.configuration.Store(&v1beta1.MutatingWebhookConfiguration{}) + manager.configuration.Store([]webhook.WebhookAccessor{}) // On any change, rebuild the config informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ -61,8 +62,8 @@ func NewMutatingWebhookConfigurationManager(f informers.SharedInformerFactory) g } // Webhooks returns the merged MutatingWebhookConfiguration. -func (m *mutatingWebhookConfigurationManager) Webhooks() []v1beta1.Webhook { - return m.configuration.Load().(*v1beta1.MutatingWebhookConfiguration).Webhooks +func (m *mutatingWebhookConfigurationManager) Webhooks() []webhook.WebhookAccessor { + return m.configuration.Load().([]webhook.WebhookAccessor) } func (m *mutatingWebhookConfigurationManager) HasSynced() bool { @@ -78,16 +79,18 @@ func (m *mutatingWebhookConfigurationManager) updateConfiguration() { m.configuration.Store(mergeMutatingWebhookConfigurations(configurations)) } -func mergeMutatingWebhookConfigurations(configurations []*v1beta1.MutatingWebhookConfiguration) *v1beta1.MutatingWebhookConfiguration { - var ret v1beta1.MutatingWebhookConfiguration +func mergeMutatingWebhookConfigurations(configurations []*v1beta1.MutatingWebhookConfiguration) []webhook.WebhookAccessor { // The internal order of webhooks for each configuration is provided by the user // but configurations themselves can be in any order. As we are going to run these // webhooks in serial, they are sorted here to have a deterministic order. sort.SliceStable(configurations, MutatingWebhookConfigurationSorter(configurations).ByName) + accessors := []webhook.WebhookAccessor{} for _, c := range configurations { - ret.Webhooks = append(ret.Webhooks, c.Webhooks...) + for i := range c.Webhooks { + accessors = append(accessors, webhook.NewMutatingWebhookAccessor(&c.Webhooks[i])) + } } - return &ret + return accessors } type MutatingWebhookConfigurationSorter []*v1beta1.MutatingWebhookConfiguration diff --git a/pkg/admission/configuration/mutating_webhook_manager_test.go b/pkg/admission/configuration/mutating_webhook_manager_test.go index 9bc037f5a..828c18430 100644 --- a/pkg/admission/configuration/mutating_webhook_manager_test.go +++ b/pkg/admission/configuration/mutating_webhook_manager_test.go @@ -45,7 +45,7 @@ func TestGetMutatingWebhookConfig(t *testing.T) { webhookConfiguration := &v1beta1.MutatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{Name: "webhook1"}, - Webhooks: []v1beta1.Webhook{{Name: "webhook1.1"}}, + Webhooks: []v1beta1.MutatingWebhook{{Name: "webhook1.1"}}, } mutatingInformer := informerFactory.Admissionregistration().V1beta1().MutatingWebhookConfigurations() @@ -57,7 +57,14 @@ func TestGetMutatingWebhookConfig(t *testing.T) { if len(configurations) == 0 { t.Errorf("expected non empty webhooks") } - if !reflect.DeepEqual(configurations, webhookConfiguration.Webhooks) { - t.Errorf("Expected\n%#v\ngot\n%#v", webhookConfiguration.Webhooks, configurations) + for i := range configurations { + h, ok := configurations[i].GetMutatingWebhook() + if !ok { + t.Errorf("Expected mutating webhook") + continue + } + if !reflect.DeepEqual(h, &webhookConfiguration.Webhooks[i]) { + t.Errorf("Expected\n%#v\ngot\n%#v", &webhookConfiguration.Webhooks[i], h) + } } } diff --git a/pkg/admission/configuration/validating_webhook_manager.go b/pkg/admission/configuration/validating_webhook_manager.go index 9258258f6..bcce1e70f 100644 --- a/pkg/admission/configuration/validating_webhook_manager.go +++ b/pkg/admission/configuration/validating_webhook_manager.go @@ -24,6 +24,7 @@ import ( "k8s.io/api/admissionregistration/v1beta1" "k8s.io/apimachinery/pkg/labels" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" "k8s.io/client-go/informers" admissionregistrationlisters "k8s.io/client-go/listers/admissionregistration/v1beta1" @@ -48,7 +49,7 @@ func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) } // Start with an empty list - manager.configuration.Store(&v1beta1.ValidatingWebhookConfiguration{}) + manager.configuration.Store([]webhook.WebhookAccessor{}) // On any change, rebuild the config informer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ -61,8 +62,8 @@ func NewValidatingWebhookConfigurationManager(f informers.SharedInformerFactory) } // Webhooks returns the merged ValidatingWebhookConfiguration. -func (v *validatingWebhookConfigurationManager) Webhooks() []v1beta1.Webhook { - return v.configuration.Load().(*v1beta1.ValidatingWebhookConfiguration).Webhooks +func (v *validatingWebhookConfigurationManager) Webhooks() []webhook.WebhookAccessor { + return v.configuration.Load().([]webhook.WebhookAccessor) } // HasSynced returns true if the shared informers have synced. @@ -79,15 +80,15 @@ func (v *validatingWebhookConfigurationManager) updateConfiguration() { v.configuration.Store(mergeValidatingWebhookConfigurations(configurations)) } -func mergeValidatingWebhookConfigurations( - configurations []*v1beta1.ValidatingWebhookConfiguration, -) *v1beta1.ValidatingWebhookConfiguration { +func mergeValidatingWebhookConfigurations(configurations []*v1beta1.ValidatingWebhookConfiguration) []webhook.WebhookAccessor { sort.SliceStable(configurations, ValidatingWebhookConfigurationSorter(configurations).ByName) - var ret v1beta1.ValidatingWebhookConfiguration + accessors := []webhook.WebhookAccessor{} for _, c := range configurations { - ret.Webhooks = append(ret.Webhooks, c.Webhooks...) + for i := range c.Webhooks { + accessors = append(accessors, webhook.NewValidatingWebhookAccessor(&c.Webhooks[i])) + } } - return &ret + return accessors } type ValidatingWebhookConfigurationSorter []*v1beta1.ValidatingWebhookConfiguration diff --git a/pkg/admission/configuration/validating_webhook_manager_test.go b/pkg/admission/configuration/validating_webhook_manager_test.go index 153b4df48..d99d99b46 100644 --- a/pkg/admission/configuration/validating_webhook_manager_test.go +++ b/pkg/admission/configuration/validating_webhook_manager_test.go @@ -46,7 +46,7 @@ func TestGetValidatingWebhookConfig(t *testing.T) { webhookConfiguration := &v1beta1.ValidatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{Name: "webhook1"}, - Webhooks: []v1beta1.Webhook{{Name: "webhook1.1"}}, + Webhooks: []v1beta1.ValidatingWebhook{{Name: "webhook1.1"}}, } validatingInformer := informerFactory.Admissionregistration().V1beta1().ValidatingWebhookConfigurations() @@ -59,7 +59,14 @@ func TestGetValidatingWebhookConfig(t *testing.T) { if len(configurations) == 0 { t.Errorf("expected non empty webhooks") } - if !reflect.DeepEqual(configurations, webhookConfiguration.Webhooks) { - t.Errorf("Expected\n%#v\ngot\n%#v", webhookConfiguration.Webhooks, configurations) + for i := range configurations { + h, ok := configurations[i].GetValidatingWebhook() + if !ok { + t.Errorf("Expected validating webhook") + continue + } + if !reflect.DeepEqual(h, &webhookConfiguration.Webhooks[i]) { + t.Errorf("Expected\n%#v\ngot\n%#v", &webhookConfiguration.Webhooks[i], h) + } } } diff --git a/pkg/admission/plugin/webhook/accessors.go b/pkg/admission/plugin/webhook/accessors.go new file mode 100644 index 000000000..362333c50 --- /dev/null +++ b/pkg/admission/plugin/webhook/accessors.go @@ -0,0 +1,139 @@ +/* +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 ( + "k8s.io/api/admissionregistration/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// WebhookAccessor provides a common interface to both mutating and validating webhook types. +type WebhookAccessor interface { + // GetName gets the webhook Name field. + GetName() string + // GetClientConfig gets the webhook ClientConfig field. + GetClientConfig() v1beta1.WebhookClientConfig + // GetRules gets the webhook Rules field. + GetRules() []v1beta1.RuleWithOperations + // GetFailurePolicy gets the webhook FailurePolicy field. + GetFailurePolicy() *v1beta1.FailurePolicyType + // GetMatchPolicy gets the webhook MatchPolicy field. + GetMatchPolicy() *v1beta1.MatchPolicyType + // GetNamespaceSelector gets the webhook NamespaceSelector field. + GetNamespaceSelector() *metav1.LabelSelector + // GetSideEffects gets the webhook SideEffects field. + GetSideEffects() *v1beta1.SideEffectClass + // GetTimeoutSeconds gets the webhook TimeoutSeconds field. + GetTimeoutSeconds() *int32 + // GetAdmissionReviewVersions gets the webhook AdmissionReviewVersions field. + GetAdmissionReviewVersions() []string + + // GetMutatingWebhook if the accessor contains a MutatingWebhook, returns it and true, else returns false. + GetMutatingWebhook() (*v1beta1.MutatingWebhook, bool) + // GetValidatingWebhook if the accessor contains a ValidatingWebhook, returns it and true, else returns false. + GetValidatingWebhook() (*v1beta1.ValidatingWebhook, bool) +} + +// NewMutatingWebhookAccessor creates an accessor for a MutatingWebhook. +func NewMutatingWebhookAccessor(h *v1beta1.MutatingWebhook) WebhookAccessor { + return mutatingWebhookAccessor{h} +} + +type mutatingWebhookAccessor struct { + *v1beta1.MutatingWebhook +} + +func (m mutatingWebhookAccessor) GetName() string { + return m.Name +} +func (m mutatingWebhookAccessor) GetClientConfig() v1beta1.WebhookClientConfig { + return m.ClientConfig +} +func (m mutatingWebhookAccessor) GetRules() []v1beta1.RuleWithOperations { + return m.Rules +} +func (m mutatingWebhookAccessor) GetFailurePolicy() *v1beta1.FailurePolicyType { + return m.FailurePolicy +} +func (m mutatingWebhookAccessor) GetMatchPolicy() *v1beta1.MatchPolicyType { + return m.MatchPolicy +} +func (m mutatingWebhookAccessor) GetNamespaceSelector() *metav1.LabelSelector { + return m.NamespaceSelector +} +func (m mutatingWebhookAccessor) GetSideEffects() *v1beta1.SideEffectClass { + return m.SideEffects +} +func (m mutatingWebhookAccessor) GetTimeoutSeconds() *int32 { + return m.TimeoutSeconds +} +func (m mutatingWebhookAccessor) GetAdmissionReviewVersions() []string { + return m.AdmissionReviewVersions +} + +func (m mutatingWebhookAccessor) GetMutatingWebhook() (*v1beta1.MutatingWebhook, bool) { + return m.MutatingWebhook, true +} + +func (m mutatingWebhookAccessor) GetValidatingWebhook() (*v1beta1.ValidatingWebhook, bool) { + return nil, false +} + +// NewValidatingWebhookAccessor creates an accessor for a ValidatingWebhook. +func NewValidatingWebhookAccessor(h *v1beta1.ValidatingWebhook) WebhookAccessor { + return validatingWebhookAccessor{h} +} + +type validatingWebhookAccessor struct { + *v1beta1.ValidatingWebhook +} + +func (v validatingWebhookAccessor) GetName() string { + return v.Name +} +func (v validatingWebhookAccessor) GetClientConfig() v1beta1.WebhookClientConfig { + return v.ClientConfig +} +func (v validatingWebhookAccessor) GetRules() []v1beta1.RuleWithOperations { + return v.Rules +} +func (v validatingWebhookAccessor) GetFailurePolicy() *v1beta1.FailurePolicyType { + return v.FailurePolicy +} +func (v validatingWebhookAccessor) GetMatchPolicy() *v1beta1.MatchPolicyType { + return v.MatchPolicy +} +func (v validatingWebhookAccessor) GetNamespaceSelector() *metav1.LabelSelector { + return v.NamespaceSelector +} +func (v validatingWebhookAccessor) GetSideEffects() *v1beta1.SideEffectClass { + return v.SideEffects +} +func (v validatingWebhookAccessor) GetTimeoutSeconds() *int32 { + return v.TimeoutSeconds +} +func (v validatingWebhookAccessor) GetAdmissionReviewVersions() []string { + return v.AdmissionReviewVersions +} + +func (v validatingWebhookAccessor) GetMutatingWebhook() (*v1beta1.MutatingWebhook, bool) { + return nil, false +} + +func (v validatingWebhookAccessor) GetValidatingWebhook() (*v1beta1.ValidatingWebhook, bool) { + return v.ValidatingWebhook, true +} diff --git a/pkg/admission/plugin/webhook/generic/interfaces.go b/pkg/admission/plugin/webhook/generic/interfaces.go index c57189543..227502de8 100644 --- a/pkg/admission/plugin/webhook/generic/interfaces.go +++ b/pkg/admission/plugin/webhook/generic/interfaces.go @@ -19,15 +19,15 @@ package generic import ( "context" - "k8s.io/api/admissionregistration/v1beta1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/webhook" ) // Source can list dynamic webhook plugins. type Source interface { - Webhooks() []v1beta1.Webhook + Webhooks() []webhook.WebhookAccessor HasSynced() bool } @@ -51,8 +51,7 @@ type VersionedAttributes struct { // WebhookInvocation describes how to call a webhook, including the resource and subresource the webhook registered for, // and the kind that should be sent to the webhook. type WebhookInvocation struct { - Webhook *v1beta1.Webhook - + Webhook webhook.WebhookAccessor Resource schema.GroupVersionResource Subresource string Kind schema.GroupVersionKind diff --git a/pkg/admission/plugin/webhook/generic/webhook.go b/pkg/admission/plugin/webhook/generic/webhook.go index 30a9b5e7b..99415292c 100644 --- a/pkg/admission/plugin/webhook/generic/webhook.go +++ b/pkg/admission/plugin/webhook/generic/webhook.go @@ -27,10 +27,11 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" genericadmissioninit "k8s.io/apiserver/pkg/admission/initializer" + "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/apiserver/pkg/admission/plugin/webhook/config" "k8s.io/apiserver/pkg/admission/plugin/webhook/namespace" "k8s.io/apiserver/pkg/admission/plugin/webhook/rules" - "k8s.io/apiserver/pkg/util/webhook" + webhookutil "k8s.io/apiserver/pkg/util/webhook" "k8s.io/client-go/informers" clientset "k8s.io/client-go/kubernetes" ) @@ -42,7 +43,7 @@ type Webhook struct { sourceFactory sourceFactory hookSource Source - clientManager *webhook.ClientManager + clientManager *webhookutil.ClientManager namespaceMatcher *namespace.Matcher dispatcher Dispatcher } @@ -53,7 +54,7 @@ var ( ) type sourceFactory func(f informers.SharedInformerFactory) Source -type dispatcherFactory func(cm *webhook.ClientManager) Dispatcher +type dispatcherFactory func(cm *webhookutil.ClientManager) Dispatcher // NewWebhook creates a new generic admission webhook. func NewWebhook(handler *admission.Handler, configFile io.Reader, sourceFactory sourceFactory, dispatcherFactory dispatcherFactory) (*Webhook, error) { @@ -62,17 +63,17 @@ func NewWebhook(handler *admission.Handler, configFile io.Reader, sourceFactory return nil, err } - cm, err := webhook.NewClientManager(admissionv1beta1.SchemeGroupVersion, admissionv1beta1.AddToScheme) + cm, err := webhookutil.NewClientManager(admissionv1beta1.SchemeGroupVersion, admissionv1beta1.AddToScheme) if err != nil { return nil, err } - authInfoResolver, err := webhook.NewDefaultAuthenticationInfoResolver(kubeconfigFile) + authInfoResolver, err := webhookutil.NewDefaultAuthenticationInfoResolver(kubeconfigFile) if err != nil { return nil, err } // Set defaults which may be overridden later. cm.SetAuthenticationInfoResolver(authInfoResolver) - cm.SetServiceResolver(webhook.NewDefaultServiceResolver()) + cm.SetServiceResolver(webhookutil.NewDefaultServiceResolver()) return &Webhook{ Handler: handler, @@ -86,13 +87,13 @@ func NewWebhook(handler *admission.Handler, configFile io.Reader, sourceFactory // SetAuthenticationInfoResolverWrapper sets the // AuthenticationInfoResolverWrapper. // TODO find a better way wire this, but keep this pull small for now. -func (a *Webhook) SetAuthenticationInfoResolverWrapper(wrapper webhook.AuthenticationInfoResolverWrapper) { +func (a *Webhook) SetAuthenticationInfoResolverWrapper(wrapper webhookutil.AuthenticationInfoResolverWrapper) { a.clientManager.SetAuthenticationInfoResolverWrapper(wrapper) } // SetServiceResolver sets a service resolver for the webhook admission plugin. // Passing a nil resolver does not have an effect, instead a default one will be used. -func (a *Webhook) SetServiceResolver(sr webhook.ServiceResolver) { +func (a *Webhook) SetServiceResolver(sr webhookutil.ServiceResolver) { a.clientManager.SetServiceResolver(sr) } @@ -128,10 +129,10 @@ func (a *Webhook) ValidateInitialization() error { // shouldCallHook returns invocation details if the webhook should be called, nil if the webhook should not be called, // or an error if an error was encountered during evaluation. -func (a *Webhook) shouldCallHook(h *v1beta1.Webhook, attr admission.Attributes, o admission.ObjectInterfaces) (*WebhookInvocation, *apierrors.StatusError) { +func (a *Webhook) shouldCallHook(h webhook.WebhookAccessor, attr admission.Attributes, o admission.ObjectInterfaces) (*WebhookInvocation, *apierrors.StatusError) { var err *apierrors.StatusError var invocation *WebhookInvocation - for _, r := range h.Rules { + for _, r := range h.GetRules() { m := rules.Matcher{Rule: r, Attr: attr} if m.Matches() { invocation = &WebhookInvocation{ @@ -143,12 +144,12 @@ func (a *Webhook) shouldCallHook(h *v1beta1.Webhook, attr admission.Attributes, break } } - if invocation == nil && h.MatchPolicy != nil && *h.MatchPolicy == v1beta1.Equivalent { + if invocation == nil && h.GetMatchPolicy() != nil && *h.GetMatchPolicy() == v1beta1.Equivalent { attrWithOverride := &attrWithResourceOverride{Attributes: attr} equivalents := o.GetEquivalentResourceMapper().EquivalentResourcesFor(attr.GetResource(), attr.GetSubresource()) // honor earlier rules first OuterLoop: - for _, r := range h.Rules { + for _, r := range h.GetRules() { // see if the rule matches any of the equivalent resources for _, equivalent := range equivalents { if equivalent == attr.GetResource() { @@ -207,7 +208,7 @@ func (a *Webhook) Dispatch(attr admission.Attributes, o admission.ObjectInterfac var relevantHooks []*WebhookInvocation for i := range hooks { - invocation, err := a.shouldCallHook(&hooks[i], attr, o) + invocation, err := a.shouldCallHook(hooks[i], attr, o) if err != nil { return err } diff --git a/pkg/admission/plugin/webhook/generic/webhook_test.go b/pkg/admission/plugin/webhook/generic/webhook_test.go index b214122ad..c2ddbb7b1 100644 --- a/pkg/admission/plugin/webhook/generic/webhook_test.go +++ b/pkg/admission/plugin/webhook/generic/webhook_test.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/apiserver/pkg/admission/plugin/webhook/namespace" ) @@ -61,7 +62,7 @@ func TestShouldCallHook(t *testing.T) { testcases := []struct { name string - webhook *v1beta1.Webhook + webhook *v1beta1.ValidatingWebhook attrs admission.Attributes expectCall bool @@ -72,13 +73,13 @@ func TestShouldCallHook(t *testing.T) { }{ { name: "no rules (just write)", - webhook: &v1beta1.Webhook{Rules: []v1beta1.RuleWithOperations{}}, + webhook: &v1beta1.ValidatingWebhook{Rules: []v1beta1.RuleWithOperations{}}, attrs: admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{"apps", "v1", "Deployment"}, "ns", "name", schema.GroupVersionResource{"apps", "v1", "deployments"}, "", admission.Create, &metav1.CreateOptions{}, false, nil), expectCall: false, }, { name: "invalid kind lookup", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, MatchPolicy: &equivalentMatch, Rules: []v1beta1.RuleWithOperations{{ @@ -91,7 +92,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "wildcard rule, match as requested", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, @@ -105,7 +106,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, prefer exact match", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, @@ -125,7 +126,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, match miss", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, @@ -139,7 +140,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, exact match miss", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &exactMatch, NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ @@ -154,7 +155,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, equivalent match, prefer extensions", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &equivalentMatch, NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ @@ -172,7 +173,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, equivalent match, prefer apps", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &equivalentMatch, NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ @@ -191,7 +192,7 @@ func TestShouldCallHook(t *testing.T) { { name: "specific rules, subresource prefer exact match", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, @@ -211,7 +212,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, subresource match miss", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, @@ -225,7 +226,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, subresource exact match miss", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &exactMatch, NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ @@ -240,7 +241,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, subresource equivalent match, prefer extensions", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &equivalentMatch, NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ @@ -258,7 +259,7 @@ func TestShouldCallHook(t *testing.T) { }, { name: "specific rules, subresource equivalent match, prefer apps", - webhook: &v1beta1.Webhook{ + webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &equivalentMatch, NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ @@ -278,7 +279,7 @@ func TestShouldCallHook(t *testing.T) { for _, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - invocation, err := a.shouldCallHook(testcase.webhook, testcase.attrs, interfaces) + invocation, err := a.shouldCallHook(webhook.NewValidatingWebhookAccessor(testcase.webhook), testcase.attrs, interfaces) if err != nil { if len(testcase.expectErr) == 0 { t.Fatal(err) diff --git a/pkg/admission/plugin/webhook/mutating/dispatcher.go b/pkg/admission/plugin/webhook/mutating/dispatcher.go index 33190ff12..b206d2a53 100644 --- a/pkg/admission/plugin/webhook/mutating/dispatcher.go +++ b/pkg/admission/plugin/webhook/mutating/dispatcher.go @@ -39,16 +39,16 @@ import ( "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" "k8s.io/apiserver/pkg/admission/plugin/webhook/request" "k8s.io/apiserver/pkg/admission/plugin/webhook/util" - "k8s.io/apiserver/pkg/util/webhook" + webhookutil "k8s.io/apiserver/pkg/util/webhook" ) type mutatingDispatcher struct { - cm *webhook.ClientManager + cm *webhookutil.ClientManager plugin *Plugin } -func newMutatingDispatcher(p *Plugin) func(cm *webhook.ClientManager) generic.Dispatcher { - return func(cm *webhook.ClientManager) generic.Dispatcher { +func newMutatingDispatcher(p *Plugin) func(cm *webhookutil.ClientManager) generic.Dispatcher { + return func(cm *webhookutil.ClientManager) generic.Dispatcher { return &mutatingDispatcher{cm, p} } } @@ -58,7 +58,10 @@ var _ generic.Dispatcher = &mutatingDispatcher{} func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attributes, o admission.ObjectInterfaces, relevantHooks []*generic.WebhookInvocation) error { var versionedAttr *generic.VersionedAttributes for _, invocation := range relevantHooks { - hook := invocation.Webhook + hook, ok := invocation.Webhook.GetMutatingWebhook() + if !ok { + return fmt.Errorf("mutating webhook dispatch requires v1beta1.MutatingWebhook, but got %T", hook) + } if versionedAttr == nil { // First webhook, create versioned attributes var err error @@ -73,14 +76,14 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib } t := time.Now() - err := a.callAttrMutatingHook(ctx, invocation, versionedAttr, o) + err := a.callAttrMutatingHook(ctx, hook, invocation, versionedAttr, o) admissionmetrics.Metrics.ObserveWebhook(time.Since(t), err != nil, versionedAttr.Attributes, "admit", hook.Name) if err == nil { continue } ignoreClientCallFailures := hook.FailurePolicy != nil && *hook.FailurePolicy == v1beta1.Ignore - if callErr, ok := err.(*webhook.ErrCallingWebhook); ok { + if callErr, ok := err.(*webhookutil.ErrCallingWebhook); ok { if ignoreClientCallFailures { klog.Warningf("Failed calling webhook, failing open %v: %v", hook.Name, callErr) utilruntime.HandleError(callErr) @@ -100,11 +103,11 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib } // note that callAttrMutatingHook updates attr -func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, invocation *generic.WebhookInvocation, attr *generic.VersionedAttributes, o admission.ObjectInterfaces) error { - h := invocation.Webhook + +func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta1.MutatingWebhook, invocation *generic.WebhookInvocation, attr *generic.VersionedAttributes, o admission.ObjectInterfaces) error { if attr.Attributes.IsDryRun() { if h.SideEffects == nil { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook SideEffects is nil")} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook SideEffects is nil")} } if !(*h.SideEffects == v1beta1.SideEffectClassNone || *h.SideEffects == v1beta1.SideEffectClassNoneOnDryRun) { return webhookerrors.NewDryRunUnsupportedErr(h.Name) @@ -113,15 +116,15 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, invocatio // Currently dispatcher only supports `v1beta1` AdmissionReview // TODO: Make the dispatcher capable of sending multiple AdmissionReview versions - if !util.HasAdmissionReviewVersion(v1beta1.SchemeGroupVersion.Version, h) { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("webhook does not accept v1beta1 AdmissionReview")} + if !util.HasAdmissionReviewVersion(v1beta1.SchemeGroupVersion.Version, invocation.Webhook) { + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("webhook does not accept v1beta1 AdmissionReview")} } // Make the webhook request request := request.CreateAdmissionReview(attr, invocation) - client, err := a.cm.HookClient(util.HookClientConfigForWebhook(h)) + client, err := a.cm.HookClient(util.HookClientConfigForWebhook(invocation.Webhook)) if err != nil { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: err} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: err} } response := &admissionv1beta1.AdmissionReview{} r := client.Post().Context(ctx).Body(&request) @@ -129,11 +132,11 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, invocatio r = r.Timeout(time.Duration(*h.TimeoutSeconds) * time.Second) } if err := r.Do().Into(response); err != nil { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: err} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: err} } if response.Response == nil { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook response was absent")} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook response was absent")} } for k, v := range response.Response.AuditAnnotations { diff --git a/pkg/admission/plugin/webhook/mutating/plugin_test.go b/pkg/admission/plugin/webhook/mutating/plugin_test.go index 4a1a1c328..b8f98eedf 100644 --- a/pkg/admission/plugin/webhook/mutating/plugin_test.go +++ b/pkg/admission/plugin/webhook/mutating/plugin_test.go @@ -46,7 +46,7 @@ func TestAdmit(t *testing.T) { defer close(stopCh) testCases := append(webhooktesting.NewMutatingTestCases(serverURL), - webhooktesting.NewNonMutatingTestCases(serverURL)...) + webhooktesting.ConvertToMutatingTestCases(webhooktesting.NewNonMutatingTestCases(serverURL))...) for _, tt := range testCases { wh, err := NewMutatingWebhook(nil) @@ -56,7 +56,7 @@ func TestAdmit(t *testing.T) { } ns := "webhook-test" - client, informer := webhooktesting.NewFakeDataSource(ns, tt.Webhooks, true, stopCh) + client, informer := webhooktesting.NewFakeMutatingDataSource(ns, tt.Webhooks, stopCh) wh.SetAuthenticationInfoResolverWrapper(webhooktesting.Wrapper(webhooktesting.NewAuthenticationInfoResolver(new(int32)))) wh.SetServiceResolver(webhooktesting.NewServiceResolver(*serverURL)) @@ -136,7 +136,7 @@ func TestAdmitCachedClient(t *testing.T) { for _, tt := range webhooktesting.NewCachedClientTestcases(serverURL) { ns := "webhook-test" - client, informer := webhooktesting.NewFakeDataSource(ns, tt.Webhooks, true, stopCh) + client, informer := webhooktesting.NewFakeMutatingDataSource(ns, webhooktesting.ConvertToMutatingWebhooks(tt.Webhooks), stopCh) // override the webhook source. The client cache will stay the same. cacheMisses := new(int32) diff --git a/pkg/admission/plugin/webhook/namespace/matcher.go b/pkg/admission/plugin/webhook/namespace/matcher.go index 78dbd5282..4530de3e6 100644 --- a/pkg/admission/plugin/webhook/namespace/matcher.go +++ b/pkg/admission/plugin/webhook/namespace/matcher.go @@ -19,13 +19,13 @@ package namespace import ( "fmt" - "k8s.io/api/admissionregistration/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/webhook" clientset "k8s.io/client-go/kubernetes" corelisters "k8s.io/client-go/listers/core/v1" ) @@ -86,7 +86,7 @@ func (m *Matcher) GetNamespaceLabels(attr admission.Attributes) (map[string]stri // MatchNamespaceSelector decideds whether the request matches the // namespaceSelctor of the webhook. Only when they match, the webhook is called. -func (m *Matcher) MatchNamespaceSelector(h *v1beta1.Webhook, attr admission.Attributes) (bool, *apierrors.StatusError) { +func (m *Matcher) MatchNamespaceSelector(h webhook.WebhookAccessor, attr admission.Attributes) (bool, *apierrors.StatusError) { namespaceName := attr.GetNamespace() if len(namespaceName) == 0 && attr.GetResource().Resource != "namespaces" { // If the request is about a cluster scoped resource, and it is not a @@ -96,7 +96,7 @@ func (m *Matcher) MatchNamespaceSelector(h *v1beta1.Webhook, attr admission.Attr return true, nil } // TODO: adding an LRU cache to cache the translation - selector, err := metav1.LabelSelectorAsSelector(h.NamespaceSelector) + selector, err := metav1.LabelSelectorAsSelector(h.GetNamespaceSelector()) if err != nil { return false, apierrors.NewInternalError(err) } diff --git a/pkg/admission/plugin/webhook/namespace/matcher_test.go b/pkg/admission/plugin/webhook/namespace/matcher_test.go index 616ff7bbe..4633cac8e 100644 --- a/pkg/admission/plugin/webhook/namespace/matcher_test.go +++ b/pkg/admission/plugin/webhook/namespace/matcher_test.go @@ -27,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/webhook" ) type fakeNamespaceLister struct { @@ -114,12 +115,12 @@ func TestGetNamespaceLabels(t *testing.T) { } func TestNotExemptClusterScopedResource(t *testing.T) { - hook := ®istrationv1beta1.Webhook{ + hook := ®istrationv1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, } attr := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "mock-name", schema.GroupVersionResource{Version: "v1", Resource: "nodes"}, "", admission.Create, &metav1.CreateOptions{}, false, nil) matcher := Matcher{} - matches, err := matcher.MatchNamespaceSelector(hook, attr) + matches, err := matcher.MatchNamespaceSelector(webhook.NewValidatingWebhookAccessor(hook), attr) if err != nil { t.Fatal(err) } diff --git a/pkg/admission/plugin/webhook/testing/testcase.go b/pkg/admission/plugin/webhook/testing/testcase.go index ee5467478..ec8e6b552 100644 --- a/pkg/admission/plugin/webhook/testing/testcase.go +++ b/pkg/admission/plugin/webhook/testing/testcase.go @@ -49,8 +49,8 @@ var sideEffectsNone = registrationv1beta1.SideEffectClassNone var sideEffectsSome = registrationv1beta1.SideEffectClassSome var sideEffectsNoneOnDryRun = registrationv1beta1.SideEffectClassNoneOnDryRun -// NewFakeDataSource returns a mock client and informer returning the given webhooks. -func NewFakeDataSource(name string, webhooks []registrationv1beta1.Webhook, mutating bool, stopCh <-chan struct{}) (clientset kubernetes.Interface, factory informers.SharedInformerFactory) { +// NewFakeValidatingDataSource returns a mock client and informer returning the given webhooks. +func NewFakeValidatingDataSource(name string, webhooks []registrationv1beta1.ValidatingWebhook, stopCh <-chan struct{}) (clientset kubernetes.Interface, factory informers.SharedInformerFactory) { var objs = []runtime.Object{ &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -61,21 +61,37 @@ func NewFakeDataSource(name string, webhooks []registrationv1beta1.Webhook, muta }, }, } - if mutating { - objs = append(objs, ®istrationv1beta1.MutatingWebhookConfiguration{ + objs = append(objs, ®istrationv1beta1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-webhooks", + }, + Webhooks: webhooks, + }) + + client := fakeclientset.NewSimpleClientset(objs...) + informerFactory := informers.NewSharedInformerFactory(client, 0) + + return client, informerFactory +} + +// NewFakeMutatingDataSource returns a mock client and informer returning the given webhooks. +func NewFakeMutatingDataSource(name string, webhooks []registrationv1beta1.MutatingWebhook, stopCh <-chan struct{}) (clientset kubernetes.Interface, factory informers.SharedInformerFactory) { + var objs = []runtime.Object{ + &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-webhooks", + Name: name, + Labels: map[string]string{ + "runlevel": "0", + }, }, - Webhooks: webhooks, - }) - } else { - objs = append(objs, ®istrationv1beta1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-webhooks", - }, - Webhooks: webhooks, - }) + }, } + objs = append(objs, ®istrationv1beta1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-webhooks", + }, + Webhooks: webhooks, + }) client := fakeclientset.NewSimpleClientset(objs...) informerFactory := informers.NewSharedInformerFactory(client, 0) @@ -181,10 +197,10 @@ func (c urlConfigGenerator) ccfgURL(urlPath string) registrationv1beta1.WebhookC } } -// Test is a webhook test case. -type Test struct { +// ValidatingTest is a validating webhook test case. +type ValidatingTest struct { Name string - Webhooks []registrationv1beta1.Webhook + Webhooks []registrationv1beta1.ValidatingWebhook Path string IsCRD bool IsDryRun bool @@ -196,19 +212,52 @@ type Test struct { ExpectStatusCode int32 } +// MutatingTest is a mutating webhook test case. +type MutatingTest struct { + Name string + Webhooks []registrationv1beta1.MutatingWebhook + Path string + IsCRD bool + IsDryRun bool + AdditionalLabels map[string]string + ExpectLabels map[string]string + ExpectAllow bool + ErrorContains string + ExpectAnnotations map[string]string + ExpectStatusCode int32 +} + +// ConvertToMutatingTestCases converts a validating test case to a mutating one for test purposes. +func ConvertToMutatingTestCases(tests []ValidatingTest) []MutatingTest { + r := make([]MutatingTest, len(tests)) + for i, t := range tests { + r[i] = MutatingTest{t.Name, ConvertToMutatingWebhooks(t.Webhooks), t.Path, t.IsCRD, t.IsDryRun, t.AdditionalLabels, t.ExpectLabels, t.ExpectAllow, t.ErrorContains, t.ExpectAnnotations, t.ExpectStatusCode} + } + return r +} + +// ConvertToMutatingWebhooks converts a validating webhook to a mutating one for test purposes. +func ConvertToMutatingWebhooks(webhooks []registrationv1beta1.ValidatingWebhook) []registrationv1beta1.MutatingWebhook { + mutating := make([]registrationv1beta1.MutatingWebhook, len(webhooks)) + for i, h := range webhooks { + mutating[i] = registrationv1beta1.MutatingWebhook{h.Name, h.ClientConfig, h.Rules, h.FailurePolicy, h.MatchPolicy, h.NamespaceSelector, h.SideEffects, h.TimeoutSeconds, h.AdmissionReviewVersions} + } + return mutating +} + // NewNonMutatingTestCases returns test cases with a given base url. // All test cases in NewNonMutatingTestCases have no Patch set in // AdmissionResponse. The test cases are used by both MutatingAdmissionWebhook // and ValidatingAdmissionWebhook. -func NewNonMutatingTestCases(url *url.URL) []Test { +func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { policyFail := registrationv1beta1.Fail policyIgnore := registrationv1beta1.Ignore ccfgURL := urlConfigGenerator{url}.ccfgURL - return []Test{ + return []ValidatingTest{ { Name: "no match", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "nomatch", ClientConfig: ccfgSVC("disallow"), Rules: []registrationv1beta1.RuleWithOperations{{ @@ -221,7 +270,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & allow", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "allow.example.com", ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, @@ -233,7 +282,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & disallow", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "disallow", ClientConfig: ccfgSVC("disallow"), Rules: matchEverythingRules, @@ -245,7 +294,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & disallow ii", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "disallowReason", ClientConfig: ccfgSVC("disallowReason"), Rules: matchEverythingRules, @@ -257,7 +306,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & disallow & but allowed because namespaceSelector exempt the ns", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "disallow", ClientConfig: ccfgSVC("disallow"), Rules: newMatchEverythingRules(), @@ -275,7 +324,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & disallow & but allowed because namespaceSelector exempt the ns ii", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "disallow", ClientConfig: ccfgSVC("disallow"), Rules: newMatchEverythingRules(), @@ -292,7 +341,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & fail (but allow because fail open)", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "internalErr A", ClientConfig: ccfgSVC("internalErr"), Rules: matchEverythingRules, @@ -319,7 +368,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & fail (but disallow because fail close on nil FailurePolicy)", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "internalErr A", ClientConfig: ccfgSVC("internalErr"), NamespaceSelector: &metav1.LabelSelector{}, @@ -343,7 +392,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & fail (but fail because fail closed)", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "internalErr A", ClientConfig: ccfgSVC("internalErr"), Rules: matchEverythingRules, @@ -370,7 +419,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & allow (url)", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "allow.example.com", ClientConfig: ccfgURL("allow"), Rules: matchEverythingRules, @@ -382,7 +431,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match & disallow (url)", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "disallow", ClientConfig: ccfgURL("disallow"), Rules: matchEverythingRules, @@ -393,7 +442,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { ErrorContains: "without explanation", }, { Name: "absent response and fail open", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "nilResponse", ClientConfig: ccfgURL("nilResponse"), FailurePolicy: &policyIgnore, @@ -405,7 +454,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "absent response and fail closed", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "nilResponse", ClientConfig: ccfgURL("nilResponse"), FailurePolicy: &policyFail, @@ -418,7 +467,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "no match dry run", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "nomatch", ClientConfig: ccfgSVC("allow"), Rules: []registrationv1beta1.RuleWithOperations{{ @@ -433,7 +482,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match dry run side effects Unknown", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "allow", ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, @@ -447,7 +496,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match dry run side effects None", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "allow", ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, @@ -461,7 +510,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match dry run side effects Some", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "allow", ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, @@ -475,7 +524,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "match dry run side effects NoneOnDryRun", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "allow", ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, @@ -489,7 +538,7 @@ func NewNonMutatingTestCases(url *url.URL) []Test { }, { Name: "illegal annotation format", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "invalidAnnotation", ClientConfig: ccfgURL("invalidAnnotation"), Rules: matchEverythingRules, @@ -506,11 +555,11 @@ func NewNonMutatingTestCases(url *url.URL) []Test { // NewMutatingTestCases returns test cases with a given base url. // All test cases in NewMutatingTestCases have Patch set in // AdmissionResponse. The test cases are only used by both MutatingAdmissionWebhook. -func NewMutatingTestCases(url *url.URL) []Test { - return []Test{ +func NewMutatingTestCases(url *url.URL) []MutatingTest { + return []MutatingTest{ { Name: "match & remove label", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.MutatingWebhook{{ Name: "removelabel.example.com", ClientConfig: ccfgSVC("removeLabel"), Rules: matchEverythingRules, @@ -524,7 +573,7 @@ func NewMutatingTestCases(url *url.URL) []Test { }, { Name: "match & add label", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.MutatingWebhook{{ Name: "addLabel", ClientConfig: ccfgSVC("addLabel"), Rules: matchEverythingRules, @@ -536,7 +585,7 @@ func NewMutatingTestCases(url *url.URL) []Test { }, { Name: "match CRD & add label", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.MutatingWebhook{{ Name: "addLabel", ClientConfig: ccfgSVC("addLabel"), Rules: matchEverythingRules, @@ -549,7 +598,7 @@ func NewMutatingTestCases(url *url.URL) []Test { }, { Name: "match CRD & remove label", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.MutatingWebhook{{ Name: "removelabel.example.com", ClientConfig: ccfgSVC("removeLabel"), Rules: matchEverythingRules, @@ -564,7 +613,7 @@ func NewMutatingTestCases(url *url.URL) []Test { }, { Name: "match & invalid mutation", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.MutatingWebhook{{ Name: "invalidMutation", ClientConfig: ccfgSVC("invalidMutation"), Rules: matchEverythingRules, @@ -576,7 +625,7 @@ func NewMutatingTestCases(url *url.URL) []Test { }, { Name: "match & remove label dry run unsupported", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.MutatingWebhook{{ Name: "removeLabel", ClientConfig: ccfgSVC("removeLabel"), Rules: matchEverythingRules, @@ -596,7 +645,7 @@ func NewMutatingTestCases(url *url.URL) []Test { // CachedTest is a test case for the client manager. type CachedTest struct { Name string - Webhooks []registrationv1beta1.Webhook + Webhooks []registrationv1beta1.ValidatingWebhook ExpectAllow bool ExpectCacheMiss bool } @@ -609,7 +658,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { return []CachedTest{ { Name: "uncached: service webhook, path 'allow'", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "cache1", ClientConfig: ccfgSVC("allow"), Rules: newMatchEverythingRules(), @@ -622,7 +671,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { }, { Name: "uncached: service webhook, path 'internalErr'", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "cache2", ClientConfig: ccfgSVC("internalErr"), Rules: newMatchEverythingRules(), @@ -635,7 +684,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { }, { Name: "cached: service webhook, path 'allow'", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "cache3", ClientConfig: ccfgSVC("allow"), Rules: newMatchEverythingRules(), @@ -648,7 +697,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { }, { Name: "uncached: url webhook, path 'allow'", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "cache4", ClientConfig: ccfgURL("allow"), Rules: newMatchEverythingRules(), @@ -661,7 +710,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { }, { Name: "cached: service webhook, path 'allow'", - Webhooks: []registrationv1beta1.Webhook{{ + Webhooks: []registrationv1beta1.ValidatingWebhook{{ Name: "cache5", ClientConfig: ccfgURL("allow"), Rules: newMatchEverythingRules(), diff --git a/pkg/admission/plugin/webhook/util/client_config.go b/pkg/admission/plugin/webhook/util/client_config.go index 8f489639a..b9907286f 100644 --- a/pkg/admission/plugin/webhook/util/client_config.go +++ b/pkg/admission/plugin/webhook/util/client_config.go @@ -17,38 +17,39 @@ limitations under the License. package util import ( - "k8s.io/api/admissionregistration/v1beta1" - "k8s.io/apiserver/pkg/util/webhook" + "k8s.io/apiserver/pkg/admission/plugin/webhook" + webhookutil "k8s.io/apiserver/pkg/util/webhook" ) -// HookClientConfigForWebhook construct a webhook.ClientConfig using a v1beta1.Webhook API object. -// webhook.ClientConfig is used to create a HookClient and the purpose of the config struct is to -// share that with other packages that need to create a HookClient. -func HookClientConfigForWebhook(w *v1beta1.Webhook) webhook.ClientConfig { - ret := webhook.ClientConfig{Name: w.Name, CABundle: w.ClientConfig.CABundle} - if w.ClientConfig.URL != nil { - ret.URL = *w.ClientConfig.URL +// HookClientConfigForWebhook construct a webhookutil.ClientConfig using a WebhookAccessor to access +// v1beta1.MutatingWebhook and v1beta1.ValidatingWebhook API objects. webhookutil.ClientConfig is used +// to create a HookClient and the purpose of the config struct is to share that with other packages +// that need to create a HookClient. +func HookClientConfigForWebhook(w webhook.WebhookAccessor) webhookutil.ClientConfig { + ret := webhookutil.ClientConfig{Name: w.GetName(), CABundle: w.GetClientConfig().CABundle} + if w.GetClientConfig().URL != nil { + ret.URL = *w.GetClientConfig().URL } - if w.ClientConfig.Service != nil { - ret.Service = &webhook.ClientConfigService{ - Name: w.ClientConfig.Service.Name, - Namespace: w.ClientConfig.Service.Namespace, + if w.GetClientConfig().Service != nil { + ret.Service = &webhookutil.ClientConfigService{ + Name: w.GetClientConfig().Service.Name, + Namespace: w.GetClientConfig().Service.Namespace, } - if w.ClientConfig.Service.Port != nil { - ret.Service.Port = *w.ClientConfig.Service.Port + if w.GetClientConfig().Service.Port != nil { + ret.Service.Port = *w.GetClientConfig().Service.Port } else { ret.Service.Port = 443 } - if w.ClientConfig.Service.Path != nil { - ret.Service.Path = *w.ClientConfig.Service.Path + if w.GetClientConfig().Service.Path != nil { + ret.Service.Path = *w.GetClientConfig().Service.Path } } return ret } // HasAdmissionReviewVersion check whether a version is accepted by a given webhook. -func HasAdmissionReviewVersion(a string, w *v1beta1.Webhook) bool { - for _, b := range w.AdmissionReviewVersions { +func HasAdmissionReviewVersion(a string, w webhook.WebhookAccessor) bool { + for _, b := range w.GetAdmissionReviewVersions() { if b == a { return true } diff --git a/pkg/admission/plugin/webhook/validating/dispatcher.go b/pkg/admission/plugin/webhook/validating/dispatcher.go index d7421e2b5..0505103d2 100644 --- a/pkg/admission/plugin/webhook/validating/dispatcher.go +++ b/pkg/admission/plugin/webhook/validating/dispatcher.go @@ -22,8 +22,6 @@ import ( "sync" "time" - "k8s.io/klog" - admissionv1beta1 "k8s.io/api/admission/v1beta1" "k8s.io/api/admissionregistration/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -35,14 +33,15 @@ import ( "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" "k8s.io/apiserver/pkg/admission/plugin/webhook/request" "k8s.io/apiserver/pkg/admission/plugin/webhook/util" - "k8s.io/apiserver/pkg/util/webhook" + webhookutil "k8s.io/apiserver/pkg/util/webhook" + "k8s.io/klog" ) type validatingDispatcher struct { - cm *webhook.ClientManager + cm *webhookutil.ClientManager } -func newValidatingDispatcher(cm *webhook.ClientManager) generic.Dispatcher { +func newValidatingDispatcher(cm *webhookutil.ClientManager) generic.Dispatcher { return &validatingDispatcher{cm} } @@ -69,18 +68,21 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attr for i := range relevantHooks { go func(invocation *generic.WebhookInvocation) { defer wg.Done() - hook := invocation.Webhook + hook, ok := invocation.Webhook.GetValidatingWebhook() + if !ok { + utilruntime.HandleError(fmt.Errorf("validating webhook dispatch requires v1beta1.ValidatingWebhook, but got %T", hook)) + return + } versionedAttr := versionedAttrs[invocation.Kind] - t := time.Now() - err := d.callHook(ctx, invocation, versionedAttr) + err := d.callHook(ctx, hook, invocation, versionedAttr) admissionmetrics.Metrics.ObserveWebhook(time.Since(t), err != nil, versionedAttr.Attributes, "validating", hook.Name) if err == nil { return } ignoreClientCallFailures := hook.FailurePolicy != nil && *hook.FailurePolicy == v1beta1.Ignore - if callErr, ok := err.(*webhook.ErrCallingWebhook); ok { + if callErr, ok := err.(*webhookutil.ErrCallingWebhook); ok { if ignoreClientCallFailures { klog.Warningf("Failed calling webhook, failing open %v: %v", hook.Name, callErr) utilruntime.HandleError(callErr) @@ -115,11 +117,10 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attr return errs[0] } -func (d *validatingDispatcher) callHook(ctx context.Context, invocation *generic.WebhookInvocation, attr *generic.VersionedAttributes) error { - h := invocation.Webhook +func (d *validatingDispatcher) callHook(ctx context.Context, h *v1beta1.ValidatingWebhook, invocation *generic.WebhookInvocation, attr *generic.VersionedAttributes) error { if attr.Attributes.IsDryRun() { if h.SideEffects == nil { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook SideEffects is nil")} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook SideEffects is nil")} } if !(*h.SideEffects == v1beta1.SideEffectClassNone || *h.SideEffects == v1beta1.SideEffectClassNoneOnDryRun) { return webhookerrors.NewDryRunUnsupportedErr(h.Name) @@ -128,15 +129,15 @@ func (d *validatingDispatcher) callHook(ctx context.Context, invocation *generic // Currently dispatcher only supports `v1beta1` AdmissionReview // TODO: Make the dispatcher capable of sending multiple AdmissionReview versions - if !util.HasAdmissionReviewVersion(v1beta1.SchemeGroupVersion.Version, h) { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("webhook does not accept v1beta1 AdmissionReviewRequest")} + if !util.HasAdmissionReviewVersion(v1beta1.SchemeGroupVersion.Version, invocation.Webhook) { + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("webhook does not accept v1beta1 AdmissionReviewRequest")} } // Make the webhook request request := request.CreateAdmissionReview(attr, invocation) - client, err := d.cm.HookClient(util.HookClientConfigForWebhook(h)) + client, err := d.cm.HookClient(util.HookClientConfigForWebhook(invocation.Webhook)) if err != nil { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: err} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: err} } response := &admissionv1beta1.AdmissionReview{} r := client.Post().Context(ctx).Body(&request) @@ -144,11 +145,11 @@ func (d *validatingDispatcher) callHook(ctx context.Context, invocation *generic r = r.Timeout(time.Duration(*h.TimeoutSeconds) * time.Second) } if err := r.Do().Into(response); err != nil { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: err} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: err} } if response.Response == nil { - return &webhook.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook response was absent")} + return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook response was absent")} } for k, v := range response.Response.AuditAnnotations { key := h.Name + "/" + k diff --git a/pkg/admission/plugin/webhook/validating/plugin_test.go b/pkg/admission/plugin/webhook/validating/plugin_test.go index 0ed2a9744..a2aec191e 100644 --- a/pkg/admission/plugin/webhook/validating/plugin_test.go +++ b/pkg/admission/plugin/webhook/validating/plugin_test.go @@ -51,7 +51,7 @@ func TestValidate(t *testing.T) { } ns := "webhook-test" - client, informer := webhooktesting.NewFakeDataSource(ns, tt.Webhooks, false, stopCh) + client, informer := webhooktesting.NewFakeValidatingDataSource(ns, tt.Webhooks, stopCh) wh.SetAuthenticationInfoResolverWrapper(webhooktesting.Wrapper(webhooktesting.NewAuthenticationInfoResolver(new(int32)))) wh.SetServiceResolver(webhooktesting.NewServiceResolver(*serverURL)) @@ -116,7 +116,7 @@ func TestValidateCachedClient(t *testing.T) { for _, tt := range webhooktesting.NewCachedClientTestcases(serverURL) { ns := "webhook-test" - client, informer := webhooktesting.NewFakeDataSource(ns, tt.Webhooks, false, stopCh) + client, informer := webhooktesting.NewFakeValidatingDataSource(ns, tt.Webhooks, stopCh) // override the webhook source. The client cache will stay the same. cacheMisses := new(int32) From b22ec2bd9882366f2c85de22a7ca110ef4e53822 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Wed, 29 May 2019 22:31:26 -0700 Subject: [PATCH 2/5] Add mutating admission webhook reinvocation Kubernetes-commit: 95fa928ecb636e8d16af31ab613678c555fc76a3 --- pkg/admission/attributes.go | 65 +++++++-- .../configuration/mutating_webhook_manager.go | 8 +- .../validating_webhook_manager.go | 8 +- pkg/admission/interfaces.go | 19 +++ pkg/admission/plugin/webhook/accessors.go | 23 +++- .../plugin/webhook/generic/webhook_test.go | 5 +- .../plugin/webhook/mutating/dispatcher.go | 71 +++++++--- .../plugin/webhook/mutating/plugin_test.go | 126 ++++++++++-------- .../webhook/mutating/reinvocationcontext.go | 68 ++++++++++ .../plugin/webhook/namespace/matcher_test.go | 2 +- .../plugin/webhook/testing/testcase.go | 110 +++++++++++---- .../plugin/webhook/testing/webhook_server.go | 7 + pkg/admission/plugins.go | 2 +- pkg/admission/reinvocation.go | 62 +++++++++ 14 files changed, 454 insertions(+), 122 deletions(-) create mode 100644 pkg/admission/plugin/webhook/mutating/reinvocationcontext.go create mode 100644 pkg/admission/reinvocation.go diff --git a/pkg/admission/attributes.go b/pkg/admission/attributes.go index ad6ca6ba6..beea941fc 100644 --- a/pkg/admission/attributes.go +++ b/pkg/admission/attributes.go @@ -44,21 +44,24 @@ type attributesRecord struct { // But ValidatingAdmissionWebhook add annotations concurrently. annotations map[string]string annotationsLock sync.RWMutex + + reinvocationContext ReinvocationContext } func NewAttributesRecord(object runtime.Object, oldObject runtime.Object, kind schema.GroupVersionKind, namespace, name string, resource schema.GroupVersionResource, subresource string, operation Operation, operationOptions runtime.Object, dryRun bool, userInfo user.Info) Attributes { return &attributesRecord{ - kind: kind, - namespace: namespace, - name: name, - resource: resource, - subresource: subresource, - operation: operation, - options: operationOptions, - dryRun: dryRun, - object: object, - oldObject: oldObject, - userInfo: userInfo, + kind: kind, + namespace: namespace, + name: name, + resource: resource, + subresource: subresource, + operation: operation, + options: operationOptions, + dryRun: dryRun, + object: object, + oldObject: oldObject, + userInfo: userInfo, + reinvocationContext: &reinvocationContext{}, } } @@ -140,6 +143,46 @@ func (record *attributesRecord) AddAnnotation(key, value string) error { return nil } +func (record *attributesRecord) GetReinvocationContext() ReinvocationContext { + return record.reinvocationContext +} + +type reinvocationContext struct { + // isReinvoke is true when admission plugins are being reinvoked + isReinvoke bool + // reinvokeRequested is true when an admission plugin requested a re-invocation of the chain + reinvokeRequested bool + // values stores reinvoke context values per plugin. + values map[string]interface{} +} + +func (rc *reinvocationContext) IsReinvoke() bool { + return rc.isReinvoke +} + +func (rc *reinvocationContext) SetIsReinvoke() { + rc.isReinvoke = true +} + +func (rc *reinvocationContext) ShouldReinvoke() bool { + return rc.reinvokeRequested +} + +func (rc *reinvocationContext) SetShouldReinvoke() { + rc.reinvokeRequested = true +} + +func (rc *reinvocationContext) SetValue(plugin string, v interface{}) { + if rc.values == nil { + rc.values = map[string]interface{}{} + } + rc.values[plugin] = v +} + +func (rc *reinvocationContext) Value(plugin string) interface{} { + return rc.values[plugin] +} + func checkKeyFormat(key string) error { parts := strings.Split(key, "/") if len(parts) != 2 { diff --git a/pkg/admission/configuration/mutating_webhook_manager.go b/pkg/admission/configuration/mutating_webhook_manager.go index bacf61722..8cff4a254 100644 --- a/pkg/admission/configuration/mutating_webhook_manager.go +++ b/pkg/admission/configuration/mutating_webhook_manager.go @@ -86,8 +86,14 @@ func mergeMutatingWebhookConfigurations(configurations []*v1beta1.MutatingWebhoo sort.SliceStable(configurations, MutatingWebhookConfigurationSorter(configurations).ByName) accessors := []webhook.WebhookAccessor{} for _, c := range configurations { + // webhook names are not validated for uniqueness, so we check for duplicates and + // add a int suffix to distinguish between them + names := map[string]int{} for i := range c.Webhooks { - accessors = append(accessors, webhook.NewMutatingWebhookAccessor(&c.Webhooks[i])) + n := c.Webhooks[i].Name + uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n]) + names[n]++ + accessors = append(accessors, webhook.NewMutatingWebhookAccessor(uid, &c.Webhooks[i])) } } return accessors diff --git a/pkg/admission/configuration/validating_webhook_manager.go b/pkg/admission/configuration/validating_webhook_manager.go index bcce1e70f..804d83fe6 100644 --- a/pkg/admission/configuration/validating_webhook_manager.go +++ b/pkg/admission/configuration/validating_webhook_manager.go @@ -84,8 +84,14 @@ func mergeValidatingWebhookConfigurations(configurations []*v1beta1.ValidatingWe sort.SliceStable(configurations, ValidatingWebhookConfigurationSorter(configurations).ByName) accessors := []webhook.WebhookAccessor{} for _, c := range configurations { + // webhook names are not validated for uniqueness, so we check for duplicates and + // add a int suffix to distinguish between them + names := map[string]int{} for i := range c.Webhooks { - accessors = append(accessors, webhook.NewValidatingWebhookAccessor(&c.Webhooks[i])) + n := c.Webhooks[i].Name + uid := fmt.Sprintf("%s/%s/%d", c.Name, n, names[n]) + names[n]++ + accessors = append(accessors, webhook.NewValidatingWebhookAccessor(uid, &c.Webhooks[i])) } } return accessors diff --git a/pkg/admission/interfaces.go b/pkg/admission/interfaces.go index 040a6268b..5f6d703b2 100644 --- a/pkg/admission/interfaces.go +++ b/pkg/admission/interfaces.go @@ -62,6 +62,9 @@ type Attributes interface { // An error is returned if the format of key is invalid. When trying to overwrite annotation with a new value, an error is returned. // Both ValidationInterface and MutationInterface are allowed to add Annotations. AddAnnotation(key, value string) error + + // GetReinvocationContext tracks the admission request information relevant to the re-invocation policy. + GetReinvocationContext() ReinvocationContext } // ObjectInterfaces is an interface used by AdmissionController to get object interfaces @@ -91,6 +94,22 @@ type AnnotationsGetter interface { GetAnnotations() map[string]string } +// ReinvocationContext provides access to the admission related state required to implement the re-invocation policy. +type ReinvocationContext interface { + // IsReinvoke returns true if the current admission check is a re-invocation. + IsReinvoke() bool + // SetIsReinvoke sets the current admission check as a re-invocation. + SetIsReinvoke() + // ShouldReinvoke returns true if any plugin has requested a re-invocation. + ShouldReinvoke() bool + // SetShouldReinvoke signals that a re-invocation is desired. + SetShouldReinvoke() + // AddValue set a value for a plugin name, possibly overriding a previous value. + SetValue(plugin string, v interface{}) + // Value reads a value for a webhook. + Value(plugin string) interface{} +} + // Interface is an abstract, pluggable interface for Admission Control decisions. type Interface interface { // Handles returns true if this admission controller can handle the given operation diff --git a/pkg/admission/plugin/webhook/accessors.go b/pkg/admission/plugin/webhook/accessors.go index 362333c50..108e8ff44 100644 --- a/pkg/admission/plugin/webhook/accessors.go +++ b/pkg/admission/plugin/webhook/accessors.go @@ -23,7 +23,12 @@ import ( // WebhookAccessor provides a common interface to both mutating and validating webhook types. type WebhookAccessor interface { - // GetName gets the webhook Name field. + // GetUID gets a string that uniquely identifies the webhook. + GetUID() string + + // GetName gets the webhook Name field. Note that the name is scoped to the webhook + // configuration and does not provide a globally unique identity, if a unique identity is + // needed, use GetUID. GetName() string // GetClientConfig gets the webhook ClientConfig field. GetClientConfig() v1beta1.WebhookClientConfig @@ -49,14 +54,18 @@ type WebhookAccessor interface { } // NewMutatingWebhookAccessor creates an accessor for a MutatingWebhook. -func NewMutatingWebhookAccessor(h *v1beta1.MutatingWebhook) WebhookAccessor { - return mutatingWebhookAccessor{h} +func NewMutatingWebhookAccessor(uid string, h *v1beta1.MutatingWebhook) WebhookAccessor { + return mutatingWebhookAccessor{uid: uid, MutatingWebhook: h} } type mutatingWebhookAccessor struct { *v1beta1.MutatingWebhook + uid string } +func (m mutatingWebhookAccessor) GetUID() string { + return m.Name +} func (m mutatingWebhookAccessor) GetName() string { return m.Name } @@ -94,14 +103,18 @@ func (m mutatingWebhookAccessor) GetValidatingWebhook() (*v1beta1.ValidatingWebh } // NewValidatingWebhookAccessor creates an accessor for a ValidatingWebhook. -func NewValidatingWebhookAccessor(h *v1beta1.ValidatingWebhook) WebhookAccessor { - return validatingWebhookAccessor{h} +func NewValidatingWebhookAccessor(uid string, h *v1beta1.ValidatingWebhook) WebhookAccessor { + return validatingWebhookAccessor{uid: uid, ValidatingWebhook: h} } type validatingWebhookAccessor struct { *v1beta1.ValidatingWebhook + uid string } +func (v validatingWebhookAccessor) GetUID() string { + return v.uid +} func (v validatingWebhookAccessor) GetName() string { return v.Name } diff --git a/pkg/admission/plugin/webhook/generic/webhook_test.go b/pkg/admission/plugin/webhook/generic/webhook_test.go index c2ddbb7b1..0d6cc9ea1 100644 --- a/pkg/admission/plugin/webhook/generic/webhook_test.go +++ b/pkg/admission/plugin/webhook/generic/webhook_test.go @@ -17,6 +17,7 @@ limitations under the License. package generic import ( + "fmt" "strings" "testing" @@ -277,9 +278,9 @@ func TestShouldCallHook(t *testing.T) { }, } - for _, testcase := range testcases { + for i, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - invocation, err := a.shouldCallHook(webhook.NewValidatingWebhookAccessor(testcase.webhook), testcase.attrs, interfaces) + invocation, err := a.shouldCallHook(webhook.NewValidatingWebhookAccessor(fmt.Sprintf("webhook-%d", i), testcase.webhook), testcase.attrs, interfaces) if err != nil { if len(testcase.expectErr) == 0 { t.Fatal(err) diff --git a/pkg/admission/plugin/webhook/mutating/dispatcher.go b/pkg/admission/plugin/webhook/mutating/dispatcher.go index b206d2a53..f1d8ce819 100644 --- a/pkg/admission/plugin/webhook/mutating/dispatcher.go +++ b/pkg/admission/plugin/webhook/mutating/dispatcher.go @@ -24,6 +24,7 @@ import ( "time" jsonpatch "github.com/evanphx/json-patch" + apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/klog" admissionv1beta1 "k8s.io/api/admission/v1beta1" @@ -56,12 +57,32 @@ func newMutatingDispatcher(p *Plugin) func(cm *webhookutil.ClientManager) generi var _ generic.Dispatcher = &mutatingDispatcher{} func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attributes, o admission.ObjectInterfaces, relevantHooks []*generic.WebhookInvocation) error { + reinvokeCtx := attr.GetReinvocationContext() + var webhookReinvokeCtx *webhookReinvokeContext + if v := reinvokeCtx.Value(PluginName); v != nil { + webhookReinvokeCtx = v.(*webhookReinvokeContext) + } else { + webhookReinvokeCtx = &webhookReinvokeContext{} + reinvokeCtx.SetValue(PluginName, webhookReinvokeCtx) + } + + if reinvokeCtx.IsReinvoke() && webhookReinvokeCtx.IsOutputChangedSinceLastWebhookInvocation(attr.GetObject()) { + // If the object has changed, we know the in-tree plugin re-invocations have mutated the object, + // and we need to reinvoke all eligible webhooks. + webhookReinvokeCtx.RequireReinvokingPreviouslyInvokedPlugins() + } + defer func() { + webhookReinvokeCtx.SetLastWebhookInvocationOutput(attr.GetObject()) + }() var versionedAttr *generic.VersionedAttributes for _, invocation := range relevantHooks { hook, ok := invocation.Webhook.GetMutatingWebhook() if !ok { return fmt.Errorf("mutating webhook dispatch requires v1beta1.MutatingWebhook, but got %T", hook) } + if reinvokeCtx.IsReinvoke() && !webhookReinvokeCtx.ShouldReinvokeWebhook(invocation.Webhook.GetUID()) { + continue + } if versionedAttr == nil { // First webhook, create versioned attributes var err error @@ -76,8 +97,17 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib } t := time.Now() - err := a.callAttrMutatingHook(ctx, hook, invocation, versionedAttr, o) + + changed, err := a.callAttrMutatingHook(ctx, hook, invocation, versionedAttr, o) admissionmetrics.Metrics.ObserveWebhook(time.Since(t), err != nil, versionedAttr.Attributes, "admit", hook.Name) + if changed { + // Patch had changed the object. Prepare to reinvoke all previous webhooks that are eligible for re-invocation. + webhookReinvokeCtx.RequireReinvokingPreviouslyInvokedPlugins() + reinvokeCtx.SetShouldReinvoke() + } + if hook.ReinvocationPolicy != nil && *hook.ReinvocationPolicy == v1beta1.IfNeededReinvocationPolicy { + webhookReinvokeCtx.AddReinvocableWebhookToPreviouslyInvoked(invocation.Webhook.GetUID()) + } if err == nil { continue } @@ -99,32 +129,33 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib if versionedAttr != nil && versionedAttr.VersionedObject != nil && versionedAttr.Dirty { return o.GetObjectConvertor().Convert(versionedAttr.VersionedObject, versionedAttr.Attributes.GetObject(), nil) } + return nil } // note that callAttrMutatingHook updates attr -func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta1.MutatingWebhook, invocation *generic.WebhookInvocation, attr *generic.VersionedAttributes, o admission.ObjectInterfaces) error { +func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta1.MutatingWebhook, invocation *generic.WebhookInvocation, attr *generic.VersionedAttributes, o admission.ObjectInterfaces) (bool, error) { if attr.Attributes.IsDryRun() { if h.SideEffects == nil { - return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook SideEffects is nil")} + return false, &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook SideEffects is nil")} } if !(*h.SideEffects == v1beta1.SideEffectClassNone || *h.SideEffects == v1beta1.SideEffectClassNoneOnDryRun) { - return webhookerrors.NewDryRunUnsupportedErr(h.Name) + return false, webhookerrors.NewDryRunUnsupportedErr(h.Name) } } // Currently dispatcher only supports `v1beta1` AdmissionReview // TODO: Make the dispatcher capable of sending multiple AdmissionReview versions if !util.HasAdmissionReviewVersion(v1beta1.SchemeGroupVersion.Version, invocation.Webhook) { - return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("webhook does not accept v1beta1 AdmissionReview")} + return false, &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("webhook does not accept v1beta1 AdmissionReview")} } // Make the webhook request request := request.CreateAdmissionReview(attr, invocation) client, err := a.cm.HookClient(util.HookClientConfigForWebhook(invocation.Webhook)) if err != nil { - return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: err} + return false, &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: err} } response := &admissionv1beta1.AdmissionReview{} r := client.Post().Context(ctx).Body(&request) @@ -132,11 +163,11 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta r = r.Timeout(time.Duration(*h.TimeoutSeconds) * time.Second) } if err := r.Do().Into(response); err != nil { - return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: err} + return false, &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: err} } if response.Response == nil { - return &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook response was absent")} + return false, &webhookutil.ErrCallingWebhook{WebhookName: h.Name, Reason: fmt.Errorf("Webhook response was absent")} } for k, v := range response.Response.AuditAnnotations { @@ -147,34 +178,34 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta } if !response.Response.Allowed { - return webhookerrors.ToStatusErr(h.Name, response.Response.Result) + return false, webhookerrors.ToStatusErr(h.Name, response.Response.Result) } patchJS := response.Response.Patch if len(patchJS) == 0 { - return nil + return false, nil } patchObj, err := jsonpatch.DecodePatch(patchJS) if err != nil { - return apierrors.NewInternalError(err) + return false, apierrors.NewInternalError(err) } if len(patchObj) == 0 { - return nil + return false, nil } // if a non-empty patch was provided, and we have no object we can apply it to (e.g. a DELETE admission operation), error if attr.VersionedObject == nil { - return apierrors.NewInternalError(fmt.Errorf("admission webhook %q attempted to modify the object, which is not supported for this operation", h.Name)) + return false, apierrors.NewInternalError(fmt.Errorf("admission webhook %q attempted to modify the object, which is not supported for this operation", h.Name)) } jsonSerializer := json.NewSerializer(json.DefaultMetaFactory, o.GetObjectCreater(), o.GetObjectTyper(), false) objJS, err := runtime.Encode(jsonSerializer, attr.VersionedObject) if err != nil { - return apierrors.NewInternalError(err) + return false, apierrors.NewInternalError(err) } patchedJS, err := patchObj.Apply(objJS) if err != nil { - return apierrors.NewInternalError(err) + return false, apierrors.NewInternalError(err) } var newVersionedObject runtime.Object @@ -185,16 +216,20 @@ func (a *mutatingDispatcher) callAttrMutatingHook(ctx context.Context, h *v1beta } else { newVersionedObject, err = o.GetObjectCreater().New(attr.VersionedKind) if err != nil { - return apierrors.NewInternalError(err) + return false, apierrors.NewInternalError(err) } } + // TODO: if we have multiple mutating webhooks, we can remember the json // instead of encoding and decoding for each one. if newVersionedObject, _, err = jsonSerializer.Decode(patchedJS, nil, newVersionedObject); err != nil { - return apierrors.NewInternalError(err) + return false, apierrors.NewInternalError(err) } + + changed := !apiequality.Semantic.DeepEqual(attr.VersionedObject, newVersionedObject) + attr.Dirty = true attr.VersionedObject = newVersionedObject o.GetObjectDefaulter().Default(attr.VersionedObject) - return nil + return changed, nil } diff --git a/pkg/admission/plugin/webhook/mutating/plugin_test.go b/pkg/admission/plugin/webhook/mutating/plugin_test.go index b8f98eedf..b618e3091 100644 --- a/pkg/admission/plugin/webhook/mutating/plugin_test.go +++ b/pkg/admission/plugin/webhook/mutating/plugin_test.go @@ -49,67 +49,77 @@ func TestAdmit(t *testing.T) { webhooktesting.ConvertToMutatingTestCases(webhooktesting.NewNonMutatingTestCases(serverURL))...) for _, tt := range testCases { - wh, err := NewMutatingWebhook(nil) - if err != nil { - t.Errorf("%s: failed to create mutating webhook: %v", tt.Name, err) - continue - } - - ns := "webhook-test" - client, informer := webhooktesting.NewFakeMutatingDataSource(ns, tt.Webhooks, stopCh) - - wh.SetAuthenticationInfoResolverWrapper(webhooktesting.Wrapper(webhooktesting.NewAuthenticationInfoResolver(new(int32)))) - wh.SetServiceResolver(webhooktesting.NewServiceResolver(*serverURL)) - wh.SetExternalKubeClientSet(client) - wh.SetExternalKubeInformerFactory(informer) - - informer.Start(stopCh) - informer.WaitForCacheSync(stopCh) - - if err = wh.ValidateInitialization(); err != nil { - t.Errorf("%s: failed to validate initialization: %v", tt.Name, err) - continue - } - - var attr admission.Attributes - if tt.IsCRD { - attr = webhooktesting.NewAttributeUnstructured(ns, tt.AdditionalLabels, tt.IsDryRun) - } else { - attr = webhooktesting.NewAttribute(ns, tt.AdditionalLabels, tt.IsDryRun) - } - - err = wh.Admit(attr, objectInterfaces) - if tt.ExpectAllow != (err == nil) { - t.Errorf("%s: expected allowed=%v, but got err=%v", tt.Name, tt.ExpectAllow, err) - } - if tt.ExpectLabels != nil { - if !reflect.DeepEqual(tt.ExpectLabels, attr.GetObject().(metav1.Object).GetLabels()) { - t.Errorf("%s: expected labels '%v', but got '%v'", tt.Name, tt.ExpectLabels, attr.GetObject().(metav1.Object).GetLabels()) + t.Run(tt.Name, func(t *testing.T) { + wh, err := NewMutatingWebhook(nil) + if err != nil { + t.Errorf("failed to create mutating webhook: %v", err) + return } - } - // ErrWebhookRejected is not an error for our purposes - if tt.ErrorContains != "" { - if err == nil || !strings.Contains(err.Error(), tt.ErrorContains) { - t.Errorf("%s: expected an error saying %q, but got: %v", tt.Name, tt.ErrorContains, err) + + ns := "webhook-test" + client, informer := webhooktesting.NewFakeMutatingDataSource(ns, tt.Webhooks, stopCh) + + wh.SetAuthenticationInfoResolverWrapper(webhooktesting.Wrapper(webhooktesting.NewAuthenticationInfoResolver(new(int32)))) + wh.SetServiceResolver(webhooktesting.NewServiceResolver(*serverURL)) + wh.SetExternalKubeClientSet(client) + wh.SetExternalKubeInformerFactory(informer) + + informer.Start(stopCh) + informer.WaitForCacheSync(stopCh) + + if err = wh.ValidateInitialization(); err != nil { + t.Errorf("failed to validate initialization: %v", err) + return } - } - if statusErr, isStatusErr := err.(*errors.StatusError); err != nil && !isStatusErr { - t.Errorf("%s: expected a StatusError, got %T", tt.Name, err) - } else if isStatusErr { - if statusErr.ErrStatus.Code != tt.ExpectStatusCode { - t.Errorf("%s: expected status code %d, got %d", tt.Name, tt.ExpectStatusCode, statusErr.ErrStatus.Code) + + var attr admission.Attributes + if tt.IsCRD { + attr = webhooktesting.NewAttributeUnstructured(ns, tt.AdditionalLabels, tt.IsDryRun) + } else { + attr = webhooktesting.NewAttribute(ns, tt.AdditionalLabels, tt.IsDryRun) } - } - fakeAttr, ok := attr.(*webhooktesting.FakeAttributes) - if !ok { - t.Errorf("Unexpected error, failed to convert attr to webhooktesting.FakeAttributes") - continue - } - if len(tt.ExpectAnnotations) == 0 { - assert.Empty(t, fakeAttr.GetAnnotations(), tt.Name+": annotations not set as expected.") - } else { - assert.Equal(t, tt.ExpectAnnotations, fakeAttr.GetAnnotations(), tt.Name+": annotations not set as expected.") - } + + err = wh.Admit(attr, objectInterfaces) + if tt.ExpectAllow != (err == nil) { + t.Errorf("expected allowed=%v, but got err=%v", tt.ExpectAllow, err) + } + if tt.ExpectLabels != nil { + if !reflect.DeepEqual(tt.ExpectLabels, attr.GetObject().(metav1.Object).GetLabels()) { + t.Errorf("expected labels '%v', but got '%v'", tt.ExpectLabels, attr.GetObject().(metav1.Object).GetLabels()) + } + } + // ErrWebhookRejected is not an error for our purposes + if tt.ErrorContains != "" { + if err == nil || !strings.Contains(err.Error(), tt.ErrorContains) { + t.Errorf("expected an error saying %q, but got: %v", tt.ErrorContains, err) + } + } + if statusErr, isStatusErr := err.(*errors.StatusError); err != nil && !isStatusErr { + t.Errorf("expected a StatusError, got %T", err) + } else if isStatusErr { + if statusErr.ErrStatus.Code != tt.ExpectStatusCode { + t.Errorf("expected status code %d, got %d", tt.ExpectStatusCode, statusErr.ErrStatus.Code) + } + } + fakeAttr, ok := attr.(*webhooktesting.FakeAttributes) + if !ok { + t.Errorf("Unexpected error, failed to convert attr to webhooktesting.FakeAttributes") + return + } + if len(tt.ExpectAnnotations) == 0 { + assert.Empty(t, fakeAttr.GetAnnotations(), tt.Name+": annotations not set as expected.") + } else { + assert.Equal(t, tt.ExpectAnnotations, fakeAttr.GetAnnotations(), tt.Name+": annotations not set as expected.") + } + reinvocationCtx := fakeAttr.Attributes.GetReinvocationContext() + reinvocationCtx.SetIsReinvoke() + for webhook, expectReinvoke := range tt.ExpectReinvokeWebhooks { + shouldReinvoke := reinvocationCtx.Value(PluginName).(*webhookReinvokeContext).ShouldReinvokeWebhook(webhook) + if expectReinvoke != shouldReinvoke { + t.Errorf("expected reinvocationContext.ShouldReinvokeWebhook(%s)=%t, but got %t", webhook, expectReinvoke, shouldReinvoke) + } + } + }) } } diff --git a/pkg/admission/plugin/webhook/mutating/reinvocationcontext.go b/pkg/admission/plugin/webhook/mutating/reinvocationcontext.go new file mode 100644 index 000000000..de0af221e --- /dev/null +++ b/pkg/admission/plugin/webhook/mutating/reinvocationcontext.go @@ -0,0 +1,68 @@ +/* +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 mutating + +import ( + apiequality "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" +) + +type webhookReinvokeContext struct { + // lastWebhookOutput holds the result of the last webhook admission plugin call + lastWebhookOutput runtime.Object + // previouslyInvokedReinvocableWebhooks holds the set of webhooks that have been invoked and + // should be reinvoked if a later mutation occurs + previouslyInvokedReinvocableWebhooks sets.String + // reinvokeWebhooks holds the set of webhooks that should be reinvoked + reinvokeWebhooks sets.String +} + +func (rc *webhookReinvokeContext) ShouldReinvokeWebhook(webhook string) bool { + return rc.reinvokeWebhooks.Has(webhook) +} + +func (rc *webhookReinvokeContext) IsOutputChangedSinceLastWebhookInvocation(object runtime.Object) bool { + return !apiequality.Semantic.DeepEqual(rc.lastWebhookOutput, object) +} + +func (rc *webhookReinvokeContext) SetLastWebhookInvocationOutput(object runtime.Object) { + if object == nil { + rc.lastWebhookOutput = nil + return + } + rc.lastWebhookOutput = object.DeepCopyObject() +} + +func (rc *webhookReinvokeContext) AddReinvocableWebhookToPreviouslyInvoked(webhook string) { + if rc.previouslyInvokedReinvocableWebhooks == nil { + rc.previouslyInvokedReinvocableWebhooks = sets.NewString() + } + rc.previouslyInvokedReinvocableWebhooks.Insert(webhook) +} + +func (rc *webhookReinvokeContext) RequireReinvokingPreviouslyInvokedPlugins() { + if len(rc.previouslyInvokedReinvocableWebhooks) > 0 { + if rc.reinvokeWebhooks == nil { + rc.reinvokeWebhooks = sets.NewString() + } + for s := range rc.previouslyInvokedReinvocableWebhooks { + rc.reinvokeWebhooks.Insert(s) + } + rc.previouslyInvokedReinvocableWebhooks = sets.NewString() + } +} diff --git a/pkg/admission/plugin/webhook/namespace/matcher_test.go b/pkg/admission/plugin/webhook/namespace/matcher_test.go index 4633cac8e..7bdf6c450 100644 --- a/pkg/admission/plugin/webhook/namespace/matcher_test.go +++ b/pkg/admission/plugin/webhook/namespace/matcher_test.go @@ -120,7 +120,7 @@ func TestNotExemptClusterScopedResource(t *testing.T) { } attr := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "mock-name", schema.GroupVersionResource{Version: "v1", Resource: "nodes"}, "", admission.Create, &metav1.CreateOptions{}, false, nil) matcher := Matcher{} - matches, err := matcher.MatchNamespaceSelector(webhook.NewValidatingWebhookAccessor(hook), attr) + matches, err := matcher.MatchNamespaceSelector(webhook.NewValidatingWebhookAccessor("mock-hook", hook), attr) if err != nil { t.Fatal(err) } diff --git a/pkg/admission/plugin/webhook/testing/testcase.go b/pkg/admission/plugin/webhook/testing/testcase.go index ec8e6b552..e3046d634 100644 --- a/pkg/admission/plugin/webhook/testing/testcase.go +++ b/pkg/admission/plugin/webhook/testing/testcase.go @@ -49,6 +49,9 @@ var sideEffectsNone = registrationv1beta1.SideEffectClassNone var sideEffectsSome = registrationv1beta1.SideEffectClassSome var sideEffectsNoneOnDryRun = registrationv1beta1.SideEffectClassNoneOnDryRun +var reinvokeNever = registrationv1beta1.NeverReinvocationPolicy +var reinvokeIfNeeded = registrationv1beta1.IfNeededReinvocationPolicy + // NewFakeValidatingDataSource returns a mock client and informer returning the given webhooks. func NewFakeValidatingDataSource(name string, webhooks []registrationv1beta1.ValidatingWebhook, stopCh <-chan struct{}) (clientset kubernetes.Interface, factory informers.SharedInformerFactory) { var objs = []runtime.Object{ @@ -199,39 +202,41 @@ func (c urlConfigGenerator) ccfgURL(urlPath string) registrationv1beta1.WebhookC // ValidatingTest is a validating webhook test case. type ValidatingTest struct { - Name string - Webhooks []registrationv1beta1.ValidatingWebhook - Path string - IsCRD bool - IsDryRun bool - AdditionalLabels map[string]string - ExpectLabels map[string]string - ExpectAllow bool - ErrorContains string - ExpectAnnotations map[string]string - ExpectStatusCode int32 + Name string + Webhooks []registrationv1beta1.ValidatingWebhook + Path string + IsCRD bool + IsDryRun bool + AdditionalLabels map[string]string + ExpectLabels map[string]string + ExpectAllow bool + ErrorContains string + ExpectAnnotations map[string]string + ExpectStatusCode int32 + ExpectReinvokeWebhooks map[string]bool } // MutatingTest is a mutating webhook test case. type MutatingTest struct { - Name string - Webhooks []registrationv1beta1.MutatingWebhook - Path string - IsCRD bool - IsDryRun bool - AdditionalLabels map[string]string - ExpectLabels map[string]string - ExpectAllow bool - ErrorContains string - ExpectAnnotations map[string]string - ExpectStatusCode int32 + Name string + Webhooks []registrationv1beta1.MutatingWebhook + Path string + IsCRD bool + IsDryRun bool + AdditionalLabels map[string]string + ExpectLabels map[string]string + ExpectAllow bool + ErrorContains string + ExpectAnnotations map[string]string + ExpectStatusCode int32 + ExpectReinvokeWebhooks map[string]bool } // ConvertToMutatingTestCases converts a validating test case to a mutating one for test purposes. func ConvertToMutatingTestCases(tests []ValidatingTest) []MutatingTest { r := make([]MutatingTest, len(tests)) for i, t := range tests { - r[i] = MutatingTest{t.Name, ConvertToMutatingWebhooks(t.Webhooks), t.Path, t.IsCRD, t.IsDryRun, t.AdditionalLabels, t.ExpectLabels, t.ExpectAllow, t.ErrorContains, t.ExpectAnnotations, t.ExpectStatusCode} + r[i] = MutatingTest{t.Name, ConvertToMutatingWebhooks(t.Webhooks), t.Path, t.IsCRD, t.IsDryRun, t.AdditionalLabels, t.ExpectLabels, t.ExpectAllow, t.ErrorContains, t.ExpectAnnotations, t.ExpectStatusCode, t.ExpectReinvokeWebhooks} } return r } @@ -240,7 +245,7 @@ func ConvertToMutatingTestCases(tests []ValidatingTest) []MutatingTest { func ConvertToMutatingWebhooks(webhooks []registrationv1beta1.ValidatingWebhook) []registrationv1beta1.MutatingWebhook { mutating := make([]registrationv1beta1.MutatingWebhook, len(webhooks)) for i, h := range webhooks { - mutating[i] = registrationv1beta1.MutatingWebhook{h.Name, h.ClientConfig, h.Rules, h.FailurePolicy, h.MatchPolicy, h.NamespaceSelector, h.SideEffects, h.TimeoutSeconds, h.AdmissionReviewVersions} + mutating[i] = registrationv1beta1.MutatingWebhook{h.Name, h.ClientConfig, h.Rules, h.FailurePolicy, h.MatchPolicy, h.NamespaceSelector, h.SideEffects, h.TimeoutSeconds, h.AdmissionReviewVersions, nil} } return mutating } @@ -639,6 +644,63 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { }, // No need to test everything with the url case, since only the // connection is different. + { + Name: "match & reinvoke if needed policy", + Webhooks: []registrationv1beta1.MutatingWebhook{{ + Name: "addLabel", + ClientConfig: ccfgSVC("addLabel"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + ReinvocationPolicy: &reinvokeIfNeeded, + }, { + Name: "removeLabel", + ClientConfig: ccfgSVC("removeLabel"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + ReinvocationPolicy: &reinvokeIfNeeded, + }}, + AdditionalLabels: map[string]string{"remove": "me"}, + ExpectAllow: true, + ExpectReinvokeWebhooks: map[string]bool{"addLabel": true}, + }, + { + Name: "match & never reinvoke policy", + Webhooks: []registrationv1beta1.MutatingWebhook{{ + Name: "addLabel", + ClientConfig: ccfgSVC("addLabel"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + ReinvocationPolicy: &reinvokeNever, + }}, + ExpectAllow: true, + ExpectReinvokeWebhooks: map[string]bool{"addLabel": false}, + }, + { + Name: "match & never reinvoke policy (by default)", + Webhooks: []registrationv1beta1.MutatingWebhook{{ + Name: "addLabel", + ClientConfig: ccfgSVC("addLabel"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectAllow: true, + ExpectReinvokeWebhooks: map[string]bool{"addLabel": false}, + }, + { + Name: "match & no reinvoke", + Webhooks: []registrationv1beta1.MutatingWebhook{{ + Name: "noop", + ClientConfig: ccfgSVC("noop"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectAllow: true, + }, } } diff --git a/pkg/admission/plugin/webhook/testing/webhook_server.go b/pkg/admission/plugin/webhook/testing/webhook_server.go index 5d080745d..193571cd6 100644 --- a/pkg/admission/plugin/webhook/testing/webhook_server.go +++ b/pkg/admission/plugin/webhook/testing/webhook_server.go @@ -138,6 +138,13 @@ func webhookHandler(w http.ResponseWriter, r *http.Request) { }, }, }) + case "/noop": + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{ + Response: &v1beta1.AdmissionResponse{ + Allowed: true, + }, + }) default: http.NotFound(w, r) } diff --git a/pkg/admission/plugins.go b/pkg/admission/plugins.go index bdf087e56..d37af509c 100644 --- a/pkg/admission/plugins.go +++ b/pkg/admission/plugins.go @@ -160,7 +160,7 @@ func (ps *Plugins) NewFromPlugins(pluginNames []string, configProvider ConfigPro if len(validationPlugins) != 0 { klog.Infof("Loaded %d validating admission controller(s) successfully in the following order: %s.", len(validationPlugins), strings.Join(validationPlugins, ",")) } - return chainAdmissionHandler(handlers), nil + return newReinvocationHandler(chainAdmissionHandler(handlers)), nil } // InitPlugin creates an instance of the named interface. diff --git a/pkg/admission/reinvocation.go b/pkg/admission/reinvocation.go new file mode 100644 index 000000000..b99e604e0 --- /dev/null +++ b/pkg/admission/reinvocation.go @@ -0,0 +1,62 @@ +/* +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 + +// newReinvocationHandler creates a handler that wraps the provided admission chain and reinvokes it +// if needed according to re-invocation policy of the webhooks. +func newReinvocationHandler(admissionChain Interface) Interface { + return &reinvoker{admissionChain} +} + +type reinvoker struct { + admissionChain Interface +} + +// Admit performs an admission control check using the wrapped admission chain, reinvoking the +// admission chain if needed according to the reinvocation policy. Plugins are expected to check +// the admission attributes' reinvocation context against their reinvocation policy to decide if +// they should re-run, and to update the reinvocation context if they perform any mutations. +func (r *reinvoker) Admit(a Attributes, o ObjectInterfaces) error { + if mutator, ok := r.admissionChain.(MutationInterface); ok { + err := mutator.Admit(a, o) + if err != nil { + return err + } + s := a.GetReinvocationContext() + if s.ShouldReinvoke() { + s.SetIsReinvoke() + // Calling admit a second time will reinvoke all in-tree plugins + // as well as any webhook plugins that need to be reinvoked based on the + // reinvocation policy. + return mutator.Admit(a, o) + } + } + return nil +} + +// Validate performs an admission control check using the wrapped admission chain, and returns immediately on first error. +func (r *reinvoker) Validate(a Attributes, o ObjectInterfaces) error { + if validator, ok := r.admissionChain.(ValidationInterface); ok { + return validator.Validate(a, o) + } + return nil +} + +// Handles will return true if any of the admission chain handlers handle the given operation. +func (r *reinvoker) Handles(operation Operation) bool { + return r.admissionChain.Handles(operation) +} From 8658264258b21af05745d63bfb4754ae06d4ec37 Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Wed, 29 May 2019 15:56:52 -0700 Subject: [PATCH 4/5] object matcher Kubernetes-commit: 6cf499db6c1dd464c6072706106dec6c5284dff7 --- pkg/admission/plugin/webhook/accessors.go | 8 ++ .../plugin/webhook/generic/interfaces.go | 17 ++- .../plugin/webhook/generic/webhook.go | 30 ++-- .../plugin/webhook/generic/webhook_test.go | 17 ++- .../plugin/webhook/mutating/dispatcher.go | 22 ++- pkg/admission/plugin/webhook/object/doc.go | 20 +++ .../plugin/webhook/object/matcher.go | 59 ++++++++ .../plugin/webhook/object/matcher_test.go | 130 ++++++++++++++++++ .../plugin/webhook/testing/testcase.go | 61 +++++++- .../plugin/webhook/testing/webhook_server.go | 11 ++ .../plugin/webhook/validating/dispatcher.go | 36 +++-- .../plugin/webhook/validating/plugin.go | 6 +- 12 files changed, 379 insertions(+), 38 deletions(-) create mode 100644 pkg/admission/plugin/webhook/object/doc.go create mode 100644 pkg/admission/plugin/webhook/object/matcher.go create mode 100644 pkg/admission/plugin/webhook/object/matcher_test.go diff --git a/pkg/admission/plugin/webhook/accessors.go b/pkg/admission/plugin/webhook/accessors.go index 108e8ff44..b44c72ebf 100644 --- a/pkg/admission/plugin/webhook/accessors.go +++ b/pkg/admission/plugin/webhook/accessors.go @@ -40,6 +40,8 @@ type WebhookAccessor interface { GetMatchPolicy() *v1beta1.MatchPolicyType // GetNamespaceSelector gets the webhook NamespaceSelector field. GetNamespaceSelector() *metav1.LabelSelector + // GetObjectSelector gets the webhook ObjectSelector field. + GetObjectSelector() *metav1.LabelSelector // GetSideEffects gets the webhook SideEffects field. GetSideEffects() *v1beta1.SideEffectClass // GetTimeoutSeconds gets the webhook TimeoutSeconds field. @@ -84,6 +86,9 @@ func (m mutatingWebhookAccessor) GetMatchPolicy() *v1beta1.MatchPolicyType { func (m mutatingWebhookAccessor) GetNamespaceSelector() *metav1.LabelSelector { return m.NamespaceSelector } +func (m mutatingWebhookAccessor) GetObjectSelector() *metav1.LabelSelector { + return m.ObjectSelector +} func (m mutatingWebhookAccessor) GetSideEffects() *v1beta1.SideEffectClass { return m.SideEffects } @@ -133,6 +138,9 @@ func (v validatingWebhookAccessor) GetMatchPolicy() *v1beta1.MatchPolicyType { func (v validatingWebhookAccessor) GetNamespaceSelector() *metav1.LabelSelector { return v.NamespaceSelector } +func (v validatingWebhookAccessor) GetObjectSelector() *metav1.LabelSelector { + return v.ObjectSelector +} func (v validatingWebhookAccessor) GetSideEffects() *v1beta1.SideEffectClass { return v.SideEffects } diff --git a/pkg/admission/plugin/webhook/generic/interfaces.go b/pkg/admission/plugin/webhook/generic/interfaces.go index 227502de8..4381691ef 100644 --- a/pkg/admission/plugin/webhook/generic/interfaces.go +++ b/pkg/admission/plugin/webhook/generic/interfaces.go @@ -35,7 +35,7 @@ type Source interface { // variants of the object and old object. type VersionedAttributes struct { // Attributes holds the original admission attributes - Attributes admission.Attributes + admission.Attributes // VersionedOldObject holds Attributes.OldObject (if non-nil), converted to VersionedKind. // It must never be mutated. VersionedOldObject runtime.Object @@ -48,6 +48,14 @@ type VersionedAttributes struct { Dirty bool } +// GetObject overrides the Attributes.GetObject() +func (v *VersionedAttributes) GetObject() runtime.Object { + if v.VersionedObject != nil { + return v.VersionedObject + } + return v.Attributes.GetObject() +} + // WebhookInvocation describes how to call a webhook, including the resource and subresource the webhook registered for, // and the kind that should be sent to the webhook. type WebhookInvocation struct { @@ -59,6 +67,9 @@ type WebhookInvocation struct { // Dispatcher dispatches webhook call to a list of webhooks with admission attributes as argument. type Dispatcher interface { - // Dispatch a request to the webhooks using the given webhooks. A non-nil error means the request is rejected. - Dispatch(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces, hooks []*WebhookInvocation) error + // Dispatch a request to the webhooks. Dispatcher may choose not to + // call a hook, either because the rules of the hook does not match, or + // the namespaceSelector or the objectSelector of the hook does not + // match. A non-nil error means the request is rejected. + Dispatch(ctx context.Context, a admission.Attributes, o admission.ObjectInterfaces, hooks []webhook.WebhookAccessor) error } diff --git a/pkg/admission/plugin/webhook/generic/webhook.go b/pkg/admission/plugin/webhook/generic/webhook.go index 99415292c..e88125ff5 100644 --- a/pkg/admission/plugin/webhook/generic/webhook.go +++ b/pkg/admission/plugin/webhook/generic/webhook.go @@ -30,6 +30,7 @@ import ( "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/apiserver/pkg/admission/plugin/webhook/config" "k8s.io/apiserver/pkg/admission/plugin/webhook/namespace" + "k8s.io/apiserver/pkg/admission/plugin/webhook/object" "k8s.io/apiserver/pkg/admission/plugin/webhook/rules" webhookutil "k8s.io/apiserver/pkg/util/webhook" "k8s.io/client-go/informers" @@ -45,6 +46,7 @@ type Webhook struct { hookSource Source clientManager *webhookutil.ClientManager namespaceMatcher *namespace.Matcher + objectMatcher *object.Matcher dispatcher Dispatcher } @@ -80,6 +82,7 @@ func NewWebhook(handler *admission.Handler, configFile io.Reader, sourceFactory sourceFactory: sourceFactory, clientManager: &cm, namespaceMatcher: &namespace.Matcher{}, + objectMatcher: &object.Matcher{}, dispatcher: dispatcherFactory(&cm), }, nil } @@ -127,9 +130,9 @@ func (a *Webhook) ValidateInitialization() error { return nil } -// shouldCallHook returns invocation details if the webhook should be called, nil if the webhook should not be called, +// ShouldCallHook returns invocation details if the webhook should be called, nil if the webhook should not be called, // or an error if an error was encountered during evaluation. -func (a *Webhook) shouldCallHook(h webhook.WebhookAccessor, attr admission.Attributes, o admission.ObjectInterfaces) (*WebhookInvocation, *apierrors.StatusError) { +func (a *Webhook) ShouldCallHook(h webhook.WebhookAccessor, attr admission.Attributes, o admission.ObjectInterfaces) (*WebhookInvocation, *apierrors.StatusError) { var err *apierrors.StatusError var invocation *WebhookInvocation for _, r := range h.GetRules() { @@ -184,6 +187,11 @@ func (a *Webhook) shouldCallHook(h webhook.WebhookAccessor, attr admission.Attri return nil, err } + matches, err = a.objectMatcher.MatchObjectSelector(h, attr) + if !matches || err != nil { + return nil, err + } + return invocation, nil } @@ -206,21 +214,5 @@ func (a *Webhook) Dispatch(attr admission.Attributes, o admission.ObjectInterfac // TODO: Figure out if adding one second timeout make sense here. ctx := context.TODO() - var relevantHooks []*WebhookInvocation - for i := range hooks { - invocation, err := a.shouldCallHook(hooks[i], attr, o) - if err != nil { - return err - } - if invocation != nil { - relevantHooks = append(relevantHooks, invocation) - } - } - - if len(relevantHooks) == 0 { - // no matching hooks - return nil - } - - return a.dispatcher.Dispatch(ctx, attr, o, relevantHooks) + return a.dispatcher.Dispatch(ctx, attr, o, hooks) } diff --git a/pkg/admission/plugin/webhook/generic/webhook_test.go b/pkg/admission/plugin/webhook/generic/webhook_test.go index 0d6cc9ea1..ad7fc7896 100644 --- a/pkg/admission/plugin/webhook/generic/webhook_test.go +++ b/pkg/admission/plugin/webhook/generic/webhook_test.go @@ -28,10 +28,11 @@ import ( "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/apiserver/pkg/admission/plugin/webhook/namespace" + "k8s.io/apiserver/pkg/admission/plugin/webhook/object" ) func TestShouldCallHook(t *testing.T) { - a := &Webhook{namespaceMatcher: &namespace.Matcher{}} + a := &Webhook{namespaceMatcher: &namespace.Matcher{}, objectMatcher: &object.Matcher{}} allScopes := v1beta1.AllScopes exactMatch := v1beta1.Exact @@ -82,6 +83,7 @@ func TestShouldCallHook(t *testing.T) { name: "invalid kind lookup", webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, MatchPolicy: &equivalentMatch, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, @@ -95,6 +97,7 @@ func TestShouldCallHook(t *testing.T) { name: "wildcard rule, match as requested", webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"*"}, APIVersions: []string{"*"}, Resources: []string{"*"}, Scope: &allScopes}, @@ -109,6 +112,7 @@ func TestShouldCallHook(t *testing.T) { name: "specific rules, prefer exact match", webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"extensions"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments"}, Scope: &allScopes}, @@ -129,6 +133,7 @@ func TestShouldCallHook(t *testing.T) { name: "specific rules, match miss", webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"extensions"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments"}, Scope: &allScopes}, @@ -144,6 +149,7 @@ func TestShouldCallHook(t *testing.T) { webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &exactMatch, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"extensions"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments"}, Scope: &allScopes}, @@ -159,6 +165,7 @@ func TestShouldCallHook(t *testing.T) { webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &equivalentMatch, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"extensions"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments"}, Scope: &allScopes}, @@ -177,6 +184,7 @@ func TestShouldCallHook(t *testing.T) { webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &equivalentMatch, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"apps"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments"}, Scope: &allScopes}, @@ -195,6 +203,7 @@ func TestShouldCallHook(t *testing.T) { name: "specific rules, subresource prefer exact match", webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"extensions"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments", "deployments/scale"}, Scope: &allScopes}, @@ -215,6 +224,7 @@ func TestShouldCallHook(t *testing.T) { name: "specific rules, subresource match miss", webhook: &v1beta1.ValidatingWebhook{ NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"extensions"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments", "deployments/scale"}, Scope: &allScopes}, @@ -230,6 +240,7 @@ func TestShouldCallHook(t *testing.T) { webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &exactMatch, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"extensions"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments", "deployments/scale"}, Scope: &allScopes}, @@ -245,6 +256,7 @@ func TestShouldCallHook(t *testing.T) { webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &equivalentMatch, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"extensions"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments", "deployments/scale"}, Scope: &allScopes}, @@ -263,6 +275,7 @@ func TestShouldCallHook(t *testing.T) { webhook: &v1beta1.ValidatingWebhook{ MatchPolicy: &equivalentMatch, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: []v1beta1.RuleWithOperations{{ Operations: []v1beta1.OperationType{"*"}, Rule: v1beta1.Rule{APIGroups: []string{"apps"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments", "deployments/scale"}, Scope: &allScopes}, @@ -280,7 +293,7 @@ func TestShouldCallHook(t *testing.T) { for i, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - invocation, err := a.shouldCallHook(webhook.NewValidatingWebhookAccessor(fmt.Sprintf("webhook-%d", i), testcase.webhook), testcase.attrs, interfaces) + invocation, err := a.ShouldCallHook(webhook.NewValidatingWebhookAccessor(fmt.Sprintf("webhook-%d", i), testcase.webhook), testcase.attrs, interfaces) if err != nil { if len(testcase.expectErr) == 0 { t.Fatal(err) diff --git a/pkg/admission/plugin/webhook/mutating/dispatcher.go b/pkg/admission/plugin/webhook/mutating/dispatcher.go index f1d8ce819..5c5c41aed 100644 --- a/pkg/admission/plugin/webhook/mutating/dispatcher.go +++ b/pkg/admission/plugin/webhook/mutating/dispatcher.go @@ -36,6 +36,7 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/admission" admissionmetrics "k8s.io/apiserver/pkg/admission/metrics" + "k8s.io/apiserver/pkg/admission/plugin/webhook" webhookerrors "k8s.io/apiserver/pkg/admission/plugin/webhook/errors" "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" "k8s.io/apiserver/pkg/admission/plugin/webhook/request" @@ -56,7 +57,7 @@ func newMutatingDispatcher(p *Plugin) func(cm *webhookutil.ClientManager) generi var _ generic.Dispatcher = &mutatingDispatcher{} -func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attributes, o admission.ObjectInterfaces, relevantHooks []*generic.WebhookInvocation) error { +func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attributes, o admission.ObjectInterfaces, hooks []webhook.WebhookAccessor) error { reinvokeCtx := attr.GetReinvocationContext() var webhookReinvokeCtx *webhookReinvokeContext if v := reinvokeCtx.Value(PluginName); v != nil { @@ -75,14 +76,31 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib webhookReinvokeCtx.SetLastWebhookInvocationOutput(attr.GetObject()) }() var versionedAttr *generic.VersionedAttributes - for _, invocation := range relevantHooks { + for _, hook := range hooks { + attrForCheck := attr + if versionedAttr != nil { + attrForCheck = versionedAttr + } + invocation, statusErr := a.plugin.ShouldCallHook(hook, attrForCheck, o) + if statusErr != nil { + return statusErr + } + if invocation == nil { + continue + } hook, ok := invocation.Webhook.GetMutatingWebhook() if !ok { return fmt.Errorf("mutating webhook dispatch requires v1beta1.MutatingWebhook, but got %T", hook) } + // This means that during reinvocation, a webhook will not be + // called for the first time. For example, if the webhook is + // skipped in the first round because of mismatching labels, + // even if the labels become matching, the webhook does not + // get called during reinvocation. if reinvokeCtx.IsReinvoke() && !webhookReinvokeCtx.ShouldReinvokeWebhook(invocation.Webhook.GetUID()) { continue } + if versionedAttr == nil { // First webhook, create versioned attributes var err error diff --git a/pkg/admission/plugin/webhook/object/doc.go b/pkg/admission/plugin/webhook/object/doc.go new file mode 100644 index 000000000..93c473440 --- /dev/null +++ b/pkg/admission/plugin/webhook/object/doc.go @@ -0,0 +1,20 @@ +/* +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 object defines the utilities that are used by the webhook plugin to +// decide if a webhook should run, as long as either the old object or the new +// object has labels matching the webhook config's objectSelector. +package object // import "k8s.io/apiserver/pkg/admission/plugin/webhook/object" diff --git a/pkg/admission/plugin/webhook/object/matcher.go b/pkg/admission/plugin/webhook/object/matcher.go new file mode 100644 index 000000000..be341dd95 --- /dev/null +++ b/pkg/admission/plugin/webhook/object/matcher.go @@ -0,0 +1,59 @@ +/* +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 object + +import ( + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/webhook" + "k8s.io/klog" +) + +// Matcher decides if a request selected by the ObjectSelector. +type Matcher struct { +} + +func matchObject(obj runtime.Object, selector labels.Selector) bool { + if obj == nil { + return false + } + accessor, err := meta.Accessor(obj) + if err != nil { + klog.V(5).Infof("cannot access metadata of %v: %v", obj, err) + return false + } + return selector.Matches(labels.Set(accessor.GetLabels())) + +} + +// MatchObjectSelector decideds whether the request matches the ObjectSelector +// of the webhook. Only when they match, the webhook is called. +func (m *Matcher) MatchObjectSelector(h webhook.WebhookAccessor, attr admission.Attributes) (bool, *apierrors.StatusError) { + // TODO: adding an LRU cache to cache the translation + selector, err := metav1.LabelSelectorAsSelector(h.GetObjectSelector()) + if err != nil { + return false, apierrors.NewInternalError(err) + } + if selector.Empty() { + return true, nil + } + return matchObject(attr.GetObject(), selector) || matchObject(attr.GetOldObject(), selector), nil +} diff --git a/pkg/admission/plugin/webhook/object/matcher_test.go b/pkg/admission/plugin/webhook/object/matcher_test.go new file mode 100644 index 000000000..823fabc96 --- /dev/null +++ b/pkg/admission/plugin/webhook/object/matcher_test.go @@ -0,0 +1,130 @@ +/* +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 object + +import ( + "testing" + + "k8s.io/api/admissionregistration/v1beta1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/webhook" +) + +func TestObjectSelector(t *testing.T) { + nodeLevel1 := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "runlevel": "1", + }, + }, + } + nodeLevel2 := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "runlevel": "2", + }, + }, + } + runLevel1Excluder := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "runlevel", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"1"}, + }, + }, + } + matcher := &Matcher{} + allScopes := v1beta1.AllScopes + testcases := []struct { + name string + + objectSelector *metav1.LabelSelector + attrs admission.Attributes + + expectCall bool + }{ + { + name: "empty object selector matches everything", + objectSelector: &metav1.LabelSelector{}, + attrs: admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "", "name", schema.GroupVersionResource{}, "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: true, + }, + { + name: "matches new object", + objectSelector: runLevel1Excluder, + attrs: admission.NewAttributesRecord(nodeLevel2, nil, schema.GroupVersionKind{}, "", "name", schema.GroupVersionResource{}, "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: true, + }, + { + name: "matches old object", + objectSelector: runLevel1Excluder, + attrs: admission.NewAttributesRecord(nil, nodeLevel2, schema.GroupVersionKind{}, "", "name", schema.GroupVersionResource{}, "", admission.Delete, &metav1.DeleteOptions{}, false, nil), + expectCall: true, + }, + { + name: "does not match new object", + objectSelector: runLevel1Excluder, + attrs: admission.NewAttributesRecord(nodeLevel1, nil, schema.GroupVersionKind{}, "", "name", schema.GroupVersionResource{}, "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + }, + { + name: "does not match old object", + objectSelector: runLevel1Excluder, + attrs: admission.NewAttributesRecord(nil, nodeLevel1, schema.GroupVersionKind{}, "", "name", schema.GroupVersionResource{}, "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + }, + { + name: "does not match object that does not implement Object interface", + objectSelector: runLevel1Excluder, + attrs: admission.NewAttributesRecord(&corev1.NodeProxyOptions{}, nil, schema.GroupVersionKind{}, "", "name", schema.GroupVersionResource{}, "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + }, + { + name: "empty selector matches everything, including object that does not implement Object interface", + objectSelector: &metav1.LabelSelector{}, + attrs: admission.NewAttributesRecord(&corev1.NodeProxyOptions{}, nil, schema.GroupVersionKind{}, "", "name", schema.GroupVersionResource{}, "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: true, + }, + } + + for _, testcase := range testcases { + hook := &v1beta1.ValidatingWebhook{ + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: testcase.objectSelector, + Rules: []v1beta1.RuleWithOperations{{ + Operations: []v1beta1.OperationType{"*"}, + Rule: v1beta1.Rule{APIGroups: []string{"*"}, APIVersions: []string{"*"}, Resources: []string{"*"}, Scope: &allScopes}, + }}} + + t.Run(testcase.name, func(t *testing.T) { + match, err := matcher.MatchObjectSelector(webhook.NewValidatingWebhookAccessor("mock-hook", hook), testcase.attrs) + if err != nil { + t.Error(err) + } + if testcase.expectCall && !match { + t.Errorf("expected the webhook to be called") + } + if !testcase.expectCall && match { + t.Errorf("expected the webhook to be called") + } + }) + } +} diff --git a/pkg/admission/plugin/webhook/testing/testcase.go b/pkg/admission/plugin/webhook/testing/testcase.go index e3046d634..4b53c4684 100644 --- a/pkg/admission/plugin/webhook/testing/testcase.go +++ b/pkg/admission/plugin/webhook/testing/testcase.go @@ -245,7 +245,7 @@ func ConvertToMutatingTestCases(tests []ValidatingTest) []MutatingTest { func ConvertToMutatingWebhooks(webhooks []registrationv1beta1.ValidatingWebhook) []registrationv1beta1.MutatingWebhook { mutating := make([]registrationv1beta1.MutatingWebhook, len(webhooks)) for i, h := range webhooks { - mutating[i] = registrationv1beta1.MutatingWebhook{h.Name, h.ClientConfig, h.Rules, h.FailurePolicy, h.MatchPolicy, h.NamespaceSelector, h.SideEffects, h.TimeoutSeconds, h.AdmissionReviewVersions, nil} + mutating[i] = registrationv1beta1.MutatingWebhook{h.Name, h.ClientConfig, h.Rules, h.FailurePolicy, h.MatchPolicy, h.NamespaceSelector, h.ObjectSelector, h.SideEffects, h.TimeoutSeconds, h.AdmissionReviewVersions, nil} } return mutating } @@ -552,6 +552,30 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { }}, ExpectAllow: true, }, + { + Name: "skip webhook whose objectSelector does not match", + Webhooks: []registrationv1beta1.ValidatingWebhook{{ + Name: "allow.example.com", + ClientConfig: ccfgSVC("allow"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + AdmissionReviewVersions: []string{"v1beta1"}, + }, { + Name: "shouldNotBeCalled", + ClientConfig: ccfgSVC("shouldNotBeCalled"), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "label": "nonexistent", + }, + }, + Rules: matchEverythingRules, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectAllow: true, + ExpectAnnotations: map[string]string{"allow.example.com/key1": "value1"}, + }, // No need to test everything with the url case, since only the // connection is different. } @@ -642,6 +666,36 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ExpectStatusCode: http.StatusBadRequest, ErrorContains: "does not support dry run", }, + { + Name: "first webhook remove labels, second webhook shouldn't be called", + Webhooks: []registrationv1beta1.MutatingWebhook{{ + Name: "removelabel.example.com", + ClientConfig: ccfgSVC("removeLabel"), + Rules: matchEverythingRules, + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "remove": "me", + }, + }, + AdmissionReviewVersions: []string{"v1beta1"}, + }, { + Name: "shouldNotBeCalled", + ClientConfig: ccfgSVC("shouldNotBeCalled"), + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "remove": "me", + }, + }, + Rules: matchEverythingRules, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + ExpectAllow: true, + AdditionalLabels: map[string]string{"remove": "me"}, + ExpectLabels: map[string]string{"pod.name": "my-pod"}, + ExpectAnnotations: map[string]string{"removelabel.example.com/key1": "value1"}, + }, // No need to test everything with the url case, since only the // connection is different. { @@ -651,6 +705,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("addLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, ReinvocationPolicy: &reinvokeIfNeeded, }, { @@ -658,6 +713,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("removeLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, ReinvocationPolicy: &reinvokeIfNeeded, }}, @@ -672,6 +728,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("addLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, ReinvocationPolicy: &reinvokeNever, }}, @@ -685,6 +742,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("addLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -697,6 +755,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("noop"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, diff --git a/pkg/admission/plugin/webhook/testing/webhook_server.go b/pkg/admission/plugin/webhook/testing/webhook_server.go index 193571cd6..6a9ee5ff1 100644 --- a/pkg/admission/plugin/webhook/testing/webhook_server.go +++ b/pkg/admission/plugin/webhook/testing/webhook_server.go @@ -82,6 +82,17 @@ func webhookHandler(w http.ResponseWriter, r *http.Request) { }, }, }) + case "/shouldNotBeCalled": + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{ + Response: &v1beta1.AdmissionResponse{ + Allowed: false, + Result: &metav1.Status{ + Message: "doesn't expect labels to match object selector", + Code: http.StatusForbidden, + }, + }, + }) case "/allow": w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{ diff --git a/pkg/admission/plugin/webhook/validating/dispatcher.go b/pkg/admission/plugin/webhook/validating/dispatcher.go index 0505103d2..dda52c90e 100644 --- a/pkg/admission/plugin/webhook/validating/dispatcher.go +++ b/pkg/admission/plugin/webhook/validating/dispatcher.go @@ -29,6 +29,7 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/admission" admissionmetrics "k8s.io/apiserver/pkg/admission/metrics" + "k8s.io/apiserver/pkg/admission/plugin/webhook" webhookerrors "k8s.io/apiserver/pkg/admission/plugin/webhook/errors" "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" "k8s.io/apiserver/pkg/admission/plugin/webhook/request" @@ -38,28 +39,45 @@ import ( ) type validatingDispatcher struct { - cm *webhookutil.ClientManager + cm *webhookutil.ClientManager + plugin *Plugin } -func newValidatingDispatcher(cm *webhookutil.ClientManager) generic.Dispatcher { - return &validatingDispatcher{cm} +func newValidatingDispatcher(p *Plugin) func(cm *webhookutil.ClientManager) generic.Dispatcher { + return func(cm *webhookutil.ClientManager) generic.Dispatcher { + return &validatingDispatcher{cm, p} + } } var _ generic.Dispatcher = &validatingDispatcher{} -func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attributes, o admission.ObjectInterfaces, relevantHooks []*generic.WebhookInvocation) error { +func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attributes, o admission.ObjectInterfaces, hooks []webhook.WebhookAccessor) error { + var relevantHooks []*generic.WebhookInvocation // Construct all the versions we need to call our webhooks versionedAttrs := map[schema.GroupVersionKind]*generic.VersionedAttributes{} - for _, call := range relevantHooks { - // If we already have this version, continue - if _, ok := versionedAttrs[call.Kind]; ok { + for _, hook := range hooks { + invocation, statusError := d.plugin.ShouldCallHook(hook, attr, o) + if statusError != nil { + return statusError + } + if invocation == nil { continue } - versionedAttr, err := generic.NewVersionedAttributes(attr, call.Kind, o) + relevantHooks = append(relevantHooks, invocation) + // If we already have this version, continue + if _, ok := versionedAttrs[invocation.Kind]; ok { + continue + } + versionedAttr, err := generic.NewVersionedAttributes(attr, invocation.Kind, o) if err != nil { return apierrors.NewInternalError(err) } - versionedAttrs[call.Kind] = versionedAttr + versionedAttrs[invocation.Kind] = versionedAttr + } + + if len(relevantHooks) == 0 { + // no matching hooks + return nil } wg := sync.WaitGroup{} diff --git a/pkg/admission/plugin/webhook/validating/plugin.go b/pkg/admission/plugin/webhook/validating/plugin.go index 388a237c9..30e5c9d33 100644 --- a/pkg/admission/plugin/webhook/validating/plugin.go +++ b/pkg/admission/plugin/webhook/validating/plugin.go @@ -51,11 +51,13 @@ var _ admission.ValidationInterface = &Plugin{} // NewValidatingAdmissionWebhook returns a generic admission webhook plugin. func NewValidatingAdmissionWebhook(configFile io.Reader) (*Plugin, error) { handler := admission.NewHandler(admission.Connect, admission.Create, admission.Delete, admission.Update) - webhook, err := generic.NewWebhook(handler, configFile, configuration.NewValidatingWebhookConfigurationManager, newValidatingDispatcher) + p := &Plugin{} + var err error + p.Webhook, err = generic.NewWebhook(handler, configFile, configuration.NewValidatingWebhookConfigurationManager, newValidatingDispatcher(p)) if err != nil { return nil, err } - return &Plugin{webhook}, nil + return p, nil } // Validate makes an admission decision based on the request attributes. From ec622aa8bd65601b4143c9403d9baed30ee9260a Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Wed, 29 May 2019 17:20:43 -0700 Subject: [PATCH 5/5] minor changes, propagating interface changes Kubernetes-commit: 7738c7ee8fbbaa79aed2ca221141a6b3b4f826be --- .../plugin/webhook/testing/testcase.go | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/pkg/admission/plugin/webhook/testing/testcase.go b/pkg/admission/plugin/webhook/testing/testcase.go index 4b53c4684..fd2d5a0d7 100644 --- a/pkg/admission/plugin/webhook/testing/testcase.go +++ b/pkg/admission/plugin/webhook/testing/testcase.go @@ -269,6 +269,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { Operations: []registrationv1beta1.OperationType{registrationv1beta1.Create}, }}, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -280,6 +281,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -292,6 +294,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("disallow"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectStatusCode: http.StatusForbidden, @@ -304,6 +307,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("disallowReason"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectStatusCode: http.StatusForbidden, @@ -322,6 +326,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { Operator: metav1.LabelSelectorOpIn, }}, }, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -340,6 +345,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { Operator: metav1.LabelSelectorOpNotIn, }}, }, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -351,6 +357,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("internalErr"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyIgnore, AdmissionReviewVersions: []string{"v1beta1"}, }, { @@ -358,6 +365,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("internalErr"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyIgnore, AdmissionReviewVersions: []string{"v1beta1"}, }, { @@ -365,6 +373,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("internalErr"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyIgnore, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -377,18 +386,21 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { Name: "internalErr A", ClientConfig: ccfgSVC("internalErr"), NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: matchEverythingRules, AdmissionReviewVersions: []string{"v1beta1"}, }, { Name: "internalErr B", ClientConfig: ccfgSVC("internalErr"), NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: matchEverythingRules, AdmissionReviewVersions: []string{"v1beta1"}, }, { Name: "internalErr C", ClientConfig: ccfgSVC("internalErr"), NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, Rules: matchEverythingRules, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -402,6 +414,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("internalErr"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyFail, AdmissionReviewVersions: []string{"v1beta1"}, }, { @@ -409,6 +422,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("internalErr"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyFail, AdmissionReviewVersions: []string{"v1beta1"}, }, { @@ -416,6 +430,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("internalErr"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyFail, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -429,6 +444,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgURL("allow"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -441,6 +457,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgURL("disallow"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectStatusCode: http.StatusForbidden, @@ -453,6 +470,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { FailurePolicy: &policyIgnore, Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -465,6 +483,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { FailurePolicy: &policyFail, Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectStatusCode: http.StatusInternalServerError, @@ -479,6 +498,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { Operations: []registrationv1beta1.OperationType{registrationv1beta1.Create}, }}, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, SideEffects: &sideEffectsSome, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -492,6 +512,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, SideEffects: &sideEffectsUnknown, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -506,6 +527,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, SideEffects: &sideEffectsNone, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -520,6 +542,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, SideEffects: &sideEffectsSome, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -534,6 +557,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgSVC("allow"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, SideEffects: &sideEffectsNoneOnDryRun, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -548,6 +572,7 @@ func NewNonMutatingTestCases(url *url.URL) []ValidatingTest { ClientConfig: ccfgURL("invalidAnnotation"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -593,6 +618,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("removeLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -607,6 +633,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("addLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectAllow: true, @@ -619,6 +646,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("addLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, IsCRD: true, @@ -632,6 +660,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("removeLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, IsCRD: true, @@ -647,6 +676,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("invalidMutation"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, AdmissionReviewVersions: []string{"v1beta1"}, }}, ExpectStatusCode: http.StatusInternalServerError, @@ -659,6 +689,7 @@ func NewMutatingTestCases(url *url.URL) []MutatingTest { ClientConfig: ccfgSVC("removeLabel"), Rules: matchEverythingRules, NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, SideEffects: &sideEffectsUnknown, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -784,6 +815,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { ClientConfig: ccfgSVC("allow"), Rules: newMatchEverythingRules(), NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyIgnore, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -797,6 +829,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { ClientConfig: ccfgSVC("internalErr"), Rules: newMatchEverythingRules(), NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyIgnore, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -810,6 +843,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { ClientConfig: ccfgSVC("allow"), Rules: newMatchEverythingRules(), NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyIgnore, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -823,6 +857,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { ClientConfig: ccfgURL("allow"), Rules: newMatchEverythingRules(), NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyIgnore, AdmissionReviewVersions: []string{"v1beta1"}, }}, @@ -836,6 +871,7 @@ func NewCachedClientTestcases(url *url.URL) []CachedTest { ClientConfig: ccfgURL("allow"), Rules: newMatchEverythingRules(), NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, FailurePolicy: &policyIgnore, AdmissionReviewVersions: []string{"v1beta1"}, }},