From fcd839c4eee78aedb7d508085b369f733351d9fb Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 11:16:35 +0200 Subject: [PATCH 1/6] use expect where possible and remove unecessary enums/Results --- .../controller/build/resource/config_map.rs | 12 +- .../src/controller/build/resource/listener.rs | 18 +- .../controller/build/resource/statefulset.rs | 177 +++++++++++------- rust/operator-binary/src/crd/mod.rs | 8 +- rust/operator-binary/src/crd/tls.rs | 18 +- .../src/security/authentication.rs | 29 +-- .../src/security/authorization.rs | 10 +- rust/operator-binary/src/security/tls.rs | 19 +- 8 files changed, 160 insertions(+), 131 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index d12e3b6f..ed335aa9 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -33,12 +33,6 @@ pub enum Error { rolegroup: RoleGroupName, }, - #[snafu(display("failed to build ConfigMap for {rolegroup}"))] - BuildRoleGroupConfig { - source: stackable_operator::builder::configmap::Error, - rolegroup: RoleGroupName, - }, - #[snafu(display("failed to serialize JVM security properties for {}", rolegroup))] JvmSecurityProperties { source: stackable_operator::v2::config_file_writer::PropertiesWriterError, @@ -130,9 +124,7 @@ pub fn build_rolegroup_config_map( ); } - cm_builder + Ok(cm_builder .build() - .with_context(|_| BuildRoleGroupConfigSnafu { - rolegroup: role_group_name.clone(), - }) + .expect("The ConfigMap metadata is set in this function.")) } diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index 9c6b521d..446bc33c 100644 --- a/rust/operator-binary/src/controller/build/resource/listener.rs +++ b/rust/operator-binary/src/controller/build/resource/listener.rs @@ -11,7 +11,7 @@ use stackable_operator::{ }, types::{ kubernetes::{ListenerClassName, ListenerName, PersistentVolumeClaimName}, - operator::RoleName, + operator::{ClusterName, RoleName}, }, }, }; @@ -24,11 +24,11 @@ use crate::{ crd::NifiRole, }; -pub const LISTENER_VOLUME_NAME: &str = "listener"; pub const LISTENER_VOLUME_DIR: &str = "/stackable/listener"; // The listener volume is provisioned as a PVC by the listener-operator; this is its typed name. -constant!(LISTENER_PVC_NAME: PersistentVolumeClaimName = "listener"); +// The volume mount referencing the PVC and the secret-operator listener scope use the same name. +constant!(pub LISTENER_PVC_NAME: PersistentVolumeClaimName = "listener"); pub fn build_group_listener( cluster: &ValidatedCluster, @@ -69,13 +69,19 @@ pub fn build_group_listener_pvc( } pub fn group_listener_name(cluster: &ValidatedCluster, role_name: &RoleName) -> ListenerName { + 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; + ListenerName::from_str(&format!( "{cluster_name}-{role_name}", cluster_name = cluster.name )) - .expect( - "the cluster name and role name form a valid Listener name, because both are length-bounded types whose combined length stays within the Listener name limit", - ) + .expect("The role listener name is a valid Listener name.") } #[cfg(test)] diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index d00a2d28..fdea9efc 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -59,7 +59,7 @@ use crate::{ recommended_labels_for_unversioned_role_group_resources, resource::{ listener::{ - LISTENER_VOLUME_DIR, LISTENER_VOLUME_NAME, build_group_listener_pvc, + LISTENER_PVC_NAME, LISTENER_VOLUME_DIR, build_group_listener_pvc, group_listener_name, }, probes::{ @@ -79,9 +79,9 @@ use crate::{ authentication::{ NifiAuthenticationConfig, STACKABLE_SERVER_TLS_DIR, STACKABLE_TLS_STORE_PASSWORD, }, - authorization::{self, OPA_TLS_MOUNT_PATH, ResolvedNifiAuthorizationConfig}, + authorization::{OPA_TLS_MOUNT_PATH, ResolvedNifiAuthorizationConfig}, tls::{ - self, KEYSTORE_NIFI_CONTAINER_MOUNT, KEYSTORE_VOLUME_NAME, TRUSTSTORE_VOLUME_NAME, + KEYSTORE_NIFI_CONTAINER_MOUNT, KEYSTORE_VOLUME_NAME, TRUSTSTORE_VOLUME_NAME, build_tls_volume, }, }, @@ -99,9 +99,6 @@ pub enum Error { source: crate::security::authentication::Error, }, - #[snafu(display("failed to build the TLS certificate Volume"))] - BuildTlsVolume { source: tls::Error }, - #[snafu(display("failed to add needed volume"))] AddVolume { source: builder::pod::Error }, @@ -114,9 +111,6 @@ pub enum Error { GracefulShutdown { source: crate::controller::build::graceful_shutdown::Error, }, - - #[snafu(display("failed to build authorization configuration"))] - AuthorizationConfiguration { source: authorization::Error }, } type Result = std::result::Result; @@ -361,29 +355,29 @@ pub(crate) fn build_node_rolegroup_statefulset( .iter() .map(NifiRepository::volume_mount), ) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(CONFIG_VOLUME_NAME.to_string(), CONFIG_VOLUME_MOUNT) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount( KEYSTORE_VOLUME_NAME.to_string(), KEYSTORE_NIFI_CONTAINER_MOUNT, ) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(ACTIVE_CONFIG_VOLUME_NAME.to_string(), NIFI_CONFIG_DIRECTORY) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount( SENSITIVE_PROPERTY_VOLUME_NAME.to_string(), SENSITIVE_PROPERTY_VOLUME_MOUNT, ) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(LOG_VOLUME_NAME.to_string(), STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(TRUSTSTORE_VOLUME_NAME.to_string(), STACKABLE_SERVER_TLS_DIR) - .context(AddVolumeMountSnafu)? - .add_volume_mount(LISTENER_VOLUME_NAME, LISTENER_VOLUME_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") + .add_volume_mount(&*LISTENER_PVC_NAME, LISTENER_VOLUME_DIR) + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mounts(authorization_config.get_volume_mounts()) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .resources( ResourceRequirementsBuilder::new() .with_cpu_request("500m") @@ -425,25 +419,30 @@ pub(crate) fn build_node_rolegroup_statefulset( KEYSTORE_VOLUME_NAME.to_string(), KEYSTORE_NIFI_CONTAINER_MOUNT, ) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mounts( PERSISTENT_REPOSITORIES .iter() .map(NifiRepository::volume_mount), ) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(ACTIVE_CONFIG_VOLUME_NAME.to_string(), NIFI_CONFIG_DIRECTORY) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(LOG_CONFIG_VOLUME_NAME.to_string(), STACKABLE_LOG_CONFIG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(LOG_VOLUME_NAME.to_string(), STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(TRUSTSTORE_VOLUME_NAME.to_string(), STACKABLE_SERVER_TLS_DIR) - .context(AddVolumeMountSnafu)? - .add_volume_mount(LISTENER_VOLUME_NAME, LISTENER_VOLUME_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") + .add_volume_mount(&*LISTENER_PVC_NAME, LISTENER_VOLUME_DIR) + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mounts(authorization_config.get_volume_mounts()) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") + .add_volume_mount( + PYTHON_WORKING_DIR_VOLUME_NAME.to_string(), + NIFI_PYTHON_WORKING_DIRECTORY, + ) + .expect("The mount paths are statically defined and there should be no duplicates.") .add_container_port(HTTPS_PORT_NAME, HTTPS_PORT.into()) .add_container_port(PROTOCOL_PORT_NAME, PROTOCOL_PORT.into()) .add_container_port(BALANCE_PORT_NAME, BALANCE_PORT.into()) @@ -459,7 +458,14 @@ pub(crate) fn build_node_rolegroup_statefulset( add_graceful_shutdown_config(merged_config, &mut pod_builder).context(GracefulShutdownSnafu)?; - // Add user configured extra volumes if any are specified + pod_builder + .add_empty_dir_volume(PYTHON_WORKING_DIR_VOLUME_NAME.to_string(), None) + .expect("The volume names are statically defined and there should be no duplicates."); + + // Mount the user configured extra volumes, if any are specified. The mounts come after every + // operator-managed mount of the NiFi container, so they can collide with those and stay + // fallible. The volumes themselves are added to the Pod at the very end, after every + // operator-managed volume, for the same reason. for volume in &cluster.cluster_config.extra_volumes { // Extract values into vars so we make it impossible to log something other than // what we actually use to create the mounts - maybe paranoid, but hey .. @@ -472,24 +478,11 @@ pub(crate) fn build_node_rolegroup_statefulset( role = NifiRole::Node.as_ref(), "Adding user specified extra volume", ); - pod_builder - .add_volume(volume.clone()) - .context(AddVolumeSnafu)?; container_nifi .add_volume_mount(volume_name, mount_point) .context(AddVolumeMountSnafu)?; } - pod_builder - .add_empty_dir_volume(PYTHON_WORKING_DIR_VOLUME_NAME.to_string(), None) - .context(AddVolumeSnafu)?; - container_nifi - .add_volume_mount( - PYTHON_WORKING_DIR_VOLUME_NAME.to_string(), - NIFI_PYTHON_WORKING_DIRECTORY, - ) - .context(AddVolumeMountSnafu)?; - container_nifi .add_volume_mounts(git_sync_resources.git_content_volume_mounts.to_owned()) .context(AddVolumeMountSnafu)?; @@ -529,7 +522,7 @@ pub(crate) fn build_node_rolegroup_statefulset( }), ..Volume::default() }) - .context(AddVolumeSnafu)?; + .expect("The volume names are statically defined and there should be no duplicates."); // The Vector logging config was validated up-front in the `validate` step. The static // `vector.yaml` is shipped in the rolegroup `ConfigMap`; the per-rolegroup values (namespace, @@ -573,7 +566,7 @@ pub(crate) fn build_node_rolegroup_statefulset( }), ..Volume::default() }) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") .add_volume(Volume { name: CONFIG_VOLUME_NAME.to_string(), config_map: Some(ConfigMapVolumeSource { @@ -582,7 +575,7 @@ pub(crate) fn build_node_rolegroup_statefulset( }), ..Volume::default() }) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") .add_empty_dir_volume( LOG_VOLUME_NAME.to_string(), // Set volume size to higher than theoretically necessary to avoid running out of disk space as log rotation triggers are only checked by Logback every 5s. @@ -594,31 +587,24 @@ pub(crate) fn build_node_rolegroup_statefulset( .into(), ), ) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") // One volume for the keystore and truststore data configmap - .add_volume( - build_tls_volume( - &cluster.cluster_config.server_tls_secret_class, - &KEYSTORE_VOLUME_NAME, - [cluster - .role_group_resource_names(role_group_name) - .metrics_service_name() - .to_string()], - SecretFormat::TlsPkcs12, - &requested_secret_lifetime, - Some(LISTENER_VOLUME_NAME), - ) - .context(BuildTlsVolumeSnafu)?, - ) - .context(AddVolumeSnafu)? + .add_volume(build_tls_volume( + &cluster.cluster_config.server_tls_secret_class, + &KEYSTORE_VOLUME_NAME, + [cluster + .role_group_resource_names(role_group_name) + .metrics_service_name() + .to_string()], + SecretFormat::TlsPkcs12, + &requested_secret_lifetime, + Some(LISTENER_PVC_NAME.as_ref()), + )) + .expect("The volume names are statically defined and there should be no duplicates.") .add_empty_dir_volume(TRUSTSTORE_VOLUME_NAME.to_string(), None) - .context(AddVolumeSnafu)? - .add_volumes( - authorization_config - .get_volumes() - .context(AuthorizationConfigurationSnafu)?, - ) - .context(AddVolumeSnafu)?; + .expect("The volume names are statically defined and there should be no duplicates.") + .add_volumes(authorization_config.get_volumes()) + .expect("The volume names are statically defined and there should be no duplicates."); pod_builder .add_volume(Volume { @@ -629,7 +615,7 @@ pub(crate) fn build_node_rolegroup_statefulset( }), ..Volume::default() }) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") .add_volume(Volume { empty_dir: Some(EmptyDirVolumeSource { medium: None, @@ -638,7 +624,7 @@ pub(crate) fn build_node_rolegroup_statefulset( name: ACTIVE_CONFIG_VOLUME_NAME.to_string(), ..Volume::default() }) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") .service_account_name( cluster .cluster_resource_names() @@ -651,6 +637,14 @@ pub(crate) fn build_node_rolegroup_statefulset( .build(), ); + // User configured extra volumes last: their names can collide with the operator-managed + // volumes above, so these adds stay fallible. + for volume in &cluster.cluster_config.extra_volumes { + pod_builder + .add_volume(volume.clone()) + .context(AddVolumeSnafu)?; + } + let mut pod_template = pod_builder.build_template(); // `rg.pod_overrides` is already the merged role <- rolegroup overrides. pod_template.merge_from(rg.pod_overrides.clone()); @@ -840,4 +834,47 @@ mod tests { )] ); } + + /// 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 cluster = validated_cluster_from_yaml( + r#" + apiVersion: nifi.stackable.tech/v1alpha1 + kind: NifiCluster + metadata: + name: simple-nifi + namespace: default + spec: + image: + productVersion: 2.9.0 + clusterConfig: + authentication: + - authenticationClass: nifi-admin-credentials-simple + sensitiveProperties: + keySecret: simple-nifi-sensitive-property-key + autoGenerate: true + extraVolumes: + - name: log + emptyDir: {} + nodes: + roleGroups: + default: + replicas: 1 + "#, + ); + let role_group_name = RoleGroupName::from_str("default").expect("valid role group name"); + let rg = default_rg(&cluster); + + let Err(error) = build_node_rolegroup_statefulset(&cluster, &role_group_name, rg, None) + else { + panic!("the colliding extra volume must be rejected"); + }; + assert!( + matches!(error, Error::AddVolume { .. }), + "unexpected error: {error:?}" + ); + } } diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 30c413c0..7de90208 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -433,9 +433,12 @@ impl Default for NifiNodeRoleConfig { } } +constant!(NODE_DEFAULT_LISTENER_CLASS: ListenerClassName = "cluster-internal"); + +/// Serde default for `listenerClass`. Kept as a function because `#[serde(default = "...")]` +/// requires a function path. fn node_default_listener_class() -> ListenerClassName { - ListenerClassName::from_str("cluster-internal") - .expect("'cluster-internal' is a valid listener class name") + NODE_DEFAULT_LISTENER_CLASS.clone() } #[cfg(test)] @@ -548,6 +551,7 @@ mod tests { fn test_constants() { // Test that dereferencing the constants does not panic. let _ = *NODE_ROLE_NAME; + let _ = *NODE_DEFAULT_LISTENER_CLASS; } impl RoundtripTestData for v1alpha1::NifiClusterSpec { diff --git a/rust/operator-binary/src/crd/tls.rs b/rust/operator-binary/src/crd/tls.rs index 3d28f265..74756b8c 100644 --- a/rust/operator-binary/src/crd/tls.rs +++ b/rust/operator-binary/src/crd/tls.rs @@ -2,10 +2,13 @@ use std::str::FromStr; use serde::{Deserialize, Serialize}; use stackable_operator::{ + constant, schemars::{self, JsonSchema}, v2::types::kubernetes::SecretClassName, }; +constant!(DEFAULT_SERVER_SECRET_CLASS: SecretClassName = "tls"); + #[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub struct NifiTls { @@ -25,7 +28,20 @@ impl Default for NifiTls { } impl NifiTls { + /// Serde default for `serverSecretClass`. Kept as a function because + /// `#[serde(default = "...")]` requires a function path. fn default_server_secret_class() -> SecretClassName { - SecretClassName::from_str("tls").expect("'tls' is a valid secret class name") + DEFAULT_SERVER_SECRET_CLASS.clone() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_constants() { + // Test that dereferencing the constants does not panic. + let _ = *DEFAULT_SERVER_SECRET_CLASS; } } diff --git a/rust/operator-binary/src/security/authentication.rs b/rust/operator-binary/src/security/authentication.rs index 61395d94..1d3927cd 100644 --- a/rust/operator-binary/src/security/authentication.rs +++ b/rust/operator-binary/src/security/authentication.rs @@ -1,10 +1,7 @@ use indoc::{formatdoc, indoc}; use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{ - builder::{ - self, - pod::{PodBuilder, container::ContainerBuilder}, - }, + builder::pod::{PodBuilder, container::ContainerBuilder}, client::Client, crd::authentication::{core as auth_core, ldap, oidc, r#static}, k8s_openapi::api::core::v1::{KeyToPath, SecretVolumeSource, Volume}, @@ -74,14 +71,6 @@ pub enum Error { "The LDAP AuthenticationClass is missing the bind credentials. Currently the NiFi operator only supports connecting to LDAP servers using bind credentials" ))] LdapAuthenticationClassMissingBindCredentials {}, - - #[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, - }, } type Result = std::result::Result; @@ -243,13 +232,13 @@ impl NifiAuthenticationConfig { }), ..Volume::default() }; - pod_builder - .add_volume(admin_volume) - .context(AddVolumeSnafu)?; + pod_builder.add_volume(admin_volume).expect( + "The volume names are statically defined and there should be no duplicates.", + ); for cb in container_builders { cb.add_volume_mount(STACKABLE_ADMIN_USERNAME, STACKABLE_USER_VOLUME_MOUNT_PATH) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); } } Self::Ldap { provider } => { @@ -276,13 +265,13 @@ impl NifiAuthenticationConfig { }), ..Volume::default() }; - pod_builder - .add_volume(admin_volume) - .context(AddVolumeSnafu)?; + pod_builder.add_volume(admin_volume).expect( + "The volume names are statically defined and there should be no duplicates.", + ); for cb in &mut container_builders { cb.add_volume_mount(STACKABLE_ADMIN_USERNAME, STACKABLE_USER_VOLUME_MOUNT_PATH) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); } provider diff --git a/rust/operator-binary/src/security/authorization.rs b/rust/operator-binary/src/security/authorization.rs index a182806f..955f2dcd 100644 --- a/rust/operator-binary/src/security/authorization.rs +++ b/rust/operator-binary/src/security/authorization.rs @@ -37,10 +37,6 @@ pub enum Error { configmap_name: String, namespace: String, }, - #[snafu(display("failed to build OPA TLS certificate volume"))] - OpaTlsCertSecretClassVolumeBuild { - source: stackable_operator::builder::pod::volume::SecretOperatorVolumeSourceBuilderError, - }, } #[derive(Clone)] @@ -270,7 +266,7 @@ impl ResolvedNifiAuthorizationConfig { volume_mounts } - pub fn get_volumes(&self) -> Result, Error> { + pub fn get_volumes(&self) -> Vec { let mut volumes = vec![]; if let ResolvedNifiAuthorizationConfig::Opa { @@ -287,13 +283,13 @@ impl ResolvedNifiAuthorizationConfig { SecretClassVolumeProvisionParts::Public, ) .build() - .context(OpaTlsCertSecretClassVolumeBuildSnafu)?, + .expect("The annotation keys are static and annotation values cannot be invalid."), ) .build(), ) }; - Ok(volumes) + volumes } pub fn has_opa_tls(&self) -> bool { diff --git a/rust/operator-binary/src/security/tls.rs b/rust/operator-binary/src/security/tls.rs index 8a0d554e..3956586b 100644 --- a/rust/operator-binary/src/security/tls.rs +++ b/rust/operator-binary/src/security/tls.rs @@ -1,6 +1,5 @@ use std::str::FromStr; -use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::pod::volume::{SecretFormat, SecretOperatorVolumeSourceBuilder, VolumeBuilder}, commons::secret_class::SecretClassVolumeProvisionParts, @@ -16,16 +15,6 @@ constant!(pub KEYSTORE_VOLUME_NAME: VolumeName = "keystore"); pub const KEYSTORE_NIFI_CONTAINER_MOUNT: &str = "/stackable/keystore"; constant!(pub TRUSTSTORE_VOLUME_NAME: VolumeName = "truststore"); -type Result = std::result::Result; - -#[derive(Snafu, Debug)] -pub enum Error { - #[snafu(display("failed to build TLS certificate SecretClass Volume"))] - TlsCertSecretClassVolumeBuild { - source: stackable_operator::builder::pod::volume::SecretOperatorVolumeSourceBuilderError, - }, -} - pub(crate) fn build_tls_volume( server_tls_secret_class: &SecretClassName, volume_name: &VolumeName, @@ -33,7 +22,7 @@ pub(crate) fn build_tls_volume( secret_format: SecretFormat, requested_secret_lifetime: &Duration, listener_scope: Option<&str>, -) -> Result { +) -> Volume { let mut secret_volume_source_builder = SecretOperatorVolumeSourceBuilder::new( server_tls_secret_class, // NiFi serves its own TLS endpoints, so the Pod needs both the public @@ -51,16 +40,16 @@ pub(crate) fn build_tls_volume( secret_volume_source_builder.with_listener_volume_scope(listener_scope); } - Ok(VolumeBuilder::new(volume_name) + VolumeBuilder::new(volume_name) .ephemeral( secret_volume_source_builder .with_pod_scope() .with_format(secret_format) .with_auto_tls_cert_lifetime(*requested_secret_lifetime) .build() - .context(TlsCertSecretClassVolumeBuildSnafu)?, + .expect("The annotation keys are static and annotation values cannot be invalid."), ) - .build()) + .build() } #[cfg(test)] From eaf9b3b34b25e1fee2c3017c299e36c50de4c92c Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 11:19:41 +0200 Subject: [PATCH 2/6] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6d40a3e..eff44741 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ All notable changes to this project will be documented in this file. a StatefulSet are immutable, StatefulSets created by older operator versions cannot be updated in place: after the operator upgrade, delete each node StatefulSet so that the operator immediately recreates it with the new labels ([#984]). +- Make operations infallible where appropriate ([#990]). ### Fixed @@ -51,6 +52,7 @@ All notable changes to this project will be documented in this file. [#982]: https://github.com/stackabletech/nifi-operator/pull/982 [#984]: https://github.com/stackabletech/nifi-operator/pull/984 [#985]: https://github.com/stackabletech/nifi-operator/pull/985 +[#990]: https://github.com/stackabletech/nifi-operator/pull/990 ## [26.7.0] - 2026-07-21 From 8c62d5ee7c4b2d598202df4cef3cdde9c71edf95 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 16:30:54 +0200 Subject: [PATCH 3/6] add panic docs to helper functions and re-order volumes/mounts adds: statics before deriveds --- .../src/controller/build/resource/listener.rs | 6 +++ .../controller/build/resource/statefulset.rs | 46 +++++++++++-------- .../src/security/authentication.rs | 6 +++ .../src/security/authorization.rs | 6 +++ rust/operator-binary/src/security/tls.rs | 6 +++ 5 files changed, 51 insertions(+), 19 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index 446bc33c..9171e53c 100644 --- a/rust/operator-binary/src/controller/build/resource/listener.rs +++ b/rust/operator-binary/src/controller/build/resource/listener.rs @@ -57,6 +57,12 @@ pub fn build_group_listener( } } +/// Builds the persistent volume claim template for the group listener volume. +/// +/// # Panics +/// +/// Panics if the volume source cannot be built, which cannot happen because the annotation +/// keys are static and annotation values cannot be invalid. pub fn build_group_listener_pvc( group_listener_name: &ListenerName, unversioned_recommended_labels: &Labels, diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index fdea9efc..bbe90494 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -498,12 +498,6 @@ pub(crate) fn build_node_rolegroup_statefulset( for container in git_sync_resources.git_sync_init_containers.iter().cloned() { pod_builder.add_init_container(container); } - pod_builder - .add_volumes(git_sync_resources.git_content_volumes.to_owned()) - .context(AddVolumeSnafu)?; - pod_builder - .add_volumes(git_sync_resources.git_ca_cert_volumes.to_owned()) - .context(AddVolumeSnafu)?; // The NiFi `log-config` volume sources from the custom log ConfigMap when one is configured, // otherwise from this rolegroup's ConfigMap (which carries the operator-generated `logback.xml`). @@ -540,22 +534,13 @@ pub(crate) fn build_node_rolegroup_statefulset( )); } - authentication_config - .add_volumes_and_mounts(&mut pod_builder, vec![&mut container_prepare]) - .context(AddAuthVolumesSnafu)?; - - let metadata = ObjectMetaBuilder::new() - .with_labels(recommended_object_labels) - .build(); - let requested_secret_lifetime = merged_config .requested_secret_lifetime .context(MissingSecretLifetimeSnafu)?; + // Operator-managed volumes with static names first: their adds are infallible. The volumes + // derived from user input (authentication, git-sync) and the user's `extraVolumes` are added + // afterwards and stay fallible, as they can collide with the operator-managed ones. pod_builder - .metadata(metadata) - .image_pull_secrets_from_product_image(resolved_product_image) - .add_init_container(container_prepare.build()) - .affinity(&merged_config.affinity) // The rolegroup `ConfigMap` mounted as-is (it also carries `vector.yaml`); read by the // Vector sidecar via [`VECTOR_LOG_CONFIG_VOLUME_NAME`]. .add_volume(Volume { @@ -624,7 +609,30 @@ pub(crate) fn build_node_rolegroup_statefulset( name: ACTIVE_CONFIG_VOLUME_NAME.to_string(), ..Volume::default() }) - .expect("The volume names are statically defined and there should be no duplicates.") + .expect("The volume names are statically defined and there should be no duplicates."); + + // Volumes derived from user input: the authentication volumes are named after the user's + // SecretClasses (the helper adds its own static `admin` volume first), the git-sync volumes + // are numbered per configured repository. + authentication_config + .add_volumes_and_mounts(&mut pod_builder, vec![&mut container_prepare]) + .context(AddAuthVolumesSnafu)?; + pod_builder + .add_volumes(git_sync_resources.git_content_volumes.to_owned()) + .context(AddVolumeSnafu)?; + pod_builder + .add_volumes(git_sync_resources.git_ca_cert_volumes.to_owned()) + .context(AddVolumeSnafu)?; + + let metadata = ObjectMetaBuilder::new() + .with_labels(recommended_object_labels) + .build(); + + pod_builder + .metadata(metadata) + .image_pull_secrets_from_product_image(resolved_product_image) + .add_init_container(container_prepare.build()) + .affinity(&merged_config.affinity) .service_account_name( cluster .cluster_resource_names() diff --git a/rust/operator-binary/src/security/authentication.rs b/rust/operator-binary/src/security/authentication.rs index 1d3927cd..48a1d445 100644 --- a/rust/operator-binary/src/security/authentication.rs +++ b/rust/operator-binary/src/security/authentication.rs @@ -211,6 +211,12 @@ impl NifiAuthenticationConfig { /// Adds the volumes and volume mounts required by the configured authentication /// method to the pod and the given container builders. + /// + /// # Panics + /// + /// Panics if the volumes or volume mounts cannot be added to the builders. Only call this + /// on builders whose volume names and mount paths are still distinct from the ones added + /// here. pub fn add_volumes_and_mounts( &self, pod_builder: &mut PodBuilder, diff --git a/rust/operator-binary/src/security/authorization.rs b/rust/operator-binary/src/security/authorization.rs index 955f2dcd..33abf773 100644 --- a/rust/operator-binary/src/security/authorization.rs +++ b/rust/operator-binary/src/security/authorization.rs @@ -266,6 +266,12 @@ impl ResolvedNifiAuthorizationConfig { volume_mounts } + /// Returns the volumes required by the configured authorization method. + /// + /// # Panics + /// + /// Panics if a volume source cannot be built, which cannot happen because the annotation + /// keys are static and annotation values cannot be invalid. pub fn get_volumes(&self) -> Vec { let mut volumes = vec![]; diff --git a/rust/operator-binary/src/security/tls.rs b/rust/operator-binary/src/security/tls.rs index 3956586b..7866b7cf 100644 --- a/rust/operator-binary/src/security/tls.rs +++ b/rust/operator-binary/src/security/tls.rs @@ -15,6 +15,12 @@ constant!(pub KEYSTORE_VOLUME_NAME: VolumeName = "keystore"); pub const KEYSTORE_NIFI_CONTAINER_MOUNT: &str = "/stackable/keystore"; constant!(pub TRUSTSTORE_VOLUME_NAME: VolumeName = "truststore"); +/// Builds the secret-operator volume providing the TLS keystore for the given SecretClass. +/// +/// # Panics +/// +/// Panics if the volume source cannot be built, which cannot happen because the annotation +/// keys are static and annotation values cannot be invalid. pub(crate) fn build_tls_volume( server_tls_secret_class: &SecretClassName, volume_name: &VolumeName, From 85c75448ae4d141c62a287b41d9f35c9498b703e Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 16:43:36 +0200 Subject: [PATCH 4/6] remove panics doc where it make no sense --- .../src/controller/build/resource/listener.rs | 6 ------ rust/operator-binary/src/security/authorization.rs | 6 ------ rust/operator-binary/src/security/tls.rs | 6 ------ 3 files changed, 18 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index 9171e53c..446bc33c 100644 --- a/rust/operator-binary/src/controller/build/resource/listener.rs +++ b/rust/operator-binary/src/controller/build/resource/listener.rs @@ -57,12 +57,6 @@ pub fn build_group_listener( } } -/// Builds the persistent volume claim template for the group listener volume. -/// -/// # Panics -/// -/// Panics if the volume source cannot be built, which cannot happen because the annotation -/// keys are static and annotation values cannot be invalid. pub fn build_group_listener_pvc( group_listener_name: &ListenerName, unversioned_recommended_labels: &Labels, diff --git a/rust/operator-binary/src/security/authorization.rs b/rust/operator-binary/src/security/authorization.rs index 33abf773..955f2dcd 100644 --- a/rust/operator-binary/src/security/authorization.rs +++ b/rust/operator-binary/src/security/authorization.rs @@ -266,12 +266,6 @@ impl ResolvedNifiAuthorizationConfig { volume_mounts } - /// Returns the volumes required by the configured authorization method. - /// - /// # Panics - /// - /// Panics if a volume source cannot be built, which cannot happen because the annotation - /// keys are static and annotation values cannot be invalid. pub fn get_volumes(&self) -> Vec { let mut volumes = vec![]; diff --git a/rust/operator-binary/src/security/tls.rs b/rust/operator-binary/src/security/tls.rs index 7866b7cf..3956586b 100644 --- a/rust/operator-binary/src/security/tls.rs +++ b/rust/operator-binary/src/security/tls.rs @@ -15,12 +15,6 @@ constant!(pub KEYSTORE_VOLUME_NAME: VolumeName = "keystore"); pub const KEYSTORE_NIFI_CONTAINER_MOUNT: &str = "/stackable/keystore"; constant!(pub TRUSTSTORE_VOLUME_NAME: VolumeName = "truststore"); -/// Builds the secret-operator volume providing the TLS keystore for the given SecretClass. -/// -/// # Panics -/// -/// Panics if the volume source cannot be built, which cannot happen because the annotation -/// keys are static and annotation values cannot be invalid. pub(crate) fn build_tls_volume( server_tls_secret_class: &SecretClassName, volume_name: &VolumeName, From faa7cdcff3466265b90635d2fcf0471de4b345de Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 8 Sep 2026 17:32:07 +0200 Subject: [PATCH 5/6] revert expects where checked data is not static/explicit --- CHANGELOG.md | 2 +- .../controller/build/resource/config_map.rs | 12 ++- .../controller/build/resource/statefulset.rs | 78 +++++++++++-------- .../src/security/authentication.rs | 25 +++--- .../src/security/authorization.rs | 10 ++- rust/operator-binary/src/security/tls.rs | 19 ++++- 6 files changed, 94 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eff44741..32d99296 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ All notable changes to this project will be documented in this file. a StatefulSet are immutable, StatefulSets created by older operator versions cannot be updated in place: after the operator upgrade, delete each node StatefulSet so that the operator immediately recreates it with the new labels ([#984]). -- Make operations infallible where appropriate ([#990]). +- Make operations infallible where dependent on static inputs ([#990]). ### Fixed diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index ed335aa9..d12e3b6f 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -33,6 +33,12 @@ pub enum Error { rolegroup: RoleGroupName, }, + #[snafu(display("failed to build ConfigMap for {rolegroup}"))] + BuildRoleGroupConfig { + source: stackable_operator::builder::configmap::Error, + rolegroup: RoleGroupName, + }, + #[snafu(display("failed to serialize JVM security properties for {}", rolegroup))] JvmSecurityProperties { source: stackable_operator::v2::config_file_writer::PropertiesWriterError, @@ -124,7 +130,9 @@ pub fn build_rolegroup_config_map( ); } - Ok(cm_builder + cm_builder .build() - .expect("The ConfigMap metadata is set in this function.")) + .with_context(|_| BuildRoleGroupConfigSnafu { + rolegroup: role_group_name.clone(), + }) } diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index bbe90494..4f729c98 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -79,9 +79,9 @@ use crate::{ authentication::{ NifiAuthenticationConfig, STACKABLE_SERVER_TLS_DIR, STACKABLE_TLS_STORE_PASSWORD, }, - authorization::{OPA_TLS_MOUNT_PATH, ResolvedNifiAuthorizationConfig}, + authorization::{self, OPA_TLS_MOUNT_PATH, ResolvedNifiAuthorizationConfig}, tls::{ - KEYSTORE_NIFI_CONTAINER_MOUNT, KEYSTORE_VOLUME_NAME, TRUSTSTORE_VOLUME_NAME, + self, KEYSTORE_NIFI_CONTAINER_MOUNT, KEYSTORE_VOLUME_NAME, TRUSTSTORE_VOLUME_NAME, build_tls_volume, }, }, @@ -99,6 +99,9 @@ pub enum Error { source: crate::security::authentication::Error, }, + #[snafu(display("failed to build the TLS certificate Volume"))] + BuildTlsVolume { source: tls::Error }, + #[snafu(display("failed to add needed volume"))] AddVolume { source: builder::pod::Error }, @@ -111,6 +114,9 @@ pub enum Error { GracefulShutdown { source: crate::controller::build::graceful_shutdown::Error, }, + + #[snafu(display("failed to build authorization configuration"))] + AuthorizationConfiguration { source: authorization::Error }, } type Result = std::result::Result; @@ -355,7 +361,7 @@ pub(crate) fn build_node_rolegroup_statefulset( .iter() .map(NifiRepository::volume_mount), ) - .expect("The mount paths are statically defined and there should be no duplicates.") + .context(AddVolumeMountSnafu)? .add_volume_mount(CONFIG_VOLUME_NAME.to_string(), CONFIG_VOLUME_MOUNT) .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount( @@ -377,7 +383,7 @@ pub(crate) fn build_node_rolegroup_statefulset( .add_volume_mount(&*LISTENER_PVC_NAME, LISTENER_VOLUME_DIR) .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mounts(authorization_config.get_volume_mounts()) - .expect("The mount paths are statically defined and there should be no duplicates.") + .context(AddVolumeMountSnafu)? .resources( ResourceRequirementsBuilder::new() .with_cpu_request("500m") @@ -425,7 +431,7 @@ pub(crate) fn build_node_rolegroup_statefulset( .iter() .map(NifiRepository::volume_mount), ) - .expect("The mount paths are statically defined and there should be no duplicates.") + .context(AddVolumeMountSnafu)? .add_volume_mount(ACTIVE_CONFIG_VOLUME_NAME.to_string(), NIFI_CONFIG_DIRECTORY) .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(LOG_CONFIG_VOLUME_NAME.to_string(), STACKABLE_LOG_CONFIG_DIR) @@ -437,7 +443,7 @@ pub(crate) fn build_node_rolegroup_statefulset( .add_volume_mount(&*LISTENER_PVC_NAME, LISTENER_VOLUME_DIR) .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mounts(authorization_config.get_volume_mounts()) - .expect("The mount paths are statically defined and there should be no duplicates.") + .context(AddVolumeMountSnafu)? .add_volume_mount( PYTHON_WORKING_DIR_VOLUME_NAME.to_string(), NIFI_PYTHON_WORKING_DIRECTORY, @@ -465,7 +471,7 @@ pub(crate) fn build_node_rolegroup_statefulset( // Mount the user configured extra volumes, if any are specified. The mounts come after every // operator-managed mount of the NiFi container, so they can collide with those and stay // fallible. The volumes themselves are added to the Pod at the very end, after every - // operator-managed volume, for the same reason. + // operator-managed volume. for volume in &cluster.cluster_config.extra_volumes { // Extract values into vars so we make it impossible to log something other than // what we actually use to create the mounts - maybe paranoid, but hey .. @@ -516,7 +522,7 @@ pub(crate) fn build_node_rolegroup_statefulset( }), ..Volume::default() }) - .expect("The volume names are statically defined and there should be no duplicates."); + .context(AddVolumeSnafu)?; // The Vector logging config was validated up-front in the `validate` step. The static // `vector.yaml` is shipped in the rolegroup `ConfigMap`; the per-rolegroup values (namespace, @@ -537,9 +543,10 @@ pub(crate) fn build_node_rolegroup_statefulset( let requested_secret_lifetime = merged_config .requested_secret_lifetime .context(MissingSecretLifetimeSnafu)?; - // Operator-managed volumes with static names first: their adds are infallible. The volumes - // derived from user input (authentication, git-sync) and the user's `extraVolumes` are added - // afterwards and stay fallible, as they can collide with the operator-managed ones. + // Operator-managed volumes first. Only the volume mounts with static paths are added + // infallibly, every volume add is fallible. The volumes derived from user input + // (authentication, git-sync) and the user's `extraVolumes` are added afterwards, so a name + // collision is reported on the user-derived side. pod_builder // The rolegroup `ConfigMap` mounted as-is (it also carries `vector.yaml`); read by the // Vector sidecar via [`VECTOR_LOG_CONFIG_VOLUME_NAME`]. @@ -551,7 +558,7 @@ pub(crate) fn build_node_rolegroup_statefulset( }), ..Volume::default() }) - .expect("The volume names are statically defined and there should be no duplicates.") + .context(AddVolumeSnafu)? .add_volume(Volume { name: CONFIG_VOLUME_NAME.to_string(), config_map: Some(ConfigMapVolumeSource { @@ -560,7 +567,7 @@ pub(crate) fn build_node_rolegroup_statefulset( }), ..Volume::default() }) - .expect("The volume names are statically defined and there should be no duplicates.") + .context(AddVolumeSnafu)? .add_empty_dir_volume( LOG_VOLUME_NAME.to_string(), // Set volume size to higher than theoretically necessary to avoid running out of disk space as log rotation triggers are only checked by Logback every 5s. @@ -572,24 +579,31 @@ pub(crate) fn build_node_rolegroup_statefulset( .into(), ), ) - .expect("The volume names are statically defined and there should be no duplicates.") + .context(AddVolumeSnafu)? // One volume for the keystore and truststore data configmap - .add_volume(build_tls_volume( - &cluster.cluster_config.server_tls_secret_class, - &KEYSTORE_VOLUME_NAME, - [cluster - .role_group_resource_names(role_group_name) - .metrics_service_name() - .to_string()], - SecretFormat::TlsPkcs12, - &requested_secret_lifetime, - Some(LISTENER_PVC_NAME.as_ref()), - )) - .expect("The volume names are statically defined and there should be no duplicates.") + .add_volume( + build_tls_volume( + &cluster.cluster_config.server_tls_secret_class, + &KEYSTORE_VOLUME_NAME, + [cluster + .role_group_resource_names(role_group_name) + .metrics_service_name() + .to_string()], + SecretFormat::TlsPkcs12, + &requested_secret_lifetime, + Some(LISTENER_PVC_NAME.as_ref()), + ) + .context(BuildTlsVolumeSnafu)?, + ) + .context(AddVolumeSnafu)? .add_empty_dir_volume(TRUSTSTORE_VOLUME_NAME.to_string(), None) .expect("The volume names are statically defined and there should be no duplicates.") - .add_volumes(authorization_config.get_volumes()) - .expect("The volume names are statically defined and there should be no duplicates."); + .add_volumes( + authorization_config + .get_volumes() + .context(AuthorizationConfigurationSnafu)?, + ) + .context(AddVolumeSnafu)?; pod_builder .add_volume(Volume { @@ -600,7 +614,7 @@ pub(crate) fn build_node_rolegroup_statefulset( }), ..Volume::default() }) - .expect("The volume names are statically defined and there should be no duplicates.") + .context(AddVolumeSnafu)? .add_volume(Volume { empty_dir: Some(EmptyDirVolumeSource { medium: None, @@ -609,7 +623,7 @@ pub(crate) fn build_node_rolegroup_statefulset( name: ACTIVE_CONFIG_VOLUME_NAME.to_string(), ..Volume::default() }) - .expect("The volume names are statically defined and there should be no duplicates."); + .context(AddVolumeSnafu)?; // Volumes derived from user input: the authentication volumes are named after the user's // SecretClasses (the helper adds its own static `admin` volume first), the git-sync volumes @@ -844,8 +858,8 @@ mod tests { } /// 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). + /// reported as an error (the operator's own volumes are added first, so the collision + /// surfaces on the user-supplied side). #[test] fn extra_volume_colliding_with_operator_volume_is_an_error() { let cluster = validated_cluster_from_yaml( diff --git a/rust/operator-binary/src/security/authentication.rs b/rust/operator-binary/src/security/authentication.rs index 48a1d445..0f011be3 100644 --- a/rust/operator-binary/src/security/authentication.rs +++ b/rust/operator-binary/src/security/authentication.rs @@ -1,7 +1,10 @@ use indoc::{formatdoc, indoc}; use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{ - builder::pod::{PodBuilder, container::ContainerBuilder}, + builder::{ + self, + pod::{PodBuilder, container::ContainerBuilder}, + }, client::Client, crd::authentication::{core as auth_core, ldap, oidc, r#static}, k8s_openapi::api::core::v1::{KeyToPath, SecretVolumeSource, Volume}, @@ -71,6 +74,9 @@ pub enum Error { "The LDAP AuthenticationClass is missing the bind credentials. Currently the NiFi operator only supports connecting to LDAP servers using bind credentials" ))] LdapAuthenticationClassMissingBindCredentials {}, + + #[snafu(display("failed to add needed volume"))] + AddVolume { source: builder::pod::Error }, } type Result = std::result::Result; @@ -214,9 +220,8 @@ impl NifiAuthenticationConfig { /// /// # Panics /// - /// Panics if the volumes or volume mounts cannot be added to the builders. Only call this - /// on builders whose volume names and mount paths are still distinct from the ones added - /// here. + /// 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_volumes_and_mounts( &self, pod_builder: &mut PodBuilder, @@ -238,9 +243,9 @@ impl NifiAuthenticationConfig { }), ..Volume::default() }; - pod_builder.add_volume(admin_volume).expect( - "The volume names are statically defined and there should be no duplicates.", - ); + pod_builder + .add_volume(admin_volume) + .context(AddVolumeSnafu)?; for cb in container_builders { cb.add_volume_mount(STACKABLE_ADMIN_USERNAME, STACKABLE_USER_VOLUME_MOUNT_PATH) @@ -271,9 +276,9 @@ impl NifiAuthenticationConfig { }), ..Volume::default() }; - pod_builder.add_volume(admin_volume).expect( - "The volume names are statically defined and there should be no duplicates.", - ); + pod_builder + .add_volume(admin_volume) + .context(AddVolumeSnafu)?; for cb in &mut container_builders { cb.add_volume_mount(STACKABLE_ADMIN_USERNAME, STACKABLE_USER_VOLUME_MOUNT_PATH) diff --git a/rust/operator-binary/src/security/authorization.rs b/rust/operator-binary/src/security/authorization.rs index 955f2dcd..a182806f 100644 --- a/rust/operator-binary/src/security/authorization.rs +++ b/rust/operator-binary/src/security/authorization.rs @@ -37,6 +37,10 @@ pub enum Error { configmap_name: String, namespace: String, }, + #[snafu(display("failed to build OPA TLS certificate volume"))] + OpaTlsCertSecretClassVolumeBuild { + source: stackable_operator::builder::pod::volume::SecretOperatorVolumeSourceBuilderError, + }, } #[derive(Clone)] @@ -266,7 +270,7 @@ impl ResolvedNifiAuthorizationConfig { volume_mounts } - pub fn get_volumes(&self) -> Vec { + pub fn get_volumes(&self) -> Result, Error> { let mut volumes = vec![]; if let ResolvedNifiAuthorizationConfig::Opa { @@ -283,13 +287,13 @@ impl ResolvedNifiAuthorizationConfig { SecretClassVolumeProvisionParts::Public, ) .build() - .expect("The annotation keys are static and annotation values cannot be invalid."), + .context(OpaTlsCertSecretClassVolumeBuildSnafu)?, ) .build(), ) }; - volumes + Ok(volumes) } pub fn has_opa_tls(&self) -> bool { diff --git a/rust/operator-binary/src/security/tls.rs b/rust/operator-binary/src/security/tls.rs index 3956586b..8a0d554e 100644 --- a/rust/operator-binary/src/security/tls.rs +++ b/rust/operator-binary/src/security/tls.rs @@ -1,5 +1,6 @@ use std::str::FromStr; +use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::pod::volume::{SecretFormat, SecretOperatorVolumeSourceBuilder, VolumeBuilder}, commons::secret_class::SecretClassVolumeProvisionParts, @@ -15,6 +16,16 @@ constant!(pub KEYSTORE_VOLUME_NAME: VolumeName = "keystore"); pub const KEYSTORE_NIFI_CONTAINER_MOUNT: &str = "/stackable/keystore"; constant!(pub TRUSTSTORE_VOLUME_NAME: VolumeName = "truststore"); +type Result = std::result::Result; + +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display("failed to build TLS certificate SecretClass Volume"))] + TlsCertSecretClassVolumeBuild { + source: stackable_operator::builder::pod::volume::SecretOperatorVolumeSourceBuilderError, + }, +} + pub(crate) fn build_tls_volume( server_tls_secret_class: &SecretClassName, volume_name: &VolumeName, @@ -22,7 +33,7 @@ pub(crate) fn build_tls_volume( secret_format: SecretFormat, requested_secret_lifetime: &Duration, listener_scope: Option<&str>, -) -> Volume { +) -> Result { let mut secret_volume_source_builder = SecretOperatorVolumeSourceBuilder::new( server_tls_secret_class, // NiFi serves its own TLS endpoints, so the Pod needs both the public @@ -40,16 +51,16 @@ pub(crate) fn build_tls_volume( secret_volume_source_builder.with_listener_volume_scope(listener_scope); } - VolumeBuilder::new(volume_name) + Ok(VolumeBuilder::new(volume_name) .ephemeral( secret_volume_source_builder .with_pod_scope() .with_format(secret_format) .with_auto_tls_cert_lifetime(*requested_secret_lifetime) .build() - .expect("The annotation keys are static and annotation values cannot be invalid."), + .context(TlsCertSecretClassVolumeBuildSnafu)?, ) - .build() + .build()) } #[cfg(test)] From 5e9e28f48cb069431c88753ac63dda89c7974881 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 9 Sep 2026 16:25:53 +0200 Subject: [PATCH 6/6] add comment/test for group_listener_name --- .../src/controller/build/resource/listener.rs | 24 +++++++++++++++++++ rust/operator-binary/src/crd/mod.rs | 3 ++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index 446bc33c..e3bc1560 100644 --- a/rust/operator-binary/src/controller/build/resource/listener.rs +++ b/rust/operator-binary/src/controller/build/resource/listener.rs @@ -68,6 +68,7 @@ pub fn build_group_listener_pvc( ) } +/// The returned ListenerName is a lowercase RFC 1035 label name (checked by a unit test). pub fn group_listener_name(cluster: &ValidatedCluster, role_name: &RoleName) -> ListenerName { const _: () = assert!( ClusterName::MAX_LENGTH + 1 /* dash */ + RoleName::MAX_LENGTH <= ListenerName::MAX_LENGTH, @@ -86,11 +87,34 @@ pub fn group_listener_name(cluster: &ValidatedCluster, role_name: &RoleName) -> #[cfg(test)] mod tests { + use strum::IntoEnumIterator; + use super::*; + use crate::controller::build::properties::test_support::minimal_validated_cluster; #[test] fn test_constants() { // Test that dereferencing the constants does not panic. let _ = *LISTENER_PVC_NAME; } + + #[test] + fn group_listener_name_is_rfc_1035_label_name() { + // Every ClusterName is a valid RFC 1035 label name, so we use just some string with maximum + // length. + let _ = ClusterName::IS_RFC_1035_LABEL_NAME; + let mut cluster = minimal_validated_cluster(); + cluster.name = ClusterName::from_str(&"a".repeat(ClusterName::MAX_LENGTH)) + .expect("is a valid ClusterName"); + + for role in NifiRole::iter() { + let group_listener_name = group_listener_name(&cluster, &role); + assert!( + stackable_operator::validation::is_lowercase_rfc_1035_label( + group_listener_name.as_ref() + ) + .is_ok() + ); + } + } } diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 7de90208..9af9f8a7 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -42,6 +42,7 @@ use stackable_operator::{ }, versioned::versioned, }; +use strum::EnumIter; use tls::NifiTls; pub const APP_NAME: &str = "nifi"; @@ -225,7 +226,7 @@ pub fn default_allow_all() -> bool { constant!(NODE_ROLE_NAME: RoleName = "node"); -#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +#[derive(Clone, Debug, EnumIter, Eq, Hash, Ord, PartialEq, PartialOrd)] pub enum NifiRole { Node, }