Repository navigation
fix(docker-outside-of-docker): check DOCKER_GID on container start - #1749
Vladimir (varmiral) wants to merge 8 commits into
Conversation
…r the container restarts if the Docker GID has been changed
The DOCKER_GID variable must be defined every time the container starts; otherwise, after the Docker GID is changed via `sudoIf groupmod --gid "\${SOCKET_GID}" docker`, the socat fallback will trigger on subsequent restarts.
Kaniska (v-Kaniska244)
left a comment
There was a problem hiding this comment.
Thank you for the contribution. Would you please look into the failed tests. Also a version bump would be required for the feature.
|
@microsoft-github-policy-service agree |
|
Kaniska (@v-Kaniska244) please forgive my inattention. |
Kaniska (v-Kaniska244)
left a comment
There was a problem hiding this comment.
LGTM. One last request if possible, could you please add some details in the PR description and modify the subject as it's currently truncated. That way this change will be better documented.
There was a problem hiding this comment.
🟡 Changes recommended
The startup no-op can leave a stale socat socket without a live listener when GIDs match on a later container start, causing Docker commands to fail with Connection refused.
1 open issue
What changed in this PR
This PR updates the Docker-outside-of-Docker feature to evaluate socket permissions at container startup rather than relying on the build-time Docker group GID.
Changes:
- Reads the Docker group GID at startup and skips proxy setup when GIDs match.
- Uses an exact
getentlookup to check for group GID collisions. - Bumps the feature version to
1.10.2.
| File | Description |
|---|---|
src/docker-outside-of-docker/install.sh |
Updates runtime GID checks and socket proxy branching. |
src/docker-outside-of-docker/devcontainer-feature.json |
Bumps the feature patch version to 1.10.2. |
Review effort: Balanced
Kaniska (v-Kaniska244)
left a comment
There was a problem hiding this comment.
Could you pls check the comment from copilot reviewer. It appears legit, kindly have a look and get back in case of any issue/ clarification.
Add check for symbolic link on target socket in Docker installation script. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The existing tests also pass with the old proxy behavior and lack deterministic coverage for GID changes, matching-GID symlink preservation without socat, and exact versus prefix-only GID collisions.
1 open issue
1 resolved since last review
Review effort: Balanced
| DOCKER_GID="\$(getent group docker | cut -d: -f3)" | ||
| SOCKET_GID=\$(stat -c '%g' ${SOURCE_SOCKET}) | ||
| if [ "\${SOCKET_GID}" != "0" ] && [ "\${SOCKET_GID}" != "${DOCKER_GID}" ] && ! grep -E ".+:x:\${SOCKET_GID}" /etc/group; then | ||
| if [ "\${SOCKET_GID}" = "\${DOCKER_GID}" ] && [ -L "${TARGET_SOCKET}" ]; then | ||
| log "Docker group matches socket GID \${SOCKET_GID}, nothing to do" | ||
| elif [ "\${SOCKET_GID}" != "0" ] && ! getent group "\${SOCKET_GID}" > /dev/null; then |
There was a problem hiding this comment.
LGTM. Regarding the latest review comment, perhaps adding a custom test like below could be useful. Please let me know if you think otherwise or concern with the test. No other changes expected apart from this.
In scenarios.json:
"docker_init_gid": {
"image": "mcr.microsoft.com/devcontainers/base:ubuntu-24.04",
"features": {
"docker-outside-of-docker": {
"moby": true
}
},
"containerUser": "vscode"
},
The actual test script docker_init_gid.sh :
#!/bin/bash
set -e
source dev-container-features-test-lib
test_gid_case() (
set -e
local scenario="$1"
export GID_TEST_DIR
GID_TEST_DIR="$(mktemp -d)"
trap 'rm -rf "${GID_TEST_DIR}"' EXIT
echo 2000 > "${GID_TEST_DIR}/docker-gid"
echo 100 > "${GID_TEST_DIR}/socket-gid"
touch "${GID_TEST_DIR}/source.sock" "${GID_TEST_DIR}/groups"
ln -s "${GID_TEST_DIR}/source.sock" "${GID_TEST_DIR}/target.sock"
sed -e "s|/var/run/docker-host.sock|${GID_TEST_DIR}/source.sock|g" \
-e "s|/var/run/docker.sock|${GID_TEST_DIR}/target.sock|g" \
-e "s|/tmp/vscr-docker-from-docker|${GID_TEST_DIR}/proxy|g" \
/usr/local/share/docker-init.sh > "${GID_TEST_DIR}/docker-init.sh"
getent() {
if [ "$1" != group ]; then
return 1
fi
if [ "$2" = docker ] || [ "$2" = "$(cat "${GID_TEST_DIR}/docker-gid")" ]; then
echo "docker:x:$(cat "${GID_TEST_DIR}/docker-gid"):vscode"
else
awk -F: -v gid="$2" '$3 == gid { print; found = 1 } END { exit !found }' "${GID_TEST_DIR}/groups"
fi
}
stat() {
[ "$1" = -c ] && [ "$2" = '%g' ] && [ "$3" = "${GID_TEST_DIR}/source.sock" ]
cat "${GID_TEST_DIR}/socket-gid"
}
groupmod() {
[ "$1" = --gid ] && [ "$3" = docker ]
echo "$2" > "${GID_TEST_DIR}/docker-gid"
echo "$*" > "${GID_TEST_DIR}/groupmod"
}
id() {
[ "$1" = -u ]
echo 0
}
socat() {
return 0
}
export -f getent stat groupmod id socat
case "${scenario}" in
matching|stale-target)
echo 100 > "${GID_TEST_DIR}/docker-gid"
if [ "${scenario}" = stale-target ]; then
rm "${GID_TEST_DIR}/target.sock"
touch "${GID_TEST_DIR}/target.sock"
fi
;;
prefix)
echo 'other:x:1000:' > "${GID_TEST_DIR}/groups"
;;
collision)
echo 'other:x:100:' > "${GID_TEST_DIR}/groups"
;;
*) return 1 ;;
esac
bash "${GID_TEST_DIR}/docker-init.sh" true || return 1
case "${scenario}" in
matching)
[ -L "${GID_TEST_DIR}/target.sock" ] &&
[ "$(readlink "${GID_TEST_DIR}/target.sock")" = "${GID_TEST_DIR}/source.sock" ] &&
[ ! -e "${GID_TEST_DIR}/proxy.pid" ] &&
[ ! -e "${GID_TEST_DIR}/groupmod" ] &&
grep -q 'Docker group matches socket GID 100, nothing to do' "${GID_TEST_DIR}/proxy.log"
;;
prefix)
[ "$(cat "${GID_TEST_DIR}/docker-gid")" = 100 ] &&
grep -qx -- '--gid 100 docker' "${GID_TEST_DIR}/groupmod" &&
[ -L "${GID_TEST_DIR}/target.sock" ] &&
[ ! -e "${GID_TEST_DIR}/proxy.pid" ]
;;
collision|stale-target)
[ ! -L "${GID_TEST_DIR}/target.sock" ] &&
[ -s "${GID_TEST_DIR}/proxy.pid" ] &&
[ ! -e "${GID_TEST_DIR}/groupmod" ] &&
grep -q 'Enabling socket proxy.' "${GID_TEST_DIR}/proxy.log"
;;
esac
)
check "runtime-matching-gid-preserves-symlink-without-proxy" test_gid_case matching
check "prefix-only-gid-match-updates-docker-group-without-proxy" test_gid_case prefix
check "exact-gid-collision-enables-proxy" test_gid_case collision
check "matching-gid-with-stale-target-enables-proxy" test_gid_case stale-target
reportResults


Problem
DOCKER_GIDwas computed once ininstall.shat feature install time and baked into the generated/usr/local/share/docker-init.shthrough heredoc expansion. This causes several problems when the container starts:socatproxy. When the socket GID already matched the docker group GID, the conditionSOCKET_GID != DOCKER_GIDwas false and execution fell through to theelsebranch. That branch replaceddocker.sockwith asocatproxy even though nothing needed fixing.grep -E ".+:x:${SOCKET_GID}" /etc/groupis not anchored to the GID field:100also matches a group with GID1000Changes
DOCKER_GID="$(grep -oP ...)"frominstall.sh.docker-init.shnow reads the docker GID at container start:DOCKER_GID="$(getent group docker | cut -d: -f3)"SOCKET_GID == DOCKER_GID→ log it and do nothing (no proxy, no group changes)SOCKET_GID != 0and no group with that GID exists →groupmod --gid "${SOCKET_GID}" dockersocatproxy, as beforegrepon/etc/groupwithgetent group "${SOCKET_GID}". It does an exact lookup by GID and also works with NSS sources other than/etc/group.