Skip to content

feat: add NTP monitor type for network time protocol monitoring#7214

Open
henricook wants to merge 4 commits intolouislam:masterfrom
henricook:feature/ntp-monitor
Open

feat: add NTP monitor type for network time protocol monitoring#7214
henricook wants to merge 4 commits intolouislam:masterfrom
henricook:feature/ntp-monitor

Conversation

@henricook
Copy link
Copy Markdown

@henricook henricook commented Mar 27, 2026

Resolves #5028, builds on the great work by @Jas0n99 on their fork

Summary

In this pull request, the following changes are made:

  • Adds a new monitor type that queries NTP servers via UDP

  • Parses the response per RFC 5905

  • Checks stratum, time offset, and root dispersion against configurable thresholds.

  • Resolves NTP check #5028

Checklist

  • ⚠️ If there are Breaking change (a fix or feature that alters existing functionality in a way that could cause issues) I have called them out
  • 🧠 I have disclosed any use of LLMs/AI in this contribution and reviewed all generated content.
    I understand that I am responsible for and able to explain every line of code I submit.
  • 🔍 Any UI changes adhere to visual style of this project.
  • 🛠️ I have self-reviewed and self-tested my code to ensure it works as expected.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • [x]🤖 I added or updated automated tests where appropriate.
  • 📄 Documentation updates are included (if applicable).
  • 🧰 Dependency updates are listed and explained.
  • ⚠️ CI passes and is green.

Screenshots for Visual Changes

Event Screenshot
Configuration NTP Config
NTP Thresholds NTP Thresholds
UP NTP Up

@github-actions
Copy link
Copy Markdown
Contributor

Hello and thanks for lending a paw to Uptime Kuma! 🐻👋
As this is your first contribution, please be sure to check out our Pull Request guidelines.
In particular: - Mark your PR as Draft while you’re still making changes - Mark it as Ready for review once it’s fully ready
If you have any design or process questions, feel free to ask them right here in this pull request - unclear documentation is a bug too.

@henricook henricook force-pushed the feature/ntp-monitor branch from fdfbb66 to 91bc4bf Compare March 27, 2026 11:38
Adds a new monitor type that queries NTP servers via UDP, parses the
response per RFC 5905, and checks stratum, time offset, and root
dispersion against configurable thresholds. Includes knex migration,
full backend test suite, and frontend UI in the Specific Monitor Type
category.

Resolves louislam#5028
@henricook henricook force-pushed the feature/ntp-monitor branch from 91bc4bf to 2f7ae74 Compare March 27, 2026 17:54
@henricook henricook marked this pull request as ready for review March 27, 2026 18:15
@github-actions github-actions bot added the pr:needs review this PR needs a review by maintainers or other community members label Mar 27, 2026
@henricook
Copy link
Copy Markdown
Author

Ready for review! My first contribution, please be gentle 🙏🏻

@smarshall-rightside
Copy link
Copy Markdown

I think there are 2 real issues here:

  1. ntp is missing from the hostname validation allowlist in isInputValid() (the ["dns", "port", ... "snmp"] list).
    So NTP hostnames skip the same validation other hostname-based monitors get.

  2. In queryNTP() socket family is chosen with hostname.includes(":") ? "udp6" : "udp4".
    That works for literal IPv6 addresses, but not for hostnames that only resolve to AAAA. Those get forced to udp4 and can fail lookup/send.

Could we add ntp to the hostname validation list, and make family selection based on actual DNS result (or fallback udp4 -> udp6)?

@henricook
Copy link
Copy Markdown
Author

Thanks for your time and thoughtful comments @smarshall-rightside . I've added a DNS lookup to gauge ipv4 Vs. ipv6 - there's no precedent for this in the code base but that's because I think I'm dealing with UDP here and not many of the existing monitors have had to think about that. I'd love to know what you think.

Other changes are just tidying up my use of dgram.createSocket

@smarshall-rightside
Copy link
Copy Markdown

smarshall-rightside commented Apr 6, 2026

Nice addition overall @henricook, thanks for taking the feedback on board. I checked the latest changes and I think the DNS lookup + socket selection is the right call here for UDP/NTP, and the socket lifecycle cleanup looks much safer/cleaner too. Nice addition overall. I'd certainly use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:needs review this PR needs a review by maintainers or other community members

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NTP check

2 participants