zuul/zuul
Code Issues Proposed changes

Move SQL web handler to driver

Browse Source 
The only rest API endpoint that uses sql queries is
/api/tenant/{tenant}/builds.  There's no connection in there, which
means it doesn't make sense for that to be attached to a sql connection
(which is currently the case).  Moreover, it doesn't make sense for
*every* tenant's endpoint to be attached to the *same* connection.

In other words, the current situation only allows for a single sql
connection system-wide, even if someone is using different connections
per tenant.

Moving the handler for the endpoint into the sql driver means that it
can dispatch the query to the appropriate connection for a given tenant
(since a tenant is always implied by the REST endpoint).

Moreover, the *rest* of the system actually allows multiple connections
within a single tenant, and we should support that here, but I don't
immediately have a solution of how to handle pagination across queries
that span multiple connections.  This is an improvement in that it is
now tenant-scoped, but it's not ideal.

This also removes the (undocumented!) sql_connection_name config file
option.

It also uses the tenant name from the path to constructe the query so
that it always includes the correct tenant (this eliminates the
inadvertant ability for one tenant to query another tenant's builds).

The internal API here isn't great, but it will get cleaned up in the
next patch which converts to cherrypy.

Change-Id: Ie1f19f0b392d4c010ef43dc6220ff1c8667f5a4a
This commit is contained in:
James E. Blair 2018-05-12 17:33:56 -07:00
parent a2a2ed90ea
commit 46e48d7b97
11 changed files with 94 additions and 97 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
etc/zuul.conf-sample
View File

@ -38,7 +38,6 @@ trusted_rw_paths=/opt/zuul-logs
listen_address=127.0.0.1
port=9000
static_cache_expiry=0
;sql_connection_name=mydatabase
status_url=https://zuul.example.com/status
[connection gerrit]

6
tests/base.py
View File

@ -1867,16 +1867,18 @@ class WebProxyFixture(fixtures.Fixture):
class ZuulWebFixture(fixtures.Fixture):
def __init__(self, gearman_server_port):
def __init__(self, gearman_server_port, connections):
super(ZuulWebFixture, self).__init__()
self.gearman_server_port = gearman_server_port
self.connections = connections
def _setUp(self):
# Start the web server
self.web = zuul.web.ZuulWeb(
listen_address='127.0.0.1', listen_port=0,
gear_server='127.0.0.1', gear_port=self.gearman_server_port,
info=zuul.model.WebInfo())
info=zuul.model.WebInfo(),
_connections=self.connections)
loop = asyncio.new_event_loop()
loop.set_debug(True)
ws_thread = threading.Thread(target=self.web.run, args=(loop,))

3
tests/unit/test_github_driver.py
View File

@ -789,7 +789,8 @@ class TestGithubWebhook(ZuulTestCase):
self.web = zuul.web.ZuulWeb(
listen_address='127.0.0.1', listen_port=0,
gear_server='127.0.0.1', gear_port=self.gearman_server.port,
connections=[self.fake_github])
connections=[self.fake_github],
_connections=self.connections)
loop = asyncio.new_event_loop()
loop.set_debug(True)
ws_thread = threading.Thread(target=self.web.run, args=(loop,))

6
tests/unit/test_streaming.py
View File

@ -282,7 +282,8 @@ class TestStreaming(tests.base.AnsibleZuulTestCase):
web_server = zuul.web.ZuulWeb(
listen_address='::', listen_port=0,
gear_server='127.0.0.1', gear_port=self.gearman_server.port,
static_path=tempfile.gettempdir())
static_path=tempfile.gettempdir(),
_connections=self.connections)
loop = asyncio.new_event_loop()
loop.set_debug(True)
ws_thread = threading.Thread(target=web_server.run, args=(loop,))
@ -372,7 +373,8 @@ class TestStreaming(tests.base.AnsibleZuulTestCase):
web_server = zuul.web.ZuulWeb(
listen_address='::', listen_port=0,
gear_server='127.0.0.1', gear_port=self.gearman_server.port,
static_path=tempfile.gettempdir())
static_path=tempfile.gettempdir(),
_connections=self.connections)
loop = asyncio.new_event_loop()
loop.set_debug(True)
ws_thread = threading.Thread(target=web_server.run, args=(loop,))

3
tests/unit/test_web.py
View File

@ -63,7 +63,8 @@ class BaseTestWeb(ZuulTestCase):
listen_address='127.0.0.1', listen_port=0,
gear_server='127.0.0.1', gear_port=self.gearman_server.port,
info=zuul.model.WebInfo.fromConfig(self.zuul_ini_config),
connections=self.connections.connections.values()
connections=self.connections.connections.values(),
_connections=self.connections
)
loop = asyncio.new_event_loop()
loop.set_debug(True)

13
tests/unit/test_web_urls.py
View File

@ -23,13 +23,14 @@ from tests.base import ZuulTestCase, WebProxyFixture
from tests.base import ZuulWebFixture
class TestWebURLs(object):
class TestWebURLs(ZuulTestCase):
tenant_config_file = 'config/single-tenant/main.yaml'
def setUp(self):
super(TestWebURLs, self).setUp()
self.web = self.useFixture(
ZuulWebFixture(self.gearman_server.port))
ZuulWebFixture(self.gearman_server.port,
self.connections))
def _get(self, port, uri):
url = "http://localhost:{}{}".format(port, uri)
@ -60,7 +61,7 @@ class TestWebURLs(object):
self._get(self.port, link)
class TestDirect(TestWebURLs, ZuulTestCase):
class TestDirect(TestWebURLs):
# Test directly accessing the zuul-web server with no proxy
def setUp(self):
super(TestDirect, self).setUp()
@ -70,7 +71,7 @@ class TestDirect(TestWebURLs, ZuulTestCase):
self._crawl('/t/tenant-one/status.html')
class TestWhiteLabel(TestWebURLs, ZuulTestCase):
class TestWhiteLabel(TestWebURLs):
# Test a zuul-web behind a whitelabel proxy (i.e., what
# zuul.openstack.org does).
def setUp(self):
@ -85,7 +86,7 @@ class TestWhiteLabel(TestWebURLs, ZuulTestCase):
self._crawl('/status.html')
class TestWhiteLabelAPI(TestWebURLs, ZuulTestCase):
class TestWhiteLabelAPI(TestWebURLs):
# Test a zuul-web behind a whitelabel proxy (i.e., what
# zuul.openstack.org does).
def setUp(self):
@ -103,7 +104,7 @@ class TestWhiteLabelAPI(TestWebURLs, ZuulTestCase):
self.assertEqual('tenant-one', info['info']['tenant'])
class TestSuburl(TestWebURLs, ZuulTestCase):
class TestSuburl(TestWebURLs):
# Test a zuul-web mounted on a suburl (i.e., what software factory
# does).
def setUp(self):

1
zuul/cmd/web.py
View File

@ -55,6 +55,7 @@ class WebServer(zuul.cmd.ZuulDaemonApp):
params['ssl_cert'] = get_default(self.config, 'gearman', 'ssl_cert')
params['ssl_ca'] = get_default(self.config, 'gearman', 'ssl_ca')
params['_connections'] = self.connections
params['connections'] = []
# Validate config here before we spin up the ZuulWeb object
for conn_name, connection in self.connections.connections.items():

63
zuul/driver/sql/__init__.py
View File

