diff --git a/CHANGELOG.md b/CHANGELOG.md index d6d40a3e..32d99296 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 dependent on static inputs ([#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 diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index 9c6b521d..e3bc1560 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, @@ -68,23 +68,53 @@ 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, + "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)] 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/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index d00a2d28..4f729c98 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::{ @@ -363,25 +363,25 @@ pub(crate) fn build_node_rolegroup_statefulset( ) .context(AddVolumeMountSnafu)? .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)? .resources( @@ -425,7 +425,7 @@ 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() @@ -433,17 +433,22 @@ pub(crate) fn build_node_rolegroup_statefulset( ) .context(AddVolumeMountSnafu)? .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)? + .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 +464,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 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 +484,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)?; @@ -505,12 +504,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`). @@ -547,22 +540,14 @@ 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 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 - .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 { @@ -606,13 +591,13 @@ pub(crate) fn build_node_rolegroup_statefulset( .to_string()], SecretFormat::TlsPkcs12, &requested_secret_lifetime, - Some(LISTENER_VOLUME_NAME), + Some(LISTENER_PVC_NAME.as_ref()), ) .context(BuildTlsVolumeSnafu)?, ) .context(AddVolumeSnafu)? .add_empty_dir_volume(TRUSTSTORE_VOLUME_NAME.to_string(), None) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") .add_volumes( authorization_config .get_volumes() @@ -638,7 +623,30 @@ pub(crate) fn build_node_rolegroup_statefulset( name: ACTIVE_CONFIG_VOLUME_NAME.to_string(), ..Volume::default() }) - .context(AddVolumeSnafu)? + .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 + // 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() @@ -651,6 +659,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 +856,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, 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( + 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..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, } @@ -433,9 +434,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 +552,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..0f011be3 100644 --- a/rust/operator-binary/src/security/authentication.rs +++ b/rust/operator-binary/src/security/authentication.rs @@ -77,11 +77,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, - }, } type Result = std::result::Result; @@ -222,6 +217,11 @@ 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 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, @@ -249,7 +249,7 @@ impl NifiAuthenticationConfig { 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 } => { @@ -282,7 +282,7 @@ impl NifiAuthenticationConfig { 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