fix(dns): filter non-portable DNS in network-isolation + Tailscale - #6705
Conversation
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 99.10% | 99.13% | 📈 +0.03% |
| Statements | 98.99% | 99.02% | 📈 +0.03% |
| Functions | 99.14% | 99.14% | ➡️ +0.00% |
| Branches | 95.62% | 95.61% | 📉 -0.01% |
📁 Per-file Coverage Changes (2 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/config-writer.ts |
95.1% → 95.2% (+0.11%) | 95.1% → 95.2% (+0.11%) |
src/log-directory-setup.ts |
96.2% → 100.0% (+3.78%) | 96.3% → 100.0% (+3.71%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
There was a problem hiding this comment.
Pull request overview
Filters host-specific DNS resolvers in network-isolation mode to prevent Tailscale-related Squid DNS failures.
Changes:
- Detects Azure, Tailscale, and link-local DNS addresses.
- Falls back to public DNS when necessary.
- Adds resolver unit tests.
Show a summary per file
| File | Description |
|---|---|
src/dns-resolver.ts |
Adds DNS classification and filtering. |
src/dns-resolver.test.ts |
Tests filtering behavior. |
src/config-writer.ts |
Applies filtering to Squid configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Medium
| const resolvedDnsServers = config.dnsServers ?? DEFAULT_DNS_SERVERS; | ||
| const squidDnsServers = config.networkIsolation | ||
| ? filterForNetworkIsolation(resolvedDnsServers, logger) | ||
| : resolvedDnsServers; |
| const squidDnsServers = config.networkIsolation | ||
| ? filterForNetworkIsolation(resolvedDnsServers, logger) | ||
| : resolvedDnsServers; |
| allowHostPorts: config.allowHostPorts, | ||
| enableDlp: config.enableDlp, | ||
| dnsServers: config.dnsServers, | ||
| dnsServers: squidDnsServers, |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
All three review points are addressed in the latest commit (
|
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
🚀 Security Guard has started processing this pull request |
|
✅ Build Test Suite completed successfully! |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
📰 VERDICT: Smoke Docker Sbx has concluded. All systems operational. This is a developing story. 🎤 |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✅ Smoke Claude passed |
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅ |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
✅ Contribution Check completed successfully! |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed... |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
Smoke Test: Claude Engine Validation
Overall result: PASS ✅
|
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
|
Overall: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
Smoke Test: Services Connectivity
Overall: FAIL —
|
|
Looks good overall, but I think this PR still needs a small docs update for the new network-isolation DNS behavior. The PR description is clear and already references the related issue, and the new tests are in the right place.
|
🔬 Smoke Test Results
Overall: PARTIAL — MCP connectivity verified; pre-computed test data was not injected (workflow template variables unexpanded). cc
|
Smoke Test: Copilot BYOK (Direct) — PR #6705
Running in direct BYOK mode (
|
Smoke Test Results
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
🔭 Smoke Test: API Proxy OpenTelemetry Tracing
All scenarios pass. OTEL tracing integration is fully functional.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Chroot Version Comparison Results
Result: ❌ FAILED — Node.js version mismatch (host: v24.18.0, chroot: v22.23.1).
|
Smoke TestReviewed: feat(api-proxy): use runtime model metadata and pricing overlays Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
🔬 Smoke Test Results — Docker Sbx
Overall:
|
In network-isolation mode + Tailscale, every Squid
CONNECTtoapi.githubcopilot.comfails with503 TCP_TUNNEL:HIER_NONE(server field-:-, indicating DNS never resolved). When Tailscale starts as a later workflow step it modifies the host's policy routing table 52 — if a Tailscale exit node or accepted subnet route covers0.0.0.0/0, Docker bridge traffic onawf-extfollows thefrom all lookup 52rule through the Tailscale tunnel. Azure DHCP DNS (168.63.129.16) is Azure-hypervisor-intercepted and unreachable via Tailscale, so all Squid DNS queries black-hole.Changes
src/dns-resolver.ts— new exportsisNonPortableDns(ip)— identifies DNS servers that depend on host-specific routing paths Tailscale can capture: Azure DHCP DNS (168.63.129.16), Tailscale Magic DNS (100.100.100.100), link-local169.254.x.xfilterForNetworkIsolation(servers, logger)— strips non-portable servers with a warning; falls back to8.8.8.8/8.8.4.4if all detected servers are non-portablesrc/config-writer.ts— apply filter in isolation modeBefore generating
squid.conf, applyfilterForNetworkIsolation()whenconfig.networkIsolationis true:Users requiring specific DNS in isolation mode can still override via
--dns-servers.src/dns-resolver.test.ts15 new unit tests covering portable/non-portable detection, mixed lists, all-non-portable fallback, and RFC1918 corporate DNS passthrough.