Merge pull request #2584 from machine424/rrr

OCPBUGS-54516: provide context-rich and case-sensitive config validation

Author: openshift-merge-bot[bot]

go.mod

@@ -42,6 +42,7 @@ require (
 	k8s.io/metrics v0.31.1
 	k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738
 	sigs.k8s.io/controller-runtime v0.19.0
+	sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3
 	sigs.k8s.io/yaml v1.4.0
 )
 
@@ -147,7 +148,6 @@ require (
 	k8s.io/kms v0.32.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f // indirect
 	sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.31.0 // indirect
-	sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3 // indirect
 	sigs.k8s.io/kube-storage-version-migrator v0.0.6-0.20230721195810-5c8923c5ff96 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.2 // indirect
 )

pkg/configvalidate/configvalidate_test.go

@@ -88,7 +88,7 @@ func TestHandle(t *testing.T) {
 				},
 			},
 			allowed: false,
-			message: "failed to parse data at key \"config.yaml\": error unmarshaling JSON: while decoding JSON: json: unknown field \"prometheus_operator\"",
+			message: "failed to parse data at key \"config.yaml\": error unmarshaling: unknown field \"prometheus_operator\"",
 		},
 		{
 			name: "non monitoring configmap",

pkg/manifests/config.go

@@ -27,10 +27,12 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	"github.com/prometheus/common/model"
 	v1 "k8s.io/api/core/v1"
-	k8syaml "k8s.io/apimachinery/pkg/util/yaml"
+	jsonutil "k8s.io/apimachinery/pkg/util/json"
 	auditv1 "k8s.io/apiserver/pkg/apis/audit/v1"
 	"k8s.io/klog/v2"
 	"k8s.io/utils/ptr"
+	kjson "sigs.k8s.io/json"
+	kyaml "sigs.k8s.io/yaml"
 
 	"github.com/openshift/cluster-monitoring-operator/pkg/metrics"
 )
@@ -313,10 +315,51 @@ func (cps CollectionProfiles) String() string {
 	return sb.String()
 }
 
+// Copied from k8s.io/apimachinery/pkg/util/yaml.UnmarshalStrict but using
+// sigs.k8s.io/json.UnmarshalStrict instead of encoding/json.UnmarshalStrict
+// to enforce case-sensitive unmarshalling and provide more detailed error context.
+// This also allows for simpler error messages.
+func UnmarshalStrict(data []byte, v interface{}) error {
+	unmarshalStrict := func(yamlBytes []byte, obj interface{}) error {
+		jsonBytes, err := kyaml.YAMLToJSONStrict(yamlBytes)
+		if err != nil {
+			return err
+		}
+		strictErrs, err := kjson.UnmarshalStrict(jsonBytes, obj)
+		if err != nil {
+			return fmt.Errorf("error unmarshaling: %w", err)
+		}
+		if len(strictErrs) != 0 {
+			return fmt.Errorf("error unmarshaling: %w", errors.Join(strictErrs...))
+		}
+		return nil
+	}
+	// Kept for backward compatibility.
+	switch v := v.(type) {
+	case *map[string]interface{}:
+		if err := unmarshalStrict(data, v); err != nil {
+			return err
+		}
+		return jsonutil.ConvertMapNumbers(*v, 0)
+	case *[]interface{}:
+		if err := unmarshalStrict(data, v); err != nil {
+			return err
+		}
+		return jsonutil.ConvertSliceNumbers(*v, 0)
+	case *interface{}:
+		if err := unmarshalStrict(data, v); err != nil {
+			return err
+		}
+		return jsonutil.ConvertInterfaceNumbers(v, 0)
+	default:
+		return unmarshalStrict(data, v)
+	}
+}
+
 func newConfig(content []byte, collectionProfilesFeatureGateEnabled bool) (*Config, error) {
 	c := Config{CollectionProfilesFeatureGateEnabled: collectionProfilesFeatureGateEnabled}
 	cmc := defaultClusterMonitoringConfiguration()
-	err := k8syaml.UnmarshalStrict(content, &cmc)
+	err := UnmarshalStrict(content, &cmc)
 	if err != nil {
 		return nil, err
 	}
@@ -679,7 +722,7 @@ func NewUserConfigFromString(content string) (*UserWorkloadConfiguration, error)
 		return NewDefaultUserWorkloadMonitoringConfig(), nil
 	}
 	u := &UserWorkloadConfiguration{}
