From fb8e544520c668fb94eca2c6e494e693f057ae0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Fri, 5 May 2023 18:35:02 +0200 Subject: [PATCH 1/6] Improve validation for prometheus receiver config --- .../opentelemetrycollector_webhook.go | 8 ++- pkg/collector/reconcile/config_replace.go | 5 ++ pkg/collector/reconcile/configmap.go | 9 ++- .../adapters/config_to_prom_config.go | 26 +++++++ .../adapters/config_to_prom_config_test.go | 68 +++++++++++++++++++ 5 files changed, 112 insertions(+), 4 deletions(-) diff --git a/apis/v1alpha1/opentelemetrycollector_webhook.go b/apis/v1alpha1/opentelemetrycollector_webhook.go index e635870f01..fd9dee5679 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook.go +++ b/apis/v1alpha1/opentelemetrycollector_webhook.go @@ -26,6 +26,8 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" + "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" + ta "github.com/open-telemetry/opentelemetry-operator/pkg/targetallocator/adapters" ) @@ -160,7 +162,11 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { // validate Prometheus config for target allocation if r.Spec.TargetAllocator.Enabled { - _, err := ta.ConfigToPromConfig(r.Spec.Config) + promCfg, err := ta.ConfigToPromConfig(r.Spec.Config) + if err != nil { + return fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) + } + err = ta.ValidatePromConfig(promCfg, r.Spec.TargetAllocator.Enabled, featuregate.EnableTargetAllocatorRewrite.IsEnabled()) if err != nil { return fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) } diff --git a/pkg/collector/reconcile/config_replace.go b/pkg/collector/reconcile/config_replace.go index 25f5083a5e..8eca3842ae 100644 --- a/pkg/collector/reconcile/config_replace.go +++ b/pkg/collector/reconcile/config_replace.go @@ -60,6 +60,11 @@ func ReplaceConfig(instance v1alpha1.OpenTelemetryCollector) (string, error) { return "", getCfgPromErr } + validateCfgPromErr := ta.ValidatePromConfig(promCfgMap, instance.Spec.TargetAllocator.Enabled, featuregate.EnableTargetAllocatorRewrite.IsEnabled()) + if validateCfgPromErr != nil { + return "", validateCfgPromErr + } + // yaml marshaling/unsmarshaling is preferred because of the problems associated with the conversion of map to a struct using mapstructure promCfg, marshalErr := yaml.Marshal(promCfgMap) if marshalErr != nil { diff --git a/pkg/collector/reconcile/configmap.go b/pkg/collector/reconcile/configmap.go index ca3840bfb7..deb5142b1e 100644 --- a/pkg/collector/reconcile/configmap.go +++ b/pkg/collector/reconcile/configmap.go @@ -103,7 +103,7 @@ func desiredTAConfigMap(params Params) (corev1.ConfigMap, error) { labels["app.kubernetes.io/version"] = "latest" } - promConfig, err := ta.ConfigToPromConfig(params.Instance.Spec.Config) + prometheusReceiverConfig, err := ta.ConfigToPromConfig(params.Instance.Spec.Config) if err != nil { return corev1.ConfigMap{}, err } @@ -114,8 +114,11 @@ func desiredTAConfigMap(params Params) (corev1.ConfigMap, error) { "app.kubernetes.io/managed-by": "opentelemetry-operator", "app.kubernetes.io/component": "opentelemetry-collector", } - // We only take the "config" from the returned object, we don't need the "target_allocator" configuration here. - taConfig["config"] = promConfig["config"] + // We only take the "config" from the returned object, if it's present + if prometheusConfig, ok := prometheusReceiverConfig["config"]; ok { + taConfig["config"] = prometheusConfig + } + if len(params.Instance.Spec.TargetAllocator.AllocationStrategy) > 0 { taConfig["allocation_strategy"] = params.Instance.Spec.TargetAllocator.AllocationStrategy } else { diff --git a/pkg/targetallocator/adapters/config_to_prom_config.go b/pkg/targetallocator/adapters/config_to_prom_config.go index 1029753ceb..1d0f86448a 100644 --- a/pkg/targetallocator/adapters/config_to_prom_config.go +++ b/pkg/targetallocator/adapters/config_to_prom_config.go @@ -15,6 +15,7 @@ package adapters import ( + "errors" "fmt" "github.com/open-telemetry/opentelemetry-operator/pkg/collector/adapters" @@ -57,3 +58,28 @@ func ConfigToPromConfig(cfg string) (map[interface{}]interface{}, error) { return prometheus, nil } + +// ValidatePromConfig checks if the prometheus receiver config is valid given other collector-level settings. +func ValidatePromConfig(config map[interface{}]interface{}, targetAllocatorEnabled bool, targetAllocatorRewriteEnabled bool) error { + _, promConfigExists := config["config"] + + if targetAllocatorEnabled { + if targetAllocatorRewriteEnabled { // if rewrite is enabled, we will add a target_allocator section during rewrite + return nil + } + _, targetAllocatorExists := config["target_allocator"] + + // otherwise, either the target_allocator or config section needs to be here + if !(promConfigExists || targetAllocatorExists) { + return errors.New("either target allocator or prometheus config needs to be present") + } + + return nil + } + // if target allocator isn't enabled, we need a config section + if !promConfigExists { + return errorNoComponent("prometheusConfig") + } + + return nil +} diff --git a/pkg/targetallocator/adapters/config_to_prom_config_test.go b/pkg/targetallocator/adapters/config_to_prom_config_test.go index f39aaecf72..a2b0a9688a 100644 --- a/pkg/targetallocator/adapters/config_to_prom_config_test.go +++ b/pkg/targetallocator/adapters/config_to_prom_config_test.go @@ -15,6 +15,7 @@ package adapters_test import ( + "errors" "fmt" "reflect" "testing" @@ -114,3 +115,70 @@ func TestExtractPromConfigFromNullConfig(t *testing.T) { // verify assert.True(t, reflect.ValueOf(promConfig).IsNil()) } + +func TestValidatePromConfig(t *testing.T) { + testCases := []struct { + description string + config map[interface{}]interface{} + targetAllocatorEnabled bool + targetAllocatorRewriteEnabled bool + expectedError error + }{ + { + description: "target_allocator and rewrite enabled", + config: map[interface{}]interface{}{}, + targetAllocatorEnabled: true, + targetAllocatorRewriteEnabled: true, + expectedError: nil, + }, + { + description: "target_allocator enabled, target_allocator section present", + config: map[interface{}]interface{}{ + "target_allocator": map[interface{}]interface{}{}, + }, + targetAllocatorEnabled: true, + targetAllocatorRewriteEnabled: false, + expectedError: nil, + }, + { + description: "target_allocator enabled, config section present", + config: map[interface{}]interface{}{ + "config": map[interface{}]interface{}{}, + }, + targetAllocatorEnabled: true, + targetAllocatorRewriteEnabled: false, + expectedError: nil, + }, + { + description: "target_allocator enabled, neither section present", + config: map[interface{}]interface{}{}, + targetAllocatorEnabled: true, + targetAllocatorRewriteEnabled: false, + expectedError: errors.New("either target allocator or prometheus config needs to be present"), + }, + { + description: "target_allocator disabled, config section present", + config: map[interface{}]interface{}{ + "config": map[interface{}]interface{}{}, + }, + targetAllocatorEnabled: false, + targetAllocatorRewriteEnabled: false, + expectedError: nil, + }, + { + description: "target_allocator disabled, config section not present", + config: map[interface{}]interface{}{}, + targetAllocatorEnabled: false, + targetAllocatorRewriteEnabled: false, + expectedError: fmt.Errorf("no %s available as part of the configuration", "prometheusConfig"), + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.description, func(t *testing.T) { + err := ta.ValidatePromConfig(testCase.config, testCase.targetAllocatorEnabled, testCase.targetAllocatorRewriteEnabled) + assert.Equal(t, testCase.expectedError, err) + }) + } +} From dba8d04f059b1c4b472c5abda8d81392f7edd109 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Tue, 9 May 2023 14:10:59 +0200 Subject: [PATCH 2/6] Improve config validation for target allocator --- cmd/otel-allocator/config/config.go | 10 ++++++++++ cmd/otel-allocator/config/config_test.go | 8 ++++++++ cmd/otel-allocator/config/testdata/no_config.yaml | 0 cmd/otel-allocator/main.go | 14 ++++++++++---- 4 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 cmd/otel-allocator/config/testdata/no_config.yaml diff --git a/cmd/otel-allocator/config/config.go b/cmd/otel-allocator/config/config.go index 7185b5ce8a..a72e69708d 100644 --- a/cmd/otel-allocator/config/config.go +++ b/cmd/otel-allocator/config/config.go @@ -129,3 +129,13 @@ func ParseCLI() (CLIConfig, error) { cLIConf.ClusterConfig = clusterConfig return cLIConf, nil } + +// ValidateConfig validates the cli and file configs together. +func ValidateConfig(config *Config, cliConfig *CLIConfig) error { + if !*cliConfig.PromCRWatcherConf.Enabled && + (config.Config == nil || + len(config.Config.ScrapeConfigs) == 0) { + return fmt.Errorf("either at least one scrape config must be defined, or Prometheus CR watching must be enabled") + } + return nil +} diff --git a/cmd/otel-allocator/config/config_test.go b/cmd/otel-allocator/config/config_test.go index 26349d6d10..3202c88f9d 100644 --- a/cmd/otel-allocator/config/config_test.go +++ b/cmd/otel-allocator/config/config_test.go @@ -91,6 +91,14 @@ func TestLoad(t *testing.T) { }, wantErr: assert.NoError, }, + { + name: "no config", + args: args{ + file: "./testdata/no_config.yaml", + }, + want: Config{}, + wantErr: assert.NoError, + }, { name: "service monitor pod monitor selector", args: args{ diff --git a/cmd/otel-allocator/config/testdata/no_config.yaml b/cmd/otel-allocator/config/testdata/no_config.yaml new file mode 100644 index 0000000000..e69de29bb2 diff --git a/cmd/otel-allocator/main.go b/cmd/otel-allocator/main.go index fbb96e6406..02159819d7 100644 --- a/cmd/otel-allocator/main.go +++ b/cmd/otel-allocator/main.go @@ -74,6 +74,10 @@ func main() { setupLog.Error(configLoadErr, "Unable to load configuration") } + if validationErr := config.ValidateConfig(&cfg, &cliConf); validationErr != nil { + setupLog.Error(validationErr, "Invalid configuration") + } + cliConf.RootLogger.Info("Starting the Target Allocator") ctx := context.Background() log := ctrl.Log.WithName("allocator") @@ -148,10 +152,12 @@ func main() { runGroup.Add( func() error { // Initial loading of the config file's scrape config - err = targetDiscoverer.ApplyConfig(allocatorWatcher.EventSourceConfigMap, cfg.Config) - if err != nil { - setupLog.Error(err, "Unable to apply initial configuration") - return err + if cfg.Config != nil { + err = targetDiscoverer.ApplyConfig(allocatorWatcher.EventSourceConfigMap, cfg.Config) + if err != nil { + setupLog.Error(err, "Unable to apply initial configuration") + return err + } } err := targetDiscoverer.Watch(allocator.SetTargets) setupLog.Info("Target discoverer exited") From 19a80b8a3fb9200f70f4b93e860b86b3341487f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Thu, 11 May 2023 10:40:30 +0200 Subject: [PATCH 3/6] add changelog entry --- .chloggen/fix_validate-ta-config.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100755 .chloggen/fix_validate-ta-config.yaml diff --git a/.chloggen/fix_validate-ta-config.yaml b/.chloggen/fix_validate-ta-config.yaml new file mode 100755 index 0000000000..f3dda56b17 --- /dev/null +++ b/.chloggen/fix_validate-ta-config.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Improve config validation for prometheus receiver and target allocator + +# One or more tracking issues related to the change +issues: [1581] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: From 445a4758778287d10d60af44bd993b602bde1271 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Tue, 16 May 2023 14:39:59 +0200 Subject: [PATCH 4/6] review fixes --- apis/v1alpha1/opentelemetrycollector_webhook.go | 1 - cmd/otel-allocator/config/config.go | 7 +++---- cmd/otel-allocator/main.go | 10 ++++------ cmd/otel-allocator/target/discovery.go | 3 +++ cmd/otel-allocator/target/discovery_test.go | 17 +++++++++++++++++ 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/apis/v1alpha1/opentelemetrycollector_webhook.go b/apis/v1alpha1/opentelemetrycollector_webhook.go index fd9dee5679..a9ef9c0750 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook.go +++ b/apis/v1alpha1/opentelemetrycollector_webhook.go @@ -27,7 +27,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" - ta "github.com/open-telemetry/opentelemetry-operator/pkg/targetallocator/adapters" ) diff --git a/cmd/otel-allocator/config/config.go b/cmd/otel-allocator/config/config.go index a72e69708d..b85e40c9e7 100644 --- a/cmd/otel-allocator/config/config.go +++ b/cmd/otel-allocator/config/config.go @@ -132,10 +132,9 @@ func ParseCLI() (CLIConfig, error) { // ValidateConfig validates the cli and file configs together. func ValidateConfig(config *Config, cliConfig *CLIConfig) error { - if !*cliConfig.PromCRWatcherConf.Enabled && - (config.Config == nil || - len(config.Config.ScrapeConfigs) == 0) { - return fmt.Errorf("either at least one scrape config must be defined, or Prometheus CR watching must be enabled") + scrapeConfigsPresent := (config.Config != nil || len(config.Config.ScrapeConfigs) > 0) + if !(*cliConfig.PromCRWatcherConf.Enabled || scrapeConfigsPresent) { + return fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled") } return nil } diff --git a/cmd/otel-allocator/main.go b/cmd/otel-allocator/main.go index 02159819d7..a33d31b48b 100644 --- a/cmd/otel-allocator/main.go +++ b/cmd/otel-allocator/main.go @@ -152,12 +152,10 @@ func main() { runGroup.Add( func() error { // Initial loading of the config file's scrape config - if cfg.Config != nil { - err = targetDiscoverer.ApplyConfig(allocatorWatcher.EventSourceConfigMap, cfg.Config) - if err != nil { - setupLog.Error(err, "Unable to apply initial configuration") - return err - } + err = targetDiscoverer.ApplyConfig(allocatorWatcher.EventSourceConfigMap, cfg.Config) + if err != nil { + setupLog.Error(err, "Unable to apply initial configuration") + return err } err := targetDiscoverer.Watch(allocator.SetTargets) setupLog.Info("Target discoverer exited") diff --git a/cmd/otel-allocator/target/discovery.go b/cmd/otel-allocator/target/discovery.go index 38d99b7985..80c818b1f8 100644 --- a/cmd/otel-allocator/target/discovery.go +++ b/cmd/otel-allocator/target/discovery.go @@ -64,6 +64,9 @@ func NewDiscoverer(log logr.Logger, manager *discovery.Manager, hook discoveryHo } func (m *Discoverer) ApplyConfig(source allocatorWatcher.EventSource, cfg *config.Config) error { + if cfg == nil { + return nil + } m.configsMap[source] = cfg jobToScrapeConfig := make(map[string]*config.ScrapeConfig) diff --git a/cmd/otel-allocator/target/discovery_test.go b/cmd/otel-allocator/target/discovery_test.go index de2f1070e6..49bfaa471d 100644 --- a/cmd/otel-allocator/target/discovery_test.go +++ b/cmd/otel-allocator/target/discovery_test.go @@ -334,6 +334,23 @@ func TestDiscovery_ScrapeConfigHashing(t *testing.T) { } } +func TestDiscovery_NoConfig(t *testing.T) { + scu := &mockScrapeConfigUpdater{mockCfg: map[string]*promconfig.ScrapeConfig{}} + ctx, cancelFunc := context.WithCancel(context.Background()) + d := discovery.NewManager(ctx, gokitlog.NewNopLogger()) + manager := NewDiscoverer(ctrl.Log.WithName("test"), d, nil, scu) + defer close(manager.close) + defer cancelFunc() + + go func() { + err := d.Run() + assert.NoError(t, err) + }() + // check the updated scrape configs + expectedScrapeConfigs := map[string]*promconfig.ScrapeConfig{} + assert.Equal(t, expectedScrapeConfigs, scu.mockCfg) +} + func BenchmarkApplyScrapeConfig(b *testing.B) { numConfigs := 1000 scrapeConfig := promconfig.ScrapeConfig{ From 9c2cbf6723c89163bea8510e63757d6fc3a69d28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Tue, 16 May 2023 17:05:46 +0200 Subject: [PATCH 5/6] log empty Prometheus configs passed to discoverer --- cmd/otel-allocator/target/discovery.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/otel-allocator/target/discovery.go b/cmd/otel-allocator/target/discovery.go index 80c818b1f8..70cf82a374 100644 --- a/cmd/otel-allocator/target/discovery.go +++ b/cmd/otel-allocator/target/discovery.go @@ -65,6 +65,7 @@ func NewDiscoverer(log logr.Logger, manager *discovery.Manager, hook discoveryHo func (m *Discoverer) ApplyConfig(source allocatorWatcher.EventSource, cfg *config.Config) error { if cfg == nil { + m.log.Info("Service Discovery got empty Prometheus config", "source", source.String()) return nil } m.configsMap[source] = cfg From c09cce372f178e1f3771387625c0e3a78837c7d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Mon, 22 May 2023 18:04:10 +0200 Subject: [PATCH 6/6] add tests for ValidateConfig --- cmd/otel-allocator/config/config.go | 2 +- cmd/otel-allocator/config/config_test.go | 53 ++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/cmd/otel-allocator/config/config.go b/cmd/otel-allocator/config/config.go index b85e40c9e7..569eac8510 100644 --- a/cmd/otel-allocator/config/config.go +++ b/cmd/otel-allocator/config/config.go @@ -132,7 +132,7 @@ func ParseCLI() (CLIConfig, error) { // ValidateConfig validates the cli and file configs together. func ValidateConfig(config *Config, cliConfig *CLIConfig) error { - scrapeConfigsPresent := (config.Config != nil || len(config.Config.ScrapeConfigs) > 0) + scrapeConfigsPresent := (config.Config != nil && len(config.Config.ScrapeConfigs) > 0) if !(*cliConfig.PromCRWatcherConf.Enabled || scrapeConfigsPresent) { return fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled") } diff --git a/cmd/otel-allocator/config/config_test.go b/cmd/otel-allocator/config/config_test.go index 3202c88f9d..65e28aeefd 100644 --- a/cmd/otel-allocator/config/config_test.go +++ b/cmd/otel-allocator/config/config_test.go @@ -165,3 +165,56 @@ func TestLoad(t *testing.T) { }) } } + +func TestValidateConfig(t *testing.T) { + enabled := true + disabled := false + testCases := []struct { + name string + cliConfig CLIConfig + fileConfig Config + expectedErr error + }{ + { + name: "promCR enabled, no Prometheus config", + cliConfig: CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &enabled}}, + fileConfig: Config{Config: nil}, + expectedErr: nil, + }, + { + name: "promCR disabled, no Prometheus config", + cliConfig: CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &disabled}}, + fileConfig: Config{Config: nil}, + expectedErr: fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled"), + }, + { + name: "promCR disabled, Prometheus config present, no scrapeConfigs", + cliConfig: CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &disabled}}, + fileConfig: Config{Config: &promconfig.Config{}}, + expectedErr: fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled"), + }, + { + name: "promCR disabled, Prometheus config present, scrapeConfigs present", + cliConfig: CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &disabled}}, + fileConfig: Config{ + Config: &promconfig.Config{ScrapeConfigs: []*promconfig.ScrapeConfig{{}}}, + }, + expectedErr: nil, + }, + { + name: "promCR enabled, Prometheus config present, scrapeConfigs present", + cliConfig: CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &enabled}}, + fileConfig: Config{ + Config: &promconfig.Config{ScrapeConfigs: []*promconfig.ScrapeConfig{{}}}, + }, + expectedErr: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateConfig(&tc.fileConfig, &tc.cliConfig) + assert.Equal(t, tc.expectedErr, err) + }) + } +}