Rename variable used for defining used docker.sock file #177

tmaczukin · 2019-08-02T10:15:52Z

#174 added an experimental "rootless" variant of the DinD service.

While the change itself is simple and should not affect current usages, a DOCKER_HOST variable was used to define which docker.sock path should be used depending on the process owner's ID.

Chosing DOCKER_HOST variable for this is unfortunately not the best option:

It's not the best semantic choice - the code that uses it defines a socket file, not the host. DOCKER_SOCKET just seems to fit better the purpose.
DOCKER_HOST is already a variable, that is used by users to define where the Docker daemon is listening. In case when it's added to the DinD container (which is a common situation for example for GitLab CI jobs that are using DinD as a service), it finally ends with assigning two times the same port, while once it uses 0.0.0.0 address, and once some unresolvable domain name (depending on what user defined; most probably docker).

This commit proposes a change of the variable name to DOCKER_SOCKET, which will better match it purpose and additionally it will stop breaking configurations of many of docker:dind image users.

Fixes #175

docker-library#174 added an experimental "rootless" variant of the DinD service. While the change itself is simple and should not affect current usages, a `DOCKER_HOST` variable was used to define which docker.sock path should be used depending on the process owner's ID. Chosing DOCKER_HOST variable for this is unfortunately not the best option: 1. It's not the best semantic choice - the code that uses it defines a socket file, not the host. DOCKER_SOCKET just seems to fit better the purpose. 1. DOCKER_HOST is already a variable, that is used by users to define where the Docker daemon is listening. In case when it's added to the DinD container (which is a common situation for example for GitLab CI jobs that are using DinD as a service), it finally ends with assigning two times the same port, while once it uses 0.0.0.0 address, and once some unresolvable domain name (depending on what user defined; most probably `docker`). This commit proposes a change of the variable name to DOCKER_SOCKET, which will better match it purpose and additionally it will stop breaking configurations of many of docker:dind image users. Signed-off-by: Tomasz Maczukin <tomasz@maczukin.pl>

tmaczukin · 2019-08-02T10:17:52Z

@tianon Could I get your attention on this PR?

vasilerobert85 · 2019-08-02T10:52:25Z

Hi Tomasz,

I have commented out DOCKER_HOST: tcp://docker:2375 and set docker:18.09.7-dind and it seems to work.

Did not risk going to 19.03.0-dind

variables:
  ECR_SCREENING: $ECR_SCREENING
  DOCKER_IMAGE_TAG: $CI_REGISTRY_IMAGE:$CI_COMMIT_SHA
  #DOCKER_HOST: tcp://docker:2375
  DOCKER_DRIVER: overlay2
  DOCKER_TLS_CERTDIR: ""

services:
  - docker:18.09.7-dind

stages:
  - build
  - test
  - push
  - deploy

build:
  image: docker:18.09.7-dind
  stage: build
  only:
    - merge_requests
    - master
  script:
    - docker login -u gitlab-ci-token -p $CI_JOB_TOKEN $CI_REGISTRY
    - docker build -t $DOCKER_IMAGE_TAG .
    - docker push $DOCKER_IMAGE_TAG
...

vasilerobert85 · 2019-08-02T11:08:21Z

tested with 19.03.0-dind as well and --> is working

tmaczukin · 2019-08-02T12:00:16Z

Also cc @AkihiroSuda and @yosifkit who were involved in reviewing and merging #174 :)

tianon · 2019-08-02T14:02:34Z

Ouch, I ran into a related problem last night just before going to bed but hadn't had a chance to dig into it yet. 😞

I agree with changing this variable name, but I think in doing so we should ditch the :- and the export (making this a local script variable instead of an environment variable, since DOCKER_SOCKET doesn't have any meaning outside of here and isn't something we want to create), and we should also check whether DOCKER_HOST is set to something that starts with unix:// and use that value if so (which was the original intention here, since Docker picks up that value, but I obviously forgot the other half 🤦‍♂️).

There's also another path where this might get set strangely: docker run ... docker:dind dockerd-entrypoint.sh ... (which has been supported previously -- the updated code turns this into dockerd-entrypoint.sh docker-entrypoint.sh dockerd-entrypoint.sh which sets DOCKER_HOST and causes the same havoc which can be healed in the same way).

Here's my proposed adjustment to this:

diff --git a/dockerd-entrypoint.sh b/dockerd-entrypoint.sh
index 47eee55..cdbea49 100755
--- a/dockerd-entrypoint.sh
+++ b/dockerd-entrypoint.sh
@@ -92,16 +92,16 @@ _tls_generate_certs() {
 # no arguments passed
 # or first arg is `-f` or `--some-option`
 if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
-	# set DOCKER_HOST to the default "--host" value (for both standard or rootless)
+	# set "dockerSocket" to the default "--host" *unix socket* value (for both standard or rootless)
 	uid="$(id -u)"
 	if [ "$uid" = '0' ]; then
-		: "${DOCKER_HOST:=unix:///var/run/docker.sock}"
+		dockerSocket='unix:///var/run/docker.sock'
 	else
 		# if we're not root, we must be trying to run rootless
 		: "${XDG_RUNTIME_DIR:=/run/user/$uid}"
-		: "${DOCKER_HOST:=unix://$XDG_RUNTIME_DIR/docker.sock}"
+		dockerSocket="unix://$XDG_RUNTIME_DIR/docker.sock"
 	fi
-	export DOCKER_HOST
+	case "${DOCKER_HOST:-}" in unix://*) dockerSocket="$DOCKER_HOST" ;; esac
 
 	# add our default arguments
 	if [ -n "${DOCKER_TLS_CERTDIR:-}" ] \
@@ -112,7 +112,7 @@ if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
 	; then
 		# generate certs and use TLS if requested/possible (default in 19.03+)
 		set -- dockerd \
-			--host="$DOCKER_HOST" \
+			--host="$dockerSocket" \
 			--host=tcp://0.0.0.0:2376 \
 			--tlsverify \
 			--tlscacert "$DOCKER_TLS_CERTDIR/server/ca.pem" \
@@ -123,7 +123,7 @@ if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
 	else
 		# TLS disabled (-e DOCKER_TLS_CERTDIR='') or missing certs
 		set -- dockerd \
-			--host="$DOCKER_HOST" \
+			--host="$dockerSocket" \
 			--host=tcp://0.0.0.0:2375 \
 			"$@"
 		DOCKERD_ROOTLESS_ROOTLESSKIT_FLAGS="${DOCKERD_ROOTLESS_ROOTLESSKIT_FLAGS:-} -p 0.0.0.0:2375:2375/tcp"
@@ -175,8 +175,9 @@ if [ "$1" = 'dockerd' ]; then
 			${DOCKERD_ROOTLESS_ROOTLESSKIT_FLAGS:-} \
 			"$@" --userland-proxy-path=rootlesskit-docker-proxy
 	fi
-else
+elif base="$(basename "$1")" && [ "$base" != 'dockerd-entrypoint.sh' ] && [ "$base" != 'docker-entrypoint.sh' ]; then
 	# if it isn't `dockerd` we're trying to run, pass it through `docker-entrypoint.sh` so it gets `DOCKER_HOST` set appropriately too
+	# ("docker run ... docker:dind dockerd-entrypoint.sh ..." is legal, and "DOCKER_HOST" affects dockerd so we don't want to set it there)
 	set -- docker-entrypoint.sh "$@"
 fi

tianon · 2019-08-02T14:13:08Z

I guess really with the DOCKER_HOST fix, we can skip that second half -- it's purely an optimization at that point (and thus unrelated):

diff --git a/dockerd-entrypoint.sh b/dockerd-entrypoint.sh
index 47eee55..cdbea49 100755
--- a/dockerd-entrypoint.sh
+++ b/dockerd-entrypoint.sh
@@ -92,16 +92,16 @@ _tls_generate_certs() {
 # no arguments passed
 # or first arg is `-f` or `--some-option`
 if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
-	# set DOCKER_HOST to the default "--host" value (for both standard or rootless)
+	# set "dockerSocket" to the default "--host" *unix socket* value (for both standard or rootless)
 	uid="$(id -u)"
 	if [ "$uid" = '0' ]; then
-		: "${DOCKER_HOST:=unix:///var/run/docker.sock}"
+		dockerSocket='unix:///var/run/docker.sock'
 	else
 		# if we're not root, we must be trying to run rootless
 		: "${XDG_RUNTIME_DIR:=/run/user/$uid}"
-		: "${DOCKER_HOST:=unix://$XDG_RUNTIME_DIR/docker.sock}"
+		dockerSocket="unix://$XDG_RUNTIME_DIR/docker.sock"
 	fi
-	export DOCKER_HOST
+	case "${DOCKER_HOST:-}" in unix://*) dockerSocket="$DOCKER_HOST" ;; esac
 
 	# add our default arguments
 	if [ -n "${DOCKER_TLS_CERTDIR:-}" ] \
@@ -112,7 +112,7 @@ if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
 	; then
 		# generate certs and use TLS if requested/possible (default in 19.03+)
 		set -- dockerd \
-			--host="$DOCKER_HOST" \
+			--host="$dockerSocket" \
 			--host=tcp://0.0.0.0:2376 \
 			--tlsverify \
 			--tlscacert "$DOCKER_TLS_CERTDIR/server/ca.pem" \
@@ -123,7 +123,7 @@ if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
 	else
 		# TLS disabled (-e DOCKER_TLS_CERTDIR='') or missing certs
 		set -- dockerd \
-			--host="$DOCKER_HOST" \
+			--host="$dockerSocket" \
 			--host=tcp://0.0.0.0:2375 \
 			"$@"
 		DOCKERD_ROOTLESS_ROOTLESSKIT_FLAGS="${DOCKERD_ROOTLESS_ROOTLESSKIT_FLAGS:-} -p 0.0.0.0:2375:2375/tcp"

tmaczukin · 2019-08-02T14:27:23Z

@tianon Thanks for the update. I agree that using the local variable may be a better way. And thanks for pointing the check whether DOCKER_HOST is set to something that starts with unix:// case, since I've totally haven thought about it.

I also think there is no need to change how the last case is handled - in context of this fix. An optimization is a separate issue (and probably it could also get some attention, but it's totally not related to #175 problem).

I've pushed an update, using your second example. What do you think now?

tianon

Just a couple minor edits 🙏 ❤️ ❗

tianon · 2019-08-02T14:29:46Z

dockerd-entrypoint.sh

 	fi
-	export DOCKER_HOST
+        case "${DOCKER_HOST:-}"
+                in unix://*)


I agree with exploding this for readability, but we typically put in on the line with the case (since a given case will only ever have one in keyword).

You're totally right! My bad, I've used [enter] button in wrong place ;)

tianon · 2019-08-02T14:30:25Z

dockerd-entrypoint.sh

 # or first arg is `-f` or `--some-option`
 if [ "$#" -eq 0 ] || [ "${1#-}" != "$1" ]; then
-	# set DOCKER_HOST to the default "--host" value (for both standard or rootless)
+        # set "dockerSocket" to the default "--host" *unix socket* value (for both standard or rootless)


Can you please adjust the space indents to tabs throughout this change (matching the rest of the file)? 🙏 ❤️

tianon

❤️ looks great, thank you!

I want to merge this ASAP and get it out but I'm going to give Travis a small chance to try and run before doing so.

(While I brush my teeth and actually get ready for the day; rolled right out of bed into this today 😆)

tianon · 2019-08-02T14:53:08Z

... and Travis is already done 🎉

Changes: - docker-library/docker@dc04548: Merge pull request docker-library/docker#177 from tmaczukin/rename-docker-socket-variable - docker-library/docker@be5681f: Rename DOCKER_SOCKET to dockerSocket and unexport it - docker-library/docker@8d3471f: Rename variable used for defining used docker.sock file

tmaczukin · 2019-08-02T14:55:31Z

Thank you for the help @tianon. Looking forward to have this fix released into Docker Hub! :)

tianon · 2019-08-02T14:56:24Z

Thank you! Feel free to follow along from here over at docker-library/official-images#6403

norrs · 2019-08-02T16:11:05Z

On behalf of the community and probably tons of gitlab dind users, thanks for the quick delivery and hard work @tmaczukin and @tianon !

EDIT: typos

- docker-library/docker#177 (DOCKER_HOST fix) - docker-library/docker#180 (dind script adjustment)

tmaczukin mentioned this pull request Aug 2, 2019

docker:dind tries to listen to two overlapping ports #175

Closed

tianon requested changes Aug 2, 2019

View reviewed changes

Rename DOCKER_SOCKET to dockerSocket and unexport it

be5681f

tmaczukin force-pushed the rename-docker-socket-variable branch from 7f6dddc to be5681f Compare August 2, 2019 14:48

tianon approved these changes Aug 2, 2019

View reviewed changes

tianon merged commit dc04548 into docker-library:master Aug 2, 2019

tmaczukin deleted the rename-docker-socket-variable branch August 2, 2019 14:54

tianon mentioned this pull request Aug 2, 2019

Fix nasty dind regression docker-library/official-images#6403

Merged

tianon added a commit to tianon/dockerfiles that referenced this pull request Aug 5, 2019

Update "dockerd-entrypoint.sh"

4d4fbda

- docker-library/docker#177 (DOCKER_HOST fix) - docker-library/docker#180 (dind script adjustment)

Rename variable used for defining used docker.sock file #177

Rename variable used for defining used docker.sock file #177

Uh oh!

Conversation

tmaczukin commented Aug 2, 2019 • edited by tianon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmaczukin commented Aug 2, 2019

Uh oh!

vasilerobert85 commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasilerobert85 commented Aug 2, 2019

Uh oh!

tmaczukin commented Aug 2, 2019

Uh oh!

tianon commented Aug 2, 2019

Uh oh!

tianon commented Aug 2, 2019

Uh oh!

tmaczukin commented Aug 2, 2019

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

tianon Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

tmaczukin Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

tmaczukin Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

tianon Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

tmaczukin Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

tmaczukin Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

tianon commented Aug 2, 2019

Uh oh!

tmaczukin commented Aug 2, 2019

Uh oh!

tianon commented Aug 2, 2019

Uh oh!

norrs commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tmaczukin commented Aug 2, 2019 •

edited by tianon

Loading

vasilerobert85 commented Aug 2, 2019 •

edited

Loading

norrs commented Aug 2, 2019 •

edited

Loading