fix(server): Forward-compatible AttachmentType (#1638)

As we did in #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 getsentry/rfcs#33, which introduces a new attachment type.
getsentry · Nov 28, 2022 · ab46f30 · ab46f30
1 parent dba7030
commit ab46f30
Show file tree

Hide file tree

Showing 9 changed files with 105 additions and 37 deletions.
diff --git 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
@@ -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::<u64>();
 
@@ -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
@@ -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
@@ -203,7 +203,7 @@ pub fn event_id_from_items(items: &Items) -> Result<Option<EventId>, 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
@@ -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
@@ -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<Self, Self::Err> {
+        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<AttachmentType> {
+    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
@@ -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
@@ -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
@@ -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)