Add net.LookipIP DNS provider implementation #208
Conversation
|
|
| func (d *dnsProvider) Addresses() []string { | ||
| d.Lock() | ||
| defer d.Unlock() | ||
| return d.addr |
There was a problem hiding this comment.
I think you need to make a copy of this otherwise the underlying storage of the slice can still be modified after the caller starts using it (I could be wrong, we do run tests with -race but this code feels like a race condition).
| return err | ||
| } | ||
| for _, ip := range ips { | ||
| d.addr = append(d.addr, ip.String()) |
There was a problem hiding this comment.
Instead of doing an append(d.addr) in a loop, I'd rather see a new slice of addresses constructed and then swapped at the end. That prevents cases where an error is returned and d.addr is only half updated. This would also allow you to avoid holding the lock while doing network operations. Something like:
var addrs []string
for _, a: range addrs {
// do the lookup
addrs = append(addrs, ip.String())
}
d.Lock()
d.addrs = addrs
d.Unlock()
return nil| d.Lock() | ||
| defer d.Unlock() | ||
| for _, a := range addrs { | ||
| ips, err := net.LookupIP(a) |
There was a problem hiding this comment.
Should the context be checked for cancellation on each iteration? That seems like it'd make this more responsive.
There was a problem hiding this comment.
Or perhaps just once before doing any DNS lookups?
As we are looking forward to using
dskitinside Grafana for HA, we would require an implementation for thekv/memberlist.DNSProvider. Currently it looks like the interface closely matches the API from Thanos, but we would need something lighter, possibly built overnet.LookupIP. This PR introduces the "builtin" DNSProvider implementation and a basic test to make sure it works.