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

Add response-policy to dns::view #252

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

supun983
Copy link

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 the response-policy parameter:

dns::view { 'example_view':
  # ...
  response_policy      => [
    {
      'zone'           => 'example.com',
      'policy'         => 'passthru',
      'max_policy_ttl' => 3600,
      'log'            => true,
    },
    {
      'zone'         => 'example.net',
      'policy'       => 'cname',
      'cname_domain' => 'example.net',
    },
  ],
}

It will generate the following BIND configuration:

view "example_view" {
    # ...
    response-policy {
        zone "example.com" policy passthru max-policy-ttl 3600 log true;
        zone "example.net" policy cname example.net;
    };
}

The response-policy may support a few other parameters, which I have not included for simplicity.

@ekohl ekohl linked an issue May 16, 2024 that may be closed by this pull request
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.

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.

types/responsepolicy.pp Outdated Show resolved Hide resolved
types/responsepolicy.pp Outdated Show resolved Hide resolved
types/responsepolicy.pp Outdated Show resolved Hide resolved
Comment on lines +42 to +45
# 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>'}]
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.

Comment on lines +46 to +48
# Example: [{'zone' => 'example.com', '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.

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

@supun983
Copy link
Author

For the test case, I have added most of the possible combinations below. However, I am not much familliar with this :)

context "with response policy configuration" do
  let(:title) { "response_policy_view" }
  let :pre_condition do
    'class {"::dns": enable_views => true}'
  end
  let(:params) { {
    response_policy: [
      {'zone' => 'example.com', 'policy' => 'passthru', 'log' => true, 'max_policy_ttl' => 3600},
      {'zone' => 'rpz', 'policy' => 'drop'},
      {'zone' => 'example.net', 'policy' => 'cname', 'cname_domain' => 'example.com'},
      {'zone' => 'rpz.local', 'policy' => 'nxdomain'},
      {'zone' => 'example.org', 'policy' => 'nodata', 'log' => false},
      {'zone' => 'private', 'policy' => 'passthru'},
    ],
  } }

  it { should compile.with_all_deps }

  it "should have response policy configuration" do
    verify_concat_fragment_exact_contents(catalogue, 'dns_view_header_response_policy_view.dns', [
      'view "response_policy_view" {',
      '    response-policy {',
      '        zone "example.com" policy passthru max-policy-ttl 3600 log true;',
      '        zone "rpz" policy drop;',
      '        zone "example.net" policy cname example.com;',
      '        zone "rpz.local" policy nxdomain;',
      '        zone "example.org" policy nodata log false;',
      '        zone "private" policy passthru;',
      '    };',
    ])
  end
end

@supun983
Copy link
Author

supun983 commented Sep 3, 2024

Hi, please let me know how to proceed with the changes.

Implemented as suggested

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
supun983 and others added 2 commits November 28, 2024 13:19
Implemented as suggested

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
Implementing as suggested

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
@supun983
Copy link
Author

The commit suggestion actions failed due to outdated changes and CI pipeline issues. I’ve added a test case to address the feedback as well. Could you please review it when you get a chance?

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.

Add response-policy to the dns::view
2 participants