refactor history server service account and role binding, added tests

adwk67 · adwk67 · commit f05b6b8d4c37 · 2026-07-21T11:41:24.000+02:00
diff --git 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
@@ -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
@@ -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
@@ -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<stackable_operator::kvp::LabelValueError>,
-    },
-
     #[snafu(display("SparkHistoryServer object is invalid"))]
     InvalidSparkHistoryServer {
         // boxed because otherwise Clippy warns about a large enum variant
@@ -135,32 +110,19 @@ 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
     // and run under the SA, so those 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 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
@@ -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<T, E = Error> = std::result::Result<T, E>;
 
 /// Builds every Kubernetes resource for the given validated SparkHistoryServer.
-pub fn build(
-    validated: &ValidatedSparkHistoryServer,
-    service_account: ServiceAccount,
-    role_binding: RoleBinding,
-) -> Result<SparkHistoryResources> {
+pub fn build(validated: &ValidatedSparkHistoryServer) -> Result<SparkHistoryResources> {
     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,12 +56,112 @@ 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,
         listener,
         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
@@ -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
diff --git a/rust/operator-binary/src/history/controller/build/resource/statefulset.rs b/rust/operator-binary/src/history/controller/build/resource/statefulset.rs
diff --git a/rust/operator-binary/src/history/controller/validate.rs b/rust/operator-binary/src/history/controller/validate.rs
