Merge "Factor up (most) CONF value in clients.Manager"
diff --git a/releasenotes/notes/service_client_config-8a1d7b4de769c633.yaml b/releasenotes/notes/service_client_config-8a1d7b4de769c633.yaml
new file mode 100644
index 0000000..3e43f9a
--- /dev/null
+++ b/releasenotes/notes/service_client_config-8a1d7b4de769c633.yaml
@@ -0,0 +1,6 @@
+---
+features:
+  - A new helper method `service_client_config` has been added
+    to the stable module config.py that returns extracts from
+    configuration into a dictionary the configuration settings
+    relevant for the initisialisation of a service client.
diff --git a/tempest/clients.py b/tempest/clients.py
index ef03e80..fd010f2 100644
--- a/tempest/clients.py
+++ b/tempest/clients.py
@@ -21,6 +21,7 @@
 from tempest import config
 from tempest import exceptions
 from tempest.lib import auth
+from tempest.lib import exceptions as lib_exc
 from tempest.lib.services import compute
 from tempest.lib.services import image
 from tempest.lib.services import network
@@ -39,15 +40,10 @@
 class Manager(service_clients.ServiceClients):
     """Top level manager for OpenStack tempest clients"""
 
-    default_params = {
-        'disable_ssl_certificate_validation':
-            CONF.identity.disable_ssl_certificate_validation,
-        'ca_certs': CONF.identity.ca_certificates_file,
-        'trace_requests': CONF.debug.trace_requests
-    }
+    default_params = config.service_client_config()
 
-    # NOTE: Tempest uses timeout values of compute API if project specific
-    # timeout values don't exist.
+    # TODO(andreaf) This is only used by data_processing and baremetal clients,
+    # and should be removed once they are out of Tempest
     default_params_with_timeout_values = {
         'build_interval': CONF.compute.build_interval,
         'build_timeout': CONF.compute.build_timeout
@@ -65,7 +61,11 @@
         _, identity_uri = get_auth_provider_class(credentials)
         super(Manager, self).__init__(
             credentials=credentials, identity_uri=identity_uri, scope=scope,
-            region=CONF.identity.region, **self.default_params)
+            region=CONF.identity.region,
+            client_parameters=self._prepare_configuration())
+        # TODO(andreaf) When clients are initialised without the right
+        # parameters available, the calls below will trigger a KeyError.
+        # We should catch that and raise a better error.
         self._set_compute_clients()
         self._set_identity_clients()
         self._set_volume_clients()
@@ -96,15 +96,38 @@
         self.negative_client = negative_rest_client.NegativeRestClient(
             self.auth_provider, service, **self.default_params)
 
+    def _prepare_configuration(self):
+        """Map values from CONF into Manager parameters
+
+        This uses `config.service_client_config` for all services to collect
+        most configuration items needed to init the clients.
+        """
+        # NOTE(andreaf) Configuration items will be passed in future patches
+        # into ClientFactory objects, but for now we update all the
+        # _set_*_client methods to consume them so we can verify that the
+        # configuration collected is correct
+
+        configuration = {}
+
+        # Setup the parameters for all Tempest services.
+        # NOTE(andreaf) Since client.py is an internal module of Tempest,
+        # it doesn't have to consider plugin configuration.
+        for service in service_clients.tempest_modules():
+            try:
+                # NOTE(andreaf) Use the unversioned service name to fetch
+                # the configuration since configuration is not versioned.
+                service_for_config = service.split('.')[0]
+                if service_for_config not in configuration:
+                    configuration[service_for_config] = (
+                        config.service_client_config(service_for_config))
+            except lib_exc.UnknownServiceClient:
+                LOG.warn(
+                    'Could not load configuration for service %s' % service)
+
+        return configuration
+
     def _set_network_clients(self):
-        params = {
-            'service': CONF.network.catalog_type,
-            'region': CONF.network.region or CONF.identity.region,
-            'endpoint_type': CONF.network.endpoint_type,
-            'build_interval': CONF.network.build_interval,
-            'build_timeout': CONF.network.build_timeout
-        }
-        params.update(self.default_params)
+        params = self.parameters['network']
         self.network_agents_client = network.AgentsClient(
             self.auth_provider, **params)
         self.network_extensions_client = network.ExtensionsClient(
@@ -135,20 +158,13 @@
             self.auth_provider, **params)
 
     def _set_image_clients(self):
-        params = {
-            'service': CONF.image.catalog_type,
-            'region': CONF.image.region or CONF.identity.region,
-            'endpoint_type': CONF.image.endpoint_type,
-            'build_interval': CONF.image.build_interval,
-            'build_timeout': CONF.image.build_timeout
-        }
-        params.update(self.default_params)
-
         if CONF.service_available.glance:
+            params = self.parameters['image']
             self.image_client = image.v1.ImagesClient(
                 self.auth_provider, **params)
             self.image_member_client = image.v1.ImageMembersClient(
                 self.auth_provider, **params)
+
             self.image_client_v2 = image.v2.ImagesClient(
                 self.auth_provider, **params)
             self.image_member_client_v2 = image.v2.ImageMembersClient(
@@ -161,14 +177,7 @@
                 self.auth_provider, **params)
 
     def _set_compute_clients(self):
-        params = {
-            'service': CONF.compute.catalog_type,
-            'region': CONF.compute.region or CONF.identity.region,
-            'endpoint_type': CONF.compute.endpoint_type,
-            'build_interval': CONF.compute.build_interval,
-            'build_timeout': CONF.compute.build_timeout
-        }
-        params.update(self.default_params)
+        params = self.parameters['compute']
 
         self.agents_client = compute.AgentsClient(self.auth_provider, **params)
         self.compute_networks_client = compute.NetworksClient(
@@ -234,10 +243,11 @@
         # NOTE: The following client needs special timeout values because
         # the API is a proxy for the other component.
         params_volume = copy.deepcopy(params)
-        params_volume.update({
-            'build_interval': CONF.volume.build_interval,
-            'build_timeout': CONF.volume.build_timeout
-        })
+        # Optional parameters
+        for _key in ('build_interval', 'build_timeout'):
+            _value = self.parameters['volume'].get(_key)
+            if _value:
+                params_volume[_key] = _value
         self.volumes_extensions_client = compute.VolumesClient(
             self.auth_provider, **params_volume)
         self.compute_versions_client = compute.VersionsClient(
@@ -246,14 +256,10 @@
             self.auth_provider, **params_volume)
 
     def _set_identity_clients(self):
-        params = {
-            'service': CONF.identity.catalog_type,
-            'region': CONF.identity.region
-        }
-        params.update(self.default_params_with_timeout_values)
+        params = self.parameters['identity']
 
         # Clients below use the admin endpoint type of Keystone API v2
-        params_v2_admin = params.copy()
+        params_v2_admin = copy.copy(params)
         params_v2_admin['endpoint_type'] = CONF.identity.v2_admin_endpoint_type
         self.endpoints_client = identity.v2.EndpointsClient(self.auth_provider,
                                                             **params_v2_admin)
@@ -269,7 +275,7 @@
             self.auth_provider, **params_v2_admin)
 
         # Clients below use the public endpoint type of Keystone API v2
-        params_v2_public = params.copy()
+        params_v2_public = copy.copy(params)
         params_v2_public['endpoint_type'] = (
             CONF.identity.v2_public_endpoint_type)
         self.identity_public_client = identity.v2.IdentityClient(
@@ -279,8 +285,9 @@
         self.users_public_client = identity.v2.UsersClient(
             self.auth_provider, **params_v2_public)
 
-        # Clients below use the endpoint type of Keystone API v3
-        params_v3 = params.copy()
+        # Clients below use the endpoint type of Keystone API v3, which is set
+        # in endpoint_type
+        params_v3 = copy.copy(params)
         params_v3['endpoint_type'] = CONF.identity.v3_endpoint_type
         self.domains_client = identity.v3.DomainsClient(self.auth_provider,
                                                         **params_v3)
@@ -326,14 +333,8 @@
                 raise exceptions.InvalidConfiguration(msg)
 
     def _set_volume_clients(self):
-        params = {
-            'service': CONF.volume.catalog_type,
-            'region': CONF.volume.region or CONF.identity.region,
-            'endpoint_type': CONF.volume.endpoint_type,
-            'build_interval': CONF.volume.build_interval,
-            'build_timeout': CONF.volume.build_timeout
-        }
-        params.update(self.default_params)
+        # Mandatory parameters (always defined)
+        params = self.parameters['volume']
 
         self.volume_qos_client = volume.v1.QosSpecsClient(self.auth_provider,
                                                           **params)
@@ -381,12 +382,8 @@
             volume.v2.AvailabilityZoneClient(self.auth_provider, **params)
 
     def _set_object_storage_clients(self):
-        params = {
-            'service': CONF.object_storage.catalog_type,
-            'region': CONF.object_storage.region or CONF.identity.region,
-            'endpoint_type': CONF.object_storage.endpoint_type
-        }
-        params.update(self.default_params_with_timeout_values)
+        # Mandatory parameters (always defined)
+        params = self.parameters['object-storage']
 
         self.account_client = object_storage.AccountClient(self.auth_provider,
                                                            **params)
@@ -404,12 +401,8 @@
 
 
 def get_auth_provider(credentials, pre_auth=False, scope='project'):
-    default_params = {
-        'disable_ssl_certificate_validation':
-            CONF.identity.disable_ssl_certificate_validation,
-        'ca_certs': CONF.identity.ca_certificates_file,
-        'trace_requests': CONF.debug.trace_requests
-    }
+    # kwargs for auth provider match the common ones used by service clients
+    default_params = config.service_client_config()
     if credentials is None:
         raise exceptions.InvalidCredentials(
             'Credentials must be specified')
diff --git a/tempest/config.py b/tempest/config.py
index 228d7e3..0c2b913 100644
--- a/tempest/config.py
+++ b/tempest/config.py
@@ -25,6 +25,7 @@
 from oslo_log import log as logging
 import testtools
 
+from tempest.lib import exceptions
 from tempest.test_discover import plugins
 
 
@@ -1350,3 +1351,84 @@
             return f(self, *func_args, **func_kwargs)
         return wrapper
     return decorator
+
+
+def service_client_config(service_client_name=None):
+    """Return a dict with the parameters to init service clients
+
+    Extracts from CONF the settings specific to the service_client_name and
+    api_version, and formats them as dict ready to be passed to the service
+    clients __init__:
+
+        * `region` (default to identity)
+        * `catalog_type`
+        * `endpoint_type`
+        * `build_timeout` (object-storage and identity default to compute)
+        * `build_interval` (object-storage and identity default to compute)
+
+    The following common settings are always returned, even if
+    `service_client_name` is None:
+
+        * `disable_ssl_certificate_validation`
+        * `ca_certs`
+        * `trace_requests`
+
+    The dict returned by this does not fit a few service clients:
+
+        * The endpoint type is not returned for identity client, since it takes
+          three different values for v2 admin, v2 public and v3
+        * The `ServersClient` from compute accepts an optional
+          `enable_instance_password` parameter, which is not returned.
+        * The `VolumesClient` for both v1 and v2 volume accept an optional
+          `default_volume_size` parameter, which is not returned.
+        * The `TokenClient` and `V3TokenClient` have a very different
+          interface, only auth_url is needed for them.
+
+    :param service_client_name: str Name of the service. Supported values are
+        'compute', 'identity', 'image', 'network', 'object-storage', 'volume'
+    :return: dictionary of __init__ parameters for the service clients
+    :rtype: dict
+    """
+    _parameters = {
+        'disable_ssl_certificate_validation':
+            CONF.identity.disable_ssl_certificate_validation,
+        'ca_certs': CONF.identity.ca_certificates_file,
+        'trace_requests': CONF.debug.trace_requests
+    }
+
+    if service_client_name is None:
+        return _parameters
+
+    # Get the group of options first, by normalising the service_group_name
+    # Services with a '-' in the name have an '_' in the option group name
+    config_group = service_client_name.replace('-', '_')
+    # NOTE(andreaf) Check if the config group exists. This allows for this
+    # helper to be used for settings from registered plugins as well
+    try:
+        options = getattr(CONF, config_group)
+    except cfg.NoSuchOptError:
+        # Option group not defined
+        raise exceptions.UnknownServiceClient(services=service_client_name)
+    # Set endpoint_type
+    # Identity uses different settings depending on API version, so do not
+    # return the endpoint at all.
+    if service_client_name != 'identity':
+        _parameters['endpoint_type'] = getattr(options, 'endpoint_type')
+    # Set build_*
+    # Object storage and identity groups do not have conf settings for
+    # build_* parameters, and we default to compute in any case
+    for setting in ['build_timeout', 'build_interval']:
+        if not hasattr(options, setting) or not getattr(options, setting):
+            _parameters[setting] = getattr(CONF.compute, setting)
+        else:
+            _parameters[setting] = getattr(options, setting)
+    # Set region
+    # If a service client does not define region or region is not set
+    # default to the identity region
+    if not hasattr(options, 'region') or not getattr(options, 'region'):
+        _parameters['region'] = CONF.identity.region
+    else:
+        _parameters['region'] = getattr(options, 'region')
+    # Set service
+    _parameters['service'] = getattr(options, 'catalog_type')
+    return _parameters
diff --git a/tempest/lib/exceptions.py b/tempest/lib/exceptions.py
index 2a6a788..5ca78f9 100644
--- a/tempest/lib/exceptions.py
+++ b/tempest/lib/exceptions.py
@@ -225,3 +225,7 @@
     message = ("Command '%(command)s', exit status: %(exit_status)d, "
                "stderr:\n%(stderr)s\n"
                "stdout:\n%(stdout)s")
+
+
+class UnknownServiceClient(TempestException):
+    message = "Service clients named %(services)s are not known"
diff --git a/tempest/service_clients.py b/tempest/service_clients.py
index 3208c8d..386e621 100644
--- a/tempest/service_clients.py
+++ b/tempest/service_clients.py
@@ -18,6 +18,22 @@
 from tempest.lib import exceptions
 
 
+def tempest_modules():
+    """List of service client modules available in Tempest.
+
+    Provides a list of service modules available Tempest.
+    """
+    return set(['compute', 'identity.v2', 'identity.v3', 'image.v1',
+                'image.v2', 'network', 'object-storage', 'volume.v1',
+                'volume.v2', 'volume.v3'])
+
+
+def available_modules():
+    """List of service client modules available in Tempest and plugins"""
+    # TODO(andreaf) For now this returns only tempest_modules
+    return tempest_modules()
+
+
 class ServiceClients(object):
     """Service client provider class
 
@@ -30,7 +46,8 @@
 
         >>> from tempest import service_clients
         >>> johndoe = cred_provider.get_creds_by_role(['johndoe'])
-        >>> johndoe_clients = service_clients.ServiceClients(johndoe)
+        >>> johndoe_clients = service_clients.ServiceClients(johndoe,
+        >>>                                                  identity_uri)
         >>> johndoe_servers = johndoe_clients.servers_client.list_servers()
 
     """
@@ -40,17 +57,45 @@
     # initialises this class using values from tempest CONF object. The wrapper
     # class should only be used by tests hosted in Tempest.
 
-    def __init__(self, credentials, identity_uri, region=None,
-                 scope='project', disable_ssl_certificate_validation=True,
-                 ca_certs=None, trace_requests=''):
+    def __init__(self, credentials, identity_uri, region=None, scope='project',
+                 disable_ssl_certificate_validation=True, ca_certs=None,
+                 trace_requests='', client_parameters=None):
         """Service Clients provider
 
         Instantiate a `ServiceClients` object, from a set of credentials and an
         identity URI. The identity version is inferred from the credentials
         object. Optionally auth scope can be provided.
+
+        A few parameters can be given a value which is applied as default
+        for all service clients: region, dscv, ca_certs, trace_requests.
+
         Parameters dscv, ca_certs and trace_requests all apply to the auth
         provider as well as any service clients provided by this manager.
 
+        Any other client parameter must be set via client_parameters.
+        The list of available parameters is defined in the service clients
+        interfaces. For reference, most clients will accept 'region',
+        'service', 'endpoint_type', 'build_timeout' and 'build_interval', which
+        are all inherited from RestClient.
+
+        The `config` module in Tempest exposes an helper function
+        `service_client_config` that can be used to extract from configuration
+        a dictionary ready to be injected in kwargs.
+
+        Exceptions are:
+        - Token clients for 'identity' have a very different interface
+        - Volume client for 'volume' accepts 'default_volume_size'
+        - Servers client from 'compute' accepts 'enable_instance_password'
+
+        Examples:
+
+            >>> identity_params = config.service_client_config('identity')
+            >>> params = {
+            >>>     'identity': identity_params,
+            >>>     'compute': {'region': 'region2'}}
+            >>> manager = lib_manager.Manager(
+            >>>     my_creds, identity_uri, client_parameters=params)
+
         :param credentials: An instance of `auth.Credentials`
         :param identity_uri: URI of the identity API. This should be a
                              mandatory parameter, and it will so soon.
@@ -60,12 +105,26 @@
                                                   service clients.
         :param ca_certs Applies to auth and to all service clients.
         :param trace_requests Applies to auth and to all service clients.
+        :param client_parameters Dictionary with parameters for service
+            clients. Keys of the dictionary are the service client service
+            name, as declared in `service_clients.available_modules()` except
+            for the version. Values are dictionaries of parameters that are
+            going to be passed to all clients in the service client module.
+
+        Examples:
+
+            >>> params_service_x = {'param_name': 'param_value'}
+            >>> client_parameters = { 'service_x': params_service_x }
+
+            >>> params_service_y = config.service_client_config('service_y')
+            >>> client_parameters['service_y'] = params_service_y
+
         """
         self.credentials = credentials
         self.identity_uri = identity_uri
         if not identity_uri:
             raise exceptions.InvalidCredentials(
-                'Manager requires a non-empty identity_uri.')
+                'ServiceClients requires a non-empty identity_uri.')
         self.region = region
         # Check if passed or default credentials are valid
         if not self.credentials.is_valid():
@@ -88,3 +147,32 @@
             self.credentials, self.identity_uri, scope=scope,
             disable_ssl_certificate_validation=self.dscv,
             ca_certs=self.ca_certs, trace_requests=self.trace_requests)
+        # Setup some defaults for client parameters of registered services
+        client_parameters = client_parameters or {}
+        self.parameters = {}
+        # Parameters are provided for unversioned services
+        unversioned_services = set(
+            [x.split('.')[0] for x in available_modules()])
+        for service in unversioned_services:
+            self.parameters[service] = self._setup_parameters(
+                client_parameters.pop(service, {}))
+        # Check that no client parameters was supplied for unregistered clients
+        if client_parameters:
+            raise exceptions.UnknownServiceClient(
+                services=list(client_parameters.keys()))
+
+    def _setup_parameters(self, parameters):
+        """Setup default values for client parameters
+
+        Region by default is the region passed as an __init__ parameter.
+        Checks that no parameter for an unknown service is provided.
+        """
+        _parameters = {}
+        # Use region from __init__
+        if self.region:
+            _parameters['region'] = self.region
+        # Update defaults with specified parameters
+        _parameters.update(parameters)
+        # If any parameter is left, parameters for an unknown service were
+        # provided as input. Fail rather than ignore silently.
+        return _parameters
diff --git a/tempest/tests/fake_config.py b/tempest/tests/fake_config.py
index 65164a0..71a4c81 100644
--- a/tempest/tests/fake_config.py
+++ b/tempest/tests/fake_config.py
@@ -57,3 +57,57 @@
     def __init__(self, parse_conf=True, config_path=None):
         self._set_attrs()
         self.lock_path = cfg.CONF.oslo_concurrency.lock_path
+
+fake_service1_group = cfg.OptGroup(name='fake-service1', title='Fake service1')
+
+FakeService1Group = [
+    cfg.StrOpt('catalog_type', default='fake-service1'),
+    cfg.StrOpt('endpoint_type', default='faketype'),
+    cfg.StrOpt('region', default='fake_region'),
+    cfg.IntOpt('build_timeout', default=99),
+    cfg.IntOpt('build_interval', default=9)]
+
+fake_service2_group = cfg.OptGroup(name='fake-service2', title='Fake service2')
+
+FakeService2Group = [
+    cfg.StrOpt('catalog_type', default='fake-service2'),
+    cfg.StrOpt('endpoint_type', default='faketype')]
+
+
+class ServiceClientsConfigFixture(conf_fixture.Config):
+
+    def __init__(self):
+        cfg.CONF([], default_config_files=[])
+        config._opts.append((fake_service1_group, FakeService1Group))
+        config._opts.append((fake_service2_group, FakeService2Group))
+        config.register_opts()
+        super(ServiceClientsConfigFixture, self).__init__()
+
+    def setUp(self):
+        super(ServiceClientsConfigFixture, self).setUp()
+        # Debug default values
+        self.conf.set_default('trace_requests', 'fake_module', 'debug')
+        # Identity default values
+        self.conf.set_default('disable_ssl_certificate_validation', True,
+                              group='identity')
+        self.conf.set_default('ca_certificates_file', '/fake/certificates',
+                              group='identity')
+        self.conf.set_default('region', 'fake_region', 'identity')
+        # Identity endpoints
+        self.conf.set_default('v3_endpoint_type', 'fake_v3_uri', 'identity')
+        self.conf.set_default('v2_public_endpoint_type', 'fake_v2_public_uri',
+                              'identity')
+        self.conf.set_default('v2_admin_endpoint_type', 'fake_v2_admin_uri',
+                              'identity')
+        # Compute default values
+        self.conf.set_default('build_interval', 88, group='compute')
+        self.conf.set_default('build_timeout', 8, group='compute')
+
+
+class ServiceClientsFakePrivate(config.TempestConfigPrivate):
+    def __init__(self, parse_conf=True, config_path=None):
+        self._set_attrs()
+        self.fake_service1 = cfg.CONF['fake-service1']
+        self.fake_service2 = cfg.CONF['fake-service2']
+        print('Services registered')
+        self.lock_path = cfg.CONF.oslo_concurrency.lock_path
diff --git a/tempest/tests/test_config.py b/tempest/tests/test_config.py
new file mode 100644
index 0000000..2808a9c
--- /dev/null
+++ b/tempest/tests/test_config.py
@@ -0,0 +1,89 @@
+# Copyright (c) 2016 Hewlett-Packard Enterprise Development Company, L.P.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may obtain a copy of
+# the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations under
+# the License.
+
+import testtools
+
+from tempest import config
+from tempest.lib import exceptions
+from tempest.tests import base
+from tempest.tests import fake_config
+
+
+class TestServiceClientConfig(base.TestCase):
+
+    expected_common_params = set(['disable_ssl_certificate_validation',
+                                  'ca_certs', 'trace_requests'])
+    expected_extra_params = set(['service', 'endpoint_type', 'region',
+                                 'build_timeout', 'build_interval'])
+
+    def setUp(self):
+        super(TestServiceClientConfig, self).setUp()
+        self.useFixture(fake_config.ServiceClientsConfigFixture())
+        self.patchobject(config, 'CONF',
+                         fake_config.ServiceClientsFakePrivate())
+        self.CONF = config.CONF
+
+    def test_service_client_config_no_service(self):
+        params = config.service_client_config()
+        for param_name in self.expected_common_params:
+            self.assertIn(param_name, params)
+        for param_name in self.expected_extra_params:
+            self.assertNotIn(param_name, params)
+        self.assertEqual(
+            self.CONF.identity.disable_ssl_certificate_validation,
+            params['disable_ssl_certificate_validation'])
+        self.assertEqual(self.CONF.identity.ca_certificates_file,
+                         params['ca_certs'])
+        self.assertEqual(self.CONF.debug.trace_requests,
+                         params['trace_requests'])
+
+    def test_service_client_config_service_all(self):
+        params = config.service_client_config(
+            service_client_name='fake-service1')
+        for param_name in self.expected_common_params:
+            self.assertIn(param_name, params)
+        for param_name in self.expected_extra_params:
+            self.assertIn(param_name, params)
+        self.assertEqual(self.CONF.fake_service1.catalog_type,
+                         params['service'])
+        self.assertEqual(self.CONF.fake_service1.endpoint_type,
+                         params['endpoint_type'])
+        self.assertEqual(self.CONF.fake_service1.region, params['region'])
+        self.assertEqual(self.CONF.fake_service1.build_timeout,
+                         params['build_timeout'])
+        self.assertEqual(self.CONF.fake_service1.build_interval,
+                         params['build_interval'])
+
+    def test_service_client_config_service_minimal(self):
+        params = config.service_client_config(
+            service_client_name='fake-service2')
+        for param_name in self.expected_common_params:
+            self.assertIn(param_name, params)
+        for param_name in self.expected_extra_params:
+            self.assertIn(param_name, params)
+        self.assertEqual(self.CONF.fake_service2.catalog_type,
+                         params['service'])
+        self.assertEqual(self.CONF.fake_service2.endpoint_type,
+                         params['endpoint_type'])
+        self.assertEqual(self.CONF.identity.region, params['region'])
+        self.assertEqual(self.CONF.compute.build_timeout,
+                         params['build_timeout'])
+        self.assertEqual(self.CONF.compute.build_interval,
+                         params['build_interval'])
+
+    def test_service_client_config_service_unknown(self):
+        unknown_service = 'unknown_service'
+        with testtools.ExpectedException(exceptions.UnknownServiceClient,
+                                         '.*' + unknown_service + '.*'):
+            config.service_client_config(service_client_name=unknown_service)
diff --git a/tempest/tests/test_service_clients.py b/tempest/tests/test_service_clients.py
index f67781c..d0158c7 100644
--- a/tempest/tests/test_service_clients.py
+++ b/tempest/tests/test_service_clients.py
@@ -12,6 +12,7 @@
 # License for the specific language governing permissions and limitations under
 # the License.
 
+import fixtures
 import testtools
 
 from tempest.lib import auth
@@ -23,7 +24,13 @@
 
 class TestServiceClients(base.TestCase):
 
-    def test__init__creds_v2_uri(self):
+    def setUp(self):
+        super(TestServiceClients, self).setUp()
+        self.useFixture(fixtures.MockPatch(
+            'tempest.service_clients.tempest_modules',
+            return_value=set(['fake_service1', 'fake_service2'])))
+
+    def test___init___creds_v2_uri(self):
         # Verify that no API request is made, since no mock
         # is required to run the test successfully
         creds = fake_credentials.FakeKeystoneV2Credentials()
@@ -32,7 +39,7 @@
         self.assertIsInstance(_manager.auth_provider,
                               auth.KeystoneV2AuthProvider)
 
-    def test__init__creds_v3_uri(self):
+    def test___init___creds_v3_uri(self):
         # Verify that no API request is made, since no mock
         # is required to run the test successfully
         creds = fake_credentials.FakeKeystoneV3Credentials()
@@ -41,22 +48,79 @@
         self.assertIsInstance(_manager.auth_provider,
                               auth.KeystoneV3AuthProvider)
 
-    def test__init__base_creds_uri(self):
+    def test___init___base_creds_uri(self):
         creds = fake_credentials.FakeCredentials()
         uri = 'fake_uri'
         with testtools.ExpectedException(exceptions.InvalidCredentials):
             service_clients.ServiceClients(creds, identity_uri=uri)
 
