refactor: replace 14-assert cm discovery checks

Author: sweb

rust/operator-binary/src/zk_controller/build/resource/discovery.rs

@@ -225,6 +225,61 @@ mod tests {
     };
 
     use super::*;
+    use crate::{
+        zk_controller::{
+            ZK_CONTROLLER_NAME,
+            test_support::{minimal_zk, validated_cluster},
+        },
+        znode_controller::{
+            ZNODE_CONTROLLER_NAME,
+            validate::test_support::{minimal_znode, validated_znode},
+        },
+    };
+
+    /// A `ZookeeperCluster` fixture. `test_support`'s auth-free `new_for_tests()` still enables
+    /// server TLS, so this resolves to the secure client port (2282).
+    const TLS_ZK: &str = r#"
+        apiVersion: zookeeper.stackable.tech/v1alpha1
+        kind: ZookeeperCluster
+        metadata:
+          name: test-zk
+        spec:
+          image:
+            productVersion: "3.9.5"
+          servers:
+            roleGroups:
+              default:
+                replicas: 3
+        "#;
+
+    /// Same as [`TLS_ZK`] but with `serverSecretClass: null`, disabling server TLS so the insecure
+    /// client port (2181) is used.
+    const NON_TLS_ZK: &str = r#"
+        apiVersion: zookeeper.stackable.tech/v1alpha1
+        kind: ZookeeperCluster
+        metadata:
+          name: test-zk
+        spec:
+          image:
+            productVersion: "3.9.5"
+          clusterConfig:
+            tls:
+              serverSecretClass: null
+          servers:
+            roleGroups:
+              default:
+                replicas: 3
+        "#;
+
+    const ZNODE: &str = r#"
+        apiVersion: zookeeper.stackable.tech/v1alpha1
+        kind: ZookeeperZnode
+        metadata:
+          name: test-znode
+        spec:
+          clusterRef:
+            name: test-zk
+        "#;
 
     fn listener(ingress_addresses: Option<Vec<ListenerIngress>>) -> Listener {
         Listener {
@@ -241,14 +296,26 @@ mod tests {
         }
     }
 
-    fn ingress(port: i32) -> ListenerIngress {
+    fn ingress_at(address: &str, port: i32) -> ListenerIngress {
         ListenerIngress {
-            address: "node-0".to_owned(),
+            address: address.to_owned(),
             address_type: AddressType::Hostname,
             ports: BTreeMap::from([(ZOOKEEPER_SERVER_PORT_NAME.to_owned(), port)]),
         }
     }
 
+    fn ingress(port: i32) -> ListenerIngress {
+        ingress_at("node-0", port)
+    }
+
+    /// Pulls a `.data` key out of a built ConfigMap.
+    fn data<'a>(cm: &'a ConfigMap, key: &str) -> &'a str {
+        cm.data
+            .as_ref()
+            .and_then(|d| d.get(key))
+            .unwrap_or_else(|| panic!("discovery ConfigMap missing key {key}"))
+    }
+
     #[test]
     fn listener_addresses_returns_host_port_pairs() {
         let listener = listener(Some(vec![ingress(2181)]));
@@ -285,4 +352,91 @@ mod tests {
             Err(Error::InvalidPort { .. })
         ));
     }
