From 41f8bfb3529067b739fa70cb87a6f6195dcfc0bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Otto=20Kr=C3=B6pke?= Date: Fri, 16 Oct 2026 09:11:02 +0200 Subject: [PATCH] cmd: validate telemetry.path and scrape.timeout-margin, warn on unknown disabled collectors (#2733) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jan-Otto Kröpke --- README.md | 4 +- UPGRADE.md | 5 +- cmd/windows_exporter/main.go | 134 +++++++++++++++++++++++++++--- cmd/windows_exporter/main_test.go | 125 ++++++++++++++++++++++++++++ internal/config/config_test.go | 25 ++++-- 5 files changed, 268 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 79bbdf49..1e707409 100644 --- a/README.md +++ b/README.md @@ -90,9 +90,9 @@ windows_exporter accepts flags to configure certain behaviours. The ones configu | Flag | Description | Default value | |---------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------| | `--web.listen-address` | host:port for exporter. | `:9182` | -| `--telemetry.path` | URL path for surfacing collected metrics. | `/metrics` | +| `--telemetry.path` | URL path for surfacing collected metrics. Must start with `/`, must not be `/health` and, with `--debug.enabled`, must not be a `/debug/pprof/` route. | `/metrics` | | `--collectors.enabled` | Comma-separated list of collectors to use. Use `[defaults]` as a placeholder which gets expanded containing all the collectors enabled by default. | `[defaults]` | -| `--scrape.timeout-margin` | Seconds to subtract from the timeout allowed by the client. The margin takes at most half of the client's timeout. Tune to allow for overhead or high loads. | `0.5` | +| `--scrape.timeout-margin` | Seconds to subtract from the timeout allowed by the client. The margin takes at most half of the client's timeout. Tune to allow for overhead or high loads. Must not be negative. | `0.5` | | `--scrape.stdout` | Collect metrics once and write them to stdout, then exit. Logs go to stderr unless `--log.file` is set. PDH rate counters cover only a short interval on the first scrape after startup. | `false` | | `--web.config.file` | A [web config][web_config] for setting up TLS and Auth | None | | `--config.file` | [Using a config file](#using-a-configuration-file) from path | None | diff --git a/UPGRADE.md b/UPGRADE.md index 75f8fda4..bc5568d7 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -12,7 +12,7 @@ Most installations need no changes. Go through the checklist below and read the | You enable the `filetime` collector | Switch to the `file` collector | [filetime collector removed](#filetime-collector-removed) | | You enable the `thermalzone` collector | Switch to a `performancecounter` object | [thermalzone collector removed](#thermalzone-collector-removed) | | You call the `/version` endpoint | Use `windows_exporter_build_info` instead | [/version endpoint removed](#version-endpoint-removed) | -| You use a configuration file, or set `--process.priority` or `--process.memory-limit` | Check that the exporter still starts | [Stricter validation](#stricter-configuration-validation) | +| You use a configuration file, or set `--process.priority`, `--process.memory-limit`, `--scrape.timeout-margin`, `--telemetry.path` or `--collectors.disabled` | Check that the exporter still starts | [Stricter validation](#stricter-configuration-validation) | | You use the `process` collector | Remove `counter-version`, check the `owner` label | [process collector](#process-collector) | | You use the `container` collector | Replace `containerd-state-dir` with `cri-endpoint` | [container collector](#container-collector) | | You query `windows_gpu_info` | Replace the `phys` label with `device_number` | [GPU collector](#gpu-collector) | @@ -106,6 +106,9 @@ windows_exporter 0.32 rejects configuration it used to ignore. The exporter stop - **Sub-collector names.** `--collector.dfsr.sources-enabled`, `--collector.dhcp.enabled`, `--collector.mscluster.enabled` and `--collector.tcp.enabled` reject unknown sub-collector names. Typos used to be ignored, which disabled that sub-collector without a warning. - **Process priority.** An unknown `--process.priority` value stops the exporter. It used to fall back to `normal`. - **Memory limit.** `--process.memory-limit=0` now disables the limit, as documented. In 0.31 it set a soft limit of zero bytes, which made the Go runtime collect garbage continuously. Negative values stop the exporter. +- **Scrape timeout margin.** A negative `--scrape.timeout-margin` stops the exporter. 0.31 treated it as `0`. +- **Telemetry path.** `--telemetry.path` must start with `/`, must not be `/health` and, with `--debug.enabled`, must not be one of the `/debug/pprof/` routes. 0.31 accepted such a path and then crashed while starting the HTTP server. +- **Disabled collectors.** Unknown names in `--collectors.disabled` are logged as a warning and otherwise ignored, so a configuration that still disables a removed collector keeps working. Spaces around the names in `--collectors.enabled` and `--collectors.disabled` are ignored: `cpu, net` now names `net`. In 0.31, `--collectors.disabled=cpu, net` left `net` enabled and `--collectors.enabled=cpu, net` stopped the exporter with `unknown collector net`. - **update collector.** In a configuration file, the key for `--collector.update.scrape-interval` is `scrape-interval`: ```yaml diff --git a/cmd/windows_exporter/main.go b/cmd/windows_exporter/main.go index 8a4ac637..92c3ea5b 100644 --- a/cmd/windows_exporter/main.go +++ b/cmd/windows_exporter/main.go @@ -65,6 +65,9 @@ const ( writeTimeoutMargin = 30 * time.Second ) +// healthPath is the URL path of the health endpoint. +const healthPath = "/health" + func main() { // os.Kill can't be caught. On Windows, Go delivers console close, logoff and shutdown events as SIGTERM. //nolint:forbidigo // os/signal only accepts syscall.Signal values, not windows.Signal. @@ -177,6 +180,20 @@ func run(ctx context.Context, args []string) int { return 1 } + if err := validateTimeoutMargin(*timeoutMargin); err != nil { + logStartupError(ctx, "Invalid scrape timeout margin", err) + + return 1 + } + + // Check the routes before the collectors are built, so that an invalid + // telemetry path fails the startup instead of panicking in the router. + if _, err := newServeMux(*metricsPath, *debugEnabled, http.NotFoundHandler()); err != nil { + logStartupError(ctx, "Invalid telemetry path", err) + + return 1 + } + // When scraping to stdout, redirect logs to stderr so they don't // mix with the metrics output. Only override if the user hasn't // explicitly set --log.file to something other than the default. @@ -237,8 +254,18 @@ func run(ctx context.Context, args []string) int { var disabledCollectorList []string if *disabledCollectors != "" { - disabledCollectorList = slices.Compact(strings.Split(*disabledCollectors, ",")) + disabledCollectorList = splitCollectorList(*disabledCollectors) collectors.Disable(disabledCollectorList) + + // An unknown name in --collectors.enabled fails the startup. Only warn about one in + // --collectors.disabled: disabling a collector that does not exist leaves nothing + // running that should not, and a failure would break configurations that disable a + // collector which a later release removed. + if unknown := unknownCollectors(disabledCollectorList); len(unknown) > 0 { + logger.LogAttrs(ctx, slog.LevelWarn, "ignoring unknown collectors in --collectors.disabled", + slog.Any("collectors", unknown), + ) + } } // Close the collectors on every return from here on. On a regular shutdown, this runs after the HTTP server has shut down. @@ -276,16 +303,13 @@ func run(ctx context.Context, args []string) int { return scrapeToStdout(ctx, os.Stdout, logger, collectors, handlerOpts) } - mux := http.NewServeMux() - mux.Handle("GET /health", httphandler.NewHealthHandler()) - mux.Handle("GET "+*metricsPath, httphandler.New(logger, collectors, handlerOpts)) + mux, err := newServeMux(*metricsPath, *debugEnabled, httphandler.New(logger, collectors, handlerOpts)) + if err != nil { + logger.LogAttrs(ctx, slog.LevelError, "failed to build router", + slog.Any("err", err), + ) - if *debugEnabled { - mux.HandleFunc("GET /debug/pprof/", pprof.Index) - mux.HandleFunc("GET /debug/pprof/cmdline", pprof.Cmdline) - mux.HandleFunc("GET /debug/pprof/profile", pprof.Profile) - mux.HandleFunc("GET /debug/pprof/symbol", pprof.Symbol) - mux.HandleFunc("GET /debug/pprof/trace", pprof.Trace) + return 1 } logger.LogAttrs(ctx, slog.LevelInfo, "started windows_exporter", @@ -481,10 +505,94 @@ func effectiveCollectors(enabled, disabled []string) []string { }) } -func expandEnabledCollectors(enabled string) []string { - expanded := strings.ReplaceAll(enabled, "[defaults]", collector.DefaultCollectors) +// unknownCollectors returns the names that are not available collectors. +// Empty names, as in "cpu,,net", are skipped. +func unknownCollectors(names []string) []string { + available := collector.Available() - return slices.Compact(strings.Split(expanded, ",")) + var unknown []string + + for _, name := range names { + if name != "" && !slices.Contains(available, name) && !slices.Contains(unknown, name) { + unknown = append(unknown, name) + } + } + + return unknown +} + +// validateTimeoutMargin rejects a negative, NaN or infinite margin. A negative +// margin would extend the scrape timeout beyond the timeout of the client. +func validateTimeoutMargin(margin float64) error { + if math.IsNaN(margin) || math.IsInf(margin, 0) || margin < 0 { + return fmt.Errorf("--scrape.timeout-margin must be a non-negative number of seconds, got %v", margin) + } + + return nil +} + +// newServeMux returns the router of the exporter. http.ServeMux panics on a +// pattern it cannot parse or that conflicts with another route, so newServeMux +// returns an error for a telemetry path that cannot be registered instead. +func newServeMux(metricsPath string, debugEnabled bool, metricsHandler http.Handler) (*http.ServeMux, error) { + if !strings.HasPrefix(metricsPath, "/") { + return nil, fmt.Errorf("--telemetry.path %q must start with /", metricsPath) + } + + if metricsPath == healthPath { + return nil, fmt.Errorf("--telemetry.path %q is reserved for the health endpoint", metricsPath) + } + + mux := http.NewServeMux() + mux.Handle("GET "+healthPath, httphandler.NewHealthHandler()) + + if debugEnabled { + mux.HandleFunc("GET /debug/pprof/", pprof.Index) + mux.HandleFunc("GET /debug/pprof/cmdline", pprof.Cmdline) + mux.HandleFunc("GET /debug/pprof/profile", pprof.Profile) + mux.HandleFunc("GET /debug/pprof/symbol", pprof.Symbol) + mux.HandleFunc("GET /debug/pprof/trace", pprof.Trace) + } + + // The metrics route is registered last, so that a conflict with any other route is detected. + if err := handle(mux, "GET "+metricsPath, metricsHandler); err != nil { + return nil, fmt.Errorf("--telemetry.path %q cannot be registered: %w", metricsPath, err) + } + + return mux, nil +} + +// handle registers handler for pattern and returns the panic of +// http.ServeMux.Handle as an error. +func handle(mux *http.ServeMux, pattern string, handler http.Handler) error { + var err error + + func() { + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("%v", r) + } + }() + + mux.Handle(pattern, handler) + }() + + return err +} + +func expandEnabledCollectors(enabled string) []string { + return splitCollectorList(strings.ReplaceAll(enabled, "[defaults]", collector.DefaultCollectors)) +} + +// splitCollectorList splits a comma-separated list of collector names and trims +// the whitespace around each name, so that "cpu, net" names net. +func splitCollectorList(list string) []string { + names := strings.Split(list, ",") + for i, name := range names { + names[i] = strings.TrimSpace(name) + } + + return slices.Compact(names) } // scrapeToStdout performs a single scrape through the HTTP handler and writes diff --git a/cmd/windows_exporter/main_test.go b/cmd/windows_exporter/main_test.go index ee6af0b3..34302d24 100644 --- a/cmd/windows_exporter/main_test.go +++ b/cmd/windows_exporter/main_test.go @@ -24,8 +24,10 @@ import ( "fmt" "io" "log/slog" + "math" "net" "net/http" + "net/http/httptest" "net/url" "os" "strings" @@ -77,6 +79,21 @@ func TestRun(t *testing.T) { config: `{"web":{"listen-address":"127.0.0.1:8083"}}`, metricsEndpoint: "http://127.0.0.1:8084/metrics", }, + { + name: "invalid telemetry.path without leading slash", + args: []string{"--web.listen-address=127.0.0.1:8085", "--telemetry.path=metrics"}, + exitCode: 1, + }, + { + name: "invalid telemetry.path health", + args: []string{"--web.listen-address=127.0.0.1:8085", "--telemetry.path=/health"}, + exitCode: 1, + }, + { + name: "invalid scrape.timeout-margin", + args: []string{"--web.listen-address=127.0.0.1:8085", "--scrape.timeout-margin=-1"}, + exitCode: 1, + }, } { t.Run(tc.name, func(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) @@ -184,6 +201,114 @@ func waitUntilListening(tb testing.TB, network, address string) error { return fmt.Errorf("listener not listening: %w", err) } +func TestNewServeMux(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + path string + debug bool + wantErr string + }{ + {path: "/metrics"}, + {path: "/"}, + {path: "/custom/metrics"}, + {path: "/metrics/"}, + {path: "/debug/pprof/"}, + {path: "metrics", wantErr: "must start with /"}, + {path: "", wantErr: "must start with /"}, + {path: "/health", wantErr: "reserved for the health endpoint"}, + {path: "/debug/pprof/", debug: true, wantErr: "cannot be registered"}, + {path: "/debug/pprof/trace", debug: true, wantErr: "cannot be registered"}, + {path: "/metrics/{name", wantErr: "cannot be registered"}, + } { + t.Run(fmt.Sprintf("%s debug=%t", tc.path, tc.debug), func(t *testing.T) { + t.Parallel() + + metricsHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("metrics")) + }) + + mux, err := newServeMux(tc.path, tc.debug, metricsHandler) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + require.Nil(t, mux) + + return + } + + require.NoError(t, err) + + for _, path := range []string{tc.path, healthPath} { + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequestWithContext(t.Context(), http.MethodGet, path, nil)) + require.Equal(t, http.StatusOK, rec.Code, path) + + if path == tc.path { + require.Equal(t, "metrics", rec.Body.String(), path) + } + } + }) + } +} + +func TestValidateTimeoutMargin(t *testing.T) { + t.Parallel() + + for _, margin := range []float64{0, 0.5, 30} { + require.NoError(t, validateTimeoutMargin(margin), margin) + } + + for _, margin := range []float64{-0.5, math.NaN(), math.Inf(1), math.Inf(-1)} { + require.Error(t, validateTimeoutMargin(margin), margin) + } +} + +func TestUnknownCollectors(t *testing.T) { + t.Parallel() + + require.Empty(t, unknownCollectors([]string{"cpu", "", "net"})) + require.Equal(t, []string{"nope", "CPU"}, unknownCollectors([]string{"cpu", "nope", "", "nope", "CPU"})) +} + +func TestSplitCollectorList(t *testing.T) { + t.Parallel() + + for list, want := range map[string][]string{ + "cpu,net": {"cpu", "net"}, + "cpu, net": {"cpu", "net"}, + " cpu ,\tnet , os": {"cpu", "net", "os"}, + "cpu,cpu, cpu": {"cpu"}, + // An empty name is kept, so that an empty --collectors.enabled still fails. + "": {""}, + } { + require.Equal(t, want, splitCollectorList(list), list) + } + + require.Equal(t, []string{"cpu", "net"}, expandEnabledCollectors("cpu, net")) + require.Contains(t, expandEnabledCollectors("[defaults], net"), "net") + require.NotContains(t, expandEnabledCollectors("[defaults], net"), " net") +} + +// TestCollectorListsWithSpaces checks that names with spaces around them enable +// and disable the collectors they name. +func TestCollectorListsWithSpaces(t *testing.T) { + t.Parallel() + + collectors := collector.NewWithFlags(kingpin.New("test", "test")) + require.NoError(t, collectors.Enable(expandEnabledCollectors("cpu, net, os"))) + + disabled := splitCollectorList("cpu, net") + collectors.Disable(disabled) + require.Empty(t, unknownCollectors(disabled), "no warning for trimmed names") + + limited, err := collectors.WithCollectors([]string{"os"}) + require.NoError(t, err) + require.NotNil(t, limited) + + _, err = collectors.WithCollectors([]string{"net"}) + require.Error(t, err, "net must be disabled") +} + func TestScrapeStdout(t *testing.T) { t.Parallel() diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 9514c357..f449f155 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -167,15 +167,22 @@ func TestNewConfigFileResolverFlags(t *testing.T) { // kingpin's built-in flags, like --help, are not configuration options. builtin := kingpin.New("test", "test") - for _, flag := range newApp(mainFlags).Model().Flags { - switch { - case builtin.GetFlag(flag.Name) != nil, - // selects the configuration file itself - flag.Name == "config.file", - // prints the available sub-collectors and exits - flag.Name == "collector.exchange.list", - // CLI-only diagnostic mode; not valid in a config file - flag.Name == "scrape.stdout": + // cliOnly lists the flags that have no configuration file key by design. + cliOnly := map[string]string{ + "config.file": "selects the configuration file itself", + "collector.exchange.list": "prints the available sub-collectors and exits", + "scrape.stdout": "CLI-only diagnostic mode; not valid in a config file", + } + + app := newApp(mainFlags) + + // A renamed or removed flag must not stay exempt under its old name. + for name := range cliOnly { + require.NotNil(t, app.GetFlag(name), "the CLI-only flag %s does not exist", name) + } + + for _, flag := range app.Model().Flags { + if _, ok := cliOnly[flag.Name]; ok || builtin.GetFlag(flag.Name) != nil { continue }