Skip to content

Add a new array allow_update_cidr in zone management #64

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 2 commits into
base: master
Choose a base branch
from
Open

Add a new array allow_update_cidr in zone management #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
680d15c
Add a new array allow_update_cidr to specify CIDR addresses as possib…
Jan 11, 2016
64b2d53
Use empty method to check arrays
Jan 11, 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
40 changes: 21 additions & 19 deletions manifests/zone.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,25 @@
# *$zone_notify*: IPs to use for also-notify entry
#
define bind::zone (
$ensure = present,
$is_dynamic = false,
$allow_update = [],
$transfer_source = undef,
$zone_type = 'master',
$zone_ttl = undef,
$zone_contact = undef,
$zone_serial = undef,
$zone_refresh = '3h',
$zone_retry = '1h',
$zone_expiracy = '1w',
$zone_ns = [],
$zone_xfers = undef,
$zone_masters = undef,
$zone_forwarders = undef,
$zone_origin = undef,
$zone_notify = undef,
$is_slave = false,
$ensure = present,
$is_dynamic = false,
$allow_update = [],
$allow_update_cidr = [],
$transfer_source = undef,
$zone_type = 'master',
$zone_ttl = undef,
$zone_contact = undef,
$zone_serial = undef,
$zone_refresh = '3h',
$zone_retry = '1h',
$zone_expiracy = '1w',
$zone_ns = [],
$zone_xfers = undef,
$zone_masters = undef,
$zone_forwarders = undef,
$zone_origin = undef,
$zone_notify = undef,
$is_slave = false,
) {

include ::bind::params
Expand All @@ -48,6 +49,7 @@
validate_bool($is_slave)
validate_bool($is_dynamic)
validate_array($allow_update)
validate_array($allow_update_cidr)
validate_string($transfer_source)
validate_string($zone_type)
validate_string($zone_ttl)
Expand All @@ -60,7 +62,7 @@

validate_string($zone_origin)

# add backwards support for is_slave parameter
# add backwards support for is_slave parameter
if ($is_slave) and ($zone_type == 'master') {
warning('$is_slave is deprecated. You should set $zone_type = \'slave\'')
$int_zone_type = 'slave'
Expand Down
7 changes: 4 additions & 3 deletions templates/zone-master.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<%-
if @is_dynamic and @allow_update.empty?
raise(Puppet::ParseError, "allow_update is empty for dynamic zone '#{name}'")
if @is_dynamic and (@allow_update.empty? and @allow_update_cidr.empty?)
raise(Puppet::ParseError, "Both allow_update and allow_update_cidr are empty for dynamic zone '#{name}'")
Copy link
Contributor

@cjeanneret cjeanneret Feb 15, 2016

Choose a reason for hiding this comment

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

Not sure if allowing both parameters to be set is wise…
I think we should allow only Array XOR string.
Also, we might is is_a?(Array) in order to re-use the standard parameter, avoiding the hassle to check one more param. Of course, doing so will change the check l51-52 in the bind::zone class.

Copy link
Contributor Author

@vide vide Feb 15, 2016

Choose a reason for hiding this comment

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

Well, IMO it's a perfectly normal scenario: I can both accept updates from a secure network via CIDR validation and from an unsecure one with the secret key.
I don't understand what you mean to use is_a?(Array), do you mean instead of .empty? ?

Copy link
Contributor

@cjeanneret cjeanneret Feb 17, 2016

Choose a reason for hiding this comment

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

Forget it, the change I proposed would break existing configuration in fact.
I'll merge shortly, and we should get a new (working) release shortly (have to add a new feature, for logging support).

Copy link
Contributor Author

@vide vide Feb 17, 2016

Choose a reason for hiding this comment

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

Nice! Thank you very much :)

end
-%>
# File managed by puppet
Expand All @@ -17,7 +17,8 @@ zone "<%= @name %>" IN {
allow-transfer { none; };
<% end -%>
<% if @is_dynamic -%>
allow-update { key <%= Array(@allow_update).join('.; key ') -%>.; };
allow-update { <% if !@allow_update.empty? -%>key <%= Array(@allow_update).join('.; key ') -%>.;<% end
-%><% if !@allow_update_cidr.empty? -%> <%= Array(@allow_update_cidr).join('; ') -%>;<% end -%> };
<% end -%>
allow-query { any; };
notify yes;
Expand Down