Add statistics cix 9690

[rsauber-ot]

PR Type

Enhancement, Bug fix

---

Description

• Add TSIG keys support and forward mode configuration

• Fix zone file validation and RHEL8 bindkeys file path

• Add statistics channel and improve template zone handling

• Update module metadata and improve documentation

---

Diagram Walkthrough

flowchart LR
  A["TSIG Keys Support"] --> B["Enhanced named.conf Template"]
  C["Zone File Validation"] --> B
  D["Forward Mode Config"] --> B
  E["Statistics Channel"] --> B
  F["RHEL8 Bindkeys Fix"] --> B
  G["Metadata.json"] --> H["Module Modernization"]
  B --> I["Improved BIND Configuration"]

Loading

File Walkthrough

	Relevant files
Documentation	3 files CHANGELOG.md Update changelog with version history                                        +37/-13  LICENSE Update copyright year to 2016                                                        +1/-1      README.md Add disclaimer about module maintenance                                    +10/-0
Configuration changes	2 files Modulefile Remove deprecated Modulefile                                                          +0/-8      metadata.json Add new metadata.json with module details                                +36/-0
Enhancement	6 files init.pp Refactor chroot handling and service naming                            +28/-17  params.pp Add OS-specific zone file paths                                                    +20/-0    conf.pp Add forward mode and keys support                                                +24/-6    file.pp Add zone validation and replace parameter                                +27/-16  service.pp Update service resource configuration                                        +6/-6      named.conf.erb Add keys section and statistics channel                                    +22/-7
Formatting	1 files package.pp Update package ensure to string literal                                    +1/-2
Bug fix	1 files server.pp Fix class inheritance and references                                          +5/-4

---

[qodo-pr-code-review-v2]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 Security concerns Command injection: manifests/server/file.pp builds validate_cmd by interpolating zonename into a shell command string. If zonename is not strictly validated, a crafted value could inject additional shell tokens/flags. Additionally, the commented example TSIG secret value in manifests/server/conf.pp may encourage storing secrets in plaintext in manifests/Hiera; consider recommending use of Puppet Sensitive data and/or external secret management in documentation/examples.

