From 61927cb7501f7dfb60007ce944c7665d23627091 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 14:58:25 +0100 Subject: [PATCH 01/13] re-emit existing config map if listener addresses are not ready --- rust/operator-binary/src/discovery.rs | 239 ++++++++++++------ .../operator-binary/src/listener_addresses.rs | 3 - rust/operator-binary/src/zk_controller.rs | 10 +- .../src/zk_controller/apply.rs | 5 +- .../src/zk_controller/build.rs | 16 +- .../src/zk_controller/dereference.rs | 20 ++ .../src/zk_controller/update_status.rs | 35 ++- .../src/zk_controller/validate.rs | 108 +++++--- 8 files changed, 300 insertions(+), 136 deletions(-) diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index 9a6a87a5..5caa0153 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -7,7 +7,7 @@ use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::{configmap::ConfigMapBuilder, meta::ObjectMetaBuilder}, k8s_openapi::api::core::v1::ConfigMap, - kube::Resource, + kube::{Resource, api::ObjectMeta}, kvp::{Label, Labels}, v2::{ HasName, HasUid, NameIsValidLabelValue, builder::meta::ownerreference_from_resource, @@ -35,33 +35,56 @@ pub enum Error { }, } -/// Build the discovery [`ConfigMap`] for the cluster controller from the -/// [`ValidatedCluster`]. +/// Build the discovery [`ConfigMap`] for the cluster controller from the [`ValidatedCluster`]. /// -/// The ConfigMap is owned by, and placed in the namespace of, the cluster. The image and security -/// settings are taken from the [`ValidatedCluster`] rather than being passed in separately. -/// -/// The connection details are read from the addresses published by the role Listener (carried on -/// [`ValidatedCluster::discovery_addresses`](ValidatedCluster#structfield.discovery_addresses), -/// fetched in the dereference step), which only the listener operator writes. While no address -/// exists around the first reconciliations, which create the Listener in the first place, the -/// ConfigMap is still written, with an empty `ZOOKEEPER` value: omitting it instead would let the -/// apply step delete an existing discovery ConfigMap as an orphan, breaking consumers that mount -/// it. The Listener watch triggers a new run that fills in the value once the addresses are -/// published. +/// The connection details come from the addresses published by the role Listener +/// ([`ValidatedCluster::discovery_addresses`]). While there is no usable address (around the +/// first reconciliation, after the Listener was re-created, or while its status is stale), no +/// fresh ConfigMap can be built. The stored one is then re-emitted unchanged, so that it stays +/// tracked by the apply step (an untracked ConfigMap would be deleted as an orphan) without +/// changing its content: an unchanged ConfigMap gets no new resource version, so consumers are +/// not restarted. `None` is returned only before a discovery ConfigMap has ever been stored. +/// The Listener watch triggers a new run once addresses are published. pub fn build_discovery_configmap( validated_cluster: &ValidatedCluster, zk_role: &ZookeeperRole, - listener_addresses: &ListenerAddresses, -) -> Result { - build_discovery_configmap_for_owner( - validated_cluster, - &validated_cluster.namespace, - recommended_labels_for_role_resources(validated_cluster, zk_role), - listener_addresses, - None, - &validated_cluster.cluster_config.zookeeper_security, - ) +) -> Result> { + let labels = recommended_labels_for_role_resources(validated_cluster, zk_role); + + match ( + &validated_cluster.discovery_addresses, + &validated_cluster.existing_discovery_config_map, + ) { + (Some(addresses), _) => build_discovery_configmap_for_owner( + validated_cluster, + &validated_cluster.namespace, + labels, + addresses, + None, + &validated_cluster.cluster_config.zookeeper_security, + ) + .map(Some), + (None, Some(existing)) => { + tracing::debug!( + "the role Listener has no usable address, re-emitting the stored discovery ConfigMap" + ); + Ok(Some(ConfigMap { + metadata: discovery_config_map_metadata( + validated_cluster, + &validated_cluster.namespace, + labels, + ), + data: existing.data.clone(), + ..ConfigMap::default() + })) + } + (None, None) => { + tracing::debug!( + "the role Listener has no usable address and no discovery ConfigMap is stored yet, skipping it" + ); + Ok(None) + } + } } /// Build the discovery [`ConfigMap`] for the znode controller. @@ -122,8 +145,6 @@ fn build_discovery_configmap_for_owner( chroot: Option<&str>, zookeeper_security: &ZookeeperSecurity, ) -> Result { - let name = owner.to_name(); - // Write a connection string of the format that Java ZooKeeper client expects: // "{host1}:{port1},{host2:port2},.../{chroot}" // See https://zookeeper.apache.org/doc/current/apidocs/zookeeper-server/org/apache/zookeeper/ZooKeeper.html#ZooKeeper-java.lang.String-int-org.apache.zookeeper.Watcher- @@ -136,14 +157,7 @@ fn build_discovery_configmap_for_owner( conn_str.push_str(chroot); } ConfigMapBuilder::new() - .metadata( - ObjectMetaBuilder::new() - .name(name) - .namespace(namespace) - .ownerreference(ownerreference_from_resource(owner, None, Some(true))) - .with_labels(labels) - .build(), - ) + .metadata(discovery_config_map_metadata(owner, namespace, labels)) .add_data("ZOOKEEPER", conn_str) // Some clients don't support ZooKeeper's merged `hosts/chroot` format, so export them separately for these clients .add_data("ZOOKEEPER_HOSTS", listener_addresses) @@ -156,17 +170,31 @@ fn build_discovery_configmap_for_owner( .context(BuildConfigMapSnafu) } +/// The metadata of a discovery [`ConfigMap`], shared by freshly built and re-emitted ConfigMaps +/// so that the two cannot drift apart. +fn discovery_config_map_metadata( + owner: &(impl Resource + HasName + HasUid + NameIsValidLabelValue), + namespace: impl Into, + labels: Labels, +) -> ObjectMeta { + ObjectMetaBuilder::new() + .name(owner.to_name()) + .namespace(namespace) + .ownerreference(ownerreference_from_resource(owner, None, Some(true))) + .with_labels(labels) + .build() +} + #[cfg(test)] mod tests { use std::collections::BTreeMap; + use stackable_operator::crd::listener; + use super::*; use crate::{ crd::ZOOKEEPER_SERVER_PORT_NAME, - listener_addresses::{ - listener_addresses, - test_support::{ingress_address, role_listener}, - }, + listener_addresses::test_support::{ingress_address, role_listener}, zk_controller::test_support::{ app_version_label, minimal_zk, try_validate_with_role_listener, }, @@ -211,33 +239,56 @@ mod tests { .clone() } - /// Addresses published under the ZooKeeper server port name, built through the real reader so - /// the fixtures cannot drift from what the validate step produces. - fn published_addresses(addresses: &[(&str, u16)]) -> ListenerAddresses { - let listener = role_listener(Some( + /// A role Listener publishing the given addresses under the ZooKeeper server port name. + fn listener_with(addresses: &[(&str, u16)]) -> listener::v1alpha1::Listener { + role_listener(Some( addresses .iter() .map(|(address, port)| { ingress_address(address, ZOOKEEPER_SERVER_PORT_NAME, i32::from(*port)) }) .collect(), - )); + )) + } - listener_addresses(&listener, ZOOKEEPER_SERVER_PORT_NAME) - .expect("the fixture publishes addresses under the server port name") - .expect("the fixture publishes addresses") + /// A discovery ConfigMap as the API server returns it: with server-populated metadata. + fn stored_config_map(zookeeper: &str) -> ConfigMap { + ConfigMap { + metadata: ObjectMeta { + name: Some("simple-zookeeper".to_owned()), + namespace: Some("default".to_owned()), + resource_version: Some("42".to_owned()), + uid: Some("0b5c7b8e-0000-0000-0000-000000000000".to_owned()), + managed_fields: Some(vec![]), + ..ObjectMeta::default() + }, + data: Some( + [ + ("ZOOKEEPER".to_owned(), zookeeper.to_owned()), + ("ZOOKEEPER_HOSTS".to_owned(), zookeeper.to_owned()), + ("ZOOKEEPER_CLIENT_PORT".to_owned(), "2282".to_owned()), + ("ZOOKEEPER_CHROOT".to_owned(), "/".to_owned()), + ] + .into(), + ), + ..ConfigMap::default() + } } /// The cluster controller advertises the whole ensemble, rooted at `/`. The fixture keeps TLS /// enabled, so the client port is the secure one. #[test] fn cluster_discovery_config_map_advertises_the_published_addresses() { - let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None) - .expect("validate should succeed for the test fixture"); - let addresses = published_addresses(&[("node-0", 2282), ("node-1", 2282)]); + let cluster = try_validate_with_role_listener( + &minimal_zk(ZK_YAML), + Some(listener_with(&[("node-0", 2282), ("node-1", 2282)])), + None, + ) + .expect("validate should succeed for the test fixture"); - let config_map = - build_discovery_configmap(&cluster, &ZookeeperRole::Server, &addresses).expect("build"); + let config_map = build_discovery_configmap(&cluster, &ZookeeperRole::Server) + .expect("build") + .expect("a fresh ConfigMap is built when addresses exist"); assert_eq!( config_map.metadata.name.as_deref(), @@ -253,32 +304,6 @@ mod tests { assert_eq!(data(&config_map, "ZOOKEEPER_CHROOT"), "/"); } - /// While the role Listener publishes no addresses the ConfigMap is still written, with an - /// empty connection string, so that the apply step does not delete the published one as an - /// orphan. - #[test] - fn cluster_discovery_config_map_without_addresses_is_still_written() { - let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None) - .expect("validate should succeed for the test fixture"); - - let config_map = build_discovery_configmap( - &cluster, - &ZookeeperRole::Server, - &ListenerAddresses::default(), - ) - .expect("build"); - - assert_eq!( - config_map.metadata.name.as_deref(), - Some("simple-zookeeper") - ); - assert_eq!(data(&config_map, "ZOOKEEPER"), ""); - assert_eq!(data(&config_map, "ZOOKEEPER_HOSTS"), ""); - // The port and chroot do not depend on the addresses, so they stay populated. - assert_eq!(data(&config_map, "ZOOKEEPER_CLIENT_PORT"), "2282"); - assert_eq!(data(&config_map, "ZOOKEEPER_CHROOT"), "/"); - } - /// The znode controller advertises the same ensemble, narrowed to the znode's chroot. Only /// `ZOOKEEPER` carries the chroot, because some clients cannot parse the merged format. #[test] @@ -343,4 +368,64 @@ mod tests { ); assert_eq!(config_map.metadata.labels, Some(expected_labels)); } + + /// Before the first successful build there is nothing to re-emit, so no ConfigMap is built. + #[test] + fn skips_cluster_discovery_config_map_without_addresses_and_without_stored_one() { + let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None, None) + .expect("validate should succeed for the test fixture"); + + let config_map = + build_discovery_configmap(&cluster, &ZookeeperRole::Server).expect("build"); + + assert!(config_map.is_none()); + } + + /// While the Listener has no usable address, the stored ConfigMap is re-emitted unchanged, with + /// fresh metadata that carries no server-populated fields. + #[test] + fn reemits_stored_cluster_discovery_config_map_without_addresses() { + let stored = stored_config_map("node-0:2282"); + let cluster = + try_validate_with_role_listener(&minimal_zk(ZK_YAML), None, Some(stored.clone())) + .expect("validate should succeed for the test fixture"); + + let config_map = build_discovery_configmap(&cluster, &ZookeeperRole::Server) + .expect("build") + .expect("the stored ConfigMap is re-emitted"); + + assert_eq!(config_map.data, stored.data); + assert_eq!(config_map.metadata.resource_version, None); + assert_eq!(config_map.metadata.uid, None); + assert_eq!(config_map.metadata.managed_fields, None); + + // Same metadata as a freshly built ConfigMap for this cluster. + let fresh_cluster = try_validate_with_role_listener( + &minimal_zk(ZK_YAML), + Some(listener_with(&[("node-0", 2282)])), + None, + ) + .expect("validate should succeed for the test fixture"); + let fresh = build_discovery_configmap(&fresh_cluster, &ZookeeperRole::Server) + .expect("build") + .expect("fresh"); + assert_eq!(config_map.metadata, fresh.metadata); + } + + /// Once addresses exist again, they replace whatever was stored. + #[test] + fn fresh_addresses_replace_stale_stored_data() { + let cluster = try_validate_with_role_listener( + &minimal_zk(ZK_YAML), + Some(listener_with(&[("node-0", 2282)])), + Some(stored_config_map("old:2282")), + ) + .expect("validate should succeed for the test fixture"); + + let config_map = build_discovery_configmap(&cluster, &ZookeeperRole::Server) + .expect("build") + .expect("fresh"); + + assert_eq!(data(&config_map, "ZOOKEEPER"), "node-0:2282"); + } } diff --git a/rust/operator-binary/src/listener_addresses.rs b/rust/operator-binary/src/listener_addresses.rs index fff6c18b..b409677e 100644 --- a/rust/operator-binary/src/listener_addresses.rs +++ b/rust/operator-binary/src/listener_addresses.rs @@ -30,9 +30,6 @@ type Result = std::result::Result; /// /// An address is a hostname or IP address of a node, a cluster IP or an external load balancer, /// depending on the Service type behind the Listener. -/// -/// The [`Default`] value is empty, which renders as an empty connection string, see -/// [`build_discovery_configmap`](crate::discovery::build_discovery_configmap). #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct ListenerAddresses(BTreeSet<(String, u16)>); diff --git a/rust/operator-binary/src/zk_controller.rs b/rust/operator-binary/src/zk_controller.rs index aeafa54f..db95c9f5 100644 --- a/rust/operator-binary/src/zk_controller.rs +++ b/rust/operator-binary/src/zk_controller.rs @@ -115,8 +115,8 @@ pub struct KubernetesResources { pub listeners: Vec, pub config_maps: Vec, /// The discovery `ConfigMap`, kept apart from the role group `config_maps` because the cluster - /// status carries a hash of it. - pub discovery_config_map: ConfigMap, + /// status carries a hash of it. `None` before it can be built for the first time. + pub discovery_config_map: Option, pub pod_disruption_budgets: Vec, pub service_accounts: Vec, pub role_bindings: Vec, @@ -190,7 +190,7 @@ pub fn error_policy( pub(crate) mod test_support { use stackable_operator::{ cli::OperatorEnvironmentOptions, commons::networking::DomainName, crd::listener, - utils::cluster_info::KubernetesClusterInfo, + k8s_openapi::api::core::v1::ConfigMap, utils::cluster_info::KubernetesClusterInfo, }; use crate::{ @@ -248,7 +248,7 @@ pub(crate) mod test_support { pub fn try_validate( zk: &v1alpha1::ZookeeperCluster, ) -> Result { - try_validate_with_role_listener(zk, None) + try_validate_with_role_listener(zk, None, None) } /// Runs the real validate step against a minimal (auth-free) fixture and the given role @@ -256,12 +256,14 @@ pub(crate) mod test_support { pub fn try_validate_with_role_listener( zk: &v1alpha1::ZookeeperCluster, maybe_role_listener: Option, + existing_discovery_config_map: Option, ) -> Result { validate( zk, &DereferencedObjects { authentication_classes: DereferencedAuthenticationClasses::new_for_tests(), maybe_role_listener, + existing_discovery_config_map, }, &operator_environment(), ) diff --git a/rust/operator-binary/src/zk_controller/apply.rs b/rust/operator-binary/src/zk_controller/apply.rs index 07fb97bc..57a35bfc 100644 --- a/rust/operator-binary/src/zk_controller/apply.rs +++ b/rust/operator-binary/src/zk_controller/apply.rs @@ -95,7 +95,10 @@ impl<'a> Applier<'a> { let services = self.add_resources(services).await?; let listeners = self.add_resources(listeners).await?; let config_maps = self.add_resources(config_maps).await?; - let discovery_config_map = self.add_resource(discovery_config_map).await?; + let discovery_config_map = match discovery_config_map { + Some(config_map) => Some(self.add_resource(config_map).await?), + None => None, + }; let pod_disruption_budgets = self.add_resources(pod_disruption_budgets).await?; let stateful_sets = self.add_resources(stateful_sets).await?; diff --git a/rust/operator-binary/src/zk_controller/build.rs b/rust/operator-binary/src/zk_controller/build.rs index 1f8b837c..6745ce7a 100644 --- a/rust/operator-binary/src/zk_controller/build.rs +++ b/rust/operator-binary/src/zk_controller/build.rs @@ -81,8 +81,8 @@ pub enum Error { /// [`Listener`](stackable_operator::crd::listener::v1alpha1::Listener) that were dereferenced and /// validated into /// [`ValidatedCluster::discovery_addresses`](ValidatedCluster#structfield.discovery_addresses); see -/// [`build_discovery_configmap`](discovery::build_discovery_configmap) for how its content depends -/// on them. +/// [`build_discovery_configmap`](discovery::build_discovery_configmap) for when it is built fresh, +/// re-emitted or skipped. pub fn build( cluster: &ValidatedCluster, cluster_info: &KubernetesClusterInfo, @@ -139,8 +139,7 @@ pub fn build( let listeners = vec![build_role_listener(cluster, &zk_role)]; let discovery_config_map = - discovery::build_discovery_configmap(cluster, &zk_role, &cluster.discovery_addresses) - .context(DiscoveryConfigMapSnafu)?; + discovery::build_discovery_configmap(cluster, &zk_role).context(DiscoveryConfigMapSnafu)?; Ok(KubernetesResources { stateful_sets, @@ -321,12 +320,9 @@ mod tests { "simple-zookeeper-server-secondary", ] ); - // The discovery ConfigMap is named after the cluster, and written even though the fixture - // has no role Listener publishing addresses yet (see `build()`). - assert_eq!( - resources.discovery_config_map.meta().name.as_deref(), - Some("simple-zookeeper") - ); + // The fixture's role Listener does not exist and no discovery + // ConfigMap is stored, so none is built yet. + assert!(resources.discovery_config_map.is_none()); // The single role-level Listener for the one ZooKeeper role (`server`). assert_eq!( sorted_names(&resources.listeners), diff --git a/rust/operator-binary/src/zk_controller/dereference.rs b/rust/operator-binary/src/zk_controller/dereference.rs index c4384a90..de7d44ea 100644 --- a/rust/operator-binary/src/zk_controller/dereference.rs +++ b/rust/operator-binary/src/zk_controller/dereference.rs @@ -9,6 +9,7 @@ use snafu::{ResultExt, Snafu}; use stackable_operator::{ client::Client, crd::listener, + k8s_openapi::api::core::v1::ConfigMap, v2::{ controller_utils::{get_cluster_name, get_namespace}, types::{kubernetes::NamespaceName, operator::ClusterName}, @@ -40,6 +41,12 @@ pub enum Error { FetchRoleListener { source: stackable_operator::client::Error, }, + + #[snafu(display("failed to fetch the discovery ConfigMap {config_map}"))] + FetchExistingDiscoveryConfigMap { + source: stackable_operator::client::Error, + config_map: String, + }, } type Result = std::result::Result; @@ -55,6 +62,10 @@ pub struct DereferencedObjects { /// this object, so it can only be built once the Listener exists and carries them. The /// controller watches Listeners, so a reconciliation is triggered as soon as that happens. pub maybe_role_listener: Option, + + /// The discovery ConfigMap as stored by an earlier reconciliation, or `None` before the first + /// successful build. The build step re-emits it while the role Listener has no usable address. + pub existing_discovery_config_map: Option, } /// Fetches all Kubernetes objects referenced from the [`v1alpha1::ZookeeperCluster`] spec. @@ -74,9 +85,18 @@ pub async fn dereference( let maybe_role_listener = fetch_role_listener(client, &cluster_name, &namespace).await?; + let discovery_config_map_name = cluster_name.as_ref(); + let existing_discovery_config_map = client + .get_opt::(discovery_config_map_name, namespace.as_ref()) + .await + .context(FetchExistingDiscoveryConfigMapSnafu { + config_map: discovery_config_map_name, + })?; + Ok(DereferencedObjects { authentication_classes, maybe_role_listener, + existing_discovery_config_map, }) } diff --git a/rust/operator-binary/src/zk_controller/update_status.rs b/rust/operator-binary/src/zk_controller/update_status.rs index 491a85cf..4380bdca 100644 --- a/rust/operator-binary/src/zk_controller/update_status.rs +++ b/rust/operator-binary/src/zk_controller/update_status.rs @@ -50,7 +50,7 @@ pub async fn update_status( ClusterOperationsConditionBuilder::new(&zk.spec.cluster_operation); let status = v1alpha1::ZookeeperClusterStatus { - discovery_hash: Some(discovery_hash(&applied.discovery_config_map)), + discovery_hash: Some(discovery_hash(applied.discovery_config_map.as_ref())), conditions: compute_conditions( zk, &[ @@ -71,14 +71,15 @@ pub async fn update_status( /// Hashes the resource version of the applied discovery ConfigMap, so that clients can tell when /// the published connection details changed. /// -/// A ConfigMap that was never applied carries no resource version, in which case the hash covers -/// nothing. -fn discovery_hash(discovery_config_map: &ConfigMap) -> String { +/// No ConfigMap (not built yet) or one without a resource version hashes nothing. +fn discovery_hash(discovery_config_map: Option<&ConfigMap>) -> String { // std's SipHasher is deprecated, and DefaultHasher is unstable across Rust releases. // We don't /need/ stability, but it's still nice to avoid spurious changes where possible. let mut discovery_hash = FnvHasher::with_key(0); - if let Some(resource_version) = &discovery_config_map.metadata.resource_version { + if let Some(resource_version) = + discovery_config_map.and_then(|config_map| config_map.metadata.resource_version.as_ref()) + { discovery_hash.write(resource_version.as_bytes()) } @@ -108,13 +109,29 @@ mod tests { /// it must follow the discovery ConfigMap's resource version. #[test] fn discovery_hash_tracks_the_resource_version() { - let hash = discovery_hash(&discovery_config_map(Some("42"))); + let hash = discovery_hash(Some(&discovery_config_map(Some("42")))); - assert_ne!(hash, discovery_hash(&discovery_config_map(Some("43")))); - assert_eq!(hash, discovery_hash(&discovery_config_map(Some("42")))); + assert_ne!( + hash, + discovery_hash(Some(&discovery_config_map(Some("43")))) + ); + assert_eq!( + hash, + discovery_hash(Some(&discovery_config_map(Some("42")))) + ); // A ConfigMap that was never applied carries no resource version, in which case the hash // covers nothing and stays at the hasher's initial state. - assert_ne!(hash, discovery_hash(&discovery_config_map(None))); + assert_ne!(hash, discovery_hash(Some(&discovery_config_map(None)))); + } + + /// While no discovery ConfigMap has been built yet, the hash is the same as for one that was + /// never applied. + #[test] + fn discovery_hash_without_config_map_is_the_initial_hash() { + assert_eq!( + discovery_hash(None), + discovery_hash(Some(&discovery_config_map(None))) + ); } } diff --git a/rust/operator-binary/src/zk_controller/validate.rs b/rust/operator-binary/src/zk_controller/validate.rs index 61ccc2d7..d2f0df34 100644 --- a/rust/operator-binary/src/zk_controller/validate.rs +++ b/rust/operator-binary/src/zk_controller/validate.rs @@ -21,7 +21,7 @@ use stackable_operator::{ }, config::fragment, deep_merger::ObjectOverrides, - k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta, + k8s_openapi::{api::core::v1::ConfigMap, apimachinery::pkg::apis::meta::v1::ObjectMeta}, kube::Resource, product_logging::spec::Logging, shared::time::Duration, @@ -235,10 +235,12 @@ pub struct ValidatedCluster { /// The client addresses published by the role Listener, which the discovery ConfigMap /// advertises. /// - /// Empty until the listener operator has published them, in which case the discovery ConfigMap - /// advertises an empty connection string until the reconciliation that the Listener watch - /// triggers fills it in. - pub discovery_addresses: ListenerAddresses, + /// `None` while there is no usable address: the Listener does not exist yet, has no status + /// yet, or publishes no address for the ZooKeeper server port. The build step then re-emits + /// [`Self::existing_discovery_config_map`] unchanged. + pub discovery_addresses: Option, + /// The discovery ConfigMap as stored by an earlier reconciliation, if any. + pub existing_discovery_config_map: Option, } impl ValidatedCluster { @@ -257,7 +259,8 @@ impl ValidatedCluster { >, cluster_operation: ClusterOperation, object_overrides: ObjectOverrides, - discovery_addresses: ListenerAddresses, + discovery_addresses: Option, + existing_discovery_config_map: Option, ) -> Self { Self { metadata: ObjectMeta { @@ -277,6 +280,7 @@ impl ValidatedCluster { cluster_operation, object_overrides, discovery_addresses, + existing_discovery_config_map, } } @@ -438,18 +442,23 @@ pub fn validate( pdb: common.pod_disruption_budget.clone(), }; - // The role Listener does not exist during the very first reconciliation, and carries no - // addresses until the listener operator has published them. Both cases resolve to no addresses - // rather than an error, so that the discovery ConfigMap is still built (see - // [`build_discovery_configmap`](crate::discovery::build_discovery_configmap)). - let discovery_addresses = dereferenced_objects - .maybe_role_listener - .as_ref() - .map(|listener| listener_addresses(listener, ZOOKEEPER_SERVER_PORT_NAME)) - .transpose() - .context(ReadRoleListenerAddressesSnafu)? - .flatten() - .unwrap_or_default(); + // No usable address yet means `None`, and the build step then re-emits the stored discovery + // ConfigMap unchanged (see + // [`build_discovery_configmap`](crate::discovery::build_discovery_configmap)): + // - the role Listener does not exist (first reconciliation, or it was deleted), + // - it has no status yet (`Ok(None)`), + // - it publishes no address for the server port: an empty address list, or a stale status + // whose port was renamed (`PortNotFound`). Failing here would abort the run before the apply + // step, so the Listener spec carrying the right port would never be applied. + // Any other error (an invalid port number) still fails the run. + let discovery_addresses = match &dereferenced_objects.maybe_role_listener { + None => None, + Some(listener) => match listener_addresses(listener, ZOOKEEPER_SERVER_PORT_NAME) { + Ok(addresses) => addresses, + Err(listener_addresses::Error::PortNotFound { .. }) => None, + Err(source) => return Err(source).context(ReadRoleListenerAddressesSnafu), + }, + }; Ok(ValidatedCluster::new( name, @@ -466,6 +475,7 @@ pub fn validate( zk.spec.cluster_operation.clone(), zk.spec.object_overrides.clone(), discovery_addresses, + dereferenced_objects.existing_discovery_config_map.clone(), )) } @@ -742,10 +752,11 @@ mod tests { ); } - /// The `zk` port is a constant, so a role Listener that publishes addresses without it is a - /// fault rather than a transient state, and must fail validation. + /// A Listener whose status does not (yet) carry the server port, e.g. a stale status after a + /// port rename, is treated like a Listener without addresses: the stored discovery ConfigMap + /// is re-emitted rather than failing the run before the apply step. #[test] - fn role_listener_without_the_expected_port_fails_validation() { + fn role_listener_without_the_expected_port_validates_to_no_addresses() { let zk = minimal_zk(MINIMAL_ZK_YAML); let listener = role_listener(Some(vec![ingress_address( "node-0", @@ -753,10 +764,10 @@ mod tests { 2181, )])); - assert!(matches!( - try_validate_with_role_listener(&zk, Some(listener)), - Err(Error::ReadRoleListenerAddresses { .. }) - )); + let validated = try_validate_with_role_listener(&zk, Some(listener), None) + .expect("validate should succeed for the test fixture"); + + assert_eq!(validated.discovery_addresses, None); } /// The published addresses reach the discovery ConfigMap through `discovery_addresses`. @@ -768,28 +779,61 @@ mod tests { ingress_address("node-0", ZOOKEEPER_SERVER_PORT_NAME, 2181), ])); - let validated = try_validate_with_role_listener(&zk, Some(listener)) + let validated = try_validate_with_role_listener(&zk, Some(listener), None) .expect("validate should succeed for the test fixture"); assert_eq!( - validated.discovery_addresses.to_connection_string(), + validated + .discovery_addresses + .expect("valid addresses") + .to_connection_string(), "node-0:2181,node-1:2181" ); } - /// Unlike the znode controller, missing addresses are not an error here: the reconciliation - /// that creates the role Listener in the first place has to get as far as the apply step. + /// Unlike the znode controller, missing addresses are not an error here and will + /// result in the existing ConfigMap being re-emitted: the reconciliation that + /// creates the role Listener in the first place has to get as far as the apply + /// step. #[test] fn missing_role_listener_addresses_validate_to_no_addresses() { let zk = minimal_zk(MINIMAL_ZK_YAML); // The Listener does not exist yet. let validated = validated_cluster(&zk); - assert_eq!(validated.discovery_addresses, ListenerAddresses::default()); + assert_eq!(validated.discovery_addresses, None); // The Listener exists, but the listener operator has not published its status yet. - let validated = try_validate_with_role_listener(&zk, Some(role_listener(None))) + let validated = try_validate_with_role_listener(&zk, Some(role_listener(None)), None) + .expect("validate should succeed for the test fixture"); + assert_eq!(validated.discovery_addresses, None); + } + + /// When no listener addresses are available the existing ConfigMap will be emitted. + #[test] + fn existing_discovery_config_map_is_carried_into_the_validated_cluster() { + let zk = minimal_zk(MINIMAL_ZK_YAML); + + let stored = ConfigMap { + data: Some([("ZOOKEEPER".to_owned(), "node-0:2181".to_owned())].into()), + ..ConfigMap::default() + }; + + let validated = try_validate_with_role_listener(&zk, None, Some(stored.clone())) .expect("validate should succeed for the test fixture"); - assert_eq!(validated.discovery_addresses, ListenerAddresses::default()); + + assert_eq!(validated.existing_discovery_config_map, Some(stored)); + } + + /// A Listener status with an empty address list is treated like no addresses. + #[test] + fn role_listener_with_empty_ingress_list_validates_to_no_addresses() { + let zk = minimal_zk(MINIMAL_ZK_YAML); + + let validated = + try_validate_with_role_listener(&zk, Some(role_listener(Some(vec![]))), None) + .expect("validate should succeed for the test fixture"); + + assert_eq!(validated.discovery_addresses, None); } } From cca1b5a1cde13fde8a42c7e63f65645f4b29cb36 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 16:17:28 +0100 Subject: [PATCH 02/13] drop unused Default --- rust/operator-binary/src/listener_addresses.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/operator-binary/src/listener_addresses.rs b/rust/operator-binary/src/listener_addresses.rs index b409677e..483a54c9 100644 --- a/rust/operator-binary/src/listener_addresses.rs +++ b/rust/operator-binary/src/listener_addresses.rs @@ -30,7 +30,7 @@ type Result = std::result::Result; /// /// An address is a hostname or IP address of a node, a cluster IP or an external load balancer, /// depending on the Service type behind the Listener. -#[derive(Clone, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct ListenerAddresses(BTreeSet<(String, u16)>); impl ListenerAddresses { From bf1cef88d83e4533d5b7b2044d1d547a0987fc93 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 16:25:36 +0100 Subject: [PATCH 03/13] add discovery_config_map_name() helper --- rust/operator-binary/src/discovery.rs | 46 +++++++++++++++++-- .../src/zk_controller/dereference.rs | 17 ++++--- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index 5caa0153..c86acc77 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -3,6 +3,8 @@ //! Shared by the build steps of both controllers: the ZookeeperCluster controller publishes the //! whole ensemble, the ZookeeperZnode controller publishes the same ensemble narrowed to a chroot. +use std::str::FromStr; + use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::{configmap::ConfigMapBuilder, meta::ObjectMetaBuilder}, @@ -10,8 +12,10 @@ use stackable_operator::{ kube::{Resource, api::ObjectMeta}, kvp::{Label, Labels}, v2::{ - HasName, HasUid, NameIsValidLabelValue, builder::meta::ownerreference_from_resource, + HasName, HasUid, NameIsValidLabelValue, + builder::meta::ownerreference_from_resource, kvp::label, + types::{kubernetes::ConfigMapName, operator::ClusterName}, }, }; @@ -35,6 +39,21 @@ pub enum Error { }, } +/// The name of the cluster controller's discovery [`ConfigMap`], which is the cluster name. +/// +/// Takes the bare cluster name (not [`ValidatedCluster`]) so that the dereference step, which runs +/// before validation, fetches the same ConfigMap that the build step writes. +pub fn discovery_config_map_name(cluster_name: &ClusterName) -> ConfigMapName { + const _: () = assert!( + ClusterName::MAX_LENGTH <= ConfigMapName::MAX_LENGTH, + "The string `` must not exceed the limit of ConfigMap names." + ); + let _ = ClusterName::IS_RFC_1123_SUBDOMAIN_NAME; + + ConfigMapName::from_str(cluster_name.as_ref()) + .expect("a valid cluster name is a valid ConfigMap name") +} + /// Build the discovery [`ConfigMap`] for the cluster controller from the [`ValidatedCluster`]. /// /// The connection details come from the addresses published by the role Listener @@ -113,8 +132,8 @@ pub fn build_znode_discovery_configmap( /// The znode controller's discovery ConfigMap cannot use the label functions from /// [`stackable_operator::v2::kvp::label`], because its `app.kubernetes.io/instance` value is the /// name of the owning [`ZookeeperZnode`](crate::crd::v1alpha1::ZookeeperZnode), not a -/// [`ClusterName`](stackable_operator::v2::types::operator::ClusterName). The label set matches -/// the role-level recommended labels of the cluster controller's discovery ConfigMap otherwise. +/// [`ClusterName`]. The label set matches the role-level recommended labels of the +/// cluster controller's discovery ConfigMap otherwise. fn znode_discovery_labels(validated_znode: &ValidatedZnode) -> Labels { Labels::from_iter([ Label::instance(&validated_znode.to_label_value()).expect( @@ -428,4 +447,25 @@ mod tests { assert_eq!(data(&config_map, "ZOOKEEPER"), "node-0:2282"); } + + /// The dereference step fetches the stored ConfigMap by [`discovery_config_map_name`], so it + /// must be the name the build step writes; otherwise nothing would ever be re-emitted. + #[test] + fn discovery_config_map_name_matches_the_built_config_map() { + let cluster = try_validate_with_role_listener( + &minimal_zk(ZK_YAML), + Some(listener_with(&[("node-0", 2282)])), + None, + ) + .expect("validate should succeed for the test fixture"); + + let config_map = build_discovery_configmap(&cluster, &ZookeeperRole::Server) + .expect("build") + .expect("fresh"); + + assert_eq!( + config_map.metadata.name.as_deref(), + Some(discovery_config_map_name(&cluster.name).as_ref()) + ); + } } diff --git a/rust/operator-binary/src/zk_controller/dereference.rs b/rust/operator-binary/src/zk_controller/dereference.rs index de7d44ea..7c5e0072 100644 --- a/rust/operator-binary/src/zk_controller/dereference.rs +++ b/rust/operator-binary/src/zk_controller/dereference.rs @@ -16,10 +16,13 @@ use stackable_operator::{ }, }; -use crate::crd::{ - ZookeeperRole, - authentication::{self, DereferencedAuthenticationClasses}, - role_listener_name, v1alpha1, +use crate::{ + crd::{ + ZookeeperRole, + authentication::{self, DereferencedAuthenticationClasses}, + role_listener_name, v1alpha1, + }, + discovery::discovery_config_map_name, }; #[derive(Snafu, Debug)] @@ -85,12 +88,12 @@ pub async fn dereference( let maybe_role_listener = fetch_role_listener(client, &cluster_name, &namespace).await?; - let discovery_config_map_name = cluster_name.as_ref(); + let discovery_config_map_name = discovery_config_map_name(&cluster_name); let existing_discovery_config_map = client - .get_opt::(discovery_config_map_name, namespace.as_ref()) + .get_opt::(discovery_config_map_name.as_ref(), namespace.as_ref()) .await .context(FetchExistingDiscoveryConfigMapSnafu { - config_map: discovery_config_map_name, + config_map: discovery_config_map_name.as_ref(), })?; Ok(DereferencedObjects { From 1bd20bd7f2c0f48f81725d7285748f34be776d9a Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 16:27:35 +0100 Subject: [PATCH 04/13] invalid port test --- .../src/zk_controller/validate.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/rust/operator-binary/src/zk_controller/validate.rs b/rust/operator-binary/src/zk_controller/validate.rs index d2f0df34..04d85fe7 100644 --- a/rust/operator-binary/src/zk_controller/validate.rs +++ b/rust/operator-binary/src/zk_controller/validate.rs @@ -836,4 +836,21 @@ mod tests { assert_eq!(validated.discovery_addresses, None); } + + /// A port number that does not fit into a u16 is a fault, unlike a missing port, and still + /// fails validation. + #[test] + fn role_listener_with_an_invalid_port_fails_validation() { + let zk = minimal_zk(MINIMAL_ZK_YAML); + let listener = role_listener(Some(vec![ingress_address( + "node-0", + ZOOKEEPER_SERVER_PORT_NAME, + 70_000, + )])); + + assert!(matches!( + try_validate_with_role_listener(&zk, Some(listener), None), + Err(Error::ReadRoleListenerAddresses { .. }) + )); + } } From cb7964d9657425d5c05fa2438e7e877096ff8c8d Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 16:37:25 +0100 Subject: [PATCH 05/13] warn on port-not-found --- .../operator-binary/src/listener_addresses.rs | 20 +++++++++++++++++-- .../src/zk_controller/validate.rs | 18 ++++++++++++----- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/rust/operator-binary/src/listener_addresses.rs b/rust/operator-binary/src/listener_addresses.rs index 483a54c9..54e3606e 100644 --- a/rust/operator-binary/src/listener_addresses.rs +++ b/rust/operator-binary/src/listener_addresses.rs @@ -47,8 +47,9 @@ impl ListenerAddresses { /// Reads the addresses that `listener` publishes for `port_name`. /// -/// Returns `Ok(None)` while the Listener carries no ingress addresses at all, which is the normal -/// state between creating the Listener and the listener operator publishing its addresses. A +/// Returns `Ok(None)` while the Listener carries no ingress addresses at all +/// (no status yet, or an empty address list), which is the normal state between +/// creating the Listener and the listener operator publishing its addresses. A /// Listener that does publish addresses, but none for `port_name`, is an error. // TODO (@NickLarsenNZ): Move this to stackable-operator, so it can be used as // listener.addresses_for_port(port_name) @@ -64,6 +65,12 @@ pub fn listener_addresses( return Ok(None); }; + // The listener operator publishes an empty list until it knows an address, e.g. before the + // Pods behind a NodePort Listener are scheduled. That is "not ready yet", not a missing port. + if ingress_addresses.is_empty() { + return Ok(None); + } + let address_port_pairs = ingress_addresses .iter() // Filter the addresses that have the port we are interested in (they likely all have it though) @@ -160,6 +167,15 @@ mod tests { ); } + #[test] + fn listener_addresses_with_empty_ingress_list_is_not_ready_yet() { + assert_eq!( + listener_addresses(&role_listener(Some(vec![])), ZOOKEEPER_SERVER_PORT_NAME) + .expect("addresses"), + None + ); + } + #[test] fn listener_addresses_missing_port_name_is_error() { let listener = role_listener(Some(vec![ingress(2181)])); diff --git a/rust/operator-binary/src/zk_controller/validate.rs b/rust/operator-binary/src/zk_controller/validate.rs index 04d85fe7..1ce723ce 100644 --- a/rust/operator-binary/src/zk_controller/validate.rs +++ b/rust/operator-binary/src/zk_controller/validate.rs @@ -446,16 +446,24 @@ pub fn validate( // ConfigMap unchanged (see // [`build_discovery_configmap`](crate::discovery::build_discovery_configmap)): // - the role Listener does not exist (first reconciliation, or it was deleted), - // - it has no status yet (`Ok(None)`), - // - it publishes no address for the server port: an empty address list, or a stale status - // whose port was renamed (`PortNotFound`). Failing here would abort the run before the apply - // step, so the Listener spec carrying the right port would never be applied. + // - it has no status yet, or an empty address list (`Ok(None)`), + // - it publishes addresses, but none for the server port, e.g. a stale status whose port was + // renamed (`PortNotFound`). Failing here would abort the run before the apply step, so the + // Listener spec carrying the right port would never be applied. This is not expected in + // normal operation, so it is logged as a warning. // Any other error (an invalid port number) still fails the run. let discovery_addresses = match &dereferenced_objects.maybe_role_listener { None => None, Some(listener) => match listener_addresses(listener, ZOOKEEPER_SERVER_PORT_NAME) { Ok(addresses) => addresses, - Err(listener_addresses::Error::PortNotFound { .. }) => None, + Err(error @ listener_addresses::Error::PortNotFound { .. }) => { + tracing::warn!( + error = %error, + "the role Listener publishes no address for the ZooKeeper server port, so the \ + discovery ConfigMap is not updated until it does" + ); + None + } Err(source) => return Err(source).context(ReadRoleListenerAddressesSnafu), }, }; From 355f7af86a1577e94e56da4167d81c21d0b77739 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 16:43:21 +0100 Subject: [PATCH 06/13] only re-emit explicit keys, not overrides --- rust/operator-binary/src/discovery.rs | 66 ++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index c86acc77..a060fbda 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -93,7 +93,12 @@ pub fn build_discovery_configmap( &validated_cluster.namespace, labels, ), - data: existing.data.clone(), + data: existing.data.as_ref().map(|data| { + data.iter() + .filter(|(key, _)| DISCOVERY_CONFIG_MAP_KEYS.contains(&key.as_str())) + .map(|(key, value)| (key.clone(), value.clone())) + .collect() + }), ..ConfigMap::default() })) } @@ -150,6 +155,17 @@ fn znode_discovery_labels(validated_znode: &ValidatedZnode) -> Labels { ]) } +/// The keys of a discovery [`ConfigMap`], as written by [`build_discovery_configmap_for_owner`]. +/// +/// Only these are re-emitted, so that a key added by an objectOverride is not kept alive after the +/// override was removed. +const DISCOVERY_CONFIG_MAP_KEYS: [&str; 4] = [ + "ZOOKEEPER", + "ZOOKEEPER_HOSTS", + "ZOOKEEPER_CLIENT_PORT", + "ZOOKEEPER_CHROOT", +]; + /// Build a discovery [`ConfigMap`] containing ZooKeeper connection details from the /// [`ListenerAddresses`] published by the role Listener. /// @@ -468,4 +484,52 @@ mod tests { Some(discovery_config_map_name(&cluster.name).as_ref()) ); } + + /// [`DISCOVERY_CONFIG_MAP_KEYS`] must list exactly the keys a fresh build writes, or the + /// re-emit would drop or keep the wrong ones. + #[test] + fn discovery_config_map_keys_match_a_fresh_build() { + let cluster = try_validate_with_role_listener( + &minimal_zk(ZK_YAML), + Some(listener_with(&[("node-0", 2282)])), + None, + ) + .expect("validate should succeed for the test fixture"); + + let config_map = build_discovery_configmap(&cluster, &ZookeeperRole::Server) + .expect("build") + .expect("fresh"); + + let mut expected_keys = DISCOVERY_CONFIG_MAP_KEYS.to_vec(); + expected_keys.sort_unstable(); + assert_eq!( + config_map + .data + .expect("data") + .keys() + .map(String::as_str) + .collect::>(), + expected_keys + ); + } + + /// Keys that the operator does not write (e.g. added by a since removed objectOverride) are + /// not re-emitted. + #[test] + fn reemit_keeps_only_the_discovery_keys() { + let mut stored = stored_config_map("node-0:2282"); + stored + .data + .as_mut() + .expect("data") + .insert("EXTRA".to_owned(), "x".to_owned()); + let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None, Some(stored)) + .expect("validate should succeed for the test fixture"); + + let config_map = build_discovery_configmap(&cluster, &ZookeeperRole::Server) + .expect("build") + .expect("the stored ConfigMap is re-emitted"); + + assert_eq!(config_map.data, stored_config_map("node-0:2282").data); + } } From 1e92b75717c25368a393f2cddc6b715937543c61 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 17:02:08 +0100 Subject: [PATCH 07/13] add owner check via uid --- rust/operator-binary/src/discovery.rs | 56 +++++++++++++++++-- rust/operator-binary/src/zk_controller.rs | 5 +- .../src/zk_controller/build/resource/rbac.rs | 8 ++- .../zk_controller/build/resource/service.rs | 6 +- .../src/zk_controller/validate.rs | 9 +-- 5 files changed, 66 insertions(+), 18 deletions(-) diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index a060fbda..f2a0f0ab 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -15,7 +15,10 @@ use stackable_operator::{ HasName, HasUid, NameIsValidLabelValue, builder::meta::ownerreference_from_resource, kvp::label, - types::{kubernetes::ConfigMapName, operator::ClusterName}, + types::{ + kubernetes::{ConfigMapName, Uid}, + operator::ClusterName, + }, }, }; @@ -54,6 +57,19 @@ pub fn discovery_config_map_name(cluster_name: &ClusterName) -> ConfigMapName { .expect("a valid cluster name is a valid ConfigMap name") } +/// Whether `config_map` is owned by the object with the given UID. +/// +/// A ConfigMap of the same name left behind by a deleted cluster (e.g. after +/// `kubectl delete --cascade=orphan`) must not be re-emitted for a new cluster. +fn is_owned_by(config_map: &ConfigMap, uid: &Uid) -> bool { + config_map + .metadata + .owner_references + .iter() + .flatten() + .any(|owner_reference| owner_reference.uid == uid.to_string()) +} + /// Build the discovery [`ConfigMap`] for the cluster controller from the [`ValidatedCluster`]. /// /// The connection details come from the addresses published by the role Listener @@ -83,7 +99,7 @@ pub fn build_discovery_configmap( &validated_cluster.cluster_config.zookeeper_security, ) .map(Some), - (None, Some(existing)) => { + (None, Some(existing)) if is_owned_by(existing, &validated_cluster.uid) => { tracing::debug!( "the role Listener has no usable address, re-emitting the stored discovery ConfigMap" ); @@ -102,9 +118,10 @@ pub fn build_discovery_configmap( ..ConfigMap::default() })) } - (None, None) => { + (None, _) => { tracing::debug!( - "the role Listener has no usable address and no discovery ConfigMap is stored yet, skipping it" + "the role Listener has no usable address and no discovery ConfigMap of this cluster \ + is stored, skipping it" ); Ok(None) } @@ -224,14 +241,16 @@ fn discovery_config_map_metadata( mod tests { use std::collections::BTreeMap; - use stackable_operator::crd::listener; + use stackable_operator::{ + crd::listener, k8s_openapi::apimachinery::pkg::apis::meta::v1::OwnerReference, + }; use super::*; use crate::{ crd::ZOOKEEPER_SERVER_PORT_NAME, listener_addresses::test_support::{ingress_address, role_listener}, zk_controller::test_support::{ - app_version_label, minimal_zk, try_validate_with_role_listener, + CLUSTER_UID, app_version_label, minimal_zk, try_validate_with_role_listener, }, znode_controller::test_support::{minimal_znode, validated_znode}, }; @@ -295,6 +314,14 @@ mod tests { resource_version: Some("42".to_owned()), uid: Some("0b5c7b8e-0000-0000-0000-000000000000".to_owned()), managed_fields: Some(vec![]), + owner_references: Some(vec![OwnerReference { + api_version: "zookeeper.stackable.tech/v1alpha1".to_owned(), + kind: "ZookeeperCluster".to_owned(), + name: "simple-zookeeper".to_owned(), + uid: CLUSTER_UID.to_owned(), + controller: Some(true), + block_owner_deletion: Some(true), + }]), ..ObjectMeta::default() }, data: Some( @@ -532,4 +559,21 @@ mod tests { assert_eq!(config_map.data, stored_config_map("node-0:2282").data); } + + /// A stored ConfigMap that another object owns, e.g. left behind by a deleted cluster of the + /// same name, is not re-emitted. + #[test] + fn does_not_reemit_a_config_map_owned_by_another_cluster() { + let mut stored = stored_config_map("node-0:2282"); + for owner_reference in stored.metadata.owner_references.iter_mut().flatten() { + owner_reference.uid = "00000000-0000-0000-0000-000000000000".to_owned(); + } + let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None, Some(stored)) + .expect("validate should succeed for the test fixture"); + + let config_map = + build_discovery_configmap(&cluster, &ZookeeperRole::Server).expect("build"); + + assert!(config_map.is_none()); + } } diff --git a/rust/operator-binary/src/zk_controller.rs b/rust/operator-binary/src/zk_controller.rs index db95c9f5..50eb21fd 100644 --- a/rust/operator-binary/src/zk_controller.rs +++ b/rust/operator-binary/src/zk_controller.rs @@ -201,6 +201,9 @@ pub(crate) mod test_support { }, }; + /// The UID that `minimal_zk` gives the fixture cluster. + pub const CLUSTER_UID: &str = "c27b3971-ca72-42c1-80a4-abdfc1db0ddd"; + /// 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 @@ -225,7 +228,7 @@ pub(crate) mod test_support { .get_or_insert_with(|| "default".to_owned()); zk.metadata .uid - .get_or_insert_with(|| "c27b3971-ca72-42c1-80a4-abdfc1db0ddd".to_owned()); + .get_or_insert_with(|| CLUSTER_UID.to_owned()); zk } diff --git a/rust/operator-binary/src/zk_controller/build/resource/rbac.rs b/rust/operator-binary/src/zk_controller/build/resource/rbac.rs index 32fc2a38..3504a0a2 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/rbac.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/rbac.rs @@ -33,7 +33,9 @@ mod tests { use serde_json::json; use super::*; - use crate::zk_controller::test_support::{app_version_label, minimal_zk, validated_cluster}; + use crate::zk_controller::test_support::{ + CLUSTER_UID, app_version_label, minimal_zk, validated_cluster, + }; // `simple-zookeeper` vs `zookeeper`: see the swap-guard note on `minimal_zk`. fn cluster() -> ValidatedCluster { @@ -81,7 +83,7 @@ mod tests { "controller": true, "kind": "ZookeeperCluster", "name": "simple-zookeeper", - "uid": "c27b3971-ca72-42c1-80a4-abdfc1db0ddd" + "uid": CLUSTER_UID.to_owned() } ] } @@ -114,7 +116,7 @@ mod tests { "controller": true, "kind": "ZookeeperCluster", "name": "simple-zookeeper", - "uid": "c27b3971-ca72-42c1-80a4-abdfc1db0ddd" + "uid": CLUSTER_UID.to_owned() } ] }, 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 45f346a0..45ee483d 100644 --- a/rust/operator-binary/src/zk_controller/build/resource/service.rs +++ b/rust/operator-binary/src/zk_controller/build/resource/service.rs @@ -129,7 +129,9 @@ mod tests { use super::*; use crate::{ crd::ZookeeperRole, - zk_controller::test_support::{app_version_label, minimal_zk, validated_cluster}, + zk_controller::test_support::{ + CLUSTER_UID, app_version_label, minimal_zk, validated_cluster, + }, }; /// Every metrics Service must carry the Prometheus scrape label and the @@ -193,7 +195,7 @@ mod tests { "controller": true, "kind": "ZookeeperCluster", "name": "simple-zookeeper", - "uid": "c27b3971-ca72-42c1-80a4-abdfc1db0ddd" + "uid": CLUSTER_UID.to_owned() } ] }, diff --git a/rust/operator-binary/src/zk_controller/validate.rs b/rust/operator-binary/src/zk_controller/validate.rs index 1ce723ce..54904fa7 100644 --- a/rust/operator-binary/src/zk_controller/validate.rs +++ b/rust/operator-binary/src/zk_controller/validate.rs @@ -533,8 +533,8 @@ mod tests { use crate::{ listener_addresses::test_support::{ingress_address, role_listener}, zk_controller::test_support::{ - app_version_label, minimal_zk, try_validate, try_validate_with_role_listener, - validated_cluster, + CLUSTER_UID, app_version_label, minimal_zk, try_validate, + try_validate_with_role_listener, validated_cluster, }, }; @@ -580,10 +580,7 @@ mod tests { 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.uid.to_string(), CLUSTER_UID.to_owned()); assert_eq!( validated.image.image, format!("oci.example.org/zookeeper:{}", app_version_label("3.9.6")) From 70f3cc72c886dd1d1b455a7d698d6ecd021b678f Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 17:12:49 +0100 Subject: [PATCH 08/13] while paused, skip a discovery ConfigMap that does not exist yet --- rust/operator-binary/src/discovery.rs | 58 +++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index f2a0f0ab..6dadb8d2 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -90,6 +90,12 @@ pub fn build_discovery_configmap( &validated_cluster.discovery_addresses, &validated_cluster.existing_discovery_config_map, ) { + (Some(_), None) if validated_cluster.cluster_operation.reconciliation_paused => { + tracing::debug!( + "reconciliation is paused and no discovery ConfigMap is stored yet, skipping it" + ); + Ok(None) + } (Some(addresses), _) => build_discovery_configmap_for_owner( validated_cluster, &validated_cluster.namespace, @@ -279,6 +285,23 @@ mod tests { name: simple-zookeeper "#; + /// The fixture cluster with reconciliation paused. + const PAUSED_ZK_YAML: &str = r#" + apiVersion: zookeeper.stackable.tech/v1alpha1 + kind: ZookeeperCluster + metadata: + name: simple-zookeeper + spec: + image: + productVersion: "3.9.6" + clusterOperation: + reconciliationPaused: true + servers: + roleGroups: + default: + replicas: 3 + "#; + /// The znode path that the znode controller derives from the fixture's UID. const ZNODE_PATH: &str = "/znode-e5dbf9c2-d8b0-4c1e-9f4a-1d2e3f4a5b6c"; @@ -576,4 +599,39 @@ mod tests { assert!(config_map.is_none()); } + + /// While reconciliation is paused, the apply step only fetches resources. A ConfigMap that was + /// never stored is therefore not built, even once addresses exist, or every paused run would + /// fail fetching it. + #[test] + fn skips_cluster_discovery_config_map_while_paused_and_none_is_stored() { + let cluster = try_validate_with_role_listener( + &minimal_zk(PAUSED_ZK_YAML), + Some(listener_with(&[("node-0", 2282)])), + None, + ) + .expect("validate should succeed for the test fixture"); + + let config_map = + build_discovery_configmap(&cluster, &ZookeeperRole::Server).expect("build"); + + assert!(config_map.is_none()); + } + + /// While reconciliation is paused, a stored ConfigMap still goes through the apply step, which + /// fetches it, so the status keeps its hash. + #[test] + fn builds_cluster_discovery_config_map_while_paused_when_one_is_stored() { + let cluster = try_validate_with_role_listener( + &minimal_zk(PAUSED_ZK_YAML), + Some(listener_with(&[("node-0", 2282)])), + Some(stored_config_map("node-0:2282")), + ) + .expect("validate should succeed for the test fixture"); + + let config_map = + build_discovery_configmap(&cluster, &ZookeeperRole::Server).expect("build"); + + assert!(config_map.is_some()); + } } From 1f8ca0f26150c78a5063d8c2538c0ed0926af514 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Thu, 5 Nov 2026 17:20:16 +0100 Subject: [PATCH 09/13] updated doc comments --- rust/operator-binary/src/discovery.rs | 8 +++++++- rust/operator-binary/src/zk_controller.rs | 2 +- rust/operator-binary/src/zk_controller/dereference.rs | 5 +++-- rust/operator-binary/src/zk_controller/update_status.rs | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index 6dadb8d2..bad307f2 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -78,7 +78,13 @@ fn is_owned_by(config_map: &ConfigMap, uid: &Uid) -> bool { /// fresh ConfigMap can be built. The stored one is then re-emitted unchanged, so that it stays /// tracked by the apply step (an untracked ConfigMap would be deleted as an orphan) without /// changing its content: an unchanged ConfigMap gets no new resource version, so consumers are -/// not restarted. `None` is returned only before a discovery ConfigMap has ever been stored. +/// not restarted. Only the keys in [`DISCOVERY_CONFIG_MAP_KEYS`] are re-emitted, and only from a +/// ConfigMap owned by this cluster. +/// +/// `None` is returned while no discovery ConfigMap (owned by this cluster) is stored, e.g. before +/// the first successful build, or after it was deleted while the Listener had no usable address. +/// It is also returned while reconciliation is paused and none is stored yet: the paused apply +/// step only fetches resources, and fetching a ConfigMap that does not exist would fail every run. /// The Listener watch triggers a new run once addresses are published. pub fn build_discovery_configmap( validated_cluster: &ValidatedCluster, diff --git a/rust/operator-binary/src/zk_controller.rs b/rust/operator-binary/src/zk_controller.rs index 50eb21fd..3da48dc2 100644 --- a/rust/operator-binary/src/zk_controller.rs +++ b/rust/operator-binary/src/zk_controller.rs @@ -115,7 +115,7 @@ pub struct KubernetesResources { pub listeners: Vec, pub config_maps: Vec, /// The discovery `ConfigMap`, kept apart from the role group `config_maps` because the cluster - /// status carries a hash of it. `None` before it can be built for the first time. + /// status carries a hash of it. `None` while none can be built or re-emitted. pub discovery_config_map: Option, pub pod_disruption_budgets: Vec, pub service_accounts: Vec, diff --git a/rust/operator-binary/src/zk_controller/dereference.rs b/rust/operator-binary/src/zk_controller/dereference.rs index 7c5e0072..5afdfad5 100644 --- a/rust/operator-binary/src/zk_controller/dereference.rs +++ b/rust/operator-binary/src/zk_controller/dereference.rs @@ -66,8 +66,9 @@ pub struct DereferencedObjects { /// controller watches Listeners, so a reconciliation is triggered as soon as that happens. pub maybe_role_listener: Option, - /// The discovery ConfigMap as stored by an earlier reconciliation, or `None` before the first - /// successful build. The build step re-emits it while the role Listener has no usable address. + /// The discovery ConfigMap as stored by an earlier reconciliation (named by + /// [`discovery_config_map_name`]), or `None` while none is stored. The build step re-emits it + /// while the role Listener has no usable address. pub existing_discovery_config_map: Option, } diff --git a/rust/operator-binary/src/zk_controller/update_status.rs b/rust/operator-binary/src/zk_controller/update_status.rs index 4380bdca..c492c3c6 100644 --- a/rust/operator-binary/src/zk_controller/update_status.rs +++ b/rust/operator-binary/src/zk_controller/update_status.rs @@ -71,7 +71,7 @@ pub async fn update_status( /// Hashes the resource version of the applied discovery ConfigMap, so that clients can tell when /// the published connection details changed. /// -/// No ConfigMap (not built yet) or one without a resource version hashes nothing. +/// No ConfigMap (none built or re-emitted) or one without a resource version hashes nothing. fn discovery_hash(discovery_config_map: Option<&ConfigMap>) -> String { // std's SipHasher is deprecated, and DefaultHasher is unstable across Rust releases. // We don't /need/ stability, but it's still nice to avoid spurious changes where possible. From b7978ca08333d5a833aae601f778ab9eb6c499af Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 6 Nov 2026 09:19:21 +0100 Subject: [PATCH 10/13] add test step to check role listener recreation --- .../60-recreate-role-listener.yaml | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 tests/templates/kuttl/cluster-operation/60-recreate-role-listener.yaml diff --git a/tests/templates/kuttl/cluster-operation/60-recreate-role-listener.yaml b/tests/templates/kuttl/cluster-operation/60-recreate-role-listener.yaml new file mode 100644 index 00000000..05cf99ac --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/60-recreate-role-listener.yaml @@ -0,0 +1,46 @@ +--- +# While the role Listener is re-created it has no address for a moment. The stored discovery +# ConfigMap must be re-emitted unchanged in that window: not blanked and refilled (a new +# resourceVersion), and not deleted as an orphan and re-created (a new UID). +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +metadata: + name: recreate-role-listener +timeout: 300 +commands: + - script: | + set -eu + + config_map() { kubectl get -n "$NAMESPACE" configmap test-zk -o jsonpath="$1"; } + listener() { kubectl get -n "$NAMESPACE" listener test-zk-server -o jsonpath="$1" 2>/dev/null || true; } + + old_uid=$(config_map '{.metadata.uid}') + old_resource_version=$(config_map '{.metadata.resourceVersion}') + old_data=$(config_map '{.data}') + old_listener_uid=$(listener '{.metadata.uid}') + + kubectl delete -n "$NAMESPACE" listener test-zk-server --wait=false + + # Wait for the re-created Listener to publish an address again. + for _ in $(seq 1 120); do + listener_uid=$(listener '{.metadata.uid}') + address=$(listener '{.status.ingressAddresses[0].address}') + if [ -n "$listener_uid" ] && [ "$listener_uid" != "$old_listener_uid" ] && [ -n "$address" ]; then + break + fi + sleep 1 + done + if [ -z "${address:-}" ] || [ "${listener_uid:-}" = "$old_listener_uid" ]; then + echo "the role Listener was not re-created with an address" >&2 + exit 1 + fi + + # Let the reconcile triggered by the Listener status update finish. + sleep 10 + + [ "$(config_map '{.metadata.uid}')" = "$old_uid" ] \ + || { echo "the discovery ConfigMap was deleted and re-created" >&2; exit 1; } + [ "$(config_map '{.data}')" = "$old_data" ] \ + || { echo "the discovery ConfigMap data changed" >&2; exit 1; } + [ "$(config_map '{.metadata.resourceVersion}')" = "$old_resource_version" ] \ + || { echo "the discovery ConfigMap was rewritten (resourceVersion changed)" >&2; exit 1; } From 2110c28c07f2754e850519b5b12213afe47afa3c Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Fri, 6 Nov 2026 09:27:21 +0100 Subject: [PATCH 11/13] changelog --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e1d2ca4d..ada02cb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,8 +26,6 @@ All notable changes to this project will be documented in this file. Previously a ZookeeperCluster without it was accepted by the API server but failed reconciliation ([#1060]). - The reconciler now applies resources and derives the cluster status in discrete apply and update_status steps for the `zk_controller` and `znode_controller` ([#1069]). -- The discovery ConfigMap is now always written, with empty `ZOOKEEPER` and `ZOOKEEPER_HOSTS` while - the listener publishes no addresses ([#1069]). - All product containers now run with `securityContext.runAsNonRoot` set to `true` to improve security ([#1070]). - Remove the `app.kubernetes.io/component` and `app.kubernetes.io/role-group` labels from the resources they don't apply to (previously set to `none` or the placeholder value `discovery`) @@ -41,6 +39,8 @@ All notable changes to this project will be documented in this file. - All pods now set `securityContext.seccompProfile.type` to `RuntimeDefault` ([#1097]). - Replace the unmaintained `serde_yaml` with `yaml_serde` ([#1097]). - The operator Deployment runs with a restricted security context and a read-only root filesystem, so it can be deployed into namespaces that enforce the `restricted` Pod Security Standard ([#1101]). +- An existing discovery ConfigMap is kept unchanged while the role listener has no address and a new one is only created once the address is available. + Previously, reconciliation failed while the address was missing ([#1069], [#1105]). ### Fixed @@ -68,6 +68,7 @@ All notable changes to this project will be documented in this file. [#1100]: https://github.com/stackabletech/zookeeper-operator/pull/1100 [#1101]: https://github.com/stackabletech/zookeeper-operator/pull/1101 [#1102]: https://github.com/stackabletech/zookeeper-operator/pull/1102 +[#1105]: https://github.com/stackabletech/zookeeper-operator/pull/1105 ## [26.7.0] - 2026-07-21 From a34af9e12ed039d4fc85c9ac0deea3413e515aee Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 11 Nov 2026 13:14:59 +0100 Subject: [PATCH 12/13] move config map check from build to dereference --- rust/operator-binary/src/discovery.rs | 40 +------- .../src/zk_controller/dereference.rs | 92 +++++++++++++++++-- 2 files changed, 89 insertions(+), 43 deletions(-) diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index bad307f2..ceaaeb25 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -15,10 +15,7 @@ use stackable_operator::{ HasName, HasUid, NameIsValidLabelValue, builder::meta::ownerreference_from_resource, kvp::label, - types::{ - kubernetes::{ConfigMapName, Uid}, - operator::ClusterName, - }, + types::{kubernetes::ConfigMapName, operator::ClusterName}, }, }; @@ -57,19 +54,6 @@ pub fn discovery_config_map_name(cluster_name: &ClusterName) -> ConfigMapName { .expect("a valid cluster name is a valid ConfigMap name") } -/// Whether `config_map` is owned by the object with the given UID. -/// -/// A ConfigMap of the same name left behind by a deleted cluster (e.g. after -/// `kubectl delete --cascade=orphan`) must not be re-emitted for a new cluster. -fn is_owned_by(config_map: &ConfigMap, uid: &Uid) -> bool { - config_map - .metadata - .owner_references - .iter() - .flatten() - .any(|owner_reference| owner_reference.uid == uid.to_string()) -} - /// Build the discovery [`ConfigMap`] for the cluster controller from the [`ValidatedCluster`]. /// /// The connection details come from the addresses published by the role Listener @@ -78,8 +62,7 @@ fn is_owned_by(config_map: &ConfigMap, uid: &Uid) -> bool { /// fresh ConfigMap can be built. The stored one is then re-emitted unchanged, so that it stays /// tracked by the apply step (an untracked ConfigMap would be deleted as an orphan) without /// changing its content: an unchanged ConfigMap gets no new resource version, so consumers are -/// not restarted. Only the keys in [`DISCOVERY_CONFIG_MAP_KEYS`] are re-emitted, and only from a -/// ConfigMap owned by this cluster. +/// not restarted. Only the keys in [`DISCOVERY_CONFIG_MAP_KEYS`] are re-emitted. /// /// `None` is returned while no discovery ConfigMap (owned by this cluster) is stored, e.g. before /// the first successful build, or after it was deleted while the Listener had no usable address. @@ -111,7 +94,7 @@ pub fn build_discovery_configmap( &validated_cluster.cluster_config.zookeeper_security, ) .map(Some), - (None, Some(existing)) if is_owned_by(existing, &validated_cluster.uid) => { + (None, Some(existing)) => { tracing::debug!( "the role Listener has no usable address, re-emitting the stored discovery ConfigMap" ); @@ -589,23 +572,6 @@ mod tests { assert_eq!(config_map.data, stored_config_map("node-0:2282").data); } - /// A stored ConfigMap that another object owns, e.g. left behind by a deleted cluster of the - /// same name, is not re-emitted. - #[test] - fn does_not_reemit_a_config_map_owned_by_another_cluster() { - let mut stored = stored_config_map("node-0:2282"); - for owner_reference in stored.metadata.owner_references.iter_mut().flatten() { - owner_reference.uid = "00000000-0000-0000-0000-000000000000".to_owned(); - } - let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None, Some(stored)) - .expect("validate should succeed for the test fixture"); - - let config_map = - build_discovery_configmap(&cluster, &ZookeeperRole::Server).expect("build"); - - assert!(config_map.is_none()); - } - /// While reconciliation is paused, the apply step only fetches resources. A ConfigMap that was /// never stored is therefore not built, even once addresses exist, or every paused run would /// fail fetching it. diff --git a/rust/operator-binary/src/zk_controller/dereference.rs b/rust/operator-binary/src/zk_controller/dereference.rs index 5afdfad5..cdec9d17 100644 --- a/rust/operator-binary/src/zk_controller/dereference.rs +++ b/rust/operator-binary/src/zk_controller/dereference.rs @@ -11,8 +11,11 @@ use stackable_operator::{ crd::listener, k8s_openapi::api::core::v1::ConfigMap, v2::{ - controller_utils::{get_cluster_name, get_namespace}, - types::{kubernetes::NamespaceName, operator::ClusterName}, + controller_utils::{get_cluster_name, get_namespace, get_uid}, + types::{ + kubernetes::{NamespaceName, Uid}, + operator::ClusterName, + }, }, }; @@ -50,6 +53,11 @@ pub enum Error { source: stackable_operator::client::Error, config_map: String, }, + + #[snafu(display("failed to get the cluster UID"))] + GetUid { + source: stackable_operator::v2::controller_utils::Error, + }, } type Result = std::result::Result; @@ -67,12 +75,13 @@ pub struct DereferencedObjects { pub maybe_role_listener: Option, /// The discovery ConfigMap as stored by an earlier reconciliation (named by - /// [`discovery_config_map_name`]), or `None` while none is stored. The build step re-emits it - /// while the role Listener has no usable address. + /// [`discovery_config_map_name`]), or `None` while none owned by this cluster is stored. + /// The build step re-emits it while the role Listener has no usable address. pub existing_discovery_config_map: Option, } -/// Fetches all Kubernetes objects referenced from the [`v1alpha1::ZookeeperCluster`] spec. +/// Fetches all Kubernetes objects referenced from the [`v1alpha1::ZookeeperCluster`] spec +/// (only considering a ConfigMap owned by this cluster). pub async fn dereference( client: &Client, zk: &v1alpha1::ZookeeperCluster, @@ -88,6 +97,7 @@ pub async fn dereference( .context(FetchAuthenticationClassesSnafu)?; let maybe_role_listener = fetch_role_listener(client, &cluster_name, &namespace).await?; + let uid = get_uid(zk).context(GetUidSnafu)?; let discovery_config_map_name = discovery_config_map_name(&cluster_name); let existing_discovery_config_map = client @@ -95,7 +105,8 @@ pub async fn dereference( .await .context(FetchExistingDiscoveryConfigMapSnafu { config_map: discovery_config_map_name.as_ref(), - })?; + })? + .filter(|config_map| is_owned_by(config_map, &uid)); Ok(DereferencedObjects { authentication_classes, @@ -116,3 +127,72 @@ async fn fetch_role_listener( .await .context(FetchRoleListenerSnafu) } + +/// Whether `config_map` is owned by the object with the given UID. +/// +/// A ConfigMap of the same name left behind by a deleted cluster (e.g. after +/// `kubectl delete --cascade=orphan`) must not be re-emitted for a new cluster. +fn is_owned_by(config_map: &ConfigMap, uid: &Uid) -> bool { + config_map + .metadata + .owner_references + .iter() + .flatten() + .any(|owner_reference| owner_reference.uid == uid.to_string()) +} + +#[cfg(test)] +mod tests { + use std::str::FromStr; + + use stackable_operator::k8s_openapi::apimachinery::pkg::apis::meta::v1::{ + ObjectMeta, OwnerReference, + }; + + use super::*; + use crate::zk_controller::test_support::CLUSTER_UID; + + fn config_map_owned_by(owner_uids: &[&str]) -> ConfigMap { + ConfigMap { + metadata: ObjectMeta { + owner_references: Some( + owner_uids + .iter() + .map(|uid| OwnerReference { + uid: (*uid).to_owned(), + ..OwnerReference::default() + }) + .collect(), + ), + ..ObjectMeta::default() + }, + ..ConfigMap::default() + } + } + + fn cluster_uid() -> Uid { + Uid::from_str(CLUSTER_UID).expect("valid UID") + } + + #[test] + fn config_map_owned_by_this_cluster_is_considered() { + assert!(is_owned_by( + &config_map_owned_by(&[CLUSTER_UID]), + &cluster_uid() + )); + } + + /// E.g. left behind by a deleted cluster of the same name. + #[test] + fn config_map_owned_by_another_object_is_ignored() { + assert!(!is_owned_by( + &config_map_owned_by(&["00000000-0000-0000-0000-000000000000"]), + &cluster_uid() + )); + } + + #[test] + fn config_map_without_owner_references_is_ignored() { + assert!(!is_owned_by(&ConfigMap::default(), &cluster_uid())); + } +} From 7811227b93f34089cbd003b101118d681e3295a6 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 11 Nov 2026 16:08:53 +0100 Subject: [PATCH 13/13] use struct for config map fields --- rust/operator-binary/src/discovery.rs | 236 ++++++++++++++------------ 1 file changed, 132 insertions(+), 104 deletions(-) diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index ceaaeb25..c75c3703 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -3,13 +3,14 @@ //! Shared by the build steps of both controllers: the ZookeeperCluster controller publishes the //! whole ensemble, the ZookeeperZnode controller publishes the same ensemble narrowed to a chroot. -use std::str::FromStr; +use std::{collections::BTreeMap, str::FromStr}; +use serde::{Deserialize, Serialize}; use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::{configmap::ConfigMapBuilder, meta::ObjectMetaBuilder}, k8s_openapi::api::core::v1::ConfigMap, - kube::{Resource, api::ObjectMeta}, + kube::Resource, kvp::{Label, Labels}, v2::{ HasName, HasUid, NameIsValidLabelValue, @@ -62,7 +63,8 @@ pub fn discovery_config_map_name(cluster_name: &ClusterName) -> ConfigMapName { /// fresh ConfigMap can be built. The stored one is then re-emitted unchanged, so that it stays /// tracked by the apply step (an untracked ConfigMap would be deleted as an orphan) without /// changing its content: an unchanged ConfigMap gets no new resource version, so consumers are -/// not restarted. Only the keys in [`DISCOVERY_CONFIG_MAP_KEYS`] are re-emitted. +/// not restarted. Only the keys of [`DiscoveryData`] are re-emitted; a stored ConfigMap that +/// lacks one of them is not re-emitted. /// /// `None` is returned while no discovery ConfigMap (owned by this cluster) is stored, e.g. before /// the first successful build, or after it was deleted while the Listener had no usable address. @@ -85,34 +87,42 @@ pub fn build_discovery_configmap( ); Ok(None) } - (Some(addresses), _) => build_discovery_configmap_for_owner( - validated_cluster, - &validated_cluster.namespace, - labels, - addresses, - None, - &validated_cluster.cluster_config.zookeeper_security, - ) - .map(Some), - (None, Some(existing)) => { - tracing::debug!( - "the role Listener has no usable address, re-emitting the stored discovery ConfigMap" - ); - Ok(Some(ConfigMap { - metadata: discovery_config_map_metadata( + (Some(addresses), _) => { + let discovery_data = DiscoveryData::from_addresses( + addresses, + None, + &validated_cluster.cluster_config.zookeeper_security, + )?; + build_discovery_configmap_for_owner( + validated_cluster, + &validated_cluster.namespace, + labels, + discovery_data, + ) + .map(Some) + } + (None, Some(existing)) => match DiscoveryData::from_stored(existing) { + Some(discovery_data) => { + tracing::debug!( + "the role Listener has no usable address, re-emitting the stored discovery \ + ConfigMap" + ); + build_discovery_configmap_for_owner( validated_cluster, &validated_cluster.namespace, labels, - ), - data: existing.data.as_ref().map(|data| { - data.iter() - .filter(|(key, _)| DISCOVERY_CONFIG_MAP_KEYS.contains(&key.as_str())) - .map(|(key, value)| (key.clone(), value.clone())) - .collect() - }), - ..ConfigMap::default() - })) - } + discovery_data, + ) + .map(Some) + } + None => { + tracing::warn!( + "the role Listener has no usable address and the stored discovery ConfigMap \ + lacks one of its keys, so it is not re-emitted" + ); + Ok(None) + } + }, (None, _) => { tracing::debug!( "the role Listener has no usable address and no discovery ConfigMap of this cluster \ @@ -134,13 +144,16 @@ pub fn build_znode_discovery_configmap( listener_addresses: &ListenerAddresses, chroot: &str, ) -> Result { + let discovery_data = DiscoveryData::from_addresses( + listener_addresses, + Some(chroot), + &validated_znode.zookeeper_security, + )?; build_discovery_configmap_for_owner( validated_znode, &validated_znode.namespace, znode_discovery_labels(validated_znode), - listener_addresses, - Some(chroot), - &validated_znode.zookeeper_security, + discovery_data, ) } @@ -167,19 +180,70 @@ fn znode_discovery_labels(validated_znode: &ValidatedZnode) -> Labels { ]) } -/// The keys of a discovery [`ConfigMap`], as written by [`build_discovery_configmap_for_owner`]. +/// The contents of a discovery [`ConfigMap`]; each field is one key of the ConfigMap, named by its +/// `serde(rename)`. The key names are part of the public interface, see +/// `docs/modules/zookeeper/pages/reference/discovery.adoc`. /// -/// Only these are re-emitted, so that a key added by an objectOverride is not kept alive after the -/// override was removed. -const DISCOVERY_CONFIG_MAP_KEYS: [&str; 4] = [ - "ZOOKEEPER", - "ZOOKEEPER_HOSTS", - "ZOOKEEPER_CLIENT_PORT", - "ZOOKEEPER_CHROOT", -]; - -/// Build a discovery [`ConfigMap`] containing ZooKeeper connection details from the -/// [`ListenerAddresses`] published by the role Listener. +/// Built either fresh from the role Listener's addresses or from a stored discovery ConfigMap, so +/// that both go through [`build_discovery_configmap_for_owner`]. Only these keys are written, so a +/// key added by an objectOverride is not kept alive on re-emit after the override was removed. +#[derive(Debug, Deserialize, PartialEq, Eq, Serialize)] +struct DiscoveryData { + /// The connection string including the chroot. + #[serde(rename = "ZOOKEEPER")] + connection_string: String, + /// The connection string without the chroot, for clients that don't support ZooKeeper's + /// merged `hosts/chroot` format. + #[serde(rename = "ZOOKEEPER_HOSTS")] + hosts: String, + #[serde(rename = "ZOOKEEPER_CLIENT_PORT")] + client_port: String, + #[serde(rename = "ZOOKEEPER_CHROOT")] + chroot: String, +} + +impl DiscoveryData { + fn from_addresses( + listener_addresses: &ListenerAddresses, + chroot: Option<&str>, + zookeeper_security: &ZookeeperSecurity, + ) -> Result { + // Write a connection string of the format that Java ZooKeeper client expects: + // "{host1}:{port1},{host2:port2},.../{chroot}" + // See https://zookeeper.apache.org/doc/current/apidocs/zookeeper-server/org/apache/zookeeper/ZooKeeper.html#ZooKeeper-java.lang.String-int-org.apache.zookeeper.Watcher- + let hosts = listener_addresses.to_connection_string(); + let mut connection_string = hosts.clone(); + if let Some(chroot) = chroot { + if !chroot.starts_with('/') { + return RelativeChrootSnafu { chroot }.fail(); + } + connection_string.push_str(chroot); + } + Ok(Self { + connection_string, + hosts, + client_port: zookeeper_security.client_port().to_string(), + chroot: chroot.unwrap_or("/").to_owned(), + }) + } + + /// Reads the keys back from a stored discovery ConfigMap; `None` if any of them is missing. + /// Other keys are ignored. + fn from_stored(config_map: &ConfigMap) -> Option { + let data = serde_json::to_value(config_map.data.as_ref()?).ok()?; + serde_json::from_value(data).ok() + } + + /// The ConfigMap data, one entry per field. + fn into_config_map_data(self) -> BTreeMap { + // Serialising a struct of `String` fields into a string map cannot fail. + serde_json::to_value(self) + .and_then(serde_json::from_value) + .expect("DiscoveryData has only String fields") + } +} + +/// Build a discovery [`ConfigMap`] with the given [`DiscoveryData`]. /// /// `owner` owns the ConfigMap (the [`ZookeeperCluster`](crate::crd::v1alpha1::ZookeeperCluster) for the cluster /// controller, or the [`ZookeeperZnode`](crate::crd::v1alpha1::ZookeeperZnode) for the znode controller) and @@ -188,56 +252,29 @@ fn build_discovery_configmap_for_owner( owner: &(impl Resource + HasName + HasUid + NameIsValidLabelValue), namespace: impl Into, labels: Labels, - listener_addresses: &ListenerAddresses, - chroot: Option<&str>, - zookeeper_security: &ZookeeperSecurity, + discovery_data: DiscoveryData, ) -> Result { - // Write a connection string of the format that Java ZooKeeper client expects: - // "{host1}:{port1},{host2:port2},.../{chroot}" - // See https://zookeeper.apache.org/doc/current/apidocs/zookeeper-server/org/apache/zookeeper/ZooKeeper.html#ZooKeeper-java.lang.String-int-org.apache.zookeeper.Watcher- - let listener_addresses = listener_addresses.to_connection_string(); - let mut conn_str = listener_addresses.clone(); - if let Some(chroot) = chroot { - if !chroot.starts_with('/') { - return RelativeChrootSnafu { chroot }.fail(); - } - conn_str.push_str(chroot); - } ConfigMapBuilder::new() - .metadata(discovery_config_map_metadata(owner, namespace, labels)) - .add_data("ZOOKEEPER", conn_str) - // Some clients don't support ZooKeeper's merged `hosts/chroot` format, so export them separately for these clients - .add_data("ZOOKEEPER_HOSTS", listener_addresses) - .add_data( - "ZOOKEEPER_CLIENT_PORT", - zookeeper_security.client_port().to_string(), + .metadata( + ObjectMetaBuilder::new() + .name(owner.to_name()) + .namespace(namespace) + .ownerreference(ownerreference_from_resource(owner, None, Some(true))) + .with_labels(labels) + .build(), ) - .add_data("ZOOKEEPER_CHROOT", chroot.unwrap_or("/")) + .data(discovery_data.into_config_map_data()) .build() .context(BuildConfigMapSnafu) } -/// The metadata of a discovery [`ConfigMap`], shared by freshly built and re-emitted ConfigMaps -/// so that the two cannot drift apart. -fn discovery_config_map_metadata( - owner: &(impl Resource + HasName + HasUid + NameIsValidLabelValue), - namespace: impl Into, - labels: Labels, -) -> ObjectMeta { - ObjectMetaBuilder::new() - .name(owner.to_name()) - .namespace(namespace) - .ownerreference(ownerreference_from_resource(owner, None, Some(true))) - .with_labels(labels) - .build() -} - #[cfg(test)] mod tests { use std::collections::BTreeMap; use stackable_operator::{ - crd::listener, k8s_openapi::apimachinery::pkg::apis::meta::v1::OwnerReference, + crd::listener, + k8s_openapi::apimachinery::pkg::apis::meta::v1::{ObjectMeta, OwnerReference}, }; use super::*; @@ -524,32 +561,23 @@ mod tests { ); } - /// [`DISCOVERY_CONFIG_MAP_KEYS`] must list exactly the keys a fresh build writes, or the - /// re-emit would drop or keep the wrong ones. + /// A stored ConfigMap that lacks one of the discovery keys (e.g. edited by hand) is not + /// re-emitted with an empty value; it is left untracked instead. #[test] - fn discovery_config_map_keys_match_a_fresh_build() { - let cluster = try_validate_with_role_listener( - &minimal_zk(ZK_YAML), - Some(listener_with(&[("node-0", 2282)])), - None, - ) - .expect("validate should succeed for the test fixture"); + fn does_not_reemit_a_stored_config_map_with_a_missing_key() { + let mut stored = stored_config_map("node-0:2282"); + stored + .data + .as_mut() + .expect("data") + .remove("ZOOKEEPER_HOSTS"); + let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None, Some(stored)) + .expect("validate should succeed for the test fixture"); - let config_map = build_discovery_configmap(&cluster, &ZookeeperRole::Server) - .expect("build") - .expect("fresh"); + let config_map = + build_discovery_configmap(&cluster, &ZookeeperRole::Server).expect("build"); - let mut expected_keys = DISCOVERY_CONFIG_MAP_KEYS.to_vec(); - expected_keys.sort_unstable(); - assert_eq!( - config_map - .data - .expect("data") - .keys() - .map(String::as_str) - .collect::>(), - expected_keys - ); + assert!(config_map.is_none()); } /// Keys that the operator does not write (e.g. added by a since removed objectOverride) are