Restore behaviour of servers and pools parameters

Before the conversion to epp in voxpupuli#79, an array of servers/pools would get the `iburst` option set for each server/pool. This restores the old behaviour and gets rid of the `flatten` magic at the expense of several extra lines of template.
alexjfisher · Jan 4, 2021 · 2b78b11 · 2b78b11
1 parent cc390d7
commit 2b78b11
Show file tree

Hide file tree

Showing 5 changed files with 197 additions and 19 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -24,14 +24,6 @@ jobs:
     - rvm: 2.4.4
       bundler_args: --without system_tests development release
       env: PUPPET_VERSION="~> 5.0" CHECK=build DEPLOY_TO_FORGE=yes
-    - rvm: 2.5.3
-      bundler_args: --without development release
-      env: BEAKER_PUPPET_COLLECTION=puppet5 BEAKER_setfile=centos6-64 CHECK=beaker
-      services: docker
-    - rvm: 2.5.3
-      bundler_args: --without development release
-      env: BEAKER_PUPPET_COLLECTION=puppet6 BEAKER_setfile=centos6-64 CHECK=beaker
-      services: docker
     - rvm: 2.5.3
       bundler_args: --without development release
       env: BEAKER_PUPPET_COLLECTION=puppet5 BEAKER_setfile=centos7-64{image=centos:7.6.1810} CHECK=beaker

diff --git a/REFERENCE.md b/REFERENCE.md
@@ -297,10 +297,10 @@ Default value: ``undef``
 
 ##### `peers`
 
-Data type: `Any`
+Data type: `Variant[Hash,Array[Stdlib::Host]]`
 
 This selects the servers to use for NTP peers (symmetric association).
-It is an array of servers.
+It can be an array of peers or a hash of peers with their respective options.
 
 Default value: `[]`
 
@@ -309,7 +309,8 @@ Default value: `[]`
 Data type: `Variant[Hash,Array[Stdlib::Host]]`
 
 This selects the servers to use for NTP servers.  It can be an array of servers
-or a hash of servers to their respective options.
+or a hash of servers to their respective options. If an array is used, `iburst` will be configured for each server.
+If you don't want to use `iburst`, use a hash instead.
 
 Default value: `{
     '0.pool.ntp.org' => ['iburst'],
@@ -323,7 +324,7 @@ Default value: `{
 Data type: `Variant[Hash,Array[Stdlib::Fqdn]]`
 
 This is used to specify one or more *pools* of NTP servers to use instead of individual NTP servers.
-Similar to [`server`](#server), it can be an array of pools or a hash of pools to their respective options.
+Similar to [`server`](#server), it can be an array of pools, (using iburst), or a hash of pools to their respective options.
 See [pool](https://chrony.tuxfamily.org/doc/3.4/chrony.conf.html#pool)
 
 Default value: `{}`

diff --git a/manifests/init.pp b/manifests/init.pp
@@ -104,13 +104,14 @@
 #   Also see [`package_source`](#package_source).
 # @param peers
 #   This selects the servers to use for NTP peers (symmetric association).
-#   It is an array of servers.
+#   It can be an array of peers or a hash of peers with their respective options.
 # @param servers
 #   This selects the servers to use for NTP servers.  It can be an array of servers
-#   or a hash of servers to their respective options.
+#   or a hash of servers to their respective options. If an array is used, `iburst` will be configured for each server.
+#   If you don't want to use `iburst`, use a hash instead.
 # @param pools
 #   This is used to specify one or more *pools* of NTP servers to use instead of individual NTP servers.
-#   Similar to [`server`](#server), it can be an array of pools or a hash of pools to their respective options.
+#   Similar to [`server`](#server), it can be an array of pools, (using iburst), or a hash of pools to their respective options.
 #   See [pool](https://chrony.tuxfamily.org/doc/3.4/chrony.conf.html#pool)
 # @param refclocks
 #   This should be a Hash of hardware reference clock drivers to use.  They hash
@@ -201,7 +202,7 @@
   Optional[String] $package_source                                 = undef,
   Optional[String] $package_provider                               = undef,
   $refclocks                                                       = [],
-  $peers                                                           = [],
+  Variant[Hash,Array[Stdlib::Host]] $peers                         = [],
   Variant[Hash,Array[Stdlib::Host]] $servers                       = {
     '0.pool.ntp.org' => ['iburst'],
     '1.pool.ntp.org' => ['iburst'],

diff --git a/spec/classes/chrony_spec.rb b/spec/classes/chrony_spec.rb
@@ -16,6 +16,25 @@
       let(:facts) do
         facts
       end
+      let(:config_file) do
+        case facts[:osfamily]
+        when 'Archlinux', 'RedHat', 'Suse'
+          '/etc/chrony.conf'
+        else
+          '/etc/chrony/chrony.conf'
+        end
+      end
+      let(:keys_file) do
+        case facts[:osfamily]
+        when 'Archlinux', 'RedHat', 'Suse'
+          '/etc/chrony.keys'
+        else
+          '/etc/chrony/chrony.keys'
+        end
+      end
+      let(:config_file_contents) do
+        catalogue.resource('file', config_file).send(:parameters)[:content]
+      end
 
       context 'with defaults' do
         it { is_expected.to compile.with_all_deps }
@@ -237,6 +256,141 @@
         end
       end
 
+      describe 'servers' do
+        context 'by default' do
+          it do
+            expected_lines = [
+              'server 0.pool.ntp.org iburst',
+              'server 1.pool.ntp.org iburst',
+              'server 2.pool.ntp.org iburst',
+              'server 3.pool.ntp.org iburst'
+            ]
+            expect(config_file_contents.split("\n") & expected_lines).to eq(expected_lines)
+          end
+        end
+        context 'when servers is an array' do
+          let(:params) do
+            {
+              servers: ['ntp1.corp.com', 'ntp2.corp.com'],
+            }
+          end
+
+          it do
+            expected_lines = [
+              'server ntp1.corp.com iburst',
+              'server ntp2.corp.com iburst',
+            ]
+            expect(config_file_contents.split("\n") & expected_lines).to eq(expected_lines)
+          end
+        end
+        context 'when servers is an (unsorted) hash' do
+          let(:params) do
+            {
+              servers: {
+                'ntp3.corp.com' => [],
+                'ntp1.corp.com' => ['key 25', 'iburst'],
+                'ntp4.corp.com' => :undef,
+                'ntp2.corp.com' => ['key 25', 'iburst'],
+              }
+            }
+          end
+
+          it do
+            expected_lines = [
+              'server ntp1.corp.com key 25 iburst',
+              'server ntp2.corp.com key 25 iburst',
+              'server ntp3.corp.com',
+              'server ntp4.corp.com',
+            ]
+            expect(config_file_contents.split("\n") & expected_lines).to eq(expected_lines)
+          end
+        end
+      end
+
+      describe 'pools' do
+        context 'by default' do
+          it { expect(config_file_contents).not_to match(%r{^pool}) }
+        end
+        context 'when pools is an array' do
+          let(:params) do
+            {
+              pools: ['0.pool.ntp.org', '1.pool.ntp.org']
+            }
+          end
+
+          it do
+            expected_lines = [
+              'server 0.pool.ntp.org iburst',
+              'server 1.pool.ntp.org iburst',
+            ]
+            expect(config_file_contents.split("\n") & expected_lines).to eq(expected_lines)
+          end
+        end
+        context 'when pools is a hash' do
+          let(:params) do
+            {
+              pools: {
+                '3.pool.ntp.org' => [],
+                '0.pool.ntp.org' => ['maxsources 4'],
+                '1.pool.ntp.org' => ['maxsources 4'],
+                '2.pool.ntp.org' => ['maxsources 4'],
+              }
+            }
+          end
+
+          it do
+            expected_lines = [
+              'pool 0.pool.ntp.org maxsources 4',
+              'pool 1.pool.ntp.org maxsources 4',
+              'pool 2.pool.ntp.org maxsources 4',
+              'pool 3.pool.ntp.org',
+            ]
+            expect(config_file_contents.split("\n") & expected_lines).to eq(expected_lines)
+          end
+        end
+      end
+
+      describe 'peers' do
+        context 'by default' do
+          it { expect(config_file_contents).not_to match(%r{^peer}) }
+        end
+        context 'when peers is an array' do
+          let(:params) do
+            {
+              peers: ['peer1.example.com', 'peer2.example.com']
+            }
+          end
+
+          it do
+            expected_lines = [
+              'peer peer1.example.com',
+              'peer peer2.example.com',
+            ]
+            expect(config_file_contents.split("\n") & expected_lines).to eq(expected_lines)
+          end
+        end
+        context 'when peers is a hash' do
+          let(:params) do
+            {
+              peers: {
+                'peer1.example.com' => [],
+                'peer2.example.com' => ['maxpoll 6'],
+                'peer3.example.com' => :undef,
+              }
+            }
+          end
+
+          it do
+            expected_lines = [
+              'peer peer1.example.com',
+              'peer peer2.example.com maxpoll 6',
+              'peer peer3.example.com',
+            ]
+            expect(config_file_contents.split("\n") & expected_lines).to eq(expected_lines)
+          end
+        end
+      end
+
       context 'unmanaged chrony.keys file' do
         let(:params) do
           {

diff --git a/templates/chrony.conf.epp b/templates/chrony.conf.epp
@@ -1,12 +1,42 @@
 # NTP servers
+<% if $chrony::servers.is_a(Hash) { -%>
+<%   $chrony::servers.keys.sort.each |$server| { -%>
+<%     if $chrony::servers[$server] and !$chrony::servers[$server].empty { -%>
+server <%= $server %> <%= $chrony::servers[$server].join(' ') %>
+<% } else { -%>
+server <%= $server %>
+<% } -%>
+<% } -%>
+<% } else { -%>
 <% $chrony::servers.each |$server| { -%>
-server <%= $server.flatten.join(' ') %>
+server <%= $server %> iburst
+<% } -%>
+<% } -%>
+<% if $chrony::pools.is_a(Hash) { -%>
+<%   $chrony::pools.keys.sort.each |$pool| { -%>
+<%     if $chrony::pools[$pool] and !$chrony::pools[$pool].empty { -%>
+pool <%= $pool %> <%= $chrony::pools[$pool].join(' ') %>
+<% } else { -%>
+pool <%= $pool %>
+<% } -%>
 <% } -%>
+<% } else { -%>
 <% $chrony::pools.each |$pool| { -%>
-pool <%= $pool.flatten.join(' ') %>
+pool <%= $pool %> iburst
 <% } -%>
+<% } -%>
+<% if $chrony::peers.is_a(Hash) { -%>
+<%   $chrony::peers.keys.sort.each |$peer| { -%>
+<%     if $chrony::peers[$peer] and !$chrony::peers[$peer].empty { -%>
+peer <%= $peer %> <%= $chrony::peers[$peer].join(' ') %>
+<% } else { -%>
+peer <%= $peer %>
+<% } -%>
+<% } -%>
+<% } else { -%>
 <% $chrony::peers.each |$peer| { -%>
-peer <%= $peer.flatten.join(' ') %>
+peer <%= $peer %>
+<% } -%>
 <% } -%>
 <% if $chrony::stratumweight { -%>