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

(nested-stack): The generated nested stack names are noisy and verbose #19099

Closed
wyattisimo opened this issue Feb 23, 2022 · 7 comments
Closed
Labels
aws-cdk-lib Related to the aws-cdk-lib package bug This issue is a bug. p1

Comments

@wyattisimo
Copy link

wyattisimo commented Feb 23, 2022

General Issue

Can we simplify the generated nested stack names?

The Question

If I have a top level stack called NetworkStack and a nested stack under it called VpcStack, CDK generates this extra-fluffy path:

NetworkStack/VpcStack.NestedStack/VpcStack.NestedStackResource

The .NestedStack suffix makes sense (although arguably verbose), but repeating the nested stack name as an additional path component with a .NestedStackResource suffix is confusing, if not redundant. Or maybe it's both confusing and redundant. And confusing.

On both the master branch and the v2.0.0-alpha.0 tag, I found a @deprecated comment above the line that effectively causes this redundancy. Apparently someone had previously identified this path construction logic as redundant and flagged it for removal in v2. See:

However, this notation was removed when the backward-compatibility layer for the constructs module was removed. See:

I noticed there was a separate import line for the ./construct-compat module that was intended to facilitate future elimination of that redundant path logic while also avoiding merge conflicts with v2. I suspect it was just a casualty of the mass update to remove/replace all the ./construct-compat imports.

Did we miss the boat on this breaking change, or is it something we still do with a feature flag?

At minimum, I think we should remove the redundant path component that has the .NestedStackResource suffix, e.g.:

NetworkStack/VpcStack.NestedStack

I'm inclined to also remove the .NestedStack suffix from the nested stack name because it's not clear to me what utility that really provides, e.g.:

NetworkStack/VpcStack

Related to this is another issue around incorporating hashes when generating the unique stack name for CloudFormation.

CDK appends a hash to the end of the nested stack path and to the end of the whole thing, e.g.:

NetworkStack-VpcStackNestedStackVpcStackNestedStackResource{UNIQUEHASH}-{OTHERUNIQUEHASH}

Not sure what the reason is for going double unique here. I can see use cases for multiple nested stacks having the same name, so perhaps that first hash could be useful. But the second hash appended to the end seems unnecessary to me because the first hash already makes the nested stack unique within its scope, which is the top level stack. And top level stack names have to be unique because CDK does not append unique hashes to them when they're on their own.

This appears to be an unintentional result of two unrelated functions that are both trying to add uniqueness at different points in the scope. If that's the case, can we discuss simplifying? Or is this by design?

Circling back to the first hash appended to the nested stack... for this to actually be useful, the generated paths above would also need to include that hash. Otherwise, it's impossible to have multiple nested stacks with the same name in a given scope because it would create duplicate keys in the top level stack's CF template and fail. Because of this, as long as the top level stack name is unique, you never get to a point where you can even worry about pushing duplicate nested stack names to AWS.

CDK CLI Version

2.13

Framework Version

No response

Node.js Version

No response

OS

No response

Language

Typescript

Language Version

No response

Other information

No response

@wyattisimo wyattisimo added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Feb 23, 2022
@peterwoodworth peterwoodworth added aws-cdk-lib Related to the aws-cdk-lib package and removed needs-triage This issue or PR still needs to be triaged. labels Feb 23, 2022
@peterwoodworth
Copy link
Contributor

Thank you very much for the detail and the deep dive on this @wyattisimo, I'm not sure whether we intended to keep the functionality from v1 or not! Certainly seems like it could've been a mistake, because I can't find any comments on it either.

@madeline-k What do you think about this?

@wyattisimo wyattisimo changed the title (NestedStack): The generated names are noisy and verbose (nested-stack): The generated nested stack names are noisy and verbose Feb 23, 2022
@konstantinj
Copy link

Finally someone else is raising this. in 2020 I've favored already custom names for nested stacks. The current naming is the reason we're not using nested stacks. Please allow custom names or at least only use the parent stack as prefix to the value of the id field.

@konstantinj
Copy link

btw. my solution for now is this function inside the class of the parent stack:

    getLogicalId(element: CfnElement): string {
        if (element.node.id.includes('NestedStackResource')) {
            return /([a-zA-Z0-9]+)\.NestedStackResource/.exec(element.node.id)![1] // will be the exact id of the stack
        }
        return super.getLogicalId(element)
    }

With this your stack names will look like this: nameOfParentStack-nameOfNestedStack-CdkHash

@DaniloGT
Copy link

DaniloGT commented Mar 4, 2022

btw. my solution for now is this function inside the class of the parent stack:

    getLogicalId(element: CfnElement): string {
        if (element.node.id.includes('NestedStackResource')) {
            return /([a-zA-Z0-9]+)\.NestedStackResource/.exec(element.node.id)![1] // will be the exact id of the stack
        }
        return super.getLogicalId(element)
    }

With this your stack names will look like this: nameOfParentStack-nameOfNestedStack-CdkHash

Please, can you elaborate more?
I have the same issue and this could be a nice fix

I don't know how it works a function with a 'super' function in the return declaration, this creates an 'super' can only be referenced in members of derived classes or object literal expressions.ts(2660) error for me

@konstantinj
Copy link

@DaniloGT You need to have the function as part of your NestedStack class. It's this function that you can override: https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/core/lib/stack.ts#L474

@peterwoodworth peterwoodworth added bug This issue is a bug. p1 and removed guidance Question that needs advice or information. labels Mar 17, 2022
@peterwoodworth
Copy link
Contributor

peterwoodworth commented Mar 31, 2022

Thanks all for the discussion here, but I'm going to close this in favor of the original issue reported here #18053

@github-actions
Copy link

⚠️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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws-cdk-lib Related to the aws-cdk-lib package bug This issue is a bug. p1
Projects
None yet
Development

No branches or pull requests

6 participants