From 41adff8c9379be99de9f18859328c33df1df3f24 Mon Sep 17 00:00:00 2001 From: Max Smythe Date: Wed, 15 Mar 2023 17:23:15 -0700 Subject: [PATCH] Custom match criteria (#116350) * Add custom match conditions for CEL admission This PR is based off of, and dependent on the following PR: https://github.com/kubernetes/kubernetes/pull/116261 Signed-off-by: Max Smythe * run `make update` Signed-off-by: Max Smythe * Fix unit tests Signed-off-by: Max Smythe * Fix unit tests Signed-off-by: Max Smythe * Update compatibility test data Signed-off-by: Max Smythe * Revert "Update compatibility test data" This reverts commit 312ba7f9e74e0ec4a7ac1f07bf575479c608af28. * Allow params during validation; make match conditions optional Signed-off-by: Max Smythe * Add conditional ignoring of matcher CEL expression validation on update Signed-off-by: Max Smythe * Run codegen Signed-off-by: Max Smythe * Add more validation tests Signed-off-by: Max Smythe * Short-circuit CEL matcher when no matchers specified Signed-off-by: Max Smythe * Run codegen Signed-off-by: Max Smythe * Address review comments Signed-off-by: Max Smythe --------- Signed-off-by: Max Smythe Kubernetes-commit: e5fd204c33e90a7e8f5a0ee70242f1296a5ec7af --- go.mod | 12 ++--- go.sum | 12 ++--- .../admission_test.go | 6 +-- .../controller_reconcile.go | 16 +++++- .../validatingadmissionpolicy/validator.go | 25 ++++++++- .../validator_test.go | 53 ++++++++++++++++++- 6 files changed, 104 insertions(+), 20 deletions(-) diff --git a/go.mod b/go.mod index ad69561e5..bbdfc7796 100644 --- a/go.mod +++ b/go.mod @@ -42,12 +42,12 @@ 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-20230315055831-abe66f57fdb1 + k8s.io/api v0.0.0-20230316002315-c80582ebe125 k8s.io/apimachinery v0.0.0-20230315054728-8d1258da8f38 - k8s.io/client-go v0.0.0-20230315061912-38589731da69 + k8s.io/client-go v0.0.0-20230316040718-4666344cbcd7 k8s.io/component-base v0.0.0-20230315065615-6b9bb8ecc3d0 k8s.io/klog/v2 v2.90.1 - k8s.io/kms v0.0.0-20230315071541-54e6d3479bfc + k8s.io/kms v0.0.0-20230315071547-f5c193c64781 k8s.io/kube-openapi v0.0.0-20230308215209-15aac26d736a k8s.io/utils v0.0.0-20230209194617-a36077c30491 sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.1.1 @@ -124,9 +124,9 @@ require ( ) replace ( - k8s.io/api => k8s.io/api v0.0.0-20230315032826-0b4c449988b1 + k8s.io/api => k8s.io/api v0.0.0-20230316002315-c80582ebe125 k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230315054728-8d1258da8f38 - k8s.io/client-go => k8s.io/client-go v0.0.0-20230315061912-38589731da69 + k8s.io/client-go => k8s.io/client-go v0.0.0-20230316040718-4666344cbcd7 k8s.io/component-base => k8s.io/component-base v0.0.0-20230315065615-6b9bb8ecc3d0 - k8s.io/kms => k8s.io/kms v0.0.0-20230315071541-54e6d3479bfc + k8s.io/kms => k8s.io/kms v0.0.0-20230315071547-f5c193c64781 ) diff --git a/go.sum b/go.sum index 7f912fca6..6db25dd94 100644 --- a/go.sum +++ b/go.sum @@ -878,18 +878,18 @@ 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-20230315032826-0b4c449988b1 h1:wlCdY1kqV0RkfnfRr4mEZ3fGJ1VvLelr5Q2vCnCICIo= -k8s.io/api v0.0.0-20230315032826-0b4c449988b1/go.mod h1:aZ6MBt4NMLXSxkSKFkoDaP4hTutnZIvH5dCSpOis9g4= +k8s.io/api v0.0.0-20230316002315-c80582ebe125 h1:sNLUUpJNxIYmttU1YQIm4nhSD2jK3wOkSQVsqhlFh2A= +k8s.io/api v0.0.0-20230316002315-c80582ebe125/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-20230315061912-38589731da69 h1:LnTXY9Akksk/aAmbebKIaC0doqk1aKbZQA3OoAd0BB0= -k8s.io/client-go v0.0.0-20230315061912-38589731da69/go.mod h1:b0alWGtfu+BI7XZwwdOHJIsr7aDjKf3ANThw8Sr+tw8= +k8s.io/client-go v0.0.0-20230316040718-4666344cbcd7 h1:94RqmF9IE9dqxNjr6CRIlO+lVabE46SzrOptrmHAiCc= +k8s.io/client-go v0.0.0-20230316040718-4666344cbcd7/go.mod h1:L61adAiamj+NzecNEOyWV9fYq4VVrOCcn2enXFUiJL8= 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= k8s.io/klog/v2 v2.90.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= -k8s.io/kms v0.0.0-20230315071541-54e6d3479bfc h1:Es1pSsyPK+D9zkWXy4/VVQoZATfbQ5j8xZvKj3I3rmA= -k8s.io/kms v0.0.0-20230315071541-54e6d3479bfc/go.mod h1:JHGAns80GkaZvH3pP7YV53PCFo3eNnoD5jsQ2HL03TM= +k8s.io/kms v0.0.0-20230315071547-f5c193c64781 h1:LH9Z43UgXQkuRc1ImiT3F3LoOfj9Xz0r1BP94+uYpys= +k8s.io/kms v0.0.0-20230315071547-f5c193c64781/go.mod h1:9rk1ftD6YM/h1KCjaoIZ8lv1CYvaCcuo57ZIWqiHHGI= k8s.io/kube-openapi v0.0.0-20230308215209-15aac26d736a h1:gmovKNur38vgoWfGtP5QOGNOA7ki4n6qNYoFAgMlNvg= k8s.io/kube-openapi v0.0.0-20230308215209-15aac26d736a/go.mod h1:y5VtZWM9sHHc2ZodIH/6SHzXj+TPU5USoA8lcIeKEKY= k8s.io/utils v0.0.0-20230209194617-a36077c30491 h1:r0BAOLElQnnFhE/ApUsg3iHdVYYPBjNSSOMowRZxxsY= diff --git a/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go b/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go index 6b364436f..7ed56b12b 100644 --- a/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go +++ b/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go @@ -28,8 +28,6 @@ import ( celgo "github.com/google/cel-go/cel" "github.com/stretchr/testify/require" - "k8s.io/klog/v2" - admissionv1 "k8s.io/api/admission/v1" admissionRegistrationv1 "k8s.io/api/admissionregistration/v1" "k8s.io/api/admissionregistration/v1alpha1" @@ -47,6 +45,7 @@ import ( "k8s.io/apiserver/pkg/admission/initializer" "k8s.io/apiserver/pkg/admission/plugin/cel" "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/internal/generic" + "k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions" auditinternal "k8s.io/apiserver/pkg/apis/audit" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/features" @@ -57,6 +56,7 @@ import ( clienttesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" "k8s.io/component-base/featuregate" + "k8s.io/klog/v2" ) var ( @@ -418,7 +418,7 @@ func setupTestCommon(t *testing.T, compiler cel.FilterCompiler, matcher Matcher, // Override compiler used by controller for tests controller = handler.evaluator.(*celAdmissionController) controller.policyController.filterCompiler = compiler - controller.policyController.newValidator = func(validationFilter, auditAnnotationFilter, messageFilter cel.Filter, fail *admissionRegistrationv1.FailurePolicyType, authorizer authorizer.Authorizer) Validator { + controller.policyController.newValidator = func(validationFilter cel.Filter, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.Filter, fail *admissionRegistrationv1.FailurePolicyType, authorizer authorizer.Authorizer) Validator { f := validationFilter.(*fakeFilter) v := validatorMap[f.keyId] v.validationFilter = f diff --git a/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go b/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go index 4d2671c08..296ac416a 100644 --- a/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go +++ b/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go @@ -35,6 +35,7 @@ import ( celmetrics "k8s.io/apiserver/pkg/admission/cel" "k8s.io/apiserver/pkg/admission/plugin/cel" "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/internal/generic" + "k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions" celconfig "k8s.io/apiserver/pkg/apis/cel" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/client-go/dynamic" @@ -99,7 +100,7 @@ type policyController struct { authz authorizer.Authorizer } -type newValidator func(validationFilter cel.Filter, auditAnnotationFilter cel.Filter, messageFilter cel.Filter, failurePolicy *v1.FailurePolicyType, authorizer authorizer.Authorizer) Validator +type newValidator func(validationFilter cel.Filter, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.Filter, failurePolicy *v1.FailurePolicyType, authorizer authorizer.Authorizer) Validator func newPolicyController( restMapper meta.RESTMapper, @@ -503,11 +504,22 @@ func (c *policyController) latestPolicyData() []policyData { } optionalVars := cel.OptionalVariableDeclarations{HasParams: hasParam, HasAuthorizer: true} expressionOptionalVars := cel.OptionalVariableDeclarations{HasParams: hasParam, HasAuthorizer: false} + failurePolicy := convertv1alpha1FailurePolicyTypeTov1FailurePolicyType(definitionInfo.lastReconciledValue.Spec.FailurePolicy) + var matcher matchconditions.Matcher = nil + matchConditions := definitionInfo.lastReconciledValue.Spec.MatchConditions + if len(matchConditions) > 0 { + matchExpressionAccessors := make([]cel.ExpressionAccessor, len(matchConditions)) + for i := range matchConditions { + matchExpressionAccessors[i] = (*matchconditions.MatchCondition)(&matchConditions[i]) + } + matcher = matchconditions.NewMatcher(c.filterCompiler.Compile(matchExpressionAccessors, optionalVars, celconfig.PerCallLimit), c.authz, failurePolicy, "validatingadmissionpolicy", definitionInfo.lastReconciledValue.Name) + } bindingInfo.validator = c.newValidator( c.filterCompiler.Compile(convertv1alpha1Validations(definitionInfo.lastReconciledValue.Spec.Validations), optionalVars, celconfig.PerCallLimit), + matcher, c.filterCompiler.Compile(convertv1alpha1AuditAnnotations(definitionInfo.lastReconciledValue.Spec.AuditAnnotations), optionalVars, celconfig.PerCallLimit), c.filterCompiler.Compile(convertV1Alpha1MessageExpressions(definitionInfo.lastReconciledValue.Spec.Validations), expressionOptionalVars, celconfig.PerCallLimit), - convertv1alpha1FailurePolicyTypeTov1FailurePolicyType(definitionInfo.lastReconciledValue.Spec.FailurePolicy), + failurePolicy, c.authz, ) } diff --git a/pkg/admission/plugin/validatingadmissionpolicy/validator.go b/pkg/admission/plugin/validatingadmissionpolicy/validator.go index 0b19158a1..448750c91 100644 --- a/pkg/admission/plugin/validatingadmissionpolicy/validator.go +++ b/pkg/admission/plugin/validatingadmissionpolicy/validator.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/plugin/cel" + "k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions" celconfig "k8s.io/apiserver/pkg/apis/cel" "k8s.io/apiserver/pkg/authorization/authorizer" apiservercel "k8s.io/apiserver/pkg/cel" @@ -36,6 +37,7 @@ import ( // validator implements the Validator interface type validator struct { + celMatcher matchconditions.Matcher validationFilter cel.Filter auditAnnotationFilter cel.Filter messageFilter cel.Filter @@ -43,8 +45,9 @@ type validator struct { authorizer authorizer.Authorizer } -func NewValidator(validationFilter, auditAnnotationFilter, messageFilter cel.Filter, failPolicy *v1.FailurePolicyType, authorizer authorizer.Authorizer) Validator { +func NewValidator(validationFilter cel.Filter, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.Filter, failPolicy *v1.FailurePolicyType, authorizer authorizer.Authorizer) Validator { return &validator{ + celMatcher: celMatcher, validationFilter: validationFilter, auditAnnotationFilter: auditAnnotationFilter, messageFilter: messageFilter, @@ -77,6 +80,26 @@ func (v *validator) Validate(ctx context.Context, versionedAttr *admission.Versi f = *v.failPolicy } + if v.celMatcher != nil { + matchResults := v.celMatcher.Match(ctx, versionedAttr, versionedParams) + if matchResults.Error != nil { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: policyDecisionActionForError(f), + Evaluation: EvalError, + Message: matchResults.Error.Error(), + }, + }, + } + } + + // if preconditions are not met, then do not return any validations + if !matchResults.Matches { + return ValidateResult{} + } + } + optionalVars := cel.OptionalVariableBindings{VersionedParams: versionedParams, Authorizer: v.authorizer} expressionOptionalVars := cel.OptionalVariableBindings{VersionedParams: versionedParams} admissionRequest := cel.CreateAdmissionRequest(versionedAttr.Attributes) diff --git a/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go b/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go index 33e1796f8..9d5101ec7 100644 --- a/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go +++ b/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go @@ -23,16 +23,17 @@ import ( "strings" "testing" - "github.com/stretchr/testify/require" - celtypes "github.com/google/cel-go/common/types" + "github.com/stretchr/testify/require" admissionv1 "k8s.io/api/admission/v1" v1 "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "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/matchconditions" celconfig "k8s.io/apiserver/pkg/apis/cel" apiservercel "k8s.io/apiserver/pkg/cel" ) @@ -61,6 +62,17 @@ func (f *fakeCelFilter) CompilationErrors() []error { return []error{} } +var _ matchconditions.Matcher = &fakeCELMatcher{} + +type fakeCELMatcher struct { + error error + matches bool +} + +func (f *fakeCELMatcher) Match(ctx context.Context, versionedAttr *admission.VersionedAttributes, versionedParams runtime.Object) matchconditions.MatchResult { + return matchconditions.MatchResult{Matches: f.matches, FailedConditionName: "placeholder", Error: f.error} +} + func TestValidate(t *testing.T) { ignore := v1.Ignore fail := v1.Fail @@ -74,6 +86,7 @@ func TestValidate(t *testing.T) { cases := []struct { name string failPolicy *v1.FailurePolicyType + matcher matchconditions.Matcher evaluations []cel.EvaluationResult messageEvaluations []cel.EvaluationResult auditEvaluations []cel.EvaluationResult @@ -819,11 +832,46 @@ func TestValidate(t *testing.T) { }, costBudget: 1, // shared between expression and messageExpression, needs 1 + 1 = 2 in total }, + { + name: "no match surpresses failure", + matcher: &fakeCELMatcher{matches: false}, + evaluations: []cel.EvaluationResult{ + { + Error: errors.New("expected"), + ExpressionAccessor: &ValidationCondition{}, + }, + }, + policyDecision: []PolicyDecision{}, + failPolicy: &fail, + }, + { + name: "match error => presumed match", + matcher: &fakeCELMatcher{matches: true, error: fmt.Errorf("test error")}, + evaluations: []cel.EvaluationResult{ + { + Error: errors.New("expected"), + ExpressionAccessor: &ValidationCondition{}, + }, + }, + policyDecision: []PolicyDecision{ + { + Action: ActionDeny, + }, + }, + failPolicy: &fail, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { + var matcher matchconditions.Matcher + if tc.matcher == nil { + matcher = &fakeCELMatcher{matches: true} + } else { + matcher = tc.matcher + } v := validator{ failPolicy: tc.failPolicy, + celMatcher: matcher, validationFilter: &fakeCelFilter{ evaluations: tc.evaluations, throwError: tc.throwError, @@ -884,6 +932,7 @@ func TestContextCanceled(t *testing.T) { f := fc.Compile([]cel.ExpressionAccessor{&ValidationCondition{Expression: "[1,2,3,4,5,6,7,8,9,10].map(x, [1,2,3,4,5,6,7,8,9,10].map(y, x*y)) == []"}}, cel.OptionalVariableDeclarations{HasParams: false, HasAuthorizer: false}, celconfig.PerCallLimit) v := validator{ failPolicy: &fail, + celMatcher: &fakeCELMatcher{matches: true}, validationFilter: f, messageFilter: f, auditAnnotationFilter: &fakeCelFilter{