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-issue-65: add endpoint list api #53861

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
08343b2
add endpoint list api
nnamdifrankie Dec 31, 2019
2e70a4c
Merge branch 'master' into emt-issue-65_add-endpoint_list_api
nnamdifrankie Dec 31, 2019
7dfb50a
Merge branch 'master' into emt-issue-65_add-endpoint_list_api
elasticmachine Jan 2, 2020
91bfbd3
prefix property with endpoint
nnamdifrankie Jan 2, 2020
4ecfbf2
Merge branch 'emt-issue-65_add-endpoint_list_api' of github.com:nnamd…
nnamdifrankie Jan 2, 2020
6327a4f
review comments changes
nnamdifrankie Jan 2, 2020
cad4f76
add integration test, add more comments on properties and test
nnamdifrankie Jan 2, 2020
2b01cf5
Merge branch 'master' into emt-issue-65_add-endpoint_list_api
elasticmachine Jan 2, 2020
42e4f07
add more comments, change query params type
nnamdifrankie Jan 3, 2020
b6ca98f
Merge branch 'emt-issue-65_add-endpoint_list_api' of github.com:nnamd…
nnamdifrankie Jan 3, 2020
06ba203
remove accent from description
nnamdifrankie Jan 3, 2020
7718594
Merge branch 'master' into emt-issue-65_add-endpoint_list_api
elasticmachine Jan 3, 2020
18aaa7b
add more testing, use more function
nnamdifrankie Jan 6, 2020
13fe43a
Merge branch 'emt-issue-65_add-endpoint_list_api' of github.com:nnamd…
nnamdifrankie Jan 6, 2020
0be7a08
Merge branch 'master' into emt-issue-65_add-endpoint_list_api
elasticmachine Jan 6, 2020
b8ed0e8
fix type check
nnamdifrankie Jan 6, 2020
ac81bae
Merge branch 'emt-issue-65_add-endpoint_list_api' of github.com:nnamd…
nnamdifrankie Jan 6, 2020
0686689
fix type check, fix api variable case
nnamdifrankie Jan 6, 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
15 changes: 15 additions & 0 deletions x-pack/plugins/endpoint/server/config.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* 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.
*/
import { EndpointConfigSchema, EndpointConfigType } from './config';

describe('test config schema', () => {
it('test config defaults', () => {
const config: EndpointConfigType = EndpointConfigSchema.validate({});
expect(config.enabled).toEqual(false);
expect(config.endpointResultListDefaultPageSize).toEqual(10);
expect(config.endpointResultListDefaultFirstPageIndex).toEqual(0);
});
});
22 changes: 22 additions & 0 deletions x-pack/plugins/endpoint/server/config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* 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.
*/
import { schema, TypeOf } from '@kbn/config-schema';
import { Observable } from 'rxjs';
import { PluginInitializerContext } from 'kibana/server';

export type EndpointConfigType = ReturnType<typeof createConfig$> extends Observable<infer P>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you explain this? If I'm understanding this correctly, createConfig$ always returns a value that extends Observable, so the second part of this expression is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? P
: ReturnType<typeof createConfig$>;

export const EndpointConfigSchema = schema.object({
enabled: schema.boolean({ defaultValue: false }),
endpointResultListDefaultFirstPageIndex: schema.number({ defaultValue: 0 }),
endpointResultListDefaultPageSize: schema.number({ defaultValue: 10 }),
});

export function createConfig$(context: PluginInitializerContext) {
return context.config.create<TypeOf<typeof EndpointConfigSchema>>();
}
8 changes: 4 additions & 4 deletions x-pack/plugins/endpoint/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,23 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { schema } from '@kbn/config-schema';
import { PluginInitializer } from 'src/core/server';
import { PluginInitializer, PluginInitializerContext } from 'src/core/server';
import {
EndpointPlugin,
EndpointPluginStart,
EndpointPluginSetup,
EndpointPluginStartDependencies,
EndpointPluginSetupDependencies,
} from './plugin';
import { EndpointConfigSchema } from './config';

export const config = {
schema: schema.object({ enabled: schema.boolean({ defaultValue: false }) }),
schema: EndpointConfigSchema,
Copy link
Contributor

Choose a reason for hiding this comment

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

good idea putting this into a separate file

};

