(+ list) On Tue, 28 Apr 2015 at 16:02 Hrvoje Ribicic <[email protected]> wrote:
> LGTM, with nitpicks > > On Thu, Apr 23, 2015 at 5:30 PM, 'Helga Velroyen' via ganeti-devel < > [email protected]> wrote: > >> This patch adds two helper function to the SSH file manager >> which given a set of nodes check for the existence of the >> public/authorized key on those nodes *and* for the >> non-existance on all other nodes. >> >> Signed-off-by: Helga Velroyen <[email protected]> >> --- >> test/py/ganeti.backend_unittest.py | 42 +++++++--------------- >> test/py/testutils_ssh.py | 74 >> +++++++++++++++++++++++++------------- >> 2 files changed, 63 insertions(+), 53 deletions(-) >> >> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ >> ganeti.backend_unittest.py >> index 51b4dd2..eddabbe 100755 >> --- a/test/py/ganeti.backend_unittest.py >> +++ b/test/py/ganeti.backend_unittest.py >> @@ -1380,13 +1380,10 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - for node in self._all_nodes: >> - if node in self._online_nodes: >> - self.assertFalse(self._ssh_file_manager.NodeHasAuthorizedKey( >> - node, node_key)) >> - else: >> - self.assertTrue(self._ssh_file_manager.NodeHasAuthorizedKey( >> - node, node_key)) >> + offline_nodes = [node for node in self._all_nodes >> + if node not in self._online_nodes] >> + self._ssh_file_manager.AssertNodeSetOnlyHasAuthorizedKey( >> + offline_nodes, node_key) >> >> def testAddKeySuccessfullyOnNewNodeWithRetries(self): >> """Tests adding a new node's key when updating that node takes >> retries. >> @@ -1527,14 +1524,11 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - for node in self._all_nodes: >> - if node == other_node_name: >> - self.assertFalse(self._ssh_file_manager.NodeHasAuthorizedKey( >> - node, new_node_key)) >> - else: >> - self.assertTrue(self._ssh_file_manager.NodeHasAuthorizedKey( >> - node, new_node_key)) >> - >> + rest_nodes = [node for node in self._all_nodes >> + if node != other_node_name] >> + rest_nodes.append(new_node_name) >> + self._ssh_file_manager.AssertNodeSetOnlyHasAuthorizedKey( >> + rest_nodes, new_node_key) >> self.assertTrue([error_msg for (node, error_msg) in node_errors >> if node == other_node_name]) >> >> @@ -1596,13 +1590,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> pub_key_file=self._pub_key_file, ssconf_store=self._ssconf_mock, >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - for node in self._all_nodes: >> - if node == other_node_name: >> - self.assertTrue(self._ssh_file_manager.NodeHasAuthorizedKey( >> - node, node_key)) >> - else: >> - self.assertFalse(self._ssh_file_manager.NodeHasAuthorizedKey( >> - node, node_key)) >> + self._ssh_file_manager.AssertNodeSetOnlyHasAuthorizedKey( >> + [other_node_name], node_key) >> self.assertTrue([error_msg for (node, error_msg) in error_msgs >> if node == other_node_name]) >> >> @@ -1657,13 +1646,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> pub_key_file=self._pub_key_file, ssconf_store=self._ssconf_mock, >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> - for node in self._all_nodes: >> - if node == node_name: >> - self.assertTrue(self._ssh_file_manager.NodeHasAuthorizedKey( >> - node, node_key)) >> - else: >> - self.assertFalse(self._ssh_file_manager.NodeHasAuthorizedKey( >> - node, node_key)) >> + self._ssh_file_manager.AssertNodeSetOnlyHasAuthorizedKey( >> + [node_name], node_key) >> self.assertTrue([error_msg for (node, error_msg) in error_msgs >> if node == node_name]) >> >> diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py >> index 53dd8e0..8735e7d 100644 >> --- a/test/py/testutils_ssh.py >> +++ b/test/py/testutils_ssh.py >> @@ -253,6 +253,25 @@ class FakeSshFileManager(object): >> """ >> return key in self._authorized_keys[file_node_name] >> >> + def AssertNodeSetOnlyHasAuthorizedKey(self, node_set, query_node_key): >> + """Check if nodes in the given set only have a particular authorized >> key. >> + >> + @type node_set: list of strings >> + @param node_set: list of nodes who are supposed to have the key >> + @type query_node_key: string >> + @param query_node_key: key which is looked for >> + >> + """ >> + for node_name in self._all_node_data.keys(): >> + if node_name in node_set: >> + if not self.NodeHasAuthorizedKey(node_name, query_node_key): >> + raise Exception("Node '%s' does not have authorized key '%s'." >> + % (node_name, query_node_key)) >> + else: >> + if self.NodeHasAuthorizedKey(node_name, query_node_key): >> + raise Exception("Node '%s' does have authorized key '%s' >> although it" >> > > s/does have/has/ > ACK > > >> + " should not have." % (node_name, >> query_node_key)) >> > > s/ have// > ACK > > >> + >> def AssertAllNodesHaveAuthorizedKey(self, key): >> """Check if all nodes have a particular key in their auth. keys file. >> >> @@ -261,10 +280,7 @@ class FakeSshFileManager(object): >> @raise Exception: if a node does not have the authorized key. >> >> """ >> - for name in self._all_node_data.keys(): >> - if key not in self._authorized_keys[name]: >> - raise Exception("Node '%s' does not have the key '%s' in its" >> - " 'authorized_keys' file." % (name, key)) >> + self.AssertNodeSetOnlyHasAuthorizedKey(self._all_node_data.keys(), >> key) >> >> def AssertNoNodeHasAuthorizedKey(self, key): >> """Check if none of the nodes has a particular key in their auth. >> keys file. >> @@ -274,11 +290,32 @@ class FakeSshFileManager(object): >> @raise Exception: if a node *does* have the authorized key. >> >> """ >> - for name in self._all_node_data.keys(): >> - node_auth_keys = self._authorized_keys[name] >> - if key in node_auth_keys: >> - raise Exception("Node '%s' does have the key '%s' in its" >> - " 'authorized_keys' file." % (name, key)) >> + self.AssertNodeSetOnlyHasAuthorizedKey([], key) >> + >> + def AssertNodeSetOnlyHasPublicKey(self, node_set, query_node_uuid, >> + query_node_key): >> + """Check if nodes in the given set only have a particular public key. >> + >> + @type node_set: list of strings >> + @param node_set: list of nodes who are supposed to have the key >> + @type query_node_uuid: string >> + @param query_node_uuid: uuid of the node whose key is looked for >> + @type query_node_key: string >> + @param query_node_key: key which is looked for >> + >> + """ >> + for node_name in self._all_node_data.keys(): >> + if node_name in node_set: >> + if not self.NodeHasPublicKey(node_name, query_node_uuid, >> + query_node_key): >> + raise Exception("Node '%s' does not have public key '%s' of >> node" >> + " '%s'." % (node_name, query_node_key, >> + query_node_uuid)) >> + else: >> + if self.NodeHasPublicKey(node_name, query_node_uuid, >> query_node_key): >> + raise Exception("Node '%s' does have public key '%s' of node" >> + " '%s' although it should not have." >> > > Correct error message as per my previous comment. > ACK > > >> + % (node_name, query_node_key, query_node_uuid)) >> >> def AssertNoNodeHasPublicKey(self, uuid, key): >> """Check if none of the nodes have the given public key in their >> file. >> @@ -288,11 +325,7 @@ class FakeSshFileManager(object): >> @raise Exception: if a node *does* have the public key. >> >> """ >> - for name in self._all_node_data.keys(): >> - node_pub_keys = self._public_keys[name] >> - if (uuid, key) in node_pub_keys.items(): >> - raise Exception("Node '%s' does have public key '%s' of node >> '%s'" >> - % (name, key, uuid)) >> + self.AssertNodeSetOnlyHasPublicKey([], uuid, key) >> >> def AssertPotentialMasterCandidatesOnlyHavePublicKey(self, >> query_node_name): >> """Checks if the node's key is on all potential master candidates >> only. >> @@ -311,16 +344,9 @@ class FakeSshFileManager(object): >> """ >> query_node_uuid, query_node_key, _, _, _ = \ >> self._all_node_data[query_node_name] >> - for name, (_, _, pot_mc, _, _) in self._all_node_data.items(): >> - has_key = self.NodeHasPublicKey(name, query_node_uuid, >> query_node_key) >> - if pot_mc: >> - if not has_key: >> - raise Exception("Potential master candidate '%s' does not have >> the" >> - " key.") >> - else: >> - if has_key: >> - raise Exception("Normal node (not potential master candidate) >> '%s'" >> - " does have the key, although it should not >> have.") >> + potential_master_candidates = >> self.GetAllPotentialMasterCandidateNodeNames() >> + self.AssertNodeSetOnlyHasPublicKey( >> + potential_master_candidates, query_node_uuid, query_node_key) >> >> # Disabling a pylint warning about unused parameters. Those need >> # to be here to properly mock the real methods. >> -- >> 2.2.0.rc0.207.ga3a616c >> >> FYI, interdiff: diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py index 8735e7d..b862fea 100644 --- a/test/py/testutils_ssh.py +++ b/test/py/testutils_ssh.py @@ -269,8 +269,8 @@ class FakeSshFileManager(object): % (node_name, query_node_key)) else: if self.NodeHasAuthorizedKey(node_name, query_node_key): - raise Exception("Node '%s' does have authorized key '%s' although it" - " should not have." % (node_name, query_node_key)) + raise Exception("Node '%s' has authorized key '%s' although it" + " should not." % (node_name, query_node_key)) def AssertAllNodesHaveAuthorizedKey(self, key): """Check if all nodes have a particular key in their auth. keys file. @@ -313,8 +313,8 @@ class FakeSshFileManager(object): query_node_uuid)) else: if self.NodeHasPublicKey(node_name, query_node_uuid, query_node_key): - raise Exception("Node '%s' does have public key '%s' of node" - " '%s' although it should not have." + raise Exception("Node '%s' has public key '%s' of node" + " '%s' although it should not." % (node_name, query_node_key, query_node_uuid)) def AssertNoNodeHasPublicKey(self, uuid, key):
