chore: adding enforcement point status, vapgeneratestatus by JaydipGabani · Pull Request #3686 · open-policy-agent/gatekeeper

JaydipGabani · 2024-11-08T22:02:47Z

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 #3682

Special notes for your reviewer:

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

JaydipGabani · 2024-11-08T22:05:18Z

+	Operations          []string                           `json:"operations,omitempty"`
+	ObservedGeneration  int64                              `json:"observedGeneration,omitempty"`
+	Errors              []*templatesv1beta1.CreateCRDError `json:"errors,omitempty"`
+	VAPGenerationStatus VAPGenerationStatus                `json:"vapGenerationStatus,omitempty"`


I can remove this if we do not want status on CT for generation updates. Lmk your thoughts.

I think it makes sense to report status for CT objects... they do map directly to a VAP object.

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

codecov-commenter · 2024-11-11T21:22:45Z

Codecov Report

❌ Patch coverage is 1.47059% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.56%. Comparing base (3350319) to head (45a53f7).
⚠️ Report is 655 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/controller/constraint/constraint_controller.go	0.00%	35 Missing ⚠️
apis/status/v1beta1/zz_generated.deepcopy.go	0.00%	28 Missing ⚠️
...onstrainttemplate/constrainttemplate_controller.go	25.00%	2 Missing and 1 partial ⚠️
pkg/drivers/k8scel/schema/schema.go	0.00%	1 Missing ⚠️

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

HEAD has 1 upload less than BASE

Flag BASE (3350319) HEAD (45a53f7)

unittests 2 1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3686      +/-   ##
==========================================
- Coverage   54.49%   47.56%   -6.94%     
==========================================
  Files         134      236     +102     
  Lines       12329    19896    +7567     
==========================================
+ Hits         6719     9464    +2745     
- Misses       5116     9539    +4423     
- Partials      494      893     +399

Flag	Coverage Δ
unittests	`47.56% <1.47%> (-6.94%)`	⬇️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sozercan

LGTM

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

maxsmythe · 2024-11-14T01:30:43Z

