robojerk/codeql-action

Refactor handling of feature flags

Browse source 
This commit centralizes how feature flags are handled. All feature flags
must now add an entry in the `featureFlagConfig` dictionary. This
dictionary associates the flag with an environment variable name and
optionally a minimum version for CodeQL.

The new logic is:

- if the environment variable is set to false: disabled
- if the minimum version requirement specified and met: disabled
- if the environment variable is set to true: enable
- Otherwise check feature flag enablement from the server
This commit is contained in:
Andrew Eisenberg 2022-10-05 15:54:07 -07:00
parent 24c8de16fa
commit e5c3375225
27 changed files with 400 additions and 368 deletions
Download patch file Download diff file Expand all files Collapse all files

68
src/codeql.test.ts
View file

@ -505,47 +505,41 @@ const injectedConfigMacro = test.macro({
configOverride: Partial<Config>,
expectedConfig: any
) => {
const origCODEQL_PASS_CONFIG_TO_CLI = process.env.CODEQL_PASS_CONFIG_TO_CLI;
process.env["CODEQL_PASS_CONFIG_TO_CLI"] = "true";
try {
await util.withTmpDir(async (tempDir) => {
const runnerConstructorStub = stubToolRunnerConstructor();
const codeqlObject = await codeql.getCodeQLForTesting();
sinon
.stub(codeqlObject, "getVersion")
.resolves(codeql.CODEQL_VERSION_CONFIG_FILES);
await util.withTmpDir(async (tempDir) => {
const runnerConstructorStub = stubToolRunnerConstructor();
const codeqlObject = await codeql.getCodeQLForTesting();
sinon
.stub(codeqlObject, "getVersion")
.resolves(codeql.CODEQL_VERSION_CONFIG_FILES);
const thisStubConfig: Config = {
...stubConfig,
...configOverride,
tempDir,
augmentationProperties,
};
const thisStubConfig: Config = {
...stubConfig,
...configOverride,
tempDir,
augmentationProperties,
};
await codeqlObject.databaseInitCluster(
thisStubConfig,
"",
undefined,
undefined,
createFeatureFlags([]),
getRunnerLogger(true)
);
await codeqlObject.databaseInitCluster(
thisStubConfig,
"",
undefined,
undefined,
createFeatureFlags([FeatureFlag.CliConfigFileEnabled]),
getRunnerLogger(true)
);
const args = runnerConstructorStub.firstCall.args[1];
// should have used an config file
const configArg = args.find((arg: string) =>
arg.startsWith("--codescanning-config=")
);
t.truthy(configArg, "Should have injected a codescanning config");
const configFile = configArg.split("=")[1];
const augmentedConfig = yaml.load(fs.readFileSync(configFile, "utf8"));
t.deepEqual(augmentedConfig, expectedConfig);
const args = runnerConstructorStub.firstCall.args[1];
// should have used an config file
const configArg = args.find((arg: string) =>
arg.startsWith("--codescanning-config=")
);
t.truthy(configArg, "Should have injected a codescanning config");
const configFile = configArg.split("=")[1];
const augmentedConfig = yaml.load(fs.readFileSync(configFile, "utf8"));
t.deepEqual(augmentedConfig, expectedConfig);
await del(configFile, { force: true });
});
} finally {
process.env["CODEQL_PASS_CONFIG_TO_CLI"] = origCODEQL_PASS_CONFIG_TO_CLI;
}
await del(configFile, { force: true });
});
},
title: (providedTitle = "") =>

1
src/codeql.ts
View file

@ -247,7 +247,6 @@ 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_ML_POWERED_QUERIES = "2.7.5";
const CODEQL_VERSION_LUA_TRACER_CONFIG = "2.10.0";
export const CODEQL_VERSION_CONFIG_FILES = "2.10.1";
const CODEQL_VERSION_LUA_TRACING_GO_WINDOWS_FIXED = "2.10.4";

4
src/config-utils.ts
View file

@ -10,7 +10,6 @@ import * as api from "./api-client";
import {
CodeQL,
CODEQL_VERSION_GHES_PACK_DOWNLOAD,
CODEQL_VERSION_ML_POWERED_QUERIES,
CODEQL_VERSION_ML_POWERED_QUERIES_WINDOWS,
ResolveQueriesOutput,
} from "./codeql";
@ -412,8 +411,7 @@ async function addBuiltinSuiteQueries(
languages.includes("javascript") &&
(found === "security-extended" || found === "security-and-quality") &&
!packs.javascript?.some(isMlPoweredJsQueriesPack) &&
(await featureFlags.getValue(FeatureFlag.MlPoweredQueriesEnabled)) &&
(await codeQlVersionAbove(codeQL, CODEQL_VERSION_ML_POWERED_QUERIES))
(await featureFlags.getValue(FeatureFlag.MlPoweredQueriesEnabled, codeQL))
) {
if (!packs.javascript) {
packs.javascript = [];

180
src/feature-flags.test.ts
View file

@ -1,12 +1,18 @@
import test from "ava";
import { GitHubApiDetails } from "./api-client";
import { FeatureFlag, GitHubFeatureFlags } from "./feature-flags";
import {
FeatureFlag,
featureFlagConfig,
FeatureFlags,
GitHubFeatureFlags,
} from "./feature-flags";
import { getRunnerLogger } from "./logging";
import { parseRepositoryNwo } from "./repository";
import {
getRecordingLogger,
LoggedMessage,
mockCodeQLVersion,
mockFeatureFlagApiEndpoint,
setupActionsVars,
setupTests,
@ -42,14 +48,11 @@ const ALL_FEATURE_FLAGS_DISABLED_VARIANTS: Array<{
for (const variant of ALL_FEATURE_FLAGS_DISABLED_VARIANTS) {
test(`All feature flags are disabled if running against ${variant.description}`, async (t) => {
await withTmpDir(async (tmpDir) => {
setupActionsVars(tmpDir, tmpDir);
const loggedMessages = [];
const featureFlags = new GitHubFeatureFlags(
variant.gitHubVersion,
testApiDetails,
testRepositoryNwo,
getRecordingLogger(loggedMessages)
const featureFlags = setUpTmpDir(
tmpDir,
getRecordingLogger(loggedMessages),
variant.gitHubVersion
);
for (const flag of Object.values(FeatureFlag)) {
@ -70,13 +73,9 @@ for (const variant of ALL_FEATURE_FLAGS_DISABLED_VARIANTS) {
test("API response missing", async (t) => {
await withTmpDir(async (tmpDir) => {
setupActionsVars(tmpDir, tmpDir);
const loggedMessages = [];
const featureFlags = new GitHubFeatureFlags(
{ type: GitHubVariant.DOTCOM },
testApiDetails,
testRepositoryNwo,
const featureFlags = setUpTmpDir(
tmpDir,
getRecordingLogger(loggedMessages)
);
@ -86,7 +85,7 @@ test("API response missing", async (t) => {
t.assert((await featureFlags.getValue(flag)) === false);
}
for (const featureFlag of ["ml_powered_queries_enabled"]) {
for (const featureFlag of Object.keys(featureFlagConfig)) {
t.assert(
loggedMessages.find(
(v: LoggedMessage) =>
@ -101,13 +100,9 @@ test("API response missing", async (t) => {
test("Feature flags are disabled if they're not returned in API response", async (t) => {
await withTmpDir(async (tmpDir) => {
setupActionsVars(tmpDir, tmpDir);
const loggedMessages = [];
const featureFlags = new GitHubFeatureFlags(
{ type: GitHubVariant.DOTCOM },
testApiDetails,
testRepositoryNwo,
const featureFlags = setUpTmpDir(
tmpDir,
getRecordingLogger(loggedMessages)
);
@ -117,7 +112,7 @@ test("Feature flags are disabled if they're not returned in API response", async
t.assert((await featureFlags.getValue(flag)) === false);
}
for (const featureFlag of ["ml_powered_queries_enabled"]) {
for (const featureFlag of Object.keys(featureFlagConfig)) {
t.assert(
loggedMessages.find(
(v: LoggedMessage) =>
@ -132,14 +127,7 @@ test("Feature flags are disabled if they're not returned in API response", async
test("Feature flags exception is propagated if the API request errors", async (t) => {
await withTmpDir(async (tmpDir) => {
setupActionsVars(tmpDir, tmpDir);
const featureFlags = new GitHubFeatureFlags(
{ type: GitHubVariant.DOTCOM },
testApiDetails,
testRepositoryNwo,
getRunnerLogger(true)
);
const featureFlags = setUpTmpDir(tmpDir);
mockFeatureFlagApiEndpoint(500, {});
@ -153,34 +141,128 @@ test("Feature flags exception is propagated if the API request errors", async (t
});
});
const FEATURE_FLAGS = ["ml_powered_queries_enabled"];
for (const featureFlag of FEATURE_FLAGS) {
for (const featureFlag of Object.keys(featureFlagConfig)) {
test(`Feature flag '${featureFlag}' is enabled if enabled in the API response`, async (t) => {
await withTmpDir(async (tmpDir) => {
setupActionsVars(tmpDir, tmpDir);
const featureFlags = new GitHubFeatureFlags(
{ type: GitHubVariant.DOTCOM },
testApiDetails,
testRepositoryNwo,
getRunnerLogger(true)
);
const featureFlags = setUpTmpDir(tmpDir);
// set all feature flags to false except the one we're testing
const expectedFeatureFlags: { [flag: string]: boolean } = {};
for (const f of FEATURE_FLAGS) {
expectedFeatureFlags[f] = false;
for (const f of Object.keys(featureFlagConfig)) {
expectedFeatureFlags[f] = f === featureFlag;
}
expectedFeatureFlags[featureFlag] = true;
mockFeatureFlagApiEndpoint(200, expectedFeatureFlags);
const actualFeatureFlags: { [flag: string]: boolean } = {
ml_powered_queries_enabled: await featureFlags.getValue(
FeatureFlag.MlPoweredQueriesEnabled
),
};
// retrieve the values of the actual feature flags
const actualFeatureFlags: { [flag: string]: boolean } = {};
for (const f of Object.keys(featureFlagConfig)) {
actualFeatureFlags[f] = await featureFlags.getValue(f as FeatureFlag);
}
// Alls flags should be false except the one we're testing
t.deepEqual(actualFeatureFlags, expectedFeatureFlags);
});
});
test(`Feature flag '${featureFlag}' is enabled if the associated environment variable is true`, async (t) => {
await withTmpDir(async (tmpDir) => {
const featureFlags = setUpTmpDir(tmpDir);
// set all feature flags to false
const expectedFeatureFlags: { [flag: string]: boolean } = {};
for (const f of Object.keys(featureFlagConfig)) {
expectedFeatureFlags[f] = false;
}
mockFeatureFlagApiEndpoint(200, expectedFeatureFlags);
// feature flag should be disabled initially
t.assert(!(await featureFlags.getValue(featureFlag as FeatureFlag)));
// set env var to true and check that the feature flag is now enabled
process.env[featureFlagConfig[featureFlag].envVar] = "true";
t.assert(await featureFlags.getValue(featureFlag as FeatureFlag));
});
});
test(`Feature flag '${featureFlag}' is disabled if the associated environment variable is false`, async (t) => {
await withTmpDir(async (tmpDir) => {
const featureFlags = setUpTmpDir(tmpDir);
// set all feature flags to true
const expectedFeatureFlags: { [flag: string]: boolean } = {};
for (const f of Object.keys(featureFlagConfig)) {
expectedFeatureFlags[f] = true;
}
mockFeatureFlagApiEndpoint(200, expectedFeatureFlags);
// feature flag should be enabled initially
t.assert(await featureFlags.getValue(featureFlag as FeatureFlag));
// set env var to false and check that the feature flag is now disabled
process.env[featureFlagConfig[featureFlag].envVar] = "false";
t.assert(!(await featureFlags.getValue(featureFlag as FeatureFlag)));
});
});
if (featureFlagConfig[featureFlag].minimumVersion !== undefined) {
test(`Feature flag '${featureFlag}' is disabled if the minimum CLI version is below ${featureFlagConfig[featureFlag].minimumVersion}`, async (t) => {
await withTmpDir(async (tmpDir) => {
const featureFlags = setUpTmpDir(tmpDir);
// set all feature flags to true
const expectedFeatureFlags: { [flag: string]: boolean } = {};
for (const f of Object.keys(featureFlagConfig)) {
expectedFeatureFlags[f] = true;
}
mockFeatureFlagApiEndpoint(200, expectedFeatureFlags);
// feature flag should be enabled initially (ignoring the minimum CLI version)
t.assert(await featureFlags.getValue(featureFlag as FeatureFlag));
// feature flag should be disabled when an old CLI version is set
let codeql = mockCodeQLVersion("2.0.0");
t.assert(
!(await featureFlags.getValue(featureFlag as FeatureFlag, codeql))
);
// even setting the env var to true should not enable the feature flag if
// the minimum CLI version is not met
process.env[featureFlagConfig[featureFlag].envVar] = "true";
t.assert(
!(await featureFlags.getValue(featureFlag as FeatureFlag, codeql))
);
// feature flag should be enabled when a new CLI version is set
// and env var is not set
process.env[featureFlagConfig[featureFlag].envVar] = "";
codeql = mockCodeQLVersion(
featureFlagConfig[featureFlag].minimumVersion
);
t.assert(
await featureFlags.getValue(featureFlag as FeatureFlag, codeql)
);
// set env var to false and check that the feature flag is now disabled
process.env[featureFlagConfig[featureFlag].envVar] = "false";
t.assert(
!(await featureFlags.getValue(featureFlag as FeatureFlag, codeql))
);
});
});
}
}
function setUpTmpDir(
tmpDir: string,
logger = getRunnerLogger(true),
gitHubVersion = { type: GitHubVariant.DOTCOM } as util.GitHubVersion
): FeatureFlags {
setupActionsVars(tmpDir, tmpDir);
return new GitHubFeatureFlags(
gitHubVersion,
testApiDetails,
testRepositoryNwo,
logger
);
}

54
src/feature-flags.ts
View file

@ -1,10 +1,11 @@
import { getApiClient, GitHubApiDetails } from "./api-client";
import { CodeQL } from "./codeql";
import { Logger } from "./logging";
import { RepositoryNwo } from "./repository";
import * as util from "./util";
export interface FeatureFlags {
getValue(flag: FeatureFlag): Promise<boolean>;
getValue(flag: FeatureFlag, codeql?: CodeQL): Promise<boolean>;
}
export enum FeatureFlag {
@ -15,6 +16,32 @@ export enum FeatureFlag {
CliConfigFileEnabled = "cli_config_file_enabled",
}
export const featureFlagConfig: Record<
FeatureFlag,
{ envVar: string; minimumVersion: string | undefined }
> = {
[FeatureFlag.BypassToolcacheEnabled]: {
envVar: "CODEQL_BYPASS_TOOLCACHE",
minimumVersion: undefined,
},
[FeatureFlag.MlPoweredQueriesEnabled]: {
envVar: "CODEQL_VERSION_ML_POWERED_QUERIES",
minimumVersion: "2.7.5",
},
[FeatureFlag.TrapCachingEnabled]: {
envVar: "CODEQL_TRAP_CACHING",
minimumVersion: undefined,
},
[FeatureFlag.GolangExtractionReconciliationEnabled]: {
envVar: "CODEQL_GOLANG_EXTRACTION_RECONCILIATION",
minimumVersion: undefined,
},
[FeatureFlag.CliConfigFileEnabled]: {
envVar: "CODEQL_PASS_CONFIG_TO_CLI",
minimumVersion: "2.10.1",
},
};
/**
* A response from the GitHub API that contains feature flag enablement information for the CodeQL
* Action.
@ -33,12 +60,35 @@ export class GitHubFeatureFlags implements FeatureFlags {
private logger: Logger
) {}
async getValue(flag: FeatureFlag): Promise<boolean> {
async getValue(flag: FeatureFlag, codeql?: CodeQL): Promise<boolean> {
// Bypassing the toolcache is disabled in test mode.
if (flag === FeatureFlag.BypassToolcacheEnabled && util.isInTestMode()) {
return false;
}
const envVar = (
process.env[featureFlagConfig[flag].envVar] || ""
).toLocaleLowerCase();
// Do not use this feature if user explicitly disables it via an environment variable.
if (envVar === "false") {
return false;
}
// Never use this feature if the CLI version explicitly can't support it.
const minimumVersion = featureFlagConfig[flag].minimumVersion;
if (codeql && minimumVersion) {
if (!(await util.codeQlVersionAbove(codeql, minimumVersion))) {
return false;
}
}
// Use this feature if user explicitly enables it via an environment variable.
if (envVar === "true") {
return true;
}
// Ask the GitHub API if the feature is enabled.
const response = await this.getApiResponse();
if (response === undefined) {
this.logger.debug(

4
src/init-action.ts
View file

@ -321,7 +321,9 @@ async function getTrapCachingEnabled(
featureFlags: FeatureFlags
): Promise<boolean> {
const trapCaching = getOptionalInput("trap-caching");
if (trapCaching !== undefined) return trapCaching === "true";
if (trapCaching !== undefined) {
return trapCaching === "true";
}
return await featureFlags.getValue(FeatureFlag.TrapCachingEnabled);
}

8
src/testing-utils.ts
View file

@ -160,3 +160,11 @@ export function mockFeatureFlagApiEndpoint(
sinon.stub(apiClient, "getApiClient").value(() => client);
}
export function mockCodeQLVersion(version) {
return {
async getVersion() {
return version;
},
} as CodeQL.CodeQL;
}

109
src/util.test.ts
View file

@ -9,9 +9,7 @@ import test, { ExecutionContext } from "ava";
import * as sinon from "sinon";
import * as api from "./api-client";
import { CodeQL } from "./codeql";
import { Config } from "./config-utils";
import { createFeatureFlags, FeatureFlag } from "./feature-flags";
import { getRunnerLogger, Logger } from "./logging";
import { setupTests } from "./testing-utils";
import * as util from "./util";
@ -495,113 +493,6 @@ test("listFolder", async (t) => {
});
});
test("useCodeScanningConfigInCli with no env var", async (t) => {
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.0"),
createFeatureFlags([])
))
);
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.1"),
createFeatureFlags([])
))
);
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.0"),
createFeatureFlags([FeatureFlag.CliConfigFileEnabled])
))
);
// Yay! It works!
t.assert(
await util.useCodeScanningConfigInCli(
mockVersion("2.10.1"),
createFeatureFlags([FeatureFlag.CliConfigFileEnabled])
)
);
});
for (const val of ["TRUE", "true", "True"]) {
test(`useCodeScanningConfigInCli with env var ${val}`, async (t) => {
process.env[util.EnvVar.CODEQL_PASS_CONFIG_TO_CLI] = val;
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.0"),
createFeatureFlags([])
))
);
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.0"),
createFeatureFlags([FeatureFlag.CliConfigFileEnabled])
))
);
// Yay! It works!
t.assert(
await util.useCodeScanningConfigInCli(
mockVersion("2.10.1"),
createFeatureFlags([FeatureFlag.CliConfigFileEnabled])
)
);
t.assert(
await util.useCodeScanningConfigInCli(
mockVersion("2.10.1"),
createFeatureFlags([])
)
);
});
}
for (const val of ["FALSE", "false", "False"]) {
test(`useCodeScanningConfigInCli with env var ${val}`, async (t) => {
// Never turned on when env var is false
process.env[util.EnvVar.CODEQL_PASS_CONFIG_TO_CLI] = val;
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.0"),
createFeatureFlags([])
))
);
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.0"),
createFeatureFlags([FeatureFlag.CliConfigFileEnabled])
))
);
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.1"),
createFeatureFlags([FeatureFlag.CliConfigFileEnabled])
))
);
t.assert(
!(await util.useCodeScanningConfigInCli(
mockVersion("2.10.1"),
createFeatureFlags([])
))
);
});
}
function mockVersion(version) {
return {
async getVersion() {
return version;
},
} as CodeQL;
}
const longTime = 999_999;
const shortTime = 10;

44
src/util.ts
View file

@ -12,11 +12,7 @@ import * as semver from "semver";
import * as api from "./api-client";
import { getApiClient, GitHubApiDetails } from "./api-client";
import * as apiCompatibility from "./api-compatibility.json";
import {
CodeQL,
CODEQL_VERSION_CONFIG_FILES,
CODEQL_VERSION_NEW_TRACING,
} from "./codeql";
import { CodeQL, CODEQL_VERSION_NEW_TRACING } from "./codeql";
import {
Config,
parsePacksSpecification,
@ -524,13 +520,6 @@ export enum EnvVar {
* own sandwiched workflow mechanism
*/
FEATURE_SANDWICH = "CODEQL_ACTION_FEATURE_SANDWICH",
/**
* If set to the "true" string and the codeql CLI version is greater than
* `CODEQL_VERSION_CONFIG_FILES`, then the codeql-action will pass the
* the codeql-config file to the codeql CLI to be processed there.
*/
CODEQL_PASS_CONFIG_TO_CLI = "CODEQL_PASS_CONFIG_TO_CLI",
}
const exportVar = (mode: Mode, name: string, value: string) => {
@ -593,10 +582,6 @@ export function getRequiredEnvParam(paramName: string): string {
return value;
}
function getOptionalEnvParam(paramName: string): string {
return process.env[paramName] || "";
}
export class HTTPError extends Error {
public status: number;
@ -796,25 +781,7 @@ export async function useCodeScanningConfigInCli(
codeql: CodeQL,
featureFlags: FeatureFlags
): Promise<boolean> {
const envVarIsEnabled = getOptionalEnvParam(EnvVar.CODEQL_PASS_CONFIG_TO_CLI);
// If the user has explicitly turned off the feature, then don't use it.
if (envVarIsEnabled.toLocaleLowerCase() === "false") {
return false;
}
// If the user has explicitly turned on the feature, then use it.
// Or if the feature flag is enabled, then use it.
const isEnabled =
envVarIsEnabled.toLocaleLowerCase() === "true" ||
(await featureFlags.getValue(FeatureFlag.CliConfigFileEnabled));
if (!isEnabled) {
return false;
}
// If the CLI version is too old, then don't use it.
return await codeQlVersionAbove(codeql, CODEQL_VERSION_CONFIG_FILES);
return await featureFlags.getValue(FeatureFlag.CliConfigFileEnabled, codeql);
}
export async function logCodeScanningConfigInCli(
@ -867,11 +834,8 @@ export function listFolder(dir: string): string[] {
export async function isGoExtractionReconciliationEnabled(
featureFlags: FeatureFlags
): Promise<boolean> {
return (
process.env["CODEQL_ACTION_RECONCILE_GO_EXTRACTION"] === "true" ||
(await featureFlags.getValue(
FeatureFlag.GolangExtractionReconciliationEnabled
))
return await featureFlags.getValue(
FeatureFlag.GolangExtractionReconciliationEnabled
);
}