Skip to content

core: add NatPortMap and default it to true #3775

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion cmd/ipfs/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ environment variable:
return
}

conf = &config.Config{}
conf = config.NewConfig()
if err := json.NewDecoder(confFile).Decode(conf); err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand Down
4 changes: 2 additions & 2 deletions core/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (cfg *BuildCfg) fillDefaults() error {
}

func defaultRepo(dstore repo.Datastore) (repo.Repo, error) {
c := cfg.Config{}
c := cfg.NewConfig()
priv, pub, err := ci.GenerateKeyPairWithReader(ci.RSA, 1024, rand.Reader)
if err != nil {
return nil, err
Expand All @@ -109,7 +109,7 @@ func defaultRepo(dstore repo.Datastore) (repo.Repo, error) {

return &repo.Mock{
D: dstore,
C: c,
C: *c,
}, nil
}

Expand Down
17 changes: 13 additions & 4 deletions core/core.go
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{NatPortMap: cfg.Swarm.NatPortMap})
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 {
NatPortMap 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) {

// 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)
newOpts := []interface{}{bwr}
Copy link
Member

Choose a reason for hiding this comment

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

probably call this hostOpts

if opts.NatPortMap {
newOpts = append(newOpts, p2pbhost.NATPortMap)
}

host := p2pbhost.New(network, newOpts)

return host, nil
}
Expand Down
2 changes: 1 addition & 1 deletion core/mock/mock.go
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
21 changes: 18 additions & 3 deletions repo/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,20 @@ type Config struct {
Experimental Experiments
}

// NewConfig creates any new Config with all the values set the
// default
func NewConfig() *Config {
Copy link
Member

Choose a reason for hiding this comment

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

I would call this NewDefaultConfig

conf := &Config{}
conf.SetDefaults()
return conf
}

func (conf *Config) SetDefaults() {
// sets defaults other that are not the go-standard of
// false, 0, or the empty string
conf.Swarm.NatPortMap = true
}

const (
// DefaultPathName is the default config dir name
DefaultPathName = ".ipfs"
Expand Down Expand Up @@ -95,11 +109,11 @@ func FromMap(v map[string]interface{}) (*Config, error) {
if err := json.NewEncoder(buf).Encode(v); err != nil {
return nil, err
}
var conf Config
if err := json.NewDecoder(buf).Decode(&conf); err != nil {
conf := NewConfig()
if err := json.NewDecoder(buf).Decode(conf); err != nil {
return nil, fmt.Errorf("Failure to decode config: %s", err)
}
return &conf, nil
return conf, nil
}

func ToMap(conf *Config) (map[string]interface{}, error) {
Expand All @@ -113,3 +127,4 @@ func ToMap(conf *Config) (map[string]interface{}, error) {
}
return m, nil
}

1 change: 1 addition & 0 deletions repo/config/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ func Init(out io.Writer, nBitsForKeypair int) (*Config, error) {
Interval: "12h",
},
}
conf.SetDefaults()

return conf, nil
}
Expand Down
1 change: 1 addition & 0 deletions repo/config/swarm.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ package config
type SwarmConfig struct {
AddrFilters []string
DisableBandwidthMetrics bool
NatPortMap bool // default: true
}
7 changes: 4 additions & 3 deletions repo/fsrepo/serialize/serialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,12 @@ func Load(filename string) (*config.Config, error) {
return nil, errors.New("ipfs not initialized, please run 'ipfs init'")
}

var cfg config.Config
err := ReadConfigFile(filename, &cfg)
cfg := config.NewConfig()

Copy link
Member

Choose a reason for hiding this comment

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

I wouldnt add a space here

err := ReadConfigFile(filename, cfg)
if err != nil {
return nil, err
}

return &cfg, err
return cfg, err
}
47 changes: 47 additions & 0 deletions test/sharness/t0023-config-default.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#!/bin/sh
#
# Copyright (c) 2014 Christian Couder
# MIT Licensed; see the LICENSE file in this repository.
#

test_description="Test init command with default config"

. lib/test-lib.sh

test_init_ipfs

test_expect_success "Swarm.NatPortMap set to true in inital config" '
echo "true" > expected &&
ipfs config Swarm.NatPortMap > actual &&
test_cmp expected actual
'

test_expect_success "Swarm.NatPortMap has line in config file" '
grep -q NatPortMap .ipfs/config
Copy link
Member

Choose a reason for hiding this comment

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

redirect output to devnull

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what the -q is for.

Copy link
Member

Choose a reason for hiding this comment

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

ah, gotcha

'

test_expect_success "remove Swarm.NatPortMap from config file" '
sed -i "s/NatPortMap/XxxYyyyZzz/" .ipfs/config
Copy link
Member

Choose a reason for hiding this comment

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

use $IPFS_PATH

'

test_expect_success "load config file by replacing a unrelated key" '
ipfs config --json Swarm.DisableBandwidthMetrics false
Copy link
Member

Choose a reason for hiding this comment

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

Add a note here:

Note: this is relying on an implementation detail where setting a config field loads the json blob into the config struct, causing defaults to be properly set. Simply 'reading' the config would read the json directly and not cause defaults to be reset

'

test_expect_success "Swarm.NatPortMap set to true in new config" '
echo "true" > expected &&
ipfs config Swarm.NatPortMap > actual &&
test_cmp expected actual
'

test_expect_success "reset Swarm group to default values" '
ipfs config --json Swarm {}
'

test_expect_success "Swarm.NatPortMap still set to true in reset config" '
echo "true" > expected &&
ipfs config Swarm.NatPortMap > actual &&
test_cmp expected actual
'

test_done