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/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 921aa57f0..a85e5a8d0 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -6,9 +6,14 @@ 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]). +- Fix the logback configuration for logback versions from 1.3.6/1.4.6 to 1.3.11/1.4.11 ([#874]). +- 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 + +- 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 [#874]: https://github.com/stackabletech/operator-rs/pull/874 ## [0.76.0] - 2024-09-19 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 be89d8044..8357493ac 100644 --- a/crates/stackable-operator/src/builder/pod/container.rs +++ b/crates/stackable-operator/src/builder/pod/container.rs @@ -1,11 +1,16 @@ use std::fmt; +#[cfg(doc)] +use {k8s_openapi::api::core::v1::PodSpec, std::collections::BTreeMap}; + +use indexmap::IndexMap; use k8s_openapi::api::core::v1::{ ConfigMapKeySelector, Container, ContainerPort, EnvVar, EnvVarSource, Lifecycle, LifecycleHandler, ObjectFieldSelector, Probe, ResourceRequirements, SecretKeySelector, SecurityContext, VolumeMount, }; use snafu::{ResultExt as _, Snafu}; +use tracing::instrument; use crate::{ commons::product_image_selection::ResolvedProductImage, @@ -21,11 +26,26 @@ pub enum Error { source: validation::Errors, container_name: String, }, + + #[snafu(display( + "Colliding mountPath {mount_path:?} in volumeMounts with different content. \ + Existing volume name {existing_volume_name:?}, new volume name {new_volume_name:?}" + ))] + MountPathCollision { + mount_path: String, + existing_volume_name: String, + new_volume_name: String, + }, } /// 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 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)] pub struct ContainerBuilder { args: Option>, @@ -36,7 +56,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,29 +210,79 @@ impl ContainerBuilder { self } + /// 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 colliding 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) { + 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!( + colliding_mount_path, + ?existing_volume_mount, + "Colliding mountPath {colliding_mount_path:?} in volumeMounts with different content" + ); + } + 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); + } + + Ok(self) + } + + /// 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 colliding 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, 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 + ) -> Result<&mut Self> { + self.add_volume_mount_impl(VolumeMount { + name: name.into(), + mount_path: path.into(), + ..VolumeMount::default() + }) } + /// 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, - ) -> &mut Self { - self.volume_mounts - .get_or_insert_with(Vec::new) - .extend(volume_mounts); - self + ) -> Result<&mut Self> { + for volume_mount in volume_mounts { + self.add_volume_mount_impl(volume_mount)?; + } + + Ok(self) } pub fn readiness_probe(&mut self, probe: Probe) -> &mut Self { @@ -256,6 +328,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(), @@ -265,7 +343,7 @@ impl ContainerBuilder { resources: self.resources.clone(), name: self.name.clone(), ports: self.container_ports.clone(), - volume_mounts: self.volume_mounts.clone(), + volume_mounts, readiness_probe: self.readiness_probe.clone(), liveness_probe: self.liveness_probe.clone(), startup_probe: self.startup_probe.clone(), @@ -388,6 +466,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") + .expect("add volume mount") .add_container_port(container_port_name, container_port) .resources(resources.clone()) .add_container_ports(vec![ContainerPortBuilder::new(container_port_1) @@ -491,20 +570,18 @@ 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 + if let Error::InvalidContainerName { + container_name, + source, + } = result .err() .expect("Container name exceeding 63 characters should cause an error") { - 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" - ) - } + 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(); @@ -568,16 +645,14 @@ mod tests { result: Result, expected_err_contains: &str, ) { - match result + if let Error::InvalidContainerName { + container_name: _, + source, + } = result .err() .expect("Container name exceeding 63 characters should cause an error") { - Error::InvalidContainerName { - container_name: _, - source, - } => { - assert!(dbg!(source.to_string()).contains(dbg!(expected_err_contains))); - } + 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 4225feba0..fb5bda51d 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, @@ -9,7 +10,7 @@ use k8s_openapi::{ apimachinery::pkg::{api::resource::Quantity, apis::meta::v1::ObjectMeta}, }; use snafu::{OptionExt, ResultExt, Snafu}; -use tracing::warn; +use tracing::{instrument, warn}; use crate::kvp::Labels; use crate::{ @@ -50,9 +51,16 @@ pub enum Error { #[snafu(display("object is missing key {key:?}"))] MissingObjectKey { key: &'static str }, + + #[snafu(display("Colliding volume name {volume_name:?} in volumes with different content"))] + VolumeNameCollision { volume_name: String }, } /// 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 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 { containers: Vec, @@ -67,7 +75,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,18 +264,13 @@ 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( &mut self, name: impl Into, quantity: Option, - ) -> &mut Self { + ) -> Result<&mut Self> { self.add_volume( VolumeBuilder::new(name) .with_empty_dir(None::, quantity) @@ -273,9 +278,47 @@ impl PodBuilder { ) } - pub fn add_volumes(&mut self, volumes: Vec) -> &mut Self { - self.volumes.get_or_insert_with(Vec::new).extend(volumes); - self + /// 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 colliding 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) { + if existing_volume != &volume { + let colliding_name = &volume.name; + // We don't want to include the details in the error message, but instead trace them + tracing::error!( + colliding_name, + ?existing_volume, + "Colliding volume name {colliding_name:?} in volumes with different content" + ); + + VolumeNameCollisionSnafu { + volume_name: &volume.name, + } + .fail()?; + } + } else { + self.volumes.insert(volume.name.clone(), volume); + } + + Ok(self) + } + + /// See [`Self::add_volume`] for details + pub fn add_volumes(&mut self, volumes: Vec) -> Result<&mut Self> { + for volume in volumes { + self.add_volume(volume)?; + } + + Ok(self) } /// Add a [`Volume`] for the storage class `listeners.stackable.tech` with the given listener @@ -304,6 +347,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) @@ -359,7 +403,7 @@ impl PodBuilder { name: volume_name.into(), ephemeral: Some(volume), ..Volume::default() - }); + })?; Ok(self) } @@ -390,6 +434,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) @@ -445,7 +490,7 @@ impl PodBuilder { name: volume_name.into(), ephemeral: Some(volume), ..Volume::default() - }); + })?; Ok(self) } @@ -520,6 +565,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, @@ -533,7 +584,7 @@ impl PodBuilder { }), security_context: self.security_context.clone(), tolerations: self.tolerations.clone(), - volumes: self.volumes.clone(), + 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. @@ -673,6 +724,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 d0f066af4..2657c05e4 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 required volumes"))] + AddVolumes { source: builder::pod::Error }, + + #[snafu(display("failed to add required 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 c9b8c7b7c..c43068734 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)] @@ -79,19 +79,16 @@ 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)?; - pod_builder.add_volumes(volumes); + let (volumes, mounts) = self.volumes_and_mounts()?; + 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(()) @@ -99,16 +96,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 +110,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 +128,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 +202,7 @@ mod tests { 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 +227,7 @@ mod tests { }), }, }; - 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 +238,7 @@ mod tests { 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 +256,12 @@ mod tests { ); 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 +279,7 @@ mod tests { }), }, }; - let (volumes, mounts) = s3.volumes_and_mounts("lakehouse").unwrap(); + let (volumes, mounts) = s3.volumes_and_mounts().unwrap(); assert_eq!( s3.endpoint().unwrap(), diff --git a/crates/stackable-operator/src/commons/s3/mod.rs b/crates/stackable-operator/src/commons/s3/mod.rs index c7f36c6eb..ecd6b87d9 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 required volumes"))] + AddVolumes { source: builder::pod::Error }, + + #[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 a29695a95..0123a6bad 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 required volumes"))] + AddVolumes { source: builder::pod::Error }, + + #[snafu(display("failed to add required 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 7913e9e2b..5acfa2deb 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 required 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(); /// ``` @@ -1410,7 +1426,7 @@ sinks: /// "log", /// logging.containers.get(&Container::Vector), /// resources, -/// )); +/// ).unwrap()); /// } /// /// pod_builder.build().unwrap(); @@ -1421,7 +1437,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 @@ -1431,8 +1447,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(), @@ -1459,9 +1475,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. @@ -1489,6 +1507,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 {