Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Timeline] Rebuild nested fields structure from fields response #96187

Merged
merged 11 commits into from
Apr 12, 2021
Merged

[Security Solution][Timeline] Rebuild nested fields structure from fields response #96187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1cb213c
First pass at rebuilding nested object structure from fields response
rylnd Apr 2, 2021
cab30d4
Build objects from arbitrary levels of nesting
rylnd Apr 3, 2021
69fd5a6
Simplify parsing logic, perf improvements
rylnd Apr 3, 2021
9f8bdad
Simplify if branching
rylnd Apr 3, 2021
168612b
Declare/export a more accurate fields type
rylnd Apr 3, 2021
8143acd
Remove unnecessary condition
rylnd Apr 3, 2021
e5aada2
Fix failing tests
rylnd Apr 8, 2021
f9a598a
Merge branch 'master' into rebuild_nested_fields_structure
kibanamachine Apr 8, 2021
bbbe168
Merge branch 'master' into rebuild_nested_fields_structure
kibanamachine Apr 12, 2021
f60a165
Merge branch 'master' into rebuild_nested_fields_structure
rylnd Apr 12, 2021
9bc1ea6
Merge branch 'master' into rebuild_nested_fields_structure
kibanamachine Apr 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...curity_solution/common/search_strategy/security_solution/matrix_histogram/events/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ export interface EventsActionGroupData {
doc_count: number;
}

export type Fields = Record<string, unknown[] | Fields[]>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type is referring to itself is that kosher? does it recurse? broke my brain a little bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's recursive! This was the most accurate representation of nested fields (and thus "fields" in general) I could think of. It appears to work with all our existing uses of the property without any changes because unknown[] is greedier than Fields[], but e.g. I needed to narrow in cases where we know it's gong to be the recursive form.


