Merge pull request #289 from bobcatfish/copy_copy_copy

Always snapshot files in COPY and RUN commands

README.md

@@ -29,6 +29,7 @@ We do **not** recommend running the kaniko executor binary in another image, as
 - [Security](#security)
 - [Comparison with Other Tools](#comparison-with-other-tools)
 - [Community](#community)
+- [Limitations](#limitations)
 
 _If you are interested in contributing to kaniko, see [DEVELOPMENT.md](DEVELOPMENT.md) and [CONTRIBUTING.md](CONTRIBUTING.md)._
 
@@ -256,7 +257,8 @@ To configure credentials, you will need to do the following:
 #### --snapshotMode
 
 You can set the `--snapshotMode=<full (default), time>` flag to set how kaniko will snapshot the filesystem.
-If `--snapshotMode=time` is set, only file mtime will be considered when snapshotting.
+If `--snapshotMode=time` is set, only file mtime will be considered when snapshotting (see
+[limitations related to mtime](#mtime-and-snapshotting)).
 
 #### --build-arg
 
@@ -356,3 +358,23 @@ provides.
 [kaniko-users](https://groups.google.com/forum/#!forum/kaniko-users) Google group
 
 To Contribute to kaniko, see [DEVELOPMENT.md](DEVELOPMENT.md) and [CONTRIBUTING.md](CONTRIBUTING.md).
+
+## Limitations
+
+### mtime and snapshotting
+
+When taking a snapshot, kaniko's hashing algorithms include (or in the case of
+[`--snapshotMode=time`](#--snapshotmode), only use) a file's
+[`mtime`](https://en.wikipedia.org/wiki/Inode#POSIX_inode_description) to determine
+if the file has changed. Unfortunately there is a delay between when changes to a
+file are made and when the `mtime` is updated. This means:
+
+* With the time-only snapshot mode (`--snapshotMode=time`), kaniko may miss changes
+ introduced by `RUN` commands entirely.
+* With the default snapshot mode (`--snapshotMode=full`), whether or not kaniko will
+ add a layer in the case where a `RUN` command modifies a file **but the contents do
+ not** change is theoretically non-deterministic. This _does not affect the contents_
+ which will still be correct, but it does affect the number of layers.
+
+_Note that these issues are currently theoretical only. If you see this issue occur, please
+[open an issue](https://github.com/GoogleContainerTools/kaniko/issues)._

integration/dockerfiles/Dockerfile_test_copy_same_file_many_times

@@ -0,0 +1,49 @@
+FROM alpine@sha256:5ce5f501c457015c4b91f91a15ac69157d9b06f1a75cf9107bf2b62e0843983a
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo

integration/integration_test.go

@@ -135,11 +135,9 @@ func TestMain(m *testing.M) {
  defer DeleteFromBucket(fileInBucket)
 
  fmt.Println("Building kaniko image")
- buildKaniko := exec.Command("docker", "build", "-t", ExecutorImage, "-f", "../deploy/Dockerfile", "..")
- err = buildKaniko.Run()
- if err != nil {
- fmt.Print(err)
- fmt.Print("Building kaniko failed.")
+ cmd := exec.Command("docker", "build", "-t", ExecutorImage, "-f", "../deploy/Dockerfile", "..")
+ if _, err = RunCommandWithoutTest(cmd); err != nil {
+ fmt.Printf("Building kaniko failed: %s", err)
  os.Exit(1)
  }
 

pkg/commands/volume.go

@@ -17,6 +17,7 @@ limitations under the License.
 package commands
 
 import (
+ "fmt"
  "os"
  "strings"
 
@@ -53,13 +54,14 @@ func (v *VolumeCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.
  return err
  }
 
- logrus.Infof("Creating directory %s", volume)
- if err := os.MkdirAll(volume, 0755); err != nil {
- return err
+ // Only create and snapshot the dir if it didn't exist already
+ if _, err := os.Stat(volume); os.IsNotExist(err) {
+ logrus.Infof("Creating directory %s", volume)
+ v.snapshotFiles = []string{volume}
+ if err := os.MkdirAll(volume, 0755); err != nil {
+ return fmt.Errorf("Could not create directory for volume %s: %s", volume, err)
+ }
  }
-
- //Check if directory already exists?
- v.snapshotFiles = append(v.snapshotFiles, volume)
  }
  config.Volumes = existingVolumes
 

pkg/commands/workdir.go

@@ -47,8 +47,14 @@ func (w *WorkdirCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile
  config.WorkingDir = filepath.Join(config.WorkingDir, resolvedWorkingDir)
  }
  logrus.Infof("Changed working directory to %s", config.WorkingDir)
- w.snapshotFiles = []string{config.WorkingDir}
- return os.MkdirAll(config.WorkingDir, 0755)
+
+ // Only create and snapshot the dir if it didn't exist already
+ if _, err := os.Stat(config.WorkingDir); os.IsNotExist(err) {
+ logrus.Infof("Creating directory %s", config.WorkingDir)
+ w.snapshotFiles = []string{config.WorkingDir}
+ return os.MkdirAll(config.WorkingDir, 0755)
+ }
+ return nil
 }
 
 // FilesToSnapshot returns the workingdir, which should have been created if it didn't already exist

pkg/executor/build.go

@@ -87,23 +87,41 @@ func DoBuild(opts *options.KanikoOptions) (v1.Image, error) {
  if err := dockerCommand.ExecuteCommand(&imageConfig.Config, buildArgs); err != nil {
  return nil, err
  }
- // Don't snapshot if it's not the final stage and not the final command
- // Also don't snapshot if it's the final stage, not the final command, and single snapshot is set
- if (!finalStage && !finalCmd) || (finalStage && !finalCmd && opts.SingleSnapshot) {
- continue
- }
- // Now, we get the files to snapshot from this command and take the snapshot
  snapshotFiles := dockerCommand.FilesToSnapshot()
- if finalCmd {
- snapshotFiles = nil
+ var contents []byte
+
+ // If this is an intermediate stage, we only snapshot for the last command and we
+ // want to snapshot the entire filesystem since we aren't tracking what was changed
+ // by previous commands.
+ if !finalStage {
+ if finalCmd {
+ contents, err = snapshotter.TakeSnapshotFS()
+ }
+ } else {
+ // If we are in single snapshot mode, we only take a snapshot once, after all
+ // commands have completed.
+ if opts.SingleSnapshot {
+ if finalCmd {
+ contents, err = snapshotter.TakeSnapshotFS()
+ }
+ } else {
+ // Otherwise, in the final stage we take a snapshot at each command. If we know
+ // the files that were changed, we'll snapshot those explicitly, otherwise we'll
+ // check if anything in the filesystem changed.
+ if snapshotFiles != nil {
+ contents, err = snapshotter.TakeSnapshot(snapshotFiles)
+ } else {
+ contents, err = snapshotter.TakeSnapshotFS()
+ }
+ }
  }
- contents, err := snapshotter.TakeSnapshot(snapshotFiles)
  if err != nil {
- return nil, err
+ return nil, fmt.Errorf("Error taking snapshot of files for command %s: %s", dockerCommand, err)
  }
+
  util.MoveVolumeWhitelistToWhitelist()
  if contents == nil {
- logrus.Info("No files were changed, appending empty layer to config.")
+ logrus.Info("No files were changed, appending empty layer to config. No layer added to image.")
  continue
  }
  // Append the layer to the image

pkg/snapshot/layered_map.go

@@ -17,6 +17,7 @@ limitations under the License.
 package snapshot
 
 import (
+ "fmt"
  "path/filepath"
  "strings"
 )
@@ -82,6 +83,20 @@ func (l *LayeredMap) MaybeAddWhiteout(s string) (bool, error) {
  return true, nil
 }
 
+// Add will add the specified file s to the layered map.
+func (l *LayeredMap) Add(s string) error {
+ newV, err := l.hasher(s)
+ if err != nil {
+ return fmt.Errorf("Error creating hash for %s: %s", s, err)
+ }
+ l.layers[len(l.layers)-1][s] = newV
+ return nil
+}
+
+// MaybeAdd will add the specified file s to the layered map if
+// the layered map's hashing function determines it has changed. If
+// it has not changed, it will not be added. Returns true if the file
+// was added.
 func (l *LayeredMap) MaybeAdd(s string) (bool, error) {
  oldV, ok := l.Get(s)
  newV, err := l.hasher(s)