feat: auto-detect host DNS resolvers instead of hardcoding Google DNS#1513
feat: auto-detect host DNS resolvers instead of hardcoding Google DNS#1513
Conversation
Fixes #1512 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (4 files)
✨ New Files (1 files)
Coverage comparison generated by |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Mossaka
left a comment
Mossaka
left a comment
There was a problem hiding this comment.
Review: Auto-detect host DNS resolvers
Overall this is a well-structured change. The new dns-resolver.ts module has clean separation of concerns, good fallback behavior, and solid test coverage. A few items to address:
Issues to address
1. Unused constants with eslint-disable — remove or use them (dns-resolver.ts L7-12)
DOCKER_EMBEDDED_DNS and LOCAL_STUB_RESOLVERS are declared but never referenced, each guarded by an eslint-disable comment. These are documentation-as-code, but they create a maintenance trap: if someone later adds 127.0.0.53 filtering logic, they might not find these constants. Either:
- Remove the constants and keep the information in the
isLoopback()JSDoc, or - Actually use them in the code (e.g.,
LOCAL_STUB_RESOLVERS.some(stub => ip === stub)insideisLoopback)
The isLoopback function already handles 127.0.0.0/8 via startsWith('127.'), so DOCKER_EMBEDDED_DNS and LOCAL_STUB_RESOLVERS are redundant with the implementation.
2. isValidIp is too permissive for IPv6 (dns-resolver.ts L27)
function isValidIp(ip: string): boolean {
return IPV4_REGEX.test(ip) || ip.includes(':');
}ip.includes(':') would accept any string containing a colon, e.g., "not:valid", "http://example.com:80". For a resolv.conf parser this is low-risk since the format is well-defined, but a slightly tighter check like /^[0-9a-fA-F:]+$/ would be safer without being a full IPv6 validator.
3. EXCLUDED_ENV_VARS mutation is a side-effect bug (docker-manager.ts ~L598-602)
This is unrelated to the DNS change but was introduced in this PR:
if (config.excludeEnv && config.excludeEnv.length > 0) {
for (const name of config.excludeEnv) {
EXCLUDED_ENV_VARS.add(name);
}
}If EXCLUDED_ENV_VARS is a module-level Set, mutating it means the exclusions persist across calls within the same process (relevant for tests or if generateDockerCompose is ever called twice). Consider creating a local copy: const excluded = new Set([...EXCLUDED_ENV_VARS, ...(config.excludeEnv ?? [])]).
Minor suggestions (non-blocking)
4. Dynamic import in cli.ts L1614 is unnecessary
const { detectHostDnsServers } = await import('./dns-resolver');dns-resolver is already statically imported by squid-config.ts (which cli.ts imports), so the module is already in the dependency graph. A static import at the top of cli.ts would be cleaner and avoid the dynamic import overhead.
5. Test gap: resolv.conf with mixed loopback and usable servers from first file
There is a test for Docker DNS (127.0.0.11 + real servers), but no test where the first file (/run/systemd/resolve/resolv.conf) contains a mix of loopback + real and the code correctly uses only the real ones from that file (without falling through to the second file). The current Docker DNS test uses mockReturnValue which always returns the same content for both files.
6. Consider capping the number of DNS servers returned
Some resolv.conf files (especially in Kubernetes) can have many nameserver entries. Docker's dns: config and iptables rules are generated per-server, so a large list could create verbose configs. A cap of e.g. 3 (matching glibc's MAXNS) would be a reasonable safeguard.
7. The @deprecated re-export in cli.ts L84-86 is good, but consider removing it in a follow-up
If all internal consumers now import from dns-resolver.ts, the re-export only serves external consumers (if any). If there are none, it can be removed to avoid confusion.
What looks good
- Priority ordering of resolv.conf paths (systemd-resolved upstream first) is exactly right for the common Linux case.
- Fallback to Google DNS when no usable servers are found is a safe default.
getEffectiveDnsServers()cleanly separates "user explicit" from "auto-detect" logic.- The
--dns-servershelp text update ("auto-detected from host if omitted") is clear. - Test coverage for the core parsing/detection logic is solid with 12 tests covering the main paths.
- Consolidating
DEFAULT_DNS_SERVERSto a single source of truth is a good cleanup.
Items 1-3 are worth addressing before merge. The rest are suggestions for follow-up.
There was a problem hiding this comment.
Pull request overview
This PR replaces AWF’s hardcoded default Google DNS configuration with host DNS auto-detection (using /run/systemd/resolve/resolv.conf or /etc/resolv.conf), while preserving --dns-servers as an explicit override and retaining Google DNS only as a fallback.
Changes:
- Added
src/dns-resolver.tsto parse resolv.conf and detect non-loopback host resolvers with a fallback toDEFAULT_DNS_SERVERS. - Updated CLI behavior so omitting
--dns-serverstriggers auto-detection instead of defaulting to Google DNS. - Centralized Google DNS fallback via
DEFAULT_DNS_SERVERSimports across Squid config, docker compose generation, workflow, and host iptables setup.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dns-resolver.ts | Introduces resolv.conf parsing + host DNS detection + fallback constant. |
| src/dns-resolver.test.ts | Adds unit tests for parsing, filtering, fallback, and effective selection. |
| src/cli.ts | Removes commander default for --dns-servers; auto-detects when omitted; re-exports DEFAULT_DNS_SERVERS. |
| src/docker-manager.ts | Uses centralized DEFAULT_DNS_SERVERS when config.dnsServers is unset. |
| src/host-iptables.ts | Uses centralized DEFAULT_DNS_SERVERS when DNS servers aren’t provided. |
| src/squid-config.ts | Uses centralized DEFAULT_DNS_SERVERS for Squid dns_nameservers and policy manifest defaults. |
| src/cli-workflow.ts | Uses centralized DEFAULT_DNS_SERVERS when config.dnsServers is unset. |
Comments suppressed due to low confidence (2)
src/dns-resolver.ts:28
isValidIp()is overly permissive:ip.includes(':')will treat any string containing a colon as a valid “IPv6”, and the IPv4 regex doesn’t enforce octet ranges. If a resolv.conf contains an unexpected token, AWF may generate an invalid docker-composedns:list / Squid config and fail at runtime. Consider using Node’snet.isIP()(and optionally allowing IPv6 zone IDs explicitly) to strictly validate addresses.
const IPV4_REGEX = /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/;
function isValidIp(ip: string): boolean {
return IPV4_REGEX.test(ip) || ip.includes(':');
}
src/dns-resolver.ts:46
parseResolvConf()only matches lines that start exactly withnameserver. Some resolv.conf generators can indent directives, which would cause auto-detection to miss all resolvers and unnecessarily fall back to Google DNS. Consider allowing leading whitespace (e.g., matching^\s*nameserver\s+...).
export function parseResolvConf(content: string): string[] {
const servers: string[] = [];
for (const line of content.split('\n')) {
const match = line.match(/^nameserver\s+(\S+)/);
if (match) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Docker's embedded DNS resolver — always allowed but never used as upstream */ | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const DOCKER_EMBEDDED_DNS = '127.0.0.11'; | ||
|
|
||
| /** Local stub resolvers (systemd-resolved, dnsmasq) that can't be used inside containers */ | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const LOCAL_STUB_RESOLVERS = ['127.0.0.1', '127.0.0.53']; | ||
|
|
There was a problem hiding this comment.
DOCKER_EMBEDDED_DNS and LOCAL_STUB_RESOLVERS are declared but unused, with eslint suppressed. This adds dead code and makes it harder to see what’s actually used for filtering. Either remove these constants (and the eslint disables) or wire them into the filtering logic/tests.
This issue also appears in the following locations of the same file:
- line 24
- line 42
| /** Docker's embedded DNS resolver — always allowed but never used as upstream */ | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| const DOCKER_EMBEDDED_DNS = '127.0.0.11'; | |
| /** Local stub resolvers (systemd-resolved, dnsmasq) that can't be used inside containers */ | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| const LOCAL_STUB_RESOLVERS = ['127.0.0.1', '127.0.0.53']; |
| // Parse and validate DNS servers (auto-detect if not explicitly provided) | ||
| let dnsServers: string[]; | ||
| try { | ||
| dnsServers = parseDnsServers(options.dnsServers); | ||
| } catch (error) { | ||
| logger.error(`Invalid DNS servers: ${error instanceof Error ? error.message : error}`); | ||
| process.exit(1); | ||
| if (options.dnsServers) { | ||
| try { | ||
| dnsServers = parseDnsServers(options.dnsServers); | ||
| } catch (error) { | ||
| logger.error(`Invalid DNS servers: ${error instanceof Error ? error.message : error}`); | ||
| process.exit(1); | ||
| } | ||
| } else { | ||
| const { detectHostDnsServers } = await import('./dns-resolver'); | ||
| dnsServers = detectHostDnsServers(logger); | ||
| } |
There was a problem hiding this comment.
The if (options.dnsServers) check treats an explicitly provided empty string (e.g. --dns-servers "") as “not provided” and silently falls back to host auto-detection instead of erroring. Prefer checking options.dnsServers !== undefined (or reusing getEffectiveDnsServers() so the behavior is centralized and tested).
Smoke Test Results
Overall: PASS
|
🤖 Smoke Test Results✅ GitHub MCP — Last 2 merged PRs: #1508 Overall: PASS | Author:
|
Chroot Version Comparison Results
Result: Tests did not fully pass — Python and Node.js versions differ between host and chroot environments. Go versions match.
|
This comment has been minimized.
This comment has been minimized.
- Remove unused DOCKER_EMBEDDED_DNS and LOCAL_STUB_RESOLVERS constants (and their eslint-disable comments) - Replace hand-rolled isValidIp with Node's net.isIP() for strict IPv4/IPv6 validation - Allow leading whitespace in resolv.conf nameserver lines - Convert dynamic import to static import in cli.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed the review feedback in 88450cd: Issue 1 (unused constants): Removed Issue 2 (isValidIp too permissive): Replaced the hand-rolled regex + Issue 3 (EXCLUDED_ENV_VARS mutation): This is pre-existing code not introduced by this PR — Suggestion 4 (dynamic import): Converted to static import at the top of cli.ts. Additional: Also added tolerance for leading whitespace in resolv.conf |
Smoke Test Results — Copilot Engine
Overall: PASS PR author:
|
|
Smoke Test Results — PASS
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
This comment has been minimized.
This comment has been minimized.
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test ResultsPR titles:
|
Summary
8.8.8.8,8.8.4.4) as the default DNS servers. In enterprise VPCs or air-gapped environments, Google DNS may be unreachable, causing all DNS resolution to fail inside the firewall container./run/systemd/resolve/resolv.conf(systemd upstream config) or/etc/resolv.conf, filtering out loopback stub resolvers (127.0.0.x,::1) that won't work inside containers. Falls back to Google DNS only if no usable resolvers are found.--dns-serversstill works: Explicit--dns-serversflag takes precedence over auto-detection, so users can always override.Changes
src/dns-resolver.ts:parseResolvConf(),detectHostDnsServers(),getEffectiveDnsServers()withDEFAULT_DNS_SERVERSconstantsrc/cli.ts: Removed hardcoded default from--dns-serversoption; auto-detects when flag is omittedsrc/squid-config.ts,src/docker-manager.ts,src/host-iptables.ts,src/cli-workflow.ts: ImportDEFAULT_DNS_SERVERSfromdns-resolver.tsinstead of inline['8.8.8.8', '8.8.4.4']src/dns-resolver.test.ts: 12 unit tests covering parsing, filtering, fallback, and thegetEffectiveDnsServersorchestratorSecurity notes
DEFAULT_DNS_SERVERS(Google DNS) remains the fallback when auto-detection failsTest plan
npm run buildpassesnpm run lintpasses (0 errors)npm testpasses (all 1246 tests, including 12 new)Fixes #1512
🤖 Generated with Claude Code