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

Use GCP Geolocation for IP Headers #44

Closed
jrconlin opened this issue Oct 25, 2018 · 7 comments
Closed

Use GCP Geolocation for IP Headers #44

jrconlin opened this issue Oct 25, 2018 · 7 comments

Comments

@jrconlin
Copy link
Member

See https://cloud.google.com/load-balancing/docs/backend-service#user-defined-request-headers

@rfk
Copy link
Contributor

rfk commented Oct 25, 2018

Oh that is rad. This and the mooted rate-limiting headers injected from elsewhere will simplify the application code a lot!

We will need to think about l10n but that should be solvable.

@jrconlin
Copy link
Member Author

Note, should make mmdb an optional data source, and take the header value as preferred.

@shane-tomlinson
Copy link

We will need to think about l10n but that should be solvable.

It should be, but we should figure this out sooner rather than later, I wouldn't want to force our l10n team to translate city and town names world-wide, because from the description of client_city, this field is in English and there is no "canonical list". Perhaps there's a library that does this already.

Name of the city from which the request originated, for example, “Mountain View” for Mountain View, California. There is no canonical list of valid values for this variable. The city names may contain US-ASCII letters, numbers, spaces, and the following characters: !#$%&'*+-.^_`|~.

From the description, the city names will never contain locale specific characters, e.g., an ñ in Spanish.

@jrconlin
Copy link
Member Author

I suspect that the level of granularity for location is probably the same as the MaxMind DB, so probably draws the line at incorporated cities. I also strongly suspect that the suggested course for getting localization is to use the Translate service, and probably some heavy server side caching.

I'm guessing that the channel server should probably do the translation based on the UA provided Accept-Language header. I'm already doing that for the MaxMind stuff, so not a big deal. I would probably have to add in a DB cache mechanism which might impact initial response time for the first query for an unknown language. I'm also guessing that we'd want to be super careful about checking some city names like "Intercourse, PA" or "Anus, FR".

@shane-tomlinson
Copy link

@l-hedgehog raised concerns about using GCP specific headers for this functionality since Moz China's FxA stack will remain in AWS.

@jrconlin
Copy link
Member Author

jrconlin commented Nov 7, 2018

@shane-tomlinson @l-hedgehog, damn good note. To be honest, I don't see these as being replacements, rather, a cascade of "best effort" type attempts. Since the GCP headers will probably be more "up-to-date" than the MaxMind db, I can see using those preferentially, then falling back to doing a MaxMind if GCP comes up empty for whatever reason.

A bit more sensitive question might be determining how we actually localize a locality (e.g. Hong Kong, or disputed areas in the Kashmir). Granted, with some of those spots, not quite sure getting Firefox sync working right is the highest priority item...

jrconlin added a commit that referenced this issue Nov 17, 2018
* Also switch outbound headers to middleware
* Pull cargo package version from Cargo.toml for dev deploy

Closes #44,#59
jrconlin added a commit that referenced this issue Nov 19, 2018
* Switch outbound headers to middleware
* Pull cargo package version from Cargo.toml for dev deploy

Review note:
This is more about setting up pulling the GCP info from the header.
Since GCP does not localize, a proper fix will need to be in more than
just "EN-US", but how we address that has not yet been finalized.

Issue #44
Closes #59
@jrconlin
Copy link
Member Author

jrconlin commented Oct 1, 2019

Added w/ #63

@jrconlin jrconlin closed this as completed Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants