[CELEBORN-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager #2281

otterc · 2024-02-02T18:35:09Z

What changes were proposed in this pull request?

This adds a secured port to Celeborn Master which is used for secure communication with LifecycleManager.
This is part of adding authentication support in Celeborn (see CELEBORN-1011).

This change targets just adding the secured port to Master. The following items from the proposal are still pending:

Persisting the app secrets in Ratis.
Forwarding secrets to Workers and having ability for the workers to pull registration info from the Master.
Secured and internal port in Workers.
Secured communication between workers and clients.

In addition, since we are supporting both secured and unsecured communication for backward compatibility and seamless rolling upgrades, there is an additional change needed. An app which registers with the Master can try to talk to the workers on unsecured ports which is a security breach. So, the workers need to know whether an app registered with Master or not and for that Master has to propagate list of un-secured apps to Celeborn workers as well. We can discuss this more with https://issues.apache.org/jira/browse/CELEBORN-1261

Why are the changes needed?

It is needed for adding authentication support to Celeborn (CELEBORN-1011)

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

Added a simple UT.

codecov · 2024-02-02T21:00:07Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (277d060) 47.84% compared to head (fa0b02d) 48.61%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
...cala/org/apache/celeborn/common/CelebornConf.scala	76.93%	14 Missing and 1 partial ⚠️
...rg/apache/celeborn/common/client/MasterClient.java	43.75%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2281      +/-   ##
==========================================
+ Coverage   47.84%   48.61%   +0.77%     
==========================================
  Files         200      208       +8     
  Lines       12449    12825     +376     
  Branches     1088     1104      +16     
==========================================
+ Hits         5955     6233     +278     
- Misses       6102     6190      +88     
- Partials      392      402      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala

pan3793 · 2024-02-04T06:48:20Z

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

+    val internalPortEnabled = get(INTERNAL_PORT_ENABLED)
+    if (authEnabled && !internalPortEnabled) {
+      throw new IllegalArgumentException(
+        s"${AUTH_ENABLED.key} is true, but ${INTERNAL_PORT_ENABLED.key} is false")


what are all valid combinations of AUTH_ENABLED and INTERNAL_PORT_ENABLED?

true, true

false, false

and others?

what if we eliminate INTERNAL_PORT_ENABLED and just respect AUTH_ENABLED?

Another valid combination is auth_enabled = false and internal_port_enabled =true.
Having Masters and workers communicate on a separate port is a distinct feature from authentication. In a prior discussion with @waitinfuture, they were considering adding a separate port for internal communication for different reasons. However, it's important to note that this separate internal port is a prerequisite for authentication.

thanks for explanation

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala

master/src/main/scala/org/apache/celeborn/service/deploy/master/MasterArguments.scala

…unication with LifeCycleManager

pan3793

the code makes sense to me, thanks

waitinfuture · 2024-02-05T02:52:27Z

master/src/main/scala/org/apache/celeborn/service/deploy/master/SecuredRpcEndpoint.scala

+/**
+ * Secured RPC endpoint used by the Master to communicate with the Clients.
+ */
+private[celeborn] class SecuredRpcEndpoint(


When auth is enabled, LifecycleManager should connect to this endpoint instead of the unsecured one, right? I guess it's needed to change how masterClient is created in LifecycleManager, will add in future?

Yes, I missed making that change because in our internal POC we didn't use a dedicated secure port. I'll add that change with this PR. Thanks for pointing it out.

waitinfuture · 2024-02-05T02:58:37Z

master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala

-      logDebug(
-        s"Received ApplicationLost request $requestId, $appId from ${context.senderAddress}.")
-      executeWithLeaderChecker(context, handleApplicationLost(context, appId, requestId))
+      if (checkAuthStatus(appId, context)) {


ApplicationLost is triggered by both LifecycleManager (when unregister shuffle) and Master (when timeout dead applications). I guess we don't need to check for the later one? We can check whether senderAddress equals to rpcEnv's address.

waitinfuture

LGTM, thanks! Merging to main(v0.5.0)

…munication with LifecycleManager ### What changes were proposed in this pull request? This adds a secured port to Celeborn Master which is used for secure communication with LifecycleManager. This is part of adding authentication support in Celeborn (see CELEBORN-1011). This change targets just adding the secured port to Master. The following items from the proposal are still pending: 1. Persisting the app secrets in Ratis. 2. Forwarding secrets to Workers and having ability for the workers to pull registration info from the Master. 3. Secured and internal port in Workers. 4. Secured communication between workers and clients. In addition, since we are supporting both secured and unsecured communication for backward compatibility and seamless rolling upgrades, there is an additional change needed. An app which registers with the Master can try to talk to the workers on unsecured ports which is a security breach. So, the workers need to know whether an app registered with Master or not and for that Master has to propagate list of un-secured apps to Celeborn workers as well. We can discuss this more with https://issues.apache.org/jira/browse/CELEBORN-1261 ### Why are the changes needed? It is needed for adding authentication support to Celeborn (CELEBORN-1011) ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? Added a simple UT. Closes apache#2281 from otterc/CELEBORN-1257. Authored-by: Chandni Singh <singh.chandni@gmail.com> Signed-off-by: zky.zhoukeyong <zky.zhoukeyong@alibaba-inc.com>

### What changes were proposed in this pull request? Fix MasterClient construct method use in MasterClientSuiteJ. ### Why are the changes needed? MasterClient's construct method has changed by #2281 on main. It's a feature to support authentication on branch-0.5. #2466 's backport on branch-0.4 here caused a conflict in MasterClientSuiteJ.java:319. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Local compile test. Closes #2475 from onebox-li/branch-0.4-fix-compile. Authored-by: onebox-li <lyh-36@163.com> Signed-off-by: SteNicholas <programgeek@163.com>

### What changes were proposed in this pull request? Fix MasterClient construct method use in MasterClientSuiteJ. ### Why are the changes needed? MasterClient's construct method has changed by apache#2281 on main. It's a feature to support authentication on branch-0.5. apache#2466 's backport on branch-0.4 here caused a conflict in MasterClientSuiteJ.java:319. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Local compile test. Closes apache#2475 from onebox-li/branch-0.4-fix-compile. Authored-by: onebox-li <lyh-36@163.com> Signed-off-by: SteNicholas <programgeek@163.com>

otterc commented Feb 2, 2024

View reviewed changes

client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala Outdated Show resolved Hide resolved

pan3793 reviewed Feb 4, 2024

View reviewed changes

common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala Outdated Show resolved Hide resolved

wxplovecc reviewed Feb 4, 2024

View reviewed changes

master/src/main/scala/org/apache/celeborn/service/deploy/master/MasterArguments.scala Outdated Show resolved Hide resolved

otterc added 3 commits February 4, 2024 12:55

[CELEBORN-1257] Addsa secured port in Celeborn Master for secure comm…

c350bd7

…unication with LifeCycleManager

Updated docs and cleanup

d132f36

Removing the dependency from commons-codec

0d15254

otterc force-pushed the CELEBORN-1257 branch from c0c3d29 to 0d15254 Compare February 4, 2024 18:06

pan3793 approved these changes Feb 5, 2024

View reviewed changes

waitinfuture reviewed Feb 5, 2024

View reviewed changes

Addressed review comments

0a86474

otterc force-pushed the CELEBORN-1257 branch from 0f98c4b to 0a86474 Compare February 5, 2024 18:46

otterc added 2 commits February 5, 2024 14:11

Updated doc

8b8ad73

Added UTs

04111ca

otterc force-pushed the CELEBORN-1257 branch from b71cd81 to 04111ca Compare February 6, 2024 05:08

fixed the test

fa0b02d

waitinfuture approved these changes Feb 6, 2024

View reviewed changes

waitinfuture closed this in ab4c0bc Feb 6, 2024

onebox-li mentioned this pull request Apr 22, 2024

[CELEBORN-1397] Fix compilation error on branch-0.4 #2475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CELEBORN-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager #2281

[CELEBORN-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager #2281

Uh oh!

otterc commented Feb 2, 2024 •

edited

Loading

Uh oh!

codecov bot commented Feb 2, 2024 •

edited

Loading

Uh oh!

Uh oh!

pan3793 Feb 4, 2024 •

edited

Loading

Uh oh!

otterc Feb 4, 2024

Uh oh!

pan3793 Feb 5, 2024

Uh oh!

Uh oh!

Uh oh!

pan3793 left a comment

Uh oh!

waitinfuture Feb 5, 2024

Uh oh!

otterc Feb 5, 2024 •

edited

Loading

Uh oh!

waitinfuture Feb 5, 2024

Uh oh!

waitinfuture left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[CELEBORN-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager #2281

[CELEBORN-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager #2281

Uh oh!

Conversation

otterc commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

codecov bot commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

pan3793 Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otterc Feb 4, 2024

Choose a reason for hiding this comment

Uh oh!

pan3793 Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pan3793 left a comment

Choose a reason for hiding this comment

Uh oh!

waitinfuture Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

otterc Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waitinfuture Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

waitinfuture left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

otterc commented Feb 2, 2024 •

edited

Loading

codecov bot commented Feb 2, 2024 •

edited

Loading

pan3793 Feb 4, 2024 •

edited

Loading

otterc Feb 5, 2024 •

edited

Loading

waitinfuture left a comment •

edited

Loading