feat(servicecatalogappregistry-alpha): Introduce flag to control application sharing and association behavior for cross-account stacks #24408

jungle-amazon · 2023-03-02T03:03:13Z

Problem:

Currently, the ApplicationAssociator construct automatically shares the target Application with any accounts of cross-account stacks. [code reference]
If the owner of a cross-account stack is not part of the same AWS Organization as the owner of the ApplicationAssociator stack, or otherwise have not enabled cross-account sharing, during deployment the ApplicationAssociator will fail when attempting to share the application with the stack owner, with a message like below:

Principal 123456789012 is not in your AWS organization. You do not have permission to add external
AWS accounts to a resource share. 
(Service: AWSRAM; Status Code: 400; Error Code: OperationNotPermittedException;
Request ID: aaa; Proxy: null)

Feature:

We want to introduce a mechanism (associateCrossAccountStacks field in TargetApplicationOptions) where the user can specify if they want to allow sharing their application to any accounts of cross-account stacks in order to then subsequently associate the stack with the application.
This flag will be false by default. This allows customers to have their stack deployments proceed without being blocked on application sharing or cross-account associations.
- If set to false, ApplicationAssociator will skip the application sharing and association for cross-account stacks. During synthesis, a warning will be displayed to notify that cross-account stacks were detected but sharing and association will be skipped.
- If set to true, the application will be shared and then associated for cross-account stacks. This relies on the user properly setting up cross-account sharing beforehand.

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

…ication sharing and association behavior for cross-account scenarios

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

jungle-amazon · 2023-03-03T02:48:43Z

...s-servicecatalogappregistry/test/integ.application-associator.cross-account-stack-enabled.ts

