Custom match criteria

[maxsmythe]

Currently blocked/dependent on #116261

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This PR adds custom match conditions per this section of the CEL Admission KEP:

https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3488-cel-admission-control#match-conditions

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added matchConditions field to ValidatingAdmissionPolicy, enabled support for CEL based custom match criteria.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/e80ad39a21e6f1be90e2d514fb33df34fd496814/keps/sig-api-machinery/3488-cel-admission-control#match-conditions

[k8s-ci-robot]

Hi @maxsmythe. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[cici37]

Revert change here?

[cici37]

This should not sit in this file?

[jpbetz]

Once params is wired in through the matcher this should be able to mostly piggy-back off of webhook matchConditions, so I don't see much to do here. Even wiring in params should be timed carefully to avoid inducing excess rebase pain.

Places to consider adding tests while waiting:

• pkg/apis/admissionregistration/validation/validation_test.go - Just a sanity test that matchConditions validation doesn't get broken in the future
• staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go - Maaaybe add a test to make sure the failurePolicy settings are respected in the case of error
• staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go - This is a mini-integration test that covers quite a bit of the code path from controller actuation to validation.

For auditAnnotations (which also adds new expressions to ValidationAdmissionPolicy), I believe these were the places I added test cases before writing some full integration tests (which go in test/integration/apiserver/cel/validatingadmissionpolicy_test.go when you get to that step).

[cici37]

/triage accepted

[liggitt]

validateMatchCondition is currently set to validate for webhook admission, so sets HasParams: false,

are conditions expected to have access to the params var in CEL admission conditions for policies which set params? Do we need to plumb a conditional true to validateMatchCondition for that?

[maxsmythe]

We do expect parameters for validation. I'll need to add some plumbing.

[liggitt]

that'll make the API doc on the MatchCondition expression field interesting to write

[maxsmythe]

Yeah, maybe for now only document it in matchConditions since that is bound directly to VAP types.

[tallclair]

Is there a reason you used the singular validateMatchCondition here rather than validateMatchConditions? This is missing the name uniqueness & max-64 conditions checks.

Also, should we reuse the opts.IgnoreMatchConditions setting for this?

[maxsmythe]

reason for using singular: I don't think the plural version existed when I wrote this, sorry for missing the add.

re: opts:

Happy to re-use the opts programming model for this, though I wouldn't re-use the specific option, since it is a different feature with different gating

[liggitt]

The boolean just controls whether to validate the matchConditions field or not.

For webhooks, that is populated based on whether or not they have changed in an update. You don't have to wire it to a feature gate at all

[maxsmythe]

Oh, I think I misunderstood the match conditions bit, you want to save on the compute if nothing has changed... gotcha. Will add.

[maxsmythe]

Added

[tallclair]

you want to save on the compute if nothing has changed

It also makes it much easier to change validation constraints in the future (e.g. if we relaxed the max number of matchConditions).

[liggitt]

we already know where we're going with this type since we have it present in the v1 API package as well. Should we type alias this to v1.MatchCondition like we did for Rule and ScopeType?

[maxsmythe]

no objection to type aliasing

[maxsmythe]

I'll update PR after Gatekeeper community meeting.

[tallclair]

Is there a reason you used the singular validateMatchCondition here rather than validateMatchConditions? This is missing the name uniqueness & max-64 conditions checks.

Also, should we reuse the opts.IgnoreMatchConditions setting for this?

[tallclair]

A couple other test cases that would be good to add:

• condition w/ params, both valid & invalid variants
• don't ignore conditions if ParamKind changes

[maxsmythe]

added

[tallclair]

