Repository: boto/botocore (python)

Issue:
fix_s3_hosts always assumes you're using central aws instead of a local S3.
I've noticed when attempting to generate presigned urls that the logic behind fix_s3_hosts and the virtual styling always assumes you're using AWS S3. [fix_s3_host](https://github.com/boto/botocore/blob/6065566237de957507fc63ae4d3a034c87552adf/botocore/utils.py#L649)

However, we have a local s3 api on top of ceph, as such it throws away our local s3 endpoint and substitutes s3.amazonaws.com, although the presigned urls work when you switch out the virtual path for our local s3, it's a pain.

This is because the default_endpoint_url passed to [switch_to_virtual_host_style](https://github.com/boto/botocore/blob/6065566237de957507fc63ae4d3a034c87552adf/botocore/utils.py#L656) is always set to 's3.amazonaws.com'

The request object is available in the fix_s3_host function so it should be a fairly simple fix.

Pull request:
Respect custom endpoints in fix_s3_host
Currently we inject the hard-coded value `s3.amazonaws.com` as the endpoint
url for S3 requests when switching from path style to virtual host addressing.
This has the effect of clobbering custom endpoint urls. This PR will try to
determine if the endpoint url is an S3 endpoint, and only inject the global S3
url if so.

There are two discussion items I would like to bring up with this PR:

* Should we pass along the user's netloc (current behavior) or short-circuit
  fix_x3_host? The former will be consistent with how we handle S3, and the
  latter may respect the user's intentions better.
* Is there a better, reasonable way to determine if the URL is an S3 url? We
  have the endpoint resolver available, but since we don't have access to the
  session inside fix_s3_host we would have to load the endpoints file
  separately. I'm not a fan of using regex to solve problems like this, but I'm
  also not a fan of duplicated reads.

Closes #875
Closes #878

P.S. In addition, I also did some clean-up in the files I was working in.

cc @jamesls @kyleknap 

Patch:
diff --git a/botocore/client.py b/botocore/client.py
index 9e240f718..ebda5db07 100644
--- a/botocore/client.py
+++ b/botocore/client.py
@@ -154,6 +154,11 @@ class ClientCreator(object):
 
         config_kwargs['s3'] = s3_configuration
 
+    def _conditionally_unregister_fix_s3_host(self, endpoint_url, emitter):
+        # If the user is providing a custom endpoint, we should not alter it.
+        if endpoint_url is not None:
+            emitter.unregister('before-sign.s3', fix_s3_host)
+
     def _get_client_args(self, service_model, region_name, is_secure,
                          endpoint_url, verify, credentials,
                          scoped_config, client_config):
@@ -202,6 +207,7 @@ class ClientCreator(object):
         # Add any additional s3 configuration for client
         self._inject_s3_configuration(
             config_kwargs, scoped_config, client_config)
+        self._conditionally_unregister_fix_s3_host(endpoint_url, event_emitter)
 
         new_config = Config(**config_kwargs)
         endpoint_creator = EndpointCreator(event_emitter)


Test patch:
diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py
index 23a7fd75f..5cd3c20e0 100644
--- a/tests/functional/test_s3.py
+++ b/tests/functional/test_s3.py
@@ -155,6 +155,17 @@ class BaseS3AddressingStyle(BaseSessionTest):
         self.http_response.content = b''
 
 
+class TestCustomEndpointUrl(BaseS3AddressingStyle):
+    def test_provided_endpoint_url_is_not_mutated(self):
+        s3 = self.session.create_client('s3', endpoint_url='https://foo.com')
+        with mock.patch('botocore.endpoint.Session.send') as mock_send:
+            mock_send.return_value = self.http_response
+            s3.put_object(Bucket='mybucket', Key='mykey', Body='mybody')
+            request_sent = mock_send.call_args[0][0]
+            self.assertEqual(
+                'https://foo.com/mybucket/mykey', request_sent.url)
+
+
 class TestVirtualHostStyle(BaseS3AddressingStyle):
     def test_default_endpoint_for_virtual_addressing(self):
         s3 = self.session.create_client(
diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py
index a1baaaae3..16e675d83 100644
--- a/tests/unit/test_client.py
+++ b/tests/unit/test_client.py
@@ -1169,6 +1169,28 @@ class TestAutoGeneratedClient(unittest.TestCase):
             client.meta.events.register.call_args_list
         )
 
+    def test_custom_endpoint_unregisters_fix_s3_host(self):
+        event_emitter = mock.Mock()
+        creator = self.create_client_creator(event_emitter=event_emitter)
+
+        # It shouldn't be unregistered if we don't provide a url
+        client = creator.create_client('myservice', 'us-west-2')
+        self.assertNotIn(
+            mock.call('before-sign.s3', utils.fix_s3_host),
+            client.meta.events.unregister.call_args_list
+        )
+
+        # If we do provide a url, it must be unregistered
+        event_emitter.reset_mock()
+        client = creator.create_client(
+            'myservice', 'us-west-2',
+            endpoint_url="foo.com"
+        )
+        self.assertIn(
+            mock.call('before-sign.s3', utils.fix_s3_host),
+            client.meta.events.unregister.call_args_list
+        )
+
     def test_client_s3_accelerate_from_scoped_config(self):
         creator = self.create_client_creator()
         my_client = creator.create_client(