heat engine : subclass keystone client to encapsulate common code

Encapsulate the keystone client in a heat-specific wrapper subclass
so we can put heat-specific implementation related to keystone in
one place.  This will allow easier reuse of common code between
resources which need to manipulate stack users and ec2 keys

blueprint metsrv-remove
Change-Id: I3d9751023c52cb75ab5e1f62415b1db4e4361dec
Signed-off-by: Steven Hardy <shardy@redhat.com>

heat/common/heat_keystoneclient.py

@@ -0,0 +1,159 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+#
+#    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 eventlet
+from keystoneclient.v2_0 import client as kc
+from heat.openstack.common import cfg
+from heat.openstack.common import log as logging
+
+logger = logging.getLogger('heat.common.keystoneclient')
+
+
+class KeystoneClient(object):
+    """
+    Wrap keystone client so we can encapsulate logic used in resources
+    Note this is intended to be initialized from a resource on a per-session
+    basis, so the session context is passed in on initialization
+    Also note that a copy of this is created every resource as self.keystone()
+    via the code in engine/client.py, so there should not be any need to
+    directly instantiate instances of this class inside resources themselves
+    """
+    def __init__(self, context):
+        self.context = context
+        kwargs = {
+            'auth_url': context.auth_url,
+        }
+
+        if context.password is not None:
+            kwargs['username'] = context.username
+            kwargs['password'] = context.password
+            kwargs['tenant_name'] = context.tenant
+            kwargs['tenant_id'] = context.tenant_id
+        elif context.auth_token is not None:
+            kwargs['username'] = context.service_user
+            kwargs['password'] = context.service_password
+            kwargs['tenant_name'] = context.service_tenant
+            kwargs['token'] = context.auth_token
+        else:
+            logger.error("Keystone connection failed, no password or " +
+                         "auth_token!")
+            return
+        self.client = kc.Client(**kwargs)
+        self.client.authenticate()
+
+    def create_stack_user(self, username, password=''):
+        """
+        Create a user defined as part of a stack, either via template
+        or created internally by a resource.  This user will be added to
+        the heat_stack_user_role as defined in the config
+        Returns the keystone ID of the resulting user
+        """
+        user = self.client.users.create(username,
+                                        password,
+                                        '%s@heat-api.org' %
+                                        username,
+                                        tenant_id=self.context.tenant_id,
+                                        enabled=True)
+
+        # We add the new user to a special keystone role
+        # This role is designed to allow easier differentiation of the
+        # heat-generated "stack users" which will generally have credentials
+        # deployed on an instance (hence are implicitly untrusted)
+        roles = self.client.roles.list()
+        stack_user_role = [r.id for r in roles
+                         if r.name == cfg.CONF.heat_stack_user_role]
+        if len(stack_user_role) == 1:
+            role_id = stack_user_role[0]
+            logger.debug("Adding user %s to role %s" % (user.id, role_id))
+            self.client.roles.add_user_role(user.id, role_id,
+                                            self.context.tenant_id)
+        else:
+            logger.error("Failed to add user %s to role %s, check role exists!"
+                         % (username,
+                            cfg.CONF.heat_stack_user_role))
+
+        return user.id
+
+    def get_user_by_name(self, username):
+        """
+        Return the ID for the specified username
+        """
+        users = self.client.users.list(tenant_id=self.context.tenant_id)
+        for u in users:
+            if u.name == username:
+                return u.id
+        return None
+
+    def delete_stack_user(self, user_id):
+
+        user = self.client.users.get(user_id)
+
+        # FIXME (shardy) : need to test, do we still need this retry logic?
+        # Copied from user.py, but seems like something we really shouldn't
+        # need to do, no bug reference in the original comment (below)...
+        # tempory hack to work around an openstack bug.
+        # seems you can't delete a user first time - you have to try
+        # a couple of times - go figure!
+        tmo = eventlet.Timeout(10)
+        status = 'WAITING'
+        reason = 'Timed out trying to delete user'
+        try:
+            while status == 'WAITING':
+                try:
+                    user.delete()
+                    status = 'DELETED'
+                except Exception as ce:
+                    reason = str(ce)
+                    logger.warning("Problem deleting user %s: %s" %
+                                   (user_id, reason))
+                    eventlet.sleep(1)
+        except eventlet.Timeout as t:
+            if t is not tmo:
+                # not my timeout
+                raise
+            else:
+                status = 'TIMEDOUT'
+        finally:
+            tmo.cancel()
+
+        if status != 'DELETED':
+            raise exception.Error(reason)
+
+    def delete_ec2_keypair(self, user_id, accesskey):
+        self.client.ec2.delete(user_id, accesskey)
+
+    def get_ec2_keypair(self, user_id):
+        # Here we use the user_id of the user context of the request.  We need
+        # to avoid using users.list because it needs keystone admin role, and
+        # we want to allow an instance user to retrieve data about itself:
+        # - Users without admin role cannot create or delete, but they
+        #   can see their own secret key (but nobody elses)
+        # - Users with admin role can create/delete and view the
+        #   private keys of all users in their tenant
+        # This will allow "instance users" to retrieve resource
+        # metadata but not manipulate user resources in any other way
+        user_id = self.client.auth_user_id
+        cred = self.client.ec2.list(user_id)
+        # We make the assumption that each user will only have one
+        # ec2 keypair, it's not clear if AWS allow multiple AccessKey resources
+        # to be associated with a single User resource, but for simplicity
+        # we assume that here for now
+        if len(cred) == 0:
+            return self.client.ec2.create(user_id, self.context.tenant_id)
+        if len(cred) == 1:
+            return cred[0]
+        else:
+            logger.error("Unexpected number of ec2 credentials %s for %s" %
+                         (len(cred), user_id))

heat/engine/clients.py

@@ -15,7 +15,6 @@
 
 
 from novaclient.v1_1 import client as nc
-from keystoneclient.v2_0 import client as kc
 
 # swiftclient not available in all distributions - make s3 an optional
 # feature
@@ -32,6 +31,8 @@ try:
 except ImportError:
     quantumclient_present = False
 
+from heat.common import heat_keystoneclient as kc
+from heat.openstack.common import cfg
 from heat.openstack.common import log as logging
 
 logger = logging.getLogger('heat.engine.clients')
@@ -53,29 +54,7 @@ class Clients(object):
         if self._keystone:
             return self._keystone
 
-        con = self.context
-        args = {
-            'auth_url': con.auth_url,
-        }
-
-        if con.password is not None:
-            args['username'] = con.username
-            args['password'] = con.password
-            args['tenant_name'] = con.tenant
-            args['tenant_id'] = con.tenant_id
-        elif con.auth_token is not None:
-            args['username'] = con.service_user
-            args['password'] = con.service_password
-            args['tenant_name'] = con.service_tenant
-            args['token'] = con.auth_token
-        else:
-            logger.error("Keystone connection failed, no password or " +
-                         "auth_token!")
-            return None
-
-        client = kc.Client(**args)
-        client.authenticate()
-        self._keystone = client
+        self._keystone = kc.KeystoneClient(self.context)
         return self._keystone
 
     def nova(self, service_type='compute'):

heat/engine/resources/user.py

@@ -13,7 +13,6 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-import eventlet
 from heat.common import exception
 from heat.openstack.common import cfg
 from heat.engine import resource
@@ -29,14 +28,6 @@ logger = logging.getLogger('heat.engine.user')
 #
 
 
-class DummyId:
-    def __init__(self, id):
-        self.id = id
-
-    def __eq__(self, other):
-        return self.id == other.id
-
-
 class User(resource.Resource):
     properties_schema = {'Path': {'Type': 'String'},
                          'Groups': {'Type': 'List'},
@@ -55,69 +46,18 @@ class User(resource.Resource):
             'Password' in self.properties['LoginProfile']:
             passwd = self.properties['LoginProfile']['Password']
 
-        tenant_id = self.context.tenant_id
-        user = self.keystone().users.create(self.physical_resource_name(),
-                                            passwd,
-                                            '%s@heat-api.org' %
-                                            self.physical_resource_name(),
-                                            tenant_id=tenant_id,
-                                            enabled=True)
-        self.resource_id_set(user.id)
-
-        # We add the new user to a special keystone role
-        # This role is designed to allow easier differentiation of the
-        # heat-generated "stack users" which will generally have credentials
-        # deployed on an instance (hence are implicitly untrusted)
-        roles = self.keystone().roles.list()
-        stack_user_role = [r.id for r in roles
-                         if r.name == cfg.CONF.heat_stack_user_role]
-        if len(stack_user_role) == 1:
-            role_id = stack_user_role[0]
-            logger.debug("Adding user %s to role %s" % (user.id, role_id))
-            self.keystone().roles.add_user_role(user.id, role_id, tenant_id)
-        else:
-            logger.error("Failed to add user %s to role %s, check role exists!"
-                         % (self.physical_resource_name(),
-                            cfg.CONF.heat_stack_user_role))
+        uid = self.keystone().create_stack_user(self.physical_resource_name(),
+                                                 passwd)
+        self.resource_id_set(uid)
 
     def handle_update(self):
         return self.UPDATE_REPLACE
 
     def handle_delete(self):
         if self.resource_id is None:
+            logger.error("Cannot delete User resource before user created!")
             return
-        try:
-            user = self.keystone().users.get(DummyId(self.resource_id))
-        except Exception as ex:
-            logger.info('user %s/%s does not exist' %
-                        (self.physical_resource_name(), self.resource_id))
-            return
-
-        # tempory hack to work around an openstack bug.
-        # seems you can't delete a user first time - you have to try
-        # a couple of times - go figure!
-        tmo = eventlet.Timeout(10)
-        status = 'WAITING'
-        reason = 'Timed out trying to delete user'
-        try:
-            while status == 'WAITING':
-                try:
-                    user.delete()
-                    status = 'DELETED'
-                except Exception as ce:
-                    reason = str(ce)
-                    eventlet.sleep(1)
-        except eventlet.Timeout as t:
-            if t is not tmo:
-                # not my timeout
-                raise
-            else:
-                status = 'TIMEDOUT'
-        finally:
-            tmo.cancel()
-
-        if status != 'DELETED':
-            raise exception.Error(reason)
+        self.keystone().delete_stack_user(self.resource_id)
 
     def FnGetRefId(self):
         return unicode(self.physical_resource_name())
@@ -141,58 +81,53 @@ class AccessKey(resource.Resource):
         super(AccessKey, self).__init__(name, json_snippet, stack)
         self._secret = None
 
-    def _user_from_name(self, username):
-        tenant_id = self.context.tenant_id
-        users = self.keystone().users.list(tenant_id=tenant_id)
-        for u in users:
-            if u.name == username:
-                return u
-        return None
-
     def handle_create(self):
         username = self.properties['UserName']
-        user = self._user_from_name(username)
-        if user is None:
+        user_id = self.keystone().get_user_by_name(username)
+        if user_id is None:
             raise exception.NotFound('could not find user %s' %
                                      username)
 
-        tenant_id = self.context.tenant_id
-        cred = self.keystone().ec2.create(user.id, tenant_id)
-        self.resource_id_set(cred.access)
-        self._secret = cred.secret
+        kp = self.keystone().get_ec2_keypair(user_id)
+        if not kp:
+            raise exception.Error("Error creating ec2 keypair for user %s" %
+                                  user_id)
+        else:
+            self.resource_id_set(kp.access)
+            self._secret = kp.secret
 
     def handle_update(self):
         return self.UPDATE_REPLACE
 
     def handle_delete(self):
-        user = self._user_from_name(self.properties['UserName'])
-        if user and self.resource_id:
-            self.keystone().ec2.delete(user.id, self.resource_id)
+        self.resource_id_set(None)
+        self._secret = None
+        user_id = self.keystone().get_user_by_name(self.properties['UserName'])
+        if user_id and self.resource_id:
+            self.keystone().delete_ec2_keypair(user_id, self.resource_id)
 
     def _secret_accesskey(self):
         '''
         Return the user's access key, fetching it from keystone if necessary
         '''
+        user_id = self.keystone().get_user_by_name(self.properties['UserName'])
         if self._secret is None:
-            try:
-                # Here we use the user_id of the user context of the request
-                # We need to avoid using _user_from_name, because users.list
-                # needs keystone admin role, and we want to allow an instance
-                # user to retrieve data about itself:
-                # - Users without admin role cannot create or delete, but they
-                #   can see their own secret key (but nobody elses)
-                # - Users with admin role can create/delete and view the
-                #   private keys of all users in their tenant
-                # This will allow "instance users" to retrieve resource
-                # metadata but not manipulate user resources in any other way
-                user_id = self.keystone().auth_user_id
-                cred = self.keystone().ec2.get(user_id, self.resource_id)
-                self._secret = cred.secret
-                self.resource_id_set(cred.access)
-            except Exception as ex:
+            if not self.resource_id:
                 logger.warn('could not get secret for %s Error:%s' %
                             (self.properties['UserName'],
-                             str(ex)))
+                            "resource_id not yet set"))
+            else:
+                try:
+                    kp = self.keystone().get_ec2_keypair(user_id)
+                except Exception as ex:
+                    logger.warn('could not get secret for %s Error:%s' %
+                                (self.properties['UserName'],
+                                 str(ex)))
+                if kp.access == self.resource_id:
+                    self._secret = kp.secret
+                else:
+                    logger.error("Unexpected ec2 keypair, for %s access %s" %
+                                 (user_id, kp.access))
 
         return self._secret or '000-000-000'
 

heat/tests/fakes.py

@@ -84,3 +84,39 @@ class FakeClient(object):
 
     def authenticate(self):
         pass
+
+
+class FakeKeystoneClient():
+    def __init__(self, username='test_user', user_id='1234', access='4567',
+                 secret='8901'):
+        self.username = username
+        self.user_id = user_id
+        self.access = access
+        self.secret = secret
+        self.creds = None
+
+    def create_stack_user(self, username, password=''):
+        self.username = username
+        return self.user_id
+
+    def delete_stack_user(self, user_id):
+        self.user_id = None
+
+    def get_user_by_name(self, username):
+        if username == self.username:
+            return self.user_id
+
+    def get_ec2_keypair(self, user_id):
+        if user_id == self.user_id:
+            if not self.creds:
+                class FakeCred:
+                    access = self.access
+                    secret = self.secret
+                self.creds = FakeCred()
+            return self.creds
+
+    def delete_ec2_keypair(self, user_id, access):
+        if user_id == self.user_id and access == self.creds.access:
+            self.creds = None
+        else:
+            raise Exception('Incorrect user_id or access')

heat/tests/test_user.py

@@ -30,10 +30,7 @@ from heat.common import config
 from heat.engine import format
 from heat.engine import parser
 from heat.engine.resources import user
-from heat.tests.v1_1 import fakes
-from keystoneclient.v2_0 import users
-from keystoneclient.v2_0 import roles
-from keystoneclient.v2_0 import ec2
+from heat.tests import fakes
 from heat.openstack.common import cfg
 
 
@@ -42,22 +39,7 @@ from heat.openstack.common import cfg
 class UserTest(unittest.TestCase):
     def setUp(self):
         self.m = mox.Mox()
-        self.fc = fakes.FakeClient()
-        self.fc.users = users.UserManager(None)
-        self.fc.roles = roles.RoleManager(None)
-        self.fc.ec2 = ec2.CredentialsManager(None)
-        self.m.StubOutWithMock(user.User, 'keystone')
-        self.m.StubOutWithMock(user.AccessKey, 'keystone')
-        self.m.StubOutWithMock(self.fc.users, 'create')
-        self.m.StubOutWithMock(self.fc.users, 'get')
-        self.m.StubOutWithMock(self.fc.users, 'delete')
-        self.m.StubOutWithMock(self.fc.users, 'list')
-        self.m.StubOutWithMock(self.fc.roles, 'list')
-        self.m.StubOutWithMock(self.fc.roles, 'add_user_role')
-        self.m.StubOutWithMock(self.fc.ec2, 'create')
-        self.m.StubOutWithMock(self.fc.ec2, 'get')
-        self.m.StubOutWithMock(self.fc.ec2, 'delete')
-        self.m.StubOutWithMock(eventlet, 'sleep')
+        self.fc = fakes.FakeKeystoneClient(username='test_stack.CfnUser')
         config.register_engine_opts()
         cfg.CONF.set_default('heat_stack_user_role', 'stack_user_role')
 
@@ -112,32 +94,8 @@ class UserTest(unittest.TestCase):
 
     def test_user(self):
 
-        fake_user = users.User(self.fc.users, {'id': '1'})
-        user.User.keystone().AndReturn(self.fc)
-        self.fc.users.create('test_stack.CfnUser',
-                             '',
-                             'test_stack.CfnUser@heat-api.org',
-                             enabled=True,
-                             tenant_id='test_tenant').AndReturn(fake_user)
-
-        fake_role = roles.Role(self.fc.roles, {'id': '123',
-                                               'name': 'stack_user_role'})
-        user.User.keystone().AndReturn(self.fc)
-        self.fc.roles.list().AndReturn([fake_role])
-
-        user.User.keystone().AndReturn(self.fc)
-        self.fc.roles.add_user_role('1', '123', 'test_tenant').AndReturn(None)
-
-        # delete script
-        user.User.keystone().AndReturn(self.fc)
-        self.fc.users.get(user.DummyId('1')).AndRaise(Exception('not found'))
-        eventlet.sleep(1).AndReturn(None)
-
-        user.User.keystone().AndReturn(self.fc)
-        self.fc.users.get(user.DummyId('1')).AndReturn(fake_user)
-        self.fc.users.delete(fake_user).AndRaise(Exception('delete failed'))
-
-        self.fc.users.delete(fake_user).AndReturn(None)
+        self.m.StubOutWithMock(user.User, 'keystone')
+        user.User.keystone().MultipleTimes().AndReturn(self.fc)
 
         self.m.ReplayAll()
 
