Skip to content

Standardize error handling with error-stack #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
prk-Jr wants to merge 1 commit into
base: main
Choose a base branch
from
+191 −134
Open

Standardize error handling with error-stack #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,22 @@ Consistency is the most important. Following the existing Rust style, formatting

Style and format will be enforced with a linter when PR is crated.

## :warning: Error Handling

We use [error-stack](https://docs.rs/error-stack/latest/error_stack/) for error handling to provide rich context and traceability.

### Guidelines

1. **Use `Report<E>`**: Public functions should generally return `Result<T, Report<TrustedServerError>>`.
2. **Context**: Use `.change_context(TrustedServerError::Variant)` to wrap errors and provide semantic meaning.
```rust
// Good
file.read_to_string(&mut content)
.change_context(TrustedServerError::Configuration { message: "Failed to read config".into() })?;
```
3. **Attachments**: Use `.attach_printable("additional info")` to add debugging context without changing the error variant.
4. **Consistency**: Avoid returning bare `TrustedServerError` unless absolutely necessary (e.g. implementing traits). Wrap them in `Report::new()`.

## :pray: Credits

- https://github.com/jessesquires/.github/blob/main/CONTRIBUTING.md
Expand Down
116 changes: 68 additions & 48 deletions crates/common/src/fastly_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::io::Read;

use error_stack::{Report, ResultExt};
use fastly::{ConfigStore, Request, Response, SecretStore};
use http::StatusCode;

Expand All @@ -17,17 +18,17 @@ impl FastlyConfigStore {
}
}

