Handle ambiguous physical resource IDs

It's possible that we could end up with multiple resources with the same
physical resource ID, but that would be undetectable since we return only
one from the database layer. This change allows us to detect the problem an
return an error where the result is rendered ambiguous.

Change-Id: I2c5ddbe6731c33a09ec7c4a7b91dcfe414da4385

heat/api/aws/exception.py

@@ -311,6 +311,7 @@ def map_remote_error(ex):
         'PropertyUnspecifiedError',
         'NotSupported',
         'InvalidBreakPointHook',
+        'PhysicalResourceIDAmbiguity',
     )
     denied_errors = ('Forbidden', 'NotAuthorized')
     already_exists_errors = ('StackExists')

heat/api/middleware/fault.py

@@ -63,6 +63,7 @@ class FaultWrapper(wsgi.Middleware):
         'InvalidGlobalResource': webob.exc.HTTPInternalServerError,
         'ResourceNotAvailable': webob.exc.HTTPNotFound,
         'PhysicalResourceNameAmbiguity': webob.exc.HTTPBadRequest,
+        'PhysicalResourceIDAmbiguity': webob.exc.HTTPBadRequest,
         'InvalidTenant': webob.exc.HTTPForbidden,
         'Forbidden': webob.exc.HTTPForbidden,
         'StackExists': webob.exc.HTTPConflict,

heat/common/exception.py

@@ -177,6 +177,11 @@ class PhysicalResourceNameAmbiguity(HeatException):
         "Multiple physical resources were found with name (%(name)s).")
 
 
+class PhysicalResourceIDAmbiguity(HeatException):
+    msg_fmt = _(
+        "Multiple resources were found with the physical ID (%(phys_id)s).")
+
+
 class InvalidTenant(HeatException):
     msg_fmt = _("Searching Tenant %(target)s "
                 "from Tenant %(actual)s forbidden.")

heat/db/api.py

@@ -160,6 +160,11 @@ def resource_get_by_physical_resource_id(context, physical_resource_id):
                                                      physical_resource_id)
 
 
+def resource_get_all_by_physical_resource_id(context, physical_resource_id):
+    return IMPL.resource_get_all_by_physical_resource_id(context,
+                                                         physical_resource_id)
+
+
 def stack_get(context, stack_id, show_deleted=False, eager_load=True):
     return IMPL.stack_get(context, stack_id, show_deleted=show_deleted,
                           eager_load=eager_load)

heat/db/sqlalchemy/api.py

@@ -204,7 +204,7 @@ def resource_get_by_name_and_stack(context, resource_name, stack_id):
     return result
 
 
-def resource_get_by_physical_resource_id(context, physical_resource_id):
+def resource_get_all_by_physical_resource_id(context, physical_resource_id):
     results = (context.session.query(models.Resource)
                .filter_by(physical_resource_id=physical_resource_id)
                .all())
@@ -212,8 +212,16 @@ def resource_get_by_physical_resource_id(context, physical_resource_id):
     for result in results:
         if context is None or context.tenant_id in (
                 result.stack.tenant, result.stack.stack_user_project_id):
-            return result
-    return None
+            yield result
+
+
+def resource_get_by_physical_resource_id(context, physical_resource_id):
+    results = resource_get_all_by_physical_resource_id(context,
+                                                       physical_resource_id)
+    try:
+        return next(results)
+    except StopIteration:
+        return None
 
 
 def resource_get_all(context):

heat/engine/service.py

@@ -1907,13 +1907,21 @@ class EngineService(service.ServiceBase):
         :param cnxt: RPC context.
         :param physical_resource_id: The physical resource ID to look up.
         """
-        rs = resource_objects.Resource.get_by_physical_resource_id(
+        rsrcs = resource_objects.Resource.get_all_by_physical_resource_id(
             cnxt,
             physical_resource_id)
-        if not rs:
+
+        if not rsrcs:
             raise exception.EntityNotFound(entity='Resource',
                                            name=physical_resource_id)
+        # This call is used only in the cfn API, which only cares about
+        # finding the stack anyway. So allow duplicate resource IDs within the
+        # same stack.
+        if len({rs.stack_id for rs in rsrcs}) > 1:
+            raise exception.PhysicalResourceIDAmbiguity(
+                phys_id=physical_resource_id)
 
+        rs = rsrcs[0]
         stack = parser.Stack.load(cnxt, stack_id=rs.stack_id)
         resource = stack[rs.name]
 

heat/objects/resource.py

@@ -202,11 +202,12 @@ class Resource(
         return cls._from_db_object(cls(context), context, resource_db)
 
     @classmethod
-    def get_by_physical_resource_id(cls, context, physical_resource_id):
-        resource_db = db_api.resource_get_by_physical_resource_id(
+    def get_all_by_physical_resource_id(cls, context, physical_resource_id):
+        matches = db_api.resource_get_all_by_physical_resource_id(
             context,
             physical_resource_id)
-        return cls._from_db_object(cls(context), context, resource_db)
+        return [cls._from_db_object(cls(context), context, resource_db)
+                for resource_db in matches]
 
     @classmethod
     def update_by_id(cls, context, resource_id, values):

heat/tests/db/test_sqlalchemy_api.py

@@ -2318,6 +2318,20 @@ class DBAPIResourceTest(common.HeatTestCase):
         self.assertIsNone(db_api.resource_get_by_physical_resource_id(self.ctx,
                                                                       UUID2))
 
+    def test_resource_get_all_by_physical_resource_id(self):
+        create_resource(self.ctx, self.stack)
+        create_resource(self.ctx, self.stack)
+
+        ret_res = db_api.resource_get_all_by_physical_resource_id(self.ctx,
+                                                                  UUID1)
+        ret_list = list(ret_res)
+        self.assertTrue(ret_list)
+        for res in ret_list:
+            self.assertEqual(UUID1, res.physical_resource_id)
+
+        mt = db_api.resource_get_all_by_physical_resource_id(self.ctx, UUID2)
+        self.assertFalse(list(mt))
+
     def test_resource_get_all(self):
         values = [
             {'name': 'res1'},

heat/tests/engine/service/test_software_config.py

@@ -223,10 +223,10 @@ class SoftwareConfigServiceTest(common.HeatTestCase):
             self.ctx, server_id=server.resource_id)
         self.assertEqual([deployment], deployments)
 
-        rs = resource_objects.Resource.get_by_physical_resource_id(
+        rsrcs = resource_objects.Resource.get_all_by_physical_resource_id(
             self.ctx, server_id)
         self.assertEqual(deployment['config_id'],
-                         rs.rsrc_metadata.get('deployments')[0]['id'])
+                         rsrcs[0].rsrc_metadata.get('deployments')[0]['id'])
 
     def test_metadata_software_deployments(self):
         stack_name = 'test_metadata_software_deployments'
@@ -271,9 +271,9 @@ class SoftwareConfigServiceTest(common.HeatTestCase):
 
         # assert that metadata via metadata_software_deployments matches
         # metadata via server resource
-        rs = resource_objects.Resource.get_by_physical_resource_id(
+        rsrcs = resource_objects.Resource.get_all_by_physical_resource_id(
             self.ctx, server_id)
-        self.assertEqual(metadata, rs.rsrc_metadata.get('deployments'))
+        self.assertEqual(metadata, rsrcs[0].rsrc_metadata.get('deployments'))
 
         deployments = self.engine.metadata_software_deployments(
             self.ctx, server_id=str(uuid.uuid4()))