Skip to content

Commit

Permalink
[Security Solution] Unskip bulk actions integration tests (#174757)
Browse files Browse the repository at this point in the history 
**Resolves: #173804**

500 runs of the flaky test file in ESS env: [Buildkite
4833](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4833)

Even though I couldn't reproduce flakiness (1.5K runs), I've updated the
test to fix a suspected race condition.

What seems to be happening:
In the `beforeEach`
[here](https://github.com/nikitaindik/kibana/blob/b7b95111e010cc9fba618b625bb6f8c8969de1ec/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_bulk_actions/perform_bulk_action_ess.ts#L477),
three rules are created:
- one with investigation_fields equal to `['client.address',
'agent.name']`
 - one with investigation_fields equal to `[]`
- one with investigation_fields equal to `{ field_names: ['host.name']
}`

Then, in the
[test](https://github.com/nikitaindik/kibana/blob/b7b95111e010cc9fba618b625bb6f8c8969de1ec/x-pack/test/security_solution_api_integration/test_suites/detections_response/default_license/rule_bulk_actions/perform_bulk_action_ess.ts#L506),
we read all three rules and check them in that same order. But if the
`[]` rule is created on the backend before the `['client.address',
'agent.name']` rule, our first check would fail.

Other tests in this file don't expect rules to come in a particular
order. Instead, they search for a needed rule in the array. I used the
same approach for this flaky test.

(cherry picked from commit 3213541)
  • Loading branch information
@nikitaindik
nikitaindik committed Jan 26, 2024
1 parent 5ebb15e commit a689e5b
Show file tree
Hide file tree
Showing 3 changed files with 189 additions and 104 deletions.
254 changes: 150 additions & 104 deletions .../test/detection_engine_api_integration/security_and_spaces/group10/perform_bulk_action.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { getCreateExceptionListItemMinimalSchemaMock } from '@kbn/lists-plugin/c
import { WebhookAuthType } from '@kbn/stack-connectors-plugin/common/webhook/constants';
import {
binaryToString,
checkInvestigationFieldSoValue,
createLegacyRuleAction,
createRule,
createSignalsIndex,
Expand All @@ -45,6 +46,7 @@ import {
getRuleSavedObjectWithLegacyInvestigationFieldsEmptyArray,
deleteAllExceptions,
} from '../../utils';
import { createAlertsIndex } from '../../../security_solution_api_integration/test_suites/detections_response/utils/alerts/create_alerts_index';
import { FtrProviderContext } from '../../common/ftr_provider_context';

// eslint-disable-next-line import/no-default-export
Expand Down Expand Up @@ -3008,20 +3010,23 @@ export default ({ getService }: FtrProviderContext): void => {
describe('legacy investigation fields', () => {
let ruleWithLegacyInvestigationField: Rule<BaseRuleParams>;
let ruleWithLegacyInvestigationFieldEmptyArray: Rule<BaseRuleParams>;
let ruleWithIntendedInvestigationField: RuleResponse;

beforeEach(async () => {
await deleteAllAlerts(supertest, log, es);
await deleteAllRules(supertest, log);
await createSignalsIndex(supertest, log);
await createAlertsIndex(supertest, log);
ruleWithLegacyInvestigationField = await createRuleThroughAlertingEndpoint(
supertest,
getRuleSavedObjectWithLegacyInvestigationFields()
);

ruleWithLegacyInvestigationFieldEmptyArray = await createRuleThroughAlertingEndpoint(
supertest,
getRuleSavedObjectWithLegacyInvestigationFieldsEmptyArray()
);
await createRule(supertest, log, {

ruleWithIntendedInvestigationField = await createRule(supertest, log, {
...getSimpleRule('rule-with-investigation-field'),
name: 'Test investigation fields object',
investigation_fields: { field_names: ['host.name'] },
Expand All @@ -3041,23 +3046,26 @@ export default ({ getService }: FtrProviderContext): void => {
.parse(binaryToString);

const [rule1, rule2, rule3, exportDetailsJson] = body.toString().split(/\n/);
const exportedRules = [rule1, rule2, rule3].map((rule) => JSON.parse(rule));

const ruleToCompareWithLegacyInvestigationField = removeServerGeneratedProperties(
JSON.parse(rule1)
const exportedRuleWithLegacyInvestigationField = exportedRules.find(
(rule) => rule.id === ruleWithLegacyInvestigationField.id
);
expect(ruleToCompareWithLegacyInvestigationField.investigation_fields).to.eql({
expect(exportedRuleWithLegacyInvestigationField.investigation_fields).to.eql({
field_names: ['client.address', 'agent.name'],
});

const ruleToCompareWithLegacyInvestigationFieldEmptyArray = removeServerGeneratedProperties(
JSON.parse(rule2)
const exportedRuleWithLegacyInvestigationFieldEmptyArray = exportedRules.find(
(rule) => rule.id === ruleWithLegacyInvestigationFieldEmptyArray.id
);
expect(ruleToCompareWithLegacyInvestigationFieldEmptyArray.investigation_fields).to.eql(
expect(exportedRuleWithLegacyInvestigationFieldEmptyArray.investigation_fields).to.eql(
undefined
);

const ruleWithInvestigationField = removeServerGeneratedProperties(JSON.parse(rule3));
expect(ruleWithInvestigationField.investigation_fields).to.eql({
const exportedRuleWithInvestigationField = exportedRules.find(
(rule) => rule.id === ruleWithIntendedInvestigationField.id
);
expect(exportedRuleWithInvestigationField.investigation_fields).to.eql({
field_names: ['host.name'],
});

Expand All @@ -3066,12 +3074,14 @@ export default ({ getService }: FtrProviderContext): void => {
* the SO itself is migrated to the inteded object type, or if the transformation is
* happening just on the response. In this case, change should not include a migration on SO.
*/
const {
hits: {
hits: [{ _source: ruleSO }],
},
} = await getRuleSOById(es, JSON.parse(rule1).id);
expect(ruleSO?.alert?.params?.investigationFields).to.eql(['client.address', 'agent.name']);
const isInvestigationFieldMigratedInSo = await checkInvestigationFieldSoValue(
undefined,
{ field_names: ['client.address', 'agent.name'] },
es,
JSON.parse(rule1).id
);

expect(isInvestigationFieldMigratedInSo).to.eql(false);

const exportDetails = JSON.parse(exportDetailsJson);
expect(exportDetails).to.eql({
Expand Down Expand Up @@ -3156,7 +3166,6 @@ export default ({ getService }: FtrProviderContext): void => {
(returnedRule: RuleResponse) => returnedRule.rule_id === 'rule-with-investigation-field'
);
expect(ruleWithIntendedType.investigation_fields).to.eql({ field_names: ['host.name'] });

/**
* Confirm type on SO so that it's clear in the tests whether it's expected that
* the SO itself is migrated to the inteded object type, or if the transformation is
Expand All @@ -3167,7 +3176,12 @@ export default ({ getService }: FtrProviderContext): void => {
hits: [{ _source: ruleSO }],
},
} = await getRuleSOById(es, ruleWithLegacyField.id);
expect(ruleSO?.alert?.params?.investigationFields).to.eql(['client.address', 'agent.name']);

const isInvestigationFieldMigratedInSo = await checkInvestigationFieldSoValue(ruleSO, {
field_names: ['client.address', 'agent.name'],
});

expect(isInvestigationFieldMigratedInSo).to.eql(false);
expect(ruleSO?.alert?.enabled).to.eql(true);

const {
Expand Down Expand Up @@ -3226,26 +3240,36 @@ export default ({ getService }: FtrProviderContext): void => {
* the SO itself is migrated to the inteded object type, or if the transformation is
* happening just on the response. In this case, change should not include a migration on SO.
*/
const {
hits: {
hits: [{ _source: ruleSO }],
},
} = await getRuleSOById(es, ruleWithLegacyField.id);
expect(ruleSO?.alert?.params?.investigationFields).to.eql(['client.address', 'agent.name']);

const {
hits: {
hits: [{ _source: ruleSO2 }],
},
} = await getRuleSOById(es, ruleWithEmptyArray.id);
expect(ruleSO2?.alert?.params?.investigationFields).to.eql([]);
const isInvestigationFieldForRuleWithLegacyFieldMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{
field_names: ['client.address', 'agent.name'],
},
es,
ruleWithLegacyField.id
);
expect(isInvestigationFieldForRuleWithLegacyFieldMigratedInSo).to.eql(false);

const {
hits: {
hits: [{ _source: ruleSO3 }],
},
} = await getRuleSOById(es, ruleWithIntendedType.id);
expect(ruleSO3?.alert?.params?.investigationFields).to.eql({ field_names: ['host.name'] });
const isInvestigationFieldForRuleWithEmptyArraydMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{
field_names: [],
},
es,
ruleWithEmptyArray.id
);
expect(isInvestigationFieldForRuleWithEmptyArraydMigratedInSo).to.eql(false);

const isInvestigationFieldForRuleWithIntendedTypeMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: ['host.name'] },
es,
ruleWithIntendedType.id
);
expect(isInvestigationFieldForRuleWithIntendedTypeMigratedInSo).to.eql(true);
});

it('should duplicate rules with legacy investigation fields and transform field in response', async () => {
Expand Down Expand Up @@ -3289,66 +3313,82 @@ export default ({ getService }: FtrProviderContext): void => {
returnedRule.name === 'Test investigation fields object [Duplicate]'
);

// DUPLICATED RULES
/**
* Confirm type on SO so that it's clear in the tests whether it's expected that
* the SO itself is migrated to the inteded object type, or if the transformation is
* happening just on the response. In this case, duplicated
* rules should NOT have migrated value on write.
*/
const {
hits: {
hits: [{ _source: ruleSO }],
},
} = await getRuleSOById(es, ruleWithLegacyField.id);

expect(ruleSO?.alert?.params?.investigationFields).to.eql(['client.address', 'agent.name']);

const {
hits: {
hits: [{ _source: ruleSO2 }],
},
} = await getRuleSOById(es, ruleWithEmptyArray.id);
expect(ruleSO2?.alert?.params?.investigationFields).to.eql([]);

const {
hits: {
hits: [{ _source: ruleSO3 }],
},
} = await getRuleSOById(es, ruleWithIntendedType.id);
expect(ruleSO3?.alert?.params?.investigationFields).to.eql({ field_names: ['host.name'] });
const isInvestigationFieldForRuleWithLegacyFieldMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: ['client.address', 'agent.name'] },
es,
ruleWithLegacyField.id
);
expect(isInvestigationFieldForRuleWithLegacyFieldMigratedInSo).to.eql(false);

const isInvestigationFieldForRuleWithEmptyArrayMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: [] },
es,
ruleWithEmptyArray.id
);
expect(isInvestigationFieldForRuleWithEmptyArrayMigratedInSo).to.eql(false);

/*
It's duplicate of a rule with properly formatted "investigation fields".
So we just check that "investigation fields" are in intended format.
No migration needs to happen.
*/
const isInvestigationFieldForRuleWithIntendedTypeInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: ['host.name'] },
es,
ruleWithIntendedType.id
);
expect(isInvestigationFieldForRuleWithIntendedTypeInSo).to.eql(true);

// ORIGINAL RULES - rules selected to be duplicated
/**
* Confirm type on SO so that it's clear in the tests whether it's expected that
* the SO itself is migrated to the inteded object type, or if the transformation is
* happening just on the response. In this case, the original
* rules selected to be duplicated should not be migrated.
*/
const {
hits: {
hits: [{ _source: ruleSOOriginalLegacy }],
},
} = await getRuleSOById(es, ruleWithLegacyInvestigationField.id);

expect(ruleSOOriginalLegacy?.alert?.params?.investigationFields).to.eql([
'client.address',
'agent.name',
]);

const {
hits: {
hits: [{ _source: ruleSOOriginalLegacyEmptyArray }],
},
} = await getRuleSOById(es, ruleWithLegacyInvestigationFieldEmptyArray.id);
expect(ruleSOOriginalLegacyEmptyArray?.alert?.params?.investigationFields).to.eql([]);

const {
hits: {
hits: [{ _source: ruleSOOriginalNoLegacy }],
},
} = await getRuleSOById(es, ruleWithIntendedType.id);
expect(ruleSOOriginalNoLegacy?.alert?.params?.investigationFields).to.eql({
field_names: ['host.name'],
});
const isInvestigationFieldForOriginalRuleWithLegacyFieldMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: ['client.address', 'agent.name'] },
es,
ruleWithLegacyInvestigationField.id
);
expect(isInvestigationFieldForOriginalRuleWithLegacyFieldMigratedInSo).to.eql(false);

const isInvestigationFieldForOriginalRuleWithEmptyArrayMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: [] },
es,
ruleWithLegacyInvestigationFieldEmptyArray.id
);
expect(isInvestigationFieldForOriginalRuleWithEmptyArrayMigratedInSo).to.eql(false);

/*
Since this rule was created with intended "investigation fields" format,
it shouldn't change - no need to migrate.
*/
const isInvestigationFieldForOriginalRuleWithIntendedTypeInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: ['host.name'] },
es,
ruleWithIntendedInvestigationField.id
);
expect(isInvestigationFieldForOriginalRuleWithIntendedTypeInSo).to.eql(true);
});

it('should edit rules with legacy investigation fields', async () => {
Expand Down Expand Up @@ -3398,26 +3438,32 @@ export default ({ getService }: FtrProviderContext): void => {
* the SO itself is migrated to the inteded object type, or if the transformation is
* happening just on the response. In this case, change should not include a migration on SO.
*/
const {
hits: {
hits: [{ _source: ruleSO }],
},
} = await getRuleSOById(es, ruleWithLegacyInvestigationField.id);
expect(ruleSO?.alert?.params?.investigationFields).to.eql(['client.address', 'agent.name']);

const {
hits: {
hits: [{ _source: ruleSO2 }],
},
} = await getRuleSOById(es, ruleWithLegacyInvestigationFieldEmptyArray.id);
expect(ruleSO2?.alert?.params?.investigationFields).to.eql([]);

const {
hits: {
hits: [{ _source: ruleSO3 }],
},
} = await getRuleSOById(es, ruleWithIntendedType.id);
expect(ruleSO3?.alert?.params?.investigationFields).to.eql({ field_names: ['host.name'] });
const isInvestigationFieldForRuleWithLegacyFieldMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: ['client.address', 'agent.name'] },
es,
ruleWithLegacyInvestigationField.id
);
expect(isInvestigationFieldForRuleWithLegacyFieldMigratedInSo).to.eql(false);

const isInvestigationFieldForRuleWithEmptyArrayFieldMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: [] },
es,
ruleWithLegacyInvestigationFieldEmptyArray.id
);
expect(isInvestigationFieldForRuleWithEmptyArrayFieldMigratedInSo).to.eql(false);

const isInvestigationFieldForRuleWithIntendedTypeMigratedInSo =
await checkInvestigationFieldSoValue(
undefined,
{ field_names: ['host.name'] },
es,
ruleWithIntendedType.id
);
expect(isInvestigationFieldForRuleWithIntendedTypeMigratedInSo).to.eql(true);
});
});
});
Expand Down
38 changes: 38 additions & 0 deletions x-pack/test/detection_engine_api_integration/utils/check_investigation_field_in_so.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* 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.
*/

import { Client } from '@elastic/elasticsearch';
import { SavedObjectReference } from '@kbn/core/server';
import { InvestigationFields } from '@kbn/security-solution-plugin/common/api/detection_engine';
import { Rule } from '@kbn/alerting-plugin/common';
import { BaseRuleParams } from '@kbn/security-solution-plugin/server/lib/detection_engine/rule_schema';
import { isEqual } from 'lodash/fp';
import { getRuleSOById } from './get_rule_so_by_id';

interface RuleSO {
alert: Rule<BaseRuleParams>;
references: SavedObjectReference[];
}

export const checkInvestigationFieldSoValue = async (
ruleSO: RuleSO | undefined,
expectedSoValue: undefined | InvestigationFields,
es?: Client,
ruleId?: string
): Promise<boolean> => {
if (!ruleSO && es && ruleId) {
const {
hits: {
hits: [{ _source: rule }],
},
} = await getRuleSOById(es, ruleId);

return isEqual(rule?.alert.params.investigationFields, expectedSoValue);
}

return isEqual(ruleSO?.alert.params.investigationFields, expectedSoValue);
};
Loading

0 comments on commit a689e5b

Please sign in to comment.