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

ROUTE53: Allow R53_ALIAS records to enable target health evaluation #2649

Merged

Conversation

jonathanbouvier
Copy link
Contributor

@jonathanbouvier jonathanbouvier commented Nov 25, 2023

R53_ALIAS records have a property which allows them to evaluate target health. This was previously hardcoded to false for all R53_ALIAS records.

This has been added as a new record modifier instead of a new parameter to R53_ALIAS to remain backwards compatible. The previous default behavior has been maintained.

  • Add R53_EVALUATE_TARGET_HEALTH record modifier to enable this behavior
  • Update provider to account for this setting during record conversion
  • Update RecordConfig receivers to allow this setting to be detected during diff
  • Update getZones command to include this setting in dumped config
  • Update tests
  • Update documentation

@tlimoncelli
Copy link
Contributor

CC @vatsalyagoel for visibility.

Wow! Jonathan, I gotta say this is an impressive PR! Adding record meta modifiers is not well documented but it looks like you got it all right! Plus you've included documentation and tests! Thanks!

I'll merge this before the next release unless @vatsalyagoel has any comments/corrections.

@jonathanbouvier
Copy link
Contributor Author

Thanks @tlimoncelli ! Happy to help.

I'm looking to use dnscontrol to manage some of my more complicated Route53 zones, but I think I'm going to need some additional features in the Route53 provider. Alias target health evaluation was just one of the first challenges I encountered.

Eventually, I'd love to add support for some (or all) of the various routing policies supported by Route53:
https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/routing-policy.html

@tlimoncelli
Copy link
Contributor

Oops! I meant to CC @tresni instead of @vatsalyagoel. (Copy-and-paste error!)

@tlimoncelli
Copy link
Contributor

Eventually, I'd love to add support for some (or all) of the various routing policies supported by Route53:

Sounds good to me!

Oh, I just noticed one thing I'd like to see added to this PR. Could you add integration tests to integrationTest/integration_test.go? You could probably just add to the existing tests. Search for ROUTE53 features to see them.

R53_ALIAS records have a property which allows them to evaluate target
health. This was previously hardcoded to false for all R53_ALIAS records.

This has been added as a new record modifier instead of a new parameter
to R53_ALIAS to remain backwards compatible. The previous default behavior
has been maintained.

- Add R53_EVALUATE_TARGET_HEALTH record modifier to enable this behavior
- Update provider to account for this setting during record conversion
- Update RecordConfig receivers to allow this setting to be detected during diff
- Update getZones command to include this setting in dumped config
- Update unit tests
- Update integration tests
- Update documentation
@jonathanbouvier
Copy link
Contributor Author

Eventually, I'd love to add support for some (or all) of the various routing policies supported by Route53:

Sounds good to me!

Oh, I just noticed one thing I'd like to see added to this PR. Could you add integration tests to integrationTest/integration_test.go? You could probably just add to the existing tests. Search for ROUTE53 features to see them.

I just modified/added integration tests.

go test -provider ROUTE53
PASS
ok  	github.com/StackExchange/dnscontrol/v4/integrationTest	107.350s

@tlimoncelli
Copy link
Contributor

Great! Thanks!

@tlimoncelli tlimoncelli changed the title Allow R53_ALIAS records to enable target health evaluation ROUTE53: Allow R53_ALIAS records to enable target health evaluation Nov 27, 2023
@tlimoncelli tlimoncelli merged commit e783d70 into StackExchange:master Nov 27, 2023
2 checks passed
@jonathanbouvier jonathanbouvier deleted the evaluate_target_health branch November 28, 2023 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants