# HG changeset patch
# User osimons
# Date 1286741933 0
# Node ID 59acaa8b52c0709aa7fc7328a283235da6337639
# Parent 70aebfc3a45fb375155659b709baed9c3a8c87a9
Slave attachment support via `` is totally redone to use multi-part form post instead of inlining it in the XML (ie. like a web file upload form). For larger binaries the previous inlining would effectively be an internal denial-of-service attack...
Closes #615.
diff --git a/bitten/__init__.py b/bitten/__init__.py
--- a/bitten/__init__.py
+++ b/bitten/__init__.py
@@ -19,4 +19,4 @@
pass
# The master-slave protocol/configuration version
-PROTOCOL_VERSION = 4
+PROTOCOL_VERSION = 5
diff --git a/bitten/master.py b/bitten/master.py
--- a/bitten/master.py
+++ b/bitten/master.py
@@ -131,6 +131,8 @@
if req.args['collection'] == 'steps':
return self._process_build_step(req, config, build)
+ elif req.args['collection'] == 'attach':
+ return self._process_attachment(req, config, build)
elif req.args['collection'] == 'keepalive':
return self._process_keepalive(req, config, build)
else:
@@ -258,6 +260,7 @@
target_platform = TargetPlatform.fetch(self.env, build.platform)
xml.attr['platform'] = target_platform.name
xml.attr['name'] = build.slave
+ xml.attr['form_token'] = req.form_token # For posting attachments
body = str(xml)
self.log.info('Build slave %r initiated build %d', build.slave,
@@ -349,25 +352,6 @@
report.items.append(item)
report.insert(db=db)
- # Collect attachments from the request body
- for attach_elem in elem.children(Recipe.ATTACH):
- attach_elem = list(attach_elem.children('file'))[0] # One file only
- filename = attach_elem.attr.get('filename')
- resource_id = attach_elem.attr.get('resource') == 'config' \
- and build.config or build.resource.id
-
- try: # Delete attachment if it already exists
- old_attach = Attachment(self.env, 'build',
- parent_id=resource_id, filename=filename)
- old_attach.delete()
- except ResourceNotFound:
- pass
- attachment = Attachment(self.env, 'build', parent_id=resource_id)
- attachment.description = attach_elem.attr.get('description')
- attachment.author = req.authname
- fileobj = StringIO(attach_elem.gettext().decode('base64'))
- attachment.insert(filename, fileobj, fileobj.len, db=db)
-
# If this was the last step in the recipe we mark the build as
# completed otherwise just update last_activity
if last_step:
@@ -418,6 +402,38 @@
'Location': req.abs_href.builds(
build.id, 'steps', stepname)})
+ def _process_attachment(self, req, config, build):
+ resource_id = req.args['member'] == 'config' \
+ and build.config or build.resource.id
+ upload = req.args['file']
+ if not upload.file:
+ send_error(req, message="Attachment not received.")
+ self.log.debug('Received attachment %s for attaching to build:%s',
+ upload.filename, resource_id)
+
+ # Determine size of file
+ upload.file.seek(0, 2) # to the end
+ size = upload.file.tell()
+ upload.file.seek(0) # beginning again
+
+ # Delete attachment if it already exists
+ try:
+ old_attach = Attachment(self.env, 'build',
+ parent_id=resource_id, filename=upload.filename)
+ old_attach.delete()
+ except ResourceNotFound:
+ pass
+
+ # Save new attachment
+ attachment = Attachment(self.env, 'build', parent_id=resource_id)
+ attachment.description = req.args.get('description', '')
+ attachment.author = req.authname
+ attachment.insert(upload.filename, upload.file, size)
+
+ self._send_response(req, 201, 'Attachment created', headers={
+ 'Content-Type': 'text/plain',
+ 'Content-Length': str(len('Attachment created'))})
+
def _process_keepalive(self, req, config, build):
build.last_activity = int(time.time())
build.update()
@@ -430,7 +446,6 @@
'Content-Type': 'text/plain',
'Content-Length': str(len(body))})
-
def _start_new_step(self, build, stepname):
"""Creates the in-memory representation for a newly started
step, ready to be persisted to the database.
diff --git a/bitten/recipe.py b/bitten/recipe.py
--- a/bitten/recipe.py
+++ b/bitten/recipe.py
@@ -171,20 +171,15 @@
:param resource: which resource to attach the file to,
either 'build' (default) or 'config'
"""
- filename = self.resolve(file_)
- try:
- fileobj = open(filename, 'rb')
- try:
- xml_elem = xmlio.Element('file',
- filename=os.path.basename(filename),
- description=description,
+ # Attachments are not added as inline xml, so only adding
+ # the details for later processing.
+ if not file_:
+ self.error('No attachment file specified.')
+ return
+ xml_elem = xmlio.Element('file', filename=file_,
+ description=description or '',
resource=resource or 'build')
- xml_elem.append(fileobj.read().encode('base64'))
- self.output.append((Recipe.ATTACH, None, None, xml_elem))
- finally:
- fileobj.close()
- except IOError, e:
- self.error('Failed to read file %s as attachment' % file_)
+ self.output.append((Recipe.ATTACH, None, None, xml_elem))
def resolve(self, *path):
"""Return the path of a file relative to the base directory.
diff --git a/bitten/slave.py b/bitten/slave.py
--- a/bitten/slave.py
+++ b/bitten/slave.py
@@ -26,6 +26,7 @@
import cookielib
import threading
import os
+import mimetools
from ConfigParser import MissingSectionHeaderError
from bitten import PROTOCOL_VERSION
@@ -73,6 +74,58 @@
self.method = self.has_data() and 'POST' or 'GET'
return self.method
+
+def encode_multipart_formdata(fields):
+ """
+ Given a dictionary field parameters, returns the HTTP request body and the
+ content_type (which includes the boundary string), to be used with an
+ httplib-like call.
+
+ Normal key/value items are treated as regular parameters, but key/tuple
+ items are treated as files, where a value tuple is a (filename, data) tuple.
+
+ For example:
+ fields = {
+ 'foo': 'bar',
+ 'foofile': ('foofile.txt', 'contents of foofile'),
+ }
+ body, content_type = encode_multipart_formdata(fields)
+
+ Note: Adapted from http://code.google.com/p/urllib3/ (MIT license)
+ """
+
+ BOUNDARY = mimetools.choose_boundary()
+ ENCODE_TEMPLATE= "--%(boundary)s\r\n" \
+ "Content-Disposition: form-data; name=\"%(name)s\"\r\n" \
+ "\r\n%(value)s\r\n"
+ ENCODE_TEMPLATE_FILE = "--%(boundary)s\r\n" \
+ "Content-Disposition: form-data; name=\"%(name)s\"; " \
+ "filename=\"%(filename)s\"\r\n" \
+ "Content-Type: %(contenttype)s\r\n" \
+ "\r\n%(value)s\r\n"
+
+ body = ""
+ for key, value in fields.iteritems():
+ if isinstance(value, tuple):
+ filename, value = value
+ body += ENCODE_TEMPLATE_FILE % {
+ 'boundary': BOUNDARY,
+ 'name': str(key),
+ 'value': str(value),
+ 'filename': str(filename),
+ 'contenttype': 'application/octet-stream'
+ }
+ else:
+ body += ENCODE_TEMPLATE % {
+ 'boundary': BOUNDARY,
+ 'name': str(key),
+ 'value': str(value)
+ }
+ body += '--%s--\r\n' % BOUNDARY
+ content_type = 'multipart/form-data; boundary=%s' % BOUNDARY
+ return body, content_type
+
+
class KeepAliveThread(threading.Thread):
"A thread to periodically send keep-alive messages to the master"
@@ -401,6 +454,10 @@
failed = True
if type == Recipe.REPORT and self.dump_reports:
print output
+ if type == Recipe.ATTACH:
+ # Attachments are added out-of-band due to major
+ # performance issues with inlined base64 xml content
+ self._attach_file(build_url, recipe, output)
xml.append(xmlio.Element(type, category=category,
generator=generator)[
output
@@ -442,6 +499,32 @@
log.error('Unexpected response (%d): %s', resp.code, resp.msg)
raise ExitSlave(exit_code)
+ def _attach_file(self, build_url, recipe, attachment):
+ form_token = recipe._root.attr.get('form_token', '')
+ if self.local or self.dry_run or not form_token:
+ log.info('Attachment %s not sent due to current slave options',
+ attachment.attr['file'])
+ return
+ resource_type = attachment.attr['resource']
+ url = str(build_url + '/attach/' + resource_type)
+ path = recipe.ctxt.resolve(attachment.attr['filename'])
+ filename = os.path.basename(path)
+ log.debug('Attaching file %s to %s...', attachment.attr['filename'],
+ resource_type)
+ f = open(path)
+ try:
+ data, content_type = encode_multipart_formdata({
+ 'file': (filename, f.read()),
+ 'description': attachment.attr['description'],
+ '__FORM_TOKEN': form_token})
+ finally:
+ f.close()
+ resp = self.request('POST', url , data, {
+ 'Content-Type': content_type})
+ if not resp.code == 201:
+ msg = 'Error attaching %s to %s'
+ log.error(msg, filename, resource_type)
+ raise BuildError(msg, filename, resource_type)
class ExitSlave(Exception):
"""Exception used internally by the slave to signal that the slave process
diff --git a/bitten/tests/master.py b/bitten/tests/master.py
--- a/bitten/tests/master.py
+++ b/bitten/tests/master.py
@@ -14,8 +14,10 @@
from StringIO import StringIO
import tempfile
import unittest
+import cgi
from Cookie import SimpleCookie as Cookie
+from trac.attachment import Attachment
from trac.db import DatabaseManager
from trac.perm import PermissionCache, PermissionSystem
from trac.test import EnvironmentStub, Mock
@@ -24,6 +26,7 @@
from trac.web.href import Href
from bitten.master import BuildMaster
+from bitten.slave import encode_multipart_formdata
from bitten.model import BuildConfig, TargetPlatform, Build, BuildStep, \
BuildLog, Report, schema
from bitten import PROTOCOL_VERSION
@@ -280,6 +283,7 @@
send_response=lambda x: outheaders.setdefault('Status', x),
send_header=lambda x, y: outheaders.setdefault(x, y),
write=outbody.write,
+ form_token="12345",
incookie=Cookie('trac_auth='))
module = BuildMaster(self.env)
@@ -288,13 +292,13 @@
self.assertRaises(RequestDone, module.process_request, req)
self.assertEqual(200, outheaders['Status'])
- self.assertEqual('112', outheaders['Content-Length'])
+ self.assertEqual('131', outheaders['Content-Length'])
self.assertEqual('application/x-bitten+xml',
outheaders['Content-Type'])
self.assertEqual('attachment; filename=recipe_test_r123.xml',
outheaders['Content-Disposition'])
- self.assertEqual('',
outbody.getvalue())
@@ -531,87 +535,6 @@
'type': 'test',
}, reports[0].items[0])
- def test_process_build_step_success_with_attach(self):
- # Parse input and create attachments for config + build
- recipe = """
-
-
-
-
-"""
- BuildConfig(self.env, 'test', path='somepath', active=True,
- recipe=recipe).insert()
- build = Build(self.env, 'test', '123', 1, slave='hal', rev_time=42,
- started=42, status=Build.IN_PROGRESS)
- build.slave_info[Build.TOKEN] = '123';
- build.insert()
-
- inbody = StringIO("""
-
- aGVsbG8gYmFy\n
-
-
- aGVsbG8gYmF6\n
-
-""")
- outheaders = {}
- outbody = StringIO()
- req = Mock(method='POST', base_path='',
- path_info='/builds/%d/steps/' % build.id,
- href=Href('/trac'), abs_href=Href('http://example.org/trac'),
- remote_addr='127.0.0.1', args={},
- authname='hal',
- perm=PermissionCache(self.env, 'hal'),
- read=inbody.read,
- send_response=lambda x: outheaders.setdefault('Status', x),
- send_header=lambda x, y: outheaders.setdefault(x, y),
- write=outbody.write,
- incookie=Cookie('trac_auth=123'))
- module = BuildMaster(self.env)
-
- module._start_new_step(build, 'foo').insert()
-
- assert module.match_request(req)
-
- self.assertRaises(RequestDone, module.process_request, req)
-
- self.assertEqual(201, outheaders['Status'])
- self.assertEqual('20', outheaders['Content-Length'])
- self.assertEqual('text/plain', outheaders['Content-Type'])
- self.assertEqual('Build step processed', outbody.getvalue())
-
- build = Build.fetch(self.env, build.id)
- self.assertEqual(Build.SUCCESS, build.status)
- assert build.stopped
- assert build.stopped > build.started
-
- steps = list(BuildStep.select(self.env, build.id))
- self.assertEqual(1, len(steps))
- self.assertEqual('foo', steps[0].name)
- self.assertEqual(BuildStep.SUCCESS, steps[0].status)
-
- from trac.attachment import Attachment
- config_attachments = list(Attachment.select(self.env, 'build', 'test'))
- build_attachments = list(Attachment.select(self.env, 'build', 'test/1'))
-
- self.assertEquals(1, len(build_attachments))
- self.assertEquals('hal', build_attachments[0].author)
- self.assertEquals('bar bar', build_attachments[0].description)
- self.assertEquals('bar.txt', build_attachments[0].filename)
- self.assertEquals('hello bar',
- build_attachments[0].open().read())
-
- self.assertEquals(1, len(config_attachments))
- self.assertEquals('hal', config_attachments[0].author)
- self.assertEquals('baz baz', config_attachments[0].description)
- self.assertEquals('baz.txt', config_attachments[0].filename)
- self.assertEquals('hello baz',
- config_attachments[0].open().read())
-
def test_process_build_step_wrong_slave(self):
recipe = """
@@ -978,6 +901,129 @@
self.assertEqual(405, outheaders['Status'])
self.assertEqual('Method GET not allowed', outbody.getvalue())
+ def test_process_attach_collection_default_member(self):
+ req = Mock(args={}, path_info='/builds/12/attach/config')
+ module = BuildMaster(self.env)
+ self.assertEquals(True, module.match_request(req))
+ self.assertTrue(req.args['collection'], 'attach')
+ self.assertTrue(req.args['member'], '')
+
+ def test_process_attach_collection_config(self):
+ req = Mock(args={}, path_info='/builds/12/attach/config')
+ module = BuildMaster(self.env)
+ self.assertEquals(True, module.match_request(req))
+ self.assertTrue(req.args['collection'], 'attach')
+ self.assertTrue(req.args['member'], 'config')
+
+ def test_process_attach_collection_config(self):
+ req = Mock(args={}, path_info='/builds/12/attach/build')
+ module = BuildMaster(self.env)
+ self.assertEquals(True, module.match_request(req))
+ self.assertTrue(req.args['collection'], 'attach')
+ self.assertTrue(req.args['member'], 'build')
+
+ def test_process_attach_config(self):
+ body, content_type = encode_multipart_formdata({
+ 'description': 'baz baz',
+ 'file': ('baz.txt', 'hello baz'),
+ '__FORM_TOKEN': '123456'})
+ args = {}
+ for k, v in dict(cgi.FieldStorage(fp=StringIO(body), environ={
+ 'REQUEST_METHOD': 'POST',
+ 'CONTENT_TYPE': content_type})
+ ).items():
+ if v.filename:
+ args[k] = v
+ else:
+ args[k] = v.value
+ args.update({'collection': 'attach', 'member': 'config'})
+ self.assertTrue('file' in args)
+
+ outheaders = {}
+ outbody = StringIO()
+
+ req = Mock(args=args, form_token='123456', authname='hal',
+ remote_addr='127.0.0.1',
+ send_response=lambda x: outheaders.setdefault('Status', x),
+ send_header=lambda x, y: outheaders.setdefault(x, y),
+ write=outbody.write)
+
+ config = BuildConfig(self.env, 'test', path='somepath', active=True,
+ recipe='')
+ config.insert()
+ build = Build(self.env, 'test', '123', 1, slave='hal', rev_time=42,
+ started=42, status=Build.IN_PROGRESS)
+ build.insert()
+
+ module = BuildMaster(self.env)
+
+ self.assertRaises(RequestDone, module._process_attachment,
+ req, config, build)
+ self.assertEqual(201, outheaders['Status'])
+ self.assertEqual('18', outheaders['Content-Length'])
+ self.assertEqual('text/plain', outheaders['Content-Type'])
+ self.assertEqual('Attachment created', outbody.getvalue())
+
+ config_atts = list(Attachment.select(self.env, 'build', 'test'))
+ self.assertEquals(1, len(config_atts))
+ self.assertEquals('hal', config_atts[0].author)
+ self.assertEquals('baz baz', config_atts[0].description)
+ self.assertEquals('baz.txt', config_atts[0].filename)
+ self.assertEquals('hello baz',
+ config_atts[0].open().read())
+
+
+ def test_process_attach_build(self):
+ body, content_type = encode_multipart_formdata({
+ 'description': 'baz baz',
+ 'file': ('baz.txt', 'hello baz'),
+ '__FORM_TOKEN': '123456'})
+ args = {}
+ for k, v in dict(cgi.FieldStorage(fp=StringIO(body), environ={
+ 'REQUEST_METHOD': 'POST',
+ 'CONTENT_TYPE': content_type})
+ ).items():
+ if v.filename:
+ args[k] = v
+ else:
+ args[k] = v.value
+ args.update({'collection': 'attach', 'member': 'build'})
+ self.assertTrue('file' in args)
+
+ outheaders = {}
+ outbody = StringIO()
+
+ req = Mock(args=args, form_token='123456', authname='hal',
+ remote_addr='127.0.0.1',
+ send_response=lambda x: outheaders.setdefault('Status', x),
+ send_header=lambda x, y: outheaders.setdefault(x, y),
+ write=outbody.write)
+
+ config = BuildConfig(self.env, 'test', path='somepath', active=True,
+ recipe='')
+ config.insert()
+ build = Build(self.env, 'test', '123', 1, slave='hal', rev_time=42,
+ started=42, status=Build.IN_PROGRESS)
+ build.insert()
+
+ module = BuildMaster(self.env)
+
+ self.assertRaises(RequestDone, module._process_attachment,
+ req, config, build)
+ self.assertEqual(201, outheaders['Status'])
+ self.assertEqual('18', outheaders['Content-Length'])
+ self.assertEqual('text/plain', outheaders['Content-Type'])
+ self.assertEqual('Attachment created', outbody.getvalue())
+
+ build_atts = list(Attachment.select(self.env, 'build', 'test/1'))
+ self.assertEquals(1, len(build_atts))
+ self.assertEquals('hal', build_atts[0].author)
+ self.assertEquals('baz baz', build_atts[0].description)
+ self.assertEquals('baz.txt', build_atts[0].filename)
+ self.assertEquals('hello baz',
+ build_atts[0].open().read())
+
+
def suite():
suite = unittest.TestSuite()
diff --git a/bitten/tests_slave/recipe.py b/bitten/tests_slave/recipe.py
--- a/bitten/tests_slave/recipe.py
+++ b/bitten/tests_slave/recipe.py
@@ -45,23 +45,9 @@
except InvalidRecipeError, e:
self.failUnless("Unsupported argument 'foo'" in str(e))
- def test_attach_file_non_existing(self):
- # Verify that it raises error and that it gets logged
- ctxt = Context(self.basedir, Configuration())
- ctxt.attach(file_='nonexisting.txt',
- description='build build')
-
- self.assertEquals(1, len(ctxt.output))
- self.assertEquals(Recipe.ERROR, ctxt.output[0][0])
- self.assertEquals('Failed to read file nonexisting.txt as attachment',
- ctxt.output[0][3])
-
def test_attach_file_config(self):
# Verify output from attaching a file to a config
ctxt = Context(self.basedir, Configuration())
- test_file = open(os.path.join(self.basedir, 'config.txt'), 'w')
- test_file.write('hello config')
- test_file.close()
ctxt.attach(file_='config.txt', description='config config',
resource='config')
@@ -70,16 +56,11 @@
attach_xml = ctxt.output[0][3]
self.assertEquals(''
- 'aGVsbG8gY29uZmln\n'
- '', str(attach_xml))
+ 'filename="config.txt"/>', str(attach_xml))
def test_attach_file_build(self):
# Verify output from attaching a file to a build
ctxt = Context(self.basedir, Configuration())
- test_file = open(os.path.join(self.basedir, 'build.txt'), 'w')
- test_file.write('hello build')
- test_file.close()
ctxt.attach(file_='build.txt', description='build build')
self.assertEquals(1, len(ctxt.output))
@@ -87,9 +68,7 @@
attach_xml = ctxt.output[0][3]
self.assertEquals(''
- 'aGVsbG8gYnVpbGQ=\n'
- '', str(attach_xml))
+ 'filename="build.txt"/>', str(attach_xml))
class RecipeTestCase(unittest.TestCase):
diff --git a/bitten/tests_slave/slave.py b/bitten/tests_slave/slave.py
--- a/bitten/tests_slave/slave.py
+++ b/bitten/tests_slave/slave.py
@@ -14,6 +14,7 @@
import unittest
from bitten.slave import BuildSlave, ExitSlave
+from bitten.slave import encode_multipart_formdata
class BuildSlaveTestCase(unittest.TestCase):
@@ -33,9 +34,34 @@
def test_quit_raises(self):
self.assertRaises(ExitSlave, self.slave.quit)
+class MultiPartEncodeTestCase(unittest.TestCase):
+
+ def setUp(self):
+ self.work_dir = tempfile.mkdtemp(prefix='bitten_test')
+
+ def tearDown(self):
+ shutil.rmtree(self.work_dir)
+
+ def test_mutlipart_encode_one(self):
+ fields = {
+ 'foo': 'bar',
+ 'foofile': ('test.txt', 'contents of foofile'),
+ }
+ body, content_type = encode_multipart_formdata(fields)
+ boundary = content_type.split(';')[1].strip().split('=')[1]
+ self.assertEquals('multipart/form-data; boundary=%s' % boundary,
+ content_type)
+ self.assertEquals('--%s\r\nContent-Disposition: form-data; ' \
+ 'name="foo"\r\n\r\nbar\r\n--%s\r\nContent-Disposition: ' \
+ 'form-data; name="foofile"; filename="test.txt"\r\n' \
+ 'Content-Type: application/octet-stream\r\n\r\n' \
+ 'contents of foofile\r\n--%s--\r\n' % (
+ boundary,boundary,boundary), body)
+
def suite():
suite = unittest.TestSuite()
suite.addTest(unittest.makeSuite(BuildSlaveTestCase, 'test'))
+ suite.addTest(unittest.makeSuite(MultiPartEncodeTestCase, 'test'))
return suite
if __name__ == '__main__':