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

Disable DNS Result Caching #100

Merged
merged 2 commits into from
Jun 8, 2018
Merged

Disable DNS Result Caching #100

merged 2 commits into from
Jun 8, 2018

Conversation

snh
Copy link
Member

@snh snh commented Jun 8, 2018

Dnsruby by default appears to cache results internally, which prevents the redundant checking functionality as implemented in #90 and #98 from performing as expected.

In particular, the earlier cached entries from the default nameservers are returned when later trying the authoritative and public nameservers.

This PR disables this caching in Dnsruby.

While rudimentary, I was able to test this was effective by performing the following two queries in script/console, spaced sufficiently apart to be able to determine by the returned TTLs that they were not sharing a local cached entry:

GitHubPages::HealthCheck::Domain.new("pages.github.com", :nameservers => :default).dns
GitHubPages::HealthCheck::Domain.new("pages.github.com", :nameservers => :public).dns

Without this fix, it is obvious that they are sharing the same cache entry, as the results from these two queries return TTLs that align perfectly.

@snh snh added the bug label Jun 8, 2018
@snh snh requested a review from parkr June 8, 2018 11:30
@benbalter
Copy link
Contributor

@snh I corrected the failing Rubocop offenses on master. If you merge it in, tests should pass.

@snh snh requested a review from benbalter June 8, 2018 14:04
@@ -6,7 +6,8 @@ class Resolver
DEFAULT_RESOLVER_OPTIONS = {
:retry_times => 2,
:query_timeout => 5,
:dnssec => false
:dnssec => false,
:do_caching => false
Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously nothing we can do about it, but just wanted to note that do_caching is a silly flag name. 😄

@parkr
Copy link
Contributor

parkr commented Jun 8, 2018

@snh You're the bomb dot com, thank you! Great sleuthing. 🔍

@benbalter thanks for the quick review on this too! ❤️

@parkr parkr merged commit bdaa8b5 into github:master Jun 8, 2018
@snh snh deleted the disable-caching branch June 8, 2018 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants