[CELEBORN-2388] Decouple inter-master Ratis TLS into its own SSL module - #3767
[CELEBORN-2388] Decouple inter-master Ratis TLS into its own SSL module#3767shellfish007 wants to merge 2 commits into
Conversation
Master<->master Ratis TLS is currently derived from the rpc_service
module (MasterClusterInfo.scala -> HARaftServer.configureSsl). But
rpc_service is also the client-facing control cert, so Ratis and
external clients are forced to share one presented cert. That cert
must simultaneously satisfy:
- clients: signed by a CA they trust (e.g. the corp CA) + the
gateway/SNI hostname, and
- Ratis peers: gRPC hostname verification against the internal
master pod FQDNs (*.<svc>-master-svc.<ns>.svc.cluster.local).
In a cross-cluster / TLS-passthrough-gateway deployment these
requirements conflict (the client cert has no internal SANs), so HA
masters can't complete the Ratis TLS handshake (UNAVAILABLE: io
exception during leader election). Decoupling lets operators give
Ratis its own cert (internal SANs) while leaving the client-facing
cert untouched.
- TransportModuleConstants: add RATIS_MODULE = "ratis".
- MasterClusterInfo.scala: compute Ratis SSL from the ratis module
with fallback to rpc_service:
ratisSslEnabled = sslEnabled(ratis) || sslEnabled(rpc_service)
ratisSslModule = ratis if sslEnabled(ratis) else rpc_service
- HARaftServer.configureSsl: build the SSLFactory from the selected
module (same fallback).
Config keys (celeborn.ssl.ratis.enabled/.keyStore/.trustStore/...)
come from the existing celeborn.ssl.<module>.* machinery - no
enumerated module list needed.
Default behavior is unchanged: with only rpc_service SSL configured,
Ratis uses the rpc_service cert exactly as today. The ratis module
only takes effect when celeborn.ssl.ratis.enabled=true is explicitly
set.
|
@shellfish007, please create new issues in JIRA which follows #1053. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated SSL/TLS “ratis” transport module so inter-master Ratis (Raft) gRPC TLS can use its own keystore/certificate, while preserving the current behavior by falling back to the existing rpc_service SSL module when ratis SSL is not explicitly enabled.
Changes:
- Add
TransportModuleConstants.RATIS_MODULE = "ratis". - Update
MasterClusterInfoto enable Ratis TLS when eitherratisorrpc_serviceSSL is enabled (backward-compatible fallback). - Update
HARaftServer.configureSslto build theSSLFactoryfrom the selected SSL module (ratisif enabled, elserpc_service).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| master/src/main/scala/org/apache/celeborn/service/deploy/master/clustermeta/ha/MasterClusterInfo.scala | Adds helper logic for determining whether Ratis TLS is enabled and which SSL module to use (with rpc_service fallback). |
| master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java | Selects the SSL module used to create the Ratis SSLFactory (preferring ratis when enabled). |
| common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java | Introduces the new ratis transport module constant for per-module SSL config resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // MasterClusterInfo.ratisSslModule (kept in sync as the single source of truth for the policy). | ||
| String sslModule = | ||
| conf.sslEnabled(TransportModuleConstants.RATIS_MODULE) | ||
| ? TransportModuleConstants.RATIS_MODULE | ||
| : TransportModuleConstants.RPC_SERVICE_MODULE; |
|
JIRA ticket filed: https://issues.apache.org/jira/browse/CELEBORN-2388. PR title updated accordingly. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/HARaftServer.java:453
- The comment says
MasterClusterInfo.ratisSslModuleis the "single source of truth", but the module-selection policy is duplicated here (ternary) and the Scala helper is not actually used. This is misleading for maintainers; either call the helper, or reword this comment to reflect that the logic is mirrored/duplicated.
// Build the Ratis SSLFactory from the dedicated `ratis` SSL module when it is explicitly
// enabled, otherwise fall back to the client-facing `rpc_service` module. This decouples the
// inter-master Ratis cert from the client-facing rpc_service cert while keeping existing
// (rpc_service-only) deployments byte-for-byte unchanged. This mirrors
// MasterClusterInfo.ratisSslModule (kept in sync as the single source of truth for the policy).
master/src/main/scala/org/apache/celeborn/service/deploy/master/clustermeta/ha/MasterClusterInfo.scala:103
ratisSslModuleis currently unused in the codebase (HARaftServer re-implements the selection logic inline). Leaving this helper unused increases the risk the two implementations drift over time. Either remove this method, or make HARaftServer consume it (e.g., expose it to Java via@JvmStaticand call it from HARaftServer) so there is only one policy implementation.
if (conf.sslEnabled(TransportModuleConstants.RATIS_MODULE)) {
TransportModuleConstants.RATIS_MODULE
} else {
TransportModuleConstants.RPC_SERVICE_MODULE
}
master/src/main/scala/org/apache/celeborn/service/deploy/master/clustermeta/ha/MasterClusterInfo.scala:90
- This PR introduces a new supported SSL module (
celeborn.ssl.ratis.*) and new enablement/fallback behavior, but there is no test exercising the new scenario whereceleborn.ssl.ratis.enabled=truewhileceleborn.ssl.rpc_service.enabled=false. Given there are existing HA Ratis SSL tests, please add/extend a test to cover the dedicatedratismodule path (and verify the legacy fallback still works).
def ratisSslEnabled(conf: CelebornConf): Boolean = {
conf.sslEnabled(TransportModuleConstants.RATIS_MODULE) ||
conf.sslEnabled(TransportModuleConstants.RPC_SERVICE_MODULE)
}
What changes were proposed in this pull request?
Adds a dedicated
ratisSSL module so inter-master Ratis (Raft consensus) gRPC TLS can use its own certificate/keystore, independent of the client-facingrpc_servicemodule — with a fully backward-compatible fallback.TransportModuleConstants: addRATIS_MODULE = "ratis".MasterClusterInfo.scala: compute Ratis SSL from theratismodule with fallback torpc_service:ratisSslEnabled = sslEnabled(ratis) || sslEnabled(rpc_service)ratisSslModule = ratis if sslEnabled(ratis) else rpc_serviceHARaftServer.configureSsl: build theSSLFactoryfrom the selected module (same fallback).Config keys (
celeborn.ssl.ratis.enabled/.keyStore/.trustStore/...) come from the existingceleborn.ssl.<module>.*machinery — no enumerated module list needed.Why are the changes needed?
Master↔master Ratis TLS is currently derived from the
rpc_servicemodule (MasterClusterInfo.scala→HARaftServer.configureSsl). Butrpc_serviceis also the client-facing control cert, so Ratis and external clients are forced to share one presented cert. That cert must simultaneously satisfy:*.<svc>-master-svc.<ns>.svc.cluster.local).In a cross-cluster / TLS-passthrough-gateway deployment these requirements conflict (the client cert has no internal SANs), so HA masters can't complete the Ratis TLS handshake ("UNAVAILABLE: io exception" during leader election). Decoupling lets operators give Ratis its own cert (internal SANs) while leaving the client-facing cert untouched.
Note: CELEBORN-1356 already split the unified
rpcmodule intorpc_app/rpc_servicefor client-vs-server separation, but never separated Ratis inter-master traffic fromrpc_servicespecifically — this PR closes that remaining gap.Does this PR introduce any user-facing change?
Default behavior is unchanged: with only
rpc_serviceSSL configured, Ratis uses therpc_servicecert exactly as today. The newratismodule only takes effect whenceleborn.ssl.ratis.enabled=trueis explicitly set.How was this patch tested?
Follows existing SSL-module patterns; no new APIs beyond the additive
ratismodule constant and the fallback resolution helpers.