helper-rust: avoid spurious service recreations with remote config disabled

cataphract · cataphract · commit f0bb0c862d9c · 2026-04-24T19:25:48.000+01:00
If remote config was disabled, client_init would inform the helper that
remote config is disabled. However, subsequent config_syncs would
neverthless send a remote config path (config_sync doesn't separately
send whether remote config is enabled). The path being non-empty the
helper-rust would deem remote config enabled and create a new service.

In practice, this did not actually result in remote config being
enabled, because sidecar being informed through config that it's
disabled, the shared memory path would not be written to. Neverthless,
this resulted in the creation of extra Service's and extra
initializations of the waf, reflected in a higher number of waf.init
metrics.
diff --git a/appsec/helper-rust/src/client/protocol.rs b/appsec/helper-rust/src/client/protocol.rs
@@ -103,18 +103,19 @@ pub struct RemoteConfigSettings {
 
 impl RemoteConfigSettings {
     pub fn new(enabled: bool, shmem_path: PathBuf) -> Self {
+        // we allow enabled with an empty path -- a config_sync can then fix the path
         Self {
             enabled,
-            shmem_path,
+            shmem_path: if enabled { shmem_path } else { PathBuf::new() },
         }
     }
 
-    fn enabled(&self) -> bool {
-        self.enabled && !self.shmem_path.as_os_str().is_empty()
+    pub fn can_be_enabled(&self) -> bool {
+        self.enabled
     }
 
     pub fn shmem_path(&self) -> Option<&Path> {
-        if self.enabled() {
+        if self.enabled && !self.shmem_path.as_os_str().is_empty() {
             Some(&self.shmem_path)
         } else {
             None
diff --git a/appsec/helper-rust/src/service.rs b/appsec/helper-rust/src/service.rs
@@ -671,14 +671,17 @@ impl ServiceFixedConfig {
             None
         };
 
-        if maybe_new_rem_cfg_path != self.rem_cfg_settings.shmem_path()
-            || args.telemetry_settings != self.telemetry_settings
-        {
+        // config_sync sends paths even if RC is disabled. We reject those
+        // if we got enabled=false in client_init.
+        let rem_cfg_path_changed = self.rem_cfg_settings.can_be_enabled()
+            && maybe_new_rem_cfg_path != self.rem_cfg_settings.shmem_path();
+
+        if rem_cfg_path_changed || args.telemetry_settings != self.telemetry_settings {
             let mut new_cfg = self.clone();
-            new_cfg.rem_cfg_settings = protocol::RemoteConfigSettings::new(
-                !new_rem_cfg_path.as_os_str().is_empty(),
-                new_rem_cfg_path,
-            );
+            if rem_cfg_path_changed {
+                new_cfg.rem_cfg_settings =
+                    protocol::RemoteConfigSettings::new(true, new_rem_cfg_path);
+            }
             new_cfg.telemetry_settings = args.telemetry_settings;
             Some(new_cfg)
         } else {
@@ -807,7 +810,7 @@ mod tests {
             },
             protocol::RemoteConfigSettings::new(false, PathBuf::from(format!("/tmp/test_{}", id))),
             protocol::TelemetrySettings {
-                service_name: "test".to_string(),
+                service_name: format!("test_{}", id),
                 env_name: "test".to_string(),
             },
         )
@@ -907,4 +910,46 @@ mod tests {
         drop(s2);
         drop(s1_new);
     }
+
+    #[test]
+    fn config_sync_does_not_enable_rc_when_client_init_disabled_it() {
+        // Simulates DD_REMOTE_CONFIG_ENABLED=0 in client_init: the path is
+        // non-empty but enabled=false. A later config_sync carrying the same
+        // (non-empty) path must NOT produce a new ServiceFixedConfig, because
+        // doing so would flip RC to enabled and trigger a WAF reload per
+        // PHP-FPM worker.
+        let telemetry = protocol::TelemetrySettings {
+            service_name: "svc".to_string(),
+            env_name: "env".to_string(),
+        };
+        let cfg = ServiceFixedConfig::new(
+            true,
+            protocol::WafSettings {
+                rules_file: Some(TEST_RULES_FILE.to_string()),
+                waf_timeout_us: Some(10000),
+                trace_rate_limit: 100,
+                obfuscator_key_regex: None,
+                obfuscator_value_regex: None,
+                schema_extraction: protocol::SchemaExtraction {
+                    enabled: false,
+                    sampling_period: 1.0,
+                },
+            },
+            // RC disabled by client_init, but the PHP side still has a path.
+            protocol::RemoteConfigSettings::new(false, PathBuf::from("/ddrc-test")),
+            telemetry.clone(),
+        );
+
+        let result = cfg.new_from_config_sync(protocol::ConfigSyncArgs {
+            rem_cfg_path: "/ddrc-test".to_string(),
+            telemetry_settings: telemetry,
+        });
+
+        assert!(
+            result.is_none(),
+            "config_sync with the same path must not create a new config when \
+             client_init disabled RC, but got: {:?}",
+            result
+        );
+    }
 }
