Add extra relabelings to scrape classes

[QuentinBisson]

Description

Fixes #3255

This PR allows users of the operator to ensure a set of custom relabelings to all their pod/service monitors.

This superseds this PR #5978

Notable examples would be adding the node label using __meta_kubernetes_pod_node_name but it could be used for other organization purposes such as:

- sourceLabels: [__meta_kubernetes_pod_label_app_kubernetes_io_name]
  action: "replace"
  targetLabel: app

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

Verification

Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.

Changelog entry

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

Allow addition of extra relabelings to all scrape classes

[QuentinBisson]

@nicolastakashi and @ArthurSens now that scrape classes are finally out, could you check this one? :)

[simonpasquier]

Thanks for kicking this!

[simonpasquier]

It should be obvious that it applies to the target relabeling rules and when the scrape class' relabelings are applied compared to the monitor object's relabelings.

[QuentinBisson]

I took the comment fom the podmonitor CR and adjusted it to also add when the scrape class relabelings are applied. Could you check again?

[simonpasquier]

I'm not sure why we went with []*RelabelConfig in the first place for service/pod monitors but I'd rather use []RelabelConfig.

[QuentinBisson]

The issue with this is that the generator function requires an array of pointers:
func generateRelabelConfig(rc []*monitoringv1.RelabelConfig) []yaml.MapSlice

[simonpasquier]

Right, let's keep it like that for now and tackle my remark in another PR.

[ArthurSens]

Thanks for getting back to this!

I've made a few comments but I think we're pretty close already :)

[ArthurSens]

Suggested change

	ExtraRelabelings []*RelabelConfig `json:"extraRelabelings,omitempty"`
	Relabelings []*RelabelConfig `json:"relabelings,omitempty"`

I'm still not sure about my suggestion, but I think "Extra" might be redundant with the concept of scrape classes. What do you think?

[QuentinBisson]

That's fine with me :)

[QuentinBisson]

It's now renamed :)

[ArthurSens]

Non-blocking!

This part is duplicated with the implementation of MergeTLSConfigWithScrapeClass and will probably repeat itself for every other field we add to ScrapeClasses. It would be nice if we could find a way of removing this duplication 🤔

[QuentinBisson]

So I just noticed that we use this

prometheus-operator/pkg/prometheus/promcfg.go

Line 837 in e8d020e

scrapeClass := cg.getScrapeClassOrDefault(m.Spec.ScrapeClassName)

at the top of all configured resources and it returns nil if there is no scrape class so I simplified this PR's code and maybe the same should be done for the existing fields

[ArthurSens]

I wonder if we should log or warn through admission webhooks if we have clashing relabeling rules in the ScrapeClass and the underlying resource.

I guess we can start with a log and iterate with validating webhooks later?

[QuentinBisson]

I think this might then become an issue for use case where it actually make sense to override some custom fields though (not that I have one in mind)

[nicolastakashi]

I'd keep the consistency with the TLS feature on the ScrapeClass.
If the Monitor Resource already has a relabel config we should merge keeping and the existing config on the monitor object will take precedence over the one defined on the scrape class.

[QuentinBisson]

Due the the nature of relabelings, should we merge them or let them apply after the other ? I'm afraid merging will cause unkown side effects to users

[simonpasquier]

I agree that a "merging" strategy would be hard to get right. My expectation would be that the scrape class relabelings apply first then the service/pod monitor's relabelings. The scrape class relabelings may delete a discovered label that the monitor relabelings rely on but I feel like it's a corner case and not something that the operator could/should prevent.

[QuentinBisson]

Thanks for your really fast reviews. I tried to adjust to fix your comments :) Let me know how I can improve

[simonpasquier]

We need to call validateRelabelConfigs() on the scrape class relabelings and fail the reconciliation if invalid.

prometheus-operator/pkg/prometheus/resource_selector.go

Lines 244 to 256 in 4030eb7

	func validateRelabelConfigs(p monitoringv1.PrometheusInterface, rcs []*monitoringv1.RelabelConfig) error {
	for i, rc := range rcs {
	if rc == nil {
	return fmt.Errorf("null relabel config")
	}
	if err := validateRelabelConfig(p, *rc); err != nil {
	return fmt.Errorf("[%d]: %w", i, err)
	}
	}
	return nil
	}

[QuentinBisson]

Validation is added

[simonpasquier]

Ideally it would need to happen before (e.g. at the beginning of the reconciliation) because it's a problem with the Prometheus/PrometheusAgent spec not with the monitoring object.

[QuentinBisson]

Oh makes sensé but I could not find where to put the validation. Could you give me a hint ?

[QuentinBisson]

I think I found a nice place in the NewConfigGenerator function. Can you check the new version?

[simonpasquier]

I don't think that wrapping the error is required here.

[QuentinBisson]

Oh I did that because it is wrapped for the service monitor validation

[QuentinBisson]

In that case should I remove it from thé servicemonitor validation ?

[QuentinBisson]

I removed it for all 4 places

[QuentinBisson]

I'm not sure why those were ignored, maybe I should not have fixed them in this PR

[QuentinBisson]

@simonpasquier This is where I thought it would be best but I'm not sure about the error message. Do you have any suggestions?

[simonpasquier]

Looks good to me
(nit) validateRelabelConfigs should work with nil slices too.

Suggested change

if scrapeClass.Relabelings != nil {

[QuentinBisson]

So I'm not sure why there this was changed to _ as it was not when I initially did my first PR. It was named servicemonitor first and the issue is that the sm variable defined line 121 was used instead so I renamed it to sm.

I'm not sure there is a reason to even have the servicemonitor paramater in the reject function as the code always use the loop value above but if there is, the function name should shadow the loop variable and if there is none, maybe the variable should just be removed in all resources (probes, podmonitors ...).

What do you think?

[simonpasquier]

it might a side effect of #6384?

[QuentinBisson]

I'm trying to have this can be build typo fixed in kubernetes upstream but let's see kubernetes/kubernetes#123844 :p

[QuentinBisson]

@simonpasquier the e2e test failures looks to be a fluke. Is there a way to retest?

[simonpasquier]

Looks good to me
(nit) validateRelabelConfigs should work with nil slices too.

Suggested change

if scrapeClass.Relabelings != nil {

[simonpasquier]

it might a side effect of #6384?

[simonpasquier]

Thanks! This is almost there. Could you also a few more unit tests, esp. making sure that a default scrape class gets applied?

[QuentinBisson]

I hope it's better with the new tests :)

[nicolastakashi]

Amazing progress 👏🏽
It looks great to me.

[simonpasquier]

Great!