Skip to content

Commit c4be308

Browse files
maltesanderadwk67
andauthored
refactor: Extract build and apply steps in the zk- and znode-controller (#1069)
* refactor: Build the discovery ConfigMap in the build step The discovery ConfigMap advertises the addresses that the listener operator publishes on the ZooKeeper role Listener, so it used to be built and applied inline in reconcile_zk, from the Listener that had just been applied. That Listener carries no addresses yet on the first reconciliation, so the controller relied on the reconciliation failing and being requeued five seconds later. Follow the pattern the OpenSearch operator already uses: dereference the role Listener, validate its addresses into the ValidatedCluster, and let build() emit an Option<ConfigMap>. The controller now watches Listeners, so the reconciliation that writes the discovery ConfigMap is triggered as soon as the addresses appear, and it also reruns when they change. The listener address extraction moves into a shared listener_addresses module, because the ZookeeperZnode controller needs it too. The operator ClusterRole gains the watch verb on listeners, which is required for the new watch. * refactor: Extract the apply step into an Applier reconcile_zk built the ClusterResources itself and applied every collection with its own inline loop, so the driver carried the apply order, the orphan deletion and the resource specific error variants. Extract all of that into an Applier, following the airflow and hbase operators. KubernetesResources is now marked as either Prepared or Applied, which makes it impossible to derive the cluster status from resources that were only built. apply() destructures the resource set exhaustively, so a new field fails to compile here instead of silently never being applied. Unlike the sibling operators, the apply module is declared in zk_controller.rs itself, so the Applier is imported without `self` to avoid a name collision with the module declaration. * refactor: Extract the update status step The cluster conditions and the discovery hash were computed inline at the end of reconcile_zk, from resources that happened to be in scope. Move both into an update_status step, following the airflow and hbase operators. It takes KubernetesResources<Applied>, so the type system proves the status is derived from resources that were actually applied rather than merely built. The discovery hash, which the sibling operators do not have, becomes a private helper next to it. reconcile_zk is now the dereference, validate, build, apply and update_status pipeline and nothing else. * refactor: Extract build and apply steps in the znode controller reconcile_apply created the ClusterResources, talked to ZooKeeper, fetched the role Listener, built the discovery ConfigMap and applied it, all inline, so the ZookeeperZnode controller was the only one left without a pipeline. Give it the same dereference, validate, build and apply structure as the ZookeeperCluster controller. The Listener fetch moves into the dereference step, where a missing Listener stays a non error so it can never block finalizer removal, and its addresses become a validated field on ValidatedZnode. Creating the znode inside the ZooKeeper ensemble lives in the apply step as a free function, next to the Applier, because it is a client side effect that the client free build() cannot perform. The discovery ConfigMap builders move to a shared discovery module, so the znode controller no longer reaches into the cluster controller's build tree. Unlike the cluster controller, the resources carry no Prepared or Applied marker: the ZookeeperZnode has no cluster conditions, so there is no status step the marker could protect. * chore: adapted changelog * docs: fix rustdoc warnings in discovery and znode apply * fix: Always build the discovery ConfigMap Skipping the discovery ConfigMap while the role Listener publishes no addresses let the apply step delete the already published one as an orphan, breaking consumers that mount it. Before the pipeline extraction, the missing addresses aborted the run before delete_orphaned_resources() ran, so this only surfaced now. Follow the kafka-operator pattern instead: always write the ConfigMap, with an empty ZOOKEEPER value while no address is known. The Listener watch triggers a new run that fills the value in. * test: Cover the znode validate step's role Listener errors The znode controller had no tests at all. Add fixtures for it and cover both ways the role Listener can block validation: addresses published under an unexpected port name (ReadRoleListenerAddresses), and no addresses at all, either because the Listener does not exist yet or because the listener operator has not published its status (NoRoleListenerAddresses). The Listener fixtures move into a shared test_support module in listener_addresses, so the cluster controller's tests can use them too. * test: Cover the role Listener addresses in the cluster validate step Add a try_validate variant that takes a role Listener, and cover the three outcomes: addresses published under an unexpected port name fail with ReadRoleListenerAddresses, published addresses reach the validated cluster sorted, and a missing or unpublished Listener validates to no addresses rather than an error, which is what lets the build step still write the discovery ConfigMap. * test: Cover the discovery hash in the update status step * test: Cover the discovery ConfigMap builders Assert the ZOOKEEPER, ZOOKEEPER_HOSTS, ZOOKEEPER_CLIENT_PORT and ZOOKEEPER_CHROOT content for both the cluster and the znode variant, that only ZOOKEEPER carries the chroot, and that a relative chroot is rejected. The empty-address case is covered too, pinning that the ConfigMap is written even while the role Listener publishes nothing. The Listener fixtures now publish the secure client port, which is what the role Listener serves while the fixture keeps TLS enabled. * Update CHANGELOG.md Co-authored-by: Andrew Kenworthy <1712947+adwk67@users.noreply.github.com> --------- Co-authored-by: Andrew Kenworthy <1712947+adwk67@users.noreply.github.com>
1 parent b791a3c commit c4be308

18 files changed

Lines changed: 1470 additions & 571 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@ All notable changes to this project will be documented in this file.
1717
functions and carry the full set of recommended labels ([#1060]).
1818
- BREAKING: The `servers` role is now required by the CRD.
1919
Previously a ZookeeperCluster without it was accepted by the API server but failed reconciliation ([#1060]).
20+
- The reconciler now applies resources and derives the cluster status in discrete
21+
apply and update_status steps for the `zk_controller` and `znode_controller` ([#1069]).
22+
- The discovery ConfigMap is now always written, with empty `ZOOKEEPER` and `ZOOKEEPER_HOSTS` while
23+
the listener publishes no addresses ([#1069]).
2024
- All product containers now run with `securityContext.runAsNonRoot` set to `true` to improve security ([#1070]).
2125

2226
### Fixed
@@ -29,6 +33,7 @@ All notable changes to this project will be documented in this file.
2933
[#1060]: https://github.com/stackabletech/zookeeper-operator/pull/1060
3034
[#1063]: https://github.com/stackabletech/zookeeper-operator/pull/1063
3135
[#1068]: https://github.com/stackabletech/zookeeper-operator/pull/1068
36+
[#1069]: https://github.com/stackabletech/zookeeper-operator/pull/1069
3237
[#1070]: https://github.com/stackabletech/zookeeper-operator/pull/1070
3338

3439
## [26.7.0] - 2026-07-21

‎deploy/helm/zookeeper-operator/templates/clusterrole-operator.yaml‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,9 @@ rules:
118118
verbs:
119119
- create
120120
- patch
121-
# Listener created per role group for external access. Applied via SSA and tracked for
122-
# orphan cleanup.
121+
# Listener created per role for external access. Applied via SSA and tracked for
122+
# orphan cleanup. Watched as well, because the discovery ConfigMap advertises the addresses
123+
# that the listener operator publishes on it.
123124
- apiGroups:
124125
- listeners.stackable.tech
125126
resources:
@@ -130,6 +131,7 @@ rules:
130131
- get
131132
- list
132133
- patch
134+
- watch
133135
# Primary CRD: watched and read during reconciliation.
134136
- apiGroups:
135137
- {{ include "operator.name" . }}.stackable.tech
Lines changed: 325 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,325 @@
1+
//! Builders for the discovery ConfigMaps, which advertise how to connect to a ZooKeeper ensemble.
2+
//!
3+
//! Shared by the build steps of both controllers: the ZookeeperCluster controller publishes the
4+
//! whole ensemble, the ZookeeperZnode controller publishes the same ensemble narrowed to a chroot.
5+
6+
use std::str::FromStr;
7+
8+
use snafu::{ResultExt, Snafu};
9+
use stackable_operator::{
10+
builder::{configmap::ConfigMapBuilder, meta::ObjectMetaBuilder},
11+
k8s_openapi::api::core::v1::ConfigMap,
12+
kube::Resource,
13+
v2::{
14+
HasName, HasUid, NameIsValidLabelValue,
15+
builder::meta::ownerreference_from_resource,
16+
kvp::label::recommended_labels,
17+
types::operator::{ControllerName, ProductVersion, RoleGroupName},
18+
},
19+
};
20+
21+
use crate::{
22+
crd::{ZookeeperRole, security::ZookeeperSecurity},
23+
listener_addresses::ListenerAddresses,
24+
zk_controller::validate::{ValidatedCluster, operator_name, product_name},
25+
znode_controller::validate::ValidatedZnode,
26+
};
27+
28+
// Placeholder role-group name used for the recommended labels of the role-level discovery
29+
// `ConfigMap` (which is not tied to a single role group).
30+
stackable_operator::constant!(PLACEHOLDER_DISCOVERY_ROLE_GROUP: RoleGroupName = "discovery");
31+
32+
type Result<T, E = Error> = std::result::Result<T, E>;
33+
34+
#[derive(Snafu, Debug)]
35+
pub enum Error {
36+
#[snafu(display("chroot path {} was relative (must be absolute)", chroot))]
37+
RelativeChroot { chroot: String },
38+
39+
#[snafu(display("failed to build ConfigMap"))]
40+
BuildConfigMap {
41+
source: stackable_operator::builder::configmap::Error,
42+
},
43+
}
44+
45+
/// Build the discovery [`ConfigMap`] for the cluster controller from the
46+
/// [`ValidatedCluster`].
47+
///
48+
/// The ConfigMap is owned by, and placed in the namespace of, the cluster. The image and security
49+
/// settings are taken from the [`ValidatedCluster`] rather than being passed in separately.
50+
///
51+
/// The connection details are read from the addresses published by the role Listener (carried on
52+
/// [`ValidatedCluster::discovery_addresses`](ValidatedCluster#structfield.discovery_addresses),
53+
/// fetched in the dereference step), which only the listener operator writes. While no address
54+
/// exists around the first reconciliations, which create the Listener in the first place, the
55+
/// ConfigMap is still written, with an empty `ZOOKEEPER` value: omitting it instead would let the
56+
/// apply step delete an existing discovery ConfigMap as an orphan, breaking consumers that mount
57+
/// it. The Listener watch triggers a new run that fills in the value once the addresses are
58+
/// published.
59+
pub fn build_discovery_configmap(
60+
validated_cluster: &ValidatedCluster,
61+
controller_name: &str,
62+
listener_addresses: &ListenerAddresses,
63+
) -> Result<ConfigMap> {
64+
build_discovery_configmap_for_owner(
65+
validated_cluster,
66+
&validated_cluster.namespace,
67+
controller_name,
68+
&validated_cluster.product_version,
69+
listener_addresses,
70+
None,
71+
&validated_cluster.cluster_config.zookeeper_security,
72+
)
73+
}
74+
75+
/// Build the discovery [`ConfigMap`] for the znode controller.
76+
///
77+
/// The ConfigMap is owned by, and placed in the namespace of, the
78+
/// [`ValidatedZnode`]. The product version and `zookeeper_security` originate from the referenced
79+
/// cluster (via the validated znode), while `chroot` isolates the znode within the shared ZooKeeper
80+
/// ensemble.
81+
pub fn build_znode_discovery_configmap(
82+
validated_znode: &ValidatedZnode,
83+
controller_name: &str,
84+
listener_addresses: &ListenerAddresses,
85+
chroot: &str,
86+
) -> Result<ConfigMap> {
87+
build_discovery_configmap_for_owner(
88+
validated_znode,
89+
&validated_znode.namespace,
90+
controller_name,
91+
&validated_znode.product_version,
92+
listener_addresses,
93+
Some(chroot),
94+
&validated_znode.zookeeper_security,
95+
)
96+
}
97+
98+
/// Build a discovery [`ConfigMap`] containing ZooKeeper connection details from the
99+
/// [`ListenerAddresses`] published by the role Listener.
100+
///
101+
/// `owner` owns the ConfigMap (the [`ZookeeperCluster`](crate::crd::v1alpha1::ZookeeperCluster) for the cluster
102+
/// controller, or the [`ZookeeperZnode`](crate::crd::v1alpha1::ZookeeperZnode) for the znode controller) and
103+
/// `namespace` is where the ConfigMap is placed.
104+
fn build_discovery_configmap_for_owner(
105+
owner: &(impl Resource<DynamicType = ()> + HasName + HasUid + NameIsValidLabelValue),
106+
namespace: impl Into<String>,
107+
controller_name: &str,
108+
product_version: &ProductVersion,
109+
listener_addresses: &ListenerAddresses,
110+
chroot: Option<&str>,
111+
zookeeper_security: &ZookeeperSecurity,
112+
) -> Result<ConfigMap> {
113+
let name = owner.to_name();
114+
115+
// The discovery ConfigMap is a role-level resource of the `server` role, conventionally
116+
// labelled with the `discovery` role group. The controller name differs between the cluster and
117+
// znode controllers, so it is passed in and validated into the type-safe newtype here.
118+
let controller_name = ControllerName::from_str(controller_name)
119+
.expect("the controller name is a valid label value");
120+
let role_group_name = PLACEHOLDER_DISCOVERY_ROLE_GROUP.clone();
121+
122+
// Write a connection string of the format that Java ZooKeeper client expects:
123+
// "{host1}:{port1},{host2:port2},.../{chroot}"
124+
// See https://zookeeper.apache.org/doc/current/apidocs/zookeeper-server/org/apache/zookeeper/ZooKeeper.html#ZooKeeper-java.lang.String-int-org.apache.zookeeper.Watcher-
125+
let listener_addresses = listener_addresses.to_connection_string();
126+
let mut conn_str = listener_addresses.clone();
127+
if let Some(chroot) = chroot {
128+
if !chroot.starts_with('/') {
129+
return RelativeChrootSnafu { chroot }.fail();
130+
}
131+
conn_str.push_str(chroot);
132+
}
133+
ConfigMapBuilder::new()
134+
.metadata(
135+
ObjectMetaBuilder::new()
136+
.name(name)
137+
.namespace(namespace)
138+
.ownerreference(ownerreference_from_resource(owner, None, Some(true)))
139+
.with_labels(recommended_labels(
140+
owner,
141+
&product_name(),
142+
product_version,
143+
&operator_name(),
144+
&controller_name,
145+
&ZookeeperRole::Server.into(),
146+
&role_group_name,
147+
))
148+
.build(),
149+
)
150+
.add_data("ZOOKEEPER", conn_str)
151+
// Some clients don't support ZooKeeper's merged `hosts/chroot` format, so export them separately for these clients
152+
.add_data("ZOOKEEPER_HOSTS", listener_addresses)
153+
.add_data(
154+
"ZOOKEEPER_CLIENT_PORT",
155+
zookeeper_security.client_port().to_string(),
156+
)
157+
.add_data("ZOOKEEPER_CHROOT", chroot.unwrap_or("/"))
158+
.build()
159+
.context(BuildConfigMapSnafu)
160+
}
161+
162+
#[cfg(test)]
163+
mod tests {
164+
use super::*;
165+
use crate::{
166+
crd::ZOOKEEPER_SERVER_PORT_NAME,
167+
listener_addresses::{
168+
listener_addresses,
169+
test_support::{ingress_address, role_listener},
170+
},
171+
zk_controller::{
172+
ZK_CONTROLLER_NAME,
173+
test_support::{minimal_zk, try_validate_with_role_listener},
174+
},
175+
znode_controller::{
176+
ZNODE_CONTROLLER_NAME,
177+
test_support::{minimal_znode, validated_znode},
178+
},
179+
};
180+
181+
const ZK_YAML: &str = r#"
182+
apiVersion: zookeeper.stackable.tech/v1alpha1
183+
kind: ZookeeperCluster
184+
metadata:
185+
name: simple-zookeeper
186+
spec:
187+
image:
188+
productVersion: "3.9.5"
189+
servers:
190+
roleGroups:
191+
default:
192+
replicas: 3
193+
"#;
194+
195+
const ZNODE_YAML: &str = r#"
196+
apiVersion: zookeeper.stackable.tech/v1alpha1
197+
kind: ZookeeperZnode
198+
metadata:
199+
name: simple-znode
200+
spec:
201+
clusterRef:
202+
name: simple-zookeeper
203+
"#;
204+
205+
/// The znode path that the znode controller derives from the fixture's UID.
206+
const ZNODE_PATH: &str = "/znode-e5dbf9c2-d8b0-4c1e-9f4a-1d2e3f4a5b6c";
207+
208+
/// The value of `key` in the given discovery ConfigMap.
209+
fn data(config_map: &ConfigMap, key: &str) -> String {
210+
config_map
211+
.data
212+
.as_ref()
213+
.expect("the discovery ConfigMap should carry data")
214+
.get(key)
215+
.unwrap_or_else(|| panic!("the discovery ConfigMap should carry {key}"))
216+
.clone()
217+
}
218+
219+
/// Addresses published under the ZooKeeper server port name, built through the real reader so
220+
/// the fixtures cannot drift from what the validate step produces.
221+
fn published_addresses(addresses: &[(&str, u16)]) -> ListenerAddresses {
222+
let listener = role_listener(Some(
223+
addresses
224+
.iter()
225+
.map(|(address, port)| {
226+
ingress_address(address, ZOOKEEPER_SERVER_PORT_NAME, i32::from(*port))
227+
})
228+
.collect(),
229+
));
230+
231+
listener_addresses(&listener, ZOOKEEPER_SERVER_PORT_NAME)
232+
.expect("the fixture publishes addresses under the server port name")
233+
.expect("the fixture publishes addresses")
234+
}
235+
236+
/// The cluster controller advertises the whole ensemble, rooted at `/`. The fixture keeps TLS
237+
/// enabled, so the client port is the secure one.
238+
#[test]
239+
fn cluster_discovery_config_map_advertises_the_published_addresses() {
240+
let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None)
241+
.expect("validate should succeed for the test fixture");
242+
let addresses = published_addresses(&[("node-0", 2282), ("node-1", 2282)]);
243+
244+
let config_map =
245+
build_discovery_configmap(&cluster, ZK_CONTROLLER_NAME, &addresses).expect("build");
246+
247+
assert_eq!(
248+
config_map.metadata.name.as_deref(),
249+
Some("simple-zookeeper")
250+
);
251+
assert_eq!(config_map.metadata.namespace.as_deref(), Some("default"));
252+
assert_eq!(data(&config_map, "ZOOKEEPER"), "node-0:2282,node-1:2282");
253+
assert_eq!(
254+
data(&config_map, "ZOOKEEPER_HOSTS"),
255+
"node-0:2282,node-1:2282"
256+
);
257+
assert_eq!(data(&config_map, "ZOOKEEPER_CLIENT_PORT"), "2282");
258+
assert_eq!(data(&config_map, "ZOOKEEPER_CHROOT"), "/");
259+
}
260+
261+
/// While the role Listener publishes no addresses the ConfigMap is still written, with an
262+
/// empty connection string, so that the apply step does not delete the published one as an
263+
/// orphan.
264+
#[test]
265+
fn cluster_discovery_config_map_without_addresses_is_still_written() {
266+
let cluster = try_validate_with_role_listener(&minimal_zk(ZK_YAML), None)
267+
.expect("validate should succeed for the test fixture");
268+
269+
let config_map =
270+
build_discovery_configmap(&cluster, ZK_CONTROLLER_NAME, &ListenerAddresses::default())
271+
.expect("build");
272+
273+
assert_eq!(
274+
config_map.metadata.name.as_deref(),
275+
Some("simple-zookeeper")
276+
);
277+
assert_eq!(data(&config_map, "ZOOKEEPER"), "");
278+
assert_eq!(data(&config_map, "ZOOKEEPER_HOSTS"), "");
279+
// The port and chroot do not depend on the addresses, so they stay populated.
280+
assert_eq!(data(&config_map, "ZOOKEEPER_CLIENT_PORT"), "2282");
281+
assert_eq!(data(&config_map, "ZOOKEEPER_CHROOT"), "/");
282+
}
283+
284+
/// The znode controller advertises the same ensemble, narrowed to the znode's chroot. Only
285+
/// `ZOOKEEPER` carries the chroot, because some clients cannot parse the merged format.
286+
#[test]
287+
fn znode_discovery_config_map_narrows_the_ensemble_to_the_chroot() {
288+
let znode = validated_znode(&minimal_znode(ZNODE_YAML));
289+
290+
let config_map = build_znode_discovery_configmap(
291+
&znode,
292+
ZNODE_CONTROLLER_NAME,
293+
&znode.discovery_addresses,
294+
ZNODE_PATH,
295+
)
296+
.expect("build");
297+
298+
// The ConfigMap is named after the znode, not after the referenced cluster.
299+
assert_eq!(config_map.metadata.name.as_deref(), Some("simple-znode"));
300+
assert_eq!(config_map.metadata.namespace.as_deref(), Some("default"));
301+
assert_eq!(
302+
data(&config_map, "ZOOKEEPER"),
303+
format!("node-0:2282{ZNODE_PATH}")
304+
);
305+
assert_eq!(data(&config_map, "ZOOKEEPER_HOSTS"), "node-0:2282");
306+
assert_eq!(data(&config_map, "ZOOKEEPER_CLIENT_PORT"), "2282");
307+
assert_eq!(data(&config_map, "ZOOKEEPER_CHROOT"), ZNODE_PATH);
308+
}
309+
310+
/// A relative chroot would silently produce a connection string pointing at the ensemble root.
311+
#[test]
312+
fn relative_chroot_is_rejected() {
313+
let znode = validated_znode(&minimal_znode(ZNODE_YAML));
314+
315+
assert!(matches!(
316+
build_znode_discovery_configmap(
317+
&znode,
318+
ZNODE_CONTROLLER_NAME,
319+
&znode.discovery_addresses,
320+
"znode-without-a-leading-slash",
321+
),
322+
Err(Error::RelativeChroot { .. })
323+
));
324+
}
325+
}

0 commit comments

Comments
 (0)