Add Zstandard compression support and update tests

[JakeChampion]

• Implemented Zstandard (zstd) compression in the compress plugin.
• Updated CMake configuration to include zstd.
• Enhanced the normalization of the Accept-Encoding header to support zstd.
• Added tests for zstd compression functionality.

[masaori335]

[approve ci]

[JakeChampion]

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

[masaori335]

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?

[JakeChampion]

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]

Looks like all the FreeBSD machines for CI are offline

[cmcfarlen]

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

[bryancall]

[approve ci autest]

[JakeChampion]

@bryancall i think this is ready to review now 🙏

[JakeChampion]

@cmcfarlen do you know if there's anything I can do to help get this reviewed? I'm happy to join a mailing list or slack, discord, etc - my work uses ATS quite a bit and I'm eager to ensure we contribute back to the project where we can

[cmcfarlen]

I think the code is fine at this point, but I have a few concerns:

• I'm not convinced autests will pass if the library is not available.
• There is some code (transact headers, xpack) that unconditionally expect zstd
• Maybe we should just require zstd for the build (probably not, and would have to be done for ATS 11)

[cmcfarlen]

building without zstd enabled does compile, but the compress autest doesn't run:

% ./autest.sh -f compress
Running Test compress: Skipped
Warning: Skipping test compress because:
 ATS feature not enabled: TS_HAS_ZSTD

Generating Report: --------------
1 Tests were skipped:
--------------------------------------------------------------------------------
 Test "compress" Skipped
    File: compress.test.py
    Directory: /Users/cmcfarlen/projects/oss/trafficserver/tests/gold_tests/pluginTest/compress
  Reason: ATS feature not enabled: TS_HAS_ZSTD

Total of 1 test
  Unknown: 0
  Exception: 0
  Failed: 0
  Warning: 0
  Skipped: 1
  Passed: 0

[bryancall]

Comprehensive Testing Results for ZSTD Compression Support

I've completed comprehensive testing of PR #12201 across three test phases. All tests PASSED ✅

Test Environment

• Build: dev-asan preset
• Test file: 4701 bytes HTML
• Configuration: proxy.config.http.normalize_ae: 0 (required for Brotli/ZSTD)

---

📊 Test Results Summary

Test 1: Baseline (Master Branch)

Established baseline functionality with existing compression algorithms:

• ✅ GZIP: 4701 → 91 bytes (98.1% reduction)
• ✅ Brotli: 4701 → 53 bytes (98.9% reduction)

Test 2: PR #12201 WITHOUT ZSTD Libraries

Verified backward compatibility when ZSTD libraries are not available:

• ✅ GZIP: 4701 → 91 bytes (98.1% reduction)
• ✅ Brotli: 4701 → 53 bytes (98.9% reduction)
• ✅ ZSTD: 4701 bytes (uncompressed - graceful fallback)

Result: No regressions. Existing compression algorithms continue to work when ZSTD is unavailable.

Test 3: PR #12201 WITH ZSTD Libraries

Verified new ZSTD compression functionality:

• ✅ GZIP: 4701 → 91 bytes (98.1% reduction)
• ✅ Brotli: 4701 → 53 bytes (98.9% reduction)
• ✅ ZSTD: 4701 → 68 bytes (98.6% reduction) ⭐ NEW!

Result: ZSTD compression works correctly and produces valid Zstandard compressed data.

---

🎯 Key Findings

1. ✅ ZSTD compression works correctly - Produces valid compressed output with excellent compression ratios
2. ✅ Backward compatibility maintained - PR works with and without ZSTD libraries installed
3. ✅ No regressions - Existing GZIP and Brotli compression continue to work perfectly
4. ✅ Graceful fallback - When ZSTD libraries are unavailable, requests are served uncompressed without errors

📈 Compression Algorithm Performance

On the 4701-byte test file:

1. Brotli: 53 bytes (best compression)
2. ZSTD: 68 bytes (+28% vs Brotli)
3. GZIP: 91 bytes (+72% vs Brotli)

ZSTD achieves excellent compression, sitting between Brotli and GZIP in terms of compression ratio.

---

⚙️ Configuration Requirements

Critical: For Brotli and ZSTD to function properly, set:

records:
  http:
    normalize_ae: 0

Without this setting, ATS core normalizes the Accept-Encoding header before the compress plugin processes it, which interferes with Brotli/ZSTD compression.

---

🏗️ Build Notes

The PR's ZSTD detection via find_package(zstd CONFIG) requires a CMake config file. On systems where libzstd-devel doesn't provide zstdConfig.cmake, a simple config file can be created:

add_library(zstd::zstd UNKNOWN IMPORTED)
set_target_properties(zstd::zstd PROPERTIES
  IMPORTED_LOCATION "/usr/lib64/libzstd.so"
  INTERFACE_INCLUDE_DIRECTORIES "/usr/include"
)
set(zstd_FOUND TRUE)

---

✅ Conclusion

PR #12201 successfully implements ZSTD compression support without any regressions. The implementation:

• Maintains full backward compatibility
• Handles missing ZSTD libraries gracefully
• Achieves excellent compression ratios (98.6% reduction)
• Integrates seamlessly with existing compression infrastructure

All three test phases passed successfully. Great work on this feature! 🎉

[bryancall]

Code Review - ZSTD Compression Implementation

Thank you for this excellent work on adding ZSTD compression support! The testing shows the feature works well and achieves great compression ratios (98.6%). I've completed a thorough code review and found a few issues that should be addressed before merging.

---

🔴 Critical Issue

Resource Management in zstd_transform_init()

Location: plugins/compress/compress.cc:558-580

