pass endpoint interface to http client

The 'interface' argument was being ignored so that the HTTP client was
always using the public endpoint for Ironic. This fixes it so that the
'interface' argument is taken into consideration.

There's also no need to explicitly set the interface to 'publicURL'
because that's already the default in keystoneauth.

Change-Id: I610836e5038774621690aca88b2aee25670f0262
story: 2005118
task: 29802

3 files changed, 83 insertions, 11 deletions

diff --git a/ironicclient/client.py b/ironicclient/client.py
index b785faf..83b0e14 100644
--- a/ironicclient/client.py
+++ b/ironicclient/client.py
@@ -100,14 +100,22 @@ def get_client(api_version, auth_type=None, os_ironic_api_version=None,
         session = session_loader.load_from_options(auth=auth_plugin,
                                                    **session_opts)
 
+    # Make sure we also pass the endpoint interface to the HTTP client.
+    # NOTE(gyee): we are supposed to be using valid_interfaces as interface
+    # is deprecated.
+    interface = kwargs.get('interface')
+
     endpoint = kwargs.get('endpoint')
     if not endpoint:
         try:
             # endpoint will be used to get hostname
             # and port that will be used for API version caching.
+            # NOTE(gyee): KSA defaults interface to 'public' if it is
+            # empty or None so there's no need to set it to publicURL
+            # explicitly.
             endpoint = session.get_endpoint(
                 service_type=kwargs.get('service_type') or 'baremetal',
-                interface=kwargs.get('interface') or 'publicURL',
+                interface=interface,
                 region_name=kwargs.get('region_name')
             )
         except Exception as e:
@@ -120,7 +128,8 @@ def get_client(api_version, auth_type=None, os_ironic_api_version=None,
         'max_retries': max_retries,
         'retry_interval': retry_interval,
         'session': session,
-        'endpoint_override': endpoint
+        'endpoint_override': endpoint,
+        'interface': interface
     }
 
     return Client(api_version, **ironicclient_kwargs)
diff --git a/ironicclient/tests/unit/test_client.py b/ironicclient/tests/unit/test_client.py
index 072ce37..7d10701 100644
--- a/ironicclient/tests/unit/test_client.py
+++ b/ironicclient/tests/unit/test_client.py
@@ -32,7 +32,8 @@ class ClientTest(utils.BaseTestCase):
     @mock.patch.object(kaloading, 'get_plugin_loader', autospec=True)
     def _test_get_client(self, mock_ks_loader, mock_ks_session,
                          mock_retrieve_data, warn_mock, version=None,
-                         auth='password', warn_mock_call_count=0, **kwargs):
+                         auth='password', warn_mock_call_count=0,
+                         expected_interface=None, **kwargs):
         session = mock_ks_session.return_value.load_from_options.return_value
         session.get_endpoint.return_value = 'http://localhost:6385/v1/f14b4123'
 
@@ -60,7 +61,7 @@ class ClientTest(utils.BaseTestCase):
         if not {'endpoint', 'ironic_url'}.intersection(kwargs):
             session.get_endpoint.assert_called_once_with(
                 service_type=kwargs.get('service_type') or 'baremetal',
-                interface=kwargs.get('interface') or 'publicURL',
+                interface=expected_interface,
                 region_name=kwargs.get('region_name'))
         if 'os_ironic_api_version' in kwargs:
             # NOTE(TheJulia): This does not test the negotiation logic
@@ -76,6 +77,15 @@ class ClientTest(utils.BaseTestCase):
                 port='6385')
             self.assertEqual(version or v1.DEFAULT_VER,
                              client.http_client.os_ironic_api_version)
+
+        # make sure the interface is conveyed to the client
+        if expected_interface is not None:
+            self.assertEqual(expected_interface,
+                             client.http_client.interface)
+        if kwargs.get('os_endpoint_type'):
+            self.assertEqual(kwargs['os_endpoint_type'],
+                             client.http_client.interface)
+
         return client
 
     def test_get_client_only_ironic_url(self):
@@ -128,6 +138,32 @@ class ClientTest(utils.BaseTestCase):
         }
         self._test_get_client(warn_mock_call_count=4, **kwargs)
 
+    def test_get_client_and_endpoint_type(self):
+        kwargs = {
+            'os_project_name': 'PROJECT_NAME',
+            'os_username': 'USERNAME',
+            'os_password': 'PASSWORD',
+            'os_auth_url': 'http://localhost:35357/v2.0',
+            'os_auth_token': '',
+            'os_service_type': '',
+            'os_endpoint_type': 'adminURL'
+        }
+        self._test_get_client(warn_mock_call_count=5,
+                              expected_interface='adminURL', **kwargs)
+
+    def test_get_client_and_interface(self):
+        kwargs = {
+            'os_project_name': 'PROJECT_NAME',
+            'os_username': 'USERNAME',
+            'os_password': 'PASSWORD',
+            'os_auth_url': 'http://localhost:35357/v2.0',
+            'os_auth_token': '',
+            'os_service_type': '',
+            'interface': 'internal'
+        }
+        self._test_get_client(warn_mock_call_count=4,
+                              expected_interface='internal', **kwargs)
+
     def test_get_client_with_region_no_auth_token(self):
         kwargs = {
             'os_project_name': 'PROJECT_NAME',
@@ -214,7 +250,7 @@ class ClientTest(utils.BaseTestCase):
         }
         iroclient.get_client('1', **kwargs)
         session.get_endpoint.assert_called_once_with(service_type='baremetal',
-                                                     interface='publicURL',
+                                                     interface=None,
                                                      region_name=None)
 
     def test_get_client_incorrect_session_passed(self):
@@ -229,7 +265,7 @@ class ClientTest(utils.BaseTestCase):
     @mock.patch.object(kaloading.session, 'Session', autospec=True)
     def _test_loader_arguments_passed_correctly(
             self, mock_ks_session, passed_kwargs, expected_kwargs,
-            loader_class):
+            loader_class, expected_interface=None):
         session = mock_ks_session.return_value.load_from_options.return_value
         session.get_endpoint.return_value = 'http://localhost:6385/v1/f14b4123'
 
@@ -246,9 +282,8 @@ 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'
-            interface = passed_kwargs.get('interface') or 'publicURL'
             session.get_endpoint.assert_called_once_with(
-                service_type=service_type, interface=interface,
+                service_type=service_type, interface=expected_interface,
                 region_name=passed_kwargs.get('region_name'))
 
     def test_loader_arguments_admin_token(self):
@@ -282,6 +317,24 @@ class ClientTest(utils.BaseTestCase):
             loader_class=identity.Token
         )
 
+    def test_loader_arguments_interface(self):
+        passed_kwargs = {
+            'os_auth_url': 'http://localhost:35357/v3',
+            'os_region_name': 'REGIONONE',
+            'os_auth_token': 'USER_AUTH_TOKEN',
+            'os_project_name': 'admin',
+            'interface': 'internal'
+        }
+        expected_kwargs = {
+            'auth_url': 'http://localhost:35357/v3',
+            'project_name': 'admin',
+            'token': 'USER_AUTH_TOKEN'
+        }
+        self._test_loader_arguments_passed_correctly(
+            passed_kwargs=passed_kwargs, expected_kwargs=expected_kwargs,
+            loader_class=identity.Token, expected_interface='internal'
+        )
+
     def test_loader_arguments_password_tenant_name(self):
         passed_kwargs = {
             'os_auth_url': 'http://localhost:35357/v3',
@@ -344,7 +397,8 @@ class ClientTest(utils.BaseTestCase):
                     'max_retries': None,
                     'retry_interval': None,
                     'session': session,
-                    'endpoint_override': 'http://ironic.example.org:6385/'}
+                    'endpoint_override': 'http://ironic.example.org:6385/',
+                    'interface': None}
         )
         self.assertFalse(session.get_endpoint.called)
 
@@ -368,7 +422,8 @@ class ClientTest(utils.BaseTestCase):
                     'max_retries': None,
                     'retry_interval': None,
                     'session': session,
-                    'endpoint_override': session.get_endpoint.return_value}
+                    'endpoint_override': session.get_endpoint.return_value,
+                    'interface': None}
         )
 
     @mock.patch.object(iroclient, 'Client', autospec=True)
@@ -385,7 +440,8 @@ class ClientTest(utils.BaseTestCase):
                     'max_retries': None,
                     'retry_interval': None,
                     'session': session,
-                    'endpoint_override': session.get_endpoint.return_value}
+                    'endpoint_override': session.get_endpoint.return_value,
+                    'interface': None}
         )
         self.assertFalse(mock_ks_session.called)
 
diff --git a/releasenotes/notes/pass-interface-argument-deb92e3feb0bf051.yaml b/releasenotes/notes/pass-interface-argument-deb92e3feb0bf051.yaml
new file mode 100644
index 0000000..afebe7e
--- /dev/null
+++ b/releasenotes/notes/pass-interface-argument-deb92e3feb0bf051.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    The ``interface`` argument was being ignored so that the HTTP client was
+    always using the public endpoint for Ironic. This fixes it so that the
+    ``interface`` argument is taken into consideration. See
+    `story 2005118 <https://storyboard.openstack.org/#!/story/2005118>`_.