Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds fabric-backed DNS resolution: new FabricClient RPC methods (ResolveCNAME/ResolveIPAddr/ResolveNS), GrpcClient wrappers, a FabricResolver that issues those RPCs, wiring to pass the fabric client into BYOC/providers and cert flows, and tests updating resolver injection and behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant ResolverAt as ResolverFactory
participant FabricRes as FabricResolver
participant Grpc as GrpcClient
participant Service as Fabric gRPC Service
CLI->>ResolverAt: resolver := ResolverAt(nsServer) %% choose resolver for NSServer
ResolverAt->>FabricRes: use FabricResolver(Client) %% if fabric configured
CLI->>FabricRes: LookupIPAddr(ctx, domain)
FabricRes->>Grpc: ResolveIPAddr(ctx, ResolveIPAddrRequest)
Grpc->>Service: ResolveIPAddr RPC
Service-->>Grpc: ResolveIPAddrResponse
Grpc-->>FabricRes: response / error
FabricRes->>CLI: []*net.IPAddr or ErrNoSuchHost / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/pkg/dns/fabric_test.go (2)
75-95: Consider adding error propagation test for CNAME.
TestFabricResolverLookupIPAddrincludes a subtest for RPC error propagation, butTestFabricResolverLookupCNAMEdoes not. For consistency and completeness:🧪 Optional: Add error propagation test
t.Run("propagates RPC error", func(t *testing.T) { boom := errors.New("rpc boom") m := &mockFabricClient{cnameErr: boom} r := FabricResolver{Client: m} if _, err := r.LookupCNAME(t.Context(), "example.com"); err != boom { t.Errorf("expected rpc error, got %v", err) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/dns/fabric_test.go` around lines 75 - 95, Add a subtest to TestFabricResolverLookupCNAME that verifies RPC errors from the mock are propagated: create an error (e.g., boom := errors.New("rpc boom")), set mockFabricClient{cnameErr: boom}, construct FabricResolver{Client: m}, call LookupCNAME with a context and domain, and assert the returned error is the same boom (not wrapped or swallowed). This mirrors the RPC error propagation test pattern used in TestFabricResolverLookupIPAddr and ensures LookupCNAME correctly returns the mock's cnameErr.
97-107: Consider using subtests forTestFabricResolverLookupNS.The other tests use
t.Runsubtests for different scenarios. For consistency and to test error propagation:🧪 Optional: Refactor to subtests
func TestFabricResolverLookupNS(t *testing.T) { t.Run("returns NS records", func(t *testing.T) { m := &mockFabricClient{nsResp: &defangv1.ResolveNSResponse{Hosts: []string{"ns1.example.com.", "ns2.example.com."}}} r := FabricResolver{Client: m} ns, err := r.LookupNS(t.Context(), "example.com") if err != nil { t.Fatalf("unexpected error: %v", err) } if len(ns) != 2 || ns[0].Host != "ns1.example.com." { t.Errorf("unexpected NS result: %+v", ns) } }) t.Run("propagates RPC error", func(t *testing.T) { boom := errors.New("rpc boom") m := &mockFabricClient{nsErr: boom} r := FabricResolver{Client: m} if _, err := r.LookupNS(t.Context(), "example.com"); err != boom { t.Errorf("expected rpc error, got %v", err) } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/dns/fabric_test.go` around lines 97 - 107, Convert TestFabricResolverLookupNS into a table-driven or t.Run style with at least two subtests: one "returns NS records" that uses mockFabricClient with nsResp (hosts "ns1.example.com.", "ns2.example.com.") and asserts successful LookupNS via FabricResolver, and one "propagates RPC error" that uses mockFabricClient with nsErr set to a sentinel error and asserts LookupNS returns that error; update assertions to run inside the respective t.Run closures and reference mockFabricClient, nsResp, nsErr and FabricResolver.LookupNS to locate the code to change.src/pkg/dns/resolver.go (1)
98-111: Consider returningErrNoSuchHostfor empty NS response.
LookupIPAddrandLookupCNAMEreturnErrNoSuchHostwhen the response is empty, butLookupNSreturns an empty slice. For consistency, consider the same treatment:♻️ Optional consistency fix
func (r FabricResolver) LookupNS(ctx context.Context, domain string) ([]*net.NS, error) { resp, err := r.Client.ResolveNS(ctx, &defangv1.ResolveNSRequest{ Domain: domain, NsServer: r.NSServer, }) if err != nil { return nil, err } + if len(resp.Hosts) == 0 { + return nil, ErrNoSuchHost + } nss := make([]*net.NS, 0, len(resp.Hosts)) for _, h := range resp.Hosts { nss = append(nss, &net.NS{Host: h}) } return nss, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/dns/resolver.go` around lines 98 - 111, LookupNS currently returns an empty slice when resp.Hosts is empty, which is inconsistent with LookupIPAddr and LookupCNAME that return ErrNoSuchHost; update FabricResolver.LookupNS to check len(resp.Hosts)==0 and return nil, ErrNoSuchHost (the same error used by LookupIPAddr/LookupCNAME) instead of an empty slice, so callers receive a consistent "no such host" error for missing NS records.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/cli/connect.go`:
- Around line 21-23: The global ResolverAt is being written by
dns.UseFabricResolver(grpcClient) in Connect while reads occur unprotected
(e.g., getCNAMEInSync, getIPInSync, resolver.go uses ResolverAt), causing a data
race; fix by making ResolverAt access concurrency-safe: either (A) protect reads
with fabricMu.RLock()/RUnlock() via a helper like getResolverAt() analogous to
getFabricClient(), and wrap dns.UseFabricResolver(grpcClient) with
fabricMu.Lock()/Unlock() as already done, or (B) replace ResolverAt with an
atomic.Value holding the resolver function and update it in UseFabricResolver
while readers load it atomically; pick one approach, update all reads
(getCNAMEInSync, getIPInSync, resolver.go usages) to use the safe helper or
atomic load, and ensure Connect no longer performs unsynchronized writes to
ResolverAt during active lookups.
---
Nitpick comments:
In `@src/pkg/dns/fabric_test.go`:
- Around line 75-95: Add a subtest to TestFabricResolverLookupCNAME that
verifies RPC errors from the mock are propagated: create an error (e.g., boom :=
errors.New("rpc boom")), set mockFabricClient{cnameErr: boom}, construct
FabricResolver{Client: m}, call LookupCNAME with a context and domain, and
assert the returned error is the same boom (not wrapped or swallowed). This
mirrors the RPC error propagation test pattern used in
TestFabricResolverLookupIPAddr and ensures LookupCNAME correctly returns the
mock's cnameErr.
- Around line 97-107: Convert TestFabricResolverLookupNS into a table-driven or
t.Run style with at least two subtests: one "returns NS records" that uses
mockFabricClient with nsResp (hosts "ns1.example.com.", "ns2.example.com.") and
asserts successful LookupNS via FabricResolver, and one "propagates RPC error"
that uses mockFabricClient with nsErr set to a sentinel error and asserts
LookupNS returns that error; update assertions to run inside the respective
t.Run closures and reference mockFabricClient, nsResp, nsErr and
FabricResolver.LookupNS to locate the code to change.
In `@src/pkg/dns/resolver.go`:
- Around line 98-111: LookupNS currently returns an empty slice when resp.Hosts
is empty, which is inconsistent with LookupIPAddr and LookupCNAME that return
ErrNoSuchHost; update FabricResolver.LookupNS to check len(resp.Hosts)==0 and
return nil, ErrNoSuchHost (the same error used by LookupIPAddr/LookupCNAME)
instead of an empty slice, so callers receive a consistent "no such host" error
for missing NS records.
🪄 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
Run ID: d7b6b16b-82f2-433a-9256-2a5fae19f893
📒 Files selected for processing (5)
src/pkg/cli/client/client.gosrc/pkg/cli/client/grpc.gosrc/pkg/cli/connect.gosrc/pkg/dns/fabric_test.gosrc/pkg/dns/resolver.go
edwardrf
force-pushed
the
edw/server-dns-resolver-impl
branch
from
0d09460 to
b4ce078
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/pkg/dns/resolver.go (2)
71-80: Consider logging or handling invalid IP addresses from the server.If
net.ParseIP(s)returnsnil, the invalid IP is silently dropped. While returningErrNoSuchHostwhen all IPs are invalid is correct, consider logging a warning for debugging when individual IPs fail to parse—this could help diagnose server-side issues.♻️ Optional: log invalid IPs
+import "log/slog" + func (r FabricResolver) LookupIPAddr(ctx context.Context, domain string) ([]net.IPAddr, error) { resp, err := r.Client.ResolveIPAddr(ctx, &defangv1.ResolveIPAddrRequest{ Domain: domain, NsServer: r.NSServer, }) if err != nil { return nil, err } ips := make([]net.IPAddr, 0, len(resp.IpAddrs)) for _, s := range resp.IpAddrs { if ip := net.ParseIP(s); ip != nil { ips = append(ips, net.IPAddr{IP: ip}) + } else { + slog.Debug("invalid IP address from fabric resolver", "ip", s, "domain", domain) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/dns/resolver.go` around lines 71 - 80, When building the ips slice from resp.IpAddrs in resolver.go, add a warning/log entry whenever net.ParseIP(s) returns nil so invalid IP strings are visible in logs; keep appending valid addresses to ips as before, and preserve the ErrNoSuchHost return when ips ends up empty. Update the block that iterates over resp.IpAddrs (the loop building ips) to emit a concise warning including the invalid string (and context like the host or response ID if available) using the package logger used elsewhere in this file, without changing the final behavior of returning ips or ErrNoSuchHost.
97-110: Inconsistent empty-result handling:LookupNSreturns empty slice instead ofErrNoSuchHost.
LookupIPAddrandLookupCNAMEreturnErrNoSuchHostwhen no records are found, butLookupNSreturns an empty slice. If this is intentional (e.g., empty NS is semantically valid), consider adding a brief comment to document the design decision.📝 Option A: Match other methods (return error)
func (r FabricResolver) LookupNS(ctx context.Context, domain string) ([]*net.NS, error) { resp, err := r.Client.ResolveNS(ctx, &defangv1.ResolveNSRequest{ Domain: domain, NsServer: r.NSServer, }) if err != nil { return nil, err } + if len(resp.Hosts) == 0 { + return nil, ErrNoSuchHost + } nss := make([]*net.NS, 0, len(resp.Hosts))📝 Option B: Document the intentional difference
func (r FabricResolver) LookupNS(ctx context.Context, domain string) ([]*net.NS, error) { + // Note: Unlike LookupIPAddr/LookupCNAME, an empty result is valid for NS + // lookups and does not return ErrNoSuchHost. resp, err := r.Client.ResolveNS(ctx, &defangv1.ResolveNSRequest{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/dns/resolver.go` around lines 97 - 110, LookupNS currently returns an empty slice when resp.Hosts is empty, which is inconsistent with LookupIPAddr and LookupCNAME; update the LookupNS method to check if len(resp.Hosts) == 0 and return nil, net.ErrNoSuchHost (or add a brief comment if the empty-slice behavior is intentional) so behavior matches the other resolvers; locate the LookupNS function and add the empty-result check before building nss, referencing net.ErrNoSuchHost to signal no records found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pkg/dns/resolver.go`:
- Around line 71-80: When building the ips slice from resp.IpAddrs in
resolver.go, add a warning/log entry whenever net.ParseIP(s) returns nil so
invalid IP strings are visible in logs; keep appending valid addresses to ips as
before, and preserve the ErrNoSuchHost return when ips ends up empty. Update the
block that iterates over resp.IpAddrs (the loop building ips) to emit a concise
warning including the invalid string (and context like the host or response ID
if available) using the package logger used elsewhere in this file, without
changing the final behavior of returning ips or ErrNoSuchHost.
- Around line 97-110: LookupNS currently returns an empty slice when resp.Hosts
is empty, which is inconsistent with LookupIPAddr and LookupCNAME; update the
LookupNS method to check if len(resp.Hosts) == 0 and return nil,
net.ErrNoSuchHost (or add a brief comment if the empty-slice behavior is
intentional) so behavior matches the other resolvers; locate the LookupNS
function and add the empty-result check before building nss, referencing
net.ErrNoSuchHost to signal no records found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51dc1001-7b9f-4b2f-a4fb-8603a743fade
📒 Files selected for processing (7)
src/pkg/cli/client/client.gosrc/pkg/cli/client/grpc.gosrc/pkg/cli/connect.gosrc/pkg/dns/check_test.gosrc/pkg/dns/fabric_test.gosrc/pkg/dns/resolver.gosrc/pkg/dns/resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pkg/cli/connect.go
- src/pkg/cli/client/grpc.go
- src/pkg/dns/fabric_test.go
| fabricMu.Lock() | ||
| defer fabricMu.Unlock() | ||
| fabricClient = c | ||
| resolverAt = func(nsServer string) Resolver { |
Member
There was a problem hiding this comment.
Swapping this global like this makes me really uncomfortable, what do you think about refactoring this to use dependency injection instead? I took a stab at this in #2078
…ons (#2078) * refactor(aws): carry FabricResolverClient in ByocAws for explicit DNS routing PrepareDomainDelegation now builds a FabricResolver directly from the stored fabricClient instead of reading the global dns.ResolverAt. This makes the NS conflict check's DNS path explicit and traceable without relying on UseFabricResolver having been called first. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(dns): thread resolverAt explicitly through check.go and FindNSServers CheckDomainDNSReady, getCNAMEInSync, getIPInSync, and FindNSServers now accept an explicit resolverAt parameter instead of reading the global. cert.go's waitForCNAME builds a FabricResolver-backed factory from its FabricClient argument and passes it through. MockFabricClient gains stub implementations for the three Resolve RPCs to prevent nil-interface panics when used in tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(dns): remove global resolver state; add ResolverAt field to RootResolver UseFabricResolver, getFabricClient, fabricMu, fabricClient, and the package-level resolverAt var are all deleted. RootResolver gains an explicit ResolverAt field (nil falls back to DirectResolverAt), and FindNSServers takes a resolverAt parameter so callers control which resolver is used when walking the NS delegation chain. connect.go no longer calls UseFabricResolver. fabric_test.go drops the tests for the now-deleted functions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(dns): restore fabric DNS routing for all lookup paths ALB/CNAME IP lookups in CheckDomainDNSReady, the HTTP client used for cert generation, and CheckTLSCert were still using direct DNS after the DI refactor. Thread the fabric resolver through all three paths so no lookup silently falls back to direct UDP DNS. - DirectResolverAt("") now returns RootResolver{} as the "root" fallback, enabling resolverAt("") as a uniform way to request recursive resolution regardless of whether fabric or direct DNS is in use - CheckDomainDNSReady uses resolverAt("") for ALB/CNAME IP lookups instead of a package-level RootResolver{} - cert.CheckTLSCert accepts a dns.Resolver parameter - cli/cert: replace global resolver/httpClient vars with newCertHTTPClient(r dns.Resolver); thread resolver through generateCert → triggerCertGeneration / waitForTLS / CheckTLSCert Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pkg/dns/fabric_test.go (1)
39-107: ⚡ Quick winPrefer a table-driven harness for the new
FabricResolversuite.These cases repeat the same setup/assertion pattern across IP, CNAME, and NS lookups. Converting the new file to a table-driven structure will make it much easier to extend the resolver contract with more record/error combinations.
As per coding guidelines "Use table-driven tests for comprehensive test coverage in unit tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/dns/fabric_test.go` around lines 39 - 107, Tests for FabricResolver repeat similar setup/assertions; convert them into table-driven tests by defining a slice of test cases containing fields like name, method ("LookupIPAddr"/"LookupCNAME"/"LookupNS"), mockFabricClient fields (ipResp/ipErr, cnameResp, nsResp), input domain, expected result or expected error (e.g., ErrNoSuchHost) and any expected request checks; then iterate with t.Run and for each case construct FabricResolver{Client: &mockFabricClient{...}, NSServer: ... if needed}, call the appropriate method (LookupIPAddr/LookupCNAME/LookupNS) based on the method field, and perform unified assertions for returned value, error handling, and mockFabricClient.last* request contents; keep existing behavior for parsing IPs (valid/invalid), error propagation (ipErr), and empty-result => ErrNoSuchHost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/dns/check.go`:
- Around line 27-31: The function CheckDomainDNSReady currently dereferences
resolverAt and will panic if callers pass nil; change it to default a nil
resolverAt to DirectResolverAt at the start (e.g., if resolverAt == nil {
resolverAt = DirectResolverAt }) before any calls to getCNAMEInSync or other
lookups, and apply the same nil-to-DirectResolverAt defaulting in the other
function(s) in the same file that accept resolverAt (the block referenced at
lines 44-68) so all resolverAt usages mirror RootResolver.resolverFn() behavior.
In `@src/pkg/dns/resolver.go`:
- Around line 86-99: The final authoritative lookup still uses DirectResolver
instead of honoring the injected ResolverAt; update the lookup logic in
functions that perform the terminal A/CNAME/NS query to call
RootResolver.resolverFn() (or r.resolverFn()) rather than directly
constructing/using DirectResolver so the injected resolver is used for both NS
discovery and the final authoritative query; locate the code paths referenced
around resolverFn(), resolverAt, and the final lookup (the block that currently
instantiates or calls DirectResolver for the terminal query) and replace that
usage with r.resolverFn()(host) (or equivalent) so the injected resolver is
consistently applied.
---
Nitpick comments:
In `@src/pkg/dns/fabric_test.go`:
- Around line 39-107: Tests for FabricResolver repeat similar setup/assertions;
convert them into table-driven tests by defining a slice of test cases
containing fields like name, method ("LookupIPAddr"/"LookupCNAME"/"LookupNS"),
mockFabricClient fields (ipResp/ipErr, cnameResp, nsResp), input domain,
expected result or expected error (e.g., ErrNoSuchHost) and any expected request
checks; then iterate with t.Run and for each case construct
FabricResolver{Client: &mockFabricClient{...}, NSServer: ... if needed}, call
the appropriate method (LookupIPAddr/LookupCNAME/LookupNS) based on the method
field, and perform unified assertions for returned value, error handling, and
mockFabricClient.last* request contents; keep existing behavior for parsing IPs
(valid/invalid), error propagation (ipErr), and empty-result => ErrNoSuchHost.
🪄 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
Run ID: f1f88201-8314-4203-96a7-dbac6bdb417f
📒 Files selected for processing (13)
src/cmd/cli/command/cd.gosrc/pkg/cert/check.gosrc/pkg/cli/cert.gosrc/pkg/cli/cert_test.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/mock.gosrc/pkg/cli/connect.gosrc/pkg/dns/check.gosrc/pkg/dns/check_test.gosrc/pkg/dns/fabric_test.gosrc/pkg/dns/resolver.gosrc/pkg/dns/resolver_test.go
✅ Files skipped from review due to trivial changes (2)
- src/pkg/cli/client/byoc/aws/byoc_test.go
- src/pkg/dns/check_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/dns/resolver_test.go
Comment on lines
+27
to
+31
| func CheckDomainDNSReady(ctx context.Context, domain string, validCNAMEs []string, resolverAt func(string) Resolver) bool { | ||
| for i, validCNAME := range validCNAMEs { | ||
| validCNAMEs[i] = Normalize(validCNAME) | ||
| } | ||
| cname, err := getCNAMEInSync(ctx, domain) | ||
| cname, err := getCNAMEInSync(ctx, domain, resolverAt) |
Contributor
There was a problem hiding this comment.
Default a nil resolverAt to DirectResolverAt.
This new exported signature panics as soon as a caller passes nil, because the function dereferences resolverAt on the first lookup. Mirroring RootResolver.resolverFn() here would preserve the old default behavior and avoid an unnecessary crash path.
Suggested fix
func CheckDomainDNSReady(ctx context.Context, domain string, validCNAMEs []string, resolverAt func(string) Resolver) bool {
+ if resolverAt == nil {
+ resolverAt = DirectResolverAt
+ }
for i, validCNAME := range validCNAMEs {
validCNAMEs[i] = Normalize(validCNAME)
}Also applies to: 44-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/dns/check.go` around lines 27 - 31, The function CheckDomainDNSReady
currently dereferences resolverAt and will panic if callers pass nil; change it
to default a nil resolverAt to DirectResolverAt at the start (e.g., if
resolverAt == nil { resolverAt = DirectResolverAt }) before any calls to
getCNAMEInSync or other lookups, and apply the same nil-to-DirectResolverAt
defaulting in the other function(s) in the same file that accept resolverAt (the
block referenced at lines 44-68) so all resolverAt usages mirror
RootResolver.resolverFn() behavior.
Comment on lines
+86
to
+99
| // RootResolver performs recursive DNS resolution starting from the root | ||
| // nameservers. Set ResolverAt to override how individual nameservers are | ||
| // queried (e.g. to route through the Fabric gRPC API). A nil ResolverAt | ||
| // falls back to DirectResolverAt. | ||
| type RootResolver struct { | ||
| ResolverAt func(string) Resolver | ||
| } | ||
|
|
||
| func (r RootResolver) resolverFn() func(string) Resolver { | ||
| if r.ResolverAt != nil { | ||
| return r.ResolverAt | ||
| } | ||
| return DirectResolverAt | ||
| } |
Contributor
There was a problem hiding this comment.
Honor ResolverAt for the final authoritative lookup.
Line 147 still hardcodes DirectResolver, so RootResolver{ResolverAt: fabricResolverAt} only uses the injected resolver for NS discovery and then sends the actual A/CNAME/NS lookup over raw UDP/53. That reintroduces the intercepted-DNS failure mode this PR is trying to eliminate for any caller that relies on RootResolver injection.
Suggested fix
func (r RootResolver) getResolver(ctx context.Context, domain string) Resolver {
ns, err := FindNSServers(ctx, domain, r.resolverFn())
if err != nil {
return DirectResolver{}
}
- return DirectResolver{NSServer: ns[pkg.RandomIndex(len(ns))].Host}
+ return r.resolverFn()(ns[pkg.RandomIndex(len(ns))].Host)
}Also applies to: 142-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pkg/dns/resolver.go` around lines 86 - 99, The final authoritative lookup
still uses DirectResolver instead of honoring the injected ResolverAt; update
the lookup logic in functions that perform the terminal A/CNAME/NS query to call
RootResolver.resolverFn() (or r.resolverFn()) rather than directly
constructing/using DirectResolver so the injected resolver is used for both NS
discovery and the final authoritative query; locate the code paths referenced
around resolverFn(), resolverAt, and the final lookup (the block that currently
instantiates or calls DirectResolver for the terminal query) and replace that
usage with r.resolverFn()(host) (or equivalent) so the injected resolver is
consistently applied.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Use fabric dns client for dns resolves
Linked Issues
Fixes: #1955
Checklist
Summary by CodeRabbit
New Features
Tests