Streamify log expansion (for discussion, not review) - #6266
Conversation
| let log_stream: Result<Box<dyn Iterator<Item = OurLog>>> = match integration { | ||
| LogsIntegration::Nel => nel::expand2(&payload, headers), | ||
| LogsIntegration::OtelV1 { format } => otel::expand2(format, &payload), |
There was a problem hiding this comment.
OTel log expansion panics on malformed input instead of returning an error
otel::expand2 unwraps the parsing result of attacker-controlled payload bytes, causing a panic on invalid input instead of returning a rejection error.
Evidence
- The hunk adds
otel::expand2(format, &payload)atmod.rs:32, feeding attacker-controlled payload bytes into the new function. otel::expand2inrelay-server/src/processing/logs/integrations/otel.rs:15callsparse_logs_data(format, payload).unwrap().parse_logs_datacorrectly returnsResult<LogsData, Error>, propagating JSON and protobuf parse failures.- The
.unwrap()converts every parse failure into an uncatchable panic, crashing the worker thread instead of returning aDiscardReasonrejection outcome to the caller. - Sibling
expand2functions innel.rsandvercel.rsproperly propagate errors via?andmap_err, confirming the omission is unintended.
Also found at 2 additional locations
relay-server/src/processing/logs/integrations/otel.rs:13relay-server/src/processing/logs/process.rs:50-51
Identified by Warden · wrdn-dos-review · RMP-KHP
| filter::feature_flag(ctx).reject(&logs)?; | ||
|
|
||
| let mut logs = process::expand(logs)?; | ||
| let mut logs = process::expand(logs, ctx.config.max_expanded_log_count())?; |
There was a problem hiding this comment.
max_expanded_log_count cap is post-materialization for integrations and skipped for containers
The max_expanded_log_count limit passed to process::expand is not enforced before integration payloads are fully parsed into memory, and the container expansion path ignores it entirely.
Evidence
mod.rs:161passesctx.config.max_expanded_log_count()intoprocess::expand.- In
process.rs, theLogItems::Containerbranch callsexpand_log_container(&item, trust)without the cap, so all container logs are parsed into memory and returned. - In
process.rs, theLogItems::Integrationbranch passes the cap tointegrations::expand(), which applies.take(max_expanded_log_count)atintegrations/mod.rs:68. nel::expand2eagerly parses the full payload withserde_json::from_slice::<Vec<_>>(payload)before returning an iterator.vercel::expand2(JSON path) eagerly parses withserde_json::from_slice::<Vec<VercelLog>>(payload)before returning an iterator.otel::expand2eagerly parses the full payload viaparse_logs_data(serde_json::from_sliceorLogsData::decode) before returning an iterator.- Because the cap via
.take()operates on an iterator over already-materialized data, the memory and parsing cost for the full integration payload is paid before the limit takes effect.
Identified by Warden · wrdn-dos-review · HQH-YVE
| /// The maximum number of logs that can result from a log expansion. | ||
| pub max_expanded_log_count: usize, |
There was a problem hiding this comment.
I wonder if we can derive this from the total size, so we don't have to derive a count when we already have a size.
| LogItems::Container(item) => expand_log_container(&item, trust)?, | ||
| LogItems::Integration(item) => { |
There was a problem hiding this comment.
This could also be streamed, then the question arises if the stream shouldn't be consumed here?
| Ok(Settings::default()) | ||
| Ok(Box::new(logs.resource_logs.into_iter().flat_map( | ||
| |resource_logs| { | ||
| let resource = std::cell::RefCell::new(resource_logs.resource); |
There was a problem hiding this comment.
I don't think this RefCell is necessary, there shouldn't be a problem with passing in a & to the nested closures.
This entire stream may be easier expressed if you build up a stream of (resource, scope, log) items via something like .flat_map(repeat(resource).zip(scopes)) then convert them.
There was a problem hiding this comment.
It's because of all the nesting--that resource_logs from the very-first flat_map is only alive long enough to return the inner iterator, and so it really does need to be owned by the inner closures, but the only way to do that (and avoid the cloning) is to stick it in a ref-cell
No description provided.