Remove CompletableFuture from SSLConfigurationReloader #93132
Conversation
The `SSLConfigurationReloader` uses a `CompletableFuture` to receive an as-yet-unconstructed instance of the `SSLService`, but in all cases the `SSLService` is in fact already constructed so we can just directly pass it into the constructor.
DaveCTurner
added
>non-issue
:Security/Security
|
Pinging @elastic/es-security (Team:Security) |
|
@DaveCTurner I think this could potentially re-introduce the issue fixed by #54999 (see also #54867). In summary, the use of the future isn't because SslService being unavailable but to ensure update to relevant SSL files are always handled. |
|
I see. This is pretty trappy, and doesn't seem to be covered by a test? Even a comment would have helped. Ok, I'll do something different here. |
|
Thanks @ywangd, this is ready for another look |
There was a problem hiding this comment.
LGTM
This is pretty trappy, and doesn't seem to be covered by a test? Even a comment would have helped.
Yeah I agree a comment would have been good. It does have a test. But due to the racing nature and how close the two statements are at each other (with no place to inject something in between), it happens only very rarely so that the test is not guaranteed to fail when the main code changes. It's more of a best effort.
|
👍 I see, thanks Yang |
DaveCTurner
deleted the
2023-01-23-SSLConfigurationReloader-CompletableFuture
branch
Replaced with a
PlainActionFuture, since it is never completed exceptionally.