export const plugin: PluginInitializer<
EndpointPluginSetup,
EndpointPluginStart,
EndpointPluginSetupDependencies,
EndpointPluginStartDependencies
> = () => new EndpointPlugin();
> = (initializerContext: PluginInitializerContext) => new EndpointPlugin(initializerContext);
39 changes: 39 additions & 0 deletions x-pack/plugins/endpoint/server/plugin.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* 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.
*/
import { CoreSetup } from 'kibana/server';
import { EndpointPlugin, EndpointPluginSetupDependencies } from './plugin';
import { coreMock } from '../../../../src/core/server/mocks';
import { PluginSetupContract } from '../../features/server';

describe('test endpoint plugin', () => {
let plugin: EndpointPlugin;
let mockCoreSetup: MockedKeys<CoreSetup>;
let mockedEndpointPluginSetupDependencies: jest.Mocked<EndpointPluginSetupDependencies>;
let mockedPluginSetupContract: jest.Mocked<PluginSetupContract>;
beforeEach(() => {
plugin = new EndpointPlugin(
coreMock.createPluginInitializerContext({
cookieName: 'sid',
sessionTimeout: 1500,
})
);

mockCoreSetup = coreMock.createSetup();
mockedPluginSetupContract = {
registerFeature: jest.fn(),
getFeatures: jest.fn(),
getFeaturesUICapabilities: jest.fn(),
registerLegacyAPI: jest.fn(),
};
mockedEndpointPluginSetupDependencies = { features: mockedPluginSetupContract };
});

it('test properly setup plugin', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of this test is that it should check that a feature has been registered and that routes have been registered. If these behaviors were tested via API integration tests, we could test the behavior instead of checking that certain methods were called. The tests wouldn't be as coupled to the implementation details. It also wouldn't requiring writing mocks that are specific to the new platform implementation (which has been changing very frequently recently.)

How would you feel about writing an API integration test that calls one of our HTTP APIs. You could also write a test that creates a user with a certain role and then with that user checks that feature registration is working correctly by making API requests (or by asserting that elements are present in the UI.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

requiring writing mocks that are specific to the new platform implementation (which has been changing very frequently recently.)

This is actually a reason to write such test, to make it clear that the dependencies in the code are covered at a minimum. You want to detect when you code is affected by other code changes. I will add integration test.

await plugin.setup(mockCoreSetup, mockedEndpointPluginSetupDependencies);
expect(mockedPluginSetupContract.registerFeature).toBeCalledTimes(1);
expect(mockCoreSetup.http.createRouter).toBeCalledTimes(1);
});
});
27 changes: 24 additions & 3 deletions x-pack/plugins/endpoint/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { Plugin, CoreSetup } from 'kibana/server';
import { Plugin, CoreSetup, PluginInitializerContext, Logger } from 'kibana/server';
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';

export type EndpointPluginStart = void;
export type EndpointPluginSetup = void;
Expand All @@ -23,6 +27,10 @@ export class EndpointPlugin
EndpointPluginSetupDependencies,
EndpointPluginStartDependencies
> {
private readonly logger: Logger;
constructor(private readonly initializerContext: PluginInitializerContext) {
this.logger = this.initializerContext.logger.get('endpoint');
}
public setup(core: CoreSetup, plugins: EndpointPluginSetupDependencies) {
plugins.features.registerFeature({
id: 'endpoint',
Expand All @@ -49,10 +57,23 @@ export class EndpointPlugin
},
},
});
const endpointContext = {
logFactory: this.initializerContext.logger,
config: (): Promise<EndpointConfigType> => {
return createConfig$(this.initializerContext)
.pipe(first())
.toPromise();
},
} as EndpointAppContext;
const router = core.http.createRouter();
addRoutes(router);
registerEndpointRoutes(router, endpointContext);
}

public start() {}
public stop() {}
public start() {
this.logger.debug('Starting plugin');
}
public stop() {
this.logger.debug('Stopping plugin');
}
}
123 changes: 123 additions & 0 deletions x-pack/plugins/endpoint/server/routes/endpoints.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this test be better as an API integration test? It could load an archvie (via es_archiver) and instrument the route handler and ES. This would check that the routing works and that the query works. Also it wouldn't require writing any mocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this test be better as an API integration test? It could load an archvie (via es_archiver) and instrument the route handler and ES. This would check that the routing works and that the query works. Also it wouldn't require writing any mocking.

