Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding UserAgent to Nat for description of port mapping #2310

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
2 changes: 1 addition & 1 deletion config/config.go
Expand Up @@ -47,7 +47,7 @@ import (
type AddrsFactory = bhost.AddrsFactory

// NATManagerC is a NATManager constructor.
type NATManagerC func(network.Network) bhost.NATManager
type NATManagerC func(network.Network, ...bhost.Option) (bhost.NATManager, error)

type RoutingC func(host.Host) (routing.PeerRouting, error)

Expand Down
8 changes: 6 additions & 2 deletions p2p/host/basic/basic_host.go
Expand Up @@ -129,7 +129,7 @@ type HostOpts struct {

// NATManager takes care of setting NAT port mappings, and discovering external addresses.
// If omitted, this will simply be disabled.
NATManager func(network.Network) NATManager
NATManager func(network.Network, ...Option) (NATManager, error)

// ConnManager is a libp2p connection manager
ConnManager connmgr.ConnManager
Expand Down Expand Up @@ -272,7 +272,11 @@ func NewHost(n network.Network, opts *HostOpts) (*BasicHost, error) {
}

if opts.NATManager != nil {
h.natmgr = opts.NATManager(n)
natmgr, err := opts.NATManager(n, WithUserAgent(opts.UserAgent))
if err != nil {
return nil, fmt.Errorf("failed to create NAT manager: %w", err)
}
h.natmgr = natmgr
}

if opts.MultiaddrResolver != nil {
Expand Down
26 changes: 20 additions & 6 deletions p2p/host/basic/natmgr.go
Expand Up @@ -2,6 +2,7 @@ package basichost

import (
"context"
"fmt"
"io"
"net"
"net/netip"
Expand All @@ -16,6 +17,8 @@ import (
manet "github.com/multiformats/go-multiaddr/net"
)

const defaultUserAgent = "libp2p"

// NATManager is a simple interface to manage NAT devices.
// It listens Listen and ListenClose notifications from the network.Network,
// and tries to obtain port mappings for those.
Expand All @@ -26,8 +29,8 @@ type NATManager interface {
}

// NewNATManager creates a NAT manager.
func NewNATManager(net network.Network) NATManager {
return newNATManager(net)
func NewNATManager(net network.Network, opt ...Option) (NATManager, error) {
return newNATManager(net, opt...)
}

type entry struct {
Expand All @@ -43,7 +46,7 @@ type nat interface {
}

// so we can mock it in tests
var discoverNAT = func(ctx context.Context) (nat, error) { return inat.DiscoverNAT(ctx) }
var discoverNAT = func(ctx context.Context, userAgent string) (nat, error) { return inat.DiscoverNAT(ctx, userAgent) }

// natManager takes care of adding + removing port mappings to the nat.
// Initialized with the host if it has a NATPortMap option enabled.
Expand All @@ -56,6 +59,8 @@ type natManager struct {
natMx sync.RWMutex
nat nat

userAgent string

syncFlag chan struct{} // cap: 1

tracked map[entry]bool // the bool is only used in doSync and has no meaning outside of that function
Expand All @@ -65,18 +70,27 @@ type natManager struct {
ctxCancel context.CancelFunc
}

func newNATManager(net network.Network) *natManager {
func newNATManager(net network.Network, opts ...Option) (*natManager, error) {
ctx, cancel := context.WithCancel(context.Background())
nmgr := &natManager{
net: net,
syncFlag: make(chan struct{}, 1),
ctx: ctx,
ctxCancel: cancel,
tracked: make(map[entry]bool),
userAgent: defaultUserAgent,
}

for _, opt := range opts {
err := opt(nmgr)
if err != nil {
return nil, fmt.Errorf("error applying NAT manager option: %w", err)
}
}

nmgr.refCount.Add(1)
go nmgr.background(ctx)
return nmgr
return nmgr, nil
}

// Close closes the natManager, closing the underlying nat
Expand Down Expand Up @@ -107,7 +121,7 @@ func (nmgr *natManager) background(ctx context.Context) {

discoverCtx, cancel := context.WithTimeout(ctx, 10*time.Second)
defer cancel()
natInstance, err := discoverNAT(discoverCtx)
natInstance, err := discoverNAT(discoverCtx, nmgr.userAgent)
if err != nil {
log.Info("DiscoverNAT error:", err)
return
Expand Down
8 changes: 5 additions & 3 deletions p2p/host/basic/natmgr_test.go
Expand Up @@ -20,7 +20,7 @@ func setupMockNAT(t *testing.T) (mockNAT *MockNAT, reset func()) {
ctrl := gomock.NewController(t)
mockNAT = NewMockNAT(ctrl)
origDiscoverNAT := discoverNAT
discoverNAT = func(ctx context.Context) (nat, error) { return mockNAT, nil }
discoverNAT = func(ctx context.Context, userAgent string) (nat, error) { return mockNAT, nil }
return mockNAT, func() {
discoverNAT = origDiscoverNAT
ctrl.Finish()
Expand All @@ -33,7 +33,8 @@ func TestMapping(t *testing.T) {

sw := swarmt.GenSwarm(t)
defer sw.Close()
m := newNATManager(sw)
m, err := newNATManager(sw)
require.NoError(t, err)
require.Eventually(t, func() bool {
m.natMx.Lock()
defer m.natMx.Unlock()
Expand Down Expand Up @@ -67,7 +68,8 @@ func TestAddAndRemoveListeners(t *testing.T) {

sw := swarmt.GenSwarm(t)
defer sw.Close()
m := newNATManager(sw)
m, err := newNATManager(sw)
require.NoError(t, err)
require.Eventually(t, func() bool {
m.natMx.Lock()
defer m.natMx.Unlock()
Expand Down
13 changes: 13 additions & 0 deletions p2p/host/basic/options.go
@@ -0,0 +1,13 @@
package basichost

type Option func(*natManager) error

// WithUserAgent is a natManager option that sets specific user agent for the NAT manager.
func WithUserAgent(userAgent string) Option {
return func(nmgr *natManager) error {
if userAgent != "" {
nmgr.userAgent = userAgent
}
return nil
}
}
11 changes: 6 additions & 5 deletions p2p/net/nat/nat.go
Expand Up @@ -34,7 +34,7 @@ type entry struct {
var discoverGateway = nat.DiscoverGateway

// DiscoverNAT looks for a NAT device in the network and returns an object that can manage port mappings.
func DiscoverNAT(ctx context.Context) (*NAT, error) {
func DiscoverNAT(ctx context.Context, userAgent string) (*NAT, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to change the interface of this public method?

Copy link
Author

Choose a reason for hiding this comment

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

What are the alternatives?

Copy link
Contributor

Choose a reason for hiding this comment

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

Introducing a new func.

Copy link
Author

Choose a reason for hiding this comment

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

If we create a new function, we need to designate a default userAgent somewhere in this file. The default value is set in another file.
So this constructor has no sense without the userAgent string.

Copy link
Contributor

Choose a reason for hiding this comment

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

The default should be in this file. That was the old behavior. If users want to change it they can call the new function (maybe call it DiscoverNATWithUserAgent).

This change as is is a breaking API change. I like to avoid them unless necessary. I don't think it's necessary here.

Copy link
Author

Choose a reason for hiding this comment

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

@MarcoPolo Okay. Then what to do with this one?
https://github.com/libp2p/go-libp2p/pull/2310/files#diff-a5aa28aae96d02e4316747f7950909f36d447f7833e5a70121c3808852e5e440R80

What to pass in this line? What will be the default value here?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@MarcoPolo Okay. Why shouldn't there be an incoming userAgent parameter in this function? After all, we cannot communicate with NAT without userAgent.

Copy link
Member

@sukunrt sukunrt Sep 27, 2023

Choose a reason for hiding this comment

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

Why shouldn't there be an incoming userAgent parameter in this function?

Because it's a breaking API change. Users of this function, when they update go-libp2p will need to update their usage of this function and pass in a userAgent.

It'd be nicer if we add another function DiscoverNATWithUserAgent and NatManager switches between DiscoverNAT and DiscoverNATWithUserAgent based on whether it has a user configured UserAgent or not

Copy link
Author

Choose a reason for hiding this comment

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

@sukunrt Okay. I fixed it.

natInstance, err := discoverGateway(ctx)
if err != nil {
return nil, err
Expand All @@ -60,6 +60,7 @@ func DiscoverNAT(ctx context.Context) (*NAT, error) {
mappings: make(map[entry]int),
ctx: ctx,
ctxCancel: cancel,
userAgent: userAgent,
}
nat.refCount.Add(1)
go func() {
Expand All @@ -77,7 +78,8 @@ type NAT struct {
natmu sync.Mutex
nat nat.NAT
// External IP of the NAT. Will be renewed periodically (every CacheTime).
extAddr netip.Addr
extAddr netip.Addr
userAgent string

refCount sync.WaitGroup
ctx context.Context
Expand Down Expand Up @@ -222,14 +224,13 @@ func (nat *NAT) background() {

func (nat *NAT) establishMapping(ctx context.Context, protocol string, internalPort int) (externalPort int) {
log.Debugf("Attempting port map: %s/%d", protocol, internalPort)
const comment = "libp2p"

nat.natmu.Lock()
var err error
externalPort, err = nat.nat.AddPortMapping(ctx, protocol, internalPort, comment, MappingDuration)
externalPort, err = nat.nat.AddPortMapping(ctx, protocol, internalPort, nat.userAgent, MappingDuration)
if err != nil {
// Some hardware does not support mappings with timeout, so try that
externalPort, err = nat.nat.AddPortMapping(ctx, protocol, internalPort, comment, 0)
externalPort, err = nat.nat.AddPortMapping(ctx, protocol, internalPort, nat.userAgent, 0)
}
nat.natmu.Unlock()

Expand Down
4 changes: 2 additions & 2 deletions p2p/net/nat/nat_test.go
Expand Up @@ -33,7 +33,7 @@ func TestAddMapping(t *testing.T) {
defer reset()

mockNAT.EXPECT().GetExternalAddress().Return(net.IPv4(1, 2, 3, 4), nil)
nat, err := DiscoverNAT(context.Background())
nat, err := DiscoverNAT(context.Background(), "userAgent")
require.NoError(t, err)

mockNAT.EXPECT().AddPortMapping(gomock.Any(), "tcp", 10000, gomock.Any(), MappingDuration).Return(1234, nil)
Expand All @@ -53,7 +53,7 @@ func TestRemoveMapping(t *testing.T) {
defer reset()

mockNAT.EXPECT().GetExternalAddress().Return(net.IPv4(1, 2, 3, 4), nil)
nat, err := DiscoverNAT(context.Background())
nat, err := DiscoverNAT(context.Background(), "userAgent")
require.NoError(t, err)
mockNAT.EXPECT().AddPortMapping(gomock.Any(), "tcp", 10000, gomock.Any(), MappingDuration).Return(1234, nil)
require.NoError(t, nat.AddMapping(context.Background(), "tcp", 10000))
Expand Down