diff --git a/docs/API.md b/docs/API.md index 296d4cc116..f2e3d6b6f5 100644 --- a/docs/API.md +++ b/docs/API.md @@ -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 diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 07295760aa..629fa45621 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -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 diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 40604c2962..e1086c8b3f 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -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 diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 64978f4e99..0816bad6ae 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -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 diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 27c4f95c7b..e6a166e7b4 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -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 diff --git a/frontend-modern/public/docs/API.md b/frontend-modern/public/docs/API.md index 296d4cc116..f2e3d6b6f5 100644 --- a/frontend-modern/public/docs/API.md +++ b/frontend-modern/public/docs/API.md @@ -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 diff --git a/frontend-modern/src/utils/__tests__/docsLinks.test.ts b/frontend-modern/src/utils/__tests__/docsLinks.test.ts index 0ccae13f89..69ea204791 100644 --- a/frontend-modern/src/utils/__tests__/docsLinks.test.ts +++ b/frontend-modern/src/utils/__tests__/docsLinks.test.ts @@ -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]; diff --git a/internal/alerts/config/update.go b/internal/alerts/config/update.go new file mode 100644 index 0000000000..0cc9fd334b --- /dev/null +++ b/internal/alerts/config/update.go @@ -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 +} diff --git a/internal/alerts/config/update_test.go b/internal/alerts/config/update_test.go new file mode 100644 index 0000000000..9d1347f870 --- /dev/null +++ b/internal/alerts/config/update_test.go @@ -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: ¬e}}, + 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) + } +} diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index d609874c77..722515faee 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -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 diff --git a/internal/alerts/config_validation_test.go b/internal/alerts/config_validation_test.go index 82c579abe3..3b4f7e30b3 100644 --- a/internal/alerts/config_validation_test.go +++ b/internal/alerts/config_validation_test.go @@ -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) + } + } +} diff --git a/internal/api/alerting/alerts.go b/internal/api/alerting/alerts.go index 2c19c30f20..8cc4165417 100644 --- a/internal/api/alerting/alerts.go +++ b/internal/api/alerting/alerts.go @@ -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() diff --git a/internal/api/alerting/alerts_test.go b/internal/api/alerting/alerts_test.go index 1c3df1f2f9..ff40a931ee 100644 --- a/internal/api/alerting/alerts_test.go +++ b/internal/api/alerting/alerts_test.go @@ -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"))