You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "turboFei (via GitHub)" <gi...@apache.org> on 2024/03/07 17:09:07 UTC
[PR] App register 4 [incubator-celeborn]
turboFei opened a new pull request, #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1542263600
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
Agree with most messages shouldn't be sent via TLS. I believe that the most fundamental issue with the existing security authentication mechanism is that it only authenticates the connection, but does not verify the legitimacy of the messages sent by the authenticated client. At the very least, we need verify that the applicationId in the sent messages matches the applicationId provided during the initial authentication, Otherwise, an authenticated client could still access or modify with the data of other applications. I am not sure if this is in line with the expectations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-2013189145
thanks for your suggestions, could you help take a look again?
@RexXiong @mridulm @otterc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1543505154
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
Yes, this is a gap. We can validate the appId in the fetch and push messages of an application. I created https://issues.apache.org/jira/browse/CELEBORN-1360 for that and will create a PR soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1538276401
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
I think we should rename the existing `ApplicationMeta` to `ApplicationAuthMeta` and it should contain information specific to authentication. We can then use ApplicationMeta for general app info that needs to be sent to the Celeborn Master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-2010337868
Have a general question. Why is user identifier needed to be shared with Celeborn? I can't find any information in the linked jira: [CELEBORN-1285](https://issues.apache.org/jira/browse/CELEBORN-1285)
Also, I think if registration is required for the feature that we are trying to support, I think it will better to add a feature flag instead of `register.enabled` flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1543505154
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
Yes, this is a gap. I created https://issues.apache.org/jira/browse/CELEBORN-1360 to address this. Will open a PR soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-1984773390
cc @RexXiong
Could you help have a overall review? I can enhance it after that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-2010741754
> Have a general question. Why is user identifier needed to be shared with Celeborn? I can't find any information in the linked jira: [CELEBORN-1285](https://issues.apache.org/jira/browse/CELEBORN-1285) Also, I think if registration is required for the feature that we are trying to support, I think it will better to add a feature flag instead of `register.enabled` flag.
Thanks for the suggestion, will check how to implement it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1540531750
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
> This actually conflicts with safely propagating the application secret to the Celeborn Master. When auth is enabled, this will transmit application secret to Celeborn Master without any Sasl client authentication. Currently, we have added Anonymous client authentication, but the plan was to add other mechanisms. cc. @mridulm
Based on my understanding, if authentication is enabled in Celeborn, it is not possible to access the Celeborn Master without SASL client authentication. This PR does not introduce new mechanisms; it merely adds an identifier to ApplicationMeta. Therefore, I believe this PR does not compromise security. @otterc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-2008701185
> This pr is used to provide the tenant information for dashboard. https://issues.apache.org/jira/browse/CELEBORN-1227
>
> Do we plan to set `celeborn.auth.enabled` to true by defaults?
No, IMO this feature has nothing to do with whether the auth enabled or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#discussion_r1535251964
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -113,7 +113,7 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val mockDestroyFailure = conf.testMockDestroySlotsFailure
private val authEnabled = conf.authEnabledOnClient
- private var applicationMeta: ApplicationMeta = _
+ private var applicationAuthMeta: ApplicationMeta = _
Review Comment:
Keep the name as it not only for auth scenario
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -173,7 +173,7 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
if (authEnabled) {
logInfo(s"Authentication is enabled; setting up master and worker RPC environments")
val appSecret = createSecret()
- applicationMeta = ApplicationMeta(appUniqueId, appSecret)
+ applicationAuthMeta = ApplicationMeta(appUniqueId, appSecret)
Review Comment:
Add else to initialize ApplicationMeta when auth disabled?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#issuecomment-2063321382
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/2365?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `30.00000%` with `14 lines` in your changes are missing coverage. Please review.
> Project coverage is 48.93%. Comparing base [(`fc23800`)](https://app.codecov.io/gh/apache/celeborn/commit/fc238005bd8482ea41612aae6aae7e8f16f918f5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`0feedd7`)](https://app.codecov.io/gh/apache/celeborn/pull/2365?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 36 commits behind head on main.
> :exclamation: Current head 0feedd7 differs from pull request most recent head a9a4761. Consider uploading reports for the commit a9a4761 to get more accurate results
| [Files](https://app.codecov.io/gh/apache/celeborn/pull/2365?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...org/apache/celeborn/common/util/PbSerDeUtils.scala](https://app.codecov.io/gh/apache/celeborn/pull/2365?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Futil%2FPbSerDeUtils.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3V0aWwvUGJTZXJEZVV0aWxzLnNjYWxh) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...born/common/protocol/message/ControlMessages.scala](https://app.codecov.io/gh/apache/celeborn/pull/2365?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fprotocol%2Fmessage%2FControlMessages.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3Byb3RvY29sL21lc3NhZ2UvQ29udHJvbE1lc3NhZ2VzLnNjYWxh) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../apache/celeborn/common/meta/ApplicationMeta.scala](https://app.codecov.io/gh/apache/celeborn/pull/2365?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fmeta%2FApplicationMeta.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL21ldGEvQXBwbGljYXRpb25NZXRhLnNjYWxh) | 80.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2365 +/- ##
==========================================
- Coverage 48.96% 48.93% -0.03%
==========================================
Files 209 209
Lines 13146 13153 +7
Branches 1135 1137 +2
==========================================
- Hits 6436 6435 -1
- Misses 6287 6296 +9
+ Partials 423 422 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/2365?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support to register application without auth enabled [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-1984426314
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `46.31579%` with `51 lines` in your changes are missing coverage. Please review.
> Project coverage is 48.76%. Comparing base [(`835437f`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/835437f0b9d89e5c3ce44f5aacad2a7d396da1a8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`60696de`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1 commits behind head on main.
> :exclamation: Current head 60696de differs from pull request most recent head 89472cd. Consider uploading reports for the commit 89472cd to get more accurate results
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pache/celeborn/common/rpc/RpcSecurityContext.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3JwYy9ScGNTZWN1cml0eUNvbnRleHQuc2NhbGE=) | 0.00% | [21 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/celeborn/common/util/PbSerDeUtils.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3V0aWwvUGJTZXJEZVV0aWxzLnNjYWxh) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...cala/org/apache/celeborn/common/CelebornConf.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL0NlbGVib3JuQ29uZi5zY2FsYQ==) | 50.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/celeborn/common/rpc/netty/NettyRpcEnv.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3JwYy9uZXR0eS9OZXR0eVJwY0Vudi5zY2FsYQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...sasl/registration/RegistrationClientBootstrap.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9zYXNsL3JlZ2lzdHJhdGlvbi9SZWdpc3RyYXRpb25DbGllbnRCb290c3RyYXAuamF2YQ==) | 80.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...work/sasl/registration/RegistrationRpcHandler.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9zYXNsL3JlZ2lzdHJhdGlvbi9SZWdpc3RyYXRpb25ScGNIYW5kbGVyLmphdmE=) | 77.78% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../apache/celeborn/common/meta/ApplicationInfo.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL21ldGEvQXBwbGljYXRpb25JbmZvLnNjYWxh) | 80.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../apache/celeborn/common/meta/ApplicationMeta.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL21ldGEvQXBwbGljYXRpb25NZXRhLnNjYWxh) | 75.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2365 +/- ##
==========================================
- Coverage 48.83% 48.76% -0.06%
==========================================
Files 208 210 +2
Lines 12984 13060 +76
Branches 1115 1123 +8
==========================================
+ Hits 6340 6368 +28
- Misses 6233 6278 +45
- Partials 411 414 +3
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2365?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-1998921988
>is there any benefit to not supporting auth going forward given authn support has completed ?
I agree, we will also enable auth for our use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1538336421
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
checking
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#issuecomment-2063320904
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-1985017566
> cc @RexXiong
>
> Could you help have a overall review? I can enhance it after that.
OK, I will review this asap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#discussion_r1531317165
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1142,12 +1141,26 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
authEnabled && internalPortEnabled
}
+ def appRegisterEnabled: Boolean = {
+ val appRegisterEnabled = get(APP_REGISTER_ENABLED) || authEnabled
+ val internalPortEnabled = get(INTERNAL_PORT_ENABLED)
Review Comment:
without internalPort, worker can not interactive with the master rpc endpoint directly, as there is a RegisterationRpcHandler
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#discussion_r1523430529
##########
common/src/main/scala/org/apache/celeborn/common/meta/ApplicationInfo.scala:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.meta
+
+import java.util.concurrent.atomic.LongAdder
+
+/**
+ * Application Info.
+ */
+class ApplicationInfo {
Review Comment:
why we need add this class? and it seems irrelevant with this pr
##########
common/src/main/scala/org/apache/celeborn/common/identity/UserIdentifier.scala:
##########
@@ -44,6 +44,8 @@ case class UserIdentifier(tenantId: String, name: String) {
object UserIdentifier extends Logging {
val USER_IDENTIFIER = "^\\`(.+)\\`\\.\\`(.+)\\`$".r
+ val UNKNOWN_USER_IDENTIFIER = UserIdentifier("unknown", "unknown")
Review Comment:
UNKNOWN_USER_IDENTIFIER can refer to IdentityProvider.DEFAULT_TENANT_ID and DEFAULT_USERNAME, and the name can be changed to DEFAULT_USER_IDENTIFIER
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -93,6 +94,7 @@ private[celeborn] class Master(
.withServerSaslContext(
new ServerSaslContextBuilder()
.withAddRegistrationBootstrap(true)
+ .withAUthEnabled(authEnabled)
Review Comment:
.withAuthEnabled
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1142,12 +1141,26 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
authEnabled && internalPortEnabled
}
+ def appRegisterEnabled: Boolean = {
+ val appRegisterEnabled = get(APP_REGISTER_ENABLED) || authEnabled
+ val internalPortEnabled = get(INTERNAL_PORT_ENABLED)
Review Comment:
internalPort only used between Celeborn master and workers, appRegister should not pay attention to this.
##########
common/src/main/java/org/apache/celeborn/common/network/sasl/registration/RegistrationRpcHandler.java:
##########
@@ -177,7 +183,9 @@ private void processRpcMessage(
break;
case REGISTER_APPLICATION_REQUEST_VALUE:
PbRegisterApplicationRequest registerApplicationRequest = pbMsg.getParsedPayload();
- checkRequestAllowed(RegistrationState.AUTHENTICATED);
+ if (authEnabled) {
+ checkRequestAllowed(RegistrationState.AUTHENTICATED);
Review Comment:
how about move authEnabled condition into checkRequestAllowed?
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -93,14 +94,18 @@ public void updateRequestSlotsMeta(
registeredShuffle.add(shuffleKey);
String appId = Utils.splitShuffleKey(shuffleKey)._1;
- appHeartbeatTime.compute(
+ applications.compute(
Review Comment:
Better not change to applicationInfo in this pr.
##########
common/src/main/scala/org/apache/celeborn/common/util/PbSerDeUtils.scala:
##########
@@ -457,6 +459,22 @@ object PbSerDeUtils {
builder.build()
}
+ def fromPbApplicationInfo(pbApplicationInfo: PbApplicationInfo): ApplicationInfo = {
+ val applicationInfo = new ApplicationInfo()
+ applicationInfo.updateFileCount(pbApplicationInfo.getFileCount)
+ applicationInfo.updateTotalWritten(pbApplicationInfo.getTotalWritten)
+ applicationInfo.setHeartbeatTime(pbApplicationInfo.getLastHeartbeatTime)
+ applicationInfo
+ }
+
+ def toPbApplicationInfo(applicationInfo: ApplicationInfo): PbApplicationInfo = {
Review Comment:
PbApplicationMeta need add userIdentifier and should serialize/deserialize to/from ratis log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1538269411
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
This actually conflicts with safely propagating the application secret to the Celeborn Master. When auth is enabled, this will transmit application secret to Celeborn Master without any Sasl client authentication. Currently, we have added Anonymous client authentication, but the plan was to add other mechanisms.
cc. @mridulm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1544084501
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
> Yes, this is a gap. We can validate the appId in the fetch and push messages of an application. I created https://issues.apache.org/jira/browse/CELEBORN-1360 for that and will create a PR soon.
If that's the case, then we won't have to send a secret; we'll only need to update the userIdentifier. I believe that extending the ApplicationMeta to contain more application-related information is acceptable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-1996735301
A general query on this - is there any benefit to not supporting auth ?
This change is not backwardly compatible - so existing applications cannot leverage it - for newer deployments and/or upgrade - make authn standard ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#discussion_r1536555907
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -173,7 +173,7 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
if (authEnabled) {
logInfo(s"Authentication is enabled; setting up master and worker RPC environments")
val appSecret = createSecret()
- applicationMeta = ApplicationMeta(appUniqueId, appSecret)
+ applicationAuthMeta = ApplicationMeta(appUniqueId, appSecret)
Review Comment:
thanks, addressed.
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -113,7 +113,7 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val mockDestroyFailure = conf.testMockDestroySlotsFailure
private val authEnabled = conf.authEnabledOnClient
- private var applicationMeta: ApplicationMeta = _
+ private var applicationAuthMeta: ApplicationMeta = _
Review Comment:
thanks, addressed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [incubator-celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2365:
URL: https://github.com/apache/incubator-celeborn/pull/2365#issuecomment-2008402579
This pr is used to provide the tenant information for dashboard. https://issues.apache.org/jira/browse/CELEBORN-1227
Do we plan to set `celeborn.auth.enabled` to true by defaults?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1539768438
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
raised https://github.com/apache/celeborn/pull/2420 to separate application auth and general meta. cc @otterc @RexXiong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1258] Add UserIdentifier to the master application meta [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2365:
URL: https://github.com/apache/celeborn/pull/2365#discussion_r1541652506
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -207,13 +209,20 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
private val changePartitionManager = new ChangePartitionManager(conf, this)
private val releasePartitionManager = new ReleasePartitionManager(conf, this)
+ private def updateApplicationMeta(): Unit = {
+ Utils.tryLogNonFatalError(masterClient.askSync[PbApplicationMetaUpdateResponse](
+ PbSerDeUtils.toPbApplicationMeta(applicationMeta),
+ classOf[PbApplicationMetaUpdateResponse]))
+ }
+
// Since method `onStart` is executed when `rpcEnv.setupEndpoint` is executed, and
// `masterClient` is initialized after `rpcEnv` is initialized, if method `onStart` contains
// a reference to `masterClient`, there may be cases where `masterClient` is null when
// `masterClient` is called. Therefore, it's necessary to uniformly execute the initialization
// method at the end of the construction of the class to perform the initialization operations.
private def initialize(): Unit = {
// noinspection ConvertExpressionToSAM
+ updateApplicationMeta()
Review Comment:
Yes, you are correct. The connection will only be established once the client is authenticated. However, I still believe that sending the secret in plain text multiple times to the Master may cause issues in the future. Eventually, we would want to encrypt the secret, and TLS support is for that purpose. If we are sending the secret multiple times, it would mean that not only registration has to be done with TLS but also that all messages should be sent via TLS. Therefore, I think having authentication metadata separated from general application metadata will be better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org