Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 / Build and Lint (Node 20)

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 / ESLint

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 / Build and Lint (Node 20)

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 / ESLint

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 / Build and Lint (Node 20)

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 / ESLint

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 && /operation not permitted|permission denied|not permitted|EPERM|EACCES/i.test(stderr);
Comment thread
Copilot marked this conversation as resolved.
Outdated
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
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
Loading