-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 10 commits
08343b2
2e70a4c
7dfb50a
91bfbd3
4ecfbf2
6327a4f
cad4f76
2b01cf5
42e4f07
b6ca98f
06ba203
7718594
18aaa7b
13fe43a
0be7a08
b8ed0e8
ac81bae
0686689
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
}); | ||
}); |
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> | ||
? 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>>(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); |
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 () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,183 @@ | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 and instead just validate that the results get transformed into the expected format like you're already doing. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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({ | ||
body: {}, | ||
params: {}, | ||
}); | ||
|
||
const response: SearchResponse<EndpointData> = (data as unknown) as SearchResponse< | ||
EndpointData | ||
>; | ||
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response)); | ||
[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) => | ||
path.startsWith('/api/endpoint/endpoints') | ||
)!; | ||
|
||
await routeHandler( | ||
({ | ||
core: { | ||
elasticsearch: { | ||
dataClient: mockScopedClient, | ||
}, | ||
}, | ||
} as unknown) as RequestHandlerContext, | ||
mockRequest, | ||
mockResponse | ||
); | ||
|
||
expect(mockScopedClient.callAsCurrentUser).toBeCalledWith('search', { | ||
from: 0, | ||
size: 10, | ||
body: { | ||
query: { | ||
match_all: {}, | ||
}, | ||
collapse: { | ||
field: 'machine_id', | ||
inner_hits: { | ||
name: 'most_recent', | ||
size: 1, | ||
sort: [{ created_at: 'desc' }], | ||
}, | ||
}, | ||
aggs: { | ||
total: { | ||
cardinality: { | ||
field: 'machine_id', | ||
}, | ||
}, | ||
}, | ||
sort: [ | ||
{ | ||
created_at: { | ||
order: 'desc', | ||
}, | ||
}, | ||
], | ||
}, | ||
index: 'endpoint-agent*', | ||
}); | ||
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.requestIndex).toEqual(0); | ||
expect(endpointResultList.requestPageSize).toEqual(10); | ||
}); | ||
|
||
it('test find the latest of all endpoints with params', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can enforce it in the schema. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nnamdifrankie are we enforcing it or are you saying we could? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is enforced by the schema https://github.com/elastic/kibana/pull/53861/files#diff-fb44c643877621f0f4e2e866c3099c52R49 |
||
const mockRequest = httpServerMock.createKibanaRequest({ | ||
body: {}, | ||
query: { | ||
pageSize: 10, | ||
pageIndex: 1, | ||
}, | ||
}); | ||
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => | ||
Promise.resolve((data as unknown) as SearchResponse<EndpointData>) | ||
); | ||
[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) => | ||
path.startsWith('/api/endpoint/endpoints') | ||
)!; | ||
|
||
await routeHandler( | ||
({ | ||
core: { | ||
elasticsearch: { | ||
dataClient: mockScopedClient, | ||
}, | ||
}, | ||
} as unknown) as RequestHandlerContext, | ||
mockRequest, | ||
mockResponse | ||
); | ||
|
||
expect(mockScopedClient.callAsCurrentUser).toBeCalledWith('search', { | ||
from: 10, | ||
size: 10, | ||
body: { | ||
query: { | ||
match_all: {}, | ||
}, | ||
collapse: { | ||
field: 'machine_id', | ||
inner_hits: { | ||
name: 'most_recent', | ||
size: 1, | ||
sort: [{ created_at: 'desc' }], | ||
}, | ||
}, | ||
aggs: { | ||
total: { | ||
cardinality: { | ||
field: 'machine_id', | ||
}, | ||
}, | ||
}, | ||
sort: [ | ||
{ | ||
created_at: { | ||
order: 'desc', | ||
}, | ||
}, | ||
], | ||
}, | ||
index: 'endpoint-agent*', | ||
}); | ||
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.requestIndex).toEqual(10); | ||
expect(endpointResultList.requestPageSize).toEqual(10); | ||
}); | ||
}); |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed some other examples that are in master already, https://github.com/elastic/kibana/blob/master/x-pack/plugins/spaces/server/config.ts, https://github.com/elastic/kibana/blob/master/x-pack/plugins/security/server/config.ts