fix(response_cache): don't duplicate redis connection if we specify custom config for a subgraph

.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

apollo-router/src/configuration/mod.rs

@@ -1291,7 +1291,7 @@ pub(crate) fn load_key(data: &str) -> io::Result<PrivateKeyDer<'static>> {
 }
 
 /// 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

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,

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 {

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

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