From e028cf416380a7ab174be759a0aa56aaa85b9a49 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Fri, 5 Sep 2025 17:58:16 +0200 Subject: [PATCH 1/3] use config-utils to resolve S3 credentials --- rust/operator-binary/src/command.rs | 27 ++++++------- rust/operator-binary/src/controller.rs | 39 +++++++------------ .../src/crd/client_protocol.rs | 17 +------- .../src/crd/fault_tolerant_execution.rs | 17 +------- 4 files changed, 29 insertions(+), 71 deletions(-) diff --git a/rust/operator-binary/src/command.rs b/rust/operator-binary/src/command.rs index 0925b49a..8407cd92 100644 --- a/rust/operator-binary/src/command.rs +++ b/rust/operator-binary/src/command.rs @@ -11,7 +11,8 @@ use crate::{ catalog::config::CatalogConfig, controller::{STACKABLE_LOG_CONFIG_DIR, STACKABLE_LOG_DIR}, crd::{ - CONFIG_DIR_NAME, Container, LOG_PROPERTIES, RW_CONFIG_DIR_NAME, STACKABLE_CLIENT_TLS_DIR, + CONFIG_DIR_NAME, Container, EXCHANGE_MANAGER_PROPERTIES, LOG_PROPERTIES, + RW_CONFIG_DIR_NAME, SPOOLING_MANAGER_PROPERTIES, STACKABLE_CLIENT_TLS_DIR, STACKABLE_INTERNAL_TLS_DIR, STACKABLE_MOUNT_INTERNAL_TLS_DIR, STACKABLE_MOUNT_SERVER_TLS_DIR, STACKABLE_SERVER_TLS_DIR, STACKABLE_TLS_STORE_PASSWORD, SYSTEM_TRUST_STORE, SYSTEM_TRUST_STORE_PASSWORD, TrinoRole, client_protocol, @@ -97,8 +98,6 @@ pub fn container_prepare_args( pub fn container_trino_args( authentication_config: &TrinoAuthenticationConfig, catalogs: &[CatalogConfig], - resolved_fte_config: &Option, - resolved_spooling_config: &Option, ) -> Vec { let mut args = vec![ // copy config files to a writeable empty folder @@ -126,19 +125,17 @@ pub fn container_trino_args( } }); - // Add fault tolerant execution environment variables from files - if let Some(resolved_fte) = resolved_fte_config { - for (env_name, file) in &resolved_fte.load_env_from_files { - args.push(format!("export {env_name}=\"$(cat {file})\"")); - } - } + // Resolve credentials for fault tolerant execution exchange manager if needed + args.push(format!( + "test -f {rw_exchange_manager_config_file} && config-utils template {rw_exchange_manager_config_file}", + rw_exchange_manager_config_file = format!("{RW_CONFIG_DIR_NAME}/{EXCHANGE_MANAGER_PROPERTIES}") + )); - // Add client spooling environment variables from files - if let Some(resolved_spooling) = resolved_spooling_config { - for (env_name, file) in &resolved_spooling.load_env_from_files { - args.push(format!("export {env_name}=\"$(cat {file})\"")); - } - } + // Resolve credentials for spooling manager if needed + args.push(format!( + "test -f {rw_spooling_config_file} && config-utils template {rw_spooling_config_file}", + rw_spooling_config_file = format!("{RW_CONFIG_DIR_NAME}/{SPOOLING_MANAGER_PROPERTIES}") + )); args.push("set -x".to_string()); diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 50e81c06..31295c6c 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -875,31 +875,17 @@ fn build_rolegroup_config_map( // Add exchange manager properties from resolved fault tolerant execution configuration if let Some(resolved_fte) = resolved_fte_config { - if !resolved_fte.exchange_manager_properties.is_empty() { - let exchange_props_with_options: BTreeMap> = resolved_fte - .exchange_manager_properties - .iter() - .map(|(k, v)| (k.clone(), Some(v.clone()))) - .collect(); - cm_conf_data.insert( - EXCHANGE_MANAGER_PROPERTIES.to_string(), - to_java_properties_string(exchange_props_with_options.iter()) - .with_context(|_| FailedToWriteJavaPropertiesSnafu)?, - ); - } + cm_conf_data.insert( + EXCHANGE_MANAGER_PROPERTIES.to_string(), + unsafe_java_properties_string(&resolved_fte.exchange_manager_properties), + ); } // Add client protocol properties (especially spooling properties) if let Some(spooling_config) = resolved_spooling_config { - let spooling_props_with_options: BTreeMap> = spooling_config - .spooling_manager_properties - .iter() - .map(|(k, v)| (k.clone(), Some(v.clone()))) - .collect(); cm_conf_data.insert( SPOOLING_MANAGER_PROPERTIES.to_string(), - to_java_properties_string(spooling_props_with_options.iter()) - .with_context(|_| FailedToWriteJavaPropertiesSnafu)?, + unsafe_java_properties_string(&spooling_config.spooling_manager_properties), ); } @@ -943,6 +929,13 @@ fn build_rolegroup_config_map( }) } +fn unsafe_java_properties_string(map: &BTreeMap) -> String { + map.iter() + .map(|(k, v)| format!("{}={}", k, v)) + .collect::>() + .join("\n") +} + /// The rolegroup catalog [`ConfigMap`] configures the rolegroup catalog based on the configuration /// given by the administrator fn build_rolegroup_catalog_config_map( @@ -1200,13 +1193,7 @@ fn build_rolegroup_statefulset( "-c".to_string(), ]) .args(vec![ - command::container_trino_args( - trino_authentication_config, - catalogs, - resolved_fte_config, - resolved_spooling_config, - ) - .join("\n"), + command::container_trino_args(trino_authentication_config, catalogs).join("\n"), ]) .add_env_vars(env) .add_volume_mount("config", CONFIG_DIR_NAME) diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 8809568b..fafc67b8 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -20,9 +20,6 @@ use crate::{ crd::{ENV_SPOOLING_SECRET, STACKABLE_CLIENT_TLS_DIR}, }; -const SPOOLING_S3_AWS_ACCESS_KEY: &str = "SPOOLING_S3_AWS_ACCESS_KEY"; -const SPOOLING_S3_AWS_SECRET_KEY: &str = "SPOOLING_S3_AWS_SECRET_KEY"; - #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub enum ClientProtocolConfig { @@ -88,10 +85,6 @@ pub struct ResolvedClientProtocolConfig { /// Volume mounts required for the configuration pub volume_mounts: Vec, - /// Env-Vars that should be exported from files. - /// You can think of it like `export ="$(cat )"` - pub load_env_from_files: BTreeMap, - /// Additional commands that need to be executed before starting Trino /// Used to add TLS certificates to the client's trust store. pub init_container_extra_start_commands: Vec, @@ -110,7 +103,6 @@ impl ResolvedClientProtocolConfig { spooling_manager_properties: BTreeMap::new(), volumes: Vec::new(), volume_mounts: Vec::new(), - load_env_from_files: BTreeMap::new(), init_container_extra_start_commands: Vec::new(), }; @@ -194,18 +186,13 @@ impl ResolvedClientProtocolConfig { self.spooling_manager_properties.extend([ ( "s3.aws-access-key".to_string(), - format!("${{ENV:{SPOOLING_S3_AWS_ACCESS_KEY}}}"), + format!("${{file:UTF-8:{access_key_path}}}"), ), ( "s3.aws-secret-key".to_string(), - format!("${{ENV:{SPOOLING_S3_AWS_SECRET_KEY}}}"), + format!("${{file:UTF-8:{secret_key_path}}}"), ), ]); - - self.load_env_from_files.extend([ - (String::from(SPOOLING_S3_AWS_ACCESS_KEY), access_key_path), - (String::from(SPOOLING_S3_AWS_SECRET_KEY), secret_key_path), - ]); } if let Some(tls) = s3_connection.tls.tls.as_ref() { diff --git a/rust/operator-binary/src/crd/fault_tolerant_execution.rs b/rust/operator-binary/src/crd/fault_tolerant_execution.rs index a58ab302..bffcde6a 100644 --- a/rust/operator-binary/src/crd/fault_tolerant_execution.rs +++ b/rust/operator-binary/src/crd/fault_tolerant_execution.rs @@ -226,10 +226,6 @@ pub struct ResolvedFaultTolerantExecutionConfig { /// Volume mounts required for the configuration pub volume_mounts: Vec, - /// Env-Vars that should be exported from files. - /// You can think of it like `export ="$(cat )"` - pub load_env_from_files: BTreeMap, - /// Additional commands that need to be executed before starting Trino pub init_container_extra_start_commands: Vec, } @@ -453,7 +449,6 @@ impl ResolvedFaultTolerantExecutionConfig { exchange_manager_properties, volumes: Vec::new(), volume_mounts: Vec::new(), - load_env_from_files: BTreeMap::new(), init_container_extra_start_commands: Vec::new(), }; @@ -516,22 +511,14 @@ impl ResolvedFaultTolerantExecutionConfig { ); if let Some((access_key_path, secret_key_path)) = s3_connection.credentials_mount_paths() { - let access_key_env = "EXCHANGE_S3_AWS_ACCESS_KEY".to_string(); - let secret_key_env = "EXCHANGE_S3_AWS_SECRET_KEY".to_string(); - self.exchange_manager_properties.insert( "exchange.s3.aws-access-key".to_string(), - format!("${{ENV:{access_key_env}}}"), + format!("${{file:UTF-8:{access_key_path}}}"), ); self.exchange_manager_properties.insert( "exchange.s3.aws-secret-key".to_string(), - format!("${{ENV:{secret_key_env}}}"), + format!("${{file:UTF-8:{secret_key_path}}}"), ); - - self.load_env_from_files - .insert(access_key_env, access_key_path); - self.load_env_from_files - .insert(secret_key_env, secret_key_path); } if let Some(tls) = s3_connection.tls.tls.as_ref() { From d464baa870d54dcd4a09b756336f48a0373b7874 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Fri, 5 Sep 2025 18:13:43 +0200 Subject: [PATCH 2/3] add comment to function --- rust/operator-binary/src/controller.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 31295c6c..2fe89074 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -929,6 +929,11 @@ fn build_rolegroup_config_map( }) } +// This is unsafe because it does not do any escaping of keys or values. +// It is needed because the `product_config::writer::to_java_properties_string` +// function escapes `:` characters in values. +// This breaks values like ${file:UTF-8:/path/to/file} which are used +// for S3 credentials. fn unsafe_java_properties_string(map: &BTreeMap) -> String { map.iter() .map(|(k, v)| format!("{}={}", k, v)) From a25f88303201a5f66d7e50bd1f269794f07c2881 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Fri, 5 Sep 2025 21:41:11 +0200 Subject: [PATCH 3/3] revert the unsafe function --- rust/operator-binary/src/controller.rs | 36 ++++++++++++++------------ 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 2fe89074..64764826 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -875,17 +875,31 @@ fn build_rolegroup_config_map( // Add exchange manager properties from resolved fault tolerant execution configuration if let Some(resolved_fte) = resolved_fte_config { - cm_conf_data.insert( - EXCHANGE_MANAGER_PROPERTIES.to_string(), - unsafe_java_properties_string(&resolved_fte.exchange_manager_properties), - ); + if !resolved_fte.exchange_manager_properties.is_empty() { + let exchange_props_with_options: BTreeMap> = resolved_fte + .exchange_manager_properties + .iter() + .map(|(k, v)| (k.clone(), Some(v.clone()))) + .collect(); + cm_conf_data.insert( + EXCHANGE_MANAGER_PROPERTIES.to_string(), + to_java_properties_string(exchange_props_with_options.iter()) + .with_context(|_| FailedToWriteJavaPropertiesSnafu)?, + ); + } } // Add client protocol properties (especially spooling properties) if let Some(spooling_config) = resolved_spooling_config { + let spooling_props_with_options: BTreeMap> = spooling_config + .spooling_manager_properties + .iter() + .map(|(k, v)| (k.clone(), Some(v.clone()))) + .collect(); cm_conf_data.insert( SPOOLING_MANAGER_PROPERTIES.to_string(), - unsafe_java_properties_string(&spooling_config.spooling_manager_properties), + to_java_properties_string(spooling_props_with_options.iter()) + .with_context(|_| FailedToWriteJavaPropertiesSnafu)?, ); } @@ -929,18 +943,6 @@ fn build_rolegroup_config_map( }) } -// This is unsafe because it does not do any escaping of keys or values. -// It is needed because the `product_config::writer::to_java_properties_string` -// function escapes `:` characters in values. -// This breaks values like ${file:UTF-8:/path/to/file} which are used -// for S3 credentials. -fn unsafe_java_properties_string(map: &BTreeMap) -> String { - map.iter() - .map(|(k, v)| format!("{}={}", k, v)) - .collect::>() - .join("\n") -} - /// The rolegroup catalog [`ConfigMap`] configures the rolegroup catalog based on the configuration /// given by the administrator fn build_rolegroup_catalog_config_map(