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(route53): added L2 construct for Route53's health checks #30739

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

wladyslawczyzewski
Copy link

Issue # (if applicable)

Part of #9481
Another PR for this ticket #30664

Reason for this change

Added L2 construct for AWS::Route53::HealthCheck resource

Description of changes

The changes only introduces the L2 construct for Route53 health check resources. Except the L2 construct itself, I added basic validations for the input props.

Description of how you validated changes

  • unit tests
  • integration tests

Checklist


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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 3, 2024
Copy link
Contributor

@nmussy nmussy left a comment

Choose a reason for hiding this comment

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

Apologies for the number of requested changes, but it's a really good first pass. I would just be mindful of copy-pasting CloudFormation docs for the property JSDocs, some of them don't make as much sense in the context of the CDK 👍

alarmIdentifier: props.alarmIdentifier,
childHealthChecks: props.childHealthChecks,
enableSni: props.enableSNI,
failureThreshold: props.failureThreshold ?? 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have a default value for failureThreshold? CloudFormation doesn't have a default value, and doesn't even require one, see docs

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy the documentation states "If you don't specify a value for FailureThreshold, the default value is three health checks."

Copy link
Contributor

Choose a reason for hiding this comment

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

My main issue is that it's an optional Cfn parameter, and by having a default value here you are preventing CDK users from being able to pass a null-ish value. I'll let the maintainer weigh in here, I don't think it's a huge issue either way

measureLatency: props.measureLatency,
port: props.port,
regions: props.regions,
requestInterval: (props.requestInterval && props.requestInterval.toSeconds()) ?? 30,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just let CloudFormation set its default value, no need for the CDK to do it, see docs

Suggested change
requestInterval: (props.requestInterval && props.requestInterval.toSeconds()) ?? 30,
requestInterval: props.requestInterval?.toSeconds(),

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy according to design guidelines in some cases there's most certainly the default value even if the CF docs doesn't specify the one. In this case, the default value is 30 seconds because the CF docs states it's a default value for this prop when other doesn't specified and because the design guidelines mention the best when how to determine the default value for the prop:

A good way to determine what's the right sensible default is to refer to the AWS Console resource creation experience. In many cases, there will be alignment.

The console sets the default value for the health check to 30 seconds.

Copy link
Author

Choose a reason for hiding this comment

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

