From 308e414a57941675acf6cd825daafc653cf5927e Mon Sep 17 00:00:00 2001 From: Pavlo Shchelokovskyy Date: Fri, 23 Jun 2017 14:17:26 +0000 Subject: [PATCH] Introduce keystoneauth adapters for clients Currently ironic explicitly or implicitly sets the API urls for most services in the config. This is quite fragile and we should move to discovery from the keystone catalog eventually. To support this, this patch registers `keystoneauth1.adapter.Adapter` options to all config sections for service clients auth. Among others it exports `interfaces` option that we set to ['internal', 'public'] by default. Other exported options are `service_type`, `service_name`, `region_name` and `endpoint_override`. The latter will eventually be used by all clients to specify a specific endpoint to use (for example in noauth mode). Effectively this patch starts to move all clients code to load client configuration from config for all of auth, session and adapter. The first to move is [service_catalog] section, with [conductor]api_url option being deprecated in favor of [service_catalog]endpoint_override. A sane default of 'service_type' = 'baremetal' is set for this config section as well. More patches moving other clients to consume these new options and deprecate some other options will follow. Change-Id: I1283ef3b4d736ac089df0cc74a5850a93b24b6ab Partial-Bug: #1699547 Related-Bug: #1699542 --- devstack/lib/ironic | 43 +++++++++++----- etc/ironic/ironic.conf.sample | 50 +++++++++++++++++-- ironic/common/keystone.py | 24 +++++++-- ironic/conf/auth.py | 19 ++++++- ironic/conf/conductor.py | 5 ++ ironic/conf/service_catalog.py | 5 +- ironic/drivers/modules/deploy_utils.py | 38 ++++++++------ ironic/tests/unit/common/test_keystone.py | 27 +++++----- .../unit/drivers/modules/test_deploy_utils.py | 30 ++++++----- .../unit/drivers/modules/test_iscsi_deploy.py | 4 +- ...oneauth-adapter-opts-ca4f68f568e6cf6f.yaml | 41 +++++++++++++++ 11 files changed, 217 insertions(+), 69 deletions(-) create mode 100644 releasenotes/notes/keystoneauth-adapter-opts-ca4f68f568e6cf6f.yaml diff --git a/devstack/lib/ironic b/devstack/lib/ironic index 9f7a57d94a..922c7fdc67 100644 --- a/devstack/lib/ironic +++ b/devstack/lib/ironic @@ -1058,9 +1058,13 @@ function configure_ironic_api { cp -p $IRONIC_DIR/etc/ironic/policy.json $IRONIC_POLICY_JSON } -function configure_auth_for { +# configure_client_for() - is used by configure_ironic_conductor. +# Sets options to instantiate clients for other services +# single argument - config section to fill +function configure_client_for { local service_config_section service_config_section=$1 + # keystoneauth auth plugin options iniset $IRONIC_CONF_FILE $service_config_section auth_type password iniset $IRONIC_CONF_FILE $service_config_section auth_url $KEYSTONE_SERVICE_URI iniset $IRONIC_CONF_FILE $service_config_section username ironic @@ -1068,24 +1072,39 @@ function configure_auth_for { iniset $IRONIC_CONF_FILE $service_config_section project_name $SERVICE_PROJECT_NAME iniset $IRONIC_CONF_FILE $service_config_section user_domain_id default iniset $IRONIC_CONF_FILE $service_config_section project_domain_id default + # keystoneauth session options iniset $IRONIC_CONF_FILE $service_config_section cafile $SSL_BUNDLE_FILE } +# TODO(pas-ha) this function is for transition period only, +# after all clients are moved to use keystoneauth adapters, it will be merged +# into configure_client_for function +function configure_adapter_for { + local service_config_section + service_config_section=$1 + # keystoneauth adapter options + # NOTE(pas-ha) relying on defaults for valid_interfaces being "internal,public" in ironic + iniset $IRONIC_CONF_FILE $service_config_section region_name $REGION_NAME +} + # configure_ironic_conductor() - Is used by configure_ironic(). # Sets conductor specific settings. function configure_ironic_conductor { - # set keystone region for all services - iniset $IRONIC_CONF_FILE keystone region_name $REGION_NAME + # NOTE(pas-ha) service_catalog section is used to discover + # ironic API endpoint from keystone catalog + local client_sections="neutron swift glance inspector cinder service_catalog" + for conf_section in $client_sections; do + configure_client_for $conf_section + done - # set keystone auth plugin options for services - configure_auth_for neutron - configure_auth_for swift - configure_auth_for glance - configure_auth_for inspector - configure_auth_for cinder - # this one is needed for lookup of Ironic API endpoint via Keystone - configure_auth_for service_catalog + # TODO(pas-ha) this block is for transition period only, + # after all clients are moved to use keystoneauth adapters, + # it will be deleted + local sections_with_adapter="service_catalog" + for conf_section in $sections_with_adapter; do + configure_adapter_for $conf_section + done cp $IRONIC_DIR/etc/ironic/rootwrap.conf $IRONIC_ROOTWRAP_CONF cp -r $IRONIC_DIR/etc/ironic/rootwrap.d $IRONIC_CONF_DIR @@ -1239,8 +1258,6 @@ function create_ironic_accounts { get_or_create_service "ironic" "baremetal" "Ironic baremetal provisioning service" get_or_create_endpoint "baremetal" \ "$REGION_NAME" \ - "$IRONIC_SERVICE_PROTOCOL://$IRONIC_HOSTPORT" \ - "$IRONIC_SERVICE_PROTOCOL://$IRONIC_HOSTPORT" \ "$IRONIC_SERVICE_PROTOCOL://$IRONIC_HOSTPORT" # Create ironic service user diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index 5bfcff9ed5..49fcda310a 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -1026,10 +1026,15 @@ # Seconds between conductor heart beats. (integer value) #heartbeat_interval = 10 -# URL of Ironic API service. If not set ironic can get the -# current value from the keystone service catalog. If set, the -# value must start with either http:// or https://. (uri -# value) +# DEPRECATED: URL of Ironic API service. If not set ironic can +# get the current value from the keystone service catalog. If +# set, the value must start with either http:// or https://. +# (uri value) +# This option is deprecated for removal. +# Its value may be silently ignored in the future. +# Reason: Use [service_catalog]endpoint_override option +# instead if required to use a specific ironic api address, +# for example in noauth mode. #api_url = # Maximum time (in seconds) since the last check-in of a @@ -3545,12 +3550,28 @@ # Domain name to scope to (string value) #domain_name = +# Always use this endpoint URL for requests for this client. +# (string value) +#endpoint_override = + # Verify HTTPS connections. (boolean value) #insecure = false # PEM encoded client certificate key file (string value) #keyfile = +# The maximum major version of a given API, intended to be +# used as the upper bound of a range with min_version. +# Mutually exclusive with version. (string value) +#max_version = + +# The minimum major version of a given API, intended to be +# used as the lower bound of a range with max_version. +# Mutually exclusive with version. If min_version is given +# with no max_version it is as if max version is "latest". +# (string value) +#min_version = + # User's password (string value) #password = @@ -3568,6 +3589,18 @@ # Deprecated group/name - [service_catalog]/tenant_name #project_name = +# The default region_name for endpoint URL discovery. (string +# value) +#region_name = + +# The default service_name for endpoint URL discovery. (string +# value) +#service_name = + +# The default service_type for endpoint URL discovery. (string +# value) +#service_type = baremetal + # Tenant ID (string value) #tenant_id = @@ -3593,6 +3626,15 @@ # Deprecated group/name - [service_catalog]/user_name #username = +# List of interfaces, in order of preference, for endpoint +# URL. (list value) +#valid_interfaces = internal,public + +# Minimum Major API version within a given Major API version +# for endpoint URL discovery. Mutually exclusive with +# min_version and max_version (string value) +#version = + [snmp] diff --git a/ironic/common/keystone.py b/ironic/common/keystone.py index 706ac45157..bc97ac35ef 100644 --- a/ironic/common/keystone.py +++ b/ironic/common/keystone.py @@ -20,6 +20,7 @@ from oslo_log import log as logging import six from ironic.common import exception +from ironic.conf import auth as auth_conf from ironic.conf import CONF @@ -86,7 +87,22 @@ def get_auth(group, **auth_kwargs): return auth -# NOTE(pas-ha) Used by neutronclient and resolving ironic API only +@ks_exceptions +def get_adapter(group, **adapter_kwargs): + """Loads adapter from options in a configuration file section. + + The adapter_kwargs will be passed directly to keystoneauth1 Adapter + and will override the values loaded from config. + Consult keystoneauth1 docs for available adapter options. + + :param group: name of the config section to load adapter options from + + """ + return kaloading.load_adapter_from_conf_options(CONF, group, + **adapter_kwargs) + + +# NOTE(pas-ha) Used by neutronclient and glanceclient only # FIXME(pas-ha) remove this while moving to kesytoneauth adapters @ks_exceptions def get_service_url(session, **kwargs): @@ -103,7 +119,5 @@ def get_service_url(session, **kwargs): if 'interface' in kwargs: return session.get_endpoint(**kwargs) - try: - return session.get_endpoint(interface='internal', **kwargs) - except kaexception.EndpointNotFound: - return session.get_endpoint(interface='public', **kwargs) + return session.get_endpoint(interface=auth_conf.DEFAULT_VALID_INTERFACES, + **kwargs) diff --git a/ironic/conf/auth.py b/ironic/conf/auth.py index 08956460b4..35b3f492c4 100644 --- a/ironic/conf/auth.py +++ b/ironic/conf/auth.py @@ -15,13 +15,16 @@ import copy from keystoneauth1 import loading as kaloading +from oslo_config import cfg from oslo_log import log LOG = log.getLogger(__name__) +DEFAULT_VALID_INTERFACES = ['internal', 'public'] -def register_auth_opts(conf, group): + +def register_auth_opts(conf, group, service_type=None): """Register session- and auth-related options Registers only basic auth options shared by all auth plugins. @@ -29,9 +32,14 @@ def register_auth_opts(conf, group): """ kaloading.register_session_conf_options(conf, group) kaloading.register_auth_conf_options(conf, group) + if service_type: + kaloading.register_adapter_conf_options(conf, group) + conf.set_default('valid_interfaces', DEFAULT_VALID_INTERFACES, + group=group) + conf.set_default('service_type', service_type, group=group) -def add_auth_opts(options): +def add_auth_opts(options, service_type=None): """Add auth options to sample config As these are dynamically registered at runtime, @@ -55,5 +63,12 @@ def add_auth_opts(options): plugin = kaloading.get_plugin_loader(name) add_options(opts, kaloading.get_auth_plugin_conf_options(plugin)) add_options(opts, kaloading.get_session_conf_options()) + if service_type: + adapter_opts = kaloading.get_adapter_conf_options( + include_deprecated=False) + # adding defaults for valid interfaces + cfg.set_defaults(adapter_opts, service_type=service_type, + valid_interfaces=DEFAULT_VALID_INTERFACES) + add_options(opts, adapter_opts) opts.sort(key=lambda x: x.name) return opts diff --git a/ironic/conf/conductor.py b/ironic/conf/conductor.py index e8d2d73b20..5cdbf72864 100644 --- a/ironic/conf/conductor.py +++ b/ironic/conf/conductor.py @@ -30,6 +30,11 @@ opts = [ help=_('Seconds between conductor heart beats.')), cfg.URIOpt('api_url', schemes=('http', 'https'), + deprecated_for_removal=True, + deprecated_reason=_("Use [service_catalog]endpoint_override " + "option instead if required to use " + "a specific ironic api address, " + "for example in noauth mode."), help=_('URL of Ironic API service. If not set ironic can ' 'get the current value from the keystone service ' 'catalog. If set, the value must start with either ' diff --git a/ironic/conf/service_catalog.py b/ironic/conf/service_catalog.py index 56b9d0a415..7bc89351cf 100644 --- a/ironic/conf/service_catalog.py +++ b/ironic/conf/service_catalog.py @@ -26,8 +26,9 @@ SERVICE_CATALOG_GROUP = cfg.OptGroup( def register_opts(conf): - auth.register_auth_opts(conf, SERVICE_CATALOG_GROUP.name) + auth.register_auth_opts(conf, SERVICE_CATALOG_GROUP.name, + service_type='baremetal') def list_opts(): - return auth.add_auth_opts([]) + return auth.add_auth_opts([], service_type='baremetal') diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 81336871d9..cdd52d567d 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -77,9 +77,7 @@ _IRONIC_SESSION = None def _get_ironic_session(): global _IRONIC_SESSION if not _IRONIC_SESSION: - auth = keystone.get_auth('service_catalog') - _IRONIC_SESSION = keystone.get_session('service_catalog', - auth=auth) + _IRONIC_SESSION = keystone.get_session('service_catalog') return _IRONIC_SESSION @@ -94,18 +92,28 @@ def get_ironic_api_url(): either from config of from Keystone catalog. """ - ironic_api = CONF.conductor.api_url - if not ironic_api: - try: - ironic_session = _get_ironic_session() - ironic_api = keystone.get_service_url(ironic_session) - except (exception.KeystoneFailure, - exception.CatalogNotFound, - exception.KeystoneUnauthorized) as e: - raise exception.InvalidParameterValue(_( - "Couldn't get the URL of the Ironic API service from the " - "configuration file or keystone catalog. Keystone error: " - "%s") % six.text_type(e)) + adapter_opts = {'session': _get_ironic_session()} + # NOTE(pas-ha) force 'none' auth plugin for noauth mode + if CONF.auth_strategy != 'keystone': + CONF.set_override('auth_type', 'none', group='service_catalog') + adapter_opts['auth'] = keystone.get_auth('service_catalog') + + # TODO(pas-ha) remove in Rocky + # NOTE(pas-ha) if both set, the new options win + if CONF.conductor.api_url and not CONF.service_catalog.endpoint_override: + adapter_opts['endpoint_override'] = CONF.conductor.api_url + if CONF.keystone.region_name and not CONF.service_catalog.region_name: + adapter_opts['region_name'] = CONF.keystone.region_name + adapter = keystone.get_adapter('service_catalog', **adapter_opts) + try: + ironic_api = adapter.get_endpoint() + except (exception.KeystoneFailure, + exception.CatalogNotFound, + exception.KeystoneUnauthorized) as e: + raise exception.InvalidParameterValue(_( + "Couldn't get the URL of the Ironic API service from the " + "configuration file or keystone catalog. Keystone error: " + "%s") % six.text_type(e)) # NOTE: we should strip '/' from the end because it might be used in # hardcoded ramdisk script ironic_api = ironic_api.rstrip('/') diff --git a/ironic/tests/unit/common/test_keystone.py b/ironic/tests/unit/common/test_keystone.py index 1b1a420706..31c85baad0 100644 --- a/ironic/tests/unit/common/test_keystone.py +++ b/ironic/tests/unit/common/test_keystone.py @@ -12,7 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -from keystoneauth1 import exceptions as ksexception from keystoneauth1 import loading as kaloading import mock from oslo_config import cfg @@ -32,7 +31,8 @@ class KeystoneTestCase(base.TestCase): group='keystone') self.test_group = 'test_group' self.cfg_fixture.conf.register_group(cfg.OptGroup(self.test_group)) - ironic_auth.register_auth_opts(self.cfg_fixture.conf, self.test_group) + ironic_auth.register_auth_opts(self.cfg_fixture.conf, self.test_group, + service_type='vikings') self.config(auth_type='password', group=self.test_group) # NOTE(pas-ha) this is due to auth_plugin options @@ -76,20 +76,19 @@ class KeystoneTestCase(base.TestCase): self.assertEqual('spam', keystone.get_service_url(session, **params)) session.get_endpoint.assert_called_once_with(**params) - def test_get_service_url_internal(self): + def test_get_service_url(self): session = mock.Mock() session.get_endpoint.return_value = 'spam' params = {'ham': 'eggs'} self.assertEqual('spam', keystone.get_service_url(session, **params)) - session.get_endpoint.assert_called_once_with(interface='internal', - **params) + session.get_endpoint.assert_called_once_with( + interface=['internal', 'public'], **params) - def test_get_service_url_internal_fail(self): - session = mock.Mock() - session.get_endpoint.side_effect = [ksexception.EndpointNotFound(), - 'spam'] - params = {'ham': 'eggs'} - self.assertEqual('spam', keystone.get_service_url(session, **params)) - session.get_endpoint.assert_has_calls([ - mock.call(interface='internal', **params), - mock.call(interface='public', **params)]) + def test_get_adapter_from_config(self): + self.config(valid_interfaces=['internal', 'public'], + group=self.test_group) + session = keystone.get_session(self.test_group) + adapter = keystone.get_adapter(self.test_group, session=session, + interface='admin') + self.assertEqual('admin', adapter.interface) + self.assertEqual(session, adapter.session) diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 68930eae79..b35abd65b5 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -1214,38 +1214,42 @@ class OtherFunctionTestCase(db_base.DbTestCase): mock_clean_up_caches.assert_called_once_with(None, 'master_dir', [('uuid', 'path')]) + @mock.patch('ironic.common.keystone.get_auth') @mock.patch.object(utils, '_get_ironic_session') - @mock.patch('ironic.common.keystone.get_service_url') - def test_get_ironic_api_url_from_config(self, mock_get_url, mock_ks): + def test_get_ironic_api_url_from_config(self, mock_ks, mock_auth): mock_sess = mock.Mock() mock_ks.return_value = mock_sess fake_api_url = 'http://foo/' - mock_get_url.side_effect = exception.KeystoneFailure self.config(api_url=fake_api_url, group='conductor') - url = utils.get_ironic_api_url() # also checking for stripped trailing slash - self.assertEqual(fake_api_url[:-1], url) - self.assertFalse(mock_get_url.called) + self.assertEqual(fake_api_url[:-1], utils.get_ironic_api_url()) + @mock.patch('ironic.common.keystone.get_auth') @mock.patch.object(utils, '_get_ironic_session') - @mock.patch('ironic.common.keystone.get_service_url') - def test_get_ironic_api_url_from_keystone(self, mock_get_url, mock_ks): + @mock.patch('ironic.common.keystone.get_adapter') + def test_get_ironic_api_url_from_keystone(self, mock_ka, mock_ks, + mock_auth): mock_sess = mock.Mock() mock_ks.return_value = mock_sess fake_api_url = 'http://foo/' - mock_get_url.return_value = fake_api_url + mock_ka.return_value.get_endpoint.return_value = fake_api_url + # NOTE(pas-ha) endpoint_override is None by default self.config(api_url=None, group='conductor') url = utils.get_ironic_api_url() # also checking for stripped trailing slash self.assertEqual(fake_api_url[:-1], url) - mock_get_url.assert_called_with(mock_sess) + mock_ka.assert_called_with('service_catalog', session=mock_sess, + auth=mock_auth.return_value) + mock_ka.return_value.get_endpoint.assert_called_once_with() + @mock.patch('ironic.common.keystone.get_auth') @mock.patch.object(utils, '_get_ironic_session') - @mock.patch('ironic.common.keystone.get_service_url') - def test_get_ironic_api_url_fail(self, mock_get_url, mock_ks): + @mock.patch('ironic.common.keystone.get_adapter') + def test_get_ironic_api_url_fail(self, mock_ka, mock_ks, mock_auth): mock_sess = mock.Mock() mock_ks.return_value = mock_sess - mock_get_url.side_effect = exception.KeystoneFailure() + mock_ka.return_value.get_endpoint.side_effect = ( + exception.KeystoneFailure()) self.config(api_url=None, group='conductor') self.assertRaises(exception.InvalidParameterValue, utils.get_ironic_api_url) diff --git a/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py b/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py index b0a179b5a0..7dd4debc7f 100644 --- a/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py +++ b/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py @@ -521,7 +521,9 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase): iscsi_deploy.validate, task) mock_get_url.assert_called_once_with() - def test_validate_invalid_root_device_hints(self): + @mock.patch('ironic.drivers.modules.deploy_utils.get_ironic_api_url') + def test_validate_invalid_root_device_hints(self, mock_get_url): + mock_get_url.return_value = 'http://spam.ham/baremetal' with task_manager.acquire(self.context, self.node.uuid, shared=True) as task: task.node.properties['root_device'] = {'size': 'not-int'} diff --git a/releasenotes/notes/keystoneauth-adapter-opts-ca4f68f568e6cf6f.yaml b/releasenotes/notes/keystoneauth-adapter-opts-ca4f68f568e6cf6f.yaml new file mode 100644 index 0000000000..d2cde3f23c --- /dev/null +++ b/releasenotes/notes/keystoneauth-adapter-opts-ca4f68f568e6cf6f.yaml @@ -0,0 +1,41 @@ +--- +upgrade: + - | + To facilitate automatic discovery of services from the keystone catalog, + the configuration file sections for service clients may include these + configuration options: ``service_type``, ``service_name``, + ``valid_interfaces``, ``region_name`` and other keystoneauth Adaper-related + options. + + These options together must uniquely specify an endpoint for a service + registered in the keystone catalog. + Consult the ``keystoneauth`` library documentation for full list of + available options, their meaning and possible values. + + Default values for ``service_type`` are set by ironic to sane defaults + based on required services and their entries in ``service-types-authority``. + + The ``valid_interfaces`` option defaults to ``['internal', 'public']``. + + The ``region_name`` option defaults to ``None`` and must be explicitly set + for multi-regional setup for endpoint discovery to succeed. + + The configuration file sections where these new options are available are: + + - service_catalog + +features: + - | + Options for service endpoint discovery can now be set per-service in + appropriate service configuration file sections: + + - ``[service_catalog]`` for baremetal API discovery + +deprecations: + - | + Configuration option ``[conductor]api_url`` is deprecated + and will be ignored in the Rocky release. + Instead, use ``[service_catalog]endpoint_override`` configuration option + to set specific ironic API address if automatic discovery of ironic API + endpoint from keystone catalog is not desired. + This new option defaults to ``None`` and must be set explicitly if needed.