Add --chown flag to Dockerfile ADD and COPY by duglin · Pull Request #28499 · moby/moby

duglin · 2016-11-16T21:12:18Z

Carrying #27303 since Kara is off doing other exciting things.

What Kara did
Add a --chown flag to Dockerfile ADD and COPY instructions, proposed by Proposal: Implement --user flag for COPY and ADD #13600
How Kara did it
Added flag to add and disptachCopy of builder/dockerfile/dispatchers.go, and passed the relevant information through to the daemon which looks up the user/group using a new function on the container object.
How to verify it
Create a Dockerfile that uses the --chown flag on an ADD or COPY instruction, and check that the file is listed as having the appropriate owner
Description for the changelog
add --chown flag to Dockerfile ADD and COPY instructions

2nd commit is just a rebase.

Signed-off-by: Doug Davis dug@us.ibm.com

lowenna · 2016-11-18T22:49:56Z

daemon/oci_windows.go

See #28594 (comment).

I understand this solves duplication layer when changing owner, but it does not solve layer duplication needed for chmod +x (which is in my case needed on 60% of COPY/ADD in most images) ?

lauripiisang · 2016-12-21T14:49:46Z

/cc @thaJeztah I've seen you create similar issues and provide feedback, maybe you can help cc in the right people to get this merged ?

thaJeztah · 2016-12-22T19:07:28Z

Let's move this to code review

tianon · 2016-12-22T19:08:47Z

If --chmod might come later, would it make sense to call this --chown instead of --user to make it more clear what it's doing?

duglin · 2016-12-22T20:41:33Z

did someone suggest a chmod option? I missed that

tianon · 2016-12-22T20:43:17Z

It's been discussed a few times for similar reasons to wanting to control the user, and there's a comment on this PR asking about it specifically. :smile: #28499 (comment)

duglin · 2016-12-22T20:50:03Z

doi - sorry I missed that. I was scanning too quickly.

jrusiecki · 2016-12-24T07:55:04Z

I need ADD extension to use instead of:
run wget ... | chmod +x ...
which is superior (layer-wise) to

ADD ...
run chmod +x ...

Thus without +x option ADD is useless anyway (if I already have run... I put there also non-executables so will not use COPY/ADD even for --user option).
COPY would benefit from +x too (although file copied from linux is probably already +x) as it would improve quality by clearly stating +x so to limit errors.

ghost · 2016-12-25T04:05:17Z

+1

duglin · 2016-12-31T13:16:31Z

@tianon which would be better:
--user && --mode
or
--chown && --chmod
?

The 2nd feels a little like we're trying to expose our implementation detail (changing things after the fact vs setting the properties on the files during creation) - when the end user should just care about the properties having the right values regardless of how they are set.

tianon · 2017-01-03T03:59:11Z

Hmm, good point, but --user is already in use for changing the runtime user, so potentially confusing double usage. For what it's worth, rsync uses --chown and --chmod:

     --chown=USER:GROUP      simple username/groupname mapping
     --chmod=CHMOD           affect file and/or directory permissions

In my mind, the argument for --chown and --chmod is that normally, ADD/COPY try as hard as possible to preserve the source ownership/permissions, so the "change" implied by --chmod and --chown as the name is referring to the change which has to happen during the copy (ie, "copy XYZ, but change the owner to ABC").

glensc · 2017-01-03T04:28:47Z

i'm also proposing DEFATTR so that if you use multiple COPY or ADD you don't have to repeat the options:

DEFATTR chown=USER
DEFATTR chown=USER:GROUP
DEFATTR chown=UID
DEFATTR chown=UID:GID

chmod is a bit tricky because you definetely want different permissions for files and dirs:

DEFATTR chmod=a+rX
DEFATTR chmod=a+X

these would rise the x bit only if x bit is present for any of the users: chmod(1):

execute/search only if the file is a directory or already has execute permission for some user (X)

or more clearer directives for different kinds:

DEFATTR dir_chmod=0755 file_chmod=0644

thaJeztah · 2017-01-03T11:29:21Z

@glensc could you open a separate issue for that proposal? Also, would be worth describing how it should interact with different commands that take options (just for illustration, if LABEL would take --chown as an option).

duglin · 2017-01-03T13:10:48Z

@tianon ok - will go with --chown.

I'd prefer to leave --chmod for another PR because I think more thinking needs to go into that.

I have a high-order question for people.... if we were developing docker build from scratch would we have a --chown flag at all on ADD/COPY or would we have USER impact subsequent RUN, ADD and COPY commands? I'm asking because @glensc's latest comment got me wondering if normally people would have expected USER to have a broader impact than it does today. I'm not suggesting we break backwards compatibility, but I am wondering if instead of a flag on ADD/COPY we should consider a flag on USER instead - something like: USER --chown john:mygroup. This would mean use john:mygroup for all subsequent RUN commands (as well as the CMD) like it does today, but also use john:mygroup on all subsequent ADD/COPY commands too - which is probably what most newbies would have expected anyway.

To be clear, the --chown flag doesn't take any args, the john:mygroup is the normal USER arg. Perhaps --chown isn't the best word since it might be mentally parsed incorrectly, so perhaps --copy instead - dunno, I'm open to other words.

thoughts?

jrusiecki · 2017-01-07T15:02:09Z

I've copied my comments to moby/buildkit#4242.

jazoom · 2017-01-26T05:54:57Z

This still isn't in the docs. Is it used like this?

COPY from.txt /to.txt --chown someuser

cpuguy83 · 2017-01-26T14:07:11Z

@jazoom This is not merged.

dmytroleonenko · 2017-02-03T16:53:57Z

Looking for the solution for ages. Subscribed

dnephin · 2017-02-10T16:43:34Z

builder/dockerfile/internals.go

+		b.runConfig.Cmd = strslice.StrSlice(append(getShell(b.runConfig), fmt.Sprintf("#(nop) %s --chown=%s %s in %s ", cmdName, usergrp, srcHash, dest)))
+	} else {
+		b.runConfig.Cmd = strslice.StrSlice(append(getShell(b.runConfig), fmt.Sprintf("#(nop) %s %s in %s ", cmdName, srcHash, dest)))
+	}


These two branches are nearly identical. Could you move most of this line out of the branch, and leave only the inner most fmt.Sprintf() in the branch?

dnephin · 2017-02-10T16:45:02Z

builder/dockerfile/internals.go

+		comment = fmt.Sprintf("%s --chown=%s %s in %s", cmdName, usergrp, origPaths, dest)
+	} else {
+		comment = fmt.Sprintf("%s %s in %s", cmdName, origPaths, dest)
+	}


This looks awfully similar to the branch above (that I commented on).

How about a function for this? The #(nop) prefix could be an arg.

dnephin · 2017-02-10T17:02:11Z

container/container.go

