From d4cea970c2d56ef8ff813473cd5ba0e38e3f04fd Mon Sep 17 00:00:00 2001 From: Guang Yee Date: Mon, 4 Mar 2019 13:30:09 -0800 Subject: [PATCH] pass endpoint interface to Ironic client Via change [1], ironicclient began to use endpoint_filter in the version negotiation code path, whereas it was previously unused if a fully-qualified endpoint had already been determined. Suddenly it was important that the `interface` part of this endpoint_filter be correct. Prior to ironicclient change [2], there was no way to pass an appropriate `interface` value through ironicclient's initialization, so the ironicclient used from nova would always end up with the default value, `public`, in the endpoint_filter. This would break in clouds lacking a public ironic API endpoint (see the referenced bug). With this change, we pass the value of the (standard, per ksa) `valid_interfaces` ironic config option into the ironicclient initialization, where (if and only if the ironicclient fix [2] is also present) it eventually gets passed through to the ksa Adapter initialization (which is set up to accept values from exactly that conf option) to wind up in the endpoint_filter. The effect is that nova's ironicclient will actually be using the interface from nova.conf throughout. (Because `valid_interfaces` is also used in recommended configuration setups - i.e. those that use the service catalog to determine API endpoints - to construct the endpoint_override used to initialize the ironicclient, the value used during version negotiation should be in sync with that used for regular API calls.) [1] I42b66daea1f4397273a3f4eb1638abafb3bb28ce [2] I610836e5038774621690aca88b2aee25670f0262 Change-Id: I5f78d21c39ed2fd58d2a0f3649116e39883d5a2c closes-bug: 1818295 (cherry picked from commit e082bdc166cb8215576801e0c89ef1fe771681ed) --- .../unit/virt/ironic/test_client_wrapper.py | 29 +++++++++++++++++-- nova/virt/ironic/client_wrapper.py | 6 ++++ ...ace-for-ironicclient-a0b6b8f8dedc7341.yaml | 9 ++++++ 3 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/set-endpoint-interface-for-ironicclient-a0b6b8f8dedc7341.yaml diff --git a/nova/tests/unit/virt/ironic/test_client_wrapper.py b/nova/tests/unit/virt/ironic/test_client_wrapper.py index a726530c39ee..910f137366d4 100644 --- a/nova/tests/unit/virt/ironic/test_client_wrapper.py +++ b/nova/tests/unit/virt/ironic/test_client_wrapper.py @@ -88,7 +88,8 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): 'retry_interval': CONF.ironic.api_retry_interval, 'os_ironic_api_version': ['1.38', '1.37'], 'ironic_url': - self.get_ksa_adapter.return_value.get_endpoint.return_value} + self.get_ksa_adapter.return_value.get_endpoint.return_value, + 'interface': ['internal', 'public']} mock_ir_cli.assert_called_once_with(1, **expected) @mock.patch.object(keystoneauth1.session, 'Session') @@ -113,7 +114,8 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): 'max_retries': CONF.ironic.api_max_retries, 'retry_interval': CONF.ironic.api_retry_interval, 'os_ironic_api_version': ['1.38', '1.37'], - 'ironic_url': None} + 'ironic_url': None, + 'interface': ['internal', 'public']} mock_ir_cli.assert_called_once_with(1, **expected) @mock.patch.object(keystoneauth1.session, 'Session') @@ -131,7 +133,28 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): 'max_retries': CONF.ironic.api_max_retries, 'retry_interval': CONF.ironic.api_retry_interval, 'os_ironic_api_version': ['1.38', '1.37'], - 'ironic_url': endpoint} + 'ironic_url': endpoint, + 'interface': ['internal', 'public']} + mock_ir_cli.assert_called_once_with(1, **expected) + + @mock.patch.object(keystoneauth1.session, 'Session') + @mock.patch.object(ironic_client, 'get_client') + def test__get_client_and_valid_interfaces(self, mock_ir_cli, mock_session): + """Endpoint discovery via legacy api_endpoint conf option.""" + mock_session.return_value = 'session' + endpoint = 'https://baremetal.example.com/endpoint' + self.flags(api_endpoint=endpoint, group='ironic') + self.flags(valid_interfaces='admin', group='ironic') + ironicclient = client_wrapper.IronicClientWrapper() + # dummy call to have _get_client() called + ironicclient.call("node.list") + self.get_ksa_adapter.assert_not_called() + expected = {'session': 'session', + 'max_retries': CONF.ironic.api_max_retries, + 'retry_interval': CONF.ironic.api_retry_interval, + 'os_ironic_api_version': ['1.38', '1.37'], + 'ironic_url': endpoint, + 'interface': ['admin']} mock_ir_cli.assert_called_once_with(1, **expected) @mock.patch.object(client_wrapper.IronicClientWrapper, '_multi_getattr') diff --git a/nova/virt/ironic/client_wrapper.py b/nova/virt/ironic/client_wrapper.py index ca1ebf888ec4..2bdec27774e4 100644 --- a/nova/virt/ironic/client_wrapper.py +++ b/nova/virt/ironic/client_wrapper.py @@ -99,6 +99,12 @@ class IronicClientWrapper(object): kwargs['os_ironic_api_version'] = [ '%d.%d' % IRONIC_API_VERSION, '%d.%d' % PRIOR_IRONIC_API_VERSION] + ironic_conf = CONF[IRONIC_GROUP.name] + # valid_interfaces is a list. ironicclient passes this kwarg through to + # ksa, which is set up to handle 'interface' as either a list or a + # single value. + kwargs['interface'] = ironic_conf.valid_interfaces + # NOTE(clenimar/efried): by default, the endpoint is taken from the # service catalog. Use `endpoint_override` if you want to override it. if CONF.ironic.api_endpoint: diff --git a/releasenotes/notes/set-endpoint-interface-for-ironicclient-a0b6b8f8dedc7341.yaml b/releasenotes/notes/set-endpoint-interface-for-ironicclient-a0b6b8f8dedc7341.yaml new file mode 100644 index 000000000000..00c5b3f5e38e --- /dev/null +++ b/releasenotes/notes/set-endpoint-interface-for-ironicclient-a0b6b8f8dedc7341.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + [`bug 1818295 `_] + Fixes the problem with endpoint lookup in Ironic driver where only public + endpoint is possible, which breaks deployments where the controllers have + no route to the public network per security requirement. Note that + python-ironicclient fix I610836e5038774621690aca88b2aee25670f0262 must + also be present to resolve the bug.