Defer plugin validation to keystoneauth

keystoneauth plugin loading has parameter validation itself. Rather
than us doing it, let ksa do it.

This bubbles up a ksa exception- but I think I'm ok with that as an
interface.

Change-Id: I3e7741a1b623b133f24f321e97539883dc6cd153

4 files changed, 54 insertions, 57 deletions

diff --git a/os_client_config/cloud_config.py b/os_client_config/cloud_config.py
index c9f9f07..86b4f50 100644
--- a/os_client_config/cloud_config.py
+++ b/os_client_config/cloud_config.py
@@ -14,15 +14,15 @@
 
 import warnings
 
-from keystoneauth1 import loading
-
 
 class CloudConfig(object):
-    def __init__(self, name, region, config, prefer_ipv6=False):
+    def __init__(self, name, region, config,
+                 prefer_ipv6=False, auth_plugin=None):
         self.name = name
         self.region = region
         self.config = config
         self._prefer_ipv6 = prefer_ipv6
+        self._auth = auth_plugin
 
     def __getattr__(self, key):
         """Return arbitrary attributes."""
@@ -111,14 +111,4 @@ class CloudConfig(object):
 
     def get_auth(self):
         """Return a keystoneauth plugin from the auth credentials."""
-        # Re-use the admin_token plugin for the "None" plugin
-        # since it does not look up endpoints or tokens but rather
-        # does a passthrough. This is useful for things like Ironic
-        # that have a keystoneless operational mode, but means we're
-        # still dealing with a keystoneauth Session object, so all the
-        # _other_ things (SSL arg handling, timeout) all work consistently
-        if self.config['auth_type'] in (None, "None", ''):
-            self.config['auth_type'] = 'admin_token'
-            self.config['auth']['token'] = None
-        loader = loading.get_plugin_loader(self.config['auth_type'])
-        return loader.load_from_options(**self.config['auth'])
+        return self._auth
diff --git a/os_client_config/config.py b/os_client_config/config.py
index d698378..5da7799 100644
--- a/os_client_config/config.py
+++ b/os_client_config/config.py
@@ -17,13 +17,9 @@ import os
 import warnings
 
 import appdirs
+from keystoneauth1 import loading
 import yaml
 
-try:
-    import keystoneclient.auth as ksc_auth
-except ImportError:
-    ksc_auth = None
-
 from os_client_config import cloud_config
 from os_client_config import defaults
 from os_client_config import exceptions
@@ -376,19 +372,27 @@ class OpenStackConfig(object):
         if opt_name in config:
             return config[opt_name]
         else:
-            for d_opt in opt.deprecated_opts:
+            for d_opt in opt.deprecated:
                 d_opt_name = d_opt.name.replace('-', '_')
                 if d_opt_name in config:
                     return config[d_opt_name]
 
-    def _validate_auth(self, config):
+    def _get_auth_loader(self, config):
+        # Re-use the admin_token plugin for the "None" plugin
+        # since it does not look up endpoints or tokens but rather
+        # does a passthrough. This is useful for things like Ironic
+        # that have a keystoneless operational mode, but means we're
+        # still dealing with a keystoneauth Session object, so all the
+        # _other_ things (SSL arg handling, timeout) all work consistently
+        if config['auth_type'] in (None, "None", ''):
+            config['auth_type'] = 'admin_token'
+            config['auth']['token'] = None
+        return loading.get_plugin_loader(config['auth_type'])
+
+    def _validate_auth(self, config, loader):
         # May throw a keystoneclient.exceptions.NoMatchingPlugin
-        if config['auth_type'] == 'admin_endpoint':
-            auth_plugin = ksc_auth.token_endpoint.Token
-        else:
-            auth_plugin = ksc_auth.get_plugin_class(config['auth_type'])
 
-        plugin_options = auth_plugin.get_options()
+        plugin_options = loader.get_options()
 
         for p_opt in plugin_options:
             # if it's in config.auth, win, kill it from config dict
@@ -400,19 +404,8 @@ class OpenStackConfig(object):
             if not winning_value:
                 winning_value = self._find_winning_auth_value(p_opt, config)
 
-            # if the plugin tells us that this value is required
-            # then error if it's doesn't exist now
-            if not winning_value and p_opt.required:
-                raise exceptions.OpenStackConfigException(
-                    'Unable to find auth information for cloud'
-                    ' {cloud} in config files {files}'
-                    ' or environment variables. Missing value {auth_key}'
-                    ' required for auth plugin {plugin}'.format(
-                        cloud=cloud, files=','.join(self._config_files),
-                        auth_key=p_opt.name, plugin=config.get('auth_type')))
-
             # Clean up after ourselves
-            for opt in [p_opt.name] + [o.name for o in p_opt.deprecated_opts]:
+            for opt in [p_opt.name] + [o.name for o in p_opt.deprecated]:
                 opt = opt.replace('-', '_')
                 config.pop(opt, None)
                 config['auth'].pop(opt, None)
@@ -435,13 +428,16 @@ class OpenStackConfig(object):
         :param string cloud:
             The name of the configuration to load from clouds.yaml
         :param boolean validate:
-            Validate that required arguments are present and certain
-            argument combinations are valid
+            Validate the config. Setting this to False causes no auth plugin
+            to be created. It's really only useful for testing.
         :param Namespace argparse:
             An argparse Namespace object; allows direct passing in of
             argparse options to be added to the cloud config.  Values
             of None and '' will be removed.
         :param kwargs: Additional configuration options
+
+        :raises: keystoneauth1.exceptions.MissingRequiredOptions
+            on missing required auth parameters
         """
 
         if cloud is None and self.envvar_key in self.get_cloud_names():
@@ -471,12 +467,12 @@ class OpenStackConfig(object):
                 if type(config[key]) is not bool:
                     config[key] = get_boolean(config[key])
 
-        if 'auth_type' in config:
-            if config['auth_type'] in ('', 'None', None):
-                validate = False
-
-        if validate and ksc_auth:
-            config = self._validate_auth(config)
+        loader = self._get_auth_loader(config)
+        if validate:
+            config = self._validate_auth(config, loader)
+            auth_plugin = loader.load_from_options(**config['auth'])
+        else:
+            auth_plugin = None
 
         # If any of the defaults reference other values, we need to expand
         for (key, value) in config.items():
@@ -492,7 +488,8 @@ class OpenStackConfig(object):
         return cloud_config.CloudConfig(
             name=cloud_name, region=config['region_name'],
             config=self._normalize_keys(config),
-            prefer_ipv6=prefer_ipv6)
+            prefer_ipv6=prefer_ipv6,
+            auth_plugin=auth_plugin)
 
     @staticmethod
     def set_one_cloud(config_file, cloud, set_config=None):
diff --git a/os_client_config/tests/base.py b/os_client_config/tests/base.py
index 89e04c0..9a29237 100644
--- a/os_client_config/tests/base.py
+++ b/os_client_config/tests/base.py
@@ -31,6 +31,7 @@ VENDOR_CONF = {
     'public-clouds': {
         '_test_cloud_in_our_cloud': {
             'auth': {
+                'auth_url': 'http://example.com/v2',
                 'username': 'testotheruser',
                 'project_name': 'testproject',
             },
@@ -45,6 +46,7 @@ USER_CONF = {
         '_test-cloud_': {
             'profile': '_test_cloud_in_our_cloud',
             'auth': {
+                'auth_url': 'http://example.com/v2',
                 'username': 'testuser',
                 'password': 'testpass',
             },
@@ -53,6 +55,7 @@ USER_CONF = {
         '_test_cloud_no_vendor': {
             'profile': '_test_non_existant_cloud',
             'auth': {
+                'auth_url': 'http://example.com/v2',
                 'username': 'testuser',
                 'password': 'testpass',
                 'project_name': 'testproject',
@@ -64,6 +67,7 @@ USER_CONF = {
                 'username': 'testuser',
                 'password': 'testpass',
                 'project_id': 12345,
+                'auth_url': 'http://example.com/v2',
             },
             'region_name': 'test-region',
         },
@@ -72,6 +76,7 @@ USER_CONF = {
                 'username': 'testuser',
                 'password': 'testpass',
                 'project-id': 'testproject',
+                'auth_url': 'http://example.com/v2',
             },
             'regions': [
                 'region1',
@@ -83,6 +88,7 @@ USER_CONF = {
                 'username': 'testuser',
                 'password': 'testpass',
                 'project-id': '12345',
+                'auth_url': 'http://example.com/v2',
             },
             'region_name': 'test-region',
         }
diff --git a/os_client_config/tests/test_config.py b/os_client_config/tests/test_config.py
index 332e4d3..82f2fb9 100644
--- a/os_client_config/tests/test_config.py
+++ b/os_client_config/tests/test_config.py
@@ -43,7 +43,7 @@ class TestConfig(base.TestCase):
     def test_get_one_cloud(self):
         c = config.OpenStackConfig(config_files=[self.cloud_yaml],
                                    vendor_files=[self.vendor_yaml])
-        cloud = c.get_one_cloud()
+        cloud = c.get_one_cloud(validate=False)
         self.assertIsInstance(cloud, cloud_config.CloudConfig)
         self.assertEqual(cloud.name, '')
 
@@ -61,12 +61,12 @@ class TestConfig(base.TestCase):
         )
 
     def test_get_one_cloud_auth_override_defaults(self):
-        default_options = {'auth_type': 'token'}
+        default_options = {'compute_api_version': '4'}
         c = config.OpenStackConfig(config_files=[self.cloud_yaml],
                                    override_defaults=default_options)
         cc = c.get_one_cloud(cloud='_test-cloud_', auth={'username': 'user'})
         self.assertEqual('user', cc.auth['username'])
-        self.assertEqual('token', cc.auth_type)
+        self.assertEqual('4', cc.compute_api_version)
         self.assertEqual(
             defaults._defaults['identity_api_version'],
             cc.identity_api_version,
@@ -109,7 +109,7 @@ class TestConfig(base.TestCase):
         for k in os.environ.keys():
             if k.startswith('OS_'):
                 self.useFixture(fixtures.EnvironmentVariable(k))
-        c.get_one_cloud(cloud='defaults')
+        c.get_one_cloud(cloud='defaults', validate=False)
 
     def test_prefer_ipv6_true(self):
         c = config.OpenStackConfig(config_files=[self.cloud_yaml],
@@ -120,7 +120,7 @@ class TestConfig(base.TestCase):
     def test_prefer_ipv6_false(self):
         c = config.OpenStackConfig(config_files=[self.no_yaml],
                                    vendor_files=[self.no_yaml])
-        cc = c.get_one_cloud(cloud='defaults')
+        cc = c.get_one_cloud(cloud='defaults', validate=False)
         self.assertFalse(cc.prefer_ipv6)
 
     def test_get_one_cloud_auth_merge(self):
@@ -144,7 +144,7 @@ class TestConfig(base.TestCase):
         for k in os.environ.keys():
             if k.startswith('OS_'):
                 self.useFixture(fixtures.EnvironmentVariable(k))
-        c.get_one_cloud(cloud='defaults')
+        c.get_one_cloud(cloud='defaults', validate=False)
         self.assertEqual(['defaults'], sorted(c.get_cloud_names()))
 
     def test_set_one_cloud_creates_file(self):
@@ -168,7 +168,8 @@ class TestConfig(base.TestCase):
         resulting_cloud_config = {
             'auth': {
                 'password': 'newpass',
-                'username': 'testuser'
+                'username': 'testuser',
+                'auth_url': 'http://example.com/v2',
             },
             'cloud': 'new_cloud',
             'profile': '_test_cloud_in_our_cloud',
@@ -189,6 +190,10 @@ class TestConfigArgparse(base.TestCase):
         super(TestConfigArgparse, self).setUp()
 
         self.options = argparse.Namespace(
+            auth_url='http://example.com/v2',
+            username='user',
+            password='password',
+            project_name='project',
             region_name='other-test-region',
             snack_type='cookie',
         )
@@ -208,7 +213,6 @@ class TestConfigArgparse(base.TestCase):
 
         cc = c.get_one_cloud(cloud='', argparse=self.options)
         self.assertIsNone(cc.cloud)
-        self.assertNotIn('username', cc.auth)
         self.assertEqual(cc.region_name, 'other-test-region')
         self.assertEqual(cc.snack_type, 'cookie')
 
@@ -270,11 +274,11 @@ class TestConfigDefault(base.TestCase):
         self.assertEqual('password', cc.auth_type)
 
     def test_set_default_before_init(self):
-        config.set_default('auth_type', 'token')
+        config.set_default('identity_api_version', '4')
         c = config.OpenStackConfig(config_files=[self.cloud_yaml],
                                    vendor_files=[self.vendor_yaml])
         cc = c.get_one_cloud(cloud='_test-cloud_', argparse=None)
-        self.assertEqual('token', cc.auth_type)
+        self.assertEqual('4', cc.identity_api_version)
 
 
 class TestBackwardsCompatibility(base.TestCase):