Support Proxy-specific region_name

Proxy (a subclass of keystoneauth1 Adapter) is set up to know about
region_name. This is done so that consumers of cloud_region.from_conf
will have their [$project]region_name setting honored rather than
ignored.

Previously, CloudRegion was set up to deal with a single region, masking
(or rather overriding) each Proxy's notion of what its region_name
should be.

With this change, (service-specific) region_nameZ are honored by the
CloudRegion constructor, as follows:

- A service-specific region_name in the ``config`` dict (e.g.
  'compute_region_name': 'SnowflakeComputeRegion') takes first
  precedence for that service.
- If no service-specific region_name is present in the ``config`` dict,
  the value of the region_name kwarg to the CloudRegion constructor is
  used. (This kwarg should really go away, but is preserved for backward
  compatibility.)
- If neither of the above is given, the value of the (unprefixed)
  'region_name' key in the ``config`` dict is used. (This should really
  take precedence over the kwarg, but it's done this way for backward
  compatibility.)
- If none of the above exist, None is used (the previous behavior,
  whatever that may mean).

This change should hopefully be transparent to any existing usages of
the CloudRegion constructor, as ${service}_region_name would have been
ignored if present. However, if any existing usages had such a config
setting present, it will no longer be ignored, resulting in potential
RBB [1].

[1] That's "Revoked Bug Benefit"

Change-Id: Ie253823a753d09d52e45df9d515fd22870c2d4c5

openstack/cloud/_compute.py

@@ -39,6 +39,11 @@ class ComputeCloudMixin(_normalize.Normalizer):
         self._servers_time = 0
         self._servers_lock = threading.Lock()
 
+    @property
+    def _compute_region(self):
+        # This is only used in exception messages. Can we get rid of it?
+        return self.config.get_region_name('compute')
+
     def get_flavor_name(self, flavor_id):
         flavor = self.get_flavor(flavor_id, get_extra=False)
         if flavor:
@@ -896,7 +901,7 @@ class ComputeCloudMixin(_normalize.Normalizer):
                         'Network {network} is not a valid network in'
                         ' {cloud}:{region}'.format(
                             network=network,
-                            cloud=self.name, region=self.config.region_name))
+                            cloud=self.name, region=self._compute_region))
                 nics.append({'net-id': network_obj['id']})
 
             kwargs['nics'] = nics
