From 91aa3e555eccc70ffabd18bcf47e6750b62e5439 Mon Sep 17 00:00:00 2001 From: Shirly Radco Date: Wed, 12 Aug 2026 12:26:06 +0300 Subject: [PATCH] management: add single alert rule endpoints Keep the bulk PATCH/DELETE /rules APIs and add per-rule endpoints for easier client use and reviewability: - PATCH /rules/{ruleId} - DELETE /rules/{ruleId} Single update shares validation and mutation logic with BulkUpdateAlertRules. Adds unit and e2e coverage (including RBAC) plus API docs. Address review feedback: reject empty classification; ignore alertname on platform update via protected labels. Signed-off-by: Shirly Radco Co-authored-by: AI Assistant --- api/openapi.yaml | 165 +++++++ docs/alert-management.md | 35 ++ docs/alert-rule-classification.md | 24 +- .../alert_rule_bulk_update.go | 96 +--- .../alert_rule_bulk_update_test.go | 16 + .../managementrouter/alert_rule_delete.go | 23 + .../alert_rule_delete_test.go | 125 ++++++ .../managementrouter/alert_rule_update.go | 137 ++++++ .../alert_rule_update_test.go | 222 ++++++++++ internal/managementrouter/api_generated.go | 64 ++- pkg/management/update_alert_rule_labels.go | 3 + .../update_alert_rule_labels_test.go | 46 ++ pkg/management/update_platform_alert_rule.go | 6 - .../update_platform_alert_rule_test.go | 28 +- test/e2e/single_alert_rule_test.go | 411 ++++++++++++++++++ 15 files changed, 1292 insertions(+), 109 deletions(-) create mode 100644 internal/managementrouter/alert_rule_delete.go create mode 100644 internal/managementrouter/alert_rule_delete_test.go create mode 100644 internal/managementrouter/alert_rule_update.go create mode 100644 internal/managementrouter/alert_rule_update_test.go create mode 100644 pkg/management/update_alert_rule_labels_test.go create mode 100644 test/e2e/single_alert_rule_test.go diff --git a/api/openapi.yaml b/api/openapi.yaml index 317e933d5..7e6901b90 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -21,6 +21,10 @@ paths: so partial success is visible to the caller. Supports label overrides, drop/restore toggles (platform rules only), and classification label updates. + When both classification and labels are set for a rule, classification + is applied first, then labels. These steps are not atomic: if the label + update fails after classification succeeded, the classification change + remains applied and the per-rule result reports failure. requestBody: required: true content: @@ -154,6 +158,141 @@ paths: schema: $ref: "#/components/schemas/ErrorResponse" + /rules/{ruleId}: + parameters: + - name: ruleId + in: path + required: true + schema: + type: string + description: Stable alert rule ID. + patch: + operationId: UpdateAlertRule + summary: Update a single alert rule + description: > + Updates one alert rule by its stable ID. Supports label overrides, + drop/restore toggles (platform rules only), and classification label + updates. Same mutation semantics as BulkUpdateAlertRules for a single ID. + When both classification and labels are set, classification is applied + first, then labels. These steps are not atomic: if the label update + fails after classification succeeded, the classification change remains + applied and the request returns an error. + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/UpdateAlertRuleRequest" + responses: + "200": + description: > + Update result. On success statusCode is 204; the id may differ from + the path ruleId when labels change the stable ID. + content: + application/json: + schema: + $ref: "#/components/schemas/UpdateAlertRuleResult" + "400": + description: > + Invalid request body, blank ruleId, or invalid update fields + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "401": + description: Missing or invalid authorization token + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "403": + description: Forbidden (insufficient RBAC permissions) + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "404": + description: Alert rule not found + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "405": + description: Operation not allowed (e.g. rule is externally managed) + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "409": + description: Conflict (e.g. concurrent update) + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "413": + description: Request body exceeds the 1 MB limit + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "500": + description: Unexpected server error + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + delete: + operationId: DeleteAlertRule + summary: Delete a single alert rule + description: > + Deletes one alert rule by its stable ID. Same mutation semantics as + BulkDeleteUserDefinedAlertRules for a single ID. + responses: + "204": + description: Alert rule deleted successfully + "400": + description: Invalid ruleId (e.g. blank after trimming) + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "401": + description: Missing or invalid authorization token + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "403": + description: Forbidden (insufficient RBAC permissions) + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "404": + description: Alert rule not found + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "405": + description: Operation not allowed (e.g. platform or externally managed) + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "409": + description: Conflict (e.g. concurrent update) + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "500": + description: Unexpected server error + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + components: schemas: AlertRuleSpec: @@ -324,6 +463,32 @@ components: classification: $ref: "#/components/schemas/AlertRuleClassificationUpdate" + UpdateAlertRuleRequest: + type: object + description: > + Partial update for a single alert rule. At least one of labels, + alertingRuleEnabled, or classification must be set. alertingRuleEnabled + cannot be combined with labels or classification in the same request. + properties: + labels: + type: object + additionalProperties: + type: string + nullable: true + description: > + Label key/value pairs to set. A null or empty-string value removes + the label. Omitting this field leaves existing labels unchanged. + alertingRuleEnabled: + type: boolean + nullable: true + description: > + When false, drops the alert rule via an AlertRelabelConfig Drop + action — the rule no longer appears in Prometheus query results. + When true, restores a previously dropped rule. + Only supported for platform alert rules. + classification: + $ref: "#/components/schemas/AlertRuleClassificationUpdate" + UpdateAlertRuleResult: type: object required: diff --git a/docs/alert-management.md b/docs/alert-management.md index 0951ca9b6..a8f84737b 100644 --- a/docs/alert-management.md +++ b/docs/alert-management.md @@ -40,6 +40,41 @@ This is a cluster configuration choice and does not change the plugin API shape. The plugin intentionally reads from only the in-cluster Alertmanager endpoints. Supporting multiple external Alertmanagers would introduce ambiguous alert state and silencing outcomes because each instance can apply different routing, inhibition, and silence configurations. +### Managing alert rules via the Management API + +| Operation | Single | Bulk | +|---|---|---| +| Create | `POST /api/v1/alerting/rules` | n/a | +| Update (labels, drop/restore, classification) | `PATCH /api/v1/alerting/rules/{ruleId}` | `PATCH /api/v1/alerting/rules` | +| Delete | `DELETE /api/v1/alerting/rules/{ruleId}` | `DELETE /api/v1/alerting/rules` | + +**Single update** (`PATCH /rules/{ruleId}`): +- Request body uses `UpdateAlertRuleRequest` (labels and/or classification, or + `alertingRuleEnabled` alone for drop/restore). +- Success: HTTP `200` with `UpdateAlertRuleResult` (`statusCode: 204`). The + returned `id` may differ from the path `ruleId` when labels change the stable ID. +- Failure: standard `ErrorResponse` with the corresponding HTTP status + (400/401/403/404/405/409/413/500). Errors include a message so callers can act on them. +- Non-atomic combined updates: when both `classification` and `labels` are set, + classification is applied first, then labels. If the label step fails, the + classification change may already be persisted and the request still returns + an error. Retry or inspect cluster state before re-applying classification. + +**Bulk update** (`PATCH /rules`): +- Request body includes `ruleIds` (1–100) plus the same mutation fields. +- Always returns HTTP `200` with per-rule `statusCode`/`message` entries so + partial success is visible. +- Same non-atomic classification-then-labels behavior as single update; a failed + label step is reported on that rule's result while classification may remain. + +**Single delete** (`DELETE /rules/{ruleId}`): +- Success: HTTP `204`. +- Failure: `ErrorResponse` with HTTP status (400/401/403/404/405/409/500). + +**Bulk delete** (`DELETE /rules`): +- Request body includes `ruleIds` (1–100). +- Always returns HTTP `200` with per-rule results. + ### Managing user-defined alert rules | Rule ownership | Editable? | Classification? | Drop/Restore? | diff --git a/docs/alert-rule-classification.md b/docs/alert-rule-classification.md index 04141707f..176cbaf16 100644 --- a/docs/alert-rule-classification.md +++ b/docs/alert-rule-classification.md @@ -205,9 +205,11 @@ APIs: ``` - `openshift_io_alert_rule_layer`: `cluster` or `namespace` - To remove a classification override, set the field to `null` (e.g. `"openshift_io_alert_rule_layer": null`). + - Do not combine `classification` with `alertingRuleEnabled` in the same request. - Response: - - 200 OK with a status payload (same format as other rule PATCH responses), where `status_code` is 204 on success. - - Standard error body on failure (400 validation, 404 not found, etc.) + - HTTP `200` with `UpdateAlertRuleResult` (`statusCode` is `204` on success). + - Standard `ErrorResponse` body on failure (400 validation, 403 forbidden, + 404 not found, 405 not allowed for user-defined rules, 409 conflict, etc.). - Bulk update: - Method: `PATCH /api/v1/alerting/rules` - Request body: @@ -221,7 +223,8 @@ APIs: } ``` - Response: - - 200 OK with per-rule results (same format as other bulk rule PATCH responses). Clients should handle partial failures. + - HTTP `200` with per-rule results (same `UpdateAlertRuleResult` shape). + Clients should handle partial failures via per-rule `statusCode`/`message`. Direct K8s (supported for power users/GitOps): - For platform rules: create or update the `AlertRelabelConfig` CR in `openshift-monitoring` @@ -229,11 +232,16 @@ Direct K8s (supported for power users/GitOps): - UI should check update permissions with SelfSubjectAccessReview before showing an editor. Notes: -- These endpoints are intended for updating **classification only** (component/layer overrides), - with permissions enforced based on the rule's ownership (platform, user workload, operator-managed, - GitOps-managed). -- To update other rule fields (expr/labels/annotations/etc.), use `PATCH /api/v1/alerting/rules/{ruleId}`. - Clients that need to update both should issue two requests. The combined operation is not atomic. +- Classification overrides for platform rules are applied via AlertRelabelConfig. + User-defined rules reject ARC-based classification updates (`405`). +- To update other rule fields (labels/etc.), use the same + `PATCH /api/v1/alerting/rules/{ruleId}` endpoint with a `labels` body (or the + bulk `PATCH /rules` variant). `alertingRuleEnabled` cannot be combined with + `labels` or `classification` in one request; issue separate calls when needed. +- When a request includes both `classification` and `labels`, classification is + applied first, then labels. The two steps are not atomic: if labels fail after + classification succeeded, the classification override may already be persisted + while the API still returns an error (or a per-rule failure in bulk). ## Security Notes - Classification overrides are stored in AlertRelabelConfig CRs in `openshift-monitoring`, diff --git a/internal/managementrouter/alert_rule_bulk_update.go b/internal/managementrouter/alert_rule_bulk_update.go index 1042bf10b..453218c8f 100644 --- a/internal/managementrouter/alert_rule_bulk_update.go +++ b/internal/managementrouter/alert_rule_bulk_update.go @@ -5,8 +5,6 @@ import ( "io" "net/http" "strings" - - "github.com/openshift/monitoring-plugin/pkg/management" ) func (hr *httpRouter) BulkUpdateAlertRules(w http.ResponseWriter, req *http.Request) { @@ -36,22 +34,16 @@ func (hr *httpRouter) BulkUpdateAlertRules(w http.ResponseWriter, req *http.Requ 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 + fields := alertRuleUpdateFields{ + Labels: payload.Labels, + AlertingRuleEnabled: payload.AlertingRuleEnabled, + Classification: payload.Classification, } - if payload.AlertingRuleEnabled != nil && (payload.Labels != nil || payload.Classification != nil) { - writeError(w, http.StatusBadRequest, "alertingRuleEnabled cannot be combined with labels or classification in the same request") + if msg := validateAlertRuleUpdateFields(fields); msg != "" { + writeError(w, http.StatusBadRequest, msg) 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 { @@ -66,82 +58,18 @@ func (hr *httpRouter) BulkUpdateAlertRules(w http.ResponseWriter, req *http.Requ continue } - if haveToggle { - var err error - if !enabled { - err = hr.managementClient.DropAlertRule(req.Context(), id) - } else { - err = hr.managementClient.RestoreAlertRule(req.Context(), id) - } - if err != nil { - status, message := parseError(err) - results = append(results, UpdateAlertRuleResult{ - Id: id, - StatusCode: int32(status), - Message: &message, - }) - continue - } + newID, err := hr.applyAlertRuleUpdate(req.Context(), id, fields) + if err != nil { + status, message := parseError(err) results = append(results, UpdateAlertRuleResult{ Id: id, - StatusCode: int32(http.StatusNoContent), + 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 { - status, message := parseError(err) - results = append(results, UpdateAlertRuleResult{ - Id: id, - StatusCode: int32(status), - Message: &message, - }) - continue - } - } - } - - if payload.Labels != nil { - newRuleId, err := hr.managementClient.UpdateAlertRuleLabels(req.Context(), id, *payload.Labels) - 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 - } - results = append(results, UpdateAlertRuleResult{ - Id: id, + Id: newID, StatusCode: int32(http.StatusNoContent), }) } diff --git a/internal/managementrouter/alert_rule_bulk_update_test.go b/internal/managementrouter/alert_rule_bulk_update_test.go index e805f6c59..5b9abecdc 100644 --- a/internal/managementrouter/alert_rule_bulk_update_test.go +++ b/internal/managementrouter/alert_rule_bulk_update_test.go @@ -399,6 +399,22 @@ func TestBulkUpdateAlertRules_MissingAllUpdateFields(t *testing.T) { } } +func TestBulkUpdateAlertRules_RejectsEmptyClassification(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + w := f.do(t, map[string]any{ + "ruleIds": []string{user1Id}, + "classification": map[string]any{}, + }) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d", w.Code) + } + if !strings.Contains(w.Body.String(), "classification must set at least one field") { + t.Errorf("unexpected body: %s", w.Body.String()) + } +} + func TestBulkUpdateAlertRules_EnabledToggle(t *testing.T) { user1Id, _, platformId := buFixtureIDs() f := newBUFixture(t) diff --git a/internal/managementrouter/alert_rule_delete.go b/internal/managementrouter/alert_rule_delete.go new file mode 100644 index 000000000..0bd219f5f --- /dev/null +++ b/internal/managementrouter/alert_rule_delete.go @@ -0,0 +1,23 @@ +package managementrouter + +import ( + "net/http" + "strings" +) + +// DeleteAlertRule implements ServerInterface for DELETE /rules/{ruleId}. +func (hr *httpRouter) DeleteAlertRule(w http.ResponseWriter, req *http.Request, ruleId string) { + id := strings.TrimSpace(ruleId) + if id == "" { + writeError(w, http.StatusBadRequest, "ruleId is required") + return + } + + if err := hr.managementClient.DeleteAlertRuleById(req.Context(), id); err != nil { + status, message := parseError(err) + writeError(w, status, message) + return + } + + w.WriteHeader(http.StatusNoContent) +} diff --git a/internal/managementrouter/alert_rule_delete_test.go b/internal/managementrouter/alert_rule_delete_test.go new file mode 100644 index 000000000..59766614d --- /dev/null +++ b/internal/managementrouter/alert_rule_delete_test.go @@ -0,0 +1,125 @@ +package managementrouter_test + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + + "github.com/openshift/monitoring-plugin/internal/managementrouter" + alertrule "github.com/openshift/monitoring-plugin/pkg/alert_rule" + "github.com/openshift/monitoring-plugin/pkg/k8s" + "github.com/openshift/monitoring-plugin/pkg/management" + "github.com/openshift/monitoring-plugin/pkg/management/testutils" + "github.com/openshift/monitoring-plugin/pkg/managementlabels" +) + +func singleDeleteRequest(t *testing.T, router http.Handler, ruleID string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodDelete, + "/api/v1/alerting/rules/"+ruleID, + nil, + ) + req.Header.Set("Authorization", "Bearer test-token") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + return w +} + +func TestDeleteAlertRule_Succeeds(t *testing.T) { + tv := newDeleteRuleRouter(t) + w := singleDeleteRequest(t, tv.router, tv.userRule1Id) + if w.Code != http.StatusNoContent { + t.Fatalf("expected 204, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestDeleteAlertRule_NotFound(t *testing.T) { + tv := newDeleteRuleRouter(t) + w := singleDeleteRequest(t, tv.router, "missing-rule-id") + if w.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestDeleteAlertRule_MissingAuth(t *testing.T) { + tv := newDeleteRuleRouter(t) + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodDelete, + "/api/v1/alerting/rules/"+tv.userRule1Id, + nil, + ) + w := httptest.NewRecorder() + tv.router.ServeHTTP(w, req) + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestDeleteAlertRule_PlatformRule(t *testing.T) { + tv := newDeleteRuleRouter(t) + w := singleDeleteRequest(t, tv.router, tv.platformRuleId) + if w.Code != http.StatusNoContent { + t.Fatalf("expected 204 for platform delete, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestDeleteAlertRule_BlankRuleId(t *testing.T) { + tv := newDeleteRuleRouter(t) + w := singleDeleteRequest(t, tv.router, "%20") + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestDeleteAlertRule_NotAllowed(t *testing.T) { + gitOpsRule := monitoringv1.Rule{ + Alert: "gitops-alert", + Labels: map[string]string{ + k8s.PrometheusRuleLabelNamespace: "default", + k8s.PrometheusRuleLabelName: "gitops-pr", + managementlabels.RuleManagedByLabel: managementlabels.ManagedByGitOps, + }, + } + gitOpsRuleID := alertrule.GetAlertingRuleId(&gitOpsRule) + + mockK8s := &testutils.MockClient{} + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == gitOpsRuleID { + return gitOpsRule, true + } + return monitoringv1.Rule{}, false + }, + } + } + mockK8s.NamespaceFunc = func() k8s.NamespaceInterface { + return &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(string) bool { return false }, + } + } + r := managementrouter.New(management.New(context.Background(), mockK8s)) + + w := singleDeleteRequest(t, r, gitOpsRuleID) + if w.Code != http.StatusMethodNotAllowed { + t.Fatalf("expected 405, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestDeleteAlertRule_ErrorBodyIncludesMessage(t *testing.T) { + tv := newDeleteRuleRouter(t) + w := singleDeleteRequest(t, tv.router, "missing-rule-id") + if w.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d", w.Code) + } + if !strings.Contains(w.Body.String(), "error") { + t.Errorf("expected error payload, got %s", w.Body.String()) + } +} diff --git a/internal/managementrouter/alert_rule_update.go b/internal/managementrouter/alert_rule_update.go new file mode 100644 index 000000000..74e284795 --- /dev/null +++ b/internal/managementrouter/alert_rule_update.go @@ -0,0 +1,137 @@ +package managementrouter + +import ( + "context" + "encoding/json" + "io" + "net/http" + "strings" + + "github.com/openshift/monitoring-plugin/pkg/management" +) + +// alertRuleUpdateFields is the shared mutation payload for single and bulk +// update endpoints (everything except rule IDs). +type alertRuleUpdateFields struct { + Labels *map[string]*string + AlertingRuleEnabled *bool + Classification *AlertRuleClassificationPatch +} + +func validateAlertRuleUpdateFields(f alertRuleUpdateFields) string { + if f.AlertingRuleEnabled == nil && f.Labels == nil && f.Classification == nil { + return "one of alertingRuleEnabled (toggle drop/restore) or labels (set/unset) or classification is required" + } + if f.AlertingRuleEnabled != nil && (f.Labels != nil || f.Classification != nil) { + return "alertingRuleEnabled cannot be combined with labels or classification in the same request" + } + if f.Classification != nil && + !f.Classification.ComponentSet && + !f.Classification.LayerSet && + !f.Classification.ComponentFromSet && + !f.Classification.LayerFromSet { + return "classification must set at least one field" + } + return "" +} + +// applyAlertRuleUpdate applies one update mutation. On success it returns the +// effective rule ID (which may change when labels are updated). +// Classification is applied before labels when both are set; those steps are +// not atomic (a later label failure leaves a successful classification applied). +func (hr *httpRouter) applyAlertRuleUpdate(ctx context.Context, id string, f alertRuleUpdateFields) (string, error) { + if f.AlertingRuleEnabled != nil { + if !*f.AlertingRuleEnabled { + return id, hr.managementClient.DropAlertRule(ctx, id) + } + return id, hr.managementClient.RestoreAlertRule(ctx, id) + } + + resultID := id + + if f.Classification != nil { + cl := f.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(ctx, update); err != nil { + return id, err + } + } + } + + if f.Labels != nil { + newRuleId, err := hr.managementClient.UpdateAlertRuleLabels(ctx, id, *f.Labels) + if err != nil { + return id, err + } + resultID = newRuleId + } + + return resultID, nil +} + +// UpdateAlertRule implements ServerInterface for PATCH /rules/{ruleId}. +func (hr *httpRouter) UpdateAlertRule(w http.ResponseWriter, req *http.Request, ruleId string) { + id := strings.TrimSpace(ruleId) + if id == "" { + writeError(w, http.StatusBadRequest, "ruleId is required") + return + } + + 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 + } + + var payload UpdateAlertRuleRequest + if err := json.Unmarshal(body, &payload); err != nil { + writeError(w, http.StatusBadRequest, "invalid request body: "+err.Error()) + return + } + + fields := alertRuleUpdateFields{ + Labels: payload.Labels, + AlertingRuleEnabled: payload.AlertingRuleEnabled, + Classification: payload.Classification, + } + if msg := validateAlertRuleUpdateFields(fields); msg != "" { + writeError(w, http.StatusBadRequest, msg) + return + } + + newID, err := hr.applyAlertRuleUpdate(req.Context(), id, fields) + if err != nil { + status, message := parseError(err) + writeError(w, status, message) + return + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if err := json.NewEncoder(w).Encode(UpdateAlertRuleResult{ + Id: newID, + StatusCode: int32(http.StatusNoContent), + }); err != nil { + log.WithError(err).Warn("failed to encode update alert rule response") + } +} diff --git a/internal/managementrouter/alert_rule_update_test.go b/internal/managementrouter/alert_rule_update_test.go new file mode 100644 index 000000000..996b536dd --- /dev/null +++ b/internal/managementrouter/alert_rule_update_test.go @@ -0,0 +1,222 @@ +package managementrouter_test + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/openshift/monitoring-plugin/internal/managementrouter" + alertrule "github.com/openshift/monitoring-plugin/pkg/alert_rule" + "github.com/openshift/monitoring-plugin/pkg/k8s" + "github.com/openshift/monitoring-plugin/pkg/management/testutils" +) + +func (f *buFixture) doSingleUpdate(t *testing.T, ruleID string, body any) *httptest.ResponseRecorder { + t.Helper() + buf, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal: %v", err) + } + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPatch, + "/api/v1/alerting/rules/"+ruleID, + bytes.NewReader(buf), + ) + req.Header.Set("Authorization", "Bearer test-token") + w := httptest.NewRecorder() + f.router.ServeHTTP(w, req) + return w +} + +func (f *buFixture) doSingleUpdateRaw(t *testing.T, ruleID string, raw []byte) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPatch, + "/api/v1/alerting/rules/"+ruleID, + bytes.NewReader(raw), + ) + req.Header.Set("Authorization", "Bearer test-token") + w := httptest.NewRecorder() + f.router.ServeHTTP(w, req) + return w +} + +func TestUpdateAlertRule_UpdatesUserRuleLabels(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + + expectedId := alertrule.GetAlertingRuleId(&monitoringv1.Rule{ + Alert: "user-alert-1", Expr: intstr.FromString("up == 0"), + Labels: map[string]string{"severity": "warning", "component": "api"}, + }) + + w := f.doSingleUpdate(t, user1Id, map[string]any{ + "labels": map[string]string{"component": "api"}, + }) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + + var resp managementrouter.UpdateAlertRuleResult + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + if resp.Id != expectedId { + t.Errorf("id: got %s want %s", resp.Id, expectedId) + } + if resp.StatusCode != http.StatusNoContent { + t.Errorf("statusCode: got %d want %d", resp.StatusCode, http.StatusNoContent) + } +} + +func TestUpdateAlertRule_NotFound(t *testing.T) { + f := newBUFixture(t) + w := f.doSingleUpdate(t, "missing-rule-id", map[string]any{ + "labels": map[string]string{"severity": "warning"}, + }) + if w.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d: %s", w.Code, w.Body) + } +} + +func TestUpdateAlertRule_RejectsEmptyBody(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + w := f.doSingleUpdate(t, user1Id, map[string]any{}) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body) + } +} + +func TestUpdateAlertRule_RejectsEmptyClassification(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + w := f.doSingleUpdate(t, user1Id, map[string]any{ + "classification": map[string]any{}, + }) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body) + } + if !strings.Contains(w.Body.String(), "classification must set at least one field") { + t.Errorf("unexpected body: %s", w.Body.String()) + } +} + +func TestUpdateAlertRule_RejectsToggleCombinedWithLabels(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + enabled := false + w := f.doSingleUpdate(t, user1Id, map[string]any{ + "alertingRuleEnabled": enabled, + "labels": map[string]string{"severity": "warning"}, + }) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body) + } + if !strings.Contains(w.Body.String(), "alertingRuleEnabled cannot be combined") { + t.Errorf("unexpected body: %s", w.Body.String()) + } +} + +func TestUpdateAlertRule_InvalidJSON(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + w := f.doSingleUpdateRaw(t, user1Id, []byte("{not-json")) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body) + } +} + +func TestUpdateAlertRule_BodyTooLarge(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + large := make([]byte, 1<<20+1) + for i := range large { + large[i] = 'a' + } + w := f.doSingleUpdateRaw(t, user1Id, large) + if w.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("expected 413, got %d: %s", w.Code, w.Body) + } +} + +func TestUpdateAlertRule_MissingAuth(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + buf, _ := json.Marshal(map[string]any{"labels": map[string]string{"component": "api"}}) + req := httptest.NewRequestWithContext( + context.Background(), + http.MethodPatch, + "/api/v1/alerting/rules/"+user1Id, + bytes.NewReader(buf), + ) + w := httptest.NewRecorder() + f.router.ServeHTTP(w, req) + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401, got %d: %s", w.Code, w.Body) + } +} + +func TestUpdateAlertRule_DropPlatformRule(t *testing.T) { + _, _, platformId := buFixtureIDs() + f := newBUFixture(t) + f.mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{} + } + f.rebuild() + + enabled := false + w := f.doSingleUpdate(t, platformId, map[string]any{ + "alertingRuleEnabled": enabled, + }) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + var resp managementrouter.UpdateAlertRuleResult + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + if resp.Id != platformId || resp.StatusCode != http.StatusNoContent { + t.Errorf("got id=%s status=%d", resp.Id, resp.StatusCode) + } +} + +func TestUpdateAlertRule_DropUserRuleNotAllowed(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + f.mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{} + } + f.rebuild() + + enabled := false + w := f.doSingleUpdate(t, user1Id, map[string]any{ + "alertingRuleEnabled": enabled, + }) + if w.Code != http.StatusMethodNotAllowed { + t.Fatalf("expected 405, got %d: %s", w.Code, w.Body) + } +} + +func TestUpdateAlertRule_ClassificationUserRulesNotAllowed(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + component := "networking" + w := f.doSingleUpdate(t, user1Id, map[string]any{ + "classification": map[string]any{ + "openshift_io_alert_rule_component": component, + }, + }) + if w.Code != http.StatusMethodNotAllowed { + t.Fatalf("expected 405, got %d: %s", w.Code, w.Body) + } +} diff --git a/internal/managementrouter/api_generated.go b/internal/managementrouter/api_generated.go index 60496c281..093ac1dee 100644 --- a/internal/managementrouter/api_generated.go +++ b/internal/managementrouter/api_generated.go @@ -39,7 +39,7 @@ type AlertRuleSpec struct { // BulkDeleteAlertRulesRequest defines model for BulkDeleteAlertRulesRequest. type BulkDeleteAlertRulesRequest struct { - // RuleIds List of stable alert rule IDs to delete. + // RuleIds List of stable alert rule IDs to delete (at most 100 per request). RuleIds []string `json:"ruleIds"` } @@ -51,7 +51,7 @@ type BulkDeleteAlertRulesResponse struct { // BulkUpdateAlertRulesRequest defines model for BulkUpdateAlertRulesRequest. type BulkUpdateAlertRulesRequest struct { - // AlertingRuleEnabled When false, drops the alert rule via an AlertRelabelConfig Drop action — the rule no longer appears in Prometheus query results. When true, restores a previously dropped rule. Only supported for platform alert rules. Cannot be combined with labels or classification in the same request (returns HTTP 400). + // AlertingRuleEnabled When false, drops the alert rule via an AlertRelabelConfig Drop action — the rule no longer appears in Prometheus query results. When true, restores a previously dropped rule. Only supported for platform alert rules. AlertingRuleEnabled *bool `json:"alertingRuleEnabled,omitempty"` // Classification Partial update for alert rule classification labels. Each field supports three states: omitted (leave unchanged), null (clear the override), or a string value (set the override). The three-state semantics require a custom JSON decoder; the Go type AlertRuleClassificationPatch is used at runtime instead of the generated struct. @@ -60,7 +60,7 @@ type BulkUpdateAlertRulesRequest struct { // Labels Label key/value pairs to set. A null or empty-string value removes the label. Omitting this field leaves existing labels unchanged. Labels *map[string]*string `json:"labels,omitempty"` - // RuleIds List of stable alert rule IDs to update. + // RuleIds List of stable alert rule IDs to update (at most 100 per request). RuleIds []string `json:"ruleIds"` } @@ -115,6 +115,18 @@ type PrometheusRuleTarget struct { PrometheusRuleNamespace string `json:"prometheusRuleNamespace"` } +// UpdateAlertRuleRequest Partial update for a single alert rule. At least one of labels, alertingRuleEnabled, or classification must be set. alertingRuleEnabled cannot be combined with labels or classification in the same request. +type UpdateAlertRuleRequest struct { + // AlertingRuleEnabled When false, drops the alert rule via an AlertRelabelConfig Drop action — the rule no longer appears in Prometheus query results. When true, restores a previously dropped rule. Only supported for platform alert rules. + AlertingRuleEnabled *bool `json:"alertingRuleEnabled,omitempty"` + + // Classification Partial update for alert rule classification labels. Each field supports three states: omitted (leave unchanged), null (clear the override), or a string value (set the override). The three-state semantics require a custom JSON decoder; the Go type AlertRuleClassificationPatch is used at runtime instead of the generated struct. + Classification *AlertRuleClassificationUpdate `json:"classification,omitempty"` + + // Labels Label key/value pairs to set. A null or empty-string value removes the label. Omitting this field leaves existing labels unchanged. + Labels *map[string]*string `json:"labels,omitempty"` +} + // UpdateAlertRuleResult defines model for UpdateAlertRuleResult. type UpdateAlertRuleResult struct { // Id The stable alert rule ID that was processed. @@ -136,6 +148,9 @@ type BulkUpdateAlertRulesJSONRequestBody = BulkUpdateAlertRulesRequest // CreateAlertRuleJSONRequestBody defines body for CreateAlertRule for application/json ContentType. type CreateAlertRuleJSONRequestBody = CreateAlertRuleRequest +// UpdateAlertRuleJSONRequestBody defines body for UpdateAlertRule for application/json ContentType. +type UpdateAlertRuleJSONRequestBody = UpdateAlertRuleRequest + // ServerInterface represents all server handlers. type ServerInterface interface { // Bulk delete user-defined alert rules @@ -147,6 +162,12 @@ type ServerInterface interface { // Create an alert rule // (POST /rules) CreateAlertRule(w http.ResponseWriter, r *http.Request) + // Delete a single alert rule + // (DELETE /rules/{ruleId}) + DeleteAlertRule(w http.ResponseWriter, r *http.Request, ruleId string) + // Update a single alert rule + // (PATCH /rules/{ruleId}) + UpdateAlertRule(w http.ResponseWriter, r *http.Request, ruleId string) } // ServerInterfaceWrapper converts contexts to parameters. @@ -160,7 +181,6 @@ type MiddlewareFunc func(http.Handler) http.Handler // BulkDeleteUserDefinedAlertRules operation middleware func (siw *ServerInterfaceWrapper) BulkDeleteUserDefinedAlertRules(w http.ResponseWriter, r *http.Request) { - handler := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { siw.Handler.BulkDeleteUserDefinedAlertRules(w, r) })) @@ -174,7 +194,6 @@ func (siw *ServerInterfaceWrapper) BulkDeleteUserDefinedAlertRules(w http.Respon // BulkUpdateAlertRules operation middleware func (siw *ServerInterfaceWrapper) BulkUpdateAlertRules(w http.ResponseWriter, r *http.Request) { - handler := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { siw.Handler.BulkUpdateAlertRules(w, r) })) @@ -188,7 +207,6 @@ func (siw *ServerInterfaceWrapper) BulkUpdateAlertRules(w http.ResponseWriter, r // CreateAlertRule operation middleware func (siw *ServerInterfaceWrapper) CreateAlertRule(w http.ResponseWriter, r *http.Request) { - handler := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { siw.Handler.CreateAlertRule(w, r) })) @@ -200,6 +218,36 @@ func (siw *ServerInterfaceWrapper) CreateAlertRule(w http.ResponseWriter, r *htt handler.ServeHTTP(w, r) } +// DeleteAlertRule operation middleware +func (siw *ServerInterfaceWrapper) DeleteAlertRule(w http.ResponseWriter, r *http.Request) { + ruleId := mux.Vars(r)["ruleId"] + + handler := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + siw.Handler.DeleteAlertRule(w, r, ruleId) + })) + + for _, middleware := range siw.HandlerMiddlewares { + handler = middleware(handler) + } + + handler.ServeHTTP(w, r) +} + +// UpdateAlertRule operation middleware +func (siw *ServerInterfaceWrapper) UpdateAlertRule(w http.ResponseWriter, r *http.Request) { + ruleId := mux.Vars(r)["ruleId"] + + handler := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + siw.Handler.UpdateAlertRule(w, r, ruleId) + })) + + for _, middleware := range siw.HandlerMiddlewares { + handler = middleware(handler) + } + + handler.ServeHTTP(w, r) +} + type UnescapedCookieParamError struct { ParamName string Err error @@ -319,5 +367,9 @@ func HandlerWithOptions(si ServerInterface, options GorillaServerOptions) http.H r.HandleFunc(options.BaseURL+"/rules", wrapper.CreateAlertRule).Methods("POST") + r.HandleFunc(options.BaseURL+"/rules/{ruleId}", wrapper.DeleteAlertRule).Methods("DELETE") + + r.HandleFunc(options.BaseURL+"/rules/{ruleId}", wrapper.UpdateAlertRule).Methods("PATCH") + return r } diff --git a/pkg/management/update_alert_rule_labels.go b/pkg/management/update_alert_rule_labels.go index de9d08e1b..af19db231 100644 --- a/pkg/management/update_alert_rule_labels.go +++ b/pkg/management/update_alert_rule_labels.go @@ -74,6 +74,9 @@ func (c *client) updateUserRuleLabels(ctx context.Context, alertRuleId string, r userLabels := copyStringMap(sourceRule.Labels) for k, pv := range labels { + if isProtectedLabel(k) { + continue + } if pv == nil || *pv == "" { delete(userLabels, k) } else { diff --git a/pkg/management/update_alert_rule_labels_test.go b/pkg/management/update_alert_rule_labels_test.go new file mode 100644 index 000000000..a669ad4ae --- /dev/null +++ b/pkg/management/update_alert_rule_labels_test.go @@ -0,0 +1,46 @@ +package management_test + +import ( + "context" + "testing" + + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + + "github.com/openshift/monitoring-plugin/pkg/k8s" +) + +func TestUpdateAlertRuleLabels_IgnoresProtectedLabelsOnUserRule(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, udUserRule) + + var savedPR *monitoringv1.PrometheusRule + pr := makePRWithRule("user-namespace", "user-rule", originalUserRule) + pr.UpdateFunc = func(_ context.Context, p monitoringv1.PrometheusRule) error { + savedPR = &p + return nil + } + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { return pr } + + fakeID := "fake-id" + critical := "critical" + labels := map[string]*string{ + k8s.AlertRuleLabelId: &fakeID, + "severity": &critical, + } + + _, err := client.UpdateAlertRuleLabels(context.Background(), originalUserRuleId, labels) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if savedPR == nil { + t.Fatal("expected PR to be updated") + } + + savedLabels := savedPR.Spec.Groups[0].Rules[0].Labels + if savedLabels[k8s.AlertRuleLabelId] == fakeID { + t.Errorf("protected label %q must not be overridden by the request", k8s.AlertRuleLabelId) + } + if savedLabels["severity"] != "critical" { + t.Errorf("expected severity=critical, got %q", savedLabels["severity"]) + } +} diff --git a/pkg/management/update_platform_alert_rule.go b/pkg/management/update_platform_alert_rule.go index 28ebe4be5..ea8a8c87f 100644 --- a/pkg/management/update_platform_alert_rule.go +++ b/pkg/management/update_platform_alert_rule.go @@ -58,12 +58,6 @@ func (c *client) UpdatePlatformAlertRule(ctx context.Context, alertRuleId string return err } - if v, ok := alertRule.Labels[managementlabels.AlertNameLabel]; ok { - if v != originalRule.Alert { - return &ValidationError{Message: fmt.Sprintf("label %q is immutable", managementlabels.AlertNameLabel)} - } - } - arName := rule.Labels[managementlabels.AlertingRuleLabelName] if arName == "" { arName = defaultAlertingRuleName diff --git a/pkg/management/update_platform_alert_rule_test.go b/pkg/management/update_platform_alert_rule_test.go index 1901b1b1e..fb8bf8d15 100644 --- a/pkg/management/update_platform_alert_rule_test.go +++ b/pkg/management/update_platform_alert_rule_test.go @@ -576,18 +576,36 @@ func TestUpdatePlatformAlertRule_IgnoresProtectedLabels(t *testing.T) { } } -func TestUpdatePlatformAlertRule_RejectsAlertNameChange(t *testing.T) { +func TestUpdatePlatformAlertRule_IgnoresAlertNameChange(t *testing.T) { client, mockK8s := newUpdatePlatformClient(t) + + var createdARC *osmv1.AlertRelabelConfig setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { - return &testutils.MockAlertRelabelConfigInterface{} + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, _ string) (*osmv1.AlertRelabelConfig, bool, error) { + return nil, false, nil + }, + CreateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) (*osmv1.AlertRelabelConfig, error) { + createdARC = &arc + return &arc, nil + }, + } }) updatedRule := copyRule(upOriginalPlatformRule) - updatedRule.Labels = map[string]string{"alertname": "NewName"} + updatedRule.Labels[managementlabels.AlertNameLabel] = "NewName" + updatedRule.Labels["new_label"] = "new_value" err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, updatedRule) - if err == nil || !strings.Contains(err.Error(), "immutable") { - t.Errorf("expected immutable alertname error, got: %v", err) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if createdARC != nil { + for _, cfg := range createdARC.Spec.Configs { + if string(cfg.TargetLabel) == managementlabels.AlertNameLabel { + t.Errorf("protected label %q must not be overridden by the request", managementlabels.AlertNameLabel) + } + } } } diff --git a/test/e2e/single_alert_rule_test.go b/test/e2e/single_alert_rule_test.go new file mode 100644 index 000000000..b834c82b2 --- /dev/null +++ b/test/e2e/single_alert_rule_test.go @@ -0,0 +1,411 @@ +//go:build e2e + +package e2e + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/openshift/monitoring-plugin/internal/managementrouter" + "github.com/openshift/monitoring-plugin/pkg/k8s" + "github.com/openshift/monitoring-plugin/test/e2e/framework" +) + +func TestUpdateAlertRule_Single_DropRestore(t *testing.T) { + f, err := framework.New() + if err != nil { + t.Fatalf("Failed to create framework: %v", err) + } + + ctx := context.Background() + ruleID := findPlatformAlertRuleId(ctx, t, f) + t.Logf("Using platform rule with ID: %s", ruleID) + defer cleanupARCsForRule(t, f, ctx, k8s.ClusterMonitoringNamespace, ruleID) + + patchDropSingle(ctx, t, f, ruleID, false) + + arcList, err := f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(k8s.ClusterMonitoringNamespace).List(ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("Failed to list ARCs: %v", err) + } + foundDropARC := false + for _, arc := range arcList.Items { + if hasDropActionForRule(arc, ruleID) { + foundDropARC = true + break + } + } + if !foundDropARC { + t.Fatal("Expected ARC with drop action after single-rule disable") + } + + patchDropSingle(ctx, t, f, ruleID, true) + + arcList, err = f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(k8s.ClusterMonitoringNamespace).List(ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("Failed to list ARCs after restore: %v", err) + } + for _, arc := range arcList.Items { + if hasDropActionForRule(arc, ruleID) { + t.Errorf("ARC %s/%s still has drop action after single-rule restore", arc.Namespace, arc.Name) + } + } +} + +func TestUpdateAlertRule_Single_Classification(t *testing.T) { + f, err := framework.New() + if err != nil { + t.Fatalf("Failed to create framework: %v", err) + } + + ctx := context.Background() + ruleID := findPlatformAlertRuleId(ctx, t, f) + defer cleanupARCsForRule(t, f, ctx, k8s.ClusterMonitoringNamespace, ruleID) + + component := "networking" + layer := "cluster" + status, err := tryUpdateAlertRuleSingle(ctx, f, f.BearerToken, ruleID, managementrouter.UpdateAlertRuleRequest{ + Classification: &managementrouter.AlertRuleClassificationPatch{ + Component: &component, + ComponentSet: true, + Layer: &layer, + LayerSet: true, + }, + }) + if err != nil { + t.Fatalf("single classification update failed: %v", err) + } + if status != http.StatusOK { + t.Fatalf("expected HTTP 200, got %d", status) + } + + arcList, err := f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(k8s.ClusterMonitoringNamespace).List(ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("Failed to list ARCs after classification: %v", err) + } + found := false + for _, arc := range arcList.Items { + if hasClassificationForRule(arc, "networking", "cluster") { + found = true + break + } + } + if !found { + t.Fatal("Expected ARC with classification labels after single-rule PATCH") + } +} + +// TestRBAC_UpdateAlertRule_Single mirrors TestRBAC_UpdateAlertRule against +// PATCH /rules/{ruleId} (HTTP status codes, not bulk per-rule envelopes). +func TestRBAC_UpdateAlertRule_Single(t *testing.T) { + f, err := framework.New() + if err != nil { + t.Fatalf("Failed to create framework: %v", err) + } + ctx := context.Background() + + nsY, cleanupY, err := f.CreateUserNamespace(ctx, "test-rbac-upd1-y") + if err != nil { + t.Fatalf("Failed to create namespace Y: %v", err) + } + defer func() { _ = cleanupY() }() + + nsZ, cleanupZ, err := f.CreateUserNamespace(ctx, "test-rbac-upd1-z") + if err != nil { + t.Fatalf("Failed to create namespace Z: %v", err) + } + defer func() { _ = cleanupZ() }() + + anonymousUser, err := f.CreateAnonymousUser(ctx, "e2e-rbac-upd1-a", "default") + if err != nil { + t.Fatalf("Failed to create anonymous user: %v", err) + } + defer func() { _ = anonymousUser.Cleanup() }() + + scopedUser, err := f.CreateScopedUser(ctx, "e2e-rbac-upd1-b", nsY, + "monitoring.coreos.com", []string{"prometheusrules"}, []string{"get", "create", "update", "patch"}) + if err != nil { + t.Fatalf("Failed to create scoped user: %v", err) + } + defer func() { _ = scopedUser.Cleanup() }() + + ruleInY := mustCreateRule(ctx, t, f, nsY, "RBACUpd1AlertY", "e2e-rbac-upd1-pr") + ruleInZ := mustCreateRule(ctx, t, f, nsZ, "RBACUpd1AlertZ", "e2e-rbac-upd1-pr") + ruleInY2 := mustCreateRule(ctx, t, f, nsY, "RBACUpd1AlertY2", "e2e-rbac-upd1-pr") + + waitForSingleUpdateCacheSync(ctx, t, f, anonymousUser.Token, ruleInY) + + cases := []struct { + name string + token string + ruleID string + wantStatus int + }{ + {"AnonymousUser_DeniedNamespaceY", anonymousUser.Token, ruleInY, http.StatusForbidden}, + {"AnonymousUser_DeniedNamespaceZ", anonymousUser.Token, ruleInZ, http.StatusForbidden}, + {"ScopedUser_SucceedsNamespaceY", scopedUser.Token, ruleInY, http.StatusOK}, + {"ScopedUser_DeniedNamespaceZ", scopedUser.Token, ruleInZ, http.StatusForbidden}, + {"ClusterAdmin_SucceedsNamespaceZ", f.BearerToken, ruleInZ, http.StatusOK}, + {"ClusterAdmin_SucceedsNamespaceY", f.BearerToken, ruleInY2, http.StatusOK}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + status := updateAlertRuleSingleWithToken(ctx, t, f, tc.token, tc.ruleID) + if status != tc.wantStatus { + t.Fatalf("Expected HTTP status %d, got %d", tc.wantStatus, status) + } + }) + } +} + +func TestDeleteAlertRule_Single(t *testing.T) { + f, err := framework.New() + if err != nil { + t.Fatalf("Failed to create framework: %v", err) + } + ctx := context.Background() + + ns, cleanup, err := f.CreateUserNamespace(ctx, "test-delete-single") + if err != nil { + t.Fatalf("Failed to create namespace: %v", err) + } + defer func() { _ = cleanup() }() + + keepID := mustCreateRule(ctx, t, f, ns, "KeepSingleAlert", "e2e-delete-single-pr") + deleteID := mustCreateRule(ctx, t, f, ns, "DeleteSingleAlert", "e2e-delete-single-pr") + _ = keepID + + err = framework.Poll(time.Second, time.Minute, func() error { + status, err := tryDeleteAlertRuleSingle(ctx, f, f.BearerToken, deleteID) + if err != nil { + return err + } + if status != http.StatusNoContent && status != http.StatusNotFound { + return fmt.Errorf("expected 204 or 404, got %d", status) + } + return nil + }) + if err != nil { + t.Fatalf("single delete failed: %v", err) + } + + err = framework.Poll(time.Second, 20*time.Second, func() error { + promRule, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(ns).Get( + ctx, "e2e-delete-single-pr", metav1.GetOptions{}, + ) + if err != nil { + return err + } + var remaining []string + for _, group := range promRule.Spec.Groups { + for _, rule := range group.Rules { + remaining = append(remaining, rule.Alert) + } + } + if len(remaining) != 1 || remaining[0] != "KeepSingleAlert" { + return fmt.Errorf("expected only KeepSingleAlert, got %v", remaining) + } + return nil + }) + if err != nil { + t.Fatalf("prometheusrule state after single delete: %v", err) + } +} + +// TestRBAC_DeleteAlertRule_Single mirrors TestRBAC_DeleteAlertRule against +// DELETE /rules/{ruleId}. +func TestRBAC_DeleteAlertRule_Single(t *testing.T) { + f, err := framework.New() + if err != nil { + t.Fatalf("Failed to create framework: %v", err) + } + ctx := context.Background() + + nsY, cleanupY, err := f.CreateUserNamespace(ctx, "test-rbac-del1-y") + if err != nil { + t.Fatalf("Failed to create namespace Y: %v", err) + } + defer func() { _ = cleanupY() }() + + nsZ, cleanupZ, err := f.CreateUserNamespace(ctx, "test-rbac-del1-z") + if err != nil { + t.Fatalf("Failed to create namespace Z: %v", err) + } + defer func() { _ = cleanupZ() }() + + anonymousUser, err := f.CreateAnonymousUser(ctx, "e2e-rbac-del1-a", "default") + if err != nil { + t.Fatalf("Failed to create anonymous user: %v", err) + } + defer func() { _ = anonymousUser.Cleanup() }() + + scopedUser, err := f.CreateScopedUser(ctx, "e2e-rbac-del1-b", nsY, + "monitoring.coreos.com", []string{"prometheusrules"}, []string{"get", "create", "update", "patch", "delete"}) + if err != nil { + t.Fatalf("Failed to create scoped user: %v", err) + } + defer func() { _ = scopedUser.Cleanup() }() + + ruleInY := mustCreateRule(ctx, t, f, nsY, "RBACDel1AlertY", "e2e-rbac-del1-pr") + ruleInZ := mustCreateRule(ctx, t, f, nsZ, "RBACDel1AlertZ", "e2e-rbac-del1-pr") + ruleInY2 := mustCreateRule(ctx, t, f, nsY, "RBACDel1AlertY2", "e2e-rbac-del1-pr") + + for _, ruleID := range []string{ruleInY, ruleInY2, ruleInZ} { + waitForSingleDeleteCacheSync(ctx, t, f, anonymousUser.Token, ruleID) + } + + cases := []struct { + name string + token string + ruleID string + wantStatus int + }{ + {"AnonymousUser_DeniedNamespaceY", anonymousUser.Token, ruleInY, http.StatusForbidden}, + {"AnonymousUser_DeniedNamespaceZ", anonymousUser.Token, ruleInZ, http.StatusForbidden}, + {"ScopedUser_SucceedsNamespaceY", scopedUser.Token, ruleInY, http.StatusNoContent}, + {"ScopedUser_DeniedNamespaceZ", scopedUser.Token, ruleInZ, http.StatusForbidden}, + {"ClusterAdmin_SucceedsNamespaceZ", f.BearerToken, ruleInZ, http.StatusNoContent}, + {"ClusterAdmin_SucceedsNamespaceY", f.BearerToken, ruleInY2, http.StatusNoContent}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + status := deleteAlertRuleSingleWithToken(ctx, t, f, tc.token, tc.ruleID) + if status != tc.wantStatus { + t.Fatalf("Expected HTTP status %d, got %d", tc.wantStatus, status) + } + }) + } +} + +func patchDropSingle(ctx context.Context, t *testing.T, f *framework.Framework, ruleID string, enable bool) { + t.Helper() + status, err := tryUpdateAlertRuleSingle(ctx, f, f.BearerToken, ruleID, managementrouter.UpdateAlertRuleRequest{ + AlertingRuleEnabled: &enable, + }) + if err != nil { + t.Fatalf("single drop/restore request failed: %v", err) + } + if status != http.StatusOK { + t.Fatalf("single drop/restore: expected HTTP 200, got %d", status) + } +} + +func waitForSingleUpdateCacheSync(ctx context.Context, t *testing.T, f *framework.Framework, token, ruleID string) { + t.Helper() + err := framework.Poll(time.Second, 30*time.Second, func() error { + status, err := tryUpdateAlertRuleSingle(ctx, f, token, ruleID, probeLabelUpdateRequest()) + if err != nil { + return err + } + if status == http.StatusForbidden || status == http.StatusOK { + return nil + } + return fmt.Errorf("HTTP status %d, waiting for cache sync", status) + }) + if err != nil { + t.Fatalf("single-update cache sync timed out for %s: %v", ruleID, err) + } +} + +func waitForSingleDeleteCacheSync(ctx context.Context, t *testing.T, f *framework.Framework, token, ruleID string) { + t.Helper() + err := framework.Poll(time.Second, 30*time.Second, func() error { + status, err := tryDeleteAlertRuleSingle(ctx, f, token, ruleID) + if err != nil { + return err + } + if status == http.StatusForbidden { + return nil + } + return fmt.Errorf("HTTP status %d, waiting for cache sync", status) + }) + if err != nil { + t.Fatalf("single-delete cache sync timed out for %s: %v", ruleID, err) + } +} + +func probeLabelUpdateRequest() managementrouter.UpdateAlertRuleRequest { + labelVal := "true" + return managementrouter.UpdateAlertRuleRequest{ + Labels: &map[string]*string{"e2e_rbac_probe": &labelVal}, + } +} + +func tryUpdateAlertRuleSingle( + ctx context.Context, + f *framework.Framework, + token, ruleID string, + payload managementrouter.UpdateAlertRuleRequest, +) (int, error) { + reqBody, err := json.Marshal(payload) + if err != nil { + return 0, fmt.Errorf("marshal update request: %w", err) + } + updateURL, err := url.JoinPath(f.PluginURL, "api/v1/alerting/rules", ruleID) + if err != nil { + return 0, fmt.Errorf("build URL: %w", err) + } + req, err := http.NewRequestWithContext(ctx, http.MethodPatch, updateURL, bytes.NewBuffer(reqBody)) + if err != nil { + return 0, fmt.Errorf("create HTTP request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := f.HTTPClient().Do(req) + if err != nil { + return 0, fmt.Errorf("make update request: %w", err) + } + defer resp.Body.Close() + _, _ = io.ReadAll(resp.Body) + return resp.StatusCode, nil +} + +func updateAlertRuleSingleWithToken(ctx context.Context, t *testing.T, f *framework.Framework, token, ruleID string) int { + t.Helper() + status, err := tryUpdateAlertRuleSingle(ctx, f, token, ruleID, probeLabelUpdateRequest()) + if err != nil { + t.Fatalf("single update for rule %s failed: %v", ruleID, err) + } + return status +} + +func tryDeleteAlertRuleSingle(ctx context.Context, f *framework.Framework, token, ruleID string) (int, error) { + deleteURL, err := url.JoinPath(f.PluginURL, "api/v1/alerting/rules", ruleID) + if err != nil { + return 0, fmt.Errorf("build URL: %w", err) + } + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, deleteURL, nil) + if err != nil { + return 0, fmt.Errorf("create HTTP request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+token) + + resp, err := f.HTTPClient().Do(req) + if err != nil { + return 0, fmt.Errorf("make delete request: %w", err) + } + defer resp.Body.Close() + _, _ = io.ReadAll(resp.Body) + return resp.StatusCode, nil +} + +func deleteAlertRuleSingleWithToken(ctx context.Context, t *testing.T, f *framework.Framework, token, ruleID string) int { + t.Helper() + status, err := tryDeleteAlertRuleSingle(ctx, f, token, ruleID) + if err != nil { + t.Fatalf("single delete for rule %s failed: %v", ruleID, err) + } + return status +}