refactor: consolidate ValidatedRoleGroupConfig

Author: maltesander

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

@@ -1,12 +1,15 @@
-use std::collections::BTreeMap;
+use std::{collections::BTreeMap, str::FromStr};
 
 use snafu::{ResultExt, Snafu};
 use stackable_operator::{
     commons::product_image_selection::{self},
     config::merge::Merge,
     role_utils::GenericRoleConfig,
     utils::cluster_info::KubernetesClusterInfo,
-    v2::controller_utils::{get_cluster_name, get_namespace, get_uid},
+    v2::{
+        builder::pod::container::{self, EnvVarName, EnvVarSet},
+        controller_utils::{get_cluster_name, get_namespace, get_uid},
+    },
 };
 use strum::IntoEnumIterator;
 
@@ -48,6 +51,12 @@ pub enum Error {
 
     #[snafu(display("failed to construct role-specific JVM arguments"))]
     ConstructJvmArgument { source: crate::config::jvm::Error },
+
+    #[snafu(display("the environment variable override name {name:?} is invalid"))]
+    InvalidEnvVarName {
+        source: container::Error,
+        name: String,
+    },
 }
 
 pub fn validate_cluster(
@@ -102,12 +111,21 @@ pub fn validate_cluster(
                 )
                 .context(FailedToResolveConfigSnafu)?;
 
+            let rolegroup_ref =
+                hbase.server_rolegroup_ref(hbase_role.to_string(), rolegroup_name.clone());
+
             group_configs.insert(
                 rolegroup_name.clone(),
                 ValidatedRoleGroupConfig {
+                    replicas: hbase.replicas(&hbase_role, &rolegroup_ref),
                     merged_config,
                     config_overrides: merged_config_overrides(hbase, &hbase_role, &rolegroup_name),
-                    env_overrides: merged_env_overrides(hbase, &hbase_role, &rolegroup_name),
+                    env_overrides: env_var_set(merged_env_overrides(
+                        hbase,
+                        &hbase_role,
+                        &rolegroup_name,
+                    ))?,
+                    pod_overrides: hbase.merged_pod_overrides(&hbase_role, &rolegroup_ref),
                     non_heap_jvm_args: construct_role_specific_non_heap_jvm_args(
                         hbase,
                         &hbase_role,
@@ -285,6 +303,19 @@ fn merged_env_overrides(
     env_overrides
 }
 
+/// Converts merged env override pairs into a type-safe [`EnvVarSet`], validating each name so that
+/// invalid environment variable names are rejected during validation instead of producing a broken
+/// Pod.
+fn env_var_set(env_overrides: BTreeMap<String, String>) -> Result<EnvVarSet, Error> {
+    let mut set = EnvVarSet::new();
+    for (name, value) in env_overrides {
+        let env_var_name =
+            EnvVarName::from_str(&name).context(InvalidEnvVarNameSnafu { name: name.clone() })?;
+        set = set.with_value(&env_var_name, value);
+    }
+    Ok(set)
+}
+
 #[cfg(test)]
 mod tests {
     use indoc::indoc;

rust/operator-binary/src/crd/mod.rs

@@ -351,12 +351,12 @@ impl v1alpha1::HbaseCluster {
         })
     }
 
-    pub fn merge_pod_overrides(
+    /// Returns the merged (role <- role group) pod template overrides for the given role group.
+    pub fn merged_pod_overrides(
         &self,
-        pod_template: &mut PodTemplateSpec,
         role: &HbaseRole,
         role_group_ref: &RoleGroupRef<Self>,
-    ) {
+    ) -> PodTemplateSpec {
         let (role_pod_overrides, role_group_pod_overrides) = match role {
             HbaseRole::Master => (
                 self.spec
@@ -393,41 +393,40 @@ impl v1alpha1::HbaseCluster {
             ),
         };
 
+        let mut merged = PodTemplateSpec::default();
         if let Some(rpo) = role_pod_overrides {
-            pod_template.merge_from(rpo);
+            merged.merge_from(rpo);
         }
         if let Some(rgpo) = role_group_pod_overrides {
-            pod_template.merge_from(rgpo);
+            merged.merge_from(rgpo);
         }
+        merged
     }
 
     pub fn replicas(
         &self,
         hbase_role: &HbaseRole,
         role_group_ref: &RoleGroupRef<Self>,
-    ) -> Option<i32> {
+    ) -> Option<u16> {
         match hbase_role {
             HbaseRole::Master => self
                 .spec
                 .masters
                 .as_ref()
                 .and_then(|r| r.role_groups.get(&role_group_ref.role_group))
-                .and_then(|rg| rg.replicas)
-                .map(i32::from),
+                .and_then(|rg| rg.replicas),
             HbaseRole::RegionServer => self
                 .spec
                 .region_servers
                 .as_ref()
                 .and_then(|r| r.role_groups.get(&role_group_ref.role_group))
-                .and_then(|rg| rg.replicas)
-                .map(i32::from),
+                .and_then(|rg| rg.replicas),
             HbaseRole::RestServer => self
                 .spec
                 .rest_servers
                 .as_ref()
                 .and_then(|r| r.role_groups.get(&role_group_ref.role_group))
-                .and_then(|rg| rg.replicas)
-                .map(i32::from),
+                .and_then(|rg| rg.replicas),
         }
     }
 

rust/operator-binary/src/hbase_controller.rs

@@ -19,11 +19,12 @@ use stackable_operator::{
     commons::{product_image_selection::ResolvedProductImage, rbac::build_rbac_resources},
     constants::RESTART_CONTROLLER_ENABLED_LABEL,
     k8s_openapi::{
+        DeepMerge,
         api::{
             apps::v1::{StatefulSet, StatefulSetSpec},
             core::v1::{
-                ConfigMapVolumeSource, ContainerPort, EnvVar, Probe, Service, ServiceAccount,
-                ServicePort, ServiceSpec, TCPSocketAction, Volume,
+                ConfigMapVolumeSource, ContainerPort, EnvVar, PodTemplateSpec, Probe, Service,
+                ServiceAccount, ServicePort, ServiceSpec, TCPSocketAction, Volume,
             },
         },
         apimachinery::pkg::{
@@ -54,6 +55,7 @@ use stackable_operator::{
     },
     v2::{
         HasName, HasUid, NameIsValidLabelValue,
+        builder::pod::container::EnvVarSet,
         types::{
             kubernetes::{NamespaceName, Uid},
             operator::ClusterName,
@@ -218,12 +220,19 @@ pub struct ValidatedRoleConfig {
 }
 
 /// Per-rolegroup configuration: the merged CRD config plus the merged
-/// (role <- role group) `configOverrides` and `envOverrides`.
+/// (role <- role group) `configOverrides`, `envOverrides` and `podOverrides`.
+///
+/// This carries every override channel so that the build step is a pure function of
+/// [`ValidatedCluster`] and never has to reach back into the raw `HbaseCluster`.
 #[derive(Clone, Debug)]
 pub struct ValidatedRoleGroupConfig {
+    /// The desired number of replicas (`None` lets Kubernetes default to 1).
+    pub replicas: Option<u16>,
     pub merged_config: AnyServiceConfig,
     pub config_overrides: v1alpha1::HbaseConfigOverrides,
-    pub env_overrides: BTreeMap<String, String>,
+    pub env_overrides: EnvVarSet,
+    /// Merged (role <- role group) pod template overrides.
+    pub pod_overrides: PodTemplateSpec,
     /// Pre-resolved role-specific non-heap JVM args (operator-generated + role/role-group overrides).
     pub non_heap_jvm_args: String,
 }
@@ -713,7 +722,9 @@ fn build_rolegroup_statefulset(
         ("HBASE_CONF_DIR".to_string(), CONFIG_DIR_NAME.to_string()),
         ("HADOOP_CONF_DIR".to_string(), CONFIG_DIR_NAME.to_string()),
     ]);
-    env_map.extend(validated_rg_config.env_overrides.clone());
+    for env_var in validated_rg_config.env_overrides.clone() {
+        env_map.insert(env_var.name, env_var.value.unwrap_or_default());
+    }
     let mut merged_env = merged_env(&env_map);
     // This env var is set for all roles to avoid bash's "unbound variable" errors
     merged_env.extend([
@@ -904,7 +915,7 @@ fn build_rolegroup_statefulset(
 
     let mut pod_template = pod_builder.build_template();
 
-    hbase.merge_pod_overrides(&mut pod_template, hbase_role, rolegroup_ref);
+    pod_template.merge_from(validated_rg_config.pod_overrides.clone());
 
     let metadata = ObjectMetaBuilder::new()
         .name_and_namespace(hbase)
@@ -931,7 +942,7 @@ fn build_rolegroup_statefulset(
 
     let statefulset_spec = StatefulSetSpec {
         pod_management_policy: Some("Parallel".to_string()),
-        replicas: hbase.replicas(hbase_role, rolegroup_ref),
+        replicas: validated_rg_config.replicas.map(i32::from),
         selector: LabelSelector {
             match_labels: Some(statefulset_match_labels.into()),
             ..LabelSelector::default()