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

Jump to bottom

(CfnInclude): CfnInclude omits aws_autoscaling. UpdatePolicy that dont exists in the CDK (MinActiveInstancesPercent) #33810

Open
1 task
NetaNir opened this issue Mar 18, 2025 · 2 comments · May be fixed by #33852
Open
1 task

(CfnInclude): CfnInclude omits aws_autoscaling. UpdatePolicy that dont exists in the CDK (MinActiveInstancesPercent) #33810

NetaNir opened this issue Mar 18, 2025 · 2 comments · May be fixed by #33852
Assignees
@scorbiere
Labels
@aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@NetaNir
Copy link
Contributor

NetaNir commented Mar 18, 2025 •
edited
Loading

Describe the bug

When CfnInclude references a template that has an UpdatePolicy for autoscaling groups it silently omits the MinActiveInstancesPercent property because the code in cfn-parse.ts does not include the property:



      return undefinedIfAllValuesAreEmpty({
        maxBatchSize: FromCloudFormation.getNumber(p.MaxBatchSize).value,
        minInstancesInService: FromCloudFormation.getNumber(p.MinInstancesInService).value,
        minSuccessfulInstancesPercent: FromCloudFormation.getNumber(p.MinSuccessfulInstancesPercent).value,
        pauseTime: FromCloudFormation.getString(p.PauseTime).value,
        suspendProcesses: FromCloudFormation.getStringArray(p.SuspendProcesses).value,
        waitOnResourceSignals: FromCloudFormation.getBoolean(p.WaitOnResourceSignals).value,
      });

source

The result is that the property is omitted completely and silently :(

Regression Issue

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

Last Known Working CDK Version

No response

Expected Behavior

MinActiveInstancesPercent should be included in the output template

Current Behavior

The result is that the property is omitted completely and silently :(

Reproduction Steps

Create a CFN template with an autoscaling group and an update policy which includes MinActiveInstancesPercent, reference the template file using CfnInclude, the MinActiveInstancesPercent will not exist in the template in cdk.out

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.1003.0

Framework Version

2.132.0

Node.js Version

18

OS

MacOs

Language

TypeScript

Language Version

No response

Other information

No response
@NetaNir NetaNir added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 18, 2025
@github-actions github-actions bot added the @aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package label Mar 18, 2025
@pahud
Copy link
Contributor

pahud commented Mar 18, 2025 •
edited
Loading

Hi @NetaNir thank you for coming back for the issue report :-)

Root Cause

Your referenced source is at CDK 2.99.1. Looking at the main branch, the issue is in the AutoScaling UpdatePolicy parsing code in cfn-parse.ts. Here's the affected code:

aws-cdk/packages/aws-cdk-lib/core/lib/helpers-internal/cfn-parse.ts

Lines 452 to 464 in 7f3cc8f

function parseAutoScalingRollingUpdate(p: any): FromCloudFormationResult<CfnAutoScalingRollingUpdate | undefined> {
if (typeof p !== 'object') { return new FromCloudFormationResult(undefined); }
self.throwIfIsIntrinsic(p, logicalId);
const rollUp = new ObjectParser<CfnAutoScalingRollingUpdate>(p);
rollUp.parseCase('maxBatchSize', FromCloudFormation.getNumber);
rollUp.parseCase('minInstancesInService', FromCloudFormation.getNumber);
rollUp.parseCase('minSuccessfulInstancesPercent', FromCloudFormation.getNumber);
rollUp.parseCase('pauseTime', FromCloudFormation.getString);
rollUp.parseCase('suspendProcesses', FromCloudFormation.getStringArray);
rollUp.parseCase('waitOnResourceSignals', FromCloudFormation.getBoolean);
return rollUp.toResult();
}

The MinActiveInstancesPercent property from CloudFormation's UpdatePolicy is not included in the parsing code. This means when CfnInclude processes a template containing this property:

  1. The property exists in the source CloudFormation template
  2. During parsing, the parseAutoScalingRollingUpdate function doesn't have a case to handle MinActiveInstancesPercent
  3. The property is silently dropped from the final template in cdk.out

Impact:

  • This affects any CloudFormation template that uses the MinActiveInstancesPercent property in AutoScaling group update policies
  • The property is completely omitted from the synthesized template without any warning
  • This could lead to unexpected behavior during stack updates since the minimum percentage of active instances is not maintained as specified in the original template

Suggested Fix:

  1. Adding a property parser in the parseAutoScalingRollingUpdate function

I'm making this a p1 and will discuss with the team.

@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 18, 2025
@pahud
Copy link
Contributor

pahud commented Mar 19, 2025 •
edited
Loading

Attaching a diagram to explain this issue

Image

@scorbiere scorbiere self-assigned this Mar 20, 2025
@scorbiere scorbiere linked a pull request Mar 20, 2025 that will close this issue
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@scorbiere scorbiere

Labels
@aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Milestone
No milestone
3 participants
@pahud @NetaNir @scorbiere