Configurable geo healthcheck host & path #67
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- This reworks the CallerReference structure for Route53 health checks in a non-backwards compatible way. This means records will create new healthchecks for themselves which should be fine. - Since we're pre 1.0, support has NOT been added to cleanup the old healthchecks. That could be done reasonably easy, BUT we'd have to keep that around forever. The hope is that the new ref format/usage will prevent this problem going forward since enough info exists in the ref to identify things fully. :fingers_crossed: - healthcheck GC is much cleaner and more robust thanks to ^ - overall the healthcheck management code is a bit easier to follow and more robust now.
…t configed Fixes an issue where we'd be looking for custom healthcheck config on the existing record object (from the provider) which would never have a custom setup. Instead looking at desired lets us find what's actually configured to be the case
Also fixes some mocking data to match what the Dyn client libs are expecting.
More thorough unit tests while I'm in here. Ended up doing some hacks/monkey patching of DSFMonitor as the client library's object doesn't seem to be fully functional/useful and has inconsitent behavior.
Would have prevented both A and AAAA exisiting on the same node.
|
@ross can I some how help with this PR so it gets merged :) ? |
Make the Record.octodns bits more consistent
ross
commented
Apr 2, 2018
| @@ -64,7 +64,8 @@ def plan(self, desired): | |||
| self.log.info('plan: filtered out %s changes', before - after) | |||
|
|
|||
| # allow the provider to add extra changes it needs | |||
| extra = self._extra_changes(existing, changes) | |||
| extra = self._extra_changes(existing=existing, desired=desired, | |||
| changes=changes) | |||
There was a problem hiding this comment.
This change allows providers to catch them by name rather than position and thus ignore the params they don't want with **kwargs.
| @@ -230,7 +230,7 @@ class Route53Provider(BaseProvider): | |||
|
|
|||
| # This should be bumped when there are underlying changes made to the | |||
| # health check config. | |||
| HEALTH_CHECK_VERSION = '0000' | |||
| HEALTH_CHECK_VERSION = '0001' | |||
There was a problem hiding this comment.
Have to bump Route53's HC version, this will cause a replacement of all health-checks via _extra_changes including all Geo records in the plan.
octodns/provider/route53.py
Outdated
| ref = '{}:{}:{}'.format(self.HEALTH_CHECK_VERSION, record._type, | ||
| uuid4().hex[:16]) | ||
| ref = '{}:{}:{}:{}'.format(self.HEALTH_CHECK_VERSION, record._type, | ||
| record.name, uuid4().hex[:16]) |
There was a problem hiding this comment.
Include the record name in the healthcheck ref, not just the type.
theojulienne
approved these changes
Apr 4, 2018
octodns/provider/dyn.py
Outdated
| label) | ||
| extra.append(Update(record, record)) | ||
| continue | ||
| if _monitor_matches(monitor, record.healthcheck_host, |
There was a problem hiding this comment.
_monitor_matches seems to actually be checking if the monitor has any differences to the variables provided. Possibly invert the logic?
dbussink
approved these changes
Apr 4, 2018
This will ensure unique refs for different zones. Without them the ref isn't enough to make sure we're looking at the right thing (notably when we're gc'ing old health checks.) This also adds a bit more debugging around health checks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#This adds new
octodnscustom config values to records to support providing custom healthcheck paths and hosts. E.g.NOTE: this will not clean up the previous Route53 healthchecks do to changes made in the way theirCallerRefereneces work.