Refactoring swift binary retrievers to allow context auth

This change is needed to fix an issue with using proxy domains and
downloading job binaries through the ".../data" endpoint. A new function
is added to retrieve binaries from swift using the context credentials.
The raw data retrievers for swift have been refactored to allow a more
modular approach.

Changes
* adding an option to the raw binary dispatch routine to allow
  requesting of context authentication
* adding a new swift client method to create a connection with an auth
  token
* adding a new binary retriever function to use the context auth token
* refactoring the internal swift retrievers module to be more modular
* adding tests for all internal swift retrievers
* adding a new test suite for binary dispatch

Change-Id: I5e3ce1c9b61d4e2043a15702cbcc9225f9924d44
Closes-Bug: 1384889

sahara/service/edp/api.py

@@ -169,4 +169,4 @@ def get_job_binary_internal_data(id):
 
 def get_job_binary_data(id):
     job_binary = conductor.job_binary_get(context.ctx(), id)
-    return dispatch.get_raw_binary(job_binary)
+    return dispatch.get_raw_binary(job_binary, with_context=True)

sahara/service/edp/binary_retrievers/dispatch.py

@@ -19,12 +19,29 @@ from sahara.service.edp.binary_retrievers import sahara_db as db
 from sahara.swift import utils as su
 
 
-def get_raw_binary(job_binary, proxy_configs=None):
+def get_raw_binary(job_binary, proxy_configs=None, with_context=False):
+    '''Get the raw data for a job binary
+
+    This will retrieve the raw data for a job binary from it's source. In the
+    case of Swift based binaries there is a precedence of credentials for
+    authenticating the client. Requesting a context based authentication takes
+    precendence over proxy user which takes precendence over embedded
+    credentials.
+
+    :param job_binary: The job binary to retrieve
+    :param proxy_configs: Proxy user configuration to use as credentials
+    :param with_context: Use the current context as credentials
+    :returns: The raw data from a job binary
+
+    '''
     url = job_binary.url
     if url.startswith("internal-db://"):
         res = db.get_raw_data(context.ctx(), job_binary)
 
     if url.startswith(su.SWIFT_INTERNAL_PREFIX):
+        if with_context:
+            res = i_swift.get_raw_data_with_context(job_binary)
+        else:
             res = i_swift.get_raw_data(job_binary, proxy_configs)
 
     return res

sahara/service/edp/binary_retrievers/internal_swift.py

@@ -12,11 +12,13 @@
 # implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import functools
 
 from oslo.config import cfg
 import six
 import swiftclient
 
+import sahara.context as context
 import sahara.exceptions as ex
 from sahara.i18n import _
 from sahara.swift import utils as su
@@ -26,42 +28,18 @@ from sahara.utils.openstack import swift as sw
 CONF = cfg.CONF
 
 
-def _strip_sahara_suffix(container_name):
-    if container_name.endswith(su.SWIFT_URL_SUFFIX):
-        container_name = container_name[:-len(su.SWIFT_URL_SUFFIX)]
-    return container_name
+def _get_names_from_url(url):
+    parse = six.moves.urllib.parse.urlparse(url)
+    return (parse.netloc + parse.path).split('/', 1)
 
 
-def get_raw_data(job_binary, proxy_configs=None):
-    conn_kwargs = {}
-    if proxy_configs:
-        conn_kwargs.update(username=proxy_configs.get('proxy_username'),
-                           password=proxy_configs.get('proxy_password'),
-                           trust_id=proxy_configs.get('proxy_trust_id'))
-    else:
-        conn_kwargs.update(username=job_binary.extra.get('user'),
-                           password=job_binary.extra.get('password'))
-
-    conn = sw.client(**conn_kwargs)
-
-    if not (job_binary.url.startswith(su.SWIFT_INTERNAL_PREFIX)):
-        # This should have been guaranteed already,
-        # but we'll check just in case.
-        raise ex.BadJobBinaryException(
-            _("Url for binary in internal swift must start with %s")
-            % su.SWIFT_INTERNAL_PREFIX)
-
-    names = job_binary.url[job_binary.url.index("://") + 3:].split("/", 1)
-    if len(names) == 1:
-        # a container has been requested, this is currently unsupported
-        raise ex.BadJobBinaryException(
-            _('Url for binary in internal swift must specify an object not '
-              'a container'))
-    else:
+def _get_raw_data(job_binary, conn):
+    names = _get_names_from_url(job_binary.url)
     container, obj = names
 
     # if container name has '.sahara' suffix we need to strip it
-        container = _strip_sahara_suffix(container)
+    if container.endswith(su.SWIFT_URL_SUFFIX):
+        container = container[:-len(su.SWIFT_URL_SUFFIX)]
 
     try:
         # First check the size
@@ -78,3 +56,43 @@ def get_raw_data(job_binary, proxy_configs=None):
         raise ex.SwiftClientException(six.text_type(e))
 
     return body
