Add ability to restrict which accounts can work with stacks #283

petervandoros · 2019-06-25T06:27:40Z

In order to provide extra safety and reduce the risk of applying non-production changes to production, this PR adds the ability to specify which accounts are allowed to work with specific stacks.

The behaviour can be summarised using the following example stack_master.yml file:

stack_defaults:
  allowed_accounts: '555555555'
stacks:
  us-east-1:
    myapp-vpc: # inherits allowed account 555555555
      template: myapp_vpc.rb
      tags:
        purpose: front-end
    myapp-db:
      template: myapp_db.rb
      allowed_accounts: # only these accounts
        - '1234567890'
        - '9876543210'
      tags:
        purpose: back-end
    myapp-web:
      template: myapp_web.rb
      allowed_accounts: [] # allow all accounts by overriding stack defaults
      tags:
        purpose: front-end
    myapp-redis:
      template: myapp_redis.rb
      allowed_accounts: '888888888' # only this account
      tags:
        purpose: back-end

Changes

Add allowed_accounts stack definition property that lists the aws account IDs allowed to work with the stack
Add a --skip-account-check flag to bypass the account checks
Updated readme

This adds the ability to specify stack defaults and override it for a specific stack.

lukerobins · 2019-06-25T07:43:44Z

lib/stack_master/identity.rb

@@ -0,0 +1,23 @@
+module StackMaster
+  class Identity
+    def running_in_allowed_account?(allowed_accounts)


Seems odd to pass a nil or empty variable to a function.

Maybe if nil was the default value for allowed_accounts (in StackDefinition, indicating unspecified) the expected values would either be nil or an array of account IDs

Good idea. Done in b30dede.

stevehodgkiss

Looks good 👍

viraptor

This looks good.

lib/stack_master/cli.rb

patrobinson

Code looks good, I'm curious about the tests though...

patrobinson · 2019-06-27T06:13:05Z

README.md

+
+This is an opt-in feature which is enabled by specifying at least one account to allow.
+
+Unlike other stack defaults, `allowed_accounts` values specified in the stack definition override values in the stack defaults instead of merging them together. This allows specifying allowed accounts in the stack defaults for all stacks and still have different values for specific stacks. See below example config for an example.


From just reading this I'm still not entirely sure what the behaviour will be. Does it allow applying myapp-db to account 555555555 or not?

No it doesn't as the account IDs specified in the stack definition overrides the stack defaults.

I've reworded this section and example stack below. Let me know if it clarifies things.

See 4233345.

patrobinson · 2019-06-27T06:16:17Z

lib/stack_master/identity.rb

+
+    def region
+      @region ||= ENV['AWS_REGION'] || Aws.config[:region] || Aws.shared_config.region || 'us-east-1'
+    end


This method isn't necessary. If we don't specify a region the SDK does a lookup for us automatically.

You're right it does, however I added this method to ensure that a default region is used if it couldn't find a region from the environment. It's also the same as the CloudFormation driver class except for the default value.

This was me being more defensive, but happy to remove it if you think it'll be fine without it?

patrobinson · 2019-06-27T06:17:59Z

spec/stack_master/commands/status_spec.rb

+        end
+
+        after do
+          StackMaster.reset_flags


I think spec_helper already handles this

It doesn't handle this. The cucumber features do though.

Right you are

patrobinson · 2019-06-27T06:19:17Z

spec/stack_master/commands/status_spec.rb

+          us-east-1 | stack2     | UPDATE_COMPLETE    | Yes      
+           * No echo parameters can't be diffed
+        OUTPUT
+        expect { status.perform }.to output(out).to_stdout


Shouldn't this test expect an error to be returned? If the account IDs don't match it shouldn't print the status?

It's a got a special case now: Disallowed account | N/A.
It would not be great if it threw an error just because one stack is unavailable.

What @viraptor said.

Alternatively, I could leave them blank instead? I didn't go with that option because I thought it would be better to let the user know why no status is included for that stack.

Another option is to include SKIPPED for the status field and Disallowed account for different field.

Right I see. Are we able to have an integration test for when someone runs stack_master apply on a stack in the wrong account then? I thought that's what this was doing, but I didn't read the spec name 😆

Good idea. Added integration tests in f02603c.

patrobinson

Thanks

petervandoros added 9 commits June 25, 2019 12:52

Add 'allowed_accounts' property to stack definition

aef2c38

Add Identity class to check allowed accounts

d3fa4ee

Add allowed accounts support to commands touching stacks

1a92477

Add allowed accounts support to delete command

c72a000

Add allowed accounts support to status command

f79ef55

Use heredocs for status command output tests

abbbd3e

Add --skip-account-check flag

4eff0f9

Override stack default allowed_accounts instead of merging

056aeea

This adds the ability to specify stack defaults and override it for a specific stack.

Update readme with info about allowed accounts

83bd9a6

petervandoros requested review from stevehodgkiss and patrobinson June 25, 2019 06:27

lukerobins reviewed Jun 25, 2019

View reviewed changes

stevehodgkiss approved these changes Jun 25, 2019

View reviewed changes

petervandoros added 2 commits June 26, 2019 09:20

Default allowed_accounts to nil to signify unspecified

b30dede

Fix typos in readme

3ee8a08

runlevel5 approved these changes Jun 25, 2019

View reviewed changes

lukerobins approved these changes Jun 25, 2019

View reviewed changes

viraptor approved these changes Jun 26, 2019

View reviewed changes

lib/stack_master/cli.rb Outdated Show resolved Hide resolved

Use consistent instance variable naming

6351741

patrobinson reviewed Jun 27, 2019

View reviewed changes

petervandoros added 2 commits June 27, 2019 17:11

Reword allowed accounts readme text

4233345

Add integration tests for allowed accounts support

f02603c

patrobinson approved these changes Jun 28, 2019

View reviewed changes

petervandoros merged commit d3f96df into master Jul 2, 2019

petervandoros deleted the stack-allowed-accounts branch July 2, 2019 06:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to restrict which accounts can work with stacks #283

Add ability to restrict which accounts can work with stacks #283

petervandoros commented Jun 25, 2019

lukerobins Jun 25, 2019

stevehodgkiss Jun 25, 2019

petervandoros Jun 25, 2019

stevehodgkiss left a comment

viraptor left a comment

patrobinson left a comment

patrobinson Jun 27, 2019

petervandoros Jun 27, 2019

patrobinson Jun 27, 2019

petervandoros Jun 27, 2019

patrobinson Jun 27, 2019

petervandoros Jun 27, 2019

patrobinson Jun 27, 2019

patrobinson Jun 27, 2019

viraptor Jun 27, 2019

petervandoros Jun 27, 2019

petervandoros Jun 27, 2019

patrobinson Jun 27, 2019

petervandoros Jun 28, 2019

patrobinson left a comment


		This is an opt-in feature which is enabled by specifying at least one account to allow.

		Unlike other stack defaults, `allowed_accounts` values specified in the stack definition override values in the stack defaults instead of merging them together. This allows specifying allowed accounts in the stack defaults for all stacks and still have different values for specific stacks. See below example config for an example.

Add ability to restrict which accounts can work with stacks #283

Add ability to restrict which accounts can work with stacks #283

Conversation

petervandoros commented Jun 25, 2019

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevehodgkiss left a comment

Choose a reason for hiding this comment

viraptor left a comment

Choose a reason for hiding this comment

patrobinson 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

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

patrobinson left a comment

Choose a reason for hiding this comment