From 669bb90b0b58d85c89c8f5884335640344056c56 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 4 Oct 2023 17:11:21 +0200 Subject: [PATCH 1/6] feat: Make roleConfig customizable --- src/product_config_utils.rs | 73 ++++++++++++++++++++++--------------- src/role_utils.rs | 43 +++++++++++++++------- 2 files changed, 73 insertions(+), 43 deletions(-) diff --git a/src/product_config_utils.rs b/src/product_config_utils.rs index 4e014e095..52897d22d 100644 --- a/src/product_config_utils.rs +++ b/src/product_config_utils.rs @@ -3,6 +3,8 @@ use crate::error::*; use crate::role_utils::{CommonConfiguration, Role}; use product_config::types::PropertyNameKind; use product_config::{ProductConfigManager, PropertyValidationResult}; +use schemars::JsonSchema; +use serde::Serialize; use std::collections::{BTreeMap, HashMap}; use thiserror::Error; use tracing::{debug, error, warn}; @@ -147,12 +149,13 @@ 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( +pub fn transform_all_roles_to_config( resource: &T::Configurable, - roles: HashMap, Role)>, + roles: HashMap, Role)>, ) -> ConfigResult where T: Configuration, + U: Default + JsonSchema + Serialize, { let mut result = HashMap::new(); @@ -340,14 +343,15 @@ 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], ) -> ConfigResult where T: Configuration, + U: Default + JsonSchema + Serialize, { let mut result = HashMap::new(); @@ -490,7 +494,7 @@ mod tests { } use super::*; - use crate::role_utils::{Role, RoleConfig, RoleGroup}; + use crate::role_utils::{Role, RoleGroup}; use k8s_openapi::api::core::v1::PodTemplateSpec; use rstest::*; use std::collections::HashMap; @@ -521,6 +525,9 @@ mod tests { pub cli: Option, } + #[derive(Clone, Default, Debug, PartialEq, JsonSchema, Serialize)] + struct TestRoleConfig {} + impl Configuration for TestConfig { type Configurable = String; @@ -607,7 +614,7 @@ mod tests { group_config: bool, role_overrides: bool, group_overrides: bool, - ) -> Role> { + ) -> Role, TestRoleConfig> { let role_group = ROLE_GROUP.to_string(); let file_name = "foo.conf"; @@ -619,7 +626,7 @@ mod tests { build_env_override(ROLE_ENV_OVERRIDE), build_cli_override(ROLE_CLI_OVERRIDE), ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -637,7 +644,7 @@ mod tests { build_env_override(ROLE_ENV_OVERRIDE), build_cli_override(ROLE_CLI_OVERRIDE), ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -652,7 +659,7 @@ mod tests { None, None, ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -670,7 +677,7 @@ mod tests { None, None, ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -688,7 +695,7 @@ mod tests { build_env_override(ROLE_ENV_OVERRIDE), build_cli_override(ROLE_CLI_OVERRIDE), ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -706,7 +713,7 @@ mod tests { build_env_override(ROLE_ENV_OVERRIDE), build_cli_override(ROLE_CLI_OVERRIDE), ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), @@ -720,7 +727,7 @@ mod tests { None, None, ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -739,7 +746,7 @@ mod tests { None, None, ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), @@ -753,7 +760,7 @@ mod tests { build_env_override(ROLE_ENV_OVERRIDE), build_cli_override(ROLE_CLI_OVERRIDE), ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -771,7 +778,7 @@ mod tests { build_env_override(ROLE_ENV_OVERRIDE), build_cli_override(ROLE_CLI_OVERRIDE), ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -784,7 +791,7 @@ mod tests { }, (false, true, false, true) => Role { config: CommonConfiguration::default(), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -797,7 +804,7 @@ mod tests { }, (false, true, false, false) => Role { config: CommonConfiguration::default(), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -815,7 +822,7 @@ mod tests { build_env_override(ROLE_ENV_OVERRIDE), build_cli_override(ROLE_CLI_OVERRIDE), ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -833,7 +840,7 @@ mod tests { build_env_override(ROLE_ENV_OVERRIDE), build_cli_override(ROLE_CLI_OVERRIDE), ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), @@ -842,7 +849,7 @@ mod tests { }, (false, false, false, true) => Role { config: CommonConfiguration::default(), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: build_common_config( @@ -855,7 +862,7 @@ mod tests { }, (false, false, false, false) => Role { config: CommonConfiguration::default(), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group => RoleGroup { replicas: Some(1), config: CommonConfiguration::default(), @@ -1055,7 +1062,7 @@ mod tests { // should override build_cli_override("cli"), ), - role_config: RoleConfig::default(), + role_config: TestRoleConfig::default(), role_groups: collection! {role_group.to_string() => RoleGroup { replicas: Some(1), config: build_common_config( @@ -1107,7 +1114,11 @@ mod tests { let role_group_2 = "role_group_2"; let file_name = "foo.bar"; - let roles: HashMap, Role>)> = collection! { + #[allow(clippy::type_complexity)] + let roles: HashMap< + String, + (Vec, Role, TestRoleConfig>), + > = collection! { role_1.to_string() => (vec![PropertyNameKind::File(file_name.to_string()), PropertyNameKind::Env], Role { config: build_common_config( build_test_config(ROLE_CONFIG, ROLE_ENV, ROLE_CLI), @@ -1115,7 +1126,7 @@ mod tests { None, None, ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group_1.to_string() => RoleGroup { replicas: Some(1), config: build_common_config( @@ -1144,7 +1155,7 @@ mod tests { None, None, ), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! {role_group_1.to_string() => RoleGroup { replicas: Some(1), config: build_common_config( @@ -1203,10 +1214,14 @@ mod tests { let pc_bad_version = "pc_bad_version"; let pc_bad_version_value = "pc_bad_version_value"; - let roles: HashMap, Role>)> = collection! { + #[allow(clippy::type_complexity)] + let roles: HashMap< + String, + (Vec, Role, TestRoleConfig>), + > = collection! { role_1.to_string() => (vec![PropertyNameKind::File(file_name.to_string()), PropertyNameKind::Env], Role { config: CommonConfiguration::default(), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! { role_group_1.to_string() => RoleGroup { replicas: Some(1), @@ -1222,7 +1237,7 @@ mod tests { ), role_2.to_string() => (vec![PropertyNameKind::File(file_name.to_string())], Role { config: CommonConfiguration::default(), - role_config: RoleConfig::default(), + role_config: Default::default(), role_groups: collection! { role_group_2.to_string() => RoleGroup { replicas: Some(1), diff --git a/src/role_utils.rs b/src/role_utils.rs index a90227479..c19b45f3f 100644 --- a/src/role_utils.rs +++ b/src/role_utils.rs @@ -165,28 +165,33 @@ fn config_schema_default() -> serde_json::Value { serde_json::json!({}) } -#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde( - rename_all = "camelCase", - bound(deserialize = "T: Default + Deserialize<'de>") -)] -pub struct Role { - #[serde(flatten)] +/// `U` is the `RoleConfig`, which is specific to the role and can *not* be overridden at roleGroup level. +/// It is recommended to use either [`GenericRoleConfig`] or [`EmptyRoleConfig`]. +#[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct Role +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, +{ + #[serde(flatten, bound(deserialize = "T: Default + Deserialize<'de>"))] pub config: CommonConfiguration, #[serde(default)] - pub role_config: RoleConfig, + pub role_config: U, pub role_groups: HashMap>, } -impl Role { +impl Role { /// 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>> { + pub fn erase(self) -> Role>, U> { Role { config: CommonConfiguration { config: Box::new(self.config.config) @@ -196,7 +201,7 @@ impl Role { cli_overrides: self.config.cli_overrides, pod_overrides: self.config.pod_overrides, }, - role_config: RoleConfig::default(), + role_config: self.role_config, role_groups: self .role_groups .into_iter() @@ -222,13 +227,22 @@ impl Role { } } +/// This is a generic RoleConfig, which fulfills the needs for most of the product. Currently it contains: +/// +/// 1. `podDisruptionBudget` to configure the created PDBs. #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] -pub struct RoleConfig { +pub struct GenericRoleConfig { #[serde(default)] pub pod_disruption_budget: PdbConfig, } +/// This is a generic RoleConfig, with nothing in it. It is used e.g. by products that have nothing configurable +/// at role level. +#[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct EmptyRoleConfig {} + #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde( rename_all = "camelCase", @@ -244,14 +258,15 @@ pub struct RoleGroup { } impl RoleGroup { - pub fn validate_config( + pub fn validate_config( &self, - role: &Role, + role: &Role, default_config: &T, ) -> Result where C: FromFragment, T: Merge + Clone, + U: Default + JsonSchema + Serialize, { let mut role_config = role.config.config.clone(); role_config.merge(default_config); From 20f0fd5949393aa85e9a776bf081b8387206a222 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 5 Oct 2023 08:17:26 +0200 Subject: [PATCH 2/6] improve docs --- src/role_utils.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/role_utils.rs b/src/role_utils.rs index c19b45f3f..ad98d8c70 100644 --- a/src/role_utils.rs +++ b/src/role_utils.rs @@ -165,8 +165,17 @@ fn config_schema_default() -> serde_json::Value { serde_json::json!({}) } -/// `U` is the `RoleConfig`, which is specific to the role and can *not* be overridden at roleGroup level. -/// It is recommended to use either [`GenericRoleConfig`] or [`EmptyRoleConfig`]. +/// This struct represents a role - e.g. HDFS datanodes or Trino workers. It has a [`HashMap`] containing +/// all the roleGroups that are part of this role. Additionally, there is a `config`, which is configurable +/// at the role *and* roleGroup level. Everything at roleGroup level is merged on top of what is configured +/// on role level using the [`Merge`] trait. There is also a second form of config, which can only be configured +/// at role level, the `roleConfig`. +/// +/// [`T`] here is the `config` shared between role and roleGroup. +/// +/// [`U`] here is the `roleConfig` only available on the role. It defaults to [`GenericRoleConfig`], as this +/// is sufficient for most products. There are some exceptions, where e.g. [`EmptyRoleConfig`] is used. +/// 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 @@ -227,7 +236,8 @@ impl Role } } -/// This is a generic RoleConfig, which fulfills the needs for most of the product. Currently it contains: +/// This is a product-agnostic RoleConfig, which fulfills the needs for most of the product. +/// Currently it contains: /// /// 1. `podDisruptionBudget` to configure the created PDBs. #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -237,8 +247,8 @@ pub struct GenericRoleConfig { pub pod_disruption_budget: PdbConfig, } -/// This is a generic RoleConfig, with nothing in it. It is used e.g. by products that have nothing configurable -/// at role level. +/// This is a product-agnostic RoleConfig, with nothing in it. It is used e.g. by products that have +/// nothing configurable at role level. #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub struct EmptyRoleConfig {} From 37203ec871e7ee53f920842c3d2cd73db63536fa Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 5 Oct 2023 08:22:14 +0200 Subject: [PATCH 3/6] refactor --- src/role_utils.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/role_utils.rs b/src/role_utils.rs index ad98d8c70..b70c84dd5 100644 --- a/src/role_utils.rs +++ b/src/role_utils.rs @@ -194,7 +194,11 @@ where pub role_groups: HashMap>, } -impl Role { +impl Role +where + T: Configuration + 'static, + U: Default + JsonSchema + Serialize, +{ /// 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 From d0d6913369c3be2d3fcccf3f076fc728144b4a83 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 5 Oct 2023 08:51:38 +0200 Subject: [PATCH 4/6] fix docs --- src/role_utils.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/role_utils.rs b/src/role_utils.rs index b70c84dd5..ce1fea9ea 100644 --- a/src/role_utils.rs +++ b/src/role_utils.rs @@ -171,9 +171,9 @@ fn config_schema_default() -> serde_json::Value { /// on role level using the [`Merge`] trait. There is also a second form of config, which can only be configured /// at role level, the `roleConfig`. /// -/// [`T`] here is the `config` shared between role and roleGroup. +/// `T` here is the `config` shared between role and roleGroup. /// -/// [`U`] here is the `roleConfig` only available on the role. It defaults to [`GenericRoleConfig`], as this +/// `U` here is the `roleConfig` only available on the role. It defaults to [`GenericRoleConfig`], as this /// is sufficient for most products. There are some exceptions, where e.g. [`EmptyRoleConfig`] is used. /// However, product-operators can define their own - custom - struct and use that here. #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] From cd44b432645179f01f2b99dd532da4b20f6b2707 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 5 Oct 2023 09:44:37 +0200 Subject: [PATCH 5/6] changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cd0646ec..451a7134e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Changed + +- Make roleConfig customizable by making the `Role` struct generic over the `roleConfig` ([#661]). + +[#661]: https://github.com/stackabletech/operator-rs/pull/661 + ## [0.51.1] - 2023-09-26 ### Fixed @@ -14,6 +20,8 @@ All notable changes to this project will be documented in this file. ## [0.51.0] - 2023-09-25 +### Added + - Add `PdbConfig` struct and `PodDisruptionBudgetBuilder` ([#653]). [#653]: https://github.com/stackabletech/operator-rs/pull/653 From 3a0d01a386acfa342a92a5ffe37e03116067ad71 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 5 Oct 2023 09:45:58 +0200 Subject: [PATCH 6/6] Add breaking marker --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 451a7134e..549a38837 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. ### Changed -- Make roleConfig customizable by making the `Role` struct generic over the `roleConfig` ([#661]). +- BREAKING: Make roleConfig customizable by making the `Role` struct generic over the `roleConfig` ([#661]). [#661]: https://github.com/stackabletech/operator-rs/pull/661 @@ -107,7 +107,7 @@ All notable changes to this project will be documented in this file. ### Changed -- [BREAKING] ProductImageSelection now defaults `stackableVersion` to +- BREAKING: ProductImageSelection now defaults `stackableVersion` to operator version ([#619]). - Default `pullPolicy` to operator `Always` ([#619]). - BREAKING: Assume that the Vector executable is located in a directory @@ -215,7 +215,7 @@ All notable changes to this project will be documented in this file. ### Added -- [BREAKING]: Added ownerreferences and labels to `build_rbac_resources` ([#579]). +- BREAKING: Added ownerreferences and labels to `build_rbac_resources` ([#579]). [#579]: https://github.com/stackabletech/operator-rs/pull/579 @@ -310,7 +310,7 @@ All notable changes to this project will be documented in this file. ### Changed - Deprecated `to_java_heap` and `to_java_heap_value` ([#544]). -- [BREAKING]: For all products using logback. Added additional optional parameter to `create_logback_config()` to supply custom configurations not covered via the standard log configuration ([#546]). +- BREAKING: For all products using logback. Added additional optional parameter to `create_logback_config()` to supply custom configurations not covered via the standard log configuration ([#546]). [#544]: https://github.com/stackabletech/operator-rs/pull/544 [#546]: https://github.com/stackabletech/operator-rs/pull/546