+
+    #[test]
+    fn cluster_discovery_cm_data_tls() {
+        // TLS cluster -> secure client port 2282. The listener exposes the same port, as it does at
+        // runtime. No chroot, so `ZOOKEEPER` equals `ZOOKEEPER_HOSTS` and `ZOOKEEPER_CHROOT` is `/`.
+        let validated = validated_cluster(&minimal_zk(TLS_ZK));
+        let cm = build_discovery_configmap(
+            &validated,
+            ZK_CONTROLLER_NAME,
+            listener(Some(vec![ingress(2282)])),
+        )
+        .expect("discovery CM builds");
+
+        assert_eq!(data(&cm, "ZOOKEEPER"), "node-0:2282");
+        assert_eq!(data(&cm, "ZOOKEEPER_HOSTS"), "node-0:2282");
+        assert_eq!(data(&cm, "ZOOKEEPER_CLIENT_PORT"), "2282");
+        assert_eq!(data(&cm, "ZOOKEEPER_CHROOT"), "/");
+    }
+
+    #[test]
+    fn cluster_discovery_cm_data_non_tls() {
+        // Server TLS disabled -> insecure client port 2181.
+        let validated = validated_cluster(&minimal_zk(NON_TLS_ZK));
+        let cm = build_discovery_configmap(
+            &validated,
+            ZK_CONTROLLER_NAME,
+            listener(Some(vec![ingress(2181)])),
+        )
+        .expect("discovery CM builds");
+
+        assert_eq!(data(&cm, "ZOOKEEPER"), "node-0:2181");
+        assert_eq!(data(&cm, "ZOOKEEPER_HOSTS"), "node-0:2181");
+        assert_eq!(data(&cm, "ZOOKEEPER_CLIENT_PORT"), "2181");
+        assert_eq!(data(&cm, "ZOOKEEPER_CHROOT"), "/");
+    }
+
+    #[test]
+    fn cluster_discovery_cm_joins_multiple_hosts_sorted() {
+        // Multiple listener ingress addresses are joined into one comma-separated connection string,
+        // ordered by the `BTreeSet` in `listener_addresses` (i.e. sorted).
+        let validated = validated_cluster(&minimal_zk(TLS_ZK));
+        let cm = build_discovery_configmap(
+            &validated,
+            ZK_CONTROLLER_NAME,
+            listener(Some(vec![
+                ingress_at("node-1", 2282),
+                ingress_at("node-0", 2282),
+            ])),
+        )
+        .expect("discovery CM builds");
+
+        assert_eq!(data(&cm, "ZOOKEEPER"), "node-0:2282,node-1:2282");
+        assert_eq!(data(&cm, "ZOOKEEPER_HOSTS"), "node-0:2282,node-1:2282");
+    }
+
+    #[test]
+    fn znode_discovery_cm_appends_chroot_to_connection_string_only() {
+        // The chroot is appended to `ZOOKEEPER` (the merged Java client string) but NOT to
+        // `ZOOKEEPER_HOSTS` (kept separate for clients that don't understand the merged format).
+        let validated = validated_znode(&minimal_znode(ZNODE), TLS_ZK);
+        let cm = build_znode_discovery_configmap(
+            &validated,
+            ZNODE_CONTROLLER_NAME,
+            listener(Some(vec![ingress(2282)])),
+            "/znode-abc",
+        )
+        .expect("znode discovery CM builds");
+
+        assert_eq!(data(&cm, "ZOOKEEPER"), "node-0:2282/znode-abc");
+        assert_eq!(data(&cm, "ZOOKEEPER_HOSTS"), "node-0:2282");
+        assert_eq!(data(&cm, "ZOOKEEPER_CLIENT_PORT"), "2282");
+        assert_eq!(data(&cm, "ZOOKEEPER_CHROOT"), "/znode-abc");
+    }
+
+    #[test]
+    fn znode_discovery_cm_rejects_relative_chroot() {
+        let validated = validated_znode(&minimal_znode(ZNODE), TLS_ZK);
+        assert!(matches!(
+            build_znode_discovery_configmap(
+                &validated,
+                ZNODE_CONTROLLER_NAME,
+                listener(Some(vec![ingress(2282)])),
+                "znode-abc",
+            ),
+            Err(Error::RelativeChroot { .. })
+        ));
+    }
 }

rust/operator-binary/src/znode_controller/validate.rs

@@ -200,3 +200,58 @@ pub fn validate(
         object_overrides: znode.spec.object_overrides.clone(),
     })
 }
+
+/// Shared helpers for building validated test znodes from minimal YAML fixtures.
+///
+/// Mirrors `zk_controller::test_support`: rather than hand-constructing a [`ValidatedZnode`]
+/// (whose `metadata` is private), tests run the real [`validate`] step so the wiring under test
+/// (product version, `zookeeper_security`, identity) matches production.
+#[cfg(test)]
+pub(crate) mod test_support {
+    use stackable_operator::cli::OperatorEnvironmentOptions;
+
+    use super::{ValidatedZnode, validate};
+    use crate::{
+        crd::{authentication::DereferencedAuthenticationClasses, v1alpha1},
+        zk_controller::test_support::minimal_zk,
+        znode_controller::dereference::DereferencedObjects,
+    };
+
+    /// Parses a minimal `ZookeeperZnode` test fixture, defaulting `namespace`/`uid` so the
+    /// validate step can build a [`ValidatedZnode`].
+    pub fn minimal_znode(yaml: &str) -> v1alpha1::ZookeeperZnode {
+        let mut znode: v1alpha1::ZookeeperZnode =
+            serde_yaml::from_str(yaml).expect("invalid test ZookeeperZnode YAML");
+        znode
+            .metadata
+            .namespace
+            .get_or_insert_with(|| "default".to_owned());
+        znode
+            .metadata
+            .uid
+            .get_or_insert_with(|| "5f1d9a2e-3c4b-4a1e-9b7d-2e6f8c0a1b3c".to_owned());
+        znode
+    }
+
+    fn operator_environment() -> OperatorEnvironmentOptions {
+        OperatorEnvironmentOptions {
+            operator_namespace: "stackable-operators".to_owned(),
+            operator_service_name: "zookeeper-operator".to_owned(),
+            image_repository: "oci.example.org".to_owned(),
+        }
+    }
+
+    /// Runs the real znode validate step against a minimal znode fixture and the given
+    /// `ZookeeperCluster` YAML (which controls product version and `zookeeper_security`).
+    pub fn validated_znode(znode: &v1alpha1::ZookeeperZnode, zk_yaml: &str) -> ValidatedZnode {
+        validate(
+            znode,
+            &DereferencedObjects {
+                zk: minimal_zk(zk_yaml),
+                authentication_classes: DereferencedAuthenticationClasses::new_for_tests(),
+            },
+            &operator_environment(),
+        )
+        .expect("znode validate should succeed for the test fixture")
+    }
+}

