-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Filebeat] [Netflow] fix field name conversion to snake case #10950
Conversation
Pinging @elastic/secops |
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 still seems to be a corner case generating ..._napt_...
instead of _nat_
.
Good stuff!
"post_nadt_estination_ipv4_address": "192.168.128.17", | ||
"post_nast_ource_ipv4_address": "192.168.230.216", | ||
"post_nat_destination_ipv4_address": "192.168.128.17", | ||
"post_nat_source_ipv4_address": "192.168.230.216", |
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.
OMG LOL! Good fix :-)
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.
Ugly bug
"post_napst_ource_transport_port": 61776, | ||
"post_nast_ource_ipv4_address": "192.168.0.2", | ||
"post_napt_destination_transport_port": 80, | ||
"post_napt_source_transport_port": 61776, |
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.
_napt_
?
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.
Turns out is a legit name, see IPFIX field 227 postNAPTSourceTransportPort.
Stands for "Network Address Port Translation". Got me scared in there because there's a NATP acronym too 😅
|
||
func TestCamelCaseToSnakeCase(t *testing.T) { | ||
for _, testCase := range [][2]string{ | ||
{"aBCDe", "a_bc_de"}, |
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.
Love this compact test ❤️
Original field name conversion was buggy.
d90cd8b
to
42957dc
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.
If NAPT is legit, then LGTM 👍
Hope you don't mind I renamed a variable in this fix PR. I couldn't stand the ridiculous name (I meant STRIDE, which isn't a correct name for what the variable holds) |
Why didn't you go with |
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
jenkins, test this |
Bug fix was missing an entry in CHANGELOG.next
Bug fix was missing an entry in CHANGELOG.next (cherry picked from commit e0bbf2e)
Bug fix was missing an entry in CHANGELOG.next (cherry picked from commit b6c05a2)
Bug fix was missing an entry in CHANGELOG.next (cherry picked from commit b6c05a2)
Bug fix was missing an entry in CHANGELOG.next (cherry picked from commit b6c05a2)
Bug fix was missing an entry in CHANGELOG.next (cherry picked from commit b6c05a2)
…#10950) (elastic#11019) Original field name conversion was buggy. (cherry picked from commit 02e1cc5)
Bug fix was missing an entry in CHANGELOG.next (cherry picked from commit 7aaa37d)
Initial release field name conversion was buggy:
postNATSourceIPv4Address
=>post_nast_ource_ipv4_address
This includes special treatment for badly named field
VRFname
, by convention it should beVRFName
.