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.

Sign up for GitHub

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

Jump to bottom

fix(servicecatalogappregistry): synth error when associating a nested stack #23248

Merged
merged 6 commits into from Dec 14, 2022
Merged

fix(servicecatalogappregistry): synth error when associating a nested stack #23248

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
99b886e
Adding CfnOutput with App manager URL, handling conditonal nested stacks
sangosh Dec 6, 2022
140da6e
Changing integration test
sangosh Dec 6, 2022
0d7940f
Adding description for AM url
sangosh Dec 8, 2022
2f065d6
Merge branch 'main' into AppRegistry_L2_AppManagerUrl
santanugho Dec 12, 2022
72a2683
Removing un-necessary condition from resource association
sangosh Dec 13, 2022
2c9eba0
Merge branch 'main' into AppRegistry_L2_AppManagerUrl
mergify[bot] Dec 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 24 additions & 5 deletions packages/@aws-cdk/aws-servicecatalogappregistry/lib/application.ts
View file
Open in desktop
Expand Up @@ -34,6 +34,12 @@ export interface IApplication extends cdk.IResource {
*/
readonly applicationName?: string;

/**
* Application manager URL for the Application.
* @attribute
*/
readonly applicationManagerUrl?: cdk.CfnOutput;

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't put it on IApplication. By its very nature it's optional, which is not very useful.

More useful to put it definitely on Application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the new revision.

/**
* Associate this application with an attribute group.
*
Expand Down Expand Up @@ -94,6 +100,7 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
public abstract readonly applicationArn: string;
public abstract readonly applicationId: string;
public abstract readonly applicationName?: string;
public abstract readonly applicationManagerUrl?: cdk.CfnOutput;
private readonly associatedAttributeGroups: Set<string> = new Set();
private readonly associatedResources: Set<string> = new Set();

Expand Down Expand Up @@ -134,20 +141,26 @@ abstract class ApplicationBase extends cdk.Resource implements IApplication {
/**
* Associate stack with the application in the stack passed as parameter.
*
* If the stack is already associated, it will ignore duplicate request.
* A stack can only be associated with one application.
*/
public associateApplicationWithStack(stack: cdk.Stack): void {
if (!this.associatedResources.has(stack.node.addr)) {
new CfnResourceAssociation(stack, 'AppRegistryAssociation', {
const stackCondition = stack.nestedStackResource?.cfnOptions.condition;
const association = new CfnResourceAssociation(stack, 'AppRegistryAssociation', {
application: stack === cdk.Stack.of(this) ? this.applicationId : this.applicationName ?? this.applicationId,
resource: stack.stackId,
resourceType: 'CFN_STACK',
});

this.associatedResources.add(stack.node.addr);
if (stack !== cdk.Stack.of(this) && this.isSameAccount(stack) && !this.isStageScope(stack)) {
stack.addDependency(cdk.Stack.of(this));
if (stackCondition) {
association.addOverride('Condition', stackCondition.logicalId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? If the ResourceAssociation is INSIDE the nested stack, and the nested stack doesn't get created, then the resourceassociation wouldn't be created?

Please describe in the PR body and title, from a user's point of view, what the situation and error were.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you have described is the exact situation here, AWS Solutions team had a stack with conditional nested stack. While they used our L2 construct, the association failed as the condition evaluated to be false whereas our L2 construct previously couldnt handle that condition in ResourceAssociation. This is a fix for the same. I will add this detail to the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that it has something to do with putting conditions on NestedStacks, but you missed one part of my question: if I recall the design is for the ResourceAssocation constructs to be INSIDE the stacks that are being associated.

  • If that is the case, there is no need to associate the condition with the association.
  • If that is NOT the case, I think you might be fixing the wrong bug (i.e., it would be a more correct fix to move the association to inside the stack).

Allow me to explain with a diagram. This is what I think should be going on, purely looking at the CloudFormation template level:

image

If that is not what is going on (if instead the ResourceAssociation is in ParentStack instead of ChildStack), then:

  • We need to fix that; or
  • We need an explanation for why the pattern is different for nested stacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, the diagram is correct and the problem never happens for the new ApplicationAssociator construct. However, this problem occurs only if customers try to explicitly associate a nested child stack like below:

const application = new appreg.Application(stack, 'AutoApplication', {
    applicationName: 'MyAutoApplication',
});
application.associateApplicationWithStack(parentStack);
application.associateApplicationWithStack(childStack); // this is where it fails as the stack itself has not been created and our service throws the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you are right, since associateApplicationWithStack is creating the association inside child stack, we dont need to add the condition for ResourceAssociation.

}

if (!stack.nested) {
this.associatedResources.add(stack.node.addr);
if (stack !== cdk.Stack.of(this) && this.isSameAccount(stack) && !this.isStageScope(stack)) {
stack.addDependency(cdk.Stack.of(this));
}
}
}
}
Expand Down Expand Up @@ -240,6 +253,7 @@ export class Application extends ApplicationBase {
public readonly applicationArn = applicationArn;
public readonly applicationId = applicationId!;
public readonly applicationName = undefined;
public readonly applicationManagerUrl = undefined;

protected generateUniqueHash(resourceAddress: string): string {
return hashValues(this.applicationArn, resourceAddress);
Expand All @@ -254,6 +268,7 @@ export class Application extends ApplicationBase {
public readonly applicationArn: string;
public readonly applicationId: string;
public readonly applicationName?: string;
public readonly applicationManagerUrl?: cdk.CfnOutput;
private readonly nodeAddress: string;

constructor(scope: Construct, id: string, props: ApplicationProps) {
Expand All @@ -270,6 +285,10 @@ export class Application extends ApplicationBase {
this.applicationId = application.attrId;
this.applicationName = props.applicationName;
this.nodeAddress = cdk.Names.nodeUniqueId(application.node);

this.applicationManagerUrl = new cdk.CfnOutput(this, 'ApplicationManagerUrl', {
value: `https://${this.env.region}.console.aws.amazon.com/systems-manager/appmanager/application/AWS_AppRegistry_Application-${this.applicationName}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a description as well. Include the construct path in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in the new revision.

});
}

protected generateUniqueHash(resourceAddress: string): string {
Expand Down
68 changes: 61 additions & 7 deletions packages/@aws-cdk/aws-servicecatalogappregistry/test/application.test.ts
View file
Open in desktop
Expand Up @@ -34,11 +34,13 @@ describe('Application', () => {

test('application with explicit description', () => {
const description = 'my test application description';
new appreg.Application(stack, 'MyApplication', {
const application = new appreg.Application(stack, 'MyApplication', {
applicationName: 'testApplication',
description: description,
});

Template.fromStack(stack).hasOutput('MyApplicationApplicationManagerUrlB79EF34D', {});
expect(application.applicationManagerUrl?.value).toContain('AWS_AppRegistry_Application-testApplication');
Template.fromStack(stack).hasResourceProperties('AWS::ServiceCatalogAppRegistry::Application', {
Description: description,
});
Expand Down Expand Up @@ -254,7 +256,7 @@ describe('Application', () => {

Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
AllowExternalPrincipals: false,
Name: 'RAMSharee6e0e560e6f8',
Name: 'RAMShare5bb637032063',
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
Expand All @@ -268,7 +270,7 @@ describe('Application', () => {

Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
AllowExternalPrincipals: false,
Name: 'RAMSharee6e0e560e6f8',
Name: 'RAMShare5bb637032063',
Principals: ['123456789012'],
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
Expand All @@ -284,7 +286,7 @@ describe('Application', () => {

Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
AllowExternalPrincipals: false,
Name: 'RAMSharee6e0e560e6f8',
Name: 'RAMShare5bb637032063',
Principals: ['arn:aws:iam::123456789012:role/myRole'],
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
Expand All @@ -300,7 +302,7 @@ describe('Application', () => {

Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
AllowExternalPrincipals: false,
Name: 'RAMSharee6e0e560e6f8',
Name: 'RAMShare5bb637032063',
Principals: ['arn:aws:iam::123456789012:user/myUser'],
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
Expand All @@ -315,7 +317,7 @@ describe('Application', () => {

Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
AllowExternalPrincipals: false,
Name: 'RAMSharee6e0e560e6f8',
Name: 'RAMShare5bb637032063',
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly'],
Expand All @@ -330,7 +332,7 @@ describe('Application', () => {

Template.fromStack(stack).hasResourceProperties('AWS::RAM::ResourceShare', {
AllowExternalPrincipals: false,
Name: 'RAMSharee6e0e560e6f8',
Name: 'RAMShare5bb637032063',
Principals: ['arn:aws:organizations::123456789012:organization/o-70oi5564q1'],
ResourceArns: [{ 'Fn::GetAtt': ['MyApplication5C63EC1D', 'Arn'] }],
PermissionArns: ['arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationAllowAssociation'],
Expand Down Expand Up @@ -446,8 +448,60 @@ describe('Scope based Associations with Application with Cross Region/Account',
});
});

describe('Conditional nested stack Associations with Application within Same Account', () => {
let app: cdk.App;
beforeEach(() => {
app = new cdk.App({
context: {
'@aws-cdk/core:newStyleStackSynthesis': false,
},
});
});

test('Associate conditional nested stack with application', () => {
const stack = new MainStack(app, 'cdkApplication');
const application = new appreg.Application(stack, 'MyApplication', {
applicationName: 'MyApplication',
});
application.associateApplicationWithStack(stack);
application.associateApplicationWithStack(stack.nestedStack);
Template.fromStack(stack.nestedStack).hasResource('AWS::ServiceCatalogAppRegistry::ResourceAssociation', {
Properties: {
Application: 'MyApplication',
Resource: { Ref: 'AWS::StackId' },
ResourceType: 'CFN_STACK',
},
Condition: 'ShouldCreateStackCondition',
});
Template.fromStack(stack.nestedStack).hasCondition('ShouldCreateStackCondition', {
'Fn::Equals': ['us-east-1'],
});
});

});


class AppRegistrySampleStack extends cdk.Stack {
public constructor(scope: Construct, id: string, props?: cdk.StackProps) {
super(scope, id, props);
}
}

class MainStack extends cdk.Stack {
public readonly nestedStack: cdk.Stack;
public constructor(parent: cdk.App, id: string, props?: cdk.StackProps) {
super(parent, id, props);
this.nestedStack = new AppRegistryNestedStack(this, 'nested-stack');
}
}

class AppRegistryNestedStack extends cdk.NestedStack {
public constructor(scope: Construct, id: string, props?: cdk.NestedStackProps) {
super(scope, id, props);

const shouldCreateStack = new cdk.CfnCondition(this, 'ShouldCreateStackCondition', {
expression: cdk.Fn.conditionEquals(process.env.CDK_DEFAULT_REGION, 'us-east-1'),
});
(this.nestedStackResource as cdk.CfnStack).cfnOptions.condition = shouldCreateStack;
}
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/cdk.out
View file
Open in desktop
@@ -1 +1 @@
{"version":"21.0.0"}
{"version":"22.0.0"}
6 changes: 3 additions & 3 deletions ...est/integ.application.js.snapshot/integ-servicecatalogappregistry-application.assets.json
View file
Open in desktop
@@ -1,15 +1,15 @@
{
"version": "20.0.0",
"version": "22.0.0",
"files": {
"d03aa6239eb3b20f4b72fb3dd44a4082d06d7a5451d0ac3855bd1aa78aecfbe9": {
"98d56556ece0e12b5e819d33e303424de1f579cdc95dd798bb2ee407ab2bda89": {
"source": {
"path": "integ-servicecatalogappregistry-application.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "d03aa6239eb3b20f4b72fb3dd44a4082d06d7a5451d0ac3855bd1aa78aecfbe9.json",
"objectKey": "98d56556ece0e12b5e819d33e303424de1f579cdc95dd798bb2ee407ab2bda89.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
24 changes: 20 additions & 4 deletions ...t/integ.application.js.snapshot/integ-servicecatalogappregistry-application.template.json
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@
"TestApplication2FBC585F": {
"Type": "AWS::ServiceCatalogAppRegistry::Application",
"Properties": {
"Name": "myApplicationtest",
"Name": "myCdkApplication",
"Description": "my application description"
}
},
Expand Down Expand Up @@ -39,10 +39,10 @@
}
}
},
"TestApplicationRAMSharead8ba81b8cdd40199FD1": {
"TestApplicationRAMShare3dc6227daec11BF3E108": {
"Type": "AWS::RAM::ResourceShare",
"Properties": {
"Name": "RAMSharead8ba81b8cdd",
"Name": "RAMShare3dc6227daec1",
"AllowExternalPrincipals": false,
"PermissionArns": [
"arn:aws:ram::aws:permission/AWSRAMPermissionServiceCatalogAppRegistryApplicationReadOnly"
Expand Down Expand Up @@ -84,7 +84,7 @@
"release": "go time"
}
},
"Name": "myAttributeGroupTest",
"Name": "myCdkAttributeGroup",
"Description": "my attribute group description"
}
},
Expand Down Expand Up @@ -121,6 +121,22 @@
}
}
},
"Outputs": {
"TestApplicationApplicationManagerUrlE1058321": {
"Value": {
"Fn::Join": [
"",
[
"https://",
{
"Ref": "AWS::Region"
},
".console.aws.amazon.com/systems-manager/appmanager/application/AWS_AppRegistry_Application-myCdkApplication"
]
]
}
}
},
"Parameters": {
"BootstrapVersion": {
"Type": "AWS::SSM::Parameter::Value<String>",
Expand Down
2 changes: 1 addition & 1 deletion ...ages/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/integ.json
View file
Open in desktop
@@ -1,5 +1,5 @@
{
"version": "20.0.0",
"version": "22.0.0",
"testCases": {
"integ.application": {
"stacks": [
Expand Down
26 changes: 16 additions & 10 deletions ...s/@aws-cdk/aws-servicecatalogappregistry/test/integ.application.js.snapshot/manifest.json
View file
Open in desktop
@@ -1,12 +1,6 @@
{
"version": "20.0.0",
"version": "22.0.0",
"artifacts": {
"Tree": {
"type": "cdk:tree",
"properties": {
"file": "tree.json"
}
},
"integ-servicecatalogappregistry-application.assets": {
"type": "cdk:asset-manifest",
"properties": {
Expand All @@ -23,7 +17,7 @@
"validateOnSynth": false,
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/d03aa6239eb3b20f4b72fb3dd44a4082d06d7a5451d0ac3855bd1aa78aecfbe9.json",
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/98d56556ece0e12b5e819d33e303424de1f579cdc95dd798bb2ee407ab2bda89.json",
"requiresBootstrapStackVersion": 6,
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
"additionalDependencies": [
Expand All @@ -45,6 +39,12 @@
"data": "TestApplication2FBC585F"
}
],
"/integ-servicecatalogappregistry-application/TestApplication/ApplicationManagerUrl": [
{
"type": "aws:cdk:logicalId",
"data": "TestApplicationApplicationManagerUrlE1058321"
}
],
"/integ-servicecatalogappregistry-application/TestApplication/ResourceAssociationd232b63e52a8": [
{
"type": "aws:cdk:logicalId",
Expand All @@ -57,10 +57,10 @@
"data": "TestApplicationAttributeGroupAssociation4ba7f5842818B8EE1C6F"
}
],
"/integ-servicecatalogappregistry-application/TestApplication/RAMSharead8ba81b8cdd": [
"/integ-servicecatalogappregistry-application/TestApplication/RAMShare3dc6227daec1": [
{
"type": "aws:cdk:logicalId",
"data": "TestApplicationRAMSharead8ba81b8cdd40199FD1"
"data": "TestApplicationRAMShare3dc6227daec11BF3E108"
}
],
"/integ-servicecatalogappregistry-application/TestAttributeGroup/Resource": [
Expand Down Expand Up @@ -89,6 +89,12 @@
]
},
"displayName": "integ-servicecatalogappregistry-application"
},
"Tree": {
"type": "cdk:tree",
"properties": {
"file": "tree.json"
}
}
}
}