feat: Add key/value pair validation

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(