add rbac labels, use infallible functions

Author: adwk67

Cargo.lock

@@ -34,4 +34,4 @@ tracing = "0.1"
 
 [patch."https://github.com/stackabletech/operator-rs.git"]
 # stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "main" }
-# stackable-operator = { path = "../operator-rs/crates/stackable-operator" }
+stackable-operator = { path = "../operator-rs/crates/stackable-operator" }

Cargo.toml

@@ -9,14 +9,12 @@ use snafu::{ResultExt, Snafu};
 use stackable_operator::{
     cli::OperatorEnvironmentOptions,
     cluster_resources::ClusterResourceApplyStrategy,
-    commons::{random_secret_creation, rbac::build_rbac_resources},
+    commons::random_secret_creation,
     k8s_openapi::api::core::v1::EnvVar,
     kube::{
-        ResourceExt,
         core::{DeserializeGuard, error_boundary},
         runtime::controller::Action,
     },
-    kvp::LabelError,
     logging::controller::ReconcilerError,
     shared::time::Duration,
     status::condition::{
@@ -30,7 +28,7 @@ use strum::{EnumDiscriminants, IntoStaticStr};
 use crate::{
     controller::{ValidatedCluster, build, controller_name, operator_name, product_name},
     crd::{
-        APP_NAME, AirflowClusterStatus, OPERATOR_NAME,
+        AirflowClusterStatus, OPERATOR_NAME,
         internal_secret::{
             FERNET_KEY_SECRET_KEY, INTERNAL_SECRET_SECRET_KEY, JWT_SECRET_SECRET_KEY,
         },
@@ -58,21 +56,6 @@ pub enum Error {
         source: stackable_operator::cluster_resources::Error,
     },
 
-    #[snafu(display("failed to patch service account"))]
-    ApplyServiceAccount {
-        source: stackable_operator::cluster_resources::Error,
-    },
-
-    #[snafu(display("failed to patch role binding: {source}"))]
-    ApplyRoleBinding {
-        source: stackable_operator::cluster_resources::Error,
-    },
-
-    #[snafu(display("failed to build RBAC objects"))]
-    BuildRBACObjects {
-        source: stackable_operator::commons::rbac::Error,
-    },
-
     #[snafu(display("failed to build the Kubernetes resources"))]
     BuildResources { source: build::Error },
 
@@ -101,9 +84,6 @@ pub enum Error {
         source: crate::controller::validate::Error,
     },
 
-    #[snafu(display("failed to build label"))]
-    BuildLabel { source: LabelError },
-
     #[snafu(display("AirflowCluster object is invalid"))]
     InvalidAirflowCluster {
         source: error_boundary::InvalidObject,
@@ -159,33 +139,25 @@ pub async fn reconcile_airflow(
         &airflow.spec.object_overrides,
     );
 
-    let required_labels = cluster_resources
-        .get_required_labels()
-        .context(BuildLabelSnafu)?;
-
-    let (rbac_sa, rbac_rolebinding) =
-        build_rbac_resources(airflow, APP_NAME, required_labels).context(BuildRBACObjectsSnafu)?;
-
-    // The ServiceAccount name is deterministic on the built object, so the build step does not
-    // depend on the applied ServiceAccount.
-    let service_account_name = rbac_sa.name_any();
-
-    cluster_resources
-        .add(client, rbac_sa)
-        .await
-        .context(ApplyServiceAccountSnafu)?;
-    cluster_resources
-        .add(client, rbac_rolebinding)
-        .await
-        .context(ApplyRoleBindingSnafu)?;
-
-    let resources =
-        build::build(&validated_cluster, &service_account_name).context(BuildResourcesSnafu)?;
+    let resources = build::build(&validated_cluster).context(BuildResourcesSnafu)?;
 
     let mut ss_cond_builder = StatefulSetConditionBuilder::default();
 
     // Apply order is: StatefulSets last (a changed mounted ConfigMap/Secret
-    // must exist first, else Pods restart -- commons-operator#111).
+    // must exist first, else Pods restart -- commons-operator#111). The ServiceAccount comes
+    // first because the Pods reference it at creation time.
+    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)

rust/operator-binary/src/airflow_controller.rs

@@ -11,6 +11,7 @@ use crate::{
             executor::build_executor_template_config_map,
             listener::build_group_listener,
             pdb::build_pdb,
+            rbac::{build_role_binding, build_service_account},
             service::{build_rolegroup_headless_service, build_rolegroup_metrics_service},
             statefulset::build_server_rolegroup_statefulset,
         },
@@ -47,14 +48,7 @@ pub enum Error {
 /// Does not need a Kubernetes client: every reference to another Kubernetes resource is already
 /// dereferenced and validated by this point. Cluster configuration is likewise already validated,
 /// so the errors returned here are resource-assembly failures only.
-///
-/// `service_account_name` is the name of the RBAC `ServiceAccount` the role-group Pods and the
-/// Kubernetes-executor pod template run under (RBAC resources are built and applied separately,
-/// in the reconcile step).
-pub fn build(
-    cluster: &ValidatedCluster,
-    service_account_name: &str,
-) -> Result<KubernetesResources, Error> {
+pub fn build(cluster: &ValidatedCluster) -> Result<KubernetesResources, Error> {
     let mut stateful_sets = vec![];
     let mut services = vec![];
     let mut listeners = vec![];
@@ -81,7 +75,6 @@ pub fn build(
 
         let executor_template_config_map = build_executor_template_config_map(
             cluster,
-            service_account_name,
             &executor_template.config,
             &executor_template.env_overrides,
             &executor_template.pod_overrides,
@@ -123,7 +116,7 @@ pub fn build(
             config_maps.push(
                 config_map::build_rolegroup_config_map(
                     cluster,
-                    &role.role_name(),
+                    &ValidatedCluster::role_name(role),
                     role_group_name,
                     &rg_config.config_overrides,
                     logging,
@@ -140,7 +133,6 @@ pub fn build(
                     role_group_name,
                     rg_config,
                     logging,
-                    service_account_name,
                 )
                 .context(StatefulSetSnafu {
                     role_group: role_group_name.clone(),
@@ -155,11 +147,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;
@@ -184,7 +180,7 @@ mod tests {
         apiVersion: airflow.stackable.tech/v1alpha2
         kind: AirflowCluster
         metadata:
-          name: airflow
+          name: my-airflow
           namespace: default
           uid: e6ac237d-a6d4-43a1-8135-f36506110912
         spec:
@@ -263,25 +259,84 @@ mod tests {
     #[test]
     fn build_produces_expected_resource_names() {
         let cluster = celery_executor_cluster();
-        let resources = build(&cluster, "airflow-serviceaccount").expect("build succeeds");
+        let resources = build(&cluster).expect("build succeeds");
 
         assert_eq!(
             sorted_names(&resources.stateful_sets),
-            ["airflow-scheduler-default", "airflow-webserver-default"]
+            [
+                "my-airflow-scheduler-default",
+                "my-airflow-webserver-default"
+            ]
         );
         // One headless and one metrics Service per role group.
         assert_eq!(resources.services.len(), 4);
         assert_eq!(
             sorted_names(&resources.config_maps),
-            ["airflow-scheduler-default", "airflow-webserver-default"]
+            [
+                "my-airflow-scheduler-default",
+                "my-airflow-webserver-default"
+            ]
         );
         // The webserver is the only role with a group Listener.
-        assert_eq!(sorted_names(&resources.listeners), ["airflow-webserver"]);
+        assert_eq!(sorted_names(&resources.listeners), ["my-airflow-webserver"]);
         // A default PDB per role (the Celery worker included).
         assert_eq!(
             sorted_names(&resources.pod_disruption_budgets),
-            ["airflow-scheduler", "airflow-webserver", "airflow-worker"]
+            [
+                "my-airflow-scheduler",
+                "my-airflow-webserver",
+                "my-airflow-worker"
+            ]
+        );
+    }
+
+    /// 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 cluster = celery_executor_cluster();
+        let resources = build(&cluster).expect("build succeeds");
+
+        assert_eq!(
+            sorted_names(&resources.service_accounts),
+            ["my-airflow-serviceaccount"]
+        );
+        assert_eq!(
+            sorted_names(&resources.role_bindings),
+            ["my-airflow-rolebinding"]
+        );
+
+        let expected_labels = BTreeMap::from(
+            [
+                ("app.kubernetes.io/component", "none"),
+                ("app.kubernetes.io/instance", "my-airflow"),
+                (
+                    "app.kubernetes.io/managed-by",
+                    "airflow.stackable.tech_airflowcluster",
+                ),
+                ("app.kubernetes.io/name", "airflow"),
+                ("app.kubernetes.io/role-group", "none"),
+                ("app.kubernetes.io/version", "3.1.6-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, "airflow-clusterrole");
     }
 
     /// The Kubernetes-executor branch of `build()` (moved here from `reconcile`) additionally emits
@@ -290,15 +345,15 @@ mod tests {
     #[test]
     fn build_kubernetes_executor_adds_pod_template_config_maps() {
         let cluster = kubernetes_executor_cluster();
-        let resources = build(&cluster, "airflow-serviceaccount").expect("build succeeds");
+        let resources = build(&cluster).expect("build succeeds");
 
         assert_eq!(
             sorted_names(&resources.config_maps),
             [
-                "airflow-executor-kubernetes",
-                "airflow-executor-pod-template",
-                "airflow-scheduler-default",
-                "airflow-webserver-default",
+                "my-airflow-executor-kubernetes",
+                "my-airflow-executor-pod-template",
+                "my-airflow-scheduler-default",
+                "my-airflow-webserver-default",
             ]
         );
     }

rust/operator-binary/src/controller/build/mod.rs

@@ -76,7 +76,6 @@ type Result<T, E = Error> = std::result::Result<T, E>;
 
 pub fn build_executor_template_config_map(
     cluster: &ValidatedCluster,
-    sa_name: &str,
     executor_config: &ValidatedAirflowConfig,
     env_overrides: &HashMap<String, String>,
     pod_overrides: &PodTemplateSpec,
@@ -99,7 +98,12 @@ pub fn build_executor_template_config_map(
     pb.metadata(pb_metadata)
         .image_pull_secrets_from_product_image(resolved_product_image)
         .affinity(&executor_config.affinity)
-        .service_account_name(sa_name)
+        .service_account_name(
+            cluster
+                .rbac_resource_names()
+                .service_account_name()
+                .to_string(),
+        )
         .restart_policy("Never")
         .security_context(PodSecurityContextBuilder::new().fs_group(1000).build());