feat(autoscaling): add new `HealthChecks` for multiple health check types, including EBS and VPC_LATTICE types #31286

go-to-k · 2024-09-02T11:01:55Z

Issue # (if applicable)

Closes #31289 .

Reason for this change

Only ONE HealthCheckType can be selected for an existing healthCheck property: EC2 or ELB.

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts#L233

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts#L2232-L2258

However, the current CFn specification allows multiple health check types to be specified, separated by commas.

A comma-separated value string of one or more health check types.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-autoscaling-autoscalinggroup.html#cfn-autoscaling-autoscalinggroup-healthchecktype

Also, besides EC2 and ELB, EBS and VPC_LATTICE can now be configured.

The valid values are EC2, EBS, ELB, and VPC_LATTICE. EC2 is the default health check and cannot be disabled.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-autoscaling-autoscalinggroup.html#cfn-autoscaling-autoscalinggroup-healthchecktype

If it was just EC2 and ELB, there would not be a need for multiple specifications. (Because specifying EC2 and another type at the same time would result in a CFn error. This means that when specifying an ELB, it is a single specification.)

But the increase in the number of these property types makes multiple specifications necessary. Therefore, it is good to support the specification of multiple health check types and the addition of new types at the same time.

See the docs for more details.

https://docs.aws.amazon.com/autoscaling/ec2/userguide/ec2-auto-scaling-health-checks.html

Description of changes

So, I add a new HealthChecks class and healthChecks property.

One or more health check types can be selected.
Added EBS and VPC_LATTIC types.

And I deprecated the existing healthCheck property.

Description of how you validated changes

Unit and integ tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

go-to-k · 2024-09-02T11:32:11Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+/**
+ * Additional Health Check Type
+ */
+export enum AdditionalHealthCheckType {
+  /**
+   * ELB Health Check
+   */
+  ELB = 'ELB',
+  /**
+   * EBS Health Check
+   */
+  EBS = 'EBS',
+  /**
+   * VPC LATTICE Health Check
+   */
+  VPC_LATTICE = 'VPC_LATTICE',
+}


EC2 had to be excluded because specifying EC2 and other types together would result in a CFn error.

Resource handler returned message: "Specifying EC2 in addition to other health check types is not supported. Remove EC2 from HealthCheckType and try again. (Service: AutoScaling, Status Code: 400, Request ID: ...

go-to-k · 2024-09-02T11:34:02Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+   * @default Duration.seconds(0)
+   * @see https://docs.aws.amazon.com/autoscaling/ec2/userguide/health-check-grace-period.html
+   */
+  readonly grace?: Duration;


The grace has to be specified in the existing ELB options, but this is not necessary now. (I actually deployed the configuration without the grace successfully).

https://github.com/aws/aws-cdk/pull/31286/files#diff-752c40239355facf5c8611bb25a389d92835db38a094213fdc6ed47c49aa9652R41-R45

godwingrs22

Thanks @go-to-k for the contribution. Left some minor comments with some recommendations and for my understanding.

godwingrs22 · 2025-02-24T21:10:25Z

packages/aws-cdk-lib/aws-autoscaling/test/auto-scaling-group.test.ts

+
+    // THEN
+    Template.fromStack(stack).hasResourceProperties('AWS::AutoScaling::AutoScalingGroup', {
+      HealthCheckType: 'EBS,ELB,VPC_LATTICE',


qq: Since EC2 healthcheck is default and can't be disabled, shouldn't it be present in the HealthCheckType along with the additional types?

Specifying those types with EC2 will result in a CFn error.

#31286 (comment)

In other words, the following patterns can be allowed:

EC2 only

Other types without EC2

e.g. EBS,ELB

Actually includes EC2 health check internally.

godwingrs22 · 2025-02-24T21:28:23Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+export enum AdditionalHealthCheckType {
+  /**
+   * ELB Health Check
+   */
+  ELB = 'ELB',
+  /**
+   * EBS Health Check
+   */
+  EBS = 'EBS',
+  /**
+   * VPC LATTICE Health Check
+   */
+  VPC_LATTICE = 'VPC_LATTICE',
+}


Can we add EBS and VPC_LATTICE to existing enum HealthCheckType instead of creating new one?

The existing HealthCheckType has EC2 as a type. However, that makes it look like EC2 can be specified with other types, but in fact EC2 cannot be specified when other types are specified.

#31286 (comment)

That's why I have a new type to specify something other than EC2.

Thanks for claryfing. While creating a separate enum for additional health check types is a valid strategy and makes sense, I believe we can achieve the same functionality and enforce the validation in the existing enum withim the HealthChecks constructor. However I'm ok creating a new enum without EC2, but it would be good if we can rename this enum for better clarity and maintainablity. Let me know if it makes sense.

Thank you. But I'm still not sure and would like to sort this out again.

In static ec2(), the user does not specify any types (the EC2 type), and the EC2 type is automatically specified internally. In static withAdditionalChecks(), the user must not choose the EC2 type.
This means that the user never gets a chance to use the EC2 type. Therefore, I'm not sure that there is reason for EC2 to appear in the enum. (The existing old enum was not exported and only used internally, whereas the new enum is different in that it is exported.)

I considered allowing the EC2 type to be specified, and then internally excluding the EC2 if it was specified, but I thought that users might be confused when they looked at the generated CFn template and realised that the EC2 was missing.

Is there any reason why the EC2 type should still be included in the enum? Please let me know if I have misunderstood something fundamental.

Also, the method name has been changed to withAdditionalChecks(), but the name AdditionalHealthCheckType for the type name is still unnatural?

I considered allowing the EC2 type to be specified, and then internally excluding the EC2 if it was specified, but I thought that users might be confused when they looked at the generated CFn template and realised that the EC2 was missing.

I thought it would be good to use the EC2 enum in the ec2 method below instead of specifiying it as string. But I agree and makes sense that it might confuse users of having EC2 in the enum when it can't be used.

public static ec2(options: Ec2HealthChecksOptions = {}): HealthChecks { return new HealthChecks(['EC2'], options.gracePeriod); }

Also, the method name has been changed to withAdditionalChecks(), but the name AdditionalHealthCheckType for the type name is still unnatural?

I think should be ok since with withAdditionalChecks it is more clearer.

godwingrs22 · 2025-02-24T21:44:46Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+  public static addition(options: AdditionalHealthChecksOptions): HealthChecks {
+    return new HealthChecks(options.additionalTypes, options.grace);
+  }


I think would be good to rename this method to combine

public static combine(options: HealthChecksOptions): HealthChecks { return new HealthChecks(options); }

It seems to me that this would allow EC2 to be specified as well. I would suggest putting in the nuance of "in addition to" EC2, what do you think?

I understand the intention. However addition, additionalTypes doesn't sound like a better naming and unclear.

I would recommend as extendedChecks or withAdditionalChecks. What do you think?

HealthChecks.extendedChecks({ types: [HealthCheckType.ELB, HealthCheckType.EBS] }) HealthChecks.withAdditionalChecks({ types: [HealthCheckType.ELB, HealthCheckType.EBS] })

Also would be good to have consistent name for gracePeriod prop. I see in healthchecks constructor we use as gracePeriod and in other places as grace. Would be good to use either of. What do you think?

I would recommend as extendedChecks or withAdditionalChecks. What do you think?

I would prefer withAdditionalChecks, I changed the name to it.

8fc06d9

Also would be good to have consistent name for gracePeriod prop. I see in healthchecks constructor we use as gracePeriod and in other places as grace. Would be good to use either of. What do you think?

That's right. (The existing class did that, so I had copied them...)
Changed it with gracePeriod .

09f7dcd

godwingrs22 · 2025-02-24T21:50:55Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+/**
+ * Health check settings for multiple types
+ */
+export class HealthChecks {


I would like to propose something like below for the new property and healthchecks class. Let me know if it makes sense

//new property readonly healthChecks?: HealthChecks; export interface HealthChecksOptions { readonly types: HealthCheckType[]; readonly gracePeriod?: Duration; } export class HealthChecks { private constructor(public readonly options: HealthChecksOptions) {} /** * Configure one or more health check types */ public static combine(options: HealthChecksOptions): HealthChecks { return new HealthChecks(options); } /** * EC2 health check */ public static ec2(options: Ec2HealthChecksOptions = {}): HealthChecks { return new HealthChecks({ types: [HealthCheckType.EC2], gracePeriod: options.grace, }); }

Example useage:

// Single health check (backward compatible style) new autoscaling.AutoScalingGroup(stack, 'MyFleet1', { // ... other properties ... healthChecks: HealthChecks.ec2(), }); // Multiple health checks new autoscaling.AutoScalingGroup(stack, 'MyFleet2', { // ... other properties ... healthChecks: HealthChecks.combine({ types: [HealthCheckType.ELB, HealthCheckType.EBS], gracePeriod: Duration.minutes(15), }), });

This would allow HealthCheckTypes (types: HealthCheckType[];) to be passed in the static ec2 argument. However, when specifying an EC2, no other type should be passed, so extra validation would have to be implemented. Also it would be easier for the user to use if nothing of any type could be specified.

Therefore I think it would be good as it is, what do you think?

packages/aws-cdk-lib/aws-autoscaling/test/auto-scaling-group.test.ts

…alth-checks

go-to-k

@godwingrs22

Thanks for your review!

Could you please take a look at some of the comments I have returned?

go-to-k · 2025-02-25T07:22:48Z

packages/aws-cdk-lib/aws-autoscaling/test/auto-scaling-group.test.ts

+
+    // THEN
+    Template.fromStack(stack).hasResourceProperties('AWS::AutoScaling::AutoScalingGroup', {
+      HealthCheckType: 'EBS,ELB,VPC_LATTICE',


Specifying those types with EC2 will result in a CFn error.

#31286 (comment)

In other words, the following patterns can be allowed:

EC2 only

Other types without EC2

e.g. EBS,ELB

Actually includes EC2 health check internally.

go-to-k · 2025-02-25T07:44:01Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+/**
+ * Health check settings for multiple types
+ */
+export class HealthChecks {


This would allow HealthCheckTypes (types: HealthCheckType[];) to be passed in the static ec2 argument. However, when specifying an EC2, no other type should be passed, so extra validation would have to be implemented. Also it would be easier for the user to use if nothing of any type could be specified.

Therefore I think it would be good as it is, what do you think?

go-to-k · 2025-02-25T07:49:31Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+  public static addition(options: AdditionalHealthChecksOptions): HealthChecks {
+    return new HealthChecks(options.additionalTypes, options.grace);
+  }


It seems to me that this would allow EC2 to be specified as well. I would suggest putting in the nuance of "in addition to" EC2, what do you think?

go-to-k · 2025-02-25T07:56:14Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+export enum AdditionalHealthCheckType {
+  /**
+   * ELB Health Check
+   */
+  ELB = 'ELB',
+  /**
+   * EBS Health Check
+   */
+  EBS = 'EBS',
+  /**
+   * VPC LATTICE Health Check
+   */
+  VPC_LATTICE = 'VPC_LATTICE',
+}


The existing HealthCheckType has EC2 as a type. However, that makes it look like EC2 can be specified with other types, but in fact EC2 cannot be specified when other types are specified.

#31286 (comment)

That's why I have a new type to specify something other than EC2.

packages/aws-cdk-lib/aws-autoscaling/test/auto-scaling-group.test.ts

codecov · 2025-02-25T11:10:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (4879c04) to head (a8f5b81).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #31286   +/-   ##
=======================================
  Coverage   82.21%   82.21%           
=======================================
  Files         119      119           
  Lines        6876     6876           
  Branches     1162     1162           
=======================================
  Hits         5653     5653           
  Misses       1120     1120           
  Partials      103      103

Flag	Coverage Δ
suite.unit	`82.21% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.21% <ø> (ø)`

godwingrs22

Thanks @go-to-k for addressing the earlier comments. Left some followup comments with suggestions and for my understanding.

godwingrs22 · 2025-02-25T20:46:43Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+  public static addition(options: AdditionalHealthChecksOptions): HealthChecks {
+    return new HealthChecks(options.additionalTypes, options.grace);
+  }


I understand the intention. However addition, additionalTypes doesn't sound like a better naming and unclear.

I would recommend as extendedChecks or withAdditionalChecks. What do you think?

HealthChecks.extendedChecks({ types: [HealthCheckType.ELB, HealthCheckType.EBS] }) HealthChecks.withAdditionalChecks({ types: [HealthCheckType.ELB, HealthCheckType.EBS] })

godwingrs22 · 2025-02-25T20:50:53Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+  public static addition(options: AdditionalHealthChecksOptions): HealthChecks {
+    return new HealthChecks(options.additionalTypes, options.grace);
+  }


Also would be good to have consistent name for gracePeriod prop. I see in healthchecks constructor we use as gracePeriod and in other places as grace. Would be good to use either of. What do you think?

godwingrs22 · 2025-02-25T20:59:38Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+export enum AdditionalHealthCheckType {
+  /**
+   * ELB Health Check
+   */
+  ELB = 'ELB',
+  /**
+   * EBS Health Check
+   */
+  EBS = 'EBS',
+  /**
+   * VPC LATTICE Health Check
+   */
+  VPC_LATTICE = 'VPC_LATTICE',
+}


Thanks for claryfing. While creating a separate enum for additional health check types is a valid strategy and makes sense, I believe we can achieve the same functionality and enforce the validation in the existing enum withim the HealthChecks constructor. However I'm ok creating a new enum without EC2, but it would be good if we can rename this enum for better clarity and maintainablity. Let me know if it makes sense.

godwingrs22 · 2025-02-25T21:18:57Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+  public static ec2(options: Ec2HealthChecksOptions = {}): HealthChecks {
+    return new HealthChecks(['EC2'], options.grace);
+  }


For my understanding, since ec2 is a default type which is implictly enabled, I assume this method is for configuring the default EC2 healthcheck like gracePeriod?

Yes, but does it mean that the method name or the arg name should be changed to gracePeriod?

For the former, I use the method name ec2 for the exclusive choice of whether it is EC2 or an additional type as a union-like class (partly because that was the way it was in the old class).
About the latter, the reason why the argument is the options instead of a stand-alone gracePeriod prop is because I thought about the possibility of adding new properties in the future.

Thanks for the clarification. Yes it makes sense to pass the options as argument to have the possibilites of adding new properties in future.

go-to-k · 2025-02-26T06:18:25Z

@godwingrs22

I've returned some comments and I hope you'll take a look at them.

#31286 (comment)
#31286 (comment)
#31286 (comment)

godwingrs22

Thanks @go-to-k for addressing the comments and for the contribution. LGTM.

godwingrs22 · 2025-02-26T19:59:09Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+export enum AdditionalHealthCheckType {
+  /**
+   * ELB Health Check
+   */
+  ELB = 'ELB',
+  /**
+   * EBS Health Check
+   */
+  EBS = 'EBS',
+  /**
+   * VPC LATTICE Health Check
+   */
+  VPC_LATTICE = 'VPC_LATTICE',
+}


I considered allowing the EC2 type to be specified, and then internally excluding the EC2 if it was specified, but I thought that users might be confused when they looked at the generated CFn template and realised that the EC2 was missing.

I thought it would be good to use the EC2 enum in the ec2 method below instead of specifiying it as string. But I agree and makes sense that it might confuse users of having EC2 in the enum when it can't be used.

public static ec2(options: Ec2HealthChecksOptions = {}): HealthChecks { return new HealthChecks(['EC2'], options.gracePeriod); }

Also, the method name has been changed to withAdditionalChecks(), but the name AdditionalHealthCheckType for the type name is still unnatural?

I think should be ok since with withAdditionalChecks it is more clearer.

mergify · 2025-02-26T20:06:05Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2025-02-26T20:36:49Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: a8f5b81
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2025-02-26T20:36:55Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2025-02-26T20:37:05Z

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.

feat(autoscaling): add new HealthChecks instead of old HealthCheck

fa18206

go-to-k changed the title ~~feat(autoscaling): add new HealthChecks instead of old HealthCheck~~ feat(autoscaling): add new HealthChecks instead of old HealthCheck Sep 2, 2024

github-actions bot added p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Sep 2, 2024

aws-cdk-automation requested a review from a team September 2, 2024 11:02

go-to-k changed the title ~~feat(autoscaling): add new HealthChecks instead of old HealthCheck~~ feat(autoscaling): add new HealthChecks for multiple health check types, including EBS and VPC_LATTICE types Sep 2, 2024

go-to-k added 2 commits September 2, 2024 20:19

fix rosetta

312e17c

Merge branch 'main' into asg-health-checks

42e4b61

go-to-k marked this pull request as ready for review September 2, 2024 11:28

go-to-k commented Sep 2, 2024

View reviewed changes

go-to-k added 3 commits September 2, 2024 20:37

add an unit test

f77e0da

fix for rosetta

c912660

snapshots

3dc2b9c

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 2, 2024

github-actions bot added the feature-request A feature should be added or improved. label Sep 2, 2024

go-to-k added 5 commits September 2, 2024 22:23

jsdoc

9952bf9

Update integ.health-checks.ts

cc98d1f

Update auto-scaling-group.ts

d89dd71

Update auto-scaling-group.test.ts

16e2b30

typo

43f5860

github-actions bot mentioned this pull request Sep 9, 2024

Weekly PR metrics report #31370

Closed

QuantumNeuralCoder assigned godwingrs22 Feb 4, 2025

godwingrs22 reviewed Feb 24, 2025

View reviewed changes

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into asg-he…

3acd27d

…alth-checks

go-to-k commented Feb 25, 2025

View reviewed changes

go-to-k added 5 commits February 25, 2025 19:41

use UnscopedValidationError

a0135ca

comments

99bec6b

add tests without grace

3ffd7ea

raise if both healthCheck and healthChecks are specified

8601b3e

tweak for tests

4c959a7

go-to-k requested a review from godwingrs22 February 25, 2025 11:02

godwingrs22 reviewed Feb 25, 2025

View reviewed changes

go-to-k added 4 commits February 26, 2025 12:48

change name to gracePeriod

09f7dcd

change method name to withAdditionalChecks

8fc06d9

use withAdditionalChecks in README

7f7aa38

improve README

97b3be8

go-to-k requested a review from godwingrs22 February 26, 2025 06:57

godwingrs22 approved these changes Feb 26, 2025

View reviewed changes

Merge branch 'main' into asg-health-checks

a8f5b81

mergify bot merged commit b3edd0d into aws:main Feb 26, 2025
20 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Feb 26, 2025

go-to-k deleted the asg-health-checks branch February 26, 2025 22:55

feat(autoscaling): add new HealthChecks for multiple health check types, including EBS and VPC_LATTICE types #31286

feat(autoscaling): add new HealthChecks for multiple health check types, including EBS and VPC_LATTICE types #31286

Conversation

go-to-k commented Sep 2, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Choose a reason for hiding this comment

go-to-k Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

godwingrs22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k left a comment

Choose a reason for hiding this comment

go-to-k Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 25, 2025 • edited Loading

Codecov Report

godwingrs22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k commented Feb 26, 2025

godwingrs22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Feb 26, 2025

aws-cdk-automation commented Feb 26, 2025

AWS CodeBuild CI Report

mergify bot commented Feb 26, 2025

github-actions bot commented Feb 26, 2025

feat(autoscaling): add new `HealthChecks` for multiple health check types, including EBS and VPC_LATTICE types #31286

feat(autoscaling): add new `HealthChecks` for multiple health check types, including EBS and VPC_LATTICE types #31286

go-to-k commented Sep 2, 2024 •

edited

Loading

go-to-k Sep 2, 2024 •

edited

Loading

go-to-k Feb 25, 2025 •

edited

Loading

go-to-k Feb 25, 2025 •

edited

Loading

codecov bot commented Feb 25, 2025 •

edited

Loading