diff --git a/CHANGELOG.md b/CHANGELOG.md index b7d5f594..8e9d9e2a 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 dependent on static inputs ([#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 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/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 36d93b63..dfd396fb 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -177,11 +177,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, - }, - #[snafu(display("failed to build TLS volume"))] TlsVolumeBuild { source: builder::pod::volume::SecretOperatorVolumeSourceBuilderError, @@ -198,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 @@ -221,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") @@ -295,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) @@ -312,9 +300,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 +325,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 +371,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() { @@ -516,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), @@ -853,6 +841,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 +1253,60 @@ mod tests { ); } + /// The Entra backend projects its client credentials Secret like the Keycloak backend does. Its + /// 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!({ + "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..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, }, }, @@ -48,6 +48,7 @@ pub enum Error { type Result = std::result::Result; +/// Adds the Resource Info Fetcher sidecar container to the given [`PodBuilder`]. pub fn add_resource_info_fetcher_sidecar( pb: &mut PodBuilder, cluster: &ValidatedCluster, @@ -56,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. @@ -87,7 +87,7 @@ pub fn add_resource_info_fetcher_sidecar( // `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 { 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..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, }, @@ -78,6 +78,7 @@ pub enum Error { type Result = std::result::Result; +/// Adds the User Info Fetcher sidecar container to the given [`PodBuilder`]. pub fn add_user_info_fetcher_sidecar( pb: &mut PodBuilder, cluster: &ValidatedCluster, @@ -86,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 @@ -117,7 +117,7 @@ pub fn add_user_info_fetcher_sidecar( // `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 { 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 { 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; + } +}