Skip to content

Add net.LookipIP DNS provider implementation#208

Draft
zserge wants to merge 1 commit into
mainfrom
zserge/memberlist-default-dnsprovider
Draft

Add net.LookipIP DNS provider implementation#208
zserge wants to merge 1 commit into
mainfrom
zserge/memberlist-default-dnsprovider

Conversation

@zserge

@zserge zserge commented Aug 8, 2022

Copy link
Copy Markdown

As we are looking forward to using dskit inside Grafana for HA, we would require an implementation for the kv/memberlist.DNSProvider. Currently it looks like the interface closely matches the API from Thanos, but we would need something lighter, possibly built over net.LookupIP. This PR introduces the "builtin" DNSProvider implementation and a basic test to make sure it works.

@CLAassistant

CLAassistant commented Aug 8, 2022

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@56quarters 56quarters left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good change to me. A few safety related comments.

func (d *dnsProvider) Addresses() []string {
d.Lock()
defer d.Unlock()
return d.addr

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the context be checked for cancellation on each iteration? That seems like it'd make this more responsive.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps just once before doing any DNS lookups?

@pracucci

pracucci commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Hi! 👋 We recently merged #947 which changed the CI configuration for unit tests. To get CI passing on this PR, please rebase on the latest main branch:

git fetch origin main
git rebase origin/main
git push --force-with-lease

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants