From e7e3c007e9e6319da930de8bd71f3ec7c77ab70b Mon Sep 17 00:00:00 2001 From: Mauro Ezequiel Moltrasio Date: Wed, 29 Oct 2025 11:16:09 +0100 Subject: [PATCH] feat(config): add FactConfigBuilder for better semantics The new added FactConfigBuilder makes it obvious that the way of building a configuration is by parsing a set of files and putting together a FactConfig object from them and the CLI arguments. This improves semantics and makes the code more streamlined. --- fact/src/bpf.rs | 15 ++--- fact/src/config/builder.rs | 59 +++++++++++++++++++ fact/src/config/mod.rs | 53 ++--------------- fact/src/config/reloader.rs | 114 ++++++++++++++++++++---------------- fact/src/config/tests.rs | 2 +- fact/src/lib.rs | 12 ++-- fact/src/main.rs | 5 +- 7 files changed, 137 insertions(+), 123 deletions(-) create mode 100644 fact/src/config/builder.rs diff --git a/fact/src/bpf.rs b/fact/src/bpf.rs index b1b24e44..1c8fad7c 100644 --- a/fact/src/bpf.rs +++ b/fact/src/bpf.rs @@ -217,12 +217,7 @@ mod bpf_tests { use tempfile::NamedTempFile; use tokio::{sync::watch, time::timeout}; - use crate::{ - config::{reloader::Reloader, FactConfig}, - event::process::Process, - host_info, - metrics::exporter::Exporter, - }; + use crate::{config, event::process::Process, host_info, metrics::exporter::Exporter}; use super::*; @@ -247,12 +242,10 @@ mod bpf_tests { let monitored_path = env!("CARGO_MANIFEST_DIR"); let monitored_path = PathBuf::from(monitored_path); let paths = vec![monitored_path.clone()]; - let mut config = FactConfig::default(); - config.set_paths(paths); - let reloader = Reloader::from(config); + let (_, paths) = watch::channel(paths); executor.block_on(async { - let mut bpf = Bpf::new(reloader.paths(), reloader.config().ringbuf_size()) - .expect("Failed to load BPF code"); + let mut bpf = + Bpf::new(paths, config::DEFAULT_RINGBUFFER_SIZE).expect("Failed to load BPF code"); let mut rx = bpf.subscribe(); let (run_tx, run_rx) = watch::channel(true); // Create a metrics exporter, but don't start it diff --git a/fact/src/config/builder.rs b/fact/src/config/builder.rs new file mode 100644 index 00000000..f9580cf6 --- /dev/null +++ b/fact/src/config/builder.rs @@ -0,0 +1,59 @@ +use std::{fs::read_to_string, path::PathBuf, sync::LazyLock}; + +use anyhow::Context; +use clap::Parser; + +use crate::config::FactCli; + +use super::FactConfig; + +pub(super) struct FactConfigBuilder { + files: Vec, +} + +impl FactConfigBuilder { + pub(super) fn new() -> Self { + let files = Vec::new(); + FactConfigBuilder { files } + } + + pub(super) fn add_files( + mut self, + files: &[impl Into + AsRef], + ) -> Self { + for file in files { + self.files.push(file.into()); + } + self + } + + pub(super) fn files(&self) -> &[PathBuf] { + self.files.as_slice() + } + + pub(super) fn build(&self) -> anyhow::Result { + let mut config = self + .files + .iter() + .filter(|p| p.exists()) + .map(|p| { + let content = + read_to_string(p).with_context(|| format!("Failed to read {}", p.display()))?; + FactConfig::try_from(content.as_str()) + .with_context(|| format!("parsing error while processing {}", p.display())) + }) + .try_fold( + FactConfig::default(), + |mut config: FactConfig, other: anyhow::Result| { + config.update(&other?); + Ok::(config) + }, + )?; + + // Once file configuration is handled, apply CLI arguments + static CLI_ARGS: LazyLock = LazyLock::new(|| FactCli::parse().to_config()); + config.update(&CLI_ARGS); + + Ok(config) + } +} diff --git a/fact/src/config/mod.rs b/fact/src/config/mod.rs index 42e016d1..7ad8d806 100644 --- a/fact/src/config/mod.rs +++ b/fact/src/config/mod.rs @@ -1,26 +1,19 @@ use std::{ - fs::read_to_string, net::SocketAddr, path::{Path, PathBuf}, str::FromStr, - sync::LazyLock, }; -use anyhow::{bail, Context}; +use anyhow::bail; use clap::Parser; -use log::info; use yaml_rust2::{yaml, Yaml, YamlLoader}; +mod builder; pub mod reloader; #[cfg(test)] mod tests; -const CONFIG_FILES: [&str; 4] = [ - "/etc/stackrox/fact.yml", - "/etc/stackrox/fact.yaml", - "fact.yml", - "fact.yaml", -]; +pub const DEFAULT_RINGBUFFER_SIZE: u32 = 8192; #[derive(Debug, Default, PartialEq, Eq, Clone)] pub struct FactConfig { @@ -34,44 +27,6 @@ pub struct FactConfig { } impl FactConfig { - pub fn new() -> anyhow::Result { - let config = FactConfig::build()?; - info!("{config:#?}"); - Ok(config) - } - - fn build() -> anyhow::Result { - let mut config = CONFIG_FILES - .iter() - .filter_map(|p| { - let p = Path::new(p); - if p.exists() { - Some(p) - } else { - None - } - }) - .map(|p| { - let content = - read_to_string(p).with_context(|| format!("Failed to read {}", p.display()))?; - FactConfig::try_from(content.as_str()) - .with_context(|| format!("parsing error while processing {}", p.display())) - }) - .try_fold( - FactConfig::default(), - |mut config: FactConfig, other: anyhow::Result| { - config.update(&other?); - Ok::(config) - }, - )?; - - // Once file configuration is handled, apply CLI arguments - static CLI_ARGS: LazyLock = LazyLock::new(|| FactCli::parse().to_config()); - config.update(&CLI_ARGS); - - Ok(config) - } - pub fn update(&mut self, from: &FactConfig) { if let Some(paths) = from.paths.as_deref() { self.paths = Some(paths.to_owned()); @@ -110,7 +65,7 @@ impl FactConfig { } pub fn ringbuf_size(&self) -> u32 { - self.ringbuf_size.unwrap_or(8192) + self.ringbuf_size.unwrap_or(DEFAULT_RINGBUFFER_SIZE) } pub fn hotreload(&self) -> bool { diff --git a/fact/src/config/reloader.rs b/fact/src/config/reloader.rs index 0994a7c9..a9b45d09 100644 --- a/fact/src/config/reloader.rs +++ b/fact/src/config/reloader.rs @@ -9,18 +9,66 @@ use tokio::{ time::interval, }; -use super::{EndpointConfig, FactConfig, GrpcConfig, CONFIG_FILES}; +use super::{builder::FactConfigBuilder, EndpointConfig, FactConfig, GrpcConfig}; + +const CONFIG_FILES: [&str; 4] = [ + "/etc/stackrox/fact.yml", + "/etc/stackrox/fact.yaml", + "fact.yml", + "fact.yaml", +]; pub struct Reloader { config: FactConfig, + builder: FactConfigBuilder, endpoint: watch::Sender, grpc: watch::Sender, paths: watch::Sender>, - files: HashMap<&'static str, i64>, + files: HashMap, trigger: Arc, } impl Reloader { + pub fn new() -> anyhow::Result { + let builder = FactConfigBuilder::new().add_files(CONFIG_FILES.as_slice()); + let config = builder.build()?; + info!("Startup configuration: {config:#?}"); + + let (endpoint, _) = watch::channel(config.endpoint.clone()); + let (grpc, _) = watch::channel(config.grpc.clone()); + let (paths, _) = watch::channel(config.paths().to_vec()); + let trigger = Arc::new(Notify::new()); + let files = builder + .files() + .iter() + .filter_map(|path| { + if path.exists() { + let mtime = match path.metadata() { + Ok(m) => m.mtime(), + Err(e) => { + warn!("Failed to stat {}: {e}", path.display()); + warn!("Configuration reloading may not work"); + return None; + } + }; + Some((path.clone(), mtime)) + } else { + None + } + }) + .collect(); + + Ok(Reloader { + config, + builder, + endpoint, + grpc, + paths, + files, + trigger, + }) + } + /// Consume the reloader into a task /// /// The resulting task will handle reloading the configuration and @@ -91,34 +139,33 @@ impl Reloader { fn update_cache(&mut self) -> bool { let mut res = false; - for file in CONFIG_FILES { - let path = PathBuf::from(file); - if path.exists() { - let mtime = match path.metadata() { + for file in self.builder.files() { + if file.exists() { + let mtime = match file.metadata() { Ok(m) => m.mtime(), Err(e) => { - warn!("Failed to stat {file}: {e}"); + warn!("Failed to stat {}: {e}", file.display()); warn!("Configuration reloading may not work"); continue; } }; - match self.files.get_mut(&file) { + match self.files.get_mut(file) { Some(old) if *old == mtime => {} Some(old) => { - debug!("Updating '{file}'"); + debug!("Updating '{}'", file.display()); res = true; *old = mtime; } None => { - debug!("New configuration file '{file}'"); + debug!("New configuration file '{}'", file.display()); res = true; - self.files.insert(file, mtime); + self.files.insert(file.clone(), mtime); } } - } else if self.files.contains_key(&file) { - debug!("'{file}' no longer exists, removing from cache"); + } else if self.files.contains_key(file) { + debug!("'{}' no longer exists, removing from cache", file.display()); res = true; - self.files.remove(&file); + self.files.remove(file); } } res @@ -131,7 +178,7 @@ impl Reloader { return; } - let new = match FactConfig::build() { + let new = match self.builder.build() { Ok(config) => config, Err(e) => { warn!("Configuration reloading failed: {e}"); @@ -178,40 +225,3 @@ impl Reloader { self.config = new; } } - -impl From for Reloader { - fn from(config: FactConfig) -> Self { - let files = CONFIG_FILES - .iter() - .filter_map(|path| { - let p = PathBuf::from(path); - if p.exists() { - let mtime = match p.metadata() { - Ok(m) => m.mtime(), - Err(e) => { - warn!("Failed to stat {path}: {e}"); - warn!("Configuration reloading may not work"); - return None; - } - }; - Some((*path, mtime)) - } else { - None - } - }) - .collect(); - let (endpoint, _) = watch::channel(config.endpoint.clone()); - let (grpc, _) = watch::channel(config.grpc.clone()); - let (paths, _) = watch::channel(config.paths().to_vec()); - let trigger = Arc::new(Notify::new()); - - Reloader { - config, - endpoint, - grpc, - paths, - files, - trigger, - } - } -} diff --git a/fact/src/config/tests.rs b/fact/src/config/tests.rs index 46b2b973..7b50310c 100644 --- a/fact/src/config/tests.rs +++ b/fact/src/config/tests.rs @@ -831,6 +831,6 @@ fn defaults() { assert!(!config.endpoint.health_check()); assert!(!config.skip_pre_flight()); assert!(!config.json()); - assert_eq!(config.ringbuf_size(), 8192); + assert_eq!(config.ringbuf_size(), DEFAULT_RINGBUFFER_SIZE); assert!(config.hotreload()); } diff --git a/fact/src/lib.rs b/fact/src/lib.rs index 7d07c584..a7dcf6c1 100644 --- a/fact/src/lib.rs +++ b/fact/src/lib.rs @@ -19,7 +19,7 @@ mod metrics; mod output; mod pre_flight; -use config::FactConfig; +use config::reloader::Reloader; use pre_flight::pre_flight; pub fn init_log() -> anyhow::Result<()> { @@ -60,22 +60,22 @@ pub fn log_system_information() { info!("Hostname: {}", get_hostname()); } -pub async fn run(config: FactConfig) -> anyhow::Result<()> { +pub async fn run() -> anyhow::Result<()> { + let reloader = Reloader::new()?; + let config_trigger = reloader.get_trigger(); + // Log system information as early as possible so we have it // available in case of a crash log_system_information(); let (running, _) = watch::channel(true); - if !config.skip_pre_flight() { + if !reloader.config().skip_pre_flight() { debug!("Performing pre-flight checks"); pre_flight().context("Pre-flight checks failed")?; } else { debug!("Skipping pre-flight checks"); } - let reloader = config::reloader::Reloader::from(config); - let config_trigger = reloader.get_trigger(); - let mut bpf = Bpf::new(reloader.paths(), reloader.config().ringbuf_size())?; let exporter = Exporter::new(bpf.take_metrics()?); diff --git a/fact/src/main.rs b/fact/src/main.rs index 1ae88408..92d7b658 100644 --- a/fact/src/main.rs +++ b/fact/src/main.rs @@ -1,9 +1,6 @@ -use fact::config::FactConfig; - #[tokio::main] async fn main() -> anyhow::Result<()> { fact::init_log()?; - let config = FactConfig::new()?; - fact::run(config).await + fact::run().await }