fix: use docker cp instead of file bind mounts for DinD compatibility#1079
fix: use docker cp instead of file bind mounts for DinD compatibility#1079
Conversation
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
Bun Build Test Results
Overall: ✅ PASS
|
|
🤖 Smoke test results for
Overall: PASS
|
🦕 Deno Build Test Results
Overall: ✅ PASS
|
There was a problem hiding this comment.
Pull request overview
This PR updates AWF’s Docker Compose startup flow to work in Docker-in-Docker (DinD) environments by avoiding file bind mounts for generated configuration artifacts and injecting them via the Docker API.
Changes:
- Removes
squid.conf(and SSL cert/key) file bind mounts and injects them withdocker cp. - Splits container startup into create (
up --no-start) → inject → start (up -d --no-recreate) phases. - Updates unit tests to assert the new 3-phase startup behavior and SSL file injection paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/docker-manager.ts | Switches from bind-mounting config files to docker cp injection and implements a 3-phase compose startup. |
| src/docker-manager.test.ts | Updates tests to reflect the new compose create/inject/start flow and SSL injection expectations. |
Comments suppressed due to low confidence (1)
src/docker-manager.ts:270
- The comments here suggest directory bind mounts are safe in DinD because Docker will create missing directories, but for mounts where AWF expects pre-populated host content (e.g., ssl_db initialized by initSslDb, or host-side log inspection via checkSquidLogs), creating an empty directory on the daemon side is not equivalent. Consider tightening this comment to avoid implying DinD compatibility for all directory mounts, or document which mounts still require a shared host/daemon filesystem.
// Note: cert and key FILES are injected via `docker cp` in startContainers()
// to support DinD environments. Only the ssl_db DIRECTORY is bind-mounted here
// since directory mounts work in DinD (Docker creates them as dirs by default).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add SSL-related volumes if SSL Bump is enabled | ||
| // Note: cert and key FILES are injected via `docker cp` in startContainers() | ||
| // to support DinD environments. Only the ssl_db DIRECTORY is bind-mounted here | ||
| // since directory mounts work in DinD (Docker creates them as dirs by default). | ||
| if (sslConfig) { | ||
| squidVolumes.push(`${sslConfig.caFiles.certPath}:${sslConfig.caFiles.certPath}:ro`); | ||
| squidVolumes.push(`${sslConfig.caFiles.keyPath}:${sslConfig.caFiles.keyPath}:ro`); | ||
| // Mount SSL database at /var/spool/squid_ssl_db (Squid's expected location) | ||
| squidVolumes.push(`${sslConfig.sslDbPath}:/var/spool/squid_ssl_db:rw`); | ||
| } |
There was a problem hiding this comment.
The ssl_db directory is still bind-mounted from a host path (sslConfig.sslDbPath) into /var/spool/squid_ssl_db. In DinD/remote-daemon setups, that host path may not be visible to the Docker daemon, so Docker will mount an empty directory created on the daemon side; this breaks SSL Bump because the DB is expected to be pre-initialized (initSslDb) and security_file_certgen is invoked without creating it. Consider injecting the initialized ssl_db directory via docker cp, or switching to a named volume and initializing the DB inside the squid container before starting it.
This issue also appears on line 268 of the same file.
See below for a potential fix:
// to support DinD/remote-daemon environments. The ssl_db DIRECTORY is provided
// via a Docker-managed named volume so it does not rely on host path visibility.
if (sslConfig) {
// Mount SSL database at /var/spool/squid_ssl_db (Squid's expected location)
// using a named volume instead of a host bind mount.
squidVolumes.push('awf-squid-ssl-db:/var/spool/squid_ssl_db:rw');
src/docker-manager.ts
Outdated
| // Inject SSL cert/key files if SSL Bump is enabled | ||
| const sslCertPath = path.join(workDir, 'ssl', 'ca-cert.pem'); | ||
| const sslKeyPath = path.join(workDir, 'ssl', 'ca-key.pem'); | ||
| if (fs.existsSync(sslCertPath)) { | ||
| // Inject cert and key at fixed container paths that squid.conf references | ||
| // (SQUID_CONTAINER_SSL_CERT and SQUID_CONTAINER_SSL_KEY constants). | ||
| // Parent directory /etc/squid/ already exists in the squid image. | ||
| await execa('docker', ['cp', sslCertPath, `awf-squid:${SQUID_CONTAINER_SSL_CERT}`], { | ||
| stdio: 'inherit', | ||
| }); | ||
| await execa('docker', ['cp', sslKeyPath, `awf-squid:${SQUID_CONTAINER_SSL_KEY}`], { | ||
| stdio: 'inherit', | ||
| }); |
There was a problem hiding this comment.
SSL cert/key injection is gated only on the cert file existing, but the code unconditionally attempts to docker cp the key as well. If the cert exists without the key (partial/failed previous run, manual setup), this will fail with a confusing docker error. Gate on both files existing and emit a clear error when only one is present.
Go Build Test Results
Overall: ✅ PASS
|
C++ Build Test Results
Overall: PASS ✅ All C++ projects configured and built successfully with GCC 13.3.0.
|
|
fix: use docker cp instead of file bind mounts for DinD compatibility
|
🦀 Rust Build Test Results
Overall: ✅ PASS
|
.NET Build Test Results
Overall: PASS Run outputhello-world:
|
🟢 Node.js Build Test Results
Overall: ✅ PASS
|
Java Build Test Results
Overall: PASS ✅
|
Smoke Test Results
Overall: PASS
|
Chroot Version Comparison Results
Overall: ❌ Not all versions match — Go matches, but Python and Node.js differ between host and chroot environments.
|
293d857 to
a7b0fb6
Compare
Replace the squid.conf file bind mount with a base64-encoded environment variable (AWF_SQUID_CONFIG_B64) to support Docker-in-Docker environments like ARC self-hosted runners. In DinD, the Docker daemon runs in a separate container and cannot access files on the host filesystem. File bind mounts fail with "not a directory" errors because the daemon creates a directory at the non-existent path. Instead of bind-mounting squid.conf, the config is: 1. Base64-encoded and passed as AWF_SQUID_CONFIG_B64 env var 2. Decoded by an entrypoint override before squid starts This works universally because env vars are part of the container spec sent via the Docker API, bypassing filesystem sharing entirely. The startup flow (docker compose up -d) is unchanged, so all existing integration tests and behavior remain compatible. Fixes github/gh-aw#18385 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
C++ Build Test Results
Overall: PASS 🎉
|
.NET Build Test Results
Overall: PASS Run outputhello-world:
|
🦀 Rust Build Test Results
Overall: ✅ PASS Test Detailsfd:
|
Smoke Test Results
Overall: PASS
|
☕ Java Build Test Results
Overall: ✅ PASS Both projects compiled and all tests passed successfully.
|
Node.js Build Test Results ✅
Overall: ✅ PASS All 3 Node.js projects installed and tested successfully.
|
Chroot Version Comparison Results
Result: ❌ Not all versions match — Python and Node.js versions differ between host and chroot environments.
|
Run npm audit fix to resolve: - ajv <6.14.0: moderate severity ReDoS with $data option - minimatch 10.0.0-10.2.2: high severity ReDoS via GLOBSTAR segments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Docker Compose outputs build progress and container status to stdout during `docker compose up -d` and `docker compose down -v`. When tests capture AWF's stdout to check agent command output, the Docker Compose output gets mixed in, breaking assertions. Redirect Docker Compose stdout to process.stderr so: - Users still see progress output in their terminal (stderr is visible) - Test runners only capture agent command output on stdout - AWF's stderr-based logging remains consistent The `docker logs -f` command for agent output streaming retains stdio: inherit since that output IS the agent's actual stdout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Node.js Build Test Results ✅
Overall: PASS
|
.NET Build Test Results
Overall: PASS Run outputhello-world:
|
Chroot Version Comparison Results
Result: Some versions do not match. Go matches, but Python and Node.js differ between host and chroot environments.
|
Java Build Test Results
Overall: ✅ PASS All projects compiled and tests passed successfully via Maven with Squid proxy.
|
|
Smoke test results for run ✅ GitHub MCP — Last 2 merged PRs: #1078 "fix: add explicit execute directive to smoke-codex to prevent noop", #1070 "chore: investigate issue duplication detector workflow failure" Overall: PASS | PR author:
|
|
Smoke test results ✅ GitHub MCP: #1003 chore(deps): bump all-npm-dependencies, #1078 fix: add explicit execute directive to smoke-codex Overall: PASS
|
Rust Build Test Results
Overall: PASS ✅
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Go Build Test Results ✅
Overall: PASS
|
🧪 Bun Build Test Results
Overall: ✅ PASS Bun version:
|
Build Test: Node.js ✅
Overall: PASS
|
Smoke Test Results ✅ PASS
PR author:
|
C++ Build Test Results
Overall: PASS
|
.NET Build Test Results
Overall: PASS Run outputhello-world: json-parse:
|
Smoke Test Results
Overall: PASS
|
Rust Build Test Results
Overall: ✅ PASS
|
Java Build Test Results
Overall: PASS ✅
|
Deno Build Test Results
Overall: ✅ PASS
|
Chroot Version Comparison Results
Result: FAILED — Python and Node.js versions differ between host and chroot environments.
|
|
Overall: FAIL
|
Summary
Fixes #18385 (github/gh-aw#18385)
Three fixes in this PR:
Fix 1: DinD squid.conf injection
In DinD, the Docker daemon runs in a separate
dindsidecar container. File bind mounts fail because the daemon can't access files on the runner's filesystem. Instead of bind-mountingsquid.conf, the config is:AWF_SQUID_CONFIG_B64env varFix 2: Dependency vulnerabilities
npm audit fixresolves:ajv<6.14.0: moderate severity ReDoS with$dataoptionminimatch10.0.0-10.2.2: high severity ReDoS via GLOBSTAR segmentsFix 3: Docker Compose stdout isolation
Docker Compose outputs build progress and container status to stdout during
docker compose up -d. When integration tests capture AWF's stdout, this build output gets mixed in, breaking test assertions. Fixed by redirecting Docker Compose stdout toprocess.stderr, keeping it visible to users while isolating agent command output for tests.Changes
src/docker-manager.ts: env var injection, entrypoint override, YAML lineWidth fix, stdout→stderr redirectsrc/docker-manager.test.ts: updated test expectationspackage-lock.json: npm audit fixTest plan
npm run build)npm run lint— 0 errors)npm audit— 0 vulnerabilities🤖 Generated with Claude Code