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

fix(servicecatalogappregistry): default stack name is not meaningful and causes conflict when multiple stacks deployed to the same account-region #23823

Merged
merged 9 commits into from
Feb 8, 2023
Merged

fix(servicecatalogappregistry): default stack name is not meaningful and causes conflict when multiple stacks deployed to the same account-region #23823

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3ef244f
fix(servicecatalogappregistry): fix L2 ApplicationAssociator construc…
jungle-amazon Jan 25, 2023
c002d55
Fix trailing space and add integ test
jungle-amazon Jan 25, 2023
ac84048
Merge branch 'main' into l2_fixes
jungle-amazon Jan 25, 2023
bf41d5a
Keep ApplicationAssociatorStack as default stack id, but use app name…
jungle-amazon Feb 2, 2023
3b8514c
Merge branch 'main' into l2_fixes
jungle-amazon Feb 2, 2023
aa800e5
Deprecate stackID and recommend `stackName` use in TargetApplication …
jungle-amazon Feb 7, 2023
d39f9bb
Removed the extra text from Deprecated tag
jungle-amazon Feb 7, 2023
5e7f04b
Merge branch 'main' into l2_fixes
jungle-amazon Feb 7, 2023
e6fd324
Merge branch 'main' into l2_fixes
mergify[bot] Feb 8, 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
12 changes: 1 addition & 11 deletions packages/@aws-cdk/aws-servicecatalogappregistry/lib/aspects/stack-associator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ abstract class StackAssociatorBase implements IAspect {
if (Stage.isStage(childNode)) {
var stageAssociated = this.applicationAssociator?.isStageAssociated(childNode);
if (stageAssociated === false) {
this.error(childNode, 'Associate Stage: ' + childNode.stageName + ' to ensure all stacks in your cdk app are associated with AppRegistry. '
this.warning(childNode, 'Associate Stage: ' + childNode.stageName + ' to ensure all stacks in your cdk app are associated with AppRegistry. '
+ 'You can use ApplicationAssociator.associateStage to associate any stage.');
}
}
Expand All @@ -45,16 +45,6 @@ abstract class StackAssociatorBase implements IAspect {
this.application.associateApplicationWithStack(node);
}

/**
* Adds an error annotation to a node.
*
* @param node The scope to add the error to.
* @param message The error message.
*/
private error(node: IConstruct, message: string): void {
Annotations.of(node).addError(message);
}

/**
* Adds a warning annotation to a node.
*
Expand Down
9 changes: 6 additions & 3 deletions packages/@aws-cdk/aws-servicecatalogappregistry/lib/target-application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ export interface TargetApplicationCommonOptions extends cdk.StackProps {
* Stack ID in which application will be created or imported. The id of a stack is also the identifier that you use to
* refer to it in the [AWS CDK Toolkit](https://docs.aws.amazon.com/cdk/v2/guide/cli.html).
*
* @default - ApplicationAssociatorStack
* @default - references the application name for CreateTargetApplicationOptions,
* or application id for ExistingTargetApplicationOptions
*/
readonly stackId?: string;
}
Expand Down Expand Up @@ -90,7 +91,7 @@ class CreateTargetApplication extends TargetApplication {
super();
}
public bind(scope: Construct): BindTargetApplicationResult {
const stackId = this.applicationOptions.stackId ?? 'ApplicationAssociatorStack';
const stackId = this.applicationOptions.stackId ?? `CreateTargetApplication${this.applicationOptions.applicationName}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking about it a little, I think this is the wrong way to go about it. A Stack has two identifiers:

  • id, used as an ID in the construct tree (needs to be unique within the scope in the construct tree)
  • stackName, the name of the CloudFormation Stack being deployed (needs to be unique within the account+region it is deployed to, defaults to id if not supplied).

I think what is happening here is that you are trying to change stackName by changing id, is that right?

If that is the case, doesn't it make more sense to leave id where it is, and explicitly set stackName to the value you're trying to achieve?

Also, CreateXxx is not a great name for a resource (Create is a Verb, but a resource is a Noun). Maybe make it something Application-MyGreatApplication-Stack ?

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 you are correct, we are trying to change stackName by changing the id. Your proposed approach of setting the stackName makes more sense and I have updated the commit with this change instead.

I also updated the stackName format as per your suggestion.

For your first concern about "renaming" the stack, it is true that if the user wants to continue to rely on the default stack name and keep the same application name, the user will see a conflict because the new stack with the updated name will attempt to create an app that already exists.

I think a workaround for the user would be to specify the previous default stack name ApplicationAssociatorStack as a stackName option property to keep using the old stack name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a workaround for the user would be to specify the previous default stack name ApplicationAssociatorStack as a stackName option property to keep using the old stack name.

👍 Sounds good.

The next question is: how will users that get affected by this know that they have to do that?

Here is my suggestion: they will probably read the change log and find this PR. Please edit the description of this PR and add a section that says "if you run into the following error (...error message here...) make sure to set stackName to the name of your stack. For example:

new ApplicationAssociator(..., {
  stackName: 'whatever-theold-<your application name here>-stackname-was'

"

Something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense, I've added this section in the PR description.

(this.applicationOptions.description as string) =
this.applicationOptions.description || 'Stack to create AppRegistry application';
(this.applicationOptions.env as cdk.Environment) =
Expand All @@ -117,7 +118,9 @@ class ExistingTargetApplication extends TargetApplication {
super();
}
public bind(scope: Construct): BindTargetApplicationResult {
const stackId = this.applicationOptions.stackId ?? 'ApplicationAssociatorStack';
const arnComponents = cdk.Arn.split(this.applicationOptions.applicationArnValue, cdk.ArnFormat.SLASH_RESOURCE_SLASH_RESOURCE_NAME);
const applicationId = arnComponents.resourceName;
const stackId = this.applicationOptions.stackId ?? `ExistingTargetApplication${applicationId}`;
const applicationStack = new cdk.Stack(scope, stackId, this.applicationOptions);
const appRegApplication = Application.fromApplicationArn(applicationStack, 'ExistingApplication', this.applicationOptions.applicationArnValue);
return {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-servicecatalogappregistry/test/application-associator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ describe('Scope based Associations with Application with Cross Region/Account',
associateStage: false,
});
app.synth();
Annotations.fromStack(pipelineStack).hasError('*',
Annotations.fromStack(pipelineStack).hasWarning('*',
'Associate Stage: SampleStage to ensure all stacks in your cdk app are associated with AppRegistry. You can use ApplicationAssociator.associateStage to associate any stage.');
});

Expand Down