On Fri, Nov 13, 2015 at 2:21 PM, Helga Velroyen <[email protected]> wrote:
> > > On Fri, 13 Nov 2015 at 11:18 'Hrvoje Ribicic' via ganeti-devel < > [email protected]> wrote: > >> By explicitly specifying the old and new SSH key type in various >> renew-crypto operations, this patch allows the switching of SSH key >> types to take place during a SSH key renewal operation. >> > > Nit: I find the phrasing "in various renew-crypto operations' kind of odd, > as it is only the 'renew-crypto --new-ssh-keys' that is affected, right? > Will reword this to: By explicitly specifying the old and new SSH key type in the SSH key renewal, this patch allows the switching of SSH key types to take place during such an operation. > > >> >> Signed-off-by: Hrvoje Ribicic <[email protected]> >> --- >> lib/backend.py | 28 +++++++++++++----------- >> lib/client/gnt_cluster.py | 17 +++++++++++---- >> lib/cmdlib/cluster/__init__.py | 49 >> +++++++++++++++++++++++++----------------- >> lib/rpc_defs.py | 5 +++-- >> lib/server/noded.py | 7 +++--- >> src/Ganeti/OpCodes.hs | 4 +++- >> src/Ganeti/OpParams.hs | 20 +++++++++++++---- >> src/Ganeti/Rpc.hs | 12 +++++------ >> test/hs/Test/Ganeti/OpCodes.hs | 9 ++++++-- >> 9 files changed, 97 insertions(+), 54 deletions(-) >> >> diff --git a/lib/backend.py b/lib/backend.py >> index 65cb976..e20d45a 100644 >> --- a/lib/backend.py >> +++ b/lib/backend.py >> @@ -1885,7 +1885,8 @@ def _ReplaceMasterKeyOnMaster(root_keyfiles): >> >> >> def RenewSshKeys(node_uuids, node_names, master_candidate_uuids, >> - potential_master_candidates, ssh_key_type, ssh_key_bits, >> + potential_master_candidates, old_key_type, new_key_type, >> + new_key_bits, >> ganeti_pub_keys_file=pathutils.SSH_PUB_KEYS, >> ssconf_store=None, >> noded_cert_file=pathutils.NODED_CERT_FILE, >> @@ -1900,10 +1901,12 @@ def RenewSshKeys(node_uuids, node_names, >> master_candidate_uuids, >> @type master_candidate_uuids: list of str >> @param master_candidate_uuids: list of UUIDs of master candidates or >> master node >> - @type ssh_key_type: One of L{constants.SSHK_ALL} >> - @param ssh_key_type: the type of SSH key to be generated >> - @type ssh_key_bits: int >> - @param ssh_key_bits: the length of the key to be generated >> + @type old_key_type: One of L{constants.SSHK_ALL} >> + @param old_key_type: the type of SSH key already present on nodes >> + @type new_key_type: One of L{constants.SSHK_ALL} >> + @param new_key_type: the type of SSH key to be generated >> + @type new_key_bits: int >> + @param new_key_bits: the length of the key to be generated >> @type ganeti_pub_keys_file: str >> @param ganeti_pub_keys_file: file path of the the public key file >> @type noded_cert_file: str >> @@ -1927,8 +1930,9 @@ def RenewSshKeys(node_uuids, node_names, >> master_candidate_uuids, >> >> (_, root_keyfiles) = \ >> ssh.GetAllUserFiles(constants.SSH_LOGIN_USER, mkdir=False, >> dircheck=False) >> - (_, node_pub_keyfile) = root_keyfiles[ssh_key_type] >> - old_master_key = utils.ReadFile(node_pub_keyfile) >> + (_, old_pub_keyfile) = root_keyfiles[old_key_type] >> + (_, new_pub_keyfile) = root_keyfiles[new_key_type] >> + old_master_key = utils.ReadFile(old_pub_keyfile) >> >> node_uuid_name_map = zip(node_uuids, node_names) >> >> @@ -1956,7 +1960,7 @@ def RenewSshKeys(node_uuids, node_names, >> master_candidate_uuids, >> >> if master_candidate: >> logging.debug("Fetching old SSH key from node '%s'.", node_name) >> - old_pub_key = ssh.ReadRemoteSshPubKeys(node_pub_keyfile, >> + old_pub_key = ssh.ReadRemoteSshPubKeys(old_pub_keyfile, >> node_name, cluster_name, >> ssh_port_map[node_name], >> False, # ask_key >> @@ -1981,15 +1985,15 @@ def RenewSshKeys(node_uuids, node_names, >> master_candidate_uuids, >> " key. Not deleting that key on the node.", >> node_name) >> >> logging.debug("Generating new SSH key for node '%s'.", node_name) >> - _GenerateNodeSshKey(node_uuid, node_name, ssh_port_map, ssh_key_type, >> - ssh_key_bits, pub_key_file=ganeti_pub_keys_file, >> + _GenerateNodeSshKey(node_uuid, node_name, ssh_port_map, new_key_type, >> + new_key_bits, pub_key_file=ganeti_pub_keys_file, >> ssconf_store=ssconf_store, >> noded_cert_file=noded_cert_file, >> run_cmd_fn=run_cmd_fn) >> >> try: >> logging.debug("Fetching newly created SSH key from node '%s'.", >> node_name) >> - pub_key = ssh.ReadRemoteSshPubKeys(node_pub_keyfile, >> + pub_key = ssh.ReadRemoteSshPubKeys(new_pub_keyfile, >> node_name, cluster_name, >> ssh_port_map[node_name], >> False, # ask_key >> @@ -2024,7 +2028,7 @@ def RenewSshKeys(node_uuids, node_names, >> master_candidate_uuids, >> # Generate a new master key with a suffix, don't touch the old one for >> now >> logging.debug("Generate new ssh key of master.") >> _GenerateNodeSshKey(master_node_uuid, master_node_name, ssh_port_map, >> - ssh_key_type, ssh_key_bits, >> + new_key_type, new_key_bits, >> pub_key_file=ganeti_pub_keys_file, >> ssconf_store=ssconf_store, >> noded_cert_file=noded_cert_file, >> diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py >> index c7f9048..59f1eb9 100644 >> --- a/lib/client/gnt_cluster.py >> +++ b/lib/client/gnt_cluster.py >> @@ -978,11 +978,12 @@ def _ReadAndVerifyCert(cert_filename, >> verify_private_key=False): >> return pem >> >> >> +# pylint: disable=R0913 >> def _RenewCrypto(new_cluster_cert, new_rapi_cert, # pylint: disable=R0911 >> rapi_cert_filename, new_spice_cert, spice_cert_filename, >> spice_cacert_filename, new_confd_hmac_key, new_cds, >> cds_filename, force, new_node_cert, new_ssh_keys, >> - verbose, debug): >> + ssh_key_type, ssh_key_bits, verbose, debug): >> """Renews cluster certificates, keys and secrets. >> >> @type new_cluster_cert: bool >> @@ -1010,10 +1011,14 @@ def _RenewCrypto(new_cluster_cert, new_rapi_cert, >> # pylint: disable=R0911 >> @param new_node_cert: Whether to generate new node certificates >> @type new_ssh_keys: bool >> @param new_ssh_keys: Whether to generate new node SSH keys >> + @type ssh_key_type: One of L{constants.SSHK_ALL} >> + @param ssh_key_type: The type of SSH key to be generated >> + @type ssh_key_bits: int >> + @param ssh_key_bits: The length of the key to be generated >> @type verbose: boolean >> - @param verbose: show verbose output >> + @param verbose: Show verbose output >> @type debug: boolean >> - @param debug: show debug output >> + @param debug: Show debug output >> >> """ >> ToStdout("Updating certificates now. Running \"gnt-cluster verify\" " >> @@ -1194,7 +1199,9 @@ def _RenewCrypto(new_cluster_cert, new_rapi_cert, # >> pylint: disable=R0911 >> cl = GetClient() >> renew_op = opcodes.OpClusterRenewCrypto( >> node_certificates=new_node_cert or new_cluster_cert, >> - ssh_keys=new_ssh_keys) >> + renew_ssh_keys=new_ssh_keys, >> + ssh_key_type=ssh_key_type, >> + ssh_key_bits=ssh_key_bits) >> SubmitOpCode(renew_op, cl=cl) >> >> ToStdout("All requested certificates and keys have been replaced." >> @@ -1269,6 +1276,8 @@ def RenewCrypto(opts, args): >> opts.force, >> opts.new_node_cert, >> opts.new_ssh_keys, >> + opts.ssh_key_type, >> + opts.ssh_key_bits, >> opts.verbose, >> opts.debug > 0) >> >> diff --git a/lib/cmdlib/cluster/__init__.py >> b/lib/cmdlib/cluster/__init__.py >> index ed6a3b8..5658646 100644 >> --- a/lib/cmdlib/cluster/__init__.py >> +++ b/lib/cmdlib/cluster/__init__.py >> @@ -87,17 +87,6 @@ class LUClusterRenewCrypto(NoHooksLU): >> self.share_locks = ShareAll() >> self.share_locks[locking.LEVEL_NODE] = 0 >> >> - def CheckPrereq(self): >> - """Check prerequisites. >> - >> - This checks whether the cluster is empty. >> - >> - Any errors are signaled by raising errors.OpPrereqError. >> - >> - """ >> - self._ssh_renewal_suppressed = \ >> - not self.cfg.GetClusterInfo().modify_ssh_setup and self.op.ssh_keys >> - >> > > This looks odd to me. Isn't it still necessary to check whether or not > renewal is suppressed? > > Did you run a QA with --no-ssh-init? > This bit of code was only assigning the value of the _ssh_renewal_suppressed member, not actually doing anything and aborting like other prerequisite checks are supposed to. Instead of having it there and confusing me, I moved the definition to the point where it was actually being used. How I did that I can discuss below ... > > >> def _RenewNodeSslCertificates(self, feedback_fn): >> """Renews the nodes' SSL certificates. >> >> @@ -159,9 +148,12 @@ class LUClusterRenewCrypto(NoHooksLU): >> >> self.cfg.SetCandidateCerts(digest_map) >> >> - def _RenewSshKeys(self): >> + def _RenewSshKeys(self, feedback_fn): >> """Renew all nodes' SSH keys. >> >> + @type feedback_fn: function >> + @param feedback_fn: logging function, see >> L{ganeti.cmdlist.base.LogicalUnit} >> + >> """ >> master_uuid = self.cfg.GetMasterNode() >> >> @@ -175,25 +167,42 @@ class LUClusterRenewCrypto(NoHooksLU): >> >> cluster_info = self.cfg.GetClusterInfo() >> >> + new_ssh_key_type = self.op.ssh_key_type >> + if new_ssh_key_type is None: >> + new_ssh_key_type = cluster_info.ssh_key_type >> + >> + new_ssh_key_bits = self.op.ssh_key_bits >> + if new_ssh_key_bits is None: >> + new_ssh_key_bits = cluster_info.ssh_key_bits >> + >> result = self.rpc.call_node_ssh_keys_renew( >> [master_uuid], >> node_uuids, node_names, >> master_candidate_uuids, >> potential_master_candidates, >> - cluster_info.ssh_key_type, >> - cluster_info.ssh_key_bits) >> + cluster_info.ssh_key_type, # Old key type >> + new_ssh_key_type, # New key type >> + new_ssh_key_bits) # New key bits >> result[master_uuid].Raise("Could not renew the SSH keys of all >> nodes") >> >> + # After the keys have been successfully swapped, time to commit the >> change >> + # in key type >> + cluster_info.ssh_key_type = new_ssh_key_type >> + cluster_info.ssh_key_bits = new_ssh_key_bits >> + self.cfg.Update(cluster_info, feedback_fn) >> + >> def Exec(self, feedback_fn): >> if self.op.node_certificates: >> feedback_fn("Renewing Node SSL certificates") >> self._RenewNodeSslCertificates(feedback_fn) >> - if self.op.ssh_keys and not self._ssh_renewal_suppressed: >> - feedback_fn("Renewing SSH keys") >> - self._RenewSshKeys() >> - elif self._ssh_renewal_suppressed: >> - feedback_fn("Cannot renew SSH keys if the cluster is configured to >> not" >> - " modify the SSH setup.") >> + >> + if self.op.renew_ssh_keys: >> > > see above, we should still respect the modify_ssh_setup option. > The original logic was: _ssh_renewal_suppressed = not modify_ssh_setup and self.op.ssh_keys making the first condition: (self.op.ssh_keys and (modify_ssh_setup or not self.op.ssh_keys)) == self.op.ssh_keys and modify_ssh_setup and the second one: self.op.ssh_keys and not modify_ssh_setup Confusingly enough for this review, I renamed the ssh_keys opcode member to renew_ssh_keys, as it is a boolean anyway. Would appreciate you double-checking this, as these transformations are easy to mess up! > >> + if self.cfg.GetClusterInfo().modify_ssh_setup: >> + feedback_fn("Renewing SSH keys") >> + self._RenewSshKeys(feedback_fn) >> + else: >> + feedback_fn("Cannot renew SSH keys if the cluster is configured >> to not" >> + " modify the SSH setup.") >> >> >> class LUClusterActivateMasterIp(NoHooksLU): >> diff --git a/lib/rpc_defs.py b/lib/rpc_defs.py >> index 88f0ece..6337285 100644 >> --- a/lib/rpc_defs.py >> +++ b/lib/rpc_defs.py >> @@ -567,8 +567,9 @@ _NODE_CALLS = [ >> ("node_names", None, "Names of the nodes whose key is renewed"), >> ("master_candidate_uuids", None, "List of UUIDs of master >> candidates."), >> ("potential_master_candidates", None, "Potential master candidates"), >> - ("ssh_key_type", None, "The type of key to generate"), >> - ("ssh_key_bits", None, "The length of the key to generate")], >> + ("old_key_type", None, "The type of key previously used"), >> + ("new_key_type", None, "The type of key to generate"), >> + ("new_key_bits", None, "The length of the key to generate")], >> None, None, "Renew all SSH key pairs of all nodes nodes."), >> ] >> >> diff --git a/lib/server/noded.py b/lib/server/noded.py >> index dddf493..eb7eb75 100644 >> --- a/lib/server/noded.py >> +++ b/lib/server/noded.py >> @@ -946,10 +946,11 @@ class >> NodeRequestHandler(http.server.HttpServerHandler): >> >> """ >> (node_uuids, node_names, master_candidate_uuids, >> - potential_master_candidates, ssh_key_type, ssh_key_bits) = params >> + potential_master_candidates, old_key_type, new_key_type, >> + new_key_bits) = params >> return backend.RenewSshKeys(node_uuids, node_names, >> master_candidate_uuids, >> - potential_master_candidates, >> ssh_key_type, >> - ssh_key_bits) >> + potential_master_candidates, >> old_key_type, >> + new_key_type, new_key_bits) >> >> @staticmethod >> def perspective_node_ssh_key_remove(params): >> diff --git a/src/Ganeti/OpCodes.hs b/src/Ganeti/OpCodes.hs >> index 70fde67..66094a2 100644 >> --- a/src/Ganeti/OpCodes.hs >> +++ b/src/Ganeti/OpCodes.hs >> @@ -281,7 +281,9 @@ $(genOpCode "OpCode" >> [t| () |], >> OpDoc.opClusterRenewCrypto, >> [ pNodeSslCerts >> - , pSshKeys >> + , pRenewSshKeys >> + , pSshKeyType >> + , pSshKeyBits >> , pVerbose >> , pDebug >> ], >> diff --git a/src/Ganeti/OpParams.hs b/src/Ganeti/OpParams.hs >> index 08c4be8..0a793c0 100644 >> --- a/src/Ganeti/OpParams.hs >> +++ b/src/Ganeti/OpParams.hs >> @@ -299,7 +299,9 @@ module Ganeti.OpParams >> , pEnabledDataCollectors >> , pDataCollectorInterval >> , pNodeSslCerts >> - , pSshKeys >> + , pSshKeyBits >> + , pSshKeyType >> + , pRenewSshKeys >> , pNodeSetup >> , pVerifyClutter >> , pLongSleep >> @@ -1895,11 +1897,21 @@ pNodeSslCerts = >> defaultField [| False |] $ >> simpleField "node_certificates" [t| Bool |] >> >> -pSshKeys :: Field >> -pSshKeys = >> +pSshKeyBits :: Field >> +pSshKeyBits = >> + withDoc "The number of bits of the SSH key Ganeti uses" . >> + optionalField $ simpleField "ssh_key_bits" [t| Positive Int |] >> + >> +pSshKeyType :: Field >> +pSshKeyType = >> + withDoc "The type of the SSH key Ganeti uses" . >> + optionalField $ simpleField "ssh_key_type" [t| SshKeyType |] >> + >> +pRenewSshKeys :: Field >> +pRenewSshKeys = >> withDoc "Whether to renew SSH keys" . >> defaultField [| False |] $ >> - simpleField "ssh_keys" [t| Bool |] >> + simpleField "renew_ssh_keys" [t| Bool |] >> >> pNodeSetup :: Field >> pNodeSetup = >> diff --git a/src/Ganeti/Rpc.hs b/src/Ganeti/Rpc.hs >> index a43cc52..3fb128a 100644 >> --- a/src/Ganeti/Rpc.hs >> +++ b/src/Ganeti/Rpc.hs >> @@ -650,9 +650,9 @@ instance Rpc RpcCallExportList RpcResultExportList >> where >> rpcResultFill _ res = fromJSValueToRes res RpcResultExportList >> >> -- ** Job Queue Replication >> - >> + >> -- | Update a job queue file >> - >> + >> $(buildObject "RpcCallJobqueueUpdate" "rpcCallJobqueueUpdate" >> [ simpleField "file_name" [t| String |] >> , simpleField "content" [t| String |] >> @@ -702,9 +702,9 @@ instance Rpc RpcCallJobqueueRename >> RpcResultJobqueueRename where >> $ JsonDecodeError ("Expected JSNull, got " ++ show >> (pp_value res)) >> >> -- ** Watcher Status Update >> - >> + >> -- | Set the watcher status >> - >> + >> $(buildObject "RpcCallSetWatcherPause" "rpcCallSetWatcherPause" >> [ optionalField $ timeAsDoubleField "time" >> ]) >> @@ -724,9 +724,9 @@ instance Rpc RpcCallSetWatcherPause >> RpcResultSetWatcherPause where >> ("Expected JSNull, got " ++ show (pp_value res)) >> >> -- ** Queue drain status >> - >> + >> -- | Set the queu drain flag >> - >> + >> $(buildObject "RpcCallSetDrainFlag" "rpcCallSetDrainFlag" >> [ simpleField "value" [t| Bool |] >> ]) >> diff --git a/test/hs/Test/Ganeti/OpCodes.hs >> b/test/hs/Test/Ganeti/OpCodes.hs >> index 17c361a..d529576 100644 >> --- a/test/hs/Test/Ganeti/OpCodes.hs >> +++ b/test/hs/Test/Ganeti/OpCodes.hs >> @@ -168,8 +168,13 @@ instance Arbitrary OpCodes.OpCode where >> "OP_TAGS_DEL" -> >> arbitraryOpTagsDel >> "OP_CLUSTER_POST_INIT" -> pure OpCodes.OpClusterPostInit >> - "OP_CLUSTER_RENEW_CRYPTO" -> OpCodes.OpClusterRenewCrypto <$> >> - arbitrary <*> arbitrary <*> arbitrary <*> arbitrary >> + "OP_CLUSTER_RENEW_CRYPTO" -> OpCodes.OpClusterRenewCrypto >> + <$> arbitrary -- Node SSL certificates >> + <*> arbitrary -- renew_ssh_keys >> + <*> arbitrary -- ssh_key_type >> + <*> arbitrary -- ssh_key_bits >> + <*> arbitrary -- verbose >> + <*> arbitrary -- debug >> "OP_CLUSTER_DESTROY" -> pure OpCodes.OpClusterDestroy >> "OP_CLUSTER_QUERY" -> pure OpCodes.OpClusterQuery >> "OP_CLUSTER_VERIFY" -> >> -- >> 2.6.0.rc2.230.g3dd15c0 >> >> > > -- > > Helga Velroyen > Software Engineer > [email protected] > > Google Germany GmbH > Dienerstraße 12 > 80331 München > > Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > > Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, > leiten Sie diese bitte nicht weiter, informieren Sie den Absender und > löschen Sie die E-Mail und alle Anhänge. Vielen Dank. > > This e-mail is confidential. If you are not the right addressee please do > not forward it, please inform the sender, and please erase this e-mail > including any attachments. Thanks. > > Hrvoje Ribicic Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks.
