[Security Solution] add new GET endpoint metadata list api #118968

joeypoon · 2021-11-17T23:24:25Z

Summary

Creates a new GET endpoint metadata list API. It returns the same data as the existing POST endpoint metadata list API but in a more standardized format. POST API removal and frontend change to use new API will be addressed in separate PRs.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

joeypoon · 2021-11-17T23:43:14Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/handlers.ts

@@ -161,6 +167,83 @@ export const getMetadataListRequestHandler = function (
  };
 };

+export function getMetadataListRequestHandlerV2(


will remove the V2 naming across the board when we get rid of the POST API.

most of the code is purposely duplicated so that the POST API and associated code can be easily removed in later PR.

joeypoon · 2021-11-17T23:46:21Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/metadata.test.ts

-      endpointAppContextService.start({ ...startContract, packageService: mockPackageService });
-      mockAgentService = startContract.agentService!;
-      mockAgentPolicyService = startContract.agentPolicyService!;
+  describe('POST list endpoints route', () => {


most of this is actually untouched, I just moved/nested some stuff. also duplicated POST API tests then modified them for the new GET API.

joeypoon · 2021-11-17T23:48:43Z

x-pack/test/security_solution_endpoint_api_int/apis/metadata.ts

-        expect(body.request_page_size).to.eql(10);
-        expect(body.request_page_index).to.eql(0);
-      });
+  describe('test metadata apis', () => {


same as other tests, mostly just nesting/moving tests around followed by duplicating POST API tests and modifying for the new GET API.

elasticmachine · 2021-11-18T00:46:36Z

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

kibana-ci · 2021-11-18T02:28:32Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #7536 failed f3960338804f04ac0f0d7996fc576d5d13df6d1d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

academo · 2021-11-18T16:11:26Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/handlers.ts

+    }
+
+    // If no unified Index present, then perform a search using the legacy approach
+    if (!doesUnitedIndexExist || didUnitedIndexError) {


nit: move this and the next one to another method,. e.g. getMetadataListRequestHandlerLegacy()

kibanamachine · 2021-11-18T20:14:14Z

💚 Backport successful

Status	Branch	Result
✅	8.0

This backport PR will be merged automatically after passing CI.

…18968)

…119099) Co-authored-by: Joey F. Poon <[email protected]>

paul-tavares

@joeypoon
Just taking a look at this - thank you for taking this on. I left several comments. Let me know if you have any questions.

paul-tavares · 2021-11-19T15:42:23Z

x-pack/plugins/security_solution/common/endpoint/types/index.ts

+/**
+ * Returned by the server via GET /api/endpoint/metadata
+ */
+export interface MetadataListResponse extends BaseListResponse {


You could hve also made BaseListResonse a generic, like:

export interface BaseListResponse<D = unknown> { data: D[]; page: number; pageSize: number; total: number; sort?: string; sortOrder?: 'asc' | 'desc'; }

Which would make it easier to defined more domain specific types:

export type MetadataListResponse = BaseListResponse<HostInfo>

paul-tavares · 2021-11-19T15:43:06Z

x-pack/plugins/security_solution/common/endpoint/types/index.ts

+  pageSize: number;
+  total: number;
+  sort?: string;
+  sortOrder?: 'asc' | 'desc';


❓ Q. Do we support sort/sort order on the metadata API currently? (or maybe this new API does?)

no to both :(. this was just preemptive.

cool. best to remove it for now (don't forget to adjust the schema

paul-tavares · 2021-11-19T15:45:29Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/handlers.ts

          pageSize: pagingProperties.pageSize,
-          filters: request.body?.filters || {},
+          hostStatuses: request.body?.filters.host_status || [],


oh crap - was this a bug I introduced during the refactor?

nah, I'm just flatting things here for the new route so had to change some of the methods.

paul-tavares · 2021-11-19T15:47:26Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/handlers.ts

        total,
-        request_page_size: pageSize,
+        request_page_index: pagingProperties.pageIndex * pagingProperties.pageSize,


is this correct?

request_page_index is suppose to be the page index that the user requested, correct? This does not look right to me.

the naming here is a bit confusing, this was actually the index of the document since it was being pulled directly from the esQuery.from. I changed everything to be index of the page so this is the last place where we're using index of doc (for endpoint list apis at least), which we can then delete together with the route.

😱 🙀 - So happy we are deleting this API.

paul-tavares · 2021-11-19T15:48:56Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/handlers.ts

+> {
+  return async (context, request, response) => {
+    const endpointMetadataService = endpointAppContext.service.getEndpointMetadataService();
+    if (!endpointMetadataService) {


is this needed?
I would move it to the service.getEndpointMetadataService() instead if you see that it is actually needed

this is needed here but I hesitate to move the exception up since isn't there a possibility that this might not be a required service for a consumer?

I don't think so. I think that EndpointAppContext is always initialized "fully" so this service will (?should?) be available always

misunderstood you here. will just remove the null check.

paul-tavares · 2021-11-19T15:50:57Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/handlers.ts

+      body = {
+        data,
+        total,
+        page: request.query.page,


you could just use the return value from getHostMetadataList() method right (I think it has the same signature as far as the data structure.

I changed to this because I removed page and pageSize from getHostMetadataList since it just returns those directly from the received params anyways.

paul-tavares · 2021-11-19T15:51:42Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/handlers.ts

  endpointAppContext: EndpointAppContext,
  logger: Logger,
-  endpointPolicies: PackagePolicy[]
+  endpointPolicies: PackagePolicy[],
+  queryOptions: TypeOf<typeof GetMetadataListRequestSchemaV2.query>


We probably should create a type for this in security_solution/common/ because the UI will need it as well.

paul-tavares · 2021-11-19T15:55:50Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/index.ts

@@ -60,27 +65,54 @@ export const GetMetadataListRequestSchema = {
  ),
 };

+export const GetMetadataListRequestSchemaV2 = {
+  query: schema.object({


can we make all of this optional? A consumer of the list api should not be required to define all of this. It should be optional and schema assign defaults.

Also - I suggest you move it to security_solution/common/endpoint/schema, so that the Types that we create from it can be re-used on the UI side. You should also add some tests against the schema (case we ever change to io-ts we have tests in place to ensure it still works as we expected it)

paul-tavares · 2021-11-19T15:57:14Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/query_builders.test.ts

-        service: new EndpointAppContextService(),
-        config: () => Promise.resolve(createMockConfig()),
-        experimentalFeatures: parseExperimentalConfigValue(createMockConfig().enableExperimental),
+      const query = await kibanaRequestToMetadataListESQuery({


If you make the query optional, please a test for it.

paul-tavares · 2021-11-19T15:59:51Z

x-pack/plugins/security_solution/server/endpoint/routes/metadata/support/agent_status.ts

@@ -36,7 +36,7 @@ export function buildStatusesKuery(statusesToFilter: string[]): string | undefin
 export async function findAgentIdsByStatus(
  agentService: AgentService,
  esClient: ElasticsearchClient,
-  statuses: string[],
+  statuses: string[] = [],


why make it optional? the method's purpose is to get agents by statuses, so the consumer of this method must define them on input.

(I see below there is not much harm, as it exits if it finds an empty array, but still I think we should not be invoking this method without statuses)

…18968)

joeypoon added v8.0.0 Team:Defend Workflows “EDR Workflows” sub-team of Security Solution auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Nov 17, 2021

joeypoon requested a review from a team November 17, 2021 23:24

joeypoon commented Nov 17, 2021

View reviewed changes

[Security Solution] add new GET endpoint metdata list api

4096bca

joeypoon force-pushed the feature/new-metadata-list-api branch from f396033 to 4096bca Compare November 18, 2021 00:46

joeypoon marked this pull request as ready for review November 18, 2021 00:46

joeypoon requested a review from a team as a code owner November 18, 2021 00:46

joeypoon requested review from academo and dasansol92 November 18, 2021 00:46

joeypoon added the release_note:enhancement label Nov 18, 2021

academo reviewed Nov 18, 2021

View reviewed changes

academo approved these changes Nov 18, 2021

View reviewed changes

joeypoon changed the title ~~[Security Solution] add new GET endpoint metdata list api~~ [Security Solution] add new GET endpoint metadata list api Nov 18, 2021

joeypoon merged commit 293bde0 into elastic:main Nov 18, 2021

joeypoon deleted the feature/new-metadata-list-api branch November 18, 2021 20:11

kibanamachine mentioned this pull request Nov 18, 2021

[8.0] [Security Solution] add new GET endpoint metadata list api (#118968) #119099

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 18, 2021

[Security Solution] add new GET endpoint metadata list api (elastic#1…

6391b71

…18968)

kibanamachine added a commit that referenced this pull request Nov 18, 2021

[Security Solution] add new GET endpoint metadata list api (#118968) (#…

76f073f

…119099) Co-authored-by: Joey F. Poon <[email protected]>

joeypoon mentioned this pull request Nov 18, 2021

[Security Solution] migrate to new GET metadata list API #119123

Merged

2 tasks

paul-tavares reviewed Nov 19, 2021

View reviewed changes

TinLe pushed a commit to TinLe/kibana that referenced this pull request Nov 20, 2021

[Security Solution] add new GET endpoint metadata list api (elastic#1…

4f18a18

…18968)

dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021

[Security Solution] add new GET endpoint metadata list api (#118968)

7ca3749

EricDavisX mentioned this pull request Nov 29, 2021

[Fleet] update Endpoint metadata api from POST to GET per breaking change from Security OLM team elastic/e2e-testing#1847

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] add new GET endpoint metadata list api #118968

[Security Solution] add new GET endpoint metadata list api #118968

joeypoon commented Nov 17, 2021

joeypoon Nov 17, 2021 •

edited

Loading

joeypoon Nov 17, 2021 •

edited

Loading

joeypoon Nov 17, 2021

joeypoon Nov 17, 2021

elasticmachine commented Nov 18, 2021

kibana-ci commented Nov 18, 2021

academo Nov 18, 2021

kibanamachine commented Nov 18, 2021

paul-tavares left a comment

paul-tavares Nov 19, 2021

paul-tavares Nov 19, 2021

joeypoon Nov 19, 2021

paul-tavares Nov 19, 2021

paul-tavares Nov 19, 2021

joeypoon Nov 19, 2021

paul-tavares Nov 19, 2021

joeypoon Nov 19, 2021 •

edited

Loading

paul-tavares Nov 19, 2021

paul-tavares Nov 19, 2021

joeypoon Nov 19, 2021

paul-tavares Nov 19, 2021

joeypoon Nov 20, 2021

paul-tavares Nov 19, 2021

joeypoon Nov 20, 2021

paul-tavares Nov 19, 2021

paul-tavares Nov 19, 2021

paul-tavares Nov 19, 2021

paul-tavares Nov 19, 2021

[Security Solution] add new GET endpoint metadata list api #118968

[Security Solution] add new GET endpoint metadata list api #118968

Conversation

joeypoon commented Nov 17, 2021

Summary

Checklist

For maintainers

joeypoon Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

joeypoon Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 18, 2021

kibana-ci commented Nov 18, 2021

💚 Build Succeeded

Metrics [docs]

History

Choose a reason for hiding this comment

kibanamachine commented Nov 18, 2021

💚 Backport successful

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeypoon Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeypoon Nov 17, 2021 •

edited

Loading

joeypoon Nov 17, 2021 •

edited

Loading

joeypoon Nov 19, 2021 •

edited

Loading