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

Exclude eventv1.MetaTokenKey from event metadata #686

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Dec 13, 2023

eventv1.MetaTokenKey is required to be considered in rate limiting but it is only for internal use by flux components and should not be sent to alert providers. Remove eventv1.MetaTokenKey from the metadata of event before processing it for matching alerts.
Fixes #650

hc-alert-exclude-token

NOTE: Initially, I added it in cleanupMetadata() and found out during manual testing that it removes the metadata too early which excludes it from rate limiter. Also tried adding it in enhanceEventWithAlertMetadata() but realized that this is too late. The same metadata will be removed for every single alert the event matches with. Removing the internal metadata right at the beginning of the event handler seems to be most appropriate.

@darkowlzz darkowlzz added the area/alerting Alerting related issues and PRs label Dec 13, 2023
eventv1.MetaTokenKey is required to be considered in rate limiting but
it is only for internal use by flux components and should not be sent to
the alert provider. Remove eventv1.MetaTokenKey from the metadata of
event before processing the event for various matching alerts.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz darkowlzz changed the title Exclude eventv1.MetaTokenKey from alert metadata Exclude eventv1.MetaTokenKey from event metadata Dec 13, 2023
@stefanprodan stefanprodan added the backport:release/v1.2.x To be backported to release/v1.2.x label Dec 13, 2023
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @darkowlzz 🏅

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

This is in heavy need of more functional tests, but for now it looks good to me. Thanks @darkowlzz 🙇

@darkowlzz darkowlzz merged commit a817cf9 into main Dec 14, 2023
7 checks passed
@darkowlzz darkowlzz deleted the exclude-token branch December 14, 2023 08:50
@fluxcdbot
Copy link
Member

Successfully created backport PR for release/v1.2.x:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Alerting related issues and PRs backport:release/v1.2.x To be backported to release/v1.2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The token field should be excluded from the alert payload
4 participants