Add configuration entry to control including TXT records for node meta in DNS responses

[mkeeler]

Previously doing ANY or SRV queries for nodes/services would tack on extra TXT records (1 for each entry of node metadata). The changes in this PR allow for disabling that behavior and making those TXT records only accessible via requests with a query type of TXT.

Adds configuration like:

{
   "dns_config" : {
      "additional_node_meta_txt" : false
   }
}

This new configuration entry defaults to true to preserve existing behavior.

[mkeeler]

One question I have is whether this configuration name is okay. I cant say I am completely sold on it but figured I would throw it out there and if anyone else has an idea for a better name please let me know.

[kyhavlov]

I wonder if we should default this to false in 1.3.0 and add a note in the docs here about the upcoming change. This feels like kind of a niche thing that most people wouldn't be able to use without custom DNS logic.

Overall though, looks great and I'm wondering what others think about the default 👍

[banks]

@mkeeler looks awesome.

I'll bite on the naming bikeshed:

• a slight issue with the name additional_node_meta_txt is that it's not very clear what it does, what does "additional" mean here (answer it means adding them as additional records in SRV).
• would it be more consistent to name a boolean field enable/disable?

Given those, we could consider something like disable_srv_node_meta_txt: false. (not even sure it's better on the first point but seems marginally so to me 😄 )

But I don't really know if either point matters, I think it needs documentation either way, maybe look in docs and see how out of place a bool without enable/disable prefix is?

[mkeeler]

@kyhavlov This seems like a sane default. Unless you are issuing DNS queries yourself (not using the OS resolver like getaddrinfo on linux) then you wont be able to see any of the data anyways. For those people that do rely on the text records they can just flip the switch and have it be on.

@banks two issues with disable_srv_node_meta_txt. 1) it applies to ANY queries in addition to SRV so the _srv is unneeded there. Secondly it makes it seem like the TXT records are completely unavailable which they are not. You can still do a TXT query and get them. Thats where additional came from. When the are in addition to other RRs in the response.

disable_extra_node_meta_txt has similar issues in that sometimes the TXT RRs were returned in the answer section, so that could be misleading.

[banks]

It seems a little strange to me that ANY would stop returning TXT records altogether even though they are still returned by direct TXT query - that's not what most people expect from ANY. I assumed this was just stopping them from being added to the Additional section as well as the Answer but I think the tests say something different.

So I given that you called it additional... I assumed it would only need to apply to SRV since ANY should return the records as Answer not Additional as far as I understand?

I still think having disable prefix is more consistent with other stuff but it's not a hill I need to die on 😄.

[mkeeler]

The latest update makes it so that the TXT RRs are only filtered out when 1) they would end up in the Additional section of the DNS response and 2) Consul is configured to do that.

Also the configuration parameter is now enable_additional_node_meta_txt and "additional" here is with regards to the additional section of the DNS response.

[banks]

Great!

[mkeeler]

@kyhavlov If we want to set different defaults or update documentation saying we will do that in the future we can do that in a separate PR.

[scalp42]

@mkeeler @banks unfortunately it doesn't appear to be working.

I'll open an issue.