Add new field gatekeeperEnforcementAction to override enforcementAction #167

yiraeChristineKim · 2024-07-09T15:49:29Z

Add a new field of gatekeeperEnforcementAction
Default value of the field is unset and when unset, don't touch the constraint (existing behavior)
If set, then directly set spec.enforcementAction in the Gatekeeper constraint
Should be settable at the policyDefaults, policy, and manifest levels (i.e. like the ConfigurationPolicyOptions struct).
Ref: https://issues.redhat.com/browse/ACM-11076
Signed-off-by: yiraeChristineKim [email protected]

mprahl · 2024-07-09T20:53:03Z

docs/policygenerator-reference.yaml

+  # Optional. gatekeeperEnforcementAction("deny" | "warn" | "dryrun") will override gatekeeper.constraint.spec.enforcementAction
+  # This only applies to gatekeeper constraint
+  # The default is "deny"


Suggested change

# Optional. gatekeeperEnforcementAction("deny" | "warn" | "dryrun") will override gatekeeper.constraint.spec.enforcementAction

# This only applies to gatekeeper constraint

# The default is "deny"

# Optional. Overrides the spec.enforcementAction field of a Gatekeeper constraint. This only applies to Gatekeeper constraints and is ignored by other manifests. If not set, the spec.enforcementAction field is not changed.

mprahl · 2024-07-09T20:56:26Z

internal/utils.go


 					policyTemplateUnstructured.SetAnnotations(annotations)
+
+					err := setGatekeeperEnforcementAction(policyTemplate,


It might be cleaner to directly work with the manifest rather than after it is a policy template. You could do this right at the start of the for _, manifest := range manifestGroup { loop.

mprahl · 2024-07-09T20:57:47Z

internal/plugin_test.go

+				Name: "policy-gatekeeper",
+				Manifests: []types.Manifest{
+					{
+						Path:              path.Join(tmpDir, "gatekeeper.yaml"),


Could you reuse gatekeeperPath here and also in other places?

mprahl · 2024-07-09T20:58:43Z

internal/plugin_test.go

+			policyDefaultEA: "",
+			expectedEA: `
+            spec:
+                enforcementAction: deny


It'd be cleaner if you just provided deny here and the test code can do the work of checking for spec and enforcementAction being in the generated string.

mprahl · 2024-07-10T15:27:23Z

internal/utils.go


+// setGatekeeperEnforcementAction function override gatekeeper.constraint.enforcementAction
+func setGatekeeperEnforcementAction(manifest map[string]interface{}, enforcementAction string) error {
+	apiVersion, found, err := unstructured.NestedString(manifest, "apiVersion")


Could you return right away if enforcementAction == ""?

mprahl · 2024-07-10T15:29:28Z

internal/utils.go

+	apiVersion, found, err := unstructured.NestedString(manifest, "apiVersion")
+	if err != nil {
+		return fmt.Errorf("getting apiVersion has an error %w", err)
+	}
+
+	if found && strings.HasPrefix(apiVersion, "constraints.gatekeeper.sh") && enforcementAction != "" {
+		err := unstructured.SetNestedField(manifest, enforcementAction, "spec", "enforcementAction")
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil


Suggested change

apiVersion, found, err := unstructured.NestedString(manifest, "apiVersion")

if err != nil {

return fmt.Errorf("getting apiVersion has an error %w", err)

}

if found && strings.HasPrefix(apiVersion, "constraints.gatekeeper.sh") && enforcementAction != "" {

err := unstructured.SetNestedField(manifest, enforcementAction, "spec", "enforcementAction")

if err != nil {

return err

}

}

return nil

if enforcementAction == "" {

return nil

}

apiVersion, _, _ := unstructured.NestedString(manifest, "apiVersion")

if strings.HasPrefix(apiVersion, "constraints.gatekeeper.sh") {

err := unstructured.SetNestedField(manifest, enforcementAction, "spec", "enforcementAction")

if err != nil {

return err

}

}

return nil

I think it would be more safe that we have error check here
apiVersion, _, err := unstructured.NestedString(manifest, "apiVersion") if err != nil { return err }

@yiraeChristineKim that's fine but I kind of wanted to avoid validating the actual manifest in the Policy Generator and let the policy controllers handle it.

mprahl · 2024-07-10T15:30:37Z

internal/utils.go

+			err := setGatekeeperEnforcementAction(manifest,
+				policyConf.Manifests[i].GatekeeperEnforcementAction)
+			if err != nil {
+				return nil, err


Could you provide more context in the return error such as the manifest index and that this error occurred due to setting spec.enforcementAction?

Add a new field of `gatekeeperEnforcementAction` Default value of the field is unset and when unset, don't touch the constraint (existing behavior) If set, then directly set `spec.enforcementAction` in the Gatekeeper constraint Should be settable at the policyDefaults, policy, and manifest levels (i.e. like the `ConfigurationPolicyOptions` struct). Ref: https://issues.redhat.com/browse/ACM-11076 Signed-off-by: yiraeChristineKim <[email protected]>

openshift-ci · 2024-07-10T16:12:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mprahl, yiraeChristineKim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mprahl,yiraeChristineKim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from JustinKuli and gparvin July 9, 2024 15:49

openshift-ci bot added dco-signoff: yes approved labels Jul 9, 2024

yiraeChristineKim force-pushed the ACM-11076 branch from 789719a to 2f82d58 Compare July 9, 2024 16:32

yiraeChristineKim marked this pull request as draft July 9, 2024 16:37

openshift-ci bot added the do-not-merge/work-in-progress label Jul 9, 2024

yiraeChristineKim marked this pull request as ready for review July 9, 2024 16:45

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 9, 2024

openshift-ci bot requested a review from mprahl July 9, 2024 16:45

yiraeChristineKim requested review from dhaiducek and mprahl and removed request for mprahl July 9, 2024 16:45

yiraeChristineKim force-pushed the ACM-11076 branch from 2f82d58 to 974f3ac Compare July 9, 2024 16:46

mprahl reviewed Jul 9, 2024

View reviewed changes

yiraeChristineKim force-pushed the ACM-11076 branch 3 times, most recently from 53d65bb to 1215ea5 Compare July 10, 2024 15:18

mprahl reviewed Jul 10, 2024

View reviewed changes

yiraeChristineKim force-pushed the ACM-11076 branch from 1215ea5 to d516264 Compare July 10, 2024 15:52

mprahl approved these changes Jul 10, 2024

View reviewed changes

openshift-ci bot assigned mprahl Jul 10, 2024

openshift-ci bot added the lgtm label Jul 10, 2024

openshift-merge-bot bot merged commit 418a5ce into open-cluster-management-io:main Jul 10, 2024

magic-mirror-bot bot mentioned this pull request Jul 10, 2024

🤖 Sync from open-cluster-management-io/policy-generator-plugin: #167 stolostron/policy-generator-plugin#90

Merged

dhaiducek mentioned this pull request Sep 27, 2024

Allow to set remediationAction for policies with Gatekeeper Constraints and templates #155

Closed


		policyTemplateUnstructured.SetAnnotations(annotations)

		err := setGatekeeperEnforcementAction(policyTemplate,

Add new field gatekeeperEnforcementAction to override enforcementAction #167

Add new field gatekeeperEnforcementAction to override enforcementAction #167

Uh oh!

Conversation

yiraeChristineKim commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

openshift-ci bot commented Jul 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yiraeChristineKim commented Jul 9, 2024 •

edited

Loading