refactor: switch to EnvVarSet

Author: maltesander

rust/operator-binary/src/controller.rs

@@ -55,6 +55,7 @@ use stackable_operator::{
         compute_conditions, operations::ClusterOperationsConditionBuilder,
         statefulset::StatefulSetConditionBuilder,
     },
+    v2::builder::pod::container::EnvVarSet,
 };
 use strum::{EnumDiscriminants, IntoStaticStr};
 
@@ -584,7 +585,7 @@ fn build_rolegroup_statefulset(
     trino_role: &TrinoRole,
     resolved_product_image: &ResolvedProductImage,
     role_group_ref: &RoleGroupRef<v1alpha1::TrinoCluster>,
-    env_overrides: &BTreeMap<String, String>,
+    env_overrides: &EnvVarSet,
     merged_config: &v1alpha1::TrinoConfig,
     trino_authentication_config: &TrinoAuthenticationConfig,
     catalogs: &[CatalogConfig],
@@ -668,11 +669,7 @@ fn build_rolegroup_statefulset(
     });
 
     // Finally add the user defined envOverrides properties.
-    env.extend(env_overrides.iter().map(|(k, v)| EnvVar {
-        name: k.clone(),
-        value: Some(v.clone()),
-        ..EnvVar::default()
-    }));
+    env.extend(env_overrides.clone());
 
     let requested_secret_lifetime = merged_config
         .requested_secret_lifetime
@@ -1323,6 +1320,7 @@ mod tests {
         cli::OperatorEnvironmentOptions, commons::networking::DomainName,
         k8s_openapi::api::core::v1::ConfigMap, kube::runtime::reflector::ObjectRef,
         role_utils::RoleGroupRef, utils::cluster_info::KubernetesClusterInfo,
+        v2::builder::pod::container::EnvVarName,
     };
 
     use super::*;
@@ -1646,7 +1644,10 @@ mod tests {
 
         let env =
             &validated_cluster.role_group_configs[&TrinoRole::Coordinator]["default"].env_overrides;
-        let value = |name: &str| env.get(name).cloned();
+        let value = |name: &str| {
+            env.get(&EnvVarName::from_str_unsafe(name))
+                .and_then(|env_var| env_var.value.clone())
+        };
         assert_eq!(value("COMMON_VAR").as_deref(), Some("group-value"));
         assert_eq!(value("GROUP_VAR").as_deref(), Some("group-value"));
         assert_eq!(value("ROLE_VAR").as_deref(), Some("role-value"));

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

@@ -81,9 +81,10 @@ pub enum Error {
         role: String,
     },
 
-    #[snafu(display("failed to validate config fragment"))]
-    InvalidConfigFragment {
-        source: stackable_operator::config::fragment::ValidationError,
+    #[snafu(display("failed to resolve and merge config for role group {role_group}"))]
+    FailedToResolveConfig {
+        source: crate::framework::role_utils::Error,
+        role_group: String,
     },
 }
 
@@ -264,7 +265,9 @@ pub fn validate(
                 GenericRoleConfig,
                 v1alpha1::TrinoConfigOverrides,
             >(rg, &role, &default_config)
-            .context(InvalidConfigFragmentSnafu)?;
+            .with_context(|_| FailedToResolveConfigSnafu {
+                role_group: rg_name.clone(),
+            })?;
             groups.insert(rg_name.clone(), validated_rg);
         }
         role_jvm_argument_overrides.insert(

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

@@ -2,7 +2,6 @@
 //! `smooth-operator` branch, with simplifications appropriate for trino-operator.
 //!
 //! Differences from upstream:
-//! - `env_overrides` is `HashMap<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`. Upstream Trino uses `JavaCommonConfig`, which intentionally
@@ -15,9 +14,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},
@@ -26,19 +29,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 },
+}
+
 /// Trino-friendly view of a validated, merged `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,
@@ -59,18 +71,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,
@@ -79,18 +89,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(),
@@ -129,12 +130,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(