Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 22 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
b5fc246
feat(servicecatalogappregistry-alpha): Introduce flag to control appl…
jungle-amazon Feb 23, 2023
f285a77
Merge branch 'aws:main' into share_flag_cross_account
jungle-amazon Mar 2, 2023
2e3b633
Merge branch 'aws:main' into share_flag_cross_account
jungle-amazon Mar 2, 2023
f0c43e7
Updated README with share flag details
jungle-amazon Mar 2, 2023
460e406
Added integ test and set flag to 'false' by default
jungle-amazon Mar 3, 2023
e1044d7
Corrected README
jungle-amazon Mar 3, 2023
f6376cf
Typo
jungle-amazon Mar 3, 2023
6fcc98c
Fixed README format and replaced account id
jungle-amazon Mar 3, 2023
7c19d9b
Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/st…
jungle-amazon Mar 7, 2023
fd977b1
Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/target-app…
jungle-amazon Mar 7, 2023
cb7eaa2
Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/st…
jungle-amazon Mar 7, 2023
b48b2af
Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/st…
jungle-amazon Mar 7, 2023
c73dae3
Rename to associateCrossAccountStacks
jungle-amazon Mar 7, 2023
5420730
Set default behavior back to false
jungle-amazon Mar 8, 2023
2e5a1ea
Updated wording
jungle-amazon Mar 8, 2023
2659f52
Merge branch 'main' into share_flag_cross_account
jungle-amazon Mar 8, 2023
a6cd4c5
Fix typos
jungle-amazon Mar 8, 2023
b429b54
Update packages/@aws-cdk/aws-servicecatalogappregistry/lib/applicatio…
jungle-amazon Mar 9, 2023
1d05ba9
Updated test snapshots after deploying with actual accounts, and repl…
jungle-amazon Mar 9, 2023
776c861
Merge branch 'main' into share_flag_cross_account
jungle-amazon Mar 10, 2023
7c2ec66
Use 000000000000 and 111111111111 dummy account IDs in snapshot
jungle-amazon Mar 10, 2023
9560876
Merge branch 'main' into share_flag_cross_account
jungle-amazon Mar 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/@aws-cdk/aws-servicecatalogappregistry/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,24 @@ const cdkPipeline = new ApplicationPipelineStack(app, 'CDKApplicationPipelineSta
});
```

By default, ApplicationAssociator will not perform cross-account stack associations with the target Application,
to avoid deployment failures for accounts which have not been setup for cross-account associations.
To enable cross-account stack associations, make sure all accounts are in the same organization as the
target Application's account and that resource sharing is enabled within the organization.
If you wish to turn on cross-account sharing and associations, set the `associateCrossAccountStacks` field to `true`,
as shown in the example below:

```ts
const app = new App();
const associatedApp = new appreg.ApplicationAssociator(app, 'AssociatedApplication', {
applications: [appreg.TargetApplication.createApplicationStack({
associateCrossAccountStacks: true,
applicationName: 'MyAssociatedApplication',
env: { account: '123456789012', region: 'us-east-1' },
})],
});
```

## Attribute Group

An AppRegistry attribute group acts as a container for user-defined attributes for an application.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ export interface ApplicationAssociatorProps {
* in case of a `Pipeline` stack, stage underneath the pipeline will not automatically be associated and
* needs to be associated separately.
*
* If cross account stack is detected, then this construct will automatically share the application to consumer accounts.
* If cross account stack is detected and `associateCrossAccountStacks` in `TargetApplicationOptions` is `true`,
* then the application will automatically be shared with the consumer accounts to allow associations.
* Otherwise, the application will not be shared.
* Cross account feature will only work for non environment agnostic stacks.
*/
export class ApplicationAssociator extends Construct {
Expand All @@ -33,6 +35,7 @@ export class ApplicationAssociator extends Construct {
*/
private readonly application: IApplication;
private readonly associatedStages: Set<cdk.Stage> = new Set();
private readonly associateCrossAccountStacks?: boolean;

constructor(scope: cdk.App, id: string, props: ApplicationAssociatorProps) {
super(scope, id);
Expand All @@ -42,8 +45,12 @@ export class ApplicationAssociator extends Construct {
}

const targetApplication = props.applications[0];
this.application = targetApplication.bind(scope).application;
cdk.Aspects.of(scope).add(new CheckedStageStackAssociator(this));
const targetBindResult = targetApplication.bind(scope);
this.application = targetBindResult.application;
this.associateCrossAccountStacks = targetBindResult.associateCrossAccountStacks;
cdk.Aspects.of(scope).add(new CheckedStageStackAssociator(this, {
associateCrossAccountStacks: this.associateCrossAccountStacks,
}));
}

/**
Expand All @@ -52,7 +59,9 @@ export class ApplicationAssociator extends Construct {
*/
public associateStage(stage: cdk.Stage): cdk.Stage {
this.associatedStages.add(stage);
cdk.Aspects.of(stage).add(new CheckedStageStackAssociator(this));
cdk.Aspects.of(stage).add(new CheckedStageStackAssociator(this, {
associateCrossAccountStacks: this.associateCrossAccountStacks,
}));
return stage;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,16 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
}

/**
* Associate all stacks present in construct's aspect with application.
* Associate all stacks present in construct's aspect with application, including cross-account stacks.
*
* 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, {
associateCrossAccountStacks: true,
}));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@ import { ApplicationAssociator } from '../application-associator';
import { SharePermission } from '../common';
import { isRegionUnresolved, isAccountUnresolved } from '../private/utils';

export interface StackAssociatorBaseProps {
/**
* Indicates if the target Application should be shared with the cross-account stack owners and then
* associated with the cross-account stacks.
*
* @default - false
jungle-amazon marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly associateCrossAccountStacks?: boolean;
}

/**
* Aspect class, this will visit each node from the provided construct once.
*
Expand All @@ -14,9 +24,14 @@ import { isRegionUnresolved, isAccountUnresolved } from '../private/utils';
abstract class StackAssociatorBase implements IAspect {
protected abstract readonly application: IApplication;
protected abstract readonly applicationAssociator?: ApplicationAssociator;
protected readonly associateCrossAccountStacks?: boolean;

protected readonly sharedAccounts: Set<string> = new Set();

constructor(props?: StackAssociatorBaseProps) {
this.associateCrossAccountStacks = props?.associateCrossAccountStacks ?? false;
}

public visit(node: IConstruct): void {
// verify if a stage in a particular stack is associated to Application.
node.node.children.forEach((childNode) => {
Expand All @@ -42,6 +57,13 @@ abstract class StackAssociatorBase implements IAspect {
* @param node A Stage stack.
*/
private associate(node: Stack): void {
if (!isRegionUnresolved(this.application.env.region, node.region)
&& node.account != this.application.env.account
&& !this.associateCrossAccountStacks) {
// Skip association when cross-account sharing/association is not enabled.
// A warning will have been displayed as part of `handleCrossAccountStack()`.
return;
}
this.application.associateApplicationWithStack(node);
}

Expand Down Expand Up @@ -77,7 +99,7 @@ abstract class StackAssociatorBase implements IAspect {

/**
* Handle cross-account association.
* If any stack is evaluated as cross-account than that of application,
* If any stack is evaluated as cross-account than that of application, and cross-account option is enabled,
* then we will share the application to the stack owning account.
*
* @param node Cfn stack.
Expand All @@ -89,12 +111,17 @@ abstract class StackAssociatorBase implements IAspect {
}

if (node.account != this.application.env.account && !this.sharedAccounts.has(node.account)) {
this.application.shareApplication({
accounts: [node.account],
sharePermission: SharePermission.ALLOW_ACCESS,
});
if (this.associateCrossAccountStacks) {
this.application.shareApplication({
accounts: [node.account],
sharePermission: SharePermission.ALLOW_ACCESS,
});

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.');
Copy link
Contributor

@comcalvi comcalvi Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}
}
}
}
Expand All @@ -103,8 +130,8 @@ export class CheckedStageStackAssociator extends StackAssociatorBase {
protected readonly application: IApplication;
protected readonly applicationAssociator?: ApplicationAssociator;

constructor(app: ApplicationAssociator) {
super();
constructor(app: ApplicationAssociator, props?: StackAssociatorBaseProps) {
super(props);
this.application = app.appRegistryApplication();
this.applicationAssociator = app;
}
Expand All @@ -114,8 +141,8 @@ export class StageStackAssociator extends StackAssociatorBase {
protected readonly application: IApplication;
protected readonly applicationAssociator?: ApplicationAssociator;

constructor(app: IApplication) {
super();
constructor(app: IApplication, props?: StackAssociatorBaseProps) {
super(props);
this.application = app;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ export interface TargetApplicationCommonOptions extends cdk.StackProps {
* @deprecated - Use `stackName` instead to control the name and id of the stack
*/
readonly stackId?: string;

/**
* Determines whether any cross-account stacks defined in the CDK app definition should be associated with the
* target application. If set to `true`, the application will first be shared with the accounts that own the stacks.
*
* @default - false
jungle-amazon marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly associateCrossAccountStacks?: boolean;
}


Expand Down Expand Up @@ -87,6 +95,10 @@ export interface BindTargetApplicationResult {
* Created or imported application.
*/
readonly application: IApplication;
/**
* Enables cross-account associations with the target application.
*/
readonly associateCrossAccountStacks: boolean;
}

/**
Expand Down Expand Up @@ -124,6 +136,7 @@ class CreateTargetApplication extends TargetApplication {

return {
application: appRegApplication,
associateCrossAccountStacks: this.applicationOptions.associateCrossAccountStacks ?? false,
};
}
}
Expand All @@ -144,6 +157,7 @@ class ExistingTargetApplication extends TargetApplication {
const appRegApplication = Application.fromApplicationArn(applicationStack, 'ExistingApplication', this.applicationOptions.applicationArnValue);
return {
application: appRegApplication,
associateCrossAccountStacks: this.applicationOptions.associateCrossAccountStacks ?? false,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,38 @@ describe('Scope based Associations with Application with Cross Region/Account',
});
}),

test('ApplicationAssociator with cross region stacks inside cdkApp throws error', () => {
test('ApplicationAssociator with cross account stacks inside cdkApp gives warning if associateCrossAccountStacks is not provided', () => {
new appreg.ApplicationAssociator(app, 'MyApplication', {
applications: [appreg.TargetApplication.createApplicationStack({
applicationName: 'MyAssociatedApplication',
stackName: 'MyAssociatedApplicationStack',
env: { account: 'account2', region: 'region' },
})],
});

const crossAccountStack = new cdk.Stack(app, 'crossRegionStack', {
env: { account: 'account', region: 'region' },
});
Annotations.fromStack(crossAccountStack).hasWarning('*', 'Cross-account stack detected but application sharing and association will be skipped because cross-account option is not enabled.');
});

test('ApplicationAssociator with cross account stacks inside cdkApp does not give warning if associateCrossAccountStacks is set to true', () => {
new appreg.ApplicationAssociator(app, 'MyApplication', {
applications: [appreg.TargetApplication.createApplicationStack({
applicationName: 'MyAssociatedApplication',
stackName: 'MyAssociatedApplicationStack',
associateCrossAccountStacks: true,
env: { account: 'account', region: 'region' },
})],
});

const crossAccountStack = new cdk.Stack(app, 'crossRegionStack', {
env: { account: 'account2', region: 'region' },
});
Annotations.fromStack(crossAccountStack).hasNoWarning('*', 'Cross-account stack detected but application sharing and association will be skipped because cross-account option is not enabled.');
});

test('ApplicationAssociator with cross region stacks inside cdkApp gives warning', () => {
new appreg.ApplicationAssociator(app, 'MyApplication', {
applications: [appreg.TargetApplication.createApplicationStack({
applicationName: 'MyAssociatedApplication',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"version": "31.0.0",
"files": {
"19dd33f3c17e59cafd22b9459b0a8d9bedbd42252737fedb06b2bcdbcf7809cc": {
"source": {
"path": "ApplicationAssociatorTestDefaultTestDeployAssert2A5F2DB9.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "19dd33f3c17e59cafd22b9459b0a8d9bedbd42252737fedb06b2bcdbcf7809cc.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
{
"Resources": {
"AppRegistryAssociation": {
"Type": "AWS::ServiceCatalogAppRegistry::ResourceAssociation",
"Properties": {
"Application": "AppRegistryAssociatedApplication",
"Resource": {
"Ref": "AWS::StackId"
},
"ResourceType": "CFN_STACK"
}
}
},
"Parameters": {
"BootstrapVersion": {
"Type": "AWS::SSM::Parameter::Value<String>",
"Default": "/cdk-bootstrap/hnb659fds/version",
"Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
}
},
"Rules": {
"CheckBootstrapVersion": {
"Assertions": [
{
"Assert": {
"Fn::Not": [
{
"Fn::Contains": [
[
"1",
"2",
"3",
"4",
"5"
],
{
"Ref": "BootstrapVersion"
}
]
}
]
},
"AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
}
]
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"version": "31.0.0",
"files": {
"1db9045d198a45233cf79d4f87770e1d94d5efd69f70a11d40e3a6310ba3b26c": {
"source": {
"path": "TestAppRegistryApplicationStack.template.json",
"packaging": "file"
},
"destinations": {
"000000000000-current_region": {
"bucketName": "cdk-hnb659fds-assets-000000000000-${AWS::Region}",
"objectKey": "1db9045d198a45233cf79d4f87770e1d94d5efd69f70a11d40e3a6310ba3b26c.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::000000000000:role/cdk-hnb659fds-file-publishing-role-000000000000-${AWS::Region}"
}
}
}
},
"dockerImages": {}
}
Loading