From 192a8e6de48b387c5aef9be8104830a71c46827f Mon Sep 17 00:00:00 2001 From: Tim O'Farrell Date: Mon, 17 Nov 2025 18:18:40 +0000 Subject: [PATCH] Fix for docker regression (#11759) --- .../live_status_app_conversation_service.py | 6 +- .../sandbox/docker_sandbox_service.py | 8 +- openhands/app_server/utils/docker_utils.py | 16 +- .../app_server/test_docker_sandbox_service.py | 37 ++- tests/unit/app_server/test_docker_utils.py | 213 ++++++++++++++---- 5 files changed, 222 insertions(+), 58 deletions(-) diff --git a/openhands/app_server/app_conversation/live_status_app_conversation_service.py b/openhands/app_server/app_conversation/live_status_app_conversation_service.py index a0180f1fc2..b57cf3013e 100644 --- a/openhands/app_server/app_conversation/live_status_app_conversation_service.py +++ b/openhands/app_server/app_conversation/live_status_app_conversation_service.py @@ -63,7 +63,9 @@ from openhands.app_server.sandbox.sandbox_spec_service import SandboxSpecService from openhands.app_server.services.injector import InjectorState from openhands.app_server.services.jwt_service import JwtService from openhands.app_server.user.user_context import UserContext -from openhands.app_server.utils.docker_utils import replace_localhost_hostname +from openhands.app_server.utils.docker_utils import ( + replace_localhost_hostname_for_docker, +) from openhands.experiments.experiment_manager import ExperimentManagerImpl from openhands.integrations.provider import ProviderType from openhands.sdk import LocalWorkspace @@ -473,7 +475,7 @@ class LiveStatusAppConversationService(GitAppConversationService): for exposed_url in exposed_urls if exposed_url.name == AGENT_SERVER ) - agent_server_url = replace_localhost_hostname(agent_server_url) + agent_server_url = replace_localhost_hostname_for_docker(agent_server_url) return agent_server_url def _inherit_configuration_from_parent( diff --git a/openhands/app_server/sandbox/docker_sandbox_service.py b/openhands/app_server/sandbox/docker_sandbox_service.py index 34a541b924..2d7ff39c44 100644 --- a/openhands/app_server/sandbox/docker_sandbox_service.py +++ b/openhands/app_server/sandbox/docker_sandbox_service.py @@ -32,8 +32,9 @@ from openhands.app_server.sandbox.sandbox_service import ( ) from openhands.app_server.sandbox.sandbox_spec_service import SandboxSpecService from openhands.app_server.services.injector import InjectorState -from openhands.app_server.utils.docker_utils import replace_localhost_hostname -from openhands.utils.environment import is_running_in_docker +from openhands.app_server.utils.docker_utils import ( + replace_localhost_hostname_for_docker, +) _logger = logging.getLogger(__name__) SESSION_API_KEY_VARIABLE = 'OH_SESSION_API_KEYS_0' @@ -188,8 +189,7 @@ class DockerSandboxService(SandboxService): ) try: # When running in Docker, replace localhost hostname with host.docker.internal for internal requests - if is_running_in_docker(): - app_server_url = replace_localhost_hostname(app_server_url) + app_server_url = replace_localhost_hostname_for_docker(app_server_url) response = await self.httpx_client.get( f'{app_server_url}{self.health_check_path}' diff --git a/openhands/app_server/utils/docker_utils.py b/openhands/app_server/utils/docker_utils.py index cb12d8f2c3..03821c3974 100644 --- a/openhands/app_server/utils/docker_utils.py +++ b/openhands/app_server/utils/docker_utils.py @@ -1,21 +1,29 @@ from urllib.parse import urlparse, urlunparse +from openhands.utils.environment import is_running_in_docker -def replace_localhost_hostname( + +def replace_localhost_hostname_for_docker( url: str, replacement: str = 'host.docker.internal' ) -> str: - """Replace localhost hostname in URL with the specified replacement. + """Replace localhost hostname in URL with the specified replacement when running in Docker. + + This function only performs the replacement when the code is running inside a Docker + container. When not running in Docker, it returns the original URL unchanged. Only replaces the hostname if it's exactly 'localhost', preserving all other parts of the URL including port, path, query parameters, etc. Args: url: The URL to process - replacement: The hostname to replace localhost with + replacement: The hostname to replace localhost with (default: 'host.docker.internal') Returns: - URL with localhost hostname replaced, or original URL if hostname is not localhost + URL with localhost hostname replaced if running in Docker and hostname is localhost, + otherwise returns the original URL unchanged """ + if not is_running_in_docker(): + return url parsed = urlparse(url) if parsed.hostname == 'localhost': # Replace only the hostname part, preserving port and everything else diff --git a/tests/unit/app_server/test_docker_sandbox_service.py b/tests/unit/app_server/test_docker_sandbox_service.py index f79988773a..d428b3b664 100644 --- a/tests/unit/app_server/test_docker_sandbox_service.py +++ b/tests/unit/app_server/test_docker_sandbox_service.py @@ -697,10 +697,14 @@ class TestDockerSandboxService: assert result is not None assert isinstance(result.created_at, datetime) + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) async def test_container_to_checked_sandbox_info_health_check_success( - self, service, mock_running_container + self, mock_is_docker, service, mock_running_container ): - """Test health check success.""" + """Test health check success when running in Docker.""" # Setup service.httpx_client.get.return_value.raise_for_status.return_value = None @@ -715,7 +719,34 @@ class TestDockerSandboxService: assert result.exposed_urls is not None assert result.session_api_key == 'session_key_123' - # Verify health check was called + # Verify health check was called with Docker-internal URL + service.httpx_client.get.assert_called_once_with( + 'http://host.docker.internal:12345/health' + ) + + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=False, + ) + async def test_container_to_checked_sandbox_info_health_check_success_not_in_docker( + self, mock_is_docker, service, mock_running_container + ): + """Test health check success when not running in Docker.""" + # Setup + service.httpx_client.get.return_value.raise_for_status.return_value = None + + # Execute + result = await service._container_to_checked_sandbox_info( + mock_running_container + ) + + # Verify + assert result is not None + assert result.status == SandboxStatus.RUNNING + assert result.exposed_urls is not None + assert result.session_api_key == 'session_key_123' + + # Verify health check was called with original localhost URL service.httpx_client.get.assert_called_once_with( 'http://localhost:12345/health' ) diff --git a/tests/unit/app_server/test_docker_utils.py b/tests/unit/app_server/test_docker_utils.py index 61bc6432d7..127c6dfc6e 100644 --- a/tests/unit/app_server/test_docker_utils.py +++ b/tests/unit/app_server/test_docker_utils.py @@ -1,31 +1,63 @@ -from openhands.app_server.utils.docker_utils import replace_localhost_hostname +from unittest.mock import patch + +from openhands.app_server.utils.docker_utils import ( + replace_localhost_hostname_for_docker, +) -class TestReplaceLocalhostHostname: - """Test cases for replace_localhost_hostname function.""" +class TestReplaceLocalhostHostnameForDocker: + """Test cases for replace_localhost_hostname_for_docker function.""" - def test_replace_localhost_basic(self): - """Test basic localhost replacement.""" + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_replace_localhost_basic_in_docker(self, mock_is_docker): + """Test basic localhost replacement when running in Docker.""" # Basic HTTP URL - result = replace_localhost_hostname('http://localhost:8080') + result = replace_localhost_hostname_for_docker('http://localhost:8080') assert result == 'http://host.docker.internal:8080' # HTTPS URL - result = replace_localhost_hostname('https://localhost:443') + result = replace_localhost_hostname_for_docker('https://localhost:443') assert result == 'https://host.docker.internal:443' # No port specified - result = replace_localhost_hostname('http://localhost') + result = replace_localhost_hostname_for_docker('http://localhost') assert result == 'http://host.docker.internal' - def test_replace_localhost_with_path_and_query(self): + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=False, + ) + def test_replace_localhost_basic_not_in_docker(self, mock_is_docker): + """Test that localhost is NOT replaced when not running in Docker.""" + # Basic HTTP URL + result = replace_localhost_hostname_for_docker('http://localhost:8080') + assert result == 'http://localhost:8080' + + # HTTPS URL + result = replace_localhost_hostname_for_docker('https://localhost:443') + assert result == 'https://localhost:443' + + # No port specified + result = replace_localhost_hostname_for_docker('http://localhost') + assert result == 'http://localhost' + + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_replace_localhost_with_path_and_query(self, mock_is_docker): """Test localhost replacement preserving path and query parameters.""" # With path - result = replace_localhost_hostname('http://localhost:3000/api/health') + result = replace_localhost_hostname_for_docker( + 'http://localhost:3000/api/health' + ) assert result == 'http://host.docker.internal:3000/api/health' # With query parameters containing localhost - result = replace_localhost_hostname( + result = replace_localhost_hostname_for_docker( 'http://localhost:8080/path?param=localhost&other=value' ) assert ( @@ -34,94 +66,158 @@ class TestReplaceLocalhostHostname: ) # With path containing localhost - result = replace_localhost_hostname('http://localhost:9000/localhost/endpoint') + result = replace_localhost_hostname_for_docker( + 'http://localhost:9000/localhost/endpoint' + ) assert result == 'http://host.docker.internal:9000/localhost/endpoint' # With fragment - result = replace_localhost_hostname('http://localhost:8080/path#localhost') + result = replace_localhost_hostname_for_docker( + 'http://localhost:8080/path#localhost' + ) assert result == 'http://host.docker.internal:8080/path#localhost' - def test_replace_localhost_with_authentication(self): + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_replace_localhost_with_authentication(self, mock_is_docker): """Test localhost replacement with authentication in URL.""" - result = replace_localhost_hostname('http://user:pass@localhost:8080/path') + result = replace_localhost_hostname_for_docker( + 'http://user:pass@localhost:8080/path' + ) assert result == 'http://user:pass@host.docker.internal:8080/path' - result = replace_localhost_hostname('https://admin:secret@localhost:443/admin') + result = replace_localhost_hostname_for_docker( + 'https://admin:secret@localhost:443/admin' + ) assert result == 'https://admin:secret@host.docker.internal:443/admin' - def test_replace_localhost_different_protocols(self): + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_replace_localhost_different_protocols(self, mock_is_docker): """Test localhost replacement with different protocols.""" # FTP - result = replace_localhost_hostname('ftp://localhost:21/files') + result = replace_localhost_hostname_for_docker('ftp://localhost:21/files') assert result == 'ftp://host.docker.internal:21/files' # WebSocket - result = replace_localhost_hostname('ws://localhost:8080/socket') + result = replace_localhost_hostname_for_docker('ws://localhost:8080/socket') assert result == 'ws://host.docker.internal:8080/socket' # WebSocket Secure - result = replace_localhost_hostname('wss://localhost:443/secure-socket') + result = replace_localhost_hostname_for_docker( + 'wss://localhost:443/secure-socket' + ) assert result == 'wss://host.docker.internal:443/secure-socket' - def test_no_replacement_for_non_localhost(self): - """Test that non-localhost hostnames are not replaced.""" + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_no_replacement_for_non_localhost(self, mock_is_docker): + """Test that non-localhost hostnames are not replaced even when in Docker.""" # IP address - result = replace_localhost_hostname('http://127.0.0.1:8080') + result = replace_localhost_hostname_for_docker('http://127.0.0.1:8080') assert result == 'http://127.0.0.1:8080' # Different hostname - result = replace_localhost_hostname('http://example.com:8080') + result = replace_localhost_hostname_for_docker('http://example.com:8080') assert result == 'http://example.com:8080' # Hostname containing localhost but not exact match - result = replace_localhost_hostname('http://mylocalhost:8080') + result = replace_localhost_hostname_for_docker('http://mylocalhost:8080') assert result == 'http://mylocalhost:8080' # Subdomain of localhost - result = replace_localhost_hostname('http://api.localhost:8080') + result = replace_localhost_hostname_for_docker('http://api.localhost:8080') assert result == 'http://api.localhost:8080' # localhost as subdomain - result = replace_localhost_hostname('http://localhost.example.com:8080') + result = replace_localhost_hostname_for_docker( + 'http://localhost.example.com:8080' + ) assert result == 'http://localhost.example.com:8080' - def test_custom_replacement_hostname(self): + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_custom_replacement_hostname(self, mock_is_docker): """Test using custom replacement hostname.""" - result = replace_localhost_hostname('http://localhost:8080', 'custom.host') + result = replace_localhost_hostname_for_docker( + 'http://localhost:8080', 'custom.host' + ) assert result == 'http://custom.host:8080' - result = replace_localhost_hostname( + result = replace_localhost_hostname_for_docker( 'https://localhost:443/path', 'internal.docker' ) assert result == 'https://internal.docker:443/path' - def test_edge_cases(self): - """Test edge cases and malformed URLs.""" + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_edge_cases_in_docker(self, mock_is_docker): + """Test edge cases and malformed URLs when in Docker.""" # Empty string - result = replace_localhost_hostname('') + result = replace_localhost_hostname_for_docker('') assert result == '' # Malformed URL (no protocol) - result = replace_localhost_hostname('localhost:8080') + result = replace_localhost_hostname_for_docker('localhost:8080') assert result == 'localhost:8080' # Just hostname - result = replace_localhost_hostname('localhost') + result = replace_localhost_hostname_for_docker('localhost') assert result == 'localhost' # URL with no hostname - result = replace_localhost_hostname('http://:8080/path') + result = replace_localhost_hostname_for_docker('http://:8080/path') assert result == 'http://:8080/path' # Invalid URL structure - result = replace_localhost_hostname('not-a-url') + result = replace_localhost_hostname_for_docker('not-a-url') assert result == 'not-a-url' - def test_complex_urls(self): + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=False, + ) + def test_edge_cases_not_in_docker(self, mock_is_docker): + """Test edge cases and malformed URLs when not in Docker.""" + # Empty string + result = replace_localhost_hostname_for_docker('') + assert result == '' + + # Malformed URL (no protocol) + result = replace_localhost_hostname_for_docker('localhost:8080') + assert result == 'localhost:8080' + + # Just hostname + result = replace_localhost_hostname_for_docker('localhost') + assert result == 'localhost' + + # URL with no hostname + result = replace_localhost_hostname_for_docker('http://:8080/path') + assert result == 'http://:8080/path' + + # Invalid URL structure + result = replace_localhost_hostname_for_docker('not-a-url') + assert result == 'not-a-url' + + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_complex_urls(self, mock_is_docker): """Test complex URL scenarios.""" # Multiple query parameters and fragments complex_url = 'http://localhost:8080/api/v1/health?timeout=30&retry=3&host=localhost#section' - result = replace_localhost_hostname(complex_url) + result = replace_localhost_hostname_for_docker(complex_url) expected = 'http://host.docker.internal:8080/api/v1/health?timeout=30&retry=3&host=localhost#section' assert result == expected @@ -129,17 +225,21 @@ class TestReplaceLocalhostHostname: encoded_url = ( 'http://localhost:8080/path%20with%20spaces?param=value%20with%20spaces' ) - result = replace_localhost_hostname(encoded_url) + result = replace_localhost_hostname_for_docker(encoded_url) expected = 'http://host.docker.internal:8080/path%20with%20spaces?param=value%20with%20spaces' assert result == expected - def test_integration_with_docker_detection(self): - """Test integration scenario similar to actual usage.""" + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_integration_with_docker_detection_in_docker(self, mock_is_docker): + """Test integration scenario similar to actual usage when in Docker.""" # Simulate the actual usage pattern in the code app_server_url = 'http://localhost:35375' # This is how it's used in the actual code - internal_url = replace_localhost_hostname(app_server_url) + internal_url = replace_localhost_hostname_for_docker(app_server_url) assert internal_url == 'http://host.docker.internal:35375' @@ -147,10 +247,33 @@ class TestReplaceLocalhostHostname: health_check_url = f'{internal_url}/health' assert health_check_url == 'http://host.docker.internal:35375/health' - def test_preserves_original_url_structure(self): + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=False, + ) + def test_integration_with_docker_detection_not_in_docker(self, mock_is_docker): + """Test integration scenario similar to actual usage when not in Docker.""" + # Simulate the actual usage pattern in the code + app_server_url = 'http://localhost:35375' + + # This is how it's used in the actual code + internal_url = replace_localhost_hostname_for_docker(app_server_url) + + # Should return original URL when not in Docker + assert internal_url == 'http://localhost:35375' + + # Test with health check path appended + health_check_url = f'{internal_url}/health' + assert health_check_url == 'http://localhost:35375/health' + + @patch( + 'openhands.app_server.utils.docker_utils.is_running_in_docker', + return_value=True, + ) + def test_preserves_original_url_structure(self, mock_is_docker): """Test that all URL components are preserved correctly.""" original_url = 'https://user:pass@localhost:8443/api/v1/endpoint?param1=value1¶m2=value2#fragment' - result = replace_localhost_hostname(original_url) + result = replace_localhost_hostname_for_docker(original_url) expected = 'https://user:pass@host.docker.internal:8443/api/v1/endpoint?param1=value1¶m2=value2#fragment' assert result == expected