+
+
+def _validate_job_binary_url(f):
+    @functools.wraps(f)
+    def wrapper(job_binary, *args, **kwargs):
+        if not (job_binary.url.startswith(su.SWIFT_INTERNAL_PREFIX)):
+            # This should have been guaranteed already,
+            # but we'll check just in case.
+            raise ex.BadJobBinaryException(
+                _("Url for binary in internal swift must start with %s")
+                % su.SWIFT_INTERNAL_PREFIX)
+        names = _get_names_from_url(job_binary.url)
+        if len(names) == 1:
+            # a container has been requested, this is currently unsupported
+            raise ex.BadJobBinaryException(
+                _('Url for binary in internal swift must specify an object '
+                  'not a container'))
+        return f(job_binary, *args, **kwargs)
+    return wrapper
+
+
+@_validate_job_binary_url
+def get_raw_data(job_binary, proxy_configs=None):
+    conn_kwargs = {}
+    if proxy_configs:
+        conn_kwargs.update(username=proxy_configs.get('proxy_username'),
+                           password=proxy_configs.get('proxy_password'),
+                           trust_id=proxy_configs.get('proxy_trust_id'))
+    else:
+        conn_kwargs.update(username=job_binary.extra.get('user'),
+                           password=job_binary.extra.get('password'))
+
+    conn = sw.client(**conn_kwargs)
+    return _get_raw_data(job_binary, conn)
+
+
+@_validate_job_binary_url
+def get_raw_data_with_context(job_binary):
+    conn = sw.client_from_token(context.ctx().auth_token)
+    return _get_raw_data(job_binary, conn)

sahara/tests/unit/service/edp/binary_retrievers/test_dispatch.py

@@ -0,0 +1,51 @@
+# Copyright (c) 2015 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import mock
+
+from sahara.service.edp.binary_retrievers import dispatch
+from sahara.tests.unit import base
+
+
+class TestDispatch(base.SaharaTestCase):
+    def setUp(self):
+        super(TestDispatch, self).setUp()
+
+    @mock.patch(
+        'sahara.service.edp.binary_retrievers.internal_swift.'
+        'get_raw_data_with_context')
+    @mock.patch(
+        'sahara.service.edp.binary_retrievers.internal_swift.get_raw_data')
+    @mock.patch('sahara.service.edp.binary_retrievers.sahara_db.get_raw_data')
+    @mock.patch('sahara.context.ctx')
+    def test_get_raw_binary(self, ctx, db_get_raw_data, i_s_get_raw_data,
+                            i_s_get_raw_data_with_context):
+        ctx.return_value = mock.Mock()
+
+        job_binary = mock.Mock()
+        job_binary.url = 'internal-db://somebinary'
+
+        dispatch.get_raw_binary(job_binary)
+        self.assertEqual(1, db_get_raw_data.call_count)
+
+        job_binary.url = 'swift://container/object'
+        proxy_configs = dict(proxy_username='proxytest',
+                             proxy_password='proxysecret',
+                             proxy_trust_id='proxytrust')
+        dispatch.get_raw_binary(job_binary, proxy_configs)
+        dispatch.get_raw_binary(job_binary, proxy_configs, with_context=True)
+        dispatch.get_raw_binary(job_binary, with_context=True)
+        self.assertEqual(1, i_s_get_raw_data.call_count)
+        self.assertEqual(2, i_s_get_raw_data_with_context.call_count)

sahara/tests/unit/service/edp/binary_retrievers/test_internal_swift.py

@@ -24,44 +24,95 @@ class TestInternalSwift(base.SaharaTestCase):
     def setUp(self):
         super(TestInternalSwift, self).setUp()
 
-    @mock.patch('sahara.utils.openstack.swift.client')
-    def test_get_raw_data(self, swift_client):
+    def test__get_raw_data(self):
         client_instance = mock.Mock()
-        swift_client.return_value = client_instance
+        client_instance.head_object = mock.Mock()
+        client_instance.get_object = mock.Mock()
+
+        job_binary = mock.Mock()
+        job_binary.url = 'swift://container/object'
+
+        # an object that is too large should raise an exception
+        header = {'content-length': '2048'}
+        client_instance.head_object.return_value = header
+        self.override_config('job_binary_max_KB', 1)
+        self.assertRaises(ex.DataTooBigException,
+                          i_s._get_raw_data,
+                          job_binary,
+                          client_instance)
+        client_instance.head_object.assert_called_once_with('container',
+                                                            'object')
+
+        # valid return
+        header = {'content-length': '4'}
+        body = 'data'
+        client_instance.head_object.return_value = header
+        client_instance.get_object.return_value = (header, body)
+        self.assertEqual(body, i_s._get_raw_data(job_binary, client_instance))
+        client_instance.get_object.assert_called_once_with('container',
+                                                           'object')
+
+    def test__validate_job_binary_url(self):
+        @i_s._validate_job_binary_url
+        def empty_method(job_binary):
+            pass
 
         job_binary = mock.Mock()
-        job_binary.extra = dict(user='test', password='secret')
 
         # bad swift url should raise an exception
         job_binary.url = 'notswift://container/object'
         self.assertRaises(ex.BadJobBinaryException,
-                          i_s.get_raw_data,
+                          empty_method,
                           job_binary)
 
         # specifying a container should raise an exception
         job_binary.url = 'swift://container'
         self.assertRaises(ex.BadJobBinaryException,
-                          i_s.get_raw_data,
+                          empty_method,
                           job_binary)
 
-        # an object that is too large should raise an exception
+    @mock.patch(
+        'sahara.service.edp.binary_retrievers.internal_swift._get_raw_data')
+    @mock.patch('sahara.utils.openstack.swift.client')
+    def test_get_raw_data(self, swift_client, _get_raw_data):
+        client_instance = mock.Mock()
+        swift_client.return_value = client_instance
+
+        job_binary = mock.Mock()
         job_binary.url = 'swift://container/object'
-        client_instance.head_object = mock.Mock()
-        header = {'content-length': '2048'}
-        client_instance.head_object.return_value = header
-        self.override_config('job_binary_max_KB', 1)
-        self.assertRaises(ex.DataTooBigException,
-                          i_s.get_raw_data,
-                          job_binary)
-        client_instance.head_object.assert_called_once_with('container',
-                                                            'object')
 
-        # valid return
-        client_instance.get_object = mock.Mock()
-        header = {'content-length': '4'}
-        body = 'data'
-        client_instance.head_object.return_value = header
-        client_instance.get_object.return_value = (header, body)
-        self.assertEqual(body, i_s.get_raw_data(job_binary))
-        client_instance.get_object.assert_called_once_with('container',
-                                                           'object')
+        # embedded credentials
+        job_binary.extra = dict(user='test', password='secret')
+        i_s.get_raw_data(job_binary)
+        swift_client.assert_called_with(username='test',
+                                        password='secret')
+        _get_raw_data.assert_called_with(job_binary, client_instance)
+
+        # proxy configs should override embedded credentials
+        proxy_configs = dict(proxy_username='proxytest',
+                             proxy_password='proxysecret',
+                             proxy_trust_id='proxytrust')
+        i_s.get_raw_data(job_binary, proxy_configs)
+        swift_client.assert_called_with(username='proxytest',
+                                        password='proxysecret',
+                                        trust_id='proxytrust')
+        _get_raw_data.assert_called_with(job_binary, client_instance)
+
+    @mock.patch('sahara.context.ctx')
+    @mock.patch(
+        'sahara.service.edp.binary_retrievers.internal_swift._get_raw_data')
+    @mock.patch('sahara.utils.openstack.swift.client_from_token')
+    def test_get_raw_data_with_context(self, swift_client, _get_raw_data, ctx):
+        client_instance = mock.Mock()
+        swift_client.return_value = client_instance
+        test_context = mock.Mock()
+        test_context.auth_token = 'testtoken'
+        ctx.return_value = test_context
+
+        job_binary = mock.Mock()
+        job_binary.url = 'swift://container/object'
+
+        job_binary.extra = dict(user='test', password='secret')
+        i_s.get_raw_data_with_context(job_binary)
+        swift_client.assert_called_with('testtoken')
+        _get_raw_data.assert_called_with(job_binary, client_instance)

sahara/utils/openstack/swift.py

@@ -53,18 +53,23 @@ def client(username, password, trust_id=None):
     :returns: A Swift client object
 
     '''
-    client_kwargs = dict(
-        auth_version='2.0',
-        cacert=CONF.swift.ca_file,
-        insecure=CONF.swift.api_insecure)
     if trust_id:
         proxyclient = k.client_for_proxy_user(username, password, trust_id)
-        client_kwargs.update(preauthurl=su.retrieve_preauth_url(),
-                             preauthtoken=proxyclient.auth_token)
+        return client_from_token(proxyclient.auth_token)
     else:
-        client_kwargs.update(authurl=su.retrieve_auth_url(),
+        return swiftclient.Connection(auth_version='2.0',
+                                      cacert=CONF.swift.ca_file,
+                                      insecure=CONF.swift.api_insecure,
+                                      authurl=su.retrieve_auth_url(),
                                       user=username,
                                       key=password,
                                       tenant_name=sh.retrieve_tenant())
 
-    return swiftclient.Connection(**client_kwargs)
+
+def client_from_token(token):
+    '''return a Swift client authenticated from a token.'''
+    return swiftclient.Connection(auth_version='2.0',
+                                  cacert=CONF.swift.ca_file,
+                                  insecure=CONF.swift.api_insecure,
+                                  preauthurl=su.retrieve_preauth_url(),
+                                  preauthtoken=token)