refactor: remove calculated inputs from Validated* structs

Author: maltesander

rust/operator-binary/src/extensions.rs …ontroller/build/properties/extensions.rsrust/operator-binary/src/extensions.rs renamed to rust/operator-binary/src/controller/build/properties/extensions.rs rust/operator-binary/src/extensions.rs renamed to rust/operator-binary/src/controller/build/properties/extensions.rs

@@ -1,11 +1,14 @@
+//! Builder for the `druid.extensions.loadList` runtime.properties entry.
+//!
+//! The set of Druid extensions to load is derived from the metadata database, TLS, S3 and
+//! authentication settings carried on the validated cluster. This is a build-step concern: it
+//! computes a config output, so it lives here rather than in the validate step.
+
 use std::collections::HashSet;
 
 use tracing::debug;
 
-use crate::{
-    authentication::DruidAuthenticationConfig,
-    crd::{database::MetadataDatabaseConnection, security::DruidTlsSecurity, v1alpha1},
-};
+use crate::{authentication::DruidAuthenticationConfig, crd::database::MetadataDatabaseConnection};
 
 const EXT_S3: &str = "druid-s3-extensions";
 const EXT_KAFKA_INDEXING: &str = "druid-kafka-indexing-service";
@@ -19,9 +22,16 @@ const EXT_HDFS: &str = "druid-hdfs-storage";
 const EXT_SIMPLE_CLIENT_SSL_CONTEXT: &str = "simple-client-sslcontext";
 const ENV_PAC4J: &str = "druid-pac4j";
 
+/// Computes the `druid.extensions.loadList` entries from the validated cluster inputs.
+///
+/// Takes the resolved inputs (metadata database, whether TLS/S3 are in use, the user-supplied
+/// additional extensions and the resolved authentication config) rather than the raw
+/// `DruidCluster`, so it has no dependency on the validate step.
 pub fn get_extension_list(
-    druid: &v1alpha1::DruidCluster,
-    druid_tls_security: &DruidTlsSecurity,
+    metadata_database: &MetadataDatabaseConnection,
+    tls_enabled: bool,
+    uses_s3: bool,
+    additional_extensions: &HashSet<String>,
     druid_auth_settings: &Option<DruidAuthenticationConfig>,
 ) -> Vec<String> {
     let mut extensions = HashSet::from([
@@ -33,7 +43,7 @@ pub fn get_extension_list(
         EXT_HDFS.to_string(),
     ]);
 
-    match druid.spec.cluster_config.metadata_database {
+    match metadata_database {
         MetadataDatabaseConnection::Derby(..) => {} // no additional extensions required
         MetadataDatabaseConnection::Postgresql(..) => {
             extensions.insert(EXT_PSQL_MD_ST.to_string());
@@ -43,27 +53,26 @@ pub fn get_extension_list(
         }
     };
 
-    if druid_tls_security.tls_enabled() {
+    if tls_enabled {
         extensions.insert(EXT_SIMPLE_CLIENT_SSL_CONTEXT.to_string());
     }
 
-    if druid.uses_s3() {
+    if uses_s3 {
         extensions.insert(EXT_S3.to_string());
     }
 
     if let Some(DruidAuthenticationConfig::Oidc { .. }) = druid_auth_settings {
         extensions.insert(ENV_PAC4J.to_string());
     }
 
-    let additional_extensions = druid.spec.cluster_config.additional_extensions.clone();
     if !additional_extensions.is_empty() {
         debug!(
             enabled_extensions = ?extensions,
             ?additional_extensions,
             "Adding user specified additional extensions to list of enabled extensions"
         );
     }
-    extensions.extend(additional_extensions);
+    extensions.extend(additional_extensions.iter().cloned());
 
     let mut extensions = Vec::from_iter(extensions);
     extensions.sort();
@@ -77,7 +86,11 @@ mod tests {
     };
 
     use super::*;
-    use crate::crd::authentication::{AuthenticationClassResolved, AuthenticationClassesResolved};
+    use crate::crd::{
+        authentication::{AuthenticationClassResolved, AuthenticationClassesResolved},
+        security::DruidTlsSecurity,
+        v1alpha1,
+    };
 
     #[test]
     fn test_additional_extensions() {
@@ -94,42 +107,48 @@ mod tests {
             ])
         );
 
+        let druid_tls_security = DruidTlsSecurity::new_from_druid_cluster(
+            &cluster,
+            &AuthenticationClassesResolved {
+                auth_classes: vec![],
+            },
+        );
+
+        let druid_auth_config = Some(
+            DruidAuthenticationConfig::try_from(AuthenticationClassesResolved {
+                auth_classes: vec![AuthenticationClassResolved::Oidc {
+                    auth_class_name: "oidc".to_string(),
+                    provider: oidc::v1alpha1::AuthenticationProvider::new(
+                        "my-oidc-provider".to_string().try_into().unwrap(),
+                        None,
+                        "".to_string(),
+                        TlsClientDetails { tls: None },
+                        "".to_string(),
+                        vec![],
+                        None,
+                    ),
+                    oidc: crate::authentication::oidc::DruidClientAuthenticationOptions {
+                        client_credentials_secret_ref: "".to_string(),
+                        extra_scopes: vec![],
+                        product_specific_fields: oidc::v1alpha1::ClientAuthenticationMethodOption {
+                            client_authentication_method:
+                                oidc::v1alpha1::ClientAuthenticationMethod::default(),
+                        },
+                    },
+                }],
+            })
+            .unwrap()
+            .unwrap(),
+        );
+
         assert_eq!(
             get_extension_list(
-                &cluster,
-                &DruidTlsSecurity::new_from_druid_cluster(
-                    &cluster,
-                    &AuthenticationClassesResolved {
-                        auth_classes: vec![]
-                    }
-                ),
-                &Some(
-                    DruidAuthenticationConfig::try_from(AuthenticationClassesResolved {
-                        auth_classes: vec![AuthenticationClassResolved::Oidc {
-                            auth_class_name: "oidc".to_string(),
-                            provider: oidc::v1alpha1::AuthenticationProvider::new(
-                                "my-oidc-provider".to_string().try_into().unwrap(),
-                                None,
-                                "".to_string(),
-                                TlsClientDetails { tls: None },
-                                "".to_string(),
-                                vec![],
-                                None
-                            ),
-                            oidc: crate::authentication::oidc::DruidClientAuthenticationOptions {
-                                client_credentials_secret_ref: "".to_string(),
-                                extra_scopes: vec![],
-                                product_specific_fields:
-                                    oidc::v1alpha1::ClientAuthenticationMethodOption {
-                                        client_authentication_method:
-                                            oidc::v1alpha1::ClientAuthenticationMethod::default(),
-                                    },
-                            }
-                        }]
-                    })
-                    .unwrap()
-                    .unwrap()
-                )
+                &cluster.spec.cluster_config.metadata_database,
+                druid_tls_security.tls_enabled(),
+                // simple.yaml uses HDFS deep storage and no S3 ingestion.
+                false,
+                &cluster.spec.cluster_config.additional_extensions,
+                &druid_auth_config,
             ),
             [
                 "druid-avro-extensions".to_owned(),

rust/operator-binary/src/controller/build/properties/mod.rs

@@ -1,5 +1,6 @@
 //! Per-file builders for Druid `.properties` files.
 
+pub mod extensions;
 pub mod product_logging;
 pub mod runtime_properties;
 pub mod security_properties;

rust/operator-binary/src/controller/build/resource/config_map.rs

@@ -28,6 +28,7 @@ use crate::{
             jvm::construct_jvm_args,
             properties::{
                 ConfigFileName,
+                extensions::get_extension_list,
                 product_logging::{build_log4j2, vector_config_file_content},
                 runtime_properties, security_properties,
             },
@@ -163,9 +164,16 @@ pub fn build_rolegroup_config_map(
             zk_connstr.to_string(),
         );
 
+        let extensions = get_extension_list(
+            &cluster_config.metadata_database,
+            druid_tls_security.tls_enabled(),
+            cluster_config.uses_s3(),
+            &cluster_config.additional_extensions,
+            druid_auth_config,
+        );
         conf.insert(
             EXTENSIONS_LOADLIST.to_string(),
-            build_string_list(&cluster_config.extensions),
+            build_string_list(&extensions),
         );
 
         if let Some(opa_str) = opa_connstr {
@@ -174,7 +182,10 @@ pub fn build_rolegroup_config_map(
 
         conf.insert(
             crate::crd::database::METADATA_STORAGE_TYPE.to_string(),
-            cluster_config.metadata_storage_type.clone(),
+            cluster_config
+                .metadata_database
+                .as_metadata_storage_type()
+                .to_string(),
         );
 
         conf.insert(

rust/operator-binary/src/controller/validate.rs

@@ -3,7 +3,11 @@
 //! Synchronously validates inputs that don't require a Kubernetes client. Produces
 //! [`ValidatedCluster`], consumed by the rest of `reconcile_druid`.
 
-use std::{borrow::Cow, collections::BTreeMap, str::FromStr};
+use std::{
+    borrow::Cow,
+    collections::{BTreeMap, HashSet},
+    str::FromStr,
+};
 
 use snafu::{ResultExt, Snafu};
 use stackable_operator::{
@@ -35,8 +39,10 @@ use strum::IntoEnumIterator;
 use crate::{
     authentication::DruidAuthenticationConfig,
     controller::{controller_name, dereference::DereferencedObjects, operator_name, product_name},
-    crd::{DeepStorageSpec, DruidRole, security::DruidTlsSecurity, v1alpha1},
-    extensions::get_extension_list,
+    crd::{
+        DeepStorageSpec, DruidRole, database::MetadataDatabaseConnection,
+        security::DruidTlsSecurity, v1alpha1,
+    },
 };
 
 #[derive(Snafu, Debug)]
@@ -88,11 +94,12 @@ pub struct ValidatedClusterConfig {
     pub deep_storage_bucket_name: Option<String>,
     pub druid_tls_security: DruidTlsSecurity,
     pub druid_auth_config: Option<DruidAuthenticationConfig>,
-    /// The `druid.extensions.loadList` entries, resolved from the metadata database, TLS, S3 and
-    /// authentication settings during validation.
-    pub extensions: Vec<String>,
-    /// The `druid.metadata.storage.type` value derived from the configured metadata database.
-    pub metadata_storage_type: String,
+    /// The configured metadata database, carried so the build step can derive the
+    /// `druid.metadata.storage.type` value and the metadata-storage extension.
+    pub metadata_database: MetadataDatabaseConnection,
+    /// User-supplied additional `druid.extensions.loadList` entries, carried so the build step can
+    /// assemble the full extension list.
+    pub additional_extensions: HashSet<String>,
     /// The JDBC connection details (URL plus credential env vars) for the metadata database.
     pub metadata_db_connection: JdbcDatabaseConnectionDetails,
     /// The deep-storage spec, carried so the build step can derive the cluster-level
@@ -104,6 +111,13 @@ pub struct ValidatedClusterConfig {
     pub extra_volumes: Vec<Volume>,
 }
 
+impl ValidatedClusterConfig {
+    /// Whether the cluster uses S3, for deep storage or ingestion.
+    pub fn uses_s3(&self) -> bool {
+        self.s3_connection.is_some()
+    }
+}
+
 /// Per-role configuration extracted during validation, so the reconcile/build steps consume this
 /// instead of re-reading the raw [`v1alpha1::DruidCluster`].
 #[derive(Clone, Debug)]
@@ -351,13 +365,6 @@ pub fn validate(
     let namespace = get_namespace(druid).context(ClusterIdentitySnafu)?;
     let uid = get_uid(druid).context(ClusterIdentitySnafu)?;
 
-    let extensions = get_extension_list(druid, &druid_tls_security, &druid_auth_config);
-    let metadata_storage_type = druid
-        .spec
-        .cluster_config
-        .metadata_database
-        .as_metadata_storage_type()
-        .to_string();
     let metadata_db_connection = druid
         .spec
         .cluster_config
@@ -377,8 +384,8 @@ pub fn validate(
             deep_storage_bucket_name: dereferenced_objects.deep_storage_bucket_name.clone(),
             druid_tls_security,
             druid_auth_config,
-            extensions,
-            metadata_storage_type,
+            metadata_database: druid.spec.cluster_config.metadata_database.clone(),
+            additional_extensions: druid.spec.cluster_config.additional_extensions.clone(),
             metadata_db_connection,
             deep_storage: druid.spec.cluster_config.deep_storage.clone(),
             extra_volumes: druid.spec.cluster_config.extra_volumes.clone(),
@@ -409,7 +416,6 @@ pub(crate) mod test_support {
             DruidRole, authentication::AuthenticationClassesResolved, security::DruidTlsSecurity,
             v1alpha1,
         },
-        extensions::get_extension_list,
     };
 
     /// A minimal but fully-valid `DruidCluster` with one role group per role.
@@ -504,13 +510,6 @@ spec:
             );
         }
 
-        let extensions = get_extension_list(druid, &druid_tls_security, &None);
-        let metadata_storage_type = druid
-            .spec
-            .cluster_config
-            .metadata_database
-            .as_metadata_storage_type()
-            .to_string();
         let metadata_db_connection = druid
             .spec
             .cluster_config
@@ -530,8 +529,8 @@ spec:
                 deep_storage_bucket_name: None,
                 druid_tls_security,
                 druid_auth_config: None,
-                extensions,
-                metadata_storage_type,
+                metadata_database: druid.spec.cluster_config.metadata_database.clone(),
+                additional_extensions: druid.spec.cluster_config.additional_extensions.clone(),
                 metadata_db_connection,
                 deep_storage: druid.spec.cluster_config.deep_storage.clone(),
                 extra_volumes: druid.spec.cluster_config.extra_volumes.clone(),

rust/operator-binary/src/crd/mod.rs

@@ -430,20 +430,6 @@ impl v1alpha1::DruidCluster {
         }
     }
 
-    /// Returns true if the cluster uses an s3 connection.
-    /// This is a quicker convenience function over the [v1alpha1::DruidCluster::get_s3_connection] function.
-    pub fn uses_s3(&self) -> bool {
-        let s3_ingestion = self
-            .spec
-            .cluster_config
-            .ingestion
-            .as_ref()
-            .and_then(|spec| spec.s3connection.as_ref())
-            .is_some();
-        let s3_storage = self.spec.cluster_config.deep_storage.is_s3();
-        s3_ingestion || s3_storage
-    }
-
     /// Merges and validates all role groups of the given role. Invoked from the validate step
     /// (`controller::validate`).
     ///
@@ -873,12 +859,6 @@ pub enum DeepStorageSpec {
     S3(S3DeepStorageSpec),
 }
 
-impl DeepStorageSpec {
-    fn is_s3(&self) -> bool {
-        matches!(self, DeepStorageSpec::S3(_))
-    }
-}
-
 #[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct HdfsDeepStorageSpec {

rust/operator-binary/src/main.rs

@@ -40,7 +40,6 @@ use crate::{
 mod authentication;
 mod controller;
 mod crd;
-mod extensions;
 mod internal_secret;
 mod webhooks;