diff --git a/Cargo.lock b/Cargo.lock index 51ce14cc4..67a574611 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5322,6 +5322,7 @@ dependencies = [ name = "openstack-keystone-domain-config-driver-fs" version = "0.1.0" dependencies = [ + "arc-swap", "async-trait", "eyre", "inventory", diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 7b4e0521c..b717825ef 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -525,6 +525,13 @@ impl Config { if let Some(ca) = &self.database.tls.tls_client_ca_file { watched_paths.insert(ca.clone()); } + // Per-domain config files (ADR 0034 §9): watch the directory so an + // operator's edit to a `keystone..conf` triggers a reload and the + // `fs` domain-config driver re-scans. Only when it actually exists — the + // default path is rarely present and a missing watch target just logs. + if self.identity.domain_config_dir.is_dir() { + watched_paths.insert(self.identity.domain_config_dir.clone()); + } watched_paths } @@ -656,8 +663,13 @@ impl ConfigManager { let mut watcher: RecommendedWatcher = notify::recommended_watcher(move |res: notify::Result| { if let Ok(event) = res { - // Only trigger for data modifications or name changes (renames/symlink swaps) - if event.kind.is_modify() || event.kind.is_create() { + // Data modifications, name changes (renames/symlink swaps), + // creations, and removals. Removal matters for the per-domain + // config directory (ADR 0034 §9): deleting a + // `keystone..conf` must re-scan so the domain's binding + // drops. A spurious removal event on another watched file + // costs one reload that lands on last-known-good. + if event.kind.is_modify() || event.kind.is_create() || event.kind.is_remove() { // `try_send`, not `blocking_send`: this callback runs on // notify's single background event-loop thread, which // also services `watch()`/`unwatch()` control requests. @@ -820,6 +832,25 @@ mod tests { // marked `#[parallel]`, so a mutated variable (e.g. a `KEYSTONE_SITE_VARS_FILE` // pointing at a temp file that is about to be dropped) can never leak into a // concurrently loading test. + /// ADR 0034 §9: the per-domain config directory joins the reload watch set + /// when it exists on disk, and is left out when it does not (the common + /// case, where watching a missing path would only log). + #[test] + #[parallel] + fn domain_config_dir_is_watched_only_when_present() { + let dir = tempdir().unwrap(); + + let mut cfg = Config::default(); + cfg.identity.domain_config_dir = dir.path().join("absent"); + assert!( + !cfg.get_watch_files() + .contains(&cfg.identity.domain_config_dir) + ); + + cfg.identity.domain_config_dir = dir.path().to_path_buf(); + assert!(cfg.get_watch_files().contains(&dir.path().to_path_buf())); + } + #[test] #[serial] fn test_env() { diff --git a/crates/core-types/src/domain_config/config.rs b/crates/core-types/src/domain_config/config.rs index 9bd1370da..4c489947c 100644 --- a/crates/core-types/src/domain_config/config.rs +++ b/crates/core-types/src/domain_config/config.rs @@ -60,7 +60,7 @@ use super::{ /// Returned by the group-scoped endpoints /// (`/v3/domains/{domain_id}/config/{group}`), which address exactly one /// group. -#[derive(Clone)] +#[derive(Clone, PartialEq)] pub struct DomainConfigGroup { /// The group these options belong to. name: DomainConfigGroupName, @@ -321,7 +321,7 @@ impl fmt::Debug for DomainConfigGroup { /// carries (`ldap.password`) are stripped on serialization and redacted in /// `Debug`, so the same structure serves both the internal, fully resolved /// configuration and the API response. -#[derive(Clone, Default)] +#[derive(Clone, Default, PartialEq)] pub struct DomainConfig { /// The configured groups, in a stable order. groups: BTreeMap, diff --git a/crates/core/src/assignment/service.rs b/crates/core/src/assignment/service.rs index ceb65b41f..31f5765fe 100644 --- a/crates/core/src/assignment/service.rs +++ b/crates/core/src/assignment/service.rs @@ -285,6 +285,15 @@ impl AssignmentService { self } + /// Attach an `fs` domain-config backend handle so [`Self::rebuild`] re-scans + /// the per-domain config files before enumerating bound domains (ADR 0034 + /// §9). Test-only. + #[cfg(test)] + pub(crate) fn with_dc_file_backend(mut self, backend: Arc) -> Self { + self.dc_file_backend = Some(backend); + self + } + /// The backend that serves an assignment on `(kind, target_id)`. /// /// `system` targets and every operation while dispatch is off use the @@ -383,6 +392,17 @@ impl AssignmentService { let config = state.config_manager.config.read().await.clone(); let prev = self.bundle.load_full(); + // ADR 0034 §9: a per-domain `keystone..conf` the operator edited + // on disk is invisible until the `fs` domain-config driver re-scans its + // directory. The reload watch now covers `[identity] domain_config_dir`, + // so refresh the captured handle's store before the resolver enumerates + // the bound domains. Best-effort: a scan error keeps the last good scan. + if let Some(file) = &self.dc_file_backend + && let Err(error) = file.reload(&config).await + { + warn!(%error, "fs domain-config reload failed; using the last-scanned files"); + } + let global_driver_name = config.assignment.driver.clone(); let global = if global_driver_name == prev.global_driver_name { prev.global.clone() diff --git a/crates/core/src/assignment/service/tests/reload.rs b/crates/core/src/assignment/service/tests/reload.rs index 40fc00952..ce616b118 100644 --- a/crates/core/src/assignment/service/tests/reload.rs +++ b/crates/core/src/assignment/service/tests/reload.rs @@ -227,6 +227,67 @@ async fn flipping_the_dispatch_switch_on_via_reload_takes_effect() { assert!(provider.resolver.load_full().is_some()); } +/// A reload re-scans the `fs` domain-config driver before it enumerates the +/// bound domains (ADR 0034 §9), so an operator's edit to a per-domain +/// `keystone..conf` is visible without a restart. +#[tokio::test] +async fn reload_rescans_the_fs_domain_config_backend() { + let state = get_mocked_state(Some(config(assignment_section(true, false))), None).await; + + let mut fs_mock = MockDomainConfigBackend::new(); + fs_mock.expect_reload().times(1).returning(|_| Ok(true)); + fs_mock + .expect_list_domains_with_option() + .returning(|_, _, _| Ok(Vec::new())); + let fs_backend: Arc = Arc::new(fs_mock); + + let provider = AssignmentService::from_parts( + "openfga", + Arc::new(MockAssignmentBackend::default()), + HashMap::new(), + HashMap::new(), + HashMap::new(), + None, + ) + .with_dc_file_backend(fs_backend); + + // One `rebuild` per `reload`, one `fs.reload` per `rebuild`: the + // `expect_reload().times(1)` is verified when the provider (and the Arc + // holding the mock) drop at end of test. + provider.reload(&state).await.unwrap(); +} + +/// A best-effort scan failure on the `fs` driver never fails the reload; the +/// bundle rebuild carries on against the last good scan. +#[tokio::test] +async fn reload_survives_an_fs_rescan_error() { + let state = get_mocked_state(Some(config(assignment_section(true, false))), None).await; + + let mut fs_mock = MockDomainConfigBackend::new(); + fs_mock + .expect_reload() + .returning(|_| Err(DomainConfigProviderError::Driver("disk gone".into()))); + fs_mock + .expect_list_domains_with_option() + .returning(|_, _, _| Ok(Vec::new())); + let fs_backend: Arc = Arc::new(fs_mock); + + let provider = AssignmentService::from_parts( + "openfga", + Arc::new(MockAssignmentBackend::default()), + HashMap::new(), + HashMap::new(), + HashMap::new(), + None, + ) + .with_dc_file_backend(fs_backend); + + provider + .reload(&state) + .await + .expect("an fs rescan error must not fail the reload"); +} + #[tokio::test] async fn refresh_bindings_swallows_a_rebuild_error() { let named: Arc = Arc::new(MockAssignmentBackend::default()); diff --git a/crates/core/src/domain_config/backend.rs b/crates/core/src/domain_config/backend.rs index 6190f43c1..163f16b2f 100644 --- a/crates/core/src/domain_config/backend.rs +++ b/crates/core/src/domain_config/backend.rs @@ -16,6 +16,7 @@ use async_trait::async_trait; +use openstack_keystone_config::Config; use openstack_keystone_core_types::domain_config::*; use crate::domain_config::DomainConfigProviderError; @@ -149,6 +150,26 @@ pub trait DomainConfigBackend: Send + Sync { option: &'a str, ) -> Result, DomainConfigProviderError>; + /// Re-read any state this driver cached from disk at construction, after a + /// configuration reload (ADR 0034 §9). + /// + /// The default is a no-op returning `Ok(false)`: the `sql` driver holds + /// nothing cached, and the config API's own writes are already live. The + /// `fs` driver overrides it to re-scan `[identity] domain_config_dir`, so an + /// operator's edit to a per-domain `keystone..conf` takes effect + /// without a restart. + /// + /// # Parameters + /// - `_config`: The reloaded service configuration. + /// + /// # Returns + /// - `Result` - `true` when the re-read + /// changed the driver's view, `false` when it was identical or the driver + /// caches nothing. + async fn reload(&self, _config: &Config) -> Result { + Ok(false) + } + /// Merge changes into the whole configuration of a domain. /// /// Backs `PATCH /v3/domains/{domain_id}/config`: options absent from diff --git a/crates/domain-config-driver-fs/Cargo.toml b/crates/domain-config-driver-fs/Cargo.toml index ad7cca589..69b71ef94 100644 --- a/crates/domain-config-driver-fs/Cargo.toml +++ b/crates/domain-config-driver-fs/Cargo.toml @@ -11,6 +11,7 @@ homepage.workspace = true exclude.workspace = true [dependencies] +arc-swap.workspace = true async-trait.workspace = true eyre.workspace = true inventory.workspace = true @@ -19,6 +20,7 @@ openstack-keystone-core.workspace = true openstack-keystone-core-types.workspace = true rust-ini.workspace = true serde_json.workspace = true +tokio = { workspace = true, features = ["rt"] } tracing.workspace = true [dev-dependencies] diff --git a/crates/domain-config-driver-fs/src/lib.rs b/crates/domain-config-driver-fs/src/lib.rs index 7fbf5d751..c7f7205bf 100644 --- a/crates/domain-config-driver-fs/src/lib.rs +++ b/crates/domain-config-driver-fs/src/lib.rs @@ -27,11 +27,19 @@ //! strings the file spells them with; [`DomainConfig`] and its consumers //! coerce them to the option's real type when they are resolved. //! -//! The whole directory is read once, when the driver is built at startup, and -//! held in memory. A change to a file therefore takes effect only after a -//! restart. A missing directory is normal and yields an empty driver; a -//! directory that cannot be read, or a file that cannot be parsed, fails -//! startup. +//! The whole directory is read when the driver is built at startup and held in +//! memory behind an [`ArcSwap`]. A configuration reload re-scans it in place +//! (ADR 0034 §9) — the reload watch covers `[identity] domain_config_dir` — so +//! an edit to a per-domain file takes effect without a restart. A missing +//! directory is normal and yields an empty driver; a directory that cannot be +//! read, or a file that cannot be parsed, fails the startup scan (a reload that +//! hits the same error keeps the last good scan). +//! +//! The re-scan is driven by [`DomainConfigBackend::reload`], whose sole +//! in-process caller is `AssignmentService::rebuild` on the +//! `reload_assignment_drivers_on_config_change` reactor. Because the driver is a +//! single shared `Arc`, that one call also refreshes the store this backend +//! serves to the identity service and the provider's own resolver. //! //! The driver is **read-only**. Every create, update, delete and registration //! method returns [`DomainConfigProviderError::Readonly`]; writes go to the @@ -41,6 +49,7 @@ use std::sync::Arc; +use arc_swap::ArcSwap; use async_trait::async_trait; use openstack_keystone_config::Config; @@ -64,8 +73,9 @@ fn readonly(operation: &str) -> DomainConfigProviderError { /// The filesystem domain configuration driver. pub struct FsBackend { /// The parsed contents of every `keystone.{domain_name}.conf` file found - /// in `domain_config_dir`, keyed by domain name. - store: Arc, + /// in `domain_config_dir`, keyed by domain name. Swapped wholesale by + /// [`Self::reload`] on a configuration reload. + store: ArcSwap, } impl FsBackend { @@ -81,7 +91,7 @@ impl FsBackend { pub fn new(config: &Config) -> Result { let store = store::DomainConfigStore::load(&config.identity.domain_config_dir)?; Ok(Self { - store: Arc::new(store), + store: ArcSwap::from_pointee(store), }) } @@ -148,7 +158,7 @@ impl DomainConfigBackend for FsBackend { let Some(name) = self.domain_name(state, domain_id).await? else { return Ok(None); }; - get::get_config(&self.store, &name) + get::get_config(&self.store.load(), &name) } /// A single group, with sensitive options filtered out. @@ -161,7 +171,7 @@ impl DomainConfigBackend for FsBackend { let Some(name) = self.domain_name(state, domain_id).await? else { return Ok(None); }; - get::get_group(&self.store, &name, group) + get::get_group(&self.store.load(), &name, group) } /// A single option; `None` for a sensitive one, which is never readable. @@ -175,7 +185,7 @@ impl DomainConfigBackend for FsBackend { let Some(name) = self.domain_name(state, domain_id).await? else { return Ok(None); }; - get::get_option(&self.store, &name, group, option) + get::get_option(&self.store.load(), &name, group, option) } /// The IDs of every domain whose file sets `group`/`option`. @@ -188,14 +198,20 @@ impl DomainConfigBackend for FsBackend { group: DomainConfigGroupName, option: &'a str, ) -> Result, DomainConfigProviderError> { - let names = self.store.domains_with_option(group, option); + let names: Vec = self + .store + .load() + .domains_with_option(group, option) + .into_iter() + .map(str::to_string) + .collect(); if names.is_empty() { return Ok(Vec::new()); } let ctx = ExecutionContext::internal(state); let resource = state.provider.get_resource_provider(); let mut ids = Vec::with_capacity(names.len()); - for name in names { + for name in &names { let found = resource .find_domain_by_name(&ctx, name) .await @@ -207,6 +223,29 @@ impl DomainConfigBackend for FsBackend { Ok(ids) } + /// Re-scan `[identity] domain_config_dir` and swap in the fresh store + /// (ADR 0034 §9), so an operator's edit to a `keystone..conf` takes + /// effect without a restart. Detects added, edited and removed files: the + /// swap is wholesale and a removed file's domain drops out of the fresh + /// scan. + /// + /// A scan error is propagated with the last good store left in place; the + /// reload reactor logs it and keeps serving the previous scan. + /// + /// The scan is synchronous `std::fs` walking the whole directory, so it runs + /// on a blocking thread rather than the reactor's worker. + async fn reload(&self, config: &Config) -> Result { + let dir = config.identity.domain_config_dir.clone(); + let fresh = tokio::task::spawn_blocking(move || store::DomainConfigStore::load(&dir)) + .await + .map_err(|error| DomainConfigProviderError::Driver(error.to_string()))??; + if *self.store.load_full() == fresh { + return Ok(false); + } + self.store.store(Arc::new(fresh)); + Ok(true) + } + /// Read-only: always [`DomainConfigProviderError::Readonly`]. async fn update_domain_config<'a>( &self, @@ -300,6 +339,10 @@ impl DomainConfigBackend for FsBackend { #[cfg(test)] mod tests { + use std::fs; + + use tempfile::tempdir; + use super::*; /// The driver registers under `fs`, is always selected, and builds when @@ -315,4 +358,66 @@ mod tests { assert!((registration.selected)(&config)); assert!((registration.build)(&config).await.is_ok()); } + + /// A `keystone..conf` added after startup is picked up by `reload` + /// and a second reload over the unchanged directory reports no change + /// (ADR 0034 §9). + #[tokio::test] + async fn reload_rescans_the_directory() { + let dir = tempdir().unwrap(); + let mut config = Config::default(); + config.identity.domain_config_dir = dir.path().to_path_buf(); + + let backend = FsBackend::new(&config).unwrap(); + assert!(backend.store.load().get("Acme").is_none()); + + fs::write( + dir.path().join("keystone.Acme.conf"), + "[assignment]\ndriver = openfga\n", + ) + .unwrap(); + + assert!( + backend.reload(&config).await.unwrap(), + "a new domain file must be reported as a change" + ); + assert!( + backend.store.load().get("Acme").is_some(), + "the fresh scan must hold the added domain" + ); + assert!( + !backend.reload(&config).await.unwrap(), + "a reload over an unchanged directory reports no change" + ); + } + + /// `reload` reports an edit to an existing file and a file removal, and a + /// removed file's domain drops out of the store (ADR 0034 §9). + #[tokio::test] + async fn reload_picks_up_edits_and_removals() { + let dir = tempdir().unwrap(); + let mut config = Config::default(); + config.identity.domain_config_dir = dir.path().to_path_buf(); + let path = dir.path().join("keystone.Acme.conf"); + fs::write(&path, "[assignment]\ndriver = sql\n").unwrap(); + + let backend = FsBackend::new(&config).unwrap(); + assert!(backend.store.load().get("Acme").is_some()); + + fs::write(&path, "[assignment]\ndriver = openfga\n").unwrap(); + assert!( + backend.reload(&config).await.unwrap(), + "an edit to an existing file must be reported as a change" + ); + + fs::remove_file(&path).unwrap(); + assert!( + backend.reload(&config).await.unwrap(), + "a file removal must be reported as a change" + ); + assert!( + backend.store.load().get("Acme").is_none(), + "the removed file's domain must be gone from the fresh scan" + ); + } } diff --git a/crates/domain-config-driver-fs/src/store.rs b/crates/domain-config-driver-fs/src/store.rs index 3b9807a18..62f119fac 100644 --- a/crates/domain-config-driver-fs/src/store.rs +++ b/crates/domain-config-driver-fs/src/store.rs @@ -31,7 +31,7 @@ use openstack_keystone_core_types::domain_config::{ /// Every domain config file found under `domain_config_dir`, keyed by the /// domain name its filename spells. -#[derive(Debug, Default)] +#[derive(Debug, Default, PartialEq)] pub(crate) struct DomainConfigStore(HashMap); impl DomainConfigStore {