Fix answers for alt domain

[kbabuadze]

This pull request is a rewrite of #11274.
It fixes responses for requests made with altDomain.

Closes #10224

[kbabuadze]

Hello @jkirschner-hashicorp @dhiaayachi,

Here is a new PR. Could you please take a look? It should be covering all use-cases.
I've added this func to check if alt-domain is configured and request has alt-domain.
https://github.com/kbabuadze/consul/blob/7d5c69f10c1ca9687298c36c25428d9aca8d0853/agent/dns.go#L353-L365

It is case insensitive and returns response respecting the case of request meaning that if request is made as *.cOnSuL. response will have cOnSuL.

Here is a good explanation why it should be like this: https://serverfault.com/a/752295

I noticed that in several places it is explicitly converted to lowercase. But this deserves a separate issue.

[jkirschner-hashicorp]

This is great work! Looking forward to getting this over the finish line. I defer to the Consul engineering team to give a final review, but I did write one comment about something I noticed: I see 2 places where I think we might have missed handling both domain and alt_domain.

[jkirschner-hashicorp]

@kbabuadze, @dhiaayachi : something I noticed based on this community member's question about PTR and alt_domain...

When looking for instances of .domain in this file, I noticed two instances (coincidentally related to PTR) which I'm not sure this PR addresses yet:

• https://github.com/hashicorp/consul/blob/13a818a94c8132ccd3ebc0e85b821f3761c3d024/agent/dns.go#L415
• https://github.com/hashicorp/consul/blob/13a818a94c8132ccd3ebc0e85b821f3761c3d024/agent/dns.go#L443

[kbabuadze]

Hi @jkirschner-hashicorp ,

This is the only use-case which is not handled, and actually one I wanted to ask about.
Since query looks like this: "2.0.0.127.in-addr.arpa." we can return either alt or default domain. (We can not make decision based on incoming request as in other query types)

Probably a correct way is to return .alt-domain, since if somebody configured alt-domain they would expect it to be returned in PTR as well. But I will wait for your suggestions.

[dhiaayachi]

I agree this is trickier to fix then the other instances, we need to know which IP is associated with which domain. I think it's ok to keep it as the main domain and document it as a limitation

[jkirschner-hashicorp]

I agree with @dhiaayachi that we should leave this as-is for now. Since the query itself (in this case) can't specify the domain (it must be <ip>.in-addr.arpa.), we must assume that the primary domain (.domain) should be used, not the alternate (.alt_domain).

document it as a limitation

@dhiaayachi : were you thinking that we add that as a comment in the source code? I don't think we need to open an issue for it (as I don't see a way that could be supported with PTR queries).

[dhiaayachi]

@jkirschner-hashicorp I was thinking about adding it somewhere in the doc, may be as part of the altdomain description

[kbabuadze]

Pushed edit to the doc.

Optionally I can add following section to dns.mdx :

Alternate Domain

If you are using -alt-domain option, Consul will respond based on the presence of it in your queries.
For exmaple if test-domain is configured as an alternative domain the following query:

$ dig @127.0.0.1 -p 8600  consul.service.test-domain SRV

will return:

;; QUESTION SECTION:
;consul.service.test-domain.	IN	SRV

;; ANSWER SECTION:
consul.service.test-domain. 0	IN	SRV	1 1 8300 machine.node.dc1.test-domain.

;; ADDITIONAL SECTION:
machine.node.dc1.test-domain. 0	IN	A	127.0.0.1
machine.node.dc1.test-domain. 0	IN	TXT	"consul-network-segment="

-> Note: Response to <ip>.in-addr.arpa. will always be returned with your default domain, as there is no way to identify queried domain.

[jkirschner-hashicorp]

I like this idea (particularly the example). I'll need to spend a few minutes thinking about the right place for this to go in the linear flow of dns.mdx. Did you have an idea of where it should go?

[kbabuadze]

Hi @jkirschner-hashicorp ,
I'm not sure what would be the best place for it.
Maybe before
https://www.consul.io/docs/discovery/dns#caching,
but not as a sub section of
https://www.consul.io/docs/discovery/dns#service-lookups

[jkirschner-hashicorp]

@kbabuadze : I think that's a good place for it. There are a few minor things I think we can improve first though, such as:

• should have an opening sentence explaining what an alternative domain is / when you might use it with Consul
• spelling fix: "exmaple"
• change -alt-domain to alt_domain (to be consistent with how it's described elsewhere on the page

It will be easiest to discuss those changes further by commenting on a diff in a PR. However, given that everything else is already finished and approved, we have two choices:

• Merge as is, then work with you separately on a small PR to make this docs improvement, OR
• Discuss the above change in this PR

I'm inclined to do the first option (merge as is). What do you think?

[kbabuadze]

@jkirschner-hashicorp I agree, let's move with the first option and I'll open small PR for docs improvement.

[markan]

Reviewed with @dhiaayachi, looks good to me.

[dhiaayachi]

Apart of a neat pick in the doc this LGTM!
Also missing a change log.

[jkirschner-hashicorp]

@kbabuadze : engineering has approved! Other than the nitpick @dhiaayachi mentioned, the last step is to add a changelog entry - see instructions here: https://github.com/hashicorp/consul/blob/c1799923efa7ed028c44704f9686ad57564c0d17/.github/CONTRIBUTING.md#adding-a-changelog-entry

[jkirschner-hashicorp]

Changelog looks good. Engineering has approved. Merging shortly. Great work!

[hc-github-team-consul-core]

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/489699.