[SPARK-45545][CORE] Pass SSLOptions wherever we create a SparkTransportConf #43387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

hasnain-db wants to merge 5 commits into apache:master from hasnain-db:spark-tls-integrate-everywhere

Contributor

hasnain-db commented Oct 16, 2023

What changes were proposed in this pull request?

This change ensures that RPC SSL options settings inheritance works properly after #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 #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

Contributor Author

hasnain-db commented Oct 16, 2023

test failures looked unrelated, retrying now though.

github-actions bot added YARN CORE labels

Contributor

mridulm commented Oct 18, 2023

A bit swamped this week, will go over this PR later this week.
+CC @JoshRosen if you have the bandwidth to review sooner ! Thanks.

Contributor Author

hasnain-db commented Oct 18, 2023

no rush - thanks for looking into this!

mridulm reviewed

View reviewed changes

core/src/main/scala/org/apache/spark/SparkEnv.scala Outdated Show resolved Hide resolved

core/src/test/scala/org/apache/spark/SslExternalShuffleServiceSuite.scala Outdated Show resolved Hide resolved

core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala Outdated

Contributor

mridulm Oct 23, 2023

This is a repeated pattern - why not add it to SslSampleConfigs ?
SslSampleConfigs.updateConfWithDefault or some such ?

Contributor Author

hasnain-db Oct 23, 2023

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.

Contributor

mridulm Oct 24, 2023

Good point.
Why not TestUtils ?

Contributor Author

hasnain-db Oct 24, 2023

@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

spark/core/src/main/scala/org/apache/spark/TestUtils.scala

Lines 60 to 66 in 48e207f

    
           /** 
        
            * Utilities for tests. Included in main codebase since it's used by multiple 
        
            * projects. 
        
            * 
        
            * TODO: See if we can move this to the test codebase by specifying 
        
            * test dependencies between projects. 
        
            */

)

But I created an SslTestUtils and used it everywhere except yarn.

Contributor

mridulm Oct 25, 2023

Sounds good

core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala Outdated Show resolved Hide resolved

core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala Outdated Show resolved Hide resolved

core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala Outdated Show resolved Hide resolved

Contributor

mridulm commented Oct 23, 2023

Can you also fix the conflicts please ?


          working

5bb983e

hasnain-db commented

View reviewed changes

Contributor Author

hasnain-db left a comment

will address conflicts, thanks!

core/src/main/scala/org/apache/spark/SparkEnv.scala Outdated Show resolved Hide resolved

core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala Outdated Show resolved Hide resolved

core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala Outdated

Contributor Author

hasnain-db Oct 23, 2023

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/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala Outdated Show resolved Hide resolved

core/src/test/scala/org/apache/spark/network/netty/NettyBlockTransferSecuritySuite.scala Outdated Show resolved Hide resolved

core/src/test/scala/org/apache/spark/SslExternalShuffleServiceSuite.scala Outdated Show resolved Hide resolved


          comments

722dea9

hasnain-db force-pushed the spark-tls-integrate-everywhere branch from 17cb346 to 722dea9 Compare

October 23, 2023 17:49

hasnain-db requested a review from mridulm

October 23, 2023 17:59

hasnain-db added 3 commits

October 23, 2023 12:19


          fix yarn build

979b84b


          comments

50080d0


          fix typo

6000fee

mridulm approved these changes

View reviewed changes

Contributor

mridulm left a comment

Looks good to me, pending CI

mridulm approved these changes

View reviewed changes

Contributor

mridulm left a comment

Looks good to me, pending CI

Contributor Author

hasnain-db commented Oct 25, 2023

CI looks green. thank you for reviewing @mridulm !

mridulm closed this in

08c7bac

Contributor

mridulm commented Oct 25, 2023

Merged to master.
Thanks for working on this @hasnain-db !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels