-
Notifications
You must be signed in to change notification settings - Fork 645
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
Fixed permissions on items in the context tar sent to docker daemon t… #477
Conversation
} | ||
|
||
// There is probably a better way to see if we're running on windows... but this should work. | ||
if (File.separatorChar == '\\') { |
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.
What's about System.getProperty("os.name").toLowerCase().contains("windows")
?
Thanks for your PR ! Could you please describe the problem you have with the permissions as they are now ? |
Windows filesystems have no concept of an "executable" permission. In windows files either are or aren't executable based on their filename extension and the list of extensions configured to be executable. When building a linux container on windows which includes and/or uses shell scripts and those shell scripts are copied from the windows filesystem there is no way to preserve or establish that the script should be marked as executable in the docker image. This can result in images that may not function when built from windows. The docker team recognized this problem and had a discussion on how best to proceed and they chose to synthesize the executable permissions on all files when building on Windows. This is done by setting the permissions in the tar file which is uploaded to the daemon. This PR duplicates that logic when running on windows. It should have no affect on the tar archive from other platforms. This fixes the underlying problem that forced the workaround of using |
Thanks a lot for the explanation. My point here is, that the So it is definitely not optimal when this tar is created differently on different systems because this make the build non-reproducable. If there is no other solution (and according to the issues you referenced), I would go and disable by default Also I would print out a big fat warning on Windows that all files are now executable (as Docker does). Another alternative could be to allow an explicit configuration of the permission which then is evaluated for setting the permissions on individual files. Probably a bit tedious to create and maintain. Any thoughts on this ? |
The lack of executable permissions already causes the sources tar to not be reproducible across platforms. So instead of assuming nothing is executable this assumes everything is executable. Perhaps there aren't often scripts in the sources tar but if there are they otherwise wouldn't be executable when produced on windows. How you want to handle that is something that can be discussed or we can continue to let there be a rift between the permissions when building on windows. We can definitely add a warning similar to Docker's warning when building on Windows. We could have this logic be on by default on windows but configurable as well. If you wanted reliably reproducible builds you could turn it on for all platforms, I guess. |
I made the changes we discussed. You can configure with:
On Windows the default is true, all other platforms the default is false. I've also added a warning when the permissions are normalized as well. |
…o support Windows and mirror what the docker client itself does.
Thanks a lot, just reviewing the PR. One question, though: In the assembly descriptor you can explicitly set a mode (see http://maven.apache.org/plugins/maven-assembly-plugin/component.html#class_fileSet). This mode is supposed to end up in the tar archive, too (overruling file permission found on the source files). Wouldn't this be also a solution when you set the |
This should work for any assembly It won't work also not when using Dockerfiles directly since there is no assembly involved (and the tar archive is created directly from the filesystem). |
If you would like to try, you can test the sample project |
That said, I merged in your code. You can switch on the behaviour with |
Glad to see it in there. It is possible, and something my Mac & Linux using coworkers have setup, to include files in the build context by putting them in the filesystem prior to the docker-maven-plugin execution. I believe they are invoking the maven-assembly-plugin directly. The problem with doing that, at least the way that the plugin used to work, was that after they did this the docker-maven-plugin would build the context tar around their files and would loose all execution permissions on windows. The sample project works fine, and does so because the script file is added to the build context by the docker-maven-plugin invoking the assembler. |
My point was, that if you don't rely on picking up the permission from the filesystem but specify it directly in the configuration by setting it in the assembly descriptor then you can avoid this hackish workaround. However you then need a special workflow with using an assembly which does not suit everybody (i.e. not those using the Dockerfile mode) but leads to reproducible images regardless whether you build it from Windows or another OS. |
…o support Windows and mirror what the docker client itself does.