Use miekg/dns for SRV lookup

[codesome]

Fixes #1200

EDIT (by @gouthamve): This uses miekg/dns library to do the SRV lookups. We are using this in the grpc codebase as there is a A record fallback when SRV lookups fail.

[tomwilkie]

Looks good so far; lint needs fixing, and there was some mention of gRPC also using LookupSRV for querier -> frontend comms.

[codesome]

Yes, it was this comment by @bboreham. Need to fix that too. Will do when I get free from academic stuff.

[tomwilkie]

I opened grpc/grpc-go#2634 to help with the gRPC side.

[codesome]

Thanks @tomwilkie!

[bboreham]

Note the gRPC PR was rejected

[gouthamve]

Hmm, as far as I see, the grpc resolver uses grpclb port name to resolve the services, https://github.com/grpc/grpc-go/blob/master/naming/dns_resolver.go#L216-L220

If that fails, it falls back to simple A record lookup: https://github.com/grpc/grpc-go/blob/master/naming/dns_resolver.go#L261-L267

I found that in the clusters we run, we don't expose a port-named grpclb and the resolver falls back to A records. Given that it works nevertheless, I wonder if it's worth the pain to copy the dns_resolver.go file from grpc-go. Note that this doesn't exist in the latest kubernetes versions and only is a problem when running cortex with go 1.11 with an old kubernetes version. Even then nothing will break.

[bboreham]

I'm happy with A record lookup.
Can someone update #1200 with exactly what works and doesn't work?
(e.g. what's "an old kubernetes version")

[bboreham]

A. Do we really have to do this? This says the underlying issue can be addressed by upgrading kube-dns.
B. One of the seven commits claims to move from udp to tcp; the PR description says nothing about this. I'd like that addressed; also I'd like to know about performance impact before approving.
C. The description of the problem at #1200 remains imprecise.

[gouthamve]

A: We don't need to do it if we're okay asking people to upgrade kube-dns
B: The TCP-UDP change has been reverted in the last commit by me. The current state of the PR could be described as:

Use miekg/dns instead of native DNS library to be compatible with kube-dns <1.14.11 and golang >=1.11. This is what Prometheus does for DNS-SD

C: The problem is that go1.11's net library cannot read SRV records sent by kube-dns <1.14.11

[tomwilkie]

Thanks for the feedback @bboreham. I'm LGTM on this, keen to unblock the move to go modules and go 1.12.

[bboreham]

Still feels like a bad idea to me.
Note I have two questions outstanding on the go modules PR #1232 for 25 days.

[tomwilkie]

Still feels like a bad idea to me.

What feels bad about it? Can I infer from your comments you'd prefer to upgrade go without this, effectively making Cortex not work on kube-dns <1.14.11?

Note I have two questions outstanding on the go modules PR #1232 for 25 days.

Thats a different PR; lets address that there, it shouldn't block this.

[bboreham]

What feels bad about it?

It's adding 70 lines of code in a client to work around an issue in a server which was fixed months ago.

Are there reports from people who use or would like to use Cortex and cannot upgrade kube-dns?

Also I think there should be comments in the code

• why we are doing this
• why do it in some places and not others
• why you shouldn't call this code very often
• why it's ok to have a relatively inefficient implementation

[gouthamve]

Hmm, I'm going to hold off on this PR until someone actually raises an issue and proceed with Go 1.12 upgrade. We might just be optimising for something that has been fixed already as you point out.

[tomwilkie]

@khaines on the call yesterday we decided we probably didn't need this, but we just want to check you've got an update kubedns?

[tomwilkie]

No word from @khaines. I'll close, we can reopen if needs be.