-
Notifications
You must be signed in to change notification settings - Fork 313
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
Improvement/naive detection of nat #292
Conversation
064c483
to
5195372
Compare
… the same as outbound ip
5195372
to
4c52040
Compare
@@ -105,7 +110,16 @@ func (cmd *Command) Start() (err error) { | |||
return err | |||
} | |||
|
|||
sessionManager := cmd.sessionManagerFactory(primitives, vpnServerIP) | |||
if outboundIP != publicIP { | |||
log.Infof( |
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 be higher level 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.
It's just a warning - not an error
// if for some reason we will need truly external IP, use GetPublicIP() | ||
vpnServerIP, err := cmd.ipResolver.GetOutboundIP() | ||
outboundIP, err := cmd.ipResolver.GetOutboundIP() |
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.
Aren't we doing option openvpn.ip
with this?
For those who wants has different public IP and wats to map it in router
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 not such option at the moment. And this is a quick detection with posibility for user to take some action.
forwardInfo := fmt.Sprintf("%s:%v -> %s:%v", publicIP, options.OpenvpnPort, outboundIP, options.OpenvpnPort) | ||
log.Warnf( | ||
`WARNING: It seems that publicaly visible ip: [%s] does not match your local machines ip: [%s]. | ||
You should probaly need to do port forwarding on your router: %s.`, |
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.
..on your router: PUBLIC_IP:1194 -> MACHINE_IP:1194 e.g. %s:1194->%s:1194
and you can put example values from detected values
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.
Sorry, did not see that it's already like that
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.
👍
@@ -76,14 +77,14 @@ func (cmd *Command) Start() (err error) { | |||
providerContact, err := cmd.dialogWaiter.Start() | |||
|
|||
// if for some reason we will need truly external IP, use GetPublicIP() | |||
vpnServerIP, err := cmd.ipResolver.GetOutboundIP() | |||
outboundIP, err := cmd.ipResolver.GetOutboundIP() |
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.
Sad that we do 2-3 requests. Is it possible to avoid it?
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.
Without too much refactoring? Yes - by passing outboundIP to openvpnServiceAddress function as parameter instead of looking up it again inside. I can live with this approach :)
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.
Fixed :) reduced ipify query to 1 request
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.
all good
No description provided.