FYI, I forgot another occurrence of _AddPublicKeys, interdiff:
diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py
index bc704b0..a96421c 100644
--- a/test/py/testutils_ssh.py
+++ b/test/py/testutils_ssh.py
@@ -412,7 +412,7 @@ class FakeSshFileManager(object):
"""
assert self._master_node_name
key_dict = {new_uuid: new_key}
- self._AddPublicKeys(key_dict, self._master_node_name)
+ self._ReplaceOrAddPublicKeys(key_dict, self._master_node_name)
def RemovePublicKey(self, target_uuid, **kwargs):
"""Emulates ssh.RemovePublicKey on the master node.
(I assume still LGTM)
On Fri, 17 Apr 2015 at 18:19 Petr Pudlak <[email protected]> wrote:
> 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
>
