From e0f5072907a00d48a183dd8fc91a6cf6038ca279 Mon Sep 17 00:00:00 2001
From: melanie witt <melwitt@yahoo-inc.com>
Date: Sat, 14 Mar 2015 23:39:05 +0000
Subject: [PATCH] Ensure the use of volume endpoint in volumes apis

Currently, several of the volumes apis depend on callers to pass
service_type='volume' as a parameter when creating a client object,
to route correctly. The problem is, it makes it impossible for
callers to work with both the compute and volume endpoints at the
same time. They can either work with compute and have volumes.* calls
return 404, or they can work with volume and have servers.* images.*
flavors.* calls return 404.

The CLI sets service_type='volume' for client objects for the
following commands via a decorator:

volume-list
volume-show
volume-create
volume-delete
volume-snapshot-list
volume-snapshot-show
volume-snapshot-create
volume-snapshot-delete
volume-type-list
volume-type-create
volume-type-delete

With this change, the service_type 'volume' is set in the api,
so the decorators on the shell commands are no longer needed.

Closes-Bug: #1431154

Change-Id: I11b48ac14fa4c64d8aae528552ec5b363be384c5
---
 novaclient/base.py                            |   9 ++
 novaclient/client.py                          |  34 ++++--
 .../tests/functional/test_volumes_api.py      | 111 ++++++++++++++++++
 novaclient/tests/unit/test_client.py          |   3 +
 novaclient/tests/unit/test_http.py            |   1 +
 novaclient/tests/unit/v2/fakes.py             |   1 +
 novaclient/tests/unit/v2/test_auth.py         |   1 +
 novaclient/v2/shell.py                        |  11 --
 novaclient/v2/volume_snapshots.py             |  26 ++--
 novaclient/v2/volume_types.py                 |  23 ++--
 novaclient/v2/volumes.py                      |  44 ++++---
 11 files changed, 204 insertions(+), 60 deletions(-)
 create mode 100644 novaclient/tests/functional/test_volumes_api.py

diff --git a/novaclient/base.py b/novaclient/base.py
index 8cee35ae2..01b4bac94 100644
--- a/novaclient/base.py
+++ b/novaclient/base.py
@@ -80,6 +80,15 @@ class Manager(base.HookableMixin):
                 return [obj_class(self, res, loaded=True)
                         for res in data if res]
 
+    @contextlib.contextmanager
+    def alternate_service_type(self, service_type):
+        original_service_type = self.api.client.service_type
+        self.api.client.service_type = service_type
+        try:
+            yield
+        finally:
+            self.api.client.service_type = original_service_type
+
     @contextlib.contextmanager
     def completion_cache(self, cache_type, obj_class, mode):
         """
diff --git a/novaclient/client.py b/novaclient/client.py
index c40c21a18..32eb77505 100644
--- a/novaclient/client.py
+++ b/novaclient/client.py
@@ -212,6 +212,11 @@ class HTTPClient(object):
                 # otherwise we will get all the requests logging messages
                 rql.setLevel(logging.WARNING)
 
+        # NOTE(melwitt): Service catalog is only set if bypass_url isn't
+        #                used. Otherwise, we can cache using services_url.
+        self.service_catalog = None
+        self.services_url = {}
+
     def use_token_cache(self, use_it):
         self.os_cache = use_it
 
@@ -405,7 +410,12 @@ class HTTPClient(object):
             path = re.sub(r'v[1-9]/[a-z0-9]+$', '', path)
             url = parse.urlunsplit((scheme, netloc, path, None, None))
         else:
-            url = self.management_url + url
+            if self.service_catalog:
+                url = self.get_service_url(self.service_type) + url
+            else:
+                # NOTE(melwitt): The service catalog is not available
+                #                when bypass_url is used.
+                url = self.management_url + url
 
         # Perform the request once. If we get a 401 back then it
         # might be because the auth token expired, so try to
@@ -448,6 +458,19 @@ class HTTPClient(object):
     def delete(self, url, **kwargs):
         return self._cs_request(url, 'DELETE', **kwargs)
 
+    def get_service_url(self, service_type):
+        if service_type not in self.services_url:
+            url = self.service_catalog.url_for(
+                attr='region',
+                filter_value=self.region_name,
+                endpoint_type=self.endpoint_type,
+                service_type=service_type,
+                service_name=self.service_name,
+                volume_service_name=self.volume_service_name,)
+            url = url.rstrip('/')
+            self.services_url[service_type] = url
+        return self.services_url[service_type]
+
     def _extract_service_catalog(self, url, resp, body, extract_token=True):
         """See what the auth service told us and process the response.
         We may get redirected to another site, fail or actually get
