From 7cef10ebede00defa0fddeb195238d1405ffd014 Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Thu, 5 Dec 2019 15:04:33 +0100 Subject: [PATCH] improvement(container): automatically set include field based on config This avoids common issues where multiple module sources overlap, or Garden build contexts accidentally include a bunch of unnecessary files. We parse the Dockerfile if the module has one and extract the paths referenced in the ADD and COPY fields there. If there is no Dockerfile, we automatically set include to an empty list. --- garden-service/package-lock.json | 22 +++ garden-service/package.json | 5 + garden-service/src/actions.ts | 2 +- garden-service/src/config/common.ts | 2 +- .../src/plugins/container/container.ts | 7 +- .../src/plugins/container/helpers.ts | 93 +++++++++- .../unit/src/plugins/container/container.ts | 1 + .../unit/src/plugins/container/helpers.ts | 169 ++++++++++++++++++ 8 files changed, 297 insertions(+), 4 deletions(-) diff --git a/garden-service/package-lock.json b/garden-service/package-lock.json index d2f839af33..d31510d010 100644 --- a/garden-service/package-lock.json +++ b/garden-service/package-lock.json @@ -939,6 +939,18 @@ "integrity": "sha512-Hozx1nI6wJi3ZcRyIcJq7eg/tA9WHu0jEXBSYijogCAhGY3qer/yC3x5buTSpiWEA51wfd1NjYsGRhmMSMuTag==", "dev": true }, + "@types/is-glob": { + "version": "4.0.1", + "resolved": "https://registry.npmjs.org/@types/is-glob/-/is-glob-4.0.1.tgz", + "integrity": "sha512-k3RS5HyBPu4h+5hTmIEfPB2rl5P3LnGdQEZrV2b9OWTJVtsUQ2VBcedqYKGqxvZqle5UALUXdSfVA8nf3HfyWQ==", + "dev": true + }, + "@types/is-url": { + "version": "1.2.28", + "resolved": "https://registry.npmjs.org/@types/is-url/-/is-url-1.2.28.tgz", + "integrity": "sha1-kU2r1QVG2bAUKAbkLHK8fCt+B4c=", + "dev": true + }, "@types/js-yaml": { "version": "3.12.1", "resolved": "https://registry.npmjs.org/@types/js-yaml/-/js-yaml-3.12.1.tgz", @@ -3899,6 +3911,11 @@ "resolved": "https://registry.npmjs.org/directory-tree/-/directory-tree-2.2.4.tgz", "integrity": "sha512-2N43msQptKbi3WMfIs+U09yi6bfyKL+MWyj5VMj8t1F/Tx04bt1cn/EEIU3o1JBltlJk7NQnzOEuTNa/KQvbWA==" }, + "docker-file-parser": { + "version": "1.0.4", + "resolved": "https://registry.npmjs.org/docker-file-parser/-/docker-file-parser-1.0.4.tgz", + "integrity": "sha512-djh3R7KXkEPm80PXK9xbz8bCfEFuU11Tmf5l9IXKdjBPx91/cOqhwOwtOq6s35B8TqrwY6L4xLphmyYmJT0ZXw==" + }, "docker-modem": { "version": "2.0.4", "resolved": "https://registry.npmjs.org/docker-modem/-/docker-modem-2.0.4.tgz", @@ -6940,6 +6957,11 @@ "unc-path-regex": "^0.1.2" } }, + "is-url": { + "version": "1.2.4", + "resolved": "https://registry.npmjs.org/is-url/-/is-url-1.2.4.tgz", + "integrity": "sha512-ITvGim8FhRiYe4IQ5uHSkj7pVaPDrCTkNd3yq3cV7iZAcJdHTUMPMEHcqSOy9xZ9qFenQCvi+2wjH9a1nXqHww==" + }, "is-utf8": { "version": "0.2.1", "resolved": "https://registry.npmjs.org/is-utf8/-/is-utf8-0.2.1.tgz", diff --git a/garden-service/package.json b/garden-service/package.json index cffcfe3fdd..56768484db 100644 --- a/garden-service/package.json +++ b/garden-service/package.json @@ -53,6 +53,7 @@ "deline": "^1.0.4", "dependency-graph": "^0.8.0", "directory-tree": "^2.2.4", + "docker-file-parser": "^1.0.4", "dockerode": "^3.0.2", "dotenv": "^8.2.0", "elegant-spinner": "^2.0.0", @@ -68,6 +69,8 @@ "indent-string": "^4.0.0", "inquirer": "^7.0.0", "is-git-url": "^1.0.0", + "is-glob": "^4.0.1", + "is-url": "^1.2.4", "js-yaml": "^3.13.1", "json-diff": "^0.5.4", "json-merge-patch": "^0.2.3", @@ -140,6 +143,8 @@ "@types/has-ansi": "^3.0.0", "@types/inquirer": "6.5.0", "@types/is-git-url": "^1.0.0", + "@types/is-glob": "^4.0.1", + "@types/is-url": "^1.2.28", "@types/js-yaml": "^3.12.1", "@types/json-merge-patch": "0.0.4", "@types/json-stringify-safe": "^5.0.0", diff --git a/garden-service/src/actions.ts b/garden-service/src/actions.ts index 53e6b28eb0..79d2bb0b61 100644 --- a/garden-service/src/actions.ts +++ b/garden-service/src/actions.ts @@ -884,7 +884,7 @@ export class ActionRouter implements TypeGuard { pluginName, }) } - return validate(result, schema, { context: `${actionType} output from plugin ${pluginName}` }) + return validate(result, schema, { context: `${actionType} ${moduleType} output from provider ${pluginName}` }) }), { actionType, pluginName, moduleType } ) diff --git a/garden-service/src/config/common.ts b/garden-service/src/config/common.ts index 7d6b9195c7..3bc4995163 100644 --- a/garden-service/src/config/common.ts +++ b/garden-service/src/config/common.ts @@ -174,7 +174,7 @@ export const joi: Joi.Root = Joi.extend({ if (result.error) { // tslint:disable-next-line:no-invalid-this - return this.createError("posixPath", { v: value }, state, prefs) + return this.createError("string.posixPath", { v: value }, state, prefs) } const options = params.options || {} diff --git a/garden-service/src/plugins/container/container.ts b/garden-service/src/plugins/container/container.ts index 986c4f7da0..66e7bb89e5 100644 --- a/garden-service/src/plugins/container/container.ts +++ b/garden-service/src/plugins/container/container.ts @@ -44,7 +44,7 @@ const taskOutputsSchema = joi.object().keys({ ), }) -export async function configureContainerModule({ ctx, moduleConfig }: ConfigureModuleParams) { +export async function configureContainerModule({ ctx, log, moduleConfig }: ConfigureModuleParams) { // validate hot reload configuration // TODO: validate this when validating this action's output const hotReloadConfig = moduleConfig.spec.hotReload @@ -150,6 +150,11 @@ export async function configureContainerModule({ ctx, moduleConfig }: ConfigureM "deployment-image-name": deploymentImageName, } + // Automatically set the include field based on the Dockerfile and config, if not explicitly set + if (!moduleConfig.include) { + moduleConfig.include = await containerHelpers.autoResolveIncludes(moduleConfig, log) + } + return { moduleConfig } } diff --git a/garden-service/src/plugins/container/helpers.ts b/garden-service/src/plugins/container/helpers.ts index 5c765e38a0..21e289a4aa 100644 --- a/garden-service/src/plugins/container/helpers.ts +++ b/garden-service/src/plugins/container/helpers.ts @@ -6,13 +6,21 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { join } from "path" +import { join, posix } from "path" +import { readFile, pathExists, lstat } from "fs-extra" import semver from "semver" +import { parse, CommandEntry } from "docker-file-parser" +import isGlob from "is-glob" import { ConfigurationError, RuntimeError } from "../../exceptions" import { splitFirst, spawn, splitLast } from "../../util/util" import { ModuleConfig } from "../../config/module" import { ContainerModule, ContainerRegistryConfig, defaultTag, defaultNamespace, ContainerModuleConfig } from "./config" import { Writable } from "stream" +import Bluebird from "bluebird" +import { flatten, uniq } from "lodash" +import { LogEntry } from "../../logger/log-entry" +import chalk from "chalk" +import isUrl from "is-url" export const DEFAULT_BUILD_TIMEOUT = 600 export const minDockerVersion = "17.07.0" @@ -287,6 +295,89 @@ const helpers = { getDockerfileSourcePath(config: ModuleConfig) { return getDockerfilePath(config.path, config.spec.dockerfile) }, + + /** + * Parses the Dockerfile in the module (if any) and returns a list of include patterns to apply to the module. + * Returns undefined if the whole module directory should be included, or if the Dockerfile cannot be parsed. + * Returns an empty list if there is no Dockerfile, and an `image` is set. + */ + async autoResolveIncludes(config: ContainerModuleConfig, log: LogEntry) { + const dockerfilePath = helpers.getDockerfileSourcePath(config) + + if (!(await pathExists(dockerfilePath))) { + // No Dockerfile, nothing to build, return empty list + return [] + } + + const dockerfile = await readFile(dockerfilePath) + let commands: CommandEntry[] = [] + + try { + commands = parse(dockerfile.toString()).filter( + (cmd) => (cmd.name === "ADD" || cmd.name === "COPY") && cmd.args && cmd.args.length > 0 + ) + } catch (err) { + log.warn(chalk.yellow(`Unable to parse Dockerfile ${dockerfilePath}: ${err.message}`)) + return undefined + } + + const paths: string[] = uniq( + flatten( + commands.map((cmd) => { + const args = cmd.args as string[] + if (args[0].startsWith("--chown")) { + // Ignore --chown args + return args.slice(1, -1) + } else if (args[0].startsWith("--from")) { + // Skip statements copying from another build stage + return [] + } else { + return args.slice(0, -1) + } + }) + // Ignore URLs + ).filter((path) => !isUrl(path)) + ) + + for (const path of paths) { + if (path === ".") { + // If any path is "." we need the full build context + return undefined + } else if (path.match(/(? { + const absPath = join(config.path, path) + + // Unescape escaped template strings + path = path.replace(/\\\$/g, "$") + + if (isGlob(path, { strict: false })) { + // Pass globs through directly + return path + } else if (await pathExists(absPath)) { + const stat = await lstat(absPath) + + if (stat.isDirectory()) { + // If it's a directory, we want to match everything in the directory + return posix.join(path, "**", "*") + } else { + // If it's a file, pass it through as-is + return path + } + } else { + // Pass the file through directly if it can't be found (an error will occur in the build later) + return path + } + }) + }, } export const containerHelpers = helpers diff --git a/garden-service/test/unit/src/plugins/container/container.ts b/garden-service/test/unit/src/plugins/container/container.ts index 3e08ec6d43..2adb12c689 100644 --- a/garden-service/test/unit/src/plugins/container/container.ts +++ b/garden-service/test/unit/src/plugins/container/container.ts @@ -184,6 +184,7 @@ describe("plugins.container", () => { build: { dependencies: [] }, apiVersion: "garden.io/v0", name: "module-a", + include: ["Dockerfile"], outputs: { "local-image-name": "module-a", "deployment-image-name": "module-a", diff --git a/garden-service/test/unit/src/plugins/container/helpers.ts b/garden-service/test/unit/src/plugins/container/helpers.ts index a6d0e18622..3634a88efa 100644 --- a/garden-service/test/unit/src/plugins/container/helpers.ts +++ b/garden-service/test/unit/src/plugins/container/helpers.ts @@ -2,6 +2,8 @@ import { expect } from "chai" import { resolve, join } from "path" import { cloneDeep } from "lodash" import td from "testdouble" +import tmp from "tmp-promise" +import { writeFile, mkdir } from "fs-extra" import { Garden } from "../../../../../src/garden" import { PluginContext } from "../../../../../src/plugin-context" @@ -12,6 +14,8 @@ import { ModuleConfig } from "../../../../../src/config/module" import { LogEntry } from "../../../../../src/logger/log-entry" import { ContainerModuleSpec, ContainerModuleConfig } from "../../../../../src/plugins/container/config" import { containerHelpers as helpers, DEFAULT_BUILD_TIMEOUT } from "../../../../../src/plugins/container/helpers" +import { DEFAULT_API_VERSION } from "../../../../../src/constants" +import { dedent } from "../../../../../src/util/string" describe("containerHelpers", () => { const projectRoot = resolve(dataDir, "test-project-container") @@ -407,4 +411,169 @@ describe("containerHelpers", () => { expect(await helpers.hasDockerfile(module)).to.be.false }) }) + + describe("autoResolveIncludes", () => { + let tmpDir: tmp.DirectoryResult + let config: ContainerModuleConfig + let dockerfilePath: string + + beforeEach(async () => { + tmpDir = await tmp.dir({ unsafeCleanup: true }) + dockerfilePath = join(tmpDir.path, "Dockerfile") + config = { + apiVersion: DEFAULT_API_VERSION, + type: "container", + allowPublish: false, + build: { dependencies: [] }, + name: "test", + path: tmpDir.path, + outputs: {}, + serviceConfigs: [], + spec: { + build: { dependencies: [], timeout: 999 }, + buildArgs: {}, + extraFlags: [], + services: [], + tasks: [], + tests: [], + }, + taskConfigs: [], + testConfigs: [], + } + }) + + afterEach(async () => { + await tmpDir.cleanup() + }) + + it("should return empty list if no Dockerfile is not found", async () => { + expect(await helpers.autoResolveIncludes(config, log)).to.eql([]) + }) + + it("should return all paths in COPY and ADD commands + the Dockerfile path", async () => { + await writeFile( + dockerfilePath, + dedent` + FROM foo + + ADD file-a . + COPY file-b file-c file-d d/ + + ENTRYPOINT bla + ` + ) + expect(await helpers.autoResolveIncludes(config, log)).to.eql([ + "file-a", + "file-b", + "file-c", + "file-d", + "Dockerfile", + ]) + }) + + it("should handle array style COPY and ADD commands", async () => { + await writeFile( + dockerfilePath, + dedent` + FROM foo + ADD ["file-a", "."] + COPY ["file-b", "file-c", "file-d", "d/"] + ` + ) + expect(await helpers.autoResolveIncludes(config, log)).to.eql([ + "file-a", + "file-b", + "file-c", + "file-d", + "Dockerfile", + ]) + }) + + it("should ignore URLs", async () => { + await writeFile( + dockerfilePath, + dedent` + FROM foo + ADD http://example.com/bla / + ADD file-* / + ` + ) + expect(await helpers.autoResolveIncludes(config, log)).to.eql(["file-*", "Dockerfile"]) + }) + + it("should pass globs through", async () => { + await writeFile( + dockerfilePath, + dedent` + FROM foo + ADD file-* / + ` + ) + expect(await helpers.autoResolveIncludes(config, log)).to.eql(["file-*", "Dockerfile"]) + }) + + it("should ignore --chown arguments", async () => { + await writeFile( + dockerfilePath, + dedent` + FROM foo + ADD --chown=bla file-a / + COPY --chown=bla file-b file-c / + ` + ) + expect(await helpers.autoResolveIncludes(config, log)).to.eql(["file-a", "file-b", "file-c", "Dockerfile"]) + }) + + it("should ignore COPY statements with a --from argument", async () => { + await writeFile( + dockerfilePath, + dedent` + FROM foo + ADD --chown=bla file-a / + COPY --from=bla /file-b file-c / + ` + ) + expect(await helpers.autoResolveIncludes(config, log)).to.eql(["file-a", "Dockerfile"]) + }) + + it("should ignore paths containing a template string", async () => { + await writeFile(dockerfilePath, "FROM foo\nADD file-a /\nCOPY file-${foo} file-c /") + expect(await helpers.autoResolveIncludes(config, log)).to.be.undefined + }) + + it("should ignore paths containing a naked template string", async () => { + await writeFile(dockerfilePath, "FROM foo\nADD file-a /\nCOPY file-$foo file-c /") + expect(await helpers.autoResolveIncludes(config, log)).to.be.undefined + }) + + it("should pass through paths containing an escaped template string", async () => { + await writeFile(dockerfilePath, "FROM foo\nADD file-a /\nCOPY file-\\$foo file-c /") + expect(await helpers.autoResolveIncludes(config, log)).to.eql(["file-a", "file-$foo", "file-c", "Dockerfile"]) + }) + + it("should return if any source path is '.'", async () => { + await writeFile( + dockerfilePath, + dedent` + FROM foo + ADD . . + COPY file-b file-c file-d d/ + ` + ) + expect(await helpers.autoResolveIncludes(config, log)).to.be.undefined + }) + + it("should create a glob for every directory path", async () => { + await mkdir(join(tmpDir.path, "dir-a")) + await writeFile( + dockerfilePath, + dedent` + FROM foo + ADD dir-a . + COPY file-b d/ + ` + ) + expect(await helpers.autoResolveIncludes(config, log)).to.eql(["dir-a/**/*", "file-b", "Dockerfile"]) + }) + }) })