diff --git a/CHANGELOG.md b/CHANGELOG.md index bce9e16e4..931acda0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Release History +# Unreleased +- Fix: transport failures (connection refused, proxy/tunnel errors) on the Thrift backend raise `RequestError` (`OperationalError`) instead of a raw `urllib3` exception. +- Fix: an explicitly empty `access_token` raises `No valid authentication settings! access_token is empty` instead of silently starting the interactive browser OAuth login, including when a known Reyden warehouse skips Thrift. Explicit `auth_type`, `credentials_provider` and certificate authentication still take precedence. + # 4.6.0 (2026-09-24) - Upgrade Databricks SQL Kernel to 1.1.0; the kernel dependency is now stable and no longer experimental. - Transparently auto-recover Thrift connections to Reyden / Real-Time warehouses: when a warehouse rejects the default Thrift protocol (SQLSTATE `KP001`), the session is re-opened on the kernel backend and the warehouse is remembered so later connections skip Thrift. Applies only when no backend was chosen explicitly. diff --git a/src/databricks/sql/auth/auth.py b/src/databricks/sql/auth/auth.py index a4d4d6f2e..819cd7f39 100755 --- a/src/databricks/sql/auth/auth.py +++ b/src/databricks/sql/auth/auth.py @@ -10,6 +10,8 @@ from databricks.sql.auth.common import AuthType, ClientContext from databricks.sql.auth.token_federation import TokenFederationProvider +EMPTY_ACCESS_TOKEN_MESSAGE = "No valid authentication settings! access_token is empty" + def get_auth_provider(cfg: ClientContext, http_client): # Determine the base auth provider @@ -42,11 +44,15 @@ def get_auth_provider(cfg: ClientContext, http_client): http_client, cfg.auth_type, ) - elif cfg.access_token is not None: + elif cfg.access_token: base_provider = AccessTokenAuthProvider(cfg.access_token) elif cfg.use_cert_as_auth and cfg.tls_client_cert_file: # no op authenticator. authentication is performed using ssl certificate outside of headers base_provider = AuthProvider() + elif cfg.access_token is not None: + # An explicitly empty token is a missing credential; never fall back + # to the default interactive browser login for it. + raise RuntimeError(EMPTY_ACCESS_TOKEN_MESSAGE) else: if ( cfg.oauth_redirect_port_range is not None diff --git a/src/databricks/sql/backend/thrift_backend.py b/src/databricks/sql/backend/thrift_backend.py index fcf363cf0..a98053760 100644 --- a/src/databricks/sql/backend/thrift_backend.py +++ b/src/databricks/sql/backend/thrift_backend.py @@ -459,7 +459,11 @@ def attempt_request(attempt): f"GetOperationStatus failed with HTTP error and will be retried: {str(err)}" ) else: - raise err + # Not retried here (urllib3 already applied the retry policy), + # but surfaced as a DB-API RequestError rather than a raw + # urllib3 exception. + error = err + error_message = str(err) except OSError as err: error = err error_message = str(err) diff --git a/src/databricks/sql/client.py b/src/databricks/sql/client.py index 3e06aa39b..f5a0612c6 100755 --- a/src/databricks/sql/client.py +++ b/src/databricks/sql/client.py @@ -67,7 +67,10 @@ from databricks.sql.result_set import ResultSet from databricks.sql.types import Row, SSLOptions -from databricks.sql.auth.auth import get_python_sql_connector_auth_provider +from databricks.sql.auth.auth import ( + EMPTY_ACCESS_TOKEN_MESSAGE, + get_python_sql_connector_auth_provider, +) from databricks.sql.experimental.oauth_persistence import OAuthPersistence from databricks.sql.session import Session from databricks.sql.backend.types import CommandId, BackendType, CommandState, SessionId @@ -372,7 +375,7 @@ def read(self) -> Optional[OAuthToken]: http_path, ) - if access_token: + if access_token is not None: access_token_kv = {"access_token": access_token} kwargs = {**kwargs, **access_token_kv} @@ -581,6 +584,11 @@ def kernel_recovery_kwargs() -> dict: or recovery_kwargs.get("credentials_provider") ) if recovery_kwargs.get("auth_type") is None and not has_credential_shape: + if recovery_kwargs.get("access_token") is not None: + # An explicitly empty token: the Thrift path rejects it + # rather than defaulting to the interactive OAuth login + # (see get_auth_provider), so do the same here. + raise RuntimeError(EMPTY_ACCESS_TOKEN_MESSAGE) recovery_kwargs["auth_type"] = AuthType.DATABRICKS_OAUTH.value return recovery_kwargs diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index d1b941208..d3a9b7e8a 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -152,6 +152,24 @@ def test_get_python_sql_connector_auth_provider_access_token(self): auth_provider.add_headers(headers) self.assertEqual(headers["Authorization"], "Bearer dpi123") + def test_get_python_sql_connector_auth_provider_empty_access_token(self): + """An explicitly empty token must not start an interactive OAuth login.""" + with self.assertRaisesRegex(RuntimeError, "No valid authentication settings!"): + get_python_sql_connector_auth_provider( + "moderakh-test.cloud.databricks.com", MagicMock(), access_token="" + ) + + def test_get_python_sql_connector_auth_provider_empty_access_token_cert_auth(self): + """An empty token does not override certificate authentication.""" + auth_provider = get_python_sql_connector_auth_provider( + "moderakh-test.cloud.databricks.com", + MagicMock(), + access_token="", + _tls_client_cert_file="fake.cert", + _use_cert_as_auth="abc", + ) + self.assertIsNotNone(auth_provider) + def test_get_python_sql_connector_auth_provider_external(self): class MyProvider(CredentialsProvider): def auth_type(self) -> str: diff --git a/tests/unit/test_session.py b/tests/unit/test_session.py index 69522bc60..1ec10d758 100644 --- a/tests/unit/test_session.py +++ b/tests/unit/test_session.py @@ -954,6 +954,20 @@ def test_oauth_default_recovery_injects_databricks_oauth_auth_type( finally: conn.close() + @patch("%s.session.ThriftDatabricksClient" % PACKAGE) + def test_known_reyden_empty_access_token_is_rejected(self, mock_thrift): + # The Thrift path rejects an explicitly empty token instead of starting + # the interactive OAuth login; skipping Thrift for a known Reyden + # warehouse must not turn it into a databricks-oauth login either. + from databricks.sql.backend import reyden_warehouse_cache + + reyden_warehouse_cache.mark_reyden(self.HOST, "wh-reyden") + with self._fake_kernel() as mock_kernel: + with pytest.raises(RuntimeError, match="access_token is empty"): + self._connect(access_token="") + mock_kernel.assert_not_called() + mock_thrift.return_value.open_session.assert_not_called() + @patch("%s.session.ThriftDatabricksClient" % PACKAGE) def test_pat_recovery_does_not_inject_auth_type(self, mock_thrift): # With a credential shape present (here a PAT) the kernel routes on it diff --git a/tests/unit/test_thrift_backend.py b/tests/unit/test_thrift_backend.py index ddcb378ff..3c55c8820 100644 --- a/tests/unit/test_thrift_backend.py +++ b/tests/unit/test_thrift_backend.py @@ -2047,6 +2047,36 @@ def test_make_request_will_retry_GetOperationStatus_for_http_error( f"{EXPECTED_RETRIES}/{EXPECTED_RETRIES}", cm.exception.context["attempt"] ) + @patch("thrift.transport.THttpClient.THttpClient") + def test_make_request_wraps_urllib3_http_error_as_request_error( + self, t_transport_class + ): + import urllib3 + + t_transport_instance = t_transport_class.return_value + t_transport_instance.code = None + t_transport_instance.headers = {} + mock_method = Mock() + mock_method.__name__ = "OpenSession" + mock_method.side_effect = urllib3.exceptions.MaxRetryError( + None, "/", "Tunnel connection failed: 503" + ) + + thrift_backend = ThriftDatabricksClient( + "foobar", + 443, + "path", + [], + auth_provider=AuthProvider(), + ssl_options=SSLOptions(), + http_client=MagicMock(), + ) + + with self.assertRaises(RequestError) as cm: + thrift_backend.make_request(mock_method, Mock()) + + self.assertIn("Tunnel connection failed", str(cm.exception.message_with_context())) + @patch("thrift.transport.THttpClient.THttpClient") def test_make_request_wont_retry_if_error_code_not_429_or_503( self, t_transport_class