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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
