chore: check for CT generateVap intent before generating vapbinding #3479

JaydipGabani · 2024-08-08T01:58:11Z

What this PR does / why we need it:
While relying on labels for generation, we were inheriting generation behavior of VAPB from template by looking at generation-label on template. By that measure, we should check if generateVAP: true is set on template to inherit VAPB generation behavior.

Based on this comment #3478 (comment).

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 #

Special notes for your reviewer:

… binding Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

pkg/controller/constraint/constraint_controller.go

pkg/controller/constraint/constraint_controller_test.go

pkg/controller/constrainttemplate/constrainttemplate_controller_test.go

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

codecov-commenter · 2024-08-08T19:16:13Z

Codecov Report

Attention: Patch coverage is 24.00000% with 38 lines in your changes missing coverage. Please review.

Project coverage is 48.03%. Comparing base (3350319) to head (24dd83d).
Report is 119 commits behind head on master.

Files	Patch %	Lines
pkg/controller/constraint/constraint_controller.go	18.42%	30 Missing and 1 partial ⚠️
...onstrainttemplate/constrainttemplate_controller.go	41.66%	5 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (3350319) and HEAD (24dd83d). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (3350319) HEAD (24dd83d)

unittests 2 1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3479      +/-   ##
==========================================
- Coverage   54.49%   48.03%   -6.47%     
==========================================
  Files         134      219      +85     
  Lines       12329    15167    +2838     
==========================================
+ Hits         6719     7285     +566     
- Misses       5116     7066    +1950     
- Partials      494      816     +322

Flag	Coverage Δ
unittests	`48.03% <24.00%> (-6.47%)`	⬇️

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.

maxsmythe · 2024-08-08T20:36:07Z

Can we add a description on the PR as to why this behavior change is being considered? What problem it is solving?

maxsmythe · 2024-08-08T20:44:41Z

Is the bug that previously we were merely checking for the presence of CEL code, as opposed to whether that CEL code intended generation of VAP definitions (where the intent is signified by the generateVAP field value)?

JaydipGabani · 2024-08-08T21:11:51Z

Is the bug that previously we were merely checking for the presence of CEL code, as opposed to whether that CEL code intended generation of VAP definitions (where the intent is signified by the generateVAP field value)?

@maxsmythe The PR is making sure that we follow similar pattern as before. Earlier generation behavior for VAPB was inherited from looking at generation label on template. Similarly, we can check generateVAP: true on template for consistency.

PTAL at @ritazh's comment here - #3478 (comment).

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

maxsmythe

LGTM

ritazh · 2024-08-08T23:13:22Z

Is the bug that previously we were merely checking for the presence of CEL code, as opposed to whether that CEL code intended generation of VAP definitions (where the intent is signified by the generateVAP field value)?

For completeness, this PR checks for the CT generateVap intent before generating vapbinding. Updated the PR title to reflect what is does :)

ritazh · 2024-08-08T23:25:35Z

pkg/controller/constrainttemplate/constrainttemplate_controller_test.go

-	t.Run("VapBinding should be created with v1beta1", func(t *testing.T) {
-		suffix := "VapBindingShouldBeCreatedV1Beta1"
-		logger.Info("Running test: VapBinding should be created with v1beta1")
+	t.Run("VapBinding should not be created without VAP", func(t *testing.T) {


Please change all occurances where it says "without VAP" instead it should be "without generateVap intent in CT"

pkg/controller/constraint/constraint_controller.go

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

ritazh · 2024-08-08T23:34:56Z

pkg/controller/constraint/constraint_controller.go

+						generateVAPB = false
+					}
+					if !hasVAP {
+						r.log.V(1).Info("Warning: ConstraintTemplate will not create ValidatingAdmissionPolicy, cannot create VAPBinding")


Suggested change

r.log.V(1).Info("Warning: ConstraintTemplate will not create ValidatingAdmissionPolicy, cannot create VAPBinding")

r.log.V(1).Info("Warning: Conditions are not satisfied to generate ValidatingAdmissionPolicyBinding")

Should this be part of status error? and is this also reported as part of constraint template status?

This is not specifically an error right? reporting it as error might be confusing.

IIRC We discussed following up with designing a shcema for VAP/VAPB related errors and warnings on constraints and constrainttemplates.

ok lets leave it as a log for now for both constraint and constraint template. can you add this to the next meeting agenda so we can discuss?

Added to the agenda for the next meeting.

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

ritazh · 2024-08-09T04:19:59Z

pkg/controller/constrainttemplate/constrainttemplate_controller.go

+		createErr := &v1beta1.CreateCRDError{Code: ErrCreateCode, Message: "ValidatingAdmissionPolicy API is not enabled, ValidatingAdmissionPolicy resource cannot be generated for ConstraintTemplate"}
+		status.Status.Errors = append(status.Status.Errors, createErr)
+		err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Warning: ValidatingAdmissionPolicy resource cannot be generated for ConstraintTemplate", status, errors.New("ValidatingAdmissionPolicy API is not enabled"))
+		return reconcile.Result{}, err


we should be consistent everywhere in terms of logs and status reporting for these. if api is not enabled and other conditions for generateVap are not met, should they be considered errors or warnings? constraint and CT status reporting currently only has errors. @maxsmythe @sozercan thoughts? depending on this decision, please make all logs and status reporting consistent

I think it's an error -- the user has signaled intent to trigger the generation pipeline but the pipeline is not generating.

If users don't care about the generation pipeline, they can interpret the severity of the error however they'd like.

If users would like to remove the error, they can change the expressed intent.

sgtm. then lets log error and report error as part of CT and Constraint status.

sgtm, I am updating the PR.

…ternt cannot be satisfied Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

maxsmythe

LGTM

ritazh · 2024-08-09T18:58:41Z

pkg/controller/constrainttemplate/constrainttemplate_controller.go

 		currentVap, err := vapForVersion(groupVersion)
 		if err != nil {
 			logger.Error(err, "error getting vap object with respective groupVersion")
-			createErr := &v1beta1.CreateCRDError{Code: ErrCreateCode, Message: err.Error()}
-			status.Status.Errors = append(status.Status.Errors, createErr)


when you omit this, you are overriding existing status errors. please restore.

we are not preserving statues errors here anyways - https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/constrainttemplate/constrainttemplate_controller.go#L404, this code I am removing isn't doing anything.

Here is the code that we have wihtout appending to Status.Errors - https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/constrainttemplate/constrainttemplate_controller.go#L471 - before calling reportErrorOnCTStatus.

I see. ok then this has no impact on existing behavior.

ritazh

LGTM

checking for possibility of VAP existing in cluster before generating…

adef05d

… binding Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

JaydipGabani requested a review from a team as a code owner August 8, 2024 01:58

JaydipGabani mentioned this pull request Aug 8, 2024

chore: turning default-create-vap-binding-for-constraints to true #3478

Closed

fixing unit tests

62ac785

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

ritazh reviewed Aug 8, 2024

View reviewed changes

pkg/controller/constraint/constraint_controller.go Outdated Show resolved Hide resolved

ritazh reviewed Aug 8, 2024

View reviewed changes

pkg/controller/constraint/constraint_controller.go Outdated Show resolved Hide resolved

ritazh reviewed Aug 8, 2024

View reviewed changes

pkg/controller/constraint/constraint_controller.go Outdated Show resolved Hide resolved

ritazh reviewed Aug 8, 2024

View reviewed changes

pkg/controller/constraint/constraint_controller_test.go Show resolved Hide resolved

ritazh reviewed Aug 8, 2024

View reviewed changes

pkg/controller/constrainttemplate/constrainttemplate_controller_test.go Show resolved Hide resolved

ritazh reviewed Aug 8, 2024

View reviewed changes

pkg/controller/constrainttemplate/constrainttemplate_controller_test.go Outdated Show resolved Hide resolved

ritazh reviewed Aug 8, 2024

View reviewed changes

pkg/controller/constrainttemplate/constrainttemplate_controller_test.go Show resolved Hide resolved

JaydipGabani added 4 commits August 8, 2024 18:06

fixing tests

d4148f5

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

adding rego in test

6868702

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

adding rego engine in tests

b0f26cb

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

adding test for rego only engine

70478ee

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

ritazh changed the title ~~chore: checking for possibility of VAP existing in cluster before generating binding~~ chore: check for CT generateVap intent before generating vapbinding Aug 8, 2024

updating warning when template will not generate VAP

af0988f

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

maxsmythe approved these changes Aug 8, 2024

View reviewed changes

ritazh reviewed Aug 8, 2024

View reviewed changes

pkg/controller/constraint/constraint_controller.go Outdated Show resolved Hide resolved

updating logs and test names

34eb883

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

ritazh reviewed Aug 8, 2024

View reviewed changes

JaydipGabani added 3 commits August 8, 2024 23:37

updating logs and test names

ebb02b5

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

fixing test name

2327de8

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

fixing test name

589312a

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

JaydipGabani added 2 commits August 9, 2024 01:00

adding warning in CT when VAP API is not available

59cebbf

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

adding warning for CT when VAP API is not enabled and generation is on

a1ab00b

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

JaydipGabani force-pushed the generatevap-check branch from cc13d9a to a1ab00b Compare August 9, 2024 01:25

JaydipGabani requested a review from ritazh August 9, 2024 01:27

ritazh reviewed Aug 9, 2024

View reviewed changes

logging errors and reporting the same on CT and C, when generation in…

24dd83d

…ternt cannot be satisfied Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

JaydipGabani requested a review from ritazh August 9, 2024 18:44

maxsmythe approved these changes Aug 9, 2024

View reviewed changes

ritazh reviewed Aug 9, 2024

View reviewed changes

ritazh approved these changes Aug 9, 2024

View reviewed changes

sozercan approved these changes Aug 9, 2024

View reviewed changes

Merge branch 'master' into generatevap-check

d064c85

sozercan merged commit ed48135 into open-policy-agent:master Aug 9, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: check for CT generateVap intent before generating vapbinding #3479

chore: check for CT generateVap intent before generating vapbinding #3479

JaydipGabani commented Aug 8, 2024 •

edited

Loading

codecov-commenter commented Aug 8, 2024 •

edited

Loading

maxsmythe commented Aug 8, 2024

maxsmythe commented Aug 8, 2024

JaydipGabani commented Aug 8, 2024 •

edited

Loading

maxsmythe left a comment

ritazh commented Aug 8, 2024

ritazh Aug 8, 2024 •

edited

Loading

ritazh Aug 8, 2024 •

edited

Loading

ritazh Aug 8, 2024

JaydipGabani Aug 8, 2024 •

edited

Loading

ritazh Aug 9, 2024

JaydipGabani Aug 9, 2024

ritazh Aug 9, 2024 •

edited

Loading

maxsmythe Aug 9, 2024

ritazh Aug 9, 2024

JaydipGabani Aug 9, 2024

maxsmythe left a comment

ritazh Aug 9, 2024

JaydipGabani Aug 9, 2024

JaydipGabani Aug 9, 2024

ritazh Aug 9, 2024

ritazh left a comment

	r.log.V(1).Info("Warning: ConstraintTemplate will not create ValidatingAdmissionPolicy, cannot create VAPBinding")
	r.log.V(1).Info("Warning: Conditions are not satisfied to generate ValidatingAdmissionPolicyBinding")

chore: check for CT generateVap intent before generating vapbinding #3479

chore: check for CT generateVap intent before generating vapbinding #3479

Conversation

JaydipGabani commented Aug 8, 2024 • edited Loading

codecov-commenter commented Aug 8, 2024 • edited Loading

Codecov Report

maxsmythe commented Aug 8, 2024

maxsmythe commented Aug 8, 2024

JaydipGabani commented Aug 8, 2024 • edited Loading

maxsmythe left a comment

Choose a reason for hiding this comment

ritazh commented Aug 8, 2024

ritazh Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

ritazh Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydipGabani Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh Aug 9, 2024 • edited Loading

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

maxsmythe 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 left a comment

Choose a reason for hiding this comment

JaydipGabani commented Aug 8, 2024 •

edited

Loading

codecov-commenter commented Aug 8, 2024 •

edited

Loading

JaydipGabani commented Aug 8, 2024 •

edited

Loading

ritazh Aug 8, 2024 •

edited

Loading

ritazh Aug 8, 2024 •

edited

Loading

JaydipGabani Aug 8, 2024 •

edited

Loading

ritazh Aug 9, 2024 •

edited

Loading