preserve zero length strings

Author: webern

bin/agent-data-plane/src/internal/remote_agent.rs

@@ -310,10 +310,21 @@ async fn run_config_stream_event_loop(
                         Some(config_event::Event::Snapshot(snapshot)) => {
                             Some(ConfigUpdate::Snapshot(snapshot_to_settings(&snapshot)))
                         }
-                        Some(config_event::Event::Update(update)) => update
-                            .setting
-                            .as_ref()
-                            .map(|setting| ConfigUpdate::Partial(setting_to_config_setting(setting))),
+                        Some(config_event::Event::Update(update)) => {
+                            update.setting.as_ref().and_then(|setting| {
+                                match setting_to_config_setting(setting) {
+                                    Some(converted) => Some(ConfigUpdate::Partial(converted)),
+                                    None => {
+                                        // Do not inject JSON null: typed fields interpret it as an invalid value.
+                                        debug!(
+                                            key = %setting.key,
+                                            "Ignoring a configuration update for a declared key with no value."
+                                        );
+                                        None
+                                    }
+                                }
+                            })
+                        }
                         None => {
                             error!("Received a configuration update event with no data.");
                             None
@@ -338,33 +349,35 @@ async fn run_config_stream_event_loop(
     }
 }
 
-/// Agent source names for values the Agent supplied itself rather than an operator: `default` is a
-/// schema default value, and `schema` is a declared key that has no value at all.
-const AGENT_UNSET_SOURCES: [&str; 2] = ["default", "schema"];
+// Source for a value supplied by the Agent's schema default.
+const AGENT_DEFAULT_SOURCE: &str = "default";
 
-/// Converts a setting from the Agent's RPC wire protocol to our `ConfigSetting` type.
-///
-///
-/// An unrecognized `source` is treated as `Provenance::Explicit` because it is safer.
-/// `Provenance::Default` values can be overwritten downstream so we want to be sure when labeling a
-/// value as such.
-fn setting_to_config_setting(setting: &AgentConfigSetting) -> ConfigSetting {
-    let provenance = if AGENT_UNSET_SOURCES.contains(&setting.source.as_str()) {
+// Source for a schema-declared key with no configured value.
+const AGENT_DECLARED_ONLY_SOURCE: &str = "schema";
+
+// Omit settings without values; JSON null is not a missing-value sentinel for typed fields.
+fn setting_to_config_setting(setting: &AgentConfigSetting) -> Option<ConfigSetting> {
+    if setting.source == AGENT_DECLARED_ONLY_SOURCE {
+        return None;
+    }
+
+    let value = proto_value_to_serde_value(&setting.value);
+    if value.is_null() {
+        return None;
+    }
+
+    let provenance = if setting.source == AGENT_DEFAULT_SOURCE {
         Provenance::Default
     } else {
         Provenance::Explicit
     };
 
-    ConfigSetting::new(
-        setting.key.clone(),
-        proto_value_to_serde_value(&setting.value),
-        provenance,
-    )
+    Some(ConfigSetting::new(setting.key.clone(), value, provenance))
 }
 
-/// Converts a `ConfigSnapshot` into the settings it carries.
+// A snapshot must not pass valueless settings to the typed configuration layer.
 fn snapshot_to_settings(snapshot: &ConfigSnapshot) -> Vec<ConfigSetting> {
-    snapshot.settings.iter().map(setting_to_config_setting).collect()
+    snapshot.settings.iter().filter_map(setting_to_config_setting).collect()
 }
 
 /// Recursively converts a `google::protobuf::Value` into a `serde_json::Value`.
@@ -908,32 +921,77 @@ mod tests {
     }
 
     #[test]
-    fn agent_supplied_sources_are_marked_as_defaults() {
-        for source in AGENT_UNSET_SOURCES {
-            let setting = setting_to_config_setting(&agent_setting(source, "dd_url", "https://app.datadoghq.com"));
+    fn an_agent_default_is_marked_as_a_default() {
+        let setting = setting_to_config_setting(&agent_setting(
+            AGENT_DEFAULT_SOURCE,
+            "dd_url",
+            "https://app.datadoghq.com",
+        ))
+        .expect("a defaulted setting has a value");
+
+        assert_eq!(setting.key, "dd_url");
+        assert_eq!(setting.value, Value::from("https://app.datadoghq.com"));
+        assert_eq!(setting.provenance, Provenance::Default);
+    }
 
-            assert_eq!(setting.key, "dd_url");
-            assert_eq!(setting.value, Value::from("https://app.datadoghq.com"));
-            assert_eq!(
-                setting.provenance,
-                Provenance::Default,
-                "source {source} should be a default"
+    #[test]
+    fn a_declared_key_with_no_value_is_absent() {
+        // Both an omitted protobuf value and the schema-only source represent an unset key.
+        assert!(setting_to_config_setting(&AgentConfigSetting {
+            source: AGENT_DECLARED_ONLY_SOURCE.to_string(),
+            key: "api_key".to_string(),
+            value: None,
+        })
+        .is_none());
+
+        assert!(setting_to_config_setting(&agent_setting(AGENT_DECLARED_ONLY_SOURCE, "api_key", "")).is_none());
+    }
+
+    #[test]
+    fn a_valueless_setting_is_absent_whatever_its_source() {
+        for source in [AGENT_DEFAULT_SOURCE, "file", "remote-config"] {
+            assert!(
+                setting_to_config_setting(&AgentConfigSetting {
+                    source: source.to_string(),
+                    key: "api_key".to_string(),
+                    value: Some(prost_types::Value {
+                        kind: Some(Kind::NullValue(0)),
+                    }),
+                })
+                .is_none(),
+                "source {source} with a null value should be absent"
             );
         }
     }
 
+    #[test]
+    fn an_empty_string_value_is_kept_with_its_provenance() {
+        // An empty string is still a value; provenance comes from its source, not its content.
+        let setting =
+            setting_to_config_setting(&agent_setting("file", "site", "")).expect("an empty string is still a value");
+
+        assert_eq!(setting.value, Value::from(""));
+        assert_eq!(setting.provenance, Provenance::Explicit);
+
+        let setting = setting_to_config_setting(&agent_setting(AGENT_DEFAULT_SOURCE, "site", ""))
+            .expect("an empty string is still a value");
+
+        assert_eq!(setting.value, Value::from(""));
+        assert_eq!(setting.provenance, Provenance::Default);
+    }
+
     #[test]
     fn operator_supplied_sources_are_marked_as_explicit() {
-        // The last source is deliberately not one the Agent publishes today: an unrecognized source is
-        // treated as a real input rather than silently discarded as a default.
+        // Unknown sources are treated as explicit inputs rather than defaults.
         for source in [
             "file",
             "environment-variable",
             "remote-config",
             "cli",
             "source-from-the-future",
         ] {
-            let setting = setting_to_config_setting(&agent_setting(source, "dd_url", "https://app.datadoghq.eu"));
+            let setting = setting_to_config_setting(&agent_setting(source, "dd_url", "https://app.datadoghq.eu"))
+                .expect("an explicit setting has a value");
 
             assert_eq!(
                 setting.provenance,
@@ -944,13 +1002,14 @@ mod tests {
     }
 
     #[test]
-    fn snapshot_settings_keep_their_order_values_and_provenance() {
+    fn snapshot_settings_keep_order_values_and_provenance_and_drop_valueless_keys() {
         let snapshot = ConfigSnapshot {
             origin: "core-agent".to_string(),
             sequence_id: 1,
             settings: vec![
                 agent_setting("file", "site", "datadoghq.eu"),
                 agent_setting("default", "dd_url", "https://app.datadoghq.com"),
+                agent_setting(AGENT_DECLARED_ONLY_SOURCE, "api_key", ""),
             ],
         };
 

lib/agent-data-plane-config-system/src/translators/datadog_translator.rs

@@ -28,7 +28,7 @@ use agent_data_plane_config::domains::otlp::{
     DEFAULT_GRPC_MAX_RECV_MSG_SIZE_MIB,
 };
 use agent_data_plane_config::shared::ForwarderHttpProtocol;
-use agent_data_plane_config::{ConfigValue, Provenance, SalukiConfiguration};
+use agent_data_plane_config::{ConfigValue, SalukiConfiguration};
 use bytesize::ByteSize;
 use datadog_agent_config::{drive, DatadogConfigWitness, DatadogConfiguration, TranslateError, TranslateErrors};
 
@@ -73,18 +73,6 @@ impl<'a> DatadogTranslator<'a> {
     fn record_error(&mut self, error: TranslateError) {
         self.errors.push(error);
     }
-
-    /// Returns whether an input set `key`'s value explicitly, treating an empty value as a default.
-    ///
-    /// An empty string expresses no intent: it names no site and no URL. Recording it as a default
-    /// keeps a consumer from having to re-check for emptiness before honoring an override.
-    fn provenance_if_non_empty(&self, key: &str, value: &str) -> Provenance {
-        if value.is_empty() {
-            Provenance::Default
-        } else {
-            self.sources.provenance(key)
-        }
-    }
 }
 
 /// Returns `None` for an empty `s`; otherwise returns `Some(s)`.
@@ -458,11 +446,10 @@ impl DatadogConfigWitness for DatadogTranslator<'_> {
     }
 
     fn consume_dd_url(&mut self, value: String) {
-        // The Core Agent streams this key at its schema default even when the operator configured
-        // only `site`, so the URL is carried through as-is and provenance records whether it is an
-        // override anyone set. Programmatic overrides via `EndpointConfiguration::set_dd_url`
-        // (MRF, cluster-agent forwarder) bypass this translator entirely.
-        let provenance = self.provenance_if_non_empty("dd_url", &value);
+        // The Agent may send its default URL even when the operator configured only `site`; retain
+        // the value and use provenance to decide whether it overrides `site`. Programmatic
+        // overrides via `EndpointConfiguration::set_dd_url` bypass this translator.
+        let provenance = self.sources.provenance("dd_url");
         self.config.shared.endpoints.dd_url = ConfigValue::new(value, provenance);
     }
 
@@ -1062,7 +1049,7 @@ impl DatadogConfigWitness for DatadogTranslator<'_> {
     }
 
     fn consume_site(&mut self, value: String) {
-        let provenance = self.provenance_if_non_empty("site", &value);
+        let provenance = self.sources.provenance("site");
         self.config.shared.endpoints.site = ConfigValue::new(value, provenance);
     }
 
@@ -1368,11 +1355,19 @@ mod tests {
     }
 
     #[test]
-    fn an_empty_endpoint_value_is_not_explicit() {
-        // An empty string names no site and no URL, so it expresses no intent no matter which source
-        // supplied it.
+    fn an_empty_endpoint_value_keeps_its_provenance() {
+        // Empty endpoint strings retain the provenance of the source that supplied them.
         let (config, errors) = translate_explicit(json!({ "site": "", "dd_url": "" }));
 
+        assert!(errors.is_none());
+        assert_explicit(&config.shared.endpoints.site, "");
+        assert_explicit(&config.shared.endpoints.dd_url, "");
+
+        let (config, errors) = translate_stream(&[
+            ("site", json!(""), StreamProvenance::Default),
+            ("dd_url", json!(""), StreamProvenance::Default),
+        ]);
+
         assert!(errors.is_none());
         assert_defaulted(&config.shared.endpoints.site, "");
         assert_defaulted(&config.shared.endpoints.dd_url, "");