-
Notifications
You must be signed in to change notification settings - Fork 93
Configure directory with config & clap #927
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
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| use std::env; | ||
| use std::path::PathBuf; | ||
|
|
||
| use clap::{value_parser, Parser}; | ||
|
|
||
| #[derive(Debug, Parser)] | ||
| #[command( | ||
| version = env!("CARGO_PKG_VERSION"), | ||
| about = "Payjoin Directory Server", | ||
| long_about = None, | ||
| )] | ||
| pub struct Cli { | ||
| #[arg( | ||
| long, | ||
| short = 'p', | ||
| env = "PJ_DIR_PORT", | ||
| default_value = "8080", | ||
| help = "The port to bind" | ||
| )] | ||
| pub port: u16, // TODO tokio_listener::ListenerAddressLFlag | ||
|
|
||
| #[arg( | ||
| long, | ||
| short = 'p', | ||
| env = "PJ_METRIC_PORT", | ||
| default_value = "9090", | ||
| help = "The port to bind for prometheus metrics export" | ||
| )] | ||
| pub metrics_port: u16, // TODO tokio_listener::ListenerAddressLFlag | ||
|
|
||
| #[arg( | ||
| long, | ||
| env = "PJ_DIR_TIMEOUT_SECS", | ||
| default_value = "30", | ||
| help = "The timeout for long polling operations" | ||
| )] | ||
| pub timeout: u64, | ||
|
|
||
| #[arg( | ||
| long = "db-host", | ||
| env = "PJ_DB_HOST", | ||
| default_value = "localhost:6379", | ||
|
Comment on lines
+41
to
+42
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. Use default const or remove
Contributor
Author
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. it'll removed with redis, but i didn't want to break compat with the current behavior just yet in case we need to do a release for some reason this PR should behave exactly the same as before except also support a config file and -- style options |
||
| help = "The redis host to connect to" | ||
| )] | ||
| pub db_host: String, | ||
|
|
||
| #[arg( | ||
| long = "ohttp-keys", | ||
| env = "PJ_OHTTP_KEY_DIR", | ||
| help = "The ohttp key config file path", | ||
| default_value = "ohttp_keys", | ||
| value_parser = value_parser!(PathBuf) | ||
| )] | ||
| pub ohttp_keys: PathBuf, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| use std::path::PathBuf; | ||
| use std::time::Duration; | ||
|
|
||
| use anyhow::Result; | ||
| use config::builder::DefaultState; | ||
| use config::{ConfigError, File, FileFormat}; | ||
| use serde::Deserialize; | ||
|
|
||
| type Builder = config::builder::ConfigBuilder<DefaultState>; | ||
|
|
||
| use crate::cli::Cli; | ||
|
|
||
| #[derive(Debug, Clone, Deserialize)] | ||
| pub struct Config { | ||
| pub listen_addr: String, // TODO tokio_listener::ListenerAddressLFlag | ||
| pub metrics_listen_addr: String, // TODO tokio_listener::ListenerAddressLFlag | ||
| pub timeout: Duration, | ||
| pub db_host: String, | ||
| pub ohttp_keys: PathBuf, // TODO OhttpConfig struct with rotation params, etc | ||
| } | ||
|
|
||
| impl Config { | ||
| pub fn new(cli: &Cli) -> Result<Self, ConfigError> { | ||
| let mut config = config::Config::builder(); | ||
| config = add_defaults(config, cli)?; | ||
|
|
||
| // what directory should this reside in? require explicit --config-file? ~/.config? /etc? | ||
| config = config.add_source(File::new("config.toml", FileFormat::Toml).required(false)); | ||
|
Collaborator
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. I think we should follow the same pattern as payjoin-cli for configuration file handling: first, check for config file in xdg-compliant default directory This would provide a consistent user experience across the codebase and follow established conventions.
Contributor
Author
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. that's definitely one option, but i'm not sure, since payjoin-directory is a service, not a command line application intended to be run by users, so conventions are a bit different usually system services store configurations in /etc not ~/.config, and state in /var/lib, but that requires system level setup of those files & directories, so perhaps it should just be easy to override those paths and still default to xdg ones however i think i would prefer just errorring if an explicit path is not given, at least for state storage, it's less confusing than running it without error but not knowing where any state data ended up |
||
|
|
||
| let built_config = config.build()?; | ||
|
|
||
| Ok(Config { | ||
| listen_addr: built_config.get("listen_addr")?, | ||
| metrics_listen_addr: built_config.get("metrics_listen_addr")?, | ||
| timeout: Duration::from_secs(built_config.get("timeout")?), | ||
| db_host: built_config.get("db_host")?, | ||
| ohttp_keys: built_config.get("ohttp_keys")?, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| fn add_defaults(config: Builder, cli: &Cli) -> Result<Builder, ConfigError> { | ||
| config | ||
| .set_override_option("listen_addr", Some(format!("[::]:{}", cli.port)))? | ||
| .set_override_option( | ||
| "metrics_listen_addr", | ||
| Some(format!("localhost:{}", cli.metrics_port)), | ||
| )? | ||
| .set_override_option("timeout", Some(cli.timeout))? | ||
| .set_override_option("db_host", Some(cli.db_host.to_owned()))? | ||
| .set_override_option("ohttp_keys", Some(cli.ohttp_keys.to_string_lossy().into_owned())) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,36 +1,20 @@ | ||
| use std::env; | ||
| use std::net::{IpAddr, Ipv6Addr, SocketAddr}; | ||
|
|
||
| use clap::Parser; | ||
| use payjoin_directory::metrics::Metrics; | ||
| use payjoin_directory::*; | ||
| use tokio::net::TcpListener; | ||
| use tracing::error; | ||
| use tracing_subscriber::filter::LevelFilter; | ||
| use tracing_subscriber::EnvFilter; | ||
|
|
||
| const DEFAULT_KEY_CONFIG_DIR: &str = "ohttp_keys"; | ||
|
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. you removed this one but not those in lib.rs. The others are pub but I'm not sure they need to be.
Contributor
Author
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. good catch, thanks |
||
|
|
||
| #[tokio::main] | ||
| async fn main() -> Result<(), BoxError> { | ||
| init_logging(); | ||
|
|
||
| let dir_port = | ||
| env::var("PJ_DIR_PORT").map_or(DEFAULT_DIR_PORT, |s| s.parse().expect("Invalid port")); | ||
|
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. const DEFAULT_DIR_PORT definition may be removed |
||
|
|
||
| let metric_port = env::var("PJ_METRIC_PORT") | ||
| .map_or(DEFAULT_METRIC_PORT, |s| s.parse().expect("invalid metric port")); | ||
|
|
||
| let timeout_env = env::var("PJ_DIR_TIMEOUT_SECS") | ||
| .map_or(DEFAULT_TIMEOUT_SECS, |s| s.parse().expect("Invalid timeout")); | ||
| let timeout = std::time::Duration::from_secs(timeout_env); | ||
| let cli = cli::Cli::parse(); | ||
| let config = config::Config::new(&cli)?; | ||
|
|
||
| let db_host = env::var("PJ_DB_HOST").unwrap_or_else(|_| DEFAULT_DB_HOST.to_string()); | ||
|
|
||
| let key_dir = | ||
| std::env::var("PJ_OHTTP_KEY_DIR").map(std::path::PathBuf::from).unwrap_or_else(|_| { | ||
| let key_dir = std::path::PathBuf::from(DEFAULT_KEY_CONFIG_DIR); | ||
| std::fs::create_dir_all(&key_dir).expect("Failed to create key directory"); | ||
| key_dir | ||
| }); | ||
| let key_dir = config.ohttp_keys; | ||
| std::fs::create_dir_all(&key_dir).expect("Failed to create key directory"); | ||
|
|
||
| let ohttp = match key_config::read_server_config(&key_dir) { | ||
| Ok(config) => config, | ||
|
|
@@ -42,26 +26,24 @@ async fn main() -> Result<(), BoxError> { | |
| } | ||
| }; | ||
|
|
||
| // Start metrics server in the background | ||
| let db = DbPool::new(config.timeout, config.db_host).await?; | ||
| let metrics = Metrics::new(); | ||
| let metrics_listener = bind_port(metric_port).await?; | ||
|
|
||
| let listener = bind_port(dir_port).await?; | ||
| let db = DbPool::new(timeout, db_host).await?; | ||
| let service = Service::new(db, ohttp.into(), metrics); | ||
| let service_clone = service.clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(e) = payjoin_directory::serve_metrics_tcp(service_clone, metrics_listener).await | ||
| { | ||
| eprintln!("Metrics server error: {e}"); | ||
| } | ||
| }); | ||
| service.serve_tcp(listener).await | ||
| } | ||
|
|
||
| async fn bind_port(port: u16) -> Result<tokio::net::TcpListener, std::io::Error> { | ||
| let bind_addr = SocketAddr::new(IpAddr::V6(Ipv6Addr::UNSPECIFIED), port); | ||
| TcpListener::bind(bind_addr).await | ||
| // Start metrics server in the background | ||
| let metrics_listener = TcpListener::bind(config.metrics_listen_addr).await?; | ||
| { | ||
| let service = service.clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(e) = payjoin_directory::serve_metrics_tcp(service, metrics_listener).await { | ||
| error!("Metrics server error: {e}"); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| let listener = TcpListener::bind(config.listen_addr).await?; | ||
| service.serve_tcp(listener).await | ||
| } | ||
|
|
||
| fn init_logging() { | ||
|
|
||
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.
DEFAULT_TIMEOUT_SECS.to_string() can be used here or otherwise its definition removed. I think this is self-documenting so prefer removal.