-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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.
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
[SPARK-45426][CORE] Add support for a ReloadingX509TrustManager #43249
Conversation
...etwork-common/src/test/java/org/apache/spark/network/ssl/ReloadingX509TrustManagerSuite.java
Show resolved
Hide resolved
common/network-common/src/main/java/org/apache/spark/network/ssl/ReloadingX509TrustManager.java
Outdated
Show resolved
Hide resolved
common/network-common/src/main/java/org/apache/spark/network/ssl/ReloadingX509TrustManager.java
Show resolved
Hide resolved
common/network-common/src/main/java/org/apache/spark/network/ssl/ReloadingX509TrustManager.java
Show resolved
Hide resolved
common/network-common/src/main/java/org/apache/spark/network/ssl/ReloadingX509TrustManager.java
Outdated
Show resolved
Hide resolved
common/network-common/src/main/java/org/apache/spark/network/ssl/ReloadingX509TrustManager.java
Outdated
Show resolved
Hide resolved
assertEquals(1, tm.getAcceptedIssuers().length); | ||
|
||
// Wait so that the file modification time is different | ||
Thread.sleep((tm.getReloadInterval() + 1000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sleep
can be spuriously woken up - which can make this test flakey.
Ensure it has actually slept for the requested time (See SystemClock.waitTillTime
for an example).
...etwork-common/src/test/java/org/apache/spark/network/ssl/ReloadingX509TrustManagerSuite.java
Outdated
Show resolved
Hide resolved
assertEquals(10, tm.getReloadInterval()); | ||
|
||
// Wait so that the file modification time is different | ||
Thread.sleep((tm.getReloadInterval() + 200)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of relying on timing, and potentially becoming flakey (in case reloader was delayed), I would suggest to add a counter to track the number of times reload happened - and wait on that to change.
This also minimizes the arbitrary sleep values in various tests.
* @throws Exception | ||
*/ | ||
@Test | ||
public void testReload() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on this test should apply to others below as well.
...etwork-common/src/test/java/org/apache/spark/network/ssl/ReloadingX509TrustManagerSuite.java
Show resolved
Hide resolved
@mridulm thanks for the review here. I copied this code as is from Hadoop with no modifications, with the idea that that's well tested and used and we can avoid any risks from changing implementations. Happy to change the implementation here (and potentially upstream), but wanted to double check that that's what you prefer instead of keeping compatibility with a tested implementation. Please let me know. |
If this is to be part of Apache Spark codebase, divergence is to be expected as code evolves and we add functionality relevant to Spark. |
Thanks! Will address the comments and take a more detailed look in that case. |
612f15e
to
b7dac1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just couple of minor comments, we can merge once addressed.
...etwork-common/src/test/java/org/apache/spark/network/ssl/ReloadingX509TrustManagerSuite.java
Show resolved
Hide resolved
...etwork-common/src/test/java/org/apache/spark/network/ssl/ReloadingX509TrustManagerSuite.java
Outdated
Show resolved
Hide resolved
...etwork-common/src/test/java/org/apache/spark/network/ssl/ReloadingX509TrustManagerSuite.java
Show resolved
Hide resolved
The test failure does not look relevant, but can you retrigger tests please ? |
@mridulm done - I can re-request review once tests pass (might take a try or 2) |
The test failure is unrelated. |
What changes were proposed in this pull request?
This adds in support for trust store reloading, mirroring the Hadoop implementation (see source comments for a link). I believe reusing the existing code instead of adding a dependency is fine license wise (see https://github.com/apache/spark/pull/42685/files#r1333667328)
Why are the changes needed?
This helps us refresh trust stores without needing downtime
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added unit tests (also copied from upstream)
The rest of the changes and integration were tested as part of #42685
Was this patch authored or co-authored using generative AI tooling?
No