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

Avoid to have infinite recursion in DNS lookups when resolving CNAMEs #4918

Merged
merged 8 commits into from
Jan 7, 2019

Conversation

pierresouchay
Copy link
Contributor

This will avoid killing Consul when a Service.Address is using CNAME
to a Consul CNAME that creates an infinite recursion.

This will fix #4907

This will avoid killing Consul when a Service.Address is using CNAME
to a Consul CNAME that creates an infinite recursion.

This will fix hashicorp#4907
@pierresouchay
Copy link
Contributor Author

Tests do pass while not updated in github UI

agent/dns.go Outdated Show resolved Hide resolved
@ShimmerGlass
Copy link
Contributor

LGTM

@pierresouchay
Copy link
Contributor Author

This also improve quite a bit tests stability ;)

@pierresouchay
Copy link
Contributor Author

@mkeeler @banks deserves a look since it avoid breaking DNS resolver when loop in services exists

@banks
Copy link
Member

banks commented Nov 12, 2018

Cool thanks, don't have cycles for this immediately although at a glance it looks like a good fix, I'll put it in our backlog of things to look over properly!

@banks banks added this to the Upcoming milestone Nov 12, 2018
@pierresouchay
Copy link
Contributor Author

will also probably fix #4040

@pierresouchay
Copy link
Contributor Author

@pearkes this one will probably allow you closing 2 issues at once and fixes a real bug causing DoS

@pierresouchay
Copy link
Contributor Author

@pearkes @mkeeler do you think you might have a look ?
It avoid breaking the whole Consul DNS when a node does a simple mismatch registering a service causing an infinite recursion as seen in #4907 (for us, it is a big deal as we have very few Consul DNS resolvers per DC, so breaking all of them would happen very quickly in that case)

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

The code looks good to me and I can see that it would fix the problem.

One thing to note is that I don't think it would fix a problem where resolving a CNAME points to another external name which then points back to the original name within Consul. I don't think we can mitigate that scenario in Consul but regardless its good to state the limitations of the fix.

@pierresouchay
Copy link
Contributor Author

@mkeeler you are right, it would not. But resolvers usually have this kind of protection. It however protect Consul itself from crashing

@mkeeler mkeeler merged commit ae7f88f into hashicorp:master Jan 7, 2019
@pierresouchay
Copy link
Contributor Author

@mkeeler thank you very much!

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.

Loop of death when DNS queries a service that was registered wrong
4 participants