From da1d616d35e1d6ba6bbc491a0bcc6ac1b1bedda8 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 16 May 2024 11:10:49 +0200 Subject: [PATCH] fix(spans): Restrict usage of otel endpoint (#3597) Span ingestion was limited in general by the `organizations:standalone-span-ingestion` feature, but was GA'd for INP spans. However, we still want to restrict access to the experimental OTel `/spans/` endpoint, which is why this PR introduces a new feature `projects:relay-otel-endpoint`. --- CHANGELOG.md | 1 + relay-dynamic-config/src/feature.rs | 7 +++- relay-server/src/endpoints/spans.rs | 2 ++ relay-server/src/envelope.rs | 21 +++++++++++- relay-server/src/services/outcome.rs | 5 +++ relay-server/src/services/project.rs | 26 ++++++++++++--- tests/integration/test_spans.py | 48 ++++++++++++++++++++++++++++ 7 files changed, 103 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df1c3c3445..1028c42033 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ - Add support for `event.` in the `Span` `Getter` implementation. ([#3577](https://github.com/getsentry/relay/pull/3577)) - Ensure `chunk_id` and `profiler_id` are UUIDs and sort samples. ([#3588](https://github.com/getsentry/relay/pull/3588)) - Add a calculated measurement based on the AI model and the tokens used. ([#3554](https://github.com/getsentry/relay/pull/3554)) +- Restrict usage of OTel endpoint. ([#3597](github.com/getsentry/relay/pull/3597)) ## 24.4.2 diff --git a/relay-dynamic-config/src/feature.rs b/relay-dynamic-config/src/feature.rs index 0d5f2435bb..0b4daad668 100644 --- a/relay-dynamic-config/src/feature.rs +++ b/relay-dynamic-config/src/feature.rs @@ -3,7 +3,7 @@ use std::collections::BTreeSet; use serde::{Deserialize, Serialize}; /// Features exposed by project config. -#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub enum Feature { /// Enables ingestion of Session Replays (Replay Recordings and Replay Events). /// @@ -56,6 +56,11 @@ pub enum Feature { /// Serialized as `organizations:standalone-span-ingestion`. #[serde(rename = "organizations:standalone-span-ingestion")] StandaloneSpanIngestion, + /// Enable standalone span ingestion via the `/spans/` OTel endpoint. + /// + /// Serialized as `projects:relay-otel-endpoint`. + #[serde(rename = "projects:relay-otel-endpoint")] + OtelEndpoint, /// Enable processing and extracting data from profiles that would normally be dropped by dynamic sampling. /// /// This is required for [slowest function aggregation](https://github.com/getsentry/snuba/blob/b5311b404a6bd73a9e1997a46d38e7df88e5f391/snuba/snuba_migrations/functions/0001_functions.py#L209-L256). The profile payload will be dropped on the sentry side. diff --git a/relay-server/src/endpoints/spans.rs b/relay-server/src/endpoints/spans.rs index 6a11f78af1..41e6882aa7 100644 --- a/relay-server/src/endpoints/spans.rs +++ b/relay-server/src/endpoints/spans.rs @@ -7,6 +7,7 @@ use axum_extra::protobuf::Protobuf; use bytes::Bytes; use relay_config::Config; +use relay_dynamic_config::Feature; use relay_spans::otel_trace::TracesData; use crate::endpoints::common; @@ -36,6 +37,7 @@ where }; let mut envelope = Envelope::from_request(None, meta); + envelope.require_feature(Feature::OtelEndpoint); for resource_span in trace.resource_spans { for scope_span in resource_span.scope_spans { for span in scope_span.spans { diff --git a/relay-server/src/envelope.rs b/relay-server/src/envelope.rs index 3dba72ef5a..be95a1006e 100644 --- a/relay-server/src/envelope.rs +++ b/relay-server/src/envelope.rs @@ -40,7 +40,7 @@ use uuid::Uuid; use bytes::Bytes; use chrono::{DateTime, Utc}; -use relay_dynamic_config::ErrorBoundary; +use relay_dynamic_config::{ErrorBoundary, Feature}; use relay_event_normalization::{normalize_transaction_name, TransactionNameRule}; use relay_event_schema::protocol::{Event, EventId, EventType}; use relay_protocol::{Annotated, Value}; @@ -1039,6 +1039,13 @@ pub struct EnvelopeHeaders { #[serde(default, skip_serializing_if = "Option::is_none")] trace: Option>, + /// A list of features required to process this envelope. + /// + /// This is an internal field that should only be set by Relay. + /// It is a serializable header such that it persists when spooling. + #[serde(default, skip_serializing_if = "SmallVec::is_empty")] + required_features: SmallVec<[Feature; 1]>, + /// Other attributes for forward compatibility. #[serde(flatten)] other: BTreeMap, @@ -1079,6 +1086,7 @@ impl EnvelopeHeaders { retention: self.retention, sent_at: self.sent_at, trace: self.trace, + required_features: self.required_features, other: self.other, }) } @@ -1121,6 +1129,7 @@ impl Envelope { sent_at: None, other: BTreeMap::new(), trace: None, + required_features: smallvec::smallvec![], }, items: Items::new(), }) @@ -1276,6 +1285,16 @@ impl Envelope { self.headers.trace = Some(ErrorBoundary::Ok(dsc)); } + /// Features required to process this envelope. + pub fn required_features(&self) -> &[Feature] { + &self.headers.required_features + } + + /// Add a feature requirement to this envelope. + pub fn require_feature(&mut self, feature: Feature) { + self.headers.required_features.push(feature) + } + /// Returns the specified header value, if present. #[cfg_attr(not(feature = "processing"), allow(dead_code))] pub fn get_header(&self, name: &K) -> Option<&Value> diff --git a/relay-server/src/services/outcome.rs b/relay-server/src/services/outcome.rs index 016f119f9b..3ebedea859 100644 --- a/relay-server/src/services/outcome.rs +++ b/relay-server/src/services/outcome.rs @@ -18,6 +18,7 @@ use chrono::{DateTime, SecondsFormat, Utc}; use relay_base_schema::project::ProjectId; use relay_common::time::UnixTimestamp; use relay_config::{Config, EmitOutcomes}; +use relay_dynamic_config::Feature; use relay_event_schema::protocol::{ClientReport, DiscardedEvent, EventId}; use relay_filter::FilterStatKey; #[cfg(feature = "processing")] @@ -377,6 +378,9 @@ pub enum DiscardReason { /// (Relay) A span is not valid after normalization. InvalidSpan, + + /// (Relay) A required feature is not enabled. + FeatureDisabled(Feature), } impl DiscardReason { @@ -421,6 +425,7 @@ impl DiscardReason { DiscardReason::InvalidReplayVideoEvent => "invalid_replay_video", DiscardReason::Profiling(reason) => reason, DiscardReason::InvalidSpan => "invalid_span", + DiscardReason::FeatureDisabled(_) => "feature_disabled", } } } diff --git a/relay-server/src/services/project.rs b/relay-server/src/services/project.rs index 0f136fa0db..e73e9c5d3e 100644 --- a/relay-server/src/services/project.rs +++ b/relay-server/src/services/project.rs @@ -22,6 +22,7 @@ use smallvec::SmallVec; use tokio::time::Instant; use url::Url; +use crate::envelope::Envelope; use crate::metrics::MetricOutcomes; use crate::services::outcome::{DiscardReason, Outcome, TrackOutcome}; #[cfg(feature = "processing")] @@ -330,17 +331,23 @@ impl ProjectState { Ok(()) } - /// Determines whether the given request should be accepted or discarded. + /// Determines whether the given envelope should be accepted or discarded. /// - /// Returns `Ok(())` if the request should be accepted. Returns `Err(DiscardReason)` if the - /// request should be discarded, by indicating the reason. The checks preformed for this are: + /// Returns `Ok(())` if the envelope should be accepted. Returns `Err(DiscardReason)` if the + /// envelope should be discarded, by indicating the reason. The checks preformed for this are: /// /// - Allowed origin headers /// - Disabled or unknown projects /// - Disabled project keys (DSN) - pub fn check_request(&self, meta: &RequestMeta, config: &Config) -> Result<(), DiscardReason> { + /// - Feature flags + pub fn check_envelope( + &self, + envelope: &Envelope, + config: &Config, + ) -> Result<(), DiscardReason> { // Verify that the stated project id in the DSN matches the public key used to retrieve this // project state. + let meta = envelope.meta(); if !self.is_valid_project_id(meta.project_id(), config) { return Err(DiscardReason::ProjectId); } @@ -359,6 +366,15 @@ impl ProjectState { // Check for invalid or disabled projects. self.check_disabled(config)?; + // Check feature. + if let Some(disabled_feature) = envelope + .required_features() + .iter() + .find(|f| !self.has_feature(**f)) + { + return Err(DiscardReason::FeatureDisabled(*disabled_feature)); + } + Ok(()) } @@ -1040,7 +1056,7 @@ impl Project { scoping = state.scope_request(envelope.envelope().meta()); envelope.scope(scoping); - if let Err(reason) = state.check_request(envelope.envelope().meta(), &self.config) { + if let Err(reason) = state.check_envelope(envelope.envelope(), &self.config) { envelope.reject(Outcome::Invalid(reason)); return Err(reason); } diff --git a/tests/integration/test_spans.py b/tests/integration/test_spans.py index c0d4e261ce..6433e6281e 100644 --- a/tests/integration/test_spans.py +++ b/tests/integration/test_spans.py @@ -13,6 +13,8 @@ import pytest +from requests import HTTPError +from sentry_relay.consts import DataCategory from sentry_sdk.envelope import Envelope, Item, PayloadRef from .test_store import make_transaction @@ -434,6 +436,7 @@ def test_span_ingestion( project_config["config"]["features"] = [ "organizations:standalone-span-ingestion", "projects:span-metrics-extraction", + "projects:relay-otel-endpoint", ] project_config["config"]["transactionMetrics"] = {"version": 1} if extract_transaction: @@ -833,6 +836,50 @@ def test_span_ingestion( metrics_consumer.assert_empty() +def test_otel_endpoint_disabled(mini_sentry, relay): + relay = relay( + mini_sentry, + { + "outcomes": { + "emit_outcomes": True, + "batch_size": 1, + "batch_interval": 1, + "source": "relay", + } + }, + ) + project_id = 42 + project_config = mini_sentry.add_full_project_config(project_id)["config"] + project_config["features"] = ["organizations:standalone-span-ingestion"] + + end = datetime.now(timezone.utc) - timedelta(seconds=1) + start = end - timedelta(milliseconds=500) + relay.send_otel_span( + project_id, + json=make_otel_span(start, end), + ) + + (outcome,) = mini_sentry.captured_outcomes.get()["outcomes"] + assert outcome["category"] == DataCategory.SPAN + assert outcome["outcome"] == 3 # invalid + assert outcome["reason"] == "feature_disabled" + + # Second attempt will cause a 403 response: + with pytest.raises(HTTPError) as exc_info: + relay.send_otel_span( + project_id, + json=make_otel_span(start, end), + ) + response = exc_info.value.response + assert response.status_code == 403 + assert response.json() == { + "detail": "event submission rejected with_reason: FeatureDisabled(OtelEndpoint)" + } + + # No envelopes were received: + assert mini_sentry.captured_events.empty() + + def test_span_extraction_with_metrics_summary( mini_sentry, relay_with_processing, @@ -926,6 +973,7 @@ def test_extracted_transaction_gets_normalized( project_config["config"]["features"] = [ "organizations:standalone-span-ingestion", "projects:extract-transaction-from-segment-span", + "projects:relay-otel-endpoint", ] transactions_consumer = transactions_consumer()