-
Notifications
You must be signed in to change notification settings - Fork 71
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 response-policy to dns::view #252
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR. It looks good, just some minor comments.
It would be great if you could add a test or 2 in https://github.com/theforeman/puppet-dns/blob/master/spec/defines/dns_view_spec.rb.
@@ -0,0 +1,12 @@ | |||
type Dns::ResponsePolicy = Array[ | |||
Struct[{ | |||
zone => String[1], |
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.
I imagine zone
is a domain, so we can enforce a stricter type
zone => String[1], | |
zone => Stdlib::Fqdn, |
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.
It seems Stdlib::Fqdn
might be too restrictive for the zone field
example: rpz
Using String[1]
will allow more flexibility in specifying zone to allow both FQDN and non-FQDN values
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.
Technically Stdlib::Fqdn
allows rpz
. It may be poorly named, but I'd prefer that. Strong validation is important to me.
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.
yes, you are correct.
'given', 'disabled', 'passthru', 'drop', | ||
'nxdomain', 'nodata', 'tcp-only', 'cname' | ||
]], | ||
cname_domain => Optional[String[1]], |
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.
Is this also a domain?
cname_domain => Optional[String[1]], | |
cname_domain => Optional[Stdlib::Fqdn], |
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.
yes, It should be a domain.
'nxdomain', 'nodata', 'tcp-only', 'cname' | ||
]], | ||
cname_domain => Optional[String[1]], | ||
max_policy_ttl => Optional[Integer], |
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.
Can we tighten this with a lower bound like 0
?
max_policy_ttl => Optional[Integer], | |
max_policy_ttl => Optional[Integer[0]], |
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.
okay, no problem
# Optional. An array of response policy configurations for the view in the | ||
# following format: | ||
# [{'zone' => '<ZONE_NAME>', 'policy' => '<POLICY_ACTION>', 'log' => true|false, | ||
# 'max_policy_ttl' => <TTL_VALUE>, 'cname_domain' => '<CNAME_DOMAIN>'}] |
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.
puppet-strings should link to the type alias, so explaining the format is probably redundant.
# Example: [{'zone' => 'example.com', 'policy' => 'passthru', 'log' => true, | ||
# 'max_policy_ttl' => 3600}, {'zone' => 'example.net', | ||
# 'policy' => 'cname', 'cname_domain' => 'example.com'}] |
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.
Perhaps add a full @example
instead?
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.
How about this
# @param response_policy
# Optional. An array of response policy configurations for the view in the
# following format:
# @example
# dns::view { 'example':
# response_policy => [
# {'zone' => 'example', 'policy' => 'passthru', 'log' => true, 'max_policy_ttl' => 3600},
# {'zone' => 'example.net', 'policy' => 'cname', 'cname_domain' => 'example.com'}
# ],
# }
#
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.
I think that's good, but @example
takes a description of the example. This allows puppet-strings to correctly list multiple examples and link to them.
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.
Yes, a brief description is important. Further, I would like to add multiple examples for separate use cases like below.
# @param response_policy
# Optional. An array of response policy configurations for the view.
#
# @example Complex response policy with multiple zones and policies
# dns::view { 'complex_example':
# response_policy => [
# {
# 'zone' => 'example',
# 'policy' => 'passthru',
# 'log' => true,
# 'max_policy_ttl' => 3600
# },
# {
# 'zone' => 'example.net',
# 'policy' => 'cname',
# 'cname_domain' => 'example.com'
# }
# ],
# }
#
# @example Simple response policy with a single zone
# dns::view { 'simple_example':
# response_policy => [
# {
# 'zone' => 'rpz.example.com'
# }
# ],
# }
For the test case, I have added most of the possible combinations below. However, I am not much familliar with this :)
|
This PR, try to addresses the issue #251, which involves adding a response-policy to the
dns::view
.Below example shows how to use the
dns::view
defined type with theresponse-policy
parameter:It will generate the following
BIND
configuration:The
response-policy
may support a few other parameters, which I have not included for simplicity.