Improve Docker dependencies code

Fixes GoogleContainerTools#386

Author: dgageot

pkg/skaffold/bazel/bazel.go

@@ -19,7 +19,6 @@ package bazel
 import (
 	"fmt"
 	"os/exec"
-	"path/filepath"
 	"strings"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
@@ -52,9 +51,7 @@ func (*BazelDependencyResolver) GetDependencies(a *v1alpha2.Artifact) ([]string,
 			continue
 		}
 
-		dep := filepath.Join(a.Workspace, depToPath(l))
-
-		deps = append(deps, dep)
+		deps = append(deps, depToPath(l))
 	}
 	return deps, nil
 }

pkg/skaffold/build/deps.go

@@ -18,6 +18,7 @@ package build
 
 import (
 	"fmt"
+	"path/filepath"
 	"sort"
 	"strings"
 
@@ -119,7 +120,7 @@ func pathsForArtifact(a *v1alpha2.Artifact) ([]string, error) {
 			logrus.Debugf("Ignoring %s for artifact dependencies", dep)
 			continue
 		}
-		filteredDeps = append(filteredDeps, dep)
+		filteredDeps = append(filteredDeps, filepath.Join(a.Workspace, dep))
 	}
 	return filteredDeps, nil
 }

pkg/skaffold/docker/context.go

@@ -25,12 +25,8 @@ import (
 	"github.com/pkg/errors"
 )
 
-func contextTarPaths(dockerfilePath string, context string) ([]string, error) {
-	return GetDockerfileDependencies(dockerfilePath, context)
-}
-
 func CreateDockerTarContext(w io.Writer, dockerfilePath, context string) error {
-	paths, err := contextTarPaths(dockerfilePath, context)
+	paths, err := GetDockerfileDependencies(dockerfilePath, context)
 	if err != nil {
 		return errors.Wrap(err, "getting relative tar paths")
 	}
@@ -41,7 +37,7 @@ func CreateDockerTarContext(w io.Writer, dockerfilePath, context string) error {
 }
 
 func CreateDockerTarGzContext(w io.Writer, dockerfilePath, context string) error {
-	paths, err := contextTarPaths(dockerfilePath, context)
+	paths, err := GetDockerfileDependencies(dockerfilePath, context)
 	if err != nil {
 		return errors.Wrap(err, "getting relative tar paths")
 	}

pkg/skaffold/docker/parse.go

@@ -22,14 +22,12 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path"
 	"path/filepath"
 	"sort"
 	"strings"
 	"sync"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/google/go-containerregistry/v1"
 
 	"github.com/docker/docker/builder/dockerignore"
@@ -57,11 +55,9 @@ func (d *DockerfileDepResolver) GetDependencies(a *v1alpha2.Artifact) ([]string,
 	return GetDockerfileDependencies(a.DockerArtifact.DockerfilePath, a.Workspace)
 }
 
-// GetDockerfileDependencies parses a dockerfile and returns the full paths
-// of all the source files that the resulting docker image depends on.
-func GetDockerfileDependencies(dockerfilePath, workspace string) ([]string, error) {
+func readDockerfile(workspace, dockerfilePath string) ([]string, error) {
 	path := filepath.Join(workspace, dockerfilePath)
-	f, err := util.Fs.Open(path)
+	f, err := os.Open(path)
 	if err != nil {
 		return nil, errors.Wrapf(err, "opening dockerfile: %s", path)
 	}
@@ -91,7 +87,7 @@ func GetDockerfileDependencies(dockerfilePath, workspace string) ([]string, erro
 		for _, value := range r.AST.Children {
 			switch value.Value {
 			case add, copy:
-				processCopy(workspace, value, depMap, envs)
+				processCopy(value, depMap, envs)
 			case env:
 				envs[value.Next.Value] = value.Next.Next.Value
 			}
@@ -109,30 +105,86 @@ func GetDockerfileDependencies(dockerfilePath, workspace string) ([]string, erro
 
 	dispatchInstructions(res)
 
-	deps := []string{}
+	var deps []string
 	for dep := range depMap {
 		deps = append(deps, dep)
 	}
 	logrus.Infof("Found dependencies for dockerfile %s", deps)
 
-	expandedDeps, err := util.ExpandPaths(workspace, deps)
+	return deps, nil
+}
+
+func GetDockerfileDependencies(dockerfilePath, workspace string) ([]string, error) {
+	deps, err := readDockerfile(workspace, dockerfilePath)
 	if err != nil {
-		return nil, errors.Wrap(err, "expanding dockerfile paths")
+		return nil, err
 	}
-	logrus.Infof("deps %s", expandedDeps)
 
-	if !util.StrSliceContains(expandedDeps, path) {
-		expandedDeps = append(expandedDeps, path)
+	// Read patterns to ignore
+	var excludes []string
+	dockerignorePath := filepath.Join(workspace, ".dockerignore")
+	if _, err := os.Stat(dockerignorePath); !os.IsNotExist(err) {
+		r, err := os.Open(dockerignorePath)
+		if err != nil {
+			return nil, err
+		}
+		defer r.Close()
+
+		excludes, err = dockerignore.ReadAll(r)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// Walk the workspace
+	files := make(map[string]bool)
+	for _, dep := range deps {
+		filepath.Walk(filepath.Join(workspace, dep), func(fpath string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+
+			relPath, err := filepath.Rel(workspace, fpath)
+			if err != nil {
+				return err
+			}
+
+			ignored, err := fileutils.Matches(relPath, excludes)
+			if err != nil {
+				return err
+			}
+
+			if info.IsDir() && ignored {
+				return filepath.SkipDir
+			}
+
+			if !info.IsDir() && !ignored {
+				files[relPath] = true
+			}
+
+			return nil
+		})
 	}
 
-	// Look for .dockerignore.
-	ignorePath := filepath.Join(workspace, ".dockerignore")
-	filteredDeps, err := ApplyDockerIgnore(expandedDeps, ignorePath)
+	// Add dockerfile?
+	m, err := fileutils.Matches(dockerfilePath, excludes)
 	if err != nil {
-		return nil, errors.Wrap(err, "applying dockerignore")
+		return nil, err
+	}
+	if !m {
+		files[dockerfilePath] = true
+	}
+
+	// Ignore .dockerignore
+	delete(files, ".dockerignore")
+
+	var dependencies []string
+	for file := range files {
+		dependencies = append(dependencies, file)
 	}
+	sort.Strings(dependencies)
 
-	return filteredDeps, nil
+	return dependencies, nil
 }
 
 func PortsFromDockerfile(r io.Reader) ([]string, error) {
@@ -242,7 +294,7 @@ func retrieveRemoteConfig(identifier string) (*v1.ConfigFile, error) {
 	return img.ConfigFile()
 }
 
-func processCopy(workspace string, value *parser.Node, paths map[string]struct{}, envs map[string]string) error {
+func processCopy(value *parser.Node, paths map[string]struct{}, envs map[string]string) error {
 	slex := shell.NewLex('\\')
 	for {
 		// Skip last node, since it is the destination, and stop if we arrive at a comment
@@ -259,8 +311,7 @@ func processCopy(workspace string, value *parser.Node, paths map[string]struct{}
 			return nil
 		}
 		if !strings.HasPrefix(src, "http://") && !strings.HasPrefix(src, "https://") {
-			dep := path.Join(workspace, src)
-			paths[dep] = struct{}{}
+			paths[src] = struct{}{}
 		} else {
 			logrus.Debugf("Skipping watch on remote dependency %s", src)
 		}
@@ -286,42 +337,3 @@ func hasMultiStageFlag(flags []string) bool {
 	}
 	return false
 }
-
-func ApplyDockerIgnore(paths []string, dockerIgnorePath string) ([]string, error) {
-	absPaths, err := util.RelPathToAbsPath(paths)
-	if err != nil {
-		return nil, errors.Wrap(err, "getting absolute path of dependencies")
-	}
-	excludes := []string{}
-	if _, err := util.Fs.Stat(dockerIgnorePath); !os.IsNotExist(err) {
-		r, err := util.Fs.Open(dockerIgnorePath)
-		if err != nil {
-			return nil, err
-		}
-		defer r.Close()
-
-		excludes, err = dockerignore.ReadAll(r)
-		if err != nil {
-			return nil, err
-		}
-		excludes = append(excludes, ".dockerignore")
-	}
-
-	absPathExcludes, err := util.RelPathToAbsPath(excludes)
-	if err != nil {
-		return nil, errors.Wrap(err, "getting absolute path of docker ignored paths")
-	}
-
-	filteredDeps := []string{}
-	for _, d := range absPaths {
-		m, err := fileutils.Matches(d, absPathExcludes)
-		if err != nil {
-			return nil, err
-		}
-		if !m {
-			filteredDeps = append(filteredDeps, d)
-		}
-	}
-	sort.Strings(filteredDeps)
-	return filteredDeps, nil
-}

pkg/skaffold/docker/parse_test.go

@@ -18,15 +18,15 @@ package docker
 
 import (
 	"fmt"
+	"io/ioutil"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 
 	"github.com/google/go-containerregistry/v1"
-	"github.com/spf13/afero"
 )
 
 const copyDockerfile = `
@@ -159,7 +159,7 @@ func TestGetDockerfileDependencies(t *testing.T) {
 			description: "add dependency",
 			dockerfile:  addDockerfile,
 			workspace:   "docker",
-			expected:    []string{"docker/Dockerfile", "docker/nginx.conf"},
+			expected:    []string{"Dockerfile", "nginx.conf"},
 		},
 		{
 			description: "bad read",
@@ -204,11 +204,18 @@ func TestGetDockerfileDependencies(t *testing.T) {
 			expected:     []string{"Dockerfile", "file", "server.go", "test.conf", "worker.go"},
 		},
 		{
-			description:  "dockerignore with context in parent directory test",
+			description:  "dockerignore with non canonical workspace",
 			dockerfile:   contextDockerfile,
 			workspace:    "docker/../docker",
 			dockerIgnore: true,
-			expected:     []string{},
+			expected:     []string{"Dockerfile", "nginx.conf"},
+		},
+		{
+			description:  "dockerignore with context in parent directory",
+			dockerfile:   contextDockerfile,
+			workspace:    "docker/..",
+			dockerIgnore: true,
+			expected:     []string{"Dockerfile", "file", "server.go", "test.conf", "worker.go"},
 		},
 		{
 			description: "onbuild test",
@@ -231,24 +238,24 @@ func TestGetDockerfileDependencies(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			defer func(fs afero.Fs) { util.Fs = fs }(util.Fs)
-			util.Fs = afero.NewMemMapFs()
+			tmpDir, cleanup := testutil.TempDir(t)
+			defer cleanup()
 
-			util.Fs.MkdirAll("docker", 0750)
+			os.MkdirAll(filepath.Join(tmpDir, "docker"), 0750)
 			for _, file := range []string{"docker/nginx.conf", "docker/bar", "server.go", "test.conf", "worker.go", "bar", "file"} {
-				afero.WriteFile(util.Fs, file, []byte(""), 0644)
+				ioutil.WriteFile(filepath.Join(tmpDir, file), []byte(""), 0644)
 			}
 
+			workspace := filepath.Join(tmpDir, test.workspace)
 			if !test.badReader {
-				afero.WriteFile(util.Fs, filepath.Join(test.workspace, "Dockerfile"), []byte(test.dockerfile), 0644)
+				ioutil.WriteFile(filepath.Join(workspace, "Dockerfile"), []byte(test.dockerfile), 0644)
 			}
 
 			if test.dockerIgnore {
-				afero.WriteFile(util.Fs, ".dockerignore", []byte(dockerIgnore), 0644)
-				afero.WriteFile(util.Fs, filepath.Join("docker", ".dockerignore"), []byte(dockerIgnore), 0644)
+				ioutil.WriteFile(filepath.Join(workspace, ".dockerignore"), []byte(dockerIgnore), 0644)
 			}
 
-			deps, err := GetDockerfileDependencies("Dockerfile", test.workspace)
+			deps, err := GetDockerfileDependencies("Dockerfile", workspace)
 			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, deps)
 		})
 	}

pkg/skaffold/util/tar.go

@@ -31,22 +31,10 @@ func CreateTar(w io.Writer, root string, paths []string) error {
 	tw := tar.NewWriter(w)
 	defer tw.Close()
 
-	absContext, err := filepath.Abs(root)
-	if err != nil {
-		return err
-	}
 	for _, p := range paths {
-		absPath, err := filepath.Abs(p)
-		if err != nil {
-			return err
-		}
-
-		tarPath, err := filepath.Rel(absContext, absPath)
-		if err != nil {
-			return err
-		}
+		tarPath := filepath.ToSlash(filepath.Join(root, p))
 
-		if err := addFileToTar(p, filepath.ToSlash(tarPath), tw); err != nil {
+		if err := addFileToTar(tarPath, p, tw); err != nil {
 			return err
 		}