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

Implements prefix lookups for DNS TTL #4605

Merged

Conversation

pierresouchay
Copy link
Contributor

@pierresouchay pierresouchay commented Aug 29, 2018

This will fix #4509 and allow for instance

lb-* to match services lb-001 or lb-service-007

When not adding * at the end, the existing behavior is kept, meaning strict check.

This will fix hashicorp#4509
and allow forinstance lb-* to match services lb-001 or lb-service-007
@pierresouchay
Copy link
Contributor Author

pierresouchay commented Sep 4, 2018

@mkeeler Hello Matt,

Do you think you might have a look ?
This PR is backwards compatible and allow more flexibility to tune DNS TTL when having lots of services or dynamically generated services. It would really help us scaling our DNS infrastructure when using Consul resolution.

Best Regards

@pierresouchay
Copy link
Contributor Author

@pearkes Do you think it could be included? (it a quite simple patch, safe in terms of ascending compatibility and will really help having good performance for DNS usages with many many services)

@pierresouchay pierresouchay mentioned this pull request Sep 9, 2018
@pierresouchay
Copy link
Contributor Author

@mkeeler do you think you can review?

This change is very backwards compatible and it would really help us to merge this in order to reduce significantly the load generated by DNS requests on Consul server.

Kind regards

@mkeeler mkeeler requested a review from a team September 21, 2018 16:14
@pierresouchay
Copy link
Contributor Author

@pearkes Do you think you might have a look? The change is pretty simple but would really help for people having large number of services. It is also very good at ascending compatibility since it simply allow the use of * not only for '*' but with any prefix.

Kind regards

pierresouchay added a commit to criteo-forks/consul that referenced this pull request Oct 9, 2018
This will fix hashicorp#4509
and allow forinstance lb-* to match services lb-001 or lb-service-007

Added unit tests with new TTLs prefix

Fixed unstable unit tests

Use Radix lookup for default key '*' on TTL lookups

Added documentation for DNS wildcard matches with prefix

Better documentation about DNS TTL cache mechanism
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Thanks Pierre sorry this one fell through the cracks. It seems like a solid and reasonable simple enhancement.

I don't think it will make it into today's release so no rush, but I think it's pretty much OK - I've made some minor comment and doc suggestions.

website/source/docs/guides/dns-cache.html.md Outdated Show resolved Hide resolved
website/source/docs/guides/dns-cache.html.md Outdated Show resolved Hide resolved
website/source/docs/guides/dns-cache.html.md Outdated Show resolved Hide resolved
agent/dns_test.go Outdated Show resolved Hide resolved
@pierresouchay
Copy link
Contributor Author

@banks Thank you for the review, all your comments resolved in next patch

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Great LGTM. We'll try to get this into 1.4.0!

@banks banks added this to the 1.4.0 milestone Oct 15, 2018
@hanshasselberg hanshasselberg changed the base branch from master to release/1.4-staging October 19, 2018 08:50
@hanshasselberg hanshasselberg merged this pull request into hashicorp:release/1.4-staging Oct 19, 2018
pearkes pushed a commit that referenced this pull request Oct 19, 2018
This will fix #4509 and allow forinstance lb-* to match services lb-001 or lb-service-007.
@banks banks mentioned this pull request Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants