Skip to content

Commit

Permalink
fix(k8s): bad handling of directory paths for artifact sources
Browse files Browse the repository at this point in the history 
Fixes #1777

Also makes the copying process more efficient by putting it all in one
copy command, and also compressing the files (which we clumsily
neglected to do previously).
  • Loading branch information
@edvald
edvald committed Aug 16, 2021
1 parent 43e7cc8 commit 92ee09a
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 96 deletions.
143 changes: 72 additions & 71 deletions core/src/plugins/kubernetes/run.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

import { resolve } from "path"
import tar from "tar"
import tmp from "tmp-promise"
import { cloneDeep, omit, pick } from "lodash"
Expand All @@ -32,16 +31,16 @@ import { KubernetesResource, KubernetesPod, KubernetesServerResource } from "./t
import { RunModuleParams } from "../../types/plugin/module/runModule"
import { ContainerEnvVars, ContainerResourcesSpec, ContainerVolumeSpec } from "../container/config"
import { prepareEnvVars, makePodName } from "./util"
import { deline } from "../../util/string"
import { deline, randomString } from "../../util/string"
import { ArtifactSpec } from "../../config/validation"
import cpy from "cpy"
import { prepareImagePullSecrets } from "./secrets"
import { configureVolumes } from "./container/deployment"
import { PluginContext } from "../../plugin-context"
import { waitForResources, ResourceStatus } from "./status/status"
import { RuntimeContext } from "../../runtime-context"
import { getResourceRequirements } from "./container/util"
import { KUBECTL_DEFAULT_TIMEOUT } from "./kubectl"
import { copy } from "fs-extra"

// Default timeout for individual run/exec operations
const defaultTimeout = 600
Expand Down Expand Up @@ -576,79 +575,81 @@ async function runWithArtifacts({
}
}

