refactor: move to v2 types for ResourceNames, labels, ownerrefs. Reduce RoleGroupRef usage.

Author: maltesander

rust/operator-binary/src/crd/affinity.rs

@@ -25,14 +25,15 @@ pub fn get_affinity(cluster_name: &str, role: &ZookeeperRole) -> StackableAffini
 #[cfg(test)]
 mod tests {
 
-    use std::collections::BTreeMap;
+    use std::{collections::BTreeMap, str::FromStr};
 
     use stackable_operator::{
         commons::affinity::StackableAffinity,
         k8s_openapi::{
             api::core::v1::{PodAffinityTerm, PodAntiAffinity, WeightedPodAffinityTerm},
             apimachinery::pkg::apis::meta::v1::LabelSelector,
         },
+        v2::types::operator::RoleGroupName,
     };
 
     use crate::{
@@ -98,7 +99,9 @@ mod tests {
             node_selector: None,
         };
 
-        let affinity = validated_cluster(&zk).role_group_configs[&ZookeeperRole::Server]["default"]
+        let default_group = RoleGroupName::from_str("default").expect("valid role group name");
+        let affinity = validated_cluster(&zk).role_group_configs[&ZookeeperRole::Server]
+            [&default_group]
             .config
             .affinity
             .clone();

rust/operator-binary/src/crd/mod.rs

@@ -14,7 +14,7 @@ use stackable_operator::{
     crd::ClusterRef,
     deep_merger::ObjectOverrides,
     k8s_openapi::apimachinery::pkg::api::resource::Quantity,
-    kube::{CustomResource, runtime::reflector::ObjectRef},
+    kube::{CustomResource, ResourceExt, runtime::reflector::ObjectRef},
     product_logging::{self, spec::Logging},
     role_utils::{GenericRoleConfig, Role, RoleGroupRef},
     schemars::{self, JsonSchema},
@@ -450,7 +450,7 @@ impl v1alpha1::ZookeeperCluster {
     ) -> Option<String> {
         Some(format!(
             "{role_listener_name}.{namespace}.svc.{cluster_domain}",
-            role_listener_name = role_listener_name(self, &ZookeeperRole::Server),
+            role_listener_name = role_listener_name(&self.name_any(), &ZookeeperRole::Server),
             namespace = self.metadata.namespace.as_ref()?,
             cluster_domain = cluster_info.cluster_domain
         ))

rust/operator-binary/src/listener.rs

@@ -1,88 +1,49 @@
 //! Types and functions for exposing product endpoints via [listener::v1alpha1::Listener].
 
-use snafu::{ResultExt as _, Snafu};
+use std::str::FromStr;
+
 use stackable_operator::{
-    builder::meta::ObjectMetaBuilder, commons::product_image_selection::ResolvedProductImage,
-    crd::listener, kube::ResourceExt as _,
+    builder::meta::ObjectMetaBuilder,
+    crd::listener,
+    v2::{builder::meta::ownerreference_from_resource, types::operator::RoleGroupName},
 };
 
 use crate::{
-    crd::{ZOOKEEPER_SERVER_PORT_NAME, ZookeeperRole, security::ZookeeperSecurity, v1alpha1},
-    utils::build_recommended_labels,
-    zk_controller::ZK_CONTROLLER_NAME,
+    crd::{ZOOKEEPER_SERVER_PORT_NAME, ZookeeperRole, security::ZookeeperSecurity},
+    zk_controller::validate::ValidatedCluster,
 };
 
-type Result<T, E = Error> = std::result::Result<T, E>;
-
-#[derive(Snafu, Debug)]
-pub enum Error {
-    #[snafu(display("Role {zk_role:?} is not defined in the ZooKeeperCluster spec"))]
-    InvalidRole {
-        source: crate::crd::Error,
-        zk_role: String,
-    },
-
-    #[snafu(display("object is missing metadata to build owner reference"))]
-    ObjectMissingMetadataForOwnerRef {
-        source: stackable_operator::builder::meta::Error,
-    },
-
-    #[snafu(display("failed to build recommended labels"))]
-    RecommendedLabels {
-        source: stackable_operator::builder::meta::Error,
-    },
-}
-
+/// Builds the role-level [`Listener`](listener::v1alpha1::Listener) exposing the ZooKeeper servers.
+///
+/// The listener is owned by, labelled and named from the [`ValidatedCluster`]; the ListenerClass
+/// and security settings are taken from its validated cluster config.
 pub fn build_role_listener(
-    zk: &v1alpha1::ZookeeperCluster,
+    cluster: &ValidatedCluster,
     zk_role: &ZookeeperRole,
-    resolved_product_image: &ResolvedProductImage,
-    zookeeper_security: &ZookeeperSecurity,
-) -> Result<listener::v1alpha1::Listener> {
-    let listener_name = role_listener_name(zk, zk_role);
-    let listener_class = &zk
-        .role(zk_role)
-        .with_context(|_| InvalidRoleSnafu {
-            zk_role: zk_role.to_string(),
-        })?
-        .role_config
-        .listener_class;
+) -> listener::v1alpha1::Listener {
+    // The listener is a role-level resource, so it has no role group. The recommended labels
+    // require a role-group value, so a constant "none" is used (matching the previous behaviour).
+    let role_group_name =
+        RoleGroupName::from_str("none").expect("'none' is a valid role group name");
 
-    let listener = listener::v1alpha1::Listener {
+    listener::v1alpha1::Listener {
         metadata: ObjectMetaBuilder::new()
-            .name_and_namespace(zk)
-            .name(listener_name)
-            .ownerreference_from_resource(zk, None, Some(true))
-            .context(ObjectMissingMetadataForOwnerRefSnafu)?
-            // Since we only make a listener for the role, which labels should we use?
-            // We can't use with_recommended_labels because it requires an ObjectLabels which
-            // in turn requires RoleGroup stuff)
-            // TODO (@NickLarsenNZ): Make separate functions for with_recommended_labels with/without rolegroups
-            // .with_labels(manual_labels).build()
-            .with_recommended_labels(&build_recommended_labels(
-                zk,
-                ZK_CONTROLLER_NAME,
-                &resolved_product_image.app_version_label_value,
-                &zk_role.to_string(),
-                "none", // TODO (@NickLarsenNZ): update build_recommended_labels to have an optional role_group
-            ))
-            .context(RecommendedLabelsSnafu)?
+            .name_and_namespace(cluster)
+            .name(role_listener_name(cluster.name.as_ref(), zk_role))
+            .ownerreference(ownerreference_from_resource(cluster, None, Some(true)))
+            .with_labels(cluster.recommended_labels(&role_group_name))
             .build(),
         spec: listener::v1alpha1::ListenerSpec {
-            class_name: Some(listener_class.to_owned()),
-            ports: Some(listener_ports(zookeeper_security)),
+            class_name: Some(cluster.cluster_config.listener_class.clone()),
+            ports: Some(listener_ports(&cluster.cluster_config.zookeeper_security)),
             ..listener::v1alpha1::ListenerSpec::default()
         },
         status: None,
-    };
-
-    Ok(listener)
+    }
 }
 
-// TODO (@NickLarsenNZ): This could be a method we can put on a Resource that takes a role_name
-pub fn role_listener_name(zk: &v1alpha1::ZookeeperCluster, zk_role: &ZookeeperRole) -> String {
-    // TODO (@NickLarsenNZ): Make a convention, do we use name_any() and allow empty string? or handle the error (as unlikely as it would be)?
-    format!("{zk}-{zk_role}", zk = zk.name_any())
+pub fn role_listener_name(cluster_name: &str, zk_role: &ZookeeperRole) -> String {
+    format!("{cluster_name}-{zk_role}")
 }
 
 // We only use the server port here and intentionally omit the metrics one.

rust/operator-binary/src/operations/pdb.rs

@@ -1,21 +1,24 @@
+use std::str::FromStr;
+
 use snafu::{ResultExt, Snafu};
 use stackable_operator::{
-    builder::pdb::PodDisruptionBudgetBuilder, client::Client, cluster_resources::ClusterResources,
-    commons::pdb::PdbConfig, kube::ResourceExt,
+    client::Client,
+    cluster_resources::ClusterResources,
+    commons::pdb::PdbConfig,
+    kube::ResourceExt,
+    v2::{
+        builder::pdb::pod_disruption_budget_builder_with_role,
+        types::operator::{ControllerName, OperatorName, ProductName, RoleName},
+    },
 };
 
 use crate::{
-    crd::{APP_NAME, OPERATOR_NAME, ZookeeperRole, v1alpha1},
-    zk_controller::ZK_CONTROLLER_NAME,
+    crd::{APP_NAME, OPERATOR_NAME, ZookeeperRole},
+    zk_controller::{ZK_CONTROLLER_NAME, validate::ValidatedCluster},
 };
 
 #[derive(Snafu, Debug)]
 pub enum Error {
-    #[snafu(display("Cannot create PodDisruptionBudget for role [{role}]"))]
-    CreatePdb {
-        source: stackable_operator::builder::pdb::Error,
-        role: String,
-    },
     #[snafu(display("Cannot apply PodDisruptionBudget [{name}]"))]
     ApplyPdb {
         source: stackable_operator::cluster_resources::Error,
@@ -25,7 +28,7 @@ pub enum Error {
 
 pub async fn add_pdbs(
     pdb: &PdbConfig,
-    zookeeper: &v1alpha1::ZookeeperCluster,
+    validated_cluster: &ValidatedCluster,
     role: &ZookeeperRole,
     client: &Client,
     cluster_resources: &mut ClusterResources<'_>,
@@ -36,16 +39,25 @@ pub async fn add_pdbs(
     let max_unavailable = pdb.max_unavailable.unwrap_or(match role {
         ZookeeperRole::Server => max_unavailable_servers(),
     });
-    let pdb = PodDisruptionBudgetBuilder::new_with_role(
-        zookeeper,
-        APP_NAME,
-        &role.to_string(),
-        OPERATOR_NAME,
-        ZK_CONTROLLER_NAME,
+
+    // These names are derived from compile-time constants and a validated role enum, so they are
+    // guaranteed to be valid and we use the infallible v2 builder.
+    let product_name =
+        ProductName::from_str(APP_NAME).expect("APP_NAME should be a valid product name");
+    let operator_name = OperatorName::from_str(OPERATOR_NAME)
+        .expect("OPERATOR_NAME should be a valid operator name");
+    let controller_name = ControllerName::from_str(ZK_CONTROLLER_NAME)
+        .expect("ZK_CONTROLLER_NAME should be a valid controller name");
+    let role_name =
+        RoleName::from_str(&role.to_string()).expect("role name should be a valid role name");
+
+    let pdb = pod_disruption_budget_builder_with_role(
+        validated_cluster,
+        &product_name,
+        &role_name,
+        &operator_name,
+        &controller_name,
     )
-    .with_context(|_| CreatePdbSnafu {
-        role: role.to_string(),
-    })?
     .with_max_unavailable(max_unavailable)
     .build();
     let pdb_name = pdb.name_any();