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.

Sign up for GitHub

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

Jump to bottom

feat: container app vulns by default #3433

Merged
merged 2 commits into from
Jan 25, 2023
Merged

feat: container app vulns by default #3433

merged 2 commits into from
Jan 25, 2023

Conversation

yaronschwimmer
Copy link

What does this PR do?

Enables container app scan by default. We are introducing a new flag,
exclude-app-vulns in case a user would like to disable app scan.

How should this be manually tested?

snyk container test <image_with_app> should return app scan results without additional parameters.
snyk container test --exclude-app-vulns <image_with_app> should return only operating system scan results.

Any background context you want to provide?

What are the relevant tickets?

https://snyksec.atlassian.net/browse/MYC-56

@yaronschwimmer yaronschwimmer requested review from a team as code owners July 4, 2022 09:26
@yaronschwimmer yaronschwimmer force-pushed the feat/container-app-vulns-by-default branch 2 times, most recently from 5f0b94b to c0cc31d Compare July 5, 2022 07:59
garethr
garethr previously requested changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@garethr garethr left a comment

Choose a reason for hiding this comment

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

If we could hold on releasing this until we've talked about the internal enablement needs please.

@yaronschwimmer yaronschwimmer mentioned this pull request Jul 10, 2022
Merged
1 task
yaronschwimmer pushed a commit that referenced this pull request Jul 10, 2022
fix: bump snyk docker plugin version golang fixes
8d55bcd 
We had issues with Go modules not being detected. Version 5.0.1 of
`snyk-docker-plugin` should fix those issues.
This commit includes a workaround since we have a blocked PR (#3433) for
making app vulns the default using the lates version of
`snyk-docker-plugin`. Until we are able to make scanning app vulns the
default, we need to make sure that we set `exclude-app-vulns` option
when a user doesn't set the `app-vulns` option.
@connectth connectth mentioned this pull request Aug 9, 2022
Open
@ChristinaDara ChristinaDara requested review from a team as code owners January 23, 2023 12:47
@ChristinaDara ChristinaDara requested review from a team January 23, 2023 12:47
@ChristinaDara ChristinaDara requested review from a team as code owners January 23, 2023 12:47
@ChristinaDara ChristinaDara requested review from agatakrajewska and removed request for nirshirion January 23, 2023 12:47
@tommyknows tommyknows force-pushed the feat/container-app-vulns-by-default branch from 2ac384c to 12d2588 Compare January 23, 2023 12:59
tommyknows
tommyknows reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@tommyknows tommyknows left a comment

Choose a reason for hiding this comment

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

Small nit regarding the docs suggestion.

You're currently also missing the actual changes regarding the flag; e.g. the parts in src/cli/commands/{test,monitor}/index.ts :)

src/lib/formatters/test/format-test-results.ts Outdated Show resolved Hide resolved
@ChristinaDara ChristinaDara force-pushed the feat/container-app-vulns-by-default branch from c928df8 to 3e45154 Compare January 23, 2023 18:25
@neonnoon neonnoon removed the request for review from a team January 24, 2023 08:04
@ChristinaDara ChristinaDara force-pushed the feat/container-app-vulns-by-default branch 4 times, most recently from 6994477 to 3af25f6 Compare January 24, 2023 12:59
@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2023 •
edited
Loading

Warnings
⚠️

Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones.
A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax.
Files found:

  • src/cli/commands/monitor/index.ts
  • src/cli/commands/test/index.ts
  • test/tap/cli-monitor.acceptance.test.ts
  • test/tap/cli-test/cli-test.docker.spec.ts

Generated by 🚫 dangerJS against 6723568

@ChristinaDara ChristinaDara force-pushed the feat/container-app-vulns-by-default branch 11 times, most recently from b77bc8f to 6fd4878 Compare January 25, 2023 15:12
@tommyknows
test: fix tests for changed feature flag default
85d0962 
We've changed the default value of the "containerCliAppVulnsEnabled"
feature flag to be `true`, which means that the acceptance tests that
are depending on this being `false` are failing.

This commit fixes the tests and also tests for the new default behaviour
instead.
@tommyknows
refactor: improve docker tips
6723568 
This commit refactors the docker suggestion text that will be printed if
the user hasn't opted out of suggestions.
@tommyknows tommyknows force-pushed the feat/container-app-vulns-by-default branch from 6fd4878 to 6723568 Compare January 25, 2023 16:17
PeterSchafer
PeterSchafer approved these changes Jan 25, 2023
View reviewed changes
Copy link
Collaborator

@PeterSchafer PeterSchafer left a comment

Choose a reason for hiding this comment

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

LGTM

@PeterSchafer PeterSchafer dismissed garethr’s stale review January 25, 2023 16:48

From Gareth: "Don’t block on me for PR reviews."

tommyknows
tommyknows approved these changes Jan 25, 2023
View reviewed changes
Copy link
Contributor

@tommyknows tommyknows left a comment

Choose a reason for hiding this comment

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

LGTM!

mcombuechen
mcombuechen approved these changes Jan 25, 2023
View reviewed changes
Copy link
Contributor

@mcombuechen mcombuechen left a comment

Choose a reason for hiding this comment

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

LGTM

@tommyknows tommyknows merged commit 733d36f into master Jan 25, 2023
@tommyknows tommyknows deleted the feat/container-app-vulns-by-default branch January 25, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcombuechen mcombuechen mcombuechen approved these changes

@robcresswell robcresswell robcresswell approved these changes

@tommyknows tommyknows tommyknows approved these changes

@Avishagp Avishagp Avishagp approved these changes

@PeterSchafer PeterSchafer PeterSchafer approved these changes

@JamesPatrickGill JamesPatrickGill Awaiting requested review from JamesPatrickGill JamesPatrickGill was automatically assigned from snyk/snyk-open-source

@agatakrajewska agatakrajewska Awaiting requested review from agatakrajewska agatakrajewska was automatically assigned from snyk/group-infrastructure-as-code

@garethr garethr Awaiting requested review from garethr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@yaronschwimmer @garethr @mcombuechen @robcresswell @tommyknows @Avishagp @PeterSchafer