fix(geoip): preserve overlapping dat tags - #37
Conversation
db7aa21 to
d56f1f1
Compare
olicesx
left a comment
There was a problem hiding this comment.
Review
The core change is well-implemented: per-tag range indexing, matches_tag / matches_any_tag membership queries, and HTTPS/SVCB hint matching are correct and backed by tests. I verified the range merge + partition_point lookups, the case-normalization chain, and the GeoipPrivate -> is_private_ip simplification (behavior-identical, since GeoIpResult.is_private is is_private_ip(ip) on every construction path). No blocking bugs found.
Three items need a decision before merge, plus a few minor/doc nits:
- Named tags silently never match when MMDB is configured. With both
geoip_db_path+geoip_dat_path, ageoip_country: ["cloudflare"]rule never fires with no warning - undercutting this PR's headline feature. (inlinesrc/matcher/geoip.rs) - HTTPS/SVCB hints use all() within a single record - a silent behavior change. A Cloudflare HTTPS answer with both v4+v6 hints now fails country-tag matches when the v6 range isn't in the tag (false negative vs
main). (inlinesrc/matcher/mod.rs) - Moka cache bypassed in the runtime path -
matches_any_tagnever touchesself.cache;lookup()(the only cache user) has no production callers left. (inlinesrc/matcher/mod.rs)
Minor / docs:
- The PR description claims the shared DAT parser / field-number dispatch / issue #35 fix, but those are already in
main(commitaef9d7a); this PR's diff does not touchgeoip_converter.rs/geoip_proto.rs. Please update the description to match the actual scope. deserialize_string_or_vecarray branch doesn't trim/filter (string branch does). (inlinesrc/config.rs)- README doesn't note that
response_answer_ip_geoip_countryinspects Answers only, while CIDR/private matchers also inspect Additionals; nor that named tags are DAT-only when MMDB is present. uses_geoip_matchers(engine/utils.rs) still countsGeoipPrivate/ResponseAnswerIpGeoipPrivate, so .dat/MMDB gets loaded even when only private matchers are configured (harmless waste).
Index V2Ray GeoIP ranges per tag so named categories can overlap country codes. Route request, pipeline, and response matchers through tag membership while preserving MMDB precedence and legacy lookup behavior.
797b468 to
5f8f063
Compare
olicesx
left a comment
There was a problem hiding this comment.
Clean refactor. Splitting the flat range list into per-tag indexes and separating the "membership" lookup (merged ranges, O(log n)) from the "most-specific" lookup (raw ranges) is the right shape for preserving overlapping tag membership, and the test coverage (MMDB precedence, same-tag nesting, HTTPS/SVCB hints, ALL-vs-ANY scopes) is thorough.
Conclusion: 🟡 CHANGES REQUESTED (1 major, 2 nit)
Findings
🟡 major — most_specific_* misses the narrowest covering range under crossing (non-nested) overlaps — src/matcher/geoip.rs:702 (v6 :721). See inline comment. Particularly relevant because the PR's new named-tag support is exactly where aggregated, non-hierarchical (crossing) ranges are most likely to appear.
✅ nit — README doesn't mention the "≥1 address-bearing Answer required" guard — README.md:332. ResponseAnswerIpGeoipCountry returns false when no Answer carries an address, which is stricter (and safer) than the literal "every IP-bearing Answer must match" wording; one sentence would document the actual behavior.
✅ nit — empty country_codes silently disables the matcher — src/config.rs:1046 (pre-existing). country_codes: "" / [] deserializes to an empty Vec, and matches_any_tag(ip, &[]) is always false; a config-time warning would help.
Verification: cargo build ✓ · cargo clippy --all-targets -- -D warnings ✓ · cargo test ✓ (lib 128 / bin 6 / integration 9 / doctests 10, 0 failed). Reviewed in a worktree at PR HEAD 90759d6.
|
Addressed the latest review in 2638cd7:
Validation passed:
@olicesx Ready for re-review. |
olicesx
left a comment
There was a problem hiding this comment.
Thanks — verified on 2638cd7. most_specific_* now scans every covering range and picks the smallest span, so the [40,55]/[45,70] ip=50 case returns (40,55) as expected, and is_more_specific is consistent (span-first). The new test_most_specific_handles_crossing_overlaps locks it in; the README "≥1 address-bearing Answer" wording and the empty-country_codes warning are both addressed.
Conclusion: ✅ APPROVE
cargo build · cargo clippy --all-targets -- -D warnings · cargo test (lib 129 / bin 6 / integration 9 / doctests 10, 0 failed).
Problem
V2Ray/MetaCubeX GeoIP datasets can assign one IP range to both a country code and named categories such as
CLOUDFLARE. KixDNS previously flattened ranges into one sorted list and returned onecountry_code, so overlapping named categories matched inconsistently.Response GeoIP matching also considered only direct A/AAAA records. HTTPS/SVCB responses commonly carry endpoint addresses in
ipv4hintandipv6hint.Changes
matches_tag,matches_any_tag, and multi-tag diagnostics.datand JSON sources.dat/JSON tags when both sources are configuredcountry_codesas a canonical string array, single string, or comma-separated string; trim values and discard empty entriesipv4hint/ipv6hintvalues in response IP matchersMatching semantics
.dat/JSON index even when MMDB is also configuredresponse_answer_ip_geoip_countryinspects Answers only and requires every IP-bearing Answer record to matchScope
GeoIP DAT protobuf parsing and CIDR conversion remain owned by
maincommitaef9d7a. This PR does not replace or duplicate that parser.Static response construction remains outside this PR.
Validation
country_codes