Skip to content

Start of network headers RFC #1253

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

Merged
merged 3 commits into from
Feb 5, 2021
Merged

Conversation

leehinman
Copy link
Contributor

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • For proposing substantial changes or additions to the schema, have you reviewed the RFC process?
  • If submitting code/script changes, have you verified all tests pass locally using make test?
  • If submitting schema/fields updates, have you generated new artifacts by running make and committed those changes?
  • Is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • Have you added an entry to the CHANGELOG.next.md?

@leehinman leehinman added the RFC label Feb 4, 2021
@dainperkins
Copy link
Contributor

Sweet, nice work @leehinman.

1st thing that was bugging me - when you consider IPFIX many of these can be not only src based as would be typical for a packet capture, but also bi-directional, making me think using src/dst (or specific src/dst fields under network...) would be beneficial. I think I'm actually partial to nesting under network with src/dst.

Do you think it's necessary to have the header portion of the objects? Seems unnecessary to me

Do you want the rest of the list of network fields (e.g. cos, tos, dscp, windowing, etc.)? Course then it would be useful to also include some of those, particularly e.g. qos, under observer as well.

@ebeahan
Copy link
Member

ebeahan commented Feb 5, 2021

Thanks, @leehinman, for writing up this proposal.

@dainperkins brings up some great points. Let's make sure to pull those into the discussion and capture concerns if necessary in the stage 1 PR.

This looks great to me for stage 0; the proposed changes are appropriate and seem useful additions to ECS.

I'll move forward with merging, setting the date, and assigning it an RFC number.

ebeahan
ebeahan previously approved these changes Feb 5, 2021
@ebeahan ebeahan merged commit 0bd8e84 into elastic:master Feb 5, 2021
@abraxxa
Copy link

abraxxa commented Feb 25, 2021

Thanks for the pointer to this pull-request @ebeahan!

As the RFC document doesn' t describe how someone besides the RFC author can comment on the proposal I'll do it here.

I'd suggest to drop the header part to shorten the fields and define the tcp flags as an array as multiple can be set.

@legoguy1000
Copy link
Contributor

I agree with the comment above. This will be great for the suricata, zeek, snort... modules/integrations.

@legoguy1000
Copy link
Contributor

Is this RFC going to be pushed to the next stage??

@kgeller
Copy link
Contributor

kgeller commented Oct 11, 2021

@legoguy1000 no one has opened a PR for the next stage yet.

If you're interested, we certainly encourage community involvement!

@legoguy1000
Copy link
Contributor

@kgeller I opened #1641. There is a lot missing but I wanted to get a least something on the board to start the next round of conversation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants