[target-allocator] Populate store assets (authorization information) for Prometheus CR watcher #1710

matej-g · 2023-05-04T10:09:53Z

Resolves #1669.

This change adds logic to populate the so-called store assets - this includes information from service and pod monitors that relates to authorization information (e.g. basic auth, bearer token, TLS) - in order to propagate these information to scrape configs. Currently, this logic is not implement, resulting in scrape configurations missing proper credentials.

This PR also adds tests for the LoadConfig method for the Prometheus CR watcher.

Add context parameter to to LoadConfig method in perparation to use K8s client in the method. Signed-off-by: Matej Gera <matej.gera@coralogix.com>

Add K8s client and logger, in preparation for obtaining service / pod monitor sotore assets. Signed-off-by: Matej Gera <matej.gera@coralogix.com>

…tors Signed-off-by: Matej Gera <matej.gera@coralogix.com>

Signed-off-by: Matej Gera <matej.gera@coralogix.com>

jaronoff97

Just a few questions... also, were you able to test this in a real cluster to confirm this works as expected?

cmd/otel-allocator/watcher/promOperator.go

jaronoff97 · 2023-05-04T13:56:27Z

cmd/otel-allocator/watcher/promOperator.go

+// based on the service monitor and endpoints specs.
+// This code borrows from
+// https://github.com/prometheus-operator/prometheus-operator/blob/06b5c4189f3f72737766d86103d049115c3aff48/pkg/prometheus/resource_selector.go#L73.
+func (w *PrometheusCRWatcher) addStoreAssetsForServiceMonitor(


It looks like the method you linked is a public method, are we unable to use it?

ah maybe because we don't want to make a whole resource selector. Interested in your perspective here :)

I was considering use those public methods, but eventually decided to borrow some of the code, because 1) as you said we would need to build the whole resource selector; 2) the methods for monitors seem to do extra stuff (whole selection and validation of monitors), whereas here we wanted to just populate the store.

However, now that you point this out, and as I noticed these methods were recently exported, it might be a good opportunity to replace this whole part of the watcher logic directly with the resource selector methods. We could get that extra validation etc. for free, but perhaps first I'd ensure those methods will result in correct selection of monitors for our purposes.

What do you think about doing this as a follow up work?

that's a great idea for some followup – i love anything that allows us to delete code :)

Let's do it 👍 I'll open an issue for this to ensure it won't go unaddressed.

matej-g · 2023-05-04T14:19:25Z

@jaronoff97 forgot to add in the description - in addition to adding unit tests, I also tested this locally on a test cluster, with expected results, see resulting scrape config for an example service monitor with basic auth (checked in browser):

jaronoff97 · 2023-05-04T14:23:20Z

@matej-g awesome! how does this work with secret mounting? does the collector need to mount a secret volume or does the TA need to? Thinking we may want to update docs to reflect this.

Signed-off-by: Matej Gera <matejgera@gmail.com>

jaronoff97

This LGTM. I want another review from @open-telemetry/operator-ta-maintainers before it's g2g.

jaronoff97 · 2023-06-12T14:20:45Z

cmd/otel-allocator/watcher/promOperator.go

 	}
 	return labels.SelectorFromSet(s)
 }

+// getInformers returns a map of informers for the given resources.
+func getInformers(factory informers.FactoriesForNamespaces) (map[string]*informers.ForResource, error) {


thought: we should open an issue for adding in the new scrape config CRD and maybe the probe CRD too.

Agreed 👍 I'll open an issue for this.

jaronoff97 · 2023-06-12T14:23:04Z

cmd/otel-allocator/watcher/promOperator.go

+// based on the service monitor and endpoints specs.
+// This code borrows from
+// https://github.com/prometheus-operator/prometheus-operator/blob/06b5c4189f3f72737766d86103d049115c3aff48/pkg/prometheus/resource_selector.go#L73.
+func (w *PrometheusCRWatcher) addStoreAssetsForServiceMonitor(


that's a great idea for some followup – i love anything that allows us to delete code :)

Signed-off-by: Matej Gera <matejgera@gmail.com>

jaronoff97 · 2023-06-20T14:51:49Z

@matej-g looks like a bad git merge 😅

matej-g · 2023-06-21T08:00:56Z

Yup, forgot to save 🤦, should be good now though. Seems like there's an unrelated failure in the test (https://github.com/open-telemetry/opentelemetry-operator/actions/runs/5324102167/jobs/9642911432?pr=1710#step:5:1268)

pavolloffay · 2023-06-22T14:25:23Z

@matej-g could you please rebase?

matej-g · 2023-06-28T13:20:08Z

cc @open-telemetry/operator-ta-maintainers this should finally be good to merge, thank you 🙇

…for Prometheus CR watcher (open-telemetry#1710) * Adjust Watcher interface Add context parameter to to LoadConfig method in perparation to use K8s client in the method. Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Enhance the Prometheus CR watcher struct Add K8s client and logger, in preparation for obtaining service / pod monitor sotore assets. Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Adds methods and logic to obtain store assets from service / pod monitors Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add unit tests for Promtehus CR watcher Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add changelog Signed-off-by: Matej Gera <matej.gera@coralogix.com> * Add disclaimer about secrets in the readme Signed-off-by: Matej Gera <matejgera@gmail.com> * Fix store param and tests after branch update Signed-off-by: Matej Gera <matejgera@gmail.com> * Fix botched merge Signed-off-by: Matej Gera <matejgera@gmail.com> --------- Signed-off-by: Matej Gera <matej.gera@coralogix.com> Signed-off-by: Matej Gera <matejgera@gmail.com> Co-authored-by: Jacob Aronoff <jaronoff97@users.noreply.github.com> Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Co-authored-by: Pavol Loffay <p.loffay@gmail.com>

matej-g added 5 commits May 4, 2023 11:30

Adjust Watcher interface

84577bd

Add context parameter to to LoadConfig method in perparation to use K8s client in the method. Signed-off-by: Matej Gera <matej.gera@coralogix.com>

Enhance the Prometheus CR watcher struct

cb0d5a6

Add K8s client and logger, in preparation for obtaining service / pod monitor sotore assets. Signed-off-by: Matej Gera <matej.gera@coralogix.com>

Adds methods and logic to obtain store assets from service / pod moni…

bb4bd44

…tors Signed-off-by: Matej Gera <matej.gera@coralogix.com>

Add unit tests for Promtehus CR watcher

69af4e0

Signed-off-by: Matej Gera <matej.gera@coralogix.com>

Add changelog

d17ed35

Signed-off-by: Matej Gera <matej.gera@coralogix.com>

matej-g force-pushed the prometheus-cr-auth branch from 4dc5200 to d17ed35 Compare May 4, 2023 13:32

matej-g marked this pull request as ready for review May 4, 2023 13:45

matej-g requested review from a team May 4, 2023 13:45

matej-g mentioned this pull request May 4, 2023

Target Allocator - ServiceMonitor scheme #1669

Closed

jaronoff97 reviewed May 4, 2023

View reviewed changes

matej-g mentioned this pull request Jun 12, 2023

[target allocator] Re-add default global scrape interval in Promethues CR watcher configuration. #1822

Merged

matej-g added 3 commits June 12, 2023 11:13

Merge remote-tracking branch 'origin/main' into prometheus-cr-auth

85a2d42

Add disclaimer about secrets in the readme

d66f497

Signed-off-by: Matej Gera <matejgera@gmail.com>

Fix store param and tests after branch update

80aa9a5

Signed-off-by: Matej Gera <matejgera@gmail.com>

jaronoff97 approved these changes Jun 12, 2023

View reviewed changes

This was referenced Jun 15, 2023

[target allocator] Support the Probe and ScrapeConfig Prometheus Operator CRDs #1842

Closed

[target allocator] Refactor Prometheus CR watcher methods for loading service / pod monitors #1843

Closed

Merge branch 'main' into prometheus-cr-auth

940c46d

Aneurysm9 approved these changes Jun 15, 2023

View reviewed changes

matej-g added 2 commits June 20, 2023 16:35

Merge remote-tracking branch 'origin/main' into prometheus-cr-auth

e8f59b0

Fix botched merge

5266457

Signed-off-by: Matej Gera <matejgera@gmail.com>

Merge branch 'main' into prometheus-cr-auth

cf91b15

TylerHelmuth added the ready-to-merge Code review completed; ready to merge by maintainers label Jun 22, 2023

Merge branch 'main' into prometheus-cr-auth

5edae81

pavolloffay and others added 2 commits June 26, 2023 09:48

Merge branch 'main' into prometheus-cr-auth

d4ce3f2

Merge branch 'main' into prometheus-cr-auth

c2ce16b

jaronoff97 merged commit 0bce4bb into open-telemetry:main Jun 28, 2023

jaronoff97 mentioned this pull request Jun 28, 2023

Prepare Release 0.80.0 #1871

Merged

jouve mentioned this pull request Feb 28, 2024

[target allocator] Generate secret containing require TLS creds for scraping. #1844

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[target-allocator] Populate store assets (authorization information) for Prometheus CR watcher #1710

[target-allocator] Populate store assets (authorization information) for Prometheus CR watcher #1710

matej-g commented May 4, 2023

jaronoff97 left a comment

jaronoff97 May 4, 2023

jaronoff97 May 4, 2023

matej-g May 4, 2023

jaronoff97 Jun 12, 2023

matej-g Jun 15, 2023

matej-g commented May 4, 2023

jaronoff97 commented May 4, 2023 •

edited

Loading

jaronoff97 left a comment

jaronoff97 Jun 12, 2023

matej-g Jun 15, 2023

jaronoff97 Jun 12, 2023

jaronoff97 commented Jun 20, 2023

matej-g commented Jun 21, 2023

pavolloffay commented Jun 22, 2023

matej-g commented Jun 28, 2023

[target-allocator] Populate store assets (authorization information) for Prometheus CR watcher #1710

[target-allocator] Populate store assets (authorization information) for Prometheus CR watcher #1710

Conversation

matej-g commented May 4, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g commented May 4, 2023

jaronoff97 commented May 4, 2023 • edited Loading

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

Choose a reason for hiding this comment

jaronoff97 commented Jun 20, 2023

matej-g commented Jun 21, 2023

pavolloffay commented Jun 22, 2023

matej-g commented Jun 28, 2023

jaronoff97 commented May 4, 2023 •

edited

Loading