+list On Fri, 27 Nov 2015 at 14:46 Lisa Velden <[email protected]> wrote:
> On Tue, Nov 24, 2015 at 3:15 PM, 'Helga Velroyen' via ganeti-devel < > [email protected]> wrote: > >> There was a subtle bug in the unit test of backend.py >> which was masking another subtle bug in the test framework >> in testutils_ssh.py. >> >> As relict from some previous refactoring, the ssh.py >> functions assume that there can be more than one public >> key per node. The testutils so far assume there is only >> one key per node and due to a bug, this cancelled out >> nicely and was not found so far. >> >> As we actually only have one key per node, the elegant >> thing to do would be to adapt ssh.py rather than the >> testutils, but that will break the interface of the >> ssh_update.py tool. Since we would rather not do that >> in a stable, branch, this patch adapts the testutils. >> The adaption of the ssh.py will be done in a newer >> branch then. >> > > Side note: could you make sure to file an issue for that so that it will > not be forgotten? > > >> >> Additionally, this patch also sprinkles assertions >> everywhere to ensure finding these kind of type messups >> sooner. >> >> Signed-off-by: Helga Velroyen <[email protected]> >> --- >> test/py/ganeti.backend_unittest.py | 5 +- >> test/py/testutils_ssh.py | 113 >> +++++++++++++++++++++++++++++++------ >> 2 files changed, 99 insertions(+), 19 deletions(-) >> >> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ >> ganeti.backend_unittest.py >> index 43d2dde..35ea9f4 100755 >> --- a/test/py/ganeti.backend_unittest.py >> +++ b/test/py/ganeti.backend_unittest.py >> @@ -1031,6 +1031,8 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> self._ssh_file_manager.GetAllNodeNames >> self._ssconf_mock.GetOnlineNodeList.side_effect = \ >> self._ssh_file_manager.GetAllNodeNames >> + self._ssconf_mock.GetMasterNode.side_effect = \ >> + self._ssh_file_manager.GetMasterNodeName >> >> def _TearDownTestData(self): >> os.remove(self._pub_key_file) >> @@ -1579,8 +1581,9 @@ class >> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): >> noded_cert_file=self.noded_cert_file, >> run_cmd_fn=self._run_cmd_mock) >> >> + master_node = self._ssh_file_manager.GetMasterNodeName() >> for node in self._all_nodes: >> - if node == new_node_name: >> + if node in [new_node_name, master_node]: >> self.assertTrue(self._ssh_file_manager.NodeHasAuthorizedKey( >> node, new_node_key)) >> else: >> diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py >> index dd14303..e4d76c1 100644 >> --- a/test/py/testutils_ssh.py >> +++ b/test/py/testutils_ssh.py >> @@ -77,6 +77,9 @@ class FakeSshFileManager(object): >> # 'RunCommand' has already carried out. >> self._retries = {} >> >> + self._AssertTypePublicKeys() >> + self._AssertTypeAuthorizedKeys() >> + >> _NodeInfo = namedtuple( >> "NodeInfo", >> ["uuid", >> @@ -110,6 +113,9 @@ class FakeSshFileManager(object): >> >> self._all_node_data[name] = self._NodeInfo(uuid, key, pot_mc, mc, >> master) >> >> + self._AssertTypePublicKeys() >> + self._AssertTypeAuthorizedKeys() >> + >> def _FillPublicKeyOfOneNode(self, receiving_node_name): >> node_info = self._all_node_data[receiving_node_name] >> # Nodes which are not potential master candidates receive no keys >> @@ -117,7 +123,7 @@ class FakeSshFileManager(object): >> return >> for node_info in self._all_node_data.values(): >> if node_info.is_potential_master_candidate: >> - self._public_keys[receiving_node_name][node_info.uuid] = >> node_info.key >> + self._public_keys[receiving_node_name][node_info.uuid] = >> [node_info.key] >> >> def _FillAuthorizedKeyOfOneNode(self, receiving_node_name): >> for node_info in self._all_node_data.values(): >> @@ -142,6 +148,8 @@ class FakeSshFileManager(object): >> self._FillPublicKeyOfOneNode(node) >> self._FillAuthorizedKeyOfOneNode(node) >> self._SetMasterNodeName() >> + self._AssertTypePublicKeys() >> + self._AssertTypeAuthorizedKeys() >> >> def SetMaxRetries(self, node_name, retries): >> """Set the number of unsuccessful retries of 'RunCommand' per node. >> @@ -205,7 +213,7 @@ class FakeSshFileManager(object): >> >> This is necessary when testing to add new nodes to the cluster. >> Otherwise >> this new node's state would not be evaluated properly with the >> assertion >> - fuctions. >> + functions. >> >> @type name: string >> @param name: name of the new node >> @@ -228,6 +236,8 @@ class FakeSshFileManager(object): >> self._authorized_keys[name].add(key) >> if name not in self._public_keys: >> self._public_keys[name] = {} >> + self._AssertTypePublicKeys() >> + self._AssertTypeAuthorizedKeys() >> >> def NodeHasPublicKey(self, file_node_name, key_node_uuid, key): >> """Checks whether a node has another node's public key. >> @@ -355,6 +365,37 @@ class FakeSshFileManager(object): >> self.AssertNodeSetOnlyHasPublicKey( >> potential_master_candidates, query_node_uuid, query_node_key) >> >> + def _AssertTypePublicKeys(self): >> + """Asserts that the public key dictionary has the right types. >> + >> + This is helpful as an invariant that shall not be violated during the >> + tests due to type errors. >> + >> + """ >> + assert isinstance(self._public_keys, dict) >> + for node_file, pub_keys in self._public_keys.items(): >> + assert isinstance(node_file, str) >> + assert isinstance(pub_keys, dict) >> + for node_key, keys in pub_keys.items(): >> + assert isinstance(node_key, str) >> + assert isinstance(keys, list) >> + for key in keys: >> + assert isinstance(key, str) >> + >> + def _AssertTypeAuthorizedKeys(self): >> + """Asserts that the authorized keys dictionary has the right types. >> + >> + This is useful to check as an invariant that is not supposed to be >> violated >> + during the tests. >> + >> + """ >> + assert isinstance(self._authorized_keys, dict) >> + for node_file, auth_keys in self._authorized_keys.items(): >> + assert isinstance(node_file, str) >> + assert isinstance(auth_keys, set) >> + for key in auth_keys: >> + assert isinstance(key, str) >> + >> # Disabling a pylint warning about unused parameters. Those need >> # to be here to properly mock the real methods. >> # pylint: disable=W0613 >> @@ -392,52 +433,77 @@ class FakeSshFileManager(object): >> def _EnsureAuthKeyFile(self, file_node_name): >> if file_node_name not in self._authorized_keys: >> self._authorized_keys[file_node_name] = set() >> + self._AssertTypePublicKeys() >> + self._AssertTypeAuthorizedKeys() >> >> - def _AddAuthorizedKey(self, file_node_name, ssh_key): >> + def _AddAuthorizedKeys(self, file_node_name, ssh_keys): >> + """Mocks adding the given keys to the authorized_keys file.""" >> + assert isinstance(ssh_keys, list) >> self._EnsureAuthKeyFile(file_node_name) >> - self._authorized_keys[file_node_name].add(ssh_key) >> + for key in ssh_keys: >> + self._authorized_keys[file_node_name].add(key) >> + self._AssertTypePublicKeys() >> + self._AssertTypeAuthorizedKeys() >> + >> + def _RemoveAuthorizedKeys(self, file_node_name, keys): >> + """Mocks removing the keys from authorized_keys on the given node. >> >> - def _RemoveAuthorizedKey(self, file_node_name, key): >> + @param keys: list of ssh keys >> + @type keys: list of strings >> + >> + """ >> self._EnsureAuthKeyFile(file_node_name) >> self._authorized_keys[file_node_name] = \ >> - set([k for k in self._authorized_keys[file_node_name] if k != >> key]) >> + set([k for k in self._authorized_keys[file_node_name] if k not >> in keys]) >> + self._AssertTypeAuthorizedKeys() >> >> def _HandleAuthorizedKeys(self, instructions, node): >> (action, authorized_keys) = instructions >> - ssh_keys = authorized_keys.values() >> + ssh_key_sets = authorized_keys.values() >> if action == constants.SSHS_ADD: >> - for ssh_key in ssh_keys: >> - self._AddAuthorizedKey(node, ssh_key) >> + for ssh_keys in ssh_key_sets: >> + self._AddAuthorizedKeys(node, ssh_keys) >> elif action == constants.SSHS_REMOVE: >> - for ssh_key in ssh_keys: >> - self._RemoveAuthorizedKey(node, ssh_key) >> + for ssh_keys in ssh_key_sets: >> + self._RemoveAuthorizedKeys(node, ssh_keys) >> else: >> raise Exception("Unsupported action: %s" % action) >> + self._AssertTypeAuthorizedKeys() >> >> def _EnsurePublicKeyFile(self, file_node_name): >> if file_node_name not in self._public_keys: >> self._public_keys[file_node_name] = {} >> + self._AssertTypePublicKeys() >> >> def _ClearPublicKeys(self, file_node_name): >> self._public_keys[file_node_name] = {} >> + self._AssertTypePublicKeys() >> >> def _OverridePublicKeys(self, ssh_keys, file_node_name): >> + assert isinstance(ssh_keys, dict) >> self._ClearPublicKeys(file_node_name) >> for key_node_uuid, node_keys in ssh_keys.items(): >> + assert isinstance(node_keys, list) >> if key_node_uuid in self._public_keys[file_node_name]: >> raise Exception("Duplicate node in ssh_update data.") >> self._public_keys[file_node_name][key_node_uuid] = node_keys >> + self._AssertTypePublicKeys() >> >> def _ReplaceOrAddPublicKeys(self, public_keys, file_node_name): >> + assert isinstance(public_keys, dict) >> self._EnsurePublicKeyFile(file_node_name) >> for key_node_uuid, keys in public_keys.items(): >> + assert isinstance(keys, list) >> self._public_keys[file_node_name][key_node_uuid] = keys >> + self._AssertTypePublicKeys() >> >> def _RemovePublicKeys(self, public_keys, file_node_name): >> + assert isinstance(public_keys, dict) >> self._EnsurePublicKeyFile(file_node_name) >> for key_node_uuid, _ in public_keys.items(): >> if key_node_uuid in self._public_keys[file_node_name]: >> self._public_keys[file_node_name][key_node_uuid] = [] >> + self._AssertTypePublicKeys() >> >> def _HandlePublicKeys(self, instructions, node): >> (action, public_keys) = instructions >> @@ -453,6 +519,7 @@ class FakeSshFileManager(object): >> self._ClearPublicKeys(node) >> else: >> raise Exception("Unsupported action: %s." % action) >> + self._AssertTypePublicKeys() >> >> # pylint: disable=W0613 >> def AddAuthorizedKeys(self, file_obj, keys): >> @@ -464,9 +531,10 @@ class FakeSshFileManager(object): >> @see: C{ssh.AddAuthorizedKeys} >> >> """ >> + assert isinstance(keys, list) >> assert self._master_node_name >> - for key in keys: >> - self._AddAuthorizedKey(self._master_node_name, key) >> + self._AddAuthorizedKeys(self._master_node_name, keys) >> + self._AssertTypeAuthorizedKeys() >> >> def RemoveAuthorizedKeys(self, file_name, keys): >> """Emulates ssh.RemoveAuthorizeKeys on the master node. >> @@ -477,9 +545,10 @@ class FakeSshFileManager(object): >> @see: C{ssh.RemoveAuthorizedKeys} >> >> """ >> + assert isinstance(keys, list) >> assert self._master_node_name >> - for key in keys: >> - self._RemoveAuthorizedKey(self._master_node_name, key) >> + self._RemoveAuthorizedKeys(self._master_node_name, keys) >> + self._AssertTypeAuthorizedKeys() >> >> def AddPublicKey(self, new_uuid, new_key, **kwargs): >> """Emulates ssh.AddPublicKey on the master node. >> @@ -491,8 +560,10 @@ class FakeSshFileManager(object): >> >> """ >> assert self._master_node_name >> - key_dict = {new_uuid: new_key} >> + assert isinstance(new_key, str) >> + key_dict = {new_uuid: [new_key]} >> self._ReplaceOrAddPublicKeys(key_dict, self._master_node_name) >> + self._AssertTypePublicKeys() >> >> def RemovePublicKey(self, target_uuid, **kwargs): >> """Emulates ssh.RemovePublicKey on the master node. >> @@ -506,6 +577,7 @@ class FakeSshFileManager(object): >> assert self._master_node_name >> key_dict = {target_uuid: []} >> self._RemovePublicKeys(key_dict, self._master_node_name) >> + self._AssertTypePublicKeys() >> >> def QueryPubKeyFile(self, target_uuids, **kwargs): >> """Emulates ssh.QueryPubKeyFile on the master node. >> @@ -516,6 +588,7 @@ class FakeSshFileManager(object): >> @see: C{ssh.QueryPubKey} >> >> """ >> + >> > > Why have you added the empty line here? > No particular reason. Will fix that with this interdiff: diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py index d00b3c4..59511b0 100644 --- a/test/py/testutils_ssh.py +++ b/test/py/testutils_ssh.py @@ -626,7 +626,6 @@ class FakeSshFileManager(object): @see: C{ssh.QueryPubKey} """ - assert self._master_node_name all_keys = target_uuids is None if all_keys: > > >> assert self._master_node_name >> all_keys = target_uuids is None >> if all_keys: >> @@ -524,10 +597,11 @@ class FakeSshFileManager(object): >> if isinstance(target_uuids, str): >> target_uuids = [target_uuids] >> result_dict = {} >> - for key_node_uuid, key in \ >> + for key_node_uuid, keys in \ >> self._public_keys[self._master_node_name].items(): >> if key_node_uuid in target_uuids: >> - result_dict[key_node_uuid] = key >> + result_dict[key_node_uuid] = keys >> + self._AssertTypePublicKeys() >> return result_dict >> >> def ReplaceNameByUuid(self, node_uuid, node_name, **kwargs): >> @@ -539,9 +613,12 @@ class FakeSshFileManager(object): >> @see: C{ssh.ReplacenameByUuid} >> >> """ >> + assert isinstance(node_uuid, str) >> + assert isinstance(node_name, str) >> assert self._master_node_name >> if node_name in self._public_keys[self._master_node_name]: >> self._public_keys[self._master_node_name][node_uuid] = \ >> self._public_keys[self._master_node_name][node_name][:] >> del self._public_keys[self._master_node_name][node_name] >> + self._AssertTypePublicKeys() >> # pylint: enable=W0613 >> -- >> 2.6.0.rc2.230.g3dd15c0 >> >> > Rest LGTM > > > -- > Lisa Velden > Software Engineer > [email protected] > > Google Germany GmbH > Dienerstraße 12 > 80331 München > > Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > -- Helga Velroyen Software Engineer [email protected] Google Germany GmbH Dienerstraße 12 80331 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks.
