From 05d2078e680a82537d17edc6050a3451364fc1df Mon Sep 17 00:00:00 2001 From: Igor Velichkovich Date: Tue, 14 Mar 2023 22:28:26 -0500 Subject: [PATCH] Matchconditions admission webhooks alpha implementation for kep-3716 (#116261) * api changes adding match conditions * feature gate and registry strategy to drop fields * matchConditions logic for admission webhooks * feedback * update test * import order * bears.com * update fail policy ignore behavior * update docs and matcher to hold fail policy as non-pointer * update matcher error aggregation, fix early fail failpolicy ignore, update docs * final cleanup * openapi gen Kubernetes-commit: 5e5b3029f3bbfc93c3569f07ad300a5c6057fc58 --- go.mod | 8 +- go.sum | 8 +- pkg/admission/metrics/metrics.go | 31 +- pkg/admission/plugin/cel/interface.go | 15 - .../validatingadmissionpolicy/validator.go | 3 +- pkg/admission/plugin/webhook/accessors.go | 69 +++- .../plugin/webhook/accessors_test.go | 2 + .../plugin/webhook/generic/interfaces.go | 4 + .../plugin/webhook/generic/webhook.go | 33 +- .../plugin/webhook/generic/webhook_test.go | 199 ++++++++-- .../webhook/matchconditions/interface.go | 36 ++ .../plugin/webhook/matchconditions/matcher.go | 139 +++++++ .../webhook/matchconditions/matcher_test.go | 360 ++++++++++++++++++ .../plugin/webhook/mutating/dispatcher.go | 61 ++- .../plugin/webhook/validating/dispatcher.go | 39 +- pkg/apis/cel/config.go | 5 + pkg/features/kube_features.go | 10 + 17 files changed, 933 insertions(+), 89 deletions(-) create mode 100644 pkg/admission/plugin/webhook/matchconditions/interface.go create mode 100644 pkg/admission/plugin/webhook/matchconditions/matcher.go create mode 100644 pkg/admission/plugin/webhook/matchconditions/matcher_test.go diff --git a/go.mod b/go.mod index e3fcda478..ad69561e5 100644 --- a/go.mod +++ b/go.mod @@ -42,9 +42,9 @@ require ( google.golang.org/protobuf v1.28.1 gopkg.in/natefinch/lumberjack.v2 v2.0.0 gopkg.in/square/go-jose.v2 v2.6.0 - k8s.io/api v0.0.0-20230315055832-c79498c4e63b + k8s.io/api v0.0.0-20230315055831-abe66f57fdb1 k8s.io/apimachinery v0.0.0-20230315054728-8d1258da8f38 - k8s.io/client-go v0.0.0-20230315061852-9ff2627505a4 + k8s.io/client-go v0.0.0-20230315061912-38589731da69 k8s.io/component-base v0.0.0-20230315065615-6b9bb8ecc3d0 k8s.io/klog/v2 v2.90.1 k8s.io/kms v0.0.0-20230315071541-54e6d3479bfc @@ -124,9 +124,9 @@ require ( ) replace ( - k8s.io/api => k8s.io/api v0.0.0-20230315055832-c79498c4e63b + k8s.io/api => k8s.io/api v0.0.0-20230315032826-0b4c449988b1 k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230315054728-8d1258da8f38 - k8s.io/client-go => k8s.io/client-go v0.0.0-20230315061852-9ff2627505a4 + k8s.io/client-go => k8s.io/client-go v0.0.0-20230315061912-38589731da69 k8s.io/component-base => k8s.io/component-base v0.0.0-20230315065615-6b9bb8ecc3d0 k8s.io/kms => k8s.io/kms v0.0.0-20230315071541-54e6d3479bfc ) diff --git a/go.sum b/go.sum index 31d830610..7f912fca6 100644 --- a/go.sum +++ b/go.sum @@ -878,12 +878,12 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg= honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k= honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k= -k8s.io/api v0.0.0-20230315055832-c79498c4e63b h1:+XtheLjvEgurlWvtf13tutNwmW6WUHGezbiapc4s6EM= -k8s.io/api v0.0.0-20230315055832-c79498c4e63b/go.mod h1:aZ6MBt4NMLXSxkSKFkoDaP4hTutnZIvH5dCSpOis9g4= +k8s.io/api v0.0.0-20230315032826-0b4c449988b1 h1:wlCdY1kqV0RkfnfRr4mEZ3fGJ1VvLelr5Q2vCnCICIo= +k8s.io/api v0.0.0-20230315032826-0b4c449988b1/go.mod h1:aZ6MBt4NMLXSxkSKFkoDaP4hTutnZIvH5dCSpOis9g4= k8s.io/apimachinery v0.0.0-20230315054728-8d1258da8f38 h1:n1qDRCTPAXwyXYg7eSpWDO9FdW79lwAQ9dAr1vETpn4= k8s.io/apimachinery v0.0.0-20230315054728-8d1258da8f38/go.mod h1:5ikh59fK3AJ287GUvpUsryoMFtH9zj/ARfWCo3AyXTM= -k8s.io/client-go v0.0.0-20230315061852-9ff2627505a4 h1:mjPYL7mzIzB9PHI4ttnYkGY5zat6ObsndQNh7Gx12OQ= -k8s.io/client-go v0.0.0-20230315061852-9ff2627505a4/go.mod h1:daDXfDBtiJdqKrqqFL0WtQ6hHzsDxP1lCEdTt3+kijU= +k8s.io/client-go v0.0.0-20230315061912-38589731da69 h1:LnTXY9Akksk/aAmbebKIaC0doqk1aKbZQA3OoAd0BB0= +k8s.io/client-go v0.0.0-20230315061912-38589731da69/go.mod h1:b0alWGtfu+BI7XZwwdOHJIsr7aDjKf3ANThw8Sr+tw8= k8s.io/component-base v0.0.0-20230315065615-6b9bb8ecc3d0 h1:IjneP02MOB07PIP9+PQjKrOIZEZ5T7umR+GIZkU4h0U= k8s.io/component-base v0.0.0-20230315065615-6b9bb8ecc3d0/go.mod h1:kTuptveA6tUMLMKnaq4AbIAAk7IcdhwkbljAV3JZRpM= k8s.io/klog/v2 v2.90.1 h1:m4bYOKall2MmOiRaR1J+We67Do7vm9KiQVlT96lnHUw= diff --git a/pkg/admission/metrics/metrics.go b/pkg/admission/metrics/metrics.go index 5ed8d0609..26b82c37e 100644 --- a/pkg/admission/metrics/metrics.go +++ b/pkg/admission/metrics/metrics.go @@ -112,12 +112,13 @@ func (p pluginHandlerWithMetrics) Validate(ctx context.Context, a admission.Attr // AdmissionMetrics instruments admission with prometheus metrics. type AdmissionMetrics struct { - step *metricSet - controller *metricSet - webhook *metricSet - webhookRejection *metrics.CounterVec - webhookFailOpen *metrics.CounterVec - webhookRequest *metrics.CounterVec + step *metricSet + controller *metricSet + webhook *metricSet + webhookRejection *metrics.CounterVec + webhookFailOpen *metrics.CounterVec + webhookRequest *metrics.CounterVec + matchConditionEvalErrors *metrics.CounterVec } // newAdmissionMetrics create a new AdmissionMetrics, configured with default metric names. @@ -217,13 +218,24 @@ func newAdmissionMetrics() *AdmissionMetrics { }, []string{"name", "type", "operation", "code", "rejected"}) + matchConditionEvalError := metrics.NewCounterVec( + &metrics.CounterOpts{ + Namespace: namespace, + Subsystem: subsystem, + Name: "admission_match_condition_evaluation_errors_total", + Help: "Admission match condition evaluation errors count, identified by name of resource containing the match condition and broken out for each admission type (validating or mutating).", + StabilityLevel: metrics.ALPHA, + }, + []string{"name", "type"}) + step.mustRegister() controller.mustRegister() webhook.mustRegister() legacyregistry.MustRegister(webhookRejection) legacyregistry.MustRegister(webhookFailOpen) legacyregistry.MustRegister(webhookRequest) - return &AdmissionMetrics{step: step, controller: controller, webhook: webhook, webhookRejection: webhookRejection, webhookFailOpen: webhookFailOpen, webhookRequest: webhookRequest} + legacyregistry.MustRegister(matchConditionEvalError) + return &AdmissionMetrics{step: step, controller: controller, webhook: webhook, webhookRejection: webhookRejection, webhookFailOpen: webhookFailOpen, webhookRequest: webhookRequest, matchConditionEvalErrors: matchConditionEvalError} } func (m *AdmissionMetrics) reset() { @@ -267,6 +279,11 @@ func (m *AdmissionMetrics) ObserveWebhookFailOpen(ctx context.Context, name, ste m.webhookFailOpen.WithContext(ctx).WithLabelValues(name, stepType).Inc() } +// ObserveMatchConditionEvalError records validating or mutating webhook that are not called due to match conditions +func (m *AdmissionMetrics) ObserveMatchConditionEvalError(ctx context.Context, name, stepType string) { + m.matchConditionEvalErrors.WithContext(ctx).WithLabelValues(name, stepType).Inc() +} + type metricSet struct { latencies *metrics.HistogramVec latenciesSummary *metrics.SummaryVec diff --git a/pkg/admission/plugin/cel/interface.go b/pkg/admission/plugin/cel/interface.go index 265355369..d3c4a0217 100644 --- a/pkg/admission/plugin/cel/interface.go +++ b/pkg/admission/plugin/cel/interface.go @@ -29,8 +29,6 @@ import ( "k8s.io/apiserver/pkg/authorization/authorizer" ) -var _ ExpressionAccessor = &MatchCondition{} - type ExpressionAccessor interface { GetExpression() string ReturnTypes() []*cel.Type @@ -44,19 +42,6 @@ type EvaluationResult struct { Error error } -// MatchCondition contains the inputs needed to compile, evaluate and match a cel expression -type MatchCondition struct { - Expression string -} - -func (v *MatchCondition) GetExpression() string { - return v.Expression -} - -func (v *MatchCondition) ReturnTypes() []*cel.Type { - return []*cel.Type{cel.BoolType} -} - // OptionalVariableDeclarations declares which optional CEL variables // are declared for an expression. type OptionalVariableDeclarations struct { diff --git a/pkg/admission/plugin/validatingadmissionpolicy/validator.go b/pkg/admission/plugin/validatingadmissionpolicy/validator.go index fcb901ed6..0b19158a1 100644 --- a/pkg/admission/plugin/validatingadmissionpolicy/validator.go +++ b/pkg/admission/plugin/validatingadmissionpolicy/validator.go @@ -23,8 +23,6 @@ import ( celtypes "github.com/google/cel-go/common/types" - "k8s.io/klog/v2" - v1 "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -33,6 +31,7 @@ import ( celconfig "k8s.io/apiserver/pkg/apis/cel" "k8s.io/apiserver/pkg/authorization/authorizer" apiservercel "k8s.io/apiserver/pkg/cel" + "k8s.io/klog/v2" ) // validator implements the Validator interface diff --git a/pkg/admission/plugin/webhook/accessors.go b/pkg/admission/plugin/webhook/accessors.go index bbe355f31..102597cbc 100644 --- a/pkg/admission/plugin/webhook/accessors.go +++ b/pkg/admission/plugin/webhook/accessors.go @@ -19,11 +19,15 @@ package webhook import ( "sync" - "k8s.io/api/admissionregistration/v1" + v1 "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apiserver/pkg/admission/plugin/cel" + "k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions" "k8s.io/apiserver/pkg/admission/plugin/webhook/predicates/namespace" "k8s.io/apiserver/pkg/admission/plugin/webhook/predicates/object" + celconfig "k8s.io/apiserver/pkg/apis/cel" + "k8s.io/apiserver/pkg/authorization/authorizer" webhookutil "k8s.io/apiserver/pkg/util/webhook" "k8s.io/client-go/rest" ) @@ -44,6 +48,9 @@ type WebhookAccessor interface { // GetRESTClient gets the webhook client GetRESTClient(clientManager *webhookutil.ClientManager) (*rest.RESTClient, error) + // GetCompiledMatcher gets the compiled matcher object + GetCompiledMatcher(compiler cel.FilterCompiler, authorizer authorizer.Authorizer) matchconditions.Matcher + // 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. @@ -67,6 +74,9 @@ type WebhookAccessor interface { // GetAdmissionReviewVersions gets the webhook AdmissionReviewVersions field. GetAdmissionReviewVersions() []string + // GetMatchConditions gets the webhook match conditions field. + GetMatchConditions() []v1.MatchCondition + // GetMutatingWebhook if the accessor contains a MutatingWebhook, returns it and true, else returns false. GetMutatingWebhook() (*v1.MutatingWebhook, bool) // GetValidatingWebhook if the accessor contains a ValidatingWebhook, returns it and true, else returns false. @@ -94,6 +104,9 @@ type mutatingWebhookAccessor struct { initClient sync.Once client *rest.RESTClient clientErr error + + compileMatcher sync.Once + compiledMatcher matchconditions.Matcher } func (m *mutatingWebhookAccessor) GetUID() string { @@ -111,6 +124,28 @@ func (m *mutatingWebhookAccessor) GetRESTClient(clientManager *webhookutil.Clien return m.client, m.clientErr } +// TODO: graduation to beta: resolve the fact that we rebuild ALL items whenever ANY config changes in NewMutatingWebhookConfigurationManager and NewValidatingWebhookConfigurationManager ... now that we're doing CEL compilation, we probably want to avoid that +func (m *mutatingWebhookAccessor) GetCompiledMatcher(compiler cel.FilterCompiler, authorizer authorizer.Authorizer) matchconditions.Matcher { + m.compileMatcher.Do(func() { + expressions := make([]cel.ExpressionAccessor, len(m.MutatingWebhook.MatchConditions)) + for i, matchCondition := range m.MutatingWebhook.MatchConditions { + expressions[i] = &matchconditions.MatchCondition{ + Name: matchCondition.Name, + Expression: matchCondition.Expression, + } + } + m.compiledMatcher = matchconditions.NewMatcher(compiler.Compile( + expressions, + cel.OptionalVariableDeclarations{ + HasParams: false, + HasAuthorizer: true, + }, + celconfig.PerCallLimit, + ), authorizer, m.FailurePolicy, "validating", m.Name) + }) + return m.compiledMatcher +} + func (m *mutatingWebhookAccessor) GetParsedNamespaceSelector() (labels.Selector, error) { m.initNamespaceSelector.Do(func() { m.namespaceSelector, m.namespaceSelectorErr = metav1.LabelSelectorAsSelector(m.NamespaceSelector) @@ -165,6 +200,10 @@ func (m *mutatingWebhookAccessor) GetAdmissionReviewVersions() []string { return m.AdmissionReviewVersions } +func (m *mutatingWebhookAccessor) GetMatchConditions() []v1.MatchCondition { + return m.MatchConditions +} + func (m *mutatingWebhookAccessor) GetMutatingWebhook() (*v1.MutatingWebhook, bool) { return m.MutatingWebhook, true } @@ -194,6 +233,9 @@ type validatingWebhookAccessor struct { initClient sync.Once client *rest.RESTClient clientErr error + + compileMatcher sync.Once + compiledMatcher matchconditions.Matcher } func (v *validatingWebhookAccessor) GetUID() string { @@ -211,6 +253,27 @@ func (v *validatingWebhookAccessor) GetRESTClient(clientManager *webhookutil.Cli return v.client, v.clientErr } +func (v *validatingWebhookAccessor) GetCompiledMatcher(compiler cel.FilterCompiler, authorizer authorizer.Authorizer) matchconditions.Matcher { + v.compileMatcher.Do(func() { + expressions := make([]cel.ExpressionAccessor, len(v.ValidatingWebhook.MatchConditions)) + for i, matchCondition := range v.ValidatingWebhook.MatchConditions { + expressions[i] = &matchconditions.MatchCondition{ + Name: matchCondition.Name, + Expression: matchCondition.Expression, + } + } + v.compiledMatcher = matchconditions.NewMatcher(compiler.Compile( + expressions, + cel.OptionalVariableDeclarations{ + HasParams: false, + HasAuthorizer: true, + }, + celconfig.PerCallLimit, + ), authorizer, v.FailurePolicy, "validating", v.Name) + }) + return v.compiledMatcher +} + func (v *validatingWebhookAccessor) GetParsedNamespaceSelector() (labels.Selector, error) { v.initNamespaceSelector.Do(func() { v.namespaceSelector, v.namespaceSelectorErr = metav1.LabelSelectorAsSelector(v.NamespaceSelector) @@ -265,6 +328,10 @@ func (v *validatingWebhookAccessor) GetAdmissionReviewVersions() []string { return v.AdmissionReviewVersions } +func (v *validatingWebhookAccessor) GetMatchConditions() []v1.MatchCondition { + return v.MatchConditions +} + func (v *validatingWebhookAccessor) GetMutatingWebhook() (*v1.MutatingWebhook, bool) { return nil, false } diff --git a/pkg/admission/plugin/webhook/accessors_test.go b/pkg/admission/plugin/webhook/accessors_test.go index e26e3d95f..943dec114 100644 --- a/pkg/admission/plugin/webhook/accessors_test.go +++ b/pkg/admission/plugin/webhook/accessors_test.go @@ -62,6 +62,7 @@ func TestMutatingWebhookAccessor(t *testing.T) { SideEffects: accessor.GetSideEffects(), TimeoutSeconds: accessor.GetTimeoutSeconds(), AdmissionReviewVersions: accessor.GetAdmissionReviewVersions(), + MatchConditions: accessor.GetMatchConditions(), } if !reflect.DeepEqual(orig, copy) { t.Errorf("expected mutatingWebhook to round trip through WebhookAccessor, diff:\n%s", diff.ObjectReflectDiff(orig, copy)) @@ -102,6 +103,7 @@ func TestValidatingWebhookAccessor(t *testing.T) { SideEffects: accessor.GetSideEffects(), TimeoutSeconds: accessor.GetTimeoutSeconds(), AdmissionReviewVersions: accessor.GetAdmissionReviewVersions(), + MatchConditions: accessor.GetMatchConditions(), } if !reflect.DeepEqual(orig, copy) { t.Errorf("expected validatingWebhook to round trip through WebhookAccessor, diff:\n%s", diff.ObjectReflectDiff(orig, copy)) diff --git a/pkg/admission/plugin/webhook/generic/interfaces.go b/pkg/admission/plugin/webhook/generic/interfaces.go index 79666d934..af33a09f4 100644 --- a/pkg/admission/plugin/webhook/generic/interfaces.go +++ b/pkg/admission/plugin/webhook/generic/interfaces.go @@ -24,6 +24,10 @@ import ( "k8s.io/apiserver/pkg/admission/plugin/webhook" ) +type VersionedAttributeAccessor interface { + VersionedAttribute(gvk schema.GroupVersionKind) (*admission.VersionedAttributes, error) +} + // Source can list dynamic webhook plugins. type Source interface { Webhooks() []webhook.WebhookAccessor diff --git a/pkg/admission/plugin/webhook/generic/webhook.go b/pkg/admission/plugin/webhook/generic/webhook.go index 52df53af8..a58289831 100644 --- a/pkg/admission/plugin/webhook/generic/webhook.go +++ b/pkg/admission/plugin/webhook/generic/webhook.go @@ -23,19 +23,22 @@ import ( admissionv1 "k8s.io/api/admission/v1" admissionv1beta1 "k8s.io/api/admission/v1beta1" - "k8s.io/api/admissionregistration/v1" + v1 "k8s.io/api/admissionregistration/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" genericadmissioninit "k8s.io/apiserver/pkg/admission/initializer" + "k8s.io/apiserver/pkg/admission/plugin/cel" "k8s.io/apiserver/pkg/admission/plugin/webhook" "k8s.io/apiserver/pkg/admission/plugin/webhook/config" "k8s.io/apiserver/pkg/admission/plugin/webhook/predicates/namespace" "k8s.io/apiserver/pkg/admission/plugin/webhook/predicates/object" "k8s.io/apiserver/pkg/admission/plugin/webhook/predicates/rules" + "k8s.io/apiserver/pkg/authorization/authorizer" webhookutil "k8s.io/apiserver/pkg/util/webhook" "k8s.io/client-go/informers" clientset "k8s.io/client-go/kubernetes" + "k8s.io/klog/v2" ) // Webhook is an abstract admission plugin with all the infrastructure to define Admit or Validate on-top. @@ -49,6 +52,8 @@ type Webhook struct { namespaceMatcher *namespace.Matcher objectMatcher *object.Matcher dispatcher Dispatcher + filterCompiler cel.FilterCompiler + authorizer authorizer.Authorizer } var ( @@ -92,6 +97,7 @@ func NewWebhook(handler *admission.Handler, configFile io.Reader, sourceFactory namespaceMatcher: &namespace.Matcher{}, objectMatcher: &object.Matcher{}, dispatcher: dispatcherFactory(&cm), + filterCompiler: cel.NewFilterCompiler(), }, nil } @@ -124,6 +130,10 @@ func (a *Webhook) SetExternalKubeInformerFactory(f informers.SharedInformerFacto }) } +func (a *Webhook) SetAuthorizer(authorizer authorizer.Authorizer) { + a.authorizer = authorizer +} + // ValidateInitialization implements the InitializationValidator interface. func (a *Webhook) ValidateInitialization() error { if a.hookSource == nil { @@ -140,7 +150,7 @@ func (a *Webhook) ValidateInitialization() error { // ShouldCallHook returns invocation details if the webhook should be called, nil if the webhook should not be called, // or an error if an error was encountered during evaluation. -func (a *Webhook) ShouldCallHook(h webhook.WebhookAccessor, attr admission.Attributes, o admission.ObjectInterfaces) (*WebhookInvocation, *apierrors.StatusError) { +func (a *Webhook) ShouldCallHook(ctx context.Context, h webhook.WebhookAccessor, attr admission.Attributes, o admission.ObjectInterfaces, v VersionedAttributeAccessor) (*WebhookInvocation, *apierrors.StatusError) { matches, matchNsErr := a.namespaceMatcher.MatchNamespaceSelector(h, attr) // Should not return an error here for webhooks which do not apply to the request, even if err is an unexpected scenario. if !matches && matchNsErr == nil { @@ -207,6 +217,25 @@ func (a *Webhook) ShouldCallHook(h webhook.WebhookAccessor, attr admission.Attri return nil, matchObjErr } + matchConditions := h.GetMatchConditions() + if len(matchConditions) > 0 { + versionedAttr, err := v.VersionedAttribute(invocation.Kind) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + + matcher := h.GetCompiledMatcher(a.filterCompiler, a.authorizer) + matchResult := matcher.Match(ctx, versionedAttr, nil) + + if matchResult.Error != nil { + klog.Warningf("Failed evaluating match conditions, failing closed %v: %v", h.GetName(), matchResult.Error) + return nil, apierrors.NewForbidden(attr.GetResource().GroupResource(), attr.GetName(), matchResult.Error) + } else if !matchResult.Matches { + // if no match, always skip webhook + return nil, nil + } + } + return invocation, nil } diff --git a/pkg/admission/plugin/webhook/generic/webhook_test.go b/pkg/admission/plugin/webhook/generic/webhook_test.go index 94d5b162b..64595cca2 100644 --- a/pkg/admission/plugin/webhook/generic/webhook_test.go +++ b/pkg/admission/plugin/webhook/generic/webhook_test.go @@ -17,21 +17,26 @@ limitations under the License. package generic import ( + "context" + "errors" "fmt" "strings" "testing" v1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/cel" "k8s.io/apiserver/pkg/admission/plugin/webhook" + "k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions" "k8s.io/apiserver/pkg/admission/plugin/webhook/predicates/namespace" "k8s.io/apiserver/pkg/admission/plugin/webhook/predicates/object" + "k8s.io/apiserver/pkg/authorization/authorizer" ) func gvr(group, version, resource string) schema.GroupVersionResource { @@ -42,8 +47,55 @@ func gvk(group, version, kind string) schema.GroupVersionKind { return schema.GroupVersionKind{Group: group, Version: version, Kind: kind} } +var _ matchconditions.Matcher = &fakeMatcher{} + +type fakeMatcher struct { + throwError error + matchResult bool +} + +func (f *fakeMatcher) Match(ctx context.Context, versionedAttr *admission.VersionedAttributes, versionedParams runtime.Object) matchconditions.MatchResult { + if f.throwError != nil { + return matchconditions.MatchResult{ + Matches: true, + FailedConditionName: "", + Error: f.throwError, + } + } + return matchconditions.MatchResult{ + Matches: f.matchResult, + FailedConditionName: "", + } +} + +var _ webhook.WebhookAccessor = &fakeWebhookAccessor{} + +type fakeWebhookAccessor struct { + webhook.WebhookAccessor + throwError error + matchResult bool +} + +func (f *fakeWebhookAccessor) GetCompiledMatcher(compiler cel.FilterCompiler, authorizer authorizer.Authorizer) matchconditions.Matcher { + return &fakeMatcher{ + throwError: f.throwError, + matchResult: f.matchResult, + } +} + +var _ VersionedAttributeAccessor = &fakeVersionedAttributeAccessor{} + +type fakeVersionedAttributeAccessor struct{} + +func (v *fakeVersionedAttributeAccessor) VersionedAttribute(gvk schema.GroupVersionKind) (*admission.VersionedAttributes, error) { + return nil, nil +} + func TestShouldCallHook(t *testing.T) { - a := &Webhook{namespaceMatcher: &namespace.Matcher{}, objectMatcher: &object.Matcher{}} + a := &Webhook{ + namespaceMatcher: &namespace.Matcher{}, + objectMatcher: &object.Matcher{}, + } allScopes := v1.AllScopes exactMatch := v1.Exact @@ -83,12 +135,15 @@ func TestShouldCallHook(t *testing.T) { expectCallResource schema.GroupVersionResource expectCallSubresource string expectCallKind schema.GroupVersionKind + matchError error + matchResult bool }{ { - name: "no rules (just write)", - webhook: &v1.ValidatingWebhook{NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1.RuleWithOperations{}}, - attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), - expectCall: false, + name: "no rules (just write)", + webhook: &v1.ValidatingWebhook{NamespaceSelector: &metav1.LabelSelector{}, Rules: []v1.RuleWithOperations{}}, + attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + matchResult: true, }, { name: "invalid kind lookup", @@ -100,9 +155,10 @@ func TestShouldCallHook(t *testing.T) { Operations: []v1.OperationType{"*"}, Rule: v1.Rule{APIGroups: []string{"example.com"}, APIVersions: []string{"v1"}, Resources: []string{"widgets"}, Scope: &allScopes}, }}}, - attrs: admission.NewAttributesRecord(nil, nil, gvk("example.com", "v2", "Widget"), "ns", "name", gvr("example.com", "v2", "widgets"), "", admission.Create, &metav1.CreateOptions{}, false, nil), - expectCall: false, - expectErr: "unknown kind", + attrs: admission.NewAttributesRecord(nil, nil, gvk("example.com", "v2", "Widget"), "ns", "name", gvr("example.com", "v2", "widgets"), "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + expectErr: "unknown kind", + matchResult: true, }, { name: "wildcard rule, match as requested", @@ -118,6 +174,7 @@ func TestShouldCallHook(t *testing.T) { expectCallKind: gvk("apps", "v1", "Deployment"), expectCallResource: gvr("apps", "v1", "deployments"), expectCallSubresource: "", + matchResult: true, }, { name: "specific rules, prefer exact match", @@ -139,6 +196,7 @@ func TestShouldCallHook(t *testing.T) { expectCallKind: gvk("apps", "v1", "Deployment"), expectCallResource: gvr("apps", "v1", "deployments"), expectCallSubresource: "", + matchResult: true, }, { name: "specific rules, match miss", @@ -152,8 +210,9 @@ func TestShouldCallHook(t *testing.T) { Operations: []v1.OperationType{"*"}, Rule: v1.Rule{APIGroups: []string{"apps"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments"}, Scope: &allScopes}, }}}, - attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), - expectCall: false, + attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + matchResult: true, }, { name: "specific rules, exact match miss", @@ -168,8 +227,9 @@ func TestShouldCallHook(t *testing.T) { Operations: []v1.OperationType{"*"}, Rule: v1.Rule{APIGroups: []string{"apps"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments"}, Scope: &allScopes}, }}}, - attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), - expectCall: false, + attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + matchResult: true, }, { name: "specific rules, equivalent match, prefer extensions", @@ -189,6 +249,7 @@ func TestShouldCallHook(t *testing.T) { expectCallKind: gvk("extensions", "v1beta1", "Deployment"), expectCallResource: gvr("extensions", "v1beta1", "deployments"), expectCallSubresource: "", + matchResult: true, }, { name: "specific rules, equivalent match, prefer apps", @@ -208,6 +269,7 @@ func TestShouldCallHook(t *testing.T) { expectCallKind: gvk("apps", "v1beta1", "Deployment"), expectCallResource: gvr("apps", "v1beta1", "deployments"), expectCallSubresource: "", + matchResult: true, }, { @@ -230,6 +292,7 @@ func TestShouldCallHook(t *testing.T) { expectCallKind: gvk("autoscaling", "v1", "Scale"), expectCallResource: gvr("apps", "v1", "deployments"), expectCallSubresource: "scale", + matchResult: true, }, { name: "specific rules, subresource match miss", @@ -243,8 +306,9 @@ func TestShouldCallHook(t *testing.T) { Operations: []v1.OperationType{"*"}, Rule: v1.Rule{APIGroups: []string{"apps"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments", "deployments/scale"}, Scope: &allScopes}, }}}, - attrs: admission.NewAttributesRecord(nil, nil, gvk("autoscaling", "v1", "Scale"), "ns", "name", gvr("apps", "v1", "deployments"), "scale", admission.Create, &metav1.CreateOptions{}, false, nil), - expectCall: false, + attrs: admission.NewAttributesRecord(nil, nil, gvk("autoscaling", "v1", "Scale"), "ns", "name", gvr("apps", "v1", "deployments"), "scale", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + matchResult: true, }, { name: "specific rules, subresource exact match miss", @@ -259,8 +323,9 @@ func TestShouldCallHook(t *testing.T) { Operations: []v1.OperationType{"*"}, Rule: v1.Rule{APIGroups: []string{"apps"}, APIVersions: []string{"v1beta1"}, Resources: []string{"deployments", "deployments/scale"}, Scope: &allScopes}, }}}, - attrs: admission.NewAttributesRecord(nil, nil, gvk("autoscaling", "v1", "Scale"), "ns", "name", gvr("apps", "v1", "deployments"), "scale", admission.Create, &metav1.CreateOptions{}, false, nil), - expectCall: false, + attrs: admission.NewAttributesRecord(nil, nil, gvk("autoscaling", "v1", "Scale"), "ns", "name", gvr("apps", "v1", "deployments"), "scale", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + matchResult: true, }, { name: "specific rules, subresource equivalent match, prefer extensions", @@ -280,6 +345,7 @@ func TestShouldCallHook(t *testing.T) { expectCallKind: gvk("extensions", "v1beta1", "Scale"), expectCallResource: gvr("extensions", "v1beta1", "deployments"), expectCallSubresource: "scale", + matchResult: true, }, { name: "specific rules, subresource equivalent match, prefer apps", @@ -299,12 +365,86 @@ func TestShouldCallHook(t *testing.T) { expectCallKind: gvk("apps", "v1beta1", "Scale"), expectCallResource: gvr("apps", "v1beta1", "deployments"), expectCallSubresource: "scale", + matchResult: true, + }, + { + name: "wildcard rule, match conditions also match", + webhook: &v1.ValidatingWebhook{ + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + Rules: []v1.RuleWithOperations{{ + Operations: []v1.OperationType{"*"}, + Rule: v1.Rule{APIGroups: []string{"*"}, APIVersions: []string{"*"}, Resources: []string{"*"}, Scope: &allScopes}, + }}, + MatchConditions: []v1.MatchCondition{ + { + Name: "test1", + Expression: "test expression", + }, + }, + }, + attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: true, + expectCallKind: gvk("apps", "v1", "Deployment"), + expectCallResource: gvr("apps", "v1", "deployments"), + expectCallSubresource: "", + matchResult: true, + }, + { + name: "wildcard rule, match conditions do not match", + webhook: &v1.ValidatingWebhook{ + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + Rules: []v1.RuleWithOperations{{ + Operations: []v1.OperationType{"*"}, + Rule: v1.Rule{APIGroups: []string{"*"}, APIVersions: []string{"*"}, Resources: []string{"*"}, Scope: &allScopes}, + }}, + MatchConditions: []v1.MatchCondition{ + { + Name: "test1", + Expression: "test expression", + }, + }, + }, + attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + matchResult: false, + }, + { + name: "wildcard rule, match conditions error", + webhook: &v1.ValidatingWebhook{ + NamespaceSelector: &metav1.LabelSelector{}, + ObjectSelector: &metav1.LabelSelector{}, + Rules: []v1.RuleWithOperations{{ + Operations: []v1.OperationType{"*"}, + Rule: v1.Rule{APIGroups: []string{"*"}, APIVersions: []string{"*"}, Resources: []string{"*"}, Scope: &allScopes}, + }}, + MatchConditions: []v1.MatchCondition{ + { + Name: "test1", + Expression: "test expression", + }, + }, + }, + attrs: admission.NewAttributesRecord(nil, nil, gvk("apps", "v1", "Deployment"), "ns", "name", gvr("apps", "v1", "deployments"), "", admission.Create, &metav1.CreateOptions{}, false, nil), + expectCall: false, + expectErr: "deployments.apps \"name\" is forbidden: test error", + expectCallKind: gvk("apps", "v1", "Deployment"), + expectCallResource: gvr("apps", "v1", "deployments"), + expectCallSubresource: "", + matchError: errors.New("test error"), }, } for i, testcase := range testcases { t.Run(testcase.name, func(t *testing.T) { - invocation, err := a.ShouldCallHook(webhook.NewValidatingWebhookAccessor(fmt.Sprintf("webhook-%d", i), fmt.Sprintf("webhook-cfg-%d", i), testcase.webhook), testcase.attrs, interfaces) + fakeWebhook := &fakeWebhookAccessor{ + WebhookAccessor: webhook.NewValidatingWebhookAccessor(fmt.Sprintf("webhook-%d", i), fmt.Sprintf("webhook-cfg-%d", i), testcase.webhook), + matchResult: testcase.matchResult, + throwError: testcase.matchError, + } + + invocation, err := a.ShouldCallHook(context.TODO(), fakeWebhook, testcase.attrs, interfaces, &fakeVersionedAttributeAccessor{}) if err != nil { if len(testcase.expectErr) == 0 { t.Fatal(err) @@ -353,7 +493,7 @@ func (f fakeNamespaceLister) Get(name string) (*corev1.Namespace, error) { if ok { return ns, nil } - return nil, errors.NewNotFound(corev1.Resource("namespaces"), name) + return nil, k8serrors.NewNotFound(corev1.Resource("namespaces"), name) } func BenchmarkShouldCallHookWithComplexSelector(b *testing.B) { @@ -415,13 +555,16 @@ func BenchmarkShouldCallHookWithComplexSelector(b *testing.B) { }, } - wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb) + wbAccessor := &fakeWebhookAccessor{ + WebhookAccessor: webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb), + matchResult: true, + } attrs := admission.NewAttributesRecord(nil, nil, gvk("autoscaling", "v1", "Scale"), "ns", "name", gvr("apps", "v1", "deployments"), "scale", admission.Create, &metav1.CreateOptions{}, false, nil) interfaces := &admission.RuntimeObjectInterfaces{EquivalentResourceMapper: mapper} a := &Webhook{namespaceMatcher: &namespace.Matcher{NamespaceLister: namespaceLister}, objectMatcher: &object.Matcher{}} for i := 0; i < b.N; i++ { - a.ShouldCallHook(wbAccessor, attrs, interfaces) + a.ShouldCallHook(context.TODO(), wbAccessor, attrs, interfaces, nil) } } @@ -483,13 +626,16 @@ func BenchmarkShouldCallHookWithComplexRule(b *testing.B) { wb.Rules = append(wb.Rules, rule) } - wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb) + wbAccessor := &fakeWebhookAccessor{ + WebhookAccessor: webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb), + matchResult: true, + } attrs := admission.NewAttributesRecord(nil, nil, gvk("autoscaling", "v1", "Scale"), "ns", "name", gvr("apps", "v1", "deployments"), "scale", admission.Create, &metav1.CreateOptions{}, false, nil) interfaces := &admission.RuntimeObjectInterfaces{EquivalentResourceMapper: mapper} a := &Webhook{namespaceMatcher: &namespace.Matcher{NamespaceLister: namespaceLister}, objectMatcher: &object.Matcher{}} for i := 0; i < b.N; i++ { - a.ShouldCallHook(wbAccessor, attrs, interfaces) + a.ShouldCallHook(context.TODO(), wbAccessor, attrs, interfaces, &fakeVersionedAttributeAccessor{}) } } @@ -556,12 +702,15 @@ func BenchmarkShouldCallHookWithComplexSelectorAndRule(b *testing.B) { wb.Rules = append(wb.Rules, rule) } - wbAccessor := webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb) + wbAccessor := &fakeWebhookAccessor{ + WebhookAccessor: webhook.NewValidatingWebhookAccessor("webhook", "webhook-cfg", wb), + matchResult: true, + } attrs := admission.NewAttributesRecord(nil, nil, gvk("autoscaling", "v1", "Scale"), "ns", "name", gvr("apps", "v1", "deployments"), "scale", admission.Create, &metav1.CreateOptions{}, false, nil) interfaces := &admission.RuntimeObjectInterfaces{EquivalentResourceMapper: mapper} a := &Webhook{namespaceMatcher: &namespace.Matcher{NamespaceLister: namespaceLister}, objectMatcher: &object.Matcher{}} for i := 0; i < b.N; i++ { - a.ShouldCallHook(wbAccessor, attrs, interfaces) + a.ShouldCallHook(context.TODO(), wbAccessor, attrs, interfaces, nil) } } diff --git a/pkg/admission/plugin/webhook/matchconditions/interface.go b/pkg/admission/plugin/webhook/matchconditions/interface.go new file mode 100644 index 000000000..09468655b --- /dev/null +++ b/pkg/admission/plugin/webhook/matchconditions/interface.go @@ -0,0 +1,36 @@ +/* +Copyright 2023 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 matchconditions + +import ( + "context" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/admission" +) + +type MatchResult struct { + Matches bool + Error error + FailedConditionName string +} + +// Matcher contains logic for converting Evaluations to bool of matches or does not match +type Matcher interface { + // Match is used to take cel evaluations and convert into decisions + Match(ctx context.Context, versionedAttr *admission.VersionedAttributes, versionedParams runtime.Object) MatchResult +} diff --git a/pkg/admission/plugin/webhook/matchconditions/matcher.go b/pkg/admission/plugin/webhook/matchconditions/matcher.go new file mode 100644 index 000000000..09a500dd3 --- /dev/null +++ b/pkg/admission/plugin/webhook/matchconditions/matcher.go @@ -0,0 +1,139 @@ +/* +Copyright 2023 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 matchconditions + +import ( + "context" + "errors" + "fmt" + + "github.com/google/cel-go/cel" + celtypes "github.com/google/cel-go/common/types" + + v1 "k8s.io/api/admissionregistration/v1" + "k8s.io/apimachinery/pkg/runtime" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apiserver/pkg/admission" + admissionmetrics "k8s.io/apiserver/pkg/admission/metrics" + celplugin "k8s.io/apiserver/pkg/admission/plugin/cel" + celconfig "k8s.io/apiserver/pkg/apis/cel" + "k8s.io/apiserver/pkg/authorization/authorizer" + "k8s.io/klog/v2" +) + +var _ celplugin.ExpressionAccessor = &MatchCondition{} + +// MatchCondition contains the inputs needed to compile, evaluate and match a cel expression +type MatchCondition v1.MatchCondition + +func (v *MatchCondition) GetExpression() string { + return v.Expression +} + +func (v *MatchCondition) ReturnTypes() []*cel.Type { + return []*cel.Type{cel.BoolType} +} + +var _ Matcher = &matcher{} + +// matcher evaluates compiled cel expressions and determines if they match the given request or not +type matcher struct { + filter celplugin.Filter + authorizer authorizer.Authorizer + failPolicy v1.FailurePolicyType + matcherType string + objectName string +} + +func NewMatcher(filter celplugin.Filter, authorizer authorizer.Authorizer, failPolicy *v1.FailurePolicyType, matcherType, objectName string) Matcher { + var f v1.FailurePolicyType + if failPolicy == nil { + f = v1.Fail + } else { + f = *failPolicy + } + return &matcher{ + filter: filter, + authorizer: authorizer, + failPolicy: f, + matcherType: matcherType, + objectName: objectName, + } +} + +func (m *matcher) Match(ctx context.Context, versionedAttr *admission.VersionedAttributes, versionedParams runtime.Object) MatchResult { + evalResults, _, err := m.filter.ForInput(ctx, versionedAttr, celplugin.CreateAdmissionRequest(versionedAttr.Attributes), celplugin.OptionalVariableBindings{ + VersionedParams: versionedParams, + Authorizer: m.authorizer, + }, celconfig.RuntimeCELCostBudgetMatchConditions) + + if err != nil { + // filter returning error is unexpected and not an evaluation error so not incrementing metric here + if m.failPolicy == v1.Fail { + return MatchResult{ + Error: err, + } + } else if m.failPolicy == v1.Ignore { + return MatchResult{ + Matches: false, + } + } + //TODO: add default so that if in future we add different failure types it doesn't fall through + } + + errorList := []error{} + for _, evalResult := range evalResults { + matchCondition, ok := evalResult.ExpressionAccessor.(*MatchCondition) + if !ok { + // This shouldnt happen, but if it does treat same as eval error + klog.Error("Invalid type conversion to MatchCondition") + errorList = append(errorList, errors.New(fmt.Sprintf("internal error converting ExpressionAccessor to MatchCondition"))) + continue + } + if evalResult.Error != nil { + errorList = append(errorList, evalResult.Error) + //TODO: what's the best way to handle this metric since its reused by VAP for match conditions + admissionmetrics.Metrics.ObserveMatchConditionEvalError(ctx, m.objectName, m.matcherType) + } + if evalResult.EvalResult == celtypes.False { + // If any condition false, skip calling webhook always + return MatchResult{ + Matches: false, + FailedConditionName: matchCondition.Name, + } + } + } + if len(errorList) > 0 { + // If mix of true and eval errors then resort to fail policy + if m.failPolicy == v1.Fail { + // mix of true and errors with fail policy fail should fail request without calling webhook + err = utilerrors.NewAggregate(errorList) + return MatchResult{ + Error: err, + } + } else if m.failPolicy == v1.Ignore { + // if fail policy ignore then skip call to webhook + return MatchResult{ + Matches: false, + } + } + } + // if no results eval to false, return matches true with list of any errors encountered + return MatchResult{ + Matches: true, + } +} diff --git a/pkg/admission/plugin/webhook/matchconditions/matcher_test.go b/pkg/admission/plugin/webhook/matchconditions/matcher_test.go new file mode 100644 index 000000000..2ba83cbf4 --- /dev/null +++ b/pkg/admission/plugin/webhook/matchconditions/matcher_test.go @@ -0,0 +1,360 @@ +/* +Copyright 2023 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 matchconditions + +import ( + "context" + "errors" + "strings" + "testing" + + v1 "k8s.io/api/admissionregistration/v1" + + celtypes "github.com/google/cel-go/common/types" + "github.com/stretchr/testify/require" + + admissionv1 "k8s.io/api/admission/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/plugin/cel" +) + +var _ cel.Filter = &fakeCelFilter{} + +type fakeCelFilter struct { + evaluations []cel.EvaluationResult + throwError bool +} + +func (f *fakeCelFilter) ForInput(context.Context, *admission.VersionedAttributes, *admissionv1.AdmissionRequest, cel.OptionalVariableBindings, int64) ([]cel.EvaluationResult, int64, error) { + if f.throwError { + return nil, 0, errors.New("test error") + } + return f.evaluations, 0, nil +} + +func (f *fakeCelFilter) CompilationErrors() []error { + return []error{} +} + +func TestMatch(t *testing.T) { + fakeAttr := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "default", "foo", schema.GroupVersionResource{}, "", admission.Create, nil, false, nil) + fakeVersionedAttr, _ := admission.NewVersionedAttributes(fakeAttr, schema.GroupVersionKind{}, nil) + fail := v1.Fail + ignore := v1.Ignore + + cases := []struct { + name string + evaluations []cel.EvaluationResult + throwError bool + shouldMatch bool + returnedName string + failPolicy *v1.FailurePolicyType + expectError string + }{ + { + name: "test single matches", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + }, + shouldMatch: true, + }, + { + name: "test multiple match", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + }, + shouldMatch: true, + }, + { + name: "test empty evals", + evaluations: []cel.EvaluationResult{}, + shouldMatch: true, + }, + { + name: "test single no match", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.False, + ExpressionAccessor: &MatchCondition{ + Name: "test1", + }, + }, + }, + shouldMatch: false, + returnedName: "test1", + }, + { + name: "test multiple no match", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.False, + ExpressionAccessor: &MatchCondition{ + Name: "test1", + }, + }, + { + EvalResult: celtypes.False, + ExpressionAccessor: &MatchCondition{ + Name: "test2", + }, + }, + }, + shouldMatch: false, + returnedName: "test1", + }, + { + name: "test mixed with no match first", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.False, + ExpressionAccessor: &MatchCondition{ + Name: "test1", + }, + }, + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{ + Name: "test2", + }, + }, + }, + shouldMatch: false, + returnedName: "test1", + }, + { + name: "test mixed with no match last", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{ + Name: "test2", + }, + }, + { + EvalResult: celtypes.False, + ExpressionAccessor: &MatchCondition{ + Name: "test1", + }, + }, + }, + shouldMatch: false, + returnedName: "test1", + }, + { + name: "test mixed with no match middle", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{ + Name: "test2", + }, + }, + { + EvalResult: celtypes.False, + ExpressionAccessor: &MatchCondition{ + Name: "test1", + }, + }, + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{ + Name: "test2", + }, + }, + }, + shouldMatch: false, + returnedName: "test1", + }, + { + name: "test error, no fail policy", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + }, + shouldMatch: true, + throwError: true, + expectError: "test error", + }, + { + name: "test error, fail policy fail", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + }, + failPolicy: &fail, + shouldMatch: true, + throwError: true, + expectError: "test error", + }, + { + name: "test error, fail policy ignore", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + }, + failPolicy: &ignore, + shouldMatch: false, + throwError: true, + }, + { + name: "test mix of true, errors and false", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + Error: errors.New("test error"), + ExpressionAccessor: &MatchCondition{}, + }, + { + EvalResult: celtypes.False, + ExpressionAccessor: &MatchCondition{}, + }, + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + Error: errors.New("test error"), + ExpressionAccessor: &MatchCondition{}, + }, + }, + shouldMatch: false, + throwError: false, + }, + { + name: "test mix of true, errors and fail policy not set", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + Error: errors.New("test error"), + ExpressionAccessor: &MatchCondition{}, + }, + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + Error: errors.New("test error"), + ExpressionAccessor: &MatchCondition{}, + }, + }, + shouldMatch: false, + throwError: false, + expectError: "test error", + }, + { + name: "test mix of true, errors and fail policy fail", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + Error: errors.New("test error"), + ExpressionAccessor: &MatchCondition{}, + }, + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + Error: errors.New("test error"), + ExpressionAccessor: &MatchCondition{}, + }, + }, + failPolicy: &fail, + shouldMatch: false, + throwError: false, + expectError: "test error", + }, + { + name: "test mix of true, errors and fail policy ignore", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + Error: errors.New("test error"), + ExpressionAccessor: &MatchCondition{}, + }, + { + EvalResult: celtypes.True, + ExpressionAccessor: &MatchCondition{}, + }, + { + Error: errors.New("test error"), + ExpressionAccessor: &MatchCondition{}, + }, + }, + failPolicy: &ignore, + shouldMatch: false, + throwError: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := NewMatcher(&fakeCelFilter{ + evaluations: tc.evaluations, + throwError: tc.throwError, + }, nil, tc.failPolicy, "test", "testhook") + ctx := context.TODO() + matchResult := m.Match(ctx, fakeVersionedAttr, nil) + + if matchResult.Error != nil { + if len(tc.expectError) == 0 { + t.Fatal(matchResult.Error) + } + if !strings.Contains(matchResult.Error.Error(), tc.expectError) { + t.Fatalf("expected error containing %q, got %s", tc.expectError, matchResult.Error.Error()) + } + return + } else if len(tc.expectError) > 0 { + t.Fatal("expected error but did not get one") + } + if len(tc.expectError) > 0 && matchResult.Error == nil { + t.Errorf("expected error thrown when filter errors") + } + + require.Equal(t, tc.shouldMatch, matchResult.Matches) + require.Equal(t, tc.returnedName, matchResult.FailedConditionName) + }) + } +} diff --git a/pkg/admission/plugin/webhook/mutating/dispatcher.go b/pkg/admission/plugin/webhook/mutating/dispatcher.go index ef46eafb5..c1d1ca6ff 100644 --- a/pkg/admission/plugin/webhook/mutating/dispatcher.go +++ b/pkg/admission/plugin/webhook/mutating/dispatcher.go @@ -26,14 +26,13 @@ import ( jsonpatch "github.com/evanphx/json-patch" "go.opentelemetry.io/otel/attribute" - apiequality "k8s.io/apimachinery/pkg/api/equality" - "k8s.io/klog/v2" - admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer/json" utiljson "k8s.io/apimachinery/pkg/util/json" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -48,6 +47,7 @@ import ( webhookutil "k8s.io/apiserver/pkg/util/webhook" "k8s.io/apiserver/pkg/warning" "k8s.io/component-base/tracing" + "k8s.io/klog/v2" ) const ( @@ -75,6 +75,30 @@ func newMutatingDispatcher(p *Plugin) func(cm *webhookutil.ClientManager) generi } } +var _ generic.VersionedAttributeAccessor = &versionedAttributeAccessor{} + +type versionedAttributeAccessor struct { + versionedAttr *admission.VersionedAttributes + attr admission.Attributes + objectInterfaces admission.ObjectInterfaces +} + +func (v *versionedAttributeAccessor) VersionedAttribute(gvk schema.GroupVersionKind) (*admission.VersionedAttributes, error) { + if v.versionedAttr == nil { + // First call, create versioned attributes + var err error + if v.versionedAttr, err = admission.NewVersionedAttributes(v.attr, gvk, v.objectInterfaces); err != nil { + return nil, apierrors.NewInternalError(err) + } + } else { + // Subsequent call, convert existing versioned attributes to the requested version + if err := admission.ConvertVersionedAttributes(v.versionedAttr, gvk, v.objectInterfaces); err != nil { + return nil, apierrors.NewInternalError(err) + } + } + return v.versionedAttr, nil +} + var _ generic.Dispatcher = &mutatingDispatcher{} func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attributes, o admission.ObjectInterfaces, hooks []webhook.WebhookAccessor) error { @@ -95,19 +119,24 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib defer func() { webhookReinvokeCtx.SetLastWebhookInvocationOutput(attr.GetObject()) }() - var versionedAttr *admission.VersionedAttributes + v := &versionedAttributeAccessor{ + attr: attr, + objectInterfaces: o, + } for i, hook := range hooks { attrForCheck := attr - if versionedAttr != nil { - attrForCheck = versionedAttr + if v.versionedAttr != nil { + attrForCheck = v.versionedAttr } - invocation, statusErr := a.plugin.ShouldCallHook(hook, attrForCheck, o) + + invocation, statusErr := a.plugin.ShouldCallHook(ctx, hook, attrForCheck, o, v) if statusErr != nil { return statusErr } if invocation == nil { continue } + hook, ok := invocation.Webhook.GetMutatingWebhook() if !ok { return fmt.Errorf("mutating webhook dispatch requires v1.MutatingWebhook, but got %T", hook) @@ -121,17 +150,9 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib continue } - if versionedAttr == nil { - // First webhook, create versioned attributes - var err error - if versionedAttr, err = admission.NewVersionedAttributes(attr, invocation.Kind, o); err != nil { - return apierrors.NewInternalError(err) - } - } else { - // Subsequent webhook, convert existing versioned attributes to this webhook's version - if err := admission.ConvertVersionedAttributes(versionedAttr, invocation.Kind, o); err != nil { - return apierrors.NewInternalError(err) - } + versionedAttr, err := v.VersionedAttribute(invocation.Kind) + if err != nil { + return apierrors.NewInternalError(err) } t := time.Now() @@ -203,8 +224,8 @@ func (a *mutatingDispatcher) Dispatch(ctx context.Context, attr admission.Attrib } // convert versionedAttr.VersionedObject to the internal version in the underlying admission.Attributes - if versionedAttr != nil && versionedAttr.VersionedObject != nil && versionedAttr.Dirty { - return o.GetObjectConvertor().Convert(versionedAttr.VersionedObject, versionedAttr.Attributes.GetObject(), nil) + if v.versionedAttr != nil && v.versionedAttr.VersionedObject != nil && v.versionedAttr.Dirty { + return o.GetObjectConvertor().Convert(v.versionedAttr.VersionedObject, v.versionedAttr.Attributes.GetObject(), nil) } return nil diff --git a/pkg/admission/plugin/webhook/validating/dispatcher.go b/pkg/admission/plugin/webhook/validating/dispatcher.go index 19c6cdf14..14312fadd 100644 --- a/pkg/admission/plugin/webhook/validating/dispatcher.go +++ b/pkg/admission/plugin/webhook/validating/dispatcher.go @@ -62,30 +62,51 @@ func newValidatingDispatcher(p *Plugin) func(cm *webhookutil.ClientManager) gene } } +var _ generic.VersionedAttributeAccessor = &versionedAttributeAccessor{} + +type versionedAttributeAccessor struct { + versionedAttrs map[schema.GroupVersionKind]*admission.VersionedAttributes + attr admission.Attributes + objectInterfaces admission.ObjectInterfaces +} + +func (v *versionedAttributeAccessor) VersionedAttribute(gvk schema.GroupVersionKind) (*admission.VersionedAttributes, error) { + if val, ok := v.versionedAttrs[gvk]; ok { + return val, nil + } + versionedAttr, err := admission.NewVersionedAttributes(v.attr, gvk, v.objectInterfaces) + if err != nil { + return nil, err + } + v.versionedAttrs[gvk] = versionedAttr + return versionedAttr, nil +} + var _ generic.Dispatcher = &validatingDispatcher{} 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]*admission.VersionedAttributes{} + versionedAttrAccessor := &versionedAttributeAccessor{ + versionedAttrs: map[schema.GroupVersionKind]*admission.VersionedAttributes{}, + attr: attr, + objectInterfaces: o, + } for _, hook := range hooks { - invocation, statusError := d.plugin.ShouldCallHook(hook, attr, o) + invocation, statusError := d.plugin.ShouldCallHook(ctx, hook, attr, o, versionedAttrAccessor) if statusError != nil { return statusError } if invocation == nil { continue } + relevantHooks = append(relevantHooks, invocation) - // If we already have this version, continue - if _, ok := versionedAttrs[invocation.Kind]; ok { - continue - } - versionedAttr, err := admission.NewVersionedAttributes(attr, invocation.Kind, o) + // VersionedAttr result will be cached and reused later during parallel webhook calls + _, err := versionedAttrAccessor.VersionedAttribute(invocation.Kind) if err != nil { return apierrors.NewInternalError(err) } - versionedAttrs[invocation.Kind] = versionedAttr } if len(relevantHooks) == 0 { @@ -108,7 +129,7 @@ func (d *validatingDispatcher) Dispatch(ctx context.Context, attr admission.Attr go func(invocation *generic.WebhookInvocation, idx int) { ignoreClientCallFailures := false hookName := "unknown" - versionedAttr := versionedAttrs[invocation.Kind] + versionedAttr := versionedAttrAccessor.versionedAttrs[invocation.Kind] // The ordering of these two defers is critical. The wg.Done will release the parent go func to close the errCh // that is used by the second defer to report errors. The recovery and error reporting must be done first. defer wg.Done() diff --git a/pkg/apis/cel/config.go b/pkg/apis/cel/config.go index d05821899..319548cd5 100644 --- a/pkg/apis/cel/config.go +++ b/pkg/apis/cel/config.go @@ -25,6 +25,11 @@ const ( // current RuntimeCELCostBudget gives roughly 1 seconds for the validation RuntimeCELCostBudget = 10000000 + // RuntimeCELCostBudgetMatchConditions is the overall cost budget for runtime CEL validation cost on matchConditions per object with matchConditions + // this is per webhook for validatingwebhookconfigurations and mutatingwebhookconfigurations or per ValidatingAdmissionPolicyBinding + // current RuntimeCELCostBudgetMatchConditions gives roughly 1/4 seconds for the validation + RuntimeCELCostBudgetMatchConditions = 2500000 + // CheckFrequency configures the number of iterations within a comprehension to evaluate // before checking whether the function evaluation has been interrupted CheckFrequency = 100 diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 73351b585..44166447f 100644 --- a/pkg/features/kube_features.go +++ b/pkg/features/kube_features.go @@ -35,6 +35,13 @@ const ( // of code conflicts because changes are more likely to be scattered // across the file. + // owner: @ivelichkovich, @tallclair + // alpha: v1.27 + // kep: https://kep.k8s.io/3716 + // + // Enables usage of MatchConditions fields to use CEL expressions for matching on admission webhooks + AdmissionWebhookMatchConditions featuregate.Feature = "AdmissionWebhookMatchConditions" + // owner: @jefftree @alexzielenski // alpha: v1.26 // @@ -223,8 +230,11 @@ func init() { // To add a new feature, define a key for it above and add it here. The features will be // available throughout Kubernetes binaries. var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ + AggregatedDiscoveryEndpoint: {Default: true, PreRelease: featuregate.Beta}, + AdmissionWebhookMatchConditions: {Default: false, PreRelease: featuregate.Alpha}, + APIListChunking: {Default: true, PreRelease: featuregate.Beta}, APIPriorityAndFairness: {Default: true, PreRelease: featuregate.Beta},