Refactor and further improve error handling / logging

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"Cleanup: Host expired: hostname={hostname.hostname} zone={hostname.zone} "
-                    f"ip={hostname.last_ipv4}"
+                    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"Cleanup failed: hostname={hostname.hostname} "
-                        f"zone={hostname.zone} type=A error={e}"
+                        f"zone={hostname.zone} type=A"
                     )
 
             if ipv6_expired:
                 logging.info(
-                    f"Host expired: hostname={hostname.hostname} zone={hostname.zone} "
+                    f"Cleanup: Host expired: hostname={hostname.hostname} zone={hostname.zone} "
-                    f"ip={hostname.last_ipv6}"
+                    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"Cleanup failed: hostname={hostname.hostname} "
-                        f"zone={hostname.zone} type=AAAA error={e}"
+                        f"zone={hostname.zone} type=AAAA"
                     )
 
         if not (ipv4_deleted or ipv6_deleted):

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.kwargs = kwargs
-        self.username = username
-        self.hostname = hostname
 
     def __str__(self):
-        string = f"{self.message}: client={self.client_ip}"
+        if not self.kwargs:
-        if self.username:
+            return self.message
-            string += f" username={self.username}"
+
-        if self.hostname:
+        string = f"{self.message}:"
-            string += f" hostname={self.hostname}"
+        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,
+                client=client_ip,
-                username
+                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(
+                raise DDNSClientError(
-                    f"Rate limited: client={client_ip}, "
+                    "Rate limited (good requests)",
-                    f"retry_at={datetime_str(retry_at)}")
+                    429,
-                self.respond(429, STATUS_ABUSE)
+                    STATUS_ABUSE,
-                return
+                    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(
+        self._process_ip_update(
-            hostname,
             client_ip,
-            myip,
+            user,
-            myip6,
+            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,
+                client=client_ip,
-                username
+                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,
+                client=client_ip,
-                user.username,
+                username=user.username,
-                hostname_param
+                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(
+                    logging.error(f"DNS error: {e}")
-                        f"DNS update failed: client={client_ip} hostname={hostname.hostname} "
+                    raise DDNSError(
-                        f"zone={hostname.zone} ipv4={ipv4} error={e}"
+                        "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(
+                    logging.error(f"DNS error: {e}")
-                        f"DNS update failed: client={client_ip} hostname={hostname.hostname} "
+                    raise DDNSError(
-                        f"zone={hostname.zone} ipv6={ipv6} error={e}"
+                        "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}
         )