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 inconsistent defaults in UTF-8 behavior #3668

Merged

Conversation

grobinson-grafana
Copy link
Contributor

@grobinson-grafana grobinson-grafana commented Jan 12, 2024

This commit fixes inconsistent behavior in Alertmanager when neither the compat package has been initialized or feature flags passed to api.New and silence.New. This can happen when Alertmanager is used as a package in other projects such as Cortex or Mimir. Prior to this fix, Alertmanager would accept UTF-8 alerts but reject UTF-8 configurations.

The fix removes the feature flags from the API and silence package for a number of reasons:

  1. We had accidentally duplicated code when we fixed group_by (Fix UTF-8 not supported in group_by #3619).
  2. It was not simple to fix this issue at compile time. The reason for this is that Options are a struct, and the compiler will no complaint if new options are added but not initialized. We also want vendors of this package to be able to update it without breaking changes.
  3. Removing this code and deferring to the compat package means all code is now in a single place, where as before it was split between the compat, API and silence packages.
  4. When testing this for Mimir, it took me 4-5 attempts to initialize all the feature flags in the correct places. The compiler does not help here because of Fix bugs in aggregator and filters, add initial tests #2. This PR also fixes that issue by a. providing consistent default behavior and b. providing a single place to change that behavior.

This commit fixes inconsistent UTF-8 behavior if the compat package is
not initialized and feature flags are not passed to the API. This can
happen when Alertmanager is used as a package in software such
as Cortex or Mimir.

The inconsistent behavior is that Alertmanager will accept UTF-8 alerts
but reject UTF-8 configurations.

Since feature flags are optional via api.Options, we cannot force them
to be passed to api.New at compile time. Instead, it's better to defer
back to the compat package which is consistent even when not initialized.

Signed-off-by: George Robinson <[email protected]>
Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM

@gotjosh gotjosh merged commit fa6a7e6 into prometheus:main Jan 15, 2024
11 checks passed
grobinson-grafana added a commit to grobinson-grafana/alertmanager that referenced this pull request Jan 19, 2024
This commit removes some code that should have been removed in prometheus#3668.
The FeatureFlags in silence.Options are no longer used but were
still initialized. These had a no-op effect.
grobinson-grafana added a commit to grobinson-grafana/alertmanager that referenced this pull request Jan 19, 2024
This commit removes some code that should have been removed in prometheus#3668.
The FeatureFlags in silence.Options are no longer used but were
still initialized. These had a no-op effect.

Signed-off-by: George Robinson <[email protected]>
gotjosh pushed a commit that referenced this pull request Jan 19, 2024
This commit removes some code that should have been removed in #3668.
The FeatureFlags in silence.Options are no longer used but were
still initialized. These had a no-op effect.

Signed-off-by: George Robinson <[email protected]>
th0th pushed a commit to th0th/alertmanager that referenced this pull request Mar 23, 2024
This commit fixes inconsistent UTF-8 behavior if the compat package is
not initialized and feature flags are not passed to the API. This can
happen when Alertmanager is used as a package in software such
as Cortex or Mimir.

The inconsistent behavior is that Alertmanager will accept UTF-8 alerts
but reject UTF-8 configurations.

Since feature flags are optional via api.Options, we cannot force them
to be passed to api.New at compile time. Instead, it's better to defer
back to the compat package which is consistent even when not initialized.

Signed-off-by: George Robinson <[email protected]>
Signed-off-by: Gokhan Sari <[email protected]>
th0th pushed a commit to th0th/alertmanager that referenced this pull request Mar 23, 2024
This commit removes some code that should have been removed in prometheus#3668.
The FeatureFlags in silence.Options are no longer used but were
still initialized. These had a no-op effect.

Signed-off-by: George Robinson <[email protected]>
Signed-off-by: Gokhan Sari <[email protected]>
@grobinson-grafana grobinson-grafana deleted the grobinson/fix-inconsistent-defaults branch April 16, 2024 14:44
SuperQ added a commit that referenced this pull request Oct 16, 2024
* [CHANGE] Deprecate and remove api/v1/ #2970
* [CHANGE] Remove unused feature flags #3676
* [CHANGE] Newlines in smtp password file are now ignored #3681
* [CHANGE] Change compat metrics to counters #3686
* [CHANGE] Do not register compat metrics in amtool #3713
* [CHANGE] Remove metrics from compat package #3714
* [CHANGE] Mark muted alerts #3793
* [FEATURE] Add metric for inhibit rules #3681
* [FEATURE] Support UTF-8 label matchers #3453, #3507, #3523, #3483, #3567, #3568, #3569, #3571, #3595, #3604, #3619, #3658, #3659, #3662, #3668, 3572
* [FEATURE] Add counter to track alerts dropped outside of time_intervals #3565
* [FEATURE] Add date and tz functions to templates #3812
* [FEATURE] Add limits for silences #3852
* [FEATURE] Add time helpers for templates #3863
* [FEATURE] Add auto GOMAXPROCS #3837
* [FEATURE] Add auto GOMEMLIMIT #3895
* [FEATURE] Add Jira receiver integration #3590
* [ENHANCEMENT] Add the receiver name to notification metrics #3045
* [ENHANCEMENT] Add the route ID to uuid #3372
* [ENHANCEMENT] Add duration to the notify success message #3559
* [ENHANCEMENT] Implement webhook_url_file for discord and msteams #3555
* [ENHANCEMENT] Add debug logs for muted alerts #3558
* [ENHANCEMENT] API: Allow the Silences API to use their own 400 response #3610
* [ENHANCEMENT] Add summary to msteams notification #3616
* [ENHANCEMENT] Add context reasons to notifications failed counter #3631
* [ENHANCEMENT] Add optional native histogram support to latency metrics #3737
* [ENHANCEMENT] Enable setting ThreadId for Telegram notifications #3638
* [ENHANCEMENT] Allow webex roomID from template #3801
* [BUGFIX] Add missing integrations to notify metrics #3480
* [BUGFIX] Add missing ttl in pushhover #3474
* [BUGFIX] Fix scheme required for webhook url in amtool #3409
* [BUGFIX] Remove duplicate integration from metrics #3516
* [BUGFIX] Reflect Discord's max length message limits #3597
* [BUGFIX] Fix nil error in warn logs about incompatible matchers #3683
* [BUGFIX] Fix a small number of inconsistencies in compat package logging #3718
* [BUGFIX] Fix log line in featurecontrol #3719
* [BUGFIX] Fix panic in acceptance tests #3592
* [BUGFIX] Fix flaky test TestClusterJoinAndReconnect/TestTLSConnection #3722
* [BUGFIX] Fix crash on errors when url_file is used #3800
* [BUGFIX] Fix race condition in dispatch.go #3826
* [BUGFIX] Fix race conditions in the memory alerts store #3648
* [BUGFIX] Hide config.SecretURL when the URL is incorrect. #3887
* [BUGFIX] Fix invalid silence causes incomplete updates #3898
* [BUGFIX] Fix leaking of Silences matcherCache entries #3930
* [BUGFIX] Close SMTP submission correctly to handle errors #4006

Signed-off-by: SuperQ <[email protected]>
@SuperQ SuperQ mentioned this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants