Added DNS zone check #29
Conversation
|
testing |
There was a problem hiding this comment.
Overall looks good a few comments.
Also it looks like the build is failing with ruby 2.1. As ruby 2.1 is EOL if we want to we can drop support for it (must be called out in the changelog and I will version it as a major releases) or we can look into what causes the failure I am certain its dependency related and would vote for just dropping support.
bin/check-dns-zone.rb
Outdated
| Resolv::DNS.new.getresources(config[:domain], Resolv::DNS::Resource::IN::NS).map { |e| e.name.to_s } | ||
| end | ||
|
|
||
| def check_nss(entries) |
There was a problem hiding this comment.
Can we use a better name? check_nss made me think for a minute you were talking about mozilla's Network Security Services Library
bin/check-dns-zone.rb
Outdated
| [errors, success] | ||
| end | ||
|
|
||
| def check_udp(entries) |
There was a problem hiding this comment.
can we use a better name? how about something more like check_dns_udp?
bin/check-dns-zone.rb
Outdated
| [errors, success] | ||
| end | ||
|
|
||
| def check_tcp(entries) |
There was a problem hiding this comment.
can we use a better name? how about something more like check_dns_tcp?
There was a problem hiding this comment.
Also I feel like we can combine the two checks and just pass in whether we want udp or tcp.
There was a problem hiding this comment.
@majormoses, I agree with you - these methods have the same checking logic.
I will merge them.
bin/check-dns-zone.rb
Outdated
| [errors, success] | ||
| end | ||
|
|
||
| def check_dns_connection(entries, use_tcp = true) |
There was a problem hiding this comment.
DNS is default served over therefore I think that defaulting to false makes more sense.
bin/check-dns-zone.rb
Outdated
|
|
||
| success << "Resolved DNS #{ns}(#{ip}) uses #{use_tcp ? 'tcp' : 'udp'}" | ||
| rescue StandardError | ||
| errors << "Resolved DNS #{ns}(#{ip}) doesn't use #{use_tcp ? 'tcp' : 'udp'}" |
There was a problem hiding this comment.
rather than using the same conditional again just assign it once out of the rescue block and use it for both inside the begin and rescue.
CHANGELOG.md
Outdated
| - Added many checks for DNS zone (@yuri-zubov sponsored by Actility, https://www.actility.com) | ||
|
|
||
| ### Breaking Changes | ||
| - Dropping ruby `< 2.1` support (@yuri-zubov) |
There was a problem hiding this comment.
in the gemspec you say it must be 2.2+ but here you have < 2.1 I think this should be < 2.2 here.
|
@majormoses fixed |
Pull Request Checklist
Is this in reference to an existing issue?
General
Update Changelog following the conventions laid out here
Update README with any necessary configuration snippets
Binstubs are created if needed
RuboCop passes
Existing tests pass
New Plugins
Tests
Add the plugin to the README
Does it have a complete header as outlined here
Purpose
Our idea was twofold: a healtcheck against authoritative servers
Known Compatibility Issues
Create a check to test for zone transfers #27
[CVE-2017-8418] sensu-plugins/community#77
[CVE-2017-17042] sensu-plugins/community#97