-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Add support to --chown flag to ADD command (Issue #57) #1134
Conversation
@@ -534,14 +534,13 @@ func AddVolumePathToWhitelist(path string) { | |||
// 1. If <src> is a remote file URL: | |||
// - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have int64
here (and in GetUserGroup
), when there are uint32
everywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -66,7 +71,7 @@ func (a *AddCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.Bui | |||
return err | |||
} | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place conversions are needed seems to be here then. int64(uid), int64(gid)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two casts instead of four would be introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't take the time to read this in a bigger context. Good job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
resp, err := http.Get(rawurl) | ||
if err != nil { | ||
return err | ||
} | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you exploring unit tests!
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.go
as 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.