When this graduates (to beta? stable?) we should probably add this to the expression docs comment directly (noting that it doesn't apply to webhooks). Not sure if we want to do that while CEL admission is still alpha though.

[maxsmythe]

ack, leaving this as-is for now

[tallclair]

Left a couple nits, but this basically LGTM. Ping me when the comments are addressed, and I'll give it the lgtm.

[liggitt]

does c.filterCompiler.Compile or NewMatcher or the Match() impl do appreciable work even if we have no conditions? should we only pass in a matcher if we actually have conditions to match, and skip the Match() call if that's nil?

[maxsmythe]

Per below, I think this will save on evaluation in the near term (long term we should cache operations like casting the input object as unstructured).

[liggitt]

If we don't have any matchConditions, skipping the matcher construction here and tolerating nil below is easy to understand for now. I'd probably recommend that

[maxsmythe]

added

[liggitt]

related to the above question, is celMatcher always non-nil? should it be nil and should we skip this if we have no matchConditions?

[maxsmythe]

Currently celMatcher is always non-nil, it delegates it's non-trivial logic to filter, which does do some casting of parameters. Matcher, followed by ForInput below:

kubernetes/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/matchconditions/matcher.go

Lines 78 to 139 in 2ae7bc3

	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,
	}
	}

kubernetes/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go

Lines 122 to 222 in 2ae7bc3

	// ForInput evaluates the compiled CEL expressions converting them into CELEvaluations
	// errors per evaluation are returned on the Evaluation object
	// runtimeCELCostBudget was added for testing purpose only. Callers should always use const RuntimeCELCostBudget from k8s.io/apiserver/pkg/apis/cel/config.go as input.
	func (f *filter) ForInput(ctx context.Context, versionedAttr *admission.VersionedAttributes, request *admissionv1.AdmissionRequest, inputs OptionalVariableBindings, runtimeCELCostBudget int64) ([]EvaluationResult, int64, error) {
	// TODO: replace unstructured with ref.Val for CEL variables when native type support is available
	evaluations := make([]EvaluationResult, len(f.compilationResults))
	var err error
	oldObjectVal, err := objectToResolveVal(versionedAttr.VersionedOldObject)
	if err != nil {
	return nil, -1, err
	}
	objectVal, err := objectToResolveVal(versionedAttr.VersionedObject)
	if err != nil {
	return nil, -1, err
	}
	var paramsVal, authorizerVal, requestResourceAuthorizerVal any
	if inputs.VersionedParams != nil {
	paramsVal, err = objectToResolveVal(inputs.VersionedParams)
	if err != nil {
	return nil, -1, err
	}
	}
	if inputs.Authorizer != nil {
	authorizerVal = library.NewAuthorizerVal(versionedAttr.GetUserInfo(), inputs.Authorizer)
	requestResourceAuthorizerVal = library.NewResourceAuthorizerVal(versionedAttr.GetUserInfo(), inputs.Authorizer, versionedAttr)
	}
	requestVal, err := convertObjectToUnstructured(request)
	if err != nil {
	return nil, -1, err
	}
	va := &evaluationActivation{
	object: objectVal,
	oldObject: oldObjectVal,
	params: paramsVal,
	request: requestVal.Object,
	authorizer: authorizerVal,
	requestResourceAuthorizer: requestResourceAuthorizerVal,
	}
	remainingBudget := runtimeCELCostBudget
	for i, compilationResult := range f.compilationResults {
	var evaluation = &evaluations[i]
	if compilationResult.ExpressionAccessor == nil { // in case of placeholder
	continue
	}
	evaluation.ExpressionAccessor = compilationResult.ExpressionAccessor
	if compilationResult.Error != nil {
	evaluation.Error = &cel.Error{
	Type: cel.ErrorTypeInvalid,
	Detail: fmt.Sprintf("compilation error: %v", compilationResult.Error),
	}
	continue
	}
	if compilationResult.Program == nil {
	evaluation.Error = &cel.Error{
	Type: cel.ErrorTypeInternal,
	Detail: fmt.Sprintf("unexpected internal error compiling expression"),
	}
	continue
	}
	t1 := time.Now()
	evalResult, evalDetails, err := compilationResult.Program.ContextEval(ctx, va)
	elapsed := time.Since(t1)
	evaluation.Elapsed = elapsed
	if evalDetails == nil {
	return nil, -1, &cel.Error{
	Type: cel.ErrorTypeInternal,
	Detail: fmt.Sprintf("runtime cost could not be calculated for expression: %v, no further expression will be run", compilationResult.ExpressionAccessor.GetExpression()),
	}
	} else {
	rtCost := evalDetails.ActualCost()
	if rtCost == nil {
	return nil, -1, &cel.Error{
	Type: cel.ErrorTypeInvalid,
	Detail: fmt.Sprintf("runtime cost could not be calculated for expression: %v, no further expression will be run", compilationResult.ExpressionAccessor.GetExpression()),
	}
	} else {
	if *rtCost > math.MaxInt64 || int64(*rtCost) > remainingBudget {
	return nil, -1, &cel.Error{
	Type: cel.ErrorTypeInvalid,
	Detail: fmt.Sprintf("validation failed due to running out of cost budget, no further validation rules will be run"),
	}
	}
	remainingBudget -= int64(*rtCost)
	}
	}
	if err != nil {
	evaluation.Error = &cel.Error{
	Type: cel.ErrorTypeInvalid,
	Detail: fmt.Sprintf("expression '%v' resulted in error: %v", compilationResult.ExpressionAccessor.GetExpression(), err),
	}
	} else {
	evaluation.EvalResult = evalResult
	}
	}
	return evaluations, remainingBudget, nil
	}

