Keep alert settings a config save leaves out (#2689)

## Problem

Saving anything on Alerts > Thresholds turned flapping detection and
alert TTL cleanup off. The page's `PUT /api/alerts/config` body never
carries `flappingEnabled`, `maxAlertAgeDays`, `maxAcknowledgedAgeDays`,
`autoAcknowledgeAfterHours` or `customRules` (the page has no control
for any of them). `UpdateAlertConfig` decoded the body into a zero
`AlertConfig`, so every save wrote `false`/`0` for those fields, reset
the flapping tuning to defaults, dropped custom rules, and persisted
that. The `flapping`, `aggregation` and `ioNormalization` objects the
page sends are not `AlertConfig` fields and never enabled anything.

## Fix

- `PUT /api/alerts/config` now replaces only the top-level keys the body
carries. A key left out keeps its stored value. A sent key replaces its
setting whole, so `"overrides": {}` still clears overrides, and the
usual normalization still runs. A body that is not a JSON object
(including `null`) is a 400 and changes nothing.
- `alertconfig.ApplyAlertConfigUpdate` decodes the body on its own, so
sent keys get exact `encoding/json` semantics, including duplicates and
case-folded keys. It copies every unsent top-level field from a JSON
clone of the stored config.
- `Manager.ApplyConfigUpdate` reads, merges and applies under the
manager lock, so concurrent partial saves cannot revert each other. It
returns an owned snapshot, which the handler persists, so
`SaveAlertConfig`'s in-place normalization no longer writes into the
live config's maps. The handler runs one save at a time, so successful
saves reach `alerts.json` in the order they were applied.
- Stored configs are not rewritten; a value an earlier save zeroed stays
as it is.

## Verification

- Unit: `internal/alerts/config/update_test.go`; manager save path and
concurrent partial updates in
`internal/alerts/config_validation_test.go`; handler against a real
manager in `internal/api/alerting/alerts_test.go`. Each new regression
fails against main's code (overlay negative controls). The concurrency
test fails at round 0 against a non-atomic variant.
- `go test ./internal/alerts/... ./internal/api/alerting/` and the
`internal/api` alert endpoint, tenant and scope tests pass; the alert
tests pass under `-race`.
- Private stack (mock mode, Vite on main's frontend):
- On main, a node CPU change saved from the Thresholds page took
flapping and all three TTLs to false/0, in memory and in `alerts.json`.
- With this change, the same UI save kept them (true/7/1/24) and stored
the new threshold.
- The API repro (GET, drop the four keys, PUT) keeps them; explicit
`false`/`0` still applies; `null` and `[]` return 400.
- Five rounds of concurrent partial PUTs kept both changes in memory and
on disk.

## Follow-ups

- The thresholds page still sends the dead
`flapping`/`aggregation`/`ioNormalization` objects and hard-coded
`minimumDelta`/`suppressionWindow`/`hysteresisMargin`. A frontend
cleanup will follow once the open PRs touching
`alertsConfigurationModel.ts` land.
- `SaveAlertConfig` still normalizes caller-owned maps in place for
other callers (`ActivateAlerts`, startup paths), and the GET handler
encodes a shallow copy. That pre-existing race is tracked separately.
This commit is contained in:
rcourtman
2026-10-07 22:23:41 +01:00
committed by GitHub
13 changed files with 651 additions and 8 deletions
+3 -1
View File
@@ -891,7 +891,9 @@ For an existing incident whose notifications should be paused without clearing
it, use the snooze and unsnooze endpoints below.
- `GET /api/alerts/config`
- `PUT /api/alerts/config`
- `PUT /api/alerts/config` — replaces only the top-level keys the body
carries; a key left out keeps its stored value, and the merged config is
normalized as before. The body must be a JSON object.
- `GET /api/alerts/deadman/config` — returns only whether an external watchdog
is configured; `pingUrl` is `***REDACTED***` when present and never returns
the credential-bearing URL
@@ -3690,6 +3690,14 @@ Agent` secondary handoff against the live setup wizard instead of relying
## Current State
### Alert settings saves do not touch agent lifecycle
`PUT /api/alerts/config` in `internal/api/alerting/alerts.go` now keeps stored
values for the top-level keys a client leaves out. Agent alert defaults
(`agentDefaults`) follow the same rule as every other key; enrollment,
reporting leases, commands and agent removal are unchanged.
`internal/api/alerting/alerts_test.go` pins the handler.
### VM guest execution admission (backup precaution)
`AgentRegisterPayload.guest_exec_guard_version` is optional; version 1 on a
@@ -1114,6 +1114,43 @@ history and affirmative recovery. This is source-fixture lifecycle proof, not
an installed registry/destination result or a diagnosis of a reporter's daily
resolve/reopen cycle.
### A settings save keeps the alert settings it did not send
`PUT /api/alerts/config` applies the body to the stored configuration through
`Manager.ApplyConfigUpdate` and `ApplyAlertConfigUpdate` in
`internal/alerts/config/update.go`: a top-level key the client sent replaces
that setting whole, a key it left out keeps the stored value, and the result
is normalized as any update is. The thresholds page has no control for
flapping detection (`flappingEnabled` and its window, threshold and cooldown),
alert TTL cleanup (`maxAlertAgeDays`, `maxAcknowledgedAgeDays`,
`autoAcknowledgeAfterHours`) or `customRules`, so it never sends them. The
handler used to decode the body into a zero `AlertConfig`, so every thresholds
save turned flapping detection and TTL cleanup off, reset the flapping tuning
to its defaults, dropped custom rules, and persisted that. The page's
`flapping`, `aggregation` and `ioNormalization` payload objects are not
`AlertConfig` fields and never enabled anything. A stored config keeps what it
holds, including values an earlier save zeroed; nothing rewrites them.
The body is decoded on its own, so sent keys mean exactly what the decoder
makes of them, duplicates and case-folded keys included. Unsent fields are
copied from a JSON round trip of the stored config, so the result shares no
maps or slices with it. `ApplyConfigUpdate` reads, merges and applies under
the manager lock, so two concurrent partial saves cannot revert each other in
memory, and returns a JSON-cloned snapshot so that persistence, which
normalizes the config it is handed in place, never writes into the live
config's maps.
`internal/alerts/config/update_test.go` covers unsent, explicit-off,
case-folded, duplicate-key, malformed and full-body updates, a sent object
replacing its stored value whole (a sent `truenasDiskDefaults: {}` does not
inherit the stored temperature) and the no-sharing rule.
`TestUpdateConfigKeepsSettingsAThresholdsSaveDidNotSend` and
`TestApplyConfigUpdateKeepsConcurrentPartialUpdates` in
`config_validation_test.go` run saves through the manager, and
`TestUpdateAlertConfig_KeepsStoredValuesForUnsentKeys` in
`internal/api/alerting/alerts_test.go` drives the handler against a real
manager.
### Confirmed empty storage is recovery evidence
Static storage capacity evaluation must admit a zero usage observation when
@@ -10293,6 +10293,22 @@ boundaries. Alert evaluation after that save remains alerts-owned and must use
the persisted trigger/recovery pair plus a valid derived critical threshold;
the API must not invent a TrueNAS-specific threshold sidecar.
`PUT /api/alerts/config` replaces only the top-level keys the body carries
and keeps the stored value of every key it leaves out, so a client that does
not know a setting cannot turn it off by leaving it out: the thresholds page
has no flapping-detection or alert-TTL controls, and an older browser bundle
or API script predates later fields. A sent key replaces its stored value
whole, so `"overrides": {}` removes every override rather than merging, and
the merged config then goes through the same normalization as any update (for
example, activation state is never cleared). The read, merge and apply run
under the alert manager's lock, so two concurrent partial saves cannot revert
each other in memory; the handler runs one save at a time and persists a
snapshot of the applied config, so successful saves through this endpoint
reach `alerts.json` in the order they were applied. A body that is not a JSON object, `null` included, is a 400 and changes
nothing. `TestUpdateAlertConfig_KeepsStoredValuesForUnsentKeys`
and `TestUpdateAlertConfig_RejectsNonObjectBody` in
`internal/api/alerting/alerts_test.go` prove the transport.
### Alert intent and UDP availability transport
`GET /api/alerts/intent-policies` and
@@ -3008,6 +3008,15 @@ recovery evidence. Unremembered overflow deliveries remain auditable with
`reason=capacity`; cache expiry, overflow, or restart is not a successful
storage operation or a reason to delete historical events.
### Alert settings saves keep unsent keys without a new storage path
`PUT /api/alerts/config` now applies only the top-level keys the body carries
and keeps stored values for the rest, so a thresholds save no longer turns off
alert TTL cleanup. It still writes `alerts.json` through the existing
`SaveAlertConfig` path, opens no storage or recovery route, and leaves backup
and snapshot alert settings under the same replace-what-was-sent rule as every
other key. `internal/api/alerting/alerts_test.go` pins the handler.
### Container diagnostics shares private bootstrap transport (1 October 2026)
Recovery-adjacent diagnostics now reuse the canonical complete installer
+3 -1
View File
@@ -891,7 +891,9 @@ For an existing incident whose notifications should be paused without clearing
it, use the snooze and unsnooze endpoints below.
- `GET /api/alerts/config`
- `PUT /api/alerts/config`
- `PUT /api/alerts/config` — replaces only the top-level keys the body
carries; a key left out keeps its stored value, and the merged config is
normalized as before. The body must be a JSON object.
- `GET /api/alerts/deadman/config` — returns only whether an external watchdog
is configured; `pingUrl` is `***REDACTED***` when present and never returns
the credential-bearing URL
@@ -175,6 +175,17 @@ describe('docsLinks', () => {
expect(handler).toContain('agentID != "" && len(*compatibility.ObservationLocationIDs) == 1');
});
it('documents alert configuration saves as replacing only the keys they send', () => {
const api = readFileSync(path.join(repoRoot, 'docs', 'API.md'), 'utf8');
expect(api).toContain('`PUT /api/alerts/config` — replaces only the top-level keys the body');
expect(api).toContain('a key left out keeps its stored value');
const handler = readFileSync(
path.join(repoRoot, 'internal', 'api', 'alerting', 'alerts.go'),
'utf8',
);
expect(handler).toContain('GetAlertManager().ApplyConfigUpdate(body)');
});
it('separates server removal from persistent-data erasure', () => {
const installation = readFileSync(path.join(repoRoot, 'docs', 'INSTALL.md'), 'utf8');
const removal = installation.split('## 🗑️ Uninstall')[1];
+90
View File
@@ -0,0 +1,90 @@
package config
import (
"encoding/json"
"errors"
"fmt"
"reflect"
"strings"
)
var errAlertConfigUpdateNotObject = errors.New("alert configuration update must be a JSON object")
// ApplyAlertConfigUpdate decodes a client's alert configuration update onto
// the stored configuration. A top-level key the client sent replaces that
// setting whole; a key it left out keeps the stored value.
//
// Clients only send what they know about. The settings page has no control
// for flapping detection, alert TTL cleanup or custom rules, and an older
// browser bundle or API script predates any field added since. Decoding the
// body into a zero AlertConfig used to turn every one of those settings off
// on each save.
//
// The body is decoded on its own, so the keys it carries mean exactly what
// encoding/json makes of them. Each setting it left out is copied from a JSON
// round trip of stored, so the result shares no maps or slices with it.
func ApplyAlertConfigUpdate(stored AlertConfig, update []byte) (AlertConfig, error) {
var sent map[string]json.RawMessage
if err := json.Unmarshal(update, &sent); err != nil {
return AlertConfig{}, err
}
if sent == nil {
return AlertConfig{}, errAlertConfigUpdateNotObject
}
var result AlertConfig
if err := json.Unmarshal(update, &result); err != nil {
return AlertConfig{}, err
}
kept, err := CloneAlertConfig(stored)
if err != nil {
return AlertConfig{}, err
}
resultValue := reflect.ValueOf(&result).Elem()
keptValue := reflect.ValueOf(kept)
for i := 0; i < resultValue.NumField(); i++ {
if !alertConfigKeySent(sent, alertConfigJSONKey(resultValue.Type().Field(i))) {
resultValue.Field(i).Set(keptValue.Field(i))
}
}
return result, nil
}
// CloneAlertConfig returns a copy of config that shares no maps, slices or
// pointers with it, by the same JSON round trip config takes through
// alerts.json.
func CloneAlertConfig(config AlertConfig) (AlertConfig, error) {
data, err := json.Marshal(config)
if err != nil {
return AlertConfig{}, fmt.Errorf("encode alert config: %w", err)
}
var clone AlertConfig
if err := json.Unmarshal(data, &clone); err != nil {
return AlertConfig{}, fmt.Errorf("decode alert config: %w", err)
}
return clone, nil
}
// alertConfigJSONKey returns the key encoding/json reads a field from.
func alertConfigJSONKey(field reflect.StructField) string {
name, _, _ := strings.Cut(field.Tag.Get("json"), ",")
if name == "" {
return field.Name
}
return name
}
// alertConfigKeySent matches keys the way encoding/json matches them to
// fields: exactly, or else case-insensitively.
func alertConfigKeySent(sent map[string]json.RawMessage, key string) bool {
if _, ok := sent[key]; ok {
return true
}
for sentKey := range sent {
if strings.EqualFold(sentKey, key) {
return true
}
}
return false
}
+257
View File
@@ -0,0 +1,257 @@
package config
import (
"encoding/json"
"reflect"
"strings"
"testing"
"time"
)
func storedAlertConfigForUpdate() AlertConfig {
activated := time.Date(2026, 10, 1, 9, 30, 0, 0, time.UTC)
note := "keep"
return AlertConfig{
IdentitySchemaVersion: 2,
Enabled: true,
ActivationState: ActivationActive,
ActivationTime: &activated,
ObservationWindowHours: 24,
GuestDefaults: ThresholdConfig{CPU: &HysteresisThreshold{Trigger: 80, Clear: 75}},
StorageDefault: HysteresisThreshold{Trigger: 85, Clear: 80},
DiskTempByType: map[string]HysteresisThreshold{"nvme": {Trigger: 70, Clear: 65}},
IgnoredGuestPrefixes: []string{"test-"},
Overrides: map[string]ThresholdConfig{"node:pve1": {Disabled: true, Note: &note}},
CustomRules: []CustomAlertRule{{ID: "rule-1", Name: "Busy web", Enabled: true}},
Schedule: ScheduleConfig{Cooldown: 5, MaxAlertsHour: 10, InitialNotify: "all"},
MinimumDelta: 3,
SuppressionWindow: 10,
HysteresisMargin: 4,
TimeThresholds: map[string]int{"guest": 5},
MaxAlertAgeDays: 7,
MaxAcknowledgedAgeDays: 1,
AutoAcknowledgeAfterHours: 24,
FlappingEnabled: true,
FlappingWindowSeconds: 600,
FlappingThreshold: 4,
FlappingCooldownMinutes: 20,
}
}
func TestApplyAlertConfigUpdateKeepsSettingsTheClientDidNotSend(t *testing.T) {
stored := storedAlertConfigForUpdate()
// The settings page sends thresholds and schedule but has no control for
// flapping detection, alert TTL cleanup or custom rules.
got, err := ApplyAlertConfigUpdate(stored, []byte(`{
"enabled": true,
"guestDefaults": {"cpu": {"trigger": 90, "clear": 85}},
"overrides": {},
"flapping": {"enabled": true, "threshold": 5}
}`))
if err != nil {
t.Fatalf("ApplyAlertConfigUpdate: %v", err)
}
if got.GuestDefaults.CPU == nil || got.GuestDefaults.CPU.Trigger != 90 {
t.Fatalf("guest CPU default = %+v, want the sent trigger 90", got.GuestDefaults.CPU)
}
if len(got.Overrides) != 0 {
t.Fatalf("overrides = %v, want the sent empty map to replace the stored overrides", got.Overrides)
}
if !got.FlappingEnabled || got.FlappingWindowSeconds != 600 || got.FlappingThreshold != 4 || got.FlappingCooldownMinutes != 20 {
t.Fatalf("flapping = %v/%d/%d/%d, want the stored true/600/4/20",
got.FlappingEnabled, got.FlappingWindowSeconds, got.FlappingThreshold, got.FlappingCooldownMinutes)
}
if got.MaxAlertAgeDays != 7 || got.MaxAcknowledgedAgeDays != 1 || got.AutoAcknowledgeAfterHours != 24 {
t.Fatalf("TTL cleanup = %d/%d/%d, want the stored 7/1/24",
got.MaxAlertAgeDays, got.MaxAcknowledgedAgeDays, got.AutoAcknowledgeAfterHours)
}
if !reflect.DeepEqual(got.CustomRules, stored.CustomRules) {
t.Fatalf("custom rules = %+v, want the stored rules", got.CustomRules)
}
if got.MinimumDelta != 3 || got.SuppressionWindow != 10 || got.HysteresisMargin != 4 {
t.Fatalf("general settings = %v/%d/%v, want the stored 3/10/4",
got.MinimumDelta, got.SuppressionWindow, got.HysteresisMargin)
}
}
func TestApplyAlertConfigUpdateEmptyObjectChangesNothing(t *testing.T) {
stored := storedAlertConfigForUpdate()
got, err := ApplyAlertConfigUpdate(stored, []byte(`{}`))
if err != nil {
t.Fatalf("ApplyAlertConfigUpdate({}): %v", err)
}
if !reflect.DeepEqual(got, stored) {
t.Fatalf("ApplyAlertConfigUpdate({}) changed the config:\n got %+v\nwant %+v", got, stored)
}
}
func TestApplyAlertConfigUpdateHonorsExplicitOffAndClear(t *testing.T) {
stored := storedAlertConfigForUpdate()
got, err := ApplyAlertConfigUpdate(stored, []byte(`{
"flappingEnabled": false,
"maxAlertAgeDays": 0,
"maxAcknowledgedAgeDays": 0,
"autoAcknowledgeAfterHours": 0,
"customRules": null,
"ignoredGuestPrefixes": []
}`))
if err != nil {
t.Fatalf("ApplyAlertConfigUpdate: %v", err)
}
if got.FlappingEnabled || got.MaxAlertAgeDays != 0 || got.MaxAcknowledgedAgeDays != 0 || got.AutoAcknowledgeAfterHours != 0 {
t.Fatalf("explicit off was not applied: flapping=%v ttl=%d/%d/%d",
got.FlappingEnabled, got.MaxAlertAgeDays, got.MaxAcknowledgedAgeDays, got.AutoAcknowledgeAfterHours)
}
if got.CustomRules != nil {
t.Fatalf("custom rules = %+v, want null to clear them", got.CustomRules)
}
if len(got.IgnoredGuestPrefixes) != 0 {
t.Fatalf("ignored guest prefixes = %v, want the sent empty list", got.IgnoredGuestPrefixes)
}
if !got.Enabled || got.ActivationState != ActivationActive {
t.Fatalf("unsent enabled/activation = %v/%q, want the stored true/active", got.Enabled, got.ActivationState)
}
}
// The merge is top-level only: a sent object replaces the stored one whole,
// so a sent truenasDiskDefaults without a temperature must not inherit the
// stored temperature.
func TestApplyAlertConfigUpdateReplacesSentObjectsWhole(t *testing.T) {
stored := storedAlertConfigForUpdate()
stored.TrueNASDiskDefaults = ThresholdConfig{Temperature: &HysteresisThreshold{Trigger: 62, Clear: 57}}
got, err := ApplyAlertConfigUpdate(stored, []byte(`{"truenasDiskDefaults": {}}`))
if err != nil {
t.Fatalf("ApplyAlertConfigUpdate: %v", err)
}
if got.TrueNASDiskDefaults.Temperature != nil {
t.Fatalf("TrueNAS disk temperature = %+v, want the sent empty object to clear it", got.TrueNASDiskDefaults.Temperature)
}
}
// encoding/json matches keys to fields case-insensitively, so a key in another
// casing still names the field and must not leave the stored value in place.
func TestApplyAlertConfigUpdateMatchesKeysLikeTheDecoder(t *testing.T) {
stored := storedAlertConfigForUpdate()
got, err := ApplyAlertConfigUpdate(stored, []byte(`{"FlappingEnabled": false}`))
if err != nil {
t.Fatalf("ApplyAlertConfigUpdate: %v", err)
}
if got.FlappingEnabled {
t.Fatal("FlappingEnabled sent as false stayed true")
}
}
// Sent keys mean what decoding the body alone makes of them, duplicates and
// all.
func TestApplyAlertConfigUpdateDecodesSentKeysLikeTheBodyAlone(t *testing.T) {
stored := storedAlertConfigForUpdate()
for _, test := range []struct {
body string
field func(AlertConfig) any
}{
{`{"enabled": false, "enabled": null}`, func(c AlertConfig) any { return c.Enabled }},
{`{"flappingEnabled": false, "FlappingEnabled": true}`, func(c AlertConfig) any { return c.FlappingEnabled }},
{
`{"guestDefaults": {"cpu": {"trigger": 90, "clear": 85}}, "guestDefaults": {"memory": {"trigger": 70, "clear": 65}}}`,
func(c AlertConfig) any { return c.GuestDefaults },
},
} {
var alone AlertConfig
if err := json.Unmarshal([]byte(test.body), &alone); err != nil {
t.Fatalf("decode %s alone: %v", test.body, err)
}
got, err := ApplyAlertConfigUpdate(stored, []byte(test.body))
if err != nil {
t.Fatalf("ApplyAlertConfigUpdate(%s): %v", test.body, err)
}
if !reflect.DeepEqual(test.field(got), test.field(alone)) {
t.Fatalf("ApplyAlertConfigUpdate(%s) = %+v, decoding the body alone gives %+v",
test.body, test.field(got), test.field(alone))
}
}
if _, err := ApplyAlertConfigUpdate(stored, []byte(`{"enabled": "yes", "enabled": true}`)); err == nil {
t.Fatal("a type error in a duplicated key succeeded, want the decoder's error")
}
}
// The merge copies unsent settings field by field under their JSON keys, so
// an embedded, unexported or untagged field would be skipped or matched
// under the wrong key.
func TestAlertConfigFieldsHaveExplicitJSONKeys(t *testing.T) {
fields := reflect.TypeOf(AlertConfig{})
keys := make([]string, fields.NumField())
for i := 0; i < fields.NumField(); i++ {
field := fields.Field(i)
keys[i], _, _ = strings.Cut(field.Tag.Get("json"), ",")
if field.Anonymous || !field.IsExported() || keys[i] == "" || keys[i] == "-" {
t.Errorf("AlertConfig.%s needs to be an exported, named field with an explicit json key", field.Name)
}
// Keys that fold equal would both count as sent for either key.
for j := 0; j < i; j++ {
if strings.EqualFold(keys[i], keys[j]) {
t.Errorf("AlertConfig.%s and AlertConfig.%s have json keys that fold equal",
fields.Field(j).Name, field.Name)
}
}
}
}
func TestApplyAlertConfigUpdateSharesNoStateWithStored(t *testing.T) {
stored := storedAlertConfigForUpdate()
got, err := ApplyAlertConfigUpdate(stored, []byte(`{"enabled": true}`))
if err != nil {
t.Fatalf("ApplyAlertConfigUpdate: %v", err)
}
got.TimeThresholds["guest"] = 99
got.DiskTempByType["nvme"] = HysteresisThreshold{Trigger: 1, Clear: 0}
got.IgnoredGuestPrefixes[0] = "changed-"
*got.ActivationTime = time.Time{}
if stored.TimeThresholds["guest"] != 5 || stored.DiskTempByType["nvme"].Trigger != 70 ||
stored.IgnoredGuestPrefixes[0] != "test-" || stored.ActivationTime.IsZero() {
t.Fatal("editing the merged config changed the stored config it was merged from")
}
}
func TestApplyAlertConfigUpdateRejectsMalformedBodies(t *testing.T) {
stored := storedAlertConfigForUpdate()
for _, body := range []string{`null`, `[]`, `"config"`, `{"flappingEnabled": "yes"}`, `{`} {
if _, err := ApplyAlertConfigUpdate(stored, []byte(body)); err == nil {
t.Fatalf("ApplyAlertConfigUpdate(%s) succeeded, want a decode error", body)
}
}
}
// A config marshaled by a Go client still replaces every field it writes.
// Empty omitempty fields are left out of that body, so they keep their
// stored values like any other unsent key.
func TestApplyAlertConfigUpdateFullBodyReplacesWrittenFields(t *testing.T) {
stored := storedAlertConfigForUpdate()
replacement := AlertConfig{Enabled: true, Overrides: map[string]ThresholdConfig{}, TimeThresholds: map[string]int{}}
body, err := json.Marshal(replacement)
if err != nil {
t.Fatalf("marshal replacement: %v", err)
}
got, err := ApplyAlertConfigUpdate(stored, body)
if err != nil {
t.Fatalf("ApplyAlertConfigUpdate: %v", err)
}
if got.FlappingEnabled || got.MaxAlertAgeDays != 0 || got.MinimumDelta != 0 || len(got.Overrides) != 0 {
t.Fatalf("full body left written fields at their stored values: %+v", got)
}
if !reflect.DeepEqual(got.CustomRules, stored.CustomRules) {
t.Fatalf("custom rules = %+v, want the stored rules for the omitted key", got.CustomRules)
}
}
+31
View File
@@ -1,6 +1,8 @@
package alerts
import (
"errors"
"fmt"
"strings"
"time"
@@ -13,7 +15,36 @@ import (
func (m *Manager) UpdateConfig(config AlertConfig) {
m.mu.Lock()
defer m.mu.Unlock()
m.updateConfigLocked(config)
}
// ErrConfigSnapshot reports an update that was applied but could not be copied
// for the caller.
var ErrConfigSnapshot = errors.New("snapshot applied alert configuration")
// ApplyConfigUpdate applies a client's JSON update to the stored
// configuration: keys the update carries replace their settings and the rest
// keep their stored values (see alertconfig.ApplyAlertConfigUpdate). Reading,
// merging and applying under one lock keeps two partial updates from
// reverting each other's settings. It returns a snapshot of the applied
// config that the caller owns, so persisting it cannot write into the live
// config's maps.
func (m *Manager) ApplyConfigUpdate(update []byte) (AlertConfig, error) {
m.mu.Lock()
defer m.mu.Unlock()
config, err := alertconfig.ApplyAlertConfigUpdate(m.config, update)
if err != nil {
return AlertConfig{}, err
}
m.updateConfigLocked(config)
snapshot, err := alertconfig.CloneAlertConfig(m.config)
if err != nil {
return AlertConfig{}, fmt.Errorf("%w: %w", ErrConfigSnapshot, err)
}
return snapshot, nil
}
func (m *Manager) updateConfigLocked(config AlertConfig) {
// Clients and rollback-era config writers may not know about the additive
// identity schema marker. Never lower a version already held by this
// process; an older binary can still omit it on disk and the migration will
+76
View File
@@ -205,3 +205,79 @@ func TestUpdateConfigNormalizesExactEscalationRoutingAndRepeatPolicy(t *testing.
}
}
}
// The thresholds page sends no flapping, alert TTL or custom-rule keys. A save
// through ApplyConfigUpdate must leave those settings as stored, also after
// the merged config is normalized in place.
func TestUpdateConfigKeepsSettingsAThresholdsSaveDidNotSend(t *testing.T) {
m := newTestManager(t)
stored := m.GetConfig()
stored.FlappingEnabled = true
stored.FlappingWindowSeconds = 600
stored.MaxAlertAgeDays = 7
stored.MaxAcknowledgedAgeDays = 1
stored.AutoAcknowledgeAfterHours = 24
stored.CustomRules = []CustomAlertRule{{ID: "rule-1", Name: "Busy web", Enabled: true}}
m.UpdateConfig(stored)
snapshot, err := m.ApplyConfigUpdate([]byte(`{
"enabled": true,
"nodeDefaults": {"cpu": {"trigger": 82, "clear": 77}},
"overrides": {}
}`))
if err != nil {
t.Fatalf("ApplyConfigUpdate: %v", err)
}
snapshot.TimeThresholds["guest"] = 999
if m.GetConfig().TimeThresholds["guest"] == 999 {
t.Fatal("the returned snapshot shares its maps with the live config")
}
got := m.GetConfig()
if got.NodeDefaults.CPU == nil || got.NodeDefaults.CPU.Trigger != 82 {
t.Fatalf("node CPU default = %+v, want the saved trigger 82", got.NodeDefaults.CPU)
}
if !got.FlappingEnabled || got.FlappingWindowSeconds != 600 {
t.Fatalf("flapping = %v/%d after a thresholds save, want true/600", got.FlappingEnabled, got.FlappingWindowSeconds)
}
if got.MaxAlertAgeDays != 7 || got.MaxAcknowledgedAgeDays != 1 || got.AutoAcknowledgeAfterHours != 24 {
t.Fatalf("TTL cleanup = %d/%d/%d after a thresholds save, want 7/1/24",
got.MaxAlertAgeDays, got.MaxAcknowledgedAgeDays, got.AutoAcknowledgeAfterHours)
}
if len(got.CustomRules) != 1 || got.CustomRules[0].ID != "rule-1" {
t.Fatalf("custom rules = %+v after a thresholds save, want the stored rule", got.CustomRules)
}
}
// Each partial update reads, merges and applies under the manager lock, so
// two saves that touch different settings cannot revert each other.
func TestApplyConfigUpdateKeepsConcurrentPartialUpdates(t *testing.T) {
m := newTestManager(t)
for round := 0; round < 50; round++ {
base := m.GetConfig()
base.FlappingEnabled = false
base.MaxAlertAgeDays = 0
m.UpdateConfig(base)
bodies := []string{`{"flappingEnabled": true}`, `{"maxAlertAgeDays": 9}`}
done := make(chan struct{}, len(bodies))
for _, body := range bodies {
go func(body string) {
defer func() { done <- struct{}{} }()
if _, err := m.ApplyConfigUpdate([]byte(body)); err != nil {
t.Errorf("ApplyConfigUpdate(%s): %v", body, err)
}
}(body)
}
for range bodies {
<-done
}
got := m.GetConfig()
if !got.FlappingEnabled || got.MaxAlertAgeDays != 9 {
t.Fatalf("round %d: flapping=%v maxAlertAgeDays=%d, want both concurrent updates kept",
round, got.FlappingEnabled, got.MaxAlertAgeDays)
}
}
}
+21 -4
View File
@@ -32,6 +32,7 @@ import (
type AlertManager interface {
GetConfig() alerts.AlertConfig
UpdateConfig(alerts.AlertConfig)
ApplyConfigUpdate(update []byte) (alerts.AlertConfig, error)
GetActiveAlerts() []alerts.Alert
DiagnoseAlertDelivery(alertIdentifier string) (alerts.AlertDeliveryDiagnosis, bool)
DiagnoseActiveAlertDeliveries() []alerts.AlertDeliveryDiagnosis
@@ -80,6 +81,7 @@ type AlertMonitor interface {
type AlertHandlers struct {
stateMu sync.RWMutex
intentPolicyMu sync.Mutex
configSaveMu sync.Mutex
mtMonitor *monitoring.MultiTenantMonitor
defaultMonitor AlertMonitor
wsHub *websocket.Hub
@@ -292,13 +294,28 @@ func (h *AlertHandlers) UpdateAlertConfig(w http.ResponseWriter, r *http.Request
// from instances with a few hundred disabled containers (#1601).
r.Body = http.MaxBytesReader(w, r.Body, 1<<20)
var config alerts.AlertConfig
if err := json.NewDecoder(r.Body).Decode(&config); err != nil {
body, err := io.ReadAll(r.Body)
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
// One save at a time, so a slower save cannot write an older config to
// disk after a newer one was applied.
h.configSaveMu.Lock()
defer h.configSaveMu.Unlock()
// Keys the client left out keep their stored values, so a thresholds
// save no longer turns off flapping detection or alert TTL cleanup.
updatedConfig, err := h.getMonitor(r.Context()).GetAlertManager().ApplyConfigUpdate(body)
if errors.Is(err, alerts.ErrConfigSnapshot) {
log.Error().Err(err).Msg("Failed to snapshot applied alert configuration")
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
h.getMonitor(r.Context()).GetAlertManager().UpdateConfig(config)
updatedConfig := h.getMonitor(r.Context()).GetAlertManager().GetConfig()
// Update notification manager with schedule settings
notificationMgr := h.getMonitor(r.Context()).GetNotificationManager()
+89 -2
View File
@@ -74,6 +74,11 @@ func (m *MockAlertManager) UpdateConfig(cfg alerts.AlertConfig) {
m.Called(cfg)
}
func (m *MockAlertManager) ApplyConfigUpdate(update []byte) (alerts.AlertConfig, error) {
args := m.Called(update)
return args.Get(0).(alerts.AlertConfig), args.Error(1)
}
func (m *MockAlertManager) GetActiveAlerts() []alerts.Alert {
args := m.Called()
return args.Get(0).([]alerts.Alert)
@@ -238,7 +243,7 @@ func TestUpdateAlertConfig(t *testing.T) {
h := NewAlertHandlers(nil, mockMonitor, nil)
cfg := alerts.AlertConfig{Enabled: true, ActivationState: alerts.ActivationPending}
mockManager.On("UpdateConfig", testifymock.Anything).Return()
mockManager.On("ApplyConfigUpdate", testifymock.Anything).Return(cfg, nil)
mockManager.On("GetConfig").Return(cfg)
mockPersist.On("SaveAlertConfig", testifymock.Anything).Return(nil)
@@ -252,6 +257,88 @@ func TestUpdateAlertConfig(t *testing.T) {
assert.False(t, notificationMgr.IsEnabled())
}
// The Thresholds page has no control for flapping detection or alert TTL
// cleanup and never sends those keys; decoding its body into a zero config
// turned both off on every save.
func TestUpdateAlertConfig_KeepsStoredValuesForUnsentKeys(t *testing.T) {
manager := alerts.NewManagerWithDataDir(t.TempDir())
defer manager.Stop()
stored := manager.GetConfig()
stored.MaxAlertAgeDays = 7
stored.MaxAcknowledgedAgeDays = 1
stored.AutoAcknowledgeAfterHours = 24
stored.FlappingEnabled = true
manager.UpdateConfig(stored)
mockMonitor := new(MockAlertMonitor)
mockPersist := new(MockConfigPersistence)
notificationMgr := notifications.NewNotificationManagerWithDataDir("", t.TempDir())
defer notificationMgr.Stop()
mockMonitor.On("GetAlertManager").Return(manager)
mockMonitor.On("GetConfigPersistence").Return(mockPersist)
mockMonitor.On("GetNotificationManager").Return(notificationMgr)
var persisted alerts.AlertConfig
mockPersist.On("SaveAlertConfig", testifymock.Anything).Run(func(args testifymock.Arguments) {
persisted = args.Get(0).(alerts.AlertConfig)
}).Return(nil)
h := NewAlertHandlers(nil, mockMonitor, nil)
body := `{"enabled":true,"guestDefaults":{"cpu":{"trigger":90,"clear":85}},"overrides":{},` +
`"flapping":{"enabled":true,"threshold":5,"window":10,"suppressionTime":30,"minStability":0.8}}`
req := httptest.NewRequest(http.MethodPut, "/api/alerts/config", strings.NewReader(body))
w := httptest.NewRecorder()
h.UpdateAlertConfig(w, req)
if w.Code != http.StatusOK {
t.Fatalf("status = %d, want 200: %s", w.Code, w.Body.String())
}
// Persistence normalizes the config it is handed in place, so it must get
// a snapshot rather than the live config's maps.
persisted.TimeThresholds["guest"] = 999
if manager.GetConfig().TimeThresholds["guest"] == 999 {
t.Fatal("the persisted config shares its maps with the live alert config")
}
persisted.TimeThresholds["guest"] = manager.GetConfig().TimeThresholds["guest"]
for name, cfg := range map[string]alerts.AlertConfig{"live": manager.GetConfig(), "persisted": persisted} {
if cfg.GuestDefaults.CPU == nil || cfg.GuestDefaults.CPU.Trigger != 90 {
t.Fatalf("%s guest CPU default = %+v, want the sent trigger 90", name, cfg.GuestDefaults.CPU)
}
if !cfg.FlappingEnabled {
t.Fatalf("thresholds save turned %s flapping detection off", name)
}
if cfg.MaxAlertAgeDays != 7 || cfg.MaxAcknowledgedAgeDays != 1 || cfg.AutoAcknowledgeAfterHours != 24 {
t.Fatalf("%s TTL cleanup = %d/%d/%d after a thresholds save, want 7/1/24",
name, cfg.MaxAlertAgeDays, cfg.MaxAcknowledgedAgeDays, cfg.AutoAcknowledgeAfterHours)
}
}
}
func TestUpdateAlertConfig_RejectsNonObjectBody(t *testing.T) {
manager := alerts.NewManagerWithDataDir(t.TempDir())
defer manager.Stop()
before := manager.GetConfig()
mockMonitor := new(MockAlertMonitor)
mockMonitor.On("GetAlertManager").Return(manager)
h := NewAlertHandlers(nil, mockMonitor, nil)
for _, body := range []string{`null`, `[]`, `"config"`} {
req := httptest.NewRequest(http.MethodPut, "/api/alerts/config", strings.NewReader(body))
w := httptest.NewRecorder()
h.UpdateAlertConfig(w, req)
if w.Code != http.StatusBadRequest {
t.Fatalf("status = %d for body %s, want 400", w.Code, body)
}
}
if after := manager.GetConfig(); after.Enabled != before.Enabled || after.ActivationState != before.ActivationState {
t.Fatalf("a rejected body changed the config: %+v", after)
}
}
// Regression for #1341: persistence failures used to be silently swallowed
// (logged but reported as "saved successfully"), so a user setting a custom
// override would see the value stick in memory and revert on reload with
@@ -271,7 +358,7 @@ func TestUpdateAlertConfig_PersistenceFailureSurfacesAsError(t *testing.T) {
h := NewAlertHandlers(nil, mockMonitor, nil)
cfg := alerts.AlertConfig{Enabled: true, ActivationState: alerts.ActivationPending}
mockManager.On("UpdateConfig", testifymock.Anything).Return()
mockManager.On("ApplyConfigUpdate", testifymock.Anything).Return(cfg, nil)
mockManager.On("GetConfig").Return(cfg)
mockPersist.On("SaveAlertConfig", testifymock.Anything).Return(errors.New("permission denied"))