Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: alertmanager: validate labels and annotation names only, not values #441

Merged

Conversation

hileef
Copy link
Contributor

@hileef hileef commented Mar 31, 2023

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area config

What this PR does / why we need it:

Only validate Prometheus Label names and Annotation names instead of validating values as well.
Internally, Alertmanager and Prometheus use the same data model to represent both labels and annotations,
and apply the same validation logic.

See official doc here : https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels, which mentions :

Label names may contain ASCII letters, numbers, as well as underscores. They must match the regex [a-zA-Z_][a-zA-Z0-9_]*. Label names beginning with __ are reserved for internal use.

Label values may contain any Unicode characters.

As a practical example, we encountered an issue because we use dashes in our label values,
such that this was rejected : extralabels: "cloud:aws,region:eu-west-1,team:platform"

Special notes for your reviewer:

Hello, we are currently in the process of experimenting with this project for internal usage ;
it looks quite promising, great work 💪 😄

We encountered an issue when trying to use our standardized internal alert routing,
which should be fixed with these changes 🙏

It wasn't obvious to me where to add a test for such a bug fix,
I may have missed something but could not find config validation tests
so I didn't add one for this specific use case, I hope that's okay.

@poiana
Copy link

poiana commented Mar 31, 2023

Welcome @hileef! It looks like this is your first PR to falcosecurity/falcosidekick 🎉

@poiana poiana added the size/M label Mar 31, 2023
@fjogeleit fjogeleit requested a review from Issif March 31, 2023 19:04
@Issif Issif added this to the 2.28.0 milestone Mar 31, 2023
Copy link
Member

@Issif Issif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks and sorry for the delay I was sick for past few days

@poiana poiana added the lgtm label Apr 5, 2023
@poiana
Copy link

poiana commented Apr 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hileef, Issif

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana
Copy link

poiana commented Apr 5, 2023

LGTM label has been added.

Git tree hash: cbfb5439a48a68be7a18f5838a2fd6fb1782fbd2

@poiana poiana added the approved label Apr 5, 2023
@hileef
Copy link
Contributor Author

hileef commented Apr 5, 2023

Thanks and sorry for the delay I was sick for past few days

No problem 🙏 Glad to hear you are doing better @Issif 🙂

I'm not sure how to proceed with this PR though, I do notice that there are "3 workflows awaiting approval" 🤔
Is this PR experiencing something similar to #431 (comment) ?

Is there something I can do to help ? 🙂

@Issif
Copy link
Member

Issif commented Apr 5, 2023

Thanks.

The merge should be automatic after my approval, let me check

@poiana poiana merged commit 8805a50 into falcosecurity:master Apr 5, 2023
@hileef hileef deleted the fix/alertmanager/extra-labels-syntax branch April 5, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants