From 6cf499db6c1dd464c6072706106dec6c5284dff7 Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Wed, 29 May 2019 15:56:52 -0700 Subject: [PATCH] object matcher --- .../pkg/admission/plugin/webhook/accessors.go | 8 ++ .../admission/plugin/webhook/generic/BUILD | 2 + .../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 ++- .../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 +- .../admissionwebhook/reinvocation_test.go | 20 ++- 14 files changed, 397 insertions(+), 42 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/doc.go create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/matcher.go create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/matcher_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go index 108e8ff442f..b44c72ebfc9 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/BUILD index 7329da6c251..6572d3d6103 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/BUILD @@ -21,6 +21,7 @@ go_library( "//staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/config:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/namespace:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/rules:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/webhook:go_default_library", "//staging/src/k8s.io/client-go/informers:go_default_library", @@ -59,6 +60,7 @@ go_test( "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/namespace:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object:go_default_library", "//staging/src/k8s.io/apiserver/pkg/apis/example:go_default_library", "//staging/src/k8s.io/apiserver/pkg/apis/example/v1:go_default_library", "//staging/src/k8s.io/apiserver/pkg/apis/example2/v1:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/interfaces.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/interfaces.go index 227502de84e..4381691ef81 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/interfaces.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go index 99415292cbf..e88125ff5b8 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook_test.go index 0d6cc9ea167..ad7fc7896e9 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/generic/webhook_test.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go index f1d8ce819d3..5c5c41aedbd 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/dispatcher.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/doc.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/doc.go new file mode 100644 index 00000000000..93c47344095 --- /dev/null +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/matcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/matcher.go new file mode 100644 index 00000000000..be341dd95c9 --- /dev/null +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/matcher_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/object/matcher_test.go new file mode 100644 index 00000000000..823fabc9644 --- /dev/null +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go index e3046d63449..4b53c468410 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/testcase.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go index 193571cd6f2..6a9ee5ff144 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/testing/webhook_server.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go index 0505103d26d..dda52c90e57 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/dispatcher.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin.go index 388a237c984..30e5c9d3319 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/validating/plugin.go +++ b/staging/src/k8s.io/apiserver/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. diff --git a/test/integration/apiserver/admissionwebhook/reinvocation_test.go b/test/integration/apiserver/admissionwebhook/reinvocation_test.go index e1296e1150c..417f616eca1 100644 --- a/test/integration/apiserver/admissionwebhook/reinvocation_test.go +++ b/test/integration/apiserver/admissionwebhook/reinvocation_test.go @@ -52,8 +52,9 @@ func TestWebhookReinvocationPolicy(t *testing.T) { reinvokeIfNeeded := registrationv1beta1.IfNeededReinvocationPolicy type testWebhook struct { - path string - policy *registrationv1beta1.ReinvocationPolicyType + path string + policy *registrationv1beta1.ReinvocationPolicyType + objectSelector *metav1.LabelSelector } testCases := []struct { @@ -110,6 +111,16 @@ func TestWebhookReinvocationPolicy(t *testing.T) { expectLabels: map[string]string{"x": "true", "fight": "false"}, expectInvocations: map[string]int{"/settrue": 2, "/setfalse": 2}, }, + { // in-tree (mutation), webhook A is SKIPPED due to objectSelector not matching, webhook B (mutation), reinvoke in-tree (no-mutation), webhook A is SKIPPED even though the labels match now, because it's not called in the first round. No reinvocation of webhook B required + name: "no reinvocation of webhook B when in-tree or prior webhook mutations", + initialPriorityClass: "low-priority", // trigger initial in-tree mutation + webhooks: []testWebhook{ + {path: "/conditionaladdlabel", policy: &reinvokeIfNeeded, objectSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, + {path: "/addlabel", policy: &reinvokeIfNeeded}, + }, + expectLabels: map[string]string{"x": "true", "a": "true"}, + expectInvocations: map[string]int{"/addlabel": 1, "/conditionaladdlabel": 0}, + }, { name: "invalid priority class set by webhook should result in error from in-tree priority plugin", webhooks: []testWebhook{ @@ -193,7 +204,7 @@ func TestWebhookReinvocationPolicy(t *testing.T) { } for i, webhook := range tt.webhooks { - defer registerWebhook(t, client, fmt.Sprintf("admission.integration.test%d", i), webhookServer.URL+webhook.path, webhook.policy)() + defer registerWebhook(t, client, fmt.Sprintf("admission.integration.test%d", i), webhookServer.URL+webhook.path, webhook.policy, webhook.objectSelector)() } pod := &corev1.Pod{ @@ -248,7 +259,7 @@ func TestWebhookReinvocationPolicy(t *testing.T) { } } -func registerWebhook(t *testing.T, client clientset.Interface, name, endpoint string, reinvocationPolicy *registrationv1beta1.ReinvocationPolicyType) func() { +func registerWebhook(t *testing.T, client clientset.Interface, name, endpoint string, reinvocationPolicy *registrationv1beta1.ReinvocationPolicyType, objectSelector *metav1.LabelSelector) func() { fail := admissionv1beta1.Fail hook, err := client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Create(&admissionv1beta1.MutatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{Name: name}, @@ -262,6 +273,7 @@ func registerWebhook(t *testing.T, client clientset.Interface, name, endpoint st Operations: []admissionv1beta1.OperationType{admissionv1beta1.OperationAll}, Rule: admissionv1beta1.Rule{APIGroups: []string{"*"}, APIVersions: []string{"*"}, Resources: []string{"*/*"}}, }}, + ObjectSelector: objectSelector, FailurePolicy: &fail, ReinvocationPolicy: reinvocationPolicy, AdmissionReviewVersions: []string{"v1beta1"},