From e2c168e780655a0313fb8c813c275aa3d2a41e5e Mon Sep 17 00:00:00 2001 From: Anthony J Mirabella Date: Tue, 19 Dec 2023 18:27:29 -0500 Subject: [PATCH 1/3] Fix prometheusreceiver TA/scrape_config validation logic Signed-off-by: Anthony J Mirabella --- .chloggen/fix_promTACfgValidation.yaml | 6 ++++ receiver/prometheusreceiver/config.go | 13 ++++---- receiver/prometheusreceiver/config_test.go | 33 +++++++++++++++++++ ...prometheus-non-existent-scrape-config.yaml | 19 +++++++++++ 4 files changed, 65 insertions(+), 6 deletions(-) create mode 100755 .chloggen/fix_promTACfgValidation.yaml create mode 100644 receiver/prometheusreceiver/testdata/invalid-config-prometheus-non-existent-scrape-config.yaml diff --git a/.chloggen/fix_promTACfgValidation.yaml b/.chloggen/fix_promTACfgValidation.yaml new file mode 100755 index 000000000000..1ec7ff29eae5 --- /dev/null +++ b/.chloggen/fix_promTACfgValidation.yaml @@ -0,0 +1,6 @@ +change_type: 'bug_fix' +component: 'prometheusreceiver' +note: Fix configuration validation to allow specification of Target Allocator configuration without providing scrape configurations +issues: [30135] +subtext: +change_logs: [] diff --git a/receiver/prometheusreceiver/config.go b/receiver/prometheusreceiver/config.go index edcd0d2e5861..82de84c5efe6 100644 --- a/receiver/prometheusreceiver/config.go +++ b/receiver/prometheusreceiver/config.go @@ -95,11 +95,9 @@ func checkTLSConfig(tlsConfig commonconfig.TLSConfig) error { // Validate checks the receiver configuration is valid. func (cfg *Config) Validate() error { promConfig := cfg.PrometheusConfig - if promConfig != nil { - err := cfg.validatePromConfig(promConfig) - if err != nil { - return err - } + err := cfg.validatePromConfig(promConfig) + if err != nil { + return err } if cfg.TargetAllocator != nil { @@ -112,8 +110,11 @@ func (cfg *Config) Validate() error { } func (cfg *Config) validatePromConfig(promConfig *promconfig.Config) error { - if len(promConfig.ScrapeConfigs) == 0 && cfg.TargetAllocator == nil { + if (promConfig == nil || len(promConfig.ScrapeConfigs) == 0) && cfg.TargetAllocator == nil { return errors.New("no Prometheus scrape_configs or target_allocator set") + } else if promConfig == nil { + // nothing to validate + return nil } // Reject features that Prometheus supports but that the receiver doesn't support: diff --git a/receiver/prometheusreceiver/config_test.go b/receiver/prometheusreceiver/config_test.go index 5289ae4c8a5c..647ff8f2c55f 100644 --- a/receiver/prometheusreceiver/config_test.go +++ b/receiver/prometheusreceiver/config_test.go @@ -66,6 +66,7 @@ func TestLoadTargetAllocatorConfig(t *testing.T) { sub, err := cm.Sub(component.NewIDWithName(metadata.Type, "").String()) require.NoError(t, err) require.NoError(t, component.UnmarshalConfig(sub, cfg)) + require.NoError(t, component.ValidateConfig(cfg)) r0 := cfg.(*Config) assert.NotNil(t, r0.PrometheusConfig) @@ -77,6 +78,7 @@ func TestLoadTargetAllocatorConfig(t *testing.T) { require.NoError(t, err) cfg = factory.CreateDefaultConfig() require.NoError(t, component.UnmarshalConfig(sub, cfg)) + require.NoError(t, component.ValidateConfig(cfg)) r1 := cfg.(*Config) assert.NotNil(t, r0.PrometheusConfig) @@ -92,6 +94,7 @@ func TestLoadTargetAllocatorConfig(t *testing.T) { require.NoError(t, err) cfg = factory.CreateDefaultConfig() require.NoError(t, component.UnmarshalConfig(sub, cfg)) + require.NoError(t, component.ValidateConfig(cfg)) r2 := cfg.(*Config) assert.Equal(t, 1, len(r2.PrometheusConfig.ScrapeConfigs)) @@ -110,6 +113,36 @@ func TestLoadConfigFailsOnUnknownSection(t *testing.T) { require.Error(t, component.UnmarshalConfig(sub, cfg)) } +func TestLoadConfigFailsOnNoPrometheusOrTAConfig(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "invalid-config-prometheus-non-existent-scrape-config.yaml")) + require.NoError(t, err) + factory := NewFactory() + + cfg := factory.CreateDefaultConfig() + sub, err := cm.Sub(component.NewIDWithName(metadata.Type, "").String()) + require.NoError(t, err) + require.NoError(t, component.UnmarshalConfig(sub, cfg)) + require.ErrorContains(t, component.ValidateConfig(cfg), "no Prometheus scrape_configs or target_allocator set") + + cfg = factory.CreateDefaultConfig() + sub, err = cm.Sub(component.NewIDWithName(metadata.Type, "withConfigAndTA").String()) + require.NoError(t, err) + require.NoError(t, component.UnmarshalConfig(sub, cfg)) + require.NoError(t, component.ValidateConfig(cfg)) + + cfg = factory.CreateDefaultConfig() + sub, err = cm.Sub(component.NewIDWithName(metadata.Type, "withOnlyTA").String()) + require.NoError(t, err) + require.NoError(t, component.UnmarshalConfig(sub, cfg)) + require.NoError(t, component.ValidateConfig(cfg)) + + cfg = factory.CreateDefaultConfig() + sub, err = cm.Sub(component.NewIDWithName(metadata.Type, "withOnlyScrape").String()) + require.NoError(t, err) + require.NoError(t, component.UnmarshalConfig(sub, cfg)) + require.NoError(t, component.ValidateConfig(cfg)) +} + // As one of the config parameters is consuming prometheus // configuration as a subkey, ensure that invalid configuration // within the subkey will also raise an error. diff --git a/receiver/prometheusreceiver/testdata/invalid-config-prometheus-non-existent-scrape-config.yaml b/receiver/prometheusreceiver/testdata/invalid-config-prometheus-non-existent-scrape-config.yaml new file mode 100644 index 000000000000..a3a09838b45c --- /dev/null +++ b/receiver/prometheusreceiver/testdata/invalid-config-prometheus-non-existent-scrape-config.yaml @@ -0,0 +1,19 @@ +prometheus: +prometheus/withConfigAndTA: + target_allocator: + endpoint: http://localhost:8080 + interval: 30s + collector_id: collector-1 + config: + global: + scrape_interval: 30s +prometheus/withOnlyTA: + target_allocator: + endpoint: http://localhost:8080 + interval: 30s + collector_id: collector-1 +prometheus/withOnlyScrape: + config: + scrape_configs: + - job_name: 'demo' + scrape_interval: 5s From 90dede4e3e6f665d34a214b33be3cdf663a30807 Mon Sep 17 00:00:00 2001 From: Anthony J Mirabella Date: Tue, 19 Dec 2023 20:11:10 -0500 Subject: [PATCH 2/3] PR feedback Signed-off-by: Anthony J Mirabella --- receiver/prometheusreceiver/config.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/receiver/prometheusreceiver/config.go b/receiver/prometheusreceiver/config.go index 82de84c5efe6..1510f16d781c 100644 --- a/receiver/prometheusreceiver/config.go +++ b/receiver/prometheusreceiver/config.go @@ -95,9 +95,15 @@ func checkTLSConfig(tlsConfig commonconfig.TLSConfig) error { // Validate checks the receiver configuration is valid. func (cfg *Config) Validate() error { promConfig := cfg.PrometheusConfig - err := cfg.validatePromConfig(promConfig) - if err != nil { - return err + if (promConfig == nil || len(promConfig.ScrapeConfigs) == 0) && cfg.TargetAllocator == nil { + return errors.New("no Prometheus scrape_configs or target_allocator set") + } + + if promConfig != nil { + err := cfg.validatePromConfig(promConfig) + if err != nil { + return err + } } if cfg.TargetAllocator != nil { @@ -110,13 +116,6 @@ func (cfg *Config) Validate() error { } func (cfg *Config) validatePromConfig(promConfig *promconfig.Config) error { - if (promConfig == nil || len(promConfig.ScrapeConfigs) == 0) && cfg.TargetAllocator == nil { - return errors.New("no Prometheus scrape_configs or target_allocator set") - } else if promConfig == nil { - // nothing to validate - return nil - } - // Reject features that Prometheus supports but that the receiver doesn't support: // See: // * https://github.com/open-telemetry/opentelemetry-collector/issues/3863 From 8cc77b6e13184cebb40561b2e25f229787ae6b40 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Fri, 22 Dec 2023 09:12:06 -0800 Subject: [PATCH 3/3] Apply changes suggested by @bogdandrutu in #30135 Signed-off-by: Bogdan Drutu --- receiver/prometheusreceiver/config.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/receiver/prometheusreceiver/config.go b/receiver/prometheusreceiver/config.go index 1510f16d781c..728a0e73a019 100644 --- a/receiver/prometheusreceiver/config.go +++ b/receiver/prometheusreceiver/config.go @@ -99,11 +99,9 @@ func (cfg *Config) Validate() error { return errors.New("no Prometheus scrape_configs or target_allocator set") } - if promConfig != nil { - err := cfg.validatePromConfig(promConfig) - if err != nil { - return err - } + err := validatePromConfig(promConfig) + if err != nil { + return err } if cfg.TargetAllocator != nil { @@ -115,7 +113,10 @@ func (cfg *Config) Validate() error { return nil } -func (cfg *Config) validatePromConfig(promConfig *promconfig.Config) error { +func validatePromConfig(promConfig *promconfig.Config) error { + if promConfig == nil { + return nil + } // Reject features that Prometheus supports but that the receiver doesn't support: // See: // * https://github.com/open-telemetry/opentelemetry-collector/issues/3863 @@ -142,7 +143,7 @@ func (cfg *Config) validatePromConfig(promConfig *promconfig.Config) error { return fmt.Errorf("unsupported features:\n\t%s", strings.Join(unsupportedFeatures, "\n\t")) } - for _, sc := range cfg.PrometheusConfig.ScrapeConfigs { + for _, sc := range promConfig.ScrapeConfigs { if sc.HTTPClientConfig.Authorization != nil { if err := checkFile(sc.HTTPClientConfig.Authorization.CredentialsFile); err != nil { return fmt.Errorf("error checking authorization credentials file %q: %w", sc.HTTPClientConfig.Authorization.CredentialsFile, err)