pub fn get(&self, key: &str) -> Result<String, TrustedServerError> {
pub fn get(&self, key: &str) -> Result<String, Report<TrustedServerError>> {
// TODO use try_open and return the error
let store = ConfigStore::open(&self.store_name);
store
.get(key)
.ok_or_else(|| TrustedServerError::Configuration {
store.get(key).ok_or_else(|| {
Report::new(TrustedServerError::Configuration {
message: format!(
"Key '{}' not found in config store '{}'",
key, self.store_name
),
})
})
}
}

Expand All @@ -42,33 +43,36 @@ impl FastlySecretStore {
}
}

pub fn get(&self, key: &str) -> Result<Vec<u8>, TrustedServerError> {
let store =
SecretStore::open(&self.store_name).map_err(|_| TrustedServerError::Configuration {
pub fn get(&self, key: &str) -> Result<Vec<u8>, Report<TrustedServerError>> {
let store = SecretStore::open(&self.store_name).map_err(|_| {
Report::new(TrustedServerError::Configuration {
message: format!("Failed to open SecretStore '{}'", self.store_name),
})?;
})
})?;

let secret = store
.get(key)
.ok_or_else(|| TrustedServerError::Configuration {
let secret = store.get(key).ok_or_else(|| {
Report::new(TrustedServerError::Configuration {
message: format!(
"Secret '{}' not found in secret store '{}'",
key, self.store_name
),
})?;
})
})?;

secret
.try_plaintext()
.map_err(|_| TrustedServerError::Configuration {
message: "Failed to get secret plaintext".into(),
.map_err(|_| {
Report::new(TrustedServerError::Configuration {
message: "Failed to get secret plaintext".into(),
})
})
.map(|bytes| bytes.into_iter().collect())
}

pub fn get_string(&self, key: &str) -> Result<String, TrustedServerError> {
pub fn get_string(&self, key: &str) -> Result<String, Report<TrustedServerError>> {
let bytes = self.get(key)?;
String::from_utf8(bytes).map_err(|e| TrustedServerError::Configuration {
message: format!("Failed to decode secret as UTF-8: {}", e),
String::from_utf8(bytes).change_context(TrustedServerError::Configuration {
message: format!("Failed to decode secret as UTF-8: {}", key),
})
}
}
Expand All @@ -79,16 +83,19 @@ pub struct FastlyApiClient {
}

impl FastlyApiClient {
pub fn new() -> Result<Self, TrustedServerError> {
pub fn new() -> Result<Self, Report<TrustedServerError>> {
Self::from_secret_store("api-keys", "api_key")
}

pub fn from_secret_store(store_name: &str, key_name: &str) -> Result<Self, TrustedServerError> {
ensure_backend_from_url("https://api.fastly.com").map_err(|e| {
pub fn from_secret_store(
store_name: &str,
key_name: &str,
) -> Result<Self, Report<TrustedServerError>> {
ensure_backend_from_url("https://api.fastly.com").change_context(
TrustedServerError::Configuration {
message: format!("Failed to ensure API backend: {}", e),
}
})?;
message: "Failed to ensure API backend".to_string(),
},
)?;

let secret_store = FastlySecretStore::new(store_name);
let api_key = secret_store.get(key_name)?;
Expand All @@ -105,7 +112,7 @@ impl FastlyApiClient {
path: &str,
body: Option<String>,
content_type: &str,
) -> Result<Response, TrustedServerError> {
) -> Result<Response, Report<TrustedServerError>> {
let url = format!("{}{}", self.base_url, path);

let api_key_str = String::from_utf8_lossy(&self.api_key).to_string();
Expand All @@ -116,9 +123,9 @@ impl FastlyApiClient {
"PUT" => Request::put(&url),
"DELETE" => Request::delete(&url),
_ => {
return Err(TrustedServerError::Configuration {
return Err(Report::new(TrustedServerError::Configuration {
message: format!("Unsupported HTTP method: {}", method),
})
}))
}
};

Expand All @@ -133,9 +140,9 @@ impl FastlyApiClient {
}

request.send("backend_https_api_fastly_com").map_err(|e| {
TrustedServerError::Configuration {
Report::new(TrustedServerError::Configuration {
message: format!("Failed to send API request: {}", e),
}
})
})
}

Expand All @@ -144,7 +151,7 @@ impl FastlyApiClient {
store_id: &str,
key: &str,
value: &str,
) -> Result<(), TrustedServerError> {
) -> Result<(), Report<TrustedServerError>> {
let path = format!("/resources/stores/config/{}/item/{}", store_id, key);
let payload = format!("item_value={}", value);

Expand All @@ -159,20 +166,22 @@ impl FastlyApiClient {
response
.get_body_mut()
.read_to_string(&mut buf)
.map_err(|e| TrustedServerError::Configuration {
message: format!("Failed to read API response: {}", e),
.map_err(|e| {
Report::new(TrustedServerError::Configuration {
message: format!("Failed to read API response: {}", e),
})
})?;

if response.get_status() == StatusCode::OK {
Ok(())
} else {
Err(TrustedServerError::Configuration {
Err(Report::new(TrustedServerError::Configuration {
message: format!(
"Failed to update config item: HTTP {} - {}",
response.get_status(),
buf
),
})
}))
}
}

Expand All @@ -181,7 +190,7 @@ impl FastlyApiClient {
store_id: &str,
secret_name: &str,
secret_value: &str,
) -> Result<(), TrustedServerError> {
) -> Result<(), Report<TrustedServerError>> {
let path = format!("/resources/stores/secret/{}/secrets", store_id);

let payload = serde_json::json!({
Expand All @@ -196,24 +205,30 @@ impl FastlyApiClient {
response
.get_body_mut()
.read_to_string(&mut buf)
.map_err(|e| TrustedServerError::Configuration {
message: format!("Failed to read API response: {}", e),
.map_err(|e| {
Report::new(TrustedServerError::Configuration {
message: format!("Failed to read API response: {}", e),
})
})?;

if response.get_status() == StatusCode::OK {
Ok(())
} else {
Err(TrustedServerError::Configuration {
Err(Report::new(TrustedServerError::Configuration {
message: format!(
"Failed to create secret: HTTP {} - {}",
response.get_status(),
buf
),
})
}))
}
}

pub fn delete_config_item(&self, store_id: &str, key: &str) -> Result<(), TrustedServerError> {
pub fn delete_config_item(
&self,
store_id: &str,
key: &str,
) -> Result<(), Report<TrustedServerError>> {
let path = format!("/resources/stores/config/{}/item/{}", store_id, key);

let mut response = self.make_request("DELETE", &path, None, "application/json")?;
Expand All @@ -222,30 +237,32 @@ impl FastlyApiClient {
response
.get_body_mut()
.read_to_string(&mut buf)
.map_err(|e| TrustedServerError::Configuration {
message: format!("Failed to read API response: {}", e),
.map_err(|e| {
Report::new(TrustedServerError::Configuration {
message: format!("Failed to read API response: {}", e),
})
})?;

if response.get_status() == StatusCode::OK
|| response.get_status() == StatusCode::NO_CONTENT
{
Ok(())
} else {
Err(TrustedServerError::Configuration {
Err(Report::new(TrustedServerError::Configuration {
message: format!(
"Failed to delete config item: HTTP {} - {}",
response.get_status(),
buf
),
})
}))
}
}

pub fn delete_secret(
&self,
store_id: &str,
secret_name: &str,
) -> Result<(), TrustedServerError> {
) -> Result<(), Report<TrustedServerError>> {
let path = format!(
"/resources/stores/secret/{}/secrets/{}",
store_id, secret_name
Expand All @@ -257,22 +274,24 @@ impl FastlyApiClient {
response
.get_body_mut()
.read_to_string(&mut buf)
.map_err(|e| TrustedServerError::Configuration {
message: format!("Failed to read API response: {}", e),
.map_err(|e| {
Report::new(TrustedServerError::Configuration {
message: format!("Failed to read API response: {}", e),
})
})?;

if response.get_status() == StatusCode::OK
|| response.get_status() == StatusCode::NO_CONTENT
{
Ok(())
} else {
Err(TrustedServerError::Configuration {
Err(Report::new(TrustedServerError::Configuration {
message: format!(
"Failed to delete secret: HTTP {} - {}",
response.get_status(),
buf
),
})
}))
}
}
}
Expand Down Expand Up @@ -329,6 +348,7 @@ mod tests {
}
}

// Other tests logic is preserved, prints error which is now a Report
#[test]
fn test_update_config_item() {
let result = FastlyApiClient::new();
Expand Down
16 changes: 13 additions & 3 deletions crates/common/src/request_signing/jwks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//! Ed25519 keypairs in JWK format for request signing.

use ed25519_dalek::{SigningKey, VerifyingKey};
use error_stack::{Report, ResultExt};
use jose_jwk::{
jose_jwa::{Algorithm, Signing},
Jwk, Key, Okp, OkpCurves, Parameters,
Expand Down Expand Up @@ -51,9 +52,14 @@ impl Keypair {
}
}

pub fn get_active_jwks() -> Result<String, TrustedServerError> {
pub fn get_active_jwks() -> Result<String, Report<TrustedServerError>> {
let store = FastlyConfigStore::new("jwks_store");
let active_kids_str = store.get("active-kids")?;
let active_kids_str =
store
.get("active-kids")
.change_context(TrustedServerError::Configuration {
message: "Failed to get active-kids".into(),
})?;

let active_kids: Vec<&str> = active_kids_str
.split(',')
Expand All @@ -63,7 +69,11 @@ pub fn get_active_jwks() -> Result<String, TrustedServerError> {

let mut jwks = Vec::new();
for kid in active_kids {
let jwk = store.get(kid)?;
let jwk = store
.get(kid)
.change_context(TrustedServerError::Configuration {
message: format!("Failed to get JWK for kid: {}", kid),
})?;
jwks.push(jwk);
}

Expand Down
Loading