Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adding scopedenforcementactions #403

Merged
merged 34 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
4aabc45
adding scopedenforcementactions
JaydipGabani Mar 21, 2024
81b2d08
adding tests
JaydipGabani Mar 21, 2024
9deaab2
fixing tests
JaydipGabani Mar 21, 2024
909437e
refactoring template client and query
JaydipGabani Apr 8, 2024
e837a1e
Merge branch 'master' into multi-ea
JaydipGabani Apr 10, 2024
a6f1641
refatoring queryopts to reviewopts
JaydipGabani Apr 10, 2024
46a172b
updating ccomments
JaydipGabani Apr 11, 2024
7a2ff49
removing EP variables
JaydipGabani Apr 12, 2024
e054366
generic webhook EP
JaydipGabani Apr 18, 2024
9bbfbc2
Merge branch 'master' into multi-ea
JaydipGabani Jul 1, 2024
076751a
Merge branch 'master' into multi-ea
JaydipGabani Jul 8, 2024
8f28a01
simplifying constraintToBinding
JaydipGabani Jul 9, 2024
1c6b9d8
adding comments
JaydipGabani Jul 9, 2024
ae81539
Merge branch 'master' into multi-ea
JaydipGabani Jul 15, 2024
5af2f0c
Merge branch 'master' into multi-ea
JaydipGabani Jul 16, 2024
a654a76
checking lowercase eps, fixing nil variable access
JaydipGabani Jul 17, 2024
3cb703c
fixing file perm
JaydipGabani Jul 17, 2024
03dd160
Merge branch 'master' into multi-ea
JaydipGabani Jul 17, 2024
5080e95
Merge branch 'master' into multi-ea
JaydipGabani Jul 22, 2024
d996311
adding tests for case sensitivity and missing enforcementaction
JaydipGabani Jul 23, 2024
759157f
Merge branch 'multi-ea' of github.com:JaydipGabani/frameworks into mu…
JaydipGabani Jul 23, 2024
c9ba827
updating gk-webhook EP
JaydipGabani Jul 23, 2024
7cbf00d
fixing test
JaydipGabani Jul 23, 2024
86bae52
adding enforcement action and scoped enforcement actions in result an…
JaydipGabani Jul 24, 2024
2428b21
fixing faulty test
JaydipGabani Jul 25, 2024
6456bd6
refactoring code for simplycity
JaydipGabani Jul 26, 2024
7fefe2d
removing comments, removing * from review opts and client opts
JaydipGabani Jul 26, 2024
9fdd218
mandating client to be initialized with enforcment point
JaydipGabani Jul 26, 2024
bbefecf
case sensitive check for actions
JaydipGabani Jul 26, 2024
ac65fcf
updating code comments
JaydipGabani Jul 26, 2024
d87f02f
removing comments
JaydipGabani Jul 26, 2024
675353b
removing enforcement action check while adding constriant to template…
JaydipGabani Jul 26, 2024
ce5a973
removing all enforcement points from matches
JaydipGabani Jul 26, 2024
67de36b
preserving enforcement point names
JaydipGabani Jul 31, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions constraint/deploy/tools.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//go:build tools
// +build tools

// This existence of this package allows vendoring of the manifests in this directory by go 1.13+.
Expand Down
117 changes: 117 additions & 0 deletions constraint/pkg/apis/constraints/apis.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,21 @@ package constraints
import (
"errors"
"fmt"
"strings"

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
)

type ScopedEnforcementAction struct {
Action string `json:"action"`
EnforcementPoints []EnforcementPoint `json:"enforcementPoints"`
}

type EnforcementPoint struct {
Name string `json:"name"`
}

const (
// Group is the API Group of Constraints.
Group = "constraints.gatekeeper.sh"
Expand All @@ -17,6 +28,11 @@ const (
//
// This is the default EnforcementAction.
EnforcementActionDeny = "deny"

EnforcementActionScoped = "scoped"

// AllEnforcementPoints is a wildcard to indicate all enforcement points.
AllEnforcementPoints = "*"
)

var (
Expand All @@ -26,6 +42,9 @@ var (

// ErrSchema is a specific error that a Constraint failed schema validation.
ErrSchema = errors.New("schema validation failed")

// ErrMissingRequiredField is a specific error that a field is missing from a Constraint.
ErrMissingRequiredField = errors.New("missing required field")
)

// GetEnforcementAction returns a Constraint's enforcementAction, which indicates
Expand All @@ -45,3 +64,101 @@ func GetEnforcementAction(constraint *unstructured.Unstructured) (string, error)

return action, nil
}

func IsEnforcementActionScoped(action string) bool {
return strings.EqualFold(action, EnforcementActionScoped)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like other checks for enforcement action are case sensitive. We should follow the same pattern.

https://github.com/open-policy-agent/gatekeeper/blob/bd96c5263523be54642dcb4a88c2a9e502a74ea7/pkg/webhook/policy.go#L300-L309

Copy link
Contributor Author

@JaydipGabani JaydipGabani Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxsmythe were you meaning to refer to some other codeblock? I tested the switch in the code you shared, it is defaulting when r.enforcementAction is mis-matching with string(util.Dryrun) or string(util.Warn) with case sensitive r.enforcementAction.

Copy link
Member

@ritazh ritazh Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JaydipGabani From the code Max linked, it means we should preserve the case sensitivity of enforcementAction instead of converting it to all lowercase. Currently, only these are expected https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/util/enforcement_action.go#L14-L18 so unless users pass in deny, dryrun, warn, scoped (new one that we should add to that list), we should set it as unrecognized.

Copy link
Contributor Author

@JaydipGabani JaydipGabani Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ritazh Thanks for the clarification. Sorry for being confused. Checking for case sensitve matching for actions now.

}

// GetEnforcementActionsForEP returns a map of enforcement actions for enforcement points passed in.
func GetEnforcementActionsForEP(constraint *unstructured.Unstructured, eps []string) (map[string]map[string]bool, error) {
if len(eps) == 0 {
return nil, fmt.Errorf("enforcement points must be provided to get enforcement actions")
}

scopedActions, found, err := getNestedFieldAsArray(constraint.Object, "spec", "scopedEnforcementActions")
JaydipGabani marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, fmt.Errorf("%w: invalid spec.enforcementActionPerEP", ErrInvalidConstraint)
}
if !found {
return nil, fmt.Errorf("%w: spec.scopedEnforcementAction must be defined", ErrMissingRequiredField)
}

scopedEnforcementActions, err := convertToSliceScopedEnforcementAction(scopedActions)
if err != nil {
return nil, fmt.Errorf("%w: %w", ErrInvalidConstraint, err)
}

// Flag to indicate if all enforcement points should be enforced
enforceAll := false
// Initialize a map to hold enforcement actions for each enforcement point
actionsForEPs := make(map[string]map[string]bool)
// Populate the actionsForEPs map with enforcement points from eps, initializing their action maps
for _, enforcementPoint := range eps {
if enforcementPoint == AllEnforcementPoints {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some reason AllEnforcementPoints would be passed to this function?

The behavior if "*" is passed here and also exists in scopedEnforcementPoints seems poorly-defined.

Copy link
Contributor Author

@JaydipGabani JaydipGabani Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering that reviewopts and client would have * to support all enforcement points and that made the whole implementation complex. I reverted to your suggestion that wildcard should not be allowed in reviewops and client. So not there is no reson that AllEnforcementPoints would be passed to this function.

enforceAll = true // Set enforceAll to true if the special identifier for all enforcement points is found
}
actionsForEPs[enforcementPoint] = make(map[string]bool) // Initialize the action map for the enforcement point
}

// Iterate over the scoped enforcement actions to populate actions for each enforcement point
for _, scopedEA := range scopedEnforcementActions {
for _, enforcementPoint := range scopedEA.EnforcementPoints {
epName := strings.ToLower(enforcementPoint.Name)
Copy link
Member

@ritazh ritazh Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to do this here since GetEnforcementAction today just passes the value thru as is.
https://github.com/open-policy-agent/frameworks/pull/403/files#diff-5719413b9620718b003f306059ddf823427e2b3b1ee5ab983d74feac8bc850e8L36-L46

If it's not a match compare to the supported EPs, then GK will show it as unrecognized.
https://github.com/open-policy-agent/gatekeeper/pull/3321/files#diff-74281531e73b25d4771eb774e82a3268a57231ca7eb49c03ae6c35366544cb81R80

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since EP can be any string, my preference would be to not converting it to lowercase to preserve it. how do others feel? @maxsmythe @sozercan ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm down with preserving case

ea := strings.ToLower(scopedEA.Action)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with pre-existing code, case-sensitive comparison (see G8r code link above)

// If enforceAll is true, or the enforcement point is explicitly listed, initialize its action map
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is inaccurate (does not initialize if EP is explicitly listed), and also unnecessary, since it merely describes the if statement and does not explain anything nuanced or unintuitive.

if _, ok := actionsForEPs[epName]; !ok && enforceAll {
actionsForEPs[epName] = make(map[string]bool)
}
// Skip adding actions for enforcement points not in the list unless enforceAll is true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary comment

if _, ok := actionsForEPs[epName]; !ok && epName != AllEnforcementPoints {
continue
}
// If the enforcement point is the special identifier for all, apply the action to all enforcement points
switch epName {
case AllEnforcementPoints:
for ep := range actionsForEPs {
actionsForEPs[ep][ea] = true
}
default:
actionsForEPs[epName][ea] = true
}
}
}

return actionsForEPs, nil
}

