-
Notifications
You must be signed in to change notification settings - Fork 1k
Automatically get docker socket path (and fix socket path config in vite plugin) #10061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,10 @@ | ||||||
| --- | ||||||
| "@cloudflare/containers-shared": patch | ||||||
| "wrangler": patch | ||||||
| --- | ||||||
|
|
||||||
| feat: try to automatically get path of docker socket | ||||||
|
|
||||||
| Currently, if your container tool isn't set up to listen at `unix:///var/run/docker.sock` (or isn't symlinked to that), then you have to manually set this via the `dev.containerEngine` field in your Wrangler config, or via the env vars `WRANGLER_DOCKER_HOST`. This change means that we will try and get the socket of the current context automatically. This should reduce the occurrence of opaque `internal error`s thrown by the runtime when the daemon is not listening at `unix:///var/run/docker.sock`. | ||||||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| You can still override this with `WRANGLER_DOCKER_HOST`, and we also now read `DOCKER_HOST` too. | ||||||
|
||||||
| You can still override this with `WRANGLER_DOCKER_HOST`, and we also now read `DOCKER_HOST` too. | |
| In addition to `WRANGLER_DOCKER_HOST`, `DOCKER_HOST` can now also be used to set the container engine socket address. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@cloudflare/vite-plugin": patch | ||
| --- | ||
|
|
||
| fix: properly set the docker socket path in the vite plugin | ||
|
|
||
| Previously, this was only picking up the value set in Wrangler config under `dev.containerEngine`, but this value can also be set from env vars or automatically read from the current docker context. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { execFile, spawn } from "child_process"; | ||
| import { execFileSync, spawn } from "child_process"; | ||
| import { randomUUID } from "crypto"; | ||
| import { existsSync, statSync } from "fs"; | ||
| import path from "path"; | ||
|
|
@@ -61,22 +61,15 @@ export const runDockerCmd = ( | |
| }; | ||
| }; | ||
|
|
||
| export const runDockerCmdWithOutput = async ( | ||
| dockerPath: string, | ||
| args: string[] | ||
| ): Promise<string> => { | ||
| return new Promise((resolve, reject) => { | ||
| execFile(dockerPath, args, (error, stdout) => { | ||
| if (error) { | ||
| return reject( | ||
| new Error( | ||
| `Failed running docker command: ${error.message}. Command: ${dockerPath} ${args.join(" ")}` | ||
| ) | ||
| ); | ||
| } | ||
| return resolve(stdout.trim()); | ||
| }); | ||
| }); | ||
| export const runDockerCmdWithOutput = (dockerPath: string, args: string[]) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity what what this changed from async to sync? (not mentioned in the PR description). Does it deserves a comment explaining why it should not be async?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the |
||
| try { | ||
| const stdout = execFileSync(dockerPath, args, { encoding: "utf8" }); | ||
| return stdout.trim(); | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Failed running docker command: ${(error as Error).message}. Command: ${dockerPath} ${args.join(" ")}` | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| /** throws when docker is not installed */ | ||
|
|
@@ -209,7 +202,7 @@ export const getContainerIdsFromImage = async ( | |
| dockerPath: string, | ||
| ancestorImage: string | ||
| ) => { | ||
| const output = await runDockerCmdWithOutput(dockerPath, [ | ||
| const output = runDockerCmdWithOutput(dockerPath, [ | ||
| "ps", | ||
| "-a", | ||
| "--filter", | ||
|
|
@@ -250,3 +243,88 @@ export async function checkExposedPorts( | |
| export function generateContainerBuildId() { | ||
| return randomUUID().slice(0, 8); | ||
| } | ||
|
|
||
| type DockerContext = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this is parsed from Would it be worth adding some validations (zod or whatever) on top of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't really want to add zod just to validate this one thing, if it fails we have a sensible fallback. i mean, i would love to add zod to validate very many things across wrangler, but this probably isn't the place to start 😅 but comment added though :) |
||
| Current: boolean; | ||
| Description: string; | ||
| DockerEndpoint: string; | ||
| Error: string; | ||
| Name: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Run `docker context ls` to get the Docker socket from the currently active Docker context | ||
| */ | ||
| export function getDockerSocketFromContext(dockerPath: string) { | ||
|
||
| try { | ||
| const output = runDockerCmdWithOutput(dockerPath, [ | ||
| "context", | ||
| "ls", | ||
| "--format", | ||
| "json", | ||
| ]); | ||
|
|
||
| // Parse each line as a separate JSON object | ||
| const lines = output.trim().split("\n"); | ||
| const contexts: DockerContext[] = lines.map((line) => JSON.parse(line)); | ||
|
|
||
| // Find the current context | ||
| const currentContext = contexts.find((context) => context.Current === true); | ||
|
|
||
| if ( | ||
| currentContext && | ||
| currentContext.DockerEndpoint && | ||
| typeof currentContext.DockerEndpoint === "string" | ||
|
||
| ) { | ||
| return currentContext.DockerEndpoint; | ||
| } | ||
| } catch { | ||
| // Fall back to null if docker context inspection fails | ||
| } | ||
| return null; | ||
| } | ||
| /** | ||
| * Resolve Docker host as follows: | ||
| * 1. Check WRANGLER_DOCKER_HOST environment variable | ||
| * 2. Check DOCKER_HOST environment variable | ||
| * 3. Try to get socket from active Docker context | ||
| * 4. Fall back to platform-specific defaults | ||
| */ | ||
| export function resolveDockerHost(dockerPath: string) { | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (process.env.WRANGLER_DOCKER_HOST) { | ||
| return process.env.WRANGLER_DOCKER_HOST; | ||
| } | ||
|
|
||
| if (process.env.DOCKER_HOST) { | ||
| return process.env.DOCKER_HOST; | ||
| } | ||
|
|
||
| // 3. Try to get socket from by running `docker context ls` | ||
| try { | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const contextSocket = getDockerSocketFromContext(dockerPath); | ||
| if (contextSocket) { | ||
| return contextSocket; | ||
| } | ||
| } catch {} | ||
|
|
||
| // 4. Fall back to platform-specific defaults | ||
| // (note windows doesn't work yet due to a runtime limitation) | ||
| return process.platform === "win32" | ||
| ? "//./pipe/docker_engine" | ||
| : "unix:///var/run/docker.sock"; | ||
| } | ||
|
|
||
| /** | ||
| * | ||
| * Get docker host from environment variables or platform defaults. | ||
| * Does not use the docker context ls command, so we | ||
| */ | ||
| export const getDockerHostFromEnv = () => { | ||
| const fromEnv = process.env.WRANGLER_DOCKER_HOST ?? process.env.DOCKER_HOST; | ||
| if (fromEnv) { | ||
| return fromEnv; | ||
| } | ||
| return process.platform === "win32" | ||
| ? "//./pipe/docker_engine" | ||
| : "unix:///var/run/docker.sock"; | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import { resolveDockerHost } from "../src/utils"; | ||
|
|
||
| const mockedDockerContextLsOutput = `{"Current":true,"Description":"Current DOCKER_HOST based configuration","DockerEndpoint":"unix:///current/run/docker.sock","Error":"","Name":"default"} | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| {"Current":false,"Description":"Docker Desktop","DockerEndpoint":"unix:///other/run/docker.sock","Error":"","Name":"desktop-linux"}`; | ||
|
|
||
| vi.mock("node:child_process"); | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| describe.skipIf(process.platform !== "linux" && process.env.CI === "true")( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If think I have already seen this condition but I can't remember why it is. Would factoring the code into an helper function with some comment about the rationale help understanding/maintaining this code?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a helper would be great, but the other places this is used is fixtures and wrangler, and i don't know if it would be good to be importing helpers between these three contexts? i have added a comment here though |
||
| "resolveDockerHost", | ||
| () => { | ||
| let mockExecFileSync: ReturnType<typeof vi.fn>; | ||
|
|
||
| beforeEach(async () => { | ||
| vi.clearAllMocks(); | ||
| const childProcess = await import("node:child_process"); | ||
|
||
| mockExecFileSync = vi.mocked(childProcess.execFileSync); | ||
|
|
||
| mockExecFileSync.mockReturnValue(mockedDockerContextLsOutput); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllEnvs(); | ||
| }); | ||
|
|
||
| it("should return WRANGLER_DOCKER_HOST when set", async () => { | ||
| vi.stubEnv("WRANGLER_DOCKER_HOST", "unix:///foo/wrangler/socket"); | ||
| vi.stubEnv("DOCKER_HOST", "unix:///bar/docker/socket"); | ||
|
|
||
| const result = await resolveDockerHost("/no/op/docker"); | ||
| expect(result).toBe("unix:///foo/wrangler/socket"); | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| expect(mockExecFileSync).not.toHaveBeenCalled(); | ||
|
||
| }); | ||
|
|
||
| it("should return DOCKER_HOST when WRANGLER_DOCKER_HOST is not set", async () => { | ||
| expect(process.env.WRANGLER_DOCKER_HOST).toBeUndefined(); | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| vi.stubEnv("DOCKER_HOST", "unix:///bar/docker/socket"); | ||
|
|
||
| const result = await resolveDockerHost("/no/op/docker"); | ||
| expect(result).toBe("unix:///bar/docker/socket"); | ||
|
|
||
| expect(mockExecFileSync).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should use Docker context when no env vars are set", async () => { | ||
| const result = await resolveDockerHost("/no/op/docker"); | ||
| expect(result).toBe("unix:///current/run/docker.sock"); | ||
| expect(mockExecFileSync).toHaveBeenCalledWith( | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| "/no/op/docker", | ||
| ["context", "ls", "--format", "json"], | ||
| { encoding: "utf8" } | ||
| ); | ||
| }); | ||
|
|
||
| it("should fall back to platform default on Unix when context fails", async () => { | ||
emily-shen marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| mockExecFileSync.mockImplementation(() => { | ||
| throw new Error("Docker command failed"); | ||
| }); | ||
|
|
||
| const result = await resolveDockerHost("/no/op/docker"); | ||
| expect(result).toBe("unix:///var/run/docker.sock"); | ||
| }); | ||
| } | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,7 @@ | ||
| import events from "node:events"; | ||
| import { fetch, Request } from "undici"; | ||
| import { startDev } from "../dev"; | ||
| import { | ||
| getDockerHost, | ||
| getDockerPath, | ||
| } from "../environment-variables/misc-variables"; | ||
| import { getDockerPath } from "../environment-variables/misc-variables"; | ||
| import { run } from "../experimental-flags"; | ||
| import { logger } from "../logger"; | ||
| import type { Environment } from "../config"; | ||
|
|
@@ -165,6 +162,8 @@ export async function unstable_dev( | |
| const defaultLogLevel = testMode ? "warn" : "log"; | ||
| const local = options?.local ?? true; | ||
|
|
||
| const dockerPath = options?.experimental?.dockerPath ?? getDockerPath(); | ||
|
|
||
| const devOptions: StartDevOptions = { | ||
| script: script, | ||
| inspect: false, | ||
|
|
@@ -227,8 +226,8 @@ export async function unstable_dev( | |
| enableIpc: options?.experimental?.enableIpc, | ||
| nodeCompat: undefined, | ||
| enableContainers: options?.experimental?.enableContainers ?? false, | ||
| dockerPath: options?.experimental?.dockerPath ?? getDockerPath(), | ||
| containerEngine: options?.experimental?.containerEngine ?? getDockerHost(), | ||
| dockerPath, | ||
| containerEngine: options?.experimental?.containerEngine, | ||
|
Comment on lines
+229
to
+230
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the diff on the first line is only moving code around. Is the second line diff expected ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep - i don't want to try and get the socket path unless we are sure there are containers configured and enableContainers is true, but i don't think we have read the config yet here so we can't do that. instead i set it later on in configController. |
||
| }; | ||
|
|
||
| //outside of test mode, rebuilds work fine, but only one instance of wrangler will work at a time | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions:
Should "feat" imply a minor release?
Should "docker socket" be "container engine socket" (and use "container engine" below and in the other changeset rather than "docker" or "container tool")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our general rule of thumb apparently is features on experimental products get releases as patches, so i'll probably keep this as is.
good point on the docker specific language, i'll update the changesets