@@ -464,14 +487,7 @@ class HTTPClient(object):
                     self.auth_token = self.service_catalog.get_token()
                     self.tenant_id = self.service_catalog.get_tenant_id()
 
-                management_url = self.service_catalog.url_for(
-                    attr='region',
-                    filter_value=self.region_name,
-                    endpoint_type=self.endpoint_type,
-                    service_type=self.service_type,
-                    service_name=self.service_name,
-                    volume_service_name=self.volume_service_name,)
-                self.management_url = management_url.rstrip('/')
+                self.management_url = self.get_service_url(self.service_type)
                 return None
             except exceptions.AmbiguousEndpoints:
                 print(_("Found more than one valid endpoint. Use a more "
diff --git a/novaclient/tests/functional/test_volumes_api.py b/novaclient/tests/functional/test_volumes_api.py
new file mode 100644
index 000000000..e427c2e21
--- /dev/null
+++ b/novaclient/tests/functional/test_volumes_api.py
@@ -0,0 +1,111 @@
+#    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 os
+import time
+import uuid
+
+import six.moves
+
+from novaclient import client
+from novaclient import exceptions
+from novaclient.tests.functional import base
+
+
+def wait_for_delete(test, name, thing, get_func):
+    thing.delete()
+    for x in six.moves.range(60):
+        try:
+            thing = get_func(thing.id)
+        except exceptions.NotFound:
+            break
+        time.sleep(1)
+    else:
+        test.fail('%s %s still not deleted after 60s' % (name, thing.id))
+
+
+class TestVolumesAPI(base.ClientTestBase):
+
+    def setUp(self):
+        super(TestVolumesAPI, self).setUp()
+        user = os.environ['OS_USERNAME']
+        passwd = os.environ['OS_PASSWORD']
+        tenant = os.environ['OS_TENANT_NAME']
+        auth_url = os.environ['OS_AUTH_URL']
+
+        self.client = client.Client(2, user, passwd, tenant, auth_url=auth_url)
+
+    def test_volumes_snapshots_types_create_get_list_delete(self):
+        # Create a volume
+        volume = self.client.volumes.create(1)
+
+        # Make sure we can still list servers after using the volume endpoint
+        self.client.servers.list()
+
+        # This cleanup tests volume delete
+        self.addCleanup(volume.delete)
+
+        # Wait for the volume to become available
+        for x in six.moves.range(60):
+            volume = self.client.volumes.get(volume.id)
+            if volume.status == 'available':
+                break
+            elif volume.status == 'error':
+                self.fail('Volume %s is in error state' % volume.id)
+            time.sleep(1)
+        else:
+            self.fail('Volume %s not available after 60s' % volume.id)
+
+        # List all volumes
+        self.client.volumes.list()
+
+        # Create a volume snapshot
+        snapshot = self.client.volume_snapshots.create(volume.id)
+
+        # This cleanup tests volume snapshot delete. The volume
+        # can't be deleted until the dependent snapshot is gone
+        self.addCleanup(wait_for_delete, self, 'Snapshot', snapshot,
+                        self.client.volume_snapshots.get)
+
+        # Wait for the snapshot to become available
+        for x in six.moves.range(60):
+            snapshot = self.client.volume_snapshots.get(snapshot.id)
+            if snapshot.status == 'available':
+                break
+            elif snapshot.status == 'error':
+                self.fail('Snapshot %s is in error state' % snapshot.id)
+            time.sleep(1)
+        else:
+            self.fail('Snapshot %s not available after 60s' % snapshot.id)
+
+        # List snapshots
+        self.client.volume_snapshots.list()
+
+        # List servers again to make sure things are still good
+        self.client.servers.list()
+
+        # Create a volume type
+        # TODO(melwitt): Use a better random name
+        name = str(uuid.uuid4())
+        volume_type = self.client.volume_types.create(name)
+
+        # This cleanup tests volume type delete
+        self.addCleanup(self.client.volume_types.delete, volume_type.id)
+
+        # Get the volume type
+        volume_type = self.client.volume_types.get(volume_type.id)
+
+        # List all volume types
+        self.client.volume_types.list()
+
+        # One more servers list
+        self.client.servers.list()
diff --git a/novaclient/tests/unit/test_client.py b/novaclient/tests/unit/test_client.py
index db4c87d09..d240321f7 100644
--- a/novaclient/tests/unit/test_client.py
+++ b/novaclient/tests/unit/test_client.py
@@ -86,6 +86,7 @@ class ClientTest(utils.TestCase):
                                                 auth_url="http://www.blah.com")
         instance.auth_token = 'foobar'
         instance.management_url = 'http://example.com'
+        instance.get_service_url = mock.Mock(return_value='http://example.com')
         instance.version = 'v2.0'
         mock_request = mock.Mock()
         mock_request.side_effect = novaclient.exceptions.Unauthorized(401)
@@ -136,6 +137,8 @@ class ClientTest(utils.TestCase):
                                                 auth_url="http://www.blah.com")
         instance.auth_token = 'foobar'
         instance.management_url = management_url % projectid
