From dd699b706f2a390e2000c99447b5958bf208fd69 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 04:03:48 +0400 Subject: [PATCH] mavpoll refuses the flags that would kill it later (V-611) Three ways a mavpoll process died or lied after start: - -interval 0 panicked time.NewTicker on the first tick. - -timeout 0 is 'no deadline' to http.Client, so one wedged source stalls every source behind it forever. - -wg-cmd '' indexed field 0 of an empty slice in pollWg. All three are now refused in run(), where the operator reads the message, and pollWg guards its own command as well. A body that hits maxBodyBytes was silently truncated. A cut kuma page parses cleanly up to the cut and every monitor past it looks deleted, so the poller would write 'unknown' over live services and the down-rule would go quiet. Read one byte past the cap and refuse. wg's stderr was dropped by Output(), leaving 'exit status 1' in the log where the real cause is a missing CAP_NET_ADMIN or a bad interface. --- cmd/mavpoll/main.go | 35 +++++++++++++++++++++++- cmd/mavpoll/main_test.go | 57 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/cmd/mavpoll/main.go b/cmd/mavpoll/main.go index d9f299c..212e49c 100644 --- a/cmd/mavpoll/main.go +++ b/cmd/mavpoll/main.go @@ -77,6 +77,21 @@ func run(args []string) error { if *netdataURL == "" && *kumaURL == "" && *wgIface == "" && *zenTokenFile == "" { return fmt.Errorf("nothing to poll: set -netdata, -kuma, -wg and/or -zenmoney-token-file") } + // A bad duration or an empty -wg-cmd used to get past start and kill the + // poller on the first tick — time.NewTicker panics on a non-positive + // interval, and pollWg indexed field 0 of an empty command. A zero -timeout + // is worse than a crash: http.Client reads it as "no deadline", so one + // wedged source stalls every other source behind it forever. Refuse all + // three here, where the operator sees the message. + if *interval <= 0 { + return fmt.Errorf("-interval must be positive, got %s", *interval) + } + if *timeout <= 0 { + return fmt.Errorf("-timeout must be positive, got %s", *timeout) + } + if *wgIface != "" && strings.TrimSpace(*wgCmd) == "" { + return fmt.Errorf("-wg-cmd is empty but -wg is set") + } zen, err := newZenClient(*zenTokenFile, *zenURL, *timeout) if err != nil { @@ -283,9 +298,19 @@ const ( // `wg show` needs CAP_NET_ADMIN; run mavpoll with the cap or set -wg-cmd "sudo wg". func (p *poller) pollWg(ctx context.Context) error { fields := strings.Fields(p.wgCmd) + if len(fields) == 0 { + return fmt.Errorf("wg command is empty") + } args := append(fields[1:], "show", p.wgIface, "latest-handshakes") out, err := exec.CommandContext(ctx, fields[0], args...).Output() if err != nil { + // wg says why it refused on stderr — usually a missing CAP_NET_ADMIN or + // an interface that does not exist. Output() drops that, leaving a log + // line that reads "exit status 1" and diagnoses nothing. + var ee *exec.ExitError + if errors.As(err, &ee) && len(ee.Stderr) > 0 { + return fmt.Errorf("run %s: %w: %s", p.wgCmd, err, strings.TrimSpace(string(ee.Stderr))) + } return fmt.Errorf("run %s: %w", p.wgCmd, err) } maxTs := parseMaxHandshake(string(out)) @@ -552,6 +577,11 @@ func isNoFact(err error) bool { // maxBodyBytes caps what a source can make the poller hold. Kuma's whole // metrics page is a few hundred kilobytes, so 4 MiB is slack, not a budget. +// +// Hitting the cap is an error, not a shorter body. A truncated kuma page parses +// cleanly right up to the cut, and every monitor past it reads as deleted — the +// poller would write "unknown" over live services and the down-rule would go +// quiet. Reading one byte past the cap is how we tell full from truncated. const maxBodyBytes = 4 << 20 func (p *poller) get(ctx context.Context, url, basicUser string) ([]byte, error) { @@ -567,12 +597,15 @@ func (p *poller) get(ctx context.Context, url, basicUser string) ([]byte, error) return nil, err } defer resp.Body.Close() - body, err := io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes)) + body, err := io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes+1)) if err != nil { return nil, err } if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("GET %s: %s", url, resp.Status) } + if len(body) > maxBodyBytes { + return nil, fmt.Errorf("GET %s: body over %d bytes", url, maxBodyBytes) + } return body, nil } diff --git a/cmd/mavpoll/main_test.go b/cmd/mavpoll/main_test.go index 229dd19..407c0f1 100644 --- a/cmd/mavpoll/main_test.go +++ b/cmd/mavpoll/main_test.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "io" "net/http" "net/http/httptest" "os" @@ -212,3 +213,59 @@ func TestRunRequiresSomethingToPoll(t *testing.T) { t.Errorf("err = %v, want a 'nothing to poll' refusal", err) } } + +// A flag value that would kill the poller later is refused at start, before it +// dials core: a non-positive interval panics time.NewTicker on the first tick, a +// zero timeout means http.Client waits forever, and an empty wg command used to +// index field 0 of an empty slice. +func TestRunRefusesFlagsThatCrashLater(t *testing.T) { + cases := []struct { + name string + args []string + want string + }{ + {"zero interval", []string{"-interval", "0"}, "-interval must be positive"}, + {"negative interval", []string{"-interval", "-5s"}, "-interval must be positive"}, + {"zero timeout", []string{"-timeout", "0"}, "-timeout must be positive"}, + {"empty wg command", []string{"-wg", "wg0", "-wg-cmd", " "}, "-wg-cmd is empty"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + args := append([]string{"-socket", "/tmp/nope.sock"}, c.args...) + err := run(args) + if err == nil || !strings.Contains(err.Error(), c.want) { + t.Errorf("err = %v, want %q", err, c.want) + } + }) + } +} + +// pollWg refuses an empty command rather than panicking on fields[0]. +func TestPollWgEmptyCommand(t *testing.T) { + p := &poller{core: &factCore{}, wgIface: "wg0", wgCmd: ""} + if err := p.pollWg(context.Background()); err == nil { + t.Error("want an error, got a poll that ran something") + } +} + +// A body at the cap is a truncated body, and a truncated kuma page reads as +// "every monitor past the cut was deleted". Refuse it instead of parsing it. +func TestGetRefusesTruncatedBody(t *testing.T) { + big := strings.Repeat("x", maxBodyBytes+64) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + io.WriteString(w, big) + })) + defer srv.Close() + p := &poller{http: srv.Client()} + if _, err := p.get(context.Background(), srv.URL, ""); err == nil { + t.Error("want an over-size refusal, got a silently truncated body") + } + small := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + io.WriteString(w, "ok") + })) + defer small.Close() + body, err := p.get(context.Background(), small.URL, "") + if err != nil || string(body) != "ok" { + t.Errorf("get = %q, %v; want the whole small body", body, err) + } +}