fix: consistent command termination between --json and no --json flag #1870
Conversation
Co-authored-by: Ron Tal <ron.tal@snyk.io>
Co-authored-by: Ron Tal <ron.tal@snyk.io>
|
Expected release notes (by @teodora-sandu) fixes: others (will not be included in Semantic-Release notes):
|
|
| const { | ||
| result: { projectType }, | ||
| ...filteredIacTest | ||
| } = iacTest; | ||
| return { | ||
| ...filteredIacTest, | ||
| projectType, | ||
| [IAC_ISSUES_KEY]: | ||
| iacTest?.result?.cloudConfigResults.map(mapIacIssue) || [], | ||
| ok: infrastructureAsCodeIssues.length === 0, |
There was a problem hiding this comment.
The command should return with status code 1 if there are any vulnerabilities. So setting ok to false if there are any vulnerabilities takes care of that
|
|
||
| Describe "Testing status code when issues found" | ||
| Describe "Using the --json flag" | ||
| It "returns 1 even if some files failed to parse" | ||
| When run snyk iac test ../fixtures/iac/kubernetes/ --experimental --json | ||
| The status should equal 1 | ||
| The output should not equal "" | ||
| The stderr should equal "" | ||
| End | ||
| End | ||
|
|
||
| Describe "Not using the --json flag" | ||
| It "returns 1 even if some files failed to parse" | ||
| When run snyk iac test ../fixtures/iac/kubernetes/ --experimental | ||
| The status should equal 1 | ||
| The output should not equal "" | ||
| The stderr should equal "" | ||
| End | ||
| End | ||
| End | ||
|
|
||
| Describe "Testing status code when no issues found" | ||
| Describe "Using the --json flag" | ||
| It "returns 0 even if some files failed to parse" | ||
| When run snyk iac test ../fixtures/iac/no_vulnerabilities/ --experimental --severity-threshold=high --json | ||
| The status should equal 0 | ||
| The output should not equal "" | ||
| The stderr should equal "" | ||
| End | ||
| End | ||
|
|
||
| Describe "Not using the --json flag" | ||
| It "returns 0 even if some files failed to parse" | ||
| When run snyk iac test ../fixtures/iac/no_vulnerabilities/ --experimental --severity-threshold=high | ||
| The status should equal 0 | ||
| The output should not equal "" | ||
| The stderr should equal "" | ||
| End | ||
| End | ||
| End |
There was a problem hiding this comment.
Since the --json flag doesn't have IaC-specific logic now, do we need these IaC specific tests? We have specific --json flag tests in other test suites like: https://github.com/snyk/snyk/blob/master/test/acceptance/cli-json-file-output.test.ts
There was a problem hiding this comment.
We added these tests because we previously missed this inconsistency between IaC running with the --json flag and without. IaC has its own code path and so I'm thinking keeping these might ensure we don't accidentally break this behaviour over time
What does this PR do?
This PR improves the consistency between running the experimental flow with the
--jsonflag or without. We want the two commands to fail and succeed the same way, regardless of the flag's presence. This way we don't incorrectly report failed commands in our analytics.Where should the reviewer start?
Start in
src/lib/snyk-test/iac-test-result.ts, where we add a new field to our IaC results,ok, which changes based on whether there are any vulnerabilities or not. This field fixes the original problem and removes the need for custom IaC checks insrc/cli/commands/test/index.ts(added for https://snyksec.atlassian.net/browse/CC-805).How should this be manually tested?
npm run build--jsonflag:node ./dist/cli iac test test/fixtures/iac/kubernetes/ --severity-threshold=high --experimentalnode ./dist/cli iac test test/fixtures/iac/no_vulnerabilities --severity-threshold=high --experimentalAny background context you want to provide?
We noticed via our Grafana dashboard that we have a lot of empty errors, more than we used to see before making the experimental flow the default in production a few days ago.
This was due to an edge case that we hadn't accounted for when testing the success/failure behaviour of the CLI and that some of our customers seem to run into a lot: scanning a directory with a mix of valid and invalid files, but the valid files having no vulnerabilities.
To be specific, this unexpected behaviour only happens when the
--jsonflag is provided. This is the behaviour we were seeing when scanning directories with multiple files:without the
--jsonflag:with the
--jsonflag:As we can see, there was a difference in behaviour between running with and without and
--jsonflag.To highlight this, we implemented smoke tests covering those 8 cases above, and also modified our existing smoke tests for the experimental flow to specifically check the status code, since failure can look different depending on whether we find vulnerabilities or we encounter some other problem.
Screenshots
These correspond to the manual tests mentioned further above:
contains one valid file with one vulnerability:
contains one valid file with no vulnerabilities:
