From ab46f30a4fcf3682f60cdc6f57a1d16f21c7f7cc Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Mon, 28 Nov 2022 13:12:28 +0100 Subject: [PATCH] fix(server): Forward-compatible AttachmentType (#1638) As we did in https://github.com/getsentry/relay/pull/1246 for `ItemType`, add an `Unknown(String)` variant to `AttachmentType`. When `accept_unknown_items` is set to false, items with this attachment type will be dropped. This will allow outdated external Relays to forward new attachment types instead of dropping the entire envelope. This defect was discovered in https://github.com/getsentry/rfcs/pull/33, which introduces a new attachment type. --- CHANGELOG.md | 3 + relay-server/src/actors/processor.rs | 14 ++--- relay-server/src/actors/store.rs | 2 +- relay-server/src/endpoints/common.rs | 2 +- relay-server/src/endpoints/minidump.rs | 2 +- relay-server/src/envelope.rs | 78 +++++++++++++++++++------- relay-server/src/utils/sizes.rs | 10 +++- relay-server/src/utils/unreal.rs | 4 +- tests/integration/test_envelope.py | 27 +++++++-- 9 files changed, 105 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48cf1421da..3183ec84ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ - Add OpenTelemetry Context ([#1617](https://github.com/getsentry/relay/pull/1617)) - Add `app.in_foreground` and `thread.main` flag to protocol. ([#1578](https://github.com/getsentry/relay/pull/1578)) +**Bug Fixes**: +- Make `attachment_type` on envelope items forward compatible by adding fallback variant. ([#1638](https://github.com/getsentry/relay/pull/1638)) + **Internal**: - Emit a `service.back_pressure` metric that measures internal back pressure by service. ([#1583](https://github.com/getsentry/relay/pull/1583)) diff --git a/relay-server/src/actors/processor.rs b/relay-server/src/actors/processor.rs index fba743397d..eb90e4f8bf 100644 --- a/relay-server/src/actors/processor.rs +++ b/relay-server/src/actors/processor.rs @@ -1471,11 +1471,11 @@ impl EnvelopeProcessorService { 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)); + .take_item_by(|item| item.attachment_type() == Some(&AttachmentType::EventPayload)); let breadcrumbs1 = envelope - .take_item_by(|item| item.attachment_type() == Some(AttachmentType::Breadcrumbs)); + .take_item_by(|item| item.attachment_type() == Some(&AttachmentType::Breadcrumbs)); let breadcrumbs2 = envelope - .take_item_by(|item| item.attachment_type() == Some(AttachmentType::Breadcrumbs)); + .take_item_by(|item| item.attachment_type() == Some(&AttachmentType::Breadcrumbs)); // Event items can never occur twice in an envelope. if let Some(duplicate) = envelope.get_item_by(|item| self.is_duplicate(item)) { @@ -1550,9 +1550,9 @@ impl EnvelopeProcessorService { let envelope = &mut state.envelope; let minidump_attachment = - envelope.get_item_by(|item| item.attachment_type() == Some(AttachmentType::Minidump)); + envelope.get_item_by(|item| item.attachment_type() == Some(&AttachmentType::Minidump)); let apple_crash_report_attachment = envelope - .get_item_by(|item| item.attachment_type() == Some(AttachmentType::AppleCrashReport)); + .get_item_by(|item| item.attachment_type() == Some(&AttachmentType::AppleCrashReport)); if let Some(item) = minidump_attachment { let event = state.event.get_or_insert_with(Event::default); @@ -1608,7 +1608,7 @@ impl EnvelopeProcessorService { let attachment_size = envelope .items() - .filter(|item| item.attachment_type() == Some(AttachmentType::Attachment)) + .filter(|item| item.attachment_type() == Some(&AttachmentType::Attachment)) .map(|item| item.len() as u64) .sum::(); @@ -1896,7 +1896,7 @@ impl EnvelopeProcessorService { let envelope = &mut state.envelope; if let Some(ref config) = state.project_state.config.pii_config { let minidump = envelope - .get_item_by_mut(|item| item.attachment_type() == Some(AttachmentType::Minidump)); + .get_item_by_mut(|item| item.attachment_type() == Some(&AttachmentType::Minidump)); if let Some(item) = minidump { let filename = item.filename().unwrap_or_default(); diff --git a/relay-server/src/actors/store.rs b/relay-server/src/actors/store.rs index 6e5b774871..c027fa2248 100644 --- a/relay-server/src/actors/store.rs +++ b/relay-server/src/actors/store.rs @@ -316,7 +316,7 @@ impl StoreService { content_type: item .content_type() .map(|content_type| content_type.as_str().to_owned()), - attachment_type: item.attachment_type().unwrap_or_default(), + attachment_type: item.attachment_type().cloned().unwrap_or_default(), chunks: chunk_index, size: Some(size), rate_limited: Some(item.rate_limited()), diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index 64e8feaa31..e486fc412e 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -203,7 +203,7 @@ pub fn event_id_from_items(items: &Items) -> Result, BadStoreReq if let Some(item) = items .iter() - .find(|item| item.attachment_type() == Some(AttachmentType::EventPayload)) + .find(|item| item.attachment_type() == Some(&AttachmentType::EventPayload)) { if let Some(event_id) = event_id_from_msgpack(&item.payload())? { return Ok(Some(event_id)); diff --git a/relay-server/src/endpoints/minidump.rs b/relay-server/src/endpoints/minidump.rs index dd10fea530..406832a35e 100644 --- a/relay-server/src/endpoints/minidump.rs +++ b/relay-server/src/endpoints/minidump.rs @@ -126,7 +126,7 @@ fn extract_envelope( .and_then(move |mut items| { let minidump_index = items .iter() - .position(|item| item.attachment_type() == Some(AttachmentType::Minidump)); + .position(|item| item.attachment_type() == Some(&AttachmentType::Minidump)); let mut minidump_item = match minidump_index { Some(index) => items.swap_remove(index), diff --git a/relay-server/src/envelope.rs b/relay-server/src/envelope.rs index 61cc81dff2..5f7840a681 100644 --- a/relay-server/src/envelope.rs +++ b/relay-server/src/envelope.rs @@ -321,24 +321,20 @@ relay_common::impl_str_de!(ContentType, "a content type string"); /// The type of an event attachment. /// /// These item types must align with the Sentry processing pipeline. -#[derive(Clone, Copy, Debug, Eq, PartialEq, Deserialize, Serialize)] +#[derive(Clone, Debug, Eq, PartialEq)] pub enum AttachmentType { /// A regular attachment without special meaning. - #[serde(rename = "event.attachment")] Attachment, /// A minidump crash report (binary data). - #[serde(rename = "event.minidump")] Minidump, /// An apple crash report (text data). - #[serde(rename = "event.applecrashreport")] AppleCrashReport, /// A msgpack-encoded event payload submitted as part of multipart uploads. /// /// This attachment is processed by Relay immediately and never forwarded or persisted. - #[serde(rename = "event.payload")] EventPayload, /// A msgpack-encoded list of payloads. @@ -347,7 +343,6 @@ pub enum AttachmentType { /// will be merged and truncated to the maxmimum number of allowed attachments. /// /// This attachment is processed by Relay immediately and never forwarded or persisted. - #[serde(rename = "event.breadcrumbs")] Breadcrumbs, /// This is a binary attachment present in Unreal 4 events containing event context information. @@ -356,7 +351,6 @@ pub enum AttachmentType { /// [`symbolic_unreal::Unreal4Context`]. /// /// [`symbolic_unreal::Unreal4Context`]: https://docs.rs/symbolic/*/symbolic/unreal/struct.Unreal4Context.html - #[serde(rename = "unreal.context")] UnrealContext, /// This is a binary attachment present in Unreal 4 events containing event Logs. @@ -365,8 +359,11 @@ pub enum AttachmentType { /// [`symbolic_unreal::Unreal4LogEntry`]. /// /// [`symbolic_unreal::Unreal4LogEntry`]: https://docs.rs/symbolic/*/symbolic/unreal/struct.Unreal4LogEntry.html - #[serde(rename = "unreal.logs")] UnrealLogs, + + /// Unknown attachment type, forwarded for compatibility. + /// Attachments with this type will be dropped if `accept_unknown_items` is set to false. + Unknown(String), } impl Default for AttachmentType { @@ -375,6 +372,43 @@ impl Default for AttachmentType { } } +impl fmt::Display for AttachmentType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + AttachmentType::Attachment => write!(f, "event.attachment"), + AttachmentType::Minidump => write!(f, "event.minidump"), + AttachmentType::AppleCrashReport => write!(f, "event.applecrashreport"), + AttachmentType::EventPayload => write!(f, "event.payload"), + AttachmentType::Breadcrumbs => write!(f, "event.breadcrumbs"), + AttachmentType::UnrealContext => write!(f, "unreal.context"), + AttachmentType::UnrealLogs => write!(f, "unreal.logs"), + AttachmentType::Unknown(s) => s.fmt(f), + } + } +} + +impl std::str::FromStr for AttachmentType { + type Err = std::convert::Infallible; + + fn from_str(s: &str) -> Result { + Ok(match s { + "event.attachment" => AttachmentType::Attachment, + "event.minidump" => AttachmentType::Minidump, + "event.applecrashreport" => AttachmentType::AppleCrashReport, + "event.payload" => AttachmentType::EventPayload, + "event.breadcrumbs" => AttachmentType::Breadcrumbs, + "unreal.context" => AttachmentType::UnrealContext, + "unreal.logs" => AttachmentType::UnrealLogs, + other => AttachmentType::Unknown(other.to_owned()), + }) + } +} + +relay_common::impl_str_serde!( + AttachmentType, + "an attachment type (see sentry develop docs)" +); + fn is_false(val: &bool) -> bool { !*val } @@ -491,9 +525,9 @@ impl Item { } /// Returns the attachment type if this item is an attachment. - pub fn attachment_type(&self) -> Option { + pub fn attachment_type(&self) -> Option<&AttachmentType> { // TODO: consider to replace this with an ItemType? - self.headers.attachment_type + self.headers.attachment_type.as_ref() } /// Sets the attachment type of this item. @@ -607,15 +641,21 @@ impl Item { // Attachments are only event items if they are crash reports or if they carry partial // event payloads. Plain attachments never create event payloads. - ItemType::Attachment => match self.attachment_type().unwrap_or_default() { - AttachmentType::AppleCrashReport - | AttachmentType::Minidump - | AttachmentType::EventPayload - | AttachmentType::Breadcrumbs => true, - AttachmentType::Attachment - | AttachmentType::UnrealContext - | AttachmentType::UnrealLogs => false, - }, + ItemType::Attachment => { + match self.attachment_type().unwrap_or(&AttachmentType::default()) { + AttachmentType::AppleCrashReport + | AttachmentType::Minidump + | AttachmentType::EventPayload + | AttachmentType::Breadcrumbs => true, + AttachmentType::Attachment + | AttachmentType::UnrealContext + | AttachmentType::UnrealLogs => false, + // When an outdated Relay instance forwards an unknown attachment type for compatibility, + // we assume that the attachment does not create a new event. This will make it hard + // to introduce new attachment types which _do_ create a new event. + AttachmentType::Unknown(_) => false, + } + } // Form data items may contain partial event payloads, but those are only ever valid if // they occur together with an explicit event item, such as a minidump or apple crash diff --git a/relay-server/src/utils/sizes.rs b/relay-server/src/utils/sizes.rs index 90863514be..1c05bf7750 100644 --- a/relay-server/src/utils/sizes.rs +++ b/relay-server/src/utils/sizes.rs @@ -1,6 +1,6 @@ use relay_config::Config; -use crate::envelope::{Envelope, ItemType}; +use crate::envelope::{AttachmentType, Envelope, ItemType}; /// Checks for size limits of items in this envelope. /// @@ -67,7 +67,13 @@ pub fn remove_unknown_items(config: &Config, envelope: &mut Envelope) { relay_log::debug!("dropping unknown item of type '{}'", ty); false } - _ => true, + _ => match item.attachment_type() { + Some(AttachmentType::Unknown(ty)) => { + relay_log::debug!("dropping unknown attachment of type '{}'", ty); + false + } + _ => true, + }, }); } } diff --git a/relay-server/src/utils/unreal.rs b/relay-server/src/utils/unreal.rs index 38e538cdcb..e6921459b1 100644 --- a/relay-server/src/utils/unreal.rs +++ b/relay-server/src/utils/unreal.rs @@ -272,9 +272,9 @@ pub fn process_unreal_envelope( .get_header(UNREAL_USER_HEADER) .and_then(Value::as_str); let context_item = - envelope.get_item_by(|item| item.attachment_type() == Some(AttachmentType::UnrealContext)); + envelope.get_item_by(|item| item.attachment_type() == Some(&AttachmentType::UnrealContext)); let logs_item = - envelope.get_item_by(|item| item.attachment_type() == Some(AttachmentType::UnrealLogs)); + envelope.get_item_by(|item| item.attachment_type() == Some(&AttachmentType::UnrealLogs)); // Early exit if there is no information. if user_header.is_none() && context_item.is_none() && logs_item.is_none() { diff --git a/tests/integration/test_envelope.py b/tests/integration/test_envelope.py index 0ebf75d0c3..97b2665d4a 100644 --- a/tests/integration/test_envelope.py +++ b/tests/integration/test_envelope.py @@ -56,11 +56,23 @@ def test_unknown_item(mini_sentry, relay): envelope.add_item( Item(payload=PayloadRef(bytes=b"something"), type="invalid_unknown") ) + attachment = Item(payload=PayloadRef(bytes=b"something"), type="attachment") + attachment.headers["attachment_type"] = "attachment_unknown" + envelope.add_item(attachment) 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" + envelopes = [ # non-event items are split into separate envelopes, so fetch 2x here + mini_sentry.captured_events.get(timeout=1), + mini_sentry.captured_events.get(timeout=1), + ] + + types = { + (item.type, item.headers.get("attachment_type")) + for envelope in envelopes + for item in envelope.items + } + + assert types == {("invalid_unknown", None), ("attachment", "attachment_unknown")} def test_drop_unknown_item(mini_sentry, relay): @@ -69,12 +81,19 @@ def test_drop_unknown_item(mini_sentry, relay): mini_sentry.add_basic_project_config(PROJECT_ID) envelope = Envelope() + envelope.add_item(Item(payload=PayloadRef(bytes=b"something"), type="attachment")) envelope.add_item( Item(payload=PayloadRef(bytes=b"something"), type="invalid_unknown") ) + attachment = Item(payload=PayloadRef(bytes=b"something"), type="attachment") + attachment.headers["attachment_type"] = "attachment_unknown" + envelope.add_item(attachment) relay.send_envelope(PROJECT_ID, envelope) - # there is nothing sent to the upstream + envelope = mini_sentry.captured_events.get(timeout=1) + assert len(envelope.items) == 1 + assert envelope.items[0].type == "attachment" + with pytest.raises(queue.Empty): mini_sentry.captured_events.get(timeout=1)