From 82847f62771db327119d64d07c76e5000225457b Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Mon, 9 Sep 2024 18:37:37 +0200 Subject: [PATCH 1/2] Rust intropsection: disable max depth rule This protection against introspection queries generating huge responses was added recently in graphql-js https://github.com/graphql/graphql-js/pull/4118 and ported to rust https://github.com/apollographql/apollo-rs/pull/904, but is not yet present in the graphql-js version used by router-bridge. This disables it for now from Rust introspection, in order to match the current state of JS introspection. Adding this rule (in both implementations) can be revisited separately. In particular: the depth limit is hard-coded to 3. Is that the right number? Should it be configurable? Is the rule checking the right set of fields? --- apollo-router/src/query_planner/bridge_query_planner.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 30ebd5b36b2..4d94335e813 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -559,12 +559,6 @@ impl BridgeQueryPlanner { ) -> Result { let schema = self.schema.api_schema(); let operation = doc.get_operation(key.operation_name.as_deref())?; - apollo_compiler::execution::check_introspection_max_depth(&doc.executable, operation) - .map_err(|_e| { - QueryPlannerError::Introspection(IntrospectionError { - message: Some("Maximum introspection depth exceeded".to_owned()), - }) - })?; let variable_values = Default::default(); let variable_values = apollo_compiler::execution::coerce_variable_values(schema, operation, &variable_values) From f626c48dfefe359871a5ac18a82b7659051d6266 Mon Sep 17 00:00:00 2001 From: Coenen Benjamin Date: Thu, 5 Sep 2024 14:38:28 +0200 Subject: [PATCH 2/2] fix trace integration test (#5962) Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com> --- .../src/plugins/telemetry/otel/layer.rs | 1 - .../tests/integration/telemetry/datadog.rs | 18 +++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/apollo-router/src/plugins/telemetry/otel/layer.rs b/apollo-router/src/plugins/telemetry/otel/layer.rs index e1d20ec739d..866bf50a350 100644 --- a/apollo-router/src/plugins/telemetry/otel/layer.rs +++ b/apollo-router/src/plugins/telemetry/otel/layer.rs @@ -1101,7 +1101,6 @@ where attributes.insert(OTEL_ORIGINAL_NAME.into(), builder.name.into()); builder.name = forced_span_name.into(); } - // Assign end time, build and start span, drop span to export builder .with_end_time(SystemTime::now()) diff --git a/apollo-router/tests/integration/telemetry/datadog.rs b/apollo-router/tests/integration/telemetry/datadog.rs index a88f16160b4..d2d3f58d255 100644 --- a/apollo-router/tests/integration/telemetry/datadog.rs +++ b/apollo-router/tests/integration/telemetry/datadog.rs @@ -52,6 +52,7 @@ async fn test_default_span_names() -> Result<(), BoxError> { .unwrap(), id.to_datadog() ); + router.graceful_shutdown().await; TraceSpec::builder() .services(["client", "router", "subgraph"].into()) .span_names( @@ -73,7 +74,6 @@ async fn test_default_span_names() -> Result<(), BoxError> { .build() .validate_trace(id) .await?; - router.graceful_shutdown().await; Ok(()) } @@ -104,6 +104,7 @@ async fn test_override_span_names() -> Result<(), BoxError> { .unwrap(), id.to_datadog() ); + router.graceful_shutdown().await; TraceSpec::builder() .services(["client", "router", "subgraph"].into()) .span_names( @@ -125,7 +126,6 @@ async fn test_override_span_names() -> Result<(), BoxError> { .build() .validate_trace(id) .await?; - router.graceful_shutdown().await; Ok(()) } @@ -156,6 +156,7 @@ async fn test_override_span_names_late() -> Result<(), BoxError> { .unwrap(), id.to_datadog() ); + router.graceful_shutdown().await; TraceSpec::builder() .services(["client", "router", "subgraph"].into()) .span_names( @@ -177,7 +178,6 @@ async fn test_override_span_names_late() -> Result<(), BoxError> { .build() .validate_trace(id) .await?; - router.graceful_shutdown().await; Ok(()) } @@ -206,6 +206,7 @@ async fn test_basic() -> Result<(), BoxError> { .unwrap(), id.to_datadog() ); + router.graceful_shutdown().await; TraceSpec::builder() .operation_name("ExampleQuery") .services(["client", "router", "subgraph"].into()) @@ -240,7 +241,6 @@ async fn test_basic() -> Result<(), BoxError> { .build() .validate_trace(id) .await?; - router.graceful_shutdown().await; Ok(()) } @@ -274,6 +274,7 @@ async fn test_with_parent_span() -> Result<(), BoxError> { .unwrap(), id.to_datadog() ); + router.graceful_shutdown().await; TraceSpec::builder() .operation_name("ExampleQuery") .services(["client", "router", "subgraph"].into()) @@ -308,7 +309,6 @@ async fn test_with_parent_span() -> Result<(), BoxError> { .build() .validate_trace(id) .await?; - router.graceful_shutdown().await; Ok(()) } @@ -383,6 +383,7 @@ async fn test_resource_mapping_override() -> Result<(), BoxError> { .get("apollo-custom-trace-id") .unwrap() .is_empty()); + router.graceful_shutdown().await; TraceSpec::builder() .services(["client", "router", "subgraph"].into()) .span_names( @@ -403,7 +404,6 @@ async fn test_resource_mapping_override() -> Result<(), BoxError> { .build() .validate_trace(id) .await?; - router.graceful_shutdown().await; Ok(()) } @@ -428,6 +428,7 @@ async fn test_span_metrics() -> Result<(), BoxError> { .get("apollo-custom-trace-id") .unwrap() .is_empty()); + router.graceful_shutdown().await; TraceSpec::builder() .operation_name("ExampleQuery") .services(["client", "router", "subgraph"].into()) @@ -450,7 +451,6 @@ async fn test_span_metrics() -> Result<(), BoxError> { .build() .validate_trace(id) .await?; - router.graceful_shutdown().await; Ok(()) } @@ -495,12 +495,12 @@ impl TraceSpec { .await?; tracing::debug!("{}", serde_json::to_string_pretty(&trace)?); self.verify_trace_participants(&trace)?; + self.verify_spans_present(&trace)?; + self.validate_measured_spans(&trace)?; self.verify_operation_name(&trace)?; self.verify_priority_sampled(&trace)?; self.verify_version(&trace)?; - self.verify_spans_present(&trace)?; self.validate_span_kinds(&trace)?; - self.validate_measured_spans(&trace)?; Ok(()) }