Skip to content

Conversation

@vide
Copy link
Contributor

@vide vide commented Jan 11, 2016

Add a new, dedicated array so we can specify more allow-update values like networks, a part from keys, as per http://www.zytrax.com/books/dns/ch7/xfer.html#allow-update

I created a new array to maintain backwards compatibility with the keys-only array and because they have slightly different syntax + different requirements

@vide vide force-pushed the extend_allow_update branch from a5d9d1d to 64b2d53 Compare January 11, 2016 15:36
@cjeanneret
Copy link
Contributor

Hello,

Sorry for the delay — you might want to rebase on latest master so that we should be able to merge your PR.

Cheers,

C.

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

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

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

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

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

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.

2 participants