We still need a unit test. We have an api test too but I did not want to add it to this PR to blow it up. I can add it to another PR. But I can add it if you insist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why you still need a unit tests? My understanding is that this test mocks various internals and asserts that they were called. My concern is that this type of test doesn't check the behavior. I'm also concerned that since its completely coupled to internals, it might break even if the behavior is correct.

Copy link
Contributor Author

@nnamdifrankie nnamdifrankie Jan 2, 2020

Choose a reason for hiding this comment

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

Can you explain why you still need a unit tests? My understanding is that this test mocks various internals and asserts that they were called

This is what unit tests do, they test internals and expected calls on dependencies with stubs. We cannot defer all tests after the entire system is coupled together and started, without covering the units.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the integration test. I agree that mocking can cause tight coupling and personally regret the places I did it in the SMP. I can see the value in stubbing out ES so you can test the translation from the search results of endpoints into the returned format done by this function mapToEndpointResultList. My suggestion (only a suggestion) would be to leave out the expects for how a function is called and how many times it is called. Like these lines:
expect(mockScopedClient.callAsCurrentUser).toBeCalledWith('search', {
const endpointResultList = mockResponse.ok.mock.calls[0][0]?.body as EndpointResultList;

and instead just validate that the results get transformed into the expected format like you're already doing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on @jonathan-buttner's comment, I'm wondering if this could be replaced by one (or more) unit tests against a pure function called mapToEndpointResultList. This function could be responsible for any intricate business logic and could be tested without stubs/mocking.

A second set of API integration tests could instrument the vertical slice but with much less coverage. This approach might allow your tests to be less coupled to the implementation while also having no mocking/stubs.

I'm also arguing that unit tests don't necessarily require mocking. Here is the PR I'm working on: https://github.com/elastic/kibana/pull/53619/files

Here are my unit test files. These do not use mocks/stubs and they also test the behavior of a small unit:

My integration / functional tests will instrument all of the 'plumbing' that is responsible for setting up react, redux, etc. Those tests will have less coverage (they will no instrument all the branches instrumented by the unit tests.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth creating an integration test to make sure the ES query works against real data in ES? I believe these tests just mock ES out correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be worth creating an integration test to make sure the ES query works against real data in ES? I believe these tests just mock ES out correct?

I will add the integration test, but it is not going to replace the Unit test. They are both needed.

* 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.
*/
import {
IClusterClient,
IRouter,
IScopedClusterClient,
KibanaResponseFactory,
RequestHandler,
RequestHandlerContext,
RouteConfig,
} from 'kibana/server';
import {
elasticsearchServiceMock,
httpServerMock,
httpServiceMock,
loggingServiceMock,
} from '../../../../../src/core/server/mocks';
import { EndpointData } from '../types';
import { SearchResponse } from 'elasticsearch';
import { EndpointResultList, registerEndpointRoutes } from './endpoints';
import { EndpointConfigSchema } from '../config';
import * as data from '../test_data/all_endpoints_data.json';

describe('test endpoint route', () => {
let routerMock: jest.Mocked<IRouter>;
let mockResponse: jest.Mocked<KibanaResponseFactory>;
let mockClusterClient: jest.Mocked<IClusterClient>;
let mockScopedClient: jest.Mocked<IScopedClusterClient>;
let routeHandler: RequestHandler<any, any, any>;
let routeConfig: RouteConfig<any, any, any, any>;

beforeEach(() => {
mockClusterClient = elasticsearchServiceMock.createClusterClient() as jest.Mocked<
IClusterClient
>;
mockScopedClient = elasticsearchServiceMock.createScopedClusterClient();
mockClusterClient.asScoped.mockReturnValue(mockScopedClient);
routerMock = httpServiceMock.createRouter();
mockResponse = httpServerMock.createResponseFactory();
registerEndpointRoutes(routerMock, {
logFactory: loggingServiceMock.create(),
config: () => Promise.resolve(EndpointConfigSchema.validate({})),
});
});

it('test find the latest of all endpoints', async () => {
const mockRequest = httpServerMock.createKibanaRequest({});

const response: SearchResponse<EndpointData> = (data as unknown) as SearchResponse<
EndpointData
>;
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response));
[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(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(3);
expect(endpointResultList.total).toEqual(3);
expect(endpointResultList.request_index).toEqual(0);
expect(endpointResultList.request_page_size).toEqual(10);
});

it('test find the latest of all endpoints with params', async () => {
Copy link

Choose a reason for hiding this comment

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

Should we add a test with negative numbers/floats? Or does the schema enforce this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we add a test with negative numbers/floats? Or does the schema enforce this?

We can enforce it in the schema.

Choose a reason for hiding this comment

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

@nnamdifrankie are we enforcing it or are you saying we could?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const mockRequest = httpServerMock.createKibanaRequest({
body: {
paging_properties: [
{
page_size: 10,
},
{
page_index: 1,
},
],
},
});
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() =>
Promise.resolve((data as unknown) as SearchResponse<EndpointData>)
);
[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(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(3);
expect(endpointResultList.total).toEqual(3);
expect(endpointResultList.request_index).toEqual(10);
expect(endpointResultList.request_page_size).toEqual(10);
});
});
87 changes: 87 additions & 0 deletions x-pack/plugins/endpoint/server/routes/endpoints.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* 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.
*/

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';

interface HitSource {
_source: EndpointData;
}

export interface EndpointResultList {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I'm late - was getting x-schooled last week.

Should we move this to server/types.ts? Would be good for the FE to use it as well when typing up the API 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.

I will consider this and probably put a small PR out.

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, could this match the name of the paging input param? (request_page_index)

}

export function registerEndpointRoutes(router: IRouter, endpointAppContext: EndpointAppContext) {
router.post(
{
path: '/api/endpoint/endpoints',
validate: {
body: schema.nullable(
schema.object({
paging_properties: schema.arrayOf(
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering why the paging_properties is an Array<Object>. Would it be simpler to define it as an object with shorter options names:

{
    paging: {
        size: number,
        page: 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.

I made the change because for some reason the order of property mattered. So size will always have to come before page, else it would be ignored. I also wanted validation from the schema too. I gave it a shot and this was the compromise.

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 .
That seems weird - is that perhaps an issue with the new platform that we're using? If you think it is, maybe report it to the Kibana platform team (slack channel #kibana-platform )

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 will.

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 }) }),
Copy link
Contributor

Choose a reason for hiding this comment

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

if I read the code correctly, page_index is zero-based. For simplicity, should it be 1-based? (which mean it would be a pass-through to the elasticsearch from option)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the from field is 0 based, so your first page is 0 to page size.

])
),
})
),
},
options: { authRequired: true },
},
async (context, req, res) => {
Copy link

Choose a reason for hiding this comment

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

Question/DIscussion: What are the pros for using an anonymous function like this? I definitely prefer having functions named for readability purposes.

This isn't too bad because you're passing in router and only one route is being added at once, but when there are multiple routes being added it's quite hard to read IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't too bad because you're passing in router and only one route is being added at once, but when there are multiple routes being added it's quite hard to read IMHO.

This handler function is only related to this route so there is the documentation. In this case I think the name may be redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: personally, I also like the naming on the function. One drawback of anonymous functions is debugging where you just see "anonymous" in stack traces or when doing heap/cpu profiling (but perhaps we don't do much of that, so its ok)

try {
const queryParams = await kibanaRequestToEndpointListQuery(req, endpointAppContext);
const response = (await context.core.elasticsearch.dataClient.callAsCurrentUser(
'search',
queryParams
)) as SearchResponse<EndpointData>;
return res.ok({ body: mapToEndpointResultList(queryParams, response) });
} catch (err) {
return res.internalError({ body: err });
}
}
);
}

function mapToEndpointResultList(
queryParams: Record<string, any>,
searchResponse: SearchResponse<EndpointData>
): EndpointResultList {
if (searchResponse.hits.hits.length > 0) {
return {
request_page_size: queryParams.size,
request_index: queryParams.from,
endpoints: searchResponse.hits.hits
.map(response => response.inner_hits.most_recent.hits.hits)
.flatMap(data => data as HitSource)
.map(entry => entry._source),
total: searchResponse.aggregations.total.value,
};
} else {
return {
request_page_size: queryParams.size,
request_index: queryParams.from,
total: 0,
endpoints: [],
};
}
}
Loading