[SIEM] Migrate DE Routes to NP #58292
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) |
2 tasks
We're going to add our client to the route handler context. Currently, it's sole purpose is to provide us with the signalsIndex to use for the request. This will allow us to stop passing around most uses of config and getSpaceId, as they were used for this same purpose.
This is one of the more comprehensive routes, so I think we're good to convert the rest.
We're gonna need this all over the place. Until our schema can generate an NP-compatible type, we can use this helper to generate an equivalent validator.
...k/legacy/plugins/siem/server/lib/detection_engine/routes/signals/query_signals_route.test.ts
Outdated
Show resolved
Hide resolved
...ck/legacy/plugins/siem/server/lib/detection_engine/routes/signals/open_close_signals.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/update_rules_route.test.ts
Outdated
Show resolved
Hide resolved
yctercero
reviewed
Mar 3, 2020
We implicitly cast our return value here when we provide the generic type parameter, so there's no reason for the explicit cast.
This will move to server/index in NP.
We don't want consumers mutating our state.
This was a holdover from when we captured calls that had no body, but is now unnecessary.
FrankHassanabad
approved these changes
Mar 3, 2020
yctercero
approved these changes
Mar 3, 2020
If we annotate that both incoming pieces of our headers are of the correct type, then we can spread them into an object of the same type and avoid the index signature issue.
We need to thread through the utility that provides us this same functionality.
dhurley14
approved these changes
Mar 4, 2020
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
rylnd
added a commit
that referenced
this pull request
Mar 4, 2020
* Define a very basic SiemClient
We're going to add our client to the route handler context. Currently,
it's sole purpose is to provide us with the signalsIndex to use for the
request.
This will allow us to stop passing around most uses of config and
getSpaceId, as they were used for this same purpose.
* WIP: Convert first DE route to NP
This is one of the more comprehensive routes, so I think we're good to
convert the rest.
* Abstract joi/NP validator to helper
We're gonna need this all over the place. Until our schema can generate
an NP-compatible type, we can use this helper to generate an equivalent
validator.
* Second route down
* Another one
* updateRulesRoute
* Fix exports
* patchRulesRoute
* Normalize request param type names
* findRulesRoute
* addPrepackagedRulesRoute
* getPrepackagedRulesStatusRoute
* createRulesBulkRoute
* updateRulesBulkRoute
* patchRulesBulkRoute
* deleteRulesBulkRoute
* importRulesRoute
* exportRulesRoute
* findRulesStatusesRoute
* setSignalsStatusRoute
* querySignalsRoute
* Remove unused type
* createIndexRoute
* Convert readIndexRoute
Removes support for a HEAD request here, becuase it was only used by the
signal_index_exists script (which is now gone). A GET request will have
the same semantics, with a few more bytes in the response.
* deleteIndexRoute
* readTagsRoute
* readPrivilegesRoute
We're incorrectly accessing request.auth in here still, fixing that
next.
* We are always authenticated until we support optional auth
* Clean up our route initialization
* Remove our now-unused ClientsService
* Remove unused getIndex helper and references
* Remove legacy route initialization (partial application)
All our routing tests are still totally broken. Coming up!
* Reference name property in context registration
This should be replaced with a reference to the constant, but at least
this doesn't add a third way it's being referenced.
* Convert our first route tests to NP
The API for our test utils isn't final, but this at least gets some
tests passing. We'll see how it handles a more complex example.
* Create Rules tests
* Update addPrepackagedRules tests
There were a lot of incorrect tests in here due to some incorrect route
registration: we were asserting a 404 but receiving a false positive
because the route we wanted didn't exist.
* Read Privileges Route tests
* Delete Rules route tests
* Use more permissive validation in request mock
* createRulesBulkRoute tests
* deleteRulesBulkRoute tests
* More explicit result mock
It was very unclear in the tests what this mock represented.
* findRulesRoute tests
* getPrepackagedRulesRoute tests
* PatchRulesBulkRoute tests
* Convert migrated tests to newest interface
* PatchRulesRoute tests
Also fixes a potential false positive in our patch_rule_bulk_route
validation tests by providing a more realistic payload with a single key
missing.
* Rename file for consistency
* UpdateRulesBulkRoute tests
* deleteRulesBulkRoute tests
* findRulesStatusRoute tests
* updateRulesRoute tests
* setSignalStatusRoute tests
* querySignalsRoute tests
This actually caught a bug where we were not resolving our response before returning
* Update schema tests following rename of type
* Converts Import Rules route tests to NP form
Most of these tests are failing due to our request not being parsed by
Hapi as it previously was. Once we figure out how to generate a
post-middleware request with a file stream, these should be easily
fixed.
* Remove unused import
This was the last remaining reference to hapi in our server plugin. yay!
* Remove unnecessary tests
We're already covering our error paths here.
* Hit success branch of bulk patch route test
* Add test around error path in our route
* Update our router to validate requests by default
This gives us two important behaviors:
1. Requests to inject() will be populated with default values
2. We can test request validation independent of a handler call
* This allows more straightforward assertions as we don't have to
disambiguate between a schema rejection and a data rejection.
* Update route validation tests with new interface
We don't need to reach into our route in the test, nor ts-ignore it.
* Update ImportRules route tests for NP testing
Gets rid of the multipart-form processing that Hapi would convert for us
into a Readable stream. Instead, we generate our own post-middleware
requst that's got a stream on it.
* Remove unnecessary router method
A bug in an initial implementation of inject() lead me to believe that
validations were removing the stream from our requests; this turned out
to be false! YAGNI
* Add an adapter for our route responses
This provides a uniform interface of { body, status, calls }, where body
and status come from the highest-status call (in the case of many). In a
case where we build multiple responses, the preference is to return the
most problematic one first. If there's an issue, one can look into the
other calls and see what's going on.
This breaks the tests and is not fully implemented, but this will allow
us to change how we build responses without affecting our tests.
* Fix remaining assertions in one test file
Helped flesh out our adapter a bit more.
* Update our response adapter to represent how NP transforms our errors
Most importantly, we return a statusCode but not a status_code.
* Update tests to interface with our Response type
This makes them robust to framework/implementation changes.
* Generate our error responses with our siemResponse wrapper
This adds the status_code key that we desire in our error responses.
Tests were updated as well, and they're currently failing because they
expect statusCode, not status_code.
* Update test assertions to look for status_code
* Remove unused import
* Return a meaningful error if an invalid request was injected
This ensures we a) mimic platform behavior so that b) we don't risk a
false positive if our invalid request were to somehow succeed.
* Return a useful error when no route has been registered
* Add back POST variant of delete_rules_bulk_route
Some browsers do not support a request body for DELETE requests.
* Allow headers to be passed to our error response
This is used by Apollo to set some cache/allow headers in the case of
specific bad requests.
* Add back our header-passing from Apollo errors
I also inverted the logic here to handle the special case first.
* Add some tests around our error response helper
* Fix types of our error wrapper
* Move router logic into separate function
This could be decomposed further but getRoute becomes more verbose, so
meh.
* Convert our mock server to a class
It makes the shared state (mocks) of these functions more explicit, and also
does away with some dumb tuple-returns I had (a consequence of trying to
make everything pure).
* Remove need for a route spy
Instead of mocking certain router methods with the same spy and then
retrieving its calls, we can simply iterate over the calls of the router
methods we care about. This is a little less logic and a little more
straightforward.
* Update test with updated copy
We're consolidating on "signals index" when referring to the data index
where signals are stored.
* Remove unneeded type assertion from our route validation factory
We implicitly cast our return value here when we provide the generic
type parameter, so there's no reason for the explicit cast.
* Use exact message assertions
* Export our SiemRequestContext type for consumers
This will move to server/index in NP.
* Make our SiemClient properties readonly
We don't want consumers mutating our state.
* Throw error if SiemClientFactory has not been properly set up
* Remove unnecessary spread
* Use reduce's type argument instead of an explicit assertion
* Remove unnecessary optional chaining
This was a holdover from when we captured calls that had no body, but is
now unnecessary.
* Remove unnecessary headers on mock requests
* Remove non-null assertion in favor of constructor assignment
* Prefer type annotations to explicit casting
If we annotate that both incoming pieces of our headers are of the
correct type, then we can spread them into an object of the same type
and avoid the index signature issue.
* Skip test failure do to handling of authentication
We need to thread through the utility that provides us this same
functionality.
* Mock our cluster calls with realistic data
* Remove TODO as this is addressed in a later PR
Co-authored-by: Josh Dover <[email protected]>
Co-authored-by: Josh Dover <[email protected]>
3 tasks
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
#45831 #49529
Now that alerting's on NP, we're unblocked on our own migration. This migrates all remaining backend legacy code, with the exception of the config service. In a final PR, we'll use our NP-provided config object instead, and do the
git mvfrom/legacy/pluginsto/plugins.Overview of changes
getClientsservice with request handler contextinjecthelper, this meant we could easily test our routes without having to make an HTTP callinject) to invoke the request handler and return the response spy it operated uponvalidate) to run our validation function on the provided request. similar toinject, it returns the response spy it operated on, allowing us to inspect both pass/fail status and any error messages that were generated.Checklist
Delete any items that are not applicable to this PR.
For maintainers