[SIEM] Final Shimming/Prep for Server NP Migration #56814
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rylnd
added
Team:SIEM
v8.0.0
release_note:skip
|
Pinging @elastic/siem (Team:SIEM) |
rylnd
force-pushed
the
final_backend_prep
branch
2 times, most recently
from
9f6bb66
to
945410b
Compare
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.ts
Outdated
Show resolved
Hide resolved
Next we'll be trimming down ServerFacade to the final few dependencies, and using our plugin dependencies for the rest.
For now, let's try to simplify our typings by exporting our plugin's dependencies from the plugin itself, since we know it already knows about them.
I'm throwing the alerting registration in the plugin for now, too.
Now that we've audited our use of request objects, we can switch to the more friendly LegacyRequest that all our utilities are expecting. LegacyRequest doesn't have plugins, either, so our only remaining errant usage is retrieving clients from it, which we call out explicitly.
This just threw an error on startup, so I'm fixing all of them to be safe.
These come through as new KibanaRequests and not as the LegacyRequest that I had incorrectly typed them as. I'm only caring about the `body` property right now, since that's all we really deal with, and in testing it was all that was populated in our actual requests, too.
We're using the legacy version for now, but ServerFacade will be gone by the end of the migration.
This changes the signature of getIndex, which is used in quite a few places. We'll see how bad that looks in the next commit.
We're already ignoring another portion of the platform shim due to it being incompatibly typed, so we might as well remove this.
This contains our legacy stuff for now. Eventually, it will only be our NP services. This breaks a few tests due to the way createMockServer works, but I'll clean that up momentarily.
The createMockServer helper does a few things differently: * returns mocked LegacyServices that can be passed to our route factories * does not return a server object as we don't need it, except for: * returns an inject function that we can use to execute a request We're casting our services because we're only mocking a subset of what LegacySetupServices entails. Mainly, this allows me to continue moving things off of ServerFacade without significant refactoring of tests.
Unfortunately, LegacyRequest does not allow a request's `query` values to be anything other than a string or string[]. However, in practice they can (and are) objects, booleans, etc. Our request types (e.g. QueryRequest) are correct, but because service functions are LegacyRequest's implementation of `query`, we need to cast them lest a type error occur. For now, the easiest solution is to do this in the request handler: intersecting with our RequestFacade (LegacyRequest) allows us to both a) pluck our query params off the request and b) pass the request to NP services.
We're just retrieving a boolean from it right now, which is also guarded against the plugin being unavailable. If this usage becomes more widespread, we'll make this available at a higher level, probably in redux.
* Simplifies our generic type to accept two arguments: params and the return value * Options is fixed and we were never specifying anything meaningful there * Updates all DE cluster calls to use callWithRequestFactory
* createMockServer now returns the callCluster mock, so that you can easily mock a client response without needing to know the details of how we define that function
This was added during a refactor, but we were never actually using this code. We always retrieve the client from the request via getSavedObjectsClient. I think that the NP client has a slightly different interface, so we're going to create a helper to retrieve it from the request, same as we do with the elastic client. In the future, both of these will be available on the request context, and we can remove the helpers entirely.
In trying to migrate over the savedObjectsClient, I realized that it is not available during setup in the same way that the ES client is. Since our routes need pieces from both setup and start phases, I've added a Services class to accumulate/transform these services and expose scoped clients when given a legacy request. In New Platform, these clients will be available upon the request context and we should be able to remove getScopedServicesFactory for our routes. A subset of Services' functionality may still be useful, we'll see.
I decided that config shouldn't live in here, as this is only client-related stuff. Probably going to rename this ClientsService. Things are still very much broken.
This gets us client-related services (ES, SavedObjects, Alerts), but it is independent of any configuration, which is gonna be another service.
This is a weird helper, I'm not really sure where it should go.
Return clients, as this is closer to what we'll get in the request context.
Conflicts: x-pack/legacy/plugins/siem/server/kibana.index.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/index/create_index_route.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/index/delete_index_route.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/add_prepackaged_rules_route.test.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/add_prepackaged_rules_route.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_route.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/delete_rules_bulk_route.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/find_rules_route.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/update_rules_route.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/types.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/types.ts
Conflicts: x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.test.ts x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.ts
* Updates types to reflect that spaces may be unavailable * Adds a test for getSpaceId's behavior when spaces are disabled
rylnd
force-pushed
the
final_backend_prep
branch
from
99d5a78
to
668702b
Compare
dhurley14
reviewed
Feb 13, 2020
| @@ -4,6 +4,7 @@ | |||
| * you may not use this file except in compliance with the Elastic License. | |||
| */ | |||
|
|
|||
There was a problem hiding this comment.
curious - should this import use 'kibana/server' instead of the relative import? I remember seeing some sort of message saying when to use one over the other but not sure what situation warrants what.
| const pluginInstance = plugin(initializerContext); | ||
| // @ts-ignore-next-line: NewPlatform shim is too loosely typed | ||
| pluginInstance.setup(setup.core, setup.plugins, __legacy); | ||
| // @ts-ignore-next-line: NewPlatform shim is too loosely typed |
There was a problem hiding this comment.
ouch on having to do ts-ignore-next-line. If we have to, we have to. Just some ouch. Any open tickets or other information about when and how these can be removed or is this is just temporary for a shim that will be altogether removed once we are out of the legacy folder?
There was a problem hiding this comment.
This is specific to the shim; this entire file will be removed on new platform.
There was a problem hiding this comment.
A few of these (CoreStart, CoreSetup) are properly typed, but because of the dynamic DI the plugins types are understandably loose. We could make this better, but we'd still have to cast parts of it, and this is all temporary code.
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/clients_service_mock.ts
Show resolved
Hide resolved
| const callWithRequest = callWithRequestFactory(request, server); | ||
| const indexExists = await getIndexExists(callWithRequest, finalIndex); | ||
| const finalIndex = | ||
| outputIndex != null ? outputIndex : getIndex(spacesClient.getSpaceId, config); |
There was a problem hiding this comment.
optional: I think we can use the fancy smancy ?? here? Otherwise still looks good, sorry you're refactoring my code. I have old habits I am trying to stop doing such as this one.
There was a problem hiding this comment.
@FrankHassanabad In the course of making these changes I think I might've found a bug: https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts#L149 does not seem to respect an imported rule's output index.
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts
Show resolved
Hide resolved
...k/legacy/plugins/siem/server/lib/detection_engine/routes/signals/query_signals_route.test.ts
Outdated
Show resolved
Hide resolved
...k/legacy/plugins/siem/server/lib/detection_engine/routes/signals/query_signals_route.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.test.ts
Show resolved
Hide resolved
dhurley14
reviewed
Feb 13, 2020
|
|
||
| export const createReadTagsRoute: Hapi.ServerRoute = { | ||
| export const createReadTagsRoute = (getClients: GetScopedClients): Hapi.ServerRoute => ({ |
There was a problem hiding this comment.
Just throwing this out there -> we are mixing ({}) and { return {} } in between the routes. Not sure if we want to take this opportunity to create some consistency?
There was a problem hiding this comment.
Yeah, I prefer the former unless there's a good reason. In the interest of this PR, the reason was not introducing a lot of whitespace changes: if converting caused everything to be re-indented, I didn't do it.
FrankHassanabad
approved these changes
Feb 13, 2020
There was a problem hiding this comment.
Left a few comments but feel free with how big this is that maybe some of these issues pre-date this ticket so feel free to create new bugs from what you find and merge this now or fix the few things found and then merge. Up to you, but the sooner this gets in the better chance of not having another conflict you will have.
Overall, really like how large swaths of code were removed in the mocks and how this shim gets us closer to our goals of moving out of the legacy folder.
LGTM, thanks for all the disciple needed to keep all this stuff straight while we were merging code while this was still in progress.
dhurley14
approved these changes
Feb 13, 2020
* Refactors mock expectations so that they're actually evaluated; they can't go within a mockImplementation call as it's evaluated in the wrong scope. * Fixes duplicated test to use the proper assertions
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/dashboard/feature_controls/dashboard_spaces·ts.dashboard feature controls spaces space with no features disabled landing page shows "Create new Dashboard" buttonStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
yctercero
added a commit
to yctercero/kibana
that referenced
this pull request
Feb 14, 2020
rylnd
added a commit
that referenced
this pull request
Feb 14, 2020
* Route all our server setup through the plugin Next we'll be trimming down ServerFacade to the final few dependencies, and using our plugin dependencies for the rest. * Clean up server plugin exports For now, let's try to simplify our typings by exporting our plugin's dependencies from the plugin itself, since we know it already knows about them. * Move DE Routes to to conventional location I'm throwing the alerting registration in the plugin for now, too. * Loosen up our RequestFacade Now that we've audited our use of request objects, we can switch to the more friendly LegacyRequest that all our utilities are expecting. LegacyRequest doesn't have plugins, either, so our only remaining errant usage is retrieving clients from it, which we call out explicitly. * Remove uses of 'src' alias This just threw an error on startup, so I'm fixing all of them to be safe. * Fix types of our GraphQL requests These come through as new KibanaRequests and not as the LegacyRequest that I had incorrectly typed them as. I'm only caring about the `body` property right now, since that's all we really deal with, and in testing it was all that was populated in our actual requests, too. * Initialize our routes with SetupServices We're using the legacy version for now, but ServerFacade will be gone by the end of the migration. * Swap legacy spaces plugin for NP This changes the signature of getIndex, which is used in quite a few places. We'll see how bad that looks in the next commit. * Remove unneeded typing We're already ignoring another portion of the platform shim due to it being incompatibly typed, so we might as well remove this. * WIP: Converting our DE routes to use consolidated services This contains our legacy stuff for now. Eventually, it will only be our NP services. This breaks a few tests due to the way createMockServer works, but I'll clean that up momentarily. * Fix DE routing tests following refactor The createMockServer helper does a few things differently: * returns mocked LegacyServices that can be passed to our route factories * does not return a server object as we don't need it, except for: * returns an inject function that we can use to execute a request We're casting our services because we're only mocking a subset of what LegacySetupServices entails. Mainly, this allows me to continue moving things off of ServerFacade without significant refactoring of tests. * Fix incompatible request types Unfortunately, LegacyRequest does not allow a request's `query` values to be anything other than a string or string[]. However, in practice they can (and are) objects, booleans, etc. Our request types (e.g. QueryRequest) are correct, but because service functions are LegacyRequest's implementation of `query`, we need to cast them lest a type error occur. For now, the easiest solution is to do this in the request handler: intersecting with our RequestFacade (LegacyRequest) allows us to both a) pluck our query params off the request and b) pass the request to NP services. * Move our use of encryptedSavedObjects to NP We're just retrieving a boolean from it right now, which is also guarded against the plugin being unavailable. If this usage becomes more widespread, we'll make this available at a higher level, probably in redux. * Use NP elasticsearch client * Simplifies our generic type to accept two arguments: params and the return value * Options is fixed and we were never specifying anything meaningful there * Updates all DE cluster calls to use callWithRequestFactory * Update DE mocks with NP elasticsearch * createMockServer now returns the callCluster mock, so that you can easily mock a client response without needing to know the details of how we define that function * Remove savedObjects dependency from our legacy dependencies This was added during a refactor, but we were never actually using this code. We always retrieve the client from the request via getSavedObjectsClient. I think that the NP client has a slightly different interface, so we're going to create a helper to retrieve it from the request, same as we do with the elastic client. In the future, both of these will be available on the request context, and we can remove the helpers entirely. * WIP: Convert services to stateful object In trying to migrate over the savedObjectsClient, I realized that it is not available during setup in the same way that the ES client is. Since our routes need pieces from both setup and start phases, I've added a Services class to accumulate/transform these services and expose scoped clients when given a legacy request. In New Platform, these clients will be available upon the request context and we should be able to remove getScopedServicesFactory for our routes. A subset of Services' functionality may still be useful, we'll see. * WIP: Converting routes to use Services factory I decided that config shouldn't live in here, as this is only client-related stuff. Probably going to rename this ClientsService. Things are still very much broken. * WIP: Qualifying our Service to ClientsService This gets us client-related services (ES, SavedObjects, Alerts), but it is independent of any configuration, which is gonna be another service. * Fix types on getIndex function This is a weird helper, I'm not really sure where it should go. * Our ClientsService is a clients ... service Return clients, as this is closer to what we'll get in the request context. * Clean up our server types * Declare legacy types at top-level file * Don't re-export from the plugin solely for convenience, that's a slippery slope straight to circular dependencies * Remove RequestFacade as it was a facade for LegacyRequest * Rename ServerFacade/LegacySetupServices to just LegacyServices * Refactor mocks for new architecture * Separates config, server, and client mocks, as they're now independent in our system, route-wise. * gets one test working, the rest will follow. * Simplify our routing mocks * Adds mock for our new clients service * Greatly simplifies both server and mock configs * Renames factory method of client service * Loosen graphQL endpoint validations These work fine in production, but it's graphQL so we don't really need the additional validation of these endpoints, and we weren't leveraging these types anywhere in Typescript land. Additionally, these restrictive validations prevent the initial introspection calls done by graphiQL to get schema information, and without schemae graphiql wasn't very helpful. This is a dev-only problem, but that's the audience of graphiql. * Remove unused graphql endpoint This was only registered in dev mode; I thought that it was needed by graphiql. However, after digging further I realized that graphiQL also only makes POST calls to our real graphQL endpoint, so this route is unnecessary. * Reduce our dependence on PluginInitializerContext After a little more introspection I realized our FrameworkAdapter doesn't need the kibana version. It was only used in order to make a dev request via (graphiql), but even that can be performed with a simpler xsrf header. This meant that we really only wanted to know whether we're in production or not, so instead we pass that simple boolean to the constructor. * Fix FrameworkAdapter type We no longer need this property. * Update detections route tests Uses the new routes interfaces, and our corresponding new mocks. * Remove unnecessary null checks Our savedObjectsClient is always going to be there. * Remove unused type YAGNI * Remove unused savedObjects client Turns out we were only destructuring this client for the null check. * Handle case where spaces is disabled We already null-coalesce properly in the clients service, but this property access was missed. * Return default signals index if spaces are disabled * Remove unnecessary casting of our alerts client mock I think that this was the result of us importing the wrong AlertsClient type, or perhaps the types were out of sync. Regardless, they work now. * Return the 'default' space even when spaces are disabled This will allow users with spaces disabled to enable spaces without losing data. The tradeoff is that they may be surprised when signals don't exist within their configured xpack.siem.signalsIndex. * Account for spaces being disabled in ClientsService * Updates types to reflect that spaces may be unavailable * Adds a test for getSpaceId's behavior when spaces are disabled * Fix false positives in query signals routes tests * Refactors mock expectations so that they're actually evaluated; they can't go within a mockImplementation call as it's evaluated in the wrong scope. * Fixes duplicated test to use the proper assertions * style: Prefer null coalescing over ternary Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR accomplishes the following goals:
setupandstartphasesstartphase right now in order to access the necessary clients (which are normalized undergetClients). In NP, these clients will be available in the request handler context, and so the thunk (and likelygetClients) can be removed.createConfig$#45831
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers