Reorganize kvp so that KeyValuePairs is an alias for BTreeMap instead of newtyping it

Cargo.toml

@@ -18,7 +18,6 @@ const_format = "0.2.33"
 const-oid = "0.9.6"
 convert_case = "0.6.0"
 darling = "0.20.10"
-delegate = "0.13.0"
 derivative = "2.2.0"
 dockerfile-parser = "0.8.0"
 ecdsa = { version = "0.16.9", features = ["digest", "pem"] }

crates/stackable-operator/CHANGELOG.md

@@ -13,6 +13,20 @@ All notable changes to this project will be documented in this file.
 
 - BREAKING: The `CustomResourceExt` trait is now re-exported from the `stackable-shared` crate. The
   trait functions use the same parameters but return a different error type ([#883]).
+- BREAKING: `KeyValuePairs<V>` (and `Annotations`/`Labels`) is now an alias for `BTreeMap<Key, V>` ([#889]).
+  - Generally, this means that the API now behaves like a map rather than a set. For example, duplicate keys are no longer allowed (as was already documented before).
+  - Some `KeyValuePairs` methods have been renamed for certain use cases:
+    - `KeyValuePairs::insert(&mut self, kvp)`: use `::extend(&mut self, [kvp])` instead.
+    - `KeyValuePairs::try_from`: you may need to use `::try_from_iter` instead.
+    - `KeyValuePairs::contains_key`: unvalidated keys will need to use `::contains_str_key` instead.
+    - `Into<BTreeMap<String, String>>`: use `::to_unvalidated` instead.
+  - Well-known annotations have been moved from `kvp::Annotation` to `kvp::annotation::well_known`.
+  - Well-known labels have been moved from `kvp::Label` to `kvp::label::well_known`.
+  - Well-known label sets have been moved from `kvp::Labels` to `kvp::label::well_known::sets`.
+
+### Fixed
+
+- `KeyValuePairs` will now consistently use the last-written value for a given key ([#889]).
 
 ### Removed
 
@@ -22,6 +36,7 @@ All notable changes to this project will be documented in this file.
 
 [#883]: https://github.com/stackabletech/operator-rs/pull/883
 [#885]: https://github.com/stackabletech/operator-rs/pull/885
+[#889]: https://github.com/stackabletech/operator-rs/pull/889
 
 ## [0.78.0] - 2024-09-30
 

crates/stackable-operator/Cargo.toml

@@ -17,7 +17,6 @@ stackable-shared = { path = "../stackable-shared" }
 chrono.workspace = true
 clap.workspace = true
 const_format.workspace = true
-delegate.workspace = true
 derivative.workspace = true
 dockerfile-parser.workspace = true
 either.workspace = true

crates/stackable-operator/src/builder/meta.rs

@@ -3,7 +3,10 @@ use kube::{Resource, ResourceExt};
 use snafu::{ResultExt, Snafu};
 use tracing::warn;
 
-use crate::kvp::{Annotation, Annotations, Label, LabelError, Labels, ObjectLabels};
+use crate::kvp::{
+    label, Annotation, Annotations, KeyValuePairs, KeyValuePairsExt, Label, LabelError, Labels,
+    ObjectLabels,
+};
 
 type Result<T, E = Error> = std::result::Result<T, E>;
 
@@ -106,7 +109,7 @@ impl ObjectMetaBuilder {
     pub fn with_annotation(&mut self, annotation: Annotation) -> &mut Self {
         self.annotations
             .get_or_insert(Annotations::new())
-            .insert(annotation);
+            .extend([annotation]);
         self
     }
 
@@ -128,7 +131,7 @@ impl ObjectMetaBuilder {
     /// 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: Label) -> &mut Self {
-        self.labels.get_or_insert(Labels::new()).insert(label);
+        self.labels.get_or_insert(Labels::new()).extend([label]);
         self
     }
 
@@ -154,7 +157,7 @@ impl ObjectMetaBuilder {
         object_labels: ObjectLabels<T>,
     ) -> Result<&mut Self> {
         let recommended_labels =
-            Labels::recommended(object_labels).context(RecommendedLabelsSnafu)?;
+            label::well_known::sets::recommended(object_labels).context(RecommendedLabelsSnafu)?;
 
         self.labels
             .get_or_insert(Labels::new())
@@ -185,8 +188,8 @@ impl ObjectMetaBuilder {
                 .ownerreference
                 .as_ref()
                 .map(|ownerreference| vec![ownerreference.clone()]),
-            labels: self.labels.clone().map(|l| l.into()),
-            annotations: self.annotations.clone().map(|a| a.into()),
+            labels: self.labels.as_ref().map(KeyValuePairs::to_unvalidated),
+            annotations: self.annotations.as_ref().map(KeyValuePairs::to_unvalidated),
             ..ObjectMeta::default()
         }
     }

crates/stackable-operator/src/builder/pdb.rs

@@ -10,7 +10,7 @@ use snafu::{ResultExt, Snafu};
 
 use crate::{
     builder::meta::ObjectMetaBuilder,
-    kvp::{Label, Labels},
+    kvp::{label, KeyValuePairsExt},
 };
 
 type Result<T, E = Error> = std::result::Result<T, E>;
@@ -85,10 +85,10 @@ impl PodDisruptionBudgetBuilder<(), (), ()> {
         operator_name: &str,
         controller_name: &str,
     ) -> Result<PodDisruptionBudgetBuilder<ObjectMeta, LabelSelector, ()>> {
-        let role_selector_labels =
-            Labels::role_selector(owner, app_name, role).context(RoleSelectorLabelsSnafu)?;
-        let managed_by_label =
-            Label::managed_by(operator_name, controller_name).context(ManagedByLabelSnafu)?;
+        let role_selector_labels = label::well_known::sets::role_selector(owner, app_name, role)
+            .context(RoleSelectorLabelsSnafu)?;
+        let managed_by_label = label::well_known::managed_by(operator_name, controller_name)
+            .context(ManagedByLabelSnafu)?;
         let metadata = ObjectMetaBuilder::new()
             .namespace_opt(owner.namespace())
             .name(format!("{}-{}", owner.name_any(), role))
@@ -102,7 +102,7 @@ impl PodDisruptionBudgetBuilder<(), (), ()> {
             metadata,
             selector: LabelSelector {
                 match_expressions: None,
-                match_labels: Some(role_selector_labels.into()),
+                match_labels: Some(role_selector_labels.to_unvalidated()),
             },
             ..PodDisruptionBudgetBuilder::default()
         })

crates/stackable-operator/src/builder/pod/mod.rs

@@ -331,16 +331,17 @@ impl PodBuilder {
     /// ```
     /// # use stackable_operator::builder::pod::PodBuilder;
     /// # use stackable_operator::builder::pod::container::ContainerBuilder;
+    /// # use stackable_operator::iter::TryFromIterator;
     /// # use stackable_operator::kvp::Labels;
     /// # use k8s_openapi::{
     /// #     apimachinery::pkg::apis::meta::v1::ObjectMeta,
     /// # };
     /// # use std::collections::BTreeMap;
     ///
-    /// let labels: Labels = Labels::try_from(
-    ///        BTreeMap::from([("app.kubernetes.io/component", "test-role"),
+    /// let labels: Labels = Labels::try_from_iter(
+    ///        [("app.kubernetes.io/component", "test-role"),
     ///             ("app.kubernetes.io/instance", "test"),
-    ///             ("app.kubernetes.io/name", "test")]))
+    ///             ("app.kubernetes.io/name", "test")])
     /// .unwrap();
     ///
     /// let pod = PodBuilder::new()
@@ -418,16 +419,17 @@ impl PodBuilder {
     /// ```
     /// # use stackable_operator::builder::pod::PodBuilder;
     /// # use stackable_operator::builder::pod::container::ContainerBuilder;
+    /// # use stackable_operator::iter::TryFromIterator;
     /// # use stackable_operator::kvp::Labels;
     /// # use k8s_openapi::{
     /// #    apimachinery::pkg::apis::meta::v1::ObjectMeta,
     /// # };
     /// # use std::collections::BTreeMap;
     ///
-    /// let labels: Labels = Labels::try_from(
-    ///        BTreeMap::from([("app.kubernetes.io/component", "test-role"),
+    /// let labels: Labels = Labels::try_from_iter(
+    ///        [("app.kubernetes.io/component", "test-role"),
     ///             ("app.kubernetes.io/instance", "test"),
-    ///             ("app.kubernetes.io/name", "test")]))
+    ///             ("app.kubernetes.io/name", "test")])
     /// .unwrap();
     ///
     /// let pod = PodBuilder::new()

crates/stackable-operator/src/builder/pod/volume.rs

@@ -14,7 +14,7 @@ use tracing::warn;
 
 use crate::{
     builder::meta::ObjectMetaBuilder,
-    kvp::{Annotation, AnnotationError, Annotations, LabelError, Labels},
+    kvp::{annotation, Annotation, AnnotationError, Annotations, LabelError, Labels},
 };
 
 /// A builder to build [`Volume`] objects. May only contain one `volume_source`
@@ -333,34 +333,43 @@ impl SecretOperatorVolumeSourceBuilder {
     pub fn build(&self) -> Result<EphemeralVolumeSource, SecretOperatorVolumeSourceBuilderError> {
         let mut annotations = Annotations::new();
 
-        annotations
-            .insert(Annotation::secret_class(&self.secret_class).context(ParseAnnotationSnafu)?);
+        annotations.extend([annotation::well_known::secret_volume::secret_class(
+            &self.secret_class,
+        )
+        .context(ParseAnnotationSnafu)?]);
 
         if !self.scopes.is_empty() {
-            annotations
-                .insert(Annotation::secret_scope(&self.scopes).context(ParseAnnotationSnafu)?);
+            annotations.extend([
+                annotation::well_known::secret_volume::secret_scope(&self.scopes)
+                    .context(ParseAnnotationSnafu)?,
+            ]);
         }
 
         if let Some(format) = &self.format {
-            annotations
-                .insert(Annotation::secret_format(format.as_ref()).context(ParseAnnotationSnafu)?);
+            annotations.extend([annotation::well_known::secret_volume::secret_format(
+                format.as_ref(),
+            )
+            .context(ParseAnnotationSnafu)?]);
         }
 
         if !self.kerberos_service_names.is_empty() {
-            annotations.insert(
-                Annotation::kerberos_service_names(&self.kerberos_service_names)
-                    .context(ParseAnnotationSnafu)?,
-            );
+            annotations.extend([
+                annotation::well_known::secret_volume::kerberos_service_names(
+                    &self.kerberos_service_names,
+                )
+                .context(ParseAnnotationSnafu)?,
+            ]);
         }
 
         if let Some(password) = &self.tls_pkcs12_password {
             // The `tls_pkcs12_password` is only used for PKCS12 stores.
             if Some(SecretFormat::TlsPkcs12) != self.format {
                 warn!(format.actual = ?self.format, format.expected = ?Some(SecretFormat::TlsPkcs12), "A TLS PKCS12 password was set but ignored because another format was requested")
             } else {
-                annotations.insert(
-                    Annotation::tls_pkcs12_password(password).context(ParseAnnotationSnafu)?,
-                );
+                annotations.extend([annotation::well_known::secret_volume::tls_pkcs12_password(
+                    password,
+                )
+                .context(ParseAnnotationSnafu)?]);
             }
         }
 
@@ -450,7 +459,7 @@ pub enum ListenerOperatorVolumeSourceBuilderError {
 /// # use std::collections::BTreeMap;
 /// let mut pod_builder = PodBuilder::new();
 ///
-/// let labels: Labels = Labels::try_from(BTreeMap::<String, String>::new()).unwrap();
+/// let labels: Labels = Labels::new();
 ///
 /// let volume_source =
 ///     ListenerOperatorVolumeSourceBuilder::new(
@@ -555,7 +564,6 @@ impl ListenerOperatorVolumeSourceBuilder {
 mod tests {
     use super::*;
     use k8s_openapi::apimachinery::pkg::api::resource::Quantity;
-    use std::collections::BTreeMap;
 
     #[test]
     fn builder() {
@@ -615,7 +623,7 @@ mod tests {
 
     #[test]
     fn listener_operator_volume_source_builder() {
-        let labels: Labels = Labels::try_from(BTreeMap::<String, String>::new()).unwrap();
+        let labels: Labels = Labels::new();
 
         let builder = ListenerOperatorVolumeSourceBuilder::new(
             &ListenerReference::ListenerClass("public".into()),

crates/stackable-operator/src/cluster_resources.rs

@@ -12,7 +12,7 @@ use crate::{
     },
     kvp::{
         consts::{K8S_APP_INSTANCE_KEY, K8S_APP_MANAGED_BY_KEY, K8S_APP_NAME_KEY},
-        Label, LabelError, Labels,
+        label, LabelError, Labels,
     },
     utils::format_full_controller_name,
 };
@@ -464,12 +464,12 @@ impl ClusterResources {
     /// Return required labels for cluster resources to be uniquely identified for clean up.
     // TODO: This is a (quick-fix) helper method but should be replaced by better label handling
     pub fn get_required_labels(&self) -> Result<Labels, LabelError> {
-        let mut labels = Labels::common(&self.app_name, &self.app_instance)?;
+        let mut labels = label::well_known::sets::common(&self.app_name, &self.app_instance)?;
 
-        labels.insert(Label::managed_by(
+        labels.extend([label::well_known::managed_by(
             &self.operator_name,
             &self.controller_name,
-        )?);
+        )?]);
 
         Ok(labels)
     }

crates/stackable-operator/src/commons/product_image_selection.rs

@@ -5,7 +5,7 @@ use serde::{Deserialize, Serialize};
 use strum::AsRefStr;
 
 #[cfg(doc)]
-use crate::kvp::Labels;
+use crate::kvp::label;
 
 pub const STACKABLE_DOCKER_REPO: &str = "docker.stackable.tech/stackable";
 
@@ -67,7 +67,7 @@ pub struct ResolvedProductImage {
     /// Version of the product, e.g. `1.4.1`.
     pub product_version: String,
 
-    /// App version as formatted for [`Labels::recommended`]
+    /// App version as formatted for [`label::sets::recommended`]
     pub app_version_label: String,
 
     /// Image to be used for the product image e.g. `docker.stackable.tech/stackable/superset:1.4.1-stackable2.1.0`