cloud-init-dev team mailing list archive

[Ryan Harper <ryan.harper@xxxxxxxxxxxxx>]

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.

iterable?

> +                    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",

%s seconds?

> +                error.exit_code, naplen)
> +            time.sleep(naplen)
> +        except StopIteration:
> +            raise ValueError(
> +                "Failed to import key '%s' from keyserver '%s': %s" %

No more tries? or include the number of tries so far?

> +                (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."""
> +

My preference is to do a def setUpTestCase() and use self.add_patch() to save the @mock and param passing;
Not a bit deal; just like typing less.

> +    @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, )

Nit,

naplen = 1
and use naplen in both places.

> +        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.