fsgroup cel #523
fsgroup cel #523
Conversation
ritazh
force-pushed
the
fsgroup-cel
branch
2 times, most recently
from
862f610 to
3b3f636
Compare
| expression: '!has(object.spec.securityContext) ? "" : !has(object.spec.securityContext.fsGroup) ? "" : object.spec.securityContext.fsGroup' | ||
| - name: input_fsGroup_allowed | ||
| expression: | | ||
| !has(variables.params.rule) ? true : variables.params.rule == "RunAsAny" ? true : variables.params.rule == "MayRunAs" && variables.fsGroup == "" ? true : (variables.params.rule == "MayRunAs" || variables.params.rule == "MustRunAs") && has(variables.params.ranges) && size(variables.params.ranges) > 0 ? variables.params.ranges.all(range, range.min <= variables.fsGroup && range.max >= variables.fsGroup) : false |
There was a problem hiding this comment.
seems like there might be a bug in respective rego code. If input.params.rule is missing the policy will deny create violations for all objects with rego code. However, desired behavior is where we dont want to throw violations when params.rule is missing.
thoughts? @ritazh @maxsmythe
There was a problem hiding this comment.
You are correct for completeness. Though the constraint is pretty useless without the rule specified.
For completeness, we can add the check has_field(input.parameters, "rule") in rego but technically it is not backward compatible if users have missing rule today.
For this, do we want to bump major or minor?
Major version bump required: Whenever there is a breaking change in the policy e.g. updating template Kind, updating existing parameter schema, adding the requires-sync-data annotation to sync new data, or any other breaking changes
Minor version bump required: Whenever there is a backward compatible change in the policy e.g. adding a parameter, updating Rego logic
There was a problem hiding this comment.
I think Major version just to be on safe side of things.
There was a problem hiding this comment.
I cannot parse the phrase "deny create violations" what is the behavior you are describing?
This line in the violation[] rule shows if parameters.rule is unset, no violation will be thrown:
has_field(input.parameters, "rule")
Perhaps write a test case that shows the situation-of-concern to validate whether there actually is drift?
There was a problem hiding this comment.
@maxsmythe I added has_field(input.parameters, "rule") in the rego https://github.com/open-policy-agent/gatekeeper-library/pull/523/files#diff-54ba54eb002245f40e88e03c102ebf4b4783d7f3f501cf73a5996119e48e062fR8 to ensure consistency between rego and cel. without it, rego will return a violation for a constraint with unspecified rule. Is this the right behavior? or should unspecified rule not return a violation.
If it does not return a violation, then the change would be considered as not backward compat. do we keep the same behavior for both cel and rego or do we correct this and bump the major version?
There was a problem hiding this comment.
Should we just make VAP-CEL return a violation if the parameter is undefined?
Technically the constraint is invalid if a required parameter is missing. I know we have returned violations for these kinds of problems before (e.g. RAM limits that cannot be canonicalized).
I'm also okay with calling the current Rego behavior a bug and changing it via patch/minor version update.
My logic there is that if there are any users who failed to set this parameter, they will have effectively created a DenyAll constraint for Pods... it seems unlikely that that was their intent or that they would keep a constraint in that condition.
TL;DR changing the CEL to conform to the Rego or vice versa both work for me.
Signed-off-by: Rita Zhang <[email protected]>
Signed-off-by: Rita Zhang <[email protected]>
|
This issue/PR 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. |
What this PR does / why we need it:
Which issue(s) does this PR fix (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #541
Special notes for your reviewer: