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

[Endpoint] EMT-65: make endpoint data types common, restructure #54772

Merged
merged 7 commits into from
Jan 27, 2020
Merged

[Endpoint] EMT-65: make endpoint data types common, restructure #54772

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6577ba0
EMT-65: make endpoint data types common, restructure
nnamdifrankie Jan 14, 2020
5127ff5
Merge branch 'master' into EMT-65_naming_changes_are_restructuring
elasticmachine Jan 22, 2020
1128056
EMT-65: update test data and datastructure based on schema
nnamdifrankie Jan 24, 2020
ea26d3d
Merge branch 'master' into EMT-65_naming_changes_are_restructuring
elasticmachine Jan 25, 2020
d96e1f9
EMT-65: move sample files for future change
nnamdifrankie Jan 27, 2020
49d745c
Merge branch 'EMT-65_naming_changes_are_restructuring' of github.com:…
nnamdifrankie Jan 27, 2020
f5630f2
EMT-65: use date type
nnamdifrankie Jan 27, 2020
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
58 changes: 58 additions & 0 deletions x-pack/plugins/endpoint/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export class EndpointAppConstants {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to move a Class to this types.ts file? I don't see the FE needing it

Copy link
Contributor

Choose a reason for hiding this comment

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

@nnamdifrankie see my prior comment here. I don't think you should have a Class in this file (maybe you meant interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @nnamdifrankie is defining constants here, not types necessarily. So I would say that maybe we could move this somewhere else, if we're trying to have this file contain and export types for the app.

static ENDPOINT_INDEX_NAME = 'endpoint-agent*';
}

export interface EndpointResultList {
// the endpoint restricted by the page size
endpoints: EndpointData[];
// the total number of unique endpoints in the index
total: number;
// the page size requested
request_page_size: number;
// the index requested
request_page_index: number;
}

export interface EndpointData {
Copy link

Choose a reason for hiding this comment

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

@nnamdifrankie Is there a PR that defines this struct in ECS that the Endpoint team agreed upon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@james-elastic

This our current working agreement. We will need another revision it would seem, now that we have more experience. Once we do this we can make the changes on all sides.

https://docs.google.com/spreadsheets/d/1ROweh2FrHxEROcXPKWV7bvGEjg2baSA4W5dNxI5yOw8/edit#gid=0

Copy link

Choose a reason for hiding this comment

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

@nnamdifrankie Awesome! Can you condense that into a PR that's in the ECS yml format to https://github.com/elastic/endpoint-app-team/pulls - this is where we're tracking all the ECS data formats that the endpoint will be sending.

Copy link

Choose a reason for hiding this comment

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

I think the format should be in source control and easily found by everyone on the teams to make comments about the structure and format.

Copy link
Contributor

Choose a reason for hiding this comment

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

now merged: https://github.com/elastic/endpoint-app-team/pull/118

machine_id: string;
created_at: Date;
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. I think this is a string, not a Date object ?? 🤷‍♂ (same question for other date type of properties below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you think it is a str, is it because of the format.

Copy link
Contributor

@paul-tavares paul-tavares Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@nnamdifrankie JSON does not have a way to serialize/de-serialize dates. They will come across as Strings if the value of attribute is an actual JS Date object (defaults to (new Date()).toString() output which is normally the localized date+time).
My guess is that you are getting a string from Elasticsearch's query response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paul-tavares

"event": {
        "created": "2020-01-24T21:01:56.183Z"
      },

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @nnamdifrankie . So back to the type - what will it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paul-tavares I think we can map it at time of use or parameter definition. Once the other PR is merged and the type definition is available. Remember that we use it here as output, so we can map it on output or the caller can. Also recall that this is an api route and can be called by any client curl, front end e.t.c so it is not critical here for us and makes no difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @nnamdifrankie , I'm confused. Are you leaving the created_at defined as a Date? or String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to define it a date so that it is explicit that when data is deserialized into that EndpointMetadata object the value of that field should be a date type. Is this satisfactory?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine.
I'll follow up on the FE side as to how we want to handle this (ex. should we go through the known date fields we get from the API and convert them to Date object or just do the conversion when needed) (cc/ @oatkiller , @kevinlog )

host: {
name: string;
hostname: string;
ip: string;
mac_address: string;
os: {
name: string;
full: string;
};
};
endpoint: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think endpoint is the correct term to use here. endpoint is the code that is running on the computer. It seems the keys in this endpoint should be moved to host.

domain: string;
is_base_image: boolean;
active_directory_distinguished_name: string;
active_directory_hostname: string;
upgrade: {
status?: string;
updated_at?: Date;
};
isolation: {
status: boolean;
request_status?: string | boolean;
updated_at?: Date;
};
policy: {
name: string;
id: string;
};
sensor: {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change this to endpoint. sensor is an SMP term and will not be used in the Elastic security product.

persistence: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we will only have persistent endpoints with the new product. Perhaps capturing version number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can discuss this further with the larger team. As per @kevinlog we are going to peer down the data and change some of the references. Will involve us changing our generator too, so may not want to tackle that here. We just want to show the list.

status: object;
};
};
}
2 changes: 1 addition & 1 deletion x-pack/plugins/endpoint/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import { first } from 'rxjs/operators';
import { addRoutes } from './routes';
import { PluginSetupContract as FeaturesPluginSetupContract } from '../../features/server';
import { createConfig$, EndpointConfigType } from './config';
import { EndpointAppContext } from './types';
import { registerEndpointRoutes } from './routes/endpoints';
import { EndpointAppContext } from './types';

export type EndpointPluginStart = void;
export type EndpointPluginSetup = void;
Expand Down
8 changes: 4 additions & 4 deletions x-pack/plugins/endpoint/server/routes/endpoints.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ import {
httpServiceMock,
loggingServiceMock,
} from '../../../../../src/core/server/mocks';
import { EndpointData } from '../types';
import { EndpointData, EndpointResultList } from '../../common/types';
import { SearchResponse } from 'elasticsearch';
import { EndpointResultList, registerEndpointRoutes } from './endpoints';
import { registerEndpointRoutes } from './endpoints';
import { EndpointConfigSchema } from '../config';
import * as data from '../test_data/all_endpoints_data.json';

Expand Down Expand Up @@ -75,7 +75,7 @@ describe('test endpoint route', () => {
const endpointResultList = mockResponse.ok.mock.calls[0][0]?.body as EndpointResultList;
expect(endpointResultList.endpoints.length).toEqual(3);
expect(endpointResultList.total).toEqual(3);
expect(endpointResultList.request_index).toEqual(0);
expect(endpointResultList.request_page_index).toEqual(0);
expect(endpointResultList.request_page_size).toEqual(10);
});

Expand Down Expand Up @@ -117,7 +117,7 @@ describe('test endpoint route', () => {
const endpointResultList = mockResponse.ok.mock.calls[0][0]?.body as EndpointResultList;
expect(endpointResultList.endpoints.length).toEqual(3);
expect(endpointResultList.total).toEqual(3);
expect(endpointResultList.request_index).toEqual(10);
expect(endpointResultList.request_page_index).toEqual(10);
expect(endpointResultList.request_page_size).toEqual(10);
});
});
19 changes: 5 additions & 14 deletions x-pack/plugins/endpoint/server/routes/endpoints.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,15 @@
import { IRouter } from 'kibana/server';
import { SearchResponse } from 'elasticsearch';
import { schema } from '@kbn/config-schema';
import { EndpointAppContext, EndpointData } from '../types';

import { kibanaRequestToEndpointListQuery } from '../services/endpoint/endpoint_query_builders';
import { EndpointData, EndpointResultList } from '../../common/types';
import { EndpointAppContext } from '../types';

interface HitSource {
_source: EndpointData;
}

