-
Notifications
You must be signed in to change notification settings - Fork 766
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 --chown option to add/copy #336
Conversation
Can one of the admins verify this patch?
|
@bertinatto Thanks for the PR/PoC. The change sounds good to me, but I'll let @nalind and @rhatdan give the definitive answer. If we do move forward, we'll want to add some tests and man page changes too. The test failure is not related to this change. @rhatdan the error is these two versions aren't matching:
Does buildah.spec need to have it's version bumped? It's set at 0.7 atm. |
bot, add author to whitelist |
Since this made it into Docker, I agree that it should be implemented. |
add.go
Outdated
|
||
// Set permissions. For now both uid and gid need to be set and they have to be numbers. | ||
if options.Chown[0] != "" && options.Chown[1] != "" { | ||
// TODO: figure out uid/id if user/group names was passed in instead of the real uid/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 believe there is code for this already written to handle passing of of USER string.
add.go
Outdated
//AddAndCopyOptions holds options for add and copy commands. | ||
type AddAndCopyOptions struct { | ||
// These are strings because I want to accept both ids and names. | ||
Chown [2]string |
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.
Since you have a struct, why not make it
USER string,
Group String,
add.go
Outdated
} | ||
|
||
if err := os.Chown(d, uid, gid); err != nil { | ||
return errors.Wrapf(err, "error setting permissions") |
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.
error changing ownership %s:%s", options.Chown[0], options.Chown[1]
Although I would prefer options.User, options,Group
cmd/buildah/addcopy.go
Outdated
addAndCopyFlags = []cli.Flag{ | ||
cli.StringFlag{ | ||
Name: "chown", | ||
Usage: "Set the user and group ownership of the file", |
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.
of the destination content.
cmd/buildah/addcopy.go
Outdated
@@ -52,7 +67,16 @@ func addAndCopyCmd(c *cli.Context, extractLocalArchives bool) error { | |||
return errors.Wrapf(err, "error reading build container %q", name) | |||
} | |||
|
|||
err = builder.Add(dest, extractLocalArchives, args...) | |||
options := buildah.AddAndCopyOptions{} | |||
if chown := c.String("chown"); chown != "" { |
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.
Why does this have to be two, I would think you should just do a
if c.String("chown") != "" {
Then split it. I don't think we need to care of group is not set.
--chown 1234
Should be ok, and then just leave the other component alone.
}
add.go
Outdated
@@ -69,6 +78,12 @@ func (b *Builder) Add(destination string, extract bool, source ...string) error | |||
} | |||
}() | |||
dest := mountPoint | |||
|
|||
uid, gid, err := findUserGroupIDs(mountPoint, options.Chown) |
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 think it'd be better to use getUser()
from user.go
here.
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.
Wow, too bad I hadn't see getUser()
before! Thanks for pointing this out.
f1724c4
to
dcb6c85
Compare
Thank you for the initial look! I've made a few adjustments, added a few tests and updated the man pages. Would you mind taking another look? The second commit is just to make the tests pass. I tried to make my code simpler to avoid this change, but I' afraid there isn't much I can do. Any suggestions? |
add.go
Outdated
@@ -68,6 +74,18 @@ func (b *Builder) Add(destination string, extract bool, source ...string) error | |||
logrus.Errorf("error unmounting container: %v", err2) | |||
} | |||
}() | |||
// Set ownership of the destination. If this information is |
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.
It's a fairly long time before the user variable is used from this point and a number of other errors that could happen before then. I'm just wondering if it would make sense to move this new block of code down to be run just before the for statement on line 121.
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.
That's right, I missed that!
tests/validate/gometalinter.sh
Outdated
@@ -16,6 +16,6 @@ exec gometalinter.v1 \ | |||
--disable=gotype \ | |||
--disable=gas \ | |||
--disable=aligncheck \ | |||
--cyclo-over=40 \ | |||
--cyclo-over=45 \ |
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.
Not sure what this change helps with?
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.
Commit 9227692 introduced a few if
s in the Add()
function, increasing it its "cyclomatic complexity". As a result, make validate
was failing with:
add.go:67::warning: cyclomatic complexity 41 of function (*Builder).Add() is high (> 40) (gocyclo)
I increased the limit in gometalinter.sh
only to make the tests pass, however, I know this isn't a proper solution. Any suggestions on how to address this?
add.go
Outdated
@@ -100,6 +106,18 @@ func (b *Builder) Add(destination string, extract bool, source ...string) error | |||
if len(source) > 1 && (destfi == nil || !destfi.IsDir()) { | |||
return errors.Errorf("destination %q is not a directory", dest) | |||
} | |||
// Find out which user/group should the destination belong to. |
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.
Create a separate function
func (b *Builder) user(options AddAndCopyOptions) (string, error) {
// If this information is passed in through --chown, use it;
// otherwise use the user/group as whom the container should run.
if options.Chown != "" {
return getUser(mountPoint, options.Chown)
}
return getUser(mountPoint, b.User())
}
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.
That's right! Thanks!
LGTM |
add.go
Outdated
@@ -100,6 +106,11 @@ func (b *Builder) Add(destination string, extract bool, source ...string) error | |||
if len(source) > 1 && (destfi == nil || !destfi.IsDir()) { | |||
return errors.Errorf("destination %q is not a directory", dest) | |||
} | |||
// Find out which user (and group) should the destination belong to. |
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.
nit of a nit. "should the destination" to "the destination should". But don't worry about it unless there are other needed changes.
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.
LGTM, minor nit.
add.go
Outdated
} | ||
if fi.IsDir() { | ||
err2 := filepath.Walk(path, func(p string, info os.FileInfo, we error) error { | ||
if err3 := os.Chown(p, int(user.UID), int(user.GID)); err3 != 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.
os.Chown()
follows symlinks, and since we're not doing any of this in a chrooted subprocess, we could open ourselves up to something like #66 again. I think using os.Lchown()
is a first step here.
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.
Hm, interesting perspective, thank you!
Edit: by the way, fixed.
Failing test seems to be unrelated to this patch:
|
@bertinatto I concur that it's unrelated. The fedora site seems to be hiccuping like crazy today. I've seen this error in other PR's this morning and I am having difficulty pulling kits from there outside of GitHub. |
Signed-off-by: Fabio Bertinatto <fbertina@redhat.com>
Cool, tests are passing now! |
Ok I will merge, the question is do we need more protection with the chown call then just lchown. |
@bertinatto thanks for this. |
📌 Commit b81b52d has been approved by |
⚡ Test exempted: pull fully rebased and already tested. |
Hi there,
I came across a permission error while I was automating the migration of a Redis setup into a container. Basically,
buildah copy
was copying my file into the container and leaving it owned byroot
.While I know that an extra
buildah run containerID chown redis:redis /etc/redis.conf
would solve that issue, I would love to have a--chown
option inbuildah add/copy
. I know that docker added something similar recently, but I haven't looked into their implementation.This PR is simply a PoC to to get a feedback from you. It still needs some improvements, so if you think I can go ahead, I'll look into adding in some tests, docs and refactor the code.
This is an example of how it works:
Thanks for looking into this!