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

Undo field migration of nginx.access.remote_ip_list. #10272

Merged
merged 1 commit into from
Jan 25, 2019

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Jan 23, 2019

This field contains the array of forwarded IPs, and should not have been mapped to network.forwarded_ip, which is meant to contain the "real" IP.

Caveats

  • Note that the PR doesn't actually populate network.forwarded_ip with the single restored IP right now. It populates source.ip directly instead. Open to discussion.

The change reverted here was done in #9081, which went into 7.0.0-alpha2.

@webmat webmat requested review from a team as code owners January 23, 2019 03:13
@webmat webmat self-assigned this Jan 23, 2019
@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

Test failure in Jenkins is unrelated (libbeat)

@webmat webmat requested a review from ruflin January 23, 2019 05:54
@ruflin
Copy link
Member

ruflin commented Jan 23, 2019

Not sure I can follow here. You describe the field as forwarded ips but they should not go into network.forward_ip?

@webmat
Copy link
Contributor Author

webmat commented Jan 23, 2019

Yeah, I could have been more clear LOL.

When reviewing @andrewkroh's #9976, it occurred to me that people typically refer to the real source ip as forwarded_ip, because it comes from the xff header.

In my initial NGINX migration, I had moved the whole array in X-Forwarded-For header to network.forwarded_ip. The ECS field description is pretty clear that this should be the "real" restored IP, no the whole array.

Wanting to match Packetbeat's semantics, the common parlance and the ECS definition, I decided to keep the array around anyway, as the old field name, and no longer putting the array in network.forwarded_ip. And actually I'm not populating network.forwarded_ip as defined in ECS, because this value is already present in source.ip.

So I don't know, I think ECS could support the array, but this may require some discussion. Should we add one field, or consider the whole space of proxies? Aka, a discussion better left for next week :-)

@ruflin
Copy link
Member

ruflin commented Jan 24, 2019

ECS supports an array by default for every field. I think in the end it comes down to on which fields users are querying on.

Approving this PR as agree, we can discuss this in more detail later.

PS: Needs a rebase.

@webmat webmat force-pushed the ecs-fix-nginx-xff branch 2 times, most recently from 7f0d5b2 to 7947772 Compare January 24, 2019 18:57
This field contains the array of forwarded IPs, and should not have been mapped to `network.forwarded_ip`, which is meant to contain the "real" IP..
@webmat webmat merged commit b3bfa6b into elastic:master Jan 25, 2019
@webmat webmat deleted the ecs-fix-nginx-xff branch January 25, 2019 16:00
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.

2 participants