feat: adding scopedenforcementactions #3321

JaydipGabani · 2024-03-18T22:32:32Z

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 #2266 #2945

Special notes for your reviewer:
Depends on #frameworks/403

codecov-commenter · 2024-03-21T16:38:39Z

Codecov Report

Attention: Patch coverage is 52.83843% with 108 lines in your changes missing coverage. Please review.

Project coverage is 48.25%. Comparing base (3350319) to head (ee74b6c).
Report is 112 commits behind head on master.

Files	Patch %	Lines
pkg/webhook/policy.go	26.31%	42 Missing ⚠️
pkg/audit/manager.go	3.12%	31 Missing ⚠️
pkg/controller/constraint/constraint_controller.go	30.30%	23 Missing ⚠️
pkg/util/enforcement_action.go	87.87%	4 Missing and 4 partials ⚠️
cmd/gator/test/test.go	0.00%	3 Missing ⚠️
pkg/gator/test/test.go	66.66%	1 Missing ⚠️

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

HEAD has 1 upload less than BASE

Flag BASE (3350319) HEAD (ee74b6c)

unittests 2 1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3321      +/-   ##
==========================================
- Coverage   54.49%   48.25%   -6.25%     
==========================================
  Files         134      219      +85     
  Lines       12329    14989    +2660     
==========================================
+ Hits         6719     7233     +514     
- Misses       5116     6946    +1830     
- Partials      494      810     +316

Flag	Coverage Δ
unittests	`48.25% <52.83%> (-6.25%)`	⬇️

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.

pkg/audit/manager_test.go

go.mod

sozercan · 2024-03-27T21:54:50Z

pkg/audit/manager_test.go

