diff --git a/pkg/apis/admissionregistration/fuzzer/fuzzer.go b/pkg/apis/admissionregistration/fuzzer/fuzzer.go index 6e84fa5450289..86db2f44cab0e 100644 --- a/pkg/apis/admissionregistration/fuzzer/fuzzer.go +++ b/pkg/apis/admissionregistration/fuzzer/fuzzer.go @@ -55,6 +55,8 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} { obj.MatchPolicy = &m s := admissionregistration.SideEffectClassUnknown obj.SideEffects = &s + n := admissionregistration.NeverReinvocationPolicy + obj.ReinvocationPolicy = &n if obj.TimeoutSeconds == nil { i := int32(30) obj.TimeoutSeconds = &i diff --git a/pkg/apis/admissionregistration/types.go b/pkg/apis/admissionregistration/types.go index cb1624b59ad31..198f91acb7192 100644 --- a/pkg/apis/admissionregistration/types.go +++ b/pkg/apis/admissionregistration/types.go @@ -383,8 +383,39 @@ type MutatingWebhook struct { // does not understand, calls to the webhook will fail and be subject to the failure policy. // +optional AdmissionReviewVersions []string + + // reinvocationPolicy indicates whether this webhook should be called multiple times as part of a single admission evaluation. + // Allowed values are "Never" and "IfNeeded". + // + // Never: the webhook will not be called more than once in a single admission evaluation. + // + // IfNeeded: the webhook will be called at least one additional time as part of the admission evaluation + // if the object being admitted is modified by other admission plugins after the initial webhook call. + // Webhooks that specify this option *must* be idempotent, and hence able to process objects they previously admitted. + // Note: + // * the number of additional invocations is not guaranteed to be exactly one. + // * if additional invocations result in further modifications to the object, webhooks are not guaranteed to be invoked again. + // * webhooks that use this option may be reordered to minimize the number of additional invocations. + // * to validate an object after all mutations are guaranteed complete, use a validating admission webhook instead. + // + // Defaults to "Never". + // +optional + ReinvocationPolicy *ReinvocationPolicyType } +// ReinvocationPolicyType specifies what type of policy the admission hook uses. +type ReinvocationPolicyType string + +var ( + // NeverReinvocationPolicy indicates that the webhook must not be called more than once in a + // single admission evaluation. + NeverReinvocationPolicy ReinvocationPolicyType = "Never" + // IfNeededReinvocationPolicy indicates that the webhook may be called at least one + // additional time as part of the admission evaluation if the object being admitted is + // modified by other admission plugins after the initial webhook call. + IfNeededReinvocationPolicy ReinvocationPolicyType = "IfNeeded" +) + // RuleWithOperations is a tuple of Operations and Resources. It is recommended to make // sure that all the tuple expansions are valid. type RuleWithOperations struct { diff --git a/pkg/apis/admissionregistration/v1beta1/defaults.go b/pkg/apis/admissionregistration/v1beta1/defaults.go index b529dfd00bb9b..63637e7c93269 100644 --- a/pkg/apis/admissionregistration/v1beta1/defaults.go +++ b/pkg/apis/admissionregistration/v1beta1/defaults.go @@ -77,6 +77,10 @@ func SetDefaults_MutatingWebhook(obj *admissionregistrationv1beta1.MutatingWebho obj.TimeoutSeconds = new(int32) *obj.TimeoutSeconds = 30 } + if obj.ReinvocationPolicy == nil { + never := admissionregistrationv1beta1.NeverReinvocationPolicy + obj.ReinvocationPolicy = &never + } if len(obj.AdmissionReviewVersions) == 0 { obj.AdmissionReviewVersions = []string{admissionregistrationv1beta1.SchemeGroupVersion.Version} diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index 8fc4efe3544ab..4f2b7e457a968 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/pkg/apis/admissionregistration/validation/validation.go @@ -281,6 +281,9 @@ func validateMutatingWebhook(hook *admissionregistration.MutatingWebhook, fldPat if hook.NamespaceSelector != nil { allErrors = append(allErrors, metav1validation.ValidateLabelSelector(hook.NamespaceSelector, fldPath.Child("namespaceSelector"))...) } + if hook.ReinvocationPolicy != nil && !supportedReinvocationPolicies.Has(string(*hook.ReinvocationPolicy)) { + allErrors = append(allErrors, field.NotSupported(fldPath.Child("reinvocationPolicy"), *hook.ReinvocationPolicy, supportedReinvocationPolicies.List())) + } cc := hook.ClientConfig switch { @@ -319,6 +322,11 @@ var supportedOperations = sets.NewString( string(admissionregistration.Connect), ) +var supportedReinvocationPolicies = sets.NewString( + string(admissionregistration.NeverReinvocationPolicy), + string(admissionregistration.IfNeededReinvocationPolicy), +) + func hasWildcardOperation(operations []admissionregistration.OperationType) bool { for _, o := range operations { if o == admissionregistration.OperationAll { diff --git a/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go b/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go index 4e2d694d545c9..c18c9b20ec6a0 100644 --- a/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go +++ b/staging/src/k8s.io/api/admissionregistration/v1beta1/types.go @@ -404,8 +404,39 @@ type MutatingWebhook struct { // Default to `['v1beta1']`. // +optional AdmissionReviewVersions []string `json:"admissionReviewVersions,omitempty" protobuf:"bytes,8,rep,name=admissionReviewVersions"` + + // reinvocationPolicy indicates whether this webhook should be called multiple times as part of a single admission evaluation. + // Allowed values are "Never" and "IfNeeded". + // + // Never: the webhook will not be called more than once in a single admission evaluation. + // + // IfNeeded: the webhook will be called at least one additional time as part of the admission evaluation + // if the object being admitted is modified by other admission plugins after the initial webhook call. + // Webhooks that specify this option *must* be idempotent, able to process objects they previously admitted. + // Note: + // * the number of additional invocations is not guaranteed to be exactly one. + // * if additional invocations result in further modifications to the object, webhooks are not guaranteed to be invoked again. + // * webhooks that use this option may be reordered to minimize the number of additional invocations. + // * to validate an object after all mutations are guaranteed complete, use a validating admission webhook instead. + // + // Defaults to "Never". + // +optional + ReinvocationPolicy *ReinvocationPolicyType `json:"reinvocationPolicy,omitempty" protobuf:"bytes,10,opt,name=reinvocationPolicy,casttype=ReinvocationPolicyType"` } +// ReinvocationPolicyType specifies what type of policy the admission hook uses. +type ReinvocationPolicyType string + +const ( + // NeverReinvocationPolicy indicates that the webhook must not be called more than once in a + // single admission evaluation. + NeverReinvocationPolicy ReinvocationPolicyType = "Never" + // IfNeededReinvocationPolicy indicates that the webhook may be called at least one + // additional time as part of the admission evaluation if the object being admitted is + // modified by other admission plugins after the initial webhook call. + IfNeededReinvocationPolicy ReinvocationPolicyType = "IfNeeded" +) + // RuleWithOperations is a tuple of Operations and Resources. It is recommended to make // sure that all the tuple expansions are valid. type RuleWithOperations struct { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/BUILD index f89ee0ec0221d..7da1eb84e006d 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/admission/BUILD @@ -42,6 +42,7 @@ go_library( "handler.go", "interfaces.go", "plugins.go", + "reinvocation.go", "util.go", ], importmap = "k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/admission", diff --git a/staging/src/k8s.io/apiserver/pkg/admission/attributes.go b/staging/src/k8s.io/apiserver/pkg/admission/attributes.go index ad6ca6ba6fc30..beea941fc3067 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/attributes.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go index bacf61722c331..8cff4a254bf80 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go b/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go index bcce1e70f9200..804d83fe678c9 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/configuration/validating_webhook_manager.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/interfaces.go b/staging/src/k8s.io/apiserver/pkg/admission/interfaces.go index 040a6268b2fb3..5f6d703b216cc 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/interfaces.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/accessors.go index 362333c50f5d8..108e8ff442f49 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 @@ -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/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 c2ddbb7b1f889..0d6cc9ea16712 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 @@ -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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/BUILD b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/BUILD index 903afb8753b9a..27b41c947ad01 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/BUILD @@ -6,6 +6,7 @@ go_library( "dispatcher.go", "doc.go", "plugin.go", + "reinvocationcontext.go", ], importmap = "k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating", importpath = "k8s.io/apiserver/pkg/admission/plugin/webhook/mutating", @@ -13,11 +14,13 @@ go_library( deps = [ "//staging/src/k8s.io/api/admission/v1beta1:go_default_library", "//staging/src/k8s.io/api/admissionregistration/v1beta1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/serializer/json:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/configuration:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission/metrics:go_default_library", 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 b206d2a539307..f1d8ce819d376 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 @@ -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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go index b8f98eedffb18..b618e309149f3 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/plugin_test.go +++ b/staging/src/k8s.io/apiserver/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 - } + 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 + } - ns := "webhook-test" - client, informer := webhooktesting.NewFakeMutatingDataSource(ns, tt.Webhooks, stopCh) + 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) + 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) + informer.Start(stopCh) + informer.WaitForCacheSync(stopCh) - if err = wh.ValidateInitialization(); err != nil { - t.Errorf("%s: failed to validate initialization: %v", tt.Name, err) - continue - } + if err = wh.ValidateInitialization(); err != nil { + t.Errorf("failed to validate initialization: %v", err) + return + } - var attr admission.Attributes - if tt.IsCRD { - attr = webhooktesting.NewAttributeUnstructured(ns, tt.AdditionalLabels, tt.IsDryRun) - } else { - attr = webhooktesting.NewAttribute(ns, tt.AdditionalLabels, tt.IsDryRun) - } + 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()) + err = wh.Admit(attr, objectInterfaces) + if tt.ExpectAllow != (err == nil) { + t.Errorf("expected allowed=%v, but got err=%v", tt.ExpectAllow, err) } - } - // 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) + 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()) + } } - } - 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) + // 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) + } } - } - 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.") - } + 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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/reinvocationcontext.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/mutating/reinvocationcontext.go new file mode 100644 index 0000000000000..de0af221e13bf --- /dev/null +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/namespace/matcher_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/namespace/matcher_test.go index 4633cac8ecc9c..7bdf6c4509b71 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/namespace/matcher_test.go +++ b/staging/src/k8s.io/apiserver/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/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 ec8e6b552c7c8..e3046d6344928 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 @@ -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/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 5d080745dcaf7..193571cd6f2db 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 @@ -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/staging/src/k8s.io/apiserver/pkg/admission/plugins.go b/staging/src/k8s.io/apiserver/pkg/admission/plugins.go index bdf087e564f4b..d37af509c6614 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugins.go +++ b/staging/src/k8s.io/apiserver/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/staging/src/k8s.io/apiserver/pkg/admission/reinvocation.go b/staging/src/k8s.io/apiserver/pkg/admission/reinvocation.go new file mode 100644 index 0000000000000..b99e604e05c06 --- /dev/null +++ b/staging/src/k8s.io/apiserver/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) +} diff --git a/test/integration/apiserver/admissionwebhook/BUILD b/test/integration/apiserver/admissionwebhook/BUILD index 3804acaf435e8..529a1c848c931 100644 --- a/test/integration/apiserver/admissionwebhook/BUILD +++ b/test/integration/apiserver/admissionwebhook/BUILD @@ -6,6 +6,7 @@ go_test( "admission_test.go", "broken_webhook_test.go", "main_test.go", + "reinvocation_test.go", ], rundir = ".", tags = [ @@ -21,6 +22,7 @@ go_test( "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/api/extensions/v1beta1:go_default_library", "//staging/src/k8s.io/api/policy/v1beta1:go_default_library", + "//staging/src/k8s.io/api/scheduling/v1:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/test/integration/apiserver/admissionwebhook/reinvocation_test.go b/test/integration/apiserver/admissionwebhook/reinvocation_test.go new file mode 100644 index 0000000000000..e1296e1150c6d --- /dev/null +++ b/test/integration/apiserver/admissionwebhook/reinvocation_test.go @@ -0,0 +1,389 @@ +/* +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 admissionwebhook + +import ( + "crypto/tls" + "crypto/x509" + "encoding/json" + "fmt" + "io/ioutil" + "net/http" + "net/http/httptest" + "reflect" + "strings" + "sync" + "testing" + + "k8s.io/api/admission/v1beta1" + admissionv1beta1 "k8s.io/api/admissionregistration/v1beta1" + registrationv1beta1 "k8s.io/api/admissionregistration/v1beta1" + corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" + schedulingv1 "k8s.io/api/scheduling/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clientset "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" + "k8s.io/kubernetes/test/integration/framework" +) + +const ( + testReinvocationClientUsername = "webhook-reinvocation-integration-client" +) + +// TestWebhookReinvocationPolicy ensures that the admission webhook reinvocation policy is applied correctly. +func TestWebhookReinvocationPolicy(t *testing.T) { + reinvokeNever := registrationv1beta1.NeverReinvocationPolicy + reinvokeIfNeeded := registrationv1beta1.IfNeededReinvocationPolicy + + type testWebhook struct { + path string + policy *registrationv1beta1.ReinvocationPolicyType + } + + testCases := []struct { + name string + initialPriorityClass string + webhooks []testWebhook + expectLabels map[string]string + expectInvocations map[string]int + expectError bool + errorContains string + }{ + { // in-tree (mutation), webhook (no mutation), no reinvocation required + name: "no reinvocation for in-tree only mutation", + initialPriorityClass: "low-priority", // trigger initial in-tree mutation + webhooks: []testWebhook{ + {path: "/noop", policy: &reinvokeIfNeeded}, + }, + expectInvocations: map[string]int{"/noop": 1}, + }, + { // in-tree (mutation), webhook (mutation), reinvoke in-tree (no-mutation), no webhook reinvocation required + name: "no webhook reinvocation for webhook when no in-tree reinvocation mutations", + initialPriorityClass: "low-priority", // trigger initial in-tree mutation + webhooks: []testWebhook{ + {path: "/addlabel", policy: &reinvokeIfNeeded}, + }, + expectInvocations: map[string]int{"/addlabel": 1}, + }, + { // in-tree (mutation), webhook (mutation), reinvoke in-tree (mutation), webhook (no-mutation), both reinvoked + name: "webhook is reinvoked after in-tree reinvocation", + initialPriorityClass: "low-priority", // trigger initial in-tree mutation + webhooks: []testWebhook{ + // Priority plugin is ordered to run before mutating webhooks + {path: "/setpriority", policy: &reinvokeIfNeeded}, // trigger in-tree reinvoke mutation + }, + expectInvocations: map[string]int{"/setpriority": 2}, + }, + { // in-tree (mutation), webhook A (mutation), webhook B (mutation), reinvoke in-tree (no-mutation), reinvoke webhook A (no-mutation), 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: "/addlabel", policy: &reinvokeIfNeeded}, + {path: "/conditionaladdlabel", policy: &reinvokeIfNeeded}, + }, + expectLabels: map[string]string{"x": "true", "a": "true", "b": "true"}, + expectInvocations: map[string]int{"/addlabel": 2, "/conditionaladdlabel": 1}, + }, + { // in-tree (mutation), webhook A (mutation), webhook B (mutation), reinvoke in-tree (no-mutation), reinvoke webhook A (mutation), reinvoke webhook B (mutation), both webhooks reinvoked + name: "all webhooks reinvoked when any webhook reinvocation causes mutation", + initialPriorityClass: "low-priority", // trigger initial in-tree mutation + webhooks: []testWebhook{ + {path: "/settrue", policy: &reinvokeIfNeeded}, + {path: "/setfalse", policy: &reinvokeIfNeeded}, + }, + expectLabels: map[string]string{"x": "true", "fight": "false"}, + expectInvocations: map[string]int{"/settrue": 2, "/setfalse": 2}, + }, + { + name: "invalid priority class set by webhook should result in error from in-tree priority plugin", + webhooks: []testWebhook{ + // Priority plugin is ordered to run before mutating webhooks + {path: "/setinvalidpriority", policy: &reinvokeIfNeeded}, + }, + expectError: true, + errorContains: "no PriorityClass with name invalid was found", + expectInvocations: map[string]int{"/setinvalidpriority": 1}, + }, + { + name: "'reinvoke never' policy respected", + webhooks: []testWebhook{ + {path: "/conditionaladdlabel", policy: &reinvokeNever}, + {path: "/addlabel", policy: &reinvokeNever}, + }, + expectLabels: map[string]string{"x": "true", "a": "true"}, + expectInvocations: map[string]int{"/conditionaladdlabel": 1, "/addlabel": 1}, + }, + { + name: "'reinvoke never' (by default) policy respected", + webhooks: []testWebhook{ + {path: "/conditionaladdlabel", policy: nil}, + {path: "/addlabel", policy: nil}, + }, + expectLabels: map[string]string{"x": "true", "a": "true"}, + expectInvocations: map[string]int{"/conditionaladdlabel": 1, "/addlabel": 1}, + }, + } + + roots := x509.NewCertPool() + if !roots.AppendCertsFromPEM(localhostCert) { + t.Fatal("Failed to append Cert from PEM") + } + cert, err := tls.X509KeyPair(localhostCert, localhostKey) + if err != nil { + t.Fatalf("Failed to build cert with error: %+v", err) + } + + recorder := &invocationRecorder{counts: map[string]int{}} + webhookServer := httptest.NewUnstartedServer(newReinvokeWebhookHandler(recorder)) + webhookServer.TLS = &tls.Config{ + + RootCAs: roots, + Certificates: []tls.Certificate{cert}, + } + webhookServer.StartTLS() + defer webhookServer.Close() + + s := kubeapiservertesting.StartTestServerOrDie(t, kubeapiservertesting.NewDefaultTestServerOptions(), []string{ + "--disable-admission-plugins=ServiceAccount", + }, framework.SharedEtcd()) + defer s.TearDownFn() + + // Configure a client with a distinct user name so that it is easy to distinguish requests + // made by the client from requests made by controllers. We use this to filter out requests + // before recording them to ensure we don't accidentally mistake requests from controllers + // as requests made by the client. + clientConfig := rest.CopyConfig(s.ClientConfig) + clientConfig.Impersonate.UserName = testReinvocationClientUsername + clientConfig.Impersonate.Groups = []string{"system:masters", "system:authenticated"} + client, err := clientset.NewForConfig(clientConfig) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + for priorityClass, priority := range map[string]int{"low-priority": 1, "high-priority": 10} { + _, err = client.SchedulingV1().PriorityClasses().Create(&schedulingv1.PriorityClass{ObjectMeta: metav1.ObjectMeta{Name: priorityClass}, Value: int32(priority)}) + if err != nil { + t.Fatal(err) + } + } + + for i, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + recorder.Reset() + ns := fmt.Sprintf("reinvoke-%d", i) + _, err = client.CoreV1().Namespaces().Create(&v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}) + if err != nil { + t.Fatal(err) + } + + for i, webhook := range tt.webhooks { + defer registerWebhook(t, client, fmt.Sprintf("admission.integration.test%d", i), webhookServer.URL+webhook.path, webhook.policy)() + } + + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: "labeled", + Labels: map[string]string{"x": "true"}, + }, + Spec: corev1.PodSpec{ + Containers: []v1.Container{{ + Name: "fake-name", + Image: "fakeimage", + }}, + }, + } + if tt.initialPriorityClass != "" { + pod.Spec.PriorityClassName = tt.initialPriorityClass + } + obj, err := client.CoreV1().Pods(ns).Create(pod) + + if tt.expectError { + if err == nil { + t.Fatalf("expected error but got none") + } + if tt.errorContains != "" { + if !strings.Contains(err.Error(), tt.errorContains) { + t.Errorf("expected an error saying %q, but got: %v", tt.errorContains, err) + } + } + return + } + + if err != nil { + t.Fatal(err) + } + + if tt.expectLabels != nil { + labels := obj.GetLabels() + if !reflect.DeepEqual(tt.expectLabels, labels) { + t.Errorf("expected labels '%v', but got '%v'", tt.expectLabels, labels) + } + } + + if tt.expectInvocations != nil { + for k, v := range tt.expectInvocations { + if recorder.GetCount(k) != v { + t.Errorf("expected %d invocations of %s, but got %d", v, k, recorder.GetCount(k)) + } + } + } + }) + } +} + +func registerWebhook(t *testing.T, client clientset.Interface, name, endpoint string, reinvocationPolicy *registrationv1beta1.ReinvocationPolicyType) func() { + fail := admissionv1beta1.Fail + hook, err := client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Create(&admissionv1beta1.MutatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Webhooks: []admissionv1beta1.MutatingWebhook{{ + Name: name, + ClientConfig: admissionv1beta1.WebhookClientConfig{ + URL: &endpoint, + CABundle: localhostCert, + }, + Rules: []admissionv1beta1.RuleWithOperations{{ + Operations: []admissionv1beta1.OperationType{admissionv1beta1.OperationAll}, + Rule: admissionv1beta1.Rule{APIGroups: []string{"*"}, APIVersions: []string{"*"}, Resources: []string{"*/*"}}, + }}, + FailurePolicy: &fail, + ReinvocationPolicy: reinvocationPolicy, + AdmissionReviewVersions: []string{"v1beta1"}, + }}, + }) + if err != nil { + t.Fatal(err) + } + + tearDown := func() { + err := client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations().Delete(hook.GetName(), &metav1.DeleteOptions{}) + if err != nil { + t.Fatal(err) + } + } + return tearDown +} + +type invocationRecorder struct { + mu sync.Mutex + counts map[string]int +} + +func (i *invocationRecorder) Reset() { + i.mu.Lock() + defer i.mu.Unlock() + i.counts = map[string]int{} +} + +func (i *invocationRecorder) GetCount(path string) int { + i.mu.Lock() + defer i.mu.Unlock() + return i.counts[path] +} + +func (i *invocationRecorder) IncrementCount(path string) { + i.mu.Lock() + defer i.mu.Unlock() + i.counts[path]++ +} + +func newReinvokeWebhookHandler(recorder *invocationRecorder) http.Handler { + patch := func(w http.ResponseWriter, patch string) { + w.Header().Set("Content-Type", "application/json") + pt := v1beta1.PatchTypeJSONPatch + json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{ + Response: &v1beta1.AdmissionResponse{ + Allowed: true, + PatchType: &pt, + Patch: []byte(patch), + }, + }) + } + allow := func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(&v1beta1.AdmissionReview{ + Response: &v1beta1.AdmissionResponse{ + Allowed: true, + }, + }) + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + data, err := ioutil.ReadAll(r.Body) + if err != nil { + http.Error(w, err.Error(), 400) + } + review := v1beta1.AdmissionReview{} + if err := json.Unmarshal(data, &review); err != nil { + http.Error(w, err.Error(), 400) + } + if review.Request.UserInfo.Username != testReinvocationClientUsername { + // skip requests not originating from this integration test's client + allow(w) + return + } + + if len(review.Request.Object.Raw) == 0 { + http.Error(w, err.Error(), 400) + } + pod := &corev1.Pod{} + if err := json.Unmarshal(review.Request.Object.Raw, pod); err != nil { + http.Error(w, err.Error(), 400) + } + + recorder.IncrementCount(r.URL.Path) + + switch r.URL.Path { + case "/noop": + allow(w) + case "/settrue": + patch(w, `[{"op": "replace", "path": "/metadata/labels/fight", "value": "true"}]`) + case "/setfalse": + patch(w, `[{"op": "replace", "path": "/metadata/labels/fight", "value": "false"}]`) + case "/addlabel": + labels := pod.GetLabels() + if a, ok := labels["a"]; !ok || a != "true" { + patch(w, `[{"op": "add", "path": "/metadata/labels/a", "value": "true"}]`) + return + } + allow(w) + case "/conditionaladdlabel": // if 'a' is set, set 'b' to true + labels := pod.GetLabels() + if _, ok := labels["a"]; ok { + patch(w, `[{"op": "add", "path": "/metadata/labels/b", "value": "true"}]`) + return + } + allow(w) + case "/setpriority": // sets /spec/priorityClassName to high-priority if it is not already set + if pod.Spec.PriorityClassName != "high-priority" { + if pod.Spec.Priority != nil { + patch(w, `[{"op": "add", "path": "/spec/priorityClassName", "value": "high-priority"},{"op": "remove", "path": "/spec/priority"}]`) + } else { + patch(w, `[{"op": "add", "path": "/spec/priorityClassName", "value": "high-priority"}]`) + } + return + } + allow(w) + case "/setinvalidpriority": + patch(w, `[{"op": "add", "path": "/spec/priorityClassName", "value": "invalid"}]`) + default: + http.NotFound(w, r) + } + }) +}