Fixes #17290
Fixes following issues:
- Cache checksums turning off while walking a broken symlink.
- Cache checksums were taken from symlinks while targets were actually copied.
- Copying a symlink pointing to a file to a directory used the basename of the target as a destination basename, instead of basename of the symlink.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
... | ... |
@@ -33,7 +33,8 @@ type Context interface { |
33 | 33 |
Close() error |
34 | 34 |
// Stat returns an entry corresponding to path if any. |
35 | 35 |
// It is recommended to return an error if path was not found. |
36 |
- Stat(path string) (FileInfo, error) |
|
36 |
+ // If path is a symlink it also returns the path to the target file. |
|
37 |
+ Stat(path string) (string, FileInfo, error) |
|
37 | 38 |
// Open opens path from the context and returns a readable stream of it. |
38 | 39 |
Open(path string) (io.ReadCloser, error) |
39 | 40 |
// Walk walks the tree of the context with the function passed to it. |
... | ... |
@@ -64,6 +65,8 @@ type PathFileInfo struct { |
64 | 64 |
os.FileInfo |
65 | 65 |
// FilePath holds the absolute path to the file. |
66 | 66 |
FilePath string |
67 |
+ // Name holds the basename for the file. |
|
68 |
+ FileName string |
|
67 | 69 |
} |
68 | 70 |
|
69 | 71 |
// Path returns the absolute path to the file. |
... | ... |
@@ -71,6 +74,14 @@ func (fi PathFileInfo) Path() string { |
71 | 71 |
return fi.FilePath |
72 | 72 |
} |
73 | 73 |
|
74 |
+// Name returns the basename of the file. |
|
75 |
+func (fi PathFileInfo) Name() string { |
|
76 |
+ if fi.FileName != "" { |
|
77 |
+ return fi.FileName |
|
78 |
+ } |
|
79 |
+ return fi.FileInfo.Name() |
|
80 |
+} |
|
81 |
+ |
|
74 | 82 |
// Hashed defines an extra method intended for implementations of os.FileInfo. |
75 | 83 |
type Hashed interface { |
76 | 84 |
// Hash returns the hash of a file. |
... | ... |
@@ -366,7 +366,7 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression |
366 | 366 |
|
367 | 367 |
// Must be a dir or a file |
368 | 368 |
|
369 |
- fi, err := b.context.Stat(origPath) |
|
369 |
+ statPath, fi, err := b.context.Stat(origPath) |
|
370 | 370 |
if err != nil { |
371 | 371 |
return nil, err |
372 | 372 |
} |
... | ... |
@@ -383,11 +383,9 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression |
383 | 383 |
hfi.SetHash("file:" + hfi.Hash()) |
384 | 384 |
return copyInfos, nil |
385 | 385 |
} |
386 |
- |
|
387 | 386 |
// Must be a dir |
388 |
- |
|
389 | 387 |
var subfiles []string |
390 |
- b.context.Walk(origPath, func(path string, info builder.FileInfo, err error) error { |
|
388 |
+ err = b.context.Walk(statPath, func(path string, info builder.FileInfo, err error) error { |
|
391 | 389 |
if err != nil { |
392 | 390 |
return err |
393 | 391 |
} |
... | ... |
@@ -395,6 +393,9 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression |
395 | 395 |
subfiles = append(subfiles, info.(builder.Hashed).Hash()) |
396 | 396 |
return nil |
397 | 397 |
}) |
398 |
+ if err != nil { |
|
399 |
+ return nil, err |
|
400 |
+ } |
|
398 | 401 |
|
399 | 402 |
sort.Strings(subfiles) |
400 | 403 |
hasher := sha256.New() |
... | ... |
@@ -613,9 +614,9 @@ func (b *Builder) readDockerfile() error { |
613 | 613 |
// back to 'Dockerfile' and use that in the error message. |
614 | 614 |
if b.DockerfileName == "" { |
615 | 615 |
b.DockerfileName = api.DefaultDockerfileName |
616 |
- if _, err := b.context.Stat(b.DockerfileName); os.IsNotExist(err) { |
|
616 |
+ if _, _, err := b.context.Stat(b.DockerfileName); os.IsNotExist(err) { |
|
617 | 617 |
lowercase := strings.ToLower(b.DockerfileName) |
618 |
- if _, err := b.context.Stat(lowercase); err == nil { |
|
618 |
+ if _, _, err := b.context.Stat(lowercase); err == nil { |
|
619 | 619 |
b.DockerfileName = lowercase |
620 | 620 |
} |
621 | 621 |
} |
... | ... |
@@ -5,7 +5,6 @@ import ( |
5 | 5 |
"io" |
6 | 6 |
"os" |
7 | 7 |
"path/filepath" |
8 |
- "strings" |
|
9 | 8 |
|
10 | 9 |
"github.com/docker/docker/pkg/archive" |
11 | 10 |
"github.com/docker/docker/pkg/chrootarchive" |
... | ... |
@@ -43,26 +42,32 @@ func (c *tarSumContext) Open(path string) (io.ReadCloser, error) { |
43 | 43 |
return r, nil |
44 | 44 |
} |
45 | 45 |
|
46 |
-func (c *tarSumContext) Stat(path string) (fi FileInfo, err error) { |
|
46 |
+func (c *tarSumContext) Stat(path string) (string, FileInfo, error) { |
|
47 | 47 |
cleanpath, fullpath, err := c.normalize(path) |
48 | 48 |
if err != nil { |
49 |
- return nil, err |
|
49 |
+ return "", nil, err |
|
50 | 50 |
} |
51 | 51 |
|
52 | 52 |
st, err := os.Lstat(fullpath) |
53 | 53 |
if err != nil { |
54 |
- return nil, convertPathError(err, cleanpath) |
|
54 |
+ return "", nil, convertPathError(err, cleanpath) |
|
55 |
+ } |
|
56 |
+ |
|
57 |
+ rel, err := filepath.Rel(c.root, fullpath) |
|
58 |
+ if err != nil { |
|
59 |
+ return "", nil, convertPathError(err, cleanpath) |
|
55 | 60 |
} |
56 | 61 |
|
57 |
- fi = PathFileInfo{st, fullpath} |
|
58 |
- // we set sum to path by default for the case where GetFile returns nil. |
|
59 |
- // The usual case is if cleanpath is empty. |
|
62 |
+ // We set sum to path by default for the case where GetFile returns nil. |
|
63 |
+ // The usual case is if relative path is empty. |
|
60 | 64 |
sum := path |
61 |
- if tsInfo := c.sums.GetFile(cleanpath); tsInfo != nil { |
|
65 |
+ // Use the checksum of the followed path(not the possible symlink) because |
|
66 |
+ // this is the file that is actually copied. |
|
67 |
+ if tsInfo := c.sums.GetFile(rel); tsInfo != nil { |
|
62 | 68 |
sum = tsInfo.Sum() |
63 | 69 |
} |
64 |
- fi = &HashedFileInfo{fi, sum} |
|
65 |
- return fi, nil |
|
70 |
+ fi := &HashedFileInfo{PathFileInfo{st, fullpath, filepath.Base(cleanpath)}, sum} |
|
71 |
+ return rel, fi, nil |
|
66 | 72 |
} |
67 | 73 |
|
68 | 74 |
// MakeTarSumContext returns a build Context from a tar stream. |
... | ... |
@@ -114,7 +119,7 @@ func (c *tarSumContext) normalize(path string) (cleanpath, fullpath string, err |
114 | 114 |
if err != nil { |
115 | 115 |
return "", "", fmt.Errorf("Forbidden path outside the build context: %s (%s)", path, fullpath) |
116 | 116 |
} |
117 |
- _, err = os.Stat(fullpath) |
|
117 |
+ _, err = os.Lstat(fullpath) |
|
118 | 118 |
if err != nil { |
119 | 119 |
return "", "", convertPathError(err, path) |
120 | 120 |
} |
... | ... |
@@ -122,38 +127,26 @@ func (c *tarSumContext) normalize(path string) (cleanpath, fullpath string, err |
122 | 122 |
} |
123 | 123 |
|
124 | 124 |
func (c *tarSumContext) Walk(root string, walkFn WalkFunc) error { |
125 |
- for _, tsInfo := range c.sums { |
|
126 |
- path := tsInfo.Name() |
|
127 |
- path, fullpath, err := c.normalize(path) |
|
125 |
+ root = filepath.Join(c.root, filepath.Join(string(filepath.Separator), root)) |
|
126 |
+ return filepath.Walk(root, func(fullpath string, info os.FileInfo, err error) error { |
|
127 |
+ rel, err := filepath.Rel(c.root, fullpath) |
|
128 | 128 |
if err != nil { |
129 | 129 |
return err |
130 | 130 |
} |
131 |
- |
|
132 |
- // Any file in the context that starts with the given path will be |
|
133 |
- // picked up and its hashcode used. However, we'll exclude the |
|
134 |
- // root dir itself. We do this for a coupel of reasons: |
|
135 |
- // 1 - ADD/COPY will not copy the dir itself, just its children |
|
136 |
- // so there's no reason to include it in the hash calc |
|
137 |
- // 2 - the metadata on the dir will change when any child file |
|
138 |
- // changes. This will lead to a miss in the cache check if that |
|
139 |
- // child file is in the .dockerignore list. |
|
140 |
- if rel, err := filepath.Rel(root, path); err != nil { |
|
141 |
- return err |
|
142 |
- } else if rel == "." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { |
|
143 |
- continue |
|
131 |
+ if rel == "." { |
|
132 |
+ return nil |
|
144 | 133 |
} |
145 | 134 |
|
146 |
- info, err := os.Lstat(fullpath) |
|
147 |
- if err != nil { |
|
148 |
- return convertPathError(err, path) |
|
135 |
+ sum := rel |
|
136 |
+ if tsInfo := c.sums.GetFile(rel); tsInfo != nil { |
|
137 |
+ sum = tsInfo.Sum() |
|
149 | 138 |
} |
150 |
- // TODO check context breakout? |
|
151 |
- fi := &HashedFileInfo{PathFileInfo{info, fullpath}, tsInfo.Sum()} |
|
152 |
- if err := walkFn(path, fi, nil); err != nil { |
|
139 |
+ fi := &HashedFileInfo{PathFileInfo{FileInfo: info, FilePath: fullpath}, sum} |
|
140 |
+ if err := walkFn(rel, fi, nil); err != nil { |
|
153 | 141 |
return err |
154 | 142 |
} |
155 |
- } |
|
156 |
- return nil |
|
143 |
+ return nil |
|
144 |
+ }) |
|
157 | 145 |
} |
158 | 146 |
|
159 | 147 |
func (c *tarSumContext) Remove(path string) error { |
... | ... |
@@ -183,7 +183,7 @@ func (d Docker) Copy(c *daemon.Container, destPath string, src builder.FileInfo, |
183 | 183 |
|
184 | 184 |
// only needed for fixPermissions, but might as well put it before CopyFileWithTar |
185 | 185 |
if destExists && destStat.IsDir() { |
186 |
- destPath = filepath.Join(destPath, filepath.Base(srcPath)) |
|
186 |
+ destPath = filepath.Join(destPath, src.Name()) |
|
187 | 187 |
} |
188 | 188 |
|
189 | 189 |
if err := idtools.MkdirAllNewAs(filepath.Dir(destPath), 0755, rootUID, rootGID); err != nil { |
... | ... |
@@ -21,6 +21,7 @@ import ( |
21 | 21 |
|
22 | 22 |
"github.com/docker/docker/builder/dockerfile/command" |
23 | 23 |
"github.com/docker/docker/pkg/archive" |
24 |
+ "github.com/docker/docker/pkg/integration/checker" |
|
24 | 25 |
"github.com/docker/docker/pkg/stringutils" |
25 | 26 |
"github.com/go-check/check" |
26 | 27 |
) |
... | ... |
@@ -6277,3 +6278,127 @@ func (s *DockerSuite) TestBuildMultipleTags(c *check.C) { |
6277 | 6277 |
c.Assert(err, check.IsNil) |
6278 | 6278 |
c.Assert(id1, check.Equals, id2) |
6279 | 6279 |
} |
6280 |
+ |
|
6281 |
+// #17290 |
|
6282 |
+func (s *DockerSuite) TestBuildCacheBrokenSymlink(c *check.C) { |
|
6283 |
+ testRequires(c, DaemonIsLinux) |
|
6284 |
+ name := "testbuildbrokensymlink" |
|
6285 |
+ ctx, err := fakeContext(` |
|
6286 |
+ FROM busybox |
|
6287 |
+ COPY . ./`, |
|
6288 |
+ map[string]string{ |
|
6289 |
+ "foo": "bar", |
|
6290 |
+ }) |
|
6291 |
+ c.Assert(err, checker.IsNil) |
|
6292 |
+ defer ctx.Close() |
|
6293 |
+ |
|
6294 |
+ err = os.Symlink(filepath.Join(ctx.Dir, "nosuchfile"), filepath.Join(ctx.Dir, "asymlink")) |
|
6295 |
+ c.Assert(err, checker.IsNil) |
|
6296 |
+ |
|
6297 |
+ // warm up cache |
|
6298 |
+ _, err = buildImageFromContext(name, ctx, true) |
|
6299 |
+ c.Assert(err, checker.IsNil) |
|
6300 |
+ |
|
6301 |
+ // add new file to context, should invalidate cache |
|
6302 |
+ err = ioutil.WriteFile(filepath.Join(ctx.Dir, "newfile"), []byte("foo"), 0644) |
|
6303 |
+ c.Assert(err, checker.IsNil) |
|
6304 |
+ |
|
6305 |
+ _, out, err := buildImageFromContextWithOut(name, ctx, true) |
|
6306 |
+ c.Assert(err, checker.IsNil) |
|
6307 |
+ |
|
6308 |
+ c.Assert(out, checker.Not(checker.Contains), "Using cache") |
|
6309 |
+ |
|
6310 |
+} |
|
6311 |
+ |
|
6312 |
+func (s *DockerSuite) TestBuildFollowSymlinkToFile(c *check.C) { |
|
6313 |
+ testRequires(c, DaemonIsLinux) |
|
6314 |
+ name := "testbuildbrokensymlink" |
|
6315 |
+ ctx, err := fakeContext(` |
|
6316 |
+ FROM busybox |
|
6317 |
+ COPY asymlink target`, |
|
6318 |
+ map[string]string{ |
|
6319 |
+ "foo": "bar", |
|
6320 |
+ }) |
|
6321 |
+ c.Assert(err, checker.IsNil) |
|
6322 |
+ defer ctx.Close() |
|
6323 |
+ |
|
6324 |
+ err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink")) |
|
6325 |
+ c.Assert(err, checker.IsNil) |
|
6326 |
+ |
|
6327 |
+ id, err := buildImageFromContext(name, ctx, true) |
|
6328 |
+ c.Assert(err, checker.IsNil) |
|
6329 |
+ |
|
6330 |
+ out, _ := dockerCmd(c, "run", "--rm", id, "cat", "target") |
|
6331 |
+ c.Assert(out, checker.Matches, "bar") |
|
6332 |
+ |
|
6333 |
+ // change target file should invalidate cache |
|
6334 |
+ err = ioutil.WriteFile(filepath.Join(ctx.Dir, "foo"), []byte("baz"), 0644) |
|
6335 |
+ c.Assert(err, checker.IsNil) |
|
6336 |
+ |
|
6337 |
+ id, out, err = buildImageFromContextWithOut(name, ctx, true) |
|
6338 |
+ c.Assert(err, checker.IsNil) |
|
6339 |
+ c.Assert(out, checker.Not(checker.Contains), "Using cache") |
|
6340 |
+ |
|
6341 |
+ out, _ = dockerCmd(c, "run", "--rm", id, "cat", "target") |
|
6342 |
+ c.Assert(out, checker.Matches, "baz") |
|
6343 |
+} |
|
6344 |
+ |
|
6345 |
+func (s *DockerSuite) TestBuildFollowSymlinkToDir(c *check.C) { |
|
6346 |
+ testRequires(c, DaemonIsLinux) |
|
6347 |
+ name := "testbuildbrokensymlink" |
|
6348 |
+ ctx, err := fakeContext(` |
|
6349 |
+ FROM busybox |
|
6350 |
+ COPY asymlink /`, |
|
6351 |
+ map[string]string{ |
|
6352 |
+ "foo/abc": "bar", |
|
6353 |
+ "foo/def": "baz", |
|
6354 |
+ }) |
|
6355 |
+ c.Assert(err, checker.IsNil) |
|
6356 |
+ defer ctx.Close() |
|
6357 |
+ |
|
6358 |
+ err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink")) |
|
6359 |
+ c.Assert(err, checker.IsNil) |
|
6360 |
+ |
|
6361 |
+ id, err := buildImageFromContext(name, ctx, true) |
|
6362 |
+ c.Assert(err, checker.IsNil) |
|
6363 |
+ |
|
6364 |
+ out, _ := dockerCmd(c, "run", "--rm", id, "cat", "abc", "def") |
|
6365 |
+ c.Assert(out, checker.Matches, "barbaz") |
|
6366 |
+ |
|
6367 |
+ // change target file should invalidate cache |
|
6368 |
+ err = ioutil.WriteFile(filepath.Join(ctx.Dir, "foo/def"), []byte("bax"), 0644) |
|
6369 |
+ c.Assert(err, checker.IsNil) |
|
6370 |
+ |
|
6371 |
+ id, out, err = buildImageFromContextWithOut(name, ctx, true) |
|
6372 |
+ c.Assert(err, checker.IsNil) |
|
6373 |
+ c.Assert(out, checker.Not(checker.Contains), "Using cache") |
|
6374 |
+ |
|
6375 |
+ out, _ = dockerCmd(c, "run", "--rm", id, "cat", "abc", "def") |
|
6376 |
+ c.Assert(out, checker.Matches, "barbax") |
|
6377 |
+ |
|
6378 |
+} |
|
6379 |
+ |
|
6380 |
+// TestBuildSymlinkBasename tests that target file gets basename from symlink, |
|
6381 |
+// not from the target file. |
|
6382 |
+func (s *DockerSuite) TestBuildSymlinkBasename(c *check.C) { |
|
6383 |
+ testRequires(c, DaemonIsLinux) |
|
6384 |
+ name := "testbuildbrokensymlink" |
|
6385 |
+ ctx, err := fakeContext(` |
|
6386 |
+ FROM busybox |
|
6387 |
+ COPY asymlink /`, |
|
6388 |
+ map[string]string{ |
|
6389 |
+ "foo": "bar", |
|
6390 |
+ }) |
|
6391 |
+ c.Assert(err, checker.IsNil) |
|
6392 |
+ defer ctx.Close() |
|
6393 |
+ |
|
6394 |
+ err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink")) |
|
6395 |
+ c.Assert(err, checker.IsNil) |
|
6396 |
+ |
|
6397 |
+ id, err := buildImageFromContext(name, ctx, true) |
|
6398 |
+ c.Assert(err, checker.IsNil) |
|
6399 |
+ |
|
6400 |
+ out, _ := dockerCmd(c, "run", "--rm", id, "cat", "asymlink") |
|
6401 |
+ c.Assert(out, checker.Matches, "bar") |
|
6402 |
+ |
|
6403 |
+} |
... | ... |
@@ -1234,6 +1234,14 @@ func buildImage(name, dockerfile string, useCache bool, buildFlags ...string) (s |
1234 | 1234 |
} |
1235 | 1235 |
|
1236 | 1236 |
func buildImageFromContext(name string, ctx *FakeContext, useCache bool, buildFlags ...string) (string, error) { |
1237 |
+ id, _, err := buildImageFromContextWithOut(name, ctx, useCache, buildFlags...) |
|
1238 |
+ if err != nil { |
|
1239 |
+ return "", err |
|
1240 |
+ } |
|
1241 |
+ return id, nil |
|
1242 |
+} |
|
1243 |
+ |
|
1244 |
+func buildImageFromContextWithOut(name string, ctx *FakeContext, useCache bool, buildFlags ...string) (string, string, error) { |
|
1237 | 1245 |
args := []string{"build", "-t", name} |
1238 | 1246 |
if !useCache { |
1239 | 1247 |
args = append(args, "--no-cache") |
... | ... |
@@ -1244,9 +1252,13 @@ func buildImageFromContext(name string, ctx *FakeContext, useCache bool, buildFl |
1244 | 1244 |
buildCmd.Dir = ctx.Dir |
1245 | 1245 |
out, exitCode, err := runCommandWithOutput(buildCmd) |
1246 | 1246 |
if err != nil || exitCode != 0 { |
1247 |
- return "", fmt.Errorf("failed to build the image: %s", out) |
|
1247 |
+ return "", "", fmt.Errorf("failed to build the image: %s", out) |
|
1248 | 1248 |
} |
1249 |
- return getIDByName(name) |
|
1249 |
+ id, err := getIDByName(name) |
|
1250 |
+ if err != nil { |
|
1251 |
+ return "", "", err |
|
1252 |
+ } |
|
1253 |
+ return id, out, nil |
|
1250 | 1254 |
} |
1251 | 1255 |
|
1252 | 1256 |
func buildImageFromPath(name, path string, useCache bool, buildFlags ...string) (string, error) { |