+			wantViolation:   true,
+		},
+		{
+			name:            "audit excluded from constraint",


can you clarify a little more here wdym? @sozercan

This is a test for constraint without audit enforcementPoint.

stale · 2024-06-18T03:48:53Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

maxsmythe · 2024-07-18T05:13:10Z

main.go

@@ -117,7 +117,7 @@ var (
 	disabledBuiltins                     = util.NewFlagSet()
 	enableK8sCel                         = flag.Bool("experimental-enable-k8s-native-validation", false, "Alpha: enable the validating admission policy driver")
 	externaldataProviderResponseCacheTTL = flag.Duration("external-data-provider-response-cache-ttl", 3*time.Minute, "TTL for the external data provider response cache. Specify the duration in 'h', 'm', or 's' for hours, minutes, or seconds respectively. Defaults to 3 minutes if unspecified. Setting the TTL to 0 disables the cache.")
-	deferAdmissionToVAP                           = flag.Bool("defer-admission-to-vap", false, "When set to false, Gatekeeper webhook can act as a fallback in case K8s' Validating Admission Policy fails.  When set to true, Gatekeeper validating webhook will not evaluate a policy for an admission request it expects vap to enforce. May improve resource usage at the cost of race conditions detecting whether VAP enforcement is in effect. This does not impact audit results. Defaults to false.")
+	deferAdmissionToVAP                  = flag.Bool("defer-admission-to-vap", false, "When set to false, Gatekeeper webhook can act as a fallback in case K8s' Validating Admission Policy fails.  When set to true, Gatekeeper validating webhook will not evaluate a policy for an admission request it expects vap to enforce. May improve resource usage at the cost of race conditions detecting whether VAP enforcement is in effect. This does not impact audit results. Defaults to false.")


Much of this logic goes away with #3398

maxsmythe · 2024-07-18T05:17:54Z

pkg/audit/manager.go

+	Name              string   `json:"name"`
+	Namespace         string   `json:"namespace,omitempty"`
+	Message           string   `json:"message"`
+	EnforcementAction []string `json:"enforcementAction"`


I don't think we can change the schema here without breaking backwards compatibility.

maxsmythe · 2024-07-18T05:19:00Z

pkg/audit/manager.go

@@ -1169,7 +1169,7 @@ func violationMsg(constraint *unstructured.Unstructured, enforcementAction util.
 		Kind:                  constraint.GetKind(),
 		Name:                  constraint.GetName(),
 		Namespace:             constraint.GetNamespace(),
-		EnforcementAction:     string(enforcementAction),
+		EnforcementAction:     enforcementAction,


Backwards compatibility concern?

To ensure backwards compatibility, we could add a new EnforcementActions instead and not touch the existing field. wdyt? same would apply for logs and events.

maxsmythe · 2024-07-18T05:20:00Z

pkg/audit/manager.go

@@ -1182,7 +1182,7 @@ func violationMsg(constraint *unstructured.Unstructured, enforcementAction util.

 func logViolation(l logr.Logger,
 	constraint *unstructured.Unstructured,
-	enforcementAction util.EnforcementAction, resourceGroupVersionKind schema.GroupVersionKind, rnamespace, rname, message string, details interface{}, rlabels map[string]string,
+	enforcementAction []string, resourceGroupVersionKind schema.GroupVersionKind, rnamespace, rname, message string, details interface{}, rlabels map[string]string,


This is a backwards compatibility concern because it affects semantic logging. We could just log one message per enforcement action, though. Or add a new field (though what value would enforcementAction have?)

We can probably serialize the array. Something like deny/warn in-case if enforcemet point is duplicated in scopedEA config.

to ensure backwards compat, the existing enforcementAction should remain untouched. lets create a new field instead.

ritazh · 2024-07-24T01:00:10Z

pkg/controller/constraint/constraint_controller.go

+			if err != nil {
+				status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: err.Error()})
+				if err2 := r.writer.Update(ctx, status); err2 != nil {
+					log.Error(err2, "could not get enforcement actions for VAP")


This error is when updating constraint status failed right? please update the log message.

pkg/controller/constraint/constraint_controller.go

pkg/audit/manager.go

maxsmythe · 2024-07-24T20:37:56Z

pkg/controller/constraint/constraint_controller.go

+				return reconcile.Result{}, err
+			}
+			isVAPBGenerationEnabled := generateVAPB && IsVapAPIEnabled() && HasVAPCel(ct)
+			if generateVAPB != isVAPBGenerationEnabled {


We should probably just calculate these errors directly, rather than packing multiple values into a bool and then unpacking them again.

maxsmythe · 2024-07-24T20:50:45Z

pkg/gator/test/types.go

@@ -66,8 +67,8 @@ func (r *GatorResponses) Results() []*GatorResult {
 	// responses to individual constraints. This is a stopgap to make tests easier
 	// to write until then.
 	sort.Slice(res, func(i, j int) bool {
-		if res[i].EnforcementAction != res[j].EnforcementAction {


second instance of a concatenated string (backwards compatibility concern)

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

JaydipGabani · 2024-07-30T02:28:16Z

pkg/util/enforcement_action.go

+			return ErrInvalidSpecEnforcementAction
+		}
+		for _, enforcementPoint := range scopedEnforcementAction.EnforcementPoints {
+			switch strings.ToLower(enforcementPoint.Name) {


Will remove this if needed after we reach to a conclusion on - open-policy-agent/frameworks#403 (comment)

JaydipGabani · 2024-07-30T02:29:09Z

pkg/util/enforcement_action.go

+
+func enforcementPointEnabled(scopedEnforcementAction apiconstraints.ScopedEnforcementAction, enforcementPoint string) bool {
+	for _, ep := range scopedEnforcementAction.EnforcementPoints {
+		if strings.EqualFold(ep.Name, enforcementPoint) || ep.Name == AllEnforcementPoints {


remove this if need be after we reach conclusion on - open-policy-agent/frameworks#403 (comment)

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

pkg/webhook/policy.go

ritazh · 2024-07-31T04:34:58Z

pkg/controller/constraint/constraint_controller_test.go

+								Action: "warn",
+								EnforcementPoints: []apiconstraints.EnforcementPoint{
+									{
+										Name: util.WebhookEnforcementPoint,


in this test, when webhookep has both action warn and deny, which one wins?

if both the actions are there ultimatly the request is denied but output containes warning and deny and looks like this -

Warning: [cm-must-have-gk-scoped] you must provide labels: {"gatekeeper"} Error from server (Forbidden): error when creating "/mount/d/go/src/github.com/open-policy-agent/gatekeeper/test/bats/tests/bad/bad_cm_scoped.yaml": admission webhook "validation.gatekeeper.sh" denied the request: [cm-must-have-gk-scoped] you must provide labels: {"gatekeeper"}

ritazh · 2024-07-31T04:45:32Z

pkg/webhook/policy.go

+		switch r.EnforcementAction {
+		case string(util.Scoped):
+			for _, action := range r.ScopedEnforcementActions {
+				if err := util.ValidateEnforcementAction(util.EnforcementAction(action), r.Constraint.Object); err != nil {


Logging error

ritazh · 2024-07-31T04:45:41Z

pkg/webhook/policy.go

+				continue
+			}
+		default:
+			if err := util.ValidateEnforcementAction(util.EnforcementAction(r.EnforcementAction), r.Constraint.Object); err != nil {


ritazh · 2024-07-31T04:46:54Z

pkg/controller/constraint/constraint_controller.go

@@ -303,8 +300,35 @@ func (r *ReconcileConstraint) Reconcile(ctx context.Context, request reconcile.R
 		status.Status.Errors = nil

 		if c, err := r.cfClient.GetConstraint(instance); err != nil || !reflect.DeepEqual(instance, c) {
+			err := util.ValidateEnforcementAction(enforcementAction, instance.Object)
+			if err != nil {
+				status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: err.Error()})


is there a test that validates this error is written to the constraint's status errors?

Added the test constrainttemplate_controller_test.go#L811

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

ritazh

LGTM

ritazh · 2024-07-31T19:45:06Z

website/docs/enforcement-points.md

+
+### How to use different enforcement points in constraint
+
+By default, a constraint will be enforced at all enforcement points with common enforcement action defined in `spec.enforcementAction`. However, you can chose to enforce a constraint at specific enforcement points different actions using `spec.scopedEnforcementActions`. Below are the different examples and use cases that utilizes different enforcement actions for different enforcement points.


Suggested change

By default, a constraint will be enforced at all enforcement points with common enforcement action defined in `spec.enforcementAction`. However, you can chose to enforce a constraint at specific enforcement points different actions using `spec.scopedEnforcementActions`. Below are the different examples and use cases that utilizes different enforcement actions for different enforcement points.

By default, a constraint will be enforced at all enforcement points with common enforcement action defined in `spec.enforcementAction`. However, you can choose to enforce a constraint at specific enforcement points with different actions using `enforcementAction: scoped` and `spec.scopedEnforcementActions`. Below are examples and use cases that utilize different enforcement actions for different enforcement points.

ritazh · 2024-07-31T19:46:49Z

website/docs/enforcement-points.md

+By default, a constraint will be enforced at all enforcement points with common enforcement action defined in `spec.enforcementAction`. However, you can chose to enforce a constraint at specific enforcement points different actions using `spec.scopedEnforcementActions`. Below are the different examples and use cases that utilizes different enforcement actions for different enforcement points.
+
+:::note
+`spec.enforcementAction: scoped` is needed to customize specific enforcement point/enforcement action behavior. If `spec.enforcementAction: scoped` is not provided, `spec.scopedEnforcementActions` is ignored and defined `enforcementAction` will be enforced at all enforcement points.


Suggested change

`spec.enforcementAction: scoped` is needed to customize specific enforcement point/enforcement action behavior. If `spec.enforcementAction: scoped` is not provided, `spec.scopedEnforcementActions` is ignored and defined `enforcementAction` will be enforced at all enforcement points.

`spec.enforcementAction: scoped` is needed to customize specific enforcement point/enforcement action behavior. If `spec.enforcementAction: scoped` is not provided, `spec.scopedEnforcementActions` is ignored and the provided `enforcementAction` will be applied across all enforcement points.

ritazh · 2024-07-31T19:49:13Z

website/docs/enforcement-points.md

+
+###### Deny in shift-left and warn at admission
+
+You are trying out a new constraint template, and you want deny violating resources in shift-left testing, but do not want to block any resources when admitted to cluster to avoid faulty rejects. You may want to use `deny` action for `gator.gatekeeper.sh` enforcement point and `warn` for `validation.gatekeepet.sh`. The below constraint satisfies this use case.


Suggested change

You are trying out a new constraint template, and you want deny violating resources in shift-left testing, but do not want to block any resources when admitted to cluster to avoid faulty rejects. You may want to use `deny` action for `gator.gatekeeper.sh` enforcement point and `warn` for `validation.gatekeepet.sh`. The below constraint satisfies this use case.

You are trying out a new constraint template, and you want to deny violating resources in shift-left testing, but do not want to block any resources admitted to clusters to reduce impact for faulty rejections. You may want to use `deny` action for the `gator.gatekeeper.sh` shift-left enforcement point and `warn` for `the validation.gatekeepet.sh` admission webhook enforcement point. The below constraint satisfies this use case.

Add a note for:

The audit enforcement point is not included unless explicitly added to scopedEnforcementActions.enforcementPoints or if scopedEnforcementActions.enforcementPoints is set to "*"

ritazh · 2024-07-31T19:54:16Z

website/docs/validating-admission-policy.md


-To summary, these are potential options when running Gatekeeper:
+To summarize, these are potential options when running Gatekeeper:


Suggested change

To summarize, these are potential options when running Gatekeeper:

In summary, these are potential options when running Gatekeeper:

ritazh · 2024-07-31T20:01:17Z

website/docs/validating-admission-policy.md

@@ -134,7 +136,46 @@ spec:
            ...
 ```

-Constraints will follow the behavior defined by `--default-create-vap-binding-for-constraints` flag to generate K8s Validating Admission Policy Binding. By default, `--default-create-vap-binding-for-constraints` is set to `false`.
+Constraints will follow the behavior defined in `spec.scopedEnforcementActions`. When `spec.scopedEnforcementAction` is not defined, constraints will follow behavior defined by the flag `--default-create-vap-binding-for-constraints`. By default, `--default-create-vap-binding-for-constraints` is set to `false`.


Suggested change

Constraints will follow the behavior defined in `spec.scopedEnforcementActions`. When `spec.scopedEnforcementAction` is not defined, constraints will follow behavior defined by the flag `--default-create-vap-binding-for-constraints`. By default, `--default-create-vap-binding-for-constraints` is set to `false`.

Gatekeeper determines the intended enforcement actions for a given enforcement point by evaluating what is provided in `spec.scopedEnforcementActions` and `spec.enforcementAction: scoped` in the constraint. If these values are not provided in the constraint, then Gatekeeper will follow behavior defined by the flag `--default-create-vap-binding-for-constraints`. By default, `--default-create-vap-binding-for-constraints` is set to `false`.

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

maxsmythe

LGTM

sozercan

Great job! LGTM

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

JaydipGabani force-pushed the multi-ea branch 2 times, most recently from 87f4584 to 2f9f4bb Compare March 21, 2024 15:55

JaydipGabani marked this pull request as ready for review March 21, 2024 19:47

JaydipGabani requested a review from a team as a code owner March 21, 2024 19:47

JaydipGabani mentioned this pull request Mar 21, 2024

feat: adding scopedenforcementactions open-policy-agent/frameworks#403

Merged

sozercan reviewed Mar 27, 2024

View reviewed changes

pkg/audit/manager_test.go Outdated Show resolved Hide resolved

sozercan reviewed Mar 27, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

sozercan reviewed Mar 27, 2024

View reviewed changes

JaydipGabani requested review from sozercan, ritazh and maxsmythe March 28, 2024 12:12

stale bot added the stale label Jun 18, 2024

maxsmythe removed the stale label Jun 19, 2024

JaydipGabani force-pushed the multi-ea branch from 34a7519 to ad9bba6 Compare June 25, 2024 20:00

ritazh added this to the v3.17.0 milestone Jul 3, 2024

maxsmythe reviewed Jul 18, 2024

View reviewed changes

JaydipGabani force-pushed the multi-ea branch from 40598b7 to 11c8ea0 Compare July 24, 2024 00:07

ritazh reviewed Jul 24, 2024

View reviewed changes

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

maxsmythe reviewed Jul 24, 2024

View reviewed changes

JaydipGabani force-pushed the multi-ea branch 2 times, most recently from 329b7e1 to c81bc90 Compare July 25, 2024 16:22

adding scoped enforcement actions

239801c

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

JaydipGabani force-pushed the multi-ea branch from c81bc90 to 239801c Compare July 25, 2024 16:36

JaydipGabani added 3 commits July 25, 2024 16:51

fixing tests

45b3a0b

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

fixing bats

92426e6

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

fixing bats test

641315b

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

JaydipGabani requested a review from ritazh July 25, 2024 17:50

JaydipGabani commented Jul 30, 2024

View reviewed changes

JaydipGabani requested a review from ritazh July 30, 2024 02:30

JaydipGabani added 4 commits July 31, 2024 01:08

removing lowercase conversion for enforcement point

90717cb

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

Merge branch 'master' into multi-ea

4d5c965

updating with latest frameworks changes

5172a87

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

refactoring getValidationMessage for logDenies and emitAdmissionEvents

9837f83

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

ritazh reviewed Jul 31, 2024

View reviewed changes

pkg/webhook/policy.go Show resolved Hide resolved

ritazh reviewed Jul 31, 2024

View reviewed changes

adding test for unrecognized enforcement point

e341164

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

JaydipGabani requested a review from ritazh July 31, 2024 06:53

JaydipGabani added 2 commits July 31, 2024 17:48

bringing frameworks changes

a920e36

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

Merge branch 'master' into multi-ea

3be726d

JaydipGabani mentioned this pull request Jul 31, 2024

chore: bumping opa 0.66.0 to 0.67.0 open-policy-agent/frameworks#459

Merged

ritazh approved these changes Jul 31, 2024

View reviewed changes

ritazh reviewed Jul 31, 2024

View reviewed changes

updating docs

afe7999

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

maxsmythe approved these changes Jul 31, 2024

View reviewed changes

Merge branch 'master' into multi-ea

ee74b6c

sozercan approved these changes Jul 31, 2024

View reviewed changes

sozercan merged commit 23fa58c into open-policy-agent:master Jul 31, 2024
21 checks passed

JaydipGabani deleted the multi-ea branch July 31, 2024 23:59

Ankurk99 pushed a commit to Ankurk99/gatekeeper that referenced this pull request Aug 1, 2024

feat: adding scopedenforcementactions (open-policy-agent#3321)

0533ff1

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding scopedenforcementactions #3321

feat: adding scopedenforcementactions #3321

JaydipGabani commented Mar 18, 2024 •

edited

Loading

codecov-commenter commented Mar 21, 2024 •

edited

Loading

sozercan Mar 27, 2024

JaydipGabani Mar 28, 2024

stale bot commented Jun 18, 2024

maxsmythe Jul 18, 2024

JaydipGabani Jul 18, 2024

maxsmythe Jul 18, 2024

maxsmythe Jul 18, 2024

ritazh Jul 24, 2024 •

edited

Loading

maxsmythe Jul 18, 2024

JaydipGabani Jul 23, 2024

ritazh Jul 24, 2024

ritazh Jul 24, 2024

maxsmythe Jul 24, 2024

maxsmythe Jul 24, 2024

JaydipGabani Jul 30, 2024

JaydipGabani Jul 30, 2024

ritazh Jul 31, 2024

JaydipGabani Jul 31, 2024

ritazh Jul 31, 2024

JaydipGabani Jul 31, 2024

ritazh Jul 31, 2024

ritazh Jul 31, 2024

JaydipGabani Jul 31, 2024

ritazh left a comment

ritazh Jul 31, 2024

ritazh Jul 31, 2024

ritazh Jul 31, 2024

ritazh Jul 31, 2024

ritazh Jul 31, 2024

ritazh Jul 31, 2024

maxsmythe left a comment

sozercan left a comment


		### How to use different enforcement points in constraint

		By default, a constraint will be enforced at all enforcement points with common enforcement action defined in `spec.enforcementAction`. However, you can chose to enforce a constraint at specific enforcement points different actions using `spec.scopedEnforcementActions`. Below are the different examples and use cases that utilizes different enforcement actions for different enforcement points.

	`spec.enforcementAction: scoped` is needed to customize specific enforcement point/enforcement action behavior. If `spec.enforcementAction: scoped` is not provided, `spec.scopedEnforcementActions` is ignored and defined `enforcementAction` will be enforced at all enforcement points.
	`spec.enforcementAction: scoped` is needed to customize specific enforcement point/enforcement action behavior. If `spec.enforcementAction: scoped` is not provided, `spec.scopedEnforcementActions` is ignored and the provided `enforcementAction` will be applied across all enforcement points.


		###### Deny in shift-left and warn at admission

		You are trying out a new constraint template, and you want deny violating resources in shift-left testing, but do not want to block any resources when admitted to cluster to avoid faulty rejects. You may want to use `deny` action for `gator.gatekeeper.sh` enforcement point and `warn` for `validation.gatekeepet.sh`. The below constraint satisfies this use case.


		To summary, these are potential options when running Gatekeeper:
		To summarize, these are potential options when running Gatekeeper:

	To summarize, these are potential options when running Gatekeeper:
	In summary, these are potential options when running Gatekeeper:

feat: adding scopedenforcementactions #3321

feat: adding scopedenforcementactions #3321

Conversation

JaydipGabani commented Mar 18, 2024 • edited Loading

codecov-commenter commented Mar 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 18, 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

ritazh Jul 24, 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

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

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

ritazh 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

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

sozercan left a comment

Choose a reason for hiding this comment

JaydipGabani commented Mar 18, 2024 •

edited

Loading

codecov-commenter commented Mar 21, 2024 •

edited

Loading

ritazh Jul 24, 2024 •

edited

Loading