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.

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 stun NAT type check for sudph #825

Merged
merged 39 commits into from
Aug 30, 2021
Merged

Add stun NAT type check for sudph #825

merged 39 commits into from
Aug 30, 2021

Conversation

ersonp
Copy link
Contributor

@ersonp ersonp commented Jun 28, 2021

Did you run make format && make check?
yes

Fixes #776

Changes:

  • Added a STUN check for the sudph transport.
  • Separated transports into their own modules so that they can be initialized individually and the visor will start only when at least one transport has been initialized.
  • dmsg and sudph initialize separate from other modules so that the visor doesn't have to wait for them.
  • Removed the unused public field from AddTransports.

How to test this PR:

  1. Run ./skywire-visor skywire-config.json
  2. If your visor is under a symmetric NAT you should not be able to see a sudph transport. (check the logs to see your NAT type)

@jdknives
Copy link
Member

Looks good @ersonp . Will test and merge once we have a Stun server deployed ourselves.

@ersonp
Copy link
Contributor Author

ersonp commented Jun 28, 2021

Few things are remaining such as:

  1. Add stun servers to config
  2. Put public IP and Nat type in visor summary
    Will push them soon.

@ersonp ersonp marked this pull request as ready for review June 28, 2021 17:23
pkg/snet/network.go Outdated Show resolved Hide resolved
pkg/visor/api.go Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
@ersonp
Copy link
Contributor Author

ersonp commented Jul 2, 2021

@jdknives @i-hate-nicknames I have turned stun into a module but corrently all it's related code is in pkg/visor/init. I think it should go under transport or transport/network the second one being from the snet refactor. Let me know if you think otherwise.

Copy link
Contributor

@i-hate-nicknames i-hate-nicknames left a comment

Choose a reason for hiding this comment

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

Good job overall! Also I would just remove the code that you commented out. Most likely it will be gone when snet refactoring is merged, but still a good habit

pkg/snet/network.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
pkg/snet/network.go Outdated Show resolved Hide resolved
Copy link
Contributor

@i-hate-nicknames i-hate-nicknames left a comment

Choose a reason for hiding this comment

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

Just a small comment, otherwise looks good.

Also, now that snet refactoring got merged there is no snet package and snet.Network entity, but the changes should fit perfectly into transport manager.

pkg/visor/init.go Outdated Show resolved Hide resolved
@ersonp
Copy link
Contributor Author

ersonp commented Aug 12, 2021

need to merge #842

Copy link
Contributor

@mrpalide mrpalide left a comment

Choose a reason for hiding this comment

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

Everything (just a little comment) is OK. Ready to merge.

pkg/router/router.go Outdated Show resolved Hide resolved
Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

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

Looks mostly good.

pkg/visor/api.go Show resolved Hide resolved
pkg/transport/manager.go Outdated Show resolved Hide resolved
pkg/visor/init.go Outdated Show resolved Hide resolved
pkg/visor/visor.go Outdated Show resolved Hide resolved
@jdknives
Copy link
Member

Please pull in develop and fix conflicts.

@ersonp
Copy link
Contributor Author

ersonp commented Aug 26, 2021

@jdknives Currently we are not able to use sudph in integration env in skywire-services because of the STUN check. So I think we should add either a test flag in or a test field in config to turn off the STUN check.

@jdknives jdknives merged commit 53dfd83 into skycoin:develop Aug 30, 2021
@ersonp ersonp deleted the stun-check branch April 11, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants