-
Notifications
You must be signed in to change notification settings - Fork 124
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 behavior test mode. #42
Conversation
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.
Thanks for the PR. Some comments are below, but at a high level, I am not quite sure the benefit of having users specify addresses while using the default port. I know ssh
and telnet
commands do so, but their use cases always require an address and the address could be different for each run; having a default port can let users not type a port every time. However, stun is not the same: users can use the same address/port for all runs on a particular computer and they can always copy the port while copying the address. Can you help me understand it a little bit more?
stun/client.go
Outdated
if c.serverAddr == "" { | ||
c.SetServerAddr(DefaultServerAddr) | ||
} |
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.
deleting this will make us change stun.NewClient().Discover()
in
Line 57 in 4e5e57c
nat, host, err := stun.NewClient().Discover() |
s:= stun.NewClient().SetServerAddr("")
s.Discover()
which is not very good
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.
There is a bug need to fix.
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.
I think we should fix this before merging the PR
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.
Your are right, no need to change it.
type NATBehavior struct { | ||
MapBhvType NATBhvType // MappingBehaviorType | ||
FltBhvType NATBhvType // FilteringBehaviorType | ||
} |
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.
Whats up with these cryptic names? Are you paying per character?
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.
Thanks for response. It's just some ridiculous thoughts: don't want it too long. Is it better use MType and FType?
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.
Should I move these to a test branch? There may need much more adjust.
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.
The type is already called NATBehaviour, why can't you just call them FilteringType and BehaviourType?
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.
Good advice.
524123f
to
6baec2b
Compare
Just want add the behavior test mode, so remove other stuff. |
Here is some test.
|
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.
Change to MappingType and FilteringType.
type NATBehavior struct { | ||
MapBhvType NATBhvType // MappingBehaviorType | ||
FltBhvType NATBhvType // FilteringBehaviorType | ||
} |
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.
Good advice.
Add behavior test mode. Add behavior test mode.
if err != nil { | ||
return nil, err | ||
} | ||
if resp2.mappedAddr.IP() == resp1.mappedAddr.IP() && |
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.
got the following error here
➜ go-stun git:(test) ./go-stun -b -vvv
2022/01/23 20:59:42 Do Test1
2022/01/23 20:59:42 Send To: 216.93.246.18:3478
2022/01/23 20:59:42
00000000 00 01 00 18 21 12 a4 42 b3 2c 4d cc e4 65 eb 60 |....!..B.,M..e.`|
00000010 ed 9b ca 89 80 22 00 0c 53 74 75 6e 43 6c 69 65 |....."..StunClie|
00000020 6e 74 00 00 80 28 00 04 65 f9 e8 54 |nt...(..e..T|
2022/01/23 20:59:42
00000000 01 01 00 48 21 12 a4 42 b3 2c 4d cc e4 65 eb 60 |...H!..B.,M..e.`|
00000010 ed 9b ca 89 00 01 00 08 00 01 b9 88 63 40 4c 62 |............c@Lb|
00000020 00 04 00 08 00 01 b9 84 7d e7 90 f9 00 05 00 08 |........}.......|
00000030 00 01 c4 24 6b cd bb 39 80 20 00 08 00 01 24 00 |...$k..9. ....$.|
00000040 1f 0a 61 f7 80 22 00 14 56 6f 76 69 64 61 2e 6f |..a.."..Vovida.o|
00000050 72 67 20 30 2e 39 38 2d 43 50 43 00 |rg 0.98-CPC.|
2022/01/23 20:59:42 Received: {packet nil: false, local: 62.24.197.181:1298, remote: 216.93.246.18:3478, changed: 107.205.187.57:50212, other: <nil>, identical: false}
2022/01/23 20:59:42 Do Test2
2022/01/23 20:59:42 Send To: 107.205.187.57:3478
2022/01/23 20:59:42
00000000 00 01 00 18 21 12 a4 42 4f 8d 2f a4 3c db f5 5e |....!..BO./.<..^|
00000010 69 60 0f 09 80 22 00 0c 53 74 75 6e 43 6c 69 65 |i`..."..StunClie|
00000020 6e 74 00 00 80 28 00 04 a8 8f 35 b7 |nt...(....5.|
2022/01/23 20:59:51 Received: Nil
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x110ead2]
goroutine 1 [running]:
github.com/ccding/go-stun/stun.(*Client).behaviorTest(0xc00011bef8, 0x117b480, 0xc000010038, 0xc0000127e0, 0x0, 0x0, 0x0)
/Users/cong/go/src/github.com/ccding/go-stun/stun/discover.go:204 +0x232
github.com/ccding/go-stun/stun.(*Client).BehaviorTest(0xc000045ef8, 0x0, 0x0, 0x0)
/Users/cong/go/src/github.com/ccding/go-stun/stun/client.go:120 +0xe5
main.behaviorTest(0xc000045ef8)
/Users/cong/go/src/github.com/ccding/go-stun/main.go:61 +0x45
main.main()
/Users/cong/go/src/github.com/ccding/go-stun/main.go:41 +0x6a5
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.
Also local: 62.24.197.181:1298
is not my local IP address
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.
It should be nat blocked , but will be detected in the first step.Maybe the network is not stable.
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.
Also
local: 62.24.197.181:1298
is not my local IP address
Is it better to use mapped instead of local?
./go-stun -b -s stun.stunprotocol.org:3478
Mapping Behavior: EndpointIndependent
Filtering Behavior: AddressAndPortDependent
Normal NAT Type: Port Restricted cone NAT