Stop lazy importing keystoneclient

There were two basic problems:

  - We'd try to import on every attempt at getting auth, even when we
    already know keystoneclient isn't available.
  - Sometimes devs would hit some crazy import race involving (some
    combination of?) greenthreads and OS threads.

So let's just try the imports *once*, at import time, and have None
sentinels if it fails. Try both versions separately to decouple
failures; this should let us support a wider range of keystoneclient
versions.

Change-Id: I2367310aac74f1b7c5ea0cb1a822a491e4ba8e68

Author: tipabu

swiftclient/client.py

@@ -60,6 +60,19 @@ def emit(self, record):
         def createLock(self):
             self.lock = None
 
+ksexceptions = ksclient_v2 = ksclient_v3 = None
+try:
+    from keystoneclient import exceptions as ksexceptions
+    # prevent keystoneclient warning us that it has no log handlers
+    logging.getLogger('keystoneclient').addHandler(NullHandler())
+    from keystoneclient.v2_0 import client as ksclient_v2
+except ImportError:
+    pass
+try:
+    from keystoneclient.v3 import client as ksclient_v3
+except ImportError:
+    pass
+
 # requests version 1.2.3 try to encode headers in ascii, preventing
 # utf-8 encoded header to be 'prepared'
 if StrictVersion(requests.__version__) < StrictVersion('2.0.0'):
@@ -540,25 +553,6 @@ def get_keystoneclient_2_0(auth_url, user, key, os_options, **kwargs):
     return get_auth_keystone(auth_url, user, key, os_options, **kwargs)
 
 
-def _import_keystone_client(auth_version):
-    # the attempted imports are encapsulated in this function to allow
-    # mocking for tests
-    try:
-        if auth_version in AUTH_VERSIONS_V3:
-            from keystoneclient.v3 import client as ksclient
-        else:
-            from keystoneclient.v2_0 import client as ksclient
-        from keystoneclient import exceptions
-        # prevent keystoneclient warning us that it has no log handlers
-        logging.getLogger('keystoneclient').addHandler(NullHandler())
-        return ksclient, exceptions
-    except ImportError:
-        raise ClientException('''
-Auth versions 2.0 and 3 require python-keystoneclient, install it or use Auth
-version 1.0 which requires ST_AUTH, ST_USER, and ST_KEY environment
-variables to be set or overridden with -A, -U, or -K.''')
-
-
 def get_auth_keystone(auth_url, user, key, os_options, **kwargs):
     """
     Authenticate against a keystone server.
@@ -587,7 +581,20 @@ def get_auth_keystone(auth_url, user, key, os_options, **kwargs):
     # Legacy default if not set
     if auth_version is None:
         auth_version = '2'
-    ksclient, exceptions = _import_keystone_client(auth_version)
+
+    ksclient = None
+    if auth_version in AUTH_VERSIONS_V3:
+        if ksclient_v3 is not None:
+            ksclient = ksclient_v3
+    else:
+        if ksclient_v2 is not None:
+            ksclient = ksclient_v2
+
+    if ksclient is None:
+        raise ClientException('''
+Auth versions 2.0 and 3 require python-keystoneclient, install it or use Auth
+version 1.0 which requires ST_AUTH, ST_USER, and ST_KEY environment
+variables to be set or overridden with -A, -U, or -K.''')
 
     try:
         _ksclient = ksclient.Client(
@@ -608,13 +615,13 @@ def get_auth_keystone(auth_url, user, key, os_options, **kwargs):
             cert=kwargs.get('cert'),
             key=kwargs.get('cert_key'),
             auth_url=auth_url, insecure=insecure, timeout=timeout)
-    except exceptions.Unauthorized:
+    except ksexceptions.Unauthorized:
         msg = 'Unauthorized. Check username, password and tenant name/id.'
         if auth_version in AUTH_VERSIONS_V3:
             msg = ('Unauthorized. Check username/id, password, '
                    'tenant name/id and user/tenant domain name/id.')
         raise ClientException(msg)
-    except exceptions.AuthorizationFailure as err:
+    except ksexceptions.AuthorizationFailure as err:
         raise ClientException('Authorization Failure. %s' % err)
     service_type = os_options.get('service_type') or 'object-store'
     endpoint_type = os_options.get('endpoint_type') or 'publicURL'
@@ -627,7 +634,7 @@ def get_auth_keystone(auth_url, user, key, os_options, **kwargs):
             service_type=service_type,
             endpoint_type=endpoint_type,
             **filter_kwargs)
-    except exceptions.EndpointNotFound:
+    except ksexceptions.EndpointNotFound:
         raise ClientException('Endpoint for %s not found - '
                               'have you specified a region?' % service_type)
     return endpoint, _ksclient.auth_token

tests/unit/test_shell.py

@@ -37,7 +37,7 @@
 
 from os.path import basename, dirname
 from .utils import (
-    CaptureOutput, fake_get_auth_keystone, _make_fake_import_keystone_client,
+    CaptureOutput, fake_get_auth_keystone,
     FakeKeystone, StubResponse, MockHttpTest)
 from swiftclient.utils import (
     EMPTY_ETAG, EXPIRES_ISO8601_FORMAT,
@@ -2534,16 +2534,26 @@ def _test_options_passed_to_keystone(self, cmd, opts, os_opts,
                               cmd_args=cmd_args)
         ks_endpoint = 'http://example.com:8080/v1/AUTH_acc'
         ks_token = 'fake_auth_token'
+        # check correct auth version gets used
+        key = 'auth-version'
         fake_ks = FakeKeystone(endpoint=ks_endpoint, token=ks_token)
+        if no_auth:
+            fake_ks2 = fake_ks3 = None
+        elif opts.get(key, self.defaults.get(key)) == '2.0':
+            fake_ks2 = fake_ks
+            fake_ks3 = None
+        else:
+            fake_ks2 = None
+            fake_ks3 = fake_ks
         # fake_conn will check that storage_url and auth_token are as expected
         endpoint = os_opts.get('storage-url', ks_endpoint)
         token = os_opts.get('auth-token', ks_token)
         fake_conn = self.fake_http_connection(204, headers={},
                                               storage_url=endpoint,
                                               auth_token=token)
 
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)), \
+        with mock.patch('swiftclient.client.ksclient_v2', fake_ks2), \
+                mock.patch('swiftclient.client.ksclient_v3', fake_ks3), \
                 mock.patch('swiftclient.client.http_connection', fake_conn), \
                 mock.patch.dict(os.environ, env, clear=True), \
                 patch_disable_warnings() as mock_disable_warnings:
@@ -2562,16 +2572,11 @@ def _test_options_passed_to_keystone(self, cmd, opts, os_opts,
                 self.assertEqual([], mock_disable_warnings.mock_calls)
 
         if no_auth:
-            # check that keystone client was not used and terminate tests
-            self.assertIsNone(getattr(fake_ks, 'auth_version'))
-            self.assertEqual(len(fake_ks.calls), 0)
+            # We patched out both keystoneclient versions to be None;
+            # they *can't* have been used and if we tried to, we would
+            # have raised ClientExceptions
             return
 
-        # check correct auth version was passed to _import_keystone_client
-        key = 'auth-version'
-        expected = opts.get(key, self.defaults.get(key))
-        self.assertEqual(expected, fake_ks.auth_version)
-
         # check args passed to keystone Client __init__
         self.assertEqual(len(fake_ks.calls), 1)
         actual_args = fake_ks.calls[0]
@@ -2942,9 +2947,9 @@ def setUp(self):
         self.account = 'AUTH_alice'
 
         # keystone returns endpoint for another account
-        fake_ks = FakeKeystone(endpoint='http://example.com:8080/v1/AUTH_bob',
-                               token='bob_token')
-        self.fake_ks_import = _make_fake_import_keystone_client(fake_ks)
+        self.fake_ks = FakeKeystone(
+            endpoint='http://example.com:8080/v1/AUTH_bob',
+            token='bob_token')
 
         self.cont = 'c1'
         self.cont_path = '/v1/%s/%s' % (self.account, self.cont)
@@ -3023,8 +3028,7 @@ def test_upload_with_read_write_access(self):
 
         args, env = self._make_cmd('upload', cmd_args=[self.cont, self.obj,
                                                        '--leave-segments'])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3046,8 +3050,7 @@ def test_upload_with_write_only_access(self):
                                               on_request=req_handler)
         args, env = self._make_cmd('upload', cmd_args=[self.cont, self.obj,
                                                        '--leave-segments'])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3073,8 +3076,7 @@ def test_segment_upload_with_write_only_access(self):
                                              '--segment-size=10',
                                              '--segment-container=%s'
                                              % self.cont])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3112,8 +3114,7 @@ def test_segment_upload_with_write_only_access_segments_container(self):
                                    cmd_args=[self.cont, self.obj,
                                              '--leave-segments',
                                              '--segment-size=10'])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3149,8 +3150,7 @@ def test_upload_with_no_access(self):
 
         args, env = self._make_cmd('upload', cmd_args=[self.cont, self.obj,
                                                        '--leave-segments'])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3207,8 +3207,7 @@ def test_download_with_read_write_access(self):
         args, env = self._make_cmd('download', cmd_args=[self.cont,
                                                          self.obj.lstrip('/'),
                                                          '--no-download'])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3229,8 +3228,7 @@ def test_download_with_read_only_access(self):
         args, env = self._make_cmd('download', cmd_args=[self.cont,
                                                          self.obj.lstrip('/'),
                                                          '--no-download'])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3248,8 +3246,7 @@ def test_download_with_no_access(self):
         args, env = self._make_cmd('download', cmd_args=[self.cont,
                                                          self.obj.lstrip('/'),
                                                          '--no-download'])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3273,8 +3270,7 @@ def test_list_with_read_access(self):
         fake_conn = self.fake_http_connection(resp, on_request=req_handler)
 
         args, env = self._make_cmd('download', cmd_args=[self.cont])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:
@@ -3291,8 +3287,7 @@ def test_list_with_no_access(self):
         fake_conn = self.fake_http_connection(403)
 
         args, env = self._make_cmd('download', cmd_args=[self.cont])
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        self.fake_ks_import):
+        with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks):
             with mock.patch('swiftclient.client.http_connection', fake_conn):
                 with mock.patch.dict(os.environ, env):
                     with CaptureOutput() as out:

tests/unit/test_swiftclient.py

@@ -29,7 +29,7 @@
 from requests.exceptions import RequestException
 
 from .utils import (MockHttpTest, fake_get_auth_keystone, StubResponse,
-                    FakeKeystone, _make_fake_import_keystone_client)
+                    FakeKeystone)
 
 from swiftclient.utils import EMPTY_ETAG
 from swiftclient.exceptions import ClientException
@@ -322,8 +322,7 @@ def test_auth_v2_timeout(self):
         # TestConnection.test_timeout_passed_down but is required to check that
         # get_auth does the right thing when it is not passed a timeout arg
         fake_ks = FakeKeystone(endpoint='http://some_url', token='secret')
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)):
+        with mock.patch('swiftclient.client.ksclient_v2', fake_ks):
             c.get_auth('http://www.test.com', 'asdf', 'asdf',
                        os_options=dict(tenant_name='tenant'),
                        auth_version="2.0", timeout=42.0)
@@ -580,17 +579,15 @@ def test_get_keystone_client_2_0(self):
     def test_get_auth_keystone_versionless(self):
         fake_ks = FakeKeystone(endpoint='http://some_url', token='secret')
 
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)):
+        with mock.patch('swiftclient.client.ksclient_v3', fake_ks):
             c.get_auth_keystone('http://authurl', 'user', 'key', {})
         self.assertEqual(1, len(fake_ks.calls))
         self.assertEqual('http://authurl/v3', fake_ks.calls[0].get('auth_url'))
 
     def test_get_auth_keystone_versionless_auth_version_set(self):
         fake_ks = FakeKeystone(endpoint='http://some_url', token='secret')
 
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)):
+        with mock.patch('swiftclient.client.ksclient_v2', fake_ks):
             c.get_auth_keystone('http://auth_url', 'user', 'key',
                                 {}, auth_version='2.0')
         self.assertEqual(1, len(fake_ks.calls))
@@ -600,8 +597,7 @@ def test_get_auth_keystone_versionless_auth_version_set(self):
     def test_get_auth_keystone_versionful(self):
         fake_ks = FakeKeystone(endpoint='http://some_url', token='secret')
 
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)):
+        with mock.patch('swiftclient.client.ksclient_v3', fake_ks):
             c.get_auth_keystone('http://auth_url/v3', 'user', 'key',
                                 {}, auth_version='3')
         self.assertEqual(1, len(fake_ks.calls))
@@ -611,8 +607,7 @@ def test_get_auth_keystone_versionful(self):
     def test_get_auth_keystone_devstack_versionful(self):
         fake_ks = FakeKeystone(
             endpoint='http://storage.example.com/v1/AUTH_user', token='secret')
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)):
+        with mock.patch('swiftclient.client.ksclient_v3', fake_ks):
             c.get_auth_keystone('https://192.168.8.8/identity/v3',
                                 'user', 'key', {}, auth_version='3')
         self.assertEqual(1, len(fake_ks.calls))
@@ -622,8 +617,7 @@ def test_get_auth_keystone_devstack_versionful(self):
     def test_get_auth_keystone_devstack_versionless(self):
         fake_ks = FakeKeystone(
             endpoint='http://storage.example.com/v1/AUTH_user', token='secret')
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)):
+        with mock.patch('swiftclient.client.ksclient_v3', fake_ks):
             c.get_auth_keystone('https://192.168.8.8/identity',
                                 'user', 'key', {}, auth_version='3')
         self.assertEqual(1, len(fake_ks.calls))
@@ -634,8 +628,7 @@ def test_auth_keystone_url_some_junk_nonsense(self):
         fake_ks = FakeKeystone(
             endpoint='http://storage.example.com/v1/AUTH_user',
             token='secret')
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)):
+        with mock.patch('swiftclient.client.ksclient_v3', fake_ks):
             c.get_auth_keystone('http://blah.example.com/v2moo',
                                 'user', 'key', {}, auth_version='3')
         self.assertEqual(1, len(fake_ks.calls))
@@ -2456,8 +2449,7 @@ def shim_connection(*a, **kw):
             'http://auth.example.com', 'user', 'password', timeout=33.0,
             os_options=os_options, auth_version=2.0)
         fake_ks = FakeKeystone(endpoint='http://some_url', token='secret')
-        with mock.patch('swiftclient.client._import_keystone_client',
-                        _make_fake_import_keystone_client(fake_ks)):
+        with mock.patch('swiftclient.client.ksclient_v2', fake_ks):
             with mock.patch.multiple('swiftclient.client',
                                      http_connection=shim_connection,
                                      sleep=mock.DEFAULT):

tests/unit/utils.py

@@ -542,14 +542,6 @@ class EndpointNotFound(Exception):
         pass
 
 
-def _make_fake_import_keystone_client(fake_import):
-    def _fake_import_keystone_client(auth_version):
-        fake_import.auth_version = auth_version
-        return fake_import, fake_import
-
-    return _fake_import_keystone_client
-
-
 class FakeStream(object):
     def __init__(self, size):
         self.bytes_read = 0