From c3867e96e7a6bb4950846f48d5a5d2e3620f6703 Mon Sep 17 00:00:00 2001 From: Shirly Radco Date: Sun, 19 Jul 2026 12:54:46 +0300 Subject: [PATCH 1/2] management: add update alert rule APIs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add PATCH /api/v1/alerting/rules for bulk update of platform and user-defined alert rules with drop/restore, label overrides, and per-rule update support. Refactor: introduce UpdateAlertRuleLabels unified method that routes internally to platform (ARC) or user-defined (PR mutation) paths, replacing the error-sniffing fallback pattern in the HTTP handler. Extend drop/restore to user-defined rules via ARC. Rename DropPlatformAlertRule and RestorePlatformAlertRule to DropAlertRule and RestoreAlertRule. Add validateDropRestorePreconditions that checks the PrometheusRule and AlertingRule CR for GitOps management while still allowing drops on operator-managed rules (their whole purpose). Reject requests that combine alertingRuleEnabled with labels or classification — these are mutually exclusive operations to prevent partial-apply inconsistencies. Fix user label updates to read source labels from the PrometheusRule directly instead of the relabeled cache, preventing ARC overlay values from being baked into the PR source. Fixes: - Propagate errors from cleanupARCForDeletedRule instead of swallowing them (nilerr lint) - Preserve rule Drops when clearing the last label override (prevent silent restore) - Validate ARC ownership on fallback restore path (block GitOps-managed ARC modification) - Return errors from findARCByAlertRuleID instead of silently continuing - Use t.Fatalf in get_rule_by_id_test to prevent nil deref on type assertion failure - Use httptest.NewRequestWithContext (noctx) - Block user-rule edits when parent PrometheusRule is GitOps-managed - Allow ARC fallback for operator-managed rules (the ARC is a separate resource not reconciled by the operator) Signed-off-by: Shirly Radco Co-authored-by: AI Assistant --- api/openapi.yaml | 135 +++ docs/alert-management.md | 19 + docs/alert-rule-classification.md | 36 +- .../alert_rule_bulk_update.go | 154 ++++ .../alert_rule_bulk_update_test.go | 577 ++++++++++++ internal/managementrouter/api_generated.go | 58 ++ internal/managementrouter/router.go | 5 +- pkg/k8s/const.go | 3 +- pkg/management/alert_rule_preconditions.go | 82 ++ pkg/management/client_factory.go | 5 +- .../delete_user_defined_alert_rule_by_id.go | 64 +- ...lete_user_defined_alert_rule_by_id_test.go | 66 ++ pkg/management/get_rule_by_id.go | 18 + pkg/management/get_rule_by_id_test.go | 377 ++++++++ pkg/management/label_utils.go | 16 +- pkg/management/management.go | 3 +- pkg/management/types.go | 25 + pkg/management/update_alert_rule_labels.go | 85 ++ pkg/management/update_classification.go | 6 +- pkg/management/update_classification_test.go | 78 +- pkg/management/update_platform_alert_rule.go | 468 ++++++++++ .../update_platform_alert_rule_test.go | 870 ++++++++++++++++++ .../update_user_defined_alert_rule.go | 144 +++ .../update_user_defined_alert_rule_test.go | 408 ++++++++ test/e2e/helpers_test.go | 63 ++ test/e2e/update_alert_rule_test.go | 395 ++++++++ 26 files changed, 4045 insertions(+), 115 deletions(-) create mode 100644 internal/managementrouter/alert_rule_bulk_update.go create mode 100644 internal/managementrouter/alert_rule_bulk_update_test.go create mode 100644 pkg/management/get_rule_by_id.go create mode 100644 pkg/management/get_rule_by_id_test.go create mode 100644 pkg/management/update_alert_rule_labels.go create mode 100644 pkg/management/update_platform_alert_rule.go create mode 100644 pkg/management/update_platform_alert_rule_test.go create mode 100644 pkg/management/update_user_defined_alert_rule.go create mode 100644 pkg/management/update_user_defined_alert_rule_test.go create mode 100644 test/e2e/update_alert_rule_test.go diff --git a/api/openapi.yaml b/api/openapi.yaml index eaa95cf5a..317e933d5 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -12,6 +12,52 @@ servers: paths: /rules: + patch: + operationId: BulkUpdateAlertRules + summary: Bulk update alert rules + description: > + Updates one or more alert rules by their stable IDs. Each rule is + updated independently; per-rule status is returned in the response + so partial success is visible to the caller. + Supports label overrides, drop/restore toggles (platform rules only), + and classification label updates. + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/BulkUpdateAlertRulesRequest" + responses: + "200": + description: Update results (may include per-rule errors) + content: + application/json: + schema: + $ref: "#/components/schemas/BulkUpdateAlertRulesResponse" + "400": + description: Invalid request body + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorResponse" + "401": + description: Missing or invalid authorization token + 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: BulkDeleteUserDefinedAlertRules summary: Bulk delete user-defined alert rules @@ -219,6 +265,95 @@ components: $ref: "#/components/schemas/DeleteAlertRuleResult" description: Per-rule deletion results. + AlertRuleClassificationUpdate: + type: object + description: > + 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. + x-go-type: AlertRuleClassificationPatch + properties: + openshift_io_alert_rule_component: + type: string + nullable: true + description: Component classification label override. + openshift_io_alert_rule_layer: + type: string + nullable: true + description: Layer classification label override. + openshift_io_alert_rule_component_from: + type: string + nullable: true + description: Dynamic component source label key. + openshift_io_alert_rule_layer_from: + type: string + nullable: true + description: Dynamic layer source label key. + + BulkUpdateAlertRulesRequest: + type: object + required: + - ruleIds + properties: + ruleIds: + type: array + minItems: 1 + maxItems: 100 + items: + type: string + description: List of stable alert rule IDs to update (at most 100 per request). + 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: + - id + - statusCode + properties: + id: + type: string + description: The stable alert rule ID that was processed. + statusCode: + type: integer + format: int32 + minimum: 100 + maximum: 599 + description: HTTP status code for this rule's update result. + message: + type: string + description: Error message if update failed; omitted on success. + + BulkUpdateAlertRulesResponse: + type: object + required: + - rules + properties: + rules: + type: array + items: + $ref: "#/components/schemas/UpdateAlertRuleResult" + description: Per-rule update results. + ErrorResponse: type: object required: diff --git a/docs/alert-management.md b/docs/alert-management.md index 1ca39abf9..0951ca9b6 100644 --- a/docs/alert-management.md +++ b/docs/alert-management.md @@ -39,3 +39,22 @@ OpenShift supports routing user workload alerts to: This is a cluster configuration choice and does not change the plugin API shape. The plugin reads alerts from Alertmanager (for firing/silenced) and Prometheus (for pending), then merges platform and user workload results when available. 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 user-defined alert rules + +| Rule ownership | Editable? | Classification? | Drop/Restore? | +|---|---|---|---| +| User-owned | Yes (direct PR mutation) | Yes (set labels directly) | No (ARC not supported) | +| Operator-managed | No (reconciled) | No | No | +| GitOps-managed | No (reconciled) | No | No | + +**User-owned** rules can be fully edited (labels, severity, expr, annotations) +via the update API, which mutates the PrometheusRule directly. + +**Operator-managed** and **GitOps-managed** user-defined rules cannot be edited +because the owning controller would reconcile the change. These alerts can only +be **silenced** via Alertmanager silences. + +ARC-based operations (classification overrides, drop/restore) are not available +for any user-defined rule because the user workload stack does not process +AlertRelabelConfigs. If this capability is needed, open an RFE against CMO. diff --git a/docs/alert-rule-classification.md b/docs/alert-rule-classification.md index 9114b71d0..04141707f 100644 --- a/docs/alert-rule-classification.md +++ b/docs/alert-rule-classification.md @@ -5,7 +5,7 @@ The backend classifies Prometheus alerting rules into a "component" and an "impa - Computes an `openshift_io_alert_rule_id` per alerting rule. - Determines component/layer based on matcher logic and rule labels. - Allows operator-managed classification overrides via AlertRelabelConfigs (ARCs) for platform - rules. Operator-managed classification overrides of user-defined workload rules require the `ENABLE_USER_WORKLOAD_ARCS` feature flag. + rules. - Enriches the Alerts API response with `openshift_io_alert_rule_id`, `openshift_io_alert_component`, and `openshift_io_alert_layer`. This document explains how it works, how to override, and how to test it. @@ -111,19 +111,22 @@ management status — this endpoint never writes directly to `AlertingRule` CRs. (Other management endpoints, such as severity updates, may write to unmanaged `AlertingRule` CRs directly, but classification is ARC-only.) -### User-defined workload rules → blocked by default, ARC when enabled +### User-defined workload rules -Classification updates for operator-managed user-defined workload rules are **not -allowed by default**. The API returns a `NotAllowedError` when the feature flag is -disabled. +For **user-owned** rules, classification labels can be set directly on the rule +(via `UpdateAlertRuleLabels` or `UpdateUserDefinedAlertRule`) — the same way any +other label is updated. This mutates the PrometheusRule directly without needing +ARCs. -### Feature flag: `ENABLE_USER_WORKLOAD_ARCS` +The ARC-based `UpdateAlertRuleClassification` endpoint returns `NotAllowedError` +for user-defined rules because CMO does not process AlertRelabelConfigs in the +user workload stack. -Setting the environment variable `ENABLE_USER_WORKLOAD_ARCS=true` enables full -alert management for operator-managed user-defined workload rules, including -classification overrides, label updates, and rule disable/enable (Drop/Restore). -When enabled, these rules use the same ARC-based path as platform rules, with -ARCs stored in the `openshift-user-workload-monitoring` namespace. +**Operator-managed** user-defined rules cannot be edited at all (the operator +would reconcile the change). Those alerts can only be **silenced** via +Alertmanager silences. If ARC-based management for these rules is needed, please +open an RFE against CMO to add AlertRelabelConfig support in the user workload +stack. ### Dynamic classification (`_from` labels) @@ -223,8 +226,6 @@ APIs: Direct K8s (supported for power users/GitOps): - For platform rules: create or update the `AlertRelabelConfig` CR in `openshift-monitoring` with the appropriate relabel configs (respect `resourceVersion` for optimistic concurrency). -- For user-defined rules (requires `ENABLE_USER_WORKLOAD_ARCS=true`): create or update the - `AlertRelabelConfig` CR in `openshift-user-workload-monitoring`. - UI should check update permissions with SelfSubjectAccessReview before showing an editor. Notes: @@ -235,16 +236,15 @@ Notes: Clients that need to update both should issue two requests. The combined operation is not atomic. ## Security Notes -- Classification overrides are stored in AlertRelabelConfig CRs (`openshift-monitoring` - for platform rules, `openshift-user-workload-monitoring` for user-defined rules when - enabled), subject to standard Kubernetes RBAC. +- Classification overrides are stored in AlertRelabelConfig CRs in `openshift-monitoring`, + subject to standard Kubernetes RBAC. - No secrets or sensitive data are persisted in classification metadata. ## Testing and Ops Unit tests: - `pkg/management/update_classification_test.go` - - ARC-based classification for platform rules, blocked-by-default for user-defined - rules, ARC in user-workload namespace when flag enabled, dynamic `_from` label resolution. + - ARC-based classification for platform rules, not-allowed for user-defined + rules, dynamic `_from` label resolution. - `pkg/management/get_alerts_test.go` - Alert enrichment with classification labels, `_from` label behavior, fallback behavior. diff --git a/internal/managementrouter/alert_rule_bulk_update.go b/internal/managementrouter/alert_rule_bulk_update.go new file mode 100644 index 000000000..1042bf10b --- /dev/null +++ b/internal/managementrouter/alert_rule_bulk_update.go @@ -0,0 +1,154 @@ +package managementrouter + +import ( + "encoding/json" + "io" + "net/http" + "strings" + + "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 + } + 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") + 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, + StatusCode: int32(http.StatusBadRequest), + Message: &msg, + }) + 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 + } + results = append(results, UpdateAlertRuleResult{ + Id: id, + StatusCode: int32(http.StatusNoContent), + }) + 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, + 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") + } +} diff --git a/internal/managementrouter/alert_rule_bulk_update_test.go b/internal/managementrouter/alert_rule_bulk_update_test.go new file mode 100644 index 000000000..e805f6c59 --- /dev/null +++ b/internal/managementrouter/alert_rule_bulk_update_test.go @@ -0,0 +1,577 @@ +package managementrouter_test + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "regexp" + "strings" + "testing" + + osmv1 "github.com/openshift/api/monitoring/v1" + 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" + "github.com/openshift/monitoring-plugin/pkg/management/testutils" +) + +// buFixture holds all mocks and the router under test for bulk-update tests. +// Mutate mockK8s fields, then call rebuild() before the next request. +type buFixture struct { + router http.Handler + mockK8sRules *testutils.MockPrometheusRuleInterface + mockK8s *testutils.MockClient + mockRelabeledRules *testutils.MockRelabeledRulesInterface +} + +func (f *buFixture) rebuild() { + mgmt := management.New(context.Background(), f.mockK8s) + f.router = managementrouter.New(mgmt) +} + +func newBUFixture(t *testing.T) *buFixture { + t.Helper() + + userRule1 := monitoringv1.Rule{ + Alert: "user-alert-1", + Expr: intstr.FromString("up == 0"), + Labels: map[string]string{"severity": "warning"}, + } + userRule1Id := alertrule.GetAlertingRuleId(&userRule1) + + userRule2 := monitoringv1.Rule{ + Alert: "user-alert-2", + Expr: intstr.FromString("cpu > 80"), + Labels: map[string]string{"severity": "info"}, + } + userRule2Id := alertrule.GetAlertingRuleId(&userRule2) + + platformRule := monitoringv1.Rule{ + Alert: "platform-alert", + Expr: intstr.FromString("memory > 90"), + Labels: map[string]string{"severity": "critical"}, + } + platformRuleId := alertrule.GetAlertingRuleId(&platformRule) + + mockK8sRules := &testutils.MockPrometheusRuleInterface{} + + userPR := monitoringv1.PrometheusRule{} + userPR.Name = "user-pr" + userPR.Namespace = "default" + userPR.Spec.Groups = []monitoringv1.RuleGroup{{ + Name: "g1", + Rules: []monitoringv1.Rule{ + {Alert: userRule1.Alert, Expr: userRule1.Expr, Labels: map[string]string{"severity": "warning", k8s.AlertRuleLabelId: userRule1Id}}, + {Alert: userRule2.Alert, Expr: userRule2.Expr, Labels: map[string]string{"severity": "info", k8s.AlertRuleLabelId: userRule2Id}}, + }, + }} + + platformPR := monitoringv1.PrometheusRule{} + platformPR.Name = "platform-pr" + platformPR.Namespace = "platform-namespace-1" + platformPR.Spec.Groups = []monitoringv1.RuleGroup{{ + Name: "pg1", + Rules: []monitoringv1.Rule{ + {Alert: "platform-alert", Expr: intstr.FromString("memory > 90"), Labels: map[string]string{"severity": "critical"}}, + }, + }} + + mockK8sRules.SetPrometheusRules(map[string]*monitoringv1.PrometheusRule{ + "default/user-pr": &userPR, + "platform-namespace-1/platform-pr": &platformPR, + }) + + mockRelabeledRules := &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + switch id { + case userRule1Id: + return monitoringv1.Rule{ + Alert: userRule1.Alert, Expr: userRule1.Expr, + Labels: map[string]string{ + "severity": "warning", k8s.AlertRuleLabelId: userRule1Id, + k8s.PrometheusRuleLabelNamespace: "default", k8s.PrometheusRuleLabelName: "user-pr", + }, + }, true + case userRule2Id: + return monitoringv1.Rule{ + Alert: userRule2.Alert, Expr: userRule2.Expr, + Labels: map[string]string{ + "severity": "info", k8s.AlertRuleLabelId: userRule2Id, + k8s.PrometheusRuleLabelNamespace: "default", k8s.PrometheusRuleLabelName: "user-pr", + }, + }, true + case platformRuleId: + return monitoringv1.Rule{ + Alert: "platform-alert", Expr: intstr.FromString("memory > 90"), + Labels: map[string]string{ + "severity": "critical", k8s.AlertRuleLabelId: platformRuleId, + k8s.PrometheusRuleLabelNamespace: "platform-namespace-1", k8s.PrometheusRuleLabelName: "platform-pr", + }, + }, true + } + return monitoringv1.Rule{}, false + }, + } + + mockK8s := &testutils.MockClient{ + PrometheusRulesFunc: func() k8s.PrometheusRuleInterface { return mockK8sRules }, + NamespaceFunc: func() k8s.NamespaceInterface { + return &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(name string) bool { + return name == "platform-namespace-1" || name == "platform-namespace-2" + }, + } + }, + RelabeledRulesFunc: func() k8s.RelabeledRulesInterface { return mockRelabeledRules }, + } + + f := &buFixture{ + mockK8sRules: mockK8sRules, + mockK8s: mockK8s, + mockRelabeledRules: mockRelabeledRules, + } + f.rebuild() + return f +} + +// ids returns stable rule IDs for the three default fixture rules in order: +// user1, user2, platform. +func buFixtureIDs() (user1, user2, platform string) { + r1 := monitoringv1.Rule{Alert: "user-alert-1", Expr: intstr.FromString("up == 0"), Labels: map[string]string{"severity": "warning"}} + r2 := monitoringv1.Rule{Alert: "user-alert-2", Expr: intstr.FromString("cpu > 80"), Labels: map[string]string{"severity": "info"}} + rp := monitoringv1.Rule{Alert: "platform-alert", Expr: intstr.FromString("memory > 90"), Labels: map[string]string{"severity": "critical"}} + return alertrule.GetAlertingRuleId(&r1), alertrule.GetAlertingRuleId(&r2), alertrule.GetAlertingRuleId(&rp) +} + +func (f *buFixture) do(t *testing.T, 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", bytes.NewReader(buf)) + req.Header.Set("Authorization", "Bearer test-token") + w := httptest.NewRecorder() + f.router.ServeHTTP(w, req) + return w +} + +func (f *buFixture) decodeResp(t *testing.T, w *httptest.ResponseRecorder) managementrouter.BulkUpdateAlertRulesResponse { + t.Helper() + var resp managementrouter.BulkUpdateAlertRulesResponse + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + return resp +} + +// --- Tests --- + +func TestBulkUpdateAlertRules_UpdatesAllUserRules(t *testing.T) { + user1Id, user2Id, _ := buFixtureIDs() + f := newBUFixture(t) + + expectedId1 := alertrule.GetAlertingRuleId(&monitoringv1.Rule{ + Alert: "user-alert-1", Expr: intstr.FromString("up == 0"), + Labels: map[string]string{"severity": "warning", "component": "api", "team": "backend"}, + }) + expectedId2 := alertrule.GetAlertingRuleId(&monitoringv1.Rule{ + Alert: "user-alert-2", Expr: intstr.FromString("cpu > 80"), + Labels: map[string]string{"severity": "info", "component": "api", "team": "backend"}, + }) + + w := f.do(t, map[string]any{ + "ruleIds": []string{user1Id, user2Id}, + "labels": map[string]string{"component": "api", "team": "backend"}, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 2 { + t.Fatalf("expected 2 rules, got %d", len(resp.Rules)) + } + if resp.Rules[0].Id != expectedId1 || resp.Rules[0].StatusCode != http.StatusNoContent { + t.Errorf("rule[0]: id=%s status=%d", resp.Rules[0].Id, resp.Rules[0].StatusCode) + } + if resp.Rules[1].Id != expectedId2 || resp.Rules[1].StatusCode != http.StatusNoContent { + t.Errorf("rule[1]: id=%s status=%d", resp.Rules[1].Id, resp.Rules[1].StatusCode) + } +} + +func TestBulkUpdateAlertRules_DropsLabelWithEmptyString(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": "critical"}, + }) + + f.mockRelabeledRules.GetFunc = func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == user1Id { + return monitoringv1.Rule{ + Alert: "user-alert-1", Expr: intstr.FromString("up == 0"), + Labels: map[string]string{ + "severity": "warning", "team": "backend", + k8s.AlertRuleLabelId: user1Id, k8s.PrometheusRuleLabelNamespace: "default", k8s.PrometheusRuleLabelName: "user-pr", + }, + }, true + } + return monitoringv1.Rule{}, false + } + f.rebuild() + + w := f.do(t, map[string]any{ + "ruleIds": []string{user1Id}, + "labels": map[string]string{"team": "", "severity": "critical"}, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 1 || resp.Rules[0].Id != expectedId || resp.Rules[0].StatusCode != http.StatusNoContent { + t.Errorf("unexpected result: %+v", resp.Rules) + } +} + +func TestBulkUpdateAlertRules_DropsLabelWithNull(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + + // JSON null for a label key means "drop", same as empty string. + expectedId := alertrule.GetAlertingRuleId(&monitoringv1.Rule{ + Alert: "user-alert-1", Expr: intstr.FromString("up == 0"), + Labels: map[string]string{"severity": "critical"}, + }) + + f.mockRelabeledRules.GetFunc = func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == user1Id { + return monitoringv1.Rule{ + Alert: "user-alert-1", Expr: intstr.FromString("up == 0"), + Labels: map[string]string{ + "severity": "warning", "team": "backend", + k8s.AlertRuleLabelId: user1Id, k8s.PrometheusRuleLabelNamespace: "default", k8s.PrometheusRuleLabelName: "user-pr", + }, + }, true + } + return monitoringv1.Rule{}, false + } + f.rebuild() + + // Send {"team": null, "severity": "critical"} — null drops the label. + body := map[string]any{ + "ruleIds": []string{user1Id}, + "labels": map[string]any{"team": nil, "severity": "critical"}, + } + w := f.do(t, body) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 1 || resp.Rules[0].Id != expectedId || resp.Rules[0].StatusCode != http.StatusNoContent { + t.Errorf("unexpected result: %+v", resp.Rules) + } +} + +func TestBulkUpdateAlertRules_MixedPlatformAndUserRules(t *testing.T) { + user1Id, _, platformId := buFixtureIDs() + f := newBUFixture(t) + + expectedId1 := alertrule.GetAlertingRuleId(&monitoringv1.Rule{ + Alert: "user-alert-1", Expr: intstr.FromString("up == 0"), + Labels: map[string]string{"severity": "warning", "component": "api"}, + }) + + f.mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{} + } + f.rebuild() + + w := f.do(t, map[string]any{ + "ruleIds": []string{user1Id, platformId}, + "labels": map[string]string{"component": "api"}, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 2 { + t.Fatalf("expected 2 rules, got %d", len(resp.Rules)) + } + if resp.Rules[0].Id != expectedId1 || resp.Rules[0].StatusCode != http.StatusNoContent { + t.Errorf("rule[0]: id=%s status=%d", resp.Rules[0].Id, resp.Rules[0].StatusCode) + } + if resp.Rules[1].Id != platformId || resp.Rules[1].StatusCode != http.StatusNoContent { + t.Errorf("rule[1]: id=%s status=%d", resp.Rules[1].Id, resp.Rules[1].StatusCode) + } +} + +func TestBulkUpdateAlertRules_InvalidBody(t *testing.T) { + f := newBUFixture(t) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPatch, "/api/v1/alerting/rules", bytes.NewBufferString("{")) + req.Header.Set("Authorization", "Bearer test-token") + w := httptest.NewRecorder() + f.router.ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d", w.Code) + } + if !strings.Contains(w.Body.String(), "invalid request body") { + t.Errorf("expected 'invalid request body', got: %s", w.Body) + } +} + +func TestBulkUpdateAlertRules_BodyTooLarge(t *testing.T) { + f := newBUFixture(t) + // Build a body larger than maxRequestBodyBytes (1 MB). + large := make([]byte, 1<<20+1) + for i := range large { + large[i] = 'a' + } + req := httptest.NewRequestWithContext(context.Background(), http.MethodPatch, "/api/v1/alerting/rules", bytes.NewReader(large)) + req.Header.Set("Authorization", "Bearer test-token") + w := httptest.NewRecorder() + f.router.ServeHTTP(w, req) + + if w.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("expected 413, got %d: %s", w.Code, w.Body) + } + if !strings.Contains(w.Body.String(), "request body too large") { + t.Errorf("expected 'request body too large', got: %s", w.Body) + } +} + +func TestBulkUpdateAlertRules_EmptyRuleIds(t *testing.T) { + f := newBUFixture(t) + w := f.do(t, map[string]any{ + "ruleIds": []string{}, + "labels": map[string]string{"component": "api"}, + }) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d", w.Code) + } + if !strings.Contains(w.Body.String(), "ruleIds is required") { + t.Errorf("expected 'ruleIds is required', got: %s", w.Body) + } +} + +func TestBulkUpdateAlertRules_TooManyRuleIds(t *testing.T) { + f := newBUFixture(t) + ids := make([]string, 101) + for i := range ids { + ids[i] = "rule-id" + } + w := f.do(t, map[string]any{ + "ruleIds": ids, + "labels": map[string]string{"component": "api"}, + }) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d", w.Code) + } + if !strings.Contains(w.Body.String(), "ruleIds exceeds maximum of 100") { + t.Errorf("expected 'ruleIds exceeds maximum of 100', got: %s", w.Body) + } +} + +func TestBulkUpdateAlertRules_MissingAllUpdateFields(t *testing.T) { + user1Id, _, _ := buFixtureIDs() + f := newBUFixture(t) + w := f.do(t, map[string]any{"ruleIds": []string{user1Id}}) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d", w.Code) + } + if !strings.Contains(w.Body.String(), "alertingRuleEnabled") { + t.Errorf("expected 'alertingRuleEnabled' in message, got: %s", w.Body) + } +} + +func TestBulkUpdateAlertRules_EnabledToggle(t *testing.T) { + user1Id, _, platformId := buFixtureIDs() + f := newBUFixture(t) + + f.mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{} + } + f.rebuild() + + w := f.do(t, map[string]any{ + "ruleIds": []string{platformId, user1Id, "rid_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + "alertingRuleEnabled": false, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 3 { + t.Fatalf("expected 3 rules, got %d", len(resp.Rules)) + } + if resp.Rules[0].Id != platformId || resp.Rules[0].StatusCode != http.StatusNoContent { + t.Errorf("platform[0]: id=%s status=%d", resp.Rules[0].Id, resp.Rules[0].StatusCode) + } + if resp.Rules[1].Id != user1Id || resp.Rules[1].StatusCode != http.StatusMethodNotAllowed { + t.Errorf("user[1]: expected 405, got id=%s status=%d", resp.Rules[1].Id, resp.Rules[1].StatusCode) + } + if resp.Rules[2].StatusCode != http.StatusNotFound { + t.Errorf("missing[2]: expected 404, got status=%d", resp.Rules[2].StatusCode) + } +} + +func TestBulkUpdateAlertRules_MixedNotFound(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"}, + }) + + f.mockRelabeledRules.GetFunc = func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == user1Id { + return monitoringv1.Rule{ + Alert: "user-alert-1", Expr: intstr.FromString("up == 0"), + Labels: map[string]string{ + "severity": "warning", k8s.AlertRuleLabelId: user1Id, + k8s.PrometheusRuleLabelNamespace: "default", k8s.PrometheusRuleLabelName: "user-pr", + }, + }, true + } + return monitoringv1.Rule{}, false + } + f.rebuild() + + w := f.do(t, map[string]any{ + "ruleIds": []string{user1Id, "rid_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + "labels": map[string]string{"component": "api"}, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 2 { + t.Fatalf("expected 2 rules, got %d", len(resp.Rules)) + } + if resp.Rules[0].Id != expectedId || resp.Rules[0].StatusCode != http.StatusNoContent { + t.Errorf("rule[0]: id=%s status=%d", resp.Rules[0].Id, resp.Rules[0].StatusCode) + } + if resp.Rules[1].StatusCode != http.StatusNotFound { + t.Errorf("rule[1]: expected 404, got %d", resp.Rules[1].StatusCode) + } +} + +func TestBulkUpdateAlertRules_InvalidRuleId(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.do(t, map[string]any{ + "ruleIds": []string{user1Id, ""}, + "labels": map[string]string{"component": "api"}, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 2 { + t.Fatalf("expected 2 rules, got %d", len(resp.Rules)) + } + if resp.Rules[0].Id != expectedId || resp.Rules[0].StatusCode != http.StatusNoContent { + t.Errorf("rule[0]: id=%s status=%d", resp.Rules[0].Id, resp.Rules[0].StatusCode) + } + if resp.Rules[1].StatusCode != http.StatusBadRequest { + t.Errorf("rule[1]: expected 400, got %d", resp.Rules[1].StatusCode) + } + if resp.Rules[1].Message == nil || !strings.Contains(*resp.Rules[1].Message, "ruleId is empty") { + t.Errorf("rule[1]: expected 'ruleId is empty', got %v", resp.Rules[1].Message) + } +} + +func TestBulkUpdateAlertRules_RestoreToggle(t *testing.T) { + _, _, platformId := buFixtureIDs() + f := newBUFixture(t) + + // Simulate an existing ARC that holds a Drop config for the platform rule. + // RestoreAlertRule will call AlertRelabelConfigs().Get() and then + // Delete() the ARC once the Drop entry is removed (stamp-only ARC gets deleted). + mockARC := &testutils.MockAlertRelabelConfigInterface{} + mockARC.GetFunc = func(_ context.Context, namespace, name string) (*osmv1.AlertRelabelConfig, bool, error) { + arc := &osmv1.AlertRelabelConfig{} + arc.Namespace = namespace + arc.Name = name + arc.Spec.Configs = []osmv1.RelabelConfig{ + { + SourceLabels: []osmv1.LabelName{"openshift_io_alert_rule_id"}, + Regex: regexp.QuoteMeta(platformId), + Action: "Drop", + }, + } + return arc, true, nil + } + f.mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { return mockARC } + f.rebuild() + + w := f.do(t, map[string]any{ + "ruleIds": []string{platformId}, + "alertingRuleEnabled": true, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 1 { + t.Fatalf("expected 1 rule, got %d", len(resp.Rules)) + } + if resp.Rules[0].Id != platformId || resp.Rules[0].StatusCode != http.StatusNoContent { + t.Errorf("restore: id=%s status=%d", resp.Rules[0].Id, resp.Rules[0].StatusCode) + } +} + +func TestBulkUpdateAlertRules_ClassificationUserRulesNotAllowed(t *testing.T) { + user1Id, user2Id, _ := buFixtureIDs() + + f := newBUFixture(t) + f.mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{} + } + f.rebuild() + + w := f.do(t, map[string]any{ + "ruleIds": []string{user1Id, user2Id}, + "classification": map[string]any{ + "openshift_io_alert_rule_component": "team-x", + "openshift_io_alert_rule_layer": "namespace", + }, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body) + } + resp := f.decodeResp(t, w) + if len(resp.Rules) != 2 { + t.Fatalf("expected 2 rules, got %d", len(resp.Rules)) + } + if resp.Rules[0].StatusCode != http.StatusMethodNotAllowed || resp.Rules[1].StatusCode != http.StatusMethodNotAllowed { + t.Errorf("expected both rules 405, got %d / %d", resp.Rules[0].StatusCode, resp.Rules[1].StatusCode) + } +} diff --git a/internal/managementrouter/api_generated.go b/internal/managementrouter/api_generated.go index 35d149d7a..60496c281 100644 --- a/internal/managementrouter/api_generated.go +++ b/internal/managementrouter/api_generated.go @@ -10,6 +10,9 @@ import ( "github.com/gorilla/mux" ) +// AlertRuleClassificationUpdate 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. +type AlertRuleClassificationUpdate = AlertRuleClassificationPatch + // AlertRuleSpec Specification of a Prometheus alerting or recording rule. Maps to prometheus-operator Rule fields. type AlertRuleSpec struct { // Alert Name of the alert. Must be set for alerting rules. @@ -46,6 +49,27 @@ type BulkDeleteAlertRulesResponse struct { Rules []DeleteAlertRuleResult `json:"rules"` } +// 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 *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"` + + // RuleIds List of stable alert rule IDs to update. + RuleIds []string `json:"ruleIds"` +} + +// BulkUpdateAlertRulesResponse defines model for BulkUpdateAlertRulesResponse. +type BulkUpdateAlertRulesResponse struct { + // Rules Per-rule update results. + Rules []UpdateAlertRuleResult `json:"rules"` +} + // CreateAlertRuleRequest defines model for CreateAlertRuleRequest. type CreateAlertRuleRequest struct { // AlertingRule Specification of a Prometheus alerting or recording rule. Maps to prometheus-operator Rule fields. @@ -91,9 +115,24 @@ type PrometheusRuleTarget struct { PrometheusRuleNamespace string `json:"prometheusRuleNamespace"` } +// UpdateAlertRuleResult defines model for UpdateAlertRuleResult. +type UpdateAlertRuleResult struct { + // Id The stable alert rule ID that was processed. + Id string `json:"id"` + + // Message Error message if update failed; omitted on success. + Message *string `json:"message,omitempty"` + + // StatusCode HTTP status code for this rule's update result. + StatusCode int32 `json:"statusCode"` +} + // BulkDeleteUserDefinedAlertRulesJSONRequestBody defines body for BulkDeleteUserDefinedAlertRules for application/json ContentType. type BulkDeleteUserDefinedAlertRulesJSONRequestBody = BulkDeleteAlertRulesRequest +// BulkUpdateAlertRulesJSONRequestBody defines body for BulkUpdateAlertRules for application/json ContentType. +type BulkUpdateAlertRulesJSONRequestBody = BulkUpdateAlertRulesRequest + // CreateAlertRuleJSONRequestBody defines body for CreateAlertRule for application/json ContentType. type CreateAlertRuleJSONRequestBody = CreateAlertRuleRequest @@ -102,6 +141,9 @@ type ServerInterface interface { // Bulk delete user-defined alert rules // (DELETE /rules) BulkDeleteUserDefinedAlertRules(w http.ResponseWriter, r *http.Request) + // Bulk update alert rules + // (PATCH /rules) + BulkUpdateAlertRules(w http.ResponseWriter, r *http.Request) // Create an alert rule // (POST /rules) CreateAlertRule(w http.ResponseWriter, r *http.Request) @@ -130,6 +172,20 @@ func (siw *ServerInterfaceWrapper) BulkDeleteUserDefinedAlertRules(w http.Respon handler.ServeHTTP(w, r) } +// 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) + })) + + for _, middleware := range siw.HandlerMiddlewares { + handler = middleware(handler) + } + + handler.ServeHTTP(w, r) +} + // CreateAlertRule operation middleware func (siw *ServerInterfaceWrapper) CreateAlertRule(w http.ResponseWriter, r *http.Request) { @@ -259,6 +315,8 @@ func HandlerWithOptions(si ServerInterface, options GorillaServerOptions) http.H r.HandleFunc(options.BaseURL+"/rules", wrapper.BulkDeleteUserDefinedAlertRules).Methods("DELETE") + r.HandleFunc(options.BaseURL+"/rules", wrapper.BulkUpdateAlertRules).Methods("PATCH") + r.HandleFunc(options.BaseURL+"/rules", wrapper.CreateAlertRule).Methods("POST") return r diff --git a/internal/managementrouter/router.go b/internal/managementrouter/router.go index 43a8ad352..fd0e0cda7 100644 --- a/internal/managementrouter/router.go +++ b/internal/managementrouter/router.go @@ -22,7 +22,10 @@ var log = logrus.WithField("module", "managementrouter") // maxRequestBodyBytes limits incoming request bodies to 1 MB across all handlers. const maxRequestBodyBytes = 1 << 20 // 1 MB -const maxBulkDeleteRuleIds = 100 +const ( + maxBulkDeleteRuleIds = 100 + maxBulkUpdateRuleIds = 100 +) type httpRouter struct { managementClient management.Client diff --git a/pkg/k8s/const.go b/pkg/k8s/const.go index 699dc452e..243cea8d8 100644 --- a/pkg/k8s/const.go +++ b/pkg/k8s/const.go @@ -1,6 +1,5 @@ package k8s const ( - ClusterMonitoringNamespace = "openshift-monitoring" - UserWorkloadMonitoringNamespace = "openshift-user-workload-monitoring" + ClusterMonitoringNamespace = "openshift-monitoring" ) diff --git a/pkg/management/alert_rule_preconditions.go b/pkg/management/alert_rule_preconditions.go index 3e730156c..a91fb002c 100644 --- a/pkg/management/alert_rule_preconditions.go +++ b/pkg/management/alert_rule_preconditions.go @@ -8,9 +8,15 @@ import ( "github.com/openshift/monitoring-plugin/pkg/managementlabels" ) +func notAllowedGitOpsEdit() error { + return &NotAllowedError{Message: "This alert is managed by GitOps; edit it in Git."} +} func notAllowedGitOpsRemove() error { return &NotAllowedError{Message: "This alert is managed by GitOps; remove it in Git."} } +func notAllowedOperatorUpdate() error { + return &NotAllowedError{Message: "This alert is managed by an operator; it can't be updated and can only be silenced."} +} func notAllowedOperatorDelete() error { return &NotAllowedError{Message: "This alert is managed by an operator; it can't be deleted and can only be silenced."} } @@ -36,6 +42,23 @@ func validateUserDeletePreconditions(relabeled monitoringv1.Rule) error { return nil } +func validateUserUpdatePreconditions(relabeled monitoringv1.Rule, pr *monitoringv1.PrometheusRule) error { + if isRuleManagedByGitOpsLabel(relabeled) { + return notAllowedGitOpsEdit() + } + if isRuleManagedByOperator(relabeled) { + return notAllowedOperatorUpdate() + } + if pr != nil { + if gitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(pr); gitOpsManaged { + return notAllowedGitOpsEdit() + } else if operatorManaged { + return notAllowedOperatorUpdate() + } + } + return nil +} + func validatePlatformDeletePreconditions(ar *osmv1.AlertingRule) error { if ar != nil { if gitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(ar); gitOpsManaged { @@ -46,3 +69,62 @@ func validatePlatformDeletePreconditions(ar *osmv1.AlertingRule) error { } return nil } + +// validateGitOpsPreconditions checks only GitOps-related constraints on the +// rule and its parent PrometheusRule. Used by UpdatePlatformAlertRule before +// the ARC is fetched — operator-managed rules are allowed to proceed because +// the ARC path handles them. +func validateGitOpsPreconditions(relabeled monitoringv1.Rule, pr *monitoringv1.PrometheusRule) error { + if isRuleManagedByGitOpsLabel(relabeled) { + return notAllowedGitOpsEdit() + } + if pr != nil { + if gitOpsManaged, _ := k8s.IsExternallyManagedObject(pr); gitOpsManaged { + return notAllowedGitOpsEdit() + } + } + return nil +} + +func validatePlatformUpdatePreconditions(relabeled monitoringv1.Rule, pr *monitoringv1.PrometheusRule, arc *osmv1.AlertRelabelConfig) error { + if err := validateGitOpsPreconditions(relabeled, pr); err != nil { + return err + } + // Operator-managed rules are intentionally allowed to reach this point. + // The ARC path modifies a separate AlertRelabelConfig resource, not the + // operator-managed PrometheusRule/AlertingRule, so the operator won't + // reconcile away changes. Only the ARC itself is checked. + if arc != nil { + if gitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(arc); gitOpsManaged { + return notAllowedGitOpsEdit() + } else if operatorManaged { + return notAllowedOperatorUpdate() + } + } + return nil +} + +// validateDropRestorePreconditions checks whether a drop/restore operation is +// allowed. Unlike label updates, drops ARE permitted for operator-managed rules +// (that's the mechanism to suppress them). Only GitOps management blocks drops. +func validateDropRestorePreconditions(relabeled monitoringv1.Rule, pr *monitoringv1.PrometheusRule, ar *osmv1.AlertingRule, arc *osmv1.AlertRelabelConfig) error { + if isRuleManagedByGitOpsLabel(relabeled) { + return notAllowedGitOpsEdit() + } + if pr != nil { + if gitOpsManaged, _ := k8s.IsExternallyManagedObject(pr); gitOpsManaged { + return notAllowedGitOpsEdit() + } + } + if ar != nil { + if gitOpsManaged, _ := k8s.IsExternallyManagedObject(ar); gitOpsManaged { + return notAllowedGitOpsEdit() + } + } + if arc != nil { + if gitOpsManaged, _ := k8s.IsExternallyManagedObject(arc); gitOpsManaged { + return notAllowedGitOpsEdit() + } + } + return nil +} diff --git a/pkg/management/client_factory.go b/pkg/management/client_factory.go index de5b940ce..e71b7f93b 100644 --- a/pkg/management/client_factory.go +++ b/pkg/management/client_factory.go @@ -2,8 +2,6 @@ package management import ( "context" - "os" - "strings" "github.com/openshift/monitoring-plugin/pkg/k8s" ) @@ -11,7 +9,6 @@ import ( // New creates a new management client. func New(ctx context.Context, k8sClient k8s.Client) Client { return &client{ - k8sClient: k8sClient, - enableUserWorkloadARCs: strings.EqualFold(strings.TrimSpace(os.Getenv("ENABLE_USER_WORKLOAD_ARCS")), "true"), + k8sClient: k8sClient, } } diff --git a/pkg/management/delete_user_defined_alert_rule_by_id.go b/pkg/management/delete_user_defined_alert_rule_by_id.go index 27714d755..6a2a9a68a 100644 --- a/pkg/management/delete_user_defined_alert_rule_by_id.go +++ b/pkg/management/delete_user_defined_alert_rule_by_id.go @@ -2,6 +2,7 @@ package management import ( "context" + "errors" "fmt" osmv1 "github.com/openshift/api/monitoring/v1" @@ -64,7 +65,7 @@ func (c *client) deletePlatformAlertRuleById(ctx context.Context, relabeled moni return fmt.Errorf("failed to get AlertingRule %s: %w", arName, err) } if !found || ar == nil { - return &NotFoundError{Resource: "AlertingRule", Id: arName} + return c.deleteUserAlertRuleById(ctx, namespace, name, alertRuleId) } // Common preconditions for platform delete if err := validatePlatformDeletePreconditions(ar); err != nil { @@ -85,9 +86,20 @@ func (c *client) deletePlatformAlertRuleById(ctx context.Context, relabeled moni AdditionalInfo: fmt.Sprintf("alert %q not found in AlertingRule %s", originalRule.Alert, arName), } } - ar.Spec.Groups = newGroups - if err := c.k8sClient.AlertingRules().Update(ctx, *ar); err != nil { - return fmt.Errorf("failed to update AlertingRule %s: %w", ar.Name, err) + + if len(newGroups) == 0 { + if err := c.k8sClient.AlertingRules().Delete(ctx, ar.Name); err != nil { + return fmt.Errorf("failed to delete AlertingRule %s: %w", ar.Name, err) + } + } else { + ar.Spec.Groups = newGroups + if err := c.k8sClient.AlertingRules().Update(ctx, *ar); err != nil { + return fmt.Errorf("failed to update AlertingRule %s: %w", ar.Name, err) + } + } + + if err := c.deleteAssociatedARC(ctx, k8s.ClusterMonitoringNamespace, name, alertRuleId); err != nil { + return fmt.Errorf("failed to clean up ARC for platform rule %s: %w", alertRuleId, err) } return nil } @@ -101,6 +113,13 @@ func (c *client) deleteUserAlertRuleById(ctx context.Context, namespace, name, a if !found { return &NotFoundError{Resource: "PrometheusRule", Id: fmt.Sprintf("%s/%s", namespace, name)} } + gitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(pr) + if gitOpsManaged { + return notAllowedGitOpsRemove() + } + if operatorManaged { + return notAllowedOperatorDelete() + } updated := false var newGroups []monitoringv1.RuleGroup @@ -119,14 +138,47 @@ func (c *client) deleteUserAlertRuleById(ctx context.Context, namespace, name, a if err := c.k8sClient.PrometheusRules().Delete(ctx, pr.Namespace, pr.Name); err != nil { return fmt.Errorf("failed to delete PrometheusRule %s/%s: %w", pr.Namespace, pr.Name, err) } - return nil + return c.cleanupARCForDeletedRule(ctx, namespace, name, alertRuleId) } pr.Spec.Groups = newGroups if err := c.k8sClient.PrometheusRules().Update(ctx, *pr); err != nil { return fmt.Errorf("failed to update PrometheusRule %s/%s: %w", pr.Namespace, pr.Name, err) } - return nil + return c.cleanupARCForDeletedRule(ctx, namespace, name, alertRuleId) +} + +// cleanupARCForDeletedRule attempts to remove any associated ARC after a rule is deleted. +// It determines the ARC namespace from the rule's namespace and silently skips if +// ARCs are not applicable (e.g. user-defined rules where ARCs are not supported). +func (c *client) cleanupARCForDeletedRule(ctx context.Context, namespace, name, alertRuleId string) error { + nn := types.NamespacedName{Namespace: namespace, Name: name} + arcNamespace, err := c.arcNamespaceForRule(nn) + if err != nil { + var na *NotAllowedError + if errors.As(err, &na) { + return nil + } + return fmt.Errorf("failed to resolve ARC namespace for %s/%s: %w", namespace, name, err) + } + return c.deleteAssociatedARC(ctx, arcNamespace, name, alertRuleId) +} + +// deleteAssociatedARC removes the AlertRelabelConfig associated with an alert rule, if it exists. +// This is best-effort: if the ARC does not exist or is GitOps-managed, it is silently skipped. +func (c *client) deleteAssociatedARC(ctx context.Context, namespace, prName, alertRuleId string) error { + arcName := k8s.GetAlertRelabelConfigName(prName, alertRuleId) + arc, found, err := c.k8sClient.AlertRelabelConfigs().Get(ctx, namespace, arcName) + if err != nil { + return fmt.Errorf("failed to get AlertRelabelConfig %s/%s: %w", namespace, arcName, err) + } + if !found { + return nil + } + if gitOpsManaged, _ := k8s.IsExternallyManagedObject(arc); gitOpsManaged { + return nil + } + return c.k8sClient.AlertRelabelConfigs().Delete(ctx, namespace, arcName) } // removeAlertFromAlertingRuleGroups removes all instances of an alert by alert name across groups. diff --git a/pkg/management/delete_user_defined_alert_rule_by_id_test.go b/pkg/management/delete_user_defined_alert_rule_by_id_test.go index 1eba05a4d..795f8931e 100644 --- a/pkg/management/delete_user_defined_alert_rule_by_id_test.go +++ b/pkg/management/delete_user_defined_alert_rule_by_id_test.go @@ -242,6 +242,72 @@ func TestDeleteAlertRuleById_PlatformRuleOperatorManaged(t *testing.T) { } } +func TestDeleteAlertRuleById_PlatformFallbackRejectsOperatorManagedPR(t *testing.T) { + mockK8s := &testutils.MockClient{} + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == deletePlatformRuleId { + return deletePlatformRule, true + } + return monitoringv1.Rule{}, false + }, + } + } + mockK8s.NamespaceFunc = func() k8s.NamespaceInterface { + return &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(name string) bool { return name == "openshift-monitoring" }, + } + } + controller := true + var updatedPR bool + var deletedPR bool + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, namespace, name string) (*monitoringv1.PrometheusRule, bool, error) { + return &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + OwnerReferences: []metav1.OwnerReference{ + {Kind: "ClusterOperator", Name: "monitoring", Controller: &controller}, + }, + }, + Spec: monitoringv1.PrometheusRuleSpec{ + Groups: []monitoringv1.RuleGroup{{Name: "grp", Rules: []monitoringv1.Rule{deletePlatformRule}}}, + }, + }, true, nil + }, + UpdateFunc: func(_ context.Context, _ monitoringv1.PrometheusRule) error { + updatedPR = true + return nil + }, + DeleteFunc: func(_ context.Context, _, _ string) error { + deletedPR = true + return nil + }, + } + } + mockK8s.AlertingRulesFunc = func() k8s.AlertingRuleInterface { + return &testutils.MockAlertingRuleInterface{ + GetFunc: func(_ context.Context, _ string) (*osmv1.AlertingRule, bool, error) { + return nil, false, nil + }, + } + } + + err := newDeleteClient(mockK8s).DeleteAlertRuleById(context.Background(), deletePlatformRuleId) + if err == nil { + t.Fatal("expected error for operator-managed PrometheusRule fallback") + } + if !errors.As(err, new(*management.NotAllowedError)) { + t.Errorf("expected NotAllowedError, got %T: %v", err, err) + } + if updatedPR || deletedPR { + t.Error("operator-managed PrometheusRule must not be mutated when AlertingRule is absent") + } +} + func TestDeleteAlertRuleById_PrometheusRuleNotFound(t *testing.T) { mockK8s := &testutils.MockClient{} mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { diff --git a/pkg/management/get_rule_by_id.go b/pkg/management/get_rule_by_id.go new file mode 100644 index 000000000..212fb5e92 --- /dev/null +++ b/pkg/management/get_rule_by_id.go @@ -0,0 +1,18 @@ +package management + +import ( + "context" + + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" +) + +// GetRuleById retrieves a specific alert rule by its ID from the relabeled +// rules cache, returning a NotFoundError when the rule is not present. +func (c *client) GetRuleById(ctx context.Context, alertRuleId string) (monitoringv1.Rule, error) { + rule, found := c.k8sClient.RelabeledRules().Get(ctx, alertRuleId) + if !found { + return monitoringv1.Rule{}, &NotFoundError{Resource: "AlertRule", Id: alertRuleId} + } + + return rule, nil +} diff --git a/pkg/management/get_rule_by_id_test.go b/pkg/management/get_rule_by_id_test.go new file mode 100644 index 000000000..36b38236b --- /dev/null +++ b/pkg/management/get_rule_by_id_test.go @@ -0,0 +1,377 @@ +package management_test + +import ( + "context" + "errors" + "maps" + "testing" + + osmv1 "github.com/openshift/api/monitoring/v1" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + + 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" +) + +var ( + grTestRule = monitoringv1.Rule{ + Alert: "TestAlert", + Expr: intstr.FromString("up == 0"), + Labels: map[string]string{ + "severity": "warning", + k8s.PrometheusRuleLabelNamespace: "test-namespace", + k8s.PrometheusRuleLabelName: "test-rule", + }, + } + grTestRuleId = alertrule.GetAlertingRuleId(&grTestRule) +) + +func newGetRuleClient(t *testing.T) (management.Client, *testutils.MockClient) { + t.Helper() + mockK8s := &testutils.MockClient{} + return management.New(context.Background(), mockK8s), mockK8s +} + +func TestGetRuleById_Found(t *testing.T) { + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == grTestRuleId { + return grTestRule, true + } + return monitoringv1.Rule{}, false + }, + } + } + + rule, err := client.GetRuleById(context.Background(), grTestRuleId) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule.Alert != "TestAlert" { + t.Errorf("expected alert %q, got %q", "TestAlert", rule.Alert) + } + if rule.Labels["severity"] != "warning" { + t.Errorf("expected severity %q, got %q", "warning", rule.Labels["severity"]) + } +} + +func TestGetRuleById_NotFound(t *testing.T) { + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, _ string) (monitoringv1.Rule, bool) { + return monitoringv1.Rule{}, false + }, + } + } + + _, err := client.GetRuleById(context.Background(), "nonexistent-id") + if err == nil { + t.Fatal("expected NotFoundError") + } + var nf *management.NotFoundError + if !errors.As(err, &nf) { + t.Fatalf("expected NotFoundError, got %T: %v", err, err) + } + if nf.Resource != "AlertRule" { + t.Errorf("expected Resource %q, got %q", "AlertRule", nf.Resource) + } + if nf.Id != "nonexistent-id" { + t.Errorf("expected Id %q, got %q", "nonexistent-id", nf.Id) + } +} + +func TestGetRuleById_MultipleRules(t *testing.T) { + rule1 := monitoringv1.Rule{Alert: "Alert1", Expr: intstr.FromString("up == 0")} + rule1Id := alertrule.GetAlertingRuleId(&rule1) + rule2 := monitoringv1.Rule{Alert: "Alert2", Expr: intstr.FromString("down == 1")} + rule2Id := alertrule.GetAlertingRuleId(&rule2) + + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + switch id { + case rule1Id: + return rule1, true + case rule2Id: + return rule2, true + } + return monitoringv1.Rule{}, false + }, + } + } + + r1, err := client.GetRuleById(context.Background(), rule1Id) + if err != nil || r1.Alert != "Alert1" { + t.Errorf("rule1: got alert=%q err=%v", r1.Alert, err) + } + r2, err := client.GetRuleById(context.Background(), rule2Id) + if err != nil || r2.Alert != "Alert2" { + t.Errorf("rule2: got alert=%q err=%v", r2.Alert, err) + } +} + +func TestGetRuleById_RecordingRule(t *testing.T) { + recRule := monitoringv1.Rule{ + Record: "job:request_latency_seconds:mean5m", + Expr: intstr.FromString("avg by (job) (request_latency_seconds)"), + } + recRuleId := alertrule.GetAlertingRuleId(&recRule) + + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == recRuleId { + return recRule, true + } + return monitoringv1.Rule{}, false + }, + } + } + + rule, err := client.GetRuleById(context.Background(), recRuleId) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule.Record != "job:request_latency_seconds:mean5m" { + t.Errorf("expected record name, got %q", rule.Record) + } +} + +func buildRuleWithManagedBy(base monitoringv1.Rule, ruleId string, prName, prNS string, + ruleManagedBy, relabelManagedBy string) monitoringv1.Rule { + r := base + r.Labels = maps.Clone(base.Labels) + if r.Labels == nil { + r.Labels = make(map[string]string) + } + r.Labels[managementlabels.AlertNameLabel] = r.Alert + r.Labels[k8s.AlertRuleLabelId] = ruleId + r.Labels[k8s.PrometheusRuleLabelNamespace] = prNS + r.Labels[k8s.PrometheusRuleLabelName] = prName + if ruleManagedBy != "" { + r.Labels[managementlabels.RuleManagedByLabel] = ruleManagedBy + } + if relabelManagedBy != "" { + r.Labels[managementlabels.RelabelConfigManagedByLabel] = relabelManagedBy + } + return r +} + +func TestGetRuleById_OperatorManagedByLabel(t *testing.T) { + ctx := context.Background() + mockARC := &testutils.MockAlertRelabelConfigInterface{} + mockNS := &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(_ string) bool { return false }, + } + + promRule := &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "operator-rule", Namespace: "test-namespace", + OwnerReferences: []metav1.OwnerReference{ + {APIVersion: "apps/v1", Kind: "Deployment", Name: "test-operator", UID: "test-uid"}, + }, + }, + } + ruleManagedBy, relabelManagedBy := k8s.DetermineManagedBy(ctx, mockARC, mockNS, promRule, grTestRuleId) + ruleWithLabel := buildRuleWithManagedBy(grTestRule, grTestRuleId, promRule.Name, promRule.Namespace, ruleManagedBy, relabelManagedBy) + + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == grTestRuleId { + return ruleWithLabel, true + } + return monitoringv1.Rule{}, false + }, + } + } + + rule, err := client.GetRuleById(context.Background(), grTestRuleId) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule.Labels[managementlabels.RuleManagedByLabel] != "operator" { + t.Errorf("expected managed_by=operator, got %q", rule.Labels[managementlabels.RuleManagedByLabel]) + } +} + +func TestGetRuleById_NoManagedByLabelForNormalRule(t *testing.T) { + ctx := context.Background() + mockARC := &testutils.MockAlertRelabelConfigInterface{} + mockNS := &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(_ string) bool { return false }, + } + + promRule := &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{Name: "local-rule", Namespace: "test-namespace"}, + } + ruleManagedBy, relabelManagedBy := k8s.DetermineManagedBy(ctx, mockARC, mockNS, promRule, grTestRuleId) + ruleWithLabel := buildRuleWithManagedBy(grTestRule, grTestRuleId, promRule.Name, promRule.Namespace, ruleManagedBy, relabelManagedBy) + + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == grTestRuleId { + return ruleWithLabel, true + } + return monitoringv1.Rule{}, false + }, + } + } + + rule, err := client.GetRuleById(context.Background(), grTestRuleId) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if _, ok := rule.Labels[managementlabels.RuleManagedByLabel]; ok { + t.Error("expected no managed_by label for normal rule") + } +} + +func TestGetRuleById_RelabelConfigGitOpsManagedBy(t *testing.T) { + ctx := context.Background() + mockARC := &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, ns, name string) (*osmv1.AlertRelabelConfig, bool, error) { + return &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, Namespace: ns, + Annotations: map[string]string{"argocd.argoproj.io/tracking-id": "test-id"}, + }, + }, true, nil + }, + } + mockNS := &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(_ string) bool { return true }, + } + + promRule := &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "platform-rule", Namespace: "openshift-monitoring", + OwnerReferences: []metav1.OwnerReference{ + {APIVersion: "apps/v1", Kind: "Deployment", Name: "test-operator", UID: "test-uid"}, + }, + }, + } + ruleManagedBy, relabelManagedBy := k8s.DetermineManagedBy(ctx, mockARC, mockNS, promRule, grTestRuleId) + ruleWithLabel := buildRuleWithManagedBy(grTestRule, grTestRuleId, promRule.Name, promRule.Namespace, ruleManagedBy, relabelManagedBy) + + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == grTestRuleId { + return ruleWithLabel, true + } + return monitoringv1.Rule{}, false + }, + } + } + + rule, err := client.GetRuleById(context.Background(), grTestRuleId) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule.Labels[managementlabels.RuleManagedByLabel] != "operator" { + t.Errorf("expected managed_by=operator, got %q", rule.Labels[managementlabels.RuleManagedByLabel]) + } + if rule.Labels[managementlabels.RelabelConfigManagedByLabel] != "gitops" { + t.Errorf("expected relabel_config_managed_by=gitops, got %q", rule.Labels[managementlabels.RelabelConfigManagedByLabel]) + } +} + +func TestGetRuleById_GitOpsManagedByLabel(t *testing.T) { + ctx := context.Background() + mockARC := &testutils.MockAlertRelabelConfigInterface{} + mockNS := &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(_ string) bool { return true }, + } + + promRule := &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "platform-rule", Namespace: "openshift-monitoring", + Annotations: map[string]string{"argocd.argoproj.io/tracking-id": "test-id"}, + }, + } + ruleManagedBy, relabelManagedBy := k8s.DetermineManagedBy(ctx, mockARC, mockNS, promRule, grTestRuleId) + ruleWithLabel := buildRuleWithManagedBy(grTestRule, grTestRuleId, promRule.Name, promRule.Namespace, ruleManagedBy, relabelManagedBy) + + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == grTestRuleId { + return ruleWithLabel, true + } + return monitoringv1.Rule{}, false + }, + } + } + + rule, err := client.GetRuleById(context.Background(), grTestRuleId) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule.Labels[managementlabels.RuleManagedByLabel] != "gitops" { + t.Errorf("expected managed_by=gitops, got %q", rule.Labels[managementlabels.RuleManagedByLabel]) + } +} + +func TestGetRuleById_NoRelabelConfigManagedByWhenNotGitOps(t *testing.T) { + ctx := context.Background() + mockARC := &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, ns, name string) (*osmv1.AlertRelabelConfig, bool, error) { + return &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + }, true, nil + }, + } + mockNS := &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(_ string) bool { return true }, + } + + promRule := &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "platform-rule", Namespace: "openshift-monitoring", + OwnerReferences: []metav1.OwnerReference{ + {APIVersion: "apps/v1", Kind: "Deployment", Name: "test-operator", UID: "test-uid"}, + }, + }, + } + ruleManagedBy, relabelManagedBy := k8s.DetermineManagedBy(ctx, mockARC, mockNS, promRule, grTestRuleId) + ruleWithLabel := buildRuleWithManagedBy(grTestRule, grTestRuleId, promRule.Name, promRule.Namespace, ruleManagedBy, relabelManagedBy) + + client, mockK8s := newGetRuleClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == grTestRuleId { + return ruleWithLabel, true + } + return monitoringv1.Rule{}, false + }, + } + } + + rule, err := client.GetRuleById(context.Background(), grTestRuleId) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule.Labels[managementlabels.RuleManagedByLabel] != "operator" { + t.Errorf("expected managed_by=operator, got %q", rule.Labels[managementlabels.RuleManagedByLabel]) + } + if _, ok := rule.Labels[managementlabels.RelabelConfigManagedByLabel]; ok { + t.Error("expected no relabel_config_managed_by label when ARC not GitOps-managed") + } +} diff --git a/pkg/management/label_utils.go b/pkg/management/label_utils.go index a4d4bb2df..3c3d6c211 100644 --- a/pkg/management/label_utils.go +++ b/pkg/management/label_utils.go @@ -1,6 +1,20 @@ package management -import "strings" +import ( + "strings" + + "github.com/openshift/monitoring-plugin/pkg/k8s" + "github.com/openshift/monitoring-plugin/pkg/managementlabels" +) + +var protectedLabels = map[string]bool{ + managementlabels.AlertNameLabel: true, + k8s.AlertRuleLabelId: true, +} + +func isProtectedLabel(label string) bool { + return protectedLabels[label] +} var validSeverities = map[string]bool{ "critical": true, diff --git a/pkg/management/management.go b/pkg/management/management.go index b7eec3c09..652ac14de 100644 --- a/pkg/management/management.go +++ b/pkg/management/management.go @@ -7,8 +7,7 @@ import ( ) type client struct { - k8sClient k8s.Client - enableUserWorkloadARCs bool + k8sClient k8s.Client } // isPlatformManagedPrometheusRule returns true when the target diff --git a/pkg/management/types.go b/pkg/management/types.go index 0d235fdec..e150832c8 100644 --- a/pkg/management/types.go +++ b/pkg/management/types.go @@ -8,15 +8,40 @@ import ( // Client is the interface for managing alert rules type Client interface { + // GetRuleById retrieves a specific alert rule by its ID + GetRuleById(ctx context.Context, alertRuleId string) (monitoringv1.Rule, error) + // CreateUserDefinedAlertRule creates a new user-defined alert rule CreateUserDefinedAlertRule(ctx context.Context, alertRule monitoringv1.Rule, prOptions PrometheusRuleOptions) (alertRuleId string, err error) + // UpdateUserDefinedAlertRule updates an existing user-defined alert rule by its ID + // Returns the new rule ID after the update + UpdateUserDefinedAlertRule(ctx context.Context, alertRuleId string, alertRule monitoringv1.Rule) (newRuleId string, err error) + + // UpdateAlertRuleLabels updates labels on any alert rule (platform or user-defined), + // routing to the appropriate path internally. A nil value or a pointer to an empty + // string signals "drop this label"; a pointer to a non-empty string sets the value. + // Returns the (possibly new) rule ID. + UpdateAlertRuleLabels(ctx context.Context, alertRuleId string, labels map[string]*string) (newRuleId string, err error) + // DeleteAlertRuleById deletes an alert rule by its ID (user-defined or platform). DeleteAlertRuleById(ctx context.Context, alertRuleId string) error // CreatePlatformAlertRule creates a new platform alert rule CreatePlatformAlertRule(ctx context.Context, alertRule monitoringv1.Rule) (alertRuleId string, err error) + // UpdatePlatformAlertRule updates an existing platform alert rule by its ID + // Platform alert rules can only have the labels updated through AlertRelabelConfigs + UpdatePlatformAlertRule(ctx context.Context, alertRuleId string, alertRule monitoringv1.Rule) error + + // DropAlertRule hides an alert rule by adding a scoped Drop relabel entry via ARC. + // Only supported for platform alert rules. + DropAlertRule(ctx context.Context, alertRuleId string) error + + // RestoreAlertRule restores a previously dropped alert rule by removing its Drop relabel entry. + // Only supported for platform alert rules. + RestoreAlertRule(ctx context.Context, alertRuleId string) error + // UpdateAlertRuleClassification updates component/layer for a single alert rule id UpdateAlertRuleClassification(ctx context.Context, req UpdateRuleClassificationRequest) error // BulkUpdateAlertRuleClassification updates classification for multiple rule ids diff --git a/pkg/management/update_alert_rule_labels.go b/pkg/management/update_alert_rule_labels.go new file mode 100644 index 000000000..4ec46dc40 --- /dev/null +++ b/pkg/management/update_alert_rule_labels.go @@ -0,0 +1,85 @@ +package management + +import ( + "context" + + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/openshift/monitoring-plugin/pkg/k8s" +) + +// UpdateAlertRuleLabels updates labels on any alert rule, routing internally +// to the platform (ARC-based) or user-defined (PrometheusRule mutation) path. +// Labels with nil or empty-string values are dropped; non-empty values are set. +func (c *client) UpdateAlertRuleLabels(ctx context.Context, alertRuleId string, labels map[string]*string) (string, error) { + rule, found := c.k8sClient.RelabeledRules().Get(ctx, alertRuleId) + if !found { + return "", &NotFoundError{Resource: "AlertRule", Id: alertRuleId} + } + + namespace := rule.Labels[k8s.PrometheusRuleLabelNamespace] + name := rule.Labels[k8s.PrometheusRuleLabelName] + nn := types.NamespacedName{Namespace: namespace, Name: name} + + if c.isPlatformManagedPrometheusRule(nn) { + return c.updatePlatformRuleLabels(ctx, alertRuleId, labels) + } + + return c.updateUserRuleLabels(ctx, alertRuleId, rule, labels) +} + +// updatePlatformRuleLabels applies label changes to a platform rule via ARC. +// The platform path uses "" to signal "drop this label". +func (c *client) updatePlatformRuleLabels(ctx context.Context, alertRuleId string, labels map[string]*string) (string, error) { + platformLabels := make(map[string]string, len(labels)) + for k, pv := range labels { + if pv == nil || *pv == "" { + platformLabels[k] = "" + } else { + platformLabels[k] = *pv + } + } + + updatedRule := monitoringv1.Rule{Labels: platformLabels} + if err := c.UpdatePlatformAlertRule(ctx, alertRuleId, updatedRule); err != nil { + return "", err + } + return alertRuleId, nil +} + +// updateUserRuleLabels merges label changes onto the source rule (from the +// PrometheusRule, not the relabeled cache) and updates the PrometheusRule directly. +func (c *client) updateUserRuleLabels(ctx context.Context, alertRuleId string, relabeled monitoringv1.Rule, labels map[string]*string) (string, error) { + namespace := relabeled.Labels[k8s.PrometheusRuleLabelNamespace] + name := relabeled.Labels[k8s.PrometheusRuleLabelName] + + pr, prFound, err := c.k8sClient.PrometheusRules().Get(ctx, namespace, name) + if err != nil { + return "", err + } + if !prFound { + return "", &NotFoundError{Resource: "PrometheusRule", Id: alertRuleId} + } + + sourceRule, err := getOriginalPlatformRuleFromPR(pr, namespace, name, alertRuleId) + if err != nil { + return "", err + } + + userLabels := make(map[string]string, len(sourceRule.Labels)) + for k, v := range sourceRule.Labels { + userLabels[k] = v + } + for k, pv := range labels { + if pv == nil || *pv == "" { + delete(userLabels, k) + } else { + userLabels[k] = *pv + } + } + + updatedRule := *sourceRule + updatedRule.Labels = userLabels + return c.UpdateUserDefinedAlertRule(ctx, alertRuleId, updatedRule) +} diff --git a/pkg/management/update_classification.go b/pkg/management/update_classification.go index 1e05cfe32..777fc8846 100644 --- a/pkg/management/update_classification.go +++ b/pkg/management/update_classification.go @@ -81,11 +81,7 @@ func (c *client) UpdateAlertRuleClassification(ctx context.Context, req UpdateRu return c.applyClassificationViaARC(ctx, req.RuleId, rule, labels, k8s.ClusterMonitoringNamespace) } - if !c.enableUserWorkloadARCs { - return &NotAllowedError{Message: "classification updates for user-defined workload rules require ENABLE_USER_WORKLOAD_ARCS"} - } - - return c.applyClassificationViaARC(ctx, req.RuleId, rule, labels, k8s.UserWorkloadMonitoringNamespace) + return &NotAllowedError{Message: "classification updates are only supported for platform alert rules"} } // BulkUpdateAlertRuleClassification updates multiple entries; returns per-item errors collected by caller diff --git a/pkg/management/update_classification_test.go b/pkg/management/update_classification_test.go index 42ed908f3..4a6332a71 100644 --- a/pkg/management/update_classification_test.go +++ b/pkg/management/update_classification_test.go @@ -371,7 +371,7 @@ func TestUpdateAlertRuleClassification_PlatformRule_CreatesARCWithFromLabels(t * // --- User-defined rules --- -func TestUpdateAlertRuleClassification_UserRule_NotAllowedWhenFlagDisabled(t *testing.T) { +func TestUpdateAlertRuleClassification_UserRule_NotAllowed(t *testing.T) { client, mockK8s := newClassificationClient(t) mockK8s.NamespaceFunc = func() k8s.NamespaceInterface { @@ -390,7 +390,7 @@ func TestUpdateAlertRuleClassification_UserRule_NotAllowedWhenFlagDisabled(t *te ComponentSet: true, }) if err == nil { - t.Fatal("expected NotAllowedError when ENABLE_USER_WORKLOAD_ARCS is disabled") + t.Fatal("expected NotAllowedError for user-defined rule classification") } var na *management.NotAllowedError if !errors.As(err, &na) { @@ -398,80 +398,6 @@ func TestUpdateAlertRuleClassification_UserRule_NotAllowedWhenFlagDisabled(t *te } } -func TestUpdateAlertRuleClassification_UserRule_CreatesARCInUserWorkloadNamespace(t *testing.T) { - t.Setenv("ENABLE_USER_WORKLOAD_ARCS", "true") - // Recreate client to pick up the env var. - mockK8s := &testutils.MockClient{} - client := management.New(context.Background(), mockK8s) - - mockK8s.NamespaceFunc = func() k8s.NamespaceInterface { - return &testutils.MockNamespaceInterface{ - MonitoringNamespaces: map[string]bool{clTestPlatformNamespace: true}, - } - } - - relabeled := makeUserRelabeled() - pr := makeClassificationPR(clTestUserNamespace, clTestRuleName, userOriginal) - - mockK8s.RelabeledRulesFunc = mockRelabeledRules(userRuleId, relabeled) - prStore := &testutils.MockPrometheusRuleInterface{ - PrometheusRules: map[string]*monitoringv1.PrometheusRule{ - clTestUserNamespace + "/" + clTestRuleName: pr, - }, - } - mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { return prStore } - - arcStore := &testutils.MockAlertRelabelConfigInterface{} - mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { return arcStore } - - component := "team_a" - layer := "namespace" - if err := client.UpdateAlertRuleClassification(context.Background(), management.UpdateRuleClassificationRequest{ - RuleId: userRuleId, - Component: &component, - ComponentSet: true, - Layer: &layer, - LayerSet: true, - }); err != nil { - t.Fatalf("unexpected error: %v", err) - } - - if len(arcStore.AlertRelabelConfigs) != 1 { - t.Fatalf("expected 1 ARC, got %d", len(arcStore.AlertRelabelConfigs)) - } - - for _, arc := range arcStore.AlertRelabelConfigs { - if arc.Namespace != k8s.UserWorkloadMonitoringNamespace { - t.Errorf("expected ARC namespace %q, got %q", k8s.UserWorkloadMonitoringNamespace, arc.Namespace) - } - if arc.Annotations[managementlabels.ARCAnnotationAlertRuleIDKey] != userRuleId { - t.Errorf("ARC missing expected alert rule ID annotation") - } - - hasComponent, hasLayer := false, false - for _, rc := range arc.Spec.Configs { - if rc.Action == "Replace" && rc.TargetLabel == k8s.AlertRuleClassificationComponentKey { - if rc.Replacement != "team_a" { - t.Errorf("expected component replacement %q, got %q", "team_a", rc.Replacement) - } - hasComponent = true - } - if rc.Action == "Replace" && rc.TargetLabel == k8s.AlertRuleClassificationLayerKey { - if rc.Replacement != "namespace" { - t.Errorf("expected layer replacement %q, got %q", "namespace", rc.Replacement) - } - hasLayer = true - } - } - if !hasComponent { - t.Error("ARC should have component replace config") - } - if !hasLayer { - t.Error("ARC should have layer replace config") - } - } -} - // --- ApplyDynamicClassification tests --- func TestApplyDynamicClassification_DefaultsWhenNoFromLabels(t *testing.T) { diff --git a/pkg/management/update_platform_alert_rule.go b/pkg/management/update_platform_alert_rule.go new file mode 100644 index 000000000..0bb906628 --- /dev/null +++ b/pkg/management/update_platform_alert_rule.go @@ -0,0 +1,468 @@ +package management + +import ( + "context" + "fmt" + "regexp" + + osmv1 "github.com/openshift/api/monitoring/v1" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + "k8s.io/apimachinery/pkg/types" + + "github.com/openshift/monitoring-plugin/pkg/k8s" + "github.com/openshift/monitoring-plugin/pkg/managementlabels" +) + +// arcNamespaceForRule returns the ARC namespace for the given rule's PrometheusRule. +// Platform rules use openshift-monitoring. User-defined workload rules are not +// supported for ARC-based operations because CMO does not process ARCs in the +// user-workload namespace. +func (c *client) arcNamespaceForRule(nn types.NamespacedName) (string, error) { + if c.isPlatformManagedPrometheusRule(nn) { + return k8s.ClusterMonitoringNamespace, nil + } + return "", &NotAllowedError{ + Message: fmt.Sprintf("ARC-based management is only supported for platform alert rules (%s/%s)", nn.Namespace, nn.Name), + } +} + +// UpdatePlatformAlertRule applies label overrides to a platform alert rule via +// its AlertingRule CR or an AlertRelabelConfig. +func (c *client) UpdatePlatformAlertRule(ctx context.Context, alertRuleId string, alertRule monitoringv1.Rule) error { + rule, found := c.k8sClient.RelabeledRules().Get(ctx, alertRuleId) + if !found { + return &NotFoundError{Resource: "AlertRule", Id: alertRuleId} + } + + namespace := rule.Labels[k8s.PrometheusRuleLabelNamespace] + name := rule.Labels[k8s.PrometheusRuleLabelName] + nn := types.NamespacedName{Namespace: namespace, Name: name} + + arcNamespace, err := c.arcNamespaceForRule(nn) + if err != nil { + return err + } + + var prMeta *monitoringv1.PrometheusRule + if pr, found, err := c.k8sClient.PrometheusRules().Get(ctx, namespace, name); err != nil { + return err + } else if found { + prMeta = pr + } + if err := validateGitOpsPreconditions(rule, prMeta); err != nil { + return err + } + + originalRule, err := getOriginalPlatformRuleFromPR(prMeta, namespace, name, alertRuleId) + if err != nil { + 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 + } + ar, arFound, arErr := c.getAlertingRule(ctx, arName) + if arErr != nil { + return arErr + } + if arFound && ar != nil { + if gitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(ar); gitOpsManaged { + return notAllowedGitOpsEdit() + } else if operatorManaged { + return c.applyLabelChangesViaAlertRelabelConfig(ctx, arcNamespace, alertRuleId, *originalRule, alertRule.Labels) + } + return c.updateAlertingRuleLabels(ctx, ar, originalRule.Alert, alertRuleId, alertRule.Labels, arName) + } + + return c.applyLabelChangesViaAlertRelabelConfig(ctx, arcNamespace, alertRuleId, *originalRule, alertRule.Labels) +} + +func filterAndValidatePlatformLabelChanges(labels map[string]string) (map[string]string, error) { + filtered := make(map[string]string) + for k, v := range labels { + if !isProtectedLabel(k) { + filtered[k] = v + } + } + for k, v := range filtered { + if k == managementlabels.AlertNameLabel { + continue + } + if k == "severity" { + if v == "" { + return nil, &NotAllowedError{Message: fmt.Sprintf("label %q cannot be dropped for platform alerts", k)} + } + if !isValidSeverity(v) { + return nil, &ValidationError{Message: fmt.Sprintf("invalid severity %q: must be one of critical|warning|info|none", v)} + } + } + } + return filtered, nil +} + +func (c *client) getAlertingRule(ctx context.Context, name string) (*osmv1.AlertingRule, bool, error) { + ar, found, err := c.k8sClient.AlertingRules().Get(ctx, name) + if err != nil { + return nil, false, fmt.Errorf("failed to get AlertingRule %s: %w", name, err) + } + return ar, found, nil +} + +func (c *client) updateAlertingRuleLabels( + ctx context.Context, + ar *osmv1.AlertingRule, + originalAlertName string, + alertRuleId string, + rawLabels map[string]string, + arName string, +) error { + filteredLabels, err := filterAndValidatePlatformLabelChanges(rawLabels) + if err != nil { + return err + } + target, found := findAlertByNameInAlertingRule(ar, originalAlertName) + if !found || target == nil { + return &NotFoundError{ + Resource: "AlertRule", + Id: alertRuleId, + AdditionalInfo: fmt.Sprintf("alert %q not found in AlertingRule %s", originalAlertName, arName), + } + } + if target.Labels == nil { + target.Labels = map[string]string{} + } + for k, v := range filteredLabels { + if v == "" { + delete(target.Labels, k) + } else { + target.Labels[k] = v + } + } + if err := c.k8sClient.AlertingRules().Update(ctx, *ar); err != nil { + return fmt.Errorf("failed to update AlertingRule %s: %w", ar.Name, err) + } + return nil +} + +func findAlertByNameInAlertingRule(ar *osmv1.AlertingRule, alertName string) (*osmv1.Rule, bool) { + for gi := range ar.Spec.Groups { + for ri := range ar.Spec.Groups[gi].Rules { + r := &ar.Spec.Groups[gi].Rules[ri] + if r.Alert == alertName { + return r, true + } + } + } + return nil, false +} + +func (c *client) applyLabelChangesViaAlertRelabelConfig(ctx context.Context, namespace string, alertRuleId string, originalRule monitoringv1.Rule, rawLabels map[string]string) error { + filtered, err := filterAndValidatePlatformLabelChanges(rawLabels) + if err != nil { + return err + } + relabeled, found := c.k8sClient.RelabeledRules().Get(ctx, alertRuleId) + if !found || relabeled.Labels == nil { + return &NotFoundError{ + Resource: "AlertRule", + Id: alertRuleId, + AdditionalInfo: "relabeled rule not found or has no labels", + } + } + prName := relabeled.Labels[k8s.PrometheusRuleLabelName] + arcName := k8s.GetAlertRelabelConfigName(prName, alertRuleId) + + existingArc, found, err := c.k8sClient.AlertRelabelConfigs().Get(ctx, namespace, arcName) + if err != nil { + return fmt.Errorf("failed to get AlertRelabelConfig %s/%s: %w", namespace, arcName, err) + } + if err := validatePlatformUpdatePreconditions(relabeled, nil, relabelConfigIfFound(found, existingArc)); err != nil { + return err + } + + original := copyStringMap(originalRule.Labels) + existingOverrides, existingDrops := collectExistingFromARC(found, existingArc) + existingRuleDrops := getExistingRuleDrops(existingArc, alertRuleId) + effective := computeEffectiveLabels(original, existingOverrides, existingDrops) + + if len(filtered) == 0 { + return nil + } + + desired := buildDesiredLabels(effective, filtered) + nextChanges := buildNextLabelChanges(original, desired) + + if len(nextChanges) == 0 { + if !found { + return nil + } + if len(existingRuleDrops) > 0 { + relabelConfigs := buildRelabelConfigs(originalRule.Alert, original, alertRuleId, nil) + relabelConfigs = appendPreservedRuleDrops(relabelConfigs, existingRuleDrops) + return upsertAlertRelabelConfig(c.k8sClient, ctx, namespace, arcName, prName, originalRule.Alert, alertRuleId, found, existingArc, relabelConfigs) + } + if err := c.k8sClient.AlertRelabelConfigs().Delete(ctx, namespace, arcName); err != nil { + return fmt.Errorf("failed to delete AlertRelabelConfig %s/%s: %w", namespace, arcName, err) + } + return nil + } + + relabelConfigs := buildRelabelConfigs(originalRule.Alert, original, alertRuleId, nextChanges) + relabelConfigs = appendPreservedRuleDrops(relabelConfigs, existingRuleDrops) + + return upsertAlertRelabelConfig(c.k8sClient, ctx, namespace, arcName, prName, originalRule.Alert, alertRuleId, found, existingArc, relabelConfigs) +} + +func relabelConfigIfFound(found bool, arc *osmv1.AlertRelabelConfig) *osmv1.AlertRelabelConfig { + if found { + return arc + } + return nil +} + +func ensureStampAndDrop(next *[]osmv1.RelabelConfig, stamp osmv1.RelabelConfig, dropCfg osmv1.RelabelConfig, alertRuleId string) bool { + stampExists := false + dropExists := false + for _, rc := range *next { + if rc.Action == "Replace" && rc.TargetLabel == k8s.AlertRuleLabelId && + rc.Regex == stamp.Regex && rc.Replacement == alertRuleId { + stampExists = true + } + if rc.Action == "Drop" && rc.Regex == dropCfg.Regex && + len(rc.SourceLabels) == 1 && rc.SourceLabels[0] == k8s.AlertRuleLabelId { + dropExists = true + } + } + changed := false + if !stampExists { + *next = append(*next, stamp) + changed = true + } + if !dropExists { + *next = append(*next, dropCfg) + changed = true + } + return changed +} + +func filterOutDrop(configs []osmv1.RelabelConfig, alertRuleId string) ([]osmv1.RelabelConfig, bool) { + target := regexp.QuoteMeta(alertRuleId) + var out []osmv1.RelabelConfig + removed := false + for _, rc := range configs { + if rc.Action == "Drop" && (rc.Regex == target || rc.Regex == alertRuleId) { + removed = true + continue + } + out = append(out, rc) + } + return out, removed +} + +func isStampOnly(configs []osmv1.RelabelConfig) bool { + if len(configs) == 0 { + return true + } + for _, rc := range configs { + if rc.Action != "Replace" || rc.TargetLabel != k8s.AlertRuleLabelId { + return false + } + } + return true +} + +// DropAlertRule disables an alert rule by creating an AlertRelabelConfig with a +// Drop action for the given rule ID. Only supported for platform alert rules. +func (c *client) DropAlertRule(ctx context.Context, alertRuleId string) error { + relabeled, found := c.k8sClient.RelabeledRules().Get(ctx, alertRuleId) + if !found || relabeled.Labels == nil { + return &NotFoundError{Resource: "AlertRule", Id: alertRuleId} + } + + namespace := relabeled.Labels[k8s.PrometheusRuleLabelNamespace] + name := relabeled.Labels[k8s.PrometheusRuleLabelName] + nn := types.NamespacedName{Namespace: namespace, Name: name} + + arcNamespace, err := c.arcNamespaceForRule(nn) + if err != nil { + return err + } + + pr, prFound, prErr := c.k8sClient.PrometheusRules().Get(ctx, namespace, name) + if prErr != nil { + return fmt.Errorf("failed to get PrometheusRule %s/%s: %w", namespace, name, prErr) + } + if !prFound { + return &NotFoundError{Resource: "PrometheusRule", Id: alertRuleId} + } + + originalRule, err := getOriginalPlatformRuleFromPR(pr, namespace, name, alertRuleId) + if err != nil { + return err + } + + // Platform-only: arcNamespaceForRule returned successfully above. + arName := relabeled.Labels[managementlabels.AlertingRuleLabelName] + if arName == "" { + arName = defaultAlertingRuleName + } + var ar *osmv1.AlertingRule + if fetched, arFound, arErr := c.getAlertingRule(ctx, arName); arErr != nil { + return arErr + } else if arFound { + ar = fetched + } + + arcName := k8s.GetAlertRelabelConfigName(name, alertRuleId) + + existingArc, arcExists, err := c.k8sClient.AlertRelabelConfigs().Get(ctx, arcNamespace, arcName) + if err != nil { + return fmt.Errorf("failed to get AlertRelabelConfig %s/%s: %w", arcNamespace, arcName, err) + } + if err := validateDropRestorePreconditions(relabeled, pr, ar, relabelConfigIfFound(arcExists, existingArc)); err != nil { + return err + } + + original := copyStringMap(originalRule.Labels) + stampOnly := buildRelabelConfigs(originalRule.Alert, original, alertRuleId, nil) + var stamp osmv1.RelabelConfig + if len(stampOnly) > 0 { + stamp = stampOnly[0] + } + + dropCfg := osmv1.RelabelConfig{ + SourceLabels: []osmv1.LabelName{k8s.AlertRuleLabelId}, + Regex: regexp.QuoteMeta(alertRuleId), + Action: "Drop", + } + + var next []osmv1.RelabelConfig + if arcExists && existingArc != nil { + next = append(next, existingArc.Spec.Configs...) + } + + changed := ensureStampAndDrop(&next, stamp, dropCfg, alertRuleId) + + if !changed { + return nil + } + + return upsertAlertRelabelConfig(c.k8sClient, ctx, arcNamespace, arcName, name, originalRule.Alert, alertRuleId, arcExists, existingArc, next) +} + +// RestoreAlertRule re-enables a previously dropped alert rule by removing its +// Drop action from the AlertRelabelConfig. Only supported for platform alert rules. +func (c *client) RestoreAlertRule(ctx context.Context, alertRuleId string) error { + relabeled, found := c.k8sClient.RelabeledRules().Get(ctx, alertRuleId) + var existingArc *osmv1.AlertRelabelConfig + var arcName string + var arcNamespace string + var err error + if found && relabeled.Labels != nil { + namespace := relabeled.Labels[k8s.PrometheusRuleLabelNamespace] + name := relabeled.Labels[k8s.PrometheusRuleLabelName] + nn := types.NamespacedName{Namespace: namespace, Name: name} + arcNamespace, err = c.arcNamespaceForRule(nn) + if err != nil { + return err + } + arcName = k8s.GetAlertRelabelConfigName(name, alertRuleId) + var arcExists bool + existingArc, arcExists, err = c.k8sClient.AlertRelabelConfigs().Get(ctx, arcNamespace, arcName) + if err != nil { + return fmt.Errorf("failed to get AlertRelabelConfig %s/%s: %w", arcNamespace, arcName, err) + } + if !arcExists || existingArc == nil { + return nil + } + + var pr *monitoringv1.PrometheusRule + if fetched, prFound, prErr := c.k8sClient.PrometheusRules().Get(ctx, namespace, name); prErr != nil { + return fmt.Errorf("failed to get PrometheusRule %s/%s: %w", namespace, name, prErr) + } else if prFound { + pr = fetched + } + + // Platform-only: arcNamespaceForRule returned successfully above. + arName := relabeled.Labels[managementlabels.AlertingRuleLabelName] + if arName == "" { + arName = defaultAlertingRuleName + } + var ar *osmv1.AlertingRule + if fetched, arFound, arErr := c.getAlertingRule(ctx, arName); arErr != nil { + return arErr + } else if arFound { + ar = fetched + } + if err := validateDropRestorePreconditions(relabeled, pr, ar, existingArc); err != nil { + return err + } + } else { + // Dropped rules may not appear in the relabeled rules cache because + // the Drop action suppresses them from Prometheus results. Fall back + // to scanning ARCs by annotation to locate the one to restore. + var findErr error + arcNamespace, existingArc, arcName, findErr = c.findARCByAlertRuleID(ctx, alertRuleId) + if findErr != nil { + return findErr + } + if existingArc == nil { + return nil + } + if err := validateDropRestorePreconditions(monitoringv1.Rule{}, nil, nil, existingArc); err != nil { + return err + } + } + + filtered, removed := filterOutDrop(existingArc.Spec.Configs, alertRuleId) + + if !removed { + return nil + } + + if len(filtered) == 0 || isStampOnly(filtered) { + if err := c.k8sClient.AlertRelabelConfigs().Delete(ctx, arcNamespace, arcName); err != nil { + return fmt.Errorf("failed to delete AlertRelabelConfig %s/%s: %w", arcNamespace, arcName, err) + } + return nil + } + + arc := existingArc.DeepCopy() + arc.Spec = osmv1.AlertRelabelConfigSpec{Configs: filtered} + if arc.Annotations == nil { + arc.Annotations = map[string]string{} + } + arc.Annotations[managementlabels.ARCAnnotationAlertRuleIDKey] = alertRuleId + + if err := c.k8sClient.AlertRelabelConfigs().Update(ctx, *arc); err != nil { + return fmt.Errorf("failed to update AlertRelabelConfig %s/%s: %w", arc.Namespace, arc.Name, err) + } + return nil +} + +// findARCByAlertRuleID searches for an ARC by its alert-rule-id annotation in +// the platform monitoring namespace. +func (c *client) findARCByAlertRuleID(ctx context.Context, alertRuleId string) (string, *osmv1.AlertRelabelConfig, string, error) { + namespaces := []string{k8s.ClusterMonitoringNamespace} + for _, ns := range namespaces { + arcs, err := c.k8sClient.AlertRelabelConfigs().List(ctx, ns) + if err != nil { + return "", nil, "", fmt.Errorf("failed to list AlertRelabelConfigs in %s: %w", ns, err) + } + for i := range arcs { + arc := arcs[i] + if arc.Annotations != nil && arc.Annotations[managementlabels.ARCAnnotationAlertRuleIDKey] == alertRuleId { + arcCopy := arc + return ns, &arcCopy, arc.Name, nil + } + } + } + return "", nil, "", nil +} diff --git a/pkg/management/update_platform_alert_rule_test.go b/pkg/management/update_platform_alert_rule_test.go new file mode 100644 index 000000000..1901b1b1e --- /dev/null +++ b/pkg/management/update_platform_alert_rule_test.go @@ -0,0 +1,870 @@ +package management_test + +import ( + "context" + "errors" + "strings" + "testing" + + osmv1 "github.com/openshift/api/monitoring/v1" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + + 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" +) + +var ( + // upOriginalPlatformRule is as stored in the PrometheusRule (without k8s labels). + upOriginalPlatformRule = monitoringv1.Rule{ + Alert: "PlatformAlert", + Expr: intstr.FromString("node_down == 1"), + Labels: map[string]string{ + "severity": "critical", + }, + } + upOriginalPlatformRuleId = alertrule.GetAlertingRuleId(&upOriginalPlatformRule) + + // upPlatformRule is as seen by RelabeledRules (with k8s labels added). + upPlatformRule = monitoringv1.Rule{ + Alert: "PlatformAlert", + Expr: intstr.FromString("node_down == 1"), + Labels: map[string]string{ + "severity": "critical", + k8s.PrometheusRuleLabelNamespace: "openshift-monitoring", + k8s.PrometheusRuleLabelName: "platform-rule", + k8s.AlertRuleLabelId: upOriginalPlatformRuleId, + }, + } + upPlatformRuleId = alertrule.GetAlertingRuleId(&upPlatformRule) + + upUserRule = monitoringv1.Rule{ + Alert: "UserAlert", + Labels: map[string]string{ + k8s.PrometheusRuleLabelNamespace: "user-namespace", + k8s.PrometheusRuleLabelName: "user-rule", + }, + } + upUserRuleId = alertrule.GetAlertingRuleId(&upUserRule) +) + +func newUpdatePlatformClient(t *testing.T) (management.Client, *testutils.MockClient) { + t.Helper() + mockK8s := &testutils.MockClient{} + mockK8s.NamespaceFunc = func() k8s.NamespaceInterface { + return &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(name string) bool { + return name == "openshift-monitoring" + }, + } + } + return management.New(context.Background(), mockK8s), mockK8s +} + +func mockPlatformRelabeledGet(ruleId string, rule monitoringv1.Rule) func() k8s.RelabeledRulesInterface { + return func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == ruleId { + return rule, true + } + return monitoringv1.Rule{}, false + }, + } + } +} + +func makePlatformPR(namespace, name string, rules ...monitoringv1.Rule) *testutils.MockPrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, ns, n string) (*monitoringv1.PrometheusRule, bool, error) { + return &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: n}, + Spec: monitoringv1.PrometheusRuleSpec{ + Groups: []monitoringv1.RuleGroup{{Name: "grp", Rules: rules}}, + }, + }, true, nil + }, + } +} + +// --- Managed-by / GitOps blocks --- + +func TestUpdatePlatformAlertRule_BlocksOperatorManagedWithGitOpsPR(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + opRule := copyRuleWithLabels(upPlatformRule, managementlabels.RuleManagedByLabel, managementlabels.ManagedByOperator) + mockK8s.RelabeledRulesFunc = mockPlatformRelabeledGet(upPlatformRuleId, opRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, namespace, name string) (*monitoringv1.PrometheusRule, bool, error) { + return &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + Annotations: map[string]string{"argocd.argoproj.io/tracking-id": "gitops-track"}, + }, + Spec: monitoringv1.PrometheusRuleSpec{ + Groups: []monitoringv1.RuleGroup{{Name: "grp", Rules: []monitoringv1.Rule{upOriginalPlatformRule}}}, + }, + }, true, nil + }, + } + } + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, _ string) (*osmv1.AlertRelabelConfig, bool, error) { + return nil, false, nil + }, + } + } + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, upOriginalPlatformRule) + if err == nil || !strings.Contains(err.Error(), "managed by GitOps") { + t.Errorf("expected GitOps block, got: %v", err) + } +} + +func TestUpdatePlatformAlertRule_BlocksGitOpsManagedARC(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + opRule := copyRuleWithLabels(upPlatformRule, managementlabels.RuleManagedByLabel, managementlabels.ManagedByOperator) + mockK8s.RelabeledRulesFunc = mockPlatformRelabeledGet(upPlatformRuleId, opRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return makePlatformPR("openshift-monitoring", "platform-rule", upOriginalPlatformRule) + } + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, ns, name string) (*osmv1.AlertRelabelConfig, bool, error) { + return &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, Namespace: ns, + Annotations: map[string]string{"argocd.argoproj.io/tracking-id": "abc"}, + }, + }, true, nil + }, + } + } + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, upOriginalPlatformRule) + if err == nil || !strings.Contains(err.Error(), "managed by GitOps") { + t.Errorf("expected GitOps block (ARC), got: %v", err) + } +} + +func TestUpdatePlatformAlertRule_BlocksGitOpsManagedRule(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + gitopsRule := copyRuleWithLabels(upPlatformRule, managementlabels.RuleManagedByLabel, managementlabels.ManagedByGitOps) + mockK8s.RelabeledRulesFunc = mockPlatformRelabeledGet(upPlatformRuleId, gitopsRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return makePlatformPR("openshift-monitoring", "platform-rule", upOriginalPlatformRule) + } + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, _ string) (*osmv1.AlertRelabelConfig, bool, error) { + return nil, false, nil + }, + } + } + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, upOriginalPlatformRule) + if err == nil || !strings.Contains(err.Error(), "managed by GitOps") { + t.Errorf("expected GitOps block (rule), got: %v", err) + } +} + +// --- Not found / wrong type --- + +func TestUpdatePlatformAlertRule_NotFound(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, _ string) (monitoringv1.Rule, bool) { return monitoringv1.Rule{}, false }, + } + } + + err := client.UpdatePlatformAlertRule(context.Background(), "nonexistent-id", upPlatformRule) + var nf *management.NotFoundError + if !errors.As(err, &nf) || nf.Resource != "AlertRule" { + t.Errorf("expected NotFoundError for AlertRule, got: %v", err) + } +} + +func TestUpdatePlatformAlertRule_UserRuleReturnsError(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + mockK8s.RelabeledRulesFunc = mockPlatformRelabeledGet(upUserRuleId, upUserRule) + + err := client.UpdatePlatformAlertRule(context.Background(), upUserRuleId, upUserRule) + if err == nil || !strings.Contains(err.Error(), "only supported for platform alert rules") { + t.Errorf("expected user workload error, got: %v", err) + } +} + +func TestUpdatePlatformAlertRule_PRNotFound(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + mockK8s.RelabeledRulesFunc = mockPlatformRelabeledGet(upPlatformRuleId, upPlatformRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, _, _ string) (*monitoringv1.PrometheusRule, bool, error) { + return nil, false, nil + }, + } + } + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, upPlatformRule) + var nf *management.NotFoundError + if !errors.As(err, &nf) || nf.Resource != "PrometheusRule" { + t.Errorf("expected NotFoundError for PrometheusRule, got: %v", err) + } +} + +func TestUpdatePlatformAlertRule_PRGetError(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + mockK8s.RelabeledRulesFunc = mockPlatformRelabeledGet(upPlatformRuleId, upPlatformRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, _, _ string) (*monitoringv1.PrometheusRule, bool, error) { + return nil, false, errors.New("failed to get PrometheusRule") + }, + } + } + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, upPlatformRule) + if err == nil || !strings.Contains(err.Error(), "failed to get PrometheusRule") { + t.Errorf("expected PR get error, got: %v", err) + } +} + +// --- No label changes / revert --- + +func setupPlatformWithARC(t *testing.T, mockK8s *testutils.MockClient, arcFn func() k8s.AlertRelabelConfigInterface) { + t.Helper() + mockK8s.RelabeledRulesFunc = mockPlatformRelabeledGet(upPlatformRuleId, upPlatformRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return makePlatformPR("openshift-monitoring", "platform-rule", upOriginalPlatformRule) + } + mockK8s.AlertRelabelConfigsFunc = arcFn +} + +func TestUpdatePlatformAlertRule_DeletesARCOnRevert(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + deleted := false + setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, ns, name string) (*osmv1.AlertRelabelConfig, bool, error) { + return &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: osmv1.AlertRelabelConfigSpec{Configs: []osmv1.RelabelConfig{}}, + }, true, nil + }, + DeleteFunc: func(_ context.Context, _, _ string) error { deleted = true; return nil }, + } + }) + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, upOriginalPlatformRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !deleted { + t.Error("expected ARC to be deleted on revert") + } +} + +// --- Label changes / ARC creation --- + +func TestUpdatePlatformAlertRule_CreatesARCForLabelChange(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + var createdARC *osmv1.AlertRelabelConfig + setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { + 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["new_label"] = "new_value" + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if createdARC == nil { + t.Fatal("expected ARC to be created") + } + if createdARC.Namespace != "openshift-monitoring" { + t.Errorf("expected ARC namespace openshift-monitoring, got %q", createdARC.Namespace) + } + if !strings.HasPrefix(createdARC.Name, "arc-") { + t.Errorf("expected ARC name to start with arc-, got %q", createdARC.Name) + } + if len(createdARC.Spec.Configs) == 0 { + t.Error("expected ARC to have relabel configs") + } +} + +func TestUpdatePlatformAlertRule_IdStampAndSeverityChange(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + var createdARC *osmv1.AlertRelabelConfig + setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { + 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["severity"] = "info" + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if createdARC == nil { + t.Fatal("expected ARC to be created") + } + if len(createdARC.Spec.Configs) != 2 { + t.Fatalf("expected 2 relabel configs (id-stamp + severity), got %d", len(createdARC.Spec.Configs)) + } + cfg0 := createdARC.Spec.Configs[0] + if string(cfg0.Action) != "Replace" || string(cfg0.TargetLabel) != "openshift_io_alert_rule_id" { + t.Errorf("cfg0: expected id-stamp Replace, got action=%s target=%s", cfg0.Action, cfg0.TargetLabel) + } + if cfg0.Replacement != upPlatformRuleId { + t.Errorf("cfg0.Replacement: expected %q, got %q", upPlatformRuleId, cfg0.Replacement) + } + cfg1 := createdARC.Spec.Configs[1] + if string(cfg1.Action) != "Replace" || string(cfg1.TargetLabel) != "severity" || cfg1.Replacement != "info" { + t.Errorf("cfg1: expected severity Replace info, got action=%s target=%s replacement=%s", cfg1.Action, cfg1.TargetLabel, cfg1.Replacement) + } +} + +func TestUpdatePlatformAlertRule_IdStampScopesStaticLabels(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + // Override PR to have extra stable labels. + origWithExtras := copyRule(upOriginalPlatformRule) + origWithExtras.Labels = map[string]string{"severity": "critical", "component": "kube", "team": "sre"} + idForExtras := alertrule.GetAlertingRuleId(&origWithExtras) + + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == idForExtras { + return monitoringv1.Rule{ + Alert: "PlatformAlert", Expr: intstr.FromString("node_down == 1"), + Labels: map[string]string{ + k8s.PrometheusRuleLabelNamespace: "openshift-monitoring", + k8s.PrometheusRuleLabelName: "platform-rule", + k8s.AlertRuleLabelId: idForExtras, + "severity": "critical", + }, + }, true + } + return monitoringv1.Rule{}, false + }, + } + } + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, namespace, name string) (*monitoringv1.PrometheusRule, bool, error) { + return &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, + Spec: monitoringv1.PrometheusRuleSpec{ + Groups: []monitoringv1.RuleGroup{{Name: "test-group", Rules: []monitoringv1.Rule{origWithExtras}}}, + }, + }, true, nil + }, + } + } + + var createdARC *osmv1.AlertRelabelConfig + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + 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{"severity": "info"} + + err := client.UpdatePlatformAlertRule(context.Background(), idForExtras, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if createdARC == nil || len(createdARC.Spec.Configs) != 2 { + t.Fatalf("expected 2 ARC configs, got %d", len(createdARC.Spec.Configs)) + } + + idCfg := createdARC.Spec.Configs[0] + if string(idCfg.Action) != "Replace" || string(idCfg.TargetLabel) != "openshift_io_alert_rule_id" { + t.Errorf("expected id-stamp config, got action=%s target=%s", idCfg.Action, idCfg.TargetLabel) + } + var srcLabels []string + for _, s := range idCfg.SourceLabels { + srcLabels = append(srcLabels, string(s)) + } + for _, expected := range []string{"alertname", "component", "severity", "team"} { + found := false + for _, sl := range srcLabels { + if sl == expected { + found = true + break + } + } + if !found { + t.Errorf("expected source label %q in id-stamp config", expected) + } + } + for _, unexpected := range []string{"namespace"} { + for _, sl := range srcLabels { + if sl == unexpected { + t.Errorf("unexpected source label %q in id-stamp config", unexpected) + } + } + } + if !strings.HasPrefix(idCfg.Regex, "^") || !strings.HasSuffix(idCfg.Regex, "$") { + t.Errorf("expected anchored regex, got %q", idCfg.Regex) + } + if !strings.Contains(idCfg.Regex, "^PlatformAlert;kube;critical;sre$") { + t.Errorf("expected sorted label values in regex, got %q", idCfg.Regex) + } +} + +func TestUpdatePlatformAlertRule_UpdatesExistingARC(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + expectedArcName := k8s.GetAlertRelabelConfigName("platform-rule", upPlatformRuleId) + var updatedARC *osmv1.AlertRelabelConfig + + setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { + existing := &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: expectedArcName, Namespace: "openshift-monitoring"}, + Spec: osmv1.AlertRelabelConfigSpec{ + Configs: []osmv1.RelabelConfig{{TargetLabel: "testing2", Replacement: "newlabel2", Action: "Replace"}}, + }, + } + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, name string) (*osmv1.AlertRelabelConfig, bool, error) { + if name == expectedArcName { + return existing, true, nil + } + return nil, false, nil + }, + UpdateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) error { updatedARC = &arc; return nil }, + } + }) + + updatedRule := copyRule(upOriginalPlatformRule) + updatedRule.Labels["severity"] = "info" + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if updatedARC == nil { + t.Fatal("expected existing ARC to be updated") + } + if len(updatedARC.Spec.Configs) == 0 { + t.Error("expected updated ARC to have configs") + } +} + +func TestUpdatePlatformAlertRule_DeletesARCWhenNoOverridesRemain(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + expectedArcName := k8s.GetAlertRelabelConfigName("platform-rule", upPlatformRuleId) + deleted := false + var updatedARC *osmv1.AlertRelabelConfig + + setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { + existing := &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: expectedArcName, Namespace: "openshift-monitoring"}, + Spec: osmv1.AlertRelabelConfigSpec{ + Configs: []osmv1.RelabelConfig{{TargetLabel: "testing2", Replacement: "newlabel2", Action: "Replace"}}, + }, + } + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, name string) (*osmv1.AlertRelabelConfig, bool, error) { + if name == expectedArcName { + return existing, true, nil + } + return nil, false, nil + }, + UpdateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) error { updatedARC = &arc; return nil }, + DeleteFunc: func(_ context.Context, _, _ string) error { deleted = true; return nil }, + } + }) + + // Drop testing2 (explicit delete); keep severity unchanged (no override needed) + updatedRule := copyRule(upOriginalPlatformRule) + updatedRule.Labels = map[string]string{"severity": "critical", "testing2": ""} + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if updatedARC != nil { + t.Error("expected ARC to be deleted, not updated") + } + if !deleted { + t.Error("expected ARC to be deleted when no overrides remain") + } +} + +// --- Validation --- + +func TestUpdatePlatformAlertRule_RejectDropSeverity(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{} + }) + + updatedRule := copyRule(upOriginalPlatformRule) + updatedRule.Labels = map[string]string{"severity": ""} + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, updatedRule) + if err == nil || !strings.Contains(err.Error(), `label "severity" cannot be dropped`) { + t.Errorf("expected severity drop error, got: %v", err) + } +} + +func TestUpdatePlatformAlertRule_IgnoresProtectedLabels(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + + var createdARC *osmv1.AlertRelabelConfig + setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { + 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["openshift_io_alert_rule_id"] = "fake" + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if createdARC != nil { + for _, cfg := range createdARC.Spec.Configs { + if string(cfg.TargetLabel) == "openshift_io_alert_rule_id" && cfg.Replacement == "fake" { + t.Error("protected label openshift_io_alert_rule_id must not be overridden by the request") + } + } + } +} + +func TestUpdatePlatformAlertRule_RejectsAlertNameChange(t *testing.T) { + client, mockK8s := newUpdatePlatformClient(t) + setupPlatformWithARC(t, mockK8s, func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{} + }) + + updatedRule := copyRule(upOriginalPlatformRule) + updatedRule.Labels = map[string]string{"alertname": "NewName"} + + err := client.UpdatePlatformAlertRule(context.Background(), upPlatformRuleId, updatedRule) + if err == nil || !strings.Contains(err.Error(), "immutable") { + t.Errorf("expected immutable alertname error, got: %v", err) + } +} + +// ============================================================ +// Drop/Restore Platform Alert Rule tests +// ============================================================ + +var ( + drOriginalPlatformRule = monitoringv1.Rule{ + Alert: "PlatformAlertDrop", + Expr: intstr.FromString("up == 0"), + Labels: map[string]string{ + "severity": "warning", + "team": "sre", + }, + } + drOriginalPlatformRuleId = alertrule.GetAlertingRuleId(&drOriginalPlatformRule) + + drPlatformRule = monitoringv1.Rule{ + Alert: "PlatformAlertDrop", + Expr: intstr.FromString("up == 0"), + Labels: map[string]string{ + "severity": "warning", + "team": "sre", + k8s.PrometheusRuleLabelNamespace: "openshift-monitoring", + k8s.PrometheusRuleLabelName: "platform-rule-drop", + k8s.AlertRuleLabelId: drOriginalPlatformRuleId, + }, + } + drPlatformRuleId = alertrule.GetAlertingRuleId(&drPlatformRule) +) + +func newDropRestoreClient(t *testing.T) (management.Client, *testutils.MockClient) { + t.Helper() + mockK8s := &testutils.MockClient{} + mockK8s.NamespaceFunc = func() k8s.NamespaceInterface { + return &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(name string) bool { return name == "openshift-monitoring" }, + } + } + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == drPlatformRuleId { + return drPlatformRule, true + } + return monitoringv1.Rule{}, false + }, + } + } + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return makePlatformPR("openshift-monitoring", "platform-rule-drop", drOriginalPlatformRule) + } + return management.New(context.Background(), mockK8s), mockK8s +} + +func TestDropAlertRule_CreatesARCWithIdStampAndDrop(t *testing.T) { + client, mockK8s := newDropRestoreClient(t) + + var result *osmv1.AlertRelabelConfig + existing := &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "arc-platform-rule-drop-xxxx", Namespace: "openshift-monitoring"}, + Spec: osmv1.AlertRelabelConfigSpec{ + Configs: []osmv1.RelabelConfig{{TargetLabel: "component", Replacement: "kube-apiserver", Action: "Replace"}}, + }, + } + + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, ns, name string) (*osmv1.AlertRelabelConfig, bool, error) { + if ns == "openshift-monitoring" && strings.HasPrefix(name, "arc-") { + return existing, true, nil + } + return nil, false, nil + }, + UpdateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) error { result = &arc; return nil }, + CreateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) (*osmv1.AlertRelabelConfig, error) { + result = &arc + return &arc, nil + }, + } + } + + err := client.DropAlertRule(context.Background(), drPlatformRuleId) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result == nil { + t.Fatal("expected ARC to be created/updated") + } + if result.Namespace != "openshift-monitoring" || !strings.HasPrefix(result.Name, "arc-") { + t.Errorf("unexpected ARC name/namespace: %s/%s", result.Namespace, result.Name) + } + + var hasPriorReplace, hasIdStamp, hasDrop bool + for _, rc := range result.Spec.Configs { + switch string(rc.Action) { + case "Replace": + if string(rc.TargetLabel) == "component" && rc.Replacement == "kube-apiserver" { + hasPriorReplace = true + } + if string(rc.TargetLabel) == "openshift_io_alert_rule_id" && rc.Replacement == drPlatformRuleId { + hasIdStamp = true + } + case "Drop": + if len(rc.SourceLabels) == 1 && string(rc.SourceLabels[0]) == "openshift_io_alert_rule_id" && rc.Regex == drPlatformRuleId { + hasDrop = true + } + } + } + if !hasPriorReplace { + t.Error("expected prior Replace config to be preserved") + } + if !hasIdStamp { + t.Error("expected id-stamp Replace config") + } + if !hasDrop { + t.Error("expected Drop config") + } +} + +func TestDropAlertRule_Idempotent(t *testing.T) { + client, mockK8s := newDropRestoreClient(t) + + var stored *osmv1.AlertRelabelConfig + var last *osmv1.AlertRelabelConfig + + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, _ string) (*osmv1.AlertRelabelConfig, bool, error) { + if stored == nil { + return nil, false, nil + } + return stored, true, nil + }, + CreateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) (*osmv1.AlertRelabelConfig, error) { + stored = &arc + last = &arc + return &arc, nil + }, + UpdateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) error { + stored = &arc + last = &arc + return nil + }, + } + } + + if err := client.DropAlertRule(context.Background(), drPlatformRuleId); err != nil { + t.Fatalf("first drop: %v", err) + } + cfgCount := len(last.Spec.Configs) + + if err := client.DropAlertRule(context.Background(), drPlatformRuleId); err != nil { + t.Fatalf("second drop: %v", err) + } + if len(last.Spec.Configs) != cfgCount { + t.Errorf("expected same config count after second drop: got %d, want %d", len(last.Spec.Configs), cfgCount) + } +} + +func TestRestoreAlertRule_DeletesARCWhenOnlyDropRemains(t *testing.T) { + client, mockK8s := newDropRestoreClient(t) + deleted := false + + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + onlyDrop := &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "arc-to-delete", Namespace: "openshift-monitoring"}, + Spec: osmv1.AlertRelabelConfigSpec{ + Configs: []osmv1.RelabelConfig{ + {SourceLabels: []osmv1.LabelName{"openshift_io_alert_rule_id"}, Regex: drPlatformRuleId, Action: "Drop"}, + }, + }, + } + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, _ string) (*osmv1.AlertRelabelConfig, bool, error) { + return onlyDrop, true, nil + }, + DeleteFunc: func(_ context.Context, _, _ string) error { deleted = true; return nil }, + UpdateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) error { return errors.New("should not update") }, + } + } + + if err := client.RestoreAlertRule(context.Background(), drPlatformRuleId); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !deleted { + t.Error("expected ARC to be deleted") + } +} + +func TestRestoreAlertRule_KeepsOtherConfigsRemovesDropOnly(t *testing.T) { + client, mockK8s := newDropRestoreClient(t) + deleted := false + var updated *osmv1.AlertRelabelConfig + + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + withOthers := &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "arc-keep", Namespace: "openshift-monitoring"}, + Spec: osmv1.AlertRelabelConfigSpec{ + Configs: []osmv1.RelabelConfig{ + {TargetLabel: "component", Replacement: "kube-apiserver", Action: "Replace"}, + {SourceLabels: []osmv1.LabelName{"openshift_io_alert_rule_id"}, Regex: drPlatformRuleId, Action: "Drop"}, + }, + }, + } + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, _ string) (*osmv1.AlertRelabelConfig, bool, error) { + return withOthers, true, nil + }, + DeleteFunc: func(_ context.Context, _, _ string) error { deleted = true; return nil }, + UpdateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) error { updated = &arc; return nil }, + } + } + + if err := client.RestoreAlertRule(context.Background(), drPlatformRuleId); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if deleted { + t.Error("expected ARC to be updated, not deleted") + } + if updated == nil { + t.Fatal("expected ARC to be updated") + } + for _, rc := range updated.Spec.Configs { + if string(rc.Action) == "Drop" { + t.Error("Drop config should have been removed") + } + } + found := false + for _, rc := range updated.Spec.Configs { + if string(rc.Action) == "Replace" && string(rc.TargetLabel) == "component" && rc.Replacement == "kube-apiserver" { + found = true + } + } + if !found { + t.Error("expected Replace config for component to be preserved") + } +} + +func TestRestoreAlertRule_DeletesARCWhenOnlyStampAndDropRemain(t *testing.T) { + client, mockK8s := newDropRestoreClient(t) + deleted := false + var updated *osmv1.AlertRelabelConfig + + mockK8s.AlertRelabelConfigsFunc = func() k8s.AlertRelabelConfigInterface { + stampAndDrop := &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "arc-stamp-drop", Namespace: "openshift-monitoring"}, + Spec: osmv1.AlertRelabelConfigSpec{ + Configs: []osmv1.RelabelConfig{ + { + SourceLabels: []osmv1.LabelName{"alertname", "severity", "team"}, + Regex: "^PlatformAlertDrop;warning;sre$", + TargetLabel: "openshift_io_alert_rule_id", + Replacement: drPlatformRuleId, + Action: "Replace", + }, + {SourceLabels: []osmv1.LabelName{"openshift_io_alert_rule_id"}, Regex: drPlatformRuleId, Action: "Drop"}, + }, + }, + } + return &testutils.MockAlertRelabelConfigInterface{ + GetFunc: func(_ context.Context, _, _ string) (*osmv1.AlertRelabelConfig, bool, error) { + return stampAndDrop, true, nil + }, + DeleteFunc: func(_ context.Context, _, _ string) error { deleted = true; return nil }, + UpdateFunc: func(_ context.Context, arc osmv1.AlertRelabelConfig) error { updated = &arc; return nil }, + } + } + + if err := client.RestoreAlertRule(context.Background(), drPlatformRuleId); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !deleted { + t.Error("expected ARC to be deleted when only stamp remains after removing Drop") + } + if updated != nil { + t.Error("ARC should not be updated when deleted") + } +} diff --git a/pkg/management/update_user_defined_alert_rule.go b/pkg/management/update_user_defined_alert_rule.go new file mode 100644 index 000000000..7f32853b1 --- /dev/null +++ b/pkg/management/update_user_defined_alert_rule.go @@ -0,0 +1,144 @@ +package management + +import ( + "context" + "fmt" + + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + "k8s.io/apimachinery/pkg/types" + + alertrule "github.com/openshift/monitoring-plugin/pkg/alert_rule" + "github.com/openshift/monitoring-plugin/pkg/k8s" +) + +// UpdateUserDefinedAlertRule updates an existing user-defined alert rule by its +// ID. It rejects platform-managed, operator-managed, and GitOps-managed rules, +// validates severity, rejects duplicate rule definitions, and returns the +// recomputed rule ID. +func (c *client) UpdateUserDefinedAlertRule(ctx context.Context, alertRuleId string, alertRule monitoringv1.Rule) (string, error) { + rule, found := c.k8sClient.RelabeledRules().Get(ctx, alertRuleId) + if !found { + return "", &NotFoundError{Resource: "AlertRule", Id: alertRuleId} + } + + namespace := rule.Labels[k8s.PrometheusRuleLabelNamespace] + name := rule.Labels[k8s.PrometheusRuleLabelName] + + // Common preconditions on relabeled rule (labels-based) + if err := validateUserUpdatePreconditions(rule, nil); err != nil { + return "", err + } + + if c.isPlatformManagedPrometheusRule(types.NamespacedName{Namespace: namespace, Name: name}) { + return "", &NotAllowedError{Message: "cannot update alert rule in a platform-managed PrometheusRule"} + } + + pr, found, err := c.k8sClient.PrometheusRules().Get(ctx, namespace, name) + if err != nil { + return "", err + } + + if !found { + return "", &NotFoundError{ + Resource: "PrometheusRule", + Id: alertRuleId, + AdditionalInfo: fmt.Sprintf("PrometheusRule %s/%s not found", namespace, name), + } + } + + // After fetching the PR, block edits for operator-managed PrometheusRules (they will be reconciled) + if err := validateUserUpdatePreconditions(rule, pr); err != nil { + return "", err + } + + // Locate the target rule once and update it after validation + var foundGroupIdx, foundRuleIdx int + ruleFound := false + for groupIdx := range pr.Spec.Groups { + for ruleIdx := range pr.Spec.Groups[groupIdx].Rules { + rule := &pr.Spec.Groups[groupIdx].Rules[ruleIdx] + if ruleMatchesAlertRuleID(*rule, alertRuleId) { + foundGroupIdx = groupIdx + foundRuleIdx = ruleIdx + ruleFound = true + break + } + } + if ruleFound { + break + } + } + + if !ruleFound { + return "", &NotFoundError{ + Resource: "AlertRule", + Id: alertRuleId, + AdditionalInfo: fmt.Sprintf("in PrometheusRule %s/%s", namespace, name), + } + } + + // Validate severity if present + if sev, ok := alertRule.Labels["severity"]; ok && sev != "" { + if !isValidSeverity(sev) { + return "", &ValidationError{Message: fmt.Sprintf("invalid severity %q: must be one of critical|warning|info|none", sev)} + } + } + + computedId := alertrule.GetAlertingRuleId(&alertRule) + + // Treat "true clones" (spec-identical rules that compute to the same id) as unsupported. + // If the updated rule would collide with some other existing rule, reject the update. + if computedId != "" && computedId != alertRuleId { + // Check within the same PrometheusRule first (authoritative). + for groupIdx := range pr.Spec.Groups { + for ruleIdx := range pr.Spec.Groups[groupIdx].Rules { + if groupIdx == foundGroupIdx && ruleIdx == foundRuleIdx { + continue + } + existing := pr.Spec.Groups[groupIdx].Rules[ruleIdx] + // Treat "true clones" as unsupported: identical definitions compute to the same id. + if existing.Alert != "" && alertrule.GetAlertingRuleId(&existing) == computedId { + return "", &ConflictError{Message: "alert rule with exact config already exists"} + } + } + } + + _, found := c.k8sClient.RelabeledRules().Get(ctx, computedId) + if found { + return "", &ConflictError{Message: "alert rule with exact config already exists"} + } + } + + if alertRule.Labels == nil { + alertRule.Labels = map[string]string{} + } + alertRule.Labels[k8s.AlertRuleLabelId] = computedId + + // Perform the update in-place exactly once + pr.Spec.Groups[foundGroupIdx].Rules[foundRuleIdx] = alertRule + + err = c.k8sClient.PrometheusRules().Update(ctx, *pr) + if err != nil { + return "", fmt.Errorf("failed to update PrometheusRule %s/%s: %w", pr.Namespace, pr.Name, err) + } + + if err := c.migrateClassificationOverrideIfRuleIDChanged(ctx, namespace, name, alertRuleId, computedId); err != nil { + return "", err + } + + return computedId, nil +} + +// migrateClassificationOverrideIfRuleIDChanged is a no-op for user-defined +// rules because the platform does not support ARCs in the user-workload +// namespace. When CMO adds that support, this function should migrate any +// existing ARC to the new rule ID. +func (c *client) migrateClassificationOverrideIfRuleIDChanged( + _ context.Context, + _ string, + _ string, + _ string, + _ string, +) error { + return nil +} diff --git a/pkg/management/update_user_defined_alert_rule_test.go b/pkg/management/update_user_defined_alert_rule_test.go new file mode 100644 index 000000000..1b27d9a7d --- /dev/null +++ b/pkg/management/update_user_defined_alert_rule_test.go @@ -0,0 +1,408 @@ +package management_test + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + + 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" +) + +var ( + // originalUserRule is as stored in the PrometheusRule (without k8s labels). + originalUserRule = monitoringv1.Rule{ + Alert: "UserAlert", + Expr: intstr.FromString("up == 0"), + Labels: map[string]string{ + "severity": "warning", + }, + } + originalUserRuleId = alertrule.GetAlertingRuleId(&originalUserRule) + + // userRule is as seen by RelabeledRules (with k8s labels added). + udUserRule = monitoringv1.Rule{ + Alert: "UserAlert", + Expr: intstr.FromString("up == 0"), + Labels: map[string]string{ + "severity": "warning", + k8s.PrometheusRuleLabelNamespace: "user-namespace", + k8s.PrometheusRuleLabelName: "user-rule", + }, + } + + udPlatformRule = monitoringv1.Rule{ + Alert: "PlatformAlert", + Labels: map[string]string{ + k8s.PrometheusRuleLabelNamespace: "openshift-monitoring", + k8s.PrometheusRuleLabelName: "platform-rule", + }, + } + udPlatformRuleId = alertrule.GetAlertingRuleId(&udPlatformRule) +) + +func newUpdateUserDefinedClient(t *testing.T) (management.Client, *testutils.MockClient) { + t.Helper() + mockK8s := &testutils.MockClient{} + mockK8s.NamespaceFunc = func() k8s.NamespaceInterface { + return &testutils.MockNamespaceInterface{ + IsClusterMonitoringNamespaceFunc: func(name string) bool { + return name == "openshift-monitoring" + }, + } + } + return management.New(context.Background(), mockK8s), mockK8s +} + +func mockUDRelabeledGet(ruleId string, rule monitoringv1.Rule) func() k8s.RelabeledRulesInterface { + return func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, id string) (monitoringv1.Rule, bool) { + if id == ruleId { + return rule, true + } + return monitoringv1.Rule{}, false + }, + } + } +} + +func makePRWithRule(ns, name string, rule monitoringv1.Rule) *testutils.MockPrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, namespace, prName string) (*monitoringv1.PrometheusRule, bool, error) { + return &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: prName}, + Spec: monitoringv1.PrometheusRuleSpec{ + Groups: []monitoringv1.RuleGroup{{Name: "test-group", Rules: []monitoringv1.Rule{rule}}}, + }, + }, true, nil + }, + } +} + +// --- Managed-by enforcement --- + +func TestUpdateUserDefinedAlertRule_BlocksGitOpsManaged(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + gitopsRule := copyRuleWithLabels(udUserRule, managementlabels.RuleManagedByLabel, managementlabels.ManagedByGitOps) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, gitopsRule) + + _, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, udUserRule) + if err == nil || !strings.Contains(err.Error(), "managed by GitOps") { + t.Errorf("expected GitOps block error, got: %v", err) + } +} + +func TestUpdateUserDefinedAlertRule_BlocksOperatorManaged(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + opRule := copyRuleWithLabels(udUserRule, managementlabels.RuleManagedByLabel, managementlabels.ManagedByOperator) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, opRule) + + _, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, udUserRule) + if err == nil || !strings.Contains(err.Error(), "managed by an operator") { + t.Errorf("expected operator block error, got: %v", err) + } +} + +// --- Not found --- + +func TestUpdateUserDefinedAlertRule_NotFound(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = func() k8s.RelabeledRulesInterface { + return &testutils.MockRelabeledRulesInterface{ + GetFunc: func(_ context.Context, _ string) (monitoringv1.Rule, bool) { return monitoringv1.Rule{}, false }, + } + } + + _, err := client.UpdateUserDefinedAlertRule(context.Background(), "nonexistent-id", udUserRule) + var nf *management.NotFoundError + if !errors.As(err, &nf) || nf.Resource != "AlertRule" { + t.Errorf("expected NotFoundError for AlertRule, got: %v", err) + } +} + +func TestUpdateUserDefinedAlertRule_PlatformRuleReturnsError(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(udPlatformRuleId, udPlatformRule) + + _, err := client.UpdateUserDefinedAlertRule(context.Background(), udPlatformRuleId, udPlatformRule) + if err == nil || !strings.Contains(err.Error(), "cannot update alert rule in a platform-managed PrometheusRule") { + t.Errorf("expected platform rule error, got: %v", err) + } +} + +// --- PrometheusRule errors --- + +func TestUpdateUserDefinedAlertRule_PRNotFound(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, udUserRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, _, _ string) (*monitoringv1.PrometheusRule, bool, error) { + return nil, false, nil + }, + } + } + + _, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, udUserRule) + var nf *management.NotFoundError + if !errors.As(err, &nf) || nf.Resource != "PrometheusRule" { + t.Errorf("expected NotFoundError for PrometheusRule, got: %v", err) + } +} + +func TestUpdateUserDefinedAlertRule_PRGetError(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, udUserRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, _, _ string) (*monitoringv1.PrometheusRule, bool, error) { + return nil, false, errors.New("failed to get PrometheusRule") + }, + } + } + + _, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, udUserRule) + if err == nil || !strings.Contains(err.Error(), "failed to get PrometheusRule") { + t.Errorf("expected PR get error, got: %v", err) + } +} + +func TestUpdateUserDefinedAlertRule_RuleNotInPR(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, udUserRule) + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, namespace, name string) (*monitoringv1.PrometheusRule, bool, error) { + return &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, + Spec: monitoringv1.PrometheusRuleSpec{ + Groups: []monitoringv1.RuleGroup{{Name: "test-group", Rules: []monitoringv1.Rule{}}}, + }, + }, true, nil + }, + } + } + + _, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, udUserRule) + if err == nil || !strings.Contains(err.Error(), fmt.Sprintf("AlertRule with id %s not found", originalUserRuleId)) { + t.Errorf("expected 'not found in PR' error, got: %v", err) + } +} + +func TestUpdateUserDefinedAlertRule_PRUpdateError(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, udUserRule) + pr := makePRWithRule("user-namespace", "user-rule", originalUserRule) + pr.UpdateFunc = func(_ context.Context, _ monitoringv1.PrometheusRule) error { + return errors.New("failed to update PrometheusRule") + } + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { return pr } + + _, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, originalUserRule) + if err == nil || !strings.Contains(err.Error(), "failed to update PrometheusRule") { + t.Errorf("expected PR update error, got: %v", err) + } +} + +// --- Successful updates --- + +func TestUpdateUserDefinedAlertRule_UpdatesRule(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 } + + updatedRule := copyRule(originalUserRule) + updatedRule.Labels["severity"] = "critical" + updatedRule.Expr = intstr.FromString("up == 1") + expectedId := alertrule.GetAlertingRuleId(&updatedRule) + + newId, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if newId != expectedId { + t.Errorf("expected newId %q, got %q", expectedId, newId) + } + if savedPR == nil { + t.Fatal("expected PR to be updated") + } + if savedPR.Spec.Groups[0].Rules[0].Labels["severity"] != "critical" { + t.Errorf("expected severity=critical in saved PR") + } +} + +func TestUpdateUserDefinedAlertRule_RuleIdChanges(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 } + + updatedRule := copyRule(originalUserRule) + updatedRule.Labels["severity"] = "critical" + updatedRule.Expr = intstr.FromString("up == 1") + expectedId := alertrule.GetAlertingRuleId(&updatedRule) + + newId, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if newId == originalUserRuleId { + t.Error("expected new ID to differ from original") + } + if newId != expectedId { + t.Errorf("expected new ID %q, got %q", expectedId, newId) + } + if savedPR == nil { + t.Fatal("expected PR to be saved") + } +} + +func TestUpdateUserDefinedAlertRule_OnlyMatchingRuleUpdated(t *testing.T) { + anotherRule := monitoringv1.Rule{Alert: "AnotherAlert", Expr: intstr.FromString("down == 1")} + + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, udUserRule) + + var savedPR *monitoringv1.PrometheusRule + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, namespace, name string) (*monitoringv1.PrometheusRule, bool, error) { + return &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, + Spec: monitoringv1.PrometheusRuleSpec{ + Groups: []monitoringv1.RuleGroup{{Name: "test-group", Rules: []monitoringv1.Rule{originalUserRule, anotherRule}}}, + }, + }, true, nil + }, + UpdateFunc: func(_ context.Context, p monitoringv1.PrometheusRule) error { savedPR = &p; return nil }, + } + } + + updatedRule := copyRule(originalUserRule) + updatedRule.Labels["severity"] = "info" + expectedId := alertrule.GetAlertingRuleId(&updatedRule) + + newId, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if newId != expectedId { + t.Errorf("expected %q, got %q", expectedId, newId) + } + if savedPR == nil { + t.Fatal("expected PR to be saved") + } + if len(savedPR.Spec.Groups[0].Rules) != 2 { + t.Fatalf("expected 2 rules, got %d", len(savedPR.Spec.Groups[0].Rules)) + } + if savedPR.Spec.Groups[0].Rules[0].Labels["severity"] != "info" { + t.Error("expected severity=info on first rule") + } + if savedPR.Spec.Groups[0].Rules[1].Alert != "AnotherAlert" { + t.Error("expected second rule to be AnotherAlert") + } +} + +func TestUpdateUserDefinedAlertRule_MultipleGroups(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, udUserRule) + + var savedPR *monitoringv1.PrometheusRule + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { + return &testutils.MockPrometheusRuleInterface{ + GetFunc: func(_ context.Context, namespace, name string) (*monitoringv1.PrometheusRule, bool, error) { + return &monitoringv1.PrometheusRule{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, + Spec: monitoringv1.PrometheusRuleSpec{ + Groups: []monitoringv1.RuleGroup{ + {Name: "group1", Rules: []monitoringv1.Rule{}}, + {Name: "group2", Rules: []monitoringv1.Rule{originalUserRule}}, + }, + }, + }, true, nil + }, + UpdateFunc: func(_ context.Context, p monitoringv1.PrometheusRule) error { savedPR = &p; return nil }, + } + } + + updatedRule := copyRule(originalUserRule) + updatedRule.Labels["new_label"] = "new_value" + expectedId := alertrule.GetAlertingRuleId(&updatedRule) + + newId, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, updatedRule) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if newId != expectedId { + t.Errorf("expected %q, got %q", expectedId, newId) + } + if savedPR == nil { + t.Fatal("expected PR to be saved") + } + if len(savedPR.Spec.Groups) != 2 { + t.Fatalf("expected 2 groups, got %d", len(savedPR.Spec.Groups)) + } + if len(savedPR.Spec.Groups[0].Rules) != 0 { + t.Error("expected group1 to remain empty") + } + if len(savedPR.Spec.Groups[1].Rules) != 1 { + t.Error("expected group2 to have 1 rule") + } + if savedPR.Spec.Groups[1].Rules[0].Labels["new_label"] != "new_value" { + t.Error("expected new_label in group2 rule") + } +} + +// --- Severity validation --- + +func TestUpdateUserDefinedAlertRule_InvalidSeverity(t *testing.T) { + client, mockK8s := newUpdateUserDefinedClient(t) + mockK8s.RelabeledRulesFunc = mockUDRelabeledGet(originalUserRuleId, udUserRule) + pr := makePRWithRule("user-namespace", "user-rule", originalUserRule) + pr.UpdateFunc = func(_ context.Context, _ monitoringv1.PrometheusRule) error { return nil } + mockK8s.PrometheusRulesFunc = func() k8s.PrometheusRuleInterface { return pr } + + updatedRule := copyRule(originalUserRule) + updatedRule.Labels = map[string]string{"severity": "urgent"} + _, err := client.UpdateUserDefinedAlertRule(context.Background(), originalUserRuleId, updatedRule) + if err == nil || !strings.Contains(err.Error(), "invalid severity") { + t.Errorf("expected invalid severity error, got: %v", err) + } +} + +// --- Helpers --- + +func copyRule(r monitoringv1.Rule) monitoringv1.Rule { + out := r + out.Labels = make(map[string]string) + for k, v := range r.Labels { + out.Labels[k] = v + } + return out +} + +func copyRuleWithLabels(r monitoringv1.Rule, extraKey, extraVal string) monitoringv1.Rule { + out := copyRule(r) + out.Labels[extraKey] = extraVal + return out +} diff --git a/test/e2e/helpers_test.go b/test/e2e/helpers_test.go index ad52b5d4c..cbd2e98eb 100644 --- a/test/e2e/helpers_test.go +++ b/test/e2e/helpers_test.go @@ -10,9 +10,14 @@ import ( "io" "net/http" "net/url" + "testing" "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/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/test/e2e/framework" ) @@ -29,6 +34,37 @@ func createRuleViaAPIWithRetry(ctx context.Context, f *framework.Framework, crea return id, err } +func strPtr(s string) *string { return &s } + +// findPlatformAlertRuleId discovers an existing platform alert rule from +// openshift-monitoring and returns its computed alert rule ID. +func findPlatformAlertRuleId(ctx context.Context, t *testing.T, f *framework.Framework) string { + t.Helper() + + prList, err := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(k8s.ClusterMonitoringNamespace).List(ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("Failed to list PrometheusRules in %s: %v", k8s.ClusterMonitoringNamespace, err) + } + + for _, pr := range prList.Items { + for _, group := range pr.Spec.Groups { + for i := range group.Rules { + rule := &group.Rules[i] + if rule.Alert == "" { + continue + } + id := alertrule.GetAlertingRuleId(rule) + if id != "" { + return id + } + } + } + } + + t.Fatal("No platform alert rules found in openshift-monitoring") + return "" +} + func createRuleViaAPI(ctx context.Context, f *framework.Framework, payload managementrouter.CreateAlertRuleRequest) (string, error) { reqBody, err := json.Marshal(payload) if err != nil { @@ -72,3 +108,30 @@ func createRuleViaAPI(ctx context.Context, f *framework.Framework, payload manag } return createResp.Id, nil } + +// mustCreateRule is a test convenience wrapper around createRuleViaAPI that +// builds the request from individual parameters and calls t.Fatal on error. +func mustCreateRule(ctx context.Context, t *testing.T, f *framework.Framework, namespace, alertName, prName string) string { + t.Helper() + + expr := fmt.Sprintf("absent(nonexistent{e2e_rule=%q})", alertName) + + id, err := createRuleViaAPI(ctx, f, managementrouter.CreateAlertRuleRequest{ + AlertingRule: &managementrouter.AlertRuleSpec{ + Alert: &alertName, + Expr: &expr, + For: strPtr("1m"), + Labels: &map[string]string{ + "severity": "info", + }, + }, + PrometheusRule: &managementrouter.PrometheusRuleTarget{ + PrometheusRuleName: prName, + PrometheusRuleNamespace: namespace, + }, + }) + if err != nil { + t.Fatalf("Failed to create rule %s in %s: %v", alertName, namespace, err) + } + return id +} diff --git a/test/e2e/update_alert_rule_test.go b/test/e2e/update_alert_rule_test.go new file mode 100644 index 000000000..a2d37b620 --- /dev/null +++ b/test/e2e/update_alert_rule_test.go @@ -0,0 +1,395 @@ +//go:build e2e + +package e2e + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "testing" + "time" + + osmv1 "github.com/openshift/api/monitoring/v1" + 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/pkg/managementlabels" + "github.com/openshift/monitoring-plugin/test/e2e/framework" +) + +func TestUpdateAlertRule_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) + + patchDrop(t, f, ctx, 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) + } + + var foundDropARC bool + for _, arc := range arcList.Items { + if hasDropActionForRule(arc, ruleID) { + foundDropARC = true + t.Logf("Found ARC %s/%s with drop action for rule %s", arc.Namespace, arc.Name, ruleID) + break + } + } + if !foundDropARC { + t.Fatal("Expected to find an ARC with drop action after disabling rule") + } + + patchDrop(t, f, ctx, 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 restore", arc.Namespace, arc.Name) + } + } + + t.Log("Drop/restore e2e test passed successfully") +} + +func TestUpdateAlertRule_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) + t.Logf("Using platform rule with ID: %s", ruleID) + + defer cleanupARCsForRule(t, f, ctx, k8s.ClusterMonitoringNamespace, ruleID) + + component := "networking" + layer := "cluster" + classificationPatch := managementrouter.AlertRuleClassificationPatch{ + Component: &component, + ComponentSet: true, + Layer: &layer, + LayerSet: true, + } + + patchReq := managementrouter.BulkUpdateAlertRulesRequest{ + RuleIds: []string{ruleID}, + Classification: &classificationPatch, + } + + reqBody, err := json.Marshal(patchReq) + if err != nil { + t.Fatalf("Failed to marshal classification patch: %v", err) + } + + patchURL := f.PluginURL + "/api/v1/alerting/rules" + req, err := http.NewRequestWithContext(ctx, http.MethodPatch, patchURL, bytes.NewBuffer(reqBody)) + if err != nil { + t.Fatalf("Failed to create patch request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + if f.BearerToken != "" { + req.Header.Set("Authorization", "Bearer "+f.BearerToken) + } + + resp, err := f.HTTPClient().Do(req) + if err != nil { + t.Fatalf("Failed to make classification patch request: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + t.Fatalf("Expected status 200, got %d. Body: %s", resp.StatusCode, string(body)) + } + + var patchResp managementrouter.BulkUpdateAlertRulesResponse + if err := json.NewDecoder(resp.Body).Decode(&patchResp); err != nil { + t.Fatalf("Failed to decode patch response: %v", err) + } + + if len(patchResp.Rules) != 1 { + t.Fatalf("Expected 1 rule result, got %d", len(patchResp.Rules)) + } + if patchResp.Rules[0].StatusCode != http.StatusNoContent { + msg := "" + if patchResp.Rules[0].Message != nil { + msg = *patchResp.Rules[0].Message + } + t.Fatalf("Expected per-rule status 204, got %d: %s", + patchResp.Rules[0].StatusCode, msg) + } + + 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) + } + + var foundClassificationARC bool + for _, arc := range arcList.Items { + if hasClassificationForRule(arc, "networking", "cluster") { + foundClassificationARC = true + t.Logf("Found ARC %s/%s with classification labels", arc.Namespace, arc.Name) + break + } + } + if !foundClassificationARC { + t.Fatal("Expected to find an ARC with classification relabel configs") + } + + t.Log("Classification e2e test passed successfully") +} + +// TestRBAC_UpdateAlertRule verifies that the bulk-update endpoint enforces +// Kubernetes RBAC across three user profiles: unprivileged (403), +// namespace-scoped (204 in own namespace, 403 elsewhere), and cluster-admin +// (204 everywhere). +func TestRBAC_UpdateAlertRule(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-upd-y") + if err != nil { + t.Fatalf("Failed to create namespace Y: %v", err) + } + defer func() { _ = cleanupY() }() + + nsZ, cleanupZ, err := f.CreateUserNamespace(ctx, "test-rbac-upd-z") + if err != nil { + t.Fatalf("Failed to create namespace Z: %v", err) + } + defer func() { _ = cleanupZ() }() + + userA, err := f.CreateAnonymousUser(ctx, "e2e-rbac-upd-a", "default") + if err != nil { + t.Fatalf("Failed to create unprivileged user A: %v", err) + } + defer func() { _ = userA.Cleanup() }() + + userB, err := f.CreateScopedUser(ctx, "e2e-rbac-upd-b", nsY, + "monitoring.coreos.com", []string{"prometheusrules"}, []string{"get", "create", "update", "patch"}) + if err != nil { + t.Fatalf("Failed to create scoped user B: %v", err) + } + defer func() { _ = userB.Cleanup() }() + + ruleInY := mustCreateRule(ctx, t, f, nsY, "RBACUpdateAlertY", "e2e-rbac-upd-pr") + t.Logf("Created rule in namespace Y: %s", ruleInY) + + ruleInZ := mustCreateRule(ctx, t, f, nsZ, "RBACUpdateAlertZ", "e2e-rbac-upd-pr") + t.Logf("Created rule in namespace Z: %s", ruleInZ) + + ruleInY2 := mustCreateRule(ctx, t, f, nsY, "RBACUpdateAlertY2", "e2e-rbac-upd-pr") + t.Logf("Created second rule in namespace Y: %s", ruleInY2) + + waitForUpdateCacheSync(t, f, ctx, userA.Token, ruleInY) + waitForUpdateCacheSync(t, f, ctx, userA.Token, ruleInY2) + waitForUpdateCacheSync(t, f, ctx, userA.Token, ruleInZ) + + cases := []struct { + name string + token string + ruleID string + wantStatus int + }{ + {"UserA_NoPerms_DeniedNamespaceY", userA.Token, ruleInY, http.StatusForbidden}, + {"UserB_ScopedPerms_SucceedsNamespaceY", userB.Token, ruleInY, http.StatusNoContent}, + {"UserB_ScopedPerms_DeniedNamespaceZ", userB.Token, ruleInZ, http.StatusForbidden}, + {"UserC_Admin_SucceedsNamespaceZ", f.BearerToken, ruleInZ, http.StatusNoContent}, + {"UserC_Admin_SucceedsNamespaceY", f.BearerToken, ruleInY2, http.StatusNoContent}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + status := updateAlertRuleWithToken(t, f, ctx, tc.token, tc.ruleID) + if status != tc.wantStatus { + t.Fatalf("Expected per-rule status %d, got %d", tc.wantStatus, status) + } + }) + } +} + +// waitForUpdateCacheSync polls until the relabeled-rules cache has synced by +// attempting a bulk-update probe with an unprivileged token. A 403 (Forbidden) +// per-rule status indicates the rule was found in cache and RBAC was evaluated. +func waitForUpdateCacheSync(t *testing.T, f *framework.Framework, ctx context.Context, token, ruleID string) { + t.Helper() + const timeout = 30 * time.Second + const interval = time.Second + deadline := time.Now().Add(timeout) + for { + status, err := tryUpdateAlertRule(f, ctx, token, ruleID) + if err == nil && (status == http.StatusForbidden || status == http.StatusNoContent) { + return + } + if time.Now().After(deadline) { + t.Fatalf("Cache sync timed out after %v (last status=%d, err=%v)", timeout, status, err) + } + if err != nil { + t.Logf("Cache sync: %v, retrying...", err) + } else { + t.Logf("Cache sync: per-rule status %d, retrying...", status) + } + time.Sleep(interval) + } +} + +// tryUpdateAlertRule attempts a single-rule bulk-update (label addition) and +// returns the per-rule status code without calling t.Fatal, making it suitable +// for polling loops. +func tryUpdateAlertRule(f *framework.Framework, ctx context.Context, token, ruleID string) (int, error) { + labelVal := "true" + payload := managementrouter.BulkUpdateAlertRulesRequest{ + RuleIds: []string{ruleID}, + Labels: &map[string]*string{"e2e_rbac_probe": &labelVal}, + } + 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") + 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() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return resp.StatusCode, fmt.Errorf("expected bulk response 200, got %d: %s", resp.StatusCode, string(body)) + } + + var updateResp managementrouter.BulkUpdateAlertRulesResponse + if err := json.NewDecoder(resp.Body).Decode(&updateResp); err != nil { + return 0, fmt.Errorf("decode update response: %w", err) + } + if len(updateResp.Rules) != 1 { + return 0, fmt.Errorf("expected 1 per-rule result, got %d", len(updateResp.Rules)) + } + return int(updateResp.Rules[0].StatusCode), nil +} + +// updateAlertRuleWithToken sends a bulk-update request for a single rule ID +// using the given bearer token and returns the per-rule HTTP status code. +func updateAlertRuleWithToken(t *testing.T, f *framework.Framework, ctx context.Context, token, ruleID string) int { + t.Helper() + + status, err := tryUpdateAlertRule(f, ctx, token, ruleID) + if err != nil { + t.Fatalf("Update request for rule %s failed: %v", ruleID, err) + } + return status +} + +func patchDrop(t *testing.T, f *framework.Framework, ctx context.Context, ruleID string, enable bool) { + t.Helper() + + patchReq := managementrouter.BulkUpdateAlertRulesRequest{ + RuleIds: []string{ruleID}, + AlertingRuleEnabled: &enable, + } + + reqBody, err := json.Marshal(patchReq) + if err != nil { + t.Fatalf("Failed to marshal drop/restore patch: %v", err) + } + + patchURL := f.PluginURL + "/api/v1/alerting/rules" + req, err := http.NewRequestWithContext(ctx, http.MethodPatch, patchURL, bytes.NewBuffer(reqBody)) + if err != nil { + t.Fatalf("Failed to create patch request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + if f.BearerToken != "" { + req.Header.Set("Authorization", "Bearer "+f.BearerToken) + } + + resp, err := f.HTTPClient().Do(req) + if err != nil { + t.Fatalf("Failed to make drop/restore request: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + t.Fatalf("Drop/restore: expected 200, got %d. Body: %s", resp.StatusCode, string(body)) + } +} + +func hasDropActionForRule(arc osmv1.AlertRelabelConfig, ruleID string) bool { + hasRuleID := arc.Annotations[managementlabels.ARCAnnotationAlertRuleIDKey] == ruleID + if !hasRuleID { + return false + } + for _, cfg := range arc.Spec.Configs { + if cfg.Action == "Drop" { + return true + } + } + return false +} + +func hasClassificationForRule(arc osmv1.AlertRelabelConfig, component, layer string) bool { + var hasComponent, hasLayer bool + for _, cfg := range arc.Spec.Configs { + if cfg.TargetLabel == "openshift_io_alert_rule_component" && cfg.Replacement == component { + hasComponent = true + } + if cfg.TargetLabel == "openshift_io_alert_rule_layer" && cfg.Replacement == layer { + hasLayer = true + } + } + return hasComponent && hasLayer +} + +func cleanupARCsForRule(t *testing.T, f *framework.Framework, ctx context.Context, namespace, ruleID string) { + t.Helper() + arcList, err := f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(namespace).List(ctx, metav1.ListOptions{}) + if err != nil { + t.Logf("cleanup: failed to list ARCs: %v", err) + return + } + for _, arc := range arcList.Items { + if arc.Annotations[managementlabels.ARCAnnotationAlertRuleIDKey] == ruleID { + if err := f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(namespace).Delete(ctx, arc.Name, metav1.DeleteOptions{}); err != nil { + t.Logf("cleanup: failed to delete ARC %s/%s: %v", namespace, arc.Name, err) + } else { + t.Logf("cleanup: deleted ARC %s/%s for rule %s", namespace, arc.Name, ruleID) + } + } + } +} From 0a977a3843772decae993cd86e6bf779e5263907 Mon Sep 17 00:00:00 2001 From: Shirly Radco Date: Wed, 12 Aug 2026 12:26:06 +0300 Subject: [PATCH 2/2] 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. 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 +--- .../managementrouter/alert_rule_delete.go | 23 + .../alert_rule_delete_test.go | 125 ++++++ .../managementrouter/alert_rule_update.go | 130 ++++++ .../alert_rule_update_test.go | 208 +++++++++ internal/managementrouter/api_generated.go | 64 ++- test/e2e/single_alert_rule_test.go | 411 ++++++++++++++++++ 10 files changed, 1183 insertions(+), 98 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 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..d42cef2b4 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`. +- 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_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..1a5d0e352 --- /dev/null +++ b/internal/managementrouter/alert_rule_update.go @@ -0,0 +1,130 @@ +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" + } + 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..4bc7aaae8 --- /dev/null +++ b/internal/managementrouter/alert_rule_update_test.go @@ -0,0 +1,208 @@ +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_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/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 +}