Formalize the existence of external (read-only) stacks #618

danielkza · 2018-07-03T21:46:48Z

Currently, the ref and xref lookups always use the same AWS profile and region that Stacker is initialized with. That means they cannot access outputs in other accounts or regions. It is possible to create a stack that will just "pull" those outputs by adding locked: true to it. But that still does not make it possible to reference a stack outside of the current namespace, if one is configured. Additionally, options that should never take effect for a "read-only" stack were allowed, such as class_path or stack_policy_path.

To handle that use case, introduce and document the external: yes option for stacks. External stacks do not have any of the options related to creating or updating resources, such as class_path and template_path, and are used only as sources of values to other stacks (through outputs and lookups).
Additionally, add an fqn option to external stacks to fix the namespace issue.

Internal changes

To ensure the separation has the desired effect, enable strict mode while parsing configuration into models, but only for non-top-level keys. Achieve it by manually filtering unrecognized top-level keys before forwarding the data for parsing.

To better separate external and managed stacks in config, rework the model hierarchy a little: add BaseStack that only contains the shared config options between both stack types, make it a superclass of Stack, and introduce ExternalStack. Unfortunately Schematics 2.0 has a bug where it did not validate models parsed using polymorphism, so upgrade to version 2.1.

Move decisions on whether a stack needs to be submitted or updated to the stacker.stack.Stack class, to simply the logic and allow it to differ between external and managed stacks. Introduce stacker.stack.ExternalStack with "empty" implementations of all the methods/properties related to templates, blueprints and updates.

Documentation updates

Rework the Stack documentation to list options allowed for all stacks, external and managed stacks separately. Document everything in YAML format, so that people can copy snippets to their configs from the different parts. Fix some typos and clarify some details in the process.

Open questions

The external/managed terminology is the best I could come up with, but I still feel it can be improved. Any suggestions of alternatives are welcome!

Notes

I ran functional tests manually on my all account and they passed - I even had to fix one that had stack_policy_path misspelled and was no longer accepted due to strict parsing.

danielkza · 2018-07-03T21:51:53Z

Should't the unit tests also be running on Python 3 now?

danielkza · 2018-07-03T21:54:20Z

stacker/config/__init__.py

+        self.external = True
+
+    def validate_fqn(self, data, value):
+        if (not value and not data["stack_name"]) or \


I just noticed this is wrong, since we can infer the CF name from the stack name. Will fix!

ejholmes · 2018-07-03T23:25:41Z

stacker/context.py

-                    enabled=stack_def.enabled,
-                    protected=stack_def.protected,
-                )
+                if isinstance(stack_def, ExternalStackModel):


I like the change to inject a fake Stack implementation, but I'm wondering if an external flag + config.ExternalStack is overkill. At the moment, a locked stack that has no blueprint or template_path should be considered a read-only stack.

What if the condition here was just:

if stack_def.locked and not stack_def.blueprint and not stack_def.template_path: # ExternalStack

That would be a much smaller change and help reduce the size of this PR (and be more backwards compatible with existing configs).

That was my first approach, but I noticed more than half of an external stack's attributes would be completely invalid/useless, and that I needed to add conditions that checked stack.external in dozens of places. Additionally, being loose with parsing can be an easy source of mistakes, so much that one of Stacker's own functional tests had a mistyped key that was never noticed! While there were a significant amount of changes, adding enough tests to the Stack models to cover all cases where external was at would be as big if not bigger.

danielkza · 2018-07-07T19:00:37Z

Interesting note: I found 20+ usages of invalid options in stacks in my company's repo when using this PR (due to enabling strict parsing). Most of them were harmless (using enable instead of enabled), but a few were a bit more dangerous.

I can make a PR that just enables strict parsing before this one (which is larger and a bit more controversial). What do you think @ejholmes?

edit: PR here: #623

phobologic · 2018-07-08T05:21:38Z

docs/config.rst

+
+  namespace: other-example
+  stacks:
+    - name: vpc


maybe throw a comment here stating that this stack won't be changed/build/etc because of the external flag (it is redundant, but I think it's useful)

phobologic · 2018-07-08T05:26:11Z

docs/stack-config-managed.yml

+# dependencies are automatically created when using an output from a stack in
+# the variables of another stack. Use it when you know that a hidden dependency
+# exists (or one is documented in the CloudFormation reference).
+dependencies:


This should be requires, right?

Ping on this one ^^

Indeed, I missed it :)

phobologic · 2018-07-30T03:52:36Z

@ejholmes @danielkza - I'm thinking we should spin a release of stacker this week, it's been quite a while, and we have a lot of features/updates already. Do you think we should wait for this? What's left for discussing here?

danielkza · 2018-10-13T19:01:34Z

I'm looking to revive this PR as I update our internal Stacker fork to the latest master. The strict parsing change has been integrated for a while already, any other concerns I should handle to get this merged? @ejholmes @phobologic

phobologic

Some minor changes, otherwise this looks good to me. Thanks!

phobologic · 2018-10-14T02:08:08Z

docs/config.rst

+Stacks can be classified as:
+
+Managed
+  Stacks that are fully managed by the current Stacker configuration.


Worth mentioning this is the default?

Yeah, I can clarify this section a bit!

phobologic · 2018-10-14T02:08:49Z

docs/config.rst

+  Stacks that are fully managed by the current Stacker configuration.
+  They will be created, updated or destroyed as needed.
+External
+  Stacks that will not be modified by the current Stacker configuration.


Might be worth saying that these stacks are configured with the external flag.

phobologic · 2018-10-14T02:09:49Z

docs/stack-config-managed.yml

+# dependencies are automatically created when using an output from a stack in
+# the variables of another stack. Use it when you know that a hidden dependency
+# exists (or one is documented in the CloudFormation reference).
+dependencies:


Ping on this one ^^

phobologic · 2018-10-14T02:14:30Z

stacker/stack.py

+    def set_outputs(self, outputs):
+        self.outputs = outputs
+
+    def should_submit(self):


What does this end up looking like in the CLI output when we hit an external stack? Does it say it's submitting/creating?

I believe it prints "skipped". Do you think we should add an additional status to print something different?

Hmm - yeah, that might be useful. It can still be skipped, just with a slightly different message maybe?

phobologic · 2018-10-14T02:15:09Z

tests/suite.bats

@@ -162,7 +162,7 @@ namespace: ${STACKER_NAMESPACE}
 stacks:
  - name: vpc
    class_path: stacker.tests.fixtures.mock_blueprints.VPC
-    stack_policy: tests/fixtures/stack_policies/default.json
+    stack_policy_path: fixtures/stack_policies/default.json


We broke these up into individual files, so you'll probably want to merge master and get that cleared up here.

danielkza · 2018-10-14T19:59:38Z

Rebased to latest master. Was a bit harder than what I hoped, and I ended up having to make some changes to the build action tests.

I'll address your review comments in further commits @phobologic :)

danielkza · 2018-10-14T21:48:41Z

I also found some issues running the functional tests, good thing we have those!

danielkza · 2019-03-12T18:35:06Z

I'm circling back to this. Rebased to latest master again, but unfortunately I'm seeing failed unit tests locally due to #709.

To import outputs from stacks that are not managed directly by a Stacker config and require custom settings (such as a profile or region), it was previously possible to just omit both a `class_path` and `template_path`. Make that pattern "official" by introducing an `external` option, and a simplified `ExternalStack` model that omits all the stack keys that do not make sense if we're never going to create/update the stack. Note: the upgrade to schematics 2.1.0 is required due to a bug [1] where polymorphic models were not properly validated. [1]: schematics/schematics#440

Don't ever attempt to update them, but always submit them so outputs can be grabbed.

This makes handling the difference between managed and external stacks much easier. Also clean up the build tests a little bit by creating a TestStack class instead of using Mocks.

phobologic

