You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hasnain-db (via GitHub)" <gi...@apache.org> on 2023/10/16 15:54:02 UTC
[PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
hasnain-db opened a new pull request, #43387:
URL: https://github.com/apache/spark/pull/43387
### What changes were proposed in this pull request?
This change ensures that RPC SSL options settings inheritance works properly after https://github.com/apache/spark/pull/43238 - we pass `sslOptions` wherever we call `fromSparkConf`.
In addition to that minor mechanical change, duplicate/add tests for every place that calls this method, to add a test case that runs with SSL support in the config.
### Why are the changes needed?
These changes are needed to ensure that the RPC SSL functionality can work properly with settings inheritance. In addition, through these tests we can ensure that any changes to these modules are also tested with SSL support and avoid regressions in the future.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Full integration testing also done as part of https://github.com/apache/spark/pull/42685
Added some tests and ran them:
```
build/sbt
> project core
> testOnly org.apache.spark.*Ssl*
> testOnly org.apache.spark.network.netty.NettyBlockTransferSecuritySuite
```
and
```
build/sbt -Pyarn
> project yarn
> testOnly org.apache.spark.network.yarn.SslYarnShuffleServiceWithRocksDBBackendSuite
```
### Was this patch authored or co-authored using generative AI tooling?
No
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43387:
URL: https://github.com/apache/spark/pull/43387#issuecomment-1778606613
Merged to master.
Thanks for working on this @hasnain-db !
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1368097008
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
Review Comment:
The tests are just duplicating what already exists.
Why not simply parameterize it ? This has been done in other suites already (See more below)
For exmaple:
```
class NettyBlockTransferSecuritySuite extends SparkFunSuite ... {
protected def createSparkConf(): SparkConf = new SparkConf()
test("security default off") {
val conf = createSparkConf()
...
}
...
}
class SSLNettyBlockTransferSecuritySuite extends NettyBlockTransferSecuritySuite(" for ssl") {
protected def createSparkConf(): SparkConf = <what is in sparkConfWithSsl>
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1370802419
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
+
+ private def sparkConfWithSsl(): SparkConf = {
+ val conf = new SparkConf()
+ val updatedConfigs = SslSampleConfigs.createDefaultConfigMap()
+ updatedConfigs.entrySet().forEach(entry => conf.set(entry.getKey, entry.getValue))
Review Comment:
@mridulm I took a look but unfortunately I can't -- `TestUtils` is in the main codebase, not the test codebase. `SslSampleConfigs` is test only (See https://github.com/apache/spark/blob/48e207f4a2192d474f2c0f141b984ef0c36a78c3/core/src/main/scala/org/apache/spark/TestUtils.scala#L60-L66)
But I created an `SslTestUtils` and used it everywhere except yarn.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1369598438
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -112,6 +179,19 @@ class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar wi
}
}
+ test("security with aes encryption over ssl") {
Review Comment:
Agree, it is not a case we fail over. Let us keep 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1368992955
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
+
+ private def sparkConfWithSsl(): SparkConf = {
+ val conf = new SparkConf()
+ val updatedConfigs = SslSampleConfigs.createDefaultConfigMap()
+ updatedConfigs.entrySet().forEach(entry => conf.set(entry.getKey, entry.getValue))
Review Comment:
I wanted to do that, but `SparkConf` is in `core` and `SslSampleConfigs` is in `network-common` - this would create a dependency loop since we don't have access to `SparkConf` inside `network-common`. If there is another good place to put this helper I can do it.
##########
core/src/main/scala/org/apache/spark/SparkEnv.scala:
##########
@@ -374,7 +374,12 @@ object SparkEnv extends Logging {
}
val externalShuffleClient = if (conf.get(config.SHUFFLE_SERVICE_ENABLED)) {
- val transConf = SparkTransportConf.fromSparkConf(conf, "shuffle", numUsableCores)
+ val transConf = SparkTransportConf.fromSparkConf(
+ conf,
+ "shuffle",
+ numUsableCores,
+ sslOptions = Some(securityManager.getSSLOptions("rpc"))
Review Comment:
that was private - but I can make it public and address this here and elsewhere. thanks!
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -52,6 +61,17 @@ class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar wi
}
}
+ test("ssl with cloned config") {
+ val conf = sparkConfWithSsl()
+ .set("spark.app.id", "app-id")
+ val conf1 = conf.clone
Review Comment:
this was based on the existing test, should go away once I merge per your suggestion
##########
core/src/test/scala/org/apache/spark/SslExternalShuffleServiceSuite.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.spark
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.config
+import org.apache.spark.network.TransportContext
+import org.apache.spark.network.netty.SparkTransportConf
+import org.apache.spark.network.shuffle.ExternalBlockHandler
+import org.apache.spark.network.ssl.SslSampleConfigs
+
+
+/**
+ * This suite creates an external shuffle server and routes all shuffle fetches through it.
+ * Note that failures in this suite may arise due to changes in Spark that invalidate expectations
+ * set up in `ExternalShuffleBlockHandler`, such as changing the format of shuffle files or how
+ * we hash files into folders.
+ */
+class SslExternalShuffleServiceSuite extends ExternalShuffleServiceSuite {
+
+ override def initializeHandlers(): Unit = {
+ val updatedConfigs = SslSampleConfigs.createDefaultConfigMap()
+ updatedConfigs.entrySet().forEach(entry => conf.set(entry.getKey, entry.getValue))
Review Comment:
will do, thanks! as per the comment elsewhere, I unfortunately can't easily inline this. If you have a pointer to a place I can stuff a helper method to do this, I can do it.
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -112,6 +179,19 @@ class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar wi
}
}
+ test("security with aes encryption over ssl") {
Review Comment:
two things:
1. Once I merge the tests per your suggestion, this will run still.
2. I think it's nice to test that we do not consider this misconfiguration fatal (with that warning being logged) - what do you think?
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
Review Comment:
earlier version of the tests (until we made the change in https://github.com/apache/spark/pull/43238 to allow auth + RPC SSL to work together) needed different behavior. This is a good catch, I'll do this fix.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1368097008
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
Review Comment:
The tests are just duplicating what already exists.
Why not pull the conf creation out ? This has been done in other suites already (See more below)
For exmaple:
```
class NettyBlockTransferSecuritySuite extends SparkFunSuite ... {
protected def createSparkConf(): SparkConf = new SparkConf()
test("security default off") {
val conf = createSparkConf()
...
}
...
}
class SSLNettyBlockTransferSecuritySuite extends NettyBlockTransferSecuritySuite(" for ssl") {
protected def createSparkConf(): SparkConf = <what is in sparkConfWithSsl>
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43387:
URL: https://github.com/apache/spark/pull/43387#issuecomment-1774418178
Can you also fix the conflicts please ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43387:
URL: https://github.com/apache/spark/pull/43387#issuecomment-1768890569
no rush - thanks for looking into this!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43387:
URL: https://github.com/apache/spark/pull/43387#issuecomment-1767696772
A bit swamped this week, will go over this PR later this week.
+CC @JoshRosen if you have the bandwidth to review sooner ! Thanks.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43387:
URL: https://github.com/apache/spark/pull/43387#issuecomment-1778412037
CI looks green. thank you for reviewing @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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1369579551
##########
core/src/main/scala/org/apache/spark/SparkEnv.scala:
##########
@@ -374,7 +374,12 @@ object SparkEnv extends Logging {
}
val externalShuffleClient = if (conf.get(config.SHUFFLE_SERVICE_ENABLED)) {
- val transConf = SparkTransportConf.fromSparkConf(conf, "shuffle", numUsableCores)
+ val transConf = SparkTransportConf.fromSparkConf(
+ conf,
+ "shuffle",
+ numUsableCores,
+ sslOptions = Some(securityManager.getSSLOptions("rpc"))
Review Comment:
you can expose a getter to surface it, though given it is immutable, should not matter to expose the variable
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1369041492
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -52,6 +61,17 @@ class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar wi
}
}
+ test("ssl with cloned config") {
+ val conf = sparkConfWithSsl()
+ .set("spark.app.id", "app-id")
+ val conf1 = conf.clone
Review Comment:
hm, not sure where I hallucinated this test. will remove.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1369589341
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
+
+ private def sparkConfWithSsl(): SparkConf = {
+ val conf = new SparkConf()
+ val updatedConfigs = SslSampleConfigs.createDefaultConfigMap()
+ updatedConfigs.entrySet().forEach(entry => conf.set(entry.getKey, entry.getValue))
Review Comment:
Good point.
Why not `TestUtils` ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1368088945
##########
core/src/test/scala/org/apache/spark/SslExternalShuffleServiceSuite.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.spark
+
+import org.apache.spark.deploy.SparkHadoopUtil
+import org.apache.spark.internal.config
+import org.apache.spark.network.TransportContext
+import org.apache.spark.network.netty.SparkTransportConf
+import org.apache.spark.network.shuffle.ExternalBlockHandler
+import org.apache.spark.network.ssl.SslSampleConfigs
+
+
+/**
+ * This suite creates an external shuffle server and routes all shuffle fetches through it.
+ * Note that failures in this suite may arise due to changes in Spark that invalidate expectations
+ * set up in `ExternalShuffleBlockHandler`, such as changing the format of shuffle files or how
+ * we hash files into folders.
+ */
+class SslExternalShuffleServiceSuite extends ExternalShuffleServiceSuite {
+
+ override def initializeHandlers(): Unit = {
+ val updatedConfigs = SslSampleConfigs.createDefaultConfigMap()
+ updatedConfigs.entrySet().forEach(entry => conf.set(entry.getKey, entry.getValue))
Review Comment:
(Here and elsewhere)
nit: `updatedConfigs` -> `sslConfig` or `defaultSslConfig` or some such
Or better, simply inline it :
```suggestion
SslSampleConfigs.createDefaultConfigMap().entrySet().
forEach(entry => conf.set(entry.getKey, entry.getValue))
```
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -112,6 +179,19 @@ class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar wi
}
}
+ test("security with aes encryption over ssl") {
Review Comment:
This is [not supported](https://github.com/apache/spark/blob/376de8a502fca6b46d7f21560a60024d643144ea/core/src/main/scala/org/apache/spark/SecurityManager.scala#L278) currently, drop this ?
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -52,6 +61,17 @@ class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar wi
}
}
+ test("ssl with cloned config") {
+ val conf = sparkConfWithSsl()
+ .set("spark.app.id", "app-id")
+ val conf1 = conf.clone
Review Comment:
nit: Drop `.clone` and use `conf` for both args ?
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
+
+ private def sparkConfWithSsl(): SparkConf = {
+ val conf = new SparkConf()
+ val updatedConfigs = SslSampleConfigs.createDefaultConfigMap()
+ updatedConfigs.entrySet().forEach(entry => conf.set(entry.getKey, entry.getValue))
Review Comment:
This is a repeated pattern - why not add it to `SslSampleConfigs` ?
`SslSampleConfigs.updateConfWithDefault` or some such ?
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
Review Comment:
The tests are just duplicating what already exists.
Why not simply parameterize it ? This has been done above already
For exmaple:
```
class NettyBlockTransferSecuritySuite extends SparkFunSuite ... {
protected def createSparkConf(): SparkConf = new SparkConf()
test("security default off") {
val conf = createSparkConf()
...
}
...
}
class SSLNettyBlockTransferSecuritySuite extends NettyBlockTransferSecuritySuite(" for ssl") {
protected def createSparkConf(): SparkConf = <what is in sparkConfWithSsl>
}
```
##########
core/src/main/scala/org/apache/spark/SparkEnv.scala:
##########
@@ -374,7 +374,12 @@ object SparkEnv extends Logging {
}
val externalShuffleClient = if (conf.get(config.SHUFFLE_SERVICE_ENABLED)) {
- val transConf = SparkTransportConf.fromSparkConf(conf, "shuffle", numUsableCores)
+ val transConf = SparkTransportConf.fromSparkConf(
+ conf,
+ "shuffle",
+ numUsableCores,
+ sslOptions = Some(securityManager.getSSLOptions("rpc"))
Review Comment:
`SecurityManager` already has a `rpcSSLOptions` - cant we use 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #43387: [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf
URL: https://github.com/apache/spark/pull/43387
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1369579551
##########
core/src/main/scala/org/apache/spark/SparkEnv.scala:
##########
@@ -374,7 +374,12 @@ object SparkEnv extends Logging {
}
val externalShuffleClient = if (conf.get(config.SHUFFLE_SERVICE_ENABLED)) {
- val transConf = SparkTransportConf.fromSparkConf(conf, "shuffle", numUsableCores)
+ val transConf = SparkTransportConf.fromSparkConf(
+ conf,
+ "shuffle",
+ numUsableCores,
+ sslOptions = Some(securityManager.getSSLOptions("rpc"))
Review Comment:
you can expose a getter to surface 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1370995264
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -38,11 +38,20 @@ import org.apache.spark.internal.config.Network
import org.apache.spark.network.{BlockDataManager, BlockTransferService}
import org.apache.spark.network.buffer.{ManagedBuffer, NioManagedBuffer}
import org.apache.spark.network.shuffle.BlockFetchingListener
+import org.apache.spark.network.ssl.SslSampleConfigs
import org.apache.spark.serializer.{JavaSerializer, SerializerManager}
import org.apache.spark.storage.{BlockId, ShuffleBlockId}
import org.apache.spark.util.ThreadUtils
class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar with Matchers {
+
+ private def sparkConfWithSsl(): SparkConf = {
+ val conf = new SparkConf()
+ val updatedConfigs = SslSampleConfigs.createDefaultConfigMap()
+ updatedConfigs.entrySet().forEach(entry => conf.set(entry.getKey, entry.getValue))
Review Comment:
Sounds good
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43387:
URL: https://github.com/apache/spark/pull/43387#discussion_r1369598438
##########
core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala:
##########
@@ -112,6 +179,19 @@ class NettyBlockTransferSecuritySuite extends SparkFunSuite with MockitoSugar wi
}
}
+ test("security with aes encryption over ssl") {
Review Comment:
~Agree, it is not a case we fail over. Let us keep it.~
We return `false` in case both are enabled - so even though users can specify, it wont work and there is no expectation for it to work (in case this changes in future).
We can add a boolean to control which tests to disable in SSL and enable it in the subclass
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43387:
URL: https://github.com/apache/spark/pull/43387#issuecomment-1764790250
test failures looked unrelated, retrying now though.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org