Implement Geolocation mocking #165

bbutkovic · 2022-08-09T08:22:03Z

This PR resolves #10 by adding support for mocked Geolocation responses. By default it will return the address of Fastly's HQ in San Francisco and users can override the response based on the requested IP address by modifying the fastly.toml file in the following manner:

[local_server]
	[local_server.geolocation]
                format = "inline-toml"
                [local_server.geolocation.addresses]
		[local_server.geolocation.addresses."127.0.0.1"]
			as_name = "Dummy, Inc."
			city = "New York"
			latitude = 1.23
			longitude = 1.23

or by providing it as an external resource:

[local_server]
	[local_server.geolocation]
                format = "json"
		file = "./test-geolocation-data.json"

test-geolocation-data.json:

{
    "127.0.0.1": {
        "as_name": "Fastly Test",
        "as_number": 12345,
        "area_code": 123,
        "city": "Test City",
        "conn_speed": "broadband",
        "conn_type": "wired",
        "continent": "NA",
        "country_code": "CA",
        "country_code3": "CAN",
        "country_name": "Canada",
        "latitude": 12.345,
        "longitude": 54.321,
        "metro_code": 0,
        "postal_code": "12345",
        "proxy_description": "?",
        "proxy_type": "?",
        "region": "CA-BC",
        "utc_offset": -700
    }
}

Current progress

support for mocked GeoIP responses
support for configuration through fastly.toml
support for IPv6 lookups
tests

aturon · 2022-08-10T20:39:03Z

👋 hi @bbutkovic, thanks very much for taking up this work! I've been working to get in touch with stakeholders on this topic to make sure we know the design constraints. I'll be happy to review the details of the code, and @JakeChampion is going to help shepherd the overall feature/design.

In general, the folks I spoke to internally at Fastly are in support of adding mocking along the lines you are taking here! 👍

A couple bits of context/feedback on the details:

The precise geoip data does not have strong guarantees in terms of which fields will be available in the future. While the Rust SDK, for example, nails down a specific expectation, that could potentially change later on (and is something we'd like to refine at the SDK level). I wonder if we can take a strategy that is less rigid about precisely which fields are included, since in the end we're just providing a json blob anyway.
In the future, we may want to support other means of mocking, e.g. by including an external, freely available geoip database. I'd suggest we follow the approach we take for dictionaries, where there's an explicit format option, and initially we only support inline-toml. Then the actual contents would show up in a TOML table underneath.

Those two points actually work nicely together: when using inline-toml, we could just take the contents of the inner TOML table, and translate them directly to json when servicing geoip requests.

How does all that sound?

JakeChampion

Thanks for working on this, having geoip data for local development will be really nice to have ☺️

I understand that the code will likely change a bit based on @aturon's suggestions such as adding an explicit format option but I thought I would have an initial first-pass review of the code as it currently is.

In case it is useful, this is the test plan I would use when adding similar features:

Add a test-fixture/app in test-fixtures/src/bin which can be used to shows that the feature is working, this is similar to an integration test - this test would only work for valid fastly.toml configurations, the tests for invalid fastly.toml configurations are handled in the next step
Add unit-tests for parsing the new configuration section of fastly.toml in lib/src/config/unit_tests.rs - The test scenarios to cover would be similar to the already existing inline_toml_dictionary_config_tests

Let me know if you want any help or to talk through any parts of the tests or implementation, I'm very happy to help ☺️

lib/src/wiggle_abi/geo_impl.rs

lib/src/session.rs

bbutkovic · 2022-08-11T12:43:36Z

Thank you both for the suggestions!

@aturon:

Those two points actually work nicely together: when using inline-toml, we could just take the contents of the inner TOML table, and translate them directly to json when servicing geoip requests.

I think going with something similar to how dictionaries work is the way to go ultimately.

Perhaps we should leave a default blob of JSON with the current up-to-date specification and periodically update it just to make local development easier, but allow developers to define their own object to be returned entirely as you described, how does that sound?

@JakeChampion:

Add a test-fixture/app in test-fixtures/src/bin which can be used to shows that the feature is working, this is similar to an integration test - this test would only work for valid fastly.toml configurations, the tests for invalid fastly.toml configurations are handled in the next step

Add unit-tests for parsing the new configuration section of fastly.toml in lib/src/config/unit_tests.rs - The test scenarios to cover would be similar to the already existing inline_toml_dictionary_config_tests

This will be definitely helpful when I get there. I'll update the code to include tests and reach out if I hit a roadblock.

TLDR; My plan now is to rework how the configuration data is passed in the TOML file to be loose and not follow a structure, basically to work similarly to how dictionaries work at the moment. Additionally I'll include a default blob of JSON that returns a currently-valid response just so that developers have an easier time spinning up the code locally.

I'll also address the suggestions by Jake and update the code to include tests.

aturon · 2022-08-11T16:58:19Z

@bbutkovic That all sounds great to me!

bbutkovic · 2022-09-12T21:57:46Z

test-fixtures/src/bin/geoip-lookup.rs

+    assert_eq!(geo_v4.proxy_description(), ProxyDescription::Unknown);
+    assert_eq!(geo_v4.proxy_type(), ProxyType::Unknown);
+    assert_eq!(geo_v4.region(), Some("CA-BC"));
+    // assert_eq!(geo_v4.utc_offset(), Some(UtcOffset::from_hms(-7, 0, 0).unwrap()));


This fails because for negative UTC offset (-700 in the case above) Geo::utc_offset() returns None. I also tested this on C@E by using an IP address that belongs to North America region and found that it yields a None as well.

bbutkovic · 2022-09-13T08:26:28Z

I have a working version right now with changes as requested above and tests added. A few things to note, however:

Currently inline-toml entries are added as [local_server.geoip_mapping.contents."127.0.0.1"]. I think we should probably rename the contents bit to addresses because I think it makes more sense in the case above. Thoughts?
We can probably do with more tests. I can add more tests when I get a chance, any ideas for additional test cases?
At the moment developers will need to define all of the fields they would like to provide for a certain IP address, there is no 'merging' with some sane default values. Can this stay like this?

aturon · 2022-09-16T18:08:58Z

Hi @bbutkovic, just wanted to acknowledge your updates -- I or somebody else will work to review ASAP!

mgattozzi

Hey @bbutkovic I'll be taking over the PR review for Aaron! 😄 Excited to help you get this over the finish line.

Currently inline-toml entries are added as [local_server.geoip_mapping.contents."127.0.0.1"]. I think we should probably rename the contents bit to addresses because I think it makes more sense in the case above. Thoughts?

I think addresses makes more sense here rather than contents. I say make the change!

We can probably do with more tests. I can add more tests when I get a chance, any ideas for additional test cases?

What did you have in mind? I think making sure to test failure modes for the config are good. Maybe a test case to trip the BufferSizeError for a lookup, though the Rust SDK handles this case already so this might not be needed here or might be too finicky to implement without much gain. Overall though I feel you've handled most of the needed testing here imo.

At the moment developers will need to define all of the fields they would like to provide for a certain IP address, there is no 'merging' with some sane default values. Can this stay like this?

My gut instinct is this is fine since the customer will likely want to make distinct values to use and you can't really use default values, but I'll ask to see what others on the team think.

A not so fun legal note about GeoIP, but unfortunately it is trademarked. Therefore we need to replace any reference to that with Geolocation to avoid any legal issues before we can merge this.

Overall this is fantastic work and a very comprehensive PR that I enjoyed reading and reviewing. I've noted a small idiom nit and one ask around defaulting to fastly for all lookups. I'll follow up when I get other's opinions around merging sane default values!

mgattozzi · 2022-09-19T19:22:06Z

lib/src/config/geoip.rs

+    }
+
+    pub fn read_json_contents(
+        file: &PathBuf,


We should switch this to be &Path since PathBuf will deref to Path.

mgattozzi · 2022-09-19T19:47:53Z

lib/src/session.rs

+    pub fn geoip_lookup(&self, addr: &IpAddr) -> String {
+        match self.geoip_mapping.get(addr) {
+            Some(geoip) => geoip.to_string(),
+            None => String::from(


Can we have this instead be a default value in the mapping for 127.0.0.1 and the ipv6 equivalent that can be overwritten in the config. By that I mean when the mapping is first initialized in GeoIPMapping::new() this entry is included for localhost and can be overwritten after when the fastly.toml is read.

The reasoning being that if a customer is trying to test doing an IP lookup for a value that isn't available they will always get a value now which can be confusing since there are cases where on C@E it will return a None value.

I'm just thinking if we could somehow even disable the placeholder for loopback addresses as well? Perhaps we can make it so that it returns the placeholder address only in the case when you aren't reading addresses from JSON or reading them from TOML, effectively making it work only when you use the absolute default geolocation settings. Or perhaps we can have this behind a settings flag in the TOML?

For example, sometimes you may want to test what happens to your code when the IP address of the incoming request yields no result.

@bbutkovic am I understanding correctly that you'd want to add an option to remove the default loopback value?

Yes, precisely, if you choose to provide your own resolving through a JSON or TOML you must specify the result for loopback as well, otherwise it will be a "failed" lookup.

Essentially, the default would only work if you don't use TOML or JSON.

Ah okay! I think what we want is sort of a hybrid approach

By default we provide a loopback address default

If it's overwritten by a value in the toml/json use that

You can disable the default by setting a boolean field in the config under local_server.geolocation_mapping.use_default_loopback to false and this defaults to true

I think this will let us document the behavior better and allow people to opt into turning it off or overriding it! Does that sound good to you?

That sounds good!

Great! I think this is probably the last thing we'll need. I'll review it whenever you get it in (just give me an @ ping)

bbutkovic · 2022-09-21T10:49:19Z

Hey @mgattozzi thank you for your input! I took care of changing everything to Geolocation, the idiom change you suggested as well as changed contents to addresses.

bbutkovic · 2022-09-22T07:10:27Z

test-fixtures/src/bin/geolocation-lookup.rs

+    assert_eq!(geo_v4.proxy_description(), ProxyDescription::Unknown);
+    assert_eq!(geo_v4.proxy_type(), ProxyType::Unknown);
+    assert_eq!(geo_v4.region(), Some("CA-BC"));
+    // assert_eq!(geo_v4.utc_offset(), Some(UtcOffset::from_hms(-7, 0, 0).unwrap()));


@mgattozzi For some reason this line fails. I even tried an IP address in California with Compute@Edge and the result was wrong. Perhaps an issue with Rust SDK?

I saw this before and I tried digging into it a little bit (not too much mind you), but I'm not sure whether it's a bug in the SDK (I haven't tried our JS one to check) or if it is totally correct. If you're seeing this on C@E then I think we can just leave this as is for now and I can open up an issue on our internal tracker to investigate it more. You can put a small note saying why it's commented out.

mgattozzi · 2022-10-05T16:33:23Z

@bbutkovic just so you know I'll be on vacation next week so if you do make any updates I'll make sure someone on the team can review it, unless you get the default changes in this week!

bbutkovic · 2022-10-13T13:40:31Z

Hey, @mgattozzi enjoy your vacation!

I managed to wrap this one up, let me know if any more modifications will be needed.

edhjer · 2022-10-26T22:47:33Z

test-fixtures/src/bin/geolocation-lookup.rs

+    assert_eq!(geo_v4.postal_code(), "12345");
+    assert_eq!(geo_v4.proxy_description(), ProxyDescription::Unknown);
+    assert_eq!(geo_v4.proxy_type(), ProxyType::Unknown);
+    assert_eq!(geo_v4.region(), Some("CA-BC"));


These region values are the subdivision part only

I was a bit confused seeing geo_v4.region() == "CA-BC" based on the description here https://developer.fastly.com/reference/vcl/variables/geolocation/client-geo-region-ascii. Should the test fixture use "BC" without the "CA-" to better represent the format returned in C@E?

You're right, that's a mistake on my end. I corrected it now.

Thanks for the catch!

mgattozzi

Hey @bbutkovic I'm back from vacation and finally have had some cycles to review this. Again thanks for your patience in getting this all working!

I have one last test to ask for here. This PR is stellar phenomenal work on your part. We just need one more test in cli/tests/integration/geolocation_lookup.rs for when use_default_loopback is set to true or not present in the manifest since it defaults to true and test that we provide a value for 127.0.0.1 for people to use. Just to cover all of our bases. Once that's in I'm more than happy to approve and merge these changes!

bbutkovic · 2022-11-09T12:06:46Z

Hey, @mgattozzi, I updated the branch with requested changes to test default config.

mgattozzi

@bbutkovic thanks for all the hard work and patience with getting this in. It's an absolutely phenomenal piece of work and one that we greatly appreciate. With the final test in I'm happy to approve this!

mgattozzi · 2022-11-09T16:00:21Z

@bbutkovic I'm gonna run the test suite now before I merge this and if anything happens I'll let you know 😄

implement geo-ip mocking

7b8cc62

bbutkovic mentioned this pull request Aug 9, 2022

Implement geolocation support #10

Closed

JakeChampion reviewed Aug 11, 2022

View reviewed changes

lib/src/wiggle_abi/geo_impl.rs Outdated Show resolved Hide resolved

lib/src/session.rs Outdated Show resolved Hide resolved

bbutkovic added 15 commits August 12, 2022 11:14

check if the resulting JSON fits the buffer

ae95387

Merge branch 'main' into bbutkovic/geo-impl

598c308

refactor to geoip mapping

1b8b818

finalize refactoring to geoip mapping

718aca1

rename module

cd03a47

refactor to use GeoIP data struct

4ba8bc3

fix default JSON

ec53e12

change config naming

1ba76be

refactor geoip config and api

cabfc5c

tests

a14cdd3

make clippy happy

33c88ec

fix typos

8ca47fb

add unit tests

d96f93a

change ip querying to query by IpAddr

772c993

implement ipv6 support

dece79e

bbutkovic commented Sep 12, 2022

View reviewed changes

bbutkovic added 2 commits September 13, 2022 09:09

fix code formatting

44b8fcf

handle invalid IP address entries

e896b32

bbutkovic marked this pull request as ready for review September 13, 2022 08:21

bbutkovic requested a review from JakeChampion September 13, 2022 08:21

mgattozzi reviewed Sep 19, 2022

View reviewed changes

change contents to addresses for geo TOML

dc4eee7

bbutkovic added 2 commits September 21, 2022 09:36

idiomatic changes to Path vs PathBuf

cbc96d3

rename to Geolocation

e4c230f

bbutkovic changed the title ~~Implement GeoIP mocking~~ Implement Geolocation mocking Sep 22, 2022

bbutkovic commented Sep 22, 2022

View reviewed changes

bbutkovic added 7 commits October 10, 2022 13:28

add default loopback control

dba79b7

Merge branch 'main' of github.com:fastly/Viceroy into bbutkovic/geo-impl

bdee162

refactor Geolocation

e07dbb1

organize code

b5a3fae

format code

725c543

add note regarding utc_offset

b4d39e8

update tests, format options

aca2f59

Merge branch 'main' into bbutkovic/geo-impl

7f1e29f

edhjer reviewed Oct 26, 2022

View reviewed changes

bbutkovic added 2 commits October 27, 2022 08:36

Merge remote-tracking branch 'origin/main' into bbutkovic/geo-impl

2aaae29

change test region to a proper return value

bed1f47

mgattozzi suggested changes Nov 4, 2022

View reviewed changes

bbutkovic added 2 commits November 9, 2022 12:21

Merge branch 'main' into bbutkovic/geo-impl

1f7cd8d

add test for default geolocation config

cfc2f72

bbutkovic requested review from mgattozzi and removed request for JakeChampion November 9, 2022 12:06

mgattozzi approved these changes Nov 9, 2022

View reviewed changes

mgattozzi merged commit 1905d9a into fastly:main Nov 9, 2022

mgattozzi mentioned this pull request Nov 17, 2022

Release Viceroy 0.3.2 #208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Geolocation mocking #165

Implement Geolocation mocking #165

bbutkovic commented Aug 9, 2022 •

edited

aturon commented Aug 10, 2022

JakeChampion left a comment •

edited

bbutkovic commented Aug 11, 2022

aturon commented Aug 11, 2022

bbutkovic Sep 12, 2022

bbutkovic commented Sep 13, 2022

aturon commented Sep 16, 2022

mgattozzi left a comment

mgattozzi Sep 19, 2022

mgattozzi Sep 19, 2022

bbutkovic Sep 21, 2022

mgattozzi Sep 27, 2022

bbutkovic Sep 29, 2022

mgattozzi Sep 29, 2022

bbutkovic Sep 30, 2022

mgattozzi Sep 30, 2022

bbutkovic commented Sep 21, 2022

bbutkovic Sep 22, 2022

mgattozzi Sep 22, 2022

mgattozzi commented Oct 5, 2022

bbutkovic commented Oct 13, 2022

edhjer Oct 26, 2022

bbutkovic Oct 27, 2022

mgattozzi left a comment

bbutkovic commented Nov 9, 2022

mgattozzi left a comment

mgattozzi commented Nov 9, 2022

Implement Geolocation mocking #165

Implement Geolocation mocking #165

Conversation

bbutkovic commented Aug 9, 2022 • edited

aturon commented Aug 10, 2022

JakeChampion left a comment • edited

Choose a reason for hiding this comment

bbutkovic commented Aug 11, 2022

aturon commented Aug 11, 2022

Choose a reason for hiding this comment

bbutkovic commented Sep 13, 2022

aturon commented Sep 16, 2022

mgattozzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbutkovic commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgattozzi commented Oct 5, 2022

bbutkovic commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgattozzi left a comment

Choose a reason for hiding this comment

bbutkovic commented Nov 9, 2022

mgattozzi left a comment

Choose a reason for hiding this comment

mgattozzi commented Nov 9, 2022

bbutkovic commented Aug 9, 2022 •

edited

JakeChampion left a comment •

edited