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.

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-67: add kql support for endpoint list #56328

65 changes: 64 additions & 1 deletion x-pack/plugins/endpoint/server/routes/endpoints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ describe('test endpoint route', () => {
expect(endpointResultList.request_page_size).toEqual(10);
});

it('test find the latest of all endpoints with params', async () => {
it('test find the latest of all endpoints with paging properties', async () => {
const mockRequest = httpServerMock.createKibanaRequest({
body: {
paging_properties: [
Expand Down Expand Up @@ -112,6 +112,69 @@ describe('test endpoint route', () => {
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser.mock.calls[0][1]?.body?.query).toEqual({
match_all: {},
});
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.ok).toBeCalled();
const endpointResultList = mockResponse.ok.mock.calls[0][0]?.body as EndpointResultList;
expect(endpointResultList.endpoints.length).toEqual(2);
expect(endpointResultList.total).toEqual(2);
expect(endpointResultList.request_page_index).toEqual(10);
expect(endpointResultList.request_page_size).toEqual(10);
});

it('test find the latest of all endpoints with paging and filters properties', async () => {
const mockRequest = httpServerMock.createKibanaRequest({
body: {
paging_properties: [
{
page_size: 10,
},
{
page_index: 1,
},
],

filters: 'not host.ip:10.140.73.246',
},
});
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() =>
Promise.resolve((data as unknown) as SearchResponse<EndpointMetadata>)
);
[routeConfig, routeHandler] = routerMock.post.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/endpoints')
)!;

await routeHandler(
({
core: {
elasticsearch: {
dataClient: mockScopedClient,
},
},
} as unknown) as RequestHandlerContext,
mockRequest,
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(mockScopedClient.callAsCurrentUser.mock.calls[0][1]?.body?.query).toEqual({
bool: {
must_not: {
bool: {
minimum_should_match: 1,
should: [
{
match: {
'host.ip': '10.140.73.246',
},
},
],
},
},
},
});
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.ok).toBeCalled();
const endpointResultList = mockResponse.ok.mock.calls[0][0]?.body as EndpointResultList;
Expand Down
28 changes: 19 additions & 9 deletions x-pack/plugins/endpoint/server/routes/endpoints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,26 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp
validate: {
body: schema.nullable(
schema.object({
paging_properties: schema.arrayOf(
schema.oneOf([
// the number of results to return for this request per page
schema.object({
page_size: schema.number({ defaultValue: 10, min: 1, max: 10000 }),
}),
// the index of the page to return
schema.object({ page_index: schema.number({ defaultValue: 0, min: 0 }) }),
])
paging_properties: schema.nullable(
Copy link
Contributor

@madirey madirey Jan 31, 2020

Choose a reason for hiding this comment

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

We're also using paging_properties and filters for alert list, so let's consider moving this schema out to a common 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.

@madirey I have a refactor ticket coming behind this and can move it then, if this is fine with you. I think we should share only things we strongly feel are really generic, such that other paths in the system does not consume non relevant changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure @nnamdifrankie , we can refactor once both endpoints are integrated too, might be easier that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madirey I see your point now, since it one app we cannot have different names for the same thing. Thanks for the input. Once we merge let work together to merge some of the properties.

schema.arrayOf(
schema.oneOf([
/**
* the number of results to return for this request per page
*/
schema.object({
page_size: schema.number({ defaultValue: 10, min: 1, max: 10000 }),
}),
/**
* the index of the first result in the page in the total matching set
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem to be the correct description based on the current implementation. page_index is the actual page number (where 0 is the first page). The actual first result index within the total matching set is calculated based on this number and the page_size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

*/
schema.object({ page_index: schema.number({ defaultValue: 0, min: 0 }) }),
])
)
),
/**
* filter to be applied, it could be a kql expression or discrete filter to be implemented
*/
filters: schema.nullable(schema.oneOf([schema.string()])),
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just schema.string()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also - we have talked about having other types of filtering possible in the future. Should this param be named something else other than filter? (FYI: in Ingest, they used a param named kuery.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry - just remember another thing.
I think (during x-school) I heard that things like support for KQL is only available at certain licensing levels (like not in Free version). Do we need to do that type of validation here? (we likely have the same question to answer on the UI side 😃 )

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 am sure you are right about this. But there are many licenses and license has to be configured at the plugin level using license plugin. And we also have to know what the minimum level of license, basic e.t.c. And what features should be free or licensed. Do you want us to move take offline? and not address it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sound good @nnamdifrankie . I just did quick check around KQL and it seems that its available at all levels - its the AutoComplete functionality that seems to only be enabled for Basic license and above (ref: https://www.elastic.co/guide/en/kibana/current/kuery-query.html#kuery-query), so I don't think you need any checks here

})
),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,65 @@ describe('query builder', () => {
});
});

describe('test query builder with kql filter', () => {
it('test default query params for all endpoints when no params or body is provided', async () => {
const mockRequest = httpServerMock.createKibanaRequest({
body: {
filters: 'not host.ip:10.140.73.246',
},
});
const query = await kibanaRequestToEndpointListQuery(mockRequest, {
logFactory: loggingServiceMock.create(),
config: () => Promise.resolve(EndpointConfigSchema.validate({})),
});
expect(query).toEqual({
body: {
query: {
bool: {
must_not: {
bool: {
minimum_should_match: 1,
should: [
{
match: {
'host.ip': '10.140.73.246',
},
},
],
},
},
},
},
collapse: {
field: 'host.id.keyword',
inner_hits: {
name: 'most_recent',
size: 1,
sort: [{ 'event.created': 'desc' }],
},
},
aggs: {
total: {
cardinality: {
field: 'host.id.keyword',
},
},
},
sort: [
{
'event.created': {
order: 'desc',
},
},
],
},
from: 0,
size: 10,
index: 'endpoint-agent*',
} as Record<string, any>);
});
});

describe('EndpointFetchQuery', () => {
it('searches for the correct ID', () => {
const mockID = 'AABBCCDD-0011-2233-AA44-DEADBEEF8899';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
import { KibanaRequest } from 'kibana/server';
import { EndpointAppConstants } from '../../../common/types';
import { EndpointAppContext } from '../../types';
import {
fromKueryExpression,
toElasticsearchQuery,
} from '../../../../../../src/plugins/data/common/es_query/kuery/ast';

export const kibanaRequestToEndpointListQuery = async (
request: KibanaRequest<any, any, any>,
Expand All @@ -14,9 +18,7 @@ export const kibanaRequestToEndpointListQuery = async (
const pagingProperties = await getPagingProperties(request, endpointAppContext);
return {
body: {
query: {
match_all: {},
},
query: buildQueryBody(request),
collapse: {
field: 'host.id.keyword',
inner_hits: {
Expand Down Expand Up @@ -66,6 +68,15 @@ async function getPagingProperties(
};
}

function buildQueryBody(request: KibanaRequest<any, any, any>): Record<string, any> {
if (request?.body?.filters && typeof request.body.filters === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm new to the ? operator - very cool. Could this be shortened to just typeof request?.body?.filters === 'string'?

return toElasticsearchQuery(fromKueryExpression(request.body.filters));
Copy link
Member

Choose a reason for hiding this comment

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

is this the KQL translation magic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is using function from the AST (Abstract Syntax Tree) Module to transform the text to an elastic query. Not magical, just classic language syntax tree transformation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to be using this function too, for the alert list page... let's think about pulling this out into a common location?

}
return {
match_all: {},
};
}

export const kibanaRequestToEndpointFetchQuery = (
request: KibanaRequest<any, any, any>,
endpointAppContext: EndpointAppContext
Expand Down
36 changes: 35 additions & 1 deletion x-pack/test/api_integration/apis/endpoint/endpoints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export default function({ getService }: FtrProviderContext) {
expect(body.request_page_index).to.eql(0);
});

it('endpoints api should return page based on params passed.', async () => {
it('endpoints api should return page based on paging properties passed.', async () => {
const { body } = await supertest
.post('/api/endpoint/endpoints')
.set('kbn-xsrf', 'xxx')
Expand Down Expand Up @@ -102,6 +102,40 @@ export default function({ getService }: FtrProviderContext) {
.expect(400);
expect(body.message).to.contain('Value is [0] but it must be equal to or greater than [1]');
});

it('endpoints api should return page based on filters passed.', async () => {
const { body } = await supertest
.post('/api/endpoint/endpoints')
.set('kbn-xsrf', 'xxx')
.send({ filters: 'not host.ip:10.101.149.26' })
.expect(200);
expect(body.total).to.eql(2);
expect(body.endpoints.length).to.eql(2);
expect(body.request_page_size).to.eql(10);
expect(body.request_page_index).to.eql(0);
});

it('endpoints api should return page based on filters and paging passed.', async () => {
const { body } = await supertest
.post('/api/endpoint/endpoints')
.set('kbn-xsrf', 'xxx')
.send({
paging_properties: [
{
page_size: 1,
},
{
page_index: 1,
},
],
filters: 'not host.ip:10.101.149.26',
})
.expect(200);
expect(body.total).to.eql(2);
expect(body.endpoints.length).to.eql(1);
expect(body.request_page_size).to.eql(1);
expect(body.request_page_index).to.eql(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have any testing around the actual set of endpoints returned and ensuring its the expected set for the requested page_index + page_size?

});
});
});
}