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

feat: disable container app scan with feature flag #4105
snyk · Dec 13, 2022 · 6a26463 · 6a26463
2 parents bc16000 + b12216d
commit 6a26463
Show file tree

Hide file tree

Showing 6 changed files with 109 additions and 20 deletions.
diff --git a/src/cli/commands/monitor/index.ts b/src/cli/commands/monitor/index.ts
@@ -48,6 +48,7 @@ import { isMultiProjectScan } from '../../../lib/is-multi-project-scan';
 import { getEcosystem, monitorEcosystem } from '../../../lib/ecosystems';
 import { getFormattedMonitorOutput } from '../../../lib/ecosystems/monitor';
 import { processCommandArgs } from '../process-command-args';
+import { hasFeatureFlag } from '../../../lib/feature-flags';
 
 const SEPARATOR = '\n-------------------------------------------------------\n';
 const debug = Debug('snyk');
@@ -96,14 +97,31 @@ export default async function monitor(...args0: MethodArgs): Promise<any> {
   // 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:
+    // 1) exclude-app-vulns set -> no app vulns
+    // 2) app-vulns set -> app-vulns
+    // 3) neither set -> containerAppVulnsEnabled
+    if (options['exclude-app-vulns']) {
       options['exclude-app-vulns'] = true;
-    }
-
-    // we can't print the warning message with JSON output as that would make
-    // the JSON output invalid.
-    if (!options['app-vulns'] && !options['json']) {
-      console.log(theme.color.status.warn(appVulnsReleaseWarningMsg));
+    } else if (options['app-vulns']) {
+      options['exclude-app-vulns'] = false;
+    } else {
+      options['exclude-app-vulns'] = !(await hasFeatureFlag(
+        'containerCliAppVulnsEnabled',
+        options,
+      ));
+
+      // we can't print the warning message with JSON output as that would make
+      // 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'] &&
+        !options['sarif']
+      ) {
+        console.log(theme.color.status.warn(appVulnsReleaseWarningMsg));
+      }
     }
   }
 

diff --git a/src/cli/commands/test/index.ts b/src/cli/commands/test/index.ts
@@ -98,14 +98,31 @@ 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:
+    // 1) exclude-app-vulns set -> no app vulns
+    // 2) app-vulns set -> app-vulns
+    // 3) neither set -> containerAppVulnsEnabled
+    if (options['exclude-app-vulns']) {
       options['exclude-app-vulns'] = true;
-    }
-
-    // we can't print the warning message with JSON output as that would make
-    // the JSON output invalid.
-    if (!options['app-vulns'] && !options['json']) {
-      console.log(theme.color.status.warn(appVulnsReleaseWarningMsg));
+    } else if (options['app-vulns']) {
+      options['exclude-app-vulns'] = false;
+    } else {
+      options['exclude-app-vulns'] = !(await hasFeatureFlag(
+        'containerCliAppVulnsEnabled',
+        options,
+      ));
+
+      // we can't print the warning message with JSON output as that would make
+      // 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'] &&
+        !options['sarif']
+      ) {
+        console.log(theme.color.status.warn(appVulnsReleaseWarningMsg));
+      }
     }
   }
 

diff --git a/test/acceptance/fake-server.ts b/test/acceptance/fake-server.ts
@@ -10,6 +10,7 @@ const featureFlagDefaults = (): Map<string, boolean> => {
   return new Map([
     ['cliFailFast', false],
     ['iacIntegratedExperience', false],
+    ['containerCliAppVulnsEnabled', false],
   ]);
 };
 

diff --git a/test/jest/acceptance/cli-args.spec.ts b/test/jest/acceptance/cli-args.spec.ts
@@ -12,7 +12,7 @@ const isWindows =
 jest.setTimeout(1000 * 60 * 5);
 
 describe('cli args', () => {
-  let server;
+  let server: ReturnType<typeof fakeServer>;
   let env: Record<string, string>;
 
   beforeAll((done) => {
@@ -301,7 +301,7 @@ describe('cli args', () => {
   });
 
   test('iac test with flags not allowed with --sarif', async () => {
-    const { code, stdout } = await runSnykCLI(`test iac --sarif --json`, {
+    const { code, stdout } = await runSnykCLI(`iac test --sarif --json`, {
       env,
     });
     expect(stdout).toMatch(
@@ -312,10 +312,13 @@ describe('cli args', () => {
     expect(code).toEqual(2);
   });
 
-  test('iac container with flags not allowed with --sarif', async () => {
-    const { code, stdout } = await runSnykCLI(`test container --sarif --json`, {
-      env,
-    });
+  test('container test with flags not allowed with --sarif', async () => {
+    const { code, stdout } = await runSnykCLI(
+      `container test  --sarif --json`,
+      {
+        env,
+      },
+    );
     expect(stdout).toMatch(
       new UnsupportedOptionCombinationError(['test', 'sarif', 'json'])
         .userMessage,

diff --git a/test/tap/cli-fail-on-docker.test.ts b/test/tap/cli-fail-on-docker.test.ts
@@ -81,6 +81,11 @@ test('test docker image with no fixable vulns and --fail-on=all', async (t) => {
     await cli.test('debian/sqlite3:latest', {
       failOn: 'all',
       docker: true,
+      // TODO: we should be able to remove that setting once once we remove the
+      // containerCliAppVulnsEnabled feature flag has been removed as well.
+      // Currently without setting this (or app-vulns), the code tries to reach
+      // the API to check the feature flag and throws an exception.
+      'exclude-app-vulns': true,
     });
     t.pass('should not throw exception');
   } catch (err) {
@@ -98,6 +103,11 @@ test('test docker image with fixable vulns and --fail-on=all', async (t) => {
     await cli.test('garethr/snyky:alpine', {
       failOn: 'all',
       docker: true,
+      // TODO: we should be able to remove that setting once once we remove the
+      // containerCliAppVulnsEnabled feature flag has been removed as well.
+      // Currently without setting this (or app-vulns), the code tries to reach
+      // the API to check the feature flag and throws an exception.
+      'exclude-app-vulns': true,
     });
     t.fail('expected test to throw exception');
   } catch (err) {

diff --git a/test/tap/cli-monitor.acceptance.test.ts b/test/tap/cli-monitor.acceptance.test.ts
@@ -1876,6 +1876,46 @@ if (!isWindows) {
     t.deepEqual(policyString, expected, 'sends correct policy');
   });
 
+  test('`monitor foo:latest --docker` with app vulns feature flag enabled', async (t) => {
+    chdirWorkspaces('npm-package-policy');
+    const spyPlugin = stubDockerPluginResponse(
+      {
+        scanResults: [
+          {
+            identity: {
+              type: 'rpm',
+            },
+            target: {
+              image: 'docker-image|foo',
+            },
+            facts: [{ type: 'depGraph', data: {} }],
+          },
+        ],
+        attributes: {},
+      },
+      t,
+    );
+
+    server.setFeatureFlag('containerCliAppVulnsEnabled', true);
+    await cli.monitor('foo:latest', {
+      docker: true,
+      org: 'explicit-org',
+    });
+    t.same(
+      spyPlugin.getCall(0).args,
+      [
+        {
+          docker: true,
+          'exclude-app-vulns': false,
+          org: 'explicit-org',
+          path: 'foo:latest',
+        },
+      ],
+      'calls docker plugin with expected arguments',
+    );
+    server.setFeatureFlag('containerCliAppVulnsEnabled', false);
+  });
+
   test('`monitor foo:latest --docker --platform=linux/arm64`', async (t) => {
     const platform = 'linux/arm64';
     const spyPlugin = stubDockerPluginResponse(