Docker version detection fails on Ubuntu 16.04

[TimMoore]

Expected behaviour

sbt docker:publish (or sbt docker:publishLocal) should succeed on Ubuntu 16.04 systems.

Actual behaviour

[info] Step 7/14 : FROM adoptopenjdk/openjdk8
[info]  ---> 43b85f10fa8f
[info] Step 8/14 : USER root
[info]  ---> Using cache
[info]  ---> 0a8165c3554f
[info] Step 9/14 : RUN id -u demiourgos728 2> /dev/null || useradd --system --create-home --uid 1001 --gid 0 demiourgos728
[info]  ---> Using cache
[info]  ---> 0df060343a74
[info] Step 10/14 : WORKDIR /opt/docker
[info]  ---> Using cache
[info]  ---> 985238b236c6
[info] Step 11/14 : COPY --from=stage0 --chown=demiourgos728:root /opt/docker /opt/docker
[error] Unknown flag: chown
[error] java.lang.RuntimeException: Nonzero exit value: 1
[error]     at com.typesafe.sbt.packager.docker.DockerPlugin$.publishLocalDocker(DockerPlugin.scala:483)
[error]     at com.typesafe.sbt.packager.docker.DockerPlugin$.$anonfun$projectSettings$33(DockerPlugin.scala:187)
[error]     at com.typesafe.sbt.packager.docker.DockerPlugin$.$anonfun$projectSettings$33$adapted(DockerPlugin.scala:185)
[error]     at scala.Function1.$anonfun$compose$1(Function1.scala:44)
[error]     at sbt.internal.util.$tilde$greater.$anonfun$$u2219$1(TypeFunctions.scala:40)
[error]     at sbt.std.Transform$$anon$4.work(System.scala:67)
[error]     at sbt.Execute.$anonfun$submit$2(Execute.scala:269)
[error]     at sbt.internal.util.ErrorHandling$.wideConvert(ErrorHandling.scala:16)
[error]     at sbt.Execute.work(Execute.scala:278)
[error]     at sbt.Execute.$anonfun$submit$1(Execute.scala:269)
[error]     at sbt.ConcurrentRestrictions$$anon$4.$anonfun$submitValid$1(ConcurrentRestrictions.scala:178)
[error]     at sbt.CompletionService$$anon$2.call(CompletionService.scala:37)
[error]     at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[error]     at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
[error]     at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[error]     at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[error]     at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[error]     at java.lang.Thread.run(Thread.java:748)

I think the problem is that the reported Docker version (17.05.0~ce-0~ubuntu-xenial) doesn't match the regex used to determine Docker version and fall back to running chown explicitly in the build. The problem seems to be the "~" character isn't matched by the pattern:

sbt-native-packager/src/main/scala/com/typesafe/sbt/packager/docker/DockerVersion.scala

Line 8 in 435c155

private val DockerVersionPattern: Regex = "^'?([0-9]+).([0-9]+).([0-9]+)-?([-a-z0-9]+)?'?$".r

scala> DockerVersionPattern.pattern.matcher("17.05.0~ce-0ubuntu-xenial").matches
res4: Boolean = false
scala> DockerVersionPattern.pattern.matcher("17.05.0-ce-0ubuntu-xenial").matches
res6: Boolean = true

