fix(k8s): bad handling of directory paths for artifact sources

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).

core/src/plugins/kubernetes/run.ts

@@ -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"
@@ -32,16 +31,16 @@ import { KubernetesResource, KubernetesPod } from "./types"
 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
@@ -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()
   }

core/test/data/test-projects/container/simple/garden.yml

@@ -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"]
@@ -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"]