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

santanugho · 2022-12-06T18:43:39Z

In this PR, we are fixing the following:

For nested stack association, customers have observed a synth error Nested stack cannot depend on a parent stack. In this PR we are providing the fix for the same.
Adding Application manager URL as CDK output. Customers will now have ability to directly access Application Manager for the application created by AppRegistry L2 construct using the link provide in CDK and CFN output.
Unit test to verify conditional nested stack association can be handled by this L2 construct gracefully as the ResourceAssociation is getting created within the child stack.

All Submissions:

[ X] Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-12-06T18:43:45Z

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.

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

rix0rrr · 2022-12-08T08:58:47Z

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

-      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);


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.

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.

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:

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.

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.

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

rix0rrr · 2022-12-08T09:00:27Z

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

@@ -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}`,


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

Addressed in the new revision.

rix0rrr · 2022-12-08T09:01:08Z

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

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


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.

Addressed in the new revision.

Pull request has been modified.

rix0rrr · 2022-12-13T08:41:02Z

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

-      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);


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:

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.

Pull request has been modified.

mergify · 2022-12-14T17:11:25Z

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).

aws-cdk-automation · 2022-12-14T17:50:06Z

AWS CodeBuild CI Report

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

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

mergify · 2022-12-14T17:50:20Z

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).

… stack (aws#23248) In this PR, we are fixing the following: - For nested stack association, customers have observed a synth error `Nested stack cannot depend on a parent stack`. In this PR we are providing the fix for the same. - Adding Application manager URL as CDK output. Customers will now have ability to directly access Application Manager for the application created by AppRegistry L2 construct using the link provide in CDK and CFN output. - Unit test to verify conditional nested stack association can be handled by this L2 construct gracefully as the `ResourceAssociation` is getting created within the child stack. ### All Submissions: * [ X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Adding CfnOutput with App manager URL, handling conditonal nested stacks

99b886e

aws-cdk-automation requested a review from a team December 6, 2022 18:43

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Dec 6, 2022

aws-cdk-automation previously requested changes Dec 6, 2022

View reviewed changes

Changing integration test

140da6e

rix0rrr previously requested changes Dec 8, 2022

View reviewed changes

rix0rrr self-assigned this Dec 8, 2022

Adding description for AM url

0d7940f

Merge branch 'main' into AppRegistry_L2_AppManagerUrl

2f065d6

rix0rrr previously requested changes Dec 13, 2022

View reviewed changes

Removing un-necessary condition from resource association

72a2683

santanugho changed the title ~~fix(servicecatalogappregistry): Adding CfnOutput with App manager URL, handling conditonal nested stacks~~ fix(servicecatalogappregistry): Adding CfnOutput with App manager URL Dec 13, 2022

santanugho changed the title ~~fix(servicecatalogappregistry): Adding CfnOutput with App manager URL~~ fix(servicecatalogappregistry): Nested stack association synth error Dec 14, 2022

rix0rrr changed the title ~~fix(servicecatalogappregistry): Nested stack association synth error~~ fix(servicecatalogappregistry): synth error when associating a nested stack Dec 14, 2022

rix0rrr approved these changes Dec 14, 2022

View reviewed changes

Merge branch 'main' into AppRegistry_L2_AppManagerUrl

2c9eba0

mergify bot merged commit 30301d9 into aws:main Dec 14, 2022

santanugho deleted the AppRegistry_L2_AppManagerUrl branch January 11, 2023 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

santanugho commented Dec 6, 2022 •

edited

Loading

gitpod-io bot commented Dec 6, 2022

aws-cdk-automation left a comment

rix0rrr Dec 8, 2022

santanugho Dec 8, 2022

rix0rrr Dec 13, 2022

santanugho Dec 13, 2022

santanugho Dec 13, 2022

rix0rrr Dec 8, 2022

santanugho Dec 8, 2022

rix0rrr Dec 8, 2022

santanugho Dec 8, 2022

rix0rrr Dec 13, 2022

mergify bot commented Dec 14, 2022

aws-cdk-automation commented Dec 14, 2022

mergify bot commented Dec 14, 2022

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

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

Conversation

santanugho commented Dec 6, 2022 • edited Loading

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Dec 6, 2022

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

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

mergify bot commented Dec 14, 2022

aws-cdk-automation commented Dec 14, 2022

AWS CodeBuild CI Report

mergify bot commented Dec 14, 2022

santanugho commented Dec 6, 2022 •

edited

Loading