diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c419f58..56cd009a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,13 @@ 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]). +- 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 [#918]: https://github.com/stackabletech/trino-operator/pull/918 ## [26.7.0] - 2026-07-21 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/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/build.rs b/rust/operator-binary/src/controller/build.rs index 7f4e4109..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::{ @@ -13,6 +16,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 +56,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 +115,6 @@ pub fn build( role, role_group_name, role_group_config, - service_account_name, ) .context(StatefulSetSnafu { role_group: role_group_name.clone(), @@ -147,9 +146,30 @@ pub fn build( listeners, config_maps, pod_disruption_budgets, + service_accounts: vec![build_service_account(cluster)], + role_bindings: vec![build_role_binding(cluster)], }) } +/// 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::{ @@ -177,8 +197,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!( @@ -218,5 +237,14 @@ mod tests { sorted_names(&resources.pod_disruption_budgets), ["simple-trino-coordinator", "simple-trino-worker"] ); + // The cluster-shared RBAC pair. + assert_eq!( + sorted_names(&resources.service_accounts), + ["simple-trino-serviceaccount"] + ); + assert_eq!( + sorted_names(&resources.role_bindings), + ["simple-trino-rolebinding"] + ); } } 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..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, @@ -74,7 +77,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(); @@ -164,7 +167,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, ) @@ -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/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/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/build/resource/rbac.rs b/rust/operator-binary/src/controller/build/resource/rbac.rs new file mode 100644 index 00000000..4f7d935e --- /dev/null +++ b/rust/operator-binary/src/controller/build/resource/rbac.rs @@ -0,0 +1,135 @@ +//! 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.cluster_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.cluster_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) +} + +#[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 00607d3d..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 - .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 - .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()), @@ -102,3 +105,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/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 20668630..a7c7145e 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, object_meta, 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; @@ -150,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(); @@ -422,7 +421,12 @@ pub fn build_rolegroup_statefulset( )), ) .context(AddVolumeSnafu)? - .service_account_name(sa_name) + .service_account_name( + cluster + .cluster_resource_names() + .service_account_name() + .to_string(), + ) .security_context(PodSecurityContextBuilder::new().fs_group(1000).build()); let mut pod_template = pod_builder.build_template(); @@ -438,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 7e00d944..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, @@ -10,8 +9,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, @@ -19,9 +19,9 @@ 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, types::{ kubernetes::{ListenerClassName, NamespaceName, SecretClassName, Uid}, operator::{ @@ -64,6 +64,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 +74,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 +158,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 +175,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 +190,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,15 +230,24 @@ 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 cluster_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( + pub(crate) fn role_group_resource_names( &self, role: &TrinoRole, role_group_name: &RoleGroupName, ) -> ResourceNames { ResourceNames { cluster_name: self.name.clone(), - role_name: Self::role_name(role), + role_name: role.into(), role_group_name: role_group_name.clone(), } } @@ -241,45 +261,16 @@ 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() ) } - /// 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 role_name(role: &TrinoRole) -> RoleName { - 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,44 +279,39 @@ 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(&role.into(), 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, &role.into(), 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 { - role_group_selector( - self, - &product_name(), - &Self::role_name(role), - role_group_name, - ) + pub fn role_group_selector(&self, role: &TrinoRole, role_group_name: &RoleGroupName) -> Labels { + role_group_selector(self, &product_name(), &role.into(), role_group_name) } } @@ -391,7 +377,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 @@ -445,3 +447,21 @@ pub(crate) fn validated_cluster() -> ValidatedCluster { validate::validate(&minimal_trino(), &derefs, &operator_env).expect("validate should succeed") } + +#[cfg(test)] +mod tests { + use stackable_operator::v2::types::operator::RoleName; + use strum::IntoEnumIterator; + + use crate::crd::TrinoRole; + + /// 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() { + let _: RoleName = (&role).into(); + let _: RoleName = role.into(); + } + } +} diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 21d82b90..ee84bdce 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, @@ -188,7 +182,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(), })?; @@ -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), }, ); @@ -310,7 +300,7 @@ pub fn validate( namespace, uid, image, - product_version, + numeric_product_version, cluster_config, role_configs, role_group_configs, @@ -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 @@ -387,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}, }; @@ -442,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"); @@ -452,24 +452,74 @@ mod tests { validated.uid.to_string(), "e6ac237d-a6d4-43a1-8135-f36506110912" ); - assert_eq!(validated.product_version, 481); - assert!(!validated.cluster_config.authentication_enabled()); - assert!( - validated - .role_group_configs - .contains_key(&TrinoRole::Coordinator) + 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_eq!( + validated.product_version.to_string(), + app_version_label("481") ); - assert!( - validated - .role_group_configs - .contains_key(&TrinoRole::Worker) + + // 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!( - validated.role_group_configs[&TrinoRole::Coordinator] - [&RoleGroupName::from_str("default").unwrap()] - .replicas, - Some(1) + 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" + ); + + // 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_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] diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 5661b002..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)] @@ -420,14 +411,22 @@ 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 { - 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, } } @@ -505,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, } } @@ -546,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() @@ -642,6 +632,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} "#; @@ -661,6 +659,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: @@ -682,6 +688,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: @@ -701,6 +715,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: @@ -725,6 +747,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} "#; @@ -744,6 +774,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: @@ -765,6 +803,14 @@ mod tests { spec: image: productVersion: "481" + coordinators: + roleGroups: + default: + replicas: 1 + workers: + roleGroups: + default: + replicas: 1 clusterConfig: catalogLabelSelector: {} tls: 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)