Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

techjoshua
Copy link

…o support Windows and mirror what the docker client itself does.

}

// There is probably a better way to see if we're running on windows... but this should work.
if (File.separatorChar == '\\') {
Copy link
Collaborator

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") ?

@rhuss
Copy link
Collaborator

rhuss commented Jun 9, 2016

Thanks for your PR !

Could you please describe the problem you have with the permissions as they are now ?

@techjoshua
Copy link
Author

techjoshua commented Jun 9, 2016

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 <mode>tar</mode> in the build to preserve execute permissions which had been granted by the assembly plugin. It also works on files outside the /maven directory, allowing them to be referenced by the docker file. For me this is why this fix is so important. My co-workers have already setup practices around using your plugin across multiple projects but the images that they were producing on windows weren't working due to this issue.

@rhuss
Copy link
Collaborator

rhuss commented Jun 9, 2016

Thanks a lot for the explanation. My point here is, that the docker.tar created in the step can also be used as an artefact in the Maven repository with the docker:source goal. As this it can be versioned and distributed over the Maven repository and e.g. build as binary input for an OpenShift build.

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 docker:source on Windows (which can be switched on by a property) to let only unix system create a deployable source artefact. Of course, there this is still fragile /wrt to umask setting etc. and depends on the checkout. But I think the risk here is much less.

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 ?

@techjoshua
Copy link
Author

techjoshua commented Jun 9, 2016

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.

@techjoshua
Copy link
Author

I made the changes we discussed. You can configure with:

<build>
    <normalizePermissions>true|false|auto</normalizePermissions>
</build>

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.
@rhuss
Copy link
Collaborator

rhuss commented Jun 26, 2016

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 fileMode to 755 for the affected scripts ?

@rhuss
Copy link
Collaborator

rhuss commented Jun 26, 2016

This should work for any assembly mode except dir. You can set the assembly mode with <mode> in the <assembly> configuration section.

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).

rhuss pushed a commit that referenced this pull request Jun 26, 2016
…aemon to support Windows and mirror what the docker client itself does.

Introduced <permissions> for setting various permission related options.
This extends #477, and allows on windows systems to set the execution bit on all files. Please follow the discussion in #477 for more details.
@rhuss
Copy link
Collaborator

rhuss commented Jun 26, 2016

If you would like to try, you can test the sample project samples/data-jolokia-demo with the latest version on the intgeration branch on Windows. The created docker.tar (can be found in target/docker/..../tmp/) contains a hello.sh with exec permissions, even without moving everything with permission x.

@rhuss
Copy link
Collaborator

rhuss commented Jun 26, 2016

That said, I merged in your code. You can switch on the behaviour with <permissions>auto</permissions> (or <permissions>exec</permissions>). The default is still keep (for backwards compatibility)

@rhuss rhuss closed this Jun 26, 2016
@techjoshua
Copy link
Author

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.

@rhuss
Copy link
Collaborator

rhuss commented Jun 28, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants