Use fabric dns client for dns resolves#2069
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds fabric-backed DNS resolution: new FabricClient Resolve* RPCs, GrpcClient wrappers and mock stubs, a FabricResolver implementation, resolver-factory injection into BYOC and cert flows, CheckDomainDNSReady/resolver refactors, and tests updated to inject resolvers. ChangesFabric DNS integration
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)
ResolverAt->>FabricRes: use FabricResolver(Client)
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.12.1)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." 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
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
lionello
pushed a commit
that referenced
this pull request
May 7, 2026
Mirrors the existing ResolveIPAddr / ResolveCNAME / ResolveNS shape: domain + optional ns_server in the request, repeated strings in the response. NO_SIDE_EFFECTS idempotency. Server-side handler will land in defang-mvp on top of this. CLI client wiring (FabricClient / GrpcClient / MockFabricClient / FabricResolver) will follow in the in-flight #2069 branch once it picks up these regenerated bindings. Co-authored-by: Edward J <edw@defang.io> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
edwardrf
added a commit
that referenced
this pull request
May 7, 2026
* dns: add LookupTXT to Resolver interface
Mirrors LookupIPAddr / LookupCNAME / LookupNS:
- Resolver interface gains LookupTXT(ctx, domain) ([]string, error)
- DirectResolver.LookupTXT issues a TypeTXT query and joins each
record's wire-split strings into one entry per RR
- RootResolver.LookupTXT delegates to its per-domain DirectResolver
- MockResolver.LookupTXT reads from Records[{Type:"TXT"}]
- Test mocks (cli.MockResolver, aws.CallbackMockResolver) add stubs
to keep satisfying the interface
Prereq for the defang-mvp ResolveTXT server handler (which calls
LookupTXT on the resolver) and for the in-flight FabricResolver work
in #2069 (which adds a fabric-backed Resolver impl that will need a
LookupTXT method too).
Standalone from the cert-validation flow on edw/azure-cert-gen — that
branch's cert.go integration uses these primitives but isn't part of
this change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* dns: follow CNAME chains in LookupTXT
A TXT query against a hostname that's actually a CNAME returns only
the CNAME RR — no TXT records — so RootResolver.LookupTXT must follow
the alias, matching how LookupIPAddr handles A/AAAA over CNAME.
DirectResolver.LookupTXT now captures any CNAME from the answer
section and returns ErrCNAMEFound when the result has no TXT records;
RootResolver.LookupTXT loops up to 10 times following each CNAME.
Without this, _dnsauth.<host> -> CNAME -> Azure-managed validation
record would silently 404, since the upstream wouldn't see the TXT
sitting at the CNAME target.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Edward J <edw@defang.io>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…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>
ResolveTXT (added in #2100, server-side handled in defang-mvp#2960) is now part of the FabricResolverClient interface, with a matching FabricResolver.LookupTXT implementation that mirrors LookupIPAddr's shape: empty Txts -> ErrNoSuchHost, RPC errors propagate. FabricClient gains ResolveTXT; GrpcClient implements it via the generated connect client; MockFabricClient returns an empty response. Tests follow the existing pattern in fabric_test.go: - returns_txts_and_forwards_NSServer - empty_Txts_returns_ErrNoSuchHost - propagates_RPC_error Cert-validation TXT polling (Azure managed cert flow) now goes through the fabric resolver chain instead of net.DefaultResolver, sidestepping upstream NXDOMAIN caches that bit us during apex-domain testing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
edwardrf
force-pushed
the
edw/server-dns-resolver-impl
branch
from
b6b6d9a to
a75aa5f
Compare
Two CodeRabbit findings on #2069: 1. CheckDomainDNSReady panicked if a caller passed nil for resolverAt (the func dereferences it on the first lookup). Mirror RootResolver.resolverFn() and default nil to DirectResolverAt. 2. RootResolver.getResolver only used the injected resolverFn for NS discovery, then hard-coded DirectResolver{NSServer: ...} for the actual A/CNAME/NS/TXT query. That meant a FabricResolver-injected caller still went over raw UDP for the lookup, defeating the point of injection. Use resolverFn() for the auth lookup too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai both findings applied in
Build + short tests still green. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Glad the build and tests are green. Thanks for the thorough follow-up! ( ˶ˆᗜˆ˵ ) |
lionello
approved these changes
May 10, 2026
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
4 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