From ce932f0110ec7f4d0bf72a943a0cade52b8a471e Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 6 Dec 2024 11:48:59 +0100 Subject: [PATCH 1/3] WIP: First draft of ProductSpecificCommonConfig --- Cargo.lock | 4 +- .../src/product_config_utils.rs | 34 ++++-- crates/stackable-operator/src/role_utils.rs | 111 +++++++++++++++--- 3 files changed, 118 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1cdc26c21..222e62c07 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3277,14 +3277,14 @@ dependencies = [ [[package]] name = "stackable-versioned" -version = "0.4.1" +version = "0.5.0" dependencies = [ "stackable-versioned-macros", ] [[package]] name = "stackable-versioned-macros" -version = "0.4.1" +version = "0.5.0" dependencies = [ "convert_case", "darling", diff --git a/crates/stackable-operator/src/product_config_utils.rs b/crates/stackable-operator/src/product_config_utils.rs index 1d1a68c6a..240957751 100644 --- a/crates/stackable-operator/src/product_config_utils.rs +++ b/crates/stackable-operator/src/product_config_utils.rs @@ -167,13 +167,21 @@ pub fn config_for_role_and_group<'a>( /// - `resource` - Not used directly. It's passed on to the `Configuration::compute_*` calls. /// - `roles` - A map keyed by role names. The value is a tuple of a vector of `PropertyNameKind` /// like (Cli, Env or Files) and [`crate::role_utils::Role`] with a boxed [`Configuration`]. -pub fn transform_all_roles_to_config( +#[allow(clippy::type_complexity)] +pub fn transform_all_roles_to_config( resource: &T::Configurable, - roles: HashMap, Role)>, + roles: HashMap< + String, + ( + Vec, + Role, + ), + >, ) -> Result where T: Configuration, U: Default + JsonSchema + Serialize, + ProductSpecificCommonConfig: Default + JsonSchema + Serialize, { let mut result = HashMap::new(); @@ -359,15 +367,16 @@ fn process_validation_result( /// - `role_name` - The name of the role. /// - `role` - The role for which to transform the configuration parameters. /// - `property_kinds` - Used as "buckets" to partition the configuration properties by. -fn transform_role_to_config( +fn transform_role_to_config( resource: &T::Configurable, role_name: &str, - role: &Role, + role: &Role, property_kinds: &[PropertyNameKind], ) -> Result where T: Configuration, U: Default + JsonSchema + Serialize, + ProductSpecificCommonConfig: Default + JsonSchema + Serialize, { let mut result = HashMap::new(); @@ -422,10 +431,10 @@ where /// - `role_name` - Not used directly but passed on to the `Configuration::compute_*` calls. /// - `config` - The configuration properties to partition. /// - `property_kinds` - The "buckets" used to partition the configuration properties. -fn parse_role_config( +fn parse_role_config( resource: &::Configurable, role_name: &str, - config: &CommonConfiguration, + config: &CommonConfiguration, property_kinds: &[PropertyNameKind], ) -> Result>>> where @@ -452,8 +461,8 @@ where Ok(result) } -fn parse_role_overrides( - config: &CommonConfiguration, +fn parse_role_overrides( + config: &CommonConfiguration, property_kinds: &[PropertyNameKind], ) -> Result>>> where @@ -489,8 +498,8 @@ where Ok(result) } -fn parse_file_overrides( - config: &CommonConfiguration, +fn parse_file_overrides( + config: &CommonConfiguration, file: &str, ) -> Result>> where @@ -522,7 +531,7 @@ mod tests { } use super::*; - use crate::role_utils::{Role, RoleGroup}; + use crate::role_utils::{GenericProductSpecificCommonConfig, Role, RoleGroup}; use k8s_openapi::api::core::v1::PodTemplateSpec; use rstest::*; use std::collections::HashMap; @@ -610,13 +619,14 @@ mod tests { config_overrides: Option>>, env_overrides: Option>, cli_overrides: Option>, - ) -> CommonConfiguration> { + ) -> CommonConfiguration, GenericProductSpecificCommonConfig> { CommonConfiguration { config: test_config.unwrap_or_default(), config_overrides: config_overrides.unwrap_or_default(), env_overrides: env_overrides.unwrap_or_default(), cli_overrides: cli_overrides.unwrap_or_default(), pod_overrides: PodTemplateSpec::default(), + product_specific_common_config: GenericProductSpecificCommonConfig::default(), } } diff --git a/crates/stackable-operator/src/role_utils.rs b/crates/stackable-operator/src/role_utils.rs index 04ecd2593..370a9b82c 100644 --- a/crates/stackable-operator/src/role_utils.rs +++ b/crates/stackable-operator/src/role_utils.rs @@ -89,7 +89,7 @@ use crate::{ commons::pdb::PdbConfig, config::{ fragment::{self, FromFragment}, - merge::Merge, + merge::{Atomic, Merge}, }, product_config_utils::Configuration, utils::crds::raw_object_schema, @@ -99,18 +99,27 @@ use k8s_openapi::api::core::v1::PodTemplateSpec; use kube::{runtime::reflector::ObjectRef, Resource}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use snafu::{OptionExt, Snafu}; + +#[derive(Debug, Snafu)] +pub enum Error { + #[snafu(display("missing roleGroup {role_group:?}"))] + MissingRoleGroup { role_group: String }, +} #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde( rename_all = "camelCase", - bound(deserialize = "T: Default + Deserialize<'de>") + bound( + deserialize = "T: Default + Deserialize<'de>, ProductSpecificCommonConfig: Default + Deserialize<'de>" + ) )] -pub struct CommonConfiguration { +pub struct CommonConfiguration { #[serde(default)] // We can't depend on T being `Default`, since that trait is not object-safe // We only need to generate schemas for fully specified types, but schemars_derive // does not support specifying custom bounds. - #[schemars(default = "config_schema_default")] + #[schemars(default = "Self::default_config")] pub config: T, /// The `configOverrides` can be used to configure properties in product config files @@ -144,10 +153,42 @@ pub struct CommonConfiguration { #[serde(default)] #[schemars(schema_with = "raw_object_schema")] pub pod_overrides: PodTemplateSpec, + + // No docs needed, as we flatten this struct. + // + // This field is product-specific and can contain e.g. jvmArgumentOverrides. + // It is not accessible by operators, please use to read the value + #[serde(flatten, default)] + pub(crate) product_specific_common_config: ProductSpecificCommonConfig, +} + +impl CommonConfiguration { + fn default_config() -> serde_json::Value { + serde_json::json!({}) + } +} + +#[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] +pub struct GenericProductSpecificCommonConfig {} + +#[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize, Merge)] +#[merge(path_overrides(merge = "crate::config::merge"))] +#[serde(rename_all = "camelCase")] +pub struct JavaCommonConfig { + /// Allows overriding JVM arguments. + /// + // TODO: Docs + #[serde(default)] + pub jvm_argument_overrides: BTreeMap, } -fn config_schema_default() -> serde_json::Value { - serde_json::json!({}) +#[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] +pub struct JvmArgument(pub Option); +impl Atomic for JvmArgument {} +impl Merge for JvmArgument { + fn merge(&mut self, defaults: &Self) { + *self = defaults.clone(); + } } /// This struct represents a role - e.g. HDFS datanodes or Trino workers. It has a key-value-map containing @@ -168,33 +209,46 @@ fn config_schema_default() -> serde_json::Value { // However, product-operators can define their own - custom - struct and use that here. #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] -pub struct Role -where +pub struct Role< + T, + U = GenericRoleConfig, + ProductSpecificCommonConfig = GenericProductSpecificCommonConfig, +> where // Don't remove this trait bounds!!! // We don't know why, but if you remove either of them, the generated default value in the CRDs will // be missing! U: Default + JsonSchema + Serialize, + ProductSpecificCommonConfig: Default + JsonSchema + Serialize, { - #[serde(flatten, bound(deserialize = "T: Default + Deserialize<'de>"))] - pub config: CommonConfiguration, + #[serde( + flatten, + bound( + deserialize = "T: Default + Deserialize<'de>, ProductSpecificCommonConfig: Deserialize<'de>" + ) + )] + pub config: CommonConfiguration, #[serde(default)] pub role_config: U, - pub role_groups: HashMap>, + pub role_groups: HashMap>, } -impl Role +impl Role where T: Configuration + 'static, U: Default + JsonSchema + Serialize, + ProductSpecificCommonConfig: Default + JsonSchema + Serialize + Clone + Merge, { /// This casts a generic struct implementing [`crate::product_config_utils::Configuration`] /// and used in [`Role`] into a Box of a dynamically dispatched /// [`crate::product_config_utils::Configuration`] Trait. This is required to use the generic /// [`Role`] with more than a single generic struct. For example different roles most likely /// have different structs implementing Configuration. - pub fn erase(self) -> Role>, U> { + pub fn erase( + self, + ) -> Role>, U, ProductSpecificCommonConfig> + { Role { config: CommonConfiguration { config: Box::new(self.config.config) @@ -203,6 +257,7 @@ where env_overrides: self.config.env_overrides, cli_overrides: self.config.cli_overrides, pod_overrides: self.config.pod_overrides, + product_specific_common_config: self.config.product_specific_common_config, }, role_config: self.role_config, role_groups: self @@ -219,6 +274,9 @@ where env_overrides: group.config.env_overrides, cli_overrides: group.config.cli_overrides, pod_overrides: group.config.pod_overrides, + product_specific_common_config: group + .config + .product_specific_common_config, }, replicas: group.replicas, }, @@ -227,6 +285,23 @@ where .collect(), } } + + pub fn merged_product_specific_common_config( + &self, + role_group: &str, + ) -> Result { + let from_role = &self.config.product_specific_common_config; + let mut merged = self + .role_groups + .get(role_group) + .with_context(|| MissingRoleGroupSnafu { role_group })? + .config + .product_specific_common_config + .clone(); + merged.merge(from_role); + + Ok(merged) + } } /// This is a product-agnostic RoleConfig, which is sufficient for most of the products. @@ -246,15 +321,17 @@ pub struct EmptyRoleConfig {} #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde( rename_all = "camelCase", - bound(deserialize = "T: Default + Deserialize<'de>") + bound( + deserialize = "T: Default + Deserialize<'de>, ProductSpecificCommonConfig: Default + Deserialize<'de>" + ) )] -pub struct RoleGroup { +pub struct RoleGroup { #[serde(flatten)] - pub config: CommonConfiguration, + pub config: CommonConfiguration, pub replicas: Option, } -impl RoleGroup { +impl RoleGroup { pub fn validate_config( &self, role: &Role, From 3462c9e9b670d17b07150e4a4a617bb944bb1b5a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 6 Dec 2024 14:01:46 +0100 Subject: [PATCH 2/3] v2: Allow deletion of operator generated arguments --- crates/stackable-operator/src/role_utils.rs | 223 +++++++++++++++++++- 1 file changed, 217 insertions(+), 6 deletions(-) diff --git a/crates/stackable-operator/src/role_utils.rs b/crates/stackable-operator/src/role_utils.rs index 370a9b82c..ffbce666f 100644 --- a/crates/stackable-operator/src/role_utils.rs +++ b/crates/stackable-operator/src/role_utils.rs @@ -89,7 +89,7 @@ use crate::{ commons::pdb::PdbConfig, config::{ fragment::{self, FromFragment}, - merge::{Atomic, Merge}, + merge::Merge, }, product_config_utils::Configuration, utils::crds::raw_object_schema, @@ -178,16 +178,38 @@ pub struct JavaCommonConfig { /// Allows overriding JVM arguments. /// // TODO: Docs + // Use [`JavaCommonConfig::effective_jvm_config`] to retrieve the effective JVM arguments! #[serde(default)] pub jvm_argument_overrides: BTreeMap, } -#[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] -pub struct JvmArgument(pub Option); -impl Atomic for JvmArgument {} +impl JavaCommonConfig { + /// Returns all arguments that should be passed to the JVM. + /// + /// Please note that the values of the [`BTreeMap`] are [`Option`]. A value of [`None`] + /// expresses that the given argument is just a flag without any argument. + pub fn effective_jvm_config(&self) -> BTreeMap> { + self.jvm_argument_overrides + .iter() + .filter_map(|(k, v)| match v { + JvmArgument::Argument(argument) => Some((k.to_owned(), Some(argument.to_owned()))), + JvmArgument::Flag {} => Some((k.to_owned(), None)), + JvmArgument::Remove {} => None, + }) + .collect() + } +} + +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub enum JvmArgument { + Argument(String), + Flag {}, + Remove {}, +} impl Merge for JvmArgument { - fn merge(&mut self, defaults: &Self) { - *self = defaults.clone(); + fn merge(&mut self, _defaults: &Self) { + // We ignore whatever was in there before, later values override earlier ones } } @@ -373,3 +395,192 @@ impl Display for RoleGroupRef { )) } } + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use crate::{config::merge::Merge, role_utils::JvmArgument}; + + use super::JavaCommonConfig; + + #[test] + fn test_parse_java_common_config() { + let input = r#" + jvmArgumentOverrides: + -XX:+UseG1GC: + flag: {} + -Dhttps.proxyHost: + argument: proxy.my.corp + -XX:+ExitOnOutOfMemoryError: + remove: {} + "#; + let deserializer = serde_yaml::Deserializer::from_str(input); + let java_common_config: JavaCommonConfig = + serde_yaml::with::singleton_map_recursive::deserialize(deserializer).unwrap(); + + assert_eq!( + java_common_config, + JavaCommonConfig { + jvm_argument_overrides: BTreeMap::from([ + ("-XX:+UseG1GC".to_owned(), JvmArgument::Flag {}), + ( + "-Dhttps.proxyHost".to_owned(), + JvmArgument::Argument("proxy.my.corp".to_owned()) + ), + ( + "-XX:+ExitOnOutOfMemoryError".to_owned(), + JvmArgument::Remove {} + ) + ]) + } + ); + } + + #[test] + fn test_merge_java_common_config() { + // The operator generates some JVM arguments + let operator_generated = JavaCommonConfig { + jvm_argument_overrides: BTreeMap::from([ + // Some flags + ("-Xms34406m".to_owned(), JvmArgument::Flag {}), + ("-Xmx34406m".to_owned(), JvmArgument::Flag {}), + ("-XX:+UseG1GC".to_owned(), JvmArgument::Flag {}), + ( + "-XX:+ExitOnOutOfMemoryError".to_owned(), + JvmArgument::Flag {}, + ), + // And some arguments + ( + "-Djava.protocol.handler.pkgs".to_owned(), + JvmArgument::Argument("sun.net.www.protocol".to_owned()), + ), + ( + "-Dsun.net.http.allowRestrictedHeaders".to_owned(), + JvmArgument::Argument(true.to_string()), + ), + ( + "-Djava.security.properties".to_owned(), + JvmArgument::Argument("/stackable/nifi/conf/security.properties".to_owned()), + ), + ]), + }; + + // Let's say we want to set some additional HTTP Proxy and IPv4 settings + // And we don't like the garbage collector for some reason... + let role = serde_yaml::Deserializer::from_str( + r#" + jvmArgumentOverrides: + -XX:+UseG1GC: + remove: {} + -Dhttps.proxyHost: + argument: proxy.my.corp + -Dhttps.proxyPort: + argument: "8080" + -Djava.net.preferIPv4Stack: + argument: "true" + "#, + ); + let role: JavaCommonConfig = + serde_yaml::with::singleton_map_recursive::deserialize(role).unwrap(); + + // For the roleGroup, let's say we need a different memory config. + // For that to work we first remove the flags generated by the operator and add our own. + // Also we override the proxy port to test that the roleGroup config takes precedence over the role config. + let role_group = serde_yaml::Deserializer::from_str( + r#" + jvmArgumentOverrides: + # We need more memory! + -Xmx34406m: + remove: {} + -Xmx40000m: + flag: {} + -Dhttps.proxyPort: + argument: "1234" + "#, + ); + let role_group: JavaCommonConfig = + serde_yaml::with::singleton_map_recursive::deserialize(role_group).unwrap(); + + let mut merged = role_group; + merged.merge(&role); + merged.merge(&operator_generated); + + assert_eq!( + merged, + JavaCommonConfig { + jvm_argument_overrides: BTreeMap::from([ + // Flags + ("-Xms34406m".to_owned(), JvmArgument::Flag {}), + // Note the different memory config from the roleGroup! + ("-Xmx34406m".to_owned(), JvmArgument::Remove {}), + ("-Xmx40000m".to_owned(), JvmArgument::Flag {}), + // Note that the "-XX:+UseG1GC" flag is removed! + ("-XX:+UseG1GC".to_owned(), JvmArgument::Remove {}), + ( + "-XX:+ExitOnOutOfMemoryError".to_owned(), + JvmArgument::Flag {}, + ), + // Arguments + ( + "-Djava.protocol.handler.pkgs".to_owned(), + JvmArgument::Argument("sun.net.www.protocol".to_owned()), + ), + ( + "-Dsun.net.http.allowRestrictedHeaders".to_owned(), + JvmArgument::Argument(true.to_string()), + ), + ( + "-Djava.security.properties".to_owned(), + JvmArgument::Argument( + "/stackable/nifi/conf/security.properties".to_owned() + ), + ), + ( + "-Dhttps.proxyHost".to_owned(), + JvmArgument::Argument("proxy.my.corp".to_owned()), + ), + ( + "-Dhttps.proxyPort".to_owned(), + // Note: This is overridden by the roleGroup + JvmArgument::Argument("1234".to_owned()), + ), + ( + "-Djava.net.preferIPv4Stack".to_owned(), + JvmArgument::Argument("true".to_owned()), + ), + ]) + } + ); + + assert_eq!( + merged.effective_jvm_config(), + BTreeMap::from([ + ("-Xms34406m".to_owned(), None), + ("-Xmx40000m".to_owned(), None), + ("-XX:+ExitOnOutOfMemoryError".to_owned(), None), + ( + "-Djava.protocol.handler.pkgs".to_owned(), + Some("sun.net.www.protocol".to_owned()) + ), + ( + "-Dsun.net.http.allowRestrictedHeaders".to_owned(), + Some("true".to_owned()) + ), + ( + "-Djava.security.properties".to_owned(), + Some("/stackable/nifi/conf/security.properties".to_owned()) + ), + ( + "-Dhttps.proxyHost".to_owned(), + Some("proxy.my.corp".to_owned()) + ), + ("-Dhttps.proxyPort".to_owned(), Some("1234".to_owned())), + ( + "-Djava.net.preferIPv4Stack".to_owned(), + Some("true".to_owned()) + ), + ]) + ); + } +} From a7ae846f88c2cc0e4f0f41f2664f6c807f702b4c Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 9 Dec 2024 13:17:27 +0100 Subject: [PATCH 3/3] refactor!: Make field private --- crates/stackable-operator/src/role_utils.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/role_utils.rs b/crates/stackable-operator/src/role_utils.rs index ffbce666f..1ac5d5637 100644 --- a/crates/stackable-operator/src/role_utils.rs +++ b/crates/stackable-operator/src/role_utils.rs @@ -180,10 +180,16 @@ pub struct JavaCommonConfig { // TODO: Docs // Use [`JavaCommonConfig::effective_jvm_config`] to retrieve the effective JVM arguments! #[serde(default)] - pub jvm_argument_overrides: BTreeMap, + jvm_argument_overrides: BTreeMap, } impl JavaCommonConfig { + pub fn new(jvm_argument_overrides: BTreeMap) -> Self { + Self { + jvm_argument_overrides, + } + } + /// Returns all arguments that should be passed to the JVM. /// /// Please note that the values of the [`BTreeMap`] are [`Option`]. A value of [`None`]