⚡ Recommended focus areas for review Command Injection The validate_cmd for zone files interpolates zonename directly into a shell command. If zonename can be influenced by user input (directly or via Hiera), this could allow shell metacharacters or unexpected arguments to be injected. Consider validating/sanitizing zonename (e.g., strict DNS label regex) or avoiding shell interpolation patterns. file { "${zonedir}/${title}": ensure => $ensure, owner => $owner, group => $bindgroup, mode => $mode, source => $zone_source, content => $content, replace => $replace, validate_cmd => "/usr/sbin/named-checkzone -k fail -m fail -M fail -n fail -r fail -S fail -T warn -W warn ${zonename} %", notify => Class['::bind::service'], # For the parent directory require => [ Class['::bind::package'], File[$zonedir], ], } Behavior Change The default DNSSEC-related behavior changes: dnssec_validation now defaults to no, and dnssec-enable / dnssec-lookaside parameters were removed from the define. This is likely a breaking change for existing users expecting DNSSEC validation enabled by default; confirm intent and consider documenting prominently (or preserving previous defaults/parameters for compatibility). define bind::server::conf ( $acls = {}, $masters = {}, $listen_on_port = '53', $listen_on_addr = [ '127.0.0.1' ], $listen_on_v6_port = '53', $listen_on_v6_addr = [ '::1' ], $forwarders = [], $directory = '/var/named', $managed_keys_directory = undef, $hostname = undef, $forward = undef, $server_id = undef, $version = undef, $dump_file = '/var/named/data/cache_dump.db', $statistics_file = '/var/named/data/named_stats.txt', $memstatistics_file = '/var/named/data/named_mem_stats.txt', $allow_query = [ 'localhost' ], $allow_query_cache = [], $recursion = 'yes', $allow_recursion = [], $allow_transfer = [], $check_names = [], $extra_options = {}, $dnssec_validation = 'no', $zones = {}, $keys = {}, $includes = [], $views = {}, ) { # OS Defaults include '::bind::params' $file_hint = $::bind::params::file_hint $file_rfc1912 = $::bind::params::file_rfc1912 $file_bindkeys = $::bind::params::file_bindkeys # Everything is inside a single template file { $title: notify => Class['::bind::service'], content => template('bind/named.conf.erb'), require => Class['::bind::package'], } Template Correctness The new ERB blocks around ACL/key rendering and view/global zone sections add several end statements and conditionals. It would be good to double-check the block structure renders valid named.conf across combinations (empty/non-empty acls, keys, views, zones) and that there are no extra/missing end tags that change output formatting or omit sections. acl <%= key %> { <% value.each do |line| -%> <%= line %>; <% end -%> }; <% end -%> <% end -%> <% if !@keys.empty? -%> <% @keys.sort_by {|key, value| key}.each do |key,value| -%> key "<%= key %>" { <% value.each do |line| -%> <%= line %>; <% end -%> }; <% end -%> <% end -%> <% if !@masters.empty? -%> <% @masters.sort_by {|key, value| key}.each do |key,value| -%> masters <%= key %> { <% value.each do |line| -%> <%= line %>; <% end -%> }; <% end -%> <% end -%> options { <% if @listen_on_port -%> listen-on port <%= @listen_on_port %> { <%= @listen_on_addr.join("; ") %>; }; <% end -%> <% if @listen_on_v6_port -%> listen-on-v6 port <%= @listen_on_v6_port %> { <%= @listen_on_v6_addr.join("; ") %>; }; <% end -%> <% if !@forwarders.empty? -%> forwarders { <%= @forwarders.join("; ") %>; }; <% end -%> <% if @forward -%> forward <%= @forward %>; <% end -%> directory "<%= @directory %>"; <% if @managed_keys_directory -%> managed-keys-directory "<%= @managed_keys_directory %>"; <% end -%> <% if @hostname -%> hostname "<%= @hostname %>"; <% end -%> <% if @server_id -%> server-id "<% @server_id.each_byte do |byte| -%><%= byte.to_s(base=16) %><% end -%>"; <% end -%> <% if @version -%> version "<%= @version %>"; <% end -%> <% if @dump_file -%> dump-file "<%= @dump_file %>"; <% end -%> <% if @statistics_file -%> statistics-file "<%= @statistics_file %>"; <% end -%> <% if @memstatistics_file -%> memstatistics-file "<%= @memstatistics_file %>"; <% end -%> <% if !@allow_query.empty? -%> allow-query { <%= @allow_query.join("; ") %>; }; <% end -%> <% if !@allow_query_cache.empty? -%> allow-query-cache { <%= @allow_query_cache.join("; ") %>; }; <% end -%> recursion <%= @recursion %>; <% if !@allow_recursion.empty? -%> allow-recursion { <%= @allow_recursion.join("; ") %>; }; <% end -%> <% if !@allow_transfer.empty? -%> allow-transfer { <%= @allow_transfer.join("; ") %>; }; <% end -%> <% if !@check_names.empty? -%> <% @check_names.each do |line| -%> check-names <%= line %>; <% end -%> <% end -%> <% if !@extra_options.empty? -%> <% @extra_options.sort_by {|key, value| key}.each do |key,value| -%> <%= key %> <%= value %>; <% end -%> <% end -%> dnssec-validation <%= @dnssec_validation %>; /* Path to ISC DLV key */ bindkeys-file "<%= @file_bindkeys %>"; }; statistics-channels { inet 127.0.0.1 port 8053 allow { 127.0.0.1; }; }; logging { channel main_log { file "/var/log/named/named.log" versions 3 size 5m; severity info; print-time yes; print-severity yes; print-category yes; }; category default{ main_log; }; category lame-servers { null; }; }; <% if !@views.empty? -%> <% @views.sort_by {|key,value| key}.each do |key,value| -%> <% valid_keys = %w(allow-notify allow-query allow-recursion allow-transfer allow-update-forwarding forwarders match-clients match-destinations match-recursive-only) -%> view "<%= key %>" { <% valid_keys.sort.each do |valid_key| -%> <% if value[valid_key] and !value[valid_key].empty? -%> <%= valid_key %> { <%= value[valid_key].join('; ') %>; }; <% end -%> <% end -%> <% if value['includes'] and !value['includes'].empty? -%> <% value['includes'].each do |filename| -%> include "<%= filename %>"; <% end -%> <% end -%> <% if value['zones'] and !value['zones'].empty? -%> /* View specific zones */ <% value['zones'].sort_by {|key, value| key}.each do |key,value| -%> zone "<%= key %>" IN { <% value.each do |line| -%> <%= line %>; <% end -%> }; <% end -%> <% end -%> <% if !@zones.empty? -%> /* Global zones */ <% @zones.sort_by {|key, value| key}.each do |key,value| -%> zone "<%= key %>" IN { <% value.each do |line| -%> <%= line %>; <% end -%> }; <% end -%> <% end -%> }; <% end -%> <% else -%><%# end views, start no views -%> <% if @recursion == 'yes' && @file_hint -%> zone "." IN { type hint; file "<%= @file_hint %>"; }; <% end -%> <% if !@zones.empty? -%> <% @zones.sort_by {|key, value| key}.each do |key,value| -%> zone "<%= key %>" IN { <% value.each do |line| -%> <%= line %>; <% end -%> }; <% end -%> <% end -%> <% if @recursion == 'yes' -%> include "<%= @file_rfc1912 %>"; <% end -%>

📄 References No matching references available

[qodo-pr-code-review-v2]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Avoid hardcoding the statistics channel The newly added BIND statistics channel is hardcoded and always enabled. It should be made optional and its parameters (address, port, ACLs) should be configurable. Examples: templates/named.conf.erb [102-104] statistics-channels { inet 127.0.0.1 port 8053 allow { 127.0.0.1; }; }; Solution Walkthrough: Before: // templates/named.conf.erb // ... options { // ... }; statistics-channels { inet 127.0.0.1 port 8053 allow { 127.0.0.1; }; }; logging { // ... } // ... After: # manifests/server/conf.pp define bind::server::conf ( # ... $statistics_channel_enable = false, $statistics_channel_options = 'inet 127.0.0.1 port 8053 allow { 127.0.0.1; }', # ... ) // templates/named.conf.erb // ... <% if @statistics_channel_enable -%> statistics-channels { <%= @statistics_channel_options %>; }; <% end -%> // ... Suggestion importance[1-10]: 8 __ Why: This is a significant design flaw; unconditionally enabling a new service endpoint like the statistics-channels without user control is poor practice for a reusable Puppet module.	Medium
Possible issue	Default file ensure to present Set a default value of 'present' for the $ensure parameter in the bind::server::file definition to avoid passing an invalid undef value to the file resource. manifests/server/file.pp [33-45] define bind::server::file ( ... - $ensure = undef, + $ensure = 'present', ... ) { ... file { "${zonedir}/${title}": ensure => $ensure, [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that undef is not a valid value for the ensure parameter and proposes a sensible default, improving the robustness of the defined type.	Low
Update