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

gphat · 2016-08-31T14:46:44Z

What does this PR do?

Allows the DNS check to verify the resolved address against a list of expected values.

Motivation

We have a number of DNS sanity checks that we check for known IPs. Think of public-facing ingestion points for which IP addresses are published.

Testing Guidelines

Includes unit tests within existing dns_check tests.

Additional Notes

I like your new PR format. 😸

cc @rhwlo

degemer · 2016-09-21T20:38:30Z

checks.d/dns_check.py

            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))


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 ?

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

degemer · 2016-09-21T20:38:51Z

conf.d/dns_check.yaml.example

+      # 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:


Only two s needed 😃

degemer

Thanks for the PR @gphat !

I added a few comments, and we have to decide what happens when there are multiple addresses. Let me know what you think !

degemer · 2016-09-21T21:10:41Z

checks.d/dns_check.py


        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()


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 ?

gphat · 2016-09-23T15:22:47Z

Looks like this might be right now? I need to verify this on my end as well…

degemer

Thanks for the changes @gphat, a few new comments. 😃

degemer · 2016-10-03T13:28:31Z

checks.d/dns_check.py

-            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)


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

degemer · 2016-10-03T14:26:47Z

checks.d/dns_check.py

-            self.service_check(self.SERVICE_CHECK_NAME, status, tags=self._get_tags(instance))
+        except Exception as err:
+            self.log.exception(err)
+            self.service_check(self.SERVICE_CHECK_NAME, status, tags=self._get_tags(instance), message=err)


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

degemer · 2016-10-04T10:34:19Z

checks.d/dns_check.py

            raise
        else:
+            if len(expected_results) > 0:
+                missing_values = list(set(expected_results, resolved_results))


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

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

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

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

degemer · 2016-10-04T10:44:14Z

checks.d/dns_check.py

+                missing_values = list(set(expected_results, resolved_results))
+                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))


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 ?

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)

degemer · 2016-10-04T10:46:04Z

checks.d/dns_check.py


        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)


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 != [] ?

cory-stripe · 2016-10-27T19:41:31Z

Oops, kinda forgot about this. :)

sjenriquez · 2017-01-31T22:34:18Z

Hey @cory-stripe, my most recent PR #2924 impacts your work here. I'm moving the DNS check over to the integrations-core repo. The changes here are great, we hope you'll move this PR to the new repo!

cory-stripe · 2017-01-31T22:36:35Z

Ok, I'll check in to this soon. Thanks!

gphat changed the title ~~Allow expected address to be checked.~~ Allow expected address to be checked in dns_check. Aug 31, 2016

Allow expected address to be checked.

5e3ceaa

gphat changed the title ~~Allow expected address to be checked in dns_check.~~ [dns] Allow expected address to be checked in dns_check. Aug 31, 2016

Adjust exception message.

5f6a781

degemer reviewed Sep 21, 2016

View reviewed changes

degemer suggested changes Sep 21, 2016

View reviewed changes

degemer added checks feature community labels Sep 21, 2016

degemer added this to the Triage milestone Sep 21, 2016

cory-stripe added 3 commits September 22, 2016 08:09

Adjustments from PR and a first stab at multiple addresses.

aa11e01

Try and test multiple addresses.

65787fe

Fix bad c&p :P

474a1f5

degemer self-assigned this Sep 27, 2016

degemer modified the milestones: 5.10.0, Triage Oct 3, 2016

degemer suggested changes Oct 4, 2016

View reviewed changes

sjenriquez mentioned this pull request Oct 18, 2016

[dns_check] change to network check #2924

Merged

olivielpeau modified the milestones: Triage, 5.10.0 Oct 24, 2016

truthbk added the sdk-triage label Jan 30, 2017

gmmeyer assigned sjenriquez Jan 31, 2017

gmmeyer added sdk-later and removed sdk-triage labels Jan 31, 2017

sjenriquez assigned irabinovitch and unassigned degemer and sjenriquez Jan 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gphat commented Aug 31, 2016 •

edited

Loading

degemer Sep 21, 2016

degemer Sep 21, 2016

degemer Sep 21, 2016

degemer left a comment

degemer Sep 21, 2016 •

edited

Loading

gphat commented Sep 23, 2016

degemer left a comment

degemer Oct 3, 2016

degemer Oct 3, 2016

degemer Oct 4, 2016 •

edited

Loading

rhwlo Oct 4, 2016

degemer Oct 4, 2016

degemer Oct 4, 2016

hkaj Oct 4, 2016

degemer Oct 4, 2016

cory-stripe commented Oct 27, 2016

sjenriquez commented Jan 31, 2017

cory-stripe commented Jan 31, 2017

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

Are you sure you want to change the base?

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

Conversation

gphat commented Aug 31, 2016 • edited Loading

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

degemer left a comment

Choose a reason for hiding this comment

degemer Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

gphat commented Sep 23, 2016

degemer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

degemer Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cory-stripe commented Oct 27, 2016

sjenriquez commented Jan 31, 2017

cory-stripe commented Jan 31, 2017

gphat commented Aug 31, 2016 •

edited

Loading

degemer Sep 21, 2016 •

edited

Loading

degemer Oct 4, 2016 •

edited

Loading