@ -12,6 +12,10 @@
# License for the specific language governing permissions and limitations
# under the License.
import logging
from aiohttp import web
import urllib.parse
from zuul.driver import Driver, ConnectionInterface, ReporterInterface
from zuul.driver.sql import sqlconnection
from zuul.driver.sql import sqlreporter
@ -19,6 +23,28 @@ from zuul.driver.sql import sqlreporter
class SQLDriver(Driver, ConnectionInterface, ReporterInterface):
name = 'sql'
log = logging.getLogger("zuul.SQLDriver")
def __init__(self):
self.tenant_connections = {}
def reconfigure(self, tenant):
# NOTE(corvus): This stores the connection of the first
# reporter seen for each tenant; we should figure out how to
# support multiple connections for a tenant (how do we deal
# with pagination of queries across multiple connections), or
# otherwise, require there only be one connection in a tenant.
if tenant.name in self.tenant_connections:
del self.tenant_connections[tenant.name]
for pipeline in tenant.layout.pipelines.values():
reporters = (pipeline.start_actions + pipeline.success_actions
+ pipeline.failure_actions
+ pipeline.merge_failure_actions)
for reporter in reporters:
if not isinstance(reporter, sqlreporter.SQLReporter):
continue
self.tenant_connections[tenant.name] = reporter.connection
return
def registerScheduler(self, scheduler):
self.sched = scheduler
@ -31,3 +57,40 @@ class SQLDriver(Driver, ConnectionInterface, ReporterInterface):
def getReporterSchema(self):
return sqlreporter.getSchema()
# TODO(corvus): these are temporary, remove after cherrypy conversion
def setEventLoop(self, event_loop):
self.event_loop = event_loop
async def handleRequest(self, request):
tenant_name = request.match_info["tenant"]
connection = self.tenant_connections.get(tenant_name)
if not connection:
return
try:
args = {
'buildset_filters': {},
'build_filters': {},
'limit': 50,
'skip': 0,
'tenant': tenant_name,
}
for k, v in urllib.parse.parse_qsl(request.rel_url.query_string):
if k in ("project", "pipeline", "change", "branch",
"patchset", "ref", "newrev"):
args['buildset_filters'].setdefault(k, []).append(v)
elif k in ("uuid", "job_name", "voting", "node_name",
"result"):
args['build_filters'].setdefault(k, []).append(v)
elif k in ("limit", "skip"):
args[k] = int(v)
else:
raise ValueError("Unknown parameter %s" % k)
data = await connection.get_builds(args, self.event_loop)
resp = web.json_response(data)
resp.headers['Access-Control-Allow-Origin'] = '*'
except Exception as e:
self.log.exception("Jobs exception:")
resp = web.json_response({'error_description': 'Internal error'},
status=500)
return resp

82
zuul/driver/sql/sqlconnection.py
View File

