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.

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

Rename source field in Filebeat #8902

Merged
merged 1 commit into from
Nov 7, 2018

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Nov 2, 2018

The source field in Filebeat was used to store the file path for logs or the source ip for syslog, udp, tcp input. As source is in ECS an object the fields are now moved to ECS pattern.

  • For UDP, TCP, syslog input the source field is converted to log.source.ip
  • For the log input the source field is converted to log.file.path

Done:

  • Test files updated
  • Changelog updated
  • Migration file updated
  • source removed from fields.yml, two new fields added

@ruflin ruflin added in progress Pull request is currently in progress. Packetbeat Filebeat Filebeat Auditbeat ecs labels Nov 2, 2018
@@ -74,7 +74,9 @@ func NewInput(
},
Fields: common.MapStr{
"message": string(data),
"source": metadata.RemoteAddr.String(),
"source": common.MapStr{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ph Does this make sense for udp, tcp and syslog?

@ruflin
Copy link
Contributor Author

ruflin commented Nov 5, 2018

@ph @andrewkroh @webmat I actually have second thoughts here about the proposed fields. file.path and source.ip can both show up in the log files itself. What if the apache logs are sent through syslog and both have source.ip.

The file.path and source.ip are meta information about a log event. Because of this I propose log.file.path and log.source.ip. Both are not ECS fields but follow the naming patterns.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

I agree with nesting this information under log.. Without it, you're right that there's a conflict with this meta information about the source of the event, and the event content itself.

And I also like that you're keeping the full name of log.file.path. It opens up the possibility to fill up more of the file fields under there with log file metadata (e.g. log.file.owner & so on), if someone ever needs this.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Let me revert that back to "Comment" from "Approve". Of course the tests need to be fixed :-)

But I like the proposed field names as they are.

@ph
Copy link
Contributor

ph commented Nov 5, 2018

@ruflin proposed changes LGTM

@webmat
Copy link
Contributor

webmat commented Nov 5, 2018

Since this is one field being split in two locations (file path vs IP), we can't list it in dev-tools/ecs-migration.yml, correct?

@ruflin
Copy link
Contributor Author

ruflin commented Nov 5, 2018

@webmat I'll figure out a way to list it

@ruflin ruflin changed the title [WIP] Convert source field in Filebeat Convert source field in Filebeat Nov 5, 2018
@ruflin ruflin added review and removed in progress Pull request is currently in progress. labels Nov 5, 2018
@ruflin ruflin force-pushed the convert-source-filebeat branch 2 times, most recently from 7885f48 to cb16cbf Compare November 6, 2018 07:25
@ruflin ruflin mentioned this pull request Nov 6, 2018
@ruflin ruflin force-pushed the convert-source-filebeat branch from cb16cbf to f7b8647 Compare November 6, 2018 08:46
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Thanks for making this change. Left only minor comments.

@@ -20,6 +20,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
- Use `initial_scan` action for new paths. {pull}7954[7954]

*Filebeat*
- Move source to log.file.path and log.source.ip
Copy link
Member

Choose a reason for hiding this comment

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

I'd use "Rename" rather than "Move".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

type: ip
required: false
description: >
Source ip from which the log event was read / sent from.
Copy link
Member

Choose a reason for hiding this comment

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

s/ip/IP/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

Two minor comments

@@ -20,7 +20,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
- Use `initial_scan` action for new paths. {pull}7954[7954]

*Filebeat*

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You've removed the empty line between the header (Filebeat) and the list of changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix this in a follow up PR

@@ -16,6 +16,7 @@
"http.request.method": "GET",
"http.response.status_code": "200",
"input.type": "log",
"log.file.path": "/Users/ruflin/Dev/gopath/src/github.com/elastic/beats/x-pack/filebeat/module/suricata/eve/test/eve-alerts.log",
"log.offset": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

As this field is ignored by test_modules.py, can we get rid of this entries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Will need to investigate. Will also tackle this in one of the other PR's that is conflicting with this one as soon as it's merged if that is ok with you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got lucky, have to rebase now anyways. Will fix these issues.

@ruflin ruflin force-pushed the convert-source-filebeat branch from 13db3e9 to a1437a9 Compare November 6, 2018 15:18
@ruflin ruflin changed the title Convert source field in Filebeat Rename source field in Filebeat Nov 6, 2018
@ruflin ruflin force-pushed the convert-source-filebeat branch from a1437a9 to f1cdd06 Compare November 6, 2018 15:20
The source field in Filebeat was used to store the file path for logs or the source ip for syslog, udp, tcp input. As source is in ECS an object the fields are now moved to ECS pattern.

* For UDP, TCP, syslog input the source field is converted to log.source.ip
* For the log input the source field is converted to log.file.path

Done:

* Test files updated
* Changelog updated
* Migration file updated
* `source` removed from fields.yml, two new fields added
@ruflin ruflin force-pushed the convert-source-filebeat branch from f1cdd06 to b659e10 Compare November 6, 2018 18:30
@ruflin
Copy link
Contributor Author

ruflin commented Nov 7, 2018

Ready for an other round of reviews. Failing tests should not be related.

Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

LGTM

@ruflin ruflin merged commit 0e42349 into elastic:master Nov 7, 2018
@ruflin ruflin deleted the convert-source-filebeat branch November 7, 2018 11:52
ruflin added a commit to ruflin/beats that referenced this pull request Dec 12, 2018
Related to elastic#8902 but adding the fields instead of replacing
ruflin added a commit that referenced this pull request Dec 12, 2018
…9435)

Related to #8902 but adding the fields instead of replacing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants