Diff comments:
> diff --git a/cloudinit/gpg.py b/cloudinit/gpg.py
> index d58d73e..c1cebd2 100644
> --- a/cloudinit/gpg.py
> +++ b/cloudinit/gpg.py
> @@ -25,16 +27,46 @@ def export_armour(key):
> return armour
>
>
> -def recv_key(key, keyserver):
> - """Receive gpg key from the specified keyserver"""
> - LOG.debug('Receive gpg key "%s"', key)
> - try:
> - util.subp(["gpg", "--keyserver", keyserver, "--recv", key],
> - capture=True)
> - except util.ProcessExecutionError as error:
> - raise ValueError(('Failed to import key "%s" '
> - 'from server "%s" - error %s') %
> - (key, keyserver, error))
> +def recv_key(key, keyserver, retries=(1, 1)):
> + """Receive gpg key from the specified keyserver.
> +
> + Retries are done by default because keyservers can be unreliable.
> + Additionally, there is no way to determine the difference between
> + a non-existant key and a failure. In both cases gpg (at least 2.2.4)
> + exits with status 2 and stderr: "keyserver receive failed: No data"
> + It is assumed that a key provided to cloud-init exists on the keyserver
> + so re-trying makes better sense than failing.
> +
> + @param key: a string key fingerprint (as passed to gpg --recv-keys).
> + @param keyserver: the keyserver to request keys from.
> + @param retries: an interable of sleep lengths for retries.
> + Use None to indicate no retries."""
> + LOG.debug("Importing key '%s' from keyserver '%s'", key, keyserver)
> + cmd = ["gpg", "--keyserver=%s" % keyserver, "--recv-keys", key]
> + if retries is None:
> + retries = []
> + trynum = 0
> + error = None
> + sleeps = iter(retries)
> + while True:
> + trynum += 1
> + try:
> + util.subp(cmd, capture=True)
> + LOG.debug("Imported key '%s' from keyserver '%s' on try %d",
> + key, keyserver, trynum)
> + return
> + except util.ProcessExecutionError as e:
> + error = e
> + try:
> + naplen = next(sleeps)
> + LOG.debug(
> + "Import failed with exit code %d, will try again in %ss",
that feels just longer with no added information. "try again in 10s" is pretty
clear.
no?
> + error.exit_code, naplen)
> + time.sleep(naplen)
> + except StopIteration:
> + raise ValueError(
> + "Failed to import key '%s' from keyserver '%s': %s" %
added number of tries to the value error. this is only the final try (when
stopiteration is raised).
> + (key, keyserver, error))
>
>
> def delete_key(key):
> diff --git a/cloudinit/tests/test_gpg.py b/cloudinit/tests/test_gpg.py
> new file mode 100644
> index 0000000..810dbe9
> --- /dev/null
> +++ b/cloudinit/tests/test_gpg.py
> @@ -0,0 +1,61 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +"""Test gpg module."""
> +
> +from cloudinit import gpg
> +from cloudinit import util
> +from cloudinit.tests.helpers import CiTestCase
> +
> +import mock
> +
> +
> +class TestReceiveKeys(CiTestCase):
> + """Test the recv_key method."""
> +
moved mock.patch of these two common things to the class.
the issue with this is it is just a pita if you want to add a test and *not*
mock them.
> + @mock.patch("cloudinit.gpg.time.sleep")
> + @mock.patch("cloudinit.gpg.util.subp")
> + def test_retries_on_subp_exc(self, m_subp, m_sleep):
> + """retry should be done on gpg receive keys failure."""
> + retries = (1, 2, 4)
> + my_exc = util.ProcessExecutionError(
> + stdout='', stderr='', exit_code=2, cmd=['mycmd'])
> + m_subp.side_effect = (my_exc, my_exc, ('', ''))
> + gpg.recv_key("ABCD", "keyserver.example.com", retries=retries)
> + self.assertEqual([mock.call(1), mock.call(2)],
> m_sleep.call_args_list)
> +
> + @mock.patch("cloudinit.gpg.time.sleep")
> + @mock.patch("cloudinit.gpg.util.subp")
> + def test_raises_error_after_retries(self, m_subp, m_sleep):
> + """If the final run fails, error should be raised."""
> + retries = (1, )
done.
> + my_exc = util.ProcessExecutionError(
> + stdout='', stderr='', exit_code=2, cmd=['mycmd'])
> + m_subp.side_effect = (my_exc)
> + keyid, keyserver = ("ABCD", "keyserver.example.com")
> + with self.assertRaises(ValueError) as rcm:
> + gpg.recv_key(keyid, keyserver, retries=retries)
> + self.assertIn(keyid, str(rcm.exception))
> + self.assertIn(keyserver, str(rcm.exception))
> + m_sleep.assert_called_with(1)
> +
> + @mock.patch("cloudinit.gpg.time.sleep")
> + @mock.patch("cloudinit.gpg.util.subp")
> + def test_no_retries_on_none(self, m_subp, m_sleep):
> + """retry should not be done if retries is None."""
> + m_subp.side_effect = util.ProcessExecutionError(
> + stdout='', stderr='', exit_code=2, cmd=['mycmd'])
> + with self.assertRaises(ValueError):
> + gpg.recv_key("ABCD", "keyserver.example.com", retries=None)
> + m_sleep.assert_not_called()
> +
> + @mock.patch("cloudinit.gpg.time.sleep")
> + @mock.patch("cloudinit.gpg.util.subp")
> + def test_expected_gpg_command(self, m_subp, m_sleep):
> + """Verify gpg is called with expected args."""
> + key, keyserver = ("DEADBEEF", "keyserver.example.com")
> + retries = (1, 2, 4)
> + m_subp.return_value = ('', '')
> + gpg.recv_key(key, keyserver, retries=retries)
> + m_subp.assert_called_once_with(
> + ['gpg', '--keyserver=%s' % keyserver, '--recv-keys', key],
> + capture=True)
> + m_sleep.assert_not_called()
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/348711
Your team cloud-init commiters is requested to review the proposed merge of
~smoser/cloud-init:fix/gpg-receive-retry into cloud-init:master.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp