Fix #9 - add support for --chown flag to COPY #250

pmcq · 2018-07-24T16:03:38Z

To test, I've built an image locally and used it to run dockerfiles with COPY --chown flag succesfully. All existing unit tests pass, however I don't see any existing tests on util.CopyDir/CopyFile or CopySymlink that need modification.

Since the user running the testcases isn't going to be a root user, it wouldn't be allowed to chown a file to someone else, so any testcases I wrote around the chown logic was failing for permission denied reasons.

googlebot · 2018-07-24T16:03:43Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

priyawadhwa · 2018-07-24T22:19:49Z

Hey @pmcq , looks like you need to sign the CLA and fix some linting errors for travis to pass.

pmcq · 2018-07-25T16:28:38Z

Hi, sorry, I'm currently working with my company internally to get the corporate CLA signed but I think it could be a couple of days for this to get set up so I might have to put this on hold until that goes through. I can close this for now and re-open when that's finished or whatever you would prefer. Thanks.

priyawadhwa · 2018-07-25T16:59:06Z

No rush, feel free to keep it open if that's easier.

googlebot · 2018-08-03T17:35:34Z

CLAs look good, thanks!

…hared between USER and COPY commands. Update COPY commands to chown files/directories/symlinks appropriately

pmcq · 2018-08-06T17:30:27Z

Hi I believe the CLAs are all set and I fixed up some merge conflicts on my end

sharifelgamal · 2018-08-06T18:05:43Z

All of our integration tests are failing with an invalid userid error:

time="2018-08-06T17:58:48Z" level=info msg="Unpacking layer: 0" time="2018-08-06T17:58:48Z" level=info msg="Not adding /dev because it is whitelisted" time="2018-08-06T17:58:48Z" level=info msg="Not adding /etc/hostname because it is whitelisted" time="2018-08-06T17:58:48Z" level=info msg="Not adding /etc/hosts because it is whitelisted" time="2018-08-06T17:58:48Z" level=info msg="Not adding /proc because it is whitelisted" time="2018-08-06T17:58:48Z" level=info msg="Not adding /sys because it is whitelisted" time="2018-08-06T17:58:48Z" level=info msg="Not adding /var/run because it is whitelisted" time="2018-08-06T17:58:48Z" level=info msg="Taking snapshot of full filesystem..." time="2018-08-06T17:58:49Z" level=info msg="cmd: copy [context/foo]" time="2018-08-06T17:58:49Z" level=info msg="dest: foo" time="2018-08-06T17:58:49Z" level=error msg="user: invalid userid "

priyawadhwa · 2018-08-06T19:35:29Z

You can find instructions for running integration tests locally here!

pmcq · 2018-08-07T15:32:06Z

I don't have a GCP account but can start the process of setting that stuff up for integration tests then. I have run the normal unit tests, and built some images locally.

priyawadhwa · 2018-08-07T17:07:49Z

@pmcq thanks! let me know if you run into any issues

pmcq · 2018-08-16T22:02:14Z

I've fixed up the code a little so that the images are now being built correctly but I can't get the integration tests to pass locally. At the end of the build I keep getting a Failed to push message and a Get https://gcr.io/v2/token?scope.... exit status 1. I am able to manually docker push the image to my gcr.io registry, but I can't seem to get kaniko to pick up the auth token correctly in the integration tests. Any advice on this?

dlorenc · 2018-08-23T20:59:04Z

Is this ready to merge?

pmcq · 2018-08-31T22:41:03Z

I just performed another merge from master to fix some merge conflicts, all tests are still passing.

oleh-ozimok · 2018-11-14T11:32:02Z

Any news about this feature?

24el · 2018-11-15T13:44:32Z

Very useful feature

bobcatfish · 2018-11-16T22:42:14Z

Hey @pmcq ! We'd like to give this another review, looks like there's a conflict that needs to be updated? Let us know when we should take a look again! (or if you want us to resolve the conflicts and get the PR merged, that's fine too!)

naliferov · 2018-11-19T11:05:26Z

Wait for this feature!

pmcq · 2018-11-19T23:15:50Z

I'm taking a look at addressing the merge conflict and will try to re-push before I go on vacation this week.

priyawadhwa

Thanks for rebasing so quickly! I left a few comments.

priyawadhwa · 2018-11-20T16:00:59Z

pkg/util/command_util.go

+		return "", "", err
+	}
+
+	uidStr, gidStr, err := GetUserFromUsername(userStr, groupStr)


nit: you can return GetUserFromUsername(userStr, groupStr) here

priyawadhwa · 2018-11-20T16:03:23Z

pkg/util/fs_util.go

 // It returns a list of files it copied over
-func CopyDir(src, dest string) ([]string, error) {
+func CopyDir(src string, dest string, chownUID int, chownGid int) ([]string, error) {


nit: this can be simplified to (src, dest string, uid, gid int)

priyawadhwa · 2018-11-20T16:03:50Z

pkg/util/fs_util.go

 				return nil, err
 			}
 		} else if fi.Mode()&os.ModeSymlink != 0 {
 			// If file is a symlink, we want to create the same relative symlink
-			if err := CopySymlink(fullPath, destPath); err != nil {
+			if err := CopySymlink(fullPath, destPath, chownUID, chownGid); err != nil {


should these be uid and gid instead of chownUID and chownGid? I think with that change determineChownUIDGid won't need to be called again in CopySymlink

My thought here was since CopySymlink is exported it could be called in other places that didn't have the resolved uid/gid (or to allow callers to those functions to specify "keep uid the same", e.g. https://github.com/GoogleContainerTools/kaniko/pull/250/files#diff-a29f0f6c0a35589007a1c4e592f5c189R166) and that it would be less error-prone for callers if determineChownUIDGid was called within CopySymlink/CopyFile. I agree that expectation could be changed and I can change this if you'd like.

Hi @priyawadhwa - what do you think?

That makes sense -- we can keep the determineChownUIDGid in both places, but let's still pass in uid and gid in this function since it's a bit cleaner

okay I can do that but it would have to be int(uid) and int(gid) (since those values are uint32s but the other functions expect signed ints which determineChownUIDGid returns) - I wasn't sure if the type casting to signed was frowned upon or not.

priyawadhwa · 2018-11-20T16:04:06Z

pkg/util/fs_util.go

 				return nil, err
 			}
 		} else {
 			// ... Else, we want to copy over a file
-			if err := CopyFile(fullPath, destPath); err != nil {
+			if err := CopyFile(fullPath, destPath, chownUID, chownGid); err != nil {


same as above, should these be uid and gid?

priyawadhwa

This is looking really good! Could you just add to our copy integration test Dockerfile so that we can test this change?

priyawadhwa · 2018-11-30T17:42:28Z

pkg/util/fs_util.go

 				return nil, err
 			}
 		} else if fi.Mode()&os.ModeSymlink != 0 {
 			// If file is a symlink, we want to create the same relative symlink
-			if err := CopySymlink(fullPath, destPath); err != nil {
+			if err := CopySymlink(fullPath, destPath, chownUID, chownGid); err != nil {


That makes sense -- we can keep the determineChownUIDGid in both places, but let's still pass in uid and gid in this function since it's a bit cleaner

priyawadhwa · 2018-11-30T17:42:54Z

pkg/util/fs_util.go

+// If the given uid/gid are negative, returns the uid/gid from the
+// file info.
+// Otherwise returns the given uid/gid
+func determineChownUIDGid(fileInfo os.FileInfo, chownUID, chownGid int) (uint32, uint32) {


nit: let's call this determineChownUidGid

the linter actually raised errors when using uid or Uid here - https://travis-ci.org/GoogleContainerTools/kaniko/builds/457513881 I forgot to also look at renaming Gid to GID (which I guess it didn't check against)

priyawadhwa · 2018-11-30T17:43:04Z

pkg/util/command_util.go

+// for the corresponding user as a string, replacing any build arguments
+// and environment variables.  Group is not required to be specified, and
+// if left off, gid will be empty
+func GetUIDGidFromUserString(commandUserStr string, replacementEnvs []string) (string, string, error) {


nit: getUidGidFromUserString

pmcq · 2018-12-20T19:55:47Z

I actually don't think I will have any time soon to get this PR ready for approval - I wasn't able to get integration tests running (always get some auth issues with gcr) and will be busy with work/holidays. If someone else can pick this up that would be nice but I'll probably stick with my fork until then.

…s for chowning purposes, otherwise /etc/passwd isn't found, or wouldn't have the users/groups from the base image

Rowern · 2019-02-12T15:14:20Z

@pmcq do you think I can pick up where you left ? Or would you consider continuing to work on this ?
(It would be a game changer for us as our images do need to copy with specific owner)

bobcatfish · 2019-02-12T22:03:52Z

@pmcq do you think I can pick up where you left ? Or would you consider continuing to work on this ?
(It would be a game changer for us as our images do need to copy with specific owner)

@Rowern based on @pmcq 's last comment it sounds like you can go for it! :D 🎉

pmcq · 2019-02-12T23:58:40Z

Correct I will not be working on this any longer. There are also some problems I ran into with kaniko not unpacking base layers and since it uses /etc/passwd to resolve usernames you have to use numeric user IDs instead of strings or make another tweak - #477 (comment).

swistaczek · 2019-03-18T12:55:05Z

@Rowern is there any new PR where I can watch progress for this fix? It's really important for my team :).

huapox · 2019-05-13T15:01:59Z

How's it going?

googlebot · 2019-09-20T18:40:04Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tejal29 · 2019-09-20T18:45:17Z

Since @pmcq hasnt signed CLA from patrick@algorithmia.io I am going to close this pr and create another one.

pmcq force-pushed the copy-chown branch from 509fdfd to b74f415 Compare August 3, 2018 17:35

pmcq added 2 commits August 3, 2018 16:23

Refactor determining uid/gid to a separate method so that it can be s…

aa75407

…hared between USER and COPY commands. Update COPY commands to chown files/directories/symlinks appropriately

Run gofmt

8718b23

pmcq force-pushed the copy-chown branch from b74f415 to 8718b23 Compare August 3, 2018 23:26

sharifelgamal added the kokoro:run label Aug 6, 2018

kokoro-team removed the kokoro:run label Aug 6, 2018

Only look up uid/gid if chown string is specified

b45d033

priyawadhwa added the kokoro:run label Aug 20, 2018

kokoro-team removed the kokoro:run label Aug 20, 2018

Merge remote-tracking branch 'upstream/master' into copy-chown

5e166cd

container-tools-bot added the size/L label Aug 31, 2018

priyawadhwa added the kokoro:run label Aug 31, 2018

kokoro-team removed the kokoro:run label Aug 31, 2018

pmcq added 2 commits September 10, 2018 10:48

Merge remote-tracking branch 'upstream/master' into copy-chown

4d6d657

Remove unused imports

4a9590f

sharifelgamal added the kokoro:run label Sep 10, 2018

kokoro-team removed the kokoro:run label Sep 10, 2018

Merge branch 'master' into copy-chown

ee70ffc

pmcq added 2 commits November 20, 2018 07:27

Fixup call to CopyFile post-merge

4c09458

Rename some usage of uid -> UID for linter

2e9b0f0

priyawadhwa reviewed Nov 20, 2018

View reviewed changes

Style tweaks from CR feedback

128f24b

bobcatfish mentioned this pull request Nov 27, 2018

Support for --chown flag in ADD command #57

Closed

priyawadhwa reviewed Nov 30, 2018

View reviewed changes

pmcq closed this Dec 20, 2018

pmcq added 3 commits December 20, 2018 15:52

Merge remote-tracking branch 'upstream/master' into copy-chown

0da0bc2

Merge branch 'master' into copy-chown

e15d5dc

The rootfs needs to be unpacked in order to properly resolve uids/gid…

04ea1cd

…s for chowning purposes, otherwise /etc/passwd isn't found, or wouldn't have the users/groups from the base image

swistaczek mentioned this pull request Mar 18, 2019

Add support for --chown flag to COPY command #9

Closed

tequilarista added the help wanted Looking for a volunteer! label Aug 2, 2019

tejal29 reopened this Sep 20, 2019

Merge branch 'master' into copy-chown

84e4d8c

tejal29 closed this Sep 20, 2019

tejal29 mentioned this pull request Sep 20, 2019

COPY changes ownership of target directory #550

Closed

Fix #9 - add support for --chown flag to COPY #250

Fix #9 - add support for --chown flag to COPY #250

Conversation

pmcq commented Jul 24, 2018

googlebot commented Jul 24, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

priyawadhwa commented Jul 24, 2018

pmcq commented Jul 25, 2018

priyawadhwa commented Jul 25, 2018

googlebot commented Aug 3, 2018

pmcq commented Aug 6, 2018

sharifelgamal commented Aug 6, 2018

priyawadhwa commented Aug 6, 2018

pmcq commented Aug 7, 2018

priyawadhwa commented Aug 7, 2018

pmcq commented Aug 16, 2018 • edited Loading

dlorenc commented Aug 23, 2018

pmcq commented Aug 31, 2018

oleh-ozimok commented Nov 14, 2018

24el commented Nov 15, 2018

bobcatfish commented Nov 16, 2018

naliferov commented Nov 19, 2018

pmcq commented Nov 19, 2018

priyawadhwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyawadhwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmcq commented Dec 20, 2018

Rowern commented Feb 12, 2019

bobcatfish commented Feb 12, 2019

pmcq commented Feb 12, 2019

swistaczek commented Mar 18, 2019

huapox commented May 13, 2019 • edited Loading

googlebot commented Sep 20, 2019

tejal29 commented Sep 20, 2019

pmcq commented Aug 16, 2018 •

edited

Loading

huapox commented May 13, 2019 •

edited

Loading