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

event.client.geo can throw #537

Open
twhid opened this issue May 20, 2023 · 8 comments
Open

event.client.geo can throw #537

twhid opened this issue May 20, 2023 · 8 comments

Comments

@twhid
Copy link

twhid commented May 20, 2023

2023-05-19T21:19:12.255885Z ERROR request{id=0}: Hostcall yielded an error: No geolocation data: 10.154.65.4
Error while running request handler: get_geo_info: A `None` error. This status code is used to indicate when an optional value did not exist, as opposed to an empty value.

I assume this isn't a bug, but working as designed.

tl;dr event.client.geo can throw, but there's no indication of this in the docs or the types (or maybe I missed it? Looking here https://js-compute-reference-docs.edgecompute.app/docs/globals/FetchEvent/#instance-properties).

====

I wanted to bring to your attention a subtle issue that could cause requests to fail with an error since it's not clear that one should guard against an an error when using geo data.

This occurred when running fastly compute serve in a docker container. It seems to work fine running on 127.0.0.1 outside a container.

My issue occurred when accessing geo data via expressly req.clientInfo, which is simply event.client (from the fetch event). I was then accessing req.clientInfo.geo, which I assume is a getter of some sort that under the hood invokes getGeolocationForIpAddress (or something like it). And in my scenario in the container, throws an error. The types in js-compute don't indicate that geo will be anything other than Geolocation interface so this was surprising to me.

expressly@1.3
js-compute@1.5.1

@JakeChampion
Copy link
Contributor

Thank you for reporting this documentation issue, we will definitely improve the documentation here (and possibly the code also to not error but maybe return null values for all the geo fields)

I believe the root issue here which causes the error to be thrown is that fastly compute serve requires geolocation information to be defined in the projects fastly.toml and I am assuming that the project you are running locally does not have those fields defined. - https://developer.fastly.com/reference/compute/fastly-toml/#geolocation

@twhid
Copy link
Author

twhid commented May 20, 2023

Thanks for the reply!

I don't believe it's required to be in the toml file? It was working when running on 127.0.0.1 without it anyway.

@JakeChampion
Copy link
Contributor

Thanks for the reply!

I don't believe it's required to be in the toml file? It was working when running on 127.0.0.1 without it anyway.

I don't think it was working as it threw an error, the error was thrown because there was no geolocation data, when running locally the geolocation data needs to be manually defined in fastly.toml - this is because we don't provide a local implementation of IP address geolocation data as that would be too large (all IP addresses geolocation data is very large).

I'll be improving the documentation and probably the error message to mention this difference between running on fastly and running locally, defining the data in fastly.tonl is required for local development using geolocation

@twhid
Copy link
Author

twhid commented May 21, 2023

Not to be annoying but it was working, it threw when in the container but not when running locally when the request IP was 127.0.0.1. I assume there's an affordance for that in js-compute and the docs seem to indicate that. Says it's mocked and sounds optional re setting it on the toml file:

When running within the local server, geolocation data is mocked and if you wish can be defined in the configuration

My guess is that my problem is that the request IP in the container is in the 10 range.

@JakeChampion
Copy link
Contributor

JakeChampion commented May 22, 2023

My apologies, I didn't realise that Viceroy will mock the default loopback address automatically (and not other addresses) -- https://github.com/fastly/Viceroy/blob/3041220c606b5aca2268517670b7f8402ec49b64/lib/src/config/geolocation.rs#L46-L54

When running within the local server, geolocation data is mocked and if you wish can be defined in the configuration

My guess is that my problem is that the request IP in the container is in the 10 range.

Yes, that would be correct, viceroy it seems will mock the loopback address by default and nothing else.

You could disable the loopback mocking if that's useful to do and then provide your own data:

[local_server]
	[local_server.geolocation]
	use_default_loopback = false
	file = "./geolocation-mapping.json"
	format = "json"

@twhid
Copy link
Author

twhid commented May 22, 2023

Thanks @JakeChampion !

It seems very unlikely to run into this issue within a Fastly service with legitimate traffic. It's just something I need to handle for my internal testing environment.

But that leads me to another question. Is Viceroy used in the Fastly infra? Or is it only for local test environments? If it's only for local testing, how do you ensure that the prod vs testing runtimes are as identical as possible?

Thanks again for your replies.

@JakeChampion
Copy link
Contributor

It's only for local development, not used in fastly infrastructure. The teams that work on the fastly infrastructure also work on viceroy 👍 we also test the SDKs on fastly infrastructure and on viceroy. Both of those help us ensure that viceroy matches compute@edge where it makes sense to match it. Some scenarios don't make sense to match compute@edge such as the storage location for config stores and KV stores, those make more sense to have locally defined for development purposes 👍

@twhid
Copy link
Author

twhid commented May 22, 2023

Very good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants