feat: provider egress geolocation ingest and integration#407
Open
Ryanmello07 wants to merge 14 commits into
Open
feat: provider egress geolocation ingest and integration#407Ryanmello07 wants to merge 14 commits into
Ryanmello07 wants to merge 14 commits into
Conversation
SetConnectionLocation panicked when a client's IP resolved to a country- or region-only location (no city), because it inserted the location row's NULL city_location_id / region_location_id into network_client_location, where both are NOT NULL. The panic is the real damage, not just a failed lookup: it propagates out of the connection announce goroutine (connect/transport_announce.go), whose HandleError wrapper cancels the connection-level context -- tearing down the entire connect connection right after auth. And because the panic hits before the disconnect-cleanup defer is registered, the connection row is orphaned as connected=true forever. How often this fires depends on the geo database's city coverage, so it is rare with a full commercial dataset and constant with a sparse one. Fix: fall back to the coarsest available granularity so the columns are always non-null -- a country-only location stores its country id for city/region too, keeping the provider locatable at country level instead of crashing the connection. If even the country id is absent, return a clean error (the caller already has a graceful retry path) rather than panic.
The sub-project's binding constraint is that country codes are stored/
compared lowercased, and CreateLocation (network_client_location_model.go)
already enforces this at the model layer via strings.ToLower before writing.
SetProviderEgressLocation wrote e.CountryCode verbatim, so an uppercase code
from an operator-run prober (the raw "US" the real geolocation APIs return)
would silently persist uppercase and violate the invariant that
countryCodeLocationIds and other lookups depend on.
Normalize to lowercase before the INSERT/UPDATE, mirroring CreateLocation's
idiom, and add a test asserting SetProviderEgressLocation("US") round-trips
as "us" via GetProviderEgressLocation.
…ion test TestSubmitProviderEgressLocationCountryOnly only checked the persisted CityConfident flag, which is copied straight from args independent of the location-resolution branch. It never asserted the resolved location's LocationType, so a broken granularity gate (resolving city unconditionally) would slip past all 5 existing tests. Add an assertion that the resolved location is LocationTypeCountry, mirroring the pattern already used by TestSubmitProviderEgressLocationCityConfidentStoresCity. Also assert CityLocationId/RegionLocationId are unset, since a country-granularity row's INSERT never populates those columns (verified against CreateLocation and GetLocation).
Adds POST /network/provider-egress-location, an operator-to-server endpoint (not a client route) that ingests probed provider egress locations. Authenticated by a shared secret from the vault (beta-vault/vault/provider_egress.yml, key ingest_secret) compared with hmac.Equal, not a network jwt. Fails closed: an absent vault resource or empty/missing key disables the endpoint (every request rejected) without panicking the api process at startup or per-request.
… just reject Both committed tests for ProviderEgressLocationSubmit ran with the vault unconfigured, so hmac.Equal never executed - a handler that always returned 401 would have passed the same suite. Extract the memoized secret reader (sync.OnceValue) into a reassignable package var plus a plain readOperatorIngestSecret function, with no change to the read logic or the handler's auth gate, so tests can inject a known secret without racing the existing unconfigured-vault reject tests in the same process. Add a positive-path test (correct secret clears auth and reaches the controller, 400 "Unknown client." for an unregistered id) and a negative discrimination test (wrong secret against a configured vault still 401), plus a direct vault-read test. Document in the vault example that the secret is memoized at first request, so rotating it requires an api restart.
…n parity, cover unprotected branches SetConnectionLocation ran on the connect-announce hot path (every client, every connection, inside a retry loop) with two separate DB round trips before ever reaching the mmdb fallback. Fix review findings against the probed-egress location change: - model.GetFreshProviderEgressLocationForConnection replaces the GetNetworkClientForConnection + GetFreshProviderEgressLocation pair with a single query joining network_client_connection to provider_egress_location. Freshness cutoff is still computed/compared in Go, not SQL now(). Cuts the probed-hit path from 3 DB round trips to 2, and the fallback path from 4 to 3. - The probed path now also runs the ARIN org-vs-country foreign check (against the probed country code), matching the mmdb path's GetLocationForIp, so net_type_foreign no longer gives probed providers an unearned ranking advantage over unprobed ones. Factored into a shared arinForeignScore helper; on any ARIN/ip-parse failure it just leaves NetTypeForeign at 0, never erroring or panicking the hot path. - Added tests for the stale-probed fallback, a probed write error (bad location_id) falling back to mmdb without panicking, and the Hosting/Proxy flag-to-score mapping. Verified the stale-fallback test has teeth by temporarily widening maxAge and confirming it fails. Co-Authored-By: Claude <[email protected]> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…oversized submissions, wire up ingest secret Pre-merge fixes from the final whole-branch review of provider-egress geolocation: - Fix the probed-path ARIN "foreign" check: it compared the control ip's ARIN org country against the probed egress country (two different ips), flagging a provider foreign precisely when probing changed the answer. Now matches the mmdb path exactly: ARIN org country of the control ip vs. the mmdb country of that same control ip, restoring probed/unprobed ranking parity. - Reject provider-egress submissions with an empty (post-trim) Country/City/Region instead of silently creating a permanently-blank canonical location row via CreateLocation's dedupe-on-name behavior. - Reject over-long Country/City/Region (>128) or Org (>256) instead of panicking inside CreateLocation on a Postgres "value too long" error. - Make SetProviderEgressLocation's upsert monotonic in observed_at so a replayed older submission cannot clobber a newer stored row. - Replace three production-comment references to the (upstream-absent) design-spec doc path with inline prose, and drop the hardcoded beta vault filesystem path from the ingest-secret handler comment. - Delete GetNetworkClientForConnection (model/provider_egress_location_model.go), a fully superseded, zero-caller helper. - Provision the ingest secret end-to-end: beta-setup.sh now generates beta-vault/vault/provider_egress.yml (guarded the same way as the other generated secrets), and BETA.md documents it plus the api-restart requirement, since the endpoint otherwise ships silently disabled. Adds tests for each behavioral fix; all pass, including TestSetConnectionLocationToleratesCountryOnlyLocation. Co-Authored-By: Claude <[email protected]> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
The example config lives in this fork's beta deployment tree, which does not exist upstream. Operators configure the ingest secret through their own vault; the handler documents the resource name and key.
…AssertEqual The three provider-egress-location test files imported github.com/go-playground/assert/v2, which is present in this fork's go.mod but not in upstream's, breaking compilation on this upstream-based branch (no required module provides package github.com/go-playground/assert/v2). Upstream already has an equivalent helper, connect.AssertEqual (from github.com/urnetwork/connect, used throughout model/ and controller/ test files), so no new third-party dependency is needed. This is a mechanical 1:1 swap (assert.Equal(t, v1, v2) -> connect.AssertEqual(t, v1, v2), same argument order and semantics) with no change to test logic or assertions. None of the three files used assert.NotEqual. No go.mod/go.sum changes: go-playground/assert was never present in this branch's go.mod/go.sum to begin with.
… overflow, and other review findings Five review findings on the provider-egress-location PR, each empirically reproduced before being fixed: - A far-future observed_at had no upper bound: it would win the monotonic upsert forever, read as fresh forever, and outlive the taskworker sweep, permanently pinning a provider's location with no API-side recovery. Add MaxProviderEgressLocationSubmissionSkew (5m) alongside the existing MaxProviderEgressLocationSubmissionAge check, and reject observed_at further in the future than that. - provider_egress_location.asn was `int` (Postgres int4, max ~2.147e9); ASNs are 32-bit unsigned (max ~4.295e9), so a real ASN like 4200000000 panicked pgx's arg encoding after a ~78s retry-storm hang. The table is new in this same unmerged PR, so the fix edits the CREATE TABLE migration directly (asn int -> asn bigint) rather than adding a second migration. - Removed a "fix(beta)" fork marker comment from model/network_client_location_model.go, rewritten to be vendor-neutral while keeping the NULL-city/region panic explanation intact. - arinForeignScore's doc comment said its second argument was "the mmdb-resolved country ... or the probed egress country" -- a later commit made both callers pass the mmdb country for ranking parity, so the doc was updated to describe what the code actually does and why. - SetConnectionLocation mapped the probed Mobile flag onto NetTypeVirtual, but IpInfo has no Mobile concept and NetTypeVirtual is only ever set from the ipinfo schema's is_satellite field. This gave probed mobile providers a ranking penalty an identical unprobed provider never takes -- the opposite of the parity this feature promises. Mobile stays in the model/wire contract as metadata but no longer feeds net_type_virtual; Hosting/Proxy keep feeding their scores since they do have direct mmdb-path equivalents (ipInfo.Hosting/ipInfo.Privacy). Tests added: future/within-skew observed_at rejection and acceptance, large-ASN round-trip, and a Mobile-does-not-set-net_type_virtual assertion folded into the existing probed-flags-to-scores test. Co-Authored-By: Claude <[email protected]> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds server-side ingest and integration for provider egress geolocation: an operator-run prober determines a provider's real egress location and submits it, and the server prefers that over the built-in mmdb lookup when resolving where a provider is.
The motivation is cost and decentralization. Accurate city-level IP geolocation currently requires a paid database, which every network operator would have to buy independently. Geolocating a provider by probing through it — so the geolocation source reports the provider's own egress IP — gets the country reliably for free, from sources that already answer that question. This PR is the server half; the prober itself lives in a separate operator tool.
This also means the location reflects where user traffic actually exits, rather than where the provider's control connection happens to originate — those can differ for multi-homed or NAT'd providers.
What's here
provider_egress_locationtable, keyed byclient_id(append-only migration,IF NOT EXISTS, no FKs, no backfill, no lock on existing tables).POST /network/provider-egress-location— operator-authenticated via a vault shared secret (provider_egress.yml, keyingest_secret), constant-time compared. Fails closed: if the secret is absent or empty, every request is rejected, and a missing vault resource disables the endpoint rather than crashing the api.SetConnectionLocationprefers a fresh stored egress location and otherwise falls back to the existing mmdb path unchanged — on a miss, a stale entry, a non-provider client, or an error writing the probed location.Only country-confident submissions are accepted; city/region are stored only when the submission is also city-confident. In practice free geolocation sources agree on country but frequently disagree on city, so most locations resolve at country granularity by design.
Included bug fix
The first commit fixes a latent panic that this feature would otherwise hit constantly:
network_client_locationrequirescity_location_idandregion_location_idNOT NULL, but a country-granularitylocationrow has themNULL.SetConnectionLocationinserted thoseNULLs directly, panicking insideserver.Tx. That panic escapes the connection announce goroutine, whoseHandleErrorwrapper cancels the connection context — tearing down the client's connection right after auth, and (because the panic hits before the disconnect-cleanup defer is registered) orphaning the connection row asconnected=true.How often it fires depends on the geo database's city coverage: rare with a full commercial dataset, constant with a sparse one. It is included here because this feature makes country-granularity locations the common case.
Compatibility
Fully backward compatible and inert until deliberately enabled:
SetConnectionLocationbehaves exactly as before.Testing
27 tests across
model,controller, andapi/handlers, covering the storage layer, submission validation and rejection paths, the auth gate (including that it accepts a correct secret, not only that it rejects bad ones), and all four fallback paths.Note for reviewers running locally: the three mmdb-fallback tests need a geo database whose schema
ip.gosupports (DB-IP or ipinfo). With an unsupported fixture they fail withUnknown schema type, which is a fixture mismatch rather than a code failure.