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

Add DisableNatPortMap option. #3798

Merged
merged 1 commit into from
Mar 22, 2017
Merged

Add DisableNatPortMap option. #3798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 13 additions & 4 deletions core/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ func (n *IpfsNode) startOnlineServices(ctx context.Context, routingOption Routin
}

peerhost, err := hostOption(ctx, n.Identity, n.Peerstore, n.Reporter,
addrfilter, tpt, protec)
addrfilter, tpt, protec, &ConstructPeerHostOpts{DisableNatPortMap: cfg.Swarm.DisableNatPortMap})
if err != nil {
return err
}
Expand Down Expand Up @@ -709,12 +709,16 @@ func listenAddresses(cfg *config.Config) ([]ma.Multiaddr, error) {
return listen, nil
}

type HostOption func(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, tpt smux.Transport, protc ipnet.Protector) (p2phost.Host, error)
type ConstructPeerHostOpts struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make a new type if you don't attach any method? Why not pass it simple as a boolean param? (For the constructPeerHost function down at line 721?

Copy link
Member

Choose a reason for hiding this comment

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

Because HostOption func argument list is getting very long and in other occasion we decided to stop adding params and just add one struct. The existing parameters will be moved to this struct on other occasion.

DisableNatPortMap bool
}

type HostOption func(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, tpt smux.Transport, protc ipnet.Protector, opts *ConstructPeerHostOpts) (p2phost.Host, error)

var DefaultHostOption HostOption = constructPeerHost

// isolates the complex initialization steps
func constructPeerHost(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, tpt smux.Transport, protec ipnet.Protector) (p2phost.Host, error) {
func constructPeerHost(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, tpt smux.Transport, protec ipnet.Protector, opts *ConstructPeerHostOpts) (p2phost.Host, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the function contains more than 3, 4 params make a new struct type like constructParams or constructConfig and pass it rather than use this long lists of params.
This way we group things much more tighter and make notify the caller that, he needs to use a single type that holds multiple params in order to call the function. If the func if is not exported make {name}{Params/Cfg-prefix} type not exported also.
We don't want to expose anymore types or functions that we don't use in other packages, because every function/type that are exposed comes with more difficult a cost of maintaining. We should try to avoid any unnecessary burden for the caller and make a stable well defined API.


// no addresses to begin with. we'll start later.
swrm, err := swarm.NewSwarmWithProtector(ctx, nil, id, ps, protec, tpt, bwr)
Expand All @@ -728,7 +732,12 @@ func constructPeerHost(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr
network.Swarm().Filters.AddDialFilter(f)
}

host := p2pbhost.New(network, p2pbhost.NATPortMap, bwr)
hostOpts := []interface{}{bwr}
if !opts.DisableNatPortMap {
hostOpts = append(hostOpts, p2pbhost.NATPortMap)
}

host := p2pbhost.New(network, hostOpts...)

return host, nil
}
Expand Down
2 changes: 1 addition & 1 deletion core/mock/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func NewMockNode() (*core.IpfsNode, error) {
}

func MockHostOption(mn mocknet.Mocknet) core.HostOption {
return func(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, _ smux.Transport, _ ipnet.Protector) (host.Host, error) {
return func(ctx context.Context, id peer.ID, ps pstore.Peerstore, bwr metrics.Reporter, fs []*net.IPNet, _ smux.Transport, _ ipnet.Protector, _ *core.ConstructPeerHostOpts) (host.Host, error) {
return mn.AddPeerWithPeerstore(id, ps)
}
}
Expand Down
5 changes: 4 additions & 1 deletion docs/config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,10 @@ See https://github.com/ipfs/go-ipfs/issues/1226#issuecomment-120494604 for more
- `DisableBandwidthMetrics`
A boolean value that when set to true, will cause ipfs to not keep track of
bandwidth metrics. Disabling bandwidth metrics can lead to a slight performance
improvement, as well as a reduction in memory usage.
improvement, as well as a reduction in memory usage.

- `DisableNatPortMap`
Disable NAT discovery.

## `Tour`
Unused.
1 change: 1 addition & 0 deletions repo/config/swarm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ package config
type SwarmConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please also add doc strings if we are already here?

AddrFilters []string
DisableBandwidthMetrics bool
DisableNatPortMap bool
}