Identify config error when expected map is other value type (#1641)

Fixes #1599

config/config.go

@@ -38,6 +38,7 @@ type configErrorCode int
 
 const (
 	_ configErrorCode = iota // skip 0, start errors codes from 1.
+	errInvalidSubConfig
 	errInvalidTypeAndNameKey
 	errUnknownType
 	errDuplicateName
@@ -233,8 +234,11 @@ func errorDuplicateName(component string, fullName string) error {
 }
 
 func loadExtensions(v *viper.Viper, factories map[configmodels.Type]component.ExtensionFactory) (configmodels.Extensions, error) {
-	// Get the list of all "extensions" sub vipers from config source.
-	extensionsConfig := ViperSub(v, extensionsKeyName)
+	// Get the list of all "receivers" sub vipers from config source.
+	extensionsConfig, err := ViperSubExact(v, extensionsKeyName)
+	if err != nil {
+		return nil, err
+	}
 	expandEnvConfig(extensionsConfig)
 
 	// Get the map of "extensions" sub-keys.
@@ -263,7 +267,10 @@ func loadExtensions(v *viper.Viper, factories map[configmodels.Type]component.Ex
 		extensionCfg.SetName(fullName)
 
 		// Unmarshal only the subconfig for this exporter.
-		componentConfig := ViperSub(extensionsConfig, key)
+		componentConfig, err := ViperSubExact(extensionsConfig, key)
+		if err != nil {
+			return nil, err
+		}
 
 		// Now that the default config struct is created we can Unmarshal into it
 		// and it will apply user-defined config on top of the default.
@@ -284,7 +291,10 @@ func loadExtensions(v *viper.Viper, factories map[configmodels.Type]component.Ex
 
 func loadService(v *viper.Viper) (configmodels.Service, error) {
 	var service configmodels.Service
-	serviceSub := ViperSub(v, serviceKeyName)
+	serviceSub, err := ViperSubExact(v, serviceKeyName)
+	if err != nil {
+		return service, err
+	}
 	expandEnvConfig(serviceSub)
 
 	// Process the pipelines first so in case of error on them it can be properly
@@ -328,7 +338,10 @@ func LoadReceiver(componentConfig *viper.Viper, typeStr configmodels.Type, fullN
 
 func loadReceivers(v *viper.Viper, factories map[configmodels.Type]component.ReceiverFactory) (configmodels.Receivers, error) {
 	// Get the list of all "receivers" sub vipers from config source.
-	receiversConfig := ViperSub(v, receiversKeyName)
+	receiversConfig, err := ViperSubExact(v, receiversKeyName)
+	if err != nil {
+		return nil, err
+	}
 	expandEnvConfig(receiversConfig)
 
 	// Get the map of "receivers" sub-keys.
@@ -351,7 +364,13 @@ func loadReceivers(v *viper.Viper, factories map[configmodels.Type]component.Rec
 			return nil, errorUnknownType(receiversKeyName, typeStr, fullName)
 		}
 
-		receiverCfg, err := LoadReceiver(ViperSub(receiversConfig, key), typeStr, fullName, factory)
+		// Unmarshal only the subconfig for this exporter.
+		componentConfig, err := ViperSubExact(receiversConfig, key)
+		if err != nil {
+			return nil, err
+		}
+
+		receiverCfg, err := LoadReceiver(componentConfig, typeStr, fullName, factory)
 
 		if err != nil {
 			// LoadReceiver already wraps the error.
@@ -369,7 +388,10 @@ func loadReceivers(v *viper.Viper, factories map[configmodels.Type]component.Rec
 
 func loadExporters(v *viper.Viper, factories map[configmodels.Type]component.ExporterFactory) (configmodels.Exporters, error) {
 	// Get the list of all "exporters" sub vipers from config source.
-	exportersConfig := ViperSub(v, exportersKeyName)
+	exportersConfig, err := ViperSubExact(v, exportersKeyName)
+	if err != nil {
+		return nil, err
+	}
 	expandEnvConfig(exportersConfig)
 
 	// Get the map of "exporters" sub-keys.
@@ -398,7 +420,10 @@ func loadExporters(v *viper.Viper, factories map[configmodels.Type]component.Exp
 		exporterCfg.SetName(fullName)
 
 		// Unmarshal only the subconfig for this exporter.
-		componentConfig := ViperSub(exportersConfig, key)
+		componentConfig, err := ViperSubExact(exportersConfig, key)
+		if err != nil {
+			return nil, err
+		}
 
 		// Now that the default config struct is created we can Unmarshal into it
 		// and it will apply user-defined config on top of the default.
@@ -419,7 +444,10 @@ func loadExporters(v *viper.Viper, factories map[configmodels.Type]component.Exp
 
 func loadProcessors(v *viper.Viper, factories map[configmodels.Type]component.ProcessorFactory) (configmodels.Processors, error) {
 	// Get the list of all "processors" sub vipers from config source.
-	processorsConfig := ViperSub(v, processorsKeyName)
+	processorsConfig, err := ViperSubExact(v, processorsKeyName)
+	if err != nil {
+		return nil, err
+	}
 	expandEnvConfig(processorsConfig)
 
 	// Get the map of "processors" sub-keys.
@@ -448,7 +476,10 @@ func loadProcessors(v *viper.Viper, factories map[configmodels.Type]component.Pr
 		processorCfg.SetName(fullName)
 
 		// Unmarshal only the subconfig for this processor.
-		componentConfig := ViperSub(processorsConfig, key)
+		componentConfig, vsErr := ViperSubExact(processorsConfig, key)
+		if vsErr != nil {
+			return nil, vsErr
+		}
 
 		// Now that the default config struct is created we can Unmarshal into it
 		// and it will apply user-defined config on top of the default.
@@ -469,7 +500,10 @@ func loadProcessors(v *viper.Viper, factories map[configmodels.Type]component.Pr
 
 func loadPipelines(v *viper.Viper) (configmodels.Pipelines, error) {
 	// Get the list of all "pipelines" sub vipers from config source.
-	pipelinesConfig := ViperSub(v, pipelinesKeyName)
+	pipelinesConfig, err := ViperSubExact(v, pipelinesKeyName)
+	if err != nil {
+		return nil, err
+	}
 
 	// Get the map of "pipelines" sub-keys.
 	keyMap := v.GetStringMap(pipelinesKeyName)
@@ -498,7 +532,10 @@ func loadPipelines(v *viper.Viper) (configmodels.Pipelines, error) {
 			return nil, errorUnknownType(pipelinesKeyName, typeStr, fullName)
 		}
 
-		pipelineConfig := ViperSub(pipelinesConfig, key)
+		pipelineConfig, err := ViperSubExact(pipelinesConfig, key)
+		if err != nil {
+			return nil, err
+		}
 
 		// Now that the default config struct is created we can Unmarshal into it
 		// and it will apply user-defined config on top of the default.
@@ -744,18 +781,23 @@ func defaultUnmarshaler(componentViperSection *viper.Viper, intoCfg interface{})
 	return componentViperSection.UnmarshalExact(intoCfg)
 }
 
-// Copied from the Viper but changed to use the same delimiter.
+// Copied from the Viper but changed to use the same delimiter
+// and return error if the sub is not a map.
 // See https://github.com/spf13/viper/issues/871
-func ViperSub(v *viper.Viper, key string) *viper.Viper {
-	subv := NewViper()
+func ViperSubExact(v *viper.Viper, key string) (*viper.Viper, error) {
 	data := v.Get(key)
 	if data == nil {
-		return subv
+		return NewViper(), nil
 	}
 
 	if reflect.TypeOf(data).Kind() == reflect.Map {
-		subv.MergeConfigMap(cast.ToStringMap(data))
-		return subv
+		subv := NewViper()
+		// Cannot return error because the subv is empty.
+		_ = subv.MergeConfigMap(cast.ToStringMap(data))
+		return subv, nil
+	}
+	return nil, &configError{
+		code: errInvalidSubConfig,
+		msg:  fmt.Sprintf("unexpected sub-config value kind for key:%s value:%v kind:%v)", key, data, reflect.TypeOf(data).Kind()),
 	}
-	return subv
 }

config/config_test.go

@@ -506,28 +506,34 @@ func TestDecodeConfig_Invalid(t *testing.T) {
 		{name: "invalid-processor-section", expected: errUnmarshalError, expectedMessage: "processors"},
 		{name: "invalid-exporter-section", expected: errUnmarshalError, expectedMessage: "exporters"},
 		{name: "invalid-pipeline-section", expected: errUnmarshalError, expectedMessage: "pipelines"},
+		{name: "invalid-extension-sub-config", expected: errInvalidSubConfig},
+		{name: "invalid-exporter-sub-config", expected: errInvalidSubConfig},
+		{name: "invalid-processor-sub-config", expected: errInvalidSubConfig},
+		{name: "invalid-receiver-sub-config", expected: errInvalidSubConfig},
+		{name: "invalid-pipeline-sub-config", expected: errInvalidSubConfig},
 	}
 
 	factories, err := componenttest.ExampleComponents()
 	assert.NoError(t, err)
 
 	for _, test := range testCases {
-		_, err := loadConfigFile(t, path.Join(".", "testdata", test.name+".yaml"), factories)
-		if err == nil {
-			t.Errorf("expected error but succeeded on invalid config case: %s", test.name)
-		} else if test.expected != 0 {
-			cfgErr, ok := err.(*configError)
-			if !ok {
-				t.Errorf("expected config error code %v but got a different error '%v' on invalid config case: %s",
-					test.expected, err, test.name)
-			} else {
-				assert.Equal(t, test.expected, cfgErr.code)
-				if test.expectedMessage != "" {
-					assert.Contains(t, cfgErr.Error(), test.expectedMessage)
+		t.Run(test.name, func(t *testing.T) {
+			_, err := loadConfigFile(t, path.Join(".", "testdata", test.name+".yaml"), factories)
+			if err == nil {
+				t.Error("expected error but succeeded")
+			} else if test.expected != 0 {
+				cfgErr, ok := err.(*configError)
+				if !ok {
+					t.Errorf("expected config error code %v but got a different error '%v'", test.expected, err)
+				} else {
+					assert.Equal(t, test.expected, cfgErr.code, err)
+					if test.expectedMessage != "" {
+						assert.Contains(t, cfgErr.Error(), test.expectedMessage)
+					}
+					assert.NotEmpty(t, cfgErr.Error(), "returned config error %v with empty error message", cfgErr.code)
 				}
-				assert.NotEmpty(t, cfgErr.Error(), "returned config error %v with empty error message", cfgErr.code)
 			}
-		}
+		})
 	}
 }
 

config/testdata/invalid-exporter-sub-config.yaml

@@ -0,0 +1,13 @@
+receivers:
+  multireceiver:
+exporters:
+  exampleexporter:
+    tests
+processors:
+  exampleprocessor:
+service:
+  pipelines:
+    traces:
+      receivers: [multireceiver]
+      exporters: [exampleexporter]
+      processors: [exampleprocessor]

config/testdata/invalid-extension-sub-config.yaml

@@ -0,0 +1,16 @@
+extensions:
+  exampleextension:
+    tests
+receivers:
+  examplereceiver:
+processors:
+  exampleprocessor:
+exporters:
+  exampleexporter:
+service:
+  extensions: [exampleextension]
+  pipelines:
+    traces:
+      receivers: [examplereceiver]
+      processors: [exampleprocessor]
+      exporters: [exampleexporter]

config/testdata/invalid-pipeline-sub-config.yaml

@@ -0,0 +1,9 @@
+receivers:
+  multireceiver:
+exporters:
+  exampleexporter:
+processors:
+  exampleprocessor:
+service:
+  pipelines:
+    traces

config/testdata/invalid-processor-sub-config.yaml

@@ -0,0 +1,13 @@
+receivers:
+  multireceiver:
+exporters:
+  exampleexporter:
+processors:
+  exampleprocessor:
+    tests
+service:
+  pipelines:
+    traces:
+      receivers: [multireceiver]
+      exporters: [exampleexporter]
+      processors: [exampleprocessor]

config/testdata/invalid-receiver-sub-config.yaml

@@ -0,0 +1,13 @@
+receivers:
+  multireceiver:
+    tests
+exporters:
+  exampleexporter:
+processors:
+  exampleprocessor:
+service:
+  pipelines:
+    traces:
+      receivers: [multireceiver]
+      exporters: [exampleexporter]
+      processors: [exampleprocessor]

receiver/hostmetricsreceiver/factory.go

@@ -16,6 +16,7 @@ package hostmetricsreceiver
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -95,9 +96,12 @@ func customUnmarshaler(componentViperSection *viper.Viper, intoCfg interface{})
 
 	cfg.Scrapers = map[string]internal.Config{}
 
-	scrapersViperSection := config.ViperSub(componentViperSection, scrapersKey)
-	if scrapersViperSection == nil || len(scrapersViperSection.AllKeys()) == 0 {
-		return fmt.Errorf("must specify at least one scraper when using hostmetrics receiver")
+	scrapersViperSection, err := config.ViperSubExact(componentViperSection, scrapersKey)
+	if err != nil {
+		return err
+	}
+	if len(scrapersViperSection.AllKeys()) == 0 {
+		return errors.New("must specify at least one scraper when using hostmetrics receiver")
 	}
 
 	for key := range componentViperSection.GetStringMap(scrapersKey) {
@@ -107,8 +111,11 @@ func customUnmarshaler(componentViperSection *viper.Viper, intoCfg interface{})
 		}
 
 		collectorCfg := factory.CreateDefaultConfig()
-		collectorViperSection := config.ViperSub(scrapersViperSection, key)
-		err := collectorViperSection.UnmarshalExact(collectorCfg)
+		collectorViperSection, err := config.ViperSubExact(scrapersViperSection, key)
+		if err != nil {
+			return err
+		}
+		err = collectorViperSection.UnmarshalExact(collectorCfg)
 		if err != nil {
 			return fmt.Errorf("error reading settings for scraper type %q: %v", key, err)
 		}