remove CacheKey::default impl

Anyone using caching must must now implement cache_key_callback themselves. This
forces an explicit decision about what belongs in the cache key for anyone using
thet trait for caching rather than providing an unsafe default that does not
support web standards.

Author: gumpt

.bleep

@@ -1 +1 @@
-118e3d5e0e12ebf28edd7310d88eaee183f95861
+41876b9acd525b8c3a8718ace49d3407b4ec6fb9

pingora-cache/src/key.rs

@@ -14,8 +14,6 @@
 
 //! Cache key
 
-use super::*;
-
 use blake2::{Blake2b, Digest};
 use http::Extensions;
 use serde::{Deserialize, Serialize};
@@ -214,18 +212,6 @@ impl CacheKey {
         hasher
     }
 
-    /// Create a default [CacheKey] from a request, which just takes its URI as the primary key.
-    pub fn default(req_header: &ReqHeader) -> Self {
-        CacheKey {
-            namespace: Vec::new(),
-            primary: format!("{}", req_header.uri).into_bytes(),
-            primary_bin_override: None,
-            variance: None,
-            user_tag: "".into(),
-            extensions: Extensions::new(),
-        }
-    }
-
     /// Create a new [CacheKey] from the given namespace, primary, and user_tag input.
     ///
     /// Both `namespace` and `primary` will be used for the primary hash

pingora-proxy/src/proxy_trait.rs

@@ -137,14 +137,24 @@ pub trait ProxyHttp {
         Ok(())
     }
 
-    /// This callback generates the cache key
+    /// This callback generates the cache key.
     ///
-    /// This callback is called only when cache is enabled for this request
+    /// This callback is called only when cache is enabled for this request.
     ///
-    /// By default this callback returns a default cache key generated from the request.
-    fn cache_key_callback(&self, session: &Session, _ctx: &mut Self::CTX) -> Result<CacheKey> {
-        let req_header = session.req_header();
-        Ok(CacheKey::default(req_header))
+    /// There is no sensible default cache key for all proxy applications. The
+    /// correct key depends on which request properties affect upstream responses
+    /// (e.g. `Vary` headers, custom request filters that modify the origin host).
+    /// Getting this wrong leads to cache poisoning.
+    ///
+    /// See `pingora-proxy/tests/utils/server_utils.rs` for a minimal (not
+    /// production-ready) reference implementation.
+    ///
+    /// # Panics
+    ///
+    /// The default implementation panics. You **must** override this method when
+    /// caching is enabled.
+    fn cache_key_callback(&self, _session: &Session, _ctx: &mut Self::CTX) -> Result<CacheKey> {
+        unimplemented!("cache_key_callback must be implemented when caching is enabled")
     }
 
     /// This callback is invoked when a cacheable response is ready to be admitted to cache.

pingora-proxy/tests/utils/server_utils.rs

@@ -26,8 +26,8 @@ use pingora_cache::key::HashBinary;
 use pingora_cache::lock::CacheKeyLockImpl;
 use pingora_cache::{
     eviction::simple_lru::Manager, filters::resp_cacheable, lock::CacheLock, predictor::Predictor,
-    set_compression_dict_path, CacheMeta, CacheMetaDefaults, CachePhase, MemCache, NoCacheReason,
-    RespCacheable,
+    set_compression_dict_path, CacheKey, CacheMeta, CacheMetaDefaults, CachePhase, MemCache,
+    NoCacheReason, RespCacheable,
 };
 use pingora_cache::{
     CacheOptionOverrides, ForcedFreshness, HitHandler, PurgeType, VarianceBuilder,
@@ -489,6 +489,35 @@ impl ProxyHttp for ExampleProxyCache {
         Ok(())
     }
 
+    /// Reference `cache_key_callback` implementation for integration tests.
+    ///
+    /// Builds the primary key as `{host}{path_and_query}` from the request.
+    /// This is **not production ready**: it does not account for `Vary`, custom
+    /// request filters, or scheme differences. See the rustdoc on
+    /// [`ProxyHttp::cache_key_callback`] for details.
+    fn cache_key_callback(&self, session: &Session, _ctx: &mut Self::CTX) -> Result<CacheKey> {
+        let req_header = session.req_header();
+
+        let host = req_header
+            .headers
+            .get(http::header::HOST)
+            .and_then(|v| v.to_str().ok())
+            .or_else(|| req_header.uri.authority().map(|a| a.as_str()))
+            .unwrap_or("");
+
+        let path_and_query = req_header
+            .uri
+            .path_and_query()
+            .map(|pq| pq.as_str())
+            .unwrap_or("/");
+
+        Ok(CacheKey::new(
+            String::new(),
+            format!("{host}{path_and_query}"),
+            String::new(),
+        ))
+    }
+
     async fn cache_hit_filter(
         &self,
         session: &mut Session,