diff --git a/CHANGELOG.md b/CHANGELOG.md index 158b9611..7a4dfcb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,9 +8,12 @@ All notable changes to this project will be documented in this file. - Internal operator refactoring: introduce a build() step in the reconciler that assembles all relevant Kubernetes resources before anything is applied ([#841]). +- The RBAC ServiceAccount and RoleBinding are now built with the operator-rs `v2::rbac` + functions and carry the full set of recommended labels ([#846]). - Bump stackable-operator to 0.114.0 ([#855]). [#841]: https://github.com/stackabletech/druid-operator/pull/841 +[#846]: https://github.com/stackabletech/druid-operator/pull/846 [#855]: https://github.com/stackabletech/druid-operator/pull/855 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index ed5aef14..642a31bb 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -8,19 +8,17 @@ use snafu::{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::{ - ResourceExt, core::{DeserializeGuard, error_boundary}, runtime::controller::Action, }, - kvp::{KeyValuePairError, LabelValueError}, logging::controller::ReconcilerError, shared::time::Duration, status::condition::{ @@ -83,6 +81,8 @@ pub struct KubernetesResources { pub listeners: Vec, pub config_maps: Vec, pub pod_disruption_budgets: Vec, + pub service_accounts: Vec, + pub role_bindings: Vec, } #[derive(Snafu, Debug, EnumDiscriminants)] @@ -122,26 +122,6 @@ pub enum Error { source: crate::internal_secret::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 get required labels"))] - GetRequiredLabels { - source: KeyValuePairError, - }, - #[snafu(display("DruidCluster object is invalid"))] InvalidDruidCluster { source: error_boundary::InvalidObject, @@ -196,39 +176,30 @@ pub async fn reconcile_druid( &druid.spec.object_overrides, ); - let (rbac_sa, rbac_rolebinding) = build_rbac_resources( - druid, - APP_NAME, - cluster_resources - .get_required_labels() - .context(GetRequiredLabelsSnafu)?, - ) - .context(BuildRbacResourcesSnafu)?; - - // The ServiceAccount name is deterministic on the built object, so the StatefulSet builder only - // needs the name and 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)?; - // The internal secret is shared across all roles and role groups, so it only needs to be // created once per reconcile rather than inside the role loop below. create_shared_internal_secret(&validated_cluster, client, DRUID_CONTROLLER_NAME) .await .context(FailedInternalSecretCreationSnafu)?; - 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(); + 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)?; + } + // Apply order: everything a Pod mounts (ConfigMaps) must exist before the StatefulSets, so the // StatefulSets are applied last to prevent unnecessary Pod restarts. // See https://github.com/stackabletech/commons-operator/issues/111 for details. diff --git a/rust/operator-binary/src/controller/build/mod.rs b/rust/operator-binary/src/controller/build/mod.rs index ceeada95..10f57a86 100644 --- a/rust/operator-binary/src/controller/build/mod.rs +++ b/rust/operator-binary/src/controller/build/mod.rs @@ -3,7 +3,7 @@ use std::str::FromStr; use snafu::{ResultExt, Snafu}; -use stackable_operator::v2::types::operator::{ProductVersion, RoleGroupName}; +use stackable_operator::v2::types::operator::RoleGroupName; use crate::{ controller::{ @@ -12,6 +12,7 @@ use crate::{ config_map::build_rolegroup_config_map, listener::{build_group_listener, group_listener_name}, pdb::build_pdb, + rbac::{build_role_binding, build_service_account}, service::{build_rolegroup_headless_service, build_rolegroup_metrics_service}, statefulset::build_rolegroup_statefulset, }, @@ -26,11 +27,7 @@ stackable_operator::constant!(pub(crate) PLACEHOLDER_DISCOVERY_ROLE_GROUP: RoleG // 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"); - -// 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"); +stackable_operator::constant!(pub(crate) NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); pub mod authentication; pub mod graceful_shutdown; @@ -57,19 +54,13 @@ pub enum Error { /// Builds the Kubernetes resources for the given validated cluster. /// /// Does not need a Kubernetes client: every reference to another Kubernetes resource is already -/// dereferenced and validated by this point. The remaining errors are resource-assembly failures -/// only. -/// -/// `service_account_name` is the name of the RBAC `ServiceAccount` the role-group Pods run under -/// (RBAC resources are built and applied separately, in the reconcile step). +/// dereferenced and validated by this point. +/// The remaining errors are resource-assembly failures only. /// /// The Router group `Listener` and the discovery `ConfigMap`s are not built here: the discovery /// `ConfigMap` derives from the *applied* Router listener's ingress address, so both are built and /// applied in the reconcile step instead. -pub fn build( - cluster: &ValidatedCluster, - service_account_name: &str, -) -> Result { +pub fn build(cluster: &ValidatedCluster) -> Result { let mut stateful_sets = vec![]; let mut services = vec![]; let mut listeners = vec![]; @@ -116,16 +107,11 @@ pub fn build( )?, ); stateful_sets.push( - build_rolegroup_statefulset( - cluster, - druid_role, - role_group_name, - rg, - service_account_name, - ) - .context(StatefulSetSnafu { - role_group: role_group_name.clone(), - })?, + build_rolegroup_statefulset(cluster, druid_role, role_group_name, rg).context( + StatefulSetSnafu { + role_group: role_group_name.clone(), + }, + )?, ); } } @@ -136,11 +122,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; @@ -161,7 +151,7 @@ mod tests { fn build_produces_expected_resource_names() { let druid = druid_from_yaml(MINIMAL_DRUID_YAML); let cluster = validated_cluster(&druid); - let resources = build(&cluster, "simple-druid-serviceaccount").expect("build succeeds"); + let resources = build(&cluster).expect("build succeeds"); // One StatefulSet and one ConfigMap per role group (one role group per role). let expected_role_group_names = [ @@ -188,4 +178,54 @@ mod tests { ["simple-druid-broker", "simple-druid-coordinator"] ); } + + /// 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 druid = druid_from_yaml(MINIMAL_DRUID_YAML); + let cluster = validated_cluster(&druid); + let resources = build(&cluster).expect("build succeeds"); + + assert_eq!( + sorted_names(&resources.service_accounts), + ["simple-druid-serviceaccount"] + ); + assert_eq!( + sorted_names(&resources.role_bindings), + ["simple-druid-rolebinding"] + ); + + let expected_labels = BTreeMap::from( + [ + ("app.kubernetes.io/component", "none"), + ("app.kubernetes.io/instance", "simple-druid"), + ( + "app.kubernetes.io/managed-by", + "druid.stackable.tech_druidcluster", + ), + ("app.kubernetes.io/name", "druid"), + ("app.kubernetes.io/role-group", "none"), + ("app.kubernetes.io/version", "30.0.0-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, "druid-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 ee8805e6..6e3c0357 100644 --- a/rust/operator-binary/src/controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/controller/build/resource/config_map.rs @@ -151,7 +151,7 @@ pub fn build_rolegroup_config_map( role_group_name: &RoleGroupName, rg: &DruidRoleGroupConfig, ) -> Result { - let resource_names = cluster.resource_names(role, role_group_name); + let resource_names = cluster.role_group_resource_names(role, role_group_name); let cluster_config = &cluster.cluster_config; let druid_tls_security = &cluster_config.druid_tls_security; let druid_auth_config = &cluster_config.druid_auth_config; diff --git a/rust/operator-binary/src/controller/build/resource/listener.rs b/rust/operator-binary/src/controller/build/resource/listener.rs index a154cce6..78845d01 100644 --- a/rust/operator-binary/src/controller/build/resource/listener.rs +++ b/rust/operator-binary/src/controller/build/resource/listener.rs @@ -15,7 +15,7 @@ use stackable_operator::{ use crate::{ controller::{ - build::{PLACEHOLDER_LISTENER_ROLE_GROUP, security::listener_ports}, + build::{NONE_ROLE_GROUP_NAME, security::listener_ports}, validate::ValidatedCluster, }, crd::{ @@ -52,7 +52,7 @@ pub fn build_group_listener( .object_meta( listener_group_name.to_string(), druid_role, - &PLACEHOLDER_LISTENER_ROLE_GROUP, + &NONE_ROLE_GROUP_NAME, ) .build(), 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 d7fba99e..ecdc6cfe 100644 --- a/rust/operator-binary/src/controller/build/resource/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/mod.rs @@ -4,5 +4,6 @@ pub mod config_map; pub mod discovery; pub mod listener; pub mod pdb; +pub mod rbac; pub mod service; pub mod statefulset; diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index e7de455b..33ab8e9b 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -27,7 +27,7 @@ pub fn build_pdb( let pdb = pod_disruption_budget_builder_with_role( cluster, &product_name(), - &role.to_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..77495b11 --- /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::validate::ValidatedCluster; + +stackable_operator::constant!(NONE_ROLE_NAME: RoleName = "none"); +stackable_operator::constant!(NONE_ROLE_GROUP_NAME: RoleGroupName = "none"); + +/// Builds the [`ServiceAccount`] that all 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 eb914248..f8bfb5c9 100644 --- a/rust/operator-binary/src/controller/build/resource/service.rs +++ b/rust/operator-binary/src/controller/build/resource/service.rs @@ -22,7 +22,7 @@ pub fn build_rolegroup_headless_service( metadata: cluster .object_meta( cluster - .resource_names(druid_role, role_group_name) + .role_group_resource_names(druid_role, role_group_name) .headless_service_name() .to_string(), druid_role, @@ -59,7 +59,7 @@ pub fn build_rolegroup_metrics_service( metadata: cluster .object_meta( cluster - .resource_names(druid_role, role_group_name) + .role_group_resource_names(druid_role, role_group_name) .metrics_service_name() .to_string(), druid_role, diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 54a478fe..7c267d63 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -34,7 +34,7 @@ use stackable_operator::{ use crate::{ controller::{ build::{ - UNVERSIONED_PRODUCT_VERSION, authentication, + authentication, graceful_shutdown::add_graceful_shutdown_config, properties::product_logging::MAX_DRUID_LOG_FILES_SIZE, resource::listener::{ @@ -113,10 +113,9 @@ pub fn build_rolegroup_statefulset( role: &DruidRole, role_group_name: &RoleGroupName, rg: &DruidRoleGroupConfig, - service_account_name: &str, ) -> Result { let merged_rolegroup_config = &rg.config; - let resource_names = cluster.resource_names(role, role_group_name); + let resource_names = cluster.role_group_resource_names(role, role_group_name); // Everything below used to be threaded in as separate parameters; it all lives on the // `ValidatedCluster` now. let resolved_product_image = &cluster.image; @@ -301,11 +300,8 @@ pub fn build_rolegroup_statefulset( .add_volume_mount(&*LISTENER_VOLUME_NAME, LISTENER_VOLUME_DIR) .context(AddVolumeMountSnafu)?; - // Used for PVC templates that cannot be modified once they are deployed - // A version value is required, and we do want to use the "recommended" format for the - // other desired labels, hence the unversioned product version. let unversioned_recommended_labels = - cluster.recommended_labels_for(role, &UNVERSIONED_PRODUCT_VERSION, role_group_name); + cluster.unversioned_recommended_labels(role, role_group_name); pvcs = Some(vec![build_group_listener_pvc( &group_listener_name, @@ -321,7 +317,12 @@ pub fn build_rolegroup_statefulset( .add_init_container(cb_prepare.build()) .add_container(cb_druid.build()) .metadata(metadata) - .service_account_name(service_account_name) + .service_account_name( + cluster + .cluster_resource_names() + .service_account_name() + .to_string(), + ) .security_context(PodSecurityContextBuilder::new().fs_group(1000).build()); // The Vector agent reads the static `vector.yaml` (added to the rolegroup ConfigMap) from the diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index bc268437..140a0512 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -28,9 +28,10 @@ use stackable_operator::{ controller_utils::{get_cluster_name, get_namespace, get_uid}, kvp::label::{recommended_labels, role_group_selector}, role_group_utils::ResourceNames, + role_utils, types::{ kubernetes::{ListenerClassName, NamespaceName, Uid}, - operator::{ClusterName, ProductVersion, RoleGroupName}, + operator::{ClusterName, ProductVersion, RoleGroupName, RoleName}, }, }, }; @@ -91,6 +92,9 @@ type Result = std::result::Result; /// re-exported here for the build step. pub use crate::crd::DruidRoleGroupConfig; +// Placeholder version label value for resources whose labels must not change after deployment. +stackable_operator::constant!(UNVERSIONED_PRODUCT_VERSION: ProductVersion = "none"); + /// Cluster-wide resolved fields that are not role/rolegroup specific. pub struct ValidatedClusterConfig { pub zookeeper_connection_string: String, @@ -193,14 +197,10 @@ impl ValidatedCluster { .expect("every DruidRole has a validated role config") } - /// Recommended labels for a role-group resource, using the given product version. - /// - /// Kept separate so the listener PVC templates (which require an immutable, version-independent - /// label set) can pass the unversioned product version. - pub(crate) fn recommended_labels_for( + fn recommended_labels_with( &self, - role: &DruidRole, product_version: &ProductVersion, + role_name: &RoleName, role_group_name: &RoleGroupName, ) -> Labels { recommended_labels( @@ -209,27 +209,39 @@ impl ValidatedCluster { product_version, &operator_name(), &controller_name(), - &role.to_role_name(), + role_name, role_group_name, ) } - /// Recommended labels for a role-group resource. - pub(crate) fn recommended_labels( + /// Recommended labels for a resource that is not tied to a concrete [`DruidRole`] (e.g. the + /// cluster-shared RBAC resources), using a free-form role/role-group label value. + pub fn recommended_labels_for( &self, - role: &DruidRole, + role_name: &RoleName, role_group_name: &RoleGroupName, ) -> Labels { - self.recommended_labels_for(role, &self.product_version, role_group_name) + self.recommended_labels_with(&self.product_version, role_name, role_group_name) } - /// Selector labels matching the pods of a role group. - pub(crate) fn role_group_selector( + /// Recommended labels for a role-group resource. + pub fn recommended_labels(&self, role: &DruidRole, role_group_name: &RoleGroupName) -> Labels { + self.recommended_labels_for(&role.into(), role_group_name) + } + + /// Recommended labels with the constant [`UNVERSIONED_PRODUCT_VERSION`], for PVC templates + /// that cannot be modified after deployment (keeps the labels stable across version upgrades). + pub fn unversioned_recommended_labels( &self, role: &DruidRole, role_group_name: &RoleGroupName, ) -> Labels { - role_group_selector(self, &product_name(), &role.to_role_name(), role_group_name) + self.recommended_labels_with(&UNVERSIONED_PRODUCT_VERSION, &role.into(), role_group_name) + } + + /// Selector labels matching the pods of a role group. + pub fn role_group_selector(&self, role: &DruidRole, role_group_name: &RoleGroupName) -> Labels { + role_group_selector(self, &product_name(), &role.into(), role_group_name) } /// Returns an [`ObjectMetaBuilder`] pre-filled with the namespace, an owner reference back to @@ -252,15 +264,24 @@ impl ValidatedCluster { builder } + /// 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(), + } + } + /// Type-safe names for the resources of the given role's role group. - pub(crate) fn resource_names( + pub(crate) fn role_group_resource_names( &self, role: &DruidRole, role_group_name: &RoleGroupName, ) -> ResourceNames { ResourceNames { cluster_name: self.name.clone(), - role_name: role.to_role_name(), + role_name: role.into(), role_group_name: role_group_name.clone(), } } @@ -574,7 +595,7 @@ spec: #[cfg(test)] mod tests { - use stackable_operator::cli::OperatorEnvironmentOptions; + use stackable_operator::{cli::OperatorEnvironmentOptions, v2::types::operator::RoleName}; use strum::IntoEnumIterator; use super::{ @@ -682,4 +703,14 @@ mod tests { "expected a ClusterIdentity error when the cluster has no uid" ); } + + /// Locks the invariant behind the `expect` in the `From for RoleName` impls: + /// every `DruidRole` variant (present and future) must serialise to a valid `RoleName`. + #[test] + fn every_druid_role_serialises_to_a_valid_role_name() { + for role in DruidRole::iter() { + let _: RoleName = (&role).into(); + let _: RoleName = role.into(); + } + } } diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index d7f572ee..dbd2f360 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -630,13 +630,21 @@ pub enum DruidRole { Router, } -impl DruidRole { - /// Returns the typed role name used for Kubernetes labels and selectors. - pub fn to_role_name(&self) -> RoleName { - RoleName::from_str(&self.to_string()) +impl From for RoleName { + fn from(value: DruidRole) -> Self { + RoleName::from_str(&value.to_string()) .expect("a DruidRole always serializes to a valid role name") } +} +impl From<&DruidRole> for RoleName { + fn from(value: &DruidRole) -> Self { + RoleName::from_str(&value.to_string()) + .expect("a DruidRole always serializes to a valid role name") + } +} + +impl DruidRole { /// Returns the name of the internal druid process name associated with the role. /// These strings are used by druid internally to identify processes. fn get_process_name(&self) -> &str {