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 all 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
15 changes: 11 additions & 4 deletions checks.d/dns_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,23 +52,30 @@ 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)
assert(answer.rrset.items[0].to_text())
resolved_results = map(lambda x: x.to_text(), answer.rrset.items)
Copy link
Member

Choose a reason for hiding this comment

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

This won't fail when the first item is the empty string, whereas it was failing previously. Maybe filter this resolved_results to remove empty strings and later check that resolved_results != [] ?

end_time = time.time()

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 len(expected_results) > 0:
missing_values = list(set(expected_results, resolved_results))
Copy link
Member

@degemer degemer Oct 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

missing_values = [r for r in expected_results if r not in resolved_results]

maybe ? Unless you have a better way to do it.

Copy link

Choose a reason for hiding this comment

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

I’d prefer list(set(expected_results) - set(resolved_results)) for efficiency unless there’s a reason to prefer the comprehension

Copy link
Member

Choose a reason for hiding this comment

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

Your method is also way easier to read/understand. 👍

if len(missing_values) > 0:
self.log.error('DNS resolution of %s did not contain expected address(es) %s.' % (hostname, ", ".join(missing_values)))
self.service_check(self.SERVICE_CHECK_NAME, status, tags=self._get_tags(instance))
Copy link
Member

Choose a reason for hiding this comment

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

A OK service check might be sent L74 even when this CRITICAL is sent.
One way to solve this would be to handle the case end_time < start_time first (and maybe send a service check with WARNING ?), then the check for the missing values and finally the OK service check.
What do you think @hkaj ?

Copy link
Member

Choose a reason for hiding this comment

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

We can probably remove the if end_time - start_time > 0: condition and just do:

if len(missing_values) > 0:
    self.log.error('DNS resolution of %s did not contain expected address(es) %s.' % (hostname, ", ".join(missing_values)))
    self.service_check(self.SERVICE_CHECK_NAME, status, tags=self._get_tags(instance))
else:
    self.service_check(self.SERVICE_CHECK_NAME, AgentCheck.OK, tags=self._get_tags(instance))
self.gauge('dns.response_time', end_time - start_time, tags=tags)

if end_time - start_time > 0:
self.gauge('dns.response_time', end_time - start_time, tags=tags)
self.service_check(self.SERVICE_CHECK_NAME, AgentCheck.OK, tags=self._get_tags(instance))
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 ALL included addresses
# will need to match!
# addresses:
# - 127.0.0.1
59 changes: 55 additions & 4 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 @@ -18,12 +18,12 @@


class MockDNSAnswer:
def __init__(self, address):
self.rrset = MockDNSAnswer.MockRrset(address)
def __init__(self, *args):
self.rrset = MockDNSAnswer.MockRrset(args)

class MockRrset:
def __init__(self, address):
self.items = [MockDNSAnswer.MockItem(address)]
def __init__(self, *args):
self.items = map(lambda x: MockDNSAnswer.MockItem(x), args)

class MockItem:
def __init__(self, address):
Expand All @@ -39,6 +39,9 @@ def success_query_mock(d_name, rdtype):
elif rdtype == 'CNAME':
return MockDNSAnswer('alias.example.org')

def multiple_records_query_mock(d_name, rdtype):
return MockDNSAnswer('127.0.0.1','127.0.0.2')


class TestDns(AgentCheckTest):
CHECK_NAME = 'dns_check'
Expand All @@ -55,6 +58,54 @@ 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_success_multiple_addresses(self, multiple_records_query_mock):
config = {
'instances': [{'hostname': 'www.example.org', 'nameserver': '127.0.0.1', 'addresses': ['127.0.0.1','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.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_multiple_addresses(self, multiple_records_query_mock):
config = {
'instances': [{'hostname': 'www.example.org', 'nameserver': '127.0.0.1', 'addresses': ['127.0.0.1','127.0.0.3']}]
}
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_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