Check for circular dependencies in template validation

We ought to iterate over the dependency list in the validate_template call,
to check for any circular dependencies. This check was removed by
d12cbe7959 in order to avoid trying to
calculate implicit dependencies added by resource plugins (as e.g. many
Neutron resources do) at a time when we don't have sufficient data to do a
good job of it anyway.

However, the previous patch d12cbe7959 split
apart the calculation of explicit and implicit dependencies, so we can
easily do this while still checking for circular references in the
explicit dependencies. This patch splits the dependency calculation into
two phases - the first using only data available from the template, and the
second querying the resources for implicit dependencies.

Use strict_validate = False as the criterion for when to avoid calculating
implicit dependencies, since this corresponds exactly to when we don't have
enough data to do so.

Change-Id: I21c63dcc8d1cad20dbc237b472670753779b0ff8
Closes-bug: #1691575

heat/engine/service.py

@@ -1245,7 +1245,6 @@ class EngineService(service.ServiceBase):
                              service_check_defer=service_check_defer)
         try:
             stack.validate(ignorable_errors=ignorable_errors,
-                           validate_by_deps=False,
                            validate_res_tmpl_only=True)
         except exception.StackValidationFailed as ex:
             return {'Error': six.text_type(ex)}

heat/engine/stack.py

@@ -163,6 +163,7 @@ class Stack(collections.Mapping):
         self._outputs = None
         self._resources = None
         self._dependencies = None
+        self._implicit_deps_loaded = False
         self._access_allowed_handlers = {}
         self._db_resources = None
         self._tags = tags
@@ -409,12 +410,15 @@ class Stack(collections.Mapping):
 
     @property
     def dependencies(self):
-        if self._dependencies is None:
-            self._dependencies = self._get_dependencies(
-                ignore_errors=self.id is not None)
+        if not self._implicit_deps_loaded:
+            self._explicit_dependencies()
+            self._add_implicit_dependencies(self._dependencies,
+                                            ignore_errors=self.id is not None)
+            self._implicit_deps_loaded = True
         return self._dependencies
 
     def reset_dependencies(self):
+        self._implicit_deps_loaded = False
         self._dependencies = None
 
     def root_stack_id(self):
@@ -486,12 +490,23 @@ class Stack(collections.Mapping):
         return set(itertools.chain.from_iterable(
             res.dep_attrs(resource_name) for res in resources))
 
-    def _get_dependencies(self, ignore_errors=True):
-        """Return the dependency graph for a list of resources."""
-        deps = dependencies.Dependencies()
-        for res in six.itervalues(self.resources):
-            res.add_explicit_dependencies(deps)
+    def _explicit_dependencies(self):
+        """Return dependencies without making any resource plugin calls.
 
+        This includes at least all of the dependencies that are explicitly
+        expressed in the template (via depends_on or an intrinsic function). It
+        may include implicit dependencies defined by resource plugins, but only
+        if they have already been calculated.
+        """
+        if self._dependencies is None:
+            deps = dependencies.Dependencies()
+            for res in six.itervalues(self.resources):
+                res.add_explicit_dependencies(deps)
+            self._dependencies = deps
+        return self._dependencies
+
+    def _add_implicit_dependencies(self, deps, ignore_errors=True):
+        """Augment the given dependencies with implicit ones from plugins."""
         for res in six.itervalues(self.resources):
             try:
                 res.add_dependencies(deps)
@@ -504,8 +519,6 @@ class Stack(collections.Mapping):
                                 {'res': six.text_type(res),
                                  'err': six.text_type(exc)})
 
-        return deps
-
     @classmethod
     def load(cls, context, stack_id=None, stack=None, show_deleted=True,
              use_stored_context=False, force_reload=False, cache_data=None,
@@ -801,8 +814,7 @@ class Stack(collections.Mapping):
         return handler and handler(resource_name)
 
     @profiler.trace('Stack.validate', hide_args=False)
-    def validate(self, ignorable_errors=None, validate_by_deps=True,
-                 validate_res_tmpl_only=False):
+    def validate(self, ignorable_errors=None, validate_res_tmpl_only=False):
         """Validates the stack."""
         # TODO(sdake) Should return line number of invalid reference
 
@@ -844,10 +856,10 @@ class Stack(collections.Mapping):
             raise exception.StackValidationFailed(
                 message=_("Duplicate names %s") % dup_names)
 
-        if validate_by_deps:
+        if self.strict_validate:
             iter_rsc = self.dependencies
         else:
-            iter_rsc = six.itervalues(resources)
+            iter_rsc = self._explicit_dependencies()
 
         unique_defns = set(res.t for res in six.itervalues(resources))
         unique_defn_names = set(defn.name for defn in unique_defns)

heat/tests/test_validate.py

@@ -21,6 +21,7 @@ from heat.common.i18n import _
 from heat.common import template_format
 from heat.common import urlfetch
 from heat.engine.clients.os import glance
+from heat.engine import dependencies
 from heat.engine import environment
 from heat.engine.hot import template as hot_tmpl
 from heat.engine import resources
@@ -898,6 +899,21 @@ outputs:
     value: {get_attr: [[random_str, value]]}
 '''
 
+test_template_circular_reference = '''
+heat_template_version: 2013-05-23
+
+resources:
+  res1:
+    type: OS::Heat::None
+    depends_on: res3
+  res2:
+    type: OS::Heat::None
+    depends_on: res1
+  res3:
+    type: OS::Heat::None
+    depends_on: res2
+'''
+
 
 test_template_external_rsrc = '''
 heat_template_version: pike
@@ -1834,3 +1850,12 @@ parameter_groups:
             return_value={'id': 'foobar'}
         ):
             self.assertIsNone(stack.validate(validate_res_tmpl_only=False))
+
+    def test_validate_circular_reference(self):
+        t = template_format.parse(test_template_circular_reference)
+
+        exc = self.assertRaises(dispatcher.ExpectedException,
+                                self.engine.validate_template,
+                                self.ctx, t, {})
+        self.assertEqual(dependencies.CircularDependencyException,
+                         exc.exc_info[0])