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

chore: adding vap testing #618

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

JaydipGabani
Copy link
Contributor

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:

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)
Copy link
Member

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)
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makefile Outdated
@@ -36,9 +36,9 @@ deploy:
ifeq ($(POLICY_ENGINE), rego)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

@JaydipGabani JaydipGabani requested review from a team and ritazh January 6, 2025 22:32
Signed-off-by: Jaydip Gabani <[email protected]>
Copy link
Member

@ritazh ritazh left a 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create a new set of tests with vap generation
2 participants