@ -15,19 +15,15 @@
import asyncio
import logging
from aiohttp import web
import alembic
import alembic.command
import alembic.config
import sqlalchemy as sa
import sqlalchemy.pool
from sqlalchemy.sql import select
import urllib.parse
import voluptuous
from zuul.connection import BaseConnection
from zuul.lib.config import get_default
from zuul.web.handler import BaseTenantWebHandler
BUILDSET_TABLE = 'zuul_buildset'
BUILD_TABLE = 'zuul_build'
@ -127,53 +123,13 @@ class SQLConnection(BaseConnection):
return zuul_buildset_table, zuul_build_table
def getWebHandlers(self, zuul_web, info):
info.capabilities.job_history = True
return [
SqlWebHandler(self, zuul_web, 'GET', 'builds'),
]
def validateWebConfig(self, config, connections):
sql_conn_name = get_default(config, 'web', 'sql_connection_name')
if sql_conn_name:
# The config wants a specific sql connection. Check the whole
# list of connections to make sure it can be satisfied.
sql_conn = connections.connections.get(sql_conn_name)
if not sql_conn:
raise Exception(
"Couldn't find sql connection '%s'" % sql_conn_name)
if self.connection_name == sql_conn.connection_name:
return True
else:
# Check to see if there is more than one connection
conn_objects = [c for c in connections.connections.values()
if isinstance(c, SQLConnection)]
if len(conn_objects) > 1:
raise Exception("Multiple sql connection found, "
"set the sql_connection_name option "
"in zuul.conf [web] section")
return True
def onStop(self):
self.log.debug("Stopping SQL connection %s" % self.connection_name)
self.engine.dispose()
class SqlWebHandler(BaseTenantWebHandler):
log = logging.getLogger("zuul.web.SqlHandler")
filters = ("project", "pipeline", "change", "branch", "patchset", "ref",
"result", "uuid", "job_name", "voting", "node_name", "newrev")
def __init__(self, connection, zuul_web, method, path):
super(SqlWebHandler, self).__init__(
connection=connection, zuul_web=zuul_web, method=method, path=path)
def setEventLoop(self, event_loop):
self.event_loop = event_loop
def query(self, args):
build = self.connection.zuul_build_table
buildset = self.connection.zuul_buildset_table
build = self.zuul_build_table
buildset = self.zuul_buildset_table
query = select([
buildset.c.project,
buildset.c.branch,
@ -201,12 +157,12 @@ class SqlWebHandler(BaseTenantWebHandler):
return query.limit(args['limit']).offset(args['skip']).order_by(
build.c.id.desc())
async def get_builds(self, args):
async def get_builds(self, args, event_loop):
"""Return a list of build"""
builds = []
with self.connection.engine.begin() as conn:
with self.engine.begin() as conn:
query = self.query(args)
query_task = self.event_loop.run_in_executor(
query_task = event_loop.run_in_executor(
None,
conn.execute,
query
@ -229,34 +185,6 @@ class SqlWebHandler(BaseTenantWebHandler):
builds.append(build)
return builds
async def handleRequest(self, request):
try:
args = {
'buildset_filters': {},
'build_filters': {},
'limit': 50,
'skip': 0,
}
for k, v in urllib.parse.parse_qsl(request.rel_url.query_string):
if k in ("tenant", "project", "pipeline", "change", "branch",
"patchset", "ref", "newrev"):
args['buildset_filters'].setdefault(k, []).append(v)
elif k in ("uuid", "job_name", "voting", "node_name",
"result"):
args['build_filters'].setdefault(k, []).append(v)
elif k in ("limit", "skip"):
args[k] = int(v)
else:
raise ValueError("Unknown parameter %s" % k)
data = await self.get_builds(args)
resp = web.json_response(data)
resp.headers['Access-Control-Allow-Origin'] = '*'
except Exception as e:
self.log.exception("Jobs exception:")
resp = web.json_response({'error_description': 'Internal error'},
status=500)
return resp
def getSchema():
sql_connection = voluptuous.Any(str, voluptuous.Schema(dict))

6
zuul/web/__init__.py
View File

@ -265,6 +265,7 @@ class ZuulWeb(object):
ssl_key=None, ssl_cert=None, ssl_ca=None,
static_cache_expiry=3600,
connections=None,
_connections=None,
info=None,
static_path=None):
self.start_time = time.time()
@ -287,6 +288,7 @@ class ZuulWeb(object):
for connection in connections:
self._connection_handlers.extend(
connection.getWebHandlers(self, self.info))
self.connections = _connections
self._plugin_routes.extend(self._connection_handlers)
async def _handleWebsocket(self, request):
@ -349,6 +351,7 @@ class ZuulWeb(object):
thread event loop is used. This should be supplied if ZuulWeb
is run within a separate (non-main) thread.
"""
sql_driver = self.connections.drivers['sql']
routes = [
('GET', '/api/info', self._handleRootInfo),
('GET', '/api/tenants', self._handleTenantsRequest),
@ -361,6 +364,8 @@ class ZuulWeb(object):
self._handleWebsocket),
('GET', '/api/tenant/{tenant}/key/{project:.*}.pub',
self._handleKeyRequest),
('GET', '/api/tenant/{tenant}/builds',
sql_driver.handleRequest),
]
static_routes = [
@ -385,6 +390,7 @@ class ZuulWeb(object):
self.event_loop = loop
self.log_streaming_handler.setEventLoop(loop)
self.gearman_handler.setEventLoop(loop)
sql_driver.setEventLoop(loop)
for handler in self._connection_handlers:
if hasattr(handler, 'setEventLoop'):

7
zuul/web/handler.py
View File

@ -31,13 +31,6 @@ class BaseWebHandler(object, metaclass=abc.ABCMeta):
"""Process a web request."""
class BaseTenantWebHandler(BaseWebHandler):
def __init__(self, connection, zuul_web, method, path):
super(BaseTenantWebHandler, self).__init__(
connection, zuul_web, method, '/api/tenant/{tenant}/' + path)
class BaseDriverWebHandler(BaseWebHandler):
def __init__(self, connection, zuul_web, method, path):