This looks good @danielkza - can you merge master in (it looks like it's confused about some docs changes), and then we'll get it merged. Thanks!

danielkza · 2019-04-08T01:35:05Z

@phobologic Things looked up-to-date to me, but I took the opportunity to improve the wording on the docs a little bit and forced-pushed.

phobologic · 2019-04-08T01:37:00Z

Weird! I wonder why it is showing the additions of stack_name, which is already in the master branch. Ok, once tests pass I'll go ahead and merge :)

danielkza · 2019-04-08T01:38:36Z

I think it's because I moved it around in the file a little bit.

danielkza commented Jul 3, 2018

View reviewed changes

ejholmes reviewed Jul 3, 2018

View reviewed changes

phobologic requested changes Jul 8, 2018

View reviewed changes

phobologic requested changes Oct 14, 2018

View reviewed changes

danielkza force-pushed the external-stacks branch from 05e6bbb to ee1ac01 Compare October 14, 2018 19:55

danielkza force-pushed the external-stacks branch from ee1ac01 to caf7860 Compare March 12, 2019 18:33

danielkza added 9 commits March 29, 2019 17:12

Properly handle external stacks in actions

17ff01b

Don't ever attempt to update them, but always submit them so outputs can be grabbed.

config: add fqn option to external stacks

f614885

tests: add functional tests for external stacks

fc868ad

Move should_update and should_submit methods to Stack subclasses

d0ee28e

This makes handling the difference between managed and external stacks much easier. Also clean up the build tests a little bit by creating a TestStack class instead of using Mocks.

stack: remove redundant parameters from constructor

19a0386

session_cache: clarify arg types

9a7e889

config: validate FQN correctly in ExternalStack

6978716

Update Build Action tests for external stack changes

8e36bf6

danielkza force-pushed the external-stacks branch from caf7860 to 3b13657 Compare March 29, 2019 20:12

phobologic approved these changes Apr 8, 2019

View reviewed changes

docs: document external stacks

2ce72e2

danielkza force-pushed the external-stacks branch from 3b13657 to 2ce72e2 Compare April 8, 2019 01:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Formalize the existence of external (read-only) stacks #618

Formalize the existence of external (read-only) stacks #618

danielkza commented Jul 3, 2018

danielkza commented Jul 3, 2018

danielkza Jul 3, 2018

ejholmes Jul 3, 2018

danielkza Jul 4, 2018

danielkza commented Jul 7, 2018 •

edited

Loading

phobologic Jul 8, 2018

danielkza Oct 14, 2018

phobologic Jul 8, 2018

phobologic Oct 14, 2018

danielkza Oct 14, 2018

phobologic commented Jul 30, 2018

danielkza commented Oct 13, 2018

phobologic left a comment

phobologic Oct 14, 2018

danielkza Oct 14, 2018

phobologic Oct 14, 2018

phobologic Oct 14, 2018

phobologic Oct 14, 2018

danielkza Oct 14, 2018

phobologic Oct 14, 2018

phobologic Oct 14, 2018

danielkza commented Oct 14, 2018

danielkza commented Oct 14, 2018

danielkza commented Mar 12, 2019 •

edited

Loading

phobologic left a comment

danielkza commented Apr 8, 2019

phobologic commented Apr 8, 2019

danielkza commented Apr 8, 2019

Formalize the existence of external (read-only) stacks #618

Are you sure you want to change the base?

Formalize the existence of external (read-only) stacks #618

Conversation

danielkza commented Jul 3, 2018

Internal changes

Documentation updates

Open questions

Notes

danielkza commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielkza commented Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phobologic commented Jul 30, 2018

danielkza commented Oct 13, 2018

phobologic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielkza commented Oct 14, 2018

danielkza commented Oct 14, 2018

danielkza commented Mar 12, 2019 • edited Loading

phobologic left a comment

Choose a reason for hiding this comment

danielkza commented Apr 8, 2019

phobologic commented Apr 8, 2019

danielkza commented Apr 8, 2019

danielkza commented Jul 7, 2018 •

edited

Loading

danielkza commented Mar 12, 2019 •

edited

Loading