From 78cdd982349389aef57718e27b261325f90ab013 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Mon, 7 Sep 2026 16:06:27 +0200 Subject: [PATCH] feat(adr0034): Gate sources on domain_config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - effective_domain_config_sources() takes the [domain_config] from_files/from_database keys where set, falling back to the deprecated [identity] switches; warns once per key when an operator set both to conflicting values - DomainConfigResolver::new consumes it instead of reading the [identity] switches directly - DomainConfigBackend::list_domains_with_option: sql runs a DISTINCT domain_id query on whitelisted_config, fs scans the parsed files and resolves each name to an id - DomainConfigResolver::bound_domains unions assignment/driver bindings across active sources for the §5 fan-out set Signed-off-by: Artem Goncharov --- crates/core/src/domain_config/backend.rs | 23 ++++ crates/core/src/domain_config/resolver.rs | 123 ++++++++++++++++-- .../core/src/domain_config/resolver/tests.rs | 115 ++++++++++++++++ crates/domain-config-driver-fs/src/lib.rs | 29 +++++ crates/domain-config-driver-fs/src/store.rs | 20 +++ .../src/store/tests.rs | 31 ++++- crates/domain-config-driver-sql/src/get.rs | 74 ++++++++++- crates/domain-config-driver-sql/src/lib.rs | 18 +++ 8 files changed, 417 insertions(+), 16 deletions(-) diff --git a/crates/core/src/domain_config/backend.rs b/crates/core/src/domain_config/backend.rs index df2d25171..6190f43c1 100644 --- a/crates/core/src/domain_config/backend.rs +++ b/crates/core/src/domain_config/backend.rs @@ -126,6 +126,29 @@ pub trait DomainConfigBackend: Send + Sync { option: &'a str, ) -> Result, DomainConfigProviderError>; + /// The IDs of every domain that stores `option` in `group`. + /// + /// Backs the fan-out set of ADR 0034 §5: the assignment provider needs the + /// domains that have bound a non-global `assignment/driver` without reading + /// each domain's configuration in turn. The result order is unspecified and + /// may carry duplicates within a single driver; the caller deduplicates + /// across drivers. + /// + /// # Parameters + /// - `state`: The current service state. + /// - `group`: The group to look in. + /// - `option`: The option that must be present. + /// + /// # Returns + /// - `Result, DomainConfigProviderError>` - The matching domain + /// IDs, or an error. + async fn list_domains_with_option<'a>( + &self, + state: &ServiceState, + group: DomainConfigGroupName, + option: &'a str, + ) -> Result, DomainConfigProviderError>; + /// Merge changes into the whole configuration of a domain. /// /// Backs `PATCH /v3/domains/{domain_id}/config`: options absent from diff --git a/crates/core/src/domain_config/resolver.rs b/crates/core/src/domain_config/resolver.rs index 64445e94d..811fbf500 100644 --- a/crates/core/src/domain_config/resolver.rs +++ b/crates/core/src/domain_config/resolver.rs @@ -18,11 +18,16 @@ //! `[identity] domain_config_dir` (the `fs` driver) and the database (the `sql` //! driver, written through the config API). python-keystone layers the file //! configuration first and lets the database one override it; this module does -//! the same, gated by the two `[identity]` switches: +//! the same, gated by two switches (ADR 0034 §2): //! -//! - `domain_specific_drivers_enabled` — consult the file source at all; -//! - `domain_configurations_from_database` — consult the database source, which -//! wins where both set the same option. +//! - files — the `[domain_config] from_files` key, falling back to the +//! deprecated `[identity] domain_specific_drivers_enabled`; +//! - database — the `[domain_config] from_database` key, falling back to the +//! deprecated `[identity] domain_configurations_from_database`; the database +//! source wins where both set the same option. +//! +//! See [`effective_domain_config_sources`] for the fallback and the conflict +//! warning. //! //! The result is the raw, still-serializable [`DomainConfig`] the config API //! returns. A consumer that needs a configuration a driver can use — the @@ -32,28 +37,86 @@ //! not reach a response, which is why they are deliberately left to the //! caller. +use std::collections::HashSet; use std::sync::Arc; +use tracing::warn; + use openstack_keystone_config::Config; -use openstack_keystone_core_types::domain_config::DomainConfig; +use openstack_keystone_core_types::domain_config::{DomainConfig, DomainConfigGroupName}; use crate::domain_config::backend::DomainConfigBackend; use crate::domain_config::error::DomainConfigProviderError; use crate::keystone::ServiceState; use crate::plugin_manager::PluginManagerApi; +/// Whether the `fs` and `sql` domain-config sources are consulted, taking the +/// `[domain_config]` keys where set and the deprecated `[identity]` switches +/// otherwise (ADR 0034 §2). +/// +/// A `[domain_config]` key set to a value that contradicts an `[identity]` +/// switch the operator moved off its default logs one `WARN`; the +/// `[domain_config]` value still wins. +/// +/// # Parameters +/// - `config`: The running service configuration. +/// +/// # Returns +/// - `(bool, bool)` - `(consult files, consult database)`. +pub fn effective_domain_config_sources(config: &Config) -> (bool, bool) { + let from_files = resolve_source( + "from_files", + config.domain_config.from_files, + "domain_specific_drivers_enabled", + config.identity.domain_specific_drivers_enabled, + false, + ); + let from_database = resolve_source( + "from_database", + config.domain_config.from_database, + "domain_configurations_from_database", + config.identity.domain_configurations_from_database, + true, + ); + (from_files, from_database) +} + +/// One source's effective value: the explicit `[domain_config]` key when set, +/// the `[identity]` switch otherwise. Warns only when both were set to +/// conflicting values (the `[identity]` switch differs from its own default and +/// from the explicit key). +fn resolve_source( + key: &str, + explicit: Option, + identity_key: &str, + identity_value: bool, + identity_default: bool, +) -> bool { + match explicit { + Some(value) => { + if identity_value != identity_default && value != identity_value { + warn!( + "[domain_config] {key} = {value} overrides the conflicting \ + [identity] {identity_key} = {identity_value}" + ); + } + value + } + None => identity_value, + } +} + /// Merges a domain's file-based and database-stored configuration into one. /// /// Holds a handle to each source it is configured to use; a source that is /// switched off in `[identity]` is simply `None`, and a resolver with neither /// resolves every domain to the empty configuration. pub struct DomainConfigResolver { - /// The `fs` driver, `Some` when - /// `[identity] domain_specific_drivers_enabled` is set. + /// The `fs` driver, `Some` when [`effective_domain_config_sources`] reports + /// the file source on. file: Option>, - /// The `sql` driver, `Some` when - /// `[identity] domain_configurations_from_database` is set. Overrides the - /// file source option by option. + /// The `sql` driver, `Some` when [`effective_domain_config_sources`] reports + /// the database source on. Overrides the file source option by option. database: Option>, } @@ -62,8 +125,8 @@ impl DomainConfigResolver { /// domain-config backends. /// /// # Parameters - /// - `config`: The running service configuration; its `[identity]` - /// switches decide which sources are consulted. + /// - `config`: The running service configuration; [`effective_domain_config_sources`] + /// decides which sources are consulted. /// - `plugin_manager`: Provides the `"fs"` / `"sql"` backends by name. /// /// # Returns @@ -74,12 +137,13 @@ impl DomainConfigResolver { config: &Config, plugin_manager: &P, ) -> Result { - let file = if config.identity.domain_specific_drivers_enabled { + let (from_files, from_database) = effective_domain_config_sources(config); + let file = if from_files { Some(plugin_manager.get_domain_config_backend("fs")?.clone()) } else { None }; - let database = if config.identity.domain_configurations_from_database { + let database = if from_database { Some(plugin_manager.get_domain_config_backend("sql")?.clone()) } else { None @@ -143,6 +207,37 @@ impl DomainConfigResolver { } Ok(resolved) } + + /// Every domain that stores an `assignment/driver` binding in any active + /// source, deduplicated across sources (ADR 0034 §5). + /// + /// This is the fan-out set the assignment provider sizes its per-domain + /// backend instances against: a domain absent from it resolves to the + /// global driver and needs no dedicated instance. + /// + /// # Parameters + /// - `state`: The current service state, handed to each backend. + /// + /// # Returns + /// - `Result, DomainConfigProviderError>` - The bound domain + /// IDs in no particular order, or the first error a source returns. + pub async fn bound_domains( + &self, + state: &ServiceState, + ) -> Result, DomainConfigProviderError> { + let mut domains: HashSet = HashSet::new(); + for source in [self.file.as_ref(), self.database.as_ref()] + .into_iter() + .flatten() + { + domains.extend( + source + .list_domains_with_option(state, DomainConfigGroupName::Assignment, "driver") + .await?, + ); + } + Ok(domains.into_iter().collect()) + } } #[cfg(test)] diff --git a/crates/core/src/domain_config/resolver/tests.rs b/crates/core/src/domain_config/resolver/tests.rs index 8f772dcb0..e14bc3088 100644 --- a/crates/core/src/domain_config/resolver/tests.rs +++ b/crates/core/src/domain_config/resolver/tests.rs @@ -160,3 +160,118 @@ async fn an_error_from_a_source_propagates() { }; assert!(resolver.effective_config(&state, "d1").await.is_err()); } + +mod effective_sources { + use openstack_keystone_config::Config; + use tracing_test::traced_test; + + use super::super::effective_domain_config_sources; + + /// A config with the two `[identity]` switches and the two + /// `[domain_config]` keys set as given. + fn config( + identity_files: bool, + identity_db: bool, + from_files: Option, + from_database: Option, + ) -> Config { + let mut config = Config::default(); + config.identity.domain_specific_drivers_enabled = identity_files; + config.identity.domain_configurations_from_database = identity_db; + config.domain_config.from_files = from_files; + config.domain_config.from_database = from_database; + config + } + + #[test] + fn unset_keys_inherit_the_identity_switches_including_defaults() { + // `[identity]` defaults: files off, database on. + assert_eq!( + effective_domain_config_sources(&config(false, true, None, None)), + (false, true) + ); + assert_eq!( + effective_domain_config_sources(&config(true, false, None, None)), + (true, false) + ); + } + + #[traced_test] + #[test] + fn an_explicit_key_wins_over_the_identity_switch_at_its_default() { + // `domain_specific_drivers_enabled` still at its default `false`, so + // turning files on through `[domain_config]` is not a conflict. + assert_eq!( + effective_domain_config_sources(&config(false, true, Some(true), Some(false))), + (true, false) + ); + assert!(!logs_contain("overrides the conflicting")); + } + + #[traced_test] + #[test] + fn an_explicit_key_that_contradicts_a_moved_identity_switch_warns() { + // Operator set `domain_specific_drivers_enabled = true` and then + // `[domain_config] from_files = false`: the new key wins, with a warning. + assert_eq!( + effective_domain_config_sources(&config(true, false, Some(false), Some(true))), + (false, true) + ); + assert!(logs_contain( + "[domain_config] from_files = false overrides the conflicting [identity] domain_specific_drivers_enabled = true" + )); + assert!(logs_contain( + "[domain_config] from_database = true overrides the conflicting [identity] domain_configurations_from_database = false" + )); + } +} + +/// A backend whose `list_domains_with_option` always answers with `domains`. +fn listing_source(domains: Vec) -> Arc { + let mut mock = MockDomainConfigBackend::new(); + mock.expect_list_domains_with_option() + .returning(move |_, _, _| Ok(domains.clone())); + Arc::new(mock) +} + +#[tokio::test] +async fn bound_domains_unions_and_dedups_across_sources() { + let state = get_mocked_state(None, None).await; + let resolver = DomainConfigResolver { + file: Some(listing_source(vec!["a".to_string(), "b".to_string()])), + database: Some(listing_source(vec!["b".to_string(), "c".to_string()])), + }; + + let mut domains = resolver.bound_domains(&state).await.expect("resolvable"); + domains.sort(); + assert_eq!(domains, ["a", "b", "c"]); +} + +#[tokio::test] +async fn bound_domains_is_empty_with_no_source() { + let state = get_mocked_state(None, None).await; + let resolver = DomainConfigResolver { + file: None, + database: None, + }; + assert!( + resolver + .bound_domains(&state) + .await + .expect("resolvable") + .is_empty() + ); +} + +#[tokio::test] +async fn bound_domains_propagates_a_source_error() { + let state = get_mocked_state(None, None).await; + let mut mock = MockDomainConfigBackend::new(); + mock.expect_list_domains_with_option() + .returning(|_, _, _| Err(DomainConfigProviderError::Driver("boom".to_string()))); + let resolver = DomainConfigResolver { + file: Some(Arc::new(mock)), + database: None, + }; + assert!(resolver.bound_domains(&state).await.is_err()); +} diff --git a/crates/domain-config-driver-fs/src/lib.rs b/crates/domain-config-driver-fs/src/lib.rs index 57982ee3a..7fbf5d751 100644 --- a/crates/domain-config-driver-fs/src/lib.rs +++ b/crates/domain-config-driver-fs/src/lib.rs @@ -178,6 +178,35 @@ impl DomainConfigBackend for FsBackend { get::get_option(&self.store, &name, group, option) } + /// The IDs of every domain whose file sets `group`/`option`. + /// + /// The files are keyed by domain name, so each match is resolved back to an + /// ID through the resource provider; a name with no live domain is skipped. + async fn list_domains_with_option<'a>( + &self, + state: &ServiceState, + group: DomainConfigGroupName, + option: &'a str, + ) -> Result, DomainConfigProviderError> { + let names = self.store.domains_with_option(group, option); + 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 { + let found = resource + .find_domain_by_name(&ctx, name) + .await + .map_err(|err| DomainConfigProviderError::Driver(err.to_string()))?; + if let Some(domain) = found { + ids.push(domain.id); + } + } + Ok(ids) + } + /// Read-only: always [`DomainConfigProviderError::Readonly`]. async fn update_domain_config<'a>( &self, diff --git a/crates/domain-config-driver-fs/src/store.rs b/crates/domain-config-driver-fs/src/store.rs index 7af560554..3b9807a18 100644 --- a/crates/domain-config-driver-fs/src/store.rs +++ b/crates/domain-config-driver-fs/src/store.rs @@ -101,6 +101,26 @@ impl DomainConfigStore { self.0.get(domain_name) } + /// The names of every domain whose file sets `group`/`option`. + /// + /// Used to size the assignment fan-out set (ADR 0034 §5); the caller maps + /// the names back to domain IDs. + pub(crate) fn domains_with_option( + &self, + group: DomainConfigGroupName, + option: &str, + ) -> Vec<&str> { + self.0 + .iter() + .filter(|(_, config)| { + config + .group(group) + .is_some_and(|stored| stored.get(option).is_some()) + }) + .map(|(name, _)| name.as_str()) + .collect() + } + /// Build a store straight from parsed configurations. Test helper. #[cfg(test)] pub(crate) fn from_map(map: HashMap) -> Self { diff --git a/crates/domain-config-driver-fs/src/store/tests.rs b/crates/domain-config-driver-fs/src/store/tests.rs index d54c1c8ff..ab6e46597 100644 --- a/crates/domain-config-driver-fs/src/store/tests.rs +++ b/crates/domain-config-driver-fs/src/store/tests.rs @@ -12,6 +12,7 @@ // // SPDX-License-Identifier: Apache-2.0 +use std::collections::HashMap; use std::fs; use std::path::Path; @@ -19,7 +20,7 @@ use serde_json::json; use tempfile::tempdir; use tracing_test::traced_test; -use openstack_keystone_core_types::domain_config::DomainConfigGroupName; +use openstack_keystone_core_types::domain_config::{DomainConfig, DomainConfigGroupName}; use super::*; @@ -241,3 +242,31 @@ fn section_names_are_matched_case_sensitively() { assert!(store.get("Acme").is_none(), "[Identity] is not [identity]"); assert!(logs_contain("unsupported [section]")); } + +#[test] +fn domains_with_option_names_only_the_files_that_set_it() { + let bound = DomainConfig::from_value(json!({"assignment": {"driver": "openfga"}})) + .expect("a valid domain configuration"); + let other = DomainConfig::from_value(json!({"identity": {"driver": "ldap"}})) + .expect("a valid domain configuration"); + let store = DomainConfigStore::from_map(HashMap::from([ + ("Bound".to_string(), bound), + ("Other".to_string(), other), + ])); + + assert_eq!( + store.domains_with_option(DomainConfigGroupName::Assignment, "driver"), + ["Bound"] + ); + assert!( + store + .domains_with_option(DomainConfigGroupName::Identity, "driver") + .contains(&"Other") + ); + // A group that is set but not the queried option. + assert!( + store + .domains_with_option(DomainConfigGroupName::Assignment, "url") + .is_empty() + ); +} diff --git a/crates/domain-config-driver-sql/src/get.rs b/crates/domain-config-driver-sql/src/get.rs index 5aa031f61..b66c3b77e 100644 --- a/crates/domain-config-driver-sql/src/get.rs +++ b/crates/domain-config-driver-sql/src/get.rs @@ -205,9 +205,43 @@ pub async fn get_option( .next()) } +/// The IDs of every domain with a `whitelisted_config` row for `group`/`option`. +/// +/// Reads the readable table only: an `assignment/driver` binding is a +/// whitelisted option, never a sensitive one. +/// +/// # Parameters +/// - `db`: The database connection. +/// - `group`: The group the option is in. +/// - `option`: The option that must be present. +/// +/// # Returns +/// - `Result, DomainConfigProviderError>` - The distinct domain +/// IDs, ascending. +pub async fn list_domains_with_option( + db: &C, + group: DomainConfigGroupName, + option: &str, +) -> Result, DomainConfigProviderError> { + let rows: Vec<(String,)> = DbWhitelistedConfig::find() + .select_only() + .column(whitelisted_config::Column::DomainId) + .distinct() + .filter(whitelisted_config::Column::Group.eq(group.as_str())) + .filter(whitelisted_config::Column::Option.eq(option)) + .order_by_asc(whitelisted_config::Column::DomainId) + .into_tuple() + .all(db) + .await + .context("listing domains configured with an option")?; + Ok(rows.into_iter().map(|(domain_id,)| domain_id).collect()) +} + #[cfg(test)] mod tests { - use sea_orm::{DatabaseBackend, MockDatabase, Transaction}; + use std::collections::BTreeMap; + + use sea_orm::{DatabaseBackend, IntoMockRow, MockDatabase, Transaction, Value}; use serde_json::json; use super::*; @@ -444,4 +478,42 @@ mod tests { // Not a single statement was issued for it. assert_eq!(db.into_transaction_log(), []); } + + #[tokio::test] + async fn test_list_domains_with_option() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([vec![ + BTreeMap::from([("domain_id", Value::from("d1"))]).into_mock_row(), + BTreeMap::from([("domain_id", Value::from("d2"))]).into_mock_row(), + ]]) + .into_connection(); + + let domains = list_domains_with_option(&db, DomainConfigGroupName::Assignment, "driver") + .await + .unwrap(); + assert_eq!(domains, ["d1".to_string(), "d2".to_string()]); + + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT DISTINCT "whitelisted_config"."domain_id" FROM "whitelisted_config" WHERE "whitelisted_config"."group" = $1 AND "whitelisted_config"."option" = $2 ORDER BY "whitelisted_config"."domain_id" ASC"#, + ["assignment".into(), "driver".into()] + )] + ); + } + + #[tokio::test] + async fn test_list_domains_with_option_is_empty_when_nothing_is_bound() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([Vec::::new()]) + .into_connection(); + + assert!( + list_domains_with_option(&db, DomainConfigGroupName::Assignment, "driver") + .await + .unwrap() + .is_empty() + ); + } } diff --git a/crates/domain-config-driver-sql/src/lib.rs b/crates/domain-config-driver-sql/src/lib.rs index 24aeb287a..4e7618238 100644 --- a/crates/domain-config-driver-sql/src/lib.rs +++ b/crates/domain-config-driver-sql/src/lib.rs @@ -160,6 +160,24 @@ impl DomainConfigBackend for SqlBackend { get::get_option(&state.db.connection(), domain_id, group, option).await } + /// The IDs of every domain with a readable row for `group`/`option`. + /// + /// # Parameters + /// - `state`: The service state. + /// - `group`: The group the option is in. + /// - `option`: The option that must be present. + /// + /// # Returns + /// A `Result` containing the distinct domain IDs, or an `Error`. + async fn list_domains_with_option<'a>( + &self, + state: &ServiceState, + group: DomainConfigGroupName, + option: &'a str, + ) -> Result, DomainConfigProviderError> { + get::list_domains_with_option(&state.db.connection(), group, option).await + } + /// Merge changes into the whole configuration of a domain. /// /// # Parameters