-
Notifications
You must be signed in to change notification settings - Fork 321
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
ci: testing with cel policies #519
Conversation
Signed-off-by: Jaydip Gabani <[email protected]>
@maxsmythe I added CEL source from your draft PR #503 here as well, that leaves you with 3 other policies to write CEL source for. |
.github/workflows/workflow.yaml
Outdated
@@ -65,7 +65,8 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
gatekeeper: [ "release-3.13", "release-3.14" ] | |||
gatekeeper: [ "3.14.2", "3.15.1" ] |
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.
why do we prefer to use specific patch versions instead of the latest?
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.
AFAIK, we have always tested with past two release versions. I am fine with testing it with latest, unless others have any objections.
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.
using the branch instead of the tag ensures we are always testing against the latest patch version without having to push a PR to update it.
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.
AFAIK helm can only install specifi version, I am not sure if it can take a branch and install the latest patch on that branch. If we do not use helm, then we would need to use sed
or something similar to modify the file and set the flag manually.
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.
+1 to @JaydipGabani i think this is a better approach now since we previously installed from yaml directly
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.
ah I see. I'm ok with merging this as is. but it would be better to not having to bump the tag all the time. @JaydipGabani you can create an issue to do it as a follow up.
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.
Sure!
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 need to bump the patch version. We'll need to bump the minor version, which is same as today. I am not sure if there's a way we can automate that, but this can be a part of post GK release checklist
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]>
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.
Couple comments on the CEL code. Changes to testing flow LGTM!
Signed-off-by: Jaydip Gabani <[email protected]>
@@ -4,7 +4,7 @@ metadata: | |||
name: k8srequiredlabels | |||
annotations: | |||
metadata.gatekeeper.sh/title: "Required Labels" | |||
metadata.gatekeeper.sh/version: 1.0.1 | |||
metadata.gatekeeper.sh/version: 1.0.2 |
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.
metadata.gatekeeper.sh/version: 1.0.2 | |
metadata.gatekeeper.sh/version: 1.1.0 |
do we want to at least bump the minor version? @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.
Makes sense.
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]>
Can you drop the CEL from my draft PR? I'd rather only have one SOT for that, and just rebase/merge once the build system is ready, otherwise feedback will get clobbered. |
Signed-off-by: Jaydip Gabani <[email protected]>
Per this #519 (comment) all updates for "Allow Privilege Escalation" policy have been removed |
@@ -4,7 +4,7 @@ metadata: | |||
name: k8srequiredlabels | |||
annotations: | |||
metadata.gatekeeper.sh/title: "Required Labels" | |||
metadata.gatekeeper.sh/version: 1.0.1 | |||
metadata.gatekeeper.sh/version: 1.0.2 |
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.
Makes sense.
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
@JaydipGabani ptal conflicts |
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.
1 minor comment, otherwise lgtm
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
Was able to get some time to look!
What this PR does / why we need it:
gator verify
with cel and rego enginesrequiredLables
andallow-priviledge-escalation
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 #
Special notes for your reviewer: