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

EMT-146: use ingest agent for status info #63921

Merged
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ad9dad3
EMT-146: use ingest agent for status info
nnamdifrankie Apr 18, 2020
c653765
EMT-146: add integration tests
nnamdifrankie Apr 18, 2020
eb13637
Merge branch 'master' into EMT-146_use_agent_service_for_status
elasticmachine Apr 18, 2020
110efc6
EMT-146: use beforeEach and afterEach
nnamdifrankie Apr 18, 2020
e29c087
Merge branch 'EMT-146_use_agent_service_for_status' of github.com:nna…
nnamdifrankie Apr 18, 2020
4e53a4f
EMT-146: remove failing test
nnamdifrankie Apr 18, 2020
ba532d0
EMT-146: add back integration test
nnamdifrankie Apr 18, 2020
179d09e
EMT-146: revert ingest calls
nnamdifrankie Apr 19, 2020
229df97
EMT-146: clean up
nnamdifrankie Apr 19, 2020
578e0bf
Merge branch 'master' into EMT-146_use_agent_service_for_status
elasticmachine Apr 20, 2020
bcd6e4c
EMT-146: reorder test load
nnamdifrankie Apr 20, 2020
b36c29f
Merge branch 'EMT-146_use_agent_service_for_status' of github.com:nna…
nnamdifrankie Apr 20, 2020
f591e86
EMT-146: add ingest calls back
nnamdifrankie Apr 20, 2020
62eba60
EMT-146: remove metadata status test, it cross some boundaries
nnamdifrankie Apr 20, 2020
e990c47
EMT-146: review comments and refactor the code
nnamdifrankie Apr 20, 2020
2bd6601
EMT-146: add more documentation and test
nnamdifrankie Apr 21, 2020
6d4c1ed
EMT-146: fall back to host id if elastic id is missing
nnamdifrankie Apr 21, 2020
9bb3550
EMT-146: add warning log, and improve logging
nnamdifrankie Apr 21, 2020
f7d0bad
Merge branch 'master' into EMT-146_use_agent_service_for_status
elasticmachine Apr 21, 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
17 changes: 16 additions & 1 deletion x-pack/plugins/endpoint/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { IngestManagerSetupContract } from '../../ingest_manager/server';
import { AgentService } from '../../ingest_manager/common/types';

/**
* Creates a mock IndexPatternRetriever for use in tests.
*
Expand All @@ -28,17 +31,29 @@ export const createMockMetadataIndexPatternRetriever = () => {
return createMockIndexPatternRetriever(MetadataIndexPattern);
};

/**
* Creates a mock AgentService
*/
export const createMockAgentService = (): jest.Mocked<AgentService> => {
return {
getAgentStatus: jest.fn(),
};
};

/**
* Creates a mock IndexPatternService for use in tests that need to interact with the Ingest Manager's
* ESIndexPatternService.
*
* @param indexPattern a string index pattern to return when called by a test
* @returns the same value as `indexPattern` parameter
*/
export const createMockIndexPatternService = (indexPattern: string) => {
export const createMockIngestManagerSetupContract = (
indexPattern: string
): IngestManagerSetupContract => {
return {
esIndexPatternService: {
getESIndexPattern: jest.fn().mockResolvedValue(indexPattern),
},
agentService: createMockAgentService(),
};
};
4 changes: 2 additions & 2 deletions x-pack/plugins/endpoint/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { EndpointPlugin, EndpointPluginSetupDependencies } from './plugin';
import { coreMock } from '../../../../src/core/server/mocks';
import { PluginSetupContract } from '../../features/server';
import { createMockIndexPatternService } from './mocks';
import { createMockIngestManagerSetupContract } from './mocks';

describe('test endpoint plugin', () => {
let plugin: EndpointPlugin;
Expand All @@ -31,7 +31,7 @@ describe('test endpoint plugin', () => {
};
mockedEndpointPluginSetupDependencies = {
features: mockedPluginSetupContract,
ingestManager: createMockIndexPatternService(''),
ingestManager: createMockIngestManagerSetupContract(''),
};
});

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/endpoint/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export class EndpointPlugin
plugins.ingestManager.esIndexPatternService,
this.initializerContext.logger
),
agentService: plugins.ingestManager.agentService,
logFactory: this.initializerContext.logger,
config: (): Promise<EndpointConfigType> => {
return createConfig$(this.initializerContext)
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/endpoint/server/routes/alerts/alerts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
import { registerAlertRoutes } from './index';
import { EndpointConfigSchema } from '../../config';
import { alertingIndexGetQuerySchema } from '../../../common/schema/alert_index';
import { createMockIndexPatternRetriever } from '../../mocks';
import { createMockAgentService, createMockIndexPatternRetriever } from '../../mocks';

describe('test alerts route', () => {
let routerMock: jest.Mocked<IRouter>;
Expand All @@ -26,6 +26,7 @@ describe('test alerts route', () => {
routerMock = httpServiceMock.createRouter();
registerAlertRoutes(routerMock, {
indexPatternRetriever: createMockIndexPatternRetriever('events-endpoint-*'),
agentService: createMockAgentService(),
logFactory: loggingServiceMock.create(),
config: () => Promise.resolve(EndpointConfigSchema.validate({})),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ export const alertDetailsHandlerWrapper = function(
indexPattern
);

const currentHostInfo = await getHostData(ctx, response._source.host.id, indexPattern);
const currentHostInfo = await getHostData(
{
endpointAppContext,
requestHandlerContext: ctx,
},
response._source.host.id
);

return res.ok({
body: {
Expand Down
88 changes: 67 additions & 21 deletions x-pack/plugins/endpoint/server/routes/metadata/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,29 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { IRouter, RequestHandlerContext } from 'kibana/server';
import { IRouter, Logger, RequestHandlerContext } from 'kibana/server';
import { SearchResponse } from 'elasticsearch';
import { schema } from '@kbn/config-schema';

import { kibanaRequestToMetadataListESQuery, getESQueryHostMetadataByID } from './query_builders';
import { getESQueryHostMetadataByID, kibanaRequestToMetadataListESQuery } from './query_builders';
import { HostInfo, HostMetadata, HostResultList, HostStatus } from '../../../common/types';
import { EndpointAppContext } from '../../types';
import { AgentStatus } from '../../../../ingest_manager/common/types/models';

interface HitSource {
_source: HostMetadata;
}

interface MetadataRequestContext {
requestHandlerContext: RequestHandlerContext;
endpointAppContext: EndpointAppContext;
}

const HOST_STATUS_MAPPING = new Map<AgentStatus, HostStatus>([
['online', HostStatus.ONLINE],
['offline', HostStatus.OFFLINE],
]);

export function registerEndpointRoutes(router: IRouter, endpointAppContext: EndpointAppContext) {
router.post(
{
Expand Down Expand Up @@ -62,7 +73,12 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp
'search',
queryParams
)) as SearchResponse<HostMetadata>;
return res.ok({ body: mapToHostResultList(queryParams, response) });
return res.ok({
body: await mapToHostResultList(queryParams, response, {
endpointAppContext,
requestHandlerContext: context,
}),
});
} catch (err) {
return res.internalError({ body: err });
}
Expand All @@ -79,11 +95,13 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp
},
async (context, req, res) => {
try {
const index = await endpointAppContext.indexPatternRetriever.getMetadataIndexPattern(
context
const doc = await getHostData(
{
endpointAppContext,
requestHandlerContext: context,
},
req.params.id
);

const doc = await getHostData(context, req.params.id, index);
if (doc) {
return res.ok({ body: doc });
}
Expand All @@ -96,12 +114,14 @@ export function registerEndpointRoutes(router: IRouter, endpointAppContext: Endp
}

export async function getHostData(
context: RequestHandlerContext,
id: string,
index: string
metadataRequestContext: MetadataRequestContext,
id: string
): Promise<HostInfo | undefined> {
const index = await metadataRequestContext.endpointAppContext.indexPatternRetriever.getMetadataIndexPattern(
metadataRequestContext.requestHandlerContext
);
const query = getESQueryHostMetadataByID(id, index);
const response = (await context.core.elasticsearch.dataClient.callAsCurrentUser(
const response = (await metadataRequestContext.requestHandlerContext.core.elasticsearch.dataClient.callAsCurrentUser(
'search',
query
)) as SearchResponse<HostMetadata>;
Expand All @@ -110,22 +130,25 @@ export async function getHostData(
return undefined;
}

return enrichHostMetadata(response.hits.hits[0]._source);
return await enrichHostMetadata(response.hits.hits[0]._source, metadataRequestContext);
}

function mapToHostResultList(
async function mapToHostResultList(
queryParams: Record<string, any>,
searchResponse: SearchResponse<HostMetadata>
): HostResultList {
searchResponse: SearchResponse<HostMetadata>,
metadataRequestContext: MetadataRequestContext
): Promise<HostResultList> {
const totalNumberOfHosts = searchResponse?.aggregations?.total?.value || 0;
if (searchResponse.hits.hits.length > 0) {
return {
request_page_size: queryParams.size,
request_page_index: queryParams.from,
hosts: searchResponse.hits.hits
.map(response => response.inner_hits.most_recent.hits.hits)
.flatMap(data => data as HitSource)
.map(entry => enrichHostMetadata(entry._source)),
hosts: await Promise.all(
searchResponse.hits.hits
.map(response => response.inner_hits.most_recent.hits.hits)
.flatMap(data => data as HitSource)
.map(async entry => enrichHostMetadata(entry._source, metadataRequestContext))
),
total: totalNumberOfHosts,
};
} else {
Expand All @@ -138,9 +161,32 @@ function mapToHostResultList(
}
}

function enrichHostMetadata(hostMetadata: HostMetadata): HostInfo {
async function enrichHostMetadata(
hostMetadata: HostMetadata,
metadataRequestContext: MetadataRequestContext
): Promise<HostInfo> {
let hostStatus = HostStatus.ERROR;
try {
const status = await metadataRequestContext.endpointAppContext.agentService.getAgentStatus(
metadataRequestContext.requestHandlerContext,
hostMetadata.elastic.agent.id
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const status = await metadataRequestContext.endpointAppContext.agentService.getAgentStatus(
metadataRequestContext.requestHandlerContext,
hostMetadata.elastic.agent.id
);
const { endpointAppContext, requestHandlerContext } = metadataRequestContext;
const { getAgent, getAgentStatus } = endpointAppContext.agentService;
const agent = await getAgent(requestHandlerContext.core.savedObjects.client, hostMetadata.elastic.agent.id);
const status = await getAgentStatus(agent);

Copy link
Contributor

Choose a reason for hiding this comment

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

I won't be able to try this locally until Monday afternoon, but I think this will work. If so, we can avoid the new getAgentStatus this PR adds in Ingest Manager.

Copy link
Contributor Author

@nnamdifrankie nnamdifrankie Apr 20, 2020

Choose a reason for hiding this comment

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

@jfsiii Can we talk about this. I would think a wholesale service will be better that performs the functionality will be better. And this does not isolate from changes to how agent status is derived. With this approach we have too much knowledge of the internal working of Ingest. This will mean will do a wholesale export of the entire services directory?

Copy link
Contributor

@jfsiii jfsiii Apr 20, 2020

Choose a reason for hiding this comment

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

@nnamdifrankie sure, we can talk anytime. I'm available most days from 1pm-5pm ET.

Before then, I'll try to summary my understanding & suggestions.

As I understand it, this PR adds an Agent Service but Ingest Manager already has one

It also adds a function getAgentStatus which a) is a duplicate of an existing function and b) uses the existing services functions from Ingest.

I'd like to avoid both those issues and think we can do that by moving that function into services/agents/status, changing its name to something like getAgentStatusById, and have it accept a saved object client and string id. That's in line with lots of our other service functions.

That way, all you're really changing at this call site is the function name and the first argument. It's still only calling one function and Ingest is the one in control of that logic.

-    const status = await metadataRequestContext.endpointAppContext.agentService.getAgentStatus(
-      metadataRequestContext.requestHandlerContext,

+    const status = await metadataRequestContext.endpointAppContext.agentService.getAgentStatusById(
+      metadataRequestContext.requestHandlerContext.core.savedObjects.client,

Hope that helps. Reach out any time to talk more.

hostStatus = HOST_STATUS_MAPPING.get(status) || HostStatus.ERROR;
} catch (e) {
if (e.isBoom && e.output.statusCode === 404) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the ingest api throw a boom?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if a saved object is not found is going throw a Boom error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes,
server log [10:02:10.301] [warning][endpoint][metadata][plugins] agent with id 023fa40c-411d-4188-a941-4147bfadd095 not found

logger(metadataRequestContext.endpointAppContext).warn(
`agent with id ${hostMetadata.elastic.agent.id} not found`
);
} else {
throw e;
}
}
return {
metadata: hostMetadata,
host_status: HostStatus.ERROR,
host_status: hostStatus,
};
}

const logger = (endpointAppContext: EndpointAppContext): Logger => {
return endpointAppContext.logFactory.get('metadata');
};
72 changes: 68 additions & 4 deletions x-pack/plugins/endpoint/server/routes/metadata/metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ import { SearchResponse } from 'elasticsearch';
import { registerEndpointRoutes } from './index';
import { EndpointConfigSchema } from '../../config';
import * as data from '../../test_data/all_metadata_data.json';
import { createMockMetadataIndexPatternRetriever } from '../../mocks';
import { createMockAgentService, createMockMetadataIndexPatternRetriever } from '../../mocks';
import { AgentService } from '../../../../ingest_manager/common/types';
import Boom from 'boom';

describe('test endpoint route', () => {
let routerMock: jest.Mocked<IRouter>;
Expand All @@ -35,6 +37,7 @@ describe('test endpoint route', () => {
let mockSavedObjectClient: jest.Mocked<SavedObjectsClientContract>;
let routeHandler: RequestHandler<any, any, any>;
let routeConfig: RouteConfig<any, any, any, any>;
let mockAgentService: jest.Mocked<AgentService>;

beforeEach(() => {
mockClusterClient = elasticsearchServiceMock.createClusterClient() as jest.Mocked<
Expand All @@ -45,8 +48,10 @@ describe('test endpoint route', () => {
mockClusterClient.asScoped.mockReturnValue(mockScopedClient);
routerMock = httpServiceMock.createRouter();
mockResponse = httpServerMock.createResponseFactory();
mockAgentService = createMockAgentService();
registerEndpointRoutes(routerMock, {
indexPatternRetriever: createMockMetadataIndexPatternRetriever(),
agentService: mockAgentService,
logFactory: loggingServiceMock.create(),
config: () => Promise.resolve(EndpointConfigSchema.validate({})),
});
Expand Down Expand Up @@ -83,7 +88,7 @@ describe('test endpoint route', () => {
[routeConfig, routeHandler] = routerMock.post.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;

mockAgentService.getAgentStatus = jest.fn().mockReturnValue('error');
await routeHandler(
createRouteHandlerContext(mockScopedClient, mockSavedObjectClient),
mockRequest,
Expand Down Expand Up @@ -113,6 +118,8 @@ describe('test endpoint route', () => {
],
},
});

mockAgentService.getAgentStatus = jest.fn().mockReturnValue('error');
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() =>
Promise.resolve((data as unknown) as SearchResponse<HostMetadata>)
);
Expand Down Expand Up @@ -154,6 +161,8 @@ describe('test endpoint route', () => {
filter: 'not host.ip:10.140.73.246',
},
});

mockAgentService.getAgentStatus = jest.fn().mockReturnValue('error');
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() =>
Promise.resolve((data as unknown) as SearchResponse<HostMetadata>)
);
Expand Down Expand Up @@ -216,10 +225,10 @@ describe('test endpoint route', () => {
},
})
);
mockAgentService.getAgentStatus = jest.fn().mockReturnValue('error');
[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;

await routeHandler(
createRouteHandlerContext(mockScopedClient, mockSavedObjectClient),
mockRequest,
Expand All @@ -233,13 +242,14 @@ describe('test endpoint route', () => {
expect(message).toEqual('Endpoint Not Found');
});

it('should return a single endpoint with status error', async () => {
it('should return a single endpoint with status online', async () => {
const mockRequest = httpServerMock.createKibanaRequest({
params: { id: (data as any).hits.hits[0]._id },
});
const response: SearchResponse<HostMetadata> = (data as unknown) as SearchResponse<
HostMetadata
>;
mockAgentService.getAgentStatus = jest.fn().mockReturnValue('online');
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response));
[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
Expand All @@ -256,6 +266,60 @@ describe('test endpoint route', () => {
expect(mockResponse.ok).toBeCalled();
const result = mockResponse.ok.mock.calls[0][0]?.body as HostInfo;
expect(result).toHaveProperty('metadata.endpoint');
expect(result.host_status).toEqual(HostStatus.ONLINE);
});

it('should return a single endpoint with status error when AgentService throw 404', async () => {
const mockRequest = httpServerMock.createKibanaRequest({
params: { id: (data as any).hits.hits[0]._id },
});
const response: SearchResponse<HostMetadata> = (data as unknown) as SearchResponse<
HostMetadata
>;
mockAgentService.getAgentStatus = jest.fn().mockImplementation(() => {
throw Boom.notFound('Agent not found');
});
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response));
[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;

await routeHandler(
createRouteHandlerContext(mockScopedClient, mockSavedObjectClient),
mockRequest,
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.ok).toBeCalled();
const result = mockResponse.ok.mock.calls[0][0]?.body as HostInfo;
expect(result.host_status).toEqual(HostStatus.ERROR);
});

it('should return a single endpoint with status error when status is not offline or online', async () => {
const mockRequest = httpServerMock.createKibanaRequest({
params: { id: (data as any).hits.hits[0]._id },
});
const response: SearchResponse<HostMetadata> = (data as unknown) as SearchResponse<
HostMetadata
>;
mockAgentService.getAgentStatus = jest.fn().mockReturnValue('warning');
mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => Promise.resolve(response));
[routeConfig, routeHandler] = routerMock.get.mock.calls.find(([{ path }]) =>
path.startsWith('/api/endpoint/metadata')
)!;

await routeHandler(
createRouteHandlerContext(mockScopedClient, mockSavedObjectClient),
mockRequest,
mockResponse
);

expect(mockScopedClient.callAsCurrentUser).toBeCalled();
expect(routeConfig.options).toEqual({ authRequired: true });
expect(mockResponse.ok).toBeCalled();
const result = mockResponse.ok.mock.calls[0][0]?.body as HostInfo;
expect(result.host_status).toEqual(HostStatus.ERROR);
});
});
Expand Down
Loading