+        mock_get_service_url = mock.Mock(return_value=instance.management_url)
+        instance.get_service_url = mock_get_service_url
         instance.version = 'v2.0'
 
         # If passing None as the part of url, a client accesses the url which
diff --git a/novaclient/tests/unit/test_http.py b/novaclient/tests/unit/test_http.py
index 5e676828a..a80d94426 100644
--- a/novaclient/tests/unit/test_http.py
+++ b/novaclient/tests/unit/test_http.py
@@ -82,6 +82,7 @@ def get_authed_client():
     cl = get_client()
     cl.management_url = "http://example.com"
     cl.auth_token = "token"
+    cl.get_service_url = mock.Mock(return_value="http://example.com")
     return cl
 
 
diff --git a/novaclient/tests/unit/v2/fakes.py b/novaclient/tests/unit/v2/fakes.py
index 07db412b3..343c421e2 100644
--- a/novaclient/tests/unit/v2/fakes.py
+++ b/novaclient/tests/unit/v2/fakes.py
@@ -2196,6 +2196,7 @@ class FakeSessionMockClient(base_client.SessionClient, FakeHTTPClient):
         self.callstack = []
         self.auth = mock.Mock()
         self.session = mock.Mock()
+        self.service_type = 'service_type'
 
         self.auth.get_auth_ref.return_value.project_id = 'tenant_id'
 
diff --git a/novaclient/tests/unit/v2/test_auth.py b/novaclient/tests/unit/v2/test_auth.py
index 95e96ded1..696428a4a 100644
--- a/novaclient/tests/unit/v2/test_auth.py
+++ b/novaclient/tests/unit/v2/test_auth.py
@@ -363,6 +363,7 @@ class AuthenticationTests(utils.TestCase):
                            "project_id", utils.AUTH_URL)
         http_client = cs.client
         http_client.management_url = ''