Ideally a lot of this would be lazily evaluated and cached (since these processes would be needed for evaluating CEL expressions anyway), but I think toggling will add some performance in the short term.

[liggitt]

nit: allowParamsInMatchConditions might be clearer. it's hard to tell if this means matchConditions are using a params field or if matchConditions are allowed to use a params var.

[maxsmythe]

ack

[liggitt]

looks like update-codegen.sh / update-gofmt.sh need running as well

[maxsmythe]

I believe I've addressed all the outstanding comments

[maxsmythe]

interesting, I can't re-request from both liggitt and tallclair at the same time

[k8s-ci-robot]

@maxsmythe: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-cos-alpha-features	509501a378d3feb948059259d844dd52c603d2aa	link	false	/test pull-kubernetes-e2e-gce-cos-alpha-features
check-dependency-stats	509501a378d3feb948059259d844dd52c603d2aa	link	false	/test check-dependency-stats
pull-kubernetes-conformance-kind-ipv6-parallel	509501a378d3feb948059259d844dd52c603d2aa	link	false	/test pull-kubernetes-conformance-kind-ipv6-parallel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[maxsmythe]

It looks like some auto-gen stuff failed even though update-codegen.sh yielded no results, re-running full make update

[maxsmythe]

Codegen re-ran

[liggitt]

It looks like some auto-gen stuff failed even though update-codegen.sh yielded no results, re-running full make update

oh, looks like update-openapi-spec isn't included in update-codegen

[liggitt]

/approve
API and validation lgtm
squash at will
@tallclair has lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cici37, liggitt, maxsmythe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• api/OWNERS [liggitt]
• pkg/apis/OWNERS [liggitt]
• pkg/generated/openapi/OWNERS [liggitt]
• staging/src/k8s.io/api/OWNERS [liggitt]
• staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/OWNERS [cici37,liggitt]
• staging/src/k8s.io/client-go/applyconfigurations/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cici37]

Nothing is merging blocker. LGTM. Thank you!

[cici37]

nit: ignoreValidatingAdmissionPolicyMatchConditions?
I understand the comment tried to say that we set opt ignoreMatchConditions to true if any new expressions are added while updating but it might not so clear as the func name doesn't match the comment :). Maybe something like ignoreValidatingAdmissionPolicyMatchConditions returns true if both paramsKind and matchConditions remain the same while updating hence the update in ValidatingAdmissionPolicySpec will skip matchConditions. ?

[maxsmythe]

In the spirit of making comments semantically valuable and avoiding describing logic, how about:

"ignoreValidatingAdmissionPolicyMatchConditions returns true if there have been no updates that could invalidate previously-valid match conditions"?

[cici37]

Sure! 🤗

[cici37]

nit: do we wanna check the len before construct matchExpressionAccessors and matchconditions.Matcher? something like:

matchConditions := definitionInfo.lastReconciledValue.Spec.MatchConditions
var matcher matchconditions.Matcher = nil
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)
}

[maxsmythe]

Yeah, that seems a bit clearer, changing.

[tallclair]

/lgtm
/label tide/merge-method-squash

[k8s-ci-robot]

LGTM label has been added.

Details

Git tree hash: 3207223f1b9306ae73616112359fb37f50019ce8