refactor: compute zoo.cfg server addresses in build, not validate

maltesander · maltesander · commit a837ec130367 · 2026-06-17T20:31:47.000+02:00
diff --git a/rust/operator-binary/src/zk_controller.rs b/rust/operator-binary/src/zk_controller.rs
@@ -218,13 +218,9 @@ pub async fn reconcile_zk(
         .context(DereferenceSnafu)?;
 
     // validate (no client required)
-    let validated_cluster = validate::validate(
-        zk,
-        &dereferenced_objects,
-        &ctx.operator_environment,
-        &client.kubernetes_cluster_info,
-    )
-    .context(ValidateClusterSnafu)?;
+    let validated_cluster =
+        validate::validate(zk, &dereferenced_objects, &ctx.operator_environment)
+            .context(ValidateClusterSnafu)?;
 
     // Names are derived from compile-time constants.
     let mut cluster_resources = cluster_resources_new(
@@ -278,6 +274,7 @@ pub async fn reconcile_zk(
         );
         let rg_configmap = config_map::build_server_rolegroup_config_map(
             &validated_cluster,
+            &client.kubernetes_cluster_info,
             rolegroup_name,
             rolegroup_config,
         )
@@ -423,7 +420,7 @@ pub(crate) mod test_support {
         zk
     }
 
-    fn cluster_info() -> KubernetesClusterInfo {
+    pub fn cluster_info() -> KubernetesClusterInfo {
         KubernetesClusterInfo {
             cluster_domain: DomainName::try_from("cluster.local").expect("valid domain"),
         }
@@ -445,7 +442,6 @@ pub(crate) mod test_support {
                 authentication_classes: DereferencedAuthenticationClasses::new_for_tests(),
             },
             &operator_environment(),
-            &cluster_info(),
         )
         .expect("validate should succeed for the test fixture")
     }
@@ -460,7 +456,7 @@ mod tests {
     };
 
     use super::*;
-    use crate::zk_controller::test_support::{minimal_zk, validated_cluster};
+    use crate::zk_controller::test_support::{cluster_info, minimal_zk, validated_cluster};
 
     #[test]
     fn test_default_config() {
@@ -613,6 +609,7 @@ mod tests {
 
         config_map::build_server_rolegroup_config_map(
             &validated_cluster,
+            &cluster_info(),
             &role_group_name,
             rolegroup_config,
         )
diff --git a/rust/operator-binary/src/zk_controller/build/properties/zoo_cfg.rs b/rust/operator-binary/src/zk_controller/build/properties/zoo_cfg.rs
@@ -2,11 +2,12 @@
 
 use std::collections::BTreeMap;
 
-use stackable_operator::v2::types::common::Port;
+use stackable_operator::{utils::cluster_info::KubernetesClusterInfo, v2::types::common::Port};
 
 use crate::{
     crd::{
         METRICS_PROVIDER_HTTP_PORT, METRICS_PROVIDER_HTTP_PORT_KEY, STACKABLE_DATA_DIR,
+        ZOOKEEPER_ELECTION_PORT, ZOOKEEPER_LEADER_PORT, ZookeeperPodRef, ZookeeperRole,
         security::ZookeeperSecurity, v1alpha1::ZookeeperConfig,
     },
     zk_controller::validate::{ValidatedCluster, ZookeeperRoleGroupConfig},
@@ -21,25 +22,65 @@ const DEFAULT_INIT_LIMIT: &str = "5";
 const DEFAULT_SYNC_LIMIT: &str = "2";
 const DEFAULT_TICK_TIME: &str = "3000";
 
-/// Builds the `zoo.cfg` key/value pairs for a role group.
+/// Builds the `server.<myid>` quorum entries for `zoo.cfg` from the expected pods.
+///
+/// The pods are predicted from the validated role-group configs (`replicas` + `myidOffset`)
+/// rather than from the live cluster state, to avoid instance churn.
+pub(crate) fn server_addresses(
+    cluster: &ValidatedCluster,
+    cluster_info: &KubernetesClusterInfo,
+) -> BTreeMap<String, String> {
+    let security = &cluster.cluster_config.zookeeper_security;
+    let mut server_addresses = BTreeMap::new();
+    for (rg_name, rg_config) in cluster
+        .role_group_configs
+        .get(&ZookeeperRole::Server)
+        .into_iter()
+        .flatten()
+    {
+        let resource_names = cluster.resource_names(rg_name);
+        let headless_service_name = resource_names.headless_service_name();
+        let stateful_set_name = resource_names.stateful_set_name().to_string();
+        // An unset replica count (HPA-managed) predicts a single-server quorum entry, matching
+        // the historical default.
+        for i in 0..rg_config.replicas.unwrap_or(1) {
+            let pod_ref = ZookeeperPodRef {
+                namespace: cluster.namespace.clone(),
+                role_group_headless_service_name: headless_service_name.clone(),
+                pod_name: format!("{stateful_set_name}-{i}"),
+                zookeeper_myid: i + rg_config.config.myid_offset,
+            };
+            server_addresses.insert(
+                format!("server.{id}", id = pod_ref.zookeeper_myid),
+                format!(
+                    "{internal_fqdn}:{ZOOKEEPER_LEADER_PORT}:{ZOOKEEPER_ELECTION_PORT};{client_port}",
+                    internal_fqdn = pod_ref.internal_fqdn(cluster_info),
+                    client_port = security.client_port()
+                ),
+            );
+        }
+    }
+    server_addresses
+}
+
+/// Builds the `zoo.cfg` key/value pairs for a role group, excluding the
+/// `server.<myid>` quorum entries (which depend on `cluster_info`).
 ///
 /// Precedence (lowest to highest):
-/// 1. `server.<myid>` quorum entries (precomputed in validate)
-/// 2. operator-injected defaults (formerly `product-config` `properties.yaml`)
-/// 3. TLS / quorum settings from [`ZookeeperSecurity`]
-/// 4. user-set merged config (`initLimit` / `syncLimit` / `tickTime`)
-/// 5. `configOverrides` for `zoo.cfg`
-pub fn build(
+/// 1. operator-injected defaults (formerly `product-config` `properties.yaml`)
+/// 2. TLS / quorum settings from [`ZookeeperSecurity`]
+/// 3. user-set merged config (`initLimit` / `syncLimit` / `tickTime`)
+/// 4. `configOverrides` for `zoo.cfg`
+fn build_base(
     cluster: &ValidatedCluster,
     rolegroup_config: &ZookeeperRoleGroupConfig,
 ) -> BTreeMap<String, String> {
     let security = &cluster.cluster_config.zookeeper_security;
     let config = &rolegroup_config.config;
 
-    // 1. server.<myid> quorum entries.
-    let mut zoo_cfg = cluster.cluster_config.server_addresses.clone();
+    let mut zoo_cfg = BTreeMap::new();
 
-    // 2. Operator-injected defaults (former properties.yaml recommended/default
+    // 1. Operator-injected defaults (former properties.yaml recommended/default
     //    values and the `Configuration::compute_files` output).
     zoo_cfg.insert(
         ADMIN_SERVER_PORT_KEY.to_string(),
@@ -74,10 +115,10 @@ pub fn build(
         METRICS_PROVIDER_HTTP_PORT.to_string(),
     );
 
-    // 3. TLS / quorum settings.
+    // 2. TLS / quorum settings.
     zoo_cfg.extend(security.config_settings());
 
-    // 4. User-set merged config overrides the seeded defaults above.
+    // 3. User-set merged config overrides the seeded defaults above.
     if let Some(init_limit) = config.init_limit {
         zoo_cfg.insert(
             ZookeeperConfig::INIT_LIMIT.to_string(),
@@ -97,24 +138,73 @@ pub fn build(
         );
     }
 
-    // 5. configOverrides go last so they win.
+    // 4. configOverrides go last so they win.
     zoo_cfg.extend(rolegroup_config.config_overrides.zoo_cfg.clone());
 
     zoo_cfg
 }
 
+/// Builds the full `zoo.cfg` key/value pairs for a role group.
+///
+/// The `server.<myid>` quorum entries take lowest precedence; everything from
+/// [`build_base`] is layered on top.
+pub fn build(
+    cluster: &ValidatedCluster,
+    rolegroup_config: &ZookeeperRoleGroupConfig,
+    server_addresses: &BTreeMap<String, String>,
+) -> BTreeMap<String, String> {
+    let mut zoo_cfg = server_addresses.clone();
+    zoo_cfg.extend(build_base(cluster, rolegroup_config));
+    zoo_cfg
+}
+
 impl ValidatedCluster {
     /// Resolves the metrics HTTP port for the given role group, honoring a
     /// `metricsProvider.httpPort` `configOverride` if present.
-    ///
-    /// Defined here (in the build layer) rather than in `validate` so that resolving the port —
-    /// which renders the full `zoo.cfg` via [`build`] — does not invert the validate → build
-    /// dependency direction.
     pub fn metrics_http_port(&self, rolegroup_config: &ZookeeperRoleGroupConfig) -> Port {
-        build(self, rolegroup_config)
+        // The metrics port is independent of the server.<myid> quorum entries.
+        build_base(self, rolegroup_config)
             .get(METRICS_PROVIDER_HTTP_PORT_KEY)
             .and_then(|port| port.parse::<u16>().ok())
             .map(Port::from)
             .unwrap_or(METRICS_PROVIDER_HTTP_PORT)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::zk_controller::test_support::{cluster_info, minimal_zk, validated_cluster};
+
+    #[test]
+    fn server_addresses_predicts_one_entry_per_replica() {
+        let zk = minimal_zk(
+            r#"
+            apiVersion: zookeeper.stackable.tech/v1alpha1
+            kind: ZookeeperCluster
+            metadata:
+              name: test-zk
+            spec:
+              image:
+                productVersion: "3.9.5"
+              servers:
+                roleGroups:
+                  default:
+                    replicas: 3
+            "#,
+        );
+        let validated = validated_cluster(&zk);
+
+        let addresses = server_addresses(&validated, &cluster_info());
+
+        // One quorum entry per replica, keyed by `server.<myid>` (myidOffset default 1).
+        assert_eq!(addresses.len(), 3);
+        for myid in 1..=3 {
+            let entry = addresses
+                .get(&format!("server.{myid}"))
+                .unwrap_or_else(|| panic!("missing server.{myid}"));
+            // host:leader:election;client_port — default (non-TLS) client port.
+            assert!(entry.contains(":2888:3888;"), "unexpected entry: {entry}");
+        }
+    }
+}
diff --git a/rust/operator-binary/src/zk_controller/build/resource/config_map.rs b/rust/operator-binary/src/zk_controller/build/resource/config_map.rs
@@ -12,6 +12,7 @@ use stackable_operator::{
     builder::configmap::ConfigMapBuilder,
     k8s_openapi::api::core::v1::ConfigMap,
     product_logging::framework::VECTOR_CONFIG_FILE,
+    utils::cluster_info::KubernetesClusterInfo,
     v2::{
         config_file_writer::{PropertiesWriterError, to_java_properties_string},
         types::operator::RoleGroupName,
@@ -50,20 +51,23 @@ type Result<T, E = Error> = std::result::Result<T, E>;
 /// [`ResourceNames`]: stackable_operator::v2::role_group_utils::ResourceNames
 pub fn build_server_rolegroup_config_map(
     cluster: &ValidatedCluster,
+    cluster_info: &KubernetesClusterInfo,
     role_group_name: &RoleGroupName,
     rolegroup_config: &ZookeeperRoleGroupConfig,
 ) -> Result<ConfigMap> {
     let mut data: BTreeMap<String, String> = BTreeMap::new();
 
     // zoo.cfg
+    let server_addresses = zoo_cfg::server_addresses(cluster, cluster_info);
     data.insert(
         ConfigFileName::ZooCfg.to_string(),
-        to_java_properties_string(zoo_cfg::build(cluster, rolegroup_config).iter()).with_context(
-            |_| SerializePropertiesSnafu {
-                file: ConfigFileName::ZooCfg.to_string(),
-                role_group: role_group_name.clone(),
-            },
-        )?,
+        to_java_properties_string(
+            zoo_cfg::build(cluster, rolegroup_config, &server_addresses).iter(),
+        )
+        .with_context(|_| SerializePropertiesSnafu {
+            file: ConfigFileName::ZooCfg.to_string(),
+            role_group: role_group_name.clone(),
+        })?,
     );
 
     // security.properties
diff --git a/rust/operator-binary/src/zk_controller/validate.rs b/rust/operator-binary/src/zk_controller/validate.rs
@@ -28,7 +28,6 @@ use stackable_operator::{
     product_logging::spec::Logging,
     role_utils::RoleGroup,
     shared::time::Duration,
-    utils::cluster_info::KubernetesClusterInfo,
     v2::{
         HasName, HasUid, NameIsValidLabelValue,
         builder::{
@@ -61,8 +60,7 @@ use strum::IntoEnumIterator;
 
 use crate::{
     crd::{
-        APP_NAME, CONTAINER_IMAGE_BASE_NAME, OPERATOR_NAME, ZOOKEEPER_ELECTION_PORT,
-        ZOOKEEPER_LEADER_PORT, ZookeeperPodRef, ZookeeperRole, ZookeeperServerRoleType,
+        APP_NAME, CONTAINER_IMAGE_BASE_NAME, OPERATOR_NAME, ZookeeperRole, ZookeeperServerRoleType,
         authentication, default_listener_class,
         security::ZookeeperSecurity,
         v1alpha1::{self, ZookeeperConfig, ZookeeperConfigOverrides, ZookeeperServerRoleConfig},
@@ -444,10 +442,6 @@ impl Resource for ValidatedCluster {
 pub struct ValidatedClusterConfig {
     pub zookeeper_security: ZookeeperSecurity,
 
-    /// The `server.<myid>` entries for `zoo.cfg`, precomputed from the expected
-    /// pods so the ConfigMap builder does not need the cluster object.
-    pub server_addresses: BTreeMap<String, String>,
-
     /// The ListenerClass used to expose the ZooKeeper servers, so the listener builder does not
     /// need the raw cluster object.
     pub listener_class: ListenerClassName,
@@ -464,7 +458,6 @@ pub fn validate(
     zk: &v1alpha1::ZookeeperCluster,
     dereferenced_objects: &DereferencedObjects,
     operator_environment: &OperatorEnvironmentOptions,
-    cluster_info: &KubernetesClusterInfo,
 ) -> Result<ValidatedCluster> {
     let image = zk
         .spec
@@ -531,14 +524,6 @@ pub fn validate(
         .map(|role| role.role_config.listener_class.clone())
         .unwrap_or_else(|_| default_listener_class());
 
-    let server_addresses = server_addresses(
-        &name,
-        &namespace,
-        &role_group_configs,
-        &zookeeper_security,
-        cluster_info,
-    );
-
     let role_config = zk.role_config(&ZookeeperRole::Server).map(
         |ZookeeperServerRoleConfig { common, .. }| ValidatedRoleConfig {
             pdb: common.pod_disruption_budget.clone(),
@@ -553,7 +538,6 @@ pub fn validate(
         product_version,
         ValidatedClusterConfig {
             zookeeper_security,
-            server_addresses,
             listener_class,
         },
         role_config,
@@ -610,52 +594,6 @@ fn validate_role_group_config(
     })
 }
 
-/// Builds the `server.<myid>` quorum entries for `zoo.cfg` from the expected pods.
-///
-/// The pods are predicted from the validated role-group configs (`replicas` + `myidOffset`)
-/// rather than from the live cluster state, to avoid instance churn.
-fn server_addresses(
-    cluster_name: &ClusterName,
-    namespace: &NamespaceName,
-    role_group_configs: &BTreeMap<ZookeeperRole, BTreeMap<RoleGroupName, ZookeeperRoleGroupConfig>>,
-    zookeeper_security: &ZookeeperSecurity,
-    cluster_info: &KubernetesClusterInfo,
-) -> BTreeMap<String, String> {
-    let mut server_addresses = BTreeMap::new();
-    for (rg_name, rg_config) in role_group_configs
-        .get(&ZookeeperRole::Server)
-        .into_iter()
-        .flatten()
-    {
-        let resource_names = ResourceNames {
-            cluster_name: cluster_name.clone(),
-            role_name: ValidatedCluster::role_name(),
-            role_group_name: rg_name.clone(),
-        };
-        let headless_service_name = resource_names.headless_service_name();
-        let stateful_set_name = resource_names.stateful_set_name().to_string();
-        // An unset replica count (HPA-managed) predicts a single-server quorum entry, matching
-        // the historical default.
-        for i in 0..rg_config.replicas.unwrap_or(1) {
-            let pod_ref = ZookeeperPodRef {
-                namespace: namespace.clone(),
-                role_group_headless_service_name: headless_service_name.clone(),
-                pod_name: format!("{stateful_set_name}-{i}"),
-                zookeeper_myid: i + rg_config.config.myid_offset,
-            };
-            server_addresses.insert(
-                format!("server.{id}", id = pod_ref.zookeeper_myid),
-                format!(
-                    "{internal_fqdn}:{ZOOKEEPER_LEADER_PORT}:{ZOOKEEPER_ELECTION_PORT};{client_port}",
-                    internal_fqdn = pod_ref.internal_fqdn(cluster_info),
-                    client_port = zookeeper_security.client_port()
-                ),
-            );
-        }
-    }
-    server_addresses
-}
-
 #[cfg(test)]
 mod tests {
     use stackable_operator::k8s_openapi::apimachinery::pkg::api::resource::Quantity;
