Hi! On Fri, 17 Apr 2015 at 15:39 Petr Pudlak <[email protected]> wrote:
> On Thu, Apr 16, 2015 at 05:31:56PM +0200, 'Helga Velroyen' via > ganeti-devel wrote: > >Testing the backend functions which update SSH keys is a > >pain and maintaining the tests even more. Therefore, this > >patch introduces a manager for all SSH key files of a > >clusters (ganeti_pub_keys and authorized_keys). It > >emulates all operations on these files for all nodes in > >the cluster. > > > >This has the following advantages: > >- One can query the state of the entire cluster in a > > consisten way, for example "Do all nodes have this > > master candidates' key?" instead of tediously evaluating > > a history of mock calls. > >- The file manager emulates both local changes in the > > master nodes' key files and changes on other nodes' > > key files using the ssh_update tool. This way, the > > state of the cluster ssh files is managed consistently > > no matter by what mechanism they were changed. > >- The file manager offers a couple of convenience > > functions to set up the test data and to query their > > state after test operation. > > > >Note that this might look like a lot of code, but it > >vastly simplifies the current unit tests and it will > >make future tests (for example for invalid calls) much > >more easier. As it is a test utility, it is properly > >documented to make it maintainable. > > > >Signed-off-by: Helga Velroyen <[email protected]> > >--- > > Makefile.am | 1 + > > test/py/testutils_ssh.py | 491 > +++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 492 insertions(+) > > create mode 100644 test/py/testutils_ssh.py > > > >diff --git a/Makefile.am b/Makefile.am > >index 7312ea1..518eb2b 100644 > >--- a/Makefile.am > >+++ b/Makefile.am > >@@ -1842,6 +1842,7 @@ python_test_support = \ > > test/py/__init__.py \ > > test/py/lockperf.py \ > > test/py/testutils.py \ > >+ test/py/testutils_ssh.py \ > > test/py/mocks.py \ > > test/py/cmdlib/__init__.py \ > > test/py/cmdlib/testsupport/__init__.py \ > >diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py > >new file mode 100644 > >index 0000000..1e7edf8 > >--- /dev/null > >+++ b/test/py/testutils_ssh.py > >@@ -0,0 +1,491 @@ > >+#!/usr/bin/python > >+# > >+ > >+# Copyright (C) 2010, 2013, 2015 Google Inc. > >+# All rights reserved. > >+# > >+# Redistribution and use in source and binary forms, with or without > >+# modification, are permitted provided that the following conditions are > >+# met: > >+# > >+# 1. Redistributions of source code must retain the above copyright > notice, > >+# this list of conditions and the following disclaimer. > >+# > >+# 2. Redistributions in binary form must reproduce the above copyright > >+# notice, this list of conditions and the following disclaimer in the > >+# documentation and/or other materials provided with the distribution. > >+# > >+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS > >+# IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > >+# TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A > PARTICULAR > >+# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR > >+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, > >+# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > >+# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR > >+# PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > >+# LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING > >+# NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS > >+# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > >+ > >+ > >+"""Helper class to test ssh-related code.""" > >+ > >+from ganeti import constants > >+from ganeti import pathutils > >+ > >+ > >+class FakeSshFileManager(object): > >+ """Class which 'fakes' the lowest layer of SSH key manipulation. > >+ > >+ There are various operations which touch the nodes' SSH keys and their > >+ respective key files (authorized_keys and ganeti_pub_keys). Those are > >+ tedious to test as file operations have to be mocked on different > levels > >+ (direct access to the authorized_keys and ganeti_pub_keys) of the > master > >+ node, indirect access to those files of the non-master nodes (via the > >+ ssh_update tool). In order to make unit tests of those operations more > >+ readable and managable, we introduce this class, which mocks all > >+ direct and indirect access to SSH key files on all nodes. This way, > >+ the state of this FakeSshFileManager represents the state of a > cluster's > >+ nodes' SSH key files in a consise and easily accessible way. > >+ > >+ """ > >+ def __init__(self): > >+ # Dictionary mapping node name to node properties. The properties > >+ # are a tuple of (node_uuid, ssh_key, is_potential_master_candidate, > >+ # is_master_candidate, is_master). > >+ self._all_node_data = {} > > Perhaps it'd be easier from a long-term maintenance perpsective to have a > dedicated class instead of 5-tuple). It'd be easier to extend, if needed, > field names would be more descriptive than just tuples, and some method > signatures might get simler. Or perhaps a class created by > `namedtuple(...)` > would be a good option. But I'd leave it as your call, what do you think > it's better, I'm not that Python expert. > Indeed, I had this thought as well. I wasn't sure if this would be overkill, but I kinda like the idea. I'll put it on my todo list to change it in a separate change (as this patch series is already long and merging this change into every patch would be a pain). > > >+ # Dictionary emulating the authorized keys files of all nodes. The > >+ # indices of the dictionary are the node names, the values are lists > >+ # of keys (strings). > >+ self._authorized_keys = {} > > Is there a reason for using lists as values instead of sets? For example to > keep the order of the keys? Just it seems that using sets would simplify a > lot of code below (add if not present, remove etc.). > Actually no, this is a very good point. I'll update that. > > >+ # Dictionary emulating the public keys file of all nodes. The indices > >+ # of the dictionary are the node names where the public key file is > >+ # 'located' (if it wasn't faked). The values of the dictionary are > >+ # dictionaries itself. Each of those dictionaries is indexed by the > >+ # node UUIDs mapping to a list of public keys. > >+ self._public_keys = {} # dict of dicts > >+ # Node name of the master node > >+ self._master_node_name = None > >+ > >+ def _SetMasterNodeName(self): > >+ self._master_node_name = [name for name, (_, _, _, _, master) > >+ in self._all_node_data.items() if > master][0] > >+ > >+ def GetMasterNodeName(self): > >+ return self._master_node_name > >+ > >+ def _CreateNodeDict(self, num_nodes, num_pot_mcs, num_mcs): > >+ """Creates a dictionary of all nodes and their properties.""" > >+ > >+ self._all_node_data = {} > >+ for i in range(num_nodes): > >+ name = "node_name_%i" % i > >+ uuid = "node_uuid_%i" % i > >+ key = "key%s" % i > >+ self._public_keys[name] = {} > >+ self._authorized_keys[name] = [] > >+ > >+ pot_mc = None > >+ if i in range(num_pot_mcs): > >+ pot_mc = True > >+ else: > >+ pot_mc = False > > Could be replaced by > > pot_mc = i < num_pot_mcs > Ack. > > >+ > >+ mc = None > >+ if i in range(num_mcs): > >+ mc = True > >+ else: > >+ mc = False > > .. also here. > Ack. > > >+ > >+ master = None > >+ if i == num_mcs / 2: > >+ master = True > >+ else: > >+ master = False > > also here - master = i == num_mcs / 2 > Ack. > > >+ > >+ self._all_node_data[name] = (uuid, key, pot_mc, mc, master) > >+ > >+ def _FillPublicKeyOfOneNode(self, receiving_node_name): > >+ _, _, is_pot_mc, _, _ = self._all_node_data[receiving_node_name] > >+ # Nodes which are not potential master candidates receive no keys > >+ if not is_pot_mc: > >+ return > >+ for uuid, key, pot_mc, _, _ in self._all_node_data.values(): > >+ if pot_mc: > >+ self._public_keys[receiving_node_name][uuid] = key > >+ > >+ def _FillAuthorizedKeyOfOneNode(self, receiving_node_name): > >+ for _, key, _, mc, _ in self._all_node_data.values(): > >+ if mc: > >+ self._authorized_keys[receiving_node_name].append(key) > >+ > >+ def InitAllNodes(self, num_nodes, num_pot_mcs, num_mcs): > >+ """Initializes the entire state of the cluster wrt SSH keys. > >+ > >+ @type num_nodes: int > >+ @param num_nodes: number of nodes in the cluster > >+ @type num_pot_mcs: int > >+ @param num_pot_mcs: number of potential master candidates in the > cluster > >+ @type num_mcs: in > >+ @param num_mcs: number of master candidates in the cluster. > >+ > >+ """ > >+ self._public_keys = {} > >+ self._authorized_keys = {} > >+ self._CreateNodeDict(num_nodes, num_pot_mcs, num_mcs) > >+ for node in self._all_node_data.keys(): > >+ self._FillPublicKeyOfOneNode(node) > >+ self._FillAuthorizedKeyOfOneNode(node) > >+ self._SetMasterNodeName() > >+ > >+ def GetSshPortMap(self, port): > >+ """Creates a SSH port map with all nodes mapped to the given port. > >+ > >+ @type port: int > >+ @param port: SSH port number for all nodes > >+ > >+ """ > >+ port_map = {} > >+ for node in self._all_node_data.keys(): > >+ port_map[node] = port > >+ return port_map > >+ > >+ def GetAllNodeNames(self): > >+ return self._all_node_data.keys() > >+ > >+ def GetAllPotentialMasterCandidateNodeNames(self): > >+ return [name for name, (_, _, pot_mc, _, _) > >+ in self._all_node_data.items() if pot_mc] > >+ > >+ def GetAllMasterCandidateUuids(self): > >+ return [uuid for uuid, _, _, mc, _ > >+ in self._all_node_data.values() if mc] > >+ > >+ def GetAllPotentialMasterCandidates(self): > >+ return [(name, uuid, key, pot_mc, mc, master) > >+ for name, (uuid, key, pot_mc, mc, master) > >+ in self._all_node_data.items() if pot_mc] > >+ > >+ def SetOrAddNode(self, name, uuid, key, pot_mc, mc, master): > >+ """Adds a new node to the state of the file manager. > >+ > >+ 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. > >+ > >+ @type name: string > >+ @param name: name of the new node > >+ @type uuid: string > >+ @param uuid: UUID of the new node > >+ @type key: string > >+ @param key: SSH key of the new node > >+ @type pot_mc: boolean > >+ @param pot_mc: whether the new node is a potential master candidate > >+ @type mc: boolean > >+ @param mc: whether the new node is a master candidate > >+ @type master: boolean > >+ @param master: whether the new node is the master > >+ > >+ """ > >+ self._all_node_data[name] = (uuid, key, pot_mc, mc, master) > >+ if name not in self._authorized_keys: > >+ self._authorized_keys[name] = [] > >+ if mc: > >+ if not key in self._authorized_keys[name]: > >+ self._authorized_keys[name].append(key) > >+ if name not in self._public_keys: > >+ self._public_keys[name] = {} > >+ > >+ def NodeHasPublicKey(self, file_node_name, key_node_uuid, key): > >+ """Checks whether a node has another node's public key. > >+ > >+ @type file_node_name: string > >+ @param file_node_name: name of the node whose public key file is > inspected > >+ @type key_node_uuid: string > >+ @param key_node_uuid: UUID of the node whose key is checked for > >+ @rtype: boolean > >+ @return: True if the key_node's UUID is found with the machting key > 'key' > >+ > >+ """ > >+ for (node_uuid, pub_keys) in > self._public_keys[file_node_name].items(): > >+ if key in pub_keys and key_node_uuid == node_uuid: > >+ return True > >+ return False > >+ > >+ def NodeHasAuthorizedKey(self, file_node_name, key): > >+ """Checks whether a node has a particular key in its authorized_keys > file. > >+ > >+ @type file_node_name: string > >+ @param file_node_name: name of the node whose authorized_key file is > >+ inspected > >+ @type key: string > >+ @param key: key which is expected to be found in the node's > authorized_key > >+ file > >+ @rtype: boolean > >+ @return: True if the key is found in the node's authorized_key file > >+ > >+ """ > >+ return key in self._authorized_keys[file_node_name] > >+ > >+ def AllNodesHaveAuthorizedKey(self, key): > >+ """Check if all nodes have a particular key in their auth. keys file. > >+ > >+ @type key: string > >+ @param key: key exptected to be present in all node's > authorized_keys file > >+ @rtype: boolean > >+ @return: True if key is present in all node's authorized_keys file > >+ > >+ """ > >+ for name in self._all_node_data.keys(): > >+ node_auth_keys = self._authorized_keys[name] > >+ if key not in node_auth_keys: > > Just nitpicking, probably not necessary to assign the variable, could be > directly in `if` (also applies to other methods below), but I'm not sure > what's more idiomatic Python. > Ack. > > >+ raise Exception("Node '%s' does not have the key '%s' in its" > >+ " 'authorized_keys' file." % (name, key)) > >+ return True > > I'm a bit puzzled by the output of the method. It'd expect it to either > return True/False (like in the following method), or have no return value > and throw the exception if a key is missing. Currently it returns true or > throws the exception, so the return value has no use. > I agree. I realized that already and have (separate) patch already prepared for this. As it would be a pain to merge that into every patch of the series, I'd rather simplify that properly at the end. Would that be fine for you? > > >+ > >+ def NoNodeHasAuthorizedKey(self, key): > >+ """Check if none of the nodes has a particular key in their auth. > keys file. > >+ > >+ @type key: string > >+ @param key: key exptected to be present in all node's > authorized_keys file > >+ @rtype: boolean > >+ @return: True if key is not present in all node's authorized_keys > file > >+ > >+ """ > >+ for name in self._all_node_data.keys(): > >+ node_auth_keys = self._authorized_keys[name] > >+ if key in node_auth_keys: > >+ return False > >+ return True > >+ > >+ def NoNodeHasPublicKey(self, uuid, key): > >+ """Check if none of the nodes have the given public key in their > file. > >+ > >+ @type uuid: string > >+ @param uuid: UUID of the node whose key is looked for > >+ @type key: string > >+ @param key: SSH key to be looked for > >+ > >+ """ > >+ for name in self._all_node_data.keys(): > >+ node_pub_keys = self._public_keys[name] > >+ if (uuid, key) in node_pub_keys.items(): > >+ return False > >+ return True > >+ > >+ def PotentialMasterCandidatesOnlyHavePublicKey(self, query_node_name): > >+ """Checks if the node's key is on all potential master candidates > only. > >+ > >+ This ensures that the node's key is in all public key files of all > >+ potential master candidates, and it also checks whether the key is > >+ *not* in all other nodes's key files. > >+ > >+ @param query_node_name: name of the node whose key is expected to be > >+ in the public key file of all potential > master > >+ candidates > >+ @type query_node_name: string > >+ > >+ """ > >+ 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.") > >+ > >+ # Disabling a pylint warning about unused parameters. Those need > >+ # to be here to properly mock the real methods. > >+ # pylint: disable=W0613 > >+ def RunCommand(self, cluster_name, node, base_cmd, port, data, > >+ debug=False, verbose=False, use_cluster_key=False, > >+ ask_key=False, strict_host_check=False, > >+ ensure_version=False): > >+ """This emulates ssh.RunSshCmdWithStdin calling ssh_update. > >+ > >+ While in real SSH operations, ssh.RunSshCmdWithStdin is called > >+ with the command ssh_update to manipulate a remote node's SSH > >+ key files (authorized_keys and ganeti_pub_key) file, this method > >+ emulates the operation by manipulating only its internal dictionaries > >+ of SSH keys. No actual key files of any node is touched. > >+ > >+ """ > >+ assert base_cmd == pathutils.SSH_UPDATE > >+ > >+ if constants.SSHS_SSH_AUTHORIZED_KEYS in data: > >+ instructions_auth = data[constants.SSHS_SSH_AUTHORIZED_KEYS] > >+ self._HandleAuthorizedKeys(instructions_auth, node) > >+ if constants.SSHS_SSH_PUBLIC_KEYS in data: > >+ instructions_pub = data[constants.SSHS_SSH_PUBLIC_KEYS] > >+ self._HandlePublicKeys(instructions_pub, node) > >+ # pylint: enable=W0613 > >+ > >+ def _EnsureAuthKeyFile(self, file_node_name): > >+ if file_node_name not in self._authorized_keys: > >+ self._authorized_keys[file_node_name] = [] > >+ > >+ def _AddAuthorizedKey(self, file_node_name, ssh_key): > >+ self._EnsureAuthKeyFile(file_node_name) > >+ if not ssh_key in self._authorized_keys[file_node_name]: > >+ self._authorized_keys[file_node_name].append(ssh_key) > >+ > >+ def _RemoveAuthorizedKey(self, file_node_name, key): > >+ self._EnsureAuthKeyFile(file_node_name) > >+ self._authorized_keys[file_node_name][:] = \ > >+ [k for k in self._authorized_keys[file_node_name] if k != key] > >+ > >+ def _HandleAuthorizedKeys(self, instructions, node): > >+ (action, authorized_keys) = instructions > >+ ssh_keys = [] > >+ for ssh_key in authorized_keys.values(): > >+ ssh_keys.append(ssh_key) > > Could this be replaced by just > > ssh_keys = authorized_keys.values() > > ? > Indeed, I think that was some refactoring mishap. > > >+ if action == constants.SSHS_ADD: > >+ for ssh_key in ssh_keys: > >+ self._AddAuthorizedKey(node, ssh_key) > >+ elif action == constants.SSHS_REMOVE: > >+ for ssh_key in ssh_keys: > >+ self._RemoveAuthorizedKey(node, ssh_key) > >+ else: > >+ raise Exception("Unsupported action: %s" % action) > >+ > >+ def _EnsurePublicKeyFile(self, file_node_name): > >+ if file_node_name not in self._public_keys: > >+ self._public_keys[file_node_name] = {} > >+ > >+ def _ClearPublicKeys(self, file_node_name): > >+ self._public_keys[file_node_name] = {} > >+ > >+ def _OverridePublicKeys(self, ssh_keys, file_node_name): > >+ self._ClearPublicKeys(file_node_name) > >+ for key_node_uuid, node_keys in ssh_keys.items(): > >+ 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 > >+ > >+ def _AddPublicKeys(self, public_keys, file_node_name): > >+ self._EnsurePublicKeyFile(file_node_name) > >+ for key_node_uuid, keys in public_keys.items(): > >+ self._public_keys[file_node_name][key_node_uuid] = keys > >+ > >+ def _ReplaceOrAddPublicKeys(self, public_keys, file_node_name): > >+ self._EnsurePublicKeyFile(file_node_name) > >+ for key_node_uuid, keys in public_keys.items(): > >+ self._public_keys[file_node_name][key_node_uuid] = keys > > These two methods seem to me to be the same, perhaps some check is missing > in the first one? > I removed the first one alltogether and just called the second one. This is consistent the implementation that it emulates. > > >+ > >+ def _RemovePublicKeys(self, public_keys, file_node_name): > >+ 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] = [] > >+ > >+ def _HandlePublicKeys(self, instructions, node): > >+ (action, public_keys) = instructions > >+ if action == constants.SSHS_OVERRIDE: > >+ self._OverridePublicKeys(public_keys, node) > >+ elif action == constants.SSHS_ADD: > >+ self._AddPublicKeys(public_keys, node) > >+ elif action == constants.SSHS_REPLACE_OR_ADD: > >+ self._ReplaceOrAddPublicKeys(public_keys, node) > >+ elif action == constants.SSHS_REMOVE: > >+ self._RemovePublicKeys(public_keys, node) > >+ elif action == constants.SSHS_CLEAR: > >+ self._ClearPublicKeys(node) > >+ else: > >+ raise Exception("Unsupported action: %s." % action) > >+ > >+ # pylint: disable=W0613 > >+ def AddAuthorizedKeys(self, file_obj, keys): > >+ """Emulates ssh.AddAuthorizedKeys on the master node. > >+ > >+ Instead of actually mainpulating the authorized_keys file, this > method > >+ keeps the state of the file in a dictionary in memory. > >+ > >+ @see: C{ssh.AddAuthorizedKeys} > >+ > >+ """ > >+ assert self._master_node_name > >+ for key in keys: > >+ self._AddAuthorizedKey(self._master_node_name, key) > >+ > >+ def RemoveAuthorizedKeys(self, file_name, keys): > >+ """Emulates ssh.RemoveAuthorizeKeys on the master node. > >+ > >+ Instead of actually mainpulating the authorized_keys file, this > method > >+ keeps the state of the file in a dictionary in memory. > >+ > >+ @see: C{ssh.RemoveAuthorizedKeys} > >+ > >+ """ > >+ assert self._master_node_name > >+ for key in keys: > >+ self._RemoveAuthorizedKey(self._master_node_name, key) > >+ > >+ def AddPublicKey(self, new_uuid, new_key, **kwargs): > >+ """Emulates ssh.AddPublicKey on the master node. > >+ > >+ Instead of actually mainpulating the authorized_keys file, this > method > >+ keeps the state of the file in a dictionary in memory. > >+ > >+ @see: C{ssh.AddPublicKey} > >+ > >+ """ > >+ assert self._master_node_name > >+ key_dict = {new_uuid: new_key} > >+ self._AddPublicKeys(key_dict, self._master_node_name) > >+ > >+ def RemovePublicKey(self, target_uuid, **kwargs): > >+ """Emulates ssh.RemovePublicKey on the master node. > >+ > >+ Instead of actually mainpulating the authorized_keys file, this > method > >+ keeps the state of the file in a dictionary in memory. > >+ > >+ @see: {ssh.RemovePublicKey} > >+ > >+ """ > >+ assert self._master_node_name > >+ key_dict = {target_uuid: []} > >+ self._RemovePublicKeys(key_dict, self._master_node_name) > >+ > >+ def QueryPubKeyFile(self, target_uuids, **kwargs): > >+ """Emulates ssh.QueryPubKeyFile on the master node. > >+ > >+ Instead of actually mainpulating the authorized_keys file, this > method > >+ keeps the state of the file in a dictionary in memory. > >+ > >+ @see: C{ssh.QueryPubKey} > >+ > >+ """ > >+ assert self._master_node_name > >+ all_keys = target_uuids is None > >+ if all_keys: > >+ return self._public_keys[self._master_node_name] > >+ > >+ if isinstance(target_uuids, str): > >+ target_uuids = [target_uuids] > >+ result_dict = {} > >+ for key_node_uuid, key in \ > >+ self._public_keys[self._master_node_name].items(): > >+ if key_node_uuid in target_uuids: > >+ result_dict[key_node_uuid] = key > >+ return result_dict > >+ > >+ def ReplaceNameByUuid(self, node_uuid, node_name, **kwargs): > >+ """Emulates ssh.ReplaceNameByUuid on the master node. > >+ > >+ Instead of actually mainpulating the authorized_keys file, this > method > >+ keeps the state of the file in a dictionary in memory. > >+ > >+ @see: C{ssh.ReplacenameByUuid} > >+ > >+ """ > >+ 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] > >+ # pylint: enable=W0613 > >-- > >2.2.0.rc0.207.ga3a616c > > > > Rest LGTM > Interdiff: diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py index 1e7edf8..bc704b0 100644 --- a/test/py/testutils_ssh.py +++ b/test/py/testutils_ssh.py @@ -55,7 +55,7 @@ class FakeSshFileManager(object): # is_master_candidate, is_master). self._all_node_data = {} # Dictionary emulating the authorized keys files of all nodes. The - # indices of the dictionary are the node names, the values are lists + # indices of the dictionary are the node names, the values are sets # of keys (strings). self._authorized_keys = {} # Dictionary emulating the public keys file of all nodes. The indices @@ -83,25 +83,11 @@ class FakeSshFileManager(object): uuid = "node_uuid_%i" % i key = "key%s" % i self._public_keys[name] = {} - self._authorized_keys[name] = [] + self._authorized_keys[name] = set() - pot_mc = None - if i in range(num_pot_mcs): - pot_mc = True - else: - pot_mc = False - - mc = None - if i in range(num_mcs): - mc = True - else: - mc = False - - master = None - if i == num_mcs / 2: - master = True - else: - master = False + pot_mc = i < num_pot_mcs + mc = i < num_mcs + master = i == num_mcs / 2 self._all_node_data[name] = (uuid, key, pot_mc, mc, master) @@ -117,7 +103,7 @@ class FakeSshFileManager(object): def _FillAuthorizedKeyOfOneNode(self, receiving_node_name): for _, key, _, mc, _ in self._all_node_data.values(): if mc: - self._authorized_keys[receiving_node_name].append(key) + self._authorized_keys[receiving_node_name].add(key) def InitAllNodes(self, num_nodes, num_pot_mcs, num_mcs): """Initializes the entire state of the cluster wrt SSH keys. @@ -189,10 +175,9 @@ class FakeSshFileManager(object): """ self._all_node_data[name] = (uuid, key, pot_mc, mc, master) if name not in self._authorized_keys: - self._authorized_keys[name] = [] + self._authorized_keys[name] = set() if mc: - if not key in self._authorized_keys[name]: - self._authorized_keys[name].append(key) + self._authorized_keys[name].add(key) if name not in self._public_keys: self._public_keys[name] = {} @@ -237,8 +222,7 @@ class FakeSshFileManager(object): """ for name in self._all_node_data.keys(): - node_auth_keys = self._authorized_keys[name] - if key not in node_auth_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)) return True @@ -327,23 +311,20 @@ class FakeSshFileManager(object): def _EnsureAuthKeyFile(self, file_node_name): if file_node_name not in self._authorized_keys: - self._authorized_keys[file_node_name] = [] + self._authorized_keys[file_node_name] = set() def _AddAuthorizedKey(self, file_node_name, ssh_key): self._EnsureAuthKeyFile(file_node_name) - if not ssh_key in self._authorized_keys[file_node_name]: - self._authorized_keys[file_node_name].append(ssh_key) + self._authorized_keys[file_node_name].add(ssh_key) def _RemoveAuthorizedKey(self, file_node_name, key): self._EnsureAuthKeyFile(file_node_name) - self._authorized_keys[file_node_name][:] = \ - [k for k in self._authorized_keys[file_node_name] if k != key] + self._authorized_keys[file_node_name] = \ + set([k for k in self._authorized_keys[file_node_name] if k != key]) def _HandleAuthorizedKeys(self, instructions, node): (action, authorized_keys) = instructions - ssh_keys = [] - for ssh_key in authorized_keys.values(): - ssh_keys.append(ssh_key) + ssh_keys = authorized_keys.values() if action == constants.SSHS_ADD: for ssh_key in ssh_keys: self._AddAuthorizedKey(node, ssh_key) @@ -367,11 +348,6 @@ class FakeSshFileManager(object): raise Exception("Duplicate node in ssh_update data.") self._public_keys[file_node_name][key_node_uuid] = node_keys - def _AddPublicKeys(self, public_keys, file_node_name): - self._EnsurePublicKeyFile(file_node_name) - for key_node_uuid, keys in public_keys.items(): - self._public_keys[file_node_name][key_node_uuid] = keys - def _ReplaceOrAddPublicKeys(self, public_keys, file_node_name): self._EnsurePublicKeyFile(file_node_name) for key_node_uuid, keys in public_keys.items(): @@ -388,7 +364,7 @@ class FakeSshFileManager(object): if action == constants.SSHS_OVERRIDE: self._OverridePublicKeys(public_keys, node) elif action == constants.SSHS_ADD: - self._AddPublicKeys(public_keys, node) + self._ReplaceOrAddPublicKeys(public_keys, node) elif action == constants.SSHS_REPLACE_OR_ADD: self._ReplaceOrAddPublicKeys(public_keys, node) elif action == constants.SSHS_REMOVE:
