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

feat(elasticloadbalancingv2): health check interval greater than timeout #29075

Merged
merged 9 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,6 @@ export class NetworkTargetGroup extends TargetGroupBase implements INetworkTarge
if (timeoutSeconds < lowHealthCheckTimeout || timeoutSeconds > highHealthCheckTimeout) {
ret.push(`Health check timeout '${timeoutSeconds}' not supported. Must be a number between ${lowHealthCheckTimeout} and ${highHealthCheckTimeout}.`);
}
if (healthCheck.interval && healthCheck.interval.toSeconds() < timeoutSeconds) {
ret.push(`Health check timeout '${timeoutSeconds}' must not be greater than the interval '${healthCheck.interval.toSeconds()}'`);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving this validation to shared

}

return ret;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ export abstract class TargetGroupBase extends Construct implements ITargetGroup
this.targetGroupName = this.resource.attrTargetGroupName;
this.defaultPort = additionalProps.port;

this.node.addValidation({ validate: () => this.validateHealthCheck() });
this.node.addValidation({ validate: () => this.validateTargetGroup() });
}

Expand Down Expand Up @@ -351,6 +352,22 @@ export abstract class TargetGroupBase extends Construct implements ITargetGroup

return ret;
}

protected validateHealthCheck(): string[] {
const ret = new Array<string>();

const intervalSeconds = this.healthCheck.interval?.toSeconds();
const timeoutSeconds = this.healthCheck.timeout?.toSeconds();

if (intervalSeconds && timeoutSeconds) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work as expected if either intervalSeconds or timeoutSeconds equals to zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HealthCheckTimeoutSeconds has a range of 2-120 and HealthCheckIntervalSeconds is 5-300. We have separate checks on the ranges, so they'll never be 0.

if (intervalSeconds < timeoutSeconds) {
// < instead of <= for backwards compatibility, see discussion in https://github.com/aws/aws-cdk/pull/26031
ret.push('Health check interval must be greater than or equal to the timeout; received interval ' +
`${intervalSeconds}, timeout ${timeoutSeconds}.`);
}
}
return ret;
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,45 @@ describe('tests', () => {
}).toThrow('Healthcheck interval 1 minute must be greater than the timeout 2 minute');
});

test('Throws error for health check interval less than timeout', () => {
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const vpc = new ec2.Vpc(stack, 'Vpc');

new elbv2.ApplicationTargetGroup(stack, 'TargetGroup', {
vpc,
port: 80,
healthCheck: {
interval: cdk.Duration.seconds(10),
timeout: cdk.Duration.seconds(20),
},
});

expect(() => {
app.synth();
}).toThrow('Health check interval must be greater than or equal to the timeout; received interval 10, timeout 20.');
});

// for backwards compatibility these can be equal, see discussion in https://github.com/aws/aws-cdk/pull/26031
test('Throws error for health check interval less than timeout', () => {
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const vpc = new ec2.Vpc(stack, 'Vpc');

new elbv2.ApplicationTargetGroup(stack, 'TargetGroup', {
vpc,
port: 80,
healthCheck: {
interval: cdk.Duration.seconds(10),
timeout: cdk.Duration.seconds(20),
},
});

expect(() => {
app.synth();
}).toThrow('Health check interval must be greater than or equal to the timeout; received interval 10, timeout 20.');
});

test('imported targetGroup has targetGroupName', () => {
// GIVEN
const app = new cdk.App();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ describe('tests', () => {
// THEN
const validationErrors: string[] = targetGroup.node.validate();
expect(validationErrors).toEqual([
"Health check timeout '40' must not be greater than the interval '30'",
'Health check interval must be greater than or equal to the timeout; received interval 30, timeout 40.',
]);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,45 @@ describe('tests', () => {
}).toThrow(/Health check interval '3' not supported. Must be between 5 and 300./);
});

test('Throws error for health check interval less than timeout', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an edge case test for interval == timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for this. See discussion above on why we chose to allow them to be equal.

const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const vpc = new ec2.Vpc(stack, 'Vpc');

new elbv2.NetworkTargetGroup(stack, 'Group', {
vpc,
port: 80,
healthCheck: {
interval: cdk.Duration.seconds(10),
timeout: cdk.Duration.seconds(20),
},
});

expect(() => {
app.synth();
}).toThrow('Health check interval must be greater than or equal to the timeout; received interval 10, timeout 20.');
});

// for backwards compatibility these can be equal, see discussion in https://github.com/aws/aws-cdk/pull/26031
test('No error for health check interval == timeout', () => {
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const vpc = new ec2.Vpc(stack, 'Vpc');

new elbv2.NetworkTargetGroup(stack, 'Group', {
vpc,
port: 80,
healthCheck: {
interval: cdk.Duration.seconds(10),
timeout: cdk.Duration.seconds(10),
},
});

expect(() => {
app.synth();
}).not.toThrow();
});

test('targetGroupName unallowed: more than 32 characters', () => {
// GIVEN
const app = new cdk.App();
Expand Down