ref(opsgenie): verify integration key upon key save rather than upon alert rule save #67081
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Backend
cathteng
commented
Mar 15, 2024
Comment on lines
-36
to
-42
| # This doesn't work if the team name is "." or "..", which Opsgenie allows for some reason | ||
| # despite their API not working with these names. | ||
| def get_team_id(self, team_name: str) -> BaseApiResponseX: | ||
| params = {"identifierType": "name"} | ||
| quoted_name = quote(team_name) | ||
| path = f"/teams/{quoted_name}" | ||
| return self.get(path=path, headers=self._get_auth_headers(), params=params) |
ykamo001
approved these changes
Mar 15, 2024
ykamo001
approved these changes
Mar 15, 2024
Comment on lines
185
to
196
| except ApiError as e: | ||
| logger.info( | ||
| "opsgenie.authorization_error", | ||
| extra={"error": str(e), "status_code": e.code}, | ||
| ) | ||
| if e.code == 429: | ||
| raise ApiRateLimitedError( | ||
| "Too many requests. Please try updating one team/key at a time." | ||
| ) | ||
| elif e.code == 401: | ||
| raise ApiUnauthorized(f"Invalid integration key {integration_key}") | ||
| raise |
Contributor
There was a problem hiding this comment.
do we know how this affects our UX on the FE/product?
Member
Author
There was a problem hiding this comment.
good catch, i improved the UX by slightly changing what we return from the API if we raise an error
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #67081 +/- ##
=======================================
Coverage 84.32% 84.32%
=======================================
Files 5306 5306
Lines 237081 237100 +19
Branches 41001 41008 +7
=======================================
+ Hits 199907 199938 +31
+ Misses 36956 36944 -12
Partials 218 218
|
vartec
approved these changes
Mar 18, 2024
Contributor
vartec
left a comment
vartec
left a comment
There was a problem hiding this comment.
I have some opinions, but nothing blocking
src/sentry/api/endpoints/integrations/organization_integrations/details.py
Show resolved
Hide resolved
cathteng
force-pushed
the
cathy/opsgenie/verify-integration-key
branch
from
b73a53b to
a1d596a
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Opsgenie has a complex rate limiting strategy: https://docs.opsgenie.com/docs/api-rate-limiting 😞
Currently, when someone saves a metric alert, if they have Opsgenie trigger actions, all of them are validated consecutively by POSTing to the authenticate integration API, which we appear to do in order to validate the integration key since we don't do anything with the response. Opsgenie doesn't have an API to verify integration keys, so our approach has been to hit an API to check if it's an authorized request.
Due to to Opsgenie's rate limiting strategy, it is easy for someone to get rate limited for this API because we are calling this POST repeatedly when saving an alert rule. The current response is "Invalid integration key" regardless of the actual status code of the API, which is not helpful.
We should be validating the integration key as it is saved rather than doing so upon alert save, because people might have multiple Opsgenie trigger actions per alert. Thus we can prevent invalid integration keys from being saved in the first place. I also switched the API we try to hit to check the validity of the integration key to a GET rather than a POST to hopefully increase the rate limit.
Also modified parsing the error so the error messages when filling out the form are more informative.
Screen.Recording.2024-03-15.at.14.50.54.mov