-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
@ashb @stuartornum here's the new approach as we discussed for review. |
includes: | ||
- /path/to/cloudformation.json | ||
|
||
The tool will then merge this with the generated template *overwriting* any keys in the original template that clash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite clear on two counts:
- What's a key? (Are we talking a matching logical resource id)
- That we do deep merging (i.e. we're not overwriting the Output section entirely.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try to make it clearer. Quite hard to explain deep merging in English though!
The few minor comments above not withstanding this PR actually does three completely separate things
These happen to be related since you want them all, but even your commits show that it's three features (the first three commits are point 1, next is point 2 and last is point 3). Sorry to be a pain but can you split into three different PRs please? |
(Or at least come up with a better PR title encompasses all the features) |
Yeah, wasn't sure whether to split or not. I feel like it's kind of one story so nice to have in one place but they are quite distinct features. I think there's some good history in this PR, so I'd rather leave it be if you're happy for me to rename it and tidy up the description :-) |
Title is much better now 👍. It looks like you need to rebase this onto master at least - possibly squash a few of the commits too in the process (but not all of them) Also can you document AWS_ROLE_ARN_ID (and possibly how you would get it) and add a changelog entry for this feature. |
Moving dictionary deep merge to utils module so that it is easier to reuse accross classes.
This adds the option to include extra cloudformation json from a static file. The includes are merged with the template we generate, overwriting existing keys that clash.
This adds an extra bit of code to the AWS api connection utility so that it is possible to assume an IAM role from another account (if your user has the right to). The ARN id of the role to assume is expected as an environment variable.
This adds the path explicitly in the salt_utils module so that a user with limited access does not have to set their PYTHONPATH.
This commit fixes a couple of mistakes in the merging code for included cloudformation templates. It also makes the documentation clearer on how the merge works.
b8a2355
to
21d6a8e
Compare
@ashb rebased and tidy up 😄 |
This PR adds several features:
The overall goal is that we can create an IAM user in a central AWS account which will get limited permissions by way of an included template. The user will then be able to login using the new cross account authentication. Finally the user will be able to run the salt_utils script as a limited sudoer.
This is we think cleaner than #71 and allows us to move the IAM roles into template deploy with https://github.com/ministryofjustice/template-deploy/pull/23