Refactor AdmissionPolicy for code sharing with mutating

[alexzielenski]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

KEP-3962 adds MutatingAdmissionPolicy. This PR refactors the ValidatingAdmissionPolicy code to be useable from mutatingadmissionpolicy, and makes it look a lot like the familiar generic webhook implementation.

ValidatingAdmissionPolicy controller was already split into controller.go and controller_reconcile.go. This separation just happened to be the same separation used for the webhook framework's dispatcher and source concepts; so it was natural to refactor it in the same way.

I created a generic admission Plugin framework (that may also be merged with webhooks eventually? sicne it is very similar) which links together a Source and a Dispatcher. The source is in charge of gathering configurations and digesting them into "Hooks", or in our case matching policies/bindings/params. The dispatcher evaluates the compiled & matched policies/bindings from the source to evaluate them against an admissoin request.

For mutating and validating admission policy, we can share the same PolicySource[P, B] to manage params, matching policies to bindings, and compilation where P, B are the types for the (Validating/Mutating)Admission(Policy/Biinding). Most of the code for controller_reconcile.go was used for this implementation.

The dispatcher for validating/mutating will be different. For validating, it was a small refactor to change controller.go into a dispatcher in the new framework. Mutating dispatcher will come in a future PR.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

/assign @cici37 @jiahuif

Does this PR introduce a user-facing change?

NONE

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

- [KEP-3962]: https://github.com/kubernetes/enhancements/issues/3962

[alexzielenski]

I think this is ready for review @cici37

[jpbetz]

I've reviewed all the implementation code. Are we satisfied with the test coverage?

[jpbetz]

Is this policies update when a non-nil err is returned intentional?

[alexzielenski]

Yes, added godocs to make it more clear. err is non-nil if a policy had a configuration error. We still want those partial policies included in the result to produce errors for the user.

[alexzielenski]

I've reviewed all the implementation code. Are we satisfied with the test coverage?

The current refactor passes all existing tests. I reviewed a combined test coverage output for the unit and integration tests against k8s.io/apiserver/pkg/admission/plugin/policy/...:

profile.cov.zip

80% package coverage. The new files of interest have following coverage:

• policy/generic/policy_source.go: 88.1% (~500 LOC)
• policy/generic/plugin.go: 80.4% (~200 LOC)
• policy/validating/accessor.go: 92..9% (~100 LOC)
• policy/validating/dispatcher.go: 81.5% (~500 LOC)

I reviewed the remaining lines which for the most part are unreachable by precondition. I think coverage is good to continue for now. We will have time later in release to improve it even more.

[jpbetz]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 7fad7602acf583611744a6e2f39445700fa6e502

[deads2k]

Validation and controllers deps cel compiler which moved. The usage didn't change, so we're equivalent

/approve

[BenTheElder]

/approve
for vendor/

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, BenTheElder, cici37, deads2k, jpbetz

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

The pull request process is described here

Needs approval from an approver in each of these files:

• cmd/kube-controller-manager/OWNERS [deads2k,jpbetz]
• pkg/apis/OWNERS [deads2k]
• pkg/controller/OWNERS [deads2k]
• pkg/kubeapiserver/OWNERS [deads2k,jpbetz]
• staging/src/k8s.io/apiserver/OWNERS [deads2k,jpbetz]
• vendor/OWNERS [BenTheElder]

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

[k8s-ci-robot]

@alexzielenski: 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-node-e2e-containerd	b7e70f0	link	unknown	/test pull-kubernetes-node-e2e-containerd
pull-kubernetes-e2e-gce	b7e70f0	link	unknown	/test pull-kubernetes-e2e-gce

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.

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.

[alexzielenski]

/retest