From 1d56e606020ecc4df434bb15fcc5be999f67ccce Mon Sep 17 00:00:00 2001 From: Maximilian Wittich <56642549+Maleware@users.noreply.github.com> Date: Thu, 22 Aug 2024 09:35:25 +0200 Subject: [PATCH 1/2] feat: Add constants for configOverrides file header and footer keys (#843) * Introduce pub consts for file_header and file_footerw * Rename conts * Moving vars to more appropriate place * Better Var names * Changelog * changelog --------- Co-authored-by: Sebastian Bernauer --- crates/stackable-operator/CHANGELOG.md | 8 ++++++-- crates/stackable-operator/src/product_config_utils.rs | 3 +++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 0b74a15de..92bc3d31a 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,13 +4,17 @@ All notable changes to this project will be documented in this file. ## [Unreleased] -- BREAKING: Replace `lazy_static` with `std::cell::LazyCell` (the original implementation was done in [#827] and reverted in [#835]) ([#840]). - ### Added - `iter::reverse_if` helper ([#838]). +- Add two new constants `CONFIG_OVERRIDE_FILE_HEADER_KEY` and `CONFIG_OVERRIDE_FILE_FOOTER_KEY` ([#843]). + +### Changed + +- BREAKING: Replace `lazy_static` with `std::cell::LazyCell` (the original implementation was done in [#827] and reverted in [#835]) ([#840]). [#838]: https://github.com/stackabletech/operator-rs/pull/838 +[#843]: https://github.com/stackabletech/operator-rs/pull/843 ## [0.73.0] - 2024-08-09 diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index 70d14f97b..13878bbd3 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -8,6 +8,9 @@ use tracing::{debug, error, warn}; use crate::role_utils::{CommonConfiguration, Role}; +pub const CONFIG_OVERRIDE_FILE_HEADER_KEY: &str = "EXPERIMENTAL_FILE_HEADER"; +pub const CONFIG_OVERRIDE_FILE_FOOTER_KEY: &str = "EXPERIMENTAL_FILE_FOOTER"; + type Result = std::result::Result; #[derive(Debug, PartialEq, Snafu)] From 004b9bda3218c2fc155981eba6e7f5acae2faa7f Mon Sep 17 00:00:00 2001 From: Xenia Date: Thu, 22 Aug 2024 11:25:51 +0200 Subject: [PATCH 2/2] fix: swap priority order of role group config and role overrides in configuration merging (#841) * change evaluation order of product configurations * add changelog * rename variable and move clone * add unit test for config override order * remove dublicate heading * Update crates/stackable-operator/src/product_config_utils.rs Co-authored-by: Techassi --------- Co-authored-by: Techassi --- crates/stackable-operator/CHANGELOG.md | 2 + .../src/product_config_utils.rs | 242 ++++++++++++++---- 2 files changed, 194 insertions(+), 50 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 92bc3d31a..444945686 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -12,8 +12,10 @@ All notable changes to this project will be documented in this file. ### Changed - BREAKING: Replace `lazy_static` with `std::cell::LazyCell` (the original implementation was done in [#827] and reverted in [#835]) ([#840]). +- BREAKING: Swap priority order of role group config and role overrides in configuration merging to prioritize overrides in general ([#841]). [#838]: https://github.com/stackabletech/operator-rs/pull/838 +[#841]: https://github.com/stackabletech/operator-rs/pull/841 [#843]: https://github.com/stackabletech/operator-rs/pull/843 ## [0.73.0] - 2024-08-09 diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index 13878bbd3..47f281614 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -348,7 +348,7 @@ fn process_validation_result( /// with the highest priority. The merge priority chain looks like this where '->' means /// "overwrites if existing or adds": /// -/// group overrides -> group config -> role overrides -> role config (TODO: -> common_config) +/// group overrides -> role overrides -> group config -> role config (TODO: -> common_config) /// /// The output is a map where the [`crate::role_utils::RoleGroup] name points to another map of /// [`product_config::types::PropertyValidationResult`] that points to the mapped configuration @@ -371,24 +371,42 @@ where { let mut result = HashMap::new(); + // Properties from the role have the lowest priority, so they are computed first... let role_properties = parse_role_config(resource, role_name, &role.config, property_kinds)?; + let role_overrides = parse_role_overrides(&role.config, property_kinds)?; // for each role group ... for (role_group_name, role_group) in &role.role_groups { - // ... compute the group properties ... + let mut role_group_properties_merged = role_properties.clone(); + + // ... compute the group properties and merge them into role properties. let role_group_properties = parse_role_config(resource, role_name, &role_group.config, property_kinds)?; - - // ... and merge them with the role properties. - let mut role_properties_copy = role_properties.clone(); for (property_kind, properties) in role_group_properties { - role_properties_copy + role_group_properties_merged .entry(property_kind) .or_default() .extend(properties); } - result.insert(role_group_name.clone(), role_properties_copy); + // ... copy role overrides and merge them into `role_group_properties_merged`. + for (property_kind, property_overrides) in role_overrides.clone() { + role_group_properties_merged + .entry(property_kind) + .or_default() + .extend(property_overrides); + } + + // ... compute the role group overrides and merge them into `role_group_properties_merged`. + let role_group_overrides = parse_role_overrides(&role_group.config, property_kinds)?; + for (property_kind, property_overrides) in role_group_overrides { + role_group_properties_merged + .entry(property_kind) + .or_default() + .extend(property_overrides); + } + + result.insert(role_group_name.clone(), role_group_properties_merged); } Ok(result) @@ -414,20 +432,19 @@ where T: Configuration, { let mut result = HashMap::new(); - for property_kind in property_kinds { match property_kind { PropertyNameKind::File(file) => result.insert( property_kind.clone(), - parse_file_properties(resource, role_name, config, file)?, + config.config.compute_files(resource, role_name, file)?, ), PropertyNameKind::Env => result.insert( property_kind.clone(), - parse_env_properties(resource, role_name, config)?, + config.config.compute_env(resource, role_name)?, ), PropertyNameKind::Cli => result.insert( property_kind.clone(), - parse_cli_properties(resource, role_name, config)?, + config.config.compute_cli(resource, role_name)?, ), }; } @@ -435,65 +452,60 @@ where Ok(result) } -fn parse_cli_properties( - resource: &::Configurable, - role_name: &str, - config: &CommonConfiguration, -) -> Result>> -where - T: Configuration, -{ - // Properties from the role have the lowest priority, so they are computed and added first... - let mut final_properties = config.config.compute_cli(resource, role_name)?; - - // ...followed by config_overrides from the role - for (key, value) in &config.cli_overrides { - final_properties.insert(key.clone(), Some(value.clone())); - } - - Ok(final_properties) -} - -fn parse_env_properties( - resource: &::Configurable, - role_name: &str, +fn parse_role_overrides( config: &CommonConfiguration, -) -> Result>> + property_kinds: &[PropertyNameKind], +) -> Result>>> where T: Configuration, { - // Properties from the role have the lowest priority, so they are computed and added first... - let mut final_properties = config.config.compute_env(resource, role_name)?; - - // ...followed by config_overrides from the role - for (key, value) in &config.env_overrides { - final_properties.insert(key.clone(), Some(value.clone())); + let mut result = HashMap::new(); + for property_kind in property_kinds { + match property_kind { + PropertyNameKind::File(file) => { + result.insert(property_kind.clone(), parse_file_overrides(config, file)?) + } + PropertyNameKind::Env => result.insert( + property_kind.clone(), + config + .env_overrides + .clone() + .into_iter() + .map(|(k, v)| (k, Some(v))) + .collect(), + ), + PropertyNameKind::Cli => result.insert( + property_kind.clone(), + config + .cli_overrides + .clone() + .into_iter() + .map(|(k, v)| (k, Some(v))) + .collect(), + ), + }; } - Ok(final_properties) + Ok(result) } -fn parse_file_properties( - resource: &::Configurable, - role_name: &str, +fn parse_file_overrides( config: &CommonConfiguration, file: &str, ) -> Result>> where T: Configuration, { - // Properties from the role have the lowest priority, so they are computed and added first... - let mut final_properties = config.config.compute_files(resource, role_name, file)?; + let mut final_overrides: BTreeMap> = BTreeMap::new(); - // ...followed by config_overrides from the role // For Conf files only process overrides that match our file name if let Some(config) = config.config_overrides.get(file) { for (key, value) in config { - final_properties.insert(key.clone(), Some(value.clone())); + final_overrides.insert(key.clone(), Some(value.clone())); } } - Ok(final_properties) + Ok(final_overrides) } #[cfg(test)] @@ -1049,6 +1061,136 @@ mod tests { assert_eq!(config, expected); } + #[rstest] + #[case( + HashMap::from([ + ("env".to_string(), ROLE_ENV_OVERRIDE.to_string()), + ]), + HashMap::from([ + ("env".to_string(), GROUP_ENV_OVERRIDE.to_string()), + ]), + BTreeMap::from([ + ("cli".to_string(), ROLE_CLI_OVERRIDE.to_string()), + ]), + BTreeMap::from([ + ("cli".to_string(), GROUP_CLI_OVERRIDE.to_string()), + ]), + HashMap::from([ + ("file".to_string(), HashMap::from([ + ("file".to_string(), ROLE_CONF_OVERRIDE.to_string()) + ])) + ]), + HashMap::from([ + ("file".to_string(), HashMap::from([ + ("file".to_string(), GROUP_CONF_OVERRIDE.to_string()) + ])) + ]), + collection ! { + ROLE_GROUP.to_string() => collection ! { + PropertyNameKind::Env => collection ! { + "env".to_string() => Some(GROUP_ENV_OVERRIDE.to_string()), + }, + PropertyNameKind::Cli => collection ! { + "cli".to_string() => Some(GROUP_CLI_OVERRIDE.to_string()), + }, + PropertyNameKind::File("file".to_string()) => collection ! { + "file".to_string() => Some(GROUP_CONF_OVERRIDE.to_string()), + } + } + } + )] + #[case( + HashMap::from([ + ("env".to_string(), ROLE_ENV_OVERRIDE.to_string()), + ]), + HashMap::from([]), + BTreeMap::from([ + ("cli".to_string(), ROLE_CLI_OVERRIDE.to_string()), + ]), + BTreeMap::from([]), + HashMap::from([ + ("file".to_string(), HashMap::from([ + ("file".to_string(), ROLE_CONF_OVERRIDE.to_string()) + ])) + ]), + HashMap::from([]), + collection ! { + ROLE_GROUP.to_string() => collection ! { + PropertyNameKind::Env => collection ! { + "env".to_string() => Some(ROLE_ENV_OVERRIDE.to_string()), + }, + PropertyNameKind::Cli => collection ! { + "cli".to_string() => Some(ROLE_CLI_OVERRIDE.to_string()), + }, + PropertyNameKind::File("file".to_string()) => collection ! { + "file".to_string() => Some(ROLE_CONF_OVERRIDE.to_string()), + } + } + } + )] + #[case( + HashMap::from([]), + HashMap::from([]), + BTreeMap::from([]), + BTreeMap::from([]), + HashMap::from([]), + HashMap::from([]), + collection ! { + ROLE_GROUP.to_string() => collection ! { + PropertyNameKind::Env => collection ! { + "env".to_string() => Some(GROUP_ENV.to_string()), + }, + PropertyNameKind::Cli => collection ! { + "cli".to_string() => Some(GROUP_CLI.to_string()), + }, + PropertyNameKind::File("file".to_string()) => collection ! { + "file".to_string() => Some(GROUP_CONFIG.to_string()), + } + } + } + )] + fn test_order_in_transform_role_to_config( + #[case] role_env_override: HashMap, + #[case] group_env_override: HashMap, + #[case] role_cli_override: BTreeMap, + #[case] group_cli_override: BTreeMap, + #[case] role_conf_override: HashMap>, + #[case] group_conf_override: HashMap>, + #[case] expected: HashMap< + String, + HashMap>>, + >, + ) { + let role: Role, TestRoleConfig> = Role { + config: build_common_config( + build_test_config(ROLE_CONFIG, ROLE_ENV, ROLE_CLI), + Some(role_conf_override), + Some(role_env_override), + Some(role_cli_override), + ), + role_config: Default::default(), + role_groups: collection! {"role_group".to_string() => RoleGroup { + replicas: Some(1), + config: build_common_config( + build_test_config(GROUP_CONFIG, GROUP_ENV, GROUP_CLI), + Some(group_conf_override), + Some(group_env_override), + Some(group_cli_override)), + }}, + }; + + let property_kinds = vec![ + PropertyNameKind::Env, + PropertyNameKind::Cli, + PropertyNameKind::File("file".to_string()), + ]; + + let config = + transform_role_to_config(&String::new(), ROLE_GROUP, &role, &property_kinds).unwrap(); + + assert_eq!(config, expected); + } + #[test] fn test_transform_role_to_config_overrides() { let role_group = "role_group"; @@ -1088,7 +1230,7 @@ mod tests { ), PropertyNameKind::Cli => collection!( - "cli".to_string() => Some(GROUP_CLI.to_string()), + "cli".to_string() => Some("cli".to_string()), ), } };