LGTM On Tue, Apr 28, 2015 at 4:10 PM, Helga Velroyen <[email protected]> wrote:
> (+ 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): > > Hrvoje Ribicic Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
