feat: Add key/value pair validation (#684)

* Initial commit

* Fix failing doc tests

* Start to add more doc comments

* Finish doc comments

* Add KeyValuePairs, add TryFrom<(&str, &str)> impl

* Add BTreeMap helper functions

* Add FromIterator impl

* Start to add more test cases

* Add invalid kvp tests

* Add key, key prefix, and key name tests

* Add value tests

* Rename max length constants

* Add associated function `push`

* Rename unit tests

* Introduce generic type parameter to allow different value impls

* Fix annotation macro, add unit test

* Rename generic type param, fix rustdoc error

* Add doc comments to macros

* Update changelog

* Add macros and full feature

* Start to change KeyValuePairs from Vec to HashSet

* Implement serde serialize

* Implement serde deserialize

* Use BTreeSet instead of HashSet for deterministic ordering

* Add common func, add serde tests, fix deserialization

* Introduce newtype Label and Annotation structs

* Add recommended label function

* Start to adjust other label handling code (wip)

* Continue label/annotation handling code

* Add final fixes without breaking changes

* Add label and annotation error type aliases

* Remove unwraps in secret volume builder

* Remove unwraps from listener volume builder

* Remove unwraps in object meta builder

* Remove unwraps in PDB builder

* Rename trait ValueExt to Value

* Change deref target

* Simplify key parsing

* Remove unneeded annotation validation

* Remove FromStr impl for KeyValuePair, Label, and Annotation

* Remove unwraps from ClusterResources

* Add annotation doc comments

* Add label doc comments

* Add module doc comment, remove outdated struct comment

* Update const names, add doc comments

* Remove label and annotation macro

* Add const module doc comment

* Update and move label selector to query string functionality

* Fix rustdoc error

* Move workspace member back

* Add doc comments to ObjectLabels struct

* Add missing doc comments

* Remove Deserialize and Serialize support

* Rework contains and add contains_key function

CHANGELOG.md

@@ -4,6 +4,19 @@ All notable changes to this project will be documented in this file.
 
 ## [Unreleased]
 
+### Added
+
+- Add `stackble_operator::kvp` module and types to allow validated construction of key/value pairs, like labels and
+  annotations. Most users want to use the exported type aliases `Label` and `Annotation` ([#684]).
+
+### Changed
+
+- Move `stackable_operator::label_selector::convert_label_selector_to_query_string` into `kvp` module. The conversion
+  functionality now is encapsulated in a new trait `LabelSelectorExt`. An instance of a `LabelSelector` can now be
+  converted into a query string by calling the associated function `ls.to_query_string()` ([#684]).
+
+[#684]: https://github.com/stackabletech/operator-rs/pull/684
+
 ## [0.58.1] - 2023-12-12
 
 ### Added

src/builder/meta.rs

@@ -1,10 +1,20 @@
-use crate::error::{Error, OperatorResult};
-use crate::labels::{self, ObjectLabels};
 use k8s_openapi::apimachinery::pkg::apis::meta::v1::{ObjectMeta, OwnerReference};
 use kube::{Resource, ResourceExt};
-use std::collections::BTreeMap;
+use snafu::{ResultExt, Snafu};
 use tracing::warn;
 
+use crate::{
+    error::{Error, OperatorResult},
+    kvp::{Annotation, Annotations, Label, LabelError, Labels, ObjectLabels},
+};
+
+// NOTE (Techassi): Think about that name
+#[derive(Debug, Snafu)]
+pub enum ObjectMetaBuilderError {
+    #[snafu(display("failed to set recommended labels"))]
+    RecommendedLabels { source: LabelError },
+}
+
 /// A builder to build [`ObjectMeta`] objects.
 ///
 /// Of special interest is the [`Self::ownerreference_from_resource()`] function.
@@ -13,12 +23,12 @@ use tracing::warn;
 /// It is strongly recommended to always call [`Self::with_recommended_labels()`]!
 #[derive(Clone, Default)]
 pub struct ObjectMetaBuilder {
-    name: Option<String>,
+    ownerreference: Option<OwnerReference>,
+    annotations: Option<Annotations>,
     generate_name: Option<String>,
     namespace: Option<String>,
-    ownerreference: Option<OwnerReference>,
-    labels: Option<BTreeMap<String, String>>,
-    annotations: Option<BTreeMap<String, String>>,
+    labels: Option<Labels>,
+    name: Option<String>,
 }
 
 impl ObjectMetaBuilder {
@@ -92,74 +102,70 @@ impl ObjectMetaBuilder {
 
     /// This adds a single annotation to the existing annotations.
     /// It'll override an annotation with the same key.
-    pub fn with_annotation(
-        &mut self,
-        annotation_key: impl Into<String>,
-        annotation_value: impl Into<String>,
-    ) -> &mut Self {
+    pub fn with_annotation(&mut self, annotation: Annotation) -> &mut Self {
         self.annotations
-            .get_or_insert_with(BTreeMap::new)
-            .insert(annotation_key.into(), annotation_value.into());
+            .get_or_insert(Annotations::new())
+            .insert(annotation);
         self
     }
 
     /// This adds multiple annotations to the existing annotations.
     /// Any existing annotation with a key that is contained in `annotations` will be overwritten
-    pub fn with_annotations(&mut self, annotations: BTreeMap<String, String>) -> &mut Self {
+    pub fn with_annotations(&mut self, annotations: Annotations) -> &mut Self {
         self.annotations
-            .get_or_insert_with(BTreeMap::new)
+            .get_or_insert(Annotations::new())
             .extend(annotations);
         self
     }
 
     /// This will replace all existing annotations
-    pub fn annotations(&mut self, annotations: BTreeMap<String, String>) -> &mut Self {
+    pub fn annotations(&mut self, annotations: Annotations) -> &mut Self {
         self.annotations = Some(annotations);
         self
     }
 
     /// This adds a single label to the existing labels.
     /// It'll override a label with the same key.
-    pub fn with_label(
-        &mut self,
-        label_key: impl Into<String>,
-        label_value: impl Into<String>,
-    ) -> &mut Self {
-        self.labels
-            .get_or_insert_with(BTreeMap::new)
-            .insert(label_key.into(), label_value.into());
+    pub fn with_label(&mut self, label: Label) -> &mut Self {
+        self.labels.get_or_insert(Labels::new()).insert(label);
         self
     }
 
     /// This adds multiple labels to the existing labels.
     /// Any existing label with a key that is contained in `labels` will be overwritten
-    pub fn with_labels(&mut self, labels: BTreeMap<String, String>) -> &mut Self {
-        self.labels.get_or_insert_with(BTreeMap::new).extend(labels);
+    pub fn with_labels(&mut self, labels: Labels) -> &mut Self {
+        self.labels.get_or_insert(Labels::new()).extend(labels);
         self
     }
 
     /// This will replace all existing labels
-    pub fn labels(&mut self, labels: BTreeMap<String, String>) -> &mut Self {
+    pub fn labels(&mut self, labels: Labels) -> &mut Self {
         self.labels = Some(labels);
         self
     }
 
-    /// This sets the common recommended labels (in the `app.kubernetes.io` namespace).
-    /// It is recommended to always call this method.
-    /// The only reasons it is not _required_ is to make testing easier and to allow for more
-    /// flexibility if needed.
+    /// This sets the common recommended labels (in the `app.kubernetes.io`
+    /// namespace). It is recommended to always call this method. The only
+    /// reasons it is not _required_ is to make testing easier and to allow
+    /// for more flexibility if needed.
     pub fn with_recommended_labels<T: Resource>(
         &mut self,
         object_labels: ObjectLabels<T>,
-    ) -> &mut Self {
-        let recommended_labels = labels::get_recommended_labels(object_labels);
+    ) -> Result<&mut Self, ObjectMetaBuilderError> {
+        let recommended_labels =
+            Labels::recommended(object_labels).context(RecommendedLabelsSnafu)?;
+
         self.labels
-            .get_or_insert_with(BTreeMap::new)
+            .get_or_insert(Labels::new())
             .extend(recommended_labels);
-        self
+
+        Ok(self)
     }
 
     pub fn build(&self) -> ObjectMeta {
+        // NOTE (Techassi): Shouldn't this take self instead of &self to consume
+        // the builder and build ObjectMeta without cloning?
+
         // if 'generate_name' and 'name' are set, Kubernetes will prioritize the 'name' field and
         // 'generate_name' has no impact.
         if let (Some(name), Some(generate_name)) = (&self.name, &self.generate_name) {
@@ -178,8 +184,8 @@ impl ObjectMetaBuilder {
                 .ownerreference
                 .as_ref()
                 .map(|ownerreference| vec![ownerreference.clone()]),
-            labels: self.labels.clone(),
-            annotations: self.annotations.clone(),
+            labels: self.labels.clone().map(|l| l.into()),
+            annotations: self.annotations.clone().map(|a| a.into()),
             ..ObjectMeta::default()
         }
     }
@@ -329,7 +335,8 @@ mod tests {
                 role: "role",
                 role_group: "rolegroup",
             })
-            .with_annotation("foo", "bar")
+            .unwrap()
+            .with_annotation(("foo", "bar").try_into().unwrap())
             .build();
 
         assert_eq!(meta.generate_name, Some("generate_foo".to_string()));

src/builder/pdb.rs

@@ -1,9 +1,3 @@
-use crate::{
-    builder::ObjectMetaBuilder,
-    error::OperatorResult,
-    labels::{role_selector_labels, APP_MANAGED_BY_LABEL},
-    utils::format_full_controller_name,
-};
 use k8s_openapi::{
     api::policy::v1::{PodDisruptionBudget, PodDisruptionBudgetSpec},
     apimachinery::pkg::{
@@ -13,6 +7,12 @@ use k8s_openapi::{
 };
 use kube::{Resource, ResourceExt};
 
+use crate::{
+    builder::ObjectMetaBuilder,
+    error::OperatorResult,
+    kvp::{Label, Labels},
+};
+
 /// This builder is used to construct [`PodDisruptionBudget`]s.
 /// If you are using this to create [`PodDisruptionBudget`]s according to [ADR 30 on Allowed Pod disruptions][adr],
 /// the use of [`PodDisruptionBudgetBuilder::new_with_role`] is recommended.
@@ -48,39 +48,43 @@ impl PodDisruptionBudgetBuilder<(), (), ()> {
         PodDisruptionBudgetBuilder::default()
     }
 
-    /// This method populates [`PodDisruptionBudget::metadata`] and [`PodDisruptionBudgetSpec::selector`] from the give role
-    /// (not roleGroup!).
+    /// This method populates [`PodDisruptionBudget::metadata`] and
+    /// [`PodDisruptionBudgetSpec::selector`] from the give role (not roleGroup!).
     ///
-    /// The parameters are the same as the fields from [`crate::labels::ObjectLabels`]:
-    /// * `owner` - Reference to the k8s object owning the created resource, such as `HdfsCluster` or `TrinoCluster`.
-    /// * `app_name` - The name of the app being managed, such as `hdfs` or `trino`.
-    /// * `role` - The role that this object belongs to, e.g. `datanode` or `worker`.
-    /// * `operator_name` - The DNS-style name of the operator managing the object (such as `hdfs.stackable.tech`).
-    /// * `controller_name` - The name of the controller inside of the operator managing the object (such as `hdfscluster`)
+    /// The parameters are the same as the fields from
+    /// [`ObjectLabels`][crate::kvp::ObjectLabels]:
+    ///
+    /// * `owner` - Reference to the k8s object owning the created resource,
+    ///   such as `HdfsCluster` or `TrinoCluster`.
+    /// * `app_name` - The name of the app being managed, such as `hdfs` or
+    ///   `trino`.
+    /// * `role` - The role that this object belongs to, e.g. `datanode` or
+    ///   `worker`.
+    /// * `operator_name` - The DNS-style name of the operator managing the
+    ///   object (such as `hdfs.stackable.tech`).
+    /// * `controller_name` - The name of the controller inside of the operator
+    ///   managing the object (such as `hdfscluster`)
     pub fn new_with_role<T: Resource<DynamicType = ()>>(
         owner: &T,
         app_name: &str,
         role: &str,
         operator_name: &str,
         controller_name: &str,
     ) -> OperatorResult<PodDisruptionBudgetBuilder<ObjectMeta, LabelSelector, ()>> {
-        let role_selector_labels = role_selector_labels(owner, app_name, role);
+        let role_selector_labels = Labels::role_selector(owner, app_name, role)?;
         let metadata = ObjectMetaBuilder::new()
             .namespace_opt(owner.namespace())
             .name(format!("{}-{}", owner.name_any(), role))
             .ownerreference_from_resource(owner, None, Some(true))?
             .with_labels(role_selector_labels.clone())
-            .with_label(
-                APP_MANAGED_BY_LABEL.to_string(),
-                format_full_controller_name(operator_name, controller_name),
-            )
+            .with_label(Label::managed_by(operator_name, controller_name)?)
             .build();
 
         Ok(PodDisruptionBudgetBuilder {
             metadata,
             selector: LabelSelector {
                 match_expressions: None,
-                match_labels: Some(role_selector_labels),
+                match_labels: Some(role_selector_labels.into()),
             },
             ..PodDisruptionBudgetBuilder::default()
         })

src/builder/pod/mod.rs

@@ -1,15 +1,20 @@
-pub mod container;
-pub mod resources;
-pub mod security;
-pub mod volume;
+use std::{collections::BTreeMap, num::TryFromIntError};
 
-use std::collections::BTreeMap;
-use std::num::TryFromIntError;
+use k8s_openapi::{
+    api::core::v1::{
+        Affinity, Container, LocalObjectReference, NodeAffinity, Pod, PodAffinity, PodAntiAffinity,
+        PodCondition, PodSecurityContext, PodSpec, PodStatus, PodTemplateSpec,
+        ResourceRequirements, Toleration, Volume,
+    },
+    apimachinery::pkg::{api::resource::Quantity, apis::meta::v1::ObjectMeta},
+};
+use snafu::{ResultExt, Snafu};
+use tracing::warn;
 
 use crate::{
     builder::{
-        meta::ObjectMetaBuilder, ListenerOperatorVolumeSourceBuilder, ListenerReference,
-        VolumeBuilder,
+        meta::ObjectMetaBuilder, ListenerOperatorVolumeSourceBuilder,
+        ListenerOperatorVolumeSourceBuilderError, ListenerReference, VolumeBuilder,
     },
     commons::{
         affinity::StackableAffinity,
@@ -23,23 +28,24 @@ use crate::{
     time::Duration,
 };
 
-use k8s_openapi::{
-    api::core::v1::{
-        Affinity, Container, LocalObjectReference, NodeAffinity, Pod, PodAffinity, PodAntiAffinity,
-        PodCondition, PodSecurityContext, PodSpec, PodStatus, PodTemplateSpec,
-        ResourceRequirements, Toleration, Volume,
-    },
-    apimachinery::pkg::{api::resource::Quantity, apis::meta::v1::ObjectMeta},
-};
-use tracing::warn;
+pub mod container;
+pub mod resources;
+pub mod security;
+pub mod volume;
 
-#[derive(Debug, thiserror::Error)]
+#[derive(Debug, Snafu)]
 pub enum Error {
-    #[error("termination grace period is too long (got {duration}, maximum allowed is {max})", max = Duration::from_secs(i64::MAX as u64))]
+    #[snafu(display("termination grace period is too long (got {duration}, maximum allowed is {max})", max = Duration::from_secs(i64::MAX as u64)))]
     TerminationGracePeriodTooLong {
         source: TryFromIntError,
         duration: Duration,
     },
+
+    #[snafu(display("failed to add listener volume '{name}' to the pod"))]
+    ListenerVolume {
+        source: ListenerOperatorVolumeSourceBuilderError,
+        name: String,
+    },
 }
 pub type Result<T> = std::result::Result<T, Error>;
 
@@ -299,6 +305,7 @@ impl PodBuilder {
     ///             .build(),
     ///     )
     ///     .add_listener_volume_by_listener_class("listener", "nodeport")
+    ///     .unwrap()
     ///     .build()
     ///     .unwrap();
     ///
@@ -341,18 +348,19 @@ impl PodBuilder {
         &mut self,
         volume_name: &str,
         listener_class: &str,
-    ) -> &mut Self {
+    ) -> Result<&mut Self> {
+        let listener_reference = ListenerReference::ListenerClass(listener_class.to_string());
+        let volume = ListenerOperatorVolumeSourceBuilder::new(&listener_reference)
+            .build()
+            .context(ListenerVolumeSnafu { name: volume_name })?;
+
         self.add_volume(Volume {
             name: volume_name.into(),
-            ephemeral: Some(
-                ListenerOperatorVolumeSourceBuilder::new(&ListenerReference::ListenerClass(
-                    listener_class.into(),
-                ))
-                .build(),
-            ),
+            ephemeral: Some(volume),
             ..Volume::default()
         });
-        self
+
+        Ok(self)
     }
 
     /// Add a [`Volume`] for the storage class `listeners.stackable.tech` with the given listener
@@ -386,6 +394,7 @@ impl PodBuilder {
     ///             .build(),
     ///     )
     ///     .add_listener_volume_by_listener_name("listener", "preprovisioned-listener")
+    ///     .unwrap()
     ///     .build()
     ///     .unwrap();
     ///
@@ -428,18 +437,19 @@ impl PodBuilder {
         &mut self,
         volume_name: &str,
         listener_name: &str,
-    ) -> &mut Self {
+    ) -> Result<&mut Self> {
+        let listener_reference = ListenerReference::ListenerName(listener_name.to_string());
+        let volume = ListenerOperatorVolumeSourceBuilder::new(&listener_reference)
+            .build()
+            .context(ListenerVolumeSnafu { name: volume_name })?;
+
         self.add_volume(Volume {
             name: volume_name.into(),
-            ephemeral: Some(
-                ListenerOperatorVolumeSourceBuilder::new(&ListenerReference::ListenerName(
-                    listener_name.into(),
-                ))
-                .build(),
-            ),
+            ephemeral: Some(volume),
             ..Volume::default()
         });
-        self
+
+        Ok(self)
     }
 
     pub fn image_pull_secrets(