METRON-424 ability to validate ip addresses against both IPV4 and IPV6 #303

ottobackwards · 2016-10-11T15:03:08Z

Allow the specification of multiple validation types for IP
{ "fieldValidations" : [ { "input" : [ "field1", "field2" ] ,"validation" : "IP" ,"config" : { "type" : ["IPV4","IPV6"] } } ] }

The question with this PR is if this is the correct approach. Is this the most consistent approach? Is it the least surprising? Would it be better to add a ALL keyword such that instead of specifying both types you can just do

"type": "ALL"?

I did it this way to start, but I'm open to change it based on review.

JonZeolla · 2016-10-11T16:58:21Z

I would personally vote for "type" : ["IPV4","IPV6"]. I usually push for explicit and obvious so something like retrieve_json.py | grep IPV4 would work.

cestella · 2016-10-11T17:01:11Z

I tend to agree, I prefer ["IPV4", "IPV6"] where we just do anor` between the two.

nickwallen · 2016-10-11T17:04:17Z

Jon described it well. I'd prefer "explicit and obvious."

ottobackwards · 2016-10-11T17:12:20Z

OK, good then. I don't have to change anything.
I have no idea why the travis build is failing - it doesn't look todo with my changes ( btw there are always so many errors it is hard to look at the log ). How does on kick off a new build in the event of a transient error? Can anyone help with what is going on in the log?

cestella · 2016-10-11T17:57:40Z

So, the problem with the tests is this:

test(org.apache.metron.parsers.integration.YafIntegrationTest)  Time elapsed: 58.442 sec  <<< ERROR!
java.lang.RuntimeException: Too many retries: 11
    at org.apache.metron.integration.ComponentRunner.process(ComponentRunner.java:140)
    at org.apache.metron.parsers.integration.ParserIntegrationTest.test(ParserIntegrationTest.java:72)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:483)
    at org.junit.internal.runners.TestMethod.invoke(TestMethod.java:59)
    at org.junit.internal.runners.MethodRoadie.runTestMethod(MethodRoadie.java:98)
    at org.junit.internal.runners.MethodRoadie$2.run(MethodRoadie.java:79)
    at org.junit.internal.runners.MethodRoadie.runBeforesThenTestThenAfters(MethodRoadie.java:87)
    at org.junit.internal.runners.MethodRoadie.runTest(MethodRoadie.java:77)
    at org.junit.internal.runners.MethodRoadie.run(MethodRoadie.java:42)
    at org.junit.internal.runners.JUnit4ClassRunner.invokeTestMethod(JUnit4ClassRunner.java:88)
    at org.junit.internal.runners.JUnit4ClassRunner.runMethods(JUnit4ClassRunner.java:51)
    at org.junit.internal.runners.JUnit4ClassRunner$1.run(JUnit4ClassRunner.java:44)
    at org.junit.internal.runners.ClassRoadie.runUnprotected(ClassRoadie.java:27)
    at org.junit.internal.runners.ClassRoadie.runProtected(ClassRoadie.java:37)
    at org.junit.internal.runners.JUnit4ClassRunner.run(JUnit4ClassRunner.java:42)
    at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:283)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:173)
    at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153)
    at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:128)
    at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:203)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:155)
    at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103)

This test failed because of a timeout (probably because the box running it is overloaded. The stream of failures after that point is due to the Parser Integration tests not shutting down the infrastructure it spins up (e.g. storm, etc.) when the tests fail. I'd close and immediately reopen this PR and it should trigger a test rerun. It is probably worth while making a JIRA to correct the integration tests to be smarter and not spew horrible things.

justinleet · 2016-10-12T16:21:01Z

I pulled the PR down locally, and ran into the same integration test issue. I also ran on master locally and it succeeded. I'm not sure what the root issue is if that's (consistently) the case.

ottobackwards · 2016-10-12T16:34:01Z

Justin, how do I run the integration tests?

justinleet · 2016-10-12T17:11:23Z

I just outright did a mvn clean install for the entire project and let it run while I was doing other stuff. but integration-test target I believe also hits them.

nickwallen · 2016-10-12T17:15:25Z

To run the integration tests... Run mvn integration-test from Metron's root directory.

I do not believe that the Integration tests are run with an install or package, those just run the unit tests.

On a side note, you can see what Travis is doing by looking at the .travisyml file that is in Metron's root directory.

nickwallen · 2016-10-12T18:30:31Z

FWIW - I am also getting the integration test failures for this PR when I run them locally. Not sure what the problem is though. I think an exception is being swallowed somewhere.

ottobackwards · 2016-10-12T18:33:58Z

Ok - I'm running the integration-test now. Is there a way to debug it?

nickwallen · 2016-10-12T19:00:44Z

I would run one of the tests and put a break point at ParserIntegrationTest.test() and start walking through it from there.

ottobackwards · 2016-10-12T19:01:33Z

I'm running as in https://maven.apache.org/surefire/maven-surefire-plugin/examples/debugging.html and attaching with intellij in test, as you describe actually. It was how to kick off the test in debug that got me, not having much luck though.

nickwallen · 2016-10-12T19:38:05Z

If that works for you, then great.

I usually use Maven when I need to run all the tests before a merge or PR. When I want to dig into one test and debug something, I just open up Intellij. That makes setting the breakpoint and triggering a single test run very easy.

You just have to watch out because sometimes (rarely) something will work in Intellij, but not in Maven or vice versa. Each builds the code slightly differently.

cestella · 2016-10-13T13:43:20Z

I figured out what was going on with this PR and it's kinda interesting, so forgive the long-winded explanation.

@ottobackwards is indeed right, it is not apparent why his PR about a FieldTransformation and Stellar Function would affect the YafIntegrationTest. Unfortunately, it became apparent when it consistently failed on this PR in both travis and locally that something was happening.

It turns out the problem is here. The keen observer will notice in the global config we are ensuring that the src_ip_addr and dst_ip_addr are valid IP addresses for the integration tests. This, of course, is a typo (that I made :P ). The correct field names are ip_src_addr and ip_dst_addr.
Now, you may ask:

How has this ever worked?
Given that it did work before, why is it broken now?

Well, for the first question, it was never correct in the integration tests, but since we did not exercise the situation where we send in invalid IP addresses it was not apparent. Furthermore, the default behavior for fields which do not exist is to return true (probably wrong).

For the second question, @ottobackwards has changed the default behavior for fields which do not resolve is to return false (almost assuredly right).

For clarity to show how I go about debugging these problems, I'll be rather explicit in how I figured out what was happening:

In my git branch, I added @ottobackwards 's repo as a remote repo and checked out the branch:
- git remote add otto https://github.com/ottobackwards/incubator-metron.git
- git checkout otto/METRON-424
- git branch METRON-424-debug
- git checkout METRON-424-debug
In Intellij, I ran the YafIntegrationTest to verify that it continued to fail
I set breakpoints at the exit points of the ParserBolt:
It became apparent that we were exiting because the message was not valid. I then set breakpoints inside of @ottobackwards 's code and validated that it was doing the right thing, but the fields were coming in as null, which means the fields were not in the message.
I tracked the global.json down and looked at the field names and there the bug became apparent.

I submitted a PR to your PR branch, @ottobackwards, so if you merge that in, the build should work (fingers crossed ;)

nickwallen · 2016-10-13T14:13:13Z

Interesting. Is there anything we can do to make integration test failures like this more transparent in the future?

cestella · 2016-10-13T14:16:10Z

@nickwallen Funny you mention that; yes, I think we can. We should modify ParserIntegrationTest here to check the invalid and error queues and dump their contents and fail immediately.

ottobackwards · 2016-10-13T14:16:45Z

Ok - from what I can see things are written to the kafka topic, but the bolt is never called. I don't see an crashes in the field validation code at this time. Can anyone make sense of these exceptions I get ( I am only running the BRO Integration test )

brointegrationexception.txt

ottobackwards · 2016-10-13T14:31:12Z

That is awesome. Thank you.
I have been looking at the bro test and getting the failures, but I haven’t
gotten hits inside the IP Validator, so i’m a few steps behind.
I will take care of your pull request right way.

The good news, is in true ‘learning by carnage’ fashion, I know know a bit
about the integration tests ;)

On October 13, 2016 at 09:43:23, Casey Stella (notifications@github.com)
wrote:

I figured out what was going on with this PR and it's kinda interesting, so
forgive the long-winded explanation.

@ottobackwards https://github.com/ottobackwards is indeed right, it is
not apparent why his PR about a FieldTransformation and Stellar Function
would affect the YafIntegrationTest. Unfortunately, it became apparent when
it consistently failed on this PR in both travis and locally that something
was happening.

It turns out the problem is here
https://github.com/apache/incubator-metron/blob/master/metron-platform/metron-integration-test/src/main/config/zookeeper/global.json#L14.
The keen observer will notice in the global config we are ensuring that the
src_ip_addr and dst_ip_addr are valid IP addresses for the integration
tests. This, of course, is a typo (that I made :P ). The correct field
names are ip_src_addr and ip_dst_addr.

Now, you may ask:

How has this ever worked?
Given that it did work before, why is it broken now?

Well, for the first question, it was never correct in the integration
tests, but since we did not exercise the situation where we send in invalid
IP addresses. Furthermore, the default behavior for fields which do not
exist is to return true (probably wrong).

For the second question, @ottobackwards https://github.com/ottobackwards
has changed the default behavior for fields which do not resolve is to
return false (almost assuredly right).

For clarity to show how I go about debugging these problems, I'll be rather
explicit in how I figured out what was happening:

In my git branch, I added @ottobackwards
https://github.com/ottobackwards 's repo as a remote repo and checked
out the branch:
- git remote add otto
  https://github.com/ottobackwards/incubator-metron.git
- git checkout otto/METRON-424
- git branch METRON-424-debug
- git checkout METRON-424-debug
In Intellij, I ran the YafIntegrationTest to verify that it continued
to fail
I set breakpoints at the exit points of the ParserBolt:
It became apparent that we were exiting because the message was not
valid. I then set breakpoints inside of @ottobackwards
https://github.com/ottobackwards 's code and validated that it was
doing the right thing, but the fields were coming in as null, which
means the fields were not in the message.
I tracked the global.json down and looked at the field names and there
the bug became apparent.

I submitted a PR to your PR branch, @ottobackwards
https://github.com/ottobackwards, so if you merge that in, the build
should work (fingers crossed ;)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#303 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABD1_wxd18U5OE1jrhDTJjPWRGPJZJqTks5qzjV6gaJpZM4KTvIX
.

Fixed a long-standing misconfiguration in the tests

ottobackwards · 2016-10-13T14:33:56Z

Are you going to open a jira for that?

On October 13, 2016 at 10:16:13, Casey Stella (notifications@github.com)
wrote:

@nickwallen https://github.com/nickwallen Funny you mention that; yes, I
think we can. We should modify ParserIntegrationTest here
https://github.com/apache/incubator-metron/blob/4a4cb8b117dbb66bbfb4915bca9d871a06682c28/metron-platform/metron-parsers/src/test/java/org/apache/metron/parsers/integration/ParserIntegrationTest.java#L77
to check the invalid and error queues and dump their contents and fail
immediately.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#303 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABD1_w70G8oXifH31dkGUvkTkogQqJohks5qzj0sgaJpZM4KTvIX
.

cestella · 2016-10-13T14:48:46Z

Yep, I will. :)

cestella · 2016-10-13T15:48:23Z

Ok, so there is still an issue here. Some of the other parsers do not create an ip_src_addr and ip_dst_addr field in their test data (e.g. JSONMapParser). In those cases, the global.json used fails every message as invalid because the default behavior changed for the IP validation code.

As I see it, we have 2 choices:

Make the behavior of the IP validator return valid if the field doesn't exist.
Remove the IP validation check from the integration test global config
Force the various parsers to provide those fields.

I suggest 1 upon further consideration. Global validation across ALL parsers is going to have to be tolerant of missing values as not all parsers are going to provide every fields. I think it should be interpreted, "Are these fields globally valid if they exist?" rather than "are these fields globally valid?"

That being said, I think that the accommodation for this should exist in the FieldValidation function, not in the individual validator functions. I'll make a PR to your PR to this effect, but I wanted to make sure everyone was on board with that and gather thoughts.

cestella · 2016-10-13T16:10:16Z

Actually, upon even further consideration, I think we should revert the default behavior of the IP validator to return true if the field doesn't exist. Handling it in the FieldValidator means we cannot express a validation function that ensures the existence of a field across all parsers (e.g. timestamp).

Thoughts?

ottobackwards · 2016-10-13T16:13:05Z

I would prefer changing the IPValidator. As an aside to this, if someone were to want to add in new models to the system I would think Metron would need to have the validation mapped to model type and not so hard coded into the system. Some of this would fall out of that effort, unless I'm mistaken

cestella · 2016-10-13T16:17:51Z

Yeah, I agree. I'd like to have a sensor-specific validator as well as a global validator. Right now we have a MessageFilter, but it's slightly different in intent. It is intended to pick out which messages we want to pass through the parser and drop those that don't meet the criteria. I'd like to see all of these concepts merged into a mapping between stellar statements and actions (i.e. DROP, REDIRECT_TO_INVALID, REDIRECT_TO_ERROR) and have sensor specific as well as global versions of this.

cestella · 2016-10-13T16:26:34Z

That JIRA about making the ParserIntegrationTest better is at https://issues.apache.org/jira/browse/METRON-502

ottobackwards · 2016-10-13T16:27:23Z

I would like to get the discussion on that going, as adding in new models is something near and dear.

ottobackwards · 2016-10-13T16:27:59Z

I can't be assigned jiras but I'll take a stab at METRON-502, unless you have already completed it ;)

cestella · 2016-10-13T16:28:11Z

@ottobackwards Sure, why don't you start a DISCUSS thread and lay out how you envision that looking?

cestella · 2016-10-13T16:29:10Z

Go for it, man. It's all yours. :)

Changed ipvalidation to return true if the field doesn't exist.

cestella · 2016-10-14T14:40:10Z

+1 on inspection

METRON-424 ability to validate ip addresses against both IPV4 and IPV6

9064ec0

ottobackwards closed this Oct 11, 2016

ottobackwards reopened this Oct 11, 2016

kylerichardson mentioned this pull request Oct 11, 2016

METRON-363 Fix Cisco ASA Parser #276

Closed

ottobackwards closed this Oct 11, 2016

ottobackwards reopened this Oct 11, 2016

ottobackwards closed this Oct 12, 2016

ottobackwards reopened this Oct 12, 2016

ottobackwards closed this Oct 12, 2016

ottobackwards reopened this Oct 12, 2016

Fixed a long-standing misconfiguration in the tests

57801c1

Merge pull request #1 from cestella/METRON-424-debug

3095c1d

Fixed a long-standing misconfiguration in the tests

cestella and others added 2 commits October 13, 2016 12:56

Changed ipvalidation to return true if thefield doesn't exist.

5df0d3f

Merge pull request #2 from cestella/METRON-424-debug-rest-parsers

261e0b7

Changed ipvalidation to return true if the field doesn't exist.

asfgit closed this in 812fe75 Oct 17, 2016

ottobackwards deleted the METRON-424 branch October 17, 2016 14:10

iraghumitra mentioned this pull request May 22, 2024

[Snyk] Security upgrade http-proxy-middleware from 0.17.4 to 0.20.0 iraghumitra/incubator-metron#64

Open

METRON-424 ability to validate ip addresses against both IPV4 and IPV6 #303

METRON-424 ability to validate ip addresses against both IPV4 and IPV6 #303

Uh oh!

Conversation

ottobackwards commented Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonZeolla commented Oct 11, 2016

Uh oh!

cestella commented Oct 11, 2016

Uh oh!

nickwallen commented Oct 11, 2016

Uh oh!

ottobackwards commented Oct 11, 2016

Uh oh!

cestella commented Oct 11, 2016

Uh oh!

justinleet commented Oct 12, 2016

Uh oh!

ottobackwards commented Oct 12, 2016

Uh oh!

justinleet commented Oct 12, 2016

Uh oh!

nickwallen commented Oct 12, 2016

Uh oh!

nickwallen commented Oct 12, 2016

Uh oh!

ottobackwards commented Oct 12, 2016

Uh oh!

nickwallen commented Oct 12, 2016

Uh oh!

ottobackwards commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickwallen commented Oct 12, 2016

Uh oh!

cestella commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickwallen commented Oct 13, 2016

Uh oh!

cestella commented Oct 13, 2016

Uh oh!

ottobackwards commented Oct 13, 2016

Uh oh!

ottobackwards commented Oct 13, 2016

Uh oh!

ottobackwards commented Oct 13, 2016

Uh oh!

cestella commented Oct 13, 2016

Uh oh!

cestella commented Oct 13, 2016

Uh oh!

cestella commented Oct 13, 2016

Uh oh!

ottobackwards commented Oct 13, 2016

Uh oh!

cestella commented Oct 13, 2016

Uh oh!

cestella commented Oct 13, 2016

Uh oh!

ottobackwards commented Oct 13, 2016

Uh oh!

ottobackwards commented Oct 13, 2016

Uh oh!

cestella commented Oct 13, 2016

Uh oh!

cestella commented Oct 13, 2016

Uh oh!

cestella commented Oct 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ottobackwards commented Oct 11, 2016 •

edited

Loading

ottobackwards commented Oct 12, 2016 •

edited

Loading

cestella commented Oct 13, 2016 •

edited

Loading