hooks: treat lookup dependencies specially to handle destroy

When running the destroy action, we reverse the order of execution
compared to build, such that steps that required others are now
required by them instead.

Unfortunately that is not enough to express the execution requirements
of hooks: they might be `required_by` a stack to run some cleanup before
destroying that stack, but still need it to be deployed to lookup
outputs
from.

Handle that by manually checking for the dependencies of lookups before
executing hooks, skipping them if a required stack is not deployed (or
has already been destroyed).

Author: danielkza

Diff for: stacker/actions/base.py

@@ -13,10 +13,11 @@
 
 import botocore.exceptions
 from stacker.session_cache import get_session
-from stacker.exceptions import HookExecutionFailed, PlanFailed
-from stacker.status import COMPLETE, SKIPPED, FailedStatus
+from stacker.exceptions import PlanFailed
+from stacker.status import COMPLETE
 from stacker.util import ensure_s3_bucket, get_s3_endpoint
 
+
 logger = logging.getLogger(__name__)
 
 # After submitting a stack update/create, this controls how long we'll wait
@@ -142,18 +143,8 @@ def target_fn(*args, **kwargs):
             return COMPLETE
 
         def hook_fn(hook, *args, **kwargs):
-            provider = self.provider_builder.build(profile=hook.profile,
-                                                   region=hook.region)
-
-            try:
-                result = hook.run(provider, self.context)
-            except HookExecutionFailed as e:
-                return FailedStatus(reason=str(e))
-
-            if result is None:
-                return SKIPPED
-
-            return COMPLETE
+            return hook.run_step(provider_builder=self.provider_builder,
+                                 context=self.context)
 
         pre_hooks_target = Target(
             name="pre_{}_hooks".format(action_name))

Diff for: stacker/exceptions.py

@@ -126,6 +126,7 @@ def __init__(self, stack_name, *args, **kwargs):
         message = ("Stack: \"%s\" does not exist in outputs or the lookup is "
                    "not available in this stacker run") % (stack_name,)
         super(StackDoesNotExist, self).__init__(message, *args, **kwargs)
+        self.stack_name = stack_name
 
 
 class MissingParameterException(Exception):
@@ -278,14 +279,14 @@ def __init__(self, exception, stack, dependency):
 class HookExecutionFailed(Exception):
     """Raised when running a required hook fails"""
 
-    def __init__(self, hook, result=None, exception=None):
+    def __init__(self, hook, result=None, cause=None):
         self.hook = hook
         self.result = result
-        self.exception = exception
+        self.cause = cause
 
-        if self.exception:
+        if self.cause:
             message = ("Hook '{}' threw exception: {}".format(
-                hook.name, exception))
+                hook.name, cause))
         else:
             message = ("Hook '{}' failed (result: {})".format(
                 hook.name, result))

Diff for: stacker/hooks/__init__.py

@@ -5,8 +5,11 @@
 import logging
 from collections import Mapping, namedtuple
 
-from stacker.exceptions import HookExecutionFailed
+from stacker.exceptions import HookExecutionFailed, StackDoesNotExist
 from stacker.util import load_object_from_string
+from stacker.status import (
+    COMPLETE, SKIPPED, FailedStatus, NotSubmittedStatus, SkippedStatus
+)
 from stacker.variables import Variable
 
 logger = logging.getLogger(__name__)
