From 2fb8720406d7f19bc77a93ad845706ff5fe9eb64 Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Tue, 2 Apr 2019 17:13:50 +0200 Subject: [PATCH] fix(config): catch module self-references instead of crashing --- garden-service/src/config/config-context.ts | 26 +++++++++++++++---- garden-service/src/garden.ts | 18 ++++++++----- garden-service/src/template-string.ts | 6 +++-- .../test-projects/module-self-ref/garden.yml | 6 +++++ .../module-self-ref/module-a/garden.yml | 5 ++++ garden-service/test/unit/src/garden.ts | 13 ++++++++++ garden-service/test/unit/src/vcs/base.ts | 15 ++++++----- 7 files changed, 68 insertions(+), 21 deletions(-) create mode 100644 garden-service/test/unit/data/test-projects/module-self-ref/garden.yml create mode 100644 garden-service/test/unit/data/test-projects/module-self-ref/module-a/garden.yml diff --git a/garden-service/src/config/config-context.ts b/garden-service/src/config/config-context.ts index 2fdd7c0192..2f07885562 100644 --- a/garden-service/src/config/config-context.ts +++ b/garden-service/src/config/config-context.ts @@ -93,14 +93,25 @@ export abstract class ConfigContext { if (typeof value === "function") { // call the function to resolve the value, then continue - value = await value() + if (opts.stack.includes(stackEntry)) { + throw new ConfigurationError( + `Circular reference detected when resolving key ${stackEntry} (from ${opts.stack.join(" -> ")})`, + { + nodePath, + fullPath, + opts, + }, + ) + } + + opts.stack.push(stackEntry) + value = await value({ key: remainder, nodePath: nestedNodePath, opts }) } // handle nested contexts if (value instanceof ConfigContext) { - const nestedKey = remainder opts.stack.push(stackEntry) - value = await value.resolve({ key: nestedKey, nodePath: nestedNodePath, opts }) + value = await value.resolve({ key: remainder, nodePath: nestedNodePath, opts }) break } @@ -273,9 +284,14 @@ export class ModuleConfigContext extends ProjectConfigContext { this.environment = new EnvironmentContext(_this, environment.name) this.modules = new Map(moduleConfigs.map((config) => - <[string, () => Promise]>[config.name, async () => { + <[string, () => Promise]>[config.name, async (opts: ContextResolveOpts) => { // NOTE: This is a temporary hacky solution until we implement module resolution as a TaskGraph task - const resolvedConfig = await garden.resolveModuleConfig(config.name) + const stackKey = "modules." + config.name + const resolvedConfig = await garden.resolveModuleConfig(config.name, { + configContext: _this, + ...opts, + stack: [...opts.stack || [], stackKey], + }) const version = await garden.resolveVersion(resolvedConfig.name, resolvedConfig.build.dependencies) const buildPath = await garden.buildDir.buildPath(config.name) diff --git a/garden-service/src/garden.ts b/garden-service/src/garden.ts index 9c7499eba3..8f2d71c617 100644 --- a/garden-service/src/garden.ts +++ b/garden-service/src/garden.ts @@ -65,7 +65,7 @@ import { BaseTask } from "./tasks/base" import { LocalConfigStore } from "./config-store" import { getLinkedSources, ExternalSourceType } from "./util/ext-source-util" import { BuildDependencyConfig, ModuleConfig } from "./config/module" -import { ProjectConfigContext, ModuleConfigContext } from "./config/config-context" +import { ProjectConfigContext, ModuleConfigContext, ContextResolveOpts } from "./config/config-context" import { ActionHelper } from "./actions" import { createPluginContext } from "./plugin-context" import { ModuleAndRuntimeActions, Plugins, RegisterPluginParam } from "./types/plugin/plugin" @@ -104,6 +104,10 @@ export interface GardenOpts { plugins?: Plugins, } +interface ModuleConfigResolveOpts extends ContextResolveOpts { + configContext?: ModuleConfigContext +} + const scanLock = new AsyncLock() export class Garden { @@ -501,15 +505,15 @@ export class Garden { * plugin handlers). * Scans for modules in the project root and remote/linked sources if it hasn't already been done. */ - async resolveModuleConfigs(keys?: string[], configContext?: ModuleConfigContext): Promise { + async resolveModuleConfigs(keys?: string[], opts: ModuleConfigResolveOpts = {}): Promise { const configs = await this.getRawModuleConfigs(keys) - if (!configContext) { - configContext = new ModuleConfigContext(this, this.environment, Object.values(this.moduleConfigs)) + if (!opts.configContext) { + opts.configContext = new ModuleConfigContext(this, this.environment, Object.values(this.moduleConfigs)) } return Bluebird.map(configs, async (config) => { - config = await resolveTemplateStrings(cloneDeep(config), configContext!) + config = await resolveTemplateStrings(cloneDeep(config), opts.configContext!, opts) const configureHandler = await this.actions.getModuleActionHandler({ actionType: "configure", @@ -529,8 +533,8 @@ export class Garden { /** * Returns the module with the specified name. Throws error if it doesn't exist. */ - async resolveModuleConfig(name: string, configContext?: ModuleConfigContext): Promise { - return (await this.resolveModuleConfigs([name], configContext))[0] + async resolveModuleConfig(name: string, opts: ModuleConfigResolveOpts = {}): Promise { + return (await this.resolveModuleConfigs([name], opts))[0] } /** diff --git a/garden-service/src/template-string.ts b/garden-service/src/template-string.ts index 9218c361ca..fec6174057 100644 --- a/garden-service/src/template-string.ts +++ b/garden-service/src/template-string.ts @@ -64,10 +64,12 @@ export async function resolveTemplateString(string: string, context: ConfigConte /** * Recursively parses and resolves all templated strings in the given object. */ -export async function resolveTemplateStrings(obj: T, context: ConfigContext): Promise { +export async function resolveTemplateStrings( + obj: T, context: ConfigContext, opts: ContextResolveOpts = {}, +): Promise { return asyncDeepMap( obj, - (v) => typeof v === "string" ? resolveTemplateString(v, context) : v, + (v) => typeof v === "string" ? resolveTemplateString(v, context, opts) : v, // need to iterate sequentially to catch potential circular dependencies { concurrency: 1 }, ) diff --git a/garden-service/test/unit/data/test-projects/module-self-ref/garden.yml b/garden-service/test/unit/data/test-projects/module-self-ref/garden.yml new file mode 100644 index 0000000000..06b1cd1c3b --- /dev/null +++ b/garden-service/test/unit/data/test-projects/module-self-ref/garden.yml @@ -0,0 +1,6 @@ +project: + name: duplicate-module + environments: + - name: local + providers: + - name: test-plugin diff --git a/garden-service/test/unit/data/test-projects/module-self-ref/module-a/garden.yml b/garden-service/test/unit/data/test-projects/module-self-ref/module-a/garden.yml new file mode 100644 index 0000000000..662ac8e73a --- /dev/null +++ b/garden-service/test/unit/data/test-projects/module-self-ref/module-a/garden.yml @@ -0,0 +1,5 @@ +kind: Module +name: module-a +type: test +build: + args: ["${modules.module-a.version}"] diff --git a/garden-service/test/unit/src/garden.ts b/garden-service/test/unit/src/garden.ts index 0b0fe6988f..442cbdb8d7 100644 --- a/garden-service/test/unit/src/garden.ts +++ b/garden-service/test/unit/src/garden.ts @@ -203,6 +203,19 @@ describe("Garden", () => { }) }) + describe("resolveModuleConfigs", () => { + it("should throw if a module references itself in a template string", async () => { + const projectRoot = resolve(dataDir, "test-projects", "module-self-ref") + const garden = await makeTestGarden(projectRoot) + await expectError( + () => garden.resolveModuleConfigs(), + (err) => expect(err.message).to.equal( + "Circular reference detected when resolving key modules.module-a (from modules.module-a)", + ), + ) + }) + }) + describe("resolveVersion", () => { beforeEach(() => td.reset()) diff --git a/garden-service/test/unit/src/vcs/base.ts b/garden-service/test/unit/src/vcs/base.ts index af9d15342b..17fcda6d5a 100644 --- a/garden-service/test/unit/src/vcs/base.ts +++ b/garden-service/test/unit/src/vcs/base.ts @@ -11,6 +11,7 @@ import { expect } from "chai" import { cloneDeep } from "lodash" import { Garden } from "../../../../src/garden" import { ModuleConfigContext } from "../../../../src/config/config-context" +import { ModuleConfig } from "../../../../src/config/module" class TestVcsHandler extends VcsHandler { name = "test" @@ -85,10 +86,10 @@ describe("VcsHandler", () => { }) describe("getVersionString", () => { - let moduleABefore - let moduleAAfter - let moduleBBefore - let moduleBAfter + let moduleABefore: ModuleConfig + let moduleAAfter: ModuleConfig + let moduleBBefore: ModuleConfig + let moduleBAfter: ModuleConfig before(async () => { const templateGarden = await makeTestGarden(getDataDir("test-project-variable-versioning")) @@ -98,11 +99,11 @@ describe("VcsHandler", () => { const moduleAAfterEnv = cloneDeep(templateGarden.environment) moduleAAfterEnv.variables["echo-string"] = "something else" - const changedModuleConfigContext = new ModuleConfigContext( + const configContext = new ModuleConfigContext( templateGarden, moduleAAfterEnv, await templateGarden.getRawModuleConfigs()) - moduleAAfter = await templateGarden.resolveModuleConfig("module-a", changedModuleConfigContext) - moduleBAfter = await templateGarden.resolveModuleConfig("module-b", changedModuleConfigContext) + moduleAAfter = await templateGarden.resolveModuleConfig("module-a", { configContext }) + moduleBAfter = await templateGarden.resolveModuleConfig("module-b", { configContext }) }) it("should return a different version for a module when a variable used by it changes", async () => {