tests/templates/kuttl/smoke/14-assert.yaml.j2

@@ -24,45 +24,33 @@ commands:
   # The Vector sidecar's env wiring is set by the upstream `vector_container` helper and its
   # presence on the StatefulSet is unit-tested (statefulset.rs
   # `vector_agent_adds_vector_container_to_statefulset`), so no runtime env assertion is needed here.
+  #
+  # Discovery ConfigMap content (`ZOOKEEPER`/`ZOOKEEPER_HOSTS`/`ZOOKEEPER_CHROOT`/
+  # `ZOOKEEPER_CLIENT_PORT`, incl. the znode chroot suffix and the secure-vs-insecure client port)
+  # is unit-tested by `build_(znode_)discovery_configmap` (discovery.rs). The checks below only guard
+  # what a unit test cannot: that the CMs actually reconciled with the real service-name/namespace
+  # wiring and carry non-empty values.
   - script: |
-      expected=$(cat <<'YAMLEOF' | sed "s|__NAMESPACE__|$NAMESPACE|g" | yq -o=json
-      ZOOKEEPER: test-zk-server.__NAMESPACE__.svc.cluster.local:{{ zk_client_port }}
-      ZOOKEEPER_CHROOT: /
-      ZOOKEEPER_CLIENT_PORT: "{{ zk_client_port }}"
-      ZOOKEEPER_HOSTS: test-zk-server.__NAMESPACE__.svc.cluster.local:{{ zk_client_port }}
-      YAMLEOF
-      )
-      actual=$(kubectl -n $NAMESPACE get cm test-zk -o yaml | yq -o=json '.data')
-      expected_file=$(mktemp) && actual_file=$(mktemp)
-      printf '%s\n' "$expected" > "$expected_file"
-      printf '%s\n' "$actual" > "$actual_file"
-      if ! diff_out=$(diff -u "$expected_file" "$actual_file"); then
-        echo "ERROR: ConfigMap test-zk data drifted from snapshot."
-        printf '%s\n' "$diff_out"
-        rm -f "$expected_file" "$actual_file"
-        exit 1
-      fi
-      rm -f "$expected_file" "$actual_file"
-  - script: |
-      ZNODE_UID=$(kubectl -n $NAMESPACE get zookeeperznode test-znode -o jsonpath='{.metadata.uid}')
-      expected=$(cat <<'YAMLEOF' | sed -e "s|__NAMESPACE__|$NAMESPACE|g" -e "s|__ZNODE_UID__|$ZNODE_UID|g" | yq -o=json
-      ZOOKEEPER: test-zk-server.__NAMESPACE__.svc.cluster.local:{{ zk_client_port }}/znode-__ZNODE_UID__
-      ZOOKEEPER_CHROOT: /znode-__ZNODE_UID__
-      ZOOKEEPER_CLIENT_PORT: "{{ zk_client_port }}"
-      ZOOKEEPER_HOSTS: test-zk-server.__NAMESPACE__.svc.cluster.local:{{ zk_client_port }}
-      YAMLEOF
-      )
-      actual=$(kubectl -n $NAMESPACE get cm test-znode -o yaml | yq -o=json '.data')
-      expected_file=$(mktemp) && actual_file=$(mktemp)
-      printf '%s\n' "$expected" > "$expected_file"
-      printf '%s\n' "$actual" > "$actual_file"
-      if ! diff_out=$(diff -u "$expected_file" "$actual_file"); then
-        echo "ERROR: ConfigMap test-znode data drifted from snapshot."
-        printf '%s\n' "$diff_out"
-        rm -f "$expected_file" "$actual_file"
-        exit 1
-      fi
-      rm -f "$expected_file" "$actual_file"
+      for cm in test-zk test-znode; do
+        data=$(kubectl -n $NAMESPACE get cm "$cm" -o yaml | yq -o=json '.data')
+        for key in ZOOKEEPER ZOOKEEPER_HOSTS ZOOKEEPER_CLIENT_PORT ZOOKEEPER_CHROOT; do
+          value=$(printf '%s' "$data" | yq -r ".$key // \"\"")
+          if [ -z "$value" ]; then
+            echo "ERROR: discovery ConfigMap $cm is missing/empty key $key."
+            printf '%s\n' "$data"
+            exit 1
+          fi
+        done
+        # The connection string must point at the cluster-internal Service in this namespace.
+        host=$(printf '%s' "$data" | yq -r '.ZOOKEEPER_HOSTS')
+        case "$host" in
+          test-zk-server.$NAMESPACE.svc.cluster.local:*) ;;
+          *)
+            echo "ERROR: discovery ConfigMap $cm ZOOKEEPER_HOSTS=$host does not target the expected Service."
+            exit 1
+            ;;
+        esac
+      done
   - script: |
       expected=$(cat <<'YAMLEOF' | sed "s|__NAMESPACE__|$NAMESPACE|g" | yq -o=json
       logback.xml: |