Skip to content

docker.Makefile: Use active context socket#7112

Merged
thaJeztah merged 1 commit into
docker:masterfrom
vvoland:makesocket
Jul 17, 2026
Merged

docker.Makefile: Use active context socket#7112
thaJeztah merged 1 commit into
docker:masterfrom
vvoland:makesocket

Conversation

@vvoland

@vvoland vvoland commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

The development and e2e containers hard-code /var/run/docker.sock, so rootless and other local contexts cannot expose their daemon socket. Resolve the active context's Docker endpoint and strip the unix scheme before using it as the bind source.

- What I did

- How I did it

- How to verify it

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

@vvoland vvoland added this to the 29.7.0 milestone Jul 17, 2026
@vvoland vvoland self-assigned this Jul 17, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟡 NEEDS ATTENTION

Two CONFIRMED medium-severity findings on the new DOCKER_SOCK variable assignment introduced by this PR:

  1. Non-unix endpoint passes raw URL as bind-mount source — when the active Docker context uses a TCP, SSH, or named-pipe endpoint, patsubst unix://%,% finds no match and the raw scheme URL is used as src= for --mount type=bind, which Docker will reject.
  2. Context inspect failure yields empty DOCKER_SOCK — if docker context inspect fails (missing binary, unknown context, etc.), Make swallows stderr and DOCKER_SOCK becomes empty, producing an opaque "field src must not be empty" error with no indication of the root cause.

Both findings include a suggested fix: wrapping the expression with `` so a safe default is always present.

Comment thread docker.Makefile Outdated
Comment thread docker.Makefile Outdated

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟡 NEEDS ATTENTION

The PR correctly replaces hardcoded /var/run/docker.sock with a dynamically resolved socket path from the active Docker context. The core logic using patsubst + filter + or fallback is sound for the common unix-socket case. Two edge cases are introduced: non-unix context endpoints (tcp://, ssh://, npipe://) silently fall back to the hardcoded default, and docker context inspect failures are silently swallowed.

Comment thread docker.Makefile
Comment thread docker.Makefile
Comment thread docker.Makefile Outdated
The development and e2e containers hard-code /var/run/docker.sock, so
rootless and other local contexts cannot expose their daemon socket.
Resolve the active context's Docker endpoint and strip the unix scheme
before using it as the bind source.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The PR correctly resolves the active Docker context's socket path, strips the unix:// scheme, and uses it as the bind-mount source — with a safe fallback to /var/run/docker.sock for non-unix contexts and failure cases. The $(shell ...) / filter / patsubst / or chain is correct Make idiom, Make trims trailing newlines from shell output, and the single-quoting of the --mount argument is valid because Make expands variables before the shell sees the command. All three bind-mount sites are updated consistently.

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

we should have a flag for that 😜

@thaJeztah
thaJeztah merged commit 617d772 into docker:master Jul 17, 2026
97 checks passed
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.

4 participants