From 302162ad1cbe4b7746f9de0f48d23d7c399f44c2 Mon Sep 17 00:00:00 2001 From: Ben Roberts Date: Tue, 28 Jun 2022 22:46:05 +0100 Subject: [PATCH] Converge quorum member auth The current code for authenticating to quorum members generates a change event for every puppet run due to the exec having not being `refreshonly` or having any conditionals. This cases the pcsd tokens file to be updated regularly as well. The proposed change splits up the authentication so that it's done once per quorum member, rather than doing them all in one go. It also adds a conditional check to see if any authentication token is already present in the pcsd tokens file, and skips the exec if so. This is convergent, but comes with two minor costs: - if quorum member hostnames overlap (e.g. `foo` and `foobar`), then the condition will match perhaps incorrectly and one hostname may not get added - If for any reason the authentication token becomes invalid, puppet will not correct it, and manual intervention will be required (in the form of a `pcs cluster auth` or `pcs host auth` command) A `pcs host deauth` command would be handled correctly, and puppet would do a corrective re-auth. Fixes #500 --- manifests/init.pp | 26 ++++++++-------- spec/classes/corosync_spec.rb | 56 ++++++++++++++++++++++------------- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/manifests/init.pp b/manifests/init.pp index 1b4fe15f..915c2fdf 100644 --- a/manifests/init.pp +++ b/manifests/init.pp @@ -597,17 +597,17 @@ default => 'pcs host auth', } - # Attempt to authorize all members. The command will return successfully - # if they were already authenticated so it's safe to run every time this - # is applied. - # TODO - make it run only once - exec { 'authorize_members': - command => "${pcs_auth_command} ${node_string} ${auth_credential_string}", - path => $exec_path, - require => [ - Service['pcsd'], - User['hacluster'], - ], + # Attempt to authorize each member + $quorum_members.each |$node| { + exec { "authorize_member_${node}": + command => "${pcs_auth_command} ${node} ${auth_credential_string}", + unless => "grep '${node}' /var/lib/pcsd/tokens", + path => $exec_path, + require => [ + Service['pcsd'], + User['hacluster'], + ], + } } } @@ -636,7 +636,7 @@ command => "pcs cluster setup --force ${pcs_cluster_setup_namearg} ${cluster_name} ${node_string}", path => $exec_path, onlyif => 'test ! -f /etc/corosync/corosync.conf', - require => Exec['authorize_members'], + require => Exec[$quorum_members.map |$node| { "authorize_member_${node}" }], } # We need to do this so the temporary cluster doesn't delete our authkey if $enable_secauth { @@ -655,7 +655,7 @@ onlyif => $qdevice_token_check, require => [ Package[$package_quorum_device], - Exec['authorize_members'], + Exec[$quorum_members.map |$node| { "authorize_member_${node}" }], Exec['pcs_cluster_temporary'], ], } diff --git a/spec/classes/corosync_spec.rb b/spec/classes/corosync_spec.rb index ea87f527..e696c7bf 100644 --- a/spec/classes/corosync_spec.rb +++ b/spec/classes/corosync_spec.rb @@ -783,7 +783,7 @@ let(:node) { 'node2.test.org' } it 'does not perform the auth' do - is_expected.not_to contain_exec('authorize_members') + is_expected.not_to contain_exec('authorize_member_node2.test.org') end end @@ -805,14 +805,17 @@ end it 'authorizes all nodes' do - is_expected.to contain_exec('authorize_members').with( - command: "pcs #{auth_command} node1.test.org node2.test.org node3.test.org -u hacluster -p some-secret-sauce", - path: '/sbin:/bin:/usr/sbin:/usr/bin', - require: [ - 'Service[pcsd]', - 'User[hacluster]' - ] - ) + ['node1.test.org', 'node2.test.org', 'node3.test.org'].each do |node| + is_expected.to contain_exec("authorize_member_#{node}").with( + command: "pcs #{auth_command} #{node} -u hacluster -p some-secret-sauce", + unless: "grep '#{node}' /var/lib/pcsd/tokens", + path: '/sbin:/bin:/usr/sbin:/usr/bin', + require: [ + 'Service[pcsd]', + 'User[hacluster]' + ] + ) + end end end @@ -836,15 +839,18 @@ let(:facts) { override_facts(super(), networking: { ip: '192.168.0.10' }) } - it 'match ip and auth nodes by member names' do - is_expected.to contain_exec('authorize_members').with( - command: "pcs #{auth_command} 192.168.0.10 192.168.0.12 192.168.0.13 -u hacluster -p some-secret-sauce", - path: '/sbin:/bin:/usr/sbin:/usr/bin', - require: [ - 'Service[pcsd]', - 'User[hacluster]' - ] - ) + ['192.168.0.10', '192.168.0.12', '192.168.0.13'].each do |node| + it 'match ip and auth nodes by member names' do + is_expected.to contain_exec("authorize_member_#{node}").with( + command: "pcs #{auth_command} #{node} -u hacluster -p some-secret-sauce", + unless: "grep '#{node}' /var/lib/pcsd/tokens", + path: '/sbin:/bin:/usr/sbin:/usr/bin', + require: [ + 'Service[pcsd]', + 'User[hacluster]' + ] + ) + end end context 'where the auth-node IP is not the default IP' do @@ -864,7 +870,9 @@ end it 'still detects that this is the auth-node' do - is_expected.to contain_exec('authorize_members') + is_expected.to contain_exec('authorize_member_192.168.0.10') + is_expected.to contain_exec('authorize_member_192.168.0.12') + is_expected.to contain_exec('authorize_member_192.168.0.13') end end end @@ -1017,7 +1025,11 @@ command: "pcs cluster setup --force #{cluster_name_arg} cluster_test node1.test.org node2.test.org node3.test.org", path: '/sbin:/bin:/usr/sbin:/usr/bin', onlyif: 'test ! -f /etc/corosync/corosync.conf', - require: 'Exec[authorize_members]' + require: [ + 'Exec[authorize_member_node1.test.org]', + 'Exec[authorize_member_node2.test.org]', + 'Exec[authorize_member_node3.test.org]', + ] ) end @@ -1028,7 +1040,9 @@ onlyif: 'test 0 -ne $(grep quorum1.test.org /var/lib/pcsd/tokens >/dev/null 2>&1; echo $?)', require: [ 'Package[corosync-qdevice]', - 'Exec[authorize_members]', + 'Exec[authorize_member_node1.test.org]', + 'Exec[authorize_member_node2.test.org]', + 'Exec[authorize_member_node3.test.org]', 'Exec[pcs_cluster_temporary]' ] )