Repository: openaddresses/machine
Pull request #475 (2016-12-04 19:39:09)

Removed all uses of boto.s3.key.generate_url()
When used with IAM Roles, `generate_url()` was appending tokens to generated URLs. Replaced with a dumber string-building function.


Patch:
diff --git a/openaddr/ci/__init__.py b/openaddr/ci/__init__.py
index 78783f5..8bae811 100644
--- a/openaddr/ci/__init__.py
+++ b/openaddr/ci/__init__.py
@@ -1,7 +1,7 @@
 import logging; _L = logging.getLogger('openaddr.ci')
 
 from ..compat import standard_library, expand_uri
-from .. import jobs, render
+from .. import jobs, render, util
 
 from .objects import (
     add_job, write_job, read_job, complete_set, update_set_renders,
@@ -672,8 +672,6 @@ def _render_and_upload_maps(s3, good_sources, s3_prefix, dirname):
     urls = dict()
     areas = (render.WORLD, 'world'), (render.USA, 'usa'), (render.EUROPE, 'europe')
     
-    # Safe to force_http=False because we set boto.s3.connection.OrdinaryCallingFormat
-    url_kwargs = dict(expires_in=0, query_auth=False, force_http=False)
     key_kwargs = dict(policy='public-read', headers={'Content-Type': 'image/png'})
 
     for (area, area_name) in areas:
@@ -686,7 +684,7 @@ def _render_and_upload_maps(s3, good_sources, s3_prefix, dirname):
             render_key = s3.new_key(join(s3_prefix, png_basename))
             render_key.set_contents_from_string(file.read(), **key_kwargs)
 
-        urls[area_name] = render_key.generate_url(**url_kwargs)
+        urls[area_name] = util.s3_key_url(render_key)
     
     return urls['world'], urls['usa'], urls['europe']
 
diff --git a/openaddr/ci/collect.py b/openaddr/ci/collect.py
index c04b97e..6401997 100644
--- a/openaddr/ci/collect.py
+++ b/openaddr/ci/collect.py
@@ -166,7 +166,7 @@ class CollectorPublisher:
         zip_key = write_to_s3(self.s3.bucket, self.zip.filename, basename(self.zip.filename))
         _L.info(u'Uploaded {} to {}'.format(self.zip.filename, zip_key.name))
 
-        zip_url = zip_key.generate_url(expires_in=0, query_auth=False, force_http=True)
+        zip_url = util.s3_key_url(zip_key)
         length = stat(self.zip.filename).st_size if exists(self.zip.filename) else None
 
         db.execute('''DELETE FROM zips WHERE url = %s''', (zip_url, ))
diff --git a/openaddr/ci/work.py b/openaddr/ci/work.py
index f8488e1..d9d5700 100644
--- a/openaddr/ci/work.py
+++ b/openaddr/ci/work.py
@@ -15,7 +15,7 @@ def upload_file(s3, keyname, filename):
 
     kwargs = dict(policy='public-read', reduced_redundancy=True)
     key.set_contents_from_filename(filename, **kwargs)
-    url = key.generate_url(expires_in=0, query_auth=False, force_http=True)
+    url = util.s3_key_url(key)
     
     return url, key.md5.decode('ascii')
 
diff --git a/openaddr/util/__init__.py b/openaddr/util/__init__.py
index 6e61b6a..910c5f3 100644
--- a/openaddr/util/__init__.py
+++ b/openaddr/util/__init__.py
@@ -1,6 +1,6 @@
 import logging; _L = logging.getLogger('openaddr.util')
 
-from urllib.parse import urlparse, parse_qsl
+from urllib.parse import urlparse, parse_qsl, urljoin
 from datetime import datetime, timedelta, date
 from os.path import join, basename, splitext, dirname
 from operator import attrgetter
@@ -10,7 +10,7 @@ import ftplib, httmock
 import io, zipfile
 import json
 
-from ..compat import quote
+from .. import compat
 
 def get_version():
     ''' Prevent circular imports.
@@ -60,7 +60,7 @@ def _command_messages(command):
         message_failed = 'Failed {}'.format(command),
         )
     
-    return { k: quote(json.dumps(dict(text=v))) for (k, v) in strings.items() }
+    return { k: compat.quote(json.dumps(dict(text=v))) for (k, v) in strings.items() }
 
 def request_task_instance(ec2, autoscale, instance_type, chef_role, lifespan, command, bucket, slack_url):
     '''
@@ -76,13 +76,13 @@ def request_task_instance(ec2, autoscale, instance_type, chef_role, lifespan, co
     
     with open(join(dirname(__file__), 'templates', 'task-instance-userdata.sh')) as file:
         userdata_kwargs = dict(
-            role = quote(chef_role),
-            command = ' '.join(map(quote, command)),
-            lifespan = quote(str(lifespan)),
-            version = quote(get_version()),
-            log_prefix = quote('logs/{}-{}'.format(yyyymmdd, command[0])),
-            bucket = quote(bucket or 'data.openaddresses.io'),
-            slack_url = quote(slack_url or ''),
+            role = compat.quote(chef_role),
+            command = ' '.join(map(compat.quote, command)),
+            lifespan = compat.quote(str(lifespan)),
+            version = compat.quote(get_version()),
+            log_prefix = compat.quote('logs/{}-{}'.format(yyyymmdd, command[0])),
+            bucket = compat.quote(bucket or 'data.openaddresses.io'),
+            slack_url = compat.quote(slack_url or ''),
             **_command_messages(command[0])
             )
     
@@ -176,3 +176,14 @@ def request_ftp_file(url):
 
     # Using mock response because HTTP responses are expected downstream
     return httmock.response(200, file.read(), headers={'Content-Type': 'application/octet-stream'})
+
+def s3_key_url(key):
+    '''
+    '''
+    base = u'https://s3.amazonaws.com'
+    path = join(key.bucket.name, key.name.lstrip('/'))
+    
+    if compat.PY2 and type(path) is not unicode:
+        path = path.decode('utf8')
+    
+    return urljoin(base, path)


Test patch:
diff --git a/openaddr/tests/__init__.py b/openaddr/tests/__init__.py
index 4e4d036..273f044 100644
--- a/openaddr/tests/__init__.py
+++ b/openaddr/tests/__init__.py
@@ -1594,18 +1594,21 @@ class FakeS3 (S3):
     def new_key(self, name):
         return FakeKey(name, self)
 
+class FakeBucket:
+    '''
+    '''
+    name = 'fake-bucket'
+
 class FakeKey:
     ''' Just enough S3 to work for tests.
     '''
     md5 = b'0xDEADBEEF'
     
     def __init__(self, name, fake_s3):
+        self.bucket = FakeBucket()
         self.name = name
         self.s3 = fake_s3
     
-    def generate_url(self, **kwargs):
-        return 'http://fake-s3.local' + self.name
-
     def set_contents_from_string(self, string, **kwargs):
         self.s3._write_fake_key(self.name, string)
         
diff --git a/openaddr/tests/ci.py b/openaddr/tests/ci.py
index ea38e25..568dd16 100644
--- a/openaddr/tests/ci.py
+++ b/openaddr/tests/ci.py
@@ -2390,7 +2390,7 @@ class TestRuns (unittest.TestCase):
                 ((state, ), ) = db.fetchall()
             
             # Look for log file message in output.
-            self.assertIn(b'Took too long.', self.s3._read_fake_key(urlparse(state['output']).path))
+            self.assertIn(b'Took too long.', self.s3._read_fake_key(urlparse(state['output']).path[len('/fake-bucket'):]))
 
     @patch('openaddr.jobs.JOB_TIMEOUT', new=timedelta(seconds=1))
     @patch('openaddr.ci.DUETASK_DELAY', new=timedelta(seconds=1))
@@ -2716,6 +2716,8 @@ class TestWorker (unittest.TestCase):
         '''
         s3 = mock.Mock()
         s3.new_key.return_value.md5 = b'0xWHATEVER'
+        s3.new_key.return_value.name = 'a-key'
+        s3.new_key.return_value.bucket.name = 'a-bucket'
 
         input1 = {'cache': False, 'sample': False, 'processed': False, 'output': False, 'preview': False}
         state1 = RunState(assemble_output(s3, input1, 'xx/f', 1, 'dir'))
@@ -2729,23 +2731,23 @@ class TestWorker (unittest.TestCase):
         input2 = {'cache': 'cache.csv', 'sample': False, 'processed': False, 'output': False, 'preview': False}
         state2 = RunState(assemble_output(s3, input2, 'xx/f', 2, 'dir'))
 
-        self.assertEqual(state2.cache, s3.new_key.return_value.generate_url.return_value)
+        self.assertEqual(state2.cache, 'https://s3.amazonaws.com/a-bucket/a-key')
         self.assertEqual(state2.fingerprint, '0xWHATEVER')
         self.assertEqual(state2.sample, input2['sample'])
         self.assertEqual(state2.processed, input2['processed'])
         self.assertEqual(state2.output, input2['output'])
         self.assertEqual(state2.preview, input2['preview'])
-        self.assertEqual(s3.new_key.mock_calls[-3], mock.call('/runs/2/cache.csv'))
+        self.assertEqual(s3.new_key.mock_calls[-2], mock.call('/runs/2/cache.csv'))
 
         input3 = {'cache': False, 'sample': 'sample.json', 'processed': False, 'output': False, 'preview': False}
         state3 = RunState(assemble_output(s3, input3, 'xx/f', 3, 'dir'))
 
         self.assertEqual(state3.cache, input3['cache'])
-        self.assertEqual(state3.sample, s3.new_key.return_value.generate_url.return_value)
+        self.assertEqual(state3.sample, 'https://s3.amazonaws.com/a-bucket/a-key')
         self.assertEqual(state3.processed, input3['processed'])
         self.assertEqual(state3.output, input3['output'])
         self.assertEqual(state3.preview, input3['preview'])
-        self.assertEqual(s3.new_key.mock_calls[-3], mock.call('/runs/3/sample.json'))
+        self.assertEqual(s3.new_key.mock_calls[-2], mock.call('/runs/3/sample.json'))
 
         input4 = {'cache': False, 'sample': False, 'processed': False, 'output': 'out.txt', 'preview': False}
         state4 = RunState(assemble_output(s3, input4, 'xx/f', 4, 'dir'))
@@ -2753,9 +2755,9 @@ class TestWorker (unittest.TestCase):
         self.assertEqual(state4.cache, input4['cache'])
         self.assertEqual(state4.sample, input4['sample'])
         self.assertEqual(state4.processed, input4['processed'])
-        self.assertEqual(state4.output, s3.new_key.return_value.generate_url.return_value)
+        self.assertEqual(state4.output, 'https://s3.amazonaws.com/a-bucket/a-key')
         self.assertEqual(state4.preview, input4['preview'])
-        self.assertEqual(s3.new_key.mock_calls[-3], mock.call('/runs/4/out.txt'))
+        self.assertEqual(s3.new_key.mock_calls[-2], mock.call('/runs/4/out.txt'))
 
         with patch('openaddr.util.package_output') as package_output:
             package_output.return_value = 'nothing.zip'
@@ -2764,11 +2766,11 @@ class TestWorker (unittest.TestCase):
 
         self.assertEqual(state5.cache, input5['cache'])
         self.assertEqual(state5.sample, input5['sample'])
-        self.assertEqual(state5.processed, s3.new_key.return_value.generate_url.return_value)
+        self.assertEqual(state5.processed, 'https://s3.amazonaws.com/a-bucket/a-key')
         self.assertEqual(state5.process_hash, '0xWHATEVER')
         self.assertEqual(state5.output, input5['output'])
         self.assertEqual(state5.preview, input5['preview'])
-        self.assertEqual(s3.new_key.mock_calls[-3], mock.call('/runs/5/xx/f.zip'))
+        self.assertEqual(s3.new_key.mock_calls[-2], mock.call('/runs/5/xx/f.zip'))
 
         input6 = {'cache': False, 'sample': False, 'processed': False, 'output': False, 'preview': 'preview.png'}
         state6 = RunState(assemble_output(s3, input6, 'xx/f', 4, 'dir'))
@@ -2777,8 +2779,8 @@ class TestWorker (unittest.TestCase):
         self.assertEqual(state6.sample, input6['sample'])
         self.assertEqual(state6.processed, input6['processed'])
         self.assertEqual(state6.output, input6['output'])
-        self.assertEqual(state6.preview, s3.new_key.return_value.generate_url.return_value)
-        self.assertEqual(s3.new_key.mock_calls[-3], mock.call('/runs/4/preview.png'))
+        self.assertEqual(state6.preview, 'https://s3.amazonaws.com/a-bucket/a-key')
+        self.assertEqual(s3.new_key.mock_calls[-2], mock.call('/runs/4/preview.png'))
 
     @patch('tempfile.mkdtemp')
     @patch('openaddr.compat.check_output')
@@ -2834,7 +2836,7 @@ class TestWorker (unittest.TestCase):
         self.assertEqual(result['output']['license'], 'GPL')
         
         zip_path = urlparse(result['output']['processed']).path
-        zip_bytes = self.s3._read_fake_key(zip_path)
+        zip_bytes = self.s3._read_fake_key(zip_path[len('/fake-bucket'):])
         zip_file = ZipFile(BytesIO(zip_bytes), mode='r')
         self.assertTrue(u'README.txt' in zip_file.namelist())
         self.assertTrue(u'so/exalté.csv' in zip_file.namelist())
@@ -2975,6 +2977,9 @@ class TestBatch (unittest.TestCase):
         if url.hostname == 'fake-s3.local':
             return response(200, self.s3._read_fake_key(url.path))
         
+        if url.hostname == 's3.amazonaws.com' and url.path.startswith('/fake-bucket/'):
+            return response(200, self.s3._read_fake_key(url.path[len('/fake-bucket'):]))
+        
         if MHP == ('GET', GH, '/'):
             data = u'''{\r  "current_user_url": "https://api.github.com/user",\r  "current_user_authorizations_html_url": "https://github.com/settings/connections/applications{/client_id}",\r  "authorizations_url": "https://api.github.com/authorizations",\r  "code_search_url": "https://api.github.com/search/code?q={query}{&page,per_page,sort,order}",\r  "emails_url": "https://api.github.com/user/emails",\r  "emojis_url": "https://api.github.com/emojis",\r  "events_url": "https://api.github.com/events",\r  "feeds_url": "https://api.github.com/feeds",\r  "following_url": "https://api.github.com/user/following{/target}",\r  "gists_url": "https://api.github.com/gists{/gist_id}",\r  "hub_url": "https://api.github.com/hub",\r  "issue_search_url": "https://api.github.com/search/issues?q={query}{&page,per_page,sort,order}",\r  "issues_url": "https://api.github.com/issues",\r  "keys_url": "https://api.github.com/user/keys",\r  "notifications_url": "https://api.github.com/notifications",\r  "organization_repositories_url": "https://api.github.com/orgs/{org}/repos{?type,page,per_page,sort}",\r  "organization_url": "https://api.github.com/orgs/{org}",\r  "public_gists_url": "https://api.github.com/gists/public",\r  "rate_limit_url": "https://api.github.com/rate_limit",\r  "repository_url": "https://api.github.com/repos/{owner}/{repo}",\r  "repository_search_url": "https://api.github.com/search/repositories?q={query}{&page,per_page,sort,order}",\r  "current_user_repositories_url": "https://api.github.com/user/repos{?type,page,per_page,sort}",\r  "starred_url": "https://api.github.com/user/starred{/owner}{/repo}",\r  "starred_gists_url": "https://api.github.com/gists/starred",\r  "team_url": "https://api.github.com/teams",\r  "user_url": "https://api.github.com/users/{user}",\r  "user_organizations_url": "https://api.github.com/user/orgs",\r  "user_repositories_url": "https://api.github.com/users/{user}/repos{?type,page,per_page,sort}",\r  "user_search_url": "https://api.github.com/search/users?q={query}{&page,per_page,sort,order}"\r}'''
             return response(200, data.encode('utf8'), headers=response_headers)
@@ -3391,7 +3396,8 @@ class TestCollect (unittest.TestCase):
 
             with patch('openaddr.ci.collect.write_to_s3') as write_to_s3:
                 s3_key_mock = mock.Mock()
-                s3_key_mock.generate_url.return_value = 'http://internet/collected-local.zip'
+                s3_key_mock.name = 'collected-local.zip'
+                s3_key_mock.bucket.name = 'a-bucket'
                 write_to_s3.return_value = s3_key_mock
 
                 with patch('openaddr.util.summarize_result_licenses') as summarize_result_licenses:
@@ -3417,11 +3423,11 @@ class TestCollect (unittest.TestCase):
 
         self.assertEqual(db.execute.mock_calls, [
             mock.call('DELETE FROM zips WHERE url = %s',
-                      ('http://internet/collected-local.zip',)),
+                      ('https://s3.amazonaws.com/a-bucket/collected-local.zip',)),
             mock.call('''INSERT INTO zips
                       (url, datetime, is_current, content_length, collection, license_attr)
                       VALUES (%s, NOW(), true, %s, %s, %s)''',
-                      ('http://internet/collected-local.zip', None, 'everywhere', 'yo'))])
+                      ('https://s3.amazonaws.com/a-bucket/collected-local.zip', None, 'everywhere', 'yo'))])
 
     def test_collector_publisher_multipart_s3_one_part(self):
         '''
diff --git a/openaddr/tests/util.py b/openaddr/tests/util.py
index 0c4e49c..52b5acb 100644
--- a/openaddr/tests/util.py
+++ b/openaddr/tests/util.py
@@ -1,3 +1,4 @@
+# coding=utf8
 # Test suite. This code could be in a separate file
 
 from shutil import rmtree
@@ -168,3 +169,30 @@ class TestUtilities (unittest.TestCase):
                 else:
                     self.assertEqual(resp.status_code, 200)
                     self.assertEqual(resp.content, zip_bytes, 'Expected number of bytes')
+        
+    def test_s3_key_url(self):
+        '''
+        '''
+        key1 = Mock()
+        key1.name, key1.bucket.name = 'key1', 'bucket1'
+        self.assertEqual(util.s3_key_url(key1), 'https://s3.amazonaws.com/bucket1/key1')
+
+        key2 = Mock()
+        key2.name, key2.bucket.name = '/key2', 'bucket2'
+        self.assertEqual(util.s3_key_url(key2), 'https://s3.amazonaws.com/bucket2/key2')
+
+        key3 = Mock()
+        key3.name, key3.bucket.name = 'key/3', 'bucket3'
+        self.assertEqual(util.s3_key_url(key3), 'https://s3.amazonaws.com/bucket3/key/3')
+
+        key4 = Mock()
+        key4.name, key4.bucket.name = '/key/4', 'bucket4'
+        self.assertEqual(util.s3_key_url(key4), 'https://s3.amazonaws.com/bucket4/key/4')
+
+        key5 = Mock()
+        key5.name, key5.bucket.name = u'kéy5', 'bucket5'
+        self.assertEqual(util.s3_key_url(key5), u'https://s3.amazonaws.com/bucket5/kéy5')
+
+        key6 = Mock()
+        key6.name, key6.bucket.name = u'/kéy6', 'bucket6'
+        self.assertEqual(util.s3_key_url(key6), u'https://s3.amazonaws.com/bucket6/kéy6')