refactor rbac (service account and role binding), added tests

Author: adwk67

Cargo.nix

@@ -31,7 +31,7 @@ tokio-zookeeper = "0.4"
 tracing = "0.1"
 
 [patch."https://github.com/stackabletech/operator-rs.git"]
-# stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "smooth-operator" }
+stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "feat/smooth-operator/build-rbac" }
 # stackable-operator = { path = "../operator-rs/crates/stackable-operator" }
 
 [patch.crates-io]

Cargo.toml

@@ -7,19 +7,18 @@ use snafu::{OptionExt, ResultExt, Snafu};
 use stackable_operator::{
     cli::OperatorEnvironmentOptions,
     cluster_resources::ClusterResourceApplyStrategy,
-    commons::rbac::build_rbac_resources,
     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::{
         api::DynamicObject,
         core::{DeserializeGuard, error_boundary},
         runtime::controller,
     },
-    kvp::LabelError,
     logging::controller::ReconcilerError,
     shared::time::Duration,
     status::condition::{
@@ -31,7 +30,7 @@ use stackable_operator::{
 use strum::{EnumDiscriminants, IntoStaticStr};
 
 use crate::{
-    APP_NAME, OPERATOR_NAME, ObjectRef,
+    OPERATOR_NAME, ObjectRef,
     crd::v1alpha1,
     zk_controller::{
         build::resource::discovery,
@@ -106,29 +105,11 @@ pub enum Error {
         source: stackable_operator::client::Error,
     },
 
-    #[snafu(display("failed to create RBAC service account"))]
-    ApplyServiceAccount {
-        source: stackable_operator::cluster_resources::Error,
-    },
-
-    #[snafu(display("failed to create RBAC role binding"))]
-    ApplyRoleBinding {
-        source: stackable_operator::cluster_resources::Error,
-    },
-
-    #[snafu(display("failed to build RBAC resources"))]
-    BuildRbacResources {
-        source: stackable_operator::commons::rbac::Error,
-    },
-
     #[snafu(display("failed to delete orphaned resources"))]
     DeleteOrphans {
         source: stackable_operator::cluster_resources::Error,
     },
 
-    #[snafu(display("failed to build label"))]
-    BuildLabel { source: LabelError },
-
     #[snafu(display("failed to build object meta data"))]
     ObjectMeta {
         source: stackable_operator::builder::meta::Error,
@@ -154,11 +135,7 @@ impl ReconcilerError for Error {
             Error::BuildDiscoveryConfig { .. } => None,
             Error::ApplyDiscoveryConfig { .. } => None,
             Error::ApplyStatus { .. } => None,
-            Error::ApplyServiceAccount { .. } => None,
-            Error::ApplyRoleBinding { .. } => None,
-            Error::BuildRbacResources { .. } => None,
             Error::DeleteOrphans { .. } => None,
-            Error::BuildLabel { .. } => None,
             Error::ObjectMeta { .. } => None,
         }
     }
@@ -173,6 +150,8 @@ pub struct KubernetesResources {
     pub listeners: Vec<Listener>,
     pub config_maps: Vec<ConfigMap>,
     pub pod_disruption_budgets: Vec<PodDisruptionBudget>,
+    pub service_accounts: Vec<ServiceAccount>,
+    pub role_bindings: Vec<RoleBinding>,
 }
 
 pub async fn reconcile_zk(
@@ -209,30 +188,24 @@ pub async fn reconcile_zk(
         &validated_cluster.object_overrides,
     );
 
-    let (rbac_sa, rbac_rolebinding) = build_rbac_resources(
-        zk,
-        APP_NAME,
-        cluster_resources
-            .get_required_labels()
-            .context(BuildLabelSnafu)?,
-    )
-    .context(BuildRbacResourcesSnafu)?;
-
-    cluster_resources
-        .add(client, rbac_sa)
-        .await
-        .context(ApplyServiceAccountSnafu)?;
-
-    cluster_resources
-        .add(client, rbac_rolebinding)
-        .await
-        .context(ApplyRoleBindingSnafu)?;
-
     let resources = build::build(&validated_cluster, &client.kubernetes_cluster_info)
         .context(BuildResourcesSnafu)?;
 
     let mut ss_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)

crate-hashes.json

@@ -13,8 +13,7 @@ use std::str::FromStr;
 
 use snafu::{ResultExt, Snafu};
 use stackable_operator::{
-    utils::cluster_info::KubernetesClusterInfo,
-    v2::types::operator::{ProductVersion, RoleGroupName},
+    utils::cluster_info::KubernetesClusterInfo, v2::types::operator::RoleGroupName,
 };
 
 use crate::{
@@ -25,6 +24,7 @@ use crate::{
             config_map,
             listener::build_role_listener,
             pdb::build_pdb,
+            rbac::{build_role_binding, build_service_account},
             service::{
                 build_server_rolegroup_headless_service, build_server_rolegroup_metrics_service,
             },
@@ -42,10 +42,6 @@ stackable_operator::constant!(pub(crate) PLACEHOLDER_DISCOVERY_ROLE_GROUP: RoleG
 // (which is not tied to a single role group).
 stackable_operator::constant!(pub(crate) PLACEHOLDER_LISTENER_ROLE_GROUP: RoleGroupName = "none");
 
-// Placeholder product version used for labels on PVC templates, which cannot be modified once
-// deployed. A constant value keeps the labels stable across version upgrades.
-stackable_operator::constant!(pub(crate) UNVERSIONED_PRODUCT_VERSION: ProductVersion = "none");
-
 pub mod command;
 pub mod graceful_shutdown;
 pub mod jvm;
@@ -137,11 +133,15 @@ pub fn build(
         listeners,
         config_maps,
         pod_disruption_budgets,
+        service_accounts: vec![build_service_account(cluster)],
+        role_bindings: vec![build_role_binding(cluster)],
     })
 }
 
 #[cfg(test)]
 mod tests {
+    use std::collections::BTreeMap;
+
     use stackable_operator::kube::Resource;
 
     use super::build;
@@ -216,4 +216,68 @@ mod tests {
             ["simple-zookeeper-server"]
         );
     }
+
+    /// 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 zookeeper_yaml = r#"
+        apiVersion: zookeeper.stackable.tech/v1alpha1
+        kind: ZookeeperCluster
+        metadata:
+          name: simple-zookeeper
+        spec:
+          image:
+            productVersion: "3.9.5"
+          servers:
+            roleGroups:
+              default:
+                replicas: 1
+        "#;
+        let zookeeper = minimal_zk(zookeeper_yaml);
+        let cluster = validated_cluster(&zookeeper);
+
+        let resources = build(&cluster, &cluster_info()).expect("build succeeds");
+
+        assert_eq!(
+            sorted_names(&resources.service_accounts),
+            ["simple-zookeeper-serviceaccount"]
+        );
+        assert_eq!(
+            sorted_names(&resources.role_bindings),
+            ["simple-zookeeper-rolebinding"]
+        );
+
+        let expected_labels = BTreeMap::from(
+            [
+                ("app.kubernetes.io/component", "none"),
+                ("app.kubernetes.io/instance", "simple-zookeeper"),
+                (
+                    "app.kubernetes.io/managed-by",
+                    "zookeeper.stackable.tech_zookeepercluster",
+                ),
+                ("app.kubernetes.io/name", "zookeeper"),
+                ("app.kubernetes.io/role-group", "none"),
+                ("app.kubernetes.io/version", "3.9.5-stackable0.0.0-dev"),
+                ("stackable.tech/vendor", "Stackable"),
+            ]
+            .map(|(key, value)| (key.to_string(), value.to_string())),
+        );
+        let service_account = resources
+            .service_accounts
+            .first()
+            .expect("a ServiceAccount is built");
+        assert_eq!(
+            service_account.metadata.labels,
+            Some(expected_labels.clone())
+        );
+
+        let role_binding = resources
+            .role_bindings
+            .first()
+            .expect("a RoleBinding is built");
+        assert_eq!(role_binding.metadata.labels, Some(expected_labels));
+        assert_eq!(role_binding.role_ref.name, "zookeeper-clusterrole");
+    }
 }

rust/operator-binary/src/zk_controller.rs

@@ -5,5 +5,6 @@ pub mod config_map;
 pub mod discovery;
 pub mod listener;
 pub mod pdb;
+pub mod rbac;
 pub mod service;
 pub mod statefulset;

rust/operator-binary/src/zk_controller/build.rs

@@ -0,0 +1,42 @@
+//! Builds the RBAC resources (ServiceAccount + RoleBinding) shared by all role groups.
+
+use std::str::FromStr;
+
+use stackable_operator::{
+    k8s_openapi::api::{core::v1::ServiceAccount, rbac::v1::RoleBinding},
+    kvp::Labels,
+    v2::{
+        rbac,
+        types::operator::{RoleGroupName, RoleName},
+    },
+};
+
+use crate::zk_controller::validate::ValidatedCluster;
+
+stackable_operator::constant!(NONE_ROLE_NAME: RoleName = "none");
+stackable_operator::constant!(NONE_ROLE_GROUP_NAME: RoleGroupName = "none");
+
+/// Builds the [`ServiceAccount`] that the role-group Pods run under.
+pub fn build_service_account(cluster: &ValidatedCluster) -> ServiceAccount {
+    rbac::build_service_account(
+        cluster,
+        &cluster.rbac_resource_names(),
+        rbac_labels(cluster),
+    )
+}
+
+/// Builds the [`RoleBinding`] that binds the [`ServiceAccount`] from [`build_service_account`] to
+/// the operator-deployed ClusterRole.
+pub fn build_role_binding(cluster: &ValidatedCluster) -> RoleBinding {
+    rbac::build_role_binding(
+        cluster,
+        &cluster.rbac_resource_names(),
+        rbac_labels(cluster),
+    )
+}
+
+/// Both resources are shared by the whole cluster rather than tied to a role or role group, so
+/// the recommended labels carry `none` for both values.
+fn rbac_labels(cluster: &ValidatedCluster) -> Labels {
+    cluster.recommended_labels_for(&NONE_ROLE_NAME, &NONE_ROLE_GROUP_NAME)
+}

rust/operator-binary/src/zk_controller/build/resource/mod.rs

@@ -57,7 +57,6 @@ use crate::{
     zk_controller::{
         LISTENER_VOLUME_DIR, LISTENER_VOLUME_NAME,
         build::{
-            UNVERSIONED_PRODUCT_VERSION,
             command::create_init_container_command_args,
             graceful_shutdown::add_graceful_shutdown_config,
             jvm::{construct_non_heap_jvm_args, construct_zk_server_heap_env},
@@ -194,8 +193,7 @@ pub fn build_server_rolegroup_statefulset(
 
     // Used for PVC templates that cannot be modified once they are deployed. A constant version
     // keeps the labels stable across version upgrades.
-    let unversioned_recommended_labels =
-        cluster.recommended_labels_for(&UNVERSIONED_PRODUCT_VERSION, role_group_name);
+    let unversioned_recommended_labels = cluster.unversioned_recommended_labels(role_group_name);
 
     let listener_pvc = build_role_listener_pvc(
         role_listener_name(cluster.name.as_ref(), &ZookeeperRole::Server).as_ref(),
@@ -383,7 +381,7 @@ pub fn build_server_rolegroup_statefulset(
             fs_group: Some(1000),
             ..PodSecurityContext::default()
         })
-        .service_account_name(cluster.rbac_service_account_name());
+        .service_account_name(cluster.rbac_resource_names().service_account_name());
 
     // Use the user-provided custom log ConfigMap if one is configured, otherwise fall back to the
     // rolegroup's own ConfigMap. This branches on the *validated* logging choice.