dnscontrol icon indicating copy to clipboard operation
dnscontrol copied to clipboard

ROUTE53: make caching of zones thread-safe

Open das7pad opened this issue 11 months ago • 2 comments

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 https://github.com/StackExchange/dnscontrol/issues/3007

das7pad avatar Jan 08 '25 20:01 das7pad

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


tlimoncelli avatar Jan 10 '25 20:01 tlimoncelli

@tresni Can you try running this branch on a dnsconfig.js with a lot of ROUTE53 domains? The concurrency should work a lot better now.

tlimoncelli avatar May 13 '25 19:05 tlimoncelli

Friendly reminder to rebase so that this can be merged. Thanks!

tlimoncelli avatar Nov 03 '25 18:11 tlimoncelli

Thanks!

tlimoncelli avatar Dec 03 '25 13:12 tlimoncelli