Merge pull request #189 from cipherstash/configuration-error-messages

docs: improve config error messaging

Author: tobyhede

Cargo.lock

@@ -15,6 +15,7 @@ use serde::Deserialize;
 use std::collections::HashMap;
 use std::env;
 use std::path::PathBuf;
+use std::sync::LazyLock;
 use uuid::Uuid;
 
 #[derive(Clone, Debug, Deserialize)]
@@ -148,25 +149,29 @@ impl TandemConfig {
             .add_source(stash_setup_source)
             .build()?
             .try_deserialize()
-            .map_err(|err| match err {
-                config::ConfigError::Message(ref s) => match s {
+            .map_err(|err| {
+                // ConfigError is not helping here
+                //  - does not carry the information in structured form
+                //  - missing parameters are returned by at least two different errors, depending the source of the error
+                // Easier to inspect the error message.
+                match err.to_string() {
                     s if s.contains("UUID parsing failed") => ConfigError::InvalidDatasetId,
                     s if s.contains("missing field") => {
-                        let mut name = extract_field_name(s).map_or("unknown".to_string(), |s| s);
-
-                        if name == "name" {
-                            name = "database.name".to_string();
+                        let (field, key) = extract_missing_field_and_key(&s);
+                        match (field, key) {
+                            (field, None) if field == "auth" => ConfigError::MissingAuthKey,
+                            (field, None) if field == "encrypt" => ConfigError::MissingEncryptKey,
+                            (field, None) if field == "database" => ConfigError::MissingDatabaseKey,
+                            (field, None) => ConfigError::MissingField { field },
+                            (field, Some(key)) => ConfigError::MissingFieldForKey { key, field },
                         }
-
-                        ConfigError::MissingParameter { name }
                     }
                     s if s.contains("does not have variant constructor") => {
-                        let (name, value) = extract_invalid_field(s);
+                        let (name, value) = extract_invalid_field(&s);
                         ConfigError::InvalidParameter { name, value }
                     }
                     _ => err.into(),
-                },
-                _ => err.into(),
+                }
             })?;
 
         Ok(config)
@@ -254,18 +259,29 @@ impl Default for PrometheusConfig {
     }
 }
 
+static RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"`([^`]+)`").unwrap());
+
 ///
-/// Extracts a field name (if present) from a config::ConfigError::Message
+/// Extracts a field name (if present) from a config::ConfigError string
 /// This is called in `build` if a ConfigError message contains the string `missing field`
+/// Expected string is in the forms:
+///     "missing field `{field}}` for key `{key}`"
+///     "missing field `{field}}`"
 ///
-fn extract_field_name(input: &str) -> Option<String> {
-    let re = Regex::new(r"`(\w+)`").unwrap();
-    re.captures(input)
-        .and_then(|caps| caps.get(1).map(|m| m.as_str().to_string()))
+fn extract_missing_field_and_key(input: &str) -> (String, Option<String>) {
+    let default = "unknown";
+    let values = RE
+        .find_iter(input)
+        .map(|m| m.as_str().trim_matches('`'))
+        .collect::<Vec<_>>();
+    (
+        values.first().map_or(default.to_owned(), |s| s.to_string()),
+        values.get(1).map(|s| s.to_string()),
+    )
 }
 
 ///
-/// Extracts a field name (if present) from a config::ConfigError::Message
+/// Extracts a field name (if present) from a config::ConfigError string
 /// This is called in `build` if a ConfigError message contains the string `does not have variant constructor`
 ///
 /// Error string is `enum {name} does not have variant constructor {value}`
@@ -293,10 +309,14 @@ fn extract_invalid_field(input: &str) -> (String, String) {
 
 #[cfg(test)]
 mod tests {
-    use crate::{config::TandemConfig, error::Error};
+    use crate::{
+        config::{tandem::extract_missing_field_and_key, TandemConfig},
+        error::Error,
+    };
     use cipherstash_client::config::vars::{
         CS_CLIENT_ACCESS_KEY, CS_CLIENT_ID, CS_CLIENT_KEY, CS_DEFAULT_KEYSET_ID, CS_WORKSPACE_ID,
     };
+    use std::collections::HashMap;
     use uuid::Uuid;
 
     const CS_PREFIX: &str = "CS_TEST";
@@ -419,4 +439,176 @@ mod tests {
             },
         );
     }
+
+    #[test]
+    /// the env vars from stash setup should be the preferred option
+    /// File -> extended env (generated by the config struct layout) -> stash setup env
+    fn extract_field_and_key_name_from_config_error() {
+        let s = "missing field `client_access_key` for key `auth`";
+
+        let (field, key) = extract_missing_field_and_key(s);
+
+        assert_eq!(field, "client_access_key".to_string());
+        assert_eq!(key.unwrap(), "auth".to_string());
+
+        // Bad input - auth is extracted as field
+        let s = "blah {client_access_key} for vtha `auth`";
+
+        let (field, key) = extract_missing_field_and_key(s);
+
+        assert_eq!(field, "auth".to_string());
+        assert!(key.is_none());
+
+        // Bad input - no field can be extracted is extracted as field
+        let s = "blah {client_access_key} for vtha auth";
+
+        let (field, key) = extract_missing_field_and_key(s);
+
+        assert_eq!(field, "unknown".to_string());
+        assert!(key.is_none());
+    }
+
+    /// Returns the default environment variables as a Vec
+    fn default_env_vars() -> Vec<(&'static str, Option<&'static str>)> {
+        vec![
+            ("CS_CLIENT_ID", Some("00000000-0000-0000-0000-000000000000")),
+            ("CS_CLIENT_KEY", Some("CS_CLIENT_KEY")),
+            (
+                "CS_DEFAULT_KEYSET_ID",
+                Some("00000000-0000-0000-0000-000000000000"),
+            ),
+            ("CS_WORKSPACE_ID", Some("CS_WORKSPACE_ID")),
+            ("CS_CLIENT_ACCESS_KEY", Some("CS_CLIENT_ACCESS_KEY")),
+            ("CS_DATABASE__USERNAME", Some("CS_DATABASE__USERNAME")),
+            ("CS_DATABASE__PASSWORD", Some("CS_DATABASE__PASSWORD")),
+            ("CS_DATABASE__NAME", Some("CS_DATABASE__NAME")),
+        ]
+    }
+
+    /// Merges the default environment variables with overrides
+    fn merge_env_vars(
+        overrides: Vec<(&'static str, Option<&'static str>)>,
+    ) -> Vec<(&'static str, Option<&'static str>)> {
+        let mut env_map: HashMap<&str, Option<&str>> = default_env_vars().into_iter().collect();
+
+        for (key, value) in overrides {
+            env_map.insert(key, value);
+        }
+
+        env_map.into_iter().collect()
+    }
+
+    #[test]
+    fn missing_auth_config() {
+        // Missing both workspace id and client_access_key
+
+        let env = merge_env_vars(vec![
+            ("CS_WORKSPACE_ID", None),
+            ("CS_CLIENT_ACCESS_KEY", None),
+        ]);
+
+        temp_env::with_vars(env, || {
+            let result = TandemConfig::build("tests/config/unknown.toml");
+            assert!(result.is_err());
+
+            if let Err(err) = result {
+                assert!(err.to_string().contains("Missing [auth] configuration"));
+            } else {
+                unreachable!();
+            }
+        });
+
+        // Missing client_access_key
+        let env = merge_env_vars(vec![("CS_CLIENT_ACCESS_KEY", None)]);
+
+        temp_env::with_vars(env, || {
+            let result = TandemConfig::build("tests/config/unknown.toml");
+            assert!(result.is_err());
+
+            if let Err(err) = result {
+                assert!(err
+                    .to_string()
+                    .contains("Missing client_access_key from [auth] configuration."));
+            } else {
+                unreachable!();
+            }
+        });
+    }
+
+    #[test]
+    fn missing_encrypt_config() {
+        // Missing all encrypt config
+        let env = merge_env_vars(vec![
+            ("CS_CLIENT_ID", None),
+            ("CS_CLIENT_KEY", None),
+            ("CS_DEFAULT_KEYSET_ID", None),
+        ]);
+
+        temp_env::with_vars(env, || {
+            let result = TandemConfig::build("tests/config/unknown.toml");
+            assert!(result.is_err());
+
+            if let Err(err) = result {
+                assert!(err.to_string().contains("Missing [encrypt] configuration"));
+            } else {
+                unreachable!();
+            }
+        });
+
+        // Missing client_id
+        let env = merge_env_vars(vec![("CS_CLIENT_ID", None)]);
+
+        temp_env::with_vars(env, || {
+            let result = TandemConfig::build("tests/config/unknown.toml");
+            assert!(result.is_err());
+
+            if let Err(err) = result {
+                assert!(err
+                    .to_string()
+                    .contains("Missing client_id from [encrypt] configuration."));
+            } else {
+                unreachable!();
+            }
+        });
+    }
+
+    #[test]
+    fn missing_database_config() {
+        // Missing all database config
+
+        let env = merge_env_vars(vec![
+            ("CS_DATABASE__USERNAME", None),
+            ("CS_DATABASE__PASSWORD", None),
+            ("CS_DATABASE__NAME", None),
+            ("CS_DATABASE__HOST", None),
+            ("CS_DATABASE__PORT", None),
+        ]);
+
+        temp_env::with_vars(env, || {
+            let result = TandemConfig::build("tests/config/unknown.toml");
+            assert!(result.is_err());
+
+            if let Err(err) = result {
+                assert!(err.to_string().contains("Missing [database] configuration"));
+            } else {
+                unreachable!();
+            }
+        });
+
+        // Missing name
+        let env = merge_env_vars(vec![("CS_DATABASE__NAME", None)]);
+
+        temp_env::with_vars(env, || {
+            let result = TandemConfig::build("tests/config/unknown.toml");
+            assert!(result.is_err());
+
+            if let Err(err) = result {
+                assert!(err
+                    .to_string()
+                    .contains("Missing name from [database] configuration."));
+            } else {
+                unreachable!();
+            }
+        });
+    }
 }

packages/cipherstash-proxy/src/config/tandem.rs

@@ -6,6 +6,7 @@ use std::{io, time::Duration};
 use thiserror::Error;
 
 const ERROR_DOC_BASE_URL: &str = "https://github.com/cipherstash/proxy/blob/main/docs/errors.md";
+const ERROR_DOC_CONFIG_URL: &str = "https://github.com/cipherstash/proxy#configuring-proxy";
 
 #[derive(Error, Debug)]
 pub enum Error {
@@ -119,8 +120,35 @@ pub enum ConfigError {
     #[error("Missing an active Encrypt configuration")]
     MissingActiveEncryptConfig,
 
-    #[error("Missing field {name} from configuration file or environment")]
-    MissingParameter { name: String },
+    #[error(
+        "Missing {field} from [{key}] configuration. For help visit {}",
+        ERROR_DOC_CONFIG_URL
+    )]
+    MissingFieldForKey { field: String, key: String },
+
+    #[error(
+        "Missing {field} from configuration. For help visit {}",
+        ERROR_DOC_CONFIG_URL
+    )]
+    MissingField { field: String },
+
+    #[error(
+        "Missing [auth] configuration. Check that workspace_id and client_access_key are defined. For help visit {}",
+        ERROR_DOC_CONFIG_URL
+    )]
+    MissingAuthKey,
+
+    #[error(
+        "Missing [encrypt] configuration. Check that client_id, client_key, and default_keyset_id are defined. For help visit {}",
+        ERROR_DOC_CONFIG_URL
+    )]
+    MissingEncryptKey,
+
+    #[error(
+        "Missing [database] configuration. Check that username, password, and name are defined. For help visit {}",
+        ERROR_DOC_CONFIG_URL
+    )]
+    MissingDatabaseKey,
 
     #[error("Expected an Encrypt configuration table")]
     MissingEncryptConfigTable,

packages/cipherstash-proxy/src/error.rs

@@ -19,7 +19,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     let config = match TandemConfig::load(&args) {
         Ok(config) => config,
         Err(err) => {
-            eprintln!("Configuration Error: {}", err);
+            eprintln!("{}", err);
             std::process::exit(exitcode::CONFIG);
         }
     };