Skip to content
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

added Alternative Domain section to dns page in docs #11458

Merged
merged 11 commits into from
Dec 3, 2021

Conversation

kbabuadze
Copy link
Contributor

Hello @jkirschner-hashicorp ,

As promised here is a doc change PR adding alternative domain section to dns.mdx.
Please let me know if there are any change you would like me to make.

Thanks.

@hashicorp-ci
Copy link
Contributor

🤔 This PR has changes in the website/ directory but does not have a type/docs-cherrypick label. If the changes are for the next version, this can be ignored. If they are updates to current docs, attach the label to auto cherrypick to the stable-website branch after merging.

@jkirschner-hashicorp
Copy link
Contributor

jkirschner-hashicorp commented Nov 3, 2021

Thanks for putting this up, @kbabuadze. I'll get back to you by end of week! I'll also check with the docs team to see if they need to take a look, too.

Copy link
Contributor

@eculver eculver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have one small suggestion/typo. Otherwise looks good. Will wait for docs team to confirm.

Co-authored-by: Evan Culver <eculver@users.noreply.github.com>
@jkirschner-hashicorp
Copy link
Contributor

Hi @kbabuadze - sorry for the late response, I had something come up outside of work that caused this to fall off my radar. I'll take a pass at this before tomorrow and then ping the docs team.

Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbabuadze : looks good - thanks for writing this up! I suggested a few small changes. After those are addressed, I'll ping the docs team!

website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
@kbabuadze
Copy link
Contributor Author

kbabuadze commented Nov 21, 2021

Hi @jkirschner-hashicorp,
Thanks for the suggestions. I've committed them!

@jkirschner-hashicorp jkirschner-hashicorp added type/docs Documentation needs to be created/updated/clarified type/docs-cherrypick labels Nov 22, 2021
@jkirschner-hashicorp
Copy link
Contributor

@hashicorp/consul-docs : ready for your review!

Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for integrating the suggested changes. Looks good! Passing to docs team now.

Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some suggestions

Comment on lines 101 to 111
By default, Consul responds to DNS queries only for its configured
[`domain`](/docs/agent/options#domain).

Some use cases require responding to queries for more than one domain,
such as during a DNS migration or to distinguish between internal and
external queries by using different domains.

Consul can be configured to respond to DNS queries on an alternative domain
through the [`alt_domain`](/docs/agent/options#alt_domain) agent configuration
option. Consul's DNS response will use the same domain as was used in the query.
For example, if `test-domain` is configured as the alternative domain, the following query:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
By default, Consul responds to DNS queries only for its configured
[`domain`](/docs/agent/options#domain).
Some use cases require responding to queries for more than one domain,
such as during a DNS migration or to distinguish between internal and
external queries by using different domains.
Consul can be configured to respond to DNS queries on an alternative domain
through the [`alt_domain`](/docs/agent/options#alt_domain) agent configuration
option. Consul's DNS response will use the same domain as was used in the query.
For example, if `test-domain` is configured as the alternative domain, the following query:
By default, Consul responds to DNS queries in the `consul` domain, but you can set a specific domain for responding to DNS queries by configuring the [`domain`](/docs/agent/options#domain) parameter.
You can specify an alternative domain for DNS queries in the `alt_domain` parameter. Configuring the [`alt_domain`](/docs/agent/options#alt_domain) agent parameter enables Consul to respond to DNS queries to the specified domain.
In the following example, the `alt_domain` parameter is set to `test-domain`:
<CodeTabs heading="agent.hcl">
<CodeBlockConfig>
```hcl
alt_domain = "test-domain"
"alt_domain" : "test-domain"

The configuration enables the following test-domain query to run successfully:

I was just trying out some alternate wording to see if I could make it smoother. Please ignore if it doesn't seem like an improvement. 
I also wanted to show an example of the `alt_domain` configuration. This is because it wasn't clear from looking at the entry in the reference whether the option took a Boolean value to enable alt domains or if you have to specify the actual domain as a string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal opinion is that the new wording leaves out some important information (e.g., why someone might want to use an alternative domain, what domain is used in the response). I personally prefer the previous wording.

However, it is problematic if the alt_domain configuration explanation doesn't make its use clear.

@trujillo-adam : Does it make more sense to update the description on the agent configuration option page to make its use more obvious (and perhaps include an example)? Or perhaps provide the configuration example here as you suggested, but also link to this "Alternative Domain" section on the DNS page from the alt_domain agent config option description?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's because I accidentally left a paragraph out that I planned on putting back in at line 102:

"In some instances, Consul may need to respond to queries in more than one domain, such as during a DNS migration or to distinguish between internal and external queries. "

As far as updating the alt_domain reference, yes, we should update it so include at least the data type and default. As far as cross-linking, we should probably think about it a little more. We ca either put 0 guidance in the reference and link internal pages for usage or vice versa. We're currently a little inconsistent. For now I think just adding a little more detail to the reference section is good enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbabuadze - can you add the HCL configuration example @trujillo-adam suggested?

website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbabuadze : two small comments regarding which Consul versions the alternative domain behaviors described apply to. Let me know what you think. Sorry I didn't catch these on my first review!

website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
@kisunji kisunji added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Nov 30, 2021
kbabuadze and others added 2 commits December 2, 2021 22:38
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
kbabuadze and others added 2 commits December 2, 2021 22:39
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
Co-authored-by: trujillo-adam <47586768+trujillo-adam@users.noreply.github.com>
@jkirschner-hashicorp
Copy link
Contributor

jkirschner-hashicorp commented Dec 2, 2021

@kbabuadze: I saw your recent commits. Have you made all the changes you wanted to make? If so, I can give a quick final read and then merge. Thanks!

@github-actions github-actions bot removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Dec 2, 2021
@kbabuadze
Copy link
Contributor Author

Hello @jkirschner-hashicorp,
Sorry for the late reply, I was a bit overloaded.

I've committed your suggestions.
Also I think I went in wrong order and @trujillo-adam 's suggestions got outdated. I've made an attempt to incorporate them. Could you please take a look.

Thanks.

@jkirschner-hashicorp
Copy link
Contributor

Sorry for the late reply, I was a bit overloaded.

Been there :) same thing happened to me when I needed to perform the first review!

I'll take a look tomorrow (I want to load this up locally for a final check rather than just read the markdown).

Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jkirschner-hashicorp
Copy link
Contributor

It looks like @trujillo-adam took a second look before me and agrees it looks good, so I'll go ahead and merge! Thanks for seeing this through, @kbabuadze!

@jkirschner-hashicorp jkirschner-hashicorp merged commit eb90c7f into hashicorp:main Dec 3, 2021
@hc-github-team-consul-core
Copy link
Contributor

🍒 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/516697.

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit eb90c7f onto stable-website succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Dec 3, 2021
added Alternative Domain section to dns page in docs
@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit eb90c7f onto release/1.10.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Dec 3, 2021
added Alternative Domain section to dns page in docs
@jkirschner-hashicorp
Copy link
Contributor

jkirschner-hashicorp commented Dec 3, 2021

Just realized now that this is live that the section was placed above "Service Lookups", but should instead be after it. It doesn't make sense to have "Node Lookups", then "Alternative Domain", then "Service Lookups". I can fix that sometime next week.

That was the original intent in our discussion here: #11348 (comment)

Just didn't remember to check that when looking at the PR markdown...

@kbabuadze
Copy link
Contributor Author

Oh boy, I missed that one too. I can do it if you want, that's just cutting the paragraph and pasting it just before the Caching.
Can I push it here?

@jkirschner-hashicorp
Copy link
Contributor

I think since it's merged already it would have to be a separate branch. You're welcome to do it :)

I was actually going to use this as an excuse to try editing something directly in Github rather than making the changes locally, since as you said it's just a cut/paste that shouldn't require any previewing.

@jkirschner-hashicorp
Copy link
Contributor

@kbabuadze : I put up a PR for it: #11770

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants