Add validation for pod/service monitors for TargetAllocator and skip invalid ones #2328

rashmichandrashekar · 2023-11-09T02:17:22Z

Description:
Fixing a bug - When a podmonitor/svcmonitor CRD (with Targetallocator enabled)has an erroneous field for regex in the relabeling/metric relabeling fields, the crd creation succeeds since it can be a valid string but not a valid regex( ex- .*( ). This causes any subsequent valid CRDs being created from getting picked up.

Link to tracking Issue: #2309

Testing: Tested this out with otel operator setup

jaronoff97 · 2023-11-09T20:53:12Z

cmd/otel-allocator/watcher/promOperator.go

+// based on the service monitor and pod metrics endpoints specs.
+// This code borrows from
+// https://github.com/prometheus-operator/prometheus-operator/blob/ba536405154d18f3a6f312818283d671182af6f3/pkg/prometheus/resource_selector.go#L237
+func validateRelabelConfig(rc monitoringv1.RelabelConfig) error {


Would it be useful for us to reach out to the prometheus operator folks to see if they would expose this method?

Thanks @jaronoff97. I have reached out to them, waiting for a response,(prometheus-operator/prometheus-operator#6057 (comment)) based on that I can add the tests.

Also created an issue here - prometheus-operator/prometheus-operator#6076

@jaronoff97 - There is a question about breaking changes. I have answered it from what I understand and what I think the way forward would be. Could you please respond to provide your perspective and correct me if i am wrong?
prometheus-operator/prometheus-operator#6076 (comment)

yep, i responded there :) thanks for making the issue. Let's wait to hear from them before merging this – i would love to just use their method rather than having to write it ourselves.

jaronoff97

Could you add some tests in to the promOperator_test.go file to ensure this works how we expect?

jaronoff97 · 2023-11-29T18:13:25Z

@rashmichandrashekar could you re-implement this PR to use the interfaces recommended by the prometheus operator?

rashmichandrashekar · 2023-11-29T18:22:49Z

@rashmichandrashekar could you re-implement this PR to use the interfaces recommended by the prometheus operator?

@jaronoff97 - For sure, I have the changes all ready. Just cleaning some things up and I will create a PR today. Thanks for helping with this!

rashmichandrashekar · 2023-12-01T20:16:15Z

@rashmichandrashekar could you re-implement this PR to use the interfaces recommended by the prometheus operator?

@jaronoff97 - For sure, I have the changes all ready. Just cleaning some things up and I will create a PR today. Thanks for helping with this!

@jaronoff97 - I have reimplemented this and added relevant tests. Please take a look and lmk if you have any feedback. Also a couple tests seem to be timing out, please requeue them if possible. Thanks!

jaronoff97 · 2023-12-01T20:16:58Z

Thank you! I'll try to take look today or monday.

…former

cmd/otel-allocator/watcher/promOperator.go

jaronoff97

A few nits/ questions. Does this change how our existing label selector works for the target allocator? I'm wondering if we should also write a new e2e or two to verify this works as expected. cc @swiatekm-sumo

jaronoff97 · 2023-12-04T21:10:49Z

cmd/otel-allocator/config/config.go

+	ServiceMonitorNamespaceSelector map[string]string  `yaml:"service_monitor_namespace_selector,omitempty"`
+	PodMonitorNamespaceSelector     map[string]string  `yaml:"pod_monitor_namespace_selector,omitempty"`


we should be defaulting these to empty so that we can keep the current behavior (link)

Thanks @jaronoff97. Since the common prometheus fields have the ServiceMonitorNamespaceSelector and PodMonitorNamespaceSelector already initialized, even if the targetallocator config is not setting these selectors (or have nil selectors), the default behavior will be to match all namespaces which is the current behavior. But I can add defaults, in case you think otherwise. Please let me know.
The following tests also depict the behavior with no selectors in the config vs with selectors.
https://github.com/open-telemetry/opentelemetry-operator/blob/883b583c4c93038d6fddc55b192659c4aa90f0fe/cmd/otel-allocator/watcher/promOperator_test.go#L91C4-L91C34
https://github.com/open-telemetry/opentelemetry-operator/blob/883b583c4c93038d6fddc55b192659c4aa90f0fe/cmd/otel-allocator/watcher/promOperator_test.go#L505C3-L509C6

jaronoff97 · 2023-12-04T21:14:45Z

cmd/otel-allocator/watcher/promOperator.go

+				"ServiceMonitorNamespaceSelector": w.serviceMonitorNamespaceSelector,
+			} {
+
+				sync, err := k8sutil.LabelSelectionHasChanged(old.Labels, cur.Labels, selector)


can you confirm my understanding here: this event handler waits for any namespace changes in which case we check if one of our namespace selectors has changed as well in which case we send an event to the notifyEvents channel?

This event handler watches for any namespace changes and then calls this method - LabelSelectionHasChanged to basically make sure that the TA's selector yields the same results with old and new labels, if it does then an event is not sent and the new labels lead to a change in behavior then an event is sent.
For ex- If the cfg.podMonitorNamespaceSelector was set to match namespaces with labels "testns" : "testns" and the initial set of namespace labels on for example - namespace1 were "testns" : "testns" and later if the labels were removed, then an event would be sent. But if later, the labels on the namespace were updated to
{
"testns" : "testns",
"testns1": "testns1",
}
it wouldnt send an event.

got it, thanks for the great explanation! 🙇

rashmichandrashekar · 2023-12-04T22:57:20Z

A few nits/ questions. Does this change how our existing label selector works for the target allocator? I'm wondering if we should also write a new e2e or two to verify this works as expected. cc @swiatekm-sumo

Sure @jaronoff97. This doesnt change the existing behavior of the selectors. This just adds a couple other selectors just like in prometheus-operator. I did add unit tests for all these selectors to make sure that the selectors result in the right configuration and select the right pod and svc monitors as well as selectors on namespaces result in picking up pod and svc monitors in the matching namespaces. please lmk if e2e are also necessary.

swiatekm

Thanks for submitting this change, I really like the watcher cleanup. I do have a concern about this being a breaking change in its current state though - see my comment on the namespace informer.

cmd/otel-allocator/watcher/promOperator.go

jaronoff97 · 2023-12-07T18:13:34Z

@rashmichandrashekar we just talked about this at the operator SIG and want to attempt to get this in prior to our next release. In order to do so, I opened up #2426 to see if we can ensure that we are not going to break users by checking they have the new permission for the TA to function. We're going to pause merging this until we have an answer there.

rashmichandrashekar · 2023-12-07T19:53:46Z

[like] Rashmi Chandrashekar reacted to your message:

…

________________________________ From: Jacob Aronoff ***@***.***> Sent: Thursday, December 7, 2023 7:52:29 PM To: open-telemetry/opentelemetry-operator ***@***.***> Cc: Rashmi Chandrashekar ***@***.***>; Mention ***@***.***> Subject: Re: [open-telemetry/opentelemetry-operator] Add validation for pod/service monitors for TargetAllocator and skip invalid ones (PR #2328) nope, this is a different SIG :) you can find in slack at #otel-operator and you can see the other sigs here<https://github.com/open-telemetry/community/?tab=readme-ov-file#implementation-sigs>. — Reply to this email directly, view it on GitHub<#2328 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AIZHOKKZ4RNAJSHL4GXAEADYIIM73AVCNFSM6AAAAAA7D2OTK6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBWGAYDMNJQHE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

…drashekar/opentelemetry-operator into rashmi/add-validation

jaronoff97 · 2024-01-05T16:42:16Z

@rashmichandrashekar i've merged in my PR so now you should be able to fix the conflicts and update the necessary TA permissions :)

rashmichandrashekar · 2024-01-05T18:20:53Z

@rashmichandrashekar i've merged in my PR so now you should be able to fix the conflicts and update the necessary TA permissions :)

Thanks so much @jaronoff97 for unblocking this PR. I will get the latest and test it out and update this PR.

rashmichandrashekar · 2024-01-13T01:56:52Z

@rashmichandrashekar i've merged in my PR so now you should be able to fix the conflicts and update the necessary TA permissions :)

Thanks so much @jaronoff97 for unblocking this PR. I will get the latest and test it out and update this PR.

@jaronoff97 - I have updated the PR by fixing conflicts and adding permissions check for namespaces in the webhook. Please take a look. Thanks!

swiatekm

This looks good to me overall, modulo some relatively minor problems.

.chloggen/target-allocator-use-recommended-prometheus-operator-interfaces.yaml

cmd/otel-allocator/README.md

cmd/otel-allocator/watcher/promOperator.go

cmd/otel-allocator/watcher/promOperator_test.go

swiatekm

LGTM, but I'd like to use LabelSelectors directly in configuration instead of repeating the mistake we made with existing ones. @jaronoff97 can you also have a final look?

cmd/otel-allocator/config/config.go

swiatekm

👍 thanks a lot for doing this, and your patience in accepting our feedback.

jaronoff97

absolutely fantastic work here. Thank you so much for your patience and contribution. 🎉 the added tests are also great to see.

…invalid ones (open-telemetry#2328) * fix for adding validation * removing unused references * adding tests * adding some changes * cleaning up * adding change log * running goimports and adding return value check code for namespace informer * fixing lint error * fixing tests and comment * adding permissions for e2e tests * adding cluster roles instead of roles * updaintg readme * fixing comments * adding contant to same block * fixing lint errors * running go import * adding namespaces since that is required for informer * adding extected warnings * addressing comments * adding test for namespace label update * fixing goimports * making namespaceselectores as labelselectors

fix for adding validation

1b7e32b

rashmichandrashekar requested a review from a team November 9, 2023 02:17

removing unused references

a55e688

jaronoff97 reviewed Nov 9, 2023

View reviewed changes

adding tests

401ad2c

rashmichandrashekar added 4 commits November 29, 2023 11:14

adding some changes

b582198

syncing with latest main

1a90fae

cleaning up

2c276dc

adding change log

e97f969

rashmichandrashekar requested a review from a team November 29, 2023 19:30

rashmichandrashekar added 3 commits December 4, 2023 12:07

Merge branch 'main' into rashmi/add-validation

26843ed

running goimports and adding return value check code for namespace in…

533dff8

…former

fixing lint error

537a2bb

jaronoff97 reviewed Dec 4, 2023

View reviewed changes

cmd/otel-allocator/watcher/promOperator.go Show resolved Hide resolved

jaronoff97 reviewed Dec 4, 2023

View reviewed changes

rashmichandrashekar added 2 commits December 4, 2023 13:20

fixing tests and comment

90bcb75

adding permissions for e2e tests

dfcc746

rashmichandrashekar added 2 commits December 4, 2023 15:50

adding cluster roles instead of roles

5406145

updaintg readme

883b583

swiatekm requested changes Dec 5, 2023

View reviewed changes

cmd/otel-allocator/watcher/promOperator.go Outdated Show resolved Hide resolved

rashmichandrashekar added 2 commits December 5, 2023 12:38

fixing comments

7885db1

Merge branch 'main' into rashmi/add-validation

83abf55

jaronoff97 mentioned this pull request Dec 7, 2023

Investigate if we can mandate RBAC rules for the TA in webhooks #2426

Closed

jaronoff97 mentioned this pull request Dec 20, 2023

Add new operator capability to check if it has access to do an operation #2467

Merged

rashmichandrashekar added 2 commits January 2, 2024 13:39

adding contant to same block

c9f3aa5

Merge branch 'rashmi/add-validation' of https://github.com/rashmichan…

436b888

…drashekar/opentelemetry-operator into rashmi/add-validation

matej-g mentioned this pull request Jan 10, 2024

Target Allocator - ServiceMonitor scheme #1669

Closed

rashmichandrashekar added 4 commits January 12, 2024 15:33

merging with upstream main

319e72f

fixing lint errors

4fc1403

running go import

a943cdb

adding namespaces since that is required for informer

bf33615

adding extected warnings

bc6dc56

swiatekm reviewed Jan 14, 2024

View reviewed changes

rashmichandrashekar added 2 commits January 17, 2024 12:56

Merge branch 'main' into rashmi/add-validation

b627b24

addressing comments

b742f0a

swiatekm reviewed Jan 18, 2024

View reviewed changes

cmd/otel-allocator/watcher/promOperator.go Outdated Show resolved Hide resolved

cmd/otel-allocator/watcher/promOperator_test.go Outdated Show resolved Hide resolved

rashmichandrashekar added 3 commits January 18, 2024 15:36

adding test for namespace label update

d062dbd

Merge branch 'main' into rashmi/add-validation

859ed13

fixing goimports

da40b95

swiatekm reviewed Jan 19, 2024

View reviewed changes

cmd/otel-allocator/config/config.go Outdated Show resolved Hide resolved

rashmichandrashekar force-pushed the rashmi/add-validation branch from 1b06342 to da40b95 Compare January 20, 2024 02:01

making namespaceselectores as labelselectors

f0bc9d2

swiatekm approved these changes Jan 20, 2024

View reviewed changes

swiatekm requested a review from jaronoff97 January 20, 2024 10:36

jaronoff97 approved these changes Jan 22, 2024

View reviewed changes

jaronoff97 merged commit 3ee6aad into open-telemetry:main Jan 22, 2024
27 checks passed

jaronoff97 mentioned this pull request Jan 22, 2024

Ignore invalid CRDs and pick up valid CRDs with TargetAllocator #2309

Closed

swiatekm mentioned this pull request Jan 25, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for pod/service monitors for TargetAllocator and skip invalid ones #2328

Add validation for pod/service monitors for TargetAllocator and skip invalid ones #2328

rashmichandrashekar commented Nov 9, 2023

jaronoff97 Nov 9, 2023

rashmichandrashekar Nov 10, 2023

rashmichandrashekar Nov 10, 2023

rashmichandrashekar Nov 13, 2023

jaronoff97 Nov 14, 2023

jaronoff97 left a comment

jaronoff97 commented Nov 29, 2023

rashmichandrashekar commented Nov 29, 2023

rashmichandrashekar commented Dec 1, 2023

jaronoff97 commented Dec 1, 2023

jaronoff97 left a comment

jaronoff97 Dec 4, 2023

rashmichandrashekar Dec 4, 2023 •

edited

Loading

jaronoff97 Dec 4, 2023

rashmichandrashekar Dec 4, 2023

jaronoff97 Dec 5, 2023

rashmichandrashekar commented Dec 4, 2023

swiatekm left a comment

jaronoff97 commented Dec 7, 2023

rashmichandrashekar commented Dec 7, 2023 via email

jaronoff97 commented Jan 5, 2024

rashmichandrashekar commented Jan 5, 2024

rashmichandrashekar commented Jan 13, 2024

swiatekm left a comment

swiatekm left a comment •

edited

Loading

swiatekm left a comment

jaronoff97 left a comment

		ServiceMonitorNamespaceSelector map[string]string `yaml:"service_monitor_namespace_selector,omitempty"`
		PodMonitorNamespaceSelector map[string]string `yaml:"pod_monitor_namespace_selector,omitempty"`

Add validation for pod/service monitors for TargetAllocator and skip invalid ones #2328

Add validation for pod/service monitors for TargetAllocator and skip invalid ones #2328

Conversation

rashmichandrashekar commented Nov 9, 2023

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

jaronoff97 commented Nov 29, 2023

rashmichandrashekar commented Nov 29, 2023

rashmichandrashekar commented Dec 1, 2023

jaronoff97 commented Dec 1, 2023

jaronoff97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rashmichandrashekar Dec 4, 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

rashmichandrashekar commented Dec 4, 2023

swiatekm left a comment

Choose a reason for hiding this comment

jaronoff97 commented Dec 7, 2023

rashmichandrashekar commented Dec 7, 2023 via email

jaronoff97 commented Jan 5, 2024

rashmichandrashekar commented Jan 5, 2024

rashmichandrashekar commented Jan 13, 2024

swiatekm left a comment

Choose a reason for hiding this comment

swiatekm left a comment • edited Loading

Choose a reason for hiding this comment

swiatekm left a comment

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

rashmichandrashekar Dec 4, 2023 •

edited

Loading

swiatekm left a comment •

edited

Loading