@@ -145,7 +103,7 @@ class UserTest(unittest.TestCase):
         stack = self.parse_stack(t)
 
         resource = self.create_user(t, stack, 'CfnUser')
-        self.assertEqual('1', resource.resource_id)
+        self.assertEqual(self.fc.user_id, resource.resource_id)
         self.assertEqual('test_stack.CfnUser', resource.FnGetRefId())
 
         self.assertEqual('CREATE_COMPLETE', resource.state)
@@ -156,7 +114,7 @@ class UserTest(unittest.TestCase):
         self.assertEqual(None, resource.delete())
         self.assertEqual('DELETE_COMPLETE', resource.state)
 
-        resource.resource_id = '1'
+        resource.resource_id = self.fc.access
         resource.state_set('CREATE_COMPLETE')
         self.assertEqual('CREATE_COMPLETE', resource.state)
 
@@ -172,31 +130,8 @@ class UserTest(unittest.TestCase):
 
     def test_access_key(self):
 
-        fake_user = users.User(self.fc.users, {'id': '1',
-                                               'name': 'test_stack.CfnUser'})
-        fake_cred = ec2.EC2(self.fc.ec2, {
-                        'access': '03a4967889d94a9c8f707d267c127a3d',
-                        'secret': 'd5fd0c08f8cc417ead0355c67c529438'})
-
-        user.AccessKey.keystone().AndReturn(self.fc)
-        self.fc.users.list(tenant_id='test_tenant').AndReturn([fake_user])
-
-        user.AccessKey.keystone().AndReturn(self.fc)
-        self.fc.ec2.create('1', 'test_tenant').AndReturn(fake_cred)
-
-        # fetch secret key
-        user.AccessKey.keystone().AndReturn(self.fc)
-        self.fc.auth_user_id = '1'
-        user.AccessKey.keystone().AndReturn(self.fc)
-        self.fc.ec2.get('1',
-                '03a4967889d94a9c8f707d267c127a3d').AndReturn(fake_cred)
-
-        # delete script
-        user.AccessKey.keystone().AndReturn(self.fc)
-        self.fc.users.list(tenant_id='test_tenant').AndReturn([fake_user])
-        user.AccessKey.keystone().AndReturn(self.fc)
-        self.fc.ec2.delete('1',
-                           '03a4967889d94a9c8f707d267c127a3d').AndReturn(None)
+        self.m.StubOutWithMock(user.AccessKey, 'keystone')
+        user.AccessKey.keystone().MultipleTimes().AndReturn(self.fc)
 
         self.m.ReplayAll()
 
@@ -207,16 +142,16 @@ class UserTest(unittest.TestCase):
 
         self.assertEqual(user.AccessKey.UPDATE_REPLACE,
                   resource.handle_update())
-        self.assertEqual('03a4967889d94a9c8f707d267c127a3d',
+        self.assertEqual(self.fc.access,
                          resource.resource_id)
 
-        self.assertEqual('d5fd0c08f8cc417ead0355c67c529438',
+        self.assertEqual(self.fc.secret,
                          resource._secret)
 
         self.assertEqual(resource.FnGetAtt('UserName'), 'test_stack.CfnUser')
         resource._secret = None
         self.assertEqual(resource.FnGetAtt('SecretAccessKey'),
-                         'd5fd0c08f8cc417ead0355c67c529438')
+                         self.fc.secret)
         try:
             resource.FnGetAtt('Foo')
         except exception.InvalidTemplateAttribute:
@@ -229,18 +164,20 @@ class UserTest(unittest.TestCase):
 
     def test_access_key_no_user(self):
 
-        user.AccessKey.keystone().AndReturn(self.fc)
-        self.fc.users.list(tenant_id='test_tenant').AndReturn([])
+        self.m.StubOutWithMock(user.AccessKey, 'keystone')
+        user.AccessKey.keystone().MultipleTimes().AndReturn(self.fc)
 
         self.m.ReplayAll()
 
         t = self.load_template()
         stack = self.parse_stack(t)
 
+        # Set the resource properties to an unknown user
+        t['Resources']['HostKeys']['Properties']['UserName'] = 'NoExist'
         resource = user.AccessKey('HostKeys',
-                                      t['Resources']['HostKeys'],
-                                      stack)
-        self.assertEqual('could not find user test_stack.CfnUser',
+                                  t['Resources']['HostKeys'],
+                                  stack)
+        self.assertEqual('could not find user NoExist',
                          resource.create())
         self.assertEqual(user.AccessKey.CREATE_FAILED,
                          resource.state)