You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "otterc (via GitHub)" <gi...@apache.org> on 2024/02/02 18:35:12 UTC
[PR] [CELEBORN-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
otterc opened a new pull request, #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281
### 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?
Yet to add UTs.
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1476853675
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -1573,4 +1634,12 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
heartbeater.stop()
super.stop()
}
+
+ private def createSecret(): String = {
+ val bits = 256
+ val rnd = new SecureRandom()
+ val secretBytes = new Array[Byte](bits / JByte.SIZE)
+ rnd.nextBytes(secretBytes)
+ Hex.encodeHexString(secretBytes)
Review Comment:
The dependency on `commons-codec` is causing issues. Maybe we can just use bytes but this is how it is generated in Spark and I think that it is a common practice to convert secrets into hexadecimal format.
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "wxplovecc (via GitHub)" <gi...@apache.org>.
wxplovecc commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477199713
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/MasterArguments.scala:
##########
@@ -45,11 +46,13 @@ class MasterArguments(args: Array[String], conf: CelebornConf) {
_host = _host.orElse(Some(conf.haMasterNodeHost(localNode.nodeId)))
_port = _port.orElse(Some(conf.haMasterNodePort(localNode.nodeId)))
_internalPort = _internalPort.orElse(Some(conf.haMasterNodeInternalPort(localNode.nodeId)))
+ _securedPort = _securedPort.orElse(Some(conf.haMasterNodeSecuredPort(localNode.nodeId)))
Review Comment:
Same as https://github.com/apache/incubator-celeborn/pull/2283
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477593541
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1116,7 +1116,23 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
// //////////////////////////////////////////////////////
// Authentication //
// //////////////////////////////////////////////////////
- def authEnabled: Boolean = get(AUTH_ENABLED)
+ def authEnabled: Boolean = {
+ val authEnabled = get(AUTH_ENABLED)
+ 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")
Review Comment:
thanks for explanation
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477415296
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/MasterArguments.scala:
##########
@@ -45,11 +46,13 @@ class MasterArguments(args: Array[String], conf: CelebornConf) {
_host = _host.orElse(Some(conf.haMasterNodeHost(localNode.nodeId)))
_port = _port.orElse(Some(conf.haMasterNodePort(localNode.nodeId)))
_internalPort = _internalPort.orElse(Some(conf.haMasterNodeInternalPort(localNode.nodeId)))
+ _securedPort = _securedPort.orElse(Some(conf.haMasterNodeSecuredPort(localNode.nodeId)))
Review Comment:
I fixed this. Thanks for letting me know.
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477188099
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -1573,4 +1634,12 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
heartbeater.stop()
super.stop()
}
+
+ private def createSecret(): String = {
+ val bits = 256
+ val rnd = new SecureRandom()
+ val secretBytes = new Array[Byte](bits / JByte.SIZE)
+ rnd.nextBytes(secretBytes)
+ Hex.encodeHexString(secretBytes)
Review Comment:
reducing dependencies is generally a good idea, but please open a dedicated PR for it, also, SBT and LICENSE parts should be updated too
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2281: [CELEBORN-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager
URL: https://github.com/apache/incubator-celeborn/pull/2281
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477188099
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -1573,4 +1634,12 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
heartbeater.stop()
super.stop()
}
+
+ private def createSecret(): String = {
+ val bits = 256
+ val rnd = new SecureRandom()
+ val secretBytes = new Array[Byte](bits / JByte.SIZE)
+ rnd.nextBytes(secretBytes)
+ Hex.encodeHexString(secretBytes)
Review Comment:
reducing dependencies is generally a good idea, but please open a dedicated PR for it, also, SBT and LICENSE parts should be updated too
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1478539412
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/SecuredRpcEndpoint.scala:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.service.deploy.master
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.protocol.PbUnregisterShuffle
+import org.apache.celeborn.common.protocol.message.ControlMessages.{ApplicationLost, CheckQuota, HeartbeatFromApplication, RequestSlots}
+import org.apache.celeborn.common.rpc._
+
+/**
+ * Secured RPC endpoint used by the Master to communicate with the Clients.
+ */
+private[celeborn] class SecuredRpcEndpoint(
Review Comment:
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. Thank you for pointing it out.
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477189359
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1116,7 +1116,23 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
// //////////////////////////////////////////////////////
// Authentication //
// //////////////////////////////////////////////////////
- def authEnabled: Boolean = get(AUTH_ENABLED)
+ def authEnabled: Boolean = {
+ val authEnabled = get(AUTH_ENABLED)
+ 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")
Review Comment:
what are all valid combinations of AUTH_ENABLED and INTERNAL_PORT_ENABLED?
- true, true
- false, false
what if we eliminate INTERNAL_PORT_ENABLED and just respect AUTH_ENABLED?
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477189493
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1116,7 +1116,23 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
// //////////////////////////////////////////////////////
// Authentication //
// //////////////////////////////////////////////////////
- def authEnabled: Boolean = get(AUTH_ENABLED)
+ def authEnabled: Boolean = {
+ val authEnabled = get(AUTH_ENABLED)
+ 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")
+ }
+ return authEnabled && internalPortEnabled
+ }
+
+ def haMasterNodeSecuredPort(nodeId: String): Int = {
+ val key = HA_MASTER_NODE_SECURED_PORT.key.replace("<id>", nodeId)
+ val legacyKey = HA_MASTER_NODE_SECURED_PORT.alternatives.head._1.replace("<id>", nodeId)
Review Comment:
we don't need handle legacy cases for new added properties
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1116,7 +1116,23 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
// //////////////////////////////////////////////////////
// Authentication //
// //////////////////////////////////////////////////////
- def authEnabled: Boolean = get(AUTH_ENABLED)
+ def authEnabled: Boolean = {
+ val authEnabled = get(AUTH_ENABLED)
+ 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")
+ }
+ return authEnabled && internalPortEnabled
+ }
+
+ def haMasterNodeSecuredPort(nodeId: String): Int = {
+ val key = HA_MASTER_NODE_SECURED_PORT.key.replace("<id>", nodeId)
+ val legacyKey = HA_MASTER_NODE_SECURED_PORT.alternatives.head._1.replace("<id>", nodeId)
Review Comment:
we don't need to handle legacy cases for new added configurations
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477189359
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1116,7 +1116,23 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
// //////////////////////////////////////////////////////
// Authentication //
// //////////////////////////////////////////////////////
- def authEnabled: Boolean = get(AUTH_ENABLED)
+ def authEnabled: Boolean = {
+ val authEnabled = get(AUTH_ENABLED)
+ 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")
Review Comment:
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?
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477415335
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1116,7 +1116,23 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
// //////////////////////////////////////////////////////
// Authentication //
// //////////////////////////////////////////////////////
- def authEnabled: Boolean = get(AUTH_ENABLED)
+ def authEnabled: Boolean = {
+ val authEnabled = get(AUTH_ENABLED)
+ 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")
+ }
+ return authEnabled && internalPortEnabled
+ }
+
+ def haMasterNodeSecuredPort(nodeId: String): Int = {
+ val key = HA_MASTER_NODE_SECURED_PORT.key.replace("<id>", nodeId)
+ val legacyKey = HA_MASTER_NODE_SECURED_PORT.alternatives.head._1.replace("<id>", nodeId)
Review Comment:
Fixed 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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#issuecomment-1924689548
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2281?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `10 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`4c5e6f0`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/4c5e6f065c96ffc6366b19becabec0c7b7f6e6db?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 47.98% compared to head [(`127dc9b`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2281?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 47.87%.
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2281?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...cala/org/apache/celeborn/common/CelebornConf.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2281?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL0NlbGVib3JuQ29uZi5zY2FsYQ==) | 67.75% | [6 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2281?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 #2281 +/- ##
==========================================
- Coverage 47.98% 47.87% -0.10%
==========================================
Files 200 200
Lines 12449 12477 +28
Branches 1088 1092 +4
==========================================
Hits 5972 5972
- Misses 6088 6108 +20
- Partials 389 397 +8
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2281?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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477188488
##########
client/src/main/scala/org/apache/celeborn/client/LifecycleManager.scala:
##########
@@ -1573,4 +1634,12 @@ class LifecycleManager(val appUniqueId: String, val conf: CelebornConf) extends
heartbeater.stop()
super.stop()
}
+
+ private def createSecret(): String = {
+ val bits = 256
+ val rnd = new SecureRandom()
+ val secretBytes = new Array[Byte](bits / JByte.SIZE)
+ rnd.nextBytes(secretBytes)
+ Hex.encodeHexString(secretBytes)
Review Comment:
ignore my previous comments, I misinterpret the message.
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1478539412
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/SecuredRpcEndpoint.scala:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.service.deploy.master
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.protocol.PbUnregisterShuffle
+import org.apache.celeborn.common.protocol.message.ControlMessages.{ApplicationLost, CheckQuota, HeartbeatFromApplication, RequestSlots}
+import org.apache.celeborn.common.rpc._
+
+/**
+ * Secured RPC endpoint used by the Master to communicate with the Clients.
+ */
+private[celeborn] class SecuredRpcEndpoint(
Review Comment:
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.
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477415372
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1116,7 +1116,23 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
// //////////////////////////////////////////////////////
// Authentication //
// //////////////////////////////////////////////////////
- def authEnabled: Boolean = get(AUTH_ENABLED)
+ def authEnabled: Boolean = {
+ val authEnabled = get(AUTH_ENABLED)
+ 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")
Review Comment:
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.
--
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-1257] Adds a secured port in Celeborn Master for secure communication with LifecycleManager [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2281:
URL: https://github.com/apache/incubator-celeborn/pull/2281#discussion_r1477618783
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -394,23 +433,32 @@ private[celeborn] class Master(
// keep it for compatible reason
context.reply(ReleaseSlotsResponse(StatusCode.SUCCESS))
- case requestSlots @ RequestSlots(_, _, _, _, _, _, _, _, _, _, _) =>
- logTrace(s"Received RequestSlots request $requestSlots.")
- executeWithLeaderChecker(context, handleRequestSlots(context, requestSlots))
+ case requestSlots @ RequestSlots(applicationId, _, _, _, _, _, _, _, _, _, _) =>
+ if (checkAuthStatus(applicationId, context)) {
+ // TODO: [CELEBORN-1261]
+ logTrace(s"Received RequestSlots request $requestSlots.")
+ executeWithLeaderChecker(context, handleRequestSlots(context, requestSlots))
+ }
case pb: PbUnregisterShuffle =>
val applicationId = pb.getAppId
val shuffleId = pb.getShuffleId
val requestId = pb.getRequestId
- logDebug(s"Received UnregisterShuffle request $requestId, $applicationId, $shuffleId")
- executeWithLeaderChecker(
- context,
- handleUnregisterShuffle(context, applicationId, shuffleId, requestId))
+ if (checkAuthStatus(applicationId, context)) {
+ // TODO: [CELEBORN-1261]
+ logDebug(s"Received UnregisterShuffle request $requestId, $applicationId, $shuffleId")
+ executeWithLeaderChecker(
+ context,
+ handleUnregisterShuffle(context, applicationId, shuffleId, requestId))
+ }
case ApplicationLost(appId, requestId) =>
- logDebug(
- s"Received ApplicationLost request $requestId, $appId from ${context.senderAddress}.")
- executeWithLeaderChecker(context, handleApplicationLost(context, appId, requestId))
+ if (checkAuthStatus(appId, context)) {
Review Comment:
`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.
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/SecuredRpcEndpoint.scala:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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.service.deploy.master
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.protocol.PbUnregisterShuffle
+import org.apache.celeborn.common.protocol.message.ControlMessages.{ApplicationLost, CheckQuota, HeartbeatFromApplication, RequestSlots}
+import org.apache.celeborn.common.rpc._
+
+/**
+ * Secured RPC endpoint used by the Master to communicate with the Clients.
+ */
+private[celeborn] class SecuredRpcEndpoint(
Review Comment:
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?
--
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