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

Add a warning for node certificate expiring within 30 days (DB-138) #3855

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

dhingrak
Copy link
Contributor

Fixed: Log a warning when certificates are close to expiry.

Fixes https://linear.app/eventstore/issue/DB-138/log-a-warning-when-certificates-are-close-to-expiry

The goal of this PR is to add a warning daily if the node certificate is going to expire within 30 days.

@linear
Copy link

linear bot commented May 30, 2023

DB-138 Log a warning when certificates are close to expiry

  • Start warning when the certificate is 1 month from expiry
  • Log the warning daily

@dhingrak dhingrak self-assigned this May 30, 2023
@dhingrak dhingrak force-pushed the kunal/notify-certificates-about-to-expire branch 2 times, most recently from eb65011 to a849d82 Compare June 13, 2023 14:12
@dhingrak dhingrak changed the title Add a warning for node certificate expiring within 30 days Add a warning for node certificate expiring within 30 days (DB-138) Jun 13, 2023
@dhingrak dhingrak force-pushed the kunal/notify-certificates-about-to-expire branch from a849d82 to 2e7d332 Compare June 15, 2023 17:42
@hayley-jean hayley-jean requested review from timothycoleman and removed request for shaan1337 June 16, 2023 11:10
src/EventStore.Core/ClusterVNode.cs Outdated Show resolved Hide resolved
src/EventStore.Core/ClusterVNode.cs Outdated Show resolved Hide resolved
@dhingrak dhingrak force-pushed the kunal/notify-certificates-about-to-expire branch 2 times, most recently from 46a3833 to 02a608b Compare June 19, 2023 16:14
public VerifyNodeCertificateExpiry() {
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

I think it'd be nice to pass _certificateSelector into a new class that is responsible for monitoring the expiry rather than adding the logic to ClusterVNode. _certificateSelector is already passed elsewhere

We can store the ScheduleMessage returned by TimerMessage.Schedule.Create in a member variable and reuse it each time to save on these allocations that will end up in gen2 because they're long lived

@dhingrak dhingrak force-pushed the kunal/notify-certificates-about-to-expire branch from 02a608b to c038bd7 Compare June 21, 2023 20:44
@timothycoleman timothycoleman self-requested a review June 22, 2023 11:10
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

Ah sorry I should have been a bit clearer. The recent changes are a step in the right direction but I meant slightly more. We could have the CertificateExpiryMonitor be a component that communicates with messages (i.e. Handle the VerifyNodeCertificateExpiry and publish the _nodeCertificateExpirySchedule), so the ClusterVNode would wire it up and start it like it does the other major components without having to worry about calling it again after

@dhingrak dhingrak force-pushed the kunal/notify-certificates-about-to-expire branch 3 times, most recently from ec78f55 to e544b10 Compare June 22, 2023 20:48
Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

great thats look good

it would be cool if schedule message construction could be left to the monitor itself as well, then the whole implementation of how it works would be encapsulated away from the ClusterVNode. Then we'd be free to change the implementation of the monitor without it having a knock on effect elsewhere

it would likely make sense to have the Monitor Handle SystemMessage.SystemStart to achieve this

and then we'll be in a good position to write some unit tests for the monitor. if you find that the DateTime.Now call in the monitor is awkward in the test, you can do something like pass a Func to the monitor (which would be () => DateTime.Now in real life) but allows you to control time however you want in the test

one other thing, the monitor can receive the mainbus as a IPublisher rather than IQueuedHandler (all we are going to do with it is publish), and the member variable needn't be called _mainQueue because the monitor doesn't mind what queue it really is (which is nice because it means we can change the details of the ClusterVNode if we want without having a knock on effect on the monitor) so we can just call it _publisher or _output

@timothycoleman
Copy link
Contributor

come to think of it the easiest thing to do would probably be to pass a

Func<DateTime?> getCertificateExpiry instead of Func<X509Certificate2> certificateSelector

then the tests will be easy even using DateTIme.now

@dhingrak dhingrak force-pushed the kunal/notify-certificates-about-to-expire branch 2 times, most recently from e4b826e to 4d5c287 Compare June 26, 2023 21:24
@dhingrak dhingrak force-pushed the kunal/notify-certificates-about-to-expire branch from 4d5c287 to 3e16399 Compare June 27, 2023 14:10
@dhingrak
Copy link
Contributor Author

Thank you so much Tim for your suggestions.

Copy link
Contributor

@timothycoleman timothycoleman left a comment

Choose a reason for hiding this comment

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

Great, thanks!


if (timeUntilExpiry <= _warningThreshold) {
_logger.Warning(
"Certificates are going to expire in {daysUntilExpiry:N1} days",
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, I'm happy to merge as-is but I think it would be better to only show the remaining days as whole numbers:

Suggested change
"Certificates are going to expire in {daysUntilExpiry:N1} days",
"Certificates are going to expire in {daysUntilExpiry:N0} days",

Copy link
Contributor

Choose a reason for hiding this comment

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

(for posterity, it can go either way but seeing 0.4 days is slightly more informative than 0 days)

@timothycoleman timothycoleman merged commit 8174199 into master Jul 3, 2023
6 of 8 checks passed
@timothycoleman timothycoleman deleted the kunal/notify-certificates-about-to-expire branch July 3, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants