feat: disable container app scan with feature flag #4105

tommyknows · 2022-10-05T09:18:50Z

What does this PR do?

This commit adds a check for the containerCliAppVulnsEnabled feature flag. If neither --exclude-app-vulns nor --app-vulns are set, the feature flag will be used to determine whether app vulnerabilities will be scanned or not.

Any background context you want to provide?

Part of the work to enable app vulnerability scanning in containers by default. Build upon @yaronschwimmer's change, but that needed some adjustments to ensure the ordering and precedence of flags.

What are the relevant tickets?

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

github-actions · 2022-10-05T09:20:13Z

	Warnings
⚠️	Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style `import`s and `export`s 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`

Generated by 🚫 dangerJS against a2cbec3

noaleibo1 · 2022-10-06T07:12:22Z

src/cli/commands/monitor/index.ts

+      // the JSON output invalid.
+      // We also only want to print the message if the user did not overwrite
+      // the default with one of the flags.
+      if (options['exclude-app-vulns'] && !options['json']) {


It might be my basic TS knowledge, but don't we want here options['exclude-app-vulns'] == null && !options['json']?

I don't think so, though I agree that having a negated boolean is confusing (exclude-app-vulns vs. app-vulns).

Either way, we want to print the message if exclude-app-vulns is true. Also, this condition is within the larger if-else chain of "has the user overwritten this through the CLI". We only get to this expression if the user has not done so, and relies on the setting of the feature flag on the API.

The reasoning behind this is that if the user explicitly sets --app-vulns or --exclude-app-vulns, he already knows about this & does not need a warning message :)

noaleibo1 · 2022-10-06T07:15:06Z

src/cli/commands/test/index.ts

@@ -98,14 +98,27 @@ export default async function test(
  // TODO remove 'app-vulns' options and warning message once
  // https://github.com/snyk/cli/pull/3433 is merged
  if (options.docker) {
-    if (!options['app-vulns'] || options['exclude-app-vulns']) {
+    // order is important here, we want:


I guess there isn't a nice way to reuse the code we have for monitor..?

I guess there is, but that would probably be for a separate ticket. It would be a good idea though so that we also don't need reviews from other teams when changing these parts....

(I'd also like to split out the API feature flag names into a separate package with constants, so that we don't need to copy that string...)

👍 for both

noaleibo1 · 2022-10-06T07:16:02Z

test/acceptance/fake-server.ts

@@ -10,6 +10,7 @@ const featureFlagDefaults = (): Map<string, boolean> => {
  return new Map([
    ['cliFailFast', false],
    ['iacIntegratedExperience', false],
+    ['containerCliAppVulnsEnabled', false],


Is this the only testing we have? Makes me feel a little uncomfortable 😅

Not directly - I actually had a couple of tests fail before I set that, as it returned true as the default value? By setting this here we make sure the fake API returns false, which makes our tests pass :)

I can add some more tests to check if everything behaves correctly if the API returns true though.

Added a dedicated test, when the feature flag is set to true, and also had to fix some other tests :)

noaleibo1 · 2022-10-13T08:25:33Z

src/cli/commands/test/index.ts

@@ -98,14 +98,27 @@ export default async function test(
  // TODO remove 'app-vulns' options and warning message once
  // https://github.com/snyk/cli/pull/3433 is merged
  if (options.docker) {
-    if (!options['app-vulns'] || options['exclude-app-vulns']) {
+    // order is important here, we want:


👍 for both

noaleibo1 · 2022-10-13T08:26:15Z

test/acceptance/fake-server.ts

@@ -10,6 +10,7 @@ const featureFlagDefaults = (): Map<string, boolean> => {
  return new Map([
    ['cliFailFast', false],
    ['iacIntegratedExperience', false],
+    ['containerCliAppVulnsEnabled', false],


This commit adds a check for the `containerCliAppVulnsEnabled` feature flag. If neither `--exclude-app-vulns` nor `--app-vulns` are set, the feature flag will be used to determine whether app vulnerabilities will be scanned or not.

tommyknows · 2022-10-18T10:45:29Z

Added a second commit to fix the test failures.

See the commit message for a proper description. What I want to add is why this just came up with this PR, and has not been an issue before.

global-agent has two different initialisation routines, either by import 'global-agent/bootstrap'; or by import {bootstrap} from 'global-agent'; bootstrap().

The second way means that bootstrap needs to be explicitly called, and before this PR it was only called after we were done talking to the docker socket.

By adding the hasFeatureFlag call, which in turn calls bootstrap, the agent has now been replaced before we talked to the docker socket, thus triggering the issue.

For CLIv2, all traffic is proxied through a local server that the CLIv2 spawns. This is achieved by updating the `http.globalAgent` and `http.request` and `http.get` functions through the `global-agent` library. However, this causes issues with HTTP traffic that is supposed to go to local sockets. For example, `snyk container` talks to the `/var/run/docker.sock` socket for doing some image operations (if the socket exists). When the http agent and functions are replaced, this causes traffic to the socket to be routed to / through the proxy, and because at that point it is "proper" HTTP traffic, the destination is not the socket anymore but `localhost:80` (the fallback). The proxy has no way of handling this correctly, as all the information of the socket is lost. To fix this, there would have been two possibilities: - Make it possible to use custom agents. Currently, even if a user creates a custom agent and passes it to `http.request` or `http.get`, this custom agent will not be used. This is because the `global-agent` library *enforces* the usage of the proxy-agent. There is a flag to change this behaviour (`forceGLobalAgent`), but we do not want to use this as traffic should *always* go through the proxy. - Fix the `global-agent` library to not use the proxy-agent if traffic is supposed to go to sockets. A [PR](gajus/global-agent#60) has been raised on the library, but the repository seems to be unmaintaned. Because we need this fix to continue with the app vulnerability enablement, this commit adds a `postinstall` hook that patches the library in the `node-modules` folder through a `git apply` command. With the applied patch, traffic to the Docker socket (and all other sockets) will not be rerouted through the proxy anymore.

danlucian · 2022-10-27T14:21:00Z

scripts/apply-patches.sh

@@ -0,0 +1,11 @@
+#!/usr/bin/env bash


Quick note: How can we make sure the logic in this script file will correctly work (e.g. do we have any tests)?

We don't have any tests directly, but...
every npm install (or npm ci) will call this script.
I've also needed to add the patch because some tests where failing without it.

-> if applying the patch wouldn't work, the tests won't pass :-)

So we have indirect tests, I guess 😄

I hope that's good enough, let me know if not.

Okey dokey, sounds good to me.

Hi @tommyknows,

Just a heads up about this issue that started happening today:
https://github.com/snyk/cli/issues/4183

Really sorry for this, @tony-snoop! And thank you a lot for reporting it!
We are rollbacking the changes right now: #4184

It's all good - thanks for jumping onto the fix so quickly @danlucian and for rolling back the changes.
Really REALLY appreciated :)

Quick question - do you know when the new version will become publicly available on https://www.npmjs.com/package/snyk ?

It's currently on version 1.1045.0. I'm refreshing the page regularly waiting for 1.1046.0 to appear :)

fix: rollback #4105

feat: disable container app scan with feature flag #4105

tommyknows requested a review from a team as a code owner October 5, 2022 09:18

tommyknows requested review from klesniewski, JamesPatrickGill and mika-bar October 5, 2022 09:18

tommyknows force-pushed the feat/app-vulns-feature-flag branch 2 times, most recently from 8cde5d7 to 76fa28b Compare October 5, 2022 14:51

tommyknows requested a review from a team as a code owner October 5, 2022 14:51

noaleibo1 reviewed Oct 6, 2022

View reviewed changes

tommyknows force-pushed the feat/app-vulns-feature-flag branch 9 times, most recently from 0470984 to fdc274b Compare October 10, 2022 07:26

noaleibo1 approved these changes Oct 13, 2022

View reviewed changes

feat: disable container app scan with feature flag

39fcaf2

This commit adds a check for the `containerCliAppVulnsEnabled` feature flag. If neither `--exclude-app-vulns` nor `--app-vulns` are set, the feature flag will be used to determine whether app vulnerabilities will be scanned or not.

tommyknows force-pushed the feat/app-vulns-feature-flag branch from fdc274b to e6d1cda Compare October 18, 2022 10:41

tommyknows force-pushed the feat/app-vulns-feature-flag branch from e6d1cda to e697727 Compare October 18, 2022 11:35

tommyknows force-pushed the feat/app-vulns-feature-flag branch from e697727 to a2cbec3 Compare October 19, 2022 07:18

PeterSchafer approved these changes Oct 20, 2022

View reviewed changes

danlucian reviewed Oct 27, 2022

View reviewed changes

danlucian approved these changes Oct 27, 2022

View reviewed changes

tommyknows merged commit 08c30f1 into master Oct 31, 2022

tommyknows deleted the feat/app-vulns-feature-flag branch October 31, 2022 08:07

danlucian added a commit that referenced this pull request Oct 31, 2022

fix: rollback #4105

6fec157

danlucian mentioned this pull request Oct 31, 2022

fix: rollback #4105 #4184

Merged

Avishagp added a commit that referenced this pull request Oct 31, 2022

Merge pull request #4184 from snyk/fix/rollback-#4105

266ca30

fix: rollback #4105

tommyknows mentioned this pull request Nov 30, 2022

feat: disable container app scan with feature flag #4105 #4224

Merged

1 task

tommyknows added a commit that referenced this pull request Dec 13, 2022

Merge pull request #4224 from snyk/feat/app-vulns-featureflag

6a26463

feat: disable container app scan with feature flag #4105

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: disable container app scan with feature flag #4105

feat: disable container app scan with feature flag #4105

tommyknows commented Oct 5, 2022

github-actions bot commented Oct 5, 2022 •

edited

Loading

noaleibo1 Oct 6, 2022

tommyknows Oct 6, 2022

noaleibo1 Oct 6, 2022

tommyknows Oct 6, 2022

tommyknows Oct 6, 2022

noaleibo1 Oct 13, 2022

noaleibo1 Oct 6, 2022

tommyknows Oct 6, 2022

tommyknows Oct 6, 2022

noaleibo1 Oct 13, 2022

noaleibo1 Oct 13, 2022

noaleibo1 Oct 13, 2022

tommyknows commented Oct 18, 2022

danlucian Oct 27, 2022

tommyknows Oct 27, 2022

danlucian Oct 31, 2022

tony-snoop Oct 31, 2022 •

edited

Loading

danlucian Oct 31, 2022

tony-snoop Oct 31, 2022 •

edited

Loading

feat: disable container app scan with feature flag #4105

feat: disable container app scan with feature flag #4105

Conversation

tommyknows commented Oct 5, 2022

What does this PR do?

Any background context you want to provide?

What are the relevant tickets?

github-actions bot commented Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommyknows commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-snoop Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-snoop Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2022 •

edited

Loading

tony-snoop Oct 31, 2022 •

edited

Loading

tony-snoop Oct 31, 2022 •

edited

Loading