Re-enable passing the codescanning config file to the CLI #1105

aeisenberg · 2022-06-19T23:45:55Z

This PR un-reverts #1018

Additionally, it adds the fix for adding queries and packs from the actions input into the codescanning config file before it is sent to the CLI.

When the + is used, the actions input value is combined with the
config value and when it is not used, the input value overrides the
config value.

This commit also adds a bunch of integration tests for this feature.
In order to avoid adding too many new jobs, all of the tests are
run sequentially in a single job (matrixed across relevant operating
systems and OSes).

Recommended to look at the commits individually. The first commit is the un-revert. The second commit is the new work.

This change is currently hidden behind an environment variable. I will probably convert this into a feature flag before getting external users to try this.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

.github/check-codescanning-config/action.yml

@@ -0,0 +1,59 @@
+name: Check Code-Scanning Config


This reverts commit 43d0664.

This commit adds the packs and queries from the actions input to the config file used by the CodeQL CLI. When the `+` is used, the actions input value is combined with the config value and when it is not used, the input value overrides the config value. This commit also adds a bunch of integration tests for this feature. In order to avoid adding too many new jobs, all of the tests are run sequentially in a single job (matrixed across relevant operating systems and OSes).

edoardopirovano

Haven't finished reviewing this yet, but partial comments below - one is significant enough that I'll want to do a full re-review afterwards anyways hence the partial review.

edoardopirovano · 2022-06-29T09:51:44Z

src/codeql.ts

@@ -225,6 +226,7 @@ const CODEQL_VERSION_GROUP_RULES = "2.5.5";
 const CODEQL_VERSION_SARIF_GROUP = "2.5.3";
 export const CODEQL_VERSION_COUNTS_LINES = "2.6.2";
 const CODEQL_VERSION_CUSTOM_QUERY_HELP = "2.7.1";
+export const CODEQL_VERSION_CONFIG_FILES = "2.8.2"; // Versions before 2.8.2 weren't tolerant to unknown properties


This probably wants bumping to 2.10.1, so you only get CLI versions with https://github.com/github/semmle-code/pull/42877 in them.

edoardopirovano · 2022-06-29T10:03:58Z

src/codeql.ts

