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

fix: check if settings field is not nil. #28

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Conversation

jvanz
Copy link
Member

@jvanz jvanz commented Apr 8, 2024

Description

The settings fields could be nil when user does not define the settings for some of the resource type. Therefore, when the policy access the field to check if it should ignores the values defined by the user, the policy must check if the settings is not nil first. Thus, a method has been added to encapsulate this logic.

Fix #27

Test

make annotated-policy.wasm test e2e-tests --always-make

Also try it out in a running policy server. I'll keep a tag with this fix in my ghcr.io registry for a while

@jvanz jvanz self-assigned this Apr 8, 2024
@jvanz jvanz requested a review from a team as a code owner April 8, 2024 19:38
Adds additional tests to trigger a nil pointer dereference in the policy
execution. This can happen because the policy settings fields are
pointer which can be nil when user does not want to validate those
resources. This commit adds two tests to simulate this situation.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

wip

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
The settings fields could be nil when user does not define the settings
for some of the resource type. Therefore, when the policy access the
field to check if it should ignores the values defined by the user, the
policy must check if the settings is not nil first. Thus, a method has
been added to encapsulate this logic.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
@jvanz
Copy link
Member Author

jvanz commented Apr 8, 2024

@TechDufus, can you test this fix as well? Thanks!

@TechDufus
Copy link
Contributor

@jvanz I'm no longer seeing the policy crash, but I'd like to let this run until tomorrow to see if the audit-scanner picks up on me having a deployment beyond my defined allowed limit. :)

Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM!

@TechDufus
Copy link
Contributor

SHIP IT TO PROD. :)

@jvanz jvanz merged commit 8d3ae7c into kubewarden:main Apr 9, 2024
4 checks passed
@jvanz jvanz deleted the issue27 branch April 9, 2024 13:55
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.

Error: wasm trap: wasm unreachable instruction executed
3 participants