adam review: changelog, enum config, shorter comment

Author: david-crespo

CHANGELOG.adoc

@@ -16,6 +16,7 @@
 https://github.com/oxidecomputer/dropshot/compare/v0.16.4\...HEAD[Full list of commits]
 
 * https://github.com/oxidecomputer/dropshot/pull/1475[#1475] Added `ClientSpecifiesVersionInHeader::on_missing` to provide a default version when the header is missing, intended for use when you're not in control of all clients
+* https://github.com/oxidecomputer/dropshot/pull/1448[#1448] Added support for gzipping responses when clients request it through the `Accept-Encoding` header. Compression is off by default: existing consumers won't see a behavior change unless they opt in with `compression: CompressionConfig::Gzip`.
 
 == 0.16.4 (released 2025-09-04)
 

dropshot/src/config.rs

@@ -6,6 +6,17 @@ use serde::Serialize;
 use std::net::SocketAddr;
 use std::path::PathBuf;
 
+/// Configuration for response compression
+#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Serialize)]
+#[serde(rename_all = "lowercase")]
+pub enum CompressionConfig {
+    /// Compression is disabled
+    #[default]
+    None,
+    /// Gzip compression is enabled
+    Gzip,
+}
+
 /// Raw [`rustls::ServerConfig`] TLS configuration for use with
 /// [`ConfigTls::Dynamic`]
 pub type RawTlsConfig = rustls::ServerConfig;
@@ -63,10 +74,9 @@ pub struct ConfigDropshot {
     /// is made to deal with headers that appear multiple times in a single
     /// request.
     pub log_headers: Vec<String>,
-    /// Whether to enable gzip compression for responses when response contents
-    /// allow it and clients ask for it through the Accept-Encoding header.
-    /// Defaults to true.
-    pub compression: bool,
+    /// Whether to enable compression for responses (when response contents
+    /// allow it and clients ask for it through the Accept-Encoding header).
+    pub compression: CompressionConfig,
 }
 
 /// Enum specifying options for how a Dropshot server should run its handler
@@ -123,7 +133,7 @@ impl Default for ConfigDropshot {
             default_request_body_max_bytes: 1024,
             default_handler_task_mode: HandlerTaskMode::Detached,
             log_headers: Default::default(),
-            compression: false,
+            compression: CompressionConfig::default(),
         }
     }
 }
@@ -142,7 +152,7 @@ struct DeserializedConfigDropshot {
     request_body_max_bytes: Option<InvalidConfig>,
     default_handler_task_mode: HandlerTaskMode,
     log_headers: Vec<String>,
-    compression: bool,
+    compression: CompressionConfig,
 }
 
 impl From<DeserializedConfigDropshot> for ConfigDropshot {

dropshot/src/lib.rs

@@ -907,6 +907,7 @@ pub use api_description::TagDetails;
 pub use api_description::TagExternalDocs;
 pub use body::Body;
 pub use compression::NoCompression;
+pub use config::CompressionConfig;
 pub use config::ConfigDropshot;
 pub use config::ConfigTls;
 pub use config::HandlerTaskMode;

dropshot/src/server.rs

@@ -7,7 +7,7 @@ use super::compression::add_vary_header;
 use super::compression::apply_gzip_compression;
 use super::compression::is_compressible_content_type;
 use super::compression::should_compress_response;
-use super::config::{ConfigDropshot, ConfigTls};
+use super::config::{CompressionConfig, ConfigDropshot, ConfigTls};
 #[cfg(feature = "usdt-probes")]
 use super::dtrace::probes;
 use super::handler::HandlerError;
@@ -109,9 +109,8 @@ pub struct ServerConfig {
     /// is made to deal with headers that appear multiple times in a single
     /// request.
     pub log_headers: Vec<String>,
-    /// Whether to enable gzip compression for responses when response contents
-    /// allow it and clients ask for it through the Accept-Encoding header.
-    pub compression: bool,
+    /// Configuration for response compression.
+    pub compression: CompressionConfig,
 }
 
 /// See [`ServerBuilder`] instead.
@@ -991,19 +990,16 @@ async fn http_request_handle<C: ServerContext>(
         }
     };
 
-    if server.config.compression
+    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 how to handle the possibility that
-        // responses could be compressed or not.
-        //
-        // This 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.
+        // 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(

dropshot/src/websocket.rs

@@ -349,7 +349,7 @@ impl tokio::io::AsyncWrite for WebsocketConnectionRaw {
 #[cfg(test)]
 mod tests {
     use crate::body::Body;
-    use crate::config::HandlerTaskMode;
+    use crate::config::{CompressionConfig, HandlerTaskMode};
     use crate::router::HttpRouter;
     use crate::server::{DropshotState, ServerConfig};
     use crate::{
@@ -385,7 +385,7 @@ mod tests {
                     default_handler_task_mode:
                         HandlerTaskMode::CancelOnDisconnect,
                     log_headers: Default::default(),
-                    compression: true,
+                    compression: CompressionConfig::Gzip,
                 },
                 router: HttpRouter::new(),
                 log: log.clone(),

dropshot/tests/integration-tests/config.rs

@@ -5,8 +5,8 @@
 use dropshot::test_util::read_config;
 use dropshot::Body;
 use dropshot::{
-    ConfigDropshot, ConfigTls, HandlerTaskMode, HttpError, HttpResponseOk,
-    RequestContext,
+    CompressionConfig, ConfigDropshot, ConfigTls, HandlerTaskMode, HttpError,
+    HttpResponseOk, RequestContext,
 };
 use dropshot::{HttpServer, ServerBuilder};
 use slog::o;
@@ -66,7 +66,7 @@ fn test_valid_config_all_settings() {
             default_request_body_max_bytes: 1048576,
             default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect,
             log_headers: vec!["X-Forwarded-For".to_string()],
-            compression: false,
+            compression: CompressionConfig::default(),
         },
     );
 }
@@ -182,7 +182,7 @@ fn make_config(
         default_request_body_max_bytes: 1024,
         default_handler_task_mode,
         log_headers: Default::default(),
-        compression: true,
+        compression: CompressionConfig::Gzip,
     }
 }
 

dropshot/tests/integration-tests/gzip.rs

@@ -25,8 +25,10 @@ fn test_setup(
     test_name: &str,
     api: ApiDescription<usize>,
 ) -> dropshot::test_util::TestContext<usize> {
-    let config =
-        dropshot::ConfigDropshot { compression: true, ..Default::default() };
+    let config = dropshot::ConfigDropshot {
+        compression: dropshot::CompressionConfig::Gzip,
+        ..Default::default()
+    };
     let logctx = create_log_context(test_name);
     let log = logctx.log.new(slog::o!());
     dropshot::test_util::TestContext::new(
@@ -667,9 +669,11 @@ async fn test_no_compression_for_304_not_modified() {
 
 #[tokio::test]
 async fn test_compression_config_disabled() {
-    // Test that compression is disabled when config.compression = false (default)
-    let config =
-        dropshot::ConfigDropshot { compression: false, ..Default::default() };
+    // Test that compression is disabled when compression = "none" (default)
+    let config = dropshot::ConfigDropshot {
+        compression: dropshot::CompressionConfig::None,
+        ..Default::default()
+    };
     let logctx = create_log_context("compression_config_disabled");
     let log = logctx.log.new(slog::o!());
     let testctx = dropshot::test_util::TestContext::new(
@@ -695,9 +699,11 @@ async fn test_compression_config_disabled() {
 
 #[tokio::test]
 async fn test_compression_config_enabled() {
-    // Test that compression works when config.compression = true
-    let config =
-        dropshot::ConfigDropshot { compression: true, ..Default::default() };
+    // Test that compression works when compression = "gzip"
+    let config = dropshot::ConfigDropshot {
+        compression: dropshot::CompressionConfig::Gzip,
+        ..Default::default()
+    };
     let logctx = create_log_context("compression_config_enabled");
     let log = logctx.log.new(slog::o!());
     let testctx = dropshot::test_util::TestContext::new(
@@ -713,7 +719,7 @@ async fn test_compression_config_enabled() {
     let uri = client.url("/large-response");
     let mut response = get_gzip_response(client, &uri).await;
 
-    // Should be compressed since config.compression = true
+    // Should be compressed since compression = "gzip"
     assert_gzip_encoded(&response);
 
     // Verify the response can be decompressed

dropshot/tests/integration-tests/tls.rs

@@ -4,8 +4,8 @@
 //! mode, including certificate loading and supported modes.
 
 use dropshot::{
-    ConfigDropshot, ConfigTls, HandlerTaskMode, HttpResponseOk, HttpServer,
-    ServerBuilder,
+    CompressionConfig, ConfigDropshot, ConfigTls, HandlerTaskMode,
+    HttpResponseOk, HttpServer, ServerBuilder,
 };
 use slog::{o, Logger};
 use std::convert::TryFrom;
@@ -116,7 +116,7 @@ fn make_server(
         default_request_body_max_bytes: 1024,
         default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect,
         log_headers: Default::default(),
-        compression: true,
+        compression: CompressionConfig::Gzip,
     };
     let config_tls = Some(ConfigTls::AsFile {
         cert_file: cert_file.to_path_buf(),
@@ -431,7 +431,7 @@ async fn test_server_is_https() {
         default_request_body_max_bytes: 1024,
         default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect,
         log_headers: Default::default(),
-        compression: true,
+        compression: CompressionConfig::Gzip,
     };
     let config_tls = Some(ConfigTls::AsFile {
         cert_file: cert_file.path().to_path_buf(),