-
Notifications
You must be signed in to change notification settings - Fork 553
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
IaC local execution experimental release #1 #1601
Conversation
|
067a821
to
913c536
Compare
913c536
to
806b5e6
Compare
fb7cad4
to
51e3e76
Compare
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.
👏 This looks great from my side. Only high level comments I have is that we should better document the entry-point in index.ts to be clear about what this new code path is and that the current code makes many uses of the term "legacy" but nowhere do we say what is legacy or why.
|
||
const readFileContentsAsync = util.promisify(fs.readFile); | ||
|
||
export default async function legacyWrapper(pathToScan: string, options) { |
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 is this a legacy wrapper? What are legacy results? Can we add types for options
and the return value?
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.
maybe the naming is inaccurate...
I didn't want the old structure and types to dictate how we re-build the new experience.
So this basically just does some "adapter" logic for the results to "plug&play" with the rest of the CLI, which made it easier to achieve lots of extra functionality like the --json
& --sarif
support.
got ideas for better naming?
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.
Oh I see, yes let's avoid legacy as the rest of the CLI will continue to use it. How about we refer to the output of this function as the "cli results" and the legacy-adapter.ts as the cli-results-formatter with a comment about how it takes the internal data model and formats it for consumption by the wider snyk cli helpers.
We can just call this function execute()
I think or test()
with a comment above to explain why it's doing the formatting.
Thoughts?
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.
removed all use of legacy
and renamed.
} | ||
|
||
return [ | ||
{ filePath: pathToScan, fileType: getFileType(pathToScan) as IacFileTypes }, |
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 we avoid this cast? Or document why it is needed.
docId: yamlDocuments.length > 1 ? docId : undefined, | ||
}); | ||
} else { | ||
throw new Error('Invalid K8s File!'); |
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 we include the missing fields here to help with debugging.
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.
If we throw
in one of the iterations, are we ok w/ throwing away all the process?
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.
@aviadatsnyk
for now yes, as it supports a single file scan.
when we add directory support (upcoming follow-up work), probably partial results are the right approach, which is also how it works now if I remember correctly..
Anyway, ones of our goal along this migration to the local-exec model, is to align with the experience we currently have today for the iac remote-processing cli flow.
c91706d
to
6ef4f48
Compare
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.
👍 Nice work, very excited to see this used internally.
// } | ||
// } | ||
|
||
function iacLocalFileScanToFormattedResult( |
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.
Should return a TestResult
type (defined in src/lib/snyk-test/legacy.ts
).
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.
absolutely correct, but for now adding a TODO on these as it's a typing rabbit-hole due to lack of accurate typing down the road in the rest of the CLI..
// CloudConfigFileTypes, | ||
// } from '@snyk/cloud-config-parser'; | ||
|
||
export function formatResults( |
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.
Should return a TestResult[]
type (defined in src/lib/snyk-test/legacy.ts
).
const fileDataToScan = await parseFileContentsForPolicyEngine( | ||
filePathsToScan, | ||
); | ||
const scanResults = await policyEngine.scanFiles(fileDataToScan); |
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.
maybe worth align this with the evaluate of CCPE (I saw that there is an internal evaluate function there - but maybe there it should be evaluateData or something similar)
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.
will consider that for next release.
although I don't want to have our old types & terminology restrict us here, as it's an opportunity to rethink naming as well.
@@ -58,6 +58,7 @@ | |||
"author": "snyk.io", | |||
"license": "Apache-2.0", | |||
"dependencies": { | |||
"@open-policy-agent/opa-wasm": "git+https://github.com/open-policy-agent/npm-opa-wasm.git#f4a21fe6f4d70706f85106dc6ea867983747e040", |
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 are we binding to a git commit and not the npm
distro?
https://www.npmjs.com/package/@open-policy-agent/opa-wasm
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.
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.
intentionally.
the package is not mature, and the latest tagged release is missing functionality that we are dependent on.
therefore, until they do a new release, we're targeting a specific commit-hash.
see:
open-policy-agent/npm-opa-wasm#31
// without these files this method would fail. | ||
// if you're interested in trying out the experimental local execution model for IaC scanning, please reach-out. | ||
export async function test(pathToScan: string, options) { | ||
// TODO: add support for proper typing of old TestResult interface. |
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.
this is a future todo
?
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.
yes, as it is a typing rabbit-hole, and I prefer not having the old types in the CLI dictate how we build the new experience.
+I want to focus on the bare-minimum of fields for the new experience data-structures, and not jam tons of fields just because the main CLI flow requires them.
so this will be typed, but soon :)
]; | ||
} | ||
|
||
const REQUIRED_K8S_FIELDS = ['apiVersion', 'kind', 'metadata']; |
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 we hoist this?
docId: yamlDocuments.length > 1 ? docId : undefined, | ||
}); | ||
} else { | ||
throw new Error('Invalid K8s File!'); |
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.
If we throw
in one of the iterations, are we ok w/ throwing away all the process?
import * as fs from 'fs'; | ||
|
||
const LOCAL_POLICY_ENGINE_DIR = `.iac-data`; | ||
const LOCAL_POLICY_ENGINE_WASM_PATH = `${LOCAL_POLICY_ENGINE_DIR}/policy.wasm`; |
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.
please use path.sep
instead of /
:)
} | ||
|
||
// | ||
// function getFileTypeForLineNumber( |
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.
🧹💨
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.
generally I 100% agree.
but keeping these intentionally, as this is logic that will be entered in one of the next follow-up PR's and is commented out due to a blocker.
wrote a comment on it here:
https://github.com/snyk/snyk/pull/1601/files#diff-2e4fd6421898cc5c5947d03530196bee66469cd25276fcdfcc0ba4381268b2c9R44
otherwise it will just get re-implemented soon.
return Object.values(groupedData); | ||
} | ||
|
||
const SEVERITIES = [SEVERITY.LOW, SEVERITY.MEDIUM, SEVERITY.HIGH]; |
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.
hoist please
import { SEVERITY } from '../../../../lib/snyk-test/common'; | ||
import { IacFileInDirectory } from '../../../../lib/types'; | ||
|
||
// eslint-disable-next-line |
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.
?
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.
wanted to do aliasing, but the linter doesn't like it.
replacing with:
export type IacFileMetadata = IacFileInDirectory;
which the linter is ok with.
@@ -0,0 +1,50 @@ | |||
#shellcheck shell=sh |
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 not #!/bin/sh
?
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're mainly adapting the standards used here.
@JackuB do you have an answer for that?
After snyk_logout | ||
|
||
Describe "k8s single file scan" | ||
It "finds issues in k8s file" |
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 are we testing this way?
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.
- it has lots of benefits for E2E integration tests which we can easily cover most of the core flows of our product.
- it's a new testing framework Hammer introduced to the CLI and we are ones of the first to adopt it out.
- our old test-coverage for
test iac
are mainly E2E tests which are written in Tap, and are deprecated and no longer maintained. - We will probably add later-on some Jest unit-tests, but for now we're fine with that coverage.
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.
Exciting!
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.
Please squash commit messages :)
87b270b
to
f8bd3f3
Compare
🎉 This PR is included in version 1.441.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Revert "Merge pull request #1601 from snyk/feat/iac-experimental-local-exec"
What does this PR do?
This PR adds a new
--experimental
flag to be used withsnyk iac test
.It is part of a wider project called Local Execution.
It adds new logic which does issue scanning locally with a provided
.wasm
file that will be manually given to Snykers for a dog-fooding release.This is intentionally not exposed yet under
--help
.This flag currently supports the following functionality:
It does not support yet:
Running Locally
In order to run locally, checkout the branch, build and run the local built version.
make sure to have a folder called
.iac-data
in your CWD where you're running the CLI from, and have thepolicy.wasm
&data.json
files in it (can be found in the attachments of: https://snyksec.atlassian.net/browse/CC-601)TODO