From 40043b8d369f0cde58cf8c46456ea900d6f3f80c Mon Sep 17 00:00:00 2001 From: Benjamin <5719034+bnjjj@users.noreply.github.com> Date: Tue, 16 Dec 2025 16:45:04 +0100 Subject: [PATCH 1/6] fix(response_cache): don't duplicate redis connection if we specify custom config for a subgraph Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/configuration/mod.rs | 4 +- .../src/plugins/response_cache/plugin.rs | 19 +++- .../plugins/response_cache/storage/config.rs | 2 +- .../tests/integration/response_cache.rs | 100 ++++++++++++++++++ .../tests/integration/telemetry/metrics.rs | 2 +- 5 files changed, 118 insertions(+), 9 deletions(-) diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 1c0f7add86..ab126e4d9b 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -1291,7 +1291,7 @@ pub(crate) fn load_key(data: &str) -> io::Result> { } /// Configuration options pertaining to the subgraph server component. -#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)] +#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, PartialEq)] #[serde(deny_unknown_fields)] #[serde(default)] pub(crate) struct TlsClient { @@ -1322,7 +1322,7 @@ impl Default for TlsClient { } /// TLS client authentication -#[derive(Debug, Deserialize, Serialize, JsonSchema)] +#[derive(Debug, Deserialize, Serialize, JsonSchema, PartialEq)] #[serde(deny_unknown_fields)] pub(crate) struct TlsClientAuth { /// list of certificates in PEM format diff --git a/apollo-router/src/plugins/response_cache/plugin.rs b/apollo-router/src/plugins/response_cache/plugin.rs index cdddf8d96e..eefcbe682e 100644 --- a/apollo-router/src/plugins/response_cache/plugin.rs +++ b/apollo-router/src/plugins/response_cache/plugin.rs @@ -310,12 +310,21 @@ impl PluginPrivate for ResponseCache { if Self::static_subgraph_enabled(init.config.enabled, &init.config.subgraph, subgraph) { match subgraph_config.redis.clone() { Some(config) => { - let storage = Arc::new(OnceLock::new()); - storage_interface - .subgraphs - .insert(subgraph.clone(), storage.clone()); - connect_or_spawn_reconnection_task(config, storage, drop_tx.subscribe()) + // We need to do this because with the SubgraphConfig struct when deserialization happens it automatically clone the config from all in all subgraphs config + // So we shouldn't try to connect to a new redis if the config does just inherit from the all config + // If it's different though it would require a new connection + if Some(&config) != init.config.subgraph.all.redis.as_ref() { + let storage = Arc::new(OnceLock::new()); + storage_interface + .subgraphs + .insert(subgraph.clone(), storage.clone()); + connect_or_spawn_reconnection_task( + config, + storage, + drop_tx.subscribe(), + ) .await?; + } } None => { if storage_interface.all.is_none() { diff --git a/apollo-router/src/plugins/response_cache/storage/config.rs b/apollo-router/src/plugins/response_cache/storage/config.rs index 022ee905ba..a5583e5842 100644 --- a/apollo-router/src/plugins/response_cache/storage/config.rs +++ b/apollo-router/src/plugins/response_cache/storage/config.rs @@ -9,7 +9,7 @@ use crate::configuration::TlsClient; use crate::configuration::default_metrics_interval; use crate::configuration::default_required_to_start; -#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)] +#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, PartialEq)] #[serde(deny_unknown_fields)] /// Redis cache configuration pub(crate) struct Config { diff --git a/apollo-router/tests/integration/response_cache.rs b/apollo-router/tests/integration/response_cache.rs index b586b9ce78..1a98a9df65 100644 --- a/apollo-router/tests/integration/response_cache.rs +++ b/apollo-router/tests/integration/response_cache.rs @@ -25,6 +25,7 @@ use tower::BoxError; use tower::Service as _; use tower::ServiceExt as _; +use crate::integration::IntegrationTest; use crate::integration::common::graph_os_enabled; const REDIS_URL: &str = "redis://127.0.0.1:6379"; @@ -89,6 +90,69 @@ fn base_config() -> Value { }) } +fn config_with_subgraph_prometheus() -> Value { + json!({ + "telemetry": { + "exporters": { + "metrics": { + "prometheus": { + "enabled": true, + "listen": "127.0.0.1:9090", + "path": "/metrics" + } + } + } + }, + "include_subgraph_errors": { + "all": true, + }, + "rhai": { + "scripts": "tests/integration/fixtures", + "main": "test_cache.rhai", + }, + "headers": { + "all": { + "request": [ + { + "propagate": { + "named": "private_id" + } + } + ] + } + }, + "response_cache": { + "enabled": true, + "subgraph": { + "all": { + "redis": { + "urls": ["redis://127.0.0.1:6379"], + "pool_size": 3, + "namespace": namespace(), + "required_to_start": true, + }, + "ttl": "10m", + "invalidation": { + "enabled": true, + "shared_key": INVALIDATION_SHARED_KEY, + }, + "private_id": "private_id" + }, + "subgraphs": { + "user": { + "enabled": true, + "private_id": "user" + } + } + }, + "invalidation": { + "listen": "127.0.0.1:4000", + "path": INVALIDATION_PATH, + }, + }, + }) +} + fn failure_config() -> Value { json!({ "include_subgraph_errors": { @@ -285,6 +349,42 @@ where (headers, serde_json::from_slice(&body).unwrap()) } +#[tokio::test(flavor = "multi_thread")] +async fn dont_duplicate_redis_connections() { + if !graph_os_enabled() { + return; + } + + let mut router = IntegrationTest::builder() + .config(serde_yaml::to_string(&config_with_subgraph_prometheus()).unwrap()) + .build() + .await; + + router.start().await; + router.assert_started().await; + + let metrics = router + .get_metrics_response() + .await + .expect("failed to fetch metrics") + .text() + .await + .unwrap(); + + check_metrics_contains( + &metrics, + r#"apollo_router_cache_redis_clients{otel_scope_name="apollo/router"} 3"#, + ); +} + +#[track_caller] +fn check_metrics_contains(metrics: &str, text: &str) { + assert!( + metrics.contains(text), + "'{text}' not detected in metrics\n{metrics}" + ); +} + #[tokio::test(flavor = "multi_thread")] async fn basic_cache_skips_subgraph_request() { if !graph_os_enabled() { diff --git a/apollo-router/tests/integration/telemetry/metrics.rs b/apollo-router/tests/integration/telemetry/metrics.rs index 513efd417e..9b634bc65b 100644 --- a/apollo-router/tests/integration/telemetry/metrics.rs +++ b/apollo-router/tests/integration/telemetry/metrics.rs @@ -84,7 +84,7 @@ async fn test_metrics_reloading() { } #[track_caller] -fn check_metrics_contains(metrics: &str, text: &str) { +pub(crate) fn check_metrics_contains(metrics: &str, text: &str) { assert!( metrics.contains(text), "'{text}' not detected in metrics\n{metrics}" From beaf217a919e3ea2c43e2e452c3e6d9b4e0aa1da Mon Sep 17 00:00:00 2001 From: Benjamin <5719034+bnjjj@users.noreply.github.com> Date: Tue, 16 Dec 2025 16:49:33 +0100 Subject: [PATCH 2/6] add changelog Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --- .changesets/fix_bnjjj_dont_duplicate_redis_connection.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changesets/fix_bnjjj_dont_duplicate_redis_connection.md diff --git a/.changesets/fix_bnjjj_dont_duplicate_redis_connection.md b/.changesets/fix_bnjjj_dont_duplicate_redis_connection.md new file mode 100644 index 0000000000..e7088983df --- /dev/null +++ b/.changesets/fix_bnjjj_dont_duplicate_redis_connection.md @@ -0,0 +1,8 @@ +### Response cache: don't duplicate redis connection if we specify custom config for a subgraph ([PR #8764](https://github.com/apollographql/router/pull/8764)) + + +Fixed an issue where the response cache would create duplicate Redis connections when a custom configuration was specified for individual subgraphs (without any specific Redis configuration). Previously, if a subgraph inherited the same Redis configuration from the global `all` setting, the router would unnecessarily establish a redundant connection. Now, the router correctly reuses the existing connection pool when the configuration is identical, improving resource efficiency and reducing connection overhead. + +**Impact**: Users with response caching enabled who specify Redis configurations at both the global and subgraph levels will see reduced Redis connection usage, leading to better resource utilization. + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/8764 \ No newline at end of file From 0f51072ad0bcb28b89a39abb6989e51031acf5a8 Mon Sep 17 00:00:00 2001 From: Benjamin <5719034+bnjjj@users.noreply.github.com> Date: Tue, 16 Dec 2025 17:00:48 +0100 Subject: [PATCH 3/6] undo pub(crate) Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --- apollo-router/tests/integration/telemetry/metrics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-router/tests/integration/telemetry/metrics.rs b/apollo-router/tests/integration/telemetry/metrics.rs index 9b634bc65b..513efd417e 100644 --- a/apollo-router/tests/integration/telemetry/metrics.rs +++ b/apollo-router/tests/integration/telemetry/metrics.rs @@ -84,7 +84,7 @@ async fn test_metrics_reloading() { } #[track_caller] -pub(crate) fn check_metrics_contains(metrics: &str, text: &str) { +fn check_metrics_contains(metrics: &str, text: &str) { assert!( metrics.contains(text), "'{text}' not detected in metrics\n{metrics}" From 5876b59dad8edc99a994553308d93168d8fa14b7 Mon Sep 17 00:00:00 2001 From: Benjamin <5719034+bnjjj@users.noreply.github.com> Date: Tue, 16 Dec 2025 17:11:03 +0100 Subject: [PATCH 4/6] address review comments Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --- .../src/plugins/response_cache/plugin.rs | 5 +- .../tests/integration/response_cache.rs | 54 +++---------------- 2 files changed, 9 insertions(+), 50 deletions(-) diff --git a/apollo-router/src/plugins/response_cache/plugin.rs b/apollo-router/src/plugins/response_cache/plugin.rs index eefcbe682e..d2b2090159 100644 --- a/apollo-router/src/plugins/response_cache/plugin.rs +++ b/apollo-router/src/plugins/response_cache/plugin.rs @@ -310,9 +310,8 @@ impl PluginPrivate for ResponseCache { if Self::static_subgraph_enabled(init.config.enabled, &init.config.subgraph, subgraph) { match subgraph_config.redis.clone() { Some(config) => { - // We need to do this because with the SubgraphConfig struct when deserialization happens it automatically clone the config from all in all subgraphs config - // So we shouldn't try to connect to a new redis if the config does just inherit from the all config - // If it's different though it would require a new connection + // We need to do this because the subgraph config automatically clones from the `all` config during deserialization. + // We don't want to create a new connection pool if the subgraph just inherits from the `all` config. if Some(&config) != init.config.subgraph.all.redis.as_ref() { let storage = Arc::new(OnceLock::new()); storage_interface diff --git a/apollo-router/tests/integration/response_cache.rs b/apollo-router/tests/integration/response_cache.rs index 1a98a9df65..a7b6574967 100644 --- a/apollo-router/tests/integration/response_cache.rs +++ b/apollo-router/tests/integration/response_cache.rs @@ -103,24 +103,6 @@ fn config_with_subgraph_prometheus() -> Value { } } }, - "include_subgraph_errors": { - "all": true, - }, - "rhai": { - "scripts": "tests/integration/fixtures", - "main": "test_cache.rhai", - }, - "headers": { - "all": { - "request": [ - { - "propagate": { - "named": "private_id" - } - } - ] - } - }, "response_cache": { "enabled": true, "subgraph": { @@ -132,10 +114,6 @@ fn config_with_subgraph_prometheus() -> Value { "required_to_start": true, }, "ttl": "10m", - "invalidation": { - "enabled": true, - "shared_key": INVALIDATION_SHARED_KEY, - }, "private_id": "private_id" }, "subgraphs": { @@ -144,11 +122,7 @@ fn config_with_subgraph_prometheus() -> Value { "private_id": "user" } } - }, - "invalidation": { - "listen": "127.0.0.1:4000", - "path": INVALIDATION_PATH, - }, + } }, }) } @@ -363,26 +337,12 @@ async fn dont_duplicate_redis_connections() { router.start().await; router.assert_started().await; - let metrics = router - .get_metrics_response() - .await - .expect("failed to fetch metrics") - .text() - .await - .unwrap(); - - check_metrics_contains( - &metrics, - r#"apollo_router_cache_redis_clients{otel_scope_name="apollo/router"} 3"#, - ); -} - -#[track_caller] -fn check_metrics_contains(metrics: &str, text: &str) { - assert!( - metrics.contains(text), - "'{text}' not detected in metrics\n{metrics}" - ); + router + .assert_metrics_contains( + r#"apollo_router_cache_redis_clients{otel_scope_name="apollo/router"} 3"#, + None, + ) + .await; } #[tokio::test(flavor = "multi_thread")] From b2c6033389e8f0fa40bcbe98f8bfbb16293b819a Mon Sep 17 00:00:00 2001 From: Benjamin <5719034+bnjjj@users.noreply.github.com> Date: Wed, 17 Dec 2025 09:48:12 +0100 Subject: [PATCH 5/6] fix tests Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --- apollo-router/src/plugins/response_cache/plugin.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apollo-router/src/plugins/response_cache/plugin.rs b/apollo-router/src/plugins/response_cache/plugin.rs index d2b2090159..144314583d 100644 --- a/apollo-router/src/plugins/response_cache/plugin.rs +++ b/apollo-router/src/plugins/response_cache/plugin.rs @@ -311,8 +311,10 @@ impl PluginPrivate for ResponseCache { match subgraph_config.redis.clone() { Some(config) => { // We need to do this because the subgraph config automatically clones from the `all` config during deserialization. - // We don't want to create a new connection pool if the subgraph just inherits from the `all` config. - if Some(&config) != init.config.subgraph.all.redis.as_ref() { + // We don't want to create a new connection pool if the subgraph just inherits from the `all` config (only if all is enabled). + if Some(&config) != init.config.subgraph.all.redis.as_ref() + || storage_interface.all.is_none() + { let storage = Arc::new(OnceLock::new()); storage_interface .subgraphs @@ -3043,7 +3045,7 @@ mod tests { "Redis storage is set globally" ); } - if subgraph_enabled { + if subgraph_enabled && !all_enabled { assert_eq!( response_cache.storage.subgraphs.len(), 1, From 642b1d355b6de2979b2ec09cf71568a6cc3de3e7 Mon Sep 17 00:00:00 2001 From: Benjamin <5719034+bnjjj@users.noreply.github.com> Date: Wed, 17 Dec 2025 10:23:10 +0100 Subject: [PATCH 6/6] fix tests Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --- apollo-router/tests/integration/allowed_features.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apollo-router/tests/integration/allowed_features.rs b/apollo-router/tests/integration/allowed_features.rs index 4f82cda0b3..9b33d8b93d 100644 --- a/apollo-router/tests/integration/allowed_features.rs +++ b/apollo-router/tests/integration/allowed_features.rs @@ -649,6 +649,9 @@ async fn feature_violation_when_license_past_warn_at_but_not_expired_allowed_fea ) .build() .await; + router.replace_config_string("http://localhost:{{PRODUCTS_PORT}}", "5000"); + router.replace_config_string("http://localhost:{{ACCOUNTS_PORT}}", "5001"); + router.replace_config_string("http://localhost:{{COPROCESSOR_PORT}}", "5002"); router.start().await; router