fix: strict flag values, MI NULL properties and container without HCS (#2553)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>

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",

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)
 		}
 

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)
+		})
+	}
+}

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)
 		}
 

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)
+		})
+	}
+}

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 {

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},
 	}

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)) ||

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) {