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 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..144314583d 100644 --- a/apollo-router/src/plugins/response_cache/plugin.rs +++ b/apollo-router/src/plugins/response_cache/plugin.rs @@ -310,12 +310,22 @@ 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 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 (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 + .insert(subgraph.clone(), storage.clone()); + connect_or_spawn_reconnection_task( + config, + storage, + drop_tx.subscribe(), + ) .await?; + } } None => { if storage_interface.all.is_none() { @@ -3035,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, 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/allowed_features.rs b/apollo-router/tests/integration/allowed_features.rs index 88c3f52e04..2ead66ccdd 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.replace_config_string("http://localhost:{{PRODUCTS_PORT}}", "localhost:4001"); router.replace_config_string("http://localhost:{{ACCOUNTS_PORT}}", "localhost:4002"); diff --git a/apollo-router/tests/integration/response_cache.rs b/apollo-router/tests/integration/response_cache.rs index b586b9ce78..a7b6574967 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,43 @@ 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" + } + } + } + }, + "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", + "private_id": "private_id" + }, + "subgraphs": { + "user": { + "enabled": true, + "private_id": "user" + } + } + } + }, + }) +} + fn failure_config() -> Value { json!({ "include_subgraph_errors": { @@ -285,6 +323,28 @@ 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; + + router + .assert_metrics_contains( + r#"apollo_router_cache_redis_clients{otel_scope_name="apollo/router"} 3"#, + None, + ) + .await; +} + #[tokio::test(flavor = "multi_thread")] async fn basic_cache_skips_subgraph_request() { if !graph_os_enabled() {