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

feat: yaml support for iac wasm bundles [CFG-1271] #2467

Merged
merged 2 commits into from
Jan 4, 2022

Conversation

aron
Copy link
Contributor

@aron aron commented Dec 15, 2021

What does this PR do?

This PR bumps @open-policy-agent/opa-wasm to 1.6.0 to include support for the yaml.unmarshal() function in Rego and support for Node 10. This allows the snyk iac command to support additional policies that unmarshall YAML content within Terraform configuration.

Where should the reviewer start?

package.json is the primary change here but the implementation of yaml.unmarshal() is here: open-policy-agent/npm-opa-wasm#100

This also removes the use of the mock-fs package in the file-scanner.spec.ts test as the opa-wasm package will use console.error to write out a warning to stderr when loading the wasm fixture and this somehow causes mock-fs to throw the following error bringing the test suite down:

ENOENT: no such file or directory, lstat '/Users/Aron/Code/snyk/node_modules/@jest/console/node_modules'

Rather than debug this weird behavior we've decided to just mock out the function that loads the fixtures directly in the test suite.

How should this be manually tested?

rules.zip

Download the above zip file containing a custom rules bundle and unzip:

With the snyk-iac-rules tool build a custom bundle:

% snyk-iac-rules build

Then test the fixture with the latest snyk cli:

% snyk-dev iac test --rules=bundle.tar.gz rules/HELLO/fixtures/sg.tf

You should see 15 issues found vs. 14 when run with current snyk.

What are the relevant tickets?

CFG-1271

@aron aron requested review from a team as code owners December 15, 2021 11:18
@aron aron requested a review from rontalx December 15, 2021 12:49
@ipapast ipapast requested review from p0tr3c and removed request for rontalx December 15, 2021 12:51
@ipapast
Copy link
Contributor

ipapast commented Dec 15, 2021

I validated it locally by generating a new bundle and running with the new version: ✅
image

Edit: This fails when being run with node 10

@ipapast ipapast force-pushed the feat/iac-yaml-support branch 8 times, most recently from b0ca454 to 4f01006 Compare December 16, 2021 13:52
@aron
Copy link
Contributor Author

aron commented Dec 16, 2021

I tracked this down to an issue in the npm-opa-wasm package. Looks like something changed in the WASM API between Node 10 and 12. So when we look at the part of the opa-wasm package that checks the ABI version:

https://github.com/open-policy-agent/npm-opa-wasm/blob/f1be838de45e3310e7c37bcebbfa387ac17c4db3/src/opa.js#L195-L212

In Node 12+ we see the wasm.instance.exports.opa_wasm_abi_version is an object with a version property that returns the value 1. In Node 10 the value is just a number 1 so when it tries to access abiVersionGlobal.value it get's undefined.

If we patch the code to remove the .value the tests pass fine on Node 10 :/ so it looks like we'll need to open another PR on the opa-wasm project.

@ipapast ipapast removed the request for review from p0tr3c December 20, 2021 12:09
@ipapast
Copy link
Contributor

ipapast commented Dec 20, 2021

For whoever picks it up next - the last 2 (or even 3) commits can be reverted.

Aron has actually had the PR merged with the fix already open-policy-agent/npm-opa-wasm#108, so when this is a new release in that repo, revert these commits and include the new version in the package.json here.

aron and others added 2 commits January 4, 2022 10:56
@aron aron force-pushed the feat/iac-yaml-support branch from 01f1b6b to ce9fbe4 Compare January 4, 2022 10:56
@aron
Copy link
Contributor Author

aron commented Jan 4, 2022

This has now been updated to use the latest opa-wasm release with Node 10 support. All tests are now passing but I wasn't able to remove the mock-fs workaround. Our fixture still doesn't have the correct opa_wasm_abi_version and I'm not sure what is needed to get it working.

@ipapast would you mind re-reviewing on behalf of @snyk/group-infrastructure-as-code?

Copy link
Contributor

@ipapast ipapast left a comment

Choose a reason for hiding this comment

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

This is tested by generating a new bundle with the snyk-iac-rules tool and then running a scan with the CLI using node versions (16, 14, 12, 10).

> ./snyk-iac-rules build rules
Generated bundle: bundle.tar.gz

The following brings up 15 issues as expected:

> snyk-dev iac test --rules=../bundle.tar.gz rules/HELLO/fixtures/sg.tf

Attaching screenshots for the buggy version of node10 as well.
image

@aron aron merged commit 0c76461 into master Jan 4, 2022
@aron aron deleted the feat/iac-yaml-support branch January 4, 2022 12:44
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.

3 participants