Page MenuHomePhabricator
Create Task
Maniphest T118557

Replace Analytics XFF/client.ip data with X-Client-IP
Closed, ResolvedPublic5 Estimated Story Points
Actions

Description

We now have X-Client-IP as a standard header based on thorough decoding of XFF (and related data) at the Varnish layer, which we'd like to adopt as the standard notion of the client's IP for both applayer and analytics/logging.

It looks like the steps here, if I understand correctly, will be:

  1. Add XCIP to response headers, so that varnishkafka has access to it at all
  2. Add client_ip field based on the above to the webrequest varnishkafka log format (We can probably start getting this into e.g. oxygen logs at this point as well)
  3. Add new field in hive consuming that data
  4. Start using the new field in place of the computed value in analytics, dropping old XFF-processing code.
  5. Remove the "ip" and "x_forwarded_for" fields from webrequest varnishkafka log format

Details

SubjectRepoBranchLines +/-
Remove deprecated ClientIpUDF and deprecate Legacy Pageview code.analytics/refinery/sourcemaster+10 -36
webrequest: remove X-Forwarded-Foroperations/puppetproduction+1 -1
Remove single use of x_forwarded_for in insert_hourly_pagecounts.hqanalytics/refinerymaster+3 -3
statsv: switch "ip" field to X-Client-IP like webrequestoperations/puppetproduction+1 -1
webrequest: move client_ip data to legacy "ip" fieldoperations/puppetproduction+1 -1
webrequest: remove "ip" fieldoperations/puppetproduction+1 -1
webrequest: Add X-Client-IP -> client_ipoperations/puppetproduction+1 -1
Remove webrequest "ip" and "x_forwarded_for"operations/puppetproduction+1 -1
Add resp.http.X-Client-IP -> webrequest:client_ipoperations/puppetproduction+1 -1
add X-Client-IP to response headers for vk to consumeoperations/puppetproduction+4 -0
Customize query in gerrit

Related Objects

Event Timeline

BBlack created this task.Nov 13 2015, 1:17 PM
BBlack raised the priority of this task from to Medium.
BBlack updated the task description. (Show Details)
BBlack added projects: Traffic, Analytics.
BBlack added subscribers: BBlack, Ottomata, faidon, JAllemandou.
Restricted Application added a project: SRE. · View Herald TranscriptNov 13 2015, 1:17 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
gerritbot subscribed.Nov 13 2015, 1:18 PM

Change 252427 had a related patch set uploaded (by BBlack):
add X-Client-IP to response headers for vk to consume

https://gerrit.wikimedia.org/r/252427

gerritbot added a project: Patch-For-Review.Nov 13 2015, 1:18 PM

Change 252928 had a related patch set uploaded (by BBlack):
Add resp.http.X-Client-IP -> webrequest:client_ip

https://gerrit.wikimedia.org/r/252928

gerritbot added a comment.Nov 13 2015, 1:18 PM

Change 252929 had a related patch set uploaded (by BBlack):
Remove webrequest "ip" and "x_forwarded_for"

https://gerrit.wikimedia.org/r/252929

BBlack updated the task description. (Show Details)Nov 13 2015, 1:41 PM
BBlack set Security to None.
Ottomata added a subscriber: leila.Nov 13 2015, 4:01 PM

Add client_ip field based on the above to the webrequest varnishkafka log format (We can probably start getting this into e.g. oxygen logs at this point as well)

This will happen as soon as varnishkafka starts sending it.

Remove the "ip" and "x_forwarded_for" fields from webrequest varnishkafka log format

Hm, not sure about this. ip, maybe. But I think that @leila was saying that they use x_forwarded_for to make it easier for them to correlate requests from the same user. (It is possible that x_forwarded_for is different but client_ip is the same for different end users, no? E.g. if they originated from behind the same NAT but used different proxies?)

@BBlack, are you aware of T113817? I'll CC you over there, since that is related. Also, (perhaps this should be in a different ticket), but yesterday analytics and researcher folks were talking about using the same rotating etcd salt key that EventLogging uses to hash IPs in refined webrequest data. This would allow for correlation of requests without knowing actual IPs. We could use the same salt to also hash x_forwarded_for if needed.

BBlack added a comment.Nov 13 2015, 10:03 PM
In T118557#1803813, @Ottomata wrote:

Hm, not sure about this. ip, maybe.

'ip' is mostly-pointless even now, as it's almost universally our own internal IPs from nginx passing the request into varnish for HTTPS.

But I think that @leila was saying that they use x_forwarded_for to make it easier for them to correlate requests from the same user. (It is possible that x_forwarded_for is different but client_ip is the same for different end users, no? E.g. if they originated from behind the same NAT but used different proxies?)

That seems like an extreme edge case. If a bunch of users are behind one NAT IP, there's not great odds of being able to sort them out based on one of them using a certain remote proxy and the other not. Even then that would mostly be in the pre-HTTPS world. With HTTPS, very few proxies can be used at all (well, technically you could use a CONNECT-style proxy, but then that wouldn't add anything to the XFF chain).

The common XFF cases are only going to be with special clients (e.g. OperaMini, which we're already decoding for an indicating), or with corporate proxies that use company-installed fake root certs on company machines (in which case all the company machines behind a given NAT will share the same proxy and it's still not a point of differentiation).

I could see wanting to know XFF to know about proxy usage itself, separately from knowing about users, but really we should be adding those to the netmapper proxy database if so and then they'll be in our new X-Trusted-Proxy header, which we could start sending to analytics as well if you want it. Currently it only knows about OperaMini and Nokia.

The point of the new headers is to centralize the parsing of XFF in one place with well-understood semantics that are correct, where we can adapt it moving forward as our infrastructure and the internet at large changes and thus the correct parsing of it also changes.

Ottomata added a comment.Nov 16 2015, 2:58 PM

I'd like both @JAllemandou and @leila to review this and give the go ahead to remove ip and x_forwarded_for. If they say cool, then it is fine with me.

Hm, if we do remove the ip field, we might as well just reuse it and stick client ip there. This would then require 0 changes on the analytics side of things.

gerritbot added a comment.Nov 16 2015, 3:07 PM

Change 252427 merged by BBlack:
add X-Client-IP to response headers for vk to consume

https://gerrit.wikimedia.org/r/252427

gerritbot added a comment.Nov 16 2015, 3:08 PM

Change 252928 merged by BBlack:
Add resp.http.X-Client-IP -> webrequest:client_ip

https://gerrit.wikimedia.org/r/252928

gerritbot added a comment.Nov 16 2015, 9:55 PM

Change 252929 abandoned by BBlack:
Remove webrequest "ip" and "x_forwarded_for"

Reason:
rebasing after parent revert got annoying, starting over

https://gerrit.wikimedia.org/r/252929

gerritbot added a comment.Nov 16 2015, 10:18 PM

Change 253472 had a related patch set uploaded (by BBlack):
webrequest: Add X-Client-IP -> client_ip

https://gerrit.wikimedia.org/r/253472

gerritbot added a comment.Nov 16 2015, 10:18 PM

Change 253473 had a related patch set uploaded (by BBlack):
webrequest: remove "ip" field

https://gerrit.wikimedia.org/r/253473

gerritbot added a comment.Nov 16 2015, 10:18 PM

Change 253474 had a related patch set uploaded (by BBlack):
webrequest: remove X-Forwarded-For

https://gerrit.wikimedia.org/r/253474

leila mentioned this in T119144: EventLogging sees too few distinct client IPs {oryx} [8 pts].Nov 23 2015, 4:52 PM
gerritbot added a comment.Nov 30 2015, 2:19 PM

Change 253472 merged by BBlack:
webrequest: Add X-Client-IP -> client_ip

https://gerrit.wikimedia.org/r/253472

BBlack added a comment.Nov 30 2015, 2:28 PM

With varnishkafka-1.0.7 deployed and the patch above merged, the webrequest stream now has a correct "client_ip" field that analytics can transition to. Do we need to add it to other varnishkafka streams as well?

BBlack added a comment.Nov 30 2015, 2:29 PM

(and, I just read @Ottomata's comment above - we can certainly switch the data into "ip" instead of "client_ip". That might be simpler than switching analytics code to the new field then removing the old).

JAllemandou added a comment.Nov 30 2015, 2:33 PM

No problem for me to remove and reuse ip, and remove x_forwarded_for :)

gerritbot added a comment.Nov 30 2015, 2:43 PM

Change 256002 had a related patch set uploaded (by BBlack):
webrequest: move client_ip data to legacy "ip" field

https://gerrit.wikimedia.org/r/256002

gerritbot added a comment.Nov 30 2015, 3:46 PM

Change 256002 merged by BBlack:
webrequest: move client_ip data to legacy "ip" field

https://gerrit.wikimedia.org/r/256002

gerritbot added a comment.Nov 30 2015, 3:47 PM

Change 253473 abandoned by BBlack:
webrequest: remove "ip" field

Reason:
Obviated by https://gerrit.wikimedia.org/r/#/c/256002/

https://gerrit.wikimedia.org/r/253473

gerritbot added a comment.Nov 30 2015, 4:19 PM

Change 256020 had a related patch set uploaded (by BBlack):
statsv: switch "ip" field to X-Client-IP like webrequest

https://gerrit.wikimedia.org/r/256020

BBlack added a comment.Nov 30 2015, 4:19 PM

Ok, the old "ip" field now has the X-Client-IP data in the webrequest logs.

The remaining pending patches here are:

the (updated) one to remove XFF data from webrequest: https://gerrit.wikimedia.org/r/#/c/253474/
a new one to make the same "ip" switch for statsv: https://gerrit.wikimedia.org/r/#/c/256020/

gerritbot added a comment.Nov 30 2015, 8:39 PM

Change 256020 merged by BBlack:
statsv: switch "ip" field to X-Client-IP like webrequest

https://gerrit.wikimedia.org/r/256020

Milimetric closed this task as a duplicate of T116772: Update client IP in webrequest table to use IP [5 pts] {hawk}.Dec 7 2015, 6:00 PM
BBlack moved this task from Backlog to Done on the Traffic board.Dec 11 2015, 4:23 PM
gerritbot added a comment.Mar 23 2016, 4:01 PM

Change 279132 had a related patch set uploaded (by Ottomata):
Remove single use of x_forwarded_for in insert_hourly_pagecounts.hq

https://gerrit.wikimedia.org/r/279132

gerritbot added a comment.Mar 23 2016, 4:02 PM

Change 279132 merged by Joal:
Remove single use of x_forwarded_for in insert_hourly_pagecounts.hq

https://gerrit.wikimedia.org/r/279132

Ottomata added a comment.EditedMay 25 2017, 1:04 PM

Just noticed this ticket again! Can/should we move forward with this?

BBlack added a comment.May 25 2017, 1:10 PM

From my perspective, where we last stalled out is waiting for Analytics to say it's ok to merge https://gerrit.wikimedia.org/r/#/c/253474 (which removes XFF data from the webrequest stream). I'm not sure what blockers/deps/validation may be pending over in the Analytics side before that.

• Nuria reopened this task as Open.May 25 2017, 3:36 PM
• Nuria removed a project: Analytics.
• Nuria added a project: Analytics.
• Nuria moved this task from Incoming to Operational Excellence Future on the Analytics board.
• Nuria edited projects, added Analytics-Kanban; removed Analytics.Jun 8 2017, 4:22 PM
• Nuria set the point value for this task to 5.
Ottomata moved this task from Next Up to In Progress on the Analytics-Kanban board.Jun 13 2017, 1:30 PM
gerritbot added a comment.Jun 13 2017, 2:08 PM

Change 358603 had a related patch set uploaded (by Ottomata; owner: Ottomata):
[analytics/refinery/source@master] Remove deprecated ClientIpUDF and deprecate Legacy Pageview code.

https://gerrit.wikimedia.org/r/358603

Ottomata added a comment.Jun 13 2017, 2:14 PM

Ooook, I just checked some things.

  • x_forwarded_for was only being used by legacy pageview code in refinery, which itself is not used.
  • I created a hive table on top of JSON data with this field missing, and hive selected it as NULL, no problem.
  • I then refined the JSON table into parquet, and hive selected it as NULL, no problem (expected).

So, we should be good to go and merge that patch. I'll take care of that.

• Nuria assigned this task to Ottomata.Jun 13 2017, 3:17 PM
Ottomata added a comment.Jun 13 2017, 3:26 PM

@BBlack, just one last double check: are you sure XFF is not useful for ops purposes? We can easily exclude this data from refined webrequest analytics data, while still keeping it in the JSON webrequest logs in Kafka.

I'm ready to merge though, so gimme one final +1 and I'll do it.

BBlack added a comment.EditedJun 13 2017, 3:31 PM

No I don't think we need it for non-immediate analysis like this. We still have zero, zeronet and proxy in the X-Analytics string too (which is also in webreq data)

Ottomata added a comment.Jun 13 2017, 3:32 PM

Ok! Will merge this today then, thanks.

gerritbot added a comment.Jun 13 2017, 5:48 PM

Change 253474 merged by Ottomata:
[operations/puppet@production] webrequest: remove X-Forwarded-For

https://gerrit.wikimedia.org/r/253474

Ottomata moved this task from In Progress to Done on the Analytics-Kanban board.Jun 13 2017, 5:57 PM
gerritbot added a comment.Jun 22 2017, 5:40 PM

Change 358603 merged by Ottomata:
[analytics/refinery/source@master] Remove deprecated ClientIpUDF and deprecate Legacy Pageview code.

https://gerrit.wikimedia.org/r/358603

Ottomata closed this task as Resolved.Jun 22 2017, 5:42 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits