Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Make roleConfig customizable #661

Merged
merged 6 commits into from
Oct 5, 2023
Merged

feat: Make roleConfig customizable #661

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
669bb90
feat: Make roleConfig customizable
sbernauer Oct 4, 2023
20f0fd5
improve docs
sbernauer Oct 5, 2023
37203ec
refactor
sbernauer Oct 5, 2023
d0d6913
fix docs
sbernauer Oct 5, 2023
cd44b43
changelog
sbernauer Oct 5, 2023
3a0d01a
Add breaking marker
sbernauer Oct 5, 2023
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
73 changes: 44 additions & 29 deletions src/product_config_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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<T>(
pub fn transform_all_roles_to_config<T, U>(
resource: &T::Configurable,
roles: HashMap<String, (Vec<PropertyNameKind>, Role<T>)>,
roles: HashMap<String, (Vec<PropertyNameKind>, Role<T, U>)>,
) -> ConfigResult<RoleConfigByPropertyKind>
where
T: Configuration,
U: Default + JsonSchema + Serialize,
{
let mut result = HashMap::new();

Expand Down Expand Up @@ -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<T>(
fn transform_role_to_config<T, U>(
resource: &T::Configurable,
role_name: &str,
role: &Role<T>,
role: &Role<T, U>,
property_kinds: &[PropertyNameKind],
) -> ConfigResult<RoleGroupConfigByPropertyKind>
where
T: Configuration,
U: Default + JsonSchema + Serialize,
{
let mut result = HashMap::new();

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -521,6 +525,9 @@ mod tests {
pub cli: Option<String>,
}

#[derive(Clone, Default, Debug, PartialEq, JsonSchema, Serialize)]
struct TestRoleConfig {}

impl Configuration for TestConfig {
type Configurable = String;

Expand Down Expand Up @@ -607,7 +614,7 @@ mod tests {
group_config: bool,
role_overrides: bool,
group_overrides: bool,
) -> Role<Box<TestConfig>> {
) -> Role<Box<TestConfig>, TestRoleConfig> {
let role_group = ROLE_GROUP.to_string();
let file_name = "foo.conf";

Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(),
Expand All @@ -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(
Expand All @@ -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(),
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(),
Expand All @@ -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(
Expand All @@ -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(),
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -1107,15 +1114,19 @@ mod tests {
let role_group_2 = "role_group_2";
let file_name = "foo.bar";

let roles: HashMap<String, (Vec<PropertyNameKind>, Role<Box<TestConfig>>)> = collection! {
#[allow(clippy::type_complexity)]
let roles: HashMap<
String,
(Vec<PropertyNameKind>, Role<Box<TestConfig>, TestRoleConfig>),
> = collection! {
fhennig marked this conversation as resolved.
Show resolved Hide resolved
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),
None,
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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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<String, (Vec<PropertyNameKind>, Role<Box<TestConfig>>)> = collection! {
#[allow(clippy::type_complexity)]
let roles: HashMap<
String,
(Vec<PropertyNameKind>, Role<Box<TestConfig>, 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),
Expand All @@ -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),
Expand Down
57 changes: 43 additions & 14 deletions src/role_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,28 +165,46 @@ 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<T> {
#[serde(flatten)]
/// 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<T, U = GenericRoleConfig>
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<T>,

#[serde(default)]
pub role_config: RoleConfig,
pub role_config: U,

pub role_groups: HashMap<String, RoleGroup<T>>,
}

impl<T: Configuration + 'static> Role<T> {
impl<T, U> Role<T, U>
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
/// [`Role`] with more than a single generic struct. For example different roles most likely
/// have different structs implementing Configuration.
pub fn erase(self) -> Role<Box<dyn Configuration<Configurable = T::Configurable>>> {
pub fn erase(self) -> Role<Box<dyn Configuration<Configurable = T::Configurable>>, U> {
Role {
config: CommonConfiguration {
config: Box::new(self.config.config)
Expand All @@ -196,7 +214,7 @@ impl<T: Configuration + 'static> Role<T> {
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()
Expand All @@ -222,13 +240,23 @@ impl<T: Configuration + 'static> Role<T> {
}
}

/// 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)]
#[serde(rename_all = "camelCase")]
pub struct RoleConfig {
pub struct GenericRoleConfig {
#[serde(default)]
pub pod_disruption_budget: PdbConfig,
}

/// 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 {}

#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)]
#[serde(
rename_all = "camelCase",
Expand All @@ -244,14 +272,15 @@ pub struct RoleGroup<T> {
}

impl<T> RoleGroup<T> {
pub fn validate_config<C>(
pub fn validate_config<C, U>(
&self,
role: &Role<T>,
role: &Role<T, U>,
default_config: &T,
) -> Result<C, fragment::ValidationError>
where
C: FromFragment<Fragment = T>,
T: Merge + Clone,
U: Default + JsonSchema + Serialize,
{
let mut role_config = role.config.config.clone();
role_config.merge(default_config);
Expand Down