Skip to content

Commit 8f748ec

Browse files
lpcoxCopilot
andcommitted
fix: address review feedback on DinD configuration
- Export getLocalDockerEnv() and use it in host-iptables.ts for all docker CLI calls (network inspect/create/rm) so they target the local daemon when DOCKER_HOST points at an external TCP daemon - Forward full set of Docker client env vars into agent container (DOCKER_TLS_VERIFY, DOCKER_CERT_PATH, DOCKER_CONTEXT, etc.) not just DOCKER_HOST, so TLS-authenticated DinD daemons work - Update warning message to describe auto-redirect behavior instead of reusing old fatal error text - Validate --docker-host flag is a unix:// URI; reject TCP/SSH - Remove extra blank line before Troubleshooting in docs/environment - Update host-iptables tests for new env option in docker calls Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent f5c2cb9 commit 8f748ec

5 files changed

Lines changed: 41 additions & 20 deletions

File tree

docs/environment.md

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -246,8 +246,6 @@ The DinD TCP address (e.g., `tcp://localhost:2375`) typically refers to the runn
246246
- **`--enable-host-access`** — allows the agent to reach `host.docker.internal` and set `DOCKER_HOST=tcp://host.docker.internal:2375` inside the agent.
247247
- **`--enable-dind`** — mounts the local Docker socket (`/var/run/docker.sock`) directly into the agent container (only works when using the local daemon, not a remote DinD TCP socket).
248248

249-
250-
251249
## Troubleshooting
252250

253251
**Variable not accessible:** Use `sudo -E` or pass explicitly with `--env VAR="$VAR"`

src/cli.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1615,9 +1615,8 @@ program
16151615
// agent workload can still reach the DinD daemon.
16161616
const dockerHostCheck = checkDockerHost();
16171617
if (!dockerHostCheck.valid) {
1618-
logger.warn(`⚠️ ${dockerHostCheck.error}`);
1619-
logger.warn(' AWF will use the local Docker socket for its own containers.');
1620-
logger.warn(' The original DOCKER_HOST is forwarded into the agent container.');
1618+
logger.warn('⚠️ External DOCKER_HOST detected. AWF will redirect its own Docker calls to the local socket.');
1619+
logger.warn(' The original DOCKER_HOST (and related Docker client env vars) are forwarded into the agent container.');
16211620
}
16221621

16231622
// Parse domains from both --allow-domains flag and --allow-domains-file
@@ -1924,6 +1923,11 @@ program
19241923

19251924
// Apply --docker-host override for AWF's own container operations.
19261925
// This must be called before startContainers/stopContainers/runAgentCommand.
1926+
if (config.awfDockerHost && !config.awfDockerHost.startsWith('unix://')) {
1927+
logger.error(`❌ --docker-host must be a unix:// socket URI, got: ${config.awfDockerHost}`);
1928+
logger.error(' Example: --docker-host unix:///run/user/1000/docker.sock');
1929+
process.exit(1);
1930+
}
19271931
setAwfDockerHost(config.awfDockerHost);
19281932

19291933
// Parse and validate --agent-timeout

src/docker-manager.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ export function setAwfDockerHost(host: string | undefined): void {
6565
* The original DOCKER_HOST value is NOT removed from the agent container's
6666
* environment — see generateDockerCompose for the passthrough logic.
6767
*/
68-
function getLocalDockerEnv(): NodeJS.ProcessEnv {
68+
export function getLocalDockerEnv(): NodeJS.ProcessEnv {
6969
const env = { ...process.env };
7070

7171
if (awfDockerHostOverride !== undefined) {
@@ -828,10 +828,23 @@ export function generateDockerCompose(
828828
if (process.env.ACTIONS_ID_TOKEN_REQUEST_URL) environment.ACTIONS_ID_TOKEN_REQUEST_URL = process.env.ACTIONS_ID_TOKEN_REQUEST_URL;
829829
if (process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN) environment.ACTIONS_ID_TOKEN_REQUEST_TOKEN = process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN;
830830

831-
// Forward DOCKER_HOST so the agent workload can reach a DinD daemon or custom Docker socket.
832-
// AWF itself uses the local socket (see getLocalDockerEnv), but the agent container should
833-
// inherit the original value so docker commands inside the agent work as expected.
834-
if (process.env.DOCKER_HOST) environment.DOCKER_HOST = process.env.DOCKER_HOST;
831+
// Forward Docker client environment so the agent workload can reach the same DinD daemon,
832+
// custom Docker socket, or TCP endpoint as the parent process. DOCKER_HOST alone is not
833+
// sufficient for TLS/authenticated daemons; the companion Docker client variables must also
834+
// be preserved so docker commands inside the agent work as expected.
835+
const dockerClientEnvVars = [
836+
'DOCKER_HOST',
837+
'DOCKER_TLS',
838+
'DOCKER_TLS_VERIFY',
839+
'DOCKER_CERT_PATH',
840+
'DOCKER_CONTEXT',
841+
'DOCKER_CONFIG',
842+
'DOCKER_API_VERSION',
843+
'DOCKER_DEFAULT_PLATFORM',
844+
] as const;
845+
for (const dockerEnvVar of dockerClientEnvVars) {
846+
if (process.env[dockerEnvVar]) environment[dockerEnvVar] = process.env[dockerEnvVar]!;
847+
}
835848

836849
}
837850

src/host-iptables.test.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@ import execa from 'execa';
55
jest.mock('execa');
66
const mockedExeca = execa as jest.MockedFunction<typeof execa>;
77

8+
// Mock getLocalDockerEnv to return a predictable env for assertions
9+
jest.mock('./docker-manager', () => ({
10+
getLocalDockerEnv: () => process.env,
11+
}));
12+
813
// Mock logger to avoid console output during tests
914
jest.mock('./logger', () => ({
1015
logger: {
@@ -41,8 +46,8 @@ describe('host-iptables', () => {
4146
});
4247

4348
// Should only check if network exists, not create it
44-
expect(mockedExeca).toHaveBeenCalledWith('docker', ['network', 'inspect', 'awf-net']);
45-
expect(mockedExeca).not.toHaveBeenCalledWith('docker', expect.arrayContaining(['network', 'create']));
49+
expect(mockedExeca).toHaveBeenCalledWith('docker', ['network', 'inspect', 'awf-net'], { env: expect.any(Object) });
50+
expect(mockedExeca).not.toHaveBeenCalledWith('docker', expect.arrayContaining(['network', 'create']), expect.anything());
4651
});
4752

4853
it('should create network when it does not exist', async () => {
@@ -65,7 +70,7 @@ describe('host-iptables', () => {
6570
proxyIp: '172.30.0.30',
6671
});
6772

68-
expect(mockedExeca).toHaveBeenCalledWith('docker', ['network', 'inspect', 'awf-net']);
73+
expect(mockedExeca).toHaveBeenCalledWith('docker', ['network', 'inspect', 'awf-net'], { env: expect.any(Object) });
6974
expect(mockedExeca).toHaveBeenCalledWith('docker', [
7075
'network',
7176
'create',
@@ -74,7 +79,7 @@ describe('host-iptables', () => {
7479
'172.30.0.0/24',
7580
'--opt',
7681
'com.docker.network.bridge.name=fw-bridge',
77-
]);
82+
], { env: expect.any(Object) });
7883
});
7984
});
8085

@@ -1101,7 +1106,7 @@ describe('host-iptables', () => {
11011106

11021107
await cleanupFirewallNetwork();
11031108

1104-
expect(mockedExeca).toHaveBeenCalledWith('docker', ['network', 'rm', 'awf-net'], { reject: false });
1109+
expect(mockedExeca).toHaveBeenCalledWith('docker', ['network', 'rm', 'awf-net'], { reject: false, env: expect.any(Object) });
11051110
});
11061111

11071112
it('should not throw on errors (best-effort cleanup)', async () => {

src/host-iptables.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import execa from 'execa';
22
import { logger } from './logger';
33
import { API_PROXY_PORTS } from './types';
44
import { DEFAULT_DNS_SERVERS } from './dns-resolver';
5+
import { getLocalDockerEnv } from './docker-manager';
56

67
const NETWORK_NAME = 'awf-net';
78
const CHAIN_NAME = 'FW_WRAPPER';
@@ -71,7 +72,7 @@ async function getNetworkBridgeName(): Promise<string | null> {
7172
NETWORK_NAME,
7273
'-f',
7374
'{{index .Options "com.docker.network.bridge.name"}}',
74-
]);
75+
], { env: getLocalDockerEnv() });
7576
const bridgeName = stdout.trim();
7677
return bridgeName || null;
7778
} catch (error) {
@@ -89,7 +90,7 @@ export async function getDockerBridgeGateway(): Promise<string | null> {
8990
const { stdout } = await execa('docker', [
9091
'network', 'inspect', 'bridge',
9192
'-f', '{{(index .IPAM.Config 0).Gateway}}',
92-
]);
93+
], { env: getLocalDockerEnv() });
9394
const gateway = stdout.trim();
9495
if (!gateway) return null;
9596
// Validate IPv4 format before using in iptables rules
@@ -173,7 +174,7 @@ export async function ensureFirewallNetwork(): Promise<{
173174
// Check if network already exists
174175
let networkExists = false;
175176
try {
176-
await execa('docker', ['network', 'inspect', NETWORK_NAME]);
177+
await execa('docker', ['network', 'inspect', NETWORK_NAME], { env: getLocalDockerEnv() });
177178
networkExists = true;
178179
logger.debug(`Network '${NETWORK_NAME}' already exists`);
179180
} catch {
@@ -191,7 +192,7 @@ export async function ensureFirewallNetwork(): Promise<{
191192
NETWORK_SUBNET,
192193
'--opt',
193194
'com.docker.network.bridge.name=fw-bridge',
194-
]);
195+
], { env: getLocalDockerEnv() });
195196
logger.success(`Created network '${NETWORK_NAME}' with bridge 'fw-bridge'`);
196197
}
197198

@@ -693,7 +694,7 @@ export async function cleanupFirewallNetwork(): Promise<void> {
693694
logger.debug(`Removing firewall network '${NETWORK_NAME}'...`);
694695

695696
try {
696-
await execa('docker', ['network', 'rm', NETWORK_NAME], { reject: false });
697+
await execa('docker', ['network', 'rm', NETWORK_NAME], { reject: false, env: getLocalDockerEnv() });
697698
logger.debug('Firewall network removed');
698699
} catch (error) {
699700
logger.debug('Error removing firewall network:', error);

0 commit comments

Comments
 (0)