Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/artifact-permissions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,30 @@ describe('artifact-permissions', () => {
}
});

it('does not warn for benign permission errors on restricted runners', () => {
const auditDir = makeTempDir();
let errorSpy: jest.SpyInstance | undefined;
try {
getuidSpy = jest.spyOn(process, 'getuid').mockReturnValue(1001);
errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
mockExecaSync.mockReturnValue({
stdout: '',
stderr: 'chmod: /fix: Operation not permitted',
exitCode: 1,
});
fixArtifactPermissionsForRootless([auditDir], undefined, undefined, undefined, undefined);
// At the default 'info' log level, the benign case is logged at debug
// (suppressed) and must never surface as a [WARN] ... failed message.
const warnedFailure = (errorSpy.mock.calls as unknown[][]).some(
call => typeof call[0] === 'string' && /\[WARN\].*repair failed/i.test(call[0]),
);
expect(warnedFailure).toBe(false);
} finally {
errorSpy?.mockRestore();
fs.rmSync(auditDir, { recursive: true, force: true });
}
});

it('runs rootless permission repair with translated mount paths', () => {
const auditDir = makeTempDir();
try {
Expand Down
22 changes: 18 additions & 4 deletions src/artifact-permissions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
}

const existingDirs = dirs.filter(
(dir): dir is string => typeof dir === 'string' && dir.length > 0 && fs.existsSync(dir),

Check warning on line 37 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / ESLint

Found existsSync from package "fs" with non literal argument at index 0

Check warning on line 37 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / Build and Lint (Node 22)

Found existsSync from package "fs" with non literal argument at index 0

Check warning on line 37 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / Build and Lint (Node 20)

Found existsSync from package "fs" with non literal argument at index 0
);
if (existingDirs.length === 0) {
return;
Expand Down Expand Up @@ -65,9 +65,9 @@
'--cap-add',
'FOWNER',
'-e',
`TUID=${uid}`,

Check warning on line 68 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / ESLint

Avoid template literals with expressions in execa arguments. Pass arguments as separate array elements

Check warning on line 68 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / Build and Lint (Node 22)

Avoid template literals with expressions in execa arguments. Pass arguments as separate array elements

Check warning on line 68 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / Build and Lint (Node 20)

Avoid template literals with expressions in execa arguments. Pass arguments as separate array elements
'-e',
`TGID=${gid}`,

Check warning on line 70 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / ESLint

Avoid template literals with expressions in execa arguments. Pass arguments as separate array elements

Check warning on line 70 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / Build and Lint (Node 22)

Avoid template literals with expressions in execa arguments. Pass arguments as separate array elements

Check warning on line 70 in src/artifact-permissions.ts

View workflow job for this annotation

GitHub Actions / Build and Lint (Node 20)

Avoid template literals with expressions in execa arguments. Pass arguments as separate array elements
'-v',
mount,
imageRef,
Expand All @@ -80,10 +80,24 @@

if (typeof result.exitCode === 'number' && result.exitCode !== 0) {
const stderr = result.stderr?.trim();
logger.warn(
`Rootless artifact permission repair failed for ${dir} (exit ${result.exitCode})` +
(stderr ? `: ${stderr}` : ''),
);
// Ownership/permission repair is best-effort: the agent has already
// finished and its artifacts are still readable by the owning user.
// On rootless or restricted runners (e.g. ARC/DinD with a non-root
// runner container) the repair container may be denied CHOWN/chmod,
// producing "Operation not permitted" / "Permission denied". Those are
// expected and non-fatal, so log them at debug to avoid alarming users
// who otherwise see a scary WARN for a benign, non-blocking condition.
const isBenignPermissionError =
!!stderr && /(?:^|\n)(?:chown|chmod):.*(?:operation not permitted|permission denied|EPERM|EACCES)/i.test(stderr);
const detail = `for ${dir} (exit ${result.exitCode})` + (stderr ? `: ${stderr}` : '');
if (isBenignPermissionError) {
logger.debug(
`Rootless artifact permission repair skipped ${detail}. ` +
`This is expected on restricted runners and does not affect the run.`,
);
} else {
logger.warn(`Rootless artifact permission repair failed ${detail}`);
}
}
} catch (error) {
logger.warn(`Rootless artifact permission repair failed for ${dir}:`, error);
Expand Down
51 changes: 47 additions & 4 deletions src/cli-workflow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,13 @@ const runWorkflowWithDefaults = async (
};

describe('runMainWorkflow', () => {
beforeEach(() => {
// Default: topology peer lookup returns an empty map so onNetworkReady's
// static-DNS pre-registration runs without throwing in tests that don't
// configure specific peers.
(topology.getTopologyContainerIps as jest.Mock).mockResolvedValue(new Map());
});

it('executes workflow steps in order and logs success for zero exit code', async () => {
const callOrder: string[] = [];
const dependencies = createOrderedWorkflowDependencies(callOrder);
Expand Down Expand Up @@ -651,7 +658,7 @@ describe('runMainWorkflow', () => {
expect(performCleanup).not.toHaveBeenCalled();
});

describe('onNetworkReady with runtimeNeedsStaticDns', () => {
describe('onNetworkReady static DNS pre-registration', () => {
const mockedRuntimeNeedsStaticDns = containerRuntime.runtimeNeedsStaticDns as jest.MockedFunction<typeof containerRuntime.runtimeNeedsStaticDns>;
const mockedGetTopologyContainerIps = topology.getTopologyContainerIps as jest.MockedFunction<typeof topology.getTopologyContainerIps>;
const mockedPatchComposeWithTopologyHosts = topology.patchComposeWithTopologyHosts as jest.MockedFunction<typeof topology.patchComposeWithTopologyHosts>;
Expand Down Expand Up @@ -759,13 +766,49 @@ describe('runMainWorkflow', () => {
expect(mockedPatchComposeWithTopologyHosts).toHaveBeenCalled();
});

it('does not call getTopologyContainerIps when runtimeNeedsStaticDns is false', async () => {
it('pre-registers topology hosts under network isolation even when runtimeNeedsStaticDns is false', async () => {
// Embedded DNS is also unreliable on ARC/DinD with the standard runtime,
// so pre-registration must happen for all network-isolation runs, not
// only gVisor.
mockedRuntimeNeedsStaticDns.mockReturnValue(false);
const peerIps = new Map([['mcp-gateway', '172.30.0.100']]);
mockedGetTopologyContainerIps.mockResolvedValue(peerIps);
mockedPatchComposeWithTopologyHosts.mockImplementation(() => {});

const config: WrapperConfig = {
...baseConfig,
networkIsolation: true,
topologyAttach: ['mcp-gateway'],
};

const startContainers = jest.fn().mockImplementation(
async (_workDir: string, _domains: string[], _logs?: string, _skip?: boolean, onNetworkReady?: () => Promise<void>) => {
if (onNetworkReady) await onNetworkReady();
},
);

await runMainWorkflow(
config,
createWorkflowDependencies({ startContainers, connectTopologyContainers: jest.fn() }),
createWorkflowOptions(),
);

expect(mockedGetTopologyContainerIps).toHaveBeenCalledWith('awf-net', ['mcp-gateway']);
const patchCall = mockedPatchComposeWithTopologyHosts.mock.calls[0][1] as Map<string, string>;
expect(patchCall.get('squid-proxy')).toBe('172.30.0.10');
});

it('adds cli-proxy entry when difcProxyHost is set', async () => {
mockedRuntimeNeedsStaticDns.mockReturnValue(false);
const peerIps = new Map([['peer', '10.0.0.1']]);
mockedGetTopologyContainerIps.mockResolvedValue(peerIps);
mockedPatchComposeWithTopologyHosts.mockImplementation(() => {});

const config: WrapperConfig = {
...baseConfig,
networkIsolation: true,
topologyAttach: ['peer'],
difcProxyHost: 'proxy.corp.com:18443',
};

const startContainers = jest.fn().mockImplementation(
Expand All @@ -780,8 +823,8 @@ describe('runMainWorkflow', () => {
createWorkflowOptions(),
);

expect(mockedGetTopologyContainerIps).not.toHaveBeenCalled();
expect(mockedPatchComposeWithTopologyHosts).not.toHaveBeenCalled();
const patchCall = mockedPatchComposeWithTopologyHosts.mock.calls[0][1] as Map<string, string>;
expect(patchCall.get('cli-proxy')).toBe('172.30.0.50');
});
});
});
34 changes: 25 additions & 9 deletions src/cli-workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { HostAccessConfig, CliProxyHostConfig } from './host-iptables';
import { DEFAULT_DNS_SERVERS } from './dns-resolver';
import { parseDifcProxyHost } from './docker-manager';
import { CLI_PROXY_IP, DOH_PROXY_IP, SQUID_IP, API_PROXY_IP } from './host-iptables-shared';
import { buildInternalServiceHosts } from './services/internal-service-hosts';
import { TOPOLOGY_NETWORK_NAME, getTopologyContainerIps, patchComposeWithTopologyHosts } from './topology';
import { runtimeNeedsStaticDns } from './container-runtime';

/**
* Dependencies injected into the main workflow.
Expand Down Expand Up @@ -133,18 +133,34 @@ export async function runMainWorkflow(
logger.info(`Attaching ${config.topologyAttach!.length} trusted container(s) to the internal network...`);
await dependencies.connectTopologyContainers!(TOPOLOGY_NETWORK_NAME, config.topologyAttach!);

// When the agent uses a runtime whose network stack cannot reach
// Docker's embedded DNS (e.g. gVisor), inject /etc/hosts entries for
// topology peers and compose-internal services so hostname resolution
// works without DNS.
if (runtimeNeedsStaticDns(config.containerRuntime)) {
// Docker's embedded DNS (127.0.0.11) is not always reachable from
// inside the sandbox: gVisor's userspace netstack cannot reach it,
// and on ARC/DinD runners the Docker-in-Docker network does not
// forward lookups to the Kubernetes cluster resolver — which is what
// produces "getaddrinfo EAI_AGAIN <peer>" failures.
//
// Every peer we might resolve here is known in advance with a fixed
// IP: the topology peers (e.g. the MCP gateway) are discovered via
// `getTopologyContainerIps`, and the compose-internal proxies have
// static IPs. So we always pre-register them in /etc/hosts whenever
// network isolation is active. This is a no-op when embedded DNS
// works (the entries match what DNS would return) and prevents the
// DNS-isolation failure when it does not — turning a diagnosis into a
// fix. Previously this ran only for gVisor; ARC/DinD needs it too.
{
const peerIps = await getTopologyContainerIps(TOPOLOGY_NETWORK_NAME, config.topologyAttach!);

// Include compose-internal services whose hostnames the agent may
// need to resolve — normally handled by Docker DNS at 127.0.0.11.
peerIps.set('squid-proxy', SQUID_IP);
if (config.enableApiProxy) {
peerIps.set('api-proxy', API_PROXY_IP);
// Uses the same service→name mapping as the gVisor compose path
// (buildInternalServiceHosts); the topology path sources the fixed
// sidecar IPs from constants since it has no host networkConfig.
for (const [name, ip] of Object.entries(buildInternalServiceHosts({
squidIp: SQUID_IP,
apiProxyIp: config.enableApiProxy ? API_PROXY_IP : undefined,
cliProxyIp: config.difcProxyHost ? CLI_PROXY_IP : undefined,
}))) {
peerIps.set(name, ip);
}

if (peerIps.size > 0) {
Expand Down
27 changes: 21 additions & 6 deletions src/container-lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
handleHealthcheckError,
logContainerLogsToStderr,
reportBlockedDomains,
detectDnsResolutionFailure,
} from './container-startup-diagnostics';
import { checkSquidLogs } from './squid-log-reader';

Expand Down Expand Up @@ -80,13 +81,25 @@ async function attemptContainerStartup(
}
}

function createCliProxyStartupError(): Error {
return new Error(
function createCliProxyStartupError(dnsFailureHost?: string | null): Error {
let message =
`AWF firewall failed to start: ${CLI_PROXY_CONTAINER_NAME} could not connect to the external DIFC proxy (or exited before establishing a connection). ` +
`Failing fast to avoid repeated in-agent retries. ` +
`The agent was never invoked. ` +
`See ${CLI_PROXY_CONTAINER_NAME} container logs above for details.`
);
`See ${CLI_PROXY_CONTAINER_NAME} container logs above for details.`;

if (dnsFailureHost) {
message +=
`\n\nDNS resolution failed for "${dnsFailureHost}" (getaddrinfo EAI_AGAIN/ENOTFOUND). ` +
`On ARC/DinD runners, containers created by the Docker-in-Docker daemon run on the DinD ` +
`Docker network, which does not forward DNS to the Kubernetes cluster resolver. If ` +
`"${dnsFailureHost}" is a Kubernetes Service name, the cli-proxy cannot resolve it. ` +
`To fix this, address the DIFC proxy by IP instead of a Service name, or configure the ` +
`DinD daemon's DNS (e.g. dockerd --dns <kube-dns-ip>) so container lookups reach ` +
`Kubernetes DNS. See https://github.github.io/gh-aw/guides/arc-dind-copilot-agent/ for details.`;
}

return new Error(message);
}

function createRepeatedApiProxyStartupError(): Error {
Expand Down Expand Up @@ -117,7 +130,8 @@ async function handleRetryStartupFailure(
}
if (await didContainerFailStartup(retryErrorMsg, CLI_PROXY_CONTAINER_NAME)) {
await logContainerLogsToStderr(CLI_PROXY_CONTAINER_NAME);
throw createCliProxyStartupError();
const dnsFailureHost = await detectDnsResolutionFailure(CLI_PROXY_CONTAINER_NAME);
throw createCliProxyStartupError(dnsFailureHost);
}
// Any remaining retry error (e.g. squid healthcheck or domain blockage) falls
// through to the Squid log diagnostic path below as if it were the first error.
Expand Down Expand Up @@ -178,7 +192,8 @@ async function handleStartupFailure(

if (firstAttemptCliProxyStartupFailure) {
await logContainerLogsToStderr(CLI_PROXY_CONTAINER_NAME);
throw createCliProxyStartupError();
const dnsFailureHost = await detectDnsResolutionFailure(CLI_PROXY_CONTAINER_NAME);
throw createCliProxyStartupError(dnsFailureHost);
}

await handleHealthcheckError(errorMsg, error, workDir, proxyLogsDir, allowedDomains);
Expand Down
32 changes: 32 additions & 0 deletions src/container-startup-diagnostics-coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
logContainerLogsToStderr,
handleHealthcheckError,
reportBlockedDomains,
detectDnsResolutionFailure,
} from './container-startup-diagnostics';
import { logger } from './logger';
import { checkSquidLogs } from './squid-log-reader';
Expand Down Expand Up @@ -166,6 +167,37 @@ describe('logContainerLogsToStderr', () => {
});
});

// ─── detectDnsResolutionFailure ───────────────────────────────────────────────

describe('detectDnsResolutionFailure', () => {
it('extracts the unresolved host from an EAI_AGAIN log line', async () => {
mockExecaFn.mockResolvedValueOnce(
execaOk('[tcp-tunnel] Upstream error (::1:48644): getaddrinfo EAI_AGAIN awmg-cli-proxy', '', 0),
);
await expect(detectDnsResolutionFailure('awf-cli-proxy')).resolves.toBe('awmg-cli-proxy');
});

it('extracts the unresolved host from an ENOTFOUND log line', async () => {
mockExecaFn.mockResolvedValueOnce(execaOk('', 'getaddrinfo ENOTFOUND difc-proxy.svc', 0));
await expect(detectDnsResolutionFailure('awf-cli-proxy')).resolves.toBe('difc-proxy.svc');
});

it('returns null when no DNS failure appears in the logs', async () => {
mockExecaFn.mockResolvedValueOnce(execaOk('[cli-proxy] connection refused', '', 0));
await expect(detectDnsResolutionFailure('awf-cli-proxy')).resolves.toBeNull();
});

it('returns null when docker logs exits non-zero', async () => {
mockExecaFn.mockResolvedValueOnce(execaOk('', 'No such container', 1));
await expect(detectDnsResolutionFailure('awf-missing')).resolves.toBeNull();
});

it('returns null and swallows exceptions', async () => {
mockExecaFn.mockRejectedValueOnce(new Error('docker CLI not found'));
await expect(detectDnsResolutionFailure('awf-cli-proxy')).resolves.toBeNull();
});
});

// ─── handleHealthcheckError ───────────────────────────────────────────────────

describe('handleHealthcheckError', () => {
Expand Down
30 changes: 30 additions & 0 deletions src/container-startup-diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,36 @@ export async function logContainerLogsToStderr(containerName: string): Promise<v
}
}

/**
* Scans a container's recent logs for a DNS resolution failure of the form
* `getaddrinfo EAI_AGAIN <hostname>` (or `ENOTFOUND`). On ARC/DinD runners the
* cli-proxy runs inside a Docker network managed by the DinD daemon, which does
* not forward DNS to the Kubernetes cluster resolver. When the external DIFC
* proxy is addressed by a Kubernetes Service name (e.g. `awmg-cli-proxy`), the
* lookup fails with EAI_AGAIN and the generic "could not connect" error hides
* the real (DNS-isolation) root cause.
*
* @returns the unresolved hostname when a DNS failure is detected, otherwise null.
*/
export async function detectDnsResolutionFailure(containerName: string): Promise<string | null> {
try {
const result = await execa('docker', ['logs', '--tail', '50', containerName], {
reject: false,
env: getLocalDockerEnv(),
});
if (result.exitCode !== 0) {
return null;
}
const combined = [result.stdout, result.stderr].filter(Boolean).join('\n');
// Matches: "getaddrinfo EAI_AGAIN awmg-cli-proxy" or "getaddrinfo ENOTFOUND host"
const match = combined.match(/getaddrinfo\s+(?:EAI_AGAIN|ENOTFOUND)\s+([^\s:"']+)/i);
return match ? match[1] : null;
} catch (error) {
logger.debug(`Could not scan ${containerName} logs for DNS failures:`, error);
return null;
}
}

/**
* Classifies and logs each blocked target, then emits actionable fix suggestions.
* Extracted to avoid duplicating this logic between the startup-error path
Expand Down
15 changes: 15 additions & 0 deletions src/services/agent-service-build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,21 @@ describe('agent service', () => {
});
});

it('should inject cli-proxy host when cliProxyIp is present', () => {
const configWithRuntime = {
...mockConfig,
containerRuntime: 'gvisor',
};
const networkWithCliProxy = {
...mockNetworkConfig,
cliProxyIp: '172.30.0.50',
};
const result = generateDockerCompose(configWithRuntime, networkWithCliProxy);
const agent = result.services.agent as any;

expect(agent.extra_hosts['cli-proxy']).toBe('172.30.0.50');
});

it('should not inject api-proxy host when proxyIp is absent', () => {
const configWithRuntime = {
...mockConfig,
Expand Down
16 changes: 9 additions & 7 deletions src/services/agent-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
import { ACT_PRESET_BASE_IMAGE, getSafeHostUid, getSafeHostGid } from '../host-identity';
import { buildRuntimeImageRef } from '../image-tag';
import { resolveDockerRuntime, runtimeNeedsStaticDns } from '../container-runtime';
import { buildInternalServiceHosts } from './internal-service-hosts';
import { logger } from '../logger';
import { WrapperConfig } from '../types';
import { NetworkConfig, ImageBuildConfig } from './squid-service';
Expand Down Expand Up @@ -175,13 +176,14 @@ export function buildAgentService(params: AgentServiceParams): any {
// compose-internal services the agent may need to reach by hostname.
// See: https://github.com/google/gvisor/issues/7469
if (runtimeNeedsStaticDns(config.containerRuntime)) {
if (!agentService.extra_hosts) {
agentService.extra_hosts = {};
}
agentService.extra_hosts['squid-proxy'] = networkConfig.squidIp;
if (networkConfig.proxyIp) {
agentService.extra_hosts['api-proxy'] = networkConfig.proxyIp;
}
agentService.extra_hosts = {
...agentService.extra_hosts,
...buildInternalServiceHosts({
squidIp: networkConfig.squidIp,
apiProxyIp: networkConfig.proxyIp,
cliProxyIp: networkConfig.cliProxyIp,
}),
};
logger.debug('Injected compose-internal service hosts for static DNS compatibility');
}
}
Expand Down
Loading
Loading