From f8ada403f21e9f75662d4192df8d7662b784435d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Otto=20Kr=C3=B6pke?= Date: Thu, 8 Oct 2026 19:43:38 +0200 Subject: [PATCH] fix: strict flag values, MI NULL properties and container without HCS (#2553) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Claude Opus 5.5 Signed-off-by: Jan-Otto Kröpke --- internal/collector/container/container.go | 10 +++ .../performancecounter/performancecounter.go | 8 ++- .../performancecounter_flags_test.go | 67 +++++++++++++++++++ internal/collector/registry/registry.go | 8 ++- .../collector/registry/registry_flags_test.go | 67 +++++++++++++++++++ internal/mi/session.go | 7 ++ internal/mi/unmarshal_test.go | 5 ++ pkg/collector/collection.go | 5 +- pkg/collector/collection_test.go | 2 + 9 files changed, 176 insertions(+), 3 deletions(-) create mode 100644 internal/collector/performancecounter/performancecounter_flags_test.go create mode 100644 internal/collector/registry/registry_flags_test.go diff --git a/internal/collector/container/container.go b/internal/collector/container/container.go index 351d9333..7ae5e41f 100644 --- a/internal/collector/container/container.go +++ b/internal/collector/container/container.go @@ -184,6 +184,16 @@ func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error { c.annotationsCacheHCS = make(map[string]containerInfo) c.annotationsCacheJob = make(map[string]containerInfo) + // Without the Containers feature, the Host Compute Service is missing and + // every scrape would fail. Report the collector as unsupported instead, so + // it is skipped once at startup rather than logging a warning per scrape. + if slices.Contains(c.config.CollectorsEnabled, subCollectorHCS) { + if _, err := hcs.GetContainers(); errors.Is(err, hcs.ErrServiceNotAvailable) { + return fmt.Errorf("host compute service not available, is the Containers feature installed? %w: %w", + errors.ErrUnsupported, err) + } + } + c.containerAvailable = prometheus.NewDesc( prometheus.BuildFQName(types.Namespace, Name, "available"), "Available", diff --git a/internal/collector/performancecounter/performancecounter.go b/internal/collector/performancecounter/performancecounter.go index 08fe041d..6acbf31f 100644 --- a/internal/collector/performancecounter/performancecounter.go +++ b/internal/collector/performancecounter/performancecounter.go @@ -20,6 +20,7 @@ package performancecounter import ( "errors" "fmt" + "io" "log/slog" "maps" "math" @@ -104,7 +105,12 @@ func NewWithFlags(app *kingpin.Application) *Collector { return nil } - if err := yaml.Unmarshal([]byte(objects), &c.config.Objects); err != nil { + // Reject unknown keys like the configuration file does, so that a typo + // fails instead of silently dropping the setting. + decoder := yaml.NewDecoder(strings.NewReader(objects)) + decoder.KnownFields(true) + + if err := decoder.Decode(&c.config.Objects); err != nil && !errors.Is(err, io.EOF) { return fmt.Errorf("failed to parse objects %s: %w", objects, err) } diff --git a/internal/collector/performancecounter/performancecounter_flags_test.go b/internal/collector/performancecounter/performancecounter_flags_test.go new file mode 100644 index 00000000..8dcedc5d --- /dev/null +++ b/internal/collector/performancecounter/performancecounter_flags_test.go @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Copyright The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build windows + +package performancecounter_test + +import ( + "testing" + + "github.com/alecthomas/kingpin/v2" + "github.com/prometheus-community/windows_exporter/internal/collector/performancecounter" + "github.com/stretchr/testify/require" +) + +// The flag value must be as strict as the configuration file block: unknown +// keys fail instead of being dropped silently. +func TestNewWithFlagsRejectsUnknownKeys(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + value string + wantErr bool + }{ + {name: "valid", value: `- name: memory + object: Memory + counters: + - name: Available Bytes`}, + {name: "comments only", value: "# nothing configured"}, + {name: "unknown key", value: `- name: memory + object: Memory + invalid: x + counters: + - name: Available Bytes`, wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + app := kingpin.New("test", "") + performancecounter.NewWithFlags(app) + + // The = form is required, because kingpin reads a separate value + // starting with "-", like a YAML list, as another flag. + _, err := app.Parse([]string{"--collector.performancecounter.objects=" + tc.value}) + if tc.wantErr { + require.ErrorContains(t, err, "invalid") + + return + } + + require.NoError(t, err) + }) + } +} diff --git a/internal/collector/registry/registry.go b/internal/collector/registry/registry.go index 108e50b6..85cd8e40 100644 --- a/internal/collector/registry/registry.go +++ b/internal/collector/registry/registry.go @@ -20,6 +20,7 @@ package registry import ( "errors" "fmt" + "io" "log/slog" "maps" "regexp" @@ -93,7 +94,12 @@ func NewWithFlags(app *kingpin.Application) *Collector { return nil } - if err := yaml.Unmarshal([]byte(keys), &c.config.Keys); err != nil { + // Reject unknown keys like the configuration file does, so that a typo + // fails instead of silently dropping the setting. + decoder := yaml.NewDecoder(strings.NewReader(keys)) + decoder.KnownFields(true) + + if err := decoder.Decode(&c.config.Keys); err != nil && !errors.Is(err, io.EOF) { return fmt.Errorf("failed to parse keys %s: %w", keys, err) } diff --git a/internal/collector/registry/registry_flags_test.go b/internal/collector/registry/registry_flags_test.go new file mode 100644 index 00000000..ebdd9020 --- /dev/null +++ b/internal/collector/registry/registry_flags_test.go @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Copyright The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build windows + +package registry_test + +import ( + "testing" + + "github.com/alecthomas/kingpin/v2" + "github.com/prometheus-community/windows_exporter/internal/collector/registry" + "github.com/stretchr/testify/require" +) + +// The flag value must be as strict as the configuration file block: unknown +// keys fail instead of being dropped silently. +func TestNewWithFlagsRejectsUnknownKeys(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + value string + wantErr bool + }{ + {name: "valid", value: `- name: crash + key: HKLM\SYSTEM\CurrentControlSet\Control\CrashControl + values: + - name: AutoReboot`}, + {name: "comments only", value: "# nothing configured"}, + {name: "unknown key", value: `- name: crash + key: HKLM\SYSTEM\CurrentControlSet\Control\CrashControl + invalid: x + values: + - name: AutoReboot`, wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + app := kingpin.New("test", "") + registry.NewWithFlags(app) + + // The = form is required, because kingpin reads a separate value + // starting with "-", like a YAML list, as another flag. + _, err := app.Parse([]string{"--collector.registry.keys=" + tc.value}) + if tc.wantErr { + require.ErrorContains(t, err, "invalid") + + return + } + + require.NoError(t, err) + }) + } +} diff --git a/internal/mi/session.go b/internal/mi/session.go index cce450d6..87f57dd5 100644 --- a/internal/mi/session.go +++ b/internal/mi/session.go @@ -222,6 +222,13 @@ func unmarshalInstance(instance *Instance, fields []miField, structValue reflect // setField assigns the value of element to field, rejecting Go field types that // cannot hold the MI value instead of letting the reflect package panic. func setField(miTag string, field reflect.Value, element *Element) error { + if element.IsNull() { + // MI makes no promise about the contents of a NULL element's union. + // Check the Go type against a zeroed copy, so a mismatched field fails + // whether or not the property is NULL, but leave the field untouched. + return setField(miTag, reflect.New(field.Type()).Elem(), &Element{valueType: element.valueType}) + } + switch element.valueType { case ValueTypeBOOLEAN: if field.Kind() != reflect.Bool { diff --git a/internal/mi/unmarshal_test.go b/internal/mi/unmarshal_test.go index 03f17dcd..bc1a4466 100644 --- a/internal/mi/unmarshal_test.go +++ b/internal/mi/unmarshal_test.go @@ -69,6 +69,11 @@ func TestSetField(t *testing.T) { {"char16_overflows_uint8", Element{value: 0x20AC, valueType: ValueTypeCHAR16}, newField[uint8](), nil, true}, {"char16_into_bool", Element{value: 'A', valueType: ValueTypeCHAR16}, newField[bool](), nil, true}, + {"null_uint32_keeps_zero_value", Element{value: 42, valueType: ValueTypeUINT32, flags: flagNull}, newField[uint32](), uint32(0), false}, + {"null_real64_keeps_zero_value", Element{value: uintptr(math.Float64bits(9.875)), valueType: ValueTypeREAL64, flags: flagNull}, newField[float64](), float64(0), false}, + {"null_char16_into_string_keeps_zero_value", Element{value: 'A', valueType: ValueTypeCHAR16, flags: flagNull}, newField[string](), "", false}, + {"null_still_rejects_wrong_type", Element{value: 42, valueType: ValueTypeUINT32, flags: flagNull}, newField[string](), nil, true}, + {"uint16a_into_wrong_slice", Element{value: 0, valueType: ValueTypeUINT16A}, newField[[]uint32](), nil, true}, {"unsupported_type", Element{value: 0, valueType: ValueTypeDATETIME}, newField[string](), nil, true}, } diff --git a/pkg/collector/collection.go b/pkg/collector/collection.go index 270961e5..47c1a058 100644 --- a/pkg/collector/collection.go +++ b/pkg/collector/collection.go @@ -274,7 +274,10 @@ func (c *Collection) Build(ctx context.Context, logger *slog.Logger) error { err := fmt.Errorf("error build collector %s: %w", result.name, result.err) - if errors.Is(err, pdh.ErrNoData) || + // errors.ErrUnsupported marks a collector whose subsystem is not + // available on this host, e.g. a Windows feature that is not installed. + if errors.Is(err, errors.ErrUnsupported) || + errors.Is(err, pdh.ErrNoData) || errors.Is(err, winregistry.ErrNotExist) || errors.Is(err, pdh.NewPdhError(pdh.CstatusNoObject)) || errors.Is(err, pdh.NewPdhError(pdh.CstatusNoCounter)) || diff --git a/pkg/collector/collection_test.go b/pkg/collector/collection_test.go index 56e80bf9..775363c7 100644 --- a/pkg/collector/collection_test.go +++ b/pkg/collector/collection_test.go @@ -19,6 +19,7 @@ package collector_test import ( "errors" + "fmt" "log/slog" "testing" "time" @@ -69,6 +70,7 @@ func TestCollectionBuildAndClose(t *testing.T) { {name: "success"}, {name: "build failure", buildErr: buildFailure, wantBuildErr: buildFailure}, {name: "optional counter missing", buildErr: pdh.ErrNoData}, + {name: "unsupported on this host", buildErr: fmt.Errorf("feature missing: %w", errors.ErrUnsupported)}, {name: "close failure", closeErr: closeFailure}, } { t.Run(tc.name, func(t *testing.T) {