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.

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

Jump to bottom

Add response-policy to dns::view #252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add response-policy to dns::view #252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions manifests/view.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@
# not to be unmanaged to be effective.
# @param order
# The order parameter to the concat fragment.
# @param response_policy
# 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>'}]
Comment on lines +42 to +45
Copy link
Member

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'}]
Comment on lines +46 to +48
Copy link
Member

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?

Copy link
Author

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'}
#     ],
#   }
#

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 that's good, but @example takes a description of the example. This allows puppet-strings to correctly list multiple examples and link to them.

Copy link
Author

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'
#       }
#     ],
#   }

Copy link
Author

Choose a reason for hiding this comment

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

Hi,
kindly review this and please proceed if everything is okay

#
define dns::view (
Array[String] $match_clients = [],
Expand All @@ -57,6 +65,7 @@
Boolean $include_localzones = true,
Boolean $include_defaultzones = true,
String $order = '-',
Optional[Dns::ResponsePolicy] $response_policy = undef,
) {
unless $dns::enable_views {
fail('Must set $dns::enable_views to true in order to use dns::view')
Expand Down
7 changes: 7 additions & 0 deletions templates/named.view_header.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ view "<%= @title %>" {
<% if @dnssec_validation -%>
dnssec-validation <%= @dnssec_validation %>;
<% end -%>
<% if @response_policy -%>
response-policy {
<% @response_policy.each do |policy| -%>
zone "<%= policy['zone'] %>"<% if policy['policy'] -%> policy <%= policy['policy'] %><% end -%><% if policy['policy'] == 'cname' && policy['cname_domain'] -%> <%= policy['cname_domain'] %><% end -%><% if policy['max_policy_ttl'] -%> max-policy-ttl <%= policy['max_policy_ttl'] %><% end -%><% if policy['log'] -%> log <%= policy['log'] %><% end -%>;
<% end -%>
};
<% end -%>

<% if @include_localzones -%>
<% if scope.lookupvar("::dns::localzonepath") != 'unmanaged' -%>
Expand Down
12 changes: 12 additions & 0 deletions types/responsepolicy.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
type Dns::ResponsePolicy = Array[
Struct[{
zone => String[1],
Copy link
Member

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

Suggested change
zone => String[1],
zone => Stdlib::Fqdn,

Copy link
Author

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

Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

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

yes, you are correct.

policy => Optional[Enum[
'given', 'disabled', 'passthru', 'drop',
'nxdomain', 'nodata', 'tcp-only', 'cname'
]],
cname_domain => Optional[String[1]],
Copy link
Member

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?

Suggested change
cname_domain => Optional[String[1]],
cname_domain => Optional[Stdlib::Fqdn],

Copy link
Author

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.

max_policy_ttl => Optional[Integer],
Copy link
Member

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?

Suggested change
max_policy_ttl => Optional[Integer],
max_policy_ttl => Optional[Integer[0]],

Copy link
Author

Choose a reason for hiding this comment

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

okay, no problem

log => Optional[Boolean]
}], 1, 32
]