Alerting: Add endpoint to revert to a previous alertmanager configuration #65751
Conversation
…tion This endpoint is meant to be used in conjunction with /api/alertmanager/grafana/config/history to revert to a previously applied alertmanager configuration. This is done by ID instead of raw config string in order to avoid secure field complications.
JacobsonMT
added
area/alerting
There was a problem hiding this comment.
I just tested the following flow:
- Breaking the current AM configuration.
- Trying to reset state from a broken historic configuration (it fails, expected).
- Resetting state using a valid historic configuration (success).
Everything works as expected!
I added a few comments about typos, tests, naming, and the API spec.
| @@ -408,6 +414,16 @@ func (api *API) RegisterAlertmanagerApiEndpoints(srv AlertmanagerApi, m *metrics | |||
| m, | |||
| ), | |||
| ) | |||
| group.Post( | |||
| toMacaronPath("/api/alertmanager/grafana/config/history/{id}/_activate"), | |||
| api.authorize(http.MethodPost, "/api/alertmanager/grafana/config/history/{id}/_activate"), | |||
There was a problem hiding this comment.
Do we have other endpoints prefixed by underscores? I assume it's because "activate" refers to an action rather than a resource, but what's the motivation behind this?
There was a problem hiding this comment.
I assume it's because "activate" refers to an action rather than a resource
Correct. @yuri-tceretian and I briefly discussed what format the endpoint should be in and came to this compromise. It has the benefits of mirroring the related endpoint /api/alertmanager/grafana/config/history and referencing the historical config by resource id. The drawback being the, somewhat strange, underscore action.
The other options we discussed were:
- POST
/api/alertmanager/grafana/config/api/v1/alertswith{id:<int>}. The benefits of this being no extra endpoint needed. The drawbacks are that it conflates this new use-case with an existing core endpoint. - POST
/api/alertmanager/grafana/config/1with body{ active : true }or something similar. This doesn't have_activatebut poses the question of what does this endpoint do ifactive=false.
I'm open to discussing alternatives if there's a compelling reason.
| // Responses: | ||
| // 202: Ack | ||
| // 400: ValidationError | ||
| // 404: NotFound |
There was a problem hiding this comment.
We're missing status codes 409 and 500.
There was a problem hiding this comment.
409 would be from ErrAlertmanagerNotReady which actually shouldn't ever be thrown by this endpoint. I know I added a specific condition in the API to catch this, but it's mostly defensive programming and to mirror the errors of RoutePostAlertingConfig (although I can't see where that endpoint can throw this error either)
500 is a catch-all "unknown" error, it doesn't feel right to document it in the api specs to me. The other endpoints (mostly) leave it out as well.
|
This pull request was removed from the 9.5.0 milestone because 9.5.0 is currently being released. |
| @@ -166,6 +166,8 @@ func (api *API) authorize(method, path string) web.Handler { | |||
| case http.MethodPost + "/api/alertmanager/grafana/config/api/v1/alerts": | |||
| // additional authorization is done in the request handler | |||
| eval = ac.EvalAny(ac.EvalPermission(ac.ActionAlertingNotificationsWrite)) | |||
| case http.MethodPost + "/api/alertmanager/grafana/config/history/{id}/_activate": | |||
There was a problem hiding this comment.
How come this is _activate instead of something like activate?
There was a problem hiding this comment.
I had the same question and here's the answer. I'm not very convinced by that underscore.
There was a problem hiding this comment.
Context is here: #65751 (comment)
|
Tested and confirmed it works! 🚀 |
What is this feature?
This endpoint is meant to be used in conjunction with
/api/alertmanager/grafana/config/historyto revert to a previously applied alertmanager configuration. This is done by ID instead of raw config string in order to avoid secure field complications.Why do we need this feature?
This endpoint will be used by the frontend to revert to a previous alertmanager configuration in case the current one is broken.
Which issue(s) does this PR fix?:
Partial #54505
Special notes for your reviewer: