Skip to content

Commit fa1f541

Browse files
committed
fix(config): migrate endpoints to typed config for provenance awareness (#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
1 parent e03611d commit fa1f541

37 files changed

Lines changed: 2182 additions & 2720 deletions

File tree

.claude/skills/config-system/SKILL.md

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,11 @@ into `SalukiConfiguration`.
5858

5959
In `SalukiConfiguration`, you may use `ConfigValue<T>` when we need to know the difference between a
6060
value that was explicitly set by the user, or where the value is a default. The Agent API provides a
61-
`source` field which we are simplifying into `Provinence`, which is either `Default` or `Explicit`.
61+
`source` field which we are simplifying into `Provenance`, which is either `Default` or `Explicit`.
62+
63+
Some values must be resolved based on their provenance, for example `dd_url` overrides `site` only
64+
if it is explicitly set. This sort of resolution should be done in the config layer with a member
65+
function getter on the relevant struct. The component should store the resolved value.
6266

6367
Paths and type names can move. Notify the user when this skill needs an update.
6468

@@ -239,7 +243,9 @@ the witnessed model.
239243
affected configurations.
240244

241245
A cutover should be behaviorally transparent. If the old behavior conflicts with the source schema
242-
or typed-system invariants, surface the conflict rather than silently choosing one.
246+
or typed-system invariants, surface the conflict rather than silently choosing one. After cutover
247+
artifacts of deserialization logic should not be left behind. For example, no `derive(Deserialize)`
248+
and if possible a held `Option<SomeType>` should collapse to a held `SomeType` if possible.
243249

244250
## Review checklist
245251

Cargo.lock

Lines changed: 0 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

bin/agent-data-plane/src/cli/run.rs

Lines changed: 31 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use std::{
44
time::{Duration, Instant},
55
};
66

7-
use agent_data_plane_config::shared::{Endpoints, MetricsEncoding};
7+
use agent_data_plane_config::{domains::multi_region_failover, shared::SharedConfiguration};
88
use agent_data_plane_config_system::{ConfigurationSystem, LoadedConfiguration};
99
use argh::FromArgs;
1010
use datadog_agent_commons::platform::PlatformSettings;
@@ -394,8 +394,7 @@ async fn create_topology(
394394
let dp = DataPlaneConfiguration::from_configuration(&config);
395395
let mut blueprint = TopologyBlueprint::new("primary", component_registry);
396396
blueprint.with_shutdown_timeout(dp.stop_timeout());
397-
let metrics_encoding = config.shared.metrics_encoding.clone();
398-
let endpoints = config.shared.endpoints.clone();
397+
let shared = config.shared.clone();
399398

400399
let mut control_surfaces = TopologyControlSurfaces::default();
401400

@@ -420,24 +419,12 @@ async fn create_topology(
420419
|| dp.service_checks_pipeline_required()
421420
|| dp.traces_pipeline_required()
422421
{
423-
let dd_forwarder_config = DatadogForwarderConfiguration::from_configuration_with_metrics_routing(
424-
&config_system.raw_map(),
425-
&metrics_encoding,
426-
&endpoints,
427-
)
428-
.error_context("Failed to configure Datadog forwarder.")?;
422+
let dd_forwarder_config = DatadogForwarderConfiguration::from_configuration(&shared, &config_system.raw_map());
429423
blueprint.add_forwarder("dd_out", dd_forwarder_config)?;
430424
}
431425

432426
if dp.metrics_pipeline_required() {
433-
add_baseline_metrics_pipeline_to_blueprint(
434-
&mut blueprint,
435-
config_system,
436-
&metrics_encoding,
437-
&endpoints,
438-
env_provider,
439-
)
440-
.await?;
427+
add_baseline_metrics_pipeline_to_blueprint(&mut blueprint, config_system, &shared, env_provider).await?;
441428
}
442429

443430
if dp.logs_pipeline_required() {
@@ -518,8 +505,8 @@ async fn add_checks_pipeline_to_blueprint(
518505
}
519506

520507
async fn add_baseline_metrics_pipeline_to_blueprint(
521-
blueprint: &mut TopologyBlueprint, config_system: &ConfigurationSystem, metrics: &MetricsEncoding,
522-
endpoints: &Endpoints, env_provider: &ADPEnvironmentProvider,
508+
blueprint: &mut TopologyBlueprint, config_system: &ConfigurationSystem, shared: &SharedConfiguration,
509+
env_provider: &ADPEnvironmentProvider,
523510
) -> Result<(), GenericError> {
524511
// Create the back half of the metrics processing pipeline.
525512
let host_enrichment_config = HostEnrichmentConfiguration::from_environment_provider(env_provider.clone());
@@ -535,12 +522,7 @@ async fn add_baseline_metrics_pipeline_to_blueprint(
535522
}
536523
}
537524

538-
let dd_metrics_config = DatadogMetricsConfiguration::from_configuration_with_metrics_routing(
539-
&config_system.raw_map(),
540-
metrics,
541-
endpoints,
542-
)
543-
.error_context("Failed to configure Datadog Metrics encoder.")?;
525+
let dd_metrics_config = DatadogMetricsConfiguration::from_configuration(shared);
544526

545527
blueprint
546528
// Components.
@@ -549,17 +531,22 @@ async fn add_baseline_metrics_pipeline_to_blueprint(
549531
// Metrics, then forwarding.
550532
.connect_components_in_order(["metrics_enrich", "dd_metrics_encode", "dd_out"])?;
551533

552-
add_mrf_metrics_pipeline_to_blueprint(blueprint, &config_system.raw_map(), metrics, endpoints)?;
553-
add_autoscaling_failover_metrics_pipeline_to_blueprint(blueprint, &config_system.raw_map(), metrics, endpoints)?;
534+
add_mrf_metrics_pipeline_to_blueprint(
535+
blueprint,
536+
&config_system.raw_map(),
537+
shared,
538+
&config.domains.multi_region_failover,
539+
)?;
540+
add_autoscaling_failover_metrics_pipeline_to_blueprint(blueprint, &config_system.raw_map(), shared)?;
554541

555542
Ok(())
556543
}
557544

558545
fn add_mrf_metrics_pipeline_to_blueprint(
559-
blueprint: &mut TopologyBlueprint, config: &GenericConfiguration, metrics: &MetricsEncoding, endpoints: &Endpoints,
546+
blueprint: &mut TopologyBlueprint, config: &GenericConfiguration, shared: &SharedConfiguration,
547+
mrf: &multi_region_failover::Domain,
560548
) -> Result<(), GenericError> {
561-
let mrf_config = MrfConfiguration::from_configuration(config)
562-
.error_context("Failed to configure Multi-Region Failover metrics pipeline.")?;
549+
let mrf_config = MrfConfiguration::from_configuration(mrf);
563550

564551
let Some((mrf_dd_url, mrf_api_key)) = mrf_config.metrics_endpoint_override() else {
565552
if mrf_config.is_enabled() {
@@ -576,20 +563,15 @@ fn add_mrf_metrics_pipeline_to_blueprint(
576563

577564
let mrf_gateway_config = MrfMetricsGatewayConfiguration::new(mrf_config.clone(), config.clone());
578565
let mrf_metrics_config =
579-
DatadogMetricsConfiguration::from_configuration_with_metrics_routing(config, metrics, endpoints)
580-
.error_context("Failed to configure Multi-Region Failover Datadog Metrics encoder.")?
581-
.with_metrics_endpoint_override(mrf_dd_url.clone());
582-
583-
let mrf_forwarder_config =
584-
DatadogForwarderConfiguration::from_configuration_with_metrics_routing(config, metrics, endpoints)
585-
.map(|config| {
586-
config.with_endpoint_override_and_api_key_refresh_config_path(
587-
mrf_dd_url,
588-
mrf_api_key,
589-
"multi_region_failover.api_key",
590-
)
591-
})
592-
.error_context("Failed to configure Multi-Region Failover Datadog forwarder.")?;
566+
DatadogMetricsConfiguration::from_configuration(shared).with_metrics_endpoint_override(mrf_dd_url.clone());
567+
568+
let mrf_forwarder_config = DatadogForwarderConfiguration::for_endpoint_override(
569+
shared,
570+
config,
571+
mrf_dd_url,
572+
mrf_api_key,
573+
"multi_region_failover.api_key",
574+
);
593575

594576
blueprint
595577
.add_transform("mrf_metrics_gateway", mrf_gateway_config)?
@@ -606,7 +588,7 @@ fn add_mrf_metrics_pipeline_to_blueprint(
606588
}
607589

608590
fn add_autoscaling_failover_metrics_pipeline_to_blueprint(
609-
blueprint: &mut TopologyBlueprint, config: &GenericConfiguration, metrics: &MetricsEncoding, endpoints: &Endpoints,
591+
blueprint: &mut TopologyBlueprint, config: &GenericConfiguration, shared: &SharedConfiguration,
610592
) -> Result<(), GenericError> {
611593
let af_config = AutoscalingFailoverConfiguration::from_configuration(config)
612594
.error_context("Failed to configure autoscaling failover metrics pipeline.")?;
@@ -631,14 +613,10 @@ fn add_autoscaling_failover_metrics_pipeline_to_blueprint(
631613
}
632614

633615
let af_gateway_config = AutoscalingFailoverGatewayConfiguration::new(af_config);
634-
let af_metrics_config =
635-
DatadogMetricsConfiguration::from_configuration_with_metrics_routing(config, metrics, endpoints)
636-
.error_context("Failed to configure autoscaling failover metrics encoder.")?
637-
.with_v2_series_only();
638-
let cluster_agent_forwarder_config = ClusterAgentForwarderConfiguration::from_configuration_with_metrics_routing(
639-
config, metrics, endpoints, ca_url, ca_token,
640-
)
641-
.error_context("Failed to configure Cluster Agent forwarder.")?;
616+
let af_metrics_config = DatadogMetricsConfiguration::from_configuration(shared).with_v2_series_only();
617+
let cluster_agent_forwarder_config =
618+
ClusterAgentForwarderConfiguration::from_configuration(shared, config, ca_url, ca_token)
619+
.error_context("Failed to configure Cluster Agent forwarder.")?;
642620

643621
blueprint
644622
.add_transform("af_metrics_gateway", af_gateway_config)?

lib/agent-data-plane-config-system/src/loaded.rs

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use tokio::sync::mpsc;
1818
use crate::env_provider::EnvironmentProvider;
1919
use crate::saluki_env_overlay;
2020
use crate::source::SourceTree;
21-
use crate::system::{translate_strict, ConfigurationSystem, Error};
21+
use crate::system::{translate_strict, validate, ConfigurationSystem, Error};
2222

2323
// The environment-variable prefix ADP reads (`DD_`). Mirrors
2424
// `PlatformSettings::get_env_var_prefix()`; hardcoded so the configuration system need not depend on
@@ -62,6 +62,11 @@ pub struct LoadedConfiguration {
6262
impl LoadedConfiguration {
6363
/// Loads and strictly translates the local file and environment using the requested precedence.
6464
///
65+
/// The snapshot is translated but not validated: it is not yet authoritative. Under the Datadog
66+
/// Agent the stream still has settings to contribute, so a local snapshot that could not run on
67+
/// its own is normal here. Whichever authority [`run`](Self::run) or
68+
/// [`standalone`](Self::standalone) selects applies validation.
69+
///
6570
/// # Errors
6671
///
6772
/// Returns an error if a local source cannot be read, decoded, deserialized, or translated.
@@ -107,12 +112,16 @@ impl LoadedConfiguration {
107112

108113
/// Uses the translated local configuration as the runtime authority.
109114
///
110-
/// No configuration stream or update task is created.
115+
/// No configuration stream or update task is created. Because nothing further will be layered on,
116+
/// the local snapshot is validated here, where [`run`](Self::run) instead validates the merged
117+
/// result of the Agent's initial snapshot.
111118
///
112119
/// # Errors
113120
///
114-
/// Returns an error if the compatibility map cannot be built from the local sources.
121+
/// Returns an error if the compatibility map cannot be built from the local sources, or if the
122+
/// local configuration fails validation.
115123
pub async fn standalone(self) -> Result<ConfigurationSystem, Error> {
124+
validate(&self.local)?;
116125
let compat_map = self.loader.into_generic().await?;
117126
Ok(ConfigurationSystem::standalone(compat_map, self.local))
118127
}
@@ -257,6 +266,26 @@ mod tests {
257266
assert!(matches!(result, Err(Error::Translate { .. })));
258267
}
259268

269+
#[tokio::test]
270+
async fn load_leaves_an_incomplete_local_snapshot_to_the_selected_authority() {
271+
// A local snapshot with no API key is normal: under the Datadog Agent the key arrives over the
272+
// configuration stream, and the CLI subcommands read this snapshot without submitting
273+
// anything. Validation therefore belongs to whichever authority the caller then selects.
274+
let path = std::env::temp_dir().join(format!("adp_no_api_key_{}.yaml", std::process::id()));
275+
std::fs::write(&path, "log_level: warn\n").unwrap();
276+
277+
let loaded = LoadedConfiguration::load(&path, EnvPrecedence::Disabled)
278+
.await
279+
.expect("a local snapshot without an API key loads");
280+
assert_eq!("", loaded.local().shared.endpoints.api_key);
281+
282+
// Standalone mode makes that same snapshot authoritative, so the missing key is fatal there.
283+
let result = loaded.standalone().await;
284+
285+
std::fs::remove_file(&path).ok();
286+
assert!(matches!(result, Err(Error::MissingApiKey)));
287+
}
288+
260289
// `LoadedConfiguration::load` is `async` only for symmetry with the rest of the API; it awaits
261290
// nothing. The environment tests below drive it on a local runtime rather than with
262291
// `#[tokio::test]`, so the blocking environment guard is never held across an await point.

0 commit comments

Comments
 (0)