+// uid:gid, username:gid, or uid:groupname and parses the passwd file in the container
+// to return the ExecUser referred to by `username`.
+func (container *Container) ParseUserGrp(username string) (*user.ExecUser, error) {
+	passwdPath, err := user.GetPasswdPath()


I believe this call always returns an err on windows, which means this new flag would not work for the windows builder.

Are we ok with that?

dnephin · 2017-02-10T17:10:00Z

Needs a rebase as well

dnephin · 2017-03-09T21:35:08Z

@duglin Thanks for working on this!

It seems this PR hasn't seen any activity in 4 weeks. I'm going to close the PR since there is no activity. If/when you have time to rebase, and respond to the comments please feel free to re-open it (or ping me and I can).

duglin · 2017-03-09T21:59:36Z

@dnephin this hasn't been touched in a while because people asked for the discussion to be moved over to: #30110

I can rebase now if we want but we really need #30110 to be done/discussed first.

dnephin · 2017-03-09T22:07:01Z

Ok, let's reopen once the discussion in #30110 has produced some results

italomaia · 2017-06-26T20:59:40Z

I'm unsure how #32816 is blocking this issue. To some extent, they seem unrelated as the problem of 32816 is related to the source, right? This PR seems so helpful it hurts to see it goes by closed for another week.

thaJeztah · 2017-07-20T18:31:18Z

reopening (again) following #30110 (comment), but probably needs a bit of rewriting

thaJeztah · 2017-07-20T18:31:33Z

(make that a lot of rewriting LOL)

estesp · 2017-07-24T14:35:21Z

I can carry this if that's helpful to @duglin?

duglin · 2017-07-24T14:36:35Z

go for it! thanks @estesp

estesp · 2017-07-26T16:34:26Z

Carried in #34263

GordonTheTurtle added the status/0-triage label Nov 16, 2016

duglin added impact/changelog impact/dockerfile status/1-design-review and removed status/0-triage labels Nov 16, 2016

duglin mentioned this pull request Nov 16, 2016

Add --user flag to Dockerfile ADD and COPY #27303

Closed

lowenna reviewed Nov 18, 2016

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Dec 22, 2016

duglin force-pushed the copyUser branch 5 times, most recently from bb9a873 to 9291fea Compare December 23, 2016 16:42

thaJeztah mentioned this pull request Dec 24, 2016

Permission denied for directories created automatically by Dockerfile ADD command #1295

Closed

duglin force-pushed the copyUser branch 2 times, most recently from b8790bf to beee26d Compare December 28, 2016 15:29

duglin mentioned this pull request Jan 12, 2017

USER and Layers Discussion #30110

Closed

GordonTheTurtle assigned dnephin Feb 10, 2017

dnephin reviewed Feb 10, 2017

View reviewed changes

griff mentioned this pull request Feb 14, 2017

Containerized app runs as root umweltdk/docker-node#1

Open

esergion mentioned this pull request Feb 27, 2017

permissions lambci/docker-lambda#19

Closed

romaindequidt mentioned this pull request Mar 5, 2017

Review from PR #5355 pascalgrimaud/generator-jhipster#9

Closed

dnephin closed this Mar 9, 2017

pjvleeuwen mentioned this pull request Mar 11, 2017

Spin-out builder #14298

Closed

2 tasks

thaJeztah mentioned this pull request Jun 13, 2017

Multiple COPY statements in one layer is not possible #33551

Closed

jtprobst mentioned this pull request Jun 23, 2017

Docker build cache does not ignore uid/gid on ADD or COPY #32816

Closed

imaia mentioned this pull request Jul 7, 2017

Docker Compose mounts named volumes as 'root' exclusively docker/compose#3270

Closed

italomaia mentioned this pull request Jul 11, 2017

Dockerfile: ADD does not honor USER: files always owned by root #6119

Closed

thaJeztah reopened this Jul 20, 2017

estesp mentioned this pull request Jul 26, 2017

Add --chown flag to ADD/COPY commands #34263

Merged

estesp closed this Jul 26, 2017

thaJeztah added the area/builder Build label Sep 15, 2020

Conversation

duglin commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lowenna Nov 18, 2016

Choose a reason for hiding this comment

Uh oh!

jrusiecki Nov 24, 2016

Choose a reason for hiding this comment

Uh oh!

lauripiisang commented Dec 21, 2016

Uh oh!

thaJeztah commented Dec 22, 2016

Uh oh!

tianon commented Dec 22, 2016

Uh oh!

duglin commented Dec 22, 2016

Uh oh!

tianon commented Dec 22, 2016 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duglin commented Dec 22, 2016

Uh oh!

jrusiecki commented Dec 24, 2016

Uh oh!

ghost commented Dec 25, 2016

Uh oh!

duglin commented Dec 31, 2016

Uh oh!

tianon commented Jan 3, 2017

Uh oh!

glensc commented Jan 3, 2017

Uh oh!

thaJeztah commented Jan 3, 2017

Uh oh!

duglin commented Jan 3, 2017

Uh oh!

jrusiecki commented Jan 7, 2017

Uh oh!

jazoom commented Jan 26, 2017

Uh oh!

cpuguy83 commented Jan 26, 2017

Uh oh!

dmytroleonenko commented Feb 3, 2017

Uh oh!

dnephin Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin commented Feb 10, 2017

Uh oh!

dnephin commented Mar 9, 2017

Uh oh!

duglin commented Mar 9, 2017

Uh oh!

dnephin commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

italomaia commented Jun 26, 2017

Uh oh!

thaJeztah commented Jul 20, 2017

Uh oh!

thaJeztah commented Jul 20, 2017

Uh oh!

estesp commented Jul 24, 2017

Uh oh!

duglin commented Jul 24, 2017

Uh oh!

estesp commented Jul 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

duglin commented Nov 16, 2016 •

edited

Loading

tianon commented Dec 22, 2016 via email •

edited

Loading

dnephin commented Mar 9, 2017 •

edited

Loading