That makes more sense :) LGTM, thanks On Mon, 16 Nov 2015 at 10:09 Hrvoje Ribicic <[email protected]> wrote:
> On Fri, Nov 13, 2015 at 2:41 PM, Helga Velroyen <[email protected]> wrote: > >> >> >> On Fri, 13 Nov 2015 at 11:18 'Hrvoje Ribicic' via ganeti-devel < >> [email protected]> wrote: >> >>> This patch expands the testing of SSH key renewal by changing the key >>> type existing on a cluster during the QA. >>> >>> Signed-off-by: Hrvoje Ribicic <[email protected]> >>> --- >>> qa/qa_cluster.py | 55 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 52 insertions(+), 3 deletions(-) >>> >>> diff --git a/qa/qa_cluster.py b/qa/qa_cluster.py >>> index 1ad2fb9..941967c 100644 >>> --- a/qa/qa_cluster.py >>> +++ b/qa/qa_cluster.py >>> @@ -1195,6 +1195,56 @@ def _AssertSsconfCertFiles(): >>> " '%s'." % (node, first_node)) >>> >>> >>> +def _TestSSHKeyChanges(master_node): >>> + """Tests a lot of SSH key type- and size- related functionality. >>> + >>> + @type master_node: string >>> + @param master_node: The cluster master. >>> >> >> Nit: I personally would rephrase that to "the *name* of the cluster >> master" since we have way to many places already where one has to think 'is >> this now the UUID or the name?". >> > > Well, good that you pointed this out as this is actually not a string :D > Interdiff: > > diff --git a/qa/qa_cluster.py b/qa/qa_cluster.py > index 941967c..fbb90d0 100644 > --- a/qa/qa_cluster.py > +++ b/qa/qa_cluster.py > @@ -1198,7 +1198,7 @@ def _AssertSsconfCertFiles(): > def _TestSSHKeyChanges(master_node): > """Tests a lot of SSH key type- and size- related functionality. > > - @type master_node: string > + @type master_node: L{qa_config._QaNode} > @param master_node: The cluster master. > > """ > > >> >> >>> + >> >> >>> + """ >>> + # Helper fn to avoid specifying base params too many times >>> + def _RenewWithParams(new_params, verify=True, fail=False): >>> + AssertCommand(["gnt-cluster", "renew-crypto", "--new-ssh-keys", >>> "-f", >>> + "--no-ssh-key-check"] + new_params, fail=fail) >>> + if not fail and verify: >>> + AssertCommand(["gnt-cluster", "verify"]) >>> + >>> + # And the actual tests >>> + with qa_config.AcquireManyNodesCtx(1, exclude=[master_node]) as nodes: >>> + node_name = nodes[0].primary >>> + >>> + # Another helper function for checking whether a specific key can >>> log in >>> + def _CheckLoginWithKey(key_path, fail=False): >>> + AssertCommand(["ssh", "-oIdentityFile=%s" % key_path, >>> "-oBatchMode=yes", >>> + "-oStrictHostKeyChecking=no", >>> "-oIdentitiesOnly=yes", >>> + "-F/dev/null", node_name, "true"], >>> + fail=fail, forward_agent=False) >>> + >>> >> Nice to actually test that! >> >> >>> + # First test the simplest change >>> + _RenewWithParams([]) >>> + >>> + _RenewWithParams(["--ssh-key-type=dsa"]) >>> + _CheckLoginWithKey(".ssh/id_dsa") >>> + # Stash the key for now >>> + old_key_backup = qa_utils.BackupFile(master_node.primary, >>> ".ssh/id_dsa") >>> + >>> + try: >>> + _RenewWithParams(["--ssh-key-type=rsa"]) >>> + _CheckLoginWithKey(".ssh/id_rsa") >>> + # And check that we cannot log in with the old key >>> + _CheckLoginWithKey(old_key_backup, fail=True) >>> + finally: >>> + AssertCommand(["rm", "-f", old_key_backup]) >>> + >>> + _RenewWithParams(["--ssh-key-bits=4096"]) >>> + _RenewWithParams(["--ssh-key-bits=521"], fail=True) >>> + >>> + # Restore the cluster to its pristine state, skipping the verify as >>> we did >>> + # way too many already >>> + _RenewWithParams(["--ssh-key-type=rsa", "--ssh-key-bits=2048"], >>> + verify=False) >>> + >>> + >>> def TestClusterRenewCrypto(): >>> """gnt-cluster renew-crypto""" >>> master = qa_config.GetMasterNode() >>> @@ -1266,9 +1316,8 @@ def TestClusterRenewCrypto(): >>> _AssertSsconfCertFiles() >>> AssertCommand(["gnt-cluster", "verify"]) >>> >>> - # Only renew SSH keys >>> - AssertCommand(["gnt-cluster", "renew-crypto", "--force", >>> - "--new-ssh-keys", "--no-ssh-key-check"]) >>> + # Comprehensively test various types of SSH key changes >>> + _TestSSHKeyChanges(master) >>> >>> # Restore RAPI certificate >>> AssertCommand(["gnt-cluster", "renew-crypto", "--force", >>> -- >>> 2.6.0.rc2.230.g3dd15c0 >>> >>> >> Nice patch, LGTM. >> > -- >> >> 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. > > -- 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.
