fix(workflows): forbid use of global options

Global options (such as `--env` or `-l`) have no effect when used in workflow step commands, since the step commands share a Garden instance with the enclosing `run workflow` command. To prevent user confusion, we now validate that global options are not used in workflow step command specs, and throw an error if any are detected.
garden-io · Oct 14, 2020 · 34d980f · 34d980f
1 parent 91a6976
commit 34d980f
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 18 deletions.
diff --git a/core/src/cli/helpers.ts b/core/src/cli/helpers.ts
@@ -117,10 +117,22 @@ export type ParamSpec = {
 /**
  * Parses the given CLI arguments using minimist. The result should be fed to `processCliArgs()`
  *
- * @param stringArgs Raw string arguments
- * @param command    The Command that the arguments are for, if any
+ * @param stringArgs  Raw string arguments
+ * @param command     The Command that the arguments are for, if any
+ * @param cli         If true, prefer `param.cliDefault` to `param.defaultValue`
+ * @param skipDefault Defaults to `false`. If `true`, don't populate default values.
  */
-export function parseCliArgs({ stringArgs, command, cli }: { stringArgs: string[]; command?: Command; cli: boolean }) {
+export function parseCliArgs({
+  stringArgs,
+  command,
+  cli,
+  skipDefault = false,
+}: {
+  stringArgs: string[]
+  command?: Command
+  cli: boolean
+  skipDefault?: boolean
+}) {
   // Tell minimist which flags are to be treated explicitly as booleans and strings
   const allOptions = { ...globalOptions, ...(command?.options || {}) }
   const booleanKeys = Object.keys(pickBy(allOptions, (spec) => spec.type === "boolean"))
@@ -131,11 +143,15 @@ export function parseCliArgs({ stringArgs, command, cli }: { stringArgs: string[
   const defaultValues = {}
 
   for (const [name, spec] of Object.entries(allOptions)) {
-    defaultValues[name] = spec.getDefaultValue(cli)
+    if (!skipDefault) {
+      defaultValues[name] = spec.getDefaultValue(cli)
+    }
 
     if (spec.alias) {
       aliases[name] = spec.alias
-      defaultValues[spec.alias] = defaultValues[name]
+      if (!skipDefault) {
+        defaultValues[spec.alias] = defaultValues[name]
+      }
     }
   }
 

diff --git a/core/src/config/workflow.ts b/core/src/config/workflow.ts
@@ -6,7 +6,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-import { cloneDeep, isEqual, omit, take } from "lodash"
+import { cloneDeep, isEqual, merge, omit, take } from "lodash"
 import { joi, joiUserIdentifier, joiVariableName, joiIdentifier } from "./common"
 import { DEFAULT_API_VERSION } from "../constants"
 import { deline, dedent } from "../util/string"
@@ -19,6 +19,9 @@ import { ConfigurationError } from "../exceptions"
 import { coreCommands } from "../commands/commands"
 import { CommandGroup } from "../commands/base"
 import { EnvironmentConfig, getNamespace } from "./project"
+import { globalOptions } from "../cli/params"
+import { isTruthy } from "../util/util"
+import { parseCliArgs, pickCommand } from "../cli/helpers"
 
 export interface WorkflowConfig {
   apiVersion: string
@@ -319,34 +322,95 @@ function getStepCommands() {
     .filter((cmd) => cmd.workflows)
 }
 
+const globalOptionNames = Object.keys(globalOptions).sort()
+
 /**
- * Throws if one or more steps refers to a command that is not supported in workflows.
+ * Throws if one or more steps refers to a command that is not supported in workflows, or one that uses CLI options
+ * that are not supported for step commands.
  */
 function validateSteps(config: WorkflowConfig) {
-  const validStepCommandPrefixes = getStepCommands().map((c) => c.getPath())
+  const prefixErrors = validateStepCommandPrefixes(config)
+  const argumentErrors = validateStepCommandOptions(config)
+  const errors = [prefixErrors, argumentErrors].filter(isTruthy)
+  let errorMsg = errors.map(({ msg }) => msg).join("\n\n")
+  let errorDetail = merge({}, ...errors.map(({ detail }) => detail))
+
+  if (errorMsg) {
+    throw new ConfigurationError(errorMsg, errorDetail)
+  }
+}
 
-  const invalidSteps: WorkflowStepSpec[] = config.steps.filter(
+function validateStepCommandPrefixes(config: WorkflowConfig) {
+  const validStepCommandPrefixes = getStepCommands().map((c) => c.getPath())
+  const stepsWithInvalidPrefix: WorkflowStepSpec[] = config.steps.filter(
     (step) =>
       !!step.command && !validStepCommandPrefixes.find((valid) => isEqual(valid, take(step.command, valid.length)))
   )
 
-  if (invalidSteps.length > 0) {
-    const msgPrefix =
-      invalidSteps.length === 1
-        ? `Invalid step command for workflow ${config.name}:`
-        : `Invalid step commands for workflow ${config.name}:`
-    const descriptions = invalidSteps.map((step) => `[${step.command!.join(", ")}]`)
+  if (stepsWithInvalidPrefix.length > 0) {
+    const cmdString = stepsWithInvalidPrefix.length === 1 ? "command" : "commands"
+    const descriptions = stepsWithInvalidPrefix.map((step) => `[${step.command!.join(", ")}]`)
     const validDescriptions = validStepCommandPrefixes.map((cmd) => `[${cmd.join(", ")}]`)
     const msg = dedent`
-      ${msgPrefix}
+      Invalid step ${cmdString} for workflow ${config.name}:
 
       ${descriptions.join("\n")}
 
-      Valid step command prefixes:
+      Valid step commands:
 
       ${validDescriptions.join("\n")}
     `
-    throw new ConfigurationError(msg, { invalidSteps })
+    return { msg, detail: { stepsWithInvalidPrefix } }
+  } else {
+    return null
+  }
+}
+
+/**
+ * Finds usages of global CLI options in `step.commandSpec` (if present).
+ *
+ * These will be ignored when the step command is run, so we warn the user not to use them.
+ *
+ * TODO: Also detect invalid command args at validation time (these will result in exceptions when the workflow is run).
+ */
+function findInvalidOptions(step: WorkflowStepSpec) {
+  if (!step.command) {
+    return null
+  }
+  const { command, rest } = pickCommand(getStepCommands(), step.command!)
+  const parsedArgs = parseCliArgs({ stringArgs: rest, command, cli: false, skipDefault: true })
+  const usedGlobalOptions = Object.entries(parsedArgs)
+    .filter(([name, value]) => globalOptionNames.find((optName) => optName === name) && !!value)
+    .map(([name, _]) => `--${name}`)
+  if (usedGlobalOptions.length > 0) {
+    const availableOptions = Object.keys(command!.options || {})
+    const availableDescription =
+      availableOptions.length > 0 ? `(available options: ${availableOptions.map((opt) => `--${opt}`).join(", ")})` : ""
+    const errorMsg = dedent`
+      Invalid options in step command [${step.command!.join(", ")}]: ${usedGlobalOptions.join(", ")}
+      ${availableDescription}
+    `
+    return { step, errorMsg }
+  } else {
+    return null
+  }
+}
+
+function validateStepCommandOptions(config: WorkflowConfig) {
+  const invalidSteps = config.steps.map((step) => findInvalidOptions(step)).filter(isTruthy)
+
+  if (invalidSteps.length > 0) {
+    const msgPrefix = `Invalid step command options for workflow ${config.name}:`
+    const msg = dedent`
+      ${msgPrefix}
+
+      ${invalidSteps.map((s) => s.errorMsg).join("\n\n")}
+
+      Global options (such as --env or --log-level) are not available in workflow step commands
+    `
+    return { msg, detail: { invalidStepCommands: invalidSteps.map((e) => e.step.command) } }
+  } else {
+    return null
   }
 }
 

diff --git a/core/src/util/util.ts b/core/src/util/util.ts
@@ -607,6 +607,13 @@ export function isPromise(obj: any): obj is Promise<any> {
   return !!obj && (typeof obj === "object" || typeof obj === "function") && typeof obj.then === "function"
 }
 
+/**
+ * A type guard that's useful e.g. when filtering an array which may have blank entries.
+ */
+export function isTruthy<T>(value: T | undefined | null | false | 0 | ""): value is T {
+  return !!value
+}
+
 // Used to make the platforms more consistent with other tools
 const platformMap = {
   win32: "windows",

diff --git a/core/test/unit/src/config/workflow.ts b/core/test/unit/src/config/workflow.ts
@@ -133,6 +133,33 @@ describe("resolveWorkflowConfig", () => {
     )
   })
 
+  it("should throw if a step command uses a global option", async () => {
+    const config: WorkflowConfig = {
+      ...defaults,
+      apiVersion: DEFAULT_API_VERSION,
+      kind: "Workflow",
+      name: "workflow-a",
+      path: "/tmp/foo",
+      description: "Sample workflow",
+      steps: [{ command: ["test", "--env=foo", "-l", "4"] }, { command: ["test", "--silent"] }],
+      triggers: [
+        {
+          environment: "local",
+          events: ["pull-request"],
+          branches: ["feature*"],
+          ignoreBranches: ["feature-ignored*"],
+          tags: ["v1*"],
+          ignoreTags: ["v1-ignored*"],
+        },
+      ],
+    }
+
+    await expectError(
+      () => resolveWorkflowConfig(garden, config),
+      (err) => expect(err.message).to.match(/Invalid step command options for workflow workflow-a/)
+    )
+  })
+
   it("should throw if a trigger uses an environment that isn't defined in the project", async () => {
     const config: WorkflowConfig = {
       ...defaults,