Skip to content

[dns] Allow expected address to be checked in dns_check. #2799

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[dns] Allow expected address to be checked in dns_check. #2799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e3ceaa
Allow expected address to be checked.
cory-stripe Aug 31, 2016
5f6a781
Adjust exception message.
cory-stripe Sep 1, 2016
aa11e01
Adjustments from PR and a first stab at multiple addresses.
cory-stripe Sep 22, 2016
65787fe
Try and test multiple addresses.
cory-stripe Sep 22, 2016
474a1f5
Fix bad c&p :P
cory-stripe Sep 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions checks.d/dns_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,27 @@ def query_dns(self, instance, timeout):
resolver.nameservers = [nameserver]

record_type = instance.get('record_type', 'A')
expected_results = instance.get('addresses', [])

status = AgentCheck.CRITICAL
start_time = time.time()
try:
self.log.debug('Querying "%s" record for hostname "%s"...' % (record_type, hostname))
answer = resolver.query(hostname, rdtype=record_type)
resolved_value = answer.rrset.items[0].to_text()
Copy link
Member

@degemer degemer Sep 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What about multiple DNS records ? We should probably takes all the addresses and try to see if one of them matches (all of them maybe makes more sense). What do you think ?

assert(answer.rrset.items[0].to_text())
end_time = time.time()
if len(expected_results) > 0:
if resolved_value not in expected_results:
raise Exception('DNS resolution of %s resulted in unexpected address %s.' % (hostname, resolved_value))
Copy link
Member

Choose a reason for hiding this comment

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

There might be a better way than raising an exception, since this verification can only take place if the DNS resolution was done correctly: put this block after the else, before the if end_time - start_time > 0, and log an error + service critical. What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

Just noticed, this is also a way to fix https://travis-ci.org/DataDog/dd-agent/jobs/156839184#L608


except dns.exception.Timeout:
self.log.error('DNS resolution of %s timed out' % hostname)
self.service_check(self.SERVICE_CHECK_NAME, status, tags=self._get_tags(instance))
raise
except Exception:
self.log.exception('DNS resolution of %s has failed.' % hostname)
self.service_check(self.SERVICE_CHECK_NAME, status, tags=self._get_tags(instance))
except Exception as err:
self.log.exception(err)
Copy link
Member

Choose a reason for hiding this comment

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

Passing the exception is not needed, log.exception prints the exception anyway. :)

self.service_check(self.SERVICE_CHECK_NAME, status, tags=self._get_tags(instance), message=err)
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what kind of exception is going to be raised ? I'm wondering if seeing the python exception is useful.

raise
else:
if end_time - start_time > 0:
Expand Down
6 changes: 6 additions & 0 deletions conf.d/dns_check.yaml.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,9 @@ instances:
# Specify an (optional) `record_type` to customize the record type
# queried by the check (default: "A")
# record_type: A

# Specify expected results if you want to check that the returned IP
# matches. If you supply multiple records then a match to any one will
# be considered success
# addressses:
Copy link
Member

Choose a reason for hiding this comment

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

Only two s needed 😃

# - 127.0.0.1
24 changes: 24 additions & 0 deletions tests/checks/mock/test_dns_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,30 @@ def test_success(self, mocked_query):
tags=['resolved_hostname:www.example.org', 'nameserver:127.0.0.1'])
self.coverage_report()

@mock.patch.object(Resolver, 'query', side_effect=success_query_mock)
def test_success_addresses(self, mocked_query):
config = {
'instances': [{'hostname': 'www.example.org', 'nameserver': '127.0.0.1', 'addresses': ['127.0.0.1']}]
}
self.run_check(config)
self.assertMetric('dns.response_time', count=1,
tags=['nameserver:127.0.0.1', 'resolved_hostname:www.example.org'])
self.assertServiceCheck(SERVICE_CHECK_NAME, status=AgentCheck.OK,
tags=['resolved_hostname:www.example.org', 'nameserver:127.0.0.1'])
self.coverage_report()

@mock.patch.object(Resolver, 'query', side_effect=success_query_mock)
def test_failure_addresses(self, mocked_query):
config = {
'instances': [{'hostname': 'www.example.org', 'nameserver': '127.0.0.1', 'addresses': ['127.0.0.2']}]
}
self.run_check(config)
self.assertMetric('dns.response_time', count=1,
tags=['nameserver:127.0.0.1', 'resolved_hostname:www.example.org'])
self.assertServiceCheck(SERVICE_CHECK_NAME, status=AgentCheck.CRITICAL,
tags=['resolved_hostname:www.example.org', 'nameserver:127.0.0.1'])
self.coverage_report()

@mock.patch.object(Resolver, 'query', side_effect=success_query_mock)
def test_success_CNAME(self, mocked_query):
config = {
Expand Down