Skip to content
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

[SPARK-45544][CORE] Integrate SSL support into TransportContext #43541

Closed
wants to merge 7 commits into from
Closed

[SPARK-45544][CORE] Integrate SSL support into TransportContext #43541

wants to merge 7 commits into from

Conversation

hasnain-db
Copy link
Contributor

What changes were proposed in this pull request?

This integrates SSL support into TransportContext and related modules so that the RPC SSL functionality can work when properly configured.

Why are the changes needed?

This is needed in order to support SSL for RPC connections.

Does this PR introduce any user-facing change?

No

How was this patch tested?

CI

Ran the following tests:

build/sbt -P yarn
> project network-common
> testOnly
> project network-shuffle
> testOnly
> project core
> testOnly *Ssl*
> project yarn
> testOnly org.apache.spark.network.yarn.SslYarnShuffleServiceWithRocksDBBackendSuite

I verified traffic was encrypted using TLS using two mechanisms:

  • Enabled trace level logging for Netty and JDK SSL and saw logs confirming TLS handshakes were happening
  • I ran wireshark on my machine and snooped on traffic while sending queries shuffling a fixed string. Without any encryption, I could find that string in the network traffic. With this encryption enabled, that string did not show up, and wireshark logs confirmed a TLS handshake was happening.

Was this patch authored or co-authored using generative AI tooling?

No

@hasnain-db
Copy link
Contributor Author

cc @mridulm - this should be the final functionality PR for the SSL support (the only other remaining PR is the docs one).

mridulm
mridulm reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments, mostly looks good.
Thanks for working on this @hasnain-db !

common/network-common/src/main/java/org/apache/spark/network/TransportContext.java Outdated Show resolved Hide resolved
common/network-common/src/main/java/org/apache/spark/network/TransportContext.java Outdated Show resolved Hide resolved
common/network-common/src/main/java/org/apache/spark/network/TransportContext.java Show resolved Hide resolved
common/network-common/src/main/java/org/apache/spark/network/TransportContext.java Outdated Show resolved Hide resolved
common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java Outdated Show resolved Hide resolved
.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/ShuffleTransportContext.java Outdated Show resolved Hide resolved
...-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleIntegrationSuite.java Outdated Show resolved Hide resolved
...-shuffle/src/test/java/org/apache/spark/network/shuffle/ExternalShuffleIntegrationSuite.java Outdated Show resolved Hide resolved
...ork-shuffle/src/test/java/org/apache/spark/network/shuffle/ShuffleTransportContextSuite.java Outdated Show resolved Hide resolved
...ork-shuffle/src/test/java/org/apache/spark/network/shuffle/ShuffleTransportContextSuite.java Outdated Show resolved Hide resolved
mridulm
mridulm reviewed Oct 29, 2023
View reviewed changes
common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java Outdated
}
});
if (!future.await(conf.connectionTimeoutMs())) {
logger.info("failed to connect to " + address + " within connection timeout");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ: Will we have two log messages for the same failure ? Here and in operationComplete ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so - will remove this.

mridulm
mridulm reviewed Oct 29, 2023
View reviewed changes
Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor comments, mostly looks good.

@mridulm
Copy link
Contributor

mridulm commented Oct 30, 2023

The test WorkerDecommissionSuite.'verify a running task with all workers decommissioned succeeds', but this is unrelated to current pr.
Will merge to master.

@mridulm mridulm closed this in 884f6f7 Oct 30, 2023
@mridulm
Copy link
Contributor

mridulm commented Oct 30, 2023

Merged to master.
Thanks for working on this @hasnain-db !
And thanks for the review @HyukjinKwon :-)

@mridulm
Copy link
Contributor

mridulm commented Oct 30, 2023

@hasnain-db , can you link all the individual jira's to the original jira please ?
That will help link and identify all the relevant PR's for this feature.

@hasnain-db
Copy link
Contributor Author

@mridulm I see them all linked as child JIRAs on https://issues.apache.org/jira/browse/SPARK-44937 -- let's continue the conversation there if you do not see them?

@mridulm
Copy link
Contributor

mridulm commented Oct 30, 2023

Sigh, it does not show up if you are not logged in ... did not realize I had gotten logged out of jira.
It does show up there, thanks.

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

@mridulm mridulm mridulm left review comments

@HyukjinKwon HyukjinKwon Awaiting requested review from HyukjinKwon

Assignees
No one assigned
Labels
CORE YARN
Projects
None yet
Milestone
No milestone
3 participants
@hasnain-db @mridulm @HyukjinKwon