launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:limited-effort-webhook-retries into launchpad:master.

Commit message:
When dispatching webhooks, reduce the amount of retries depending on the target host

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1879687 in Launchpad itself: "Don't retry webhook deliveries to obviously-invalid addresses"
  https://bugs.launchpad.net/launchpad/+bug/1879687

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/387236
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:limited-effort-webhook-retries into launchpad:master.

diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index a670d54..e8d980f 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -14,7 +14,10 @@ from datetime import (
     datetime,
     timedelta,
     )
+import re
+import socket
 
+import ipaddress
 import iso8601
 from lazr.delegates import delegate_to
 from lazr.enum import (
@@ -23,6 +26,7 @@ from lazr.enum import (
     )
 from pytz import utc
 import six
+from six.moves.urllib.parse import urlsplit
 from storm.expr import Desc
 from storm.properties import (
     Bool,
@@ -436,6 +440,14 @@ class WebhookDeliveryJob(WebhookJobDerived):
     # retry_automatically and raising a fatal exception instead.
     max_retries = 1000
 
+    # Reduce the max retries for URL with hosts matching one of the
+    # following pattern, or to invalid IP addresses (such as localhost or
+    # multicast).
+    limited_effort_host_patterns = [
+        re.compile(r".*\.lxd$"),
+    ]
+    limited_effort_retry_period = timedelta(minutes=5)
+
     config = config.IWebhookDeliveryJobSource
 
     @classmethod
@@ -450,6 +462,30 @@ class WebhookDeliveryJob(WebhookJobDerived):
             (job, event_type, _redact_payload(event_type, payload)))
         return job
 
+    def is_limited_effort_delivery(self):
+        """Checks if the webhook delivery URL should have limited effort on
+        delivery, reducing the number of retries.
+
+        We do limited effort to deliver in any of the following situations:
+            - URL's host resolves to a loopback or invalid IP address
+            - URL's host matches one of the self.limited_effort_host_patterns
+        """
+        url = self.webhook.delivery_url
+        netloc = six.text_type(urlsplit(url).netloc)
+        if any(i.match(netloc) for i in self.limited_effort_host_patterns):
+            return True
+        try:
+            ip = ipaddress.ip_address(netloc)
+        except (ValueError, ipaddress.AddressValueError):
+            try:
+                resolved_addr = six.text_type(socket.gethostbyname(netloc))
+                ip = ipaddress.ip_address(resolved_addr)
+            except socket.error:
+                # If we could not resolve, we limit the effort to delivery.
+                return True
+        return ip.is_loopback or ip.is_unspecified or (
+                not ip.is_global and not ip.is_private)
+
     @property
     def pending(self):
         return self.job.is_pending
@@ -518,7 +554,9 @@ class WebhookDeliveryJob(WebhookJobDerived):
     def retry_automatically(self):
         if 'result' not in self.json_data:
             return False
-        if self.json_data['result'].get('connection_error') is not None:
+        if self.is_limited_effort_delivery():
+            duration = self.limited_effort_retry_period
+        elif self.json_data['result'].get('connection_error') is not None:
             duration = timedelta(days=1)
         else:
             status_code = self.json_data['result']['response']['status_code']
diff --git a/lib/lp/services/webhooks/tests/test_job.py b/lib/lp/services/webhooks/tests/test_job.py
index fde24da..ccb43e4 100644
--- a/lib/lp/services/webhooks/tests/test_job.py
+++ b/lib/lp/services/webhooks/tests/test_job.py
@@ -717,6 +717,43 @@ class TestWebhookDeliveryJob(TestCaseWithFactory):
         self.assertEqual(JobStatus.WAITING, job.status)
         self.assertIs(None, job.date_first_sent)
 
+    def assertRetriesWithLimitedEffort(self, url):
+        client = MockWebhookClient(response_status=503)
+        self.useFixture(ZopeUtilityFixture(client, IWebhookClient))
+
+        hook = self.factory.makeWebhook(delivery_url=url)
+        job = WebhookDeliveryJob.create(hook, 'test', payload={'foo': 'bar'})
+        self.assertTrue(job.is_limited_effort_delivery())
+
+        # Running it will fail because the server returns 503 error,
+        # but set the job status to WAITING, so it can be retried.
+        self.assertEqual(False, self.runJob(job))
+        self.assertEqual(JobStatus.WAITING, job.status)
+
+        # Force the next attempt to fail hard by pretending it was more
+        # than job.limited_effort_retry_period later.
+        self.assertTrue(job.retry_automatically)
+        a_while_ago = timedelta(minutes=5, seconds=1)
+        job.json_data['date_first_sent'] = (
+            job.date_first_sent - a_while_ago).isoformat()
+        self.assertFalse(job.retry_automatically)
+
+        self.assertEqual(False, self.runJob(job))
+        self.assertEqual(JobStatus.FAILED, job.status)
+
+    def test_retries_matching_url_with_limited_effort(self):
+        self.assertRetriesWithLimitedEffort(u"http://foo.lxd/path";)
+
+    def test_retries_localhost_with_limited_effort(self):
+        self.assertRetriesWithLimitedEffort(u"localhost")
+        self.assertRetriesWithLimitedEffort(u"127.0.0.1")
+
+    def test_broadcast_address_with_limited_effort(self):
+        self.assertRetriesWithLimitedEffort(u"224.0.18.255")
+
+    def test_multicast_address_with_limited_effort(self):
+        self.assertRetriesWithLimitedEffort(u"224.0.0.255")
+
 
 class TestViaCronscript(TestCaseWithFactory):