Skip to content

Conversation

@JakeChampion
Copy link
Contributor

@JakeChampion JakeChampion commented Apr 18, 2025

This pull request adds full support for the zstd (Zstandard) compression algorithm throughout Apache Traffic Server, including build system integration, compression plugin support, Accept-Encoding header normalization, and test coverage.

Key Features

Build system and dependencies:

  • Add CMake support for finding zstd library with new Findzstd.cmake
  • Update Docker build files to include libzstd-dev package
  • Add TS_HAS_ZSTD feature flag for conditional compilation

Core compression support:

  • Extend compress plugin to support zstd compression alongside gzip and brotli
  • Add zstd stream handling structures and functions
  • Update compression configuration to include zstd in supported algorithms list

Accept-Encoding header normalization:

  • Extend proxy.config.http.normalize_ae configuration to support values 4 and 5 for zstd normalization
  • Update HTTP transaction cache matching to handle zstd encoding

API and infrastructure:

  • Add TS_HTTP_VALUE_ZSTD and TS_HTTP_LEN_ZSTD constants
  • Update MIME field handling to recognize zstd encoding
  • Add zstd support to traffic_layout feature detection

Improved cache matching:

  • Fix Accept-Encoding quality calculation logic in HTTP transaction cache
  • Replace multiplicative quality combining with minimum quality selection
  • Remove problematic gzip-specific fallback logic that caused cache inconsistencies
  • Ensure proper cache behavior for all compression algorithms

Test Coverage

  • Comprehensive compress plugin tests covering zstd compression scenarios
  • Accept-Encoding normalization tests for all zstd combinations
  • Updated golden files to include zstd compression test results
  • New compress3.config for zstd-specific plugin configuration
  • Test all combinations of zstd, br, and gzip in various scenarios
  • Cache matching tests demonstrate correct behavior without workarounds

Standards Compliance

The implementation follows RFC 8878 standards for zstd compression and maintains backward compatibility with existing gzip and brotli compression functionality. All tests pass and the feature is properly integrated with existing caching and content negotiation mechanisms.

Configuration

New normalization modes:

  • proxy.config.http.normalize_ae = 4: Prioritize zstd, fallback to br then gzip
  • proxy.config.http.normalize_ae = 5: Support all combinations of zstd, br, and gzip

Benefits

  • Improved compression ratios compared to gzip
  • Better performance than brotli for compression speed
  • Reduced bandwidth usage and faster page loads
  • Seamless integration with existing ATS infrastructure

@JakeChampion JakeChampion marked this pull request as ready for review April 18, 2025 18:34
@masaori335 masaori335 self-requested a review April 18, 2025 23:25
@masaori335 masaori335 added the compress compress plugin label Apr 18, 2025
@masaori335 masaori335 added this to the 10.2.0 milestone Apr 18, 2025
@masaori335
Copy link
Contributor

[approve ci]

@JakeChampion
Copy link
Contributor Author

@masaori335 do you know which files I need to modify to install zstd on the osx used in the Jenkins job?

@masaori335
Copy link
Contributor

We need to ask @ezelkow1 to install the package in the osx env, I guess.

However, this PR has #if HAVE_ZSTD_F check, why it's failing?

@cmcfarlen cmcfarlen self-requested a review April 21, 2025 22:23
@JakeChampion JakeChampion force-pushed the zstd branch 2 times, most recently from 0167bf5 to 74cd8b5 Compare April 22, 2025 07:51
@JakeChampion
Copy link
Contributor Author

We need to ask @ezelkow1 to install the package in the osx env, I guess.

However, this PR has #if HAVE_ZSTD_F check, why it's failing?

thank you, _F was a mistake, it was meant to be _H

@JakeChampion JakeChampion force-pushed the zstd branch 4 times, most recently from 308032c to c598282 Compare April 22, 2025 10:12
@JakeChampion
Copy link
Contributor Author

Looks like all the FreeBSD machines for CI are offline

Copy link
Contributor

@shukitchan shukitchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really appreciate it if you can please add some documentation to the changes.

Thanks.

@cmcfarlen
Copy link
Contributor

Hi! This doesn't compile for me. I added a line to include/tscore/ink_config.cmake.in so HAVE_ZSTD_H would be truthy for the CPP macros. This lit up that code, but then there were several compiler errors:

/Users/cmcfarlen/projects/oss/trafficserver/plugins/compress/compress.cc:203:9: error: no member named 'zstd_ctx' in 'Data'; did you mean 'zstd_cctx'?
  203 |   data->zstd_ctx = nullptr;
      |         ^~~~~~~~
      |         zstd_cctx
/Users/cmcfarlen/projects/oss/trafficserver/plugins/compress/misc.h:93:14: note: 'zstd_cctx' declared here
   93 |   ZSTD_CCtx *zstd_cctx;
      |              ^
/Users/cmcfarlen/projects/oss/trafficserver/plugins/compress/compress.cc:206:11: error: no member named 'zstd_ctx' in 'Data'; did you mean 'zstd_cctx'?
  206 |     data->zstd_ctx = ZSTD_createCCtx();
      |           ^~~~~~~~
      |           zstd_cctx
/Users/cmcfarlen/projects/oss/trafficserver/plugins/compress/misc.h:93:14: note: 'zstd_cctx' declared here
   93 |   ZSTD_CCtx *zstd_cctx;
      |              ^
/Users/cmcfarlen/projects/oss/trafficserver/plugins/compress/compress.cc:207:16: error: no member named 'zstd_ctx' in 'Data'; did you mean 'zstd_cctx'?
  207 |     if (!data->zstd_ctx) {
      |                ^~~~~~~~
      |                zstd_cctx
/Users/cmcfarlen/projects/oss/trafficserver/plugins/compress/misc.h:93:14: note: 'zstd_cctx' declared here
   93 |   ZSTD_CCtx *zstd_cctx;
      |              ^

Please include this patch in your PR to enable the ZSTD code. Thanks!

diff --git a/include/tscore/ink_config.h.cmake.in b/include/tscore/ink_config.h.cmake.in
index 634ed94c3..7da0771fd 100644
--- a/include/tscore/ink_config.h.cmake.in
+++ b/include/tscore/ink_config.h.cmake.in
@@ -184,3 +184,5 @@ const int DEFAULT_STACKSIZE = @DEFAULT_STACK_SIZE@;
 #cmakedefine YAMLCPP_LIB_VERSION "@YAMLCPP_LIB_VERSION@"

 #cmakedefine01 TS_HAS_CRIPTS
+
+#cmakedefine HAVE_ZSTD_H 1

Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please patch ink_config.h and resolve compiler issues.

@JakeChampion JakeChampion force-pushed the zstd branch 3 times, most recently from 4b8603e to 72c0dbf Compare May 28, 2025 19:48
bryancall
bryancall previously approved these changes Nov 6, 2025
Copy link
Contributor

@bryancall bryancall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary - LGTM ✅

I've completed a thorough code review and functional testing of the zstd compression implementation.

Code Review

Implementation Quality:

  • Clean separation of concerns with zstd_compress.cc/h following the existing compression plugin architecture
  • Proper resource management with context creation/destruction lifecycle
  • Good error handling with input validation (null checks, size validation, progress detection)
  • Uses modern zstd streaming API (ZSTD_compressStream2) with appropriate flush handling
  • Checksum enabled for data integrity verification
  • Configurable compression levels per host (1-22, default 12)

API & Integration:

  • Proper addition of TS_HTTP_VALUE_ZSTD and TS_HTTP_LEN_ZSTD constants
  • Extends normalize_ae configuration appropriately (values 4 and 5)
  • Consistent with existing gzip/brotli patterns

Build System:

  • The CMake integration works well. Note: I needed to add a pkg-config fallback for Fedora/RHEL systems that don't ship zstd CMake configs, which you may want to consider for broader distribution compatibility.

Testing Results

Built and tested with dev-asan preset:

  • ✅ Zstd compression confirmed working (98.6% reduction on 5KB text content)
  • ✅ Content-Encoding header correctly set
  • ✅ Compressed data verified as valid Zstandard format (v0.8+)
  • ✅ Decompressed content matches original exactly
  • ✅ Cache disabled mode works correctly
  • ✅ No ASAN errors detected

Test Configuration:

  • Origin serving 5,100 bytes of compressible text
  • Compressed to 71 bytes on wire
  • Level 12 compression
  • Cache disabled for testing

Observations

  1. The Accept-Encoding cache quality calculation fix (commit facf384) is a nice improvement that fixes the multiplicative quality issue
  2. Documentation updates are comprehensive and include good examples
  3. All CI checks passing

Recommendation: This is a solid implementation that adds valuable functionality while maintaining code quality and backward compatibility. Ready to merge.

@bryancall
Copy link
Contributor

Fedora 43 Build Support

For Fedora 43 (and potentially other RHEL-based systems), the zstd package doesn't ship CMake config files, only pkg-config files. Here's a suggested enhancement to add pkg-config fallback support:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 692b11985a..0e1d26e08f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -365,7 +365,20 @@ if(zstd_FOUND)
     endif()
   endif()
 else()
-  set(HAVE_ZSTD_H FALSE)
+  # Try pkg-config as fallback
+  find_package(PkgConfig QUIET)
+  if(PKG_CONFIG_FOUND)
+    pkg_check_modules(ZSTD QUIET IMPORTED_TARGET libzstd)
+    if(ZSTD_FOUND)
+      add_library(zstd::zstd ALIAS PkgConfig::ZSTD)
+      set(HAVE_ZSTD_H TRUE)
+      message(STATUS "Found zstd via pkg-config: ${ZSTD_VERSION}")
+    else()
+      set(HAVE_ZSTD_H FALSE)
+    endif()
+  else()
+    set(HAVE_ZSTD_H FALSE)
+  endif()
 endif()
 
 # ncurses is used in traffic_top

This allows the build to find zstd on distributions that only provide libzstd.pc without CMake package config files. Tested successfully on Fedora 43 with libzstd-devel 1.5.7.

@JakeChampion
Copy link
Contributor Author

Fedora 43 Build Support

For Fedora 43 (and potentially other RHEL-based systems), the zstd package doesn't ship CMake config files, only pkg-config files. Here's a suggested enhancement to add pkg-config fallback support:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 692b11985a..0e1d26e08f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -365,7 +365,20 @@ if(zstd_FOUND)
     endif()
   endif()
 else()
-  set(HAVE_ZSTD_H FALSE)
+  # Try pkg-config as fallback
+  find_package(PkgConfig QUIET)
+  if(PKG_CONFIG_FOUND)
+    pkg_check_modules(ZSTD QUIET IMPORTED_TARGET libzstd)
+    if(ZSTD_FOUND)
+      add_library(zstd::zstd ALIAS PkgConfig::ZSTD)
+      set(HAVE_ZSTD_H TRUE)
+      message(STATUS "Found zstd via pkg-config: ${ZSTD_VERSION}")
+    else()
+      set(HAVE_ZSTD_H FALSE)
+    endif()
+  else()
+    set(HAVE_ZSTD_H FALSE)
+  endif()
 endif()
 
 # ncurses is used in traffic_top

This allows the build to find zstd on distributions that only provide libzstd.pc without CMake package config files. Tested successfully on Fedora 43 with libzstd-devel 1.5.7.

@bryancall could this be done in a follow up or would we want this as part of the current pull request?

@bryancall
Copy link
Contributor

bryancall commented Nov 7, 2025

@JakeChampion It can be done in a followup PR. I am thinking about merging this in today.

bneradt
bneradt previously approved these changes Nov 8, 2025
cmcfarlen
cmcfarlen previously approved these changes Nov 8, 2025
Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is looking good. Thanks for hanging in there!

@JakeChampion JakeChampion dismissed stale reviews from cmcfarlen, bneradt, and bryancall via 6d440f1 November 8, 2025 08:08
@JakeChampion
Copy link
Contributor Author

Apologies for the final commit here which voided the reviews, I saw a code refactor I previously did and realised doesn't need to happen at all so I reverted it, I'll not touch the pr again now

cmcfarlen
cmcfarlen previously approved these changes Nov 10, 2025
@zwoop zwoop self-requested a review November 10, 2025 18:50
// Disable Vary mismatch checking for Accept-Encoding. This is only safe to
// set if you are promising to fix any Accept-Encoding/Content-Encoding mismatches.
if (http_config_params->get_ignore_accept_encoding_mismatch() &&
// Only suppress variability checks when the operator explicitly set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a bug that's been here for a long time. @cmcfarlen @ezelkow1 I wonder if we should consider this for back ports to 10.x and even 9.2 ? Only this portion though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a pr open for this specific change which we can backport
#12618

}

if (strncasecmp(value, "br", sizeof("br") - 1) == 0) {
info("Accept-Encoding value [%.*s]", len, value);
Copy link
Contributor

@zwoop zwoop Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit picking (and I know it's the same), but this feels more like a debug() than an info(). I'm honestly not sure why this plugin has debug / info / warning all going to the same Dbg() tag.

@bryancall
Copy link
Contributor

@JakeChampion looks like there is now a conflict and should look at the comment from @zwoop above about info vs debug

@JakeChampion
Copy link
Contributor Author

@JakeChampion looks like there is now a conflict and should look at the comment from @zwoop above about info vs debug

I've fixed conflicts and changed the call from info to debug now

@JakeChampion
Copy link
Contributor Author

@bryancall I forgot to tag you, apologies - I fixed all the conflicts and the tests pass once again now

Copy link
Contributor

@bryancall bryancall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a solid, well-implemented feature that adds comprehensive zstd (Zstandard) compression support to Apache Traffic Server. The implementation follows existing patterns for gzip and brotli, has good test coverage, and all CI checks are passing.

Strengths:

  • Clean implementation with proper namespace separation
  • Good CMake integration handling multiple zstd target names
  • Comprehensive test coverage including normalization modes and cache matching
  • Well-documented with examples and RFC 8878 compliance
  • Configurable compression levels with validation

Minor recommendations for follow-up PRs:

  • Review the cache matching logic revert (commit 6d440f1) to ensure gzip-specific fallback behavior is appropriate across all compression algorithms
  • Consider documenting performance implications of normalize_ae modes 4 and 5
  • May want to evaluate the default compression level (currently 12) for typical use cases

The recommendations above can be addressed in follow-up PRs if needed. Approving for merge.

@bryancall bryancall dismissed shukitchan’s stale review December 2, 2025 21:09

Documentation was added to the PR

@bryancall bryancall merged commit c183dd1 into apache:master Dec 2, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this to For v10.1.1 in ATS v10.1.x Dec 2, 2025
@bryancall bryancall removed this from ATS v10.1.x Dec 3, 2025
@bryancall
Copy link
Contributor

I changed this feature to only go into ATS 10.2.0. We are going to branch in January and release in Q1. We have limited time to test this feature and doing it in one version would be significantly easier for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants