Cross-account parameter resolving #292

petervandoros · 2019-11-12T11:58:51Z

This PR adds support for assuming a role when resolving parameters.

We've recently had to configure VPC peering and found it error prone to have to hard code VPC and peering IDs in the stacks' parameters files. It's also hard and tedious to find all the relevant values, especially for old VPCs.

Being able to reference stack outputs for stacks in other accounts would make this much more maintainable and less error prone, especially if the stacks need re-creating.

Here's some examples of what it looks like:

vpc_peering_id:
  role: cross-account-parameter-resolver
  account: 1234567890
  stack_output: vpc-peering-stack-in-other-account/peering_id
an_array_param:
  role: cross-account-parameter-resolver
  account: 1234567890
  stack_outputs:
    - stack-in-account1/output
    - stack-in-account1/another_output
another_array_param:
  - role: cross-account-parameter-resolver
    account: 1234567890
    stack_output: stack-in-account1/output
  - role: cross-account-parameter-resolver
    account: 0987654321
    stack_output: stack-in-account2/output

Considerations

The relevant IAM permissions to allow cross-account parameter resolving is left as an exercise for the reader.

This class is responsible to actually assuming a role and ensuring StackMaster and AWS SDK global state is updated properly.

It adds optional 'account' and 'role' properties to the parameter resolver definition. If set, the RoleAssumer class is used to assume the specified role before resolving the parameter.

This is needed because the integration tests swap the driver to the test driver which is needed to run the tests. This ensures the test driver is instantiated.

README.md

stevehodgkiss

Looks good 👍 👏 💯

orien

🎉

patrobinson

👍

patrobinson · 2019-11-13T23:49:09Z

lib/stack_master/role_assumer.rb

+          role_arn: "arn:aws:iam::#{account}:role/#{role}",
+          role_session_name: "stack-master-assume-role-parameter-resolver"
+        )
+      end


Why do we want to save credentials to an instance variable?

So it doesn't have to assume the role for every parameter wishing to assume the same role.

patrobinson · 2019-11-13T23:50:27Z

spec/stack_master/parameter_resolver_spec.rb

+        expect(StackMaster::RoleAssumer).to receive(:new).once
+
+        parameter_resolver.resolve
+      end


right I see now

and add another example.

viraptor

This all looks good

viraptor · 2019-11-18T03:09:03Z

lib/stack_master/role_assumer.rb

+
+    private
+
+    def replace_aws_global_config


I think the replace/restore functions are not very pretty. Part of that logic also exists in assume_role.

How about:

def assume_role(...) ... role_credentials = assume_role_credentials(account, role) with_temporary_role(role_credentials) do with_temporary_driver do block.call end end ... end def with_temporary_role(credentials, &block) original_aws_config = Aws.config Aws.config = original_aws_config.deep_dup block.call ensure Aws.config = original_aws_config end def with_temporary_driver(&block) original_driver = StackMaster.cloud_formation_driver StackMaster.cloud_formation_driver = original_driver.class.new block.call ensure StackMaster.cloud_formation_driver = original_driver end

Great idea. Done in 95a8825.

viraptor · 2019-11-18T03:11:07Z

lib/stack_master/role_assumer.rb

+    end
+
+    def restore_cf_driver(driver)
+      return if driver.nil?


If the driver is nil, driver.class.new in replace_cf_driver would fail earier.

orien · 2019-11-18T05:20:10Z

Do you mind adding to the changelog?

It's unnecessary as it doesn't cache API calls and is only used within the resolve method. It also will make it easier to support the assume role functionality.

It's unnecessary and makes supporting assuming a role more difficult to implement.

This is required to support assuming a role.

This is required when assuming a role.

petervandoros · 2019-12-16T11:16:37Z

@orien Updated changelog in 41592b3.

petervandoros · 2019-12-16T11:17:55Z

@orien @stevehodgkiss @patrobinson I've made a few more changes to resolve an issue I found.

It would be great if you could re-review the PR.

lib/stack_master/parameter_resolvers/ejson.rb

README.md

lib/stack_master/parameter_resolvers/ejson.rb

stevehodgkiss · 2019-12-16T12:56:29Z

lib/stack_master/role_assumer.rb