-    def test__init__invalid_creds_uri(self):
+    def test___init___invalid_creds_uri(self):
         creds = fake_credentials.FakeKeystoneV2Credentials()
         delattr(creds, 'username')
         uri = 'fake_uri'
         with testtools.ExpectedException(exceptions.InvalidCredentials):
             service_clients.ServiceClients(creds, identity_uri=uri)
 
-    def test__init__creds_uri_none(self):
+    def test___init___creds_uri_none(self):
         creds = fake_credentials.FakeKeystoneV2Credentials()
-        msg = "Invalid Credentials\nDetails: Manager requires a non-empty"
+        msg = ("Invalid Credentials\nDetails: ServiceClients requires a "
+               "non-empty")
         with testtools.ExpectedException(exceptions.InvalidCredentials,
                                          value_re=msg):
             service_clients.ServiceClients(creds, None)
+
+    def test___init___creds_uri_params(self):
+        creds = fake_credentials.FakeKeystoneV2Credentials()
+        expeted_params = {'fake_param1': 'fake_value1',
+                          'fake_param2': 'fake_value2'}
+        params = {'fake_service1': expeted_params}
+        uri = 'fake_uri'
+        _manager = service_clients.ServiceClients(creds, identity_uri=uri,
+                                                  client_parameters=params)
+        self.assertIn('fake_service1', _manager.parameters.keys())
+        for _key in expeted_params.keys():
+            self.assertIn(_key, _manager.parameters['fake_service1'].keys())
+            self.assertEqual(expeted_params[_key],
+                             _manager.parameters['fake_service1'].get(_key))
+
+    def test___init___creds_uri_params_unknown_services(self):
+        creds = fake_credentials.FakeKeystoneV2Credentials()
+        fake_params = {'fake_param1': 'fake_value1'}
+        params = {'unknown_service1': fake_params,
+                  'unknown_service2': fake_params}
+        uri = 'fake_uri'
+        msg = "(?=.*{0})(?=.*{1})".format(*list(params.keys()))
+        with testtools.ExpectedException(
+                exceptions.UnknownServiceClient, value_re=msg):
+            service_clients.ServiceClients(creds, identity_uri=uri,
+                                           client_parameters=params)
+
+    def _get_manager(self, init_region='fake_region'):
+        # Get a manager to invoke _setup_parameters on
+        creds = fake_credentials.FakeKeystoneV2Credentials()
+        return service_clients.ServiceClients(creds, identity_uri='fake_uri',
+                                              region=init_region)
+
+    def test__setup_parameters_none_no_region(self):
+        kwargs = {}
+        _manager = self._get_manager(init_region=None)
+        _params = _manager._setup_parameters(kwargs)
+        self.assertNotIn('region', _params)
+
+    def test__setup_parameters_none(self):
+        kwargs = {}
+        _manager = self._get_manager()
+        _params = _manager._setup_parameters(kwargs)
+        self.assertIn('region', _params)
+        self.assertEqual('fake_region', _params['region'])
+
+    def test__setup_parameters_all(self):
+        expected_params = {'region': 'fake_region1',
+                           'catalog_type': 'fake_service2_mod',
+                           'fake_param1': 'fake_value1',
+                           'fake_param2': 'fake_value2'}
+        _manager = self._get_manager()
+        _params = _manager._setup_parameters(expected_params)
+        for _key in _params.keys():
+            self.assertEqual(expected_params[_key],
+                             _params[_key])