diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 4e23072..4d27a3a 100644 --- a/rust/operator-binary/src/controller.rs +++ b/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 = ::DynamicType; diff --git a/rust/operator-binary/src/controller/build/node_config.rs b/rust/operator-binary/src/controller/build/node_config.rs index 6eb9ba6..66a4fb9 100644 --- a/rust/operator-binary/src/controller/build/node_config.rs +++ b/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 { @@ -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 - } - ], - >>::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 ); } } diff --git a/rust/operator-binary/src/controller/build/role_group_builder.rs b/rust/operator-binary/src/controller/build/role_group_builder.rs index 49d5f22..9b561da 100644 --- a/rust/operator-binary/src/controller/build/role_group_builder.rs +++ b/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, @@ -233,7 +233,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(), @@ -274,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")); @@ -444,3 +445,17 @@ impl<'a> RoleGroupBuilder<'a> { ) } } + +#[cfg(test)] +mod tests { + use strum::IntoEnumIterator; + + use crate::{controller::build::role_group_builder::RoleGroupBuilder, crd::v1alpha1}; + + #[test] + fn test_build_node_role_label() { + for node_role in v1alpha1::NodeRole::iter() { + RoleGroupBuilder::build_node_role_label(&node_role); + } + } +} diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 959ef80..00b3f60 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/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, diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 84a6efa..babb645 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/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") + ); + } +} diff --git a/rust/operator-binary/src/framework.rs b/rust/operator-binary/src/framework.rs index 07c5699..42d38dc 100644 --- a/rust/operator-binary/src/framework.rs +++ b/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; diff --git a/rust/operator-binary/src/framework/builder/pod/container.rs b/rust/operator-binary/src/framework/builder/pod/container.rs index 22f0fa6..67ee112 100644 --- a/rust/operator-binary/src/framework/builder/pod/container.rs +++ b/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 { + // 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); @@ -16,16 +57,10 @@ impl EnvVarSet { Self::default() } - pub fn get_env_var(&self, env_var_name: impl Into) -> Option<&EnvVar> { + pub fn get(&self, env_var_name: impl Into) -> 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 { diff --git a/rust/operator-binary/src/framework/role_utils.rs b/rust/operator-binary/src/framework/role_utils.rs index db0c5d7..3539d3b 100644 --- a/rust/operator-binary/src/framework/role_utils.rs +++ b/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 { pub replicas: u16, pub config: T, pub config_overrides: HashMap>, - pub env_overrides: HashMap, + pub env_overrides: EnvVarSet, pub cli_overrides: BTreeMap, pub pod_overrides: PodTemplateSpec, // allow(dead_code) is not necessary anymore when moved to operator-rs