Add IP-addresses and MAC-addresses to event #6878

hypp · 2018-04-17T07:42:29Z

This is a pull request for issue #5396

elasticmachine · 2018-04-17T07:42:30Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

ruflin

@hypp Thanks a lot for taking a stab at this. Code makes it much easier to discuss it.

I left a few comments in the code. Let's sort these out first.

Afterwards you will have to run make update to make sure the fields end up in the auto generate docs. Also please add a changelog entry and we will have to add a note to the docs. But the docs will depend on the conclusions of the comments.

ruflin · 2018-04-17T11:22:38Z

libbeat/processors/add_host_metadata/_meta/fields.yml

+            List of IP-addresses.
+        - name: net.hw
+          description: >
+            List of hardware-addresses, usually MAC-addresses.


I assume the type here would be keyword.

Ok, I'll fix that

ruflin · 2018-04-17T11:23:05Z

libbeat/processors/add_host_metadata/_meta/fields.yml

@@ -31,3 +31,10 @@
          type: keyword
          description: >
            OS family (e.g. redhat, debian, freebsd, windows).
+        - name: net.ip
+          description: >
+            List of IP-addresses.


We can use the type ip here: https://www.elastic.co/guide/en/elasticsearch/reference/6.2/ip.html

Yes, of course! I'll fix that too.

ruflin · 2018-04-17T11:24:38Z

libbeat/processors/add_host_metadata/add_host_metadata.go

@@ -71,10 +72,54 @@ func (p *addHostMetadata) loadData() {
 		if p.info.OS.Build != "" {
 			p.data.Put("host.os.build", p.info.OS.Build)
 		}
+
+		// IP-address and MAC-address
+		var ipList, hwList = p.getNetInfo()


I'm thinking to make this information optional, meaning it is not sent by default but we have a config option in the processor to enable it. Not sure yet how we should call the config options, any suggestions?

Perhaps
netinfo: enabled/disabled
or
add_netinfo: true/false?

Looking at the code I was even wondering if we should separate mac address and ip. I think lots of people want to have the list of ip addresses but are less interested in the actual mac address. This could lead to a config like:

add_fields: [`ip`, `mac`]

I don't like add_fields name to much but the gist is that we could add more fields in the future here.

@andrewkroh Thoughts?

I think this a good idea, because without the feature you would then have to add a drop_fields processor causing unnecessary work.

I'd have a default set of fields built into the processor's defaultConfig. Then if you want to change anything you must specific the full set of fields you want the process to add.

Should we do that for all fields or just for ip and mac?

@andrewkroh If I understand this correctly, my config example above would mean host.name for example would not be sent?

add_fields: [ip, mac]

Correct. That would give you only those two fields. But I wouldn't call it add_fields if we went with this behavior.

My main issue going with the above suggestion is that if someone wants ip and mac in addition, he has to know all the other fields that are there to add them to the list.

To not block the PR on this discussion perhaps we can go first with the initial suggestion of netinfo.enabled: true and continue the field list discussion separately as I think such a logic would have to be applied to all add_ processors.

I have time to work on this on friday, so it would be great to have a decision by then.

@hypp We had a quick internal chat and decided to go with the proposal for netinfo.enabled for now and we can still improve it later.

ruflin · 2018-04-17T11:25:55Z

libbeat/processors/add_host_metadata/add_host_metadata.go

+
+		// IP-address and MAC-address
+		var ipList, hwList = p.getNetInfo()
+		p.data.Put("host.net.ip", ipList)


I suggest to call these fields host.ip and host.mac.

Fine by me!

houndci-bot · 2018-04-27T15:09:04Z

libbeat/processors/add_host_metadata/config.go

@@ -0,0 +1,16 @@
+package add_host_metadata


don't use an underscore in package name

ruflin

Change LGTM. To make CI happy you need to run make fmt first to format the Golang files and then make update in the metricbeat directory to generate the docs with the new fields.

ruflin · 2018-04-30T06:39:22Z

libbeat/processors/add_host_metadata/add_host_metadata_test.go

+	assert.Error(t, err)
+	assert.Nil(t, v)
+
+	event = &beat.Event{


Could you split this up into 2 tests? Like this if the tests fail we can already tell based on the name which part is probably not working.

@ruflin Is it really enough to make update in the metricbeat directory? Or should I do it for every beat?

Sorry, my mistake. You have to run it in the top level beats directory. Same for make fmt.

ruflin

I had again a look at the code. One question that got raised on my end is what is the behaviour in case of errors. Do we continue trying to get the information or do we abort?

Could you update the two following files with the new config option and run make update again?

reference config file: https://github.com/elastic/beats/blob/master/libbeat/_meta/config.reference.yml#L186
asciidoc: https://github.com/elastic/beats/blob/master/libbeat/docs/processors-using.asciidoc#add-host-metadata

Sorry for the additional iteration but I think we are close to get it in.

ruflin · 2018-05-02T06:27:13Z

libbeat/processors/add_host_metadata/add_host_metadata.go

+	// Get all interfaces and loop through them
+	ifaces, err := net.Interfaces()
+	if err != nil {
+		return ipList, hwList


Instead or returning the empty lists could we return here the error? Meaning it would return nil, nil, err. On line 89 we could then check for the error and would not add the fields in case of an error and could also log the error.

Sure, do you have a code example of logging, that I could look at?

ruflin · 2018-05-02T06:28:49Z

libbeat/processors/add_host_metadata/add_host_metadata.go

+
+		addrs, err := i.Addrs()
+		if err != nil {
+			return ipList, hwList


Similar question as above. I wonder if in case of an error we should just log it but continue trying to collect the addresses from the interfaces instead of returning.

It is a matter of taste, I prefer to fail fast. You decide.

ruflin · 2018-05-02T06:29:28Z

libbeat/processors/add_host_metadata/add_host_metadata.go

+		if p.config.NetInfoEnabled {
+			// IP-address and MAC-address
+			var ipList, hwList = p.getNetInfo()
+			p.data.Put("host.ip", ipList)


Reviewing the code below and the possibility that ipList and hwList can be empty, we should check for len(x) == 0 and in case of 0 not add the field.

Ok, I'll fix that

ruflin · 2018-05-02T06:29:57Z

libbeat/processors/add_host_metadata/config.go

@@ -0,0 +1,16 @@
+package add_host_metadata
+
+//import (


ruflin

I like your current implementation that if one address fetching fails we still continue like this the user still has the chance to get some data.

ruflin · 2018-05-03T06:47:46Z

libbeat/docs/processors-using.asciidoc

+processors:
+- add_host_metadata:
+    netinfo.enabled: false
+


Here the end of the source block seems to be missing.

Should be fixed now

ruflin · 2018-05-03T06:49:47Z

libbeat/processors/add_host_metadata/add_host_metadata.go

+			// IP-address and MAC-address
+			var ipList, hwList, err = p.getNetInfo()
+			if err != nil {
+				logp.Warn("Error when getting network information %v", err)


We prefer not to use Warn as it's not clear if this is something that needs action. I suggest we set it to Info for now.

ruflin · 2018-05-03T06:51:57Z

libbeat/processors/add_host_metadata/add_host_metadata.go

+	// Get all interfaces and loop through them
+	ifaces, err := net.Interfaces()
+	if err != nil {
+		return ipList, hwList, err


Can you use return nil, nil, err here? This makes it clear that both ipList and hwList do not have any values yet. Code logic is exactly the same as before.

ruflin · 2018-05-03T06:54:36Z

libbeat/processors/add_host_metadata/add_host_metadata.go

+		addrs, err := i.Addrs()
+		if err != nil {
+			// If we get an error, log it and continue with the next interface
+			logp.Warn("Error when getting IP address %v", err)


Lets not log each error but add it to the list. We can use multierror here. Outside the loop you can specify var errs multierror.Errors and then use errs = append(errs, err). On line 145 you can then return errs.Err().

ruflin · 2018-05-04T10:33:45Z

jenkins, test this

ruflin · 2018-05-04T13:00:32Z

@hypp Thanks a lot for your contribution.

hypp · 2018-05-05T07:56:03Z

@ruflin So it is all done now? Great!

ruflin · 2018-05-08T08:19:01Z

@hypp Yes. This should automatically end up in the 6.4 release. If you want to test it earlier, there are snapshot build available: https://beats-package-snapshots.s3.amazonaws.com/index.html?prefix=metricbeat/ (warning, just saw they are not 100% up-to-date. Need to check why).

hypp added 3 commits April 16, 2018 20:51

added ip and hw addresses

215b308

added new test case for ip and hw addresses

06c1a3d

added description of net.ip and net.hw fields

bf696b1

ruflin changed the title ~~Issues/5396~~ Add IP-addresses and MAC-addresses to event Apr 17, 2018

ruflin reviewed Apr 17, 2018

View reviewed changes

ruflin added in progress Pull request is currently in progress. libbeat :Processors labels Apr 17, 2018

hypp added 4 commits April 17, 2018 19:34

added types to fields

70bf057

added changelog

5bd56a1

rename to host.mac and host.ip

5538abf

Added configuration option netinfo.enabled for IP and MAC

65463ab

houndci-bot reviewed Apr 27, 2018

View reviewed changes

libbeat/processors/add_host_metadata/config.go

@@ -0,0 +1,16 @@

package add_host_metadata

Copy link

houndci-bot Apr 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use an underscore in package name

Explicitly set NetInfoEnabled to false

3e438f8

ruflin reviewed Apr 30, 2018

View reviewed changes

hypp added 3 commits April 30, 2018 08:59

after make fmt

959e2f6

split testcase into two

5c20a52

after make update

df58825

ruflin reviewed May 2, 2018

View reviewed changes

hypp added 5 commits May 2, 2018 17:38

updated docs

1b09cd6

after make update

1cba03f

yet another make update

dfef568

removed left over comment

1ef240d

added logging on error

7c44e2a

ruflin reviewed May 3, 2018

View reviewed changes

hypp added 4 commits May 3, 2018 21:54

use Info instead of Warn

e909f81

added missing end of source

dde24f7

return nil, nil instead of ipList, hwList

993b2bc

keep track of all errors using multierror

014f5d5

ruflin approved these changes May 4, 2018

View reviewed changes

ruflin removed the in progress Pull request is currently in progress. label May 4, 2018

ruflin merged commit 8621db3 into elastic:master May 4, 2018

ruflin mentioned this pull request Jun 27, 2018

Add IP-addresses and MAC-addresses to event #5396

Closed

ph mentioned this pull request Aug 27, 2018

Panic 'fatal error: concurrent map iteration and map write' autodiscover and processor. #8040

Closed

Add IP-addresses and MAC-addresses to event #6878

Add IP-addresses and MAC-addresses to event #6878

Conversation

hypp commented Apr 17, 2018

elasticmachine commented Apr 17, 2018

ruflin left a 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

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

Choose a reason for hiding this comment

ruflin left a 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

ruflin left a 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

ruflin left a 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

Choose a reason for hiding this comment

ruflin commented May 4, 2018

ruflin commented May 4, 2018

hypp commented May 5, 2018

ruflin commented May 8, 2018