From 9ce8a829d1a728cf92980116fb098015534c0b9a Mon Sep 17 00:00:00 2001 From: Jeremy Hanna Date: Tue, 1 Oct 2024 18:14:14 -0400 Subject: [PATCH] [ASCII-2262] Extend config.UnmarshalKey unit tests around primitives (#29604) --- pkg/config/structure/unmarshal.go | 18 +- pkg/config/structure/unmarshal_test.go | 687 +++++++++++++++++++++++-- 2 files changed, 666 insertions(+), 39 deletions(-) diff --git a/pkg/config/structure/unmarshal.go b/pkg/config/structure/unmarshal.go index be426cb88fe819..c336a8bbc91d20 100644 --- a/pkg/config/structure/unmarshal.go +++ b/pkg/config/structure/unmarshal.go @@ -10,6 +10,7 @@ import ( "fmt" "reflect" "slices" + "strconv" "strings" "unicode" "unicode/utf8" @@ -249,6 +250,8 @@ func (n *leafNodeImpl) ChildrenKeys() ([]string, error) { func (n *leafNodeImpl) GetBool() (bool, error) { if n.val.Kind() == reflect.Bool { return n.val.Bool(), nil + } else if n.val.Kind() == reflect.Int { + return n.val.Int() != 0, nil } else if n.val.Kind() == reflect.String { return convertToBool(n.val.String()) } @@ -283,7 +286,20 @@ func (n *leafNodeImpl) GetFloat() (float64, error) { // GetString returns the scalar as a string, or an error otherwise func (n *leafNodeImpl) GetString() (string, error) { - if n.val.Kind() == reflect.String { + switch n.val.Kind() { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + stringVal := strconv.FormatInt(n.val.Int(), 10) + return stringVal, nil + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + stringVal := strconv.FormatUint(n.val.Uint(), 10) + return stringVal, nil + case reflect.Float32: + stringVal := strconv.FormatFloat(n.val.Float(), 'f', -1, 32) + return stringVal, nil + case reflect.Float64: + stringVal := strconv.FormatFloat(n.val.Float(), 'f', -1, 64) + return stringVal, nil + case reflect.String: return n.val.String(), nil } return "", newConversionError(n.val, "string") diff --git a/pkg/config/structure/unmarshal_test.go b/pkg/config/structure/unmarshal_test.go index e6d15fae0fe9eb..fffeb2bf43c170 100644 --- a/pkg/config/structure/unmarshal_test.go +++ b/pkg/config/structure/unmarshal_test.go @@ -6,7 +6,9 @@ package structure import ( + "math" "reflect" + "strings" "testing" "github.com/DataDog/datadog-agent/pkg/config/mock" @@ -14,7 +16,7 @@ import ( ) // Struct that is used within the config -type UserV3 struct { +type userV3 struct { Username string `yaml:"user"` UsernameLegacy string `yaml:"username"` AuthKey string `yaml:"authKey"` @@ -24,10 +26,10 @@ type UserV3 struct { } // Type that gets parsed out of config -type TrapsConfig struct { +type trapsConfig struct { Enabled bool `yaml:"enabled"` Port uint16 `yaml:"port"` - Users []UserV3 `yaml:"users"` + Users []userV3 `yaml:"users"` CommunityStrings []string `yaml:"community_strings"` BindHost string `yaml:"bind_host"` StopTimeout int `yaml:"stop_timeout"` @@ -58,7 +60,7 @@ network_devices: ` mockConfig := mock.NewFromYAML(t, confYaml) - var trapsCfg = TrapsConfig{} + var trapsCfg = trapsConfig{} err := UnmarshalKey(mockConfig, "network_devices.snmp_traps", &trapsCfg) assert.NoError(t, err) @@ -83,18 +85,25 @@ network_devices: assert.Equal(t, trapsCfg.Namespace, "abc") } -type ServiceDescription struct { +type serviceDescription struct { Host string - Endpoint Endpoint `mapstructure:",squash"` + Endpoint endpoint `mapstructure:",squash"` } -type Endpoint struct { +type endpoint struct { Name string `yaml:"name"` APIKey string `yaml:"apikey"` } func TestUnmarshalKeySliceOfStructures(t *testing.T) { - confYaml := ` + testcases := []struct { + name string + conf string + want []endpoint + }{ + { + name: "simple wellformed", + conf: ` endpoints: - name: intake apikey: abc1 @@ -102,21 +111,44 @@ endpoints: apikey: abc2 - name: health apikey: abc3 -` - mockConfig := mock.NewFromYAML(t, confYaml) - mockConfig.SetKnown("endpoints") - - var endpoints = []Endpoint{} - err := UnmarshalKey(mockConfig, "endpoints", &endpoints) - assert.NoError(t, err) - - assert.Equal(t, len(endpoints), 3) - assert.Equal(t, endpoints[0].Name, "intake") - assert.Equal(t, endpoints[0].APIKey, "abc1") - assert.Equal(t, endpoints[1].Name, "config") - assert.Equal(t, endpoints[1].APIKey, "abc2") - assert.Equal(t, endpoints[2].Name, "health") - assert.Equal(t, endpoints[2].APIKey, "abc3") +`, + want: []endpoint{ + {Name: "intake", APIKey: "abc1"}, + {Name: "config", APIKey: "abc2"}, + {Name: "health", APIKey: "abc3"}, + }, + }, + { + name: "missing a field is zero value", + conf: ` +endpoints: +- name: intake +- name: config + apikey: abc2 +`, + want: []endpoint{ + {Name: "intake", APIKey: ""}, + {Name: "config", APIKey: "abc2"}, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + mockConfig := mock.NewFromYAML(t, tc.conf) + mockConfig.SetKnown("endpoints") + + var endpoints = []endpoint{} + err := UnmarshalKey(mockConfig, "endpoints", &endpoints) + assert.NoError(t, err, "%s failed to marshal: %s", tc.name, err) + + assert.Equal(t, len(endpoints), len(tc.want), "%s marshalled unexepected length of slices, wanted: %s got: %s", tc.name, len(tc.want), len(endpoints)) + for i := range endpoints { + assert.Equal(t, endpoints[i].Name, tc.want[i].Name) + assert.Equal(t, endpoints[i].APIKey, tc.want[i].APIKey) + } + }) + } } func TestUnmarshalKeyWithSquash(t *testing.T) { @@ -129,7 +161,7 @@ service: mockConfig := mock.NewFromYAML(t, confYaml) mockConfig.SetKnown("service") - var svc = ServiceDescription{} + var svc = serviceDescription{} // fails without EnableSquash being given err := UnmarshalKey(mockConfig, "service", &svc) assert.Error(t, err) @@ -144,26 +176,605 @@ service: assert.Equal(t, svc.Endpoint.APIKey, "abc1") } -type FeatureConfig struct { +type featureConfig struct { Enabled bool `yaml:"enabled"` } -func TestUnmarshalKeyParseStringAsBool(t *testing.T) { - confYaml := ` +func TestUnmarshalKeyAsBool(t *testing.T) { + testcases := []struct { + name string + conf string + want bool + skip bool + }{ + { + name: "string value to true", + conf: ` feature: enabled: "true" -` - mockConfig := mock.NewFromYAML(t, confYaml) - mockConfig.SetKnown("feature") +`, + want: true, + skip: false, + }, + { + name: "yaml boolean value true", + conf: ` +feature: + enabled: true +`, + want: true, + skip: false, + }, + { + name: "string value to false", + conf: ` +feature: + enabled: "false" +`, + want: false, + skip: false, + }, + { + name: "yaml boolean value false", + conf: ` +feature: + enabled: false +`, + want: false, + skip: false, + }, + { + name: "missing value is false", + conf: ` +feature: + not_enabled: "the missing key should be false" +`, + want: false, + skip: false, + }, + { + name: "string 'y' value is true", + conf: ` +feature: + enabled: y +`, + want: true, + skip: false, + }, + { + name: "string 'yes' value is true", + conf: ` +feature: + enabled: yes +`, + want: true, + skip: false, + }, + { + name: "string 'on' value is true", + conf: ` +feature: + enabled: on +`, + want: true, + skip: false, + }, + { + name: "string '1' value is true", + conf: ` +feature: + enabled: "1" +`, + want: true, + skip: false, + }, + { + name: "int 1 value is true", + conf: ` +feature: + enabled: 1 +`, + want: true, + skip: false, + }, + { + name: "string 'n' value is false", + conf: ` +feature: + enabled: n +`, + want: false, + skip: false, + }, + { + name: "string 'no' value is false", + conf: ` +feature: + enabled: no +`, + want: false, + skip: false, + }, + { + name: "string 'off' value is false", + conf: ` +feature: + enabled: off +`, + want: false, + skip: false, + }, + { + name: "string '0' value is false", + conf: ` +feature: + enabled: "0" +`, + want: false, + skip: false, + }, + { + name: "int 0 value is false", + conf: ` +feature: + enabled: 0 +`, + want: false, + skip: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + if tc.skip { + t.Skip("Skipping test case") + } + + mockConfig := mock.NewFromYAML(t, tc.conf) + mockConfig.SetKnown("feature") + + var feature = featureConfig{} + err := UnmarshalKey(mockConfig, "feature", &feature) + assert.NoError(t, err, "%s failed to marshal: %s", tc.name, err) + + assert.Equal(t, feature.Enabled, tc.want, "%s unexpected marshal value, want: %s got: %s", tc.name, tc.want, feature.Enabled) + }) + } +} - var feature = FeatureConfig{} - err := UnmarshalKey(mockConfig, "feature", &feature) - assert.NoError(t, err) +type uintConfig struct { + Fielduint8 uint8 `yaml:"uint8"` + Fielduint16 uint16 `yaml:"uint16"` + Fielduint32 uint32 `yaml:"uint32"` + Fielduint64 uint64 `yaml:"uint64"` + Fieldint8 int8 `yaml:"int8"` + Fieldint16 int16 `yaml:"int16"` + Fieldint32 int32 `yaml:"int32"` + Fieldint64 int64 `yaml:"int64"` +} - assert.Equal(t, feature.Enabled, true) +func TestUnmarshalKeyAsInt(t *testing.T) { + testcases := []struct { + name string + conf string + want uintConfig + skip bool + }{ + { + name: "value int config map", + conf: ` +feature: + uint8: 123 + uint16: 1234 + uint32: 1234 + uint64: 1234 + int8: 123 + int16: 1234 + int32: 1234 + int64: 1234 +`, + want: uintConfig{ + Fielduint8: 123, + Fielduint16: 1234, + Fielduint32: 1234, + Fielduint64: 1234, + Fieldint8: 123, + Fieldint16: 1234, + Fieldint32: 1234, + Fieldint64: 1234, + }, + skip: false, + }, + { + name: "float convert to int config map", + conf: ` +feature: + uint8: 12.0 + uint16: 1234.0 + uint32: 1234 + uint64: 1234 + int8: 12.3 + int16: 12.9 + int32: 12.34 + int64: -12.34 +`, + want: uintConfig{ + Fielduint8: 12, + Fielduint16: 1234, + Fielduint32: 1234, + Fielduint64: 1234, + Fieldint8: 12, + Fieldint16: 12, // expected truncation of the decimal, no rounding + Fieldint32: 12, + Fieldint64: -12, + }, + skip: false, + }, + { + name: "missing field is zero value config map", + conf: ` +feature: + uint16: 1234 + uint32: 1234 + uint64: 1234 + int8: 123 + int16: 1234 + int32: 1234 + int64: 1234 +`, + want: uintConfig{ + Fielduint8: 0, + Fielduint16: 1234, + Fielduint32: 1234, + Fielduint64: 1234, + Fieldint8: 123, + Fieldint16: 1234, + Fieldint32: 1234, + Fieldint64: 1234, + }, + skip: false, + }, + { + name: "overflow int config map", + conf: ` +feature: + uint8: 1234 + uint16: 1234 + uint32: 1234 + uint64: 1234 + int8: 123 + int16: 1234 + int32: 1234 + int64: 1234 +`, + want: uintConfig{ + Fielduint8: math.MaxUint8, // actual 230 - unclear what this behavior should be + Fielduint16: 1234, + Fielduint32: 1234, + Fielduint64: 1234, + Fieldint8: 123, + Fieldint16: 1234, + Fieldint32: 1234, + Fieldint64: 1234, + }, + skip: true, + }, + { + name: "underflow int config map", + conf: ` +feature: + uint8: -123 + uint16: 1234 + uint32: 1234 + uint64: 1234 + int8: 123 + int16: 1234 + int32: 1234 + int64: 1234 +`, + want: uintConfig{ + Fielduint8: 0, // actual 133 - unclear what this behavior should be + Fielduint16: 1234, + Fielduint32: 1234, + Fielduint64: 1234, + Fieldint8: 123, + Fieldint16: 1234, + Fieldint32: 1234, + Fieldint64: 1234, + }, + skip: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + if tc.skip { + t.Skip("Skipping test case") + } + + mockConfig := mock.NewFromYAML(t, tc.conf) + mockConfig.SetKnown("feature") + + var feature = uintConfig{} + err := UnmarshalKey(mockConfig, "feature", &feature) + assert.NoError(t, err, "%s failed to marshal: %s", tc.name, err) + if err != nil { + t.FailNow() + } + + confvalues := reflect.ValueOf(feature) + wantvalues := reflect.ValueOf(tc.want) + + for i := 0; i < confvalues.NumField(); i++ { + wantType := strings.ReplaceAll(confvalues.Type().Field(i).Name, "Field", "") + actual := confvalues.Field(i).Type().Name() + assert.Equal(t, wantType, actual, "%s unexpected marshal type, want: %s got: %s", tc.name, wantType, actual) + assert.True(t, reflect.DeepEqual(wantvalues.Field(i).Interface(), confvalues.Field(i).Interface()), "%s marshalled values not equal, want: %s, got: %s", tc.name, wantvalues.Field(i), confvalues.Field(i)) + } + }) + } +} + +type floatConfig struct { + Fieldfloat32 float32 `yaml:"float32"` + Fieldfloat64 float64 `yaml:"float64"` +} + +func TestUnmarshalKeyAsFloat(t *testing.T) { + testcases := []struct { + name string + conf string + want floatConfig + skip bool + }{ + { + name: "value float config map", + conf: ` +feature: + float32: 12.34 + float64: 12.34 +`, + want: floatConfig{ + Fieldfloat32: 12.34, + Fieldfloat64: 12.34, + }, + skip: false, + }, + { + name: "missing field zero value float config map", + conf: ` +feature: + float64: 12.34 +`, + want: floatConfig{ + Fieldfloat32: 0.0, + Fieldfloat64: 12.34, + }, + skip: false, + }, + { + name: "converts ints to float config map", + conf: ` +feature: + float32: 12 + float64: 12 +`, + want: floatConfig{ + Fieldfloat32: 12.0, + Fieldfloat64: 12.0, + }, + skip: false, + }, + { + name: "converts negatives to float config map", + conf: ` +feature: + float32: -12 + float64: -12.34 +`, + want: floatConfig{ + Fieldfloat32: -12.0, + Fieldfloat64: -12.34, + }, + skip: false, + }, + { + name: "starting decimal to float config map", + conf: ` +feature: + float32: .34 + float64: -.34 +`, + want: floatConfig{ + Fieldfloat32: 0.34, + Fieldfloat64: -0.34, + }, + skip: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + if tc.skip { + t.Skip("Skipping test case") + } + + mockConfig := mock.NewFromYAML(t, tc.conf) + mockConfig.SetKnown("feature") + + var feature = floatConfig{} + err := UnmarshalKey(mockConfig, "feature", &feature) + assert.NoError(t, err, "%s failed to marshal: %s", tc.name, err) + if err != nil { + t.FailNow() + } + + confvalues := reflect.ValueOf(feature) + wantvalues := reflect.ValueOf(tc.want) + + for i := 0; i < confvalues.NumField(); i++ { + wantType := strings.ReplaceAll(confvalues.Type().Field(i).Name, "Field", "") + actual := confvalues.Field(i).Type().Name() + assert.Equal(t, wantType, actual, "%s unexpected marshal type, want: %s got: %s", tc.name, wantType, actual) + assert.True(t, reflect.DeepEqual(wantvalues.Field(i).Interface(), confvalues.Field(i).Interface()), "%s marshalled values not equal, want: %s, got: %s", tc.name, wantvalues.Field(i), confvalues.Field(i)) + } + }) + } +} + +type stringConfig struct { + Field string `yaml:"value"` +} + +func TestUnmarshalKeyAsString(t *testing.T) { + testcases := []struct { + name string + conf string + want stringConfig + skip bool + }{ + { + name: "string value config map", + conf: ` +feature: + value: a string +`, + want: stringConfig{ + Field: "a string", + }, + skip: false, + }, + { + name: "quoted string config map", + conf: ` +feature: + value: "12.34" +`, + want: stringConfig{ + Field: "12.34", + }, + skip: false, + }, + { + name: "missing field is a empty string", + conf: ` +feature: + float64: 12.34 +`, + want: stringConfig{ + Field: string(""), + }, + skip: false, + }, + { + name: "converts yaml parsed int to match struct", + conf: ` +feature: + value: 42 +`, + want: stringConfig{ + Field: "42", + }, + skip: false, + }, + { + name: "truncates large yaml floats instead of using exponents", + conf: ` +feature: + value: 4.2222222222222222222222 +`, + want: stringConfig{ + Field: "4.222222222222222", + }, + skip: false, + }, + { + name: "converts yaml parsed float to match struct", + conf: ` +feature: + value: 4.2 +`, + want: stringConfig{ + Field: "4.2", + }, + skip: false, + }, + { + name: "commas are part of the string and not a list", + conf: ` +feature: + value: not, a, list +`, + want: stringConfig{ + Field: "not, a, list", + }, + skip: false, + }, + { + name: "parses special characters", + conf: ` +feature: + value: ☺☻☹ +`, + want: stringConfig{ + Field: "☺☻☹", + }, + skip: false, + }, + { + name: "does not parse invalid ascii to byte sequences", + conf: ` +feature: + value: \xff-\xff +`, + want: stringConfig{ + Field: `\xff-\xff`, + }, + skip: false, + }, + { + name: "retains string utf-8", + conf: ` +feature: + value: 日本語 +`, + want: stringConfig{ + Field: "日本語", + }, + skip: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + if tc.skip { + t.Skip("Skipping test case") + } + + mockConfig := mock.NewFromYAML(t, tc.conf) + mockConfig.SetKnown("feature") + + var feature = stringConfig{} + err := UnmarshalKey(mockConfig, "feature", &feature) + assert.NoError(t, err, "%s failed to marshal: %s", tc.name, err) + if err != nil { + t.FailNow() + } + + confvalues := reflect.ValueOf(feature) + wantvalues := reflect.ValueOf(tc.want) + + for i := 0; i < confvalues.NumField(); i++ { + wantType := "string" + actual := confvalues.Field(i).Type().Name() + assert.Equal(t, wantType, actual, "%s unexpected marshal type, want: %s got: %s", tc.name, wantType, actual) + assert.True(t, reflect.DeepEqual(wantvalues.Field(i).Interface(), confvalues.Field(i).Interface()), "%s marshalled values not equal, want: %s, got: %s", tc.name, wantvalues.Field(i), confvalues.Field(i)) + } + }) + } } -type FeatureConfigDiffCase struct { +type featureConfigDiffCase struct { ENaBLEd bool } @@ -175,13 +786,13 @@ feature: mockConfig := mock.NewFromYAML(t, confYaml) mockConfig.SetKnown("feature") - var feature = FeatureConfig{} + var feature = featureConfig{} err := UnmarshalKey(mockConfig, "feature", &feature) assert.NoError(t, err) assert.Equal(t, feature.Enabled, true) - var diffcase = FeatureConfigDiffCase{} + var diffcase = featureConfigDiffCase{} err = UnmarshalKey(mockConfig, "feature", &diffcase) assert.NoError(t, err) @@ -198,7 +809,7 @@ feature: // If the data from the config is missing, UnmarshalKey is a no-op, does // nothing, and returns no error - var endpoints = []Endpoint{} + var endpoints = []endpoint{} err := UnmarshalKey(mockConfig, "config_providers", &endpoints) assert.NoError(t, err) }