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

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.46', '1.38'],
             'endpoint':
-                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.46', '1.38'],
-                    'endpoint': None}
+                    'endpoint': 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.46', '1.38'],
-                    'endpoint': endpoint}
+                    'endpoint': 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.46', '1.38'],
+                    'endpoint': endpoint,
+                    'interface': ['admin']}
         mock_ir_cli.assert_called_once_with(1, **expected)
 
     @mock.patch.object(client_wrapper.IronicClientWrapper, '_multi_getattr')

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:

releasenotes/notes/set-endpoint-interface-for-ironicclient-a0b6b8f8dedc7341.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    [`bug 1818295 <https://bugs.launchpad.net/nova/+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.