export interface EventHit extends SearchHit {
sort: string[];
_source: EventSource;
fields: Record<string, unknown[]>;
fields: Fields;
aggregations: {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[agg: string]: any;
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/security_solution/common/utils/mock_event_details.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export const eventHit = {
'source.geo.location': [{ coordinates: [118.7778, 32.0617], type: 'Point' }],
'threat.indicator': [
{
'matched.field': ['matched_field'],
'matched.field': ['matched_field', 'other_matched_field'],
first_seen: ['2021-02-22T17:29:25.195Z'],
provider: ['yourself'],
type: ['custom'],
Expand Down Expand Up @@ -259,8 +259,8 @@ export const eventDetailsFormattedFields = [
{
category: 'threat',
field: 'threat.indicator.matched.field',
values: ['matched_field', 'matched_field_2'],
originalValue: ['matched_field', 'matched_field_2'],
values: ['matched_field', 'other_matched_field', 'matched_field_2'],
originalValue: ['matched_field', 'other_matched_field', 'matched_field_2'],
isObjectArray: false,
},
{
Expand Down
10 changes: 10 additions & 0 deletions ...plugins/security_solution/server/search_strategy/timeline/factory/events/all/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@
* 2.0.
*/

export const TIMELINE_CTI_FIELDS = [
'threat.indicator.event.dataset',
'threat.indicator.event.reference',
'threat.indicator.matched.atomic',
'threat.indicator.matched.field',
'threat.indicator.matched.type',
'threat.indicator.provider',
];

export const TIMELINE_EVENTS_FIELDS = [
'@timestamp',
'signal.status',
Expand Down Expand Up @@ -230,4 +239,5 @@ export const TIMELINE_EVENTS_FIELDS = [
'zeek.ssl.established',
'zeek.ssl.resumed',
'zeek.ssl.version',
...TIMELINE_CTI_FIELDS,
];
209 changes: 203 additions & 6 deletions ...gins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
* 2.0.
*/

import { eventHit } from '../../../../../../common/utils/mock_event_details';
import { EventHit } from '../../../../../../common/search_strategy';
import { TIMELINE_EVENTS_FIELDS } from './constants';
import { formatTimelineData } from './helpers';
import { eventHit } from '../../../../../../common/utils/mock_event_details';
import { buildObjectForFieldPath, formatTimelineData } from './helpers';

describe('#formatTimelineData', () => {
it('happy path', async () => {
Expand Down Expand Up @@ -42,12 +42,12 @@ describe('#formatTimelineData', () => {
value: ['beats-ci-immutable-ubuntu-1804-1605624279743236239'],
},
{
field: 'source.geo.location',
value: [`{"lon":118.7778,"lat":32.0617}`],
field: 'threat.indicator.matched.field',
value: ['matched_field', 'other_matched_field', 'matched_field_2'],
},
{
field: 'threat.indicator.matched.field',
value: ['matched_field', 'matched_field_2'],
field: 'source.geo.location',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order here changed due to the addition of threat match fields to the ECS fields list, meaning it comes sooner than a requested field like this one (source.geo.location)

value: [`{"lon":118.7778,"lat":32.0617}`],
},
],
ecs: {
Expand Down Expand Up @@ -94,6 +94,34 @@ describe('#formatTimelineData', () => {
user: {
name: ['jenkins'],
},
threat: {
rylnd marked this conversation as resolved.
Show resolved Hide resolved
indicator: [
{
event: {
dataset: [],
reference: [],
},
matched: {
atomic: ['matched_atomic'],
field: ['matched_field', 'other_matched_field'],
type: [],
},
provider: ['yourself'],
},
{
event: {
dataset: [],
reference: [],
},
matched: {
atomic: ['matched_atomic_2'],
field: ['matched_field_2'],
type: [],
},
provider: ['other_you'],
},
],
},
},
},
});
Expand Down Expand Up @@ -371,4 +399,173 @@ describe('#formatTimelineData', () => {
},
});
});

describe('buildObjectForFieldPath', () => {
it('builds an object from a single non-nested field', () => {
expect(buildObjectForFieldPath('@timestamp', eventHit)).toEqual({
'@timestamp': ['2020-11-17T14:48:08.922Z'],
});
});

it('builds an object with no fields response', () => {
Copy link
Contributor Author

@rylnd rylnd Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XavierM I added this test and the accompanying null check because the parseEqlResponse unit tests were feeding in events without a fields property and failing. It certainly seems plausible for a hit not to have that property, hence the changes here, but I wanted to double-check it with you as well.

const { fields, ...fieldLessHit } = eventHit;
// @ts-expect-error fieldLessHit is intentionally missing fields
expect(buildObjectForFieldPath('@timestamp', fieldLessHit)).toEqual({
'@timestamp': [],
});
});

it('does not misinterpret non-nested fields with a common prefix', () => {
// @ts-expect-error hit is minimal
const hit: EventHit = {
fields: {
'foo.bar': ['baz'],
'foo.barBaz': ['foo'],
},
};

expect(buildObjectForFieldPath('foo.barBaz', hit)).toEqual({
foo: { barBaz: ['foo'] },
});
});

it('builds an array of objects from a nested field', () => {
// @ts-expect-error hit is minimal
const hit: EventHit = {
fields: {
foo: [{ bar: ['baz'] }],
},
};
expect(buildObjectForFieldPath('foo.bar', hit)).toEqual({
foo: [{ bar: ['baz'] }],
});
});

it('builds intermediate objects for nested fields', () => {
// @ts-expect-error nestedHit is minimal
const nestedHit: EventHit = {
fields: {
'foo.bar': [
{
baz: ['host.name'],
},
],
},
};
expect(buildObjectForFieldPath('foo.bar.baz', nestedHit)).toEqual({
foo: {
bar: [
{
baz: ['host.name'],
},
],
},
});
});

it('builds intermediate objects at multiple levels', () => {
expect(buildObjectForFieldPath('threat.indicator.matched.atomic', eventHit)).toEqual({
threat: {
indicator: [
{
matched: {
atomic: ['matched_atomic'],
},
},
{
matched: {
atomic: ['matched_atomic_2'],
},
},
],
},
});
});

it('preserves multiple values for a single leaf', () => {
expect(buildObjectForFieldPath('threat.indicator.matched.field', eventHit)).toEqual({
threat: {
indicator: [
{
matched: {
field: ['matched_field', 'other_matched_field'],
},
},
{
matched: {
field: ['matched_field_2'],
},
},
],
},
});
});

describe('multiple levels of nested fields', () => {
let nestedHit: EventHit;

beforeEach(() => {
// @ts-expect-error nestedHit is minimal
nestedHit = {
fields: {
'nested_1.foo': [
{
'nested_2.bar': [
{ leaf: ['leaf_value'], leaf_2: ['leaf_2_value'] },
{ leaf_2: ['leaf_2_value_2', 'leaf_2_value_3'] },
],
},
{
'nested_2.bar': [
{ leaf: ['leaf_value_2'], leaf_2: ['leaf_2_value_4'] },
{ leaf: ['leaf_value_3'], leaf_2: ['leaf_2_value_5'] },
],
},
],
},
};
});
Comment on lines +505 to +527
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need the let or beforeEach

Suggested change
let nestedHit: EventHit;
beforeEach(() => {
// @ts-expect-error nestedHit is minimal
nestedHit = {
fields: {
'nested_1.foo': [
{
'nested_2.bar': [
{ leaf: ['leaf_value'], leaf_2: ['leaf_2_value'] },
{ leaf_2: ['leaf_2_value_2', 'leaf_2_value_3'] },
],
},
{
'nested_2.bar': [
{ leaf: ['leaf_value_2'], leaf_2: ['leaf_2_value_4'] },
{ leaf: ['leaf_value_3'], leaf_2: ['leaf_2_value_5'] },
],
},
],
},
};
});
// @ts-expect-error nestedHit is minimal
const nestedHit: EventHit = {
fields: {
'nested_1.foo': [
{
'nested_2.bar': [
{ leaf: ['leaf_value'], leaf_2: ['leaf_2_value'] },
{ leaf_2: ['leaf_2_value_2', 'leaf_2_value_3'] },
],
},
{
'nested_2.bar': [
{ leaf: ['leaf_value_2'], leaf_2: ['leaf_2_value_4'] },
{ leaf: ['leaf_value_3'], leaf_2: ['leaf_2_value_5'] },
],
},
],
},
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this let/beforeEach is my paranoid "reset test data" pattern, meant to address the case where some function under test permutes its arguments. Consider the following scenario:

describe('permuting test data', () => {
  const a = ['foo'];

  it('works once', () => {
    expect(a).toEqual(['foo']);
    a.push('bar');
  });

  it('then breaks', () => {
    expect(a).toEqual(['foo']);
  });
});

Contrived example? Yes, we're not usually writing code with side effects like this, and should strive not to. However, it inevitably happens, and occasionally the solution is to "fix" the tests such that they work against the permuted data. And then we have both order-dependent and incorrect tests 😦 .

Given the fact that this suite was using that shared eventHit variable all over the place, and I had no idea how the code worked before writing these new tests, I opted to go this route and ensure that these new tests were "clean" and not subject to previous permutations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotchya


it('includes objects without the field', () => {
expect(buildObjectForFieldPath('nested_1.foo.nested_2.bar.leaf', nestedHit)).toEqual({
nested_1: {
foo: [
{
nested_2: {
bar: [{ leaf: ['leaf_value'] }, { leaf: [] }],
},
},
{
nested_2: {
bar: [{ leaf: ['leaf_value_2'] }, { leaf: ['leaf_value_3'] }],
},
},
],
},
});
});

it('groups multiple leaf values', () => {
expect(buildObjectForFieldPath('nested_1.foo.nested_2.bar.leaf_2', nestedHit)).toEqual({
nested_1: {
foo: [
{
nested_2: {
bar: [
{ leaf_2: ['leaf_2_value'] },
{ leaf_2: ['leaf_2_value_2', 'leaf_2_value_3'] },
],
},
},
{
nested_2: {
bar: [{ leaf_2: ['leaf_2_value_4'] }, { leaf_2: ['leaf_2_value_5'] }],
},
},
],
},
});
});
});
});
});
Loading