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

Alerting: Fix provenance guard checks for Alertmanager configuration to not cause panic when compared nested objects #69009

Merged

Conversation

yuri-tceretian
Copy link
Contributor

What is this feature?
This PR fixes a panic that is caused when the provenance guard compares the settings of the current and submitted integrations that contain nested objects.

Why do we need this feature?
Currently, this makes it impossible to update non-provisioned contact points via regular API if the configuration contains provisioned PagerDuty integration. The request will cause panic

level=error msg="Request error" error="runtime error: comparing uncomparable type map[string]interface {}" stack="/usr/local/go/src/runtime/alg.go:249 (0x40cfd8)\n/drone/src/pkg/services/ngalert/api/api_alertmanager_guards.go:116 

Who is this feature for?
Users who provision PagerDuty integration because it is the only one that accepts nested objects in settings.

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@yuri-tceretian yuri-tceretian requested a review from a team as a code owner May 24, 2023 16:56
@yuri-tceretian yuri-tceretian self-assigned this May 24, 2023
@yuri-tceretian yuri-tceretian added this to the 10.0.x milestone May 24, 2023
@grafanabot
Copy link
Contributor

Hello @yuri-tceretian!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@grafanabot
Copy link
Contributor

Hello @yuri-tceretian!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@yuri-tceretian yuri-tceretian modified the milestones: 10.0.x, 10.1.x May 24, 2023
@yuri-tceretian yuri-tceretian added area/alerting Grafana Alerting area/alerting/notifications Issues when sending alert notifications labels May 24, 2023
Copy link
Member

@JacobsonMT JacobsonMT left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@yuri-tceretian yuri-tceretian merged commit e002604 into main May 25, 2023
11 checks passed
@yuri-tceretian yuri-tceretian deleted the yuri-tceretian/fix-provenance-guard-nested-objects branch May 25, 2023 15:41
grafanabot pushed a commit that referenced this pull request May 25, 2023
…to not cause panic when compared nested objects (#69009)

* fix current settings parsed as new
* replace map comparison with cmp.Diff and log the diff

(cherry picked from commit e002604)
grafanabot pushed a commit that referenced this pull request May 25, 2023
…to not cause panic when compared nested objects (#69009)

* fix current settings parsed as new
* replace map comparison with cmp.Diff and log the diff

(cherry picked from commit e002604)
yuri-tceretian added a commit that referenced this pull request May 25, 2023
…iguration to not cause panic when compared nested objects (#69094)

Alerting: Fix provenance guard checks for Alertmanager configuration to not cause panic when compared nested objects (#69009)

* fix current settings parsed as new
* replace map comparison with cmp.Diff and log the diff

(cherry picked from commit e002604)

Co-authored-by: Yuri Tseretyan <yuriy.tseretyan@grafana.com>
yuri-tceretian added a commit that referenced this pull request May 25, 2023
…guration to not cause panic when compared nested objects (#69092)

Alerting: Fix provenance guard checks for Alertmanager configuration to not cause panic when compared nested objects (#69009)

* fix current settings parsed as new
* replace map comparison with cmp.Diff and log the diff

(cherry picked from commit e002604)

Co-authored-by: Yuri Tseretyan <yuriy.tseretyan@grafana.com>
grobinson-grafana pushed a commit that referenced this pull request May 31, 2023
…to not cause panic when compared nested objects (#69009)

* fix current settings parsed as new
* replace map comparison with cmp.Diff and log the diff
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/alerting/notifications Issues when sending alert notifications area/alerting Grafana Alerting area/backend backport v9.5.x Bot will automatically open backport PR backport v10.0.x type/bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants