diff --git a/src/db/spanner/pool.rs b/src/db/spanner/pool.rs index 1d031159..610335c7 100644 --- a/src/db/spanner/pool.rs +++ b/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 ErrorSink 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> { + Box::new(*self) + } +} diff --git a/src/error.rs b/src/error.rs index e2d96dd6..061f5f41 100644 --- a/src/error.rs +++ b/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> 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), diff --git a/src/server/mod.rs b/src/server/mod.rs index 786bf666..d9644ef2 100644 --- a/src/server/mod.rs +++ b/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) } } diff --git a/src/settings.rs b/src/settings.rs index e271929d..17b22d35 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -32,6 +32,8 @@ pub struct Settings { #[cfg(test)] pub database_use_test_transactions: bool, + pub actix_keep_alive: Option, + /// 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, diff --git a/src/web/extractors.rs b/src/web/extractors.rs index ba1a52d8..4a089667 100644 --- a/src/web/extractors.rs +++ b/src/web/extractors.rs @@ -66,19 +66,11 @@ pub struct UidParam { uid: u64, } -fn clean_entry(s: &str) -> Result { - // 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 { + 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 { + fn bsoparam_from_path(uri: &Uri, tags: &Tags) -> Result { // 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(()) diff --git a/src/web/middleware/sentry.rs b/src/web/middleware/sentry.rs index 56d7c31c..20c5d741 100644 --- a/src/web/middleware/sentry.rs +++ b/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 diff --git a/src/web/tags.rs b/src/web/tags.rs index f3840100..d5923331 100644 --- a/src/web/tags.rs +++ b/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) -> Tags {