Improve config validation for prometheus receiver and target allocator #1729

swiatekm · 2023-05-10T11:13:30Z

In a follow-up to #1688, improve validation for prometheus receiver configuration. This turned out to be more complicated than expected, so I'd like some more eyes to verify I haven't missed anything.

The validation logic for the prometheus receiver config itself is:

if target allocator is not enabled on the Collector CR, prometheus config under the config key is required
if target allocator is enabled
- either config has to exist to inject http_sd_configs, or target_allocator needs to exist
- if additionally target allocator rewrite is enabled, nothing is required

However, I also noticed that target allocator itself doesn't correctly validate its configuration. For example, it's possible to pass an empty Prometheus config, but this will cause a crash in the targetDiscoverer. So I added an additional check, requiring that either:

there is a non-empty Prometheus config
or PrometheusCR is enabled

I submitted both of these changes in a single PR, as they're related, but I can resubmit separately if that'll make review easier.

matej-g

This looks like a nice addition! 💪 I added only a few nits.

apis/v1alpha1/opentelemetrycollector_webhook.go

matej-g · 2023-05-11T14:42:29Z

cmd/otel-allocator/main.go

-				setupLog.Error(err, "Unable to apply initial configuration")
-				return err
+			if cfg.Config != nil {
+				err = targetDiscoverer.ApplyConfig(allocatorWatcher.EventSourceConfigMap, cfg.Config)


I wonder whether it would make more sense to check for cfg.Config == nil inside of ApplyConfig and if so, just do early return

I thought about it, but realistically, this is the only place in the whole package where this can happen, because we're getting the config from user input. So I think it's better to keep this bit of complexity here instead of in the component. We could also just have the Config itself come with a non-nil default if the user doesn't supply it.

could you actually reverse this condition such that we do

if cfg.Config == nil { setupLog.Error("cannot apply an empty configuration") return fmt.Error("...") }

That way we prevent startup correctly on bad config?

I can't reverse it if it stays where it is in main, as cfg.Config == nil by itself does not imply an error. Or do you mean doing this inside ApplyConfig?

wouldn't cfg.Config == nil imply an error is i guess my conjecture here. i.e. if we don't have a config to apply at all we should fail fast.

@swiatekm-sumo IMHO still it's an exported method, could be used anywhere else. That way whoever re-uses this method in whichever place they do, do not need to worry about the nil condition.

@matej-g makes sense, you've convinced me.

@jaronoff97 Having the raw Prometheus config be nil and Prometheus CR handling enabled looks like a valid configuration to me. That's why I added the validate method and check for this specific state.

I moved the check to ApplyConfig and added a Discoverer test for this.

matej-g · 2023-05-11T14:46:06Z

cmd/otel-allocator/config/config.go

+	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")


Small nit

Suggested change

return fmt.Errorf("either at least one scrape config must be defined, or Prometheus CR watching must be enabled")

return fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled")

i'd prefer us to actually split up this if in to three statements. Although it's more LOC we'll be able to give a more descriptive error back which I think would be better.

I wrote it this way anticipating that we might want to add more checks in the future, in which case they all need to be negative checks (as in "this error condition doesn't occur"). And I can't really rewrite it into three separate statements, as it's logically an OR.

I could make it nested instead, but I'm not sure if you can really make the error message any more useful that way.

I rewrote this condition to be clearer, let me know if this helps.

cmd/otel-allocator/config/config.go

jaronoff97

just a few thoughts, overall this is going to be super useful though. thank you!

jaronoff97 · 2023-05-11T15:29:07Z

cmd/otel-allocator/config/config.go

+	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")


i'd prefer us to actually split up this if in to three statements. Although it's more LOC we'll be able to give a more descriptive error back which I think would be better.

jaronoff97 · 2023-05-11T15:31:15Z

cmd/otel-allocator/main.go

-				setupLog.Error(err, "Unable to apply initial configuration")
-				return err
+			if cfg.Config != nil {
+				err = targetDiscoverer.ApplyConfig(allocatorWatcher.EventSourceConfigMap, cfg.Config)


could you actually reverse this condition such that we do

if cfg.Config == nil { setupLog.Error("cannot apply an empty configuration") return fmt.Error("...") }

That way we prevent startup correctly on bad config?

jaronoff97

just a few thoughts, overall this is going to be super useful though. thank you!

cmd/otel-allocator/target/discovery.go

jaronoff97 · 2023-05-16T22:09:37Z

@swiatekm-sumo my colleague @kristinapathak is also going to take a look at this as well before i merge. We've been deep in to configuration related PRs for the TA as of late so this will be a good continuation.

kristinapathak

This looks great! Left two questions/suggestions.

cmd/otel-allocator/config/config.go

kristinapathak · 2023-05-18T03:30:54Z

pkg/collector/reconcile/config_replace.go

@@ -57,6 +57,11 @@ func ReplaceConfig(instance v1alpha1.OpenTelemetryCollector) (string, error) {
 		return "", getCfgPromErr
 	}

+	validateCfgPromErr := ta.ValidatePromConfig(promCfgMap, instance.Spec.TargetAllocator.Enabled, featuregate.EnableTargetAllocatorRewrite.IsEnabled())


Is ValidatePromConfig() meant to be called every time ta.ConfigToPromConfig() is called? Put another way, is there ever a situation when we want to call one function without the other? I'm wondering the benefit of two separate functions. Is it separation of concerns? Not making a breaking change to the adapters pkg API?

Not making a breaking change and keeping current semantics of ta.ConfigToPromConfig() is part of it, although the separation of concerns is a nice bonus from that. Though overall there isn't any strong reason behind it, if we're ok breaking ConfigToPromConfig, we could simply make validation a private function and have ConfigToPromConfig call it.

that sounds like a good followup for the future :)

jaronoff97 · 2023-05-24T21:45:01Z

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")


in a future version should we fast fail here?

jaronoff97 · 2023-05-24T21:46:20Z

pkg/collector/reconcile/config_replace.go

@@ -57,6 +57,11 @@ func ReplaceConfig(instance v1alpha1.OpenTelemetryCollector) (string, error) {
 		return "", getCfgPromErr
 	}

+	validateCfgPromErr := ta.ValidatePromConfig(promCfgMap, instance.Spec.TargetAllocator.Enabled, featuregate.EnableTargetAllocatorRewrite.IsEnabled())


that sounds like a good followup for the future :)

open-telemetry#1729) * Improve validation for prometheus receiver config * Improve config validation for target allocator * add changelog entry * review fixes * log empty Prometheus configs passed to discoverer * add tests for ValidateConfig

swiatekm force-pushed the fix/validate-ta-config branch 5 times, most recently from e2507b9 to e2c14ac Compare May 11, 2023 11:35

swiatekm marked this pull request as ready for review May 11, 2023 11:41

swiatekm requested review from a team May 11, 2023 11:41

matej-g approved these changes May 11, 2023

View reviewed changes

jaronoff97 reviewed May 11, 2023

View reviewed changes

swiatekm requested a review from jaronoff97 May 16, 2023 13:36

jaronoff97 reviewed May 16, 2023

View reviewed changes

cmd/otel-allocator/target/discovery.go Show resolved Hide resolved

kristinapathak reviewed May 18, 2023

View reviewed changes

swiatekm requested a review from kristinapathak May 22, 2023 16:05

Mikołaj Świątek added 6 commits May 24, 2023 19:11

Improve validation for prometheus receiver config

fb8e544

Improve config validation for target allocator

dba8d04

add changelog entry

19a80b8

review fixes

445a475

log empty Prometheus configs passed to discoverer

9c2cbf6

add tests for ValidateConfig

c09cce3

swiatekm force-pushed the fix/validate-ta-config branch from 7479876 to 1515f43 Compare May 24, 2023 19:04

swiatekm requested a review from jaronoff97 May 24, 2023 19:05

swiatekm force-pushed the fix/validate-ta-config branch 2 times, most recently from b774b95 to c09cce3 Compare May 24, 2023 20:24

jaronoff97 approved these changes May 24, 2023

View reviewed changes

jaronoff97 merged commit 0a92a11 into open-telemetry:main May 24, 2023

swiatekm deleted the fix/validate-ta-config branch May 25, 2023 07:42

This was referenced May 25, 2023

[target-allocator] Exit on invalid config #1767

Merged

REQUEST: New membership for @swiatekm-sumo open-telemetry/community#1512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve config validation for prometheus receiver and target allocator #1729

Improve config validation for prometheus receiver and target allocator #1729

swiatekm commented May 10, 2023 •

edited

Loading

matej-g left a comment

matej-g May 11, 2023

swiatekm May 11, 2023

jaronoff97 May 11, 2023

swiatekm May 11, 2023 •

edited

Loading

jaronoff97 May 11, 2023

matej-g May 15, 2023

swiatekm May 15, 2023

swiatekm May 16, 2023 •

edited

Loading

matej-g May 11, 2023

jaronoff97 May 11, 2023

swiatekm May 11, 2023

swiatekm May 16, 2023

jaronoff97 left a comment

jaronoff97 May 11, 2023

jaronoff97 May 11, 2023

jaronoff97 left a comment

jaronoff97 commented May 16, 2023

kristinapathak left a comment

kristinapathak May 18, 2023

swiatekm May 18, 2023

jaronoff97 May 24, 2023

jaronoff97 May 24, 2023

jaronoff97 May 24, 2023

	return fmt.Errorf("either at least one scrape config must be defined, or Prometheus CR watching must be enabled")
	return fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled")

Improve config validation for prometheus receiver and target allocator #1729

Improve config validation for prometheus receiver and target allocator #1729

Conversation

swiatekm commented May 10, 2023 • edited Loading

matej-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swiatekm May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swiatekm May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

jaronoff97 commented May 16, 2023

kristinapathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swiatekm commented May 10, 2023 •

edited

Loading

swiatekm May 11, 2023 •

edited

Loading

swiatekm May 16, 2023 •

edited

Loading