-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: adding scopedenforcementactions #403
Conversation
0c29c4c
to
b09049c
Compare
7bea138
to
8a991c7
Compare
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! A few comments, but looking good.
Signed-off-by: Jaydip Gabani <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #403 +/- ##
==========================================
- Coverage 54.68% 53.80% -0.89%
==========================================
Files 71 104 +33
Lines 5241 6745 +1504
==========================================
+ Hits 2866 3629 +763
- Misses 2073 2745 +672
- Partials 302 371 +69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@maxsmythe Sorry for the delay. I have addressed all the comments and implemented the feedback. PTAL. |
Signed-off-by: Jaydipkumar Arvindbhai Gabani <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries! Sorry for the delay on my end. Getting close!
constraint/pkg/client/drivers/k8scel/transform/make_vap_objects.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Jaydip Gabani <[email protected]>
} | ||
return result, nil | ||
} | ||
return nil, fmt.Errorf("scopedEnforcementActions value must be a []scopedEnforcementAction{action: string, enforcementPoints: []EnforcementPoint{name: string}}") |
There was a problem hiding this comment.
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}}") | ||
} | ||
} | ||
return result, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
Signed-off-by: Jaydip Gabani <[email protected]>
@@ -45,3 +64,101 @@ func GetEnforcementAction(constraint *unstructured.Unstructured) (string, error) | |||
|
|||
return action, nil | |||
} | |||
|
|||
func IsEnforcementActionScoped(action string) bool { | |||
return strings.EqualFold(action, EnforcementActionScoped) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
for _, scopedEA := range scopedEnforcementActions { | ||
for _, enforcementPoint := range scopedEA.EnforcementPoints { | ||
epName := strings.ToLower(enforcementPoint.Name) | ||
ea := strings.ToLower(scopedEA.Action) |
There was a problem hiding this comment.
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)
for _, enforcementPoint := range scopedEA.EnforcementPoints { | ||
epName := strings.ToLower(enforcementPoint.Name) | ||
ea := strings.ToLower(scopedEA.Action) | ||
// If enforceAll is true, or the enforcement point is explicitly listed, initialize its action map |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary comment
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
case string(Warn), string(Dryrun): | ||
action = scopedEA.Action | ||
default: | ||
action = string(Deny) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt we return error? we cant assume non valid values just all default to deny. we can only set to deny when enforcementAction is not provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/open-policy-agent/gatekeeper/blob/bd96c5263523be54642dcb4a88c2a9e502a74ea7/pkg/webhook/policy.go#L300-L309 this code assumes not dryrun, not warn, means deny because it's processing the response returned by the driver. but we should not make that assumption here.
https://github.com/open-policy-agent/gatekeeper/blob/bd96c5263523be54642dcb4a88c2a9e502a74ea7/pkg/webhook/policy.go should also be updated to include scope otherwise, it will default to deny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the behavior is, if someone turns off the disable-enforcementaction-validation
flag on gatekeeper, then constraints can be created with whatever enforcementAction (enforcementAction: fake
) - Link. In this case, constraint gets added to the template client regardless of the "valid/invalid" enforcement action. Keeping the behavior same IMO we should just pass whatever scoped enforcement action is in the constraint without validating it when adding the constraint to template client. The mechanism to handle this already exists in gatekeeper when we receive responses in gatekeeper with "unrecognised" actions - Link.
By default, we are vlidating enforcement action and scoped enforcement actions to reject constraint with not valid enforcement action. enforcementAction: deny, dryrun, warn, scoped
and scopedEnforcementActions[].action: deny, dryrun, warn
So should I remove this block altogether then? I dont think we need this check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/open-policy-agent/gatekeeper/blob/bd96c5263523be54642dcb4a88c2a9e502a74ea7/pkg/webhook/policy.go should also be updated to include scope otherwise, it will default to deny.
I think we should keep this as is because when response include scoped
enforcement action, the implementation looks at scoped
enforcement actions. https://github.com/open-policy-agent/gatekeeper/pull/3321/files#diff-d3c8e52b699d3596d1057b82890a2ed8624445f1ee8f6d3d8a532e13766b99a8R261
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rita is correct, the code should be removed.
The fact that constraints can be created with arbitrary enforcement actions is intentional. This is because we cannot know, and should not presume to know, what enforcement actions are available to any EPs.
We should not validate enforcement actions and just take them as we come.
If EPs want to perform additional validation (e.g. Gatekeeper raising a warning/reporting via status
if it is on the hook for taking an action it does not recognize), that's fine, but CF is not equipped to do that. At best it can provide helper functions to allow EPs to implement that extra porcelain.
Assuming unrecognized actions == deny is not a good assumption to make. Example: if Gatekeeper adds a PubSub action, the CF would rewrite it as deny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should not return an error. As I alluded to earlier, CF does not and should not know the full set of valid enforcement actions, it just forwards them to EPs, who are responsible for interpreting them.
… client Signed-off-by: Jaydip Gabani <[email protected]>
if apiconstraints.IsEnforcementActionScoped(c.enforcementAction) { | ||
for _, ep := range enforcementPoints { | ||
var actions []string | ||
if ep == apiconstraints.AllEnforcementPoints { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*
cannot be provided to matches()
, calls to matches()
must provide a specific enforcement point.
Remember, *
only makes sense when being specified on the constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. sorry for missing this. FIxed it.
Signed-off-by: Jaydip Gabani <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, have not reviewed tests, but I defer to Rita there.
} | ||
for _, scopedEA := range scopedEnforcementActions { | ||
for _, enforcementPoint := range scopedEA.EnforcementPoints { | ||
epName := strings.ToLower(enforcementPoint.Name) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
constraint/pkg/client/client_opts.go
Outdated
func EnforcementPoints(eps ...string) Opt { | ||
return func(client *Client) error { | ||
for i, ep := range eps { | ||
eps[i] = strings.ToLower(ep) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
Signed-off-by: Jaydip Gabani <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* adding scopedenforcementactions Signed-off-by: Jaydip Gabani <[email protected]> * adding tests Signed-off-by: Jaydip Gabani <[email protected]> * fixing tests Signed-off-by: Jaydip Gabani <[email protected]> * refactoring template client and query Signed-off-by: Jaydip Gabani <[email protected]> * refatoring queryopts to reviewopts Signed-off-by: Jaydip Gabani <[email protected]> * updating ccomments Signed-off-by: Jaydip Gabani <[email protected]> * removing EP variables Signed-off-by: Jaydip Gabani <[email protected]> * generic webhook EP Signed-off-by: Jaydip Gabani <[email protected]> * simplifying constraintToBinding Signed-off-by: Jaydip Gabani <[email protected]> * adding comments Signed-off-by: Jaydip Gabani <[email protected]> * checking lowercase eps, fixing nil variable access Signed-off-by: Jaydip Gabani <[email protected]> * fixing file perm Signed-off-by: Jaydip Gabani <[email protected]> * adding tests for case sensitivity and missing enforcementaction Signed-off-by: Jaydip Gabani <[email protected]> * updating gk-webhook EP Signed-off-by: Jaydip Gabani <[email protected]> * fixing test Signed-off-by: Jaydip Gabani <[email protected]> * adding enforcement action and scoped enforcement actions in result and response spec Signed-off-by: Jaydip Gabani <[email protected]> * fixing faulty test Signed-off-by: Jaydip Gabani <[email protected]> * refactoring code for simplycity Signed-off-by: Jaydip Gabani <[email protected]> * removing comments, removing * from review opts and client opts Signed-off-by: Jaydip Gabani <[email protected]> * mandating client to be initialized with enforcment point Signed-off-by: Jaydip Gabani <[email protected]> * case sensitive check for actions Signed-off-by: Jaydip Gabani <[email protected]> * updating code comments Signed-off-by: Jaydip Gabani <[email protected]> * removing comments Signed-off-by: Jaydip Gabani <[email protected]> * removing enforcement action check while adding constriant to template client Signed-off-by: Jaydip Gabani <[email protected]> * removing all enforcement points from matches Signed-off-by: Jaydip Gabani <[email protected]> * preserving enforcement point names Signed-off-by: Jaydip Gabani <[email protected]> --------- Signed-off-by: Jaydip Gabani <[email protected]> Signed-off-by: Jaydipkumar Arvindbhai Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]> feat: adding scopedenforcementactions (open-policy-agent#403) * adding scopedenforcementactions Signed-off-by: Jaydip Gabani <[email protected]> * adding tests Signed-off-by: Jaydip Gabani <[email protected]> * fixing tests Signed-off-by: Jaydip Gabani <[email protected]> * refactoring template client and query Signed-off-by: Jaydip Gabani <[email protected]> * refatoring queryopts to reviewopts Signed-off-by: Jaydip Gabani <[email protected]> * updating ccomments Signed-off-by: Jaydip Gabani <[email protected]> * removing EP variables Signed-off-by: Jaydip Gabani <[email protected]> * generic webhook EP Signed-off-by: Jaydip Gabani <[email protected]> * simplifying constraintToBinding Signed-off-by: Jaydip Gabani <[email protected]> * adding comments Signed-off-by: Jaydip Gabani <[email protected]> * checking lowercase eps, fixing nil variable access Signed-off-by: Jaydip Gabani <[email protected]> * fixing file perm Signed-off-by: Jaydip Gabani <[email protected]> * adding tests for case sensitivity and missing enforcementaction Signed-off-by: Jaydip Gabani <[email protected]> * updating gk-webhook EP Signed-off-by: Jaydip Gabani <[email protected]> * fixing test Signed-off-by: Jaydip Gabani <[email protected]> * adding enforcement action and scoped enforcement actions in result and response spec Signed-off-by: Jaydip Gabani <[email protected]> * fixing faulty test Signed-off-by: Jaydip Gabani <[email protected]> * refactoring code for simplycity Signed-off-by: Jaydip Gabani <[email protected]> * removing comments, removing * from review opts and client opts Signed-off-by: Jaydip Gabani <[email protected]> * mandating client to be initialized with enforcment point Signed-off-by: Jaydip Gabani <[email protected]> * case sensitive check for actions Signed-off-by: Jaydip Gabani <[email protected]> * updating code comments Signed-off-by: Jaydip Gabani <[email protected]> * removing comments Signed-off-by: Jaydip Gabani <[email protected]> * removing enforcement action check while adding constriant to template client Signed-off-by: Jaydip Gabani <[email protected]> * removing all enforcement points from matches Signed-off-by: Jaydip Gabani <[email protected]> * preserving enforcement point names Signed-off-by: Jaydip Gabani <[email protected]> --------- Signed-off-by: Jaydip Gabani <[email protected]> Signed-off-by: Jaydipkumar Arvindbhai Gabani <[email protected]> chore: bump golang from `b03f3ba` to `5c56bd4` in /constraint (open-policy-agent#434) Bumps golang from `b03f3ba` to `5c56bd4`. --- updated-dependencies: - dependency-name: golang dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Sertaç Özercan <[email protected]> removing test file Signed-off-by: Jaydip Gabani <[email protected]>
This PR implements changes for multi ea/ep design.
The gists of the changes are:
For gatekeeper changes and CI tests refer to PR gatekeeper/3321