-
Notifications
You must be signed in to change notification settings - Fork 462
fix: correct exception handling in get_external_ip fallback chain #3311
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| """Utils for handling local network with ip and ports.""" | ||
|
|
||
| import os | ||
| import subprocess | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why use |
||
| from typing import Optional | ||
| from urllib import request as urllib_request | ||
|
|
||
|
|
@@ -66,55 +67,58 @@ def get_external_ip() -> str: | |
| # --- Try AWS | ||
| try: | ||
| external_ip = requests.get("https://checkip.amazonaws.com").text.strip() | ||
| assert isinstance(ip_to_int(external_ip), int) | ||
| ip_to_int(external_ip) | ||
| return str(external_ip) | ||
| except ExternalIPNotFound: | ||
| except Exception: | ||
| pass | ||
|
|
||
| # --- Try ipconfig. | ||
| # --- Try ifconfig.me | ||
| try: | ||
| process = os.popen("curl -s ifconfig.me") | ||
| external_ip = process.readline() | ||
| process.close() | ||
| assert isinstance(ip_to_int(external_ip), int) | ||
| result = subprocess.run( | ||
| ["curl", "-s", "ifconfig.me"], capture_output=True, text=True, timeout=10 | ||
| ) | ||
|
Comment on lines
+77
to
+79
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| external_ip = result.stdout.strip() | ||
| ip_to_int(external_ip) | ||
| return str(external_ip) | ||
| except ExternalIPNotFound: | ||
| except Exception: | ||
| pass | ||
|
|
||
| # --- Try ipinfo. | ||
| try: | ||
| process = os.popen("curl -s https://ipinfo.io") | ||
| external_ip = json.loads(process.read())["ip"] | ||
| process.close() | ||
| assert isinstance(ip_to_int(external_ip), int) | ||
| result = subprocess.run( | ||
| ["curl", "-s", "https://ipinfo.io"], capture_output=True, text=True, timeout=10 | ||
| ) | ||
| external_ip = json.loads(result.stdout)["ip"] | ||
| ip_to_int(external_ip) | ||
| return str(external_ip) | ||
| except ExternalIPNotFound: | ||
| except Exception: | ||
| pass | ||
|
|
||
| # --- Try myip.dnsomatic | ||
| try: | ||
| process = os.popen("curl -s myip.dnsomatic.com") | ||
| external_ip = process.readline() | ||
| process.close() | ||
| assert isinstance(ip_to_int(external_ip), int) | ||
| result = subprocess.run( | ||
| ["curl", "-s", "myip.dnsomatic.com"], capture_output=True, text=True, timeout=10 | ||
| ) | ||
| external_ip = result.stdout.strip() | ||
| ip_to_int(external_ip) | ||
| return str(external_ip) | ||
| except ExternalIPNotFound: | ||
| except Exception: | ||
| pass | ||
|
|
||
| # --- Try urllib ipv6 | ||
| try: | ||
| external_ip = urllib_request.urlopen("https://ident.me").read().decode("utf8") | ||
| assert isinstance(ip_to_int(external_ip), int) | ||
| ip_to_int(external_ip) | ||
| return str(external_ip) | ||
| except ExternalIPNotFound: | ||
| except Exception: | ||
| pass | ||
|
|
||
| # --- Try Wikipedia | ||
| try: | ||
| external_ip = requests.get("https://www.wikipedia.org").headers["X-Client-IP"] | ||
| assert isinstance(ip_to_int(external_ip), int) | ||
| ip_to_int(external_ip) | ||
| return str(external_ip) | ||
| except ExternalIPNotFound: | ||
| except Exception: | ||
| pass | ||
|
|
||
| raise ExternalIPNotFound | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two things missing on the test side: First: test_get_external_ip_os_broken still patches os.popen, but this PR removes all os.popen usage from production code. That mock now does nothing; the test passes solely because requests.get is mocked to succeed on the first try. The test name and docstring say "when os.popen is broken," but it no longer tests that scenario. Second: there's no test for the cascade itself. For example: mock requests.get to raise, mock the first subprocess.run call to return a valid IP -> assert the function returns that IP. This is the core contract of get_external_ip() - "try multiple sources in order" - and it has zero coverage today. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,31 +134,22 @@ def mock_call(): | |
|
|
||
|
|
||
| def test_get_external_ip_os_request_urllib_broken(): | ||
| """Test getting the external IP address when os.popen and requests.get/urllib.request are broken.""" | ||
| """Test getting the external IP address when subprocess.run, requests.get, and urllib.request are broken.""" | ||
|
|
||
| class FakeReadline: | ||
| def readline(self): | ||
| return 1 | ||
|
|
||
| def mock_call(): | ||
| return FakeReadline() | ||
| def mock_subprocess_run(*args, **kwargs): | ||
| raise OSError("subprocess.run mocked to fail") | ||
|
|
||
| class FakeResponse: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't |
||
| def text(self): | ||
| return 1 | ||
| text = "not-an-ip" | ||
|
|
||
| def mock_call_two(): | ||
| return FakeResponse() | ||
| def mock_requests_get(*args, **kwargs): | ||
| raise requests.exceptions.ConnectionError("requests.get mocked to fail") | ||
|
|
||
| class FakeRequest: | ||
| def urlopen(self): | ||
| return 1 | ||
|
|
||
| with mock.patch.object(os, "popen", new=mock_call): | ||
| with mock.patch.object(requests, "get", new=mock_call_two): | ||
| urllib.request = MagicMock(return_value=FakeRequest()) | ||
| with pytest.raises(Exception): | ||
| assert utils.networking.get_external_ip() | ||
| with mock.patch("subprocess.run", new=mock_subprocess_run): | ||
| with mock.patch.object(requests, "get", new=mock_requests_get): | ||
| with mock.patch("bittensor.utils.networking.urllib_request.urlopen", side_effect=OSError("urlopen mocked to fail")): | ||
| with pytest.raises(Exception): | ||
| assert utils.networking.get_external_ip() | ||
|
|
||
|
|
||
| # Test formatting WebSocket endpoint URL | ||
|
|
||
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.
dead import