helper-rust: avoid unnecessary waf updates; remove old cfd before adding

cataphract · cataphract · commit 0623c3c37cd1 · 2026-04-17T17:29:22.000+01:00
diff --git a/appsec/helper-rust/src/rc.rs b/appsec/helper-rust/src/rc.rs
@@ -240,13 +240,26 @@ impl<'a> Iterator for ConfigIter<'a> {
     }
 }
 
+#[derive(Debug, PartialEq, Eq, Hash, Clone)]
+pub struct RcPath(String);
+impl RcPath {
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+}
+impl AsRef<str> for RcPath {
+    fn as_ref(&self) -> &str {
+        self.as_str()
+    }
+}
+
 #[derive(Debug, PartialEq, Eq)]
 pub struct Config<'a> {
     shm_path: &'a Path,
-    rc_path: String,
+    rc_path: RcPath,
 }
 impl<'a> Config<'a> {
-    pub fn rc_path(&self) -> &str {
+    pub fn rc_path(&self) -> &RcPath {
         &self.rc_path
     }
 
@@ -281,7 +294,7 @@ impl<'a> Config<'a> {
 
         Ok(Config {
             shm_path: Path::new(OsStr::from_bytes(shm_path)),
-            rc_path,
+            rc_path: RcPath(rc_path),
         })
     }
 
@@ -333,9 +346,9 @@ pub struct ParsedConfigKey {
 }
 
 impl ParsedConfigKey {
-    pub fn from_rc_path(rc_path: &str) -> Option<Self> {
+    pub fn from_rc_path(rc_path: &RcPath) -> Option<Self> {
         // Format: (datadog/<org_id> | employee)/<PRODUCT>/<config_id>/<name>
-        let parts: Vec<&str> = rc_path.split('/').collect();
+        let parts: Vec<&str> = rc_path.as_str().split('/').collect();
 
         if parts.len() >= 4 && parts[0] == "datadog" {
             // datadog/<org_id>/<PRODUCT>/<config_id>/...
diff --git a/appsec/helper-rust/src/service.rs b/appsec/helper-rust/src/service.rs
@@ -348,64 +348,47 @@ impl Service {
     ) -> anyhow::Result<()> {
         debug!("Applying config for runtime id {}", cfg_dir.runtime_id()?);
 
+        let new_configs = cfg_dir
+            .iter()
+            .map_err(|e| {
+                let any_error = e.context("Failed to start iteration over configs");
+                self.log_general_rc_error(&any_error);
+                any_error
+            })?
+            .map(|res_cfg| {
+                res_cfg
+                    .map_err(|e| {
+                        let any_error = e.context("Failure during config iteration");
+                        self.log_general_rc_error(&any_error);
+                        any_error
+                    })
+                    .map(|cfg| cfg.rc_path().clone())
+            })
+            .collect::<Result<HashSet<_>, anyhow::Error>>()?;
+
         let mut new_snapshot = (**self.config_snapshot.load()).clone();
-        let mut new_configs = HashSet::new();
         let mut waf_changed = false;
         let mut all_diagnostics = Vec::new();
         let mut rules_version: Option<String> = None;
 
-        let maybe_cfg_iter = cfg_dir.iter();
-        match maybe_cfg_iter {
-            Ok(cfg_iter) => {
-                for maybe_cfg in cfg_iter {
-                    // Handle each new/updated config
-                    let cfg = maybe_cfg?;
-                    let rc_path = cfg.rc_path();
-                    new_configs.insert(rc_path.to_string());
-
-                    let result = self.apply_config_file(
-                        &cfg,
-                        rc_path,
-                        state,
-                        &mut all_diagnostics,
-                        &mut rules_version,
-                        &mut new_snapshot,
-                        &mut waf_changed,
-                    );
-                    if let Err(e) = result {
-                        self.log_product_rc_error(rc_path, &self.logs_collector, e);
-                    }
-                }
-            }
-            Err(e) => {
-                self.log_general_rc_error(e.context("Failed to iterate over configs"));
-            }
-        }
-
-        for maybe_cfg in cfg_dir.iter()? {
-            let cfg = maybe_cfg?;
-            let rc_path = cfg.rc_path();
-            new_configs.insert(rc_path.to_string());
-        }
-
         // Handle removal of configs
         for old_path in state.last_configs.difference(&new_configs) {
-            if old_path.contains("/ASM_FEATURES/") {
+            if old_path.as_str().contains("/ASM_FEATURES/") {
                 state.asm_feature_config_manager.remove(old_path);
             } else {
-                let res = self.waf.remove_config(old_path);
+                let res = self.waf.remove_config(old_path.as_str());
                 match res {
                     Ok(true) => {
-                        debug!("Removed WAF config: {}", old_path);
+                        debug!("Removed WAF config: {:?}", old_path);
                         waf_changed = true;
                     }
                     Ok(false) => {
-                        warning!("No WAF config found to remove: {}", old_path);
+                        warning!("No WAF config found to remove: {:?}", old_path);
                     }
                     Err(e) => {
-                        self.log_general_rc_error(e.context(format!(
+                        self.log_general_rc_error(&e.context(format!(
                             concat!(
-                                "Failed to remove WAF config {}; ",
+                                "Failed to remove WAF config {:?}; ",
                                 "this should happen only when reading the default config"
                             ),
                             old_path
@@ -414,6 +397,28 @@ impl Service {
                 }
             }
         }
+
+        // cfg_dir iteration worked above, so it will presumably work here too
+        let cfg_iter = cfg_dir.iter()?;
+        for maybe_cfg in cfg_iter {
+            // Handle each config in the new config directory
+            let cfg = maybe_cfg?;
+            let rc_path = cfg.rc_path();
+
+            let result = self.apply_config_file(
+                &cfg,
+                rc_path,
+                state,
+                &mut all_diagnostics,
+                &mut rules_version,
+                &mut new_snapshot,
+                &mut waf_changed,
+            );
+            if let Err(e) = result {
+                self.log_product_rc_error(rc_path, &self.logs_collector, e);
+            }
+        }
+
         state.last_configs = new_configs;
 
         // Telemetry waf.config_errors metrics and telemetry logs - always process
@@ -445,7 +450,7 @@ impl Service {
                 }
                 Err(e) => {
                     self.log_general_rc_error(
-                        e.context("Failed to rebuild WAF after config update"),
+                        &e.context("Failed to rebuild WAF after config update"),
                     );
                     false
                 }
@@ -496,19 +501,24 @@ impl Service {
     fn apply_config_file(
         &self,
         cfg: &rc::Config,
-        rc_path: &str,
+        rc_path: &rc::RcPath,
         state: &mut RcUpdateState,
         all_diagnostics: &mut Vec<(
-            String,
+            rc::RcPath,
             libddwaf::object::WafOwnedDefaultAllocator<libddwaf::object::WafMap>,
         )>,
         rules_version: &mut Option<String>,
         new_snapshot: &mut ConfigSnapshot,
         waf_changed: &mut bool,
     ) -> anyhow::Result<()> {
         let product = cfg.product();
+        if state.last_configs.contains(rc_path) {
+            debug!("Config already applied on previous update: {:?}", rc_path);
+            return Ok(());
+        }
+
         debug!(
-            "Processing config: rc_path={}, product={}",
+            "Processing config: rc_path={:?}, product={}",
             rc_path,
             product.name()
         );
@@ -518,33 +528,35 @@ impl Service {
                 let data = unsafe { shmem.as_slice() };
                 state
                     .asm_feature_config_manager
-                    .add(rc_path.to_string(), data)?;
+                    .add(rc_path.as_str().to_string(), data)?;
                 Ok(())
             }
             "ASM_DD" | "ASM" | "ASM_DATA" => {
                 let shmem = cfg.read()?;
                 let data = unsafe { shmem.as_slice() };
 
                 let ruleset = waf_ruleset::WafRuleset::from_slice(data)
-                    .with_context(|| format!("Failed to parse WAF config for {}", rc_path))?;
+                    .with_context(|| format!("Failed to parse WAF config for {:?}", rc_path))?;
 
                 let waf_obj: libddwaf::object::WafObject = ruleset.into();
                 let mut diagnostics = Default::default();
 
-                let upd_result =
-                    self.waf
-                        .add_or_update_config(rc_path, &waf_obj, Some(&mut diagnostics));
+                let upd_result = self.waf.add_or_update_config(
+                    rc_path.as_str(),
+                    &waf_obj,
+                    Some(&mut diagnostics),
+                );
 
                 if upd_result.is_ok() {
-                    debug!("Added/updated WAF config: {}", rc_path);
+                    debug!("Added/updated WAF config: {:?}", rc_path);
                     if product.name() == "ASM_DD" {
                         *rules_version = waf_diag::extract_ruleset_version(&diagnostics);
                         *new_snapshot = new_snapshot.with_new_rules_version(rules_version.clone());
                     }
                     *waf_changed = true;
                 }
 
-                all_diagnostics.push((rc_path.to_string(), diagnostics));
+                all_diagnostics.push((rc_path.clone(), diagnostics));
                 upd_result
             }
             _ => {
@@ -556,12 +568,12 @@ impl Service {
 
     fn log_product_rc_error(
         &self,
-        rc_path: &str,
+        rc_path: &rc::RcPath,
         logs_submitter: &TelemetryLogsCollector,
         error: anyhow::Error,
     ) {
         let message = format!(
-            "Failed to apply config {} in service with config {:?}: {:#}",
+            "Failed to apply config {:?} in service with config {:?}: {:#}",
             rc_path, self.fixed_config, error
         );
 
@@ -589,13 +601,13 @@ impl Service {
             });
         } else {
             error!(
-                "Can't parse RC path: {}; no submission of telemetry log",
+                "Can't parse RC path: {:?}; no submission of telemetry log",
                 rc_path
             );
         }
     }
 
-    fn log_general_rc_error(&self, error: anyhow::Error) {
+    fn log_general_rc_error(&self, error: &anyhow::Error) {
         let message = format!(
             "Failed to apply config for service with config {:?}: {:#}",
             self.fixed_config, error
@@ -755,7 +767,7 @@ impl ServiceManagerInner {
 
 struct RcUpdateState {
     poller: Option<rc::ConfigPoller>,
-    last_configs: HashSet<String>,
+    last_configs: HashSet<rc::RcPath>,
     asm_feature_config_manager: AsmFeatureConfigManager,
     pending_telemetry_metrics: TelemetryMetricsCollector,
     pending_init_diagnostics_legacy: Option<InitDiagnosticsLegacy>,
diff --git a/appsec/helper-rust/src/service/waf_diag.rs b/appsec/helper-rust/src/service/waf_diag.rs
@@ -1,6 +1,6 @@
 use crate::{
     client::log::{debug, warning},
-    rc::ParsedConfigKey,
+    rc,
     telemetry::{
         LogLevel, TelemetryLog, TelemetryLogsCollector, TelemetryMetricSubmitter, TelemetryTags,
         WAF_CONFIG_ERRORS,
@@ -22,26 +22,26 @@ const DIAGNOSTIC_KEYS: &[&str] = &[
 ];
 
 pub fn report_diagnostics_errors(
-    rc_path: &str,
+    rc_path: &rc::RcPath,
     diagnostics: &libddwaf::object::WafOwnedDefaultAllocator<libddwaf::object::WafMap>,
     rules_version: &str,
     metric_submitter: &mut impl TelemetryMetricSubmitter,
     log_submitter: &TelemetryLogsCollector,
 ) {
     use libddwaf::object::WafObjectType;
 
-    let maybe_parsed_key = ParsedConfigKey::from_rc_path(rc_path);
+    let maybe_parsed_key = rc::ParsedConfigKey::from_rc_path(rc_path);
     let parsed_key = match maybe_parsed_key {
         Some(parsed_key) => {
             debug!(
-                "Processing diagnostics for {}: {} keys",
-                rc_path,
+                "Processing diagnostics for {:?}: {} keys",
+                rc_path.as_str(),
                 diagnostics.len()
             );
             parsed_key
         }
         None => {
-            warning!("Failed to parse config key for {}", rc_path,);
+            warning!("Failed to parse config key for {:?}", rc_path);
             return;
         }
     };
@@ -53,7 +53,7 @@ pub fn report_diagnostics_errors(
         let value = keyed.value();
         if value.object_type() != WafObjectType::Map {
             warning!(
-                "Diagnostic key {} for {} is not a map, skipping",
+                "Diagnostic key {} for {:?} is not a map, skipping",
                 config_key,
                 rc_path
             );
@@ -67,7 +67,7 @@ pub fn report_diagnostics_errors(
         }
 
         debug!(
-            "Diagnostic {} for {} has {} entries",
+            "Diagnostic {} for {:?} has {} entries",
             config_key,
             rc_path,
             map.len()
@@ -245,7 +245,7 @@ pub fn extract_init_diagnostics_legacy(
 
 fn submit_diagnostic_log(
     log_submitter: &TelemetryLogsCollector,
-    parsed_key: &ParsedConfigKey,
+    parsed_key: &rc::ParsedConfigKey,
     config_key: &str,
     suffix: &str,
     level: LogLevel,
diff --git a/appsec/tests/integration/build.gradle b/appsec/tests/integration/build.gradle
@@ -671,6 +671,10 @@ def runMainTask = { String phpVersion, String variant ->
             includeEngines('junit-jupiter')
             excludeEngines('junit-vintage')
 
+            if (!project.hasProperty('slowTests')) {
+                excludeTags('slow')
+            }
+
             if (isMusl) {
                 includeTags('musl')
             }
@@ -770,6 +774,10 @@ if (project.hasProperty('testClass')) {
         it.useJUnitPlatform {
             includeEngines('junit-jupiter')
             excludeEngines('junit-vintage')
+
+            if (!project.hasProperty('slowTests')) {
+                excludeTags('slow')
+            }
         }
 
         it.testClassesDirs = sourceSets.test.output.classesDirs
diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy
