fix: Move K8scel driver from framework #3570

abhipatnala · 2024-10-03T21:22:10Z

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 · 2024-10-03T21:32:07Z

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 ⚠️
pkg/drivers/k8scel/driver.go	29.13%	100 Missing and 7 partials ⚠️
pkg/drivers/k8scel/transform/vap_util.go	0.00%	51 Missing ⚠️
pkg/drivers/k8scel/transform/make_vap_objects.go	74.52%	18 Missing and 9 partials ⚠️
pkg/drivers/k8scel/transform/request.go	69.76%	21 Missing and 5 partials ⚠️
pkg/drivers/k8scel/transform/cel_snippets.go	87.50%	12 Missing ⚠️
pkg/drivers/k8scel/args.go	0.00%	6 Missing ⚠️
pkg/drivers/k8scel/new.go	55.55%	3 Missing and 1 partial ⚠️
pkg/controller/constraint/constraint_controller.go	0.00%	1 Missing ⚠️

❗ 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%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

ritazh · 2024-10-07T19:50:50Z

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

ritazh · 2024-10-18T23:07:08Z

go.mod

@@ -2,6 +2,8 @@ module github.com/open-policy-agent/gatekeeper/v3

 go 1.22.0

+replace github.com/open-policy-agent/frameworks/constraint v0.0.0-20240927180816-0f64229c5539 => github.com/abhipatnala/frameworks/constraint v0.0.0-20241010172729-61e15952c5c5


@abhipatnala can you pls open a PR on framework as well so we can review and get it merged before merging this one? thanks!

@ritazh Here is the Pr for removing cel driver from framework: open-policy-agent/frameworks#486

@abhipatnala PTAL and remove the go.mod override 😄

JaydipGabani · 2024-10-22T17:58:34Z

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?

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

…ginal repo for testing Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

… once we fallback to original framwork repo Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

abhipatnala · 2024-10-23T22:26:03Z

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 · 2024-10-23T23:25:45Z

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 · 2024-10-23T23:26:52Z

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.

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

JaydipGabani · 2024-10-29T01:09:47Z

pkg/controller/constraint/constraint_controller.go

-var GroupVersion *schema.GroupVersion
+// var VapAPIEnabled *bool
+
+// var GroupVersion *schema.GroupVersion


pls remove commented code.

JaydipGabani · 2024-10-29T01:10:44Z

pkg/controller/constraint/constraint_controller.go

-	return false, nil
-}
-
-func vapBindingForVersion(gvk schema.GroupVersion) (client.Object, error) {


I agree with @maxsmythe on

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

Let's not move there two methods.

JaydipGabani · 2024-10-29T01:11:05Z

pkg/controller/constrainttemplate/constrainttemplate_controller.go

@@ -747,17 +747,6 @@ func makeGvk(kind string) schema.GroupVersionKind {
 	}
 }

-func vapForVersion(gvk *schema.GroupVersion) (client.Object, error) {


I agree with @maxsmythe on

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

Let's not move there two methods.

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

JaydipGabani

Two changes, otherwise LGTM.

JaydipGabani · 2024-10-30T17:13:35Z

pkg/controller/constrainttemplate/constrainttemplate_controller.go

v1beta1ToV1 should be moved to transform probably under make_vap_object.go, since it acts as "constructor" for v1 VAP.

JaydipGabani · 2024-10-30T17:15:20Z

pkg/controller/constraint/constraint_controller.go

v1beta1ToV1 should be moved to transform probably under make_vap_object.go, since it acts as "constructor" for v1 VAPB.

We should probably just wait for the scheme.Convert() bug to be fixed, as that's the "proper K8s" way to convert across versions. In general, core libraries should deal only with unversioned representations.

This has the added advantage of reducing merge conflicts with your PR.

If that's the case, I am good with the changes. @ritazh @sozercan PTAL.

I'm ok with leaving it as is. but can we add a TODO comment for the bug and open an issue so we can come back to it later to clean it up? @JaydipGabani

I added the TODO -

gatekeeper/pkg/controller/constrainttemplate/constrainttemplate_controller.go

Line 792 in d17700d

// TODO(jgabani): Use r.scheme.Convert to convert from v1beta1 to v1 once the conversion bug is fixed - https://github.com/kubernetes/kubernetes/issues/126582

and had opened an issue - kubernetes/kubernetes#126582

This is the response I got "Conversion is only defined to the internal "hub" API type, not directly from one external version to another, and is only done by the api server, not by clients." I will follow up with api-server to see if this is something that can be fixed.

hmm then sounds like its not a bug. not a blocker for this PR tho.

maxsmythe

LGTM once gomod override is removed.

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

ritazh · 2024-11-04T20:29:42Z

go.mod

@@ -15,7 +15,8 @@ require (
 	github.com/google/uuid v1.6.0
 	github.com/onsi/gomega v1.34.1
 	github.com/open-policy-agent/cert-controller v0.11.0
-	github.com/open-policy-agent/frameworks/constraint v0.0.0-20240927180816-0f64229c5539
+	github.com/open-policy-agent/frameworks/constraint v0.0.0-20241007142041-e84361fed758


@abhipatnala this should point to e78c8abd754aa12538eb21ee0c145cbe5465569d

abhipatnala requested a review from a team as a code owner October 3, 2024 21:22

abhipatnala changed the title ~~Fix: Move K8scel driver from framework~~ fix: Move K8scel driver from framework Oct 3, 2024

JaydipGabani reviewed Oct 7, 2024

View reviewed changes

abhipatnala force-pushed the move_cel_driver branch 4 times, most recently from 639f1b5 to 6efad9d Compare October 14, 2024 20:55

ritazh reviewed Oct 18, 2024

View reviewed changes

Avinash Patnala added 4 commits October 23, 2024 17:37

copy k8scel driver from framework

ebb08b7

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

update references of cel driver in codebase

94cad3f

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

deleted cel driver from forked framework repo and replace it with ori…

78c4ef4

…ginal repo for testing Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

sync vendor mods by running go mod vendor, this will be reverted back…

99c98e9

… once we fallback to original framwork repo Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

abhipatnala force-pushed the move_cel_driver branch from 6efad9d to 99c98e9 Compare October 23, 2024 17:37

refactor vap code to util

eeee348

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

abhipatnala force-pushed the move_cel_driver branch from 71230fd to 80c7275 Compare October 23, 2024 19:15

update test cases

fcde683

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

abhipatnala force-pushed the move_cel_driver branch from 80c7275 to fcde683 Compare October 23, 2024 19:19

use vap binding in constraint controller

78f720f

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

JaydipGabani requested changes Oct 29, 2024

View reviewed changes

Avinash Patnala added 3 commits October 29, 2024 17:34

move back vap binding methods

02c4374

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

Merge branch 'master' into move_cel_driver

617d47f

remove commented code

d17700d

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

ritazh added this to the v3.18.0 milestone Oct 30, 2024

JaydipGabani requested changes Oct 30, 2024

View reviewed changes

maxsmythe mentioned this pull request Oct 30, 2024

feat: remove cel driver from framework open-policy-agent/frameworks#486

Merged

JaydipGabani approved these changes Oct 30, 2024

View reviewed changes

ritazh approved these changes Oct 31, 2024

View reviewed changes

maxsmythe approved these changes Nov 1, 2024

View reviewed changes

remove replacement of constraint framework repo

6f598fa

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

ritazh reviewed Nov 4, 2024

View reviewed changes

maxsmythe merged commit 682a493 into open-policy-agent:master Nov 4, 2024
20 checks passed

BrewTestBot mentioned this pull request Dec 12, 2024

gator 3.18.0 Homebrew/homebrew-core#200877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Move K8scel driver from framework #3570

fix: Move K8scel driver from framework #3570

abhipatnala commented Oct 3, 2024

codecov-commenter commented Oct 3, 2024 •

edited

Loading

JaydipGabani left a comment •

edited

Loading

ritazh commented Oct 7, 2024

ritazh Oct 18, 2024

abhipatnala Oct 21, 2024

JaydipGabani Nov 1, 2024

JaydipGabani commented Oct 22, 2024

abhipatnala commented Oct 23, 2024 •

edited

Loading

maxsmythe commented Oct 23, 2024

maxsmythe commented Oct 23, 2024

JaydipGabani Oct 29, 2024

abhipatnala Oct 29, 2024

JaydipGabani Oct 29, 2024

abhipatnala Oct 29, 2024

JaydipGabani Oct 29, 2024

abhipatnala Oct 29, 2024

JaydipGabani left a comment

JaydipGabani Oct 30, 2024

JaydipGabani Oct 30, 2024

maxsmythe Oct 30, 2024

JaydipGabani Oct 30, 2024

ritazh Oct 30, 2024 •

edited

Loading

JaydipGabani Oct 31, 2024

ritazh Oct 31, 2024

maxsmythe left a comment

ritazh Nov 4, 2024

		@@ -2,6 +2,8 @@ module github.com/open-policy-agent/gatekeeper/v3

		go 1.22.0

		replace github.com/open-policy-agent/frameworks/constraint v0.0.0-20240927180816-0f64229c5539 => github.com/abhipatnala/frameworks/constraint v0.0.0-20241010172729-61e15952c5c5

fix: Move K8scel driver from framework #3570

fix: Move K8scel driver from framework #3570

Conversation

abhipatnala commented Oct 3, 2024

codecov-commenter commented Oct 3, 2024 • edited Loading

Codecov Report

JaydipGabani left a comment • edited Loading

Choose a reason for hiding this comment

ritazh commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydipGabani commented Oct 22, 2024

abhipatnala commented Oct 23, 2024 • edited Loading

maxsmythe commented Oct 23, 2024

maxsmythe commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydipGabani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 3, 2024 •

edited

Loading

JaydipGabani left a comment •

edited

Loading

abhipatnala commented Oct 23, 2024 •

edited

Loading

ritazh Oct 30, 2024 •

edited

Loading