Skip to content

chore: Consolidate error enums and use expect where possible #1084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adwk67 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore: Consolidate error enums and use expect where possible #1084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ae3936
use expect where possible and remove unecessary enums/Results
adwk67 Sep 8, 2026
e14b740
Merge branch 'main' into fix/consolidate-result--handling
adwk67 Sep 8, 2026
3046f9a
Apply suggestions from code review
adwk67 Sep 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions rust/operator-binary/src/crd/affinity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
use stackable_operator::{
commons::affinity::{StackableAffinityFragment, affinity_between_role_pods},
k8s_openapi::api::core::v1::PodAntiAffinity,
v2::types::operator::ClusterName,
};

use crate::crd::{APP_NAME, ZookeeperRole};

pub fn get_affinity(cluster_name: &str, role: &ZookeeperRole) -> StackableAffinityFragment {
pub fn get_affinity(cluster_name: &ClusterName, role: &ZookeeperRole) -> StackableAffinityFragment {
let affinity_between_role_pods =
affinity_between_role_pods(APP_NAME, cluster_name, role.as_ref(), 70);
affinity_between_role_pods(APP_NAME, cluster_name.as_ref(), role.as_ref(), 70);

StackableAffinityFragment {
pod_affinity: None,
Expand Down
3 changes: 2 additions & 1 deletion rust/operator-binary/src/crd/authentication.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ impl DereferencedAuthenticationClasses {
Ok(self.clone())
}

/// USE ONLY IN TESTS! We can not put it behind `#[cfg(test)]` because of <https://github.com/rust-lang/cargo/issues/8379>
/// Test fixture without any AuthenticationClasses.
#[cfg(test)]
pub fn new_for_tests() -> Self {
DereferencedAuthenticationClasses {
dereferenced_authentication_classes: vec![],
Expand Down
83 changes: 57 additions & 26 deletions rust/operator-binary/src/crd/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use stackable_operator::{
crd::ClusterRef,
deep_merger::ObjectOverrides,
k8s_openapi::apimachinery::pkg::api::resource::Quantity,
kube::{CustomResource, ResourceExt},
kube::CustomResource,
product_logging::{self, spec::Logging},
role_utils::GenericRoleConfig,
schemars::{self, JsonSchema},
Expand All @@ -31,7 +31,7 @@ use stackable_operator::{
kubernetes::{
ConfigMapName, ListenerClassName, ListenerName, NamespaceName, ServiceName,
},
operator::{OperatorName, ProductName, RoleName},
operator::{ClusterName, OperatorName, ProductName, RoleName},
},
},
versioned::versioned,
Expand All @@ -49,10 +49,35 @@ pub mod tls;
/// exposing the given `zk_role`, `<cluster>-<role>`.
///
/// Lives in the `crd` module (rather than the controller build tree) because it is shared by both
/// controllers and by [`v1alpha1::ZookeeperCluster::server_role_listener_fqdn`].
pub fn role_listener_name(cluster_name: &str, zk_role: &ZookeeperRole) -> ListenerName {
ListenerName::from_str(&format!("{cluster_name}-{role}", role = zk_role.as_ref()))
.expect("the role listener name should be a valid Listener name")
/// controllers and by [`role_listener_fqdn`].
pub fn role_listener_name(cluster_name: &ClusterName, zk_role: &ZookeeperRole) -> ListenerName {
const _: () = assert!(
ClusterName::MAX_LENGTH + 1 /* dash */ + RoleName::MAX_LENGTH <= ListenerName::MAX_LENGTH,
"The string `<cluster_name>-<role_name>` must not exceed the limit of Listener names."
);
// Both halves are RFC 1123 labels joined by a dash, which is a valid RFC 1123 subdomain.
let _ = ClusterName::IS_RFC_1123_SUBDOMAIN_NAME;

@maltesander maltesander Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also used as a service name and as the first label of the FQDN, so it shouldbe a label with max 63 chars and no dots? This would affect the check above.

Suggested change
let _ = ClusterName::IS_RFC_1123_SUBDOMAIN_NAME;
let _ = ClusterName::IS_RFC_1035_LABEL_NAME;

@siegfriedweber siegfriedweber Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just document that the returned ListenerName is an RFC 1035 label name and create a unit test:

diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs
index d2df7a1..fc81e09 100644
--- a/rust/operator-binary/src/crd/mod.rs
+++ b/rust/operator-binary/src/crd/mod.rs
@@ -50,6 +50,8 @@ pub mod tls;
 ///
 /// Lives in the `crd` module (rather than the controller build tree) because it is shared by both
 /// controllers and by [`role_listener_fqdn`].
+///
+/// The returned ListenerName is a lowercase RFC 1035 label name (checked by a unit test).
 pub fn role_listener_name(cluster_name: &ClusterName, zk_role: &ZookeeperRole) -> ListenerName {
     const _: () = assert!(
         ClusterName::MAX_LENGTH + 1 /* dash */ + RoleName::MAX_LENGTH <= ListenerName::MAX_LENGTH,
@@ -340,7 +342,7 @@ pub mod versioned {
     }
 }

-#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
+#[derive(Clone, Debug, Eq, EnumIter, Hash, Ord, PartialEq, PartialOrd)]
 pub enum ZookeeperRole {
     Server,
 }
@@ -478,6 +480,7 @@ mod tests {
     use stackable_operator::{
         commons::networking::DomainName, versioned::test_utils::RoundtripTestData,
     };
+    use strum::IntoEnumIterator;

     use super::*;

@@ -774,4 +777,22 @@ mod tests {
             .expect("Failed to parse ZookeeperZnodeSpec YAML")
         }
     }
+
+    #[test]
+    fn role_listener_name_is_rfc_1035_label_name() {
+        // Every ClusterName is a valid RFC 1035 label name, so we use just some string with maximum
+        // length.
+        let _ = ClusterName::IS_RFC_1035_LABEL_NAME;
+        let cluster_name = ClusterName::from_str_unsafe(&"a".repeat(ClusterName::MAX_LENGTH));
+
+        for role in ZookeeperRole::iter() {
+            let role_listener_name = role_listener_name(&cluster_name, &role);
+            assert!(
+                stackable_operator::validation::is_lowercase_rfc_1035_label(
+                    role_listener_name.as_ref()
+                )
+                .is_ok()
+            );
+        }
+    }
 }

let _ = RoleName::IS_RFC_1123_LABEL_NAME;

let role_name: &RoleName = zk_role;
ListenerName::from_str(&format!("{cluster_name}-{role_name}"))
.expect("is a valid Listener name")
}

/// The fully-qualified domain name of the role-level
/// [`Listener`](stackable_operator::crd::listener::v1alpha1::Listener) exposing the given
/// `zk_role`, `<cluster>-<role>.<namespace>.svc.<cluster_domain>`.
pub fn role_listener_fqdn(
cluster_name: &ClusterName,
namespace: &NamespaceName,
zk_role: &ZookeeperRole,
cluster_info: &KubernetesClusterInfo,
) -> String {
format!(
"{role_listener_name}.{namespace}.svc.{cluster_domain}",
role_listener_name = role_listener_name(cluster_name, zk_role),
cluster_domain = cluster_info.cluster_domain
)
}

pub const APP_NAME: &str = "zookeeper";
Expand Down Expand Up @@ -87,7 +112,7 @@ pub const STACKABLE_RW_CONFIG_DIR: &str = "/stackable/rwconfig";
pub const CONTAINER_IMAGE_BASE_NAME: &str = "zookeeper";

const DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT: Duration = Duration::from_minutes_unchecked(2);
pub const DEFAULT_LISTENER_CLASS: &str = "cluster-internal";
constant!(pub DEFAULT_LISTENER_CLASS: ListenerClassName = "cluster-internal");

pub type ZookeeperServerRoleType = Role<
v1alpha1::ZookeeperConfigFragment,
Expand Down Expand Up @@ -358,8 +383,7 @@ fn cluster_config_default() -> v1alpha1::ZookeeperClusterConfig {
}

pub(crate) fn default_listener_class() -> ListenerClassName {
ListenerClassName::from_str(DEFAULT_LISTENER_CLASS)
.expect("the default listener class should be a valid ListenerClass name")
DEFAULT_LISTENER_CLASS.clone()
}

impl Default for ZookeeperServerRoleConfig {
Expand All @@ -380,7 +404,7 @@ impl v1alpha1::ZookeeperConfig {
pub const TICK_TIME: &'static str = "tickTime";

pub(crate) fn default_server_config(
cluster_name: &str,
cluster_name: &ClusterName,
role: &ZookeeperRole,
) -> v1alpha1::ZookeeperConfigFragment {
v1alpha1::ZookeeperConfigFragment {
Expand Down Expand Up @@ -435,21 +459,6 @@ impl ZookeeperPodRef {
}

impl v1alpha1::ZookeeperCluster {
/// The fully-qualified domain name of the role-level [Listener]
///
/// [Listener]: stackable_operator::crd::listener::v1alpha1::Listener
pub fn server_role_listener_fqdn(
&self,
cluster_info: &KubernetesClusterInfo,
) -> Option<String> {
Some(format!(
"{role_listener_name}.{namespace}.svc.{cluster_domain}",
role_listener_name = role_listener_name(&self.name_any(), &ZookeeperRole::Server),
namespace = self.metadata.namespace.as_ref()?,
cluster_domain = cluster_info.cluster_domain
))
}

/// Returns the given role (the `servers` role is required by the CRD).
pub fn role(&self, role_variant: &ZookeeperRole) -> &ZookeeperServerRoleType {
match role_variant {
Expand All @@ -466,7 +475,9 @@ impl v1alpha1::ZookeeperCluster {

#[cfg(test)]
mod tests {
use stackable_operator::versioned::test_utils::RoundtripTestData;
use stackable_operator::{
commons::networking::DomainName, versioned::test_utils::RoundtripTestData,
};

use super::*;

Expand All @@ -476,6 +487,26 @@ mod tests {
let _ = *PRODUCT_NAME;
let _ = *OPERATOR_NAME;
let _ = *SERVER_ROLE_NAME;
let _ = *DEFAULT_LISTENER_CLASS;
}

#[test]
fn role_listener_fqdn_joins_name_namespace_and_cluster_domain() {
let cluster_name = ClusterName::from_str("simple-zookeeper").expect("valid cluster name");
let namespace = NamespaceName::from_str("default").expect("valid namespace");
let cluster_info = KubernetesClusterInfo {
cluster_domain: DomainName::from_str("cluster.local").expect("valid domain"),
};

assert_eq!(
role_listener_fqdn(
&cluster_name,
&namespace,
&ZookeeperRole::Server,
&cluster_info
),
"simple-zookeeper-server.default.svc.cluster.local"
);
}

fn get_server_secret_class(zk: &v1alpha1::ZookeeperCluster) -> Option<&str> {
Expand Down
95 changes: 28 additions & 67 deletions rust/operator-binary/src/crd/security.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,11 @@
//! This is required due to overlaps between TLS encryption and e.g. mTLS authentication or Kerberos
use std::{collections::BTreeMap, str::FromStr};

use snafu::{ResultExt, Snafu};
use stackable_operator::{
builder::{
self,
pod::{
PodBuilder,
container::ContainerBuilder,
volume::{
SecretFormat, SecretOperatorVolumeSourceBuilder,
SecretOperatorVolumeSourceBuilderError, VolumeBuilder,
},
},
builder::pod::{
PodBuilder,
container::ContainerBuilder,
volume::{SecretFormat, SecretOperatorVolumeSourceBuilder, VolumeBuilder},
},
commons::secret_class::SecretClassVolumeProvisionParts,
constant,
Expand All @@ -39,25 +32,6 @@ use crate::{
constant!(SERVER_TLS_VOLUME_NAME: VolumeName = "server-tls");
constant!(QUORUM_TLS_VOLUME_NAME: VolumeName = "quorum-tls");

type Result<T, E = Error> = std::result::Result<T, E>;

#[derive(Snafu, Debug)]
pub enum Error {
#[snafu(display("failed to build TLS volume for {volume_name:?}"))]
BuildTlsVolume {
source: SecretOperatorVolumeSourceBuilderError,
volume_name: String,
},

#[snafu(display("failed to add needed volume"))]
AddVolume { source: builder::pod::Error },

#[snafu(display("failed to add needed volumeMount"))]
AddVolumeMount {
source: builder::pod::container::Error,
},
}

/// Helper struct combining TLS settings for server and quorum with the resolved AuthenticationClasses
pub struct ZookeeperSecurity {
resolved_authentication_classes: DereferencedAuthenticationClasses,
Expand Down Expand Up @@ -151,40 +125,47 @@ impl ZookeeperSecurity {

/// Adds required volumes and volume mounts to the pod and container builders
/// depending on the tls and authentication settings.
///
/// # Panics
///
/// Panics if the volumes or volume mounts cannot be added to the builders. Only call this on
/// builders whose volume names and mount paths are still distinct from the ones added here.
pub fn add_volume_mounts(
&self,
pod_builder: &mut PodBuilder,
cb_zookeeper: &mut ContainerBuilder,
requested_secret_lifetime: &Duration,
) -> Result<()> {
) {
let tls_secret_class = self.get_tls_secret_class();

if let Some(secret_class) = tls_secret_class {
cb_zookeeper
.add_volume_mount(&*SERVER_TLS_VOLUME_NAME, Self::SERVER_TLS_DIR)
.context(AddVolumeMountSnafu)?;
.expect(
"The mount paths are statically defined and there should be no duplicates.",
);
pod_builder
.add_volume(Self::create_server_tls_volume(
&SERVER_TLS_VOLUME_NAME,
secret_class,
requested_secret_lifetime,
)?)
.context(AddVolumeSnafu)?;
))
.expect(
"The volume names are statically defined and there should be no duplicates.",
);
}

// quorum
cb_zookeeper
.add_volume_mount(&*QUORUM_TLS_VOLUME_NAME, Self::QUORUM_TLS_DIR)
.context(AddVolumeMountSnafu)?;
.expect("The mount paths are statically defined and there should be no duplicates.");
pod_builder
.add_volume(Self::create_quorum_tls_volume(
&QUORUM_TLS_VOLUME_NAME,
self.quorum_secret_class.as_ref(),
requested_secret_lifetime,
)?)
.context(AddVolumeSnafu)?;

Ok(())
))
.expect("The volume names are statically defined and there should be no duplicates.");
}

/// Returns required ZooKeeper configuration settings for the `zoo.cfg` properties file
Expand Down Expand Up @@ -328,8 +309,8 @@ impl ZookeeperSecurity {
volume_name: &VolumeName,
secret_class_name: &str,
requested_secret_lifetime: &Duration,
) -> Result<Volume> {
let volume = VolumeBuilder::new(volume_name.to_string())
) -> Volume {
VolumeBuilder::new(volume_name.to_string())
.ephemeral(
SecretOperatorVolumeSourceBuilder::new(
secret_class_name,
Expand All @@ -340,13 +321,9 @@ impl ZookeeperSecurity {
.with_format(SecretFormat::TlsPkcs12)
.with_auto_tls_cert_lifetime(*requested_secret_lifetime)
.build()
.context(BuildTlsVolumeSnafu {
volume_name: volume_name.to_string(),
})?,
.expect("All inputs are valid and complete, so the builder does not fail."),
)
.build();

Ok(volume)
.build()
}

/// Creates ephemeral volumes to mount the `SecretClass` with the pod scope into the Pods.
Expand All @@ -356,8 +333,8 @@ impl ZookeeperSecurity {
volume_name: &VolumeName,
secret_class_name: &str,
requested_secret_lifetime: &Duration,
) -> Result<Volume> {
let volume = VolumeBuilder::new(volume_name.to_string())
) -> Volume {
VolumeBuilder::new(volume_name.to_string())
.ephemeral(
SecretOperatorVolumeSourceBuilder::new(
secret_class_name,
Expand All @@ -368,25 +345,9 @@ impl ZookeeperSecurity {
.with_format(SecretFormat::TlsPkcs12)
.with_auto_tls_cert_lifetime(*requested_secret_lifetime)
.build()
.context(BuildTlsVolumeSnafu {
volume_name: volume_name.to_string(),
})?,
.expect("All inputs are valid and complete, so the builder does not fail."),
)
.build();

Ok(volume)
}

/// USE ONLY IN TESTS! We can not put it behind `#[cfg(test)]` because of <https://github.com/rust-lang/cargo/issues/8379>
pub fn new_for_tests() -> Self {
ZookeeperSecurity {
resolved_authentication_classes: DereferencedAuthenticationClasses::new_for_tests(),
server_secret_class: Some(
SecretClassName::from_str("tls").expect("'tls' is a valid SecretClass name"),
),
quorum_secret_class: SecretClassName::from_str("tls")
.expect("'tls' is a valid SecretClass name"),
}
.build()
}
}

Expand Down
17 changes: 14 additions & 3 deletions rust/operator-binary/src/crd/tls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ use std::str::FromStr;

use serde::{Deserialize, Serialize};
use stackable_operator::{
constant,
schemars::{self, JsonSchema},
v2::types::kubernetes::SecretClassName,
versioned::versioned,
};

const TLS_DEFAULT_SECRET_CLASS: &str = "tls";
constant!(TLS_DEFAULT_SECRET_CLASS: SecretClassName = "tls");

#[versioned(version(name = "v1alpha1"))]
pub mod versioned {
Expand Down Expand Up @@ -53,6 +54,16 @@ pub fn server_tls_default() -> Option<SecretClassName> {

/// Helper methods to provide defaults in the CRDs and tests
pub fn quorum_tls_default() -> SecretClassName {
SecretClassName::from_str(TLS_DEFAULT_SECRET_CLASS)
.expect("the default TLS secret class should be a valid SecretClass name")
TLS_DEFAULT_SECRET_CLASS.clone()
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_constants() {
// Test that dereferencing the constant does not panic.
let _ = *TLS_DEFAULT_SECRET_CLASS;
}
}
Loading
Loading