On Fri, Apr 17, 2015 at 02:03:51PM +0000, Helga Velroyen wrote:
>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:
LGTM, thanks