NB. changed the implementation to use optional chaining operator

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +391 to +403
function validateIpAddress(props: HealthCheckProps) {
if (props.ipAddress === undefined) {
return;
}

if (
!new RegExp(
'^((([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]).){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))$|^(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]).){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]).){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))$',
).test(props.ipAddress)
) {
throw new Error('IpAddress must be a valid IPv4 or IPv6 address');
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a bit overkill, especially given how complex the RegExp is. I would remove this function and let CloudFormation validate it

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy this is the regex from the official CF documentation, imho it's better to fail on synthesize phase rather than on deployment phase

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I'll leave it up to the maintainer review

Comment on lines 219 to 222
function validateProperties(props: HealthCheckProps) {
switch (props.type) {
case HealthCheckType.HTTP: {
ruleAlarmIdentifierIsNotAllowed(props);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably refactor this switch, given how many types and functions there are. You already have a common signature, so you can use a Record to map out the functions for each type, something like:

const rules: Record<HealthCheckType, (props: HealthCheckProps) => void> = {
  [HealthCheckType.HTTP]: [ruleAlarmIdentifierIsNotAllowed, ruleEnableSNIIsNotAllowed, ...],
  // ...
];

function validateProperties(props: HealthCheckProps) {
  for (const rule of rules[props.type]) rule(props);

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy thanks for this advice – reimplemented this as you suggested ✅

Comment on lines 304 to 305
default:
throw new Error(`Unsupported health check type: ${props.type}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Switching to a map would also remove the need for this exception, since the rules type will require that all possible HealthCheckType values are covered

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy removed the switch in favour of rules as you suggested ✅

/**
* Specify whether you want Amazon Route 53 to invert the status of a health check, so a health check that would normally be considered unhealthy is considered healthy, and vice versa.
*
* @default - not configured
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @default - not configured
* @default false

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy changed the default value to false

Copy link
Contributor

Choose a reason for hiding this comment

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

See comment about setting the default value: #30739 (comment)

/**
* Specify whether you want Amazon Route 53 to measure the latency between health checkers in multiple AWS regions and your endpoint, and to display CloudWatch latency graphs on the Health Checks page in the Route 53 console.
*
* @default - not configured
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, not explicitly said, but I'm going to assume the default is false

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy changed the default value to false. thanks for this!

Copy link
Contributor

Choose a reason for hiding this comment

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

See comment about setting the default value: #30739 (comment)

readonly alarmIdentifier?: AlarmIdentifier;

/**
* A complex type that contains one ChildHealthCheck element for each health check that you want to associate with a CALCULATED health check.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Complex types" don't make a ton of sense in the CDK, this is just an array

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy updated the docstring, thanks for flagging this

Comment on lines 117 to 119
* A complex type that contains one Region element for each region from which you want Amazon Route 53 health checkers to check the specified endpoint.
*
* @default - not configured
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value is documented in the Cfn docs. Also, "complex type" and "Region element" sound a bit off here. I assume it's just "an array of region identifiers", but you can add this to your integration tests to make sure

Suggested change
* A complex type that contains one Region element for each region from which you want Amazon Route 53 health checkers to check the specified endpoint.
*
* @default - not configured
* An array of region identifiers from which you want Amazon Route 53 health checkers to check the specified endpoint.
*
* @default - performs checks from all of the regions that are listed under Valid Values.

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy thanks for this, I have refactored this part of the construct to make it strong-typed and added some validations, please check It out ✅

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the doc update, I've added some comments about the typing and validation though:
https://github.com/aws/aws-cdk/pull/30739/files#r1671746172
https://github.com/aws/aws-cdk/pull/30739/files#r1671739388

Comment on lines 124 to 126
* The number of seconds between the time that Amazon Route 53 gets a response from your endpoint and the time that it sends the next health check request. Each Route 53 health checker makes requests at this interval.
*
* @default 30
Copy link
Contributor

@nmussy nmussy Jul 4, 2024

Choose a reason for hiding this comment

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

"30" is not a valid Duration, and we don't ask CDK users for a number of seconds here, just a duration

Suggested change
* The number of seconds between the time that Amazon Route 53 gets a response from your endpoint and the time that it sends the next health check request. Each Route 53 health checker makes requests at this interval.
*
* @default 30
* The duration between the time that Amazon Route 53 gets a response from your endpoint and the time that it sends the next health check request. Each Route 53 health checker makes requests at this interval.
*
* @default Duration.seconds(30)

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy updated the docstring for this property, please check it

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 4, 2024
Comment on lines +213 to +214
// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Route53::RecordSet', {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add an assertion for a AWS::Route53::HealthCheck

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy thanks for this – added ✅

import { HealthCheck, HealthCheckType } from '../lib';

describe('health check', () => {
test('resolves routing control arn', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add tests to make sure all possible properties are properly mapped to their corresponding HealthCheckConfig keys

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy added more unit tests, please check it out ✅

@@ -188,6 +188,36 @@ describe('record set', () => {
});
});

test('A record with health check', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add a test to make sure that imported health checks can be properly mapped to Record sets

Copy link
Author

Choose a reason for hiding this comment

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

@nmussy added additional test for that test case ✅

Copy link
Author

@wladyslawczyzewski wladyslawczyzewski left a comment

Choose a reason for hiding this comment

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

hi @nmussy , thanks for you review – I applied some changes according to your comments, could you please review them? thanks in advance!

alarmIdentifier: props.alarmIdentifier,
childHealthChecks: props.childHealthChecks,
enableSni: props.enableSNI,
failureThreshold: props.failureThreshold ?? 3,
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy the documentation states "If you don't specify a value for FailureThreshold, the default value is three health checks."

measureLatency: props.measureLatency,
port: props.port,
regions: props.regions,
requestInterval: (props.requestInterval && props.requestInterval.toSeconds()) ?? 30,
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy according to design guidelines in some cases there's most certainly the default value even if the CF docs doesn't specify the one. In this case, the default value is 30 seconds because the CF docs states it's a default value for this prop when other doesn't specified and because the design guidelines mention the best when how to determine the default value for the prop:

A good way to determine what's the right sensible default is to refer to the AWS Console resource creation experience. In many cases, there will be alignment.

The console sets the default value for the health check to 30 seconds.

measureLatency: props.measureLatency,
port: props.port,
regions: props.regions,
requestInterval: (props.requestInterval && props.requestInterval.toSeconds()) ?? 30,
Copy link
Author

Choose a reason for hiding this comment

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

NB. changed the implementation to use optional chaining operator

Comment on lines 319 to 323
if (props.searchString === null || props.searchString === undefined) {
throw new Error(`SearchString is required for health check type: ${props.type}`);
}

if (props.searchString.length === 0 || props.searchString.length > 255) {
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy not providing the value (undefined or null) is not the same as providing the empty value (''), the error messages clearly states that in one case the value is required and that the length of the value should be between 1 and 255 in another.

Comment on lines +391 to +403
function validateIpAddress(props: HealthCheckProps) {
if (props.ipAddress === undefined) {
return;
}

if (
!new RegExp(
'^((([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]).){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))$|^(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]).){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]).){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))$',
).test(props.ipAddress)
) {
throw new Error('IpAddress must be a valid IPv4 or IPv6 address');
}
}
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy this is the regex from the official CF documentation, imho it's better to fail on synthesize phase rather than on deployment phase

@@ -220,6 +220,37 @@ Constructs are available for A, AAAA, CAA, CNAME, MX, NS, SRV and TXT records.
Use the `CaaAmazonRecord` construct to easily restrict certificate authorities
allowed to issue certificates for a domain to Amazon only.

### Health Checks

You can associate health checks with records:
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy thanks for this – updated the README file with the description of what the health checks are and how to use them with the example.

});
```

Possible values for `type` are `HTTP`, `HTTPS`, `TCP`, `CLOUDWATCH_METRIC`, `CALCULATED` and `RECOVERY_CONTROL`.
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy update the docs to redirect users to CF docs ✅

Comment on lines 71 to 75
new IntegTest(app, 'integ-test', {
testCases: [stack],
diffAssets: true,
enableLookups: true,
});
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy updated the integration tests, please check it out ✅

import { HealthCheck, HealthCheckType } from '../lib';

describe('health check', () => {
test('resolves routing control arn', () => {
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy added more unit tests, please check it out ✅

Comment on lines 117 to 119
* A complex type that contains one Region element for each region from which you want Amazon Route 53 health checkers to check the specified endpoint.
*
* @default - not configured
Copy link
Author

Choose a reason for hiding this comment

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

@nmussy thanks for this, I have refactored this part of the construct to make it strong-typed and added some validations, please check It out ✅

case HealthCheckType.RECOVERY_CONTROL:
return undefined;
default:
return VALID_REGIONS;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be a pain to maintain in the future when new regions are added to this list, when we can just let Cfn set all the valid regions by itself

Comment on lines 185 to 192
/**
* An array of region identifiers that you want Amazon Route 53 health checkers to check the health of the endpoint from.
*
* Valid values are: 'us-east-1', 'us-west-1', 'us-west-2', 'eu-west-1', 'ap-southeast-1', 'ap-southeast-2', 'ap-northeast-1', 'sa-east-1'
* @default - if the type is CALCULATED, CLOUDWATCH_METRIC, or RECOVERY_CONTROL, this property is not configured.
* - otherwise, the default value is all valid regions: 'us-east-1', 'us-west-1', 'us-west-2', 'eu-west-1', 'ap-southeast-1', 'ap-southeast-2', 'ap-northeast-1', 'sa-east-1'
*/
readonly regions?: ('us-east-1' | 'us-west-1' | 'us-west-2' | 'eu-west-1' | 'ap-southeast-1' | 'ap-southeast-2' | 'ap-northeast-1' | 'sa-east-1')[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same maintainability concerns for this JSDoc and this type, I would just revert this type to a string[] and add a link to the Cfn documentation.

Just as an FYI, you could have used a const assertion to derive this type from the VALID_REGIONS array, see this example Playground

Copy link
Contributor

@nmussy nmussy left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I had a few more comments

@wladyslawczyzewski
Copy link
Author

hi @nmussy , thanks for you review!

I made the regions weak-typed string[] property and as result passing all the validation responsibility to CloudFormation itself, however, I think having this prop strong-typed will be better option even if this will require more maintainer work in the future – not all AWS' regions are used for health checks, therefore user may provide the wrong value and will get the error only on deployment phase.

On the default values stuff – you're having a valid points why to not provide default values in the CDK codebase, however, each of that properties have implicit default values, imho, it's better to let users know what the default value is and if they indeed want to have the resource configured differently, they will need to provide their configuration explicitly in the code rather then relying on CloudFormation hidden decisions.

I pushed few changes according to your last review, could you please take a look and let me know if you see anything else that could be improved?

Copy link
Contributor

@nmussy nmussy left a comment

Choose a reason for hiding this comment

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

LGTM, I'll let the maintainer make a decision regarding the default values. I'm also not sure using weight: 0 is a great example, see comment

new route53.ARecord(this, 'ARecord2', {
zone: myZone,
target: route53.RecordTarget.fromIpAddresses('5.6.7.8'),
weight: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure using a zero weight here is the best example, it's a pretty complex behavior with a few pitfalls, see Weight and How Amazon Route 53 chooses records when health checking is configured

Suggested change
weight: 0,
weight: 50,

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 6, 2024
@shikha372 shikha372 self-assigned this Sep 24, 2024
}
}

const validationRules: Record<HealthCheckType, ((props: HealthCheckProps) => void)[]> = {
Copy link
Contributor

@shikha372 shikha372 Oct 1, 2024

Choose a reason for hiding this comment

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

instead of having all these functions to validate these scenarios, can we include something like a enum based properties or class to define these props, i think one of the design principles we follow is to prevent many check cases.

The fewer “if statements” the better.

Copy link
Author

Choose a reason for hiding this comment

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

sorry @shikha372 could you please explain this further?

});
```

See the [Route 53 Health Checks documentation](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-route53-healthcheck-healthcheckconfig.html#cfn-route53-healthcheck-healthcheckconfig-type) for possible types of health checks.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest adding these documentation links upward so that its easier to get the overview before looking at the implementation in CDK

Copy link
Author

Choose a reason for hiding this comment

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

@shikha372 moved this link to the top of the Health Checks section

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants