fix: Move K8scel driver from framework

[abhipatnala]

trafficstars

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged): Fixes #3500

Special notes for your reviewer:

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 48.68421% with 351 lines in your changes missing coverage. Please review.

Project coverage is 47.58%. Comparing base (3350319) to head (6f598fa). Report is 171 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/drivers/k8scel/schema/schema.go	33.89%	109 Missing and 8 partials :warning:
pkg/drivers/k8scel/driver.go	29.13%	100 Missing and 7 partials :warning:
pkg/drivers/k8scel/transform/vap_util.go	0.00%	51 Missing :warning:
pkg/drivers/k8scel/transform/make_vap_objects.go	74.52%	18 Missing and 9 partials :warning:
pkg/drivers/k8scel/transform/request.go	69.76%	21 Missing and 5 partials :warning:
pkg/drivers/k8scel/transform/cel_snippets.go	87.50%	12 Missing :warning:
pkg/drivers/k8scel/args.go	0.00%	6 Missing :warning:
pkg/drivers/k8scel/new.go	55.55%	3 Missing and 1 partial :warning:
pkg/controller/constraint/constraint_controller.go	0.00%	1 Missing :warning:

:exclamation: There is a different number of reports uploaded between BASE (3350319) and HEAD (6f598fa). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (3350319)	HEAD (6f598fa)
unittests	2	1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3570      +/-   ##
==========================================
- Coverage   54.49%   47.58%   -6.92%     
==========================================
  Files         134      236     +102     
  Lines       12329    19717    +7388     
==========================================
+ Hits         6719     9382    +2663     
- Misses       5116     9456    +4340     
- Partials      494      879     +385     

Flag	Coverage Δ
unittests	47.58% <48.68%> (-6.92%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[ritazh]

is k8scel driver going to be removed from frameworks after these changes are in GK?

Ideally this PR should test against a PR in framework that removes the k8scel driver to ensure all k8scel changes are migrated from framework to GK.

I think we should move "VAP related" code - https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/constraint/constraint_controller.go#L623 to EOF - under transform. wdyt? @open-policy-agent/gatekeeper-maintainers

+1 and https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/constrainttemplate/constrainttemplate_controller.go#L750 will need to be moved too

[JaydipGabani]

is k8scel driver going to be removed from frameworks after these changes are in GK?

I think we should move "VAP related" code - https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/constraint/constraint_controller.go#L623 to EOF - under transform. wdyt? @open-policy-agent/gatekeeper-maintainers

@abhipatnala can you move this code as well and the code block suggested by @ritazh in above comment?

[abhipatnala]

is k8scel driver going to be removed from frameworks after these changes are in GK?

Ideally this PR should test against a PR in framework that removes the k8scel driver to ensure all k8scel changes are migrated from framework to GK.

I think we should move "VAP related" code - https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/constraint/constraint_controller.go#L623 to EOF - under transform. wdyt? @open-policy-agent/gatekeeper-maintainers

+1 and https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/constrainttemplate/constrainttemplate_controller.go#L750 will need to be moved too

I have updated the pr with those changes @ritazh @JaydipGabani

[maxsmythe]

Actually, reflecting on moving vapForVersion and vapBindingForVersion, I think those should stay with the controller code.

This is because they are entirely used by the controllers as a way to govern which API version they use to communicate with the API server and are tightly coupled to the implementation of the controllers themselves.

Under the theory that centralized code should have official versions that it supports (usually versionless), we should avoid introducing versioning semantics anywhere other than the edge (i.e. immediately before requests are sent to K8s).

Arguably this is also true for the IsVapAPIEnabled as well, but at least there the code is shared, so having a common import path makes sense. I don't like the idea of the drivers/k8scel package interacting with the API server directly (this makes its functionality selectively portable to other use cases like Gator), but there is no great central owner. Leaving it in constraint controller also works.

[maxsmythe]

NOT moving the functions defined in the constraint/template controller would also limit merge conflicts with Jaydip's PR adding time delay to binding creation.