From 0277b91ca4de674e4803ba9d39385e8fba8f86c2 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 17:45:07 +0200 Subject: [PATCH 1/3] give each role its own validated config instead of a common one --- rust/operator-binary/src/controller/build.rs | 38 +-- .../src/controller/build/graceful_shutdown.rs | 15 +- .../properties/access_control_properties.rs | 10 +- .../build/properties/config_properties.rs | 6 +- .../properties/exchange_manager_properties.rs | 10 +- .../build/properties/log_properties.rs | 10 +- .../build/properties/node_properties.rs | 3 +- .../build/properties/security_properties.rs | 10 +- .../properties/spooling_manager_properties.rs | 10 +- .../controller/build/resource/config_map.rs | 11 +- .../src/controller/build/resource/pdb.rs | 6 +- .../controller/build/resource/statefulset.rs | 5 +- rust/operator-binary/src/controller/mod.rs | 68 ++++-- .../src/controller/validate.rs | 217 +++++++++++------- rust/operator-binary/src/crd/mod.rs | 34 --- rust/operator-binary/src/trino_controller.rs | 3 +- 16 files changed, 245 insertions(+), 211 deletions(-) diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index d6f8b1c5..5fa652fa 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -29,6 +29,7 @@ use crate::{ statefulset, }, }, + crd::TrinoRole, trino_controller::{CONTROLLER_NAME, OPERATOR_NAME, PRODUCT_NAME}, }; @@ -68,7 +69,16 @@ pub fn build( let mut config_maps = vec![]; let mut pod_disruption_budgets = vec![]; - for (role, role_group_configs) in &cluster.role_group_configs { + // One entry per role, in `TrinoRole` declaration order. Each role's groups come from its own + // field, so the role and its groups cannot be paired up wrongly here. + for (role, role_group_configs) in [ + ( + TrinoRole::Coordinator, + &cluster.coordinator_role_group_configs, + ), + (TrinoRole::Worker, &cluster.worker_role_group_configs), + ] { + let role: &TrinoRole = &role; for (role_group_name, role_group_config) in role_group_configs { let selector = role_group_selector(cluster, role, role_group_name); @@ -115,22 +125,18 @@ pub fn build( ); } - let Some(role_config) = cluster.role_config(role) else { - continue; - }; - - if let Some(listener_class) = &role_config.listener_class - && let Some(listener_group_name) = group_listener_name(cluster, role) - { - listeners.push(build_group_listener( - cluster, - role, - listener_class, - &listener_group_name, - )); - } + pod_disruption_budgets.extend(build_pdb(cluster.pdb(role), cluster, role)); + } - pod_disruption_budgets.extend(build_pdb(&role_config.pdb, cluster, role)); + // Only the coordinator has a group listener, so this is not inside the loop above asking each + // role whether it happens to have a listener class. + if let Some(listener_group_name) = group_listener_name(cluster, &TrinoRole::Coordinator) { + listeners.push(build_group_listener( + cluster, + &TrinoRole::Coordinator, + &cluster.coordinator_config.listener_class, + &listener_group_name, + )); } Ok(KubernetesResources { diff --git a/rust/operator-binary/src/controller/build/graceful_shutdown.rs b/rust/operator-binary/src/controller/build/graceful_shutdown.rs index 3f000bd2..1619aa7e 100644 --- a/rust/operator-binary/src/controller/build/graceful_shutdown.rs +++ b/rust/operator-binary/src/controller/build/graceful_shutdown.rs @@ -62,16 +62,13 @@ pub fn graceful_shutdown_config_properties( } } -/// Returns the minimal `gracefulShutdownTimeout` across all worker role-groups, read from the -/// validated [`ValidatedCluster::role_group_configs`]. +/// Returns the minimal `gracefulShutdownTimeout` across all worker role-groups fn min_worker_graceful_shutdown_timeout( cluster: &ValidatedCluster, ) -> stackable_operator::shared::time::Duration { cluster - .role_group_configs - .get(&TrinoRole::Worker) - .into_iter() - .flat_map(|groups| groups.values()) + .worker_role_group_configs + .values() .filter_map(|rg| rg.config.graceful_shutdown_timeout) .min() .unwrap_or(DEFAULT_WORKER_GRACEFUL_SHUTDOWN_TIMEOUT) @@ -309,7 +306,8 @@ mod tests { #[test] fn worker_termination_grace_period_adds_overhead_and_sets_pre_stop() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let merged = &cluster.role_group_configs[&TrinoRole::Worker] + let merged = &cluster + .worker_role_group_configs .values() .next() .expect("the fixture defines a worker role group") @@ -348,7 +346,8 @@ mod tests { #[test] fn coordinator_termination_grace_period_has_no_overhead_or_pre_stop() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let merged = &cluster.role_group_configs[&TrinoRole::Coordinator] + let merged = &cluster + .coordinator_role_group_configs .values() .next() .expect("the fixture defines a coordinator role group") diff --git a/rust/operator-binary/src/controller/build/properties/access_control_properties.rs b/rust/operator-binary/src/controller/build/properties/access_control_properties.rs index 0c7e47b8..00d2d872 100644 --- a/rust/operator-binary/src/controller/build/properties/access_control_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/access_control_properties.rs @@ -27,17 +27,15 @@ pub fn build(cluster: &ValidatedCluster, rg: &TrinoRoleGroupConfig) -> BTreeMap< #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; #[test] fn default_renders_empty_when_no_opa() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/config_properties.rs b/rust/operator-binary/src/controller/build/properties/config_properties.rs index c8754d7d..c2cb5529 100644 --- a/rust/operator-binary/src/controller/build/properties/config_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/config_properties.rs @@ -312,7 +312,8 @@ mod tests { } fn rg(cluster: &ValidatedCluster, role: &TrinoRole) -> TrinoRoleGroupConfig { - cluster.role_group_configs[role] + cluster + .role_group_configs(role) .values() .next() .expect("the fixture defines a role group") @@ -322,7 +323,8 @@ mod tests { #[test] fn default_renders_includes_coordinator_default_and_query_max_memory_default() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/exchange_manager_properties.rs b/rust/operator-binary/src/controller/build/properties/exchange_manager_properties.rs index 337e2175..bd4e8ada 100644 --- a/rust/operator-binary/src/controller/build/properties/exchange_manager_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/exchange_manager_properties.rs @@ -27,17 +27,15 @@ pub fn build(cluster: &ValidatedCluster, rg: &TrinoRoleGroupConfig) -> BTreeMap< #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; #[test] fn default_renders_empty_when_no_fte() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/log_properties.rs b/rust/operator-binary/src/controller/build/properties/log_properties.rs index 6c5831b8..5241a7a5 100644 --- a/rust/operator-binary/src/controller/build/properties/log_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/log_properties.rs @@ -29,17 +29,15 @@ pub fn build(rg: &TrinoRoleGroupConfig) -> BTreeMap { #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; #[test] fn default_renders_root_logger_only() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/node_properties.rs b/rust/operator-binary/src/controller/build/properties/node_properties.rs index 2046b56d..96d1d4ac 100644 --- a/rust/operator-binary/src/controller/build/properties/node_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/node_properties.rs @@ -36,7 +36,8 @@ mod tests { #[test] fn default_renders_node_environment_from_cluster_name() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&crate::crd::TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/security_properties.rs b/rust/operator-binary/src/controller/build/properties/security_properties.rs index cdc01fcb..24e30e34 100644 --- a/rust/operator-binary/src/controller/build/properties/security_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/security_properties.rs @@ -37,17 +37,15 @@ pub fn build(rg: &TrinoRoleGroupConfig) -> BTreeMap { #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; fn coordinator_rg( cluster: &crate::controller::ValidatedCluster, ) -> crate::controller::TrinoRoleGroupConfig { - cluster.role_group_configs[&TrinoRole::Coordinator] + cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/properties/spooling_manager_properties.rs b/rust/operator-binary/src/controller/build/properties/spooling_manager_properties.rs index 94a6d63a..941cd747 100644 --- a/rust/operator-binary/src/controller/build/properties/spooling_manager_properties.rs +++ b/rust/operator-binary/src/controller/build/properties/spooling_manager_properties.rs @@ -27,17 +27,15 @@ pub fn build(cluster: &ValidatedCluster, rg: &TrinoRoleGroupConfig) -> BTreeMap< #[cfg(test)] mod tests { use super::*; - use crate::{ - controller::build::properties::test_support::{ - MINIMAL_TRINO_YAML, validated_cluster_from_yaml, - }, - crd::TrinoRole, + use crate::controller::build::properties::test_support::{ + MINIMAL_TRINO_YAML, validated_cluster_from_yaml, }; #[test] fn default_renders_empty_when_no_spooling() { let cluster = validated_cluster_from_yaml(MINIMAL_TRINO_YAML); - let rg = cluster.role_group_configs[&TrinoRole::Coordinator] + let rg = cluster + .coordinator_role_group_configs .values() .next() .unwrap() diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index af80b8a6..1a2863a5 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -61,15 +61,8 @@ pub fn build_rolegroup_config_map( role_group_name: &RoleGroupName, cluster_info: &KubernetesClusterInfo, ) -> Result { - let role_group_configs = - cluster - .role_group_configs - .get(role) - .with_context(|| MissingRoleGroupSnafu { - role: role.to_string(), - role_group: role_group_name.to_string(), - })?; - let rg = role_group_configs + let rg = cluster + .role_group_configs(role) .get(role_group_name) .with_context(|| MissingRoleGroupSnafu { role: role.to_string(), diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index 68a9ffb7..1b744795 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -47,10 +47,8 @@ pub fn build_pdb( /// contribute nothing, as their size is not known at reconcile time. fn worker_count(cluster: &ValidatedCluster) -> u16 { cluster - .role_group_configs - .get(&TrinoRole::Worker) - .into_iter() - .flat_map(|groups| groups.values()) + .worker_role_group_configs + .values() .filter_map(|rg| rg.replicas) .sum() } diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index a7597d39..6cd36c48 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -807,8 +807,7 @@ mod tests { /// Builds the coordinator `default` role-group StatefulSet for the given cluster. fn build_coordinator_statefulset(cluster: &ValidatedCluster) -> Result { let role_group_name = RoleGroupName::from_str("default").expect("valid role group name"); - let role_group_config = - &cluster.role_group_configs[&TrinoRole::Coordinator][&role_group_name]; + let role_group_config = &cluster.coordinator_role_group_configs[&role_group_name]; build_rolegroup_statefulset( cluster, @@ -851,7 +850,7 @@ mod tests { let cluster = validated_cluster(); let role_group_name = RoleGroupName::from_str("default").expect("valid role group name"); let mut role_group_config = - cluster.role_group_configs[&TrinoRole::Coordinator][&role_group_name].clone(); + cluster.coordinator_role_group_configs[&role_group_name].clone(); role_group_config.env_overrides = EnvVarSet::new().with_value( &EnvVarName::from_str("CONTAINERDEBUG_LOG_DIRECTORY").expect("valid env var name"), "/custom/log/dir", diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 65f171b0..3567420a 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -158,15 +158,26 @@ impl ValidatedTrinoConfig { } } -/// Per-role configuration extracted during validation. +/// The coordinator's validated role-level configuration. /// -/// Lets the reconciler and build steps consume this controller-owned type instead of re-reading -/// the raw [`v1alpha1::TrinoCluster`]. +/// A type of its own rather than a shared one, because the coordinator's role config really is a +/// different type in the CRD ([`v1alpha1::TrinoCoordinatorRoleConfig`] against the worker's +/// `GenericRoleConfig`). Flattening the two into one shape made `listener_class` an `Option` that +/// is mandatory for the coordinator and meaningless for the worker, so every reader had to +/// rediscover which role it was holding. #[derive(Clone, Debug)] -pub struct ValidatedRoleConfig { +pub struct ValidatedCoordinatorRoleConfig { + pub pdb: stackable_operator::commons::pdb::PdbConfig, + /// The listener class of the coordinator's group listener. Not optional: the CRD defaults it. + pub listener_class: ListenerClassName, +} + +/// The worker's validated role-level configuration. +/// +/// Workers have no group listener, so there is no listener class here to be `None`. +#[derive(Clone, Debug)] +pub struct ValidatedWorkerRoleConfig { pub pdb: stackable_operator::commons::pdb::PdbConfig, - /// The listener class for the role's group listener, if it has one (coordinator only). - pub listener_class: Option, } /// The validated TrinoCluster. The output of the validate step. @@ -188,8 +199,14 @@ pub struct ValidatedCluster { /// parsed once from the resolved image's app version label value. pub product_version: ProductVersion, pub cluster_config: ValidatedClusterConfig, - pub role_configs: BTreeMap, - pub role_group_configs: BTreeMap>, + /// The coordinator's role-level config. + pub coordinator_config: ValidatedCoordinatorRoleConfig, + /// The validated config of every coordinator role group, keyed by role group name. + pub coordinator_role_group_configs: BTreeMap, + /// The worker's role-level config. + pub worker_config: ValidatedWorkerRoleConfig, + /// The validated config of every worker role group, keyed by role group name. + pub worker_role_group_configs: BTreeMap, } impl ValidatedCluster { @@ -201,8 +218,10 @@ impl ValidatedCluster { image: ResolvedProductImage, numeric_product_version: u16, cluster_config: ValidatedClusterConfig, - role_configs: BTreeMap, - role_group_configs: BTreeMap>, + coordinator_config: ValidatedCoordinatorRoleConfig, + coordinator_role_group_configs: BTreeMap, + worker_config: ValidatedWorkerRoleConfig, + worker_role_group_configs: BTreeMap, ) -> Self { Self { metadata: ObjectMeta { @@ -219,14 +238,33 @@ impl ValidatedCluster { image, numeric_product_version, cluster_config, - role_configs, - role_group_configs, + coordinator_config, + coordinator_role_group_configs, + worker_config, + worker_role_group_configs, + } + } + + /// The role groups of `role`. + /// + /// A lookup, not a search: both roles always exist, because the CRD makes `coordinators` and + /// `workers` non-optional, so there is no `Option` to unwrap. + pub(crate) fn role_group_configs( + &self, + role: &TrinoRole, + ) -> &BTreeMap { + match role { + TrinoRole::Coordinator => &self.coordinator_role_group_configs, + TrinoRole::Worker => &self.worker_role_group_configs, } } - /// The validated per-role config for `role`, if the role is defined. - pub(crate) fn role_config(&self, role: &TrinoRole) -> Option<&ValidatedRoleConfig> { - self.role_configs.get(role) + /// The PodDisruptionBudget config of `role`. + pub(crate) fn pdb(&self, role: &TrinoRole) -> &stackable_operator::commons::pdb::PdbConfig { + match role { + TrinoRole::Coordinator => &self.coordinator_config.pdb, + TrinoRole::Worker => &self.worker_config.pdb, + } } /// Whether the (client-facing) server TLS is enabled. diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 41580b96..d8a8e0ac 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -12,7 +12,6 @@ use stackable_operator::{ config::fragment, kube::ResourceExt as _, product_logging::spec::Logging, - role_utils::GenericRoleConfig, v2::{ controller_utils::{get_cluster_name, get_namespace, get_uid}, product_logging::framework::{ @@ -23,13 +22,13 @@ use stackable_operator::{ types::kubernetes::ConfigMapName, }, }; -use strum::{EnumDiscriminants, IntoEnumIterator, IntoStaticStr}; +use strum::{EnumDiscriminants, IntoStaticStr}; use crate::{ authentication::{self, TrinoAuthenticationConfig, TrinoAuthenticationTypes}, controller::{ - ValidatedCluster, ValidatedClusterConfig, ValidatedRoleConfig, ValidatedTls, - ValidatedTrinoConfig, dereference::DereferencedObjects, + ValidatedCluster, ValidatedClusterConfig, ValidatedCoordinatorRoleConfig, ValidatedTls, + ValidatedTrinoConfig, ValidatedWorkerRoleConfig, dereference::DereferencedObjects, }, crd::{Container, TrinoRole, catalog::TrinoCatalogName, v1alpha1}, }; @@ -208,56 +207,43 @@ pub fn validate( .vector_aggregator_config_map_name .clone(); - let mut role_configs: BTreeMap = BTreeMap::new(); - let mut role_group_configs: BTreeMap> = - BTreeMap::new(); - for trino_role in TrinoRole::iter() { - let role = trino.role(&trino_role); - - // Extract the per-role PDB and (optional) listener class up-front, so the reconciler and - // build steps consume the validated config instead of re-reading the raw cluster. - role_configs.insert( - trino_role.clone(), - ValidatedRoleConfig { - pdb: trino - .generic_role_config(&trino_role) - .pod_disruption_budget - .clone(), - listener_class: trino_role.listener_class_name(trino), - }, - ); - - let default_config = v1alpha1::TrinoConfig::default_config( - &trino.name_any(), - &trino_role, - &dereferenced_objects.catalog_definitions, - ); - let mut groups = BTreeMap::new(); - for (rg_name, rg) in &role.role_groups { - let role_group_name = - RoleGroupName::from_str(rg_name).with_context(|_| ParseRoleGroupNameSnafu { - role_group: rg_name.clone(), - })?; - // Merges and validates the role group config (default <- role <- role group). Because - // `JavaCommonConfig` implements `Merge`, the role and role-group `jvmArgumentOverrides` - // are merged here too and carried by `product_specific_common_config`. - let merged = with_validated_config::< - v1alpha1::TrinoConfig, - JavaCommonConfig, - v1alpha1::TrinoConfigFragment, - GenericRoleConfig, - v1alpha1::TrinoConfigOverrides, - >(rg, &role, &default_config) - .with_context(|_| FailedToResolveConfigSnafu { - role_group: role_group_name.clone(), - })?; - groups.insert( - role_group_name, - into_role_group_config(merged, &vector_aggregator_config_map_name)?, - ); - } - role_group_configs.insert(trino_role, groups); - } + // Each role's role groups are validated from the role's own CRD type. `validate_role_groups` + // is generic over the role config, so the coordinator no longer has to be converted down to + // the worker's shape first. + // + // Validated in `TrinoRole` declaration order, as the `TrinoRole::iter()` loop this replaced + // was: the first role that fails is the error the user sees, so swapping these two calls + // changes which misconfiguration gets reported when both roles are wrong. + let coordinator_role_group_configs = validate_role_groups( + &trino.spec.coordinators, + trino, + &TrinoRole::Coordinator, + dereferenced_objects, + &vector_aggregator_config_map_name, + )?; + let worker_role_group_configs = validate_role_groups( + &trino.spec.workers, + trino, + &TrinoRole::Worker, + dereferenced_objects, + &vector_aggregator_config_map_name, + )?; + + // Read from each role's own role config, so the coordinator's mandatory `listener_class` stays + // mandatory instead of becoming an `Option` that the worker leaves `None`. + let coordinator_config = ValidatedCoordinatorRoleConfig { + pdb: trino + .spec + .coordinators + .role_config + .common + .pod_disruption_budget + .clone(), + listener_class: trino.spec.coordinators.role_config.listener_class.clone(), + }; + let worker_config = ValidatedWorkerRoleConfig { + pdb: trino.spec.workers.role_config.pod_disruption_budget.clone(), + }; let mut catalogs = BTreeMap::new(); for catalog in &dereferenced_objects.catalogs { @@ -296,13 +282,69 @@ pub fn validate( image, numeric_product_version, cluster_config, - role_configs, - role_group_configs, + coordinator_config, + coordinator_role_group_configs, + worker_config, + worker_role_group_configs, )) } /// Adapts the validated [`RoleGroup`] produced by [`with_validated_config`] into the flattened /// [`TrinoRoleGroupConfig`] consumed by the build steps. +/// Validates every role group of one role, merging default <- role <- role group. +/// +/// Generic over the role's `RoleConfig` so that each role is read from its own CRD type: the +/// coordinator's [`v1alpha1::TrinoCoordinatorRoleConfig`] no longer has to be converted down to +/// the worker's `GenericRoleConfig` before its role groups can be validated. +fn validate_role_groups( + role: &stackable_operator::v2::role_utils::Role< + v1alpha1::TrinoConfigFragment, + v1alpha1::TrinoConfigOverrides, + RoleConfig, + JavaCommonConfig, + >, + trino: &v1alpha1::TrinoCluster, + trino_role: &TrinoRole, + dereferenced_objects: &DereferencedObjects, + vector_aggregator_config_map_name: &Option, +) -> Result> +where + RoleConfig: Default + stackable_operator::schemars::JsonSchema + serde::Serialize, +{ + let default_config = v1alpha1::TrinoConfig::default_config( + &trino.name_any(), + trino_role, + &dereferenced_objects.catalog_definitions, + ); + + let mut role_groups = BTreeMap::new(); + for (rg_name, rg) in &role.role_groups { + let role_group_name = + RoleGroupName::from_str(rg_name).with_context(|_| ParseRoleGroupNameSnafu { + role_group: rg_name.clone(), + })?; + // Merges and validates the role group config (default <- role <- role group). Because + // `JavaCommonConfig` implements `Merge`, the role and role-group `jvmArgumentOverrides` + // are merged here too and carried by `product_specific_common_config`. + let merged = with_validated_config::< + v1alpha1::TrinoConfig, + JavaCommonConfig, + v1alpha1::TrinoConfigFragment, + RoleConfig, + v1alpha1::TrinoConfigOverrides, + >(rg, role, &default_config) + .with_context(|_| FailedToResolveConfigSnafu { + role_group: role_group_name.clone(), + })?; + role_groups.insert( + role_group_name, + into_role_group_config(merged, vector_aggregator_config_map_name)?, + ); + } + + Ok(role_groups) +} + fn into_role_group_config( merged: RoleGroup, vector_aggregator_config_map_name: &Option, @@ -334,24 +376,30 @@ pub(crate) fn merged_role_group_config( role_group: &str, trino_catalogs: &[crate::crd::catalog::v1alpha1::TrinoCatalog], ) -> TrinoRoleGroupConfig { - let role = trino.role(trino_role); - let default_config = - v1alpha1::TrinoConfig::default_config(&trino.name_any(), trino_role, trino_catalogs); - let rg = role - .role_groups - .get(role_group) - .expect("role group should be defined"); - let merged = with_validated_config::< - v1alpha1::TrinoConfig, - JavaCommonConfig, - v1alpha1::TrinoConfigFragment, - GenericRoleConfig, - v1alpha1::TrinoConfigOverrides, - >(rg, &role, &default_config) - .expect("role group config should be valid"); // The shared test clusters do not enable the Vector agent, so no aggregator ConfigMap name is // required here. - into_role_group_config(merged, &None).expect("env overrides should be valid") + let derefs = DereferencedObjects { + catalog_definitions: trino_catalogs.to_vec(), + resolved_authentication_classes: Vec::new(), + catalogs: Vec::new(), + trino_opa_config: None, + resolved_fte_config: None, + resolved_client_protocol_config: None, + }; + let role_groups = match trino_role { + TrinoRole::Coordinator => { + validate_role_groups(&trino.spec.coordinators, trino, trino_role, &derefs, &None) + } + TrinoRole::Worker => { + validate_role_groups(&trino.spec.workers, trino, trino_role, &derefs, &None) + } + } + .expect("role group config should be valid"); + + role_groups + .get(&RoleGroupName::from_str(role_group).expect("valid role group name")) + .expect("role group should be defined") + .clone() } #[cfg(test)] @@ -479,29 +527,22 @@ mod tests { "simple-trino-coordinator-default-0" ); - // Per-role configs: default (enabled) PDBs; only the coordinator has a group listener. - let roles: Vec<_> = validated.role_configs.keys().collect(); - assert_eq!(roles, [&TrinoRole::Coordinator, &TrinoRole::Worker]); - for role_config in validated.role_configs.values() { - assert!(role_config.pdb.enabled); - assert_eq!(role_config.pdb.max_unavailable, None); + // Per-role configs: default (enabled) PDBs. The listener class is on the coordinator's + // config only — the worker type has no such field to assert `None` against. + for role in [TrinoRole::Coordinator, TrinoRole::Worker] { + let pdb = validated.pdb(&role); + assert!(pdb.enabled); + assert_eq!(pdb.max_unavailable, None); } assert_eq!( - validated.role_configs[&TrinoRole::Coordinator] - .listener_class - .as_ref() - .map(ToString::to_string), - Some("cluster-internal".to_string()) - ); - assert_eq!( - validated.role_configs[&TrinoRole::Worker].listener_class, - None + validated.coordinator_config.listener_class.to_string(), + "cluster-internal" ); // One `default` role group per role; the Vector agent is off. let default_rg = RoleGroupName::from_str("default").expect("valid role group name"); for role in [TrinoRole::Coordinator, TrinoRole::Worker] { - let role_group = &validated.role_group_configs[&role][&default_rg]; + let role_group = &validated.role_group_configs(&role)[&default_rg]; assert_eq!(role_group.replicas, Some(1)); assert!(!role_group.config.logging.enable_vector_agent); assert_eq!(role_group.config.logging.vector_container, None); diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index b6529bd4..4a077f4e 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -426,15 +426,6 @@ impl From<&TrinoRole> for RoleName { } } -impl TrinoRole { - pub fn listener_class_name(&self, trino: &v1alpha1::TrinoCluster) -> Option { - match self { - Self::Coordinator => Some(trino.spec.coordinators.role_config.listener_class.clone()), - Self::Worker => None, - } - } -} - #[derive( Clone, Debug, @@ -526,23 +517,6 @@ impl v1alpha1::TrinoConfig { } impl v1alpha1::TrinoCluster { - /// Returns the given role (both roles are required by the CRD). - pub fn role(&self, role_variant: &TrinoRole) -> TrinoRoleType { - match role_variant { - TrinoRole::Coordinator => { - extract_role_from_coordinator_config(self.spec.coordinators.to_owned()) - } - TrinoRole::Worker => self.spec.workers.to_owned(), - } - } - - pub fn generic_role_config(&self, role: &TrinoRole) -> &GenericRoleConfig { - match role { - TrinoRole::Coordinator => &self.spec.coordinators.role_config.common, - TrinoRole::Worker => &self.spec.workers.role_config, - } - } - /// List all coordinator pods expected to form the cluster /// /// We try to predict the pods here rather than looking at the current cluster state in order to @@ -598,14 +572,6 @@ impl v1alpha1::TrinoCluster { /// Converts the coordinator role (which carries the coordinator-specific `role_config`) into the /// generic [`TrinoRoleType`]. Only the `role_config` type parameter differs between the two; the /// `config` and `role_groups` carry over unchanged. -fn extract_role_from_coordinator_config(fragment: TrinoCoordinatorRoleType) -> TrinoRoleType { - Role { - config: fragment.config, - role_config: fragment.role_config.common, - role_groups: fragment.role_groups, - } -} - impl HasStatusCondition for v1alpha1::TrinoCluster { fn conditions(&self) -> Vec { match &self.status { diff --git a/rust/operator-binary/src/trino_controller.rs b/rust/operator-binary/src/trino_controller.rs index ca633904..9f8f92a7 100644 --- a/rust/operator-binary/src/trino_controller.rs +++ b/rust/operator-binary/src/trino_controller.rs @@ -485,7 +485,8 @@ mod tests { let validated_cluster = validate::validate(&trino, &derefs, &operator_env).expect("validate should succeed"); - let env = &validated_cluster.role_group_configs[&TrinoRole::Coordinator] + let env = &validated_cluster + .coordinator_role_group_configs .values() .next() .unwrap() From 2d4324aeb9b04b04f03d3fa6ecdf4b04b0c9df79 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 17:53:03 +0200 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1098b35..2dc94235 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,9 @@ All notable changes to this project will be documented in this file. delete each coordinator StatefulSet so that the operator immediately recreates it with the new labels ([#932]). - Make operations infallible where dependent on static inputs ([#939], [#943]). +- Internal operator refactoring: the validated cluster carries each role's configuration in its own + typed fields instead of maps keyed by role, and the coordinator's role config is no longer + converted to the worker's shape and its listener class recovered afterwards ([#945]). ### Fixed @@ -49,6 +52,7 @@ All notable changes to this project will be documented in this file. [#934]: https://github.com/stackabletech/trino-operator/pull/934 [#939]: https://github.com/stackabletech/trino-operator/pull/939 [#943]: https://github.com/stackabletech/trino-operator/pull/943 +[#945]: https://github.com/stackabletech/trino-operator/pull/945 ## [26.7.0] - 2026-07-21 From e9ac3288da54e956e723305c67901658ad806944 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 14 Sep 2026 18:06:13 +0200 Subject: [PATCH 3/3] tighten/correct doc comments --- rust/operator-binary/src/controller/build.rs | 4 +-- rust/operator-binary/src/controller/mod.rs | 12 ++++----- .../src/controller/validate.rs | 27 +++++++++---------- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index 5fa652fa..3046ce33 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -128,8 +128,8 @@ pub fn build( pod_disruption_budgets.extend(build_pdb(cluster.pdb(role), cluster, role)); } - // Only the coordinator has a group listener, so this is not inside the loop above asking each - // role whether it happens to have a listener class. + // Only the coordinator has a group listener, so it is built once here rather than inside the + // role loop. if let Some(listener_group_name) = group_listener_name(cluster, &TrinoRole::Coordinator) { listeners.push(build_group_listener( cluster, diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 3567420a..bcf20e6b 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -160,11 +160,11 @@ impl ValidatedTrinoConfig { /// The coordinator's validated role-level configuration. /// -/// A type of its own rather than a shared one, because the coordinator's role config really is a -/// different type in the CRD ([`v1alpha1::TrinoCoordinatorRoleConfig`] against the worker's -/// `GenericRoleConfig`). Flattening the two into one shape made `listener_class` an `Option` that -/// is mandatory for the coordinator and meaningless for the worker, so every reader had to -/// rediscover which role it was holding. +/// Separate from the worker's because the two are different types in the CRD: +/// [`v1alpha1::TrinoCoordinatorRoleConfig`] carries a `listener_class` for which the worker's +/// `GenericRoleConfig` has no equivalent. One shared type would have to make that field an +/// `Option` — mandatory for one role, meaningless for the other — leaving every reader to work +/// out which role it is holding. #[derive(Clone, Debug)] pub struct ValidatedCoordinatorRoleConfig { pub pdb: stackable_operator::commons::pdb::PdbConfig, @@ -174,7 +174,7 @@ pub struct ValidatedCoordinatorRoleConfig { /// The worker's validated role-level configuration. /// -/// Workers have no group listener, so there is no listener class here to be `None`. +/// Workers have no group listener, so there is no listener class to carry. #[derive(Clone, Debug)] pub struct ValidatedWorkerRoleConfig { pub pdb: stackable_operator::commons::pdb::PdbConfig, diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index d8a8e0ac..31832894 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -207,13 +207,12 @@ pub fn validate( .vector_aggregator_config_map_name .clone(); - // Each role's role groups are validated from the role's own CRD type. `validate_role_groups` - // is generic over the role config, so the coordinator no longer has to be converted down to - // the worker's shape first. + // Each role's role groups are validated from that role's own CRD type: `validate_role_groups` + // is generic over the role config, so neither role has to be converted to the other's shape. // - // Validated in `TrinoRole` declaration order, as the `TrinoRole::iter()` loop this replaced - // was: the first role that fails is the error the user sees, so swapping these two calls - // changes which misconfiguration gets reported when both roles are wrong. + // Validated in `TrinoRole` declaration order: the first role that fails is the error the user + // sees, so swapping these two calls changes which misconfiguration gets reported when both + // roles are wrong. let coordinator_role_group_configs = validate_role_groups( &trino.spec.coordinators, trino, @@ -229,8 +228,8 @@ pub fn validate( &vector_aggregator_config_map_name, )?; - // Read from each role's own role config, so the coordinator's mandatory `listener_class` stays - // mandatory instead of becoming an `Option` that the worker leaves `None`. + // Each role's config comes from its own role config in the spec, so the coordinator's + // `listener_class` stays mandatory and the worker has no such field at all. let coordinator_config = ValidatedCoordinatorRoleConfig { pdb: trino .spec @@ -289,13 +288,11 @@ pub fn validate( )) } -/// Adapts the validated [`RoleGroup`] produced by [`with_validated_config`] into the flattened -/// [`TrinoRoleGroupConfig`] consumed by the build steps. /// Validates every role group of one role, merging default <- role <- role group. /// /// Generic over the role's `RoleConfig` so that each role is read from its own CRD type: the -/// coordinator's [`v1alpha1::TrinoCoordinatorRoleConfig`] no longer has to be converted down to -/// the worker's `GenericRoleConfig` before its role groups can be validated. +/// coordinator's [`v1alpha1::TrinoCoordinatorRoleConfig`] and the worker's `GenericRoleConfig` +/// are both accepted, neither converted to the other. fn validate_role_groups( role: &stackable_operator::v2::role_utils::Role< v1alpha1::TrinoConfigFragment, @@ -345,6 +342,8 @@ where Ok(role_groups) } +/// Adapts the validated [`RoleGroup`] produced by [`with_validated_config`] into the flattened +/// [`TrinoRoleGroupConfig`] consumed by the build steps. fn into_role_group_config( merged: RoleGroup, vector_aggregator_config_map_name: &Option, @@ -527,8 +526,8 @@ mod tests { "simple-trino-coordinator-default-0" ); - // Per-role configs: default (enabled) PDBs. The listener class is on the coordinator's - // config only — the worker type has no such field to assert `None` against. + // Per-role configs: default (enabled) PDBs. Only the coordinator's config carries a + // listener class; the worker type has no such field. for role in [TrinoRole::Coordinator, TrinoRole::Worker] { let pdb = validated.pdb(&role); assert!(pdb.enabled);