Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 58 additions & 17 deletions pkg/tunnel/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,12 @@ func newEdgeTunnel(c *v1alpha1.EdgeTunnelConfig) (*EdgeTunnel, error) {
return nil, fmt.Errorf("failed to new conn manager: %w", err)
}

listenAddr, err := generateListenAddr(c)
ips, err := GetIPsFromInterfaces(c.ListenInterfaces, c.ExtraFilteredInterfaces)
if err != nil {
return nil, fmt.Errorf("failed to get ips from listen interfaces: %w", err)
}

listenAddr, err := generateListenAddr(c, ips)
if err != nil {
return nil, fmt.Errorf("failed to generate listenAddr: %w", err)
}
Expand All @@ -122,18 +127,19 @@ func newEdgeTunnel(c *v1alpha1.EdgeTunnelConfig) (*EdgeTunnel, error) {
}))
}

// If the relayMap does not contain any public IP, NATService will not be able to assist this non-relay node to
// identify its own network(public, private or unknown), so it needs to configure libp2p.ForceReachabilityPrivate()
if !isRelay && !relayMap.ContainsPublicIP() {
// Nodes that only listen on non-public addresses cannot rely on AutoNAT to infer
// public reachability in fronted-relay setups such as ACK+NLB, so force private reachability.
if !isRelay && (!relayMap.ContainsPublicIP() || ShouldForceReachabilityPrivate(ips)) {
klog.Infof("Configure libp2p.ForceReachabilityPrivate()")
opts = append(opts, libp2p.ForceReachabilityPrivate())
}

relayNums := len(relayMap)
if c.MaxCandidates < relayNums {
klog.Infof("MaxCandidates=%d is less than len(relayMap)=%d, set MaxCandidates to len(relayMap)",
c.MaxCandidates, relayNums)
c.MaxCandidates = relayNums
minCandidates, maxCandidates, numRelays := normalizeAutoRelayConfig(c.MaxCandidates, relayNums)
if maxCandidates != c.MaxCandidates {
klog.Infof("MaxCandidates adjusted from %d to %d (relayNums=%d)",
c.MaxCandidates, maxCandidates, relayNums)
c.MaxCandidates = maxCandidates
}
Comment on lines +138 to 143
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation for handling maxCandidates is a bit convoluted. It calculates a new maxCandidates, logs an update, modifies the input configuration c.MaxCandidates, and then uses this modified value in buildAutoRelayOpts.

For better clarity and to avoid side effects on the input configuration, it's preferable to use the local maxCandidates variable directly.

I suggest removing the update to c.MaxCandidates here, and then passing the local maxCandidates variable to buildAutoRelayOpts on line 174 like so: buildAutoRelayOpts(minCandidates, maxCandidates, numRelays)...,.

minCandidates, maxCandidates, numRelays := normalizeAutoRelayConfig(c.MaxCandidates, relayNums)
	if maxCandidates != c.MaxCandidates {
		klog.Infof("MaxCandidates adjusted from %d to %d (relayNums=%d)",
			c.MaxCandidates, maxCandidates, relayNums)
	}


// configures libp2p to use the given private network protector
Expand Down Expand Up @@ -165,9 +171,7 @@ func newEdgeTunnel(c *v1alpha1.EdgeTunnelConfig) (*EdgeTunnel, error) {
func(ctx context.Context, numPeers int) <-chan peer.AddrInfo {
return peerSource
},
autorelay.WithMinCandidates(0),
autorelay.WithMaxCandidates(c.MaxCandidates),
autorelay.WithBackoff(30*time.Second),
buildAutoRelayOpts(minCandidates, c.MaxCandidates, numRelays)...,
),
libp2p.EnableNATService(),
libp2p.EnableHolePunching(),
Expand Down Expand Up @@ -270,12 +274,7 @@ func newEdgeTunnel(c *v1alpha1.EdgeTunnelConfig) (*EdgeTunnel, error) {
return edgeTunnel, nil
}

func generateListenAddr(c *v1alpha1.EdgeTunnelConfig) (libp2p.Option, error) {
ips, err := GetIPsFromInterfaces(c.ListenInterfaces, c.ExtraFilteredInterfaces)
if err != nil {
return nil, fmt.Errorf("failed to get ips from listen interfaces: %w", err)
}

func generateListenAddr(c *v1alpha1.EdgeTunnelConfig, ips []string) (libp2p.Option, error) {
multiAddrStrings := make([]string, 0)
if c.Mode == defaults.ServerClientMode {
for _, ip := range ips {
Expand All @@ -290,3 +289,45 @@ func generateListenAddr(c *v1alpha1.EdgeTunnelConfig) (libp2p.Option, error) {
listenAddr := libp2p.ListenAddrStrings(multiAddrStrings...)
return listenAddr, nil
}

// normalizeAutoRelayConfig computes normalized autorelay parameters.
//
// Fixes the bug where WithMinCandidates(0) prevents peerSource from ever being
// triggered (issue #583):
// - minCandidates is set to at least 1 so relay_finder calls peerSource and
// enters the Reserve() path.
// - maxCandidates is at least relayNums so the candidate pool is large enough.
// - For single-relay setups, numRelays=1 avoids waiting for a second relay
// (the default desiredRelays=2 would stall indefinitely with only one relay).
func normalizeAutoRelayConfig(cfgMaxCandidates, relayNums int) (minCandidates, maxCandidates, numRelays int) {
maxCandidates = cfgMaxCandidates
if maxCandidates < relayNums {
maxCandidates = relayNums
}
// minCandidates must be >= 1; otherwise the condition
// `numCandidates < minCandidates` in relay_finder.findNodes() is always
// false and peerSource is never called.
minCandidates = 1
if maxCandidates > 0 && minCandidates > maxCandidates {
minCandidates = maxCandidates
}
// For single-relay setups, explicitly set desiredRelays=1 to avoid
// waiting forever for a second relay (default desiredRelays=2).
if relayNums == 1 {
numRelays = 1
}
return
}

// buildAutoRelayOpts constructs the autorelay option list from normalized parameters.
func buildAutoRelayOpts(minCandidates, maxCandidates, numRelays int) []autorelay.Option {
opts := []autorelay.Option{
autorelay.WithMinCandidates(minCandidates),
autorelay.WithMaxCandidates(maxCandidates),
autorelay.WithBackoff(30 * time.Second),
}
if numRelays > 0 {
opts = append(opts, autorelay.WithNumRelays(numRelays))
}
return opts
}
97 changes: 97 additions & 0 deletions pkg/tunnel/module_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package tunnel

import (
"testing"
)

func TestNormalizeAutoRelayConfig(t *testing.T) {
tests := []struct {
name string
cfgMaxCandidates int
relayNums int
wantMinCandidates int
wantMaxCandidates int
wantNumRelays int
}{
{
// root cause scenario from issue #583: single relay with default MaxCandidates;
// after the fix minCandidates must be >= 1 and numRelays must be 1
name: "single relay – min candidates fixed to 1, numRelays set to 1",
cfgMaxCandidates: 4,
relayNums: 1,
wantMinCandidates: 1,
wantMaxCandidates: 4,
wantNumRelays: 1,
},
{
// multiple relays, maxCandidates is large enough
name: "multi relay – maxCandidates sufficient, numRelays stays 0",
cfgMaxCandidates: 4,
relayNums: 3,
wantMinCandidates: 1,
wantMaxCandidates: 4,
wantNumRelays: 0,
},
{
// multiple relays, maxCandidates is too small and must be boosted to relayNums
name: "multi relay – maxCandidates boosted to relayNums",
cfgMaxCandidates: 2,
relayNums: 5,
wantMinCandidates: 1,
wantMaxCandidates: 5,
wantNumRelays: 0,
},
{
// cfgMaxCandidates=0 with single relay:
// maxCandidates should be boosted to relayNums=1, minCandidates=1
name: "zero maxCandidates with single relay – both boosted to 1",
cfgMaxCandidates: 0,
relayNums: 1,
wantMinCandidates: 1,
wantMaxCandidates: 1,
wantNumRelays: 1,
},
{
// no relay nodes configured; minCandidates=1 must still hold
name: "no relay nodes",
cfgMaxCandidates: 4,
relayNums: 0,
wantMinCandidates: 1,
wantMaxCandidates: 4,
wantNumRelays: 0,
},
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test suite for TestNormalizeAutoRelayConfig is missing a key edge case: when both cfgMaxCandidates and relayNums are 0. This case would have exposed the bug in the original implementation of normalizeAutoRelayConfig. Adding this test case will help prevent regressions.

Also, some comments in this file are in Chinese (e.g., lines 84 and 92). It would be great to translate them to English for consistency.

		},
		{
			// cfgMaxCandidates=0, no relay nodes
			// minCandidates should be 0 if maxCandidates is 0
			name:              "zero maxCandidates with no relays",
			cfgMaxCandidates:  0,
			relayNums:         0,
			wantMinCandidates: 0,
			wantMaxCandidates: 0,
			wantNumRelays:     0,
		},


for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotMin, gotMax, gotNum := normalizeAutoRelayConfig(tt.cfgMaxCandidates, tt.relayNums)
if gotMin != tt.wantMinCandidates {
t.Errorf("minCandidates: got %d, want %d", gotMin, tt.wantMinCandidates)
}
if gotMax != tt.wantMaxCandidates {
t.Errorf("maxCandidates: got %d, want %d", gotMax, tt.wantMaxCandidates)
}
if gotNum != tt.wantNumRelays {
t.Errorf("numRelays: got %d, want %d", gotNum, tt.wantNumRelays)
}
})
}
}

func TestBuildAutoRelayOpts(t *testing.T) {
t.Run("without numRelays", func(t *testing.T) {
opts := buildAutoRelayOpts(1, 4, 0)
// should contain 3 options: WithMinCandidates, WithMaxCandidates, WithBackoff
if len(opts) != 3 {
t.Errorf("expected 3 options, got %d", len(opts))
}
})

t.Run("with numRelays", func(t *testing.T) {
opts := buildAutoRelayOpts(1, 1, 1)
// should contain 4 options: WithMinCandidates, WithMaxCandidates, WithBackoff, WithNumRelays
if len(opts) != 4 {
t.Errorf("expected 4 options, got %d", len(opts))
}
})
}
41 changes: 37 additions & 4 deletions pkg/tunnel/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/libp2p/go-libp2p/p2p/transport/tcp"
ws "github.com/libp2p/go-libp2p/p2p/transport/websocket"
ma "github.com/multiformats/go-multiaddr"
manet "github.com/multiformats/go-multiaddr/net"
"k8s.io/klog/v2"

"github.com/kubeedge/edgemesh/pkg/apis/config/v1alpha1"
Expand Down Expand Up @@ -182,16 +183,48 @@ func GenerateRelayMap(relayNodes []*v1alpha1.RelayNode, protocol string, listenP
func FilterPrivateMaddr(maddrs []ma.Multiaddr) []ma.Multiaddr {
result := make([]ma.Multiaddr, 0)
for _, maddr := range maddrs {
maddrElements := strings.Split(maddr.String(), "/")
ip := maddrElements[ipIndex]
ipAddress := net.ParseIP(ip)
if !ipAddress.IsLoopback() && !ipAddress.IsPrivate() {
if manet.IsPublicAddr(maddr) || isDNSMaddr(maddr) {
result = append(result, maddr)
}
}
return result
}

func isDNSMaddr(maddr ma.Multiaddr) bool {
first, _ := ma.SplitFirst(maddr)
if first == nil {
return false
}

switch first.Protocol().Code {
case ma.P_DNS, ma.P_DNS4, ma.P_DNS6, ma.P_DNSADDR:
return true
default:
return false
}
}

func ShouldForceReachabilityPrivate(ips []string) bool {
if len(ips) == 0 {
return false
}

for _, ip := range ips {
if isPublicIP(ip) {
return false
}
}
return true
}

func isPublicIP(ip string) bool {
maddr, err := ma.NewMultiaddr(GenerateMultiAddrString(TCP, ip, 1))
if err != nil {
return false
}
return manet.IsPublicAddr(maddr)
}

func FilterCircuitMaddr(maddrs []ma.Multiaddr) []ma.Multiaddr {
result := make([]ma.Multiaddr, 0)
for _, maddr := range maddrs {
Expand Down
59 changes: 59 additions & 0 deletions pkg/tunnel/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,65 @@ func TestAddCircuitAddrsToPeer(t *testing.T) {
assertEqual(relayPeer.String(), want, t)
}

func TestFilterPrivateMaddrDropsNonRoutableRelayAddrs(t *testing.T) {
maddrs, err := StringsToMaddrs([]string{
"/ip4/47.95.1.47/tcp/20006",
"/ip4/10.112.27.223/tcp/20006",
"/ip4/169.254.20.10/tcp/20006",
"/ip4/198.18.1.255/tcp/20006",
"/dns4/nlb-9qpaugg3doihkhemjw.cn-beijing.nlb.aliyuncsslb.com/tcp/20006",
})
if !assertEqual(err, nil, t) {
t.Fatalf("failed to create multiaddrs: %v", err)
}

want, err := StringsToMaddrs([]string{
"/ip4/47.95.1.47/tcp/20006",
"/dns4/nlb-9qpaugg3doihkhemjw.cn-beijing.nlb.aliyuncsslb.com/tcp/20006",
})
if !assertEqual(err, nil, t) {
t.Fatalf("failed to create expected multiaddrs: %v", err)
}

got := FilterPrivateMaddr(maddrs)
if !assertEqual(got, want, t) {
t.Fatalf("expected only routable relay addrs %v, got %v", want, got)
}
}

func TestShouldForceReachabilityPrivate(t *testing.T) {
tests := []struct {
name string
ips []string
want bool
}{
{
name: "only private and loopback addresses",
ips: []string{"10.80.9.54", "127.0.0.1"},
want: true,
},
{
name: "public address present",
ips: []string{"10.80.9.54", "47.95.1.47"},
want: false,
},
{
name: "empty addresses",
ips: nil,
want: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := ShouldForceReachabilityPrivate(tt.ips)
if !assertEqual(got, tt.want, t) {
t.Fatalf("ShouldForceReachabilityPrivate(%v) = %v, want %v", tt.ips, got, tt.want)
}
})
}
}

func TestAppendMultiaddrs(t *testing.T) {
// generate multiAddress
var nodes = []*testNode{
Expand Down
Loading