diff --git a/.github/workflows/unit-tests.yaml b/.github/workflows/unit-tests.yaml index be87b68356..44735b1a0c 100644 --- a/.github/workflows/unit-tests.yaml +++ b/.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 diff --git a/pkg/commands/add.go b/pkg/commands/add.go index 2ac4078c48..c1958c3faf 100644 --- a/pkg/commands/add.go +++ b/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") diff --git a/pkg/commands/add_test.go b/pkg/commands/add_test.go index 86eadb14cc..6d5bd905c9 100644 --- a/pkg/commands/add_test.go +++ b/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} diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index 2d5fefd1ec..ddbe6385b3 100644 --- a/pkg/commands/copy.go +++ b/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") diff --git a/pkg/commands/copy_test.go b/pkg/commands/copy_test.go index 3f979ee4b0..b7b59af7d8 100755 --- a/pkg/commands/copy_test.go +++ b/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() diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 794e0a9d1d..b9ac4f26a7 100644 --- a/pkg/executor/build.go +++ b/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 } } diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 02db6e76cd..156d4150ff 100644 --- a/pkg/executor/build_test.go +++ b/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") diff --git a/pkg/executor/cache_probe_test.go b/pkg/executor/cache_probe_test.go index 86b54848b2..c72c7abe43 100644 --- a/pkg/executor/cache_probe_test.go +++ b/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() diff --git a/pkg/executor/composite_cache.go b/pkg/executor/composite_cache.go index 40bf7f3d60..4777da9d3a 100644 --- a/pkg/executor/composite_cache.go +++ b/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 } diff --git a/pkg/executor/copy_multistage_test.go b/pkg/executor/copy_multistage_test.go index 949fa2f437..31f026036c 100644 --- a/pkg/executor/copy_multistage_test.go +++ b/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() diff --git a/pkg/util/command_util_test.go b/pkg/util/command_util_test.go index 180bff4c43..db63b41b9e 100644 --- a/pkg/util/command_util_test.go +++ b/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 { diff --git a/pkg/util/fs_util.go b/pkg/util/fs_util.go index d5b0307d59..9b641f1ff3 100644 --- a/pkg/util/fs_util.go +++ b/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 diff --git a/pkg/util/util.go b/pkg/util/util.go index 7501984fbe..fc3d4f5eb6 100644 --- a/pkg/util/util.go +++ b/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))) diff --git a/scripts/test.sh b/scripts/test.sh index 380d0e2896..489d1c1047 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -14,7 +14,7 @@ # 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 @@ -22,8 +22,13 @@ 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}"