From 12fbfa1605c041df516b14726257846121e03adb Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Wed, 11 Nov 2026 22:31:45 +0100 Subject: [PATCH] fix: Return an error instead of panicking on an invalid TrinoCluster name A TrinoCluster name that is valid for Kubernetes but not a valid cluster name, for example longer than 40 characters, made the operator panic while predicting the coordinator pods. The panic crash-looped the operator, so no TrinoCluster was reconciled while the object existed. The name is now validated before anything uses it, and the coordinator pods are predicted from the already validated role groups, so neither the cluster name nor the role group names are parsed with expect. --- CHANGELOG.md | 4 ++ .../src/controller/validate.rs | 59 +++++++++++++++++-- rust/operator-binary/src/crd/mod.rs | 58 ++---------------- 3 files changed, 63 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c78656cc..51d010b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,9 @@ All notable changes to this project will be documented in this file. `CONTAINERDEBUG_LOG_DIRECTORY`, which containerdebug stopped reading in its version 0.2.0 when it moved to `stackable-telemetry`; the directory is now set via `FILE_LOG_DIRECTORY`. The file is rotated hourly and at most two files are kept, because rotation is opt-in ([#970]). +- A `TrinoCluster` whose name is not a valid cluster name, for example longer than 40 characters, + now fails its own reconcile with an error instead of panicking the operator, which then + crash-looped and reconciled no `TrinoCluster` at all ([#973]). ### Removed @@ -107,6 +110,7 @@ All notable changes to this project will be documented in this file. [#966]: https://github.com/stackabletech/trino-operator/pull/966 [#970]: https://github.com/stackabletech/trino-operator/pull/970 [#972]: https://github.com/stackabletech/trino-operator/pull/972 +[#973]: https://github.com/stackabletech/trino-operator/pull/973 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index a642c6ed..9a190084 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -18,8 +18,12 @@ use stackable_operator::{ ValidatedContainerLogConfigChoice, VectorContainerLogConfig, validate_logging_configuration_for_container, }, + role_group_utils::ResourceNames, role_utils::{JavaCommonConfig, RoleGroup, RoleGroupConfig, with_validated_config}, - types::kubernetes::ConfigMapName, + types::{ + kubernetes::{ConfigMapName, NamespaceName}, + operator::{ClusterName, RoleName}, + }, }, }; use strum::{EnumDiscriminants, IntoStaticStr}; @@ -31,7 +35,7 @@ use crate::{ ValidatedCoordinatorRoleConfig, ValidatedTls, ValidatedTrinoConfig, ValidatedWorkerRoleConfig, dereference::DereferencedObjects, }, - crd::{Container, TrinoRole, catalog::TrinoCatalogName, v1alpha1}, + crd::{Container, TrinoRole, catalog::TrinoCatalogName, discovery::TrinoPodRef, v1alpha1}, }; #[derive(Snafu, Debug, EnumDiscriminants)] @@ -178,6 +182,7 @@ pub fn validate( operator_environment: &OperatorEnvironmentOptions, ) -> Result { let namespace = get_namespace(trino).context(GetClusterNamespaceSnafu)?; + let name = get_cluster_name(trino).context(GetClusterNameSnafu)?; let image = trino .spec @@ -290,11 +295,10 @@ pub fn validate( fault_tolerant_execution: dereferenced_objects.resolved_fte_config.clone(), client_protocol: dereferenced_objects.resolved_client_protocol_config.clone(), lineage: dereferenced_objects.resolved_lineage_config.clone(), - coordinator_pod_refs: trino.coordinator_pods(&namespace).collect(), + coordinator_pod_refs: coordinator_pods(&name, &namespace, &coordinator_role_group_configs), catalogs, }; - let name = get_cluster_name(trino).context(GetClusterNameSnafu)?; let uid = get_uid(trino).context(GetClusterUidSnafu)?; Ok(ValidatedCluster::new(ValidatedClusterParams { @@ -311,6 +315,44 @@ pub fn validate( })) } +/// The replica count lower bound used for enumerating role groups when this is not set explicitly (in such cases Kubernetes +/// runs a single pod for a `StatefulSet`). +const COORDINATOR_REPLICAS_IF_UNSET: u16 = 1; + +/// 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 +/// avoid instance churn. +/// +/// A role group without an explicit `replicas` counts will be assigned a single pod by Kubernetes. Counting it as +/// zero would yield no pod refs at all for a single-role-group cluster, and the first of these +/// is what sets `discovery.uri`. +fn coordinator_pods( + cluster_name: &ClusterName, + namespace: &NamespaceName, + role_group_configs: &BTreeMap, +) -> Vec { + let role_name = RoleName::from(&TrinoRole::Coordinator); + role_group_configs + .iter() + .flat_map(|(role_group_name, role_group_config)| { + let resource_names = ResourceNames { + cluster_name: cluster_name.clone(), + role_name: role_name.clone(), + role_group_name: role_group_name.clone(), + }; + (0..role_group_config + .replicas + .unwrap_or(COORDINATOR_REPLICAS_IF_UNSET)) + .map(move |i| TrinoPodRef { + namespace: namespace.to_string(), + role_group_service_name: resource_names.headless_service_name().to_string(), + pod_name: format!("{}-{i}", resource_names.stateful_set_name()), + }) + }) + .collect() +} + /// Validates the user-provided `datasetNamespaceUri`, so that a typo fails the reconcile instead of /// only surfacing once Trino starts. /// @@ -591,6 +633,15 @@ mod tests { } } + #[test] + fn validate_rejects_cluster_name_longer_than_cluster_name_limit() { + let yaml = minimal_yaml("481").replace("simple-trino", &"a".repeat(41)); + + let err = validate_yaml(&yaml, &empty_derefs()) + .expect_err("a 41 character cluster name exceeds the ClusterName limit"); + assert!(matches!(err, Error::GetClusterName { .. })); + } + #[test] fn validate_logging_rejects_invalid_custom_config_map_name() { let yaml = r#" diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index afc6dc4d..86bce6e4 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -5,7 +5,7 @@ pub mod client_protocol; pub mod discovery; pub mod fault_tolerant_execution; -use std::{collections::BTreeMap, ops::Deref, str::FromStr}; +use std::{ops::Deref, str::FromStr}; use affinity::get_affinity; use serde::{Deserialize, Serialize}; @@ -25,7 +25,7 @@ use stackable_operator::{ crd::{authentication::core, openlineage}, deep_merger::ObjectOverrides, k8s_openapi::apimachinery::pkg::{api::resource::Quantity, apis::meta::v1::LabelSelector}, - kube::{CustomResource, ResourceExt}, + kube::CustomResource, memory::{BinaryMultiple, MemoryQuantity}, product_logging::{self, spec::Logging}, role_utils::GenericRoleConfig, @@ -35,23 +35,19 @@ use stackable_operator::{ v2::{ builder::pod::container::EnvVarName, config_overrides::KeyValueConfigOverrides, - role_group_utils::ResourceNames, role_utils::{JavaCommonConfig, Role}, types::{ common::Port, kubernetes::{ - ConfigMapName, ContainerName, ListenerClassName, NamespaceName, SecretClassName, - SecretKey, + ConfigMapName, ContainerName, ListenerClassName, SecretClassName, SecretKey, }, - operator::{ClusterName, RoleGroupName, RoleName}, + operator::RoleName, }, }, versioned::versioned, }; use strum::{Display, EnumIter}; -use crate::crd::discovery::TrinoPodRef; - pub type TrinoCoordinatorRoleType = Role< v1alpha1::TrinoConfigFragment, v1alpha1::TrinoConfigOverrides, @@ -561,53 +557,7 @@ impl v1alpha1::TrinoConfig { } } -/// The replica count lower bound used for enumerating role groups when this is not set explicitly (in such cases Kubernetes -/// runs a single pod for a `StatefulSet`). -const COORDINATOR_REPLICAS_IF_UNSET: u16 = 1; - impl v1alpha1::TrinoCluster { - /// 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 - /// avoid instance churn. - /// - /// A role group without an explicit `replicas` counts will be assigned a single pod by Kubernetes. Counting it as - /// zero would yield no pod refs at all for a single-role-group cluster, and the first of these - /// is what sets `discovery.uri`. - pub fn coordinator_pods( - &self, - namespace: &NamespaceName, - ) -> impl Iterator + '_ { - let ns = namespace.to_string(); - let cluster_name = ClusterName::from_str(&self.name_any()) - .expect("a TrinoCluster name is a valid cluster name"); - let role_name = RoleName::from_str(&TrinoRole::Coordinator.to_string()) - .expect("the coordinator role name is a valid role name"); - self.spec - .coordinators - .role_groups - .iter() - // Order rolegroups consistently, to avoid spurious downstream rewrites - .collect::>() - .into_iter() - .flat_map(move |(rolegroup_name, rolegroup)| { - let resource_names = ResourceNames { - cluster_name: cluster_name.clone(), - role_name: role_name.clone(), - role_group_name: RoleGroupName::from_str(rolegroup_name) - .expect("a role group name is a valid role group name"), - }; - let ns = ns.clone(); - (0..rolegroup.replicas.unwrap_or(COORDINATOR_REPLICAS_IF_UNSET)).map(move |i| { - TrinoPodRef { - namespace: ns.clone(), - role_group_service_name: resource_names.headless_service_name().to_string(), - pod_name: format!("{}-{i}", resource_names.stateful_set_name()), - } - }) - }) - } - /// Returns user provided authentication settings pub fn get_authentication(&self) -> &Vec { &self.spec.cluster_config.authentication