From 0c5e6465ecbb5f1b7b140615be9a5d937dfca703 Mon Sep 17 00:00:00 2001 From: Brian Aydemir Date: Fri, 6 Dec 2024 18:22:09 -0600 Subject: [PATCH] Pre-process GeoIP overrides into CIDRs for more efficient testing (#1384) --- .goreleaser.yml | 2 +- director/cache_ads_test.go | 4 +- director/sort.go | 86 +++++++++++++++++++++----------------- director/sort_test.go | 10 ++--- 4 files changed, 55 insertions(+), 47 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index 5ae8b446f..0c64af02c 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -95,7 +95,7 @@ archives: checksum: name_template: 'checksums.txt' snapshot: - name_template: "{{ incpatch .Version }}-next" + name_template: "7.11.99-next" changelog: sort: asc filters: diff --git a/director/cache_ads_test.go b/director/cache_ads_test.go index 15699834c..34944efdb 100644 --- a/director/cache_ads_test.go +++ b/director/cache_ads_test.go @@ -441,11 +441,11 @@ func TestRecordAd(t *testing.T) { statUtils = make(map[string]serverStatUtil) serverAds.DeleteAll() - geoIPOverrides = nil + geoNetOverrides = nil }) server_utils.ResetTestState() func() { - geoIPOverrides = nil + geoNetOverrides = nil healthTestUtilsMutex.Lock() statUtilsMutex.Lock() diff --git a/director/sort.go b/director/sort.go index cc7054cc1..62dca2bf8 100644 --- a/director/sort.go +++ b/director/sort.go @@ -61,6 +61,11 @@ type ( Coordinate Coordinate `mapstructure:"Coordinate"` } + GeoNetOverride struct { + IPNet net.IPNet + Coordinate Coordinate + } + geoIPError struct { labels prometheus.Labels errorMsg string @@ -72,8 +77,8 @@ func (e geoIPError) Error() string { } var ( - invalidOverrideLogOnce = map[string]bool{} - geoIPOverrides []GeoIPOverride + // Stores the unmarshalled GeoIP override config in a form that's efficient to test + geoNetOverrides []GeoNetOverride // Stores a mapping of client IPs that have been randomly assigned a coordinate clientIpCache = ttlcache.New(ttlcache.WithTTL[netip.Addr, Coordinate](20 * time.Minute)) @@ -108,53 +113,58 @@ func (me SwapMaps) Swap(left, right int) { me[left], me[right] = me[right], me[left] } +// Unmarshal any configured GeoIP overrides. +// Malformed IPs and CIDRs are logged but not returned as errors. +func unmarshalOverrides() error { + var geoIPOverrides []GeoIPOverride + + // Ensure that we're starting with an empty slice. + geoNetOverrides = nil + + err := param.GeoIPOverrides.Unmarshal(&geoIPOverrides) + if err != nil { + return err + } + + for _, override := range geoIPOverrides { + var configuredCIDR string + + if strings.Contains(override.IP, "/") { + configuredCIDR = override.IP + } else { + configuredCIDR = override.IP + "/32" + } + + _, ipNet, err := net.ParseCIDR(configuredCIDR) + if err != nil { + // Log the error, and continue looking for good configuration. + log.Warningf("Failed to parse configured GeoIPOverride address (%s). Unable to use for GeoIP resolution!", override.IP) + continue + } + geoNetOverrides = append(geoNetOverrides, GeoNetOverride{IPNet: *ipNet, Coordinate: override.Coordinate}) + } + return nil +} + // Check for any pre-configured IP-to-lat/long overrides. If the passed address // matches an override IP (either directly or via CIDR masking), then we use the // configured lat/long from the override instead of relying on MaxMind. // NOTE: We don't return an error because if checkOverrides encounters an issue, // we still have GeoIP to fall back on. func checkOverrides(addr net.IP) (coordinate *Coordinate) { - // Unmarshal the values, but only the first time we run through this block - if geoIPOverrides == nil { - err := param.GeoIPOverrides.Unmarshal(&geoIPOverrides) + // Unmarshal the GeoIP override config if we haven't already done so. + if geoNetOverrides == nil { + err := unmarshalOverrides() if err != nil { - log.Warningf("Error while unmarshaling GeoIP Overrides: %v", err) + log.Warningf("Error while unmarshalling GeoIP overrides: %v", err) + return nil } } - - for _, geoIPOverride := range geoIPOverrides { - // Check for regular IP addresses before CIDR - overrideIP := net.ParseIP(geoIPOverride.IP) - if overrideIP == nil { - // The IP is malformed - if !invalidOverrideLogOnce[geoIPOverride.IP] && !strings.Contains(geoIPOverride.IP, "/") { - // Don't return here, because we have more to check. - // Do provide a notice to the user, however. - log.Warningf("Failed to parse configured GeoIPOverride address (%s). Unable to use for GeoIP resolution!", geoIPOverride.IP) - invalidOverrideLogOnce[geoIPOverride.IP] = true - } - } - if overrideIP.Equal(addr) { - return &geoIPOverride.Coordinate - } - - // Alternatively, we can match by CIDR blocks - if strings.Contains(geoIPOverride.IP, "/") { - _, ipNet, err := net.ParseCIDR(geoIPOverride.IP) - if err != nil { - if !invalidOverrideLogOnce[geoIPOverride.IP] { - // Same reason as above for not returning. - log.Warningf("Failed to parse configured GeoIPOverride CIDR address (%s): %v. Unable to use for GeoIP resolution!", geoIPOverride.IP, err) - invalidOverrideLogOnce[geoIPOverride.IP] = true - } - continue - } - if ipNet.Contains(addr) { - return &geoIPOverride.Coordinate - } + for _, override := range geoNetOverrides { + if override.IPNet.Contains(addr) { + return &override.Coordinate } } - return nil } diff --git a/director/sort_test.go b/director/sort_test.go index 753ba752d..d20b6deb0 100644 --- a/director/sort_test.go +++ b/director/sort_test.go @@ -49,7 +49,7 @@ func TestCheckOverrides(t *testing.T) { server_utils.ResetTestState() t.Cleanup(func() { server_utils.ResetTestState() - geoIPOverrides = nil + geoNetOverrides = nil }) // We'll also check that our logging feature responsibly reports @@ -80,11 +80,9 @@ func TestCheckOverrides(t *testing.T) { t.Run("test-log-output", func(t *testing.T) { // Check that the log caught our malformed IP and CIDR. We only need to test this once, because it is only logged the very first time. require.Contains(t, logOutput.String(), "Failed to parse configured GeoIPOverride address (192.168.0). Unable to use for GeoIP resolution!") - require.Contains(t, logOutput.String(), "Failed to parse configured GeoIPOverride CIDR address (10.0.0./24): invalid CIDR address: 10.0.0./24."+ - " Unable to use for GeoIP resolution!") + require.Contains(t, logOutput.String(), "Failed to parse configured GeoIPOverride address (10.0.0./24). Unable to use for GeoIP resolution!") require.Contains(t, logOutput.String(), "Failed to parse configured GeoIPOverride address (FD00::000G). Unable to use for GeoIP resolution!") - require.Contains(t, logOutput.String(), "Failed to parse configured GeoIPOverride CIDR address (FD00::000F/11S): invalid CIDR address: FD00::000F/11S."+ - " Unable to use for GeoIP resolution!") + require.Contains(t, logOutput.String(), "Failed to parse configured GeoIPOverride address (FD00::000F/11S). Unable to use for GeoIP resolution!") }) t.Run("test-ipv4-match", func(t *testing.T) { @@ -196,7 +194,7 @@ func TestSortServerAds(t *testing.T) { server_utils.ResetTestState() t.Cleanup(func() { server_utils.ResetTestState() - geoIPOverrides = nil + geoNetOverrides = nil }) // A random IP that should geo-resolve to roughly the same location as the Madison server