fix: ignore file ownership for copy from context (#29)

Fix for caching when context files permissions change. This is irrelevant for a COPY operation since the they are either copied as root:root, or a specific owner/group depending on COPY --chown= argument.

Relates to coder/terraform-provider-envbuilder#43

Co-authored-by: Cian Johnston <[email protected]>

Author: mafredri

.github/workflows/unit-tests.yaml

@@ -2,9 +2,9 @@ name: Unit tests
 
 on:
   push:
-    branches: ['main']
+    branches: ["main"]
   pull_request:
-    branches: ['main']
+    branches: ["main"]
 
 permissions:
   contents: read
@@ -13,8 +13,10 @@ jobs:
   tests:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 # v4.01
-      with:
-        go-version: '1.22'
-    - uses: actions/checkout@b0e28b5ac45a892f91e7d036f8200cf5ed489415 # v3
-    - run: make test
+      - uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 # v4.01
+        with:
+          go-version: "1.22"
+      - uses: actions/checkout@b0e28b5ac45a892f91e7d036f8200cf5ed489415 # v3
+      # Some unit tests need to be run as root to function correctly as they
+      # may attempt to run `chown`.
+      - run: sudo make test

pkg/commands/add.go

@@ -56,6 +56,15 @@ func (a *AddCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bui
 		chmod = fs.FileMode(0o600)
 	}
 
+	// All files and directories copied from the build context are created with a
+	// UID and GID of 0 unless the optional --chown flag specifies a given
+	// username, groupname, or UID/GID combination to request specific ownership
+	// of the copied content.
+	// See also: ./copy.go#L57
+	chownStr := a.cmd.Chown
+	if chownStr == "" {
+		chownStr = "0:0"
+	}
 	uid, gid, err := util.GetUserGroup(a.cmd.Chown, replacementEnvs)
 	if err != nil {
 		return errors.Wrap(err, "getting user group from chown")

pkg/commands/add_test.go

@@ -123,6 +123,9 @@ func setupAddTest(t *testing.T) string {
 }
 
 func Test_AddCommand(t *testing.T) {
+	if os.Getuid() != 0 {
+		t.Skip("Test requires root privileges as it attempts to chown")
+	}
 	tempDir := setupAddTest(t)
 
 	fileContext := util.FileContext{Root: tempDir}

pkg/commands/copy.go

@@ -53,7 +53,16 @@ func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bu
 	}
 
 	replacementEnvs := buildArgs.ReplacementEnvs(config.Env)
-	uid, gid, err := getUserGroup(c.cmd.Chown, replacementEnvs)
+
+	// All files and directories copied from the build context are created with a
+	// UID and GID of 0 unless the optional --chown flag specifies a given
+	// username, groupname, or UID/GID combination to request specific ownership
+	// of the copied content.
+	chownStr := c.cmd.Chown
+	if chownStr == "" && c.From() == "" {
+		chownStr = "0:0"
+	}
+	uid, gid, err := getUserGroup(chownStr, replacementEnvs)
 	logrus.Debugf("found uid %v and gid %v for chown string %v", uid, gid, c.cmd.Chown)
 	if err != nil {
 		return errors.Wrap(err, "getting user group from chown")

pkg/commands/copy_test.go

@@ -296,6 +296,9 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
 }
 
 func TestCopyExecuteCmd(t *testing.T) {
+	if os.Getuid() != 0 {
+		t.Skip("Test requires root privileges as it attempts to chown")
+	}
 	tempDir := setupTestTemp(t)
 
 	cfg := &v1.Config{
@@ -431,6 +434,9 @@ func Test_resolveIfSymlink(t *testing.T) {
 }
 
 func Test_CopyEnvAndWildcards(t *testing.T) {
+	if os.Getuid() != 0 {
+		t.Skip("Test requires root privileges as it attempts to chown")
+	}
 	setupDirs := func(t *testing.T) (string, string) {
 		testDir := t.TempDir()
 
@@ -495,6 +501,9 @@ func Test_CopyEnvAndWildcards(t *testing.T) {
 }
 
 func TestCopyCommand_ExecuteCommand_Extended(t *testing.T) {
+	if os.Getuid() != 0 {
+		t.Skip("Test requires root privileges as it attempts to chown")
+	}
 	setupDirs := func(t *testing.T) (string, string) {
 		testDir := t.TempDir()
 

pkg/executor/build.go

@@ -227,11 +227,20 @@ func (s *stageBuilder) populateCompositeKey(command commands.DockerCommand, file
 		}
 	}
 
+	// We want to avoid mutating the entire file context for the rest of its
+	// lifetime, just for adding to the cache key. Make a copy.
+	addPathCtx := s.fileContext
+	if f, ok := command.(interface{ From() string }); ok {
+		if f.From() == "" {
+			addPathCtx.IgnoreOwnerAndGroup = true
+		}
+	}
+
 	// Add the next command to the cache key.
 	compositeKey.AddKey(command.String())
 
 	for _, f := range files {
-		if err := compositeKey.AddPath(f, s.fileContext); err != nil {
+		if err := compositeKey.AddPath(f, addPathCtx); err != nil {
 			return compositeKey, err
 		}
 	}

pkg/executor/build_test.go

@@ -20,6 +20,7 @@ import (
 	"archive/tar"
 	"bytes"
 	"fmt"
+	"os"
 	"path/filepath"
 	"reflect"
 	"sort"
@@ -948,6 +949,7 @@ func Test_stageBuilder_build(t *testing.T) {
 		crossStageDeps     map[int][]string
 		mockGetFSFromImage func(root string, img v1.Image, extract util.ExtractFunction) ([]string, error)
 		shouldInitSnapshot bool
+		skipReason         string
 	}
 
 	testCases := []testcase{
@@ -1136,6 +1138,13 @@ func Test_stageBuilder_build(t *testing.T) {
 			}
 		}(),
 		func() testcase {
+			name := "copy command cache enabled and key is not in cache"
+			if os.Getuid() != 0 {
+				return testcase{
+					description: name,
+					skipReason:  "test requires root, attempts chown",
+				}
+			}
 			dir, filenames := tempDirAndFile(t)
 			filename := filenames[0]
 			tarContent := []byte{}
@@ -1174,7 +1183,7 @@ COPY %s foo.txt
 
 			cmds := stage.Commands
 			return testcase{
-				description: "copy command cache enabled and key is not in cache",
+				description: name,
 				opts:        opts,
 				config:      &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}},
 				layerCache:  &fakeLayerCache{},
@@ -1193,6 +1202,13 @@ COPY %s foo.txt
 			}
 		}(),
 		func() testcase {
+			name := "cached run command followed by uncached copy command results in consistent read and write hashes"
+			if os.Getuid() != 0 {
+				return testcase{
+					description: name,
+					skipReason:  "test requires root, attempts chown",
+				}
+			}
 			dir, filenames := tempDirAndFile(t)
 			filename := filenames[0]
 			tarContent := generateTar(t, filename)
@@ -1251,7 +1267,7 @@ COPY %s bar.txt
 
 			cmds := stage.Commands
 			return testcase{
-				description: "cached run command followed by uncached copy command results in consistent read and write hashes",
+				description: name,
 				opts:        &config.KanikoOptions{Cache: true, CacheCopyLayers: true, CacheRunLayers: true},
 				rootDir:     dir,
 				config:      &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}},
@@ -1267,6 +1283,14 @@ COPY %s bar.txt
 			}
 		}(),
 		func() testcase {
+			name := "copy command followed by cached run command results in consistent read and write hashes"
+			if os.Getuid() != 0 {
+				return testcase{
+					description: name,
+					skipReason:  "test requires root, attempts chown",
+				}
+			}
+
 			dir, filenames := tempDirAndFile(t)
 			filename := filenames[0]
 			tarContent := generateTar(t, filename)
@@ -1325,7 +1349,7 @@ RUN foobar
 
 			cmds := stage.Commands
 			return testcase{
-				description: "copy command followed by cached run command results in consistent read and write hashes",
+				description: name,
 				opts:        &config.KanikoOptions{Cache: true, CacheRunLayers: true},
 				rootDir:     dir,
 				config:      &v1.ConfigFile{Config: v1.Config{WorkingDir: destDir}},
@@ -1471,6 +1495,9 @@ RUN foobar
 	}
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
+			if tc.skipReason != "" {
+				t.Skip(tc.skipReason)
+			}
 			var fileName string
 			if tc.commands == nil {
 				file, err := filesystem.CreateTemp("", "foo")

pkg/executor/cache_probe_test.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"net/http/httptest"
 	"net/url"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -34,6 +35,9 @@ import (
 )
 
 func TestDoCacheProbe(t *testing.T) {
+	if os.Getuid() != 0 {
+		t.Skip("Test setup requires root privileges as it attempts to chown")
+	}
 	t.Run("Empty", func(t *testing.T) {
 		testDir, fn := setupCacheProbeTests(t)
 		defer fn()

pkg/executor/composite_cache.go

@@ -79,7 +79,7 @@ func (s *CompositeCache) AddPath(p string, context util.FileContext) error {
 	if context.ExcludesFile(p) {
 		return nil
 	}
-	fh, err := util.CacheHasher()(p)
+	fh, err := util.CacheHasher(context.IgnoreOwnerAndGroup)(p)
 	if err != nil {
 		return err
 	}
@@ -104,7 +104,7 @@ func hashDir(p string, context util.FileContext) (bool, string, error) {
 			return nil
 		}
 
-		fileHash, err := util.CacheHasher()(path)
+		fileHash, err := util.CacheHasher(context.IgnoreOwnerAndGroup)(path)
 		if err != nil {
 			return err
 		}

pkg/executor/copy_multistage_test.go

@@ -48,6 +48,9 @@ func readDirectory(dirName string) ([]fs.FileInfo, error) {
 }
 
 func TestCopyCommand_Multistage(t *testing.T) {
+	if os.Getuid() != 0 {
+		t.Skip("Test requires root privileges as it attempts to chown")
+	}
 	t.Run("copy a file across multistage", func(t *testing.T) {
 		testDir, fn := setupMultistageTests(t)
 		defer fn()