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

@aws-cdk/aws-servicecatalogappregistry-alpha: ApplicationAssociator's hardcoded stack id is not meaningful and causes conflicts if user deploys multiple apps in the same account-region #23861

Closed
jungle-amazon opened this issue Jan 27, 2023 · 5 comments · Fixed by #23823
Labels
@aws-cdk/aws-servicecatalogappregistry bug This issue is a bug. in-progress This issue is being actively worked on. p1

Comments

@jungle-amazon
Copy link
Contributor

jungle-amazon commented Jan 27, 2023

Describe the bug

Note from user:

By default, application_name value will be used as the stack ID and stack name. cdk.Stack uses construct path as the stack ID. When cdk.Stack has cdk.App as the parent, like in the TargetApplication use case, effectively the cdk.Stack construct ID is used as stack ID. We can mimic that behavior by using application_name value as the resulting cdk.Stack construct ID. For an example, if I use UserManagementBackend as the application name, I will get that value as both stack ID and stack name.

In addition, if a stage is not associated then currently an error is thrown. A warning should be displayed instead to continue with the rest of the child nodes, instead of stopping on the error.

Expected Behavior

  • If a stack's stage is not associated then a warning is displayed
  • Stack name for CreateTargetApplication should not be statically defined. If the name is not provided by the user, we should use a dynamic identifier that prevents conflicts and has more meaning to the user.
  • Stack name for ExistingTargetApplication should not be statically defined. If the name is not provided by the user, we should use a dynamic identifier that prevents conflicts and has more meaning to the user.

Current Behavior

  • If a stage is not associated then an error is thrown
  • By default, the stack name for CreateTargetApplication and ExistingTargetApplication stacks are hardcoded to ApplicationAssociatorStack because the stack ID is hardcoded to that value (if stack name is not specified by the user then the stack id is used as the name). ApplicationAssociatorStack also doesn't provide meaningful details.

Reproduction Steps

Sample code using the ApplicationAssociator to create a new application named NewApplication:

const autoApp = new appreg.ApplicationAssociator(app, 'TestApplicationAssociatorOne', {
    applications: [
        appreg.TargetApplication.createApplicationStack({
            applicationName: 'NewApplication',
            applicationDescription: 'Associated Application description us-west-2',
            env: { account: '11111111', region: 'us-west-2' },
        }),
    ],
});

Run CDK synthesis on this, and note that the stack name is ApplicationAssociatorStack

Possible Solution

  • Replace stage association error with warning.
  • Replace the hard coding in stack name here with a reference to the application name.
  • Replace the hard coding in stack name here with a reference to application ID.

Additional Information/Context

No response

CDK CLI Version

2.50.0

Framework Version

No response

Node.js Version

16.5.0

OS

macOS 12.6.2 Monterey

Language

Typescript

Language Version

No response

Other information

No response

@jungle-amazon jungle-amazon added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 27, 2023
@jungle-amazon jungle-amazon changed the title fix(servicecatalogappregistry): ApplicationAssociator should display warning instead of error if a stack's stage is not associated; ApplicationAssociator should reference app id or name for TargetApplication's stack id @aws-cdk/aws-servicecatalogappregistry-alpha: ApplicationAssociator should display warning instead of error if a stack's stage is not associated; ApplicationAssociator should reference app id or name for TargetApplication's stack id Jan 27, 2023
@jungle-amazon jungle-amazon changed the title @aws-cdk/aws-servicecatalogappregistry-alpha: ApplicationAssociator should display warning instead of error if a stack's stage is not associated; ApplicationAssociator should reference app id or name for TargetApplication's stack id @aws-cdk/aws-servicecatalogappregistry-alpha: ApplicationAssociator should display warning instead of error if a stack's stage is not associated; ApplicationAssociator should use dynamic identifier for the app stack to prevent conflicting stack names Feb 2, 2023
@jungle-amazon jungle-amazon changed the title @aws-cdk/aws-servicecatalogappregistry-alpha: ApplicationAssociator should display warning instead of error if a stack's stage is not associated; ApplicationAssociator should use dynamic identifier for the app stack to prevent conflicting stack names @aws-cdk/aws-servicecatalogappregistry-alpha: ApplicationAssociator should display warning instead of error if a stack's stage is not associated; ApplicationAssociator should use dynamic identifier for the app stack to prevent conflicting stack names in same account/region Feb 2, 2023
@jungle-amazon jungle-amazon changed the title @aws-cdk/aws-servicecatalogappregistry-alpha: ApplicationAssociator should display warning instead of error if a stack's stage is not associated; ApplicationAssociator should use dynamic identifier for the app stack to prevent conflicting stack names in same account/region @aws-cdk/aws-servicecatalogappregistry-alpha: ApplicationAssociator's hardcoded stack id is not meaningful and causes conflicts if user deploys multiple apps in the same account-region Feb 2, 2023
@peterwoodworth peterwoodworth added p1 in-progress This issue is being actively worked on. and removed needs-triage This issue or PR still needs to be triaged. labels Feb 8, 2023
@peterwoodworth
Copy link
Contributor