@@ -933,7 +941,9 @@ async function getCodeQLForCmd(
      if (extraSearchPath !== undefined) {
        codeqlArgs.push("--additional-packs", extraSearchPath);
      }
-      codeqlArgs.push(querySuitePath);
+      if (!(await util.useCodeScanningConfigInCli(this))) {
+        codeqlArgs.push(querySuitePath);


I think this is quite broken (and probably already was in my version). We call databaseRunQueries potentially multiple times since runQueryGroup gets called once for each different group of queries. So, if we just do this we'll end up running all the queries every time. I think we need a new code path from the top-level runQueries that just calls databaseRunQueries once without any arguments if we're using the CLI-side config file parsing, and skips all the calls to runQueryGroup.

So if I understand correctly, the action will run the custom, the builtin, and the packaging queries in separate calls. The logic of the CLI invocation avoids pushing the query suite path onto the args. This has the effect of passing no query specs to the CLI invocation, which means that the config-queries.qls suite inside the database temp directory is used instead.

The config-queries.qls suite contains all the queries to run for a given language. So, when running with the config enabled, we only want to run this suite a single time during the analysis.

Assuming I am right here, I can make the fix.

Something else to think about. We are sending status reports for various timings of the package groups. Specifically, we are sending time to evaluate builtin and custom packs also timing for interpretation of these two groups.

With this change, we are combining the groups, and we can't just add new fields to our status reports without a backend change (I think!), so I can stuff the timing into the builtin section. However, the value when running using the config will not be comparable to the value when running in the old way.

So, we will need to expand the status report with the new fields.

For now, I'll stuff it into builtin, but this isn't correct. I will do the extra work later.

Correction: it's only query evaluation that needs an extra entry in the status report, not interpretation.

Assuming I am right here, I can make the fix.

Right, I think your understanding is correct. Regarding what to do with the status reports, I agree that we will no longer be able to distinguish the custom queries from the built in ones, so we'll need a new field that represents the time spent on the whole set of queries being run.

I'll note that while it makes our telemetry a bit less good, there's big potential performance gains from doing this: giving everything to the evaluator at once means we won't have to rely on the disk cache between one invocation and the next so should significantly reduce our IO usage (and, IO usage is probably close to 100% of the time we spend on custom queries, since we'll already have evaluated all the standard library for our built-in queries, and custom queries are unlikely to have particularly complex logic on top of that). In a multi-threaded setting, it also means we can work on the custom queries at the same time as the built-in ones. So, we're trading some telemetry for better performance, which at least from our users point of view is a win.

edoardopirovano · 2022-06-29T10:27:25Z

.github/workflows/codescanning-config-cli.yml

+        tools: ${{ steps.prepare-test.outputs.tools-url }}
+
+    - name: Packs from input
+      if: success() || failure()


I think this could just be always()?

Always includes canceled(), which we don't need here.

I see, I wasn't aware of that distinction. Thanks for clarifying!

edoardopirovano · 2022-06-29T12:01:01Z

CHANGELOG.md

@@ -42,6 +42,7 @@ No user facing changes.
 ## 2.1.7 - 05 Apr 2022

 - A bug where additional queries specified in the workflow file would sometimes not be respected has been fixed. [#1018](https://github.com/github/codeql-action/pull/1018)
+No user facing changes.


This looks like a bad merge.

When the codescanning config is being used by the CLI, there is a single query suite that is generated that contains all queries to be run by the analysis. This is different from the traditional way, where there are potentially three query suites: builtin, custom, and packs. We need to ensure that when the codescanning config is being used, only a single call to run queries is used, and this call uses the single generated query suite. Also, this commit changes the cutoff version for codescanning config to 2.10.1. Earlier versions work, but there were some bugs that are only fixed in 2.10.1 and later.

aeisenberg · 2022-06-29T23:10:00Z

Hmmm...the job is failing now because latest-nightly is still 2.10.0 and the feature is not being used. I think I need to hold off on merging until 2.10.1 is available as the latest nightly.

aeisenberg · 2022-07-29T21:35:05Z

Code-Scanning config CLI tests / Code Scanning Configuration tests (ubuntu-latest, cached) failing because "cached" is still 2.10.0. Need to wait for 2.10.1.

This makes some syntax in tests somewhat simpler.

edoardopirovano · 2022-08-11T15:42:07Z

src/codeql.ts

+ * @param config The configuration to use.
+ * @returns the path to the generated user configuration file.
+ */
+async function generateCodescanningConfig(codeql: CodeQL, config: Config) {


Should this have a return type? I guess Promise<string | undefined>?

Typescript determines the return type implicitly, but I can add it to make it easier to read.

edoardopirovano · 2022-08-11T15:55:44Z

src/codeql.ts

+  }
+  const configLocation = path.resolve(config.tempDir, "user-config.yaml");
+  // make a copy so we can modify it
+  const augmentedConfig = JSON.parse(JSON.stringify(config.originalUserInput));


Could we implement a clone method, or something similar to copy this? It seems clunky to turn it into a string and parse it again just to make a copy.

Sure. Generic clone functions in js are really tricky to implement since they need to keep track of the prototype chain of values. We don't need that here. Since we are only copying raw objects, this is the simplest thing to do. I can extract it into a separate function.

edoardopirovano · 2022-08-11T15:59:31Z

src/config-utils.test.ts

@@ -1621,6 +1623,7 @@ function parseInputAndConfigMacro(
    configUtils.parsePacks(
      packsFromConfig,
      packsFromInput,
+      !!packsFromInput?.trim().startsWith("+"),


Is the idea of this double negation that it handles the undefined case? Could we add a comment to explain that to future readers of the code?

Yes. That's what's happening. It coerces the result into a boolean. It's a standard js idiom, but I understand that it is weird coming from statically typed languages.

edoardopirovano · 2022-08-11T16:18:59Z

src/util.ts

+  return (
+    (process.env[EnvVar.CODEQL_PASS_CONFIG_TO_CLI] === "true" &&
+      (await codeQlVersionAbove(codeql, CODEQL_VERSION_CONFIG_FILES))) ||
+    false


Is the || false at the end doing anything?

Hmmm...I don't think its necessary.

…ig-files

…config-files

edoardopirovano

This is still a pretty scary change, but it does certainly look much better with all the extra tests. Thanks for adding those in! And I guess it's behind an env variable, so we won't break anything - let's merge it :)

aeisenberg · 2022-08-12T18:16:07Z

Thanks for the review. We are one step closer to removing this technical debt.

aeisenberg requested a review from a team as a code owner June 19, 2022 23:45

aeisenberg removed the request for review from a team June 19, 2022 23:46

aeisenberg marked this pull request as draft June 19, 2022 23:46

aeisenberg force-pushed the aeisenberg/fix-config-files branch 16 times, most recently from fe71459 to 2fd87c6 Compare June 24, 2022 18:10

github-advanced-security bot found potential problems Jun 24, 2022

View reviewed changes

aeisenberg force-pushed the aeisenberg/fix-config-files branch 10 times, most recently from f1cd9f6 to 1c847ad Compare June 24, 2022 23:07

edoardopirovano self-assigned this Jun 28, 2022

Revert "Revert usage of --codescanning-config flag"

237260b

This reverts commit 43d0664.

aeisenberg force-pushed the aeisenberg/fix-config-files branch from dbcf6d0 to 62097bc Compare June 28, 2022 20:05

aeisenberg force-pushed the aeisenberg/fix-config-files branch from 62097bc to 6fabde2 Compare June 28, 2022 21:08

edoardopirovano reviewed Jun 29, 2022

View reviewed changes

Merge branch 'main' into aeisenberg/fix-config-files

01d16b1

aeisenberg force-pushed the aeisenberg/fix-config-files branch from 6efc13c to 01d16b1 Compare July 13, 2022 21:06

aeisenberg added 2 commits July 25, 2022 11:20

Merge branch 'main' into aeisenberg/fix-config-files

4e46a69

Merge branch 'main' into aeisenberg/fix-config-files

907f1de

aeisenberg added 2 commits August 10, 2022 15:39

Merge branch 'main' into aeisenberg/fix-config-files

0403fb7

Add the defaultAugmentationProperties constant

2314063

This makes some syntax in tests somewhat simpler.

erik-krogh mentioned this pull request Aug 11, 2022

QL: update codeql-action in QL-for-QL github/codeql#10012

Merged

edoardopirovano reviewed Aug 11, 2022

View reviewed changes

aeisenberg added 3 commits August 11, 2022 09:56

Fix failing test and address PR comments

a09a029

Merge remote-tracking branch 'upstream/main' into aeisenberg/fix-conf…

d74f663

…ig-files

Merge branch 'aeisenberg/unrevert-query-filters' into aeisenberg/fix-…

fa2bc21

…config-files

aeisenberg force-pushed the aeisenberg/fix-config-files branch from 0fcf0fc to fa2bc21 Compare August 11, 2022 21:57

edoardopirovano approved these changes Aug 12, 2022

View reviewed changes

aeisenberg merged commit 680d08e into main Aug 12, 2022

aeisenberg deleted the aeisenberg/fix-config-files branch August 12, 2022 18:15

This was referenced Aug 17, 2022

Merge main into releases/v2 #1189

Closed

Merge main into releases/v2 #1192

Merged

Merge releases/v2 into releases/v1 #1195

Merged

aliscco mentioned this pull request Jun 22, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/codeql-action#613

Open

aliscco mentioned this pull request Nov 30, 2023

[Snyk] Fix for 4 vulnerabilities aliscco/codeql-action#1004

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable passing the codescanning config file to the CLI #1105

Re-enable passing the codescanning config file to the CLI #1105

aeisenberg commented Jun 19, 2022 •

edited

Loading

edoardopirovano left a comment

edoardopirovano Jun 29, 2022

edoardopirovano Jun 29, 2022

aeisenberg Jun 29, 2022

aeisenberg Jun 29, 2022

aeisenberg Jun 29, 2022

aeisenberg Jun 29, 2022

edoardopirovano Jun 30, 2022

edoardopirovano Jun 29, 2022

aeisenberg Jun 29, 2022

edoardopirovano Jun 30, 2022

edoardopirovano Jun 29, 2022

aeisenberg commented Jun 29, 2022

aeisenberg commented Jul 29, 2022

edoardopirovano Aug 11, 2022

aeisenberg Aug 11, 2022

edoardopirovano Aug 11, 2022

aeisenberg Aug 11, 2022

edoardopirovano Aug 11, 2022

aeisenberg Aug 11, 2022

edoardopirovano Aug 11, 2022

aeisenberg Aug 11, 2022

edoardopirovano left a comment

aeisenberg commented Aug 12, 2022

Re-enable passing the codescanning config file to the CLI #1105

Re-enable passing the codescanning config file to the CLI #1105

Conversation

aeisenberg commented Jun 19, 2022 • edited Loading

Merge / deployment checklist

edoardopirovano left a 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

Choose a reason for hiding this comment

aeisenberg commented Jun 29, 2022

aeisenberg commented Jul 29, 2022

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

edoardopirovano left a comment

Choose a reason for hiding this comment

aeisenberg commented Aug 12, 2022

aeisenberg commented Jun 19, 2022 •

edited

Loading