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.

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
Closed
wants to merge 3 commits into from
Closed

core: add NatPortMap and default it to true #3775

wants to merge 3 commits into from

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Mar 10, 2017

Closes #2964

License: MIT
Signed-off-by: Kevin Atkinson [email protected]

@kevina kevina added the status/in-progress In progress label Mar 10, 2017
@kevina
Copy link
Contributor Author

kevina commented Mar 10, 2017

@whyrusleeping defaulting NatPortMap to be true turned out to be more complicated that you outlined. In order for it to be done reliably it needs to be set to true when ever a new Config struct is created. I attempted to do this, but I am sure I didn't get all the case.

Other than that I followed your outline.

This still needs tests that the actual NAT discovery is turned off, but I am not sure how to do that. Hints welcome.

@kevina
core: add NatPortMap and default it to true
5082f83 
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@kevina kevina force-pushed the kevina/nat-discovery-config branch from 4af66e5 to 5082f83 Compare March 10, 2017 18:46
whyrusleeping
whyrusleeping reviewed Mar 11, 2017
View reviewed changes
core/core.go Outdated
@@ -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

whyrusleeping
whyrusleeping reviewed Mar 11, 2017
View reviewed changes
repo/config/config.go Outdated
@@ -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

whyrusleeping
whyrusleeping reviewed Mar 11, 2017
View reviewed changes
repo/fsrepo/serialize/serialize.go Outdated
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

whyrusleeping
whyrusleeping reviewed Mar 11, 2017
View reviewed changes
test/sharness/t0023-config-default.sh Outdated
'

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

whyrusleeping
whyrusleeping reviewed Mar 11, 2017
View reviewed changes
test/sharness/t0023-config-default.sh Outdated
'

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

whyrusleeping
whyrusleeping reviewed Mar 11, 2017
View reviewed changes
test/sharness/t0023-config-default.sh Outdated
'

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

whyrusleeping
whyrusleeping requested changes Mar 11, 2017
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

A few comments

@whyrusleeping whyrusleeping added this to the Ipfs 0.4.8 milestone Mar 11, 2017
kevina added 2 commits March 11, 2017 17:32
@kevina
core constructPeerHost: bug fix
3c73aa7 
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@kevina
Address CR feedback
e4c16c2 
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@kevina
Copy link
Contributor Author

kevina commented Mar 11, 2017

Addressed CR feedback. I am not going to be comfortable with the PR until we figure out how to test for NAT discovery. I caught a rather serious bug that should of caused tests to fail in constructPeerHost. This seams to indicate that NAT discovery is not tested at all.

@whyrusleeping
Copy link
Member

@kevina NAT traversal is not tested, yes. I've looked into testing this (spending a significant amount of time in doing so) and came up empty. It seems that even nat traversal libraries don't have proper tests.

@kevina
Copy link
Contributor Author

kevina commented Mar 12, 2017

Okay than I am going to do some hackish manual testing to make sure that the setting is at least being honored. I will remove the WIP from the title when that is done.

@kevina kevina changed the title WIP: core: add NatPortMap and default it to true core: add NatPortMap and default it to true Mar 15, 2017
@kevina
Copy link
Contributor Author

kevina commented Mar 15, 2017

All right, I tested it manually and the settings are being honored.

I not sure I like how the default to true is being handled but if @whyrusleeping and @Kubuxu are okay with it than so am I.

@whyrusleeping whyrusleeping requested a review from Kubuxu March 15, 2017 03:51
@whyrusleeping
Copy link
Member

@Kubuxu What do you think?

@Kubuxu
Copy link
Member

Kubuxu commented Mar 15, 2017

This just highlights defectiveness of config system to me. What do you think about shifting to default + overrides config? If we decide for it, I would love to see some other project that did something similar in past and it worked out well.

@kevina
Copy link
Contributor Author

kevina commented Mar 15, 2017

Or we could change the option to NoNatPortMap until we design a better config system. That is the option I would recommend.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 15, 2017

@whyrusleeping your call

@whyrusleeping
Copy link
Member

Alright, we can use NoNatPortMap.

Lets also make sure to add this field to the config options document in docs/config.md

@kevina kevina mentioned this pull request Mar 18, 2017
Merged
@kevina
Copy link
Contributor Author

kevina commented Mar 18, 2017

Because removing the default to true was the majority of this pull request I decided to redo the P.R. as #3798. Closing. Please don't delete the branch.

@kevina kevina closed this Mar 18, 2017
@kevina kevina removed the status/in-progress In progress label Mar 18, 2017
@whyrusleeping whyrusleeping deleted the kevina/nat-discovery-config branch March 24, 2017 19:45
@kevina
Copy link
Contributor Author

kevina commented Mar 25, 2017

@whyrusleeping is there a reason you deleted this branch when I asked "Please don't delete the branch." We (or at least me) might want to refer back to it. I thought the idea was for old branched to be moved to go-ipfs-archived. For some reason GitHub isn't allowing me to restore this branch so I re-pushed my code from my local repo.

@whyrusleeping
Copy link
Member

@kevina whoops. I just compulsively click the delete branch button

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyrusleeping whyrusleeping whyrusleeping requested changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Ipfs 0.4.8
Development

Successfully merging this pull request may close these issues.
3 participants
@kevina @whyrusleeping @Kubuxu