Skip to content

Commit

Permalink
Improve error messages when server misconfigured (#1520)
Browse files Browse the repository at this point in the history
<!--
Thank you for your Pull Request. Please provide a description above and
review
the requirements below.

Bug fixes and new features should include tests.
-->

## Motivation

<!--
Explain the context and why you're making that change. What is the
problem
you're trying to solve? If a new feature is being added, describe the
intended
use case that feature fulfills.
-->

Relates to #1435

This PR is trying out a way to improve the error message shown when
something is misconfigured.

Currently running the server binary (`cargo run` in the `svix-server/`
folder) panics when some needed config is missing.

e.g. `listen_address` not set
```
thread 'main' panicked at svix-server/src/cfg.rs:550:10:
Error loading configuration: Error { tag: Tag::Default, profile: Some(Profile(Uncased { string: "default" })), metadata: None, path: [], kind: MissingField("listen_address"), prev: None }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

e.g. `jwt_secret` not set - in this case the error message doesn't
indicate the missing field due to an issue with it coming from an
inlined struct inside `ConfigurationInner` -
<SergioBenitez/Figment#80>
```
thread 'main' panicked at svix-server/src/cfg.rs:550:10:
Error loading configuration: Error { tag: Tag::Default, profile: Some(Profile(Uncased { string: "default" })), metadata: None, path: [], kind: Message("data did not match any variant of untagged enum JwtSigningConfig"), prev: None }
```

## Solution

<!--
Summarize the solution and provide any necessary context needed to
understand
the code change.
-->

If `cfg::load()` errors out, terminate with exit code 1 and print the
formatted error to stderr. I tried switching to using `anyhow::Result`
for `main()` and `cfg::load()` to make propagating errors with human
friendly context easier.

```
Error: failed to extract configuration

Caused by:
    missing field `listen_address`
```

Check upfront that `jwt_secret` is set and override the error message to
be more helpful due to
<SergioBenitez/Figment#80>

```
Error: failed to extract configuration

Caused by:
    missing field `jwt_secret`
```
  • Loading branch information
svix-onelson authored Nov 19, 2024
2 parents bf7cdeb + 0f6240e commit 38130cc
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 12 deletions.
2 changes: 1 addition & 1 deletion server/svix-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@ omniqueue = { git = "https://github.com/svix/omniqueue-rs", rev = "75e5a9510ad33
# Switch to hyper-http-proxy when upgrading hyper to 1.0.
hyper-proxy = { version = "=0.9.1", default-features = false, features = ["openssl-tls"] }
hex = "0.4.3"
anyhow = "1.0.56"

[target.'cfg(not(target_env = "msvc"))'.dependencies]
tikv-jemallocator = { version = "0.5", optional = true }

[dev-dependencies]
anyhow = "1.0.56"
assert_matches = "1.5.0"
# NOTE: Purposely not the latest version such as not to mess up the `hyper` fork patch
axum-server = { version = "0.5", features = ["tls-openssl"] }
Expand Down
40 changes: 33 additions & 7 deletions server/svix-server/src/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use std::{borrow::Cow, collections::HashMap, fmt, net::SocketAddr, sync::Arc, time::Duration};

use anyhow::{bail, Context};
use figment::{
providers::{Env, Format, Toml},
Figment,
Expand Down Expand Up @@ -533,7 +534,20 @@ impl From<SentinelConfig> for omniqueue::backends::redis::SentinelConfig {
}
}

pub fn load() -> Result<Arc<ConfigurationInner>> {
/// Try to extract a [`ConfigurationInner`] from the provided [`Figment`]. Any error message should
/// indicate the missing required field(s).
fn try_extract(figment: Figment) -> anyhow::Result<ConfigurationInner> {
// Explicitly override error if `jwt_secret` is not set, as the default error does not mention
// the field name due it coming from an inlined field `ConfigurationInner::jwt_signing_config`
// See: <https://github.com/SergioBenitez/Figment/issues/80>
if !figment.contains("jwt_secret") {
bail!("missing field `jwt_secret`");
}

Ok(figment.extract()?)
}

pub fn load() -> anyhow::Result<Arc<ConfigurationInner>> {
if let Ok(db_url) = std::env::var("DATABASE_URL") {
// If we have DATABASE_URL set, we should potentially use it.
const DB_DSN: &str = "SVIX_DB_DSN";
Expand All @@ -542,14 +556,16 @@ pub fn load() -> Result<Arc<ConfigurationInner>> {
}
}

let config: ConfigurationInner = Figment::new()
let merged = Figment::new()
.merge(Toml::string(DEFAULTS))
.merge(Toml::file("config.toml"))
.merge(Env::prefixed("SVIX_"))
.extract()
.expect("Error loading configuration");
.merge(Env::prefixed("SVIX_"));

let config = try_extract(merged).context("failed to extract configuration")?;

config.validate().expect("Error validating configuration");
config
.validate()
.context("failed to validate configuration")?;
Ok(Arc::from(config))
}

Expand All @@ -562,7 +578,7 @@ mod tests {
Figment,
};

use super::{load, CacheBackend, CacheType, QueueBackend, QueueType};
use super::{load, try_extract, CacheBackend, CacheType, QueueBackend, QueueType};
use crate::core::security::{JWTAlgorithm, JwtSigningConfig};

#[test]
Expand All @@ -582,6 +598,16 @@ mod tests {
assert_eq!(cfg.cache_backend(), CacheBackend::Redis("test_b"));
}

#[test]
fn test_try_extract_missing_jwt_secret() {
let defaults = Figment::new();

let actual = try_extract(defaults);

let err = actual.unwrap_err();
assert_eq!(err.to_string(), "missing field `jwt_secret`");
}

#[test]
fn test_jwt_signing_fallback() {
let raw_config = r#"
Expand Down
9 changes: 5 additions & 4 deletions server/svix-server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#![warn(clippy::all)]
#![forbid(unsafe_code)]

use anyhow::bail;
use clap::{Parser, Subcommand};
use dotenvy::dotenv;
use svix_server::{
Expand Down Expand Up @@ -108,11 +109,11 @@ fn org_id_parser(s: &str) -> Result<OrganizationId, String> {
}

#[tokio::main]
async fn main() {
async fn main() -> anyhow::Result<()> {
dotenv().ok();

let args = Args::parse();
let cfg = cfg::load().expect("Error loading configuration");
let cfg = cfg::load()?;

let (tracing_subscriber, _guard) = setup_tracing(&cfg, /* for_test = */ false);
tracing_subscriber.init();
Expand All @@ -136,8 +137,7 @@ async fn main() {
}

if let Err(e) = futures::future::try_join_all(wait_for).await {
tracing::error!("{e}");
return;
bail!(e);
}
}

Expand Down Expand Up @@ -201,4 +201,5 @@ async fn main() {
};

opentelemetry::global::shutdown_tracer_provider();
Ok(())
}

0 comments on commit 38130cc

Please sign in to comment.