refactor connect server service account and role binding

Author: adwk67

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" }

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,32 +139,19 @@ 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
     // they must exist first).
     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)

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<SparkConnectResources, Error> {
     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,

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)
+}

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<String>,
@@ -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<String, String>>) -> Result<Vec<EnvVar>, E
 pub(crate) fn server_properties(
     validated: &ValidatedSparkConnectServer,
     driver_service: &Service,
-    service_account: &ServiceAccount,
 ) -> Result<BTreeMap<String, Option<String>>, 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(),

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<T, E = Error> = std::result::Result<T, E>;
 
+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<String>,
         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,