METRON-1671: Initial PCAP UI by tiborm · Pull Request #1103 · apache/metron

tiborm · 2018-07-13T18:30:35Z

Contributor Comments

This PR contains the initial cut of PCAP UI.

The user is able to submit a PCAP process job by the search button (magnifier icon) at the top of the page. In the same filter bar User is able to narrow the result set by filtering to the following fields:

IP Source Address
IP Source Port
IP Dest Address
IP Dest Port
Protocol
Include Reverse Traffic
Free text filtering

While the PCAP job is in progress a progress bar shows up below the filter bar. (Sorry, having no screenshot about it.)

The result of the PCAP processing is visualized in a grid. User able to open each record to get access to the details of the packet.

Running tests

If you like to run the unit tests run:
npm install
npm test

For addition info please check the readme.md in metron-alerts.

Testing

This has been tested in full dev. In order to test:

Spin up full dev.
Put pcap data inside of /apps/metron/pcap/input in your VM.
Start metron-alerts.
Sign in and click on the PCAP tab in the upper left hand corner of the screen.
Submit a pcap search which will return a set of results from your loaded pcap data, preferably on that will match at least a few dozen results.

For test scenarios please check https://issues.apache.org/jira/browse/METRON-1671

Pull Request Checklist

Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.

In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:

For all changes:

Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?

For code changes:

Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh 
```
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via site-book/target/site/index.html:
```
cd site-book
mvn site
```

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.

into pcaprest Conflicts: metron-interface/metron-alerts/src/app/app.module.ts

Conflicts: metron-interface/metron-rest/src/main/java/org/apache/metron/rest/MetronRestApplication.java metron-interface/metron-rest/src/main/java/org/apache/metron/rest/controller/PcapQueryController.java metron-interface/metron-rest/src/main/java/org/apache/metron/rest/util/pcapQueryThread.java

mmiklavc · 2018-07-13T20:20:52Z

@tiborm Thanks for the contribution! A few comments for starters:

Can you provide a test script for reviewers?
What's this 10k+ line spec file? Are these working AngularJS unit tests, and if so, are they now part of the build? Please comment, and if they are not part of the automated build, please provide instructions for running them even if it's simply pointing to an existing doc.
When you have run through the items in the PR checklist from above, please mark them as such.

mmiklavc · 2018-07-13T20:22:34Z

Some screenshots of your UI changes with accompanying explanation would also be useful so reviewers know what we're looking at.

ottobackwards · 2018-07-16T12:46:10Z

I think we should rename from alert ui to investigate or something

tiborm · 2018-07-17T14:33:05Z

@mmiklavc The 10k+ line spec file is a working unit test which contains mock data as well.
I also cerry-picked a commit which fixes all the failing unit tests.

@justinleet managed to make UI unit tests part of our Travis build. He opens a PR for that soon.

james-sirota · 2018-07-19T03:31:38Z

I reviewed the UI. Everything comes up fine and looks good. One assumption I am making is that protocol is numeric because the protocol field will not allow me to enter a character. +1

….com/tiborm/metron into METRON-1671-review

…che/metron into feature/METRON-1554-pcap-query-panel

mmiklavc · 2018-07-20T17:51:00Z

@tiborm @sardell Can one of you list out the UI PR's and indicate the order in which they should be tested and subsequently merged into the feature branch? Since this is the initial major ticket, you can probably add that as a comment here. In addition, each PR should clearly indicate which PR's it depends on and those that are depending on it.

cestella

Do we have any idea why or how these .ts files are able to make their way through the RAT checks on license headers?

cestella · 2018-07-20T18:48:20Z

metron-interface/metron-alerts/src/app/pcap/pcap-filters/pcap-filters.component.spec.ts

@@ -0,0 +1,29 @@
+import { async, ComponentFixture, TestBed } from '@angular/core/testing';


Some of these .ts files don't have license headers, how is this passing the rat check? Do we have a rat exclusion that is too broad?

Thanks, we added license headers.

cestella · 2018-07-20T18:48:29Z

metron-interface/metron-alerts/src/app/pcap/pcap-filters/pcap-filters.component.ts

@@ -0,0 +1,24 @@
+import { Component, OnInit, Input, Output, EventEmitter } from '@angular/core';


we need a license header here

Thanks, we added license headers.

cestella · 2018-07-20T18:48:38Z

metron-interface/metron-alerts/src/app/pcap/pcap-list/pcap-list.component.spec.ts

@@ -0,0 +1,53 @@
+import { async, ComponentFixture, TestBed } from '@angular/core/testing';


we need a license header here

Thanks, we added license headers.

cestella · 2018-07-20T18:48:47Z

metron-interface/metron-alerts/src/app/pcap/pcap-list/pcap-list.component.ts

@@ -0,0 +1,22 @@
+import { Component, OnInit, Input } from '@angular/core';


we need a license header here

cestella · 2018-07-20T18:49:15Z

metron-interface/metron-alerts/src/app/pcap/pcap-packet-line/pcap-packet-line.component.ts

@@ -0,0 +1,38 @@
+import { Component, OnInit, Input } from '@angular/core';


we need a license header here

cestella · 2018-07-20T18:49:45Z

metron-interface/metron-alerts/src/app/pcap/pcap-packet/pcap-packet.component.ts

@@ -0,0 +1,22 @@
+import { Component, OnInit, Input } from '@angular/core';


we need a license header here

Thanks, we added license headers.

cestella · 2018-07-20T18:49:55Z

metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.spec.ts

@@ -0,0 +1,51 @@
+import { async, ComponentFixture, TestBed } from '@angular/core/testing';


we need a license header here

Thanks, we added license headers.

cestella · 2018-07-20T18:50:05Z

metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.ts

@@ -0,0 +1,65 @@
+import { Component, OnInit, Input } from '@angular/core';


we need a license header here

Thanks, we added license headers.

cestella · 2018-07-20T18:50:20Z

metron-interface/metron-alerts/src/app/pcap/service/pcap.service.spec.ts

@@ -0,0 +1,1735 @@
+import { TestBed, async, inject } from '@angular/core/testing';


we need a license header here

cestella · 2018-07-20T18:50:52Z

metron-interface/metron-alerts/src/app/pcap/service/pcap.service.ts

@@ -0,0 +1,10186 @@
+import {Injectable, NgZone} from '@angular/core';


we need a license header here

justinleet · 2018-07-20T19:04:02Z

There's a bug report that rat treats .ts files as binary and ignores them.
https://issues.apache.org/jira/browse/RAT-234

cestella · 2018-07-20T20:15:45Z

I have submitted a PR to up the rat plugin version and have fixed it in master #1126, let's make sure to add license headers here for new files added in this branch though.

Add backend integration and add license headers

….com/tiborm/metron into METRON-1671-review

merrimanr · 2018-07-23T19:00:52Z

I tested this in full dev and the fixed query request is incorrect. Here's what is being sent:

{
  "startTime": 0,
  "endTime": 150000000000000000,
  "srcIp": "",
  "srcPort": 22,
  "dstIp": "",
  "dstPort": "",
  "protocol": "",
  "packetFilter": "",
  "includeReverseTraffic": false
}

when it should look like:

{
  "startTimeMs": 0,
  "endTimeMs": 150000000000000000,
  "ipSrcAddr": "",
  "ipSrcPort": 22,
  "ipDstAddr": "",
  "ipDstPort": "",
  "protocol": "",
  "packetFilter": "",
  "includeReverse": false
}

I've submitted a PR that fixes the issue. The UI works as expected with those changes. Let me know if there is anything missing in that.

mmiklavc · 2018-07-23T19:37:44Z

Have you included steps to reproduce the behavior or problem that is being changed or addressed?

Have you included steps or a guide to how the change may be verified and tested manually?

Can you submit or link to the test script/instructions for this?

justinleet · 2018-07-23T19:55:27Z

I have a couple comments after spinning up #1122 (which should have all these changes, I believe).

Better input validation would be nice. I can currently enter negative ports (which don't match anything) or ports beyond max int (which match everything!). Ideally this would also happen in the REST layer, because it kicks off jobs that have nonsense parameters.
The UI doesn't appear to allow users to kill jobs (although the REST API exists for it). Is it correct that this isn't supported?
The parameters differ if you've never entered a field vs. entered then deleted it. For example, for ports (and possibly just the two port fields), it'll be empty string if never entered and null if entered then deleted. This appears to be benign, although surprising.
Kicking off a job, then navigating to the alerts view, then navigating back allows a user to kick off a second job. I'm not sure what happens when as these jobs finish in potentially either order (or any order if you repeat this and kick off many jobs).
The UI polls for job status infinitely if the REST UI dies.
It's odd that the port fields have increment / decrement buttons. I would expect a user to manually enter that every time.
I get an NPE if there are no pcaps within the date range provided by the query. This is a backend query thing, so it might be worth creating a separate ticket for it?

mmiklavc · 2018-07-24T04:27:32Z

I'm not sure the Hadoop Job object is asynchronous anyways.

@merrimanr If I'm right on the context of the Q and A here, the PcapJob around the underlying MR job handles it async or sync. For REST, we expose it asynchronously via the job manager.

Updated fixed pcap request

merrimanr · 2018-07-24T13:17:47Z

@mmiklavc The job manager is asynchronous in that it accepts a Finalizer. We have the polling loop in place but we would need to refactor the job manager to expose a callback function for getStatus.

@justinleet I get where you're coming from, async communication would be ideal. For this use case though, I don't see much of a benefit. There is no reason we can't send status for all user jobs in response to a single polling request (a trivial change since there is already a getJobs method on the job manager). Also, we are reporting percentage done so there would likely be multiple aysnc calls anyways as the job progresses.

This is definitely something we should add to our platform and I would be happy to work on it with you. Sounds like you're ok with this being a follow on. I'm sure we'll need this construct at some point in the near future.

mmiklavc · 2018-07-24T18:06:50Z

@tiborm - I see a lot of recent changes to dependency versions in the package lock file - is that intentional? Just a short comment about what it's for would be helpful.

justinleet · 2018-07-25T13:30:46Z

In the screenshots you provide, the timestamp field contains both the epoch timestamp and the date. I'd have to dig in, but based on PDML files I've seen, it's displaying both the "show" and the "value". Seems like we should either be splitting that display field, or just choosing one.

justinleet · 2018-07-25T13:31:42Z

metron-interface/metron-alerts/src/app/pcap/pcap-packet-line/pcap-packet-line.component.html

+	OR CONDITIONS OF ANY KIND, either express or implied. See the License for
+  the specific language governing permissions and limitations under the License.
+  -->
+<td class="timestamp">{{ip.timestamp.value}} <span class="date">{{ip.timestamp.show}}</span></td>


This looks like the culprit for the double timestamp/data showing

@justinleet I created a ticket to get some feedback on whether we should be displaying both field values as separate fields in the UI or if we should just choose one. I'm going to hold off on making changes to this code until we get some feedback from the community.

…che/metron into feature/METRON-1554-pcap-query-panel

tiborm · 2018-07-25T14:51:23Z

@mmiklavc We had to update a package-lock.json file because of the original one contained a package collision. npm ci command just failed on that.

tiborm · 2018-07-26T18:52:55Z

@cestella We added the license headers for all new files.

tiborm · 2018-07-26T19:08:30Z

@mmiklavc The list of the sequential PR's in order are the following:

METRON-1671: Initial PCAP UI
#1103

METRON-1662: Adding download button
#1118

METRON-1676: Adding date range selector to PCAP filter bar
#1119

METRON-1675: Add Pagination to PCAP
#1121

METRON-1683: Fix the download progress bar in PCAP UI
#1122

tiborm · 2018-07-27T10:57:09Z

@mmiklavc Thanks for the comment! I extended the JIRA ticket with user story like test scenarios.
Also added a short description of to the PR description about how to spin up a full dev with pcap.

tiborm · 2018-07-27T11:02:09Z

As part of the latest commits I removed commented code blocks, and fixed the variable naming issues in pcap-packet-line.component.ts.
This PR and the followup ones are updated by the latest changes from the base branch.

sardell · 2018-07-27T13:14:00Z

@justinleet In response to your UI-related questions:

Better input validation would be nice. I can currently enter negative ports (which don't match anything) or ports beyond max int (which match everything!). Ideally this would also happen in the REST layer, because it kicks off jobs that have nonsense parameters.

It looks like @ruffle1986 added some validation in this branch. Are you fine with this being a follow up PR?

The UI doesn't appear to allow users to kill jobs (although the REST API exists for it). Is it correct that this isn't supported?

In the current iteration of the UI, a button to kill jobs is not available. I agree that we should add this to the UI, but if it's okay with you I think this could be a follow on PR.

The parameters differ if you've never entered a field vs. entered then deleted it. For example, for ports (and possibly just the two port fields), it'll be empty string if never entered and null if entered then deleted. This appears to be benign, although surprising.

Nice catch. I think we should open a ticket to investigate this further.

Kicking off a job, then navigating to the alerts view, then navigating back allows a user to kick off a second job. I'm not sure what happens when as these jobs finish in potentially either order (or any order if you repeat this and kick off many jobs).

If a job is now running, an error is returned from the API. The UI work to handle this is in this commit.

It's odd that the port fields have increment / decrement buttons. I would expect a user to manually enter that every time.

It looks like this was resolved by @ruffle1986 in this branch by removing the type attribute on the input.

merrimanr · 2018-07-27T20:13:25Z

Given that unit tests will be addressed in a forthcoming PR, this is good enough for me as an initial UI. +1 pending other commenters are satisfied.

merrimanr · 2018-07-30T21:46:24Z

Several people have given feedback on this PR. What do you think @mmiklavc, @justinleet and @cestella? Is this good enough for a first pass at the UI?

justinleet · 2018-07-31T14:00:36Z

@sardell I'm fine with this stuff being follow on. Can we make sure we have tickets linked to https://issues.apache.org/jira/browse/METRON-1554?

I believe the follow-ons are

Input validation
Kill button for in progress jobs
Testing the backend for the null vs blank discrepancy. I don't care that the front end gives either, I care about making sure there are tests such that we're sure being provided either works as expected.

Is there anything else that tickets need to be created for (assuming I didn't just miss them)?

Once we get the tickets made (and linked here, so it's easy for anyone following the discussion to find them), I'm good with this being the basis for the rest of the UI work.

sardell · 2018-08-01T14:25:12Z

@justinleet Here are the tickets for the follow-on items:

Input validation: https://issues.apache.org/jira/browse/METRON-1712
Kill button for in progress jobs: https://issues.apache.org/jira/browse/METRON-1713
Testing different parameter values: https://issues.apache.org/jira/browse/METRON-1718

I believe that's all the tickets that need to be created stemming from the discussion above.

justinleet · 2018-08-01T14:28:43Z

@sardell Thanks for adding the tickets.

+1, @tiborm Thanks for your patience during the whole review process. I'm looking forward to getting the ball rolling on the rest of the contributions!

mmiklavc · 2018-08-01T16:29:38Z

Looks like all comments have been addressed, reviewers have been able to test successfully, and any remaining follow-on work has been added to the feature branch Jira/Epic. I'm +1 by inspection. Nice work gentlemen.

merrimanr · 2018-08-01T16:46:41Z

This has been merged into the feature branch. Can you close it @tiborm?

tiborm · 2018-08-01T18:21:31Z

Thanks to everyone for the feedback and comments!

merrimanr added 3 commits July 13, 2018 16:31

Merge branch 'pcap-front' of https://github.com/simonellistonball/metron

872d1b1

into pcaprest Conflicts: metron-interface/metron-alerts/src/app/app.module.ts

added license headers

55cf2d9

tiborm changed the title ~~Feature/metron 1554 pcap query panel~~ METRON-1554: Initial PCAP UI Jul 16, 2018

tiborm closed this Jul 16, 2018

tiborm reopened this Jul 16, 2018

tiborm changed the title ~~METRON-1554: Initial PCAP UI~~ METRON-1671: Initial PCAP UI Jul 16, 2018

METRON-1662: making unit tests running

c25fd63

fixing broken protocol field

5720f87

tiborm closed this Jul 19, 2018

tiborm reopened this Jul 19, 2018

merrimanr and others added 2 commits July 19, 2018 16:40

Merge branch 'feature/METRON-1554-pcap-query-panel' of https://github…

69f0f69

….com/tiborm/metron into METRON-1671-review

Merge branch 'feature/METRON-1554-pcap-query-panel' of github.com:apa…

c114f30

…che/metron into feature/METRON-1554-pcap-query-panel

cestella reviewed Jul 20, 2018

View reviewed changes

Shane Ardell and others added 5 commits July 23, 2018 14:01

integrate with backend services

596cdeb

add license headers to new .ts files

dd7c66d

Merge pull request #1 from sardell/feature/METRON-1554-pcap-query-panel

cf24659

Add backend integration and add license headers

Merge branch 'feature/METRON-1554-pcap-query-panel' of https://github…

0ecb2de

….com/tiborm/metron into METRON-1671-review

update fixed pcap request

8011d44

tiborm and others added 5 commits July 24, 2018 14:54

refreshing package-lock.json to fix npm ci conflict

65a86b2

adding error message bar

493425d

leting pcap.service throwing error, cleaning code

bfc4517

adding error handling for 404 and status=FAILED, 401 was managed well

db1e675

Merge pull request #4 from merrimanr/METRON-1671-review

a808025

Updated fixed pcap request

justinleet reviewed Jul 25, 2018

View reviewed changes

Merge branch 'feature/METRON-1554-pcap-query-panel' of github.com:apa…

6dac71c

…che/metron into feature/METRON-1554-pcap-query-panel

sardell mentioned this pull request Jul 26, 2018

METRON-1675: Add Pagination to PCAP #1121

Closed

10 tasks

tiborm added 2 commits July 27, 2018 11:57

removing commented code

724e36e

fixing variable nameing

fc9f04e

asfgit pushed a commit that referenced this pull request Aug 1, 2018

METRON-1671 Create PCAP UI (tiborm via merrimanr) closes #1103

d5eb56a

tiborm closed this Aug 1, 2018

		@@ -0,0 +1,29 @@
		import { async, ComponentFixture, TestBed } from '@angular/core/testing';

		@@ -0,0 +1,24 @@
		import { Component, OnInit, Input, Output, EventEmitter } from '@angular/core';

		@@ -0,0 +1,53 @@
		import { async, ComponentFixture, TestBed } from '@angular/core/testing';

		@@ -0,0 +1,22 @@
		import { Component, OnInit, Input } from '@angular/core';

		@@ -0,0 +1,38 @@
		import { Component, OnInit, Input } from '@angular/core';

		@@ -0,0 +1,51 @@
		import { async, ComponentFixture, TestBed } from '@angular/core/testing';

		@@ -0,0 +1,65 @@
		import { Component, OnInit, Input } from '@angular/core';

		@@ -0,0 +1,1735 @@
		import { TestBed, async, inject } from '@angular/core/testing';

		@@ -0,0 +1,10186 @@
		import {Injectable, NgZone} from '@angular/core';

Conversation

tiborm commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor Comments

Running tests

Testing

Pull Request Checklist

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

mmiklavc commented Jul 13, 2018

Uh oh!

mmiklavc commented Jul 13, 2018

Uh oh!

ottobackwards commented Jul 16, 2018

Uh oh!

tiborm commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-sirota commented Jul 19, 2018

Uh oh!

mmiklavc commented Jul 20, 2018

Uh oh!

cestella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinleet commented Jul 20, 2018

Uh oh!

cestella commented Jul 20, 2018

Uh oh!

merrimanr commented Jul 23, 2018

Uh oh!

mmiklavc commented Jul 23, 2018

Uh oh!

justinleet commented Jul 23, 2018

Uh oh!

mmiklavc commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merrimanr commented Jul 24, 2018

Uh oh!

mmiklavc commented Jul 24, 2018

Uh oh!

justinleet commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

tiborm commented Jul 13, 2018 •

edited

Loading

tiborm commented Jul 17, 2018 •

edited

Loading

mmiklavc commented Jul 24, 2018 •

edited

Loading