Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 33 additions & 2 deletions crates/config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.<name>.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
}

Expand Down Expand Up @@ -656,8 +663,13 @@ impl ConfigManager {
let mut watcher: RecommendedWatcher =
notify::recommended_watcher(move |res: notify::Result<notify::Event>| {
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.<name>.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.
Expand Down Expand Up @@ -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() {
Expand Down
4 changes: 2 additions & 2 deletions crates/core-types/src/domain_config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<DomainConfigGroupName, DomainConfigGroup>,
Expand Down
20 changes: 20 additions & 0 deletions crates/core/src/assignment/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<dyn DomainConfigBackend>) -> 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
Expand Down Expand Up @@ -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.<name>.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()
Expand Down
61 changes: 61 additions & 0 deletions crates/core/src/assignment/service/tests/reload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.<name>.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<dyn DomainConfigBackend> = 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<dyn DomainConfigBackend> = 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<dyn AssignmentBackend> = Arc::new(MockAssignmentBackend::default());
Expand Down
21 changes: 21 additions & 0 deletions crates/core/src/domain_config/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -149,6 +150,26 @@ pub trait DomainConfigBackend: Send + Sync {
option: &'a str,
) -> Result<Vec<String>, 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.<name>.conf` takes effect
/// without a restart.
///
/// # Parameters
/// - `_config`: The reloaded service configuration.
///
/// # Returns
/// - `Result<bool, DomainConfigProviderError>` - `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<bool, DomainConfigProviderError> {
Ok(false)
}

/// Merge changes into the whole configuration of a domain.
///
/// Backs `PATCH /v3/domains/{domain_id}/config`: options absent from
Expand Down
2 changes: 2 additions & 0 deletions crates/domain-config-driver-fs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]
Expand Down
Loading
Loading