fix: env-overrides with tests (#550)

* implemented env-var merge and added an integration test * added unit test for env-overrides * formatting * linting * changelog * added indoc to cargo.lock * refactor: use merged rolegroup-config * make function stand-alone
stackabletech · Aug 20, 2024 · dc480ca · dc480ca
1 parent e30fb70
commit dc480ca
Show file tree

Hide file tree

Showing 18 changed files with 477 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,7 +8,12 @@
   - `podOverrides`
   - `affinity`
 
+### Fixed
+
+- Implement `envOverrides` for HbaseCluster ([#550]).
+
 [#548]: https://github.com/stackabletech/hbase-operator/pull/548
+[#550]: https://github.com/stackabletech/hbase-operator/pull/550
 
 ## [24.7.0] - 2024-07-24
 

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.nix b/Cargo.nix
diff --git a/rust/crd/Cargo.toml b/rust/crd/Cargo.toml
@@ -9,6 +9,7 @@ repository.workspace = true
 publish = false
 
 [dependencies]
+product-config.workspace = true
 serde.workspace = true
 serde_json.workspace = true
 snafu.workspace = true
@@ -19,3 +20,4 @@ tracing.workspace = true
 [dev-dependencies]
 rstest.workspace = true
 serde_yaml.workspace = true
+indoc.workspace = true
diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs
@@ -13,8 +13,11 @@ use stackable_operator::{
             Resources, ResourcesFragment,
         },
     },
-    config::{fragment, fragment::Fragment, fragment::ValidationError, merge::Merge},
-    k8s_openapi::apimachinery::pkg::api::resource::Quantity,
+    config::{
+        fragment::{self, Fragment, ValidationError},
+        merge::Merge,
+    },
+    k8s_openapi::{api::core::v1::EnvVar, apimachinery::pkg::api::resource::Quantity},
     kube::{runtime::reflector::ObjectRef, CustomResource, ResourceExt},
     product_config_utils::Configuration,
     product_logging::{self, spec::Logging},
@@ -419,7 +422,20 @@ impl Configuration for HbaseConfigFragment {
         _role_name: &str,
     ) -> Result<BTreeMap<String, Option<String>>, stackable_operator::product_config_utils::Error>
     {
-        Ok(BTreeMap::new())
+        // Maps env var name to env var object. This allows env_overrides to work
+        // as expected (i.e. users can override the env var value).
+        let mut vars: BTreeMap<String, Option<String>> = BTreeMap::new();
+
+        vars.insert(
+            "HBASE_CONF_DIR".to_string(),
+            Some(CONFIG_DIR_NAME.to_string()),
+        );
+        // required by phoenix (for cases where Kerberos is enabled): see https://issues.apache.org/jira/browse/PHOENIX-2369
+        vars.insert(
+            "HADOOP_CONF_DIR".to_string(),
+            Some(CONFIG_DIR_NAME.to_string()),
+        );
+        Ok(vars)
     }
 
     fn compute_cli(
@@ -656,3 +672,133 @@ impl HbaseCluster {
         fragment::validate(conf_rolegroup).context(FragmentValidationFailureSnafu)
     }
 }
+
+pub fn merged_env(rolegroup_config: Option<&BTreeMap<String, String>>) -> Vec<EnvVar> {
+    let merged_env: Vec<EnvVar> = if let Some(rolegroup_config) = rolegroup_config {
+        let env_vars_from_config: BTreeMap<String, EnvVar> = rolegroup_config
+            .iter()
+            .map(|(env_name, env_value)| {
+                (
+                    env_name.clone(),
+                    EnvVar {
+                        name: env_name.clone(),
+                        value: Some(env_value.to_owned()),
+                        value_from: None,
+                    },
+                )
+            })
+            .collect();
+        env_vars_from_config.into_values().collect()
+    } else {
+        vec![]
+    };
+    merged_env
+}
+
+#[cfg(test)]
+mod tests {
+    use std::collections::{BTreeMap, HashMap};
+
+    use indoc::indoc;
+    use stackable_operator::product_config_utils::{
+        transform_all_roles_to_config, validate_all_roles_and_groups_config,
+    };
+
+    use crate::{merged_env, HbaseCluster, HbaseRole};
+
+    use product_config::{types::PropertyNameKind, ProductConfigManager};
+
+    #[test]
+    pub fn test_env_overrides() {
+        let input = indoc! {r#"
+---
+apiVersion: hbase.stackable.tech/v1alpha1
+kind: HbaseCluster
+metadata:
+  name: test-hbase
+spec:
+  image:
+    productVersion: 2.4.18
+  clusterConfig:
+    hdfsConfigMapName: test-hdfs
+    zookeeperConfigMapName: test-znode
+  masters:
+    envOverrides:
+      TEST_VAR_FROM_MASTER: MASTER
+      TEST_VAR: MASTER
+    config:
+      logging:
+        enableVectorAgent: False
+    roleGroups:
+      default:
+        replicas: 1
+        envOverrides:
+          TEST_VAR_FROM_MRG: MASTER
+          TEST_VAR: MASTER_RG
+  regionServers:
+    config:
+      logging:
+        enableVectorAgent: False
+    roleGroups:
+      default:
+        replicas: 1
+  restServers:
+    config:
+      logging:
+        enableVectorAgent: False
+    roleGroups:
+      default:
+        replicas: 1
+        "#};
+
+        let deserializer = serde_yaml::Deserializer::from_str(input);
+        let hbase: HbaseCluster =
+            serde_yaml::with::singleton_map_recursive::deserialize(deserializer).unwrap();
+
+        let roles = HashMap::from([(
+            HbaseRole::Master.to_string(),
+            (
+                vec![PropertyNameKind::Env],
+                hbase.get_role(&HbaseRole::Master).cloned().unwrap(),
+            ),
+        )]);
+
+        let validated_config = validate_all_roles_and_groups_config(
+            "2.4.18",
+            &transform_all_roles_to_config(&hbase, roles).unwrap(),
+            &ProductConfigManager::from_yaml_file("../../deploy/config-spec/properties.yaml")
+                .unwrap(),
+            false,
+            false,
+        )
+        .unwrap();
+
+        let rolegroup_config = validated_config
+            .get(&HbaseRole::Master.to_string())
+            .unwrap()
+            .get("default")
+            .unwrap()
+            .get(&PropertyNameKind::Env);
+        let merged_env = merged_env(rolegroup_config);
+
+        let env_map: BTreeMap<&str, Option<String>> = merged_env
+            .iter()
+            .map(|env_var| (env_var.name.as_str(), env_var.value.clone()))
+            .collect();
+
+        println!("{:#?}", merged_env);
+
+        assert_eq!(
+            Some(&Some("MASTER_RG".to_string())),
+            env_map.get("TEST_VAR")
+        );
+        assert_eq!(
+            Some(&Some("MASTER".to_string())),
+            env_map.get("TEST_VAR_FROM_MASTER")
+        );
+        assert_eq!(
+            Some(&Some("MASTER".to_string())),
+            env_map.get("TEST_VAR_FROM_MRG")
+        );
+    }
+}
diff --git a/rust/operator-binary/src/hbase_controller.rs b/rust/operator-binary/src/hbase_controller.rs
@@ -62,9 +62,9 @@ use stackable_operator::{
 use strum::{EnumDiscriminants, IntoStaticStr, ParseError};
 
 use stackable_hbase_crd::{
-    Container, HbaseCluster, HbaseClusterStatus, HbaseConfig, HbaseConfigFragment, HbaseRole,
-    APP_NAME, CONFIG_DIR_NAME, HBASE_ENV_SH, HBASE_HEAPSIZE, HBASE_MANAGES_ZK, HBASE_MASTER_OPTS,
-    HBASE_REGIONSERVER_OPTS, HBASE_REST_OPTS, HBASE_REST_PORT_NAME_HTTP,
+    merged_env, Container, HbaseCluster, HbaseClusterStatus, HbaseConfig, HbaseConfigFragment,
+    HbaseRole, APP_NAME, CONFIG_DIR_NAME, HBASE_ENV_SH, HBASE_HEAPSIZE, HBASE_MANAGES_ZK,
+    HBASE_MASTER_OPTS, HBASE_REGIONSERVER_OPTS, HBASE_REST_OPTS, HBASE_REST_PORT_NAME_HTTP,
     HBASE_REST_PORT_NAME_HTTPS, HBASE_SITE_XML, JVM_HEAP_FACTOR, JVM_SECURITY_PROPERTIES_FILE,
     METRICS_PORT, SSL_CLIENT_XML, SSL_SERVER_XML,
 };
@@ -419,6 +419,7 @@ pub async fn reconcile_hbase(hbase: Arc<HbaseCluster>, ctx: Arc<Ctx>) -> Result<
                 &hbase,
                 &hbase_role,
                 &rolegroup,
+                rolegroup_config,
                 &merged_config,
                 &resolved_product_image,
             )?;
@@ -703,7 +704,7 @@ fn build_rolegroup_service(
 
     let metadata = ObjectMetaBuilder::new()
         .name_and_namespace(hbase)
-        .name(&rolegroup.object_name())
+        .name(rolegroup.object_name())
         .ownerreference_from_resource(hbase, None, Some(true))
         .context(ObjectMissingMetadataForOwnerRefSnafu)?
         .with_recommended_labels(build_recommended_labels(
@@ -744,6 +745,7 @@ fn build_rolegroup_statefulset(
     hbase: &HbaseCluster,
     hbase_role: &HbaseRole,
     rolegroup_ref: &RoleGroupRef<HbaseCluster>,
+    rolegroup_config: &HashMap<PropertyNameKind, BTreeMap<String, String>>,
     config: &HbaseConfig,
     resolved_product_image: &ResolvedProductImage,
 ) -> Result<StatefulSet> {
@@ -818,6 +820,8 @@ fn build_rolegroup_statefulset(
         ..probe_template
     };
 
+    let merged_env = merged_env(rolegroup_config.get(&PropertyNameKind::Env));
+
     let log4j_properties_file_name =
         log4j_properties_file_name(&resolved_product_image.product_version);
     let mut hbase_container = ContainerBuilder::new("hbase").expect("ContainerBuilder not created");
@@ -853,9 +857,7 @@ fn build_rolegroup_statefulset(
             create_vector_shutdown_file_command =
                 create_vector_shutdown_file_command(STACKABLE_LOG_DIR),
         }])
-        .add_env_var("HBASE_CONF_DIR", CONFIG_DIR_NAME)
-        // required by phoenix (for cases where Kerberos is enabled): see https://issues.apache.org/jira/browse/PHOENIX-2369
-        .add_env_var("HADOOP_CONF_DIR", CONFIG_DIR_NAME)
+        .add_env_vars(merged_env)
         .add_volume_mount("hbase-config", HBASE_CONFIG_TMP_DIR)
         .add_volume_mount("hdfs-discovery", HDFS_DISCOVERY_TMP_DIR)
         .add_volume_mount("log-config", HBASE_LOG_CONFIG_TMP_DIR)
@@ -973,7 +975,7 @@ fn build_rolegroup_statefulset(
 
     let metadata = ObjectMetaBuilder::new()
         .name_and_namespace(hbase)
-        .name(&rolegroup_ref.object_name())
+        .name(rolegroup_ref.object_name())
         .ownerreference_from_resource(hbase, None, Some(true))
         .context(ObjectMissingMetadataForOwnerRefSnafu)?
         .with_recommended_labels(build_recommended_labels(
@@ -1018,6 +1020,7 @@ fn build_roles(
     hbase: &HbaseCluster,
 ) -> Result<HashMap<String, (Vec<PropertyNameKind>, Role<HbaseConfigFragment>)>> {
     let config_types = vec![
+        PropertyNameKind::Env,
         PropertyNameKind::File(HBASE_ENV_SH.to_string()),
         PropertyNameKind::File(HBASE_SITE_XML.to_string()),
         PropertyNameKind::File(SSL_SERVER_XML.to_string()),

diff --git a/tests/templates/kuttl/overrides/00-limit-range.yaml b/tests/templates/kuttl/overrides/00-limit-range.yaml
@@ -0,0 +1,11 @@
+---
+apiVersion: v1
+kind: LimitRange
+metadata:
+  name: limit-request-ratio
+spec:
+  limits:
+    - type: "Container"
+      maxLimitRequestRatio:
+        cpu: 5
+        memory: 1
diff --git a/tests/templates/kuttl/overrides/00-patch-ns.yaml.j2 b/tests/templates/kuttl/overrides/00-patch-ns.yaml.j2
@@ -0,0 +1,9 @@
+{% if test_scenario['values']['openshift'] == 'true' %}
+# see https://github.com/stackabletech/issues/issues/566
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - script: kubectl patch namespace $NAMESPACE -p '{"metadata":{"labels":{"pod-security.kubernetes.io/enforce":"privileged"}}}'
+    timeout: 120
+{% endif %}
diff --git a/tests/templates/kuttl/overrides/01-assert.yaml.j2 b/tests/templates/kuttl/overrides/01-assert.yaml.j2
@@ -0,0 +1,10 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+{% if lookup('env', 'VECTOR_AGGREGATOR') %}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: vector-aggregator-discovery
+{% endif %}
diff --git a/tests/templates/kuttl/overrides/01-install-vector-aggregator-discovery-configmap.yaml.j2 b/tests/templates/kuttl/overrides/01-install-vector-aggregator-discovery-configmap.yaml.j2
@@ -0,0 +1,9 @@
+{% if lookup('env', 'VECTOR_AGGREGATOR') %}
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: vector-aggregator-discovery
+data:
+  ADDRESS: {{ lookup('env', 'VECTOR_AGGREGATOR') }}
+{% endif %}
diff --git a/tests/templates/kuttl/overrides/10-assert.yaml b/tests/templates/kuttl/overrides/10-assert.yaml
@@ -0,0 +1,12 @@
+---
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+timeout: 600
+---
+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  name: test-zk-server-default
+status:
+  readyReplicas: 1
+  replicas: 1
diff --git a/tests/templates/kuttl/overrides/10-install-zookeeper.yaml.j2 b/tests/templates/kuttl/overrides/10-install-zookeeper.yaml.j2
@@ -0,0 +1,28 @@
+---
+apiVersion: zookeeper.stackable.tech/v1alpha1
+kind: ZookeeperCluster
+metadata:
+  name: test-zk
+spec:
+  image:
+    productVersion: "{{ test_scenario['values']['zookeeper-latest'] }}"
+    pullPolicy: IfNotPresent
+{% if lookup('env', 'VECTOR_AGGREGATOR') %}
+  clusterConfig:
+    vectorAggregatorConfigMapName: vector-aggregator-discovery
+{% endif %}
+  servers:
+    config:
+      logging:
+        enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }}
+    roleGroups:
+      default:
+        replicas: 1
+---
+apiVersion: zookeeper.stackable.tech/v1alpha1
+kind: ZookeeperZnode
+metadata:
+  name: test-znode
+spec:
+  clusterRef:
+    name: test-zk