Strip prefix when paginating

The way the next URL is parsed and rebuilt in the pagination code
doesn't take a prefixed path into account like devstack uses, e.g.
/baremetal/v1/nodes. It ends up prepending the base URL and we get
/baremetal/baremetal/v1/nodes. Drop the extra prefix when building
this URL.

Change-Id: I7e46068521c40f19c1e48eedc69b4ecd862e88fc
Story: 2006216
Task: 35809

ironicclient/common/base.py

@@ -155,6 +155,20 @@ class Manager(object):
             kwargs['headers'] = {'X-OpenStack-Ironic-API-Version':
                                  os_ironic_api_version}
 
+        # NOTE(jroll)
+        # endpoint_trimmed is what is prepended if we only pass a path
+        # to json_request. This might be something like
+        # https://example.com:4443/baremetal
+        # The code below which parses the 'next' URL keeps any path prefix
+        # we're using, so it ends up calling json_request() with e.g.
+        # /baremetal/v1/nodes. When this is appended to endpoint_trimmed,
+        # we end up with a full URL of e.g.
+        # https://example.com:4443/baremetal/baremetal/v1/nodes.
+        # Parse out the prefix from the base endpoint here, so we can strip
+        # it below and make sure we're passing a correct path.
+        endpoint_parts = urlparse.urlparse(self.api.endpoint_trimmed)
+        url_path_prefix = endpoint_parts[2]
+
         object_list = []
         object_count = 0
         limit_reached = False
@@ -179,6 +193,9 @@ class Manager(object):
                 # the scheme and netloc
                 url_parts = list(urlparse.urlparse(url))
                 url_parts[0] = url_parts[1] = ''
+                if url_path_prefix:
+                    url_parts[2] = url_parts[2].replace(
+                        url_path_prefix, '', 1)
                 url = urlparse.urlunparse(url_parts)
 
         return object_list

ironicclient/common/http.py

@@ -614,20 +614,25 @@ class SessionClient(VersionNegotiationMixin, adapter.LegacyJsonAdapter):
 
         super(SessionClient, self).__init__(**kwargs)
 
+        endpoint_filter = self._get_endpoint_filter()
+        endpoint = self.session.get_endpoint(**endpoint_filter)
+        self.endpoint_trimmed = _trim_endpoint_api_version(endpoint)
+
     def _parse_version_headers(self, resp):
         return self._generic_parse_version_headers(resp.headers.get)
 
-    def _make_simple_request(self, conn, method, url):
+    def _get_endpoint_filter(self):
-        endpoint_filter = {
+        return {
             'interface': self.interface,
             'service_type': self.service_type,
             'region_name': self.region_name
         }
 
+    def _make_simple_request(self, conn, method, url):
         # NOTE: conn is self.session for this class
         return conn.request(url, method, raise_exc=False,
                             user_agent=USER_AGENT,
-                            endpoint_filter=endpoint_filter,
+                            endpoint_filter=self._get_endpoint_filter(),
                             endpoint_override=self.endpoint_override)
 
     @with_retries

ironicclient/tests/unit/common/test_http.py

@@ -642,7 +642,8 @@ class SessionClientTest(utils.BaseTestCase):
 
     @mock.patch.object(http.LOG, 'warning', autospec=True)
     def test_session_client_endpoint_deprecation(self, log_mock):
-        http.SessionClient(os_ironic_api_version=1, session=mock.Mock(),
+        http.SessionClient(os_ironic_api_version=1,
+                           session=utils.mockSession({}),
                            api_version_select_state='user', max_retries=5,
                            retry_interval=5, endpoint='abc')
         self.assertIn('deprecated', log_mock.call_args[0][0])
@@ -728,7 +729,7 @@ class SessionClientTest(utils.BaseTestCase):
         self._test_endpoint_override(True)
 
     def test_make_simple_request(self):
-        session = mock.Mock(spec=['request'])
+        session = utils.mockSession({})
 
         client = _session_client(session=session,
                                  endpoint_override='http://127.0.0.1')
@@ -890,7 +891,7 @@ class RetriesTestCase(utils.BaseTestCase):
             {'Content-Type': 'application/json'},
             b"OK",
             http_client.OK)
-        fake_session = mock.Mock(spec=requests.Session)
+        fake_session = utils.mockSession({})
         fake_session.request.side_effect = iter((fake_resp, ok_resp))
 
         client = _session_client(session=fake_session)
@@ -908,7 +909,7 @@ class RetriesTestCase(utils.BaseTestCase):
             {'Content-Type': 'application/json'},
             b"OK",
             http_client.OK)
-        fake_session = mock.Mock(spec=requests.Session)
+        fake_session = utils.mockSession({})
         fake_session.request.side_effect = iter((fake_resp, ok_resp))
 
         client = _session_client(session=fake_session)
@@ -920,7 +921,7 @@ class RetriesTestCase(utils.BaseTestCase):
             {'Content-Type': 'application/json'},
             b"OK",
             http_client.OK)
-        fake_session = mock.Mock(spec=requests.Session)
+        fake_session = utils.mockSession({})
         fake_session.request.side_effect = iter((exc.ConnectionRefused(),
                                                  ok_resp))
 
@@ -933,7 +934,7 @@ class RetriesTestCase(utils.BaseTestCase):
             {'Content-Type': 'application/json'},
             b"OK",
             http_client.OK)
-        fake_session = mock.Mock(spec=requests.Session)
+        fake_session = utils.mockSession({})
         fake_session.request.side_effect = iter(
             (kexc.RetriableConnectionFailure(), ok_resp))
 
@@ -948,7 +949,7 @@ class RetriesTestCase(utils.BaseTestCase):
             {'Content-Type': 'application/json'},
             error_body,
             http_client.CONFLICT)
-        fake_session = mock.Mock(spec=requests.Session)
+        fake_session = utils.mockSession({})
         fake_session.request.return_value = fake_resp
 
         client = _session_client(session=fake_session)
@@ -965,7 +966,7 @@ class RetriesTestCase(utils.BaseTestCase):
             {'Content-Type': 'application/json'},
             error_body,
             http_client.CONFLICT)
-        fake_session = mock.Mock(spec=requests.Session)
+        fake_session = utils.mockSession({})
         fake_session.request.return_value = fake_resp
 
         client = _session_client(session=fake_session)
@@ -983,7 +984,7 @@ class RetriesTestCase(utils.BaseTestCase):
             {'Content-Type': 'application/json'},
             error_body,
             http_client.CONFLICT)
-        fake_session = mock.Mock(spec=requests.Session)
+        fake_session = utils.mockSession({})
         fake_session.request.return_value = fake_resp
 
         client = _session_client(session=fake_session)

ironicclient/tests/unit/test_client.py

@@ -58,11 +58,19 @@ class ClientTest(utils.BaseTestCase):
                         [o.dest for o in session_loader_options]}
         mock_ks_session.return_value.load_from_options.assert_called_once_with(
             auth='auth', **session_opts)
+        get_endpoint_call = mock.call(
+            service_type=kwargs.get('service_type') or 'baremetal',
+            interface=expected_interface,
+            region_name=kwargs.get('region_name'))
         if not {'endpoint', 'ironic_url'}.intersection(kwargs):
-            session.get_endpoint.assert_called_once_with(
+            calls = [get_endpoint_call,
-                service_type=kwargs.get('service_type') or 'baremetal',
+                     mock.call(interface=client.http_client.interface,
-                interface=expected_interface,
+                               service_type=client.http_client.service_type,
-                region_name=kwargs.get('region_name'))
+                               region_name=client.http_client.region_name)]
+            self.assertEqual(calls, session.get_endpoint.call_args_list)
+        else:
+            self.assertEqual([get_endpoint_call],
+                             session.get_endpoint.call_args_list)
         if 'os_ironic_api_version' in kwargs:
             # NOTE(TheJulia): This does not test the negotiation logic
             # as a request must be triggered in order for any version
@@ -279,9 +287,10 @@ class ClientTest(utils.BaseTestCase):
             'session': session,
         }
         iroclient.get_client('1', **kwargs)
-        session.get_endpoint.assert_called_once_with(service_type='baremetal',
+        endpoint_calls = 2 * [mock.call(service_type='baremetal',
-                                                     interface=None,
+                                        interface=None,
-                                                     region_name=None)
+                                        region_name=None)]
+        self.assertEqual(endpoint_calls, session.get_endpoint.call_args_list)
 
     def test_get_client_incorrect_session_passed(self):
         session = mock.Mock()
@@ -302,7 +311,7 @@ class ClientTest(utils.BaseTestCase):
         with mock.patch.object(loader_class, '__init__',
                                autospec=True) as init_mock:
             init_mock.return_value = None
-            iroclient.get_client('1', **passed_kwargs)
+            client = iroclient.get_client('1', **passed_kwargs)
             iroclient.convert_keystoneauth_opts(passed_kwargs)
 
         init_mock.assert_called_once_with(mock.ANY, **expected_kwargs)
@@ -312,9 +321,16 @@ class ClientTest(utils.BaseTestCase):
             auth=mock.ANY, **session_opts)
         if 'ironic_url' not in passed_kwargs:
             service_type = passed_kwargs.get('service_type') or 'baremetal'
-            session.get_endpoint.assert_called_once_with(
+            endpoint_calls = [
-                service_type=service_type, interface=expected_interface,
+                mock.call(service_type=service_type,
-                region_name=passed_kwargs.get('region_name'))
+                          interface=expected_interface,
+                          region_name=passed_kwargs.get('region_name')),
+                mock.call(service_type=client.http_client.service_type,
+                          interface=client.http_client.interface,
+                          region_name=client.http_client.region_name)
+            ]
+            self.assertEqual(endpoint_calls,
+                             session.get_endpoint.call_args_list)
 
     def test_loader_arguments_admin_token(self):
         passed_kwargs = {

ironicclient/tests/unit/utils.py

@@ -40,11 +40,16 @@ class BaseTestCase(testtools.TestCase):
 
 
 class FakeAPI(object):
-    def __init__(self, responses):
+    def __init__(self, responses, path_prefix=None):
         self.responses = responses
         self.calls = []
+        self.path_prefix = path_prefix or ''
+        self.endpoint_trimmed = 'http://127.0.0.1:6385' + self.path_prefix
 
     def _request(self, method, url, headers=None, body=None, params=None):
+        # url should always just be a path here, e.g. /v1/nodes
+        url = self.path_prefix + url
+
         call = (method, url, headers or {}, body)
         if params:
             call += (params,)
@@ -62,7 +67,7 @@ class FakeAPI(object):
 
 
 class FakeConnection(object):
-    def __init__(self, response=None):
+    def __init__(self, response=None, path_prefix=None):
         self._response = response
         self._last_request = None
 
@@ -135,6 +140,7 @@ def mockSession(headers, content=None, status_code=None, version=None):
     session = mock.Mock(spec=requests.Session,
                         verify=False,
                         cert=('test_cert', 'test_key'))
+    session.get_endpoint = mock.Mock(return_value='https://test')
     response = mockSessionResponse(headers, content, status_code, version)
     session.request = mock.Mock(return_value=response)
 

ironicclient/tests/unit/v1/test_node.py

@@ -575,6 +575,24 @@ fake_responses_pagination = {
     },
 }
 
+fake_responses_pagination_path_prefix = {
+    '/baremetal/v1/nodes':
+    {
+        'GET': (
+            {},
+            {"nodes": [NODE1],
+             "next": "http://127.0.0.1:6385/baremetal/v1/nodes/?limit=1"}
+        ),
+    },
+    '/baremetal/v1/nodes/?limit=1':
+    {
+        'GET': (
+            {},
+            {"nodes": [NODE2]}
+        ),
+    },
+}
+
 fake_responses_sorting = {
     '/v1/nodes/?sort_key=updated_at':
     {
@@ -699,6 +717,18 @@ class NodeManagerTest(testtools.TestCase):
         self.assertEqual(expect, self.api.calls)
         self.assertEqual(2, len(nodes))
 
+    def test_node_list_pagination_no_limit_path_prefix(self):
+        self.api = utils.FakeAPI(fake_responses_pagination_path_prefix,
+                                 path_prefix='/baremetal')
+        self.mgr = node.NodeManager(self.api)
+        nodes = self.mgr.list(limit=0)
+        expect = [
+            ('GET', '/baremetal/v1/nodes', {}, None),
+            ('GET', '/baremetal/v1/nodes/?limit=1', {}, None)
+        ]
+        self.assertEqual(expect, self.api.calls)
+        self.assertEqual(2, len(nodes))
+
     def test_node_list_sort_key(self):
         self.api = utils.FakeAPI(fake_responses_sorting)
         self.mgr = node.NodeManager(self.api)