CNV-80608: management: add update alert rule APIs #1047
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,223 @@ | ||
| package managementrouter | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "errors" | ||
| "io" | ||
| "net/http" | ||
| "strings" | ||
|
|
||
| monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" | ||
|
|
||
| "github.com/openshift/monitoring-plugin/pkg/management" | ||
| ) | ||
|
|
||
| func (hr *httpRouter) BulkUpdateAlertRules(w http.ResponseWriter, req *http.Request) { | ||
| req.Body = http.MaxBytesReader(w, req.Body, maxRequestBodyBytes) | ||
|
|
||
| body, err := io.ReadAll(req.Body) | ||
| if err != nil { | ||
| writeError(w, http.StatusRequestEntityTooLarge, "request body too large") | ||
| return | ||
| } | ||
|
|
||
| // BulkUpdateAlertRulesRequest.Classification is typed as | ||
| // *AlertRuleClassificationPatch (via x-go-type in the spec), so the | ||
| // three-state omitted/null/string semantics are preserved on decode. | ||
| var payload BulkUpdateAlertRulesRequest | ||
| if err := json.Unmarshal(body, &payload); err != nil { | ||
| writeError(w, http.StatusBadRequest, "invalid request body: "+err.Error()) | ||
| return | ||
| } | ||
|
|
||
| if len(payload.RuleIds) == 0 { | ||
| writeError(w, http.StatusBadRequest, "ruleIds is required") | ||
| return | ||
| } | ||
| if len(payload.RuleIds) > maxBulkUpdateRuleIds { | ||
| writeError(w, http.StatusBadRequest, "ruleIds exceeds maximum of 100") | ||
| return | ||
| } | ||
|
|
||
| if payload.AlertingRuleEnabled == nil && payload.Labels == nil && payload.Classification == nil { | ||
| writeError(w, http.StatusBadRequest, "one of alertingRuleEnabled (toggle drop/restore) or labels (set/unset) or classification is required") | ||
| return | ||
| } | ||
|
|
||
| var haveToggle bool | ||
| var enabled bool | ||
| if payload.AlertingRuleEnabled != nil { | ||
| enabled = *payload.AlertingRuleEnabled | ||
| haveToggle = true | ||
| } | ||
|
|
||
| results := make([]UpdateAlertRuleResult, 0, len(payload.RuleIds)) | ||
|
|
||
| for _, rawId := range payload.RuleIds { | ||
| id := strings.TrimSpace(rawId) | ||
| if id == "" { | ||
| msg := "ruleId is empty or whitespace-only" | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: rawId, | ||
|
Contributor
There was a problem hiding this comment. Won't this just be a whitespace only string? |
||
| StatusCode: int32(http.StatusBadRequest), | ||
| Message: &msg, | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| notAllowedEnabled := false | ||
| if haveToggle { | ||
| var err error | ||
| if !enabled { | ||
| err = hr.managementClient.DropPlatformAlertRule(req.Context(), id) | ||
| } else { | ||
| err = hr.managementClient.RestorePlatformAlertRule(req.Context(), id) | ||
| } | ||
| if err != nil { | ||
| var na *management.NotAllowedError | ||
| if errors.As(err, &na) { | ||
| notAllowedEnabled = true | ||
| } else { | ||
| status, message := parseError(err) | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(status), | ||
| Message: &message, | ||
| }) | ||
| continue | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if payload.Classification != nil { | ||
| cl := payload.Classification | ||
| update := management.UpdateRuleClassificationRequest{RuleId: id} | ||
| if cl.ComponentSet { | ||
| update.Component = cl.Component | ||
| update.ComponentSet = true | ||
| } | ||
| if cl.LayerSet { | ||
| update.Layer = cl.Layer | ||
| update.LayerSet = true | ||
| } | ||
| if cl.ComponentFromSet { | ||
| update.ComponentFrom = cl.ComponentFrom | ||
| update.ComponentFromSet = true | ||
| } | ||
| if cl.LayerFromSet { | ||
| update.LayerFrom = cl.LayerFrom | ||
| update.LayerFromSet = true | ||
| } | ||
|
|
||
| if update.ComponentSet || update.LayerSet || update.ComponentFromSet || update.LayerFromSet { | ||
| if err := hr.managementClient.UpdateAlertRuleClassification(req.Context(), update); err != nil { | ||
|
Contributor
There was a problem hiding this comment. I think looking more at the code it would be best to have the label update code and the drop/restore code also flow through a single function similar to |
||
| status, message := parseError(err) | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(status), | ||
| Message: &message, | ||
| }) | ||
| continue | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+68
to
+123
There was a problem hiding this comment. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Make each rule update produce one coherent outcome. A toggle Also applies to: 125-215 🧰 Tools🪛 ast-grep (0.44.1)[warning] 83-83: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) [warning] 116-116: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) 🤖 Prompt for AI Agents |
||
|
|
||
| if payload.Labels != nil { | ||
| currentRule, err := hr.managementClient.GetRuleById(req.Context(), id) | ||
| if err != nil { | ||
| status, message := parseError(err) | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(status), | ||
| Message: &message, | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| // platformLabels uses "" to signal "drop this label"; the management | ||
| // layer's UpdatePlatformAlertRule interprets "" as a delete directive. | ||
| // userLabels is the fully-merged map for user-defined rules where we | ||
| // simply omit deleted keys rather than set them to "". | ||
| platformLabels := make(map[string]string) | ||
| userLabels := make(map[string]string) | ||
| for k, v := range currentRule.Labels { | ||
| userLabels[k] = v | ||
| } | ||
| for k, pv := range *payload.Labels { | ||
| if pv == nil || *pv == "" { | ||
| platformLabels[k] = "" | ||
| delete(userLabels, k) | ||
| } else { | ||
| platformLabels[k] = *pv | ||
| userLabels[k] = *pv | ||
| } | ||
| } | ||
|
|
||
| updatedPlatformRule := monitoringv1.Rule{Labels: platformLabels} | ||
|
|
||
| err = hr.managementClient.UpdatePlatformAlertRule(req.Context(), id, updatedPlatformRule) | ||
| if err != nil { | ||
| var ve *management.ValidationError | ||
| var nf *management.NotFoundError | ||
| if errors.As(err, &ve) || errors.As(err, &nf) { | ||
| status, message := parseError(err) | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(status), | ||
| Message: &message, | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| var na *management.NotAllowedError | ||
| if errors.As(err, &na) { | ||
| updatedUserRule := currentRule | ||
| updatedUserRule.Labels = userLabels | ||
|
|
||
| newRuleId, err := hr.managementClient.UpdateUserDefinedAlertRule(req.Context(), id, updatedUserRule) | ||
|
Contributor
There was a problem hiding this comment. Shouldn't we determine if an alert rule is a platform or user defined ar before just trying platform and then falling back to user on a not allowed error? |
||
| if err != nil { | ||
| status, message := parseError(err) | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(status), | ||
| Message: &message, | ||
| }) | ||
| continue | ||
| } | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: newRuleId, | ||
| StatusCode: int32(http.StatusNoContent), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| status, message := parseError(err) | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(status), | ||
| Message: &message, | ||
| }) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| if notAllowedEnabled && payload.Labels == nil && payload.Classification == nil { | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(http.StatusMethodNotAllowed), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(http.StatusNoContent), | ||
| }) | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) | ||
| if err := json.NewEncoder(w).Encode(BulkUpdateAlertRulesResponse{Rules: results}); err != nil { | ||
| log.WithError(err).Warn("failed to encode bulk update response") | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are looping through up to 100 alert rules I think rather than repeatedly trying to perform an update only to get a permission failure it would be best to either cache the permission failures or perform the permission checks ahead of time