dns: add LookupTXT to Resolver interface#2101
Conversation
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>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds DNS TXT lookup support: the ChangesDNS TXT Record Lookup Support
Sequence Diagram(s)sequenceDiagram
participant Client
participant RootResolver
participant DirectResolver
participant DNS
Client->>RootResolver: LookupTXT(ctx, domain)
RootResolver->>DirectResolver: LookupTXT(ctx, domain)
DirectResolver->>DNS: Query TXT
DNS-->>DirectResolver: TXT RRs or CNAME
DirectResolver-->>RootResolver: TXT strings or ErrCNAMEFound/ErrNoSuchHost
RootResolver-->>Client: TXT strings or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pkg/cli/cert_test.go (1)
194-197: ⚡ Quick winPrefer fail-fast behavior for unexpected TXT lookups in this test mock.
If
LookupTXTis not expected in these tests, returningnil, nilcan mask behavior drift. Panicking here makes misuse explicit.Based on learnings In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests.Proposed change
func (mr *MockResolver) LookupTXT(ctx context.Context, domain string) ([]string, error) { mr.calls++ - return nil, nil + panic("unexpected LookupTXT call") }🤖 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 `@src/pkg/cli/cert_test.go` around lines 194 - 197, The mock method MockResolver.LookupTXT should fail fast instead of quietly returning nil,nil; modify LookupTXT (in the MockResolver type) to still increment mr.calls but panic with a clear message (including the unexpected domain) so tests surface unintended TXT lookups immediately rather than masking them.
🤖 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 `@src/pkg/dns/resolver.go`:
- Around line 187-189: The error return in the DNS query block ("if err != nil {
return nil, err }") should be wrapped with context so callers see the queried
name/type; replace the raw return with fmt.Errorf that includes the domain/name
and query type (e.g. fmt.Errorf("dns lookup %s %s: %w", name, qtype, err)) and
ensure fmt is imported; locate the error handling immediately after the DNS
query call in resolver.go and update it to wrap err using %w.
---
Nitpick comments:
In `@src/pkg/cli/cert_test.go`:
- Around line 194-197: The mock method MockResolver.LookupTXT should fail fast
instead of quietly returning nil,nil; modify LookupTXT (in the MockResolver
type) to still increment mr.calls but panic with a clear message (including the
unexpected domain) so tests surface unintended TXT lookups immediately rather
than masking them.
🪄 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: ed5d48af-fd32-4949-a501-7a51a22a19d1
📒 Files selected for processing (4)
src/pkg/cli/cert_test.gosrc/pkg/cli/client/byoc/aws/domain_test.gosrc/pkg/dns/mock.gosrc/pkg/dns/resolver.go
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>
|
@coderabbitai thanks. Applied the first one (CNAME-following on Skipping the second one (wrap |
|
Tip For best results, initiate chat on the files or code changes.
The CNAME-following implementation looks well-reasoned — The consistency argument for skipping error wrapping is solid. Wrapping only ✏️ Learnings added
|
3 participants
Description
Adds
LookupTXT(ctx, domain) ([]string, error)todns.Resolverand implementations:TypeTXTquery, joins each record's wire-split strings into one entry per RRRecords[{Type: \"TXT\"}]cli.MockResolver,aws.CallbackMockResolver) gain matchingLookupTXTmethods to keep satisfying the interfaceWhy
Prereq for two follow-ups:
ResolveTXTserver handler — callsresolver.LookupTXT(ctx, req.Domain), mirroring the existingResolveCNAME/ResolveNSserver impls.Resolverimpl; once this lands, that PR'sFabricResolvergains a matchingLookupTXTthat calls the newResolveTXTRPC (added in proto: add ResolveTXT RPC #2100).The cert-validation flow on
edw/azure-cert-genalready consumes these primitives (Azure managed-cert TXT polling), but itscert.gointegration is intentionally not part of this PR — kept narrow so server-side and client-side teams can land in any order.Linked issues
Builds on #2100 (proto: add ResolveTXT RPC).
Checklist
go test -short ./pkg/dns/ ./pkg/cli/ ./pkg/cli/client/byoc/aws/)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests