launchpad-reviewers team mailing list archive

[Colin Watson <mp+425211@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad-buildd:revoke-proxy-token-timeout into launchpad-buildd:master.

Commit message:
Add a timeout when revoking proxy tokens

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/425211

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:revoke-proxy-token-timeout into launchpad-buildd:master.

diff --git a/debian/changelog b/debian/changelog
index b63d1f8..537af7c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+launchpad-buildd (216) UNRELEASED; urgency=medium
+
+  * Add a timeout when revoking proxy tokens.
+
+ -- Colin Watson <cjwatson@xxxxxxxxxx>  Wed, 22 Jun 2022 10:16:58 +0100
+
 launchpad-buildd (215) focal; urgency=medium
 
   [ Jürgen Gmach ]
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 59f0446..84f1ad4 100644
--- a/lpbuildd/tests/test_snap.py
+++ b/lpbuildd/tests/test_snap.py
@@ -515,8 +515,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)