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()

pkg/util/command_util_test.go

@@ -536,6 +536,7 @@ func TestGetUserGroup(t *testing.T) {
 		description  string
 		chown        string
 		env          []string
+		from         string
 		mockIDGetter func(userStr string, groupStr string) (uint32, uint32, error)
 		// needed, in case uid is a valid number, but group is a name
 		mockGroupIDGetter func(groupStr string) (*user.Group, error)
@@ -571,8 +572,8 @@ func TestGetUserGroup(t *testing.T) {
 			mockIDGetter: func(string, string) (uint32, uint32, error) {
 				return 0, 0, fmt.Errorf("should not be called")
 			},
-			expectedU: -1,
-			expectedG: -1,
+			expectedU: DoNotChangeUID,
+			expectedG: DoNotChangeGID,
 		},
 	}
 	for _, tc := range tests {

pkg/util/fs_util.go

@@ -100,8 +100,9 @@ var skipKanikoDir = func() otiai10Cpy.Options {
 }
 
 type FileContext struct {
-	Root          string
-	ExcludedFiles []string
+	Root                string
+	ExcludedFiles       []string
+	IgnoreOwnerAndGroup bool
 }
 
 type ExtractFunction func(string, *tar.Header, string, io.Reader) error

pkg/util/util.go

@@ -24,9 +24,7 @@ import (
 	"io"
 	"math"
 	"os"
-	"path/filepath"
 	"strconv"
-	"strings"
 	"sync"
 	"syscall"
 	"time"
@@ -96,7 +94,7 @@ type CacheHasherFileInfoSum interface {
 }
 
 // CacheHasher takes into account everything the regular hasher does except for mtime
-func CacheHasher() func(string) (string, error) {
+func CacheHasher(ignoreOwnerAndGroup bool) func(string) (string, error) {
 	hasher := func(p string) (string, error) {
 		h := md5.New()
 		fi, err := filesystem.FS.Lstat(p)
@@ -114,18 +112,7 @@ func CacheHasher() func(string) (string, error) {
 
 		h.Write([]byte(fi.Mode().String()))
 
-		// Cian: this is a disgusting hack, but it removes the need for the
-		// envbuilder binary to be owned by root when doing a cache probe.
-		// We want to ignore UID and GID changes for the envbuilder binary
-		// specifically. When building and pushing an image using the envbuilder
-		// image, the embedded envbuilder binary will most likely be owned by
-		// root:root. However, when performing a cache probe operation, it is more
-		// likely that the file will be owned by the UID/GID that is running
-		// envbuilder, which in this case is not guaranteed to be root.
-		// Let's just pretend that it is, cross our fingers, and hope for the best.
-		lyingAboutOwnership := !fi.IsDir() &&
-			strings.HasSuffix(filepath.Clean(filepath.Dir(p)), ".envbuilder.tmp")
-		if lyingAboutOwnership {
+		if ignoreOwnerAndGroup {
 			h.Write([]byte(strconv.FormatUint(uint64(0), 36)))
 			h.Write([]byte(","))
 			h.Write([]byte(strconv.FormatUint(uint64(0), 36)))

scripts/test.sh

@@ -14,16 +14,21 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 
 #set -e
 
 RED='\033[0;31m'
 GREEN='\033[0;32m'
 RESET='\033[0m'
 
+# Warn if the script is not running as root.
+if [[ $(id -u) != "0" ]]; then
+    trap 'echo "WARN: Not run as root. Some tests were skipped!" 1>&2' EXIT
+fi
+
 echo "Running go tests..."
-go test -cover -coverprofile=out/coverage.out -v -timeout 120s `go list ./... | grep -v integration` | sed ''/PASS/s//$(printf "${GREEN}PASS${RESET}")/'' | sed ''/FAIL/s//$(printf "${RED}FAIL${RESET}")/''
+go test -cover -coverprofile=out/coverage.out -v -timeout 120s $(go list ./... | grep -v integration) | sed ''/PASS/s//$(printf "${GREEN}PASS${RESET}")/'' | sed ''/FAIL/s//$(printf "${RED}FAIL${RESET}")/''
 GO_TEST_EXIT_CODE=${PIPESTATUS[0]}
 if [[ $GO_TEST_EXIT_CODE -ne 0 ]]; then
     exit $GO_TEST_EXIT_CODE
@@ -35,8 +40,7 @@ scripts=(
     "$DIR/../hack/gofmt.sh"
 )
 fail=0
-for s in "${scripts[@]}"
-do
+for s in "${scripts[@]}"; do
     echo "RUN ${s}"
     if "${s}"; then
         echo -e "${GREEN}PASSED${RESET} ${s}"