Pārlūkot izejas kodu

fix(inbounds): scope port check to node and preserve caller tag

Different nodes are different machines, so same port + transport across
NodeIDs shouldn't conflict. resolveInboundTag now keeps a caller-supplied
unique tag verbatim so central and node panels stay in agreement instead
of regenerating into a UNIQUE constraint failure on sync.
MHSanaei 3 dienas atpakaļ
vecāks
revīzija
7214ffafc5
3 mainītis faili ar 176 papildinājumiem un 13 dzēšanām
  1. 2 9
      web/service/inbound.go
  2. 49 4
      web/service/port_conflict.go
  3. 125 0
      web/service/port_conflict_test.go

+ 2 - 9
web/service/inbound.go

@@ -291,12 +291,7 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo
 		return inbound, false, common.NewError("Port already exists:", inbound.Port)
 	}
 
-	// pick a tag that won't collide with an existing row. for the common
-	// case this is the same "inbound-<port>" string the controller already
-	// set; only when this port already has another inbound on a different
-	// transport (now possible after the transport-aware port check) does
-	// this disambiguate with a -tcp/-udp suffix. see #4103.
-	inbound.Tag, err = s.generateInboundTag(inbound, 0)
+	inbound.Tag, err = s.resolveInboundTag(inbound, 0)
 	if err != nil {
 		return inbound, false, err
 	}
@@ -636,9 +631,7 @@ func (s *InboundService) UpdateInbound(inbound *model.Inbound) (*model.Inbound,
 	oldInbound.Settings = inbound.Settings
 	oldInbound.StreamSettings = inbound.StreamSettings
 	oldInbound.Sniffing = inbound.Sniffing
-	// regenerate tag with collision-aware logic. for this row we pass
-	// inbound.Id as ignoreId so it doesn't see its own old tag in the db.
-	oldInbound.Tag, err = s.generateInboundTag(inbound, inbound.Id)
+	oldInbound.Tag, err = s.resolveInboundTag(inbound, inbound.Id)
 	if err != nil {
 		return inbound, false, err
 	}

+ 49 - 4
web/service/port_conflict.go

@@ -118,15 +118,16 @@ func isAnyListen(s string) bool {
 // port-only check, this one understands that tcp/443 and udp/443 are
 // independent sockets in linux and may coexist on the same address.
 //
+// node scope: inbounds with different NodeID run on different physical
+// machines (local panel xray vs a remote node, or two remote nodes),
+// so their sockets can't collide. only candidates with the same NodeID
+// participate in the listen/transport overlap check.
+//
 // the listen-overlap rule (specific addr conflicts with any-addr on the
 // same port, both directions) is preserved from the previous check.
 func (s *InboundService) checkPortConflict(inbound *model.Inbound, ignoreId int) (bool, error) {
 	db := database.GetDB()
 
-	// pull every candidate on this port; we filter by listen-overlap and
-	// transport in go to keep the sql plain. the port column is indexed
-	// in practice by the existing port check, and the candidate set is
-	// tiny (one per coexisting socket family at most).
 	var candidates []*model.Inbound
 	q := db.Model(model.Inbound{}).Where("port = ?", inbound.Port)
 	if ignoreId > 0 {
@@ -138,6 +139,9 @@ func (s *InboundService) checkPortConflict(inbound *model.Inbound, ignoreId int)
 
 	newBits := inboundTransports(inbound.Protocol, inbound.StreamSettings, inbound.Settings)
 	for _, c := range candidates {
+		if !sameNode(c.NodeID, inbound.NodeID) {
+			continue
+		}
 		if !listenOverlaps(c.Listen, inbound.Listen) {
 			continue
 		}
@@ -148,6 +152,21 @@ func (s *InboundService) checkPortConflict(inbound *model.Inbound, ignoreId int)
 	return false, nil
 }
 
+// sameNode reports whether two NodeID pointers refer to the same xray
+// process. nil/nil means both inbounds run on the local panel; non-nil
+// with equal value means they share the same remote node. any mix
+// (local vs remote, remote-A vs remote-B) is "different node" and
+// can't produce a real socket collision.
+func sameNode(a, b *int) bool {
+	if a == nil && b == nil {
+		return true
+	}
+	if a == nil || b == nil {
+		return false
+	}
+	return *a == *b
+}
+
 // baseInboundTag is the historical "inbound-<port>" / "inbound-<listen>:<port>"
 // shape. kept exactly so existing routing rules that reference these tags
 // keep working after the upgrade.
@@ -220,6 +239,32 @@ func (s *InboundService) generateInboundTag(inbound *model.Inbound, ignoreId int
 	return "", common.NewError("could not pick a unique inbound tag for port:", inbound.Port)
 }
 
+// resolveInboundTag chooses a tag for an Add or Update. when the caller
+// supplied a non-empty Tag (e.g. the central panel pushed its picked
+// tag to a node during a multi-node sync) and that tag is free in the
+// local DB, it's used verbatim so the two panels stay in agreement —
+// otherwise the node would regenerate (often back to bare
+// "inbound-<port>") and the eventual traffic sync-back would try to
+// INSERT a row whose tag already exists, hitting the UNIQUE constraint
+// on inbounds.tag and rolling the node-side row right back out.
+// when Tag is empty (the common UI path) or collides, fall back to the
+// transport-aware generateInboundTag.
+//
+// ignoreId mirrors generateInboundTag: pass 0 on add, the inbound's
+// own id on update so a row doesn't see its own current tag as taken.
+func (s *InboundService) resolveInboundTag(inbound *model.Inbound, ignoreId int) (string, error) {
+	if inbound.Tag != "" {
+		taken, err := s.tagExists(inbound.Tag, ignoreId)
+		if err != nil {
+			return "", err
+		}
+		if !taken {
+			return inbound.Tag, nil
+		}
+	}
+	return s.generateInboundTag(inbound, ignoreId)
+}
+
 func (s *InboundService) tagExists(tag string, ignoreId int) (bool, error) {
 	db := database.GetDB()
 	q := db.Model(model.Inbound{}).Where("tag = ?", tag)

+ 125 - 0
web/service/port_conflict_test.go

@@ -35,6 +35,11 @@ func setupConflictDB(t *testing.T) {
 }
 
 func seedInboundConflict(t *testing.T, tag, listen string, port int, protocol model.Protocol, streamSettings, settings string) {
+	t.Helper()
+	seedInboundConflictNode(t, tag, listen, port, protocol, streamSettings, settings, nil)
+}
+
+func seedInboundConflictNode(t *testing.T, tag, listen string, port int, protocol model.Protocol, streamSettings, settings string, nodeID *int) {
 	t.Helper()
 	in := &model.Inbound{
 		Tag:            tag,
@@ -44,12 +49,15 @@ func seedInboundConflict(t *testing.T, tag, listen string, port int, protocol mo
 		Protocol:       protocol,
 		StreamSettings: streamSettings,
 		Settings:       settings,
+		NodeID:         nodeID,
 	}
 	if err := database.GetDB().Create(in).Error; err != nil {
 		t.Fatalf("seed inbound %s: %v", tag, err)
 	}
 }
 
+func intPtr(v int) *int { return &v }
+
 func TestInboundTransports(t *testing.T) {
 	cases := []struct {
 		name           string
@@ -345,6 +353,123 @@ func TestGenerateInboundTag_SpecificListenSameDisambiguation(t *testing.T) {
 	}
 }
 
+// inbounds bound to different nodes run on different physical machines,
+// so the same port + transport must be allowed across nodes. covers
+// local-vs-remote, remote-A-vs-remote-B, and the still-clashing
+// same-node case.
+func TestCheckPortConflict_NodeScope(t *testing.T) {
+	setupConflictDB(t)
+	seedInboundConflictNode(t, "local-443-tcp", "0.0.0.0", 443, model.VLESS, `{"network":"tcp"}`, `{}`, nil)
+	seedInboundConflictNode(t, "node1-443-tcp", "0.0.0.0", 443, model.VLESS, `{"network":"tcp"}`, `{}`, intPtr(1))
+
+	svc := &InboundService{}
+
+	cases := []struct {
+		name   string
+		nodeID *int
+		want   bool
+	}{
+		{"new local same port + tcp clashes with local", nil, true},
+		{"new remote on different node from local is fine", intPtr(2), false},
+		{"new remote on existing node 1 clashes", intPtr(1), true},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			candidate := &model.Inbound{
+				Listen:         "0.0.0.0",
+				Port:           443,
+				Protocol:       model.VLESS,
+				StreamSettings: `{"network":"tcp"}`,
+				NodeID:         c.nodeID,
+			}
+			got, err := svc.checkPortConflict(candidate, 0)
+			if err != nil {
+				t.Fatalf("checkPortConflict: %v", err)
+			}
+			if got != c.want {
+				t.Fatalf("got conflict=%v, want %v", got, c.want)
+			}
+		})
+	}
+}
+
+// when the caller passes an explicit non-empty Tag that doesn't collide,
+// resolveInboundTag returns it verbatim. this is the cross-panel path:
+// the central panel picks a tag, pushes the inbound to a node, and the
+// node must keep that exact tag so the eventual traffic sync-back can
+// match the row by tag. previously the node regenerated and the two
+// panels diverged, causing a UNIQUE constraint failure on sync.
+func TestResolveInboundTag_RespectsCallerTagWhenFree(t *testing.T) {
+	setupConflictDB(t)
+	seedInboundConflictNode(t, "inbound-5000", "0.0.0.0", 5000, model.VLESS, `{"network":"tcp"}`, `{}`, nil)
+	seedInboundConflictNode(t, "inbound-5000-udp", "0.0.0.0", 5000, model.Hysteria2, ``, ``, nil)
+
+	svc := &InboundService{}
+	pushed := &model.Inbound{
+		Tag:            "inbound-5000-tcp",
+		Listen:         "0.0.0.0",
+		Port:           5000,
+		Protocol:       model.VLESS,
+		StreamSettings: `{"network":"tcp"}`,
+		NodeID:         intPtr(1),
+	}
+	got, err := svc.resolveInboundTag(pushed, 0)
+	if err != nil {
+		t.Fatalf("resolveInboundTag: %v", err)
+	}
+	if got != "inbound-5000-tcp" {
+		t.Fatalf("caller tag must be preserved when free, got %q", got)
+	}
+}
+
+// when the caller leaves Tag empty (the local UI path) resolveInboundTag
+// falls back to generateInboundTag, which keeps the historical
+// "inbound-<port>" shape so existing routing rules don't change.
+func TestResolveInboundTag_GeneratesWhenTagEmpty(t *testing.T) {
+	setupConflictDB(t)
+
+	svc := &InboundService{}
+	in := &model.Inbound{
+		Listen:   "0.0.0.0",
+		Port:     8443,
+		Protocol: model.VLESS,
+	}
+	got, err := svc.resolveInboundTag(in, 0)
+	if err != nil {
+		t.Fatalf("resolveInboundTag: %v", err)
+	}
+	if got != "inbound-8443" {
+		t.Fatalf("expected generated inbound-8443, got %q", got)
+	}
+}
+
+// when the caller's Tag collides (e.g. a node that was used standalone
+// happens to already own the tag the central panel picked),
+// resolveInboundTag falls back to generateInboundTag rather than
+// failing — the inbound still lands, just under a slightly different
+// tag that the central will pick up via the AddInbound response.
+func TestResolveInboundTag_RegeneratesOnCollision(t *testing.T) {
+	setupConflictDB(t)
+	seedInboundConflictNode(t, "inbound-5000-tcp", "0.0.0.0", 5000, model.VLESS, `{"network":"tcp"}`, `{}`, nil)
+
+	svc := &InboundService{}
+	pushed := &model.Inbound{
+		Tag:            "inbound-5000-tcp",
+		Listen:         "0.0.0.0",
+		Port:           5000,
+		Protocol:       model.Hysteria2,
+		StreamSettings: ``,
+		Settings:       ``,
+	}
+	got, err := svc.resolveInboundTag(pushed, 0)
+	if err != nil {
+		t.Fatalf("resolveInboundTag: %v", err)
+	}
+	if got == "inbound-5000-tcp" {
+		t.Fatalf("colliding caller tag must be replaced, but resolver kept %q", got)
+	}
+}
+
 // updating an inbound must not see itself as a conflict, that's what
 // ignoreId is for.
 func TestCheckPortConflict_IgnoreSelfOnUpdate(t *testing.T) {