@@ -54,11 +57,55 @@ def __init__(self, name, path, required=True, enabled=True,
         self.region = region
 
         self._args = {}
+        self._args, deps = self.parse_args(args)
+        self.requires.update(deps)
+
+        self._callable = self.resolve_path()
+
+    def parse_args(self, args):
+        arg_vars = {}
+        deps = set()
+
         if args:
             for key, value in args.items():
-                var = self._args[key] = \
+                var = arg_vars[key] = \
                     Variable('{}.args.{}'.format(self.name, key), value)
-                self.requires.update(var.dependencies())
+                deps.update(var.dependencies())
+
+        return arg_vars, deps
+
+    def resolve_path(self):
+        try:
+            return load_object_from_string(self.path)
+        except (AttributeError, ImportError) as e:
+            raise ValueError("Unable to load method at %s for hook %s: %s",
+                             self.path, self.name, str(e))
+
+    def check_args_dependencies(self, provider, context):
+        # When running hooks for destruction, we might rely on outputs of
+        # stacks that we assume have been deployed. Unfortunately, since
+        # destruction must happen in the reverse order of creation, those stack
+        # dependencies will not be present on `requires`, but in `required_by`,
+        # meaning the execution engine won't stop the hook from running early.
+
+        # To deal with that, manually find the dependencies coming from
+        # lookups in the hook arguments, select those that represent stacks,
+        # and check if they are actually available.
+
+        dependencies = set()
+        for value in self._args.values():
+            dependencies.update(value.dependencies())
+
+        for dep in dependencies:
+            # We assume all dependency names are valid here. Hence, if we can't
+            # find a stack with that same name, it must be a target or a hook,
+            # and hence we don't need to check it
+            stack = context.get_stack(dep)
+            if stack is None:
+                continue
+
+            # This will raise if the stack is missing
+            provider.get_stack(stack.fqn)
 
     def resolve_args(self, provider, context):
         for key, value in self._args.items():
@@ -85,29 +132,15 @@ def run(self, provider, context):
         """
 
         logger.info("Executing hook %s", self)
-
-        if not self.enabled:
-            logger.debug("Hook %s is disabled, skipping", self.name)
-            return
-
-        try:
-            method = load_object_from_string(self.path)
-        except (AttributeError, ImportError) as e:
-            logger.exception("Unable to load method at %s for hook %s:",
-                             self.path, self.name)
-            if self.required:
-                raise HookExecutionFailed(self, exception=e)
-
-            return
-
         kwargs = dict(self.resolve_args(provider, context))
         try:
-            result = method(context=context, provider=provider, **kwargs)
+            result = self._callable(context=context, provider=provider,
+                                    **kwargs)
         except Exception as e:
             if self.required:
-                raise HookExecutionFailed(self, exception=e)
+                raise HookExecutionFailed(self, cause=e)
 
-            return
+            return None
 
         if not result:
             if self.required:
@@ -125,6 +158,29 @@ def run(self, provider, context):
 
         return result
 
+    def run_step(self, provider_builder, context):
+        if not self.enabled:
+            return NotSubmittedStatus()
+
+        provider = provider_builder.build(profile=self.profile,
+                                          region=self.region)
+
+        try:
+            self.check_args_dependencies(provider, context)
+        except StackDoesNotExist as e:
+            reason = "required stack not deployed: {}".format(e.stack_name)
+            return SkippedStatus(reason=reason)
+
+        try:
+            result = self.run(provider, context)
+        except HookExecutionFailed as e:
+            return FailedStatus(reason=str(e))
+
+        if not result:
+            return SKIPPED
+
+        return COMPLETE
+
     def __str__(self):
         return 'Hook(name={}, path={}, profile={}, region={})'.format(
             self.name, self.path, self.profile, self.region)

Diff for: stacker/tests/test_context.py

@@ -9,6 +9,9 @@
 from stacker.config import load, Config
 
 
+FAKE_HOOK_PATH = "stacker.tests.fixtures.mock_hooks.mock_hook"
+
+
 class TestContext(unittest.TestCase):
 
     def setUp(self):
@@ -119,7 +122,7 @@ def test_hook_with_sys_path(self):
             "pre_build": [
                 {
                     "data_key": "myHook",
-                    "path": "fixtures.mock_hooks.mock_hook",
+                    "path": FAKE_HOOK_PATH.replace('stacker.tests.', ''),
                     "required": True,
                     "args": {
                         "value": "mockResult"}}]})
@@ -134,43 +137,46 @@ def test_hook_with_sys_path(self):
         self.assertEqual("mockResult", context.hook_data["myHook"]["result"])
 
     def test_get_hooks_for_action(self):
+
         config = Config({
             "pre_build": [
-                {"path": "fake.hook"},
-                {"name": "pre_build_test", "path": "fake.hook"},
-                {"path": "fake.hook"}
+                {"path": FAKE_HOOK_PATH},
+                {"name": "pre_build_test", "path": FAKE_HOOK_PATH},
+                {"path": FAKE_HOOK_PATH}
             ],
             "post_build": [
-                {"path": "fake.hook"},
-                {"name": "post_build_test", "path": "fake.hook"},
-                {"path": "fake.hook"}
+                {"path": FAKE_HOOK_PATH},
+                {"name": "post_build_test", "path": FAKE_HOOK_PATH},
+                {"path": FAKE_HOOK_PATH}
             ],
             "build_hooks": [
-                {"path": "fake.hook"},
-                {"name": "build_test", "path": "fake.hook"},
-                {"path": "fake.hook"}
+                {"path": FAKE_HOOK_PATH},
+                {"name": "build_test", "path": FAKE_HOOK_PATH},
+                {"path": FAKE_HOOK_PATH}
             ]
         })
 
         context = Context(config=config)
         hooks = context.get_hooks_for_action('build')
 
-        assert hooks.pre[0].name == "pre_build_1_fake.hook"
+        assert hooks.pre[0].name == "pre_build_1_{}".format(FAKE_HOOK_PATH)
         assert hooks.pre[1].name == "pre_build_test"
-        assert hooks.pre[2].name == "pre_build_3_fake.hook"
+        assert hooks.pre[2].name == "pre_build_3_{}".format(FAKE_HOOK_PATH)
 
-        assert hooks.post[0].name == "post_build_1_fake.hook"
+        assert hooks.post[0].name == "post_build_1_{}".format(FAKE_HOOK_PATH)
         assert hooks.post[1].name == "post_build_test"
-        assert hooks.post[2].name == "post_build_3_fake.hook"
+        assert hooks.post[2].name == "post_build_3_{}".format(FAKE_HOOK_PATH)
 
-        assert hooks.custom[0].name == "build_hooks_1_fake.hook"
+        assert hooks.custom[0].name == \
+            "build_hooks_1_{}".format(FAKE_HOOK_PATH)
         assert hooks.custom[1].name == "build_test"
-        assert hooks.custom[2].name == "build_hooks_3_fake.hook"
+        assert hooks.custom[2].name == \
+            "build_hooks_3_{}".format(FAKE_HOOK_PATH)
 
     def test_hook_data_key_fallback(self):
         config = Config({
             "build_hooks": [
-                {"name": "my-hook", "path": "fake.hook"}
+                {"name": "my-hook", "path": FAKE_HOOK_PATH}
             ]
         })
         context = Context(config=config)