+
+new cdk.Stack(app, 'integ-servicecatalogappregistry-cross-account-resource', {
+  env: {
+    account: '123456',


While generating the snapshots, I used an actual account to complete the deployment successfully, and then replaced all places in the snapshot files where that account was mentioned with this dummy ID. Please let me know if this is an acceptable approach and/or if there is a better way to setup these integ test snapshots.

in general we don't specify the env unless there's a specific region limitation for these integ test stacks

If you specify a fake id, then it won't deploy when someone attempts to deploy it. We should leave env unspecified here

Got it, then I don't think there's a good way to test a true cross-account deployment scenario; instead I will make this focus on just one env while still exercising the code logic of handling cross-account stacks.

comcalvi

There's a key piece I don't understand here. How does a user run into the error with cross-account sharing not working? How are we trying to share these cross-account today? I'd expect that if we're trying to share these cross-account, it's because the user has it set up to use multiple account IDs and we need this thing to be shared cross account for their app to work. Giving them a flag that lets their deployment succeed but doesn't do what they asked for isn't the solution

comcalvi · 2023-03-03T22:38:14Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts

   *
   * NOTE: This method won't automatically register stacks under pipeline stages,
   * and requires association of each pipeline stage by calling this method with stage Construct.
   *
   */
  public associateAllStacksInScope(scope: Construct): void {
-    cdk.Aspects.of(scope).add(new StageStackAssociator(this));
+    cdk.Aspects.of(scope).add(new StageStackAssociator(this, {
+      enableCrossAccount: true,


I don't mind this defaulting to true, but why does this need to be specified at all? Can we make enableCrossAccount true by default on the StageStackAssociator?

Yes, I think defaulting to true makes sense.

I have since discussed this with the team and we strongly favor making this false on StageStackAssociator because it is more likely that customers have not setup properly for cross-account cases. Setting it to false makes this safe assumption so that the default behavior doesn't break any customers.

Then, we'd have to set the value to true here as we want associateAllStacksInScope() to include cross-account stacks, and in this case the customer would be explicitly associating all stacks by calling this method.

packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/stack-associator.ts

comcalvi · 2023-03-03T22:41:07Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/stack-associator.ts

-      this.sharedAccounts.add(node.account);
+        this.sharedAccounts.add(node.account);
+      } else {
+        this.warning(node, 'Cross-account stack detected but application sharing and association will be skipped because cross-account option is not enabled.');


why is this not an error? It sounds like the user wanted to enable cross-stack, but that they didn't enable it. That's a bad configuration => it should throw an error

Please explain to me how we get here today. What did the user configure that made us detect the extra account? It seems like that's where the issue is

It is a warning instead of an error to not block customers from proceeding with the deployment of cross-account stacks. In the CDK app definition, the user should be able to specify stacks owned by other accounts, and as long as the accounts that own those stacks have applied the permissions to allow CDK deployments, then they should be allowed.

The warning states that the stack would not be associated with the AppRegistry Application, but otherwise, the deployment of the resources can proceed. Later, if the customer wishes, when the account of the stack is configured to be part of the same AWS Organization as the Application owner, and cross-account sharing is enabled, only then can the customer enable the option to perform the cross-account association.

packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/stack-associator.ts

comcalvi · 2023-03-03T22:42:25Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/stack-associator.ts

 }

 export class StageStackAssociator extends StackAssociatorBase {
  protected readonly application: IApplication;
  protected readonly applicationAssociator?: ApplicationAssociator;
+  protected readonly enableCrossAccount: boolean;


Should we add this to StageStackAssociatorBase, since we're using it on two separate subclasses, or are there more subclasses of StackStackAssociatorBase than these two?

I missed that this is possible; yes, I think we can just handle the field in the StageStackAssociatorBase constructor.

packages/@aws-cdk/aws-servicecatalogappregistry/lib/target-application.ts

comcalvi · 2023-03-03T22:44:51Z

packages/@aws-cdk/aws-servicecatalogappregistry/lib/target-application.ts

@@ -108,6 +120,7 @@ class CreateTargetApplication extends TargetApplication {

    return {
      application: appRegApplication,
+      enableCrossAccountStacks: this.applicationOptions.enableCrossAccountStacks ?? false,


Why do we have so many places where this default needs to be set?

Is there a way we can configure their accounts to be able to do cross-account sharing if it isn't set up correctly?

Why do we have so many places where this default needs to be set?

We need to set this in both TargetApplication sub-classes CreateTargetApplication and ExistingTargetApplication to set a default if the ApplicationAssociator user has not specified it, and then a separate flag has to be handled in StackAssociatorBase for defining the default behavior for scenarios that don't involve ApplicationAssociator.

Is there a way we can configure their accounts to be able to do cross-account sharing if it isn't set up correctly?

There could be a way to configure within CDK to automatically manage the AWS Organization membership of an account, but for now we are limiting ApplicationAssociator to focus more on the AppRegistry application associations. For now, outside of CDK, each account will need to take action to be part of the same AWS Organization as a "Management" account, and then the Management account will need to enable cross-account sharing.

jungle-amazon · 2023-03-07T00:44:24Z

There's a key piece I don't understand here. How does a user run into the error with cross-account sharing not working? How are we trying to share these cross-account today? I'd expect that if we're trying to share these cross-account, it's because the user has it set up to use multiple account IDs and we need this thing to be shared cross account for their app to work. Giving them a flag that lets their deployment succeed but doesn't do what they asked for isn't the solution

My original PR description wasn't clear on this; here is my re-worded explanation of the scenario.

This feature targets the situation where the customer is more interested in successful deployments rather than the association of the AppRegistry application to the cross-account stacks. In this case, they may have not set up all their accounts to be part of the same AWS Organization. (AppRegistry cross-account sharing only works for accounts within the same AWS organization).

For example, let's say there's a CDK app defined, with the Application stack in Account A, and other CFN resource stacks defined across Accounts A, B and C. (With the assumption that A, B, and C have set up permissions to allow deployments by the role of CDK deployer).

Our original motivation with defaulting to automatic sharing of the Application, was to fully expand the coverage of ApplicationAssociator, so that even if a stack was part of a different account, it would still be associated since it was still in the same CDK app.
However, cross-account sharing for AppRegistry is only supported for accounts within an organization. We want to unblock customers who have not setup AWS Organizations (yet), but otherwise are using ApplicationAssociator, which will at least associate the stacks that are part of the same account as the Application stack.

…ack-associator.ts Remove unnecessary empty line Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Pull request has been modified.

…lication.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

…ack-associator.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Set default to true

packages/@aws-cdk/aws-servicecatalogappregistry/lib/application-associator.ts

comcalvi · 2023-03-09T22:00:32Z

...alogappregistry/test/integ.application-associator.cross-account-stack-association-enabled.ts

+const app = new cdk.App();
+const localStack = new cdk.Stack(app, 'integ-servicecatalogappregistry-first-resource');
+
+new appreg.ApplicationAssociator(app, 'RegisterCdkApplication', {


the expected way to do these tests is to use your personal account IDs when you deploy it, but switch them to dummy account IDs when you submit the PR.

Understood; as part of this next commit, I took these steps:

Specified env and account properties such that localStack and the Application stack was in Account 1, and crossAccountStack was in Account 2

Set up Accounts 1 and 2 such that they were both in the same AWS organization with cross-account sharing enabled

Modified Account 2's CDK role(s) to allow Account 1 to perform CDK deployments

Ran yarn integ --update-on-failed

After successful deployments of all stacks in their respective accounts which generated the test snapshots, I replaced all mentions of Accounts 1 and 2 ~~with ${AWS::AccountId}~~ with 000000000000 and 111111111111 respectively in the snapshots and removed the env properties in this test file.

…n-associator.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Pull request has been modified.

…aced accountIds with variable

mergify · 2023-03-14T17:16:15Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

jungle-amazon · 2023-03-15T17:09:54Z

@Mergifyio refresh

mergify · 2023-03-15T17:09:58Z

refresh

✅ Pull request refreshed

jungle-amazon · 2023-03-15T17:23:51Z

@Mergifyio refresh

mergify · 2023-03-15T17:23:54Z

refresh

✅ Pull request refreshed

Pull request has been modified.

aws-cdk-automation · 2023-03-15T18:13:29Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 9560876
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-03-15T23:01:54Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

alexpulver · 2023-03-23T07:38:49Z

@jungle-amazon a few follow-up questions if I may 😃.

What is the use case for the below scenario?

If the owner of a cross-account stack is not part of the same AWS Organization as the owner of the ApplicationAssociator stack, or otherwise have not enabled cross-account sharing, during deployment the ApplicationAssociator will fail when attempting to share the application with the stack owner.

How would the developer experience for the below look like?

This relies on the user properly setting up cross-account sharing beforehand.

Let's assume I own an application and deploy it to multiple environments (e.g. alpha, beta, gamma, production US, production EU). Each environment has a dedicated account in the same AWS Organization.

Would this fail by default? Would I need to set up the cross-account sharing?

jungle-amazon · 2023-03-23T22:40:04Z

What is the use case for the below scenario?

If the owner of a cross-account stack is not part of the same AWS Organization as the owner of the ApplicationAssociator stack, or otherwise have not enabled cross-account sharing, during deployment the ApplicationAssociator will fail when attempting to share the application with the stack owner.

The use case is when a CDK app has defined cross-account stacks for deployment. So for example, Account1 will deploy ApplicationAssociator, but Account2 and Account3 have additional CFN stacks that are part of the CDK app definition. The deployment is possible through IAM permissions that allow CDK deployments on Account2 and Account3 by the role that is performing the deployment, but it's possible that those accounts are not part of the same AWS organization as Account1.

By disabling the share behavior, those deployments can still succeed without forcing the stacks to become associated with the target application. The end result is that Account1 deploys ApplicationAssociator's application, and the CFN stacks are deployed in Account2 and Account3, but those stacks are not associated with the application in Account1.

How would the developer experience for the below look like?

This relies on the user properly setting up cross-account sharing beforehand.

For this experience, the owner would set up an AWS organization, and then all the accounts that own a stack defined in the CDK app definition would be invited to be added to the organization. Once the accounts have accepted, they will become part of the organization, and when the ApplicationAssociator owner account deploys the Application stack with the associateCrossAccountStacks option enabled, then the AppRegistry Application will get shared with the stack owning accounts within the organization which then allows the associations with the stack and the shared application.

Let's assume I own an application and deploy it to multiple environments (e.g. alpha, beta, gamma, production US, production EU). Each environment has a dedicated account in the same AWS Organization.
Would this fail by default? Would I need to set up the cross-account sharing?

If I understand this scenario correctly, this is not related to the cross-account scenario we addressed with this change. It seems in this case each AppRegistry application already sits in the same account as the resources getting deployed, so no sharing would be needed or performed.

The scenario is more for handling cross-account stacks defined as part of the same CDK application. For example, if the CDK has defined a CFN-Stack-Account1, CFN-Stack-Account2, and ApplicationAssociator-Account 1. In this case, Account 2 needs to be part of the same AWS organization as Account 1 so that the AppRegistry application created and deployed in Account 1 can be shared with Account 2. In your case, it seems like the definition would instead have CFN-Stack-Account1 and ApplicationAssociator-Account 1, and Account 1 is the one changing based on the environment (alpha, beta, gamma, etc) so there is a dedicated AppRegistry application that depends on the environment. For this, no cross-account setup is needed.

alexpulver · 2023-03-24T13:20:33Z

@jungle-amazon wow, thanks for the detailed reply!

To clarify the scenario I described (sorry that I wasn't clear), by "dedicated account" I meant "different account". So that would be the CFN-Stack-AppRegistry-Account1, CFN-Stack-Component-Account2, CFN-Stack-Component-Account3 use case.

Do I understand correctly that if Account1, Account2, and Account3 belong to the same AWS organization, sharing and association will work out of the box?

jungle-amazon · 2023-03-27T18:54:00Z

@jungle-amazon wow, thanks for the detailed reply!

To clarify the scenario I described (sorry that I wasn't clear), by "dedicated account" I meant "different account". So that would be the CFN-Stack-AppRegistry-Account1, CFN-Stack-Component-Account2, CFN-Stack-Component-Account3 use case.

Do I understand correctly that if Account1, Account2, and Account3 belong to the same AWS organization, sharing and association will work out of the box?

@alexpulver The sharing and association will work but it will be an "opt-in" feature. This means the user has to intentionally set associateCrossAccountStacks to true. Otherwise, the association and sharing will not occur for cross-account stacks by default, accompanied by a warning text that cross-account stacks have been detected but will not be associated.

The default value will be false to accommodate those users who have not set up the AWS organization and sharing, so that their deployments do not fail out of the box.

…ication sharing and association behavior for cross-account stacks (aws#24408) Problem: * Currently, the ApplicationAssociator construct automatically shares the target Application with any accounts of cross-account stacks. [[code reference](https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/stack-associator.ts#L91-L95)] * If the owner of a cross-account stack is not part of the same AWS Organization as the owner of the ApplicationAssociator stack, or otherwise have not enabled cross-account sharing, during deployment the ApplicationAssociator will fail when attempting to share the application with the stack owner, with a message like below: ``` Principal 123456789012 is not in your AWS organization. You do not have permission to add external AWS accounts to a resource share. (Service: AWSRAM; Status Code: 400; Error Code: OperationNotPermittedException; Request ID: aaa; Proxy: null) ``` Feature: * We want to introduce a mechanism (`associateCrossAccountStacks` field in TargetApplicationOptions) where the user can specify if they want to allow sharing their application to any accounts of cross-account stacks in order to then subsequently associate the stack with the application. * This flag will be `false` by default. This allows customers to have their stack deployments proceed without being blocked on application sharing or cross-account associations. * If set to `false`, ApplicationAssociator will skip the application sharing and association for cross-account stacks. During synthesis, a warning will be displayed to notify that cross-account stacks were detected but sharing and association will be skipped. * If set to `true`, the application will be shared and then associated for cross-account stacks. This relies on the user properly setting up cross-account sharing beforehand. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

jungle-amazon and others added 2 commits March 1, 2023 18:55

feat(servicecatalogappregistry-alpha): Introduce flag to control appl…

b5fc246

…ication sharing and association behavior for cross-account scenarios

Merge branch 'aws:main' into share_flag_cross_account

f285a77

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Mar 2, 2023

aws-cdk-automation requested a review from a team March 2, 2023 03:03

github-actions bot added the p2 label Mar 2, 2023

aws-cdk-automation previously requested changes Mar 2, 2023

View reviewed changes

jungle-amazon and others added 3 commits March 2, 2023 11:13

Merge branch 'aws:main' into share_flag_cross_account

2e3b633

Updated README with share flag details

f0c43e7

Added integ test and set flag to 'false' by default

460e406

jungle-amazon commented Mar 3, 2023

View reviewed changes

jungle-amazon added 3 commits March 2, 2023 18:54

Corrected README

e1044d7

Typo

f6376cf

Fixed README format and replaced account id

6fcc98c

jungle-amazon marked this pull request as ready for review March 3, 2023 03:06

comcalvi previously requested changes Mar 3, 2023

View reviewed changes

Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/st…

7c19d9b

…ack-associator.ts Remove unnecessary empty line Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

jungle-amazon and others added 8 commits March 6, 2023 17:05

Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/target-app…

fd977b1

…lication.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/st…

cb7eaa2

…ack-associator.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/st…

b48b2af

…ack-associator.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Rename to associateCrossAccountStacks

c73dae3

Set default to true

Set default behavior back to false

5420730

Updated wording

2e5a1ea

Merge branch 'main' into share_flag_cross_account

2659f52

Fix typos

a6cd4c5

comcalvi previously requested changes Mar 9, 2023

View reviewed changes

Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/applicatio…

b429b54

…n-associator.ts Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

jungle-amazon and others added 3 commits March 9, 2023 15:56

Updated test snapshots after deploying with actual accounts, and repl…

1d05ba9

…aced accountIds with variable

Merge branch 'main' into share_flag_cross_account

776c861

Use 000000000000 and 111111111111 dummy account IDs in snapshot

7c2ec66

comcalvi previously approved these changes Mar 14, 2023

View reviewed changes

Merge branch 'main' into share_flag_cross_account

9560876

comcalvi approved these changes Mar 15, 2023

View reviewed changes

mergify bot merged commit 2167289 into aws:main Mar 15, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(servicecatalogappregistry-alpha): Introduce flag to control application sharing and association behavior for cross-account stacks #24408

feat(servicecatalogappregistry-alpha): Introduce flag to control application sharing and association behavior for cross-account stacks #24408

jungle-amazon commented Mar 2, 2023 •

edited

Loading

aws-cdk-automation left a comment

jungle-amazon Mar 3, 2023

comcalvi Mar 3, 2023

comcalvi Mar 3, 2023

jungle-amazon Mar 7, 2023

comcalvi left a comment

comcalvi Mar 3, 2023

jungle-amazon Mar 7, 2023

jungle-amazon Mar 8, 2023

comcalvi Mar 3, 2023 •

edited

Loading

jungle-amazon Mar 7, 2023

comcalvi Mar 3, 2023

jungle-amazon Mar 7, 2023

comcalvi Mar 3, 2023

comcalvi Mar 3, 2023

jungle-amazon Mar 7, 2023

jungle-amazon commented Mar 7, 2023

comcalvi Mar 9, 2023

jungle-amazon Mar 10, 2023 •

edited

Loading

mergify bot commented Mar 14, 2023

jungle-amazon commented Mar 15, 2023

mergify bot commented Mar 15, 2023

jungle-amazon commented Mar 15, 2023

mergify bot commented Mar 15, 2023

aws-cdk-automation commented Mar 15, 2023

mergify bot commented Mar 15, 2023

alexpulver commented Mar 23, 2023

jungle-amazon commented Mar 23, 2023

alexpulver commented Mar 24, 2023

jungle-amazon commented Mar 27, 2023

feat(servicecatalogappregistry-alpha): Introduce flag to control application sharing and association behavior for cross-account stacks #24408

feat(servicecatalogappregistry-alpha): Introduce flag to control application sharing and association behavior for cross-account stacks #24408

Conversation

jungle-amazon commented Mar 2, 2023 • edited Loading

aws-cdk-automation 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

comcalvi 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

comcalvi Mar 3, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungle-amazon commented Mar 7, 2023

Choose a reason for hiding this comment

jungle-amazon Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

mergify bot commented Mar 14, 2023

jungle-amazon commented Mar 15, 2023

mergify bot commented Mar 15, 2023

✅ Pull request refreshed

jungle-amazon commented Mar 15, 2023

mergify bot commented Mar 15, 2023

✅ Pull request refreshed

aws-cdk-automation commented Mar 15, 2023

AWS CodeBuild CI Report

mergify bot commented Mar 15, 2023

alexpulver commented Mar 23, 2023

jungle-amazon commented Mar 23, 2023

alexpulver commented Mar 24, 2023

jungle-amazon commented Mar 27, 2023

jungle-amazon commented Mar 2, 2023 •

edited

Loading

comcalvi Mar 3, 2023 •

edited

Loading

jungle-amazon Mar 10, 2023 •

edited

Loading