-	err := k8syaml.UnmarshalStrict([]byte(content), &u)
+	err := UnmarshalStrict([]byte(content), &u)
 	if err != nil {
 		return nil, err
 	}

pkg/manifests/config_test.go

@@ -29,7 +29,7 @@ func TestNewConfigFromString(t *testing.T) {
 	tcs := []struct {
 		name         string
 		configString func() string
-		shouldFail   bool
+		err          string
 		configCheck  func(*Config)
 	}{
 		{
@@ -54,22 +54,29 @@ func TestNewConfigFromString(t *testing.T) {
 			configString: func() string {
 				return `{"prometheusK8ss": {}}`
 			},
-			shouldFail: true,
+			err: "error unmarshaling: unknown field \"prometheusK8ss\"",
+		},
+		{
+			name: "json string with root field of the wrong case",
+			configString: func() string {
+				return `{"PROMETHEUSK8S": {}}`
+			},
+			err: "error unmarshaling: unknown field \"PROMETHEUSK8S\"",
 		},
 		{
 			name: "json string with unknown field",
 			configString: func() string {
 				return `{"prometheusK8s": {"unknown": "bar"}}`
 			},
-			shouldFail: true,
+			err: "error unmarshaling: unknown field \"prometheusK8s.unknown\"",
 		},
 		{
 			name: "json string with duplicated field",
 			// users should be aware of this as unmarshalling would only take one part into account.
 			configString: func() string {
 				return `{"prometheusK8s": {"foo": {}}, "prometheusK8s": {"bar": {}}}`
 			},
-			shouldFail: true,
+			err: "yaml: unmarshal errors:\n  line 1: key \"prometheusK8s\" already set in map",
 		},
 		{
 			name: "empty json string",
@@ -88,7 +95,14 @@ func TestNewConfigFromString(t *testing.T) {
 			configString: func() string {
 				return `metricsServe:`
 			},
-			shouldFail: true,
+			err: "error unmarshaling: unknown field \"metricsServe\"",
+		},
+		{
+			name: "yaml string with root field of the wrong case",
+			configString: func() string {
+				return `metricserver:`
+			},
+			err: "error unmarshaling: unknown field \"metricserver\"",
 		},
 		{
 			name: "yaml string with unknown field",
@@ -97,7 +111,7 @@ func TestNewConfigFromString(t *testing.T) {
 metricsServer:
   unknown:`
 			},
-			shouldFail: true,
+			err: "error unmarshaling: unknown field \"metricsServer.unknown\"",
 		},
 		{
 			name: "yaml string with duplicated field",
@@ -108,7 +122,13 @@ metricsServer:
 metricsServer:
   bar:`
 			},
-			shouldFail: true,
+			err: "yaml: unmarshal errors:\n  line 5: key \"metricsServer\" already set in map",
+		},
+		{
+			name: "empty yaml string",
+			configString: func() string {
+				return ``
+			},
 		},
 		{
 			name: "empty yaml string",
@@ -121,8 +141,8 @@ metricsServer:
 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {
 			c, err := NewConfigFromString(tc.configString(), false)
-			if tc.shouldFail {
-				require.Error(t, err)
+			if tc.err != "" {
+				require.ErrorContains(t, err, tc.err)
 				return
 			}
 			require.NoError(t, err)
@@ -137,7 +157,7 @@ func TestNewUserConfigFromString(t *testing.T) {
 	tcs := []struct {
 		name         string
 		configString func() string
-		shouldFail   bool
+		err          string
 		configCheck  func(*UserWorkloadConfiguration)
 	}{
 		{
@@ -163,22 +183,29 @@ func TestNewUserConfigFromString(t *testing.T) {
 			configString: func() string {
 				return `{"unknown": {}}`
 			},
-			shouldFail: true,
+			err: "error unmarshaling: unknown field \"unknown\"",
 		},
 		{
 			name: "json string with unknown field",
 			configString: func() string {
 				return `{"prometheusOperator": {"unknown": "bar"}}`
 			},
-			shouldFail: true,
+			err: "error unmarshaling: unknown field \"prometheusOperator.unknown\"",
+		},
+		{
+			name: "json string with field of wrong case",
+			configString: func() string {
+				return `{"prometheusOperator": {"nodeselector": ""}}`
+			},
+			err: "error unmarshaling: unknown field \"prometheusOperator.nodeselector\"",
 		},
 		{
 			name: "json string with duplicated field",
 			// users should be aware of this as unmarshalling would only take one part into account.
 			configString: func() string {
 				return `{"prometheus": {"foo": {}}, "prometheus": {"bar": {}}}`
 			},
-			shouldFail: true,
+			err: "yaml: unmarshal errors:\n  line 1: key \"prometheus\"",
 		},
 		{
 			name: "empty json string",
@@ -197,7 +224,7 @@ func TestNewUserConfigFromString(t *testing.T) {
 			configString: func() string {
 				return `unknown:`
 			},
-			shouldFail: true,
+			err: "error unmarshaling: unknown field \"unknown\"",
 		},
 		{
 			name: "yaml string with unknown field",
@@ -206,7 +233,16 @@ func TestNewUserConfigFromString(t *testing.T) {
 prometheusOperator:
   unknown:`
 			},
-			shouldFail: true,
+			err: "error unmarshaling: unknown field \"prometheusOperator.unknown\"",
+		},
+		{
+			name: "yaml string with field of wrong case",
+			configString: func() string {
+				return `
+prometheusOperator:
+  nodeselector:`
+			},
+			err: "error unmarshaling: unknown field \"prometheusOperator.nodeselector\"",
 		},
 		{
 			name: "yaml string with duplicated field",
@@ -217,7 +253,7 @@ thanosRuler:
 thanosRuler:
   bar:`
 			},
-			shouldFail: true,
+			err: "yaml: unmarshal errors:\n  line 5: key \"thanosRuler\"",
 		},
 		{
 			name: "empty yaml string",
@@ -230,8 +266,8 @@ thanosRuler:
 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {
 			c, err := NewUserConfigFromString(tc.configString())
-			if tc.shouldFail {
-				require.Error(t, err)
+			if tc.err != "" {
+				require.ErrorContains(t, err, tc.err)
 				return
 			}
 			require.NoError(t, err)
@@ -729,23 +765,23 @@ func TestCollectionProfilePreCheck(t *testing.T) {
 		},
 		{
 			name: "full_profile",
-			config: `prometheusk8s:
+			config: `prometheusK8s:
   collectionProfile: full
   `,
 			expected:      CollectionProfile("full"),
 			expectedError: false,
 		},
 		{
 			name: "minimal_profile",
-			config: `prometheusk8s:
+			config: `prometheusK8s:
   collectionProfile: minimal
   `,
 			expected:      CollectionProfile("minimal"),
 			expectedError: false,
 		},
 		{
 			name: "incorrect_profile",
-			config: `prometheusk8s:
+			config: `prometheusK8s:
   collectionProfile: foo
   `,
 			expected:      "",
@@ -831,7 +867,7 @@ func TestUnsupportedAlertmanagerVersion(t *testing.T) {
 			name: "using default value",
 			config: `prometheusK8s:
   additionalAlertmanagerConfigs:
-    - Scheme: foo
+    - scheme: foo
   `,
 		},
 	} {

pkg/manifests/manifests_test.go

@@ -3747,7 +3747,7 @@ func TestKubeStateMetrics(t *testing.T) {
 }
 
 func TestOpenShiftStateMetrics(t *testing.T) {
-	config := `openShiftStateMetrics:
+	config := `openshiftStateMetrics:
   resources:
     requests:
       cpu: 100m