+    def assume_role_credentials(account, role)
+      credentials_key = "#{account}:#{role}"
+      @credentials.fetch(credentials_key) do
+        @credentials[credentials_key] = Aws::AssumeRoleCredentials.new(


Is it necessary to set the hash here if fetch is being used?

Suggested change

@credentials[credentials_key] = Aws::AssumeRoleCredentials.new(

Aws::AssumeRoleCredentials.new(

The block passed to the fetch method specifies the return value if the key is not found. It doesn't set the value in the hash if it's missing. We need to set the hash value for it to persist.

irb(main):001:0> h = {a: 'a', b: 'b'} => {:a=>"a", :b=>"b"} irb(main):002:0> h.fetch(:z) do irb(main):003:1* 'z' irb(main):004:1> end => "z" irb(main):005:0> h => {:a=>"a", :b=>"b"} irb(main):006:0> h.fetch(:z) do irb(main):007:1* h[:z] = 'z' irb(main):008:1> end => "z" irb(main):009:0> h => {:a=>"a", :b=>"b", :z=>"z"}

stevehodgkiss · 2019-12-16T12:58:04Z

features/apply_with_assume_role_parameter_resolvers.feature

@@ -0,0 +1,57 @@
+Feature: Apply command with assume role parameter resolvers


There's a build error with the following message: bundler's executable bundle conflicts with /home/travis/.rvm/rubies/ruby-2.3.8/bin/bundle Overwrite the executable? [yN]

This reverts commit 2905ad2.

orien

❤️

petervandoros added 6 commits November 12, 2019 12:54

Add RoleAssumer class

d72a685

This class is responsible to actually assuming a role and ensuring StackMaster and AWS SDK global state is updated properly.

Update ParameterResolver to support assuming roles

98c3b03

It adds optional 'account' and 'role' properties to the parameter resolver definition. If set, the RoleAssumer class is used to assume the specified role before resolving the parameter.

Use existing cloudformation driver's class to create new instance

f14b567

This is needed because the integration tests swap the driver to the test driver which is needed to run the tests. This ensures the test driver is instantiated.

Add integration test for assuming a role for parameter resolving

7015ef3

Add parameter resolver assume role docs to readme

c96653a

Minor typo fix in readme

d8a7bd6

petervandoros force-pushed the assume-role-parameter-resolving branch from b224117 to d8a7bd6 Compare November 12, 2019 12:16

petervandoros requested review from patrobinson and stevehodgkiss November 12, 2019 12:22

orien reviewed Nov 13, 2019

View reviewed changes

README.md Show resolved Hide resolved

stevehodgkiss approved these changes Nov 13, 2019

View reviewed changes

orien approved these changes Nov 13, 2019

View reviewed changes

patrobinson approved these changes Nov 13, 2019

View reviewed changes

petervandoros added 2 commits November 14, 2019 10:59

Be explicit about all parameters can assume a role

31bbaa3

and add another example.

Remove unused code in SnsTopicName

83934f3

viraptor approved these changes Nov 18, 2019

View reviewed changes

viraptor reviewed Nov 18, 2019

View reviewed changes

petervandoros added 8 commits December 11, 2019 20:47

Don't cache AmiFinder instance in latest_ami resolver

fbc6d65

It's unnecessary as it doesn't cache API calls and is only used within the resolve method. It also will make it easier to support the assume role functionality.

Don't cache SSM client in parameter_store resolver

8a9f15d

It's unnecessary and makes supporting assuming a role more difficult to implement.

Fix stack_output tests verifying stack caching

fd78c9d

Update stack_output resolver to take credentials into account

bb447b6

This is required to support assuming a role.

Update ejson parameter resolve to take credentials into account

530ed3d

This is required when assuming a role.

Refactor RoleAssumer for clarity

95a8825

Merge branch 'master' into assume-role-parameter-resolving

7ff4180

Update changelog to include cross-account parameter resolving

41592b3

petervandoros requested review from patrobinson, stevehodgkiss and orien December 16, 2019 11:16

petervandoros commented Dec 16, 2019

View reviewed changes

lib/stack_master/parameter_resolvers/ejson.rb Show resolved Hide resolved

Whitespace between rspec contexts

56cc3a6

petervandoros force-pushed the assume-role-parameter-resolving branch from a8b8670 to 56cc3a6 Compare December 16, 2019 12:15

stevehodgkiss approved these changes Dec 16, 2019

View reviewed changes

petervandoros added 4 commits December 17, 2019 09:26

Minor grammatical tweak to the README

4ad4ade

Don't install bundler in Travis CI

2905ad2

There's a build error with the following message: bundler's executable bundle conflicts with /home/travis/.rvm/rubies/ruby-2.3.8/bin/bundle Overwrite the executable? [yN]

Revert "Don't install bundler in Travis CI"

9da0c49

This reverts commit 2905ad2.

Merge branch 'master' into assume-role-parameter-resolving

b21ea00

orien approved these changes Dec 18, 2019

View reviewed changes

petervandoros merged commit 0d4c63d into master Dec 23, 2019

petervandoros deleted the assume-role-parameter-resolving branch December 23, 2019 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-account parameter resolving #292

Cross-account parameter resolving #292

petervandoros commented Nov 12, 2019

stevehodgkiss left a comment

orien left a comment

patrobinson left a comment

patrobinson Nov 13, 2019

petervandoros Nov 13, 2019

patrobinson Nov 13, 2019

viraptor left a comment

viraptor Nov 18, 2019

petervandoros Dec 16, 2019

viraptor Nov 18, 2019

orien commented Nov 18, 2019

petervandoros commented Dec 16, 2019

petervandoros commented Dec 16, 2019 •

edited

Loading

stevehodgkiss Dec 16, 2019

petervandoros Dec 16, 2019

stevehodgkiss Dec 16, 2019

orien left a comment

	@credentials[credentials_key] = Aws::AssumeRoleCredentials.new(
	Aws::AssumeRoleCredentials.new(

		@@ -0,0 +1,57 @@
		Feature: Apply command with assume role parameter resolvers

Cross-account parameter resolving #292

Cross-account parameter resolving #292

Conversation

petervandoros commented Nov 12, 2019

Considerations

stevehodgkiss left a comment

Choose a reason for hiding this comment

orien 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

viraptor 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

orien commented Nov 18, 2019

petervandoros commented Dec 16, 2019

petervandoros commented Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orien left a comment

Choose a reason for hiding this comment

petervandoros commented Dec 16, 2019 •

edited

Loading