From ad9c8bb34905fc28f8ff2294382f7d05c0a3ce55 Mon Sep 17 00:00:00 2001 From: Akash Askoolum Date: Thu, 26 Aug 2021 13:55:31 +0100 Subject: [PATCH 1/2] fix: Ensure the healthcheck for a Target Group is legal A Target Group's healthcheck must have a timeout lower than the interval. Annoyingly, AWS CDK doesn't catch this and will happily synth a stack that violates this rule. This change checks that `timeout` is less than `interval` and throws otherwise to prevent synth from completing. See: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-elb-health-check.html#cfn-elb-healthcheck-timeout. --- .../alb/application-target-group.test.ts | 17 ++++++++++- .../alb/application-target-group.ts | 28 +++++++++++++++---- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/constructs/loadbalancing/alb/application-target-group.test.ts b/src/constructs/loadbalancing/alb/application-target-group.test.ts index 71f0b2f506..54cfdf94c5 100644 --- a/src/constructs/loadbalancing/alb/application-target-group.test.ts +++ b/src/constructs/loadbalancing/alb/application-target-group.test.ts @@ -2,7 +2,7 @@ import "@aws-cdk/assert/jest"; import "../../../utils/test/jest"; import { Vpc } from "@aws-cdk/aws-ec2"; import { ApplicationProtocol } from "@aws-cdk/aws-elasticloadbalancingv2"; -import { Stack } from "@aws-cdk/core"; +import { Duration, Stack } from "@aws-cdk/core"; import { simpleGuStackForTesting } from "../../../utils/test"; import type { AppIdentity } from "../../core/identity"; import { GuApplicationTargetGroup } from "./application-target-group"; @@ -120,4 +120,19 @@ describe("The GuApplicationTargetGroup class", () => { Protocol: "HTTPS", }); }); + + test("An illegal healthcheck is flagged", () => { + const stack = simpleGuStackForTesting(); + + expect(() => { + new GuApplicationTargetGroup(stack, "TargetGroup", { + ...app, + vpc, + healthCheck: { + interval: Duration.seconds(10), + timeout: Duration.seconds(10), + }, + }); + }).toThrow(new Error("Illegal healthcheck configuration: timeout (10) must be lower than interval (10)")); + }); }); diff --git a/src/constructs/loadbalancing/alb/application-target-group.ts b/src/constructs/loadbalancing/alb/application-target-group.ts index c402e688db..371a8f2cb9 100644 --- a/src/constructs/loadbalancing/alb/application-target-group.ts +++ b/src/constructs/loadbalancing/alb/application-target-group.ts @@ -1,6 +1,6 @@ import { ApplicationProtocol, ApplicationTargetGroup, Protocol } from "@aws-cdk/aws-elasticloadbalancingv2"; -import type { ApplicationTargetGroupProps } from "@aws-cdk/aws-elasticloadbalancingv2"; -import { Duration } from "@aws-cdk/core"; +import type { ApplicationTargetGroupProps, HealthCheck } from "@aws-cdk/aws-elasticloadbalancingv2"; +import { Annotations, Duration } from "@aws-cdk/core"; import { GuStatefulMigratableConstruct } from "../../../utils/mixin"; import type { GuStack } from "../../core"; import { AppIdentity } from "../../core/identity"; @@ -34,19 +34,22 @@ export interface GuApplicationTargetGroupProps extends ApplicationTargetGroupPro * ``` */ export class GuApplicationTargetGroup extends GuStatefulMigratableConstruct(ApplicationTargetGroup) { - static DefaultHealthCheck = { + private static defaultHealthcheckInterval = Duration.seconds(10); + private static defaultHealthcheckTimeout = Duration.seconds(5); + + static DefaultHealthCheck: HealthCheck = { path: "/healthcheck", protocol: Protocol.HTTP, healthyThresholdCount: 5, unhealthyThresholdCount: 2, - interval: Duration.seconds(10), - timeout: Duration.seconds(10), + interval: GuApplicationTargetGroup.defaultHealthcheckInterval, + timeout: GuApplicationTargetGroup.defaultHealthcheckTimeout, }; constructor(scope: GuStack, id: string, props: GuApplicationTargetGroupProps) { const { app } = props; - const mergedProps = { + const mergedProps: ApplicationTargetGroupProps = { protocol: ApplicationProtocol.HTTP, // We terminate HTTPS at the load balancer level, so load balancer to ASG/EC2 traffic can be over HTTP deregistrationDelay: Duration.seconds(30), ...props, @@ -55,6 +58,19 @@ export class GuApplicationTargetGroup extends GuStatefulMigratableConstruct(Appl super(scope, AppIdentity.suffixText({ app }, id), mergedProps); + const interval = mergedProps.healthCheck?.interval ?? GuApplicationTargetGroup.defaultHealthcheckInterval; + const timeout = mergedProps.healthCheck?.timeout ?? GuApplicationTargetGroup.defaultHealthcheckTimeout; + + /* + The healthcheck `timeout` must be lower than `interval` + See https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-elb-health-check.html#cfn-elb-healthcheck-timeout + */ + if (timeout.toSeconds() >= interval.toSeconds()) { + const message = `Illegal healthcheck configuration: timeout (${timeout.toSeconds()}) must be lower than interval (${interval.toSeconds()})`; + Annotations.of(this).addError(message); // adds a useful message to the console to aid debugging + throw new Error(message); + } + AppIdentity.taggedConstruct({ app }, this); } } From 711f9b02394c7e527b84b2e807931dc676459bc8 Mon Sep 17 00:00:00 2001 From: Akash Askoolum Date: Thu, 26 Aug 2021 14:06:57 +0100 Subject: [PATCH 2/2] chore: Update tests with updated default healthcheck timeout value --- .../cloudwatch/__snapshots__/ec2-alarms.test.ts.snap | 2 +- .../loadbalancing/alb/application-target-group.test.ts | 4 ++-- src/patterns/__snapshots__/ec2-app.test.ts.snap | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/constructs/cloudwatch/__snapshots__/ec2-alarms.test.ts.snap b/src/constructs/cloudwatch/__snapshots__/ec2-alarms.test.ts.snap index a9617a922e..11863f7d69 100644 --- a/src/constructs/cloudwatch/__snapshots__/ec2-alarms.test.ts.snap +++ b/src/constructs/cloudwatch/__snapshots__/ec2-alarms.test.ts.snap @@ -406,7 +406,7 @@ Object { "HealthCheckIntervalSeconds": 10, "HealthCheckPath": "/healthcheck", "HealthCheckProtocol": "HTTP", - "HealthCheckTimeoutSeconds": 10, + "HealthCheckTimeoutSeconds": 5, "HealthyThresholdCount": 5, "Port": 80, "Protocol": "HTTP", diff --git a/src/constructs/loadbalancing/alb/application-target-group.test.ts b/src/constructs/loadbalancing/alb/application-target-group.test.ts index 54cfdf94c5..f234130c28 100644 --- a/src/constructs/loadbalancing/alb/application-target-group.test.ts +++ b/src/constructs/loadbalancing/alb/application-target-group.test.ts @@ -69,7 +69,7 @@ describe("The GuApplicationTargetGroup class", () => { HealthCheckIntervalSeconds: 10, HealthCheckPath: "/healthcheck", HealthCheckProtocol: "HTTP", - HealthCheckTimeoutSeconds: 10, + HealthCheckTimeoutSeconds: 5, HealthyThresholdCount: 5, UnhealthyThresholdCount: 2, }); @@ -91,7 +91,7 @@ describe("The GuApplicationTargetGroup class", () => { HealthCheckPath: "/test", HealthCheckPort: "9000", HealthCheckProtocol: "HTTP", - HealthCheckTimeoutSeconds: 10, + HealthCheckTimeoutSeconds: 5, HealthyThresholdCount: 5, UnhealthyThresholdCount: 2, }); diff --git a/src/patterns/__snapshots__/ec2-app.test.ts.snap b/src/patterns/__snapshots__/ec2-app.test.ts.snap index 1367178205..4b0ed800b9 100644 --- a/src/patterns/__snapshots__/ec2-app.test.ts.snap +++ b/src/patterns/__snapshots__/ec2-app.test.ts.snap @@ -774,7 +774,7 @@ Object { "HealthCheckIntervalSeconds": 10, "HealthCheckPath": "/healthcheck", "HealthCheckProtocol": "HTTP", - "HealthCheckTimeoutSeconds": 10, + "HealthCheckTimeoutSeconds": 5, "HealthyThresholdCount": 5, "Port": 3000, "Protocol": "HTTP", @@ -1562,7 +1562,7 @@ Object { "HealthCheckIntervalSeconds": 10, "HealthCheckPath": "/healthcheck", "HealthCheckProtocol": "HTTP", - "HealthCheckTimeoutSeconds": 10, + "HealthCheckTimeoutSeconds": 5, "HealthyThresholdCount": 5, "Port": 3000, "Protocol": "HTTP",