Skip to content

Commit

Permalink
[Security Solution] Extend the /upgrade/_review endpoint contract a…
Browse files Browse the repository at this point in the history
…nd functionality (#187770)

**Resolves: #180153
**Resolves: #188277

## Summary

- Extend the POST /upgrade/_review API endpoint's contract and
functionality
- Changes `has_conflict` property within each rule field's
`ThreeWayDiff` from `boolean` to `enum` with possible values:
    - `NONE`: no conflicts in three way diff
- `SOLVABLE`: conflict detected but was successfully resolved by our
algorithms
- `NON_SOLVABLE`: conflict detected and could not be resolved by our
algorithms.

- Adds `has_base_version` boolean field within each field diff
calculation. Has values:
- true: the base version of the field was found and is either defined or
undefined
    - false: the base version of the field was not found

- The possible values for `has_conflict` for each concrete diff
algorithm are:
    - **single line strings**: `NO`, `NON_SOLVABLE` 
    - **multi line strings**: `NO`, `SOLVABLE`, `NON_SOLVABLE` 
    - **numbers**: `NO`, `NON_SOLVABLE` 
    - **array of scalar values**: `NO`, `SOLVABLE`

- [ ] Adds new logic to handle
#186435 (comment)


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
jpdjere committed Jul 25, 2024
1 parent 054ae81 commit fb82b0e
Show file tree
Hide file tree
Showing 24 changed files with 837 additions and 632 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ export * from './model/diff/rule_diff/fields_diff';
export * from './model/diff/rule_diff/rule_diff';
export * from './model/diff/three_way_diff/three_way_diff_outcome';
export * from './model/diff/three_way_diff/three_way_diff';
export * from './model/diff/three_way_diff/three_way_diff_conflict';
export * from './model/diff/three_way_diff/three_way_merge_outcome';
Original file line number Diff line number Diff line change
Expand Up @@ -54,24 +54,27 @@ export type RuleFieldsDiff = CommonFieldsDiff &
| NewTermsFieldsDiff
);

interface BaseRuleDiff {
num_fields_with_updates: number;
num_fields_with_conflicts: number;
num_fields_with_non_solvable_conflicts: number;
}
/**
* Full rule diff contains diffs for all the top-level rule fields.
* Even if there's no change at all to a given field, its diff will be included in this object.
* This diff can be useful for internal server-side calculations or debugging.
* Note that this is a pretty large object so returning it from the API might be undesirable.
*/
export interface FullRuleDiff {
export interface FullRuleDiff extends BaseRuleDiff {
fields: RuleFieldsDiff;
has_conflict: boolean;
}

/**
* Partial rule diff contains diffs only for those rule fields that have some changes to them.
* This diff can be useful for returning info from REST API endpoints because its size is tolerable.
*/
export interface PartialRuleDiff {
export interface PartialRuleDiff extends BaseRuleDiff {
fields: Partial<RuleFieldsDiff>;
has_conflict: boolean;
}

export type RuleFieldsDiffWithDataSource =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import type { ThreeWayDiffOutcome } from './three_way_diff_outcome';
import type { ThreeWayDiffConflict } from './three_way_diff_conflict';
import type { ThreeWayMergeOutcome } from './three_way_merge_outcome';

/**
Expand Down Expand Up @@ -66,7 +67,27 @@ export interface ThreeVersionsOf<TValue> {
* 6. base=A, current=B, target=C => merged=C, conflict=true
* Customized rule, the value has changed, conflict between B and C couldn't be resolved automatically.
*/
export interface ThreeWayDiff<TValue> extends ThreeVersionsOf<TValue> {
export interface ThreeWayDiff<TValue> {
/**
* Corresponds to the stock version of the currently installed prebuilt rule.
* This field is optional because the base version is not always available in the package.
* This type is part of the API response, so the type of this field is replaced from possibly
* a MissingVersion (a JS Symbol), which can't be serialized in JSON, to possibly `undefined`.
*/
base_version: TValue | undefined;

/**
* Corresponds exactly to the currently installed prebuilt rule:
* - to the customized version (if it's customized)
* - to the stock version (if it's not customized)
*/
current_version: TValue;

/**
* Corresponds to the "new" stock version that the user is trying to upgrade to.
*/
target_version: TValue;

/**
* The result of an automatic three-way merge of three values:
* - base version
Expand All @@ -92,12 +113,19 @@ export interface ThreeWayDiff<TValue> extends ThreeVersionsOf<TValue> {

/**
* The type of result of an automatic three-way merge of three values:
* - base version
* - current version
* - target version
* - merged version
*/
merge_outcome: ThreeWayMergeOutcome;

/**
* Boolean which determines if a base version was found and returned for the three-way-diff of the field
* - true: the base version of the field was found and is either defined or undefined
* - false: the base version of the field was not found
*/
has_base_version: boolean;

/**
* Tells if the value has changed in the target version and the current version could be updated.
* True if:
Expand All @@ -107,15 +135,9 @@ export interface ThreeWayDiff<TValue> extends ThreeVersionsOf<TValue> {
has_update: boolean;

/**
* True if:
* - current != target and we couldn't automatically resolve the conflict between them
*
* False if:
* - current == target (value won't change)
* - current != target && current == base (stock rule will get a new value)
* - current != target and we automatically resolved the conflict between them
* Enum of possible conflict outcomes of a three-way diff.
*/
has_conflict: boolean;
conflict: ThreeWayDiffConflict;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

/**
* Enum of possible conflict outcomes of a three-way diff:
* - NON_SOLVABLE_CONFLICT: current != target and we couldn't automatically resolve the conflict between them
* - SOLVABLE_CONFLICT: current != target and we automatically resolved the conflict between them
* - NO_CONFLICT:
* - current == target (value won't change)
* - current != target && current == base (stock rule will get a new value)
* See RFC: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/docs/rfcs/detection_response/prebuilt_rules_customization.md#concrete-field-diff-algorithms-by-type
*/
export enum ThreeWayDiffConflict {
NONE = 'NONE',
SOLVABLE = 'SOLVABLE',
NON_SOLVABLE = 'NON_SOLVABLE',
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ export enum ThreeWayDiffOutcome {

/** Customized rule, the value has changed in the target version and is not equal to the current version. */
CustomizedValueCanUpdate = 'BASE=A, CURRENT=B, TARGET=C',

/** Missing base, the value hasn't changed in the target version. */
MissingBaseNoUpdate = 'BASE=-, CURRENT=A, TARGET=A',

/** Missing base, the value changed in the target version. */
MissingBaseCanUpdate = 'BASE=-, CURRENT=A, TARGET=B',
}

export const determineDiffOutcome = <TValue>(
Expand Down Expand Up @@ -85,12 +91,12 @@ const getThreeWayDiffOutcome = ({
if (!hasBaseVersion) {
/**
* We couldn't find the base version of the rule in the package so further
* version comparison is not possible. We assume that the rule is not
* version comparison is not possible. We assume that the rule is
* customized and the value can be updated if there's an update.
*/
return currentEqlTarget
? ThreeWayDiffOutcome.StockValueNoUpdate
: ThreeWayDiffOutcome.StockValueCanUpdate;
? ThreeWayDiffOutcome.MissingBaseNoUpdate
: ThreeWayDiffOutcome.MissingBaseCanUpdate;
}

if (baseEqlCurrent) {
Expand All @@ -111,6 +117,7 @@ const getThreeWayDiffOutcome = ({
export const determineIfValueCanUpdate = (diffCase: ThreeWayDiffOutcome): boolean => {
return (
diffCase === ThreeWayDiffOutcome.StockValueCanUpdate ||
diffCase === ThreeWayDiffOutcome.CustomizedValueCanUpdate
diffCase === ThreeWayDiffOutcome.CustomizedValueCanUpdate ||
diffCase === ThreeWayDiffOutcome.MissingBaseCanUpdate
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
*/

/**
* Type of result of an automatic three-way merge of three values:
* - base version
* The type of result of an automatic three-way merge of three values:
* - current version
* - target version
* - merged version
*/
export enum ThreeWayMergeOutcome {
/** Took current version and returned as the merged one. */
Expand All @@ -20,7 +20,4 @@ export enum ThreeWayMergeOutcome {

/** Merged three versions successfully into a new one. */
Merged = 'MERGED',

/** Couldn't merge three versions because of a conflict. */
Conflict = 'CONFLICT',
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ export * from './diff/rule_diff/fields_diff';
export * from './diff/rule_diff/rule_diff';
export * from './diff/three_way_diff/three_way_diff_outcome';
export * from './diff/three_way_diff/three_way_diff';
export * from './diff/three_way_diff/three_way_merge_outcome';
export * from './diff/three_way_diff/three_way_diff_conflict';
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ export interface RuleUpgradeStatsForReview {
/** Number of installed prebuilt rules available for upgrade (stock + customized) */
num_rules_to_upgrade_total: number;

/** Number of installed prebuilt rules with upgrade conflicts (SOLVABLE or NON_SOLVALBE) */
num_rules_with_conflicts: number;

/** Number of installed prebuilt rules with NON_SOLVABLE upgrade conflicts */
num_rules_with_non_solvable_conflicts: number;

/** A union of all tags of all rules available for upgrade */
tags: RuleTagArray;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ export const filterUnsupportedDiffOutcomes = (
const diff = value as ThreeWayDiff<unknown>;
return (
diff.diff_outcome !== ThreeWayDiffOutcome.CustomizedValueNoUpdate &&
diff.diff_outcome !== ThreeWayDiffOutcome.CustomizedValueSameUpdate
diff.diff_outcome !== ThreeWayDiffOutcome.CustomizedValueSameUpdate &&
diff.diff_outcome !== ThreeWayDiffOutcome.MissingBaseNoUpdate
);
})
);
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import {
KqlQueryType,
ThreeWayDiffConflict,
ThreeWayDiffOutcome,
ThreeWayMergeOutcome,
} from '../../../../../common/api/detection_engine';
Expand All @@ -18,8 +19,9 @@ import { PerFieldRuleDiffTab } from './per_field_rule_diff_tab';

const ruleFieldsDiffBaseFieldsMock = {
diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate,
has_conflict: false,
conflict: ThreeWayDiffConflict.NONE,
has_update: true,
has_base_version: true,
merge_outcome: ThreeWayMergeOutcome.Target,
};

Expand All @@ -33,7 +35,9 @@ const ruleFieldsDiffMock: PartialRuleDiff = {
target_version: 2,
},
},
has_conflict: false,
num_fields_with_updates: 1,
num_fields_with_conflicts: 0,
num_fields_with_non_solvable_conflicts: 0,
};

const renderPerFieldRuleDiffTab = (ruleDiff: PartialRuleDiff) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,34 @@ export const reviewRuleUpgradeRoute = (router: SecuritySolutionPluginRouter) =>
};

const calculateRuleStats = (results: CalculateRuleDiffResult[]): RuleUpgradeStatsForReview => {
const allTags = new Set<string>(
results.flatMap((result) => result.ruleVersions.input.current?.tags ?? [])
const allTags = new Set<string>();

const stats = results.reduce(
(acc, result) => {
acc.num_rules_to_upgrade_total += 1;

if (result.ruleDiff.num_fields_with_conflicts > 0) {
acc.num_rules_with_conflicts += 1;
}

if (result.ruleDiff.num_fields_with_non_solvable_conflicts > 0) {
acc.num_rules_with_non_solvable_conflicts += 1;
}

result.ruleVersions.input.current?.tags.forEach((tag) => allTags.add(tag));

return acc;
},
{
num_rules_to_upgrade_total: 0,
num_rules_with_conflicts: 0,
num_rules_with_non_solvable_conflicts: 0,
}
);

return {
num_rules_to_upgrade_total: results.length,
tags: [...allTags].sort((a, b) => a.localeCompare(b)),
...stats,
tags: Array.from(allTags),
};
};

Expand Down Expand Up @@ -123,9 +145,13 @@ const calculateRuleInfos = (results: CalculateRuleDiffResult[]): RuleUpgradeInfo
diff: {
fields: pickBy<ThreeWayDiff<unknown>>(
ruleDiff.fields,
(fieldDiff) => fieldDiff.diff_outcome !== ThreeWayDiffOutcome.StockValueNoUpdate
(fieldDiff) =>
fieldDiff.diff_outcome !== ThreeWayDiffOutcome.StockValueNoUpdate &&
fieldDiff.diff_outcome !== ThreeWayDiffOutcome.MissingBaseNoUpdate
),
has_conflict: ruleDiff.has_conflict,
num_fields_with_updates: ruleDiff.num_fields_with_updates,
num_fields_with_conflicts: ruleDiff.num_fields_with_conflicts,
num_fields_with_non_solvable_conflicts: ruleDiff.num_fields_with_non_solvable_conflicts,
},
};
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ import type {
DiffableRule,
FullRuleDiff,
ThreeWayDiff,
RuleFieldsDiff,
} from '../../../../../../common/api/detection_engine/prebuilt_rules';
import {
MissingVersion,
ThreeWayDiffConflict,
} from '../../../../../../common/api/detection_engine/prebuilt_rules';
import { MissingVersion } from '../../../../../../common/api/detection_engine/prebuilt_rules';
import type { RuleResponse } from '../../../../../../common/api/detection_engine/model/rule_schema';
import { invariant } from '../../../../../../common/utils/invariant';
import type { PrebuiltRuleAsset } from '../../model/rule_assets/prebuilt_rule_asset';
Expand Down Expand Up @@ -75,14 +79,18 @@ export const calculateRuleDiff = (args: RuleVersions): CalculateRuleDiffResult =
target_version: diffableTargetVersion,
});

const hasAnyFieldConflict = Object.values<ThreeWayDiff<unknown>>(fieldsDiff).some(
(fieldDiff) => fieldDiff.has_conflict
);
const {
numberFieldsWithUpdates,
numberFieldsWithConflicts,
numberFieldsWithNonSolvableConflicts,
} = getNumberOfFieldsByChangeType(fieldsDiff);

return {
ruleDiff: {
fields: fieldsDiff,
has_conflict: hasAnyFieldConflict,
num_fields_with_updates: numberFieldsWithUpdates,
num_fields_with_conflicts: numberFieldsWithConflicts,
num_fields_with_non_solvable_conflicts: numberFieldsWithNonSolvableConflicts,
},
ruleVersions: {
input: {
Expand All @@ -98,3 +106,31 @@ export const calculateRuleDiff = (args: RuleVersions): CalculateRuleDiffResult =
},
};
};

const getNumberOfFieldsByChangeType = (fieldsDiff: RuleFieldsDiff) =>
Object.values<ThreeWayDiff<unknown>>(fieldsDiff).reduce<{
numberFieldsWithUpdates: number;
numberFieldsWithConflicts: number;
numberFieldsWithNonSolvableConflicts: number;
}>(
(counts, fieldDiff) => {
if (fieldDiff.has_update) {
counts.numberFieldsWithUpdates += 1;
}

if (fieldDiff.conflict !== ThreeWayDiffConflict.NONE) {
counts.numberFieldsWithConflicts += 1;

if (fieldDiff.conflict === ThreeWayDiffConflict.NON_SOLVABLE) {
counts.numberFieldsWithNonSolvableConflicts += 1;
}
}

return counts;
},
{
numberFieldsWithUpdates: 0,
numberFieldsWithConflicts: 0,
numberFieldsWithNonSolvableConflicts: 0,
}
);
Loading

0 comments on commit fb82b0e

Please sign in to comment.