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 <git@techassi.dev>

---------

Co-authored-by: Techassi <git@techassi.dev>

Author: xeniape

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

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,86 +432,80 @@ 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)?,
             ),
         };
     }
 
     Ok(result)
 }
 
-fn parse_cli_properties<T>(
-    resource: &<T as Configuration>::Configurable,
-    role_name: &str,
-    config: &CommonConfiguration<T>,
-) -> Result<BTreeMap<String, Option<String>>>
-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<T>(
-    resource: &<T as Configuration>::Configurable,
-    role_name: &str,
+fn parse_role_overrides<T>(
     config: &CommonConfiguration<T>,
-) -> Result<BTreeMap<String, Option<String>>>
+    property_kinds: &[PropertyNameKind],
+) -> Result<HashMap<PropertyNameKind, BTreeMap<String, Option<String>>>>
 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<T>(
-    resource: &<T as Configuration>::Configurable,
-    role_name: &str,
+fn parse_file_overrides<T>(
     config: &CommonConfiguration<T>,
     file: &str,
 ) -> Result<BTreeMap<String, Option<String>>>
 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<String, Option<String>> = 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<String, String>,
+        #[case] group_env_override: HashMap<String, String>,
+        #[case] role_cli_override: BTreeMap<String, String>,
+        #[case] group_cli_override: BTreeMap<String, String>,
+        #[case] role_conf_override: HashMap<String, HashMap<String, String>>,
+        #[case] group_conf_override: HashMap<String, HashMap<String, String>>,
+        #[case] expected: HashMap<
+            String,
+            HashMap<PropertyNameKind, BTreeMap<String, Option<String>>>,
+        >,
+    ) {
+        let role: Role<Box<TestConfig>, 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()),
                     ),
             }
         };