[CELEBORN-2411][CIP22] Preserve Spark IO cipher transformation - #3790
[CELEBORN-2411][CIP22] Preserve Spark IO cipher transformation#3790SparksFyz wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3790 +/- ##
============================================
+ Coverage 58.41% 58.55% +0.15%
Complexity 229 229
============================================
Files 398 399 +1
Lines 27993 28054 +61
Branches 2734 2738 +4
============================================
+ Hits 16349 16424 +75
+ Misses 10446 10430 -16
- Partials 1198 1200 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the fix. One suggestion on the tests: both new interop tests use |
69422a6 to
3e7d31e
Compare
|
Thanks for the suggestion. Updated. @zaynt4606 |
3e7d31e to
b5bde31
Compare
There was a problem hiding this comment.
Pull request overview
Preserves Spark’s effective IO cipher transformation when Celeborn’s SparkCryptoHandler builds a minimized SparkConf, and adds cross-implementation interoperability tests to prevent silent fallback to Spark’s default cipher.
Changes:
- Propagate
spark.io.encryption.crypto.cipher.transformationinto the minimizedSparkConfused bySparkCryptoHandler. - Add bidirectional interoperability tests between Celeborn
SparkCryptoHandlerand SparkCryptoStreamUtils, including a non-defaultAES/CBC/PKCS5Paddingtransformation and the default transformation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| client-spark/common/src/main/java/org/apache/spark/shuffle/celeborn/SparkCryptoHandler.java | Copies Spark’s effective IO cipher transformation into the minimized SparkConf used for crypto stream creation. |
| client-spark/common/src/test/java/org/apache/spark/shuffle/celeborn/SparkCryptoHandlerSuiteJ.java | Adds Spark↔Celeborn crypto interoperability tests for both default and explicitly configured cipher transformations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks @SparksFyz for the contribution! |
### What changes were proposed in this pull request? Preserve Spark's effective IO cipher transformation when `SparkCryptoHandler` builds its minimized `SparkConf`. Add bidirectional interoperability tests between `SparkCryptoHandler` and Spark's native `CryptoStreamUtils` using `AES/CBC/PKCS5Padding`. ### Why are the changes needed? `CryptoStreamUtils.toCryptoConf()` only extracts `spark.io.encryption.commons.config.*`, while Spark reads `IO_CRYPTO_CIPHER_TRANSFORMATION` separately from `SparkConf`. As a result, `SparkCryptoHandler` previously dropped a non-default transformation and silently fell back to Spark's default `AES/CTR/NoPadding`. Existing self-round-trip tests did not detect this because both encryption and decryption used the same minimized configuration. ### Does this PR resolve a correctness bug? - [ ] Yes ### Does this PR introduce _any_ user-facing change? - [x] Yes A non-default Spark IO cipher transformation is now preserved by Celeborn. The default `AES/CTR/NoPadding` behavior remains unchanged. ### How was this patch tested? - Added Celeborn encryption -> Spark `CryptoStreamUtils` decryption coverage. - Added Spark `CryptoStreamUtils` encryption -> Celeborn decryption coverage. - Spark 3.5 `SparkCryptoHandlerSuiteJ`: 10 tests passed. - Spark 3.0 / Scala 2.12.10 / JDK 11 compilation passed. - Spark 4.0 / Scala 2.13.16 / JDK 17 compilation passed. - Spark 3.5 Spotless check passed. Closes #3790 from SparksFyz/CELEBORN-2411-cipher-transformation. Authored-by: SparksFyz <sparksvann@gmail.com> Signed-off-by: zhengtao <shuaizhentao.szt@alibaba-inc.com> (cherry picked from commit 9941064) Signed-off-by: zhengtao <shuaizhentao.szt@alibaba-inc.com>
What changes were proposed in this pull request?
Preserve Spark's effective IO cipher transformation when
SparkCryptoHandlerbuilds its minimized
SparkConf.Add bidirectional interoperability tests between
SparkCryptoHandlerandSpark's native
CryptoStreamUtilsusingAES/CBC/PKCS5Padding.Why are the changes needed?
CryptoStreamUtils.toCryptoConf()only extractsspark.io.encryption.commons.config.*, while Spark readsIO_CRYPTO_CIPHER_TRANSFORMATIONseparately fromSparkConf.As a result,
SparkCryptoHandlerpreviously dropped a non-defaulttransformation and silently fell back to Spark's default
AES/CTR/NoPadding.Existing self-round-trip tests did not detect this because both encryption
and decryption used the same minimized configuration.
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
A non-default Spark IO cipher transformation is now preserved by Celeborn.
The default
AES/CTR/NoPaddingbehavior remains unchanged.How was this patch tested?
CryptoStreamUtilsdecryption coverage.CryptoStreamUtilsencryption -> Celeborn decryption coverage.SparkCryptoHandlerSuiteJ: 10 tests passed.