[Serverless] Add schema validation to Search Strategies in security solution & timelines #162539

lgestc · 2023-07-26T06:04:48Z

Summary

Specifies validation schemas for enpoints listed here:
https://github.com/elastic/security-team/issues/6486

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Add schemas
filterQuery schema is too liberal (?)
runtime fields type in model allows everything really

Fixing tests on CI

Adding this to kibana.dev.yml helps a lot in debugging.

uiSettings:
  overrides:
    'bfetch:disable': true

For maintainers

This was checked for breaking API changes and was labeled appropriately

x-pack/plugins/security_solution/common/api/search_strategy/model/request_basic_options.ts

michaelolo24 · 2023-09-12T18:39:17Z

x-pack/plugins/timelines/common/api/search_strategy/model/filter_query.ts

@@ -0,0 +1,83 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one


Going through some of these schema's, I feel like these should be available at the Kibana level, because I can see it being replicated across plugins. Not something for you to do, but worth adding a ticket to circle back and see if Kibana platform can package these

michaelolo24 · 2023-09-12T18:39:58Z

x-pack/plugins/timelines/common/api/search_strategy/model/language.ts

+
+import { z } from 'zod';
+
+export const language = z.union([z.literal('eql'), z.literal('kuery'), z.literal('lucene')]);


Awesome! Going to have to add esql to this soon as well. Same comment about kibana platform here

x-pack/plugins/timelines/common/api/search_strategy/model/runtime_mappings.ts

michaelolo24 · 2023-09-12T18:49:39Z

x-pack/plugins/timelines/server/search_strategy/timeline/factory/events/all/index.ts

-    const { activePage, querySize } = options.pagination;
+    const {
+      pagination: { activePage, querySize } = {
+        activePage: undefined,


why default this to undefined rather than 0?

michaelolo24

Nice work with this PR! Just had some questions around certain typing decisions, but it looks great and works great. Still a couple todo's but giving the approve and would love to chat about some of the type decisions I commented on.

marshallmain

Did a first pass, will review more later today

marshallmain · 2023-09-13T07:50:18Z

x-pack/plugins/security_solution/server/search_strategy/security_solution/index.ts

+      const parsedRequest = searchStrategyRequestSchema.parse(request);
+
+      const queryFactory = securitySolutionFactory[parsedRequest.factoryQueryType];

-      const queryFactory: SecuritySolutionFactory<T> =
-        securitySolutionFactory[request.factoryQueryType];
-      const dsl = queryFactory.buildDsl(request);
+      const dsl = queryFactory.buildDsl(parsedRequest);


I was digging in here to figure out how the compiler ensures that parsedRequest is passed into the correct implementation of buildDsl for a particular factoryQueryType. The short answer is it doesn't: in x-pack/plugins/security_solution/server/search_strategy/security_solution/factory/hosts/index.ts, for example, we have

export const hostsFactory: Record< HostsQueries | HostsKpiQueries, SecuritySolutionFactory<FactoryQueryTypes> > = { [HostsQueries.details]: hostDetails, [HostsQueries.hosts]: allHosts, [HostsQueries.overview]: hostOverview, [HostsQueries.uncommonProcesses]: uncommonProcesses, [HostsKpiQueries.kpiHosts]: hostsKpiHosts, [HostsKpiQueries.kpiUniqueIps]: hostsKpiUniqueIps, };

but the compiler does not throw any error if we assign hostDetails or any other value from this record to the wrong key. The problem here is that hostDetails should not actually be assignable to a key of type SecuritySolutionFactory<FactoryQueryTypes> because the types are incompatible. The type of buildDsl in SecuritySolutionFactory<FactoryQueryTypes> says it must accept any factoryQueryType, but the type SecuritySolutionFactory<HostsQueries.details> specifies a type for buildDsl that accepts only hostDetails for factoryQueryType. This should throw a type error but does not due to a bug in the compiler. The linked bug report also shows how this sort of error can lead to runtime exceptions.

In this file, the result of the bug is that the type system says we have always get an implementation of buildDsl that can accept any factoryQueryType from securitySolutionFactory, so it's no problem to pass in any parsedRequest to any queryFactory that we retrieve.

The problem is not introduced by this PR but we should definitely fix it either here or soon after in a follow up PR. A good start to fixing the issue would be replacing all the factory record types like

export const hostsFactory: Record< HostsQueries | HostsKpiQueries, SecuritySolutionFactory<FactoryQueryTypes> >

with something more like

export type FactoryRecordType = { [T in FactoryQueryTypes]: SecuritySolutionFactory<T>; }; export const hostsFactory: FactoryRecordType ....

This uses the compiler to enforce that the factory implementations are associated with the correct keys in the record. A number of additional changes will need to be made as well to fix corresponding types.

thank you for the heads up, I will try to fix it in scope of this PR even

marshallmain · 2023-09-13T07:59:17Z

x-pack/plugins/security_solution/common/api/search_strategy/model/request_basic_options.ts

+    filterQuery,
+    defaultIndex: z.array(z.string()).optional(),
+    id: z.string().optional(),
+    params: z.any().optional(),


z.any() here seems problematic - are we using this param in requests right now somewhere?

I will take a look, though something should be here for basic search request compat

marshallmain

Overall this looks great! This was a huge number of new schemas to add so thank you for the hard work on hardening these APIs. I left a couple more comments about improvements we can make, but I don't think they should block merging the huge step forward we have here already.

marshallmain · 2023-09-15T03:02:45Z

...ins/security_solution/common/search_strategy/security_solution/hosts/kpi/unique_ips/index.ts

 import type { HostsKpiHistogramData } from '../common';

-export type HostsKpiUniqueIpsRequestOptions = RequestBasicOptions;
-
 export interface HostsKpiUniqueIpsStrategyResponse extends IEsSearchResponse {


We should move these Response types to the common/api folder next to the associated Request type - doesn't have to be now, since this PR is large and has major improvements already, but soon. It's convenient to have both the request and response types together so the full contract for each API is contained in a single place. We don't have to implement runtime schemas for the response types though because the compiler can ensure that our response conforms to the response type.

marshallmain · 2023-09-15T04:19:48Z

x-pack/plugins/security_solution/common/search_strategy/security_solution/index.ts

@@ -218,6 +219,76 @@ export type StrategyResponseType<T extends FactoryQueryTypes> = T extends HostsQ
  ? UsersRelatedHostsStrategyResponse
  : never;

+export type StrategyRequestInputType<T extends FactoryQueryTypes> = T extends HostsQueries.hosts


A neat trick we can use here to avoid maintaining this large conditional is the TypeScript Extract feature (https://stackoverflow.com/questions/64527150/in-typescript-how-to-select-a-type-from-a-union-using-a-literal-type-property). Since we have all our request schemas in one discriminated union searchStrategyRequestSchema, we can create this generic type based on that union:

type SearchStrategyRequestInputSchema = z.input<typeof searchStrategyRequestSchema>; export type RequestSchemaType<T extends FactoryQueryTypes> = Extract<SearchStrategyRequestInputSchema, {factoryQueryType: T}>; export type Test = RequestSchemaType<HostsQueries.hosts>;

This code makes the Test type equivalent to StrategyRequestInputType<HostsQueries.hosts>. We can do the same thing for the existing code (StrategyRequestType and StrategyResponseType) that established this pattern and reduce some of the tedious code maintenance here.

kibana-ci · 2023-09-19T12:43:10Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 01ba165

Failed CI Steps

Serverless Security Investigations Cypress Tests #2

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4487	4541	+54
`timelines`	93	121	+28
total			+82

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`timelines`	213	196	-17

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	12.8MB	12.8MB	+34.5KB
`timelines`	30.0KB	30.0KB	+2.0B
total			+34.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`timelines`	22	17	-5

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`timelines`	42.6KB	109.5KB	+66.8KB

Unknown metric groups

API count

id	before	after	diff
`timelines`	257	240	-17

ESLint disabled line counts

id	before	after	diff
`securitySolution`	453	455	+2
`timelines`	22	23	+1
total			+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	519	521	+2
`timelines`	22	23	+1
total			+3

History

💔 Build #159911 failed fb2555d
💔 Build #159903 failed 8ccba2f
💔 Build #159594 failed 2b3c3e2
💛 Build #159135 was flaky e855070
💛 Build #158975 was flaky 9736cf3
💔 Build #158802 failed 766e198

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

angorayc

Tested locally and LGTM, thanks so much for adding the schemas.

…egies in security solution & timelines (elastic#162539) ## Summary This PR specifies validation schemas for enpoints listed here: elastic/security-team#6486

lgestc force-pushed the hosts_api_schema branch 4 times, most recently from e77950d to 1f6a83b Compare July 26, 2023 08:03

lgestc changed the title ~~[Serverless] Hosts search strategy schema~~ [Serverless] Remaining strategy schemas Jul 26, 2023

lgestc force-pushed the hosts_api_schema branch 21 times, most recently from 14209d9 to 56ee1b5 Compare July 31, 2023 12:44

lgestc requested review from michaelolo24 and marshallmain July 31, 2023 12:57

lgestc force-pushed the hosts_api_schema branch from 56ee1b5 to c9dbf09 Compare August 1, 2023 14:09

lgestc commented Aug 1, 2023

View reviewed changes

x-pack/plugins/security_solution/common/api/search_strategy/model/request_basic_options.ts Outdated Show resolved Hide resolved

michaelolo24 reviewed Sep 12, 2023

View reviewed changes

x-pack/plugins/timelines/common/api/search_strategy/model/runtime_mappings.ts Show resolved Hide resolved

michaelolo24 reviewed Sep 12, 2023

View reviewed changes

michaelolo24 approved these changes Sep 12, 2023

View reviewed changes

marshallmain reviewed Sep 13, 2023

View reviewed changes

lgestc added 10 commits September 13, 2023 15:23

pr suggestions

b2995aa

pr requested changes

4aa5d7f

pr changes

9dff579

Merge branch 'main' into hosts_api_schema

444ca97

pr changes

1a1932b

remove passthroughs

5944944

improve types for useSearchStrategy

766e198

fix use_first_last_seen tests

8a6e639

Merge branch 'main' into hosts_api_schema

9736cf3

sorting non optinal in hostDetailsSchema

e855070

marshallmain approved these changes Sep 15, 2023

View reviewed changes

lgestc added 4 commits September 18, 2023 17:08

improve timelines schemas

2b3c3e2

improve timeline types

8ccba2f

Merge branch 'main' into hosts_api_schema

fb2555d

remove non-public import

01ba165

lgestc added 8.11 candidate and removed 8.10 candidate labels Sep 20, 2023

angorayc approved these changes Sep 20, 2023

View reviewed changes

lgestc merged commit 3a017de into elastic:main Sep 21, 2023

lgestc deleted the hosts_api_schema branch September 21, 2023 05:57

kibanamachine added the v8.11.0 label Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serverless] Add schema validation to Search Strategies in security solution & timelines #162539

[Serverless] Add schema validation to Search Strategies in security solution & timelines #162539

lgestc commented Jul 26, 2023 •

edited

Loading

michaelolo24 Sep 12, 2023

michaelolo24 Sep 12, 2023

michaelolo24 Sep 12, 2023

michaelolo24 left a comment •

edited

Loading

marshallmain left a comment

marshallmain Sep 13, 2023

lgestc Sep 14, 2023

marshallmain Sep 13, 2023

lgestc Sep 18, 2023

marshallmain left a comment

marshallmain Sep 15, 2023

marshallmain Sep 15, 2023 •

edited

Loading

kibana-ci commented Sep 19, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

angorayc left a comment

		@@ -0,0 +1,83 @@
		/*
		* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one


		import { z } from 'zod';

		export const language = z.union([z.literal('eql'), z.literal('kuery'), z.literal('lucene')]);

[Serverless] Add schema validation to Search Strategies in security solution & timelines #162539

[Serverless] Add schema validation to Search Strategies in security solution & timelines #162539

Conversation

lgestc commented Jul 26, 2023 • edited Loading

Summary

Checklist

Fixing tests on CI

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelolo24 left a comment • edited Loading

Choose a reason for hiding this comment

marshallmain 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

marshallmain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marshallmain Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Sep 19, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

angorayc left a comment

Choose a reason for hiding this comment

lgestc commented Jul 26, 2023 •

edited

Loading

michaelolo24 left a comment •

edited

Loading

marshallmain Sep 15, 2023 •

edited

Loading