From 7854f9ab07b8f48bdc9210e0880b0c9be217eea2 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 2 Sep 2025 19:33:55 +0200 Subject: [PATCH 01/39] add new client protocol module --- rust/operator-binary/src/controller.rs | 59 +++- .../src/crd/client_protocol.rs | 271 ++++++++++++++++++ rust/operator-binary/src/crd/mod.rs | 6 + 3 files changed, 328 insertions(+), 8 deletions(-) create mode 100644 rust/operator-binary/src/crd/client_protocol.rs diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 58e4775e..aac4a48f 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -84,11 +84,11 @@ use crate::{ DISCOVERY_URI, ENV_INTERNAL_SECRET, EXCHANGE_MANAGER_PROPERTIES, HTTP_PORT, HTTP_PORT_NAME, HTTPS_PORT, HTTPS_PORT_NAME, JVM_CONFIG, JVM_SECURITY_PROPERTIES, LOG_PROPERTIES, MAX_TRINO_LOG_FILES_SIZE, METRICS_PORT, METRICS_PORT_NAME, NODE_PROPERTIES, - RW_CONFIG_DIR_NAME, STACKABLE_CLIENT_TLS_DIR, STACKABLE_INTERNAL_TLS_DIR, - STACKABLE_MOUNT_INTERNAL_TLS_DIR, STACKABLE_MOUNT_SERVER_TLS_DIR, STACKABLE_SERVER_TLS_DIR, - TrinoRole, + 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, TrinoRole, authentication::resolve_authentication_classes, - catalog, + catalog, client_protocol, discovery::{TrinoDiscovery, TrinoDiscoveryProtocol, TrinoPodRef}, fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, rolegroup_headless_service_name, v1alpha1, @@ -133,6 +133,12 @@ pub enum Error { #[snafu(display("object defines no namespace"))] ObjectHasNoNamespace, + #[snafu(display("Trino cluster {} has no namespace", name))] + MissingTrinoNamespace { + source: crate::crd::Error, + name: String, + }, + #[snafu(display("object defines no {role:?} role"))] MissingTrinoRole { source: crate::crd::Error, @@ -374,6 +380,9 @@ pub enum Error { ResolveProductImage { source: product_image_selection::Error, }, + + #[snafu(display("failed to resolve client protocol configuration"))] + ClientProtocolConfiguration { source: client_protocol::Error }, } type Result = std::result::Result; @@ -397,6 +406,10 @@ pub async fn reconcile_trino( .context(InvalidTrinoClusterSnafu)?; let client = &ctx.client; + let namespace = trino.namespace_r().context(MissingTrinoNamespaceSnafu { + name: trino.name_any(), + })?; + let resolved_product_image = trino .spec .image @@ -443,13 +456,23 @@ pub async fn reconcile_trino( // Resolve fault tolerant execution configuration with S3 connections if needed let resolved_fte_config = match trino.spec.cluster_config.fault_tolerant_execution.as_ref() { Some(fte_config) => Some( - ResolvedFaultTolerantExecutionConfig::from_config( - fte_config, + ResolvedFaultTolerantExecutionConfig::from_config(fte_config, Some(client), &namespace) + .await + .context(FaultTolerantExecutionSnafu)?, + ), + None => None, + }; + + // Resolve client spooling protocol configuration with S3 connections if needed + let resolved_spooling_config = match trino.spec.cluster_config.client_protocol.as_ref() { + Some(client_protocol_config) => Some( + client_protocol::ResolvedSpoolingProtocolConfig::from_config( + &client_protocol_config.spooling, Some(client), - &trino.namespace_r().context(ReadRoleSnafu)?, + &namespace, ) .await - .context(FaultTolerantExecutionSnafu)?, + .context(ClientProtocolConfigurationSnafu)?, ), None => None, }; @@ -557,6 +580,7 @@ pub async fn reconcile_trino( &trino_opa_config, &client.kubernetes_cluster_info, &resolved_fte_config, + &resolved_spooling_config, )?; let rg_catalog_configmap = build_rolegroup_catalog_config_map( trino, @@ -684,6 +708,7 @@ fn build_rolegroup_config_map( trino_opa_config: &Option, cluster_info: &KubernetesClusterInfo, resolved_fte_config: &Option, + resolved_spooling_protocol_config: &Option, ) -> Result { let mut cm_conf_data = BTreeMap::new(); @@ -835,6 +860,23 @@ fn build_rolegroup_config_map( } } + // Add client protocol properties (especially spooling properties) + if let Some(resolved_client_protocol) = resolved_spooling_protocol_config { + if resolved_client_protocol.is_enabled() { + let spooling_props_with_options: BTreeMap> = + resolved_client_protocol + .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)?, + ); + } + } + let jvm_sec_props: BTreeMap> = config .get(&PropertyNameKind::File(JVM_SECURITY_PROPERTIES.to_string())) .cloned() @@ -1864,6 +1906,7 @@ mod tests { &trino_opa_config, &cluster_info, &None, + &None, ) .unwrap() } diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs new file mode 100644 index 00000000..031d648d --- /dev/null +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -0,0 +1,271 @@ +use std::collections::{BTreeMap, HashMap}; + +/// This module manages the client protocol properties, especially the for spooling. +/// Trino documentation is available here: https://trino.io/docs/current/client/client-protocol.html +use serde::{Deserialize, Serialize}; +use snafu::Snafu; +use stackable_operator::{ + client::Client, + commons::tls_verification::{CaCert, TlsServerVerification, TlsVerification}, + crd::s3, + k8s_openapi::{ + api::core::v1::{Volume, VolumeMount}, + apimachinery::pkg::api::resource::Quantity, + }, + schemars::{self, JsonSchema}, + shared::time::Duration, +}; +use strum::Display; + +use crate::{command, crd::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 struct ClientProtocolConfig { + #[serde(flatten)] + pub spooling: SpoolingProtocolConfig, +} + +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct SpoolingProtocolConfig { + // Spooling protocol properties + /// Enable spooling protocol. + pub enabled: bool, + + // Name of the Kubernetes Secret with one entry ("key") + // to use as protocol.spooling.shared-secret-key property + pub shared_secret: String, + + // Segment retrieval mode used by clients. + #[serde(skip_serializing_if = "Option::is_none")] + pub retrieval_mode: Option, + + // Spooled segment size. Is translated to both initial and max segment size. + // Use overrides for set those explicitly to distinct values. + #[serde(skip_serializing_if = "Option::is_none")] + pub segment_size: Option, + + // Spooling filesystem properties + + // Spool segment location. Each Trino cluster must have its own + // location independent of any other clusters. + pub location: String, + + // Spool segment TTL. Is translated to both fs.segment.ttl as well as + // fs.segment.direct.ttl. + // Use overrides for set those explicitly to distinct values. + #[serde(skip_serializing_if = "Option::is_none")] + pub segment_ttl: Option, + + // Spool segment encryption. + #[serde(skip_serializing_if = "Option::is_none")] + pub segment_encryption: Option, + + // Spooling filesystem properties. Only S3 is supported. + #[serde(flatten)] + pub filesystem: SpoolingFileSystemConfig, + + /// The `configOverrides` allow overriding arbitrary client protocol properties. + #[serde(default)] + pub config_overrides: HashMap, +} + +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize, Display)] +#[strum(serialize_all = "SCREAMING_SNAKE_CASE")] +pub enum SpoolingRetrievalMode { + Storage, + CoordinatorStorageRedirect, + CoordinatorProxy, + WorkerProxy, +} + +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +pub enum SpoolingFileSystemConfig { + S3(S3SpoolingConfig), +} +// TODO: this is exactly the same as fault_tolerant_execution::S3ExchangeConfig +// but without the base_directory property. +// Consolidate Trino S3 properties in a single reusable struct. +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct S3SpoolingConfig { + /// S3 connection configuration. + /// Learn more about S3 configuration in the [S3 concept docs](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). + pub connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference, + + /// IAM role to assume for S3 access. + #[serde(skip_serializing_if = "Option::is_none")] + pub iam_role: Option, + + /// External ID for the IAM role trust policy. + #[serde(skip_serializing_if = "Option::is_none")] + pub external_id: Option, + + /// Maximum number of times the S3 client should retry a request. + #[serde(skip_serializing_if = "Option::is_none")] + pub max_error_retries: Option, + + /// Part data size for S3 multi-part upload. + #[serde(skip_serializing_if = "Option::is_none")] + pub upload_part_size: Option, +} + +pub struct ResolvedSpoolingProtocolConfig { + /// Enable spooling protocol. + pub enabled: bool, + + // Properties for spooling-manager.properties + pub spooling_manager_properties: BTreeMap, + + /// Volumes required for the configuration (e.g., for S3 credentials) + pub volumes: Vec, + + /// 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, +} + +impl ResolvedSpoolingProtocolConfig { + /// Resolve S3 connection properties from Kubernetes resources + /// and prepare spooling filesystem configuration. + pub async fn from_config( + config: &SpoolingProtocolConfig, + client: Option<&Client>, + namespace: &str, + ) -> Result { + let spooling_manager_properties = BTreeMap::new(); + + let mut resolved_config = Self { + enabled: config.enabled, + spooling_manager_properties, + volumes: Vec::new(), + volume_mounts: Vec::new(), + load_env_from_files: BTreeMap::new(), + init_container_extra_start_commands: Vec::new(), + }; + + // Resolve external resources if Kubernetes client is available + // This should always be the case, except for when this function is called during unit tests + if let Some(client) = client { + match &config.filesystem { + SpoolingFileSystemConfig::S3(s3_config) => { + resolved_config + .resolve_s3_backend(s3_config, client, namespace) + .await?; + } + } + } + + Ok(resolved_config) + } + + async fn resolve_s3_backend( + &mut self, + s3_config: &S3SpoolingConfig, + client: &Client, + namespace: &str, + ) -> Result<(), Error> { + use snafu::ResultExt; + + let s3_connection = s3_config + .connection + .clone() + .resolve(client, namespace) + .await + .context(S3ConnectionSnafu)?; + + let (volumes, mounts) = s3_connection + .volumes_and_mounts() + .context(S3ConnectionSnafu)?; + self.volumes.extend(volumes); + self.volume_mounts.extend(mounts); + + self.spooling_manager_properties + .insert("s3.region".to_string(), s3_connection.region.name.clone()); + self.spooling_manager_properties.insert( + "s3.endpoint".to_string(), + s3_connection + .endpoint() + .context(S3ConnectionSnafu)? + .to_string(), + ); + self.spooling_manager_properties.insert( + "s3.path-style-access".to_string(), + (s3_connection.access_style == s3::v1alpha1::S3AccessStyle::Path).to_string(), + ); + + if let Some((access_key_path, secret_key_path)) = s3_connection.credentials_mount_paths() { + self.spooling_manager_properties.extend([ + ( + "s3.aws-access-key".to_string(), + format!("${{ENV:{SPOOLING_S3_AWS_ACCESS_KEY}}}"), + ), + ( + "s3.aws-secret-key".to_string(), + format!("${{ENV:{SPOOLING_S3_AWS_SECRET_KEY}}}"), + ), + ]); + + 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() { + match &tls.verification { + TlsVerification::None {} => return S3TlsNoVerificationNotSupportedSnafu.fail(), + TlsVerification::Server(TlsServerVerification { + ca_cert: CaCert::WebPki {}, + }) => {} + TlsVerification::Server(TlsServerVerification { + ca_cert: CaCert::SecretClass(_), + }) => { + if let Some(ca_cert) = s3_connection.tls.tls_ca_cert_mount_path() { + self.init_container_extra_start_commands.extend( + command::add_cert_to_truststore( + &ca_cert, + STACKABLE_CLIENT_TLS_DIR, + "spooling-s3-ca-cert", + ), + ); + } + } + } + } + + Ok(()) + } + + pub(crate) fn is_enabled(&self) -> bool { + return self.enabled; + } +} + +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display("Failed to resolve S3 connection"))] + S3Connection { + source: s3::v1alpha1::ConnectionError, + }, + + #[snafu(display("trino does not support disabling the TLS verification of S3 servers"))] + S3TlsNoVerificationNotSupported, + + #[snafu(display("failed to convert data size for [{field}] to bytes"))] + QuantityConversion { + source: stackable_operator::memory::Error, + field: &'static str, + }, +} diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index a5c69932..efdd7974 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -1,6 +1,7 @@ pub mod affinity; pub mod authentication; pub mod catalog; +pub mod client_protocol; pub mod discovery; pub mod fault_tolerant_execution; @@ -61,6 +62,7 @@ pub const LOG_PROPERTIES: &str = "log.properties"; pub const ACCESS_CONTROL_PROPERTIES: &str = "access-control.properties"; pub const JVM_SECURITY_PROPERTIES: &str = "security.properties"; pub const EXCHANGE_MANAGER_PROPERTIES: &str = "exchange-manager.properties"; +pub const SPOOLING_MANAGER_PROPERTIES: &str = "spooling-manager.properties"; // node.properties pub const NODE_ENVIRONMENT: &str = "node.environment"; // config.properties @@ -300,6 +302,10 @@ pub mod versioned { pub fault_tolerant_execution: Option, + /// Client spooling protocol configuration. + #[serde(skip_serializing_if = "Option::is_none")] + pub client_protocol: Option, + /// Name of the Vector aggregator [discovery ConfigMap](DOCS_BASE_URL_PLACEHOLDER/concepts/service_discovery). /// It must contain the key `ADDRESS` with the address of the Vector aggregator. /// Follow the [logging tutorial](DOCS_BASE_URL_PLACEHOLDER/tutorials/logging-vector-aggregator) From 32dfb3a6bca5c06dd648192af401721edf0b5f99 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Wed, 3 Sep 2025 14:08:05 +0200 Subject: [PATCH 02/39] update crd fields to match decision --- deploy/helm/trino-operator/crds/crds.yaml | 167 ++++++++++++++++++ rust/operator-binary/src/controller.rs | 5 +- .../src/crd/client_protocol.rs | 41 +---- rust/operator-binary/src/crd/mod.rs | 2 +- 4 files changed, 175 insertions(+), 40 deletions(-) diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index 19ee4127..a62faee0 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -105,6 +105,173 @@ spec: description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. type: object type: object + clientSpoolingProtocol: + description: Client spooling protocol configuration. + nullable: true + oneOf: + - required: + - s3 + properties: + configOverrides: + additionalProperties: + type: string + default: {} + description: The `configOverrides` allow overriding arbitrary client protocol properties. + type: object + enabled: + description: Enable spooling protocol. + type: boolean + location: + type: string + s3: + properties: + connection: + description: S3 connection configuration. Learn more about S3 configuration in the [S3 concept docs](https://docs.stackable.tech/home/nightly/concepts/s3). + oneOf: + - required: + - inline + - required: + - reference + properties: + inline: + description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). + properties: + accessStyle: + default: VirtualHosted + description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). + enum: + - Path + - VirtualHosted + type: string + credentials: + description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. + nullable: true + properties: + scope: + description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' + nullable: true + properties: + listenerVolumes: + default: [] + description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. + items: + type: string + type: array + node: + default: false + description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. + type: boolean + pod: + default: false + description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. + type: boolean + services: + default: [] + description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. + items: + type: string + type: array + type: object + secretClass: + description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' + type: string + required: + - secretClass + type: object + host: + description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' + type: string + port: + description: Port the S3 server listens on. If not specified the product will determine the port to use. + format: uint16 + minimum: 0.0 + nullable: true + type: integer + region: + default: + name: us-east-1 + description: |- + Bucket region used for signing headers (sigv4). + + This defaults to `us-east-1` which is compatible with other implementations such as Minio. + + WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. + properties: + name: + default: us-east-1 + type: string + type: object + tls: + description: Use a TLS connection. If not specified no TLS will be used. + nullable: true + properties: + verification: + description: The verification method used to verify the certificates of the server and/or the client. + oneOf: + - required: + - none + - required: + - server + properties: + none: + description: Use TLS but don't verify certificates. + type: object + server: + description: Use TLS and a CA certificate to verify the server. + properties: + caCert: + description: CA cert to verify the server. + oneOf: + - required: + - webPki + - required: + - secretClass + properties: + secretClass: + description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. + type: string + webPki: + description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. + type: object + type: object + required: + - caCert + type: object + type: object + required: + - verification + type: object + required: + - host + type: object + reference: + type: string + type: object + externalId: + description: External ID for the IAM role trust policy. + nullable: true + type: string + iamRole: + description: IAM role to assume for S3 access. + nullable: true + type: string + maxErrorRetries: + description: Maximum number of times the S3 client should retry a request. + format: uint32 + minimum: 0.0 + nullable: true + type: integer + uploadPartSize: + description: Part data size for S3 multi-part upload. + nullable: true + type: string + required: + - connection + type: object + required: + - enabled + - location + type: object faultTolerantExecution: description: Fault tolerant execution configuration. When enabled, Trino can automatically retry queries or tasks in case of failures. nullable: true diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index aac4a48f..35165fc9 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -464,10 +464,11 @@ pub async fn reconcile_trino( }; // Resolve client spooling protocol configuration with S3 connections if needed - let resolved_spooling_config = match trino.spec.cluster_config.client_protocol.as_ref() { + let resolved_spooling_config = match trino.spec.cluster_config.client_spooling_protocol.as_ref() + { Some(client_protocol_config) => Some( client_protocol::ResolvedSpoolingProtocolConfig::from_config( - &client_protocol_config.spooling, + client_protocol_config, Some(client), &namespace, ) diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 031d648d..d93dc79e 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -13,7 +13,6 @@ use stackable_operator::{ apimachinery::pkg::api::resource::Quantity, }, schemars::{self, JsonSchema}, - shared::time::Duration, }; use strum::Display; @@ -24,47 +23,14 @@ const SPOOLING_S3_AWS_SECRET_KEY: &str = "SPOOLING_S3_AWS_SECRET_KEY"; #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] -pub struct ClientProtocolConfig { - #[serde(flatten)] - pub spooling: SpoolingProtocolConfig, -} - -#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] -pub struct SpoolingProtocolConfig { - // Spooling protocol properties +pub struct ClientSpoolingProtocolConfig { /// Enable spooling protocol. pub enabled: bool, - // Name of the Kubernetes Secret with one entry ("key") - // to use as protocol.spooling.shared-secret-key property - pub shared_secret: String, - - // Segment retrieval mode used by clients. - #[serde(skip_serializing_if = "Option::is_none")] - pub retrieval_mode: Option, - - // Spooled segment size. Is translated to both initial and max segment size. - // Use overrides for set those explicitly to distinct values. - #[serde(skip_serializing_if = "Option::is_none")] - pub segment_size: Option, - - // Spooling filesystem properties - // Spool segment location. Each Trino cluster must have its own // location independent of any other clusters. pub location: String, - // Spool segment TTL. Is translated to both fs.segment.ttl as well as - // fs.segment.direct.ttl. - // Use overrides for set those explicitly to distinct values. - #[serde(skip_serializing_if = "Option::is_none")] - pub segment_ttl: Option, - - // Spool segment encryption. - #[serde(skip_serializing_if = "Option::is_none")] - pub segment_encryption: Option, - // Spooling filesystem properties. Only S3 is supported. #[serde(flatten)] pub filesystem: SpoolingFileSystemConfig, @@ -84,6 +50,7 @@ pub enum SpoolingRetrievalMode { } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] pub enum SpoolingFileSystemConfig { S3(S3SpoolingConfig), } @@ -140,7 +107,7 @@ impl ResolvedSpoolingProtocolConfig { /// Resolve S3 connection properties from Kubernetes resources /// and prepare spooling filesystem configuration. pub async fn from_config( - config: &SpoolingProtocolConfig, + config: &ClientSpoolingProtocolConfig, client: Option<&Client>, namespace: &str, ) -> Result { @@ -249,7 +216,7 @@ impl ResolvedSpoolingProtocolConfig { } pub(crate) fn is_enabled(&self) -> bool { - return self.enabled; + self.enabled } } diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index efdd7974..f9f88dc2 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -304,7 +304,7 @@ pub mod versioned { /// Client spooling protocol configuration. #[serde(skip_serializing_if = "Option::is_none")] - pub client_protocol: Option, + pub client_spooling_protocol: Option, /// Name of the Vector aggregator [discovery ConfigMap](DOCS_BASE_URL_PLACEHOLDER/concepts/service_discovery). /// It must contain the key `ADDRESS` with the address of the Vector aggregator. From e732da4520aef68dbed07d013c3d34cc788f8f89 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Wed, 3 Sep 2025 15:59:41 +0200 Subject: [PATCH 03/39] add spooling config to STS and some unit tests --- deploy/helm/trino-operator/crds/crds.yaml | 2 +- rust/operator-binary/src/command.rs | 16 +- rust/operator-binary/src/controller.rs | 119 +++++++++------ .../src/crd/client_protocol.rs | 138 ++++++++++++++++-- rust/operator-binary/src/crd/mod.rs | 1 + 5 files changed, 220 insertions(+), 56 deletions(-) diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index a62faee0..b60db6ef 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -115,8 +115,8 @@ spec: configOverrides: additionalProperties: type: string - default: {} description: The `configOverrides` allow overriding arbitrary client protocol properties. + nullable: true type: object enabled: description: Enable spooling protocol. diff --git a/rust/operator-binary/src/command.rs b/rust/operator-binary/src/command.rs index 4b6791d5..50ab6fbc 100644 --- a/rust/operator-binary/src/command.rs +++ b/rust/operator-binary/src/command.rs @@ -14,7 +14,7 @@ use crate::{ CONFIG_DIR_NAME, Container, LOG_PROPERTIES, RW_CONFIG_DIR_NAME, 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, + SYSTEM_TRUST_STORE, SYSTEM_TRUST_STORE_PASSWORD, TrinoRole, client_protocol, fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, v1alpha1, }, }; @@ -24,6 +24,7 @@ pub fn container_prepare_args( catalogs: &[CatalogConfig], merged_config: &v1alpha1::TrinoConfig, resolved_fte_config: &Option, + resolved_spooling_config: &Option, ) -> Vec { let mut args = vec![]; @@ -85,6 +86,11 @@ pub fn container_prepare_args( args.extend_from_slice(&resolved_fte.init_container_extra_start_commands); } + // Add the commands that are needed for the client spooling protocol (e.g., TLS certificates for S3) + if let Some(resolved_spooling) = resolved_spooling_config { + args.extend_from_slice(&resolved_spooling.init_container_extra_start_commands); + } + args } @@ -92,6 +98,7 @@ 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,6 +133,13 @@ pub fn container_trino_args( } } + // 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})\"")); + } + } + args.push("set -x".to_string()); // Start command diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 35165fc9..101038ed 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -81,11 +81,11 @@ use crate::{ command, config, crd::{ ACCESS_CONTROL_PROPERTIES, APP_NAME, CONFIG_DIR_NAME, CONFIG_PROPERTIES, Container, - DISCOVERY_URI, ENV_INTERNAL_SECRET, EXCHANGE_MANAGER_PROPERTIES, HTTP_PORT, HTTP_PORT_NAME, - HTTPS_PORT, HTTPS_PORT_NAME, JVM_CONFIG, JVM_SECURITY_PROPERTIES, LOG_PROPERTIES, - MAX_TRINO_LOG_FILES_SIZE, METRICS_PORT, METRICS_PORT_NAME, NODE_PROPERTIES, - RW_CONFIG_DIR_NAME, SPOOLING_MANAGER_PROPERTIES, STACKABLE_CLIENT_TLS_DIR, - STACKABLE_INTERNAL_TLS_DIR, STACKABLE_MOUNT_INTERNAL_TLS_DIR, + DISCOVERY_URI, ENV_INTERNAL_SECRET, ENV_SPOOLING_SECRET, EXCHANGE_MANAGER_PROPERTIES, + HTTP_PORT, HTTP_PORT_NAME, HTTPS_PORT, HTTPS_PORT_NAME, JVM_CONFIG, + JVM_SECURITY_PROPERTIES, LOG_PROPERTIES, MAX_TRINO_LOG_FILES_SIZE, METRICS_PORT, + METRICS_PORT_NAME, NODE_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, TrinoRole, authentication::resolve_authentication_classes, catalog, client_protocol, @@ -466,9 +466,9 @@ pub async fn reconcile_trino( // Resolve client spooling protocol configuration with S3 connections if needed let resolved_spooling_config = match trino.spec.cluster_config.client_spooling_protocol.as_ref() { - Some(client_protocol_config) => Some( + Some(spooling_config) => Some( client_protocol::ResolvedSpoolingProtocolConfig::from_config( - client_protocol_config, + spooling_config, Some(client), &namespace, ) @@ -524,7 +524,21 @@ pub async fn reconcile_trino( None => None, }; - create_shared_internal_secret(trino, client).await?; + create_random_secret( + &shared_internal_secret_name(trino), + ENV_INTERNAL_SECRET, + trino, + client, + ) + .await?; + + create_random_secret( + &shared_spooling_secret_name(trino), + ENV_SPOOLING_SECRET, + trino, + client, + ) + .await?; let mut sts_cond_builder = StatefulSetConditionBuilder::default(); @@ -600,6 +614,7 @@ pub async fn reconcile_trino( &catalogs, &rbac_sa.name_any(), &resolved_fte_config, + &resolved_spooling_config, )?; cluster_resources @@ -709,7 +724,7 @@ fn build_rolegroup_config_map( trino_opa_config: &Option, cluster_info: &KubernetesClusterInfo, resolved_fte_config: &Option, - resolved_spooling_protocol_config: &Option, + resolved_spooling_config: &Option, ) -> Result { let mut cm_conf_data = BTreeMap::new(); @@ -862,20 +877,17 @@ fn build_rolegroup_config_map( } // Add client protocol properties (especially spooling properties) - if let Some(resolved_client_protocol) = resolved_spooling_protocol_config { - if resolved_client_protocol.is_enabled() { - let spooling_props_with_options: BTreeMap> = - resolved_client_protocol - .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)?, - ); - } + 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)?, + ); } let jvm_sec_props: BTreeMap> = config @@ -987,6 +999,7 @@ fn build_rolegroup_statefulset( catalogs: &[CatalogConfig], sa_name: &str, resolved_fte_config: &Option, + resolved_spooling_config: &Option, ) -> Result { let role = trino .role(trino_role) @@ -1014,7 +1027,7 @@ fn build_rolegroup_statefulset( // additional authentication env vars let mut env = trino_authentication_config.env_vars(trino_role, &Container::Trino); - let secret_name = build_shared_internal_secret_name(trino); + let secret_name = shared_internal_secret_name(trino); env.push(env_var_from_secret(&secret_name, None, ENV_INTERNAL_SECRET)); trino_authentication_config @@ -1078,6 +1091,7 @@ fn build_rolegroup_statefulset( catalogs, &requested_secret_lifetime, resolved_fte_config, + resolved_spooling_config, )?; let mut prepare_args = vec![]; @@ -1097,6 +1111,7 @@ fn build_rolegroup_statefulset( catalogs, merged_config, resolved_fte_config, + resolved_spooling_config, )); prepare_args @@ -1165,6 +1180,7 @@ fn build_rolegroup_statefulset( trino_authentication_config, catalogs, resolved_fte_config, + resolved_spooling_config, ) .join("\n"), ]) @@ -1447,11 +1463,27 @@ fn build_recommended_labels<'a>( } } -async fn create_shared_internal_secret( +async fn create_random_secret( + secret_name: &str, + secret_key: &str, trino: &v1alpha1::TrinoCluster, client: &Client, ) -> Result<()> { - let secret = build_shared_internal_secret(trino)?; + let mut internal_secret = BTreeMap::new(); + internal_secret.insert(secret_key.to_string(), get_random_base64()); + + let secret = Secret { + immutable: Some(true), + metadata: ObjectMetaBuilder::new() + .name(secret_name) + .namespace_opt(trino.namespace()) + .ownerreference_from_resource(trino, None, Some(true)) + .context(ObjectMissingMetadataForOwnerRefSnafu)? + .build(), + string_data: Some(internal_secret), + ..Secret::default() + }; + if client .get_opt::( &secret.name_any(), @@ -1473,25 +1505,12 @@ async fn create_shared_internal_secret( Ok(()) } -fn build_shared_internal_secret(trino: &v1alpha1::TrinoCluster) -> Result { - let mut internal_secret = BTreeMap::new(); - internal_secret.insert(ENV_INTERNAL_SECRET.to_string(), get_random_base64()); - - Ok(Secret { - immutable: Some(true), - metadata: ObjectMetaBuilder::new() - .name(build_shared_internal_secret_name(trino)) - .namespace_opt(trino.namespace()) - .ownerreference_from_resource(trino, None, Some(true)) - .context(ObjectMissingMetadataForOwnerRefSnafu)? - .build(), - string_data: Some(internal_secret), - ..Secret::default() - }) +fn shared_internal_secret_name(trino: &v1alpha1::TrinoCluster) -> String { + format!("{}-internal-secret", trino.name_any()) } -fn build_shared_internal_secret_name(trino: &v1alpha1::TrinoCluster) -> String { - format!("{}-internal-secret", trino.name_any()) +fn shared_spooling_secret_name(trino: &v1alpha1::TrinoCluster) -> String { + format!("{}-spooling-secret", trino.name_any()) } fn get_random_base64() -> String { @@ -1644,6 +1663,7 @@ fn tls_volume_mounts( catalogs: &[CatalogConfig], requested_secret_lifetime: &Duration, resolved_fte_config: &Option, + resolved_spooling_config: &Option, ) -> Result<()> { if let Some(server_tls) = trino.get_server_tls() { cb_prepare @@ -1736,6 +1756,19 @@ fn tls_volume_mounts( .context(AddVolumeSnafu)?; } + // client spooling S3 credentials and other resources + if let Some(resolved_spooling) = resolved_spooling_config { + cb_prepare + .add_volume_mounts(resolved_spooling.volume_mounts.clone()) + .context(AddVolumeMountSnafu)?; + cb_trino + .add_volume_mounts(resolved_spooling.volume_mounts.clone()) + .context(AddVolumeMountSnafu)?; + pod_builder + .add_volumes(resolved_spooling.volumes.clone()) + .context(AddVolumeSnafu)?; + } + Ok(()) } diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index d93dc79e..821ec566 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -16,7 +16,10 @@ use stackable_operator::{ }; use strum::Display; -use crate::{command, crd::STACKABLE_CLIENT_TLS_DIR}; +use crate::{ + command, + 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"; @@ -37,7 +40,7 @@ pub struct ClientSpoolingProtocolConfig { /// The `configOverrides` allow overriding arbitrary client protocol properties. #[serde(default)] - pub config_overrides: HashMap, + pub config_overrides: Option>, } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize, Display)] @@ -82,9 +85,6 @@ pub struct S3SpoolingConfig { } pub struct ResolvedSpoolingProtocolConfig { - /// Enable spooling protocol. - pub enabled: bool, - // Properties for spooling-manager.properties pub spooling_manager_properties: BTreeMap, @@ -111,10 +111,21 @@ impl ResolvedSpoolingProtocolConfig { client: Option<&Client>, namespace: &str, ) -> Result { - let spooling_manager_properties = BTreeMap::new(); + let mut spooling_manager_properties = BTreeMap::new(); + + spooling_manager_properties.insert( + "protocol.spooling.enabled".to_string(), + config.enabled.to_string(), + ); + + spooling_manager_properties.insert( + "protocol.spooling.shared-secret-key".to_string(), + format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), + ); + + spooling_manager_properties.insert("fs.location".to_string(), config.location.clone()); let mut resolved_config = Self { - enabled: config.enabled, spooling_manager_properties, volumes: Vec::new(), volume_mounts: Vec::new(), @@ -134,6 +145,13 @@ impl ResolvedSpoolingProtocolConfig { } } + // Finally, extend the spooling manager properties with any user configuration + if let Some(user_config) = config.config_overrides.as_ref() { + resolved_config + .spooling_manager_properties + .extend(user_config.clone()); + } + Ok(resolved_config) } @@ -212,11 +230,11 @@ impl ResolvedSpoolingProtocolConfig { } } - Ok(()) - } + // Enable S3 filesystem after successful resolution + self.spooling_manager_properties + .insert("fs.s3.enabled".to_string(), "true".to_string()); - pub(crate) fn is_enabled(&self) -> bool { - self.enabled + Ok(()) } } @@ -236,3 +254,101 @@ pub enum Error { field: &'static str, }, } + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn test_spooling_config() { + let config = ClientSpoolingProtocolConfig { + enabled: true, + location: "s3://my-bucket/spooling".to_string(), + filesystem: SpoolingFileSystemConfig::S3(S3SpoolingConfig { + connection: + stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( + "test-s3-connection".to_string(), + ), + iam_role: None, + external_id: None, + max_error_retries: None, + upload_part_size: None, + }), + config_overrides: None, + }; + + let resolved_spooling_config = ResolvedSpoolingProtocolConfig::from_config( + &config, None, // No client, so no external resolution + "default", + ) + .await + .unwrap(); + + let expected_props = BTreeMap::from([ + ("protocol.spooling.enabled".to_string(), "true".to_string()), + ( + "protocol.spooling.shared-secret-key".to_string(), + format!("${{ENV:{}}}", ENV_SPOOLING_SECRET), + ), + ( + "fs.location".to_string(), + "s3://my-bucket/spooling".to_string(), + ), + ]); + + assert_eq!( + expected_props, + resolved_spooling_config.spooling_manager_properties + ); + } + + #[tokio::test] + async fn test_spooling_config_overrides() { + let config = ClientSpoolingProtocolConfig { + enabled: true, + location: "s3://my-bucket/spooling".to_string(), + filesystem: SpoolingFileSystemConfig::S3(S3SpoolingConfig { + connection: + stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( + "test-s3-connection".to_string(), + ), + iam_role: None, + external_id: None, + max_error_retries: None, + upload_part_size: None, + }), + config_overrides: Some(HashMap::from([( + "protocol.spooling.retrieval-mode".to_string(), + "STORAGE".to_string(), + )])), + }; + + let resolved_spooling_config = ResolvedSpoolingProtocolConfig::from_config( + &config, None, // No client, so no external resolution + "default", + ) + .await + .unwrap(); + + let expected_props = BTreeMap::from([ + ("protocol.spooling.enabled".to_string(), "true".to_string()), + ( + "protocol.spooling.shared-secret-key".to_string(), + format!("${{ENV:{}}}", ENV_SPOOLING_SECRET), + ), + ( + "fs.location".to_string(), + "s3://my-bucket/spooling".to_string(), + ), + ( + "protocol.spooling.retrieval-mode".to_string(), + "STORAGE".to_string(), + ), + ]); + + assert_eq!( + expected_props, + resolved_spooling_config.spooling_manager_properties + ); + } +} diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index f9f88dc2..ffa1f2c4 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -107,6 +107,7 @@ pub const STACKABLE_TLS_STORE_PASSWORD: &str = "changeit"; pub const SYSTEM_TRUST_STORE_PASSWORD: &str = "changeit"; // secret vars pub const ENV_INTERNAL_SECRET: &str = "INTERNAL_SECRET"; +pub const ENV_SPOOLING_SECRET: &str = "SPOOLING_SECRET"; // TLS pub const TLS_DEFAULT_SECRET_CLASS: &str = "tls"; // Logging From cc24f6422062d7133c1f0aaaeb244061b23b1fca Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Wed, 3 Sep 2025 18:30:12 +0200 Subject: [PATCH 04/39] add kuttl test --- deploy/helm/trino-operator/crds/crds.yaml | 258 +++++++++--------- .../src/crd/client_protocol.rs | 1 - .../kuttl/client-spooling/00-assert.yaml | 19 ++ ...tor-aggregator-discovery-configmap.yaml.j2 | 9 + .../kuttl/client-spooling/00-patch-ns.yaml.j2 | 9 + .../kuttl/client-spooling/00-rbac.yaml.j2 | 29 ++ .../kuttl/client-spooling/00-secrets.yaml | 62 +++++ .../kuttl/client-spooling/01-assert.yaml | 17 ++ .../client-spooling/01-install-minio.yaml | 11 + .../01_helm-bitnami-minio-values.yaml | 79 ++++++ .../kuttl/client-spooling/02-assert.yaml | 25 ++ .../client-spooling/02-install-trino.yaml.j2 | 55 ++++ .../kuttl/client-spooling/03-assert.yaml | 11 + .../03-install-test-helper.yaml | 22 ++ .../client-spooling/04-copy-scripts.yaml | 6 + .../kuttl/client-spooling/04_check-fte.py | 76 ++++++ .../kuttl/client-spooling/05-assert.yaml | 32 +++ .../kuttl/client-spooling/05-run-tests.yaml | 7 + tests/test-definition.yaml | 4 + 19 files changed, 604 insertions(+), 128 deletions(-) create mode 100644 tests/templates/kuttl/client-spooling/00-assert.yaml create mode 100644 tests/templates/kuttl/client-spooling/00-install-vector-aggregator-discovery-configmap.yaml.j2 create mode 100644 tests/templates/kuttl/client-spooling/00-patch-ns.yaml.j2 create mode 100644 tests/templates/kuttl/client-spooling/00-rbac.yaml.j2 create mode 100644 tests/templates/kuttl/client-spooling/00-secrets.yaml create mode 100644 tests/templates/kuttl/client-spooling/01-assert.yaml create mode 100644 tests/templates/kuttl/client-spooling/01-install-minio.yaml create mode 100644 tests/templates/kuttl/client-spooling/01_helm-bitnami-minio-values.yaml create mode 100644 tests/templates/kuttl/client-spooling/02-assert.yaml create mode 100644 tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 create mode 100644 tests/templates/kuttl/client-spooling/03-assert.yaml create mode 100644 tests/templates/kuttl/client-spooling/03-install-test-helper.yaml create mode 100644 tests/templates/kuttl/client-spooling/04-copy-scripts.yaml create mode 100644 tests/templates/kuttl/client-spooling/04_check-fte.py create mode 100644 tests/templates/kuttl/client-spooling/05-assert.yaml create mode 100644 tests/templates/kuttl/client-spooling/05-run-tests.yaml diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index b60db6ef..caba3260 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -108,9 +108,6 @@ spec: clientSpoolingProtocol: description: Client spooling protocol configuration. nullable: true - oneOf: - - required: - - s3 properties: configOverrides: additionalProperties: @@ -121,155 +118,162 @@ spec: enabled: description: Enable spooling protocol. type: boolean - location: - type: string - s3: + filesystem: + oneOf: + - required: + - s3 properties: - connection: - description: S3 connection configuration. Learn more about S3 configuration in the [S3 concept docs](https://docs.stackable.tech/home/nightly/concepts/s3). - oneOf: - - required: - - inline - - required: - - reference + s3: properties: - inline: - description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). + connection: + description: S3 connection configuration. Learn more about S3 configuration in the [S3 concept docs](https://docs.stackable.tech/home/nightly/concepts/s3). + oneOf: + - required: + - inline + - required: + - reference properties: - accessStyle: - default: VirtualHosted - description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). - enum: - - Path - - VirtualHosted - type: string - credentials: - description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. - nullable: true + inline: + description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). properties: - scope: - description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' + accessStyle: + default: VirtualHosted + description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). + enum: + - Path + - VirtualHosted + type: string + credentials: + description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. nullable: true properties: - listenerVolumes: - default: [] - description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. - items: - type: string - type: array - node: - default: false - description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. - type: boolean - pod: - default: false - description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. - type: boolean - services: - default: [] - description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. - items: - type: string - type: array + scope: + description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' + nullable: true + properties: + listenerVolumes: + default: [] + description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. + items: + type: string + type: array + node: + default: false + description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. + type: boolean + pod: + default: false + description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. + type: boolean + services: + default: [] + description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. + items: + type: string + type: array + type: object + secretClass: + description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' + type: string + required: + - secretClass type: object - secretClass: - description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' + host: + description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' type: string - required: - - secretClass - type: object - host: - description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' - type: string - port: - description: Port the S3 server listens on. If not specified the product will determine the port to use. - format: uint16 - minimum: 0.0 - nullable: true - type: integer - region: - default: - name: us-east-1 - description: |- - Bucket region used for signing headers (sigv4). + port: + description: Port the S3 server listens on. If not specified the product will determine the port to use. + format: uint16 + minimum: 0.0 + nullable: true + type: integer + region: + default: + name: us-east-1 + description: |- + Bucket region used for signing headers (sigv4). - This defaults to `us-east-1` which is compatible with other implementations such as Minio. + This defaults to `us-east-1` which is compatible with other implementations such as Minio. - WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. - properties: - name: - default: us-east-1 - type: string - type: object - tls: - description: Use a TLS connection. If not specified no TLS will be used. - nullable: true - properties: - verification: - description: The verification method used to verify the certificates of the server and/or the client. - oneOf: - - required: - - none - - required: - - server + WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. properties: - none: - description: Use TLS but don't verify certificates. - type: object - server: - description: Use TLS and a CA certificate to verify the server. + name: + default: us-east-1 + type: string + type: object + tls: + description: Use a TLS connection. If not specified no TLS will be used. + nullable: true + properties: + verification: + description: The verification method used to verify the certificates of the server and/or the client. + oneOf: + - required: + - none + - required: + - server properties: - caCert: - description: CA cert to verify the server. - oneOf: - - required: - - webPki - - required: - - secretClass + none: + description: Use TLS but don't verify certificates. + type: object + server: + description: Use TLS and a CA certificate to verify the server. properties: - secretClass: - description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. - type: string - webPki: - description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. + caCert: + description: CA cert to verify the server. + oneOf: + - required: + - webPki + - required: + - secretClass + properties: + secretClass: + description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. + type: string + webPki: + description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. + type: object type: object + required: + - caCert type: object - required: - - caCert type: object + required: + - verification type: object required: - - verification + - host type: object - required: - - host + reference: + type: string type: object - reference: + externalId: + description: External ID for the IAM role trust policy. + nullable: true type: string + iamRole: + description: IAM role to assume for S3 access. + nullable: true + type: string + maxErrorRetries: + description: Maximum number of times the S3 client should retry a request. + format: uint32 + minimum: 0.0 + nullable: true + type: integer + uploadPartSize: + description: Part data size for S3 multi-part upload. + nullable: true + type: string + required: + - connection type: object - externalId: - description: External ID for the IAM role trust policy. - nullable: true - type: string - iamRole: - description: IAM role to assume for S3 access. - nullable: true - type: string - maxErrorRetries: - description: Maximum number of times the S3 client should retry a request. - format: uint32 - minimum: 0.0 - nullable: true - type: integer - uploadPartSize: - description: Part data size for S3 multi-part upload. - nullable: true - type: string - required: - - connection type: object + location: + type: string required: - enabled + - filesystem - location type: object faultTolerantExecution: diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 821ec566..4a038f10 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -35,7 +35,6 @@ pub struct ClientSpoolingProtocolConfig { pub location: String, // Spooling filesystem properties. Only S3 is supported. - #[serde(flatten)] pub filesystem: SpoolingFileSystemConfig, /// The `configOverrides` allow overriding arbitrary client protocol properties. diff --git a/tests/templates/kuttl/client-spooling/00-assert.yaml b/tests/templates/kuttl/client-spooling/00-assert.yaml new file mode 100644 index 00000000..47bfe1ea --- /dev/null +++ b/tests/templates/kuttl/client-spooling/00-assert.yaml @@ -0,0 +1,19 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 300 +--- +apiVersion: v1 +kind: Secret +metadata: + name: minio-credentials +--- +apiVersion: v1 +kind: Secret +metadata: + name: minio-tls-certificates +--- +apiVersion: s3.stackable.tech/v1alpha1 +kind: S3Connection +metadata: + name: minio diff --git a/tests/templates/kuttl/client-spooling/00-install-vector-aggregator-discovery-configmap.yaml.j2 b/tests/templates/kuttl/client-spooling/00-install-vector-aggregator-discovery-configmap.yaml.j2 new file mode 100644 index 00000000..2d6a0df5 --- /dev/null +++ b/tests/templates/kuttl/client-spooling/00-install-vector-aggregator-discovery-configmap.yaml.j2 @@ -0,0 +1,9 @@ +{% if lookup('env', 'VECTOR_AGGREGATOR') %} +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: vector-aggregator-discovery +data: + ADDRESS: {{ lookup('env', 'VECTOR_AGGREGATOR') }} +{% endif %} diff --git a/tests/templates/kuttl/client-spooling/00-patch-ns.yaml.j2 b/tests/templates/kuttl/client-spooling/00-patch-ns.yaml.j2 new file mode 100644 index 00000000..67185acf --- /dev/null +++ b/tests/templates/kuttl/client-spooling/00-patch-ns.yaml.j2 @@ -0,0 +1,9 @@ +{% if test_scenario['values']['openshift'] == 'true' %} +# see https://github.com/stackabletech/issues/issues/566 +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - script: kubectl patch namespace $NAMESPACE -p '{"metadata":{"labels":{"pod-security.kubernetes.io/enforce":"privileged"}}}' + timeout: 120 +{% endif %} diff --git a/tests/templates/kuttl/client-spooling/00-rbac.yaml.j2 b/tests/templates/kuttl/client-spooling/00-rbac.yaml.j2 new file mode 100644 index 00000000..9cbf0351 --- /dev/null +++ b/tests/templates/kuttl/client-spooling/00-rbac.yaml.j2 @@ -0,0 +1,29 @@ +--- +kind: Role +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: use-integration-tests-scc +rules: +{% if test_scenario['values']['openshift'] == "true" %} + - apiGroups: ["security.openshift.io"] + resources: ["securitycontextconstraints"] + resourceNames: ["privileged"] + verbs: ["use"] +{% endif %} +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: integration-tests-sa +--- +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: use-integration-tests-scc +subjects: + - kind: ServiceAccount + name: integration-tests-sa +roleRef: + kind: Role + name: use-integration-tests-scc + apiGroup: rbac.authorization.k8s.io diff --git a/tests/templates/kuttl/client-spooling/00-secrets.yaml b/tests/templates/kuttl/client-spooling/00-secrets.yaml new file mode 100644 index 00000000..b9dd795f --- /dev/null +++ b/tests/templates/kuttl/client-spooling/00-secrets.yaml @@ -0,0 +1,62 @@ +--- +apiVersion: v1 +kind: Secret +metadata: + name: minio-credentials + labels: + secrets.stackable.tech/class: s3-credentials-class +stringData: + accessKey: minioAccessKey + secretKey: minioSecretKey + # The following two entries are used by the Bitnami chart for MinIO to + # set up credentials for accessing buckets managed by the MinIO tenant. + root-user: minioAccessKey + root-password: minioSecretKey +--- +apiVersion: secrets.stackable.tech/v1alpha1 +kind: SecretClass +metadata: + name: s3-credentials-class +spec: + backend: + k8sSearch: + searchNamespace: + pod: {} +--- +apiVersion: secrets.stackable.tech/v1alpha1 +kind: SecretClass +metadata: + name: minio-tls-certificates +spec: + backend: + k8sSearch: + searchNamespace: + pod: {} +--- +apiVersion: v1 +kind: Secret +metadata: + name: minio-tls-certificates + labels: + secrets.stackable.tech/class: minio-tls-certificates +# Have a look at the folder certs on how to create this +data: + ca.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUQyVENDQXNHZ0F3SUJBZ0lVTmpxdUdZV3R5SjVhNnd5MjNIejJHUmNNbHdNd0RRWUpLb1pJaHZjTkFRRUwKQlFBd2V6RUxNQWtHQTFVRUJoTUNSRVV4R3pBWkJnTlZCQWdNRWxOamFHeGxjM2RwWnkxSWIyeHpkR1ZwYmpFTwpNQXdHQTFVRUJ3d0ZWMlZrWld3eEtEQW1CZ05WQkFvTUgxTjBZV05yWVdKc1pTQlRhV2R1YVc1bklFRjFkR2h2CmNtbDBlU0JKYm1NeEZUQVRCZ05WQkFNTURITjBZV05yWVdKc1pTNWtaVEFnRncweU16QTJNVFl4TWpVeE1ESmEKR0E4eU1USXpNRFV5TXpFeU5URXdNbG93ZXpFTE1Ba0dBMVVFQmhNQ1JFVXhHekFaQmdOVkJBZ01FbE5qYUd4bApjM2RwWnkxSWIyeHpkR1ZwYmpFT01Bd0dBMVVFQnd3RlYyVmtaV3d4S0RBbUJnTlZCQW9NSDFOMFlXTnJZV0pzClpTQlRhV2R1YVc1bklFRjFkR2h2Y21sMGVTQkpibU14RlRBVEJnTlZCQU1NREhOMFlXTnJZV0pzWlM1a1pUQ0MKQVNJd0RRWUpLb1pJaHZjTkFRRUJCUUFEZ2dFUEFEQ0NBUW9DZ2dFQkFOblYvdmJ5M1JvNTdhMnF2UVJubjBqZQplS01VMitGMCtsWk5DQXZpR1VENWJtOGprOTFvUFpuazBiaFFxZXlFcm1EUzRXVDB6ZXZFUklCSkpEamZMMEQ4CjQ2QmU3UGlNS2UwZEdqb3FJM3o1Y09JZWpjOGFMUEhTSWxnTjZsVDNmSXJ1UzE2Y29RZ0c0dWFLaUhGNStlV0YKRFJVTGR1NmRzWXV6NmRLanFSaVVPaEh3RHd0VUprRHdQditFSXRxbzBIK01MRkxMWU0wK2xFSWFlN2RONUNRNQpTbzVXaEwyY3l2NVZKN2xqL0VBS0NWaUlFZ0NtekRSRGNSZ1NTald5SDRibjZ5WDIwMjZmUEl5V0pGeUVkTC82CmpBT0pBRERSMEd5aE5PWHJFZXFob2NTTW5JYlFWcXdBVDBrTWh1WFN2d3Zscm5MeVRwRzVqWm00bFVNMzRrTUMKQXdFQUFhTlRNRkV3SFFZRFZSME9CQllFRkVJM1JNTWl5aUJqeVExUlM4bmxPUkpWZDFwQk1COEdBMVVkSXdRWQpNQmFBRkVJM1JNTWl5aUJqeVExUlM4bmxPUkpWZDFwQk1BOEdBMVVkRXdFQi93UUZNQU1CQWY4d0RRWUpLb1pJCmh2Y05BUUVMQlFBRGdnRUJBSHRLUlhkRmR0VWh0VWpvZG1ZUWNlZEFEaEhaT2hCcEtpbnpvdTRicmRrNEhmaEYKTHIvV0ZsY1JlbWxWNm1Cc0xweU11SytUZDhaVUVRNkpFUkx5NmxTL2M2cE9HeG5CNGFDbEU4YXQrQytUakpBTwpWbTNXU0k2VlIxY0ZYR2VaamxkVlE2eGtRc2tNSnpPN2RmNmlNVFB0VjVSa01lSlh0TDZYYW1FaTU0ckJvZ05ICk5yYStFSkJRQmwvWmU5ME5qZVlidjIwdVFwWmFhWkZhYVNtVm9OSERwQndsYTBvdXkrTWpPYkMzU3BnT3ExSUMKUGwzTnV3TkxWOFZiT3I1SHJoUUFvS21nU05iM1A4dmFUVnV4L1gwWWZqeS9TN045a1BCYUs5bUZqNzR6d1Y5dwpxU1ExNEtsNWpPM1YzaHJHV1laRWpET2diWnJyRVgxS1hFdXN0K1E9Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K + tls.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUR5RENDQXJDZ0F3SUJBZ0lVQ0kyUE5OcnR6cDZRbDdHa3VhRnhtRGE2VUJvd0RRWUpLb1pJaHZjTkFRRUwKQlFBd2V6RUxNQWtHQTFVRUJoTUNSRVV4R3pBWkJnTlZCQWdNRWxOamFHeGxjM2RwWnkxSWIyeHpkR1ZwYmpFTwpNQXdHQTFVRUJ3d0ZWMlZrWld3eEtEQW1CZ05WQkFvTUgxTjBZV05yWVdKc1pTQlRhV2R1YVc1bklFRjFkR2h2CmNtbDBlU0JKYm1NeEZUQVRCZ05WQkFNTURITjBZV05yWVdKc1pTNWtaVEFnRncweU16QTJNVFl4TWpVeE1ESmEKR0E4eU1USXpNRFV5TXpFeU5URXdNbG93WGpFTE1Ba0dBMVVFQmhNQ1JFVXhHekFaQmdOVkJBZ01FbE5qYUd4bApjM2RwWnkxSWIyeHpkR1ZwYmpFT01Bd0dBMVVFQnd3RlYyVmtaV3d4RWpBUUJnTlZCQW9NQ1ZOMFlXTnJZV0pzClpURU9NQXdHQTFVRUF3d0ZiV2x1YVc4d2dnRWlNQTBHQ1NxR1NJYjNEUUVCQVFVQUE0SUJEd0F3Z2dFS0FvSUIKQVFDanluVnorWEhCOE9DWTRwc0VFWW1qb2JwZHpUbG93d2NTUU4rWURQQ2tCZW9yMFRiODdFZ0x6SksrSllidQpwb1hCbE5JSlBRYW93SkVvL1N6U2s4ZnUyWFNNeXZBWlk0RldHeEp5Mnl4SXh2UC9pYk9HT1l1aVBHWEsyNHQ2ClpjR1RVVmhhdWlaR1Nna1dyZWpXV2g3TWpGUytjMXZhWVpxQitRMXpQczVQRk1sYzhsNVYvK2I4WjdqTUppODQKbU9mSVB4amt2SXlKcjVVa2VGM1VmTHFKUzV5NExGNHR5NEZ0MmlBZDdiYmZIYW5mdlltdjZVb0RWdE1YdFdvMQpvUVBmdjNzaFdybVJMenc2ZXVJQXRiWGM1Q2pCeUlha0NiaURuQVU4cktnK0IxSjRtdlFnckx3bzNxUHJ5Smd4ClNkaWRtWjJtRVI3RXorYzVCMG0vTGlJaEFnTUJBQUdqWHpCZE1Cc0dBMVVkRVFRVU1CS0NCVzFwYm1sdmdnbHMKYjJOaGJHaHZjM1F3SFFZRFZSME9CQllFRkpRMGdENWtFdFFyK3REcERTWjdrd1o4SDVoR01COEdBMVVkSXdRWQpNQmFBRkVJM1JNTWl5aUJqeVExUlM4bmxPUkpWZDFwQk1BMEdDU3FHU0liM0RRRUJDd1VBQTRJQkFRQmNkaGQrClI0Sm9HdnFMQms1OWRxSVVlY2N0dUZzcmRQeHNCaU9GaFlOZ1pxZWRMTTBVTDVEenlmQUhmVk8wTGZTRURkZFgKUkpMOXlMNytrTVUwVDc2Y3ZkQzlYVkFJRTZIVXdUbzlHWXNQcXN1eVpvVmpOcEVESkN3WTNDdm9ubEpWZTRkcQovZ0FiSk1ZQitUU21ZNXlEUHovSkZZL1haellhUGI3T2RlR3VqYlZUNUl4cDk3QXBTOFlJaXY3M0Mwd1ViYzZSCmgwcmNmUmJ5a1NRVWg5dmdWZFhSU1I4RFQzV0NmZHFOek5CWVh2OW1xZlc1ejRzYkdqK2wzd1VsL0kzRi9tSXcKZnlPNEN0aTRha2lHVkhsZmZFeTB3a3pWYUJ4aGNYajJJM0JVVGhCNFpxamxzc2llVmFGa3d2WG1teVJUMG9FVwo1SCtOUEhjcXVTMXpQc2NsCi0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K + tls.key: LS0tLS1CRUdJTiBQUklWQVRFIEtFWS0tLS0tCk1JSUV2QUlCQURBTkJna3Foa2lHOXcwQkFRRUZBQVNDQktZd2dnU2lBZ0VBQW9JQkFRQ2p5blZ6K1hIQjhPQ1kKNHBzRUVZbWpvYnBkelRsb3d3Y1NRTitZRFBDa0Jlb3IwVGI4N0VnTHpKSytKWWJ1cG9YQmxOSUpQUWFvd0pFbwovU3pTazhmdTJYU015dkFaWTRGV0d4SnkyeXhJeHZQL2liT0dPWXVpUEdYSzI0dDZaY0dUVVZoYXVpWkdTZ2tXCnJlaldXaDdNakZTK2MxdmFZWnFCK1ExelBzNVBGTWxjOGw1Vi8rYjhaN2pNSmk4NG1PZklQeGprdkl5SnI1VWsKZUYzVWZMcUpTNXk0TEY0dHk0RnQyaUFkN2JiZkhhbmZ2WW12NlVvRFZ0TVh0V28xb1FQZnYzc2hXcm1STHp3NgpldUlBdGJYYzVDakJ5SWFrQ2JpRG5BVThyS2crQjFKNG12UWdyTHdvM3FQcnlKZ3hTZGlkbVoybUVSN0V6K2M1CkIwbS9MaUloQWdNQkFBRUNnZ0VBQWQzdDVzdUNFMjdXY0llc3NxZ3NoSFAwZHRzKyswVzF6K3h6WC8xTnhPRFkKWVhWNkJmbi9mRHJ4dFQ4aVFaZ2VVQzJORTFQaHZveXJXdWMvMm9xYXJjdEd1OUFZV29HNjJLdG9VMnpTSFdZLwpJN3VERTFXV2xOdlJZVFdOYW5DOGV4eGpRRzE4d0RKWjFpdFhTeEl0NWJEM3lrL3dUUlh0dCt1SnpyVjVqb2N1CmNoeERMd293aXUxQWo2ZFJDWk5CejlUSnh5TnI1ME5ZVzJVWEJhVC84N1hyRkZkSndNVFZUMEI3SE9uRzdSQlYKUWxLdzhtcVZiYU5lbmhjdk1qUjI5c3hUekhSK2p4SU8zQndPNk9Hai9PRmhGQllVN1RMWGVsZDFxb2UwdmIyRwpiOGhQcEd1cHRyNUF0OWx3MXc1d1EzSWdpdXRQTkg1cXlEeUNwRWw2RVFLQmdRRGNkYnNsT2ZLSmo3TzJMQXlZCkZ0a1RwaWxFMFYzajBxbVE5M0lqclY0K0RSbUxNRUIyOTk0MDdCVVlRUWoxL0RJYlFjb1oyRUVjVUI1cGRlSHMKN0RNRUQ2WExIYjJKVTEyK2E3c1d5Q05kS2VjZStUNy9JYmxJOFR0MzQwVWxIUTZ6U01TRGNqdmZjRkhWZ3YwcwpDYWpoRng3TmtMRVhUWnI4ZlQzWUloajR2UUtCZ1FDK01nWjFVbW9KdzlJQVFqMnVJVTVDeTl4aldlWURUQU8vCllhWEl6d2xnZTQzOE1jYmI0Y04yU2FOU0dEZ1Y3bnU1a3FpaWhwalBZV0lpaU9CcDlrVFJIWE9kUFc0N3N5ZUkKdDNrd3JwMnpWbFVnbGNNWlo2bW1WM1FWYUFOWmdqVTRSU3Y0ZS9WeFVMamJaYWZqUHRaUnNqWkdwSzBZVTFvdApWajhJZVE3Zk5RS0JnQ1ArWk11ekpsSW5VQ1FTRlF4UHpxbFNtN0pNckpPaHRXV2h3TlRxWFZTc050dHV5VmVqCktIaGpneDR1b0JQcFZSVDJMTlVEWmI0RnByRjVPYVhBK3FOVEdyS0s3SU1iUlZidHArSVVVeEhHNGFGQStIUVgKUVhVVFRhNUpRT1RLVmJnWHpWM1lyTVhTUk1valZNcDMyVWJHeTVTc1p2MXpBamJ2QzhYWjYxSFJBb0dBZEJjUQp2aGU1eFpBUzVEbUtjSGkvemlHa3ViZXJuNk9NUGdxYUtJSEdsVytVOExScFR0ajBkNFRtL1Rydk1PUEovVEU1CllVcUtoenBIcmhDaCtjdHBvY0k2U1dXdm5SenpLbzNpbVFaY0Y1VEFqUTBjY3F0RmI5UzlkRHR5bi9YTUNqYWUKYWlNdll5VUVVRll5TFpDelBGWnNycDNoVVpHKzN5RmZoQXB3TzJrQ2dZQkh3WWFQSWRXNld3NytCMmhpbjBvdwpqYTNjZXN2QTRqYU1Qd1NMVDhPTnRVMUdCU01md2N6TWJuUEhMclJ2Qjg3bjlnUGFSMndRR1VtckZFTzNMUFgvCmtSY09HcFlCSHBEWEVqRGhLa1dkUnVMT0ZnNEhMWmRWOEFOWmxRMFZTY0U4dTNkRERVTzg5cEdEbjA4cVRBcmwKeDlreHN1ZEVWcmtlclpiNVV4RlZxUT09Ci0tLS0tRU5EIFBSSVZBVEUgS0VZLS0tLS0K +--- +apiVersion: s3.stackable.tech/v1alpha1 +kind: S3Connection +metadata: + name: minio +spec: + host: minio + port: 9000 + accessStyle: Path + credentials: + secretClass: s3-credentials-class + tls: + verification: + server: + caCert: + secretClass: minio-tls-certificates diff --git a/tests/templates/kuttl/client-spooling/01-assert.yaml b/tests/templates/kuttl/client-spooling/01-assert.yaml new file mode 100644 index 00000000..4d24ed7d --- /dev/null +++ b/tests/templates/kuttl/client-spooling/01-assert.yaml @@ -0,0 +1,17 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 600 +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: minio +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: v1 +kind: Service +metadata: + name: minio diff --git a/tests/templates/kuttl/client-spooling/01-install-minio.yaml b/tests/templates/kuttl/client-spooling/01-install-minio.yaml new file mode 100644 index 00000000..7a063b49 --- /dev/null +++ b/tests/templates/kuttl/client-spooling/01-install-minio.yaml @@ -0,0 +1,11 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - script: >- + helm install minio + --namespace $NAMESPACE + --version 17.0.19 + -f 01_helm-bitnami-minio-values.yaml + oci://registry-1.docker.io/bitnamicharts/minio + timeout: 240 diff --git a/tests/templates/kuttl/client-spooling/01_helm-bitnami-minio-values.yaml b/tests/templates/kuttl/client-spooling/01_helm-bitnami-minio-values.yaml new file mode 100644 index 00000000..72b41c0c --- /dev/null +++ b/tests/templates/kuttl/client-spooling/01_helm-bitnami-minio-values.yaml @@ -0,0 +1,79 @@ +--- +global: + security: + allowInsecureImages: true + +image: + repository: bitnamilegacy/minio +clientImage: + repository: bitnamilegacy/minio-client +defaultInitContainers: + volumePermissions: # volumePermissions moved under defaultInitContainers starting with Chart version 17.0.0 + enabled: false + image: + repository: bitnamilegacy/os-shell +console: + image: + repository: bitnamilegacy/minio-object-browser + +mode: standalone +disableWebUI: false +extraEnvVars: + - name: BITNAMI_DEBUG + value: "true" + - name: MINIO_LOG_LEVEL + value: DEBUG + +provisioning: + enabled: true + buckets: + - name: spooling-bucket + resources: + requests: + memory: 1Gi + cpu: "512m" + limits: + memory: "1Gi" + cpu: "1" + podSecurityContext: + enabled: false + containerSecurityContext: + enabled: false + +# volumePermissions can be removed starting with Chart version 17.0.0, moved under defaultInitContainers +volumePermissions: + enabled: false + image: + repository: bitnamilegacy/os-shell + +podSecurityContext: + enabled: false + +containerSecurityContext: + enabled: false + +persistence: + enabled: false + +resources: + requests: + memory: 1Gi + cpu: "512m" + limits: + memory: "1Gi" + cpu: "1" + +auth: + existingSecret: minio-credentials + +service: + type: NodePort + +tls: + enabled: true + autoGenerated: + enabled: false + existingCASecret: minio-tls-certificates + existingSecret: minio-tls-certificates + server: + existingSecret: minio-tls-certificates diff --git a/tests/templates/kuttl/client-spooling/02-assert.yaml b/tests/templates/kuttl/client-spooling/02-assert.yaml new file mode 100644 index 00000000..77a5cac0 --- /dev/null +++ b/tests/templates/kuttl/client-spooling/02-assert.yaml @@ -0,0 +1,25 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 600 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: trino-coordinator-default +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: trino-worker-default +status: + readyReplicas: 2 + replicas: 2 +--- +apiVersion: trino.stackable.tech/v1alpha1 +kind: TrinoCatalog +metadata: + name: tpch diff --git a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 new file mode 100644 index 00000000..84404a5d --- /dev/null +++ b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 @@ -0,0 +1,55 @@ +--- +apiVersion: trino.stackable.tech/v1alpha1 +kind: TrinoCluster +metadata: + name: trino +spec: + image: +{% if test_scenario['values']['trino'].find(",") > 0 %} + custom: "{{ test_scenario['values']['trino'].split(',')[1] }}" + productVersion: "{{ test_scenario['values']['trino'].split(',')[0] }}" +{% else %} + productVersion: "{{ test_scenario['values']['trino'] }}" +{% endif %} + pullPolicy: IfNotPresent + clusterConfig: + catalogLabelSelector: + matchLabels: + trino: trino + clientSpoolingProtocol: + enabled: true + location: "s3://spooling-bucket/trino/" + filesystem: + s3: + connection: + reference: "minio" +{% if lookup('env', 'VECTOR_AGGREGATOR') %} + vectorAggregatorConfigMapName: vector-aggregator-discovery +{% endif %} + coordinators: + config: + logging: + enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} + roleGroups: + default: + replicas: 1 + config: {} + workers: + config: + gracefulShutdownTimeout: 5s # Let the test run faster + logging: + enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} + roleGroups: + default: + replicas: 2 + config: {} +--- +apiVersion: trino.stackable.tech/v1alpha1 +kind: TrinoCatalog +metadata: + name: tpch + labels: + trino: trino +spec: + connector: + tpch: {} diff --git a/tests/templates/kuttl/client-spooling/03-assert.yaml b/tests/templates/kuttl/client-spooling/03-assert.yaml new file mode 100644 index 00000000..71abb09d --- /dev/null +++ b/tests/templates/kuttl/client-spooling/03-assert.yaml @@ -0,0 +1,11 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 300 +--- +apiVersion: v1 +kind: Pod +metadata: + name: trino-test-helper +status: + phase: Running diff --git a/tests/templates/kuttl/client-spooling/03-install-test-helper.yaml b/tests/templates/kuttl/client-spooling/03-install-test-helper.yaml new file mode 100644 index 00000000..1aa8ed9f --- /dev/null +++ b/tests/templates/kuttl/client-spooling/03-install-test-helper.yaml @@ -0,0 +1,22 @@ +--- +apiVersion: v1 +kind: Pod +metadata: + name: trino-test-helper + labels: + app: trino-test-helper +spec: + serviceAccount: integration-tests-sa + containers: + - name: trino-test-helper + image: python:3.13.7-slim-trixie + command: ["bash", "-c"] + args: + - "sleep infinity" + resources: + requests: + cpu: "250m" + memory: "512Mi" + limits: + cpu: "250m" + memory: "1024Mi" diff --git a/tests/templates/kuttl/client-spooling/04-copy-scripts.yaml b/tests/templates/kuttl/client-spooling/04-copy-scripts.yaml new file mode 100644 index 00000000..ccabd224 --- /dev/null +++ b/tests/templates/kuttl/client-spooling/04-copy-scripts.yaml @@ -0,0 +1,6 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - script: kubectl cp -n $NAMESPACE ../../../../templates/kuttl/commons/check-active-workers.py trino-test-helper:/tmp || true + - script: kubectl cp -n $NAMESPACE 04_check-fte.py trino-test-helper:/tmp/ diff --git a/tests/templates/kuttl/client-spooling/04_check-fte.py b/tests/templates/kuttl/client-spooling/04_check-fte.py new file mode 100644 index 00000000..9daaf164 --- /dev/null +++ b/tests/templates/kuttl/client-spooling/04_check-fte.py @@ -0,0 +1,76 @@ +#!/usr/bin/env python +import trino +import argparse + + +def get_connection(coordinator): + """Create anonymous connection for basic cluster health check""" + conn = trino.dbapi.connect( + host=coordinator, + port=8443, + user="test", + http_scheme="https", + verify=False, + session_properties={"query_max_execution_time": "60s"}, + ) + return conn + + +if __name__ == "__main__": + # Construct an argument parser + all_args = argparse.ArgumentParser() + + # Add arguments to the parser + all_args.add_argument( + "-c", + "--coordinator", + required=True, + help="Trino Coordinator Host to connect to", + ) + + args = vars(all_args.parse_args()) + + conn = get_connection(args["coordinator"]) + + try: + cursor = conn.cursor() + + # Test that TPCH connector is working + cursor.execute("SELECT COUNT(*) FROM tpch.tiny.nation") + result = cursor.fetchone() + if result[0] != 25: # TPCH tiny.nation has 25 rows + print(f"TPCH test failed: expected 25 nations, got {result[0]}") + exit(-1) + + print("TPCH connector test passed") + + # Test a more complex query + cursor.execute(""" + SELECT + nation.name, + COUNT(*) AS num_cust + FROM + tpch.tiny.customer + JOIN + tpch.tiny.nation ON customer.nationkey = nation.nationkey + GROUP BY + nation.name + ORDER BY + num_cust DESC + """) + results = cursor.fetchall() + if len(results) == 0: + print("Complex query returned no results") + exit(-1) + + print("Complex query test passed") + + cursor.close() + conn.close() + + except Exception as e: + print(f"Test failed with error: {e}") + import traceback + + traceback.print_exc() + exit(-1) diff --git a/tests/templates/kuttl/client-spooling/05-assert.yaml b/tests/templates/kuttl/client-spooling/05-assert.yaml new file mode 100644 index 00000000..d30e7819 --- /dev/null +++ b/tests/templates/kuttl/client-spooling/05-assert.yaml @@ -0,0 +1,32 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 300 +commands: + - script: kubectl exec -n $NAMESPACE trino-test-helper -- python /tmp/check-active-workers.py -u admin -p "" -c trino-coordinator -w 2 + # Verify that the spooling bucket contains data + - script: | + kubectl exec -n $NAMESPACE deployment/minio -- mc alias set local https://localhost:9000 minioAccessKey minioSecretKey --api S3v4 + count=$(kubectl exec -n $NAMESPACE deployment/minio -- mc stat --insecure local/spooling-bucket | awk '/Objects count:/ {print $3}') + if [ "$count" -gt 0 ]; then + echo "Objects count is $count (> 0)" + else + echo "Objects count is $count (not > 0)" + exit 1 + fi +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: trino-coordinator-default +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: trino-worker-default +status: + readyReplicas: 2 + replicas: 2 diff --git a/tests/templates/kuttl/client-spooling/05-run-tests.yaml b/tests/templates/kuttl/client-spooling/05-run-tests.yaml new file mode 100644 index 00000000..53c71647 --- /dev/null +++ b/tests/templates/kuttl/client-spooling/05-run-tests.yaml @@ -0,0 +1,7 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - script: kubectl exec -n $NAMESPACE trino-test-helper -- pip install --no-cache-dir trino==0.336.0 + - script: kubectl exec -n $NAMESPACE trino-test-helper -- python /tmp/04_check-fte.py -c trino-coordinator + timeout: 120 diff --git a/tests/test-definition.yaml b/tests/test-definition.yaml index 6102c5ca..ae4c4df7 100644 --- a/tests/test-definition.yaml +++ b/tests/test-definition.yaml @@ -115,6 +115,10 @@ tests: dimensions: - trino - openshift + - name: client-spooling + dimensions: + - trino + - openshift - name: listener dimensions: - trino From 318c6008acac704e16d29802e34480df37ed304e Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Thu, 4 Sep 2025 12:48:21 +0200 Subject: [PATCH 05/39] fix spool secret length --- rust/operator-binary/src/controller.rs | 36 +++++++++-- .../src/crd/client_protocol.rs | 59 +++++++++++-------- 2 files changed, 64 insertions(+), 31 deletions(-) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 101038ed..370efa98 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -527,14 +527,18 @@ pub async fn reconcile_trino( create_random_secret( &shared_internal_secret_name(trino), ENV_INTERNAL_SECRET, + 512, trino, client, ) .await?; + // This secret is created even if spooling is not configured. + // Trino currently requires the secret to be exactly 256 bits long. create_random_secret( &shared_spooling_secret_name(trino), ENV_SPOOLING_SECRET, + 32, trino, client, ) @@ -796,6 +800,16 @@ fn build_rolegroup_config_map( ); } + // Add spooling properties from resolved configuration + if let Some(resolved_spooling) = resolved_spooling_config { + dynamic_resolved_config.extend( + resolved_spooling + .config_properties + .iter() + .map(|(k, v)| (k.clone(), Some(v.clone()))), + ); + } + // Add static properties and overrides dynamic_resolved_config.extend(transformed_config); @@ -1027,8 +1041,19 @@ fn build_rolegroup_statefulset( // additional authentication env vars let mut env = trino_authentication_config.env_vars(trino_role, &Container::Trino); - let secret_name = shared_internal_secret_name(trino); - env.push(env_var_from_secret(&secret_name, None, ENV_INTERNAL_SECRET)); + let internal_secret_name = shared_internal_secret_name(trino); + env.push(env_var_from_secret( + &internal_secret_name, + None, + ENV_INTERNAL_SECRET, + )); + + let spooling_secret_name = shared_spooling_secret_name(trino); + env.push(env_var_from_secret( + &spooling_secret_name, + None, + ENV_SPOOLING_SECRET, + )); trino_authentication_config .add_authentication_pod_and_volume_config( @@ -1466,11 +1491,12 @@ fn build_recommended_labels<'a>( async fn create_random_secret( secret_name: &str, secret_key: &str, + secret_byte_size: usize, trino: &v1alpha1::TrinoCluster, client: &Client, ) -> Result<()> { let mut internal_secret = BTreeMap::new(); - internal_secret.insert(secret_key.to_string(), get_random_base64()); + internal_secret.insert(secret_key.to_string(), get_random_base64(secret_byte_size)); let secret = Secret { immutable: Some(true), @@ -1513,8 +1539,8 @@ fn shared_spooling_secret_name(trino: &v1alpha1::TrinoCluster) -> String { format!("{}-spooling-secret", trino.name_any()) } -fn get_random_base64() -> String { - let mut buf = [0; 512]; +fn get_random_base64(byte_size: usize) -> String { + let mut buf: Vec = vec![0; byte_size]; openssl::rand::rand_bytes(&mut buf).unwrap(); openssl::base64::encode_block(&buf) } diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 4a038f10..4a0e4005 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -84,6 +84,9 @@ pub struct S3SpoolingConfig { } pub struct ResolvedSpoolingProtocolConfig { + /// Properties to add to config.properties + pub config_properties: BTreeMap, + // Properties for spooling-manager.properties pub spooling_manager_properties: BTreeMap, @@ -110,22 +113,9 @@ impl ResolvedSpoolingProtocolConfig { client: Option<&Client>, namespace: &str, ) -> Result { - let mut spooling_manager_properties = BTreeMap::new(); - - spooling_manager_properties.insert( - "protocol.spooling.enabled".to_string(), - config.enabled.to_string(), - ); - - spooling_manager_properties.insert( - "protocol.spooling.shared-secret-key".to_string(), - format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), - ); - - spooling_manager_properties.insert("fs.location".to_string(), config.location.clone()); - let mut resolved_config = Self { - spooling_manager_properties, + config_properties: BTreeMap::new(), + spooling_manager_properties: BTreeMap::new(), volumes: Vec::new(), volume_mounts: Vec::new(), load_env_from_files: BTreeMap::new(), @@ -144,6 +134,26 @@ impl ResolvedSpoolingProtocolConfig { } } + resolved_config.spooling_manager_properties.extend([ + ("fs.location".to_string(), config.location.clone()), + ( + "spooling-manager.name".to_string(), + "filesystem".to_string(), + ), + ]); + + // Enable spooling protocol + resolved_config.config_properties.extend([ + ( + "protocol.spooling.enabled".to_string(), + config.enabled.to_string(), + ), + ( + "protocol.spooling.shared-secret-key".to_string(), + format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), + ), + ]); + // Finally, extend the spooling manager properties with any user configuration if let Some(user_config) = config.config_overrides.as_ref() { resolved_config @@ -284,17 +294,15 @@ mod tests { .unwrap(); let expected_props = BTreeMap::from([ - ("protocol.spooling.enabled".to_string(), "true".to_string()), - ( - "protocol.spooling.shared-secret-key".to_string(), - format!("${{ENV:{}}}", ENV_SPOOLING_SECRET), - ), ( "fs.location".to_string(), "s3://my-bucket/spooling".to_string(), ), + ( + "spooling-manager.name".to_string(), + "filesystem".to_string(), + ), ]); - assert_eq!( expected_props, resolved_spooling_config.spooling_manager_properties @@ -330,11 +338,6 @@ mod tests { .unwrap(); let expected_props = BTreeMap::from([ - ("protocol.spooling.enabled".to_string(), "true".to_string()), - ( - "protocol.spooling.shared-secret-key".to_string(), - format!("${{ENV:{}}}", ENV_SPOOLING_SECRET), - ), ( "fs.location".to_string(), "s3://my-bucket/spooling".to_string(), @@ -343,6 +346,10 @@ mod tests { "protocol.spooling.retrieval-mode".to_string(), "STORAGE".to_string(), ), + ( + "spooling-manager.name".to_string(), + "filesystem".to_string(), + ), ]); assert_eq!( From 202e25f5b8527b44f8aa34d19cd711ea49e4bf14 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Thu, 4 Sep 2025 15:26:08 +0200 Subject: [PATCH 06/39] integration test successful --- .../03-install-test-helper.yaml | 4 +- .../kuttl/client-spooling/04_check-fte.py | 50 +++++++------------ .../kuttl/client-spooling/05-assert.yaml | 2 +- 3 files changed, 21 insertions(+), 35 deletions(-) diff --git a/tests/templates/kuttl/client-spooling/03-install-test-helper.yaml b/tests/templates/kuttl/client-spooling/03-install-test-helper.yaml index 1aa8ed9f..bff62e61 100644 --- a/tests/templates/kuttl/client-spooling/03-install-test-helper.yaml +++ b/tests/templates/kuttl/client-spooling/03-install-test-helper.yaml @@ -16,7 +16,7 @@ spec: resources: requests: cpu: "250m" - memory: "512Mi" + memory: "1024Mi" limits: cpu: "250m" - memory: "1024Mi" + memory: "2048Mi" diff --git a/tests/templates/kuttl/client-spooling/04_check-fte.py b/tests/templates/kuttl/client-spooling/04_check-fte.py index 9daaf164..3e07d1e4 100644 --- a/tests/templates/kuttl/client-spooling/04_check-fte.py +++ b/tests/templates/kuttl/client-spooling/04_check-fte.py @@ -1,6 +1,7 @@ #!/usr/bin/env python import trino import argparse +import urllib3 def get_connection(coordinator): @@ -17,6 +18,8 @@ def get_connection(coordinator): if __name__ == "__main__": + urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) + # Construct an argument parser all_args = argparse.ArgumentParser() @@ -35,42 +38,25 @@ def get_connection(coordinator): try: cursor = conn.cursor() - # Test that TPCH connector is working - cursor.execute("SELECT COUNT(*) FROM tpch.tiny.nation") - result = cursor.fetchone() - if result[0] != 25: # TPCH tiny.nation has 25 rows - print(f"TPCH test failed: expected 25 nations, got {result[0]}") - exit(-1) - - print("TPCH connector test passed") + print("🚜 fetching many rows from Trino to trigger spooling...") - # Test a more complex query - cursor.execute(""" - SELECT - nation.name, - COUNT(*) AS num_cust - FROM - tpch.tiny.customer - JOIN - tpch.tiny.nation ON customer.nationkey = nation.nationkey - GROUP BY - nation.name - ORDER BY - num_cust DESC - """) - results = cursor.fetchall() - if len(results) == 0: - print("Complex query returned no results") - exit(-1) + cursor.execute("SELECT * FROM tpch.sf100.customer") + customer_count = 0 + batch = 10 + while batch > 0: + print(f"⏳ fetching batch {batch} of 1000 rows...") + cursor.fetchmany(1_000) + customer_count += 1_000 + batch = batch - 1 + # assert customer_count == 15_000_000, f"TPCH test failed: expected 15 mil customers, got {customer_count}" - print("Complex query test passed") + print("🎉 major success!") cursor.close() - conn.close() except Exception as e: - print(f"Test failed with error: {e}") - import traceback + print(f"💀 oh noes! cannot fetch customers from Trino: {e}") + raise e - traceback.print_exc() - exit(-1) + finally: + conn.close() diff --git a/tests/templates/kuttl/client-spooling/05-assert.yaml b/tests/templates/kuttl/client-spooling/05-assert.yaml index d30e7819..1c77f67e 100644 --- a/tests/templates/kuttl/client-spooling/05-assert.yaml +++ b/tests/templates/kuttl/client-spooling/05-assert.yaml @@ -4,9 +4,9 @@ kind: TestAssert timeout: 300 commands: - script: kubectl exec -n $NAMESPACE trino-test-helper -- python /tmp/check-active-workers.py -u admin -p "" -c trino-coordinator -w 2 + - script: kubectl exec -n $NAMESPACE deployment/minio -- mc alias set local https://localhost:9000 minioAccessKey minioSecretKey --api S3v4 # Verify that the spooling bucket contains data - script: | - kubectl exec -n $NAMESPACE deployment/minio -- mc alias set local https://localhost:9000 minioAccessKey minioSecretKey --api S3v4 count=$(kubectl exec -n $NAMESPACE deployment/minio -- mc stat --insecure local/spooling-bucket | awk '/Objects count:/ {print $3}') if [ "$count" -gt 0 ]; then echo "Objects count is $count (> 0)" From 9e2d73858c2322aa97fa2d27cd0bac951c49f52c Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Thu, 4 Sep 2025 15:41:32 +0200 Subject: [PATCH 07/39] rename python script --- .../kuttl/client-spooling/04-copy-scripts.yaml | 2 +- .../client-spooling/{04_check-fte.py => 04_query.py} | 11 ++++++++++- .../templates/kuttl/client-spooling/05-run-tests.yaml | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) rename tests/templates/kuttl/client-spooling/{04_check-fte.py => 04_query.py} (74%) diff --git a/tests/templates/kuttl/client-spooling/04-copy-scripts.yaml b/tests/templates/kuttl/client-spooling/04-copy-scripts.yaml index ccabd224..b6a3a518 100644 --- a/tests/templates/kuttl/client-spooling/04-copy-scripts.yaml +++ b/tests/templates/kuttl/client-spooling/04-copy-scripts.yaml @@ -3,4 +3,4 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: - script: kubectl cp -n $NAMESPACE ../../../../templates/kuttl/commons/check-active-workers.py trino-test-helper:/tmp || true - - script: kubectl cp -n $NAMESPACE 04_check-fte.py trino-test-helper:/tmp/ + - script: kubectl cp -n $NAMESPACE 04_query.py trino-test-helper:/tmp/ diff --git a/tests/templates/kuttl/client-spooling/04_check-fte.py b/tests/templates/kuttl/client-spooling/04_query.py similarity index 74% rename from tests/templates/kuttl/client-spooling/04_check-fte.py rename to tests/templates/kuttl/client-spooling/04_query.py index 3e07d1e4..89ebf838 100644 --- a/tests/templates/kuttl/client-spooling/04_check-fte.py +++ b/tests/templates/kuttl/client-spooling/04_query.py @@ -38,6 +38,12 @@ def get_connection(coordinator): try: cursor = conn.cursor() + # The table tpch.sf100.customer has 15 million rows but Python consumes + # too much memory to retrieve all of them at once. + # Fetching them one by one is too slow, so we fetch only 10k rows which seems to be enough + # for Trino to use the spooling protocol. + # Fetching less rows is too risky IMO as Trino might decide to not use spooling. + print("🚜 fetching many rows from Trino to trigger spooling...") cursor.execute("SELECT * FROM tpch.sf100.customer") @@ -48,7 +54,10 @@ def get_connection(coordinator): cursor.fetchmany(1_000) customer_count += 1_000 batch = batch - 1 - # assert customer_count == 15_000_000, f"TPCH test failed: expected 15 mil customers, got {customer_count}" + + assert customer_count == 10_000, ( + f"💀 crap! expected 10_000 customers, got {customer_count}" + ) print("🎉 major success!") diff --git a/tests/templates/kuttl/client-spooling/05-run-tests.yaml b/tests/templates/kuttl/client-spooling/05-run-tests.yaml index 53c71647..f2f1cd4a 100644 --- a/tests/templates/kuttl/client-spooling/05-run-tests.yaml +++ b/tests/templates/kuttl/client-spooling/05-run-tests.yaml @@ -3,5 +3,5 @@ apiVersion: kuttl.dev/v1beta1 kind: TestStep commands: - script: kubectl exec -n $NAMESPACE trino-test-helper -- pip install --no-cache-dir trino==0.336.0 - - script: kubectl exec -n $NAMESPACE trino-test-helper -- python /tmp/04_check-fte.py -c trino-coordinator + - script: kubectl exec -n $NAMESPACE trino-test-helper -- python /tmp/04_query.py -c trino-coordinator timeout: 120 From 9086f5567e2f55a617c8f8b5138823ed87de9da5 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Thu, 4 Sep 2025 17:40:35 +0200 Subject: [PATCH 08/39] increase the number of rows to fetch from Trino --- .../kuttl/client-spooling/04_query.py | 27 ++++++++++--------- .../kuttl/client-spooling/05-assert.yaml | 8 +++--- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/tests/templates/kuttl/client-spooling/04_query.py b/tests/templates/kuttl/client-spooling/04_query.py index 89ebf838..7cd1fc7e 100644 --- a/tests/templates/kuttl/client-spooling/04_query.py +++ b/tests/templates/kuttl/client-spooling/04_query.py @@ -40,23 +40,26 @@ def get_connection(coordinator): # The table tpch.sf100.customer has 15 million rows but Python consumes # too much memory to retrieve all of them at once. - # Fetching them one by one is too slow, so we fetch only 10k rows which seems to be enough + # Fetching them one by one is too slow, so we fetch enough rows # for Trino to use the spooling protocol. - # Fetching less rows is too risky IMO as Trino might decide to not use spooling. + # Fetching too few rows is risky as Trino might decide to not use spooling. print("🚜 fetching many rows from Trino to trigger spooling...") - cursor.execute("SELECT * FROM tpch.sf100.customer") customer_count = 0 - batch = 10 - while batch > 0: - print(f"⏳ fetching batch {batch} of 1000 rows...") - cursor.fetchmany(1_000) - customer_count += 1_000 - batch = batch - 1 - - assert customer_count == 10_000, ( - f"💀 crap! expected 10_000 customers, got {customer_count}" + batch_count = 50 + batch_size = 1_000 + expected_customers = batch_count * 1_000 + + cursor.execute("SELECT * FROM tpch.sf100.customer") + while batch_count > 0: + print(f"⏳ fetching batch {batch_count} of {batch_size} rows...") + _ = cursor.fetchmany(batch_size) + customer_count += batch_size + batch_count = batch_count - 1 + + assert customer_count == expected_customers, ( + f"💀 crap! expected {expected_customers} customers, got {customer_count}" ) print("🎉 major success!") diff --git a/tests/templates/kuttl/client-spooling/05-assert.yaml b/tests/templates/kuttl/client-spooling/05-assert.yaml index 1c77f67e..5c2dd20c 100644 --- a/tests/templates/kuttl/client-spooling/05-assert.yaml +++ b/tests/templates/kuttl/client-spooling/05-assert.yaml @@ -7,11 +7,9 @@ commands: - script: kubectl exec -n $NAMESPACE deployment/minio -- mc alias set local https://localhost:9000 minioAccessKey minioSecretKey --api S3v4 # Verify that the spooling bucket contains data - script: | - count=$(kubectl exec -n $NAMESPACE deployment/minio -- mc stat --insecure local/spooling-bucket | awk '/Objects count:/ {print $3}') - if [ "$count" -gt 0 ]; then - echo "Objects count is $count (> 0)" - else - echo "Objects count is $count (not > 0)" + count=$(kubectl exec -n $NAMESPACE deployment/minio -- mc ls --insecure local/spooling-bucket/trino | wc -l) + echo "Number of spool segments is [$count] (should be > 0)" + if [ "$count" -eq 0 ]; then exit 1 fi --- From 0a0c0938bb3f1c976c139e19e39404465c1890a0 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Thu, 4 Sep 2025 17:40:47 +0200 Subject: [PATCH 09/39] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b73fdf84..ccc48559 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,10 @@ All notable changes to this project will be documented in this file. ### Added - Support for fault-tolerant execution ([#779]). +- Support for the client spooling protocol ([#793]). [#779]: https://github.com/stackabletech/trino-operator/pull/779 +[#793]: https://github.com/stackabletech/trino-operator/pull/793 ## [25.7.0] - 2025-07-23 From bea8115556ee219aee2dd41dcb5d64a223cf0bdd Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Thu, 4 Sep 2025 18:09:05 +0200 Subject: [PATCH 10/39] update docs --- .../usage-guide/client-spooling-protocol.adoc | 39 +++++++++++++++++++ docs/modules/trino/partials/nav.adoc | 1 + 2 files changed, 40 insertions(+) create mode 100644 docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc diff --git a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc new file mode 100644 index 00000000..8ae0e338 --- /dev/null +++ b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc @@ -0,0 +1,39 @@ += Client Spooling Protocol +:description: Enable and configure the Client Spooling Protocol in Trino for efficient handling of large result sets. +:keywords: client spooling protocol, Trino, large result sets, memory management +:trino-docs-spooling-url: https://trino.io/docs/476/client/client-protocol.html + +The Client Spooling Protocol in Trino is designed to efficiently handle large result sets. When enabled, this protocol allows the Trino server to spool results to external storage systems, reducing memory consumption and improving performance for queries that return large datasets. + +For more details, refer to the link:{trino-docs-spooling-url}[Trino documentation on Client Spooling Protocol {external-link-icon}^]. + +== Configuration + +The client spooling protocol is disabled by default. +To enable it, you need to set the `spec.clusterConfig.clientSpoolingProtocol` configuration property as shown below. + +[source,yaml] +---- +spec: + clusterConfig: + clientSpoolingProtocol: + enabled: true # <1> + location: "s3://spooling-bucket/trino/" # <2> + filesystem: + s3: # <3> + connection: + reference: "minio" +---- +<1> Enables the client spooling protocol +<2> Specifies the location where spooled data will be stored. This example uses an S3 bucket. +<3> Configures the filesystem type for spooling. Only S3 is supported currently via the custom resource definition. + +The operator automatically fills in additional settings required by Trino, such as the `protocol.spooling.shared-secret-key`. +Users can change the contents of the `spooling-manager.properties` file by adding entries to the optional `spec.clusterConfig.clientSpoolingProtocol.config_overrides` map. + +[IMPORTANT] +==== +Even if enabled, Trino may decide to not use the client spooling protocol for certain queries. Clients cannot force Trino to use it. +==== + +The clients need to have access to the same storage location configured for spooling. diff --git a/docs/modules/trino/partials/nav.adoc b/docs/modules/trino/partials/nav.adoc index a1ff7c0d..02dbac77 100644 --- a/docs/modules/trino/partials/nav.adoc +++ b/docs/modules/trino/partials/nav.adoc @@ -7,6 +7,7 @@ ** xref:trino:usage-guide/listenerclass.adoc[] ** xref:trino:usage-guide/configuration.adoc[] ** xref:trino:usage-guide/fault-tolerant-execution.adoc[] +** xref:trino:usage-guide/client-spooling-protocol.adoc[] ** xref:trino:usage-guide/s3.adoc[] ** xref:trino:usage-guide/security.adoc[] ** xref:trino:usage-guide/monitoring.adoc[] From c5dc376c7ba8d1126391f09b07113f9dff84efd0 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Fri, 5 Sep 2025 09:03:23 +0200 Subject: [PATCH 11/39] remove unused enum --- rust/operator-binary/src/crd/client_protocol.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 4a0e4005..83bca276 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -14,7 +14,6 @@ use stackable_operator::{ }, schemars::{self, JsonSchema}, }; -use strum::Display; use crate::{ command, @@ -42,15 +41,6 @@ pub struct ClientSpoolingProtocolConfig { pub config_overrides: Option>, } -#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize, Display)] -#[strum(serialize_all = "SCREAMING_SNAKE_CASE")] -pub enum SpoolingRetrievalMode { - Storage, - CoordinatorStorageRedirect, - CoordinatorProxy, - WorkerProxy, -} - #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub enum SpoolingFileSystemConfig { From 8f58e0efa08564ea27b05ed04012f9f570f811f6 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Fri, 5 Sep 2025 09:09:46 +0200 Subject: [PATCH 12/39] remove Optional from config_overrides --- deploy/helm/trino-operator/crds/crds.yaml | 2 +- rust/operator-binary/src/crd/client_protocol.rs | 16 +++++++--------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index caba3260..054435d8 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -112,8 +112,8 @@ spec: configOverrides: additionalProperties: type: string + default: {} description: The `configOverrides` allow overriding arbitrary client protocol properties. - nullable: true type: object enabled: description: Enable spooling protocol. diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 83bca276..153c4194 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -38,7 +38,7 @@ pub struct ClientSpoolingProtocolConfig { /// The `configOverrides` allow overriding arbitrary client protocol properties. #[serde(default)] - pub config_overrides: Option>, + pub config_overrides: HashMap, } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -145,11 +145,9 @@ impl ResolvedSpoolingProtocolConfig { ]); // Finally, extend the spooling manager properties with any user configuration - if let Some(user_config) = config.config_overrides.as_ref() { - resolved_config - .spooling_manager_properties - .extend(user_config.clone()); - } + resolved_config + .spooling_manager_properties + .extend(config.config_overrides.clone()); Ok(resolved_config) } @@ -273,7 +271,7 @@ mod tests { max_error_retries: None, upload_part_size: None, }), - config_overrides: None, + config_overrides: HashMap::new(), }; let resolved_spooling_config = ResolvedSpoolingProtocolConfig::from_config( @@ -314,10 +312,10 @@ mod tests { max_error_retries: None, upload_part_size: None, }), - config_overrides: Some(HashMap::from([( + config_overrides: HashMap::from([( "protocol.spooling.retrieval-mode".to_string(), "STORAGE".to_string(), - )])), + )]), }; let resolved_spooling_config = ResolvedSpoolingProtocolConfig::from_config( From 78f2e645ea6071f6524408cd32d0c6103eb9ab0d Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Fri, 5 Sep 2025 10:09:42 +0200 Subject: [PATCH 13/39] remove the "enabled" property --- deploy/helm/trino-operator/crds/crds.yaml | 4 ---- .../pages/usage-guide/client-spooling-protocol.adoc | 10 ++++------ rust/operator-binary/src/crd/client_protocol.rs | 10 +--------- 3 files changed, 5 insertions(+), 19 deletions(-) diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index 054435d8..eafefadf 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -115,9 +115,6 @@ spec: default: {} description: The `configOverrides` allow overriding arbitrary client protocol properties. type: object - enabled: - description: Enable spooling protocol. - type: boolean filesystem: oneOf: - required: @@ -272,7 +269,6 @@ spec: location: type: string required: - - enabled - filesystem - location type: object diff --git a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc index 8ae0e338..4ff29c7d 100644 --- a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc +++ b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc @@ -17,16 +17,14 @@ To enable it, you need to set the `spec.clusterConfig.clientSpoolingProtocol` co spec: clusterConfig: clientSpoolingProtocol: - enabled: true # <1> - location: "s3://spooling-bucket/trino/" # <2> + location: "s3://spooling-bucket/trino/" # <1> filesystem: - s3: # <3> + s3: # <2> connection: reference: "minio" ---- -<1> Enables the client spooling protocol -<2> Specifies the location where spooled data will be stored. This example uses an S3 bucket. -<3> Configures the filesystem type for spooling. Only S3 is supported currently via the custom resource definition. +<1> Specifies the location where spooled data will be stored. This example uses an S3 bucket. +<2> Configures the filesystem type for spooling. Only S3 is supported currently via the custom resource definition. The operator automatically fills in additional settings required by Trino, such as the `protocol.spooling.shared-secret-key`. Users can change the contents of the `spooling-manager.properties` file by adding entries to the optional `spec.clusterConfig.clientSpoolingProtocol.config_overrides` map. diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 153c4194..9c180242 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -26,9 +26,6 @@ const SPOOLING_S3_AWS_SECRET_KEY: &str = "SPOOLING_S3_AWS_SECRET_KEY"; #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub struct ClientSpoolingProtocolConfig { - /// Enable spooling protocol. - pub enabled: bool, - // Spool segment location. Each Trino cluster must have its own // location independent of any other clusters. pub location: String, @@ -134,10 +131,7 @@ impl ResolvedSpoolingProtocolConfig { // Enable spooling protocol resolved_config.config_properties.extend([ - ( - "protocol.spooling.enabled".to_string(), - config.enabled.to_string(), - ), + ("protocol.spooling.enabled".to_string(), "true".to_string()), ( "protocol.spooling.shared-secret-key".to_string(), format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), @@ -259,7 +253,6 @@ mod tests { #[tokio::test] async fn test_spooling_config() { let config = ClientSpoolingProtocolConfig { - enabled: true, location: "s3://my-bucket/spooling".to_string(), filesystem: SpoolingFileSystemConfig::S3(S3SpoolingConfig { connection: @@ -300,7 +293,6 @@ mod tests { #[tokio::test] async fn test_spooling_config_overrides() { let config = ClientSpoolingProtocolConfig { - enabled: true, location: "s3://my-bucket/spooling".to_string(), filesystem: SpoolingFileSystemConfig::S3(S3SpoolingConfig { connection: From e9167100c6459a71c9b2b97adf096c5ce9dbe74c Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Fri, 5 Sep 2025 10:55:19 +0200 Subject: [PATCH 14/39] refactor crd to use spec.clusterConfig.clientProtocol.spooling --- deploy/helm/trino-operator/crds/crds.yaml | 282 +++++++++--------- .../usage-guide/client-spooling-protocol.adoc | 13 +- rust/operator-binary/src/command.rs | 4 +- rust/operator-binary/src/controller.rs | 15 +- .../src/crd/client_protocol.rs | 87 +++--- rust/operator-binary/src/crd/mod.rs | 2 +- .../client-spooling/02-install-trino.yaml.j2 | 14 +- 7 files changed, 216 insertions(+), 201 deletions(-) diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index eafefadf..b6b64b34 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -105,172 +105,178 @@ spec: description: matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels map is equivalent to an element of matchExpressions, whose key field is "key", the operator is "In", and the values array contains only "value". The requirements are ANDed. type: object type: object - clientSpoolingProtocol: + clientProtocol: description: Client spooling protocol configuration. nullable: true + oneOf: + - required: + - spooling properties: - configOverrides: - additionalProperties: - type: string - default: {} - description: The `configOverrides` allow overriding arbitrary client protocol properties. - type: object - filesystem: - oneOf: - - required: - - s3 + spooling: properties: - s3: + configOverrides: + additionalProperties: + type: string + default: {} + description: The `configOverrides` allow overriding arbitrary client protocol properties. + type: object + filesystem: + oneOf: + - required: + - s3 properties: - connection: - description: S3 connection configuration. Learn more about S3 configuration in the [S3 concept docs](https://docs.stackable.tech/home/nightly/concepts/s3). - oneOf: - - required: - - inline - - required: - - reference + s3: properties: - inline: - description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). + connection: + description: S3 connection configuration. Learn more about S3 configuration in the [S3 concept docs](https://docs.stackable.tech/home/nightly/concepts/s3). + oneOf: + - required: + - inline + - required: + - reference properties: - accessStyle: - default: VirtualHosted - description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). - enum: - - Path - - VirtualHosted - type: string - credentials: - description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. - nullable: true + inline: + description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). properties: - scope: - description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' + accessStyle: + default: VirtualHosted + description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). + enum: + - Path + - VirtualHosted + type: string + credentials: + description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. nullable: true properties: - listenerVolumes: - default: [] - description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. - items: - type: string - type: array - node: - default: false - description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. - type: boolean - pod: - default: false - description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. - type: boolean - services: - default: [] - description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. - items: - type: string - type: array + scope: + description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' + nullable: true + properties: + listenerVolumes: + default: [] + description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. + items: + type: string + type: array + node: + default: false + description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. + type: boolean + pod: + default: false + description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. + type: boolean + services: + default: [] + description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. + items: + type: string + type: array + type: object + secretClass: + description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' + type: string + required: + - secretClass type: object - secretClass: - description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' + host: + description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' type: string - required: - - secretClass - type: object - host: - description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' - type: string - port: - description: Port the S3 server listens on. If not specified the product will determine the port to use. - format: uint16 - minimum: 0.0 - nullable: true - type: integer - region: - default: - name: us-east-1 - description: |- - Bucket region used for signing headers (sigv4). + port: + description: Port the S3 server listens on. If not specified the product will determine the port to use. + format: uint16 + minimum: 0.0 + nullable: true + type: integer + region: + default: + name: us-east-1 + description: |- + Bucket region used for signing headers (sigv4). - This defaults to `us-east-1` which is compatible with other implementations such as Minio. + This defaults to `us-east-1` which is compatible with other implementations such as Minio. - WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. - properties: - name: - default: us-east-1 - type: string - type: object - tls: - description: Use a TLS connection. If not specified no TLS will be used. - nullable: true - properties: - verification: - description: The verification method used to verify the certificates of the server and/or the client. - oneOf: - - required: - - none - - required: - - server + WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. properties: - none: - description: Use TLS but don't verify certificates. - type: object - server: - description: Use TLS and a CA certificate to verify the server. + name: + default: us-east-1 + type: string + type: object + tls: + description: Use a TLS connection. If not specified no TLS will be used. + nullable: true + properties: + verification: + description: The verification method used to verify the certificates of the server and/or the client. + oneOf: + - required: + - none + - required: + - server properties: - caCert: - description: CA cert to verify the server. - oneOf: - - required: - - webPki - - required: - - secretClass + none: + description: Use TLS but don't verify certificates. + type: object + server: + description: Use TLS and a CA certificate to verify the server. properties: - secretClass: - description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. - type: string - webPki: - description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. + caCert: + description: CA cert to verify the server. + oneOf: + - required: + - webPki + - required: + - secretClass + properties: + secretClass: + description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. + type: string + webPki: + description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. + type: object type: object + required: + - caCert type: object - required: - - caCert type: object + required: + - verification type: object required: - - verification + - host type: object - required: - - host + reference: + type: string type: object - reference: + externalId: + description: External ID for the IAM role trust policy. + nullable: true + type: string + iamRole: + description: IAM role to assume for S3 access. + nullable: true + type: string + maxErrorRetries: + description: Maximum number of times the S3 client should retry a request. + format: uint32 + minimum: 0.0 + nullable: true + type: integer + uploadPartSize: + description: Part data size for S3 multi-part upload. + nullable: true type: string + required: + - connection type: object - externalId: - description: External ID for the IAM role trust policy. - nullable: true - type: string - iamRole: - description: IAM role to assume for S3 access. - nullable: true - type: string - maxErrorRetries: - description: Maximum number of times the S3 client should retry a request. - format: uint32 - minimum: 0.0 - nullable: true - type: integer - uploadPartSize: - description: Part data size for S3 multi-part upload. - nullable: true - type: string - required: - - connection type: object + location: + type: string + required: + - filesystem + - location type: object - location: - type: string - required: - - filesystem - - location type: object faultTolerantExecution: description: Fault tolerant execution configuration. When enabled, Trino can automatically retry queries or tasks in case of failures. diff --git a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc index 4ff29c7d..22d9594c 100644 --- a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc +++ b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc @@ -16,12 +16,13 @@ To enable it, you need to set the `spec.clusterConfig.clientSpoolingProtocol` co ---- spec: clusterConfig: - clientSpoolingProtocol: - location: "s3://spooling-bucket/trino/" # <1> - filesystem: - s3: # <2> - connection: - reference: "minio" + clientProtocol: + spooling: + location: "s3://spooling-bucket/trino/" # <1> + filesystem: + s3: # <2> + connection: + reference: "minio" ---- <1> Specifies the location where spooled data will be stored. This example uses an S3 bucket. <2> Configures the filesystem type for spooling. Only S3 is supported currently via the custom resource definition. diff --git a/rust/operator-binary/src/command.rs b/rust/operator-binary/src/command.rs index 50ab6fbc..0925b49a 100644 --- a/rust/operator-binary/src/command.rs +++ b/rust/operator-binary/src/command.rs @@ -24,7 +24,7 @@ pub fn container_prepare_args( catalogs: &[CatalogConfig], merged_config: &v1alpha1::TrinoConfig, resolved_fte_config: &Option, - resolved_spooling_config: &Option, + resolved_spooling_config: &Option, ) -> Vec { let mut args = vec![]; @@ -98,7 +98,7 @@ pub fn container_trino_args( authentication_config: &TrinoAuthenticationConfig, catalogs: &[CatalogConfig], resolved_fte_config: &Option, - resolved_spooling_config: &Option, + resolved_spooling_config: &Option, ) -> Vec { let mut args = vec![ // copy config files to a writeable empty folder diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 370efa98..50e81c06 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -464,10 +464,9 @@ pub async fn reconcile_trino( }; // Resolve client spooling protocol configuration with S3 connections if needed - let resolved_spooling_config = match trino.spec.cluster_config.client_spooling_protocol.as_ref() - { + let resolved_client_protocol_config = match trino.spec.cluster_config.client_protocol.as_ref() { Some(spooling_config) => Some( - client_protocol::ResolvedSpoolingProtocolConfig::from_config( + client_protocol::ResolvedClientProtocolConfig::from_config( spooling_config, Some(client), &namespace, @@ -599,7 +598,7 @@ pub async fn reconcile_trino( &trino_opa_config, &client.kubernetes_cluster_info, &resolved_fte_config, - &resolved_spooling_config, + &resolved_client_protocol_config, )?; let rg_catalog_configmap = build_rolegroup_catalog_config_map( trino, @@ -618,7 +617,7 @@ pub async fn reconcile_trino( &catalogs, &rbac_sa.name_any(), &resolved_fte_config, - &resolved_spooling_config, + &resolved_client_protocol_config, )?; cluster_resources @@ -728,7 +727,7 @@ fn build_rolegroup_config_map( trino_opa_config: &Option, cluster_info: &KubernetesClusterInfo, resolved_fte_config: &Option, - resolved_spooling_config: &Option, + resolved_spooling_config: &Option, ) -> Result { let mut cm_conf_data = BTreeMap::new(); @@ -1013,7 +1012,7 @@ fn build_rolegroup_statefulset( catalogs: &[CatalogConfig], sa_name: &str, resolved_fte_config: &Option, - resolved_spooling_config: &Option, + resolved_spooling_config: &Option, ) -> Result { let role = trino .role(trino_role) @@ -1689,7 +1688,7 @@ fn tls_volume_mounts( catalogs: &[CatalogConfig], requested_secret_lifetime: &Duration, resolved_fte_config: &Option, - resolved_spooling_config: &Option, + resolved_spooling_config: &Option, ) -> Result<()> { if let Some(server_tls) = trino.get_server_tls() { cb_prepare diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 9c180242..8809568b 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -23,6 +23,11 @@ use crate::{ 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 { + Spooling(ClientSpoolingProtocolConfig), +} #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub struct ClientSpoolingProtocolConfig { @@ -70,7 +75,7 @@ pub struct S3SpoolingConfig { pub upload_part_size: Option, } -pub struct ResolvedSpoolingProtocolConfig { +pub struct ResolvedClientProtocolConfig { /// Properties to add to config.properties pub config_properties: BTreeMap, @@ -92,11 +97,11 @@ pub struct ResolvedSpoolingProtocolConfig { pub init_container_extra_start_commands: Vec, } -impl ResolvedSpoolingProtocolConfig { +impl ResolvedClientProtocolConfig { /// Resolve S3 connection properties from Kubernetes resources /// and prepare spooling filesystem configuration. pub async fn from_config( - config: &ClientSpoolingProtocolConfig, + config: &ClientProtocolConfig, client: Option<&Client>, namespace: &str, ) -> Result { @@ -109,39 +114,43 @@ impl ResolvedSpoolingProtocolConfig { init_container_extra_start_commands: Vec::new(), }; - // Resolve external resources if Kubernetes client is available - // This should always be the case, except for when this function is called during unit tests - if let Some(client) = client { - match &config.filesystem { - SpoolingFileSystemConfig::S3(s3_config) => { - resolved_config - .resolve_s3_backend(s3_config, client, namespace) - .await?; + match config { + ClientProtocolConfig::Spooling(spooling_config) => { + // Resolve external resources if Kubernetes client is available + // This should always be the case, except for when this function is called during unit tests + if let Some(client) = client { + match &spooling_config.filesystem { + SpoolingFileSystemConfig::S3(s3_config) => { + resolved_config + .resolve_s3_backend(s3_config, client, namespace) + .await?; + } + } } - } - } - resolved_config.spooling_manager_properties.extend([ - ("fs.location".to_string(), config.location.clone()), - ( - "spooling-manager.name".to_string(), - "filesystem".to_string(), - ), - ]); - - // Enable spooling protocol - resolved_config.config_properties.extend([ - ("protocol.spooling.enabled".to_string(), "true".to_string()), - ( - "protocol.spooling.shared-secret-key".to_string(), - format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), - ), - ]); + resolved_config.spooling_manager_properties.extend([ + ("fs.location".to_string(), spooling_config.location.clone()), + ( + "spooling-manager.name".to_string(), + "filesystem".to_string(), + ), + ]); + + // Enable spooling protocol + resolved_config.config_properties.extend([ + ("protocol.spooling.enabled".to_string(), "true".to_string()), + ( + "protocol.spooling.shared-secret-key".to_string(), + format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), + ), + ]); - // Finally, extend the spooling manager properties with any user configuration - resolved_config - .spooling_manager_properties - .extend(config.config_overrides.clone()); + // Finally, extend the spooling manager properties with any user configuration + resolved_config + .spooling_manager_properties + .extend(spooling_config.config_overrides.clone()); + } + } Ok(resolved_config) } @@ -252,7 +261,7 @@ mod tests { #[tokio::test] async fn test_spooling_config() { - let config = ClientSpoolingProtocolConfig { + let config = ClientProtocolConfig::Spooling(ClientSpoolingProtocolConfig { location: "s3://my-bucket/spooling".to_string(), filesystem: SpoolingFileSystemConfig::S3(S3SpoolingConfig { connection: @@ -265,9 +274,9 @@ mod tests { upload_part_size: None, }), config_overrides: HashMap::new(), - }; + }); - let resolved_spooling_config = ResolvedSpoolingProtocolConfig::from_config( + let resolved_spooling_config = ResolvedClientProtocolConfig::from_config( &config, None, // No client, so no external resolution "default", ) @@ -292,7 +301,7 @@ mod tests { #[tokio::test] async fn test_spooling_config_overrides() { - let config = ClientSpoolingProtocolConfig { + let config = ClientProtocolConfig::Spooling(ClientSpoolingProtocolConfig { location: "s3://my-bucket/spooling".to_string(), filesystem: SpoolingFileSystemConfig::S3(S3SpoolingConfig { connection: @@ -308,9 +317,9 @@ mod tests { "protocol.spooling.retrieval-mode".to_string(), "STORAGE".to_string(), )]), - }; + }); - let resolved_spooling_config = ResolvedSpoolingProtocolConfig::from_config( + let resolved_spooling_config = ResolvedClientProtocolConfig::from_config( &config, None, // No client, so no external resolution "default", ) diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index ffa1f2c4..e3cd0aa9 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -305,7 +305,7 @@ pub mod versioned { /// Client spooling protocol configuration. #[serde(skip_serializing_if = "Option::is_none")] - pub client_spooling_protocol: Option, + pub client_protocol: Option, /// Name of the Vector aggregator [discovery ConfigMap](DOCS_BASE_URL_PLACEHOLDER/concepts/service_discovery). /// It must contain the key `ADDRESS` with the address of the Vector aggregator. diff --git a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 index 84404a5d..6ca00c16 100644 --- a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 +++ b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 @@ -16,13 +16,13 @@ spec: catalogLabelSelector: matchLabels: trino: trino - clientSpoolingProtocol: - enabled: true - location: "s3://spooling-bucket/trino/" - filesystem: - s3: - connection: - reference: "minio" + clientProtocol: + spooling: + location: "s3://spooling-bucket/trino/" + filesystem: + s3: + connection: + reference: "minio" {% if lookup('env', 'VECTOR_AGGREGATOR') %} vectorAggregatorConfigMapName: vector-aggregator-discovery {% endif %} From fffd64677b8bd264782be5fb2c4e9438b3ee7366 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Mon, 8 Sep 2025 11:49:38 +0200 Subject: [PATCH 15/39] remove clientProtocol.configOverrides field --- deploy/helm/trino-operator/crds/crds.yaml | 6 -- .../src/crd/client_protocol.rs | 60 +------------------ 2 files changed, 1 insertion(+), 65 deletions(-) diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index b6b64b34..37439ab6 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -114,12 +114,6 @@ spec: properties: spooling: properties: - configOverrides: - additionalProperties: - type: string - default: {} - description: The `configOverrides` allow overriding arbitrary client protocol properties. - type: object filesystem: oneOf: - required: diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 8809568b..a27fc074 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, HashMap}; +use std::collections::BTreeMap; /// This module manages the client protocol properties, especially the for spooling. /// Trino documentation is available here: https://trino.io/docs/current/client/client-protocol.html @@ -37,10 +37,6 @@ pub struct ClientSpoolingProtocolConfig { // Spooling filesystem properties. Only S3 is supported. pub filesystem: SpoolingFileSystemConfig, - - /// The `configOverrides` allow overriding arbitrary client protocol properties. - #[serde(default)] - pub config_overrides: HashMap, } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -144,11 +140,6 @@ impl ResolvedClientProtocolConfig { format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), ), ]); - - // Finally, extend the spooling manager properties with any user configuration - resolved_config - .spooling_manager_properties - .extend(spooling_config.config_overrides.clone()); } } @@ -273,7 +264,6 @@ mod tests { max_error_retries: None, upload_part_size: None, }), - config_overrides: HashMap::new(), }); let resolved_spooling_config = ResolvedClientProtocolConfig::from_config( @@ -298,52 +288,4 @@ mod tests { resolved_spooling_config.spooling_manager_properties ); } - - #[tokio::test] - async fn test_spooling_config_overrides() { - let config = ClientProtocolConfig::Spooling(ClientSpoolingProtocolConfig { - location: "s3://my-bucket/spooling".to_string(), - filesystem: SpoolingFileSystemConfig::S3(S3SpoolingConfig { - connection: - stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( - "test-s3-connection".to_string(), - ), - iam_role: None, - external_id: None, - max_error_retries: None, - upload_part_size: None, - }), - config_overrides: HashMap::from([( - "protocol.spooling.retrieval-mode".to_string(), - "STORAGE".to_string(), - )]), - }); - - let resolved_spooling_config = ResolvedClientProtocolConfig::from_config( - &config, None, // No client, so no external resolution - "default", - ) - .await - .unwrap(); - - let expected_props = BTreeMap::from([ - ( - "fs.location".to_string(), - "s3://my-bucket/spooling".to_string(), - ), - ( - "protocol.spooling.retrieval-mode".to_string(), - "STORAGE".to_string(), - ), - ( - "spooling-manager.name".to_string(), - "filesystem".to_string(), - ), - ]); - - assert_eq!( - expected_props, - resolved_spooling_config.spooling_manager_properties - ); - } } From 9ceafd6784379ff6f46cbbd554bad4119275f9d4 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Mon, 8 Sep 2025 16:04:13 +0200 Subject: [PATCH 16/39] handle config overrides for spooling-manager.properties --- rust/operator-binary/src/controller.rs | 115 +++++++++++++++++++++---- 1 file changed, 96 insertions(+), 19 deletions(-) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 50e81c06..88afdbfe 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -867,6 +867,27 @@ fn build_rolegroup_config_map( } } PropertyNameKind::File(file_name) if file_name == JVM_CONFIG => {} + PropertyNameKind::File(file_name) if file_name == SPOOLING_MANAGER_PROPERTIES => { + // Add automatic properties for the spooling protocol + if let Some(spooling_config) = resolved_spooling_config { + dynamic_resolved_config = spooling_config + .spooling_manager_properties + .iter() + .map(|(k, v)| (k.clone(), Some(v.clone()))) + .collect(); + } + + // Override automatic properties with user provided configuration for the spooling protocol + dynamic_resolved_config.extend(transformed_config); + + if !dynamic_resolved_config.is_empty() { + cm_conf_data.insert( + file_name.to_string(), + to_java_properties_string(dynamic_resolved_config.iter()) + .with_context(|_| FailedToWriteJavaPropertiesSnafu)?, + ); + } + } _ => {} } } @@ -889,20 +910,6 @@ fn build_rolegroup_config_map( } } - // 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)?, - ); - } - let jvm_sec_props: BTreeMap> = config .get(&PropertyNameKind::File(JVM_SECURITY_PROPERTIES.to_string())) .cloned() @@ -1435,6 +1442,7 @@ fn validated_product_config( PropertyNameKind::File(LOG_PROPERTIES.to_string()), PropertyNameKind::File(JVM_SECURITY_PROPERTIES.to_string()), PropertyNameKind::File(ACCESS_CONTROL_PROPERTIES.to_string()), + PropertyNameKind::File(SPOOLING_MANAGER_PROPERTIES.to_string()), ]; let coordinator_role = TrinoRole::Coordinator; @@ -1802,6 +1810,7 @@ mod tests { use stackable_operator::commons::networking::DomainName; use super::*; + use crate::crd::client_protocol::ResolvedClientProtocolConfig; #[test] fn test_config_overrides() { @@ -1838,7 +1847,7 @@ mod tests { default: replicas: 1 "#; - let cm = build_config_map(trino_yaml).data.unwrap(); + let cm = build_config_map(trino_yaml, &None, &None).data.unwrap(); let config = cm.get("config.properties").unwrap(); assert!(config.contains("foo=bar")); assert!(config.contains("level=role-group")); @@ -1861,7 +1870,74 @@ mod tests { assert!(cm.contains_key("access-control.properties")); } - fn build_config_map(trino_yaml: &str) -> ConfigMap { + #[test] + fn test_client_protocol_config_overrides() { + let trino_yaml = r#" + apiVersion: trino.stackable.tech/v1alpha1 + kind: TrinoCluster + metadata: + name: simple-trino + spec: + image: + productVersion: "470" + clusterConfig: + catalogLabelSelector: + matchLabels: + trino: simple-trino + coordinators: + configOverrides: + config.properties: + protocol.spooling.enabled: "false" + spooling-manager.properties: + fs.location: s3a://bucket/cluster/spooling + roleGroups: + default: + configOverrides: + config.properties: + protocol.spooling.enabled: "false" + spooling-manager.properties: + fs.location: s3a://bucket/cluster/spooling + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 + "#; + let resolved_spooling_config = Some(ResolvedClientProtocolConfig { + config_properties: BTreeMap::from([ + ("protocol.spooling.enabled".to_string(), "true".to_string()), + ( + "protocol.spooling.shared-secret-key".to_string(), + "test".to_string(), + ), + ]), + spooling_manager_properties: BTreeMap::from([( + "spooling-manager.name".to_string(), + "filesystem".to_string(), + )]), + volume_mounts: vec![], + volumes: vec![], + load_env_from_files: BTreeMap::new(), + init_container_extra_start_commands: vec![], + }); + + let cm = build_config_map(trino_yaml, &None, &resolved_spooling_config) + .data + .unwrap(); + let config = cm.get("config.properties").unwrap(); + assert!(config.contains("protocol.spooling.enabled=false")); + assert!(config.contains("protocol.spooling.shared-secret-key=test")); + + let config = cm.get("spooling-manager.properties").unwrap(); + assert!(config.contains("fs.location=s3a\\://bucket/cluster/spooling")); + assert!(config.contains("spooling-manager.name=filesystem")); + } + + fn build_config_map( + trino_yaml: &str, + resolved_fte_config: &Option, + resolved_spooling_config: &Option, + ) -> ConfigMap { let mut trino: v1alpha1::TrinoCluster = serde_yaml::from_str(trino_yaml).expect("illegal test input"); trino.metadata.namespace = Some("default".to_owned()); @@ -1882,6 +1958,7 @@ mod tests { PropertyNameKind::File(LOG_PROPERTIES.to_string()), PropertyNameKind::File(JVM_SECURITY_PROPERTIES.to_string()), PropertyNameKind::File(ACCESS_CONTROL_PROPERTIES.to_string()), + PropertyNameKind::File(SPOOLING_MANAGER_PROPERTIES.to_string()), ]; let validated_config = validate_all_roles_and_groups_config( // The Trino version is a single number like 396. @@ -1964,8 +2041,8 @@ mod tests { &trino_authentication_config, &trino_opa_config, &cluster_info, - &None, - &None, + resolved_fte_config, + resolved_spooling_config, ) .unwrap() } @@ -2008,7 +2085,7 @@ mod tests { replicas: 1 "#; - let cm = build_config_map(trino_yaml).data.unwrap(); + let cm = build_config_map(trino_yaml, &None, &None).data.unwrap(); let access_control_config = cm.get("access-control.properties").unwrap(); assert!(access_control_config.contains("access-control.name=opa")); From 206b5dffc13901df6b6298aeb28f41aaf8389090 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Mon, 8 Sep 2025 16:14:32 +0200 Subject: [PATCH 17/39] update docs --- .../trino/pages/usage-guide/client-spooling-protocol.adoc | 2 +- docs/modules/trino/pages/usage-guide/configuration.adoc | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc index 22d9594c..7e331084 100644 --- a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc +++ b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc @@ -28,7 +28,7 @@ spec: <2> Configures the filesystem type for spooling. Only S3 is supported currently via the custom resource definition. The operator automatically fills in additional settings required by Trino, such as the `protocol.spooling.shared-secret-key`. -Users can change the contents of the `spooling-manager.properties` file by adding entries to the optional `spec.clusterConfig.clientSpoolingProtocol.config_overrides` map. +To add or replace properties in the generated `spooling-manager.properties` file, use the `configOverrides` property as describe here : xref:usage-guide/configuration.adoc[]. [IMPORTANT] ==== diff --git a/docs/modules/trino/pages/usage-guide/configuration.adoc b/docs/modules/trino/pages/usage-guide/configuration.adoc index 44b58742..d6fe2eac 100644 --- a/docs/modules/trino/pages/usage-guide/configuration.adoc +++ b/docs/modules/trino/pages/usage-guide/configuration.adoc @@ -16,6 +16,7 @@ For a role or role group, at the same level of `config`, you can specify `config * `password-authenticator.properties` * `security.properties` * `exchange-manager.properties` +* `spooling-manager.properties` For a list of possible configuration properties consult the https://trino.io/docs/current/admin/properties.html[Trino Properties Reference]. From 83f8879f3230e757179ccf89d8b77a42b1e9398c 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 18/39] 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, 36 insertions(+), 64 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 88afdbfe..ca287da4 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -896,18 +896,18 @@ 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 { + cm_conf_data.insert( + SPOOLING_MANAGER_PROPERTIES.to_string(), + unsafe_java_properties_string(&spooling_config.spooling_manager_properties), + ); } let jvm_sec_props: BTreeMap> = config @@ -950,6 +950,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( @@ -1207,13 +1214,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 a27fc074..853950c5 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 { @@ -84,10 +81,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, @@ -106,7 +99,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(), }; @@ -185,18 +177,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 a020f5096710caa6c3fd57aa18180143ddf40016 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 19/39] 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 ca287da4..4abc58cd 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -950,6 +950,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 26b5097b91813d05fc4f4cdcf958a4b797050f53 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 20/39] 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 4abc58cd..c83e8b78 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -896,17 +896,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)?, ); } @@ -950,18 +964,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( From b63c9de0466cb3c634a31cf5a377b78d665d023f Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Mon, 8 Sep 2025 16:49:56 +0200 Subject: [PATCH 21/39] fix merge and update test --- rust/operator-binary/src/controller.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index c83e8b78..d323c773 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -910,20 +910,6 @@ fn build_rolegroup_config_map( } } - // 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)?, - ); - } - let jvm_sec_props: BTreeMap> = config .get(&PropertyNameKind::File(JVM_SECURITY_PROPERTIES.to_string())) .cloned() @@ -1925,7 +1911,6 @@ mod tests { )]), volume_mounts: vec![], volumes: vec![], - load_env_from_files: BTreeMap::new(), init_container_extra_start_commands: vec![], }); @@ -1937,6 +1922,9 @@ mod tests { assert!(config.contains("protocol.spooling.shared-secret-key=test")); let config = cm.get("spooling-manager.properties").unwrap(); + println!("------"); + println!("{}", config); + println!("------"); assert!(config.contains("fs.location=s3a\\://bucket/cluster/spooling")); assert!(config.contains("spooling-manager.name=filesystem")); } From efe10030f4873458b2d5c66f4bd292d4acf6c8d9 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 9 Sep 2025 09:56:40 +0200 Subject: [PATCH 22/39] remove test timeout --- tests/templates/kuttl/client-spooling/05-run-tests.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/templates/kuttl/client-spooling/05-run-tests.yaml b/tests/templates/kuttl/client-spooling/05-run-tests.yaml index f2f1cd4a..1e872b38 100644 --- a/tests/templates/kuttl/client-spooling/05-run-tests.yaml +++ b/tests/templates/kuttl/client-spooling/05-run-tests.yaml @@ -4,4 +4,3 @@ kind: TestStep commands: - script: kubectl exec -n $NAMESPACE trino-test-helper -- pip install --no-cache-dir trino==0.336.0 - script: kubectl exec -n $NAMESPACE trino-test-helper -- python /tmp/04_query.py -c trino-coordinator - timeout: 120 From 9e55ce11494f7e1d557f0ea31ebd4a66a5f8be44 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 9 Sep 2025 11:53:11 +0200 Subject: [PATCH 23/39] not all Trino versions support spooling --- .../trino/pages/usage-guide/client-spooling-protocol.adoc | 5 +++++ .../kuttl/client-spooling/02-install-trino.yaml.j2 | 8 ++++---- tests/test-definition.yaml | 2 +- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc index 7e331084..1dda133c 100644 --- a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc +++ b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc @@ -7,6 +7,11 @@ The Client Spooling Protocol in Trino is designed to efficiently handle large re For more details, refer to the link:{trino-docs-spooling-url}[Trino documentation on Client Spooling Protocol {external-link-icon}^]. +[IMPORTANT] +==== +The client spooling protocol was introduced in Trino 466 but it only works reliably starting with Trino 476. +==== + == Configuration The client spooling protocol is disabled by default. diff --git a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 index 6ca00c16..5608e6cb 100644 --- a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 +++ b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 @@ -5,11 +5,11 @@ metadata: name: trino spec: image: -{% if test_scenario['values']['trino'].find(",") > 0 %} - custom: "{{ test_scenario['values']['trino'].split(',')[1] }}" - productVersion: "{{ test_scenario['values']['trino'].split(',')[0] }}" +{% if test_scenario['values']['trino-latest'].find(",") > 0 %} + custom: "{{ test_scenario['values']['trino-latest'].split(',')[1] }}" + productVersion: "{{ test_scenario['values']['trino-latest'].split(',')[0] }}" {% else %} - productVersion: "{{ test_scenario['values']['trino'] }}" + productVersion: "{{ test_scenario['values']['trino-latest'] }}" {% endif %} pullPolicy: IfNotPresent clusterConfig: diff --git a/tests/test-definition.yaml b/tests/test-definition.yaml index ae4c4df7..7067d067 100644 --- a/tests/test-definition.yaml +++ b/tests/test-definition.yaml @@ -117,7 +117,7 @@ tests: - openshift - name: client-spooling dimensions: - - trino + - trino-latest - openshift - name: listener dimensions: From da557fd43e8eef112185e89b6f7d6c546303a576 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Thu, 11 Sep 2025 17:14:42 +0200 Subject: [PATCH 24/39] Apply suggestions from code review Co-authored-by: Sebastian Bernauer --- rust/operator-binary/src/controller.rs | 2 +- tests/test-definition.yaml | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index d323c773..c6e9f50e 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -133,7 +133,7 @@ pub enum Error { #[snafu(display("object defines no namespace"))] ObjectHasNoNamespace, - #[snafu(display("Trino cluster {} has no namespace", name))] + #[snafu(display("Trino cluster {name:?} has no namespace"))] MissingTrinoNamespace { source: crate::crd::Error, name: String, diff --git a/tests/test-definition.yaml b/tests/test-definition.yaml index 7067d067..393d23cd 100644 --- a/tests/test-definition.yaml +++ b/tests/test-definition.yaml @@ -117,6 +117,8 @@ tests: - openshift - name: client-spooling dimensions: + # The client spooling protocol was introduced in Trino 466 but it only works reliably starting with Trino 476. + # Long term we should test all versions - trino-latest - openshift - name: listener From 76c88daa863cc7e17f38ee9bc4006d91d7ea62b2 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Thu, 11 Sep 2025 17:16:27 +0200 Subject: [PATCH 25/39] remove leftovers --- rust/operator-binary/src/controller.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index c6e9f50e..4c663aef 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -1922,9 +1922,7 @@ mod tests { assert!(config.contains("protocol.spooling.shared-secret-key=test")); let config = cm.get("spooling-manager.properties").unwrap(); - println!("------"); - println!("{}", config); - println!("------"); + assert!(config.contains("fs.location=s3a\\://bucket/cluster/spooling")); assert!(config.contains("spooling-manager.name=filesystem")); } From d76c4a180cb05fe7c8bf1253256c6c8528ea9022 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Fri, 12 Sep 2025 17:06:01 +0200 Subject: [PATCH 26/39] remove `clusterConfig.faultTolerantExecution.configOverrides` property --- CHANGELOG.md | 4 ++ deploy/helm/trino-operator/crds/crds.yaml | 12 ---- rust/operator-binary/src/controller.rs | 39 ++++++----- .../src/crd/fault_tolerant_execution.rs | 66 +------------------ 4 files changed, 29 insertions(+), 92 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccc48559..393ce9e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ All notable changes to this project will be documented in this file. - Support for fault-tolerant execution ([#779]). - Support for the client spooling protocol ([#793]). +### Removed + +- Cluster wide FTE specific `configOverrides` has been removed in favor of the "classic" role/group overrides ([#793]). + [#779]: https://github.com/stackabletech/trino-operator/pull/779 [#793]: https://github.com/stackabletech/trino-operator/pull/793 diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index 37439ab6..32d28959 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -299,12 +299,6 @@ spec: - required: - local properties: - configOverrides: - additionalProperties: - type: string - default: {} - description: The `configOverrides` allow overriding arbitrary exchange manager properties. - type: object encryptionEnabled: description: Whether to enable encryption of spooling data. nullable: true @@ -561,12 +555,6 @@ spec: - required: - local properties: - configOverrides: - additionalProperties: - type: string - default: {} - description: The `configOverrides` allow overriding arbitrary exchange manager properties. - type: object encryptionEnabled: description: Whether to enable encryption of spooling data. nullable: true diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 4c663aef..6bc73941 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -888,28 +888,33 @@ fn build_rolegroup_config_map( ); } } + PropertyNameKind::File(file_name) if file_name == EXCHANGE_MANAGER_PROPERTIES => { + // Add exchange manager properties from resolved fault tolerant execution configuration + if let Some(resolved_fte) = resolved_fte_config { + dynamic_resolved_config = resolved_fte + .exchange_manager_properties + .iter() + .map(|(k, v)| (k.clone(), Some(v.clone()))) + .collect(); + } + + // Override automatic properties with user provided configuration for the spooling protocol + dynamic_resolved_config.extend(transformed_config); + + if !dynamic_resolved_config.is_empty() { + cm_conf_data.insert( + file_name.to_string(), + to_java_properties_string(dynamic_resolved_config.iter()) + .with_context(|_| FailedToWriteJavaPropertiesSnafu)?, + ); + } + } _ => {} } } cm_conf_data.insert(JVM_CONFIG.to_string(), jvm_config.to_string()); - // 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)?, - ); - } - } - let jvm_sec_props: BTreeMap> = config .get(&PropertyNameKind::File(JVM_SECURITY_PROPERTIES.to_string())) .cloned() @@ -1437,6 +1442,7 @@ fn validated_product_config( PropertyNameKind::File(JVM_SECURITY_PROPERTIES.to_string()), PropertyNameKind::File(ACCESS_CONTROL_PROPERTIES.to_string()), PropertyNameKind::File(SPOOLING_MANAGER_PROPERTIES.to_string()), + PropertyNameKind::File(EXCHANGE_MANAGER_PROPERTIES.to_string()), ]; let coordinator_role = TrinoRole::Coordinator; @@ -1953,6 +1959,7 @@ mod tests { PropertyNameKind::File(JVM_SECURITY_PROPERTIES.to_string()), PropertyNameKind::File(ACCESS_CONTROL_PROPERTIES.to_string()), PropertyNameKind::File(SPOOLING_MANAGER_PROPERTIES.to_string()), + PropertyNameKind::File(EXCHANGE_MANAGER_PROPERTIES.to_string()), ]; let validated_config = validate_all_roles_and_groups_config( // The Trino version is a single number like 396. diff --git a/rust/operator-binary/src/crd/fault_tolerant_execution.rs b/rust/operator-binary/src/crd/fault_tolerant_execution.rs index bffcde6a..0ae63a16 100644 --- a/rust/operator-binary/src/crd/fault_tolerant_execution.rs +++ b/rust/operator-binary/src/crd/fault_tolerant_execution.rs @@ -5,7 +5,7 @@ //! //! Based on the Trino documentation: -use std::collections::{BTreeMap, HashMap}; +use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; use snafu::Snafu; @@ -124,10 +124,6 @@ pub struct ExchangeManagerConfig { /// Backend-specific configuration. #[serde(flatten)] pub backend: ExchangeManagerBackend, - - /// The `configOverrides` allow overriding arbitrary exchange manager properties. - #[serde(default)] - pub config_overrides: HashMap, } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -440,8 +436,6 @@ impl ResolvedFaultTolerantExecutionConfig { ); } } - - exchange_manager_properties.extend(exchange_config.config_overrides.clone()); } let mut resolved_config = Self { @@ -562,6 +556,7 @@ impl ResolvedFaultTolerantExecutionConfig { #[cfg(test)] mod tests { + use super::*; #[tokio::test] @@ -625,7 +620,6 @@ mod tests { backend: ExchangeManagerBackend::Local(LocalExchangeConfig { base_directories: vec!["/tmp/exchange".to_string()], }), - config_overrides: HashMap::new(), }), }); @@ -705,7 +699,6 @@ mod tests { max_error_retries: Some(5), upload_part_size: Some(Quantity("10Mi".to_string())), }), - config_overrides: std::collections::HashMap::new(), }, }); @@ -787,59 +780,4 @@ mod tests { Some(&"8".to_string()) ); } - - #[tokio::test] - async fn test_exchange_manager_config_overrides() { - let mut config_overrides = HashMap::new(); - config_overrides.insert("custom.property".to_string(), "custom-value".to_string()); - config_overrides.insert( - "exchange.s3.upload.part-size".to_string(), - "overridden-value".to_string(), - ); - - let config = FaultTolerantExecutionConfig::Task(TaskRetryConfig { - retry_attempts_per_task: Some(2), - retry_initial_delay: None, - retry_max_delay: None, - retry_delay_scale_factor: None, - exchange_deduplication_buffer_size: None, - exchange_manager: ExchangeManagerConfig { - encryption_enabled: None, - sink_buffer_pool_min_size: None, - sink_buffers_per_partition: None, - sink_max_file_size: None, - source_concurrent_readers: None, - backend: ExchangeManagerBackend::S3(S3ExchangeConfig { - base_directories: vec!["s3://my-bucket/exchange".to_string()], - connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( - "test-s3-connection".to_string() - ), - iam_role: None, - external_id: None, - max_error_retries: None, - upload_part_size: Some(Quantity("10Mi".to_string())), - }), - config_overrides, - }, - }); - - let fte_config = - ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") - .await - .unwrap(); - - assert_eq!( - fte_config - .exchange_manager_properties - .get("custom.property"), - Some(&"custom-value".to_string()) - ); - - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.s3.upload.part-size"), - Some(&"overridden-value".to_string()) - ); - } } From 0ba2fede6bf28f1d2f84cdcec71a0f3cc8c92b9f Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Mon, 15 Sep 2025 10:16:09 +0200 Subject: [PATCH 27/39] amend FTE changelog --- CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 393ce9e9..0815a616 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,9 @@ All notable changes to this project will be documented in this file. ### Added - Support for fault-tolerant execution ([#779]). + Remove `spec.clusterConfig.faultTolerantExecution.configOverrides` and use "classic" role/group overrides ([#793]). - Support for the client spooling protocol ([#793]). -### Removed - -- Cluster wide FTE specific `configOverrides` has been removed in favor of the "classic" role/group overrides ([#793]). - [#779]: https://github.com/stackabletech/trino-operator/pull/779 [#793]: https://github.com/stackabletech/trino-operator/pull/793 From 4d32b6f282988f7ad2ea6406de59efc5931b8ffb Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Mon, 15 Sep 2025 12:52:01 +0200 Subject: [PATCH 28/39] client_protocol: refactor s3 config into crd::s3 and config::s3 to make it reusable --- rust/operator-binary/src/command.rs | 3 +- .../src/config/client_protocol.rs | 167 ++++++++++++ rust/operator-binary/src/config/mod.rs | 2 + rust/operator-binary/src/config/s3.rs | 128 +++++++++ rust/operator-binary/src/controller.rs | 5 +- .../src/crd/client_protocol.rs | 256 +----------------- rust/operator-binary/src/crd/mod.rs | 1 + rust/operator-binary/src/crd/s3.rs | 29 ++ 8 files changed, 335 insertions(+), 256 deletions(-) create mode 100644 rust/operator-binary/src/config/client_protocol.rs create mode 100644 rust/operator-binary/src/config/s3.rs create mode 100644 rust/operator-binary/src/crd/s3.rs diff --git a/rust/operator-binary/src/command.rs b/rust/operator-binary/src/command.rs index 8407cd92..ec655aa2 100644 --- a/rust/operator-binary/src/command.rs +++ b/rust/operator-binary/src/command.rs @@ -9,13 +9,14 @@ use stackable_operator::{ use crate::{ authentication::TrinoAuthenticationConfig, catalog::config::CatalogConfig, + config::client_protocol, controller::{STACKABLE_LOG_CONFIG_DIR, STACKABLE_LOG_DIR}, crd::{ 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, + SYSTEM_TRUST_STORE, SYSTEM_TRUST_STORE_PASSWORD, TrinoRole, fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, v1alpha1, }, }; diff --git a/rust/operator-binary/src/config/client_protocol.rs b/rust/operator-binary/src/config/client_protocol.rs new file mode 100644 index 00000000..9702fc85 --- /dev/null +++ b/rust/operator-binary/src/config/client_protocol.rs @@ -0,0 +1,167 @@ +// Consolidate Trino S3 properties in a single reusable struct. + +use std::collections::BTreeMap; + +use snafu::{self, ResultExt, Snafu}; +use stackable_operator::{ + client::Client, + k8s_openapi::api::core::v1::{Volume, VolumeMount}, +}; + +use crate::{ + config, + crd::{ + ENV_SPOOLING_SECRET, + client_protocol::{ClientProtocolConfig, SpoolingFileSystemConfig}, + }, +}; + +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display("Failed to resolve S3 connection"))] + ResolveS3Connection { source: config::s3::Error }, + + #[snafu(display("trino does not support disabling the TLS verification of S3 servers"))] + S3TlsNoVerificationNotSupported, + + #[snafu(display("failed to convert data size for [{field}] to bytes"))] + QuantityConversion { + source: stackable_operator::memory::Error, + field: &'static str, + }, +} + +pub struct ResolvedClientProtocolConfig { + /// Properties to add to config.properties + pub config_properties: BTreeMap, + + // Properties for spooling-manager.properties + pub spooling_manager_properties: BTreeMap, + + /// Volumes required for the configuration (e.g., for S3 credentials) + pub volumes: Vec, + + /// Volume mounts required for the configuration + pub volume_mounts: Vec, + + /// 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, +} + +impl ResolvedClientProtocolConfig { + /// Resolve S3 connection properties from Kubernetes resources + /// and prepare spooling filesystem configuration. + pub async fn from_config( + config: &ClientProtocolConfig, + client: Option<&Client>, + namespace: &str, + ) -> Result { + let mut resolved_config = Self { + config_properties: BTreeMap::new(), + spooling_manager_properties: BTreeMap::new(), + volumes: Vec::new(), + volume_mounts: Vec::new(), + init_container_extra_start_commands: Vec::new(), + }; + + match config { + ClientProtocolConfig::Spooling(spooling_config) => { + // Resolve external resources if Kubernetes client is available + // This should always be the case, except for when this function is called during unit tests + if let Some(client) = client { + match &spooling_config.filesystem { + SpoolingFileSystemConfig::S3(s3_config) => { + let resolved_s3_config = config::s3::ResolvedS3Config::from_config( + s3_config, client, namespace, + ) + .await + .context(ResolveS3ConnectionSnafu)?; + + // Enable S3 filesystem after successful resolution + resolved_config + .spooling_manager_properties + .insert("fs.s3.enabled".to_string(), "true".to_string()); + + // Copy the S3 configuration over + resolved_config + .spooling_manager_properties + .extend(resolved_s3_config.properties); + resolved_config.volumes.extend(resolved_s3_config.volumes); + resolved_config + .volume_mounts + .extend(resolved_s3_config.volume_mounts); + resolved_config + .init_container_extra_start_commands + .extend(resolved_s3_config.init_container_extra_start_commands); + } + } + } + + resolved_config.spooling_manager_properties.extend([ + ("fs.location".to_string(), spooling_config.location.clone()), + ( + "spooling-manager.name".to_string(), + "filesystem".to_string(), + ), + ]); + + // Enable spooling protocol + resolved_config.config_properties.extend([ + ("protocol.spooling.enabled".to_string(), "true".to_string()), + ( + "protocol.spooling.shared-secret-key".to_string(), + format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), + ), + ]); + } + } + + Ok(resolved_config) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::crd::{client_protocol::ClientSpoolingProtocolConfig, s3::S3Config}; + + #[tokio::test] + async fn test_spooling_config() { + let config = ClientProtocolConfig::Spooling(ClientSpoolingProtocolConfig { + location: "s3://my-bucket/spooling".to_string(), + filesystem: SpoolingFileSystemConfig::S3(S3Config { + connection: + stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( + "test-s3-connection".to_string(), + ), + iam_role: None, + external_id: None, + max_error_retries: None, + upload_part_size: None, + }), + }); + + let resolved_spooling_config = ResolvedClientProtocolConfig::from_config( + &config, None, // No client, so no external resolution + "default", + ) + .await + .unwrap(); + + let expected_props = BTreeMap::from([ + ( + "fs.location".to_string(), + "s3://my-bucket/spooling".to_string(), + ), + ( + "spooling-manager.name".to_string(), + "filesystem".to_string(), + ), + ]); + assert_eq!( + expected_props, + resolved_spooling_config.spooling_manager_properties + ); + } +} diff --git a/rust/operator-binary/src/config/mod.rs b/rust/operator-binary/src/config/mod.rs index 271c6d99..26d1557f 100644 --- a/rust/operator-binary/src/config/mod.rs +++ b/rust/operator-binary/src/config/mod.rs @@ -1 +1,3 @@ +pub mod client_protocol; pub mod jvm; +pub mod s3; diff --git a/rust/operator-binary/src/config/s3.rs b/rust/operator-binary/src/config/s3.rs new file mode 100644 index 00000000..97df8c5f --- /dev/null +++ b/rust/operator-binary/src/config/s3.rs @@ -0,0 +1,128 @@ +use std::collections::BTreeMap; + +use snafu::{ResultExt, Snafu}; +use stackable_operator::{ + client::Client, + commons::tls_verification::{CaCert, TlsServerVerification, TlsVerification}, + crd::s3, + k8s_openapi::api::core::v1::{Volume, VolumeMount}, +}; + +use crate::{ + command, + crd::{STACKABLE_CLIENT_TLS_DIR, s3 as trino_s3}, +}; + +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display("Failed to resolve S3 connection"))] + S3Connection { + source: s3::v1alpha1::ConnectionError, + }, + + #[snafu(display("trino does not support disabling the TLS verification of S3 servers"))] + S3TlsNoVerificationNotSupported, + + #[snafu(display("failed to convert data size for [{field}] to bytes"))] + QuantityConversion { + source: stackable_operator::memory::Error, + field: &'static str, + }, +} + +pub struct ResolvedS3Config { + /// Properties to add to config.properties + pub properties: BTreeMap, + + /// Volumes required for the configuration (e.g., for S3 credentials) + pub volumes: Vec, + + /// Volume mounts required for the configuration + pub volume_mounts: Vec, + + /// 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, +} + +impl ResolvedS3Config { + /// Resolve S3 connection properties from Kubernetes resources + /// and prepare spooling filesystem configuration. + pub async fn from_config( + config: &trino_s3::S3Config, + client: &Client, + namespace: &str, + ) -> Result { + let mut resolved_config = Self { + properties: BTreeMap::new(), + volumes: Vec::new(), + volume_mounts: Vec::new(), + init_container_extra_start_commands: Vec::new(), + }; + + let s3_connection = config + .connection + .clone() + .resolve(client, namespace) + .await + .context(S3ConnectionSnafu)?; + + let (volumes, mounts) = s3_connection + .volumes_and_mounts() + .context(S3ConnectionSnafu)?; + resolved_config.volumes.extend(volumes); + resolved_config.volume_mounts.extend(mounts); + + resolved_config + .properties + .insert("s3.region".to_string(), s3_connection.region.name.clone()); + resolved_config.properties.insert( + "s3.endpoint".to_string(), + s3_connection + .endpoint() + .context(S3ConnectionSnafu)? + .to_string(), + ); + resolved_config.properties.insert( + "s3.path-style-access".to_string(), + (s3_connection.access_style == s3::v1alpha1::S3AccessStyle::Path).to_string(), + ); + + if let Some((access_key_path, secret_key_path)) = s3_connection.credentials_mount_paths() { + resolved_config.properties.extend([ + ( + "s3.aws-access-key".to_string(), + format!("${{file:UTF-8:{access_key_path}}}"), + ), + ( + "s3.aws-secret-key".to_string(), + format!("${{file:UTF-8:{secret_key_path}}}"), + ), + ]); + } + + if let Some(tls) = s3_connection.tls.tls.as_ref() { + match &tls.verification { + TlsVerification::None {} => return S3TlsNoVerificationNotSupportedSnafu.fail(), + TlsVerification::Server(TlsServerVerification { + ca_cert: CaCert::WebPki {}, + }) => {} + TlsVerification::Server(TlsServerVerification { + ca_cert: CaCert::SecretClass(_), + }) => { + if let Some(ca_cert) = s3_connection.tls.tls_ca_cert_mount_path() { + resolved_config.init_container_extra_start_commands.extend( + command::add_cert_to_truststore( + &ca_cert, + STACKABLE_CLIENT_TLS_DIR, + "resolved-s3-ca-cert", + ), + ); + } + } + } + } + + Ok(resolved_config) + } +} diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 6bc73941..70686cd5 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -79,6 +79,7 @@ use crate::{ authorization::opa::TrinoOpaConfig, catalog::{FromTrinoCatalogError, config::CatalogConfig}, command, config, + config::client_protocol, crd::{ ACCESS_CONTROL_PROPERTIES, APP_NAME, CONFIG_DIR_NAME, CONFIG_PROPERTIES, Container, DISCOVERY_URI, ENV_INTERNAL_SECRET, ENV_SPOOLING_SECRET, EXCHANGE_MANAGER_PROPERTIES, @@ -88,7 +89,7 @@ use crate::{ STACKABLE_CLIENT_TLS_DIR, STACKABLE_INTERNAL_TLS_DIR, STACKABLE_MOUNT_INTERNAL_TLS_DIR, STACKABLE_MOUNT_SERVER_TLS_DIR, STACKABLE_SERVER_TLS_DIR, TrinoRole, authentication::resolve_authentication_classes, - catalog, client_protocol, + catalog, discovery::{TrinoDiscovery, TrinoDiscoveryProtocol, TrinoPodRef}, fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, rolegroup_headless_service_name, v1alpha1, @@ -1810,7 +1811,7 @@ mod tests { use stackable_operator::commons::networking::DomainName; use super::*; - use crate::crd::client_protocol::ResolvedClientProtocolConfig; + use crate::config::client_protocol::ResolvedClientProtocolConfig; #[test] fn test_config_overrides() { diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 853950c5..62490a4f 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -1,24 +1,9 @@ -use std::collections::BTreeMap; - /// This module manages the client protocol properties, especially the for spooling. /// Trino documentation is available here: https://trino.io/docs/current/client/client-protocol.html use serde::{Deserialize, Serialize}; -use snafu::Snafu; -use stackable_operator::{ - client::Client, - commons::tls_verification::{CaCert, TlsServerVerification, TlsVerification}, - crd::s3, - k8s_openapi::{ - api::core::v1::{Volume, VolumeMount}, - apimachinery::pkg::api::resource::Quantity, - }, - schemars::{self, JsonSchema}, -}; +use stackable_operator::schemars::{self, JsonSchema}; -use crate::{ - command, - crd::{ENV_SPOOLING_SECRET, STACKABLE_CLIENT_TLS_DIR}, -}; +use crate::crd::s3::S3Config; #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] @@ -39,240 +24,5 @@ pub struct ClientSpoolingProtocolConfig { #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub enum SpoolingFileSystemConfig { - S3(S3SpoolingConfig), -} -// TODO: this is exactly the same as fault_tolerant_execution::S3ExchangeConfig -// but without the base_directory property. -// Consolidate Trino S3 properties in a single reusable struct. -#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] -pub struct S3SpoolingConfig { - /// S3 connection configuration. - /// Learn more about S3 configuration in the [S3 concept docs](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). - pub connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference, - - /// IAM role to assume for S3 access. - #[serde(skip_serializing_if = "Option::is_none")] - pub iam_role: Option, - - /// External ID for the IAM role trust policy. - #[serde(skip_serializing_if = "Option::is_none")] - pub external_id: Option, - - /// Maximum number of times the S3 client should retry a request. - #[serde(skip_serializing_if = "Option::is_none")] - pub max_error_retries: Option, - - /// Part data size for S3 multi-part upload. - #[serde(skip_serializing_if = "Option::is_none")] - pub upload_part_size: Option, -} - -pub struct ResolvedClientProtocolConfig { - /// Properties to add to config.properties - pub config_properties: BTreeMap, - - // Properties for spooling-manager.properties - pub spooling_manager_properties: BTreeMap, - - /// Volumes required for the configuration (e.g., for S3 credentials) - pub volumes: Vec, - - /// Volume mounts required for the configuration - pub volume_mounts: Vec, - - /// 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, -} - -impl ResolvedClientProtocolConfig { - /// Resolve S3 connection properties from Kubernetes resources - /// and prepare spooling filesystem configuration. - pub async fn from_config( - config: &ClientProtocolConfig, - client: Option<&Client>, - namespace: &str, - ) -> Result { - let mut resolved_config = Self { - config_properties: BTreeMap::new(), - spooling_manager_properties: BTreeMap::new(), - volumes: Vec::new(), - volume_mounts: Vec::new(), - init_container_extra_start_commands: Vec::new(), - }; - - match config { - ClientProtocolConfig::Spooling(spooling_config) => { - // Resolve external resources if Kubernetes client is available - // This should always be the case, except for when this function is called during unit tests - if let Some(client) = client { - match &spooling_config.filesystem { - SpoolingFileSystemConfig::S3(s3_config) => { - resolved_config - .resolve_s3_backend(s3_config, client, namespace) - .await?; - } - } - } - - resolved_config.spooling_manager_properties.extend([ - ("fs.location".to_string(), spooling_config.location.clone()), - ( - "spooling-manager.name".to_string(), - "filesystem".to_string(), - ), - ]); - - // Enable spooling protocol - resolved_config.config_properties.extend([ - ("protocol.spooling.enabled".to_string(), "true".to_string()), - ( - "protocol.spooling.shared-secret-key".to_string(), - format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), - ), - ]); - } - } - - Ok(resolved_config) - } - - async fn resolve_s3_backend( - &mut self, - s3_config: &S3SpoolingConfig, - client: &Client, - namespace: &str, - ) -> Result<(), Error> { - use snafu::ResultExt; - - let s3_connection = s3_config - .connection - .clone() - .resolve(client, namespace) - .await - .context(S3ConnectionSnafu)?; - - let (volumes, mounts) = s3_connection - .volumes_and_mounts() - .context(S3ConnectionSnafu)?; - self.volumes.extend(volumes); - self.volume_mounts.extend(mounts); - - self.spooling_manager_properties - .insert("s3.region".to_string(), s3_connection.region.name.clone()); - self.spooling_manager_properties.insert( - "s3.endpoint".to_string(), - s3_connection - .endpoint() - .context(S3ConnectionSnafu)? - .to_string(), - ); - self.spooling_manager_properties.insert( - "s3.path-style-access".to_string(), - (s3_connection.access_style == s3::v1alpha1::S3AccessStyle::Path).to_string(), - ); - - if let Some((access_key_path, secret_key_path)) = s3_connection.credentials_mount_paths() { - self.spooling_manager_properties.extend([ - ( - "s3.aws-access-key".to_string(), - format!("${{file:UTF-8:{access_key_path}}}"), - ), - ( - "s3.aws-secret-key".to_string(), - format!("${{file:UTF-8:{secret_key_path}}}"), - ), - ]); - } - - if let Some(tls) = s3_connection.tls.tls.as_ref() { - match &tls.verification { - TlsVerification::None {} => return S3TlsNoVerificationNotSupportedSnafu.fail(), - TlsVerification::Server(TlsServerVerification { - ca_cert: CaCert::WebPki {}, - }) => {} - TlsVerification::Server(TlsServerVerification { - ca_cert: CaCert::SecretClass(_), - }) => { - if let Some(ca_cert) = s3_connection.tls.tls_ca_cert_mount_path() { - self.init_container_extra_start_commands.extend( - command::add_cert_to_truststore( - &ca_cert, - STACKABLE_CLIENT_TLS_DIR, - "spooling-s3-ca-cert", - ), - ); - } - } - } - } - - // Enable S3 filesystem after successful resolution - self.spooling_manager_properties - .insert("fs.s3.enabled".to_string(), "true".to_string()); - - Ok(()) - } -} - -#[derive(Snafu, Debug)] -pub enum Error { - #[snafu(display("Failed to resolve S3 connection"))] - S3Connection { - source: s3::v1alpha1::ConnectionError, - }, - - #[snafu(display("trino does not support disabling the TLS verification of S3 servers"))] - S3TlsNoVerificationNotSupported, - - #[snafu(display("failed to convert data size for [{field}] to bytes"))] - QuantityConversion { - source: stackable_operator::memory::Error, - field: &'static str, - }, -} - -#[cfg(test)] -mod tests { - use super::*; - - #[tokio::test] - async fn test_spooling_config() { - let config = ClientProtocolConfig::Spooling(ClientSpoolingProtocolConfig { - location: "s3://my-bucket/spooling".to_string(), - filesystem: SpoolingFileSystemConfig::S3(S3SpoolingConfig { - connection: - stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( - "test-s3-connection".to_string(), - ), - iam_role: None, - external_id: None, - max_error_retries: None, - upload_part_size: None, - }), - }); - - let resolved_spooling_config = ResolvedClientProtocolConfig::from_config( - &config, None, // No client, so no external resolution - "default", - ) - .await - .unwrap(); - - let expected_props = BTreeMap::from([ - ( - "fs.location".to_string(), - "s3://my-bucket/spooling".to_string(), - ), - ( - "spooling-manager.name".to_string(), - "filesystem".to_string(), - ), - ]); - assert_eq!( - expected_props, - resolved_spooling_config.spooling_manager_properties - ); - } + S3(S3Config), } diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index e3cd0aa9..f1cafc83 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -4,6 +4,7 @@ pub mod catalog; pub mod client_protocol; pub mod discovery; pub mod fault_tolerant_execution; +pub mod s3; use std::{collections::BTreeMap, ops::Div, str::FromStr}; diff --git a/rust/operator-binary/src/crd/s3.rs b/rust/operator-binary/src/crd/s3.rs new file mode 100644 index 00000000..db414e42 --- /dev/null +++ b/rust/operator-binary/src/crd/s3.rs @@ -0,0 +1,29 @@ +use serde::{Deserialize, Serialize}; +use stackable_operator::{ + k8s_openapi::apimachinery::pkg::api::resource::Quantity, + schemars::{self, JsonSchema}, +}; + +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct S3Config { + /// S3 connection configuration. + /// Learn more about S3 configuration in the [S3 concept docs](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). + pub connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference, + + /// IAM role to assume for S3 access. + #[serde(skip_serializing_if = "Option::is_none")] + pub iam_role: Option, + + /// External ID for the IAM role trust policy. + #[serde(skip_serializing_if = "Option::is_none")] + pub external_id: Option, + + /// Maximum number of times the S3 client should retry a request. + #[serde(skip_serializing_if = "Option::is_none")] + pub max_error_retries: Option, + + /// Part data size for S3 multi-part upload. + #[serde(skip_serializing_if = "Option::is_none")] + pub upload_part_size: Option, +} From d6943fcca7fc6f08ad08a4fbe20eb555db948455 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Mon, 15 Sep 2025 15:58:00 +0200 Subject: [PATCH 29/39] fte: refactor to reuse crd::s3 anc move resolved struct out of the crd module --- deploy/helm/trino-operator/crds/crds.yaml | 10 - rust/operator-binary/src/command.rs | 7 +- .../src/config/client_protocol.rs | 2 - .../src/config/fault_tolerant_execution.rs | 572 ++++++++++++++++ rust/operator-binary/src/config/mod.rs | 1 + rust/operator-binary/src/controller.rs | 25 +- .../src/crd/fault_tolerant_execution.rs | 625 +----------------- rust/operator-binary/src/crd/s3.rs | 13 +- 8 files changed, 599 insertions(+), 656 deletions(-) create mode 100644 rust/operator-binary/src/config/fault_tolerant_execution.rs diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index 32d28959..42bd0bc3 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -251,16 +251,6 @@ spec: description: IAM role to assume for S3 access. nullable: true type: string - maxErrorRetries: - description: Maximum number of times the S3 client should retry a request. - format: uint32 - minimum: 0.0 - nullable: true - type: integer - uploadPartSize: - description: Part data size for S3 multi-part upload. - nullable: true - type: string required: - connection type: object diff --git a/rust/operator-binary/src/command.rs b/rust/operator-binary/src/command.rs index ec655aa2..29c5769a 100644 --- a/rust/operator-binary/src/command.rs +++ b/rust/operator-binary/src/command.rs @@ -9,15 +9,14 @@ use stackable_operator::{ use crate::{ authentication::TrinoAuthenticationConfig, catalog::config::CatalogConfig, - config::client_protocol, + config::{client_protocol, fault_tolerant_execution}, controller::{STACKABLE_LOG_CONFIG_DIR, STACKABLE_LOG_DIR}, crd::{ 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, - fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, v1alpha1, + SYSTEM_TRUST_STORE, SYSTEM_TRUST_STORE_PASSWORD, TrinoRole, v1alpha1, }, }; @@ -25,7 +24,7 @@ pub fn container_prepare_args( trino: &v1alpha1::TrinoCluster, catalogs: &[CatalogConfig], merged_config: &v1alpha1::TrinoConfig, - resolved_fte_config: &Option, + resolved_fte_config: &Option, resolved_spooling_config: &Option, ) -> Vec { let mut args = vec![]; diff --git a/rust/operator-binary/src/config/client_protocol.rs b/rust/operator-binary/src/config/client_protocol.rs index 9702fc85..d8618f51 100644 --- a/rust/operator-binary/src/config/client_protocol.rs +++ b/rust/operator-binary/src/config/client_protocol.rs @@ -137,8 +137,6 @@ mod tests { ), iam_role: None, external_id: None, - max_error_retries: None, - upload_part_size: None, }), }); diff --git a/rust/operator-binary/src/config/fault_tolerant_execution.rs b/rust/operator-binary/src/config/fault_tolerant_execution.rs new file mode 100644 index 00000000..b2497f56 --- /dev/null +++ b/rust/operator-binary/src/config/fault_tolerant_execution.rs @@ -0,0 +1,572 @@ +use std::collections::BTreeMap; + +use snafu::{ResultExt, Snafu}; +use stackable_operator::{ + builder::pod::volume::{VolumeBuilder, VolumeMountBuilder}, + client::Client, + crd::s3, + k8s_openapi::{ + api::core::v1::{Volume, VolumeMount}, + apimachinery::pkg::api::resource::Quantity, + }, +}; + +use crate::{ + config, + crd::{ + CONFIG_DIR_NAME, + fault_tolerant_execution::{ + ExchangeManagerBackend, FaultTolerantExecutionConfig, HdfsExchangeConfig, + }, + }, +}; + +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display("Failed to resolve S3 connection"))] + S3Connection { + source: s3::v1alpha1::ConnectionError, + }, + + #[snafu(display("Failed to resolve S3 connection"))] + ResolveS3Connection { source: config::s3::Error }, + + #[snafu(display("trino does not support disabling the TLS verification of S3 servers"))] + S3TlsNoVerificationNotSupported, + + #[snafu(display("failed to convert data size for [{field}] to bytes"))] + QuantityConversion { + source: stackable_operator::memory::Error, + field: &'static str, + }, +} + +/// Fault tolerant execution configuration with external resources resolved +pub struct ResolvedFaultTolerantExecutionConfig { + /// Properties to add to config.properties + pub config_properties: BTreeMap, + + /// Properties to add to exchange-manager.properties (if needed) + pub exchange_manager_properties: BTreeMap, + + /// Volumes required for the configuration (e.g., for S3 credentials) + pub volumes: Vec, + + /// Volume mounts required for the configuration + pub volume_mounts: Vec, + + /// Additional commands that need to be executed before starting Trino + pub init_container_extra_start_commands: Vec, +} + +impl ResolvedFaultTolerantExecutionConfig { + /// Helper function to insert optional values into properties map + fn insert_if_present( + properties: &mut BTreeMap, + key: &str, + value: Option, + ) { + if let Some(v) = value { + properties.insert(key.to_string(), v.to_string()); + } + } + + /// Helper function to insert optional Quantity values after converting to Trino bytes string + fn insert_quantity_if_present( + properties: &mut BTreeMap, + key: &'static str, + quantity: Option<&Quantity>, + ) -> Result<(), Error> { + if let Some(q) = quantity { + use snafu::ResultExt; + let v = crate::crd::quantity_to_trino_bytes(q) + .context(QuantityConversionSnafu { field: key })?; + properties.insert(key.to_string(), v); + } + Ok(()) + } + + /// Create a resolved fault tolerant execution configuration from the cluster config + pub async fn from_config( + config: &FaultTolerantExecutionConfig, + client: Option<&Client>, + namespace: &str, + ) -> Result { + let mut config_properties = BTreeMap::new(); + + // Handle different retry policies and their configurations + let (retry_policy_str, exchange_manager_opt) = match config { + FaultTolerantExecutionConfig::Query(query_config) => { + // Set query-specific properties + Self::insert_if_present( + &mut config_properties, + "query-retry-attempts", + query_config.retry_attempts, + ); + Self::insert_if_present( + &mut config_properties, + "retry-initial-delay", + query_config + .retry_initial_delay + .as_ref() + .map(|d| format!("{}s", d.as_secs())), + ); + Self::insert_if_present( + &mut config_properties, + "retry-max-delay", + query_config + .retry_max_delay + .as_ref() + .map(|d| format!("{}s", d.as_secs())), + ); + Self::insert_if_present( + &mut config_properties, + "retry-delay-scale-factor", + query_config.retry_delay_scale_factor.as_ref(), + ); + Self::insert_quantity_if_present( + &mut config_properties, + "exchange.deduplication-buffer-size", + query_config.exchange_deduplication_buffer_size.as_ref(), + )?; + + ("QUERY", query_config.exchange_manager.as_ref()) + } + FaultTolerantExecutionConfig::Task(task_config) => { + // Set task-specific properties + Self::insert_if_present( + &mut config_properties, + "task-retry-attempts-per-task", + task_config.retry_attempts_per_task, + ); + Self::insert_if_present( + &mut config_properties, + "retry-initial-delay", + task_config + .retry_initial_delay + .as_ref() + .map(|d| format!("{}s", d.as_secs())), + ); + Self::insert_if_present( + &mut config_properties, + "retry-max-delay", + task_config + .retry_max_delay + .as_ref() + .map(|d| format!("{}s", d.as_secs())), + ); + Self::insert_if_present( + &mut config_properties, + "retry-delay-scale-factor", + task_config.retry_delay_scale_factor.as_ref(), + ); + Self::insert_quantity_if_present( + &mut config_properties, + "exchange.deduplication-buffer-size", + task_config.exchange_deduplication_buffer_size.as_ref(), + )?; + + ("TASK", Some(&task_config.exchange_manager)) + } + }; + + config_properties.insert("retry-policy".to_string(), retry_policy_str.to_string()); + + let mut exchange_manager_properties = BTreeMap::new(); + if let Some(exchange_config) = exchange_manager_opt { + Self::insert_if_present( + &mut config_properties, + "fault-tolerant-execution.exchange-encryption-enabled", + exchange_config.encryption_enabled, + ); + Self::insert_if_present( + &mut exchange_manager_properties, + "exchange.sink-buffer-pool-min-size", + exchange_config.sink_buffer_pool_min_size, + ); + Self::insert_if_present( + &mut exchange_manager_properties, + "exchange.sink-buffers-per-partition", + exchange_config.sink_buffers_per_partition, + ); + Self::insert_quantity_if_present( + &mut exchange_manager_properties, + "exchange.sink-max-file-size", + exchange_config.sink_max_file_size.as_ref(), + )?; + Self::insert_if_present( + &mut exchange_manager_properties, + "exchange.source-concurrent-readers", + exchange_config.source_concurrent_readers, + ); + + // Add backend-specific configuration + match &exchange_config.backend { + ExchangeManagerBackend::S3(s3_exchange_config) => { + exchange_manager_properties.insert( + "exchange-manager.name".to_string(), + "filesystem".to_string(), + ); + exchange_manager_properties.insert( + "exchange.base-directories".to_string(), + s3_exchange_config.base_directories.join(","), + ); + + Self::insert_if_present( + &mut exchange_manager_properties, + "exchange.s3.iam-role", + s3_exchange_config.s3_config.iam_role.as_ref(), + ); + Self::insert_if_present( + &mut exchange_manager_properties, + "exchange.s3.external-id", + s3_exchange_config.s3_config.external_id.as_ref(), + ); + Self::insert_if_present( + &mut exchange_manager_properties, + "exchange.s3.max-error-retries", + s3_exchange_config.max_error_retries, + ); + Self::insert_quantity_if_present( + &mut exchange_manager_properties, + "exchange.s3.upload.part-size", + s3_exchange_config.upload_part_size.as_ref(), + )?; + } + ExchangeManagerBackend::Hdfs(hdfs_config) => { + exchange_manager_properties + .insert("exchange-manager.name".to_string(), "hdfs".to_string()); + exchange_manager_properties.insert( + "exchange.base-directories".to_string(), + hdfs_config.base_directories.join(","), + ); + + Self::insert_quantity_if_present( + &mut exchange_manager_properties, + "exchange.hdfs.block-size", + hdfs_config.block_size.as_ref(), + )?; + Self::insert_if_present( + &mut exchange_manager_properties, + "exchange.hdfs.skip-directory-scheme-validation", + hdfs_config.skip_directory_scheme_validation, + ); + + let hdfs_config_dir = format!("{CONFIG_DIR_NAME}/exchange-hdfs-config"); + exchange_manager_properties.insert( + "hdfs.config.resources".to_string(), + format!("{hdfs_config_dir}/core-site.xml,{hdfs_config_dir}/hdfs-site.xml"), + ); + } + ExchangeManagerBackend::Local(local_config) => { + exchange_manager_properties.insert( + "exchange-manager.name".to_string(), + "filesystem".to_string(), + ); + exchange_manager_properties.insert( + "exchange.base-directories".to_string(), + local_config.base_directories.join(","), + ); + } + } + } + + let mut resolved_config = Self { + config_properties, + exchange_manager_properties, + volumes: Vec::new(), + volume_mounts: Vec::new(), + init_container_extra_start_commands: Vec::new(), + }; + + // Resolve external resources if Kubernetes client is available + // This should always be the case, except for when this function is called during unit tests + if let (Some(client), Some(exchange_config)) = (client, exchange_manager_opt) { + match &exchange_config.backend { + ExchangeManagerBackend::S3(s3_config) => { + let resolved_s3_config = config::s3::ResolvedS3Config::from_config( + &s3_config.s3_config, + client, + namespace, + ) + .await + .context(ResolveS3ConnectionSnafu)?; + + // Copy the S3 configuration over and add "exchange." prefix + resolved_config.exchange_manager_properties.extend( + resolved_s3_config + .properties + .into_iter() + .map(|(k, v)| (format!("exchange.{k}"), v)), + ); + resolved_config.volumes.extend(resolved_s3_config.volumes); + resolved_config + .volume_mounts + .extend(resolved_s3_config.volume_mounts); + resolved_config + .init_container_extra_start_commands + .extend(resolved_s3_config.init_container_extra_start_commands); + } + ExchangeManagerBackend::Hdfs(hdfs_config) => { + resolved_config.resolve_hdfs_backend(hdfs_config); + } + ExchangeManagerBackend::Local(_) => { + // Local backend requires no external resource resolution + } + } + } + + Ok(resolved_config) + } + + fn resolve_hdfs_backend(&mut self, hdfs_config: &HdfsExchangeConfig) { + let hdfs_config_dir = format!("{CONFIG_DIR_NAME}/exchange-hdfs-config"); + let volume_name = "exchange-hdfs-config".to_string(); + + self.volumes.push( + VolumeBuilder::new(&volume_name) + .with_config_map(&hdfs_config.hdfs.config_map) + .build(), + ); + self.volume_mounts + .push(VolumeMountBuilder::new(&volume_name, &hdfs_config_dir).build()); + } +} + +#[cfg(test)] +mod tests { + + use stackable_operator::shared::time::Duration; + + use super::*; + use crate::crd::{ + fault_tolerant_execution::{ + ExchangeManagerConfig, LocalExchangeConfig, QueryRetryConfig, S3ExchangeConfig, + TaskRetryConfig, + }, + s3::S3Config, + }; + + #[tokio::test] + async fn test_query_retry_policy_without_exchange_manager() { + let config = FaultTolerantExecutionConfig::Query(QueryRetryConfig { + retry_attempts: Some(5), + retry_initial_delay: Some(Duration::from_secs(15)), + retry_max_delay: Some(Duration::from_secs(90)), + retry_delay_scale_factor: Some(3.0), + exchange_deduplication_buffer_size: Some(Quantity("64Mi".to_string())), + exchange_manager: None, + }); + + let fte_config = + ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") + .await + .unwrap(); + + assert_eq!( + fte_config.config_properties.get("retry-policy"), + Some(&"QUERY".to_string()) + ); + assert_eq!( + fte_config.config_properties.get("query-retry-attempts"), + Some(&"5".to_string()) + ); + assert_eq!( + fte_config.config_properties.get("retry-initial-delay"), + Some(&"15s".to_string()) + ); + assert_eq!( + fte_config.config_properties.get("retry-max-delay"), + Some(&"90s".to_string()) + ); + assert_eq!( + fte_config.config_properties.get("retry-delay-scale-factor"), + Some(&"3".to_string()) + ); + assert_eq!( + fte_config + .config_properties + .get("exchange.deduplication-buffer-size"), + Some(&"67108864B".to_string()) + ); + } + + #[tokio::test] + async fn test_query_retry_policy_with_exchange_manager() { + let config = FaultTolerantExecutionConfig::Query(QueryRetryConfig { + retry_attempts: Some(3), + retry_initial_delay: Some(Duration::from_secs(10)), + retry_max_delay: Some(Duration::from_secs(60)), + retry_delay_scale_factor: Some(2.0), + exchange_deduplication_buffer_size: Some(Quantity("100Mi".to_string())), + exchange_manager: Some(ExchangeManagerConfig { + encryption_enabled: Some(true), + sink_buffer_pool_min_size: Some(10), + sink_buffers_per_partition: Some(2), + sink_max_file_size: Some(Quantity("1Gi".to_string())), + source_concurrent_readers: Some(4), + backend: ExchangeManagerBackend::Local(LocalExchangeConfig { + base_directories: vec!["/tmp/exchange".to_string()], + }), + }), + }); + + let fte_config = + ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") + .await + .unwrap(); + + assert_eq!( + fte_config.config_properties.get("retry-policy"), + Some(&"QUERY".to_string()) + ); + assert_eq!( + fte_config.config_properties.get("query-retry-attempts"), + Some(&"3".to_string()) + ); + assert_eq!( + fte_config.config_properties.get("retry-initial-delay"), + Some(&"10s".to_string()) + ); + assert_eq!( + fte_config.config_properties.get("retry-max-delay"), + Some(&"60s".to_string()) + ); + assert_eq!( + fte_config.config_properties.get("retry-delay-scale-factor"), + Some(&"2".to_string()) + ); + + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange-manager.name"), + Some(&"filesystem".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.base-directories"), + Some(&"/tmp/exchange".to_string()) + ); + assert_eq!( + fte_config + .config_properties + .get("exchange.deduplication-buffer-size"), + Some(&"104857600B".to_string()) + ); + assert_eq!( + fte_config + .config_properties + .get("fault-tolerant-execution.exchange-encryption-enabled"), + Some(&"true".to_string()) + ); + } + + #[tokio::test] + async fn test_task_retry_policy_with_s3_exchange_manager() { + let config = FaultTolerantExecutionConfig::Task(TaskRetryConfig { + retry_attempts_per_task: Some(2), + retry_initial_delay: None, + retry_max_delay: None, + retry_delay_scale_factor: None, + exchange_deduplication_buffer_size: None, + exchange_manager: ExchangeManagerConfig { + encryption_enabled: None, + sink_buffer_pool_min_size: Some(20), + sink_buffers_per_partition: Some(4), + sink_max_file_size: Some(Quantity("2Gi".to_string())), + source_concurrent_readers: Some(8), + backend: ExchangeManagerBackend::S3(S3ExchangeConfig { + base_directories: vec!["s3://my-bucket/exchange".to_string()], + max_error_retries: Some(5), + upload_part_size: Some(Quantity("10Mi".to_string())), + s3_config: S3Config { connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( + "test-s3-connection".to_string() + ), + iam_role: Some("arn:aws:iam::123456789012:role/TrinoRole".to_string()), + external_id: Some("external-id-123".to_string()), + } + }), + }, + }); + + let fte_config = + ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") + .await + .unwrap(); + + assert_eq!( + fte_config.config_properties.get("retry-policy"), + Some(&"TASK".to_string()) + ); + assert_eq!( + fte_config + .config_properties + .get("task-retry-attempts-per-task"), + Some(&"2".to_string()) + ); + + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange-manager.name"), + Some(&"filesystem".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.base-directories"), + Some(&"s3://my-bucket/exchange".to_string()) + ); + + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.s3.iam-role"), + Some(&"arn:aws:iam::123456789012:role/TrinoRole".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.s3.external-id"), + Some(&"external-id-123".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.s3.max-error-retries"), + Some(&"5".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.s3.upload.part-size"), + Some(&"10485760B".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.sink-buffer-pool-min-size"), + Some(&"20".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.sink-buffers-per-partition"), + Some(&"4".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.sink-max-file-size"), + Some(&"2147483648B".to_string()) + ); + assert_eq!( + fte_config + .exchange_manager_properties + .get("exchange.source-concurrent-readers"), + Some(&"8".to_string()) + ); + } +} diff --git a/rust/operator-binary/src/config/mod.rs b/rust/operator-binary/src/config/mod.rs index 26d1557f..152d5080 100644 --- a/rust/operator-binary/src/config/mod.rs +++ b/rust/operator-binary/src/config/mod.rs @@ -1,3 +1,4 @@ pub mod client_protocol; +pub mod fault_tolerant_execution; pub mod jvm; pub mod s3; diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 70686cd5..4e1af1c8 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -79,7 +79,7 @@ use crate::{ authorization::opa::TrinoOpaConfig, catalog::{FromTrinoCatalogError, config::CatalogConfig}, command, config, - config::client_protocol, + config::{client_protocol, fault_tolerant_execution}, crd::{ ACCESS_CONTROL_PROPERTIES, APP_NAME, CONFIG_DIR_NAME, CONFIG_PROPERTIES, Container, DISCOVERY_URI, ENV_INTERNAL_SECRET, ENV_SPOOLING_SECRET, EXCHANGE_MANAGER_PROPERTIES, @@ -91,7 +91,6 @@ use crate::{ authentication::resolve_authentication_classes, catalog, discovery::{TrinoDiscovery, TrinoDiscoveryProtocol, TrinoPodRef}, - fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, rolegroup_headless_service_name, v1alpha1, }, listener::{ @@ -312,7 +311,7 @@ pub enum Error { #[snafu(display("failed to configure fault tolerant execution"))] FaultTolerantExecution { - source: crate::crd::fault_tolerant_execution::Error, + source: fault_tolerant_execution::Error, }, #[snafu(display("failed to get required Labels"))] @@ -457,9 +456,13 @@ pub async fn reconcile_trino( // Resolve fault tolerant execution configuration with S3 connections if needed let resolved_fte_config = match trino.spec.cluster_config.fault_tolerant_execution.as_ref() { Some(fte_config) => Some( - ResolvedFaultTolerantExecutionConfig::from_config(fte_config, Some(client), &namespace) - .await - .context(FaultTolerantExecutionSnafu)?, + fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig::from_config( + fte_config, + Some(client), + &namespace, + ) + .await + .context(FaultTolerantExecutionSnafu)?, ), None => None, }; @@ -727,7 +730,7 @@ fn build_rolegroup_config_map( trino_authentication_config: &TrinoAuthenticationConfig, trino_opa_config: &Option, cluster_info: &KubernetesClusterInfo, - resolved_fte_config: &Option, + resolved_fte_config: &Option, resolved_spooling_config: &Option, ) -> Result { let mut cm_conf_data = BTreeMap::new(); @@ -1024,7 +1027,7 @@ fn build_rolegroup_statefulset( trino_authentication_config: &TrinoAuthenticationConfig, catalogs: &[CatalogConfig], sa_name: &str, - resolved_fte_config: &Option, + resolved_fte_config: &Option, resolved_spooling_config: &Option, ) -> Result { let role = trino @@ -1696,7 +1699,7 @@ fn tls_volume_mounts( cb_trino: &mut ContainerBuilder, catalogs: &[CatalogConfig], requested_secret_lifetime: &Duration, - resolved_fte_config: &Option, + resolved_fte_config: &Option, resolved_spooling_config: &Option, ) -> Result<()> { if let Some(server_tls) = trino.get_server_tls() { @@ -1936,7 +1939,9 @@ mod tests { fn build_config_map( trino_yaml: &str, - resolved_fte_config: &Option, + resolved_fte_config: &Option< + fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, + >, resolved_spooling_config: &Option, ) -> ConfigMap { let mut trino: v1alpha1::TrinoCluster = diff --git a/rust/operator-binary/src/crd/fault_tolerant_execution.rs b/rust/operator-binary/src/crd/fault_tolerant_execution.rs index 0ae63a16..fd8b57cc 100644 --- a/rust/operator-binary/src/crd/fault_tolerant_execution.rs +++ b/rust/operator-binary/src/crd/fault_tolerant_execution.rs @@ -5,28 +5,15 @@ //! //! Based on the Trino documentation: -use std::collections::BTreeMap; - use serde::{Deserialize, Serialize}; -use snafu::Snafu; use stackable_operator::{ - builder::pod::volume::{VolumeBuilder, VolumeMountBuilder}, - client::Client, - commons::tls_verification::{CaCert, TlsServerVerification, TlsVerification}, - crd::s3, - k8s_openapi::{ - api::core::v1::{Volume, VolumeMount}, - apimachinery::pkg::api::resource::Quantity, - }, + k8s_openapi::apimachinery::pkg::api::resource::Quantity, schemars::{self, JsonSchema}, shared::time::Duration, }; use super::catalog::commons::HdfsConnection; -use crate::{ - command, - crd::{CONFIG_DIR_NAME, STACKABLE_CLIENT_TLS_DIR}, -}; +use crate::crd::s3::S3Config; #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] @@ -145,18 +132,6 @@ pub struct S3ExchangeConfig { /// S3 bucket URIs for spooling data (e.g., s3://bucket1,s3://bucket2). pub base_directories: Vec, - /// S3 connection configuration. - /// Learn more about S3 configuration in the [S3 concept docs](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). - pub connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference, - - /// IAM role to assume for S3 access. - #[serde(skip_serializing_if = "Option::is_none")] - pub iam_role: Option, - - /// External ID for the IAM role trust policy. - #[serde(skip_serializing_if = "Option::is_none")] - pub external_id: Option, - /// Maximum number of times the S3 client should retry a request. #[serde(skip_serializing_if = "Option::is_none")] pub max_error_retries: Option, @@ -164,6 +139,11 @@ pub struct S3ExchangeConfig { /// Part data size for S3 multi-part upload. #[serde(skip_serializing_if = "Option::is_none")] pub upload_part_size: Option, + + /// S3 connection configuration. + /// Learn more about S3 configuration in the [S3 concept docs](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). + #[serde(flatten)] + pub s3_config: S3Config, } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -190,594 +170,3 @@ pub struct LocalExchangeConfig { /// Local filesystem paths for exchange storage. pub base_directories: Vec, } - -#[derive(Snafu, Debug)] -pub enum Error { - #[snafu(display("Failed to resolve S3 connection"))] - S3Connection { - source: s3::v1alpha1::ConnectionError, - }, - - #[snafu(display("trino does not support disabling the TLS verification of S3 servers"))] - S3TlsNoVerificationNotSupported, - - #[snafu(display("failed to convert data size for [{field}] to bytes"))] - QuantityConversion { - source: stackable_operator::memory::Error, - field: &'static str, - }, -} - -/// Fault tolerant execution configuration with external resources resolved -pub struct ResolvedFaultTolerantExecutionConfig { - /// Properties to add to config.properties - pub config_properties: BTreeMap, - - /// Properties to add to exchange-manager.properties (if needed) - pub exchange_manager_properties: BTreeMap, - - /// Volumes required for the configuration (e.g., for S3 credentials) - pub volumes: Vec, - - /// Volume mounts required for the configuration - pub volume_mounts: Vec, - - /// Additional commands that need to be executed before starting Trino - pub init_container_extra_start_commands: Vec, -} - -impl ResolvedFaultTolerantExecutionConfig { - /// Helper function to insert optional values into properties map - fn insert_if_present( - properties: &mut BTreeMap, - key: &str, - value: Option, - ) { - if let Some(v) = value { - properties.insert(key.to_string(), v.to_string()); - } - } - - /// Helper function to insert optional Quantity values after converting to Trino bytes string - fn insert_quantity_if_present( - properties: &mut BTreeMap, - key: &'static str, - quantity: Option<&Quantity>, - ) -> Result<(), Error> { - if let Some(q) = quantity { - use snafu::ResultExt; - let v = crate::crd::quantity_to_trino_bytes(q) - .context(QuantityConversionSnafu { field: key })?; - properties.insert(key.to_string(), v); - } - Ok(()) - } - - /// Create a resolved fault tolerant execution configuration from the cluster config - pub async fn from_config( - config: &FaultTolerantExecutionConfig, - client: Option<&Client>, - namespace: &str, - ) -> Result { - let mut config_properties = BTreeMap::new(); - - // Handle different retry policies and their configurations - let (retry_policy_str, exchange_manager_opt) = match config { - FaultTolerantExecutionConfig::Query(query_config) => { - // Set query-specific properties - Self::insert_if_present( - &mut config_properties, - "query-retry-attempts", - query_config.retry_attempts, - ); - Self::insert_if_present( - &mut config_properties, - "retry-initial-delay", - query_config - .retry_initial_delay - .as_ref() - .map(|d| format!("{}s", d.as_secs())), - ); - Self::insert_if_present( - &mut config_properties, - "retry-max-delay", - query_config - .retry_max_delay - .as_ref() - .map(|d| format!("{}s", d.as_secs())), - ); - Self::insert_if_present( - &mut config_properties, - "retry-delay-scale-factor", - query_config.retry_delay_scale_factor.as_ref(), - ); - Self::insert_quantity_if_present( - &mut config_properties, - "exchange.deduplication-buffer-size", - query_config.exchange_deduplication_buffer_size.as_ref(), - )?; - - ("QUERY", query_config.exchange_manager.as_ref()) - } - FaultTolerantExecutionConfig::Task(task_config) => { - // Set task-specific properties - Self::insert_if_present( - &mut config_properties, - "task-retry-attempts-per-task", - task_config.retry_attempts_per_task, - ); - Self::insert_if_present( - &mut config_properties, - "retry-initial-delay", - task_config - .retry_initial_delay - .as_ref() - .map(|d| format!("{}s", d.as_secs())), - ); - Self::insert_if_present( - &mut config_properties, - "retry-max-delay", - task_config - .retry_max_delay - .as_ref() - .map(|d| format!("{}s", d.as_secs())), - ); - Self::insert_if_present( - &mut config_properties, - "retry-delay-scale-factor", - task_config.retry_delay_scale_factor.as_ref(), - ); - Self::insert_quantity_if_present( - &mut config_properties, - "exchange.deduplication-buffer-size", - task_config.exchange_deduplication_buffer_size.as_ref(), - )?; - - ("TASK", Some(&task_config.exchange_manager)) - } - }; - - config_properties.insert("retry-policy".to_string(), retry_policy_str.to_string()); - - let mut exchange_manager_properties = BTreeMap::new(); - if let Some(exchange_config) = exchange_manager_opt { - Self::insert_if_present( - &mut config_properties, - "fault-tolerant-execution.exchange-encryption-enabled", - exchange_config.encryption_enabled, - ); - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.sink-buffer-pool-min-size", - exchange_config.sink_buffer_pool_min_size, - ); - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.sink-buffers-per-partition", - exchange_config.sink_buffers_per_partition, - ); - Self::insert_quantity_if_present( - &mut exchange_manager_properties, - "exchange.sink-max-file-size", - exchange_config.sink_max_file_size.as_ref(), - )?; - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.source-concurrent-readers", - exchange_config.source_concurrent_readers, - ); - - // Add backend-specific configuration - match &exchange_config.backend { - ExchangeManagerBackend::S3(s3_config) => { - exchange_manager_properties.insert( - "exchange-manager.name".to_string(), - "filesystem".to_string(), - ); - exchange_manager_properties.insert( - "exchange.base-directories".to_string(), - s3_config.base_directories.join(","), - ); - - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.s3.iam-role", - s3_config.iam_role.as_ref(), - ); - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.s3.external-id", - s3_config.external_id.as_ref(), - ); - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.s3.max-error-retries", - s3_config.max_error_retries, - ); - Self::insert_quantity_if_present( - &mut exchange_manager_properties, - "exchange.s3.upload.part-size", - s3_config.upload_part_size.as_ref(), - )?; - } - ExchangeManagerBackend::Hdfs(hdfs_config) => { - exchange_manager_properties - .insert("exchange-manager.name".to_string(), "hdfs".to_string()); - exchange_manager_properties.insert( - "exchange.base-directories".to_string(), - hdfs_config.base_directories.join(","), - ); - - Self::insert_quantity_if_present( - &mut exchange_manager_properties, - "exchange.hdfs.block-size", - hdfs_config.block_size.as_ref(), - )?; - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.hdfs.skip-directory-scheme-validation", - hdfs_config.skip_directory_scheme_validation, - ); - - let hdfs_config_dir = format!("{CONFIG_DIR_NAME}/exchange-hdfs-config"); - exchange_manager_properties.insert( - "hdfs.config.resources".to_string(), - format!("{hdfs_config_dir}/core-site.xml,{hdfs_config_dir}/hdfs-site.xml"), - ); - } - ExchangeManagerBackend::Local(local_config) => { - exchange_manager_properties.insert( - "exchange-manager.name".to_string(), - "filesystem".to_string(), - ); - exchange_manager_properties.insert( - "exchange.base-directories".to_string(), - local_config.base_directories.join(","), - ); - } - } - } - - let mut resolved_config = Self { - config_properties, - exchange_manager_properties, - volumes: Vec::new(), - volume_mounts: Vec::new(), - init_container_extra_start_commands: Vec::new(), - }; - - // Resolve external resources if Kubernetes client is available - // This should always be the case, except for when this function is called during unit tests - if let (Some(client), Some(exchange_config)) = (client, exchange_manager_opt) { - match &exchange_config.backend { - ExchangeManagerBackend::S3(s3_config) => { - resolved_config - .resolve_s3_backend(s3_config, client, namespace) - .await?; - } - ExchangeManagerBackend::Hdfs(hdfs_config) => { - resolved_config.resolve_hdfs_backend(hdfs_config); - } - ExchangeManagerBackend::Local(_) => { - // Local backend requires no external resource resolution - } - } - } - - Ok(resolved_config) - } - - async fn resolve_s3_backend( - &mut self, - s3_config: &S3ExchangeConfig, - client: &Client, - namespace: &str, - ) -> Result<(), Error> { - use snafu::ResultExt; - - let s3_connection = s3_config - .connection - .clone() - .resolve(client, namespace) - .await - .context(S3ConnectionSnafu)?; - - let (volumes, mounts) = s3_connection - .volumes_and_mounts() - .context(S3ConnectionSnafu)?; - self.volumes.extend(volumes); - self.volume_mounts.extend(mounts); - - self.exchange_manager_properties.insert( - "exchange.s3.region".to_string(), - s3_connection.region.name.clone(), - ); - self.exchange_manager_properties.insert( - "exchange.s3.endpoint".to_string(), - s3_connection - .endpoint() - .context(S3ConnectionSnafu)? - .to_string(), - ); - self.exchange_manager_properties.insert( - "exchange.s3.path-style-access".to_string(), - (s3_connection.access_style == s3::v1alpha1::S3AccessStyle::Path).to_string(), - ); - - if let Some((access_key_path, secret_key_path)) = s3_connection.credentials_mount_paths() { - self.exchange_manager_properties.insert( - "exchange.s3.aws-access-key".to_string(), - format!("${{file:UTF-8:{access_key_path}}}"), - ); - self.exchange_manager_properties.insert( - "exchange.s3.aws-secret-key".to_string(), - format!("${{file:UTF-8:{secret_key_path}}}"), - ); - } - - if let Some(tls) = s3_connection.tls.tls.as_ref() { - match &tls.verification { - TlsVerification::None {} => return S3TlsNoVerificationNotSupportedSnafu.fail(), - TlsVerification::Server(TlsServerVerification { - ca_cert: CaCert::WebPki {}, - }) => {} - TlsVerification::Server(TlsServerVerification { - ca_cert: CaCert::SecretClass(_), - }) => { - if let Some(ca_cert) = s3_connection.tls.tls_ca_cert_mount_path() { - self.init_container_extra_start_commands.extend( - command::add_cert_to_truststore( - &ca_cert, - STACKABLE_CLIENT_TLS_DIR, - "exchange-s3-ca-cert", - ), - ); - } - } - } - } - - Ok(()) - } - - fn resolve_hdfs_backend(&mut self, hdfs_config: &HdfsExchangeConfig) { - let hdfs_config_dir = format!("{CONFIG_DIR_NAME}/exchange-hdfs-config"); - let volume_name = "exchange-hdfs-config".to_string(); - - self.volumes.push( - VolumeBuilder::new(&volume_name) - .with_config_map(&hdfs_config.hdfs.config_map) - .build(), - ); - self.volume_mounts - .push(VolumeMountBuilder::new(&volume_name, &hdfs_config_dir).build()); - } -} - -#[cfg(test)] -mod tests { - - use super::*; - - #[tokio::test] - async fn test_query_retry_policy_without_exchange_manager() { - let config = FaultTolerantExecutionConfig::Query(QueryRetryConfig { - retry_attempts: Some(5), - retry_initial_delay: Some(Duration::from_secs(15)), - retry_max_delay: Some(Duration::from_secs(90)), - retry_delay_scale_factor: Some(3.0), - exchange_deduplication_buffer_size: Some(Quantity("64Mi".to_string())), - exchange_manager: None, - }); - - let fte_config = - ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") - .await - .unwrap(); - - assert_eq!( - fte_config.config_properties.get("retry-policy"), - Some(&"QUERY".to_string()) - ); - assert_eq!( - fte_config.config_properties.get("query-retry-attempts"), - Some(&"5".to_string()) - ); - assert_eq!( - fte_config.config_properties.get("retry-initial-delay"), - Some(&"15s".to_string()) - ); - assert_eq!( - fte_config.config_properties.get("retry-max-delay"), - Some(&"90s".to_string()) - ); - assert_eq!( - fte_config.config_properties.get("retry-delay-scale-factor"), - Some(&"3".to_string()) - ); - assert_eq!( - fte_config - .config_properties - .get("exchange.deduplication-buffer-size"), - Some(&"67108864B".to_string()) - ); - } - - #[tokio::test] - async fn test_query_retry_policy_with_exchange_manager() { - let config = FaultTolerantExecutionConfig::Query(QueryRetryConfig { - retry_attempts: Some(3), - retry_initial_delay: Some(Duration::from_secs(10)), - retry_max_delay: Some(Duration::from_secs(60)), - retry_delay_scale_factor: Some(2.0), - exchange_deduplication_buffer_size: Some(Quantity("100Mi".to_string())), - exchange_manager: Some(ExchangeManagerConfig { - encryption_enabled: Some(true), - sink_buffer_pool_min_size: Some(10), - sink_buffers_per_partition: Some(2), - sink_max_file_size: Some(Quantity("1Gi".to_string())), - source_concurrent_readers: Some(4), - backend: ExchangeManagerBackend::Local(LocalExchangeConfig { - base_directories: vec!["/tmp/exchange".to_string()], - }), - }), - }); - - let fte_config = - ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") - .await - .unwrap(); - - assert_eq!( - fte_config.config_properties.get("retry-policy"), - Some(&"QUERY".to_string()) - ); - assert_eq!( - fte_config.config_properties.get("query-retry-attempts"), - Some(&"3".to_string()) - ); - assert_eq!( - fte_config.config_properties.get("retry-initial-delay"), - Some(&"10s".to_string()) - ); - assert_eq!( - fte_config.config_properties.get("retry-max-delay"), - Some(&"60s".to_string()) - ); - assert_eq!( - fte_config.config_properties.get("retry-delay-scale-factor"), - Some(&"2".to_string()) - ); - - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange-manager.name"), - Some(&"filesystem".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.base-directories"), - Some(&"/tmp/exchange".to_string()) - ); - assert_eq!( - fte_config - .config_properties - .get("exchange.deduplication-buffer-size"), - Some(&"104857600B".to_string()) - ); - assert_eq!( - fte_config - .config_properties - .get("fault-tolerant-execution.exchange-encryption-enabled"), - Some(&"true".to_string()) - ); - } - - #[tokio::test] - async fn test_task_retry_policy_with_s3_exchange_manager() { - let config = FaultTolerantExecutionConfig::Task(TaskRetryConfig { - retry_attempts_per_task: Some(2), - retry_initial_delay: None, - retry_max_delay: None, - retry_delay_scale_factor: None, - exchange_deduplication_buffer_size: None, - exchange_manager: ExchangeManagerConfig { - encryption_enabled: None, - sink_buffer_pool_min_size: Some(20), - sink_buffers_per_partition: Some(4), - sink_max_file_size: Some(Quantity("2Gi".to_string())), - source_concurrent_readers: Some(8), - backend: ExchangeManagerBackend::S3(S3ExchangeConfig { - base_directories: vec!["s3://my-bucket/exchange".to_string()], - connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( - "test-s3-connection".to_string() - ), - iam_role: Some("arn:aws:iam::123456789012:role/TrinoRole".to_string()), - external_id: Some("external-id-123".to_string()), - max_error_retries: Some(5), - upload_part_size: Some(Quantity("10Mi".to_string())), - }), - }, - }); - - let fte_config = - ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") - .await - .unwrap(); - - assert_eq!( - fte_config.config_properties.get("retry-policy"), - Some(&"TASK".to_string()) - ); - assert_eq!( - fte_config - .config_properties - .get("task-retry-attempts-per-task"), - Some(&"2".to_string()) - ); - - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange-manager.name"), - Some(&"filesystem".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.base-directories"), - Some(&"s3://my-bucket/exchange".to_string()) - ); - - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.s3.iam-role"), - Some(&"arn:aws:iam::123456789012:role/TrinoRole".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.s3.external-id"), - Some(&"external-id-123".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.s3.max-error-retries"), - Some(&"5".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.s3.upload.part-size"), - Some(&"10485760B".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.sink-buffer-pool-min-size"), - Some(&"20".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.sink-buffers-per-partition"), - Some(&"4".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.sink-max-file-size"), - Some(&"2147483648B".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.source-concurrent-readers"), - Some(&"8".to_string()) - ); - } -} diff --git a/rust/operator-binary/src/crd/s3.rs b/rust/operator-binary/src/crd/s3.rs index db414e42..69027289 100644 --- a/rust/operator-binary/src/crd/s3.rs +++ b/rust/operator-binary/src/crd/s3.rs @@ -1,8 +1,5 @@ use serde::{Deserialize, Serialize}; -use stackable_operator::{ - k8s_openapi::apimachinery::pkg::api::resource::Quantity, - schemars::{self, JsonSchema}, -}; +use stackable_operator::schemars::{self, JsonSchema}; #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] @@ -18,12 +15,4 @@ pub struct S3Config { /// External ID for the IAM role trust policy. #[serde(skip_serializing_if = "Option::is_none")] pub external_id: Option, - - /// Maximum number of times the S3 client should retry a request. - #[serde(skip_serializing_if = "Option::is_none")] - pub max_error_retries: Option, - - /// Part data size for S3 multi-part upload. - #[serde(skip_serializing_if = "Option::is_none")] - pub upload_part_size: Option, } From c030c383a59beafb7db00034c7dd8e832a792ee4 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 16 Sep 2025 15:01:34 +0200 Subject: [PATCH 30/39] Apply suggestions from code review Co-authored-by: Sebastian Bernauer --- CHANGELOG.md | 3 +-- .../src/config/client_protocol.rs | 26 ++++++++++--------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0815a616..7851cc73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,7 @@ All notable changes to this project will be documented in this file. ### Added -- Support for fault-tolerant execution ([#779]). - Remove `spec.clusterConfig.faultTolerantExecution.configOverrides` and use "classic" role/group overrides ([#793]). +- Support for fault-tolerant execution ([#779], [#793]). - Support for the client spooling protocol ([#793]). [#779]: https://github.com/stackabletech/trino-operator/pull/779 diff --git a/rust/operator-binary/src/config/client_protocol.rs b/rust/operator-binary/src/config/client_protocol.rs index d8618f51..2e8c6fcc 100644 --- a/rust/operator-binary/src/config/client_protocol.rs +++ b/rust/operator-binary/src/config/client_protocol.rs @@ -111,7 +111,7 @@ impl ResolvedClientProtocolConfig { ("protocol.spooling.enabled".to_string(), "true".to_string()), ( "protocol.spooling.shared-secret-key".to_string(), - format!("${{ENV:{secret}}}", secret = ENV_SPOOLING_SECRET), + format!("${{ENV:{ENV_SPOOLING_SECRET}}}"), ), ]); } @@ -128,17 +128,19 @@ mod tests { #[tokio::test] async fn test_spooling_config() { - let config = ClientProtocolConfig::Spooling(ClientSpoolingProtocolConfig { - location: "s3://my-bucket/spooling".to_string(), - filesystem: SpoolingFileSystemConfig::S3(S3Config { - connection: - stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( - "test-s3-connection".to_string(), - ), - iam_role: None, - external_id: None, - }), - }); + let config_yaml = indoc::indoc! {r#" + spooling: + location: s3://my-bucket/spooling + filesystem: + s3: + connection: + reference: test-s3-connection + "#}; + + let deserializer = serde_yaml::Deserializer::from_str(config_yaml); + let config = serde_yaml::with::singleton_map_recursive::deserialize(deserializer) + .expect("invalid test input"); + let resolved_spooling_config = ResolvedClientProtocolConfig::from_config( &config, None, // No client, so no external resolution From 6757b5e93254ec908a11759e9e66edc97a75c17d Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 16 Sep 2025 15:18:26 +0200 Subject: [PATCH 31/39] remove unused imports after suggestion --- rust/operator-binary/src/config/client_protocol.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/rust/operator-binary/src/config/client_protocol.rs b/rust/operator-binary/src/config/client_protocol.rs index 2e8c6fcc..a39653aa 100644 --- a/rust/operator-binary/src/config/client_protocol.rs +++ b/rust/operator-binary/src/config/client_protocol.rs @@ -124,7 +124,6 @@ impl ResolvedClientProtocolConfig { #[cfg(test)] mod tests { use super::*; - use crate::crd::{client_protocol::ClientSpoolingProtocolConfig, s3::S3Config}; #[tokio::test] async fn test_spooling_config() { @@ -141,7 +140,6 @@ mod tests { let config = serde_yaml::with::singleton_map_recursive::deserialize(deserializer) .expect("invalid test input"); - let resolved_spooling_config = ResolvedClientProtocolConfig::from_config( &config, None, // No client, so no external resolution "default", From b5c194122b35ef6f3c77e5d6a7f70d3738a0042a Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 16 Sep 2025 15:39:08 +0200 Subject: [PATCH 32/39] remove crd::s3 which included iam fields --- deploy/helm/trino-operator/crds/crds.yaml | 228 ++++++++---------- .../src/config/client_protocol.rs | 3 +- .../src/config/fault_tolerant_execution.rs | 38 +-- rust/operator-binary/src/config/s3.rs | 10 +- .../src/crd/client_protocol.rs | 4 +- .../src/crd/fault_tolerant_execution.rs | 4 +- rust/operator-binary/src/crd/mod.rs | 1 - rust/operator-binary/src/crd/s3.rs | 18 -- 8 files changed, 110 insertions(+), 196 deletions(-) delete mode 100644 rust/operator-binary/src/crd/s3.rs diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index 42bd0bc3..1a47f7bd 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -120,139 +120,125 @@ spec: - s3 properties: s3: + oneOf: + - required: + - inline + - required: + - reference properties: - connection: - description: S3 connection configuration. Learn more about S3 configuration in the [S3 concept docs](https://docs.stackable.tech/home/nightly/concepts/s3). - oneOf: - - required: - - inline - - required: - - reference + inline: + description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). properties: - inline: - description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). + accessStyle: + default: VirtualHosted + description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). + enum: + - Path + - VirtualHosted + type: string + credentials: + description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. + nullable: true properties: - accessStyle: - default: VirtualHosted - description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). - enum: - - Path - - VirtualHosted - type: string - credentials: - description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. + scope: + description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' nullable: true properties: - scope: - description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' - nullable: true - properties: - listenerVolumes: - default: [] - description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. - items: - type: string - type: array - node: - default: false - description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. - type: boolean - pod: - default: false - description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. - type: boolean - services: - default: [] - description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. - items: - type: string - type: array - type: object - secretClass: - description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' - type: string - required: - - secretClass + listenerVolumes: + default: [] + description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. + items: + type: string + type: array + node: + default: false + description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. + type: boolean + pod: + default: false + description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. + type: boolean + services: + default: [] + description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. + items: + type: string + type: array type: object - host: - description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' + secretClass: + description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' type: string - port: - description: Port the S3 server listens on. If not specified the product will determine the port to use. - format: uint16 - minimum: 0.0 - nullable: true - type: integer - region: - default: - name: us-east-1 - description: |- - Bucket region used for signing headers (sigv4). + required: + - secretClass + type: object + host: + description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' + type: string + port: + description: Port the S3 server listens on. If not specified the product will determine the port to use. + format: uint16 + minimum: 0.0 + nullable: true + type: integer + region: + default: + name: us-east-1 + description: |- + Bucket region used for signing headers (sigv4). - This defaults to `us-east-1` which is compatible with other implementations such as Minio. + This defaults to `us-east-1` which is compatible with other implementations such as Minio. - WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. - properties: - name: - default: us-east-1 - type: string - type: object - tls: - description: Use a TLS connection. If not specified no TLS will be used. - nullable: true + WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. + properties: + name: + default: us-east-1 + type: string + type: object + tls: + description: Use a TLS connection. If not specified no TLS will be used. + nullable: true + properties: + verification: + description: The verification method used to verify the certificates of the server and/or the client. + oneOf: + - required: + - none + - required: + - server properties: - verification: - description: The verification method used to verify the certificates of the server and/or the client. - oneOf: - - required: - - none - - required: - - server + none: + description: Use TLS but don't verify certificates. + type: object + server: + description: Use TLS and a CA certificate to verify the server. properties: - none: - description: Use TLS but don't verify certificates. - type: object - server: - description: Use TLS and a CA certificate to verify the server. + caCert: + description: CA cert to verify the server. + oneOf: + - required: + - webPki + - required: + - secretClass properties: - caCert: - description: CA cert to verify the server. - oneOf: - - required: - - webPki - - required: - - secretClass - properties: - secretClass: - description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. - type: string - webPki: - description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. - type: object + secretClass: + description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. + type: string + webPki: + description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. type: object - required: - - caCert type: object + required: + - caCert type: object - required: - - verification type: object required: - - host + - verification type: object - reference: - type: string + required: + - host type: object - externalId: - description: External ID for the IAM role trust policy. - nullable: true + reference: type: string - iamRole: - description: IAM role to assume for S3 access. - nullable: true - type: string - required: - - connection type: object type: object location: @@ -463,14 +449,6 @@ spec: reference: type: string type: object - externalId: - description: External ID for the IAM role trust policy. - nullable: true - type: string - iamRole: - description: IAM role to assume for S3 access. - nullable: true - type: string maxErrorRetries: description: Maximum number of times the S3 client should retry a request. format: uint32 @@ -719,14 +697,6 @@ spec: reference: type: string type: object - externalId: - description: External ID for the IAM role trust policy. - nullable: true - type: string - iamRole: - description: IAM role to assume for S3 access. - nullable: true - type: string maxErrorRetries: description: Maximum number of times the S3 client should retry a request. format: uint32 diff --git a/rust/operator-binary/src/config/client_protocol.rs b/rust/operator-binary/src/config/client_protocol.rs index a39653aa..6e4822b8 100644 --- a/rust/operator-binary/src/config/client_protocol.rs +++ b/rust/operator-binary/src/config/client_protocol.rs @@ -132,8 +132,7 @@ mod tests { location: s3://my-bucket/spooling filesystem: s3: - connection: - reference: test-s3-connection + reference: test-s3-connection "#}; let deserializer = serde_yaml::Deserializer::from_str(config_yaml); diff --git a/rust/operator-binary/src/config/fault_tolerant_execution.rs b/rust/operator-binary/src/config/fault_tolerant_execution.rs index b2497f56..8aa311d5 100644 --- a/rust/operator-binary/src/config/fault_tolerant_execution.rs +++ b/rust/operator-binary/src/config/fault_tolerant_execution.rs @@ -212,16 +212,6 @@ impl ResolvedFaultTolerantExecutionConfig { s3_exchange_config.base_directories.join(","), ); - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.s3.iam-role", - s3_exchange_config.s3_config.iam_role.as_ref(), - ); - Self::insert_if_present( - &mut exchange_manager_properties, - "exchange.s3.external-id", - s3_exchange_config.s3_config.external_id.as_ref(), - ); Self::insert_if_present( &mut exchange_manager_properties, "exchange.s3.max-error-retries", @@ -285,7 +275,7 @@ impl ResolvedFaultTolerantExecutionConfig { match &exchange_config.backend { ExchangeManagerBackend::S3(s3_config) => { let resolved_s3_config = config::s3::ResolvedS3Config::from_config( - &s3_config.s3_config, + &s3_config.connection, client, namespace, ) @@ -339,12 +329,9 @@ mod tests { use stackable_operator::shared::time::Duration; use super::*; - use crate::crd::{ - fault_tolerant_execution::{ - ExchangeManagerConfig, LocalExchangeConfig, QueryRetryConfig, S3ExchangeConfig, - TaskRetryConfig, - }, - s3::S3Config, + use crate::crd::fault_tolerant_execution::{ + ExchangeManagerConfig, LocalExchangeConfig, QueryRetryConfig, S3ExchangeConfig, + TaskRetryConfig, }; #[tokio::test] @@ -481,12 +468,9 @@ mod tests { base_directories: vec!["s3://my-bucket/exchange".to_string()], max_error_retries: Some(5), upload_part_size: Some(Quantity("10Mi".to_string())), - s3_config: S3Config { connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( + connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( "test-s3-connection".to_string() ), - iam_role: Some("arn:aws:iam::123456789012:role/TrinoRole".to_string()), - external_id: Some("external-id-123".to_string()), - } }), }, }); @@ -520,18 +504,6 @@ mod tests { Some(&"s3://my-bucket/exchange".to_string()) ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.s3.iam-role"), - Some(&"arn:aws:iam::123456789012:role/TrinoRole".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.s3.external-id"), - Some(&"external-id-123".to_string()) - ); assert_eq!( fte_config .exchange_manager_properties diff --git a/rust/operator-binary/src/config/s3.rs b/rust/operator-binary/src/config/s3.rs index 97df8c5f..4df59553 100644 --- a/rust/operator-binary/src/config/s3.rs +++ b/rust/operator-binary/src/config/s3.rs @@ -8,10 +8,7 @@ use stackable_operator::{ k8s_openapi::api::core::v1::{Volume, VolumeMount}, }; -use crate::{ - command, - crd::{STACKABLE_CLIENT_TLS_DIR, s3 as trino_s3}, -}; +use crate::{command, crd::STACKABLE_CLIENT_TLS_DIR}; #[derive(Snafu, Debug)] pub enum Error { @@ -49,7 +46,7 @@ impl ResolvedS3Config { /// Resolve S3 connection properties from Kubernetes resources /// and prepare spooling filesystem configuration. pub async fn from_config( - config: &trino_s3::S3Config, + connection: &stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference, client: &Client, namespace: &str, ) -> Result { @@ -60,8 +57,7 @@ impl ResolvedS3Config { init_container_extra_start_commands: Vec::new(), }; - let s3_connection = config - .connection + let s3_connection = connection .clone() .resolve(client, namespace) .await diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index 62490a4f..b5bc6c54 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -3,8 +3,6 @@ use serde::{Deserialize, Serialize}; use stackable_operator::schemars::{self, JsonSchema}; -use crate::crd::s3::S3Config; - #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub enum ClientProtocolConfig { @@ -24,5 +22,5 @@ pub struct ClientSpoolingProtocolConfig { #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub enum SpoolingFileSystemConfig { - S3(S3Config), + S3(stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference), } diff --git a/rust/operator-binary/src/crd/fault_tolerant_execution.rs b/rust/operator-binary/src/crd/fault_tolerant_execution.rs index fd8b57cc..595cea50 100644 --- a/rust/operator-binary/src/crd/fault_tolerant_execution.rs +++ b/rust/operator-binary/src/crd/fault_tolerant_execution.rs @@ -13,7 +13,6 @@ use stackable_operator::{ }; use super::catalog::commons::HdfsConnection; -use crate::crd::s3::S3Config; #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] @@ -142,8 +141,7 @@ pub struct S3ExchangeConfig { /// S3 connection configuration. /// Learn more about S3 configuration in the [S3 concept docs](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). - #[serde(flatten)] - pub s3_config: S3Config, + pub connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference, } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index f1cafc83..e3cd0aa9 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -4,7 +4,6 @@ pub mod catalog; pub mod client_protocol; pub mod discovery; pub mod fault_tolerant_execution; -pub mod s3; use std::{collections::BTreeMap, ops::Div, str::FromStr}; diff --git a/rust/operator-binary/src/crd/s3.rs b/rust/operator-binary/src/crd/s3.rs deleted file mode 100644 index 69027289..00000000 --- a/rust/operator-binary/src/crd/s3.rs +++ /dev/null @@ -1,18 +0,0 @@ -use serde::{Deserialize, Serialize}; -use stackable_operator::schemars::{self, JsonSchema}; - -#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] -pub struct S3Config { - /// S3 connection configuration. - /// Learn more about S3 configuration in the [S3 concept docs](DOCS_BASE_URL_PLACEHOLDER/concepts/s3). - pub connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference, - - /// IAM role to assume for S3 access. - #[serde(skip_serializing_if = "Option::is_none")] - pub iam_role: Option, - - /// External ID for the IAM role trust policy. - #[serde(skip_serializing_if = "Option::is_none")] - pub external_id: Option, -} From af0806b1d1ec0763031e9f6f6c57a7188647c5d2 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 16 Sep 2025 15:56:30 +0200 Subject: [PATCH 33/39] fte tests: replace inline structs with indoc --- .../src/config/fault_tolerant_execution.rs | 125 ++++++------------ 1 file changed, 43 insertions(+), 82 deletions(-) diff --git a/rust/operator-binary/src/config/fault_tolerant_execution.rs b/rust/operator-binary/src/config/fault_tolerant_execution.rs index 8aa311d5..f9662df3 100644 --- a/rust/operator-binary/src/config/fault_tolerant_execution.rs +++ b/rust/operator-binary/src/config/fault_tolerant_execution.rs @@ -326,24 +326,26 @@ impl ResolvedFaultTolerantExecutionConfig { #[cfg(test)] mod tests { - use stackable_operator::shared::time::Duration; + use indoc::indoc; use super::*; - use crate::crd::fault_tolerant_execution::{ - ExchangeManagerConfig, LocalExchangeConfig, QueryRetryConfig, S3ExchangeConfig, - TaskRetryConfig, - }; + + fn parse_config(config_yaml: &str) -> FaultTolerantExecutionConfig { + let deserializer = serde_yaml::Deserializer::from_str(config_yaml); + serde_yaml::with::singleton_map_recursive::deserialize(deserializer) + .expect("invalid test input") + } #[tokio::test] async fn test_query_retry_policy_without_exchange_manager() { - let config = FaultTolerantExecutionConfig::Query(QueryRetryConfig { - retry_attempts: Some(5), - retry_initial_delay: Some(Duration::from_secs(15)), - retry_max_delay: Some(Duration::from_secs(90)), - retry_delay_scale_factor: Some(3.0), - exchange_deduplication_buffer_size: Some(Quantity("64Mi".to_string())), - exchange_manager: None, - }); + let config = parse_config(indoc! {r#" + query: + retryAttempts: 5 + retryInitialDelay: 15s + retryMaxDelay: 90s + retryDelayScaleFactor: 3 + exchangeDeduplicationBufferSize: 64Mi + "#}); let fte_config = ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") @@ -380,23 +382,22 @@ mod tests { #[tokio::test] async fn test_query_retry_policy_with_exchange_manager() { - let config = FaultTolerantExecutionConfig::Query(QueryRetryConfig { - retry_attempts: Some(3), - retry_initial_delay: Some(Duration::from_secs(10)), - retry_max_delay: Some(Duration::from_secs(60)), - retry_delay_scale_factor: Some(2.0), - exchange_deduplication_buffer_size: Some(Quantity("100Mi".to_string())), - exchange_manager: Some(ExchangeManagerConfig { - encryption_enabled: Some(true), - sink_buffer_pool_min_size: Some(10), - sink_buffers_per_partition: Some(2), - sink_max_file_size: Some(Quantity("1Gi".to_string())), - source_concurrent_readers: Some(4), - backend: ExchangeManagerBackend::Local(LocalExchangeConfig { - base_directories: vec!["/tmp/exchange".to_string()], - }), - }), - }); + let config = parse_config(indoc! {r#" + query: + retryAttempts: 3 + retryInitialDelay: 10s + retryMaxDelay: 1m + retryDelayScaleFactor: 2 + exchangeDeduplicationBufferSize: 100Mi + exchangeManager: + encryptionEnabled: true + sinkBufferPoolMinSize: 10 + sinkBuffersPerPartition: 2 + sinkMaxFileSize: 1Gi + sourceConcurrentReaders: 4 + local: + baseDirectories: ["/tmp/exchange"] + "#}); let fte_config = ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") @@ -452,28 +453,18 @@ mod tests { #[tokio::test] async fn test_task_retry_policy_with_s3_exchange_manager() { - let config = FaultTolerantExecutionConfig::Task(TaskRetryConfig { - retry_attempts_per_task: Some(2), - retry_initial_delay: None, - retry_max_delay: None, - retry_delay_scale_factor: None, - exchange_deduplication_buffer_size: None, - exchange_manager: ExchangeManagerConfig { - encryption_enabled: None, - sink_buffer_pool_min_size: Some(20), - sink_buffers_per_partition: Some(4), - sink_max_file_size: Some(Quantity("2Gi".to_string())), - source_concurrent_readers: Some(8), - backend: ExchangeManagerBackend::S3(S3ExchangeConfig { - base_directories: vec!["s3://my-bucket/exchange".to_string()], - max_error_retries: Some(5), - upload_part_size: Some(Quantity("10Mi".to_string())), - connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference::Reference( - "test-s3-connection".to_string() - ), - }), - }, - }); + let config = parse_config(indoc! {r#" + task: + exchangeManager: + s3: + baseDirectories: ["s3://my-bucket/exchange"] + connection: + reference: test-s3-connection + maxErrorRetries: 5 + uploadPartSize: 10Mi + iamRole: arn:aws:iam::123456789012:role/TrinoRole + externalId: external-id-123 + "#}); let fte_config = ResolvedFaultTolerantExecutionConfig::from_config(&config, None, "default") @@ -484,12 +475,6 @@ mod tests { fte_config.config_properties.get("retry-policy"), Some(&"TASK".to_string()) ); - assert_eq!( - fte_config - .config_properties - .get("task-retry-attempts-per-task"), - Some(&"2".to_string()) - ); assert_eq!( fte_config @@ -516,29 +501,5 @@ mod tests { .get("exchange.s3.upload.part-size"), Some(&"10485760B".to_string()) ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.sink-buffer-pool-min-size"), - Some(&"20".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.sink-buffers-per-partition"), - Some(&"4".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.sink-max-file-size"), - Some(&"2147483648B".to_string()) - ); - assert_eq!( - fte_config - .exchange_manager_properties - .get("exchange.source-concurrent-readers"), - Some(&"8".to_string()) - ); } } From dc31a89d6f1e496367f14b5f432ca72229ff5938 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 16 Sep 2025 16:03:54 +0200 Subject: [PATCH 34/39] controller tests: apply PR review patch --- rust/operator-binary/src/controller.rs | 112 ++++++++++++++----------- 1 file changed, 63 insertions(+), 49 deletions(-) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 4e1af1c8..a582c303 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -1814,10 +1814,16 @@ mod tests { use stackable_operator::commons::networking::DomainName; use super::*; - use crate::config::client_protocol::ResolvedClientProtocolConfig; + use crate::{ + config::{ + client_protocol::ResolvedClientProtocolConfig, + fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, + }, + crd::v1alpha1::TrinoCluster, + }; - #[test] - fn test_config_overrides() { + #[tokio::test] + async fn test_config_overrides() { let trino_yaml = r#" apiVersion: trino.stackable.tech/v1alpha1 kind: TrinoCluster @@ -1851,7 +1857,7 @@ mod tests { default: replicas: 1 "#; - let cm = build_config_map(trino_yaml, &None, &None).data.unwrap(); + let cm = build_config_map(trino_yaml).await.data.unwrap(); let config = cm.get("config.properties").unwrap(); assert!(config.contains("foo=bar")); assert!(config.contains("level=role-group")); @@ -1874,8 +1880,8 @@ mod tests { assert!(cm.contains_key("access-control.properties")); } - #[test] - fn test_client_protocol_config_overrides() { + #[tokio::test] + async fn test_client_protocol_config_overrides() { let trino_yaml = r#" apiVersion: trino.stackable.tech/v1alpha1 kind: TrinoCluster @@ -1888,64 +1894,48 @@ mod tests { catalogLabelSelector: matchLabels: trino: simple-trino + clientProtocol: + spooling: + location: s3://my-bucket/spooling + filesystem: + s3: + reference: test-s3-connection coordinators: configOverrides: config.properties: - protocol.spooling.enabled: "false" + foo: bar spooling-manager.properties: - fs.location: s3a://bucket/cluster/spooling + fs.location: s3a://role-level roleGroups: default: + replicas: 1 configOverrides: - config.properties: - protocol.spooling.enabled: "false" spooling-manager.properties: - fs.location: s3a://bucket/cluster/spooling - replicas: 1 + fs.location: s3a://role-group-level workers: roleGroups: default: replicas: 1 "#; - let resolved_spooling_config = Some(ResolvedClientProtocolConfig { - config_properties: BTreeMap::from([ - ("protocol.spooling.enabled".to_string(), "true".to_string()), - ( - "protocol.spooling.shared-secret-key".to_string(), - "test".to_string(), - ), - ]), - spooling_manager_properties: BTreeMap::from([( - "spooling-manager.name".to_string(), - "filesystem".to_string(), - )]), - volume_mounts: vec![], - volumes: vec![], - init_container_extra_start_commands: vec![], - }); - let cm = build_config_map(trino_yaml, &None, &resolved_spooling_config) - .data - .unwrap(); + let cm = build_config_map(trino_yaml).await.data.unwrap(); let config = cm.get("config.properties").unwrap(); - assert!(config.contains("protocol.spooling.enabled=false")); - assert!(config.contains("protocol.spooling.shared-secret-key=test")); + assert!(config.contains("protocol.spooling.enabled=true")); + assert!(config.contains(&format!( + "protocol.spooling.shared-secret-key=${{ENV\\:{ENV_SPOOLING_SECRET}}}" + ))); + assert!(config.contains("foo=bar")); let config = cm.get("spooling-manager.properties").unwrap(); - - assert!(config.contains("fs.location=s3a\\://bucket/cluster/spooling")); + assert!(config.contains("fs.location=s3a\\://role-group-level")); assert!(config.contains("spooling-manager.name=filesystem")); } - fn build_config_map( - trino_yaml: &str, - resolved_fte_config: &Option< - fault_tolerant_execution::ResolvedFaultTolerantExecutionConfig, - >, - resolved_spooling_config: &Option, - ) -> ConfigMap { - let mut trino: v1alpha1::TrinoCluster = - serde_yaml::from_str(trino_yaml).expect("illegal test input"); + async fn build_config_map(trino_yaml: &str) -> ConfigMap { + let deserializer = serde_yaml::Deserializer::from_str(trino_yaml); + let mut trino: TrinoCluster = + serde_yaml::with::singleton_map_recursive::deserialize(deserializer) + .expect("invalid test input"); trino.metadata.namespace = Some("default".to_owned()); trino.metadata.uid = Some("42".to_owned()); let cluster_info = KubernetesClusterInfo { @@ -2029,6 +2019,30 @@ mod tests { ), allow_permission_management_operations: true, }); + let resolved_fte_config = match &trino.spec.cluster_config.fault_tolerant_execution { + Some(fault_tolerant_execution) => Some( + ResolvedFaultTolerantExecutionConfig::from_config( + fault_tolerant_execution, + None, + &trino.namespace().unwrap(), + ) + .await + .unwrap(), + ), + None => None, + }; + let resolved_spooling_config = match &trino.spec.cluster_config.client_protocol { + Some(client_protocol) => Some( + ResolvedClientProtocolConfig::from_config( + client_protocol, + None, + &trino.namespace().unwrap(), + ) + .await + .unwrap(), + ), + None => None, + }; let merged_config = trino .merged_config(&trino_role, &rolegroup_ref, &[]) .unwrap(); @@ -2048,14 +2062,14 @@ mod tests { &trino_authentication_config, &trino_opa_config, &cluster_info, - resolved_fte_config, - resolved_spooling_config, + &resolved_fte_config, + &resolved_spooling_config, ) .unwrap() } - #[test] - fn test_access_control_overrides() { + #[tokio::test] + async fn test_access_control_overrides() { let trino_yaml = r#" apiVersion: trino.stackable.tech/v1alpha1 kind: TrinoCluster @@ -2092,7 +2106,7 @@ mod tests { replicas: 1 "#; - let cm = build_config_map(trino_yaml, &None, &None).data.unwrap(); + let cm = build_config_map(trino_yaml).await.data.unwrap(); let access_control_config = cm.get("access-control.properties").unwrap(); assert!(access_control_config.contains("access-control.name=opa")); From 68809f81b90410c9c64ecae49b1b84ff0fa45292 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Tue, 16 Sep 2025 17:13:41 +0200 Subject: [PATCH 35/39] client-spooling: update schema for docs and tests --- .../trino/pages/usage-guide/client-spooling-protocol.adoc | 3 +-- tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc index 1dda133c..6321aee9 100644 --- a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc +++ b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc @@ -26,8 +26,7 @@ spec: location: "s3://spooling-bucket/trino/" # <1> filesystem: s3: # <2> - connection: - reference: "minio" + reference: "minio" ---- <1> Specifies the location where spooled data will be stored. This example uses an S3 bucket. <2> Configures the filesystem type for spooling. Only S3 is supported currently via the custom resource definition. diff --git a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 index 5608e6cb..39d80c87 100644 --- a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 +++ b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 @@ -21,8 +21,7 @@ spec: location: "s3://spooling-bucket/trino/" filesystem: s3: - connection: - reference: "minio" + reference: "minio" {% if lookup('env', 'VECTOR_AGGREGATOR') %} vectorAggregatorConfigMapName: vector-aggregator-discovery {% endif %} From f7f1e9561d73d2b350982ccc90498dfa021e7fa3 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Wed, 17 Sep 2025 09:36:49 +0200 Subject: [PATCH 36/39] Apply suggestions from code review Co-authored-by: Sebastian Bernauer --- rust/operator-binary/src/config/fault_tolerant_execution.rs | 2 -- rust/operator-binary/src/controller.rs | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/operator-binary/src/config/fault_tolerant_execution.rs b/rust/operator-binary/src/config/fault_tolerant_execution.rs index f9662df3..e2a14465 100644 --- a/rust/operator-binary/src/config/fault_tolerant_execution.rs +++ b/rust/operator-binary/src/config/fault_tolerant_execution.rs @@ -462,8 +462,6 @@ mod tests { reference: test-s3-connection maxErrorRetries: 5 uploadPartSize: 10Mi - iamRole: arn:aws:iam::123456789012:role/TrinoRole - externalId: external-id-123 "#}); let fte_config = diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index a582c303..19fa51f8 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -1550,6 +1550,8 @@ fn shared_spooling_secret_name(trino: &v1alpha1::TrinoCluster) -> String { format!("{}-spooling-secret", trino.name_any()) } +// TODO: Maybe switch to something non-openssl. +// See https://github.com/stackabletech/airflow-operator/pull/686#discussion_r2348354468 (which is currently under discussion) fn get_random_base64(byte_size: usize) -> String { let mut buf: Vec = vec![0; byte_size]; openssl::rand::rand_bytes(&mut buf).unwrap(); From 67945ba67001a3fa5139f41f766ac49461176c13 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Wed, 17 Sep 2025 09:59:58 +0200 Subject: [PATCH 37/39] client spooling: make s3 filesystem consistent with FTE backend --- deploy/helm/trino-operator/crds/crds.yaml | 205 +++++++++--------- .../usage-guide/client-spooling-protocol.adoc | 3 +- .../src/config/client_protocol.rs | 7 +- rust/operator-binary/src/controller.rs | 3 +- .../src/crd/client_protocol.rs | 11 +- .../client-spooling/02-install-trino.yaml.j2 | 3 +- 6 files changed, 126 insertions(+), 106 deletions(-) diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml index 1a47f7bd..71e503e7 100644 --- a/deploy/helm/trino-operator/crds/crds.yaml +++ b/deploy/helm/trino-operator/crds/crds.yaml @@ -120,125 +120,130 @@ spec: - s3 properties: s3: - oneOf: - - required: - - inline - - required: - - reference properties: - inline: - description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). + connection: + oneOf: + - required: + - inline + - required: + - reference properties: - accessStyle: - default: VirtualHosted - description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). - enum: - - Path - - VirtualHosted - type: string - credentials: - description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. - nullable: true + inline: + description: S3 connection definition as a resource. Learn more on the [S3 concept documentation](https://docs.stackable.tech/home/nightly/concepts/s3). properties: - scope: - description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' + accessStyle: + default: VirtualHosted + description: Which access style to use. Defaults to virtual hosted-style as most of the data products out there. Have a look at the [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html). + enum: + - Path + - VirtualHosted + type: string + credentials: + description: If the S3 uses authentication you have to specify you S3 credentials. In the most cases a [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) providing `accessKey` and `secretKey` is sufficient. nullable: true properties: - listenerVolumes: - default: [] - description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. - items: - type: string - type: array - node: - default: false - description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. - type: boolean - pod: - default: false - description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. - type: boolean - services: - default: [] - description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. - items: - type: string - type: array + scope: + description: '[Scope](https://docs.stackable.tech/home/nightly/secret-operator/scope) of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass).' + nullable: true + properties: + listenerVolumes: + default: [] + description: The listener volume scope allows Node and Service scopes to be inferred from the applicable listeners. This must correspond to Volume names in the Pod that mount Listeners. + items: + type: string + type: array + node: + default: false + description: The node scope is resolved to the name of the Kubernetes Node object that the Pod is running on. This will typically be the DNS name of the node. + type: boolean + pod: + default: false + description: The pod scope is resolved to the name of the Kubernetes Pod. This allows the secret to differentiate between StatefulSet replicas. + type: boolean + services: + default: [] + description: The service scope allows Pod objects to specify custom scopes. This should typically correspond to Service objects that the Pod participates in. + items: + type: string + type: array + type: object + secretClass: + description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' + type: string + required: + - secretClass type: object - secretClass: - description: '[SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) containing the LDAP bind credentials.' + host: + description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' type: string - required: - - secretClass - type: object - host: - description: 'Host of the S3 server without any protocol or port. For example: `west1.my-cloud.com`.' - type: string - port: - description: Port the S3 server listens on. If not specified the product will determine the port to use. - format: uint16 - minimum: 0.0 - nullable: true - type: integer - region: - default: - name: us-east-1 - description: |- - Bucket region used for signing headers (sigv4). + port: + description: Port the S3 server listens on. If not specified the product will determine the port to use. + format: uint16 + minimum: 0.0 + nullable: true + type: integer + region: + default: + name: us-east-1 + description: |- + Bucket region used for signing headers (sigv4). - This defaults to `us-east-1` which is compatible with other implementations such as Minio. + This defaults to `us-east-1` which is compatible with other implementations such as Minio. - WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. - properties: - name: - default: us-east-1 - type: string - type: object - tls: - description: Use a TLS connection. If not specified no TLS will be used. - nullable: true - properties: - verification: - description: The verification method used to verify the certificates of the server and/or the client. - oneOf: - - required: - - none - - required: - - server + WARNING: Some products use the Hadoop S3 implementation which falls back to us-east-2. properties: - none: - description: Use TLS but don't verify certificates. - type: object - server: - description: Use TLS and a CA certificate to verify the server. + name: + default: us-east-1 + type: string + type: object + tls: + description: Use a TLS connection. If not specified no TLS will be used. + nullable: true + properties: + verification: + description: The verification method used to verify the certificates of the server and/or the client. + oneOf: + - required: + - none + - required: + - server properties: - caCert: - description: CA cert to verify the server. - oneOf: - - required: - - webPki - - required: - - secretClass + none: + description: Use TLS but don't verify certificates. + type: object + server: + description: Use TLS and a CA certificate to verify the server. properties: - secretClass: - description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. - type: string - webPki: - description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. + caCert: + description: CA cert to verify the server. + oneOf: + - required: + - webPki + - required: + - secretClass + properties: + secretClass: + description: Name of the [SecretClass](https://docs.stackable.tech/home/nightly/secret-operator/secretclass) which will provide the CA certificate. Note that a SecretClass does not need to have a key but can also work with just a CA certificate, so if you got provided with a CA cert but don't have access to the key you can still use this method. + type: string + webPki: + description: Use TLS and the CA certificates trusted by the common web browsers to verify the server. This can be useful when you e.g. use public AWS S3 or other public available services. + type: object type: object + required: + - caCert type: object - required: - - caCert type: object + required: + - verification type: object required: - - verification + - host type: object - required: - - host + reference: + type: string type: object - reference: - type: string + required: + - connection type: object type: object location: diff --git a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc index 6321aee9..1dda133c 100644 --- a/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc +++ b/docs/modules/trino/pages/usage-guide/client-spooling-protocol.adoc @@ -26,7 +26,8 @@ spec: location: "s3://spooling-bucket/trino/" # <1> filesystem: s3: # <2> - reference: "minio" + connection: + reference: "minio" ---- <1> Specifies the location where spooled data will be stored. This example uses an S3 bucket. <2> Configures the filesystem type for spooling. Only S3 is supported currently via the custom resource definition. diff --git a/rust/operator-binary/src/config/client_protocol.rs b/rust/operator-binary/src/config/client_protocol.rs index 6e4822b8..85543ef1 100644 --- a/rust/operator-binary/src/config/client_protocol.rs +++ b/rust/operator-binary/src/config/client_protocol.rs @@ -73,7 +73,9 @@ impl ResolvedClientProtocolConfig { match &spooling_config.filesystem { SpoolingFileSystemConfig::S3(s3_config) => { let resolved_s3_config = config::s3::ResolvedS3Config::from_config( - s3_config, client, namespace, + &s3_config.connection, + client, + namespace, ) .await .context(ResolveS3ConnectionSnafu)?; @@ -132,7 +134,8 @@ mod tests { location: s3://my-bucket/spooling filesystem: s3: - reference: test-s3-connection + connection: + reference: test-s3-connection "#}; let deserializer = serde_yaml::Deserializer::from_str(config_yaml); diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 19fa51f8..f8c95eac 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -1901,7 +1901,8 @@ mod tests { location: s3://my-bucket/spooling filesystem: s3: - reference: test-s3-connection + connection: + reference: test-s3-connection coordinators: configOverrides: config.properties: diff --git a/rust/operator-binary/src/crd/client_protocol.rs b/rust/operator-binary/src/crd/client_protocol.rs index b5bc6c54..19d4dcae 100644 --- a/rust/operator-binary/src/crd/client_protocol.rs +++ b/rust/operator-binary/src/crd/client_protocol.rs @@ -22,5 +22,14 @@ pub struct ClientSpoolingProtocolConfig { #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub enum SpoolingFileSystemConfig { - S3(stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference), + S3(S3FilesystemConfig), +} + +// This adds a `connection` property to keep the structure consistent with the fault-tolerant execution +// config. It is similar to the `crate::crd::fault_tolerant_execution::S3ExchangeConfig` and maybe +// these two structures can be merged in the future. +#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct S3FilesystemConfig { + pub connection: stackable_operator::crd::s3::v1alpha1::InlineConnectionOrReference, } diff --git a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 index 39d80c87..5608e6cb 100644 --- a/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 +++ b/tests/templates/kuttl/client-spooling/02-install-trino.yaml.j2 @@ -21,7 +21,8 @@ spec: location: "s3://spooling-bucket/trino/" filesystem: s3: - reference: "minio" + connection: + reference: "minio" {% if lookup('env', 'VECTOR_AGGREGATOR') %} vectorAggregatorConfigMapName: vector-aggregator-discovery {% endif %} From 35e7f2cb3dba632ca3b007a4bc1f82089794a06b Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Wed, 17 Sep 2025 10:11:56 +0200 Subject: [PATCH 38/39] ensure error message are lowercased --- rust/operator-binary/src/authentication/mod.rs | 10 +++++----- rust/operator-binary/src/authentication/oidc/mod.rs | 10 +++++----- .../src/authentication/password/ldap.rs | 6 +++--- rust/operator-binary/src/catalog/mod.rs | 2 +- rust/operator-binary/src/config/client_protocol.rs | 2 +- .../src/config/fault_tolerant_execution.rs | 4 ++-- rust/operator-binary/src/config/s3.rs | 2 +- rust/operator-binary/src/controller.rs | 8 ++++---- rust/operator-binary/src/crd/authentication.rs | 4 ++-- .../src/operations/graceful_shutdown.rs | 2 +- rust/operator-binary/src/operations/pdb.rs | 4 ++-- 11 files changed, 27 insertions(+), 27 deletions(-) diff --git a/rust/operator-binary/src/authentication/mod.rs b/rust/operator-binary/src/authentication/mod.rs index 04bb317a..fd91436f 100644 --- a/rust/operator-binary/src/authentication/mod.rs +++ b/rust/operator-binary/src/authentication/mod.rs @@ -43,26 +43,26 @@ const HTTP_SERVER_AUTHENTICATION_TYPE: &str = "http-server.authentication.type"; #[derive(Snafu, Debug)] pub enum Error { #[snafu(display( - "The Trino Operator does not support the AuthenticationClass provider [{authentication_class_provider}] from AuthenticationClass [{authentication_class}]." + "the Trino Operator does not support the AuthenticationClass provider [{authentication_class_provider}] from AuthenticationClass [{authentication_class}]." ))] AuthenticationClassProviderNotSupported { authentication_class_provider: String, authentication_class: ObjectRef, }, - #[snafu(display("Failed to format trino authentication java properties"))] + #[snafu(display("failed to format trino authentication java properties"))] FailedToWriteJavaProperties { source: product_config::writer::PropertiesWriterError, }, - #[snafu(display("Failed to configure trino password authentication"))] + #[snafu(display("failed to configure trino password authentication"))] InvalidPasswordAuthenticationConfig { source: password::Error }, - #[snafu(display("Failed to configure trino OAuth2 authentication"))] + #[snafu(display("failed to configure trino OAuth2 authentication"))] InvalidOauth2AuthenticationConfig { source: oidc::Error }, #[snafu(display( - "OIDC authentication details not specified. The AuthenticationClass {auth_class_name:?} uses an OIDC provider, you need to specify OIDC authentication details (such as client credentials) as well" + "oidc authentication details not specified. The AuthenticationClass {auth_class_name:?} uses an OIDC provider, you need to specify OIDC authentication details (such as client credentials) as well" ))] OidcAuthenticationDetailsNotSpecified { auth_class_name: String }, diff --git a/rust/operator-binary/src/authentication/oidc/mod.rs b/rust/operator-binary/src/authentication/oidc/mod.rs index 181a9358..7d3615f5 100644 --- a/rust/operator-binary/src/authentication/oidc/mod.rs +++ b/rust/operator-binary/src/authentication/oidc/mod.rs @@ -28,12 +28,12 @@ const WEB_UI_AUTHENTICATION_TYPE: &str = "web-ui.authentication.type"; #[derive(Snafu, Debug)] pub enum Error { #[snafu(display( - "No OAuth2 AuthenticationClass provided. This is an internal operator failure and should not be happening." + "no OAuth2 AuthenticationClass provided. This is an internal operator failure and should not be happening." ))] NoOauth2AuthenticationClassProvided, #[snafu(display( - "Trino cannot configure OAuth2 with multiple Identity providers. \ + "trino cannot configure OAuth2 with multiple Identity providers. \ Received the following AuthenticationClasses {authentication_class_names:?}. \ Please only provide one OAuth2 AuthenticationClass!" ))] @@ -41,15 +41,15 @@ pub enum Error { authentication_class_names: Vec, }, - #[snafu(display("Failed to create OAuth2 issuer endpoint url."))] + #[snafu(display("failed to create OAuth2 issuer endpoint url."))] FailedToCreateIssuerEndpointUrl { source: stackable_operator::crd::authentication::oidc::v1alpha1::Error, }, - #[snafu(display("Trino does not support unverified TLS connections to OIDC"))] + #[snafu(display("trino does not support unverified TLS connections to OIDC"))] UnverifiedOidcTlsConnectionNotSupported, - #[snafu(display("Failed to create OIDC Volumes and VolumeMounts"))] + #[snafu(display("failed to create OIDC Volumes and VolumeMounts"))] FailedToCreateOidcVolumeAndVolumeMounts { source: TlsClientDetailsError }, } diff --git a/rust/operator-binary/src/authentication/password/ldap.rs b/rust/operator-binary/src/authentication/password/ldap.rs index 84a31a1a..671b4a10 100644 --- a/rust/operator-binary/src/authentication/password/ldap.rs +++ b/rust/operator-binary/src/authentication/password/ldap.rs @@ -22,15 +22,15 @@ const LDAP_PASSWORD_ENV: &str = "LDAP_PASSWORD"; #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("Trino does not support unverified TLS connections to LDAP"))] + #[snafu(display("trino does not support unverified TLS connections to LDAP"))] UnverifiedLdapTlsConnectionNotSupported, - #[snafu(display("Failed to construct LDAP endpoint URL"))] + #[snafu(display("failed to construct LDAP endpoint URL"))] LdapEndpoint { source: stackable_operator::crd::authentication::ldap::v1alpha1::Error, }, - #[snafu(display("Failed to construct LDAP Volumes and VolumeMounts"))] + #[snafu(display("failed to construct LDAP Volumes and VolumeMounts"))] LdapVolumeAndVolumeMounts { source: stackable_operator::crd::authentication::ldap::v1alpha1::Error, }, diff --git a/rust/operator-binary/src/catalog/mod.rs b/rust/operator-binary/src/catalog/mod.rs index 0087c0c4..63c670a6 100644 --- a/rust/operator-binary/src/catalog/mod.rs +++ b/rust/operator-binary/src/catalog/mod.rs @@ -57,7 +57,7 @@ pub enum FromTrinoCatalogError { data_key: String, }, - #[snafu(display("Failed to create the Secret Volume for the S3 credentials"))] + #[snafu(display("failed to create the Secret Volume for the S3 credentials"))] CreateS3CredentialsSecretOperatorVolume { source: stackable_operator::builder::pod::volume::SecretOperatorVolumeSourceBuilderError, }, diff --git a/rust/operator-binary/src/config/client_protocol.rs b/rust/operator-binary/src/config/client_protocol.rs index 85543ef1..96354c21 100644 --- a/rust/operator-binary/src/config/client_protocol.rs +++ b/rust/operator-binary/src/config/client_protocol.rs @@ -18,7 +18,7 @@ use crate::{ #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("Failed to resolve S3 connection"))] + #[snafu(display("failed to resolve S3 connection"))] ResolveS3Connection { source: config::s3::Error }, #[snafu(display("trino does not support disabling the TLS verification of S3 servers"))] diff --git a/rust/operator-binary/src/config/fault_tolerant_execution.rs b/rust/operator-binary/src/config/fault_tolerant_execution.rs index e2a14465..852f15c3 100644 --- a/rust/operator-binary/src/config/fault_tolerant_execution.rs +++ b/rust/operator-binary/src/config/fault_tolerant_execution.rs @@ -23,12 +23,12 @@ use crate::{ #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("Failed to resolve S3 connection"))] + #[snafu(display("failed to resolve S3 connection"))] S3Connection { source: s3::v1alpha1::ConnectionError, }, - #[snafu(display("Failed to resolve S3 connection"))] + #[snafu(display("failed to resolve S3 connection"))] ResolveS3Connection { source: config::s3::Error }, #[snafu(display("trino does not support disabling the TLS verification of S3 servers"))] diff --git a/rust/operator-binary/src/config/s3.rs b/rust/operator-binary/src/config/s3.rs index 4df59553..dedcbcd6 100644 --- a/rust/operator-binary/src/config/s3.rs +++ b/rust/operator-binary/src/config/s3.rs @@ -12,7 +12,7 @@ use crate::{command, crd::STACKABLE_CLIENT_TLS_DIR}; #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("Failed to resolve S3 connection"))] + #[snafu(display("failed to resolve S3 connection"))] S3Connection { source: s3::v1alpha1::ConnectionError, }, diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index f8c95eac..0febcaf1 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -133,7 +133,7 @@ pub enum Error { #[snafu(display("object defines no namespace"))] ObjectHasNoNamespace, - #[snafu(display("Trino cluster {name:?} has no namespace"))] + #[snafu(display("trino cluster {name:?} has no namespace"))] MissingTrinoNamespace { source: crate::crd::Error, name: String, @@ -278,17 +278,17 @@ pub enum Error { source: stackable_operator::commons::rbac::Error, }, - #[snafu(display("Failed to retrieve AuthenticationClass"))] + #[snafu(display("failed to retrieve AuthenticationClass"))] AuthenticationClassRetrieval { source: crate::crd::authentication::Error, }, - #[snafu(display("Unsupported Trino authentication"))] + #[snafu(display("unsupported Trino authentication"))] UnsupportedAuthenticationConfig { source: crate::authentication::Error, }, - #[snafu(display("Invalid Trino authentication"))] + #[snafu(display("invalid Trino authentication"))] InvalidAuthenticationConfig { source: crate::authentication::Error, }, diff --git a/rust/operator-binary/src/crd/authentication.rs b/rust/operator-binary/src/crd/authentication.rs index 8c6fd227..f7b31262 100644 --- a/rust/operator-binary/src/crd/authentication.rs +++ b/rust/operator-binary/src/crd/authentication.rs @@ -7,12 +7,12 @@ use stackable_operator::{ #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("Failed to retrieve AuthenticationClass"))] + #[snafu(display("failed to retrieve AuthenticationClass"))] AuthenticationClassRetrieval { source: stackable_operator::client::Error, }, - #[snafu(display("Invalid OIDC configuration"))] + #[snafu(display("invalid OIDC configuration"))] InvalidOidcConfiguration { source: stackable_operator::crd::authentication::core::v1alpha1::Error, }, diff --git a/rust/operator-binary/src/operations/graceful_shutdown.rs b/rust/operator-binary/src/operations/graceful_shutdown.rs index 5b00bbab..50bad7ae 100644 --- a/rust/operator-binary/src/operations/graceful_shutdown.rs +++ b/rust/operator-binary/src/operations/graceful_shutdown.rs @@ -15,7 +15,7 @@ use crate::crd::{ #[derive(Debug, Snafu)] pub enum Error { - #[snafu(display("Failed to set terminationGracePeriod"))] + #[snafu(display("failed to set terminationGracePeriod"))] SetTerminationGracePeriod { source: stackable_operator::builder::pod::Error, }, diff --git a/rust/operator-binary/src/operations/pdb.rs b/rust/operator-binary/src/operations/pdb.rs index bee6bac0..4f8a7bb7 100644 --- a/rust/operator-binary/src/operations/pdb.rs +++ b/rust/operator-binary/src/operations/pdb.rs @@ -13,13 +13,13 @@ use crate::{ #[derive(Snafu, Debug)] pub enum Error { - #[snafu(display("Cannot create PodDisruptionBudget for role [{role}]"))] + #[snafu(display("cannot create PodDisruptionBudget for role [{role}]"))] CreatePdb { source: stackable_operator::builder::pdb::Error, role: String, }, - #[snafu(display("Cannot apply PodDisruptionBudget [{name}]"))] + #[snafu(display("cannot apply PodDisruptionBudget [{name}]"))] ApplyPdb { source: stackable_operator::cluster_resources::Error, name: String, From ae1dc26c6b82000d4400accbc9d343a4a28004c9 Mon Sep 17 00:00:00 2001 From: Razvan-Daniel Mihai <84674+razvan@users.noreply.github.com> Date: Wed, 17 Sep 2025 10:27:33 +0200 Subject: [PATCH 39/39] client spooling: raise error for trino 451 --- rust/operator-binary/src/controller.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 0febcaf1..d07a817b 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -383,6 +383,11 @@ pub enum Error { #[snafu(display("failed to resolve client protocol configuration"))] ClientProtocolConfiguration { source: client_protocol::Error }, + + #[snafu(display( + "client spooling protocol is not supported for Trino version {product_version}" + ))] + ClientSpoolingProtocolTrinoVersion { product_version: String }, } type Result = std::result::Result; @@ -480,6 +485,13 @@ pub async fn reconcile_trino( ), None => None, }; + if resolved_client_protocol_config.is_some() + && resolved_product_image.product_version.starts_with("45") + { + return Err(Error::ClientSpoolingProtocolTrinoVersion { + product_version: resolved_product_image.product_version, + }); + } let validated_config = validated_product_config( trino,