Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from
May 4, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
215b308
added ip and hw addresses
hypp Apr 16, 2018
06c1a3d
added new test case for ip and hw addresses
hypp Apr 16, 2018
bf696b1
added description of net.ip and net.hw fields
hypp Apr 16, 2018
70bf057
added types to fields
hypp Apr 17, 2018
5bd56a1
added changelog
hypp Apr 17, 2018
5538abf
rename to host.mac and host.ip
hypp Apr 20, 2018
65463ab
Added configuration option netinfo.enabled for IP and MAC
hypp Apr 27, 2018
3e438f8
Explicitly set NetInfoEnabled to false
hypp Apr 27, 2018
959e2f6
after make fmt
hypp Apr 30, 2018
5c20a52
split testcase into two
hypp Apr 30, 2018
df58825
after make update
hypp Apr 30, 2018
1b09cd6
updated docs
hypp May 2, 2018
1cba03f
after make update
hypp May 2, 2018
dfef568
yet another make update
hypp May 2, 2018
1ef240d
removed left over comment
hypp May 2, 2018
7c44e2a
added logging on error
hypp May 2, 2018
e909f81
use Info instead of Warn
hypp May 3, 2018
dde24f7
added missing end of source
hypp May 3, 2018
993b2bc
return nil, nil instead of ipList, hwList
hypp May 3, 2018
014f5d5
keep track of all errors using multierror
hypp May 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions libbeat/processors/add_host_metadata/_meta/fields.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,10 @@
type: keyword
description: >
OS family (e.g. redhat, debian, freebsd, windows).
- name: net.ip
description: >
List of IP-addresses.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

- name: net.hw
description: >
List of hardware-addresses, usually MAC-addresses.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the type here would be keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll fix that


45 changes: 45 additions & 0 deletions libbeat/processors/add_host_metadata/add_host_metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package add_host_metadata

import (
"net"
"time"

"github.com/elastic/beats/libbeat/beat"
Expand Down Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

p.data.Put("host.net.ip", ipList)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me!

p.data.Put("host.net.hw", hwList)

p.lastUpdate = time.Now()
}
}

func (p addHostMetadata) getNetInfo() ([]string, []string) {
var ipList []string
var hwList []string

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
for _, i := range ifaces {
// Skip loopback interfaces
if i.Flags&net.FlagLoopback == net.FlagLoopback {
continue
}

hw := i.HardwareAddr.String()
// Skip empty hardware addresses
if hw != "" {
hwList = append(hwList, hw)
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
for _, addr := range addrs {
switch v := addr.(type) {
case *net.IPNet:
ipList = append(ipList, v.IP.String())
case *net.IPAddr:
ipList = append(ipList, v.IP.String())
}
}
}

return ipList, hwList
}

func (p addHostMetadata) String() string {
return "add_host_metadata=[]"
}
8 changes: 8 additions & 0 deletions libbeat/processors/add_host_metadata/add_host_metadata_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,12 @@ func TestRun(t *testing.T) {
v, err := newEvent.GetValue("host.os.family")
assert.NoError(t, err)
assert.NotNil(t, v)

v, err = newEvent.GetValue("host.net.ip")
assert.NoError(t, err)
assert.NotNil(t, v)

v, err = newEvent.GetValue("host.net.hw")
assert.NoError(t, err)
assert.NotNil(t, v)
}