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

Add ECS option to EDNS responses where appropriate #4647

Merged
merged 2 commits into from
Sep 11, 2018

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Sep 10, 2018

If ECS opt is present in the request we will mirror it back and return a response with a scope of 0 (global) or with the same prefix length as the request (indicating its valid specifically for that subnet).

We only mirror the prefix-length (non-global) for prepared queries as those could potentially use nearness checks that could be affected by the subnet. In the future we could get more sophisticated with determining the scope bits and allow for better caching of prepared queries that don’t rely on nearness checks but that would require a larger change than we want to do right now.

The other thing this does not do is implement the part of the ECS RFC related to originating ECS headers when acting as a intermediate DNS server (forwarding/recursive resolver). That would take a quite a bit more effort and in general it is expected that if users have sophisticated caching infrastructure that is ECS aware they probably aren’t using Consul to resolve non-consul related information.

If ECS opt is present in the request we will mirror it back and return a response with a scope of 0 (global) or with the same prefix length as the request (indicating its valid specifically for that subnet).

We only mirror the prefix-length (non-global) for prepared queries as those could potentially use nearness checks that could be affected by the subnet. In the future we could get more sophisticated with determining the scope bits and allow for better caching of prepared queries that don’t rely on nearness checks.

The other thing this does not do is implement the part of the ECS RFC related to originating ECS headers when acting as a intermediate DNS server (forwarding/recursive resolver). That would take a quite a bit more effort and in general it is expected that if users have sophisticated caching infrastructure that is ECS aware they probably aren’t using Consul to resolve non-consul related information.
@mkeeler mkeeler requested a review from a team September 10, 2018 21:27
agent/dns.go Outdated
@@ -135,6 +135,37 @@ func (d *DNSServer) ListenAndServe(network, addr string, notif func()) error {
return d.Server.ListenAndServe()
}

func setEDNS(request *dns.Msg, response *dns.Msg, ecsGlobal bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a func comment here. I realize its not exported but even then I try to do it unless the implementation is trivial.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

LGTM based on your rationale and a quick skim of the RFC :)

@mkeeler
Copy link
Member Author

mkeeler commented Sep 11, 2018

Just for clarification this implements the parts of RFC 7871. In particular we implement that parts of the RFC related to authoritative nameservers and partially those relating to forwarding resolvers (with regards to passing along ECS headers to the recursor).

The only relevant bit for our use case we don't do is originate ECS headers for non-ECS requests when querying recursors on the behalf of other clients. This would enable "location" aware results from queried recursors that gets passed along to the clients. Like I mentioned above, if you have sophisticated caching infrastructure that is ECS aware then most likely you are not using Consul recursors and have the main recursor be an actual caching resolver that uses Consul as an upstream. This functionality isn't also mandatory. From my reading of the RFC this isn't strictly necessary, but could be a feature for better recursive resolving for non-ecs aware clients.

@mkeeler mkeeler merged commit d3ee66e into master Sep 11, 2018
@mkeeler mkeeler deleted the feature/edns-client-subnet-response branch October 24, 2018 13:48
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.

3 participants