Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNS SRV fixed in upstream Go, switch to native LookupSRV in Prometheus now? #360

Closed
juliusv opened this Issue Aug 13, 2013 · 16 comments

Comments

Projects
None yet
7 participants
@juliusv

This comment has been minimized.

Copy link
Member Author

juliusv commented Aug 13, 2013

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented May 21, 2015

@fabxc Is that still valid? We use github.com/miekg/dns in retrieval/discovery/dns.go . Is that still needed/intended?

@juliusv

This comment has been minimized.

Copy link
Member Author

juliusv commented May 21, 2015

We would have to re-try with the standard library. The fix back then didn't solve all problems IIRC, but it might be all good nowadays.

@grobie

This comment has been minimized.

Copy link
Member

grobie commented May 21, 2015

What are the reasons to switch?

@juliusv

This comment has been minimized.

Copy link
Member Author

juliusv commented May 21, 2015

@grobie All things being equal, it's always nicer to use the standard library for something (if it works). Less dependencies, often simpler and almost guaranteed to be well-maintained.

@juliusv

This comment has been minimized.

Copy link
Member Author

juliusv commented May 21, 2015

That said, there's no pressure here at all to change this.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented May 21, 2015

Having this in the back of my head for some time. Once this area is a bit
more stable I'll give it another shot.

On Thu, May 21, 2015 at 6:13 PM Julius Volz notifications@github.com
wrote:

That said, there's no pressure here at all to change this.


Reply to this email directly or view it on GitHub
#360 (comment)
.

@grobie

This comment has been minimized.

Copy link
Member

grobie commented May 20, 2016

The golang stdlib DNS resolver still doesn't support EDNS0 and there are no signs they're going to implement it anytime soon. There are DNS recursors which assume EDNS0 to be always supported. I'm in favor of closing this issue.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented May 20, 2016

Makes sense to me, this has been lying around a while with no sign of the Go library being ready for our use yet.

There are DNS recursors which assume EDNS0 to be always supported.

Such recursors are broken.

@discordianfish

This comment has been minimized.

Copy link
Member

discordianfish commented May 20, 2016

👍

@matthiasr

This comment has been minimized.

Copy link
Contributor

matthiasr commented May 20, 2016

👍 to closing.

Such recursors are broken.

True, but not helpful.

@grobie grobie closed this May 20, 2016

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Oct 30, 2016

Reopening because the head of github.com/miekg/dns apparently cannot deal with long DNS SRV requests.

Typical error message:
WARN[0684] resolving srv.eample.net failed: dns: failed to unpack truncated message source=dns.go:187

We are just staying with the vendored version we have, but looking at the commit log of github.com/miekg/dns, there seem to be some worrying bugs including race conditions.

We have to revisit the issue in that light: Investigate standard library capabilities once more on the one hand, and finding out what's wrong with newer versions of github.com/miekg/dns on the other hand.

@discordianfish

This comment has been minimized.

Copy link
Member

discordianfish commented Oct 31, 2016

I assume nobody reported the bug to miekg/dns yet?
@beorn7 Does this happen with all larger SRV queries or only under special circumstances?

This looks like ErrTruncated: https://godoc.org/github.com/miekg/dns#pkg-variables
And according to the miekg/dns examples, you are suppose to check for that error. So I think that the older version of the code returned err = nil when a message is truncated, now it returns err = ErrTruncated. If that's the case, this should fix it: https://github.com/prometheus/prometheus/compare/master...discordianfish:fish/fix-dns-sd?expand=1
@beorn7 Can you test this? If it works, I can also look into adding some unit tests.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Oct 31, 2016

@discordianfish Thanks. That could very well be it. I just had no time to look into details here.

I'll have a look as soon as I'm done with all those higher-priority tasks.

@beorn7 beorn7 self-assigned this Oct 31, 2016

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Nov 3, 2016

Addressed in #2153

@beorn7 beorn7 closed this Nov 3, 2016

simonpasquier pushed a commit to simonpasquier/prometheus that referenced this issue Oct 12, 2017

Merge pull request prometheus#360 from knyar/nginx
Add a link to nginx-lua-prometheus
@lock

This comment has been minimized.

Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.