-
Notifications
You must be signed in to change notification settings - Fork 112
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.
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
Add forwarders
to dns::zone
; deprecate allow_forwarder
#210
Conversation
Note - as this PR deprecates a parameter, SemVer requires a minor version bump if/when this is published. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one style suggestion.
manifests/zone.pp
Outdated
validate_array($forwarders) | ||
$zone_forwarders = $forwarders | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the style on this module, but this would read cleaner for me if lines 224-225 were replaced with elsif
.
This change adds the `forwarders` parameter to `dns::zone`, deprecating `allow_forwarder` in favor of `forwarders`. `forwarders` extends `allow_forwarder` by allowing both an *empty* forwarders list as well as an *undefined* forwarders list (the former meaning that the zone does not forward, while the latter leaves the global forwarders option in place): > [*forwarders*] > An array of IP addresses and optional port numbers to which queries > for this zone will be forwarded (based on the *forward_policy* > setting). If the optional port number is included, it must be > separated from the IP address by the word `port` - for example, > `[ '192.168.100.102 port 1234' ]`. If passed an empty array or the > boolean value `false`, the zone will not forward. If passed `true` > or left undefined, the zone will use the global forwarders defined > in `dns::server::options`. > *Note* - this parameter deprecates and should be used in place of > the *allow_forwarder* parameter. If both parameters are passed in, > only *forwarders* will take effect. This addresses Issue #204
I have a similar change in #196, except I went with option 2 and extended forwarders to slave and stub zones (see the zone statement grammar for justification). If this (option 3) is the preferred approach, can we augment templates/zone.erb to support slave and stub zones and get this merged? If it's preferable to keep the empty forwarder feature addition, and slave and stub forwarder support in separate PRs, I can submit a follow-up PR (it likely makes sense to merge this first as (either way) one will have to be rebased on the other). Is there any particular hold-up on this PR? |
As an aside, https://github.com/jjthiessen/puppet-dns/commits/add-forwarder-support-for-slave-and-stub-zones is this branch with the practically trivial addition of slave and stub forwarder support. |
This change adds the
forwarders
parameter todns::zone
, deprecatingallow_forwarder
in favor offorwarders
.forwarders
extendsallow_forwarder
by allowing both an empty forwarders list as well asan undefined forwarders list (the former meaning that the zone does not
forward, while the latter leaves the global forwarders option in place):
This addresses Issue #204