Fixes #91 - Allow using the ISP database #94

imriz · 2016-08-07T14:10:24Z

No description provided.

tegud · 2016-11-17T15:31:04Z

Is there any idea on when this will be merged in? No support for ISP lookup is a pretty major issue for us upgrading to 5.

jordansissel · 2016-11-17T17:05:12Z

@tegud well, it needs to be reviewed before it will be merged. :)

tegud · 2016-11-18T14:21:00Z

I'm happy to review if that's acceptable? :)

jordansissel · 2016-11-17T17:08:32Z

lib/logstash/filters/geoip.rb

    end
+    geo_data_hash.reject! {|key, value| !@fields.include? key} unless @fields.empty?


This mechanism of building a hash and then removing fields that are not selected is doing extra work that could be avoided.

The current version of this file selectively adds fields based on @fields -- is there a reason you rewrote it in this way?

Yes. This is a more generic way. The docs state that if you leave fields
empty, you will get all fields - that was not the case. Previously, if you
left it empty, you would have gotten what the plugin has in the default
list (nit picking - the doc states there is no default for the fields parameter, and there is a default value).
Putting aside the fact it is not semantically the same, it would mean
we need to maintain a list for every type of DB, plus running multiple
calls to the DB wrapper to get specific fields. This way, you make one
call, don't need to maintain a list of default fields, and the semantics
match the docs better.

I haven't benchmarked it, but I think this might even be faster for use cases where the user wants all the fields.

jordansissel · 2016-11-21T21:54:44Z

@tegud All are welcome to participate in whatever means they feel comfortable. So, yes, you are welcome to review. :)

addrians · 2016-12-16T13:43:36Z

lib/logstash/filters/geoip.rb

        geo_data_hash["longitude"] = location.getLongitude()
      else
-        raise Exception.new("[#{field}] is not a supported field option.")
-      end
+        geo_data_hash.merge!(JSON.parse(response.toJson()))


require "json" is needed for this call

It actually works without it. I guess it is loaded implicitly somehow?

I cherry-picked your code into the master branch, so my conditions are different. I get :exception=>#<NameError: uninitialized constant LogStash::Filters::GeoIP::JSON> at this line without including the json module.
Anyway, I think we should work on getting the PR to work against the mater branch.

Unfortunately, I don't know when I will be able to do that. If someone wants to continue from here, fine, if not, I will try to find a time for it in my next sprint.

There should be a better way to get the things out of the response than having it parse some immediately-generated JSON.

@jordansissel I am not aware of a better way. The alternative would be to either hard code each field method (e.g. getAutonomousSystemOrganization()) for every possible DB, or use some reflection hacks. toJson() seems like MaxMind's opinionated way of getting all the fields. This resembles what you will get from their API.
The obvious benefit is that we don't need to be aware of the possible fields in advance, and if they add more fields, we will seamlessly support it.

sh4t · 2017-01-14T03:51:49Z

Is this going to allow us to use the ISP databse (paid sub) from mm?

sh4t · 2017-01-18T23:54:31Z

I'm happy to test this in my environment as well.

SMAPPER · 2017-01-29T16:01:20Z

Any idea when this will this will be supported? This is the number one reason some of my clients cannot upgrade to version 5. The ability to use the ISP database is highly important for security logs as well as for filtering. I have to keep telling people to hold off on going to 5 as many of their visualizations and dashboards are dependent on the ISP database support. I appreciate your help on this and would also be willing to test.

jjm · 2017-02-23T16:31:47Z

I would also really like to see this support logstash plugin, in fact just raised a Elastic support request about how is it possible to use this database.

jordansissel · 2017-03-03T22:11:13Z

lib/logstash/filters/geoip.rb

@@ -88,6 +86,12 @@ class LogStash::Filters::GeoIP < LogStash::Filters::Base
  # is still valid GeoJSON.
  config :target, :validate => :string, :default => 'geoip'

+  # Specify the database type. By default "city" is selected (This is the type of the built-in GeoLite2 City database)
+  config :dbtype, :validate => ['country', 'city', 'anonymousIp', 'connectionType', 'domain', 'enterprise', 'isp'  ], :default => 'city'


Since the database file seems to infer the database type, can we avoid this setting and use the database file to determine what the database type is?

This inference is what the geoip plugin did before we upgraded to geoip2:

https://github.com/logstash-plugins/logstash-filter-geoip/blob/v2.0.0/lib/logstash/filters/geoip.rb#L104-L115

Good idea. I will change.

I looked into it. The geoip gem is doing some low level logic to understand the type (https://github.com/cjheath/geoip/blob/master/lib/geoip.rb#L546) and as far as I can tell, this is not possible with the Java SDK. In the Java SDK, you can get the database type via the metadata, but I couldn't find a full list of possible values for the databases_type field (https://maxmind.github.io/MaxMind-DB/). Therefore, I am leaving it as is for now, unless someone can provide me with a full list of values, or a better way to detect it.

jordansissel · 2017-03-03T22:58:52Z

lib/logstash/filters/geoip.rb

+  config :dbtype, :validate => ['country', 'city', 'anonymousIp', 'connectionType', 'domain', 'enterprise', 'isp'  ], :default => 'city'
+
+  # Should we merge the results into target. If set to false, we will override the value of the target field
+  config :merge, :validate => :boolean, :default => false


The behavior prior to geoip2 was merging by default with no option to turn it off (reference :https://github.com/logstash-plugins/logstash-filter-geoip/blob/v2.0.0/lib/logstash/filters/geoip.rb#L147-L161)

Should this default to true?

Will change

jordansissel · 2017-03-03T22:59:29Z

lib/logstash/filters/geoip.rb

-    event[@target] = geo_data_hash
+    if @merge
+      event[@target] ||= {}
+      event[@target].merge!(geo_data_hash)


.merge! is not a safe thing to do on a Logstash Event. You should insert each key separately.

OK. Out of curiosity - why isn't it safe?

Because the api for accessing an event is only available as 'event.getandevent.set`. Getting a value, then modifying that value, instead of using just 'get' and 'set' APIs is not supported and not guaranteed to work correctly.

jordansissel · 2017-03-03T23:01:58Z

lib/logstash/filters/geoip.rb

        geo_data_hash["longitude"] = location.getLongitude()
      else
-        raise Exception.new("[#{field}] is not a supported field option.")
-      end
+        geo_data_hash.merge!(JSON.parse(response.toJson()))


There should be a better way to get the things out of the response than having it parse some immediately-generated JSON.

s1sfa · 2017-03-14T19:48:15Z

I received error while trying to use it in 5: "Direct event field references (i.e. event['field']) have been disabled in favor of using event get and set methods (e.g. event.get('field')). Please consult the Logstash 5.0 breaking changes documentation for more details"

After this change is approved/merged the same change can be applied to plugin-api-v2 just updating the event references? Or maybe different ways that other things should be done too?
It's pretty easy to change the event references to .set or .get.
I changed the "if @merge" section a bit to make it work there is no event.merge().

    if @merge
      merge_event = event.get([@target])
      merge_event ||= {}
      merge_event.merge!(geo_data_hash)
      event.set(@target,merge_event)
    else
      event.set(@target,geo_data_hash)
    end

Seems to work great in 5.2.2 after only this change and updating the various other event references to .set or .get.

Thanks for this so far @imriz !

suyograo · 2017-04-20T00:17:13Z

@imriz thanks for this contribution! For a long time, I wanted to move GeoIP to pure Java since we already use the Java APIs (and not the ruby code). I decided to do this and took a stab at supporting ISP database in the same PR -- #111.

#111 could supersede this PR, if you are ok with it.

imriz · 2017-04-20T07:34:17Z

@suyograo I have no objections. My 2 cents are that the new commit again follows the path of using explicit methods to fetch explicit fields. This will require changing code if MaxMind add fields. I find this rather limiting and not really required.
Also, until the documentation is amended, it is also inconsistent with how the default behaviour is described - it says that if you leave it empty, you get all the fields, and that is not the case.

imriz mentioned this pull request Aug 7, 2016

Add support for GeoIP2-ISP database #91

Closed

imriz force-pushed the plugin-api-v1 branch 2 times, most recently from ff32cea to 2f1b28e Compare August 8, 2016 06:42

Fixes logstash-plugins#91 - Allow using the ISP database

42e7d27

imriz force-pushed the plugin-api-v1 branch from 2f1b28e to 42e7d27 Compare August 8, 2016 15:22

jordansissel reviewed Nov 21, 2016

View reviewed changes

addrians reviewed Dec 16, 2016

View reviewed changes

jordansissel suggested changes Mar 3, 2017

View reviewed changes

suyograo closed this May 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #91 - Allow using the ISP database #94

Fixes #91 - Allow using the ISP database #94

imriz commented Aug 7, 2016

tegud commented Nov 17, 2016

jordansissel commented Nov 17, 2016

tegud commented Nov 18, 2016

jordansissel Nov 17, 2016

imriz Nov 22, 2016 •

edited

Loading

jordansissel commented Nov 21, 2016

addrians Dec 16, 2016

imriz Dec 19, 2016

addrians Dec 20, 2016

imriz Dec 20, 2016

jordansissel Mar 3, 2017

imriz Mar 8, 2017

sh4t commented Jan 14, 2017

sh4t commented Jan 18, 2017

SMAPPER commented Jan 29, 2017

jjm commented Feb 23, 2017

jordansissel Mar 3, 2017

imriz Mar 8, 2017

imriz Mar 8, 2017

jordansissel Mar 3, 2017

imriz Mar 8, 2017

jordansissel Mar 3, 2017

imriz Mar 8, 2017

jordansissel Apr 17, 2017

jordansissel Mar 3, 2017

s1sfa commented Mar 14, 2017

suyograo commented Apr 20, 2017

imriz commented Apr 20, 2017

		end
		geo_data_hash.reject! {\|key, value\| !@fields.include? key} unless @fields.empty?

Fixes #91 - Allow using the ISP database #94

Fixes #91 - Allow using the ISP database #94

Conversation

imriz commented Aug 7, 2016

tegud commented Nov 17, 2016

jordansissel commented Nov 17, 2016

tegud commented Nov 18, 2016

Choose a reason for hiding this comment

imriz Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

jordansissel commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sh4t commented Jan 14, 2017

sh4t commented Jan 18, 2017

SMAPPER commented Jan 29, 2017

jjm commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1sfa commented Mar 14, 2017

suyograo commented Apr 20, 2017

imriz commented Apr 20, 2017

imriz Nov 22, 2016 •

edited

Loading