+// EnforcementPointStatus represents the status of a single enforcement point.
+type EnforcementPointStatus struct {
+	EnforcementPoint string `json:"enforcementPoint"`
+	Enforced         bool   `json:"enforced"`


I think it was a mistake to have an enforced boolean state with constraints. And with VAP, we cannot make guarantees as to whether they are enforced b/c there is no status reporting, so we cannot know if the VAP object has been ingested or whether there are any errors. Maybe we want something like a string field called Status?

There we can put a string like "GENERATED", "ERROR", etc.

Of course, if we call it "status", we should rename the parent field to avoid stutter.

how about code - generated/error/...?

"code" seems a bit ambiguous given we are also dealing with "code" in the sense of CEL code.

"state"?

I like state, updated the name to state.

maxsmythe · 2024-11-14T01:32:33Z

+	Operations          []string                           `json:"operations,omitempty"`
+	ObservedGeneration  int64                              `json:"observedGeneration,omitempty"`
+	Errors              []*templatesv1beta1.CreateCRDError `json:"errors,omitempty"`
+	VAPGenerationStatus VAPGenerationStatus                `json:"vapGenerationStatus,omitempty"`


I think it makes sense to report status for CT objects... they do map directly to a VAP object.

maxsmythe · 2024-11-14T01:37:21Z

 			err := util.ValidateEnforcementAction(enforcementAction, instance.Object)
 			if err != nil {
-				return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, err, "could not validate enforcement actions")
+				return reconcile.Result{}, r.reportErrorOnConstraintStatus(ctx, status, constraintstatusv1beta1.EnforcementPointStatus{}, err, "could not validate enforcement actions")


Will this have the effect of wiping out previously reported status? Should we preserve the original VAP status unless there is a failure related to VAP generation?

Of course, if we do that, VAP status should probably have it's own observedGeneration field, since it's possible that VAP status corresponds to an older object than the rest of the status.

The other option is to keep the current behavior, which wipes status on every update.

Added obeservedGeneration for each enforcement point status.

maxsmythe · 2024-11-14T01:39:55Z

+
+// VAPGenerationStatus represents the status of VAP generation.
+type VAPGenerationStatus struct {
+	Generated bool   `json:"generated,omitempty"`


Same comment about bool field vs. string field.

Updated to string field.

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

maxsmythe · 2024-11-15T22:55:27Z

 	status.Status.TemplateUID = ct.GetUID()
 	status.Status.ObservedGeneration = ct.GetGeneration()
 	status.Status.Errors = nil
+	status.Status.VAPGenerationStatus = statusv1beta1.VAPGenerationStatus{}


This appears to have the same problem of wiping the generation status that constraints had.

Fixed it. PTAL @maxsmythe

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

maxsmythe · 2024-11-18T22:09:15Z

-	if err != nil && !errors.Is(err, celSchema.ErrCodeNotDefined) {
+	if err != nil {
 		logger.Error(err, "generateVap error")
+		if generateVap {


by Golang convention, generateVap would be false if ShouldGenerateVAP returns an error. We should probably report the error regardless.

This would mean all rego templates will have this status message regarldess of if there is intention of VAP generation or not. When should we report this error?

all the time, regardless of indication of VAP generation for CT?

Or only when VAP is supposed to be generated but is not being generated?

I agree that we will be reporting this error for all rego CT when we go to beta.

It sounds like ShouldGenerateVAP should be implemented differently then?

If we are struggling to implement the behaviors we want, that's a sign we should rethink primitives, not double-down on pre-existing shapes as if they were carved in stone, adding edge cases and complexity along the way.

It seems like ShouldGenerateVAP() should return (false, nil) if we get ErrCodeNotDefined:

gatekeeper/pkg/drivers/k8scel/schema/schema.go

Line 291 in 3555a65

return nil, ErrCodeNotDefined

That would handle templates where VAP generation is not expected, like for Rego.

Updated shouldGenerateVAP to return false. Still returning ErrCodeNotDefined as it is needed for constraints to report respective status.

@ritazh @sozercan PTAL.

maxsmythe · 2024-11-18T22:11:19Z

 				err := r.reportErrorOnCTStatus(ctx, ErrUpdateCode, "Could not delete VAP object", status, err)
 				return err
 			}
+			status.Status.VAPGenerationStatus.State = DeletedVAPState


We probably shouldn't have a "deleted" state, since that's more imperative than declarative. We could have a "NotGenerated" state, or similar. But if we do that, we should use it consistently (i.e. any time VAP objects are not generated)

Removed "delete" state

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

maxsmythe

LGTM once we fix the edge case of CTs without CEL code

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

maxsmythe

LGTM

ritazh · 2024-11-21T00:55:11Z

+	ErrGenerateVAPState = "error"
+	// GeneratedVAPState indicates a VAP was generated.
+	GeneratedVAPState = "generated"
+	// DeletedVAPState indicates a VAP was deleted.


do we still need this

Nope! Good catch, removed it.

ritazh · 2024-11-21T01:06:50Z

+	vapEnforcementPointStatus := constraintstatusv1beta1.EnforcementPointStatus{EnforcementPoint: enforcementPoint, State: state, ObservedGeneration: observedGeneration, Message: message}
+	for i, ep := range status.Status.EnforcementPointsStatus {
+		if ep.EnforcementPoint == enforcementPoint {
+			status.Status.EnforcementPointsStatus[i] = vapEnforcementPointStatus


enforcementPoint can be more than just vap enforcement point right? this var name vapEnforcementPointStatus makes it look like its just for the vap EP.

updated the variable to make it generic.

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

ritazh

LGTM

adding enforcement point status, vapgeneratestatus

a492dcd

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

JaydipGabani requested a review from a team as a code owner November 8, 2024 22:02

removing not needed file

c72db95

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

JaydipGabani commented Nov 8, 2024

View reviewed changes

fixing unit tests

8c27363

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

Merge branch 'master' into enforcementPointStatus

ceedd2d

JaydipGabani added this to the v3.18.0 milestone Nov 13, 2024

JaydipGabani added 2 commits November 13, 2024 12:20

Merge branch 'master' into enforcementPointStatus

d65d3c3

Merge branch 'master' into enforcementPointStatus

8183975

sozercan approved these changes Nov 13, 2024

View reviewed changes

fixing unit test

0e40ed2

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

maxsmythe reviewed Nov 14, 2024

View reviewed changes

adding observegeneration for enforcement point status

0238c73

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

sozercan approved these changes Nov 15, 2024

View reviewed changes

renaming code -> state field

a4f17a3

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

JaydipGabani requested a review from maxsmythe November 15, 2024 17:26

Merge branch 'master' into enforcementPointStatus

d67ef29

maxsmythe reviewed Nov 15, 2024

View reviewed changes

JaydipGabani added 3 commits November 16, 2024 00:15

adding observed generation and not wiping CT VAPgeneration status

ae48233

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

updating vap status on delete, fixing tests

8ef8278

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

fixing test

b25ca53

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

JaydipGabani requested a review from maxsmythe November 18, 2024 06:27

Merge branch 'master' into enforcementPointStatus

a935efd

maxsmythe reviewed Nov 18, 2024

View reviewed changes

JaydipGabani added 2 commits November 19, 2024 01:01

remobing delete status for vap EP

c33b601

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

removing unused constant

5dff00d

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

JaydipGabani requested a review from maxsmythe November 19, 2024 21:30

maxsmythe reviewed Nov 20, 2024

View reviewed changes

updating shouldgeneratevap

1afa5ac

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

Merge branch 'master' into enforcementPointStatus

9727998

JaydipGabani requested a review from maxsmythe November 20, 2024 19:29

maxsmythe approved these changes Nov 20, 2024

View reviewed changes

ritazh approved these changes Nov 21, 2024

View reviewed changes

ritazh reviewed Nov 21, 2024

View reviewed changes

removing deletedVAPState constraint

45a53f7

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

JaydipGabani requested a review from ritazh November 21, 2024 01:30

ritazh approved these changes Nov 21, 2024

View reviewed changes

JaydipGabani merged commit 26bfc0f into open-policy-agent:master Nov 21, 2024

JaydipGabani deleted the enforcementPointStatus branch November 21, 2024 01:59

BrewTestBot mentioned this pull request Dec 12, 2024

gator 3.18.0 Homebrew/homebrew-core#200877

Merged

Conversation

JaydipGabani commented Nov 8, 2024

Uh oh!

JaydipGabani Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sozercan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxsmythe left a comment

Choose a reason for hiding this comment

Uh oh!

maxsmythe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ritazh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JaydipGabani Nov 8, 2024 •

edited

Loading

codecov-commenter commented Nov 11, 2024 •

edited

Loading