/cc @shykes @crosbymichael @unclejack @cpuguy83 @erikh @tiborvass

this will need documentation in builder.md if it passes core / UX review

@SvenDowideit I've documented the new UX in builder.md, PTAL?

Docs LGTM - @fredlf @jamtur01 @ostezer

Docs LGTM. Not sure I agree at all about not implementing ADD too but YMMV.

@jamtur01 If you look at the original proposal, it was decided that ADD will remain the same for compatibility reasons (@shykes and others agreed on this on IRC) -- and would eventually be deprecated. We were very lucky that the time between the addition of COPY and this proposal was fairly small, because you don't have a strong backward compatibility incentive for relatively new instructions.

@cyphar Yeah I read the proposal - just don't agree. :)

@jamtur01 Haha, fair enough. You can bring it up with @shykes on IRC (or here), I don't really mind (personally I think this was a somewhat fair compromise between breaking backward compatibility and having an inconsistent UI -- though changing both ADD and COPY would be the best case scenario).

@cyphar Yeah the track record of deprecation hasn't been good and ADD is used extensively in examples, Dockerfiles, etc. I sense a very very long period in future where we'll have two instructions that look the same but behave subtly differently - which is support annoyance. :(

Seems early to review text / docs but LGTM.

FWIW, the change required to make ADD follow this functionality is just this trivial patch:

From e3a7e237b3752f02161546143f88fb9d2139c245 Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <cyphar@cyphar.com>
Date: Tue, 11 Nov 2014 01:49:55 +1100
Subject: [PATCH] builder: dispatchers: change ADD to chown resultant files
 like COPY

This patch modifies ADD such that files and directories added using the
ADD instruction will be owned by the current runconfig user (just like
files copied with COPY are owned by the current runconfig user).

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> (github: cyphar)
---
 builder/dispatchers.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builder/dispatchers.go b/builder/dispatchers.go
index 1c7b1a3..9cd3db5 100644
--- a/builder/dispatchers.go
+++ b/builder/dispatchers.go
@@ -70,7 +70,7 @@ func add(b *Builder, args []string, attributes map[string]bool, original string)
                return fmt.Errorf("ADD requires at least two arguments")
        }

-       return b.runContextCommand(args, true, true, false, "ADD")
+       return b.runContextCommand(args, true, true, true, "ADD")
 }

 // COPY foo /path
-- 
2.1.3

Docs LGTM, but agree with @jamtur01 re: consistency.

+1

While I think the switch to use the USER instead of root is probably the right thing to do, I think ADD and COPY should act the same way. As a newbie I had no clue what the difference was between the two (except for the remote-ness of ADD) and I would switch between the two - I doubt others aren't in the same boat. Having said that, this would break backwards compat and I think it may need to wait for the next version - or until the next rev of the parser which will allow for us to totally "fix" the Dockerfile commands w/o breaking backwards compat.

@duglin FWIW, I do agree somewhat. I've pinged @shykes to give us his opinion (he was the main proponent of only modifying COPY). But the main problem I have with changing ADD is that the magic that ADD does makes simple features like this very difficult to implement "correctly".

@cyphar I'd love to see this enabled for ADD as well. However, when ADD-ing a .tar.gz (with the patch), the ownerships saved within the archive are preserved.

@kdomanski That's an example of the "magic" that ADD does that starts to make the implementation of simple features far too complicated.

fwiw, I agree with @cyphar that ADD (which will inevitably be deprecated and/or removed) should not be affected. Last I heard, we want to steer people towards COPY and their own tar handling anyway.

On Nov 11, 2014, at 6:45 AM, Aleksa Sarai notifications@github.com wrote:

@kdomanski https://github.com/kdomanski That's an example of the "magic" that ADD does that starts to make the implementation of simple features far too complicated.

—
Reply to this email directly or view it on GitHub #9046 (comment).

Sorry for the wait on this. LGTM, nice and clean.

I'm not sure if this is still valid. When the proposal was crated and approved COPY was not released right? Because this was just now opened COPY has already been in a release so this would be a breaking change for that one.

Hehe, breaking changes, breaking changes everywhere...
I'm personally vote for having this behavior, because current is unnatural and weird.

@crosbymichael COPY had already been released at the time, IIRC. Also, the wait was because of the 2 months it took to merge docker-archive/libcontainer#158.

Yes it took two months because it was a large refactor of a critical part and we had a few other things that had more priority to take care of first. Sorry that it took so long but sometimes we have to prioritize things, let us know if there is a time constraint or if something is a blocker if you run into this again.

@shykes do you think it still makes sense to change COPY to honor the USER's ids?

Looks like this could use some documentation on the new behavior!

Updated the patches to some libcontainer changes, rebased and added the new documentation. However, I'm not going to track this PR and consistently add new doc changes ad infinitum. I am not going wait out the next few centuries for a code review that isn't going to happen.

@shykes @crosbymichael If you have an issue with this PR or proposal that hasn't been raised and responded to, please put it forward. If you have no issues with it, please LGTM and merge it. If you don't like it on a fundamental level, please say so and close it.

@cyphar I already approved the design. Up to @crosbymichael and the other maintainers to merge to close the implementation.

@cyphar update, the discussion has been re-opened since my initial approval of the design was based on the fact that COPY was fairly recent and not heavily used, which could justify making an exception to reverse compatibility. However, since unfortunately the implementation review has been in the backlog for over 50 days... We are discussing whether that exception is still justified.

If you're interested the discussion is taking place on #docker-maintainers in Freenode (it's open for everyone to listen in).

@shykes I'm not entirely sure that I agree with your strong view on what "backward compatibility" entails (nor the lengths to which it seems to be pushed on developers). First of all, this change is only a breaking change for essentially one use case -- COPYing a file with a USER set and not intending to chown it later. This is not what I would call a "common" use case, and I don't understand why we should pander to something which (AFAIK) is not commonly used (nor recommended anywhere).

However, even if this was a common use case, Docker is a versioned piece of software! Docker binaries have versions and you state changes between versions in a change log. Why not just say "The COPY directive in Dockerfiles has been updated to provide a more clean and intuitive user experience. As of this version, COPY will change the owner of any files copied to the currently set USER. We recommend that you test (and make the required changes to) your Dockerfiles before upgrading to this version."? That way, people are informed that there has been a change to the UX and that they should upgrade to the new version (with the old UX being a subset of features of the new UX).

The problem with having such a strong aversion to potential backward compatibility issues is that you end up with what I dub "the PHP problem" (see: the mysql_escape_string and mysql_real_escape_string fiasco). Strong backward compatibility requirements for features that were not correctly designed results in things like REALCOPY and COPYFORREALTHISTIMEWEPROMISE. It's just not sustainable to require that all features (particularly ones that were written with little to no actual planning) must be backward compatible until the end of time or (more worryingly) that you need to add syntax that supersedes the old syntax but is also backward compatible with it.

As for the delay between the proposal acceptance and the merging of the required patches, my patchsets were ready within a week of the proposal being accepted. The reason for the delay was delay by the libcontainer maintainers in merging the libcontainer patches. I'm not blaming them or you, I understand that the libcontainer patches were quite large and touched a lot of things.

The point I'm trying to make is that such strong backward compatibility requirements for features that were essentially written on a dare does not make sense and it stops features which people have clearly stated they are in favour of (and in some cases state they need in order to effectively use Docker).

I would also add that I'm totally +1 on this very small surface breaking change, and my reasoning is simple. Today, doing USER and then COPY will almost never do what you expect. Since the files are owned by root:root and have whatever permissions applied to them that were applied in the source, it's also very possible and likely that the given USER can't even read them or use them in a meaningful way. I think this is one of the biggest reasons for the COPY+RUN chown -R ... pattern, which leads to potential AUFS issues.

This fix still seems like a very clean solution to the problem IMO, without introducing any new syntax or even any new ideas (since instructions like WORKDIR and USER are already defined as having an effect on subsequent commands).

Although I applaud the effort to preserve BC, I agree with @cyphar and @tianon here; I think this is more a bugfix than a new feature. Given the number of issues I've seen coming by, I think others see it that way as well.

Two points;

We recommend that you test (and make the required changes to) your Dockerfiles before upgrading to this version."?

You touch a problematic case here; environments where the user has no control over the version being used. Most prominent being automated builds on the registry; afaik there's no way for users to tell what version of docker is used on the registry and (since Dockerfiles are not versioned), there's no way to control behavior there.

_Process_
I'm not really sure what the follow up to the decision on #docker-maintainers will be, but if the decision on IRC is final, does that match the guidelines (and #9137) (PR -> discuss -> approve/reject)? To be clear; I understand some discussions need some speed-up, but a new syntax In the Dockerfile without a PR first seems odd. Will an alternative PR to this, with the proposed design changes, be created?

@thaJeztah You could write it as "Please be wary of this when you upgrade to the new Docker version". Again, the potential use cases where this would break their images are far from common (and I would claim are bordering on unsafe anyway).

Just for the purpose of demonstration, this is the one (and practically only) use case where this patchset "breaks" backward compatibility:

USER 1337
COPY somefile .
# NOTE: at this point, the runconfig user probably can't use the file just added anyway
USER root
# Use the file just copied.
# ... without `chown 1337 ./somefile`.

I don't know about anyone else here, but the above Dockerfile looks badly written and I don't see why we should pander to this use case which (AFAIK) isn't actually done anywhere ... reputable.

@cyphar I'm with you there! And thanks for adding the example. I mentioned the registry scenario because it is something to consider when making a decision. In this case, I think the scenario is indeed very rare. (Perhaps add an example to the docs for those who need the old behavior?)

We've changed the behavior of instructions in the past and that has caused problems. Even when there wasn't a problem as an actual regression, users who were relying on the previous behavior were unhappy.

ADD was an instruction which had way too much magic and functionality to it. COPY was introduced to start making behavior more explicit: COPY would only operate on local files, not on remote files. The discussion continued even back then to figure out what to do when it comes to ownership of the files and to extracting (or not extracting) archives.

Another issue I've run into with RUN was that I had to specify something like

USER foo
RUN something
USER root
RUN something-else-which-needs-root-and-must-run-after-something
USER foo
RUN the-final-thing-to-run

There was a long discussion last week and the conclusion was that we need to start making instructions flexible via instruction configuration.

The general syntax for instruction configuration will look like this:

a=1 b=2 OP [ARG...]

This will make Dockerfile instructions more flexible.

The configuration for COPY will look like this:

user=<user id or username> COPY . /home/someuser

USER will not influence COPY instructions, thus preserving the current behavior Docker has had for a few releases.

I'm going to carry this PR to master using the instruction configuration described above.

@cyphar Please let me know if you'd like to help with that. Given how much time you've put into rebasing and maintaining this PR, I'd be more than happy to take care of getting it merged into master. At the same time, I understand if you wish to help with this.

Having some way to configure the behavior of instructions can offer some nice new possibilities. I'm not very fond on the proposed notation, as it really looks odd compared to the existing instructions.

Having to add a USER xx, IMO, isn't really less clean than putting the same thing in front of an instruction. And having a (possibly growing list of) configurations before each instructions makes the Dockerfile really hard to read.

If white-space is going to be allowed in configuration-values, we might even end up with;

user=COPY \
group=COPY-EDITORS \
mode=755 COPY /foo /bar

I think adding the configuration syntax without deprecating USER will confuse people, because there are now two possible ways to change user, three if shell commands are taken into account. To use your original example, and add a COPY and ADD to that;

user=foo COPY /some/file /to/here
ADD http://www.example.com/archive.tar.gz /app/
USER root
RUN chown -r foo /app && chmod -r 755 /app
USER foo
RUN /app/something
USER root
RUN something-else-which-needs-root-and-must-run-after-something
USER foo
RUN the-final-thing-to-run

So there's not going to be an explicit proposal for this new syntax? I have to be honest, I really, really hate the way it flows, and find that visually, it really disrupts the focus of the Dockerfile away from the actual instructions.

I will ensure there is a proposal before any commits start flying.

On Mon, 2015-01-05 at 13:23 -0800, Tianon Gravi wrote:

So there's not going to be an explicit proposal for this new syntax? I
have to be honest, I really, really hate the way it flows, and find
that visually, it really disrupts the focus of the Dockerfile away
from the actual instructions.

—
Reply to this email directly or view it on GitHub.

@tianon is right, this needs a proposal and this discussion does not
belong in this issue.

@thaJeztah since you seem to have a clear understanding of what needs to
happen, can you bootstrap a proposal that describes the existing
proposed interface and engineer it in a way that makes it amenable to
modification so we can discuss it? It would be insanely helpful and a
great way to contribute to the project.

If you do this, please follow the docs-PR proposal process. Thanks!

-Erik

On Mon, 2015-01-05 at 13:07 -0800, Sebastiaan van Stijn wrote:

Having some way to configure the behavior of instructions can offer
some nice new possibilities. I'm not very fond on the proposed
notation, as it really looks odd compared to the existing
instructions.

Having to add a USER xx, IMO, isn't really less clean than putting the
same thing in front of an instruction. And having a (possibly growing
list of) configurations before each instructions makes the Dockerfile
really hard to read.

If white-space is going to be allowed in configuration-values, we
might even end up with;

user=COPY
group=COPY-EDITORS
mode=755 COPY /foo /bar

I think adding the configuration syntax without deprecating USER will
confuse people, because there are now two possible ways to change
user, three if shell commands are taken into account. To use your
original example, and add a COPY and ADD to that;

user=foo COPY /some/file /to/here
ADD http://www.example.com/archive.tar.gz /app/
USER root
RUN chown -r foo /app && chmod -r 755 /app
USER foo
RUN /app/something
USER root
RUN something-else-which-needs-root-and-must-run-after-something
USER foo
RUN the-final-thing-to-run

—
Reply to this email directly or view it on GitHub.

@erikh so, basically a proposal based on the discussion on #docker-maintainers?

Yup, can probably do that tomorrow (it's now close to midnight here)

Cool, that'd be perfect! Thank you.

-Erik

On Mon, 2015-01-05 at 15:02 -0800, Sebastiaan van Stijn wrote:

@erikh so, basically a proposal based on the discussion on
#docker-maintainers?

Yup, can probably do that tomorrow (it's now close to midnight here)

—
Reply to this email directly or view it on GitHub.

I'm going to close this one as we don't want it merged, it will change the semantics of how COPY works and will causes issues for people.

@cyphar sorry about all of this. We didn't know that your other PRs were time sensitive for when COPY was introduced in order to make this change.

We can continue the discussion on the new proposed changes elsewhere but I think it's safe to close this PR now.