Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix copying ownership #1725

Merged
merged 5 commits into from
Dec 23, 2021
Merged

Fix copying ownership #1725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a8055d8
fix uid, gid overriding
kvaps Aug 19, 2021
48489c9
fix ownership for staging building
kvaps Aug 19, 2021
0589cb2
add integration test
kvaps Aug 19, 2021
21f8bba
add check for ignored files
kvaps Aug 19, 2021
7f59238
improve errors
kvaps Aug 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions integration/dockerfiles-with-context/issue-1315/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
FROM alpine:3.11 as builder

RUN mkdir -p /myapp/somedir \
&& touch /myapp/somedir/somefile \
&& chown 123:123 /myapp/somedir \
&& chown 321:321 /myapp/somedir/somefile

FROM alpine:3.11
COPY --from=builder /myapp /myapp
RUN printf "%s\n" \
"0 0 /myapp/" \
"123 123 /myapp/somedir" \
"321 321 /myapp/somedir/somefile" \
> /tmp/expected \
&& stat -c "%u %g %n" \
/myapp/ \
/myapp/somedir \
/myapp/somedir/somefile \
> /tmp/got \
&& diff -u /tmp/got /tmp/expected
61 changes: 59 additions & 2 deletions pkg/util/fs_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ func CopyDir(src, dest string, context FileContext, uid, gid int64) ([]string, e
logrus.Tracef("Creating directory %s", destPath)

mode := fi.Mode()
uid, gid = DetermineTargetFileOwnership(fi, uid, gid)
uid, gid := DetermineTargetFileOwnership(fi, uid, gid)
if err := mkdirAllWithPermissions(destPath, mode, uid, gid); err != nil {
return nil, err
}
Expand Down Expand Up @@ -894,7 +894,64 @@ func CopyFileOrSymlink(src string, destDir string, root string) error {
}
return os.Symlink(link, destFile)
}
return otiai10Cpy.Copy(src, destFile)
err := otiai10Cpy.Copy(src, destFile)
if err != nil {
return errors.Wrap(err, "copying file")
}
err = CopyOwnership(src, destDir)
if err != nil {
return errors.Wrap(err, "copying ownership")
}
return nil
}

// CopyOwnership copies the file or directory ownership recursively at src to dest
func CopyOwnership(src string, destDir string) error {
return filepath.Walk(src, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if IsSymlink(info) {
return nil
}
relPath, err := filepath.Rel(filepath.Dir(src), path)
if err != nil {
return err
}
destPath := filepath.Join(destDir, relPath)

if CheckIgnoreList(src) && CheckIgnoreList(destPath) {
if !isExist(destPath) {
logrus.Debugf("Path %s ignored, but not exists", destPath)
return nil
}
if info.IsDir() {
return filepath.SkipDir
}
logrus.Debugf("Not copying ownership for %s, as it's ignored", destPath)
return nil
}
if CheckIgnoreList(destDir) && CheckIgnoreList(path) {
if !isExist(path) {
logrus.Debugf("Path %s ignored, but not exists", path)
return nil
}
if info.IsDir() {
return filepath.SkipDir
}
logrus.Debugf("Not copying ownership for %s, as it's ignored", path)
return nil
}

info, err = os.Stat(path)
if err != nil {
return errors.Wrap(err, "reading ownership")
}
stat := info.Sys().(*syscall.Stat_t)
err = os.Chown(destPath, int(stat.Uid), int(stat.Gid))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This failed a lint check for ineffective error assignment that wasn't present when you proposed this PR. In #1858 I tried just having this return the error, but it leads to a unit test failure:

time="2021-12-23T19:27:59Z" level=info msg="Saving file copied/bam.txt for later use"
    util.go:73: chown /tmp/592266814/kaniko/0/bam.txt: no such file or directory
        copying ownership
        github.com/GoogleContainerTools/kaniko/pkg/util.CopyFileOrSymlink
        	/home/runner/work/kaniko/kaniko/pkg/util/fs_util.go:910
        github.com/GoogleContainerTools/kaniko/pkg/executor.DoBuild
        	/home/runner/work/kaniko/kaniko/pkg/executor/build.go:680
        github.com/GoogleContainerTools/kaniko/pkg/executor.TestCopyCommand_Multistage.func1
        	/home/runner/work/kaniko/kaniko/pkg/executor/copy_multistage_test.go:47
        testing.tRunner
        	/opt/hostedtoolcache/go/1.17.5/x64/src/testing/testing.go:1259
        runtime.goexit
        	/opt/hostedtoolcache/go/1.17.5/x64/src/runtime/asm_amd64.s:1581
        could not save file
        github.com/GoogleContainerTools/kaniko/pkg/executor.DoBuild
        	/home/runner/work/kaniko/kaniko/pkg/executor/build.go:681
        github.com/GoogleContainerTools/kaniko/pkg/executor.TestCopyCommand_Multistage.func1
        	/home/runner/work/kaniko/kaniko/pkg/executor/copy_multistage_test.go:47
        testing.tRunner
        	/opt/hostedtoolcache/go/1.17.5/x64/src/testing/testing.go:1259
        runtime.goexit
        	/opt/hostedtoolcache/go/1.17.5/x64/src/runtime/asm_amd64.s:1581
    copy_multistage_test.go:52: open /tmp/592266814/output: no such file or directory

If you can figure out the fix that'd be great, otherwise I may have to revert this change to ensure we can keep the project moving forward. Sorry for the difficulty, we're trying to get the gears moving again.

Copy link
Contributor Author

@kvaps kvaps Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I'll take a look

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the test to pass with the change in fe94bcc but I'd love to have your feedback on whether that's the right way forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I have similar errors even without merging this PR (just in v1.7.0 for example)


INFO[0000] Using dockerignore file: ../../integration/.dockerignore
--- FAIL: Test_GetFSFromLayers_with_whiteouts_include_whiteout_enabled (0.00s)
    fs_util_test.go:1124: initializing filesystem ignore list: checking filesystem mount paths for ignore list: open /proc/self/mountinfo: no such file or directory
--- FAIL: Test_GetFSFromLayers_with_whiteouts_include_whiteout_disabled (0.00s)
    fs_util_test.go:1228: initializing filesystem ignore list: checking filesystem mount paths for ignore list: open /proc/self/mountinfo: no such file or directory
--- FAIL: Test_GetFSFromLayers_ignorelist (0.00s)
    fs_util_test.go:1319: initializing filesystem ignore list: checking filesystem mount paths for ignore list: open /proc/self/mountinfo: no such file or directory
--- FAIL: Test_GetFSFromLayers (0.00s)
    fs_util_test.go:1445: initializing filesystem ignore list: checking filesystem mount paths for ignore list: open /proc/self/mountinfo: no such file or directory

Copy link
Contributor Author

@kvaps kvaps Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, it seems problem was caused by architecture of my new M1 chip.

In x86_64 virtual machine all tests are passed with no problems 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this problem is caused by the fact that kaniko directory is overrided just for this unit test:

kaniko/pkg/executor/copy_multistage_test.go

Lines 167 to 168 in 7065921

config.RootDir = testDir
config.KanikoDir = fmt.Sprintf("%s/%s", testDir, "kaniko")

But this path is not actually ignored, thus util.CopyOwnership is trying to chown /tmp/592266814/kaniko/0/bam.txt but this file is actually located in staging directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess was wring, the relative path was calculated wrong.
Here is PR which fixes this #1859


return nil
})
}

func createParentDirectory(path string) error {
Expand Down