improvement(core): allow relative symlinks within module root for builds

This addresses a limitation where we silently ignored all symlinks.
Docker supports relative symlinks within the build context, and we
should support those as well.

We now also log (on verbose level) when we filter out symlinks.

Fixes #1421

garden-service/src/build-dir.ts

@@ -161,7 +161,25 @@ export class BuildDir {
     sourcePath = stripWildcard(sourcePath)
     destinationPath = stripWildcard(destinationPath)
 
-    const syncOpts = ["-rptgo", "--ignore-missing-args", "--temp-dir", normalizeLocalRsyncPath(tmpDir)]
+    const syncOpts = [
+      "--recursive",
+      // Preserve modification times
+      "--times",
+      // Preserve owner + group
+      "--owner",
+      "--group",
+      // Copy permissions
+      "--perms",
+      // Copy symlinks
+      "--links",
+      // Only allow links that point within the copied tree
+      "--safe-links",
+      // Ignore missing files in file list
+      "--ignore-missing-args",
+      // Set a temp directory outside of the target directory to avoid potential conflicts
+      "--temp-dir",
+      normalizeLocalRsyncPath(tmpDir),
+    ]
 
     let logMsg =
       `Syncing ${module.version.files.length} files from ` +

garden-service/src/plugins/kubernetes/container/build.ts

@@ -140,7 +140,22 @@ const remoteBuild: BuildHandler = async (params) => {
   // https://stackoverflow.com/questions/1636889/rsync-how-can-i-configure-it-to-create-target-directory-on-server
   let src = normalizeLocalRsyncPath(`${buildRoot}`) + `/./${module.name}/`
   const destination = `rsync://localhost:${syncFwd.localPort}/volume/${ctx.workingCopyId}/`
-  const syncArgs = ["-vrpztgo", "--relative", "--delete", "--temp-dir", "/tmp", src, destination]
+  const syncArgs = [
+    "--recursive",
+    "--relative",
+    // Copy symlinks (Note: These are sanitized while syncing to the build staging dir)
+    "--links",
+    // Preserve permissions
+    "--perms",
+    // Preserve modification times
+    "--times",
+    "--compress",
+    "--delete",
+    "--temp-dir",
+    "/tmp",
+    src,
+    destination,
+  ]
 
   log.debug(`Syncing from ${src} to ${destination}`)
 

garden-service/src/util/fs.ts

@@ -155,7 +155,7 @@ export async function findConfigPathsInPath({
   log: LogEntry
 }) {
   // TODO: we could make this lighter/faster using streaming
-  const files = await vcs.getFiles({ path: dir, include, exclude: exclude || [], log })
+  const files = await vcs.getFiles({ path: dir, pathDescription: "project root", include, exclude: exclude || [], log })
   return files.map((f) => f.path).filter((f) => isConfigFilename(basename(f)))
 }
 

garden-service/src/vcs/git.ts

@@ -6,9 +6,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-import { join, resolve, relative } from "path"
+import { join, resolve, relative, isAbsolute } from "path"
 import { flatten } from "lodash"
-import { ensureDir, pathExists, stat, createReadStream } from "fs-extra"
+import { ensureDir, pathExists, createReadStream, Stats, realpath, readlink, lstat } from "fs-extra"
 import { PassThrough } from "stream"
 import hasha from "hasha"
 import split2 = require("split2")
@@ -104,7 +104,7 @@ export class GitHandler extends VcsHandler {
    * Returns a list of files, along with file hashes, under the given path, taking into account the configured
    * .ignore files, and the specified include/exclude filters.
    */
-  async getFiles({ log, path, include, exclude }: GetFilesParams): Promise<VcsFile[]> {
+  async getFiles({ log, path, pathDescription, include, exclude }: GetFilesParams): Promise<VcsFile[]> {
     const git = this.gitCli(log, path)
     const gitRoot = await this.getRepoRoot(log, path)
 
@@ -212,9 +212,9 @@ export class GitHandler extends VcsHandler {
         if (submodulePaths.includes(f.path)) {
           // This path is a submodule, so we recursively call getFiles for that path again.
           // Note: We apply include/exclude filters after listing files from submodule
-          return (await this.getFiles({ log, path: f.path, exclude: [] })).filter((submoduleFile) =>
-            matchPath(relative(path, submoduleFile.path), include, exclude)
-          )
+          return (
+            await this.getFiles({ log, path: f.path, pathDescription: "submodule", exclude: [] })
+          ).filter((submoduleFile) => matchPath(relative(path, submoduleFile.path), include, exclude))
         } else {
           return [f]
         }
@@ -224,26 +224,52 @@ export class GitHandler extends VcsHandler {
     // Make sure we have a fresh hash for each file
     return Bluebird.map(withSubmodules, async (f) => {
       const resolvedPath = resolve(path, f.path)
-      if (!f.hash || modified.has(resolvedPath)) {
-        // If we can't compute the hash, i.e. the file is gone, we filter it out below
-        let hash = ""
-        try {
-          // "git ls-files" returns a symlink even if it points to a directory.
-          // We filter symlinked directories out, since hashObject() will fail to
-          // process them.
-          if (!(await stat(resolvedPath)).isDirectory()) {
-            hash = (await this.hashObject(resolvedPath)) || ""
-          }
-        } catch (err) {
-          // 128 = File no longer exists
-          if (err.exitCode !== 128 && err.code !== "ENOENT") {
-            throw err
+      let output = { path: resolvedPath, hash: f.hash || "" }
+      let stats: Stats
+
+      try {
+        stats = await lstat(resolvedPath)
+      } catch (err) {
+        // 128 = File no longer exists
+        if (err.exitCode === 128 || err.code === "ENOENT") {
+          // If the file is gone, we filter it out below
+          return { path: resolvedPath, hash: "" }
+        } else {
+          throw err
+        }
+      }
+
+      // We need to special-case handling of symlinks. We disallow any "unsafe" symlinks, i.e. any ones that may
+      // link outside of `path` (which is usually a project or module root). The `hashObject` method also special-cases
+      // symlinks, to match git's hashing behavior (which is to hash the link itself, and not what the link points to).
+      if (stats.isSymbolicLink()) {
+        const target = await readlink(resolvedPath)
+
+        // Make sure symlink is relative and points within `path`
+        if (isAbsolute(target)) {
+          log.verbose(`Ignoring symlink with absolute target at ${resolvedPath}`)
+          output.hash = ""
+          return output
+        } else if (target.startsWith("..")) {
+          const realTarget = await realpath(resolvedPath)
+          const relPath = relative(path, realTarget)
+
+          if (relPath.startsWith("..")) {
+            log.verbose(`Ignoring symlink pointing outside of ${pathDescription} at ${resolvedPath}`)
+            output.hash = ""
+            return output
           }
         }
-        return { path: resolvedPath, hash }
-      } else {
-        return { path: resolvedPath, hash: f.hash }
       }
+
+      if (output.hash === "" || modified.has(resolvedPath)) {
+        // Don't attempt to hash directories. Directories will by extension be filtered out of the list.
+        if (!stats.isDirectory()) {
+          output.hash = (await this.hashObject(stats, resolvedPath)) || ""
+        }
+      }
+
+      return output
     }).filter((f) => f.hash !== "")
   }
 
@@ -325,12 +351,20 @@ export class GitHandler extends VcsHandler {
   /**
    * Replicates the `git hash-object` behavior. See https://stackoverflow.com/a/5290484/3290965
    */
-  async hashObject(path: string) {
-    const info = await stat(path)
+  async hashObject(stats: Stats, path: string) {
     const stream = new PassThrough()
     const output = hasha.fromStream(stream, { algorithm: "sha1" })
-    stream.push(`blob ${info.size}\0`)
-    createReadStream(path).pipe(stream)
+    stream.push(`blob ${stats.size}\0`)
+
+    if (stats.isSymbolicLink()) {
+      // For symlinks, we follow git's behavior, which is to hash the link itself (i.e. the path it contains) as
+      // opposed to the file/directory that it points to.
+      stream.push(await readlink(path))
+      stream.end()
+    } else {
+      createReadStream(path).pipe(stream)
+    }
+
     return output
   }
 

garden-service/src/vcs/vcs.ts

@@ -70,6 +70,7 @@ export const moduleVersionSchema = joi.object().keys({
 export interface GetFilesParams {
   log: LogEntry
   path: string
+  pathDescription?: string
   include?: string[]
   exclude?: string[]
 }
@@ -101,6 +102,7 @@ export abstract class VcsHandler {
     let files = await this.getFiles({
       log,
       path: moduleConfig.path,
+      pathDescription: "module root",
       include: moduleConfig.include,
       exclude: moduleConfig.exclude,
     })

garden-service/test/data/test-projects/build-dir/symlink-absolute/garden.yml

@@ -0,0 +1,3 @@
+kind: Module
+type: exec
+name: symlink-absolute

garden-service/test/data/test-projects/build-dir/symlink-absolute/symlink.txt

@@ -0,0 +1 @@
+/tmp

garden-service/test/data/test-projects/build-dir/symlink-outside-module/garden.yml

@@ -0,0 +1,3 @@
+kind: Module
+type: exec
+name: symlink-outside-module

garden-service/test/data/test-projects/build-dir/symlink-outside-module/symlink.txt

@@ -0,0 +1 @@
+../garden.yml

garden-service/test/data/test-projects/build-dir/symlink-within-module/foo.txt

@@ -0,0 +1 @@
+something

garden-service/test/data/test-projects/build-dir/symlink-within-module/garden.yml

@@ -0,0 +1,3 @@
+kind: Module
+type: exec
+name: symlink-within-module

garden-service/test/data/test-projects/build-dir/symlink-within-module/nested/symlink.txt

@@ -0,0 +1 @@
+../foo.txt

garden-service/test/data/test-projects/build-dir/symlink-within-module/symlink.txt

@@ -0,0 +1 @@
+foo.txt

garden-service/test/unit/src/build-dir.ts

@@ -8,7 +8,7 @@ import { getConfigFilePath } from "../../../src/util/fs"
 import { Garden } from "../../../src/garden"
 
 /*
-  Module dependency diagram for test-project-build-products
+  Module dependency diagram for build-dir test project
 
     a   b
      \ /
@@ -17,7 +17,7 @@ import { Garden } from "../../../src/garden"
           f
  */
 
-const projectRoot = join(dataDir, "test-project-build-products")
+const projectRoot = join(dataDir, "test-projects", "build-dir")
 
 const makeGarden = async () => {
   return await makeTestGarden(projectRoot)
@@ -102,6 +102,37 @@ describe("BuildDir", () => {
 
       expect(await pathExists(deleteMe)).to.be.false
     })
+
+    it("should sync symlinks that point within the module root", async () => {
+      const graph = await garden.getConfigGraph(garden.log)
+      const module = await graph.getModule("symlink-within-module")
+
+      await garden.buildDir.syncFromSrc(module, garden.log)
+
+      const buildDir = await garden.buildDir.buildPath(module)
+      expect(await pathExists(join(buildDir, "symlink.txt"))).to.be.true
+      expect(await pathExists(join(buildDir, "nested", "symlink.txt"))).to.be.true
+    })
+
+    it("should not sync symlinks that point outside the module root", async () => {
+      const graph = await garden.getConfigGraph(garden.log)
+      const module = await graph.getModule("symlink-outside-module")
+
+      await garden.buildDir.syncFromSrc(module, garden.log)
+
+      const buildDir = await garden.buildDir.buildPath(module)
+      expect(await pathExists(join(buildDir, "symlink.txt"))).to.be.false
+    })
+
+    it("should not sync absolute symlinks", async () => {
+      const graph = await garden.getConfigGraph(garden.log)
+      const module = await graph.getModule("symlink-absolute")
+
+      await garden.buildDir.syncFromSrc(module, garden.log)
+
+      const buildDir = await garden.buildDir.buildPath(module)
+      expect(await pathExists(join(buildDir, "symlink.txt"))).to.be.false
+    })
   })
 
   it("should sync dependency products to their specified destinations", async () => {

garden-service/test/unit/src/vcs/git.ts

@@ -10,7 +10,7 @@ import execa = require("execa")
 import { expect } from "chai"
 import tmp from "tmp-promise"
 import uuid from "uuid"
-import { createFile, writeFile, realpath, mkdir, remove, symlink } from "fs-extra"
+import { createFile, writeFile, realpath, mkdir, remove, symlink, ensureSymlink, lstat } from "fs-extra"
 import { join, resolve, basename, relative } from "path"
 
 import { expectError, makeTestGardenA } from "../../../helpers"
@@ -326,22 +326,44 @@ describe("GitHandler", () => {
       expect(files).to.eql([])
     })
 
-    it("should exclude an untracked symlink to a directory", async () => {
-      const tmpDir2 = await tmp.dir({ unsafeCleanup: true })
-      const tmpPathB = await realpath(tmpDir2.path)
+    it("should include a relative symlink within the path", async () => {
+      const fileName = "foo"
+      const filePath = resolve(tmpPath, fileName)
+      const symlinkPath = resolve(tmpPath, "symlink")
 
-      const name = "a-symlink-to-a-directory"
-      const path = resolve(tmpPath, name)
+      await createFile(filePath)
+      await symlink(fileName, symlinkPath)
 
-      await symlink(tmpPathB, path)
+      const files = (await handler.getFiles({ path: tmpPath, exclude: [], log })).map((f) => f.path)
+      expect(files).to.eql([filePath, symlinkPath])
+    })
 
-      const files = (await handler.getFiles({ path: tmpPath, exclude: [], log })).filter(
-        (f) => !f.path.includes(defaultIgnoreFilename)
-      )
+    it("should exclude a relative symlink that points outside the path", async () => {
+      const subPath = resolve(tmpPath, "subdir")
+
+      const fileName = "foo"
+      const filePath = resolve(tmpPath, fileName)
+      const symlinkPath = resolve(subPath, "symlink")
 
+      await createFile(filePath)
+      await ensureSymlink(join("..", fileName), symlinkPath)
+
+      const files = (await handler.getFiles({ path: subPath, exclude: [], log })).map((f) => f.path)
       expect(files).to.eql([])
     })
 
+    it("should exclude an absolute symlink that points inside the path", async () => {
+      const fileName = "foo"
+      const filePath = resolve(tmpPath, fileName)
+      const symlinkPath = resolve(tmpPath, "symlink")
+
+      await createFile(filePath)
+      await symlink(filePath, symlinkPath)
+
+      const files = (await handler.getFiles({ path: tmpPath, exclude: [], log })).map((f) => f.path)
+      expect(files).to.eql([filePath])
+    })
+
     context("path contains a submodule", () => {
       let submodule: tmp.DirectoryResult
       let submodulePath: string
@@ -451,10 +473,42 @@ describe("GitHandler", () => {
     it("should return the same result as `git hash-object` for a file", async () => {
       const path = resolve(tmpPath, "foo.txt")
       await createFile(path)
+      await writeFile(path, "iogjeiojgeowigjewoijoeiw")
+      const stats = await lstat(path)
 
       const expected = (await git("hash-object", path))[0]
 
-      expect(await handler.hashObject(path)).to.equal(expected)
+      expect(await handler.hashObject(stats, path)).to.equal(expected)
+    })
+
+    it("should return the same result as `git ls-files` for a file", async () => {
+      const path = resolve(tmpPath, "foo.txt")
+      await createFile(path)
+      await writeFile(path, "iogjeiojgeowigjewoijoeiw")
+      const stats = await lstat(path)
+      await git("add", path)
+
+      const files = (await git("ls-files", "-s", path))[0]
+      const expected = files.split(" ")[1]
+
+      expect(await handler.hashObject(stats, path)).to.equal(expected)
+    })
+
+    it("should return the same result as `git ls-files` for a symlink", async () => {
+      const filePath = resolve(tmpPath, "foo")
+      const symlinkPath = resolve(tmpPath, "bar")
+      await createFile(filePath)
+      await writeFile(filePath, "kfgjdslgjaslj")
+
+      await symlink("foo", symlinkPath)
+      await git("add", symlinkPath)
+
+      const stats = await lstat(symlinkPath)
+
+      const files = (await git("ls-files", "-s", symlinkPath))[0]
+      const expected = files.split(" ")[1]
+
+      expect(await handler.hashObject(stats, symlinkPath)).to.equal(expected)
     })
   })