Validate environment variable names

Author: siegfriedweber

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

@@ -5,7 +5,10 @@ use super::ValidatedCluster;
 use crate::{
     controller::OpenSearchRoleGroupConfig,
     crd::v1alpha1,
-    framework::{builder::pod::container::EnvVarSet, role_group_utils},
+    framework::{
+        builder::pod::container::{EnvVarName, EnvVarSet},
+        role_group_utils,
+    },
 };
 
 pub const CONFIGURATION_FILE_OPENSEARCH_YML: &str = "opensearch.yml";
@@ -132,17 +135,20 @@ impl NodeConfig {
         EnvVarSet::new()
             // Set the OpenSearch node name to the Pod name.
             // The node name is used e.g. for `{INITIAL_CLUSTER_MANAGER_NODES}`.
-            .with_field_path(CONFIG_OPTION_NODE_NAME, FieldPathEnvVar::Name)
+            .with_field_path(
+                EnvVarName::from_str_unsafe(CONFIG_OPTION_NODE_NAME),
+                FieldPathEnvVar::Name,
+            )
             .with_value(
-                CONFIG_OPTION_DISCOVERY_SEED_HOSTS,
+                EnvVarName::from_str_unsafe(CONFIG_OPTION_DISCOVERY_SEED_HOSTS),
                 &self.discovery_service_name,
             )
             .with_value(
-                CONFIG_OPTION_INITIAL_CLUSTER_MANAGER_NODES,
+                EnvVarName::from_str_unsafe(CONFIG_OPTION_INITIAL_CLUSTER_MANAGER_NODES),
                 self.initial_cluster_manager_nodes(),
             )
             .with_value(
-                CONFIG_OPTION_NODE_ROLES,
+                EnvVarName::from_str_unsafe(CONFIG_OPTION_NODE_ROLES),
                 self.role_group_config
                     .config
                     .node_roles
@@ -153,7 +159,7 @@ impl NodeConfig {
                     // is safe.
                     .join(","),
             )
-            .with_values(self.role_group_config.env_overrides.clone())
+            .merge(self.role_group_config.env_overrides.clone())
     }
 
     fn to_yaml(kv: serde_json::Map<String, Value>) -> String {
@@ -250,7 +256,7 @@ mod tests {
             affinity::StackableAffinity, product_image_selection::ProductImage,
             resources::Resources,
         },
-        k8s_openapi::api::core::v1::{EnvVar, EnvVarSource, ObjectFieldSelector, PodTemplateSpec},
+        k8s_openapi::api::core::v1::PodTemplateSpec,
         kube::api::ObjectMeta,
         role_utils::GenericRoleConfig,
     };
@@ -289,7 +295,8 @@ mod tests {
                 listener_class: "cluster-internal".to_string(),
             },
             config_overrides: HashMap::default(),
-            env_overrides: [("TEST".to_owned(), "value".to_owned())].into(),
+            env_overrides: EnvVarSet::new()
+                .with_value(EnvVarName::from_str_unsafe("TEST"), "value"),
             cli_overrides: BTreeMap::default(),
             pod_overrides: PodTemplateSpec::default(),
             product_specific_common_config: GenericProductSpecificCommonConfig::default(),
@@ -303,44 +310,23 @@ mod tests {
 
         let env_vars = node_config.environment_variables();
 
-        // TODO Test EnvVarSet and compare EnvVarSets
         assert_eq!(
-            vec![
-                EnvVar {
-                    name: "TEST".to_owned(),
-                    value: Some("value".to_owned()),
-                    value_from: None
-                },
-                EnvVar {
-                    name: "cluster.initial_cluster_manager_nodes".to_owned(),
-                    value: Some("".to_owned()),
-                    value_from: None
-                },
-                EnvVar {
-                    name: "discovery.seed_hosts".to_owned(),
-                    value: Some("my-opensearch-cluster-manager".to_owned()),
-                    value_from: None
-                },
-                EnvVar {
-                    name: "node.name".to_owned(),
-                    value: None,
-                    value_from: Some(EnvVarSource {
-                        config_map_key_ref: None,
-                        field_ref: Some(ObjectFieldSelector {
-                            api_version: None,
-                            field_path: "metadata.name".to_owned()
-                        }),
-                        resource_field_ref: None,
-                        secret_key_ref: None
-                    })
-                },
-                EnvVar {
-                    name: "node.roles".to_owned(),
-                    value: Some("".to_owned()),
-                    value_from: None
-                }
-            ],
-            <EnvVarSet as Into<Vec<EnvVar>>>::into(env_vars)
+            EnvVarSet::new()
+                .with_value(EnvVarName::from_str_unsafe("TEST"), "value",)
+                .with_value(
+                    EnvVarName::from_str_unsafe("cluster.initial_cluster_manager_nodes"),
+                    "",
+                )
+                .with_value(
+                    EnvVarName::from_str_unsafe("discovery.seed_hosts"),
+                    "my-opensearch-cluster-manager",
+                )
+                .with_field_path(
+                    EnvVarName::from_str_unsafe("node.name"),
+                    FieldPathEnvVar::Name
+                )
+                .with_value(EnvVarName::from_str_unsafe("node.roles"), "",),
+            env_vars
         );
     }
 }

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

@@ -23,7 +23,7 @@ use crate::{
     crd::v1alpha1,
     framework::{
         RoleGroupName,
-        builder::meta::ownerreference_from_resource,
+        builder::{meta::ownerreference_from_resource, pod::container::EnvVarName},
         kvp::label::{recommended_labels, role_group_selector, role_selector},
         listener::listener_pvc,
         role_group_utils::ResourceNames,
@@ -275,13 +275,13 @@ impl<'a> RoleGroupBuilder<'a> {
 
         // Use `OPENSEARCH_HOME` from envOverrides or default to `DEFAULT_OPENSEARCH_HOME`.
         let opensearch_home = env_vars
-            .get_env_var("OPENSEARCH_HOME")
+            .get(EnvVarName::from_str_unsafe("OPENSEARCH_HOME"))
             .and_then(|env_var| env_var.value.clone())
             .unwrap_or(DEFAULT_OPENSEARCH_HOME.to_owned());
         // Use `OPENSEARCH_PATH_CONF` from envOverrides or default to `{OPENSEARCH_HOME}/config`,
         // i.e. depend on `OPENSEARCH_HOME`.
         let opensearch_path_conf = env_vars
-            .get_env_var("OPENSEARCH_PATH_CONF")
+            .get(EnvVarName::from_str_unsafe("OPENSEARCH_PATH_CONF"))
             .and_then(|env_var| env_var.value.clone())
             .unwrap_or(format!("{opensearch_home}/config"));
 

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

@@ -16,6 +16,7 @@ use crate::{
     crd::v1alpha1::{self, OpenSearchConfig, OpenSearchConfigFragment},
     framework::{
         ClusterName,
+        builder::pod::container::{EnvVarName, EnvVarSet},
         role_utils::{GenericProductSpecificCommonConfig, RoleGroupConfig, with_validated_config},
     },
 };
@@ -41,6 +42,11 @@ pub enum Error {
     #[snafu(display("failed to set role-group name"))]
     ParseRoleGroupName { source: crate::framework::Error },
 
+    #[snafu(display("failed to parse environment variable"))]
+    ParseEnvironmentVariable {
+        source: crate::framework::builder::pod::container::Error,
+    },
+
     #[snafu(display("fragment validation failure"))]
     ValidateOpenSearchConfig {
         source: stackable_operator::config::fragment::ValidationError,
@@ -125,12 +131,21 @@ fn validate_role_group_config(
         listener_class: merged_role_group.config.config.listener_class,
     };
 
+    let mut env_overrides = EnvVarSet::new();
+
+    for (env_var_name, env_var_value) in merged_role_group.config.env_overrides {
+        env_overrides = env_overrides.with_value(
+            EnvVarName::from_str(&env_var_name).context(ParseEnvironmentVariableSnafu)?,
+            env_var_value,
+        );
+    }
+
     Ok(RoleGroupConfig {
         // Kubernetes defaults to 1 if not set
         replicas: merged_role_group.replicas.unwrap_or(1),
         config: validated_config,
         config_overrides: merged_role_group.config.config_overrides,
-        env_overrides: merged_role_group.config.env_overrides,
+        env_overrides,
         cli_overrides: merged_role_group.config.cli_overrides,
         pod_overrides: merged_role_group.config.pod_overrides,
         product_specific_common_config: merged_role_group.config.product_specific_common_config,

rust/operator-binary/src/framework/builder/pod/container.rs

@@ -1,12 +1,53 @@
-use std::collections::BTreeMap;
+use std::{collections::BTreeMap, fmt::Display, str::FromStr};
 
+use snafu::Snafu;
 use stackable_operator::{
     builder::pod::container::FieldPathEnvVar,
     k8s_openapi::api::core::v1::{EnvVar, EnvVarSource, ObjectFieldSelector},
 };
+use strum::{EnumDiscriminants, IntoStaticStr};
+
+#[derive(Snafu, Debug, EnumDiscriminants)]
+#[strum_discriminants(derive(IntoStaticStr))]
+pub enum Error {
+    #[snafu(display(
+        "invalid environment variable name: a valid environment variable name must consist only of printable ASCII characters other than '='"
+    ))]
+    ParseEnvVarName { env_var_name: String },
+}
+
+#[derive(Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
+pub struct EnvVarName(String);
+
+impl EnvVarName {
+    pub fn from_str_unsafe(s: &str) -> Self {
+        EnvVarName::from_str(s).expect("should be a valid environment variable name")
+    }
+}
+
+impl Display for EnvVarName {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl FromStr for EnvVarName {
+    type Err = Error;
 
-// TODO Use validated type
-type EnvVarName = String;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        // The length of the environment variable names seems not to be restricted.
+
+        if s.find(|c: char| !c.is_ascii_graphic() || c == '=')
+            .is_none()
+        {
+            Ok(Self(s.to_owned()))
+        } else {
+            Err(Error::ParseEnvVarName {
+                env_var_name: s.to_owned(),
+            })
+        }
+    }
+}
 
 #[derive(Clone, Debug, Default, PartialEq)]
 pub struct EnvVarSet(BTreeMap<EnvVarName, EnvVar>);
@@ -16,16 +57,10 @@ impl EnvVarSet {
         Self::default()
     }
 
-    pub fn get_env_var(&self, env_var_name: impl Into<EnvVarName>) -> Option<&EnvVar> {
+    pub fn get(&self, env_var_name: impl Into<EnvVarName>) -> Option<&EnvVar> {
         self.0.get(&env_var_name.into())
     }
 
-    pub fn add_env_var(mut self, env_var: EnvVar) -> Self {
-        self.0.insert(env_var.name.clone(), env_var);
-
-        self
-    }
-
     pub fn merge(mut self, mut env_var_set: EnvVarSet) -> Self {
         self.0.append(&mut env_var_set.0);
 
@@ -51,7 +86,7 @@ impl EnvVarSet {
         self.0.insert(
             name.clone(),
             EnvVar {
-                name,
+                name: name.to_string(),
                 value: Some(value.into()),
                 value_from: None,
             },
@@ -70,7 +105,7 @@ impl EnvVarSet {
         self.0.insert(
             name.clone(),
             EnvVar {
-                name,
+                name: name.to_string(),
                 value: None,
                 value_from: Some(EnvVarSource {
                     field_ref: Some(ObjectFieldSelector {

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

@@ -11,7 +11,7 @@ use stackable_operator::{
     schemars::JsonSchema,
 };
 
-use super::ProductName;
+use super::{ProductName, builder::pod::container::EnvVarSet};
 use crate::framework::{ClusterName, MAX_OBJECT_NAME_LENGTH, kvp::label::MAX_LABEL_VALUE_LENGTH};
 
 #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)]
@@ -27,7 +27,7 @@ pub struct RoleGroupConfig<ProductSpecificCommonConfig, T> {
     pub replicas: u16,
     pub config: T,
     pub config_overrides: HashMap<String, HashMap<String, String>>,
-    pub env_overrides: HashMap<String, String>,
+    pub env_overrides: EnvVarSet,
     pub cli_overrides: BTreeMap<String, String>,
     pub pod_overrides: PodTemplateSpec,
     // allow(dead_code) is not necessary anymore when moved to operator-rs