Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ConfigValidate and add Validate on the Config struct #2665

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Remove ConfigValidate and add Validate on the Config struct #2665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 0 additions & 175 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -42,15 +41,6 @@ const (
errInvalidTypeAndNameKey
errUnknownType
errDuplicateName
errMissingPipelines
errPipelineMustHaveReceiver
errPipelineMustHaveExporter
errExtensionNotExists
errPipelineReceiverNotExists
errPipelineProcessorNotExists
errPipelineExporterNotExists
errMissingReceivers
errMissingExporters
errUnmarshalTopLevelStructureError
)

Expand Down Expand Up @@ -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) {
Expand Down
79 changes: 30 additions & 49 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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},
Expand All @@ -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)
Expand All @@ -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)
}

Expand All @@ -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 {
Expand Down
Loading