Fix technical debts (#20)

* test: Add unit test for RoleGroupBuilder::build_node_role_label

* test: Test serialization and deserialization of NodeRole

* chore: Extend comment of MAX_OBJECT_NAME_LENGTH

* chore: Remove TODO because the code is okay as it is

* Validate environment variable names

* Remove completed TODO

* Fix the remaining TODOs or remove them if not necessary

Author: siegfriedweber

rust/operator-binary/src/controller.rs

@@ -185,7 +185,6 @@ impl IsLabelValue for ValidatedCluster {
     }
 }
 
-// TODO Remove boilerplate (like derive_more)
 impl Resource for ValidatedCluster {
     type DynamicType =
         <v1alpha1::OpenSearchCluster as stackable_operator::kube::Resource>::DynamicType;

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

@@ -7,52 +7,24 @@ 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";
 
-// TODO Document how to enter config_overrides of various types, e.g. string, list, boolean,
-// object, ...
-
-// Configuration file format
-//
-// This is not well documented.
-//
-// A list setting can be written as
-// - a comma-separated list, e.g.
-//   ```
-//   setting: a,b,c
-//   ```
-//   Commas in the values cannot be escaped.
-// - a JSON list, e.g.
-//   ```
-//   setting: ["a", "b", "c"]
-//   ```
-// - a YAML list, e.g.
-//   ```
-//   setting:
-//     - a
-//     - b
-//     - c
-//   ```
-// - a (legacy) flat list, e.g.
-//   ```
-//   setting.0: a
-//   setting.1: b
-//   setting.2: b
-//   ```
-
 /// type: string
 pub const CONFIG_OPTION_CLUSTER_NAME: &str = "cluster.name";
 
-/// type: list of strings
+/// type: (comma-separated) list of strings
 pub const CONFIG_OPTION_DISCOVERY_SEED_HOSTS: &str = "discovery.seed_hosts";
 
 /// type: string
 pub const CONFIG_OPTION_DISCOVERY_TYPE: &str = "discovery.type";
 
-/// type: list of strings
+/// type: (comma-separated) list of strings
 pub const CONFIG_OPTION_INITIAL_CLUSTER_MANAGER_NODES: &str =
     "cluster.initial_cluster_manager_nodes";
 
@@ -62,10 +34,10 @@ pub const CONFIG_OPTION_NETWORK_HOST: &str = "network.host";
 /// type: string
 pub const CONFIG_OPTION_NODE_NAME: &str = "node.name";
 
-/// type: list of strings
+/// type: (comma-separated) list of strings
 pub const CONFIG_OPTION_NODE_ROLES: &str = "node.roles";
 
-/// type: list of strings
+/// type: (comma-separated) list of strings
 pub const CONFIG_OPTION_PLUGINS_SECURITY_NODES_DN: &str = "plugins.security.nodes_dn";
 
 pub struct NodeConfig {
@@ -156,17 +128,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
@@ -177,7 +152,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 {
@@ -274,7 +249,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,
     };
@@ -350,7 +325,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(),
@@ -364,44 +340,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_builder.rs

@@ -49,8 +49,6 @@ impl<'a> RoleBuilder<'a> {
         }
     }
 
-    // TODO Only one builder function which calls the other ones?
-
     pub fn role_group_builders(&self) -> Vec<RoleGroupBuilder<'_>> {
         self.cluster
             .role_group_configs

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

@@ -22,7 +22,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,
@@ -235,7 +235,8 @@ impl<'a> RoleGroupBuilder<'a> {
     }
 
     fn build_node_role_label(node_role: &v1alpha1::NodeRole) -> Label {
-        // TODO Check the maximum length at compile-time
+        // It is not possible to check the infallibility of the following statement at
+        // compile-time. Instead, it is tested in `tests::test_build_node_role_label`.
         Label::try_from((
             format!("stackable.tech/opensearch-role.{node_role}"),
             "true".to_string(),
@@ -276,13 +277,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"));
 
@@ -482,7 +483,17 @@ impl<'a> RoleGroupBuilder<'a> {
 
 #[cfg(test)]
 mod tests {
+    use strum::IntoEnumIterator;
+
     use super::RoleGroupBuilder;
+    use crate::crd::v1alpha1;
+
+    #[test]
+    fn test_build_node_role_label() {
+        for node_role in v1alpha1::NodeRole::iter() {
+            RoleGroupBuilder::build_node_role_label(&node_role);
+        }
+    }
 
     #[test]
     pub fn test_prometheus_labels() {

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/crd/mod.rs

@@ -23,7 +23,7 @@ use stackable_operator::{
     time::Duration,
     versioned::versioned,
 };
-use strum::Display;
+use strum::{Display, EnumIter};
 
 use crate::framework::{
     ClusterName, IsLabelValue, ProductName, RoleName,
@@ -76,32 +76,34 @@ pub mod versioned {
     // https://github.com/opensearch-project/ml-commons/blob/3.0.0.0/plugin/src/main/java/org/opensearch/ml/plugin/MachineLearningPlugin.java#L394.
     // If such a plugin is added, then this enumeration must be extended accordingly.
     #[derive(
-        Clone, Debug, Deserialize, Display, Eq, JsonSchema, Ord, PartialEq, PartialOrd, Serialize,
+        Clone,
+        Debug,
+        Deserialize,
+        Display,
+        EnumIter,
+        Eq,
+        JsonSchema,
+        Ord,
+        PartialEq,
+        PartialOrd,
+        Serialize,
     )]
     // The OpenSearch configuration uses snake_case. To make it easier to match the log output of
     // OpenSearch with this cluster configuration, snake_case is also used here.
     #[serde(rename_all = "snake_case")]
+    #[strum(serialize_all = "snake_case")]
     pub enum NodeRole {
         // Built-in node roles
         // see https://github.com/opensearch-project/OpenSearch/blob/3.0.0/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java#L341-L346
-
-        // TODO https://github.com/Peternator7/strum/issues/113
-        #[strum(serialize = "cluster_manager")]
         ClusterManager,
-        #[strum(serialize = "coordinating_only")]
         CoordinatingOnly,
-        #[strum(serialize = "data")]
         Data,
-        #[strum(serialize = "ingest")]
         Ingest,
-        #[strum(serialize = "remote_cluster_client")]
         RemoteClusterClient,
-        #[strum(serialize = "warm")]
         Warm,
 
         // Search node role
         // see https://github.com/opensearch-project/OpenSearch/blob/3.0.0/server/src/main/java/org/opensearch/cluster/node/DiscoveryNodeRole.java#L313-L339
-        #[strum(serialize = "search")]
         Search,
     }
 
@@ -257,3 +259,29 @@ impl NodeRoles {
 }
 
 impl Atomic for NodeRoles {}
+
+#[cfg(test)]
+mod tests {
+    use crate::crd::v1alpha1;
+
+    #[test]
+    fn test_node_role() {
+        assert_eq!(
+            String::from("cluster_manager"),
+            v1alpha1::NodeRole::ClusterManager.to_string()
+        );
+        assert_eq!(
+            String::from("cluster_manager"),
+            format!("{}", v1alpha1::NodeRole::ClusterManager)
+        );
+        assert_eq!(
+            "\"cluster_manager\"",
+            serde_json::to_string(&v1alpha1::NodeRole::ClusterManager)
+                .expect("should be serializable")
+        );
+        assert_eq!(
+            v1alpha1::NodeRole::ClusterManager,
+            serde_json::from_str("\"cluster_manager\"").expect("should be deserializable")
+        );
+    }
+}

rust/operator-binary/src/framework.rs

@@ -33,8 +33,9 @@ pub enum Error {
     },
 }
 
-// TODO The maximum length of objects differs.
 /// Maximum length of a DNS subdomain name as defined in RFC 1123.
+/// The object names of most types, e.g. ConfigMap and StatefulSet, must not exceed this limit.
+/// However, there are kinds that allow longer object names, e.g. ClusterRole.
 #[allow(dead_code)]
 pub const MAX_OBJECT_NAME_LENGTH: usize = 253;