Remove ConfigValidate and add Validate on the Config struct (#2665)

Signed-off-by: Bogdan Drutu <[email protected]>

Author: bogdandrutu

config/config.go

@@ -26,7 +26,6 @@ import (
 
 	"github.com/spf13/cast"
 	"github.com/spf13/viper"
-	"go.uber.org/zap"
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/config/configmodels"
@@ -42,15 +41,6 @@ const (
 	errInvalidTypeAndNameKey
 	errUnknownType
 	errDuplicateName
-	errMissingPipelines
-	errPipelineMustHaveReceiver
-	errPipelineMustHaveExporter
-	errExtensionNotExists
-	errPipelineReceiverNotExists
-	errPipelineProcessorNotExists
-	errPipelineExporterNotExists
-	errMissingReceivers
-	errMissingExporters
 	errUnmarshalTopLevelStructureError
 )
 
@@ -478,171 +468,6 @@ func loadPipelines(pipelinesConfig map[string]pipelineSettings) (configmodels.Pi
 	return pipelines, nil
 }
 
-// ValidateConfig validates config.
-func ValidateConfig(cfg *configmodels.Config, _ *zap.Logger) error {
-	// This function performs basic validation of configuration. There may be more subtle
-	// invalid cases that we currently don't check for but which we may want to add in
-	// the future (e.g. disallowing receiving and exporting on the same endpoint).
-
-	if err := validateReceivers(cfg); err != nil {
-		return err
-	}
-
-	if err := validateExporters(cfg); err != nil {
-		return err
-	}
-
-	if err := validateService(cfg); err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func validateService(cfg *configmodels.Config) error {
-	if err := validatePipelines(cfg); err != nil {
-		return err
-	}
-
-	return validateServiceExtensions(cfg)
-}
-
-func validateServiceExtensions(cfg *configmodels.Config) error {
-	if len(cfg.Service.Extensions) == 0 {
-		return nil
-	}
-
-	// Validate extensions.
-	for _, ref := range cfg.Service.Extensions {
-		// Check that the name referenced in the Service extensions exists in the top-level extensions
-		if cfg.Extensions[ref] == nil {
-			return &configError{
-				code: errExtensionNotExists,
-				msg:  fmt.Sprintf("Service references extension %q which does not exist", ref),
-			}
-		}
-	}
-
-	return nil
-}
-
-func validatePipelines(cfg *configmodels.Config) error {
-	// Must have at least one pipeline.
-	if len(cfg.Service.Pipelines) == 0 {
-		return &configError{code: errMissingPipelines, msg: "must have at least one pipeline"}
-	}
-
-	// Validate pipelines.
-	for _, pipeline := range cfg.Service.Pipelines {
-		if err := validatePipeline(cfg, pipeline); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func validatePipeline(cfg *configmodels.Config, pipeline *configmodels.Pipeline) error {
-	if err := validatePipelineReceivers(cfg, pipeline); err != nil {
-		return err
-	}
-
-	if err := validatePipelineExporters(cfg, pipeline); err != nil {
-		return err
-	}
-
-	if err := validatePipelineProcessors(cfg, pipeline); err != nil {
-		return err
-	}
-
-	return nil
-}
-
-func validatePipelineReceivers(cfg *configmodels.Config, pipeline *configmodels.Pipeline) error {
-	if len(pipeline.Receivers) == 0 {
-		return &configError{
-			code: errPipelineMustHaveReceiver,
-			msg:  fmt.Sprintf("pipeline %q must have at least one receiver", pipeline.Name),
-		}
-	}
-
-	// Validate pipeline receiver name references.
-	for _, ref := range pipeline.Receivers {
-		// Check that the name referenced in the pipeline's receivers exists in the top-level receivers
-		if cfg.Receivers[ref] == nil {
-			return &configError{
-				code: errPipelineReceiverNotExists,
-				msg:  fmt.Sprintf("pipeline %q references receiver %q which does not exist", pipeline.Name, ref),
-			}
-		}
-	}
-
-	return nil
-}
-
-func validatePipelineExporters(cfg *configmodels.Config, pipeline *configmodels.Pipeline) error {
-	if len(pipeline.Exporters) == 0 {
-		return &configError{
-			code: errPipelineMustHaveExporter,
-			msg:  fmt.Sprintf("pipeline %q must have at least one exporter", pipeline.Name),
-		}
-	}
-
-	// Validate pipeline exporter name references.
-	for _, ref := range pipeline.Exporters {
-		// Check that the name referenced in the pipeline's Exporters exists in the top-level Exporters
-		if cfg.Exporters[ref] == nil {
-			return &configError{
-				code: errPipelineExporterNotExists,
-				msg:  fmt.Sprintf("pipeline %q references exporter %q which does not exist", pipeline.Name, ref),
-			}
-		}
-	}
-
-	return nil
-}
-
-func validatePipelineProcessors(cfg *configmodels.Config, pipeline *configmodels.Pipeline) error {
-	if len(pipeline.Processors) == 0 {
-		return nil
-	}
-
-	// Validate pipeline processor name references
-	for _, ref := range pipeline.Processors {
-		// Check that the name referenced in the pipeline's processors exists in the top-level processors.
-		if cfg.Processors[ref] == nil {
-			return &configError{
-				code: errPipelineProcessorNotExists,
-				msg:  fmt.Sprintf("pipeline %q references processor %s which does not exist", pipeline.Name, ref),
-			}
-		}
-	}
-
-	return nil
-}
-
-func validateReceivers(cfg *configmodels.Config) error {
-	// Currently there is no default receiver enabled. The configuration must specify at least one enabled receiver to
-	// be valid.
-	if len(cfg.Receivers) == 0 {
-		return &configError{
-			code: errMissingReceivers,
-			msg:  "no enabled receivers specified in config",
-		}
-	}
-	return nil
-}
-
-func validateExporters(cfg *configmodels.Config) error {
-	// There must be at least one enabled exporter to be considered a valid configuration.
-	if len(cfg.Exporters) == 0 {
-		return &configError{
-			code: errMissingExporters,
-			msg:  "no enabled exporters specified in config",
-		}
-	}
-	return nil
-}
-
 // expandEnvConfig creates a new viper config with expanded values for all the values (simple, list or map value).
 // It does not expand the keys.
 func expandEnvConfig(v *viper.Viper) {

config/config_test.go

@@ -21,7 +21,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"go.uber.org/zap"
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/component/componenttest"
@@ -414,47 +413,40 @@ func TestDecodeConfig_Invalid(t *testing.T) {
 		expected        configErrorCode // expected error (if nil any error is acceptable)
 		expectedMessage string          // string that the error must contain
 	}{
-		{name: "empty-config"},
-		{name: "missing-all-sections"},
-		{name: "missing-exporters", expected: errMissingExporters},
-		{name: "missing-receivers", expected: errMissingReceivers},
-		{name: "missing-processors"},
-		{name: "invalid-extension-name", expected: errExtensionNotExists},
-		{name: "invalid-receiver-name"},
-		{name: "invalid-receiver-reference", expected: errPipelineReceiverNotExists},
-		{name: "missing-extension-type", expected: errInvalidTypeAndNameKey},
-		{name: "missing-receiver-type", expected: errInvalidTypeAndNameKey},
-		{name: "missing-exporter-name-after-slash", expected: errInvalidTypeAndNameKey},
-		{name: "missing-processor-type", expected: errInvalidTypeAndNameKey},
-		{name: "missing-pipelines", expected: errMissingPipelines},
-		{name: "pipeline-must-have-exporter-logs", expected: errPipelineMustHaveExporter},
-		{name: "pipeline-must-have-exporter-metrics", expected: errPipelineMustHaveExporter},
-		{name: "pipeline-must-have-exporter-traces", expected: errPipelineMustHaveExporter},
-		{name: "pipeline-must-have-receiver-logs", expected: errPipelineMustHaveReceiver},
-		{name: "pipeline-must-have-receiver-metrics", expected: errPipelineMustHaveReceiver},
-		{name: "pipeline-must-have-receiver-traces", expected: errPipelineMustHaveReceiver},
-		{name: "pipeline-exporter-not-exists", expected: errPipelineExporterNotExists},
-		{name: "pipeline-processor-not-exists", expected: errPipelineProcessorNotExists},
+		{name: "invalid-extension-type", expected: errInvalidTypeAndNameKey},
+		{name: "invalid-receiver-type", expected: errInvalidTypeAndNameKey},
+		{name: "invalid-exporter-type", expected: errInvalidTypeAndNameKey},
+		{name: "invalid-processor-type", expected: errInvalidTypeAndNameKey},
+		{name: "invalid-pipeline-type", expected: errInvalidTypeAndNameKey},
+
+		{name: "invalid-extension-name-after-slash", expected: errInvalidTypeAndNameKey},
+		{name: "invalid-receiver-name-after-slash", expected: errInvalidTypeAndNameKey},
+		{name: "invalid-exporter-name-after-slash", expected: errInvalidTypeAndNameKey},
+		{name: "invalid-processor-name-after-slash", expected: errInvalidTypeAndNameKey},
+		{name: "invalid-pipeline-name-after-slash", expected: errInvalidTypeAndNameKey},
+
 		{name: "unknown-extension-type", expected: errUnknownType, expectedMessage: "extensions"},
 		{name: "unknown-receiver-type", expected: errUnknownType, expectedMessage: "receivers"},
 		{name: "unknown-exporter-type", expected: errUnknownType, expectedMessage: "exporters"},
 		{name: "unknown-processor-type", expected: errUnknownType, expectedMessage: "processors"},
-		{name: "invalid-service-extensions-value", expected: errUnmarshalTopLevelStructureError, expectedMessage: "service"},
-		{name: "invalid-sequence-value", expected: errUnmarshalTopLevelStructureError, expectedMessage: "pipelines"},
-		{name: "invalid-pipeline-type", expected: errUnknownType, expectedMessage: "pipelines"},
-		{name: "invalid-pipeline-type-and-name", expected: errInvalidTypeAndNameKey},
+		{name: "unknown-pipeline-type", expected: errUnknownType, expectedMessage: "pipelines"},
+
 		{name: "duplicate-extension", expected: errDuplicateName, expectedMessage: "extensions"},
 		{name: "duplicate-receiver", expected: errDuplicateName, expectedMessage: "receivers"},
 		{name: "duplicate-exporter", expected: errDuplicateName, expectedMessage: "exporters"},
 		{name: "duplicate-processor", expected: errDuplicateName, expectedMessage: "processors"},
 		{name: "duplicate-pipeline", expected: errDuplicateName, expectedMessage: "pipelines"},
+
 		{name: "invalid-top-level-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "top level"},
 		{name: "invalid-extension-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "extensions"},
-		{name: "invalid-service-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "service"},
 		{name: "invalid-receiver-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "receivers"},
 		{name: "invalid-processor-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "processors"},
 		{name: "invalid-exporter-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "exporters"},
+		{name: "invalid-service-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "service"},
+		{name: "invalid-service-extensions-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "service"},
 		{name: "invalid-pipeline-section", expected: errUnmarshalTopLevelStructureError, expectedMessage: "pipelines"},
+		{name: "invalid-sequence-value", expected: errUnmarshalTopLevelStructureError, expectedMessage: "pipelines"},
+
 		{name: "invalid-extension-sub-config", expected: errUnmarshalTopLevelStructureError},
 		{name: "invalid-exporter-sub-config", expected: errUnmarshalTopLevelStructureError},
 		{name: "invalid-processor-sub-config", expected: errUnmarshalTopLevelStructureError},
@@ -468,9 +460,8 @@ func TestDecodeConfig_Invalid(t *testing.T) {
 	for _, test := range testCases {
 		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 {
+			require.Error(t, err)
+			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)
@@ -486,25 +477,19 @@ func TestDecodeConfig_Invalid(t *testing.T) {
 	}
 }
 
-func TestLoadEmptyConfig(t *testing.T) {
+func TestLoadEmpty(t *testing.T) {
 	factories, err := componenttest.ExampleComponents()
 	assert.NoError(t, err)
 
-	// Open the file for reading.
-	file, err := os.Open(path.Join(".", "testdata", "empty-config.yaml"))
-	require.NoError(t, err)
-
-	defer func() {
-		require.NoError(t, file.Close())
-	}()
+	_, err = loadConfigFile(t, path.Join(".", "testdata", "empty-config.yaml"), factories)
+	assert.NoError(t, err)
+}
 
-	// Read yaml config from file
-	v := NewViper()
-	v.SetConfigType("yaml")
-	require.NoError(t, v.ReadConfig(file))
+func TestLoadEmptyAllSections(t *testing.T) {
+	factories, err := componenttest.ExampleComponents()
+	assert.NoError(t, err)
 
-	// Load the config from viper using the given factories.
-	_, err = Load(v, factories)
+	_, err = loadConfigFile(t, path.Join(".", "testdata", "empty-all-sections.yaml"), factories)
 	assert.NoError(t, err)
 }
 
@@ -515,11 +500,7 @@ func loadConfigFile(t *testing.T, fileName string, factories component.Factories
 	require.NoErrorf(t, v.ReadInConfig(), "unable to read the file %v", fileName)
 
 	// Load the config from viper using the given factories.
-	cfg, err := Load(v, factories)
-	if err != nil {
-		return nil, err
-	}
-	return cfg, ValidateConfig(cfg, zap.NewNop())
+	return Load(v, factories)
 }
 
 type nestedConfig struct {