fix: fix up issues in target separation (#1365)

- Removal of duplication check
- Proper loading of targets on server init
- Masking of sensitive information (slack URL, alert manager auth credentials)
- Error on modifying name
- Renaming repeat to notificationConfig and setting least count of interval to 1 minute

Author: parmesant

src/alerts/mod.rs

@@ -28,6 +28,7 @@ use serde_json::Error as SerdeError;
 use std::collections::{HashMap, HashSet};
 use std::fmt::{self, Display};
 use std::thread;
+use std::time::Duration;
 use tokio::sync::oneshot::{Receiver, Sender};
 use tokio::sync::{mpsc, RwLock};
 use tokio::task::JoinHandle;
@@ -610,10 +611,12 @@ impl AlertConfig {
         // validate that target repeat notifs !> eval_frequency
         for target_id in &self.targets {
             let target = TARGETS.get_target_by_id(target_id).await?;
-            match &target.timeout.times {
+            match &target.notification_config.times {
                 target::Retry::Infinite => {}
                 target::Retry::Finite(repeat) => {
-                    let notif_duration = target.timeout.interval * *repeat as u32;
+                    let notif_duration =
+                        Duration::from_secs(60 * target.notification_config.interval)
+                            * *repeat as u32;
                     if (notif_duration.as_secs_f64()).gt(&((eval_frequency * 60) as f64)) {
                         return Err(AlertError::Metadata(
                             "evalFrequency should be greater than target repetition  interval",
@@ -853,8 +856,8 @@ pub enum AlertError {
     FromStrError(#[from] FromStrError),
     #[error("Invalid Target ID- {0}")]
     InvalidTargetID(String),
-    #[error("Target already exists")]
-    DuplicateTargetConfig,
+    #[error("Invalid target modification request: {0}")]
+    InvalidTargetModification(String),
     #[error("Can't delete a Target which is being used")]
     TargetInUse,
 }
@@ -875,7 +878,7 @@ impl actix_web::ResponseError for AlertError {
             Self::InvalidAlertModifyRequest => StatusCode::BAD_REQUEST,
             Self::FromStrError(_) => StatusCode::BAD_REQUEST,
             Self::InvalidTargetID(_) => StatusCode::BAD_REQUEST,
-            Self::DuplicateTargetConfig => StatusCode::BAD_REQUEST,
+            Self::InvalidTargetModification(_) => StatusCode::BAD_REQUEST,
             Self::TargetInUse => StatusCode::CONFLICT,
         }
     }

src/alerts/target.rs

@@ -27,10 +27,10 @@ use base64::Engine;
 use bytes::Bytes;
 use chrono::Utc;
 use http::{header::AUTHORIZATION, HeaderMap, HeaderValue};
-use humantime_serde::re::humantime;
 use itertools::Itertools;
 use once_cell::sync::Lazy;
 use reqwest::ClientBuilder;
+use serde_json::{json, Value};
 use tokio::sync::RwLock;
 use tracing::{error, trace, warn};
 use ulid::Ulid;
@@ -66,14 +66,6 @@ impl TargetConfigs {
 
     pub async fn update(&self, target: Target) -> Result<(), AlertError> {
         let mut map = self.target_configs.write().await;
-        if map.values().any(|t| {
-            t.target == target.target
-                && t.timeout.interval == target.timeout.interval
-                && t.timeout.times == target.timeout.times
-                && t.id != target.id
-        }) {
-            return Err(AlertError::DuplicateTargetConfig);
-        }
         map.insert(target.id, target.clone());
 
         let path = target_json_path(&target.id);
@@ -148,16 +140,84 @@ pub struct Target {
     pub name: String,
     #[serde(flatten)]
     pub target: TargetType,
-    #[serde(default, rename = "repeat")]
-    pub timeout: Timeout,
+    pub notification_config: Timeout,
     #[serde(default = "Ulid::new")]
     pub id: Ulid,
 }
 
 impl Target {
+    pub fn mask(self) -> Value {
+        match self.target {
+            TargetType::Slack(slack_web_hook) => {
+                let endpoint = slack_web_hook.endpoint.to_string();
+                let masked_endpoint = if endpoint.len() > 20 {
+                    format!("{}********", &endpoint[..20])
+                } else {
+                    "********".to_string()
+                };
+                json!({
+                   "name":self.name,
+                   "type":"slack",
+                   "endpoint":masked_endpoint,
+                   "notificationConfig":self.notification_config,
+                   "id":self.id
+                })
+            }
+            TargetType::Other(other_web_hook) => {
+                let endpoint = other_web_hook.endpoint.to_string();
+                let masked_endpoint = if endpoint.len() > 20 {
+                    format!("{}********", &endpoint[..20])
+                } else {
+                    "********".to_string()
+                };
+                json!({
+                    "name":self.name,
+                    "type":"webhook",
+                    "endpoint":masked_endpoint,
+                    "headers":other_web_hook.headers,
+                    "skipTlsCheck":other_web_hook.skip_tls_check,
+                    "notificationConfig":self.notification_config,
+                    "id":self.id
+                })
+            }
+            TargetType::AlertManager(alert_manager) => {
+                let endpoint = alert_manager.endpoint.to_string();
+                let masked_endpoint = if endpoint.len() > 20 {
+                    format!("{}********", &endpoint[..20])
+                } else {
+                    "********".to_string()
+                };
+                if let Some(auth) = alert_manager.auth {
+                    let password = "********";
+                    json!({
+                        "name":self.name,
+                        "type":"webhook",
+                        "endpoint":masked_endpoint,
+                        "username":auth.username,
+                        "password":password,
+                        "skipTlsCheck":alert_manager.skip_tls_check,
+                        "notificationConfig":self.notification_config,
+                        "id":self.id
+                    })
+                } else {
+                    json!({
+                        "name":self.name,
+                        "type":"webhook",
+                        "endpoint":masked_endpoint,
+                        "username":Value::Null,
+                        "password":Value::Null,
+                        "skipTlsCheck":alert_manager.skip_tls_check,
+                        "notificationConfig":self.notification_config,
+                        "id":self.id
+                    })
+                }
+            }
+        }
+    }
+
     pub fn call(&self, context: Context) {
         trace!("target.call context- {context:?}");
-        let timeout = &self.timeout;
+        let timeout = &self.notification_config;
         let resolves = context.alert_info.alert_state;
         let mut state = timeout.state.lock().unwrap();
         trace!("target.call state- {state:?}");
@@ -205,7 +265,7 @@ impl Target {
         let sleep_and_check_if_call =
             move |timeout_state: Arc<Mutex<TimeoutState>>, current_state: AlertState| {
                 async move {
-                    tokio::time::sleep(timeout).await;
+                    tokio::time::sleep(Duration::from_secs(timeout * 60)).await;
 
                     let mut state = timeout_state.lock().unwrap();
 
@@ -276,8 +336,8 @@ fn call_target(target: TargetType, context: Context) {
 }
 
 #[derive(Debug, serde::Deserialize)]
-pub struct RepeatVerifier {
-    interval: Option<String>,
+pub struct NotificationConfigVerifier {
+    interval: Option<u64>,
     times: Option<usize>,
 }
 
@@ -288,7 +348,7 @@ pub struct TargetVerifier {
     #[serde(flatten)]
     pub target: TargetType,
     #[serde(default)]
-    pub repeat: Option<RepeatVerifier>,
+    pub notification_config: Option<NotificationConfigVerifier>,
     #[serde(default = "Ulid::new")]
     pub id: Ulid,
 }
@@ -304,26 +364,22 @@ impl TryFrom<TargetVerifier> for Target {
             timeout.times = Retry::Infinite
         }
 
-        if let Some(repeat_config) = value.repeat {
-            let interval = repeat_config
-                .interval
-                .map(|ref interval| humantime::parse_duration(interval))
-                .transpose()
-                .map_err(|err| err.to_string())?;
+        if let Some(notification_config) = value.notification_config {
+            let interval = notification_config.interval.map(|ref interval| *interval);
 
             if let Some(interval) = interval {
                 timeout.interval = interval
             }
 
-            if let Some(times) = repeat_config.times {
+            if let Some(times) = notification_config.times {
                 timeout.times = Retry::Finite(times)
             }
         }
 
         Ok(Target {
             name: value.name,
             target: value.target,
-            timeout,
+            notification_config: timeout,
             id: value.id,
         })
     }
@@ -518,8 +574,7 @@ impl CallableTarget for AlertManager {
 
 #[derive(Debug, serde::Serialize, serde::Deserialize, Clone)]
 pub struct Timeout {
-    #[serde(with = "humantime_serde")]
-    pub interval: Duration,
+    pub interval: u64,
     #[serde(default = "Retry::default")]
     pub times: Retry,
     #[serde(skip)]
@@ -529,7 +584,7 @@ pub struct Timeout {
 impl Default for Timeout {
     fn default() -> Self {
         Self {
-            interval: Duration::from_secs(60),
+            interval: 1,
             times: Retry::default(),
             state: Arc::<Mutex<TimeoutState>>::default(),
         }

src/handlers/http/modal/mod.rs

@@ -34,7 +34,7 @@ use tokio::sync::oneshot;
 use tracing::{error, info, warn};
 
 use crate::{
-    alerts::ALERTS,
+    alerts::{target::TARGETS, ALERTS},
     cli::Options,
     correlation::CORRELATIONS,
     oidc::Claims,
@@ -173,18 +173,20 @@ pub trait ParseableServer {
 
 pub async fn load_on_init() -> anyhow::Result<()> {
     // Run all loading operations concurrently
-    let (correlations_result, filters_result, dashboards_result, alerts_result) = future::join4(
-        async {
-            CORRELATIONS
-                .load()
-                .await
-                .context("Failed to load correlations")
-        },
-        async { FILTERS.load().await.context("Failed to load filters") },
-        async { DASHBOARDS.load().await.context("Failed to load dashboards") },
-        async { ALERTS.load().await.context("Failed to load alerts") },
-    )
-    .await;
+    let (correlations_result, filters_result, dashboards_result, alerts_result, targets_result) =
+        future::join5(
+            async {
+                CORRELATIONS
+                    .load()
+                    .await
+                    .context("Failed to load correlations")
+            },
+            async { FILTERS.load().await.context("Failed to load filters") },
+            async { DASHBOARDS.load().await.context("Failed to load dashboards") },
+            async { ALERTS.load().await.context("Failed to load alerts") },
+            async { TARGETS.load().await.context("Failed to load targets") },
+        )
+        .await;
 
     // Handle errors from each operation
     if let Err(e) = correlations_result {
@@ -203,6 +205,10 @@ pub async fn load_on_init() -> anyhow::Result<()> {
         error!("{err}");
     }
 
+    if let Err(err) = targets_result {
+        error!("{err}");
+    }
+
     Ok(())
 }
 

src/handlers/http/targets.rs

@@ -2,6 +2,7 @@ use actix_web::{
     web::{self, Json, Path},
     HttpRequest, Responder,
 };
+use itertools::Itertools;
 use ulid::Ulid;
 
 use crate::alerts::{
@@ -18,13 +19,18 @@ pub async fn post(
     // add to the map
     TARGETS.update(target.clone()).await?;
 
-    Ok(web::Json(target))
+    Ok(web::Json(target.mask()))
 }
 
 // GET /targets
 pub async fn list(_req: HttpRequest) -> Result<impl Responder, AlertError> {
     // add to the map
-    let list = TARGETS.list().await?;
+    let list = TARGETS
+        .list()
+        .await?
+        .into_iter()
+        .map(|t| t.mask())
+        .collect_vec();
 
     Ok(web::Json(list))
 }
@@ -35,7 +41,7 @@ pub async fn get(_req: HttpRequest, target_id: Path<Ulid>) -> Result<impl Respon
 
     let target = TARGETS.get_target_by_id(&target_id).await?;
 
-    Ok(web::Json(target))
+    Ok(web::Json(target.mask()))
 }
 
 // PUT /targets/{target_id}
@@ -47,7 +53,14 @@ pub async fn update(
     let target_id = target_id.into_inner();
 
     // if target_id does not exist, error
-    TARGETS.get_target_by_id(&target_id).await?;
+    let old_target = TARGETS.get_target_by_id(&target_id).await?;
+
+    // do not allow modifying name
+    if old_target.name != target.name {
+        return Err(AlertError::InvalidTargetModification(
+            "Can't modify target name".to_string(),
+        ));
+    }
 
     // esnure that the supplied target id is assigned to the target config
     target.id = target_id;
@@ -56,7 +69,7 @@ pub async fn update(
     // add to the map
     TARGETS.update(target.clone()).await?;
 
-    Ok(web::Json(target))
+    Ok(web::Json(target.mask()))
 }
 
 // DELETE /targets/{target_id}
@@ -68,5 +81,5 @@ pub async fn delete(
 
     let target = TARGETS.delete(&target_id).await?;
 
-    Ok(web::Json(target))
+    Ok(web::Json(target.mask()))
 }