-
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
Merged
jkirschner-hashicorp
merged 9 commits into
hashicorp:main
from
kbabuadze:fix-answers-alt-domain
Oct 29, 2021
Merged
Fix answers for alt domain #11348
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
0864bfd
fixed alt-domain answer for SRV records, and TXT records in additiona…
kbabuadze a828c45
edited TestDNS_AltDomains_Service to test responses for altDomains, a…
kbabuadze ffb00f0
fixed altDomain response for NS type queries, and added test
kbabuadze a7e8c51
fix encodeIPAsFqdn to return alt-domain when requested, added test case
kbabuadze ce85d2e
fix altDomain responses for services where address is IP, added tests
kbabuadze 55599d0
remove spaces
kbabuadze a864333
describe how alt-domain works in docs
kbabuadze ec98e33
fixed configurations options order in dns.mdx
kbabuadze a02daec
added changelog
kbabuadze File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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
).@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
descriptionThere 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:will return:
-> 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:
-alt-domain
toalt_domain
(to be consistent with how it's described elsewhere on the pageIt 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:
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.