From d6eea403cbf0d11b06acbecc704f422a7e278462 Mon Sep 17 00:00:00 2001 From: Guang Yee Date: Fri, 1 Mar 2019 10:57:04 -0800 Subject: [PATCH] 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 --- ironicclient/client.py | 13 +++- ironicclient/tests/unit/test_client.py | 74 ++++++++++++++++--- ...s-interface-argument-deb92e3feb0bf051.yaml | 7 ++ 3 files changed, 83 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/pass-interface-argument-deb92e3feb0bf051.yaml diff --git a/ironicclient/client.py b/ironicclient/client.py index b785fafa3..83b0e148a 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 072ce3738..7d10701f4 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 000000000..afebe7eba --- /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 `_.