refactor: improve shutdown config, fix securitiy config exposed_ports

Author: maltesander

rust/operator-binary/src/controller/build/graceful_shutdown.rs

@@ -26,37 +26,29 @@ pub fn add_graceful_shutdown_config(
     // This must be always set by the merge mechanism, as we provide a default value,
     // users can not disable graceful shutdown.
     if let Some(termination_grace_period) = graceful_shutdown_timeout {
-        match role {
-            DruidRole::Coordinator
-            | DruidRole::Broker
-            | DruidRole::Historical
-            | DruidRole::Router => {
-                pod_builder
-                    .termination_grace_period(&termination_grace_period)
-                    .context(SetTerminationGracePeriodSnafu)?;
-            }
-            DruidRole::MiddleManager => {
-                pod_builder
-                    .termination_grace_period(&termination_grace_period)
-                    .context(SetTerminationGracePeriodSnafu)?;
+        // Every role gets the termination grace period; only the MiddleManager additionally needs a
+        // pre-stop hook that drains running tasks before the period elapses.
+        pod_builder
+            .termination_grace_period(&termination_grace_period)
+            .context(SetTerminationGracePeriodSnafu)?;
 
-                let (protocol, port) = if tls_security.tls_enabled() {
-                    ("https", role.get_https_port())
-                } else {
-                    ("http", role.get_http_port())
-                };
+        if *role == DruidRole::MiddleManager {
+            let (protocol, port) = if tls_security.tls_enabled() {
+                ("https", role.get_https_port())
+            } else {
+                ("http", role.get_http_port())
+            };
 
-                let middle_manager_host = format!("{protocol}://127.0.0.1:{port}");
-                let debug_message =
-                    "$(date --utc +%FT%T,%3N) INFO [stackable_lifecycle_pre_stop] -";
-                let sleep_interval = 2;
+            let middle_manager_host = format!("{protocol}://127.0.0.1:{port}");
+            let debug_message = "$(date --utc +%FT%T,%3N) INFO [stackable_lifecycle_pre_stop] -";
+            let sleep_interval = 2;
 
-                // The middle manager can be terminated gracefully by disabling it, meaning
-                // the overlord will not send any new tasks and it will terminate after
-                // all tasks are finished or the termination grace period is exceeded.
-                // See: https://druid.apache.org/docs/latest/operations/rolling-updates/#rolling-restart-graceful-termination-based
-                // The DRUID_PID is set in the crd/src/lib.rs `main_container_start_command` method.
-                druid_builder.lifecycle_pre_stop(LifecycleHandler {
+            // The middle manager can be terminated gracefully by disabling it, meaning
+            // the overlord will not send any new tasks and it will terminate after
+            // all tasks are finished or the termination grace period is exceeded.
+            // See: https://druid.apache.org/docs/latest/operations/rolling-updates/#rolling-restart-graceful-termination-based
+            // The DRUID_PID is set in the crd/src/lib.rs `main_container_start_command` method.
+            druid_builder.lifecycle_pre_stop(LifecycleHandler {
                     exec: Some(ExecAction {
                         command: Some(vec![
                             "/bin/bash".to_string(),
@@ -103,7 +95,6 @@ pub fn add_graceful_shutdown_config(
                     }),
                     ..Default::default()
                 });
-            }
         }
     }
 

rust/operator-binary/src/controller/build/properties/security_properties.rs

@@ -13,7 +13,7 @@ const DEFAULT_NETWORKADDRESS_CACHE_NEGATIVE_TTL: &str = "0";
 /// `overrides` are the user's merged (role <- rolegroup) `security.properties`
 /// config overrides, as returned by `DruidConfigOverrides::get_key_value_overrides`.
 /// Override values win.
-pub fn build(overrides: &BTreeMap<String, String>) -> BTreeMap<String, String> {
+pub fn build(overrides: BTreeMap<String, String>) -> BTreeMap<String, String> {
     let mut props: BTreeMap<String, String> = BTreeMap::new();
     props.insert(
         NETWORKADDRESS_CACHE_TTL.to_string(),
@@ -23,7 +23,7 @@ pub fn build(overrides: &BTreeMap<String, String>) -> BTreeMap<String, String> {
         NETWORKADDRESS_CACHE_NEGATIVE_TTL.to_string(),
         DEFAULT_NETWORKADDRESS_CACHE_NEGATIVE_TTL.to_string(),
     );
-    props.extend(overrides.iter().map(|(k, v)| (k.clone(), v.clone())));
+    props.extend(overrides);
     props
 }
 
@@ -37,7 +37,7 @@ mod tests {
     //   networkaddress.cache.ttl=30
     #[test]
     fn defaults_match_snapshot() {
-        let props = build(&BTreeMap::new());
+        let props = build(BTreeMap::new());
         assert_eq!(props["networkaddress.cache.ttl"], "30".to_string());
         assert_eq!(props["networkaddress.cache.negative.ttl"], "0".to_string());
         assert_eq!(props.len(), 2);
@@ -46,7 +46,7 @@ mod tests {
     #[test]
     fn override_wins() {
         let ov = BTreeMap::from([("networkaddress.cache.ttl".to_string(), "60".to_string())]);
-        let props = build(&ov);
+        let props = build(ov);
         assert_eq!(props["networkaddress.cache.ttl"], "60".to_string());
     }
 }

rust/operator-binary/src/controller/build/resource/config_map.rs

@@ -314,7 +314,7 @@ pub fn build_rolegroup_config_map(
         }
         let security_overrides =
             key_value_overrides(&rg.config_overrides, ConfigFileName::SecurityProperties);
-        security_config.extend(security_properties::build(&security_overrides));
+        security_config.extend(security_properties::build(security_overrides));
 
         cm_conf_data.insert(
             ConfigFileName::SecurityProperties.to_string(),

rust/operator-binary/src/controller/build/security.rs

@@ -87,6 +87,9 @@ const AUTH_TRUST_STORE_PASSWORD: &str = "druid.auth.basic.ssl.trustStorePassword
 const TRUST_STORE_FILE: &str = "truststore.p12";
 const KEY_STORE_FILE: &str = "keystore.p12";
 
+// The layer-4 protocol used by all of Druid's exposed ports.
+const TCP_PROTOCOL: &str = "TCP";
+
 // directories
 const STACKABLE_MOUNT_TLS_DIR: &str = "/stackable/mount_tls";
 
@@ -95,48 +98,43 @@ stackable_operator::constant!(TLS_VOLUME_NAME: VolumeName = "tls");
 stackable_operator::constant!(TLS_MOUNT_VOLUME_NAME: VolumeName = "tls-mount");
 
 pub fn container_ports(tls: &DruidTlsSecurity, role: &DruidRole) -> Vec<ContainerPort> {
-    exposed_ports(tls, role)
-        .into_iter()
-        .map(|(name, val)| ContainerPort {
-            name: Some(name),
-            container_port: val.into(),
-            protocol: Some("TCP".to_string()),
-            ..ContainerPort::default()
-        })
-        .collect()
+    let (name, port) = exposed_port(tls, role);
+    vec![ContainerPort {
+        name: Some(name.to_string()),
+        container_port: port.into(),
+        protocol: Some(TCP_PROTOCOL.to_string()),
+        ..ContainerPort::default()
+    }]
 }
 
 pub fn service_ports(tls: &DruidTlsSecurity, role: &DruidRole) -> Vec<ServicePort> {
-    exposed_ports(tls, role)
-        .into_iter()
-        .map(|(name, val)| ServicePort {
-            name: Some(name),
-            port: val.into(),
-            protocol: Some("TCP".to_string()),
-            ..ServicePort::default()
-        })
-        .collect()
+    let (name, port) = exposed_port(tls, role);
+    vec![ServicePort {
+        name: Some(name.to_string()),
+        port: port.into(),
+        protocol: Some(TCP_PROTOCOL.to_string()),
+        ..ServicePort::default()
+    }]
 }
 
 pub fn listener_ports(
     tls: &DruidTlsSecurity,
     role: &DruidRole,
 ) -> Vec<listener::v1alpha1::ListenerPort> {
-    exposed_ports(tls, role)
-        .into_iter()
-        .map(|(name, val)| listener::v1alpha1::ListenerPort {
-            name,
-            port: val.into(),
-            protocol: Some("TCP".to_string()),
-        })
-        .collect()
+    let (name, port) = exposed_port(tls, role);
+    vec![listener::v1alpha1::ListenerPort {
+        name: name.to_string(),
+        port: port.into(),
+        protocol: Some(TCP_PROTOCOL.to_string()),
+    }]
 }
 
-fn exposed_ports(tls: &DruidTlsSecurity, role: &DruidRole) -> Vec<(String, Port)> {
+/// The single port (TLS or plaintext, depending on the TLS decision) Druid exposes for the role.
+fn exposed_port(tls: &DruidTlsSecurity, role: &DruidRole) -> (&'static str, Port) {
     if tls.tls_enabled() {
-        vec![(TLS_PORT_NAME.to_string(), role.get_https_port())]
+        (TLS_PORT_NAME, role.get_https_port())
     } else {
-        vec![(PLAINTEXT_PORT_NAME.to_string(), role.get_http_port())]
+        (PLAINTEXT_PORT_NAME, role.get_http_port())
     }
 }