CNV-80608: management: add update alert rule APIs#1047
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sradco: This pull request references CNV-80608 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sradco The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (17)
WalkthroughThis PR adds a bulk alert-rule update capability: a new ChangesBulk Update Alert Rules
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BulkUpdateAlertRules
participant ManagementClient
participant K8sAPI
Client->>BulkUpdateAlertRules: PATCH /rules {ruleIds, labels, classification, alertingRuleEnabled}
loop each ruleId
BulkUpdateAlertRules->>ManagementClient: DropPlatformAlertRule/RestorePlatformAlertRule
ManagementClient->>K8sAPI: upsert/delete AlertRelabelConfig
BulkUpdateAlertRules->>ManagementClient: UpdateAlertRuleClassification
ManagementClient->>K8sAPI: update classification labels
BulkUpdateAlertRules->>ManagementClient: UpdatePlatformAlertRule
ManagementClient->>K8sAPI: update AlertingRule or AlertRelabelConfig
ManagementClient-->>BulkUpdateAlertRules: NotAllowedError (fallback)
BulkUpdateAlertRules->>ManagementClient: UpdateUserDefinedAlertRule
ManagementClient->>K8sAPI: update PrometheusRule
ManagementClient-->>BulkUpdateAlertRules: new rule id / error
end
BulkUpdateAlertRules-->>Client: BulkUpdateAlertRulesResponse{Rules: results}
🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
pkg/management/get_rule_by_id.go (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a doc comment on the exported method.
GetRuleByIdis an exported method but lacks a doc comment beginning with the method name. As per coding guidelines, "Exported Go functions and methods must have doc comments beginning with the function name".📝 Proposed fix
+// GetRuleById retrieves a specific alert rule by its ID, returning a +// NotFoundError when the rule is not present. func (c *client) GetRuleById(ctx context.Context, alertRuleId string) (monitoringv1.Rule, error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/management/get_rule_by_id.go` at line 9, Add a Go doc comment for the exported client method GetRuleById so it begins with the method name and describes what it returns; place it directly above GetRuleById on the client type, following the same comment style used for exported methods in this package.Source: Coding guidelines
pkg/management/alert_rule_preconditions.go (1)
87-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: reuse
validateGitOpsPreconditionsto avoid duplicated GitOps checks.Lines 88-95 replicate the rule-label and parent-
PrometheusRuleGitOps checks already implemented invalidateGitOpsPreconditions(Lines 76-83). Delegating keeps the two paths in sync if GitOps gating logic changes.♻️ Proposed refactor
func validatePlatformUpdatePreconditions(relabeled monitoringv1.Rule, pr *monitoringv1.PrometheusRule, arc *osmv1.AlertRelabelConfig) error { - if isRuleManagedByGitOpsLabel(relabeled) { - return notAllowedGitOpsEdit() - } - if pr != nil { - if gitOpsManaged, _ := k8s.IsExternallyManagedObject(pr); gitOpsManaged { - return notAllowedGitOpsEdit() - } - } + if err := validateGitOpsPreconditions(relabeled, pr); err != nil { + return err + } if arc != nil { if k8s.IsManagedByGitOps(arc.Annotations, arc.Labels) { return notAllowedGitOpsEdit() } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/management/alert_rule_preconditions.go` around lines 87 - 95, The GitOps checks in validatePlatformUpdatePreconditions are duplicated from validateGitOpsPreconditions, so update validatePlatformUpdatePreconditions to delegate to validateGitOpsPreconditions instead of repeating the rule-label and parent PrometheusRule checks. Keep the existing behavior by passing the same relabeled, pr, and arc inputs through the shared helper so the GitOps gating logic stays centralized and consistent.internal/managementrouter/alert_rule_bulk_update_test.go (2)
152-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
httptest.NewRequestWithContextinstead ofhttptest.NewRequest.golangci-lint's
noctxlinter flags three calls tohttptest.NewRequest(lines 158, 322, 342). If this linter is enabled in CI, these will fail the build.🔧 Fix: pass context to httptest.NewRequest
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.NewRequest(http.MethodPatch, "/api/v1/alerting/rules", bytes.NewReader(buf)) + 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 }Apply the same change to lines 322 and 342:
- req := httptest.NewRequest(http.MethodPatch, "/api/v1/alerting/rules", bytes.NewBufferString("{")) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPatch, "/api/v1/alerting/rules", bytes.NewBufferString("{"))- req := httptest.NewRequest(http.MethodPatch, "/api/v1/alerting/rules", bytes.NewReader(large)) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPatch, "/api/v1/alerting/rules", bytes.NewReader(large))Also applies to: 320-333, 335-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/managementrouter/alert_rule_bulk_update_test.go` around lines 152 - 163, The test helper and the other two request builders in alert_rule_bulk_update_test are using httptest.NewRequest, which trips the noctx linter. Update do and the two other request-creation sites to use httptest.NewRequestWithContext instead, passing an appropriate context such as context.Background() or the test context already available. Keep the same request method, URL, body, and headers, and make sure the change is applied consistently across the helper and the related bulk update tests.
176-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood test coverage — consider adding combined-operation tests.
The suite covers validation, label updates (set/drop via empty-string and null), toggle (drop/restore), classification, mixed platform/user, not-found, and invalid rule IDs. Two gaps worth considering:
- Toggle + labels combined: Verify the "silently absorbed" behavior when
alertingRuleEnabledis set on a user-defined rule alongside labels that succeed (spec says result should be 204).- Toggle + classification combined: Same absorbed-rejection behavior with classification instead of labels.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/managementrouter/alert_rule_bulk_update_test.go` around lines 176 - 579, Add two combined-operation tests in TestBulkUpdateAlertRules: one for alertingRuleEnabled with labels on a user-defined rule, and one for alertingRuleEnabled with classification. Use the existing bulk-update fixture/helpers (newBUFixture, f.do, f.rebuild, buFixtureIDs) to assert the user-rule toggle rejection is silently absorbed while the successful labels/classification update still returns 204 for the rule and 200 overall. Reuse the same response shape checks used in the other bulk update tests to confirm the returned rule status codes.pkg/management/update_user_defined_alert_rule.go (1)
128-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
alertNameparameter is unused.The body derives the alert name from the re-fetched
updatedRule.Alert(Lines 196, 199) and never references thealertNameargument. Either drop the parameter or use it in place of the re-fetch to avoid confusion about intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/management/update_user_defined_alert_rule.go` around lines 128 - 135, The migrateClassificationOverrideIfRuleIDChanged function currently accepts alertName but never uses it, while the alert name is re-derived from updatedRule.Alert later in the flow. Either remove the alertName parameter from the function signature and its callers, or replace the re-fetched alert name usage with this argument consistently so the intent is clear and the API does not expose an unused parameter.pkg/management/update_platform_alert_rule.go (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd doc comments to exported methods.
UpdatePlatformAlertRule(Line 31),DropPlatformAlertRule(Line 279), andRestorePlatformAlertRule(Line 335) are exported but lack doc comments beginning with the method name.As per coding guidelines, "Exported Go functions and methods must have doc comments beginning with the function name."
📝 Example
+// UpdatePlatformAlertRule applies label overrides to a platform alert rule via +// its AlertingRule or AlertRelabelConfig. func (c *client) UpdatePlatformAlertRule(ctx context.Context, alertRuleId string, alertRule monitoringv1.Rule) error {Also applies to: 279-279, 335-335
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/management/update_platform_alert_rule.go` at line 31, Add Go doc comments for the exported methods in this client: UpdatePlatformAlertRule, DropPlatformAlertRule, and RestorePlatformAlertRule. Each comment should start with the exact method name and briefly describe what the method does, so the exported API follows the standard Go documentation convention. Update the declarations in the client type near those method definitions to include the missing comments.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/managementrouter/router.go`:
- Around line 107-118: `parseParam` is incorrectly URL-decoding rule IDs coming
from JSON body payloads, which can alter literal `%` sequences and differs from
the bulk delete flow. Update the bulk update path that uses `parseParam` to trim
the JSON `payload.RuleIds` values directly with `strings.TrimSpace`, matching
`user_defined_alert_rule_bulk_delete.go`, and keep `parseParam` reserved for
actual URL path parameters where `url.PathUnescape` belongs.
In `@pkg/management/delete_user_defined_alert_rule_by_id.go`:
- Around line 161-164: In deleteAssociatedARC, the current `if err != nil ||
!found { return nil }` path hides real `AlertRelabelConfigs().Get` failures by
treating them the same as a missing ARC. Update the lookup handling so `found ==
false` still returns nil, but any non-nil error is returned to the caller; keep
the existing delete flow in deleteAssociatedARC and let the higher-level caller
observe genuine API failures instead of silently skipping them.
---
Nitpick comments:
In `@internal/managementrouter/alert_rule_bulk_update_test.go`:
- Around line 152-163: The test helper and the other two request builders in
alert_rule_bulk_update_test are using httptest.NewRequest, which trips the noctx
linter. Update do and the two other request-creation sites to use
httptest.NewRequestWithContext instead, passing an appropriate context such as
context.Background() or the test context already available. Keep the same
request method, URL, body, and headers, and make sure the change is applied
consistently across the helper and the related bulk update tests.
- Around line 176-579: Add two combined-operation tests in
TestBulkUpdateAlertRules: one for alertingRuleEnabled with labels on a
user-defined rule, and one for alertingRuleEnabled with classification. Use the
existing bulk-update fixture/helpers (newBUFixture, f.do, f.rebuild,
buFixtureIDs) to assert the user-rule toggle rejection is silently absorbed
while the successful labels/classification update still returns 204 for the rule
and 200 overall. Reuse the same response shape checks used in the other bulk
update tests to confirm the returned rule status codes.
In `@pkg/management/alert_rule_preconditions.go`:
- Around line 87-95: The GitOps checks in validatePlatformUpdatePreconditions
are duplicated from validateGitOpsPreconditions, so update
validatePlatformUpdatePreconditions to delegate to validateGitOpsPreconditions
instead of repeating the rule-label and parent PrometheusRule checks. Keep the
existing behavior by passing the same relabeled, pr, and arc inputs through the
shared helper so the GitOps gating logic stays centralized and consistent.
In `@pkg/management/get_rule_by_id.go`:
- Line 9: Add a Go doc comment for the exported client method GetRuleById so it
begins with the method name and describes what it returns; place it directly
above GetRuleById on the client type, following the same comment style used for
exported methods in this package.
In `@pkg/management/update_platform_alert_rule.go`:
- Line 31: Add Go doc comments for the exported methods in this client:
UpdatePlatformAlertRule, DropPlatformAlertRule, and RestorePlatformAlertRule.
Each comment should start with the exact method name and briefly describe what
the method does, so the exported API follows the standard Go documentation
convention. Update the declarations in the client type near those method
definitions to include the missing comments.
In `@pkg/management/update_user_defined_alert_rule.go`:
- Around line 128-135: The migrateClassificationOverrideIfRuleIDChanged function
currently accepts alertName but never uses it, while the alert name is
re-derived from updatedRule.Alert later in the flow. Either remove the alertName
parameter from the function signature and its callers, or replace the re-fetched
alert name usage with this argument consistently so the intent is clear and the
API does not expose an unused parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c5fe80fb-3711-47a6-a729-28a333d7b34d
📒 Files selected for processing (17)
api/openapi.yamlinternal/managementrouter/alert_rule_bulk_update.gointernal/managementrouter/alert_rule_bulk_update_test.gointernal/managementrouter/api_generated.gointernal/managementrouter/router.gointernal/managementrouter/user_defined_alert_rule_bulk_delete.gopkg/management/alert_rule_preconditions.gopkg/management/delete_user_defined_alert_rule_by_id.gopkg/management/get_rule_by_id.gopkg/management/get_rule_by_id_test.gopkg/management/label_utils.gopkg/management/types.gopkg/management/update_platform_alert_rule.gopkg/management/update_platform_alert_rule_test.gopkg/management/update_user_defined_alert_rule.gopkg/management/update_user_defined_alert_rule_test.gotest/e2e/update_alert_rule_test.go
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. Signed-off-by: Shirly Radco <sradco@redhat.com> Signed-off-by: João Vilaça <jvilaca@redhat.com> Signed-off-by: Aviv Litman <alitman@redhat.com> Co-authored-by: AI Assistant <noreply@cursor.com>
sradco
force-pushed
the
alert-mgmt-restructured-05-update-rule
branch
from
9997bc6 to
5a3282e
Compare
|
@sradco: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| type: boolean | ||
| nullable: true | ||
| description: > | ||
| When false, drops (silences) the platform alert rule. |
There was a problem hiding this comment.
I don't think it is accurate to state that dropping an alert silences it, they are two separate actions
| // 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") |
There was a problem hiding this comment.
can we include the error in the response as well so that rather than trying to guess at what went wrong the caller can directly make an adjustment
| } | ||
|
|
||
| if payload.AlertingRuleEnabled == nil && payload.Labels == nil && payload.Classification == nil { | ||
| writeError(w, http.StatusBadRequest, "alertingRuleEnabled (toggle drop/restore) or labels (set/unset) or classification is required") |
There was a problem hiding this comment.
| writeError(w, http.StatusBadRequest, "alertingRuleEnabled (toggle drop/restore) or labels (set/unset) or classification is required") | |
| writeError(w, http.StatusBadRequest, "one of alertingRuleEnabled (toggle drop/restore) or labels (set/unset) or classification is required") |
| if id == "" { | ||
| msg := "missing ruleId" | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: rawId, |
There was a problem hiding this comment.
Won't this just be a whitespace only string?
|
|
||
| notAllowedEnabled := false | ||
| if haveToggle { | ||
| var derr error |
There was a problem hiding this comment.
Other checks use err, I don't see any reason to have this one have a different name
| return c.applyLabelChangesViaAlertRelabelConfig(ctx, arcNamespace, alertRuleId, *originalRule, alertRule.Labels) | ||
| } | ||
| return c.updateAlertingRuleLabels(ctx, ar, originalRule.Alert, alertRuleId, alertRule.Labels, arName) | ||
| } |
There was a problem hiding this comment.
Should !arFound be an error case?
| return protectedLabels[label] | ||
| } | ||
|
|
||
| var validSeverities = map[string]bool{ |
There was a problem hiding this comment.
This may have already been gone over, but alertmanager doesn't have a set of valid severities, and user's can put whatever strings they want as a severity. Was it a design decision to only allow these 4?
| updatedUserRule := currentRule | ||
| updatedUserRule.Labels = userLabels | ||
|
|
||
| newRuleId, err := hr.managementClient.UpdateUserDefinedAlertRule(req.Context(), id, updatedUserRule) |
There was a problem hiding this comment.
Shouldn't we determine if an alert rule is a platform or user defined ar before just trying platform and then falling back to user on a not allowed error?
| if !enabled { | ||
| derr = hr.managementClient.DropPlatformAlertRule(req.Context(), id) | ||
| } else { | ||
| derr = hr.managementClient.RestorePlatformAlertRule(req.Context(), id) |
There was a problem hiding this comment.
Is it not possible to drop/restore user ar's? For example if an ar is gitops or operator managed?
| } | ||
|
|
||
| if update.ComponentSet || update.LayerSet || update.ComponentFromSet || update.LayerFromSet { | ||
| if err := hr.managementClient.UpdateAlertRuleClassification(req.Context(), update); err != nil { |
There was a problem hiding this comment.
I think looking more at the code it would be best to have the label update code and the drop/restore code also flow through a single function similar to UpdateAlertRuleClassification for the classification rather than being split into platform and user functions which are called in this http method. The difference between user and platform seems that it is being mixed in with whether something is gitops or operator managed. I think having a single point of entry for those actions which can perform all of the validation and route the actions to user or platform would help clarify the codepaths quite a bit
3 participants
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.
Signed-off-by: Shirly Radco sradco@redhat.com
Signed-off-by: João Vilaça jvilaca@redhat.com
Signed-off-by: Aviv Litman alitman@redhat.com
Co-authored-by: AI Assistant noreply@cursor.com
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes