-
Notifications
You must be signed in to change notification settings - Fork 505
Conversation
|
Simon this looks very nice, I'm going to review. One thing I would ask if you can do an integration test, with integration test data and a configuration for this as we have done with the other parsers? |
|
Agree with @ottobackwards. I just had to mess about in the integration tests for parsers, so here are some links to help that process: |
mmiklavc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @simonellistonball! This is looking pretty good so far. I have a handful of comments that should be pretty trivial to address. This parser will also need to have documentation added here - https://github.com/apache/metron/tree/master/metron-platform/metron-parsing#introduction
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Outdated
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Outdated
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Outdated
Show resolved
Hide resolved
...tron-parsing/metron-parsers/src/test/java/org/apache/metron/parsers/leef/LEEFParserTest.java
Outdated
Show resolved
Hide resolved
...tron-parsing/metron-parsers/src/test/java/org/apache/metron/parsers/leef/LEEFParserTest.java
Outdated
Show resolved
Hide resolved
...tron-parsing/metron-parsers/src/test/java/org/apache/metron/parsers/leef/LEEFParserTest.java
Outdated
Show resolved
Hide resolved
...tron-parsing/metron-parsers/src/test/java/org/apache/metron/parsers/leef/LEEFParserTest.java
Outdated
Show resolved
Hide resolved
...etron-parsing/metron-parsers/src/test/resources/org/apache/metron/parsers/leef/sample.schema
Show resolved
Hide resolved
|
Lots of good catches there Mike, I'll do some clean up. Many of the issues are inherited from the fact that I heavily 'borrowed' from the existing CEF parser. Do you think it would be worth fixing that up at the same time on this PR, since I'm refactoring bits of it anyway? |
|
@ottobackwards sure, integration tests will come shortly. I've not had much luck finding good sample data to make those exercise much more than the unit tests do already (except of course for the pure integration with the parser runner). I would love to see if anyone else has better samples they could contribute to beef up the tests and help us find any edge cases too. |
ottobackwards
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments, looking good
...n-parsing/metron-parsers-common/src/main/java/org/apache/metron/parsers/utils/DateUtils.java
Show resolved
Hide resolved
...orm/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/cef/CEFParser.java
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Outdated
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Outdated
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Outdated
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Outdated
Show resolved
Hide resolved
...m/metron-parsing/metron-parsers/src/main/java/org/apache/metron/parsers/leef/LEEFParser.java
Show resolved
Hide resolved
...tron-parsing/metron-parsers/src/test/java/org/apache/metron/parsers/leef/LEEFParserTest.java
Show resolved
Hide resolved
Is it really a good idea to keep integration tests for the metron-parsers package in the metron-parsers-common package? Seems like we're mixing concerns there. |
I tend to agree, though a handful of them are considered "common."
This should probably be split up a bit, but I don't think it needs to happen in this PR. |
Sure, that sounds reasonable. What did you have in mind? |
|
I would leave the parsers over all testing refactor ( the configurations required for integration getting installed ) for a separate pr and not mix concerns here. Let's get a standard parser through in this pr. |
I was thinking that the same string to constant work done here in the LEEF parser should also be backported to the CEF parser, but that's arguably a follow on task from this PR since it's in a different parser. |
|
I think I've addressed all the good points raised by @ottobackwards and @mmiklavc Anything else blocking this for anyone? |
|
|
||
| private Pattern p; | ||
| private Pattern pext; | ||
| private static final Pattern pext = Pattern.compile(EXTENSION_CAPTURE_PATTERN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, can we give this a better name too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda left that as out of scope since it's in the CEF parser, not the LEEF parser (the LEEF version doesn't use that technique). I'll fix it anyway, although I would say it was a bit of out of the scope of this work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, my mistake. But if we have already touched the file, might as well, but if you don't no biggie
|
nice work simon, i have one more nit comment, after that I'm set to approve |
|
@mmiklavc you all set with this? |
|
Looks good, +1 by inspection. Thanks @simonellistonball. |
|
+1 |
Contributor Comments
LEEF is a popular format in IBM shops as it is the default supported by Qradar. In a number of ways it is similar to CEF. This PR supports LEEF 1.0 and 2.0 per the IBM guide at https://www.ibm.com/support/knowledgecenter/SS42VS_DSM/c_LEEF_Format_Guide_intro.html and also some found in the wild examples which are technically not up to the IBM spec, and are much closer to the CEF spec.
The CEF parser has been slightly refactored in this effort to expose CEF extension parsing for reuse in the LEEF parser for in the wild examples of CEF style 'delimiters'.
This has been tested against a variety of samples from public sources, and from synthetic data generated according to the spec in new unit tests.
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:
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:
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:Have you ensured that any documentation diagrams have been updated, along with their source files, using draw.io? See Metron Development Guidelines for instructions.
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.