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.

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

Force cert reload on a daily basis. #14535

Merged
merged 5 commits into from
Nov 27, 2024

Conversation

itachiunited
Copy link
Contributor

Forcing SSL reload once a day using a scheduled thread. This is to prevent any discrepancy due to FileWatcher in reloading the certs.

There has been scenarios where the FileWatcher is not reliable in detecting changes and reloading the certs as expected when changed. The certs were reloaded successfully in few components but it was never detected and run in others. This will result in few components with stale certificates. In order to prevent this issue, we are adding a new scheduled thread which will run once a day and reload the certs.

…event any discrepancy due to FileWatcher in reloading the certs.
Refactored the reloadSSL method to be used by both FileWatcher and Scheduled Thread.
Made the reloadSSL synchronized so that it is thread safe.
Refactored the reloadSSL method to be used by both FileWatcher and Scheduled Thread.
Made the reloadSSL synchronized so that it is thread safe.
Copy link
Contributor

@soumitra-st soumitra-st left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

High level question: Is it watcher based triggering right now? Seems reloadSslFactoryWhenFileStoreChanges() is also executed in a separate executor, is it possible to have a single entry point which monitors both triggering condition and interval?

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 12 lines in your changes missing coverage. Please review.

Project coverage is 63.91%. Comparing base (59551e4) to head (660cc3b).
Report is 1390 commits behind head on master.

Files with missing lines Patch % Lines
...ache/pinot/common/utils/tls/RenewableTlsUtils.java 55.55% 12 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14535      +/-   ##
============================================
+ Coverage     61.75%   63.91%   +2.16%     
- Complexity      207     1569    +1362     
============================================
  Files          2436     2675     +239     
  Lines        133233   146985   +13752     
  Branches      20636    22559    +1923     
============================================
+ Hits          82274    93952   +11678     
- Misses        44911    46105    +1194     
- Partials       6048     6928     +880     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.90% <55.55%> (+2.19%) ⬆️
java-21 63.78% <55.55%> (+2.16%) ⬆️
skip-bytebuffers-false 63.91% <55.55%> (+2.16%) ⬆️
skip-bytebuffers-true 63.76% <55.55%> (+36.03%) ⬆️
temurin 63.91% <55.55%> (+2.16%) ⬆️
unittests 63.91% <55.55%> (+2.16%) ⬆️
unittests1 55.64% <55.55%> (+8.74%) ⬆️
unittests2 34.55% <0.00%> (+6.82%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@itachiunited
Copy link
Contributor Author

High level question: Is it watcher based triggering right now? Seems reloadSslFactoryWhenFileStoreChanges() is also executed in a separate executor, is it possible to have a single entry point which monitors both triggering condition and interval?

@Jackie-Jiang - Yes, the current flow is watcher based. We want to keep both separate since this is a backup to make sure that the certs are reloaded everyday. FileWatcher thread just has to run once after restart to set up the FileWatcher and the new executor is to make sure that it runs once a day to reload the SSL.

@Jackie-Jiang Jackie-Jiang merged commit 34b43a3 into apache:master Nov 27, 2024
21 checks passed
yashmayya pushed a commit to yashmayya/pinot that referenced this pull request Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants