fix(config): permissively coerce scalar config leaves - #2325
Conversation
Binary Size Analysis (Agent Data Plane)Baseline: 6825bb3 · Comparison: fa1f541 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
There was a problem hiding this comment.
The new coercion layer still rejects two forms accepted by the Agent’s pinned cast implementation: floating-point booleans and zero-fraction integer strings. Either form can make ADP’s strict configuration gate fail while the Agent accepts the same setting.
🤖 Datadog Autotest · Commit bcb9c8c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
| .await | ||
| .expect("scalars in Agent-castable forms boot"); | ||
|
|
||
| assert_eq!(system.config().shared.metrics_encoding.v3_series_mode.mode, "true"); |
There was a problem hiding this comment.
What's going on here? Are we storing system.config().shared.metrics_encoding.v3_series_mode.mode as a String instead of a bool? Why? In typed config we should be storing values in system-native types (i.e. as a primitive boolean) not as a config-friendly-string that the system later has to coerce.
| assert_eq!(system.config().shared.metrics_encoding.v3_series_mode.mode, "true"); | |
| assert_eq!(system.config().shared.metrics_encoding.v3_series_mode.mode, true); |
There was a problem hiding this comment.
Turns out this field is not a boolean. It is an enum (defined in schema documentation rather than typed), with the values true, false and datadog_only. So ea9db67 models it as an enum following an established pattern we have for these and documents the pattern in the config-system skill.
aqian01
left a comment
There was a problem hiding this comment.
I think the start up path still uses the raw config
Code trace:
- passes raw map to dogstatsd
- Uses the raw config
- Constructed from raw config
- And the field requires u16
Ran a regression test to confirm
#[tokio::test]
async fn dogstatsd_accepts_quoted_port() {
let values = json!({ "dogstatsd_port": "8126" });
let (config, _) = ConfigurationLoader::for_tests(Some(values), None, false).await;
let config = DogStatsDConfiguration::from_configuration(&config)
.expect("quoted port should work");
assert_eq!(config.port, 8126);
}
This is the behavior on main as well, confirmed by running your test there. This PR does not change any raw config path behavior. It makes the typed configuration model coerce scalar leaves the way the Agent does, which will cover |
137b8c2 to
7f6ad8f
Compare
…ss (#2317) ## Human Summary This migrates all components affected by #1965 (we were unable to detect explicitly set to default vs default config values) to typed config to pick up the fix provided in #2279. After implementation I ran a clean-room Opus audit with this prompt: > please do a clean-room audit of this commit `f4ae503e807b398ca55ee3f6275f554889cff645` > > Our goal is to correct a previous incorrect behavior in which we were not sensitive to whether a configuration value was set by the Agent as a default or whether the customer explicitly set the value (and happened to set it to the default value). > > Other than that specific intentional bug fix, no other behavioral changes are intended. > > Validate that only the desired behavioral change has occurred and that configuration defaults are in-tact. > > Read AGENTS.md and /config-system It only found two actionable items, which I fixed. One was that we were previously rejecting empty API keys, so I retained that behavior. The other was that an MRF endpoint consisting only of whitespace could be treated as a valid setting. Also fixed. All defaults stood up to scrutiny. Edit: @aqian01 found a flaw that went undetected which became #2325. ### Review Guide I tried breaking this up, which is why there are a bunch of subissues being closed all at once, but it did not divide very well. As such, the diff a bit large: Focus on these files: - `lib/saluki-components/*` - `bin/agent-data-plane/src/cli/run.rs` - `lib/agent-data-plane-config*` Deprioritize generated code and inventory churn in: - `lib/datadog-agent/*` ## AI Summary Migrate Datadog endpoint, retry, proxy, forwarder, metrics encoder, MRF, and Cluster Agent configuration consumers from raw `GenericConfiguration` deserialization to resolved typed configuration. Primary endpoint resolution now happens once in the typed configuration layer: - A default-sourced `dd_url` no longer shadows `site`. - An explicitly configured `dd_url`, including the schema-default URL, remains an override. - MRF and Cluster Agent destination overrides cannot be overwritten by global endpoint settings. - Retry queue size precedence uses configuration provenance, so explicit zero values are preserved. - Raw configuration access remains only for live API-key refresh, secrets retry behavior, and the `run_path` compatibility fallback whose schema default is still an unresolved placeholder. The schema inventories, generated registries, smoke-test metadata, and component tests are updated for the typed consumers. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - unit tests updated and created throughout - integration test modified to test this behavior. this was proven to have caught the original bug (`red -> green`) ## References - Closes: #1965 - Closes: #2310 - Closes: #2312 - Closes: #2313 - Closes: #2314 - Closes: #2315 - Closes: #2316 - Related to: #2279
The Agent never reads a setting as the type its YAML holds: GetBool,
GetInt, GetFloat64, and GetString each cast the stored value through
spf13/cast. Permissiveness is therefore a property of a leaf's declared
type, not of the key, so `dogstatsd_port: "8125"` and a string-typed
leaf written as a YAML boolean are configurations the Agent accepts.
The generated Datadog source model took each leaf's JSON type
literally, so those spellings failed deserialization. That aborts the
strict startup gate, and at runtime it rejects the whole Agent snapshot,
holding every other key at its last-known-good value.
Port cast.To{Bool,Int64,Float64,String}E into cast_de and have codegen
attach one by leaf type, keeping the schema's type as the field type.
env_decode now shares those parsers, so one accept-set serves the file,
the environment, and the Agent stream. A value the cast cannot convert
stays a hard error rather than the Agent's silent zero value.
Every generated field is classified, and an unrecognized type fails the
build, so a schema change cannot quietly ship a leaf that rejects input
the Agent accepts. This subsumes the overlay's `input_shape` metadata,
whose one shape (a byte size written as an integer) is what a string
leaf now accepts by type, so it and string_de are removed.
Type-directed coercion cannot cover a setting whose values are a closed
set rather than a type, so model `use_v3_api.series.enabled` as a
`V3SeriesMode` enum. Its `FromStr` lists the spellings the Agent's
evaluator interprets and each variant serializes back to one of them, so
the translator parses once instead of every consumer re-parsing a
`String`. The Agent's evaluator defines this setting's recovery itself:
it warns and routes to the older intake. Both the translator and the raw
path do the same rather than record a translation error, so the strict
startup gate does not reject a configuration the Agent runs with. A
per-endpoint mode arrives as raw JSON, so render it through the same
string cast the Agent reads it with, and reject only a compound value.
…ss (#2317) ## Human Summary This migrates all components affected by #1965 (we were unable to detect explicitly set to default vs default config values) to typed config to pick up the fix provided in #2279. After implementation I ran a clean-room Opus audit with this prompt: > please do a clean-room audit of this commit `f4ae503e807b398ca55ee3f6275f554889cff645` > > Our goal is to correct a previous incorrect behavior in which we were not sensitive to whether a configuration value was set by the Agent as a default or whether the customer explicitly set the value (and happened to set it to the default value). > > Other than that specific intentional bug fix, no other behavioral changes are intended. > > Validate that only the desired behavioral change has occurred and that configuration defaults are in-tact. > > Read AGENTS.md and /config-system It only found two actionable items, which I fixed. One was that we were previously rejecting empty API keys, so I retained that behavior. The other was that an MRF endpoint consisting only of whitespace could be treated as a valid setting. Also fixed. All defaults stood up to scrutiny. Edit: @aqian01 found a flaw that went undetected which became #2325. ### Review Guide I tried breaking this up, which is why there are a bunch of subissues being closed all at once, but it did not divide very well. As such, the diff a bit large: Focus on these files: - `lib/saluki-components/*` - `bin/agent-data-plane/src/cli/run.rs` - `lib/agent-data-plane-config*` Deprioritize generated code and inventory churn in: - `lib/datadog-agent/*` ## AI Summary Migrate Datadog endpoint, retry, proxy, forwarder, metrics encoder, MRF, and Cluster Agent configuration consumers from raw `GenericConfiguration` deserialization to resolved typed configuration. Primary endpoint resolution now happens once in the typed configuration layer: - A default-sourced `dd_url` no longer shadows `site`. - An explicitly configured `dd_url`, including the schema-default URL, remains an override. - MRF and Cluster Agent destination overrides cannot be overwritten by global endpoint settings. - Retry queue size precedence uses configuration provenance, so explicit zero values are preserved. - Raw configuration access remains only for live API-key refresh, secrets retry behavior, and the `run_path` compatibility fallback whose schema default is still an unresolved placeholder. The schema inventories, generated registries, smoke-test metadata, and component tests are updated for the typed consumers. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - unit tests updated and created throughout - integration test modified to test this behavior. this was proven to have caught the original bug (`red -> green`) ## References - Closes: #1965 - Closes: #2310 - Closes: #2312 - Closes: #2313 - Closes: #2314 - Closes: #2315 - Closes: #2316 - Related to: #2279
## Human Summary In #2317 @aqian01 detected a flaw in the typed config deserialization mechanism. The ported, permissive, Viper type coercions that the Agent uses were only being applied to environment variables. This means that when we parsed a config file ourselves, we would not have applied those permissive coercions. This is now fixed so that we more completely mirror Agent config parsing behavior when migrating to typed config. ## AI Summary The Datadog Agent accepts configuration values through `cast`, so a leaf's declared type determines which alternate spellings it accepts. `dogstatsd_port: "8125"` and a schema-`string` leaf written as a YAML boolean (`use_v3_api.series.enabled: true`, read with `GetString`) are both valid Agent configuration. ADP's generated model instead deserialized each leaf strictly according to its schema type, causing startup translation to fail or rejecting the entire runtime configuration snapshot. This change: - Adds serde deserializers for boolean, integer, float, and string schema scalar types. - Adds a build-time `permissivize` pass that applies the appropriate deserializer to every generated scalar leaf based on its generated Rust type. - Makes `env_decode` use the same parsers, aligning file, environment, and configuration-stream inputs. - Fails code generation for unrecognized leaf types, so new schema types require an explicit coercion decision. - Retains hard errors for values that cannot be converted, rather than accepting `cast`'s zero values. - Removes the now-unnecessary `input_shape: string_or_integer` metadata and related types. Field types remain unchanged; only deserialization behavior is permissive. Numeric strings are accepted in decimal form only. The generated model diff adds deserializer attributes, and the change covers the full generated scalar model rather than individual keys. ## Change Type - [x] Bug fix ## How did you test this PR? - Unit tests cover accepted and rejected spellings for each scalar type. - A schema-driven test exercises every scalar leaf with a castable value and verifies the coerced value through a serialization round-trip. - Translation-gate tests cover a boolean on the V3 series mode string leaf and a quoted `dogstatsd_port`. - `make build-schema-overlay`, `cargo check --workspace --tests`, and `cargo clippy --workspace --tests` pass; config crate tests pass. ## References - Related: #2317 --- ## Note: #2317 merged into the wrong base I merged #2317 into `m/confra-cast` (this PR) instead of `main`. This PR now carries both changes. The #2317 description follows verbatim. --- # Merged from #2317: fix(config): migrate endpoints to typed config for provenance awareness ## Human Summary This migrates all components affected by #1965 (we were unable to detect explicitly set to default vs default config values) to typed config to pick up the fix provided in #2279. After implementation I ran a clean-room Opus audit with this prompt: > please do a clean-room audit of this commit `f4ae503e807b398ca55ee3f6275f554889cff645` > > Our goal is to correct a previous incorrect behavior in which we were not sensitive to whether a configuration value was set by the Agent as a default or whether the customer explicitly set the value (and happened to set it to the default value). > > Other than that specific intentional bug fix, no other behavioral changes are intended. > > Validate that only the desired behavioral change has occurred and that configuration defaults are in-tact. > > Read AGENTS.md and /config-system It only found two actionable items, which I fixed. One was that we were previously rejecting empty API keys, so I retained that behavior. The other was that an MRF endpoint consisting only of whitespace could be treated as a valid setting. Also fixed. All defaults stood up to scrutiny. Edit: @aqian01 found a flaw that went undetected which became #2325. ### Review Guide I tried breaking this up, which is why there are a bunch of subissues being closed all at once, but it did not divide very well. As such, the diff a bit large: Focus on these files: - `lib/saluki-components/*` - `bin/agent-data-plane/src/cli/run.rs` - `lib/agent-data-plane-config*` Deprioritize generated code and inventory churn in: - `lib/datadog-agent/*` ## AI Summary Migrate Datadog endpoint, retry, proxy, forwarder, metrics encoder, MRF, and Cluster Agent configuration consumers from raw `GenericConfiguration` deserialization to resolved typed configuration. Primary endpoint resolution now happens once in the typed configuration layer: - A default-sourced `dd_url` no longer shadows `site`. - An explicitly configured `dd_url`, including the schema-default URL, remains an override. - MRF and Cluster Agent destination overrides cannot be overwritten by global endpoint settings. - Retry queue size precedence uses configuration provenance, so explicit zero values are preserved. - Raw configuration access remains only for live API-key refresh, secrets retry behavior, and the `run_path` compatibility fallback whose schema default is still an unresolved placeholder. The schema inventories, generated registries, smoke-test metadata, and component tests are updated for the typed consumers. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? - unit tests updated and created throughout - integration test modified to test this behavior. this was proven to have caught the original bug (`red -> green`) ## References - Closes: #1965 - Closes: #2310 - Closes: #2312 - Closes: #2313 - Closes: #2314 - Closes: #2315 - Closes: #2316 - Related to: #2279 Co-authored-by: matt.briggs <matt.briggs@datadoghq.com> 79f5b0c

Human Summary
In #2317 @aqian01 detected a flaw in the typed config deserialization mechanism. The ported, permissive, Viper type coercions that the Agent uses were only being applied to environment variables. This means that when we parsed a config file ourselves, we would not have applied those permissive coercions. This is now fixed so that we more completely mirror Agent config parsing behavior when migrating to typed config.
AI Summary
The Datadog Agent accepts configuration values through
cast, so a leaf's declared type determines which alternate spellings it accepts.dogstatsd_port: "8125"and a schema-stringleaf written as a YAML boolean (use_v3_api.series.enabled: true, read withGetString) are both valid Agent configuration. ADP's generated model instead deserialized each leaf strictly according to its schema type, causing startup translation to fail or rejecting the entire runtime configuration snapshot.This change:
permissivizepass that applies the appropriate deserializer to every generated scalar leaf based on its generated Rust type.env_decodeuse the same parsers, aligning file, environment, and configuration-stream inputs.cast's zero values.input_shape: string_or_integermetadata and related types.Field types remain unchanged; only deserialization behavior is permissive. Numeric strings are accepted in decimal form only. The generated model diff adds deserializer attributes, and the change covers the full generated scalar model rather than individual keys.
Change Type
How did you test this PR?
dogstatsd_port.make build-schema-overlay,cargo check --workspace --tests, andcargo clippy --workspace --testspass; config crate tests pass.References
Note: #2317 merged into the wrong base
I merged #2317 into
m/confra-cast(this PR) instead ofmain. This PR now carries both changes. The #2317 description follows verbatim.Merged from #2317: fix(config): migrate endpoints to typed config for provenance awareness
Human Summary
This migrates all components affected by #1965 (we were unable to detect explicitly set to default vs default config values) to typed config to pick up the fix provided in #2279.
After implementation I ran a clean-room Opus audit with this prompt:
It only found two actionable items, which I fixed. One was that we were previously rejecting empty API keys, so I retained that behavior. The other was that an MRF endpoint consisting only of whitespace could be treated as a valid setting. Also fixed. All defaults stood up to scrutiny.
Edit: @aqian01 found a flaw that went undetected which became #2325.
Review Guide
I tried breaking this up, which is why there are a bunch of subissues being closed all at once, but it did not divide very well. As such, the diff a bit large:
Focus on these files:
lib/saluki-components/*bin/agent-data-plane/src/cli/run.rslib/agent-data-plane-config*Deprioritize generated code and inventory churn in:
lib/datadog-agent/*AI Summary
Migrate Datadog endpoint, retry, proxy, forwarder, metrics encoder, MRF, and Cluster Agent configuration consumers from raw
GenericConfigurationdeserialization to resolved typed configuration.Primary endpoint resolution now happens once in the typed configuration layer:
dd_urlno longer shadowssite.dd_url, including the schema-default URL, remains an override.run_pathcompatibility fallback whose schema default is still an unresolved placeholder.The schema inventories, generated registries, smoke-test metadata, and component tests are updated for the typed consumers.
Change Type
How did you test this PR?
red -> green)References
dd_url's schema default makessiteunreachable in ADP. #1965