-
Notifications
You must be signed in to change notification settings - Fork 328
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
chore: adding vap testing #618
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Jaydip Gabani <[email protected]>
8a749fe
to
05cf84f
Compare
Signed-off-by: Jaydip Gabani <[email protected]>
7dd57a7
to
0a8532a
Compare
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Makefile
Outdated
@@ -36,9 +36,9 @@ deploy: | |||
ifeq ($(POLICY_ENGINE), rego) |
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.
Can you add a comment here that only the first engine in the template gets used for evaluation UNLESS enableK8sNativeValidation=false which ensures the subsequent non-K8sNativeValidation engine gets used?
Makefile
Outdated
helm install -n gatekeeper-system gatekeeper gatekeeper/gatekeeper --create-namespace --version $(GATEKEEPER_VERSION) --set enableK8sNativeValidation=true | ||
endif | ||
else ifeq ($(POLICY_ENGINE), vap) |
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 these, since Gatekeeper webhook is a fallback for VAP, how do we ensure the failure resulted from VAP instead of the GK webhook?
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.
We check the violation string to make sure it includes ValidatingAdmissionPolicy
- https://github.com/open-policy-agent/gatekeeper-library/pull/618/files#diff-3b0266d06db240d302bf9febd17c09cf104dccce0bb2ee8e799e49c85f7082ffR96.
Makefile
Outdated
@@ -36,9 +36,9 @@ deploy: | |||
ifeq ($(POLICY_ENGINE), rego) |
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.
In general, I think because we are not actually testing the error message, it's hard to tell which engine caused the violation and which enforcement point caused the failure. Not sure how hard it is to add that, could be a follow up issue/PR if you want to track 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.
Since GK does not have fallback between engines, violations are thrown through CEL engine if it is enabled and CT has CEL. Otherwise the source of violation is rego engine.
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 GK does not have fallback between engines, violations are thrown through CEL engine if it is enabled and CT has CEL. Otherwise the source of violation is rego engine.
yea thats how it works today but if we ever expose priority like the issue you opened, then we need to test the actual violation message. maybe open an issue to track for future?
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.
In this case, we may want to modify the violation message to include engine information as well. Since as of now the violation message is the same regardless of the engine used to evaluate CTs. IMO, I don't think users apart from CT authors would care about which engine is being used to enforce policies. And CT authors would only care to verify the logic written for policy, which I think can be attained with gator
to test CT.
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
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
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 #574
Special notes for your reviewer: