Fix IPv6 fake-IP routing for dual-stack DNS#545
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughRefactors fake-IP DNS to dual-stack: adds IPv4/IPv6 range constants and a dedicated ChangesDual-stack fake-IP DNS refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates Radiance’s sing-box DNS and routing configuration so both A and AAAA DNS answers can be represented via fake-IP, preserving domain context for routing decisions in dual-stack scenarios before IPv6-reject fallback behavior applies.
Changes:
- Extend the fake-IP DNS server to dual-stack (IPv4 + IPv6) and route both A/AAAA DNS queries through it.
- Add logic to ensure server-provided DNS overrides still get a dual-stack fake-IP server and a fallback rule appended after server rules.
- Update and add tests covering dual-stack fake-IP server configuration and merge behavior for server DNS overrides.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| vpn/dnsoptions.go | Adds dual-stack fake-IP server/rule helpers and fallback injection for server DNS overrides. |
| vpn/dnsoptions_test.go | Updates DNS options tests to validate dual-stack fake-IP behavior and removal of AAAA suppression. |
| vpn/boxoptions.go | Adjusts DNS merge behavior to preserve server options and append fake-IP fallback; updates related comments. |
| vpn/boxoptions_test.go | Adds coverage ensuring merge preserves server DNS rule ordering while adding fake-IP dual-stack fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
vpn/dnsoptions_test.go (2)
152-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Go doc comments above the new functions.
These new package-level functions are missing doc comments, and the helper contracts are not obvious from the names alone. As per coding guidelines,
**/*.go: Use Go doc comments (// Foo ...) for exported identifiers and any unexported ones with non-obvious contracts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vpn/dnsoptions_test.go` around lines 152 - 197, Add Go doc comments above the new helper/test functions in dnsoptions_test.go, especially TestBuildDNSServers_FakeIPDualStack, TestBuildDNSRules_RoutesAddressQueriesToFakeIP, requireFakeIPServer, and requireDualStackFakeIPServer. Use short descriptive comments that explain each function’s contract so the purpose is clear from the identifier and comment together.Source: Coding guidelines
178-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMatch
fakeIPServerTagwhen selecting the fake-IP server.This helper returns the first
DNSTypeFakeIPentry, so it will start failing against the wrong server as soon as another fake-IP server is added. Filtering by tag here keeps the test aligned with the routing contract asserted below.Suggested change
func requireFakeIPServer(t *testing.T, servers []option.DNSServerOptions) option.DNSServerOptions { t.Helper() idx := slices.IndexFunc(servers, func(server option.DNSServerOptions) bool { - return server.Type == constant.DNSTypeFakeIP + return server.Type == constant.DNSTypeFakeIP && server.Tag == fakeIPServerTag }) require.NotEqual(t, -1, idx, "expected a fake-IP DNS server") return servers[idx] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vpn/dnsoptions_test.go` around lines 178 - 184, The requireFakeIPServer helper currently selects the first DNSTypeFakeIP entry, which can pick the wrong server once multiple fake-IP servers exist. Update the selection logic in requireFakeIPServer to match the fakeIPServerTag on DNSServerOptions so it returns the tagged fake-IP server that the routing tests expect.vpn/boxoptions_test.go (1)
211-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Go doc comment above the new test function.
As per coding guidelines,
**/*.go: Use Go doc comments (// Foo ...) for exported identifiers and any unexported ones with non-obvious contracts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vpn/boxoptions_test.go` at line 211, Add a Go doc comment directly above the new test function TestMergeAndCollectTags_AddsDualStackFakeIPDNS. The issue is that this test has no comment even though it follows a non-obvious contract, so update the declaration in boxoptions_test.go with a brief descriptive Go-style comment that explains what the test covers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vpn/boxoptions_test.go`:
- Around line 229-230: The tests around mergeAndCollectTags only verify slice
lengths, so they can still pass if src.DNS.Servers or src.DNS.Rules are mutated
in place. Update the assertions in boxoptions_test.go to check the original
source values remain unchanged after calling mergeAndCollectTags, using the
existing src.DNS.Servers and src.DNS.Rules setup plus the corresponding
destination result checks, so the “must not mutate source” contract is actually
covered.
---
Nitpick comments:
In `@vpn/boxoptions_test.go`:
- Line 211: Add a Go doc comment directly above the new test function
TestMergeAndCollectTags_AddsDualStackFakeIPDNS. The issue is that this test has
no comment even though it follows a non-obvious contract, so update the
declaration in boxoptions_test.go with a brief descriptive Go-style comment that
explains what the test covers.
In `@vpn/dnsoptions_test.go`:
- Around line 152-197: Add Go doc comments above the new helper/test functions
in dnsoptions_test.go, especially TestBuildDNSServers_FakeIPDualStack,
TestBuildDNSRules_RoutesAddressQueriesToFakeIP, requireFakeIPServer, and
requireDualStackFakeIPServer. Use short descriptive comments that explain each
function’s contract so the purpose is clear from the identifier and comment
together.
- Around line 178-184: The requireFakeIPServer helper currently selects the
first DNSTypeFakeIP entry, which can pick the wrong server once multiple fake-IP
servers exist. Update the selection logic in requireFakeIPServer to match the
fakeIPServerTag on DNSServerOptions so it returns the tagged fake-IP server that
the routing tests expect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ee66b01-b4a1-492b-bcbc-9491d0821075
📒 Files selected for processing (4)
vpn/boxoptions.govpn/boxoptions_test.govpn/dnsoptions.govpn/dnsoptions_test.go
We need to reject IPv6 DNS queries as well. Since the fakeip server doesn't actually do any lookup, sing-box sets the request destination to the fully-qualified domain, so it won't be caught by the IPv6 reject routing rule.
The sing-box options in the config should be treated as the authoritative source. DNS and routing options should be applied as specified, unless the options depend on something the server cannot know; such as whether the client has IPv6 or not. If the fakeip server and rule aren't included, that would be intentional or a bug in lantern-cloud. |
Proposed fix for https://github.com/getlantern/engineering/issues/3649
Routes both A and AAAA queries through dual-stack fake-IP so domain rules can handle IPv6 DNS results before the IPv6 reject fallback.
Server DNS overrides also keep this fake-IP fallback
Summary by CodeRabbit