From 242d3df93e9af3810618d0bdd67f8abd73149e75 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 27 Apr 2022 11:56:44 +0200 Subject: [PATCH 1/6] ref(envelope): Add an item type for unknown items --- relay-server/src/actors/envelopes.rs | 5 +- relay-server/src/envelope.rs | 89 ++++++++++++++++++++++++--- relay-server/src/utils/rate_limits.rs | 1 + relay-server/src/utils/sizes.rs | 1 + 4 files changed, 85 insertions(+), 11 deletions(-) diff --git a/relay-server/src/actors/envelopes.rs b/relay-server/src/actors/envelopes.rs index 846b6b7fce..2e95e2708d 100644 --- a/relay-server/src/actors/envelopes.rs +++ b/relay-server/src/actors/envelopes.rs @@ -1275,13 +1275,16 @@ impl EnvelopeProcessor { ItemType::Attachment => false, ItemType::UserReport => false, - // aggregate data is never considered as part of deduplication + // Aggregate data is never considered as part of deduplication ItemType::Session => false, ItemType::Sessions => false, ItemType::Metrics => false, ItemType::MetricBuckets => false, ItemType::ClientReport => false, ItemType::Profile => false, + + // Without knowing more, `Unknown` items are allowed to be repeated + ItemType::Unknown(_) => false, } } diff --git a/relay-server/src/envelope.rs b/relay-server/src/envelope.rs index c79c3d0b00..3d44f0b109 100644 --- a/relay-server/src/envelope.rs +++ b/relay-server/src/envelope.rs @@ -73,8 +73,7 @@ pub enum EnvelopeError { } /// The type of an envelope item. -#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize, Serialize)] -#[serde(rename_all = "snake_case")] +#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] pub enum ItemType { /// Event payload encoded in JSON. Event, @@ -104,6 +103,13 @@ pub enum ItemType { ClientReport, /// Profile event payload encoded in JSON Profile, + /// A new item type that is yet unknown by this version of Relay. + /// + /// By default, items of this type are forwarded without modification. Processing Relays and + /// Relays explicitly configured to do so will instead drop those items. This allows + /// forward-compatibility with new item types where we expect outdated Relays. + Unknown(String), + // Keep `Unknown` last in the list. Add new items above `Unknown`. } impl ItemType { @@ -124,22 +130,49 @@ impl fmt::Display for ItemType { match *self { Self::Event => write!(f, "event"), Self::Transaction => write!(f, "transaction"), - Self::Security => write!(f, "security report"), + Self::Security => write!(f, "security"), Self::Attachment => write!(f, "attachment"), - Self::FormData => write!(f, "form data"), - Self::RawSecurity => write!(f, "raw security report"), - Self::UnrealReport => write!(f, "unreal report"), - Self::UserReport => write!(f, "user feedback"), + Self::FormData => write!(f, "form_data"), + Self::RawSecurity => write!(f, "raw_security"), + Self::UnrealReport => write!(f, "unreal_report"), + Self::UserReport => write!(f, "user_report"), Self::Session => write!(f, "session"), - Self::Sessions => write!(f, "aggregated sessions"), + Self::Sessions => write!(f, "sessions"), Self::Metrics => write!(f, "metrics"), - Self::MetricBuckets => write!(f, "metric buckets"), - Self::ClientReport => write!(f, "client report"), + Self::MetricBuckets => write!(f, "metric_buckets"), + Self::ClientReport => write!(f, "client_report"), Self::Profile => write!(f, "profile"), + Self::Unknown(s) => s.fmt(f), } } } +impl std::str::FromStr for ItemType { + type Err = std::convert::Infallible; + + fn from_str(s: &str) -> Result { + Ok(match s { + "event" => Self::Event, + "transaction" => Self::Transaction, + "security" => Self::Security, + "attachment" => Self::Attachment, + "form_data" => Self::FormData, + "raw_security" => Self::RawSecurity, + "unreal_report" => Self::UnrealReport, + "user_report" => Self::UserReport, + "session" => Self::Session, + "sessions" => Self::Sessions, + "metrics" => Self::Metrics, + "metric_buckets" => Self::MetricBuckets, + "client_report" => Self::ClientReport, + "profile" => Self::Profile, + other => Self::Unknown(other.to_owned()), + }) + } +} + +relay_common::impl_str_serde!(ItemType, "an envelope item type (see sentry develop docs)"); + /// Payload content types. /// /// This is an optimized enum intended to reduce allocations for common content types. @@ -581,6 +614,10 @@ impl Item { | ItemType::MetricBuckets | ItemType::ClientReport | ItemType::Profile => false, + + // The unknown item type can observe any behavior, most likely there are going to be no + // item types added that create events. + ItemType::Unknown(_) => false, } } @@ -603,6 +640,20 @@ impl Item { ItemType::MetricBuckets => false, ItemType::ClientReport => false, ItemType::Profile => false, + + // Since this Relay cannot interpret the semantics of this item, it does not know + // whether it requires an event or not. Depending on the strategy, this can cause two + // wrong actions here: + // 1. return false, but the item requires an event. It is split off by Relay and + // handled separately. If the event is rate limited or filtered, the item still gets + // ingested and needs to be pruned at a later point in the pipeline. This also + // happens with standalone attachments. + // 2. return true, but the item does not require an event. It is kept in the same + // envelope and dropped along with the event, even though it should be ingested. + // Realistically, most new item types should be ingested largely independent of events, + // and the ingest pipeline needs to assume split submission from clients. This makes + // returning `false` the safer option. + ItemType::Unknown(_) => false, } } } @@ -1286,6 +1337,24 @@ mod tests { assert_eq!(items[1].filename(), Some("application.log")); } + #[test] + fn test_deserialize_envelope_unknown_item() { + // With terminating newline after item payload + let bytes = Bytes::from( + "\ + {\"event_id\":\"9ec79c33ec9942ab8353589fcb2e04dc\",\"dsn\":\"https://e12d836b15bb49d7bbf99e64295d995b:@sentry.io/42\"}\n\ + {\"type\":\"invalid_unknown\"}\n\ + helloworld\n\ + ", + ); + + let envelope = Envelope::parse_bytes(bytes).unwrap(); + assert_eq!(envelope.len(), 1); + + let items: Vec<_> = envelope.items().collect(); + assert_eq!(items[0].len(), 10); + } + #[test] fn test_parse_request_envelope() { let bytes = Bytes::from("{\"event_id\":\"9ec79c33ec9942ab8353589fcb2e04dc\"}"); diff --git a/relay-server/src/utils/rate_limits.rs b/relay-server/src/utils/rate_limits.rs index cf09bcb64c..11c72180b9 100644 --- a/relay-server/src/utils/rate_limits.rs +++ b/relay-server/src/utils/rate_limits.rs @@ -105,6 +105,7 @@ fn infer_event_category(item: &Item) -> Option { ItemType::UserReport => None, ItemType::Profile => None, ItemType::ClientReport => None, + ItemType::Unknown(_) => None, } } diff --git a/relay-server/src/utils/sizes.rs b/relay-server/src/utils/sizes.rs index 8a443fcb0b..a6e3c8703e 100644 --- a/relay-server/src/utils/sizes.rs +++ b/relay-server/src/utils/sizes.rs @@ -45,6 +45,7 @@ pub fn check_envelope_size_limits(config: &Config, envelope: &Envelope) -> bool return false; } } + ItemType::Unknown(_) => (), } } From 47ca2064cc6353a0a8985dfdeb6768318c4455fd Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 27 Apr 2022 11:57:26 +0200 Subject: [PATCH 2/6] ref: Update code due to dropped Copy implementation --- relay-server/src/actors/envelopes.rs | 28 +++++++++++----------- relay-server/src/actors/store.rs | 4 ++-- relay-server/src/endpoints/common.rs | 4 ++-- relay-server/src/envelope.rs | 26 ++++++++++---------- relay-server/src/utils/dynamic_sampling.rs | 6 ++--- relay-server/src/utils/multipart.rs | 2 +- relay-server/src/utils/rate_limits.rs | 6 ++--- relay-server/src/utils/unreal.rs | 2 +- 8 files changed, 39 insertions(+), 39 deletions(-) diff --git a/relay-server/src/actors/envelopes.rs b/relay-server/src/actors/envelopes.rs index 2e95e2708d..25069d3c84 100644 --- a/relay-server/src/actors/envelopes.rs +++ b/relay-server/src/actors/envelopes.rs @@ -793,7 +793,7 @@ impl EnvelopeProcessor { /// is written back into the item. fn process_user_reports(&self, state: &mut ProcessEnvelopeState) { state.envelope.retain_items(|item| { - if item.ty() != ItemType::UserReport { + if item.ty() != &ItemType::UserReport { return true; }; @@ -831,7 +831,7 @@ impl EnvelopeProcessor { if self.config.processing_enabled() { state .envelope - .retain_items(|item| item.ty() != ItemType::ClientReport); + .retain_items(|item| item.ty() != &ItemType::ClientReport); } return; } @@ -846,7 +846,7 @@ impl EnvelopeProcessor { // we're going through all client reports but we're effectively just merging // them into the first one. state.envelope.retain_items(|item| { - if item.ty() != ItemType::ClientReport { + if item.ty() != &ItemType::ClientReport { return true; }; match ClientReport::parse(&item.payload()) { @@ -1038,7 +1038,7 @@ impl EnvelopeProcessor { fn expand_unreal(&self, state: &mut ProcessEnvelopeState) -> Result<(), ProcessingError> { let envelope = &mut state.envelope; - if let Some(item) = envelope.take_item_by(|item| item.ty() == ItemType::UnrealReport) { + if let Some(item) = envelope.take_item_by(|item| item.ty() == &ItemType::UnrealReport) { utils::expand_unreal_envelope(item, envelope, &self.config)?; } @@ -1302,11 +1302,11 @@ impl EnvelopeProcessor { // Remove all items first, and then process them. After this function returns, only // attachments can remain in the envelope. The event will be added again at the end of // `process_event`. - let event_item = envelope.take_item_by(|item| item.ty() == ItemType::Event); - let transaction_item = envelope.take_item_by(|item| item.ty() == ItemType::Transaction); - let security_item = envelope.take_item_by(|item| item.ty() == ItemType::Security); - let raw_security_item = envelope.take_item_by(|item| item.ty() == ItemType::RawSecurity); - let form_item = envelope.take_item_by(|item| item.ty() == ItemType::FormData); + let event_item = envelope.take_item_by(|item| item.ty() == &ItemType::Event); + let transaction_item = envelope.take_item_by(|item| item.ty() == &ItemType::Transaction); + let security_item = envelope.take_item_by(|item| item.ty() == &ItemType::Security); + let raw_security_item = envelope.take_item_by(|item| item.ty() == &ItemType::RawSecurity); + let form_item = envelope.take_item_by(|item| item.ty() == &ItemType::FormData); let attachment_item = envelope .take_item_by(|item| item.attachment_type() == Some(AttachmentType::EventPayload)); let breadcrumbs1 = envelope @@ -1316,7 +1316,7 @@ impl EnvelopeProcessor { // Event items can never occur twice in an envelope. if let Some(duplicate) = envelope.get_item_by(|item| self.is_duplicate(item)) { - return Err(ProcessingError::DuplicateItem(duplicate.ty())); + return Err(ProcessingError::DuplicateItem(duplicate.ty().clone())); } let (event, event_len) = if let Some(mut item) = event_item.or(security_item) { @@ -2010,7 +2010,7 @@ impl Handler for EnvelopeProcessor { for item in items { let payload = item.payload(); - if item.ty() == ItemType::Metrics { + if item.ty() == &ItemType::Metrics { let mut timestamp = item.timestamp().unwrap_or(received_timestamp); clock_drift_processor.process_timestamp(&mut timestamp); @@ -2027,7 +2027,7 @@ impl Handler for EnvelopeProcessor { relay_log::trace!("inserting metrics into project cache"); project_cache.do_send(InsertMetrics::new(public_key, metrics)); } - } else if item.ty() == ItemType::MetricBuckets { + } else if item.ty() == &ItemType::MetricBuckets { match Bucket::parse_all(&payload) { Ok(mut buckets) => { for bucket in &mut buckets { @@ -3040,7 +3040,7 @@ mod tests { let new_envelope = envelope_response.envelope.unwrap(); assert_eq!(new_envelope.len(), 1); - assert_eq!(new_envelope.items().next().unwrap().ty(), ItemType::Event); + assert_eq!(new_envelope.items().next().unwrap().ty(), &ItemType::Event); } #[test] @@ -3153,7 +3153,7 @@ mod tests { let envelope = envelope_response.envelope.unwrap(); let item = envelope.items().next().unwrap(); - assert_eq!(item.ty(), ItemType::ClientReport); + assert_eq!(item.ty(), &ItemType::ClientReport); } #[test] diff --git a/relay-server/src/actors/store.rs b/relay-server/src/actors/store.rs index ba08289369..daa52ceaa5 100644 --- a/relay-server/src/actors/store.rs +++ b/relay-server/src/actors/store.rs @@ -687,7 +687,7 @@ impl Message for StoreEnvelope { /// /// Slow items must be routed to the `Attachments` topic. fn is_slow_item(item: &Item) -> bool { - item.ty() == ItemType::Attachment || item.ty() == ItemType::UserReport + item.ty() == &ItemType::Attachment || item.ty() == &ItemType::UserReport } impl Handler for StoreForwarder { @@ -712,7 +712,7 @@ impl Handler for StoreForwarder { let topic = if envelope.get_item_by(is_slow_item).is_some() { KafkaTopic::Attachments - } else if event_item.map(|x| x.ty()) == Some(ItemType::Transaction) { + } else if event_item.map(|x| x.ty()) == Some(&ItemType::Transaction) { KafkaTopic::Transactions } else { KafkaTopic::Events diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index 8da869f0d5..77037a5265 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -247,7 +247,7 @@ pub fn event_id_from_formdata(data: &[u8]) -> Result, BadStoreRe /// Extracting the event id from chunked formdata fields on the Minidump endpoint (`sentry__1`, /// `sentry__2`, ...) is not supported. In this case, `None` is returned. pub fn event_id_from_items(items: &Items) -> Result, BadStoreRequest> { - if let Some(item) = items.iter().find(|item| item.ty() == ItemType::Event) { + if let Some(item) = items.iter().find(|item| item.ty() == &ItemType::Event) { if let Some(event_id) = event_id_from_json(&item.payload())? { return Ok(Some(event_id)); } @@ -262,7 +262,7 @@ pub fn event_id_from_items(items: &Items) -> Result, BadStoreReq } } - if let Some(item) = items.iter().find(|item| item.ty() == ItemType::FormData) { + if let Some(item) = items.iter().find(|item| item.ty() == &ItemType::FormData) { // Swallow all other errors here since it is quite common to receive invalid secondary // payloads. `EnvelopeProcessor` also retains events in such cases. if let Ok(Some(event_id)) = event_id_from_formdata(&item.payload()) { diff --git a/relay-server/src/envelope.rs b/relay-server/src/envelope.rs index 3d44f0b109..546f6f1d16 100644 --- a/relay-server/src/envelope.rs +++ b/relay-server/src/envelope.rs @@ -127,7 +127,7 @@ impl ItemType { impl fmt::Display for ItemType { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match *self { + match self { Self::Event => write!(f, "event"), Self::Transaction => write!(f, "transaction"), Self::Security => write!(f, "security"), @@ -454,8 +454,8 @@ impl Item { } /// Returns the `ItemType` of this item. - pub fn ty(&self) -> ItemType { - self.headers.ty + pub fn ty(&self) -> &ItemType { + &self.headers.ty } /// Returns the length of this item's payload. @@ -1140,7 +1140,7 @@ mod tests { let items: Vec<_> = envelope.items().collect(); assert_eq!(items.len(), 1); - assert_eq!(items[0].ty(), ItemType::Attachment); + assert_eq!(items[0].ty(), &ItemType::Attachment); } #[test] @@ -1157,13 +1157,13 @@ mod tests { envelope.add_item(item2); let taken = envelope - .take_item_by(|item| item.ty() == ItemType::Attachment) + .take_item_by(|item| item.ty() == &ItemType::Attachment) .expect("should return some item"); assert_eq!(taken.filename(), Some("item1")); assert!(envelope - .take_item_by(|item| item.ty() == ItemType::Event) + .take_item_by(|item| item.ty() == &ItemType::Event) .is_none()); } @@ -1319,7 +1319,7 @@ mod tests { assert_eq!(envelope.len(), 2); let items: Vec<_> = envelope.items().collect(); - assert_eq!(items[0].ty(), ItemType::Attachment); + assert_eq!(items[0].ty(), &ItemType::Attachment); assert_eq!(items[0].len(), 10); assert_eq!( items[0].payload(), @@ -1327,7 +1327,7 @@ mod tests { ); assert_eq!(items[0].content_type(), Some(&ContentType::Text)); - assert_eq!(items[1].ty(), ItemType::Event); + assert_eq!(items[1].ty(), &ItemType::Event); assert_eq!(items[1].len(), 41); assert_eq!( items[1].payload(), @@ -1492,7 +1492,7 @@ mod tests { envelope.add_item(Item::new(ItemType::Attachment)); // Does not split when no item matches. - let split_opt = envelope.split_by(|item| item.ty() == ItemType::Session); + let split_opt = envelope.split_by(|item| item.ty() == &ItemType::Session); assert!(split_opt.is_none()); } @@ -1503,7 +1503,7 @@ mod tests { envelope.add_item(Item::new(ItemType::Session)); // Does not split when all items match. - let split_opt = envelope.split_by(|item| item.ty() == ItemType::Session); + let split_opt = envelope.split_by(|item| item.ty() == &ItemType::Session); assert!(split_opt.is_none()); } @@ -1513,7 +1513,7 @@ mod tests { envelope.add_item(Item::new(ItemType::Session)); envelope.add_item(Item::new(ItemType::Attachment)); - let split_opt = envelope.split_by(|item| item.ty() == ItemType::Session); + let split_opt = envelope.split_by(|item| item.ty() == &ItemType::Session); let split_envelope = split_opt.expect("split_by returns an Envelope"); assert_eq!(split_envelope.len(), 1); @@ -1521,11 +1521,11 @@ mod tests { // Matching items have moved into the split envelope. for item in split_envelope.items() { - assert_eq!(item.ty(), ItemType::Session); + assert_eq!(item.ty(), &ItemType::Session); } for item in envelope.items() { - assert_eq!(item.ty(), ItemType::Attachment); + assert_eq!(item.ty(), &ItemType::Attachment); } } } diff --git a/relay-server/src/utils/dynamic_sampling.rs b/relay-server/src/utils/dynamic_sampling.rs index 9a389c6246..92883a315c 100644 --- a/relay-server/src/utils/dynamic_sampling.rs +++ b/relay-server/src/utils/dynamic_sampling.rs @@ -80,7 +80,7 @@ fn sample_transaction_internal( } let trace_context = envelope.trace_context(); - let transaction_item = envelope.get_item_by(|item| item.ty() == ItemType::Transaction); + let transaction_item = envelope.get_item_by(|item| item.ty() == &ItemType::Transaction); let trace_context = match (trace_context, transaction_item) { // we don't have what we need, can't sample the transactions in this envelope @@ -93,7 +93,7 @@ fn sample_transaction_internal( if let SamplingResult::Drop(rule_id) = trace_context.should_keep(client_ip, sampling_config) { // remove transaction and dependent items if envelope - .take_item_by(|item| item.ty() == ItemType::Transaction) + .take_item_by(|item| item.ty() == &ItemType::Transaction) .is_some() { // we have removed the transaction from the envelope @@ -134,7 +134,7 @@ pub fn sample_trace( Some(project) => project, }; let trace_context = envelope.trace_context(); - let transaction_item = envelope.get_item_by(|item| item.ty() == ItemType::Transaction); + let transaction_item = envelope.get_item_by(|item| item.ty() == &ItemType::Transaction); // if there is no trace context or there are no transactions to sample return here if trace_context.is_none() || transaction_item.is_none() { diff --git a/relay-server/src/utils/multipart.rs b/relay-server/src/utils/multipart.rs index 38fc8ccb0e..e2d9f70d33 100644 --- a/relay-server/src/utils/multipart.rs +++ b/relay-server/src/utils/multipart.rs @@ -363,7 +363,7 @@ mod tests { writer.append("blub", "blub"); let item = writer.into_item(); - assert_eq!(item.ty(), ItemType::FormData); + assert_eq!(item.ty(), &ItemType::FormData); let payload = item.payload(); let iter = FormDataIter::new(&payload); diff --git a/relay-server/src/utils/rate_limits.rs b/relay-server/src/utils/rate_limits.rs index 11c72180b9..ccad7fe483 100644 --- a/relay-server/src/utils/rate_limits.rs +++ b/relay-server/src/utils/rate_limits.rs @@ -142,7 +142,7 @@ impl EnvelopeSummary { for item in envelope.items() { if item.creates_event() { summary.infer_category(item); - } else if item.ty() == ItemType::Attachment { + } else if item.ty() == &ItemType::Attachment { // Plain attachments do not create events. summary.has_plain_attachments = true; } @@ -412,7 +412,7 @@ where } // Remove attachments, except those required for processing - if enforcement.attachments.is_active() && item.ty() == ItemType::Attachment { + if enforcement.attachments.is_active() && item.ty() == &ItemType::Attachment { if item.creates_event() { item.set_rate_limited(true); return true; @@ -422,7 +422,7 @@ where } // Remove sessions independently of events - if enforcement.sessions.is_active() && item.ty() == ItemType::Session { + if enforcement.sessions.is_active() && item.ty() == &ItemType::Session { return false; } diff --git a/relay-server/src/utils/unreal.rs b/relay-server/src/utils/unreal.rs index 80d9ad1e81..2412d8a876 100644 --- a/relay-server/src/utils/unreal.rs +++ b/relay-server/src/utils/unreal.rs @@ -55,7 +55,7 @@ pub fn expand_unreal_envelope( let crash = Unreal4Crash::parse_with_limit(&payload, config.max_envelope_size())?; let mut has_event = envelope - .get_item_by(|item| item.ty() == ItemType::Event) + .get_item_by(|item| item.ty() == &ItemType::Event) .is_some(); for file in crash.files() { From 221a45ff5227dc8d51f1e2d8a8d8597e8d668491 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 27 Apr 2022 15:06:07 +0200 Subject: [PATCH 3/6] ref(server): Drop unknown items based on config --- relay-config/src/config.rs | 15 +++++++++++++++ relay-server/src/endpoints/common.rs | 13 ++++++++++++- relay-server/src/utils/sizes.rs | 16 ++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/relay-config/src/config.rs b/relay-config/src/config.rs index 15e6e488de..ebbf991299 100644 --- a/relay-config/src/config.rs +++ b/relay-config/src/config.rs @@ -576,6 +576,13 @@ impl Default for Limits { } } +/// Controls traffic steering. +#[derive(Debug, Default, Deserialize, Serialize)] +#[serde(default)] +pub struct Routing { + forward_unknown_items: Option, +} + /// Http content encoding for both incoming and outgoing web requests. #[derive(Clone, Copy, Debug, Deserialize, Serialize)] #[serde(rename_all = "lowercase")] @@ -1224,6 +1231,8 @@ struct ConfigValues { #[serde(default)] logging: relay_log::LogConfig, #[serde(default)] + routing: Routing, + #[serde(default)] metrics: Metrics, #[serde(default)] sentry: relay_log::SentryConfig, @@ -1940,6 +1949,12 @@ impl Config { pub fn static_relays(&self) -> &HashMap { &self.values.auth.static_relays } + + /// Returns `true` if unknown items should be forwarded (default). + pub fn forward_unknown_items(&self) -> bool { + let forward = self.values.routing.forward_unknown_items; + forward.unwrap_or_else(|| !self.processing_enabled()) + } } impl Default for Config { diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index 77037a5265..4e471b89ef 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -346,8 +346,14 @@ where let future = extract_envelope(&request, meta) .into_future() - .and_then(clone!(envelope_context, |envelope| { + .and_then(clone!(config, envelope_context, |mut envelope| { envelope_context.borrow_mut().update(&envelope); + + // If configured, remove unknown items at the very beginning. If the envelope is + // empty, we fail the request with a special control flow error to skip checks and + // queueing, that still results in a `200 OK` response. + utils::remove_unknown_items(&config, &mut envelope); + if envelope.is_empty() { // envelope is empty, cannot send outcomes Err(BadStoreRequest::EmptyEnvelope) @@ -429,6 +435,11 @@ where return Ok(create_response(event_id)); } + // This is a control-flow error without a bad status code. + if matches!(error, BadStoreRequest::EmptyEnvelope) { + return Ok(create_response(event_id)); + } + let response = error.error_response(); if response.status().is_server_error() { relay_log::error!("error handling request: {}", LogError(&error)); diff --git a/relay-server/src/utils/sizes.rs b/relay-server/src/utils/sizes.rs index a6e3c8703e..34a40c3a02 100644 --- a/relay-server/src/utils/sizes.rs +++ b/relay-server/src/utils/sizes.rs @@ -54,3 +54,19 @@ pub fn check_envelope_size_limits(config: &Config, envelope: &Envelope) -> bool && session_count <= config.max_session_count() && client_reports_size <= config.max_client_reports_size() } + +/// Checks for valid envelope items. +/// +/// If Relay is configured to drop unknown items, this function removes them from the Envelope. All +/// known items will be retained. +pub fn remove_unknown_items(config: &Config, envelope: &mut Envelope) { + if !config.forward_unknown_items() { + envelope.retain_items(|item| match item.ty() { + ItemType::Unknown(ty) => { + relay_log::debug!("dropping unknown item of type '{}'", ty); + false + } + _ => true, + }); + } +} From e8b571064e0adc678cff0e32557bf426bf07d61f Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 27 Apr 2022 15:16:17 +0200 Subject: [PATCH 4/6] meta: Changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d07d8e71c9..86820353fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ - Add platform, op, http.method and status tag to all extracted transaction metrics. ([#1227](https://github.com/getsentry/relay/pull/1227)) - Add units in built-in measurements. ([#1229](https://github.com/getsentry/relay/pull/1229)) +**Bug Fixes**: +- Accept and forward unknown Envelope items. In processing mode, drop items individually rather than rejecting the entire request. This allows SDKs to send new data in combined Envelopes in the future. ([#1246](https://github.com/getsentry/relay/pull/1246)) + **Internal**: * Add sampling + tagging by event platform and transaction op. Some (unused) tagging rules from 22.4.0 have been renamed. ([#1231](https://github.com/getsentry/relay/pull/1231)) From 19817fee548aa3a36b96ffe7bfddaa2c540ce97f Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 27 Apr 2022 18:31:37 +0200 Subject: [PATCH 5/6] ref: Rename the config option and update integration tests --- relay-config/src/config.rs | 17 +++++++++--- relay-server/src/utils/sizes.rs | 2 +- tests/integration/test_envelope.py | 44 ++++++++++++++++++++++++++---- 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/relay-config/src/config.rs b/relay-config/src/config.rs index ebbf991299..9723a53529 100644 --- a/relay-config/src/config.rs +++ b/relay-config/src/config.rs @@ -580,7 +580,16 @@ impl Default for Limits { #[derive(Debug, Default, Deserialize, Serialize)] #[serde(default)] pub struct Routing { - forward_unknown_items: Option, + /// Accept and forward unknown Envelope items to the upstream. + /// + /// Forwarding unknown items should be enabled in most cases to allow proxying traffic for newer + /// SDK versions. The upstream in Sentry makes the final decision on which items are valid. If + /// this is disabled, just the unknown items are removed from Envelopes, and the rest is + /// processed as usual. + /// + /// Defaults to `true` for all Relay modes other than processing mode. In processing mode, this + /// is disabled by default since the item cannot be handled. + unknown_items: Option, } /// Http content encoding for both incoming and outgoing web requests. @@ -1950,9 +1959,9 @@ impl Config { &self.values.auth.static_relays } - /// Returns `true` if unknown items should be forwarded (default). - pub fn forward_unknown_items(&self) -> bool { - let forward = self.values.routing.forward_unknown_items; + /// Returns `true` if unknown items should be accepted and forwarded. + pub fn accept_unknown_items(&self) -> bool { + let forward = self.values.routing.unknown_items; forward.unwrap_or_else(|| !self.processing_enabled()) } } diff --git a/relay-server/src/utils/sizes.rs b/relay-server/src/utils/sizes.rs index 34a40c3a02..a8acf4d471 100644 --- a/relay-server/src/utils/sizes.rs +++ b/relay-server/src/utils/sizes.rs @@ -60,7 +60,7 @@ pub fn check_envelope_size_limits(config: &Config, envelope: &Envelope) -> bool /// If Relay is configured to drop unknown items, this function removes them from the Envelope. All /// known items will be retained. pub fn remove_unknown_items(config: &Config, envelope: &mut Envelope) { - if !config.forward_unknown_items() { + if !config.accept_unknown_items() { envelope.retain_items(|item| match item.ty() { ItemType::Unknown(ty) => { relay_log::debug!("dropping unknown item of type '{}'", ty); diff --git a/tests/integration/test_envelope.py b/tests/integration/test_envelope.py index ba700b3261..31f96589ad 100644 --- a/tests/integration/test_envelope.py +++ b/tests/integration/test_envelope.py @@ -1,7 +1,7 @@ import pytest +import queue -from requests.exceptions import HTTPError -from sentry_sdk.envelope import Envelope +from sentry_sdk.envelope import Envelope, Item, PayloadRef def test_envelope(mini_sentry, relay_chain): @@ -23,11 +23,11 @@ def test_envelope_empty(mini_sentry, relay): mini_sentry.add_basic_project_config(PROJECT_ID) envelope = Envelope() + relay.send_envelope(PROJECT_ID, envelope) - with pytest.raises(HTTPError) as excinfo: - relay.send_envelope(PROJECT_ID, envelope) - - assert excinfo.value.response.status_code == 400 + # there is nothing sent to the upstream + with pytest.raises(queue.Empty): + mini_sentry.captured_events.get(timeout=1) def test_envelope_without_header(mini_sentry, relay): @@ -47,6 +47,38 @@ def test_envelope_without_header(mini_sentry, relay): assert event["logentry"] == {"formatted": "Hello, World!"} +def test_unknown_item(mini_sentry, relay): + relay = relay(mini_sentry) + PROJECT_ID = 42 + mini_sentry.add_basic_project_config(PROJECT_ID) + + envelope = Envelope() + envelope.add_item( + Item(payload=PayloadRef(bytes=b"something"), type="invalid_unknown") + ) + relay.send_envelope(PROJECT_ID, envelope) + + envelope = mini_sentry.captured_events.get(timeout=1) + assert len(envelope.items) == 1 + assert envelope.items[0].type == "invalid_unknown" + + +def test_drop_unknown_item(mini_sentry, relay): + relay = relay(mini_sentry, {"routing": {"unknown_items": False}}) + PROJECT_ID = 42 + mini_sentry.add_basic_project_config(PROJECT_ID) + + envelope = Envelope() + envelope.add_item( + Item(payload=PayloadRef(bytes=b"something"), type="invalid_unknown") + ) + relay.send_envelope(PROJECT_ID, envelope) + + # there is nothing sent to the upstream + with pytest.raises(queue.Empty): + mini_sentry.captured_events.get(timeout=1) + + def generate_transaction_item(): return { "event_id": "d2132d31b39445f1938d7e21b6bf0ec4", From 4b56ecb0c0b578abb81e312527addcd99a961208 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Thu, 28 Apr 2022 20:02:30 +0200 Subject: [PATCH 6/6] ref: Rename option to accept_unknown_items --- relay-config/src/config.rs | 4 ++-- tests/integration/test_envelope.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/relay-config/src/config.rs b/relay-config/src/config.rs index 9723a53529..774d013a8f 100644 --- a/relay-config/src/config.rs +++ b/relay-config/src/config.rs @@ -589,7 +589,7 @@ pub struct Routing { /// /// Defaults to `true` for all Relay modes other than processing mode. In processing mode, this /// is disabled by default since the item cannot be handled. - unknown_items: Option, + accept_unknown_items: Option, } /// Http content encoding for both incoming and outgoing web requests. @@ -1961,7 +1961,7 @@ impl Config { /// Returns `true` if unknown items should be accepted and forwarded. pub fn accept_unknown_items(&self) -> bool { - let forward = self.values.routing.unknown_items; + let forward = self.values.routing.accept_unknown_items; forward.unwrap_or_else(|| !self.processing_enabled()) } } diff --git a/tests/integration/test_envelope.py b/tests/integration/test_envelope.py index 31f96589ad..6b079b3157 100644 --- a/tests/integration/test_envelope.py +++ b/tests/integration/test_envelope.py @@ -64,7 +64,7 @@ def test_unknown_item(mini_sentry, relay): def test_drop_unknown_item(mini_sentry, relay): - relay = relay(mini_sentry, {"routing": {"unknown_items": False}}) + relay = relay(mini_sentry, {"routing": {"accept_unknown_items": False}}) PROJECT_ID = 42 mini_sentry.add_basic_project_config(PROJECT_ID)