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 |
WalkthroughThis PR adds bulk alert-rule updates through ChangesBulk Alert-Rule Update
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BulkUpdateAlertRules
participant ManagementClient
participant AlertRelabelConfig
Client->>BulkUpdateAlertRules: PATCH /rules
BulkUpdateAlertRules->>ManagementClient: process each rule ID
ManagementClient->>AlertRelabelConfig: apply labels or drop/restore state
AlertRelabelConfig-->>ManagementClient: resource result
ManagementClient-->>BulkUpdateAlertRules: per-rule status
BulkUpdateAlertRules-->>Client: BulkUpdateAlertRulesResponse
Suggested reviewers: 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 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
sradco
force-pushed
the
alert-mgmt-restructured-05-update-rule
branch
from
9997bc6 to
5a3282e
Compare
| 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
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
5a3282e to
79db1db
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/alert_rule_bulk_update.go`:
- Around line 68-123: The per-rule update flow in the bulk handler must produce
one atomic outcome: do not suppress a toggle NotAllowedError when classification
or labels are also requested, and do not report success after a partial
mutation. Update the logic around the visible toggle handling and classification
update calls to either preflight all requested changes and apply them as one
management operation, or reject combined updates before mutation; return 204
only when every requested field succeeds. Add regression coverage for combined
toggle, label, and classification updates.
In `@pkg/management/delete_user_defined_alert_rule_by_id.go`:
- Around line 146-154: Update cleanupARCForDeletedRule to propagate errors
returned by arcNamespaceForRule instead of unconditionally returning nil. Only
treat the explicit “ARC not applicable” result as a successful no-op, while
returning all other namespace-resolution failures to the caller.
- Around line 89-101: The delete flow around the primary AlertingRule
deletion/update and deleteAssociatedARC must remain recoverable when ARC cleanup
fails. Make cleanup idempotent or ensure a retry/reconciliation path can remove
the retained ARC even when the rule is already absent, while preserving the
existing primary operation behavior in both len(newGroups) branches.
In `@pkg/management/get_rule_by_id_test.go`:
- Around line 79-83: Update the error assertion in the test around the errors.As
call to stop execution immediately when the error is not a
management.NotFoundError, using the test framework’s fatal assertion so nf is
never dereferenced while nil. Preserve the existing diagnostic message and
Resource validation for successful type matches.
In `@pkg/management/update_platform_alert_rule.go`:
- Around line 366-374: After findARCByAlertRuleID returns a non-nil existingArc
in the fallback branch, apply the same shared ARC ownership validation used by
the normal path before proceeding to deletion or update. Preserve the existing
nil return for a missing ARC and ensure GitOps-managed ARCs cannot be restored
through the cache-miss path.
- Around line 404-413: Update findARCByAlertRuleID to return an error alongside
its existing results, and return any AlertRelabelConfigs().List failure instead
of continuing silently. Propagate that error through RestorePlatformAlertRule
and ensure restore does not report success when ARC discovery fails; update all
callers and return paths accordingly.
- Around line 208-214: Update the nextChanges-empty branch in the
AlertRelabelConfig update flow to preserve existingRuleDrops: rebuild or retain
the ARC with its stamp and rule drops when drops remain, rather than deleting it
solely because label overrides are gone. Delete the ARC only when both
nextChanges and existingRuleDrops are empty.
🪄 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: b66ef24d-96b5-4343-a538-f47ae1b1f33f
📒 Files selected for processing (18)
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/helpers_test.gotest/e2e/update_alert_rule_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/management/get_rule_by_id.go
- pkg/management/label_utils.go
- pkg/management/update_platform_alert_rule_test.go
- internal/managementrouter/api_generated.go
- test/e2e/update_alert_rule_test.go
- pkg/management/update_user_defined_alert_rule.go
- pkg/management/alert_rule_preconditions.go
- pkg/management/update_user_defined_alert_rule_test.go
- pkg/management/types.go
- api/openapi.yaml
| notAllowedEnabled := false | ||
| if haveToggle { | ||
| var err error | ||
| if !enabled { | ||
| err = hr.managementClient.DropPlatformAlertRule(req.Context(), id) | ||
| } else { | ||
| err = hr.managementClient.RestorePlatformAlertRule(req.Context(), id) | ||
| } | ||
| if err != nil { | ||
| var na *management.NotAllowedError | ||
| if errors.As(err, &na) { | ||
| notAllowedEnabled = true | ||
| } else { | ||
| status, message := parseError(err) | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(status), | ||
| Message: &message, | ||
| }) | ||
| continue | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if payload.Classification != nil { | ||
| cl := payload.Classification | ||
| update := management.UpdateRuleClassificationRequest{RuleId: id} | ||
| if cl.ComponentSet { | ||
| update.Component = cl.Component | ||
| update.ComponentSet = true | ||
| } | ||
| if cl.LayerSet { | ||
| update.Layer = cl.Layer | ||
| update.LayerSet = true | ||
| } | ||
| if cl.ComponentFromSet { | ||
| update.ComponentFrom = cl.ComponentFrom | ||
| update.ComponentFromSet = true | ||
| } | ||
| if cl.LayerFromSet { | ||
| update.LayerFrom = cl.LayerFrom | ||
| update.LayerFromSet = true | ||
| } | ||
|
|
||
| if update.ComponentSet || update.LayerSet || update.ComponentFromSet || update.LayerFromSet { | ||
| if err := hr.managementClient.UpdateAlertRuleClassification(req.Context(), update); err != nil { | ||
| status, message := parseError(err) | ||
| results = append(results, UpdateAlertRuleResult{ | ||
| Id: id, | ||
| StatusCode: int32(status), | ||
| Message: &message, | ||
| }) | ||
| continue | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make each rule update produce one coherent outcome.
A toggle NotAllowedError is suppressed when labels or classification are also present, so the response can return 204 although the toggle failed. Conversely, successful toggle/classification mutations remain committed when a later operation fails. Since the response exposes only one status per rule, preflight and apply these fields through one management operation, or reject combined updates; return 204 only when every requested change succeeds. Add regression coverage for combined fields.
Also applies to: 125-215
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 83-83: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(status)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 116-116: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(status)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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.go` around lines 68 - 123,
The per-rule update flow in the bulk handler must produce one atomic outcome: do
not suppress a toggle NotAllowedError when classification or labels are also
requested, and do not report success after a partial mutation. Update the logic
around the visible toggle handling and classification update calls to either
preflight all requested changes and apply them as one management operation, or
reject combined updates before mutation; return 204 only when every requested
field succeeds. Add regression coverage for combined toggle, label, and
classification updates.
| 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) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make post-delete ARC cleanup retryable.
Both branches commit the primary update or deletion before cleaning the ARC. If cleanup fails, the caller receives an error although the rule is already gone; a retry then cannot locate the rule to finish cleanup. Make this lifecycle idempotent or provide reconciliation for the retained ARC.
Also applies to: 132-143
🤖 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/delete_user_defined_alert_rule_by_id.go` around lines 89 -
101, The delete flow around the primary AlertingRule deletion/update and
deleteAssociatedARC must remain recoverable when ARC cleanup fails. Make cleanup
idempotent or ensure a retry/reconciliation path can remove the retained ARC
even when the rule is already absent, while preserving the existing primary
operation behavior in both len(newGroups) branches.
| // 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 no ARC exists. | ||
| 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 { | ||
| return nil | ||
| } | ||
| return c.deleteAssociatedARC(ctx, arcNamespace, name, alertRuleId) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Propagate ARC namespace-resolution failures.
Returning nil for every arcNamespaceForRule error reports successful cleanup while potentially leaving a stale ARC, and it triggers the nilerr lint failure. Only suppress an explicit “ARC not applicable” outcome.
Proposed fix
arcNamespace, err := c.arcNamespaceForRule(nn)
if err != nil {
- return nil
+ return fmt.Errorf("failed to resolve ARC namespace for %s/%s: %w", namespace, name, err)
}As per path instructions, “Never ignore error returns.”
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 152-152: error is not nil (line 150) but it returns nil
(nilerr)
🤖 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/delete_user_defined_alert_rule_by_id.go` around lines 146 -
154, Update cleanupARCForDeletedRule to propagate errors returned by
arcNamespaceForRule instead of unconditionally returning nil. Only treat the
explicit “ARC not applicable” result as a successful no-op, while returning all
other namespace-resolution failures to the caller.
Sources: Path instructions, Linters/SAST tools
| var nf *management.NotFoundError | ||
| if !errors.As(err, &nf) { | ||
| t.Errorf("expected NotFoundError, got %T: %v", err, err) | ||
| } | ||
| if nf.Resource != "AlertRule" { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Stop the test before dereferencing a failed errors.As target.
If the implementation returns another error type, nf remains nil and Line 83 panics.
Proposed fix
var nf *management.NotFoundError
if !errors.As(err, &nf) {
- t.Errorf("expected NotFoundError, got %T: %v", err, err)
+ t.Fatalf("expected NotFoundError, got %T: %v", err, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var nf *management.NotFoundError | |
| if !errors.As(err, &nf) { | |
| t.Errorf("expected NotFoundError, got %T: %v", err, err) | |
| } | |
| if nf.Resource != "AlertRule" { | |
| var nf *management.NotFoundError | |
| if !errors.As(err, &nf) { | |
| t.Fatalf("expected NotFoundError, got %T: %v", err, err) | |
| } | |
| if nf.Resource != "AlertRule" { |
🤖 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_test.go` around lines 79 - 83, Update the error
assertion in the test around the errors.As call to stop execution immediately
when the error is not a management.NotFoundError, using the test framework’s
fatal assertion so nf is never dereferenced while nil. Preserve the existing
diagnostic message and Resource validation for successful type matches.
| if len(nextChanges) == 0 { | ||
| if found { | ||
| 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 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the rule Drop when removing the final label override.
When nextChanges is empty, deleting the entire ARC also deletes existingRuleDrops, silently restoring a disabled alert. Rebuild the ARC with its stamp and preserved rule Drops instead; delete it only when neither overrides nor rule Drops remain.
Proposed fix
if len(nextChanges) == 0 {
- if found {
- if err := c.k8sClient.AlertRelabelConfigs().Delete(ctx, namespace, arcName); err != nil {
- return fmt.Errorf("failed to delete AlertRelabelConfig %s/%s: %w", namespace, arcName, err)
- }
+ 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, true, 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
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(nextChanges) == 0 { | |
| if found { | |
| 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 | |
| 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, true, 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 | |
| } |
🤖 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` around lines 208 - 214, Update
the nextChanges-empty branch in the AlertRelabelConfig update flow to preserve
existingRuleDrops: rebuild or retain the ARC with its stamp and rule drops when
drops remain, rather than deleting it solely because label overrides are gone.
Delete the ARC only when both nextChanges and existingRuleDrops are empty.
| } 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. | ||
| arcNamespace, existingArc, arcName = c.findARCByAlertRuleID(ctx, alertRuleId) | ||
| if existingArc == nil { | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce ownership preconditions on fallback restores.
The normal path validates existingArc, but the cache-miss fallback proceeds directly to deletion or update. A dropped rule can therefore restore by modifying a GitOps-managed ARC. Apply the shared ARC ownership validation after findARCByAlertRuleID succeeds.
🤖 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` around lines 366 - 374, After
findARCByAlertRuleID returns a non-nil existingArc in the fallback branch, apply
the same shared ARC ownership validation used by the normal path before
proceeding to deletion or update. Preserve the existing nil return for a missing
ARC and ensure GitOps-managed ARCs cannot be restored through the cache-miss
path.
| func (c *client) findARCByAlertRuleID(ctx context.Context, alertRuleId string) (string, *osmv1.AlertRelabelConfig, string) { | ||
| namespaces := []string{k8s.ClusterMonitoringNamespace} | ||
| if c.enableUserWorkloadARCs { | ||
| namespaces = append(namespaces, k8s.UserWorkloadMonitoringNamespace) | ||
| } | ||
| for _, ns := range namespaces { | ||
| arcs, err := c.k8sClient.AlertRelabelConfigs().List(ctx, ns) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate ARC listing failures instead of reporting a successful restore.
A permissions, API, or context error is discarded, causing RestorePlatformAlertRule to return success while leaving the alert dropped. Return an error from findARCByAlertRuleID and propagate it through the caller.
As per path instructions, Go security requires: “Never ignore error returns.”
🤖 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` around lines 404 - 413, Update
findARCByAlertRuleID to return an error alongside its existing results, and
return any AlertRelabelConfigs().List failure instead of continuing silently.
Propagate that error through RestorePlatformAlertRule and ensure restore does
not report success when ARC discovery fails; update all callers and return paths
accordingly.
Source: Path instructions
|
@sradco: The following test 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. |
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
/rules) supporting per-rule outcomes in one request.null), classification overrides, and enable/disable toggles.Bug Fixes
Tests