fix(server): Forward-compatible AttachmentType

CHANGELOG.md

@@ -6,6 +6,7 @@
 
 - Dynamic sampling is now based on the volume received by Relay by default and does not include the original volume dropped by client-side sampling in SDKs. This is required for the final dynamic sampling feature in the latest Sentry plans. ([#1591](https://github.com/getsentry/relay/pull/1591))
 - Add OpenTelemetry Context ([#1617](https://github.com/getsentry/relay/pull/1617))
+- Make `attachment_type` on envelope items forward compatible by adding fallback variant. ([#1638](https://github.com/getsentry/relay/pull/1638))
 
 **Internal**:
 

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();

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()),

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));

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),

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

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,
+            },
         });
     }
 }

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() {

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)