Merge pull request #792 from mozilla-services/feat/786

feat: emit internal bb8 Pool errors to logs/sentry

src/db/spanner/pool.rs

@@ -1,5 +1,5 @@
 use async_trait::async_trait;
-use bb8::Pool;
+use bb8::{ErrorSink, Pool};
 
 use std::{
     collections::HashMap,
@@ -49,7 +49,8 @@ impl SpannerDbPool {
         let max_size = settings.database_pool_max_size.unwrap_or(10);
         let builder = bb8::Pool::builder()
             .max_size(max_size)
-            .min_idle(settings.database_pool_min_idle);
+            .min_idle(settings.database_pool_min_idle)
+            .error_sink(Box::new(LoggingErrorSink));
 
         Ok(Self {
             pool: builder.build(manager).await?,
@@ -164,3 +165,19 @@ impl Default for CollectionCache {
         }
     }
 }
+
+/// Logs internal bb8 errors
+#[derive(Debug, Clone, Copy)]
+pub struct LoggingErrorSink;
+
+impl<E: failure::Fail> ErrorSink<E> for LoggingErrorSink {
+    fn sink(&self, e: E) {
+        error!("bb8 Error: {}", e);
+        let event = sentry::integrations::failure::event_from_fail(&e);
+        sentry::capture_event(event);
+    }
+
+    fn boxed_clone(&self) -> Box<dyn ErrorSink<E>> {
+        Box::new(*self)
+    }
+}

src/error.rs

@@ -76,9 +76,6 @@ pub enum ApiErrorKind {
 
     #[fail(display = "{}", _0)]
     Validation(#[cause] ValidationError),
-
-    #[fail(display = "Invalid Submission: {}", _0)]
-    InvalidSubmission(String),
 }
 
 impl ApiError {
@@ -226,7 +223,6 @@ impl From<Context<ApiErrorKind>> for ApiError {
                 StatusCode::INTERNAL_SERVER_ERROR
             }
             ApiErrorKind::Validation(error) => error.status,
-            ApiErrorKind::InvalidSubmission(_) => StatusCode::BAD_REQUEST,
         };
 
         Self { inner, status }
@@ -280,8 +276,7 @@ impl Serialize for ApiErrorKind {
         match *self {
             ApiErrorKind::Db(ref error) => serialize_string_to_array(serializer, error),
             ApiErrorKind::Hawk(ref error) => serialize_string_to_array(serializer, error),
-            ApiErrorKind::Internal(ref description)
-            | ApiErrorKind::InvalidSubmission(ref description) => {
+            ApiErrorKind::Internal(ref description) => {
                 serialize_string_to_array(serializer, description)
             }
             ApiErrorKind::Validation(ref error) => Serialize::serialize(error, serializer),

src/server/mod.rs

@@ -165,7 +165,7 @@ impl Server {
 
         spawn_pool_periodic_reporter(Duration::from_secs(10), metrics.clone(), db_pool.clone())?;
 
-        let server = HttpServer::new(move || {
+        let mut server = HttpServer::new(move || {
             // Setup the server state
             let state = ServerState {
                 db_pool: db_pool.clone(),
@@ -177,10 +177,14 @@ impl Server {
             };
 
             build_app!(state, limits)
-        })
-        .bind(format!("{}:{}", settings.host, settings.port))
-        .expect("Could not get Server in Server::with_settings")
-        .run();
+        });
+        if let Some(keep_alive) = settings.actix_keep_alive {
+            server = server.keep_alive(keep_alive as usize);
+        }
+        let server = server
+            .bind(format!("{}:{}", settings.host, settings.port))
+            .expect("Could not get Server in Server::with_settings")
+            .run();
         Ok(server)
     }
 }

src/settings.rs

@@ -32,6 +32,8 @@ pub struct Settings {
     #[cfg(test)]
     pub database_use_test_transactions: bool,
 
+    pub actix_keep_alive: Option<u32>,
+
     /// Server-enforced limits for request payloads.
     pub limits: ServerLimits,
 
@@ -57,6 +59,7 @@ impl Default for Settings {
             database_pool_min_idle: None,
             #[cfg(test)]
             database_use_test_transactions: false,
+            actix_keep_alive: None,
             limits: ServerLimits::default(),
             master_secret: Secrets::default(),
             statsd_host: None,

src/web/extractors.rs

@@ -66,19 +66,11 @@ pub struct UidParam {
     uid: u64,
 }
 
-fn clean_entry(s: &str) -> Result<String, ApiError> {
-    // URL decode and check that the string is all ascii.
-    let decoded: String = match urlencoding::decode(s) {
-        Ok(v) => v,
-        Err(e) => {
-            debug!("unclean entry: {:?} {:?}", s, e);
-            return Err(ApiErrorKind::Internal(e.to_string()).into());
-        }
-    };
-    if !decoded.is_ascii() {
-        debug!("unclean entry, non-ascii value in {:?}", decoded);
-        return Err(ApiErrorKind::InvalidSubmission("invalid value".into()).into());
-    };
+fn urldecode(s: &str) -> Result<String, ApiError> {
+    let decoded: String = urlencoding::decode(s).map_err(|e| {
+        debug!("unclean entry: {:?} {:?}", s, e);
+        ApiErrorKind::Internal(e.to_string())
+    })?;
     Ok(decoded)
 }
 
@@ -316,20 +308,7 @@ impl FromRequest for BsoBodies {
                 }
                 // Save all id's we get, check for missing id, or duplicate.
                 let bso_id = if let Some(id) = bso.get("id").and_then(serde_json::Value::as_str) {
-                    let id = match clean_entry(&id.to_string()) {
-                        Ok(v) => v,
-                        Err(_) => {
-                            return future::err(
-                                ValidationErrorKind::FromDetails(
-                                    "Invalid BSO ID".to_owned(),
-                                    RequestErrorLocation::Body,
-                                    Some("bsos".to_owned()),
-                                    None,
-                                )
-                                .into(),
-                            );
-                        }
-                    };
+                    let id = id.to_string();
                     if bso_ids.contains(&id) {
                         return future::err(
                             ValidationErrorKind::FromDetails(
@@ -532,13 +511,12 @@ pub struct BsoParam {
 }
 
 impl BsoParam {
-    pub fn bsoparam_from_path(uri: &Uri, tags: &Tags) -> Result<Self, Error> {
+    fn bsoparam_from_path(uri: &Uri, tags: &Tags) -> Result<Self, Error> {
         // TODO: replace with proper path parser
         // path: "/1.5/{uid}/storage/{collection}/{bso}"
         let elements: Vec<&str> = uri.path().split('/').collect();
         let elem = elements.get(3);
         if elem.is_none() || elem != Some(&"storage") || elements.len() != 6 {
-            warn!("⚠️ Unexpected BSO URI: {:?}", uri.path(); tags);
             return Err(ValidationErrorKind::FromDetails(
                 "Invalid BSO".to_owned(),
                 RequestErrorLocation::Path,
@@ -547,7 +525,7 @@ impl BsoParam {
             ))?;
         }
         if let Some(v) = elements.get(5) {
-            let sv = clean_entry(&String::from_str(v).map_err(|e| {
+            let sv = urldecode(&String::from_str(v).map_err(|e| {
                 warn!("⚠️ Invalid BsoParam Error: {:?} {:?}", v, e; tags);
                 ValidationErrorKind::FromDetails(
                     "Invalid BSO".to_owned(),
@@ -631,7 +609,7 @@ impl CollectionParam {
                     Some(tags.clone()),
                 )
             })?;
-            sv = clean_entry(&sv).map_err(|_e| {
+            sv = urldecode(&sv).map_err(|_e| {
                 ValidationErrorKind::FromDetails(
                     "Invalid Collection".to_owned(),
                     RequestErrorLocation::Path,
@@ -1109,7 +1087,7 @@ impl HawkIdentifier {
         // path: "/1.5/{uid}"
         let elements: Vec<&str> = uri.path().split('/').collect();
         if let Some(v) = elements.get(2) {
-            let clean = match clean_entry(v) {
+            let clean = match urldecode(v) {
                 Err(e) => {
                     warn!("⚠️ HawkIdentifier Error invalid UID {:?} {:?}", v, e);
                     return Err(ValidationErrorKind::FromDetails(
@@ -1709,9 +1687,7 @@ fn validate_body_bso_sortindex(sort: i32) -> Result<(), ValidationError> {
 
 /// Verifies the BSO id string is valid
 fn validate_body_bso_id(id: &str) -> Result<(), ValidationError> {
-    let clean =
-        clean_entry(id).map_err(|_| request_error("Invalid id", RequestErrorLocation::Body))?;
-    if !VALID_ID_REGEX.is_match(&clean) {
+    if !VALID_ID_REGEX.is_match(id) {
         return Err(request_error("Invalid id", RequestErrorLocation::Body));
     }
     Ok(())

src/web/middleware/sentry.rs

@@ -100,7 +100,6 @@ where
 
     fn call(&mut self, sreq: ServiceRequest) -> Self::Future {
         let mut tags = Tags::from_request_head(sreq.head());
-        let uri = sreq.head().uri.to_string();
         sreq.extensions_mut().insert(tags.clone());
 
         Box::pin(self.service.call(sreq).and_then(move |mut sresp| {
@@ -119,8 +118,6 @@ where
                     tags.tags.insert(k, v);
                 }
             };
-            // add the uri.path (which can cause influx to puke)
-            tags.extra.insert("uri.path".to_owned(), uri);
             match sresp.response().error() {
                 None => {
                     // Middleware errors are eaten by current versions of Actix. Errors are now added

src/web/tags.rs

@@ -57,6 +57,7 @@ impl Tags {
         // Return an Option<> type because the later consumers (ApiErrors) presume that
         // tags are optional and wrapped by an Option<> type.
         let mut tags = HashMap::new();
+        let mut extra = HashMap::new();
         if let Some(ua) = req_head.headers().get(USER_AGENT) {
             if let Ok(uas) = ua.to_str() {
                 let (ua_result, metrics_os, metrics_browser) = parse_user_agent(uas);
@@ -65,14 +66,14 @@ impl Tags {
                 insert_if_not_empty("ua.name", ua_result.name, &mut tags);
                 insert_if_not_empty("ua.os.ver", &ua_result.os_version.to_owned(), &mut tags);
                 insert_if_not_empty("ua.browser.ver", ua_result.version, &mut tags);
+                extra.insert("ua".to_owned(), uas.to_string());
             }
         }
-        // `uri.path` causes too much cardinality for influx.
         tags.insert("uri.method".to_owned(), req_head.method.to_string());
-        Tags {
-            tags,
-            extra: HashMap::new(),
-        }
+        // `uri.path` causes too much cardinality for influx but keep it in
+        // extra for sentry
+        extra.insert("uri.path".to_owned(), req_head.uri.to_string());
+        Tags { tags, extra }
     }
 
     pub fn with_tags(tags: HashMap<String, String>) -> Tags {