-
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 all 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(containers): try to automatically get the socket path that the container engine is listening on. | ||
|
|
||
| Currently, if your container engine isn't set up to listen on `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 on `unix:///var/run/docker.sock`. | ||
|
|
||
| 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 socket path that the container engine is listening on. | ||
|
|
||
| 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[]) => { | ||
| 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,85 @@ export async function checkExposedPorts( | |
| export function generateContainerBuildId() { | ||
| return randomUUID().slice(0, 8); | ||
| } | ||
|
|
||
| /** | ||
| * Output of docker context ls --format json | ||
| */ | ||
| 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 socket from the currently active Docker context | ||
| * @returns The socket path or null if we are not able to determine it | ||
| */ | ||
| export function getDockerSocketFromContext(dockerPath: string): string | null { | ||
| 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) { | ||
| return currentContext.DockerEndpoint; | ||
| } | ||
| } catch { | ||
| // Fall back to null if docker context inspection fails so that we can use platform defaults | ||
| } | ||
| 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): string { | ||
| 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` | ||
|
|
||
| const contextSocket = getDockerSocketFromContext(dockerPath); | ||
| if (contextSocket) { | ||
| return contextSocket; | ||
| } | ||
|
|
||
| // 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 = (): string => { | ||
| const fromEnv = process.env.WRANGLER_DOCKER_HOST ?? process.env.DOCKER_HOST; | ||
|
|
||
| return fromEnv ?? process.platform === "win32" | ||
| ? "//./pipe/docker_engine" | ||
| : "unix:///var/run/docker.sock"; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { execFileSync } from "child_process"; | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { resolveDockerHost } from "../src/utils"; | ||
|
|
||
| vi.mock("node:child_process"); | ||
| // We can only really run these tests on Linux, because we build our images for linux/amd64, | ||
| // and github runners don't really support container virtualization in any sane way | ||
| 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", | ||
| () => { | ||
| afterEach(() => { | ||
| vi.unstubAllEnvs(); | ||
| }); | ||
|
|
||
| it("should return WRANGLER_DOCKER_HOST when set", async () => { | ||
| vi.stubEnv( | ||
| "WRANGLER_DOCKER_HOST", | ||
| "unix:///FROM/WRANGLER/DOCKER/HOST/wrangler/socket" | ||
| ); | ||
| vi.stubEnv("DOCKER_HOST", "unix:///FROM/DOCKER/HOST/docker/socket"); | ||
|
|
||
| const result = resolveDockerHost("/no/op/docker"); | ||
| expect(result).toBe("unix:///FROM/WRANGLER/DOCKER/HOST/wrangler/socket"); | ||
| }); | ||
|
|
||
| it("should return DOCKER_HOST when WRANGLER_DOCKER_HOST is not set", async () => { | ||
| vi.stubEnv("WRANGLER_DOCKER_HOST", undefined); | ||
| vi.stubEnv("DOCKER_HOST", "unix:///FROM/DOCKER/HOST/docker/socket"); | ||
|
|
||
| const result = resolveDockerHost("/no/op/docker"); | ||
| expect(result).toBe("unix:///FROM/DOCKER/HOST/docker/socket"); | ||
| }); | ||
|
|
||
| it("should use Docker context when no env vars are set", async () => { | ||
| vi.mocked(execFileSync) | ||
| .mockReturnValue(`{"Current":true,"Description":"Current DOCKER_HOST based configuration","DockerEndpoint":"unix:///FROM/CURRENT/CONTEXT/run/docker.sock","Error":"","Name":"default"} | ||
| {"Current":false,"Description":"Docker Desktop","DockerEndpoint":"unix:///FROM/OTHER/CONTEXT/run/docker.sock","Error":"","Name":"desktop-linux"}`); | ||
| const result = resolveDockerHost("/no/op/docker"); | ||
| expect(result).toBe("unix:///FROM/CURRENT/CONTEXT/run/docker.sock"); | ||
| }); | ||
|
|
||
| it("should fall back to platform default when context fails", () => { | ||
| vi.mocked(execFileSync).mockImplementation(() => { | ||
| throw new Error("Docker command failed"); | ||
| }); | ||
|
|
||
| const result = 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.
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?
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.
the
unstable_getMiniflareWorkerOptionsapi needs to use this to resolve the socket path, and that function is sync (and part of wranglers public api surface) so i wanted to avoid changing that.