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

ProductStackProps should extend StackProps #31924

Closed
1 task
bdoyle0182 opened this issue Oct 28, 2024 · 5 comments · Fixed by #31985
Closed
1 task

ProductStackProps should extend StackProps #31924

bdoyle0182 opened this issue Oct 28, 2024 · 5 comments · Fixed by #31985
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 package/tools Related to AWS CDK Tools or CLI

Comments

@bdoyle0182
Copy link

bdoyle0182 commented Oct 28, 2024

Describe the bug

ProductStack does not allow configuration of the underlying stack template for stack level configuration. This means you can't disable things like analyticsReporting on the stack template that the ProductStack generates. ProductStack constructor currently takes ProductStackProps which doesn't extend the base level StackProps even though ProductStack extends Stack Since CDK::AWS::METADATA resource type does not exist in GovCloud, this means a Stack with a StackSet that references a ProductStack cannot be deployed to GovCloud.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

Being able to configure the Stack template that is generated by a ProductStack

Current Behavior

ProductStack doesn't allow configuring stack level options for how it synthesizes its stack templates.

Reproduction Steps

    readonly templateFile: string;
    private _parentProductStackHistory?;
    private _templateUrl?;
    private _parentStack;
    private assetBucket?;
    constructor(scope: Construct, id: string, props?: **ProductStackProps**);```

### Possible Solution

_No response_

### Additional Information/Context

_No response_

### CDK CLI Version

2.164.1

### Framework Version

_No response_

### Node.js Version

node22

### OS

n/a

### Language

TypeScript

### Language Version

_No response_

### Other information

_No response_
@bdoyle0182 bdoyle0182 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 28, 2024
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Oct 28, 2024
@bdoyle0182
Copy link
Author

I've found a workaround where the ProductStack template will not include the AWS::CDK::METADATA resource if I include "versionReporting": false in my cdk.json for my repo where this is defined. However this limits me from adding conditional logic to disable the versionReporting / analyticsReporting only for the GovCloud template, allowing me to keep it on for all other regions that would be able to be done if ProductStackProps extended StackProps.

@ashishdhingra ashishdhingra self-assigned this Oct 28, 2024
@ashishdhingra ashishdhingra added p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 28, 2024
@ashishdhingra
Copy link
Contributor

@bdoyle0182 Good afternoon. Could you please share self-contained minimal code to reproduce the issue that demonstrates CDK modules and constructs you are trying to use? Are you using ProductStackProps from aws-cdk-lib » aws_servicecatalog package? If yes, I do see that ProductStackProps interface doesn't extend StackProps interface, which would have provided access to other stack level properties like https://github.com/aws/aws-cdk/blob/a9d3b02783104fe862a3e863e8c89af845b49c8c/packages/aws-cdk-lib/core/lib/stack.ts#L167.

StackProps.analyticsReporting is used to set Stack's _versionReportingEnabled property here, which in injectMetadataResources() is used to determine whether metadata should be emitted in the synthesized CloudFormation template.

I'm unsure if ProductStackProps interface could be modified to extend StackProps interface since it could be a breaking change. Also, most of the constructs use pattern of defining property interface with relevant fields for construct and using it as one of the parameters for L2 construct.

The documentation for analyticsReporting specifies that it is an optional property with the default value as analyticsReporting setting of containing App, or value of 'aws:cdk:version-reporting' context key. Hence, as a workaround, you could set analyticsReporting at CDK App level const app = new cdk.App({analyticsReporting: true});.

Thanks,
Ashish

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Oct 28, 2024
@bdoyle0182
Copy link
Author

Hi Ashish,

Unfortunately I can't share any code. But your assessment is correct, we're using that ProductStack from aws_servicecatalog.

Here is how the project looks

  1. Create an app that instantiates a stack for each region we deploy to
  2. The stack for each region instantiated by the app manages a stack set resource.
  3. The stack set resource uses an instantiated ProductStack as the reference for the stack template to deploy as a part of the stack set.

I am actually using your work around right now by setting the config in my cdk.json of my project and it successfully does what I need. However, it is limiting. I have no issue with having version reporting enabled, it is just that AWS::CDK::METADATA does not exist in govcloud so there is no way to have version reporting enabled when one of my stack instances for my app is going to govcloud. If I wanted to have an override for just this region so that my commercial regions can still report, that does not work with the work around since it's app level and I can't override the setting on the ProductStack conditionally for the current region.

So there's really two issues here:

  1. ProductStack type needs to be more flexible on how you configure the Stack template it uses by leveraging StackProps or something similar, whatever is needed to make the change non-breaking; but really at present you can't really do any stack configs on ProductStack which would be possible if you just hand provided the yaml / json for the template.
  2. AWS CDK's current versions at the base level should not allow version reporting to be turned on for a stack
    resource if the stack's region is govcloud until the AWS::CDK::METADATA resource type is supported in govcloud. i.e. this can be fixed by having the Stack class have conditional logic that sets version reporting to false if the region for the Stack is a govcloud region. This should be non-breaking since you can't currently deploy a stack to govcloud with this turned on for a stack.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 29, 2024
@pahud pahud added feature-request A feature should be added or improved. effort/medium Medium work item – several days of effort and removed bug This issue is a bug. labels Oct 30, 2024
@ashishdhingra ashishdhingra removed their assignment Oct 31, 2024
@mergify mergify bot closed this as completed in #31985 Nov 7, 2024
@mergify mergify bot closed this as completed in d8ad02a Nov 7, 2024
Copy link

github-actions bot commented Nov 7, 2024

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

1 similar comment
Copy link

github-actions bot commented Nov 7, 2024

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2024
Leo10Gama pushed a commit to Leo10Gama/aws-cdk that referenced this issue Nov 13, 2024
…rting and stack descriptions (aws#31985)

### Issue # (if applicable)

Closes aws#31924

### Reason for this change

Product Stack cannot override analytics reporting and descriptions. Support these two props.

### Description of changes

The reason I didn't choose to allow ProductStackProps to extend StackProps and instead manually add these two properties are because all of the other properties, i.e. `stackName`, `env`, `notificationArns`, `terminationProtection`, `crossRegionReferences`, `permissionsBoundary`, `suppressTemplateIndentation`, do not mutate the stack template but are used by CDK CLI. These properties have no impact on the Product Stack template generated and thus I did not include them.

### Description of how you validated changes

Unit and integ tests added.

### Checklist
- [ ] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
3 participants