Skip to content

fix: Return an error instead of panicking on an invalid TrinoCluster name #973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lfrancke merged 1 commit into from
Nov 12, 2026
Merged

fix: Return an error instead of panicking on an invalid TrinoCluster name #973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down
59 changes: 55 additions & 4 deletions rust/operator-binary/src/controller/validate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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)]
Expand Down Expand Up @@ -178,6 +182,7 @@ pub fn validate(
operator_environment: &OperatorEnvironmentOptions,
) -> Result<ValidatedCluster> {
let namespace = get_namespace(trino).context(GetClusterNamespaceSnafu)?;
let name = get_cluster_name(trino).context(GetClusterNameSnafu)?;

let image = trino
.spec
Expand Down Expand Up @@ -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 {
Expand All @@ -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<RoleGroupName, TrinoRoleGroupConfig>,
) -> Vec<TrinoPodRef> {
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.
///
Expand Down Expand Up @@ -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#"
Expand Down
58 changes: 4 additions & 54 deletions rust/operator-binary/src/crd/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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<Item = TrinoPodRef> + '_ {
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::<BTreeMap<_, _>>()
.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<core::v1alpha1::ClientAuthenticationDetails> {
&self.spec.cluster_config.authentication
Expand Down
Loading