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

Update DHCPv4 protocol to use ECS fields #10089

Merged
merged 2 commits into from
Jan 17, 2019

Conversation

andrewkroh
Copy link
Member

The DHCPv4 protocol dataset works on uni-flows (it's not transacted, see #7956) so
the source and destination will indicate the original packet header data. Meanwhile
the client / server fields are copies of the source/destination, but they are
copied based on which side is the client and server.

Here's a summary of what fields changed.

Part of #7968

Changed

  • bytes_in -> source.bytes
  • transport -> network.transport = udp

Added

  • source
  • destination
  • event.dataset = dhcpv4
  • event.start
  • network.bytes
  • network.community_id
  • network.protocol = dhcpv4
  • network.type

Unchanged Packetbeat Fields

  • status
  • type = dhcpv4 (we might remove this since we have event.dataset)

The DHCPv4 protocol dataset works on uni-flows (it's not transacted, see elastic#7956) so
the `source` and `destination` will indicate the original packet header data. Meanwhile
the `client` / `server` fields are copies of the `source`/`destination`, but they are
copied based on which side is the client and server.

Here's a summary of what fields changed.

Part of elastic#7968

Changed

- bytes_in -> source.bytes
- transport -> network.transport = udp

Added

- source
- destination
- event.dataset = dhcpv4
- event.start
- network.bytes
- network.community_id
- network.protocol = dhcpv4
- network.type

Unchanged Packetbeat Fields

- status
- type = dhcpv4 (we might remove this since we have event.dataset)
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM.

But someone with more knowledge of the packetbeat code should also have a look.

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. One nit/question about the columns used in the saved search.

@@ -258,7 +258,8 @@
"dhcpv4.transaction_id",
"dhcpv4.op_code",
"dhcpv4.option.message_type",
"client_ip",
"source.ip",
"destination.ip",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: shouldn't you use client.ip and server.ip here, to be better aligned with past naming, and the "data transfer" visualization?

Unless it's semantically important to use those because of the mapping src/dst vs cli/srv flipping around?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using source/destination makes more sense to me because this dataset produces unidirectional flows. The table essentially shows the traffic at a packet level so with source and destination you can see that A sent this message to B and then know that B responded to A. When I use client/server for a uni-flow I'm not 100% which direction the communication is going since the client/server fields remain the same for all events.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Would it make sense to align the "Data transfer" visualization to use source and destination, then?

In any case, I'm all good with this. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

No it wouldn't because for a uni-flow only the source has bytes. I had the same thought.

// Reverse
client, server := ecs.Client(*pbf.Destination), ecs.Server(*pbf.Source)
pbf.Client = &client
pbf.Server = &server
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for filling out src/dst, even if the cli/srv pair makes more sense at a conceptual level. 👍

@andrewkroh andrewkroh merged commit a16cd5b into elastic:master Jan 17, 2019
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.

3 participants