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.

Sign up for GitHub

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

Jump to bottom

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

adding UserAgent to Nat for description of port mapping #2310

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9deed9d
Adding UserAgent to Nat for description of port mapping
master255 May 24, 2023
996c3eb
Adding UserAgent to Nat for description of port mapping
master255 May 24, 2023
721799b
Adding UserAgent to Nat for description of port mapping
master255 May 26, 2023
41d0843
Adding UserAgent to Nat for description of port mapping
master255 May 30, 2023
f84217d
Adding UserAgent to Nat for description of port mapping
master255 May 30, 2023
6c1e4e3
Adding UserAgent to Nat for description of port mapping
master255 Jun 3, 2023
087c446
Merge pull request #1 from libp2p/master
master255 Jun 5, 2023
89290fe
Merge branch 'master' into 2307-nat-useragent
master255 Jun 5, 2023
be95951
Adding UserAgent to Nat for description of port mapping
master255 Jun 10, 2023
75f4c11
Merge pull request #2 from master255/master
master255 Aug 18, 2023
9ce892b
Merge pull request #3 from master255/master
master255 Sep 15, 2023
f4c6c04
Adding UserAgent to Nat for description of port mapping
master255 Sep 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/config.go
View file
Open in desktop
Expand Up @@ -46,7 +46,7 @@ import (
type AddrsFactory = bhost.AddrsFactory

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

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

Expand Down
4 changes: 2 additions & 2 deletions p2p/host/basic/basic_host.go
View file
Open in desktop
Expand Up @@ -131,7 +131,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, string) NATManager

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

if opts.NATManager != nil {
h.natmgr = opts.NATManager(n)
h.natmgr = opts.NATManager(n, opts.UserAgent)
}

if opts.MultiaddrResolver != nil {
Expand Down
14 changes: 7 additions & 7 deletions p2p/host/basic/natmgr.go
View file
Open in desktop
Expand Up @@ -25,8 +25,8 @@ type NATManager interface {
}

// NewNATManager creates a NAT manager.
func NewNATManager(net network.Network) NATManager {
return newNATManager(net)
func NewNATManager(net network.Network, userAgent string) NATManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicer: use the option pattern.

Copy link
Author

Choose a reason for hiding this comment

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

@marten-seemann What does that mean? In what place? Where is it better to take the variable from?

Copy link
Author

Choose a reason for hiding this comment

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

@marten-seemann Ok. Maybe this is the right way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Marten likely meant defining a new options type for the NATManager instead of extending Swarm's options set and Network interface with UserAgant getter.

Copy link
Author

Choose a reason for hiding this comment

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

@Wondertan Why not?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was talking about the option pattern, like here: https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/circuitv2/relay/options.go

Copy link
Author

Choose a reason for hiding this comment

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

@marten-seemann And you think putting the UserAgent in the Swarm - Network is not right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it doesn't really belong in the swarm. What made you put it there?

Copy link
Author

Choose a reason for hiding this comment

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

@marten-seemann I don't know. I just read the comments about what the swarm was doing and looked at which direction the data was going. If the Network is establishing connections, it needs a UserAgent.

Okay, if this idea of mine is wrong, I'll fix the code. I'll try to make an options.

Copy link
Author

@master255 master255 May 30, 2023 •
edited

Choose a reason for hiding this comment

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

@marten-seemann The new version with options is ready. But it seems to me it is too much for one parameter.
I like the second version. Through Network.

return newNATManager(net, userAgent)
}

type entry struct {
Expand All @@ -42,7 +42,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 @@ -63,7 +63,7 @@ type natManager struct {
ctxCancel context.CancelFunc
}

func newNATManager(net network.Network) *natManager {
func newNATManager(net network.Network, userAgent string) *natManager {
ctx, cancel := context.WithCancel(context.Background())
nmgr := &natManager{
net: net,
Expand All @@ -72,7 +72,7 @@ func newNATManager(net network.Network) *natManager {
tracked: make(map[entry]bool),
}
nmgr.refCount.Add(1)
go nmgr.background(ctx)
go nmgr.background(ctx, userAgent)
return nmgr
}

Expand All @@ -84,7 +84,7 @@ func (nmgr *natManager) Close() error {
return nil
}

func (nmgr *natManager) background(ctx context.Context) {
func (nmgr *natManager) background(ctx context.Context, userAgent string) {
defer nmgr.refCount.Done()

defer func() {
Expand All @@ -98,7 +98,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, userAgent)
if err != nil {
log.Info("DiscoverNAT error:", err)
return
Expand Down
6 changes: 3 additions & 3 deletions p2p/host/basic/natmgr_test.go
View file
Open in desktop
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,7 @@ func TestMapping(t *testing.T) {

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

sw := swarmt.GenSwarm(t)
defer sw.Close()
m := newNATManager(sw)
m := newNATManager(sw, "userAgent")
require.Eventually(t, func() bool {
m.natMx.Lock()
defer m.natMx.Unlock()
Expand Down
11 changes: 6 additions & 5 deletions p2p/net/nat/nat.go
View file
Open in desktop
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.

And then what will be in this place? "if" and two constructors?
https://github.com/libp2p/go-libp2p/pull/2310/files#diff-a5aa28aae96d02e4316747f7950909f36d447f7833e5a70121c3808852e5e440R117

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 •
edited

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 @@ -220,14 +222,13 @@ func (nat *NAT) background() {

func (nat *NAT) establishMapping(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(protocol, internalPort, comment, MappingDuration)
externalPort, err = nat.nat.AddPortMapping(protocol, internalPort, nat.userAgent, MappingDuration)
if err != nil {
// Some hardware does not support mappings with timeout, so try that
externalPort, err = nat.nat.AddPortMapping(protocol, internalPort, comment, 0)
externalPort, err = nat.nat.AddPortMapping(protocol, internalPort, nat.userAgent, 0)
}
nat.natmu.Unlock()

Expand Down
4 changes: 2 additions & 2 deletions p2p/net/nat/nat_test.go
View file
Open in desktop
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("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("tcp", 10000, gomock.Any(), MappingDuration).Return(1234, nil)
require.NoError(t, nat.AddMapping("tcp", 10000))
Expand Down