From cd5979556c4566562e6e57f268841d93c5704318 Mon Sep 17 00:00:00 2001 From: Thomas Oettli Date: Sun, 1 Feb 2026 04:25:21 +0100 Subject: [PATCH] Refactor and further improve error handling / logging --- src/ddns_service/cleanup.py | 18 +-- src/ddns_service/server.py | 233 +++++++++++++++++++++--------------- 2 files changed, 144 insertions(+), 107 deletions(-) diff --git a/src/ddns_service/cleanup.py b/src/ddns_service/cleanup.py index 412bf52..c1177d5 100644 --- a/src/ddns_service/cleanup.py +++ b/src/ddns_service/cleanup.py @@ -48,30 +48,32 @@ def cleanup_expired(app): if app.dns_service: if ipv4_expired: logging.info( - f"Host expired: hostname={hostname.hostname} zone={hostname.zone} " - f"ip={hostname.last_ipv4}" + f"Cleanup: Host expired: hostname={hostname.hostname} zone={hostname.zone} " + f"ipv4={hostname.last_ipv4}" ) try: app.dns_service.delete_record(hostname.hostname, hostname.zone, "A") ipv4_deleted = True except Exception as e: + logging.error(f"DNS error: {e}") logging.error( - f"DNS delete failed: hostname={hostname.hostname} " - f"zone={hostname.zone} type=A error={e}" + f"Cleanup failed: hostname={hostname.hostname} " + f"zone={hostname.zone} type=A" ) if ipv6_expired: logging.info( - f"Host expired: hostname={hostname.hostname} zone={hostname.zone} " - f"ip={hostname.last_ipv6}" + f"Cleanup: Host expired: hostname={hostname.hostname} zone={hostname.zone} " + f"ipv6={hostname.last_ipv6}" ) try: app.dns_service.delete_record(hostname.hostname, hostname.zone, "AAAA") ipv6_deleted = True except Exception as e: + logging.error(f"DNS error: {e}") logging.error( - f"DNS delete failed: hostname={hostname.hostname} " - f"zone={hostname.zone} type=AAAA error={e}" + f"Cleanup failed: hostname={hostname.hostname} " + f"zone={hostname.zone} type=AAAA" ) if not (ipv4_deleted or ipv6_deleted): diff --git a/src/ddns_service/server.py b/src/ddns_service/server.py index be9ddce..4d050f7 100644 --- a/src/ddns_service/server.py +++ b/src/ddns_service/server.py @@ -139,24 +139,29 @@ class DDNSServer(ThreadingHTTPServer): class DDNSError(Exception): - def __init__(self, message, code, status, client_ip, username=None, hostname=None): + def __init__(self, message, status, **kwargs): super().__init__(self, message) self.message = message - self.code = code self.status = status - self.client_ip = client_ip - self.username = username - self.hostname = hostname + self.kwargs = kwargs def __str__(self): - string = f"{self.message}: client={self.client_ip}" - if self.username: - string += f" username={self.username}" - if self.hostname: - string += f" hostname={self.hostname}" + if not self.kwargs: + return self.message + + string = f"{self.message}:" + for key, value in self.kwargs.items(): + string += f" {key}={value}" + return string +class DDNSClientError(DDNSError): + def __init__(self, message, code, status, **kwargs): + super().__init__(message, status, **kwargs) + self.code = code + + class DDNSRequestHandler(BaseHTTPRequestHandler): """HTTP request handler for DDNS updates.""" @@ -243,23 +248,16 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): self.respond(400, "Bad Request") return - # Bad rate limit check - if self.app.bad_limiter: - blocked, retry_at = self.app.bad_limiter.is_blocked(client_ip) - if blocked: - logging.warning( - f"Rate limited (bad): client={client_ip}, " - f"retry_at={datetime_str(retry_at)}") - self.respond(429, STATUS_ABUSE) - return - try: self._handle_get_request(client_ip) - except DDNSError as e: + except DDNSClientError as e: if self.app.bad_limiter: self.app.bad_limiter.record(client_ip) logging.warning(e) self.respond(e.code, e.status) + except DDNSError as e: + logging.error(e) + self.respond(500, e.status) except DatabaseError as e: logging.error(f"Database error: {e}") self.respond(500, "Internal Server Error") @@ -280,6 +278,18 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): self.respond(404, "Not Found") return + # Bad rate limit check + if self.app.bad_limiter: + blocked, retry_at = self.app.bad_limiter.is_blocked(client_ip) + if blocked: + raise DDNSClientError( + "Rate limited (bad requests)", + 429, + STATUS_ABUSE, + client=client_ip, + retry_at=datetime_str(retry_at) + ) + # Parse query parameters params = parse_qs(parsed.query) @@ -290,7 +300,7 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): password = extract_param(params, endpoint["params"]["password"]) if not username or not password: - raise DDNSError( + raise DDNSClientError( "Auth failed", 401, STATUS_BADAUTH, @@ -300,48 +310,103 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): # Get hostname parameter hostname_param = extract_param(params, endpoint["params"]["hostname"]) if not hostname_param: - raise DDNSError( + raise DDNSClientError( "Missing hostname", 400, STATUS_NOHOST, - client_ip, - username + client=client_ip, + username=username ) # Validate credentials user = self._authenticate(client_ip, username, password) # Check hostname ownership - hostname = self._check_permissions(client_ip, hostname_param, user) + hostname = self._check_permissions(client_ip, user, hostname_param) + + # Process myip parameter + ipv4 = None + myip = extract_param(params, endpoint["params"]["ipv4"]) + if myip: + try: + rtype, myip = detect_ip_type(myip) + if rtype == "A": + ipv4 = myip + else: + ipv6 = myip + except ValueError: + raise DDNSClientError( + "Bad IP address", + 400, + STATUS_BADIP, + client=client_ip, + username=username, + hostname=hostname.hostname, + zone=hostname.zone, + ip=myip + ) + + # Process myip6 parameter + ipv6 = None + myip6 = extract_param(params, endpoint["params"]["ipv6"]) + if myip6: + try: + rtype, myip6 = detect_ip_type(myip6) + if rtype == "AAAA": + ipv6 = myip6 + else: + raise ValueError + except ValueError: + raise DDNSClientError( + "Bad IP address", + 400, + STATUS_BADIP, + client=client_ip, + username=username, + hostname=hostname.hostname, + zone=hostname.zone, + ipv6=myip6 + ) + + # Auto-detect from client IP if no params + if ipv4 is None and ipv6 is None: + rtype, ip = detect_ip_type(client_ip) + if rtype == "A": + ipv4 = ip + else: + ipv6 = ip + + # Process notify_change parameter + notify_change = extract_param(params, endpoint["params"]["notify_change"]) + notify_change = notify_change.lower() in ["1", "y", "yes", "on", "true"] \ + if notify_change else False # Good rate limit check if self.app.good_limiter: blocked, retry_at = self.app.good_limiter.is_blocked(client_ip) if blocked: - logging.warning( - f"Rate limited: client={client_ip}, " - f"retry_at={datetime_str(retry_at)}") - self.respond(429, STATUS_ABUSE) - return + raise DDNSClientError( + "Rate limited (good requests)", + 429, + STATUS_ABUSE, + client=client_ip, + username=username, + retry_at=datetime_str(retry_at) + ) # Record good request if self.app.good_limiter: self.app.good_limiter.record(client_ip) - # get myip, myip6 and notify_change parameters - myip = extract_param(params, endpoint["params"]["ipv4"]) - myip6 = extract_param(params, endpoint["params"]["ipv6"]) - notify_change = extract_param(params, endpoint["params"]["notify_change"]) - # Process update request - code, status, kwargs = self._process_ip_update( - hostname, + self._process_ip_update( client_ip, - myip, - myip6, + user, + hostname, + ipv4, + ipv6, notify_change ) - self.respond(code, status, **kwargs) def _authenticate(self, client_ip, username, password): try: @@ -354,17 +419,17 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): self.app.password_hasher.verify(user.password_hash, password) except (DoesNotExist, VerifyMismatchError): - raise DDNSError( + raise DDNSClientError( "Auth failed", 401, STATUS_BADAUTH, - client_ip, - username + client=client_ip, + username=username ) return user - def _check_permissions(self, client_ip, hostname_param, user): + def _check_permissions(self, client_ip, user, hostname_param): # Check hostname ownership code = None @@ -376,55 +441,19 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): code = 400 if code: - raise DDNSError( + raise DDNSClientError( "Access denied", code, STATUS_NOHOST, - client_ip, - user.username, - hostname_param + client=client_ip, + username=user.username, + hostname=hostname_param ) return hostname - def _process_ip_update(self, hostname, client_ip, myip, myip6, notify_change): + def _process_ip_update(self, client_ip, user, hostname, ipv4, ipv6, notify_change): """Process IP update for hostname.""" - ipv4 = None - ipv6 = None - - # Process myip parameter - if myip: - try: - rtype, myip = detect_ip_type(myip) - if rtype == "A": - ipv4 = myip - else: - ipv6 = myip - except ValueError: - return (400, STATUS_BADIP, {}) - - # Process myip6 parameter - if myip6: - try: - rtype, myip6 = detect_ip_type(myip6) - if rtype == "AAAA": - ipv6 = myip6 - else: - return (400, STATUS_BADIP, {}) - except ValueError: - return (400, STATUS_BADIP, {}) - - # Auto-detect from client IP if no params - if ipv4 is None and ipv6 is None: - try: - rtype, ip = detect_ip_type(client_ip) - if rtype == "A": - ipv4 = ip - else: - ipv6 = ip - except ValueError: - return (400, STATUS_BADIP, {}) - now = now_utc() ipv4_changed = False @@ -444,11 +473,15 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): ipv4_changed = True except Exception as e: hostname.save() - logging.error( - f"DNS update failed: client={client_ip} hostname={hostname.hostname} " - f"zone={hostname.zone} ipv4={ipv4} error={e}" + logging.error(f"DNS error: {e}") + raise DDNSError( + "Update failed", + STATUS_DNSERR, + client=client_ip, + hostname=hostname.hostname, + zone=hostname.zone, + ipv4=ipv4 ) - return (500, STATUS_DNSERR, {}) if ipv6: hostname.last_ipv6_update = now @@ -465,11 +498,15 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): ipv6_changed = True except Exception as e: hostname.save() - logging.error( - f"DNS update failed: client={client_ip} hostname={hostname.hostname} " - f"zone={hostname.zone} ipv6={ipv6} error={e}" + logging.error(f"DNS error: {e}") + raise DDNSError( + "Update failed", + STATUS_DNSERR, + client=client_ip, + hostname=hostname.hostname, + zone=hostname.zone, + ipv6=ipv6 ) - return (500, STATUS_DNSERR, {}) # Update database hostname.save() @@ -480,18 +517,16 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): if ipv6_changed: changed_addrs += f" ipv6={ipv6}" - notify_change = notify_change.lower() in ["1", "y", "yes", "on", "true"] \ - if notify_change else False - if not ipv4_changed and not ipv6_changed: logging.info( f"No change: client={client_ip} hostname={hostname.hostname} " f"zone={hostname.zone}{changed_addrs} notify_change={str(notify_change).lower()}" ) - return ( + self.respond( 200, STATUS_NOCHG, {"ipv4": hostname.last_ipv4, "ipv6": hostname.last_ipv6} ) + return logging.info( f"Updated: client={client_ip} hostname={hostname.hostname} " @@ -509,7 +544,7 @@ class DDNSRequestHandler(BaseHTTPRequestHandler): except Exception as e: logging.error(f"Sending change notification error: {e}") - return ( + self.respond( 200, STATUS_GOOD, {"ipv4": hostname.last_ipv4, "ipv6": hostname.last_ipv6} )