[client] Fix stale local records #3776
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with stale local DNS records by updating the local resolver’s record registration and update logic while also renaming several methods to improve consistency across the DNS handling code. Key changes include replacing numeric DNS type constants with symbolic ones, renaming methods (e.g. id() to ID(), stop() to Stop(), probeAvailability() to ProbeAvailability()), and updating the local resolver API to correctly handle record updates.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| dns/dns.go | Replaces numeric DNS type codes with symbolic constants for better clarity |
| client/internal/dns/upstream.go | Renames and updates methods to return types.HandlerID, Stop(), and ProbeAvailability() for consistent API design |
| client/internal/dns/server_test.go | Updates tests to work with the new local resolver API and renamed methods |
| client/internal/dns/server.go | Switches from the old local resolver update pattern to the new Update() method |
| client/internal/dns/local/local.go | Implements a revised local resolver with improved record update logic using maps.Clear |
| client/internal/dns/local/local_test.go | Adds tests to ensure stale records are correctly updated and replaced |
| client/internal/dns/test/mock.go | Provides updated mock implementations for DNS response writer |
Comments suppressed due to low confidence (2)
client/internal/dns/server.go:435
- [nitpick] The variable name 'localRecordsByDomain' may be misleading now that the Update() method expects a slice of SimpleRecord rather than a map. Consider renaming it to better reflect its data structure (e.g. 'localRecordsSlice').
s.localResolver.Update(localRecordsByDomain)
client/internal/dns/local/local.go:113
- Clearing all DNS records with maps.Clear may result in a brief period with no registered records during an update. Confirm that this approach is acceptable in production, especially under concurrent query load, and consider whether a more incremental update strategy is needed.
maps.Clear(d.records)
client/internal/dns/local/local.go
Outdated
| question := r.Question[0] | ||
| question.Name = strings.ToLower(dns.Fqdn(question.Name)) | ||
|
|
||
| log.Tracef("received local question: domain=%s type=%v class=%v", r.Question[0].Name, r.Question[0].Qtype, r.Question[0].Qclass) |
There was a problem hiding this comment.
| log.Tracef("received local question: domain=%s type=%v class=%v", r.Question[0].Name, r.Question[0].Qtype, r.Question[0].Qclass) | |
| log.Tracef("received local question: domain=%s type=%v class=%v", question.Name, question.Qtype, question.Qclass) |
There was a problem hiding this comment.
should we log both r.Question[0].Name and question.Name ?
There was a problem hiding this comment.
| log.Tracef("received local question: domain=%s type=%v class=%v", r.Question[0].Name, r.Question[0].Qtype, r.Question[0].Qclass) | |
| log.Tracef("received local question: domain=%s type=%v class=%v", r.Question[0].Name, question.Qtype, question.Qclass) |
I think this one's better to preserve the original casing
|
Describe your changes
Issue ticket number and link
Stack
Checklist