From 4cc92dd17571329bd138f2be0552db26b7026055 Mon Sep 17 00:00:00 2001 From: Jason Volk Date: Fri, 12 Jul 2024 07:41:01 +0000 Subject: [PATCH] refactor Error::bad_config Signed-off-by: Jason Volk --- src/api/router/auth.rs | 3 +- src/api/routes.rs | 4 +-- src/core/config/check.rs | 52 ++++++++++++++++++++------------- src/core/config/mod.rs | 6 ++-- src/core/error.rs | 36 ++++++++++------------- src/main/tracing.rs | 19 ++++++------ src/service/globals/resolver.rs | 7 ++--- src/service/rooms/alias/mod.rs | 4 +-- src/service/sending/resolve.rs | 3 +- src/service/sending/send.rs | 3 +- 10 files changed, 71 insertions(+), 66 deletions(-) diff --git a/src/api/router/auth.rs b/src/api/router/auth.rs index 08a08e08..6c2922b9 100644 --- a/src/api/router/auth.rs +++ b/src/api/router/auth.rs @@ -6,6 +6,7 @@ use axum_extra::{ typed_header::TypedHeaderRejectionReason, TypedHeader, }; +use conduit::Err; use http::uri::PathAndQuery; use ruma::{ api::{client::error::ErrorKind, AuthScheme, Metadata}, @@ -183,7 +184,7 @@ fn auth_appservice(request: &Request, info: Box) -> Result) -> Result { if !services().globals.allow_federation() { - return Err(Error::bad_config("Federation is disabled.")); + return Err!(Config("allow_federation", "Federation is disabled.")); } let TypedHeader(Authorization(x_matrix)) = request diff --git a/src/api/routes.rs b/src/api/routes.rs index b22a32cb..3a8b2c74 100644 --- a/src/api/routes.rs +++ b/src/api/routes.rs @@ -3,7 +3,7 @@ use axum::{ routing::{any, get, post}, Router, }; -use conduit::{Error, Server}; +use conduit::{err, Error, Server}; use http::Uri; use ruma::api::client::error::ErrorKind; @@ -236,4 +236,4 @@ async fn initial_sync(_uri: Uri) -> impl IntoResponse { Error::BadRequest(ErrorKind::GuestAccessForbidden, "Guest access not implemented") } -async fn federation_disabled() -> impl IntoResponse { Error::bad_config("Federation is disabled.") } +async fn federation_disabled() -> impl IntoResponse { err!(Config("allow_federation", "Federation is disabled.")) } diff --git a/src/core/config/check.rs b/src/core/config/check.rs index 95b4df4e..67d19c72 100644 --- a/src/core/config/check.rs +++ b/src/core/config/check.rs @@ -1,6 +1,9 @@ -use crate::{error::Error, warn, Config}; +use crate::{error, error::Error, info, warn, Config, Err}; pub fn check(config: &Config) -> Result<(), Error> { + #[cfg(debug_assertions)] + info!("Note: conduwuit was built without optimisations (i.e. debug build)"); + #[cfg(all(feature = "rocksdb", not(feature = "sha256_media")))] // prevents catching this in `--all-features` warn!( "Note the rocksdb feature was deleted from conduwuit. SQLite support was removed and RocksDB is the only \ @@ -16,7 +19,7 @@ pub fn check(config: &Config) -> Result<(), Error> { config.warn_unknown_key(); if config.sentry && config.sentry_endpoint.is_none() { - return Err(Error::bad_config("Sentry cannot be enabled without an endpoint set")); + return Err!(Config("sentry_endpoint", "Sentry cannot be enabled without an endpoint set")); } #[cfg(all(feature = "hardened_malloc", feature = "jemalloc"))] @@ -27,7 +30,8 @@ pub fn check(config: &Config) -> Result<(), Error> { #[cfg(not(unix))] if config.unix_socket_path.is_some() { - return Err(Error::bad_config( + return Err!(Config( + "unix_socket_path", "UNIX socket support is only available on *nix platforms. Please remove \"unix_socket_path\" from your \ config.", )); @@ -44,7 +48,7 @@ pub fn check(config: &Config) -> Result<(), Error> { if Path::new("/proc/vz").exists() /* Guest */ && !Path::new("/proc/bz").exists() /* Host */ { - crate::error!( + error!( "You are detected using OpenVZ with a loopback/localhost listening address of {addr}. If you \ are using OpenVZ for containers and you use NAT-based networking to communicate with the \ host and guest, this will NOT work. Please change this to \"0.0.0.0\". If this is expected, \ @@ -53,7 +57,7 @@ pub fn check(config: &Config) -> Result<(), Error> { } if Path::new("/.dockerenv").exists() { - crate::error!( + error!( "You are detected using Docker with a loopback/localhost listening address of {addr}. If you \ are using a reverse proxy on the host and require communication to conduwuit in the Docker \ container via NAT-based networking, this will NOT work. Please change this to \"0.0.0.0\". \ @@ -62,7 +66,7 @@ pub fn check(config: &Config) -> Result<(), Error> { } if Path::new("/run/.containerenv").exists() { - crate::error!( + error!( "You are detected using Podman with a loopback/localhost listening address of {addr}. If you \ are using a reverse proxy on the host and require communication to conduwuit in the Podman \ container via NAT-based networking, this will NOT work. Please change this to \"0.0.0.0\". \ @@ -75,36 +79,40 @@ pub fn check(config: &Config) -> Result<(), Error> { // rocksdb does not allow max_log_files to be 0 if config.rocksdb_max_log_files == 0 { - return Err(Error::bad_config( - "rocksdb_max_log_files cannot be 0. Please set a value at least 1.", + return Err!(Config( + "max_log_files", + "rocksdb_max_log_files cannot be 0. Please set a value at least 1." )); } // yeah, unless the user built a debug build hopefully for local testing only #[cfg(not(debug_assertions))] if config.server_name == "your.server.name" { - return Err(Error::bad_config( - "You must specify a valid server name for production usage of conduwuit.", + return Err!(Config( + "server_name", + "You must specify a valid server name for production usage of conduwuit." )); } - #[cfg(debug_assertions)] - crate::info!("Note: conduwuit was built without optimisations (i.e. debug build)"); - // check if the user specified a registration token as `""` if config.registration_token == Some(String::new()) { - return Err(Error::bad_config("Registration token was specified but is empty (\"\")")); + return Err!(Config( + "registration_token", + "Registration token was specified but is empty (\"\")" + )); } if config.max_request_size < 5_120_000 { - return Err(Error::bad_config("Max request size is less than 5MB. Please increase it.")); + return Err!(Config( + "max_request_size", + "Max request size is less than 5MB. Please increase it." + )); } // check if user specified valid IP CIDR ranges on startup for cidr in &config.ip_range_denylist { if let Err(e) = ipaddress::IPAddress::parse(cidr) { - crate::error!("Error parsing specified IP CIDR range from string: {e}"); - return Err(Error::bad_config("Error parsing specified IP CIDR ranges from strings")); + return Err!(Config("ip_range_denylist", "Parsing specified IP CIDR range from string: {e}.")); } } @@ -112,13 +120,14 @@ pub fn check(config: &Config) -> Result<(), Error> { && !config.yes_i_am_very_very_sure_i_want_an_open_registration_server_prone_to_abuse && config.registration_token.is_none() { - return Err(Error::bad_config( + return Err!(Config( + "registration_token", "!! You have `allow_registration` enabled without a token configured in your config which means you are \ allowing ANYONE to register on your conduwuit instance without any 2nd-step (e.g. registration token).\n If this is not the intended behaviour, please set a registration token with the `registration_token` config option.\n For security and safety reasons, conduwuit will shut down. If you are extra sure this is the desired behaviour you \ want, please set the following config option to true: -`yes_i_am_very_very_sure_i_want_an_open_registration_server_prone_to_abuse`", +`yes_i_am_very_very_sure_i_want_an_open_registration_server_prone_to_abuse`" )); } @@ -135,8 +144,9 @@ For security and safety reasons, conduwuit will shut down. If you are extra sure } if config.allow_outgoing_presence && !config.allow_local_presence { - return Err(Error::bad_config( - "Outgoing presence requires allowing local presence. Please enable \"allow_local_presence\".", + return Err!(Config( + "allow_local_presence", + "Outgoing presence requires allowing local presence. Please enable 'allow_local_presence'." )); } diff --git a/src/core/config/mod.rs b/src/core/config/mod.rs index aadfe564..d12e7780 100644 --- a/src/core/config/mod.rs +++ b/src/core/config/mod.rs @@ -24,7 +24,7 @@ use url::Url; pub use self::check::check; use self::proxy::ProxyConfig; -use crate::error::Error; +use crate::{error::Error, Err}; pub mod check; pub mod proxy; @@ -433,13 +433,13 @@ impl Config { }; let config = match raw_config.extract::() { - Err(e) => return Err(Error::BadConfig(format!("{e}"))), + Err(e) => return Err!("There was a problem with your configuration file: {e}"), Ok(config) => config, }; // don't start if we're listening on both UNIX sockets and TCP at same time if Self::is_dual_listening(&raw_config) { - return Err(Error::bad_config("dual listening on UNIX and TCP sockets not allowed.")); + return Err!(Config("address", "dual listening on UNIX and TCP sockets not allowed.")); }; Ok(config) diff --git a/src/core/error.rs b/src/core/error.rs index a9f44dc9..77faae3a 100644 --- a/src/core/error.rs +++ b/src/core/error.rs @@ -18,35 +18,36 @@ use crate::{debug::panic_str, debug_error, error}; #[macro_export] macro_rules! err { (error!($($args:tt),+)) => {{ - error!($($args),+); - $crate::error::Error::Err(format!($($args),+)) + $crate::error!($($args),+); + $crate::error::Error::Err(std::format!($($args),+)) }}; (debug_error!($($args:tt),+)) => {{ - debug_error!($($args),+); - $crate::error::Error::Err(format!($($args),+)) + $crate::debug_error!($($args),+); + $crate::error::Error::Err(std::format!($($args),+)) }}; ($variant:ident(error!($($args:tt),+))) => {{ - error!($($args),+); - $crate::error::Error::$variant(format!($($args),+)) + $crate::error!($($args),+); + $crate::error::Error::$variant(std::format!($($args),+)) }}; ($variant:ident(debug_error!($($args:tt),+))) => {{ - debug_error!($($args),+); - $crate::error::Error::$variant(format!($($args),+)) + $crate::debug_error!($($args),+); + $crate::error::Error::$variant(std::format!($($args),+)) }}; - ($variant:ident(format!($($args:tt),+))) => { - $crate::error::Error::$variant(format!($($args),+)) - }; + (Config($item:literal, $($args:tt),+)) => {{ + $crate::error!(config = %$item, $($args),+); + $crate::error::Error::Config($item, std::format!($($args),+)) + }}; ($variant:ident($($args:tt),+)) => { - $crate::error::Error::$variant(format!($($args),+)) + $crate::error::Error::$variant(std::format!($($args),+)) }; ($string:literal$(,)? $($args:tt),*) => { - $crate::error::Error::Err(format!($string, $($args),*)) + $crate::error::Error::Err(std::format!($string, $($args),*)) }; } @@ -123,8 +124,8 @@ pub enum Error { // conduwuit #[error("Arithmetic operation failed: {0}")] Arithmetic(&'static str), - #[error("There was a problem with your configuration: {0}")] - BadConfig(String), + #[error("There was a problem with the '{0}' directive in your configuration: {1}")] + Config(&'static str, String), #[error("{0}")] BadDatabase(&'static str), #[error("{0}")] @@ -145,11 +146,6 @@ impl Error { Self::BadDatabase(message) } - pub fn bad_config(message: &str) -> Self { - error!("BadConfig: {}", message); - Self::BadConfig(message.to_owned()) - } - #[must_use] pub fn from_panic(e: Box) -> Self { Self::Panic(panic_str(&e), e) } diff --git a/src/main/tracing.rs b/src/main/tracing.rs index 67977088..0217f38a 100644 --- a/src/main/tracing.rs +++ b/src/main/tracing.rs @@ -2,9 +2,9 @@ use std::sync::Arc; use conduit::{ config::Config, - debug_warn, + debug_warn, err, log::{capture, LogLevelReloadHandles}, - Error, Result, + Result, }; use tracing_subscriber::{layer::SubscriberExt, reload, EnvFilter, Layer, Registry}; @@ -17,8 +17,7 @@ pub(crate) type TracingFlameGuard = (); pub(crate) fn init(config: &Config) -> Result<(LogLevelReloadHandles, TracingFlameGuard, Arc)> { let reload_handles = LogLevelReloadHandles::default(); - let console_filter = - EnvFilter::try_new(&config.log).map_err(|e| Error::BadConfig(format!("in the 'log' setting: {e}.")))?; + let console_filter = EnvFilter::try_new(&config.log).map_err(|e| err!(Config("log", "{e}.")))?; let console_layer = tracing_subscriber::fmt::Layer::new(); let (console_reload_filter, console_reload_handle) = reload::Layer::new(console_filter.clone()); reload_handles.add("console", Box::new(console_reload_handle)); @@ -32,8 +31,8 @@ pub(crate) fn init(config: &Config) -> Result<(LogLevelReloadHandles, TracingFla #[cfg(feature = "sentry_telemetry")] let subscriber = { - let sentry_filter = EnvFilter::try_new(&config.sentry_filter) - .map_err(|e| Error::BadConfig(format!("in the 'sentry_filter' setting: {e}.")))?; + let sentry_filter = + EnvFilter::try_new(&config.sentry_filter).map_err(|e| err!(Config("sentry_filter", "{e}.")))?; let sentry_layer = sentry_tracing::layer(); let (sentry_reload_filter, sentry_reload_handle) = reload::Layer::new(sentry_filter); reload_handles.add("sentry", Box::new(sentry_reload_handle)); @@ -44,9 +43,9 @@ pub(crate) fn init(config: &Config) -> Result<(LogLevelReloadHandles, TracingFla let (subscriber, flame_guard) = { let (flame_layer, flame_guard) = if config.tracing_flame { let flame_filter = EnvFilter::try_new(&config.tracing_flame_filter) - .map_err(|e| Error::BadConfig(format!("in the 'tracing_flame_filter' setting: {e}.")))?; + .map_err(|e| err!(Config("tracing_flame_filter", "{e}.")))?; let (flame_layer, flame_guard) = tracing_flame::FlameLayer::with_file(&config.tracing_flame_output_path) - .map_err(|e| Error::BadConfig(format!("in the 'tracing_flame_output_path' setting: {e}.")))?; + .map_err(|e| err!(Config("tracing_flame_output_path", "{e}.")))?; let flame_layer = flame_layer .with_empty_samples(false) .with_filter(flame_filter); @@ -55,8 +54,8 @@ pub(crate) fn init(config: &Config) -> Result<(LogLevelReloadHandles, TracingFla (None, None) }; - let jaeger_filter = EnvFilter::try_new(&config.jaeger_filter) - .map_err(|e| Error::BadConfig(format!("in the 'jaeger_filter' setting: {e}.")))?; + let jaeger_filter = + EnvFilter::try_new(&config.jaeger_filter).map_err(|e| err!(Config("jaeger_filter", "{e}.")))?; let jaeger_layer = config.allow_jaeger.then(|| { opentelemetry::global::set_text_map_propagator(opentelemetry_jaeger::Propagator::new()); let tracer = opentelemetry_jaeger::new_agent_pipeline() diff --git a/src/service/globals/resolver.rs b/src/service/globals/resolver.rs index 3082f2fd..3002decf 100644 --- a/src/service/globals/resolver.rs +++ b/src/service/globals/resolver.rs @@ -7,7 +7,7 @@ use std::{ time::Duration, }; -use conduit::{error, Config, Error, Result}; +use conduit::{error, Config, Result}; use hickory_resolver::TokioAsyncResolver; use reqwest::dns::{Addrs, Name, Resolve, Resolving}; use ruma::OwnedServerName; @@ -33,10 +33,7 @@ impl Resolver { #[allow(clippy::as_conversions, clippy::cast_sign_loss, clippy::cast_possible_truncation)] pub(super) fn new(config: &Config) -> Self { let (sys_conf, mut opts) = hickory_resolver::system_conf::read_system_conf() - .map_err(|e| { - error!("Failed to set up hickory dns resolver with system config: {e}"); - Error::bad_config("Failed to set up hickory dns resolver with system config.") - }) + .inspect_err(|e| error!("Failed to set up hickory dns resolver with system config: {e}")) .expect("DNS system config must be valid"); let mut conf = hickory_resolver::config::ResolverConfig::new(); diff --git a/src/service/rooms/alias/mod.rs b/src/service/rooms/alias/mod.rs index 4af1035e..97eb0f48 100644 --- a/src/service/rooms/alias/mod.rs +++ b/src/service/rooms/alias/mod.rs @@ -3,7 +3,7 @@ mod remote; use std::sync::Arc; -use conduit::{Error, Result}; +use conduit::{err, Error, Result}; use data::Data; use ruma::{ api::{appservice, client::error::ErrorKind}, @@ -171,7 +171,7 @@ impl Service { .rooms .alias .resolve_local_alias(room_alias)? - .ok_or_else(|| Error::bad_config("Room does not exist."))?, + .ok_or_else(|| err!(BadConfig("Room does not exist.")))?, )); } } diff --git a/src/service/sending/resolve.rs b/src/service/sending/resolve.rs index d38509ba..91041a2f 100644 --- a/src/service/sending/resolve.rs +++ b/src/service/sending/resolve.rs @@ -5,6 +5,7 @@ use std::{ time::SystemTime, }; +use conduit::Err; use hickory_resolver::{error::ResolveError, lookup::SrvLookup}; use ipaddress::IPAddress; use ruma::{OwnedServerName, ServerName}; @@ -354,7 +355,7 @@ fn handle_resolve_error(e: &ResolveError) -> Result<()> { fn validate_dest(dest: &ServerName) -> Result<()> { if dest == services().globals.server_name() { - return Err(Error::bad_config("Won't send federation request to ourselves")); + return Err!("Won't send federation request to ourselves"); } if dest.is_ip_literal() || IPAddress::is_valid(dest.host()) { diff --git a/src/service/sending/send.rs b/src/service/sending/send.rs index f4825ff3..18a98828 100644 --- a/src/service/sending/send.rs +++ b/src/service/sending/send.rs @@ -1,5 +1,6 @@ use std::{fmt::Debug, mem}; +use conduit::Err; use http::{header::AUTHORIZATION, HeaderValue}; use ipaddress::IPAddress; use reqwest::{Client, Method, Request, Response, Url}; @@ -26,7 +27,7 @@ where T: OutgoingRequest + Debug + Send, { if !services().globals.allow_federation() { - return Err(Error::bad_config("Federation is disabled.")); + return Err!(Config("allow_federation", "Federation is disabled.")); } let actual = resolve::get_actual_dest(dest).await?;