From 90013aa8c8860a2fe9b00a186ada47e976721e19 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Thu, 11 Sep 2025 18:50:58 +0200 Subject: [PATCH 1/7] chore: Use specific types for each Kubernetes (resource) name --- Cargo.lock | 11 + Cargo.nix | 66 +++ Cargo.toml | 1 + rust/operator-binary/Cargo.toml | 1 + rust/operator-binary/src/controller.rs | 93 ++-- rust/operator-binary/src/controller/apply.rs | 12 +- rust/operator-binary/src/controller/build.rs | 10 +- .../src/controller/build/node_config.rs | 14 +- .../src/controller/build/role_builder.rs | 18 +- .../controller/build/role_group_builder.rs | 84 ++-- .../src/controller/validate.rs | 46 +- rust/operator-binary/src/crd/mod.rs | 2 +- rust/operator-binary/src/framework.rs | 431 +++++++++++++++--- .../src/framework/builder/meta.rs | 27 +- .../src/framework/builder/pdb.rs | 29 +- .../src/framework/builder/pod.rs | 1 + .../src/framework/builder/pod/volume.rs | 46 ++ .../src/framework/cluster_resources.rs | 24 +- .../src/framework/kvp/label.rs | 30 +- .../operator-binary/src/framework/listener.rs | 19 - .../src/framework/role_group_utils.rs | 64 +-- .../src/framework/role_utils.rs | 64 ++- tests/templates/kuttl/smoke/10-assert.yaml.j2 | 56 ++- 23 files changed, 875 insertions(+), 274 deletions(-) create mode 100644 rust/operator-binary/src/framework/builder/pod/volume.rs delete mode 100644 rust/operator-binary/src/framework/listener.rs 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/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..c8198b8 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..216fe08 100644 --- a/rust/operator-binary/src/controller/apply.rs +++ b/rust/operator-binary/src/controller/apply.rs @@ -8,9 +8,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))] @@ -37,14 +35,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, ); diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index e105b1b..a81672d 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -52,6 +52,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 +62,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 +143,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..a68e563 100644 --- a/rust/operator-binary/src/controller/build/node_config.rs +++ b/rust/operator-binary/src/controller/build/node_config.rs @@ -8,6 +8,7 @@ use crate::{ controller::OpenSearchRoleGroupConfig, crd::v1alpha1, framework::{ + ServiceName, builder::pod::container::{EnvVarName, EnvVarSet}, role_group_utils, }, @@ -43,7 +44,7 @@ pub const CONFIG_OPTION_PLUGINS_SECURITY_NODES_DN: &str = "plugins.security.node 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 +53,7 @@ impl NodeConfig { pub fn new( cluster: ValidatedCluster, role_group_config: OpenSearchRoleGroupConfig, - discovery_service_name: String, + discovery_service_name: ServiceName, ) -> Self { Self { cluster, @@ -248,13 +249,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 +305,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 +318,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..e3c5509 100644 --- a/rust/operator-binary/src/controller/build/role_builder.rs +++ b/rust/operator-binary/src/controller/build/role_builder.rs @@ -21,7 +21,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, }, @@ -83,13 +83,13 @@ 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()), }]), } } @@ -208,6 +208,7 @@ mod tests { k8s_openapi::api::core::v1::PodTemplateSpec, role_utils::GenericRoleConfig, }; + use uuid::uuid; use super::RoleBuilder; use crate::{ @@ -216,8 +217,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 +260,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..a5e3a37 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,5 @@ +use std::str::FromStr; + use stackable_operator::{ builder::{meta::ObjectMetaBuilder, pod::container::ContainerBuilder}, crd::listener::{self}, @@ -21,10 +23,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 +49,21 @@ 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") +} + pub struct RoleGroupBuilder<'a> { - service_account_name: String, + service_account_name: ServiceAccountName, cluster: ValidatedCluster, node_config: NodeConfig, role_group_name: RoleGroupName, @@ -54,12 +74,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, @@ -111,19 +131,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 +159,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() @@ -194,16 +214,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() @@ -300,14 +320,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 { @@ -350,7 +370,7 @@ impl<'a> RoleGroupBuilder<'a> { ]; self.build_role_group_service( - self.resource_names.headless_service_name(), + &self.resource_names.headless_service_name(), ports, Self::prometheus_labels(), Self::prometheus_annotations(self.node_config.tls_on_http_port_enabled()), @@ -389,7 +409,7 @@ impl<'a> RoleGroupBuilder<'a> { fn build_role_group_service( &self, - service_name: impl Into, + service_name: &ServiceName, ports: Vec, extra_labels: Labels, extra_annotations: Annotations, @@ -419,7 +439,7 @@ impl<'a> RoleGroupBuilder<'a> { 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(); @@ -495,19 +515,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 +575,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 +598,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/validate.rs b/rust/operator-binary/src/controller/validate.rs index 98bf027..b8a2b2c 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -15,7 +15,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 +36,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, @@ -69,9 +75,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 +177,7 @@ mod tests { role_utils::{CommonConfiguration, GenericRoleConfig, Role, RoleGroup}, time::Duration, }; + use uuid::uuid; use super::{ErrorDiscriminants, validate}; use crate::{ @@ -178,7 +187,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 +204,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 +374,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 +390,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..a4a43b2 100644 --- a/rust/operator-binary/src/framework.rs +++ b/rust/operator-binary/src/framework.rs @@ -1,62 +1,97 @@ -// 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 strum::{EnumDiscriminants, IntoStaticStr}; +use uuid::Uuid; 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 DNS subdomain name as defined in RFC 1123"))] + InvalidRfc1123DnsSubdomainName { source: stackable_operator::validation::Errors, }, - #[snafu(display("failed to use as label"))] - InvalidLabelValue { - source: stackable_operator::kvp::LabelValueError, + #[snafu(display("not a valid label name as defined in RFC 1123"))] + InvalidRfc1123LabelName { + source: stackable_operator::validation::Errors, }, + + #[snafu(display("not a valid UUID"))] + InvalidUid { source: uuid::Error }, } -/// 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; +/// 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; -/// 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; -} +/// 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 values +/// +/// Duplicates the private constant `stackable-operator::kvp::label::value::LABEL_VALUE_MAX_LEN` +pub const MAX_LABEL_VALUE_LENGTH: usize = 63; -/// Has a namespace -pub trait HasNamespace { - fn to_namespace(&self) -> String; +/// 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; } @@ -73,11 +108,34 @@ macro_rules! attributed_string_type { } } + 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 FromStr for $name { type Err = Error; fn from_str(s: &str) -> std::result::Result { + ensure!( + !s.is_empty(), + EmptyStringSnafu {} + ); + $(attributed_string_type!(@from_str $name, s, $attribute);)* Ok(Self(s.to_owned())) @@ -109,27 +167,43 @@ macro_rules! attributed_string_type { } ); }; - (@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(InvalidRfc1123DnsSubdomainNameSnafu)?; + }; + (@from_str $name:ident, $s:expr, is_rfc_1123_label_name) => { + stackable_operator::validation::is_rfc_1123_label($s).context(InvalidRfc1123LabelNameSnafu)?; }; (@from_str $name:ident, $s:expr, is_valid_label_value) => { LabelValue::from_str($s).context(InvalidLabelValueSnafu)?; }; + (@from_str $name:ident, $s:expr, is_uid) => { + Uuid::try_parse($s).context(InvalidUidSnafu)?; + }; (@trait_impl $name:ident, (max_length = $max_length:expr)) => { impl $name { // type arithmetic would be better 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_1123_dns_subdomain_name) => { + }; + (@trait_impl $name:ident, is_rfc_1123_label_name) => { + }; + (@trait_impl $name:ident, is_uid) => { + impl From for $name { + fn from(value: Uuid) -> Self { + Self(value.to_string()) + } + } + + impl From<&Uuid> for $name { + fn from(value: &Uuid) -> Self { + Self(value.to_string()) } } }; (@trait_impl $name:ident, is_valid_label_value) => { - impl IsLabelValue for $name { + impl NameIsValidLabelValue for $name { fn to_label_value(&self) -> String { self.0.clone() } @@ -137,13 +211,130 @@ 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_1123_LABEL_NAME_LENGTH, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1123_label_name, + is_valid_label_value +} +attributed_string_type! { + StatefulSetName, + "The name of a StatefulSet", + "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 +} +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_valid_label_value } attributed_string_type! { @@ -159,8 +350,7 @@ attributed_string_type! { "my-opensearch-cluster", // Suffixes are added to produce a resource names. According compile-time check ensures that // max_length cannot be set higher. - (max_length = 24), - is_object_name, + (max_length = min(24, MAX_LABEL_VALUE_LENGTH)), is_valid_label_value } attributed_string_type! { @@ -181,30 +371,52 @@ 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_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_valid_label_value } #[cfg(test)] mod tests { - use std::str::FromStr; + use std::{fmt::Display, str::FromStr}; + + use snafu::{ResultExt, ensure}; + use uuid::{Uuid, uuid}; use super::{ - ClusterName, ControllerName, OperatorName, ProductVersion, RoleGroupName, RoleName, + ClusterName, ClusterRoleName, ConfigMapName, ControllerName, EmptyStringSnafu, Error, + ErrorDiscriminants, InvalidLabelValueSnafu, InvalidRfc1123DnsSubdomainNameSnafu, + InvalidRfc1123LabelNameSnafu, InvalidUidSnafu, LabelValue, LengthExceededSnafu, + 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 +426,138 @@ 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_fmt() { + 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!( - "my-cluster-name".to_owned(), - format!("{}", ClusterName::from_str_unsafe("my-cluster-name")) + Err(ErrorDiscriminants::LengthExceeded), + T::from_str("testX").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()); + 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", + "a-b", + 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; + + 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; + + 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..c41a136 100644 --- a/rust/operator-binary/src/framework/builder/meta.rs +++ b/rust/operator-binary/src/framework/builder/meta.rs @@ -3,19 +3,21 @@ 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` 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 +36,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 +83,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 +93,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..2fd4303 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/volume.rs b/rust/operator-binary/src/framework/builder/pod/volume.rs new file mode 100644 index 0000000..1e3ebaa --- /dev/null +++ b/rust/operator-binary/src/framework/builder/pod/volume.rs @@ -0,0 +1,46 @@ +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..0e3819e 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..681aa83 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::label::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::label::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::label::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..5e6359b 100644 --- a/rust/operator-binary/src/framework/role_group_utils.rs +++ b/rust/operator-binary/src/framework/role_group_utils.rs @@ -1,5 +1,7 @@ -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}; +use crate::framework::ServiceName; pub struct ResourceNames { pub cluster_name: ClusterName, @@ -23,64 +25,72 @@ impl ResourceNames { fn qualified_role_group_name(&self) -> String { 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, ) } - pub fn role_group_config_map(&self) -> String { + 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." + ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH <= ConfigMapName::MAX_LENGTH, + "The string `--` must not exceed the limit of \ + ConfigMap names." ); - self.qualified_role_group_name() + ConfigMapName::from_str(&self.qualified_role_group_name()) + .expect("should be a valid ConfigMap name") } - pub fn stateful_set_name(&self) -> String { + pub fn stateful_set_name(&self) -> StatefulSetName { // Compile-time check 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)" + <= StatefulSetName::MAX_LENGTH, + "The string `---` \ + must not exceed the limit of StatefulSet names." ); - self.qualified_role_group_name() + StatefulSetName::from_str(&self.qualified_role_group_name()) + .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 const _: () = assert!( ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH + SUFFIX.len() - <= MAX_LABEL_VALUE_LENGTH, - "The Service name `---headless` must not exceed 63 characters." + <= ServiceName::MAX_LENGTH, + "The string `---headless` must not exceed the \ + limit of Service names." ); - 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 { + pub fn listener_name(&self) -> ListenerName { // Compile-time check const _: () = assert!( - ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH <= MAX_OBJECT_NAME_LENGTH, - "The listener name `--` must not exceed 253 characters." + ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH <= ListenerName::MAX_LENGTH, + "The string `--` must not exceed the limit of \ + Listener names." ); - self.qualified_role_group_name() + ListenerName::from_str(&self.qualified_role_group_name()) + .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::ResourceNames, + }; #[test] fn test_resource_names() { @@ -95,20 +105,20 @@ mod tests { 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..db6cc27 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,8 +14,11 @@ 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}; #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] pub struct GenericProductSpecificCommonConfig {} @@ -161,42 +167,53 @@ pub struct ResourceNames { } impl ResourceNames { - pub fn service_account_name(&self) -> String { + pub fn service_account_name(&self) -> ServiceAccountName { const SUFFIX: &str = "-serviceaccount"; // Compile-time check 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." ); - 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 check + const _: () = assert!( + ClusterName::MAX_LENGTH + SUFFIX.len() <= RoleBindingName::MAX_LENGTH, + "The string `-rolebinding` must not exceed the limit of RoleBinding names." + ); - 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 check + const _: () = assert!( + ProductName::MAX_LENGTH + SUFFIX.len() <= ClusterRoleName::MAX_LENGTH, + "The string `-clusterrole` must not exceed the limit of cluster role names." + ); - 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 { + pub fn discovery_service_name(&self) -> ServiceName { // Compile-time check 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." ); - format!("{}", self.cluster_name) + ServiceName::from_str(self.cluster_name.as_ref()).expect("should be a valid Service name") } } @@ -215,7 +232,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 +379,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 From 1a30db1d6bad854d706bc7184c21352da72c328e Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Mon, 15 Sep 2025 17:31:42 +0200 Subject: [PATCH 2/7] doc: Add code comments --- rust/operator-binary/src/controller/apply.rs | 5 ++++ rust/operator-binary/src/controller/build.rs | 7 ++++++ .../src/controller/build/role_builder.rs | 6 ++--- .../src/controller/update_status.rs | 1 + .../src/controller/validate.rs | 8 ++++++- .../src/framework/builder/pod/container.rs | 23 ++++++++++++++++++- .../src/framework/role_group_utils.rs | 1 + .../src/framework/role_utils.rs | 16 ++++++++++--- 8 files changed, 59 insertions(+), 8 deletions(-) diff --git a/rust/operator-binary/src/controller/apply.rs b/rust/operator-binary/src/controller/apply.rs index 216fe08..01ee68e 100644 --- a/rust/operator-binary/src/controller/apply.rs +++ b/rust/operator-binary/src/controller/apply.rs @@ -26,6 +26,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, @@ -56,6 +60,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 a81672d..f10661c 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -8,6 +8,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![]; diff --git a/rust/operator-binary/src/controller/build/role_builder.rs b/rust/operator-binary/src/controller/build/role_builder.rs index e3c5509..3c376ff 100644 --- a/rust/operator-binary/src/controller/build/role_builder.rs +++ b/rust/operator-binary/src/controller/build/role_builder.rs @@ -177,11 +177,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); diff --git a/rust/operator-binary/src/controller/update_status.rs b/rust/operator-binary/src/controller/update_status.rs index 923f7db..9480a34 100644 --- a/rust/operator-binary/src/controller/update_status.rs +++ b/rust/operator-binary/src/controller/update_status.rs @@ -32,6 +32,7 @@ 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 b8a2b2c..b1da37b 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -67,7 +67,13 @@ pub enum Error { type Result = std::result::Result; -// no client needed +/// Validates the `v1alpha1::OpenSearchCluster` and returns a `ValidateCluster` +/// +/// 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, diff --git a/rust/operator-binary/src/framework/builder/pod/container.rs b/rust/operator-binary/src/framework/builder/pod/container.rs index 4914f79..b05d0e4 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/role_group_utils.rs b/rust/operator-binary/src/framework/role_group_utils.rs index 5e6359b..f887ba7 100644 --- a/rust/operator-binary/src/framework/role_group_utils.rs +++ b/rust/operator-binary/src/framework/role_group_utils.rs @@ -3,6 +3,7 @@ use std::str::FromStr; use super::{ClusterName, ConfigMapName, ListenerName, RoleGroupName, RoleName, StatefulSetName}; use crate::framework::ServiceName; +/// Type-safe names for role-group resources pub struct ResourceNames { pub cluster_name: ClusterName, pub role_name: RoleName, diff --git a/rust/operator-binary/src/framework/role_utils.rs b/rust/operator-binary/src/framework/role_utils.rs index db6cc27..a8fa419 100644 --- a/rust/operator-binary/src/framework/role_utils.rs +++ b/rust/operator-binary/src/framework/role_utils.rs @@ -20,6 +20,8 @@ use super::{ }; 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 {} @@ -27,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, @@ -51,7 +58,9 @@ impl RoleGroupConfig( role_group: &RoleGroup, role: &Role, @@ -70,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, @@ -161,6 +170,7 @@ where merge(role_group_config, &role_config) } +/// Type-safe names for role resources pub struct ResourceNames { pub cluster_name: ClusterName, pub product_name: ProductName, From 53d0529cd20f1d43b81d9d8020dfbbea6f7b14d8 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Tue, 16 Sep 2025 10:33:24 +0200 Subject: [PATCH 3/7] doc: Add code comments --- rust/operator-binary/src/controller/apply.rs | 2 + rust/operator-binary/src/controller/build.rs | 2 + .../src/controller/build/node_config.rs | 50 +++++++--- .../src/controller/build/role_builder.rs | 18 +++- .../controller/build/role_group_builder.rs | 95 ++++++++++--------- .../src/controller/update_status.rs | 2 + .../src/controller/validate.rs | 2 + 7 files changed, 113 insertions(+), 58 deletions(-) diff --git a/rust/operator-binary/src/controller/apply.rs b/rust/operator-binary/src/controller/apply.rs index 01ee68e..16202bd 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}; diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index f10661c..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; diff --git a/rust/operator-binary/src/controller/build/node_config.rs b/rust/operator-binary/src/controller/build/node_config.rs index a68e563..8268384 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}; @@ -14,33 +16,49 @@ use crate::{ }, }; +/// 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, @@ -62,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(); @@ -107,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 @@ -124,7 +147,10 @@ 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. diff --git a/rust/operator-binary/src/controller/build/role_builder.rs b/rust/operator-binary/src/controller/build/role_builder.rs index 3c376ff..ccef780 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::{ @@ -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()); @@ -94,6 +100,14 @@ impl<'a> RoleBuilder<'a> { } } + /// Builds a Service that references all nodes with the cluster_manager node role + /// + /// Initially, this service was meant to be used by + /// `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( 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 a5e3a37..4f75be8 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,5 @@ +//! Builder for role-group resources + use std::str::FromStr; use stackable_operator::{ @@ -62,6 +64,7 @@ fn listener_volume_name() -> PersistentVolumeClaimName { .expect("should be a valid PersistentVolumeClaim name") } +/// Builder for role-group resources pub struct RoleGroupBuilder<'a> { service_account_name: ServiceAccountName, cluster: ValidatedCluster, @@ -100,6 +103,7 @@ 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()) @@ -118,6 +122,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()) @@ -172,6 +177,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() { @@ -237,6 +243,10 @@ impl<'a> RoleGroupBuilder<'a> { pod_template } + /// Returns the labels of OpenSearch nodes with the `cluster_manager` role. + /// + /// As described in `RoleBuilder::build_cluster_manager_service`, this function will be + /// changed or deleted. pub fn cluster_manager_labels( cluster: &ValidatedCluster, context_names: &ContextNames, @@ -254,6 +264,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`. @@ -264,6 +275,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 @@ -355,7 +367,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()), @@ -369,12 +390,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 @@ -407,36 +437,10 @@ impl<'a> RoleGroupBuilder<'a> { .expect("should be valid annotations") } - fn build_role_group_service( - &self, - service_name: &ServiceName, - 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 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_name()) @@ -444,25 +448,24 @@ impl<'a> RoleGroupBuilder<'a> { 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(); @@ -479,6 +482,7 @@ impl<'a> RoleGroupBuilder<'a> { builder } + /// Recommended labels for role-group resources fn recommended_labels(&self) -> Labels { recommended_labels( &self.cluster, @@ -491,6 +495,7 @@ impl<'a> RoleGroupBuilder<'a> { ) } + /// Labels to select a Pod in the role-group fn pod_selector(&self) -> Labels { role_group_selector( &self.cluster, diff --git a/rust/operator-binary/src/controller/update_status.rs b/rust/operator-binary/src/controller/update_status.rs index 9480a34..e13181a 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, diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index b1da37b..10a589f 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}; From 68afc57234b57166665c318151fd606c413a4278 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Tue, 16 Sep 2025 13:01:25 +0200 Subject: [PATCH 4/7] doc: Fix references --- rust/operator-binary/src/controller.rs | 22 +++++++++--------- rust/operator-binary/src/controller/apply.rs | 2 +- .../src/controller/build/node_config.rs | 10 ++++---- .../src/controller/build/role_builder.rs | 4 ++-- .../controller/build/role_group_builder.rs | 23 +++++++++++-------- .../src/controller/update_status.rs | 3 ++- .../src/controller/validate.rs | 2 +- rust/operator-binary/src/framework.rs | 23 ++++++++++--------- .../src/framework/builder/meta.rs | 3 ++- .../src/framework/builder/pdb.rs | 2 +- .../src/framework/builder/pod/container.rs | 20 ++++++++-------- .../src/framework/builder/pod/volume.rs | 5 ++-- .../src/framework/cluster_resources.rs | 4 ++-- .../src/framework/kvp/label.rs | 6 ++--- .../src/framework/role_group_utils.rs | 5 +++- .../src/framework/role_utils.rs | 12 +++++----- 16 files changed, 78 insertions(+), 68 deletions(-) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index c8198b8..2823c98 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -1,4 +1,4 @@ -//! Controller for `v1alpha1::OpenSearchCluster` +//! Controller for [`v1alpha1::OpenSearchCluster`] //! //! The cluster specification is validated, Kubernetes resource specifications are created and //! applied and the cluster status is updated. @@ -46,7 +46,7 @@ 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 +/// 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, @@ -123,7 +123,7 @@ type OpenSearchRoleGroupConfig = type OpenSearchNodeResources = stackable_operator::commons::resources::Resources; -/// The validated `v1alpha1::OpenSearchConfig`. +/// The validated [`v1alpha1::OpenSearchConfig`] #[derive(Clone, Debug, PartialEq)] pub struct ValidatedOpenSearchConfig { pub affinity: StackableAffinity, @@ -133,14 +133,14 @@ pub struct ValidatedOpenSearchConfig { pub listener_class: String, } -/// The validated `v1alpha1::OpenSearchCluster` +/// 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. +/// 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, @@ -275,7 +275,7 @@ 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. +/// 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 @@ -330,8 +330,8 @@ 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 +/// `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, diff --git a/rust/operator-binary/src/controller/apply.rs b/rust/operator-binary/src/controller/apply.rs index 16202bd..49afd02 100644 --- a/rust/operator-binary/src/controller/apply.rs +++ b/rust/operator-binary/src/controller/apply.rs @@ -31,7 +31,7 @@ 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. +/// stackable_operator if [`KubernetesResources`] would contain all possible resource types. pub struct Applier<'a> { client: &'a Client, cluster_resources: ClusterResources, diff --git a/rust/operator-binary/src/controller/build/node_config.rs b/rust/operator-binary/src/controller/build/node_config.rs index 8268384..379104e 100644 --- a/rust/operator-binary/src/controller/build/node_config.rs +++ b/rust/operator-binary/src/controller/build/node_config.rs @@ -136,7 +136,7 @@ impl NodeConfig { == Some(true) } - /// Converts the given JSON value to `bool` if possible + /// 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 @@ -154,7 +154,7 @@ impl NodeConfig { 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, @@ -189,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 /// @@ -236,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. diff --git a/rust/operator-binary/src/controller/build/role_builder.rs b/rust/operator-binary/src/controller/build/role_builder.rs index ccef780..4423e71 100644 --- a/rust/operator-binary/src/controller/build/role_builder.rs +++ b/rust/operator-binary/src/controller/build/role_builder.rs @@ -103,7 +103,7 @@ impl<'a> RoleBuilder<'a> { /// Builds a Service that references all nodes with the cluster_manager node role /// /// Initially, this service was meant to be used by - /// `NodeConfig::initial_cluster_manager_nodes`, but the function uses now another approach. + /// [`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 @@ -144,7 +144,7 @@ impl<'a> RoleBuilder<'a> { } } - /// Builds a PodDisruptionBudget used by all role-groups + /// Builds a [`PodDisruptionBudget`] used by all role-groups pub fn build_pdb(&self) -> Option { let pdb_config = &self.cluster.role_config.pod_disruption_budget; 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 4f75be8..63afa9e 100644 --- a/rust/operator-binary/src/controller/build/role_group_builder.rs +++ b/rust/operator-binary/src/controller/build/role_group_builder.rs @@ -103,7 +103,8 @@ impl<'a> RoleGroupBuilder<'a> { } } - /// Builds the ConfigMap containing the configuration files of the role-group StatefulSet + /// 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()) @@ -122,7 +123,7 @@ impl<'a> RoleGroupBuilder<'a> { } } - /// Builds the role-group StatefulSet + /// Builds the role-group [`StatefulSet`] pub fn build_stateful_set(&self) -> StatefulSet { let metadata = self .common_metadata(self.resource_names.stateful_set_name()) @@ -177,7 +178,7 @@ impl<'a> RoleGroupBuilder<'a> { } } - /// Builds the PodTemplateSpec for the role-group StatefulSet + /// 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() { @@ -245,8 +246,8 @@ impl<'a> RoleGroupBuilder<'a> { /// Returns the labels of OpenSearch nodes with the `cluster_manager` role. /// - /// As described in `RoleBuilder::build_cluster_manager_service`, this function will be - /// changed or deleted. + /// 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, @@ -275,7 +276,7 @@ impl<'a> RoleGroupBuilder<'a> { .expect("should be a valid label") } - /// Builds the container for the PodTemplateSpec + /// Builds the container for the [`PodTemplateSpec`] fn build_container(&self, role_group_config: &OpenSearchRoleGroupConfig) -> Container { let product_image = self .cluster @@ -312,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")) @@ -367,7 +368,7 @@ impl<'a> RoleGroupBuilder<'a> { .build() } - /// Builds the headless Service for the role-group + /// 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()) @@ -437,7 +438,7 @@ impl<'a> RoleGroupBuilder<'a> { .expect("should be valid annotations") } - /// Builds the Listener for the role-group + /// 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. @@ -495,7 +496,9 @@ impl<'a> RoleGroupBuilder<'a> { ) } - /// Labels to select a Pod in the role-group + /// 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, diff --git a/rust/operator-binary/src/controller/update_status.rs b/rust/operator-binary/src/controller/update_status.rs index e13181a..b018533 100644 --- a/rust/operator-binary/src/controller/update_status.rs +++ b/rust/operator-binary/src/controller/update_status.rs @@ -34,7 +34,8 @@ pub enum Error { type Result = std::result::Result; -/// Updates the status of the `v1alpha1::OpenSearchCluster` according to the given applied resources +/// 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 10a589f..ac2015f 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -69,7 +69,7 @@ pub enum Error { type Result = std::result::Result; -/// Validates the `v1alpha1::OpenSearchCluster` and returns a `ValidateCluster` +/// 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. diff --git a/rust/operator-binary/src/framework.rs b/rust/operator-binary/src/framework.rs index a4a43b2..bebe388 100644 --- a/rust/operator-binary/src/framework.rs +++ b/rust/operator-binary/src/framework.rs @@ -1,7 +1,7 @@ //! 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 +//! 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, @@ -9,16 +9,16 @@ //! //! 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`. +//! 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. +//! 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. +//! 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}; @@ -63,17 +63,18 @@ pub enum Error { /// Maximum length of DNS subdomain names as defined in RFC 1123. /// -/// Duplicates the private constant `stackable-operator::validation::RFC_1123_SUBDOMAIN_MAX_LENGTH` +/// 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` +/// 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 values /// -/// Duplicates the private constant `stackable-operator::kvp::label::value::LABEL_VALUE_MAX_LEN` +/// 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 @@ -213,7 +214,7 @@ 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. +/// As opposed to [`std::cmp::min`], this function can be used at compile-time. /// /// # Examples /// diff --git a/rust/operator-binary/src/framework/builder/meta.rs b/rust/operator-binary/src/framework/builder/meta.rs index c41a136..5034004 100644 --- a/rust/operator-binary/src/framework/builder/meta.rs +++ b/rust/operator-binary/src/framework/builder/meta.rs @@ -5,7 +5,8 @@ use stackable_operator::{ 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 + HasName + HasUid), block_owner_deletion: Option, diff --git a/rust/operator-binary/src/framework/builder/pdb.rs b/rust/operator-binary/src/framework/builder/pdb.rs index 2fd4303..b45383e 100644 --- a/rust/operator-binary/src/framework/builder/pdb.rs +++ b/rust/operator-binary/src/framework/builder/pdb.rs @@ -9,7 +9,7 @@ use crate::framework::{ }; /// Infallible variant of -/// `stackable_operator::builder::pdb::PodDisruptionBudgetBuilder::new_with_role` +/// [`stackable_operator::builder::pdb::PodDisruptionBudgetBuilder::new_with_role`] pub fn pod_disruption_budget_builder_with_role( owner: &(impl Resource + HasName + NameIsValidLabelValue + HasUid), product_name: &ProductName, diff --git a/rust/operator-binary/src/framework/builder/pod/container.rs b/rust/operator-binary/src/framework/builder/pod/container.rs index b05d0e4..589922f 100644 --- a/rust/operator-binary/src/framework/builder/pod/container.rs +++ b/rust/operator-binary/src/framework/builder/pod/container.rs @@ -22,7 +22,7 @@ pub enum Error { pub struct EnvVarName(String); impl EnvVarName { - /// Creates an `EnvVarName` from the given string and panics if the validation failed + /// 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 { @@ -52,35 +52,35 @@ impl FromStr for EnvVarName { } } -/// A set of `EnvVar`s +/// 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` + /// Creates an empty [`EnvVarSet`] pub fn new() -> Self { Self::default() } - /// Returns a reference to the `EnvVar` with the given name + /// 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. + /// Moves all [`EnvVar`]s from the given set into this one. /// - /// `EnvVar`s with the same name are overridden. + /// [`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 + /// Adds the given [`EnvVar`]s to this set /// - /// `EnvVar`s with the same name are overridden. + /// [`EnvVar`]s with the same name are overridden. pub fn with_values(self, env_vars: I) -> Self where I: IntoIterator, @@ -96,7 +96,7 @@ impl EnvVarSet { /// Adds an environment variable with the given name and string value to this set /// - /// An `EnvVar` with the same name is overridden. + /// 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(); @@ -114,7 +114,7 @@ impl EnvVarSet { /// Adds an environment variable with the given name and field path to this set /// - /// An `EnvVar` with the same name is overridden. + /// 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 index 1e3ebaa..24f20fe 100644 --- a/rust/operator-binary/src/framework/builder/pod/volume.rs +++ b/rust/operator-binary/src/framework/builder/pod/volume.rs @@ -5,7 +5,7 @@ use stackable_operator::{ use crate::framework::{ListenerClassName, ListenerName, PersistentVolumeClaimName}; -/// Infallible variant of `stackable_operator::builder::pod::volume::ListenerReference` +/// Infallible variant of [`stackable_operator::builder::pod::volume::ListenerReference`] #[derive(Clone, Debug, Eq, PartialEq)] pub enum ListenerReference { ListenerClass(ListenerClassName), @@ -29,7 +29,8 @@ impl From<&ListenerReference> for stackable_operator::builder::pod::volume::List } } -/// Infallible variant of `stackable_operator::builder::pod::volume::ListenerOperatorVolumeSourceBuilder::build_pvc` +/// 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, diff --git a/rust/operator-binary/src/framework/cluster_resources.rs b/rust/operator-binary/src/framework/cluster_resources.rs index 0e3819e..b1123b7 100644 --- a/rust/operator-binary/src/framework/cluster_resources.rs +++ b/rust/operator-binary/src/framework/cluster_resources.rs @@ -6,7 +6,7 @@ use stackable_operator::{ 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` +/// Infallible variant of [`stackable_operator::cluster_resources::ClusterResources::new`] pub fn cluster_resources_new( product_name: &ProductName, operator_name: &OperatorName, @@ -19,7 +19,7 @@ pub fn cluster_resources_new( // 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 - // `MAX_LABEL_VALUE_LENGTH`. + // MAX_LABEL_VALUE_LENGTH. const _: () = assert!( ProductName::MAX_LENGTH + "-operator".len() <= MAX_LABEL_VALUE_LENGTH, "The string `-operator` must not exceed the limit of Label names." diff --git a/rust/operator-binary/src/framework/kvp/label.rs b/rust/operator-binary/src/framework/kvp/label.rs index 681aa83..7f87e0f 100644 --- a/rust/operator-binary/src/framework/kvp/label.rs +++ b/rust/operator-binary/src/framework/kvp/label.rs @@ -8,7 +8,7 @@ use crate::framework::{ RoleGroupName, RoleName, }; -/// Infallible variant of `stackable_operator::kvp::label::Labels::recommended` +/// Infallible variant of [`stackable_operator::kvp::Labels::recommended`] pub fn recommended_labels( owner: &(impl Resource + HasName + NameIsValidLabelValue), product_name: &ProductName, @@ -33,7 +33,7 @@ pub fn recommended_labels( ) } -/// Infallible variant of `stackable_operator::kvp::label::Labels::role_selector` +/// Infallible variant of [`stackable_operator::kvp::Labels::role_selector`] pub fn role_selector( owner: &(impl Resource + HasName + NameIsValidLabelValue), product_name: &ProductName, @@ -47,7 +47,7 @@ pub fn role_selector( .expect("Labels should be created because all given parameters produce valid label values") } -/// Infallible variant of `stackable_operator::kvp::label::Labels::role_group_selector` +/// Infallible variant of [`stackable_operator::kvp::Labels::role_group_selector`] pub fn role_group_selector( owner: &(impl Resource + HasName + NameIsValidLabelValue), product_name: &ProductName, diff --git a/rust/operator-binary/src/framework/role_group_utils.rs b/rust/operator-binary/src/framework/role_group_utils.rs index f887ba7..5599053 100644 --- a/rust/operator-binary/src/framework/role_group_utils.rs +++ b/rust/operator-binary/src/framework/role_group_utils.rs @@ -21,8 +21,11 @@ impl ResourceNames { /// 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. + /// for a [`StatefulSet`]. + /// + /// [`StatefulSet`]: stackable_operator::k8s_openapi::api::apps::v1::StatefulSet fn qualified_role_group_name(&self) -> String { format!( "{}-{}-{}", diff --git a/rust/operator-binary/src/framework/role_utils.rs b/rust/operator-binary/src/framework/role_utils.rs index a8fa419..b5476d7 100644 --- a/rust/operator-binary/src/framework/role_utils.rs +++ b/rust/operator-binary/src/framework/role_utils.rs @@ -20,8 +20,8 @@ use super::{ }; use crate::framework::{ClusterName, ClusterRoleName}; -/// Variant of `stackable_operator::role_utils::GenericProductSpecificCommonConfig` that implements -/// `Merge` +/// Variant of [`stackable_operator::role_utils::GenericProductSpecificCommonConfig`] that +/// implements [`Merge`] #[derive(Clone, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)] pub struct GenericProductSpecificCommonConfig {} @@ -29,12 +29,12 @@ impl Merge for GenericProductSpecificCommonConfig { fn merge(&mut self, _defaults: &Self) {} } -/// Variant of `stackable_operator::role_utils::RoleGroup` that is easier to work with +/// 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`. +/// * The [`HashMap`] in `env_overrides` is replaced with an [`EnvVarSet`]. #[derive(Clone, Debug, PartialEq)] pub struct RoleGroupConfig { pub replicas: u16, @@ -58,7 +58,7 @@ impl RoleGroupConfig( @@ -79,7 +79,7 @@ where fragment::validate(rolegroup_config) } -/// Merges and validates the `RoleGroup` with the given `role` and `default_config` +/// Merges and validates the [`RoleGroup`] with the given `role` and `default_config` pub fn with_validated_config( role_group: &RoleGroup, role: &Role, From eb14740ab5a3d5a2f41f6f740f50e12117b06440 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Tue, 16 Sep 2025 15:03:44 +0200 Subject: [PATCH 5/7] chore: Regenerate charts --- deploy/helm/opensearch-operator/crds/crds.yaml | 1 + rust/operator-binary/src/framework/builder/pod/volume.rs | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) 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/src/framework/builder/pod/volume.rs b/rust/operator-binary/src/framework/builder/pod/volume.rs index 24f20fe..0d0b912 100644 --- a/rust/operator-binary/src/framework/builder/pod/volume.rs +++ b/rust/operator-binary/src/framework/builder/pod/volume.rs @@ -41,7 +41,7 @@ pub fn listener_operator_volume_source_builder_build_pvc( .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", + listener_reference is a valid annotation value and there are no restrictions on single \ + annotation values", ) } From 44830bb6991193efe7b993660ae500421f8efa9f Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Tue, 16 Sep 2025 18:17:38 +0200 Subject: [PATCH 6/7] fix: Require an RFC 1035 label name for Services --- rust/operator-binary/src/framework.rs | 48 +++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/rust/operator-binary/src/framework.rs b/rust/operator-binary/src/framework.rs index bebe388..3ab0f44 100644 --- a/rust/operator-binary/src/framework.rs +++ b/rust/operator-binary/src/framework.rs @@ -47,6 +47,11 @@ pub enum Error { 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("not a valid DNS subdomain name as defined in RFC 1123"))] InvalidRfc1123DnsSubdomainName { source: stackable_operator::validation::Errors, @@ -72,6 +77,11 @@ pub const MAX_RFC_1123_DNS_SUBDOMAIN_NAME_LENGTH: usize = 253; /// 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`] @@ -174,6 +184,9 @@ macro_rules! attributed_string_type { (@from_str $name:ident, $s:expr, is_rfc_1123_label_name) => { stackable_operator::validation::is_rfc_1123_label($s).context(InvalidRfc1123LabelNameSnafu)?; }; + (@from_str $name:ident, $s:expr, is_rfc_1035_label_name) => { + stackable_operator::validation::is_rfc_1035_label($s).context(InvalidRfc1035LabelNameSnafu)?; + }; (@from_str $name:ident, $s:expr, is_valid_label_value) => { LabelValue::from_str($s).context(InvalidLabelValueSnafu)?; }; @@ -190,6 +203,8 @@ macro_rules! attributed_string_type { }; (@trait_impl $name:ident, is_rfc_1123_label_name) => { }; + (@trait_impl $name:ident, is_rfc_1035_label_name) => { + }; (@trait_impl $name:ident, is_uid) => { impl From for $name { fn from(value: Uuid) -> Self { @@ -298,8 +313,8 @@ attributed_string_type! { ServiceName, "The name of a Service", "opensearch-nodes-default-headless", - (max_length = min(MAX_RFC_1123_LABEL_NAME_LENGTH, MAX_LABEL_VALUE_LENGTH)), - is_rfc_1123_label_name, + (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! { @@ -398,10 +413,11 @@ mod tests { use super::{ ClusterName, ClusterRoleName, ConfigMapName, ControllerName, EmptyStringSnafu, Error, - ErrorDiscriminants, InvalidLabelValueSnafu, InvalidRfc1123DnsSubdomainNameSnafu, - InvalidRfc1123LabelNameSnafu, InvalidUidSnafu, LabelValue, LengthExceededSnafu, - NamespaceName, OperatorName, PersistentVolumeClaimName, ProductVersion, RoleBindingName, - RoleGroupName, RoleName, ServiceAccountName, ServiceName, StatefulSetName, Uid, VolumeName, + ErrorDiscriminants, InvalidLabelValueSnafu, InvalidRfc1035LabelNameSnafu, + InvalidRfc1123DnsSubdomainNameSnafu, InvalidRfc1123LabelNameSnafu, InvalidUidSnafu, + LabelValue, LengthExceededSnafu, NamespaceName, OperatorName, PersistentVolumeClaimName, + ProductVersion, RoleBindingName, RoleGroupName, RoleName, ServiceAccountName, ServiceName, + StatefulSetName, Uid, VolumeName, }; use crate::framework::{NameIsValidLabelValue, ProductName}; @@ -478,6 +494,24 @@ mod tests { ); } + attributed_string_type! { + IsRfc1035LabelNameTest, + "is_rfc_1035_label_name test", + "a-b", + is_rfc_1035_label_name + } + + #[test] + fn test_attributed_string_type_is_rfc_1035_label_name() { + type T = IsRfc1035LabelNameTest; + + T::test_example(); + assert_eq!( + Err(ErrorDiscriminants::InvalidRfc1035LabelName), + T::from_str("A").map_err(ErrorDiscriminants::from) + ); + } + attributed_string_type! { IsRfc1123DnsSubdomainNameTest, "is_rfc_1123_dns_subdomain_name test", @@ -499,7 +533,7 @@ mod tests { attributed_string_type! { IsRfc1123LabelNameTest, "is_rfc_1123_label_name test", - "a-b", + "1-a", is_rfc_1123_label_name } From 22c5b98558487c85f7b4b08aae56662e66478ab8 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Wed, 17 Sep 2025 17:36:51 +0200 Subject: [PATCH 7/7] feat: Extend the compile-time checks for name types --- rust/operator-binary/src/framework.rs | 108 ++++++++++++------ .../src/framework/cluster_resources.rs | 2 +- .../src/framework/role_group_utils.rs | 107 ++++++++++------- .../src/framework/role_utils.rs | 13 ++- 4 files changed, 148 insertions(+), 82 deletions(-) diff --git a/rust/operator-binary/src/framework.rs b/rust/operator-binary/src/framework.rs index 3ab0f44..7ed6206 100644 --- a/rust/operator-binary/src/framework.rs +++ b/rust/operator-binary/src/framework.rs @@ -20,12 +20,8 @@ //! 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 snafu::{ResultExt, Snafu, ensure}; -use stackable_operator::kvp::LabelValue; +use snafu::Snafu; use strum::{EnumDiscriminants, IntoStaticStr}; -use uuid::Uuid; pub mod builder; pub mod cluster_resources; @@ -107,13 +103,16 @@ pub trait NameIsValidLabelValue { } /// 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) } @@ -137,14 +136,17 @@ macro_rules! attributed_string_type { } } - impl FromStr for $name { - type Err = Error; + 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; - ensure!( + snafu::ensure!( !s.is_empty(), - EmptyStringSnafu {} + $crate::framework::EmptyStringSnafu {} ); $(attributed_string_type!(@from_str $name, s, $attribute);)* @@ -157,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. @@ -170,28 +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_rfc_1123_dns_subdomain_name) => { - stackable_operator::validation::is_rfc_1123_subdomain($s).context(InvalidRfc1123DnsSubdomainNameSnafu)?; + 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(InvalidRfc1123LabelNameSnafu)?; + 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(InvalidRfc1035LabelNameSnafu)?; + 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::try_parse($s).context(InvalidUidSnafu)?; + uuid::Uuid::try_parse($s).context($crate::framework::InvalidUidSnafu)?; }; (@trait_impl $name:ident, (max_length = $max_length:expr)) => { impl $name { @@ -199,27 +201,43 @@ macro_rules! attributed_string_type { pub const MAX_LENGTH: usize = $max_length; } }; + (@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) => { - }; - (@trait_impl $name:ident, is_rfc_1035_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) -> Self { + impl From for $name { + fn from(value: uuid::Uuid) -> Self { Self(value.to_string()) } } - impl From<&Uuid> for $name { - fn from(value: &Uuid) -> Self { + 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 NameIsValidLabelValue 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() } @@ -321,7 +339,12 @@ attributed_string_type! { StatefulSetName, "The name of a StatefulSet", "opensearch-nodes-default", - (max_length = min(MAX_RFC_1123_LABEL_NAME_LENGTH, MAX_LABEL_VALUE_LENGTH)), + (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 } @@ -351,6 +374,7 @@ attributed_string_type! { // A suffix is added to produce a label value. An according compile-time check ensures that // max_length cannot be set higher. (max_length = min(54, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1123_dns_subdomain_name, is_valid_label_value } attributed_string_type! { @@ -364,9 +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 = min(24, MAX_LABEL_VALUE_LENGTH)), + is_rfc_1035_label_name, is_valid_label_value } attributed_string_type! { @@ -391,6 +416,7 @@ attributed_string_type! { // 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! { @@ -401,23 +427,20 @@ attributed_string_type! { // 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 } #[cfg(test)] mod tests { - use std::{fmt::Display, str::FromStr}; + use std::str::FromStr; - use snafu::{ResultExt, ensure}; - use uuid::{Uuid, uuid}; + use uuid::uuid; use super::{ - ClusterName, ClusterRoleName, ConfigMapName, ControllerName, EmptyStringSnafu, Error, - ErrorDiscriminants, InvalidLabelValueSnafu, InvalidRfc1035LabelNameSnafu, - InvalidRfc1123DnsSubdomainNameSnafu, InvalidRfc1123LabelNameSnafu, InvalidUidSnafu, - LabelValue, LengthExceededSnafu, NamespaceName, OperatorName, PersistentVolumeClaimName, - ProductVersion, RoleBindingName, RoleGroupName, RoleName, ServiceAccountName, ServiceName, - StatefulSetName, Uid, VolumeName, + ClusterName, ClusterRoleName, ConfigMapName, ControllerName, ErrorDiscriminants, + NamespaceName, OperatorName, PersistentVolumeClaimName, ProductVersion, RoleBindingName, + RoleGroupName, RoleName, ServiceAccountName, ServiceName, StatefulSetName, Uid, VolumeName, }; use crate::framework::{NameIsValidLabelValue, ProductName}; @@ -505,6 +528,10 @@ mod tests { 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!( Err(ErrorDiscriminants::InvalidRfc1035LabelName), @@ -523,6 +550,8 @@ mod tests { fn test_attributed_string_type_is_rfc_1123_dns_subdomain_name() { type T = IsRfc1123DnsSubdomainNameTest; + let _ = T::IS_RFC_1123_SUBDOMAIN_NAME; + T::test_example(); assert_eq!( Err(ErrorDiscriminants::InvalidRfc1123DnsSubdomainName), @@ -541,6 +570,9 @@ mod tests { 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!( Err(ErrorDiscriminants::InvalidRfc1123LabelName), @@ -559,6 +591,8 @@ mod tests { fn test_attributed_string_type_is_valid_label_value() { type T = IsValidLabelValueTest; + let _ = T::IS_VALID_LABEL_VALUE; + T::test_example(); assert_eq!( Err(ErrorDiscriminants::InvalidLabelValue), diff --git a/rust/operator-binary/src/framework/cluster_resources.rs b/rust/operator-binary/src/framework/cluster_resources.rs index b1123b7..b218e53 100644 --- a/rust/operator-binary/src/framework/cluster_resources.rs +++ b/rust/operator-binary/src/framework/cluster_resources.rs @@ -16,7 +16,7 @@ pub fn cluster_resources_new( cluster_uid: &Uid, apply_strategy: ClusterResourceApplyStrategy, ) -> ClusterResources { - // Compile-time check + // 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 // MAX_LABEL_VALUE_LENGTH. diff --git a/rust/operator-binary/src/framework/role_group_utils.rs b/rust/operator-binary/src/framework/role_group_utils.rs index 5599053..c2881d5 100644 --- a/rust/operator-binary/src/framework/role_group_utils.rs +++ b/rust/operator-binary/src/framework/role_group_utils.rs @@ -1,7 +1,23 @@ use std::str::FromStr; -use super::{ClusterName, ConfigMapName, ListenerName, RoleGroupName, RoleName, StatefulSetName}; -use crate::framework::ServiceName; +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 { @@ -11,80 +27,88 @@ 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`]. - /// - /// [`StatefulSet`]: stackable_operator::k8s_openapi::api::apps::v1::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, self.role_name, self.role_group_name, - ) + )) + .expect("should be a valid QualifiedRoleGroupName") } pub fn role_group_config_map(&self) -> ConfigMapName { - // Compile-time check + // compile-time check const _: () = assert!( - ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH <= ConfigMapName::MAX_LENGTH, + QualifiedRoleGroupName::MAX_LENGTH <= ConfigMapName::MAX_LENGTH, "The string `--` must not exceed the limit of \ ConfigMap names." ); + let _ = QualifiedRoleGroupName::IS_RFC_1123_SUBDOMAIN_NAME; - ConfigMapName::from_str(&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) -> StatefulSetName { - // Compile-time check + // 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 - <= StatefulSetName::MAX_LENGTH, - "The string `---` \ - must not exceed the limit of StatefulSet names." + 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; - StatefulSetName::from_str(&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) -> ServiceName { const SUFFIX: &str = "-headless"; - // Compile-time check + // compile-time checks const _: () = assert!( - ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH + SUFFIX.len() - <= ServiceName::MAX_LENGTH, + 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; ServiceName::from_str(&format!("{}{SUFFIX}", self.qualified_role_group_name())) .expect("should be a valid Service name") } pub fn listener_name(&self) -> ListenerName { - // Compile-time check + // compile-time checks const _: () = assert!( - ResourceNames::MAX_QUALIFIED_ROLE_GROUP_NAME_LENGTH <= ListenerName::MAX_LENGTH, + QualifiedRoleGroupName::MAX_LENGTH <= ListenerName::MAX_LENGTH, "The string `--` must not exceed the limit of \ Listener names." ); + let _ = QualifiedRoleGroupName::IS_RFC_1123_SUBDOMAIN_NAME; - ListenerName::from_str(&self.qualified_role_group_name()) + ListenerName::from_str(self.qualified_role_group_name().as_ref()) .expect("should be a valid Listener name") } } @@ -93,11 +117,14 @@ impl ResourceNames { mod tests { use super::{ClusterName, RoleGroupName, RoleName}; use crate::framework::{ - ConfigMapName, ListenerName, ServiceName, StatefulSetName, role_group_utils::ResourceNames, + 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"), @@ -105,7 +132,7 @@ 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!( diff --git a/rust/operator-binary/src/framework/role_utils.rs b/rust/operator-binary/src/framework/role_utils.rs index b5476d7..cfda143 100644 --- a/rust/operator-binary/src/framework/role_utils.rs +++ b/rust/operator-binary/src/framework/role_utils.rs @@ -180,11 +180,12 @@ impl ResourceNames { pub fn service_account_name(&self) -> ServiceAccountName { const SUFFIX: &str = "-serviceaccount"; - // Compile-time check + // compile-time checks const _: () = assert!( 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; ServiceAccountName::from_str(&format!("{}{SUFFIX}", self.cluster_name)) .expect("should be a valid ServiceAccount name") @@ -193,11 +194,12 @@ impl ResourceNames { pub fn role_binding_name(&self) -> RoleBindingName { const SUFFIX: &str = "-rolebinding"; - // Compile-time check + // 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; RoleBindingName::from_str(&format!("{}{SUFFIX}", self.cluster_name)) .expect("should be a valid RoleBinding name") @@ -206,22 +208,25 @@ impl ResourceNames { pub fn cluster_role_name(&self) -> ClusterRoleName { const SUFFIX: &str = "-clusterrole"; - // Compile-time check + // 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; ClusterRoleName::from_str(&format!("{}{SUFFIX}", self.product_name)) .expect("should be a valid cluster role name") } pub fn discovery_service_name(&self) -> ServiceName { - // Compile-time check + // compile-time checks const _: () = assert!( 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; ServiceName::from_str(self.cluster_name.as_ref()).expect("should be a valid Service name") }