Merge #1552: Refactor UDP error event logic before adding new metrics

a8f3a97 refactor: [#1551] extract event handler for each udp event (Jose Celano)
89ac87c refactor: [#1551] extract methods in udp event handler" (Jose Celano)
07c6e89 refactor: rename UDP tracker server error variants (Jose Celano)
21bea5b refactor: [#1456] increase ban counters asyncronously (Jose Celano)
ad1b19a feat: trigger UDP error event when there is no transaction ID too (Jose Celano)
525ab73 refactor: [#1456] extract methods (Jose Celano)
f485501 refactor: [#1456 clean code (Jose Celano)
0108c26 fix: test. Error message changed (Jose Celano)
d7902f1 refactor: [#1456] remove unused enum variant in udp server error (Jose Celano)
8f3c22a feat: [#1456] expose error kind in the UdpError event (Jose Celano)
52b9660 feat: [#1456] wrapper over aquatic RequestParseError to make it sendable (Jose Celano)

Pull request description:

  I will add a new metric to count which client's software is used when a UDP error is produced (only for connection ID errors, for now).

  This PR make some changes before adding the new metric.

  ### Subtasks

  - [x] Wrapper for aquatic parse error. It's not sendable.
  - [x] Add the error to the `Event::UdpError` event.
  - [x] Move logic to increase the number of wrong connection IDs per IP to the event listener.
  - [x] Rename errors.
  - [x] Refactor event handler.

ACKs for top commit:
  josecelano:
    ACK a8f3a97

Tree-SHA512: faa185a8050ef9e45f317ef06aa74e52bb385c31167fa0f199411bb1a47a573429daa31b2cccdd024f8fd75c91227618350d10e41d12e8b4062fb1e8d7f7bfdc

Author: josecelano

Cargo.lock

@@ -84,7 +84,7 @@ pub enum ScrapeError {
 ///
 /// This error is returned when an operation involves a torrent that is not
 /// present in the whitelist.
-#[derive(thiserror::Error, Debug, Clone)]
+#[derive(thiserror::Error, Debug, Clone, PartialEq, Eq)]
 pub enum WhitelistError {
     /// Indicates that the torrent identified by `info_hash` is not whitelisted.
     #[error("The torrent: {info_hash}, is not whitelisted, {location}")]

packages/tracker-core/src/error.rs

@@ -86,7 +86,7 @@ use zerocopy::AsBytes;
 use crate::crypto::keys::CipherArrayBlowfish;
 
 /// Error returned when there was an error with the connection cookie.
-#[derive(Error, Debug, Clone)]
+#[derive(Error, Debug, Clone, PartialEq)]
 pub enum ConnectionCookieError {
     #[error("cookie value is not normal: {not_normal_value}")]
     ValueNotNormal { not_normal_value: f64 },

packages/udp-tracker-core/src/connection_cookie.rs

@@ -30,7 +30,6 @@ torrust-server-lib = { version = "3.0.0-develop", path = "../server-lib" }
 torrust-tracker-clock = { version = "3.0.0-develop", path = "../clock" }
 torrust-tracker-configuration = { version = "3.0.0-develop", path = "../configuration" }
 torrust-tracker-events = { version = "3.0.0-develop", path = "../events" }
-torrust-tracker-located-error = { version = "3.0.0-develop", path = "../located-error" }
 torrust-tracker-metrics = { version = "3.0.0-develop", path = "../metrics" }
 torrust-tracker-primitives = { version = "3.0.0-develop", path = "../primitives" }
 torrust-tracker-swarm-coordination-registry = { version = "3.0.0-develop", path = "../swarm-coordination-registry" }

packages/udp-tracker-server/Cargo.toml

@@ -82,6 +82,7 @@ impl Environment<Stopped> {
         let udp_server_event_listener_job = Some(crate::statistics::event::listener::run_event_listener(
             self.container.udp_tracker_server_container.event_bus.receiver(),
             &self.container.udp_tracker_server_container.stats_repository,
+            &self.container.udp_tracker_core_container.ban_service,
         ));
 
         // Start the UDP tracker server

packages/udp-tracker-server/src/environment.rs

@@ -1,68 +1,100 @@
 //! Error types for the UDP server.
+use std::fmt::Display;
 use std::panic::Location;
 
-use aquatic_udp_protocol::{ConnectionId, RequestParseError};
+use aquatic_udp_protocol::{ConnectionId, RequestParseError, TransactionId};
 use bittorrent_udp_tracker_core::services::announce::UdpAnnounceError;
 use bittorrent_udp_tracker_core::services::scrape::UdpScrapeError;
 use derive_more::derive::Display;
 use thiserror::Error;
-use torrust_tracker_located_error::LocatedError;
 
 #[derive(Display, Debug)]
 #[display(":?")]
 pub struct ConnectionCookie(pub ConnectionId);
 
 /// Error returned by the UDP server.
-#[derive(Error, Debug)]
+#[derive(Error, Debug, Clone)]
 pub enum Error {
     /// Error returned when the request is invalid.
-    #[error("error when phrasing request: {request_parse_error:?}")]
-    RequestParseError { request_parse_error: RequestParseError },
+    #[error("error parsing request: {request_parse_error:?}")]
+    InvalidRequest { request_parse_error: SendableRequestParseError },
 
     /// Error returned when the domain tracker returns an announce error.
     #[error("tracker announce error: {source}")]
-    UdpAnnounceError { source: UdpAnnounceError },
+    AnnounceFailed { source: UdpAnnounceError },
 
     /// Error returned when the domain tracker returns an scrape error.
     #[error("tracker scrape error: {source}")]
-    UdpScrapeError { source: UdpScrapeError },
+    ScrapeFailed { source: UdpScrapeError },
 
     /// Error returned from a third-party library (`aquatic_udp_protocol`).
     #[error("internal server error: {message}, {location}")]
-    InternalServer {
+    Internal {
         location: &'static Location<'static>,
         message: String,
     },
 
-    /// Error returned when the request is invalid.
-    #[error("bad request: {source}")]
-    BadRequest {
-        source: LocatedError<'static, dyn std::error::Error + Send + Sync>,
-    },
-
     /// Error returned when tracker requires authentication.
     #[error("domain tracker requires authentication but is not supported in current UDP implementation. Location: {location}")]
-    TrackerAuthenticationRequired { location: &'static Location<'static> },
+    AuthRequired { location: &'static Location<'static> },
 }
 
 impl From<RequestParseError> for Error {
     fn from(request_parse_error: RequestParseError) -> Self {
-        Self::RequestParseError { request_parse_error }
+        Self::InvalidRequest {
+            request_parse_error: request_parse_error.into(),
+        }
     }
 }
 
 impl From<UdpAnnounceError> for Error {
     fn from(udp_announce_error: UdpAnnounceError) -> Self {
-        Self::UdpAnnounceError {
+        Self::AnnounceFailed {
             source: udp_announce_error,
         }
     }
 }
 
 impl From<UdpScrapeError> for Error {
     fn from(udp_scrape_error: UdpScrapeError) -> Self {
-        Self::UdpScrapeError {
+        Self::ScrapeFailed {
             source: udp_scrape_error,
         }
     }
 }
+
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub struct SendableRequestParseError {
+    pub message: String,
+    pub opt_connection_id: Option<ConnectionId>,
+    pub opt_transaction_id: Option<TransactionId>,
+}
+
+impl Display for SendableRequestParseError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "SendableRequestParseError: message: {}, connection_id: {:?}, transaction_id: {:?}",
+            self.message, self.opt_connection_id, self.opt_transaction_id
+        )
+    }
+}
+
+impl From<RequestParseError> for SendableRequestParseError {
+    fn from(request_parse_error: RequestParseError) -> Self {
+        let (message, opt_connection_id, opt_transaction_id) = match request_parse_error {
+            RequestParseError::Sendable {
+                connection_id,
+                transaction_id,
+                err,
+            } => ((*err).to_string(), Some(connection_id), Some(transaction_id)),
+            RequestParseError::Unsendable { err } => (err.to_string(), None, None),
+        };
+
+        Self {
+            message,
+            opt_connection_id,
+            opt_transaction_id,
+        }
+    }
+}

packages/udp-tracker-server/src/error.rs

@@ -2,12 +2,17 @@ use std::fmt;
 use std::net::SocketAddr;
 use std::time::Duration;
 
+use bittorrent_tracker_core::error::{AnnounceError, ScrapeError};
+use bittorrent_udp_tracker_core::services::announce::UdpAnnounceError;
+use bittorrent_udp_tracker_core::services::scrape::UdpScrapeError;
 use torrust_tracker_metrics::label::{LabelSet, LabelValue};
 use torrust_tracker_metrics::label_name;
 use torrust_tracker_primitives::service_binding::ServiceBinding;
 
+use crate::error::Error;
+
 /// A UDP server event.
-#[derive(Debug, PartialEq, Eq, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 pub enum Event {
     UdpRequestReceived {
         context: ConnectionContext,
@@ -30,6 +35,7 @@ pub enum Event {
     UdpError {
         context: ConnectionContext,
         kind: Option<UdpRequestKind>,
+        error: ErrorKind,
     },
 }
 
@@ -109,6 +115,42 @@ impl From<ConnectionContext> for LabelSet {
     }
 }
 
+#[derive(Debug, Clone, PartialEq)]
+pub enum ErrorKind {
+    RequestParse(String),
+    ConnectionCookie(String),
+    Whitelist(String),
+    Database(String),
+    InternalServer(String),
+    BadRequest(String),
+    TrackerAuthentication(String),
+}
+
+impl From<Error> for ErrorKind {
+    fn from(error: Error) -> Self {
+        match error {
+            Error::InvalidRequest { request_parse_error } => Self::RequestParse(request_parse_error.to_string()),
+            Error::AnnounceFailed { source } => match source {
+                UdpAnnounceError::ConnectionCookieError { source } => Self::ConnectionCookie(source.to_string()),
+                UdpAnnounceError::TrackerCoreAnnounceError { source } => match source {
+                    AnnounceError::Whitelist(whitelist_error) => Self::Whitelist(whitelist_error.to_string()),
+                    AnnounceError::Database(error) => Self::Database(error.to_string()),
+                },
+                UdpAnnounceError::TrackerCoreWhitelistError { source } => Self::Whitelist(source.to_string()),
+            },
+            Error::ScrapeFailed { source } => match source {
+                UdpScrapeError::ConnectionCookieError { source } => Self::ConnectionCookie(source.to_string()),
+                UdpScrapeError::TrackerCoreScrapeError { source } => match source {
+                    ScrapeError::Whitelist(whitelist_error) => Self::Whitelist(whitelist_error.to_string()),
+                },
+                UdpScrapeError::TrackerCoreWhitelistError { source } => Self::Whitelist(source.to_string()),
+            },
+            Error::Internal { location: _, message } => Self::InternalServer(message.to_string()),
+            Error::AuthRequired { location } => Self::TrackerAuthentication(location.to_string()),
+        }
+    }
+}
+
 pub mod sender {
     use std::sync::Arc;
 

packages/udp-tracker-server/src/event.rs

@@ -2,8 +2,7 @@
 use std::net::SocketAddr;
 use std::ops::Range;
 
-use aquatic_udp_protocol::{ErrorResponse, RequestParseError, Response, TransactionId};
-use bittorrent_udp_tracker_core::connection_cookie::{check, gen_remote_fingerprint};
+use aquatic_udp_protocol::{ErrorResponse, Response, TransactionId};
 use bittorrent_udp_tracker_core::{self, UDP_TRACKER_LOG_TARGET};
 use torrust_tracker_primitives::service_binding::ServiceBinding;
 use tracing::{instrument, Level};
@@ -22,55 +21,62 @@ pub async fn handle_error(
     request_id: Uuid,
     opt_udp_server_stats_event_sender: &crate::event::sender::Sender,
     cookie_valid_range: Range<f64>,
-    e: &Error,
-    transaction_id: Option<TransactionId>,
+    error: &Error,
+    opt_transaction_id: Option<TransactionId>,
 ) -> Response {
     tracing::trace!("handle error");
 
     let server_socket_addr = server_service_binding.bind_address();
 
-    match transaction_id {
+    log_error(error, client_socket_addr, server_socket_addr, opt_transaction_id, request_id);
+
+    trigger_udp_error_event(
+        error,
+        client_socket_addr,
+        server_service_binding,
+        opt_udp_server_stats_event_sender,
+        req_kind,
+    )
+    .await;
+
+    Response::from(ErrorResponse {
+        transaction_id: opt_transaction_id.unwrap_or(TransactionId(I32::new(0))),
+        message: error.to_string().into(),
+    })
+}
+
+fn log_error(
+    error: &Error,
+    client_socket_addr: SocketAddr,
+    server_socket_addr: SocketAddr,
+    opt_transaction_id: Option<TransactionId>,
+    request_id: Uuid,
+) {
+    match opt_transaction_id {
         Some(transaction_id) => {
             let transaction_id = transaction_id.0.to_string();
-            tracing::error!(target: UDP_TRACKER_LOG_TARGET, error = %e, %client_socket_addr, %server_socket_addr, %request_id, %transaction_id, "response error");
+            tracing::error!(target: UDP_TRACKER_LOG_TARGET, error = %error, %client_socket_addr, %server_socket_addr, %request_id, %transaction_id, "response error");
         }
         None => {
-            tracing::error!(target: UDP_TRACKER_LOG_TARGET, error = %e, %client_socket_addr, %server_socket_addr, %request_id, "response error");
+            tracing::error!(target: UDP_TRACKER_LOG_TARGET, error = %error, %client_socket_addr, %server_socket_addr, %request_id, "response error");
         }
     }
+}
 
-    let e = if let Error::RequestParseError { request_parse_error } = e {
-        match request_parse_error {
-            RequestParseError::Sendable {
-                connection_id,
-                transaction_id,
-                err,
-            } => {
-                if let Err(e) = check(connection_id, gen_remote_fingerprint(&client_socket_addr), cookie_valid_range) {
-                    (e.to_string(), Some(*transaction_id))
-                } else {
-                    ((*err).to_string(), Some(*transaction_id))
-                }
-            }
-            RequestParseError::Unsendable { err } => (err.to_string(), transaction_id),
-        }
-    } else {
-        (e.to_string(), transaction_id)
-    };
-
-    if e.1.is_some() {
-        if let Some(udp_server_stats_event_sender) = opt_udp_server_stats_event_sender.as_deref() {
-            udp_server_stats_event_sender
-                .send(Event::UdpError {
-                    context: ConnectionContext::new(client_socket_addr, server_service_binding),
-                    kind: req_kind,
-                })
-                .await;
-        }
+async fn trigger_udp_error_event(
+    error: &Error,
+    client_socket_addr: SocketAddr,
+    server_service_binding: ServiceBinding,
+    opt_udp_server_stats_event_sender: &crate::event::sender::Sender,
+    req_kind: Option<UdpRequestKind>,
+) {
+    if let Some(udp_server_stats_event_sender) = opt_udp_server_stats_event_sender.as_deref() {
+        udp_server_stats_event_sender
+            .send(Event::UdpError {
+                context: ConnectionContext::new(client_socket_addr, server_service_binding),
+                kind: req_kind,
+                error: error.clone().into(),
+            })
+            .await;
     }
-
-    Response::from(ErrorResponse {
-        transaction_id: e.1.unwrap_or(TransactionId(I32::new(0))),
-        message: e.0.into(),
-    })
 }

packages/udp-tracker-server/src/handlers/error.rs

@@ -13,7 +13,6 @@ use announce::handle_announce;
 use aquatic_udp_protocol::{Request, Response, TransactionId};
 use bittorrent_tracker_core::MAX_SCRAPE_TORRENTS;
 use bittorrent_udp_tracker_core::container::UdpTrackerCoreContainer;
-use bittorrent_udp_tracker_core::services::announce::UdpAnnounceError;
 use connect::handle_connect;
 use error::handle_error;
 use scrape::handle_scrape;
@@ -84,15 +83,6 @@ pub(crate) async fn handle_packet(
             {
                 Ok((response, req_kid)) => return (response, Some(req_kid)),
                 Err((error, transaction_id, req_kind)) => {
-                    if let Error::UdpAnnounceError {
-                        source: UdpAnnounceError::ConnectionCookieError { .. },
-                    } = error
-                    {
-                        // code-review: should we include `RequestParseError` and `BadRequest`?
-                        let mut ban_service = udp_tracker_core_container.ban_service.write().await;
-                        ban_service.increase_counter(&udp_request.from.ip());
-                    }
-
                     let response = handle_error(
                         Some(req_kind.clone()),
                         udp_request.from,
@@ -109,6 +99,14 @@ pub(crate) async fn handle_packet(
                 }
             },
             Err(e) => {
+                // The request payload could not be parsed, so we handle it as an error.
+
+                let opt_transaction_id = if let Error::InvalidRequest { request_parse_error } = e.clone() {
+                    request_parse_error.opt_transaction_id
+                } else {
+                    None
+                };
+
                 let response = handle_error(
                     None,
                     udp_request.from,
@@ -117,7 +115,7 @@ pub(crate) async fn handle_packet(
                     &udp_tracker_server_container.stats_event_sender,
                     cookie_time_values.valid_range.clone(),
                     &e,
-                    None,
+                    opt_transaction_id,
                 )
                 .await;