From 6d801a3caa54ad7ef574bc426aa1ffc412c5af82 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Thu, 23 Oct 2014 14:30:11 -0700 Subject: [PATCH] Have .dockerignore support Dockerfile/.dockerignore If .dockerignore mentions either then the client will send them to the daemon but the daemon will erase them after the Dockerfile has been parsed to simulate them never being sent in the first place. an events test kept failing for me so I tried to fix that too Closes #8330 Signed-off-by: Doug Davis --- api/client/commands.go | 42 +++++------ builder/evaluator.go | 70 +++++++++++------ builder/internals.go | 10 ++- daemon/container.go | 4 +- daemon/graphdriver/aufs/aufs.go | 4 +- docs/sources/reference/builder.md | 6 ++ graph/load.go | 2 +- integration-cli/docker_cli_build_test.go | 92 +++++++++++++++++++++-- integration-cli/docker_cli_events_test.go | 5 +- pkg/archive/archive.go | 48 ++++++++---- pkg/archive/archive_test.go | 8 +- pkg/chrootarchive/archive.go | 4 +- pkg/chrootarchive/archive_test.go | 2 +- pkg/tarsum/builder_context.go | 20 +++++ utils/utils.go | 32 ++++++++ volumes/volume.go | 6 +- 16 files changed, 271 insertions(+), 84 deletions(-) create mode 100644 pkg/tarsum/builder_context.go diff --git a/api/client/commands.go b/api/client/commands.go index e666d43205..14b01f5603 100644 --- a/api/client/commands.go +++ b/api/client/commands.go @@ -14,7 +14,6 @@ import ( "os" "os/exec" "path" - "path/filepath" "runtime" "strconv" "strings" @@ -30,6 +29,7 @@ import ( "github.com/docker/docker/nat" "github.com/docker/docker/opts" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/fileutils" flag "github.com/docker/docker/pkg/mflag" "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/parsers/filters" @@ -143,32 +143,32 @@ func (cli *DockerCli) CmdBuild(args ...string) error { if _, err = os.Stat(filename); os.IsNotExist(err) { return fmt.Errorf("no Dockerfile found in %s", cmd.Arg(0)) } - var excludes []string - ignore, err := ioutil.ReadFile(path.Join(root, ".dockerignore")) - if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("Error reading .dockerignore: '%s'", err) + var includes []string = []string{"."} + + excludes, err := utils.ReadDockerIgnore(path.Join(root, ".dockerignore")) + if err != nil { + return err } - for _, pattern := range strings.Split(string(ignore), "\n") { - pattern = strings.TrimSpace(pattern) - if pattern == "" { - continue - } - pattern = filepath.Clean(pattern) - ok, err := filepath.Match(pattern, "Dockerfile") - if err != nil { - return fmt.Errorf("Bad .dockerignore pattern: '%s', error: %s", pattern, err) - } - if ok { - return fmt.Errorf("Dockerfile was excluded by .dockerignore pattern '%s'", pattern) - } - excludes = append(excludes, pattern) + + // If .dockerignore mentions .dockerignore or Dockerfile + // then make sure we send both files over to the daemon + // because Dockerfile is, obviously, needed no matter what, and + // .dockerignore is needed to know if either one needs to be + // removed. The deamon will remove them for us, if needed, after it + // parses the Dockerfile. + keepThem1, _ := fileutils.Matches(".dockerignore", excludes) + keepThem2, _ := fileutils.Matches("Dockerfile", excludes) + if keepThem1 || keepThem2 { + includes = append(includes, ".dockerignore", "Dockerfile") } + if err = utils.ValidateContextDirectory(root, excludes); err != nil { return fmt.Errorf("Error checking context is accessible: '%s'. Please check permissions and try again.", err) } options := &archive.TarOptions{ - Compression: archive.Uncompressed, - Excludes: excludes, + Compression: archive.Uncompressed, + ExcludePatterns: excludes, + IncludeFiles: includes, } context, err = archive.TarWithOptions(root, options) if err != nil { diff --git a/builder/evaluator.go b/builder/evaluator.go index eef222b943..43fb419fce 100644 --- a/builder/evaluator.go +++ b/builder/evaluator.go @@ -31,6 +31,7 @@ import ( "github.com/docker/docker/builder/parser" "github.com/docker/docker/daemon" "github.com/docker/docker/engine" + "github.com/docker/docker/pkg/fileutils" "github.com/docker/docker/pkg/tarsum" "github.com/docker/docker/registry" "github.com/docker/docker/runconfig" @@ -136,30 +137,10 @@ func (b *Builder) Run(context io.Reader) (string, error) { } }() - filename := path.Join(b.contextPath, "Dockerfile") - - fi, err := os.Stat(filename) - if os.IsNotExist(err) { - return "", fmt.Errorf("Cannot build a directory without a Dockerfile") - } - if fi.Size() == 0 { - return "", ErrDockerfileEmpty - } - - f, err := os.Open(filename) - if err != nil { + if err := b.readDockerfile("Dockerfile"); err != nil { return "", err } - defer f.Close() - - ast, err := parser.Parse(f) - if err != nil { - return "", err - } - - b.dockerfile = ast - // some initializations that would not have been supplied by the caller. b.Config = &runconfig.Config{} b.TmpContainers = map[string]struct{}{} @@ -185,6 +166,53 @@ func (b *Builder) Run(context io.Reader) (string, error) { return b.image, nil } +// Reads a Dockerfile from the current context. It assumes that the +// 'filename' is a relative path from the root of the context +func (b *Builder) readDockerfile(filename string) error { + filename = path.Join(b.contextPath, filename) + + fi, err := os.Stat(filename) + if os.IsNotExist(err) { + return fmt.Errorf("Cannot build a directory without a Dockerfile") + } + if fi.Size() == 0 { + return ErrDockerfileEmpty + } + + f, err := os.Open(filename) + if err != nil { + return err + } + + b.dockerfile, err = parser.Parse(f) + f.Close() + + if err != nil { + return err + } + + // After the Dockerfile has been parsed, we need to check the .dockerignore + // file for either "Dockerfile" or ".dockerignore", and if either are + // present then erase them from the build context. These files should never + // have been sent from the client but we did send them to make sure that + // we had the Dockerfile to actually parse, and then we also need the + // .dockerignore file to know whether either file should be removed. + // Note that this assumes the Dockerfile has been read into memory and + // is now safe to be removed. + + excludes, _ := utils.ReadDockerIgnore(path.Join(b.contextPath, ".dockerignore")) + if rm, _ := fileutils.Matches(".dockerignore", excludes); rm == true { + os.Remove(path.Join(b.contextPath, ".dockerignore")) + b.context.(tarsum.BuilderContext).Remove(".dockerignore") + } + if rm, _ := fileutils.Matches("Dockerfile", excludes); rm == true { + os.Remove(path.Join(b.contextPath, "Dockerfile")) + b.context.(tarsum.BuilderContext).Remove("Dockerfile") + } + + return nil +} + // This method is the entrypoint to all statement handling routines. // // Almost all nodes will have this structure: diff --git a/builder/internals.go b/builder/internals.go index 1caa33141c..909e7a8d10 100644 --- a/builder/internals.go +++ b/builder/internals.go @@ -390,7 +390,15 @@ func calcCopyInfo(b *Builder, cmdName string, cInfos *[]*copyInfo, origPath stri for _, fileInfo := range b.context.GetSums() { absFile := path.Join(b.contextPath, fileInfo.Name()) - if strings.HasPrefix(absFile, absOrigPath) || absFile == absOrigPathNoSlash { + // 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 strings.HasPrefix(absFile, absOrigPath) && absFile != absOrigPathNoSlash { subfiles = append(subfiles, fileInfo.Sum()) } } diff --git a/daemon/container.go b/daemon/container.go index 75cd133fec..45aaa8be59 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -891,8 +891,8 @@ func (container *Container) Copy(resource string) (io.ReadCloser, error) { } archive, err := archive.TarWithOptions(basePath, &archive.TarOptions{ - Compression: archive.Uncompressed, - Includes: filter, + Compression: archive.Uncompressed, + IncludeFiles: filter, }) if err != nil { container.Unmount() diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index 82a5c89051..220be2de55 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/daemon/graphdriver/aufs/aufs.go @@ -300,8 +300,8 @@ func (a *Driver) Put(id string) { func (a *Driver) Diff(id, parent string) (archive.Archive, error) { // AUFS doesn't need the parent layer to produce a diff. return archive.TarWithOptions(path.Join(a.rootPath(), "diff", id), &archive.TarOptions{ - Compression: archive.Uncompressed, - Excludes: []string{".wh..wh.*"}, + Compression: archive.Uncompressed, + ExcludePatterns: []string{".wh..wh.*"}, }) } diff --git a/docs/sources/reference/builder.md b/docs/sources/reference/builder.md index fa7393efa3..90862334ef 100644 --- a/docs/sources/reference/builder.md +++ b/docs/sources/reference/builder.md @@ -154,6 +154,12 @@ Exclusion patterns match files or directories relative to the source repository that will be excluded from the context. Globbing is done using Go's [filepath.Match](http://golang.org/pkg/path/filepath#Match) rules. +> **Note**: +> The `.dockerignore` file can even be used to ignore the `Dockerfile` and +> `.dockerignore` files. This might be useful if you are copying files from +> the root of the build context into your new containter but do not want to +> include the `Dockerfile` or `.dockerignore` files (e.g. `ADD . /someDir/`). + The following example shows the use of the `.dockerignore` file to exclude the `.git` directory from the context. Its effect can be seen in the changed size of the uploaded context. diff --git a/graph/load.go b/graph/load.go index 6ef219c077..4399da25d9 100644 --- a/graph/load.go +++ b/graph/load.go @@ -57,7 +57,7 @@ func (s *TagStore) CmdLoad(job *engine.Job) engine.Status { excludes[i] = k i++ } - if err := chrootarchive.Untar(repoFile, repoDir, &archive.TarOptions{Excludes: excludes}); err != nil { + if err := chrootarchive.Untar(repoFile, repoDir, &archive.TarOptions{ExcludePatterns: excludes}); err != nil { return job.Error(err) } diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index bb53b68acc..c4d7ff7c54 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -3131,28 +3131,106 @@ func TestBuildDockerignoringDockerfile(t *testing.T) { name := "testbuilddockerignoredockerfile" defer deleteImages(name) dockerfile := ` - FROM scratch` + FROM busybox + ADD . /tmp/ + RUN ! ls /tmp/Dockerfile + RUN ls /tmp/.dockerignore` ctx, err := fakeContext(dockerfile, map[string]string{ - "Dockerfile": "FROM scratch", + "Dockerfile": dockerfile, ".dockerignore": "Dockerfile\n", }) if err != nil { t.Fatal(err) } - defer ctx.Close() - if _, err = buildImageFromContext(name, ctx, true); err == nil { - t.Fatalf("Didn't get expected error from ignoring Dockerfile") + if _, err = buildImageFromContext(name, ctx, true); err != nil { + t.Fatalf("Didn't ignore Dockerfile correctly:%s", err) } // now try it with ./Dockerfile ctx.Add(".dockerignore", "./Dockerfile\n") - if _, err = buildImageFromContext(name, ctx, true); err == nil { - t.Fatalf("Didn't get expected error from ignoring ./Dockerfile") + if _, err = buildImageFromContext(name, ctx, true); err != nil { + t.Fatalf("Didn't ignore ./Dockerfile correctly:%s", err) } logDone("build - test .dockerignore of Dockerfile") } +func TestBuildDockerignoringDockerignore(t *testing.T) { + name := "testbuilddockerignoredockerignore" + defer deleteImages(name) + dockerfile := ` + FROM busybox + ADD . /tmp/ + RUN ! ls /tmp/.dockerignore + RUN ls /tmp/Dockerfile` + ctx, err := fakeContext(dockerfile, map[string]string{ + "Dockerfile": dockerfile, + ".dockerignore": ".dockerignore\n", + }) + defer ctx.Close() + if err != nil { + t.Fatal(err) + } + if _, err = buildImageFromContext(name, ctx, true); err != nil { + t.Fatalf("Didn't ignore .dockerignore correctly:%s", err) + } + logDone("build - test .dockerignore of .dockerignore") +} + +func TestBuildDockerignoreTouchDockerfile(t *testing.T) { + var id1 string + var id2 string + + name := "testbuilddockerignoretouchdockerfile" + defer deleteImages(name) + dockerfile := ` + FROM busybox + ADD . /tmp/` + ctx, err := fakeContext(dockerfile, map[string]string{ + "Dockerfile": dockerfile, + ".dockerignore": "Dockerfile\n", + }) + defer ctx.Close() + if err != nil { + t.Fatal(err) + } + + if id1, err = buildImageFromContext(name, ctx, true); err != nil { + t.Fatalf("Didn't build it correctly:%s", err) + } + + if id2, err = buildImageFromContext(name, ctx, true); err != nil { + t.Fatalf("Didn't build it correctly:%s", err) + } + if id1 != id2 { + t.Fatalf("Didn't use the cache - 1") + } + + // Now make sure touching Dockerfile doesn't invalidate the cache + if err = ctx.Add("Dockerfile", dockerfile+"\n# hi"); err != nil { + t.Fatalf("Didn't add Dockerfile: %s", err) + } + if id2, err = buildImageFromContext(name, ctx, true); err != nil { + t.Fatalf("Didn't build it correctly:%s", err) + } + if id1 != id2 { + t.Fatalf("Didn't use the cache - 2") + } + + // One more time but just 'touch' it instead of changing the content + if err = ctx.Add("Dockerfile", dockerfile+"\n# hi"); err != nil { + t.Fatalf("Didn't add Dockerfile: %s", err) + } + if id2, err = buildImageFromContext(name, ctx, true); err != nil { + t.Fatalf("Didn't build it correctly:%s", err) + } + if id1 != id2 { + t.Fatalf("Didn't use the cache - 3") + } + + logDone("build - test .dockerignore touch dockerfile") +} + func TestBuildDockerignoringWholeDir(t *testing.T) { name := "testbuilddockerignorewholedir" defer deleteImages(name) diff --git a/integration-cli/docker_cli_events_test.go b/integration-cli/docker_cli_events_test.go index be6f1202c4..322d622b55 100644 --- a/integration-cli/docker_cli_events_test.go +++ b/integration-cli/docker_cli_events_test.go @@ -10,14 +10,13 @@ import ( ) func TestEventsUntag(t *testing.T) { - out, _, _ := dockerCmd(t, "images", "-q") - image := strings.Split(out, "\n")[0] + image := "busybox" dockerCmd(t, "tag", image, "utest:tag1") dockerCmd(t, "tag", image, "utest:tag2") dockerCmd(t, "rmi", "utest:tag1") dockerCmd(t, "rmi", "utest:tag2") eventsCmd := exec.Command("timeout", "0.2", dockerBinary, "events", "--since=1") - out, _, _ = runCommandWithOutput(eventsCmd) + out, _, _ := runCommandWithOutput(eventsCmd) events := strings.Split(out, "\n") nEvents := len(events) // The last element after the split above will be an empty string, so we diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index ec45d8546d..35566520b1 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -30,11 +30,11 @@ type ( ArchiveReader io.Reader Compression int TarOptions struct { - Includes []string - Excludes []string - Compression Compression - NoLchown bool - Name string + IncludeFiles []string + ExcludePatterns []string + Compression Compression + NoLchown bool + Name string } // Archiver allows the reuse of most utility functions of this package @@ -378,7 +378,7 @@ func escapeName(name string) string { } // TarWithOptions creates an archive from the directory at `path`, only including files whose relative -// paths are included in `options.Includes` (if non-nil) or not in `options.Excludes`. +// paths are included in `options.IncludeFiles` (if non-nil) or not in `options.ExcludePatterns`. func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) { pipeReader, pipeWriter := io.Pipe() @@ -401,12 +401,14 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) // mutating the filesystem and we can see transient errors // from this - if options.Includes == nil { - options.Includes = []string{"."} + if options.IncludeFiles == nil { + options.IncludeFiles = []string{"."} } + seen := make(map[string]bool) + var renamedRelFilePath string // For when tar.Options.Name is set - for _, include := range options.Includes { + for _, include := range options.IncludeFiles { filepath.Walk(filepath.Join(srcPath, include), func(filePath string, f os.FileInfo, err error) error { if err != nil { log.Debugf("Tar: Can't stat file %s to tar: %s", srcPath, err) @@ -420,10 +422,19 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) return nil } - skip, err := fileutils.Matches(relFilePath, options.Excludes) - if err != nil { - log.Debugf("Error matching %s", relFilePath, err) - return err + skip := false + + // If "include" is an exact match for the current file + // then even if there's an "excludePatterns" pattern that + // matches it, don't skip it. IOW, assume an explicit 'include' + // is asking for that file no matter what - which is true + // for some files, like .dockerignore and Dockerfile (sometimes) + if include != relFilePath { + skip, err = fileutils.Matches(relFilePath, options.ExcludePatterns) + if err != nil { + log.Debugf("Error matching %s", relFilePath, err) + return err + } } if skip { @@ -433,6 +444,11 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) return nil } + if seen[relFilePath] { + return nil + } + seen[relFilePath] = true + // Rename the base resource if options.Name != "" && filePath == srcPath+"/"+filepath.Base(relFilePath) { renamedRelFilePath = relFilePath @@ -487,7 +503,7 @@ loop: // This keeps "../" as-is, but normalizes "/../" to "/" hdr.Name = filepath.Clean(hdr.Name) - for _, exclude := range options.Excludes { + for _, exclude := range options.ExcludePatterns { if strings.HasPrefix(hdr.Name, exclude) { continue loop } @@ -563,8 +579,8 @@ func Untar(archive io.Reader, dest string, options *TarOptions) error { if options == nil { options = &TarOptions{} } - if options.Excludes == nil { - options.Excludes = []string{} + if options.ExcludePatterns == nil { + options.ExcludePatterns = []string{} } decompressedArchive, err := DecompressStream(archive) if err != nil { diff --git a/pkg/archive/archive_test.go b/pkg/archive/archive_test.go index fdba6fb87c..6cd95d5ad5 100644 --- a/pkg/archive/archive_test.go +++ b/pkg/archive/archive_test.go @@ -165,8 +165,8 @@ func TestTarUntar(t *testing.T) { Gzip, } { changes, err := tarUntar(t, origin, &TarOptions{ - Compression: c, - Excludes: []string{"3"}, + Compression: c, + ExcludePatterns: []string{"3"}, }) if err != nil { @@ -196,8 +196,8 @@ func TestTarWithOptions(t *testing.T) { opts *TarOptions numChanges int }{ - {&TarOptions{Includes: []string{"1"}}, 1}, - {&TarOptions{Excludes: []string{"2"}}, 1}, + {&TarOptions{IncludeFiles: []string{"1"}}, 1}, + {&TarOptions{ExcludePatterns: []string{"2"}}, 1}, } for _, testCase := range cases { changes, err := tarUntar(t, origin, testCase.opts) diff --git a/pkg/chrootarchive/archive.go b/pkg/chrootarchive/archive.go index 66f837314a..ae15a2a54d 100644 --- a/pkg/chrootarchive/archive.go +++ b/pkg/chrootarchive/archive.go @@ -50,8 +50,8 @@ func Untar(tarArchive io.Reader, dest string, options *archive.TarOptions) error if options == nil { options = &archive.TarOptions{} } - if options.Excludes == nil { - options.Excludes = []string{} + if options.ExcludePatterns == nil { + options.ExcludePatterns = []string{} } var ( diff --git a/pkg/chrootarchive/archive_test.go b/pkg/chrootarchive/archive_test.go index bb8a22dc70..b3f7d57688 100644 --- a/pkg/chrootarchive/archive_test.go +++ b/pkg/chrootarchive/archive_test.go @@ -40,7 +40,7 @@ func TestChrootTarUntar(t *testing.T) { if err := os.MkdirAll(dest, 0700); err != nil { t.Fatal(err) } - if err := Untar(stream, dest, &archive.TarOptions{Excludes: []string{"lolo"}}); err != nil { + if err := Untar(stream, dest, &archive.TarOptions{ExcludePatterns: []string{"lolo"}}); err != nil { t.Fatal(err) } } diff --git a/pkg/tarsum/builder_context.go b/pkg/tarsum/builder_context.go new file mode 100644 index 0000000000..06a42825e7 --- /dev/null +++ b/pkg/tarsum/builder_context.go @@ -0,0 +1,20 @@ +package tarsum + +// This interface extends TarSum by adding the Remove method. In general +// there was concern about adding this method to TarSum itself so instead +// it is being added just to "BuilderContext" which will then only be used +// during the .dockerignore file processing - see builder/evaluator.go +type BuilderContext interface { + TarSum + Remove(string) +} + +func (bc *tarSum) Remove(filename string) { + for i, fis := range bc.sums { + if fis.Name() == filename { + bc.sums = append(bc.sums[:i], bc.sums[i+1:]...) + // Note, we don't just return because there could be + // more than one with this name + } + } +} diff --git a/utils/utils.go b/utils/utils.go index 8d3b3eb73e..4e36b4061e 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -1,6 +1,7 @@ package utils import ( + "bufio" "bytes" "crypto/rand" "crypto/sha1" @@ -492,3 +493,34 @@ func StringsContainsNoCase(slice []string, s string) bool { } return false } + +// Reads a .dockerignore file and returns the list of file patterns +// to ignore. Note this will trim whitespace from each line as well +// as use GO's "clean" func to get the shortest/cleanest path for each. +func ReadDockerIgnore(path string) ([]string, error) { + // Note that a missing .dockerignore file isn't treated as an error + reader, err := os.Open(path) + if err != nil { + if !os.IsNotExist(err) { + return nil, fmt.Errorf("Error reading '%s': %v", path, err) + } + return nil, nil + } + defer reader.Close() + + scanner := bufio.NewScanner(reader) + var excludes []string + + for scanner.Scan() { + pattern := strings.TrimSpace(scanner.Text()) + if pattern == "" { + continue + } + pattern = filepath.Clean(pattern) + excludes = append(excludes, pattern) + } + if err = scanner.Err(); err != nil { + return nil, fmt.Errorf("Error reading '%s': %v", path, err) + } + return excludes, nil +} diff --git a/volumes/volume.go b/volumes/volume.go index d718b07d70..db99aed5dc 100644 --- a/volumes/volume.go +++ b/volumes/volume.go @@ -47,9 +47,9 @@ func (v *Volume) Export(resource, name string) (io.ReadCloser, error) { basePath = path.Dir(basePath) } return archive.TarWithOptions(basePath, &archive.TarOptions{ - Compression: archive.Uncompressed, - Name: name, - Includes: filter, + Compression: archive.Uncompressed, + Name: name, + IncludeFiles: filter, }) }