From 2b78b115095724cfb7a6910e2d1d42a0be4b5f6c Mon Sep 17 00:00:00 2001 From: Alexander Fisher Date: Mon, 4 Jan 2021 14:42:16 +0000 Subject: [PATCH] Restore behaviour of `servers` and `pools` parameters Before the conversion to epp in https://github.com/voxpupuli/puppet-chrony/pull/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. --- .travis.yml | 8 -- REFERENCE.md | 9 ++- manifests/init.pp | 9 ++- spec/classes/chrony_spec.rb | 154 ++++++++++++++++++++++++++++++++++++ templates/chrony.conf.epp | 36 ++++++++- 5 files changed, 197 insertions(+), 19 deletions(-) diff --git a/.travis.yml b/.travis.yml index da5762b..a991c32 100644 --- 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 index 568629f..ac24c42 100644 --- 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 index e2050ce..69f9809 100644 --- 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 index 5267803..6c44d97 100644 --- 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 index 1970f6d..f67fad0 100644 --- 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 { -%>