-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
There was a problem hiding this 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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
requestInterval: (props.requestInterval && props.requestInterval.toSeconds()) ?? 30, | |
requestInterval: props.requestInterval?.toSeconds(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #30739 (comment)
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'); | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
function validateProperties(props: HealthCheckProps) { | ||
switch (props.type) { | ||
case HealthCheckType.HTTP: { | ||
ruleAlarmIdentifierIsNotAllowed(props); |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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 ✅
default: | ||
throw new Error(`Unsupported health check type: ${props.type}`); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @default - not configured | |
* @default false |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
* 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 |
There was a problem hiding this comment.
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
* 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. |
There was a problem hiding this comment.
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 ✅
There was a problem hiding this comment.
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
* 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 |
There was a problem hiding this comment.
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
* 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) |
There was a problem hiding this comment.
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
// THEN | ||
Template.fromStack(stack).hasResourceProperties('AWS::Route53::RecordSet', { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ✅
few validation changes use IHeatlCheck instead of string for `childHealthChecks` props
integration test
also updated the unit tests and integration tests for this change
There was a problem hiding this 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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
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) { |
There was a problem hiding this comment.
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.
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'); | ||
} | ||
} |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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`. |
There was a problem hiding this comment.
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 ✅
new IntegTest(app, 'integ-test', { | ||
testCases: [stack], | ||
diffAssets: true, | ||
enableLookups: true, | ||
}); |
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
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 ✅
* 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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
/** | ||
* 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')[]; |
There was a problem hiding this comment.
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
There was a problem hiding this 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
}); | ||
``` | ||
|
||
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #30739 +/- ##
==========================================
+ Coverage 78.66% 78.67% +0.01%
==========================================
Files 107 107
Lines 7237 7237
Branches 1329 1329
==========================================
+ Hits 5693 5694 +1
+ Misses 1358 1357 -1
Partials 186 186
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi sorry for keep you waiting on this PR. It's great work and it's a lot of work done and I'll try to help you to get this merged ASAP.
Left some comments to get your thoughts.
Co-authored-by: GZ <[email protected]>
hi @GavinZZ , could you please re-review the PR – I've deleted the props validation piece as it was too opinionated, I prefer to have the validation to be in synthesis phase instead deployment as this reduces the feedback time by a lot, but I also see the arguments against this. |
IMO I am usually in favour of synthesis validation rather than deployment validation. However, the validation was too complicated and I think we should only include the essential/most obvious ones in the synthesis validation. Either way, I think it's a two way door to release this feature without the validation first and we can always add more validation in the future. |
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). |
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). |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Part of #9481
Another PR for this ticket #30664
Reason for this change
Added L2 construct for
AWS::Route53::HealthCheck
resourceDescription 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
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license