fix(api): protocol-aware monitor filtering for multi-protocol monitors

Servers with monitor filtering returned incorrect results when monitors
have same names but different protocols (v4/v6). Monitor lookup now
considers both name and IP version to match the correct protocol.

- Add GetMonitorByNameAndIPVersion SQL query with protocol matching
- Update history parameter parsing to use server IP version context
- Fix both /scores/{ip}/log and Grafana endpoints
- Remove unused GetMonitorByName query

Fixes abh/ntppool#264
Reported-by: Anssi Johansson <https://github.com/avijc>

ntpdb/otel.go

@@ -8,7 +8,6 @@ package ntpdb
 
 import (
 	"context"
-	"database/sql"
 
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/attribute"
@@ -82,14 +81,14 @@ func (_d QuerierTxWithTracing) Commit(ctx context.Context) (err error) {
 	return _d.QuerierTx.Commit(ctx)
 }
 
-// GetMonitorByName implements QuerierTx
+// GetMonitorByNameAndIPVersion implements QuerierTx
-func (_d QuerierTxWithTracing) GetMonitorByName(ctx context.Context, tlsName sql.NullString) (m1 Monitor, err error) {
+func (_d QuerierTxWithTracing) GetMonitorByNameAndIPVersion(ctx context.Context, arg GetMonitorByNameAndIPVersionParams) (m1 Monitor, err error) {
-	ctx, _span := otel.Tracer(_d._instance).Start(ctx, "QuerierTx.GetMonitorByName")
+	ctx, _span := otel.Tracer(_d._instance).Start(ctx, "QuerierTx.GetMonitorByNameAndIPVersion")
 	defer func() {
 		if _d._spanDecorator != nil {
 			_d._spanDecorator(_span, map[string]interface{}{
-				"ctx":     ctx,
+				"ctx": ctx,
-				"tlsName": tlsName}, map[string]interface{}{
+				"arg": arg}, map[string]interface{}{
 				"m1":  m1,
 				"err": err})
 		} else if err != nil {
@@ -103,7 +102,7 @@ func (_d QuerierTxWithTracing) GetMonitorByName(ctx context.Context, tlsName sql
 
 		_span.End()
 	}()
-	return _d.QuerierTx.GetMonitorByName(ctx, tlsName)
+	return _d.QuerierTx.GetMonitorByNameAndIPVersion(ctx, arg)
 }
 
 // GetMonitorsByID implements QuerierTx

ntpdb/querier.go

@@ -6,11 +6,10 @@ package ntpdb
 
 import (
 	"context"
-	"database/sql"
 )
 
 type Querier interface {
-	GetMonitorByName(ctx context.Context, tlsName sql.NullString) (Monitor, error)
+	GetMonitorByNameAndIPVersion(ctx context.Context, arg GetMonitorByNameAndIPVersionParams) (Monitor, error)
 	GetMonitorsByID(ctx context.Context, monitorids []uint32) ([]Monitor, error)
 	GetServerByID(ctx context.Context, id uint32) (Server, error)
 	GetServerByIP(ctx context.Context, ip string) (Server, error)

ntpdb/query.sql.go

@@ -12,16 +12,24 @@ import (
 	"time"
 )
 
-const getMonitorByName = `-- name: GetMonitorByName :one
+const getMonitorByNameAndIPVersion = `-- name: GetMonitorByNameAndIPVersion :one
 select id, id_token, type, user_id, account_id, hostname, location, ip, ip_version, tls_name, api_key, status, config, client_version, last_seen, last_submit, created_on, deleted_on, is_current from monitors
 where
-  tls_name like ?
+  tls_name like ? AND
+  ip_version = ? AND
+  is_current = 1 AND
+  status != 'deleted'
   order by id
   limit 1
 `
 
-func (q *Queries) GetMonitorByName(ctx context.Context, tlsName sql.NullString) (Monitor, error) {
+type GetMonitorByNameAndIPVersionParams struct {
-	row := q.db.QueryRowContext(ctx, getMonitorByName, tlsName)
+	TlsName   sql.NullString        `db:"tls_name" json:"tls_name"`
+	IpVersion NullMonitorsIpVersion `db:"ip_version" json:"ip_version"`
+}
+
+func (q *Queries) GetMonitorByNameAndIPVersion(ctx context.Context, arg GetMonitorByNameAndIPVersionParams) (Monitor, error) {
+	row := q.db.QueryRowContext(ctx, getMonitorByNameAndIPVersion, arg.TlsName, arg.IpVersion)
 	var i Monitor
 	err := row.Scan(
 		&i.ID,

query.sql

@@ -47,10 +47,13 @@ select * from servers
 where
   ip = sqlc.arg(ip);
 
--- name: GetMonitorByName :one
+-- name: GetMonitorByNameAndIPVersion :one
 select * from monitors
 where
-  tls_name like sqlc.arg('tls_name')
+  tls_name like sqlc.arg('tls_name') AND
+  ip_version = sqlc.arg('ip_version') AND
+  is_current = 1 AND
+  status != 'deleted'
   order by id
   limit 1;
 

server/grafana.go

@@ -42,11 +42,11 @@ type timeRangeParams struct {
 }
 
 // parseTimeRangeParams parses and validates time range parameters
-func (srv *Server) parseTimeRangeParams(ctx context.Context, c echo.Context) (timeRangeParams, error) {
+func (srv *Server) parseTimeRangeParams(ctx context.Context, c echo.Context, server ntpdb.Server) (timeRangeParams, error) {
 	log := logger.FromContext(ctx)
 
 	// Start with existing parameter parsing logic
-	baseParams, err := srv.getHistoryParameters(ctx, c)
+	baseParams, err := srv.getHistoryParameters(ctx, c, server)
 	if err != nil {
 		return timeRangeParams{}, err
 	}
@@ -291,18 +291,7 @@ func (srv *Server) scoresTimeRange(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusNotFound, "invalid mode - only json supported")
 	}
 
-	// Parse and validate time range parameters
+	// Find and validate server first
-	params, err := srv.parseTimeRangeParams(ctx, c)
-	if err != nil {
-		if he, ok := err.(*echo.HTTPError); ok {
-			return he
-		}
-		log.ErrorContext(ctx, "parse time range parameters", "err", err)
-		span.RecordError(err)
-		return echo.NewHTTPError(http.StatusInternalServerError, "internal error")
-	}
-
-	// Find and validate server
 	server, err := srv.FindServer(ctx, c.Param("server"))
 	if err != nil {
 		log.ErrorContext(ctx, "find server", "err", err)
@@ -321,6 +310,17 @@ func (srv *Server) scoresTimeRange(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusNotFound, "server not found")
 	}
 
+	// Parse and validate time range parameters
+	params, err := srv.parseTimeRangeParams(ctx, c, server)
+	if err != nil {
+		if he, ok := err.(*echo.HTTPError); ok {
+			return he
+		}
+		log.ErrorContext(ctx, "parse time range parameters", "err", err)
+		span.RecordError(err)
+		return echo.NewHTTPError(http.StatusInternalServerError, "internal error")
+	}
+
 	// Query ClickHouse for time range data
 	log.InfoContext(ctx, "executing clickhouse time range query",
 		"server_id", server.ID,
@@ -350,7 +350,9 @@ func (srv *Server) scoresTimeRange(c echo.Context) error {
 		"first_few_ids", func() []uint64 {
 			ids := make([]uint64, 0, 3)
 			for i, ls := range logScores {
-				if i >= 3 { break }
+				if i >= 3 {
+					break
+				}
 				ids = append(ids, ls.ID)
 			}
 			return ids
@@ -424,14 +426,18 @@ func (srv *Server) scoresTimeRange(c echo.Context) error {
 			}
 			first := grafanaResponse[0]
 			return map[string]interface{}{
-				"target": first.Target,
+				"target":        first.Target,
-				"tags": first.Tags,
+				"tags":          first.Tags,
 				"columns_count": len(first.Columns),
-				"values_count": len(first.Values),
+				"values_count":  len(first.Values),
 				"first_few_values": func() [][]interface{} {
-					if len(first.Values) == 0 { return [][]interface{}{} }
+					if len(first.Values) == 0 {
+						return [][]interface{}{}
+					}
 					count := 2
-					if len(first.Values) < count { count = len(first.Values) }
+					if len(first.Values) < count {
+						count = len(first.Values)
+					}
 					return first.Values[:count]
 				}(),
 			}

server/history.go

@@ -69,7 +69,7 @@ type historyParameters struct {
 	fullHistory bool
 }
 
-func (srv *Server) getHistoryParameters(ctx context.Context, c echo.Context) (historyParameters, error) {
+func (srv *Server) getHistoryParameters(ctx context.Context, c echo.Context, server ntpdb.Server) (historyParameters, error) {
 	log := logger.FromContext(ctx)
 
 	p := historyParameters{}
@@ -94,9 +94,18 @@ func (srv *Server) getHistoryParameters(ctx context.Context, c echo.Context) (hi
 	switch monitorParam {
 	case "":
 		name := "recentmedian.scores.ntp.dev"
-		monitor, err := q.GetMonitorByName(ctx, sql.NullString{Valid: true, String: name})
+		var ipVersion ntpdb.NullMonitorsIpVersion
+		if server.IpVersion == ntpdb.ServersIpVersionV4 {
+			ipVersion = ntpdb.NullMonitorsIpVersion{MonitorsIpVersion: ntpdb.MonitorsIpVersionV4, Valid: true}
+		} else {
+			ipVersion = ntpdb.NullMonitorsIpVersion{MonitorsIpVersion: ntpdb.MonitorsIpVersionV6, Valid: true}
+		}
+		monitor, err := q.GetMonitorByNameAndIPVersion(ctx, ntpdb.GetMonitorByNameAndIPVersionParams{
+			TlsName:   sql.NullString{Valid: true, String: name},
+			IpVersion: ipVersion,
+		})
 		if err != nil {
-			log.Warn("could not find monitor", "name", name, "err", err)
+			log.Warn("could not find monitor", "name", name, "ip_version", server.IpVersion, "err", err)
 		}
 		monitorID = monitor.ID
 	case "*":
@@ -113,12 +122,21 @@ func (srv *Server) getHistoryParameters(ctx context.Context, c echo.Context) (hi
 			}
 
 			monitorParam = monitorParam + ".%"
-			monitor, err := q.GetMonitorByName(ctx, sql.NullString{Valid: true, String: monitorParam})
+			var ipVersion ntpdb.NullMonitorsIpVersion
+			if server.IpVersion == ntpdb.ServersIpVersionV4 {
+				ipVersion = ntpdb.NullMonitorsIpVersion{MonitorsIpVersion: ntpdb.MonitorsIpVersionV4, Valid: true}
+			} else {
+				ipVersion = ntpdb.NullMonitorsIpVersion{MonitorsIpVersion: ntpdb.MonitorsIpVersionV6, Valid: true}
+			}
+			monitor, err := q.GetMonitorByNameAndIPVersion(ctx, ntpdb.GetMonitorByNameAndIPVersionParams{
+				TlsName:   sql.NullString{Valid: true, String: monitorParam},
+				IpVersion: ipVersion,
+			})
 			if err != nil {
 				if err == sql.ErrNoRows {
 					return p, echo.NewHTTPError(http.StatusNotFound, "monitor not found").WithInternal(err)
 				}
-				log.WarnContext(ctx, "could not find monitor", "name", monitorParam, "err", err)
+				log.WarnContext(ctx, "could not find monitor", "name", monitorParam, "ip_version", server.IpVersion, "err", err)
 				return p, echo.NewHTTPError(http.StatusNotFound, "monitor not found (sql)")
 			}
 			monitorID = monitor.ID
@@ -127,7 +145,7 @@ func (srv *Server) getHistoryParameters(ctx context.Context, c echo.Context) (hi
 	}
 
 	p.monitorID = int(monitorID)
-	log.DebugContext(ctx, "monitor param", "monitor", monitorID)
+	log.DebugContext(ctx, "monitor param", "monitor", monitorID, "ip_version", server.IpVersion)
 
 	since, _ := strconv.ParseInt(c.QueryParam("since"), 10, 64) // defaults to 0 so don't care if it parses
 	if since > 0 {
@@ -171,16 +189,6 @@ func (srv *Server) history(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusNotFound, "invalid mode")
 	}
 
-	p, err := srv.getHistoryParameters(ctx, c)
-	if err != nil {
-		if he, ok := err.(*echo.HTTPError); ok {
-			return he
-		}
-		log.ErrorContext(ctx, "get history parameters", "err", err)
-		span.RecordError(err)
-		return echo.NewHTTPError(http.StatusInternalServerError, "internal error")
-	}
-
 	server, err := srv.FindServer(ctx, c.Param("server"))
 	if err != nil {
 		log.ErrorContext(ctx, "find server", "err", err)
@@ -199,6 +207,16 @@ func (srv *Server) history(c echo.Context) error {
 		return echo.NewHTTPError(http.StatusNotFound, "server not found")
 	}
 
+	p, err := srv.getHistoryParameters(ctx, c, server)
+	if err != nil {
+		if he, ok := err.(*echo.HTTPError); ok {
+			return he
+		}
+		log.ErrorContext(ctx, "get history parameters", "err", err)
+		span.RecordError(err)
+		return echo.NewHTTPError(http.StatusInternalServerError, "internal error")
+	}
+
 	p.server = server
 
 	var history *logscores.LogScoreHistory
@@ -456,4 +474,3 @@ func setHistoryCacheControl(c echo.Context, history *logscores.LogScoreHistory)
 		}
 	}
 }
-