mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Fix OTEL requirement merging
This commit is contained in:
@@ -2,6 +2,7 @@ use crate::ConfigRequirementsToml;
|
||||
use crate::ManagedHooksRequirementsToml;
|
||||
use crate::RequirementSource;
|
||||
use crate::RequirementsExecPolicyToml;
|
||||
use crate::types::OtelConfigToml;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_absolute_path::AbsolutePathBufGuard;
|
||||
use toml::Value as TomlValue;
|
||||
@@ -80,6 +81,7 @@ impl ComposableRequirementsLayer {
|
||||
domain_fields: DomainMergedRequirementsFields {
|
||||
rules: requirements.rules,
|
||||
hooks: requirements.hooks,
|
||||
otel: requirements.otel,
|
||||
permissions: requirements.permissions,
|
||||
},
|
||||
})
|
||||
@@ -90,6 +92,7 @@ impl ComposableRequirementsLayer {
|
||||
pub(super) struct DomainMergedRequirementsFields {
|
||||
pub(super) rules: Option<RequirementsExecPolicyToml>,
|
||||
pub(super) hooks: Option<ManagedHooksRequirementsToml>,
|
||||
pub(super) otel: Option<OtelConfigToml>,
|
||||
pub(super) permissions: Option<crate::config_requirements::PermissionsRequirementsToml>,
|
||||
}
|
||||
|
||||
@@ -163,6 +166,7 @@ fn toml_value_from_serializable<T: serde::Serialize>(
|
||||
fn strip_special_fields(layer_toml: &mut TomlValue) {
|
||||
remove_top_level_field(layer_toml, "rules");
|
||||
remove_top_level_field(layer_toml, "hooks");
|
||||
remove_top_level_field(layer_toml, "otel");
|
||||
remove_nested_field_and_prune_empty(layer_toml, &["permissions", "filesystem", "deny_read"]);
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
mod hooks;
|
||||
mod layer;
|
||||
mod otel;
|
||||
mod permissions;
|
||||
mod rules;
|
||||
mod stack;
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
use crate::RequirementSource;
|
||||
use crate::Sourced;
|
||||
use crate::types::OtelConfigToml;
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
/// Merges one OTEL requirements layer into an output assembled high-to-low.
|
||||
///
|
||||
/// Exporters and scalar settings are exact leaves, while trace metadata is
|
||||
/// recursively merged by key. Keeping this policy typed prevents exporter
|
||||
/// variants, credentials, and TLS settings from being combined field by field.
|
||||
pub(super) fn merge(
|
||||
output: &mut Option<Sourced<OtelConfigToml>>,
|
||||
incoming: Option<OtelConfigToml>,
|
||||
source: &RequirementSource,
|
||||
) {
|
||||
let Some(incoming) = incoming.filter(|value| !is_empty(value)) else {
|
||||
return;
|
||||
};
|
||||
let Some(output) = output.as_mut() else {
|
||||
*output = Some(Sourced::new(incoming, source.clone()));
|
||||
return;
|
||||
};
|
||||
|
||||
let OtelConfigToml {
|
||||
log_user_prompt,
|
||||
environment,
|
||||
exporter,
|
||||
trace_exporter,
|
||||
metrics_exporter,
|
||||
span_attributes,
|
||||
tracestate,
|
||||
} = incoming;
|
||||
|
||||
macro_rules! fill_exact_leaf {
|
||||
($field:ident) => {
|
||||
if output.value.$field.is_none() {
|
||||
output.value.$field = $field;
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
fill_exact_leaf!(log_user_prompt);
|
||||
fill_exact_leaf!(environment);
|
||||
fill_exact_leaf!(exporter);
|
||||
fill_exact_leaf!(trace_exporter);
|
||||
fill_exact_leaf!(metrics_exporter);
|
||||
merge_map(&mut output.value.span_attributes, span_attributes);
|
||||
merge_nested_map(&mut output.value.tracestate, tracestate);
|
||||
super::stack::merge_output_source(&mut output.source, source);
|
||||
}
|
||||
|
||||
fn merge_map(
|
||||
output: &mut Option<BTreeMap<String, String>>,
|
||||
incoming: Option<BTreeMap<String, String>>,
|
||||
) {
|
||||
let Some(incoming) = incoming else {
|
||||
return;
|
||||
};
|
||||
let output = output.get_or_insert_default();
|
||||
for (key, value) in incoming {
|
||||
output.entry(key).or_insert(value);
|
||||
}
|
||||
}
|
||||
|
||||
fn merge_nested_map(
|
||||
output: &mut Option<BTreeMap<String, BTreeMap<String, String>>>,
|
||||
incoming: Option<BTreeMap<String, BTreeMap<String, String>>>,
|
||||
) {
|
||||
let Some(incoming) = incoming else {
|
||||
return;
|
||||
};
|
||||
let output = output.get_or_insert_default();
|
||||
for (member, fields) in incoming {
|
||||
let output_fields = output.entry(member).or_default();
|
||||
for (key, value) in fields {
|
||||
output_fields.entry(key).or_insert(value);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn is_empty(value: &OtelConfigToml) -> bool {
|
||||
value.log_user_prompt.is_none()
|
||||
&& value.environment.is_none()
|
||||
&& value.exporter.is_none()
|
||||
&& value.trace_exporter.is_none()
|
||||
&& value.metrics_exporter.is_none()
|
||||
&& value.span_attributes.is_none()
|
||||
&& value.tracestate.is_none()
|
||||
}
|
||||
@@ -6,6 +6,7 @@
|
||||
//!
|
||||
//! A few fields carry domain-specific meaning that raw TOML replacement would
|
||||
//! break:
|
||||
//! - `otel` combines recursively merged metadata with atomic exporter leaves.
|
||||
//! - `remote_sandbox_config` is evaluated within each layer before merging.
|
||||
//! - `rules.prefix_rules` append high-priority rules first.
|
||||
//! - `hooks` append high-priority event groups first while failing closed on
|
||||
@@ -132,11 +133,11 @@ impl RequirementsLayerStack {
|
||||
let mut hooks = HookMergeState::new(hook_directory_field);
|
||||
let mut hooks_output = None;
|
||||
let mut deny_read = DenyReadMergeState::default();
|
||||
// Regular TOML fields are folded low-to-high like config. These custom
|
||||
// fields append or union values, so process them high-to-low to keep
|
||||
// priority order visible in the output.
|
||||
// Domain-specific fields are processed high-to-low so exact leaves and
|
||||
// keyed values naturally retain the highest-priority contribution.
|
||||
for layer in layers.iter().rev() {
|
||||
let domain_fields = &layer.domain_fields;
|
||||
super::otel::merge(&mut output.otel, domain_fields.otel.clone(), &layer.source);
|
||||
super::rules::merge(&mut rules, domain_fields.rules.clone(), &layer.source);
|
||||
hooks.merge(
|
||||
&mut hooks_output,
|
||||
@@ -181,7 +182,7 @@ fn populate_merged_regular_fields_with_sources(
|
||||
log_dir,
|
||||
model_catalog_json,
|
||||
check_for_update_on_startup,
|
||||
otel,
|
||||
otel: _,
|
||||
allow_login_shell,
|
||||
shell_environment_policy,
|
||||
feedback,
|
||||
@@ -219,7 +220,6 @@ fn populate_merged_regular_fields_with_sources(
|
||||
check_for_update_on_startup,
|
||||
&["check_for_update_on_startup"]
|
||||
);
|
||||
set_sourced!(otel, &["otel"]);
|
||||
set_sourced!(allow_login_shell, &["allow_login_shell"]);
|
||||
set_sourced!(shell_environment_policy, &["shell_environment_policy"]);
|
||||
set_sourced!(feedback, &["feedback"]);
|
||||
|
||||
@@ -392,6 +392,89 @@ sandbox_private_desktop = true
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn otel_exporters_replace_atomically_across_layers() {
|
||||
let high_source = RequirementSource::EnterpriseManaged {
|
||||
id: "req_high".to_string(),
|
||||
name: "High".to_string(),
|
||||
};
|
||||
let low_source = RequirementSource::EnterpriseManaged {
|
||||
id: "req_low".to_string(),
|
||||
name: "Low".to_string(),
|
||||
};
|
||||
let composed = compose_requirements_for_hostname(
|
||||
vec![
|
||||
RequirementsLayerEntry::from_toml(
|
||||
low_source.clone(),
|
||||
r#"
|
||||
[otel]
|
||||
log_user_prompt = true
|
||||
environment = "low"
|
||||
exporter = { otlp-http = { endpoint = "https://logs.low.example", protocol = "binary", headers = { Authorization = "old-secret" } } }
|
||||
trace_exporter = { otlp-http = { endpoint = "https://traces.low.example", protocol = "json", headers = { Trace = "old-trace" } } }
|
||||
metrics_exporter = { otlp-grpc = { endpoint = "https://metrics.low.example", headers = { Metrics = "old-metrics" } } }
|
||||
|
||||
[otel.span_attributes]
|
||||
region = "us"
|
||||
team = "low"
|
||||
|
||||
[otel.tracestate.vendor]
|
||||
low = "one"
|
||||
shared = "low"
|
||||
"#,
|
||||
),
|
||||
RequirementsLayerEntry::from_toml(
|
||||
high_source.clone(),
|
||||
r#"
|
||||
[otel]
|
||||
environment = "high"
|
||||
exporter = { otlp-grpc = { endpoint = "https://logs.high.example" } }
|
||||
trace_exporter = { otlp-http = { endpoint = "https://traces.high.example", protocol = "binary" } }
|
||||
metrics_exporter = "none"
|
||||
|
||||
[otel.span_attributes]
|
||||
team = "high"
|
||||
|
||||
[otel.tracestate.vendor]
|
||||
high = "two"
|
||||
shared = "high"
|
||||
"#,
|
||||
),
|
||||
],
|
||||
/*hostname*/ None,
|
||||
)
|
||||
.expect("compose requirements")
|
||||
.expect("requirements present");
|
||||
|
||||
assert_eq!(
|
||||
composed.otel,
|
||||
Some(Sourced::new(
|
||||
expected_requirements(
|
||||
r#"
|
||||
[otel]
|
||||
log_user_prompt = true
|
||||
environment = "high"
|
||||
exporter = { otlp-grpc = { endpoint = "https://logs.high.example" } }
|
||||
trace_exporter = { otlp-http = { endpoint = "https://traces.high.example", protocol = "binary" } }
|
||||
metrics_exporter = "none"
|
||||
|
||||
[otel.span_attributes]
|
||||
region = "us"
|
||||
team = "high"
|
||||
|
||||
[otel.tracestate.vendor]
|
||||
high = "two"
|
||||
low = "one"
|
||||
shared = "high"
|
||||
"#,
|
||||
)
|
||||
.otel
|
||||
.expect("expected OTEL requirements"),
|
||||
RequirementSource::composite([high_source, low_source]),
|
||||
))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merged_table_source_is_composite_in_priority_order() {
|
||||
let high_source = RequirementSource::EnterpriseManaged {
|
||||
|
||||
@@ -9456,6 +9456,50 @@ async fn invalid_required_otel_trace_metadata_fails_closed() -> std::io::Result<
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn required_otel_tracestate_survives_invalid_user_merge() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let configured_overflow = "u".repeat(/*n*/ 120);
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
format!(
|
||||
r#"
|
||||
[otel.tracestate.vendor]
|
||||
required = "configured"
|
||||
keep = "kept"
|
||||
zz_overflow = {configured_overflow:?}
|
||||
"#
|
||||
),
|
||||
)?;
|
||||
let required_value = "r".repeat(/*n*/ 120);
|
||||
let requirements = format!("[otel.tracestate.vendor]\nrequired = {required_value:?}\n");
|
||||
|
||||
let config = ConfigBuilder::without_managed_config_for_tests()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(requirements),
|
||||
)
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
config.otel.tracestate,
|
||||
BTreeMap::from([(
|
||||
"vendor".to_string(),
|
||||
BTreeMap::from([
|
||||
("keep".to_string(), "kept".to_string()),
|
||||
("required".to_string(), required_value),
|
||||
]),
|
||||
)])
|
||||
);
|
||||
assert!(config.startup_warnings.iter().any(|warning| {
|
||||
warning.contains("otel.tracestate.vendor.zz_overflow")
|
||||
&& warning.contains("would invalidate tracestate required by")
|
||||
}));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn conflicting_login_method_requirements_fail_closed() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
@@ -1999,16 +1999,24 @@ fn apply_otel_requirement(
|
||||
configured.extend(required);
|
||||
}
|
||||
if let Some(required) = required.tracestate {
|
||||
let configured = configured.tracestate.get_or_insert_default();
|
||||
for (member, required_fields) in required {
|
||||
let configured_fields = configured.entry(member).or_default();
|
||||
conflict |= required_fields.iter().any(|(key, value)| {
|
||||
configured_fields
|
||||
.get(key)
|
||||
.is_some_and(|current| current != value)
|
||||
});
|
||||
configured_fields.extend(required_fields);
|
||||
}
|
||||
let configured_tracestate = configured.tracestate.take().unwrap_or_default();
|
||||
conflict |= required.iter().any(|(member, required_fields)| {
|
||||
configured_tracestate
|
||||
.get(member)
|
||||
.is_some_and(|configured_fields| {
|
||||
required_fields.iter().any(|(key, value)| {
|
||||
configured_fields
|
||||
.get(key)
|
||||
.is_some_and(|current| current != value)
|
||||
})
|
||||
})
|
||||
});
|
||||
configured.tracestate = Some(otel::merge_required_tracestate(
|
||||
configured_tracestate,
|
||||
required,
|
||||
&source,
|
||||
startup_warnings,
|
||||
));
|
||||
}
|
||||
|
||||
if conflict {
|
||||
|
||||
@@ -36,6 +36,65 @@ pub(crate) fn resolve_config(
|
||||
}
|
||||
}
|
||||
|
||||
/// Merges user-configured tracestate into a validated required base.
|
||||
///
|
||||
/// Required member fields always win. Other configured fields are admitted one
|
||||
/// at a time only when the complete candidate remains W3C-valid; rejected
|
||||
/// fields produce startup warnings rather than erasing required metadata.
|
||||
pub(crate) fn merge_required_tracestate(
|
||||
configured: BTreeMap<String, BTreeMap<String, String>>,
|
||||
required: BTreeMap<String, BTreeMap<String, String>>,
|
||||
source: &impl Display,
|
||||
startup_warnings: &mut Vec<String>,
|
||||
) -> BTreeMap<String, BTreeMap<String, String>> {
|
||||
let mut effective = required.clone();
|
||||
for (member_key, configured_fields) in configured {
|
||||
for (field_key, value) in configured_fields {
|
||||
if required
|
||||
.get(&member_key)
|
||||
.is_some_and(|required_fields| required_fields.contains_key(&field_key))
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
let mut candidate = effective.clone();
|
||||
candidate
|
||||
.entry(member_key.clone())
|
||||
.or_default()
|
||||
.insert(field_key.clone(), value);
|
||||
retain_valid_configured_tracestate(
|
||||
candidate,
|
||||
&format!("otel.tracestate.{member_key}.{field_key}"),
|
||||
source,
|
||||
startup_warnings,
|
||||
&mut effective,
|
||||
);
|
||||
}
|
||||
}
|
||||
effective
|
||||
}
|
||||
|
||||
fn retain_valid_configured_tracestate(
|
||||
candidate: BTreeMap<String, BTreeMap<String, String>>,
|
||||
config_key: &str,
|
||||
source: &impl Display,
|
||||
startup_warnings: &mut Vec<String>,
|
||||
effective: &mut BTreeMap<String, BTreeMap<String, String>>,
|
||||
) {
|
||||
match codex_otel::validate_tracestate_entries(&candidate) {
|
||||
Ok(()) => {
|
||||
*effective = candidate;
|
||||
}
|
||||
Err(err) => {
|
||||
let message = format!(
|
||||
"Ignoring configured `{config_key}` because it would invalidate tracestate required by {source}: {err}"
|
||||
);
|
||||
tracing::warn!("{message}");
|
||||
startup_warnings.push(message);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn resolve_span_attributes(
|
||||
span_attributes: Option<BTreeMap<String, String>>,
|
||||
startup_warnings: &mut Vec<String>,
|
||||
|
||||
Reference in New Issue
Block a user