Hi! On Mon, 16 Nov 2015 at 10:09 Hrvoje Ribicic <[email protected]> wrote:
> 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. > Much better :) > > >> >> >>> >>> 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 > ... > Ah, okay, get it. Yes, makes sense. > > >> >> >>> 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! > I doublechecked and I think it makes sense what you did. However, I assume you ran enough QA's (with and without ssh-init, for example the vcluster QA) to ensure it does not break anything. > > >> >>> + 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. > > LGTM, thanks -- 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.
