From 8ff2aea9ddaee17120e3a3b82eaf9b11dc700430 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Mon, 20 Jul 2026 18:14:18 +0200 Subject: [PATCH 1/6] refactor connect server service account and role binding --- Cargo.toml | 2 +- .../operator-binary/src/connect/controller.rs | 32 +------- .../src/connect/controller/build/mod.rs | 32 ++++---- .../src/connect/controller/build/rbac.rs | 29 +++++++ .../src/connect/controller/build/server.rs | 13 +--- .../src/connect/controller/validate.rs | 75 +++++++++++++------ 6 files changed, 105 insertions(+), 78 deletions(-) create mode 100644 rust/operator-binary/src/connect/controller/build/rbac.rs diff --git a/Cargo.toml b/Cargo.toml index c80ce727..3cf6fd6e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,5 +31,5 @@ indoc = "2" regex = "1.12" [patch."https://github.com/stackabletech/operator-rs.git"] -# stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch="main" } +stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch="feat/smooth-operator/build-rbac" } # stackable-operator = { path = "../operator-rs/crates/stackable-operator" } diff --git a/rust/operator-binary/src/connect/controller.rs b/rust/operator-binary/src/connect/controller.rs index 70b67ec4..fddf9303 100644 --- a/rust/operator-binary/src/connect/controller.rs +++ b/rust/operator-binary/src/connect/controller.rs @@ -3,7 +3,6 @@ use std::sync::Arc; use snafu::{ResultExt, Snafu}; use stackable_operator::{ cluster_resources::ClusterResourceApplyStrategy, - commons::rbac::build_rbac_resources, crd::listener, k8s_openapi::api::{ apps::v1::StatefulSet, @@ -25,7 +24,7 @@ use stackable_operator::{ }; use strum::{EnumDiscriminants, IntoStaticStr}; -use super::crd::{CONNECT_APP_NAME, v1alpha1}; +use super::crd::v1alpha1; use crate::{Ctx, connect::crd::SparkConnectServerStatus, crd::constants::OPERATOR_NAME}; pub mod build; @@ -44,16 +43,6 @@ pub enum Error { source: stackable_operator::cluster_resources::Error, }, - #[snafu(display("failed to apply role ServiceAccount"))] - ApplyServiceAccount { - source: stackable_operator::cluster_resources::Error, - }, - - #[snafu(display("failed to apply global RoleBinding"))] - ApplyRoleBinding { - source: stackable_operator::cluster_resources::Error, - }, - #[snafu(display("failed to update status of spark connect server {name}"))] ApplyStatus { source: stackable_operator::client::Error, @@ -150,20 +139,7 @@ pub async fn reconcile( &scs.spec.object_overrides, ); - // Use a dedicated service account for connect server pods. Building the RBAC resources needs - // the cluster-resource labels, so it stays in the reconcile step; the built objects (whose - // names are deterministic) are handed to the client-free build step. - let (service_account, role_binding) = build_rbac_resources( - scs, - CONNECT_APP_NAME, - cluster_resources - .get_required_labels() - .context(GetRequiredLabelsSnafu)?, - ) - .context(BuildRbacResourcesSnafu)?; - - let resources = build::build(&validated, service_account, role_binding, &scs.spec.args) - .context(BuildResourcesSnafu)?; + let resources = build::build(&validated, &scs.spec.args).context(BuildResourcesSnafu)?; // Apply order: ServiceAccount and RoleBinding first, then the Services, ConfigMaps and // Listener, and finally the StatefulSet (it mounts the ConfigMaps and runs under the SA, so @@ -171,11 +147,11 @@ pub async fn reconcile( cluster_resources .add(client, resources.service_account) .await - .context(ApplyServiceAccountSnafu)?; + .context(ApplyResourceSnafu)?; cluster_resources .add(client, resources.role_binding) .await - .context(ApplyRoleBindingSnafu)?; + .context(ApplyResourceSnafu)?; for service in resources.services { cluster_resources .add(client, service) diff --git a/rust/operator-binary/src/connect/controller/build/mod.rs b/rust/operator-binary/src/connect/controller/build/mod.rs index da79b9b9..3aa4f8f7 100644 --- a/rust/operator-binary/src/connect/controller/build/mod.rs +++ b/rust/operator-binary/src/connect/controller/build/mod.rs @@ -6,18 +6,20 @@ //! together in one module is clearer than scattering them across per-kind modules. pub(crate) mod executor; +pub(crate) mod rbac; pub(crate) mod server; pub(crate) mod service; use snafu::{ResultExt, Snafu}; -use stackable_operator::{ - k8s_openapi::api::{core::v1::ServiceAccount, rbac::v1::RoleBinding}, - kube::ResourceExt, -}; +use stackable_operator::kube::ResourceExt; use crate::connect::{ common, - controller::{SparkConnectResources, validate::ValidatedSparkConnectServer}, + controller::{ + SparkConnectResources, + build::rbac::{build_role_binding, build_service_account}, + validate::ValidatedSparkConnectServer, + }, }; #[derive(Snafu, Debug)] @@ -56,8 +58,6 @@ pub enum Error { /// Builds every Kubernetes resource for the given validated SparkConnectServer. pub(crate) fn build( validated: &ValidatedSparkConnectServer, - service_account: ServiceAccount, - role_binding: RoleBinding, user_args: &[String], ) -> Result { let resolved_s3 = &validated.cluster_config.resolved_s3; @@ -70,8 +70,7 @@ pub(crate) fn build( resolved_s3 .spark_properties() .context(S3SparkPropertiesSnafu)?, - server::server_properties(validated, &headless_service, &service_account) - .context(ServerPropertiesSnafu)?, + server::server_properties(validated, &headless_service).context(ServerPropertiesSnafu)?, executor::executor_properties(validated).context(ExecutorPropertiesSnafu)?, ]) .context(SerializePropertiesSnafu)?; @@ -97,18 +96,13 @@ pub(crate) fn build( let listener = server::build_listener(validated); let args = server::command_args(user_args); - let stateful_set = server::build_stateful_set( - validated, - &service_account, - &server_config_map, - &listener.name_any(), - args, - ) - .context(BuildServerStatefulSetSnafu)?; + let stateful_set = + server::build_stateful_set(validated, &server_config_map, &listener.name_any(), args) + .context(BuildServerStatefulSetSnafu)?; Ok(SparkConnectResources { - service_account, - role_binding, + service_account: build_service_account(validated), + role_binding: build_role_binding(validated), services: vec![headless_service, metrics_service], config_maps: vec![executor_config_map, server_config_map], listener, diff --git a/rust/operator-binary/src/connect/controller/build/rbac.rs b/rust/operator-binary/src/connect/controller/build/rbac.rs new file mode 100644 index 00000000..0d3f4123 --- /dev/null +++ b/rust/operator-binary/src/connect/controller/build/rbac.rs @@ -0,0 +1,29 @@ +//! 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::connect::controller::validate::ValidatedSparkConnectServer; + +stackable_operator::constant!(NONE_ROLE_NAME: RoleName = "none"); +stackable_operator::constant!(NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); + +pub fn build_service_account(server: &ValidatedSparkConnectServer) -> ServiceAccount { + rbac::build_service_account(server, &server.rbac_resource_names(), rbac_labels(server)) +} + +pub fn build_role_binding(server: &ValidatedSparkConnectServer) -> RoleBinding { + rbac::build_role_binding(server, &server.rbac_resource_names(), rbac_labels(server)) +} + +fn rbac_labels(server: &ValidatedSparkConnectServer) -> Labels { + server.recommended_labels_for(&NONE_ROLE_NAME, &NONE_ROLE_GROUP_NAME) +} diff --git a/rust/operator-binary/src/connect/controller/build/server.rs b/rust/operator-binary/src/connect/controller/build/server.rs index 8ab09233..3c152d04 100644 --- a/rust/operator-binary/src/connect/controller/build/server.rs +++ b/rust/operator-binary/src/connect/controller/build/server.rs @@ -23,10 +23,7 @@ use stackable_operator::{ DeepMerge, api::{ apps::v1::{StatefulSet, StatefulSetSpec}, - core::v1::{ - ConfigMap, EnvVar, HTTPGetAction, PodSecurityContext, Probe, Service, - ServiceAccount, - }, + core::v1::{ConfigMap, EnvVar, HTTPGetAction, PodSecurityContext, Probe, Service}, }, apimachinery::pkg::{apis::meta::v1::LabelSelector, util::intstr::IntOrString}, }, @@ -176,7 +173,6 @@ pub(crate) fn server_config_map( pub(crate) fn build_stateful_set( validated: &ValidatedSparkConnectServer, - service_account: &ServiceAccount, config_map: &ConfigMap, listener_name: &str, args: Vec, @@ -194,7 +190,7 @@ pub(crate) fn build_stateful_set( let mut pb = PodBuilder::new(); - pb.service_account_name(service_account.name_unchecked()) + pb.service_account_name(validated.rbac_resource_names().service_account_name()) .metadata(metadata) .image_pull_secrets_from_product_image(resolved_product_image) .add_volume( @@ -402,13 +398,12 @@ fn env(env_overrides: Option<&HashMap>) -> Result, E pub(crate) fn server_properties( validated: &ValidatedSparkConnectServer, driver_service: &Service, - service_account: &ServiceAccount, ) -> Result>, Error> { let config = &validated.server_config; let resolved_product_image = &validated.resolved_product_image; let spark_image = resolved_product_image.image.clone(); let spark_version = resolved_product_image.product_version.clone(); - let service_account_name = service_account.name_unchecked(); + let service_account_name = validated.rbac_resource_names().service_account_name(); let namespace = driver_service .namespace() .context(ObjectHasNoNamespaceSnafu)?; @@ -434,7 +429,7 @@ pub(crate) fn server_properties( ("spark.kubernetes.namespace".to_string(), Some(namespace)), ( "spark.kubernetes.authenticate.driver.serviceAccountName".to_string(), - Some(service_account_name), + Some(service_account_name.to_string()), ), ( "spark.kubernetes.driver.pod.name".to_string(), diff --git a/rust/operator-binary/src/connect/controller/validate.rs b/rust/operator-binary/src/connect/controller/validate.rs index c1a4de67..23ebe97c 100644 --- a/rust/operator-binary/src/connect/controller/validate.rs +++ b/rust/operator-binary/src/connect/controller/validate.rs @@ -22,7 +22,7 @@ use stackable_operator::{ product_logging::framework::{ VectorContainerLogConfig, validate_logging_configuration_for_container, }, - role_utils::JavaCommonConfig, + role_utils::{self, JavaCommonConfig}, types::{ kubernetes::{ConfigMapName, ListenerClassName, NamespaceName, Uid}, operator::{ @@ -118,6 +118,10 @@ fn validate_logging( type Result = std::result::Result; +stackable_operator::constant!( + DEFAULT_SPARK_CONNECT_ROLE_GROUP: RoleGroupName = DEFAULT_SPARK_CONNECT_GROUP_NAME +); + /// Validated logging configuration for the (optional) Vector container. /// /// Produced up-front by [`validate_logging`] so that an @@ -134,6 +138,7 @@ pub struct ValidatedSparkConnectServer { pub name: ClusterName, pub namespace: NamespaceName, pub uid: Uid, + pub product_version: ProductVersion, pub resolved_product_image: ResolvedProductImage, pub cluster_config: ValidatedClusterConfig, pub role_config: ValidatedRoleConfig, @@ -167,41 +172,56 @@ pub struct ValidatedRoleConfig { } impl ValidatedSparkConnectServer { + /// Recommended labels for a resource that is not tied to a concrete [`SparkConnectRole`] + /// (e.g. the cluster-shared RBAC resources), using a free-form role/role-group label value. + pub fn recommended_labels_for( + &self, + role_name: &RoleName, + role_group_name: &RoleGroupName, + ) -> Labels { + self.recommended_labels_with(&self.product_version, role_name, role_group_name) + } + /// Recommended labels for a resource of the given role. - pub(crate) fn recommended_labels(&self, role: SparkConnectRole) -> Labels { - // `app_version_label_value` is constructed to be a valid label value, so it is also a - // valid `ProductVersion`. - let product_version = - ProductVersion::from_str(&self.resolved_product_image.app_version_label_value) - .expect("the app version label value is a valid product version"); - let role_group = RoleGroupName::from_str(DEFAULT_SPARK_CONNECT_GROUP_NAME) - .expect("DEFAULT_SPARK_CONNECT_GROUP_NAME is a valid role group name"); + pub fn recommended_labels(&self, role: SparkConnectRole) -> Labels { + self.recommended_labels_for(&role_name(role), &DEFAULT_SPARK_CONNECT_ROLE_GROUP) + } + + fn recommended_labels_with( + &self, + product_version: &ProductVersion, + role_name: &RoleName, + role_group_name: &RoleGroupName, + ) -> Labels { recommended_labels( self, &product_name(), - &product_version, + product_version, &operator_name(), &controller_name(), - &role_name(role), - &role_group, + role_name, + role_group_name, ) } /// Selector labels matching the pods of the given role. - pub(crate) fn role_selector(&self, role: SparkConnectRole) -> Labels { + pub fn role_selector(&self, role: SparkConnectRole) -> Labels { role_selector(self, &product_name(), &role_name(role)) } /// Selector labels matching the pods of the given role's (single) role group. - pub(crate) fn role_group_selector(&self, role: SparkConnectRole) -> Labels { - let role_group = RoleGroupName::from_str(DEFAULT_SPARK_CONNECT_GROUP_NAME) - .expect("DEFAULT_SPARK_CONNECT_GROUP_NAME is a valid role group name"); - role_group_selector(self, &product_name(), &role_name(role), &role_group) + pub fn role_group_selector(&self, role: SparkConnectRole) -> Labels { + role_group_selector( + self, + &product_name(), + &role_name(role), + &DEFAULT_SPARK_CONNECT_ROLE_GROUP, + ) } /// Object metadata for a child resource named `name`, owned by this SparkConnectServer and /// carrying the recommended labels for the given role. - pub(crate) fn object_meta( + pub fn object_meta( &self, name: impl Into, role: SparkConnectRole, @@ -214,20 +234,29 @@ impl ValidatedSparkConnectServer { .with_labels(self.recommended_labels(role)); builder } + + /// Type-safe names for the per-cluster RBAC resources: the ServiceAccount, + /// 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(), + } + } } /// The product name (`spark-connect`) as a type-safe label value. -pub(crate) fn product_name() -> ProductName { +pub fn product_name() -> ProductName { ProductName::from_str(CONNECT_APP_NAME).expect("CONNECT_APP_NAME is a valid product name") } /// The operator name as a type-safe label value. -pub(crate) fn operator_name() -> OperatorName { +pub fn operator_name() -> OperatorName { OperatorName::from_str(OPERATOR_NAME).expect("the operator name is a valid label value") } /// The controller name as a type-safe label value. -pub(crate) fn controller_name() -> ControllerName { +pub fn controller_name() -> ControllerName { ControllerName::from_str(CONNECT_CONTROLLER_NAME) .expect("the controller name is a valid label value") } @@ -304,6 +333,9 @@ pub fn validate( ) .context(ResolveProductImageSnafu)?; + let product_version = ProductVersion::from_str(&resolved_product_image.app_version_label_value) + .expect("the app version label value is a valid product version"); + let server_config = scs.server_config().context(ServerConfigSnafu)?; let executor_config = scs.executor_config().context(ExecutorConfigSnafu)?; @@ -350,6 +382,7 @@ pub fn validate( name, namespace, uid, + product_version, resolved_product_image, cluster_config: ValidatedClusterConfig { resolved_s3: dereferenced.resolved_s3, From 572b3d4044f89c8c8e16905f9b8c071a52af0e86 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 21 Jul 2026 11:41:24 +0200 Subject: [PATCH 2/6] refactor history server service account and role binding, added tests --- .../operator-binary/src/connect/controller.rs | 5 - .../src/connect/controller/build/mod.rs | 95 +++++++++++++++ rust/operator-binary/src/connect/s3.rs | 10 ++ .../operator-binary/src/history/controller.rs | 46 +------ .../src/history/controller/build/mod.rs | 114 ++++++++++++++++-- .../history/controller/build/resource/mod.rs | 1 + .../history/controller/build/resource/rbac.rs | 29 +++++ .../controller/build/resource/statefulset.rs | 77 ++++++------ .../src/history/controller/validate.rs | 47 +++++--- 9 files changed, 317 insertions(+), 107 deletions(-) create mode 100644 rust/operator-binary/src/history/controller/build/resource/rbac.rs diff --git a/rust/operator-binary/src/connect/controller.rs b/rust/operator-binary/src/connect/controller.rs index fddf9303..2a7666fb 100644 --- a/rust/operator-binary/src/connect/controller.rs +++ b/rust/operator-binary/src/connect/controller.rs @@ -65,11 +65,6 @@ pub enum Error { source: error_boundary::InvalidObject, }, - #[snafu(display("failed to build RBAC resources"))] - BuildRbacResources { - source: stackable_operator::commons::rbac::Error, - }, - #[snafu(display("failed to dereference SparkConnectServer"))] DereferenceSparkConnectServer { source: dereference::Error }, diff --git a/rust/operator-binary/src/connect/controller/build/mod.rs b/rust/operator-binary/src/connect/controller/build/mod.rs index 3aa4f8f7..3ea6b607 100644 --- a/rust/operator-binary/src/connect/controller/build/mod.rs +++ b/rust/operator-binary/src/connect/controller/build/mod.rs @@ -109,3 +109,98 @@ pub(crate) fn build( stateful_set, }) } + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use indoc::indoc; + use stackable_operator::{cli::OperatorEnvironmentOptions, utils::yaml_from_str_singleton_map}; + + use super::build; + use crate::connect::{ + controller::{ + dereference::DereferencedSparkConnectServer, + validate::{ValidatedSparkConnectServer, validate}, + }, + crd::v1alpha1, + s3::ResolvedS3, + }; + + /// Minimal S3-free `SparkConnectServer` fixture, keeping the dereference step client-free; + /// the `uid` allows owner references to be derived from it. + const CONNECT_YAML: &str = indoc! {r#" + apiVersion: spark.stackable.tech/v1alpha1 + kind: SparkConnectServer + metadata: + name: my-connect + namespace: default + uid: 12345678-1234-1234-1234-123456789012 + spec: + image: + productVersion: 4.1.2 + "#}; + + /// Runs the real validate step against the minimal fixture. + fn minimal_validated_cluster() -> ValidatedSparkConnectServer { + let scs: v1alpha1::SparkConnectServer = yaml_from_str_singleton_map(CONNECT_YAML) + .expect("invalid test SparkConnectServer YAML"); + validate( + &scs, + DereferencedSparkConnectServer { + resolved_s3: ResolvedS3::none(), + }, + &OperatorEnvironmentOptions { + operator_namespace: "stackable-operators".to_string(), + operator_service_name: "spark-k8s-operator".to_string(), + image_repository: "oci.example.org/sdp".to_string(), + }, + ) + .expect("validate should succeed for the test fixture") + } + + /// 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 resources = build(&minimal_validated_cluster(), &[]).expect("build succeeds"); + + assert_eq!( + resources.service_account.metadata.name.as_deref(), + Some("my-connect-serviceaccount") + ); + assert_eq!( + resources.role_binding.metadata.name.as_deref(), + Some("my-connect-rolebinding") + ); + + let expected_labels = BTreeMap::from( + [ + ("app.kubernetes.io/component", "none"), + ("app.kubernetes.io/instance", "my-connect"), + ( + "app.kubernetes.io/managed-by", + "spark.stackable.tech_connect", + ), + ("app.kubernetes.io/name", "spark-connect"), + ("app.kubernetes.io/role-group", "none"), + ("app.kubernetes.io/version", "4.1.2-stackable0.0.0-dev"), + ("stackable.tech/vendor", "Stackable"), + ] + .map(|(key, value)| (key.to_string(), value.to_string())), + ); + assert_eq!( + resources.service_account.metadata.labels, + Some(expected_labels.clone()) + ); + assert_eq!( + resources.role_binding.metadata.labels, + Some(expected_labels) + ); + assert_eq!( + resources.role_binding.role_ref.name, + "spark-connect-clusterrole" + ); + } +} diff --git a/rust/operator-binary/src/connect/s3.rs b/rust/operator-binary/src/connect/s3.rs index 624720dc..0e46f89f 100644 --- a/rust/operator-binary/src/connect/s3.rs +++ b/rust/operator-binary/src/connect/s3.rs @@ -54,6 +54,16 @@ pub(crate) struct ResolvedS3 { } impl ResolvedS3 { + /// The resolved form of a SparkConnectServer without any S3 connectors, for tests that must + /// stay client-free. + #[cfg(test)] + pub(crate) fn none() -> Self { + Self { + s3_buckets: Vec::new(), + s3_connection: None, + } + } + pub(crate) async fn resolve( client: &stackable_operator::client::Client, connect_server: &crd::v1alpha1::SparkConnectServer, diff --git a/rust/operator-binary/src/history/controller.rs b/rust/operator-binary/src/history/controller.rs index 250e8d30..bc7f46de 100644 --- a/rust/operator-binary/src/history/controller.rs +++ b/rust/operator-binary/src/history/controller.rs @@ -3,7 +3,6 @@ use std::sync::Arc; use snafu::{ResultExt, Snafu}; use stackable_operator::{ cluster_resources::ClusterResourceApplyStrategy, - commons::rbac::build_rbac_resources, crd::listener, k8s_openapi::api::{ apps::v1::StatefulSet, @@ -21,10 +20,7 @@ use stackable_operator::{ }; use strum::{EnumDiscriminants, IntoStaticStr}; -use crate::{ - Ctx, - crd::{constants::HISTORY_APP_NAME, history::v1alpha1}, -}; +use crate::{Ctx, crd::history::v1alpha1}; pub mod build; pub mod dereference; @@ -34,11 +30,6 @@ pub mod validate; #[strum_discriminants(derive(IntoStaticStr))] #[allow(clippy::enum_variant_names)] pub enum Error { - #[snafu(display("failed to build RBAC resources"))] - BuildRbacResources { - source: stackable_operator::commons::rbac::Error, - }, - #[snafu(display("failed to build SparkHistoryServer resources"))] BuildSparkHistoryServer { source: build::Error }, @@ -47,16 +38,6 @@ pub enum Error { source: stackable_operator::cluster_resources::Error, }, - #[snafu(display("failed to apply role ServiceAccount"))] - ApplyServiceAccount { - source: stackable_operator::cluster_resources::Error, - }, - - #[snafu(display("failed to apply global RoleBinding"))] - ApplyRoleBinding { - source: stackable_operator::cluster_resources::Error, - }, - #[snafu(display("failed to dereference SparkHistoryServer"))] DereferenceSparkHistoryServer { source: dereference::Error }, @@ -68,12 +49,6 @@ pub enum Error { source: stackable_operator::cluster_resources::Error, }, - #[snafu(display("failed to get required Labels"))] - GetRequiredLabels { - source: - stackable_operator::kvp::KeyValuePairError, - }, - #[snafu(display("SparkHistoryServer object is invalid"))] InvalidSparkHistoryServer { // boxed because otherwise Clippy warns about a large enum variant @@ -135,20 +110,7 @@ pub async fn reconcile( &shs.spec.object_overrides, ); - // Use a dedicated service account for history server pods. Building the RBAC resources needs - // the cluster-resource labels, so it stays in the reconcile step; the built objects (whose - // names are deterministic) are handed to the client-free build step. - let (service_account, role_binding) = build_rbac_resources( - shs, - HISTORY_APP_NAME, - cluster_resources - .get_required_labels() - .context(GetRequiredLabelsSnafu)?, - ) - .context(BuildRbacResourcesSnafu)?; - - let resources = build::build(&validated, service_account, role_binding) - .context(BuildSparkHistoryServerSnafu)?; + let resources = build::build(&validated).context(BuildSparkHistoryServerSnafu)?; // Apply order: ServiceAccount and RoleBinding first, then the ConfigMaps, metrics Services, // Listener and PodDisruptionBudget, and finally the StatefulSets (they mount the ConfigMaps @@ -156,11 +118,11 @@ pub async fn reconcile( cluster_resources .add(client, resources.service_account) .await - .context(ApplyServiceAccountSnafu)?; + .context(ApplyResourceSnafu)?; cluster_resources .add(client, resources.role_binding) .await - .context(ApplyRoleBindingSnafu)?; + .context(ApplyResourceSnafu)?; for config_map in resources.config_maps { cluster_resources .add(client, config_map) diff --git a/rust/operator-binary/src/history/controller/build/mod.rs b/rust/operator-binary/src/history/controller/build/mod.rs index e23d2609..3004ff51 100644 --- a/rust/operator-binary/src/history/controller/build/mod.rs +++ b/rust/operator-binary/src/history/controller/build/mod.rs @@ -1,7 +1,6 @@ pub mod resource; use snafu::{ResultExt, Snafu}; -use stackable_operator::k8s_openapi::api::{core::v1::ServiceAccount, rbac::v1::RoleBinding}; use crate::{ crd::constants::HISTORY_ROLE_NAME, @@ -11,6 +10,7 @@ use crate::{ config_map::{self, build_config_map}, listener::build_group_listener, pdb::build_pdb, + rbac::{build_role_binding, build_service_account}, service::build_rolegroup_metrics_service, statefulset::{self, build_stateful_set}, }, @@ -30,11 +30,7 @@ pub enum Error { type Result = std::result::Result; /// Builds every Kubernetes resource for the given validated SparkHistoryServer. -pub fn build( - validated: &ValidatedSparkHistoryServer, - service_account: ServiceAccount, - role_binding: RoleBinding, -) -> Result { +pub fn build(validated: &ValidatedSparkHistoryServer) -> Result { let log_dir = &validated.cluster_config.log_dir; let mut config_maps = vec![]; @@ -46,7 +42,7 @@ pub fn build( .push(build_config_map(validated, role_group_name, rg).context(BuildConfigMapSnafu)?); metrics_services.push(build_rolegroup_metrics_service(validated, role_group_name)); stateful_sets.push( - build_stateful_set(validated, role_group_name, rg, log_dir, &service_account) + build_stateful_set(validated, role_group_name, rg, log_dir) .context(BuildStatefulSetSnafu)?, ); } @@ -60,8 +56,8 @@ pub fn build( let pod_disruption_budget = build_pdb(&validated.role_config.pdb, validated); Ok(SparkHistoryResources { - service_account, - role_binding, + service_account: build_service_account(validated), + role_binding: build_role_binding(validated), config_maps, metrics_services, stateful_sets, @@ -69,3 +65,103 @@ pub fn build( pod_disruption_budget, }) } + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use indoc::indoc; + use stackable_operator::{cli::OperatorEnvironmentOptions, utils::yaml_from_str_singleton_map}; + + use super::build; + use crate::{ + crd::{history::v1alpha1, logdir::ResolvedLogDir}, + history::controller::{ + dereference::DereferencedSparkHistoryServer, + validate::{ValidatedSparkHistoryServer, validate}, + }, + }; + + /// Minimal custom-log-dir `SparkHistoryServer` fixture. The custom log directory keeps the + /// dereference step client-free; the `uid` allows owner references to be derived from it. + const HISTORY_YAML: &str = indoc! {r#" + apiVersion: spark.stackable.tech/v1alpha1 + kind: SparkHistoryServer + metadata: + name: my-history + namespace: default + uid: 12345678-1234-1234-1234-123456789012 + spec: + image: + productVersion: 3.5.8 + logFileDirectory: + customLogDirectory: file:///stackable/spark/logs + nodes: + roleGroups: + default: + replicas: 1 + "#}; + + /// Runs the real validate step against the minimal fixture. + fn minimal_validated_cluster() -> ValidatedSparkHistoryServer { + let shs: v1alpha1::SparkHistoryServer = yaml_from_str_singleton_map(HISTORY_YAML) + .expect("invalid test SparkHistoryServer YAML"); + validate( + &shs, + DereferencedSparkHistoryServer { + log_dir: ResolvedLogDir::Custom("file:///stackable/spark/logs".to_string()), + }, + &OperatorEnvironmentOptions { + operator_namespace: "stackable-operators".to_string(), + operator_service_name: "spark-k8s-operator".to_string(), + image_repository: "oci.example.org/sdp".to_string(), + }, + ) + .expect("validate should succeed for the test fixture") + } + + /// 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 resources = build(&minimal_validated_cluster()).expect("build succeeds"); + + assert_eq!( + resources.service_account.metadata.name.as_deref(), + Some("my-history-serviceaccount") + ); + assert_eq!( + resources.role_binding.metadata.name.as_deref(), + Some("my-history-rolebinding") + ); + + let expected_labels = BTreeMap::from( + [ + ("app.kubernetes.io/component", "none"), + ("app.kubernetes.io/instance", "my-history"), + ( + "app.kubernetes.io/managed-by", + "spark.stackable.tech_history", + ), + ("app.kubernetes.io/name", "spark-history"), + ("app.kubernetes.io/role-group", "none"), + ("app.kubernetes.io/version", "3.5.8-stackable0.0.0-dev"), + ("stackable.tech/vendor", "Stackable"), + ] + .map(|(key, value)| (key.to_string(), value.to_string())), + ); + assert_eq!( + resources.service_account.metadata.labels, + Some(expected_labels.clone()) + ); + assert_eq!( + resources.role_binding.metadata.labels, + Some(expected_labels) + ); + assert_eq!( + resources.role_binding.role_ref.name, + "spark-history-clusterrole" + ); + } +} diff --git a/rust/operator-binary/src/history/controller/build/resource/mod.rs b/rust/operator-binary/src/history/controller/build/resource/mod.rs index 2923f3a2..a15a440e 100644 --- a/rust/operator-binary/src/history/controller/build/resource/mod.rs +++ b/rust/operator-binary/src/history/controller/build/resource/mod.rs @@ -1,5 +1,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/history/controller/build/resource/rbac.rs b/rust/operator-binary/src/history/controller/build/resource/rbac.rs new file mode 100644 index 00000000..32a21546 --- /dev/null +++ b/rust/operator-binary/src/history/controller/build/resource/rbac.rs @@ -0,0 +1,29 @@ +//! 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::history::controller::validate::ValidatedSparkHistoryServer; + +stackable_operator::constant!(NONE_ROLE_NAME: RoleName = "none"); +stackable_operator::constant!(NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); + +pub fn build_service_account(server: &ValidatedSparkHistoryServer) -> ServiceAccount { + rbac::build_service_account(server, &server.rbac_resource_names(), rbac_labels(server)) +} + +pub fn build_role_binding(server: &ValidatedSparkHistoryServer) -> RoleBinding { + rbac::build_role_binding(server, &server.rbac_resource_names(), rbac_labels(server)) +} + +fn rbac_labels(server: &ValidatedSparkHistoryServer) -> Labels { + server.recommended_labels_for(&NONE_ROLE_NAME, &NONE_ROLE_GROUP_NAME) +} diff --git a/rust/operator-binary/src/history/controller/build/resource/statefulset.rs b/rust/operator-binary/src/history/controller/build/resource/statefulset.rs index 8ef1f5b2..fa01acd2 100644 --- a/rust/operator-binary/src/history/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/history/controller/build/resource/statefulset.rs @@ -10,11 +10,10 @@ use stackable_operator::{ DeepMerge, api::{ apps::v1::{StatefulSet, StatefulSetSpec}, - core::v1::{PodSecurityContext, ServiceAccount}, + core::v1::PodSecurityContext, }, apimachinery::pkg::apis::meta::v1::LabelSelector, }, - kube::ResourceExt, product_logging::{ framework::calculate_log_volume_size_limit, spec::{ @@ -84,7 +83,6 @@ pub(crate) fn build_stateful_set( role_group_name: &RoleGroupName, rg: &ValidatedHistoryRoleGroup, log_dir: &ResolvedLogDir, - serviceaccount: &ServiceAccount, ) -> Result { let resolved_product_image = &validated.resolved_product_image; let resource_names = validated.resource_names(role_group_name); @@ -119,40 +117,45 @@ pub(crate) fn build_stateful_set( .config .requested_secret_lifetime .context(MissingSecretLifetimeSnafu)?; - pb.service_account_name(serviceaccount.name_unchecked()) - .metadata(pb_metadata) - .image_pull_secrets_from_product_image(resolved_product_image) - .add_volume( - VolumeBuilder::new(VOLUME_MOUNT_NAME_CONFIG.as_ref()) - .with_config_map(resource_names.role_group_config_map().to_string()) - .build(), - ) - .context(AddVolumeSnafu)? - .add_volume( - VolumeBuilder::new(VOLUME_MOUNT_NAME_LOG_CONFIG.as_ref()) - .with_config_map(log_config_map) - .build(), - ) - .context(AddVolumeSnafu)? - .add_volume( - VolumeBuilder::new(VOLUME_MOUNT_NAME_LOG.as_ref()) - .with_empty_dir( - None::, - Some(calculate_log_volume_size_limit(&[MAX_SPARK_LOG_FILES_SIZE])), - ) - .build(), - ) - .context(AddVolumeSnafu)? - .add_volumes( - log_dir - .volumes(&requested_secret_lifetime) - .context(CreateLogDirVolumesSpecSnafu)?, - ) - .context(AddVolumeSnafu)? - .security_context(PodSecurityContext { - fs_group: Some(1000), - ..PodSecurityContext::default() - }); + pb.service_account_name( + validated + .rbac_resource_names() + .service_account_name() + .to_string(), + ) + .metadata(pb_metadata) + .image_pull_secrets_from_product_image(resolved_product_image) + .add_volume( + VolumeBuilder::new(VOLUME_MOUNT_NAME_CONFIG.as_ref()) + .with_config_map(resource_names.role_group_config_map().to_string()) + .build(), + ) + .context(AddVolumeSnafu)? + .add_volume( + VolumeBuilder::new(VOLUME_MOUNT_NAME_LOG_CONFIG.as_ref()) + .with_config_map(log_config_map) + .build(), + ) + .context(AddVolumeSnafu)? + .add_volume( + VolumeBuilder::new(VOLUME_MOUNT_NAME_LOG.as_ref()) + .with_empty_dir( + None::, + Some(calculate_log_volume_size_limit(&[MAX_SPARK_LOG_FILES_SIZE])), + ) + .build(), + ) + .context(AddVolumeSnafu)? + .add_volumes( + log_dir + .volumes(&requested_secret_lifetime) + .context(CreateLogDirVolumesSpecSnafu)?, + ) + .context(AddVolumeSnafu)? + .security_context(PodSecurityContext { + fs_group: Some(1000), + ..PodSecurityContext::default() + }); // Base environment variables, with the already-merged (role + role group) env overrides // layered on top (overrides win). The base names are static and known to be valid. diff --git a/rust/operator-binary/src/history/controller/validate.rs b/rust/operator-binary/src/history/controller/validate.rs index 83f6aedc..84b396e5 100644 --- a/rust/operator-binary/src/history/controller/validate.rs +++ b/rust/operator-binary/src/history/controller/validate.rs @@ -30,7 +30,7 @@ use stackable_operator::{ VectorContainerLogConfig, validate_logging_configuration_for_container, }, role_group_utils::ResourceNames, - role_utils::{JavaCommonConfig, RoleGroupConfig, with_validated_config}, + role_utils::{self, JavaCommonConfig, RoleGroupConfig, with_validated_config}, types::{ kubernetes::{ConfigMapName, ListenerClassName, NamespaceName, Uid}, operator::{ @@ -202,8 +202,17 @@ impl ValidatedSparkHistoryServer { RoleName::from_str(HISTORY_ROLE_NAME).expect("HISTORY_ROLE_NAME is a valid role name") } + /// Type-safe names for the per-cluster RBAC resources: the ServiceAccount, + /// 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, role_group_name: &RoleGroupName) -> ResourceNames { + pub fn resource_names(&self, role_group_name: &RoleGroupName) -> ResourceNames { ResourceNames { cluster_name: self.name.clone(), role_name: Self::role_name(), @@ -211,10 +220,25 @@ impl ValidatedSparkHistoryServer { } } - /// Recommended labels for a role-group resource, using the given product version. - fn recommended_labels_for( + /// Recommended labels for a resource of the given role. + pub fn recommended_labels(&self, role_group_name: &RoleGroupName) -> Labels { + self.recommended_labels_for(&Self::role_name(), role_group_name) + } + + /// Recommended labels for a resource that is not tied to a concrete role + /// (e.g. the cluster-shared RBAC resources), using a free-form role/role-group label value. + pub fn recommended_labels_for( + &self, + role_name: &RoleName, + role_group_name: &RoleGroupName, + ) -> Labels { + self.recommended_labels_with(&self.product_version, role_name, role_group_name) + } + + fn recommended_labels_with( &self, product_version: &ProductVersion, + role_name: &RoleName, role_group_name: &RoleGroupName, ) -> Labels { recommended_labels( @@ -223,16 +247,11 @@ impl ValidatedSparkHistoryServer { product_version, &operator_name(), &controller_name(), - &Self::role_name(), + role_name, role_group_name, ) } - /// Recommended labels for a role-group resource. - pub fn recommended_labels(&self, role_group_name: &RoleGroupName) -> Labels { - self.recommended_labels_for(&self.product_version, role_group_name) - } - /// Selector labels matching the pods of a role group. pub fn role_group_selector(&self, role_group_name: &RoleGroupName) -> Labels { role_group_selector(self, &product_name(), &Self::role_name(), role_group_name) @@ -241,7 +260,7 @@ impl ValidatedSparkHistoryServer { /// Object metadata for a child resource named `name`, owned by this SparkHistoryServer and /// carrying the recommended labels for the given role group. Returns the builder so callers can /// add extra labels (e.g. Prometheus annotations) before building. - pub(crate) fn object_meta( + pub fn object_meta( &self, name: impl Into, role_group_name: &RoleGroupName, @@ -257,17 +276,17 @@ impl ValidatedSparkHistoryServer { } /// The product name (`spark-history`) as a type-safe label value. -pub(crate) fn product_name() -> ProductName { +pub fn product_name() -> ProductName { ProductName::from_str(HISTORY_APP_NAME).expect("HISTORY_APP_NAME is a valid product name") } /// The operator name as a type-safe label value. -pub(crate) fn operator_name() -> OperatorName { +pub fn operator_name() -> OperatorName { OperatorName::from_str(OPERATOR_NAME).expect("the operator name is a valid label value") } /// The controller name as a type-safe label value. -pub(crate) fn controller_name() -> ControllerName { +pub fn controller_name() -> ControllerName { ControllerName::from_str(HISTORY_CONTROLLER_NAME) .expect("the controller name is a valid label value") } From be4f296bf756524cd8a8c08b012c34e522aa804a Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 21 Jul 2026 14:38:56 +0200 Subject: [PATCH 3/6] remove dead error type --- rust/operator-binary/src/connect/controller.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/rust/operator-binary/src/connect/controller.rs b/rust/operator-binary/src/connect/controller.rs index 2a7666fb..33835b5e 100644 --- a/rust/operator-binary/src/connect/controller.rs +++ b/rust/operator-binary/src/connect/controller.rs @@ -54,12 +54,6 @@ pub enum Error { source: stackable_operator::cluster_resources::Error, }, - #[snafu(display("failed to get required Labels"))] - GetRequiredLabels { - source: - stackable_operator::kvp::KeyValuePairError, - }, - #[snafu(display("SparkConnectServer object is invalid"))] InvalidSparkConnectServer { source: error_boundary::InvalidObject, From 2bf61076bed8e52adefcb04ce41a5509c17b82d9 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 23 Jul 2026 12:53:44 +0200 Subject: [PATCH 4/6] rename resource name functions, remove patch reference --- Cargo.toml | 2 +- .../src/connect/controller/build/rbac.rs | 12 ++++++++++-- .../src/connect/controller/build/server.rs | 4 ++-- .../src/connect/controller/validate.rs | 2 +- .../history/controller/build/resource/config_map.rs | 2 +- .../src/history/controller/build/resource/rbac.rs | 12 ++++++++++-- .../src/history/controller/build/resource/service.rs | 2 +- .../history/controller/build/resource/statefulset.rs | 4 ++-- .../src/history/controller/validate.rs | 4 ++-- 9 files changed, 30 insertions(+), 14 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 3cf6fd6e..f3778979 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,5 +31,5 @@ indoc = "2" regex = "1.12" [patch."https://github.com/stackabletech/operator-rs.git"] -stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch="feat/smooth-operator/build-rbac" } +# stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch="feat/smooth-operator/build-rbac" } # stackable-operator = { path = "../operator-rs/crates/stackable-operator" } diff --git a/rust/operator-binary/src/connect/controller/build/rbac.rs b/rust/operator-binary/src/connect/controller/build/rbac.rs index 0d3f4123..ee314f87 100644 --- a/rust/operator-binary/src/connect/controller/build/rbac.rs +++ b/rust/operator-binary/src/connect/controller/build/rbac.rs @@ -17,11 +17,19 @@ stackable_operator::constant!(NONE_ROLE_NAME: RoleName = "none"); stackable_operator::constant!(NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); pub fn build_service_account(server: &ValidatedSparkConnectServer) -> ServiceAccount { - rbac::build_service_account(server, &server.rbac_resource_names(), rbac_labels(server)) + rbac::build_service_account( + server, + &server.cluster_resource_names(), + rbac_labels(server), + ) } pub fn build_role_binding(server: &ValidatedSparkConnectServer) -> RoleBinding { - rbac::build_role_binding(server, &server.rbac_resource_names(), rbac_labels(server)) + rbac::build_role_binding( + server, + &server.cluster_resource_names(), + rbac_labels(server), + ) } fn rbac_labels(server: &ValidatedSparkConnectServer) -> Labels { diff --git a/rust/operator-binary/src/connect/controller/build/server.rs b/rust/operator-binary/src/connect/controller/build/server.rs index 3c152d04..4e629547 100644 --- a/rust/operator-binary/src/connect/controller/build/server.rs +++ b/rust/operator-binary/src/connect/controller/build/server.rs @@ -190,7 +190,7 @@ pub(crate) fn build_stateful_set( let mut pb = PodBuilder::new(); - pb.service_account_name(validated.rbac_resource_names().service_account_name()) + pb.service_account_name(validated.cluster_resource_names().service_account_name()) .metadata(metadata) .image_pull_secrets_from_product_image(resolved_product_image) .add_volume( @@ -403,7 +403,7 @@ pub(crate) fn server_properties( let resolved_product_image = &validated.resolved_product_image; let spark_image = resolved_product_image.image.clone(); let spark_version = resolved_product_image.product_version.clone(); - let service_account_name = validated.rbac_resource_names().service_account_name(); + let service_account_name = validated.cluster_resource_names().service_account_name(); let namespace = driver_service .namespace() .context(ObjectHasNoNamespaceSnafu)?; diff --git a/rust/operator-binary/src/connect/controller/validate.rs b/rust/operator-binary/src/connect/controller/validate.rs index 23ebe97c..3c7cadac 100644 --- a/rust/operator-binary/src/connect/controller/validate.rs +++ b/rust/operator-binary/src/connect/controller/validate.rs @@ -237,7 +237,7 @@ impl ValidatedSparkConnectServer { /// Type-safe names for the per-cluster RBAC resources: the ServiceAccount, /// 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(), diff --git a/rust/operator-binary/src/history/controller/build/resource/config_map.rs b/rust/operator-binary/src/history/controller/build/resource/config_map.rs index c19f18b2..cdfbb518 100644 --- a/rust/operator-binary/src/history/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/history/controller/build/resource/config_map.rs @@ -55,7 +55,7 @@ pub(crate) fn build_config_map( rg: &ValidatedHistoryRoleGroup, ) -> Result { let cm_name = validated - .resource_names(role_group_name) + .role_group_resource_names(role_group_name) .role_group_config_map() .to_string(); diff --git a/rust/operator-binary/src/history/controller/build/resource/rbac.rs b/rust/operator-binary/src/history/controller/build/resource/rbac.rs index 32a21546..78aca8dd 100644 --- a/rust/operator-binary/src/history/controller/build/resource/rbac.rs +++ b/rust/operator-binary/src/history/controller/build/resource/rbac.rs @@ -17,11 +17,19 @@ stackable_operator::constant!(NONE_ROLE_NAME: RoleName = "none"); stackable_operator::constant!(NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); pub fn build_service_account(server: &ValidatedSparkHistoryServer) -> ServiceAccount { - rbac::build_service_account(server, &server.rbac_resource_names(), rbac_labels(server)) + rbac::build_service_account( + server, + &server.cluster_resource_names(), + rbac_labels(server), + ) } pub fn build_role_binding(server: &ValidatedSparkHistoryServer) -> RoleBinding { - rbac::build_role_binding(server, &server.rbac_resource_names(), rbac_labels(server)) + rbac::build_role_binding( + server, + &server.cluster_resource_names(), + rbac_labels(server), + ) } fn rbac_labels(server: &ValidatedSparkHistoryServer) -> Labels { diff --git a/rust/operator-binary/src/history/controller/build/resource/service.rs b/rust/operator-binary/src/history/controller/build/resource/service.rs index f1773fff..e5d4c4b3 100644 --- a/rust/operator-binary/src/history/controller/build/resource/service.rs +++ b/rust/operator-binary/src/history/controller/build/resource/service.rs @@ -19,7 +19,7 @@ pub fn build_rolegroup_metrics_service( metadata: validated .object_meta( validated - .resource_names(role_group_name) + .role_group_resource_names(role_group_name) .metrics_service_name() .to_string(), role_group_name, diff --git a/rust/operator-binary/src/history/controller/build/resource/statefulset.rs b/rust/operator-binary/src/history/controller/build/resource/statefulset.rs index fa01acd2..040b1417 100644 --- a/rust/operator-binary/src/history/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/history/controller/build/resource/statefulset.rs @@ -85,7 +85,7 @@ pub(crate) fn build_stateful_set( log_dir: &ResolvedLogDir, ) -> Result { let resolved_product_image = &validated.resolved_product_image; - let resource_names = validated.resource_names(role_group_name); + let resource_names = validated.role_group_resource_names(role_group_name); let log_config_map = if let Some(ContainerLogConfig { choice: @@ -119,7 +119,7 @@ pub(crate) fn build_stateful_set( .context(MissingSecretLifetimeSnafu)?; pb.service_account_name( validated - .rbac_resource_names() + .cluster_resource_names() .service_account_name() .to_string(), ) diff --git a/rust/operator-binary/src/history/controller/validate.rs b/rust/operator-binary/src/history/controller/validate.rs index 84b396e5..4dde25f6 100644 --- a/rust/operator-binary/src/history/controller/validate.rs +++ b/rust/operator-binary/src/history/controller/validate.rs @@ -204,7 +204,7 @@ impl ValidatedSparkHistoryServer { /// Type-safe names for the per-cluster RBAC resources: the ServiceAccount, /// 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(), @@ -212,7 +212,7 @@ impl ValidatedSparkHistoryServer { } /// Type-safe names for the resources of a given role group. - pub fn resource_names(&self, role_group_name: &RoleGroupName) -> ResourceNames { + pub fn role_group_resource_names(&self, role_group_name: &RoleGroupName) -> ResourceNames { ResourceNames { cluster_name: self.name.clone(), role_name: Self::role_name(), From 49dbde1bb60945f117983d3656c45f26286320c2 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 23 Jul 2026 13:05:39 +0200 Subject: [PATCH 5/6] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74832625..612d19bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,11 @@ All notable changes to this project will be documented in this file. server reconcilers that assembles all relevant Kubernetes resources before anything is applied ([#721]). - Bump stackable-operator to 0.114.0 ([#732]). +- The RBAC ServiceAccounts and RoleBindings of the history and connect servers are now + built with the operator-rs `v2::rbac` functions and carry the recommended labels ([#727]). [#721]: https://github.com/stackabletech/spark-k8s-operator/pull/721 +[#727]: https://github.com/stackabletech/spark-k8s-operator/pull/727 [#732]: https://github.com/stackabletech/spark-k8s-operator/pull/732 ## [26.7.0] - 2026-07-21 From 1b7feae936fbba5c8f5cf602b5a972786dbc12ba Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 24 Jul 2026 14:07:08 +0200 Subject: [PATCH 6/6] added role/From impl for connect roles plus parsing test --- rust/operator-binary/src/connect/common.rs | 42 +++++++++++++++++-- .../src/connect/controller/validate.rs | 21 +++------- 2 files changed, 44 insertions(+), 19 deletions(-) diff --git a/rust/operator-binary/src/connect/common.rs b/rust/operator-binary/src/connect/common.rs index 85cd8b8e..2cba2620 100644 --- a/rust/operator-binary/src/connect/common.rs +++ b/rust/operator-binary/src/connect/common.rs @@ -1,11 +1,12 @@ -use std::collections::BTreeMap; +use std::{collections::BTreeMap, str::FromStr}; use snafu::{ResultExt, Snafu}; use stackable_operator::v2::{ config_file_writer::{PropertiesWriterError, to_java_properties_string}, role_utils::JavaCommonConfig, + types::operator::RoleName, }; -use strum::Display; +use strum::{Display, EnumIter}; use super::crd::CONNECT_EXECUTOR_ROLE_NAME; use crate::{ @@ -29,13 +30,30 @@ pub enum Error { MetricsProperties { source: PropertiesWriterError }, } -#[derive(Clone, Debug, Display)] +#[derive(Clone, Debug, Display, EnumIter)] #[strum(serialize_all = "lowercase")] pub(crate) enum SparkConnectRole { Server, Executor, } +impl From for RoleName { + fn from(value: SparkConnectRole) -> Self { + (&value).into() + } +} + +impl From<&SparkConnectRole> for RoleName { + fn from(value: &SparkConnectRole) -> Self { + match value { + SparkConnectRole::Server => RoleName::from_str(CONNECT_SERVER_ROLE_NAME) + .expect("CONNECT_SERVER_ROLE_NAME is a valid role name"), + SparkConnectRole::Executor => RoleName::from_str(CONNECT_EXECUTOR_ROLE_NAME) + .expect("CONNECT_EXECUTOR_ROLE_NAME is a valid role name"), + } + } +} + pub(crate) fn object_name(stacklet_name: &str, role: SparkConnectRole) -> String { match role { SparkConnectRole::Server => format!("{}-{}", stacklet_name, CONNECT_SERVER_ROLE_NAME), @@ -110,3 +128,21 @@ pub(crate) fn metrics_properties( to_java_properties_string(result.iter()).context(MetricsPropertiesSnafu) } + +#[cfg(test)] +mod tests { + use stackable_operator::v2::types::operator::RoleName; + use strum::IntoEnumIterator; + + use super::SparkConnectRole; + + /// Locks the invariant behind the `expect` in the `From for RoleName` + /// impls: every variant (present and future) must map to a valid `RoleName`. + #[test] + fn every_spark_connect_role_maps_to_a_valid_role_name() { + for role in SparkConnectRole::iter() { + let _: RoleName = (&role).into(); + let _: RoleName = role.into(); + } + } +} diff --git a/rust/operator-binary/src/connect/controller/validate.rs b/rust/operator-binary/src/connect/controller/validate.rs index 3c7cadac..e15d50dc 100644 --- a/rust/operator-binary/src/connect/controller/validate.rs +++ b/rust/operator-binary/src/connect/controller/validate.rs @@ -38,9 +38,8 @@ use crate::{ common::SparkConnectRole, controller::dereference::DereferencedSparkConnectServer, crd::{ - self, CONNECT_APP_NAME, CONNECT_CONTROLLER_NAME, CONNECT_EXECUTOR_ROLE_NAME, - CONNECT_SERVER_ROLE_NAME, DEFAULT_SPARK_CONNECT_GROUP_NAME, SparkConnectContainer, - v1alpha1, + self, CONNECT_APP_NAME, CONNECT_CONTROLLER_NAME, DEFAULT_SPARK_CONNECT_GROUP_NAME, + SparkConnectContainer, v1alpha1, }, s3::ResolvedS3, }, @@ -184,7 +183,7 @@ impl ValidatedSparkConnectServer { /// Recommended labels for a resource of the given role. pub fn recommended_labels(&self, role: SparkConnectRole) -> Labels { - self.recommended_labels_for(&role_name(role), &DEFAULT_SPARK_CONNECT_ROLE_GROUP) + self.recommended_labels_for(&role.into(), &DEFAULT_SPARK_CONNECT_ROLE_GROUP) } fn recommended_labels_with( @@ -206,7 +205,7 @@ impl ValidatedSparkConnectServer { /// Selector labels matching the pods of the given role. pub fn role_selector(&self, role: SparkConnectRole) -> Labels { - role_selector(self, &product_name(), &role_name(role)) + role_selector(self, &product_name(), &role.into()) } /// Selector labels matching the pods of the given role's (single) role group. @@ -214,7 +213,7 @@ impl ValidatedSparkConnectServer { role_group_selector( self, &product_name(), - &role_name(role), + &role.into(), &DEFAULT_SPARK_CONNECT_ROLE_GROUP, ) } @@ -261,16 +260,6 @@ pub fn controller_name() -> ControllerName { .expect("the controller name is a valid label value") } -/// The role name for the given Spark Connect role as a type-safe label value. -fn role_name(role: SparkConnectRole) -> RoleName { - match role { - SparkConnectRole::Server => RoleName::from_str(CONNECT_SERVER_ROLE_NAME) - .expect("CONNECT_SERVER_ROLE_NAME is a valid role name"), - SparkConnectRole::Executor => RoleName::from_str(CONNECT_EXECUTOR_ROLE_NAME) - .expect("CONNECT_EXECUTOR_ROLE_NAME is a valid role name"), - } -} - impl NameIsValidLabelValue for ValidatedSparkConnectServer { fn to_label_value(&self) -> String { self.name.to_label_value()