refactor: use upstream EnvVarSet

Author: maltesander

rust/operator-binary/src/controller.rs

@@ -74,7 +74,6 @@ use stackable_operator::{
     utils::COMMON_BASH_TRAP_FUNCTIONS,
 };
 use strum::EnumDiscriminants;
-use tracing::warn;
 
 use crate::{
     OPERATOR_NAME,
@@ -632,17 +631,8 @@ fn build_metastore_rolegroup_statefulset(
     database_connection_details.add_to_container(&mut container_builder);
 
     // Environment variable overrides (highest precedence), merged from role and role group.
-    for (property_name, property_value) in &rg.env_overrides {
-        if property_name.is_empty() {
-            warn!(
-                property_name,
-                property_value,
-                "The env variable had an empty name, not adding it to the container"
-            );
-            continue;
-        }
-        container_builder.add_env_var(property_name, property_value);
-    }
+    // Names are validated during cluster validation, so they can be applied directly here.
+    container_builder.add_env_vars(rg.env_overrides.clone());
 
     let mut pod_builder = PodBuilder::new();
 

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

@@ -59,7 +59,7 @@ pub enum Error {
 
     #[snafu(display("failed to resolve and merge config for role group {role_group}"))]
     FailedToResolveConfig {
-        source: stackable_operator::config::fragment::ValidationError,
+        source: crate::framework::role_utils::Error,
         role_group: String,
     },
 

rust/operator-binary/src/framework.rs

@@ -2,10 +2,8 @@
 //! `stackable_operator::v2::*` modules.
 //!
 //! We vendor `role_utils` because the upstream `v2::role_utils` requires
-//! `CommonConfig: Merge` and uses `EnvVarSet` for `env_overrides`. Hive (like
-//! trino) uses `JavaCommonConfig`, whose JVM-argument merge is fallible and so
-//! does not implement `Merge`; we also want `env_overrides` as a plain
-//! `BTreeMap<String, String>`.
+//! `CommonConfig: Merge`. Hive (like trino) uses `JavaCommonConfig`, whose
+//! JVM-argument merge is fallible and so does not implement `Merge`.
 //!
 //! Follow-up: replace with `stackable_operator::v2::role_utils::*` once upstream
 //! relaxes the `Merge` bound.

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

@@ -2,7 +2,6 @@
 //! `smooth-operator` branch, with simplifications appropriate for hive-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`. Hive uses `JavaCommonConfig`, which intentionally does not
@@ -13,9 +12,13 @@
 //! Replace with `stackable_operator::v2::role_utils::*` once upstream relaxes the
 //! `Merge` bound.
 
-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},
@@ -24,15 +27,25 @@ 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 },
+}
+
 /// Hive-friendly view of a validated, merged `RoleGroup`.
 #[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,
@@ -43,18 +56,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,
@@ -63,18 +74,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(),
@@ -113,12 +115,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(