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

[Filebeat] [Netflow] fix field name conversion to snake case #10950

Merged
merged 2 commits into from
Mar 1, 2019

Conversation

adriansr
Copy link
Contributor

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 be VRFName.

@adriansr adriansr added bug review Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. SecOps labels Feb 26, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

@adriansr adriansr changed the title Netflow: fix field name conversion to snake case [Filebeat] [Netflow] fix field name conversion to snake case Feb 27, 2019
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.

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",
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG LOL! Good fix :-)

Copy link
Contributor Author

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

_napt_?

Copy link
Contributor Author

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"},
Copy link
Contributor

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.
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.

If NAPT is legit, then LGTM 👍

@adriansr
Copy link
Contributor Author

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)

@webmat
Copy link
Contributor

webmat commented Feb 27, 2019

Why didn't you go with upperCut as an in-between? 😂

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.

LGTM

@adriansr
Copy link
Contributor Author

adriansr commented Mar 1, 2019

jenkins, test this

@adriansr adriansr merged commit 85e470e into elastic:master Mar 1, 2019
adriansr added a commit to adriansr/beats that referenced this pull request Mar 1, 2019
Bug fix was missing an entry in CHANGELOG.next
adriansr added a commit to adriansr/beats that referenced this pull request Mar 1, 2019
…#10950)

Original field name conversion was buggy.

(cherry picked from commit 85e470e)
@adriansr adriansr added v6.6.2 and removed needs_backport PR is waiting to be backported to other branches. labels Mar 1, 2019
adriansr added a commit to adriansr/beats that referenced this pull request Mar 1, 2019
…#10950)

Original field name conversion was buggy.

(cherry picked from commit 85e470e)
@adriansr adriansr added the v6.7.0 label Mar 1, 2019
adriansr added a commit to adriansr/beats that referenced this pull request Mar 1, 2019
…#10950)

Original field name conversion was buggy.

(cherry picked from commit 85e470e)
@adriansr adriansr added the v7.0.0 label Mar 1, 2019
adriansr added a commit that referenced this pull request Mar 1, 2019
…#11022)

Original field name conversion was buggy.

(cherry picked from commit 85e470e)
adriansr added a commit that referenced this pull request Mar 1, 2019
…#11020)

Original field name conversion was buggy.

(cherry picked from commit 85e470e)
adriansr added a commit that referenced this pull request Mar 1, 2019
…#11019)

Original field name conversion was buggy.

(cherry picked from commit 85e470e)
adriansr added a commit to adriansr/beats that referenced this pull request Mar 1, 2019
…#10950)

Original field name conversion was buggy.

(cherry picked from commit 85e470e)
@adriansr adriansr added the v7.2.0 label Mar 1, 2019
adriansr added a commit to adriansr/beats that referenced this pull request Mar 1, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit e0bbf2e)
adriansr added a commit that referenced this pull request Mar 2, 2019
…#11033)

Original field name conversion was buggy.

(cherry picked from commit 85e470e)
adriansr added a commit that referenced this pull request Mar 5, 2019
Bug fix was missing an entry in CHANGELOG.next
adriansr added a commit to adriansr/beats that referenced this pull request Mar 5, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit b6c05a2)
adriansr added a commit to adriansr/beats that referenced this pull request Mar 5, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit b6c05a2)
adriansr added a commit to adriansr/beats that referenced this pull request Mar 5, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit b6c05a2)
adriansr added a commit to adriansr/beats that referenced this pull request Mar 5, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit b6c05a2)
adriansr added a commit that referenced this pull request Mar 6, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit b6c05a2)
adriansr added a commit that referenced this pull request Mar 6, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit b6c05a2)
adriansr added a commit that referenced this pull request Mar 6, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit b6c05a2)
adriansr added a commit that referenced this pull request Mar 6, 2019
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit b6c05a2)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…#10950) (elastic#11019)

Original field name conversion was buggy.

(cherry picked from commit 02e1cc5)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Bug fix was missing an entry in CHANGELOG.next

(cherry picked from commit 7aaa37d)
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.

4 participants