From c0c9c0de4eb7caf5a516c57185c96421110385d8 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 20 Sep 2024 11:22:54 +0200 Subject: [PATCH 01/25] WIP --- Cargo.lock | 1 + Cargo.toml | 1 + crates/stackable-operator/Cargo.toml | 1 + .../src/builder/pod/container.rs | 65 +++++++++++++++---- .../stackable-operator/src/builder/pod/mod.rs | 53 ++++++++++++--- .../src/commons/s3/helpers.rs | 51 +++++---------- 6 files changed, 116 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 35ae6b63f..35adde1a5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2936,6 +2936,7 @@ dependencies = [ "dockerfile-parser", "either", "futures", + "indexmap 2.5.0", "json-patch", "k8s-openapi", "kube", diff --git a/Cargo.toml b/Cargo.toml index b92b0ab2b..b3e8c2e14 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,7 @@ ecdsa = { version = "0.16.9", features = ["digest", "pem"] } either = "1.13.0" futures = "0.3.30" futures-util = "0.3.30" +indexmap = "2.5" hyper = { version = "1.4.1", features = ["full"] } hyper-util = "0.1.8" itertools = "0.13.0" diff --git a/crates/stackable-operator/Cargo.toml b/crates/stackable-operator/Cargo.toml index 0aaa82e57..b71a0cfea 100644 --- a/crates/stackable-operator/Cargo.toml +++ b/crates/stackable-operator/Cargo.toml @@ -21,6 +21,7 @@ derivative.workspace = true dockerfile-parser.workspace = true either.workspace = true futures.workspace = true +indexmap.workspace = true json-patch.workspace = true k8s-openapi.workspace = true kube.workspace = true diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index ae18a7eae..13ce1da42 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -1,5 +1,6 @@ use std::fmt; +use indexmap::IndexMap; use k8s_openapi::api::core::v1::{ ConfigMapKeySelector, Container, ContainerPort, EnvVar, EnvVarSource, Lifecycle, LifecycleHandler, ObjectFieldSelector, Probe, ResourceRequirements, SecretKeySelector, @@ -26,6 +27,11 @@ pub enum Error { /// A builder to build [`Container`] objects. /// /// This will automatically create the necessary volumes and mounts for each `ConfigMap` which is added. +/// +/// This struct is often times using an [`IndexMap`] to have consistent ordering (so we don't produce reconcile loops). +/// We are also choosing it over an [`std::collections::BTreeMap`], as it's easier to debug for users, as logically +/// grouped volumeMounts (e.g. all volumeMounts related to S3) are near each other in the list instead of "just" being +/// sorted alphabetically. #[derive(Clone, Default)] pub struct ContainerBuilder { args: Option>, @@ -36,7 +42,9 @@ pub struct ContainerBuilder { image_pull_policy: Option, name: String, resources: Option, - volume_mounts: Option>, + + /// The key is the volumeMount mountPath. + volume_mounts: IndexMap, readiness_probe: Option, liveness_probe: Option, startup_probe: Option, @@ -188,28 +196,55 @@ impl ContainerBuilder { self } + /// This function only adds the [`VolumeMount`] in case there is no volumeMount with the same mountPath already. + /// In case there already was a volumeMount with the same path already, an [`tracing::error`] is raised in case the + /// contents of the volumeMounts differ. + /// + /// Historically this function unconditionally added volumeMounts, which resulted in invalid + /// [`k8s_openapi::api::core::v1::PodSpec`]s, as volumeMounts where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] + /// two times, resulting in e.g. the s3 credentials being mounted twice as the same volumeMount. + /// + /// 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 { + 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." + ); + } + } else { + self.volume_mounts + .insert(volume_mount.mount_path.clone(), volume_mount); + } + + self + } + + /// See [`Self::add_volume_mount_struct`] for details pub fn add_volume_mount( &mut self, name: impl Into, path: impl Into, ) -> &mut Self { - self.volume_mounts - .get_or_insert_with(Vec::new) - .push(VolumeMount { - name: name.into(), - mount_path: path.into(), - ..VolumeMount::default() - }); - self + self.add_volume_mount_struct(VolumeMount { + name: name.into(), + mount_path: path.into(), + ..VolumeMount::default() + }) } + /// See [`Self::add_volume_mount_struct`] for details pub fn add_volume_mounts( &mut self, volume_mounts: impl IntoIterator, ) -> &mut Self { - self.volume_mounts - .get_or_insert_with(Vec::new) - .extend(volume_mounts); + for volume_mount in volume_mounts { + self.add_volume_mount_struct(volume_mount); + } self } @@ -265,7 +300,11 @@ impl ContainerBuilder { resources: self.resources.clone(), name: self.name.clone(), ports: self.container_ports.clone(), - volume_mounts: self.volume_mounts.clone(), + volume_mounts: if self.volume_mounts.is_empty() { + None + } else { + Some(self.volume_mounts.clone().into_values().collect()) + }, readiness_probe: self.readiness_probe.clone(), liveness_probe: self.liveness_probe.clone(), startup_probe: self.startup_probe.clone(), diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 56fc62b70..9e28935f6 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -1,5 +1,6 @@ use std::{collections::BTreeMap, num::TryFromIntError}; +use indexmap::IndexMap; use k8s_openapi::{ api::core::v1::{ Affinity, Container, LocalObjectReference, NodeAffinity, Pod, PodAffinity, PodAntiAffinity, @@ -53,6 +54,10 @@ pub enum Error { } /// A builder to build [`Pod`] or [`PodTemplateSpec`] objects. +/// +/// This struct is often times using an [`IndexMap`] to have consistent ordering (so we don't produce reconcile loops). +/// We are also choosing it over an [`BTreeMap`], as it's easier to debug for users, as logically grouped volumes +/// (e.g. all volumes related to S3) are near each other in the list instead of "just" being sorted alphabetically. #[derive(Clone, Debug, Default, PartialEq)] pub struct PodBuilder { containers: Vec, @@ -67,7 +72,9 @@ pub struct PodBuilder { status: Option, security_context: Option, tolerations: Option>, - volumes: Option>, + + /// The key is the volume name. + volumes: IndexMap, service_account_name: Option, image_pull_secrets: Option>, restart_policy: Option, @@ -254,11 +261,6 @@ impl PodBuilder { self } - pub fn add_volume(&mut self, volume: Volume) -> &mut Self { - self.volumes.get_or_insert_with(Vec::new).push(volume); - self - } - /// Utility function for the common case of adding an emptyDir Volume /// with the given name and no medium and no quantity. pub fn add_empty_dir_volume( @@ -273,8 +275,39 @@ impl PodBuilder { ) } + /// This function only adds the [`Volume`] in case there is no volume with the same name already. + /// In case there already was a volume with the same name already, an [`tracing::error`] is raised in case the + /// contents of the volumes differ. + /// + /// Historically this function unconditionally added volumes, which resulted in invalid [`PodSpec`]s, as volumes + /// where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] two times, + /// resulting in e.g. the s3 credentials being mounted twice as the sake volume. + /// + /// 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 { + 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." + ); + } + } else { + self.volumes.insert(volume.name.clone(), volume); + } + + self + } + + /// See [`Self::add_volume`] for details pub fn add_volumes(&mut self, volumes: Vec) -> &mut Self { - self.volumes.get_or_insert_with(Vec::new).extend(volumes); + for volume in volumes { + self.add_volume(volume); + } + self } @@ -533,7 +566,11 @@ impl PodBuilder { }), security_context: self.security_context.clone(), tolerations: self.tolerations.clone(), - volumes: self.volumes.clone(), + volumes: if self.volumes.is_empty() { + None + } else { + Some(self.volumes.clone().into_values().collect()) + }, // Legacy feature for ancient Docker images // In practice, this just causes a bunch of unused environment variables that may conflict with other uses, // such as https://github.com/stackabletech/spark-operator/pull/256. diff --git a/crates/stackable-operator/src/commons/s3/helpers.rs b/crates/stackable-operator/src/commons/s3/helpers.rs index c6392dbe3..5b23d308e 100644 --- a/crates/stackable-operator/src/commons/s3/helpers.rs +++ b/crates/stackable-operator/src/commons/s3/helpers.rs @@ -79,16 +79,12 @@ impl ResolvedS3Connection { /// /// * Credentials needed to connect to S3 /// * Needed TLS volumes - /// - /// `unique_identifier` needs to be a unique identifier (e.g. in case of trino-operator the name of the catalog), - /// so that multiple mounts of the same SecretClass do not produce clashing volumes and volumeMounts. pub fn add_volumes_and_mounts( &self, - unique_identifier: &str, pod_builder: &mut PodBuilder, container_builders: Vec<&mut ContainerBuilder>, ) -> Result<(), S3Error> { - let (volumes, mounts) = self.volumes_and_mounts(unique_identifier)?; + let (volumes, mounts) = self.volumes_and_mounts()?; pod_builder.add_volumes(volumes); for cb in container_builders { cb.add_volume_mounts(mounts.clone()); @@ -99,16 +95,13 @@ impl ResolvedS3Connection { /// It is recommended to use [`Self::add_volumes_and_mounts`], this function returns you the /// volumes and mounts in case you need to add them by yourself. - pub fn volumes_and_mounts( - &self, - unique_identifier: &str, - ) -> Result<(Vec, Vec), S3Error> { + pub fn volumes_and_mounts(&self) -> Result<(Vec, Vec), S3Error> { let mut volumes = Vec::new(); let mut mounts = Vec::new(); if let Some(credentials) = &self.credentials { let secret_class = &credentials.secret_class; - let volume_name = format!("{unique_identifier}-{secret_class}-s3-credentials"); + let volume_name = format!("{secret_class}-s3-credentials"); volumes.push( credentials @@ -116,11 +109,8 @@ impl ResolvedS3Connection { .context(AddS3CredentialVolumesSnafu)?, ); mounts.push( - VolumeMountBuilder::new( - volume_name, - format!("{SECRET_BASE_PATH}/{unique_identifier}-{secret_class}"), - ) - .build(), + VolumeMountBuilder::new(volume_name, format!("{SECRET_BASE_PATH}/{secret_class}")) + .build(), ); } @@ -137,15 +127,12 @@ impl ResolvedS3Connection { /// Returns the path of the files containing bind user and password. /// This will be None if there are no credentials for this LDAP connection. - /// - /// `unique_identifier` needs to be a unique identifier (e.g. in case of trino-operator the name of the catalog), - /// so that multiple mounts of the same SecretClass do not produce clashing volumes and volumeMounts. - pub fn credentials_mount_paths(&self, unique_identifier: &str) -> Option<(String, String)> { + pub fn credentials_mount_paths(&self) -> Option<(String, String)> { self.credentials.as_ref().map(|bind_credentials| { let secret_class = &bind_credentials.secret_class; ( - format!("{SECRET_BASE_PATH}/{unique_identifier}-{secret_class}/accessKey"), - format!("{SECRET_BASE_PATH}/{unique_identifier}-{secret_class}/secretKey"), + format!("{SECRET_BASE_PATH}/{secret_class}/accessKey"), + format!("{SECRET_BASE_PATH}/{secret_class}/secretKey"), ) }) } @@ -214,7 +201,7 @@ mod test { credentials: None, tls: TlsClientDetails { tls: None }, }; - let (volumes, mounts) = s3.volumes_and_mounts("lakehouse").unwrap(); + let (volumes, mounts) = s3.volumes_and_mounts().unwrap(); assert_eq!(s3.endpoint().unwrap(), Url::parse("http://minio").unwrap()); assert_eq!(volumes, vec![]); @@ -239,7 +226,7 @@ mod test { }), }, }; - let (mut volumes, mut mounts) = s3.volumes_and_mounts("lakehouse").unwrap(); + let (mut volumes, mut mounts) = s3.volumes_and_mounts().unwrap(); assert_eq!( s3.endpoint().unwrap(), @@ -250,10 +237,7 @@ mod test { assert_eq!(mounts.len(), 1); let mount = mounts.remove(0); - assert_eq!( - &volume.name, - "lakehouse-ionos-s3-credentials-s3-credentials" - ); + assert_eq!(&volume.name, "ionos-s3-credentials-s3-credentials"); assert_eq!( &volume .ephemeral @@ -271,15 +255,12 @@ mod test { ); assert_eq!(mount.name, volume.name); + assert_eq!(mount.mount_path, "/stackable/secrets/ionos-s3-credentials"); assert_eq!( - mount.mount_path, - "/stackable/secrets/lakehouse-ionos-s3-credentials" - ); - assert_eq!( - s3.credentials_mount_paths("lakehouse"), + s3.credentials_mount_paths(), Some(( - "/stackable/secrets/lakehouse-ionos-s3-credentials/accessKey".to_string(), - "/stackable/secrets/lakehouse-ionos-s3-credentials/secretKey".to_string() + "/stackable/secrets/ionos-s3-credentials/accessKey".to_string(), + "/stackable/secrets/ionos-s3-credentials/secretKey".to_string() )) ); } @@ -297,7 +278,7 @@ mod test { }), }, }; - let (volumes, mounts) = s3.volumes_and_mounts("lakehouse").unwrap(); + let (volumes, mounts) = s3.volumes_and_mounts().unwrap(); assert_eq!( s3.endpoint().unwrap(), From a508c90bf5c2c6bab69a563a1819eee7c69a966e Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 20 Sep 2024 12:38:12 +0200 Subject: [PATCH 02/25] changelog --- crates/stackable-operator/CHANGELOG.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 83a197b50..c758296ed 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,6 +4,16 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed + +- Avoid clashing volumes and mounts by only adding volumes if they do not already exist ([#871]). + +### Changed + +- BREAKING: Remove the `unique_identifier` argument from `ResolvedS3Connection::add_volumes_and_mounts`, `ResolvedS3Connection::volumes_and_mounts` and `ResolvedS3Connection::credentials_mount_paths` as it is not needed anymore ([#871]). + +[#871]: https://github.com/stackabletech/operator-rs/pull/871 + ## [0.76.0] - 2024-09-19 ### Added From 6288d214527cee4c0f5fe71a61b644d6c2a70305 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 20 Sep 2024 13:08:08 +0200 Subject: [PATCH 03/25] changelog --- crates/stackable-operator/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index c758296ed..3078e569e 100644 --- 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 if they do not already exist ([#871]). +- Avoid clashing volumes and mounts by only adding volumes or mounts if they do not already exist ([#871]). ### Changed From 5b25c0d4a36ffad9b32b0985cc501d7ca2519a2a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 20 Sep 2024 16:05:59 +0200 Subject: [PATCH 04/25] Make fn fallible --- crates/stackable-operator/CHANGELOG.md | 2 +- .../src/builder/pod/container.rs | 42 ++++++++++------- .../stackable-operator/src/builder/pod/mod.rs | 45 ++++++++++++------- .../src/commons/authentication/ldap.rs | 20 +++++++-- .../src/commons/s3/helpers.rs | 11 ++--- .../stackable-operator/src/commons/s3/mod.rs | 10 +++++ .../src/commons/tls_verification.rs | 20 +++++++-- .../src/product_logging/framework.rs | 31 ++++++++++--- 8 files changed, 129 insertions(+), 52 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 3078e569e..c90f09dd2 100644 --- 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 index 13ce1da42..1e2ff68d1 100644 --- 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,22 +215,21 @@ 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 @@ -229,7 +237,7 @@ impl ContainerBuilder { &mut self, name: impl Into, path: impl Into, - ) -> &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, - ) -> &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 index 9e28935f6..231c48367 100644 --- 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, quantity: Option, - ) -> &mut Self { + ) -> Result<&mut Self> { self.add_volume( VolumeBuilder::new(name) .with_empty_dir(None::, 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) -> &mut Self { + pub fn add_volumes(&mut self, volumes: Vec) -> 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 index 2b43bbd71..4c3e72188 100644 --- 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 = std::result::Result; -#[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 index 5b23d308e..d72af003a 100644 --- 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 index c7f36c6eb..9040f4ec5 100644 --- 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 index a29695a95..cfb3539a4 100644 --- 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 index 18795f7f3..210faa3de 100644 --- a/crates/stackable-operator/src/product_logging/framework.rs +++ b/crates/stackable-operator/src/product_logging/framework.rs @@ -2,8 +2,10 @@ use std::{cmp, fmt::Write, ops::Mul}; +use snafu::{ResultExt, Snafu}; + use crate::{ - builder::pod::container::ContainerBuilder, + builder::{self, pod::container::ContainerBuilder}, commons::product_image_selection::ResolvedProductImage, k8s_openapi::{ api::core::v1::{Container, ResourceRequirements}, @@ -31,6 +33,19 @@ const SHUTDOWN_FILE: &str = "shutdown"; /// File name of the Vector config file pub const VECTOR_CONFIG_FILE: &str = "vector.yaml"; +#[derive(Debug, Snafu)] +pub enum LoggingError { + #[snafu(display("failed to create container"))] + CreateContainer { + source: builder::pod::container::Error, + }, + + #[snafu(display("failed to add needed volumeMounts"))] + AddVolumeMounts { + source: builder::pod::container::Error, + }, +} + /// Calculate the size limit for the log volume. /// /// The size limit must be much larger than the sum of the given maximum log file sizes for the @@ -81,6 +96,7 @@ pub const VECTOR_CONFIG_FILE: &str = "vector.yaml"; /// ], /// )), /// ) +/// .unwrap() /// .build() /// .unwrap(); /// ``` @@ -1398,7 +1414,7 @@ sinks: /// "log", /// logging.containers.get(&Container::Vector), /// resources, -/// )); +/// ).unwrap()); /// } /// /// pod_builder.build().unwrap(); @@ -1409,7 +1425,7 @@ pub fn vector_container( log_volume_name: &str, log_config: Option<&ContainerLogConfig>, resources: ResourceRequirements, -) -> Container { +) -> Result { let log_level = if let Some(ContainerLogConfig { choice: Some(ContainerLogConfigChoice::Automatic(automatic_log_config)), }) = log_config @@ -1419,8 +1435,8 @@ pub fn vector_container( LogLevel::INFO }; - ContainerBuilder::new("vector") - .unwrap() + Ok(ContainerBuilder::new("vector") + .context(CreateContainerSnafu)? .image_from_product_image(image) .command(vec![ "/bin/bash".to_string(), @@ -1447,9 +1463,11 @@ kill $vector_pid )]) .add_env_var("VECTOR_LOG", log_level.to_vector_literal()) .add_volume_mount(config_volume_name, STACKABLE_CONFIG_DIR) + .context(AddVolumeMountsSnafu)? .add_volume_mount(log_volume_name, STACKABLE_LOG_DIR) + .context(AddVolumeMountsSnafu)? .resources(resources) - .build() + .build()) } /// Command to create a shutdown file for the vector container. @@ -1477,6 +1495,7 @@ kill $vector_pid /// .command(vec!["bash".to_string(), "-c".to_string()]) /// .args(vec![args.join(" && ")]) /// .add_volume_mount("log", STACKABLE_LOG_DIR) +/// .unwrap() /// .build(); /// ``` pub fn create_vector_shutdown_file_command(stackable_log_dir: &str) -> String { From 1882e2fc32a807a9b1e6780c0bf01753af5f538b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 20 Sep 2024 16:26:08 +0200 Subject: [PATCH 05/25] clippy --- .../src/builder/pod/container.rs | 40 ++++++++----------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 1e2ff68d1..8f0c88758 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -28,8 +28,8 @@ pub enum Error { The existing volumeMount is {existing_volume_mount:?}, the new one is {new_volume_mount:?}" ))] ClashingVolumeMountMountPath { - existing_volume_mount: VolumeMount, - new_volume_mount: VolumeMount, + existing_volume_mount: Box, + new_volume_mount: Box, }, } @@ -539,21 +539,17 @@ mod tests { "lengthexceededlengthexceededlengthexceededlengthexceededlengthex"; assert_eq!(long_container_name.len(), 64); // 63 characters is the limit for container names let result = ContainerBuilder::new(long_container_name); - match result - .err() - .expect("Container name exceeding 63 characters should cause an error") - { - Error::InvalidContainerName { + if let Error::InvalidContainerName { container_name, source, - } => { - assert_eq!(container_name, long_container_name); - assert_eq!( - source.to_string(), - "input is 64 bytes long but must be no more than 63" - ) - } - _ => {} + } = result + .err() + .expect("Container name exceeding 63 characters should cause an error") { + assert_eq!(container_name, long_container_name); + assert_eq!( + source.to_string(), + "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(); @@ -617,17 +613,13 @@ mod tests { result: Result, expected_err_contains: &str, ) { - match result - .err() - .expect("Container name exceeding 63 characters should cause an error") - { - Error::InvalidContainerName { + if let Error::InvalidContainerName { container_name: _, source, - } => { - assert!(dbg!(source.to_string()).contains(dbg!(expected_err_contains))); - } - _ => {} + } = result + .err() + .expect("Container name exceeding 63 characters should cause an error") { + assert!(dbg!(source.to_string()).contains(dbg!(expected_err_contains))); } } } From 9ce0c8d697f2e3be79ed00542008800b188fa293 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 20 Sep 2024 16:35:54 +0200 Subject: [PATCH 06/25] fix: Put Volume behind a Box --- crates/stackable-operator/src/builder/pod/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 231c48367..61db00a19 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -56,8 +56,8 @@ pub enum Error { The existing volume is {existing_volume:?}, the new one is {new_volume:?}"))] ClashingVolumeName { volume_name: String, - existing_volume: Volume, - new_volume: Volume, + existing_volume: Box, + new_volume: Box, }, } From af6f83076b54a093e17968a615fe5a4aa39ae6c8 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 09:43:57 +0200 Subject: [PATCH 07/25] cargo fmt --- .../src/builder/pod/container.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 8f0c88758..6537e4fa4 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -540,11 +540,12 @@ mod tests { assert_eq!(long_container_name.len(), 64); // 63 characters is the limit for container names let result = ContainerBuilder::new(long_container_name); if let Error::InvalidContainerName { - container_name, - source, - } = result + container_name, + source, + } = result .err() - .expect("Container name exceeding 63 characters should cause an error") { + .expect("Container name exceeding 63 characters should cause an error") + { assert_eq!(container_name, long_container_name); assert_eq!( source.to_string(), @@ -614,11 +615,12 @@ mod tests { expected_err_contains: &str, ) { if let Error::InvalidContainerName { - container_name: _, - source, - } = result + container_name: _, + source, + } = result .err() - .expect("Container name exceeding 63 characters should cause an error") { + .expect("Container name exceeding 63 characters should cause an error") + { assert!(dbg!(source.to_string()).contains(dbg!(expected_err_contains))); } } From f689dd3d1026477ee0d9260c79c8c996e56773c0 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 11:28:50 +0200 Subject: [PATCH 08/25] Apply suggestions from code review Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com> --- crates/stackable-operator/src/builder/pod/container.rs | 5 +---- crates/stackable-operator/src/builder/pod/mod.rs | 7 ++----- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 6537e4fa4..ac3cf89cc 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -38,7 +38,7 @@ pub enum Error { /// This will automatically create the necessary volumes and mounts for each `ConfigMap` which is added. /// /// This struct is often times using an [`IndexMap`] to have consistent ordering (so we don't produce reconcile loops). -/// We are also choosing it over an [`std::collections::BTreeMap`], as it's easier to debug for users, as logically +/// We are also choosing it over a [`BTreeMap`], because it is easier to debug for users, as logically /// grouped volumeMounts (e.g. all volumeMounts related to S3) are near each other in the list instead of "just" being /// sorted alphabetically. #[derive(Clone, Default)] @@ -212,9 +212,6 @@ impl ContainerBuilder { /// Historically this function unconditionally added volumeMounts, which resulted in invalid /// [`k8s_openapi::api::core::v1::PodSpec`]s, as volumeMounts where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] /// two times, resulting in e.g. the s3 credentials being mounted twice as the same volumeMount. - /// - /// 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) -> Result<&mut Self> { if let Some(existing_volume_mount) = self.volume_mounts.get(&volume_mount.mount_path) { ensure!( diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 61db00a19..926a68e5e 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -52,7 +52,7 @@ 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. \ + #[snafu(display("The volume {volume_name:?} clashes with an existing volume of the same name but with different content. \ The existing volume is {existing_volume:?}, the new one is {new_volume:?}"))] ClashingVolumeName { volume_name: String, @@ -64,7 +64,7 @@ pub enum Error { /// A builder to build [`Pod`] or [`PodTemplateSpec`] objects. /// /// This struct is often times using an [`IndexMap`] to have consistent ordering (so we don't produce reconcile loops). -/// We are also choosing it over an [`BTreeMap`], as it's easier to debug for users, as logically grouped volumes +/// We are also choosing it over a [`BTreeMap`], because it is easier to debug for users, as logically grouped volumes /// (e.g. all volumes related to S3) are near each other in the list instead of "just" being sorted alphabetically. #[derive(Clone, Debug, Default, PartialEq)] pub struct PodBuilder { @@ -290,9 +290,6 @@ impl PodBuilder { /// Historically this function unconditionally added volumes, which resulted in invalid [`PodSpec`]s, as volumes /// where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] two times, /// resulting in e.g. the s3 credentials being mounted twice as the sake volume. - /// - /// 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) -> Result<&mut Self> { if let Some(existing_volume) = self.volumes.get(&volume.name) { ensure!( From a14eecf99b11444ac8dab45f6c91075370494469 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 11:32:35 +0200 Subject: [PATCH 09/25] fix docs --- crates/stackable-operator/src/builder/pod/container.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index ac3cf89cc..7d395da52 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -1,5 +1,8 @@ use std::fmt; +#[cfg(doc)] +use std::collections::BTreeMap; + use indexmap::IndexMap; use k8s_openapi::api::core::v1::{ ConfigMapKeySelector, Container, ContainerPort, EnvVar, EnvVarSource, Lifecycle, From ddfc2662152c1da4f8e008c4d128258f13e9b9d2 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 11:34:57 +0200 Subject: [PATCH 10/25] Update crates/stackable-operator/src/builder/pod/container.rs Co-authored-by: Nick <10092581+NickLarsenNZ@users.noreply.github.com> --- crates/stackable-operator/src/builder/pod/container.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 7d395da52..af03a5c1d 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -436,7 +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() + .expect("add volume mount") .add_container_port(container_port_name, container_port) .resources(resources.clone()) .add_container_ports(vec![ContainerPortBuilder::new(container_port_1) From 53ef278de66ca07c9680810230235ee7a8b260a9 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 11:43:05 +0200 Subject: [PATCH 11/25] expect --- crates/stackable-operator/src/builder/pod/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 0322a1391..4548ae26d 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -717,7 +717,7 @@ mod tests { .with_config_map("configmap") .build(), ) - .unwrap() + .expect("Add volume") .termination_grace_period(&Duration::from_secs(42)) .unwrap() .build() From 1a3f95f359a75599390730575d002797d4ed659a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 13:39:38 +0200 Subject: [PATCH 12/25] Use == --- crates/stackable-operator/src/builder/pod/container.rs | 2 +- crates/stackable-operator/src/builder/pod/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 921b02fbc..567353a02 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -218,7 +218,7 @@ impl ContainerBuilder { 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) { ensure!( - existing_volume_mount.eq(&volume_mount), + existing_volume_mount == &volume_mount, ClashingVolumeMountMountPathSnafu { existing_volume_mount: existing_volume_mount.clone(), new_volume_mount: volume_mount, diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 4548ae26d..0eb4685f5 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -293,7 +293,7 @@ impl PodBuilder { pub fn add_volume(&mut self, volume: Volume) -> Result<&mut Self> { if let Some(existing_volume) = self.volumes.get(&volume.name) { ensure!( - existing_volume.eq(&volume), + existing_volume == &volume, ClashingVolumeNameSnafu { volume_name: volume.name.clone(), existing_volume: existing_volume.clone(), From 6518daa05bc10b107773c3b0b6e37e7ce100c03b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 13:42:31 +0200 Subject: [PATCH 13/25] Apply suggestions from code review Co-authored-by: Techassi --- crates/stackable-operator/src/commons/authentication/ldap.rs | 4 ++-- crates/stackable-operator/src/commons/s3/mod.rs | 4 ++-- crates/stackable-operator/src/commons/tls_verification.rs | 4 ++-- crates/stackable-operator/src/product_logging/framework.rs | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/stackable-operator/src/commons/authentication/ldap.rs b/crates/stackable-operator/src/commons/authentication/ldap.rs index 2a1d5d4ba..2657c05e4 100644 --- a/crates/stackable-operator/src/commons/authentication/ldap.rs +++ b/crates/stackable-operator/src/commons/authentication/ldap.rs @@ -32,10 +32,10 @@ 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"))] + #[snafu(display("failed to add required volumes"))] AddVolumes { source: builder::pod::Error }, - #[snafu(display("failed to add needed volumeMounts"))] + #[snafu(display("failed to add required volumeMounts"))] AddVolumeMounts { source: builder::pod::container::Error, }, diff --git a/crates/stackable-operator/src/commons/s3/mod.rs b/crates/stackable-operator/src/commons/s3/mod.rs index 9040f4ec5..ecd6b87d9 100644 --- a/crates/stackable-operator/src/commons/s3/mod.rs +++ b/crates/stackable-operator/src/commons/s3/mod.rs @@ -34,10 +34,10 @@ 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"))] + #[snafu(display("failed to add required volumes"))] AddVolumes { source: builder::pod::Error }, - #[snafu(display("failed to add needed volumeMounts"))] + #[snafu(display("failed to add required 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 index cfb3539a4..0123a6bad 100644 --- a/crates/stackable-operator/src/commons/tls_verification.rs +++ b/crates/stackable-operator/src/commons/tls_verification.rs @@ -19,10 +19,10 @@ 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"))] + #[snafu(display("failed to add required volumes"))] AddVolumes { source: builder::pod::Error }, - #[snafu(display("failed to add needed volumeMounts"))] + #[snafu(display("failed to add required volumeMounts"))] AddVolumeMounts { source: builder::pod::container::Error, }, diff --git a/crates/stackable-operator/src/product_logging/framework.rs b/crates/stackable-operator/src/product_logging/framework.rs index 0b1a01537..ad0d16c31 100644 --- a/crates/stackable-operator/src/product_logging/framework.rs +++ b/crates/stackable-operator/src/product_logging/framework.rs @@ -40,7 +40,7 @@ pub enum LoggingError { source: builder::pod::container::Error, }, - #[snafu(display("failed to add needed volumeMounts"))] + #[snafu(display("failed to add required volumeMounts"))] AddVolumeMounts { source: builder::pod::container::Error, }, From a814f41cf79fb7ed825a1fade13b2790221ad023 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 13:45:03 +0200 Subject: [PATCH 14/25] Make add_volume_mount_impl private --- crates/stackable-operator/src/builder/pod/container.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 567353a02..534a7a90e 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -215,7 +215,7 @@ impl ContainerBuilder { /// Historically this function unconditionally added volumeMounts, which resulted in invalid /// [`k8s_openapi::api::core::v1::PodSpec`]s, as volumeMounts where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] /// two times, resulting in e.g. the s3 credentials being mounted twice as the same volumeMount. - pub fn add_volume_mount_struct(&mut self, volume_mount: VolumeMount) -> Result<&mut Self> { + fn add_volume_mount_impl(&mut self, volume_mount: VolumeMount) -> Result<&mut Self> { if let Some(existing_volume_mount) = self.volume_mounts.get(&volume_mount.mount_path) { ensure!( existing_volume_mount == &volume_mount, @@ -232,26 +232,26 @@ impl ContainerBuilder { Ok(self) } - /// See [`Self::add_volume_mount_struct`] for details + /// See [`Self::add_volume_mount_impl`] for details pub fn add_volume_mount( &mut self, name: impl Into, path: impl Into, ) -> Result<&mut Self> { - self.add_volume_mount_struct(VolumeMount { + self.add_volume_mount_impl(VolumeMount { name: name.into(), mount_path: path.into(), ..VolumeMount::default() }) } - /// See [`Self::add_volume_mount_struct`] for details + /// See [`Self::add_volume_mount_impl`] for details pub fn add_volume_mounts( &mut self, volume_mounts: impl IntoIterator, ) -> Result<&mut Self> { for volume_mount in volume_mounts { - self.add_volume_mount_struct(volume_mount)?; + self.add_volume_mount_impl(volume_mount)?; } Ok(self) From b7fcdaaadb646395fd7bfad909402baac5760882 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 23 Sep 2024 13:54:41 +0200 Subject: [PATCH 15/25] Move variables out --- .../stackable-operator/src/builder/pod/container.rs | 12 +++++++----- crates/stackable-operator/src/builder/pod/mod.rs | 12 +++++++----- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 534a7a90e..73bc0916e 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -300,6 +300,12 @@ impl ContainerBuilder { } pub fn build(&self) -> Container { + let volume_mounts = if self.volume_mounts.is_empty() { + None + } else { + Some(self.volume_mounts.values().cloned().collect()) + }; + Container { args: self.args.clone(), command: self.command.clone(), @@ -309,11 +315,7 @@ impl ContainerBuilder { resources: self.resources.clone(), name: self.name.clone(), ports: self.container_ports.clone(), - volume_mounts: if self.volume_mounts.is_empty() { - None - } else { - Some(self.volume_mounts.clone().into_values().collect()) - }, + volume_mounts, readiness_probe: self.readiness_probe.clone(), liveness_probe: self.liveness_probe.clone(), startup_probe: self.startup_probe.clone(), diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 0eb4685f5..62ed3b3bd 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -560,6 +560,12 @@ impl PodBuilder { } fn build_spec(&self) -> PodSpec { + let volumes = if self.volumes.is_empty() { + None + } else { + Some(self.volumes.values().cloned().collect()) + }; + let pod_spec = PodSpec { containers: self.containers.clone(), host_network: self.host_network, @@ -573,11 +579,7 @@ impl PodBuilder { }), security_context: self.security_context.clone(), tolerations: self.tolerations.clone(), - volumes: if self.volumes.is_empty() { - None - } else { - Some(self.volumes.clone().into_values().collect()) - }, + volumes, // Legacy feature for ancient Docker images // In practice, this just causes a bunch of unused environment variables that may conflict with other uses, // such as https://github.com/stackabletech/spark-operator/pull/256. From cdf5c3623ed437f266970a46b7987ca03a6b8f4b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 25 Sep 2024 11:19:25 +0200 Subject: [PATCH 16/25] Carry indivual fields in error message --- .../src/builder/pod/container.rs | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 73bc0916e..2d648a3e6 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -28,11 +28,22 @@ pub enum Error { #[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:?}" + The shared mountPath is {mount_path:?}, \ + the existing mount's volume name is {existing_volume_name:?}, \ + the existing mount's subPath is {existing_sub_path:?}, \ + the existing mount's SubPathExpr is {existing_sub_path_expr:?}, \ + the new mount's volume name is {new_volume_name:?}, \ + the new mount's subPath is {new_sub_path:?}, \ + the new mount's SubPathExpr is {new_sub_path_expr:?}" ))] - ClashingVolumeMountMountPath { - existing_volume_mount: Box, - new_volume_mount: Box, + ClashingMountPath { + mount_path: String, + existing_volume_name: String, + existing_sub_path: Option, + existing_sub_path_expr: Option, + new_volume_name: String, + new_sub_path: Option, + new_sub_path_expr: Option, }, } @@ -219,9 +230,14 @@ impl ContainerBuilder { if let Some(existing_volume_mount) = self.volume_mounts.get(&volume_mount.mount_path) { ensure!( existing_volume_mount == &volume_mount, - ClashingVolumeMountMountPathSnafu { - existing_volume_mount: existing_volume_mount.clone(), - new_volume_mount: volume_mount, + ClashingMountPathSnafu { + mount_path: volume_mount.mount_path, + existing_volume_name: existing_volume_mount.name.clone(), + existing_sub_path: existing_volume_mount.sub_path.clone(), + existing_sub_path_expr: existing_volume_mount.sub_path_expr.clone(), + new_volume_name: volume_mount.name, + new_sub_path: volume_mount.sub_path, + new_sub_path_expr: volume_mount.sub_path_expr, } ); } else { From 8727f9ab93b4145581534124c1355df3e7ad2522 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Wed, 25 Sep 2024 11:33:32 +0200 Subject: [PATCH 17/25] Reduce Error enum size by boxing ClashingMountPathDetails --- .../src/builder/pod/container.rs | 57 ++++++++++++------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 2d648a3e6..b985902d9 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -28,25 +28,38 @@ pub enum Error { #[snafu(display( "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content. \ - The shared mountPath is {mount_path:?}, \ - the existing mount's volume name is {existing_volume_name:?}, \ - the existing mount's subPath is {existing_sub_path:?}, \ - the existing mount's SubPathExpr is {existing_sub_path_expr:?}, \ - the new mount's volume name is {new_volume_name:?}, \ - the new mount's subPath is {new_sub_path:?}, \ - the new mount's SubPathExpr is {new_sub_path_expr:?}" + The shared mountPath is {:?}, \ + the existing mount's volume name is {:?}, \ + the existing mount's subPath is {:?}, \ + the existing mount's SubPathExpr is {:?}, \ + the new mount's volume name is {:?}, \ + the new mount's subPath is {:?}, \ + the new mount's SubPathExpr is {:?}", + details.mount_path, + details.existing_volume_name, + details.existing_sub_path, + details.existing_sub_path_expr, + details.new_volume_name, + details.new_sub_path, + details.new_sub_path_expr ))] ClashingMountPath { - mount_path: String, - existing_volume_name: String, - existing_sub_path: Option, - existing_sub_path_expr: Option, - new_volume_name: String, - new_sub_path: Option, - new_sub_path_expr: Option, + /// We need to box the details, so that the [`Error`] enum does not get too big + details: Box, }, } +#[derive(Debug)] +pub struct ClashingMountPathDetails { + mount_path: String, + existing_volume_name: String, + existing_sub_path: Option, + existing_sub_path_expr: Option, + new_volume_name: String, + new_sub_path: Option, + new_sub_path_expr: Option, +} + /// A builder to build [`Container`] objects. /// /// This will automatically create the necessary volumes and mounts for each `ConfigMap` which is added. @@ -231,13 +244,15 @@ impl ContainerBuilder { ensure!( existing_volume_mount == &volume_mount, ClashingMountPathSnafu { - mount_path: volume_mount.mount_path, - existing_volume_name: existing_volume_mount.name.clone(), - existing_sub_path: existing_volume_mount.sub_path.clone(), - existing_sub_path_expr: existing_volume_mount.sub_path_expr.clone(), - new_volume_name: volume_mount.name, - new_sub_path: volume_mount.sub_path, - new_sub_path_expr: volume_mount.sub_path_expr, + details: Box::new(ClashingMountPathDetails { + mount_path: volume_mount.mount_path, + existing_volume_name: existing_volume_mount.name.clone(), + existing_sub_path: existing_volume_mount.sub_path.clone(), + existing_sub_path_expr: existing_volume_mount.sub_path_expr.clone(), + new_volume_name: volume_mount.name, + new_sub_path: volume_mount.sub_path, + new_sub_path_expr: volume_mount.sub_path_expr, + }), } ); } else { From b7b43b82e9a3d4970e2e011384ace06ab8d9bcf2 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 26 Sep 2024 09:43:42 +0200 Subject: [PATCH 18/25] Revert "Reduce Error enum size by boxing ClashingMountPathDetails" This reverts commit 8727f9ab93b4145581534124c1355df3e7ad2522. --- .../src/builder/pod/container.rs | 57 +++++++------------ 1 file changed, 21 insertions(+), 36 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index b985902d9..2d648a3e6 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -28,38 +28,25 @@ pub enum Error { #[snafu(display( "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content. \ - The shared mountPath is {:?}, \ - the existing mount's volume name is {:?}, \ - the existing mount's subPath is {:?}, \ - the existing mount's SubPathExpr is {:?}, \ - the new mount's volume name is {:?}, \ - the new mount's subPath is {:?}, \ - the new mount's SubPathExpr is {:?}", - details.mount_path, - details.existing_volume_name, - details.existing_sub_path, - details.existing_sub_path_expr, - details.new_volume_name, - details.new_sub_path, - details.new_sub_path_expr + The shared mountPath is {mount_path:?}, \ + the existing mount's volume name is {existing_volume_name:?}, \ + the existing mount's subPath is {existing_sub_path:?}, \ + the existing mount's SubPathExpr is {existing_sub_path_expr:?}, \ + the new mount's volume name is {new_volume_name:?}, \ + the new mount's subPath is {new_sub_path:?}, \ + the new mount's SubPathExpr is {new_sub_path_expr:?}" ))] ClashingMountPath { - /// We need to box the details, so that the [`Error`] enum does not get too big - details: Box, + mount_path: String, + existing_volume_name: String, + existing_sub_path: Option, + existing_sub_path_expr: Option, + new_volume_name: String, + new_sub_path: Option, + new_sub_path_expr: Option, }, } -#[derive(Debug)] -pub struct ClashingMountPathDetails { - mount_path: String, - existing_volume_name: String, - existing_sub_path: Option, - existing_sub_path_expr: Option, - new_volume_name: String, - new_sub_path: Option, - new_sub_path_expr: Option, -} - /// A builder to build [`Container`] objects. /// /// This will automatically create the necessary volumes and mounts for each `ConfigMap` which is added. @@ -244,15 +231,13 @@ impl ContainerBuilder { ensure!( existing_volume_mount == &volume_mount, ClashingMountPathSnafu { - details: Box::new(ClashingMountPathDetails { - mount_path: volume_mount.mount_path, - existing_volume_name: existing_volume_mount.name.clone(), - existing_sub_path: existing_volume_mount.sub_path.clone(), - existing_sub_path_expr: existing_volume_mount.sub_path_expr.clone(), - new_volume_name: volume_mount.name, - new_sub_path: volume_mount.sub_path, - new_sub_path_expr: volume_mount.sub_path_expr, - }), + mount_path: volume_mount.mount_path, + existing_volume_name: existing_volume_mount.name.clone(), + existing_sub_path: existing_volume_mount.sub_path.clone(), + existing_sub_path_expr: existing_volume_mount.sub_path_expr.clone(), + new_volume_name: volume_mount.name, + new_sub_path: volume_mount.sub_path, + new_sub_path_expr: volume_mount.sub_path_expr, } ); } else { From e40749a298e41ca16eb5443c1e55bcafe83ec1a3 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 26 Sep 2024 09:44:58 +0200 Subject: [PATCH 19/25] remove subPathExpr --- crates/stackable-operator/src/builder/pod/container.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 2d648a3e6..d7d64c032 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -31,19 +31,15 @@ pub enum Error { The shared mountPath is {mount_path:?}, \ the existing mount's volume name is {existing_volume_name:?}, \ the existing mount's subPath is {existing_sub_path:?}, \ - the existing mount's SubPathExpr is {existing_sub_path_expr:?}, \ the new mount's volume name is {new_volume_name:?}, \ - the new mount's subPath is {new_sub_path:?}, \ - the new mount's SubPathExpr is {new_sub_path_expr:?}" + the new mount's subPath is {new_sub_path:?}" ))] ClashingMountPath { mount_path: String, existing_volume_name: String, existing_sub_path: Option, - existing_sub_path_expr: Option, new_volume_name: String, new_sub_path: Option, - new_sub_path_expr: Option, }, } @@ -234,10 +230,8 @@ impl ContainerBuilder { mount_path: volume_mount.mount_path, existing_volume_name: existing_volume_mount.name.clone(), existing_sub_path: existing_volume_mount.sub_path.clone(), - existing_sub_path_expr: existing_volume_mount.sub_path_expr.clone(), new_volume_name: volume_mount.name, new_sub_path: volume_mount.sub_path, - new_sub_path_expr: volume_mount.sub_path_expr, } ); } else { From 5a4dd0e74454bb56469fc1e7cc3e99c73fbf87cb Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 26 Sep 2024 09:47:56 +0200 Subject: [PATCH 20/25] doc comment --- crates/stackable-operator/src/builder/pod/container.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index d7d64c032..5b9d13770 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -35,6 +35,8 @@ pub enum Error { the new mount's subPath is {new_sub_path:?}" ))] ClashingMountPath { + // The VolumeMount structs where not added to avoid to many information for the users. Instead we pick the most + // relevant information only. mount_path: String, existing_volume_name: String, existing_sub_path: Option, From 8e773ee9b5ecd129019b00b7973464ae914f907d Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 26 Sep 2024 10:20:53 +0200 Subject: [PATCH 21/25] Trace error details instead of including them in the error message --- .../src/builder/pod/container.rs | 28 ++++++++++--------- .../stackable-operator/src/builder/pod/mod.rs | 24 +++++++++------- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index 5b9d13770..e98b616e3 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -28,20 +28,15 @@ pub enum Error { #[snafu(display( "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content. \ - The shared mountPath is {mount_path:?}, \ - the existing mount's volume name is {existing_volume_name:?}, \ - the existing mount's subPath is {existing_sub_path:?}, \ - the new mount's volume name is {new_volume_name:?}, \ - the new mount's subPath is {new_sub_path:?}" + The clashing mountPath is {clashing_mount_path:?}, \ + the existing mount's volume name is {existing_volume_name:?} \ + and the new mount's volume name is {new_volume_name:?}. \ + Please have a look at the log/traces for details" ))] ClashingMountPath { - // The VolumeMount structs where not added to avoid to many information for the users. Instead we pick the most - // relevant information only. - mount_path: String, + clashing_mount_path: String, existing_volume_name: String, - existing_sub_path: Option, new_volume_name: String, - new_sub_path: Option, }, } @@ -226,14 +221,21 @@ impl ContainerBuilder { /// two times, resulting in e.g. the s3 credentials being mounted twice as the same volumeMount. fn add_volume_mount_impl(&mut self, volume_mount: VolumeMount) -> Result<&mut Self> { if let Some(existing_volume_mount) = self.volume_mounts.get(&volume_mount.mount_path) { + // We don't want to include the details in the error message, but instead trace them + if existing_volume_mount != &volume_mount { + tracing::error!( + clashing_mount_path = &volume_mount.mount_path, + ?existing_volume_mount, + new_volume_mount = ?volume_mount, + "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content" + ); + } ensure!( existing_volume_mount == &volume_mount, ClashingMountPathSnafu { - mount_path: volume_mount.mount_path, + clashing_mount_path: volume_mount.mount_path, existing_volume_name: existing_volume_mount.name.clone(), - existing_sub_path: existing_volume_mount.sub_path.clone(), new_volume_name: volume_mount.name, - new_sub_path: volume_mount.sub_path, } ); } else { diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 62ed3b3bd..bc9a56e4a 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -52,13 +52,9 @@ pub enum Error { #[snafu(display("object is missing key {key:?}"))] MissingObjectKey { key: &'static str }, - #[snafu(display("The volume {volume_name:?} clashes with an existing volume of the same name but with different content. \ - The existing volume is {existing_volume:?}, the new one is {new_volume:?}"))] - ClashingVolumeName { - volume_name: String, - existing_volume: Box, - new_volume: Box, - }, + #[snafu(display("The volume {clashing_volume_name:?} clashes with an existing volume of the same name but with different content. \ + Please have a look at the log/traces for details."))] + ClashingVolumeName { clashing_volume_name: String }, } /// A builder to build [`Pod`] or [`PodTemplateSpec`] objects. @@ -292,12 +288,20 @@ impl PodBuilder { /// resulting in e.g. the s3 credentials being mounted twice as the sake volume. pub fn add_volume(&mut self, volume: Volume) -> Result<&mut Self> { if let Some(existing_volume) = self.volumes.get(&volume.name) { + // We don't want to include the details in the error message, but instead trace them + if existing_volume != &volume { + let clashing_name = &volume.name; + tracing::error!( + clashing_name, + ?existing_volume, + new_volume = ?volume, + "The volume {clashing_name:?} clashes with an existing volume of the same name but with different content", + ); + } ensure!( existing_volume == &volume, ClashingVolumeNameSnafu { - volume_name: volume.name.clone(), - existing_volume: existing_volume.clone(), - new_volume: volume + clashing_volume_name: volume.name.clone(), } ); } else { From cc6f5ae813a00ae09e697825005888fddb27b218 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 26 Sep 2024 11:20:58 +0200 Subject: [PATCH 22/25] error handling --- .../src/builder/pod/container.rs | 37 +++++++++---------- .../stackable-operator/src/builder/pod/mod.rs | 30 +++++++-------- 2 files changed, 31 insertions(+), 36 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index e98b616e3..decc9fc76 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -9,7 +9,8 @@ use k8s_openapi::api::core::v1::{ LifecycleHandler, ObjectFieldSelector, Probe, ResourceRequirements, SecretKeySelector, SecurityContext, VolumeMount, }; -use snafu::{ensure, ResultExt as _, Snafu}; +use snafu::{ResultExt as _, Snafu}; +use tracing::instrument; use crate::{ commons::product_image_selection::ResolvedProductImage, @@ -27,14 +28,11 @@ pub enum Error { }, #[snafu(display( - "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content. \ - The clashing mountPath is {clashing_mount_path:?}, \ - the existing mount's volume name is {existing_volume_name:?} \ - and the new mount's volume name is {new_volume_name:?}. \ - Please have a look at the log/traces for details" + "Colliding mountPath {mount_path:?} in volumeMounts with different content. \ + Existing volume name {existing_volume_name:?}, new volume name {new_volume_name:?}" ))] - ClashingMountPath { - clashing_mount_path: String, + MountPathCollision { + mount_path: String, existing_volume_name: String, new_volume_name: String, }, @@ -219,25 +217,24 @@ impl ContainerBuilder { /// Historically this function unconditionally added volumeMounts, which resulted in invalid /// [`k8s_openapi::api::core::v1::PodSpec`]s, as volumeMounts where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] /// two times, resulting in e.g. the s3 credentials being mounted twice as the same volumeMount. + #[instrument(skip(self))] fn add_volume_mount_impl(&mut self, volume_mount: VolumeMount) -> Result<&mut Self> { if let Some(existing_volume_mount) = self.volume_mounts.get(&volume_mount.mount_path) { - // We don't want to include the details in the error message, but instead trace them if existing_volume_mount != &volume_mount { + let colliding_mount_path = &volume_mount.mount_path; + // We don't want to include the details in the error message, but instead trace them tracing::error!( - clashing_mount_path = &volume_mount.mount_path, + colliding_mount_path, ?existing_volume_mount, - new_volume_mount = ?volume_mount, - "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content" + "Colliding mountPath {colliding_mount_path:?} in volumeMounts with different content" ); } - ensure!( - existing_volume_mount == &volume_mount, - ClashingMountPathSnafu { - clashing_mount_path: volume_mount.mount_path, - existing_volume_name: existing_volume_mount.name.clone(), - new_volume_name: volume_mount.name, - } - ); + MountPathCollisionSnafu { + mount_path: &volume_mount.mount_path, + existing_volume_name: &existing_volume_mount.name, + new_volume_name: &volume_mount.name, + } + .fail()?; } else { self.volume_mounts .insert(volume_mount.mount_path.clone(), volume_mount); diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index bc9a56e4a..9bee00d77 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -9,8 +9,8 @@ use k8s_openapi::{ }, apimachinery::pkg::{api::resource::Quantity, apis::meta::v1::ObjectMeta}, }; -use snafu::{ensure, OptionExt, ResultExt, Snafu}; -use tracing::warn; +use snafu::{OptionExt, ResultExt, Snafu}; +use tracing::{instrument, warn}; use crate::kvp::Labels; use crate::{ @@ -52,9 +52,8 @@ pub enum Error { #[snafu(display("object is missing key {key:?}"))] MissingObjectKey { key: &'static str }, - #[snafu(display("The volume {clashing_volume_name:?} clashes with an existing volume of the same name but with different content. \ - Please have a look at the log/traces for details."))] - ClashingVolumeName { clashing_volume_name: String }, + #[snafu(display("Colliding volume name {volume_name:?} in volumes with different content"))] + VolumeNameCollision { volume_name: String }, } /// A builder to build [`Pod`] or [`PodTemplateSpec`] objects. @@ -286,24 +285,23 @@ impl PodBuilder { /// Historically this function unconditionally added volumes, which resulted in invalid [`PodSpec`]s, as volumes /// where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] two times, /// resulting in e.g. the s3 credentials being mounted twice as the sake volume. + #[instrument(skip(self))] pub fn add_volume(&mut self, volume: Volume) -> Result<&mut Self> { if let Some(existing_volume) = self.volumes.get(&volume.name) { - // We don't want to include the details in the error message, but instead trace them if existing_volume != &volume { - let clashing_name = &volume.name; + let colliding_name = &volume.name; + // We don't want to include the details in the error message, but instead trace them tracing::error!( - clashing_name, + colliding_name, ?existing_volume, - new_volume = ?volume, - "The volume {clashing_name:?} clashes with an existing volume of the same name but with different content", + "Colliding volume name {colliding_name:?} in volumes with different content" ); - } - ensure!( - existing_volume == &volume, - ClashingVolumeNameSnafu { - clashing_volume_name: volume.name.clone(), + + VolumeNameCollisionSnafu { + volume_name: &volume.name, } - ); + .fail()?; + } } else { self.volumes.insert(volume.name.clone(), volume); } From 58c7092325059a4ef23272f6bc63b9a51dd27304 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 26 Sep 2024 11:40:36 +0200 Subject: [PATCH 23/25] docs :P --- .../src/builder/pod/container.rs | 35 ++++++++++++++----- .../stackable-operator/src/builder/pod/mod.rs | 15 ++++---- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/crates/stackable-operator/src/builder/pod/container.rs b/crates/stackable-operator/src/builder/pod/container.rs index decc9fc76..e24071da1 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -1,7 +1,7 @@ use std::fmt; #[cfg(doc)] -use std::collections::BTreeMap; +use {k8s_openapi::api::core::v1::PodSpec, std::collections::BTreeMap}; use indexmap::IndexMap; use k8s_openapi::api::core::v1::{ @@ -210,13 +210,17 @@ impl ContainerBuilder { self } - /// This function only adds the [`VolumeMount`] in case there is no volumeMount with the same mountPath already. - /// In case there already was a volumeMount with the same path already, an [`tracing::error`] is raised in case the - /// contents of the volumeMounts differ. + /// Adds a new [`VolumeMount`] to the container while ensuring that no colliding [`VolumeMount`] + /// exists. /// - /// Historically this function unconditionally added volumeMounts, which resulted in invalid - /// [`k8s_openapi::api::core::v1::PodSpec`]s, as volumeMounts where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] - /// two times, resulting in e.g. the s3 credentials being mounted twice as the same volumeMount. + /// A colliding [`VolumeMount`] would have the same mountPath but a different content than + /// another [`VolumeMount`]. An appropriate error is returned when such a clashing mount path is + /// encountered. + /// + /// ### Note + /// + /// Previously, this function unconditionally added [`VolumeMount`]s, which resulted in invalid + /// [`PodSpec`]s. #[instrument(skip(self))] fn add_volume_mount_impl(&mut self, volume_mount: VolumeMount) -> Result<&mut Self> { if let Some(existing_volume_mount) = self.volume_mounts.get(&volume_mount.mount_path) { @@ -243,7 +247,17 @@ impl ContainerBuilder { Ok(self) } - /// See [`Self::add_volume_mount_impl`] for details + /// Adds a new [`VolumeMount`] to the container while ensuring that no colliding [`VolumeMount`] + /// exists. + /// + /// A colliding [`VolumeMount`] would have the same mountPath but a different content than + /// another [`VolumeMount`]. An appropriate error is returned when such a clashing mount path is + /// encountered. + /// + /// ### Note + /// + /// Previously, this function unconditionally added [`VolumeMount`]s, which resulted in invalid + /// [`PodSpec`]s. pub fn add_volume_mount( &mut self, name: impl Into, @@ -256,7 +270,10 @@ impl ContainerBuilder { }) } - /// See [`Self::add_volume_mount_impl`] for details + /// Adds new [`VolumeMount`]s to the container while ensuring that no colliding [`VolumeMount`] + /// exists. + /// + /// See [`Self::add_volume_mount`] for details. pub fn add_volume_mounts( &mut self, volume_mounts: impl IntoIterator, diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 9bee00d77..535f1bb6d 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -278,13 +278,16 @@ impl PodBuilder { ) } - /// This function only adds the [`Volume`] in case there is no volume with the same name already. - /// In case there already was a volume with the same name already, an [`tracing::error`] is raised in case the - /// contents of the volumes differ. + /// Adds a new [`Volume`] to the container while ensuring that no colliding [`Volume`] exists. /// - /// Historically this function unconditionally added volumes, which resulted in invalid [`PodSpec`]s, as volumes - /// where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] two times, - /// resulting in e.g. the s3 credentials being mounted twice as the sake volume. + /// A colliding [`Volume`] would have the same name but a different content than another + /// [`Volume`]. An appropriate error is returned when such a clashing volume name is + /// encountered. + /// + /// ### Note + /// + /// Previously, this function unconditionally added [`Volume`]s, which resulted in invalid + /// [`PodSpec`]s. #[instrument(skip(self))] pub fn add_volume(&mut self, volume: Volume) -> Result<&mut Self> { if let Some(existing_volume) = self.volumes.get(&volume.name) { From 246f0c8105c8a0136dcdc84277acd6deccd5661a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 26 Sep 2024 11:42:56 +0200 Subject: [PATCH 24/25] Revert "expect" This reverts commit 53ef278de66ca07c9680810230235ee7a8b260a9. --- crates/stackable-operator/src/builder/pod/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 535f1bb6d..9d0e75ba5 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -724,7 +724,7 @@ mod tests { .with_config_map("configmap") .build(), ) - .expect("Add volume") + .unwrap() .termination_grace_period(&Duration::from_secs(42)) .unwrap() .build() From 988615542a2e9afc4751fee92def6f1a2779c57f Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 26 Sep 2024 11:45:14 +0200 Subject: [PATCH 25/25] clashing -> colliding --- crates/stackable-operator/CHANGELOG.md | 2 +- crates/stackable-operator/src/builder/pod/container.rs | 4 ++-- crates/stackable-operator/src/builder/pod/mod.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index a73643523..a85e5a8d0 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -7,7 +7,7 @@ All notable changes to this project will be documented in this file. ### Fixed - Fix the logback configuration for logback versions from 1.3.6/1.4.6 to 1.3.11/1.4.11 ([#874]). -- 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]). +- BREAKING: Avoid colliding 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 index e24071da1..8357493ac 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -214,7 +214,7 @@ impl ContainerBuilder { /// exists. /// /// A colliding [`VolumeMount`] would have the same mountPath but a different content than - /// another [`VolumeMount`]. An appropriate error is returned when such a clashing mount path is + /// another [`VolumeMount`]. An appropriate error is returned when such a colliding mount path is /// encountered. /// /// ### Note @@ -251,7 +251,7 @@ impl ContainerBuilder { /// exists. /// /// A colliding [`VolumeMount`] would have the same mountPath but a different content than - /// another [`VolumeMount`]. An appropriate error is returned when such a clashing mount path is + /// another [`VolumeMount`]. An appropriate error is returned when such a colliding mount path is /// encountered. /// /// ### Note diff --git a/crates/stackable-operator/src/builder/pod/mod.rs b/crates/stackable-operator/src/builder/pod/mod.rs index 9d0e75ba5..fb5bda51d 100644 --- a/crates/stackable-operator/src/builder/pod/mod.rs +++ b/crates/stackable-operator/src/builder/pod/mod.rs @@ -281,7 +281,7 @@ impl PodBuilder { /// Adds a new [`Volume`] to the container while ensuring that no colliding [`Volume`] exists. /// /// A colliding [`Volume`] would have the same name but a different content than another - /// [`Volume`]. An appropriate error is returned when such a clashing volume name is + /// [`Volume`]. An appropriate error is returned when such a colliding volume name is /// encountered. /// /// ### Note