diff --git a/CHANGELOG.md b/CHANGELOG.md index 839c88e8..bc484788 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ All notable changes to this project will be documented in this file. StatefulSets created by older operator versions cannot be updated in place: after the operator upgrade, delete each broker, coordinator and router StatefulSet so that the operator immediately recreates it with the new labels ([#865]). +- Make operations infallible where dependent on static inputs ([#869]). ### Fixed @@ -41,6 +42,7 @@ All notable changes to this project will be documented in this file. [#860]: https://github.com/stackabletech/druid-operator/pull/860 [#865]: https://github.com/stackabletech/druid-operator/pull/865 [#867]: https://github.com/stackabletech/druid-operator/pull/867 +[#869]: https://github.com/stackabletech/druid-operator/pull/869 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index 969dcd4b..d8d50cdc 100644 --- a/rust/operator-binary/src/controller/build/resource/listener.rs +++ b/rust/operator-binary/src/controller/build/resource/listener.rs @@ -13,7 +13,7 @@ use stackable_operator::{ }, types::{ kubernetes::{ListenerClassName, ListenerName, PersistentVolumeClaimName}, - operator::ClusterName, + operator::{ClusterName, RoleName}, }, }, }; @@ -90,11 +90,16 @@ pub fn general_group_listener_name( cluster_name: &ClusterName, druid_role: &DruidRole, ) -> ListenerName { - ListenerName::from_str(&format!( - "{cluster_name}-{druid_role}", - druid_role = druid_role.as_ref() - )) - .expect("a valid listener name") + const _: () = assert!( + ClusterName::MAX_LENGTH + 1 /* dash */ + RoleName::MAX_LENGTH <= ListenerName::MAX_LENGTH, + "The string `-` must not exceed the limit of Listener names." + ); + // Both halves are RFC 1123 labels joined by a dash, which is a valid RFC 1123 subdomain. + let _ = ClusterName::IS_RFC_1123_SUBDOMAIN_NAME; + let _ = RoleName::IS_RFC_1123_LABEL_NAME; + + let role_name: &RoleName = druid_role; + ListenerName::from_str(&format!("{cluster_name}-{role_name}")).expect("a valid listener name") } /// The connection string (`
:`) for the given ingress address, or `None` when the diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 4adca08d..b92e7c85 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -164,19 +164,11 @@ pub fn build_rolegroup_statefulset( } prepare_container_commands.extend(build_tls_key_stores_cmd(druid_tls_security)); - if let Some(auth_config) = druid_auth_config { - authentication::add_volumes_and_mounts( - auth_config, - &mut pb, - &mut cb_druid, - &mut cb_prepare, - ) - .context(AuthVolumesBuildSnafu)?; - prepare_container_commands.extend(authentication::prepare_container_commands(auth_config)); - main_container_commands.extend(authentication::main_container_commands(auth_config)) - } - - // volume and volume mounts + // Operator-managed volumes and volume mounts first. Their mount paths are constants, so the + // mounts cannot collide with each other and adding them is infallible. Adding the volumes + // stays fallible, because the volumes are built from computed arguments. Volumes and mounts + // derived from user input (authentication, S3, `extraVolumes`) are added afterwards and stay + // fallible, as they can collide with the operator-managed ones. add_tls_volume_and_volume_mounts( druid_tls_security, &mut cb_prepare, @@ -187,15 +179,6 @@ pub fn build_rolegroup_statefulset( secret_volume_listener_scope(role), ) .context(FailedToInitializeSecurityContextSnafu)?; - - if let Some(s3) = s3_conn { - if s3.tls.uses_tls() && !s3.tls.uses_tls_verification() { - S3TlsNoVerificationNotSupportedSnafu.fail()?; - } - s3.add_volumes_and_mounts(&mut pb, vec![&mut cb_druid]) - .context(ConfigureS3Snafu)?; - } - add_config_volume_and_volume_mounts(&resource_names, &mut cb_druid, &mut pb)?; add_log_config_volume_and_volume_mounts( &resource_names, @@ -214,6 +197,45 @@ pub fn build_rolegroup_statefulset( .update_volumes_and_volume_mounts(&mut cb_druid, &mut pb) .context(UpdateDruidConfigFromResourcesSnafu)?; + // The listener volume mount is static as well, so it belongs here, before the derived volumes + // and mounts below. The listener volume itself is a PVC template, see `pvcs`. + let mut pvcs: Option> = None; + if let Some(group_listener_name) = group_listener_name(&cluster.name, role) { + cb_druid + .add_volume_mount(&*LISTENER_VOLUME_NAME, LISTENER_VOLUME_DIR) + .expect("The mount paths are statically defined and there should be no duplicates."); + + // Used for PVC templates, which cannot be modified once they are deployed. The version + // label is omitted so the labels stay stable across version upgrades. + let unversioned_recommended_labels = + recommended_labels_for_unversioned_role_group_resources(cluster, role, role_group_name); + + pvcs = Some(vec![build_group_listener_pvc( + &group_listener_name, + &unversioned_recommended_labels, + )]); + } + + if let Some(auth_config) = druid_auth_config { + authentication::add_volumes_and_mounts( + auth_config, + &mut pb, + &mut cb_druid, + &mut cb_prepare, + ) + .context(AuthVolumesBuildSnafu)?; + prepare_container_commands.extend(authentication::prepare_container_commands(auth_config)); + main_container_commands.extend(authentication::main_container_commands(auth_config)) + } + + if let Some(s3) = s3_conn { + if s3.tls.uses_tls() && !s3.tls.uses_tls_verification() { + S3TlsNoVerificationNotSupportedSnafu.fail()?; + } + s3.add_volumes_and_mounts(&mut pb, vec![&mut cb_druid]) + .context(ConfigureS3Snafu)?; + } + cb_prepare .image_from_product_image(resolved_product_image) .command(vec![ @@ -312,24 +334,6 @@ pub fn build_rolegroup_statefulset( .context(AddVolumeMountSnafu)?; } - let mut pvcs: Option> = None; - - if let Some(group_listener_name) = group_listener_name(&cluster.name, role) { - cb_druid - .add_volume_mount(&*LISTENER_VOLUME_NAME, LISTENER_VOLUME_DIR) - .context(AddVolumeMountSnafu)?; - - // Used for PVC templates, which cannot be modified once they are deployed. The version - // label is omitted so the labels stay stable across version upgrades. - let unversioned_recommended_labels = - recommended_labels_for_unversioned_role_group_resources(cluster, role, role_group_name); - - pvcs = Some(vec![build_group_listener_pvc( - &group_listener_name, - &unversioned_recommended_labels, - )]); - } - let metadata = ObjectMetaBuilder::new() .with_labels(recommended_labels_for_role_group_resources( cluster, @@ -398,6 +402,12 @@ pub fn build_rolegroup_statefulset( }) } +/// Adds the HDFS discovery ConfigMap volume and its mount if HDFS deep storage is configured. +/// +/// # Panics +/// +/// Panics if the volume mounts cannot be added to the container builder. Only call this on a +/// container builder whose mount paths are still distinct from the ones added here. fn add_hdfs_cm_volume_and_volume_mounts( deep_storage_spec: &DeepStorageSpec, cb_druid: &mut ContainerBuilder, @@ -407,7 +417,7 @@ fn add_hdfs_cm_volume_and_volume_mounts( if let DeepStorageSpec::Hdfs(hdfs) = deep_storage_spec { cb_druid .add_volume_mount(&*HDFS_CONFIG_VOLUME_NAME, HDFS_CONFIG_DIRECTORY) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); pb.add_volume( VolumeBuilder::new(&*HDFS_CONFIG_VOLUME_NAME) .with_config_map(hdfs.config_map_name.to_string()) @@ -419,6 +429,12 @@ fn add_hdfs_cm_volume_and_volume_mounts( Ok(()) } +/// Adds the role group ConfigMap volume, the writable config volume and their mounts. +/// +/// # Panics +/// +/// Panics if the volume mounts cannot be added to the container builder. Only call this on a +/// container builder whose mount paths are still distinct from the ones added here. fn add_config_volume_and_volume_mounts( resource_names: &ResourceNames, cb_druid: &mut ContainerBuilder, @@ -426,7 +442,7 @@ fn add_config_volume_and_volume_mounts( ) -> Result<()> { cb_druid .add_volume_mount(&*DRUID_CONFIG_VOLUME_NAME, DRUID_CONFIG_DIRECTORY) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); pb.add_volume( VolumeBuilder::new(&*DRUID_CONFIG_VOLUME_NAME) .with_config_map(resource_names.role_group_config_map().to_string()) @@ -435,7 +451,7 @@ fn add_config_volume_and_volume_mounts( .context(AddVolumeSnafu)?; cb_druid .add_volume_mount(&*RW_CONFIG_VOLUME_NAME, RW_CONFIG_DIRECTORY) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); pb.add_volume( VolumeBuilder::new(&*RW_CONFIG_VOLUME_NAME) .with_empty_dir(Some(""), None) @@ -446,6 +462,12 @@ fn add_config_volume_and_volume_mounts( Ok(()) } +/// Adds the log config ConfigMap volume and its mount. +/// +/// # Panics +/// +/// Panics if the volume mounts cannot be added to the container builder. Only call this on a +/// container builder whose mount paths are still distinct from the ones added here. fn add_log_config_volume_and_volume_mounts( resource_names: &ResourceNames, merged_rolegroup_config: &ValidatedDruidConfig, @@ -454,7 +476,7 @@ fn add_log_config_volume_and_volume_mounts( ) -> Result<()> { cb_druid .add_volume_mount(&*LOG_CONFIG_VOLUME_NAME, LOG_CONFIG_DIRECTORY) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); let config_map = match &merged_rolegroup_config.logging.druid_container { ValidatedContainerLogConfigChoice::Custom(config_map_name) => config_map_name.to_string(), @@ -473,6 +495,12 @@ fn add_log_config_volume_and_volume_mounts( Ok(()) } +/// Adds the log volume and its mounts on the druid and prepare containers. +/// +/// # Panics +/// +/// Panics if the volume mounts cannot be added to the container builders. Only call this on +/// container builders whose mount paths are still distinct from the ones added here. fn add_log_volume_and_volume_mounts( cb_druid: &mut ContainerBuilder, cb_prepare: &mut ContainerBuilder, @@ -480,10 +508,10 @@ fn add_log_volume_and_volume_mounts( ) -> Result<()> { cb_druid .add_volume_mount(&*LOG_VOLUME_NAME, STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); cb_prepare .add_volume_mount(&*LOG_VOLUME_NAME, STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); pb.add_volume( VolumeBuilder::new(&*LOG_VOLUME_NAME) .with_empty_dir( @@ -501,7 +529,10 @@ fn add_log_volume_and_volume_mounts( #[cfg(test)] mod tests { - use stackable_operator::v2::types::operator::RoleGroupName; + use stackable_operator::{ + k8s_openapi::api::core::v1::{ConfigMapVolumeSource, Volume}, + v2::types::operator::RoleGroupName, + }; use super::*; use crate::controller::validate::test_support::{ @@ -567,4 +598,50 @@ mod tests { ); assert_eq!(containerdebug[0].value.as_deref(), Some("/custom/log/dir")); } + + /// A user-supplied extra volume whose name collides with an operator-managed volume must be + /// reported as an error (the operator's own volumes are added first and are infallible, so + /// the collision must surface on the user-supplied side, never as a panic). + #[test] + fn extra_volume_colliding_with_operator_volume_is_an_error() { + let mut druid = druid_from_yaml(MINIMAL_DRUID_YAML); + druid.spec.cluster_config.extra_volumes = vec![user_volume(LOG_VOLUME_NAME.as_ref())]; + let cluster = validated_cluster(&druid); + let role_group_name = RoleGroupName::from_str("default").expect("valid role group name"); + let rg = broker_default_role_group(&cluster, &role_group_name); + + let Err(error) = + build_rolegroup_statefulset(&cluster, &DruidRole::Broker, &role_group_name, &rg) + else { + panic!("the colliding extra volume must be rejected"); + }; + assert!( + matches!(error, Error::AddVolume { .. }), + "unexpected error: {error:?}" + ); + } + + fn user_volume(name: &str) -> Volume { + Volume { + name: name.to_owned(), + config_map: Some(ConfigMapVolumeSource { + name: "user-cm".to_owned(), + ..ConfigMapVolumeSource::default() + }), + ..Volume::default() + } + } + + fn broker_default_role_group( + cluster: &ValidatedCluster, + role_group_name: &RoleGroupName, + ) -> DruidRoleGroupConfig { + cluster + .role_group_configs + .get(&DruidRole::Broker) + .expect("broker role groups") + .get(role_group_name) + .expect("default role group") + .clone() + } } diff --git a/rust/operator-binary/src/controller/build/security.rs b/rust/operator-binary/src/controller/build/security.rs index 61b13505..547b7885 100644 --- a/rust/operator-binary/src/controller/build/security.rs +++ b/rust/operator-binary/src/controller/build/security.rs @@ -44,11 +44,6 @@ pub enum Error { #[snafu(display("failed to add needed volume"))] AddVolume { source: builder::pod::Error }, - - #[snafu(display("failed to add needed volumeMount"))] - AddVolumeMount { - source: builder::pod::container::Error, - }, } // Ports @@ -137,6 +132,11 @@ fn exposed_port(tls: &DruidTlsSecurity, role: &DruidRole) -> (&'static str, Port /// Adds required tls volume mounts to image and product container builders /// Adds required tls volumes to pod builder +/// +/// # Panics +/// +/// Panics if the volume mounts cannot be added to the container builders. Only call this on +/// container builders whose mount paths are still distinct from the ones added here. pub fn add_tls_volume_and_volume_mounts( tls: &DruidTlsSecurity, prepare: &mut ContainerBuilder, @@ -175,10 +175,10 @@ pub fn add_tls_volume_and_volume_mounts( .context(AddVolumeSnafu)?; prepare .add_volume_mount(&*TLS_MOUNT_VOLUME_NAME, STACKABLE_MOUNT_TLS_DIR) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); druid .add_volume_mount(&*TLS_MOUNT_VOLUME_NAME, STACKABLE_MOUNT_TLS_DIR) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); pod.add_volume( VolumeBuilder::new(&*TLS_VOLUME_NAME) @@ -189,10 +189,10 @@ pub fn add_tls_volume_and_volume_mounts( prepare .add_volume_mount(&*TLS_VOLUME_NAME, STACKABLE_TLS_DIR) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); druid .add_volume_mount(&*TLS_VOLUME_NAME, STACKABLE_TLS_DIR) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); } Ok(()) } diff --git a/rust/operator-binary/src/crd/authentication.rs b/rust/operator-binary/src/crd/authentication.rs index 6b201502..d0125789 100644 --- a/rust/operator-binary/src/crd/authentication.rs +++ b/rust/operator-binary/src/crd/authentication.rs @@ -205,8 +205,8 @@ impl AuthenticationClassesResolved { None => { info!( "No OIDC provider hint given in AuthClass {auth_class_name}, assuming {default_oidc_provider_name}", - default_oidc_provider_name = - serde_json::to_string(&DEFAULT_OIDC_PROVIDER).unwrap() + default_oidc_provider_name = serde_json::to_string(&DEFAULT_OIDC_PROVIDER) + .expect("an IdentityProviderHint serialises to a plain JSON string") ); DEFAULT_OIDC_PROVIDER } @@ -217,7 +217,8 @@ impl AuthenticationClassesResolved { SUPPORTED_OIDC_PROVIDERS.contains(&oidc_provider), OidcProviderNotSupportedSnafu { auth_class_name, - oidc_provider: serde_json::to_string(&oidc_provider).unwrap(), + oidc_provider: serde_json::to_string(&oidc_provider) + .expect("an IdentityProviderHint serialises to a plain JSON string"), } ); diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 2d0bdd34..0ebf8b23 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -156,12 +156,6 @@ pub enum Error { #[snafu(display("missing secret lifetime"))] MissingSecretLifetime, - #[snafu(display("the role group {rolegroup_name} is not defined"))] - CannotRetrieveRoleGroup { rolegroup_name: String }, - - #[snafu(display("fragment validation failure"))] - FragmentValidationFailure { source: ValidationError }, - #[snafu(display("failed to merge and validate config for role group {role_group:?}"))] FailedToMergeRoleGroupConfig { source: ValidationError, @@ -583,8 +577,12 @@ impl Default for v1alpha1::DruidRoleConfig { } } +constant!(DRUID_DEFAULT_LISTENER_CLASS: ListenerClassName = "cluster-internal"); + +/// Serde default for `listenerClass`. Kept as a function because `#[serde(default = "...")]` +/// requires a function path. fn druid_default_listener_class() -> ListenerClassName { - ListenerClassName::from_str("cluster-internal").expect("a valid listener class name") + DRUID_DEFAULT_LISTENER_CLASS.clone() } constant!(COORDINATOR_ROLE_NAME: RoleName = "coordinator"); @@ -917,6 +915,7 @@ mod tests { let _ = *ROUTER_ROLE_NAME; let _ = *COOKIE_PASSPHRASE_ENV; let _ = *COOKIE_PASSPHRASE_SECRET_KEY; + let _ = *DRUID_DEFAULT_LISTENER_CLASS; } impl RoundtripTestData for v1alpha1::DruidClusterSpec { diff --git a/rust/operator-binary/src/crd/resource.rs b/rust/operator-binary/src/crd/resource.rs index 5e1c384b..9512afc5 100644 --- a/rust/operator-binary/src/crd/resource.rs +++ b/rust/operator-binary/src/crd/resource.rs @@ -1,4 +1,4 @@ -use std::{collections::BTreeMap, sync::LazyLock}; +use std::{collections::BTreeMap, str::FromStr, sync::LazyLock}; use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{ @@ -8,11 +8,13 @@ use stackable_operator::{ CpuLimitsFragment, MemoryLimitsFragment, NoRuntimeLimits, NoRuntimeLimitsFragment, Resources, ResourcesFragment, }, + constant, k8s_openapi::{ api::core::v1::{EmptyDirVolumeSource, ResourceRequirements}, apimachinery::pkg::api::resource::Quantity, }, memory::MemoryQuantity, + v2::types::kubernetes::VolumeName, }; use crate::crd::{ @@ -24,7 +26,7 @@ use crate::crd::{ const PATH_SEGMENT_CACHE: &str = "/stackable/var/druid/segment-cache"; // volume names -const SEGMENT_CACHE_VOLUME_NAME: &str = "segment-cache"; +constant!(SEGMENT_CACHE_VOLUME_NAME: VolumeName = "segment-cache"); /// This Error cannot derive PartialEq because fragment::ValidationError doesn't derive it #[derive(Snafu, Debug)] @@ -46,11 +48,6 @@ pub enum Error { #[snafu(display("failed to add needed volume"))] AddVolume { source: builder::pod::Error }, - - #[snafu(display("failed to add needed volumeMount"))] - AddVolumeMount { - source: builder::pod::container::Error, - }, } #[derive(Debug, Clone, PartialEq)] @@ -95,16 +92,24 @@ impl RoleResource { Ok(()) } + /// Adds the segment cache volume and its mount for the Historical role. + /// + /// # Panics + /// + /// Panics if the volume mount cannot be added to the container builder. Only call this on a + /// container builder whose mount paths are still distinct from the one added here. pub fn update_volumes_and_volume_mounts( &self, cb: &mut ContainerBuilder, pb: &mut PodBuilder, ) -> Result<(), Error> { if let Self::Historical(r) = self { - cb.add_volume_mount(SEGMENT_CACHE_VOLUME_NAME, PATH_SEGMENT_CACHE) - .context(AddVolumeMountSnafu)?; + cb.add_volume_mount(&*SEGMENT_CACHE_VOLUME_NAME, PATH_SEGMENT_CACHE) + .expect( + "The mount paths are statically defined and there should be no duplicates.", + ); pb.add_volume( - VolumeBuilder::new(SEGMENT_CACHE_VOLUME_NAME) + VolumeBuilder::new(&*SEGMENT_CACHE_VOLUME_NAME) .empty_dir(EmptyDirVolumeSource { medium: r.storage.segment_cache.empty_dir.medium.clone(), size_limit: Some(r.storage.segment_cache.empty_dir.capacity.clone()), @@ -260,6 +265,12 @@ mod test { v1alpha1, }; + #[test] + fn test_constants() { + // Test that dereferencing the constants does not panic. + let _ = *SEGMENT_CACHE_VOLUME_NAME; + } + #[rstest] #[case( Some(ResourcesFragment{ diff --git a/rust/operator-binary/src/internal_secret.rs b/rust/operator-binary/src/internal_secret.rs index eaeac691..10c235e8 100644 --- a/rust/operator-binary/src/internal_secret.rs +++ b/rust/operator-binary/src/internal_secret.rs @@ -7,8 +7,16 @@ use std::str::FromStr; use stackable_operator::v2::types::{kubernetes::SecretName, operator::ClusterName}; pub fn build_shared_internal_secret_name(cluster_name: &ClusterName) -> SecretName { - SecretName::from_str(&format!("{cluster_name}-shared-internal-secret")).expect( - "the shared internal secret name is a valid Secret name, because a ClusterName is at \ - most 40 characters long, so the suffixed name stays within the length limit", - ) + const SUFFIX: &str = "-shared-internal-secret"; + const _: () = assert!( + ClusterName::MAX_LENGTH + SUFFIX.len() <= SecretName::MAX_LENGTH, + "The string `-shared-internal-secret` must not exceed the limit of Secret \ + names." + ); + // A ClusterName is an RFC 1035 label, so appending an alphanumeric-terminated suffix keeps it a + // valid RFC 1123 subdomain. + let _ = ClusterName::IS_RFC_1123_SUBDOMAIN_NAME; + + SecretName::from_str(&format!("{cluster_name}{SUFFIX}")) + .expect("the shared internal secret name is a valid Secret name") }