// Copy the artifacts
await Promise.all(
artifacts.map(async (artifact) => {
const tmpDir = await tmp.dir({ unsafeCleanup: true })
// Remove leading slash (which is required in the schema)
const sourcePath = artifact.source.slice(1)
const targetPath = resolve(artifactsPath, artifact.target || ".")

const tarCmd = [
"tar",
"-c", // create an archive
"-f",
"-", // pipe to stdout
// Files to match. The .DS_Store file is a trick to avoid errors when no files are matched. The file is
// ignored later when copying from the temp directory. See https://github.com/sindresorhus/cpy#ignorejunk
`$(ls ${sourcePath} 2>/dev/null) /tmp/.DS_Store`,
// Fix issue https://github.com/garden-io/garden/issues/2445
"| cat",
]

try {
await new Promise<void>((_resolve, reject) => {
// Create an extractor to receive the tarball we will stream from the container
// and extract to the artifacts directory.
let done = 0

const extractor = tar.x({
cwd: tmpDir.path,
strict: true,
onentry: (entry) => log.debug("tar: got file " + entry.path),
})
// TODO: only interpret target as directory if it ends with a slash (breaking change, so slated for 0.13)
const directoriesToCreate = artifacts.map((a) => a.target).filter((target) => !!target && target !== ".")
const tmpPath = "/tmp/.garden-artifacts-" + randomString(8)

// This script will
// 1. Create temp directory in the container
// 2. Create directories for each target, as necessary
// 3. Recursively (and silently) copy all specified artifact files/directories into the temp directory
// 4. Tarball the directory and pipe to stdout
// TODO: escape the user paths somehow?
const tarScript = `
rm -rf ${tmpPath} >/dev/null || true
mkdir -p ${tmpPath}
cd ${tmpPath}
touch .garden-placeholder
${directoriesToCreate.map((target) => `mkdir -p ${target}`).join("\n")}
${artifacts.map(({ source, target }) => `cp -r ${source} ${target || "."} >/dev/null || true`).join("\n")}
tar -c -z -f - . | cat
rm -rf ${tmpPath} >/dev/null || true
`

extractor.on("end", () => {
// Need to make sure both processes are complete before resolving (may happen in either order)
done++
done === 2 && _resolve()
})
extractor.on("error", (err) => {
reject(err)
})
// Copy the artifacts
const tmpDir = await tmp.dir({ unsafeCleanup: true })

// Tarball the requested files and stream to the above extractor.
runner
.exec({
command: ["sh", "-c", "cd / && touch /tmp/.DS_Store && " + tarCmd.join(" ")],
containerName: mainContainerName,
log,
stdout: extractor,
timeoutSec,
buffer: false,
})
.then(() => {
// Need to make sure both processes are complete before resolving (may happen in either order)
done++
done === 2 && _resolve()
})
.catch(reject)
try {
await new Promise<void>((_resolve, reject) => {
// Create an extractor to receive the tarball we will stream from the container
// and extract to the artifacts directory.
let done = 0

const extractor = tar.x({
cwd: tmpDir.path,
strict: true,
onentry: (entry) => log.debug("tar: got entry " + entry.path),
})

extractor.on("end", () => {
// Need to make sure both processes are complete before resolving (may happen in either order)
done++
done === 2 && _resolve()
})
extractor.on("error", (err) => {
reject(err)
})

// Tarball the requested files and stream to the above extractor.
runner
.exec({
command: ["sh", "-c", tarScript],
containerName: mainContainerName,
log,
stdout: extractor,
timeoutSec,
buffer: false,
})
.then(() => {
// Need to make sure both processes are complete before resolving (may happen in either order)
done++
done === 2 && _resolve()
})
.catch(reject)
})

// Copy the resulting files to the artifacts directory
try {
await cpy("**/*", targetPath, { cwd: tmpDir.path, ignoreJunk: true })
} catch (err) {
// Ignore error thrown when the directory is empty
if (err.name !== "CpyError" || !err.message.includes("the file doesn't exist")) {
throw err
}
}
} finally {
await tmpDir.cleanup()
// Copy the resulting files to the artifacts directory
try {
await copy(tmpDir.path, artifactsPath, { filter: (f) => !f.endsWith(".garden-placeholder") })
} catch (err) {
// Ignore error thrown when the directory is empty
if (err.name !== "CpyError" || !err.message.includes("the file doesn't exist")) {
throw err
}
})
)
}
} finally {
await tmpDir.cleanup()
}
} finally {
await runner.stop()
}
Expand Down
8 changes: 3 additions & 5 deletions core/test/data/test-projects/container/simple/garden.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ kind: Module
name: simple
type: container
image: busybox:1.31.1

services:
- name: echo-service
command: [sh, -c, "echo ok"]
- name: env-service
command: [sh, -c, "echo $ENV_VAR"]
env:
ENV_VAR: foo

tasks:
- name: echo-task
command: [sh, -c, "echo ok"]
Expand All @@ -30,11 +32,7 @@ tasks:
- source: /task.*
target: subdir
- source: /tasks/*
- name: dir-task
command: [sh, -c, "mkdir -p /report && touch /report/output.txt && echo ok"]
artifacts:
- source: /report/*
target: my-task-report

tests:
- name: echo-test
command: [sh, -c, "echo ok"]
Expand Down
60 changes: 41 additions & 19 deletions core/test/integ/src/plugins/kubernetes/run.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -929,6 +929,8 @@ describe("kubernetes Pod runner functions", () => {
})

describe("runAndCopy", () => {
const image = "busybox:1.31.1"

let tmpDir: tmp.DirectoryResult

beforeEach(async () => {
Expand All @@ -941,7 +943,6 @@ describe("kubernetes Pod runner functions", () => {

it("should run a basic module", async () => {
const module = graph.getModule("simple")
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

const result = await runAndCopy({
ctx: await garden.getPluginContext(provider),
Expand All @@ -961,7 +962,6 @@ describe("kubernetes Pod runner functions", () => {

it("should clean up the created container", async () => {
const module = graph.getModule("simple")
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)
const podName = makePodName("test", module.name)

await runAndCopy({
Expand All @@ -987,7 +987,6 @@ describe("kubernetes Pod runner functions", () => {
it("should return with success=false when command exceeds timeout", async () => {
const task = graph.getTask("artifacts-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

const result = await runAndCopy({
ctx: await garden.getPluginContext(provider),
Expand All @@ -1012,7 +1011,6 @@ describe("kubernetes Pod runner functions", () => {
it("should copy artifacts out of the container", async () => {
const task = graph.getTask("artifacts-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

const result = await runAndCopy({
ctx: await garden.getPluginContext(provider),
Expand All @@ -1037,7 +1035,7 @@ describe("kubernetes Pod runner functions", () => {
it("should clean up the created Pod", async () => {
const task = graph.getTask("artifacts-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

const podName = makePodName("test", module.name)

await runAndCopy({
Expand Down Expand Up @@ -1065,7 +1063,6 @@ describe("kubernetes Pod runner functions", () => {
it("should handle globs when copying artifacts out of the container", async () => {
const task = graph.getTask("globs-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

await runAndCopy({
ctx: await garden.getPluginContext(provider),
Expand All @@ -1089,7 +1086,6 @@ describe("kubernetes Pod runner functions", () => {
it("should not throw when an artifact is missing", async () => {
const task = graph.getTask("artifacts-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

await runAndCopy({
ctx: await garden.getPluginContext(provider),
Expand All @@ -1108,20 +1104,23 @@ describe("kubernetes Pod runner functions", () => {
})

it("should correctly copy a whole directory", async () => {
const task = graph.getTask("dir-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)
const module = graph.getModule("simple")

await runAndCopy({
ctx: await garden.getPluginContext(provider),
log: garden.log,
command: task.spec.command,
command: ["sh", "-c", "mkdir -p /report && touch /report/output.txt && echo ok"],
args: [],
interactive: false,
module,
namespace,
runtimeContext: { envVars: {}, dependencies: [] },
artifacts: task.spec.artifacts,
artifacts: [
{
source: "/report/*",
target: "my-task-report",
},
],
artifactsPath: tmpDir.path,
image,
version: module.version.versionString,
Expand All @@ -1131,10 +1130,35 @@ describe("kubernetes Pod runner functions", () => {
expect(await pathExists(join(tmpDir.path, "my-task-report", "output.txt"))).to.be.true
})

it("should correctly copy a whole directory without setting a wildcard or target", async () => {
const module = graph.getModule("simple")

await runAndCopy({
ctx: await garden.getPluginContext(provider),
log: garden.log,
command: ["sh", "-c", "mkdir -p /report && touch /report/output.txt && echo ok"],
args: [],
interactive: false,
module,
namespace,
runtimeContext: { envVars: {}, dependencies: [] },
artifacts: [
{
source: "/report",
},
],
artifactsPath: tmpDir.path,
image,
version: module.version.versionString,
})

expect(await pathExists(join(tmpDir.path, "report"))).to.be.true
expect(await pathExists(join(tmpDir.path, "report", "output.txt"))).to.be.true
})

it("should return with logs and success=false when command exceeds timeout", async () => {
const task = graph.getTask("artifacts-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

const result = await runAndCopy({
ctx: await garden.getPluginContext(provider),
Expand All @@ -1159,7 +1183,6 @@ describe("kubernetes Pod runner functions", () => {
it("should copy artifacts out of the container even when task times out", async () => {
const task = graph.getTask("artifacts-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

const result = await runAndCopy({
ctx: await garden.getPluginContext(provider),
Expand All @@ -1185,7 +1208,7 @@ describe("kubernetes Pod runner functions", () => {
it("should throw when container doesn't contain sh", async () => {
const task = graph.getTask("missing-sh-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)
const _image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

const actions = await garden.getActionRouter()
await garden.buildStaging.syncFromSrc(module, garden.log)
Expand All @@ -1208,7 +1231,7 @@ describe("kubernetes Pod runner functions", () => {
artifacts: task.spec.artifacts,
artifactsPath: tmpDir.path,
description: "Foo",
image,
image: _image,
timeout: 20000,
stdout: process.stdout,
stderr: process.stderr,
Expand All @@ -1226,7 +1249,7 @@ describe("kubernetes Pod runner functions", () => {
it("should throw when container doesn't contain tar", async () => {
const task = graph.getTask("missing-tar-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)
const _image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

const actions = await garden.getActionRouter()
await garden.buildStaging.syncFromSrc(module, garden.log)
Expand All @@ -1249,7 +1272,7 @@ describe("kubernetes Pod runner functions", () => {
artifacts: task.spec.artifacts,
artifactsPath: tmpDir.path,
description: "Foo",
image,
image: _image,
timeout: 20000,
stdout: process.stdout,
stderr: process.stderr,
Expand All @@ -1267,7 +1290,6 @@ describe("kubernetes Pod runner functions", () => {
it("should throw when no command is specified", async () => {
const task = graph.getTask("missing-tar-task")
const module = task.module
const image = containerHelpers.getDeploymentImageId(module, module.version, provider.config.deploymentRegistry)

await expectError(
async () =>
Expand Down
Loading

0 comments on commit 92ee09a

Please sign in to comment.