+        http_client.get_service_url = mock.Mock(return_value='')
         mock_request = mock.Mock(return_value=(None, None))
 
         @mock.patch.object(http_client, 'request', mock_request)
diff --git a/novaclient/v2/shell.py b/novaclient/v2/shell.py
index 6ce1681ab..7e3a53100 100644
--- a/novaclient/v2/shell.py
+++ b/novaclient/v2/shell.py
@@ -1983,7 +1983,6 @@ def _translate_availability_zone_keys(collection):
     type=int,
     const=1,
     help=argparse.SUPPRESS)
-@cliutils.service_type('volume')
 def do_volume_list(cs, args):
     """List all the volumes."""
     search_opts = {'all_tenants': args.all_tenants}
@@ -2002,7 +2001,6 @@ def do_volume_list(cs, args):
     'volume',
     metavar='<volume>',
     help=_('Name or ID of the volume.'))
-@cliutils.service_type('volume')
 def do_volume_show(cs, args):
     """Show details about a volume."""
     volume = _find_volume(cs, args.volume)
@@ -2055,7 +2053,6 @@ def do_volume_show(cs, args):
     '--availability-zone', metavar='<availability-zone>',
     help=_('Optional Availability Zone for volume. (Default=None)'),
     default=None)
-@cliutils.service_type('volume')
 def do_volume_create(cs, args):
     """Add a new volume."""
     volume = cs.volumes.create(args.size,
@@ -2072,7 +2069,6 @@ def do_volume_create(cs, args):
     'volume',
     metavar='<volume>', nargs='+',
     help=_('Name or ID of the volume(s) to delete.'))
-@cliutils.service_type('volume')
 def do_volume_delete(cs, args):
     """Remove volume(s)."""
     for volume in args.volume:
@@ -2139,7 +2135,6 @@ def do_volume_detach(cs, args):
                                     args.attachment_id)
 
 
-@cliutils.service_type('volume')
 def do_volume_snapshot_list(cs, _args):
     """List all the snapshots."""
     snapshots = cs.volume_snapshots.list()
@@ -2152,7 +2147,6 @@ def do_volume_snapshot_list(cs, _args):
     'snapshot',
     metavar='<snapshot>',
     help=_('Name or ID of the snapshot.'))
-@cliutils.service_type('volume')
 def do_volume_snapshot_show(cs, args):
     """Show details about a snapshot."""
     snapshot = _find_volume_snapshot(cs, args.snapshot)
@@ -2185,7 +2179,6 @@ def do_volume_snapshot_show(cs, args):
 @cliutils.arg(
     '--display_description',
     help=argparse.SUPPRESS)
-@cliutils.service_type('volume')
 def do_volume_snapshot_create(cs, args):
     """Add a new snapshot."""
     snapshot = cs.volume_snapshots.create(args.volume_id,
@@ -2199,7 +2192,6 @@ def do_volume_snapshot_create(cs, args):
     'snapshot',
     metavar='<snapshot>',
     help=_('Name or ID of the snapshot to delete.'))
-@cliutils.service_type('volume')
 def do_volume_snapshot_delete(cs, args):
     """Remove a snapshot."""
     snapshot = _find_volume_snapshot(cs, args.snapshot)
@@ -2210,7 +2202,6 @@ def _print_volume_type_list(vtypes):
     utils.print_list(vtypes, ['ID', 'Name'])
 
 
-@cliutils.service_type('volume')
 def do_volume_type_list(cs, args):
     """Print a list of available 'volume types'."""
     vtypes = cs.volume_types.list()
@@ -2221,7 +2212,6 @@ def do_volume_type_list(cs, args):
     'name',
     metavar='<name>',
     help=_("Name of the new volume type"))
-@cliutils.service_type('volume')
 def do_volume_type_create(cs, args):
     """Create a new volume type."""
     vtype = cs.volume_types.create(args.name)
@@ -2232,7 +2222,6 @@ def do_volume_type_create(cs, args):
     'id',
     metavar='<id>',
     help=_("Unique ID of the volume type to delete"))
-@cliutils.service_type('volume')
 def do_volume_type_delete(cs, args):
     """Delete a specific volume type."""
     cs.volume_types.delete(args.id)
diff --git a/novaclient/v2/volume_snapshots.py b/novaclient/v2/volume_snapshots.py
index d3bc91808..6ff6deb9e 100644
--- a/novaclient/v2/volume_snapshots.py
+++ b/novaclient/v2/volume_snapshots.py
@@ -55,11 +55,12 @@ class SnapshotManager(base.ManagerWithFind):
         :param display_description: Description of the snapshot
         :rtype: :class:`Snapshot`
         """
-        body = {'snapshot': {'volume_id': volume_id,
-                             'force': force,
-                             'display_name': display_name,
-                             'display_description': display_description}}
-        return self._create('/snapshots', body, 'snapshot')
+        with self.alternate_service_type('volume'):
+            body = {'snapshot': {'volume_id': volume_id,
+                                 'force': force,
+                                 'display_name': display_name,
+                                 'display_description': display_description}}
+            return self._create('/snapshots', body, 'snapshot')
 
     def get(self, snapshot_id):
         """
@@ -68,7 +69,8 @@ class SnapshotManager(base.ManagerWithFind):
         :param snapshot_id: The ID of the snapshot to get.
         :rtype: :class:`Snapshot`
         """
-        return self._get("/snapshots/%s" % snapshot_id, "snapshot")
+        with self.alternate_service_type('volume'):
+            return self._get("/snapshots/%s" % snapshot_id, "snapshot")
 
     def list(self, detailed=True):
         """
@@ -76,10 +78,11 @@ class SnapshotManager(base.ManagerWithFind):
 
         :rtype: list of :class:`Snapshot`
         """
-        if detailed is True:
-            return self._list("/snapshots/detail", "snapshots")
-        else:
-            return self._list("/snapshots", "snapshots")
+        with self.alternate_service_type('volume'):
+            if detailed is True:
+                return self._list("/snapshots/detail", "snapshots")
+            else:
+                return self._list("/snapshots", "snapshots")
 
     def delete(self, snapshot):
         """
@@ -87,4 +90,5 @@ class SnapshotManager(base.ManagerWithFind):
 
         :param snapshot: The :class:`Snapshot` to delete.
         """
-        self._delete("/snapshots/%s" % base.getid(snapshot))
+        with self.alternate_service_type('volume'):
+            self._delete("/snapshots/%s" % base.getid(snapshot))
diff --git a/novaclient/v2/volume_types.py b/novaclient/v2/volume_types.py
index 3d1c7f531..e36225b45 100644
--- a/novaclient/v2/volume_types.py
+++ b/novaclient/v2/volume_types.py
@@ -41,7 +41,8 @@ class VolumeTypeManager(base.ManagerWithFind):
 
         :rtype: list of :class:`VolumeType`.
         """
-        return self._list("/types", "volume_types")
+        with self.alternate_service_type('volume'):
+            return self._list("/types", "volume_types")
 
     def get(self, volume_type):
         """
@@ -50,7 +51,9 @@ class VolumeTypeManager(base.ManagerWithFind):
         :param volume_type: The ID of the :class:`VolumeType` to get.
         :rtype: :class:`VolumeType`
         """
-        return self._get("/types/%s" % base.getid(volume_type), "volume_type")
+        with self.alternate_service_type('volume'):
+            return self._get("/types/%s" % base.getid(volume_type),
+                             "volume_type")
 
     def delete(self, volume_type):
         """
@@ -58,7 +61,8 @@ class VolumeTypeManager(base.ManagerWithFind):
 
         :param volume_type: The ID of the :class:`VolumeType` to get.
         """
-        self._delete("/types/%s" % base.getid(volume_type))
+        with self.alternate_service_type('volume'):
+            self._delete("/types/%s" % base.getid(volume_type))
 
     def create(self, name):
         """
@@ -67,11 +71,10 @@ class VolumeTypeManager(base.ManagerWithFind):
         :param name: Descriptive name of the volume type
         :rtype: :class:`VolumeType`
         """
-
-        body = {
-            "volume_type": {
-                "name": name,
+        with self.alternate_service_type('volume'):
+            body = {
+                "volume_type": {
+                    "name": name,
+                }
             }
-        }
-
-        return self._create("/types", body, "volume_type")
+            return self._create("/types", body, "volume_type")
diff --git a/novaclient/v2/volumes.py b/novaclient/v2/volumes.py
index 99e3d6fed..182698ff5 100644
--- a/novaclient/v2/volumes.py
+++ b/novaclient/v2/volumes.py
@@ -60,14 +60,16 @@ class VolumeManager(base.ManagerWithFind):
         :rtype: :class:`Volume`
         :param imageRef: reference to an image stored in glance
         """
-        body = {'volume': {'size': size,
-                           'snapshot_id': snapshot_id,
-                           'display_name': display_name,
-                           'display_description': display_description,
-                           'volume_type': volume_type,
-                           'availability_zone': availability_zone,
-                           'imageRef': imageRef}}
-        return self._create('/volumes', body, 'volume')
+        # NOTE(melwitt): Ensure we use the volume endpoint for this call
+        with self.alternate_service_type('volume'):
+            body = {'volume': {'size': size,
+                               'snapshot_id': snapshot_id,
+                               'display_name': display_name,
+                               'display_description': display_description,
+                               'volume_type': volume_type,
+                               'availability_zone': availability_zone,
+                               'imageRef': imageRef}}
+            return self._create('/volumes', body, 'volume')
 
     def get(self, volume_id):
         """
@@ -76,7 +78,8 @@ class VolumeManager(base.ManagerWithFind):
         :param volume_id: The ID of the volume to get.
         :rtype: :class:`Volume`
         """
-        return self._get("/volumes/%s" % volume_id, "volume")
+        with self.alternate_service_type('volume'):
+            return self._get("/volumes/%s" % volume_id, "volume")
 
     def list(self, detailed=True, search_opts=None):
         """
@@ -84,19 +87,21 @@ class VolumeManager(base.ManagerWithFind):
 
         :rtype: list of :class:`Volume`
         """
-        search_opts = search_opts or {}
+        with self.alternate_service_type('volume'):
+            search_opts = search_opts or {}
 
-        if 'name' in search_opts.keys():
-            search_opts['display_name'] = search_opts.pop('name')
+            if 'name' in search_opts.keys():
+                search_opts['display_name'] = search_opts.pop('name')
 
-        qparams = dict((k, v) for (k, v) in six.iteritems(search_opts) if v)
+            qparams = dict((k, v) for (k, v) in
+                           six.iteritems(search_opts) if v)
 
-        query_string = '?%s' % parse.urlencode(qparams) if qparams else ''
+            query_str = '?%s' % parse.urlencode(qparams) if qparams else ''
 
-        if detailed is True:
-            return self._list("/volumes/detail%s" % query_string, "volumes")
-        else:
-            return self._list("/volumes%s" % query_string, "volumes")
+            if detailed is True:
+                return self._list("/volumes/detail%s" % query_str, "volumes")
+            else:
+                return self._list("/volumes%s" % query_str, "volumes")
 
     def delete(self, volume):
         """
@@ -104,7 +109,8 @@ class VolumeManager(base.ManagerWithFind):
 
         :param volume: The :class:`Volume` to delete.
         """
-        self._delete("/volumes/%s" % base.getid(volume))
+        with self.alternate_service_type('volume'):
+            self._delete("/volumes/%s" % base.getid(volume))
 
     def create_server_volume(self, server_id, volume_id, device):
         """