diff --git a/builder/builder.go b/builder/builder.go index c333b6a309..8d6c8ef3e2 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -33,7 +33,8 @@ type Context interface { Close() error // Stat returns an entry corresponding to path if any. // It is recommended to return an error if path was not found. - Stat(path string) (FileInfo, error) + // If path is a symlink it also returns the path to the target file. + Stat(path string) (string, FileInfo, error) // Open opens path from the context and returns a readable stream of it. Open(path string) (io.ReadCloser, error) // Walk walks the tree of the context with the function passed to it. @@ -64,6 +65,8 @@ type PathFileInfo struct { os.FileInfo // FilePath holds the absolute path to the file. FilePath string + // Name holds the basename for the file. + FileName string } // Path returns the absolute path to the file. @@ -71,6 +74,14 @@ func (fi PathFileInfo) Path() string { return fi.FilePath } +// Name returns the basename of the file. +func (fi PathFileInfo) Name() string { + if fi.FileName != "" { + return fi.FileName + } + return fi.FileInfo.Name() +} + // Hashed defines an extra method intended for implementations of os.FileInfo. type Hashed interface { // Hash returns the hash of a file. diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 51caee8d79..ab6c1cdf19 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -366,7 +366,7 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression // Must be a dir or a file - fi, err := b.context.Stat(origPath) + statPath, fi, err := b.context.Stat(origPath) if err != nil { return nil, err } @@ -383,11 +383,9 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression hfi.SetHash("file:" + hfi.Hash()) return copyInfos, nil } - // Must be a dir - var subfiles []string - b.context.Walk(origPath, func(path string, info builder.FileInfo, err error) error { + err = b.context.Walk(statPath, func(path string, info builder.FileInfo, err error) error { if err != nil { return err } @@ -395,6 +393,9 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression subfiles = append(subfiles, info.(builder.Hashed).Hash()) return nil }) + if err != nil { + return nil, err + } sort.Strings(subfiles) hasher := sha256.New() @@ -613,9 +614,9 @@ func (b *Builder) readDockerfile() error { // back to 'Dockerfile' and use that in the error message. if b.DockerfileName == "" { b.DockerfileName = api.DefaultDockerfileName - if _, err := b.context.Stat(b.DockerfileName); os.IsNotExist(err) { + if _, _, err := b.context.Stat(b.DockerfileName); os.IsNotExist(err) { lowercase := strings.ToLower(b.DockerfileName) - if _, err := b.context.Stat(lowercase); err == nil { + if _, _, err := b.context.Stat(lowercase); err == nil { b.DockerfileName = lowercase } } diff --git a/builder/tarsum.go b/builder/tarsum.go index b020972657..201e3ef7d9 100644 --- a/builder/tarsum.go +++ b/builder/tarsum.go @@ -5,7 +5,6 @@ import ( "io" "os" "path/filepath" - "strings" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/chrootarchive" @@ -43,26 +42,32 @@ func (c *tarSumContext) Open(path string) (io.ReadCloser, error) { return r, nil } -func (c *tarSumContext) Stat(path string) (fi FileInfo, err error) { +func (c *tarSumContext) Stat(path string) (string, FileInfo, error) { cleanpath, fullpath, err := c.normalize(path) if err != nil { - return nil, err + return "", nil, err } st, err := os.Lstat(fullpath) if err != nil { - return nil, convertPathError(err, cleanpath) + return "", nil, convertPathError(err, cleanpath) } - fi = PathFileInfo{st, fullpath} - // we set sum to path by default for the case where GetFile returns nil. - // The usual case is if cleanpath is empty. + rel, err := filepath.Rel(c.root, fullpath) + if err != nil { + return "", nil, convertPathError(err, cleanpath) + } + + // We set sum to path by default for the case where GetFile returns nil. + // The usual case is if relative path is empty. sum := path - if tsInfo := c.sums.GetFile(cleanpath); tsInfo != nil { + // Use the checksum of the followed path(not the possible symlink) because + // this is the file that is actually copied. + if tsInfo := c.sums.GetFile(rel); tsInfo != nil { sum = tsInfo.Sum() } - fi = &HashedFileInfo{fi, sum} - return fi, nil + fi := &HashedFileInfo{PathFileInfo{st, fullpath, filepath.Base(cleanpath)}, sum} + return rel, fi, nil } // MakeTarSumContext returns a build Context from a tar stream. @@ -114,7 +119,7 @@ func (c *tarSumContext) normalize(path string) (cleanpath, fullpath string, err if err != nil { return "", "", fmt.Errorf("Forbidden path outside the build context: %s (%s)", path, fullpath) } - _, err = os.Stat(fullpath) + _, err = os.Lstat(fullpath) if err != nil { return "", "", convertPathError(err, path) } @@ -122,38 +127,26 @@ func (c *tarSumContext) normalize(path string) (cleanpath, fullpath string, err } func (c *tarSumContext) Walk(root string, walkFn WalkFunc) error { - for _, tsInfo := range c.sums { - path := tsInfo.Name() - path, fullpath, err := c.normalize(path) + root = filepath.Join(c.root, filepath.Join(string(filepath.Separator), root)) + return filepath.Walk(root, func(fullpath string, info os.FileInfo, err error) error { + rel, err := filepath.Rel(c.root, fullpath) if err != nil { return err } - - // Any file in the context that starts with the given path will be - // picked up and its hashcode used. However, we'll exclude the - // root dir itself. We do this for a coupel of reasons: - // 1 - ADD/COPY will not copy the dir itself, just its children - // so there's no reason to include it in the hash calc - // 2 - the metadata on the dir will change when any child file - // changes. This will lead to a miss in the cache check if that - // child file is in the .dockerignore list. - if rel, err := filepath.Rel(root, path); err != nil { - return err - } else if rel == "." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { - continue + if rel == "." { + return nil } - info, err := os.Lstat(fullpath) - if err != nil { - return convertPathError(err, path) + sum := rel + if tsInfo := c.sums.GetFile(rel); tsInfo != nil { + sum = tsInfo.Sum() } - // TODO check context breakout? - fi := &HashedFileInfo{PathFileInfo{info, fullpath}, tsInfo.Sum()} - if err := walkFn(path, fi, nil); err != nil { + fi := &HashedFileInfo{PathFileInfo{FileInfo: info, FilePath: fullpath}, sum} + if err := walkFn(rel, fi, nil); err != nil { return err } - } - return nil + return nil + }) } func (c *tarSumContext) Remove(path string) error { diff --git a/daemon/daemonbuilder/builder.go b/daemon/daemonbuilder/builder.go index 4bba3b4166..f6bbec4d67 100644 --- a/daemon/daemonbuilder/builder.go +++ b/daemon/daemonbuilder/builder.go @@ -183,7 +183,7 @@ func (d Docker) Copy(c *daemon.Container, destPath string, src builder.FileInfo, // only needed for fixPermissions, but might as well put it before CopyFileWithTar if destExists && destStat.IsDir() { - destPath = filepath.Join(destPath, filepath.Base(srcPath)) + destPath = filepath.Join(destPath, src.Name()) } if err := idtools.MkdirAllNewAs(filepath.Dir(destPath), 0755, rootUID, rootGID); err != nil { diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index b5cd181d6a..86fb731e72 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -21,6 +21,7 @@ import ( "github.com/docker/docker/builder/dockerfile/command" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/integration/checker" "github.com/docker/docker/pkg/stringutils" "github.com/go-check/check" ) @@ -6277,3 +6278,127 @@ func (s *DockerSuite) TestBuildMultipleTags(c *check.C) { c.Assert(err, check.IsNil) c.Assert(id1, check.Equals, id2) } + +// #17290 +func (s *DockerSuite) TestBuildCacheBrokenSymlink(c *check.C) { + testRequires(c, DaemonIsLinux) + name := "testbuildbrokensymlink" + ctx, err := fakeContext(` + FROM busybox + COPY . ./`, + map[string]string{ + "foo": "bar", + }) + c.Assert(err, checker.IsNil) + defer ctx.Close() + + err = os.Symlink(filepath.Join(ctx.Dir, "nosuchfile"), filepath.Join(ctx.Dir, "asymlink")) + c.Assert(err, checker.IsNil) + + // warm up cache + _, err = buildImageFromContext(name, ctx, true) + c.Assert(err, checker.IsNil) + + // add new file to context, should invalidate cache + err = ioutil.WriteFile(filepath.Join(ctx.Dir, "newfile"), []byte("foo"), 0644) + c.Assert(err, checker.IsNil) + + _, out, err := buildImageFromContextWithOut(name, ctx, true) + c.Assert(err, checker.IsNil) + + c.Assert(out, checker.Not(checker.Contains), "Using cache") + +} + +func (s *DockerSuite) TestBuildFollowSymlinkToFile(c *check.C) { + testRequires(c, DaemonIsLinux) + name := "testbuildbrokensymlink" + ctx, err := fakeContext(` + FROM busybox + COPY asymlink target`, + map[string]string{ + "foo": "bar", + }) + c.Assert(err, checker.IsNil) + defer ctx.Close() + + err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink")) + c.Assert(err, checker.IsNil) + + id, err := buildImageFromContext(name, ctx, true) + c.Assert(err, checker.IsNil) + + out, _ := dockerCmd(c, "run", "--rm", id, "cat", "target") + c.Assert(out, checker.Matches, "bar") + + // change target file should invalidate cache + err = ioutil.WriteFile(filepath.Join(ctx.Dir, "foo"), []byte("baz"), 0644) + c.Assert(err, checker.IsNil) + + id, out, err = buildImageFromContextWithOut(name, ctx, true) + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Not(checker.Contains), "Using cache") + + out, _ = dockerCmd(c, "run", "--rm", id, "cat", "target") + c.Assert(out, checker.Matches, "baz") +} + +func (s *DockerSuite) TestBuildFollowSymlinkToDir(c *check.C) { + testRequires(c, DaemonIsLinux) + name := "testbuildbrokensymlink" + ctx, err := fakeContext(` + FROM busybox + COPY asymlink /`, + map[string]string{ + "foo/abc": "bar", + "foo/def": "baz", + }) + c.Assert(err, checker.IsNil) + defer ctx.Close() + + err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink")) + c.Assert(err, checker.IsNil) + + id, err := buildImageFromContext(name, ctx, true) + c.Assert(err, checker.IsNil) + + out, _ := dockerCmd(c, "run", "--rm", id, "cat", "abc", "def") + c.Assert(out, checker.Matches, "barbaz") + + // change target file should invalidate cache + err = ioutil.WriteFile(filepath.Join(ctx.Dir, "foo/def"), []byte("bax"), 0644) + c.Assert(err, checker.IsNil) + + id, out, err = buildImageFromContextWithOut(name, ctx, true) + c.Assert(err, checker.IsNil) + c.Assert(out, checker.Not(checker.Contains), "Using cache") + + out, _ = dockerCmd(c, "run", "--rm", id, "cat", "abc", "def") + c.Assert(out, checker.Matches, "barbax") + +} + +// TestBuildSymlinkBasename tests that target file gets basename from symlink, +// not from the target file. +func (s *DockerSuite) TestBuildSymlinkBasename(c *check.C) { + testRequires(c, DaemonIsLinux) + name := "testbuildbrokensymlink" + ctx, err := fakeContext(` + FROM busybox + COPY asymlink /`, + map[string]string{ + "foo": "bar", + }) + c.Assert(err, checker.IsNil) + defer ctx.Close() + + err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink")) + c.Assert(err, checker.IsNil) + + id, err := buildImageFromContext(name, ctx, true) + c.Assert(err, checker.IsNil) + + out, _ := dockerCmd(c, "run", "--rm", id, "cat", "asymlink") + c.Assert(out, checker.Matches, "bar") + +} diff --git a/integration-cli/docker_utils.go b/integration-cli/docker_utils.go index 5540b792d7..bca3926070 100644 --- a/integration-cli/docker_utils.go +++ b/integration-cli/docker_utils.go @@ -1234,6 +1234,14 @@ func buildImage(name, dockerfile string, useCache bool, buildFlags ...string) (s } func buildImageFromContext(name string, ctx *FakeContext, useCache bool, buildFlags ...string) (string, error) { + id, _, err := buildImageFromContextWithOut(name, ctx, useCache, buildFlags...) + if err != nil { + return "", err + } + return id, nil +} + +func buildImageFromContextWithOut(name string, ctx *FakeContext, useCache bool, buildFlags ...string) (string, string, error) { args := []string{"build", "-t", name} if !useCache { args = append(args, "--no-cache") @@ -1244,9 +1252,13 @@ func buildImageFromContext(name string, ctx *FakeContext, useCache bool, buildFl buildCmd.Dir = ctx.Dir out, exitCode, err := runCommandWithOutput(buildCmd) if err != nil || exitCode != 0 { - return "", fmt.Errorf("failed to build the image: %s", out) + return "", "", fmt.Errorf("failed to build the image: %s", out) } - return getIDByName(name) + id, err := getIDByName(name) + if err != nil { + return "", "", err + } + return id, out, nil } func buildImageFromPath(name, path string, useCache bool, buildFlags ...string) (string, error) {