GitHub-issue#253 : Implemented GeoIP processor functionality

[venkataraopasyavula]

Description

Implemented GeoIP processor functionality

Issues Resolved

GitHub-issue #253

Check List

• New functionality includes testing.
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[kkondaka]

It should be "tags_on_geoip_source_not_found". We should not use same tag for all processor. You should also have this in the GeoIP config. The YAML config option can be "tags_on_source_not_found" but the actual tag should have "geoip" in it like I suggested above.

[venkataraopasyavula]

Added List tagsOnSourceNotFoundFailure in YAML configuration and necessary changes in respective classes and Readme.MD

[kkondaka]

I think you should do this before it is used in the "synchronized" block, right?

[venkataraopasyavula]

The downloadReady should be false until download successful. The flag will become true once download successful in downloadThroughURLandS3()

[kkondaka]

The downloadReady is not initialized, right? Is it not possible while (!downloadReady()) executed before it is initialized to false?

[kkondaka]

Since togge = !toggle is done in both cases, if can be done outside of if-else block.

[venkataraopasyavula]

Moved the line "togge = !toggle" above if and else block.

[kkondaka]

Is this right? You are using a public static variable directly here?

[venkataraopasyavula]

Yes, the downloadReady should be static to access in without creating instance. Checking this condition because enrichment should not happen until mmdb download completes.

[kkondaka]

Same thing here. I don't think this is correct approach.

[venkataraopasyavula]

Yes, the downloadReady should be static to access in without creating instance. Checking this condition because enrichment should not happen until mmdb download completes.

[kkondaka]

Do we need this? Can we not use ``InetAddress.isSiteLocalAddress()`?

See http://docs.oracle.com/javase/1.5.0/docs/api/java/net/InetAddress.html#isSiteLocalAddress().

[venkataraopasyavula]

Updated the code with InetAddress.isSiteLocalAddress.

[oeyh]

Thanks for the PR! A few comments below:

[oeyh]

Need to test that events have been enriched with Geo data here.

[venkataraopasyavula]

Already tested in GetGeoLite2DataTest file in two methods.
1.getGeoDataTest_without_attributes()
2.getGeoDataTest_with_attributes().

[oeyh]

I'm looking for assertions like the following added here. This is to test that the prepared Geo data actually gets into the event:

assertThat(event.get("location/country_name", String.class), equalTo("United States"));
assertThat(event.get("location/timezone", String.class), equalTo("America/Chicago"));
... ...

[venkataraopasyavula]

This test case is to read the IP from the input JSON file based on the key present in YAML configuration. For example the path for reading the IP is peer/ip. Please find the below sample json input:

"peer" : {
"ip" : "1.2.3.4"
"host" : "example.org"
}
"status" : "success"

There is no enrichment in this test testcase to add assertions for country_name and timezone.
Enrichment related are tested in GetGeoLite2DataTest file in two methods.
1.getGeoDataTest_without_attributes()
2.getGeoDataTest_with_attributes()

[oeyh]

Thanks for addressing other comments! Looks pretty good other than this one:

There is no enrichment in this test testcase.

The test case is testing GeoIPProcessor's doExecute method, which enriches the events with GeoIP data. And we want to verify that the events are actually enriched. This is a standard test for processors, this one is an example.

Something like this should work:

Suggested change

	assertThat(event.get("/peer/ip", String.class), equalTo("136.226.242.205"));
	assertThat(event.get("/peer/ip", String.class), equalTo("136.226.242.205"));
	assertThat(event.get("location/country_name", String.class), equalTo("United States"));
	assertThat(event.get("location/timezone", String.class), equalTo("America/Chicago"));
	assertThat(event.get("location/country_IsoCode", String.class), equalTo("US"));
	assertThat(event.get("location/continent_name", String.class), equalTo("North America"));

[venkataraopasyavula]

doExecute() is common method for all the functionality to read IP from input json, calling enrichment method and updated records with enriched data.
The Enrichment related are tested in GetGeoLite2DataTest file in two methods.
1.getGeoDataTest_without_attributes()
2.getGeoDataTest_with_attributes()

[oeyh]

I understand that doExecute() indeed includes call to getGeoData(), which have been fully tested and are being mocked in this doExecuteTest(). But as you said, there's more than just getGeoData() in doExecute(), which is why we need to assert the update event here. Specifically, this line that adds geo data to the event:

eventRecord.getData().put(key.getKeyConfig().getTarget(), geoData);

Imagine this line is missing, doExecuteTest() should catch it. But in its current form, it won't.

[oeyh]

Approving so we can move forward.

[kkondaka]

Approving and merging to make progress. I would still want you to address the comment downloadReady flag and also global usage of this flag.