Split github hook ingest and processing
We were taking in the hook and processing the content fully while the connection to github remained open. This created a long delay and blocked the thread from continuing, which can lead to timeouts. This change splits things apart to model gerrit a bit more, so that when a hook comes in, we minimally validate it and toss it into a queue, so that we close the connection quickly. A second thread will iterate over the queue to process the (potential) events. This change drops handling of ping events, which would validate if the project is one we know about. A follow up change will introduce project validation at a higher level. Change-Id: I463f4b888be056a3e2175ccdab0286d2ef4fa2b2 Signed-off-by: Jesse Keating <omgjlk@us.ibm.com>
This commit is contained in:
parent
b0e9fc711a
commit
64d2901084
|
@ -2145,6 +2145,7 @@ class ZuulTestCase(BaseTestCase):
|
||||||
def getGithubConnection(driver, name, config):
|
def getGithubConnection(driver, name, config):
|
||||||
con = FakeGithubConnection(driver, name, config,
|
con = FakeGithubConnection(driver, name, config,
|
||||||
upstream_root=self.upstream_root)
|
upstream_root=self.upstream_root)
|
||||||
|
self.event_queues.append(con.event_queue)
|
||||||
setattr(self, 'fake_' + name, con)
|
setattr(self, 'fake_' + name, con)
|
||||||
return con
|
return con
|
||||||
|
|
||||||
|
|
|
@ -17,6 +17,7 @@ import re
|
||||||
from testtools.matchers import MatchesRegex, StartsWith
|
from testtools.matchers import MatchesRegex, StartsWith
|
||||||
import urllib
|
import urllib
|
||||||
import time
|
import time
|
||||||
|
from unittest import skip
|
||||||
|
|
||||||
import git
|
import git
|
||||||
|
|
||||||
|
@ -685,6 +686,8 @@ class TestGithubDriver(ZuulTestCase):
|
||||||
# New timestamp should be greater than the old timestamp
|
# New timestamp should be greater than the old timestamp
|
||||||
self.assertLess(old, new)
|
self.assertLess(old, new)
|
||||||
|
|
||||||
|
# TODO(jlk): Make this a more generic test for unknown project
|
||||||
|
@skip("Skipped for rewrite of webhook handler")
|
||||||
@simple_layout('layouts/basic-github.yaml', driver='github')
|
@simple_layout('layouts/basic-github.yaml', driver='github')
|
||||||
def test_ping_event(self):
|
def test_ping_event(self):
|
||||||
# Test valid ping
|
# Test valid ping
|
||||||
|
|
|
@ -17,6 +17,8 @@ import datetime
|
||||||
import logging
|
import logging
|
||||||
import hmac
|
import hmac
|
||||||
import hashlib
|
import hashlib
|
||||||
|
import queue
|
||||||
|
import threading
|
||||||
import time
|
import time
|
||||||
import re
|
import re
|
||||||
|
|
||||||
|
@ -80,11 +82,10 @@ class GithubWebhookListener():
|
||||||
delivery=delivery))
|
delivery=delivery))
|
||||||
|
|
||||||
self._validate_signature(request)
|
self._validate_signature(request)
|
||||||
|
# TODO(jlk): Validate project in the request is a project we know
|
||||||
|
|
||||||
try:
|
try:
|
||||||
self.__dispatch_event(request)
|
self.__dispatch_event(request)
|
||||||
except webob.exc.HTTPNotFound:
|
|
||||||
raise
|
|
||||||
except:
|
except:
|
||||||
self.log.exception("Exception handling Github event:")
|
self.log.exception("Exception handling Github event:")
|
||||||
|
|
||||||
|
@ -97,21 +98,59 @@ class GithubWebhookListener():
|
||||||
raise webob.exc.HTTPBadRequest('Please specify a X-Github-Event '
|
raise webob.exc.HTTPBadRequest('Please specify a X-Github-Event '
|
||||||
'header.')
|
'header.')
|
||||||
|
|
||||||
try:
|
|
||||||
method = getattr(self, '_event_' + event)
|
|
||||||
except AttributeError:
|
|
||||||
message = "Unhandled X-Github-Event: {0}".format(event)
|
|
||||||
self.log.debug(message)
|
|
||||||
# Returns empty 200 on unhandled events
|
|
||||||
raise webob.exc.HTTPOk()
|
|
||||||
|
|
||||||
try:
|
try:
|
||||||
json_body = request.json_body
|
json_body = request.json_body
|
||||||
|
self.connection.addEvent(json_body, event)
|
||||||
except:
|
except:
|
||||||
message = 'Exception deserializing JSON body'
|
message = 'Exception deserializing JSON body'
|
||||||
self.log.exception(message)
|
self.log.exception(message)
|
||||||
raise webob.exc.HTTPBadRequest(message)
|
raise webob.exc.HTTPBadRequest(message)
|
||||||
|
|
||||||
|
def _validate_signature(self, request):
|
||||||
|
secret = self.connection.connection_config.get('webhook_token', None)
|
||||||
|
if secret is None:
|
||||||
|
raise RuntimeError("webhook_token is required")
|
||||||
|
|
||||||
|
body = request.body
|
||||||
|
try:
|
||||||
|
request_signature = request.headers['X-Hub-Signature']
|
||||||
|
except KeyError:
|
||||||
|
raise webob.exc.HTTPUnauthorized(
|
||||||
|
'Please specify a X-Hub-Signature header with secret.')
|
||||||
|
|
||||||
|
payload_signature = _sign_request(body, secret)
|
||||||
|
|
||||||
|
self.log.debug("Payload Signature: {0}".format(str(payload_signature)))
|
||||||
|
self.log.debug("Request Signature: {0}".format(str(request_signature)))
|
||||||
|
if not hmac.compare_digest(
|
||||||
|
str(payload_signature), str(request_signature)):
|
||||||
|
raise webob.exc.HTTPUnauthorized(
|
||||||
|
'Request signature does not match calculated payload '
|
||||||
|
'signature. Check that secret is correct.')
|
||||||
|
|
||||||
|
return True
|
||||||
|
|
||||||
|
|
||||||
|
class GithubEventConnector(threading.Thread):
|
||||||
|
"""Move events from GitHub into the scheduler"""
|
||||||
|
|
||||||
|
log = logging.getLogger("zuul.GithubEventConnector")
|
||||||
|
|
||||||
|
def __init__(self, connection):
|
||||||
|
super(GithubEventConnector, self).__init__()
|
||||||
|
self.daemon = True
|
||||||
|
self.connection = connection
|
||||||
|
self._stopped = False
|
||||||
|
|
||||||
|
def stop(self):
|
||||||
|
self._stopped = True
|
||||||
|
self.connection.addEvent(None)
|
||||||
|
|
||||||
|
def _handleEvent(self):
|
||||||
|
json_body, event_type = self.connection.getEvent()
|
||||||
|
if self._stopped:
|
||||||
|
return
|
||||||
|
|
||||||
# If there's any installation mapping information in the body then
|
# If there's any installation mapping information in the body then
|
||||||
# update the project mapping before any requests are made.
|
# update the project mapping before any requests are made.
|
||||||
installation_id = json_body.get('installation', {}).get('id')
|
installation_id = json_body.get('installation', {}).get('id')
|
||||||
|
@ -126,10 +165,18 @@ class GithubWebhookListener():
|
||||||
|
|
||||||
self.connection.installation_map[project_name] = installation_id
|
self.connection.installation_map[project_name] = installation_id
|
||||||
|
|
||||||
|
try:
|
||||||
|
method = getattr(self, '_event_' + event_type)
|
||||||
|
except AttributeError:
|
||||||
|
# TODO(jlk): Gracefully handle event types we don't care about
|
||||||
|
# instead of logging an exception.
|
||||||
|
message = "Unhandled X-Github-Event: {0}".format(event_type)
|
||||||
|
self.log.debug(message)
|
||||||
|
# Returns empty on unhandled events
|
||||||
|
return
|
||||||
|
|
||||||
try:
|
try:
|
||||||
event = method(json_body)
|
event = method(json_body)
|
||||||
except webob.exc.HTTPNotFound:
|
|
||||||
raise
|
|
||||||
except:
|
except:
|
||||||
self.log.exception('Exception when handling event:')
|
self.log.exception('Exception when handling event:')
|
||||||
event = None
|
event = None
|
||||||
|
@ -240,14 +287,6 @@ class GithubWebhookListener():
|
||||||
event.action = body.get('action')
|
event.action = body.get('action')
|
||||||
return event
|
return event
|
||||||
|
|
||||||
def _event_ping(self, body):
|
|
||||||
project_name = body['repository']['full_name']
|
|
||||||
if not self.connection.getProject(project_name):
|
|
||||||
self.log.warning("Ping received for unknown project %s" %
|
|
||||||
project_name)
|
|
||||||
raise webob.exc.HTTPNotFound("Sorry, this project is not "
|
|
||||||
"registered")
|
|
||||||
|
|
||||||
def _event_status(self, body):
|
def _event_status(self, body):
|
||||||
action = body.get('action')
|
action = body.get('action')
|
||||||
if action == 'pending':
|
if action == 'pending':
|
||||||
|
@ -277,30 +316,6 @@ class GithubWebhookListener():
|
||||||
(number, project_name))
|
(number, project_name))
|
||||||
return pr_body
|
return pr_body
|
||||||
|
|
||||||
def _validate_signature(self, request):
|
|
||||||
secret = self.connection.connection_config.get('webhook_token', None)
|
|
||||||
if secret is None:
|
|
||||||
raise RuntimeError("webhook_token is required")
|
|
||||||
|
|
||||||
body = request.body
|
|
||||||
try:
|
|
||||||
request_signature = request.headers['X-Hub-Signature']
|
|
||||||
except KeyError:
|
|
||||||
raise webob.exc.HTTPUnauthorized(
|
|
||||||
'Please specify a X-Hub-Signature header with secret.')
|
|
||||||
|
|
||||||
payload_signature = _sign_request(body, secret)
|
|
||||||
|
|
||||||
self.log.debug("Payload Signature: {0}".format(str(payload_signature)))
|
|
||||||
self.log.debug("Request Signature: {0}".format(str(request_signature)))
|
|
||||||
if not hmac.compare_digest(
|
|
||||||
str(payload_signature), str(request_signature)):
|
|
||||||
raise webob.exc.HTTPUnauthorized(
|
|
||||||
'Request signature does not match calculated payload '
|
|
||||||
'signature. Check that secret is correct.')
|
|
||||||
|
|
||||||
return True
|
|
||||||
|
|
||||||
def _pull_request_to_event(self, pr_body):
|
def _pull_request_to_event(self, pr_body):
|
||||||
event = GithubTriggerEvent()
|
event = GithubTriggerEvent()
|
||||||
event.trigger_name = 'github'
|
event.trigger_name = 'github'
|
||||||
|
@ -327,6 +342,17 @@ class GithubWebhookListener():
|
||||||
if login:
|
if login:
|
||||||
return self.connection.getUser(login)
|
return self.connection.getUser(login)
|
||||||
|
|
||||||
|
def run(self):
|
||||||
|
while True:
|
||||||
|
if self._stopped:
|
||||||
|
return
|
||||||
|
try:
|
||||||
|
self._handleEvent()
|
||||||
|
except:
|
||||||
|
self.log.exception("Exception moving GitHub event:")
|
||||||
|
finally:
|
||||||
|
self.connection.eventDone()
|
||||||
|
|
||||||
|
|
||||||
class GithubUser(collections.Mapping):
|
class GithubUser(collections.Mapping):
|
||||||
log = logging.getLogger('zuul.GithubUser')
|
log = logging.getLogger('zuul.GithubUser')
|
||||||
|
@ -376,6 +402,7 @@ class GithubConnection(BaseConnection):
|
||||||
self.canonical_hostname = self.connection_config.get(
|
self.canonical_hostname = self.connection_config.get(
|
||||||
'canonical_hostname', self.server)
|
'canonical_hostname', self.server)
|
||||||
self.source = driver.getSource(self)
|
self.source = driver.getSource(self)
|
||||||
|
self.event_queue = queue.Queue()
|
||||||
|
|
||||||
# ssl verification must default to true
|
# ssl verification must default to true
|
||||||
verify_ssl = self.connection_config.get('verify_ssl', 'true')
|
verify_ssl = self.connection_config.get('verify_ssl', 'true')
|
||||||
|
@ -408,9 +435,20 @@ class GithubConnection(BaseConnection):
|
||||||
self.registerHttpHandler(self.payload_path,
|
self.registerHttpHandler(self.payload_path,
|
||||||
webhook_listener.handle_request)
|
webhook_listener.handle_request)
|
||||||
self._authenticateGithubAPI()
|
self._authenticateGithubAPI()
|
||||||
|
self._start_event_connector()
|
||||||
|
|
||||||
def onStop(self):
|
def onStop(self):
|
||||||
self.unregisterHttpHandler(self.payload_path)
|
self.unregisterHttpHandler(self.payload_path)
|
||||||
|
self._stop_event_connector()
|
||||||
|
|
||||||
|
def _start_event_connector(self):
|
||||||
|
self.github_event_connector = GithubEventConnector(self)
|
||||||
|
self.github_event_connector.start()
|
||||||
|
|
||||||
|
def _stop_event_connector(self):
|
||||||
|
if self.github_event_connector:
|
||||||
|
self.github_event_connector.stop()
|
||||||
|
self.github_event_connector.join()
|
||||||
|
|
||||||
def _createGithubClient(self):
|
def _createGithubClient(self):
|
||||||
if self.server != 'github.com':
|
if self.server != 'github.com':
|
||||||
|
@ -504,6 +542,15 @@ class GithubConnection(BaseConnection):
|
||||||
|
|
||||||
return token
|
return token
|
||||||
|
|
||||||
|
def addEvent(self, data, event=None):
|
||||||
|
return self.event_queue.put((data, event))
|
||||||
|
|
||||||
|
def getEvent(self):
|
||||||
|
return self.event_queue.get()
|
||||||
|
|
||||||
|
def eventDone(self):
|
||||||
|
self.event_queue.task_done()
|
||||||
|
|
||||||
def getGithubClient(self,
|
def getGithubClient(self,
|
||||||
project=None,
|
project=None,
|
||||||
user_id=None,
|
user_id=None,
|
||||||
|
|
Loading…
Reference in New Issue