[SIEM] Server cutover to New Platform #63430
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
|
Pinging @elastic/siem (Team:SIEM) |
rylnd
changed the title
|
@MadameSheema Update: the previous error was some temporary issue, as cypress now passes both locally and on CI. @MadameSheema if you wouldn't mind giving the cypress changes a once-over to verify I didn't break anything, I'd appreciate it! The relevant changes should be:
|
|
@rudolf I didn't want to add them here, but FYI I plan on following this up with a "Good Kibana Citizen" PR that slightly reworks the siem client, adds mocks, and renames a few non-standard/confusing exported types. |
rylnd
force-pushed
the
siem_server_np_redux
branch
from
8b182e7
to
7f34e2e
Compare
* NP config is not yet used * Relative imports are somewhat broken
These are mostly updating imports to the common/ folder on the UI side (since things changed relative to those files).
A few of these were moved into the cypress folder as they're cypress-specific. I tried to update all the relative paths but some are likely broken. I'm not going to know until other stuff is fixed, though.
The other default values live in there, this is no different.
If this was referencing the full project, it now references both paths (legacy for UI, and NP for server).
* Updates plugin to use NP config * defines new config previously coming from savedObjects config * cleans up legacy types Conflicts: x-pack/plugins/siem/server/lib/detection_engine/routes/rules/export_rules_route.ts x-pack/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts x-pack/plugins/siem/server/lib/detection_engine/rules/types.ts x-pack/plugins/siem/server/plugin.ts x-pack/plugins/siem/server/routes/index.ts x-pack/plugins/siem/server/types.ts
This was originally added to address an issue with tsserver, but that issue is no longer relevant. The presence of this file confuses typescript into thinking that siem is a separate TS project.
These are not necessarily correct in terms of what's required/optional, but this is what's declared in our types.
* Removes legacy instantiation of server plugin, which is now handled by NP * Loosens legacy config spec so we no longer have to duplicate config types
These were written against the old Hapi config function; now, we just have a POJO.
dhurley14
reviewed
Apr 16, 2020
dhurley14
reviewed
Apr 16, 2020
| IP_REPUTATION_LINKS_SETTING, | ||
| IP_REPUTATION_LINKS_SETTING_DEFAULT, | ||
| } from './common/constants'; | ||
| import { defaultIndexPattern } from './default_index_pattern'; |
There was a problem hiding this comment.
It looks like we are not using this elsewhere in the codebase. Do you think we could just delete default_index_pattern.ts?
There was a problem hiding this comment.
That file was meant to be deleted, good catch 👍
spong
reviewed
Apr 16, 2020
| "private": true, | ||
| "license": "Elastic-License", | ||
| "scripts": { | ||
| "extract-mitre-attacks": "node scripts/extract_tactics_techniques_mitre.js & node ../../../scripts/eslint ../../legacy/plugins/siem/public/pages/detection_engine/mitre/mitre_tactics_techniques.ts --fix", |
There was a problem hiding this comment.
When running yarn extract-mitre-attacks I'm seeing this diff with the generated file. Seems there's a difference in the lint config?
There was a problem hiding this comment.
Yeah, I noticed that as well but forgot to circle back. I'll verify that it's working correctly on master and then figure out what's changed.
There was a problem hiding this comment.
It looks like the eslint forget to do its jobs maybe the path change
node scripts/extract_tactics_techniques_mitre.js & node ../../../../scripts/eslint ./public/pages/detection_engine/mitre/mitre_tactics_techniques.ts --fix
There was a problem hiding this comment.
I figured out the fix in #64010 👀
|
This file was moved and has a relative path within it: When running |
spong
reviewed
Apr 17, 2020
x-pack/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts
Show resolved
Hide resolved
spong
reviewed
Apr 17, 2020
Comment on lines
+6
to
+7
| "requiredPlugins": ["actions", "alerting", "features", "licensing"], | ||
| "optionalPlugins": ["encryptedSavedObjects", "ml", "security", "spaces"], |
There was a problem hiding this comment.
Why doesn't case need to be declared here? Do all the necessary references just happen in x-pack/plugins/siem/server/plugin.ts?
There was a problem hiding this comment.
Heh, I think it's not there because we have no references to the case plugin contract in our code (i.e. "I missed it"). I'll follow up with an audit of all of our imports, because I know that we pull in code from other plugins as well.
/cc @rudolf: If we're importing arbitrary code paths from another plugin it needs to be declared as a dependency, right?
There was a problem hiding this comment.
Importing code from another plugin will add that code to your plugin's bundle so you can safely call it. But it doesn't make that plugin run or guarantee that it's running in the background. Any exported code should be stateless and if that's the case then this isn't a problem.
When you start using another plugin's setup / start API's you need to declare a dependency.
Having said that, I couldn't see any imports or setup / start API's being used from Case, but might have missed it.
spong
reviewed
Apr 17, 2020
spong
reviewed
Apr 17, 2020
| @@ -8,7 +8,6 @@ import { EuiBadge, EuiToolTip, IconType } from '@elastic/eui'; | |||
| import React from 'react'; | |||
| import styled from 'styled-components'; | |||
|
|
|||
| import { Omit } from '../../../common/utility_types'; | |||
There was a problem hiding this comment.
I think (as per my IDE) this was the last ref to this export:
dhurley14
reviewed
Apr 17, 2020
| @@ -163,13 +163,6 @@ export const mockParsedTimelineObject = omit( | |||
| mockUniqueParsedObjects[0] | |||
| ); | |||
|
|
|||
| export const mockConfig = { | |||
| get: () => { | |||
| return 100000000; | |||
There was a problem hiding this comment.
This value (which I think is used for the maxImportPayloadBytes) is an order of magnitude larger than the value in the mock config in detection engine directory which we are using in lieu of this. Not sure if that difference is important for the tests on timelines but just wanted to point it out.
https://github.com/elastic/kibana/pull/63430/files?w=1#diff-12565563a255e05c185b90d50337ec51L46-L48
edit: CC @angorayc
spong
reviewed
Apr 17, 2020
| @@ -1,16 +1,10 @@ | |||
| { | |||
| "author": "Elastic", | |||
| "name": "siem", | |||
| "name": "siem-legacy-ui", | |||
There was a problem hiding this comment.
This change is just to explicitly state that the client UI is still on the old platform (and disambiguate from the non-legacy package.json), correct? Are there any downstream effects of changing the app name here, or does all of the magic that keys off app name/id/title happen from what is set in legacy register_feature.ts?
kibana/x-pack/legacy/plugins/siem/public/register_feature.ts
Lines 11 to 14 in 27f5eaa
There was a problem hiding this comment.
@spong correct, the name here is just to disambiguate; there's a check in the build process that actually verifies that each package.json has a unique name. I have not seen anything to indicate that this name affects the build process but I'll verify that with platform.
dhurley14
approved these changes
Apr 17, 2020
Conflicts: x-pack/legacy/plugins/siem/public/containers/matrix_histogram/index.ts x-pack/legacy/plugins/siem/public/pages/detection_engine/components/signals_histogram_panel/index.tsx x-pack/legacy/plugins/siem/public/pages/overview/events_by_dataset/index.tsx x-pack/plugins/siem/server/lib/detection_engine/scripts/rules/queries/query_with_list.json x-pack/plugins/siem/server/lib/detection_engine/signals/get_filter.ts
These are strings that wouldn't be caught by typescript.
mistic
approved these changes
Apr 17, 2020
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/find_statuses·ts.detection engine api security and spaces enabled find_statuses should return a single rule status when a single rule is loaded from a find status with defaults addedStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
This was referenced Apr 17, 2020
rylnd
added a commit
that referenced
this pull request
Apr 18, 2020
* Move server code into NP folder * NP config is not yet used * Relative imports are somewhat broken * Move common folder into NP * Move cypress folder into NP * Move scripts folder into NP * Move misc. config into NP folder A few of these were moved into the cypress folder as they're cypress-specific. I tried to update all the relative paths but some are likely broken. I'm not going to know until other stuff is fixed, though. * Move value for siem index pattern into common/constants The other default values live in there, this is no different. * Update paths following file move If this was referencing the full project, it now references both paths (legacy for UI, and NP for server). * Fix typescript errors related to module resolution These are mostly updating imports to the common/ folder on the UI side (since things changed relative to those files). * Replace Legacy Config with NP Config * Updates plugin to use NP config * defines new config previously coming from savedObjects config * cleans up legacy types Conflicts: x-pack/plugins/siem/server/lib/detection_engine/routes/rules/export_rules_route.ts x-pack/plugins/siem/server/lib/detection_engine/routes/rules/import_rules_route.ts x-pack/plugins/siem/server/lib/detection_engine/rules/types.ts x-pack/plugins/siem/server/plugin.ts x-pack/plugins/siem/server/routes/index.ts x-pack/plugins/siem/server/types.ts * Remove local SIEM tsconfig This was originally added to address an issue with tsserver, but that issue is no longer relevant. The presence of this file confuses typescript into thinking that siem is a separate TS project. * Update kibana.json to declare our dependencies These are not necessarily correct in terms of what's required/optional, but this is what's declared in our types. * Remove legacy plugin instantiation * Removes legacy instantiation of server plugin, which is now handled by NP * Loosens legacy config spec so we no longer have to duplicate config types * Update tests with NP config These were written against the old Hapi config function; now, we just have a POJO. * Update es_archiver helpers' paths I'm not quite sure if these are working yet, but they're no longer throwing errors. * Ignore restricted path on script This was cribbed from infra, who has made a similar change. * Ignore restricted path on temporary savedObject mappings import This will be changed subsequently when we switch to the NP form of savedObject type registration. * Add symlink to lockfile * Fix paths on circular deps script * Add separate config for Rule and Timeline saved objects We had previously used the savedObjects' config, but those are not currently exposed to us on New Platform. For now, we're going to split this into two sets of values for the SOs we deal with importing/exporting within the SIEM app, with the same defaults as savedObjects. * Fixing relative paths within cypress These are strings that wouldn't be caught by typescript.
This was referenced Apr 20, 2020
This was referenced Apr 27, 2020
MindyRS
added
the
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
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
Addresses the server side of #45831.
This does half of the work in #59624. Due to the current issues with the maps embeddables, we're going to migrate the backend now for 7.8, and circle back to the frontend once #58178 is complete.
Due to the noise related to moving/modifying these files in a single PR, I've tried my best to split that into two consecutive steps:
xpack.siem.max{Rule,Timeline}ImportExportSizexpack.siem.max{Rule,Timeline}ImportPayloadBytesChecklist
For maintainers