Fix non-deterministic router priority for hosts with multiple Targets/paths

services/config_proxy.go

@@ -986,6 +986,33 @@ func (cp *ConfigProxy) findRouterByPangolinID(routers map[string]interface{}, pa
 // findMatchingRouter finds a router that matches the given host.
 // Prefers the main websecure router over redirect routers (-redirect suffix).
 // This ensures middlewares are applied to the HTTPS router, not the HTTP->HTTPS redirect router.
+//
+// This function only matches on the Host(...) predicate of a router's rule.
+// When a single host is served by more than one non-redirect router - which
+// happens with Pangolin's multi-target path routing, where one resource with
+// several Targets produces multiple routers that share an identical Host()
+// but differ only in PathPrefix() - this function cannot tell which router
+// actually corresponds to the resource being processed, because it has no
+// visibility into anything beyond the host.
+//
+// Previously, in that ambiguous case, the function returned whichever router
+// Go's randomized map iteration over `routers` happened to place first in
+// `matches`. Since `routers` is freshly deserialized from JSON on every cache
+// refresh, the iteration order (and therefore the returned router) changed
+// from call to call. The caller (applyResourceOverrides) mutates the
+// returned router's `priority` field in place, so a resource's
+// RouterPriority override could silently land on an unrelated sibling
+// router instead of the one it belongs to - corrupting that router's
+// priority and creating a priority tie between routers whose relative order
+// must stay fixed (catch-all vs. more specific path). See GH issue #113.
+//
+// To fix this without guessing, matches are now sorted deterministically
+// before selection, and if more than one non-redirect candidate remains
+// (i.e. the host genuinely doesn't identify a single router), the function
+// returns no match at all. The caller then simply skips the override for
+// that resource on that cycle rather than risking it on the wrong router -
+// leaving Pangolin's own router configuration (including its own priority)
+// untouched, which is always safer than corrupting it.
 func (cp *ConfigProxy) findMatchingRouter(routers map[string]interface{}, host string) (string, map[string]interface{}) {
 	// Host matching regex
 	hostRegex := regexp.MustCompile(`Host\(\x60([^` + "`" + `]+)\x60\)`)
@@ -1019,25 +1046,37 @@ func (cp *ConfigProxy) findMatchingRouter(routers map[string]interface{}, host s
 		return "", nil
 	}
 
-	// Prefer the main router (websecure) over redirect routers
-	// Main routers don't have the "-redirect" suffix
+	// Make the selection below deterministic regardless of the (randomized)
+	// order in which the `routers` map was iterated above.
+	sort.Slice(matches, func(i, j int) bool {
+		return matches[i].name < matches[j].name
+	})
+
+	// Prefer the main router (websecure) over redirect routers.
+	// Main routers don't have the "-redirect" suffix.
+	var nonRedirect []matchedRouter
 	for _, m := range matches {
 		if !strings.HasSuffix(m.name, "-redirect") {
-			// Also verify it has websecure entrypoint for extra safety
-			if eps := cp.getRouterEntryPoints(m.router); len(eps) > 0 {
-				for _, ep := range eps {
-					if ep == "websecure" {
-						return m.name, m.router
-					}
-				}
-			}
-			// Even without websecure check, prefer non-redirect routers
-			return m.name, m.router
+			nonRedirect = append(nonRedirect, m)
 		}
 	}
 
-	// Fallback to first match if no non-redirect router found
-	return matches[0].name, matches[0].router
+	switch len(nonRedirect) {
+	case 0:
+		// No non-redirect router found (unexpected, but keep the previous
+		// fallback behavior) - pick the first match in deterministic order.
+		return matches[0].name, matches[0].router
+	case 1:
+		m := nonRedirect[0]
+		return m.name, m.router
+	default:
+		// Ambiguous: multiple non-redirect routers share this host (e.g.
+		// Pangolin multi-target path routing). Host alone cannot
+		// disambiguate which router belongs to the resource being
+		// processed, so report no match instead of guessing (see doc
+		// comment above and GH issue #113).
+		return "", nil
+	}
 }
 
 // getRouterEntryPoints extracts the entryPoints list from a router config

services/config_proxy_test.go

@@ -158,3 +158,204 @@ func TestConfigGeneratorWritesConfigFile(t *testing.T) {
 		t.Fatalf("second generateConfig failed: %v", err)
 	}
 }
+
+// TestConfigProxyFindMatchingRouter covers findMatchingRouter's host-fallback
+// matching, in particular the case that used to be non-deterministic: a host
+// served by more than one non-redirect router, which happens with Pangolin's
+// multi-target path routing (one resource, several Targets/paths, each
+// producing its own router that shares an identical Host() but differs in
+// PathPrefix()). See GH issue #113.
+func TestConfigProxyFindMatchingRouter(t *testing.T) {
+	cp := &ConfigProxy{}
+
+	t.Run("single non-redirect router with redirect companion", func(t *testing.T) {
+		routers := map[string]interface{}{
+			"app": map[string]interface{}{
+				"rule":        "Host(`single.example.com`)",
+				"entryPoints": []interface{}{"websecure"},
+			},
+			"app-redirect": map[string]interface{}{
+				"rule":        "Host(`single.example.com`)",
+				"entryPoints": []interface{}{"web"},
+			},
+		}
+
+		name, router := cp.findMatchingRouter(routers, "single.example.com")
+		if name != "app" {
+			t.Fatalf("expected the non-redirect router to be selected, got %q", name)
+		}
+		if router == nil {
+			t.Fatal("expected a non-nil router")
+		}
+	})
+
+	t.Run("no router matches the host", func(t *testing.T) {
+		routers := map[string]interface{}{
+			"other": map[string]interface{}{
+				"rule":        "Host(`other.example.com`)",
+				"entryPoints": []interface{}{"websecure"},
+			},
+		}
+
+		name, router := cp.findMatchingRouter(routers, "missing.example.com")
+		if name != "" || router != nil {
+			t.Fatalf("expected no match, got name=%q router=%v", name, router)
+		}
+	})
+
+	t.Run("ambiguous multi-target host returns no match, not a guess", func(t *testing.T) {
+		routers := map[string]interface{}{
+			"catch-all": map[string]interface{}{
+				"rule":        "Host(`multi.example.com`) && PathPrefix(`/`)",
+				"entryPoints": []interface{}{"websecure"},
+				"priority":    10,
+			},
+			"specific": map[string]interface{}{
+				"rule":        "Host(`multi.example.com`) && PathPrefix(`/app`)",
+				"entryPoints": []interface{}{"websecure"},
+				"priority":    200,
+			},
+		}
+
+		// Run many times: with the pre-fix implementation this returned
+		// whichever router Go's randomized map iteration visited first,
+		// which flips from call to call. The fix must consistently report
+		// "no match" instead of guessing, on every single call.
+		for i := 0; i < 200; i++ {
+			name, router := cp.findMatchingRouter(routers, "multi.example.com")
+			if name != "" || router != nil {
+				t.Fatalf("iteration %d: expected no match for an ambiguous multi-target host, got name=%q router=%v", i, name, router)
+			}
+		}
+	})
+
+	t.Run("only redirect routers match the host: deterministic fallback to first match", func(t *testing.T) {
+		// Every router matching this host has the "-redirect" suffix, so
+		// nonRedirect is empty (case 0) and the function falls back to
+		// matches[0] in the deterministic (name-sorted) order established
+		// before the redirect filter runs. Regression guard: this fallback
+		// must stay deterministic across repeated calls despite `routers`
+		// being a Go map with randomized iteration order.
+		routers := map[string]interface{}{
+			"zzz-redirect": map[string]interface{}{
+				"rule":        "Host(`redirect-only.example.com`)",
+				"entryPoints": []interface{}{"web"},
+			},
+			"aaa-redirect": map[string]interface{}{
+				"rule":        "Host(`redirect-only.example.com`)",
+				"entryPoints": []interface{}{"web"},
+			},
+		}
+
+		for i := 0; i < 20; i++ {
+			name, router := cp.findMatchingRouter(routers, "redirect-only.example.com")
+			if name != "aaa-redirect" {
+				t.Fatalf("iteration %d: expected deterministic fallback to %q, got %q", i, "aaa-redirect", name)
+			}
+			if router == nil {
+				t.Fatalf("iteration %d: expected a non-nil router", i)
+			}
+		}
+	})
+
+	t.Run("ambiguous host including redirect companions still returns no match", func(t *testing.T) {
+		routers := map[string]interface{}{
+			"catch-all": map[string]interface{}{
+				"rule":        "Host(`multi2.example.com`) && PathPrefix(`/`)",
+				"entryPoints": []interface{}{"websecure"},
+			},
+			"catch-all-redirect": map[string]interface{}{
+				"rule":        "Host(`multi2.example.com`) && PathPrefix(`/`)",
+				"entryPoints": []interface{}{"web"},
+			},
+			"specific": map[string]interface{}{
+				"rule":        "Host(`multi2.example.com`) && PathPrefix(`/app`)",
+				"entryPoints": []interface{}{"websecure"},
+			},
+			"specific-redirect": map[string]interface{}{
+				"rule":        "Host(`multi2.example.com`) && PathPrefix(`/app`)",
+				"entryPoints": []interface{}{"web"},
+			},
+		}
+
+		for i := 0; i < 50; i++ {
+			name, router := cp.findMatchingRouter(routers, "multi2.example.com")
+			if name != "" || router != nil {
+				t.Fatalf("iteration %d: expected no match, got name=%q router=%v", i, name, router)
+			}
+		}
+	})
+}
+
+// TestApplyResourceOverridesSkipsAmbiguousMultiTargetHost is the end-to-end
+// regression test for the bug reported in GH issue #113: when the direct
+// pangolin_router_id lookup misses (e.g. because Pangolin's router ID
+// changed) and the host-based fallback is ambiguous (two non-redirect
+// routers share the host), applyResourceOverrides must leave BOTH routers'
+// priority untouched instead of corrupting one of them with the resource's
+// RouterPriority.
+func TestApplyResourceOverridesSkipsAmbiguousMultiTargetHost(t *testing.T) {
+	cp := &ConfigProxy{}
+
+	routers := map[string]interface{}{
+		"catch-all": map[string]interface{}{
+			"rule":        "Host(`multi.example.com`) && PathPrefix(`/`)",
+			"entryPoints": []interface{}{"websecure"},
+			"priority":    10,
+		},
+		"specific": map[string]interface{}{
+			"rule":        "Host(`multi.example.com`) && PathPrefix(`/app`)",
+			"entryPoints": []interface{}{"websecure"},
+			"priority":    200,
+		},
+	}
+
+	config := &ProxiedTraefikConfig{
+		HTTP: &HTTPConfig{
+			Routers:     routers,
+			Middlewares: map[string]interface{}{},
+		},
+	}
+
+	resources := []*resourceData{
+		{
+			ID: "resource-1",
+			// Simulates "Pangolin changed the router ID": the stored ID no
+			// longer exists as a router key, so findRouterByPangolinID
+			// misses and the host-based fallback (findMatchingRouter) is
+			// used - which is ambiguous for this host.
+			PangolinRouterID: "stale-router-id-that-no-longer-exists",
+			Host:             "multi.example.com",
+			// Sentinel value: must never end up on either router below.
+			RouterPriority: 999,
+		},
+	}
+
+	for i := 0; i < 20; i++ {
+		if err := cp.applyResourceOverrides(config, resources, nil, nil); err != nil {
+			t.Fatalf("iteration %d: applyResourceOverrides() error = %v", i, err)
+		}
+
+		catchAll, ok := routers["catch-all"].(map[string]interface{})
+		if !ok {
+			t.Fatalf("iteration %d: catch-all router missing or wrong type", i)
+		}
+		specific, ok := routers["specific"].(map[string]interface{})
+		if !ok {
+			t.Fatalf("iteration %d: specific router missing or wrong type", i)
+		}
+
+		if catchAll["priority"] != 10 {
+			t.Fatalf("iteration %d: catch-all router priority was mutated: got %v, want 10 (ambiguous host must not be overridden)", i, catchAll["priority"])
+		}
+		if specific["priority"] != 200 {
+			t.Fatalf("iteration %d: specific router priority was mutated: got %v, want 200 (ambiguous host must not be overridden)", i, specific["priority"])
+		}
+		if _, ok := catchAll["middlewares"]; ok {
+			t.Fatalf("iteration %d: catch-all router should not have received middleware overrides for an ambiguous host match", i)
+		}
+		if _, ok := specific["middlewares"]; ok {
+			t.Fatalf("iteration %d: specific router should not have received middleware overrides for an ambiguous host match", i)
+		}
+	}
+}