Merge ~cjwatson/launchpad-buildd:revoke-proxy-token-timeout into launchpad-buildd:master

[Colin Watson]

Add a timeout when revoking proxy tokens

We should have a timeout when making external requests so that builders don't get tied up indefinitely if the external service in question doesn't respond for whatever reason. The builder proxy applies a maximum lifetime to tokens, so the worst case of failing to revoke a token is that it ends up being valid for a little longer than necessary.

[Jürgen Gmach (jugmac00)]

Indeed. All network requests should have a timeout.

Having a look at the docstring of `urlopen`, the timeout is set by default to `socket._GLOBAL_DEFAULT_TIMEOUT`.

I tried to find out more about that global timeout, but it looks like this is just a placeholder, ie `object()`. The docstring also is not really helpful. By chance, did you encounter this and do you know where this default value is set and if there is even a number?

review: Approve

[Colin Watson (cjwatson)]

As far as I can tell, the purpose of `_GLOBAL_DEFAULT_TIMEOUT` is just to be a sentinel object so that the `socket` module can distinguish `timeout=None` from not passing the keyword argument at all. If you don't pass the `timeout` argument at all, then you get whatever `socket.getdefaulttimeout` returns, which applications can change using `socket.setdefaulttimeout`. If you pass `timeout=None`, then you get a blocking socket with no timeout even if `socket.setdefaulttimeout` had previously been called.

diff --git a/debian/changelog b/debian/changelog
index fbd0d9a..4256c57 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -4,6 +4,9 @@ launchpad-buildd (216) UNRELEASED; urgency=medium
   * Allow specifying target architecture for snaps via
     SNAPCRAFT_BUILD_TO environment variable
 
+  [ Colin Watson ]
+  * Add a timeout when revoking proxy tokens.
+
  -- Andrey Fedoseev <[e-mail]>  Mon, 27 Jun 2022 12:32:05 +0500
 
 launchpad-buildd (215) focal; urgency=medium
diff --git a/lpbuildd/proxy.py b/lpbuildd/proxy.py
index 7aadbfa..9c56780 100644
--- a/lpbuildd/proxy.py
+++ b/lpbuildd/proxy.py
@@ -249,7 +249,7 @@ class BuildManagerProxyMixin:
         req = Request(self.revocation_endpoint, None, headers)
         req.get_method = lambda: "DELETE"
         try:
-            urlopen(req)
+            urlopen(req, timeout=15)
         except (HTTPError, URLError) as e:
             self._builder.log(
                 f"Unable to revoke token for {url.username}: {e}")
diff --git a/lpbuildd/tests/test_charm.py b/lpbuildd/tests/test_charm.py
index 72b4e0c..4ef2281 100644
--- a/lpbuildd/tests/test_charm.py
+++ b/lpbuildd/tests/test_charm.py
@@ -207,8 +207,10 @@ class TestCharmBuildManagerIteration(TestCase):
         self.buildmanager.proxy_url = "http://username:password@proxy_url"
         self.buildmanager.revokeProxyToken()
         self.assertEqual(1, urlopen_mock.call_count)
-        request = urlopen_mock.call_args[0][0]
+        args, kwargs = urlopen_mock.call_args
+        request = args[0]
         self.assertEqual(
             {'Authorization': "Basic dXNlcm5hbWU6cGFzc3dvcmQ="},
             request.headers)
         self.assertEqual('http://revoke_endpoint', request.get_full_url())
+        self.assertEqual({"timeout": 15}, kwargs)
diff --git a/lpbuildd/tests/test_snap.py b/lpbuildd/tests/test_snap.py
index c2bce38..7791584 100644
--- a/lpbuildd/tests/test_snap.py
+++ b/lpbuildd/tests/test_snap.py
@@ -536,8 +536,10 @@ class TestSnapBuildManagerIteration(TestCase):
         self.buildmanager.proxy_url = "http://username:password@proxy_url"
         self.buildmanager.revokeProxyToken()
         self.assertEqual(1, urlopen_mock.call_count)
-        request = urlopen_mock.call_args[0][0]
+        args, kwargs = urlopen_mock.call_args
+        request = args[0]
         self.assertEqual(
             {'Authorization': "Basic dXNlcm5hbWU6cGFzc3dvcmQ="},
             request.headers)
         self.assertEqual('http://revoke_endpoint', request.get_full_url())
+        self.assertEqual({"timeout": 15}, kwargs)