Skip to content

Add DNS Check#109

Merged
dselans merged 2 commits into
masterfrom
relistan/dns-check
Mar 6, 2017
Merged

Add DNS Check#109
dselans merged 2 commits into
masterfrom
relistan/dns-check

Conversation

@relistan
Copy link
Copy Markdown
Collaborator

@relistan relistan commented Mar 5, 2017

This adds a DNS check and docs and resolves #28 . The test suite is not fully complete since I still need to figure out how to mock out the DNS calls. The check has been tested locally and seems to work. The docs explain the mechanics.

I had to add a dependency on miekg/dns because the built-in Go resolver only supports querying hosts that are contained in the normal host resolver (i.e. /etc/resolv.conf) which is no good from a monitoring standpoint since it puts too much of the OS stuff in the way of the DNS query. That would be a useful check to make sure DNS resolution is working on a node, but not useful in checking if an arbitrary host can resolve an arbitrary record. Also, I can set a proper timeout with this library.

The configuration of the check allows it to do a number of things and the defaults try to do the sane thing.

Let me know what you think.

@skey

@relistan
Copy link
Copy Markdown
Collaborator Author

relistan commented Mar 5, 2017

Example output from /api/v1/state on my machine:

{

    "check": "google-com-dns",
    "owner": "50f7c76a",
    "status": "ok",
    "count": 0,
    "message": "N/A",
    "date": "2017-03-05T13:50:06.577859813Z",
    "config": {
        "type": "dns",
        "description": "google.com DNS check",
        "host": "8.8.8.8",
        "interval": "10s",
        "timeout": "1s",
        "dns-target": "google.com",
        "dns-record-type": "A",
        "dns-max-time": "100ms",
        "dns-expected-count": 0,
        "warning-threshold": 1,
        "critical-threshold": 3,
        "warning-alerter": [
            "primary-slack"
        ],
        "critical-alerter": [
            "primary-email"
        ]
    }

}

Comment thread monitor/dns_monitor.go Outdated
if foundCount > 0 && foundCount < expectedCount {
return fmt.Errorf(
"DNS check of %s against %s had %d records. Only %d matched",
dns.RMC.Config.DnsTarget, dns.RMC.Config.Host, dns.RMC.Config.Expect,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you're missing a field here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Fixed

@relistan
Copy link
Copy Markdown
Collaborator Author

relistan commented Mar 5, 2017

I'll rebase/squash this PR once I've got a thumbs up.

Copy link
Copy Markdown
Member

@dselans dselans left a comment

Choose a reason for hiding this comment

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

Looks awesome! One minor thing about the PTR record type.

Comment thread monitor/dns_monitor.go Outdated
case "SRV":
msgType = resolver.TypeSRV
case "TXT":
msgType = resolver.TypeTXT
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably should include PTR here as well.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I spent forever looking and couldn't find a function in that lib that did this. But it turns out there is a map in there that will do it for all the records it will support. I'll push that up shortly.

Using this resolver lib because the built-in Go
resolver will only use the resolv.conf settings
and can't be pointed to an arbitrary host to resolve
against.
@relistan relistan force-pushed the relistan/dns-check branch from 079d0fd to 0a93f84 Compare March 5, 2017 21:03
@relistan
Copy link
Copy Markdown
Collaborator Author

relistan commented Mar 5, 2017

Thanks @skey! Fixed that issue as I described above. Also found small bug in the matcher which is fixed. Finally, squashed into a nice commit on this branch. Ready for merge if you're happy.

@dselans
Copy link
Copy Markdown
Member

dselans commented Mar 6, 2017

I went ahead and added the validation -- if there's a reason you excluded it, we can pull it out in another PR. Going ahead and merging - this is awesome!

Comment thread monitor/dns_monitor.go
return dns
}

func (dns *DnsMonitor) Validate() error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about adding a record type check in here? That way if someone puts in a "foo" record type in the config, it won't even bother starting the check. Another side note (and probably separate pr) - a failed validation should probably bubble up an event.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Went ahead and added it to your branch.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Great catch, just overlooked it.

Comment thread monitor/dns_monitor.go
func (dns *DnsMonitor) dnsCheck() error {
msg := &resolver.Msg{}

qType, ok := resolver.StringToType[strings.ToUpper(dns.RecordType)]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Glad this exists!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I couldn't believe it didn't, but the docs are not great and it took hours to find it.

@dselans dselans merged commit 2a154ee into master Mar 6, 2017
@dselans dselans deleted the relistan/dns-check branch March 6, 2017 03:29
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.

DNS check

3 participants