Add support to --chown flag to ADD command (Issue #57)#1134
Add support to --chown flag to ADD command (Issue #57)#1134tejal29 merged 1 commit intoGoogleContainerTools:masterfrom dani29:master
Conversation
| // - destination will have permissions of 0600 | ||
| // - If remote file has HTTP Last-Modified header, we set the mtime of the file to that timestamp | ||
| func DownloadFileToDest(rawurl, dest string) error { | ||
| func DownloadFileToDest(rawurl, dest string, uid, gid int64) error { |
There was a problem hiding this comment.
Does it make sense to have int64 here (and in GetUserGroup), when there are uint32 everywhere else?
There was a problem hiding this comment.
in copy.go the ExecuteCommand method passes int64 uid and gid params to the util.CopyDir method, which passes this around to many other places such as fs_utils.go...
I'm not sure what should be the actual datatype (it probably doesn't matter), but making it consistent across the code-base would require touching many functions, and I am not sure if it's a good idea. WDYT?
| } | ||
| logrus.Infof("Adding remote URL %s to %s", src, urlDest) | ||
| if err := util.DownloadFileToDest(src, urlDest); err != nil { | ||
| if err := util.DownloadFileToDest(src, urlDest, uid, gid); err != nil { |
There was a problem hiding this comment.
The only place conversions are needed seems to be here then. int64(uid), int64(gid)
There was a problem hiding this comment.
I'm sorry, I'm not sure I'm following. Most methods in fs_util.go are expecting a int64 u/gid, with the exception of CreateFile (which is a bit weird IMO). Do you suggest to remove the casting from util.GetUserGroup() and perform it here? I am not sure what'd be the advantage of that.
There was a problem hiding this comment.
two casts instead of four would be introduced.
There was a problem hiding this comment.
I did not add the method GetUserGroup(), it was already there - I just moved it to the util file.
If it would return uint32 instead of int64, then in copy.go you'd need to perform casting twice, when passing these methods to util.CopyDir() & util.CopyFile().
There was a problem hiding this comment.
I didn't take the time to read this in a bigger context. Good job!
tejal29
left a comment
tejal29
left a comment
There was a problem hiding this comment.
One thing i would like to suggest is, Kaniko has a lot of integration tests which are a bottle neck right now.
We want to reserve integration tests for scenarios where
- we actually want to verify docker compatibility
( for each dockerfile in int test, we do a kaniko build and a docker build. Verify the results are same) - integrate with other third party libs
This seems like a good example to test with unit tests instead of Integration where we can verify if the added files have right user group.
e.g. here
You can set up a Test Dir and then Add with different chown group.
What do you think?
| defer resp.Body.Close() | ||
| // TODO: set uid and gid according to current user | ||
| if err := CreateFile(dest, resp.Body, 0600, 0, 0); err != nil { | ||
| if err := CreateFile(dest, resp.Body, 0600, uint32(uid), uint32(gid)); err != nil { |
There was a problem hiding this comment.
Thanks for addressing this TODO!
|
I have started writing the unit-tests but then realized we'll have a problem to run them in a predictable way, because we need to pass a valid UID and GID. Creating them on setup would be difficult (if impossible), but even if we tear down nicely, I'm not sure that's a flow we'd like to introduce to users running unit tests. And relying on some existing UID:GID might make these tests coupled to a specific platform. Sticking to integration test for now - I'd imagine this problem had/would come up in the future, especially when touching the file system or making certain syscalls. |
tejal29
left a comment
tejal29
left a comment
There was a problem hiding this comment.
Thank you exploring unit tests!
4 participants
Fixes #57
Description
Docker supports --chown flag on the ADD command, similarly to the COPY command (which is already implemented by Kaniko). The ADD command supports remote sources (URLs) and unpacking tarballs, whereas the COPY command only copies files from local context.
For local files, the ADD implementation relies on the COPY implementation, so it's just a matter of passing the Chown config. The PR adds support for remote files, whereas --chown does not affect extracted tar files.
I've moved the
GetUserGroup()method tocommand_util.goas it's now used by both COPY and ADD, hope it makes more sense.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Reviewer Notes
Release Notes
Describe any changes here so maintainer can include it in the release notes, or delete this block.