-
Notifications
You must be signed in to change notification settings - Fork 57
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 a new array allow_update_cidr in zone management #64
base: master
Are you sure you want to change the base?
Conversation
a5d9d1d
to
64b2d53
Compare
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}'") |
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 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.
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.
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?
?
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.
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).
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.
Nice! Thank you very much :)
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