-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Changes from 5 commits
b5fc246
f285a77
2e3b633
f0c43e7
460e406
e1044d7
f6376cf
6fcc98c
7c19d9b
fd977b1
cb7eaa2
b48b2af
c73dae3
5420730
2e5a1ea
2659f52
a6cd4c5
b429b54
1d05ba9
776c861
7c2ec66
9560876
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
* | ||
|
@@ -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(); | ||
|
||
|
@@ -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); | ||
} | ||
|
||
|
@@ -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. | ||
|
@@ -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.'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add this to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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; | ||
} | ||
|
||
|
||
|
@@ -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; | ||
} | ||
|
||
/** | ||
|
@@ -108,6 +120,7 @@ class CreateTargetApplication extends TargetApplication { | |
|
||
return { | ||
application: appRegApplication, | ||
enableCrossAccountStacks: this.applicationOptions.enableCrossAccountStacks ?? false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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. |
||
}; | ||
} | ||
} | ||
|
@@ -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 |
---|---|---|
@@ -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": { | ||
"689943422764-us-east-1": { | ||
"bucketName": "cdk-hnb659fds-assets-689943422764-us-east-1", | ||
"objectKey": "a1890781394947f4a79fb1f55a5bd79d964364631d75cceb228c576a071c908d.json", | ||
"region": "us-east-1", | ||
"assumeRoleArn": "arn:${AWS::Partition}:iam::689943422764:role/cdk-hnb659fds-file-publishing-role-689943422764-us-east-1" | ||
} | ||
} | ||
} | ||
}, | ||
"dockerImages": {} | ||
} |
There was a problem hiding this comment.
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 makeenableCrossAccount
true
by default on theStageStackAssociator
?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
onStageStackAssociator
because it is more likely that customers have not setup properly for cross-account cases. Setting it tofalse
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 wantassociateAllStacksInScope()
to include cross-account stacks, and in this case the customer would be explicitly associating all stacks by calling this method.