[Security Solution] Remove rule changes history feature flags - #278197
[Security Solution] Remove rule changes history feature flags#278197maximpn wants to merge 8 commits into
Conversation
maximpn
added
technical debt
5419716 to
a282a96
Compare
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the feature-flag cleanup. The change is coherent, but the removal of the xpack.alerting.ruleChangeTracking.enabled config key needs a config deprecation to avoid a fatal startup error for deployments that set it explicitly — see the inline comment. The rest of the diff (always-construct ChangeTrackingService, UI setting always registered, route always mounted, test config enabled→scope rename) looks consistent and the always-present service is still guarded in get_rule_history.ts.
Generated by Claude Reviewer for #278197 · 117.6 AIC · ⌖ 10.8 AIC · ⊞ 4.3K
| ruleChangeTracking: schema.object({ | ||
| enabled: schema.boolean({ defaultValue: true }), | ||
| scope: schema.arrayOf(ruleChangeTrackingSolutions, { defaultValue: ['security'] }), | ||
| }), |
There was a problem hiding this comment.
Removing the enabled key from the ruleChangeTracking schema without a config deprecation is an upgrade-safety regression. schema.object rejects unknown keys by default, and there's no matching entry in config_deprecations.ts, so any deployment that still has xpack.alerting.ruleChangeTracking.enabled: false (the opt-out this PR is removing) in kibana.yml will hit a fatal config validation error on startup after upgrade — including on the 9.5/9.6 backport branches.
The PR description already identifies this risk. The standard fix is to register an unused() deprecation so the stale key is dropped with a warning instead of crashing:
// config_deprecations.ts
unused('ruleChangeTracking.enabled', {
level: 'warning',
message: 'The setting "xpack.alerting.ruleChangeTracking.enabled" is deprecated and no longer used. Please remove it.',
}),There was a problem hiding this comment.
Good general rule, but I don't think it applies here. ruleChangeTracking.enabled was only ever introduced in #261981 (2026-04-28), after the 9.5 branch was cut (2026-04-10) — it's not present on the 9.4 branch at all, and 9.5.0 hasn't GA'd yet (no v9.5.0 tag; current release train is 9.4, up to v9.4.3).
So there's no released Kibana version where a real deployment could have set xpack.alerting.ruleChangeTracking.enabled: false in their kibana.yml — the key only ever existed on unreleased 9.5/main dev builds. Since this PR removes it before 9.5.0 ships, no customer upgrade path is affected, so I don't think we need an unused() deprecation for it.
7d66396 to
5276363
Compare
There was a problem hiding this comment.
One inline finding: the experimental_features.ts change appears to swap the removed ruleChangesHistoryEnabled flag for an unrelated new flag rather than just deleting it. The pre-existing config-deprecation concern already noted on alerting/server/config.ts still stands and is not repeated here.
Generated by Claude Reviewer for #278197 · 109.3 AIC · ⌖ 7.65 AIC · ⊞ 4.3K
bd9f48f to
fb40615
Compare
There was a problem hiding this comment.
Reviewed the feature-flag removal. One upgrade-safety concern on the removed alerting config key (inline). The rest — UI checks now relying solely on the advanced setting, always-constructing ChangeTrackingService, the serverless project-settings push, and the test config scope renames — looks consistent, and I found no leftover references to the removed ruleChangesHistoryEnabled / ruleChangeTracking.enabled flags.
Generated by Claude Reviewer for #278197 · 230.8 AIC · ⌖ 12.8 AIC · ⊞ 4.3K
pmuellr
left a comment
pmuellr
left a comment
There was a problem hiding this comment.
ResponseOps changes LGTM
549ad76 to
04e85ed
Compare
There was a problem hiding this comment.
Reviewed the feature-flag cleanup. One backward-compatibility concern flagged inline regarding removal of the xpack.alerting.ruleChangeTracking.enabled config key without a deprecation. The rest of the flag removals and test updates look consistent.
Generated by Claude Reviewer for #278197 · 127.5 AIC · ⌖ 11.7 AIC · ⊞ 4.6K
ec5debd to
5b2533e
Compare
There was a problem hiding this comment.
One upgrade-safety concern: the removed ruleChangeTracking.enabled config key needs an unused() deprecation so deployments that set it don't hit a fatal config-validation error on boot (see inline comment). The rest of the flag removal is mechanically consistent, and the test updates track the change appropriately.
Generated by Claude Reviewer for #278197 · claude · opus · 127.9 AIC · ⌖ 22.4 AIC · ⊞ 5.5K
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Page load bundle
Unknown metric groupsshared chunks total size
total optimizer output size
History
cc @maximpn |
5 participants
Related to: https://github.com/elastic/security-team/issues/12367
Summary
Removes the feature flags gating the Detection Rule Changes History feature, now that it's shipping unconditionally. Both flags defaulted to
true, so there's no behavior change for the majority of deployments — this only removes the opt-out.Details
Alerting plugin
ruleChangeTracking.enabledconfig schema key fromconfig.ts(keptruleChangeTracking.scope)plugin.tsalways constructsChangeTrackingService, no longer gated by the removed config flagRuleChangeTrackingDisabledErrordoc comment inget_rule_history.tsto reflect the service can no longer be disabled via configcommon/config.ts,group6/enabled.ts,group6/config_with_change_tracking_enabled.ts) and unit tests/test utils to drop the removed config keySecurity Solution
ruleChangesHistoryEnabledexperimental feature flag fromexperimental_features.tsui_settings.tsalways registersENABLE_RULE_CHANGES_HISTORY_SETTING, no longer conditional on the removed flagroutes.tsx,rule_details/index.tsx,rule_actions_overflow/index.tsx, andregister_routes.tsdrop theuseIsExperimentalFeatureEnabled('ruleChangesHistoryEnabled')check, relying only on the advanced settinglog_rule_changes.tsupdated for the alerting config changeSecurity Solution Serverless
plugin.tsalways pushessecuritySolution:enableRuleChangesHistoryto project settings, no longer gated by the removed flagconfig/ess/config.base.tsandconfig/serverless/config.base.tsTests
trial_license_complete_tier/change_tracking.tsandsecurity_solution/server/ui_settings.test.tsfor the flag removalHow to test
Covered by existing alerting integration tests (
alerting_api_integration/common/config.ts,group6/change_tracking) andsecurity_solution/server/ui_settings.test.ts. No new manual testing needed — behavior is unchanged since both flags defaulted to enabled.Release note
skip
Checklist
Identify risks
Removes the
xpack.alerting.ruleChangeTracking.enabledconfig key entirely. Any deployment with this key explicitly set tofalseinkibana.ymlwill hit a config validation error on upgrade, and any deployment relying on it to opt out will lose that opt-out on the backported branches (9.5, 9.6).