Problem: If ZSTD_CCtx_setParameter fails during initialization, the function returns early but leaves data->zstrm_zstd.cctx in a valid (non-NULL) state. The caller at line 395-398 only checks if (!data->zstrm_zstd.cctx), which won't catch these initialization failures. This could lead to compression proceeding with a misconfigured context.

Current Code:

static void
zstd_transform_init(Data *data)
{
  if (!data->zstrm_zstd.cctx) {
    error("Failed to initialize Zstd compression context");
    return;
  }

  size_t result = ZSTD_CCtx_setParameter(data->zstrm_zstd.cctx, ZSTD_c_compressionLevel, ...);
  if (ZSTD_isError(result)) {
    error("Failed to set Zstd compression level: %s", ZSTD_getErrorName(result));
    return;  // ⚠️ Context is valid but misconfigured
  }
  
  result = ZSTD_CCtx_setParameter(data->zstrm_zstd.cctx, ZSTD_c_checksumFlag, 1);
  if (ZSTD_isError(result)) {
    error("Failed to enable Zstd checksum: %s", ZSTD_getErrorName(result));
    return;  // ⚠️ Context exists but checksum not enabled
  }
}

Recommended Fix:

static void
zstd_transform_init(Data *data)
{
  if (!data->zstrm_zstd.cctx) {
    error("Failed to initialize Zstd compression context");
    return;
  }

  size_t result = ZSTD_CCtx_setParameter(data->zstrm_zstd.cctx, ZSTD_c_compressionLevel, data->hc->zstd_compression_level());
  if (ZSTD_isError(result)) {
    error("Failed to set Zstd compression level: %s", ZSTD_getErrorName(result));
    ZSTD_freeCCtx(data->zstrm_zstd.cctx);  // ✅ Clean up
    data->zstrm_zstd.cctx = nullptr;        // ✅ Mark as invalid
    return;
  }
  
  result = ZSTD_CCtx_setParameter(data->zstrm_zstd.cctx, ZSTD_c_checksumFlag, 1);
  if (ZSTD_isError(result)) {
    error("Failed to enable Zstd checksum: %s", ZSTD_getErrorName(result));
    ZSTD_freeCCtx(data->zstrm_zstd.cctx);  // ✅ Clean up
    data->zstrm_zstd.cctx = nullptr;        // ✅ Mark as invalid
    return;
  }

  debug("zstd compression context initialized with level %d", data->hc->zstd_compression_level());
}

---

⚠️ Medium Issues

1. Integer Overflow in zstd_compress_operation()

Location: plugins/compress/compress.cc:514

Problem: Casting int64_t upstream_length to size_t without bounds checking. If upstream_length is negative (malicious input or bug), the cast wraps to a huge positive value.

Current Code:

ZSTD_inBuffer input = {upstream_buffer, static_cast<size_t>(upstream_length), 0};

Recommended Fix:

if (upstream_length < 0) {
  error("Invalid upstream_length: %" PRId64 " (negative value)", upstream_length);
  return false;
}

if (upstream_buffer == nullptr && upstream_length > 0) {
  error("upstream_buffer is NULL with non-zero length");
  return false;
}

ZSTD_inBuffer input = {upstream_buffer, static_cast<size_t>(upstream_length), 0};

2. Similar Issue with downstream_length

Location: plugins/compress/compress.cc:520

Problem: Same cast issue, though less critical since it comes from ATS API.

Recommended: Add validation:

if (downstream_length < 0) {
  error("Invalid downstream_length from TSIOBufferBlockWriteStart");
  return false;
}

---

ℹ️ Minor Issues

1. Typo in Comment

Location: plugins/compress/compress.cc:236

Issue: Comment says "brotlidestory" instead of "brotli destroy"

Fix: Change to // Brotli destroy

2. Inconsistent Error Logging

Location: plugins/compress/compress.cc:395-398

Issue: Mix of TSError() and error() macro.

Current:

if (!data->zstrm_zstd.cctx) {
  TSError("Failed to create Zstandard compression context");  // Uses TSError
  return;
}

Elsewhere:

error("Failed to initialize Zstd compression context");  // Uses error()

Recommendation: Use consistent error logging throughout the ZSTD code - prefer error() macro for consistency with the rest of the file.

---

✅ Excellent Practices Observed

1. Proper error checking: All ZSTD errors are checked with ZSTD_isError() and error names are logged
2. Resource cleanup: ZSTD_freeCCtx() is properly called in data_destroy()
3. Data integrity: Checksum is enabled with ZSTD_c_checksumFlag
4. Infinite loop prevention: Code checks for no progress in compression loop (line 542-544)
5. Proper compression modes: Uses ZSTD_e_continue and ZSTD_e_flush appropriately
6. Backward compatibility: Gracefully handles missing ZSTD libraries

---

Summary

• Critical Issues: 1 (must fix before merge)
• Medium Issues: 2 (should fix for robustness)
• Minor Issues: 2 (nice to fix)

Overall Assessment: The ZSTD implementation is well-structured and follows the ZSTD API correctly. The critical initialization issue should be addressed, and the integer overflow checks would significantly improve robustness. Great work on achieving excellent compression ratios and maintaining backward compatibility!

Testing: All three test phases passed successfully ✅

• Test 1 (Master baseline): GZIP and Brotli working
• Test 2 (PR without ZSTD): No regressions, graceful fallback
• Test 3 (PR with ZSTD): ZSTD achieves 98.6% compression (4701 → 68 bytes)

[JakeChampion]

I'll make the changes in this pr if that's okay 👍

[bryancall]

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]

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]

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

[JakeChampion]

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

[bryancall]

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

[JakeChampion]

@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]

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

[bryancall]

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.