Skip to content

Commit 7f21cef

Browse files
siegfriedwebersiegfriedweber
committed
Validate node roles; Fix coordinating_only node role
1 parent 4323261 commit 7f21cef

8 files changed

Lines changed: 219 additions & 107 deletions

File tree

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

Lines changed: 53 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,12 @@
33
//! The cluster specification is validated, Kubernetes resource specifications are created and
44
//! applied and the cluster status is updated.
55
6-
use std::{collections::BTreeMap, marker::PhantomData, str::FromStr, sync::Arc};
6+
use std::{
7+
collections::{BTreeMap, BTreeSet},
8+
marker::PhantomData,
9+
str::FromStr,
10+
sync::Arc,
11+
};
712

813
use apply::Applier;
914
use build::build;
@@ -26,13 +31,13 @@ use stackable_operator::{
2631
logging::controller::ReconcilerError,
2732
shared::time::Duration,
2833
};
29-
use strum::{EnumDiscriminants, IntoStaticStr};
34+
use strum::{Display, EnumDiscriminants, EnumIter, IntoStaticStr};
3035
use update_status::update_status;
3136
use validate::validate;
3237

3338
use crate::{
3439
controller::preprocess::preprocess,
35-
crd::{NodeRoles, v1alpha1},
40+
crd::v1alpha1,
3641
framework::{
3742
HasName, HasUid, NameIsValidLabelValue,
3843
product_logging::framework::{ValidatedContainerLogConfigChoice, VectorContainerLogConfig},
@@ -159,7 +164,7 @@ pub struct ValidatedOpenSearchConfig {
159164
pub discovery_service_exposed: bool,
160165
pub listener_class: ListenerClassName,
161166
pub logging: ValidatedLogging,
162-
pub node_roles: NodeRoles,
167+
pub node_roles: ValidatedNodeRoles,
163168
pub requested_secret_lifetime: Duration,
164169
pub resources: OpenSearchNodeResources,
165170
pub termination_grace_period_seconds: i64,
@@ -178,6 +183,23 @@ impl ValidatedLogging {
178183
}
179184
}
180185

186+
/// Set of validated node roles
187+
///
188+
/// An empty set specifies a coordinating only node.
189+
type ValidatedNodeRoles = BTreeSet<ValidatedNodeRole>;
190+
191+
/// Validated node role
192+
#[derive(Clone, Copy, Debug, Display, EnumIter, Eq, PartialEq, PartialOrd, Ord)]
193+
#[strum(serialize_all = "snake_case")]
194+
pub enum ValidatedNodeRole {
195+
ClusterManager,
196+
Data,
197+
Ingest,
198+
RemoteClusterClient,
199+
Search,
200+
Warm,
201+
}
202+
181203
/// Validated security configuration
182204
#[derive(Clone, Debug, PartialEq)]
183205
pub enum ValidatedSecurity {
@@ -285,7 +307,7 @@ impl ValidatedCluster {
285307
/// Returns all role-group configurations which contain the given node role
286308
pub fn role_group_configs_filtered_by_node_role(
287309
&self,
288-
node_role: &v1alpha1::NodeRole,
310+
node_role: &ValidatedNodeRole,
289311
) -> BTreeMap<RoleGroupName, OpenSearchRoleGroupConfig> {
290312
self.role_group_configs
291313
.clone()
@@ -481,8 +503,11 @@ mod tests {
481503

482504
use super::{Context, OpenSearchRoleGroupConfig, ValidatedCluster, ValidatedLogging};
483505
use crate::{
484-
controller::{OpenSearchNodeResources, ValidatedOpenSearchConfig, ValidatedSecurity},
485-
crd::{NodeRoles, v1alpha1},
506+
controller::{
507+
OpenSearchNodeResources, ValidatedNodeRole, ValidatedNodeRoles,
508+
ValidatedOpenSearchConfig, ValidatedSecurity,
509+
},
510+
crd::v1alpha1,
486511
framework::{
487512
builder::pod::container::EnvVarSet,
488513
product_logging::framework::ValidatedContainerLogConfigChoice,
@@ -533,26 +558,26 @@ mod tests {
533558
RoleGroupName::from_str_unsafe("data1"),
534559
role_group_config(
535560
4,
536-
&[
537-
v1alpha1::NodeRole::Ingest,
538-
v1alpha1::NodeRole::Data,
539-
v1alpha1::NodeRole::RemoteClusterClient,
561+
[
562+
ValidatedNodeRole::Ingest,
563+
ValidatedNodeRole::Data,
564+
ValidatedNodeRole::RemoteClusterClient,
540565
],
541566
),
542567
),
543568
(
544569
RoleGroupName::from_str_unsafe("data2"),
545570
role_group_config(
546571
6,
547-
&[
548-
v1alpha1::NodeRole::Ingest,
549-
v1alpha1::NodeRole::Data,
550-
v1alpha1::NodeRole::RemoteClusterClient,
572+
[
573+
ValidatedNodeRole::Ingest,
574+
ValidatedNodeRole::Data,
575+
ValidatedNodeRole::RemoteClusterClient,
551576
],
552577
),
553578
),
554579
]),
555-
validated_cluster.role_group_configs_filtered_by_node_role(&v1alpha1::NodeRole::Data)
580+
validated_cluster.role_group_configs_filtered_by_node_role(&ValidatedNodeRole::Data)
556581
);
557582
}
558583

@@ -574,31 +599,31 @@ mod tests {
574599
[
575600
(
576601
RoleGroupName::from_str_unsafe("coordinating"),
577-
role_group_config(5, &[v1alpha1::NodeRole::CoordinatingOnly]),
602+
role_group_config(5, []),
578603
),
579604
(
580605
RoleGroupName::from_str_unsafe("cluster-manager"),
581-
role_group_config(3, &[v1alpha1::NodeRole::ClusterManager]),
606+
role_group_config(3, [ValidatedNodeRole::ClusterManager]),
582607
),
583608
(
584609
RoleGroupName::from_str_unsafe("data1"),
585610
role_group_config(
586611
4,
587-
&[
588-
v1alpha1::NodeRole::Ingest,
589-
v1alpha1::NodeRole::Data,
590-
v1alpha1::NodeRole::RemoteClusterClient,
612+
[
613+
ValidatedNodeRole::Ingest,
614+
ValidatedNodeRole::Data,
615+
ValidatedNodeRole::RemoteClusterClient,
591616
],
592617
),
593618
),
594619
(
595620
RoleGroupName::from_str_unsafe("data2"),
596621
role_group_config(
597622
6,
598-
&[
599-
v1alpha1::NodeRole::Ingest,
600-
v1alpha1::NodeRole::Data,
601-
v1alpha1::NodeRole::RemoteClusterClient,
623+
[
624+
ValidatedNodeRole::Ingest,
625+
ValidatedNodeRole::Data,
626+
ValidatedNodeRole::RemoteClusterClient,
602627
],
603628
),
604629
),
@@ -616,7 +641,7 @@ mod tests {
616641

617642
fn role_group_config(
618643
replicas: u16,
619-
node_roles: &[v1alpha1::NodeRole],
644+
node_roles: impl Into<ValidatedNodeRoles>,
620645
) -> OpenSearchRoleGroupConfig {
621646
OpenSearchRoleGroupConfig {
622647
replicas,
@@ -630,7 +655,7 @@ mod tests {
630655
),
631656
vector_container: None,
632657
},
633-
node_roles: NodeRoles(node_roles.to_vec()),
658+
node_roles: node_roles.into(),
634659
requested_secret_lifetime: Duration::from_str("1d")
635660
.expect("should be a valid duration"),
636661
resources: OpenSearchNodeResources::default(),

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

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,9 @@ mod tests {
8282
controller::{
8383
ContextNames, OpenSearchNodeResources, OpenSearchRoleGroupConfig, ValidatedCluster,
8484
ValidatedContainerLogConfigChoice, ValidatedDiscoveryEndpoint, ValidatedLogging,
85-
ValidatedOpenSearchConfig, ValidatedSecurity,
85+
ValidatedNodeRole, ValidatedNodeRoles, ValidatedOpenSearchConfig, ValidatedSecurity,
8686
},
87-
crd::{NodeRoles, v1alpha1},
87+
crd::v1alpha1,
8888
framework::{
8989
builder::pod::container::EnvVarSet,
9090
role_utils::GenericProductSpecificCommonConfig,
@@ -190,20 +190,20 @@ mod tests {
190190
[
191191
(
192192
RoleGroupName::from_str_unsafe("coordinating"),
193-
role_group_config(5, &[v1alpha1::NodeRole::CoordinatingOnly]),
193+
role_group_config(5, []),
194194
),
195195
(
196196
RoleGroupName::from_str_unsafe("cluster-manager"),
197-
role_group_config(3, &[v1alpha1::NodeRole::ClusterManager]),
197+
role_group_config(3, [ValidatedNodeRole::ClusterManager]),
198198
),
199199
(
200200
RoleGroupName::from_str_unsafe("data"),
201201
role_group_config(
202202
8,
203-
&[
204-
v1alpha1::NodeRole::Ingest,
205-
v1alpha1::NodeRole::Data,
206-
v1alpha1::NodeRole::RemoteClusterClient,
203+
[
204+
ValidatedNodeRole::Ingest,
205+
ValidatedNodeRole::Data,
206+
ValidatedNodeRole::RemoteClusterClient,
207207
],
208208
),
209209
),
@@ -220,7 +220,7 @@ mod tests {
220220

221221
fn role_group_config(
222222
replicas: u16,
223-
node_roles: &[v1alpha1::NodeRole],
223+
node_roles: impl Into<ValidatedNodeRoles>,
224224
) -> OpenSearchRoleGroupConfig {
225225
OpenSearchRoleGroupConfig {
226226
replicas,
@@ -234,7 +234,7 @@ mod tests {
234234
),
235235
vector_container: None,
236236
},
237-
node_roles: NodeRoles(node_roles.to_vec()),
237+
node_roles: node_roles.into(),
238238
requested_secret_lifetime: Duration::from_str("1d")
239239
.expect("should be a valid duration"),
240240
resources: OpenSearchNodeResources::default(),

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

Lines changed: 62 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,10 @@ use tracing::warn;
88

99
use super::ValidatedCluster;
1010
use crate::{
11-
controller::{OpenSearchRoleGroupConfig, build::role_group_builder::RoleGroupSecurityMode},
11+
controller::{
12+
OpenSearchRoleGroupConfig, ValidatedNodeRole,
13+
build::role_group_builder::RoleGroupSecurityMode,
14+
},
1215
crd::v1alpha1,
1316
framework::{
1417
builder::pod::container::{EnvVarName, EnvVarSet},
@@ -372,15 +375,16 @@ impl NodeConfig {
372375
)
373376
.with_value(
374377
&EnvVarName::from_str_unsafe(CONFIG_OPTION_NODE_ROLES),
375-
self.role_group_config
376-
.config
377-
.node_roles
378-
.iter()
379-
.map(|node_role| format!("{node_role}"))
380-
.collect::<Vec<_>>()
381-
// Node roles cannot contain commas, therefore creating a comma-separated list
382-
// is safe.
383-
.join(","),
378+
Self::to_comma_separated_list(
379+
&self
380+
.role_group_config
381+
.config
382+
.node_roles
383+
.iter()
384+
.map(|node_role| format!("{node_role}"))
385+
.collect::<Vec<_>>(),
386+
)
387+
.expect("Node roles cannot contain commas, therefore creating a comma-separated list is safe."),
384388
);
385389

386390
if let Some(initial_cluster_manager_nodes) = self.initial_cluster_manager_nodes() {
@@ -476,13 +480,13 @@ impl NodeConfig {
476480
.role_group_config
477481
.config
478482
.node_roles
479-
.contains(&v1alpha1::NodeRole::ClusterManager)
483+
.contains(&ValidatedNodeRole::ClusterManager)
480484
{
481485
None
482486
} else {
483487
let cluster_manager_configs = self
484488
.cluster
485-
.role_group_configs_filtered_by_node_role(&v1alpha1::NodeRole::ClusterManager);
489+
.role_group_configs_filtered_by_node_role(&ValidatedNodeRole::ClusterManager);
486490

487491
// This setting requires node names as set in NODE_NAME.
488492
// The node names are set to the pod names with
@@ -501,8 +505,8 @@ impl NodeConfig {
501505
.map(|i| format!("{}-{i}", role_group_resource_names.stateful_set_name())),
502506
);
503507
}
504-
// Pod names cannot contain commas, therefore creating a comma-separated list is safe.
505-
Some(pod_names.join(","))
508+
509+
Some(Self::to_comma_separated_list(&pod_names).expect("Pod names cannot contain commas, therefore creating a comma-separated list is safe."))
506510
}
507511
}
508512

@@ -522,6 +526,16 @@ impl NodeConfig {
522526
.and_then(|env_var| env_var.value.clone())
523527
.unwrap_or(format!("{opensearch_home}/config"))
524528
}
529+
530+
fn to_comma_separated_list(values: &[String]) -> Option<String> {
531+
if values.iter().any(|value| value.contains(",")) {
532+
None
533+
} else if values.is_empty() {
534+
Some("[]".to_owned())
535+
} else {
536+
Some(values.join(","))
537+
}
538+
}
525539
}
526540

527541
#[cfg(test)]
@@ -545,7 +559,7 @@ mod tests {
545559
use super::*;
546560
use crate::{
547561
controller::{ValidatedLogging, ValidatedOpenSearchConfig, ValidatedSecurity},
548-
crd::{NodeRoles, v1alpha1},
562+
crd::v1alpha1,
549563
framework::{
550564
product_logging::framework::ValidatedContainerLogConfigChoice,
551565
role_utils::GenericProductSpecificCommonConfig,
@@ -592,12 +606,13 @@ mod tests {
592606
),
593607
vector_container: None,
594608
},
595-
node_roles: NodeRoles(vec![
596-
v1alpha1::NodeRole::ClusterManager,
597-
v1alpha1::NodeRole::Data,
598-
v1alpha1::NodeRole::Ingest,
599-
v1alpha1::NodeRole::RemoteClusterClient,
600-
]),
609+
node_roles: [
610+
ValidatedNodeRole::ClusterManager,
611+
ValidatedNodeRole::Data,
612+
ValidatedNodeRole::Ingest,
613+
ValidatedNodeRole::RemoteClusterClient,
614+
]
615+
.into(),
601616
requested_secret_lifetime: Duration::from_str("1d")
602617
.expect("should be a valid duration"),
603618
resources: Resources::default(),
@@ -826,4 +841,30 @@ mod tests {
826841
node_config_multiple_nodes.initial_cluster_manager_nodes()
827842
);
828843
}
844+
845+
#[test]
846+
pub fn test_to_comma_separated_list() {
847+
assert_eq!(
848+
None,
849+
NodeConfig::to_comma_separated_list(&[
850+
"one".to_owned(),
851+
"two,three".to_owned(),
852+
"four".to_owned()
853+
])
854+
);
855+
856+
assert_eq!(
857+
Some("[]".to_owned()),
858+
NodeConfig::to_comma_separated_list(&[])
859+
);
860+
861+
assert_eq!(
862+
Some("one,two,three".to_owned()),
863+
NodeConfig::to_comma_separated_list(&[
864+
"one".to_owned(),
865+
"two".to_owned(),
866+
"three".to_owned()
867+
])
868+
);
869+
}
829870
}

0 commit comments

Comments
 (0)