(It's hard to see, but in the second example, the "~" was changed to a "-").

I'd suggest two fixes:

1. Extend the regex to allow any non-space ASCII characters in the version string to match (or allow prefix matches rather than requiring the whole string to match)
2. If the Docker version can't be parsed, fall back to assuming the oldest Docker version and don't enable any settings that require newer Docker versions (maybe with a warning logged) — in this case, fall back to the "Run" strategy

Information

• sbt-native-packager 1.3.20 (via Lagom 1.5.1/Play 2.7.2)
• Ubuntu 16.04
• Docker 17.05.0~ce-0ubuntu-xenial

(Note: I'm filing this on behalf of @kotdv, who reported this on the lagom/lagom Gitter channel. I don't have an Ubuntu 16.04 system handy to reproduce this on.)

[TimMoore]

CC: @eed3si9n TBH I got a bit lost in the code trying to work out how the version determines the strategy, so I might have misdiagnosed, but this is my best guess at what's happening.

[eed3si9n]

I don't think I was involved in Docker version detection code, but apparently Debian Policy Manual says you can use alphanumerics and the characters . + - ~ (full stop, plus, hyphen, tilde).

[TimMoore]

@eed3si9n I think this relates to this change #1190

I might be mistakenly assuming that the default permission strategy is chosen based on the Docker version. Maybe it's just validating that they're compatible, but this doesn't handle the case where the version can't be detected correctly.

[eed3si9n]

It's been a while, but looking at the code, the build user is always in control of the permission strategy. DockerPlugin just sets the following default:

https://github.com/sbt/sbt-native-packager/blob/v1.3.21/src/main/scala/com/typesafe/sbt/packager/docker/DockerPlugin.scala#L72-L73

    dockerPermissionStrategy := DockerPermissionStrategy.MultiStage,
    dockerChmodType := DockerChmodType.UserGroupReadExecute

Where Docker version gets involved is the validation of the specified strategy in https://github.com/sbt/sbt-native-packager/blob/v1.3.21/src/main/scala/com/typesafe/sbt/packager/docker/DockerPlugin.scala#L607-L646
If None is detected, it doesn't warn the user that they have an incompatible strategy (either by default or set manually).

Either case, I'd be happy to send a PR to improve the regex.

[eed3si9n]

I have a PR here - #1231

[eed3si9n]

If the Docker version can't be parsed, fall back to assuming the oldest Docker version and don't enable any settings that require newer Docker versions (maybe with a warning logged) — in this case, fall back to the "Run" strategy

Changing the build semantics (even if it's changing Dockerfile content) based on the detected Docker version is actually dangerous because various things can affect the "docker version", including eval $(minikube docker-env) (#1187). In my opinion, it's generally a bad practice that external things gets pulled into the build behavior since you get non-repeatable build depending on who runs it on some machine. Sometimes it's unavoidable like Java version, but we should try to minimize dynamic behavior.

[TimMoore]

All good points. Maybe one day sbt-native-packager can be reworked to use jib to build images rather than requiring an external Docker. Thanks for the PR!

[TimMoore]

If None is detected, it doesn't warn the user that they have an incompatible strategy (either by default or set manually).

I'm thinking that if None is detected it should print a different warning that the version couldn't be detected, and maybe a link to documentation around configuring the strategy … which doesn't currently exist. At least then people won't think the problem is a bug in sbt-native-packager (or one of its downstream dependencies).

I'll try to throw a PR together if I ever have some free time, but that might not happen soon.

[kotdv]

Very simple reasoning.
Current code works only with docker >=17.09.
Run version works with any docker version.

That's why default version should be Run, because it works everywhere.
Then if there's an extra option provided for how to deal with 'chown' or explicit strategy to use - it should switch to '--chown' version.

It is bad to make backward incompatible changes from feature requests, always think about majority, if one wants to save 130MB from 2GB footprint -> sure that's a great thing, but make it so it requires an extra config option if you can't provide a reliable switch mechanism (read Regex version parser, which is unreliable in my opinion).

[muuki88]

All good points. Maybe one day sbt-native-packager can be reworked to use jib to build images rather than requiring an external Docker. Thanks for the PR!

Currently the we have the Spotify java client integration. https://sbt-native-packager.readthedocs.io/en/latest/formats/docker.html?highlight=java Still the native means we focus on the native client. Maintainer resources are too scares for more.

[TimMoore]

Thanks @muuki88. The Spotify Java client is a little different, in that it still requires a running Docker Engine to communicate with, whereas jib builds OCI-compatible images directly without requiring Docker at all, so it's more self contained. Fair enough on the maintainer resources consideration, although I expect Lightbend teams to continue to contribute to sbt-native-packager in the future as needed.

I think it still makes sense to make a couple of additional minor improvements to this:

1. When it validates the permission strategy against the Docker version, if the version can't be detected, it should log a warning, as I suggested in Docker version detection fails on Ubuntu 16.04 #1230 (comment) (raised as Docker permission strategy warning when Docker version can't be detected #1233)
2. Settings should default to the most compatible option, and require opting in to features that require newer Docker versions, as @kotdv suggested in Docker version detection fails on Ubuntu 16.04 #1230 (comment) (raised as Default Docker permission strategy should be Run #1234)

I raised separate issues for these. @muuki88 if you agree with the suggestions, maybe the community can help out.

[muuki88]

@TimMoore thanks for taking care and creating the issues. I'll comment on the separate issues.