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 8 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 @@ -139,6 +139,24 @@ const cdkPipeline = new ApplicationPipelineStack(app, 'CDKApplicationPipelineSta
});
```

By default, ApplicationAssociator will not perform cross-account stack association with the target Application.
If you want to turn on this behavior, set the `enableCrossAccountStacks` 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({
enableCrossAccountStacks: true,
applicationName: 'MyAssociatedApplication',
env: { account: '123456789012', region: 'us-east-1' },
})],
});
```

This will also share the Application with the accounts of any cross-account stacks defined in the
CDK app scope so that the association becomes possible.

## 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, then this construct will skip those associations by default. To edit
* this behavior, set the `enableCrossAccountStacks` value in TargetApplicationOptions. If set to `true`,
* the application will also be automatically shared with the consumer accounts.
* 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 enableCrossAccountStacks?: 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.enableCrossAccountStacks = targetBindResult.enableCrossAccountStacks;
cdk.Aspects.of(scope).add(new CheckedStageStackAssociator(this, {
enableCrossAccount: this.enableCrossAccountStacks,
}));
}

/**
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, {
enableCrossAccount: this.enableCrossAccountStacks,
}));
return stage;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,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, {
enableCrossAccount: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think defaulting to true makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}));
}

/**
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 enableCrossAccount?: boolean;
}

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

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

Expand Down Expand Up @@ -42,6 +53,13 @@ abstract class StackAssociatorBase implements IAspect {
* @param node A Stage stack.
*/
private associate(node: Stack): void {
if (!isAccountUnresolved(this.application.env.account!, node.account)
&& node.account != this.application.env.account
&& !this.enableCrossAccount) {
// Skip association when cross-account stack is detected but cross-account sharing/association is not enabled.
// A warning would have been displayed as part of `handleCrossAccountStack()`.
return;
}
jungle-amazon marked this conversation as resolved.
Show resolved Hide resolved
this.application.associateApplicationWithStack(node);
}

Expand Down Expand Up @@ -77,7 +95,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,33 +107,43 @@ 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.enableCrossAccount) {
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;
}
}
}
}

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

constructor(app: ApplicationAssociator) {
constructor(app: ApplicationAssociator, props?: StackAssociatorBaseProps) {
super();
this.application = app.appRegistryApplication();
this.applicationAssociator = app;
this.enableCrossAccount = props?.enableCrossAccount ?? false;
}

jungle-amazon marked this conversation as resolved.
Show resolved Hide resolved
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


constructor(app: IApplication) {
constructor(app: IApplication, props?: StackAssociatorBaseProps) {
super();
this.application = app;
this.enableCrossAccount = props?.enableCrossAccount ?? false;
}
}
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 be shared with the accounts that own the stacks.
*
* @default - false
jungle-amazon marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly enableCrossAccountStacks?: boolean;
}


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

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

return {
application: appRegApplication,
enableCrossAccountStacks: this.applicationOptions.enableCrossAccountStacks ?? false,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

};
}
}
Expand All @@ -128,6 +141,7 @@ class ExistingTargetApplication extends TargetApplication {
const appRegApplication = Application.fromApplicationArn(applicationStack, 'ExistingApplication', this.applicationOptions.applicationArnValue);
return {
application: appRegApplication,
enableCrossAccountStacks: this.applicationOptions.enableCrossAccountStacks ?? false,
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,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 enableCrossAccountStacks 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 gives warning if enableCrossAccountStacks is set to true', () => {
new appreg.ApplicationAssociator(app, 'MyApplication', {
applications: [appreg.TargetApplication.createApplicationStack({
applicationName: 'MyAssociatedApplication',
stackName: 'MyAssociatedApplicationStack',
enableCrossAccountStacks: 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": "30.1.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,20 @@
{
"version": "30.1.0",
"files": {
"a1890781394947f4a79fb1f55a5bd79d964364631d75cceb228c576a071c908d": {
"source": {
"path": "TestAppRegistryApplicationStack.template.json",
"packaging": "file"
},
"destinations": {
"87654321-us-east-1": {
"bucketName": "cdk-hnb659fds-assets-87654321-us-east-1",
"objectKey": "a1890781394947f4a79fb1f55a5bd79d964364631d75cceb228c576a071c908d.json",
"region": "us-east-1",
"assumeRoleArn": "arn:${AWS::Partition}:iam::87654321:role/cdk-hnb659fds-file-publishing-role-87654321-us-east-1"
}
}
}
},
"dockerImages": {}
}
Loading