diff --git a/src/artifact-permissions.test.ts b/src/artifact-permissions.test.ts index 20549b9b6..014a44a3b 100644 --- a/src/artifact-permissions.test.ts +++ b/src/artifact-permissions.test.ts @@ -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 { diff --git a/src/artifact-permissions.ts b/src/artifact-permissions.ts index 0ab545658..de82d34a6 100644 --- a/src/artifact-permissions.ts +++ b/src/artifact-permissions.ts @@ -80,10 +80,24 @@ export function fixArtifactPermissionsForRootless( 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); diff --git a/src/cli-workflow.test.ts b/src/cli-workflow.test.ts index 221cb2032..7783487df 100644 --- a/src/cli-workflow.test.ts +++ b/src/cli-workflow.test.ts @@ -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); @@ -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; const mockedGetTopologyContainerIps = topology.getTopologyContainerIps as jest.MockedFunction; const mockedPatchComposeWithTopologyHosts = topology.patchComposeWithTopologyHosts as jest.MockedFunction; @@ -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) => { + 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; + 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( @@ -780,8 +823,8 @@ describe('runMainWorkflow', () => { createWorkflowOptions(), ); - expect(mockedGetTopologyContainerIps).not.toHaveBeenCalled(); - expect(mockedPatchComposeWithTopologyHosts).not.toHaveBeenCalled(); + const patchCall = mockedPatchComposeWithTopologyHosts.mock.calls[0][1] as Map; + expect(patchCall.get('cli-proxy')).toBe('172.30.0.50'); }); }); }); diff --git a/src/cli-workflow.ts b/src/cli-workflow.ts index ac42be7b8..d14f5be40 100644 --- a/src/cli-workflow.ts +++ b/src/cli-workflow.ts @@ -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. @@ -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 " 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) { diff --git a/src/container-lifecycle.ts b/src/container-lifecycle.ts index 51ef1d286..3ea0c22ea 100644 --- a/src/container-lifecycle.ts +++ b/src/container-lifecycle.ts @@ -15,6 +15,7 @@ import { handleHealthcheckError, logContainerLogsToStderr, reportBlockedDomains, + detectDnsResolutionFailure, } from './container-startup-diagnostics'; import { checkSquidLogs } from './squid-log-reader'; @@ -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 ) 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 { @@ -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. @@ -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); diff --git a/src/container-startup-diagnostics-coverage.test.ts b/src/container-startup-diagnostics-coverage.test.ts index 1f615bc69..29699638c 100644 --- a/src/container-startup-diagnostics-coverage.test.ts +++ b/src/container-startup-diagnostics-coverage.test.ts @@ -19,6 +19,7 @@ import { logContainerLogsToStderr, handleHealthcheckError, reportBlockedDomains, + detectDnsResolutionFailure, } from './container-startup-diagnostics'; import { logger } from './logger'; import { checkSquidLogs } from './squid-log-reader'; @@ -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', () => { diff --git a/src/container-startup-diagnostics.ts b/src/container-startup-diagnostics.ts index 1d3b772a8..42bd80eb6 100644 --- a/src/container-startup-diagnostics.ts +++ b/src/container-startup-diagnostics.ts @@ -76,6 +76,36 @@ export async function logContainerLogsToStderr(containerName: string): Promise` (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 { + 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 diff --git a/src/services/agent-service-build.test.ts b/src/services/agent-service-build.test.ts index aea10aed2..b732050c5 100644 --- a/src/services/agent-service-build.test.ts +++ b/src/services/agent-service-build.test.ts @@ -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, diff --git a/src/services/agent-service.ts b/src/services/agent-service.ts index 1c7c2ae02..b8c1a823d 100644 --- a/src/services/agent-service.ts +++ b/src/services/agent-service.ts @@ -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'; @@ -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'); } } diff --git a/src/services/internal-service-hosts.test.ts b/src/services/internal-service-hosts.test.ts new file mode 100644 index 000000000..5c23b2d56 --- /dev/null +++ b/src/services/internal-service-hosts.test.ts @@ -0,0 +1,35 @@ +import { buildInternalServiceHosts } from './internal-service-hosts'; + +describe('buildInternalServiceHosts', () => { + it('always includes squid-proxy', () => { + expect(buildInternalServiceHosts({ squidIp: '172.30.0.10' })).toEqual({ + 'squid-proxy': '172.30.0.10', + }); + }); + + it('includes api-proxy and cli-proxy only when their IPs are provided', () => { + expect( + buildInternalServiceHosts({ + squidIp: '172.30.0.10', + apiProxyIp: '172.30.0.30', + cliProxyIp: '172.30.0.50', + }), + ).toEqual({ + 'squid-proxy': '172.30.0.10', + 'api-proxy': '172.30.0.30', + 'cli-proxy': '172.30.0.50', + }); + }); + + it('omits sidecars whose IPs are undefined', () => { + const hosts = buildInternalServiceHosts({ + squidIp: '172.30.0.10', + cliProxyIp: '172.30.0.50', + }); + expect(hosts).toEqual({ + 'squid-proxy': '172.30.0.10', + 'cli-proxy': '172.30.0.50', + }); + expect(hosts['api-proxy']).toBeUndefined(); + }); +}); diff --git a/src/services/internal-service-hosts.ts b/src/services/internal-service-hosts.ts new file mode 100644 index 000000000..490cd1898 --- /dev/null +++ b/src/services/internal-service-hosts.ts @@ -0,0 +1,33 @@ +/** + * Fixed IPs of the compose-internal proxy sidecars the agent (and topology + * peers) may need to resolve by hostname. + */ +export interface InternalServiceIps { + squidIp: string; + apiProxyIp?: string; + cliProxyIp?: string; +} + +/** + * Builds the ` -> ` host entries for the compose-internal + * proxy sidecars, including only the sidecars that are actually enabled (their + * IP is defined). + * + * These hostnames normally resolve via Docker's embedded DNS (127.0.0.11), but + * that resolver is unreachable from gVisor's userspace netstack and on ARC/DinD + * runners (where the Docker-in-Docker network doesn't forward lookups to the + * Kubernetes resolver). Since every sidecar has a fixed IP known at startup, + * both the gVisor path (compose `extra_hosts`) and the network-isolation / + * ARC/DinD path (runtime `/etc/hosts` patch) pre-register the same entries via + * this single source of truth. + */ +export function buildInternalServiceHosts(ips: InternalServiceIps): Record { + const hosts: Record = { 'squid-proxy': ips.squidIp }; + if (ips.apiProxyIp) { + hosts['api-proxy'] = ips.apiProxyIp; + } + if (ips.cliProxyIp) { + hosts['cli-proxy'] = ips.cliProxyIp; + } + return hosts; +}