refactor: Use typed constants, remove duplicated merge

Author: maltesander

rust/operator-binary/src/controller/build/jvm.rs

@@ -100,7 +100,7 @@ fn is_heap_jvm_argument(jvm_argument: &str) -> bool {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::crd::{HbaseRole, v1alpha1};
+    use crate::{crd::HbaseRole, test_utils};
 
     #[test]
     fn test_construct_jvm_arguments_defaults() {
@@ -109,6 +109,8 @@ mod tests {
         kind: HbaseCluster
         metadata:
           name: simple-hbase
+          namespace: default
+          uid: 12345678-1234-1234-1234-123456789012
         spec:
           image:
             productVersion: 2.6.4
@@ -124,16 +126,20 @@ mod tests {
               default:
                 replicas: 1
         "#;
-        let (hbase, merged_config, merged_jvm_argument_overrides) = construct_boilerplate(input);
+        let hbase = test_utils::hbase_from_yaml(input);
+        let validated_cluster = test_utils::validated_cluster_from(&hbase);
+        let region_server = &validated_cluster.role_group_configs[&HbaseRole::RegionServer]
+            [&test_utils::role_group_name("default")];
 
-        let global_jvm_args = construct_global_jvm_args(false);
-        let role_specific_non_heap_jvm_args =
-            construct_role_specific_non_heap_jvm_args(&hbase, &merged_jvm_argument_overrides);
-        let hbase_heapsize_env = construct_hbase_heapsize_env(&merged_config).unwrap();
+        let global_jvm_args = construct_global_jvm_args(hbase.has_kerberos_enabled());
+        let hbase_heapsize_env =
+            construct_hbase_heapsize_env(&region_server.config.config).unwrap();
 
         assert_eq!(global_jvm_args, "");
+        // `non_heap_jvm_args` is the output of `construct_role_specific_non_heap_jvm_args`,
+        // pre-resolved during validation.
         assert_eq!(
-            role_specific_non_heap_jvm_args,
+            region_server.non_heap_jvm_args,
             "-Djava.security.properties=/stackable/conf/security.properties"
         );
         assert_eq!(hbase_heapsize_env, "819m");
@@ -146,6 +152,8 @@ mod tests {
         kind: HbaseCluster
         metadata:
           name: simple-hbase
+          namespace: default
+          uid: 12345678-1234-1234-1234-123456789012
         spec:
           image:
             productVersion: 2.6.4
@@ -180,19 +188,21 @@ mod tests {
                     - -Xmx40000m # This has no effect!
                     - -Dhttps.proxyPort=1234
         "#;
-        let (hbase, merged_config, merged_jvm_argument_overrides) = construct_boilerplate(input);
+        let hbase = test_utils::hbase_from_yaml(input);
+        let validated_cluster = test_utils::validated_cluster_from(&hbase);
+        let region_server = &validated_cluster.role_group_configs[&HbaseRole::RegionServer]
+            [&test_utils::role_group_name("default")];
 
         let global_jvm_args = construct_global_jvm_args(hbase.has_kerberos_enabled());
-        let role_specific_non_heap_jvm_args =
-            construct_role_specific_non_heap_jvm_args(&hbase, &merged_jvm_argument_overrides);
-        let hbase_heapsize_env = construct_hbase_heapsize_env(&merged_config).unwrap();
+        let hbase_heapsize_env =
+            construct_hbase_heapsize_env(&region_server.config.config).unwrap();
 
         assert_eq!(
             global_jvm_args,
             "-Djava.security.krb5.conf=/stackable/kerberos/krb5.conf"
         );
         assert_eq!(
-            role_specific_non_heap_jvm_args,
+            region_server.non_heap_jvm_args,
             "-Djava.security.properties=/stackable/conf/security.properties \
             -Djava.security.krb5.conf=/stackable/kerberos/krb5.conf \
             -Dhttps.proxyHost=proxy.my.corp \
@@ -201,28 +211,4 @@ mod tests {
         );
         assert_eq!(hbase_heapsize_env, "34406m");
     }
-
-    fn construct_boilerplate(
-        hbase_cluster: &str,
-    ) -> (
-        v1alpha1::HbaseCluster,
-        AnyServiceConfig,
-        JvmArgumentOverrides,
-    ) {
-        let hbase: v1alpha1::HbaseCluster =
-            serde_yaml::from_str(hbase_cluster).expect("illegal test input");
-
-        // Merge + validate the region server `default` role group via the real
-        // `with_validated_config` path, returning the merged config (for heap sizing) and the
-        // merged JVM argument overrides.
-        let (merged_config, merged_jvm_argument_overrides) =
-            crate::crd::test_helpers::merged_role_group_config(
-                &hbase,
-                &HbaseRole::RegionServer,
-                "default",
-                "my-hdfs",
-            );
-
-        (hbase, merged_config, merged_jvm_argument_overrides)
-    }
 }

rust/operator-binary/src/controller/build/properties/hbase_env.rs

@@ -62,7 +62,7 @@ pub fn build(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::controller::build::properties::test_support::{merged_config, validated_cluster};
+    use crate::test_utils::{merged_config, validated_cluster};
 
     #[test]
     fn renders_operator_defaults() {

rust/operator-binary/src/controller/build/properties/hbase_site.rs

@@ -112,7 +112,7 @@ pub fn build(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::controller::build::properties::test_support::{merged_config, validated_cluster};
+    use crate::test_utils::{merged_config, validated_cluster};
 
     #[test]
     fn renders_operator_defaults() {

rust/operator-binary/src/controller/build/properties/mod.rs

@@ -57,89 +57,3 @@ mod tests {
         assert_eq!(ConfigFileName::Log4j2.to_string(), "log4j2.properties");
     }
 }
-
-#[cfg(test)]
-pub(crate) mod test_support {
-    use std::str::FromStr;
-
-    use stackable_operator::{
-        commons::networking::DomainName, utils::cluster_info::KubernetesClusterInfo,
-    };
-
-    use crate::{
-        controller::{
-            ValidatedCluster, dereference::DereferencedObjects, validate::validate_cluster,
-            zookeeper::ZookeeperConnectionInformation,
-        },
-        crd::{AnyServiceConfig, HbaseRole, v1alpha1},
-    };
-
-    /// A minimal three-role HbaseCluster used to drive the per-file builder tests.
-    pub const MINIMAL_HBASE_YAML: &str = r#"
----
-apiVersion: hbase.stackable.tech/v1alpha1
-kind: HbaseCluster
-metadata:
-  name: hbase
-  namespace: default
-  uid: c2c8c5c0-0b5a-4b1e-9f3e-1a2b3c4d5e6f
-spec:
-  image:
-    productVersion: 2.6.3
-  clusterConfig:
-    hdfsConfigMapName: simple-hdfs
-    zookeeperConfigMapName: simple-znode
-  masters:
-    roleGroups:
-      default:
-        replicas: 1
-  regionServers:
-    roleGroups:
-      default:
-        replicas: 1
-  restServers:
-    roleGroups:
-      default:
-        replicas: 1
-"#;
-
-    pub fn minimal_hbase() -> v1alpha1::HbaseCluster {
-        serde_yaml::from_str(MINIMAL_HBASE_YAML).expect("invalid test HbaseCluster YAML")
-    }
-
-    pub fn cluster_info() -> KubernetesClusterInfo {
-        KubernetesClusterInfo {
-            cluster_domain: DomainName::try_from("cluster.local").unwrap(),
-        }
-    }
-
-    /// Runs the real validation pipeline once over [`minimal_hbase`], with a fixed
-    /// dereferenced ZooKeeper connection (and no OPA), so the per-file builder tests can
-    /// pull merged configs straight from the [`ValidatedCluster`] instead of re-merging by
-    /// hand via `crd::merged_config`.
-    pub fn validated_cluster() -> ValidatedCluster {
-        validate_cluster(
-            &minimal_hbase(),
-            "oci.example.org",
-            &cluster_info(),
-            DereferencedObjects {
-                zookeeper_connection_information: ZookeeperConnectionInformation::for_tests(),
-                hbase_opa_config: None,
-            },
-        )
-        .expect("validate should succeed for the minimal fixture")
-    }
-
-    /// The merged [`AnyServiceConfig`] for the `default` role group of `role`.
-    pub fn merged_config<'a>(
-        validated_cluster: &'a ValidatedCluster,
-        role: &HbaseRole,
-    ) -> &'a AnyServiceConfig {
-        let default_role_group =
-            stackable_operator::v2::types::operator::RoleGroupName::from_str("default")
-                .expect("'default' is a valid role group name");
-        &validated_cluster.role_group_configs[role][&default_role_group]
-            .config
-            .config
-    }
-}

rust/operator-binary/src/controller/build/resource/service.rs

@@ -137,13 +137,10 @@ fn prometheus_annotations(hbase: &v1alpha1::HbaseCluster, hbase_role: &HbaseRole
 
 #[cfg(test)]
 mod test {
-    use std::str::FromStr;
-
     use rstest::rstest;
-    use stackable_operator::v2::types::operator::RoleGroupName;
 
     use super::*;
-    use crate::controller::build::properties::test_support;
+    use crate::test_utils;
 
     #[rstest]
     #[case("2.6.3", HbaseRole::Master, vec!["master", "ui-http"])]
@@ -187,8 +184,8 @@ mod test {
         let hbase: v1alpha1::HbaseCluster =
             serde_yaml::from_str(&input).expect("illegal test input");
 
-        let cluster = test_support::validated_cluster();
-        let role_group_name = RoleGroupName::from_str("default").expect("valid role group name");
+        let cluster = test_utils::validated_cluster();
+        let role_group_name = test_utils::role_group_name("default");
         let service = build_rolegroup_service(&hbase, &cluster, &role, &role_group_name);
 
         assert_eq!(

rust/operator-binary/src/controller/build/resource/statefulset.rs

@@ -48,11 +48,13 @@ use crate::{
 
 stackable_operator::constant!(VECTOR_CONTAINER_NAME: ContainerName = "vector");
 
-// The Vector container reads `vector.yaml` from the rolegroup ConfigMap (mounted as the
-// `hbase-config` volume) and writes to the shared `log` volume. These reuse the existing
-// volume-name string values so the produced volume mounts match the rest of the Pod.
-stackable_operator::constant!(VECTOR_LOG_CONFIG_VOLUME_NAME: VolumeName = "hbase-config");
-stackable_operator::constant!(VECTOR_LOG_VOLUME_NAME: VolumeName = "log");
+// Pod volume names. The Vector container reuses the `hbase-config` (rolegroup ConfigMap, which
+// carries `vector.yaml`) and `log` volumes, so the produced volume mounts match the rest of the
+// Pod.
+stackable_operator::constant!(HBASE_CONFIG_VOLUME_NAME: VolumeName = "hbase-config");
+stackable_operator::constant!(HDFS_DISCOVERY_VOLUME_NAME: VolumeName = "hdfs-discovery");
+stackable_operator::constant!(LOG_CONFIG_VOLUME_NAME: VolumeName = "log-config");
+stackable_operator::constant!(LOG_VOLUME_NAME: VolumeName = "log");
 
 pub static CONTAINERDEBUG_LOG_DIRECTORY: std::sync::LazyLock<String> =
     std::sync::LazyLock::new(|| format!("{STACKABLE_LOG_DIR}/containerdebug"));
@@ -192,13 +194,13 @@ pub fn build_rolegroup_statefulset(
             "CONTAINERDEBUG_LOG_DIRECTORY",
             &*CONTAINERDEBUG_LOG_DIRECTORY,
         )
-        .add_volume_mount("hbase-config", HBASE_CONFIG_TMP_DIR)
+        .add_volume_mount(&*HBASE_CONFIG_VOLUME_NAME, HBASE_CONFIG_TMP_DIR)
         .context(AddVolumeMountSnafu)?
-        .add_volume_mount("hdfs-discovery", HDFS_DISCOVERY_TMP_DIR)
+        .add_volume_mount(&*HDFS_DISCOVERY_VOLUME_NAME, HDFS_DISCOVERY_TMP_DIR)
         .context(AddVolumeMountSnafu)?
-        .add_volume_mount("log-config", HBASE_LOG_CONFIG_TMP_DIR)
+        .add_volume_mount(&*LOG_CONFIG_VOLUME_NAME, HBASE_LOG_CONFIG_TMP_DIR)
         .context(AddVolumeMountSnafu)?
-        .add_volume_mount("log", STACKABLE_LOG_DIR)
+        .add_volume_mount(&*LOG_VOLUME_NAME, STACKABLE_LOG_DIR)
         .context(AddVolumeMountSnafu)?
         .add_volume_mount(LISTENER_VOLUME_NAME, LISTENER_VOLUME_DIR)
         .context(AddVolumeMountSnafu)?
@@ -221,7 +223,7 @@ pub fn build_rolegroup_statefulset(
         .image_pull_secrets_from_product_image(resolved_product_image)
         .affinity(merged_config.affinity())
         .add_volume(Volume {
-            name: "hbase-config".to_string(),
+            name: HBASE_CONFIG_VOLUME_NAME.to_string(),
             config_map: Some(ConfigMapVolumeSource {
                 name: resource_names.role_group_config_map().to_string(),
                 ..Default::default()
@@ -230,7 +232,7 @@ pub fn build_rolegroup_statefulset(
         })
         .context(AddVolumeSnafu)?
         .add_volume(Volume {
-            name: "hdfs-discovery".to_string(),
+            name: HDFS_DISCOVERY_VOLUME_NAME.to_string(),
             config_map: Some(ConfigMapVolumeSource {
                 name: hbase.spec.cluster_config.hdfs_config_map_name.clone(),
                 ..Default::default()
@@ -239,7 +241,7 @@ pub fn build_rolegroup_statefulset(
         })
         .context(AddVolumeSnafu)?
         .add_empty_dir_volume(
-            "log",
+            &*LOG_VOLUME_NAME,
             Some(product_logging::framework::calculate_log_volume_size_limit(
                 &[MAX_HBASE_LOG_FILES_SIZE],
             )),
@@ -259,7 +261,7 @@ pub fn build_rolegroup_statefulset(
     };
     pod_builder
         .add_volume(Volume {
-            name: "log-config".to_string(),
+            name: LOG_CONFIG_VOLUME_NAME.to_string(),
             config_map: Some(ConfigMapVolumeSource {
                 name: log_config_config_map,
                 ..ConfigMapVolumeSource::default()
@@ -290,8 +292,8 @@ pub fn build_rolegroup_statefulset(
             resolved_product_image,
             vector_log_config,
             &resource_names,
-            &VECTOR_LOG_CONFIG_VOLUME_NAME,
-            &VECTOR_LOG_VOLUME_NAME,
+            &HBASE_CONFIG_VOLUME_NAME,
+            &LOG_VOLUME_NAME,
             EnvVarSet::new(),
         ));
     }

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

@@ -426,4 +426,58 @@ spec:
         assert_eq!(env.get("TEST_VAR_FROM_MASTER"), Some(&"MASTER".to_string()));
         assert_eq!(env.get("TEST_VAR_FROM_MRG"), Some(&"MASTER".to_string()));
     }
+
+    /// A custom log ConfigMap name that is not a valid Kubernetes name is rejected up-front.
+    #[test]
+    fn validate_logging_rejects_invalid_custom_config_map_name() {
+        use stackable_operator::product_logging::spec::{
+            ConfigMapLogConfig, ContainerLogConfig, ContainerLogConfigChoice,
+            CustomContainerLogConfig,
+        };
+
+        let logging = Logging {
+            enable_vector_agent: false,
+            containers: [(
+                Container::Hbase,
+                ContainerLogConfig {
+                    choice: Some(ContainerLogConfigChoice::Custom(CustomContainerLogConfig {
+                        custom: ConfigMapLogConfig {
+                            config_map: "invalid ConfigMap name".to_owned(),
+                        },
+                    })),
+                },
+            )]
+            .into(),
+        };
+
+        assert!(validate_logging(&logging, &None).is_err());
+    }
+
+    /// Enabling the Vector agent without a Vector aggregator discovery ConfigMap name fails, but
+    /// succeeds once a valid name is provided.
+    #[test]
+    fn validate_logging_requires_vector_aggregator_when_enabled() {
+        use stackable_operator::product_logging::spec::{
+            AutomaticContainerLogConfig, ContainerLogConfig, ContainerLogConfigChoice,
+        };
+
+        let automatic = || ContainerLogConfig {
+            choice: Some(ContainerLogConfigChoice::Automatic(
+                AutomaticContainerLogConfig::default(),
+            )),
+        };
+        let logging = Logging {
+            enable_vector_agent: true,
+            containers: [
+                (Container::Hbase, automatic()),
+                (Container::Vector, automatic()),
+            ]
+            .into(),
+        };
+
+        assert!(validate_logging(&logging, &None).is_err());
+
+        let aggregator = ConfigMapName::from_str("vector-aggregator").expect("valid name");
+        assert!(validate_logging(&logging, &Some(aggregator)).is_ok());
+    }
 }