diff --git a/CHANGELOG.md b/CHANGELOG.md index 43f7b2ac..2845c927 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,13 @@ 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 ([#1053]). - Bump `stackable-operator` to 0.114.0 ([#1063]). +- The RBAC ServiceAccount and RoleBinding are now built with the operator-rs `v2::rbac` + functions and carry the full set of recommended labels ([#1060]). +- BREAKING: The `servers` role is now required by the CRD. + Previously a ZookeeperCluster without it was accepted by the API server but failed reconciliation ([#1060]). [#1053]: https://github.com/stackabletech/zookeeper-operator/pull/1053 +[#1060]: https://github.com/stackabletech/zookeeper-operator/pull/1060 [#1063]: https://github.com/stackabletech/zookeeper-operator/pull/1063 ## [26.7.0] - 2026-07-21 diff --git a/Cargo.toml b/Cargo.toml index 92483b1a..3153cb6b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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 = "main" } # stackable-operator = { path = "../operator-rs/crates/stackable-operator" } [patch.crates-io] diff --git a/extra/crds.yaml b/extra/crds.yaml index 1ae729c9..d78c39e8 100644 --- a/extra/crds.yaml +++ b/extra/crds.yaml @@ -223,7 +223,6 @@ spec: at role level, the `roleConfig`. You can learn more about this in the [Roles and role group concept documentation](https://docs.stackable.tech/home/nightly/concepts/roles-and-role-groups). - nullable: true properties: cliOverrides: additionalProperties: @@ -1303,6 +1302,7 @@ spec: type: object required: - image + - servers type: object status: nullable: true diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 890fde04..811925bf 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -1,7 +1,6 @@ use std::str::FromStr; use serde::{Deserialize, Serialize}; -use snafu::{OptionExt, Snafu}; use stackable_operator::{ commons::{ affinity::StackableAffinity, @@ -31,6 +30,7 @@ use stackable_operator::{ kubernetes::{ ConfigMapName, ListenerClassName, ListenerName, NamespaceName, ServiceName, }, + operator::RoleName, }, }, versioned::versioned, @@ -81,12 +81,6 @@ pub const CONTAINER_IMAGE_BASE_NAME: &str = "zookeeper"; const DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT: Duration = Duration::from_minutes_unchecked(2); pub const DEFAULT_LISTENER_CLASS: &str = "cluster-internal"; -#[derive(Snafu, Debug)] -pub enum Error { - #[snafu(display("the role {role} is not defined"))] - CannotRetrieveZookeeperRole { role: String }, -} - pub type ZookeeperServerRoleType = Role< v1alpha1::ZookeeperConfigFragment, v1alpha1::ZookeeperConfigOverrides, @@ -136,8 +130,7 @@ pub mod versioned { pub image: ProductImage, // no doc - it's in the struct. - #[serde(skip_serializing_if = "Option::is_none")] - pub servers: Option, + pub servers: ZookeeperServerRoleType, } #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -335,6 +328,18 @@ pub enum ZookeeperRole { Server, } +impl From for RoleName { + fn from(value: ZookeeperRole) -> Self { + RoleName::from_str(&value.to_string()).expect("a ZookeeperRole is a valid role name") + } +} + +impl From<&ZookeeperRole> for RoleName { + fn from(value: &ZookeeperRole) -> Self { + RoleName::from_str(&value.to_string()).expect("a ZookeeperRole is a valid role name") + } +} + /// Reference to a single `Pod` that is a component of a [`v1alpha1::ZookeeperCluster`] /// /// Used for service discovery. @@ -446,19 +451,16 @@ impl v1alpha1::ZookeeperCluster { )) } - /// Returns a reference to the role. Raises an error if the role is not defined. - pub fn role(&self, role_variant: &ZookeeperRole) -> Result<&ZookeeperServerRoleType, Error> { + /// Returns the given role (the `servers` role is required by the CRD). + pub fn role(&self, role_variant: &ZookeeperRole) -> &ZookeeperServerRoleType { match role_variant { - ZookeeperRole::Server => self.spec.servers.as_ref(), + ZookeeperRole::Server => &self.spec.servers, } - .with_context(|| CannotRetrieveZookeeperRoleSnafu { - role: role_variant.to_string(), - }) } - pub fn role_config(&self, role: &ZookeeperRole) -> Option<&ZookeeperServerRoleConfig> { + pub fn role_config(&self, role: &ZookeeperRole) -> &ZookeeperServerRoleConfig { match role { - ZookeeperRole::Server => self.spec.servers.as_ref().map(|s| &s.role_config), + ZookeeperRole::Server => &self.spec.servers.role_config, } } } @@ -498,6 +500,10 @@ mod tests { spec: image: productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 "#; let zookeeper: v1alpha1::ZookeeperCluster = serde_yaml::from_str(input).expect("illegal test input"); @@ -518,6 +524,10 @@ mod tests { spec: image: productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 clusterConfig: tls: serverSecretClass: simple-zookeeper-client-tls @@ -542,6 +552,10 @@ mod tests { spec: image: productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 clusterConfig: tls: serverSecretClass: null @@ -562,6 +576,10 @@ mod tests { spec: image: productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 clusterConfig: tls: quorumSecretClass: simple-zookeeper-quorum-tls @@ -588,6 +606,10 @@ mod tests { spec: image: productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 "#; let zookeeper: v1alpha1::ZookeeperCluster = serde_yaml::from_str(input).expect("illegal test input"); @@ -609,6 +631,10 @@ mod tests { spec: image: productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 clusterConfig: tls: quorumSecretClass: simple-zookeeper-quorum-tls @@ -632,6 +658,10 @@ mod tests { spec: image: productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 clusterConfig: tls: serverSecretClass: simple-zookeeper-server-tls diff --git a/rust/operator-binary/src/zk_controller.rs b/rust/operator-binary/src/zk_controller.rs index 4f81ffa6..07296bae 100644 --- a/rust/operator-binary/src/zk_controller.rs +++ b/rust/operator-binary/src/zk_controller.rs @@ -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, @@ -69,12 +68,6 @@ pub enum Error { #[snafu(display("failed to validate cluster"))] ValidateCluster { source: validate::Error }, - #[snafu(display("crd validation failure"))] - CrdValidationFailure { source: crate::crd::Error }, - - #[snafu(display("internal operator failure"))] - InternalOperatorFailure { source: crate::crd::Error }, - #[snafu(display("failed to build the Kubernetes resources"))] BuildResources { source: build::Error }, @@ -106,29 +99,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, @@ -145,8 +120,6 @@ impl ReconcilerError for Error { Error::InvalidZookeeperCluster { .. } => None, Error::Dereference { .. } => None, Error::ValidateCluster { .. } => None, - Error::CrdValidationFailure { .. } => None, - Error::InternalOperatorFailure { .. } => None, Error::BuildResources { .. } => None, Error::ApplyResource { .. } => None, Error::ObjectMissingMetadataForOwnerRef { .. } => None, @@ -154,11 +127,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 +142,8 @@ pub struct KubernetesResources { pub listeners: Vec, pub config_maps: Vec, pub pod_disruption_budgets: Vec, + pub service_accounts: Vec, + pub role_bindings: Vec, } pub async fn reconcile_zk( @@ -209,30 +180,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) @@ -345,8 +310,22 @@ pub(crate) mod test_support { }, }; + /// The expected `app.kubernetes.io/version` label value for the given product version. + /// + /// The `-stackable` suffix carries the operator's own version, which is `0.0.0-dev` on main + /// but rewritten by the release process — so tests must derive it rather than hardcode it, + /// or they fail on release branches. + pub fn app_version_label(product_version: &str) -> String { + format!( + "{product_version}-stackable{}", + crate::built_info::PKG_VERSION + ) + } + /// Parses a minimal `ZookeeperCluster` test fixture, defaulting `namespace`/`uid` so the - /// validate step can build a [`ValidatedCluster`]. + /// validate step can build a [`ValidatedCluster`]. Test fixtures name their clusters + /// `simple-zookeeper` — deliberately different from the product name (`zookeeper`), so tests + /// asserting recommended labels catch swapped `name`/`instance` values. pub fn minimal_zk(yaml: &str) -> v1alpha1::ZookeeperCluster { let mut zk: v1alpha1::ZookeeperCluster = serde_yaml::from_str(yaml).expect("invalid test ZookeeperCluster YAML"); diff --git a/rust/operator-binary/src/zk_controller/build.rs b/rust/operator-binary/src/zk_controller/build.rs index 1c6dd520..f467fe40 100644 --- a/rust/operator-binary/src/zk_controller/build.rs +++ b/rust/operator-binary/src/zk_controller/build.rs @@ -13,8 +13,9 @@ use std::str::FromStr; use snafu::{ResultExt, Snafu}; use stackable_operator::{ + builder::meta::ObjectMetaBuilder, utils::cluster_info::KubernetesClusterInfo, - v2::types::operator::{ProductVersion, RoleGroupName}, + v2::{builder::meta::ownerreference_from_resource, types::operator::RoleGroupName}, }; use crate::{ @@ -25,6 +26,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, }, @@ -40,11 +42,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 command; pub mod graceful_shutdown; @@ -123,9 +121,7 @@ pub fn build( ); } - if let Some(role_config) = &cluster.role_config - && let Some(pdb) = build_pdb(&role_config.pdb, cluster, &zk_role) - { + if let Some(pdb) = build_pdb(&cluster.role_config.pdb, cluster, &zk_role) { pod_disruption_budgets.push(pdb); } @@ -137,9 +133,30 @@ pub fn build( listeners, config_maps, pod_disruption_budgets, + service_accounts: vec![build_service_account(cluster)], + role_bindings: vec![build_role_binding(cluster)], }) } +/// Returns an [`ObjectMetaBuilder`] pre-filled with the namespace, an owner reference back to +/// the cluster, and the recommended labels for a resource named `name` in `role_group_name`. +/// +/// Consolidates the metadata chain repeated by the child-resource builders. Call sites that +/// need extra labels/annotations chain them onto the returned builder. +pub(crate) fn object_meta( + cluster: &ValidatedCluster, + name: impl Into, + role_group_name: &RoleGroupName, +) -> ObjectMetaBuilder { + let mut builder = ObjectMetaBuilder::new(); + builder + .name_and_namespace(cluster) + .name(name) + .ownerreference(ownerreference_from_resource(cluster, None, Some(true))) + .with_labels(cluster.recommended_labels(role_group_name)); + builder +} + #[cfg(test)] mod tests { use stackable_operator::kube::Resource; @@ -215,5 +232,14 @@ mod tests { sorted_names(&resources.pod_disruption_budgets), ["simple-zookeeper-server"] ); + // The cluster-shared RBAC pair. + assert_eq!( + sorted_names(&resources.service_accounts), + ["simple-zookeeper-serviceaccount"] + ); + assert_eq!( + sorted_names(&resources.role_bindings), + ["simple-zookeeper-rolebinding"] + ); } } diff --git a/rust/operator-binary/src/zk_controller/build/properties/zoo_cfg.rs b/rust/operator-binary/src/zk_controller/build/properties/zoo_cfg.rs index a679f178..b7230c1b 100644 --- a/rust/operator-binary/src/zk_controller/build/properties/zoo_cfg.rs +++ b/rust/operator-binary/src/zk_controller/build/properties/zoo_cfg.rs @@ -38,7 +38,7 @@ pub(crate) fn server_addresses( .into_iter() .flatten() { - let resource_names = cluster.resource_names(rg_name); + let resource_names = cluster.role_group_resource_names(rg_name); let headless_service_name = resource_names.headless_service_name(); let stateful_set_name = resource_names.stateful_set_name().to_string(); // An unset replica count (HPA-managed) predicts a single-server quorum entry, matching diff --git a/rust/operator-binary/src/zk_controller/build/resource/config_map.rs b/rust/operator-binary/src/zk_controller/build/resource/config_map.rs index 39364b7d..508ed29c 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/config_map.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/config_map.rs @@ -20,7 +20,10 @@ use stackable_operator::{ }; use crate::zk_controller::{ - build::properties::{ConfigFileName, product_logging, security_properties, zoo_cfg}, + build::{ + object_meta, + properties::{ConfigFileName, product_logging, security_properties, zoo_cfg}, + }, validate::{ValidatedCluster, ZookeeperRoleGroupConfig}, }; @@ -97,15 +100,15 @@ pub fn build_server_rolegroup_config_map( ConfigMapBuilder::new() .metadata( - cluster - .object_meta( - cluster - .resource_names(role_group_name) - .role_group_config_map() - .to_string(), - role_group_name, - ) - .build(), + object_meta( + cluster, + cluster + .role_group_resource_names(role_group_name) + .role_group_config_map() + .to_string(), + role_group_name, + ) + .build(), ) .data(data) .build() diff --git a/rust/operator-binary/src/zk_controller/build/resource/discovery.rs b/rust/operator-binary/src/zk_controller/build/resource/discovery.rs index 550bbf10..d1742478 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/discovery.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/discovery.rs @@ -15,7 +15,7 @@ use stackable_operator::{ }; use crate::{ - crd::{ZOOKEEPER_SERVER_PORT_NAME, security::ZookeeperSecurity}, + crd::{ZOOKEEPER_SERVER_PORT_NAME, ZookeeperRole, security::ZookeeperSecurity}, zk_controller::{ build::PLACEHOLDER_DISCOVERY_ROLE_GROUP, validate::{ValidatedCluster, operator_name, product_name}, @@ -150,7 +150,7 @@ fn build_discovery_configmap_for_owner( product_version, &operator_name(), &controller_name, - &ValidatedCluster::role_name(), + &ZookeeperRole::Server.into(), &role_group_name, )) .build(), diff --git a/rust/operator-binary/src/zk_controller/build/resource/listener.rs b/rust/operator-binary/src/zk_controller/build/resource/listener.rs index 61547533..34df12d7 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/listener.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/listener.rs @@ -6,7 +6,10 @@ use crate::{ crd::{ ZOOKEEPER_SERVER_PORT_NAME, ZookeeperRole, role_listener_name, security::ZookeeperSecurity, }, - zk_controller::{build::PLACEHOLDER_LISTENER_ROLE_GROUP, validate::ValidatedCluster}, + zk_controller::{ + build::{NONE_ROLE_GROUP_NAME, object_meta}, + validate::ValidatedCluster, + }, }; /// Builds the role-level [`Listener`](listener::v1alpha1::Listener) exposing the ZooKeeper servers. @@ -18,12 +21,12 @@ pub fn build_role_listener( zk_role: &ZookeeperRole, ) -> listener::v1alpha1::Listener { listener::v1alpha1::Listener { - metadata: cluster - .object_meta( - role_listener_name(cluster.name.as_ref(), zk_role), - &PLACEHOLDER_LISTENER_ROLE_GROUP, - ) - .build(), + metadata: object_meta( + cluster, + role_listener_name(cluster.name.as_ref(), zk_role), + &NONE_ROLE_GROUP_NAME, + ) + .build(), spec: listener::v1alpha1::ListenerSpec { class_name: Some(cluster.cluster_config.listener_class.to_string()), ports: Some(listener_ports(&cluster.cluster_config.zookeeper_security)), diff --git a/rust/operator-binary/src/zk_controller/build/resource/mod.rs b/rust/operator-binary/src/zk_controller/build/resource/mod.rs index 48396a03..7f57f617 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/mod.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/mod.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; diff --git a/rust/operator-binary/src/zk_controller/build/resource/pdb.rs b/rust/operator-binary/src/zk_controller/build/resource/pdb.rs index 6c8099e9..b32f533d 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/pdb.rs @@ -24,7 +24,7 @@ pub fn build_pdb( let pdb = pod_disruption_budget_builder_with_role( cluster, &product_name(), - &ValidatedCluster::role_name(), + &role.into(), &operator_name(), &controller_name(), ) diff --git a/rust/operator-binary/src/zk_controller/build/resource/rbac.rs b/rust/operator-binary/src/zk_controller/build/resource/rbac.rs new file mode 100644 index 00000000..a2daef7b --- /dev/null +++ b/rust/operator-binary/src/zk_controller/build/resource/rbac.rs @@ -0,0 +1,153 @@ +//! 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.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) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + use crate::zk_controller::test_support::{app_version_label, minimal_zk, validated_cluster}; + + // `simple-zookeeper` vs `zookeeper`: see the swap-guard note on `minimal_zk`. + fn cluster() -> ValidatedCluster { + let zookeeper = minimal_zk( + r#" + apiVersion: zookeeper.stackable.tech/v1alpha1 + kind: ZookeeperCluster + metadata: + name: simple-zookeeper + spec: + image: + productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 + "#, + ); + validated_cluster(&zookeeper) + } + + #[test] + fn test_service_account() { + let service_account = build_service_account(&cluster()); + + assert_eq!( + json!({ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": { + // The RBAC resources are cluster-shared, so role and role group are `none`. + "labels": { + "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": app_version_label("3.9.5"), + "stackable.tech/vendor": "Stackable" + }, + "name": "simple-zookeeper-serviceaccount", + "namespace": "default", + "ownerReferences": [ + { + "apiVersion": "zookeeper.stackable.tech/v1alpha1", + "controller": true, + "kind": "ZookeeperCluster", + "name": "simple-zookeeper", + "uid": "c27b3971-ca72-42c1-80a4-abdfc1db0ddd" + } + ] + } + }), + serde_json::to_value(service_account).expect("must be serializable") + ); + } + + #[test] + fn test_role_binding() { + let role_binding = build_role_binding(&cluster()); + + assert_eq!( + json!({ + "apiVersion": "rbac.authorization.k8s.io/v1", + "kind": "RoleBinding", + "metadata": { + "labels": { + "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": app_version_label("3.9.5"), + "stackable.tech/vendor": "Stackable" + }, + "name": "simple-zookeeper-rolebinding", + "namespace": "default", + "ownerReferences": [ + { + "apiVersion": "zookeeper.stackable.tech/v1alpha1", + "controller": true, + "kind": "ZookeeperCluster", + "name": "simple-zookeeper", + "uid": "c27b3971-ca72-42c1-80a4-abdfc1db0ddd" + } + ] + }, + "roleRef": { + "apiGroup": "rbac.authorization.k8s.io", + "kind": "ClusterRole", + "name": "zookeeper-clusterrole" + }, + "subjects": [ + { + "kind": "ServiceAccount", + "name": "simple-zookeeper-serviceaccount", + "namespace": "default" + } + ] + }), + serde_json::to_value(role_binding).expect("must be serializable") + ); + } +} diff --git a/rust/operator-binary/src/zk_controller/build/resource/service.rs b/rust/operator-binary/src/zk_controller/build/resource/service.rs index 81ac8b92..67cf1884 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/service.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/service.rs @@ -12,7 +12,10 @@ use crate::{ ZOOKEEPER_ELECTION_PORT, ZOOKEEPER_ELECTION_PORT_NAME, ZOOKEEPER_LEADER_PORT, ZOOKEEPER_LEADER_PORT_NAME, }, - zk_controller::validate::{ValidatedCluster, ZookeeperRoleGroupConfig}, + zk_controller::{ + build::object_meta, + validate::{ValidatedCluster, ZookeeperRoleGroupConfig}, + }, }; /// The rolegroup [`Service`] is a headless service that allows internal access to the instances of a certain rolegroup @@ -22,15 +25,15 @@ pub(crate) fn build_server_rolegroup_headless_service( cluster: &ValidatedCluster, role_group_name: &RoleGroupName, ) -> Service { - let metadata = cluster - .object_meta( - cluster - .resource_names(role_group_name) - .headless_service_name() - .to_string(), - role_group_name, - ) - .build(); + let metadata = object_meta( + cluster, + cluster + .role_group_resource_names(role_group_name) + .headless_service_name() + .to_string(), + role_group_name, + ) + .build(); let service_spec = ServiceSpec { // Internal communication does not need to be exposed @@ -69,21 +72,21 @@ pub(crate) fn build_server_rolegroup_metrics_service( rolegroup_config: &ZookeeperRoleGroupConfig, ) -> Service { let metrics_port = cluster.metrics_http_port(rolegroup_config); - let metadata = cluster - .object_meta( - cluster - .resource_names(role_group_name) - .metrics_service_name(), - role_group_name, - ) - .with_labels(prometheus_labels(&Scraping::Enabled)) - .with_annotations(prometheus_annotations( - &Scraping::Enabled, - &Scheme::Http, - "/metrics", - &metrics_port, - )) - .build(); + let metadata = object_meta( + cluster, + cluster + .role_group_resource_names(role_group_name) + .metrics_service_name(), + role_group_name, + ) + .with_labels(prometheus_labels(&Scraping::Enabled)) + .with_annotations(prometheus_annotations( + &Scraping::Enabled, + &Scheme::Http, + "/metrics", + &metrics_port, + )) + .build(); let service_spec = ServiceSpec { // Internal communication does not need to be exposed @@ -115,3 +118,104 @@ pub(crate) fn build_server_rolegroup_metrics_service( status: None, } } + +#[cfg(test)] +mod tests { + use serde_json::json; + use stackable_operator::v2::types::operator::RoleGroupName; + + use super::*; + use crate::{ + crd::ZookeeperRole, + zk_controller::test_support::{app_version_label, minimal_zk, validated_cluster}, + }; + + /// Every metrics Service must carry the Prometheus scrape label and the + /// `prometheus.io/path|port|scheme|scrape` annotations, or Prometheus stops discovering the + /// endpoints. + #[test] + fn test_rolegroup_metrics_service() { + let zookeeper = minimal_zk( + r#" + apiVersion: zookeeper.stackable.tech/v1alpha1 + kind: ZookeeperCluster + metadata: + name: simple-zookeeper + spec: + image: + productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 1 + "#, + ); + let cluster = validated_cluster(&zookeeper); + let role_group_name: RoleGroupName = "default".parse().expect("valid role group name"); + let rolegroup_config = + &cluster.role_group_configs[&ZookeeperRole::Server][&role_group_name]; + + let service = + build_server_rolegroup_metrics_service(&cluster, &role_group_name, rolegroup_config); + + assert_eq!( + json!({ + "apiVersion": "v1", + "kind": "Service", + "metadata": { + "annotations": { + "prometheus.io/path": "/metrics", + "prometheus.io/port": "7000", + "prometheus.io/scheme": "http", + "prometheus.io/scrape": "true" + }, + "labels": { + "app.kubernetes.io/component": "server", + "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": "default", + "app.kubernetes.io/version": app_version_label("3.9.5"), + "prometheus.io/scrape": "true", + "stackable.tech/vendor": "Stackable" + }, + "name": "simple-zookeeper-server-default-metrics", + "namespace": "default", + "ownerReferences": [ + { + "apiVersion": "zookeeper.stackable.tech/v1alpha1", + "controller": true, + "kind": "ZookeeperCluster", + "name": "simple-zookeeper", + "uid": "c27b3971-ca72-42c1-80a4-abdfc1db0ddd" + } + ] + }, + "spec": { + "clusterIP": "None", + "ports": [ + { + "name": "jmx-metrics", + "port": 9505, + "protocol": "TCP" + }, + { + "name": "metrics", + "port": 7000, + "protocol": "TCP" + } + ], + "publishNotReadyAddresses": true, + "selector": { + "app.kubernetes.io/component": "server", + "app.kubernetes.io/instance": "simple-zookeeper", + "app.kubernetes.io/name": "zookeeper", + "app.kubernetes.io/role-group": "default" + }, + "type": "ClusterIP" + } + }), + serde_json::to_value(service).expect("must be serializable") + ); + } +} diff --git a/rust/operator-binary/src/zk_controller/build/resource/statefulset.rs b/rust/operator-binary/src/zk_controller/build/resource/statefulset.rs index 21c104ef..91ede32e 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/statefulset.rs @@ -57,10 +57,10 @@ 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}, + object_meta, properties::{self, ConfigFileName}, }, validate::{ValidatedCluster, ZookeeperRoleGroupConfig}, @@ -156,7 +156,7 @@ pub fn build_server_rolegroup_statefulset( rolegroup_config: &ZookeeperRoleGroupConfig, ) -> Result { let merged_config = &rolegroup_config.config; - let resource_names = cluster.resource_names(role_group_name); + let resource_names = cluster.role_group_resource_names(role_group_name); let resolved_product_image = &cluster.image; let zookeeper_security = &cluster.cluster_config.zookeeper_security; let metrics_port = cluster.metrics_http_port(rolegroup_config); @@ -194,8 +194,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 +382,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.cluster_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. @@ -424,13 +423,13 @@ pub fn build_server_rolegroup_statefulset( let mut pod_template = pod_builder.build_template(); pod_template.merge_from(rolegroup_config.pod_overrides.clone()); - let metadata = cluster - .object_meta( - resource_names.stateful_set_name().to_string(), - role_group_name, - ) - .with_label(RESTART_CONTROLLER_ENABLED_LABEL.to_owned()) - .build(); + let metadata = object_meta( + cluster, + resource_names.stateful_set_name().to_string(), + role_group_name, + ) + .with_label(RESTART_CONTROLLER_ENABLED_LABEL.to_owned()) + .build(); let statefulset_spec = StatefulSetSpec { pod_management_policy: Some("Parallel".to_string()), diff --git a/rust/operator-binary/src/zk_controller/validate.rs b/rust/operator-binary/src/zk_controller/validate.rs index 37bac4ee..93300a46 100644 --- a/rust/operator-binary/src/zk_controller/validate.rs +++ b/rust/operator-binary/src/zk_controller/validate.rs @@ -11,7 +11,6 @@ use std::{collections::BTreeMap, str::FromStr}; use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{ - builder::meta::ObjectMetaBuilder, cli::OperatorEnvironmentOptions, commons::{ affinity::StackableAffinity, @@ -30,10 +29,7 @@ use stackable_operator::{ shared::time::Duration, v2::{ HasName, HasUid, NameIsValidLabelValue, - builder::{ - meta::ownerreference_from_resource, - pod::container::{EnvVarName, EnvVarSet}, - }, + builder::pod::container::{EnvVarName, EnvVarSet}, controller_utils::{get_cluster_name, get_namespace, get_uid}, kvp::label::{recommended_labels, role_group_selector}, product_logging::framework::{ @@ -41,14 +37,9 @@ use stackable_operator::{ validate_logging_configuration_for_container, }, role_group_utils::ResourceNames, - role_utils::{ - JavaCommonConfig, ResourceNames as RbacResourceNames, RoleGroupConfig, - with_validated_config, - }, + role_utils::{self, JavaCommonConfig, RoleGroupConfig, with_validated_config}, types::{ - kubernetes::{ - ConfigMapName, ListenerClassName, NamespaceName, ServiceAccountName, Uid, - }, + kubernetes::{ConfigMapName, ListenerClassName, NamespaceName, Uid}, operator::{ ClusterName, ControllerName, OperatorName, ProductName, ProductVersion, RoleGroupName, RoleName, @@ -61,7 +52,7 @@ use strum::IntoEnumIterator; use crate::{ crd::{ APP_NAME, CONTAINER_IMAGE_BASE_NAME, OPERATOR_NAME, ZookeeperRole, ZookeeperServerRoleType, - authentication, default_listener_class, + authentication, security::ZookeeperSecurity, v1alpha1::{self, ZookeeperConfig, ZookeeperConfigOverrides, ZookeeperServerRoleConfig}, }, @@ -78,12 +69,6 @@ pub enum Error { #[snafu(display("failed to validate authentication classes"))] InvalidAuthenticationClassConfiguration { source: authentication::Error }, - #[snafu(display("failed to retrieve role {role:?}"))] - MissingRole { - source: crate::crd::Error, - role: String, - }, - #[snafu(display("failed to parse role group name {role_group:?}"))] ParseRoleGroupName { source: stackable_operator::v2::macros::attributed_string_type::Error, @@ -247,7 +232,7 @@ pub struct ValidatedCluster { pub cluster_config: ValidatedClusterConfig, /// Per-role config (currently just the PodDisruptionBudget), extracted during validation so the /// apply step does not reach into the raw [`crate::crd::v1alpha1::ZookeeperCluster`]. - pub role_config: Option, + pub role_config: ValidatedRoleConfig, pub role_group_configs: BTreeMap>, /// The cluster's operation settings (pause/stop), from which the @@ -259,6 +244,10 @@ pub struct ValidatedCluster { pub object_overrides: ObjectOverrides, } +// 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!(UNVERSIONED_PRODUCT_VERSION: ProductVersion = "none"); + impl ValidatedCluster { #[allow(clippy::too_many_arguments)] pub fn new( @@ -268,7 +257,7 @@ impl ValidatedCluster { image: ResolvedProductImage, product_version: ProductVersion, cluster_config: ValidatedClusterConfig, - role_config: Option, + role_config: ValidatedRoleConfig, role_group_configs: BTreeMap< ZookeeperRole, BTreeMap, @@ -296,41 +285,51 @@ impl ValidatedCluster { } } - /// The one ZooKeeper role name (`server`). - pub fn role_name() -> RoleName { - RoleName::from_str(&ZookeeperRole::Server.to_string()) - .expect("the server role name is a valid role name") - } - /// Type-safe names for the resources of a given role group. - pub(crate) fn resource_names(&self, role_group_name: &RoleGroupName) -> ResourceNames { + pub(crate) fn role_group_resource_names( + &self, + role_group_name: &RoleGroupName, + ) -> ResourceNames { ResourceNames { cluster_name: self.name.clone(), - role_name: Self::role_name(), + role_name: ZookeeperRole::Server.into(), role_group_name: role_group_name.clone(), } } - /// The RBAC ServiceAccount name for this cluster, `-serviceaccount`. - /// - /// Matches the name produced by - /// [`build_rbac_resources`](stackable_operator::commons::rbac::build_rbac_resources) so the - /// StatefulSet can reference the ServiceAccount without depending on the built object. - pub(crate) fn rbac_service_account_name(&self) -> ServiceAccountName { - RbacResourceNames { + /// 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(), } - .service_account_name() } - /// Recommended labels for a role-group resource, using the given product version. - /// - /// Used for PVC templates that cannot be modified once deployed: passing a constant version - /// (e.g. `none`) keeps those labels stable across product version upgrades. - pub(crate) fn recommended_labels_for( + pub fn recommended_labels(&self, role_group_name: &RoleGroupName) -> Labels { + self.recommended_labels_for(&ZookeeperRole::Server.into(), role_group_name) + } + + pub fn recommended_labels_for( + &self, + role_name: &RoleName, + role_group_name: &RoleGroupName, + ) -> Labels { + self.recommended_labels_with(&self.product_version, role_name, role_group_name) + } + + pub fn unversioned_recommended_labels(&self, role_group_name: &RoleGroupName) -> Labels { + self.recommended_labels_with( + &UNVERSIONED_PRODUCT_VERSION, + &ZookeeperRole::Server.into(), + role_group_name, + ) + } + + fn recommended_labels_with( &self, product_version: &ProductVersion, + role_name: &RoleName, role_group_name: &RoleGroupName, ) -> Labels { recommended_labels( @@ -339,38 +338,19 @@ impl ValidatedCluster { product_version, &operator_name(), &controller_name(), - &Self::role_name(), + role_name, role_group_name, ) } - /// Recommended labels for a role-group resource. - pub fn recommended_labels(&self, role_group_name: &RoleGroupName) -> Labels { - self.recommended_labels_for(&self.product_version, role_group_name) - } - /// Selector labels matching the pods of a role group. pub fn role_group_selector(&self, role_group_name: &RoleGroupName) -> Labels { - role_group_selector(self, &product_name(), &Self::role_name(), role_group_name) - } - - /// Returns an [`ObjectMetaBuilder`] pre-filled with the namespace, an owner reference back to - /// this cluster, and the recommended labels for a resource named `name` in `role_group_name`. - /// - /// Consolidates the metadata chain repeated by the child-resource builders. Call sites that - /// need extra labels/annotations chain them onto the returned builder. - pub(crate) fn object_meta( - &self, - name: impl Into, - role_group_name: &RoleGroupName, - ) -> ObjectMetaBuilder { - let mut builder = ObjectMetaBuilder::new(); - builder - .name_and_namespace(self) - .name(name) - .ownerreference(ownerreference_from_resource(self, None, Some(true))) - .with_labels(self.recommended_labels(role_group_name)); - builder + role_group_selector( + self, + &product_name(), + &ZookeeperRole::Server.into(), + role_group_name, + ) } } @@ -485,9 +465,7 @@ pub fn validate( let mut role_group_configs = BTreeMap::new(); for zk_role in ZookeeperRole::iter() { - let role = zk.role(&zk_role).with_context(|_| MissingRoleSnafu { - role: zk_role.to_string(), - })?; + let role = zk.role(&zk_role); let default_config = ZookeeperConfig::default_server_config(&zk.name_any(), &zk_role); let mut groups = BTreeMap::new(); @@ -521,14 +499,14 @@ pub fn validate( let listener_class = zk .role(&ZookeeperRole::Server) - .map(|role| role.role_config.listener_class.clone()) - .unwrap_or_else(|_| default_listener_class()); + .role_config + .listener_class + .clone(); - let role_config = zk.role_config(&ZookeeperRole::Server).map( - |ZookeeperServerRoleConfig { common, .. }| ValidatedRoleConfig { - pdb: common.pod_disruption_budget.clone(), - }, - ); + let ZookeeperServerRoleConfig { common, .. } = zk.role_config(&ZookeeperRole::Server); + let role_config = ValidatedRoleConfig { + pdb: common.pod_disruption_budget.clone(), + }; Ok(ValidatedCluster::new( name, @@ -599,7 +577,78 @@ mod tests { use stackable_operator::k8s_openapi::apimachinery::pkg::api::resource::Quantity; use super::*; - use crate::zk_controller::test_support::{minimal_zk, try_validate, validated_cluster}; + use crate::zk_controller::test_support::{ + app_version_label, minimal_zk, try_validate, validated_cluster, + }; + + /// Locks every value the validate step itself derives from the minimal fixture — so a + /// validation regression fails here, with a validate-shaped message, instead of surfacing as + /// a confusing build-test failure downstream. + /// + /// The merged per-role-group config (resources, affinity, logging defaults, …) is produced by + /// the config merge machinery, whose contracts are tested in operator-rs; only the values + /// this module derives on top are re-asserted here. + #[test] + fn validate_ok_derives_expected_values() { + let zookeeper = minimal_zk( + r#" + apiVersion: zookeeper.stackable.tech/v1alpha1 + kind: ZookeeperCluster + metadata: + name: simple-zookeeper + spec: + image: + productVersion: "3.9.5" + servers: + roleGroups: + default: + replicas: 3 + "#, + ); + let validated = validated_cluster(&zookeeper); + + assert_eq!(validated.name.to_string(), "simple-zookeeper"); + assert_eq!(validated.namespace.to_string(), "default"); + assert_eq!( + validated.uid.to_string(), + "c27b3971-ca72-42c1-80a4-abdfc1db0ddd" + ); + assert_eq!( + validated.image.image, + format!("oci.example.org/zookeeper:{}", app_version_label("3.9.5")) + ); + assert_eq!(validated.image.product_version, "3.9.5"); + assert_eq!( + validated.product_version.to_string(), + app_version_label("3.9.5") + ); + + // TLS towards clients is enabled by default (the secure client port), and the listener + // class falls back to its default. + let cluster_config = &validated.cluster_config; + assert!(cluster_config.zookeeper_security.tls_enabled()); + assert_eq!( + cluster_config.listener_class.to_string(), + "cluster-internal" + ); + + // The role config falls back to its defaults: PDBs enabled. + assert!(validated.role_config.pdb.enabled); + assert_eq!(validated.role_config.pdb.max_unavailable, None); + + // The single `server` role with the single `default` role group; the Vector agent is off. + assert_eq!(validated.role_group_configs.len(), 1); + let role_groups = &validated.role_group_configs[&ZookeeperRole::Server]; + let role_group_names: Vec = role_groups.keys().map(ToString::to_string).collect(); + assert_eq!(role_group_names, ["default"]); + let role_group = role_groups + .values() + .next() + .expect("the default role group exists"); + assert_eq!(role_group.replicas, Some(3)); + assert!(!role_group.config.logging.enable_vector_agent); + assert_eq!(role_group.config.logging.vector_container, None); + } #[test] fn enabling_vector_without_aggregator_name_fails_validation() { @@ -739,4 +788,14 @@ mod tests { Some(Quantity("3Gi".to_owned())) ); } + + /// Locks the invariant behind the `expect` in the `From for RoleName` impls: + /// every `ZookeeperRole` variant (present and future) must serialise to a valid `RoleName`. + #[test] + fn every_zookeeper_role_serialises_to_a_valid_role_name() { + for role in ZookeeperRole::iter() { + let _: RoleName = (&role).into(); + let _: RoleName = role.into(); + } + } }