diff --git a/CHANGELOG.md b/CHANGELOG.md index 851019e7..b5ce35fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,11 @@ - Internal operator refactoring: introduce a build() step in the reconciler that assembles all relevant Kubernetes resources before anything is applied ([#756]). - Bump stackable-operator to 0.114.0 ([#765]). +- The RBAC ServiceAccount and RoleBinding are now built with the operator-rs `v2::rbac` + functions and carry the full set of recommended labels ([#761]). [#756]: https://github.com/stackabletech/superset-operator/pull/756 +[#761]: https://github.com/stackabletech/superset-operator/pull/761 [#765]: https://github.com/stackabletech/superset-operator/pull/765 ## [26.7.0] - 2026-07-21 diff --git a/Cargo.toml b/Cargo.toml index ae1db7c5..41692c9b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,5 +29,5 @@ tokio = { version = "1.52", features = ["full"] } 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 = { 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/controller.rs b/rust/operator-binary/src/controller.rs index f9db48a2..69d83d11 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -15,17 +15,17 @@ use stackable_operator::{ affinity::StackableAffinity, product_image_selection::ResolvedProductImage, random_secret_creation::{self, create_random_secret_if_not_exists}, - rbac::build_rbac_resources, resources::{NoRuntimeLimits, Resources}, }, crd::listener, k8s_openapi::api::{ apps::v1::{Deployment, StatefulSet}, - core::v1::{ConfigMap, Secret, Service}, + core::v1::{ConfigMap, Secret, Service, ServiceAccount}, policy::v1::PodDisruptionBudget, + rbac::v1::RoleBinding, }, kube::{ - Resource, ResourceExt, + Resource, api::ObjectMeta, core::{DeserializeGuard, error_boundary}, runtime::controller::Action, @@ -44,7 +44,7 @@ use stackable_operator::{ kvp::label::{recommended_labels, role_group_selector}, product_logging::framework::{ValidatedContainerLogConfigChoice, VectorContainerLogConfig}, role_group_utils::ResourceNames, - role_utils::{GenericCommonConfig, RoleGroupConfig}, + role_utils::{self, GenericCommonConfig, RoleGroupConfig}, types::{ kubernetes::{ListenerClassName, ListenerName, NamespaceName, Uid}, operator::{ @@ -94,6 +94,8 @@ pub struct KubernetesResources { pub listeners: Vec, pub config_maps: Vec, pub pod_disruption_budgets: Vec, + pub service_accounts: Vec, + pub role_bindings: Vec, } /// Per-role configuration extracted during validation. @@ -226,24 +228,33 @@ impl ValidatedCluster { } } - pub fn resource_names( + pub fn role_group_resource_names( &self, role: &SupersetRole, role_group_name: &RoleGroupName, ) -> ResourceNames { ResourceNames { cluster_name: self.name.clone(), - role_name: role.role_name(), + role_name: role.into(), role_group_name: role_group_name.clone(), } } + /// Type-safe names for the per-cluster RBAC resources: the ServiceAccount shared by all + /// Pods, its (namespaced) RoleBinding, and the operator-deployed ClusterRole it binds. + pub fn cluster_resource_names(&self) -> role_utils::ResourceNames { + role_utils::ResourceNames { + cluster_name: self.name.clone(), + product_name: product_name(), + } + } + pub fn recommended_labels( &self, role: &SupersetRole, role_group_name: &RoleGroupName, ) -> Labels { - self.recommended_labels_for(&role.role_name(), role_group_name) + self.recommended_labels_for(&role.into(), role_group_name) } pub fn recommended_labels_for( @@ -263,7 +274,7 @@ impl ValidatedCluster { ) -> Labels { self.recommended_labels_with( &build::UNVERSIONED_PRODUCT_VERSION, - &role.role_name(), + &role.into(), role_group_name, ) } @@ -290,7 +301,7 @@ impl ValidatedCluster { role: &SupersetRole, role_group_name: &RoleGroupName, ) -> Labels { - role_group_selector(self, &product_name(), &role.role_name(), role_group_name) + role_group_selector(self, &product_name(), &role.into(), role_group_name) } /// Returns an [`ObjectMetaBuilder`] pre-filled with the namespace, an owner reference back to @@ -409,27 +420,6 @@ pub enum Error { source: stackable_operator::client::Error, }, - #[snafu(display("failed to patch service account"))] - ApplyServiceAccount { - source: stackable_operator::cluster_resources::Error, - }, - - #[snafu(display("failed to patch role binding"))] - 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 get required Labels"))] - GetRequiredLabels { - source: - stackable_operator::kvp::KeyValuePairError, - }, - #[snafu(display("SupersetCluster object is invalid"))] InvalidSupersetCluster { source: error_boundary::InvalidObject, @@ -504,24 +494,6 @@ pub async fn reconcile_superset( &superset.spec.object_overrides, ); - let (rbac_sa, rbac_rolebinding) = build_rbac_resources( - superset, - APP_NAME, - cluster_resources - .get_required_labels() - .context(GetRequiredLabelsSnafu)?, - ) - .context(BuildRBACObjectsSnafu)?; - - let rbac_sa = cluster_resources - .add(client, rbac_sa) - .await - .context(ApplyServiceAccountSnafu)?; - cluster_resources - .add(client, rbac_rolebinding) - .await - .context(ApplyRoleBindingSnafu)?; - // TODO: Can be removed after SDP 26.7 is released (it's only a migration from 26.3 - 26.7) // (don't forget about the snafu Error variants). // Removal is tracked in https://github.com/stackabletech/superset-operator/issues/755 @@ -536,7 +508,7 @@ pub async fn reconcile_superset( .await .context(CreateSecretKeySecretSnafu)?; - let resources = build::build(&validated, &rbac_sa.name_any()).context(BuildResourcesSnafu)?; + let resources = build::build(&validated).context(BuildResourcesSnafu)?; let mut statefulset_cond_builder = StatefulSetConditionBuilder::default(); let mut deployment_cond_builder = DeploymentConditionBuilder::default(); @@ -544,6 +516,18 @@ pub async fn reconcile_superset( // The StatefulSets/Deployments are applied last, so every ConfigMap and Secret they mount // already exists — otherwise a changed mount would restart the Pods. // See https://github.com/stackabletech/commons-operator/issues/111 for details. + 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) diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index 0b2a1a0e..d651a6be 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -13,6 +13,7 @@ use crate::{ deployment::build_rolegroup_deployment, 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_node_rolegroup_statefulset, }, @@ -26,7 +27,7 @@ pub mod resource; // Placeholder role-group name used for the recommended labels of the role-level `Listener` // (which is not tied to a single role group). -stackable_operator::constant!(pub(crate) PLACEHOLDER_LISTENER_ROLE_GROUP: RoleGroupName = "none"); +stackable_operator::constant!(pub(crate) NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); // Product version used for the recommended labels of PVC templates, which cannot be modified after // deployment. A constant `none` keeps those labels stable across version upgrades. @@ -54,13 +55,7 @@ pub enum Error { } /// Builds every Kubernetes resource for the given validated cluster. -/// -/// `service_account_name` is the name of the RBAC `ServiceAccount` the role-group Pods run under. -/// The RBAC resources themselves are built and applied separately in the reconcile step. -pub fn build( - cluster: &ValidatedCluster, - service_account_name: &str, -) -> Result { +pub fn build(cluster: &ValidatedCluster) -> Result { let mut stateful_sets = vec![]; let mut deployments = vec![]; let mut services = vec![]; @@ -110,7 +105,6 @@ pub fn build( superset_role, role_group_name, rolegroup_config, - service_account_name, ) .context(StatefulSetSnafu { role_group: role_group_name.clone(), @@ -124,7 +118,6 @@ pub fn build( superset_role, role_group_name, rolegroup_config, - service_account_name, ) .context(DeploymentSnafu { role_group: role_group_name.clone(), @@ -162,11 +155,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, utils::yaml_from_str_singleton_map}; use super::build; @@ -230,7 +227,7 @@ mod tests { #[test] fn build_produces_expected_resource_names() { let cluster = validated_cluster(); - let resources = build(&cluster, "simple-superset-serviceaccount").expect("build succeeds"); + let resources = build(&cluster).expect("build succeeds"); assert_eq!( sorted_names(&resources.stateful_sets), @@ -263,4 +260,53 @@ mod tests { ] ); } + + /// 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 = validated_cluster(); + let resources = build(&cluster).expect("build succeeds"); + + assert_eq!( + sorted_names(&resources.service_accounts), + ["simple-superset-serviceaccount"] + ); + assert_eq!( + sorted_names(&resources.role_bindings), + ["simple-superset-rolebinding"] + ); + + let expected_labels = BTreeMap::from( + [ + ("app.kubernetes.io/component", "none"), + ("app.kubernetes.io/instance", "simple-superset"), + ( + "app.kubernetes.io/managed-by", + "superset.stackable.tech_supersetcluster", + ), + ("app.kubernetes.io/name", "superset"), + ("app.kubernetes.io/role-group", "none"), + ("app.kubernetes.io/version", "4.1.4-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, "superset-clusterrole"); + } } diff --git a/rust/operator-binary/src/controller/build/resource/config_map.rs b/rust/operator-binary/src/controller/build/resource/config_map.rs index 77683dfb..460f7129 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -49,7 +49,7 @@ pub fn build_rolegroup_config_map( validated .object_meta( validated - .resource_names(role, role_group_name) + .role_group_resource_names(role, role_group_name) .role_group_config_map() .to_string(), role, diff --git a/rust/operator-binary/src/controller/build/resource/deployment.rs b/rust/operator-binary/src/controller/build/resource/deployment.rs index a857c602..9e0d48ac 100644 --- a/rust/operator-binary/src/controller/build/resource/deployment.rs +++ b/rust/operator-binary/src/controller/build/resource/deployment.rs @@ -64,11 +64,10 @@ pub fn build_rolegroup_deployment( superset_role: &SupersetRole, role_group_name: &RoleGroupName, rolegroup_config: &SupersetRoleGroupConfig, - sa_name: &str, ) -> Result { let merged_config = &rolegroup_config.config; - let resource_names = validated.resource_names(superset_role, role_group_name); + let resource_names = validated.role_group_resource_names(superset_role, role_group_name); let recommended_object_labels = validated.recommended_labels(superset_role, role_group_name); // The Celery process command, liveness probe and replica policy are the only differences @@ -102,7 +101,12 @@ pub fn build_rolegroup_deployment( .build(), ) .affinity(&merged_config.affinity) - .service_account_name(sa_name); + .service_account_name( + validated + .cluster_resource_names() + .service_account_name() + .to_string(), + ); let mut superset_cb = super::build_superset_container_builder(validated, rolegroup_config) .context(BuildContainerSnafu)?; diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index 1319a235..6f7ac139 100644 --- a/rust/operator-binary/src/controller/build/resource/listener.rs +++ b/rust/operator-binary/src/controller/build/resource/listener.rs @@ -1,7 +1,7 @@ use stackable_operator::{crd::listener, v2::types::kubernetes::ListenerClassName}; use crate::{ - controller::{ValidatedCluster, build::PLACEHOLDER_LISTENER_ROLE_GROUP}, + controller::{ValidatedCluster, build::NONE_ROLE_GROUP_NAME}, crd::{APP_PORT, APP_PORT_NAME, SupersetRole}, }; @@ -16,7 +16,7 @@ pub fn build_group_listener( // The group listener is a role-level object, so the constant `none` placeholder role-group is // used for the recommended labels. let metadata = validated - .object_meta(listener_group_name, role, &PLACEHOLDER_LISTENER_ROLE_GROUP) + .object_meta(listener_group_name, role, &NONE_ROLE_GROUP_NAME) .build(); let spec = listener::v1alpha1::ListenerSpec { diff --git a/rust/operator-binary/src/controller/build/resource/mod.rs b/rust/operator-binary/src/controller/build/resource/mod.rs index 9c9bd98b..05f3b529 100644 --- a/rust/operator-binary/src/controller/build/resource/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/mod.rs @@ -49,6 +49,7 @@ pub mod config_map; pub mod deployment; pub mod listener; pub mod pdb; +pub mod rbac; pub mod service; pub mod statefulset; @@ -252,7 +253,7 @@ pub(crate) fn build_vector_container( &Container::Vector.to_container_name(), &validated.image, vector_log_config, - &validated.resource_names(superset_role, role_group_name), + &validated.role_group_resource_names(superset_role, role_group_name), &CONFIG_VOLUME_NAME, &LOG_VOLUME_NAME, EnvVarSet::new(), diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index 002e73ca..7aece9a3 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -25,7 +25,7 @@ pub fn build_pdb( let pdb = pod_disruption_budget_builder_with_role( validated, &product_name(), - &role.role_name(), + &role.into(), &operator_name(), &controller_name(), ) diff --git a/rust/operator-binary/src/controller/build/resource/rbac.rs b/rust/operator-binary/src/controller/build/resource/rbac.rs new file mode 100644 index 00000000..e59a8818 --- /dev/null +++ b/rust/operator-binary/src/controller/build/resource/rbac.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::controller::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.cluster_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.cluster_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) +} diff --git a/rust/operator-binary/src/controller/build/resource/service.rs b/rust/operator-binary/src/controller/build/resource/service.rs index ab350ba0..92f1eefe 100644 --- a/rust/operator-binary/src/controller/build/resource/service.rs +++ b/rust/operator-binary/src/controller/build/resource/service.rs @@ -28,7 +28,7 @@ pub fn build_rolegroup_headless_service( metadata: validated .object_meta( validated - .resource_names(role, role_group_name) + .role_group_resource_names(role, role_group_name) .headless_service_name() .to_string(), role, @@ -54,7 +54,7 @@ pub fn build_rolegroup_metrics_service( role: &SupersetRole, role_group_name: &RoleGroupName, ) -> Service { - let resource_names = validated.resource_names(role, role_group_name); + let resource_names = validated.role_group_resource_names(role, role_group_name); Service { metadata: validated .object_meta( diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 0ec8b888..5799c899 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -94,11 +94,10 @@ pub fn build_node_rolegroup_statefulset( superset_role: &SupersetRole, role_group_name: &RoleGroupName, rolegroup_config: &SupersetRoleGroupConfig, - sa_name: &str, ) -> Result { let merged_config = &rolegroup_config.config; - let resource_names = validated.resource_names(superset_role, role_group_name); + let resource_names = validated.role_group_resource_names(superset_role, role_group_name); let recommended_object_labels = validated.recommended_labels(superset_role, role_group_name); // Used for PVC templates that cannot be modified once they are deployed (a constant "none" // version keeps the labels stable across version upgrades). @@ -118,7 +117,12 @@ pub fn build_node_rolegroup_statefulset( .build(), ) .affinity(&merged_config.affinity) - .service_account_name(sa_name); + .service_account_name( + validated + .cluster_resource_names() + .service_account_name() + .to_string(), + ); let mut superset_cb = super::build_superset_container_builder(validated, rolegroup_config) .context(BuildContainerSnafu)?; diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 0da57a77..6ddc31b4 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -30,6 +30,7 @@ use stackable_operator::{ types::{ common::Port, kubernetes::{ConfigMapName, ContainerName, ListenerClassName}, + operator::RoleName, }, }, versioned::versioned, @@ -417,11 +418,23 @@ impl SupersetRole { Self::Worker | Self::Beat => None, } } +} + +impl From for RoleName { + fn from(value: SupersetRole) -> Self { + value + .to_string() + .parse() + .expect("a SupersetRole serialises to a valid RoleName") + } +} - pub fn role_name(&self) -> stackable_operator::v2::types::operator::RoleName { - self.to_string() +impl From<&SupersetRole> for RoleName { + fn from(value: &SupersetRole) -> Self { + value + .to_string() .parse() - .expect("a Superset serialises to a valid RoleName") + .expect("a SupersetRole serialises to a valid RoleName") } } @@ -603,9 +616,22 @@ impl v1alpha1::SupersetCluster { #[cfg(test)] mod tests { - use stackable_operator::versioned::test_utils::RoundtripTestData; - - use super::v1alpha1; + use stackable_operator::{ + v2::types::operator::RoleName, versioned::test_utils::RoundtripTestData, + }; + use strum::IntoEnumIterator; + + use super::{SupersetRole, v1alpha1}; + + /// Locks the invariant behind the `expect` in the `From for RoleName` impls: + /// every `SupersetRole` variant (present and future) must serialise to a valid `RoleName`. + #[test] + fn every_superset_role_serialises_to_a_valid_role_name() { + for role in SupersetRole::iter() { + let _: RoleName = (&role).into(); + let _: RoleName = role.into(); + } + } impl RoundtripTestData for v1alpha1::SupersetClusterSpec { fn roundtrip_test_data() -> Vec {