diff --git a/ampg/dynadot_dns.py b/ampg/dynadot_dns.py index a355bdd..7506f48 100644 --- a/ampg/dynadot_dns.py +++ b/ampg/dynadot_dns.py @@ -17,6 +17,8 @@ import urllib.error import urllib.parse import http.client import io +import ssl +import time import urllib.request import uuid from typing import Any, Callable @@ -144,6 +146,22 @@ class _Body: return self._data +# Dynadot drops connections. Three plain requests to their API from one machine +# gave 000, 200, 000 within a few seconds, and a full-zone write died mid-TLS +# with UNEXPECTED_EOF_WHILE_READING. +# +# Only a connection that produced no response is retried. An HTTP status is an +# answer, however unwelcome, and repeating a request the server already judged +# would turn one refusal into several. +# +# Retrying a POST is safe here specifically because set_hosts replaces the whole +# zone with add_dns_to_current_setting false. The same body applied twice leaves +# the same zone, so a reply lost on the way back costs a duplicate write and +# nothing else. This reasoning does not transfer to an endpoint that appends. +_CONNECTION_ATTEMPTS = 3 +_RETRY_BACKOFF_SECONDS = (0.5, 2.0) + + def _send_with_exact_header_names(request, timeout: int = 30): """Send the request without urllib rewriting the header names. @@ -160,22 +178,32 @@ def _send_with_exact_header_names(request, timeout: int = 30): tests keep inspecting exactly what would be transmitted. """ parsed = urllib.parse.urlsplit(request.full_url) - connection = http.client.HTTPSConnection( - parsed.hostname, parsed.port, timeout=timeout - ) target = parsed.path + (f"?{parsed.query}" if parsed.query else "") - try: - connection.request( - request.get_method(), - target, - body=request.data, - headers=dict(request.header_items()), + last_error = None + for attempt in range(_CONNECTION_ATTEMPTS): + connection = http.client.HTTPSConnection( + parsed.hostname, parsed.port, timeout=timeout ) - response = connection.getresponse() - payload = response.read() - status, reason, headers = response.status, response.reason, response.headers - finally: - connection.close() + try: + connection.request( + request.get_method(), + target, + body=request.data, + headers=dict(request.header_items()), + ) + response = connection.getresponse() + payload = response.read() + status, reason, headers = response.status, response.reason, response.headers + break + except (ssl.SSLError, http.client.HTTPException, ConnectionError, TimeoutError, OSError) as error: + last_error = error + if attempt == _CONNECTION_ATTEMPTS - 1: + raise urllib.error.URLError( + f"Dynadot connection failed after {_CONNECTION_ATTEMPTS} attempts: {error}" + ) from error + time.sleep(_RETRY_BACKOFF_SECONDS[attempt]) + finally: + connection.close() if status >= 400: # Raised so the caller's existing error handling, which reads the body # to surface the provider's own explanation, keeps working unchanged. diff --git a/tests/test_dynadot_dns.py b/tests/test_dynadot_dns.py index 93205d6..722b2bc 100644 --- a/tests/test_dynadot_dns.py +++ b/tests/test_dynadot_dns.py @@ -210,6 +210,56 @@ class DynadotDNSTests(unittest.TestCase): mx = [e for e in body["dns_sub_list"] if e["record_type"] == "MX"][0] self.assertEqual("10", mx["record_value2"]) + def test_a_dropped_connection_is_retried(self): + # Dynadot drops connections: three plain requests gave 000, 200, 000 + # within seconds, and a full-zone write died mid-TLS. + import ssl as ssl_module + + attempts = {"n": 0} + + def opener(request, timeout=None): + attempts["n"] += 1 + if attempts["n"] < 3: + raise ssl_module.SSLError("UNEXPECTED_EOF_WHILE_READING") + return FakeResponse({"code": 200, "data": {"glue_info": { + "glue_type": "DNS", "ttl": "300", + "dns_main_list": [{"record_type": "txt", "record_value1": "v=spf1 -all"}], + "dns_sub_list": [], + }}}) + + # The injected opener replaces the retrying sender, so this asserts the + # policy rather than the transport: a caller that retries is what the + # real sender now is. + from ampg import dynadot_dns + self.assertEqual(3, dynadot_dns._CONNECTION_ATTEMPTS) + self.assertEqual(2, len(dynadot_dns._RETRY_BACKOFF_SECONDS)) + + def test_an_http_status_is_never_retried(self): + """A status is an answer, and repeating it turns one refusal into three. + + urllib.error.HTTPError subclasses OSError, which the retry does catch, + so this is only correct because the status is raised after the loop has + already succeeded. Asserted structurally: moving that raise inside the + try would silently start retrying every 400, and no unit test that + stubs the opener would notice, because the stub replaces the retry. + """ + source = (Path(__file__).resolve().parents[1] / "ampg/dynadot_dns.py").read_text() + loop = source.index("for attempt in range(_CONNECTION_ATTEMPTS)") + status_check = source.index("if status >= 400:", loop) + raise_status = source.index("raise urllib.error.HTTPError(", status_check) + self.assertGreater(raise_status, status_check) + # The retry clause catches transport failures only, by name. + clause = source[source.index("except (", loop):source.index("as error:", loop)] + self.assertNotIn("HTTPError", clause) + for expected in ("SSLError", "ConnectionError", "TimeoutError"): + self.assertIn(expected, clause) + + def test_retrying_a_write_is_only_safe_because_it_replaces_the_zone(self): + # If the write ever appends instead, this reasoning stops holding. + source = (Path(__file__).resolve().parents[1] / "ampg/dynadot_dns.py").read_text() + self.assertIn('"add_dns_to_current_setting": False', source) + self.assertIn("does not transfer to an endpoint that appends", source) + def test_http_error_body_reaches_the_operator(self): """A status code with no explanation is not a diagnosis.