fix: env overrides work as expected (#499)

* fix: env overrides work as expected

* Change release.yaml for jenkins

* Revert "Change release.yaml for jenkins"

This reverts commit f1e9f97.

* unit test env overrides

* Update cargo.lock

Author: razvan

CHANGELOG.md

@@ -30,6 +30,7 @@ All notable changes to this project will be documented in this file.
 
 - Include hdfs principals `dfs.journalnode.kerberos.principal`, `dfs.namenode.kerberos.principal`
   and `dfs.datanode.kerberos.principal` in the discovery ConfigMap in case Kerberos is enabled ([#451]).
+- User provided env overrides now work as expected ([#499]).
 
 [#429]: https://github.com/stackabletech/hdfs-operator/pull/429
 [#450]: https://github.com/stackabletech/hdfs-operator/pull/450
@@ -43,6 +44,7 @@ All notable changes to this project will be documented in this file.
 [#491]: https://github.com/stackabletech/hdfs-operator/pull/491
 [#492]: https://github.com/stackabletech/hdfs-operator/pull/492
 [#495]: https://github.com/stackabletech/hdfs-operator/pull/495
+[#499]: https://github.com/stackabletech/hdfs-operator/pull/499
 
 ## [23.11.0] - 2023-11-24
 

Cargo.lock

@@ -27,6 +27,7 @@ tracing.workspace = true
 
 [dev-dependencies]
 rstest.workspace = true
+serde_yaml.workspace = true
 
 [build-dependencies]
 built.workspace = true

rust/operator-binary/Cargo.toml

@@ -741,12 +741,18 @@ wait_for_termination $!
         env_overrides: Option<&BTreeMap<String, String>>,
         resources: Option<&ResourceRequirements>,
     ) -> Vec<EnvVar> {
-        let mut env = Vec::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 env: BTreeMap<String, EnvVar> = BTreeMap::new();
 
-        env.extend(Self::shared_env_vars(
-            self.volume_mount_dirs().final_config(),
-            zookeeper_config_map_name,
-        ));
+        env.extend(
+            Self::shared_env_vars(
+                self.volume_mount_dirs().final_config(),
+                zookeeper_config_map_name,
+            )
+            .into_iter()
+            .map(|env_var| (env_var.name.clone(), env_var)),
+        );
 
         // For the main container we use specialized env variables for every role
         // (think of like HDFS_NAMENODE_OPTS or HDFS_DATANODE_OPTS)
@@ -758,39 +764,61 @@ wait_for_termination $!
         // so we don't want to stuff all the config into HADOOP_OPTS, but rather into the specialized env variables
         // See https://github.com/stackabletech/hdfs-operator/issues/138 for details
         if let ContainerConfig::Hdfs { role, .. } = self {
-            env.push(EnvVar {
-                name: role.hadoop_opts_env_var_for_role().to_string(),
-                value: self.build_hadoop_opts(hdfs, resources).ok(),
-                ..EnvVar::default()
-            });
+            let role_opts_name = role.hadoop_opts_env_var_for_role().to_string();
+            env.insert(
+                role_opts_name.clone(),
+                EnvVar {
+                    name: role_opts_name,
+                    value: self.build_hadoop_opts(hdfs, resources).ok(),
+                    ..EnvVar::default()
+                },
+            );
         }
         // Additionally, any other init or sidecar container must have access to the following settings.
         // As the Prometheus metric emitter is not part of this config it's safe to use for hdfs cli tools as well.
         // This will not only enable the init containers to work, but also the user to run e.g.
         // `bin/hdfs dfs -ls /` without getting `Caused by: java.lang.IllegalArgumentException: KrbException: Cannot locate default realm`
         // because the `-Djava.security.krb5.conf` setting is missing
         if hdfs.has_kerberos_enabled() {
-            env.push(EnvVar {
-                name: "HADOOP_OPTS".to_string(),
-                value: Some("-Djava.security.krb5.conf=/stackable/kerberos/krb5.conf".to_string()),
-                ..EnvVar::default()
-            });
+            env.insert(
+                "HADOOP_OPTS".to_string(),
+                EnvVar {
+                    name: "HADOOP_OPTS".to_string(),
+                    value: Some(
+                        "-Djava.security.krb5.conf=/stackable/kerberos/krb5.conf".to_string(),
+                    ),
+                    ..EnvVar::default()
+                },
+            );
 
-            env.push(EnvVar {
-                name: "KRB5_CONFIG".to_string(),
-                value: Some("/stackable/kerberos/krb5.conf".to_string()),
-                ..EnvVar::default()
-            });
-            env.push(EnvVar {
-                name: "KRB5_CLIENT_KTNAME".to_string(),
-                value: Some("/stackable/kerberos/keytab".to_string()),
-                ..EnvVar::default()
-            });
+            env.insert(
+                "KRB5_CONFIG".to_string(),
+                EnvVar {
+                    name: "KRB5_CONFIG".to_string(),
+                    value: Some("/stackable/kerberos/krb5.conf".to_string()),
+                    ..EnvVar::default()
+                },
+            );
+            env.insert(
+                "KRB5_CLIENT_KTNAME".to_string(),
+                EnvVar {
+                    name: "KRB5_CLIENT_KTNAME".to_string(),
+                    value: Some("/stackable/kerberos/keytab".to_string()),
+                    ..EnvVar::default()
+                },
+            );
         }
 
         // Overrides need to come last
-        env.extend(Self::transform_env_overrides_to_env_vars(env_overrides));
-        env
+        let mut env_override_vars: BTreeMap<String, EnvVar> =
+            Self::transform_env_overrides_to_env_vars(env_overrides)
+                .into_iter()
+                .map(|env_var| (env_var.name.clone(), env_var))
+                .collect();
+
+        env.append(&mut env_override_vars);
+
+        env.into_values().collect()
     }
 
     /// Returns the container resources.

rust/operator-binary/src/container.rs

@@ -846,3 +846,115 @@ fn rolegroup_statefulset(
 pub fn error_policy(_obj: Arc<HdfsCluster>, _error: &Error, _ctx: Arc<Ctx>) -> Action {
     Action::requeue(*Duration::from_secs(5))
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    pub fn test_env_overrides() {
+        let cr = "
+---
+apiVersion: hdfs.stackable.tech/v1alpha1
+kind: HdfsCluster
+metadata:
+  name: hdfs
+spec:
+  image:
+    productVersion: 3.3.6
+  clusterConfig:
+    zookeeperConfigMapName: hdfs-zk
+  nameNodes:
+    roleGroups:
+      default:
+        replicas: 1
+  journalNodes:
+    roleGroups:
+      default:
+        replicas: 1
+  dataNodes:
+    roleGroups:
+      default:
+        envOverrides:
+          MY_ENV: my-value
+          HADOOP_HOME: /not/the/default/path
+        replicas: 1
+";
+        let product_config = "
+---
+version: 0.1.0
+spec:
+  units: []
+properties: []
+";
+
+        let hdfs: HdfsCluster = serde_yaml::from_str(cr).unwrap();
+
+        let config =
+            transform_all_roles_to_config(&hdfs, hdfs.build_role_properties().unwrap()).unwrap();
+
+        let validated_config = validate_all_roles_and_groups_config(
+            "3.3.6",
+            &config,
+            &ProductConfigManager::from_str(product_config).unwrap(),
+            false,
+            false,
+        )
+        .unwrap();
+
+        let role = HdfsRole::DataNode;
+        let rolegroup_config = validated_config
+            .get(&role.to_string())
+            .unwrap()
+            .get("default")
+            .unwrap();
+        let env_overrides = rolegroup_config.get(&PropertyNameKind::Env);
+
+        let merged_config = role.merged_config(&hdfs, "default").unwrap();
+        let resolved_product_image = hdfs.spec.image.resolve(DOCKER_IMAGE_BASE_NAME, "0.0.0-dev");
+
+        let mut pb = PodBuilder::new();
+        pb.metadata(ObjectMeta::default());
+        ContainerConfig::add_containers_and_volumes(
+            &mut pb,
+            &hdfs,
+            &role,
+            &resolved_product_image,
+            &merged_config,
+            env_overrides,
+            &hdfs.spec.cluster_config.zookeeper_config_map_name,
+            "todo",
+            &[],
+        )
+        .unwrap();
+        let containers = pb.build().unwrap().spec.unwrap().containers;
+        let main_container = containers
+            .iter()
+            .find(|c| c.name == role.to_string())
+            .unwrap();
+
+        assert_eq!(
+            main_container
+                .env
+                .clone()
+                .unwrap()
+                .into_iter()
+                .find(|e| e.name == "MY_ENV")
+                .unwrap()
+                .value,
+            Some("my-value".to_string())
+        );
+
+        assert_eq!(
+            main_container
+                .env
+                .clone()
+                .unwrap()
+                .into_iter()
+                .find(|e| e.name == "HADOOP_HOME")
+                .unwrap()
+                .value,
+            Some("/not/the/default/path".to_string())
+        );
+    }
+}