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: make caching of zones thread-safe #3328

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

Conversation

das7pad
Copy link
Contributor

@das7pad das7pad commented Jan 8, 2025

Hi @tresni!
While reviewing all the ZoneCreator implementations, I noticed that the ROUTE53 provider has an unsafe caching implementation for zones and EnsureZoneExists has a race-condition bug. EnsureZoneExists resets the cache before making the API call for creating a given zone. This might run concurrently to the processing of other zones and in turn could result in unexpected behavior: the cache could get re-populated before the creation of the zone completes and the new zone could be missing from the newly populated cache. This PR is making all access to the zone cache thread-safe and fixing the aforementioned race-condition. Would you mind giving this a try and let me know how it goes? Thanks!

Bonus: The zone cache is no longer reset when creating zones. Instead, the cache is populated with the zone that is included in the API response from creating the zone.

Part of #3007

Also do not reset the cache when creating zones.
Instead, populate the cache with the zone that is included in the API
response from creating the zone.
@tlimoncelli
Copy link
Contributor

This passes integration tests. However those tests don't use any concurrency.

...
        --- PASS: TestDNSProviders/dnscontroltest-r53.com/81:structured_TXT_as_native_records_***SKIPPED(disabled_by_only)***:Empty (0.04s)
        --- PASS: TestDNSProviders/dnscontroltest-r53.com/82:PORKBUN_URLFWD_tests_***SKIPPED(disabled_by_only)***:Empty (0.07s)
        --- PASS: TestDNSProviders/dnscontroltest-r53.com/83:GCORE_metadata_tests_***SKIPPED(disabled_by_only)***:Empty (0.04s)
        --- PASS: TestDNSProviders/dnscontroltest-r53.com/Clean_Slate:Empty#52 (0.07s)
        --- PASS: TestDNSProviders/dnscontroltest-r53.com/84:final:final (0.71s)
=== RUN   TestDualProviders
Testing Profile="ROUTE53" TYPE="ROUTE53"
    integration_test.go:414: Clearing everything
    integration_test.go:408: #1:
        - DELETE final.dnscontroltest-r53.com TXT "TestDNSProviders was successful!" ttl=300
    integration_test.go:421: Adding test nameservers
    integration_test.go:408: #1:
        + CREATE dnscontroltest-r53.com NS ns1.example.com. ttl=300
        + CREATE dnscontroltest-r53.com NS ns2.example.com. ttl=300
    integration_test.go:424: Running again to ensure stability
    integration_test.go:440: Removing test nameservers
    integration_test.go:408: #1:
        - DELETE dnscontroltest-r53.com NS ns1.example.com. ttl=300
        - DELETE dnscontroltest-r53.com NS ns2.example.com. ttl=300
--- PASS: TestDualProviders (0.88s)
=== RUN   TestNameserverDots
Testing Profile="ROUTE53" TYPE="ROUTE53"
=== RUN   TestNameserverDots/No_trailing_dot_in_nameserver
--- PASS: TestNameserverDots (0.12s)
    --- PASS: TestNameserverDots/No_trailing_dot_in_nameserver (0.00s)
PASS
ok  	github.com/StackExchange/dnscontrol/v4/integrationTest	158.090s


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants