diff --git a/Cargo.lock b/Cargo.lock index b437b15..b1ffa19 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2619,6 +2619,7 @@ dependencies = [ "strum", "tokio", "tracing", + "uuid", ] [[package]] @@ -3256,6 +3257,16 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" +[[package]] +name = "uuid" +version = "1.18.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2f87b8aa10b915a06587d0dec516c282ff295b475d94abf425d62b57710070a2" +dependencies = [ + "js-sys", + "wasm-bindgen", +] + [[package]] name = "valuable" version = "0.1.1" diff --git a/Cargo.nix b/Cargo.nix index d618989..b4bf6d6 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -8554,6 +8554,10 @@ rec { name = "tracing"; packageId = "tracing"; } + { + name = "uuid"; + packageId = "uuid"; + } ]; buildDependencies = [ { @@ -10926,6 +10930,68 @@ rec { }; resolvedDefaultFeatures = [ "default" ]; }; + "uuid" = rec { + crateName = "uuid"; + version = "1.18.1"; + edition = "2018"; + sha256 = "18kh01qmfayn4psap52x8xdjkzw2q8bcbpnhhxjs05dr22mbi1rg"; + authors = [ + "Ashley Mannix" + "Dylan DPC" + "Hunar Roop Kahlon" + ]; + dependencies = [ + { + name = "js-sys"; + packageId = "js-sys"; + optional = true; + usesDefaultFeatures = false; + target = { target, features }: (("wasm32" == target."arch" or null) && (("unknown" == target."os" or null) || ("none" == target."os" or null)) && (builtins.elem "atomics" targetFeatures)); + } + { + name = "wasm-bindgen"; + packageId = "wasm-bindgen"; + optional = true; + usesDefaultFeatures = false; + target = { target, features }: (("wasm32" == target."arch" or null) && (("unknown" == target."os" or null) || ("none" == target."os" or null))); + features = [ "msrv" ]; + } + ]; + devDependencies = [ + { + name = "wasm-bindgen"; + packageId = "wasm-bindgen"; + target = { target, features }: (("wasm32" == target."arch" or null) && (("unknown" == target."os" or null) || ("none" == target."os" or null))); + } + ]; + features = { + "arbitrary" = [ "dep:arbitrary" ]; + "atomic" = [ "dep:atomic" ]; + "borsh" = [ "dep:borsh" "dep:borsh-derive" ]; + "bytemuck" = [ "dep:bytemuck" ]; + "default" = [ "std" ]; + "fast-rng" = [ "rng" "dep:rand" ]; + "js" = [ "dep:wasm-bindgen" "dep:js-sys" ]; + "macro-diagnostics" = [ "dep:uuid-macro-internal" ]; + "md5" = [ "dep:md-5" ]; + "rng" = [ "dep:getrandom" ]; + "rng-getrandom" = [ "rng" "dep:getrandom" "uuid-rng-internal-lib" "uuid-rng-internal-lib/getrandom" ]; + "rng-rand" = [ "rng" "dep:rand" "uuid-rng-internal-lib" "uuid-rng-internal-lib/rand" ]; + "serde" = [ "dep:serde" ]; + "sha1" = [ "dep:sha1_smol" ]; + "slog" = [ "dep:slog" ]; + "std" = [ "wasm-bindgen?/std" "js-sys?/std" ]; + "uuid-rng-internal-lib" = [ "dep:uuid-rng-internal-lib" ]; + "v1" = [ "atomic" ]; + "v3" = [ "md5" ]; + "v4" = [ "rng" ]; + "v5" = [ "sha1" ]; + "v6" = [ "atomic" ]; + "v7" = [ "rng" ]; + "zerocopy" = [ "dep:zerocopy" ]; + }; + resolvedDefaultFeatures = [ "default" "std" ]; + }; "valuable" = rec { crateName = "valuable"; version = "0.1.1"; diff --git a/Cargo.toml b/Cargo.toml index 36e6dd3..08c18cc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,7 @@ snafu = "0.8" strum = { version = "0.27", features = ["derive"] } tokio = { version = "1.45", features = ["full"] } tracing = "0.1" +uuid = "1.18" #[patch."https://github.com/stackabletech/operator-rs"] # stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "main" } diff --git a/deploy/helm/opensearch-operator/crds/crds.yaml b/deploy/helm/opensearch-operator/crds/crds.yaml index 3762d44..5be4fa4 100644 --- a/deploy/helm/opensearch-operator/crds/crds.yaml +++ b/deploy/helm/opensearch-operator/crds/crds.yaml @@ -290,6 +290,7 @@ spec: type: object roleGroups: additionalProperties: + description: Variant of [`stackable_operator::role_utils::GenericProductSpecificCommonConfig`] that implements [`Merge`] properties: cliOverrides: additionalProperties: diff --git a/rust/operator-binary/Cargo.toml b/rust/operator-binary/Cargo.toml index 06272e5..c5a9213 100644 --- a/rust/operator-binary/Cargo.toml +++ b/rust/operator-binary/Cargo.toml @@ -21,6 +21,7 @@ snafu.workspace = true strum.workspace = true tokio.workspace = true tracing.workspace = true +uuid.workspace = true [build-dependencies] built.workspace = true diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 0fcf4e0..2823c98 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -1,3 +1,8 @@ +//! Controller for [`v1alpha1::OpenSearchCluster`] +//! +//! The cluster specification is validated, Kubernetes resource specifications are created and +//! applied and the cluster status is updated. + use std::{collections::BTreeMap, marker::PhantomData, str::FromStr, sync::Arc}; use apply::Applier; @@ -28,8 +33,8 @@ use crate::{ v1alpha1::{self}, }, framework::{ - ClusterName, ControllerName, HasNamespace, HasObjectName, HasUid, IsLabelValue, - OperatorName, ProductName, ProductVersion, RoleGroupName, RoleName, + ClusterName, ControllerName, HasName, HasUid, NameIsValidLabelValue, NamespaceName, + OperatorName, ProductName, ProductVersion, RoleGroupName, RoleName, Uid, role_utils::{GenericProductSpecificCommonConfig, RoleGroupConfig}, }, }; @@ -39,12 +44,17 @@ mod build; mod update_status; mod validate; +/// Names in the controller context which are passed to the submodules of the controller +/// +/// The names are not directly defined in [`Context`] because not every submodule requires a +/// Kubernetes client and unit testing is easier without an unnecessary client. pub struct ContextNames { pub product_name: ProductName, pub operator_name: OperatorName, pub controller_name: ControllerName, } +/// The controller context pub struct Context { client: stackable_operator::client::Client, names: ContextNames, @@ -113,6 +123,7 @@ type OpenSearchRoleGroupConfig = type OpenSearchNodeResources = stackable_operator::commons::resources::Resources; +/// The validated [`v1alpha1::OpenSearchConfig`] #[derive(Clone, Debug, PartialEq)] pub struct ValidatedOpenSearchConfig { pub affinity: StackableAffinity, @@ -122,19 +133,23 @@ pub struct ValidatedOpenSearchConfig { pub listener_class: String, } -// validated and converted to validated and safe types -// no user errors -// not restricted by CRD compliance +/// The validated [`v1alpha1::OpenSearchCluster`] +/// +/// Validated means that there should be no reason for Kubernetes to reject resources generated +/// from these values. This is usually achieved by using fail-safe types. For instance, the cluster +/// name is wrapped in the type [`ClusterName`]. This type implements e.g. the function +/// [`ClusterName::to_label_value`] which returns a valid label value as string. If this function +/// is used as intended, i.e. to set a label value, and if it is used as late as possible in the +/// call chain, then chances are high that the resulting Kubernetes resource is valid. #[derive(Clone, Debug, PartialEq)] pub struct ValidatedCluster { metadata: ObjectMeta, pub image: ProductImage, pub product_version: ProductVersion, pub name: ClusterName, - pub namespace: String, - pub uid: String, + pub namespace: NamespaceName, + pub uid: Uid, pub role_config: GenericRoleConfig, - // "validated" means that labels are valid and no ugly rolegroup name broke them pub role_group_configs: BTreeMap, } @@ -143,16 +158,17 @@ impl ValidatedCluster { image: ProductImage, product_version: ProductVersion, name: ClusterName, - namespace: String, - uid: String, + namespace: NamespaceName, + uid: impl Into, role_config: GenericRoleConfig, role_group_configs: BTreeMap, ) -> Self { + let uid = uid.into(); ValidatedCluster { metadata: ObjectMeta { - name: Some(name.to_object_name()), - namespace: Some(namespace.clone()), - uid: Some(uid.clone()), + name: Some(name.to_string()), + namespace: Some(namespace.to_string()), + uid: Some(uid.to_string()), ..ObjectMeta::default() }, image, @@ -165,14 +181,17 @@ impl ValidatedCluster { } } + /// Returns the one role name pub fn role_name() -> RoleName { RoleName::from_str("nodes").expect("should be a valid role name") } + /// Returns true if only a single OpenSearch node is defined in the cluster pub fn is_single_node(&self) -> bool { self.node_count() == 1 } + /// Returns the sum of the replicas in all role-groups pub fn node_count(&self) -> u32 { self.role_group_configs .values() @@ -180,6 +199,7 @@ impl ValidatedCluster { .sum() } + /// Returns all role-group configurations which contain the given node role pub fn role_group_configs_filtered_by_node_role( &self, node_role: &v1alpha1::NodeRole, @@ -192,27 +212,20 @@ impl ValidatedCluster { } } -impl HasObjectName for ValidatedCluster { - fn to_object_name(&self) -> String { - self.name.to_object_name() - } -} - -impl HasNamespace for ValidatedCluster { - fn to_namespace(&self) -> String { - self.namespace.clone() +impl HasName for ValidatedCluster { + fn to_name(&self) -> String { + self.name.to_string() } } impl HasUid for ValidatedCluster { - fn to_uid(&self) -> String { + fn to_uid(&self) -> Uid { self.uid.clone() } } -impl IsLabelValue for ValidatedCluster { +impl NameIsValidLabelValue for ValidatedCluster { fn to_label_value(&self) -> String { - // opinionated! self.name.to_label_value() } } @@ -259,6 +272,13 @@ pub fn error_policy( } } +/// Reconcile function of the OpenSearchCluster controller +/// +/// The reconcile function performs the following steps: +/// 1. Validate the given cluster specification and return a [`ValidatedCluster`] if successful. +/// 2. Build Kubernetes resource specifications from the validated cluster. +/// 3. Apply the Kubernetes resource specifications +/// 4. Update the cluster status pub async fn reconcile( object: Arc>, context: Arc, @@ -271,7 +291,7 @@ pub async fn reconcile( .map_err(stackable_operator::kube::core::error_boundary::InvalidObject::clone) .context(DeserializeClusterDefinitionSnafu)?; - // dereference (client required) + // not necessary in this controller: dereference (client required) // validate (no client required) let validated_cluster = validate(&context.names, cluster).context(ValidateClusterSnafu)?; @@ -284,14 +304,16 @@ pub async fn reconcile( let applied_resources = Applier::new( &context.client, &context.names, - &validated_cluster, + &validated_cluster.name, + &validated_cluster.namespace, + &validated_cluster.uid, apply_strategy, ) .apply(prepared_resources) .await .context(ApplyResourcesSnafu)?; - // create discovery ConfigMap based on the applied resources (client required) + // not necessary in this controller: create discovery ConfigMap based on the applied resources (client required) // update status (client required) update_status(&context.client, &context.names, cluster, applied_resources) @@ -301,10 +323,16 @@ pub async fn reconcile( Ok(Action::await_change()) } -// Marker +/// Marker for prepared Kubernetes resources which are not applied yet struct Prepared; +/// Marker for applied Kubernetes resources struct Applied; +/// List of all Kubernetes resources produced by this controller +/// +/// `T` is a marker that indicates if these resources are only [`Prepared`] or already [`Applied`]. +/// The marker is useful e.g. to ensure that the cluster status is updated based on the applied +/// resources. struct KubernetesResources { stateful_sets: Vec, services: Vec, @@ -324,13 +352,14 @@ mod tests { commons::affinity::StackableAffinity, k8s_openapi::api::core::v1::PodTemplateSpec, role_utils::GenericRoleConfig, }; + use uuid::uuid; use super::{Context, OpenSearchRoleGroupConfig, ValidatedCluster}; use crate::{ controller::{OpenSearchNodeResources, ValidatedOpenSearchConfig}, crd::{NodeRoles, v1alpha1}, framework::{ - ClusterName, OperatorName, ProductVersion, RoleGroupName, + ClusterName, NamespaceName, OperatorName, ProductVersion, RoleGroupName, builder::pod::container::EnvVarSet, role_utils::GenericProductSpecificCommonConfig, }, }; @@ -400,8 +429,8 @@ mod tests { .expect("should be a valid ProductImage structure"), ProductVersion::from_str_unsafe("3.1.0"), ClusterName::from_str_unsafe("my-opensearch"), - "default".to_owned(), - "e6ac237d-a6d4-43a1-8135-f36506110912".to_owned(), + NamespaceName::from_str_unsafe("default"), + uuid!("e6ac237d-a6d4-43a1-8135-f36506110912"), GenericRoleConfig::default(), [ ( diff --git a/rust/operator-binary/src/controller/apply.rs b/rust/operator-binary/src/controller/apply.rs index f0bd893..49afd02 100644 --- a/rust/operator-binary/src/controller/apply.rs +++ b/rust/operator-binary/src/controller/apply.rs @@ -1,3 +1,5 @@ +//! The apply step in the OpenSearchCluster controller + use std::marker::PhantomData; use snafu::{ResultExt, Snafu}; @@ -8,9 +10,7 @@ use stackable_operator::{ use strum::{EnumDiscriminants, IntoStaticStr}; use super::{Applied, ContextNames, KubernetesResources, Prepared}; -use crate::framework::{ - HasNamespace, HasObjectName, HasUid, cluster_resources::cluster_resources_new, -}; +use crate::framework::{ClusterName, NamespaceName, Uid, cluster_resources::cluster_resources_new}; #[derive(Snafu, Debug, EnumDiscriminants)] #[strum_discriminants(derive(IntoStaticStr))] @@ -28,6 +28,10 @@ pub enum Error { type Result = std::result::Result; +/// Applier for the Kubernetes resource specifications produced by this controller +/// +/// The implementation is not tied to this controller and could theoretically be moved to +/// stackable_operator if [`KubernetesResources`] would contain all possible resource types. pub struct Applier<'a> { client: &'a Client, cluster_resources: ClusterResources, @@ -37,14 +41,18 @@ impl<'a> Applier<'a> { pub fn new( client: &'a Client, names: &ContextNames, - cluster: &(impl HasObjectName + HasNamespace + HasUid), + cluster_name: &ClusterName, + cluster_namespace: &NamespaceName, + cluster_uid: &Uid, apply_strategy: ClusterResourceApplyStrategy, ) -> Applier<'a> { let cluster_resources = cluster_resources_new( &names.product_name, &names.operator_name, &names.controller_name, - cluster, + cluster_name, + cluster_namespace, + cluster_uid, apply_strategy, ); @@ -54,6 +62,7 @@ impl<'a> Applier<'a> { } } + /// Applies the given Kubernetes resources and marks them as applied pub async fn apply( mut self, resources: KubernetesResources, diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index e105b1b..287d127 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -1,3 +1,5 @@ +//! The build step in the OpenSearchCluster controller + use std::marker::PhantomData; use role_builder::RoleBuilder; @@ -8,6 +10,13 @@ pub mod node_config; pub mod role_builder; pub mod role_group_builder; +/// Builds Kubernetes resource specifications from the given validated cluster +/// +/// This function cannot fail because all failing conditions were already checked in the validation +/// step. +/// A Kubernetes client is not required because references to other Kubernetes resources must +/// already be dereferenced in a prior step and the result would be validated and added to the +/// validated cluster. pub fn build(names: &ContextNames, cluster: ValidatedCluster) -> KubernetesResources { let mut config_maps = vec![]; let mut stateful_sets = vec![]; @@ -52,6 +61,7 @@ mod tests { commons::affinity::StackableAffinity, k8s_openapi::api::core::v1::PodTemplateSpec, kube::Resource, role_utils::GenericRoleConfig, }; + use uuid::uuid; use super::build; use crate::{ @@ -61,8 +71,9 @@ mod tests { }, crd::{NodeRoles, v1alpha1}, framework::{ - ClusterName, ControllerName, OperatorName, ProductName, ProductVersion, RoleGroupName, - builder::pod::container::EnvVarSet, role_utils::GenericProductSpecificCommonConfig, + ClusterName, ControllerName, NamespaceName, OperatorName, ProductName, ProductVersion, + RoleGroupName, builder::pod::container::EnvVarSet, + role_utils::GenericProductSpecificCommonConfig, }, }; @@ -141,8 +152,8 @@ mod tests { .expect("should be a valid ProductImage structure"), ProductVersion::from_str_unsafe("3.1.0"), ClusterName::from_str_unsafe("my-opensearch"), - "default".to_owned(), - "e6ac237d-a6d4-43a1-8135-f36506110912".to_owned(), + NamespaceName::from_str_unsafe("default"), + uuid!("e6ac237d-a6d4-43a1-8135-f36506110912"), GenericRoleConfig::default(), [ ( diff --git a/rust/operator-binary/src/controller/build/node_config.rs b/rust/operator-binary/src/controller/build/node_config.rs index 0eb318c..379104e 100644 --- a/rust/operator-binary/src/controller/build/node_config.rs +++ b/rust/operator-binary/src/controller/build/node_config.rs @@ -1,3 +1,5 @@ +//! Configuration of an OpenSearch node + use std::str::FromStr; use serde_json::{Value, json}; @@ -8,42 +10,59 @@ use crate::{ controller::OpenSearchRoleGroupConfig, crd::v1alpha1, framework::{ + ServiceName, builder::pod::container::{EnvVarName, EnvVarSet}, role_group_utils, }, }; +/// The main configuration file of OpenSearch pub const CONFIGURATION_FILE_OPENSEARCH_YML: &str = "opensearch.yml"; -/// type: string +/// The cluster name. +/// Type: string pub const CONFIG_OPTION_CLUSTER_NAME: &str = "cluster.name"; -/// type: (comma-separated) list of strings +/// The list of hosts that perform discovery when a node is started. +/// Type: (comma-separated) list of strings pub const CONFIG_OPTION_DISCOVERY_SEED_HOSTS: &str = "discovery.seed_hosts"; -/// type: string +/// By default, OpenSearch forms a multi-node cluster. Set `discovery.type` to `single-node` to +/// form a single-node cluster. +/// Type: string pub const CONFIG_OPTION_DISCOVERY_TYPE: &str = "discovery.type"; -/// type: (comma-separated) list of strings +/// A list of cluster-manager-eligible nodes used to bootstrap the cluster. +/// Type: (comma-separated) list of strings pub const CONFIG_OPTION_INITIAL_CLUSTER_MANAGER_NODES: &str = "cluster.initial_cluster_manager_nodes"; -/// type: string +/// Binds an OpenSearch node to an address. +/// Type: string pub const CONFIG_OPTION_NETWORK_HOST: &str = "network.host"; -/// type: string +/// A descriptive name for the node. +/// Type: string pub const CONFIG_OPTION_NODE_NAME: &str = "node.name"; -/// type: (comma-separated) list of strings +/// Defines one or more roles for an OpenSearch node. +/// Type: (comma-separated) list of strings pub const CONFIG_OPTION_NODE_ROLES: &str = "node.roles"; -/// type: (comma-separated) list of strings +/// Specifies a list of distinguished names (DNs) that denote the other nodes in the cluster. +/// Type: (comma-separated) list of strings pub const CONFIG_OPTION_PLUGINS_SECURITY_NODES_DN: &str = "plugins.security.nodes_dn"; +/// Whether to enable TLS on the REST layer. If enabled, only HTTPS is allowed. +/// Type: boolean +pub const CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_ENABLED: &str = + "plugins.security.ssl.http.enabled"; + +/// Configuration of an OpenSearch node based on the cluster and role-group configuration pub struct NodeConfig { cluster: ValidatedCluster, role_group_config: OpenSearchRoleGroupConfig, - discovery_service_name: String, + discovery_service_name: ServiceName, } // Most functions are public because their configuration values could also be used in environment @@ -52,7 +71,7 @@ impl NodeConfig { pub fn new( cluster: ValidatedCluster, role_group_config: OpenSearchRoleGroupConfig, - discovery_service_name: String, + discovery_service_name: ServiceName, ) -> Self { Self { cluster, @@ -61,12 +80,15 @@ impl NodeConfig { } } - /// static for the cluster + /// Creates the main OpenSearch configuration file in YAML format pub fn static_opensearch_config_file(&self) -> String { Self::to_yaml(self.static_opensearch_config()) } - /// static for the cluster + /// Creates the main OpenSearch configuration file as JSON map + /// + /// The file should only contain cluster-wide configuration options. Node-specific options + /// should be defined as environment variables. pub fn static_opensearch_config(&self) -> serde_json::Map { let mut config = serde_json::Map::new(); @@ -106,13 +128,15 @@ impl NodeConfig { config } + /// Returns `true` if TLS is enabled on the HTTP port pub fn tls_on_http_port_enabled(&self) -> bool { self.static_opensearch_config() - .get("plugins.security.ssl.http.enabled") + .get(CONFIG_OPTION_PLUGINS_SECURITY_SSL_HTTP_ENABLED) .and_then(Self::value_as_bool) == Some(true) } + /// Converts the given JSON value to [`bool`] if possible pub fn value_as_bool(value: &Value) -> Option { value.as_bool().or( // OpenSearch parses the strings "true" and "false" as boolean, see @@ -123,11 +147,14 @@ impl NodeConfig { ) } - /// different for every node + /// Creates environment variables for the OpenSearch configurations + /// + /// The environment variables should only contain node-specific configuration options. + /// Cluster-wide options should be added to the configuration file. pub fn environment_variables(&self) -> EnvVarSet { EnvVarSet::new() // Set the OpenSearch node name to the Pod name. - // The node name is used e.g. for `{INITIAL_CLUSTER_MANAGER_NODES}`. + // The node name is used e.g. for INITIAL_CLUSTER_MANAGER_NODES. .with_field_path( EnvVarName::from_str_unsafe(CONFIG_OPTION_NODE_NAME), FieldPathEnvVar::Name, @@ -162,9 +189,9 @@ impl NodeConfig { .join("\n") } - /// Configuration for `{DISCOVERY_TYPE}` + /// Configuration for `discovery.type` /// - /// "zen" is the default if `{DISCOVERY_TYPE}` is not set. + /// "zen" is the default if `discovery.type` is not set. /// It is nevertheless explicitly set here. /// see /// @@ -209,7 +236,7 @@ impl NodeConfig { .cluster .role_group_configs_filtered_by_node_role(&v1alpha1::NodeRole::ClusterManager); - // This setting requires node names as set in `{NODE_NAME}`. + // This setting requires node names as set in NODE_NAME. // The node names are set to the pod names with // `valueFrom.fieldRef.fieldPath: metadata.name`, so it is okay to calculate the pod // names here and use them as node names. @@ -248,13 +275,14 @@ mod tests { k8s_openapi::api::core::v1::PodTemplateSpec, role_utils::GenericRoleConfig, }; + use uuid::uuid; use super::*; use crate::{ controller::ValidatedOpenSearchConfig, crd::NodeRoles, framework::{ - ClusterName, ProductVersion, RoleGroupName, + ClusterName, NamespaceName, ProductVersion, RoleGroupName, role_utils::GenericProductSpecificCommonConfig, }, }; @@ -303,8 +331,8 @@ mod tests { image.clone(), ProductVersion::from_str_unsafe(image.product_version()), ClusterName::from_str_unsafe("my-opensearch-cluster"), - "default".to_owned(), - "0b1e30e6-326e-4c1a-868d-ad6598b49e8b".to_owned(), + NamespaceName::from_str_unsafe("default"), + uuid!("0b1e30e6-326e-4c1a-868d-ad6598b49e8b"), GenericRoleConfig::default(), [( RoleGroupName::from_str_unsafe("default"), @@ -316,7 +344,7 @@ mod tests { NodeConfig::new( cluster, role_group_config, - "my-opensearch-cluster-manager".to_owned(), + ServiceName::from_str_unsafe("my-opensearch-cluster-manager"), ) } diff --git a/rust/operator-binary/src/controller/build/role_builder.rs b/rust/operator-binary/src/controller/build/role_builder.rs index 359597c..4423e71 100644 --- a/rust/operator-binary/src/controller/build/role_builder.rs +++ b/rust/operator-binary/src/controller/build/role_builder.rs @@ -1,3 +1,5 @@ +//! Builder for role resources + use stackable_operator::{ builder::meta::ObjectMetaBuilder, k8s_openapi::{ @@ -21,7 +23,7 @@ use super::role_group_builder::{ use crate::{ controller::{ContextNames, ValidatedCluster}, framework::{ - IsLabelValue, + NameIsValidLabelValue, builder::{ meta::ownerreference_from_resource, pdb::pod_disruption_budget_builder_with_role, }, @@ -31,6 +33,7 @@ use crate::{ const PDB_DEFAULT_MAX_UNAVAILABLE: u16 = 1; +/// Builder for role resources pub struct RoleBuilder<'a> { cluster: ValidatedCluster, context_names: &'a ContextNames, @@ -49,6 +52,7 @@ impl<'a> RoleBuilder<'a> { } } + /// Creates role-group builders which are initialized with the role-level context pub fn role_group_builders(&self) -> Vec> { self.cluster .role_group_configs @@ -66,6 +70,7 @@ impl<'a> RoleBuilder<'a> { .collect() } + /// Builds a ServiceAccount used by all role-groups pub fn build_service_account(&self) -> ServiceAccount { let metadata = self.common_metadata(self.resource_names.service_account_name()); @@ -75,6 +80,7 @@ impl<'a> RoleBuilder<'a> { } } + /// Builds a RoleBinding used by all role-groups pub fn build_role_binding(&self) -> RoleBinding { let metadata = self.common_metadata(self.resource_names.role_binding_name()); @@ -83,17 +89,25 @@ impl<'a> RoleBuilder<'a> { role_ref: RoleRef { api_group: ClusterRole::GROUP.to_owned(), kind: ClusterRole::KIND.to_owned(), - name: self.resource_names.cluster_role_name(), + name: self.resource_names.cluster_role_name().to_string(), }, subjects: Some(vec![Subject { api_group: Some(ServiceAccount::GROUP.to_owned()), kind: ServiceAccount::KIND.to_owned(), - name: self.resource_names.service_account_name(), - namespace: Some(self.cluster.namespace.clone()), + name: self.resource_names.service_account_name().to_string(), + namespace: Some(self.cluster.namespace.to_string()), }]), } } + /// Builds a Service that references all nodes with the cluster_manager node role + /// + /// Initially, this service was meant to be used by + /// [`super::node_config::NodeConfig::initial_cluster_manager_nodes`], but the function uses now another approach. + /// Afterwards, it was meant to be used as an entry point to OpenSearch, but it could also make + /// sense to use coordinating only nodes as entry points and not cluster manager nodes. + /// Therefore, this service will bei either adapted or removed. There is already an according + /// task entry in . pub fn build_cluster_manager_service(&self) -> Service { let ports = vec![ ServicePort { @@ -130,6 +144,7 @@ impl<'a> RoleBuilder<'a> { } } + /// Builds a [`PodDisruptionBudget`] used by all role-groups pub fn build_pdb(&self) -> Option { let pdb_config = &self.cluster.role_config.pod_disruption_budget; @@ -153,6 +168,7 @@ impl<'a> RoleBuilder<'a> { } } + /// Common metadata for role resources fn common_metadata(&self, resource_name: impl Into) -> ObjectMeta { ObjectMetaBuilder::new() .name(resource_name) @@ -166,7 +182,7 @@ impl<'a> RoleBuilder<'a> { .build() } - /// Labels on role resources + /// Common labels for role resources fn labels(&self) -> Labels { // Well-known Kubernetes labels let mut labels = Labels::role_selector( @@ -177,11 +193,11 @@ impl<'a> RoleBuilder<'a> { .unwrap(); let managed_by = Label::managed_by( - &self.context_names.operator_name.to_string(), - &self.context_names.controller_name.to_string(), + self.context_names.operator_name.as_ref(), + self.context_names.controller_name.as_ref(), ) .unwrap(); - let version = Label::version(&self.cluster.product_version.to_string()).unwrap(); + let version = Label::version(self.cluster.product_version.as_ref()).unwrap(); labels.insert(managed_by); labels.insert(version); @@ -208,6 +224,7 @@ mod tests { k8s_openapi::api::core::v1::PodTemplateSpec, role_utils::GenericRoleConfig, }; + use uuid::uuid; use super::RoleBuilder; use crate::{ @@ -216,8 +233,9 @@ mod tests { }, crd::{NodeRoles, v1alpha1}, framework::{ - ClusterName, ControllerName, OperatorName, ProductName, ProductVersion, RoleGroupName, - builder::pod::container::EnvVarSet, role_utils::GenericProductSpecificCommonConfig, + ClusterName, ControllerName, NamespaceName, OperatorName, ProductName, ProductVersion, + RoleGroupName, builder::pod::container::EnvVarSet, + role_utils::GenericProductSpecificCommonConfig, }, }; @@ -258,8 +276,8 @@ mod tests { image.clone(), ProductVersion::from_str_unsafe(image.product_version()), ClusterName::from_str_unsafe("my-opensearch-cluster"), - "default".to_owned(), - "0b1e30e6-326e-4c1a-868d-ad6598b49e8b".to_owned(), + NamespaceName::from_str_unsafe("default"), + uuid!("0b1e30e6-326e-4c1a-868d-ad6598b49e8b"), GenericRoleConfig::default(), [( RoleGroupName::from_str_unsafe("default"), 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 51e70c1..63afa9e 100644 --- a/rust/operator-binary/src/controller/build/role_group_builder.rs +++ b/rust/operator-binary/src/controller/build/role_group_builder.rs @@ -1,3 +1,7 @@ +//! Builder for role-group resources + +use std::str::FromStr; + use stackable_operator::{ builder::{meta::ObjectMetaBuilder, pod::container::ContainerBuilder}, crd::listener::{self}, @@ -21,10 +25,15 @@ use crate::{ controller::{ContextNames, OpenSearchRoleGroupConfig, ValidatedCluster}, crd::v1alpha1, framework::{ - RoleGroupName, - builder::{meta::ownerreference_from_resource, pod::container::EnvVarName}, + PersistentVolumeClaimName, RoleGroupName, ServiceAccountName, ServiceName, VolumeName, + builder::{ + meta::ownerreference_from_resource, + pod::{ + container::EnvVarName, + volume::{ListenerReference, listener_operator_volume_source_builder_build_pvc}, + }, + }, kvp::label::{recommended_labels, role_group_selector, role_selector}, - listener::listener_pvc, role_group_utils::ResourceNames, }, }; @@ -42,8 +51,22 @@ const LISTENER_VOLUME_DIR: &str = "/stackable/listener"; const DEFAULT_OPENSEARCH_HOME: &str = "/stackable/opensearch"; +fn config_volume_name() -> VolumeName { + VolumeName::from_str(CONFIG_VOLUME_NAME).expect("should be a valid Volume name") +} + +fn data_volume_name() -> VolumeName { + VolumeName::from_str(DATA_VOLUME_NAME).expect("should be a valid Volume name") +} + +fn listener_volume_name() -> PersistentVolumeClaimName { + PersistentVolumeClaimName::from_str(LISTENER_VOLUME_NAME) + .expect("should be a valid PersistentVolumeClaim name") +} + +/// Builder for role-group resources pub struct RoleGroupBuilder<'a> { - service_account_name: String, + service_account_name: ServiceAccountName, cluster: ValidatedCluster, node_config: NodeConfig, role_group_name: RoleGroupName, @@ -54,12 +77,12 @@ pub struct RoleGroupBuilder<'a> { impl<'a> RoleGroupBuilder<'a> { pub fn new( - service_account_name: String, + service_account_name: ServiceAccountName, cluster: ValidatedCluster, role_group_name: RoleGroupName, role_group_config: OpenSearchRoleGroupConfig, context_names: &'a ContextNames, - discovery_service_name: String, + discovery_service_name: ServiceName, ) -> RoleGroupBuilder<'a> { RoleGroupBuilder { service_account_name, @@ -80,6 +103,8 @@ impl<'a> RoleGroupBuilder<'a> { } } + /// Builds the [`ConfigMap`] containing the configuration files of the role-group + /// [`StatefulSet`] pub fn build_config_map(&self) -> ConfigMap { let metadata = self .common_metadata(self.resource_names.role_group_config_map()) @@ -98,6 +123,7 @@ impl<'a> RoleGroupBuilder<'a> { } } + /// Builds the role-group [`StatefulSet`] pub fn build_stateful_set(&self) -> StatefulSet { let metadata = self .common_metadata(self.resource_names.stateful_set_name()) @@ -111,19 +137,19 @@ impl<'a> RoleGroupBuilder<'a> { .resources .storage .data - .build_pvc(DATA_VOLUME_NAME, Some(vec!["ReadWriteOnce"])); + .build_pvc(data_volume_name().as_ref(), Some(vec!["ReadWriteOnce"])); - let listener_group_name = self.resource_names.listener_service_name(); + let listener_group_name = self.resource_names.listener_name(); // Listener endpoints for the all rolegroups will use persistent // volumes so that load balancers can hard-code the target // addresses. This will be the case even when no class is set (and // the value defaults to cluster-internal) as the address should // still be consistent. - let listener_volume_claim_template = listener_pvc( - listener_group_name, + let listener_volume_claim_template = listener_operator_volume_source_builder_build_pvc( + &ListenerReference::Listener(listener_group_name), &self.recommended_labels(), - LISTENER_VOLUME_NAME.to_string(), + &listener_volume_name(), ); let pvcs: Option> = Some(vec![ @@ -139,7 +165,7 @@ impl<'a> RoleGroupBuilder<'a> { match_labels: Some(self.pod_selector().into()), ..LabelSelector::default() }, - service_name: Some(self.resource_names.headless_service_name()), + service_name: Some(self.resource_names.headless_service_name().to_string()), template, volume_claim_templates: pvcs, ..StatefulSetSpec::default() @@ -152,6 +178,7 @@ impl<'a> RoleGroupBuilder<'a> { } } + /// Builds the [`PodTemplateSpec`] for the role-group [`StatefulSet`] fn build_pod_template(&self) -> PodTemplateSpec { let mut node_role_labels = Labels::new(); for node_role in self.role_group_config.config.node_roles.iter() { @@ -194,16 +221,16 @@ impl<'a> RoleGroupBuilder<'a> { fs_group: Some(1000), ..PodSecurityContext::default() }), - service_account_name: Some(self.service_account_name.clone()), + service_account_name: Some(self.service_account_name.to_string()), termination_grace_period_seconds: Some( self.role_group_config .config .termination_grace_period_seconds, ), volumes: Some(vec![Volume { - name: CONFIG_VOLUME_NAME.to_owned(), + name: config_volume_name().to_string(), config_map: Some(ConfigMapVolumeSource { - name: self.resource_names.role_group_config_map(), + name: self.resource_names.role_group_config_map().to_string(), ..Default::default() }), ..Volume::default() @@ -217,6 +244,10 @@ impl<'a> RoleGroupBuilder<'a> { pod_template } + /// Returns the labels of OpenSearch nodes with the `cluster_manager` role. + /// + /// As described in [`super::role_builder::RoleBuilder::build_cluster_manager_service`], this + /// function will be changed or deleted. pub fn cluster_manager_labels( cluster: &ValidatedCluster, context_names: &ContextNames, @@ -234,6 +265,7 @@ impl<'a> RoleGroupBuilder<'a> { labels } + /// Builds a label indicating the role of the OpenSearch node fn build_node_role_label(node_role: &v1alpha1::NodeRole) -> Label { // 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`. @@ -244,6 +276,7 @@ impl<'a> RoleGroupBuilder<'a> { .expect("should be a valid label") } + /// Builds the container for the [`PodTemplateSpec`] fn build_container(&self, role_group_config: &OpenSearchRoleGroupConfig) -> Container { let product_image = self .cluster @@ -280,7 +313,7 @@ impl<'a> RoleGroupBuilder<'a> { .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`, + // 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(EnvVarName::from_str_unsafe("OPENSEARCH_PATH_CONF")) @@ -300,14 +333,14 @@ impl<'a> RoleGroupBuilder<'a> { mount_path: format!( "{opensearch_path_conf}/{CONFIGURATION_FILE_OPENSEARCH_YML}" ), - name: CONFIG_VOLUME_NAME.to_owned(), + name: config_volume_name().to_string(), read_only: Some(true), sub_path: Some(CONFIGURATION_FILE_OPENSEARCH_YML.to_owned()), ..VolumeMount::default() }, VolumeMount { mount_path: format!("{opensearch_home}/data"), - name: DATA_VOLUME_NAME.to_owned(), + name: data_volume_name().to_string(), ..VolumeMount::default() }, VolumeMount { @@ -335,7 +368,16 @@ impl<'a> RoleGroupBuilder<'a> { .build() } + /// Builds the headless [`Service`] for the role-group pub fn build_headless_service(&self) -> Service { + let metadata = self + .common_metadata(self.resource_names.headless_service_name()) + .with_labels(Self::prometheus_labels()) + .with_annotations(Self::prometheus_annotations( + self.node_config.tls_on_http_port_enabled(), + )) + .build(); + let ports = vec![ ServicePort { name: Some(HTTP_PORT_NAME.to_owned()), @@ -349,12 +391,21 @@ impl<'a> RoleGroupBuilder<'a> { }, ]; - self.build_role_group_service( - self.resource_names.headless_service_name(), - ports, - Self::prometheus_labels(), - Self::prometheus_annotations(self.node_config.tls_on_http_port_enabled()), - ) + let service_spec = ServiceSpec { + // Internal communication does not need to be exposed + type_: Some("ClusterIP".to_string()), + cluster_ip: Some("None".to_string()), + ports: Some(ports), + selector: Some(self.pod_selector().into()), + publish_not_ready_addresses: Some(true), + ..ServiceSpec::default() + }; + + Service { + metadata, + spec: Some(service_spec), + status: None, + } } /// Common labels for Prometheus @@ -387,62 +438,35 @@ impl<'a> RoleGroupBuilder<'a> { .expect("should be valid annotations") } - fn build_role_group_service( - &self, - service_name: impl Into, - ports: Vec, - extra_labels: Labels, - extra_annotations: Annotations, - ) -> Service { - let metadata = self - .common_metadata(service_name) - .with_labels(extra_labels) - .with_annotations(extra_annotations) - .build(); - - let service_spec = ServiceSpec { - // Internal communication does not need to be exposed - type_: Some("ClusterIP".to_string()), - cluster_ip: Some("None".to_string()), - ports: Some(ports), - selector: Some(self.pod_selector().into()), - publish_not_ready_addresses: Some(true), - ..ServiceSpec::default() - }; - - Service { - metadata, - spec: Some(service_spec), - status: None, - } - } - + /// Builds the [`listener::v1alpha1::Listener`] for the role-group + /// + /// The Listener exposes only the HTTP port. + /// The Listener operator will create a Service per role-group. pub fn build_listener(&self) -> listener::v1alpha1::Listener { let metadata = self - .common_metadata(self.resource_names.listener_service_name()) + .common_metadata(self.resource_names.listener_name()) .build(); let listener_class = self.role_group_config.config.listener_class.to_owned(); + let ports = [listener::v1alpha1::ListenerPort { + name: HTTP_PORT_NAME.to_string(), + port: HTTP_PORT.into(), + protocol: Some("TCP".to_string()), + }]; + listener::v1alpha1::Listener { metadata, spec: listener::v1alpha1::ListenerSpec { class_name: Some(listener_class), - ports: Some(self.listener_ports()), + ports: Some(ports.to_vec()), ..listener::v1alpha1::ListenerSpec::default() }, status: None, } } - fn listener_ports(&self) -> Vec { - vec![listener::v1alpha1::ListenerPort { - name: HTTP_PORT_NAME.to_string(), - port: HTTP_PORT.into(), - protocol: Some("TCP".to_string()), - }] - } - + /// Common metadata for role-group resources fn common_metadata(&self, resource_name: impl Into) -> ObjectMetaBuilder { let mut builder = ObjectMetaBuilder::new(); @@ -459,6 +483,7 @@ impl<'a> RoleGroupBuilder<'a> { builder } + /// Recommended labels for role-group resources fn recommended_labels(&self) -> Labels { recommended_labels( &self.cluster, @@ -471,6 +496,9 @@ impl<'a> RoleGroupBuilder<'a> { ) } + /// Labels to select a [`Pod`] in the role-group + /// + /// [`Pod`]: stackable_operator::k8s_openapi::api::core::v1::Pod fn pod_selector(&self) -> Labels { role_group_selector( &self.cluster, @@ -495,19 +523,29 @@ mod tests { role_utils::GenericRoleConfig, }; use strum::IntoEnumIterator; + use uuid::uuid; - use super::RoleGroupBuilder; + use super::{RoleGroupBuilder, config_volume_name, data_volume_name, listener_volume_name}; use crate::{ controller::{ ContextNames, OpenSearchRoleGroupConfig, ValidatedCluster, ValidatedOpenSearchConfig, }, crd::{NodeRoles, v1alpha1}, framework::{ - ClusterName, ControllerName, OperatorName, ProductName, ProductVersion, RoleGroupName, - builder::pod::container::EnvVarSet, role_utils::GenericProductSpecificCommonConfig, + ClusterName, ControllerName, NamespaceName, OperatorName, ProductName, ProductVersion, + RoleGroupName, ServiceAccountName, ServiceName, builder::pod::container::EnvVarSet, + role_utils::GenericProductSpecificCommonConfig, }, }; + #[test] + fn test_volume_names() { + // Test that the functions do not panic + config_volume_name(); + data_volume_name(); + listener_volume_name(); + } + fn context_names() -> ContextNames { ContextNames { product_name: ProductName::from_str_unsafe("opensearch"), @@ -545,8 +583,8 @@ mod tests { image.clone(), ProductVersion::from_str_unsafe(image.product_version()), ClusterName::from_str_unsafe("my-opensearch-cluster"), - "default".to_owned(), - "0b1e30e6-326e-4c1a-868d-ad6598b49e8b".to_owned(), + NamespaceName::from_str_unsafe("default"), + uuid!("0b1e30e6-326e-4c1a-868d-ad6598b49e8b"), GenericRoleConfig::default(), [( RoleGroupName::from_str_unsafe("default"), @@ -568,12 +606,12 @@ mod tests { let role_group_config = role_group_config.to_owned(); RoleGroupBuilder::new( - "my-opensearch-cluster-serviceaccount".to_owned(), + ServiceAccountName::from_str_unsafe("my-opensearch-cluster-serviceaccount"), cluster, role_group_name, role_group_config, context_names, - "my-opensearch-cluster".to_owned(), + ServiceName::from_str_unsafe("my-opensearch-cluster"), ) } diff --git a/rust/operator-binary/src/controller/update_status.rs b/rust/operator-binary/src/controller/update_status.rs index 923f7db..b018533 100644 --- a/rust/operator-binary/src/controller/update_status.rs +++ b/rust/operator-binary/src/controller/update_status.rs @@ -1,3 +1,5 @@ +//! The update status step in the OpenSearchCluster controller + use snafu::{ResultExt, Snafu}; use stackable_operator::{ client::Client, @@ -32,6 +34,8 @@ pub enum Error { type Result = std::result::Result; +/// Updates the status of the [`v1alpha1::OpenSearchCluster`] according to the given applied +/// resources pub async fn update_status( client: &Client, names: &ContextNames, diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 98bf027..ac2015f 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -1,3 +1,5 @@ +//! The validate step in the OpenSearchCluster controller + use std::{collections::BTreeMap, num::TryFromIntError, str::FromStr}; use snafu::{OptionExt, ResultExt, Snafu}; @@ -15,7 +17,7 @@ use super::{ use crate::{ crd::v1alpha1::{self, OpenSearchConfig, OpenSearchConfigFragment}, framework::{ - ClusterName, + ClusterName, NamespaceName, Uid, builder::pod::container::{EnvVarName, EnvVarSet}, role_utils::{GenericProductSpecificCommonConfig, RoleGroupConfig, with_validated_config}, }, @@ -36,17 +38,23 @@ pub enum Error { #[snafu(display("failed to set cluster name"))] ParseClusterName { source: crate::framework::Error }, - #[snafu(display("failed to set product version"))] - ParseProductVersion { source: crate::framework::Error }, + #[snafu(display("failed to set cluster namespace"))] + ParseClusterNamespace { source: crate::framework::Error }, - #[snafu(display("failed to set role-group name"))] - ParseRoleGroupName { source: crate::framework::Error }, + #[snafu(display("failed to set UID"))] + ParseClusterUid { source: crate::framework::Error }, #[snafu(display("failed to parse environment variable"))] ParseEnvironmentVariable { source: crate::framework::builder::pod::container::Error, }, + #[snafu(display("failed to set product version"))] + ParseProductVersion { source: crate::framework::Error }, + + #[snafu(display("failed to set role-group name"))] + ParseRoleGroupName { source: crate::framework::Error }, + #[snafu(display("fragment validation failure"))] ValidateOpenSearchConfig { source: stackable_operator::config::fragment::ValidationError, @@ -61,7 +69,13 @@ pub enum Error { type Result = std::result::Result; -// no client needed +/// Validates the [`v1alpha1::OpenSearchCluster`] and returns a [`ValidatedCluster`] +/// +/// The validated values should be wrapped in fail-safe types so that illegal states are +/// unrepresentable in the following steps. +/// +/// A Kubernetes client is not required because references to other Kubernetes resources must +/// already be dereferenced in a prior step. pub fn validate( context_names: &ContextNames, cluster: &v1alpha1::OpenSearchCluster, @@ -69,9 +83,11 @@ pub fn validate( let raw_cluster_name = cluster.meta().name.clone().context(GetClusterNameSnafu)?; let cluster_name = ClusterName::from_str(&raw_cluster_name).context(ParseClusterNameSnafu)?; - let namespace = cluster.namespace().context(GetClusterNamespaceSnafu)?; + let raw_namespace = cluster.namespace().context(GetClusterNamespaceSnafu)?; + let namespace = NamespaceName::from_str(&raw_namespace).context(ParseClusterNamespaceSnafu)?; - let uid = cluster.uid().context(GetClusterUidSnafu)?; + let raw_uid = cluster.uid().context(GetClusterUidSnafu)?; + let uid = Uid::from_str(&raw_uid).context(ParseClusterUidSnafu)?; let product_version = ProductVersion::from_str(cluster.spec.image.product_version()) .context(ParseProductVersionSnafu)?; @@ -169,6 +185,7 @@ mod tests { role_utils::{CommonConfiguration, GenericRoleConfig, Role, RoleGroup}, time::Duration, }; + use uuid::uuid; use super::{ErrorDiscriminants, validate}; use crate::{ @@ -178,7 +195,8 @@ mod tests { v1alpha1::{self, OpenSearchClusterSpec, OpenSearchConfigFragment, StorageConfig}, }, framework::{ - ClusterName, ControllerName, OperatorName, ProductName, ProductVersion, RoleGroupName, + ClusterName, ControllerName, NamespaceName, OperatorName, ProductName, ProductVersion, + RoleGroupName, builder::pod::container::{EnvVarName, EnvVarSet}, role_utils::{GenericProductSpecificCommonConfig, RoleGroupConfig}, }, @@ -194,8 +212,8 @@ mod tests { .expect("should be a valid ProductImage structure"), ProductVersion::from_str_unsafe("3.1.0"), ClusterName::from_str_unsafe("my-opensearch"), - "default".to_owned(), - "e6ac237d-a6d4-43a1-8135-f36506110912".to_owned(), + NamespaceName::from_str_unsafe("default"), + uuid!("e6ac237d-a6d4-43a1-8135-f36506110912"), GenericRoleConfig::default(), [( RoleGroupName::from_str_unsafe("default"), @@ -364,6 +382,14 @@ mod tests { ); } + #[test] + fn test_validate_err_parse_cluster_namespace() { + test_validate_err( + |cluster| cluster.metadata.namespace = Some("invalid cluster namespace".to_owned()), + ErrorDiscriminants::ParseClusterNamespace, + ); + } + #[test] fn test_validate_err_get_cluster_uid() { test_validate_err( @@ -372,6 +398,14 @@ mod tests { ); } + #[test] + fn test_validate_err_parse_cluster_uid() { + test_validate_err( + |cluster| cluster.metadata.uid = Some("invalid cluster UID".to_owned()), + ErrorDiscriminants::ParseClusterUid, + ); + } + #[test] fn test_validate_err_parse_product_version() { test_validate_err( diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 307687b..1daa86e 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -26,7 +26,7 @@ use stackable_operator::{ use strum::{Display, EnumIter}; use crate::framework::{ - ClusterName, IsLabelValue, ProductName, RoleName, + ClusterName, NameIsValidLabelValue, ProductName, RoleName, role_utils::GenericProductSpecificCommonConfig, }; diff --git a/rust/operator-binary/src/framework.rs b/rust/operator-binary/src/framework.rs index c4ed7ba..7ed6206 100644 --- a/rust/operator-binary/src/framework.rs +++ b/rust/operator-binary/src/framework.rs @@ -1,82 +1,153 @@ -// Type-safe wrappers that cannot throw errors -// The point is, to move the validation "upwards". -// The contents of this module will be moved to operator-rs when stabilized. +//! Additions to stackable-operator +//! +//! Functions in stackable-operator usually accept generic types like strings and validate the +//! parameters as late as possible. Therefore, nearly all functions have to return a [`Result`] and +//! errors are returned along the call chain. That makes error handling complex because every +//! module re-packages the received error. Also, the validation is repeated if the value is used in +//! different function calls. Sometimes, validation is not necessary if constant values are used, +//! e.g. the name of the operator. +//! +//! The OpenSearch operator uses a different approach. The incoming values are validated as early +//! as possible and wrapped in a fail-safe type. This type is then used along the call chain, +//! validation is not necessary anymore and functions without side effects do not need to return a +//! [`Result`]. +//! +//! However, the OpenSearch operator uses stackable-operator and at the interface, the fail-safe +//! types must be unwrapped and the [`Result`] returned by the stackable-operator function must be +//! handled. This is done by calling [`Result::expect`] which requires thorough testing. +//! +//! When the development of the OpenSearch operator has progressed and changes in this module +//! become less frequent, then this module can be incorporated into stackable-operator. The module +//! structure should already resemble the one of stackable-operator. -use std::{fmt::Display, str::FromStr}; - -use kvp::label::MAX_LABEL_VALUE_LENGTH; -use snafu::{ResultExt, Snafu, ensure}; -use stackable_operator::kvp::LabelValue; +use snafu::Snafu; use strum::{EnumDiscriminants, IntoStaticStr}; pub mod builder; pub mod cluster_resources; pub mod kvp; -pub mod listener; pub mod role_group_utils; pub mod role_utils; #[derive(Snafu, Debug, EnumDiscriminants)] #[strum_discriminants(derive(IntoStaticStr))] pub enum Error { + #[snafu(display("empty strings are not allowed"))] + EmptyString {}, + #[snafu(display("maximum length exceeded"))] LengthExceeded { length: usize, max_length: usize }, - #[snafu(display("object name not RFC 1123 compliant"))] - InvalidObjectName { + #[snafu(display("not a valid label value"))] + InvalidLabelValue { + source: stackable_operator::kvp::LabelValueError, + }, + + #[snafu(display("not a valid label name as defined in RFC 1035"))] + InvalidRfc1035LabelName { source: stackable_operator::validation::Errors, }, - #[snafu(display("failed to use as label"))] - InvalidLabelValue { - source: stackable_operator::kvp::LabelValueError, + #[snafu(display("not a valid DNS subdomain name as defined in RFC 1123"))] + InvalidRfc1123DnsSubdomainName { + source: stackable_operator::validation::Errors, }, -} -/// 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; + #[snafu(display("not a valid label name as defined in RFC 1123"))] + InvalidRfc1123LabelName { + source: stackable_operator::validation::Errors, + }, -/// Has a name that can be used as a DNS subdomain name as defined in RFC 1123. -/// Most resource types, e.g. a Pod, require such a compliant name. -pub trait HasObjectName { - fn to_object_name(&self) -> String; + #[snafu(display("not a valid UUID"))] + InvalidUid { source: uuid::Error }, } -/// Has a namespace -pub trait HasNamespace { - fn to_namespace(&self) -> String; +/// Maximum length of DNS subdomain names as defined in RFC 1123. +/// +/// Duplicates the private constant +/// [`stackable-operator::validation::RFC_1123_SUBDOMAIN_MAX_LENGTH`] +pub const MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH: usize = 253; + +/// Maximum length of label names as defined in RFC 1123. +/// +/// Duplicates the private constant [`stackable-operator::validation::RFC_1123_LABEL_MAX_LENGTH`] +pub const MAX_RFC_1123_LABEL_NAME_LENGTH: usize = 63; + +/// Maximum length of label names as defined in RFC 1035. +/// +/// Duplicates the private constant [`stackable-operator::validation::RFC_1035_LABEL_MAX_LENGTH`] +pub const MAX_RFC_1035_LABEL_NAME_LENGTH: usize = 63; + +/// Maximum length of label values +/// +/// Duplicates the private constant [`stackable-operator::kvp::label::value::LABEL_VALUE_MAX_LEN`] +pub const MAX_LABEL_VALUE_LENGTH: usize = 63; + +/// Has a non-empty name +/// +/// Useful as an object reference; Should not be used to create an object because the name could +/// violate the naming constraints (e.g. maximum length) of the object. +pub trait HasName { + #[allow(dead_code)] + fn to_name(&self) -> String; } /// Has a Kubernetes UID pub trait HasUid { - fn to_uid(&self) -> String; + fn to_uid(&self) -> Uid; } -/// Is a valid label value as defined in RFC 1123. -pub trait IsLabelValue { +/// The name is a valid label value +pub trait NameIsValidLabelValue { fn to_label_value(&self) -> String; } /// Restricted string type with attributes like maximum length. +/// +/// Fully-qualified types are used to ease the import into other modules. +#[macro_export(local_inner_macros)] macro_rules! attributed_string_type { ($name:ident, $description:literal, $example:literal $(, $attribute:tt)*) => { - #[doc = concat!($description, ", e.g. \"", $example, "\"")] + #[doc = std::concat!($description, ", e.g. \"", $example, "\"")] #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] pub struct $name(String); - impl Display for $name { + impl std::fmt::Display for $name { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { self.0.fmt(f) } } - impl FromStr for $name { - type Err = Error; + impl From<$name> for String { + fn from(value: $name) -> Self { + value.0 + } + } + + impl From<&$name> for String { + fn from(value: &$name) -> Self { + value.0.clone() + } + } + + impl AsRef for $name { + fn as_ref(&self) -> &str { + &self.0 + } + } + + impl std::str::FromStr for $name { + type Err = $crate::framework::Error; fn from_str(s: &str) -> std::result::Result { + // ResultExt::context is used on most but not all usages of this macro + #[allow(unused_imports)] + use snafu::ResultExt; + + snafu::ensure!( + !s.is_empty(), + $crate::framework::EmptyStringSnafu {} + ); $(attributed_string_type!(@from_str $name, s, $attribute);)* @@ -88,7 +159,7 @@ macro_rules! attributed_string_type { impl $name { #[allow(dead_code)] pub fn from_str_unsafe(s: &str) -> Self { - FromStr::from_str(s).expect("should be a valid {name}") + std::str::FromStr::from_str(s).expect("should be a valid {name}") } // A dead_code warning is emitted if there is no unit test that calls this function. @@ -101,19 +172,28 @@ macro_rules! attributed_string_type { }; (@from_str $name:ident, $s:expr, (max_length = $max_length:expr)) => { let length = $s.len() as usize; - ensure!( + snafu::ensure!( length <= $name::MAX_LENGTH, - LengthExceededSnafu { + $crate::framework::LengthExceededSnafu { length, max_length: $name::MAX_LENGTH, } ); }; - (@from_str $name:ident, $s:expr, is_object_name) => { - stackable_operator::validation::is_rfc_1123_subdomain($s).context(InvalidObjectNameSnafu)?; + (@from_str $name:ident, $s:expr, is_rfc_1123_dns_subdomain_name) => { + stackable_operator::validation::is_rfc_1123_subdomain($s).context($crate::framework::InvalidRfc1123DnsSubdomainNameSnafu)?; + }; + (@from_str $name:ident, $s:expr, is_rfc_1123_label_name) => { + stackable_operator::validation::is_rfc_1123_label($s).context($crate::framework::InvalidRfc1123LabelNameSnafu)?; + }; + (@from_str $name:ident, $s:expr, is_rfc_1035_label_name) => { + stackable_operator::validation::is_rfc_1035_label($s).context($crate::framework::InvalidRfc1035LabelNameSnafu)?; }; (@from_str $name:ident, $s:expr, is_valid_label_value) => { - LabelValue::from_str($s).context(InvalidLabelValueSnafu)?; + stackable_operator::kvp::LabelValue::from_str($s).context($crate::framework::InvalidLabelValueSnafu)?; + }; + (@from_str $name:ident, $s:expr, is_uid) => { + uuid::Uuid::try_parse($s).context($crate::framework::InvalidUidSnafu)?; }; (@trait_impl $name:ident, (max_length = $max_length:expr)) => { impl $name { @@ -121,15 +201,43 @@ macro_rules! attributed_string_type { pub const MAX_LENGTH: usize = $max_length; } }; - (@trait_impl $name:ident, is_object_name) => { - impl HasObjectName for $name { - fn to_object_name(&self) -> String { - self.0.clone() + (@trait_impl $name:ident, is_rfc_1035_label_name) => { + impl $name { + pub const IS_RFC_1035_LABEL_NAME: bool = true; + pub const IS_RFC_1123_LABEL_NAME: bool = true; + pub const IS_RFC_1123_SUBDOMAIN_NAME: bool = true; + } + }; + (@trait_impl $name:ident, is_rfc_1123_dns_subdomain_name) => { + impl $name { + pub const IS_RFC_1123_SUBDOMAIN_NAME: bool = true; + } + }; + (@trait_impl $name:ident, is_rfc_1123_label_name) => { + impl $name { + pub const IS_RFC_1123_LABEL_NAME: bool = true; + pub const IS_RFC_1123_SUBDOMAIN_NAME: bool = true; + } + }; + (@trait_impl $name:ident, is_uid) => { + impl From for $name { + fn from(value: uuid::Uuid) -> Self { + Self(value.to_string()) + } + } + + impl From<&uuid::Uuid> for $name { + fn from(value: &uuid::Uuid) -> Self { + Self(value.to_string()) } } }; (@trait_impl $name:ident, is_valid_label_value) => { - impl IsLabelValue for $name { + impl $name { + pub const IS_VALID_LABEL_VALUE: bool = true; + } + + impl $crate::framework::NameIsValidLabelValue for $name { fn to_label_value(&self) -> String { self.0.clone() } @@ -137,13 +245,136 @@ macro_rules! attributed_string_type { }; } +/// Returns the minimum of the given values. +/// +/// As opposed to [`std::cmp::min`], this function can be used at compile-time. +/// +/// # Examples +/// +/// ```rust +/// assert_eq!(2, min(2, 3)); +/// assert_eq!(4, min(5, 4)); +/// assert_eq!(1, min(1, 1)); +/// ``` +pub const fn min(x: usize, y: usize) -> usize { + if x < y { x } else { y } +} + +// Kubernetes (resource) names + +attributed_string_type! { + ConfigMapName, + "The name of a ConfigMap", + "opensearch-nodes-default", + (max_length = MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH), + is_rfc_1123_dns_subdomain_name +} +attributed_string_type! { + ClusterRoleName, + "The name of a ClusterRole", + "opensearch-clusterrole", + // On the one hand, ClusterRoles must only contain characters that are allowed for DNS + // subdomain names, on the other hand, their length does not seem to be restricted – at least + // on Kind. However, 253 characters are sufficient for the Stackable operators, and to avoid + // problems on other Kubernetes providers, the length is restricted here. + (max_length = MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH), + is_rfc_1123_dns_subdomain_name +} +attributed_string_type! { + ListenerName, + "The name of a Listener", + "opensearch-nodes-default", + (max_length = MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH), + is_rfc_1123_dns_subdomain_name +} +attributed_string_type! { + ListenerClassName, + "The name of a Listener", + "external-stable", + (max_length = MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH), + is_rfc_1123_dns_subdomain_name +} +attributed_string_type! { + NamespaceName, + "The name of a Namespace", + "stackable-operators", + (max_length = min(MAX_RFC_1123_LABEL_NAME_LENGTH, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1123_label_name, + is_valid_label_value +} +attributed_string_type! { + PersistentVolumeClaimName, + "The name of a PersistentVolumeClaim", + "config", + (max_length = MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH), + is_rfc_1123_dns_subdomain_name +} +attributed_string_type! { + RoleBindingName, + "The name of a RoleBinding", + "opensearch-rolebinding", + // On the one hand, RoleBindings must only contain characters that are allowed for DNS + // subdomain names, on the other hand, their length does not seem to be restricted – at least + // on Kind. However, 253 characters are sufficient for the Stackable operators, and to avoid + // problems on other Kubernetes providers, the length is restricted here. + (max_length = MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH), + is_rfc_1123_dns_subdomain_name +} +attributed_string_type! { + ServiceAccountName, + "The name of a ServiceAccount", + "opensearch-serviceaccount", + (max_length = MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH), + is_rfc_1123_dns_subdomain_name +} +attributed_string_type! { + ServiceName, + "The name of a Service", + "opensearch-nodes-default-headless", + (max_length = min(MAX_RFC_1035_LABEL_NAME_LENGTH, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1035_label_name, + is_valid_label_value +} +attributed_string_type! { + StatefulSetName, + "The name of a StatefulSet", + "opensearch-nodes-default", + (max_length = min( + // see https://github.com/kubernetes/kubernetes/issues/64023 + MAX_RFC_1123_LABEL_NAME_LENGTH + - 1 /* dash */ + - 10 /* digits for the controller-revision-hash label */, + MAX_LABEL_VALUE_LENGTH)), + is_rfc_1123_label_name, + is_valid_label_value +} +attributed_string_type! { + Uid, + "A UID", + "c27b3971-ca72-42c1-80a4-abdfc1db0ddd", + (max_length = min(uuid::fmt::Hyphenated::LENGTH, MAX_LABEL_VALUE_LENGTH)), + is_uid, + is_valid_label_value +} +attributed_string_type! { + VolumeName, + "The name of a Volume", + "opensearch-nodes-default", + (max_length = min(MAX_RFC_1123_LABEL_NAME_LENGTH, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1123_label_name, + is_valid_label_value +} + +// Operator names + attributed_string_type! { ProductName, "The name of a product", "opensearch", // A suffix is added to produce a label value. An according compile-time check ensures that // max_length cannot be set higher. - (max_length = 54), + (max_length = min(54, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1123_dns_subdomain_name, is_valid_label_value } attributed_string_type! { @@ -157,10 +388,10 @@ attributed_string_type! { ClusterName, "The name of a cluster/stacklet", "my-opensearch-cluster", - // Suffixes are added to produce a resource names. According compile-time check ensures that + // Suffixes are added to produce resource names. According compile-time checks ensure that // max_length cannot be set higher. - (max_length = 24), - is_object_name, + (max_length = min(24, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1035_label_name, is_valid_label_value } attributed_string_type! { @@ -181,16 +412,22 @@ attributed_string_type! { RoleGroupName, "The name of a role-group name", "cluster-manager", - (max_length = 16), - is_object_name, + // The role-group name is used to produce resource names. To make sure that all resource names + // are valid, max_length is restricted. Compile-time checks ensure that max_length cannot be + // set higher if not other names like the RoleName are set lower accordingly. + (max_length = min(16, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1123_label_name, is_valid_label_value } attributed_string_type! { RoleName, "The name of a role name", "nodes", - (max_length = 10), - is_object_name, + // The role name is used to produce resource names. To make sure that all resource names are + // valid, max_length is restricted. Compile-time checks ensure that max_length cannot be set + // higher if not other names like the RoleGroupName are set lower accordingly. + (max_length = min(10, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1123_label_name, is_valid_label_value } @@ -198,13 +435,28 @@ attributed_string_type! { mod tests { use std::str::FromStr; + use uuid::uuid; + use super::{ - ClusterName, ControllerName, OperatorName, ProductVersion, RoleGroupName, RoleName, + ClusterName, ClusterRoleName, ConfigMapName, ControllerName, ErrorDiscriminants, + NamespaceName, OperatorName, PersistentVolumeClaimName, ProductVersion, RoleBindingName, + RoleGroupName, RoleName, ServiceAccountName, ServiceName, StatefulSetName, Uid, VolumeName, }; - use crate::framework::{HasObjectName, IsLabelValue, ProductName}; + use crate::framework::{NameIsValidLabelValue, ProductName}; #[test] fn test_attributed_string_type_examples() { + ConfigMapName::test_example(); + ClusterRoleName::test_example(); + NamespaceName::test_example(); + PersistentVolumeClaimName::test_example(); + RoleBindingName::test_example(); + ServiceAccountName::test_example(); + ServiceName::test_example(); + StatefulSetName::test_example(); + Uid::test_example(); + VolumeName::test_example(); + ProductName::test_example(); ProductVersion::test_example(); ClusterName::test_example(); @@ -214,43 +466,167 @@ mod tests { RoleName::test_example(); } + attributed_string_type! { + DisplayFmtTest, + "Display::fmt test", + "test" + } + + #[test] + fn test_attributed_string_type_display_fmt() { + type T = DisplayFmtTest; + + assert_eq!("test", format!("{}", T::from_str_unsafe("test"))); + } + + attributed_string_type! { + StringFromTest, + "String::from test", + "test" + } + + #[test] + fn test_attributed_string_type_string_from() { + type T = StringFromTest; + + T::test_example(); + assert_eq!("test", String::from(T::from_str_unsafe("test"))); + assert_eq!("test", String::from(&T::from_str_unsafe("test"))); + } + + attributed_string_type! { + LengthTest, + "empty string and max_length test", + "test", + (max_length = 4) + } + + #[test] + fn test_attributed_string_type_length() { + type T = LengthTest; + + T::test_example(); + assert_eq!(4, T::MAX_LENGTH); + assert_eq!( + Err(ErrorDiscriminants::EmptyString), + T::from_str("").map_err(ErrorDiscriminants::from) + ); + assert_eq!( + Err(ErrorDiscriminants::LengthExceeded), + T::from_str("testX").map_err(ErrorDiscriminants::from) + ); + } + + attributed_string_type! { + IsRfc1035LabelNameTest, + "is_rfc_1035_label_name test", + "a-b", + is_rfc_1035_label_name + } + #[test] - fn test_attributed_string_type_fmt() { + fn test_attributed_string_type_is_rfc_1035_label_name() { + type T = IsRfc1035LabelNameTest; + + let _ = T::IS_RFC_1035_LABEL_NAME; + let _ = T::IS_RFC_1123_LABEL_NAME; + let _ = T::IS_RFC_1123_SUBDOMAIN_NAME; + + T::test_example(); assert_eq!( - "my-cluster-name".to_owned(), - format!("{}", ClusterName::from_str_unsafe("my-cluster-name")) + Err(ErrorDiscriminants::InvalidRfc1035LabelName), + T::from_str("A").map_err(ErrorDiscriminants::from) ); } + attributed_string_type! { + IsRfc1123DnsSubdomainNameTest, + "is_rfc_1123_dns_subdomain_name test", + "a-b.c", + is_rfc_1123_dns_subdomain_name + } + #[test] - fn test_attributed_string_type_max_length() { - assert_eq!(24, ClusterName::MAX_LENGTH); + fn test_attributed_string_type_is_rfc_1123_dns_subdomain_name() { + type T = IsRfc1123DnsSubdomainNameTest; - assert!(ClusterName::from_str(&"a".repeat(ClusterName::MAX_LENGTH)).is_ok()); - assert!(ClusterName::from_str(&"a".repeat(ClusterName::MAX_LENGTH + 1)).is_err()); + let _ = T::IS_RFC_1123_SUBDOMAIN_NAME; + + T::test_example(); + assert_eq!( + Err(ErrorDiscriminants::InvalidRfc1123DnsSubdomainName), + T::from_str("A").map_err(ErrorDiscriminants::from) + ); + } + + attributed_string_type! { + IsRfc1123LabelNameTest, + "is_rfc_1123_label_name test", + "1-a", + is_rfc_1123_label_name } #[test] - fn test_attributed_string_type_is_object_name() { + fn test_attributed_string_type_is_rfc_1123_label_name() { + type T = IsRfc1123LabelNameTest; + + let _ = T::IS_RFC_1123_LABEL_NAME; + let _ = T::IS_RFC_1123_SUBDOMAIN_NAME; + + T::test_example(); assert_eq!( - "valid-object.name.123", - ClusterName::from_str_unsafe("valid-object.name.123").to_object_name() + Err(ErrorDiscriminants::InvalidRfc1123LabelName), + T::from_str("A").map_err(ErrorDiscriminants::from) ); - // A valid object name contains only lowercase characters. - assert!(ClusterName::from_str("InvalidObjectName").is_err()); + } + + attributed_string_type! { + IsValidLabelValueTest, + "is_valid_label_value test", + "a-_.1", + is_valid_label_value } #[test] fn test_attributed_string_type_is_valid_label_value() { - // Use a struct implementing the trait `IsLabelValue` but not `HasObjectName` because - // object names are proper subsets of label values and the test should not already fail on - // the object check. + type T = IsValidLabelValueTest; + + let _ = T::IS_VALID_LABEL_VALUE; + + T::test_example(); + assert_eq!( + Err(ErrorDiscriminants::InvalidLabelValue), + T::from_str("invalid label value").map_err(ErrorDiscriminants::from) + ); + assert_eq!( + "label-value", + T::from_str_unsafe("label-value").to_label_value() + ); + } + attributed_string_type! { + IsUidTest, + "is_uid test", + "c27b3971-ca72-42c1-80a4-abdfc1db0ddd", + is_uid + } + + #[test] + fn test_attributed_string_type_is_uid() { + type T = IsUidTest; + + T::test_example(); + assert_eq!( + Err(ErrorDiscriminants::InvalidUid), + T::from_str("invalid UID").map_err(ErrorDiscriminants::from) + ); + assert_eq!( + "c27b3971-ca72-42c1-80a4-abdfc1db0ddd", + T::from(uuid!("c27b3971-ca72-42c1-80a4-abdfc1db0ddd")).to_string() + ); assert_eq!( - "valid-label_value.123", - ProductName::from_str_unsafe("valid-label_value.123").to_label_value() + "c27b3971-ca72-42c1-80a4-abdfc1db0ddd", + T::from(&uuid!("c27b3971-ca72-42c1-80a4-abdfc1db0ddd")).to_string() ); - // A valid label value must end with an alphanumeric character. - assert!(ProductName::from_str("invalid-label-value-").is_err()); } } diff --git a/rust/operator-binary/src/framework/builder/meta.rs b/rust/operator-binary/src/framework/builder/meta.rs index 6ed6b53..5034004 100644 --- a/rust/operator-binary/src/framework/builder/meta.rs +++ b/rust/operator-binary/src/framework/builder/meta.rs @@ -3,19 +3,22 @@ use stackable_operator::{ k8s_openapi::apimachinery::pkg::apis::meta::v1::OwnerReference, kube::Resource, }; -use crate::framework::{HasObjectName, HasUid}; +use crate::framework::{HasName, HasUid}; -/// Infallible variant of `stackable_operator::builder::meta::ObjectMetaBuilder::ownerreference_from_resource` +/// Infallible variant of +/// [`stackable_operator::builder::meta::ObjectMetaBuilder::ownerreference_from_resource`] pub fn ownerreference_from_resource( - resource: &(impl Resource + HasObjectName + HasUid), + resource: &(impl Resource + HasName + HasUid), block_owner_deletion: Option, controller: Option, ) -> OwnerReference { OwnerReferenceBuilder::new() // Set api_version, kind, name and additionally the UID if it exists. .initialize_from_resource(resource) + // Ensure that the name is set. + .name(resource.to_name()) // Ensure that the UID is set. - .uid(resource.to_uid()) + .uid(resource.to_uid().to_string()) .block_owner_deletion_opt(block_owner_deletion) .controller_opt(controller) .build() @@ -34,7 +37,7 @@ mod tests { kube::Resource, }; - use crate::framework::{HasObjectName, HasUid, builder::meta::ownerreference_from_resource}; + use crate::framework::{HasName, HasUid, Uid, builder::meta::ownerreference_from_resource}; struct Cluster { object_meta: ObjectMeta, @@ -81,8 +84,8 @@ mod tests { } } - impl HasObjectName for Cluster { - fn to_object_name(&self) -> String { + impl HasName for Cluster { + fn to_name(&self) -> String { self.object_meta .name .clone() @@ -91,11 +94,14 @@ mod tests { } impl HasUid for Cluster { - fn to_uid(&self) -> String { - self.object_meta - .uid - .clone() - .expect("should be set in Cluster::new") + fn to_uid(&self) -> Uid { + Uid::from_str_unsafe( + &self + .object_meta + .uid + .clone() + .expect("should be set in Cluster::new"), + ) } } diff --git a/rust/operator-binary/src/framework/builder/pdb.rs b/rust/operator-binary/src/framework/builder/pdb.rs index 3721040..b45383e 100644 --- a/rust/operator-binary/src/framework/builder/pdb.rs +++ b/rust/operator-binary/src/framework/builder/pdb.rs @@ -5,11 +5,13 @@ use stackable_operator::{ }; use crate::framework::{ - ControllerName, HasObjectName, HasUid, IsLabelValue, OperatorName, ProductName, RoleName, + ControllerName, HasName, HasUid, NameIsValidLabelValue, OperatorName, ProductName, RoleName, }; +/// Infallible variant of +/// [`stackable_operator::builder::pdb::PodDisruptionBudgetBuilder::new_with_role`] pub fn pod_disruption_budget_builder_with_role( - owner: &(impl Resource + HasObjectName + HasUid + IsLabelValue), + owner: &(impl Resource + HasName + NameIsValidLabelValue + HasUid), product_name: &ProductName, role_name: &RoleName, operator_name: &OperatorName, @@ -44,8 +46,8 @@ mod tests { }; use crate::framework::{ - ControllerName, HasObjectName, HasUid, IsLabelValue, OperatorName, ProductName, RoleName, - builder::pdb::pod_disruption_budget_builder_with_role, + ControllerName, HasName, HasUid, NameIsValidLabelValue, OperatorName, ProductName, + RoleName, Uid, builder::pdb::pod_disruption_budget_builder_with_role, }; struct Cluster { @@ -93,8 +95,8 @@ mod tests { } } - impl HasObjectName for Cluster { - fn to_object_name(&self) -> String { + impl HasName for Cluster { + fn to_name(&self) -> String { self.object_meta .name .clone() @@ -103,15 +105,18 @@ mod tests { } impl HasUid for Cluster { - fn to_uid(&self) -> String { - self.object_meta - .uid - .clone() - .expect("should be set in Cluster::new") + fn to_uid(&self) -> Uid { + Uid::from_str_unsafe( + &self + .object_meta + .uid + .clone() + .expect("should be set in Cluster::new"), + ) } } - impl IsLabelValue for Cluster { + impl NameIsValidLabelValue for Cluster { fn to_label_value(&self) -> String { self.object_meta .name diff --git a/rust/operator-binary/src/framework/builder/pod.rs b/rust/operator-binary/src/framework/builder/pod.rs index 18581c4..df93bd4 100644 --- a/rust/operator-binary/src/framework/builder/pod.rs +++ b/rust/operator-binary/src/framework/builder/pod.rs @@ -1 +1,2 @@ pub mod container; +pub mod volume; diff --git a/rust/operator-binary/src/framework/builder/pod/container.rs b/rust/operator-binary/src/framework/builder/pod/container.rs index 4914f79..589922f 100644 --- a/rust/operator-binary/src/framework/builder/pod/container.rs +++ b/rust/operator-binary/src/framework/builder/pod/container.rs @@ -17,10 +17,14 @@ pub enum Error { ParseEnvVarName { env_var_name: String }, } +/// Validated environment variable name #[derive(Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)] pub struct EnvVarName(String); impl EnvVarName { + /// Creates an [`EnvVarName`] from the given string and panics if the validation failed + /// + /// Use this only with constant names that are also tested in unit tests! pub fn from_str_unsafe(s: &str) -> Self { EnvVarName::from_str(s).expect("should be a valid environment variable name") } @@ -36,7 +40,7 @@ impl FromStr for EnvVarName { type Err = Error; fn from_str(s: &str) -> Result { - // The length of the environment variable names seems not to be restricted. + // The length of environment variable names seems not to be restricted. if !s.is_empty() && s.chars().all(|c| matches!(c, ' '..='<' | '>'..='~')) { Ok(Self(s.to_owned())) @@ -48,24 +52,35 @@ impl FromStr for EnvVarName { } } +/// A set of [`EnvVar`]s +/// +/// The environment variable names in the set are unique. #[derive(Clone, Debug, Default, PartialEq)] pub struct EnvVarSet(BTreeMap); impl EnvVarSet { + /// Creates an empty [`EnvVarSet`] pub fn new() -> Self { Self::default() } + /// Returns a reference to the [`EnvVar`] with the given name pub fn get(&self, env_var_name: impl Into) -> Option<&EnvVar> { self.0.get(&env_var_name.into()) } + /// Moves all [`EnvVar`]s from the given set into this one. + /// + /// [`EnvVar`]s with the same name are overridden. pub fn merge(mut self, mut env_var_set: EnvVarSet) -> Self { self.0.append(&mut env_var_set.0); self } + /// Adds the given [`EnvVar`]s to this set + /// + /// [`EnvVar`]s with the same name are overridden. pub fn with_values(self, env_vars: I) -> Self where I: IntoIterator, @@ -79,6 +94,9 @@ impl EnvVarSet { }) } + /// Adds an environment variable with the given name and string value to this set + /// + /// An [`EnvVar`] with the same name is overridden. pub fn with_value(mut self, name: impl Into, value: impl Into) -> Self { let name: EnvVarName = name.into(); @@ -94,6 +112,9 @@ impl EnvVarSet { self } + /// Adds an environment variable with the given name and field path to this set + /// + /// An [`EnvVar`] with the same name is overridden. pub fn with_field_path( mut self, name: impl Into, diff --git a/rust/operator-binary/src/framework/builder/pod/volume.rs b/rust/operator-binary/src/framework/builder/pod/volume.rs new file mode 100644 index 0000000..0d0b912 --- /dev/null +++ b/rust/operator-binary/src/framework/builder/pod/volume.rs @@ -0,0 +1,47 @@ +use stackable_operator::{ + builder::pod::volume::ListenerOperatorVolumeSourceBuilder, + k8s_openapi::api::core::v1::PersistentVolumeClaim, kvp::Labels, +}; + +use crate::framework::{ListenerClassName, ListenerName, PersistentVolumeClaimName}; + +/// Infallible variant of [`stackable_operator::builder::pod::volume::ListenerReference`] +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum ListenerReference { + ListenerClass(ListenerClassName), + Listener(ListenerName), +} + +impl From<&ListenerReference> for stackable_operator::builder::pod::volume::ListenerReference { + fn from(value: &ListenerReference) -> Self { + match value { + ListenerReference::ListenerClass(listener_class_name) => { + stackable_operator::builder::pod::volume::ListenerReference::ListenerClass( + listener_class_name.to_string(), + ) + } + ListenerReference::Listener(listener_name) => { + stackable_operator::builder::pod::volume::ListenerReference::ListenerName( + listener_name.to_string(), + ) + } + } + } +} + +/// Infallible variant of +/// [`stackable_operator::builder::pod::volume::ListenerOperatorVolumeSourceBuilder::build_pvc`] +pub fn listener_operator_volume_source_builder_build_pvc( + listener_reference: &ListenerReference, + labels: &Labels, + pvc_name: &PersistentVolumeClaimName, +) -> PersistentVolumeClaim { + ListenerOperatorVolumeSourceBuilder::new(&listener_reference.into(), labels) + .expect("should return Ok independent of the given parameters") + .build_pvc(pvc_name.to_string()) + .expect( + "should return a PersistentVolumeClaim, because the only check is that \ + listener_reference is a valid annotation value and there are no restrictions on single \ + annotation values", + ) +} diff --git a/rust/operator-binary/src/framework/cluster_resources.rs b/rust/operator-binary/src/framework/cluster_resources.rs index 3316be4..b218e53 100644 --- a/rust/operator-binary/src/framework/cluster_resources.rs +++ b/rust/operator-binary/src/framework/cluster_resources.rs @@ -3,24 +3,26 @@ use stackable_operator::{ k8s_openapi::api::core::v1::ObjectReference, }; -use super::{ - ControllerName, HasNamespace, HasObjectName, HasUid, IsLabelValue, OperatorName, ProductName, -}; -use crate::framework::kvp::label::MAX_LABEL_VALUE_LENGTH; +use super::{ClusterName, ControllerName, NamespaceName, OperatorName, ProductName, Uid}; +use crate::framework::{MAX_LABEL_VALUE_LENGTH, NameIsValidLabelValue}; +/// Infallible variant of [`stackable_operator::cluster_resources::ClusterResources::new`] pub fn cluster_resources_new( product_name: &ProductName, operator_name: &OperatorName, controller_name: &ControllerName, - cluster: &(impl HasObjectName + HasNamespace + HasUid), + cluster_name: &ClusterName, + cluster_namespace: &NamespaceName, + cluster_uid: &Uid, apply_strategy: ClusterResourceApplyStrategy, ) -> ClusterResources { + // compile-time check // ClusterResources::new creates a label value from the given app name by appending - // `-operator`. For the resulting label value to be valid, it must not exceed 63 characters. - // Check at compile time that ProductName::MAX_LENGTH is defined accordingly. + // `-operator`. For the resulting label value to be valid, it must not exceed + // MAX_LABEL_VALUE_LENGTH. const _: () = assert!( ProductName::MAX_LENGTH + "-operator".len() <= MAX_LABEL_VALUE_LENGTH, - "The label value `-operator` must not exceed 63 characters." + "The string `-operator` must not exceed the limit of Label names." ); ClusterResources::new( @@ -28,9 +30,9 @@ pub fn cluster_resources_new( &operator_name.to_label_value(), &controller_name.to_label_value(), &ObjectReference { - name: Some(cluster.to_object_name()), - namespace: Some(cluster.to_namespace()), - uid: Some(cluster.to_uid()), + name: Some(cluster_name.to_string()), + namespace: Some(cluster_namespace.to_string()), + uid: Some(cluster_uid.to_string()), ..Default::default() }, apply_strategy, diff --git a/rust/operator-binary/src/framework/kvp/label.rs b/rust/operator-binary/src/framework/kvp/label.rs index b220488..7f87e0f 100644 --- a/rust/operator-binary/src/framework/kvp/label.rs +++ b/rust/operator-binary/src/framework/kvp/label.rs @@ -4,15 +4,13 @@ use stackable_operator::{ }; use crate::framework::{ - ControllerName, HasObjectName, IsLabelValue, OperatorName, ProductName, ProductVersion, + ControllerName, HasName, NameIsValidLabelValue, OperatorName, ProductName, ProductVersion, RoleGroupName, RoleName, }; -pub const MAX_LABEL_VALUE_LENGTH: usize = 63; - -/// Infallible variant of `Labels::recommended` +/// Infallible variant of [`stackable_operator::kvp::Labels::recommended`] pub fn recommended_labels( - owner: &(impl Resource + HasObjectName + IsLabelValue), + owner: &(impl Resource + HasName + NameIsValidLabelValue), product_name: &ProductName, product_version: &ProductVersion, operator_name: &OperatorName, @@ -29,13 +27,15 @@ pub fn recommended_labels( role: &role_name.to_label_value(), role_group: &role_group_name.to_label_value(), }; - Labels::recommended(object_labels) - .expect("Labels should be created because the owner has an object name and all given parameters produce valid label values.") + Labels::recommended(object_labels).expect( + "Labels should be created because the owner has an object name and all given parameters \ + produce valid label values.", + ) } -/// Infallible variant of `Labels::role_selector` +/// Infallible variant of [`stackable_operator::kvp::Labels::role_selector`] pub fn role_selector( - owner: &(impl Resource + IsLabelValue), + owner: &(impl Resource + HasName + NameIsValidLabelValue), product_name: &ProductName, role_name: &RoleName, ) -> Labels { @@ -47,9 +47,9 @@ pub fn role_selector( .expect("Labels should be created because all given parameters produce valid label values") } -/// Infallible variant of `Labels::role_group_selector` +/// Infallible variant of [`stackable_operator::kvp::Labels::role_group_selector`] pub fn role_group_selector( - owner: &(impl Resource + IsLabelValue), + owner: &(impl Resource + HasName + NameIsValidLabelValue), product_name: &ProductName, role_name: &RoleName, role_group_name: &RoleGroupName, @@ -72,7 +72,7 @@ mod tests { }; use crate::framework::{ - ControllerName, HasObjectName, IsLabelValue, OperatorName, ProductName, ProductVersion, + ControllerName, HasName, NameIsValidLabelValue, OperatorName, ProductName, ProductVersion, RoleGroupName, RoleName, kvp::label::{recommended_labels, role_group_selector, role_selector}, }; @@ -121,8 +121,8 @@ mod tests { } } - impl HasObjectName for Cluster { - fn to_object_name(&self) -> String { + impl HasName for Cluster { + fn to_name(&self) -> String { self.object_meta .name .clone() @@ -130,7 +130,7 @@ mod tests { } } - impl IsLabelValue for Cluster { + impl NameIsValidLabelValue for Cluster { fn to_label_value(&self) -> String { self.object_meta .name diff --git a/rust/operator-binary/src/framework/listener.rs b/rust/operator-binary/src/framework/listener.rs deleted file mode 100644 index 5151aaa..0000000 --- a/rust/operator-binary/src/framework/listener.rs +++ /dev/null @@ -1,19 +0,0 @@ -use stackable_operator::{ - builder::pod::volume::{ListenerOperatorVolumeSourceBuilder, ListenerReference}, - k8s_openapi::api::core::v1::PersistentVolumeClaim, - kvp::Labels, -}; - -pub fn listener_pvc( - listener_group_name: String, - labels: &Labels, - pvc_name: String, -) -> PersistentVolumeClaim { - ListenerOperatorVolumeSourceBuilder::new( - &ListenerReference::ListenerName(listener_group_name), - labels, - ) - .expect("should return Ok independent of the given parameters") - .build_pvc(pvc_name.to_string()) - .expect("should be a valid annotation") -} diff --git a/rust/operator-binary/src/framework/role_group_utils.rs b/rust/operator-binary/src/framework/role_group_utils.rs index c0234c1..c2881d5 100644 --- a/rust/operator-binary/src/framework/role_group_utils.rs +++ b/rust/operator-binary/src/framework/role_group_utils.rs @@ -1,6 +1,25 @@ -use super::{ClusterName, RoleGroupName, RoleName}; -use crate::framework::{HasObjectName, MAX_OBJECT_NAME_LENGTH, kvp::label::MAX_LABEL_VALUE_LENGTH}; +use std::str::FromStr; +use super::{ + ClusterName, ConfigMapName, ListenerName, RoleGroupName, RoleName, StatefulSetName, min, +}; +use crate::{ + attributed_string_type, + framework::{MAX_RFC_1035_LABEL_NAME_LENGTH, ServiceName}, +}; + +attributed_string_type! { + QualifiedRoleGroupName, + "A qualified role group name consisting of the cluster name, role name and role-group name. It is a valid label name as defined in RFC 1035 that can be used e.g. as a name for a Service or a StatefulSet.", + "opensearch-nodes-default", + // Suffixes are added to produce resource names. According compile-time checks ensure that + // max_length cannot be set higher. + (max_length = min(52, MAX_RFC_1035_LABEL_NAME_LENGTH)), + is_rfc_1035_label_name, + is_valid_label_value +} + +/// Type-safe names for role-group resources pub struct ResourceNames { pub cluster_name: ClusterName, pub role_name: RoleName, @@ -8,82 +27,104 @@ pub struct ResourceNames { } impl ResourceNames { - // used at compile-time - #[allow(dead_code)] - const MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH: usize = ClusterName::MAX_LENGTH - + 1 // dash - + RoleName::MAX_LENGTH - + 1 // dash - + RoleGroupName::MAX_LENGTH; - - /// Creates a qualified role group name consisting of the cluster name, role name and role-group - /// name. - /// The result is a valid DNS subdomain name as defined in RFC 1123 that can be used e.g. as a name - /// for a StatefulSet. - fn qualified_role_group_name(&self) -> String { - format!( + /// Creates a qualified role group name in the format + /// `--` + fn qualified_role_group_name(&self) -> QualifiedRoleGroupName { + // compile-time checks + const _: () = assert!( + ClusterName::MAX_LENGTH + + 1 // dash + + RoleName::MAX_LENGTH + + 1 // dash + + RoleGroupName::MAX_LENGTH + <= QualifiedRoleGroupName::MAX_LENGTH, + "The string `--` must not exceed the limit \ + of RFC 1035 label names." + ); + // qualified_role_group_name is only an RFC 1035 label name if it starts with an + // alphabetic character, therefore cluster_name must also be an RFC 1035 label name. + // role_name and role_group_name and the middle of the qualified_role_group_name can + // be RFC 1123 label names because digits are allowed there. + let _ = ClusterName::IS_RFC_1035_LABEL_NAME; + let _ = RoleName::IS_RFC_1123_LABEL_NAME; + let _ = RoleGroupName::IS_RFC_1123_LABEL_NAME; + + QualifiedRoleGroupName::from_str(&format!( "{}-{}-{}", - self.cluster_name.to_object_name(), - self.role_name.to_object_name(), - self.role_group_name.to_object_name() - ) + self.cluster_name, self.role_name, self.role_group_name, + )) + .expect("should be a valid QualifiedRoleGroupName") } - pub fn role_group_config_map(&self) -> String { - // Compile-time check + pub fn role_group_config_map(&self) -> ConfigMapName { + // compile-time check const _: () = assert!( - ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH <= MAX_OBJECT_NAME_LENGTH, - "The ConfigMap name `--` must not exceed 253 characters." + QualifiedRoleGroupName::MAX_LENGTH <= ConfigMapName::MAX_LENGTH, + "The string `--` must not exceed the limit of \ + ConfigMap names." ); + let _ = QualifiedRoleGroupName::IS_RFC_1123_SUBDOMAIN_NAME; - self.qualified_role_group_name() + ConfigMapName::from_str(self.qualified_role_group_name().as_ref()) + .expect("should be a valid ConfigMap name") } - pub fn stateful_set_name(&self) -> String { - // Compile-time check + pub fn stateful_set_name(&self) -> StatefulSetName { + // compile-time checks const _: () = assert!( - // see https://github.com/kubernetes/kubernetes/issues/64023 - ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH - + 1 // dash - + 10 // digits for the controller-revision-hash label - <= MAX_LABEL_VALUE_LENGTH, - "The maximum lengths of the cluster name, role name and role group name must be defined so that the combination of these names (including separators and the sequential pod number or hash) is also a valid object name with a maximum of 63 characters (see RFC 1123)" + QualifiedRoleGroupName::MAX_LENGTH <= StatefulSetName::MAX_LENGTH, + "The string `--` must not exceed the \ + limit of StatefulSet names." ); + let _ = QualifiedRoleGroupName::IS_RFC_1123_LABEL_NAME; + let _ = QualifiedRoleGroupName::IS_VALID_LABEL_VALUE; - self.qualified_role_group_name() + StatefulSetName::from_str(self.qualified_role_group_name().as_ref()) + .expect("should be a valid StatefulSet name") } - pub fn headless_service_name(&self) -> String { + pub fn headless_service_name(&self) -> ServiceName { const SUFFIX: &str = "-headless"; - // Compile-time check + // compile-time checks const _: () = assert!( - ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH + SUFFIX.len() - <= MAX_LABEL_VALUE_LENGTH, - "The Service name `---headless` must not exceed 63 characters." + QualifiedRoleGroupName::MAX_LENGTH + SUFFIX.len() <= ServiceName::MAX_LENGTH, + "The string `---headless` must not exceed the \ + limit of Service names." ); + let _ = QualifiedRoleGroupName::IS_RFC_1035_LABEL_NAME; + let _ = QualifiedRoleGroupName::IS_VALID_LABEL_VALUE; - format!("{}{SUFFIX}", self.qualified_role_group_name()) + ServiceName::from_str(&format!("{}{SUFFIX}", self.qualified_role_group_name())) + .expect("should be a valid Service name") } - pub fn listener_service_name(&self) -> String { - // Compile-time check + pub fn listener_name(&self) -> ListenerName { + // compile-time checks const _: () = assert!( - ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH <= MAX_OBJECT_NAME_LENGTH, - "The listener name `--` must not exceed 253 characters." + QualifiedRoleGroupName::MAX_LENGTH <= ListenerName::MAX_LENGTH, + "The string `--` must not exceed the limit of \ + Listener names." ); + let _ = QualifiedRoleGroupName::IS_RFC_1123_SUBDOMAIN_NAME; - self.qualified_role_group_name() + ListenerName::from_str(self.qualified_role_group_name().as_ref()) + .expect("should be a valid Listener name") } } #[cfg(test)] mod tests { use super::{ClusterName, RoleGroupName, RoleName}; - use crate::framework::role_group_utils::ResourceNames; + use crate::framework::{ + ConfigMapName, ListenerName, ServiceName, StatefulSetName, + role_group_utils::{QualifiedRoleGroupName, ResourceNames}, + }; #[test] fn test_resource_names() { + QualifiedRoleGroupName::test_example(); + let resource_names = ResourceNames { cluster_name: ClusterName::from_str_unsafe("test-cluster"), role_name: RoleName::from_str_unsafe("data-nodes"), @@ -91,24 +132,24 @@ mod tests { }; assert_eq!( - "test-cluster-data-nodes-ssd-storage", + QualifiedRoleGroupName::from_str_unsafe("test-cluster-data-nodes-ssd-storage"), resource_names.qualified_role_group_name() ); assert_eq!( - "test-cluster-data-nodes-ssd-storage", + ConfigMapName::from_str_unsafe("test-cluster-data-nodes-ssd-storage"), resource_names.role_group_config_map() ); assert_eq!( - "test-cluster-data-nodes-ssd-storage", + StatefulSetName::from_str_unsafe("test-cluster-data-nodes-ssd-storage"), resource_names.stateful_set_name() ); assert_eq!( - "test-cluster-data-nodes-ssd-storage-headless", + ServiceName::from_str_unsafe("test-cluster-data-nodes-ssd-storage-headless"), resource_names.headless_service_name() ); assert_eq!( - "test-cluster-data-nodes-ssd-storage", - resource_names.listener_service_name() + ListenerName::from_str_unsafe("test-cluster-data-nodes-ssd-storage"), + resource_names.listener_name() ); } } diff --git a/rust/operator-binary/src/framework/role_utils.rs b/rust/operator-binary/src/framework/role_utils.rs index 2829c1c..cfda143 100644 --- a/rust/operator-binary/src/framework/role_utils.rs +++ b/rust/operator-binary/src/framework/role_utils.rs @@ -1,4 +1,7 @@ -use std::collections::{BTreeMap, HashMap}; +use std::{ + collections::{BTreeMap, HashMap}, + str::FromStr, +}; use serde::{Deserialize, Serialize}; use stackable_operator::{ @@ -11,9 +14,14 @@ use stackable_operator::{ schemars::JsonSchema, }; -use super::{ProductName, builder::pod::container::EnvVarSet}; -use crate::framework::{ClusterName, MAX_OBJECT_NAME_LENGTH, kvp::label::MAX_LABEL_VALUE_LENGTH}; +use super::{ + ProductName, RoleBindingName, ServiceAccountName, ServiceName, + builder::pod::container::EnvVarSet, +}; +use crate::framework::{ClusterName, ClusterRoleName}; +/// Variant of [`stackable_operator::role_utils::GenericProductSpecificCommonConfig`] that +/// implements [`Merge`] #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] pub struct GenericProductSpecificCommonConfig {} @@ -21,7 +29,12 @@ impl Merge for GenericProductSpecificCommonConfig { fn merge(&mut self, _defaults: &Self) {} } -// much better to work with than RoleGroup +/// Variant of [`stackable_operator::role_utils::RoleGroup`] that is easier to work with +/// +/// Differences are: +/// * `replicas` is non-optional. +/// * `config` is flattened. +/// * The [`HashMap`] in `env_overrides` is replaced with an [`EnvVarSet`]. #[derive(Clone, Debug, PartialEq)] pub struct RoleGroupConfig { pub replicas: u16, @@ -45,7 +58,9 @@ impl RoleGroupConfig( role_group: &RoleGroup, role: &Role, @@ -64,7 +79,7 @@ where fragment::validate(rolegroup_config) } -// also useful for operators which use the product config +/// Merges and validates the [`RoleGroup`] with the given `role` and `default_config` pub fn with_validated_config( role_group: &RoleGroup, role: &Role, @@ -155,48 +170,65 @@ where merge(role_group_config, &role_config) } +/// Type-safe names for role resources pub struct ResourceNames { pub cluster_name: ClusterName, pub product_name: ProductName, } impl ResourceNames { - pub fn service_account_name(&self) -> String { + pub fn service_account_name(&self) -> ServiceAccountName { const SUFFIX: &str = "-serviceaccount"; - // Compile-time check + // compile-time checks const _: () = assert!( - ClusterName::MAX_LENGTH + SUFFIX.len() <= MAX_OBJECT_NAME_LENGTH, - "The ServiceAccount name `-serviceaccount` must not exceed 253 characters." + ClusterName::MAX_LENGTH + SUFFIX.len() <= ServiceAccountName::MAX_LENGTH, + "The string `-serviceaccount` must not exceed the limit of ServiceAccount names." ); + let _ = ClusterName::IS_RFC_1123_SUBDOMAIN_NAME; - format!("{}{SUFFIX}", self.cluster_name) + ServiceAccountName::from_str(&format!("{}{SUFFIX}", self.cluster_name)) + .expect("should be a valid ServiceAccount name") } - pub fn role_binding_name(&self) -> String { + pub fn role_binding_name(&self) -> RoleBindingName { const SUFFIX: &str = "-rolebinding"; - // No compile-time check, because RoleBinding names do not seem to be restricted. + // compile-time checks + const _: () = assert!( + ClusterName::MAX_LENGTH + SUFFIX.len() <= RoleBindingName::MAX_LENGTH, + "The string `-rolebinding` must not exceed the limit of RoleBinding names." + ); + let _ = ClusterName::IS_RFC_1123_SUBDOMAIN_NAME; - format!("{}{SUFFIX}", self.cluster_name) + RoleBindingName::from_str(&format!("{}{SUFFIX}", self.cluster_name)) + .expect("should be a valid RoleBinding name") } - pub fn cluster_role_name(&self) -> String { + pub fn cluster_role_name(&self) -> ClusterRoleName { const SUFFIX: &str = "-clusterrole"; - // No compile-time check, because ClusterRole names do not seem to be restricted. + // compile-time checks + const _: () = assert!( + ProductName::MAX_LENGTH + SUFFIX.len() <= ClusterRoleName::MAX_LENGTH, + "The string `-clusterrole` must not exceed the limit of cluster role names." + ); + let _ = ProductName::IS_RFC_1123_SUBDOMAIN_NAME; - format!("{}{SUFFIX}", self.product_name) + ClusterRoleName::from_str(&format!("{}{SUFFIX}", self.product_name)) + .expect("should be a valid cluster role name") } - pub fn discovery_service_name(&self) -> String { - // Compile-time check + pub fn discovery_service_name(&self) -> ServiceName { + // compile-time checks const _: () = assert!( - ClusterName::MAX_LENGTH <= MAX_LABEL_VALUE_LENGTH, - "The Service name `` must not exceed 63 characters." + ClusterName::MAX_LENGTH <= ServiceName::MAX_LENGTH, + "The string `` must not exceed the limit of Service names." ); + let _ = ClusterName::IS_RFC_1035_LABEL_NAME; + let _ = ClusterName::IS_VALID_LABEL_VALUE; - format!("{}", self.cluster_name) + ServiceName::from_str(self.cluster_name.as_ref()).expect("should be a valid Service name") } } @@ -215,7 +247,10 @@ mod tests { }; use super::ResourceNames; - use crate::framework::{ClusterName, ProductName, role_utils::with_validated_config}; + use crate::framework::{ + ClusterName, ClusterRoleName, ProductName, RoleBindingName, ServiceAccountName, + ServiceName, role_utils::with_validated_config, + }; #[derive(Debug, Fragment, PartialEq)] #[fragment_attrs(derive(Clone, Debug, Default, Merge, PartialEq))] @@ -359,19 +394,19 @@ mod tests { }; assert_eq!( - "my-cluster-serviceaccount".to_owned(), + ServiceAccountName::from_str_unsafe("my-cluster-serviceaccount"), resource_names.service_account_name() ); assert_eq!( - "my-cluster-rolebinding".to_owned(), + RoleBindingName::from_str_unsafe("my-cluster-rolebinding"), resource_names.role_binding_name() ); assert_eq!( - "my-product-clusterrole".to_owned(), + ClusterRoleName::from_str_unsafe("my-product-clusterrole"), resource_names.cluster_role_name() ); assert_eq!( - "my-cluster".to_owned(), + ServiceName::from_str_unsafe("my-cluster"), resource_names.discovery_service_name() ); } diff --git a/tests/templates/kuttl/smoke/10-assert.yaml.j2 b/tests/templates/kuttl/smoke/10-assert.yaml.j2 index 760f936..729b588 100644 --- a/tests/templates/kuttl/smoke/10-assert.yaml.j2 +++ b/tests/templates/kuttl/smoke/10-assert.yaml.j2 @@ -1,6 +1,6 @@ # All fields are checked that are set by the operator. -# This helps to detect unintentional changes. -# The maintenance effort should be okay as long as it is only done in the smoke test. +# This helps to detect unintentional changes. It is also a good reference for the output of the +# operator. The maintenance effort should be okay as long as it is only done in the smoke test. --- apiVersion: kuttl.dev/v1beta1 kind: TestAssert @@ -600,3 +600,55 @@ spec: app.kubernetes.io/component: nodes app.kubernetes.io/instance: opensearch app.kubernetes.io/name: opensearch +--- +apiVersion: listeners.stackable.tech/v1alpha1 +kind: Listener +metadata: + labels: + app.kubernetes.io/component: nodes + app.kubernetes.io/instance: opensearch + app.kubernetes.io/managed-by: opensearch.stackable.tech_opensearchcluster + app.kubernetes.io/name: opensearch + app.kubernetes.io/role-group: cluster-manager + app.kubernetes.io/version: 3.1.0 + stackable.tech/vendor: Stackable + name: opensearch-nodes-cluster-manager + ownerReferences: + - apiVersion: opensearch.stackable.tech/v1alpha1 + controller: true + kind: OpenSearchCluster + name: opensearch +spec: + className: external-stable + extraPodSelectorLabels: {} + ports: + - name: http + port: 9200 + protocol: TCP + publishNotReadyAddresses: null +--- +apiVersion: listeners.stackable.tech/v1alpha1 +kind: Listener +metadata: + labels: + app.kubernetes.io/component: nodes + app.kubernetes.io/instance: opensearch + app.kubernetes.io/managed-by: opensearch.stackable.tech_opensearchcluster + app.kubernetes.io/name: opensearch + app.kubernetes.io/role-group: data + app.kubernetes.io/version: 3.1.0 + stackable.tech/vendor: Stackable + name: opensearch-nodes-data + ownerReferences: + - apiVersion: opensearch.stackable.tech/v1alpha1 + controller: true + kind: OpenSearchCluster + name: opensearch +spec: + className: cluster-internal + extraPodSelectorLabels: {} + ports: + - name: http + port: 9200 + protocol: TCP + publishNotReadyAddresses: null