fix: reject monitors with invalid relabel configs

[simonpasquier]

Description

I noticed that probes, service monitors, pod monitors and scrape configs with invalid relabel configs were not rejected by the operator. This PR fixes it.

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

• CHANGE (fix or feature that would cause existing functionality to not work as expected)
• FEATURE (non-breaking change which adds functionality)
• BUGFIX (non-breaking change which fixes an issue)
• ENHANCEMENT (non-breaking change which improves existing functionality)
• NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

reject ServiceMonitor, PodMonitor, Probe and ScrapeConfig with invalid relabel configs.

[slashpai]

shouldn't this be already taken care since we set default as replace?

prometheus-operator/pkg/apis/monitoring/v1/prometheus_types.go

Lines 1340 to 1341 in 78a2875

	// +kubebuilder:default=replace
	Action string `json:"action,omitempty"`

[simonpasquier]

This is for tests + environments that don't have a recent version of the CRD (e.g. one without the default value).

[ArthurSens]

I'm struggling a little bit to understand what/where is the bug that you're fixing here 😬

Do I understand it correctly that we're allowing actions with a mixture of Lower/Upper case and then we fail to compare with relabel.<action name>?

If that's the case, what do you think about passing this validation to kubebuilder annotations?

[simonpasquier]

I'm struggling a little bit to understand what/where is the bug that you're fixing here 😬

One issue is here:

prometheus-operator/pkg/prometheus/resource_selector.go

Lines 535 to 548 in 34bb905

	for _, rl := range probe.Spec.MetricRelabelConfigs {
	if rl.Action != "" {
	if err = validateRelabelConfig(rs.p, *rl); err != nil {
	rejectFn(probe, err)
	continue
	}
	}
	}
	if err = validateProberURL(probe.Spec.ProberSpec.URL); err != nil {
	err := errors.Wrapf(err, "%s url specified in proberSpec is invalid, it should be of the format `hostname` or `hostname:port`", probe.Spec.ProberSpec.URL)
	rejectFn(probe, err)
	continue
	}
	res[probeName] = probe

If an invalid relabel config is detected, it will be logged but it won't continue from the outer for loop so the probe will be added to the map.

Another issue here:

prometheus-operator/pkg/prometheus/resource_selector.go

Lines 155 to 172 in 34bb905

	for _, rl := range endpoint.RelabelConfigs {
	if rl.Action != "" {
	if err = validateRelabelConfig(rs.p, *rl); err != nil {
	break
	}
	}
	}
	for _, rl := range endpoint.MetricRelabelConfigs {
	if rl.Action != "" {
	if err = validateRelabelConfig(rs.p, *rl); err != nil {
	break
	}
	}
	}
	}
	if err != nil {

If L157 returns an error, err might be overwritten at L165 and again the service monitor isn't rejected...

TL;DR: write unit tests to assert that the code is doing what it should ;)

Do I understand it correctly that we're allowing actions with a mixture of Lower/Upper case and then we fail to compare with relabel.<action name>?

If that's the case, what do you think about passing this validation to kubebuilder annotations?

No upper/lower case actions are fine (in fact the Prometheus config supports everything but will always compare to the lower-case version).

[simonpasquier]

I'm not sure why we had this test in the first place. I've modified validateRelabelConfig() to default to the replace action if the field is empty.

[ArthurSens]

I'm struggling a little bit to understand what/where is the bug that you're fixing here 😬

One issue is here:

prometheus-operator/pkg/prometheus/resource_selector.go

Lines 535 to 548 in 34bb905

	for _, rl := range probe.Spec.MetricRelabelConfigs {
	if rl.Action != "" {
	if err = validateRelabelConfig(rs.p, *rl); err != nil {
	rejectFn(probe, err)
	continue
	}
	}
	}
	if err = validateProberURL(probe.Spec.ProberSpec.URL); err != nil {
	err := errors.Wrapf(err, "%s url specified in proberSpec is invalid, it should be of the format `hostname` or `hostname:port`", probe.Spec.ProberSpec.URL)
	rejectFn(probe, err)
	continue
	}
	res[probeName] = probe

If an invalid relabel config is detected, it will be logged but it won't continue from the outer for loop so the probe will be added to the map.

Another issue here:

prometheus-operator/pkg/prometheus/resource_selector.go

Lines 155 to 172 in 34bb905

	for _, rl := range endpoint.RelabelConfigs {
	if rl.Action != "" {
	if err = validateRelabelConfig(rs.p, *rl); err != nil {
	break
	}
	}
	}
	for _, rl := range endpoint.MetricRelabelConfigs {
	if rl.Action != "" {
	if err = validateRelabelConfig(rs.p, *rl); err != nil {
	break
	}
	}
	}
	}
	if err != nil {

If L157 returns an error, err might be overwritten at L165 and again the service monitor isn't rejected...

TL;DR: write unit tests to assert that the code is doing what it should ;)

Do I understand it correctly that we're allowing actions with a mixture of Lower/Upper case and then we fail to compare with relabel.<action name>?
If that's the case, what do you think about passing this validation to kubebuilder annotations?

No upper/lower case actions are fine (in fact the Prometheus config supports everything but will always compare to the lower-case version).

Ooooh now I get it, we're breaking the inner loop when we wanted to break the outer loop. Thanks for the explanation!