Hi Arnaud, Thanks for the summary.
On Wed, Mar 6, 2019 at 9:51 AM Arnaud Rebillout <arnaud.rebill...@collabora.com> wrote: > > Dear Go team, > > So let met sum up this bug now to avoid you reading the backlog: > > In short, docker doesn't recognize the output of `runc --version`, and > then it misbehaves and flood the log forever. To be more accurate, > docker is not happy because it wants to know the git commit that was > used to build runc. This information may or may not be part of the > output of `runc --version`: it depends on how runc was built. Actually, > there are two fields, `version` and `gitcommit`, that can be passed > optionally to runc during the build (as ldflags). Therefore the output > of `runc --version` varies accordingly. > > So there's two ways out of that: > > **Solution 1** > > One is to patch docker, and fix their code so that it can handle various > runc version outputs. This is the way chosen by buildroot for example: > > https://github.com/buildroot/buildroot/blob/master/package/docker-engine/0001-Fix-faulty-runc-version-commit-scrape.patch > > Note that this patch is not the definitive solution: it just allows > docker to handle the output of runc *as built in buildroot*, and not in > every case. If we go this way, we'll do something similar, supporting > "debian's runc" output, and end up with a patch that has no chance to > make it upstream. > > Maybe the core of the issue is that the `runc --version` output is > difficult to parse properly, and that's why docker upstream don't want > to bother fixing their code... > > > **Solution 2** > > The other way is to modify the runc build so that we include the git > commit. I've pushed such a change on a branch at: > > https://salsa.debian.org/go-team/packages/runc/commit/033a60b549b0935861c92bf4e5118d03ef443a8f > > It's actually not that bad, no patch involved, it's really just adding > ldflags. The only downside I see is that there's an additional step when > a maintainer wants to release a new upstream version: he has to look for > the corresponding commit and add it to the file > `debian/upstream-version-gitcommits`. I provided a helper script to do > that, so it's really a small overhead. I think the runc should be fixed. But I don't like the patch you suggested. It's confused to user. If you set the git commit to the upstream one, like ccb5efd37fb7c86364786e9137e22948751de7ed for 1.0.0-rc6, the user would think it's 1.0.0-rc6 indeed, but apparently it's not, it's 1.0.0-rc6 with CVE-2019-5736 patch. So I would suggest to use the debian package version in the commit field. More specifically: diff --git a/debian/rules b/debian/rules index 81df53b..0087b6b 100755 --- a/debian/rules +++ b/debian/rules @@ -5,7 +5,11 @@ export DH_GOPKG := github.com/opencontainers/runc export DH_GOLANG_INSTALL_EXTRA := libcontainer/seccomp/fixtures + +include /usr/share/dpkg/pkg-info.mk + TAGS=apparmor seccomp selinux ambient +LDFLAGS := -X main.version=$(DEB_VERSION_UPSTREAM) -X main.gitCommit=$(DEB_VERSION) %: dh $@ --buildsystem=golang --with=golang --builddirectory=_build @@ -33,7 +37,7 @@ override_dh_auto_configure: # ln -svrf vendor/github.com/opencontainers/specs _build/src/github.com/opencontainers/ override_dh_auto_build: - dh_auto_build -- -tags "$(TAGS)" + dh_auto_build -- -tags "$(TAGS)" -ldflags "$(LDFLAGS)" override_dh_auto_test: DH_GOLANG_EXCLUDES="libcontainer/integration" \ > > > **What do you think?** > > I'm in favor of solution 2, it seems the path of least trouble, and is > also a correct thing to do IMHO. I'd like to have this fixed for buster, > as this issue has been opened for a long while. > > If you agree with solution 2 I'll need some DD to upload a new version > of runc with this change (sorry to bother you...). Also this bug should > be re-assigned to runc if we solve it in the runc package. > > For more details you can also look at upstream discussion: > https://github.com/moby/moby/issues/38709 > > And we're late to fix this before hard freeze. If we want this fix included in buster, we should ask release team to unblock. -- Shengjing Zhu