refactor: switch to EnvVarSet

Author: maltesander

rust/operator-binary/src/framework/role_utils.rs

@@ -3,7 +3,6 @@
 //! zookeeper-operator.
 //!
 //! Differences from upstream:
-//! - `env_overrides` is `BTreeMap<String, String>` instead of `EnvVarSet`.
 //! - No `cli_overrides_to_vec` helper, `ResourceNames`, or service-account helpers.
 //! - The `CommonConfig` (a.k.a. `product_specific_common_config`) does NOT need to
 //!   implement `Merge`. ZooKeeper uses `JavaCommonConfig`, which intentionally does
@@ -14,9 +13,13 @@
 //! Replace with `stackable_operator::v2::role_utils::*` once upstream publishes
 //! the module.
 
-use std::collections::BTreeMap;
+use std::{
+    collections::{BTreeMap, HashMap},
+    str::FromStr,
+};
 
 use serde::Serialize;
+use snafu::{ResultExt, Snafu};
 use stackable_operator::{
     config::{
         fragment::{self, FromFragment},
@@ -25,19 +28,28 @@ use stackable_operator::{
     k8s_openapi::{DeepMerge, api::core::v1::PodTemplateSpec},
     role_utils::{Role, RoleGroup},
     schemars::JsonSchema,
+    v2::builder::pod::container::{self, EnvVarName, EnvVarSet},
 };
 
+#[derive(Snafu, Debug)]
+pub enum Error {
+    #[snafu(display("failed to validate the role group config"))]
+    ValidateConfig { source: fragment::ValidationError },
+
+    #[snafu(display("invalid environment variable override name"))]
+    ParseEnvVarName { source: container::Error },
+}
+
 /// A validated, merged view of a `RoleGroup`.
 ///
 /// Mirrors `stackable_operator::v2::role_utils::RoleGroupConfig` on the
-/// `smooth-operator` branch, with `env_overrides: BTreeMap<String, String>`
-/// instead of the upstream `EnvVarSet`.
+/// `smooth-operator` branch.
 #[derive(Clone, Debug, PartialEq)]
 pub struct RoleGroupConfig<Config, CommonConfig, ConfigOverrides> {
     pub replicas: u16,
     pub config: Config,
     pub config_overrides: ConfigOverrides,
-    pub env_overrides: BTreeMap<String, String>,
+    pub env_overrides: EnvVarSet,
     pub cli_overrides: BTreeMap<String, String>,
     pub pod_overrides: PodTemplateSpec,
     pub product_specific_common_config: CommonConfig,
@@ -58,18 +70,16 @@ pub fn with_validated_config<ValidatedConfig, CommonConfig, Config, RoleConfig,
     role_group: &RoleGroup<Config, CommonConfig, ConfigOverrides>,
     role: &Role<Config, ConfigOverrides, RoleConfig, CommonConfig>,
     default_config: &Config,
-) -> Result<
-    RoleGroupConfig<ValidatedConfig, CommonConfig, ConfigOverrides>,
-    fragment::ValidationError,
->
+) -> Result<RoleGroupConfig<ValidatedConfig, CommonConfig, ConfigOverrides>, Error>
 where
     ValidatedConfig: FromFragment<Fragment = Config>,
     CommonConfig: Clone + Default + JsonSchema + Serialize,
     Config: Clone + Merge,
     RoleConfig: Default + JsonSchema + Serialize,
     ConfigOverrides: Clone + Default + JsonSchema + Merge + Serialize,
 {
-    let validated_config = validate_config(role_group, role, default_config)?;
+    let validated_config =
+        validate_config(role_group, role, default_config).context(ValidateConfigSnafu)?;
     Ok(RoleGroupConfig {
         replicas: role_group.replicas.unwrap_or(1),
         config: validated_config,
@@ -78,18 +88,9 @@ where
             role_group.config.config_overrides.clone(),
         ),
         env_overrides: merged_env_overrides(
-            role.config
-                .env_overrides
-                .iter()
-                .map(|(k, v)| (k.clone(), v.clone()))
-                .collect(),
-            role_group
-                .config
-                .env_overrides
-                .iter()
-                .map(|(k, v)| (k.clone(), v.clone()))
-                .collect(),
-        ),
+            &role.config.env_overrides,
+            &role_group.config.env_overrides,
+        )?,
         cli_overrides: merged_cli_overrides(
             role.config.cli_overrides.clone(),
             role_group.config.cli_overrides.clone(),
@@ -128,12 +129,22 @@ where
 }
 
 fn merged_env_overrides(
-    role_env_overrides: BTreeMap<String, String>,
-    role_group_env_overrides: BTreeMap<String, String>,
-) -> BTreeMap<String, String> {
-    let mut merged = role_env_overrides;
-    merged.extend(role_group_env_overrides);
-    merged
+    role_env_overrides: &HashMap<String, String>,
+    role_group_env_overrides: &HashMap<String, String>,
+) -> Result<EnvVarSet, Error> {
+    // Process the role first, then the role group, so that role-group overrides win on key
+    // collisions (`EnvVarSet::with_value` overrides earlier entries with the same name).
+    let mut env_overrides = EnvVarSet::new();
+    for (name, value) in role_env_overrides
+        .iter()
+        .chain(role_group_env_overrides.iter())
+    {
+        env_overrides = env_overrides.with_value(
+            &EnvVarName::from_str(name).context(ParseEnvVarNameSnafu)?,
+            value.clone(),
+        );
+    }
+    Ok(env_overrides)
 }
 
 fn merged_cli_overrides(

rust/operator-binary/src/zk_controller.rs

@@ -1,5 +1,5 @@
 //! Ensures that `Pod`s are configured and running for each [`v1alpha1::ZookeeperCluster`]
-use std::{collections::BTreeMap, hash::Hasher, sync::Arc};
+use std::{hash::Hasher, sync::Arc};
 
 use const_format::concatcp;
 use fnv::FnvHasher;
@@ -58,6 +58,7 @@ use stackable_operator::{
         statefulset::StatefulSetConditionBuilder,
     },
     utils::COMMON_BASH_TRAP_FUNCTIONS,
+    v2::builder::pod::container::{EnvVarName, EnvVarSet},
 };
 use strum::{EnumDiscriminants, IntoStaticStr};
 
@@ -507,7 +508,7 @@ fn build_server_rolegroup_statefulset(
     zk: &v1alpha1::ZookeeperCluster,
     zk_role: &ZookeeperRole,
     rolegroup_ref: &RoleGroupRef<v1alpha1::ZookeeperCluster>,
-    env_overrides: &BTreeMap<String, String>,
+    env_overrides: &EnvVarSet,
     zookeeper_security: &ZookeeperSecurity,
     resolved_product_image: &ResolvedProductImage,
     merged_config: &v1alpha1::ZookeeperConfig,
@@ -526,23 +527,18 @@ fn build_server_rolegroup_statefulset(
     // The operator-injected environment variables (formerly produced by the
     // product-config `Configuration::compute_env` implementation) plus the
     // user-provided `envOverrides` (which win on conflict).
-    let mut env_map: BTreeMap<String, String> = BTreeMap::new();
-    env_map.insert(
-        v1alpha1::ZookeeperConfig::MYID_OFFSET.to_string(),
-        merged_config.myid_offset.to_string(),
-    );
-    // Used by zkEnv.sh and the shell scripts in bin/. If unset it tries to find the
-    // conf directory automatically and that fails.
-    env_map.insert("ZOOCFGDIR".to_string(), STACKABLE_RW_CONFIG_DIR.to_string());
-    env_map.extend(env_overrides.clone());
-    let env_vars = env_map
-        .into_iter()
-        .map(|(name, value)| EnvVar {
-            name,
-            value: Some(value),
-            ..EnvVar::default()
-        })
-        .collect::<Vec<_>>();
+    let env_vars = EnvVarSet::new()
+        .with_value(
+            &EnvVarName::from_str_unsafe(v1alpha1::ZookeeperConfig::MYID_OFFSET),
+            merged_config.myid_offset.to_string(),
+        )
+        // Used by zkEnv.sh and the shell scripts in bin/. If unset it tries to find the
+        // conf directory automatically and that fails.
+        .with_value(
+            &EnvVarName::from_str_unsafe("ZOOCFGDIR"),
+            STACKABLE_RW_CONFIG_DIR,
+        )
+        .merge(env_overrides.clone());
 
     let (original_pvcs, resources) = zk
         .resources(zk_role, rolegroup_ref)
@@ -885,6 +881,8 @@ pub fn error_policy(
 
 #[cfg(test)]
 mod tests {
+    use std::collections::BTreeMap;
+
     use stackable_operator::{
         commons::networking::DomainName,
         k8s_openapi::api::core::v1::ConfigMap,

rust/operator-binary/src/zk_controller/validate.rs

@@ -13,7 +13,6 @@ use snafu::{ResultExt, Snafu};
 use stackable_operator::{
     cli::OperatorEnvironmentOptions,
     commons::product_image_selection::{self, ResolvedProductImage},
-    config::fragment,
     k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta,
     kube::{Resource, ResourceExt},
     role_utils::JavaCommonConfig,
@@ -59,9 +58,9 @@ pub enum Error {
     #[snafu(display("failed to list expected pods"))]
     ListPods { source: crate::crd::Error },
 
-    #[snafu(display("invalid config fragment for role group {role_group:?}"))]
-    InvalidConfigFragment {
-        source: fragment::ValidationError,
+    #[snafu(display("invalid config for role group {role_group:?}"))]
+    InvalidRoleGroupConfig {
+        source: crate::framework::role_utils::Error,
         role_group: String,
     },
 
@@ -221,7 +220,7 @@ pub fn validate(
                 _,
                 ZookeeperConfigOverrides,
             >(rg, role, &default_config)
-            .with_context(|_| InvalidConfigFragmentSnafu {
+            .with_context(|_| InvalidRoleGroupConfigSnafu {
                 role_group: rg_name.clone(),
             })?;
             groups.insert(rg_name.clone(), validated_rg);