From 02dbd934bdf15026e46091600e03d25ceae798b9 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 14:08:04 +0200 Subject: [PATCH 1/8] use expect where possible and remove unecessary enums/Results --- rust/info-fetcher-commons/src/utils/secret.rs | 2 +- rust/operator-binary/src/controller/build.rs | 5 +- .../controller/build/resource/config_map.rs | 24 ++--- .../build/resource/daemonset/mod.rs | 100 +++++++++++++----- .../daemonset/resource_info_fetcher.rs | 25 ++--- .../resource/daemonset/user_info_fetcher.rs | 60 +++++------ .../controller/build/resource/discovery.rs | 21 ++-- .../src/crd/user_info_fetcher/mod.rs | 20 +++- 8 files changed, 142 insertions(+), 115 deletions(-) diff --git a/rust/info-fetcher-commons/src/utils/secret.rs b/rust/info-fetcher-commons/src/utils/secret.rs index 70b1526d..f60f4725 100644 --- a/rust/info-fetcher-commons/src/utils/secret.rs +++ b/rust/info-fetcher-commons/src/utils/secret.rs @@ -13,7 +13,7 @@ const REDACTED: &str = "[redacted]"; /// `?token` or `#[instrument]` away from writing that token to the log file the Vector agent ships /// off the node. Wrapping the value means the leak has to be an explicit decision ([`Secret::expose`]) /// rather than an accident: the type has no [`Display`](fmt::Display), and its -/// [`Debug`](fmt::Debug) renders [`REDACTED`], so every struct that holds one can keep deriving +/// [`Debug`](fmt::Debug) renders a default value, so every struct that holds one can keep deriving /// `Debug` safely. #[derive(Clone, PartialEq, Eq, Deserialize)] #[serde(transparent)] diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index 75cdf396..db72de39 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -48,9 +48,6 @@ pub enum Error { source: resource::daemonset::Error, role_group: RoleGroupName, }, - - #[snafu(display("failed to build the discovery ConfigMap"))] - Discovery { source: resource::discovery::Error }, } /// Builds every Kubernetes resource for the given validated cluster. @@ -105,7 +102,7 @@ pub fn build( } // The cluster-level discovery ConfigMap. - config_maps.push(build_discovery_config_map(cluster, cluster_info).context(DiscoverySnafu)?); + config_maps.push(build_discovery_config_map(cluster, cluster_info)); Ok(KubernetesResources { daemon_sets, 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 321aea1f..bd501bba 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -22,21 +22,15 @@ use crate::controller::{ #[derive(Snafu, Debug)] pub enum Error { #[snafu(display("failed to build config.json"))] - BuildConfigJson { source: config_json::Error }, + ConfigJson { source: config_json::Error }, #[snafu(display("failed to build user-info-fetcher.json"))] - BuildUserInfoFetcher { source: user_info_fetcher::Error }, + UserInfoFetcher { source: user_info_fetcher::Error }, #[snafu(display("failed to build resource-info-fetcher.json"))] - BuildResourceInfoFetcher { + ResourceInfoFetcher { source: resource_info_fetcher::Error, }, - - #[snafu(display("failed to assemble ConfigMap for role group {role_group}"))] - Assemble { - source: stackable_operator::builder::configmap::Error, - role_group: RoleGroupName, - }, } type Result = std::result::Result; @@ -66,19 +60,19 @@ pub fn build_rolegroup_config_map( cm_builder.metadata(metadata).add_data( ConfigFileName::ConfigJson.to_string(), config_json::build(&rolegroup_config.config, &rolegroup_config.config_overrides) - .context(BuildConfigJsonSnafu)?, + .context(ConfigJsonSnafu)?, ); if let Some(user_info) = &cluster.cluster_config.user_info { cm_builder.add_data( ConfigFileName::UserInfoFetcher.to_string(), - user_info_fetcher::build(user_info).context(BuildUserInfoFetcherSnafu)?, + user_info_fetcher::build(user_info).context(UserInfoFetcherSnafu)?, ); } if let Some(resource_info) = &cluster.cluster_config.resource_info { cm_builder.add_data( ConfigFileName::ResourceInfoFetcher.to_string(), - resource_info_fetcher::build(resource_info).context(BuildResourceInfoFetcherSnafu)?, + resource_info_fetcher::build(resource_info).context(ResourceInfoFetcherSnafu)?, ); } @@ -89,9 +83,9 @@ pub fn build_rolegroup_config_map( ); } - cm_builder.build().with_context(|_| AssembleSnafu { - role_group: role_group_name.clone(), - }) + Ok(cm_builder + .build() + .expect("The ConfigMap metadata is set in this function.")) } #[cfg(test)] diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 36d93b63..2db5e5f9 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -8,7 +8,6 @@ use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::{Container, DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT, OpaRole}; use stackable_operator::{ builder::{ - self, meta::ObjectMetaBuilder, pod::{ PodBuilder, @@ -174,19 +173,6 @@ pub enum Error { source: crate::operations::graceful_shutdown::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, - }, - - #[snafu(display("failed to build TLS volume"))] - TlsVolumeBuild { - source: builder::pod::volume::SecretOperatorVolumeSourceBuilderError, - }, - #[snafu(display("failed to build User Info Fetcher sidecar"))] BuildUserInfoFetcherSidecar { source: user_info_fetcher::Error }, @@ -312,9 +298,9 @@ pub fn build_server_rolegroup_daemonset( .join(" && "), ]) .add_volume_mount(BUNDLES_VOLUME_NAME.as_ref(), BUNDLES_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(LOG_VOLUME_NAME.as_ref(), STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .resources(merged_config.resources.to_owned().into()); // All operator-set environment variables of the bundle-builder container, collected into an @@ -337,9 +323,9 @@ pub fn build_server_rolegroup_daemonset( )]) .add_env_vars(bundle_builder_env_vars) .add_volume_mount(BUNDLES_VOLUME_NAME.as_ref(), BUNDLES_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(LOG_VOLUME_NAME.as_ref(), STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .resources(sidecar_resource_requirements()) .readiness_probe(http_readiness_probe( BUNDLE_BUILDER_PROBE_PATH, @@ -383,16 +369,16 @@ pub fn build_server_rolegroup_daemonset( cb_opa.add_container_port(service::APP_TLS_PORT_NAME, service::APP_TLS_PORT.into()); cb_opa .add_volume_mount(TLS_VOLUME_NAME.as_ref(), TLS_STORE_DIR) - .context(AddVolumeMountSnafu)?; + .expect("The mount paths are statically defined and there should be no duplicates."); } else { cb_opa.add_container_port(APP_PORT_NAME, APP_PORT.into()); } cb_opa .add_volume_mount(CONFIG_VOLUME_NAME.as_ref(), CONFIG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .add_volume_mount(LOG_VOLUME_NAME.as_ref(), STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .resources(merged_config.resources.to_owned().into()); let (probe_port_name, probe_scheme) = if cluster.is_tls_enabled() { @@ -437,13 +423,13 @@ pub fn build_server_rolegroup_daemonset( ) .build(), ) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") .add_volume( VolumeBuilder::new(BUNDLES_VOLUME_NAME.as_ref()) .with_empty_dir(None::, None) .build(), ) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") .add_volume( VolumeBuilder::new(LOG_VOLUME_NAME.as_ref()) .empty_dir(EmptyDirVolumeSource { @@ -452,7 +438,7 @@ pub fn build_server_rolegroup_daemonset( }) .build(), ) - .context(AddVolumeSnafu)? + .expect("The volume names are statically defined and there should be no duplicates.") .service_account_name( cluster .cluster_resource_names() @@ -488,13 +474,20 @@ pub fn build_server_rolegroup_daemonset( .to_string(), ) .build() - .context(TlsVolumeBuildSnafu)?, + .expect( + "The annotation keys are static and annotation values cannot be invalid.", + ), ) .build(), ) - .context(AddVolumeSnafu)?; + .expect("The volume names are statically defined and there should be no duplicates."); } + // Both sidecars add their statically named volumes (`kerberos`, `*-credentials`) to `pb` with + // `expect`, and the TLS/LDAP helpers from operator-rs add volumes named after user-supplied + // SecretClasses fallibly. The user-info-fetcher's SecretClass-derived volumes precede the + // resource-info-fetcher's static one, which is fine: the derived names always end in `-ca-cert` + // or `-bind-credentials` and so can never equal a static volume name. add_user_info_fetcher_sidecar( &mut pb, cluster, @@ -853,6 +846,7 @@ mod tests { let _ = *BUNDLES_VOLUME_NAME; let _ = *USER_INFO_FETCHER_CREDENTIALS_VOLUME_NAME; let _ = *USER_INFO_FETCHER_KERBEROS_VOLUME_NAME; + let _ = *RESOURCE_INFO_FETCHER_CREDENTIALS_VOLUME_NAME; let _ = *TLS_VOLUME_NAME; let _ = *CONTAINERDEBUG_LOG_DIRECTORY; let _ = *WATCH_NAMESPACE; @@ -1264,6 +1258,60 @@ mod tests { ); } + /// The Entra backend projects its client credentials Secret like the Keycloak backend does. Its + /// TLS CA volume is named after the user's SecretClass and is added to the pod *before* the + /// resource-info-fetcher's statically named credentials volume, so this also pins that the two + /// cannot collide (see the comment above the sidecar calls in `build_server_rolegroup_daemonset`). + #[test] + fn user_info_fetcher_entra_backend_mounts_client_credentials_next_to_resource_info_fetcher() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { + "userInfo": { + "backend": { + "entra": { + "tenantId": "my-tenant", + "clientCredentialsSecret": "entra-credentials", + "tls": { + "verification": { + "server": { "caCert": { "secretClass": "my-ca" } } + } + }, + } + } + }, + "resourceInfo": { + "backend": { + "dataHub": { + "hostname": "datahub-gms.default.svc.cluster.local", + "credentialsSecretName": "datahub-credentials", + } + } + }, + }, + "servers": { "roleGroups": { "default": {} } }, + }))); + + let volumes = volume_names(&ds); + for expected in [ + "user-info-fetcher-credentials", + "my-ca-ca-cert", + "resource-info-fetcher-credentials", + ] { + assert!( + volumes.contains(&expected.to_owned()), + "missing volume {expected}" + ); + } + + let uif = uif_container(&ds); + assert_eq!( + mount_path(&uif, "user-info-fetcher-credentials"), + "/stackable/credentials" + ); + assert_eq!(read_only(&uif, "user-info-fetcher-credentials"), Some(true)); + } + /// A cluster running both info-fetcher sidecars, so their shared wiring can be asserted in one go. fn cluster_with_both_info_fetchers() -> ValidatedCluster { validated_cluster_from_spec(json!({ diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs index 21efc533..7606257d 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs @@ -3,10 +3,7 @@ use std::str::FromStr; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::{Container, resource_info_fetcher}; use stackable_operator::{ - builder::{ - self, - pod::{PodBuilder, volume::VolumeBuilder}, - }, + builder::pod::{PodBuilder, volume::VolumeBuilder}, commons::tls_verification::TlsClientDetailsError, constant, k8s_openapi::api::core::v1::SecretVolumeSource, @@ -36,14 +33,6 @@ pub enum Error { "failed to build volume or volume mount spec for the Resource Info Fetcher TLS config" ))] TlsVolumeAndMounts { source: TlsClientDetailsError }, - - #[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; @@ -82,12 +71,12 @@ pub fn add_resource_info_fetcher_sidecar( .image(resource_info_fetcher_image) // ...override the image .command(vec!["stackable-opa-resource-info-fetcher".to_string()]) .add_volume_mounts([read_only_mount(CONFIG_VOLUME_NAME.as_ref(), CONFIG_DIR)]) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") // The sidecar writes its file logs below this directory (see // `stackable_rust_cli_env_vars`). They have to land on the shared log volume, // because that is the only place the Vector agent collects them from. .add_volume_mount(LOG_VOLUME_NAME.as_ref(), STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .resources(sidecar_resource_requirements()); match &resource_info.backend { @@ -100,13 +89,17 @@ pub fn add_resource_info_fetcher_sidecar( }) .build(), ) - .context(AddVolumeSnafu)?; + .expect( + "The volume names are statically defined and there should be no duplicates.", + ); cb_rif .add_volume_mounts([read_only_mount( RESOURCE_INFO_FETCHER_CREDENTIALS_VOLUME_NAME.as_ref(), RESOURCE_INFO_FETCHER_CREDENTIALS_DIR, )]) - .context(AddVolumeMountSnafu)?; + .expect( + "The mount paths are statically defined and there should be no duplicates.", + ); data_hub .tls .add_volumes_and_mounts(pb, vec![&mut cb_rif]) diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs index ba2348c9..3a43dff5 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs @@ -3,10 +3,7 @@ use std::str::FromStr; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::{Container, user_info_fetcher}; use stackable_operator::{ - builder::{ - self, - pod::{PodBuilder, volume::VolumeBuilder}, - }, + builder::pod::{PodBuilder, volume::VolumeBuilder}, commons::{ secret_class::{ SecretClassVolume, SecretClassVolumeProvisionParts, SecretClassVolumeScope, @@ -42,21 +39,6 @@ constant!(KRB5CCNAME: EnvVarName = "KRB5CCNAME"); #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("failed to build volume spec for the User Info Fetcher TLS config"))] - KerberosVolume { - source: stackable_operator::builder::pod::Error, - }, - - #[snafu(display("failed to build volume mount spec for the User Info Fetcher TLS config"))] - KerberosVolumeMount { - source: stackable_operator::builder::pod::container::Error, - }, - - #[snafu(display("failed to convert the User Info Fetcher Kerberos SecretClass into a volume"))] - ConvertKerberosSecretClassVolume { - source: stackable_operator::commons::secret_class::SecretClassVolumeError, - }, - #[snafu(display( "failed to build volume or volume mount spec for the User Info Fetcher TLS config" ))] @@ -66,14 +48,6 @@ pub enum Error { "failed to build volume or volume mount spec for the User Info Fetcher LDAP config" ))] LdapVolumeAndMounts { source: ldap::v1alpha1::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; @@ -112,12 +86,12 @@ pub fn add_user_info_fetcher_sidecar( .image(user_info_fetcher_image) // ...override the image .command(vec!["stackable-opa-user-info-fetcher".to_string()]) .add_volume_mounts([read_only_mount(CONFIG_VOLUME_NAME.as_ref(), CONFIG_DIR)]) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") // The sidecar writes its file logs below this directory (see // `stackable_rust_cli_env_vars`). They have to land on the shared log volume, // because that is the only place the Vector agent collects them from. .add_volume_mount(LOG_VOLUME_NAME.as_ref(), STACKABLE_LOG_DIR) - .context(AddVolumeMountSnafu)? + .expect("The mount paths are statically defined and there should be no duplicates.") .resources(sidecar_resource_requirements()); match &user_info.backend { @@ -139,15 +113,21 @@ pub fn add_user_info_fetcher_sidecar( // The user-info-fetcher needs both the keytab (private) and the Kerberos config (public). SecretClassVolumeProvisionParts::PublicPrivate, ) - .context(ConvertKerberosSecretClassVolumeSnafu)?, + .expect( + "The annotation keys are static and annotation values cannot be invalid.", + ), ) - .context(KerberosVolumeSnafu)?; + .expect( + "The volume names are statically defined and there should be no duplicates.", + ); cb_user_info_fetcher .add_volume_mounts([read_only_mount( USER_INFO_FETCHER_KERBEROS_VOLUME_NAME.as_ref(), USER_INFO_FETCHER_KERBEROS_DIR, )]) - .context(KerberosVolumeMountSnafu)?; + .expect( + "The mount paths are statically defined and there should be no duplicates.", + ); env_vars = env_vars .with_value( &KRB5_CONFIG, @@ -171,13 +151,17 @@ pub fn add_user_info_fetcher_sidecar( }) .build(), ) - .context(AddVolumeSnafu)?; + .expect( + "The volume names are statically defined and there should be no duplicates.", + ); cb_user_info_fetcher .add_volume_mounts([read_only_mount( USER_INFO_FETCHER_CREDENTIALS_VOLUME_NAME.as_ref(), USER_INFO_FETCHER_CREDENTIALS_DIR, )]) - .context(AddVolumeMountSnafu)?; + .expect( + "The mount paths are statically defined and there should be no duplicates.", + ); keycloak .tls .add_volumes_and_mounts(pb, vec![&mut cb_user_info_fetcher]) @@ -192,13 +176,17 @@ pub fn add_user_info_fetcher_sidecar( }) .build(), ) - .context(AddVolumeSnafu)?; + .expect( + "The volume names are statically defined and there should be no duplicates.", + ); cb_user_info_fetcher .add_volume_mounts([read_only_mount( USER_INFO_FETCHER_CREDENTIALS_VOLUME_NAME.as_ref(), USER_INFO_FETCHER_CREDENTIALS_DIR, )]) - .context(AddVolumeMountSnafu)?; + .expect( + "The mount paths are statically defined and there should be no duplicates.", + ); TlsClientDetails { tls: entra.tls.clone(), diff --git a/rust/operator-binary/src/controller/build/resource/discovery.rs b/rust/operator-binary/src/controller/build/resource/discovery.rs index ae09bec7..f2334e8b 100644 --- a/rust/operator-binary/src/controller/build/resource/discovery.rs +++ b/rust/operator-binary/src/controller/build/resource/discovery.rs @@ -1,5 +1,4 @@ //! Builds the discovery [`ConfigMap`] clients use to connect to an `OpaCluster`. -use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::OpaRole; use stackable_operator::{ builder::configmap::ConfigMapBuilder, k8s_openapi::api::core::v1::ConfigMap, @@ -12,22 +11,12 @@ use crate::controller::{ build::{object_meta, recommended_labels_for_role_resources}, }; -#[derive(Snafu, Debug)] -pub enum Error { - #[snafu(display("failed to build ConfigMap"))] - BuildConfigMap { - source: stackable_operator::builder::configmap::Error, - }, -} - -type Result = std::result::Result; - /// Builds the discovery [`ConfigMap`] containing the URL (and, when TLS is enabled, the secret /// class) clients need to connect to the cluster. pub fn build_discovery_config_map( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, -) -> Result { +) -> ConfigMap { let (scheme, port) = if cluster.is_tls_enabled() { ("https", APP_TLS_PORT) } else { @@ -57,7 +46,9 @@ pub fn build_discovery_config_map( cm_builder.add_data("OPA_SECRET_CLASS", tls.server_secret_class.to_string()); } - cm_builder.build().context(BuildConfigMapSnafu) + cm_builder + .build() + .expect("The ConfigMap metadata is set in this function.") } #[cfg(test)] @@ -82,7 +73,7 @@ mod tests { "servers": { "roleGroups": { "default": {} } }, })); - let cm = build_discovery_config_map(&validated, &cluster_info()).unwrap(); + let cm = build_discovery_config_map(&validated, &cluster_info()); let data = cm.data.unwrap(); assert_eq!( @@ -100,7 +91,7 @@ mod tests { "servers": { "roleGroups": { "default": {} } }, })); - let cm = build_discovery_config_map(&validated, &cluster_info()).unwrap(); + let cm = build_discovery_config_map(&validated, &cluster_info()); let data = cm.data.unwrap(); assert_eq!( diff --git a/rust/operator-binary/src/crd/user_info_fetcher/mod.rs b/rust/operator-binary/src/crd/user_info_fetcher/mod.rs index 3de2d163..022a61bb 100644 --- a/rust/operator-binary/src/crd/user_info_fetcher/mod.rs +++ b/rust/operator-binary/src/crd/user_info_fetcher/mod.rs @@ -7,6 +7,7 @@ use stackable_operator::{ secret_class::SecretClassVolume, tls_verification::{CaCert, Tls, TlsClientDetails, TlsServerVerification, TlsVerification}, }, + constant, schemars::{self, JsonSchema}, v2::types::kubernetes::{SecretClassName, SecretName}, versioned::versioned, @@ -215,12 +216,15 @@ fn default_root_path() -> String { "/".to_string() } +constant!(ENTRA_DEFAULT_TOKEN_HOSTNAME: HostName = "login.microsoft.com"); +constant!(ENTRA_DEFAULT_USER_INFO_HOSTNAME: HostName = "graph.microsoft.com"); + fn entra_default_token_hostname() -> HostName { - HostName::from_str("login.microsoft.com").unwrap() + ENTRA_DEFAULT_TOKEN_HOSTNAME.clone() } fn entra_default_user_info_hostname() -> HostName { - HostName::from_str("graph.microsoft.com").unwrap() + ENTRA_DEFAULT_USER_INFO_HOSTNAME.clone() } fn default_tls_web_pki() -> Option { @@ -246,3 +250,15 @@ fn openldap_default_user_name_attribute() -> String { fn openldap_default_group_member_attribute() -> String { "member".to_string() } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_constants() { + // Test that dereferencing the constants does not panic. + let _ = *ENTRA_DEFAULT_TOKEN_HOSTNAME; + let _ = *ENTRA_DEFAULT_USER_INFO_HOSTNAME; + } +} From 577d51d22fe5c8c172d6bfb6a4792c791929df67 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 14:15:06 +0200 Subject: [PATCH 2/8] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7d5f594..49e7566e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ All notable changes to this project will be documented in this file. deletion is required ([#880]). - The operator now watches all resources that it creates and early-exits the reconcile action when the cluster is marked for deletion ([#882]). +- Make operations infallible where appropriate ([#886]). ### Fixed @@ -81,6 +82,7 @@ All notable changes to this project will be documented in this file. [#872]: https://github.com/stackabletech/opa-operator/pull/872 [#880]: https://github.com/stackabletech/opa-operator/pull/880 [#882]: https://github.com/stackabletech/opa-operator/pull/882 +[#886]: https://github.com/stackabletech/opa-operator/pull/886 ## [26.7.0] - 2026-07-21 From f48d4eff4fe84c6786d44c73c9515532db4bda5b Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 14:24:30 +0200 Subject: [PATCH 3/8] updated comment --- .../src/controller/build/resource/daemonset/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 2db5e5f9..116db647 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -483,11 +483,11 @@ pub fn build_server_rolegroup_daemonset( .expect("The volume names are statically defined and there should be no duplicates."); } - // Both sidecars add their statically named volumes (`kerberos`, `*-credentials`) to `pb` with - // `expect`, and the TLS/LDAP helpers from operator-rs add volumes named after user-supplied - // SecretClasses fallibly. The user-info-fetcher's SecretClass-derived volumes precede the - // resource-info-fetcher's static one, which is fine: the derived names always end in `-ca-cert` - // or `-bind-credentials` and so can never equal a static volume name. + // Both sidecars add their statically named volumes with `expect`, and the TLS/LDAP helpers + // from operator-rs add volumes named after user-supplied SecretClasses fallibly. The + // user-info-fetcher's SecretClass-derived volumes precede the resource-info-fetcher's + // static one, which is fine: the derived names always end in `-ca-cert` or + // `-bind-credentials` and so can never equal a static volume name. add_user_info_fetcher_sidecar( &mut pb, cluster, From 9e5dd74d52174cb948d74fa5040b264d45e87bfb Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 14:26:54 +0200 Subject: [PATCH 4/8] altered comment again --- .../src/controller/build/resource/daemonset/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 116db647..89b492ef 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -487,7 +487,8 @@ pub fn build_server_rolegroup_daemonset( // from operator-rs add volumes named after user-supplied SecretClasses fallibly. The // user-info-fetcher's SecretClass-derived volumes precede the resource-info-fetcher's // static one, which is fine: the derived names always end in `-ca-cert` or - // `-bind-credentials` and so can never equal a static volume name. + // `-bind-credentials` and so can never equal a static volume name (the alternative would be + // to split both calls into two parts, static and derived). add_user_info_fetcher_sidecar( &mut pb, cluster, From 6cd0b5dd93eda73f6efb820d2cebd87804fa9aa6 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 7 Sep 2026 16:33:20 +0200 Subject: [PATCH 5/8] add panic docs to helper functions --- .../build/resource/daemonset/resource_info_fetcher.rs | 7 +++++++ .../build/resource/daemonset/user_info_fetcher.rs | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs index 7606257d..d4a8d249 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs @@ -37,6 +37,13 @@ pub enum Error { type Result = std::result::Result; +/// Adds the Resource Info Fetcher sidecar container to the given [`PodBuilder`]. +/// +/// # 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_resource_info_fetcher_sidecar( pb: &mut PodBuilder, cluster: &ValidatedCluster, diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs index 3a43dff5..6c53ae4a 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs @@ -52,6 +52,13 @@ pub enum Error { type Result = std::result::Result; +/// Adds the User Info Fetcher sidecar container to the given [`PodBuilder`]. +/// +/// # 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_user_info_fetcher_sidecar( pb: &mut PodBuilder, cluster: &ValidatedCluster, From 0db43f63b46ece600fb29ee34606856077f4d311 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 8 Sep 2026 17:39:11 +0200 Subject: [PATCH 6/8] revert expects where checked data is not static/explicit --- CHANGELOG.md | 2 +- rust/operator-binary/src/controller/build.rs | 5 +- .../controller/build/resource/config_map.rs | 24 ++++--- .../build/resource/daemonset/mod.rs | 29 +++++---- .../daemonset/resource_info_fetcher.rs | 29 +++++---- .../resource/daemonset/user_info_fetcher.rs | 64 ++++++++++--------- .../controller/build/resource/discovery.rs | 21 ++++-- 7 files changed, 100 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49e7566e..8e9d9e2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,7 +55,7 @@ All notable changes to this project will be documented in this file. deletion is required ([#880]). - The operator now watches all resources that it creates and early-exits the reconcile action when the cluster is marked for deletion ([#882]). -- Make operations infallible where appropriate ([#886]). +- Make operations infallible where dependent on static inputs ([#886]). ### Fixed diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index db72de39..75cdf396 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -48,6 +48,9 @@ pub enum Error { source: resource::daemonset::Error, role_group: RoleGroupName, }, + + #[snafu(display("failed to build the discovery ConfigMap"))] + Discovery { source: resource::discovery::Error }, } /// Builds every Kubernetes resource for the given validated cluster. @@ -102,7 +105,7 @@ pub fn build( } // The cluster-level discovery ConfigMap. - config_maps.push(build_discovery_config_map(cluster, cluster_info)); + config_maps.push(build_discovery_config_map(cluster, cluster_info).context(DiscoverySnafu)?); Ok(KubernetesResources { daemon_sets, 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 bd501bba..321aea1f 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -22,15 +22,21 @@ use crate::controller::{ #[derive(Snafu, Debug)] pub enum Error { #[snafu(display("failed to build config.json"))] - ConfigJson { source: config_json::Error }, + BuildConfigJson { source: config_json::Error }, #[snafu(display("failed to build user-info-fetcher.json"))] - UserInfoFetcher { source: user_info_fetcher::Error }, + BuildUserInfoFetcher { source: user_info_fetcher::Error }, #[snafu(display("failed to build resource-info-fetcher.json"))] - ResourceInfoFetcher { + BuildResourceInfoFetcher { source: resource_info_fetcher::Error, }, + + #[snafu(display("failed to assemble ConfigMap for role group {role_group}"))] + Assemble { + source: stackable_operator::builder::configmap::Error, + role_group: RoleGroupName, + }, } type Result = std::result::Result; @@ -60,19 +66,19 @@ pub fn build_rolegroup_config_map( cm_builder.metadata(metadata).add_data( ConfigFileName::ConfigJson.to_string(), config_json::build(&rolegroup_config.config, &rolegroup_config.config_overrides) - .context(ConfigJsonSnafu)?, + .context(BuildConfigJsonSnafu)?, ); if let Some(user_info) = &cluster.cluster_config.user_info { cm_builder.add_data( ConfigFileName::UserInfoFetcher.to_string(), - user_info_fetcher::build(user_info).context(UserInfoFetcherSnafu)?, + user_info_fetcher::build(user_info).context(BuildUserInfoFetcherSnafu)?, ); } if let Some(resource_info) = &cluster.cluster_config.resource_info { cm_builder.add_data( ConfigFileName::ResourceInfoFetcher.to_string(), - resource_info_fetcher::build(resource_info).context(ResourceInfoFetcherSnafu)?, + resource_info_fetcher::build(resource_info).context(BuildResourceInfoFetcherSnafu)?, ); } @@ -83,9 +89,9 @@ pub fn build_rolegroup_config_map( ); } - Ok(cm_builder - .build() - .expect("The ConfigMap metadata is set in this function.")) + cm_builder.build().with_context(|_| AssembleSnafu { + role_group: role_group_name.clone(), + }) } #[cfg(test)] diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 89b492ef..29f77c29 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -8,6 +8,7 @@ use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::{Container, DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT, OpaRole}; use stackable_operator::{ builder::{ + self, meta::ObjectMetaBuilder, pod::{ PodBuilder, @@ -173,6 +174,14 @@ pub enum Error { source: crate::operations::graceful_shutdown::Error, }, + #[snafu(display("failed to add needed volume"))] + AddVolume { source: builder::pod::Error }, + + #[snafu(display("failed to build TLS volume"))] + TlsVolumeBuild { + source: builder::pod::volume::SecretOperatorVolumeSourceBuilderError, + }, + #[snafu(display("failed to build User Info Fetcher sidecar"))] BuildUserInfoFetcherSidecar { source: user_info_fetcher::Error }, @@ -423,13 +432,13 @@ pub fn build_server_rolegroup_daemonset( ) .build(), ) - .expect("The volume names are statically defined and there should be no duplicates.") + .context(AddVolumeSnafu)? .add_volume( VolumeBuilder::new(BUNDLES_VOLUME_NAME.as_ref()) .with_empty_dir(None::, None) .build(), ) - .expect("The volume names are statically defined and there should be no duplicates.") + .context(AddVolumeSnafu)? .add_volume( VolumeBuilder::new(LOG_VOLUME_NAME.as_ref()) .empty_dir(EmptyDirVolumeSource { @@ -438,7 +447,7 @@ pub fn build_server_rolegroup_daemonset( }) .build(), ) - .expect("The volume names are statically defined and there should be no duplicates.") + .context(AddVolumeSnafu)? .service_account_name( cluster .cluster_resource_names() @@ -474,21 +483,13 @@ pub fn build_server_rolegroup_daemonset( .to_string(), ) .build() - .expect( - "The annotation keys are static and annotation values cannot be invalid.", - ), + .context(TlsVolumeBuildSnafu)?, ) .build(), ) - .expect("The volume names are statically defined and there should be no duplicates."); + .context(AddVolumeSnafu)?; } - // Both sidecars add their statically named volumes with `expect`, and the TLS/LDAP helpers - // from operator-rs add volumes named after user-supplied SecretClasses fallibly. The - // user-info-fetcher's SecretClass-derived volumes precede the resource-info-fetcher's - // static one, which is fine: the derived names always end in `-ca-cert` or - // `-bind-credentials` and so can never equal a static volume name (the alternative would be - // to split both calls into two parts, static and derived). add_user_info_fetcher_sidecar( &mut pb, cluster, @@ -1262,7 +1263,7 @@ mod tests { /// The Entra backend projects its client credentials Secret like the Keycloak backend does. Its /// TLS CA volume is named after the user's SecretClass and is added to the pod *before* the /// resource-info-fetcher's statically named credentials volume, so this also pins that the two - /// cannot collide (see the comment above the sidecar calls in `build_server_rolegroup_daemonset`). + /// do not collide (the derived name ends in `-ca-cert`). #[test] fn user_info_fetcher_entra_backend_mounts_client_credentials_next_to_resource_info_fetcher() { let ds = build(&validated_cluster_from_spec(json!({ diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs index d4a8d249..94bafe3f 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs @@ -3,7 +3,10 @@ use std::str::FromStr; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::{Container, resource_info_fetcher}; use stackable_operator::{ - builder::pod::{PodBuilder, volume::VolumeBuilder}, + builder::{ + self, + pod::{PodBuilder, volume::VolumeBuilder}, + }, commons::tls_verification::TlsClientDetailsError, constant, k8s_openapi::api::core::v1::SecretVolumeSource, @@ -33,17 +36,19 @@ pub enum Error { "failed to build volume or volume mount spec for the Resource Info Fetcher TLS config" ))] TlsVolumeAndMounts { source: TlsClientDetailsError }, + + #[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; /// Adds the Resource Info Fetcher sidecar container to the given [`PodBuilder`]. -/// -/// # 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_resource_info_fetcher_sidecar( pb: &mut PodBuilder, cluster: &ValidatedCluster, @@ -78,7 +83,7 @@ pub fn add_resource_info_fetcher_sidecar( .image(resource_info_fetcher_image) // ...override the image .command(vec!["stackable-opa-resource-info-fetcher".to_string()]) .add_volume_mounts([read_only_mount(CONFIG_VOLUME_NAME.as_ref(), CONFIG_DIR)]) - .expect("The mount paths are statically defined and there should be no duplicates.") + .context(AddVolumeMountSnafu)? // The sidecar writes its file logs below this directory (see // `stackable_rust_cli_env_vars`). They have to land on the shared log volume, // because that is the only place the Vector agent collects them from. @@ -96,17 +101,13 @@ pub fn add_resource_info_fetcher_sidecar( }) .build(), ) - .expect( - "The volume names are statically defined and there should be no duplicates.", - ); + .context(AddVolumeSnafu)?; cb_rif .add_volume_mounts([read_only_mount( RESOURCE_INFO_FETCHER_CREDENTIALS_VOLUME_NAME.as_ref(), RESOURCE_INFO_FETCHER_CREDENTIALS_DIR, )]) - .expect( - "The mount paths are statically defined and there should be no duplicates.", - ); + .context(AddVolumeMountSnafu)?; data_hub .tls .add_volumes_and_mounts(pb, vec![&mut cb_rif]) diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs index 6c53ae4a..491e178b 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs @@ -3,7 +3,10 @@ use std::str::FromStr; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::{Container, user_info_fetcher}; use stackable_operator::{ - builder::pod::{PodBuilder, volume::VolumeBuilder}, + builder::{ + self, + pod::{PodBuilder, volume::VolumeBuilder}, + }, commons::{ secret_class::{ SecretClassVolume, SecretClassVolumeProvisionParts, SecretClassVolumeScope, @@ -39,6 +42,21 @@ constant!(KRB5CCNAME: EnvVarName = "KRB5CCNAME"); #[derive(Snafu, Debug)] pub enum Error { + #[snafu(display("failed to build volume spec for the User Info Fetcher TLS config"))] + KerberosVolume { + source: stackable_operator::builder::pod::Error, + }, + + #[snafu(display("failed to build volume mount spec for the User Info Fetcher TLS config"))] + KerberosVolumeMount { + source: stackable_operator::builder::pod::container::Error, + }, + + #[snafu(display("failed to convert the User Info Fetcher Kerberos SecretClass into a volume"))] + ConvertKerberosSecretClassVolume { + source: stackable_operator::commons::secret_class::SecretClassVolumeError, + }, + #[snafu(display( "failed to build volume or volume mount spec for the User Info Fetcher TLS config" ))] @@ -48,17 +66,19 @@ pub enum Error { "failed to build volume or volume mount spec for the User Info Fetcher LDAP config" ))] LdapVolumeAndMounts { source: ldap::v1alpha1::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; /// Adds the User Info Fetcher sidecar container to the given [`PodBuilder`]. -/// -/// # 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_user_info_fetcher_sidecar( pb: &mut PodBuilder, cluster: &ValidatedCluster, @@ -93,7 +113,7 @@ pub fn add_user_info_fetcher_sidecar( .image(user_info_fetcher_image) // ...override the image .command(vec!["stackable-opa-user-info-fetcher".to_string()]) .add_volume_mounts([read_only_mount(CONFIG_VOLUME_NAME.as_ref(), CONFIG_DIR)]) - .expect("The mount paths are statically defined and there should be no duplicates.") + .context(AddVolumeMountSnafu)? // The sidecar writes its file logs below this directory (see // `stackable_rust_cli_env_vars`). They have to land on the shared log volume, // because that is the only place the Vector agent collects them from. @@ -120,21 +140,15 @@ pub fn add_user_info_fetcher_sidecar( // The user-info-fetcher needs both the keytab (private) and the Kerberos config (public). SecretClassVolumeProvisionParts::PublicPrivate, ) - .expect( - "The annotation keys are static and annotation values cannot be invalid.", - ), + .context(ConvertKerberosSecretClassVolumeSnafu)?, ) - .expect( - "The volume names are statically defined and there should be no duplicates.", - ); + .context(KerberosVolumeSnafu)?; cb_user_info_fetcher .add_volume_mounts([read_only_mount( USER_INFO_FETCHER_KERBEROS_VOLUME_NAME.as_ref(), USER_INFO_FETCHER_KERBEROS_DIR, )]) - .expect( - "The mount paths are statically defined and there should be no duplicates.", - ); + .context(KerberosVolumeMountSnafu)?; env_vars = env_vars .with_value( &KRB5_CONFIG, @@ -158,17 +172,13 @@ pub fn add_user_info_fetcher_sidecar( }) .build(), ) - .expect( - "The volume names are statically defined and there should be no duplicates.", - ); + .context(AddVolumeSnafu)?; cb_user_info_fetcher .add_volume_mounts([read_only_mount( USER_INFO_FETCHER_CREDENTIALS_VOLUME_NAME.as_ref(), USER_INFO_FETCHER_CREDENTIALS_DIR, )]) - .expect( - "The mount paths are statically defined and there should be no duplicates.", - ); + .context(AddVolumeMountSnafu)?; keycloak .tls .add_volumes_and_mounts(pb, vec![&mut cb_user_info_fetcher]) @@ -183,17 +193,13 @@ pub fn add_user_info_fetcher_sidecar( }) .build(), ) - .expect( - "The volume names are statically defined and there should be no duplicates.", - ); + .context(AddVolumeSnafu)?; cb_user_info_fetcher .add_volume_mounts([read_only_mount( USER_INFO_FETCHER_CREDENTIALS_VOLUME_NAME.as_ref(), USER_INFO_FETCHER_CREDENTIALS_DIR, )]) - .expect( - "The mount paths are statically defined and there should be no duplicates.", - ); + .context(AddVolumeMountSnafu)?; TlsClientDetails { tls: entra.tls.clone(), diff --git a/rust/operator-binary/src/controller/build/resource/discovery.rs b/rust/operator-binary/src/controller/build/resource/discovery.rs index f2334e8b..ae09bec7 100644 --- a/rust/operator-binary/src/controller/build/resource/discovery.rs +++ b/rust/operator-binary/src/controller/build/resource/discovery.rs @@ -1,4 +1,5 @@ //! Builds the discovery [`ConfigMap`] clients use to connect to an `OpaCluster`. +use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::OpaRole; use stackable_operator::{ builder::configmap::ConfigMapBuilder, k8s_openapi::api::core::v1::ConfigMap, @@ -11,12 +12,22 @@ use crate::controller::{ build::{object_meta, recommended_labels_for_role_resources}, }; +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display("failed to build ConfigMap"))] + BuildConfigMap { + source: stackable_operator::builder::configmap::Error, + }, +} + +type Result = std::result::Result; + /// Builds the discovery [`ConfigMap`] containing the URL (and, when TLS is enabled, the secret /// class) clients need to connect to the cluster. pub fn build_discovery_config_map( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, -) -> ConfigMap { +) -> Result { let (scheme, port) = if cluster.is_tls_enabled() { ("https", APP_TLS_PORT) } else { @@ -46,9 +57,7 @@ pub fn build_discovery_config_map( cm_builder.add_data("OPA_SECRET_CLASS", tls.server_secret_class.to_string()); } - cm_builder - .build() - .expect("The ConfigMap metadata is set in this function.") + cm_builder.build().context(BuildConfigMapSnafu) } #[cfg(test)] @@ -73,7 +82,7 @@ mod tests { "servers": { "roleGroups": { "default": {} } }, })); - let cm = build_discovery_config_map(&validated, &cluster_info()); + let cm = build_discovery_config_map(&validated, &cluster_info()).unwrap(); let data = cm.data.unwrap(); assert_eq!( @@ -91,7 +100,7 @@ mod tests { "servers": { "roleGroups": { "default": {} } }, })); - let cm = build_discovery_config_map(&validated, &cluster_info()); + let cm = build_discovery_config_map(&validated, &cluster_info()).unwrap(); let data = cm.data.unwrap(); assert_eq!( From 5e977c26e50f845c4afd1fb3f67e780344420ea2 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 10 Sep 2026 11:17:28 +0200 Subject: [PATCH 7/8] replace container_name with deref impl on container names --- .../build/resource/daemonset/mod.rs | 23 +++----- .../daemonset/resource_info_fetcher.rs | 7 ++- .../resource/daemonset/user_info_fetcher.rs | 5 +- rust/operator-binary/src/crd/mod.rs | 53 +++++++++++++++++-- 4 files changed, 63 insertions(+), 25 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 29f77c29..8b1f5506 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -193,14 +193,6 @@ pub enum Error { type Result = std::result::Result; -/// The typed [`ContainerName`] for a [`Container`]. The enum's `Display` values are all valid -/// container names, so this conversion is infallible. -fn container_name(container: &Container) -> ContainerName { - ContainerName::from_str(&container.to_string()) - .expect("Container enum variants are valid container names") -} - -/// The CPU and memory requests/limits shared by the bundle-builder and user-info-fetcher sidecars. /// A [`VolumeMount`] the container may only read from. /// /// Used for the config and credential volumes of the info-fetcher sidecars: they hold data the @@ -216,6 +208,7 @@ fn read_only_mount(name: &str, mount_path: &str) -> VolumeMount { } } +/// The CPU and memory requests/limits shared by the bundle-builder and user-info-fetcher sidecars. fn sidecar_resource_requirements() -> ResourceRequirements { ResourceRequirementsBuilder::new() .with_cpu_request("100m") @@ -290,14 +283,14 @@ pub fn build_server_rolegroup_daemonset( let mut pb = PodBuilder::new(); - let prepare_container_name = container_name(&Container::Prepare); - let mut cb_prepare = new_container_builder(&prepare_container_name); + let prepare_container_name: &ContainerName = &Container::Prepare; + let mut cb_prepare = new_container_builder(prepare_container_name); - let bundle_builder_container_name = container_name(&Container::BundleBuilder); - let mut cb_bundle_builder = new_container_builder(&bundle_builder_container_name); + let bundle_builder_container_name: &ContainerName = &Container::BundleBuilder; + let mut cb_bundle_builder = new_container_builder(bundle_builder_container_name); - let opa_container_name = container_name(&Container::Opa); - let mut cb_opa = new_container_builder(&opa_container_name); + let opa_container_name: &ContainerName = &Container::Opa; + let mut cb_opa = new_container_builder(opa_container_name); cb_prepare .image_from_product_image(resolved_product_image) @@ -511,7 +504,7 @@ pub fn build_server_rolegroup_daemonset( // the Vector agent is enabled and the aggregator discovery ConfigMap name is valid. if let Some(vector_log_config) = &merged_config.logging.vector_container { pb.add_container(vector_container( - &container_name(&Container::Vector), + &Container::Vector, resolved_product_image, vector_log_config, &cluster.role_group_resource_names(role_group_name), diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs index 94bafe3f..19d8c2fe 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs @@ -20,8 +20,8 @@ use crate::controller::{ self, resource::daemonset::{ CONFIG_DIR, CONFIG_VOLUME_NAME, LOG_VOLUME_NAME, RESOURCE_INFO_FETCHER_CREDENTIALS_DIR, - RESOURCE_INFO_FETCHER_CREDENTIALS_VOLUME_NAME, STACKABLE_LOG_DIR, container_name, - read_only_mount, sidecar_container_log_level, sidecar_resource_requirements, + RESOURCE_INFO_FETCHER_CREDENTIALS_VOLUME_NAME, STACKABLE_LOG_DIR, read_only_mount, + sidecar_container_log_level, sidecar_resource_requirements, stackable_rust_cli_env_vars, }, }, @@ -57,8 +57,7 @@ pub fn add_resource_info_fetcher_sidecar( cluster_info: &KubernetesClusterInfo, ) -> Result<()> { if let Some(resource_info) = &cluster.cluster_config.resource_info { - let rif_container_name = container_name(&Container::ResourceInfoFetcher); - let mut cb_rif = new_container_builder(&rif_container_name); + let mut cb_rif = new_container_builder(&Container::ResourceInfoFetcher); // All operator-set environment variables of the resource-info-fetcher container, collected // into an `EnvVarSet` so that every name occurs only once. diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs index 491e178b..300d73da 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs @@ -27,7 +27,7 @@ use crate::controller::{ resource::daemonset::{ CONFIG_DIR, CONFIG_VOLUME_NAME, LOG_VOLUME_NAME, STACKABLE_LOG_DIR, USER_INFO_FETCHER_CREDENTIALS_DIR, USER_INFO_FETCHER_CREDENTIALS_VOLUME_NAME, - USER_INFO_FETCHER_KERBEROS_DIR, USER_INFO_FETCHER_KERBEROS_VOLUME_NAME, container_name, + USER_INFO_FETCHER_KERBEROS_DIR, USER_INFO_FETCHER_KERBEROS_VOLUME_NAME, read_only_mount, sidecar_container_log_level, sidecar_resource_requirements, stackable_rust_cli_env_vars, }, @@ -87,8 +87,7 @@ pub fn add_user_info_fetcher_sidecar( cluster_info: &KubernetesClusterInfo, ) -> Result<()> { if let Some(user_info) = &cluster.cluster_config.user_info { - let user_info_fetcher_container_name = container_name(&Container::UserInfoFetcher); - let mut cb_user_info_fetcher = new_container_builder(&user_info_fetcher_container_name); + let mut cb_user_info_fetcher = new_container_builder(&Container::UserInfoFetcher); // All operator-set environment variables of the user-info-fetcher container, collected // into an `EnvVarSet` so that every name occurs only once. The backend match below may diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index bb0ac97c..4b98649c 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -25,7 +25,7 @@ use stackable_operator::{ config_overrides::JsonConfigOverrides, role_utils::{GenericCommonConfig, Role}, types::{ - kubernetes::{ConfigMapName, SecretClassName}, + kubernetes::{ConfigMapName, ContainerName, SecretClassName}, operator::RoleName, }, }, @@ -219,6 +219,30 @@ pub enum Container { ResourceInfoFetcher, } +// Typed container names. They must match the strum `Display` (kebab-case) of the variants above, +// which is pinned by a unit test. +constant!(PREPARE_CONTAINER_NAME: ContainerName = "prepare"); +constant!(VECTOR_CONTAINER_NAME: ContainerName = "vector"); +constant!(BUNDLE_BUILDER_CONTAINER_NAME: ContainerName = "bundle-builder"); +constant!(OPA_CONTAINER_NAME: ContainerName = "opa"); +constant!(USER_INFO_FETCHER_CONTAINER_NAME: ContainerName = "user-info-fetcher"); +constant!(RESOURCE_INFO_FETCHER_CONTAINER_NAME: ContainerName = "resource-info-fetcher"); + +impl Deref for Container { + type Target = ContainerName; + + fn deref(&self) -> &Self::Target { + match self { + Container::Prepare => &PREPARE_CONTAINER_NAME, + Container::Vector => &VECTOR_CONTAINER_NAME, + Container::BundleBuilder => &BUNDLE_BUILDER_CONTAINER_NAME, + Container::Opa => &OPA_CONTAINER_NAME, + Container::UserInfoFetcher => &USER_INFO_FETCHER_CONTAINER_NAME, + Container::ResourceInfoFetcher => &RESOURCE_INFO_FETCHER_CONTAINER_NAME, + } + } +} + // NOTE (@Techassi): This struct can currently NOT be versioned because it is used via Role which // makes it incredible hard to implement the From trait for conversions. #[derive(Clone, Debug, Default, Fragment, JsonSchema, PartialEq)] @@ -331,14 +355,37 @@ impl HasStatusCondition for v1alpha2::OpaCluster { #[cfg(test)] mod tests { use indoc::formatdoc; - use stackable_operator::versioned::test_utils::RoundtripTestData; + use stackable_operator::{ + v2::types::kubernetes::ContainerName, versioned::test_utils::RoundtripTestData, + }; + use strum::IntoEnumIterator; - use super::{SERVER_ROLE_NAME, v1alpha1, v1alpha2}; + use super::{ + BUNDLE_BUILDER_CONTAINER_NAME, Container, OPA_CONTAINER_NAME, PREPARE_CONTAINER_NAME, + RESOURCE_INFO_FETCHER_CONTAINER_NAME, SERVER_ROLE_NAME, USER_INFO_FETCHER_CONTAINER_NAME, + VECTOR_CONTAINER_NAME, v1alpha1, v1alpha2, + }; #[test] fn test_constants() { // Test that dereferencing the constants does not panic. let _ = *SERVER_ROLE_NAME; + let _ = *PREPARE_CONTAINER_NAME; + let _ = *VECTOR_CONTAINER_NAME; + let _ = *BUNDLE_BUILDER_CONTAINER_NAME; + let _ = *OPA_CONTAINER_NAME; + let _ = *USER_INFO_FETCHER_CONTAINER_NAME; + let _ = *RESOURCE_INFO_FETCHER_CONTAINER_NAME; + } + + /// The typed container names behind `Container`'s `Deref` must agree with its strum + /// `Display`, which the logging configuration still uses as the per-container key. + #[test] + fn container_names_match_display() { + for container in Container::iter() { + let container_name: &ContainerName = &container; + assert_eq!(container_name.to_string(), container.to_string()); + } } impl RoundtripTestData for v1alpha1::OpaClusterSpec { From 42d4c109c0f346ee80201590ca5b87dfdd948324 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 10 Sep 2026 11:30:06 +0200 Subject: [PATCH 8/8] correct test comment --- .../src/controller/build/resource/daemonset/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 8b1f5506..dfd396fb 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -1254,9 +1254,9 @@ mod tests { } /// The Entra backend projects its client credentials Secret like the Keycloak backend does. Its - /// TLS CA volume is named after the user's SecretClass and is added to the pod *before* the - /// resource-info-fetcher's statically named credentials volume, so this also pins that the two - /// do not collide (the derived name ends in `-ca-cert`). + /// TLS CA volume is named `-ca-cert` after the user's SecretClass, so this also + /// checks that a SecretClass-derived name coexists with the statically named credentials + /// volumes of both info-fetchers in one pod. #[test] fn user_info_fetcher_entra_backend_mounts_client_credentials_next_to_resource_info_fetcher() { let ds = build(&validated_cluster_from_spec(json!({