Repository navigation
feat(cli): connection flags + DX (quiet logging, random port, best-effort TLS) #397
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
31a410d
2acf9f2
ea42e58
f87819c
fde3ad2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,6 @@ use regex::Regex; | |
| use serde::Deserialize; | ||
| use std::collections::HashMap; | ||
| use std::env; | ||
| use std::path::PathBuf; | ||
| use std::sync::LazyLock; | ||
| use uuid::Uuid; | ||
|
|
||
|
|
@@ -88,36 +87,43 @@ impl TandemConfig { | |
| } | ||
|
|
||
| pub fn load(args: &Args) -> Result<TandemConfig, Error> { | ||
| // Log a warning to user that config file is missing | ||
| if !PathBuf::from(&args.config_file_path).exists() { | ||
| println!( | ||
| "Configuration file was not found: {}", | ||
| args.config_file_path | ||
| ); | ||
| println!("Loading config values from environment variables."); | ||
| } | ||
| let mut config = TandemConfig::build(args)?; | ||
|
|
||
| // If log level is default, it has not been set by the user in config | ||
| if config.log.level == LogConfig::default_log_level() { | ||
| config.log.level = args.log_level; | ||
| if let Some(log_level) = args.log_level { | ||
| config.log.level = log_level; | ||
| } | ||
| } | ||
|
|
||
| // If log format is default, it has not been set by the user in config | ||
| if config.log.format == LogConfig::default_log_format() { | ||
| config.log.format = args.log_format; | ||
| } | ||
|
|
||
| // --no-tls forces a plaintext inbound listener, ignoring any CS_TLS__* | ||
| // env / config. (Does not affect the proxy -> database connection.) | ||
| if args.no_tls { | ||
| config.tls = None; | ||
| config.server.require_tls = false; | ||
| } | ||
|
|
||
| Ok(config) | ||
| } | ||
|
|
||
| pub fn build_path(path: &str) -> Result<Self, Error> { | ||
| let args = Args { | ||
| config_file_path: path.to_string(), | ||
| database_url: None, | ||
| db_host: None, | ||
| db_port: None, | ||
| db_name: None, | ||
| db_user: None, | ||
| log_level: LogConfig::default_log_level(), | ||
| db_password: None, | ||
| no_tls: false, | ||
| tls: false, | ||
| debug: false, | ||
| log_level: None, | ||
| log_format: LogConfig::default_log_format(), | ||
| command: None, | ||
| }; | ||
|
|
@@ -169,20 +175,31 @@ impl TandemConfig { | |
| env | ||
| })); | ||
|
|
||
| // Command line arguments override env vars | ||
| // Command line arguments override env vars. | ||
| // A full connection string is applied first; individual flags below | ||
| // then override matching parts of it. | ||
| if let Some(url) = &args.database_url { | ||
| apply_database_url(url)?; | ||
| } | ||
|
|
||
| if let Some(db_host) = &args.db_host { | ||
| println!("Overriding database host from command line argument"); | ||
| env::set_var("CS_DATABASE__HOST", db_host); | ||
| } | ||
|
|
||
| if let Some(db_port) = &args.db_port { | ||
| env::set_var("CS_DATABASE__PORT", db_port.to_string()); | ||
| } | ||
|
|
||
| if let Some(dbname) = &args.db_name { | ||
| println!("Overriding database name from command line argument"); | ||
| env::set_var("CS_DATABASE__NAME", dbname); | ||
| } | ||
|
|
||
| if let Some(db_user) = &args.db_user { | ||
| println!("Overriding database user from command line argument"); | ||
| env::set_var("CS_DATABASE__USER", db_user); | ||
| env::set_var("CS_DATABASE__USERNAME", db_user); | ||
| } | ||
|
|
||
| if let Some(db_password) = &args.db_password { | ||
| env::set_var("CS_DATABASE__PASSWORD", db_password); | ||
| } | ||
|
|
||
| // Source order is important! | ||
|
|
@@ -358,9 +375,53 @@ impl Default for PrometheusConfig { | |
|
|
||
| static RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"`([^`]+)`").unwrap()); | ||
|
|
||
| /// | ||
| /// 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` | ||
| /// Parse a PostgreSQL connection string (e.g. | ||
| /// "postgres://user:pass@host:5432/dbname") and set the corresponding | ||
| /// CS_DATABASE__* env vars for each component present. Individual --db-* flags | ||
| /// are applied after this and take precedence. | ||
| fn apply_database_url(url: &str) -> Result<(), Error> { | ||
| use std::str::FromStr; | ||
|
|
||
| let pg = | ||
| tokio_postgres::Config::from_str(url).map_err(|err| ConfigError::InvalidParameter { | ||
| name: "database-url".to_string(), | ||
| value: err.to_string(), | ||
| })?; | ||
|
Comment on lines
+382
to
+389
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Result: Fix: reject a URL that has |
||
|
|
||
| if let Some(host) = pg.get_hosts().iter().find_map(|h| match h { | ||
| tokio_postgres::config::Host::Tcp(host) => Some(host.clone()), | ||
| _ => None, | ||
| }) { | ||
| env::set_var("CS_DATABASE__HOST", host); | ||
| } | ||
|
|
||
| if let Some(port) = pg.get_ports().first() { | ||
| env::set_var("CS_DATABASE__PORT", port.to_string()); | ||
| } | ||
|
|
||
| if let Some(user) = pg.get_user() { | ||
| env::set_var("CS_DATABASE__USERNAME", user); | ||
| } | ||
|
|
||
| if let Some(password) = pg.get_password() { | ||
| // The password is required downstream as a String. Rather than silently | ||
| // dropping a non-UTF8 password (which would leave the proxy connecting | ||
| // with no password, or a stale env/config one), fail clearly. | ||
| let password = | ||
| std::str::from_utf8(password).map_err(|_| ConfigError::InvalidParameter { | ||
| name: "database-url".to_string(), | ||
| value: "password contains invalid UTF-8".to_string(), | ||
| })?; | ||
| env::set_var("CS_DATABASE__PASSWORD", password); | ||
| } | ||
|
Comment on lines
+406
to
+416
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| if let Some(dbname) = pg.get_dbname() { | ||
| env::set_var("CS_DATABASE__NAME", dbname); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Expected string is in the forms: | ||
| /// "missing field `{field}}` for key `{key}`" | ||
| /// "missing field `{field}}`" | ||
|
|
@@ -419,6 +480,61 @@ mod tests { | |
|
|
||
| const CS_PREFIX: &str = "CS_TEST"; | ||
|
|
||
| #[test] | ||
| /// --database-url sets each CS_DATABASE__* var, including USERNAME (not USER | ||
| /// -- a previous bug set the wrong var so --db-user was a silent no-op). | ||
| fn database_url_sets_connection_env() { | ||
| use crate::config::tandem::apply_database_url; | ||
| with_no_cs_vars(|| { | ||
| temp_env::with_vars_unset( | ||
| [ | ||
| "CS_DATABASE__HOST", | ||
| "CS_DATABASE__PORT", | ||
| "CS_DATABASE__USERNAME", | ||
| "CS_DATABASE__USER", | ||
| "CS_DATABASE__PASSWORD", | ||
| "CS_DATABASE__NAME", | ||
| ], | ||
| || { | ||
| apply_database_url("postgres://alice:s3cret@db.example.com:5430/orders") | ||
| .unwrap(); | ||
|
|
||
| assert_eq!( | ||
| std::env::var("CS_DATABASE__HOST").unwrap(), | ||
| "db.example.com" | ||
| ); | ||
| assert_eq!(std::env::var("CS_DATABASE__PORT").unwrap(), "5430"); | ||
| assert_eq!(std::env::var("CS_DATABASE__USERNAME").unwrap(), "alice"); | ||
| assert_eq!(std::env::var("CS_DATABASE__PASSWORD").unwrap(), "s3cret"); | ||
| assert_eq!(std::env::var("CS_DATABASE__NAME").unwrap(), "orders"); | ||
| // The old (buggy) variable name must not be set. | ||
| assert!(std::env::var("CS_DATABASE__USER").is_err()); | ||
| }, | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| /// A non-UTF8 password in --database-url is an error, not a silent no-op. | ||
| /// tokio-postgres percent-decodes the password to raw bytes without UTF-8 | ||
| /// validation, so `%FF` yields invalid UTF-8; we must reject it rather than | ||
| /// leave CS_DATABASE__PASSWORD unset (which would connect with no/stale | ||
| /// credentials). | ||
| fn database_url_rejects_non_utf8_password() { | ||
| use crate::config::tandem::apply_database_url; | ||
| with_no_cs_vars(|| { | ||
| temp_env::with_vars_unset(["CS_DATABASE__PASSWORD"], || { | ||
| let result = apply_database_url("postgres://alice:%FF@db.example.com/orders"); | ||
| assert!( | ||
| result.is_err(), | ||
| "expected a non-UTF8 password to be rejected" | ||
| ); | ||
| // And it must not have set a password from the bad URL. | ||
| assert!(std::env::var("CS_DATABASE__PASSWORD").is_err()); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| /// the env vars from stash setup should be the preferred option | ||
| /// File -> extended env (generated by the config struct layout) -> stash setup env | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This advice is wrong for
--database-url.--database-urlis also a command-line argument. A password in the URL goes into shell history andpsoutput, the same as--db-password.Fix: recommend only
CS_DATABASE__PASSWORDhere. If--database-urlis the main path fornpx stash proxy, also give it an env source (for example#[arg(long, env = "DATABASE_URL")]), and note that a URL on the command line exposes the password.