server: omit Vary: Accept-Encoding when NoCompression extension is set

dropshot/src/server.rs

@@ -4,6 +4,7 @@
 use super::ProbeRegistration;
 use super::api_description::ApiDescription;
 use super::body::Body;
+use super::compression::NoCompression;
 use super::compression::add_vary_header;
 use super::compression::apply_gzip_compression;
 use super::compression::is_compressible_content_type;
@@ -993,23 +994,25 @@ async fn http_request_handle<C: ServerContext>(
     if matches!(server.config.compression, CompressionConfig::Gzip)
         && is_compressible_content_type(response.headers())
     {
-        // Add Vary: Accept-Encoding header for all compressible content
-        // types. This needs to be there even if the response ends up not being
-        // compressed because it tells caches (like browsers and CDNs) that the
-        // response content depends on the value of the Accept-Encoding header.
-        // Without this, a cache might mistakenly serve a compressed response to
-        // a client that cannot decompress it, or serve an uncompressed response
-        // to a client that could have benefited from compression.
-        add_vary_header(response.headers_mut());
-
         if should_compress_response(
             &method,
             &request_headers,
             response.status(),
             response.headers(),
             response.extensions(),
         ) {
+            // Add Vary: Accept-Encoding and compress. The Vary header is added
+            // here so caches know they must not serve a compressed response to
+            // a client that didn't accept it.
+            add_vary_header(response.headers_mut());
             response = apply_gzip_compression(response);
+        } else if response.extensions().get::<NoCompression>().is_none() {
+            // Add Vary: Accept-Encoding for compressible responses that are
+            // skipped this time (e.g. body too small, wrong method, partial
+            // content) but that could be compressed for a different request.
+            // Omit Vary when NoCompression is set because that response will
+            // never be compressed regardless of Accept-Encoding.
+            add_vary_header(response.headers_mut());
         }
     }
 

dropshot/tests/integration-tests/gzip.rs

@@ -524,6 +524,15 @@ async fn test_compression_disabled_with_extension() {
 
     assert_eq!(response.headers().get(header::CONTENT_ENCODING), None);
 
+    // A NoCompression response will never be compressed regardless of
+    // Accept-Encoding, so it must not carry Vary: Accept-Encoding (which would
+    // cause caches to split entries on a header the body doesn't depend on).
+    assert_eq!(
+        response.headers().get(header::VARY),
+        None,
+        "NoCompression response should omit Vary: Accept-Encoding"
+    );
+
     get_response_bytes(&mut response).await;
 
     testctx.teardown().await;
@@ -540,6 +549,20 @@ async fn test_no_compression_below_size_threshold() {
 
     assert_eq!(response.headers().get(header::CONTENT_ENCODING), None);
 
+    // Unlike the NoCompression case, a compressible response skipped only
+    // because it's below the size threshold must still carry Vary: a different
+    // request to the same resource could be compressed.
+    let vary = response
+        .headers()
+        .get(header::VARY)
+        .expect("compressible-but-skipped response should keep Vary")
+        .to_str()
+        .unwrap();
+    assert!(
+        vary.to_lowercase().contains("accept-encoding"),
+        "Vary should include Accept-Encoding, got: {vary}"
+    );
+
     testctx.teardown().await;
 }