From 7ade9d9a1f45303da7449e52ebd7056ecea49bba Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Wed, 27 May 2026 19:14:22 +0200 Subject: [PATCH] refactor(inbound-tag): node-prefixed + transport-suffixed canonical shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tag scheme moves to "[n-]inbound-[:]-" 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- prefix is added when Inbound.NodeID != nil. - addInbound / importInbound drop their inline "inbound-" 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- 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. --- web/controller/inbound.go | 19 -------- web/service/inbound.go | 51 ++++++++++++++++------ web/service/port_conflict.go | 58 ++++++++++++------------- web/service/port_conflict_test.go | 72 +++++++++++++++++++++++++------ 4 files changed, 125 insertions(+), 75 deletions(-) diff --git a/web/controller/inbound.go b/web/controller/inbound.go index ba46e451..064d469c 100644 --- 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 diff --git a/web/service/inbound.go b/web/service/inbound.go index b0184b20..519ff97b 100644 --- a/web/service/inbound.go +++ b/web/service/inbound.go @@ -1258,9 +1258,15 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi Find(¢ral).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- at create. + tagToCentral := make(map[string]*model.Inbound, len(central)*2) + prefix := nodeTagPrefix(&nodeID) for i := range central { tagToCentral[central[i].Tag] = ¢ral[i] + if prefix != "" && strings.HasPrefix(central[i].Tag, prefix) { + tagToCentral[strings.TrimPrefix(central[i].Tag, prefix)] = ¢ral[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- + // 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 } diff --git a/web/service/port_conflict.go b/web/service/port_conflict.go index 39513bb1..c923446f 100644 --- 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-" / "inbound-:" -// shape. kept exactly so existing routing rules that reference these tags -// keep working after the upgrade. +// baseInboundTag is the legacy "inbound-" / "inbound-:" +// 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-]inbound-[:]-" 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) diff --git a/web/service/port_conflict_test.go b/web/service/port_conflict_test.go index 93c2752e..3f6a9aee 100644 --- 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-" 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--" 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-" shape so existing routing rules don't change. +// falls back to generateInboundTag, which emits the canonical +// "inbound--" 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-" 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) {