Make fn fallible

sbernauer · sbernauer · commit 5b25c0d4a36f · 2024-09-20T16:05:59.000+02:00
diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md
@@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file.
 
 ### Fixed
 
-- Avoid clashing volumes and mounts by only adding volumes or mounts if they do not already exist ([#871]).
+- BREAKING: Avoid clashing volumes and mounts by only adding volumes or mounts if they do not already exist. This makes functions such as `PodBuilder::add_volume` or `ContainerBuilder::add_volume_mount` as well as related ones fallible ([#871]).
 
 ### Changed
 
diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs
@@ -6,7 +6,7 @@ use k8s_openapi::api::core::v1::{
     LifecycleHandler, ObjectFieldSelector, Probe, ResourceRequirements, SecretKeySelector,
     SecurityContext, VolumeMount,
 };
-use snafu::{ResultExt as _, Snafu};
+use snafu::{ensure, ResultExt as _, Snafu};
 
 use crate::{
     commons::product_image_selection::ResolvedProductImage,
@@ -22,6 +22,15 @@ pub enum Error {
         source: validation::Errors,
         container_name: String,
     },
+
+    #[snafu(display(
+        "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content. \
+            The existing volumeMount is {existing_volume_mount:?}, the new one is {new_volume_mount:?}"
+    ))]
+    ClashingVolumeMountMountPath {
+        existing_volume_mount: VolumeMount,
+        new_volume_mount: VolumeMount,
+    },
 }
 
 /// A builder to build [`Container`] objects.
@@ -206,30 +215,29 @@ impl ContainerBuilder {
     ///
     /// We could have made this function fallible, but decided not to do so for now, as it would be a bigger breaking
     /// change.
-    pub fn add_volume_mount_struct(&mut self, volume_mount: VolumeMount) -> &mut Self {
+    pub fn add_volume_mount_struct(&mut self, volume_mount: VolumeMount) -> Result<&mut Self> {
         if let Some(existing_volume_mount) = self.volume_mounts.get(&volume_mount.mount_path) {
-            if !existing_volume_mount.eq(&volume_mount) {
-                tracing::error!(
-                    ?existing_volume_mount,
-                    new_volume_mount = ?volume_mount,
-                    "The operator tried to add a volumeMount to Container, which was already added with a different content! \
-                        The new volumeMount will be ignored."
-                );
-            }
+            ensure!(
+                existing_volume_mount.eq(&volume_mount),
+                ClashingVolumeMountMountPathSnafu {
+                    existing_volume_mount: existing_volume_mount.clone(),
+                    new_volume_mount: volume_mount,
+                }
+            );
         } else {
             self.volume_mounts
                 .insert(volume_mount.mount_path.clone(), volume_mount);
         }
 
-        self
+        Ok(self)
     }
 
     /// See [`Self::add_volume_mount_struct`] for details
     pub fn add_volume_mount(
         &mut self,
         name: impl Into<String>,
         path: impl Into<String>,
-    ) -> &mut Self {
+    ) -> Result<&mut Self> {
         self.add_volume_mount_struct(VolumeMount {
             name: name.into(),
             mount_path: path.into(),
@@ -241,11 +249,12 @@ impl ContainerBuilder {
     pub fn add_volume_mounts(
         &mut self,
         volume_mounts: impl IntoIterator<Item = VolumeMount>,
-    ) -> &mut Self {
+    ) -> Result<&mut Self> {
         for volume_mount in volume_mounts {
-            self.add_volume_mount_struct(volume_mount);
+            self.add_volume_mount_struct(volume_mount)?;
         }
-        self
+
+        Ok(self)
     }
 
     pub fn readiness_probe(&mut self, probe: Probe) -> &mut Self {
@@ -427,6 +436,7 @@ mod tests {
             .add_env_var_from_config_map("envFromConfigMap", "my-configmap", "my-key")
             .add_env_var_from_secret("envFromSecret", "my-secret", "my-key")
             .add_volume_mount("configmap", "/mount")
+            .unwrap()
             .add_container_port(container_port_name, container_port)
             .resources(resources.clone())
             .add_container_ports(vec![ContainerPortBuilder::new(container_port_1)
@@ -543,6 +553,7 @@ mod tests {
                     "input is 64 bytes long but must be no more than 63"
                 )
             }
+            _ => {}
         }
         // One characters shorter name is valid
         let max_len_container_name: String = long_container_name.chars().skip(1).collect();
@@ -616,6 +627,7 @@ mod tests {
             } => {
                 assert!(dbg!(source.to_string()).contains(dbg!(expected_err_contains)));
             }
+            _ => {}
         }
     }
 }
diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs
@@ -9,7 +9,7 @@ use k8s_openapi::{
     },
     apimachinery::pkg::{api::resource::Quantity, apis::meta::v1::ObjectMeta},
 };
-use snafu::{OptionExt, ResultExt, Snafu};
+use snafu::{ensure, OptionExt, ResultExt, Snafu};
 use tracing::warn;
 
 use crate::kvp::Labels;
@@ -51,6 +51,14 @@ pub enum Error {
 
     #[snafu(display("object is missing key {key:?}"))]
     MissingObjectKey { key: &'static str },
+
+    #[snafu(display("The volume {volume_name:?} is clashing with an already existing volume with the same name but a different content. \
+        The existing volume is {existing_volume:?}, the new one is {new_volume:?}"))]
+    ClashingVolumeName {
+        volume_name: String,
+        existing_volume: Volume,
+        new_volume: Volume,
+    },
 }
 
 /// A builder to build [`Pod`] or [`PodTemplateSpec`] objects.
@@ -267,7 +275,7 @@ impl PodBuilder {
         &mut self,
         name: impl Into<String>,
         quantity: Option<Quantity>,
-    ) -> &mut Self {
+    ) -> Result<&mut Self> {
         self.add_volume(
             VolumeBuilder::new(name)
                 .with_empty_dir(None::<String>, quantity)
@@ -285,30 +293,30 @@ impl PodBuilder {
     ///
     /// We could have made this function fallible, but decided not to do so for now, as it would be a bigger breaking
     /// change.
-    pub fn add_volume(&mut self, volume: Volume) -> &mut Self {
+    pub fn add_volume(&mut self, volume: Volume) -> Result<&mut Self> {
         if let Some(existing_volume) = self.volumes.get(&volume.name) {
-            if !existing_volume.eq(&volume) {
-                tracing::error!(
-                    ?existing_volume,
-                    new_volume = ?volume,
-                    "The operator tried to add a volume to Pod, which was already added with a different content! \
-                        The new volume will be ignored."
-                );
-            }
+            ensure!(
+                existing_volume.eq(&volume),
+                ClashingVolumeNameSnafu {
+                    volume_name: volume.name.clone(),
+                    existing_volume: existing_volume.clone(),
+                    new_volume: volume
+                }
+            );
         } else {
             self.volumes.insert(volume.name.clone(), volume);
         }
 
-        self
+        Ok(self)
     }
 
     /// See [`Self::add_volume`] for details
-    pub fn add_volumes(&mut self, volumes: Vec<Volume>) -> &mut Self {
+    pub fn add_volumes(&mut self, volumes: Vec<Volume>) -> Result<&mut Self> {
         for volume in volumes {
-            self.add_volume(volume);
+            self.add_volume(volume)?;
         }
 
-        self
+        Ok(self)
     }
 
     /// Add a [`Volume`] for the storage class `listeners.stackable.tech` with the given listener
@@ -337,6 +345,7 @@ impl PodBuilder {
     ///         ContainerBuilder::new("container")
     ///             .unwrap()
     ///             .add_volume_mount("listener", "/path/to/volume")
+    ///             .unwrap()
     ///             .build(),
     ///     )
     ///     .add_listener_volume_by_listener_class("listener", "nodeport", &labels)
@@ -392,7 +401,7 @@ impl PodBuilder {
             name: volume_name.into(),
             ephemeral: Some(volume),
             ..Volume::default()
-        });
+        })?;
 
         Ok(self)
     }
@@ -423,6 +432,7 @@ impl PodBuilder {
     ///         ContainerBuilder::new("container")
     ///             .unwrap()
     ///             .add_volume_mount("listener", "/path/to/volume")
+    ///             .unwrap()
     ///             .build(),
     ///     )
     ///     .add_listener_volume_by_listener_name("listener", "preprovisioned-listener", &labels)
@@ -478,7 +488,7 @@ impl PodBuilder {
             name: volume_name.into(),
             ephemeral: Some(volume),
             ..Volume::default()
-        });
+        })?;
 
         Ok(self)
     }
@@ -710,6 +720,7 @@ mod tests {
                     .with_config_map("configmap")
                     .build(),
             )
+            .unwrap()
             .termination_grace_period(&Duration::from_secs(42))
             .unwrap()
             .build()
diff --git a/crates/stackable-operator/src/commons/authentication/ldap.rs b/crates/stackable-operator/src/commons/authentication/ldap.rs
@@ -5,7 +5,10 @@ use snafu::{ResultExt, Snafu};
 use url::{ParseError, Url};
 
 use crate::{
-    builder::pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder},
+    builder::{
+        self,
+        pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder},
+    },
     commons::{
         authentication::SECRET_BASE_PATH,
         networking::HostName,
@@ -16,7 +19,7 @@ use crate::{
 
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
-#[derive(Debug, PartialEq, Snafu)]
+#[derive(Debug, Snafu)]
 pub enum Error {
     #[snafu(display(
         "failed to convert bind credentials (secret class volume) into named Kubernetes volume"
@@ -28,6 +31,14 @@ pub enum Error {
 
     #[snafu(display("failed to add LDAP TLS client details volumes and volume mounts"))]
     AddLdapTlsClientDetailsVolumes { source: TlsClientDetailsError },
+
+    #[snafu(display("failed to add needed volumes"))]
+    AddVolumes { source: builder::pod::Error },
+
+    #[snafu(display("failed to add needed volumeMounts"))]
+    AddVolumeMounts {
+        source: builder::pod::container::Error,
+    },
 }
 
 #[derive(
@@ -99,10 +110,11 @@ impl AuthenticationProvider {
         container_builders: Vec<&mut ContainerBuilder>,
     ) -> Result<()> {
         let (volumes, mounts) = self.volumes_and_mounts()?;
-        pod_builder.add_volumes(volumes);
+        pod_builder.add_volumes(volumes).context(AddVolumesSnafu)?;
 
         for cb in container_builders {
-            cb.add_volume_mounts(mounts.clone());
+            cb.add_volume_mounts(mounts.clone())
+                .context(AddVolumeMountsSnafu)?;
         }
 
         Ok(())
diff --git a/crates/stackable-operator/src/commons/s3/helpers.rs b/crates/stackable-operator/src/commons/s3/helpers.rs
@@ -11,9 +11,9 @@ use crate::{
 };
 
 use super::{
-    AddS3CredentialVolumesSnafu, AddS3TlsClientDetailsVolumesSnafu, ParseS3EndpointSnafu,
-    RetrieveS3ConnectionSnafu, S3Bucket, S3BucketSpec, S3Connection, S3ConnectionSpec, S3Error,
-    SetS3EndpointSchemeSnafu,
+    AddS3CredentialVolumesSnafu, AddS3TlsClientDetailsVolumesSnafu, AddVolumeMountsSnafu,
+    AddVolumesSnafu, ParseS3EndpointSnafu, RetrieveS3ConnectionSnafu, S3Bucket, S3BucketSpec,
+    S3Connection, S3ConnectionSpec, S3Error, SetS3EndpointSchemeSnafu,
 };
 
 #[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]
@@ -85,9 +85,10 @@ impl ResolvedS3Connection {
         container_builders: Vec<&mut ContainerBuilder>,
     ) -> Result<(), S3Error> {
         let (volumes, mounts) = self.volumes_and_mounts()?;
-        pod_builder.add_volumes(volumes);
+        pod_builder.add_volumes(volumes).context(AddVolumesSnafu)?;
         for cb in container_builders {
-            cb.add_volume_mounts(mounts.clone());
+            cb.add_volume_mounts(mounts.clone())
+                .context(AddVolumeMountsSnafu)?;
         }
 
         Ok(())
diff --git a/crates/stackable-operator/src/commons/s3/mod.rs b/crates/stackable-operator/src/commons/s3/mod.rs
@@ -7,6 +7,8 @@ pub use helpers::*;
 use snafu::Snafu;
 use url::Url;
 
+use crate::builder::{self};
+
 use super::{secret_class::SecretClassVolumeError, tls_verification::TlsClientDetailsError};
 
 #[derive(Debug, Snafu)]
@@ -31,4 +33,12 @@ pub enum S3Error {
 
     #[snafu(display("failed to add S3 TLS client details volumes and volume mounts"))]
     AddS3TlsClientDetailsVolumes { source: TlsClientDetailsError },
+
+    #[snafu(display("failed to add needed volumes"))]
+    AddVolumes { source: builder::pod::Error },
+
+    #[snafu(display("failed to add needed volumeMounts"))]
+    AddVolumeMounts {
+        source: builder::pod::container::Error,
+    },
 }
diff --git a/crates/stackable-operator/src/commons/tls_verification.rs b/crates/stackable-operator/src/commons/tls_verification.rs
@@ -4,17 +4,28 @@ use serde::{Deserialize, Serialize};
 use snafu::{ResultExt, Snafu};
 
 use crate::{
-    builder::pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder},
+    builder::{
+        self,
+        pod::{container::ContainerBuilder, volume::VolumeMountBuilder, PodBuilder},
+    },
     commons::{
         authentication::SECRET_BASE_PATH,
         secret_class::{SecretClassVolume, SecretClassVolumeError},
     },
 };
 
-#[derive(Debug, PartialEq, Snafu)]
+#[derive(Debug, Snafu)]
 pub enum TlsClientDetailsError {
     #[snafu(display("failed to convert secret class volume into named Kubernetes volume"))]
     SecretClassVolume { source: SecretClassVolumeError },
+
+    #[snafu(display("failed to add needed volumes"))]
+    AddVolumes { source: builder::pod::Error },
+
+    #[snafu(display("failed to add needed volumeMounts"))]
+    AddVolumeMounts {
+        source: builder::pod::container::Error,
+    },
 }
 
 #[derive(
@@ -41,10 +52,11 @@ impl TlsClientDetails {
         container_builders: Vec<&mut ContainerBuilder>,
     ) -> Result<(), TlsClientDetailsError> {
         let (volumes, mounts) = self.volumes_and_mounts()?;
-        pod_builder.add_volumes(volumes);
+        pod_builder.add_volumes(volumes).context(AddVolumesSnafu)?;
 
         for cb in container_builders {
-            cb.add_volume_mounts(mounts.clone());
+            cb.add_volume_mounts(mounts.clone())
+                .context(AddVolumeMountsSnafu)?;
         }
 
         Ok(())
diff --git a/crates/stackable-operator/src/product_logging/framework.rs b/crates/stackable-operator/src/product_logging/framework.rs
