Pre-process GeoIP overrides into CIDRs for more efficient testing (#1384

)

.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:

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()

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
 }
 

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