export interface EndpointResultList {
// the endpoint restricted by the page size
endpoints: EndpointData[];
// the total number of unique endpoints in the index
total: number;
// the page size requested
request_page_size: number;
// the index requested
request_index: number;
}

export function registerEndpointRoutes(router: IRouter, endpointAppContext: EndpointAppContext) {
router.post(
{
Expand Down Expand Up @@ -70,7 +61,7 @@ function mapToEndpointResultList(
if (searchResponse.hits.hits.length > 0) {
return {
request_page_size: queryParams.size,
request_index: queryParams.from,
request_page_index: queryParams.from,
endpoints: searchResponse.hits.hits
.map(response => response.inner_hits.most_recent.hits.hits)
.flatMap(data => data as HitSource)
Expand All @@ -80,7 +71,7 @@ function mapToEndpointResultList(
} else {
return {
request_page_size: queryParams.size,
request_index: queryParams.from,
request_page_index: queryParams.from,
total: totalNumberOfEndpoints,
endpoints: [],
};
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/endpoint/server/services/endpoint/endpoint_query_builders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { KibanaRequest } from 'kibana/server';
import { EndpointAppConstants, EndpointAppContext } from '../../types';
import { EndpointAppConstants } from '../../../common/types';
import { EndpointAppContext } from '../../types';

export const kibanaRequestToEndpointListQuery = async (
request: KibanaRequest<any, any, any>,
Expand Down
42 changes: 0 additions & 42 deletions x-pack/plugins/endpoint/server/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,45 +10,3 @@ export interface EndpointAppContext {
logFactory: LoggerFactory;
config(): Promise<EndpointConfigType>;
}

export class EndpointAppConstants {
static ENDPOINT_INDEX_NAME = 'endpoint-agent*';
}

export interface EndpointData {
machine_id: string;
created_at: Date;
host: {
name: string;
hostname: string;
ip: string;
mac_address: string;
os: {
name: string;
full: string;
};
};
endpoint: {
domain: string;
is_base_image: boolean;
active_directory_distinguished_name: string;
active_directory_hostname: string;
upgrade: {
status?: string;
updated_at?: Date;
};
isolation: {
status: boolean;
request_status?: string | boolean;
updated_at?: Date;
};
policy: {
name: string;
id: string;
};
sensor: {
persistence: boolean;
status: object;
};
};
}
8 changes: 4 additions & 4 deletions x-pack/test/api_integration/apis/endpoint/endpoints.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default function({ getService }: FtrProviderContext) {
expect(body.total).to.eql(0);
expect(body.endpoints.length).to.eql(0);
expect(body.request_page_size).to.eql(10);
expect(body.request_index).to.eql(0);
expect(body.request_page_index).to.eql(0);
});
});

Expand All @@ -37,7 +37,7 @@ export default function({ getService }: FtrProviderContext) {
expect(body.total).to.eql(3);
expect(body.endpoints.length).to.eql(3);
expect(body.request_page_size).to.eql(10);
expect(body.request_index).to.eql(0);
expect(body.request_page_index).to.eql(0);
});

it('endpoints api should return page based on params passed.', async () => {
Expand All @@ -58,7 +58,7 @@ export default function({ getService }: FtrProviderContext) {
expect(body.total).to.eql(3);
expect(body.endpoints.length).to.eql(1);
expect(body.request_page_size).to.eql(1);
expect(body.request_index).to.eql(1);
expect(body.request_page_index).to.eql(1);
});

/* test that when paging properties produces no result, the total should reflect the actual number of endpoints
Expand All @@ -82,7 +82,7 @@ export default function({ getService }: FtrProviderContext) {
expect(body.total).to.eql(3);
expect(body.endpoints.length).to.eql(0);
expect(body.request_page_size).to.eql(10);
expect(body.request_index).to.eql(30);
expect(body.request_page_index).to.eql(30);
});

it('endpoints api should return 400 when pagingProperties is below boundaries.', async () => {
Expand Down