Skip to content
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

Adds control keys and specifying update policy #108

Merged
merged 1 commit into from
Feb 14, 2018

Conversation

zyronix
Copy link
Contributor

@zyronix zyronix commented Feb 11, 2018

Fixes GH-94
This commit adds a new resource type key, which creates a rndc key which can be used to control bind using nsupdate.
Also this commit allows to specify update policy rules for zones.

I see that the beaker tests are failing and I also have not written a beaker test for this one.
Please let me know a beaker test is required.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Overall it looks pretty good. Mostly polishing comments.

manifests/key.pp Outdated
) {
$keyfilename = "${keydir}/${filename}"

if ! $secret {
Copy link
Member

Choose a reason for hiding this comment

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

This can be unless $secret but it might be better to use if $secret { file ... } else { exec ... file ... } instead

manifests/key.pp Outdated

if ! $secret {
exec { "create-${filename}":
command => "${dns::rndcconfgen} -r /dev/urandom -a -c ${keyfilename} -b 512 -k ${name}",
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to make the keysize a parameter?

manifests/key.pp Outdated
}
}

concat::fragment { 'named.conf+20-keys.dns':
Copy link
Member

Choose a reason for hiding this comment

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

You should include the key name in here because otherwise you get duplicate resource definitions when dns::key is used multiple times.

@@ -12,6 +12,11 @@ zone "<%= @zone %>" {
<% if @zonetype == 'master' -%>
update-policy {
grant rndc-key zonesub ANY;
<%- unless @update_policy_rules.empty? -%>
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is redundant since iterating over an empty hash will not produce any output either.

end


# # context 'secret set' do
Copy link
Member

Choose a reason for hiding this comment

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

What's the plan with this commented code?

key "<%= @name %>" {
algorithm <%= @algorithm %>;
secret "<%= @secret %>";
};
Copy link
Member

Choose a reason for hiding this comment

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

Could yuo add a newline here?

Enum['first', 'only'] $forward = 'first',
Array $forwarders = [],
Optional[Enum['yes', 'no', 'explicit']] $dns_notify = undef,
Hash[String, Hash[String, Data]] $update_policy_rules = {},
Copy link
Member

Choose a reason for hiding this comment

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

Instead of Data you could add a custom data type to verify the fields in it similar to theforeman/puppet-foreman#601 but that does break with kafo so I'd be fine with holding that off.

@zyronix
Copy link
Contributor Author

zyronix commented Feb 12, 2018

Bedankt!. That was a very fast reply and review 👍 . I have applied all the requested changes.

btw, re-reading the issues. I think this one also fixes: GH-90. No the part for the custom template but for specifying additional update-zone-policies.

Fixes theforemanGH-94
This commit adds a new resource type key, which creates a rndc key which can be used to control bind using nsupdate.
Also this commit allows to specify update policy rules for zones.
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@ekohl
Copy link
Member

ekohl commented Feb 12, 2018

I think we already had failing beaker tests before this patch so I think you can ignore that. Especially since we don't test the modified code.

@zyronix
Copy link
Contributor Author

zyronix commented Feb 14, 2018

What is next? Do I have to do something to get this merged?

@ekohl ekohl requested a review from mmoll February 14, 2018 10:54
@mmoll mmoll merged commit 3703d83 into theforeman:master Feb 14, 2018
@mmoll
Copy link
Contributor

mmoll commented Feb 14, 2018

merged, bedankt @zyronix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants