Allow DNS interface to use agent cache

[ShimmerGlass]

Adds to new configuration parameters "dns_config.use_cache" and
"dns_config.cache_max_age" controlling how DNS requests use the agent
cache when quering servers.

[pierresouchay]

@banks I told you about this one :-)

[pierresouchay]

@Aestek documentation mainly, but not clear to end user how max_stale VS max_age VS enable cache do work together

[ShimmerGlass]

I will work the documentation so it's clearer what setting corresponds to what. Maybe the Agent caching section could also be moved to its own page since it is not an HTTP API only thing anymore.

[mkeeler]

@Aestek One thing about the current Agent Caching documentation is that most of it is specific to HTTP including all the various headers that can be passed with the requests. It would however be worthwhile to document the agent cache usage on this page: https://www.consul.io/docs/agent/dns.html

[mkeeler]

This is looking good. There are a few questions inline.

One other prerequisite to getting this merged would be for all of the RPCs in use to be cacheable. Presumably the reason you didn't already include it was because the Catalog.NodeServices RPC was not already an implemented cache-type. It should be fairly straight forward implementation that is mostly copy pasted from the existing Catalog.ServiceNodes cache type.

Lastly after my other comment on the PR about all the configurations I found out that the cache will only perform stale queries so max_stale is superceded by using the cache and our docs should reflect that.

[mkeeler]

Probably should add a "[ERR]" to the front of the message.

[mkeeler]

@Aestek & @pierresouchay

I am not sure that the TTL calculations here are what is best/least-surprising.

One thing that initially caught me off guard was that this could potentially set a TTL lower than the remaining cache lifetime of the data.

For example, if you have a service ttl of 30s, a cache max age of 60s and your cached data is 20s old. This will set the TTL to 10s. At first glance you could ask why should a DNS client ask for the same data before the cache would have expired. If the cache hasn't expired and the request is made again it will just use the cached value the same as before. Therefore the TTL could have been something like min(<remaining cache lifetime>, <service ttl - age>) which would have resulted in a 40s TTL.

There are two reasons why that might not be best.

1. Automatic cache refreshing
2. The data might be fetched on behalf of the HTTP API with a different max age or requiring revalidation.

For prepared queries, there is no automatic cache refreshing so that doesn't matter right here.

So the question is whether it is better to have the service and node ttls be a max TTL for those resources or whether we should allow the cache lifetime to increase it. There are advantages and disadvantages to both. What are your thoughts?

[ShimmerGlass]

Are you suggesting max(<remaining cache lifetime>, <service ttl - age>) ? In your example min(<remaining cache lifetime>, <service ttl - age>) would still have resulted in a 20s TTL.
While I agree that its useless to have a TTL lower than the next cache refresh duration it seems weird to me to set a TTL higher than the one configured by the user.
When agent DNS cache the TTL settings can be lowered a lot since the requests would not hit so hard on the servers anymore. In our case we might juste use them for rate limiting with values between 1s and 5s.
Maybe it's best not to change the TTL depending on the cache age at all ?

[mkeeler]

Yeah. Sorry, max(<remaining cache lifetime>, <service ttl - age>

However thinking about it some more it might be best the way you originally did it. It may result in more requests but they should be very easy to respond to from the cache.

[pierresouchay]

@Aestek @mkeeler As an DNS admin, I would like to enjoy the feature without having to handle the complexity of implementation. I have the feeling it should be simpler than that.

My feeling is:

• TTL => stay unchanged
• max-age = max_stale duration ideally (so, I don't care what your guys are doing of kind optimizations)

Thus, enabling cache would simply be:

• dns_config.enable_cache=true

[ShimmerGlass]

• cache entry age no longer affects TTLs
• I've implemented Catalog.NodeServices cached and used in DNS

[mkeeler]

Just a few more changes if you would. In particular I liked how you split out the cache get or RPC for NodeServices into its own function. It would be great if you could do the same for the other similar code.

[ShimmerGlass]

<3

[pierresouchay]

Yeah! We are gonna test this at scale next week in our DCs
CC @Annih

[Annih]

Sounds great :)