diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f4b2685..f676f12f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,8 +28,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`) @@ -43,6 +41,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 @@ -74,6 +74,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 [#1106]: https://github.com/stackabletech/zookeeper-operator/pull/1106 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/discovery.rs b/rust/operator-binary/src/discovery.rs index 9a6a87a5..c75c3703 100644 --- a/rust/operator-binary/src/discovery.rs +++ b/rust/operator-binary/src/discovery.rs @@ -3,6 +3,9 @@ //! 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::{collections::BTreeMap, str::FromStr}; + +use serde::{Deserialize, Serialize}; use snafu::{ResultExt, Snafu}; use stackable_operator::{ builder::{configmap::ConfigMapBuilder, meta::ObjectMetaBuilder}, @@ -10,8 +13,10 @@ use stackable_operator::{ kube::Resource, 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,33 +40,97 @@ pub enum Error { }, } -/// Build the discovery [`ConfigMap`] for the cluster controller from the -/// [`ValidatedCluster`]. +/// 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 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 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. Only the keys of [`DiscoveryData`] are re-emitted; a stored ConfigMap that +/// lacks one of them is not re-emitted. /// -/// 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. +/// `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, 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(_), 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), _) => { + 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, + 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 \ + is stored, skipping it" + ); + Ok(None) + } + } } /// Build the discovery [`ConfigMap`] for the znode controller. @@ -75,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, ) } @@ -90,8 +162,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( @@ -108,8 +180,70 @@ fn znode_discovery_labels(validated_znode: &ValidatedZnode) -> Labels { ]) } -/// Build a discovery [`ConfigMap`] containing ZooKeeper connection details from the -/// [`ListenerAddresses`] published by the role Listener. +/// 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`. +/// +/// 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 @@ -118,40 +252,18 @@ 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 { - 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- - 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( ObjectMetaBuilder::new() - .name(name) + .name(owner.to_name()) .namespace(namespace) .ownerreference(ownerreference_from_resource(owner, None, Some(true))) .with_labels(labels) .build(), ) - .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(), - ) - .add_data("ZOOKEEPER_CHROOT", chroot.unwrap_or("/")) + .data(discovery_data.into_config_map_data()) .build() .context(BuildConfigMapSnafu) } @@ -160,15 +272,17 @@ fn build_discovery_configmap_for_owner( mod tests { use std::collections::BTreeMap; + use stackable_operator::{ + crd::listener, + k8s_openapi::apimachinery::pkg::apis::meta::v1::{ObjectMeta, OwnerReference}, + }; + 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, + CLUSTER_UID, app_version_label, minimal_zk, try_validate_with_role_listener, }, znode_controller::test_support::{minimal_znode, validated_znode}, }; @@ -197,6 +311,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"; @@ -211,33 +342,64 @@ 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![]), + 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( + [ + ("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 +415,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 +479,159 @@ 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"); + } + + /// 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()) + ); + } + + /// 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 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"); + + assert!(config_map.is_none()); + } + + /// 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); + } + + /// 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()); + } } diff --git a/rust/operator-binary/src/listener_addresses.rs b/rust/operator-binary/src/listener_addresses.rs index fff6c18b..54e3606e 100644 --- a/rust/operator-binary/src/listener_addresses.rs +++ b/rust/operator-binary/src/listener_addresses.rs @@ -30,10 +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. -/// -/// 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)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct ListenerAddresses(BTreeSet<(String, u16)>); impl ListenerAddresses { @@ -50,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) @@ -67,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) @@ -163,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.rs b/rust/operator-binary/src/zk_controller.rs index aeafa54f..3da48dc2 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` while none can be built or re-emitted. + 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::{ @@ -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 } @@ -248,7 +251,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 +259,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/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/dereference.rs b/rust/operator-binary/src/zk_controller/dereference.rs index c4384a90..cdec9d17 100644 --- a/rust/operator-binary/src/zk_controller/dereference.rs +++ b/rust/operator-binary/src/zk_controller/dereference.rs @@ -9,16 +9,23 @@ 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}, + controller_utils::{get_cluster_name, get_namespace, get_uid}, + types::{ + kubernetes::{NamespaceName, Uid}, + operator::ClusterName, + }, }, }; -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)] @@ -40,6 +47,17 @@ 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, + }, + + #[snafu(display("failed to get the cluster UID"))] + GetUid { + source: stackable_operator::v2::controller_utils::Error, + }, } type Result = std::result::Result; @@ -55,9 +73,15 @@ 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 (named by + /// [`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, @@ -73,10 +97,21 @@ 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 + .get_opt::(discovery_config_map_name.as_ref(), namespace.as_ref()) + .await + .context(FetchExistingDiscoveryConfigMapSnafu { + config_map: discovery_config_map_name.as_ref(), + })? + .filter(|config_map| is_owned_by(config_map, &uid)); Ok(DereferencedObjects { authentication_classes, maybe_role_listener, + existing_discovery_config_map, }) } @@ -92,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())); + } +} diff --git a/rust/operator-binary/src/zk_controller/update_status.rs b/rust/operator-binary/src/zk_controller/update_status.rs index 491a85cf..c492c3c6 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 (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. 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..54904fa7 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,31 @@ 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, 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(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), + }, + }; Ok(ValidatedCluster::new( name, @@ -466,6 +483,7 @@ pub fn validate( zk.spec.cluster_operation.clone(), zk.spec.object_overrides.clone(), discovery_addresses, + dereferenced_objects.existing_discovery_config_map.clone(), )) } @@ -515,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, }, }; @@ -562,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")) @@ -742,10 +757,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 +769,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 +784,78 @@ 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, ListenerAddresses::default()); + 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.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); + } + + /// 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 { .. }) + )); } } 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; }