feat: Add /ready endpoint to WebhookServer

[xeniape]

Description

Part of stackabletech/issues#828

This PR adds a /ready endpoint to the WebhookServer to be used by startup probes in the operators. It was added to the WebhookServer as per requirements of the ticket above, for the future and if more readiness checks come up unrelated to the CRDs and/or WebhookServer, it might be reasonable to start a separate server instead. Currently, if no webhooks were defined, the ready endpoint also would not be served (not the case with the current operators, as they all have webhooks).

As part of this the crd_established signal was also adjusted (timeout removed) because the timeout there would bite with the probes configuration. Instead of shutting down the whole controller on a crd_established timeout, the startup probe would now handle the checking and restarting.

The implementation was tested with the zookeeper-operator, changes there would then only include creating and passing a HealthCheckRegistry and marking checks as passed on crd_established signals. (few lines) -> part of the rollout later
After the rollout to the operators, the probes need to be added to the operator Deployments.

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

• Changes are OpenShift compatible
• CRD changes approved
• CRD documentation for all fields, following the style guide.
• Integration tests passed (for non trivial changes)
• Changes need to be "offline" compatible

Reviewer

• Code contains useful comments
• Code contains useful logging statements
• (Integration-)Test cases added
• Documentation added or updated. Follows the style guide.
• Changelog updated
• Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

• Feature Tracker has been updated
• Proper release label has been added

[Techassi]

I like the overall mechanism of this, nicely done! I only have a few suggestions to slightly optimize it. And I also have a few thoughts on where code is located.

All my suggestions target only code, so when (and if) they are applied, the doc tests need to be adjusted as well.

[Techassi]

The readiness checks being part of the public API makes me feel like the whole HealthCheck and HealthCheckRegistry machinery should be part of the stackable-webhook crate.

[Techassi]

We could potentially implement axum's IntoResponse trait here which internally calls to_string().

[Techassi]

The whole machinery could be simplified a little. I will add my suggestions to each piece of code individually.

Here, we only need access to the underlying AtomicBool. I also renamed the struct to better reflect what it is: a handle to mark a readiness check as ready.

Suggested change

	pub struct HealthCheck {
	name: String,
	passed: Arc<AtomicBool>,
	}
	pub struct ReadinessHandle(Arc<AtomicBool>)

[Techassi]

The impl block can also be simplified a bunch:

Suggested change

	impl HealthCheck {
	fn new(name: impl Into<String>) -> Self {
	Self {
	name: name.into(),
	passed: Arc::new(AtomicBool::new(false)),
	}
	}
	pub fn mark_passed(&self) {
	self.passed.store(true, Ordering::Release);
	}
	fn passed(&self) -> bool {
	self.passed.load(Ordering::Acquire)
	}
	}
	impl ReadinessHandle {
	pub fn ready(self) {
	self.0.store(true, Ordering::Release);
	}
	}

[Techassi]

We now use a Vec over a tuple instead:

Suggested change

	pub struct HealthCheckRegistry {
	checks: Vec<HealthCheck>,
	}
	// This has to be an AtomicBool as we could otherwise not share references to it.
	pub struct ReadinessChecks(Vec<(String, Arc<AtomicBool>)>);

[Techassi]

If both a new method and a Default impl exist, the Default impl should delegate to new, not the other way around (see CLippy lint). In this case, I would argue we can even remove the new method.

[Techassi]

Because we split the health check into its name and AtomicBool, we only have to clone the Arc (increase its reference count). I also renamed the all_passed method to all_ready.

Suggested change

	impl HealthCheckRegistry {
	pub fn new() -> Self {
	Self::default()
	}
	/// Registers a new [`HealthCheck`] with the provided name and returns it.
	pub fn register(&mut self, name: impl Into<String>) -> HealthCheck {
	let check = HealthCheck::new(name);
	self.checks.push(check.clone());
	check
	}
	/// Returns `true` if all the registered health checks have passed or no health checks are
	/// registered.
	pub fn all_passed(&self) -> bool {
	self.checks.iter().all(HealthCheck::passed)
	}
	}
	impl ReadinessChecks {
	/// Registers a new readiness check with the provided name.
	///
	/// The returned handle can be used to mark the check as ready.
	pub fn register(&mut self, name: impl Into<String>) -> ReadinessHandle {
	let ready = Arc::new(AtomicBool::default());
	// Store an reference counted clone of the same underlying AtomicBool in the list of checks.
	// Both the handle and the item in the list refer to the same AtomicBool.
	self.0.push((name.into(), ready.clone()));
	ReadinessHandle(ready)
	}
	/// Returns `true` if all the registered checks are ready or no checks are registered.
	pub fn all_ready(&self) -> bool {
	self.0
	.iter()
	.all(|(_, ready)| ready.load(Ordering::Acquire))
	}
	}

[Techassi]

We now have to adjust the handling and loading of the statuses:

Suggested change

	impl Display for HealthCheckRegistry {
	/// Renders one line per check, with the check's name and status only. Anything else, error
	/// causes in particular, must not end up in a response to an unauthenticated endpoint.
	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
	if self.checks.is_empty() {
	return writeln!(f, "[ok] no checks registered");
	}
	for check in &self.checks {
	let status = if check.passed() { "ok" } else { "pending" };
	writeln!(f, "[{status}] {name}", name = check.name)?;
	}
	Ok(())
	}
	}
	impl Display for ReadinessChecks {
	/// Renders one line per check, with the check's name and status only. Anything else, error
	/// causes in particular, must not end up in a response to an unauthenticated endpoint.
	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
	if self.0.is_empty() {
	return writeln!(f, "[ok] no readiness checks registered");
	}
	for (name, ready) in &self.0 {
	let status = if ready.load(Ordering::Acquire) {
	"ready"
	} else {
	"pending"
	};
	writeln!(f, "[{status}] {name}")?;
	}
	Ok(())
	}
	}

[Techassi]

Suggested change

	mod test {
	use super::*;
	#[test]
	fn passed_on_empty_registry() {
	let registry = HealthCheckRegistry::new();
	assert!(registry.all_passed());
	}
	#[test]
	fn passed_only_once_every_check_is() {
	let mut registry = HealthCheckRegistry::new();
	let crds = registry.register("crds-established");
	let migration = registry.register("database-migrated");
	assert!(!registry.all_passed());
	crds.mark_passed();
	assert!(!registry.all_passed());
	migration.mark_passed();
	assert!(registry.all_passed());
	}
	}
	mod test {
	use super::*;
	#[test]
	fn ready_on_empty_registry() {
	let checks = ReadinessChecks::default();
	assert!(checks.all_ready());
	}
	#[test]
	fn ready_only_once_every_check_is() {
	let mut checks = ReadinessChecks::default();
	let crds = checks.register("crds-established");
	let migration = checks.register("database-migrated");
	assert!(!checks.all_ready());
	crds.ready();
	assert!(!checks.all_ready());
	migration.ready();
	assert!(checks.all_ready());
	}
	}