// Helper function to access nested fields as an array.
func getNestedFieldAsArray(obj map[string]interface{}, fields ...string) ([]interface{}, bool, error) {
value, found, err := unstructured.NestedFieldNoCopy(obj, fields...)
if err != nil {
return nil, false, err
}
if !found {
return nil, false, nil
}
if arr, ok := value.([]interface{}); ok {
return arr, true, nil
}
return nil, false, nil
}

// Helper function to convert a value to a []ScopedEnforcementAction.
func convertToSliceScopedEnforcementAction(value interface{}) ([]ScopedEnforcementAction, error) {
var result []ScopedEnforcementAction
if arr, ok := value.([]interface{}); ok {
for _, v := range arr {
if m, ok := v.(map[string]interface{}); ok {
ritazh marked this conversation as resolved.
Show resolved Hide resolved
scopedEA := &ScopedEnforcementAction{}
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(m, scopedEA); err != nil {
return nil, err
}
result = append(result, *scopedEA)
} else {
return nil, fmt.Errorf("scopedEnforcementActions value must be a []scopedEnforcementAction{action: string, enforcementPoints: []EnforcementPoint{name: string}}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can consider checking these conditions first, then return. this will reduce the number of elses you need in the code.

}
}
return result, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

}
return nil, fmt.Errorf("scopedEnforcementActions value must be a []scopedEnforcementAction{action: string, enforcementPoints: []EnforcementPoint{name: string}}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

}
212 changes: 212 additions & 0 deletions constraint/pkg/apis/constraints/apis_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
package constraints

import (
"reflect"
"testing"

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

const (
// WebhookEnforcementPoint is the enforcement point for admission.
WebhookEnforcementPoint = "validation.gatekeeper.sh"

// AuditEnforcementPoint is the enforcement point for audit.
AuditEnforcementPoint = "audit.gatekeeper.sh"

// GatorEnforcementPoint is the enforcement point for gator cli.
GatorEnforcementPoint = "gator.gatekeeper.sh"
)

func TestGetEnforcementActionsForEP(t *testing.T) {
tests := []struct {
name string
constraint *unstructured.Unstructured
eps []string
expected map[string]map[string]bool
err error
}{
{
name: "wildcard enforcement point",
constraint: &unstructured.Unstructured{
Object: map[string]interface{}{
"spec": map[string]interface{}{
"scopedEnforcementActions": []interface{}{
map[string]interface{}{
"enforcementPoints": []interface{}{
map[string]interface{}{
"name": AuditEnforcementPoint,
},
map[string]interface{}{
"name": WebhookEnforcementPoint,
},
},
"action": "warn",
},
map[string]interface{}{
"enforcementPoints": []interface{}{
map[string]interface{}{
"name": "*",
},
},
"action": "deny",
},
},
},
},
},
expected: map[string]map[string]bool{
AuditEnforcementPoint: {
"warn": true,
"deny": true,
},
WebhookEnforcementPoint: {
"warn": true,
"deny": true,
},
GatorEnforcementPoint: {
"deny": true,
},
},
eps: []string{AuditEnforcementPoint, WebhookEnforcementPoint, GatorEnforcementPoint},
},
{
name: "Actions for selective enforcement point with case sensitive input",
constraint: &unstructured.Unstructured{
Object: map[string]interface{}{
"spec": map[string]interface{}{
"scopedEnforcementActions": []interface{}{
map[string]interface{}{
"enforcementPoints": []interface{}{
map[string]interface{}{
"name": AuditEnforcementPoint,
},
map[string]interface{}{
"name": "Validation.Gatekeeper.Sh",
},
},
"action": "Warn",
},
map[string]interface{}{
"enforcementPoints": []interface{}{
map[string]interface{}{
"name": "*",
},
},
"action": "deny",
},
},
},
},
},
expected: map[string]map[string]bool{
ritazh marked this conversation as resolved.
Show resolved Hide resolved
WebhookEnforcementPoint: {
"warn": true,
"deny": true,
},
GatorEnforcementPoint: {
"deny": true,
},
},
eps: []string{WebhookEnforcementPoint, GatorEnforcementPoint},
},
{
name: "wildcard enforcement point in scoped enforcement action, get actions for all enforcement points",
constraint: &unstructured.Unstructured{
Object: map[string]interface{}{
"spec": map[string]interface{}{
"scopedEnforcementActions": []interface{}{
map[string]interface{}{
"enforcementPoints": []interface{}{
map[string]interface{}{
"name": AuditEnforcementPoint,
},
map[string]interface{}{
"name": WebhookEnforcementPoint,
},
},
"action": "warn",
},
map[string]interface{}{
"enforcementPoints": []interface{}{
map[string]interface{}{
"name": AllEnforcementPoints,
},
},
"action": "deny",
},
},
},
},
},
expected: map[string]map[string]bool{
AuditEnforcementPoint: {
"warn": true,
"deny": true,
},
WebhookEnforcementPoint: {
"warn": true,
"deny": true,
},
AllEnforcementPoints: {
"deny": true,
},
},
eps: []string{AllEnforcementPoints},
},
{
name: "wildcard enforcement point in scoped enforcement action, get actions for two enforcement points",
constraint: &unstructured.Unstructured{
Object: map[string]interface{}{
"spec": map[string]interface{}{
"scopedEnforcementActions": []interface{}{
map[string]interface{}{
"enforcementPoints": []interface{}{
map[string]interface{}{
"name": AuditEnforcementPoint,
},
map[string]interface{}{
"name": WebhookEnforcementPoint,
},
},
"action": "warn",
},
map[string]interface{}{
"enforcementPoints": []interface{}{
map[string]interface{}{
"name": AllEnforcementPoints,
},
},
"action": "deny",
},
},
},
},
},
expected: map[string]map[string]bool{
AuditEnforcementPoint: {
"warn": true,
"deny": true,
},
WebhookEnforcementPoint: {
"warn": true,
"deny": true,
},
},
eps: []string{WebhookEnforcementPoint, AuditEnforcementPoint},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
actions, err := GetEnforcementActionsForEP(tt.constraint, tt.eps)
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if !reflect.DeepEqual(actions, tt.expected) {
t.Errorf("Expected %v, got %v", tt.expected, actions)
}
})
}
}
1 change: 1 addition & 0 deletions constraint/pkg/apis/templates/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading