-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add DisableNatPortMap option. #3798
Conversation
Redo of #3775. |
@whyrusleeping I went with |
Also, manually tested since I am not sire how to write tests for this. |
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
163d12d
to
da95e9f
Compare
@@ -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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
||
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) { |
There was a problem hiding this comment.
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.
@@ -3,4 +3,5 @@ package config | |||
type SwarmConfig struct { |
There was a problem hiding this comment.
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?
Thanks again for the PR. This is wonderful that you want to help and add more features to go-ipfs but every PR should contain unit tests. After you will add unit tests we will consider merging this. |
@hoenirvili it was also discussed before, Nat traversal is very hard to test. Currently it is not tested at all and requiring testing for this PR would require creation of test suite for nat traversal in general. |
The only somewhat reasonable way i've come up with for testing nat traversal is to have a series of docker images using a special networking setup and a upnp server in another docker image. I had very little luck figuring out how to set this up. I've even emailed developers of miniupnp asking how they do automated testing, and the general response i get is that testing for this tends to be fairly manual. @hoenirvili if you can figure out how to actually do automated testing of NAT traversal, that would be fantastic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @kevina
Add DisableNatPortMap option.
…covery-config-v2 Add DisableNatPortMap option.
No description provided.