From ffcfd8ef3ce3282b658d90fd6c87d498ae94160a Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 7 May 2026 00:31:37 +0100 Subject: [PATCH 1/6] Add CRD-runtime drift detection test framework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Silent drift between CRD types and their runtime config counterparts has caused user-facing bugs (e.g. PR #3118, issue #3125) where new fields appeared to work in tests but were not wired through the CRD conversion path. The only existing safety nets — code review and end-to-end tests — do not scale. Add a reusable reflection-based field walker and a bidirectional drift test pattern. The pattern requires every leaf JSON field on either side of a CRD/runtime boundary to be either explicitly mapped to the other side or explicitly declared as intentionally unmapped, with a justification string. Adding a field to either type without classifying it fails the test with a clear action-required message. Apply the pattern to MCPTelemetryConfig <-> telemetry.Config as the first real-world exercise. The drift test passes against the current codebase and surfaced one previously-undocumented leaf (openTelemetry.caBundleRef.configMapRef.optional) which is now explicitly declared. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/thv-operator/internal/testutil/reflect.go | 242 +++++++++++++++ .../internal/testutil/reflect_test.go | 277 ++++++++++++++++++ .../pkg/spectoconfig/telemetry_drift_test.go | 192 ++++++++++++ 3 files changed, 711 insertions(+) create mode 100644 cmd/thv-operator/internal/testutil/reflect.go create mode 100644 cmd/thv-operator/internal/testutil/reflect_test.go create mode 100644 cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go diff --git a/cmd/thv-operator/internal/testutil/reflect.go b/cmd/thv-operator/internal/testutil/reflect.go new file mode 100644 index 0000000000..67cb430288 --- /dev/null +++ b/cmd/thv-operator/internal/testutil/reflect.go @@ -0,0 +1,242 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package testutil provides reflection-based helpers used by drift-detection +// tests in the operator. It is intended for test code only. +package testutil + +import ( + "encoding/json" + "reflect" + "sort" + "strings" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + thvjson "github.com/stacklok/toolhive/pkg/json" + vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" +) + +// FlattenJSONLeafFields returns every leaf JSON field path under t as a sorted +// slice of dot-delimited paths (e.g. "openTelemetry.tracing.enabled"). +// +// Walking semantics: +// - For struct fields, the JSON tag's name (before any comma) is used as the +// path segment. If the tag is "-", the field is skipped. If the tag is +// missing, the Go field name is used. +// - Unexported fields are skipped. +// - Fields with `,inline` flatten into the parent path with no prefix. +// - Pointer types are dereferenced and walked. +// - Struct types are recursed into. +// - Slice and array types whose ELEMENT type is a struct (or pointer to a +// struct) recurse into that element type, joining with the parent path. +// A path like "tools.workload" therefore means "every element of the +// `tools` slice has a `workload` field". +// - Map types whose VALUE type is a struct (or pointer to a struct) recurse +// into that value type the same way. +// - All other types (primitives, []string, map[string]string, +// time.Duration, metav1.Duration, json.RawMessage, and any type listed +// in the leaf allowlist below) terminate the walk and contribute one +// leaf path entry. +// - To prevent infinite recursion on self-referential types, track visited +// types per walk and stop if a cycle is detected. +// +// If t is nil or, after dereferencing, is not a struct, an empty slice is +// returned rather than panicking. +func FlattenJSONLeafFields(t reflect.Type) []string { + if t == nil { + return []string{} + } + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + if t.Kind() != reflect.Struct { + return []string{} + } + + leafSet := make(map[string]struct{}) + visited := map[reflect.Type]int{} + recurseStruct(t, "", leafSet, visited) + + out := make([]string, 0, len(leafSet)) + for p := range leafSet { + out = append(out, p) + } + sort.Strings(out) + return out +} + +// leafTypes lists types that terminate the walk even though they are structs. +// They contribute a single leaf entry at their parent path. +var leafTypes = map[reflect.Type]struct{}{ + reflect.TypeOf(time.Duration(0)): {}, + reflect.TypeOf(metav1.Duration{}): {}, + reflect.TypeOf(json.RawMessage{}): {}, + reflect.TypeOf(thvjson.Map{}): {}, + reflect.TypeOf(thvjson.Any{}): {}, + reflect.TypeOf(vmcpconfig.Duration(0)): {}, +} + +// skipFieldTypes lists embedded struct types that must be skipped entirely. +var skipFieldTypes = map[reflect.Type]struct{}{ + reflect.TypeOf(metav1.TypeMeta{}): {}, + reflect.TypeOf(metav1.ObjectMeta{}): {}, + reflect.TypeOf(metav1.ListMeta{}): {}, +} + +// walkStruct recurses into a struct type and adds leaf paths to leafSet. +// prefix is the dot-delimited path accumulated so far (without trailing dot). +// visited is the set of struct types currently on the recursion stack; it is +// used to break cycles on self-referential types. Callers add t to visited +// before invoking this function and remove it afterwards. +func walkStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]int) { + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + // Skip unexported fields. PkgPath is non-empty for unexported fields. + // Anonymous (embedded) fields have an empty PkgPath only when their + // underlying type is exported, so the same check works for them. + if field.PkgPath != "" && !field.Anonymous { + continue + } + + // Skip explicit skip-list types (TypeMeta/ObjectMeta/ListMeta) when + // embedded. + if _, skip := skipFieldTypes[field.Type]; skip { + continue + } + + name, inline, omit := parseJSONTag(field) + if omit { + continue + } + + // Determine the path segment for this field. + segment := func() string { + // Anonymous fields are inlined by encoding/json when they have no + // json name (or an explicit `,inline` tag). Treat them as inline. + if field.Anonymous && (name == "" || inline) { + return "" + } + if inline { + return "" + } + if name == "" { + return field.Name + } + return name + }() + + childPrefix := joinPath(prefix, segment) + walkType(field.Type, childPrefix, leafSet, visited) + } +} + +// walkType walks a single type with the given accumulated prefix. It either +// recurses into nested structs/slices/maps or records a leaf at prefix. +func walkType(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]int) { + // Deref pointers. + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + + // Allow-listed leaf types short-circuit any further walking. + if _, ok := leafTypes[t]; ok { + recordLeaf(prefix, leafSet) + return + } + + switch t.Kind() { + case reflect.Struct: + recurseStruct(t, prefix, leafSet, visited) + case reflect.Slice, reflect.Array: + elem := t.Elem() + for elem.Kind() == reflect.Ptr { + elem = elem.Elem() + } + // Recurse only when the element is a struct that is NOT a leaf type. + if _, isLeaf := leafTypes[elem]; !isLeaf && elem.Kind() == reflect.Struct { + recurseStruct(elem, prefix, leafSet, visited) + return + } + recordLeaf(prefix, leafSet) + case reflect.Map: + val := t.Elem() + for val.Kind() == reflect.Ptr { + val = val.Elem() + } + if _, isLeaf := leafTypes[val]; !isLeaf && val.Kind() == reflect.Struct { + recurseStruct(val, prefix, leafSet, visited) + return + } + recordLeaf(prefix, leafSet) + default: + recordLeaf(prefix, leafSet) + } +} + +// maxStructRevisits caps how many times a single struct type may appear on the +// recursion stack. A value of 2 means the type may be entered at most twice +// on the same path: the original entry plus one self-referential visit. This +// is enough for one level of "next." expansion in cyclic types like +// linked lists, while still guaranteeing the walk terminates. +const maxStructRevisits = 2 + +// recurseStruct descends into a nested struct type with cycle protection. +// visited holds the count of how many times each struct type currently appears +// on the recursion stack. When entering t would exceed maxStructRevisits, the +// walk stops silently — no leaf is recorded, on the assumption that the +// useful leaves under t have already been captured by the earlier visit on +// the same path. +func recurseStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]int) { + if visited[t] >= maxStructRevisits { + return + } + visited[t]++ + defer func() { visited[t]-- }() + walkStruct(t, prefix, leafSet, visited) +} + +// parseJSONTag extracts (name, inline, omit) from the json struct tag. +// - name is the explicit JSON name (empty when not specified). +// - inline is true when the tag includes ",inline". +// - omit is true when the tag is "-" (field must be skipped entirely). +func parseJSONTag(field reflect.StructField) (string, bool, bool) { + tag, ok := field.Tag.Lookup("json") + if !ok { + return "", false, false + } + if tag == "-" { + return "", false, true + } + parts := strings.Split(tag, ",") + name := parts[0] + inline := false + for _, p := range parts[1:] { + if p == "inline" { + inline = true + } + } + return name, inline, false +} + +// joinPath joins prefix and segment with a dot, handling empty segments +// (e.g. inline fields) by returning the prefix unchanged. +func joinPath(prefix, segment string) string { + if segment == "" { + return prefix + } + if prefix == "" { + return segment + } + return prefix + "." + segment +} + +// recordLeaf records prefix as a leaf path. An empty prefix is ignored +// because it would not represent a real field. +func recordLeaf(prefix string, leafSet map[string]struct{}) { + if prefix == "" { + return + } + leafSet[prefix] = struct{}{} +} diff --git a/cmd/thv-operator/internal/testutil/reflect_test.go b/cmd/thv-operator/internal/testutil/reflect_test.go new file mode 100644 index 0000000000..1c2a2c0c57 --- /dev/null +++ b/cmd/thv-operator/internal/testutil/reflect_test.go @@ -0,0 +1,277 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "encoding/json" + "reflect" + "sort" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + thvjson "github.com/stacklok/toolhive/pkg/json" + vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" +) + +// --- Fixture types --------------------------------------------------------- + +type flatPrimitives struct { + Name string `json:"name"` + Count int `json:"count"` + Enabled bool `json:"enabled"` +} + +type withSkippedTag struct { + Visible string `json:"visible"` + Hidden string `json:"-"` +} + +type noJSONTag struct { + Visible string +} + +type withOmitempty struct { + Maybe string `json:"maybe,omitempty"` +} + +type leafInner struct { + A string `json:"a"` + B string `json:"b"` +} + +type withPointerStruct struct { + Inner *leafInner `json:"inner"` +} + +type withSliceOfStruct struct { + Items []leafInner `json:"items"` +} + +type withSliceOfPointerStruct struct { + Items []*leafInner `json:"items"` +} + +type withSliceOfPrimitive struct { + Tags []string `json:"tags"` +} + +type withMapStructValue struct { + ByKey map[string]*leafInner `json:"byKey"` +} + +type withMapPrimitiveValue struct { + Labels map[string]string `json:"labels"` +} + +type embedSource struct { + Foo string `json:"foo"` +} + +// withEmbeddedNoInline embeds a struct without an explicit `,inline` tag. +// In Go's encoding/json semantics, anonymous fields with no json tag have +// their exported fields promoted into the parent — equivalent to inline. +type withEmbeddedNoInline struct { + embedSource + Bar string `json:"bar"` +} + +type withEmbeddedInline struct { + embedSource `json:",inline"` + Bar string `json:"bar"` +} + +type withUnexportedField struct { + Visible string `json:"visible"` + hidden string //nolint:unused // exercised by reflection +} + +type withDuration struct { + Wait time.Duration `json:"wait"` + WaitMeta metav1.Duration `json:"waitMeta"` +} + +type withRawJSON struct { + Raw json.RawMessage `json:"raw"` +} + +type withThvJSON struct { + M thvjson.Map `json:"m"` + A thvjson.Any `json:"a"` +} + +type withVmcpDuration struct { + D vmcpconfig.Duration `json:"d"` +} + +type recursive struct { + Name string `json:"name"` + Next *recursive `json:"next,omitempty"` +} + +// --- Tests ----------------------------------------------------------------- + +func TestFlattenJSONLeafFields(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in any + want []string + }{ + { + name: "flat primitives", + in: flatPrimitives{}, + want: []string{"count", "enabled", "name"}, + }, + { + name: "json:\"-\" field is skipped", + in: withSkippedTag{}, + want: []string{"visible"}, + }, + { + name: "missing json tag uses Go field name", + in: noJSONTag{}, + want: []string{"Visible"}, + }, + { + name: "omitempty does not appear in path", + in: withOmitempty{}, + want: []string{"maybe"}, + }, + { + name: "pointer field recurses", + in: withPointerStruct{}, + want: []string{"inner.a", "inner.b"}, + }, + { + name: "slice of struct recurses into element", + in: withSliceOfStruct{}, + want: []string{"items.a", "items.b"}, + }, + { + name: "slice of pointer to struct recurses into element", + in: withSliceOfPointerStruct{}, + want: []string{"items.a", "items.b"}, + }, + { + name: "slice of primitive terminates at slice", + in: withSliceOfPrimitive{}, + want: []string{"tags"}, + }, + { + name: "map with struct value recurses into value type", + in: withMapStructValue{}, + want: []string{"byKey.a", "byKey.b"}, + }, + { + name: "map with primitive value terminates at map", + in: withMapPrimitiveValue{}, + want: []string{"labels"}, + }, + { + name: "embedded struct without ,inline still flattens", + in: withEmbeddedNoInline{}, + want: []string{"bar", "foo"}, + }, + { + name: "embedded struct with ,inline flattens", + in: withEmbeddedInline{}, + want: []string{"bar", "foo"}, + }, + { + name: "unexported field is skipped", + in: withUnexportedField{}, + want: []string{"visible"}, + }, + { + name: "time.Duration and metav1.Duration are leaves", + in: withDuration{}, + want: []string{"wait", "waitMeta"}, + }, + { + name: "json.RawMessage is a leaf", + in: withRawJSON{}, + want: []string{"raw"}, + }, + { + name: "thvjson Map and Any are leaves", + in: withThvJSON{}, + want: []string{"a", "m"}, + }, + { + name: "vmcp config Duration is a leaf", + in: withVmcpDuration{}, + want: []string{"d"}, + }, + { + name: "self-referential type does not recurse infinitely", + in: recursive{}, + want: []string{"name", "next.name"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := FlattenJSONLeafFields(reflect.TypeOf(tc.in)) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestFlattenJSONLeafFields_PointerInputDereferenced(t *testing.T) { + t.Parallel() + + got := FlattenJSONLeafFields(reflect.TypeOf(&flatPrimitives{})) + require.Equal(t, []string{"count", "enabled", "name"}, got) +} + +func TestFlattenJSONLeafFields_OutputIsSorted(t *testing.T) { + t.Parallel() + + got := FlattenJSONLeafFields(reflect.TypeOf(flatPrimitives{})) + require.NotEmpty(t, got) + require.True(t, sort.StringsAreSorted(got), "expected sorted output, got %v", got) +} + +func TestFlattenJSONLeafFields_NilOrNonStructReturnsEmpty(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in reflect.Type + }{ + {name: "nil reflect.Type", in: nil}, + {name: "primitive int", in: reflect.TypeOf(0)}, + {name: "string", in: reflect.TypeOf("")}, + {name: "slice", in: reflect.TypeOf([]string{})}, + {name: "pointer to primitive", in: reflect.TypeOf((*int)(nil))}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := FlattenJSONLeafFields(tc.in) + assert.Empty(t, got) + }) + } +} + +func TestFlattenJSONLeafFields_SkipsObjectMetaAndTypeMeta(t *testing.T) { + t.Parallel() + + type withK8sMeta struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + Spec flatPrimitives `json:"spec"` + } + + got := FlattenJSONLeafFields(reflect.TypeOf(withK8sMeta{})) + // Should only contain the spec.* fields; nothing from TypeMeta/ObjectMeta. + require.Equal(t, []string{"spec.count", "spec.enabled", "spec.name"}, got) +} diff --git a/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go b/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go new file mode 100644 index 0000000000..c00bcc77de --- /dev/null +++ b/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go @@ -0,0 +1,192 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package spectoconfig + +// This file holds drift-detection tests that compare the CRD-side telemetry +// configuration type (v1beta1.MCPTelemetryConfigSpec) against its runtime +// counterpart (telemetry.Config). The goal is to fail any time a new leaf +// field is added on either side without an explicit decision about whether +// the field should be mirrored across the boundary or intentionally only +// exists on one side. +// +// The test uses three declarative tables as the source of truth: +// +// * telemetryFieldMappings — leaf fields present on BOTH sides, +// paired by their dot-delimited paths. +// * telemetryIgnoredOnCRDOnly — leaf fields that only exist on the +// CRD side, with a justification. +// * telemetryIgnoredOnRuntimeOnly — leaf fields that only exist on the +// runtime side, with a justification. +// +// When either side gains or loses a field, exactly one of these tables must +// be updated. The "Mapping table sanity" test guards the tables themselves +// (no duplicates, no empty entries, no overlap with the ignore lists). + +import ( + "reflect" + "sort" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" + "github.com/stacklok/toolhive/cmd/thv-operator/internal/testutil" + "github.com/stacklok/toolhive/pkg/telemetry" +) + +// FieldMapping pairs a CRD-side leaf path with a runtime-side leaf path. Both +// sides must be present unless the field is in one of the ignore maps. +type FieldMapping struct { + CRD string + Runtime string +} + +// telemetryFieldMappings is the source of truth for CRD<->runtime field +// links. One entry per leaf field that exists on both sides. The CRD path is +// rooted at MCPTelemetryConfigSpec; the runtime path is rooted at +// telemetry.Config. +var telemetryFieldMappings = []FieldMapping{ + {CRD: "openTelemetry.endpoint", Runtime: "endpoint"}, + {CRD: "openTelemetry.insecure", Runtime: "insecure"}, + {CRD: "openTelemetry.headers", Runtime: "headers"}, + {CRD: "openTelemetry.resourceAttributes", Runtime: "customAttributes"}, + {CRD: "openTelemetry.tracing.enabled", Runtime: "tracingEnabled"}, + {CRD: "openTelemetry.tracing.samplingRate", Runtime: "samplingRate"}, + {CRD: "openTelemetry.metrics.enabled", Runtime: "metricsEnabled"}, + {CRD: "openTelemetry.useLegacyAttributes", Runtime: "useLegacyAttributes"}, + {CRD: "prometheus.enabled", Runtime: "enablePrometheusMetricsPath"}, +} + +// telemetryIgnoredOnCRDOnly lists CRD leaf fields that intentionally have no +// runtime counterpart. Each entry MUST include a justification. +var telemetryIgnoredOnCRDOnly = map[string]string{ + "openTelemetry.enabled": "CRD-only gate; controls whether the converter populates runtime fields at all", + "openTelemetry.sensitiveHeaders.name": "K8s-secret-backed headers; resolved by operator into runtime Headers", + "openTelemetry.sensitiveHeaders.secretKeyRef.name": "K8s-secret-backed headers; resolved by operator into runtime Headers", + "openTelemetry.sensitiveHeaders.secretKeyRef.key": "K8s-secret-backed headers; resolved by operator into runtime Headers", + "openTelemetry.caBundleRef.configMapRef.name": "K8s ConfigMap reference; resolved by operator into runtime CACertPath", + "openTelemetry.caBundleRef.configMapRef.key": "K8s ConfigMap reference; resolved by operator into runtime CACertPath", + "openTelemetry.caBundleRef.configMapRef.optional": "K8s ConfigMap reference flag promoted from corev1.ConfigMapKeySelector; not part of runtime config", +} + +// telemetryIgnoredOnRuntimeOnly lists runtime leaf fields that intentionally +// have no CRD counterpart. Each entry MUST include a justification. +var telemetryIgnoredOnRuntimeOnly = map[string]string{ + "serviceName": "per-server, set via MCPTelemetryConfigReference.ServiceName, not stored on the shared MCPTelemetryConfig", + "serviceVersion": "resolved at runtime from binary version (issue #2296)", + "environmentVariables": "CLI-only, not applicable to CRD-managed telemetry", + "caCertPath": "filesystem path computed by the operator from caBundleRef; not user-facing in the CRD", +} + +// TestTelemetryConfigDrift_CRDFieldsCovered walks MCPTelemetryConfigSpec and +// requires every leaf path to appear either as a CRD entry in +// telemetryFieldMappings or as a key in telemetryIgnoredOnCRDOnly. +func TestTelemetryConfigDrift_CRDFieldsCovered(t *testing.T) { + t.Parallel() + + mappedCRD := make(map[string]struct{}, len(telemetryFieldMappings)) + for _, m := range telemetryFieldMappings { + mappedCRD[m.CRD] = struct{}{} + } + + leaves := testutil.FlattenJSONLeafFields(reflect.TypeOf(v1beta1.MCPTelemetryConfigSpec{})) + for _, leaf := range leaves { + if _, ok := mappedCRD[leaf]; ok { + continue + } + if _, ok := telemetryIgnoredOnCRDOnly[leaf]; ok { + continue + } + t.Errorf( + "v1beta1.MCPTelemetryConfigSpec field %q is unclassified.\n"+ + "Action: add it to telemetryFieldMappings (with the corresponding telemetry.Config path)\n"+ + " OR add it to telemetryIgnoredOnCRDOnly with a justification string.", + leaf, + ) + } +} + +// TestTelemetryConfigDrift_RuntimeFieldsCovered walks telemetry.Config and +// requires every leaf path to appear either as a Runtime entry in +// telemetryFieldMappings or as a key in telemetryIgnoredOnRuntimeOnly. +func TestTelemetryConfigDrift_RuntimeFieldsCovered(t *testing.T) { + t.Parallel() + + mappedRuntime := make(map[string]struct{}, len(telemetryFieldMappings)) + for _, m := range telemetryFieldMappings { + mappedRuntime[m.Runtime] = struct{}{} + } + + leaves := testutil.FlattenJSONLeafFields(reflect.TypeOf(telemetry.Config{})) + for _, leaf := range leaves { + if _, ok := mappedRuntime[leaf]; ok { + continue + } + if _, ok := telemetryIgnoredOnRuntimeOnly[leaf]; ok { + continue + } + t.Errorf( + "telemetry.Config field %q is unclassified.\n"+ + "Action: add it to telemetryFieldMappings (with the corresponding MCPTelemetryConfigSpec path)\n"+ + " OR add it to telemetryIgnoredOnRuntimeOnly with a justification string.", + leaf, + ) + } +} + +// TestTelemetryConfigDrift_MappingTableSanity guards the mapping tables +// themselves. It catches mistakes like duplicate paths, empty entries, and +// overlap between mapped and ignored fields. +func TestTelemetryConfigDrift_MappingTableSanity(t *testing.T) { + t.Parallel() + + seenCRD := make(map[string]int, len(telemetryFieldMappings)) + seenRuntime := make(map[string]int, len(telemetryFieldMappings)) + + for i, m := range telemetryFieldMappings { + assert.NotEmptyf(t, m.CRD, "telemetryFieldMappings[%d].CRD must not be empty", i) + assert.NotEmptyf(t, m.Runtime, "telemetryFieldMappings[%d].Runtime must not be empty", i) + seenCRD[m.CRD]++ + seenRuntime[m.Runtime]++ + } + + for path, count := range seenCRD { + assert.Equalf(t, 1, count, "CRD path %q appears %d times in telemetryFieldMappings", path, count) + } + for path, count := range seenRuntime { + assert.Equalf(t, 1, count, "runtime path %q appears %d times in telemetryFieldMappings", path, count) + } + + // Overlap with ignore lists. + for _, m := range telemetryFieldMappings { + if _, dup := telemetryIgnoredOnCRDOnly[m.CRD]; dup { + t.Errorf("CRD path %q is both mapped and listed in telemetryIgnoredOnCRDOnly", m.CRD) + } + if _, dup := telemetryIgnoredOnRuntimeOnly[m.Runtime]; dup { + t.Errorf("runtime path %q is both mapped and listed in telemetryIgnoredOnRuntimeOnly", m.Runtime) + } + } + + // Justifications must be non-empty. + for path, reason := range telemetryIgnoredOnCRDOnly { + assert.NotEmptyf(t, reason, "telemetryIgnoredOnCRDOnly[%q] must include a justification", path) + } + for path, reason := range telemetryIgnoredOnRuntimeOnly { + assert.NotEmptyf(t, reason, "telemetryIgnoredOnRuntimeOnly[%q] must include a justification", path) + } + + // Stable iteration not strictly necessary for correctness, but it keeps + // failure output deterministic when assertions fire on multiple keys. + _ = sortedKeys(seenCRD) + _ = sortedKeys(seenRuntime) +} + +func sortedKeys[V any](m map[string]V) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} From 376f6d61ddb2390fa629f8cd0e425122826bb6b1 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 7 May 2026 01:13:28 +0100 Subject: [PATCH 2/6] Address review and lint on drift detection walker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix lint failures from CI: rename reflect.Ptr to reflect.Pointer, suppress exhaustive on the kind switch with rationale, and tag the ,inline json fixtures as a known revive false positive. Replace the explicit leafTypes allowlist with a json.Marshaler interface check. Every entry in the previous map shared one property — a custom MarshalJSON whose output bears no relation to the Go field layout — so asking the type how it serializes is more general than maintaining a hand-rolled list of K8s and project types. The walker is now self-maintaining for any future custom-marshaled type. Address reviewer findings on telemetry_drift_test.go: - delete dead sortedKeys helper and its misleading comment - upgrade per-entry empty-field checks to require to avoid empty-string pollution of the duplicate-detection maps - assert no path appears in both ignore maps (cross-pollination) - assert every mapping/ignore entry is still a live leaf on its type so renames and deletions surface instead of leaving stale entries - rewrite caCertPath, sensitiveHeaders, and serviceName justifications to name the actual wiring symbols a reviewer can grep for Tighten the FlattenJSONLeafFields godoc to describe the one-level self-reference expansion that maxStructRevisits actually permits. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/thv-operator/internal/testutil/reflect.go | 68 ++++++++------- .../internal/testutil/reflect_test.go | 6 +- .../pkg/spectoconfig/telemetry_drift_test.go | 82 ++++++++++++++----- 3 files changed, 103 insertions(+), 53 deletions(-) diff --git a/cmd/thv-operator/internal/testutil/reflect.go b/cmd/thv-operator/internal/testutil/reflect.go index 67cb430288..969a292d3e 100644 --- a/cmd/thv-operator/internal/testutil/reflect.go +++ b/cmd/thv-operator/internal/testutil/reflect.go @@ -10,12 +10,8 @@ import ( "reflect" "sort" "strings" - "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - thvjson "github.com/stacklok/toolhive/pkg/json" - vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" ) // FlattenJSONLeafFields returns every leaf JSON field path under t as a sorted @@ -35,12 +31,21 @@ import ( // `tools` slice has a `workload` field". // - Map types whose VALUE type is a struct (or pointer to a struct) recurse // into that value type the same way. -// - All other types (primitives, []string, map[string]string, -// time.Duration, metav1.Duration, json.RawMessage, and any type listed -// in the leaf allowlist below) terminate the walk and contribute one -// leaf path entry. -// - To prevent infinite recursion on self-referential types, track visited -// types per walk and stop if a cycle is detected. +// - Types that implement json.Marshaler are treated as leaves regardless of +// their underlying kind. A custom MarshalJSON produces a JSON shape that +// bears no relation to the Go field layout, so walking the fields would +// emit paths that never appear in the serialized form. This handles +// metav1.Time, metav1.Duration, intstr.IntOrString, resource.Quantity, +// json.RawMessage, and any project-local custom-marshaled types +// (e.g. pkg/json.Map, pkg/json.Any, pkg/vmcp/config.Duration) without an +// explicit allow-list. +// - All other primitive types (primitives, []string, map[string]string, +// time.Duration which is just int64) terminate the walk and contribute +// one leaf path entry via the default branch. +// - Self-referential types are expanded one level (the original visit plus +// one self-reference) and then truncated, to keep the walk terminating. +// Subsequent levels (e.g. "next.next.") are intentionally elided. +// See maxStructRevisits for the controlling constant. // // If t is nil or, after dereferencing, is not a struct, an empty slice is // returned rather than panicking. @@ -48,7 +53,7 @@ func FlattenJSONLeafFields(t reflect.Type) []string { if t == nil { return []string{} } - for t.Kind() == reflect.Ptr { + for t.Kind() == reflect.Pointer { t = t.Elem() } if t.Kind() != reflect.Struct { @@ -67,15 +72,16 @@ func FlattenJSONLeafFields(t reflect.Type) []string { return out } -// leafTypes lists types that terminate the walk even though they are structs. -// They contribute a single leaf entry at their parent path. -var leafTypes = map[reflect.Type]struct{}{ - reflect.TypeOf(time.Duration(0)): {}, - reflect.TypeOf(metav1.Duration{}): {}, - reflect.TypeOf(json.RawMessage{}): {}, - reflect.TypeOf(thvjson.Map{}): {}, - reflect.TypeOf(thvjson.Any{}): {}, - reflect.TypeOf(vmcpconfig.Duration(0)): {}, +// jsonMarshalerType is the reflect.Type of the json.Marshaler interface. Any +// type implementing it (directly or via pointer receiver) produces a JSON +// shape detached from its Go field layout, so it must terminate the walk. +var jsonMarshalerType = reflect.TypeOf((*json.Marshaler)(nil)).Elem() + +// implementsJSONMarshaler reports whether values of t (or *t) have a custom +// MarshalJSON method. The pointer-receiver check matters because Go method +// sets only include pointer-receiver methods when the receiver is addressable. +func implementsJSONMarshaler(t reflect.Type) bool { + return t.Implements(jsonMarshalerType) || reflect.PointerTo(t).Implements(jsonMarshalerType) } // skipFieldTypes lists embedded struct types that must be skipped entirely. @@ -136,36 +142,40 @@ func walkStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visi // recurses into nested structs/slices/maps or records a leaf at prefix. func walkType(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]int) { // Deref pointers. - for t.Kind() == reflect.Ptr { + for t.Kind() == reflect.Pointer { t = t.Elem() } - // Allow-listed leaf types short-circuit any further walking. - if _, ok := leafTypes[t]; ok { + // Custom JSON marshalers short-circuit any further walking. See + // jsonMarshalerType for the rationale. + if implementsJSONMarshaler(t) { recordLeaf(prefix, leafSet) return } - switch t.Kind() { + // Only Struct/Slice/Array/Map require recursion; every other Kind + // (primitives, interfaces, channels, etc.) is a leaf path captured by + // the default branch. + switch t.Kind() { //exhaustive:ignore case reflect.Struct: recurseStruct(t, prefix, leafSet, visited) case reflect.Slice, reflect.Array: elem := t.Elem() - for elem.Kind() == reflect.Ptr { + for elem.Kind() == reflect.Pointer { elem = elem.Elem() } - // Recurse only when the element is a struct that is NOT a leaf type. - if _, isLeaf := leafTypes[elem]; !isLeaf && elem.Kind() == reflect.Struct { + // Recurse only when the element is a plain struct (no custom marshaler). + if elem.Kind() == reflect.Struct && !implementsJSONMarshaler(elem) { recurseStruct(elem, prefix, leafSet, visited) return } recordLeaf(prefix, leafSet) case reflect.Map: val := t.Elem() - for val.Kind() == reflect.Ptr { + for val.Kind() == reflect.Pointer { val = val.Elem() } - if _, isLeaf := leafTypes[val]; !isLeaf && val.Kind() == reflect.Struct { + if val.Kind() == reflect.Struct && !implementsJSONMarshaler(val) { recurseStruct(val, prefix, leafSet, visited) return } diff --git a/cmd/thv-operator/internal/testutil/reflect_test.go b/cmd/thv-operator/internal/testutil/reflect_test.go index 1c2a2c0c57..6a746a02cd 100644 --- a/cmd/thv-operator/internal/testutil/reflect_test.go +++ b/cmd/thv-operator/internal/testutil/reflect_test.go @@ -81,8 +81,8 @@ type withEmbeddedNoInline struct { } type withEmbeddedInline struct { - embedSource `json:",inline"` - Bar string `json:"bar"` + embedSource `json:",inline"` //nolint:revive // inline is a valid kubernetes json tag option + Bar string `json:"bar"` } type withUnexportedField struct { @@ -266,7 +266,7 @@ func TestFlattenJSONLeafFields_SkipsObjectMetaAndTypeMeta(t *testing.T) { t.Parallel() type withK8sMeta struct { - metav1.TypeMeta `json:",inline"` + metav1.TypeMeta `json:",inline"` //nolint:revive // inline is a valid kubernetes json tag option metav1.ObjectMeta `json:"metadata,omitempty"` Spec flatPrimitives `json:"spec"` } diff --git a/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go b/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go index c00bcc77de..1c4359469d 100644 --- a/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go +++ b/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go @@ -25,10 +25,10 @@ package spectoconfig import ( "reflect" - "sort" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" "github.com/stacklok/toolhive/cmd/thv-operator/internal/testutil" @@ -61,22 +61,27 @@ var telemetryFieldMappings = []FieldMapping{ // telemetryIgnoredOnCRDOnly lists CRD leaf fields that intentionally have no // runtime counterpart. Each entry MUST include a justification. var telemetryIgnoredOnCRDOnly = map[string]string{ - "openTelemetry.enabled": "CRD-only gate; controls whether the converter populates runtime fields at all", - "openTelemetry.sensitiveHeaders.name": "K8s-secret-backed headers; resolved by operator into runtime Headers", - "openTelemetry.sensitiveHeaders.secretKeyRef.name": "K8s-secret-backed headers; resolved by operator into runtime Headers", - "openTelemetry.sensitiveHeaders.secretKeyRef.key": "K8s-secret-backed headers; resolved by operator into runtime Headers", - "openTelemetry.caBundleRef.configMapRef.name": "K8s ConfigMap reference; resolved by operator into runtime CACertPath", - "openTelemetry.caBundleRef.configMapRef.key": "K8s ConfigMap reference; resolved by operator into runtime CACertPath", - "openTelemetry.caBundleRef.configMapRef.optional": "K8s ConfigMap reference flag promoted from corev1.ConfigMapKeySelector; not part of runtime config", + "openTelemetry.enabled": "CRD-only gate; controls whether the converter populates runtime fields at all", + "openTelemetry.sensitiveHeaders.name": "K8s-secret-backed header value; injected as TOOLHIVE_OTEL_HEADER_* env vars on the proxyrunner pod " + + "(controllerutil.GenerateOpenTelemetryEnvVarsFromRef) and merged into OTLP headers at runtime; not written into telemetry.Config.Headers by the converter", + "openTelemetry.sensitiveHeaders.secretKeyRef.name": "K8s-secret-backed header value; injected as TOOLHIVE_OTEL_HEADER_* env vars on the proxyrunner pod " + + "(controllerutil.GenerateOpenTelemetryEnvVarsFromRef) and merged into OTLP headers at runtime; not written into telemetry.Config.Headers by the converter", + "openTelemetry.sensitiveHeaders.secretKeyRef.key": "K8s-secret-backed header value; injected as TOOLHIVE_OTEL_HEADER_* env vars on the proxyrunner pod " + + "(controllerutil.GenerateOpenTelemetryEnvVarsFromRef) and merged into OTLP headers at runtime; not written into telemetry.Config.Headers by the converter", + "openTelemetry.caBundleRef.configMapRef.name": "K8s ConfigMap reference; resolved by operator into runtime CACertPath", + "openTelemetry.caBundleRef.configMapRef.key": "K8s ConfigMap reference; resolved by operator into runtime CACertPath", + "openTelemetry.caBundleRef.configMapRef.optional": "K8s ConfigMap reference flag promoted from corev1.ConfigMapKeySelector; not part of runtime config", } // telemetryIgnoredOnRuntimeOnly lists runtime leaf fields that intentionally // have no CRD counterpart. Each entry MUST include a justification. var telemetryIgnoredOnRuntimeOnly = map[string]string{ - "serviceName": "per-server, set via MCPTelemetryConfigReference.ServiceName, not stored on the shared MCPTelemetryConfig", + "serviceName": "per-server, set from MCPTelemetryConfigReference.ServiceName and defaulted at runtime by " + + "telemetry.ResolveServiceName; intentionally absent from the shared MCPTelemetryConfig", "serviceVersion": "resolved at runtime from binary version (issue #2296)", "environmentVariables": "CLI-only, not applicable to CRD-managed telemetry", - "caCertPath": "filesystem path computed by the operator from caBundleRef; not user-facing in the CRD", + "caCertPath": "filesystem path injected by runconfig.AppendTelemetryRunnerOption after the operator computes the volume mount " + + "path from openTelemetry.caBundleRef; not user-facing in the CRD", } // TestTelemetryConfigDrift_CRDFieldsCovered walks MCPTelemetryConfigSpec and @@ -144,9 +149,12 @@ func TestTelemetryConfigDrift_MappingTableSanity(t *testing.T) { seenCRD := make(map[string]int, len(telemetryFieldMappings)) seenRuntime := make(map[string]int, len(telemetryFieldMappings)) + // Use require for the per-entry NotEmpty checks so that an empty CRD + // or Runtime field doesn't pollute the duplicate maps below with an + // empty-string key — that would trigger a misleading cascade. for i, m := range telemetryFieldMappings { - assert.NotEmptyf(t, m.CRD, "telemetryFieldMappings[%d].CRD must not be empty", i) - assert.NotEmptyf(t, m.Runtime, "telemetryFieldMappings[%d].Runtime must not be empty", i) + require.NotEmptyf(t, m.CRD, "telemetryFieldMappings[%d].CRD must not be empty", i) + require.NotEmptyf(t, m.Runtime, "telemetryFieldMappings[%d].Runtime must not be empty", i) seenCRD[m.CRD]++ seenRuntime[m.Runtime]++ } @@ -176,17 +184,49 @@ func TestTelemetryConfigDrift_MappingTableSanity(t *testing.T) { assert.NotEmptyf(t, reason, "telemetryIgnoredOnRuntimeOnly[%q] must include a justification", path) } - // Stable iteration not strictly necessary for correctness, but it keeps - // failure output deterministic when assertions fire on multiple keys. - _ = sortedKeys(seenCRD) - _ = sortedKeys(seenRuntime) + // A leaf path can't be classified two different ways. An entry in both + // ignore maps is a copy-paste mistake when shifting a field across the + // boundary — fail loudly instead of silently allowing the contradiction. + for path := range telemetryIgnoredOnCRDOnly { + if _, dup := telemetryIgnoredOnRuntimeOnly[path]; dup { + t.Errorf("path %q is listed in BOTH telemetryIgnoredOnCRDOnly and telemetryIgnoredOnRuntimeOnly", path) + } + } + + // Every path in the mapping/ignore tables must still be a live leaf on + // its respective type. Catches stale entries left behind by field + // renames or deletions, which would otherwise mask the rename. + crdLeaves := liveLeafSet(reflect.TypeOf(v1beta1.MCPTelemetryConfigSpec{})) + for _, m := range telemetryFieldMappings { + if _, live := crdLeaves[m.CRD]; !live { + t.Errorf("telemetryFieldMappings entry %q is not a live leaf on v1beta1.MCPTelemetryConfigSpec — stale entry?", m.CRD) + } + } + for path := range telemetryIgnoredOnCRDOnly { + if _, live := crdLeaves[path]; !live { + t.Errorf("telemetryIgnoredOnCRDOnly entry %q is not a live leaf on v1beta1.MCPTelemetryConfigSpec — stale entry?", path) + } + } + runtimeLeaves := liveLeafSet(reflect.TypeOf(telemetry.Config{})) + for _, m := range telemetryFieldMappings { + if _, live := runtimeLeaves[m.Runtime]; !live { + t.Errorf("telemetryFieldMappings entry %q is not a live leaf on telemetry.Config — stale entry?", m.Runtime) + } + } + for path := range telemetryIgnoredOnRuntimeOnly { + if _, live := runtimeLeaves[path]; !live { + t.Errorf("telemetryIgnoredOnRuntimeOnly entry %q is not a live leaf on telemetry.Config — stale entry?", path) + } + } } -func sortedKeys[V any](m map[string]V) []string { - out := make([]string, 0, len(m)) - for k := range m { - out = append(out, k) +// liveLeafSet returns the set of leaf paths reachable from t, for use in +// stale-entry checks against the drift mapping/ignore tables. +func liveLeafSet(t reflect.Type) map[string]struct{} { + leaves := testutil.FlattenJSONLeafFields(t) + out := make(map[string]struct{}, len(leaves)) + for _, l := range leaves { + out[l] = struct{}{} } - sort.Strings(out) return out } From e40df047e25dcde472e2fd59258164c0ac40a7f8 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 7 May 2026 01:23:20 +0100 Subject: [PATCH 3/6] Trim drift walker and its tests Simplify cycle detection: stop on first revisit instead of allowing one self-referential expansion. The "expand once" gold-plating produced a "next." entry on linked-list-shaped types, but no real CRD or runtime config has cyclic shape, and stop-on-revisit is the simpler contract for drift detection. The visited tracker becomes a map[reflect.Type]struct{} and maxStructRevisits goes away. Compress the FlattenJSONLeafFields godoc from a 9-bullet semantics list into one paragraph. Most of the bullets are now subsumed by the single "json.Marshaler => leaf, otherwise recurse on Struct/Slice/Array/Map" rule. The detailed encoding/json reference belongs in the standard library, not here. Drop redundant subtests in TestFlattenJSONLeafFields. After the json.Marshaler refactor, the json.RawMessage / thvjson.Map+Any / vmcpconfig.Duration cases all exercise the same short-circuit branch as metav1.Duration, so one Marshaler example is enough. Drop slice-of- pointer-to-struct (covered by combining the pointer-deref and slice-of- struct cases). Fold the dedicated PointerInputDereferenced test into the table. Remove TestFlattenJSONLeafFields_OutputIsSorted: the table- driven test already pins exact sorted-string equality on every case. Update the recursive-type expectation to ["name"] to lock in the new stop-on-revisit semantics. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/thv-operator/internal/testutil/reflect.go | 64 +++++------------ .../internal/testutil/reflect_test.go | 72 ++++--------------- 2 files changed, 29 insertions(+), 107 deletions(-) diff --git a/cmd/thv-operator/internal/testutil/reflect.go b/cmd/thv-operator/internal/testutil/reflect.go index 969a292d3e..6d02128e8f 100644 --- a/cmd/thv-operator/internal/testutil/reflect.go +++ b/cmd/thv-operator/internal/testutil/reflect.go @@ -17,35 +17,12 @@ import ( // FlattenJSONLeafFields returns every leaf JSON field path under t as a sorted // slice of dot-delimited paths (e.g. "openTelemetry.tracing.enabled"). // -// Walking semantics: -// - For struct fields, the JSON tag's name (before any comma) is used as the -// path segment. If the tag is "-", the field is skipped. If the tag is -// missing, the Go field name is used. -// - Unexported fields are skipped. -// - Fields with `,inline` flatten into the parent path with no prefix. -// - Pointer types are dereferenced and walked. -// - Struct types are recursed into. -// - Slice and array types whose ELEMENT type is a struct (or pointer to a -// struct) recurse into that element type, joining with the parent path. -// A path like "tools.workload" therefore means "every element of the -// `tools` slice has a `workload` field". -// - Map types whose VALUE type is a struct (or pointer to a struct) recurse -// into that value type the same way. -// - Types that implement json.Marshaler are treated as leaves regardless of -// their underlying kind. A custom MarshalJSON produces a JSON shape that -// bears no relation to the Go field layout, so walking the fields would -// emit paths that never appear in the serialized form. This handles -// metav1.Time, metav1.Duration, intstr.IntOrString, resource.Quantity, -// json.RawMessage, and any project-local custom-marshaled types -// (e.g. pkg/json.Map, pkg/json.Any, pkg/vmcp/config.Duration) without an -// explicit allow-list. -// - All other primitive types (primitives, []string, map[string]string, -// time.Duration which is just int64) terminate the walk and contribute -// one leaf path entry via the default branch. -// - Self-referential types are expanded one level (the original visit plus -// one self-reference) and then truncated, to keep the walk terminating. -// Subsequent levels (e.g. "next.next.") are intentionally elided. -// See maxStructRevisits for the controlling constant. +// A leaf is any type that does not produce nested JSON keys at runtime: a +// primitive, a slice/map of primitives, or a type implementing json.Marshaler +// (whose MarshalJSON shape is opaque to reflection). Structs, slices/maps of +// structs, and pointers to either are recursed into. Field names follow +// encoding/json rules including `,inline` and anonymous-field promotion. +// Self-referential types stop on revisit so the walk always terminates. // // If t is nil or, after dereferencing, is not a struct, an empty slice is // returned rather than panicking. @@ -61,7 +38,7 @@ func FlattenJSONLeafFields(t reflect.Type) []string { } leafSet := make(map[string]struct{}) - visited := map[reflect.Type]int{} + visited := map[reflect.Type]struct{}{} recurseStruct(t, "", leafSet, visited) out := make([]string, 0, len(leafSet)) @@ -96,7 +73,7 @@ var skipFieldTypes = map[reflect.Type]struct{}{ // visited is the set of struct types currently on the recursion stack; it is // used to break cycles on self-referential types. Callers add t to visited // before invoking this function and remove it afterwards. -func walkStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]int) { +func walkStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]struct{}) { for i := 0; i < t.NumField(); i++ { field := t.Field(i) // Skip unexported fields. PkgPath is non-empty for unexported fields. @@ -140,7 +117,7 @@ func walkStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visi // walkType walks a single type with the given accumulated prefix. It either // recurses into nested structs/slices/maps or records a leaf at prefix. -func walkType(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]int) { +func walkType(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]struct{}) { // Deref pointers. for t.Kind() == reflect.Pointer { t = t.Elem() @@ -185,25 +162,16 @@ func walkType(t reflect.Type, prefix string, leafSet map[string]struct{}, visite } } -// maxStructRevisits caps how many times a single struct type may appear on the -// recursion stack. A value of 2 means the type may be entered at most twice -// on the same path: the original entry plus one self-referential visit. This -// is enough for one level of "next." expansion in cyclic types like -// linked lists, while still guaranteeing the walk terminates. -const maxStructRevisits = 2 - // recurseStruct descends into a nested struct type with cycle protection. -// visited holds the count of how many times each struct type currently appears -// on the recursion stack. When entering t would exceed maxStructRevisits, the -// walk stops silently — no leaf is recorded, on the assumption that the -// useful leaves under t have already been captured by the earlier visit on -// the same path. -func recurseStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]int) { - if visited[t] >= maxStructRevisits { +// visited is the set of struct types currently on the recursion stack; if t +// is already present, the walk stops silently to keep self-referential types +// from looping forever. +func recurseStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visited map[reflect.Type]struct{}) { + if _, seen := visited[t]; seen { return } - visited[t]++ - defer func() { visited[t]-- }() + visited[t] = struct{}{} + defer delete(visited, t) walkStruct(t, prefix, leafSet, visited) } diff --git a/cmd/thv-operator/internal/testutil/reflect_test.go b/cmd/thv-operator/internal/testutil/reflect_test.go index 6a746a02cd..0a023730b4 100644 --- a/cmd/thv-operator/internal/testutil/reflect_test.go +++ b/cmd/thv-operator/internal/testutil/reflect_test.go @@ -4,18 +4,13 @@ package testutil import ( - "encoding/json" "reflect" - "sort" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - thvjson "github.com/stacklok/toolhive/pkg/json" - vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" ) // --- Fixture types --------------------------------------------------------- @@ -52,10 +47,6 @@ type withSliceOfStruct struct { Items []leafInner `json:"items"` } -type withSliceOfPointerStruct struct { - Items []*leafInner `json:"items"` -} - type withSliceOfPrimitive struct { Tags []string `json:"tags"` } @@ -90,24 +81,14 @@ type withUnexportedField struct { hidden string //nolint:unused // exercised by reflection } +// withDuration covers two leaf paths in one fixture: a primitive int64 +// (time.Duration, default branch) and a json.Marshaler (metav1.Duration, +// short-circuit branch). Other Marshaler types follow the same code path. type withDuration struct { Wait time.Duration `json:"wait"` WaitMeta metav1.Duration `json:"waitMeta"` } -type withRawJSON struct { - Raw json.RawMessage `json:"raw"` -} - -type withThvJSON struct { - M thvjson.Map `json:"m"` - A thvjson.Any `json:"a"` -} - -type withVmcpDuration struct { - D vmcpconfig.Duration `json:"d"` -} - type recursive struct { Name string `json:"name"` Next *recursive `json:"next,omitempty"` @@ -153,11 +134,6 @@ func TestFlattenJSONLeafFields(t *testing.T) { in: withSliceOfStruct{}, want: []string{"items.a", "items.b"}, }, - { - name: "slice of pointer to struct recurses into element", - in: withSliceOfPointerStruct{}, - want: []string{"items.a", "items.b"}, - }, { name: "slice of primitive terminates at slice", in: withSliceOfPrimitive{}, @@ -189,29 +165,22 @@ func TestFlattenJSONLeafFields(t *testing.T) { want: []string{"visible"}, }, { - name: "time.Duration and metav1.Duration are leaves", + // time.Duration is a primitive int64 (default branch); + // metav1.Duration is a json.Marshaler (short-circuit branch). + // Together these exercise both leaf-emission paths. + name: "primitive and json.Marshaler types are leaves", in: withDuration{}, want: []string{"wait", "waitMeta"}, }, { - name: "json.RawMessage is a leaf", - in: withRawJSON{}, - want: []string{"raw"}, - }, - { - name: "thvjson Map and Any are leaves", - in: withThvJSON{}, - want: []string{"a", "m"}, - }, - { - name: "vmcp config Duration is a leaf", - in: withVmcpDuration{}, - want: []string{"d"}, + name: "self-referential type stops on revisit", + in: recursive{}, + want: []string{"name"}, }, { - name: "self-referential type does not recurse infinitely", - in: recursive{}, - want: []string{"name", "next.name"}, + name: "pointer input is dereferenced", + in: &flatPrimitives{}, + want: []string{"count", "enabled", "name"}, }, } @@ -224,21 +193,6 @@ func TestFlattenJSONLeafFields(t *testing.T) { } } -func TestFlattenJSONLeafFields_PointerInputDereferenced(t *testing.T) { - t.Parallel() - - got := FlattenJSONLeafFields(reflect.TypeOf(&flatPrimitives{})) - require.Equal(t, []string{"count", "enabled", "name"}, got) -} - -func TestFlattenJSONLeafFields_OutputIsSorted(t *testing.T) { - t.Parallel() - - got := FlattenJSONLeafFields(reflect.TypeOf(flatPrimitives{})) - require.NotEmpty(t, got) - require.True(t, sort.StringsAreSorted(got), "expected sorted output, got %v", got) -} - func TestFlattenJSONLeafFields_NilOrNonStructReturnsEmpty(t *testing.T) { t.Parallel() From 9f982e5b0873196423b360cf0a281134321588e5 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Thu, 7 May 2026 14:29:08 +0100 Subject: [PATCH 4/6] Use //nolint:exhaustive for the kind switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit golangci-lint v2 doesn't honor the //exhaustive:ignore source directive in this configuration; the project-wide nolint convention works. The switch genuinely falls through to a default leaf branch for every Kind not listed (primitives, interfaces, channels, etc.) — the suppression is a confirmed false positive. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/thv-operator/internal/testutil/reflect.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/thv-operator/internal/testutil/reflect.go b/cmd/thv-operator/internal/testutil/reflect.go index 6d02128e8f..8f58e19486 100644 --- a/cmd/thv-operator/internal/testutil/reflect.go +++ b/cmd/thv-operator/internal/testutil/reflect.go @@ -133,7 +133,7 @@ func walkType(t reflect.Type, prefix string, leafSet map[string]struct{}, visite // Only Struct/Slice/Array/Map require recursion; every other Kind // (primitives, interfaces, channels, etc.) is a leaf path captured by // the default branch. - switch t.Kind() { //exhaustive:ignore + switch t.Kind() { //nolint:exhaustive // every other Kind falls through to the default leaf branch by design case reflect.Struct: recurseStruct(t, prefix, leafSet, visited) case reflect.Slice, reflect.Array: From e93a6f0066f6b21e4ed4032b8430555d30509668 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Fri, 8 May 2026 01:07:00 +0100 Subject: [PATCH 5/6] Address review feedback on drift detection framework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes from PR review: 1. Fix bogus symbol name in caCertPath justification (telemetry_drift_test.go). The string referenced runconfig.AppendTelemetryRunnerOption, which does not exist anywhere in the tree. The actual wiring lives at cmd/thv-operator/pkg/runconfig/telemetry.go:33 inside AddMCPTelemetryConfigRefOptions, where config.CACertPath is assigned from the operator-computed volume-mount path. 2. Drop the standalone "if inline { return \"\" }" branch in the path- segment builder (reflect.go). encoding/json does not recognize the ",inline" tag option — only ",omitempty" and ",string" are parsed; any other token is silently discarded. Flattening is driven entirely by StructField.Anonymous combined with the absence of an explicit JSON name. Today every ",inline" usage in the codebase happens to also be on an anonymous field, so the bug was masked, but a future named field tagged json:"foo,inline" would have been incorrectly flattened by the walker while encoding/json would emit "foo":{...}. Add a regression test (named_field_with_,inline_tag_stays_nested) to pin the corrected behavior, and simplify parseJSONTag to match. 3. Extract a generic AssertNoDrift[Runtime, CRD] helper (internal/testutil/drift.go). The previous three top-level test functions (CRDFieldsCovered, RuntimeFieldsCovered, MappingTableSanity) are now subtests run inside the helper, so a new domain (OIDC, vMCP, etc.) drops to a single TestXxxDrift function that supplies its three tables. The helper carries Domain through into failure messages so output stays as actionable as before. The FieldMapping type and liveLeafSet helper move to the testutil package to live alongside the harness. Net: ~130 LoC removed from telemetry_drift_test.go, ~220 LoC of reusable harness added in drift.go. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/thv-operator/internal/testutil/drift.go | 220 ++++++++++++++++++ cmd/thv-operator/internal/testutil/reflect.go | 39 ++-- .../internal/testutil/reflect_test.go | 16 ++ .../pkg/spectoconfig/telemetry_drift_test.go | 178 ++------------ 4 files changed, 270 insertions(+), 183 deletions(-) create mode 100644 cmd/thv-operator/internal/testutil/drift.go diff --git a/cmd/thv-operator/internal/testutil/drift.go b/cmd/thv-operator/internal/testutil/drift.go new file mode 100644 index 0000000000..a52680cd1f --- /dev/null +++ b/cmd/thv-operator/internal/testutil/drift.go @@ -0,0 +1,220 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// FieldMapping pairs a CRD-side leaf path with a runtime-side leaf path. +// Both sides must be present unless the field is in one of the ignore maps +// of the enclosing DriftSpec. +type FieldMapping struct { + CRD string + Runtime string +} + +// DriftSpec declares the bidirectional mapping between a CRD spec type and +// its runtime config counterpart. Every leaf path on either type must be +// classified into exactly one of three buckets: a Mappings entry, an +// IgnoredOnCRDOnly entry (with justification), or an IgnoredOnRuntimeOnly +// entry (with justification). AssertNoDrift fails the test when a leaf is +// unclassified, when a table entry is stale, or when the tables contradict +// themselves. +// +// Domain is used in failure messages and subtest names; pick something that +// matches the domain folder (e.g. "telemetry", "oidc", "vmcp"). +type DriftSpec struct { + Domain string + Mappings []FieldMapping + IgnoredOnCRDOnly map[string]string + IgnoredOnRuntimeOnly map[string]string +} + +// AssertNoDrift runs three subtests against spec: +// - CRDFieldsCovered: every CRD leaf is mapped or ignored +// - RuntimeFieldsCovered: every runtime leaf is mapped or ignored +// - MappingTableSanity: tables have no duplicates, empty entries, +// cross-pollination, or stale leaves +// +// Type parameter ordering is [Runtime, CRD] mirroring how a converter reads: +// it produces Runtime from CRD. Failure messages reference both type names so +// a developer who hits a failure can grep directly to the offending field. +// +// testutil.AssertNoDrift[telemetry.Config, v1beta1.MCPTelemetryConfigSpec]( +// t, +// testutil.DriftSpec{ +// Domain: "telemetry", +// Mappings: telemetryFieldMappings, +// IgnoredOnCRDOnly: telemetryIgnoredOnCRDOnly, +// IgnoredOnRuntimeOnly: telemetryIgnoredOnRuntimeOnly, +// }, +// ) +func AssertNoDrift[Runtime, CRD any](t *testing.T, spec DriftSpec) { + t.Helper() + + require.NotEmptyf(t, spec.Domain, "DriftSpec.Domain is required") + + var ( + crdType = reflect.TypeOf((*CRD)(nil)).Elem() + runtimeType = reflect.TypeOf((*Runtime)(nil)).Elem() + ) + + t.Run("CRDFieldsCovered", func(t *testing.T) { + t.Parallel() + assertSideCovered(t, crdType, sideCRD, spec) + }) + t.Run("RuntimeFieldsCovered", func(t *testing.T) { + t.Parallel() + assertSideCovered(t, runtimeType, sideRuntime, spec) + }) + t.Run("MappingTableSanity", func(t *testing.T) { + t.Parallel() + assertTableSanity(t, crdType, runtimeType, spec) + }) +} + +// side identifies which half of the boundary a leaf belongs to. Internal to +// the helper — callers don't see it. +type side int + +const ( + sideCRD side = iota + sideRuntime +) + +func (s side) typeLabel() string { + if s == sideCRD { + return "CRD" + } + return "Runtime" +} + +func assertSideCovered(t *testing.T, typ reflect.Type, s side, spec DriftSpec) { + t.Helper() + + mapped := make(map[string]struct{}, len(spec.Mappings)) + for _, m := range spec.Mappings { + switch s { + case sideCRD: + mapped[m.CRD] = struct{}{} + case sideRuntime: + mapped[m.Runtime] = struct{}{} + } + } + + ignored := spec.IgnoredOnCRDOnly + siblingTable := "IgnoredOnCRDOnly" + pairedSidePath := "Runtime" + if s == sideRuntime { + ignored = spec.IgnoredOnRuntimeOnly + siblingTable = "IgnoredOnRuntimeOnly" + pairedSidePath = "CRD" + } + + leaves := FlattenJSONLeafFields(typ) + for _, leaf := range leaves { + if _, ok := mapped[leaf]; ok { + continue + } + if _, ok := ignored[leaf]; ok { + continue + } + t.Errorf( + "%s drift: %s field %q on type %s is unclassified.\n"+ + "Action: add it to Mappings (paired with the corresponding %s path)\n"+ + " OR add it to %s with a justification string.", + spec.Domain, s.typeLabel(), leaf, typ.String(), pairedSidePath, siblingTable, + ) + } +} + +func assertTableSanity(t *testing.T, crdType, runtimeType reflect.Type, spec DriftSpec) { + t.Helper() + + seenCRD := make(map[string]int, len(spec.Mappings)) + seenRuntime := make(map[string]int, len(spec.Mappings)) + + // require so that an empty CRD/Runtime field doesn't pollute the dup + // maps below with empty-string keys (which would cause cascading + // misleading failures). + for i, m := range spec.Mappings { + require.NotEmptyf(t, m.CRD, "%s: Mappings[%d].CRD must not be empty", spec.Domain, i) + require.NotEmptyf(t, m.Runtime, "%s: Mappings[%d].Runtime must not be empty", spec.Domain, i) + seenCRD[m.CRD]++ + seenRuntime[m.Runtime]++ + } + + for path, count := range seenCRD { + assert.Equalf(t, 1, count, "%s: CRD path %q appears %d times in Mappings", spec.Domain, path, count) + } + for path, count := range seenRuntime { + assert.Equalf(t, 1, count, "%s: runtime path %q appears %d times in Mappings", spec.Domain, path, count) + } + + // Overlap between mapped and ignored on the same side. + for _, m := range spec.Mappings { + if _, dup := spec.IgnoredOnCRDOnly[m.CRD]; dup { + t.Errorf("%s: CRD path %q is both mapped and listed in IgnoredOnCRDOnly", spec.Domain, m.CRD) + } + if _, dup := spec.IgnoredOnRuntimeOnly[m.Runtime]; dup { + t.Errorf("%s: runtime path %q is both mapped and listed in IgnoredOnRuntimeOnly", spec.Domain, m.Runtime) + } + } + + // Justifications must be non-empty. + for path, reason := range spec.IgnoredOnCRDOnly { + assert.NotEmptyf(t, reason, "%s: IgnoredOnCRDOnly[%q] must include a justification", spec.Domain, path) + } + for path, reason := range spec.IgnoredOnRuntimeOnly { + assert.NotEmptyf(t, reason, "%s: IgnoredOnRuntimeOnly[%q] must include a justification", spec.Domain, path) + } + + // Cross-pollination: a path can't be classified two different ways. + for path := range spec.IgnoredOnCRDOnly { + if _, dup := spec.IgnoredOnRuntimeOnly[path]; dup { + t.Errorf("%s: path %q is listed in BOTH IgnoredOnCRDOnly and IgnoredOnRuntimeOnly", spec.Domain, path) + } + } + + // Stale-leaf check: every path in mapping/ignore tables must still be a + // live leaf on its respective type. Catches table entries left behind + // after a field rename or deletion, which would otherwise mask the + // change. + crdLeaves := liveLeafSet(crdType) + for _, m := range spec.Mappings { + if _, live := crdLeaves[m.CRD]; !live { + t.Errorf("%s: Mappings entry %q is not a live leaf on %s — stale entry?", spec.Domain, m.CRD, crdType.String()) + } + } + for path := range spec.IgnoredOnCRDOnly { + if _, live := crdLeaves[path]; !live { + t.Errorf("%s: IgnoredOnCRDOnly entry %q is not a live leaf on %s — stale entry?", spec.Domain, path, crdType.String()) + } + } + runtimeLeaves := liveLeafSet(runtimeType) + for _, m := range spec.Mappings { + if _, live := runtimeLeaves[m.Runtime]; !live { + t.Errorf("%s: Mappings entry %q is not a live leaf on %s — stale entry?", spec.Domain, m.Runtime, runtimeType.String()) + } + } + for path := range spec.IgnoredOnRuntimeOnly { + if _, live := runtimeLeaves[path]; !live { + t.Errorf("%s: IgnoredOnRuntimeOnly entry %q is not a live leaf on %s — stale entry?", spec.Domain, path, runtimeType.String()) + } + } +} + +func liveLeafSet(t reflect.Type) map[string]struct{} { + leaves := FlattenJSONLeafFields(t) + out := make(map[string]struct{}, len(leaves)) + for _, l := range leaves { + out[l] = struct{}{} + } + return out +} diff --git a/cmd/thv-operator/internal/testutil/reflect.go b/cmd/thv-operator/internal/testutil/reflect.go index 8f58e19486..a781e96d48 100644 --- a/cmd/thv-operator/internal/testutil/reflect.go +++ b/cmd/thv-operator/internal/testutil/reflect.go @@ -89,19 +89,19 @@ func walkStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, visi continue } - name, inline, omit := parseJSONTag(field) + name, omit := parseJSONTag(field) if omit { continue } - // Determine the path segment for this field. + // Determine the path segment for this field. Match encoding/json + // semantics: only anonymous embedded fields are flattened, and only + // when they have no explicit JSON name. The `,inline` tag option is + // a Kubernetes documentation idiom — encoding/json itself ignores + // it, so a NAMED field tagged `json:"foo,inline"` actually marshals + // as `"foo":{...}` and must NOT be flattened by the walker. segment := func() string { - // Anonymous fields are inlined by encoding/json when they have no - // json name (or an explicit `,inline` tag). Treat them as inline. - if field.Anonymous && (name == "" || inline) { - return "" - } - if inline { + if field.Anonymous && name == "" { return "" } if name == "" { @@ -175,27 +175,22 @@ func recurseStruct(t reflect.Type, prefix string, leafSet map[string]struct{}, v walkStruct(t, prefix, leafSet, visited) } -// parseJSONTag extracts (name, inline, omit) from the json struct tag. +// parseJSONTag extracts (name, omit) from the json struct tag. // - name is the explicit JSON name (empty when not specified). -// - inline is true when the tag includes ",inline". // - omit is true when the tag is "-" (field must be skipped entirely). -func parseJSONTag(field reflect.StructField) (string, bool, bool) { +// +// Other tag options (`,omitempty`, `,inline`, `,string`, ...) are +// intentionally ignored — they don't affect the path enumeration. +func parseJSONTag(field reflect.StructField) (string, bool) { tag, ok := field.Tag.Lookup("json") if !ok { - return "", false, false + return "", false } if tag == "-" { - return "", false, true - } - parts := strings.Split(tag, ",") - name := parts[0] - inline := false - for _, p := range parts[1:] { - if p == "inline" { - inline = true - } + return "", true } - return name, inline, false + name, _, _ := strings.Cut(tag, ",") + return name, false } // joinPath joins prefix and segment with a dot, handling empty segments diff --git a/cmd/thv-operator/internal/testutil/reflect_test.go b/cmd/thv-operator/internal/testutil/reflect_test.go index 0a023730b4..ca775199cf 100644 --- a/cmd/thv-operator/internal/testutil/reflect_test.go +++ b/cmd/thv-operator/internal/testutil/reflect_test.go @@ -94,6 +94,15 @@ type recursive struct { Next *recursive `json:"next,omitempty"` } +// withNamedInlineTag pins encoding/json semantics: the `,inline` token is a +// Kubernetes documentation idiom and is ignored by encoding/json. A NAMED +// field with `json:"inner,inline"` must marshal as `"inner":{...}` (nested), +// never flattened — and the walker must reflect that. +type withNamedInlineTag struct { + Inner leafInner `json:"inner,inline"` //nolint:revive // inline is a valid kubernetes json tag option + Top string `json:"top"` +} + // --- Tests ----------------------------------------------------------------- func TestFlattenJSONLeafFields(t *testing.T) { @@ -159,6 +168,13 @@ func TestFlattenJSONLeafFields(t *testing.T) { in: withEmbeddedInline{}, want: []string{"bar", "foo"}, }, + { + // encoding/json ignores `,inline`; only anonymous embedding + // flattens. A named field tagged `,inline` must stay nested. + name: "named field with ,inline tag stays nested", + in: withNamedInlineTag{}, + want: []string{"inner.a", "inner.b", "top"}, + }, { name: "unexported field is skipped", in: withUnexportedField{}, diff --git a/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go b/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go index 1c4359469d..44f7b44ab2 100644 --- a/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go +++ b/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go @@ -20,33 +20,22 @@ package spectoconfig // runtime side, with a justification. // // When either side gains or loses a field, exactly one of these tables must -// be updated. The "Mapping table sanity" test guards the tables themselves -// (no duplicates, no empty entries, no overlap with the ignore lists). +// be updated. testutil.AssertNoDrift handles the verification — see its +// godoc for the failure modes covered. import ( - "reflect" "testing" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" "github.com/stacklok/toolhive/cmd/thv-operator/internal/testutil" "github.com/stacklok/toolhive/pkg/telemetry" ) -// FieldMapping pairs a CRD-side leaf path with a runtime-side leaf path. Both -// sides must be present unless the field is in one of the ignore maps. -type FieldMapping struct { - CRD string - Runtime string -} - // telemetryFieldMappings is the source of truth for CRD<->runtime field // links. One entry per leaf field that exists on both sides. The CRD path is // rooted at MCPTelemetryConfigSpec; the runtime path is rooted at // telemetry.Config. -var telemetryFieldMappings = []FieldMapping{ +var telemetryFieldMappings = []testutil.FieldMapping{ {CRD: "openTelemetry.endpoint", Runtime: "endpoint"}, {CRD: "openTelemetry.insecure", Runtime: "insecure"}, {CRD: "openTelemetry.headers", Runtime: "headers"}, @@ -80,153 +69,20 @@ var telemetryIgnoredOnRuntimeOnly = map[string]string{ "telemetry.ResolveServiceName; intentionally absent from the shared MCPTelemetryConfig", "serviceVersion": "resolved at runtime from binary version (issue #2296)", "environmentVariables": "CLI-only, not applicable to CRD-managed telemetry", - "caCertPath": "filesystem path injected by runconfig.AppendTelemetryRunnerOption after the operator computes the volume mount " + - "path from openTelemetry.caBundleRef; not user-facing in the CRD", -} - -// TestTelemetryConfigDrift_CRDFieldsCovered walks MCPTelemetryConfigSpec and -// requires every leaf path to appear either as a CRD entry in -// telemetryFieldMappings or as a key in telemetryIgnoredOnCRDOnly. -func TestTelemetryConfigDrift_CRDFieldsCovered(t *testing.T) { - t.Parallel() - - mappedCRD := make(map[string]struct{}, len(telemetryFieldMappings)) - for _, m := range telemetryFieldMappings { - mappedCRD[m.CRD] = struct{}{} - } - - leaves := testutil.FlattenJSONLeafFields(reflect.TypeOf(v1beta1.MCPTelemetryConfigSpec{})) - for _, leaf := range leaves { - if _, ok := mappedCRD[leaf]; ok { - continue - } - if _, ok := telemetryIgnoredOnCRDOnly[leaf]; ok { - continue - } - t.Errorf( - "v1beta1.MCPTelemetryConfigSpec field %q is unclassified.\n"+ - "Action: add it to telemetryFieldMappings (with the corresponding telemetry.Config path)\n"+ - " OR add it to telemetryIgnoredOnCRDOnly with a justification string.", - leaf, - ) - } -} - -// TestTelemetryConfigDrift_RuntimeFieldsCovered walks telemetry.Config and -// requires every leaf path to appear either as a Runtime entry in -// telemetryFieldMappings or as a key in telemetryIgnoredOnRuntimeOnly. -func TestTelemetryConfigDrift_RuntimeFieldsCovered(t *testing.T) { - t.Parallel() - - mappedRuntime := make(map[string]struct{}, len(telemetryFieldMappings)) - for _, m := range telemetryFieldMappings { - mappedRuntime[m.Runtime] = struct{}{} - } - - leaves := testutil.FlattenJSONLeafFields(reflect.TypeOf(telemetry.Config{})) - for _, leaf := range leaves { - if _, ok := mappedRuntime[leaf]; ok { - continue - } - if _, ok := telemetryIgnoredOnRuntimeOnly[leaf]; ok { - continue - } - t.Errorf( - "telemetry.Config field %q is unclassified.\n"+ - "Action: add it to telemetryFieldMappings (with the corresponding MCPTelemetryConfigSpec path)\n"+ - " OR add it to telemetryIgnoredOnRuntimeOnly with a justification string.", - leaf, - ) - } -} - -// TestTelemetryConfigDrift_MappingTableSanity guards the mapping tables -// themselves. It catches mistakes like duplicate paths, empty entries, and -// overlap between mapped and ignored fields. -func TestTelemetryConfigDrift_MappingTableSanity(t *testing.T) { - t.Parallel() - - seenCRD := make(map[string]int, len(telemetryFieldMappings)) - seenRuntime := make(map[string]int, len(telemetryFieldMappings)) - - // Use require for the per-entry NotEmpty checks so that an empty CRD - // or Runtime field doesn't pollute the duplicate maps below with an - // empty-string key — that would trigger a misleading cascade. - for i, m := range telemetryFieldMappings { - require.NotEmptyf(t, m.CRD, "telemetryFieldMappings[%d].CRD must not be empty", i) - require.NotEmptyf(t, m.Runtime, "telemetryFieldMappings[%d].Runtime must not be empty", i) - seenCRD[m.CRD]++ - seenRuntime[m.Runtime]++ - } - - for path, count := range seenCRD { - assert.Equalf(t, 1, count, "CRD path %q appears %d times in telemetryFieldMappings", path, count) - } - for path, count := range seenRuntime { - assert.Equalf(t, 1, count, "runtime path %q appears %d times in telemetryFieldMappings", path, count) - } - - // Overlap with ignore lists. - for _, m := range telemetryFieldMappings { - if _, dup := telemetryIgnoredOnCRDOnly[m.CRD]; dup { - t.Errorf("CRD path %q is both mapped and listed in telemetryIgnoredOnCRDOnly", m.CRD) - } - if _, dup := telemetryIgnoredOnRuntimeOnly[m.Runtime]; dup { - t.Errorf("runtime path %q is both mapped and listed in telemetryIgnoredOnRuntimeOnly", m.Runtime) - } - } - - // Justifications must be non-empty. - for path, reason := range telemetryIgnoredOnCRDOnly { - assert.NotEmptyf(t, reason, "telemetryIgnoredOnCRDOnly[%q] must include a justification", path) - } - for path, reason := range telemetryIgnoredOnRuntimeOnly { - assert.NotEmptyf(t, reason, "telemetryIgnoredOnRuntimeOnly[%q] must include a justification", path) - } - - // A leaf path can't be classified two different ways. An entry in both - // ignore maps is a copy-paste mistake when shifting a field across the - // boundary — fail loudly instead of silently allowing the contradiction. - for path := range telemetryIgnoredOnCRDOnly { - if _, dup := telemetryIgnoredOnRuntimeOnly[path]; dup { - t.Errorf("path %q is listed in BOTH telemetryIgnoredOnCRDOnly and telemetryIgnoredOnRuntimeOnly", path) - } - } - - // Every path in the mapping/ignore tables must still be a live leaf on - // its respective type. Catches stale entries left behind by field - // renames or deletions, which would otherwise mask the rename. - crdLeaves := liveLeafSet(reflect.TypeOf(v1beta1.MCPTelemetryConfigSpec{})) - for _, m := range telemetryFieldMappings { - if _, live := crdLeaves[m.CRD]; !live { - t.Errorf("telemetryFieldMappings entry %q is not a live leaf on v1beta1.MCPTelemetryConfigSpec — stale entry?", m.CRD) - } - } - for path := range telemetryIgnoredOnCRDOnly { - if _, live := crdLeaves[path]; !live { - t.Errorf("telemetryIgnoredOnCRDOnly entry %q is not a live leaf on v1beta1.MCPTelemetryConfigSpec — stale entry?", path) - } - } - runtimeLeaves := liveLeafSet(reflect.TypeOf(telemetry.Config{})) - for _, m := range telemetryFieldMappings { - if _, live := runtimeLeaves[m.Runtime]; !live { - t.Errorf("telemetryFieldMappings entry %q is not a live leaf on telemetry.Config — stale entry?", m.Runtime) - } - } - for path := range telemetryIgnoredOnRuntimeOnly { - if _, live := runtimeLeaves[path]; !live { - t.Errorf("telemetryIgnoredOnRuntimeOnly entry %q is not a live leaf on telemetry.Config — stale entry?", path) - } - } + "caCertPath": "filesystem path assigned by runconfig.AddMCPTelemetryConfigRefOptions (cmd/thv-operator/pkg/runconfig/telemetry.go) " + + "after the operator computes the volume-mount path from openTelemetry.caBundleRef; not user-facing in the CRD", } -// liveLeafSet returns the set of leaf paths reachable from t, for use in -// stale-entry checks against the drift mapping/ignore tables. -func liveLeafSet(t reflect.Type) map[string]struct{} { - leaves := testutil.FlattenJSONLeafFields(t) - out := make(map[string]struct{}, len(leaves)) - for _, l := range leaves { - out[l] = struct{}{} - } - return out +// TestTelemetryConfigDrift exercises the full bidirectional drift contract +// between v1beta1.MCPTelemetryConfigSpec (CRD) and telemetry.Config (runtime) +// via the shared testutil harness. To extend to another domain (OIDC, vMCP, +// etc.), add the three tables in that domain's package and call +// AssertNoDrift with the matching type parameters. +func TestTelemetryConfigDrift(t *testing.T) { + testutil.AssertNoDrift[telemetry.Config, v1beta1.MCPTelemetryConfigSpec](t, testutil.DriftSpec{ + Domain: "telemetry", + Mappings: telemetryFieldMappings, + IgnoredOnCRDOnly: telemetryIgnoredOnCRDOnly, + IgnoredOnRuntimeOnly: telemetryIgnoredOnRuntimeOnly, + }) } From bf56f7d185e6a6999c6b0dfb8e04e2b47218395b Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Fri, 8 May 2026 01:19:17 +0100 Subject: [PATCH 6/6] Fix lint failures on drift detection framework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI flagged two issues on the previous push: 1. cyclomatic complexity 19 of `assertTableSanity` (limit 15). Extract the five independent invariants — empty/duplicate mappings, mapped- and-ignored overlap, justification non-emptiness, cross-pollination, and stale-entry detection — into named helpers. The orchestrator becomes a six-line dispatch and each helper is small enough to read in one screen. The stale-entry helper is parameterized by side so the CRD and runtime passes share one body. 2. paralleltest on TestTelemetryConfigDrift. Add t.Parallel() to the outer test; subtests inside AssertNoDrift already call it. Also unrelated to this PR: TestGetOverlayMounts in pkg/ignore is a known intermittent flake (race), not introduced by these changes. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/thv-operator/internal/testutil/drift.go | 80 +++++++++++++------ .../pkg/spectoconfig/telemetry_drift_test.go | 1 + 2 files changed, 55 insertions(+), 26 deletions(-) diff --git a/cmd/thv-operator/internal/testutil/drift.go b/cmd/thv-operator/internal/testutil/drift.go index a52680cd1f..7711e35e33 100644 --- a/cmd/thv-operator/internal/testutil/drift.go +++ b/cmd/thv-operator/internal/testutil/drift.go @@ -136,28 +136,41 @@ func assertSideCovered(t *testing.T, typ reflect.Type, s side, spec DriftSpec) { func assertTableSanity(t *testing.T, crdType, runtimeType reflect.Type, spec DriftSpec) { t.Helper() + assertNoEmptyOrDuplicateMappings(t, spec) + assertNoMappedAndIgnored(t, spec) + assertJustificationsNonEmpty(t, spec) + assertNoCrossPollination(t, spec) + assertNoStaleEntries(t, sideCRD, crdType, spec) + assertNoStaleEntries(t, sideRuntime, runtimeType, spec) +} +// assertNoEmptyOrDuplicateMappings requires both halves of every Mappings +// entry to be non-empty (using require so an empty key doesn't pollute the +// duplicate-detection map with cascading failures) and asserts that no path +// on either side appears more than once. +func assertNoEmptyOrDuplicateMappings(t *testing.T, spec DriftSpec) { + t.Helper() seenCRD := make(map[string]int, len(spec.Mappings)) seenRuntime := make(map[string]int, len(spec.Mappings)) - - // require so that an empty CRD/Runtime field doesn't pollute the dup - // maps below with empty-string keys (which would cause cascading - // misleading failures). for i, m := range spec.Mappings { require.NotEmptyf(t, m.CRD, "%s: Mappings[%d].CRD must not be empty", spec.Domain, i) require.NotEmptyf(t, m.Runtime, "%s: Mappings[%d].Runtime must not be empty", spec.Domain, i) seenCRD[m.CRD]++ seenRuntime[m.Runtime]++ } - for path, count := range seenCRD { assert.Equalf(t, 1, count, "%s: CRD path %q appears %d times in Mappings", spec.Domain, path, count) } for path, count := range seenRuntime { assert.Equalf(t, 1, count, "%s: runtime path %q appears %d times in Mappings", spec.Domain, path, count) } +} - // Overlap between mapped and ignored on the same side. +// assertNoMappedAndIgnored fails if a Mappings entry's CRD or Runtime path +// also appears in the corresponding ignore map. A path can be classified as +// mapped or as ignored, never both. +func assertNoMappedAndIgnored(t *testing.T, spec DriftSpec) { + t.Helper() for _, m := range spec.Mappings { if _, dup := spec.IgnoredOnCRDOnly[m.CRD]; dup { t.Errorf("%s: CRD path %q is both mapped and listed in IgnoredOnCRDOnly", spec.Domain, m.CRD) @@ -166,46 +179,61 @@ func assertTableSanity(t *testing.T, crdType, runtimeType reflect.Type, spec Dri t.Errorf("%s: runtime path %q is both mapped and listed in IgnoredOnRuntimeOnly", spec.Domain, m.Runtime) } } +} - // Justifications must be non-empty. +// assertJustificationsNonEmpty checks every entry in both ignore maps has +// a non-empty justification string. +func assertJustificationsNonEmpty(t *testing.T, spec DriftSpec) { + t.Helper() for path, reason := range spec.IgnoredOnCRDOnly { assert.NotEmptyf(t, reason, "%s: IgnoredOnCRDOnly[%q] must include a justification", spec.Domain, path) } for path, reason := range spec.IgnoredOnRuntimeOnly { assert.NotEmptyf(t, reason, "%s: IgnoredOnRuntimeOnly[%q] must include a justification", spec.Domain, path) } +} - // Cross-pollination: a path can't be classified two different ways. +// assertNoCrossPollination fails if a path is classified as ignored on both +// sides — usually a copy-paste mistake when shifting a field across the +// boundary. +func assertNoCrossPollination(t *testing.T, spec DriftSpec) { + t.Helper() for path := range spec.IgnoredOnCRDOnly { if _, dup := spec.IgnoredOnRuntimeOnly[path]; dup { t.Errorf("%s: path %q is listed in BOTH IgnoredOnCRDOnly and IgnoredOnRuntimeOnly", spec.Domain, path) } } +} - // Stale-leaf check: every path in mapping/ignore tables must still be a - // live leaf on its respective type. Catches table entries left behind - // after a field rename or deletion, which would otherwise mask the - // change. - crdLeaves := liveLeafSet(crdType) - for _, m := range spec.Mappings { - if _, live := crdLeaves[m.CRD]; !live { - t.Errorf("%s: Mappings entry %q is not a live leaf on %s — stale entry?", spec.Domain, m.CRD, crdType.String()) +// assertNoStaleEntries fails for any mapping/ignore-table entry that is no +// longer a live leaf on the given type. Catches entries left behind after a +// field rename or deletion, which would otherwise mask the change. +func assertNoStaleEntries(t *testing.T, s side, typ reflect.Type, spec DriftSpec) { + t.Helper() + leaves := liveLeafSet(typ) + + mappingPath := func(m FieldMapping) string { + if s == sideCRD { + return m.CRD } + return m.Runtime } - for path := range spec.IgnoredOnCRDOnly { - if _, live := crdLeaves[path]; !live { - t.Errorf("%s: IgnoredOnCRDOnly entry %q is not a live leaf on %s — stale entry?", spec.Domain, path, crdType.String()) - } + ignoreTable := spec.IgnoredOnCRDOnly + ignoreLabel := "IgnoredOnCRDOnly" + if s == sideRuntime { + ignoreTable = spec.IgnoredOnRuntimeOnly + ignoreLabel = "IgnoredOnRuntimeOnly" } - runtimeLeaves := liveLeafSet(runtimeType) + for _, m := range spec.Mappings { - if _, live := runtimeLeaves[m.Runtime]; !live { - t.Errorf("%s: Mappings entry %q is not a live leaf on %s — stale entry?", spec.Domain, m.Runtime, runtimeType.String()) + path := mappingPath(m) + if _, live := leaves[path]; !live { + t.Errorf("%s: Mappings entry %q is not a live leaf on %s — stale entry?", spec.Domain, path, typ.String()) } } - for path := range spec.IgnoredOnRuntimeOnly { - if _, live := runtimeLeaves[path]; !live { - t.Errorf("%s: IgnoredOnRuntimeOnly entry %q is not a live leaf on %s — stale entry?", spec.Domain, path, runtimeType.String()) + for path := range ignoreTable { + if _, live := leaves[path]; !live { + t.Errorf("%s: %s entry %q is not a live leaf on %s — stale entry?", spec.Domain, ignoreLabel, path, typ.String()) } } } diff --git a/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go b/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go index 44f7b44ab2..acfff97d1b 100644 --- a/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go +++ b/cmd/thv-operator/pkg/spectoconfig/telemetry_drift_test.go @@ -79,6 +79,7 @@ var telemetryIgnoredOnRuntimeOnly = map[string]string{ // etc.), add the three tables in that domain's package and call // AssertNoDrift with the matching type parameters. func TestTelemetryConfigDrift(t *testing.T) { + t.Parallel() testutil.AssertNoDrift[telemetry.Config, v1beta1.MCPTelemetryConfigSpec](t, testutil.DriftSpec{ Domain: "telemetry", Mappings: telemetryFieldMappings,