refactor(inbound-tag): node-prefixed + transport-suffixed canonical shape

Tag scheme moves to "[n<nodeID>-]inbound-[<listen>:]<port>-<transport>" so two long-standing collision classes go away on the create path: - tcp/443 and udp/443 on the same listener (independent sockets) - same listen+port living on the central panel and on a remote node Examples: local TCP 443 → inbound-443-tcp local UDP 443 → inbound-443-udp node 1 TCP 443 → n1-inbound-443-tcp Refactor: - composeInboundTag is the single source of truth, called from generateInboundTag. Transport segment is now always present (used to appear only on collision); n<id>- prefix is added when Inbound.NodeID != nil. - addInbound / importInbound drop their inline "inbound-<port>" fallback; an empty Tag now flows through resolveInboundTag, which keeps caller-supplied tags verbatim when free and otherwise delegates to generateInboundTag. - setRemoteTrafficLocked indexes tagToCentral under both the stored tag and the prefix-stripped form, so a node sending its bare tag still resolves to a row we may have rewritten at materialization. The create branch now picks between snap.Tag and the n<id>- prefixed form before falling back to the warn-once skip. - Tests updated for the always-on transport suffix, and two new cases cover the node-prefix behaviour. Existing inbounds keep their tags — only newly generated tags adopt the new shape, so user routing rules pointing at "inbound-443" still match the row they always did until the row is recreated.
2026-05-29 16:39:35 +00:00 · 2026-05-27 19:14:22 +02:00
parent d347605233
commit 7ade9d9a1f
4 changed files with 125 additions and 75 deletions
--- a/web/controller/inbound.go
+++ b/web/controller/inbound.go
@@ -2,7 +2,6 @@ package controller

 import (
 	"encoding/json"
-	"fmt"
 	"net"
 	"strconv"
 	"strings"
@@ -145,17 +144,6 @@ func (a *InboundController) addInbound(c *gin.Context) {
 	if inbound.NodeID != nil && *inbound.NodeID == 0 {
 		inbound.NodeID = nil
 	}
-	// When the central panel deploys an inbound to a remote node, it sends
-	// the Tag pre-computed (so both DBs agree on the identifier). Local
-	// UI submits don't include a Tag — we compute one from listen+port
-	// using the original collision-avoiding scheme.
-	if inbound.Tag == "" {
-		if inbound.Listen == "" || inbound.Listen == "0.0.0.0" || inbound.Listen == "::" || inbound.Listen == "::0" {
-			inbound.Tag = fmt.Sprintf("inbound-%v", inbound.Port)
-		} else {
-			inbound.Tag = fmt.Sprintf("inbound-%v:%v", inbound.Listen, inbound.Port)
-		}
-	}

 	inbound, needRestart, err := a.inboundService.AddInbound(inbound)
 	if err != nil {
@@ -338,13 +326,6 @@ func (a *InboundController) importInbound(c *gin.Context) {
 	if inbound.NodeID != nil && *inbound.NodeID == 0 {
 		inbound.NodeID = nil
 	}
-	if inbound.Tag == "" {
-		if inbound.Listen == "" || inbound.Listen == "0.0.0.0" || inbound.Listen == "::" || inbound.Listen == "::0" {
-			inbound.Tag = fmt.Sprintf("inbound-%v", inbound.Port)
-		} else {
-			inbound.Tag = fmt.Sprintf("inbound-%v:%v", inbound.Listen, inbound.Port)
-		}
-	}

 	for index := range inbound.ClientStats {
 		inbound.ClientStats[index].Id = 0
--- a/web/service/inbound.go
+++ b/web/service/inbound.go
@@ -1258,9 +1258,15 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
 		Find(&central).Error; err != nil {
 		return false, err
 	}
-	tagToCentral := make(map[string]*model.Inbound, len(central))
+	// Index under both stored tag and the prefix-stripped form so a snap's
+	// bare tag resolves whether or not we rewrote it with n<id>- at create.
+	tagToCentral := make(map[string]*model.Inbound, len(central)*2)
+	prefix := nodeTagPrefix(&nodeID)
 	for i := range central {
 		tagToCentral[central[i].Tag] = &central[i]
+		if prefix != "" && strings.HasPrefix(central[i].Tag, prefix) {
+			tagToCentral[strings.TrimPrefix(central[i].Tag, prefix)] = &central[i]
+		}
 	}

 	var centralClientStats []xray.ClientTraffic
@@ -1317,28 +1323,44 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi

 		c, ok := tagToCentral[snapIb.Tag]
 		if !ok {
-			var owner model.Inbound
-			if err := tx.Where("tag = ?", snapIb.Tag).First(&owner).Error; err == nil {
+			// Try snap.Tag first; on collision fall back to the n<id>-
+			// prefixed form so local+node can both own the same port.
+			pickFreeTag := func() (string, error) {
+				candidates := []string{snapIb.Tag}
+				if prefix != "" && !strings.HasPrefix(snapIb.Tag, prefix) {
+					candidates = append(candidates, prefix+snapIb.Tag)
+				}
+				for _, t := range candidates {
+					var owner model.Inbound
+					err := tx.Where("tag = ?", t).First(&owner).Error
+					if errors.Is(err, gorm.ErrRecordNotFound) {
+						return t, nil
+					}
+					if err != nil {
+						return "", err
+					}
+				}
+				return "", nil
+			}
+			chosenTag, err := pickFreeTag()
+			if err != nil {
+				logger.Warningf("setRemoteTraffic: check tag %q failed: %v", snapIb.Tag, err)
+				continue
+			}
+			if chosenTag == "" {
 				key := fmt.Sprintf("%d:%s", nodeID, snapIb.Tag)
 				if _, seen := reportedRemoteTagConflict.LoadOrStore(key, struct{}{}); !seen {
-					ownerLabel := "the local panel"
-					if owner.NodeID != nil {
-						ownerLabel = fmt.Sprintf("node #%d", *owner.NodeID)
-					}
 					logger.Warningf(
-						"setRemoteTraffic: tag %q from node %d collides with inbound owned by %s — skipping (rename one side to remove the duplicate)",
-						snapIb.Tag, nodeID, ownerLabel,
+						"setRemoteTraffic: tag %q from node %d collides with an existing inbound even after the n%d- prefix — skipping (rename one side to remove the duplicate)",
+						snapIb.Tag, nodeID, nodeID,
 					)
 				}
 				continue
-			} else if !errors.Is(err, gorm.ErrRecordNotFound) {
-				logger.Warningf("setRemoteTraffic: check tag %q failed: %v", snapIb.Tag, err)
-				continue
 			}
 			newIb := model.Inbound{
 				UserId:         defaultUserId,
 				NodeID:         &nodeID,
-				Tag:            snapIb.Tag,
+				Tag:            chosenTag,
 				Listen:         snapIb.Listen,
 				Port:           snapIb.Port,
 				Protocol:       snapIb.Protocol,
@@ -1358,6 +1380,9 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
 				continue
 			}
 			tagToCentral[snapIb.Tag] = &newIb
+			if newIb.Tag != snapIb.Tag {
+				tagToCentral[newIb.Tag] = &newIb
+			}
 			structuralChange = true
 			continue
 		}
--- a/web/service/port_conflict.go
+++ b/web/service/port_conflict.go
@@ -223,9 +223,9 @@ func sameNode(a, b *int) bool {
 	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.
+// baseInboundTag is the legacy "inbound-<port>" / "inbound-<listen>:<port>"
+// shape still emitted by node-side xray imports that pre-date the
+// transport-aware naming; kept as a probe shape in setRemoteTrafficLocked.
 func baseInboundTag(listen string, port int) string {
 	if isAnyListen(listen) {
 		return fmt.Sprintf("inbound-%v", port)
@@ -233,10 +233,6 @@ func baseInboundTag(listen string, port int) string {
 	return fmt.Sprintf("inbound-%v:%v", listen, port)
 }

-// transportTagSuffix turns a transport mask into a short, stable string.
-// used both for generateInboundTag's disambiguation ("inbound-443-udp"
-// when the base "inbound-443" is taken on a coexisting transport) and
-// for the L4 hint in portConflictDetail's user-facing error message.
 func transportTagSuffix(b transportBits) string {
 	switch b {
 	case transportTCP:
@@ -249,29 +245,32 @@ func transportTagSuffix(b transportBits) string {
 	return "any"
 }

-// generateInboundTag picks a tag for the inbound that doesn't collide with
-// any existing row. for the common single-inbound-per-port case the tag
-// stays exactly as before ("inbound-443"), so user routing rules don't
-// silently change shape on upgrade. only when a same-port neighbour
-// already owns the base tag (now possible because tcp/443 and udp/443 can
-// coexist after the transport-aware port check) does this append a
-// transport suffix like "inbound-443-udp".
-//
-// ignoreId is the inbound's own id during update so it doesn't see itself
-// as a collision; pass 0 on add.
-func (s *InboundService) generateInboundTag(inbound *model.Inbound, ignoreId int) (string, error) {
-	base := baseInboundTag(inbound.Listen, inbound.Port)
-	exists, err := s.tagExists(base, ignoreId)
-	if err != nil {
-		return "", err
-	}
-	if !exists {
-		return base, nil
+// nodeTagPrefix scopes a tag to one remote node so the same listen+port
+// can live on the central panel and on a node without bumping the global
+// UNIQUE(inbounds.tag) constraint. nil → "" (local panel).
+func nodeTagPrefix(nodeID *int) string {
+	if nodeID == nil {
+		return ""
 	}
+	return fmt.Sprintf("n%d-", *nodeID)
+}

-	suffix := transportTagSuffix(inboundTransports(inbound.Protocol, inbound.StreamSettings, inbound.Settings))
-	candidate := base + "-" + suffix
-	exists, err = s.tagExists(candidate, ignoreId)
+// composeInboundTag returns the canonical
+// "[n<id>-]inbound-[<listen>:]<port>-<transport>" shape used for every
+// newly created inbound. The transport segment lets tcp/443 and udp/443
+// coexist; the node prefix lets the same port live on local + node.
+func composeInboundTag(listen string, port int, nodeID *int, bits transportBits) string {
+	return nodeTagPrefix(nodeID) + baseInboundTag(listen, port) + "-" + transportTagSuffix(bits)
+}
+
+// generateInboundTag returns a free tag in the canonical shape. ignoreId
+// is the inbound's own id on update so it doesn't see itself as taken;
+// pass 0 on add. Numeric suffix fallback is defensive — the port check
+// should have already blocked an exact-collision insert.
+func (s *InboundService) generateInboundTag(inbound *model.Inbound, ignoreId int) (string, error) {
+	bits := inboundTransports(inbound.Protocol, inbound.StreamSettings, inbound.Settings)
+	candidate := composeInboundTag(inbound.Listen, inbound.Port, inbound.NodeID, bits)
+	exists, err := s.tagExists(candidate, ignoreId)
 	if err != nil {
 		return "", err
 	}
@@ -279,9 +278,6 @@ func (s *InboundService) generateInboundTag(inbound *model.Inbound, ignoreId int
 		return candidate, nil
 	}

-	// the transport-aware port check should have already blocked this
-	// path, but guard anyway so a unique-constraint failure doesn't reach
-	// the user as an opaque sqlite error.
 	for i := 2; i < 100; i++ {
 		c := fmt.Sprintf("%s-%d", candidate, i)
 		exists, err = s.tagExists(c, ignoreId)
--- a/web/service/port_conflict_test.go
+++ b/web/service/port_conflict_test.go
@@ -292,8 +292,9 @@ func TestGenerateInboundTag_DisambiguatesByTransportOnSamePort(t *testing.T) {
 	}
 }

-// when the port is free, the historical "inbound-<port>" shape is kept
-// so existing routing rules don't change shape on upgrade.
+// when the port is free, the canonical tag includes the transport
+// suffix so tcp/8443 and udp/8443 get distinct tags out of the box
+// (no collision-driven retry needed at INSERT time).
 func TestGenerateInboundTag_KeepsBaseTagWhenFree(t *testing.T) {
 	setupConflictDB(t)

@@ -307,19 +308,21 @@ func TestGenerateInboundTag_KeepsBaseTagWhenFree(t *testing.T) {
 	if err != nil {
 		t.Fatalf("generateInboundTag: %v", err)
 	}
-	if got != "inbound-8443" {
-		t.Fatalf("expected inbound-8443, got %q", got)
+	if got != "inbound-8443-tcp" {
+		t.Fatalf("expected inbound-8443-tcp, got %q", got)
 	}
 }

 // updating an inbound on its own port must not flag its own tag as
-// taken, that's what ignoreId is for.
+// taken, that's what ignoreId is for. Seeds with the canonical
+// "inbound-<port>-<transport>" shape so the self-update returns the
+// same tag verbatim.
 func TestGenerateInboundTag_IgnoresSelfOnUpdate(t *testing.T) {
 	setupConflictDB(t)
-	seedInboundConflict(t, "inbound-443", "0.0.0.0", 443, model.VLESS, `{"network":"tcp"}`, `{}`)
+	seedInboundConflict(t, "inbound-443-tcp", "0.0.0.0", 443, model.VLESS, `{"network":"tcp"}`, `{}`)

 	var existing model.Inbound
-	if err := database.GetDB().Where("tag = ?", "inbound-443").First(&existing).Error; err != nil {
+	if err := database.GetDB().Where("tag = ?", "inbound-443-tcp").First(&existing).Error; err != nil {
 		t.Fatalf("read seeded row: %v", err)
 	}

@@ -328,7 +331,7 @@ func TestGenerateInboundTag_IgnoresSelfOnUpdate(t *testing.T) {
 	if err != nil {
 		t.Fatalf("generateInboundTag: %v", err)
 	}
-	if got != "inbound-443" {
+	if got != "inbound-443-tcp" {
 		t.Fatalf("self-update must keep base tag, got %q", got)
 	}
 }
@@ -424,8 +427,8 @@ func TestResolveInboundTag_RespectsCallerTagWhenFree(t *testing.T) {
 }

 // 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.
+// falls back to generateInboundTag, which emits the canonical
+// "inbound-<port>-<transport>" shape.
 func TestResolveInboundTag_GeneratesWhenTagEmpty(t *testing.T) {
 	setupConflictDB(t)

@@ -439,8 +442,8 @@ func TestResolveInboundTag_GeneratesWhenTagEmpty(t *testing.T) {
 	if err != nil {
 		t.Fatalf("resolveInboundTag: %v", err)
 	}
-	if got != "inbound-8443" {
-		t.Fatalf("expected generated inbound-8443, got %q", got)
+	if got != "inbound-8443-tcp" {
+		t.Fatalf("expected generated inbound-8443-tcp, got %q", got)
 	}
 }

@@ -471,6 +474,51 @@ func TestResolveInboundTag_RegeneratesOnCollision(t *testing.T) {
 	}
 }

+// inbounds bound to a remote node get the canonical tag prefixed with
+// "n<id>-" so the same listen+port+transport can live on the central
+// panel and on the node simultaneously without bumping the global
+// UNIQUE(inbounds.tag) constraint.
+func TestGenerateInboundTag_NodePrefix(t *testing.T) {
+	setupConflictDB(t)
+
+	svc := &InboundService{}
+	in := &model.Inbound{
+		Listen:   "0.0.0.0",
+		Port:     443,
+		Protocol: model.VLESS,
+		NodeID:   intPtr(1),
+	}
+	got, err := svc.generateInboundTag(in, 0)
+	if err != nil {
+		t.Fatalf("generateInboundTag: %v", err)
+	}
+	if got != "n1-inbound-443-tcp" {
+		t.Fatalf("expected n1-inbound-443-tcp, got %q", got)
+	}
+}
+
+// a node-prefixed inbound shouldn't collide with a same-port local one:
+// the prefix scopes the tag to that specific node.
+func TestGenerateInboundTag_NodePrefixedDoesNotCollideWithLocal(t *testing.T) {
+	setupConflictDB(t)
+	seedInboundConflict(t, "inbound-443-tcp", "0.0.0.0", 443, model.VLESS, `{"network":"tcp"}`, `{}`)
+
+	svc := &InboundService{}
+	in := &model.Inbound{
+		Listen:   "0.0.0.0",
+		Port:     443,
+		Protocol: model.VLESS,
+		NodeID:   intPtr(1),
+	}
+	got, err := svc.generateInboundTag(in, 0)
+	if err != nil {
+		t.Fatalf("generateInboundTag: %v", err)
+	}
+	if got != "n1-inbound-443-tcp" {
+		t.Fatalf("expected n1-inbound-443-tcp, got %q", got)
+	}
+}
+
 // updating an inbound must not see itself as a conflict, that's what
 // ignoreId is for.
 func TestCheckPortConflict_IgnoreSelfOnUpdate(t *testing.T) {