From abd02d11079deca4099e727cf9a826b9e74f4470 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 20 Jul 2026 12:54:59 +0200 Subject: [PATCH 1/8] factor: add infallible rbac functions --- Cargo.toml | 2 +- rust/operator-binary/src/controller/build.rs | 67 ++++++++++++++-- .../controller/build/resource/config_map.rs | 2 +- .../src/controller/build/resource/mod.rs | 1 + .../src/controller/build/resource/rbac.rs | 42 ++++++++++ .../controller/build/resource/statefulset.rs | 14 ++-- rust/operator-binary/src/controller/mod.rs | 74 +++++++++++------- .../src/controller/validate.rs | 6 +- rust/operator-binary/src/trino_controller.rs | 76 +++++-------------- 9 files changed, 182 insertions(+), 102 deletions(-) create mode 100644 rust/operator-binary/src/controller/build/resource/rbac.rs diff --git a/Cargo.toml b/Cargo.toml index a82a0950..37bc3a37 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,4 +30,4 @@ tracing = "0.1" [patch."https://github.com/stackabletech/operator-rs.git"] # stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "smooth-operator"} -# stackable-operator = { path = "../operator-rs/crates/stackable-operator" } +stackable-operator = { path = "../operator-rs/crates/stackable-operator" } diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index 7f4e4109..f2646261 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -13,6 +13,7 @@ use crate::controller::{ config_map, listener::{build_group_listener, group_listener_name}, pdb::build_pdb, + rbac::{build_role_binding, build_service_account}, service::{ build_rolegroup_headless_service, build_rolegroup_metrics_service, headless_service_ports, @@ -52,13 +53,9 @@ pub enum Error { /// Does not need a Kubernetes client: every reference to another Kubernetes resource is already /// dereferenced and validated by this point, so the errors returned here are resource-assembly /// failures only. -/// -/// `service_account_name` is the name of the RBAC `ServiceAccount` the role-group Pods run under -/// (RBAC resources are built and applied separately, in the reconcile step). pub fn build( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, - service_account_name: &str, ) -> Result { let mut stateful_sets = vec![]; let mut services = vec![]; @@ -115,7 +112,6 @@ pub fn build( role, role_group_name, role_group_config, - service_account_name, ) .context(StatefulSetSnafu { role_group: role_group_name.clone(), @@ -147,11 +143,15 @@ pub fn build( listeners, config_maps, pod_disruption_budgets, + service_accounts: vec![build_service_account(cluster)], + role_bindings: vec![build_role_binding(cluster)], }) } #[cfg(test)] mod tests { + use std::collections::BTreeMap; + use stackable_operator::{ commons::networking::DomainName, kube::Resource, utils::cluster_info::KubernetesClusterInfo, }; @@ -177,8 +177,7 @@ mod tests { .expect("cluster.local is a valid domain name"), }; - let resources = - build(&cluster, &cluster_info, "simple-trino-serviceaccount").expect("build succeeds"); + let resources = build(&cluster, &cluster_info).expect("build succeeds"); // One StatefulSet per role group. assert_eq!( @@ -219,4 +218,58 @@ mod tests { ["simple-trino-coordinator", "simple-trino-worker"] ); } + + /// Locks the RBAC resource names, the roleRef, and the recommended label set against + /// accidental drift. The fixture's cluster name deliberately differs from the product name so + /// that swapped `name`/`instance` label values cannot pass unnoticed. + #[test] + fn build_produces_rbac() { + let cluster = validated_cluster(); + let cluster_info = KubernetesClusterInfo { + cluster_domain: DomainName::try_from("cluster.local") + .expect("cluster.local is a valid domain name"), + }; + + let resources = build(&cluster, &cluster_info).expect("build succeeds"); + + assert_eq!( + sorted_names(&resources.service_accounts), + ["simple-trino-serviceaccount"] + ); + assert_eq!( + sorted_names(&resources.role_bindings), + ["simple-trino-rolebinding"] + ); + + let expected_labels = BTreeMap::from( + [ + ("app.kubernetes.io/component", "none"), + ("app.kubernetes.io/instance", "simple-trino"), + ( + "app.kubernetes.io/managed-by", + "trino.stackable.tech_trinocluster", + ), + ("app.kubernetes.io/name", "trino"), + ("app.kubernetes.io/role-group", "none"), + ("app.kubernetes.io/version", "481-stackable0.0.0-dev"), + ("stackable.tech/vendor", "Stackable"), + ] + .map(|(key, value)| (key.to_string(), value.to_string())), + ); + let service_account = resources + .service_accounts + .first() + .expect("a ServiceAccount is built"); + assert_eq!( + service_account.metadata.labels, + Some(expected_labels.clone()) + ); + + let role_binding = resources + .role_bindings + .first() + .expect("a RoleBinding is built"); + assert_eq!(role_binding.metadata.labels, Some(expected_labels)); + assert_eq!(role_binding.role_ref.name, "trino-clusterrole"); + } } 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 7476f9f0..4b9cd7b0 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -164,7 +164,7 @@ pub fn build_rolegroup_config_map( // 8. jvm.config. The role + role-group `jvmArgumentOverrides` were already merged in the // validate step and are carried by `product_specific_common_config`. let jvm_config = jvm::jvm_config( - cluster.product_version, + cluster.numeric_product_version, &rg.config, &rg.product_specific_common_config.jvm_argument_overrides, ) diff --git a/rust/operator-binary/src/controller/build/resource/mod.rs b/rust/operator-binary/src/controller/build/resource/mod.rs index 0c96e590..bc6845d4 100644 --- a/rust/operator-binary/src/controller/build/resource/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/mod.rs @@ -7,5 +7,6 @@ pub mod config_map; pub mod listener; pub mod pdb; +pub mod rbac; pub mod service; pub mod statefulset; diff --git a/rust/operator-binary/src/controller/build/resource/rbac.rs b/rust/operator-binary/src/controller/build/resource/rbac.rs new file mode 100644 index 00000000..65280866 --- /dev/null +++ b/rust/operator-binary/src/controller/build/resource/rbac.rs @@ -0,0 +1,42 @@ +//! Builds the RBAC resources (ServiceAccount + RoleBinding) shared by all role groups. + +use std::str::FromStr; + +use stackable_operator::{ + k8s_openapi::api::{core::v1::ServiceAccount, rbac::v1::RoleBinding}, + kvp::Labels, + v2::{ + rbac, + types::operator::{RoleGroupName, RoleName}, + }, +}; + +use crate::controller::ValidatedCluster; + +stackable_operator::constant!(NONE_ROLE_NAME: RoleName = "none"); +stackable_operator::constant!(NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); + +/// Builds the [`ServiceAccount`] that the coordinator and worker Pods run under. +pub fn build_service_account(cluster: &ValidatedCluster) -> ServiceAccount { + rbac::build_service_account( + cluster, + &cluster.rbac_resource_names(), + rbac_labels(cluster), + ) +} + +/// Builds the [`RoleBinding`] that binds the [`ServiceAccount`] from [`build_service_account`] to +/// the operator-deployed ClusterRole. +pub fn build_role_binding(cluster: &ValidatedCluster) -> RoleBinding { + rbac::build_role_binding( + cluster, + &cluster.rbac_resource_names(), + rbac_labels(cluster), + ) +} + +/// Both resources are shared by the whole cluster rather than tied to a role or role group, so +/// the recommended labels carry `none` for both values. +fn rbac_labels(cluster: &ValidatedCluster) -> Labels { + cluster.recommended_labels_for(&NONE_ROLE_NAME, &NONE_ROLE_GROUP_NAME) +} diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 20668630..54bd8b5c 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -41,9 +41,9 @@ use crate::{ authorization::opa::OPA_TLS_VOLUME_NAME, controller::{ MAX_PREPARE_LOG_FILE_SIZE, RoleGroupName, STACKABLE_LOG_CONFIG_DIR, STACKABLE_LOG_DIR, - TrinoRoleGroupConfig, ValidatedCluster, build, + TrinoRoleGroupConfig, ValidatedCluster, build::{ - command, + self, command, resource::listener::{ LISTENER_VOLUME_DIR, LISTENER_VOLUME_NAME, build_group_listener_pvc, group_listener_name, secret_volume_listener_scope, @@ -137,10 +137,9 @@ pub fn build_rolegroup_statefulset( trino_role: &TrinoRole, role_group_name: &RoleGroupName, role_group_config: &TrinoRoleGroupConfig, - sa_name: &str, ) -> Result { // Everything below is derived from the validated cluster and the validated role-group config, - // so the caller only needs to pass those (plus the applied ServiceAccount name). + // so the caller only needs to pass those. let resolved_product_image = &cluster.image; let trino_authentication_config = &cluster.cluster_config.authentication; let catalogs = &cluster.cluster_config.catalogs; @@ -422,7 +421,12 @@ pub fn build_rolegroup_statefulset( )), ) .context(AddVolumeSnafu)? - .service_account_name(sa_name) + .service_account_name( + cluster + .rbac_resource_names() + .service_account_name() + .to_string(), + ) .security_context(PodSecurityContextBuilder::new().fs_group(1000).build()); let mut pod_template = pod_builder.build_template(); diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 7e00d944..535192e3 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -10,8 +10,9 @@ use stackable_operator::{ crd::listener::v1alpha1::Listener, k8s_openapi::api::{ apps::v1::StatefulSet, - core::v1::{ConfigMap, Service}, + core::v1::{ConfigMap, Service, ServiceAccount}, policy::v1::PodDisruptionBudget, + rbac::v1::RoleBinding, }, kube::{Resource, api::ObjectMeta}, kvp::Labels, @@ -22,6 +23,7 @@ use stackable_operator::{ builder::meta::ownerreference_from_resource, kvp::label::{recommended_labels, role_group_selector}, role_group_utils::ResourceNames, + role_utils, types::{ kubernetes::{ListenerClassName, NamespaceName, SecretClassName, Uid}, operator::{ @@ -64,6 +66,9 @@ pub(crate) fn shared_spooling_secret_name(cluster_name: &ClusterName) -> String format!("{cluster_name}-spooling-secret") } +// Placeholder version label value for resources whose labels must not change after deployment. +stackable_operator::constant!(UNVERSIONED_PRODUCT_VERSION: ProductVersion = "none"); + /// Every Kubernetes resource produced by the client-free [`build()`](build::build) step. pub struct KubernetesResources { pub stateful_sets: Vec, @@ -71,6 +76,8 @@ pub struct KubernetesResources { pub listeners: Vec, pub config_maps: Vec, pub pod_disruption_budgets: Vec, + pub service_accounts: Vec, + pub role_bindings: Vec, } #[derive(Clone, Debug)] @@ -153,7 +160,11 @@ pub struct ValidatedCluster { pub namespace: NamespaceName, pub uid: Uid, pub image: ResolvedProductImage, - pub product_version: u16, + /// The numeric Trino version (e.g. `481`), used for version-dependent configuration. + pub numeric_product_version: u16, + /// The version label value (`app.kubernetes.io/version`) as a type-safe [`ProductVersion`], + /// 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>, @@ -166,7 +177,7 @@ impl ValidatedCluster { namespace: NamespaceName, uid: Uid, image: ResolvedProductImage, - product_version: u16, + numeric_product_version: u16, cluster_config: ValidatedClusterConfig, role_configs: BTreeMap, role_group_configs: BTreeMap>, @@ -181,8 +192,10 @@ impl ValidatedCluster { name, namespace, uid, + product_version: ProductVersion::from_str(&image.app_version_label_value) + .expect("the app version label value is a valid product version"), image, - product_version, + numeric_product_version, cluster_config, role_configs, role_group_configs, @@ -219,6 +232,15 @@ impl ValidatedCluster { self.cluster_config.authentication_enabled() || self.server_tls_enabled() } + /// Type-safe names for the per-cluster RBAC resources: the ServiceAccount shared by all + /// Pods, its (namespaced) RoleBinding, and the operator-deployed ClusterRole it binds. + pub fn rbac_resource_names(&self) -> role_utils::ResourceNames { + role_utils::ResourceNames { + cluster_name: self.name.clone(), + product_name: product_name(), + } + } + /// Type-safe names for the resources of a given role group. pub(crate) fn resource_names( &self, @@ -270,16 +292,10 @@ impl ValidatedCluster { RoleName::from_str(&role.to_string()).expect("a TrinoRole is a valid RFC 1123 role name") } - /// The version label value (`app.kubernetes.io/version`) as a type-safe [`ProductVersion`]. - fn version_label(&self) -> ProductVersion { - ProductVersion::from_str(&self.image.app_version_label_value) - .expect("the app version label value is a valid product version") - } - - fn recommended_labels_with_version( + fn recommended_labels_with( &self, version: &ProductVersion, - role: &TrinoRole, + role_name: &RoleName, role_group_name: &RoleGroupName, ) -> Labels { recommended_labels( @@ -288,38 +304,42 @@ impl ValidatedCluster { version, &operator_name(), &controller_name(), - &Self::role_name(role), + role_name, role_group_name, ) } /// Recommended labels for a role-group resource (using the resolved product version). - pub(crate) fn recommended_labels( + pub fn recommended_labels(&self, role: &TrinoRole, role_group_name: &RoleGroupName) -> Labels { + self.recommended_labels_for(&Self::role_name(role), role_group_name) + } + + /// Recommended labels for a resource that is not tied to a concrete [`TrinoRole`] (e.g. the + /// cluster-shared RBAC resources), using a free-form role/role-group label value. + pub fn recommended_labels_for( &self, - role: &TrinoRole, + role_name: &RoleName, role_group_name: &RoleGroupName, ) -> Labels { - self.recommended_labels_with_version(&self.version_label(), role, role_group_name) + self.recommended_labels_with(&self.product_version, role_name, role_group_name) } - /// Recommended labels using a fixed `"none"` version, for resources whose labels must not - /// change after creation (e.g. listener PVC templates). - pub(crate) fn unversioned_recommended_labels( + /// Recommended labels with the constant [`UNVERSIONED_PRODUCT_VERSION`], for resources whose + /// labels must not change after creation (e.g. listener PVC templates). + pub fn unversioned_recommended_labels( &self, role: &TrinoRole, role_group_name: &RoleGroupName, ) -> Labels { - let none = ProductVersion::from_str("none") - .expect("\"none\" is a valid product version label value"); - self.recommended_labels_with_version(&none, role, role_group_name) + self.recommended_labels_with( + &UNVERSIONED_PRODUCT_VERSION, + &Self::role_name(role), + role_group_name, + ) } /// Selector labels matching the pods of a role group. - pub(crate) fn role_group_selector( - &self, - role: &TrinoRole, - role_group_name: &RoleGroupName, - ) -> Labels { + pub fn role_group_selector(&self, role: &TrinoRole, role_group_name: &RoleGroupName) -> Labels { role_group_selector( self, &product_name(), diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 21d82b90..e60966bc 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -188,7 +188,7 @@ pub fn validate( ) .context(ResolveProductImageSnafu)?; - let product_version = + let numeric_product_version = u16::from_str(&image.product_version).context(ParseTrinoVersionSnafu { product_version: image.product_version.clone(), })?; @@ -310,7 +310,7 @@ pub fn validate( namespace, uid, image, - product_version, + numeric_product_version, cluster_config, role_configs, role_group_configs, @@ -452,7 +452,7 @@ mod tests { validated.uid.to_string(), "e6ac237d-a6d4-43a1-8135-f36506110912" ); - assert_eq!(validated.product_version, 481); + assert_eq!(validated.numeric_product_version, 481); assert!(!validated.cluster_config.authentication_enabled()); assert!( validated diff --git a/rust/operator-binary/src/trino_controller.rs b/rust/operator-binary/src/trino_controller.rs index 8c9741eb..238c7316 100644 --- a/rust/operator-binary/src/trino_controller.rs +++ b/rust/operator-binary/src/trino_controller.rs @@ -6,9 +6,8 @@ use snafu::{ResultExt, Snafu}; use stackable_operator::{ cli::OperatorEnvironmentOptions, cluster_resources::ClusterResourceApplyStrategy, - commons::{random_secret_creation, rbac::build_rbac_resources}, + commons::random_secret_creation, kube::{ - ResourceExt, core::{DeserializeGuard, error_boundary}, runtime::controller::Action, }, @@ -27,7 +26,7 @@ use crate::{ ValidatedCluster, build, controller_name, dereference, operator_name, product_name, shared_internal_secret_name, shared_spooling_secret_name, validate, }, - crd::{APP_NAME, ENV_INTERNAL_SECRET, ENV_SPOOLING_SECRET, v1alpha1}, + crd::{ENV_INTERNAL_SECRET, ENV_SPOOLING_SECRET, v1alpha1}, }; pub struct Ctx { @@ -57,37 +56,11 @@ pub enum Error { source: stackable_operator::cluster_resources::Error, }, - #[snafu(display("failed to patch service account"))] - ApplyServiceAccount { - source: stackable_operator::cluster_resources::Error, - }, - - #[snafu(display("failed to patch role binding"))] - ApplyRoleBinding { - source: stackable_operator::cluster_resources::Error, - }, - #[snafu(display("failed to update status"))] ApplyStatus { source: stackable_operator::client::Error, }, - #[snafu(display("failed to build RBAC resources"))] - BuildRbacResources { - source: stackable_operator::commons::rbac::Error, - }, - - #[snafu(display("failed to get required Labels"))] - GetRequiredLabels { - source: - stackable_operator::kvp::KeyValuePairError, - }, - - #[snafu(display("failed to build Labels"))] - LabelBuild { - source: stackable_operator::kvp::LabelError, - }, - #[snafu(display("invalid TrinoCluster object"))] InvalidTrinoCluster { source: error_boundary::InvalidObject, @@ -153,40 +126,27 @@ pub async fn reconcile_trino( &trino.spec.object_overrides, ); - let (rbac_sa, rbac_rolebinding) = build_rbac_resources( - trino, - APP_NAME, - cluster_resources - .get_required_labels() - .context(GetRequiredLabelsSnafu)?, - ) - .context(BuildRbacResourcesSnafu)?; - - // The ServiceAccount name is deterministic on the built object, so the build step does not - // depend on the applied ServiceAccount. - let service_account_name = rbac_sa.name_any(); - - cluster_resources - .add(client, rbac_sa) - .await - .context(ApplyServiceAccountSnafu)?; - - cluster_resources - .add(client, rbac_rolebinding) - .await - .context(ApplyRoleBindingSnafu)?; - ensure_random_secrets(client, &validated_cluster).await?; - let resources = build::build( - &validated_cluster, - &client.kubernetes_cluster_info, - &service_account_name, - ) - .context(BuildResourcesSnafu)?; + let resources = build::build(&validated_cluster, &client.kubernetes_cluster_info) + .context(BuildResourcesSnafu)?; let mut sts_cond_builder = StatefulSetConditionBuilder::default(); + for service_account in resources.service_accounts { + cluster_resources + .add(client, service_account) + .await + .context(ApplyResourceSnafu)?; + } + + for role_binding in resources.role_bindings { + cluster_resources + .add(client, role_binding) + .await + .context(ApplyResourceSnafu)?; + } + for service in resources.services { cluster_resources .add(client, service) From 340bf8da368a00d885c83ed910cc1b6c9296cb02 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 23 Jul 2026 13:11:45 +0200 Subject: [PATCH 2/8] remove patch branch reference --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 37bc3a37..a82a0950 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,4 +30,4 @@ tracing = "0.1" [patch."https://github.com/stackabletech/operator-rs.git"] # stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "smooth-operator"} -stackable-operator = { path = "../operator-rs/crates/stackable-operator" } +# stackable-operator = { path = "../operator-rs/crates/stackable-operator" } From a99d59a82ff61ac5a7cfd441f013e0ee7596fd0d Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 23 Jul 2026 13:57:29 +0200 Subject: [PATCH 3/8] changelog, rename resource name functions, role parsing test --- CHANGELOG.md | 3 +++ .../controller/build/resource/config_map.rs | 2 +- .../src/controller/build/resource/rbac.rs | 4 ++-- .../src/controller/build/resource/service.rs | 4 ++-- .../controller/build/resource/statefulset.rs | 4 ++-- rust/operator-binary/src/controller/mod.rs | 23 ++++++++++++++++--- 6 files changed, 30 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c419f58..87df1568 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,11 @@ All notable changes to this project will be documented in this file. - Internal operator refactoring: introduce a build() step in the reconciler that assembles all relevant Kubernetes resources before anything is applied ([#909]). - Bump `stackable-operator` to 0.114.0 ([#918]). +- The RBAC ServiceAccount and RoleBinding are now built with the operator-rs `v2::rbac` + functions and carry the full set of recommended labels ([#913]). [#909]: https://github.com/stackabletech/trino-operator/pull/909 +[#913]: https://github.com/stackabletech/trino-operator/pull/913 [#918]: https://github.com/stackabletech/trino-operator/pull/918 ## [26.7.0] - 2026-07-21 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 4b9cd7b0..50abb32e 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -74,7 +74,7 @@ pub fn build_rolegroup_config_map( })?; let config_map_name = cluster - .resource_names(role, role_group_name) + .role_group_resource_names(role, role_group_name) .role_group_config_map() .to_string(); diff --git a/rust/operator-binary/src/controller/build/resource/rbac.rs b/rust/operator-binary/src/controller/build/resource/rbac.rs index 65280866..aa0063c0 100644 --- a/rust/operator-binary/src/controller/build/resource/rbac.rs +++ b/rust/operator-binary/src/controller/build/resource/rbac.rs @@ -20,7 +20,7 @@ stackable_operator::constant!(NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); pub fn build_service_account(cluster: &ValidatedCluster) -> ServiceAccount { rbac::build_service_account( cluster, - &cluster.rbac_resource_names(), + &cluster.cluster_resource_names(), rbac_labels(cluster), ) } @@ -30,7 +30,7 @@ pub fn build_service_account(cluster: &ValidatedCluster) -> ServiceAccount { pub fn build_role_binding(cluster: &ValidatedCluster) -> RoleBinding { rbac::build_role_binding( cluster, - &cluster.rbac_resource_names(), + &cluster.cluster_resource_names(), rbac_labels(cluster), ) } diff --git a/rust/operator-binary/src/controller/build/resource/service.rs b/rust/operator-binary/src/controller/build/resource/service.rs index 00607d3d..80645a1f 100644 --- a/rust/operator-binary/src/controller/build/resource/service.rs +++ b/rust/operator-binary/src/controller/build/resource/service.rs @@ -25,7 +25,7 @@ pub fn build_rolegroup_headless_service( metadata: cluster .object_meta( cluster - .resource_names(role, role_group_name) + .role_group_resource_names(role, role_group_name) .headless_service_name() .to_string(), recommended_labels.clone(), @@ -56,7 +56,7 @@ pub fn build_rolegroup_metrics_service( metadata: cluster .object_meta( cluster - .resource_names(role, role_group_name) + .role_group_resource_names(role, role_group_name) .metrics_service_name() .to_string(), recommended_labels.clone(), diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 54bd8b5c..ce489a42 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -149,7 +149,7 @@ pub fn build_rolegroup_statefulset( let env_overrides = &role_group_config.env_overrides; let merged_config = &role_group_config.config; - let resource_names = cluster.resource_names(trino_role, role_group_name); + let resource_names = cluster.role_group_resource_names(trino_role, role_group_name); let config_map_name = resource_names.role_group_config_map().to_string(); let mut pod_builder = PodBuilder::new(); @@ -423,7 +423,7 @@ pub fn build_rolegroup_statefulset( .context(AddVolumeSnafu)? .service_account_name( cluster - .rbac_resource_names() + .cluster_resource_names() .service_account_name() .to_string(), ) diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 535192e3..16471773 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -234,7 +234,7 @@ impl ValidatedCluster { /// Type-safe names for the per-cluster RBAC resources: the ServiceAccount shared by all /// Pods, its (namespaced) RoleBinding, and the operator-deployed ClusterRole it binds. - pub fn rbac_resource_names(&self) -> role_utils::ResourceNames { + pub fn cluster_resource_names(&self) -> role_utils::ResourceNames { role_utils::ResourceNames { cluster_name: self.name.clone(), product_name: product_name(), @@ -242,7 +242,7 @@ impl ValidatedCluster { } /// Type-safe names for the resources of a given role group. - pub(crate) fn resource_names( + pub(crate) fn role_group_resource_names( &self, role: &TrinoRole, role_group_name: &RoleGroupName, @@ -263,7 +263,7 @@ impl ValidatedCluster { ) -> String { format!( "{}-catalog", - self.resource_names(role, role_group_name) + self.role_group_resource_names(role, role_group_name) .role_group_config_map() ) } @@ -465,3 +465,20 @@ pub(crate) fn validated_cluster() -> ValidatedCluster { validate::validate(&minimal_trino(), &derefs, &operator_env).expect("validate should succeed") } + +#[cfg(test)] +mod tests { + use strum::IntoEnumIterator; + + use super::ValidatedCluster; + use crate::crd::TrinoRole; + + /// Locks the invariant behind the `expect` in [`ValidatedCluster::role_name`]: every + /// `TrinoRole` variant (present and future) must serialise to a valid `RoleName`. + #[test] + fn every_trino_role_serialises_to_a_valid_role_name() { + for role in TrinoRole::iter() { + ValidatedCluster::role_name(&role); + } + } +} From 340a00cf492ea04abbefe8523dabe5eb2c0077fe Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 24 Jul 2026 14:42:09 +0200 Subject: [PATCH 4/8] added role/From impl for trino roles plus parsing test --- .../src/controller/build/resource/pdb.rs | 5 ++-- rust/operator-binary/src/controller/mod.rs | 30 ++++++------------- rust/operator-binary/src/crd/mod.rs | 12 ++++++++ 3 files changed, 23 insertions(+), 24 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index 9f81f2cc..f3aff6ad 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -1,4 +1,4 @@ -use std::{cmp::max, str::FromStr}; +use std::cmp::max; use stackable_operator::{ commons::pdb::PdbConfig, @@ -26,8 +26,7 @@ pub fn build_pdb( TrinoRole::Coordinator => max_unavailable_coordinators(), TrinoRole::Worker => max_unavailable_workers(worker_count(cluster)), }); - let role_name = - RoleName::from_str(&role.to_string()).expect("a TrinoRole is a valid RFC 1123 role name"); + let role_name: RoleName = role.into(); let pdb = pod_disruption_budget_builder_with_role( cluster, &product_name(), diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 16471773..28e5d2b4 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -249,7 +249,7 @@ impl ValidatedCluster { ) -> ResourceNames { ResourceNames { cluster_name: self.name.clone(), - role_name: Self::role_name(role), + role_name: role.into(), role_group_name: role_group_name.clone(), } } @@ -288,10 +288,6 @@ impl ValidatedCluster { } /// A [`TrinoRole`] as a type-safe [`RoleName`]. - fn role_name(role: &TrinoRole) -> RoleName { - RoleName::from_str(&role.to_string()).expect("a TrinoRole is a valid RFC 1123 role name") - } - fn recommended_labels_with( &self, version: &ProductVersion, @@ -311,7 +307,7 @@ impl ValidatedCluster { /// Recommended labels for a role-group resource (using the resolved product version). pub fn recommended_labels(&self, role: &TrinoRole, role_group_name: &RoleGroupName) -> Labels { - self.recommended_labels_for(&Self::role_name(role), role_group_name) + self.recommended_labels_for(&role.into(), role_group_name) } /// Recommended labels for a resource that is not tied to a concrete [`TrinoRole`] (e.g. the @@ -331,21 +327,12 @@ impl ValidatedCluster { role: &TrinoRole, role_group_name: &RoleGroupName, ) -> Labels { - self.recommended_labels_with( - &UNVERSIONED_PRODUCT_VERSION, - &Self::role_name(role), - role_group_name, - ) + self.recommended_labels_with(&UNVERSIONED_PRODUCT_VERSION, &role.into(), role_group_name) } /// Selector labels matching the pods of a role group. pub fn role_group_selector(&self, role: &TrinoRole, role_group_name: &RoleGroupName) -> Labels { - role_group_selector( - self, - &product_name(), - &Self::role_name(role), - role_group_name, - ) + role_group_selector(self, &product_name(), &role.into(), role_group_name) } } @@ -468,17 +455,18 @@ pub(crate) fn validated_cluster() -> ValidatedCluster { #[cfg(test)] mod tests { + use stackable_operator::v2::types::operator::RoleName; use strum::IntoEnumIterator; - use super::ValidatedCluster; use crate::crd::TrinoRole; - /// Locks the invariant behind the `expect` in [`ValidatedCluster::role_name`]: every - /// `TrinoRole` variant (present and future) must serialise to a valid `RoleName`. + /// Locks the invariant behind the `expect` in the `From for RoleName` impls: + /// every `TrinoRole` variant (present and future) must serialise to a valid `RoleName`. #[test] fn every_trino_role_serialises_to_a_valid_role_name() { for role in TrinoRole::iter() { - ValidatedCluster::role_name(&role); + let _: RoleName = (&role).into(); + let _: RoleName = role.into(); } } } diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 5661b002..db6c0df6 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -420,6 +420,18 @@ pub enum TrinoRole { Worker, } +impl From for RoleName { + fn from(value: TrinoRole) -> Self { + RoleName::from_str(&value.to_string()).expect("a TrinoRole is a valid role name") + } +} + +impl From<&TrinoRole> for RoleName { + fn from(value: &TrinoRole) -> Self { + RoleName::from_str(&value.to_string()).expect("a TrinoRole is a valid role name") + } +} + impl TrinoRole { pub fn listener_class_name(&self, trino: &v1alpha1::TrinoCluster) -> Option { match self { From ee698bf71e668053fd0c40823a00ec4c709f33dc Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 27 Jul 2026 15:11:21 +0200 Subject: [PATCH 5/8] moved tests and made them more thorough, reset patch ref in cargo toml --- Cargo.toml | 2 +- rust/operator-binary/src/controller/build.rs | 49 +--------- .../src/controller/build/resource/rbac.rs | 93 +++++++++++++++++++ .../src/controller/build/resource/service.rs | 85 +++++++++++++++++ rust/operator-binary/src/controller/mod.rs | 16 ++++ 5 files changed, 196 insertions(+), 49 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a82a0950..5933d496 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,5 +29,5 @@ tokio = { version = "1.52", features = ["full"] } tracing = "0.1" [patch."https://github.com/stackabletech/operator-rs.git"] -# stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "smooth-operator"} +# stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "main"} # stackable-operator = { path = "../operator-rs/crates/stackable-operator" } diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index f2646261..346fe682 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -150,8 +150,6 @@ pub fn build( #[cfg(test)] mod tests { - use std::collections::BTreeMap; - use stackable_operator::{ commons::networking::DomainName, kube::Resource, utils::cluster_info::KubernetesClusterInfo, }; @@ -217,21 +215,7 @@ mod tests { sorted_names(&resources.pod_disruption_budgets), ["simple-trino-coordinator", "simple-trino-worker"] ); - } - - /// Locks the RBAC resource names, the roleRef, and the recommended label set against - /// accidental drift. The fixture's cluster name deliberately differs from the product name so - /// that swapped `name`/`instance` label values cannot pass unnoticed. - #[test] - fn build_produces_rbac() { - let cluster = validated_cluster(); - let cluster_info = KubernetesClusterInfo { - cluster_domain: DomainName::try_from("cluster.local") - .expect("cluster.local is a valid domain name"), - }; - - let resources = build(&cluster, &cluster_info).expect("build succeeds"); - + // The cluster-shared RBAC pair. assert_eq!( sorted_names(&resources.service_accounts), ["simple-trino-serviceaccount"] @@ -240,36 +224,5 @@ mod tests { sorted_names(&resources.role_bindings), ["simple-trino-rolebinding"] ); - - let expected_labels = BTreeMap::from( - [ - ("app.kubernetes.io/component", "none"), - ("app.kubernetes.io/instance", "simple-trino"), - ( - "app.kubernetes.io/managed-by", - "trino.stackable.tech_trinocluster", - ), - ("app.kubernetes.io/name", "trino"), - ("app.kubernetes.io/role-group", "none"), - ("app.kubernetes.io/version", "481-stackable0.0.0-dev"), - ("stackable.tech/vendor", "Stackable"), - ] - .map(|(key, value)| (key.to_string(), value.to_string())), - ); - let service_account = resources - .service_accounts - .first() - .expect("a ServiceAccount is built"); - assert_eq!( - service_account.metadata.labels, - Some(expected_labels.clone()) - ); - - let role_binding = resources - .role_bindings - .first() - .expect("a RoleBinding is built"); - assert_eq!(role_binding.metadata.labels, Some(expected_labels)); - assert_eq!(role_binding.role_ref.name, "trino-clusterrole"); } } diff --git a/rust/operator-binary/src/controller/build/resource/rbac.rs b/rust/operator-binary/src/controller/build/resource/rbac.rs index aa0063c0..4f7d935e 100644 --- a/rust/operator-binary/src/controller/build/resource/rbac.rs +++ b/rust/operator-binary/src/controller/build/resource/rbac.rs @@ -40,3 +40,96 @@ pub fn build_role_binding(cluster: &ValidatedCluster) -> RoleBinding { fn rbac_labels(cluster: &ValidatedCluster) -> Labels { cluster.recommended_labels_for(&NONE_ROLE_NAME, &NONE_ROLE_GROUP_NAME) } + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + use crate::controller::{app_version_label, validated_cluster}; + + // `simple-trino` vs `trino`: see the swap-guard note on `MINIMAL_TRINO_YAML`. + + #[test] + fn test_service_account() { + let service_account = build_service_account(&validated_cluster()); + + assert_eq!( + json!({ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": { + // The RBAC resources are cluster-shared, so role and role group are `none`. + "labels": { + "app.kubernetes.io/component": "none", + "app.kubernetes.io/instance": "simple-trino", + "app.kubernetes.io/managed-by": "trino.stackable.tech_trinocluster", + "app.kubernetes.io/name": "trino", + "app.kubernetes.io/role-group": "none", + "app.kubernetes.io/version": app_version_label("481"), + "stackable.tech/vendor": "Stackable" + }, + "name": "simple-trino-serviceaccount", + "namespace": "default", + "ownerReferences": [ + { + "apiVersion": "trino.stackable.tech/v1alpha1", + "controller": true, + "kind": "TrinoCluster", + "name": "simple-trino", + "uid": "e6ac237d-a6d4-43a1-8135-f36506110912" + } + ] + } + }), + serde_json::to_value(service_account).expect("must be serializable") + ); + } + + #[test] + fn test_role_binding() { + let role_binding = build_role_binding(&validated_cluster()); + + assert_eq!( + json!({ + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "RoleBinding", + "metadata": { + "labels": { + "app.kubernetes.io/component": "none", + "app.kubernetes.io/instance": "simple-trino", + "app.kubernetes.io/managed-by": "trino.stackable.tech_trinocluster", + "app.kubernetes.io/name": "trino", + "app.kubernetes.io/role-group": "none", + "app.kubernetes.io/version": app_version_label("481"), + "stackable.tech/vendor": "Stackable" + }, + "name": "simple-trino-rolebinding", + "namespace": "default", + "ownerReferences": [ + { + "apiVersion": "trino.stackable.tech/v1alpha1", + "controller": true, + "kind": "TrinoCluster", + "name": "simple-trino", + "uid": "e6ac237d-a6d4-43a1-8135-f36506110912" + } + ] + }, + "roleRef": { + "apiGroup": "rbac.authorization.k8s.io", + "kind": "ClusterRole", + "name": "trino-clusterrole" + }, + "subjects": [ + { + "kind": "ServiceAccount", + "name": "simple-trino-serviceaccount", + "namespace": "default" + } + ] + }), + serde_json::to_value(role_binding).expect("must be serializable") + ); + } +} diff --git a/rust/operator-binary/src/controller/build/resource/service.rs b/rust/operator-binary/src/controller/build/resource/service.rs index 80645a1f..14056b9d 100644 --- a/rust/operator-binary/src/controller/build/resource/service.rs +++ b/rust/operator-binary/src/controller/build/resource/service.rs @@ -102,3 +102,88 @@ fn metrics_service_ports() -> Vec { ..ServicePort::default() }] } + +#[cfg(test)] +mod tests { + use serde_json::json; + use stackable_operator::v2::types::operator::RoleGroupName; + + use super::*; + use crate::controller::{app_version_label, validated_cluster}; + + /// Every metrics Service must carry the Prometheus scrape label and the + /// `prometheus.io/path|port|scheme|scrape` annotations, or Prometheus stops discovering the + /// endpoints. + #[test] + fn test_rolegroup_metrics_service() { + let cluster = validated_cluster(); + let role = TrinoRole::Coordinator; + let role_group_name: RoleGroupName = "default".parse().expect("valid role group name"); + // Mirrors how `build()` derives the label arguments. + let recommended_labels = cluster.recommended_labels(&role, &role_group_name); + let selector = cluster.role_group_selector(&role, &role_group_name); + + let service = build_rolegroup_metrics_service( + &cluster, + &role, + &role_group_name, + &recommended_labels, + selector.into(), + ); + + assert_eq!( + json!({ + "apiVersion": "v1", + "kind": "Service", + "metadata": { + "annotations": { + "prometheus.io/path": "/metrics", + "prometheus.io/port": "8081", + "prometheus.io/scheme": "http", + "prometheus.io/scrape": "true" + }, + "labels": { + "app.kubernetes.io/component": "coordinator", + "app.kubernetes.io/instance": "simple-trino", + "app.kubernetes.io/managed-by": "trino.stackable.tech_trinocluster", + "app.kubernetes.io/name": "trino", + "app.kubernetes.io/role-group": "default", + "app.kubernetes.io/version": app_version_label("481"), + "prometheus.io/scrape": "true", + "stackable.tech/vendor": "Stackable" + }, + "name": "simple-trino-coordinator-default-metrics", + "namespace": "default", + "ownerReferences": [ + { + "apiVersion": "trino.stackable.tech/v1alpha1", + "controller": true, + "kind": "TrinoCluster", + "name": "simple-trino", + "uid": "e6ac237d-a6d4-43a1-8135-f36506110912" + } + ] + }, + "spec": { + "clusterIP": "None", + "ports": [ + { + "name": "metrics", + "port": 8081, + "protocol": "TCP" + } + ], + "publishNotReadyAddresses": true, + "selector": { + "app.kubernetes.io/component": "coordinator", + "app.kubernetes.io/instance": "simple-trino", + "app.kubernetes.io/name": "trino", + "app.kubernetes.io/role-group": "default" + }, + "type": "ClusterIP" + } + }), + serde_json::to_value(service).expect("must be serializable") + ); + } +} diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 28e5d2b4..237d43a3 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -398,7 +398,23 @@ pub(crate) fn controller_name() -> ControllerName { ControllerName::from_str(CONTROLLER_NAME).expect("the controller name is a valid label value") } +/// The expected `app.kubernetes.io/version` label value for the given product version. +/// +/// The `-stackable` suffix carries the operator's own version, which is `0.0.0-dev` on main +/// but rewritten by the release process — so tests must derive it rather than hardcode it, +/// or they fail on release branches. +#[cfg(test)] +pub(crate) fn app_version_label(product_version: &str) -> String { + format!( + "{product_version}-stackable{}", + crate::built_info::PKG_VERSION + ) +} + /// A minimal, valid TrinoCluster spec shared across unit tests. +/// +/// The cluster name (`simple-trino`) deliberately differs from the product name (`trino`), so +/// tests asserting recommended labels catch swapped `name`/`instance` values. #[cfg(test)] pub(crate) const MINIMAL_TRINO_YAML: &str = r#" apiVersion: trino.stackable.tech/v1alpha1 From 575e652a6b669df4b548926f3a15d12750f55563 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 27 Jul 2026 15:15:08 +0200 Subject: [PATCH 6/8] move object_meta to build step --- rust/operator-binary/src/controller/build.rs | 24 +++++++- .../controller/build/resource/config_map.rs | 26 +++++---- .../src/controller/build/resource/listener.rs | 9 +-- .../src/controller/build/resource/service.rs | 55 ++++++++++--------- .../controller/build/resource/statefulset.rs | 18 +++--- rust/operator-binary/src/controller/mod.rs | 21 ------- 6 files changed, 80 insertions(+), 73 deletions(-) diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index 346fe682..762b5158 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -4,7 +4,10 @@ use std::str::FromStr; use snafu::{ResultExt, Snafu}; use stackable_operator::{ - utils::cluster_info::KubernetesClusterInfo, v2::types::operator::RoleGroupName, + builder::meta::ObjectMetaBuilder, + kvp::Labels, + utils::cluster_info::KubernetesClusterInfo, + v2::{builder::meta::ownerreference_from_resource, types::operator::RoleGroupName}, }; use crate::controller::{ @@ -148,6 +151,25 @@ pub fn build( }) } +/// Returns an [`ObjectMetaBuilder`] pre-filled with the cluster's namespace, an owner +/// reference back to the cluster, the resource `name` and the given `recommended_labels`. +/// +/// Consolidates the metadata chain repeated by the child-resource builders. Call sites that +/// need extra labels/annotations chain them onto the returned builder. +pub(crate) fn object_meta( + cluster: &ValidatedCluster, + name: impl Into, + recommended_labels: Labels, +) -> ObjectMetaBuilder { + let mut builder = ObjectMetaBuilder::new(); + builder + .name_and_namespace(cluster) + .name(name) + .ownerreference(ownerreference_from_resource(cluster, None, Some(true))) + .with_labels(recommended_labels); + builder +} + #[cfg(test)] mod tests { use stackable_operator::{ 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 50abb32e..ec7b9926 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -13,10 +13,13 @@ use crate::{ config::jvm, controller::{ RoleGroupName, ValidatedCluster, - build::properties::{ - ConfigFileName, access_control_properties, config_properties, - exchange_manager_properties, log_properties, node_properties, product_logging, - security_properties, spooling_manager_properties, + build::{ + object_meta, + properties::{ + ConfigFileName, access_control_properties, config_properties, + exchange_manager_properties, log_properties, node_properties, product_logging, + security_properties, spooling_manager_properties, + }, }, }, crd::TrinoRole, @@ -181,11 +184,7 @@ pub fn build_rolegroup_config_map( } ConfigMapBuilder::new() - .metadata( - cluster - .object_meta(&config_map_name, recommended_labels.clone()) - .build(), - ) + .metadata(object_meta(cluster, &config_map_name, recommended_labels.clone()).build()) .data(data) .build() .with_context(|_| AssembleSnafu { @@ -204,9 +203,12 @@ pub fn build_rolegroup_catalog_config_map( let catalog_config_map_name = cluster.role_group_catalog_config_map_name(role, role_group_name); ConfigMapBuilder::new() .metadata( - cluster - .object_meta(&catalog_config_map_name, recommended_labels.clone()) - .build(), + object_meta( + cluster, + &catalog_config_map_name, + recommended_labels.clone(), + ) + .build(), ) .data( cluster diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index 68458ccc..73d07843 100644 --- a/rust/operator-binary/src/controller/build/resource/listener.rs +++ b/rust/operator-binary/src/controller/build/resource/listener.rs @@ -10,7 +10,10 @@ use stackable_operator::{ }; use crate::{ - controller::{ValidatedCluster, build::ports}, + controller::{ + ValidatedCluster, + build::{object_meta, ports}, + }, crd::TrinoRole, }; @@ -32,9 +35,7 @@ pub fn build_group_listener( listener_group_name: String, ) -> Listener { Listener { - metadata: cluster - .object_meta(listener_group_name, recommended_labels) - .build(), + metadata: object_meta(cluster, listener_group_name, recommended_labels).build(), spec: ListenerSpec { class_name: Some(listener_class.to_string()), ports: Some(listener_ports(cluster)), diff --git a/rust/operator-binary/src/controller/build/resource/service.rs b/rust/operator-binary/src/controller/build/resource/service.rs index 14056b9d..19e3dd3e 100644 --- a/rust/operator-binary/src/controller/build/resource/service.rs +++ b/rust/operator-binary/src/controller/build/resource/service.rs @@ -7,7 +7,10 @@ use stackable_operator::{ }; use crate::{ - controller::{RoleGroupName, ValidatedCluster, build::ports}, + controller::{ + RoleGroupName, ValidatedCluster, + build::{object_meta, ports}, + }, crd::{METRICS_PORT, METRICS_PORT_NAME, TrinoRole}, }; @@ -22,15 +25,15 @@ pub fn build_rolegroup_headless_service( ports: Vec, ) -> Service { Service { - metadata: cluster - .object_meta( - cluster - .role_group_resource_names(role, role_group_name) - .headless_service_name() - .to_string(), - recommended_labels.clone(), - ) - .build(), + metadata: object_meta( + cluster, + cluster + .role_group_resource_names(role, role_group_name) + .headless_service_name() + .to_string(), + recommended_labels.clone(), + ) + .build(), spec: Some(ServiceSpec { // Internal communication does not need to be exposed type_: Some("ClusterIP".to_string()), @@ -53,22 +56,22 @@ pub fn build_rolegroup_metrics_service( selector: BTreeMap, ) -> Service { Service { - metadata: cluster - .object_meta( - cluster - .role_group_resource_names(role, role_group_name) - .metrics_service_name() - .to_string(), - recommended_labels.clone(), - ) - .with_labels(prometheus_labels(&Scraping::Enabled)) - .with_annotations(prometheus_annotations( - &Scraping::Enabled, - &Scheme::Http, - "/metrics", - &METRICS_PORT, - )) - .build(), + metadata: object_meta( + cluster, + cluster + .role_group_resource_names(role, role_group_name) + .metrics_service_name() + .to_string(), + recommended_labels.clone(), + ) + .with_labels(prometheus_labels(&Scraping::Enabled)) + .with_annotations(prometheus_annotations( + &Scraping::Enabled, + &Scheme::Http, + "/metrics", + &METRICS_PORT, + )) + .build(), spec: Some(ServiceSpec { // Internal communication does not need to be exposed type_: Some("ClusterIP".to_string()), diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index ce489a42..a7c7145e 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -43,7 +43,7 @@ use crate::{ MAX_PREPARE_LOG_FILE_SIZE, RoleGroupName, STACKABLE_LOG_CONFIG_DIR, STACKABLE_LOG_DIR, TrinoRoleGroupConfig, ValidatedCluster, build::{ - self, command, + self, command, object_meta, resource::listener::{ LISTENER_VOLUME_DIR, LISTENER_VOLUME_NAME, build_group_listener_pvc, group_listener_name, secret_volume_listener_scope, @@ -442,14 +442,14 @@ pub fn build_rolegroup_statefulset( ); Ok(StatefulSet { - metadata: cluster - .object_meta( - resource_names.stateful_set_name().to_string(), - cluster.recommended_labels(trino_role, role_group_name), - ) - .with_label(RESTART_CONTROLLER_ENABLED_LABEL.to_owned()) - .with_annotations(annotations) - .build(), + metadata: object_meta( + cluster, + resource_names.stateful_set_name().to_string(), + cluster.recommended_labels(trino_role, role_group_name), + ) + .with_label(RESTART_CONTROLLER_ENABLED_LABEL.to_owned()) + .with_annotations(annotations) + .build(), spec: Some(StatefulSetSpec { pod_management_policy: Some("Parallel".to_string()), // Forward `None` when the user did not set `replicas`, leaving the field unset on the diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 237d43a3..551b9737 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -1,7 +1,6 @@ use std::{collections::BTreeMap, str::FromStr}; use stackable_operator::{ - builder::meta::ObjectMetaBuilder, commons::{ affinity::StackableAffinity, product_image_selection::ResolvedProductImage, @@ -20,7 +19,6 @@ use stackable_operator::{ shared::time::Duration, v2::{ HasName, HasUid, NameIsValidLabelValue, - builder::meta::ownerreference_from_resource, kvp::label::{recommended_labels, role_group_selector}, role_group_utils::ResourceNames, role_utils, @@ -268,25 +266,6 @@ impl ValidatedCluster { ) } - /// Returns an [`ObjectMetaBuilder`] pre-filled with this cluster's namespace, an owner - /// reference back to the cluster, the resource `name` and the given `recommended_labels`. - /// - /// Consolidates the metadata chain repeated by the child-resource builders. Call sites that - /// need extra labels/annotations chain them onto the returned builder. - pub(crate) fn object_meta( - &self, - name: impl Into, - recommended_labels: Labels, - ) -> ObjectMetaBuilder { - let mut builder = ObjectMetaBuilder::new(); - builder - .name_and_namespace(self) - .name(name) - .ownerreference(ownerreference_from_resource(self, None, Some(true))) - .with_labels(recommended_labels); - builder - } - /// A [`TrinoRole`] as a type-safe [`RoleName`]. fn recommended_labels_with( &self, From c0087661394da6d1c7d92a214e1a7622988b5c72 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 27 Jul 2026 15:30:29 +0200 Subject: [PATCH 7/8] make the coordinator and worker roles required in the CRD --- CHANGELOG.md | 2 + extra/crds.yaml | 4 +- rust/operator-binary/src/config/jvm.rs | 8 ++ .../src/controller/validate.rs | 18 +--- rust/operator-binary/src/crd/mod.rs | 102 ++++++++++++------ 5 files changed, 84 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87df1568..56cd009a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ All notable changes to this project will be documented in this file. - Bump `stackable-operator` to 0.114.0 ([#918]). - The RBAC ServiceAccount and RoleBinding are now built with the operator-rs `v2::rbac` functions and carry the full set of recommended labels ([#913]). +- BREAKING: The `coordinators` and `workers` roles are now required by the CRD. + Previously a TrinoCluster missing either role was accepted by the API server but failed reconciliation ([#913]). [#909]: https://github.com/stackabletech/trino-operator/pull/909 [#913]: https://github.com/stackabletech/trino-operator/pull/913 diff --git a/extra/crds.yaml b/extra/crds.yaml index 9ceb2b2a..110b4755 100644 --- a/extra/crds.yaml +++ b/extra/crds.yaml @@ -973,7 +973,6 @@ spec: at role level, the `roleConfig`. You can learn more about this in the [Roles and role group concept documentation](https://docs.stackable.tech/home/nightly/concepts/roles-and-role-groups). - nullable: true properties: cliOverrides: additionalProperties: @@ -2253,7 +2252,6 @@ spec: at role level, the `roleConfig`. You can learn more about this in the [Roles and role group concept documentation](https://docs.stackable.tech/home/nightly/concepts/roles-and-role-groups). - nullable: true properties: cliOverrides: additionalProperties: @@ -3441,7 +3439,9 @@ spec: type: object required: - clusterConfig + - coordinators - image + - workers type: object status: nullable: true diff --git a/rust/operator-binary/src/config/jvm.rs b/rust/operator-binary/src/config/jvm.rs index 77692c50..d51dd92c 100644 --- a/rust/operator-binary/src/config/jvm.rs +++ b/rust/operator-binary/src/config/jvm.rs @@ -200,6 +200,10 @@ mod tests { roleGroups: default: replicas: 1 + workers: + roleGroups: + default: + replicas: 1 "#; let jvm_config = construct_jvm_config(input); @@ -259,6 +263,10 @@ mod tests { add: - -Xmx40000m - -Dhttps.proxyPort=1234 + workers: + roleGroups: + default: + replicas: 1 "#; let jvm_config = construct_jvm_config(input); diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index e60966bc..d128c1ad 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -76,12 +76,6 @@ pub enum Error { ))] ClientSpoolingProtocolTrinoVersion { product_version: String }, - #[snafu(display("object defines no {role:?} role"))] - MissingTrinoRole { - source: crate::crd::Error, - role: String, - }, - #[snafu(display("invalid role group name {role_group}"))] ParseRoleGroupName { source: stackable_operator::v2::macros::attributed_string_type::Error, @@ -224,11 +218,7 @@ pub fn validate( let mut role_group_configs: BTreeMap> = BTreeMap::new(); for trino_role in TrinoRole::iter() { - let role = trino - .role(&trino_role) - .with_context(|_| MissingTrinoRoleSnafu { - role: trino_role.to_string(), - })?; + 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. @@ -237,8 +227,8 @@ pub fn validate( ValidatedRoleConfig { pdb: trino .generic_role_config(&trino_role) - .map(|rc| rc.pod_disruption_budget.clone()) - .unwrap_or_default(), + .pod_disruption_budget + .clone(), listener_class: trino_role.listener_class_name(trino), }, ); @@ -358,7 +348,7 @@ pub(crate) fn merged_role_group_config( role_group: &str, trino_catalogs: &[crate::crd::catalog::v1alpha1::TrinoCatalog], ) -> TrinoRoleGroupConfig { - let role = trino.role(trino_role).expect("role should be defined"); + let role = trino.role(trino_role); let default_config = v1alpha1::TrinoConfig::default_config(&trino.name_any(), trino_role, trino_catalogs); let rg = role diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index db6c0df6..0a47bd41 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -9,7 +9,6 @@ use std::{collections::BTreeMap, str::FromStr}; use affinity::get_affinity; use serde::{Deserialize, Serialize}; -use snafu::{OptionExt, Snafu}; use stackable_operator::{ commons::{ affinity::StackableAffinity, @@ -108,12 +107,6 @@ pub(crate) fn quantity_to_trino_bytes( Ok(format!("{bytes}B")) } -#[derive(Snafu, Debug)] -pub enum Error { - #[snafu(display("the role {role} is not defined"))] - CannotRetrieveTrinoRole { role: String }, -} - #[versioned( version(name = "v1alpha1"), crates( @@ -156,12 +149,10 @@ pub mod versioned { pub object_overrides: ObjectOverrides, // no doc - it's in the struct. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub coordinators: Option, + pub coordinators: super::TrinoCoordinatorRoleType, // no doc - it's in the struct. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub workers: Option, + pub workers: super::TrinoRoleType, } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -435,11 +426,7 @@ impl From<&TrinoRole> for RoleName { impl TrinoRole { pub fn listener_class_name(&self, trino: &v1alpha1::TrinoCluster) -> Option { match self { - Self::Coordinator => trino - .spec - .coordinators - .to_owned() - .map(|coordinator| coordinator.role_config.listener_class), + Self::Coordinator => Some(trino.spec.coordinators.role_config.listener_class.clone()), Self::Worker => None, } } @@ -517,29 +504,20 @@ impl v1alpha1::TrinoConfig { } impl v1alpha1::TrinoCluster { - /// Returns a reference to the role. Raises an error if the role is not defined. - pub fn role(&self, role_variant: &TrinoRole) -> Result { + /// Returns the given role (both roles are required by the CRD). + pub fn role(&self, role_variant: &TrinoRole) -> TrinoRoleType { match role_variant { - TrinoRole::Coordinator => self - .spec - .coordinators - .to_owned() - .map(extract_role_from_coordinator_config), + TrinoRole::Coordinator => { + extract_role_from_coordinator_config(self.spec.coordinators.to_owned()) + } TrinoRole::Worker => self.spec.workers.to_owned(), } - .with_context(|| CannotRetrieveTrinoRoleSnafu { - role: role_variant.to_string(), - }) } - pub fn generic_role_config(&self, role: &TrinoRole) -> Option<&GenericRoleConfig> { + pub fn generic_role_config(&self, role: &TrinoRole) -> &GenericRoleConfig { match role { - TrinoRole::Coordinator => self - .spec - .coordinators - .as_ref() - .map(|c| &c.role_config.common), - TrinoRole::Worker => self.spec.workers.as_ref().map(|w| &w.role_config), + TrinoRole::Coordinator => &self.spec.coordinators.role_config.common, + TrinoRole::Worker => &self.spec.workers.role_config, } } @@ -558,8 +536,8 @@ impl v1alpha1::TrinoCluster { .expect("the coordinator role name is a valid role name"); self.spec .coordinators + .role_groups .iter() - .flat_map(|role| &role.role_groups) // Order rolegroups consistently, to avoid spurious downstream rewrites .collect::>() .into_iter() @@ -654,6 +632,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} "#; @@ -673,6 +659,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: @@ -694,6 +688,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: @@ -713,6 +715,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: @@ -737,6 +747,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} "#; @@ -756,6 +774,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: @@ -777,6 +803,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: From e43332c70ae7ee937f21acce7f99dd9d5c8dbe27 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 27 Jul 2026 15:34:26 +0200 Subject: [PATCH 8/8] improve unit-test coverage for all derived values --- .../src/controller/validate.rs | 90 +++++++++++++++---- 1 file changed, 75 insertions(+), 15 deletions(-) diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index d128c1ad..ee84bdce 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -377,7 +377,10 @@ mod tests { use super::{Error, RoleGroupName, validate}; use crate::{ config::client_protocol::ResolvedClientProtocolConfig, - controller::{ValidatedCluster, dereference::DereferencedObjects, validated_cluster}, + controller::{ + ValidatedCluster, app_version_label, dereference::DereferencedObjects, + validated_cluster, + }, crd::{TrinoRole, v1alpha1}, }; @@ -432,8 +435,15 @@ mod tests { ) } + /// Locks every value the validate step itself derives from the minimal fixture — so a + /// validation regression fails here, with a validate-shaped message, instead of surfacing as + /// a confusing build-test failure downstream. + /// + /// The merged per-role-group config (resources, affinity, logging defaults, …) is produced by + /// the config merge machinery, whose contracts are tested in operator-rs and the properties + /// tests; only the values this module derives on top are re-asserted here. #[test] - fn validate_minimal_cluster() { + fn validate_ok_derives_expected_values() { let validated = validated_cluster(); assert_eq!(validated.name.to_string(), "simple-trino"); @@ -442,24 +452,74 @@ mod tests { validated.uid.to_string(), "e6ac237d-a6d4-43a1-8135-f36506110912" ); + assert_eq!( + validated.image.image, + format!("oci.example.org/trino:{}", app_version_label("481")) + ); + assert_eq!(validated.image.product_version, "481"); assert_eq!(validated.numeric_product_version, 481); - assert!(!validated.cluster_config.authentication_enabled()); - assert!( - validated - .role_group_configs - .contains_key(&TrinoRole::Coordinator) + assert_eq!( + validated.product_version.to_string(), + app_version_label("481") + ); + + // TLS defaults to the `tls` SecretClass for both server and internal traffic; the + // minimal fixture has no authentication, authorization, FTE or client-protocol config, + // and no catalogs. + let cluster_config = &validated.cluster_config; + assert_eq!( + cluster_config.tls.server.as_ref().map(ToString::to_string), + Some("tls".to_string()) + ); + assert_eq!( + cluster_config + .tls + .internal + .as_ref() + .map(ToString::to_string), + Some("tls".to_string()) + ); + assert!(!cluster_config.authentication_enabled()); + assert!(cluster_config.authorization.is_none()); + assert!(cluster_config.fault_tolerant_execution.is_none()); + assert!(cluster_config.client_protocol.is_none()); + assert!(cluster_config.catalogs.is_empty()); + + // One coordinator pod ref (a single `default` role group with one replica), predicted + // for the discovery config of all pods. + assert_eq!(cluster_config.coordinator_pod_refs.len(), 1); + assert_eq!( + cluster_config.coordinator_pod_refs[0].pod_name, + "simple-trino-coordinator-default-0" ); - assert!( - validated - .role_group_configs - .contains_key(&TrinoRole::Worker) + + // 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); + } + assert_eq!( + validated.role_configs[&TrinoRole::Coordinator] + .listener_class + .as_ref() + .map(ToString::to_string), + Some("cluster-internal".to_string()) ); assert_eq!( - validated.role_group_configs[&TrinoRole::Coordinator] - [&RoleGroupName::from_str("default").unwrap()] - .replicas, - Some(1) + validated.role_configs[&TrinoRole::Worker].listener_class, + None ); + + // 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]; + assert_eq!(role_group.replicas, Some(1)); + assert!(!role_group.config.logging.enable_vector_agent); + assert_eq!(role_group.config.logging.vector_container, None); + } } #[test]