Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SIEM] [Security] unified code structure phase 0 #65965

Merged
merged 8 commits into from
May 12, 2020

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented May 11, 2020

Summary

This PR is to allow our family to get bigger and to have a better dev experience in our security code structure. Instead, to have as the upper-level components/containers/pages/store etc, we wanted to have the area on the top level like hosts/network/cases/timelines/alerts -> components/containers/pages/store etc...

We call this phase 0 because we just wanted to move the file around where it makes the most sense. The next phase will be to clean up more the dependencies between the sub-plugins. This PR was just tedious because of moving all the files and fixing the imports. Thanks To @patrykkopycinski and @cnasikas for the big help.

Reviewer instructions

Note: there are too many files changed in this branch for GitHub to present a nice diff. Instead, you'll have to review this locally:

# configure kibana repo to track renames across lots of files (this is what GitHub isn't doing)
git config diff.renamelimit 2600

# check out the branch locally
git fetch origin refs/pull/65965/head
git checkout -b XavierM/security-structure-2 FETCH_HEAD

# to view the full diff against master:
git diff $(git merge-base head master)

# to view a single file's history:
git log -p --follow -- <path-to-file>

@XavierM XavierM added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels May 11, 2020
@XavierM XavierM requested a review from rylnd May 11, 2020 03:39
@XavierM XavierM requested review from a team as code owners May 11, 2020 03:39
@XavierM XavierM closed this May 11, 2020
@XavierM XavierM reopened this May 11, 2020
@XavierM XavierM closed this May 11, 2020
@XavierM XavierM reopened this May 11, 2020
@XavierM XavierM requested review from spong and oatkiller May 11, 2020 14:56
@rylnd
Copy link
Contributor

rylnd commented May 11, 2020

Just a quick comment: it does look like this inflates siem's initial bundle pretty significantly (2MB up from 200kB) :
Detections_-_Kibana

I think we can fix it after this gets merged, but there's also a quick, naive fix for this branch if you want it:

diff --git a/x-pack/plugins/siem/public/plugin.tsx b/x-pack/plugins/siem/public/plugin.tsx
index 1a76e52c98..cc46025ddc 100644
--- a/x-pack/plugins/siem/public/plugin.tsx
+++ b/x-pack/plugins/siem/public/plugin.tsx
@@ -33,12 +33,6 @@ import { APP_ID, APP_NAME, APP_PATH, APP_ICON } from '../common/constants';
 import { initTelemetry } from './common/lib/telemetry';
 import { KibanaServices } from './common/lib/kibana/services';
 import { serviceNowActionType, jiraActionType } from './common/lib/connectors';
-import { Cases } from './cases';
-import { Alerts } from './alerts';
-import { Hosts } from './hosts';
-import { Network } from './network';
-import { Overview } from './overview';
-import { Timelines } from './timelines';
 
 export interface SetupPlugins {
   home: HomePublicPluginSetup;
@@ -68,12 +62,6 @@ export interface PluginStart {}
 
 export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, StartPlugins> {
   private kibanaVersion: string;
-  private readonly alertsSubPlugin = new Alerts();
-  private readonly casesSubPlugin = new Cases();
-  private readonly hostsSubPlugin = new Hosts();
-  private readonly networkSubPlugin = new Network();
-  private readonly overviewSubPlugin = new Overview();
-  private readonly timelinesSubPlugin = new Timelines();
 
   constructor(initializerContext: PluginInitializerContext) {
     this.kibanaVersion = initializerContext.env.packageInfo.version;
@@ -108,12 +96,19 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S
         security: plugins.security,
       } as StartServices;
 
-      const alertsStart = this.alertsSubPlugin.start();
-      const casesStart = this.casesSubPlugin.start();
-      const hostsStart = this.hostsSubPlugin.start();
-      const networkStart = this.networkSubPlugin.start();
-      const overviewStart = this.overviewSubPlugin.start();
-      const timelinesStart = this.timelinesSubPlugin.start();
+      const alertsSubPlugin = new (await import('./alerts')).Alerts();
+      const casesSubPlugin = new (await import('./cases')).Cases();
+      const hostsSubPlugin = new (await import('./hosts')).Hosts();
+      const networkSubPlugin = new (await import('./network')).Network();
+      const overviewSubPlugin = new (await import('./overview')).Overview();
+      const timelinesSubPlugin = new (await import('./timelines')).Timelines();
+
+      const alertsStart = alertsSubPlugin.start();
+      const casesStart = casesSubPlugin.start();
+      const hostsStart = hostsSubPlugin.start();
+      const networkStart = networkSubPlugin.start();
+      const overviewStart = overviewSubPlugin.start();
+      const timelinesStart = timelinesSubPlugin.start();
 
       return renderApp(services, params, {
         routes: [

@rylnd
Copy link
Contributor

rylnd commented May 11, 2020

@elasticmachine merge upstream

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked it out locally, ran through some standard desk tests. Did not check every single line of code, but I did page through the entirety of it with git diff $(git merge-base master head) -w --word-diff=color to see if there were any large logical changes: besides the top-level restructuring, things looked pretty mechanical/straightforward.

I was not able to find any issues with my desk testing, and once CI is green I think that'll give us enough confidence to merge this now and fix any outstanding issues between now and release.

This new structure is going to be really nice moving forward; thank you for taking the time to do this @XavierM 🥇

@rylnd
Copy link
Contributor

rylnd commented May 11, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@XavierM XavierM merged commit 3bb51bb into elastic:master May 12, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 12, 2020
* master: (46 commits)
  [Drilldowns][chore] Remove some any's from components. Remove `PlaceContext` from components (elastic#65854)
  [functional/services] import By/until from module (elastic#66015)
  [Drilldowns][IE] fix welcome bar layout in IE (elastic#65676)
  Inspect action shows on dashboard for every chart (elastic#65998)
  Fix heigt calc in calc issue for ie11 (elastic#66010)
  [Flights] Delay Bucket - Error notification on opening sample visualization (elastic#66028)
  [SIEM] [Security] unified code structure phase 0 (elastic#65965)
  [Maps] Organize layers into subfolders (elastic#65513)
  skip flaky suite (elastic#59849)
  Cleanup prefill and edit flow. (elastic#66105)
  Fix major severity service map ring colors (elastic#66124)
  [DOCS] Improves formatting in action types (elastic#65932)
  [DOCS] APM Agent config: Setting values must be string (elastic#65875)
  Change default cert age limit value. (elastic#65918)
  [DOCS] Removed saved object options (elastic#66072)
  [SIEM] [Cases] Case API tests (elastic#65777)
  Add example of of local plugin installation (elastic#65986)
  skip flaky suite (elastic#65741)
  [SIEM][Detections] Restrict ML rule modification to ML Admins (elastic#65583)
  [Reporting/Test] Add Functional test for download CSV (elastic#65401)
  ...
jloleysens added a commit that referenced this pull request May 12, 2020
…ine-editor

* 'master' of github.com:elastic/kibana: (37 commits)
  [APM] Correct relative paths in scripts (#66159)
  [Uptime] Enable deselection of stale filters (#65523)
  [Drilldowns][chore] Remove some any's from components. Remove `PlaceContext` from components (#65854)
  [functional/services] import By/until from module (#66015)
  [Drilldowns][IE] fix welcome bar layout in IE (#65676)
  Inspect action shows on dashboard for every chart (#65998)
  Fix heigt calc in calc issue for ie11 (#66010)
  [Flights] Delay Bucket - Error notification on opening sample visualization (#66028)
  [SIEM] [Security] unified code structure phase 0 (#65965)
  [Maps] Organize layers into subfolders (#65513)
  skip flaky suite (#59849)
  Cleanup prefill and edit flow. (#66105)
  Fix major severity service map ring colors (#66124)
  [DOCS] Improves formatting in action types (#65932)
  [DOCS] APM Agent config: Setting values must be string (#65875)
  Change default cert age limit value. (#65918)
  [DOCS] Removed saved object options (#66072)
  [SIEM] [Cases] Case API tests (#65777)
  Add example of of local plugin installation (#65986)
  skip flaky suite (#65741)
  ...
XavierM added a commit to XavierM/kibana that referenced this pull request May 12, 2020
* apply new structure for teh security solutions

* fix few imports + store

* fix types

* update path in test

* miss path in api_integration

Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	x-pack/plugins/siem/public/components/paginated_table/index.tsx
#	x-pack/plugins/siem/public/containers/hosts/index.tsx
#	x-pack/plugins/siem/public/pages/detection_engine/components/activity_monitor/columns.tsx
#	x-pack/plugins/siem/public/pages/detection_engine/components/activity_monitor/index.tsx
#	x-pack/plugins/siem/public/pages/detection_engine/rules/all/columns.tsx
#	x-pack/plugins/siem/public/pages/network/ip_details/index.test.tsx
Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Pumped to work together more.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 14, 2020
@tonymeehan
Copy link

wow

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 15, 2020
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants