CNV-80608: management: add update alert rule APIs#1047
Open
sradco wants to merge 5 commits into
Open
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. |
Contributor
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a bulk ChangesAlert-rule bulk update workflow
Estimated code review effort: 5 (Critical) | ~120 minutes 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
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
sradco
force-pushed
the
alert-mgmt-restructured-05-update-rule
branch
from
79db1db to
7474fcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/managementrouter/alert_rule_bulk_update.go (1)
66-153: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake each rule update produce one coherent outcome.
The logic currently allows partial mutations and suppresses errors. If a toggle operation encounters a
NotAllowedError, it setsnotAllowedEnabled = trueand proceeds. If a subsequentClassificationorLabelsupdate succeeds, the final response returns204 No Contentbecause of the check on line 141 (payload.Labels == nil && payload.Classification == nil), hiding the fact that the toggle failed. Conversely, if the toggle succeeds but theClassificationorLabelsupdate fails, the rule is left partially updated while a failure status is returned.Preflight all requested changes and apply them as one management operation, or reject combined updates before any mutation. Return a success status only when every requested field for a rule 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 `@internal/managementrouter/alert_rule_bulk_update.go` around lines 66 - 153, Update the per-rule flow around the toggle, classification, and label operations so combined requests cannot produce partial mutations or hide failures. Preflight all requested changes and apply them atomically through the management client, or reject requests containing multiple update types before invoking any mutation; remove the notAllowedEnabled success path. Ensure each rule appends exactly one result, returning success only when every requested field succeeds and preserving the relevant error status otherwise.
🤖 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 `@pkg/management/alert_rule_preconditions.go`:
- Around line 52-56: Update validateUserUpdatePreconditions to also detect when
the parent PrometheusRule is GitOps-managed, alongside the existing
IsExternallyManagedObject check. Reuse the same GitOps management detection and
rejection behavior established by validateGitOpsPreconditions, so user-defined
rule edits return notAllowedOperatorUpdate() when either management mechanism
controls the parent.
---
Duplicate comments:
In `@internal/managementrouter/alert_rule_bulk_update.go`:
- Around line 66-153: Update the per-rule flow around the toggle,
classification, and label operations so combined requests cannot produce partial
mutations or hide failures. Preflight all requested changes and apply them
atomically through the management client, or reject requests containing multiple
update types before invoking any mutation; remove the notAllowedEnabled success
path. Ensure each rule appends exactly one result, returning success only when
every requested field succeeds and preserving the relevant error status
otherwise.
🪄 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: adddd952-4128-4a7b-939c-39c9f526a1ea
📒 Files selected for processing (19)
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_alert_rule_labels.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 (15)
- pkg/management/get_rule_by_id.go
- internal/managementrouter/router.go
- pkg/management/label_utils.go
- test/e2e/helpers_test.go
- pkg/management/get_rule_by_id_test.go
- pkg/management/types.go
- api/openapi.yaml
- test/e2e/update_alert_rule_test.go
- internal/managementrouter/api_generated.go
- internal/managementrouter/alert_rule_bulk_update_test.go
- pkg/management/delete_user_defined_alert_rule_by_id.go
- pkg/management/update_user_defined_alert_rule_test.go
- pkg/management/update_user_defined_alert_rule.go
- pkg/management/update_platform_alert_rule.go
- pkg/management/update_platform_alert_rule_test.go
sradco
force-pushed
the
alert-mgmt-restructured-05-update-rule
branch
5 times, most recently
from
23c541d to
e395513
Compare
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 (when ENABLE_USER_WORKLOAD_ARCS is enabled). Rename DropPlatformAlertRule and RestorePlatformAlertRule to DropAlertRule and RestoreAlertRule respectively. 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) - Add ErrSkip handling in e2e tests matching repo convention 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> Co-authored-by: Cursor <cursoragent@cursor.com>
sradco
force-pushed
the
alert-mgmt-restructured-05-update-rule
branch
from
e395513 to
bad70c3
Compare
validateUserUpdatePreconditions now checks both return values from IsExternallyManagedObject, returning notAllowedGitOpsEdit() when the parent PrometheusRule is managed by GitOps. Previously only operator-management was detected, allowing edits that a GitOps controller would immediately overwrite. Co-authored-by: Cursor <cursoragent@cursor.com>
gci requires project-local imports to be sorted lexically. Move internal/managementrouter before pkg/ entries. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 39-46: The generated documentation for
BulkUpdateAlertRulesRequest.AlertingRuleEnabled is outdated and describes toggle
conflicts as silently absorbed. Regenerate or update the AlertingRuleEnabled
contract in api_generated.go to state that combining it with labels or
classification returns HTTP 400, matching the validation in the bulk update
handler.
In `@pkg/management/alert_rule_preconditions.go`:
- Around line 89-112: The shared validatePlatformUpdatePreconditions logic must
allow the ARC fallback used by operator-managed platform rules instead of
rejecting ManagedByOperator on the relabeled rule or ARC. Add the narrow
carve-out needed for the UpdatePlatformAlertRule path, preserving existing
rejection behavior for other update paths, and add coverage for the
operator-managed branch in the relevant tests. The sibling site in
pkg/management/update_platform_alert_rule.go requires no direct change; it
identifies the caller relying on this precondition adjustment.
🪄 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: a2e1b0dc-9cea-4b48-a5cc-77c1744b250b
📒 Files selected for processing (19)
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_alert_rule_labels.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 (14)
- internal/managementrouter/router.go
- pkg/management/get_rule_by_id.go
- pkg/management/label_utils.go
- pkg/management/update_alert_rule_labels.go
- pkg/management/get_rule_by_id_test.go
- test/e2e/update_alert_rule_test.go
- internal/managementrouter/api_generated.go
- api/openapi.yaml
- test/e2e/helpers_test.go
- pkg/management/update_user_defined_alert_rule.go
- pkg/management/update_user_defined_alert_rule_test.go
- pkg/management/delete_user_defined_alert_rule_by_id.go
- pkg/management/update_platform_alert_rule_test.go
- internal/managementrouter/alert_rule_bulk_update_test.go
…docs Two fixes addressing CodeRabbit review: 1. Remove isRuleManagedByOperator check from validatePlatformUpdatePreconditions. The ARC path modifies a separate AlertRelabelConfig, not the operator-managed resource itself, so operator management should not block it. The check was preventing UpdatePlatformAlertRule from using its designed ARC fallback for operator-managed AlertingRules. 2. Update stale BulkUpdateAlertRulesRequest.AlertingRuleEnabled comment in api_generated.go to reflect that the toggle now works for both platform and user-defined rules and cannot be combined with labels/classification (400). Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
@sradco: all tests passed! 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. |
PeterYurkovich
left a comment
PeterYurkovich
left a comment
Contributor
There was a problem hiding this comment.
If there is an operator owned alert in a non-platform namespace is there any way to manage labels?
Contributor
|
/cc @simonpasquier |
| "github.com/openshift/monitoring-plugin/test/e2e/framework" | ||
| ) | ||
|
|
||
| func TestUpdateAlertRule_DropRestore(t *testing.T) { |
Contributor
There was a problem hiding this comment.
Do we have tests verifying that requests with unauthorized users are denied?
Contributor
There was a problem hiding this comment.
Ideally we need to cover
- User A has no permission to update PrometheusRule in any namespace
- Request fails for namespace Y
- User B has permission to update PrometheusRule in namespace Y
- Request succeeds for namespace Y
- Request fails for namespace Z
- User C has permission to update PrometheusRule in any namespace
- Request succeeds for namespace Y
- Request succeeds for namespace Z
Author
Unfortunately no. Users can only silence these alerts. They can then copy them and update to their needs. |
Address review comments from PeterYurkovich and simonpasquier: 1. Remove redundant isPlatform guard in UpdatePlatformAlertRule. The caller (updatePlatformRuleLabels) already guarantees platform context, so the AlertingRule CR lookup is now unconditional. The fallback to applyLabelChangesViaAlertRelabelConfig still handles cases where no AR exists. 2. Add RBAC e2e tests (TestUpdateAlertRule_RBAC) verifying that: - User A (no permissions) is denied in any namespace - User B (namespace-scoped) succeeds in namespace Y, fails in Z - User C (cluster-admin) succeeds everywhere Adds framework helpers CreateScopedUser and CreateUnprivilegedUser for creating ServiceAccounts with specific RBAC policies and obtaining short-lived tokens. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/management/update_platform_alert_rule.go (1)
78-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
notAllowedGitOpsEdithelper.For consistency and to reduce duplication, reuse the existing
notAllowedGitOpsEdit()helper instead of hardcoding the identical error message.♻️ Proposed refactor
- if gitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(ar); gitOpsManaged { - return &NotAllowedError{Message: "This alert is managed by GitOps; edit it in Git."} - } else if operatorManaged { + if gitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(ar); gitOpsManaged { + return notAllowedGitOpsEdit() + } else if operatorManaged {🤖 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 78 - 81, In the GitOps-managed branch of the alert update flow, replace the hardcoded NotAllowedError construction with the existing notAllowedGitOpsEdit() helper, preserving the current return behavior and leaving the operator-managed path unchanged.pkg/management/alert_rule_preconditions.go (1)
93-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsistently use
k8s.IsExternallyManagedObjectfor ARC resources.Both of these sites check GitOps management for an
AlertRelabelConfigby callingk8s.IsManagedByGitOpsmanually, whereas checks for other resources—and the Operator check for ARCs—use thek8s.IsExternallyManagedObjecthelper. Consolidating these checks improves consistency and reduces duplicate logic.
pkg/management/alert_rule_preconditions.go#L93-L105: consolidate the GitOps and operator checks into a singlegitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(arc)call.pkg/management/alert_rule_preconditions.go#L126-L130: replace the manual GitOps check withgitOpsManaged, _ := k8s.IsExternallyManagedObject(arc); if gitOpsManaged { ... }.♻️ Proposed refactors
pkg/management/alert_rule_preconditions.go#L93-L105- if arc != nil { - if k8s.IsManagedByGitOps(arc.Annotations, arc.Labels) { - return notAllowedGitOpsEdit() - } - } - // Note: operator-managed rules are intentionally allowed here. The ARC path - // modifies a separate AlertRelabelConfig resource, not the operator-managed - // PrometheusRule/AlertingRule, so the operator won't reconcile away changes. - if arc != nil { - if _, operatorManaged := k8s.IsExternallyManagedObject(arc); operatorManaged { - return notAllowedOperatorUpdate() - } - } + // Note: 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. + if arc != nil { + if gitOpsManaged, operatorManaged := k8s.IsExternallyManagedObject(arc); gitOpsManaged { + return notAllowedGitOpsEdit() + } else if operatorManaged { + return notAllowedOperatorUpdate() + } + }
pkg/management/alert_rule_preconditions.go#L126-L130- if arc != nil { - if k8s.IsManagedByGitOps(arc.Annotations, arc.Labels) { - return notAllowedGitOpsEdit() - } - } + if arc != nil { + if gitOpsManaged, _ := k8s.IsExternallyManagedObject(arc); gitOpsManaged { + 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 93 - 105, Consolidate ARC management detection in pkg/management/alert_rule_preconditions.go lines 93-105 by using one k8s.IsExternallyManagedObject(arc) result for both GitOps and operator checks, preserving their existing rejection behavior. At lines 126-130, replace the manual k8s.IsManagedByGitOps check with the helper’s GitOps result while ignoring the operator result.
🤖 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 `@test/e2e/framework/framework.go`:
- Around line 228-268: The resource-provisioning flow around the visible
ServiceAccount, Role, RoleBinding, and token creation calls must roll back
resources created earlier when a later step fails. Track creation ownership,
delete previously created resources before each error return, and update
CreateUnprivilegedUser to remove its ServiceAccount when token creation fails;
preserve AlreadyExists resources rather than deleting them.
- Around line 270-275: Stop discarding errors across the cleanup flows: in
test/e2e/framework/framework.go lines 270-275, update the cleanup closure to
attempt every deletion and return aggregated errors; at lines 298-301, return
the ServiceAccount deletion error. In test/e2e/update_alert_rule_test.go lines
263, 269, 283, and 291, register or report cleanup errors through the test
cleanup mechanisms for the relevant resources. At lines 367-369, handle the
io.ReadAll error before logging the response body.
In `@test/e2e/update_alert_rule_test.go`:
- Around line 296-318: Update the denied-case assertions in
UserA_NoPerms_FailsNamespaceY and UserB_ScopedPerms_FailsNamespaceZ to require
http.StatusForbidden exactly, replacing the current non-204 checks. Keep the
existing success assertion for UserB_ScopedPerms_SucceedsNamespaceY unchanged.
---
Nitpick comments:
In `@pkg/management/alert_rule_preconditions.go`:
- Around line 93-105: Consolidate ARC management detection in
pkg/management/alert_rule_preconditions.go lines 93-105 by using one
k8s.IsExternallyManagedObject(arc) result for both GitOps and operator checks,
preserving their existing rejection behavior. At lines 126-130, replace the
manual k8s.IsManagedByGitOps check with the helper’s GitOps result while
ignoring the operator result.
In `@pkg/management/update_platform_alert_rule.go`:
- Around line 78-81: In the GitOps-managed branch of the alert update flow,
replace the hardcoded NotAllowedError construction with the existing
notAllowedGitOpsEdit() helper, preserving the current return behavior and
leaving the operator-managed path unchanged.
🪄 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: 1c017622-1e8b-444c-a4d1-ffbe66c19dc9
📒 Files selected for processing (5)
internal/managementrouter/api_generated.gopkg/management/alert_rule_preconditions.gopkg/management/update_platform_alert_rule.gotest/e2e/framework/framework.gotest/e2e/update_alert_rule_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/managementrouter/api_generated.go
Comment on lines
+228
to
+268
| if _, err := f.Clientset.CoreV1().ServiceAccounts(namespace).Create(ctx, sa, metav1.CreateOptions{}); err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return nil, fmt.Errorf("creating service account %s/%s: %w", namespace, name, err) | ||
| } | ||
|
|
||
| role := &rbacv1.Role{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: name}, | ||
| Rules: []rbacv1.PolicyRule{{ | ||
| APIGroups: []string{apiGroup}, | ||
| Resources: resources, | ||
| Verbs: verbs, | ||
| }}, | ||
| } | ||
| if _, err := f.Clientset.RbacV1().Roles(namespace).Create(ctx, role, metav1.CreateOptions{}); err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return nil, fmt.Errorf("creating role %s/%s: %w", namespace, name, err) | ||
| } | ||
|
|
||
| rb := &rbacv1.RoleBinding{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: name}, | ||
| Subjects: []rbacv1.Subject{{ | ||
| Kind: rbacv1.ServiceAccountKind, | ||
| Name: name, | ||
| Namespace: namespace, | ||
| }}, | ||
| RoleRef: rbacv1.RoleRef{ | ||
| APIGroup: rbacv1.GroupName, | ||
| Kind: "Role", | ||
| Name: name, | ||
| }, | ||
| } | ||
| if _, err := f.Clientset.RbacV1().RoleBindings(namespace).Create(ctx, rb, metav1.CreateOptions{}); err != nil && !apierrors.IsAlreadyExists(err) { | ||
| return nil, fmt.Errorf("creating role binding %s/%s: %w", namespace, name, err) | ||
| } | ||
|
|
||
| expSeconds := int64(3600) | ||
| treq := &authv1.TokenRequest{ | ||
| Spec: authv1.TokenRequestSpec{ExpirationSeconds: &expSeconds}, | ||
| } | ||
| tokenResp, err := f.Clientset.CoreV1().ServiceAccounts(namespace).CreateToken(ctx, name, treq, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("requesting token for %s/%s: %w", namespace, name, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Roll back resources when user provisioning fails.
Role, RoleBinding, or token creation failures leave previously created resources behind. Track which resources were created and delete them before returning each error; CreateUnprivilegedUser should similarly remove its ServiceAccount if token creation fails.
Also applies to: 285-296
🤖 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 `@test/e2e/framework/framework.go` around lines 228 - 268, The
resource-provisioning flow around the visible ServiceAccount, Role, RoleBinding,
and token creation calls must roll back resources created earlier when a later
step fails. Track creation ownership, delete previously created resources before
each error return, and update CreateUnprivilegedUser to remove its
ServiceAccount when token creation fails; preserve AlreadyExists resources
rather than deleting them.
Comment on lines
+270
to
+275
| cleanup := func() error { | ||
| _ = f.Clientset.RbacV1().RoleBindings(namespace).Delete(ctx, name, metav1.DeleteOptions{}) | ||
| _ = f.Clientset.RbacV1().Roles(namespace).Delete(ctx, name, metav1.DeleteOptions{}) | ||
| _ = f.Clientset.CoreV1().ServiceAccounts(namespace).Delete(ctx, name, metav1.DeleteOptions{}) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Propagate and assert cleanup errors.
These sites discard errors, hiding leaked cluster resources and currently triggering errcheck.
test/e2e/framework/framework.go#L270-L275: attempt every deletion and return their aggregated errors.test/e2e/framework/framework.go#L298-L301: return the ServiceAccount deletion error.test/e2e/update_alert_rule_test.go#L263-L263: register cleanup witht.Cleanupand report its error.test/e2e/update_alert_rule_test.go#L269-L269: report namespace cleanup failures.test/e2e/update_alert_rule_test.go#L283-L283: report user cleanup failures.test/e2e/update_alert_rule_test.go#L291-L291: report scoped-user cleanup failures.test/e2e/update_alert_rule_test.go#L367-L369: handle theio.ReadAllerror before logging the body.
As per path instructions, “Never ignore error returns.” As per coding guidelines, “required CI checks must pass before merging.”
📍 Affects 2 files
test/e2e/framework/framework.go#L270-L275(this comment)test/e2e/framework/framework.go#L298-L301test/e2e/update_alert_rule_test.go#L263-L263test/e2e/update_alert_rule_test.go#L269-L269test/e2e/update_alert_rule_test.go#L283-L283test/e2e/update_alert_rule_test.go#L291-L291test/e2e/update_alert_rule_test.go#L367-L369
🤖 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 `@test/e2e/framework/framework.go` around lines 270 - 275, Stop discarding
errors across the cleanup flows: in test/e2e/framework/framework.go lines
270-275, update the cleanup closure to attempt every deletion and return
aggregated errors; at lines 298-301, return the ServiceAccount deletion error.
In test/e2e/update_alert_rule_test.go lines 263, 269, 283, and 291, register or
report cleanup errors through the test cleanup mechanisms for the relevant
resources. At lines 367-369, handle the io.ReadAll error before logging the
response body.
Sources: Coding guidelines, Path instructions, Linters/SAST tools
Comment on lines
+296
to
+318
| t.Run("UserA_NoPerms_FailsNamespaceY", func(t *testing.T) { | ||
| status := bulkUpdateLabelsWithToken(t, f, ctx, userA.Token, ruleIDY) | ||
| if status == http.StatusNoContent { | ||
| t.Fatalf("Expected user A to be denied, got per-rule status 204") | ||
| } | ||
| t.Logf("User A correctly denied for namespace Y (per-rule status: %d)", status) | ||
| }) | ||
|
|
||
| t.Run("UserB_ScopedPerms_SucceedsNamespaceY", func(t *testing.T) { | ||
| status := bulkUpdateLabelsWithToken(t, f, ctx, userB.Token, ruleIDY) | ||
| if status != http.StatusNoContent { | ||
| t.Fatalf("Expected user B to succeed in namespace Y, got per-rule status %d", status) | ||
| } | ||
| t.Log("User B correctly allowed for namespace Y") | ||
| }) | ||
|
|
||
| t.Run("UserB_ScopedPerms_FailsNamespaceZ", func(t *testing.T) { | ||
| status := bulkUpdateLabelsWithToken(t, f, ctx, userB.Token, ruleIDZ) | ||
| if status == http.StatusNoContent { | ||
| t.Fatalf("Expected user B to be denied for namespace Z, got per-rule status 204") | ||
| } | ||
| t.Logf("User B correctly denied for namespace Z (per-rule status: %d)", status) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require the expected authorization failure status.
Both denied cases pass for any non-204 result, including 404 or 500. Assert the documented authorization status—normally http.StatusForbidden—so unrelated failures cannot satisfy the RBAC test.
🤖 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 `@test/e2e/update_alert_rule_test.go` around lines 296 - 318, Update the
denied-case assertions in UserA_NoPerms_FailsNamespaceY and
UserB_ScopedPerms_FailsNamespaceZ to require http.StatusForbidden exactly,
replacing the current non-204 checks. Keep the existing success assertion for
UserB_ScopedPerms_SucceedsNamespaceY unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
/rules) with per-rule results.null/empty), classification overrides, and alert enable/disable with drop/restore.