Thanks for working with us on the PR for this 🙂

@mergify mergify bot closed this as completed in #23823 Feb 8, 2023
mergify bot pushed a commit that referenced this issue Feb 8, 2023
…and causes conflict when multiple stacks deployed to the same account-region (#23823)

- Replace stage association error with warning
- Deprecate `stackId` in TargetApplication options 
- Provide a default dynamic stack name for CreateTargetApplication stack with a reference to the application name
- Provide a default dynamic stack name for ExistingTargetApplication stack with a reference to the application ID

This fixes: [23861](#23861)

Note: With this change to `stackName`, you may run into the following error during deployment if you have been using the default stack id and name by not explicitly setting them.
```
Resource handler returned message: "You already own an application 'MyApplicationName' (Service: ServiceCatalogAppRegistry, Status Code: 409, Request ID: xxxx)" (RequestToken: yyyy, HandlerErrorCode: InvalidRequest)
```

To address this error, explicitly set the `stackName` value to the name of your existing stack. For example:

```typescript
const associatedApp = new ApplicationAssociator(app, 'MyApplicationAssociator', {
  applications: [ TargetApplication.createApplicationStack({
    applicationName: 'MyApplicationName',
    stackName: 'ApplicationAssociatorStack', // add your existing stack name here
    ...
```

----

### 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*
@github-actions
Copy link

github-actions bot commented Feb 8, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@alexpulver
Copy link
Contributor

alexpulver commented Feb 12, 2023

@jungle-amazon @peterwoodworth deprecating stack ID breaks the behavior of user being able to control stack ID in AWS CDK Toolkit output. This is important for project naming convention and avoid surprising output.

Using Stack ID I can set the AppRegistry application stack ID based on my project convention (UserManagementBackendApplication):

Supply a stack id (UserManagementBackendApplication, UserManagementBackendSandbox, UserManagementBackendToolchain) to display its template.

Removing Stack ID and using stack name generates a surprising name (from user perspective):

Supply a stack id (ApplicationAssociatorStack, UserManagementBackendSandbox, UserManagementBackendToolchain) to display its template.

Now that TargetApplication uses cdk.App as scope (#22973), using stack name as stack ID should address this issue. Do you want me to open a separate issue for this?

@jungle-amazon
Copy link
Contributor Author

jungle-amazon commented Feb 13, 2023

Yes, please create a separate github issue to so that we can track your issue.

If I understand correctly, the new stack name references the application name which was part of the last fix, but in your use case you need stack id to have this default value as well, not just the stack name. Additionally, the user might want to customize stack ID for easier identification in CDK Toolkit interactions, not just being limited to stack name modification.

We can look into this change.

@alexpulver
Copy link
Contributor

@jungle-amazon thanks for circling back! I opened the issue to clarify the ask: #24160

mergify bot pushed a commit that referenced this issue Feb 16, 2023
…ack name for Application stack (#24171)

* Assign value of `stackName` to `stackId` for Application stack, so the stack id will always be the same as stack name. Users wanting to control stack id can do so via `stackName`.

Closes #24160.

Background:
* Customers wish to control or modify the stack id of the Application stack to follow their project conventions within CDK.
* In previous [fix](#23823), we had deprecated the stack id to push users towards using stack name as the main mechanism for stack identification.

Note on backward-compatibility:

After this change, the `stackId` parameter can no longer be used to control the application stack id. The stack id will always match stack name, and the default stack name if not specified will be `Application-${APPLICATION_IDENTIFIER}-Stack`. `${APPLICATION_IDENTIFIER}` is application name for CreateTargetApplication and application id for ExistingTargetApplication.

If you created an application stack prior to aws-cdk [release v2.64.0](https://github.com/aws/aws-cdk/releases/tag/v2.64.0) and did not specify a stack id or name, you may run into the following error during deployment due to the creation attempt of a new stack with the same application:
```log
Resource handler returned message: "You already own an application 'MyApplicationName' (Service: ServiceCatalogAppRegistry, Status Code: 409, Request ID: xxxx)" (RequestToken: yyyy, HandlerErrorCode: InvalidRequest)
```

To address this error, you can set the `stackName` value to match your existing stack. For example:
```typescript
const associatedApp = new ApplicationAssociator(app, 'MyApplicationAssociator', {
  applications: [ TargetApplication.createApplicationStack({
    applicationName: 'MyApplicationName',
    stackName: 'ApplicationAssociatorStack', // Add your existing stack name here
    ...
```
This will result in the existing stack deploying with the previous name, and the id in CDK will reflect this same stack name.

Related links:
* Previous PR: #23823
* Previous GitHub issue: #23861
* Original reason that introduced `stackId`: #22508

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-servicecatalogappregistry bug This issue is a bug. in-progress This issue is being actively worked on. p1
Projects
None yet
3 participants