Cert smtp #6089
Cert smtp #6089
Conversation
This draft PR introduces certificate monitoring for SMTP monitors (SMTPS/STARTTLS), similar to other monitor types. What’s implemented so far: Added “Monitor Certificate” checkbox in the SMTP monitor form. Backend checks certificates when certExpiry is true and SMTP is secure (SMTPS or STARTTLS). heartbeat.certExpiryDaysRemaining and validCert are being calculated. Attempted to update TLS info using handleTlsInfo(). Current issue / help needed: Certificate info is not yet showing on the dashboard. Unsure if handleTlsInfo() is being called correctly or if the frontend binding needs adjustment. Need guidance on the best way to store/display TLS info for SMTP monitors, consistent with other monitor types. Goal: Enable users to see SMTP certificate validity and expiry in the dashboard. Make SMTP certificate monitoring consistent with HTTP/HTTPS monitors.
…into cert-smtp
|
@CommanderStorm @fpedrei, |
There was a problem hiding this comment.
I am not a fan of your LLMs output.
Right now the PR introduces a separate SMTP monitoring impelmentations and includes a lot of changes that don’t seem related to the main purpose. In its current state it’s unfortunately not reviewable.
If you can narrow this down to the intended change and make sure it’s consistent, we’ll be able to give it a proper review.
|
Hi, I’ve gone through the changes in this PR and wanted to clarify the intended scope:
Do we want both of these changes, or just the checkbox in the Edit Monitor? I can adjust the PR accordingly once I know what’s needed. Also, for clarity, should the certificate info appear in the dashboard like HTTPS monitors, or is the checkbox alone enough? |
|
I’m fine with including both of those changes. For PRs generated with the help of an LLM, please ensure you’ve done a careful review yourself before opening them. That way, our review time can focus on the meaningful parts, and not get sidetracked by things the model added unnecessarily. |
|
I understand your concern about not duplicating SMTP connection logic in the PR. I ran into an issue where the existing connection doesn’t let me inspect the certificate. |
|
@CommanderStorm can we proceed with this? |
|
While the current approach might work, I am not super excited about this approach. A better approach would be to either:
|
|
@CommanderStorm Thanks for the feedback! I agree that maintainability is important. I’ll explore both approaches—contributing upstream support and hooking into upstream APIs—and decide which aligns best with long-term maintainability. I’ll update you once I’ve evaluated the options. |
|
@Hartejs088 You could file an enhancment request against nodemailer. Perhaps an API could be added to provide the server certificate within |
3 participants
Added SMTP monitoring with SMTPS and STARTTLS support, including certificate checks. It now detects invalid or expired certificates and triggers notifications, all while keeping the code inline, maintainable, and consistent with the existing HTTP/HTTPS monitors. #6030