@@ -1025,7 +1030,7 @@ class ComputeCloudMixin(_normalize.Normalizer):
                     'Volume {boot_volume} is not a valid volume'
                     ' in {cloud}:{region}'.format(
                         boot_volume=boot_volume,
-                        cloud=self.name, region=self.config.region_name))
+                        cloud=self.name, region=self._compute_region))
             block_mapping = {
                 'boot_index': '0',
                 'delete_on_termination': terminate_volume,
@@ -1046,7 +1051,7 @@ class ComputeCloudMixin(_normalize.Normalizer):
                     'Image {image} is not a valid image in'
                     ' {cloud}:{region}'.format(
                         image=image,
-                        cloud=self.name, region=self.config.region_name))
+                        cloud=self.name, region=self._compute_region))
 
             block_mapping = {
                 'boot_index': '0',
@@ -1076,7 +1081,7 @@ class ComputeCloudMixin(_normalize.Normalizer):
                     'Volume {volume} is not a valid volume'
                     ' in {cloud}:{region}'.format(
                         volume=volume,
-                        cloud=self.name, region=self.config.region_name))
+                        cloud=self.name, region=self._compute_region))
             block_mapping = {
                 'boot_index': '-1',
                 'delete_on_termination': False,

openstack/cloud/_normalize.py

@@ -563,7 +563,10 @@ class Normalizer(object):
             ret['config_drive'] = config_drive
             ret['project_id'] = project_id
             ret['tenant_id'] = project_id
-            ret['region'] = self.config.region_name
+            # TODO(efried): This is hardcoded to 'compute' because this method
+            # should only ever be used by the compute proxy. (That said, it
+            # doesn't appear to be used at all, so can we get rid of it?)
+            ret['region'] = self.config.get_region_name('compute')
             ret['cloud'] = self.config.name
             ret['az'] = az
             for key, val in ret['properties'].items():

openstack/cloud/meta.py

@@ -340,7 +340,9 @@ def _get_interface_ip(cloud, server):
 def get_groups_from_server(cloud, server, server_vars):
     groups = []
 
-    region = cloud.config.region_name
+    # NOTE(efried): This is hardcoded to 'compute' because this method is only
+    # used from ComputeCloudMixin.
+    region = cloud.config.get_region_name('compute')
     cloud_name = cloud.name
 
     # Create a group for the cloud

openstack/cloud/openstackcloud.py

@@ -358,7 +358,7 @@ class _OpenStackCloudMixin(object):
                 service_name=self.config.get_service_name(service_type),
                 interface=self.config.get_interface(service_type),
                 endpoint_override=self.config.get_endpoint(service_type),
-                region_name=self.config.region_name,
+                region_name=self.config.get_region_name(service_type),
                 statsd_prefix=self.config.get_statsd_prefix(),
                 statsd_client=self.config.get_statsd_client(),
                 prometheus_counter=self.config.get_prometheus_counter(),
@@ -374,7 +374,7 @@ class _OpenStackCloudMixin(object):
             service_name=self.config.get_service_name(service_type),
             interface=self.config.get_interface(service_type),
             endpoint_override=self.config.get_endpoint(service_type),
-            region_name=self.config.region_name,
+            region_name=self.config.get_region_name(service_type),
             min_version=min_version,
             max_version=max_version)
 
@@ -413,7 +413,7 @@ class _OpenStackCloudMixin(object):
             interface=self.config.get_interface(service_type),
             endpoint_override=self.config.get_endpoint(
                 service_type) or endpoint_override,
-            region_name=self.config.region_name)
+            region_name=self.config.get_region_name(service_type))
 
     def _is_client_version(self, client, version):
         client_name = '_{client}_client'.format(client=client)
@@ -529,7 +529,9 @@ class _OpenStackCloudMixin(object):
     def _get_current_location(self, project_id=None, zone=None):
         return munch.Munch(
             cloud=self.name,
-            region_name=self.config.region_name,
+            # TODO(efried): This is wrong, but it only seems to be used in a
+            # repr; can we get rid of it?
+            region_name=self.config.get_region_name(),
             zone=zone,
             project=self._get_project_info(project_id),
         )
@@ -649,7 +651,8 @@ class _OpenStackCloudMixin(object):
         return self.name
 
     def get_region(self):
-        return self.config.region_name
+        # TODO(efried): This seems to be unused. Can we get rid of it?
+        return self.config.get_region_name()
 
     def get_session_endpoint(self, service_key):
         try:
@@ -666,7 +669,7 @@ class _OpenStackCloudMixin(object):
                 " {error}".format(
                     service=service_key,
                     cloud=self.name,
-                    region=self.config.region_name,
+                    region=self.config.get_region_name(service_key),
                     error=str(e)))
         return endpoint
 

openstack/config/cloud_region.py

@@ -127,8 +127,6 @@ def from_conf(conf, session=None, **kwargs):
         raise exceptions.ConfigException("A Session must be supplied.")
     config_dict = kwargs.pop('config', config_defaults.get_defaults())
     stm = os_service_types.ServiceTypes()
-    # TODO(mordred) Think about region_name here
-    region_name = kwargs.pop('region_name', None)
     for st in stm.all_types_by_service_type:
         project_name = stm.get_project_name(st)
         if project_name not in conf:
@@ -143,14 +141,10 @@ def from_conf(conf, session=None, **kwargs):
             continue
         # Load them into config_dict under keys prefixed by ${service_type}_
         for raw_name, opt_val in opt_dict.items():
-            if raw_name == 'region_name':
-                region_name = opt_val
-                continue
             config_name = '_'.join([st, raw_name])
             config_dict[config_name] = opt_val
     return CloudRegion(
-        session=session, region_name=region_name, config=config_dict,
+        session=session, config=config_dict, **kwargs)
-        **kwargs)
 
 
 class CloudRegion(object):
@@ -158,6 +152,28 @@ class CloudRegion(object):
 
     A CloudRegion encapsulates the config information needed for connections
     to all of the services in a Region of a Cloud.
+
+    TODO(efried): Doc the rest of the kwargs
+
+    :param str region_name:
+        The default region name for all services in this CloudRegion. If
+        both ``region_name`` and ``config['region_name'] are specified, the
+        kwarg takes precedence. May be overridden for a given ${service}
+        via a ${service}_region_name key in the ``config`` dict.
+    :param dict config:
+        A dict of configuration values for the CloudRegion and its
+        services. The key for a ${config_option} for a specific ${service}
+        should be ${service}_${config_option}. For example, to configure
+        the endpoint_override for the block_storage service, the ``config``
+        dict should contain::
+
+            'block_storage_endpoint_override': 'http://...'
+
+        To provide a default to be used if no service-specific override is
+        present, just use the unprefixed ${config_option} as the service
+        key, e.g.::
+
+            'interface': 'public'
     """
     def __init__(self, name=None, region_name=None, config=None,
                  force_ipv4=False, auth_plugin=None,
@@ -170,8 +186,12 @@ class CloudRegion(object):
                  statsd_host=None, statsd_port=None, statsd_prefix=None,
                  collector_registry=None):
         self._name = name
-        self.region_name = region_name
         self.config = _util.normalize_keys(config)
+        # NOTE(efried): For backward compatibility: a) continue to accept the
+        # region_name kwarg; b) make it take precedence over (non-service_type-
+        # specific) region_name set in the config dict.
+        if region_name is not None:
+            self.config['region_name'] = region_name
         self._extra_config = extra_config or {}
         self.log = _log.setup_logging('openstack.config')
         self._force_ipv4 = force_ipv4
@@ -213,7 +233,8 @@ class CloudRegion(object):
     def __eq__(self, other):
         return (
             self.name == other.name
-            and self.region_name == other.region_name
+            # Ew
+            and self.get_region_name() == other.get_region_name()
             and self.config == other.config)
 
     def __ne__(self, other):
@@ -236,12 +257,13 @@ class CloudRegion(object):
         Always returns a valid string. It will have name and region_name
         or just one of the two if only one is set, or else 'unknown'.
         """
-        if self.name and self.region_name:
+        region_name = self.get_region_name()
-            return ":".join([self.name, self.region_name])
+        if self.name and region_name:
-        elif self.name and not self.region_name:
+            return ":".join([self.name, region_name])
+        elif self.name and not region_name:
             return self.name
-        elif not self.name and self.region_name:
+        elif not self.name and region_name:
-            return self.region_name
+            return region_name
         else:
             return 'unknown'
 
@@ -335,6 +357,12 @@ class CloudRegion(object):
             if st in config_dict:
                 return config_dict[st]
 
+    def get_region_name(self, service_type=None):
+        # If a region_name for the specific service_type is configured, use it;
+        # else use the one configured for the CloudRegion as a whole.
+        return self._get_config(
+            'region_name', service_type, fallback_to_unprefixed=True)
+
     def get_interface(self, service_type=None):
         return self._get_config(
             'interface', service_type, fallback_to_unprefixed=True)
@@ -523,12 +551,13 @@ class CloudRegion(object):
         # Seriously. Don't think about the existential crisis
         # that is the next line. You'll wind up in cthulhu's lair.
         service_type = self.get_service_type(service_type)
+        region_name = self.get_region_name(service_type)
         versions = self.get_session().get_all_version_data(
             service_type=service_type,
             interface=self.get_interface(service_type),
-            region_name=self.region_name,
+            region_name=region_name,
         )
-        region_versions = versions.get(self.region_name, {})
+        region_versions = versions.get(region_name, {})
         interface_versions = region_versions.get(
             self.get_interface(service_type), {})
         return interface_versions.get(service_type, [])
@@ -539,7 +568,7 @@ class CloudRegion(object):
             service_type=self.get_service_type(service_type),
             service_name=self.get_service_name(service_type),
             interface=self.get_interface(service_type),
-            region_name=self.region_name,
+            region_name=self.get_region_name(service_type),
         )
         endpoint = adapter.get_endpoint()
         if not endpoint.rstrip().rsplit('/')[-1] == 'v2.0':
@@ -567,6 +596,7 @@ class CloudRegion(object):
         """
         version_request = self._get_version_request(service_type, version)
 
+        kwargs.setdefault('region_name', self.get_region_name(service_type))
         kwargs.setdefault('connect_retries',
                           self.get_connect_retries(service_type))
         kwargs.setdefault('status_code_retries',
@@ -599,7 +629,6 @@ class CloudRegion(object):
             service_type=self.get_service_type(service_type),
             service_name=self.get_service_name(service_type),
             interface=self.get_interface(service_type),
-            region_name=self.region_name,
             version=version,
             min_version=min_api_version,
             max_version=max_api_version,
@@ -666,6 +695,7 @@ class CloudRegion(object):
         if override_endpoint:
             return override_endpoint
 
+        region_name = self.get_region_name(service_type)
         service_name = self.get_service_name(service_type)
         interface = self.get_interface(service_type)
         session = self.get_session()
@@ -680,7 +710,7 @@ class CloudRegion(object):
             # the request
             endpoint = session.get_endpoint(
                 service_type=service_type,
-                region_name=self.region_name,
+                region_name=region_name,
                 interface=interface,
                 service_name=service_name,
                 **version_kwargs
@@ -695,7 +725,7 @@ class CloudRegion(object):
                 service_type,
                 service_name,
                 interface,
-                self.region_name,
+                region_name,
             )
         return endpoint
 

openstack/service_description.py

@@ -178,20 +178,21 @@ class ServiceDescription(object):
         )
         found_version = temp_adapter.get_api_major_version()
         if found_version is None:
+            region_name = instance.config.get_region_name(self.service_type)
             if version_kwargs:
                 raise exceptions.NotSupported(
                     "The {service_type} service for {cloud}:{region_name}"
                     " exists but does not have any supported versions.".format(
                         service_type=self.service_type,
                         cloud=instance.name,
-                        region_name=instance.config.region_name))
+                        region_name=region_name))
             else:
                 raise exceptions.NotSupported(
                     "The {service_type} service for {cloud}:{region_name}"
                     " exists but no version was discoverable.".format(
                         service_type=self.service_type,
                         cloud=instance.name,
-                        region_name=instance.config.region_name))
+                        region_name=region_name))
         proxy_class = self.supported_versions.get(str(found_version[0]))
         if not proxy_class:
             # Maybe openstacksdk is being used for the passthrough

openstack/tests/unit/cloud/test_meta.py

@@ -25,7 +25,9 @@ PUBLIC_V6 = '2001:0db8:face:0da0:face::0b00:1c'  # rfc3849
 
 
 class FakeConfig(object):
-    region_name = 'test-region'
+    def get_region_name(self, service_type=None):
+        # TODO(efried): Validate service_type?
+        return 'test-region'
 
 
 class FakeCloud(object):

openstack/tests/unit/cloud/test_operator.py

@@ -74,7 +74,7 @@ class TestOperatorCloud(base.TestCase):
         session_mock.get_endpoint.side_effect = side_effect
         get_session_mock.return_value = session_mock
         self.cloud.name = 'testcloud'
-        self.cloud.config.region_name = 'testregion'
+        self.cloud.config.config['region_name'] = 'testregion'
         with testtools.ExpectedException(
                 exc.OpenStackCloudException,
                 "Error getting image endpoint on testcloud:testregion:"

openstack/tests/unit/config/test_cloud_config.py

@@ -181,6 +181,55 @@ class TestCloudRegion(base.TestCase):
         self.assertEqual(1, cc.get_connect_retries('compute'))
         self.assertEqual(3, cc.get_connect_retries('baremetal'))
 
+    def test_get_region_name(self):
+        # TODO(efried): ddt this
+
+        # No region_name kwarg, no regions specified in services dict
+        # (including the default).
+        cc = cloud_region.CloudRegion(config=fake_services_dict)
+        self.assertIsNone(cc.region_name)
+        self.assertIsNone(cc.get_region_name())
+        self.assertIsNone(cc.get_region_name(service_type=None))
+        self.assertIsNone(cc.get_region_name(service_type='compute'))
+        self.assertIsNone(cc.get_region_name(service_type='placement'))
+
+        # Only region_name kwarg; it's returned for everything
+        cc = cloud_region.CloudRegion(
+            region_name='foo', config=fake_services_dict)
+        self.assertEqual('foo', cc.region_name)
+        self.assertEqual('foo', cc.get_region_name())
+        self.assertEqual('foo', cc.get_region_name(service_type=None))
+        self.assertEqual('foo', cc.get_region_name(service_type='compute'))
+        self.assertEqual('foo', cc.get_region_name(service_type='placement'))
+
+        # No region_name kwarg; values (including default) show through from
+        # config dict
+        services_dict = dict(
+            fake_services_dict,
+            region_name='the-default', compute_region_name='compute-region')
+        cc = cloud_region.CloudRegion(config=services_dict)
+        self.assertEqual('the-default', cc.region_name)
+        self.assertEqual('the-default', cc.get_region_name())
+        self.assertEqual('the-default', cc.get_region_name(service_type=None))
+        self.assertEqual(
+            'compute-region', cc.get_region_name(service_type='compute'))
+        self.assertEqual(
+            'the-default', cc.get_region_name(service_type='placement'))
+
+        # region_name kwarg overrides config dict default (for backward
+        # compatibility), but service-specific region_name takes precedence.
+        services_dict = dict(
+            fake_services_dict,
+            region_name='dict', compute_region_name='compute-region')
+        cc = cloud_region.CloudRegion(
+            region_name='kwarg', config=services_dict)
+        self.assertEqual('kwarg', cc.region_name)
+        self.assertEqual('kwarg', cc.get_region_name())
+        self.assertEqual('kwarg', cc.get_region_name(service_type=None))
+        self.assertEqual(
+            'compute-region', cc.get_region_name(service_type='compute'))
+        self.assertEqual('kwarg', cc.get_region_name(service_type='placement'))
+
     def test_aliases(self):
         services_dict = fake_services_dict.copy()
         services_dict['volume_api_version'] = 12

openstack/tests/unit/config/test_from_conf.py

@@ -56,13 +56,6 @@ class TestFromConf(base.TestCase):
             oslocfg, session=self.cloud.session, name='from_conf.example.com')
         self.assertEqual('from_conf.example.com', config.name)
 
-        # TODO(efried): Currently region_name gets set to the last value seen
-        # in the config, which is nondeterministic and surely incorrect.
-        # Sometimes that's SpecialRegion, but some tests use the base fixtures
-        # which have no compute endpoint in SpecialRegion. Force override for
-        # now to make those tests work.
-        config.region_name = None
-
         return connection.Connection(config=config)
 
     def test_adapter_opts_set(self):
@@ -94,8 +87,7 @@ class TestFromConf(base.TestCase):
         ])
 
         adap = conn.orchestration
-        # TODO(efried): Fix this when region_name behaves correctly.
+        self.assertEqual('SpecialRegion', adap.region_name)
-        # self.assertEqual('SpecialRegion', adap.region_name)
         self.assertEqual('orchestration', adap.service_type)
         self.assertEqual('internal', adap.interface)
         self.assertEqual('https://example.org:8888/heat/v2',