feat: error handling (#7)

* feat: proper error handling

.github/workflows/linting.yml

@@ -32,6 +32,6 @@ jobs:
       - name: Generate buf
         run: cd crates/valv && buf generate
       - name: Run Clippy
-        run: cargo clippy --all -- -D warnings
+        run: cargo clippy --all
       - name: Run fmt
         run: cargo fmt --all -- --check

crates/valv/Cargo.toml

@@ -14,7 +14,6 @@ name = "server"
 path = "src/cmd/main.rs"
 
 [dependencies]
-anyhow = "1.0.79"
 async-trait = "0.1.77"
 bincode = "1.3.3"
 boring = "4.1.0"
@@ -33,3 +32,20 @@ tonic = "0.10.2"
 
 [build-dependencies]
 tonic-build = { version = "0.10.2", features = ["prost"] }
+
+[lints.clippy]
+single_match = "warn"
+single_match_else = "warn"
+needless_match = "warn"
+needless_late_init = "warn"
+redundant_pattern_matching = "warn"
+redundant_pattern = "warn"
+redundant_guards = "warn"
+collapsible_match = "warn"
+match_single_binding = "warn"
+match_same_arms = "warn"
+match_ref_pats = "warn"
+match_bool = "warn"
+needless_bool = "deny"
+unwrap_used = "warn"
+expect_used = "warn"

crates/valv/build.rs

@@ -4,6 +4,7 @@ use std::process::{exit, Command};
 fn main() -> Result<(), Box<dyn Error>> {
     // We do not want to perform this action for a CICD release build.
     if Ok("debug".to_owned()) == env::var("PROFILE") {
+        #[allow(clippy::unwrap_used)]
         let status = Command::new("buf")
             .arg("generate")
             .current_dir(env!("CARGO_MANIFEST_DIR"))

crates/valv/proto/internal/resources.proto

@@ -6,7 +6,7 @@ import "google/protobuf/timestamp.proto";
 
 message Key {
   string key_id = 1;
-  string primary_version_id = 2;
+  uint32 primary_version_id = 2;
   string purpose = 3; // e.g., "ENCRYPT_DECRYPT", "SIGN_VERIFY"
   google.protobuf.Timestamp creation_time = 4;
   google.protobuf.Duration rotation_schedule = 5;

crates/valv/src/api/server.rs

@@ -30,14 +30,24 @@ impl MasterKeyManagementService for API {
             )
             .await;
 
-        let reply = CreateMasterKeyResponse {
-            master_key: Some(MasterKey {
-                name: key.key_id,
-                ..Default::default()
-            }),
-        };
-
-        Ok(tonic::Response::new(reply))
+        match key {
+            Ok(key) => {
+                let reply = CreateMasterKeyResponse {
+                    master_key: Some(MasterKey {
+                        name: key.key_id,
+                        ..Default::default()
+                    }),
+                };
+                return Ok(tonic::Response::new(reply));
+            }
+            Err(err) => {
+                println!("Failed to create master key {err}");
+                return Err(tonic::Status::new(
+                    tonic::Code::Internal,
+                    format!("Failed to create master key: {err}"),
+                ));
+            }
+        }
     }
 
     async fn list_master_keys(
@@ -97,12 +107,23 @@ impl MasterKeyManagementService for API {
             )
             .await;
 
-        let reply = crate::valv::proto::v1::EncryptResponse {
-            name: request.get_ref().master_key_id.clone(),
-            ciphertext: encrypted_value.into(),
-        };
+        match encrypted_value {
+            Ok(encrypted_value) => {
+                let reply = crate::valv::proto::v1::EncryptResponse {
+                    name: request.get_ref().master_key_id.clone(),
+                    ciphertext: encrypted_value.into(),
+                };
 
-        Ok(tonic::Response::new(reply))
+                Ok(tonic::Response::new(reply))
+            }
+            Err(err) => {
+                println!("Failed to encrypt plaintext {err}");
+                return Err(tonic::Status::new(
+                    tonic::Code::Internal,
+                    "Failed to encrypt plaintext",
+                ));
+            }
+        }
     }
 
     async fn decrypt(

crates/valv/src/cmd/main.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::unwrap_used, clippy::expect_used)]
+
 use std::sync::Arc;
 
 use clap::Parser;
@@ -18,13 +20,15 @@ struct Cli {
 
 #[tokio::main]
 async fn main() {
+    let _guard = unsafe { foundationdb::boot() };
+
     let args = Cli::parse();
 
-    let mut valv = Valv::new().await;
+    let mut valv = Valv::new().await.expect("Failed to initialize Valv");
 
     let master_key = args.master_key.clone().expose_secret().clone().into_bytes()[..32]
         .try_into()
-        .unwrap();
+        .expect("Master key must be 32 bytes");
 
     valv.set_master_key(master_key);
 

crates/valv/src/errors.rs

@@ -0,0 +1,45 @@
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum ValvError {
+    #[error("IO error")]
+    Io(#[from] std::io::Error),
+
+    #[error("Database error")]
+    Database(#[from] foundationdb::FdbError),
+
+    #[error("Transaction commit error")]
+    TransactionCommitError(#[from] foundationdb::TransactionCommitError),
+
+    #[error("Directory error")]
+    DirectoryError(foundationdb::directory::DirectoryError),
+
+    #[error("TuplePacking error")]
+    TuplePacking(#[from] foundationdb::tuple::PackError),
+
+    #[error("Prost decode error")]
+    Decode(#[from] prost::DecodeError),
+
+    #[error("BoringSSL error")]
+    BoringSSL(#[from] boring::error::ErrorStack),
+
+    #[error("Serialization error")]
+    Serialization(#[from] bincode::Error),
+
+    #[error("Parse error: {0}")]
+    Parse(String),
+
+    #[error("Invalid data: {0}")]
+    InvalidData(String),
+
+    #[error("Key not found: {0}")]
+    KeyNotFound(String),
+
+    #[error("Transport error")]
+    Transport(#[from] tonic::transport::Error),
+
+    #[error("Internal error: {0}")]
+    Internal(String),
+}
+
+pub type Result<T> = std::result::Result<T, ValvError>;

crates/valv/src/tests.rs → crates/valv/src/integration_tests.rs

@@ -1,12 +1,14 @@
+#[allow(clippy::unwrap_used, clippy::expect_used)]
 #[cfg(test)]
 mod tests {
     use crate::{
         api::server::API,
+        errors::ValvError,
         valv::proto::v1::{
             master_key_management_service_server::MasterKeyManagementServiceServer,
             CreateMasterKeyRequest, DecryptRequest, EncryptRequest, MasterKey,
         },
-        Valv, ValvAPI,
+        Valv,
     };
 
     use std::{sync::Arc, time::Duration};
@@ -23,9 +25,6 @@ mod tests {
         let _guard = unsafe { foundationdb::boot() };
 
         println!("Running server test suite");
-        println!("Testing valv");
-        test_valv().await.expect("test_valv error");
-        println!("Valv test passed\n");
 
         println!("Testing create master key");
         test_create_master_key()
@@ -41,57 +40,51 @@ mod tests {
     }
 
     async fn setup_server(
-    ) -> anyhow::Result<tokio::task::JoinHandle<Result<(), tonic::transport::Error>>> {
-        let addr = SERVER_ADDR.parse()?;
-        let mut valv = Valv::new().await;
+    ) -> Result<tokio::task::JoinHandle<Result<(), tonic::transport::Error>>, ValvError> {
+        let addr = SERVER_ADDR.parse().expect("Invalid address");
+        let mut valv = Valv::new().await?;
+
         let master_key_bytes: [u8; 32] = "77aaee825aa561995d7bda258f9b76b0"
             .as_bytes()
             .try_into()
-            .unwrap();
+            .expect("Invalid master key");
+
         valv.set_master_key(master_key_bytes);
         let api = API {
             valv: Arc::new(valv),
         };
+
         let svc = MasterKeyManagementServiceServer::new(api);
+
         Ok(tokio::spawn(Server::builder().add_service(svc).serve(addr)))
     }
 
     async fn setup_client(
-    ) -> anyhow::Result<MasterKeyManagementServiceClient<tonic::transport::Channel>> {
+    ) -> Result<MasterKeyManagementServiceClient<tonic::transport::Channel>, tonic::transport::Error>
+    {
         let channel = tonic::transport::Channel::from_static(CLIENT_ADDR)
             .connect()
             .await?;
         Ok(MasterKeyManagementServiceClient::new(channel))
     }
 
-    async fn wait_for_server() -> anyhow::Result<()> {
+    async fn wait_for_server() {
         for _ in 0..10 {
             if tonic::transport::Channel::from_static(CLIENT_ADDR)
                 .connect()
                 .await
                 .is_ok()
             {
-                return Ok(());
+                return;
             }
             sleep(Duration::from_millis(100)).await;
         }
-        anyhow::bail!("Failed to connect to server")
+        panic!("Failed to connect to server")
     }
 
-    async fn test_valv() -> anyhow::Result<()> {
-        let valv = Valv::new().await;
-        let key = valv
-            .create_key("tenant".to_string(), "test".to_string())
-            .await;
-        let key_metadata = valv.get_key("tenant".to_string(), key.key_id).await;
-        assert_eq!(key_metadata.unwrap().key_id, "test");
-
-        Ok(())
-    }
-
-    async fn test_create_master_key() -> anyhow::Result<()> {
+    async fn test_create_master_key() -> Result<(), ValvError> {
         let server_handle = setup_server().await?;
-        wait_for_server().await?;
+        wait_for_server().await;
 
         let mut client = setup_client().await?;
 
@@ -101,10 +94,10 @@ mod tests {
             master_key_id: "test".to_string(),
             keyring_name: "test_tenant".to_string(),
         });
-        let response = client.create_master_key(request).await?;
+        let response = client.create_master_key(request).await.unwrap();
 
         // Assert the response
-        assert_eq!(response.get_ref().master_key.is_some(), true);
+        assert!(response.get_ref().master_key.is_some());
 
         // Stop the server
         server_handle.abort();
@@ -113,9 +106,9 @@ mod tests {
     }
 
     // Similar tests can be written for other API methods
-    async fn test_encrypt_decrypt() -> anyhow::Result<()> {
+    async fn test_encrypt_decrypt() -> Result<(), ValvError> {
         let server_handle = setup_server().await?;
-        wait_for_server().await?;
+        wait_for_server().await;
 
         let mut client = setup_client().await?;
 
@@ -128,7 +121,7 @@ mod tests {
         let response = client.create_master_key(request).await.unwrap();
 
         // Assert the response
-        assert_eq!(response.get_ref().master_key.is_some(), true);
+        assert!(response.get_ref().master_key.is_some());
 
         // Make the gRPC call to encrypt
         let encrypt_request = tonic::Request::new(EncryptRequest {
@@ -168,7 +161,7 @@ mod tests {
         let response = client.create_master_key(request).await.unwrap();
 
         // Assert the response
-        assert_eq!(response.get_ref().master_key.is_some(), true);
+        assert!(response.get_ref().master_key.is_some());
 
         // Make the gRPC call to decrypt with another master key
         println!("Decrypting with the wrong key, should fail");
@@ -181,7 +174,7 @@ mod tests {
 
         // Assert the decrypt response
         // This should fail because the ciphertext was encrypted with a different master key
-        assert_eq!(decrypt_response_another_key.is_err(), true);
+        assert!(decrypt_response_another_key.is_err());
 
         // Stop the server
         server_handle.abort();