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:
