Skip to content

Commit

Permalink
Fix prometheusreceiver TA/scrape_config validation logic (open-teleme…
Browse files Browse the repository at this point in the history
…try#30181)

This PR is a fork from open-telemetry#30135 where the author did not respond to the
comments. I applied all suggestions because as a maintainer is my
responsibility to ensure we fix bugs and merge PRs.

Thanks @Aneurysm9 for the first commit.

---------

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com>
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Co-authored-by: Anthony J Mirabella <a9@aneurysm9.com>
  • Loading branch information
2 people authored and cparkins committed Jan 10, 2024
1 parent aa03ff8 commit 515a881
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 10 deletions.
6 changes: 6 additions & 0 deletions .chloggen/fix_promTACfgValidation.yaml
Original file line number Diff line number Diff line change
@@ -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: []
21 changes: 11 additions & 10 deletions receiver/prometheusreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,13 @@ 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
}
if (promConfig == nil || len(promConfig.ScrapeConfigs) == 0) && cfg.TargetAllocator == nil {
return errors.New("no Prometheus scrape_configs or target_allocator set")
}

err := validatePromConfig(promConfig)
if err != nil {
return err
}

if cfg.TargetAllocator != nil {
Expand All @@ -111,11 +113,10 @@ func (cfg *Config) Validate() error {
return nil
}

func (cfg *Config) validatePromConfig(promConfig *promconfig.Config) error {
if len(promConfig.ScrapeConfigs) == 0 && cfg.TargetAllocator == nil {
return errors.New("no Prometheus scrape_configs or target_allocator set")
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
Expand All @@ -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)
Expand Down
33 changes: 33 additions & 0 deletions receiver/prometheusreceiver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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))
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 515a881

Please sign in to comment.