-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix answers for alt domain #11348
Fix answers for alt domain #11348
Conversation
Hello @jkirschner-hashicorp @dhiaayachi, Here is a new PR. Could you please take a look? It should be covering all use-cases. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
} | ||
return domain | ||
} | ||
|
||
// handlePtr is used to handle "reverse" DNS queries | ||
func (d *DNSServer) handlePtr(resp dns.ResponseWriter, req *dns.Msg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkirschner-hashicorp I was thinking about adding it somewhere in the doc, may be as part of the altdomain
description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
toalt_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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkirschner-hashicorp I agree, let's move with the first option and I'll open small PR for docs improvement.
…nd added TXT additional section check
6106104
to
a864333
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed with @dhiaayachi, looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart of a neat pick in the doc this LGTM!
Also missing a change log.
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog looks good. Engineering has approved. Merging shortly. Great work!
🍒 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. |
This pull request is a rewrite of #11274.
It fixes responses for requests made with altDomain.
Closes #10224