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

merged 5 commits into from
Dec 23, 2021

Conversation

kvaps
Copy link
Contributor

@kvaps kvaps commented Aug 19, 2021 •
edited
Loading

Fixes #1315

Description

This PR solved two problems:

  • First problem is that uid, gid overriding by loop in CopyDir function:

    kaniko/pkg/util/fs_util.go

    Line 692 in 3ab2e1d

    uid, gid = DetermineTargetFileOwnership(fi, uid, gid)

  • Second problem is that otiai10Cpy.Copy is not preserving ownership information. Solved by implementing additional loop for copying ownership information.

It might need rebase after merging #1724.
This branch includes both fixes: kvaps:fix-copying-root-and-ownership; compiled docker images:

ghcr.io/kvaps/kaniko-executor:v1.6.0-fix
ghcr.io/kvaps/kaniko-executor:v1.6.0-fix-debug
ghcr.io/kvaps/kaniko-warmer:v1.6.0-fix

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

- Fix ownership setting for files and directories

@google-cla google-cla bot added the cla: yes CLA signed by all commit authors label Aug 19, 2021
@kvaps kvaps mentioned this pull request Aug 19, 2021
Closed
4 tasks
@kvaps kvaps changed the title Fix ownership Fix copying ownership Aug 19, 2021
@kvaps kvaps force-pushed the fix-ownership branch 4 times, most recently from 44802e6 to dfae5b9 Compare August 19, 2021 23:29
@kvaps kvaps mentioned this pull request Aug 22, 2021
Merged
@kvaps
Copy link
Contributor Author

kvaps commented Nov 2, 2021

PR rebased to v1.7.0, new docker images with this change prepared:

ghcr.io/kvaps/kaniko-executor:v1.7.0
ghcr.io/kvaps/kaniko-executor:v1.7.0-debug
ghcr.io/kvaps/kaniko-warmer:v1.7.0

source code of fork in master branch:
https://github.com/kvaps/kaniko/

@imjasonh imjasonh merged commit 7065921 into GoogleContainerTools:master Dec 23, 2021
imjasonh
imjasonh reviewed Dec 23, 2021
View reviewed changes
pkg/util/fs_util.go
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

gcalmettes pushed a commit to gcalmettes/kaniko that referenced this pull request Dec 24, 2021
@kvaps @gcalmettes-fbox
Fix copying ownership (GoogleContainerTools#1725)
819bc70 
* fix uid, gid overriding

* fix ownership for staging building

* add integration test

* add check for ignored files

* improve errors
@kruserr kruserr mentioned this pull request Dec 15, 2022
Open
@aaron-prindle aaron-prindle mentioned this pull request Jun 20, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imjasonh imjasonh imjasonh approved these changes

Assignees
No one assigned
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kaniko not preserving ownership on staging building
3 participants
@kvaps @imjasonh @tejal29