-
Notifications
You must be signed in to change notification settings - Fork 75
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
ECS compatibility #165
ECS compatibility #165
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a question on testing part plus a doubt:
- should we also set the
ecs.version
as described in guidelines ?
dda27f7
to
28e85bf
Compare
Unfortunately, we don't have minor/patch versions available to us, so we can't comply :/ |
0fdff63
to
9bc1c4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
do have some suggestion on meta-data field names
@@ -1,7 +1,7 @@ | |||
# encoding: utf-8 | |||
require 'java' | |||
|
|||
class DecoderImpl | |||
class LogStash::Inputs::Tcp::DecoderImpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💛
lib/logstash/inputs/tcp.rb
Outdated
decorate(event) | ||
@output_queue << event | ||
end | ||
|
||
# setup the field names, with respect to ECS compatibility. | ||
def setup_fields! | ||
@field_host = ecs_select[disabled: "host", v1: "[@metadata][tcp][source][name]" ].freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about using source.address
instead
so the underlying structure could be copied to a top-level and would be ecs-compliant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few reasons that make wholesale copy of this source metadata impractical.
While source
and destination
fields do have an address
, ip
, and port
, they are used to describe network transactions captured in an event. Similarly, client
and server
have all three fields, but are used to describe the initiator or recipient of the network transaction.
While the name and ip of the event's source may make sense to copy into host
or observer
, the port
is pretty meaningless and doesn't have a home there. When this plugin is run in server mode, the outbound port from the client is typically chosen at random by its host OS, and when the plugin is run in client mode, the port we are connecting to doesn't really describe the event.
I chose name
here because it aligns with both host
and observer
.
We could align with the CEF codec, which has a new device
option that accepts host
or observer
, and could populate the relevant bits if this is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for sharing the details - fine by me to keep as is - prefixed source.name
lib/logstash/inputs/tcp.rb
Outdated
@field_host = ecs_select[disabled: "host", v1: "[@metadata][tcp][source][name]" ].freeze | ||
@field_host_ip = ecs_select[disabled: "[@metadata][ip_address]", v1: "[@metadata][tcp][source][ip]" ].freeze | ||
@field_port = ecs_select[disabled: "port", v1: "[@metadata][tcp][source][port]" ].freeze | ||
@field_proxy_host = ecs_select[disabled: "proxy_host", v1: "[@metadata][tcp][proxy][ip]" ].freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess we do not have a better (ecs-like) names for proxy.host
and proxy.ip
here ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM left only a few nitpicks that you are free to skip
Also fixes a missing ip address of source when a TCP Proxy is present.
9bc1c4e
to
4b53619
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions inline for your consideration.
docs/index.asciidoc
Outdated
The value of this setting affects the placement of a TCP connection's metadata on events: | ||
|
||
.Source Metadata Location by `ecs_compatibility` value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of this setting affects the placement of a TCP connection's metadata on events: | |
.Source Metadata Location by `ecs_compatibility` value | |
The value of this setting affects the placement of a TCP connection's metadata on events. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On other plugins we've standardized putting the metadata table in the ECS metadata section. There's a compelling argument for either location. If you don't have strong feelings, perhaps we could move this under the ECS metadata section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job on the table. It renders quite nicely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved the table, and re-worded the relevant links/intros.
Co-authored-by: Karol Bucek <karol.bucek@elastic.co> Co-authored-by: Karen Metts <karen.metts@elastic.co>
9e09de5
to
fe8ec28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to removed numbered table title. Otherwise, LGTM! 🎉
remove numbered table header Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
Implements ECS Compatibility mode, supporting
disabled
(legacy), opt-inv1
(breaking), and an alias fromv8
tov1
.In ECS mode, we add tcp connection metadata in a variety of descriptive fields under
[@metadata][tcp]
, instead of the top-level fields that clash with ECS. Users can copy these metadata fields to their events if and when necessary, which allows us to avoid polluting an event with data that may or may-not align with ECS.resolves #146