feat(codepipeline): allow cross-account CloudFormation Actions #3208

skinny85 · 2019-07-05T00:16:16Z

This adds a property 'account' to all CloudFormation CodePipeline Actions,
and properly handles passing it in the Pipeline construct.

Please read the contribution guidelines and follow the pull-request checklist.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts

eladb · 2019-07-07T06:37:57Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+      let targetAccountStack: Stack | undefined = this._crossAccountSupport[targetAccount];
+      if (!targetAccountStack) {
+        const app = this.requireApp();
+        targetAccountStack = new Stack(app, `cross-account-support-stack-${targetAccount}`, {


No need to add the stack directly under the app. Add it inside the pipeline's scope

Scratch that. This does not work for some reason. The unit test in the codepipeline-actions package that tests the new behavior for the CFN actions fail when anchoring the new stack under the pipeline:

Error: Can only reference cross stacks in the same region and account. target = PipelineStack/Pipeline/cross-account-support-stack-123456789012/PipelineStackPipeline9DB740AF-Deploy-CFN-DeploymentRole/Resource

It seems to me like the cross-account behavior is not triggered at all when I add the new stack under the pipeline's scope (this is an error message from CfnReference).

Any ideas why might that be...? Could it be because Stack.of(...) does not work correctly if stacks are nested under constructs...?

I think you are right. I think there is a bug in how substacks consume references. I will run some more tests tomorrow morning and hope to have some better understanding of this.

This actually makes sense. The pipeline stack references the role in the other account and the automatic cross-stack references are triggered, but those only work for same-environment since they use exports/imports.

Digging further...

Okay, haven't managed to figure out what the issue is. I still think there is a bug somewhere, but you'll have to dig further from your end. I've added a bunch of tests to verify some assumptions: #3373

Ok. I've updated the PR to fix the out of order imports. Let's see if the build passes this time.

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

packages/@aws-cdk/aws-codepipeline/lib/action.ts

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

skinny85 · 2019-07-07T23:33:02Z

Updated with the comments on the previous revision.

packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

skinny85 · 2019-07-19T00:26:31Z

I've updated the PR with all comments.

The only exception is changing the parent of the newly created stack, as anchoring it under the pipeline construct makes the unit test fail (see above comment).

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

eladb · 2019-07-22T06:30:51Z

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

+      if (this.isAwsOwned(action)) {
+        return action.actionProperties.role;
+      } else {
+        throw new Error("Specifying a Role is not supported for actions with an owner different than 'AWS' - " +


The terminology "owner" here is extremely confusing (especially since we use it to indicate "owned resources"). How about something like "The action X does not support specifying a role".

Just to be clear: I'm talking here about this property.

Given that, does the error message makes sense? (I'm not worried about the isAwsOwned() method, it's private anyway)

EDIT: maybe this is a better explanation of the owner property

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

skinny85 · 2019-07-24T00:50:15Z

Newest batch of addressed comments.

skinny85 · 2019-07-25T23:54:19Z

Feel free to re-review @eladb .

eladb

Please update README

NGL321 · 2019-08-01T22:03:46Z

Can the conflicts here be resolved and the PR merged?

skinny85 · 2019-08-01T22:18:01Z

Please hold off on merging this ATM. I'll handle it.

skinny85 · 2019-08-03T00:27:55Z

I realized something while working on the cross-account events in #3323. In previous revisions of this PR, I was caching the "scaffold stacks" (the ones generated for the IAM roles needed for cross-account deployments) for the actions on the Pipeline level. But that doesn't make sense; it should be global, on the App level, like we did for events in #3323 . I've updated the PR changing this; I also need to change the approach for the replication bucket stacks in a similar matter, but since I'm working on large changes in that area anyway, I'll defer that to a later PR.

skinny85 · 2019-08-09T20:38:24Z

Rebased.

This adds a property 'account' to all CloudFormation CodePipeline actions, and properly handles passing it in the pipeline construct.

skinny85 · 2019-08-09T21:27:17Z

Updated the ReadMe.

skinny85 requested review from RomainMuller and a team as code owners July 5, 2019 00:16

eladb reviewed Jul 7, 2019

View reviewed changes

packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts Show resolved Hide resolved

eladb reviewed Jul 7, 2019

View reviewed changes

eladb suggested changes Jul 7, 2019

View reviewed changes

skinny85 force-pushed the feature/cross-account-cfn-codepipeline branch from 44bd30e to 15d43cb Compare July 7, 2019 23:32

eladb reviewed Jul 16, 2019

View reviewed changes

skinny85 force-pushed the feature/cross-account-cfn-codepipeline branch from 15d43cb to fee4c7a Compare July 19, 2019 00:25

skinny85 requested a review from a team as a code owner July 19, 2019 00:25

eladb reviewed Jul 22, 2019

View reviewed changes

skinny85 self-assigned this Jul 22, 2019

skinny85 mentioned this pull request Jul 23, 2019

Make the CLI experience of multi-account CDK apps awesome #3401

Closed

skinny85 force-pushed the feature/cross-account-cfn-codepipeline branch from fee4c7a to 54dacc8 Compare July 24, 2019 00:49

eladb previously approved these changes Jul 29, 2019

View reviewed changes

skinny85 force-pushed the feature/cross-account-cfn-codepipeline branch from 54dacc8 to 9a35fd3 Compare August 3, 2019 00:22

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Aug 6, 2019

skinny85 dismissed eladb’s stale review via 696e2ee August 9, 2019 20:38

skinny85 force-pushed the feature/cross-account-cfn-codepipeline branch from 9a35fd3 to 696e2ee Compare August 9, 2019 20:38

feat(codepipeline): allow cross-account CloudFormation actions

5f04f2b

This adds a property 'account' to all CloudFormation CodePipeline actions, and properly handles passing it in the pipeline construct.

skinny85 force-pushed the feature/cross-account-cfn-codepipeline branch from 696e2ee to 5f04f2b Compare August 9, 2019 21:27

skinny85 merged commit 8df4b7e into aws:master Aug 9, 2019

skinny85 deleted the feature/cross-account-cfn-codepipeline branch August 9, 2019 22:19

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

tjenkinson mentioned this pull request Feb 25, 2021

(cli): reduce deploy role scope #12985

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(codepipeline): allow cross-account CloudFormation Actions #3208

feat(codepipeline): allow cross-account CloudFormation Actions #3208

skinny85 commented Jul 5, 2019

eladb Jul 7, 2019

skinny85 Jul 17, 2019

skinny85 Jul 18, 2019

eladb Jul 21, 2019

eladb Jul 22, 2019

eladb Jul 22, 2019

skinny85 Jul 23, 2019

skinny85 commented Jul 7, 2019

skinny85 commented Jul 19, 2019

eladb Jul 22, 2019

skinny85 Jul 23, 2019 •

edited

Loading

skinny85 commented Jul 24, 2019

skinny85 commented Jul 25, 2019

eladb left a comment

NGL321 commented Aug 1, 2019

skinny85 commented Aug 1, 2019

skinny85 commented Aug 3, 2019

skinny85 commented Aug 9, 2019

skinny85 commented Aug 9, 2019

feat(codepipeline): allow cross-account CloudFormation Actions #3208

feat(codepipeline): allow cross-account CloudFormation Actions #3208

Conversation

skinny85 commented Jul 5, 2019

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

skinny85 commented Jul 7, 2019

skinny85 commented Jul 19, 2019

Choose a reason for hiding this comment

skinny85 Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

skinny85 commented Jul 24, 2019

skinny85 commented Jul 25, 2019

eladb left a comment

Choose a reason for hiding this comment

NGL321 commented Aug 1, 2019

skinny85 commented Aug 1, 2019

skinny85 commented Aug 3, 2019

skinny85 commented Aug 9, 2019

skinny85 commented Aug 9, 2019

skinny85 Jul 23, 2019 •

edited

Loading