Skip to content

Commit

Permalink
Revert "Delegate authentication to quantumclient"
Browse files Browse the repository at this point in the history
This reverts commit dd9c27f.

When this patch was merged, we were suspicious about whether it was safe
to cache the client object and have it used by multiple greenthreads.
We decided it was safe and merged it.  After thinking about it and
discussing it further, it is really a bad idea.  Sharing httplib2
connections is not considered thread-safe.

quantumclient.client.HTTPClient inherits from httplib2.Http.  The
following page says sharing instances of this object between threads is
not safe:

  https://developers.google.com/api-client-library/python/guide/thread_safety

  "The google-api-python-client library is built on top of the httplib2
  library, which is not thread-safe. Therefore, if you are running as a
  multi-threaded application, each thread that you are making requests
  from must have its own instance of httplib2.Http()."

Potentially fix bug 1192131.  Even if it doesn't fix that bug, this
needs to be reverted anyway.

Change-Id: I2e4bf5e7b6458cd7b76e30847fe07f06b25c34f7
  • Loading branch information
russellb committed Jun 18, 2013
1 parent 880ddfb commit ee5d9ae
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 53 deletions.
59 changes: 23 additions & 36 deletions nova/network/quantumv2/__init__.py
Expand Up @@ -16,7 +16,9 @@
# under the License.

from oslo.config import cfg
from quantumclient.v2_0 import client
from quantumclient import client
from quantumclient.common import exceptions
from quantumclient.v2_0 import client as clientv20

from nova.openstack.common import excutils
from nova.openstack.common import log as logging
Expand All @@ -25,24 +27,27 @@
LOG = logging.getLogger(__name__)


cached_admin_client = None


def _fill_admin_details(params):
params['username'] = CONF.quantum_admin_username
params['tenant_name'] = CONF.quantum_admin_tenant_name
params['region_name'] = CONF.quantum_region_name
params['password'] = CONF.quantum_admin_password
params['auth_url'] = CONF.quantum_admin_auth_url
params['timeout'] = CONF.quantum_url_timeout
params['auth_strategy'] = CONF.quantum_auth_strategy
params['insecure'] = CONF.quantum_api_insecure
def _get_auth_token():
try:
httpclient = client.HTTPClient(
username=CONF.quantum_admin_username,
tenant_name=CONF.quantum_admin_tenant_name,
region_name=CONF.quantum_region_name,
password=CONF.quantum_admin_password,
auth_url=CONF.quantum_admin_auth_url,
timeout=CONF.quantum_url_timeout,
auth_strategy=CONF.quantum_auth_strategy,
insecure=CONF.quantum_api_insecure)
httpclient.authenticate()
return httpclient.auth_token
except exceptions.QuantumClientException as e:
with excutils.save_and_reraise_exception():
LOG.error(_('Quantum client authentication failed: %s'), e)


def _get_client(token=None):
global cached_admin_client

should_cache = False
if not token and CONF.quantum_auth_strategy:
token = _get_auth_token()
params = {
'endpoint_url': CONF.quantum_url,
'timeout': CONF.quantum_url_timeout,
Expand All @@ -51,30 +56,12 @@ def _get_client(token=None):
if token:
params['token'] = token
else:
if CONF.quantum_auth_strategy:
should_cache = True
_fill_admin_details(params)
else:
params['auth_strategy'] = None

new_client = client.Client(**params)
if should_cache:
# in this case, we don't have the token yet
try:
new_client.httpclient.authenticate()
except Exception:
with excutils.save_and_reraise_exception():
LOG.exception(_("quantum authentication failed"))

cached_admin_client = new_client
return new_client
params['auth_strategy'] = None
return clientv20.Client(**params)


def get_client(context, admin=False):
if admin:
if cached_admin_client is not None:
return cached_admin_client

token = None
else:
token = context.auth_token
Expand Down
4 changes: 1 addition & 3 deletions nova/network/quantumv2/api.py
Expand Up @@ -33,8 +33,6 @@
from nova.openstack.common import log as logging
from nova.openstack.common import uuidutils

import quantumclient.common.exceptions

quantum_opts = [
cfg.StrOpt('quantum_url',
default='http://127.0.0.1:9696',
Expand Down Expand Up @@ -723,7 +721,7 @@ def _get_floating_ips_by_fixed_and_port(self, client, fixed_ip, port):
port_id=port)
# If a quantum plugin does not implement the L3 API a 404 from
# list_floatingips will be raised.
except quantumclient.common.exceptions.QuantumClientException as e:
except quantumv2.exceptions.QuantumClientException as e:
if e.status_code == 404:
return []
raise
Expand Down
15 changes: 1 addition & 14 deletions nova/tests/network/test_quantumv2.py
Expand Up @@ -19,8 +19,6 @@

import mox
from oslo.config import cfg
from quantumclient import client as quantum_client
from quantumclient.common import exceptions as quantum_exceptions
from quantumclient.v2_0 import client

from nova.compute import flavors
Expand Down Expand Up @@ -104,17 +102,6 @@ def test_withtoken(self):
self.mox.ReplayAll()
quantumv2.get_client(my_context)

def test_cached_admin_client(self):
self.mox.StubOutWithMock(quantum_client.HTTPClient, "authenticate")

# should be called one time only
quantum_client.HTTPClient.authenticate()
self.mox.ReplayAll()

admin1 = quantumv2.get_client(None, admin=True)
admin2 = quantumv2.get_client(None, admin=True)
self.assertIs(admin1, admin2)

def test_withouttoken_keystone_connection_error(self):
self.flags(quantum_auth_strategy='keystone')
self.flags(quantum_url='http://anyhost/')
Expand Down Expand Up @@ -1236,7 +1223,7 @@ def test_remove_fixed_ip_from_instance(self):

def test_list_floating_ips_without_l3_support(self):
api = quantumapi.API()
QuantumNotFound = quantum_exceptions.QuantumClientException(
QuantumNotFound = quantumv2.exceptions.QuantumClientException(
status_code=404)
self.moxed_client.list_floatingips(
fixed_ip_address='1.1.1.1', port_id=1).AndRaise(QuantumNotFound)
Expand Down

0 comments on commit ee5d9ae

Please sign in to comment.