Skip to content

Conversation

skroll
Copy link

@skroll skroll commented Feb 17, 2016

  • Use the -o or --host option to set the host to listen on, in cases
    where the correct IP cannot be determined.

* Use the -o or --host option to set the host to listen on, in cases
  where the correct IP cannot be determined.
@skroll
Copy link
Author

skroll commented Feb 17, 2016

On my machine I have a lot of different ethernet addresses so being able to override it is helpful. I was getting exceptions thrown preventing it form running in some cases.

@kakaroto
Copy link
Owner

Thanks for the pull. I would rather do a :

if options.host: 
    my_ip = options.host
else:
    my_ip = get_external_ip

As for the GUI, we could just make the ip field user-editable.

@skroll
Copy link
Author

skroll commented Feb 17, 2016

That works, too. In the GUI, it still uses get_external_ip but it causes a thrown exception if it can't find the IP so that GUI can't even start.

@kakaroto
Copy link
Owner

Ah ok, I hadn't understood that. In that case, it needs to be fixed but not by forcing a -o option. Can you show me the exception you get and the result of the various stages in get_external_ip so we can track down why it can't find your IP ?
I thought your issue was that you had multiple interfaces, each with an external IP (multi-homed system with multiple NICs) and you needed to select which one was on the same router as your phone.

@skroll
Copy link
Author

skroll commented Feb 17, 2016

There's actually two issues I'm facing:

  • On one machine, the wrong IP is getting selected (it's picking up a virtual NIC that is used by vmware for bridged networks)
  • On another machine, I get an exception:
Traceback (most recent call last):
  File "SWProxy.py", line 162, in <module>
    win = gui.MainWindow(get_external_ip(), options.port)
  File "SWProxy.py", line 85, in get_external_ip
    ips = [l for l in ([ip for ip in socket.gethostbyname_ex(socket.gethostname())[2] if not ip.startswith("127.")][:1], sockets) if l]
socket.gaierror: [Errno 8] nodename nor servname provided, or not known

@skroll
Copy link
Author

skroll commented Mar 25, 2016

Any way this sort of feature can get pushed upstream? I'll update my PR with the code. Not having to maintain a local branch would be nice.

@fperegrinvs
Copy link
Collaborator

the issues you were facing should be corrected by now.
The force option should be useful for some scenarios and its simple enough to maintain/support +1 for merging this (.but your branch have some conflicts ).

@skroll
Copy link
Author

skroll commented Mar 25, 2016

Yeah my PR was from a previous version. I pulled the latest code and I still have an issue.

I'll fix it up.

@fperegrinvs
Copy link
Collaborator

Many people are reporting that sw picks the wrong IP address. I guess this feature can help them

@kakaroto
Copy link
Owner

Yeah, but also, WHY is it picking the wrong IP.. it didn't do that before.. I think the change to auto-detect ip in a "reliable" way somehow screwed things up.
Being able to manually set it will be great for some people, but most non-tech people will be lost and wouldn't know what to put in there.

@fperegrinvs
Copy link
Collaborator

during the 0.98 release, some people reported that problem. I opened an issue #48 and checked what have changed since 0.97 and found this change ffa2ff11c760784624d36cd97b1073804e8440cd which seems to be an evolution from the previous code to fix something. What was fixed ?

Instead of reversing the change I decided to move forward and exclude all invalid ip and order the list putting common local ips on top. Lots of people reported that the change fixed their problems then I closed the issue.

Maybe the old code also had some problems (I don't know why it was changed) and we didn't notice that earlier because we had few mac users before.

@ghost
Copy link

ghost commented Mar 28, 2016

@kakaroto @lstern checkout the branch/PR I pushed, it enables a --ipdetect2 option, can you guys test? It requires netifaces plugin, but should be cross platform.

Since it seems like a better defined solution (if it works) perhaps we can chuck out all the current IP method detection in favor of using this? let me know what you think

@kakaroto
Copy link
Owner

Oh yeah, I remember what the issue is...
The original code I had was doing something really simple :
connect to 8.8.8.8 using any interface (let the kernel decide how to connect to that IP), then it asks the socket what is the local ip that it connected from, thus giving us the main, primary interface that is internet-enabled and is able to connect to external servers...
I had found that code snippet from stackoverflow but couldn't remember the link to it.. then when aztheros wrote the code cleaning patch, he put the link to the stackoverflow article (I think a different one, but using a similar line) and added something else to it which makes it look up the local hostname first. It's based on this https://stackoverflow.com/questions/166506/finding-local-ip-addresses-using-pythons-stdlib/1267524#1267524 and @azrethos replaced my original one liner by what appears to be an update to it by its original author of that one liner :

 -    my_ip = [[(s.connect(('8.8.8.8', 80)), s.getsockname()[0], s.close()) for s in [socket.socket(socket.AF_INET, socket.SOCK_DGRAM)]][0][1]][0]
 +def get_external_ip():
 +    # ref: http://stackoverflow.com/a/1267524
 +    try:
 +        sockets = [[(s.connect(('8.8.8.8', 80)), s.getsockname()[0], s.close()) for s in [socket.socket(socket.AF_INET, socket.SOCK_DGRAM)]][0][1]]
 +        ips = [l for l in ([ip for ip in socket.gethostbyname_ex(socket.gethostname())[2] if not ip.startswith("127.")][:1], sockets) if l]
 +        return ips[0][0]
 +    except KeyError:
 +        # fallback on error
 +        return socket.gethostbyname(socket.gethostname())

If I run the first line (without selecting the internal value from it), I get this :

> [(s.connect(("8.8.8.8", 53)), s.getsockname()[0], s.close()) for s in [socket.socket(socket.AF_INET, socket.SOCK_DGRAM)]]
     [(None, '192.168.2.44', None)]

If I run that second metod, I get this :

 > socket.gethostbyname_ex(socket.gethostname())
       ('localhost.localdomain', ['localhost', 'kakaroto'], ['127.0.0.1'])

And it looks like that gethostname() just gives whatever hostname is set in /etc/hosts (on linux/mac) so the final code basically merges those two lists, then skips the 127.* ip addresses, and in my case, we end up with just the 192.168.x address. In some other people's case, you may end up with whatever other local hostname gives you.. people with some kind of VPN app get something else, etc.. and in general, it all becomes a mess...
Even your alternative method of using the netifaces might be more elaborate, but it will just give you a bunch of interfaces, and you still have to take the first one you find pretty much, which is wrong, because you don't know if that first interface is a valid one, a virtual interface, a virtual machine interface, a VPN interface, etc... and you can try to add as many exceptions as you like to avoid 127.x or whatever other IP addresses are considered 'invalid', but the fact remains that it's not a good solution. I think the best solution is still the very first one, the original code that simply used whatever interface is able to connect to 8.8.8.8. Unless someone is multi-homed and has more than one network interface with its own valid routing to the internet (extremely rare), then the first method will work perfectly. So I vote for just reverting to that original one liner and call it a day.

@fperegrinvs
Copy link
Collaborator

There's no guarantee that old code will select the correct interface. Not sure if code was broke to someone but made a pull request with a hybrid from old and new version but instead of merging the socket.gethostbyname_ex list, it just order and filter the existing interface list.

While writing this code, I found 1 error on current one, code was filtering 192.168.0.0/12 (I did this mistake while trying to fix the code)

@kakaroto
Copy link
Owner

Well, filtering 192.168.0.0/12 might be why people were having issues :p Either way, yes, there is actually a guarantee that the old code will select the correct interface, because we create an outgoing socket that connects to 8.8.8.8 (google's DNS server FYI) so whatever IP it gives us, it's on the interface which has external connection routing. So basically, any of the virtual machines, or loopback interfaces would not get used because they wouldn't be able to connect to 8.8.8.8.
I saw your pull request #69 but the filtering you do would be useless (and doing it this way is exactly to avoid having to start filtering a hardcoded list of ip addresses) since those ips would not result in a successful outgoing connection.

@fperegrinvs
Copy link
Collaborator

fperegrinvs commented Mar 29, 2016 via email

@ghost
Copy link

ghost commented Mar 29, 2016

hey guys; this issue is directly related to simply adding an option to set --host 10.0.2.4 or whatever; I think we should move IP detection alg talk into a relevant issue/PR. Can we just get custom host setter code merged in? seems like a small change, I can submit it in a fresh pull request if you'd like (or attach it to my ip-detect-2 branch)

@fperegrinvs
Copy link
Collaborator

fperegrinvs commented Mar 29, 2016 via email

@stanleykylee
Copy link
Contributor

I agree, i would still like the ability to set a host manually along with the detection fix. I have certain situations where two NICs may be able to access 8.8.8.8 for DNS routing but one NIC doesn't actually have full internet access.

The if host / else detect would be great.

@ghost
Copy link

ghost commented Mar 29, 2016

for the record, I do run a "more than one network interface with its own valid routing to the internet" @kakaroto

@kakaroto
Copy link
Owner

Oh yes, of course having the ability to set it manually is wanted, that's why this issue isn't closed, but those who understand what an IP address is and know how to set it are a minority of the users. We need to fix the detection for everyone else. And for those who are multi-homed, like @azrethos, those can set the host manually and that's it.

@Nightreaver
Copy link

I committed this to the swproxy plugins repo - should be applicable

Nightreaver/SWProxy-plugins@b29ac72
Nightreaver/SWProxy-plugins@49ef772

I also think "host" is misleading, as the proxy will always run locally but on different interfaces

@kakaroto
Copy link
Owner

kakaroto commented Dec 5, 2016

@Nightreaver those look good, if you send a pull request with those commits, I can get it merged.
thanks

@Nightreaver
Copy link

done #165

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.

5 participants