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.

Sign up for GitHub

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

Jump to bottom

Add DNS recursor strategy option #10611

Merged
merged 7 commits into from
Jul 19, 2021
Merged

Add DNS recursor strategy option #10611

merged 7 commits into from
Jul 19, 2021

Conversation

blake
Copy link
Contributor

@blake blake commented Jul 14, 2021

This change adds a new dns_config.recursor_strategy option which controls how Consul queries DNS resolvers listed in the recursors config option. The supported options are sequential (default), and random.

This is a continuation of #8882 with additional changes to address review feedback from that PR.

Closes #8807

@munali @blake
Add DNS recursor strategy option
cf60e99 
This change adds a new `dns_config.recursor_strategy` option which
controls how Consul queries DNS resolvers listed in the `recursors`
config option. The supported options are `sequential` (default), and
`random`.

Closes #8807

Co-authored-by: Blake Covarrubias <[email protected]>
@blake blake added type/enhancement Proposed improvement or new feature theme/dns Using Consul as a DNS provider, DNS related issues labels Jul 14, 2021
@blake blake self-assigned this Jul 14, 2021
@github-actions github-actions bot added theme/config Relating to Consul Agent configuration, including reloading type/docs Documentation needs to be created/updated/clarified and removed theme/dns Using Consul as a DNS provider, DNS related issues labels Jul 14, 2021
@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.

dnephin
dnephin reviewed Jul 14, 2021
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you for working on getting this ready to merge! I think it's looking good, just a couple suggestions

agent/config/runtime.go Outdated Show resolved Hide resolved
agent/dns.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 15, 2021 08:47 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 15, 2021 08:47 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 15, 2021 08:53 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 15, 2021 08:53 Inactive
dnephin
dnephin reviewed Jul 15, 2021
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Looks good! I think the test failures should be easy to fix, one is failing because the types are different (string vs RecusorStrategy), and the other can be fixed by running go test ./agent/config -update to update the golden file.

I guess we probably also want some test coverage of the new behaviour. I'm not sure what the best way is to handle that yet, but I'd be happy to work with you to figure that one, whenever you have a chance.

@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 18, 2021 01:07 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 18, 2021 01:07 Inactive
@blake blake requested a review from dnephin July 18, 2021 01:32
dnephin
dnephin approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding these tests! Just a few minor suggestions on the test, otherwise I think this is ready to merge.

.changelog/10611.txt Outdated Show resolved Hide resolved
agent/dns_test.go Outdated Show resolved Hide resolved
agent/dns_test.go Outdated Show resolved Hide resolved
agent/dns_test.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 19, 2021 18:37 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 19, 2021 18:37 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging July 19, 2021 21:46 Inactive
@vercel vercel bot temporarily deployed to Preview – consul July 19, 2021 21:46 Inactive
@blake blake merged commit a0cd3dd into main Jul 19, 2021
@blake blake deleted the blake/consul-1.10-add-recursor-strategy branch July 19, 2021 22:22
@hc-github-team-consul-core
Copy link
Collaborator

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

@blake blake mentioned this pull request Jul 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnephin dnephin dnephin approved these changes

Assignees

@blake blake

Labels
theme/config Relating to Consul Agent configuration, including reloading type/docs Documentation needs to be created/updated/clarified type/enhancement Proposed improvement or new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to round robin the queries to upstream DNS servers
5 participants
@blake @hashicorp-ci @hc-github-team-consul-core @dnephin @munali