Skip to content

Commit 1e5d82a

Browse files
rahuls-dbIsaac
andcommitted
Mirror the Thrift auth default onto the Reyden kernel recovery path
On auto-recovery the Thrift default of auth_type=None (implicitly databricks-oauth) was forwarded unchanged to the kernel, which has no such fallback and rejects auth_type=None unless a PAT/M2M credential shape is present — so a bare OAuth-U2M connection failed to recover. Inject auth_type=databricks-oauth on the default-path kernel retry when auth_type is unset and no credential shape is present, mirroring Thrift; skip it when a credential shape exists so kernel routing is unchanged. Also correct the warehouse-cache docstring: warehouse ids are globally unique, so there is no cross-workspace collision on a shared SPOG host; the host component is an optimization, not a correctness requirement. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
1 parent e4fbaf2 commit 1e5d82a

3 files changed

Lines changed: 66 additions & 7 deletions

File tree

‎src/databricks/sql/backend/reyden_warehouse_cache.py‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,13 @@
66
here so later connections to the same warehouse skip the doomed Thrift attempt
77
and open on the kernel directly.
88
9-
Keyed by ``(host, warehouse_id)`` — the host is part of the key so the same
10-
warehouse id observed on two different workspaces never collides. Entries
11-
expire after ``_TTL_SECONDS`` so a warehouse later reconfigured to accept Thrift
12-
is eventually retried.
9+
Keyed by ``(host, warehouse_id)``. Warehouse ids are globally unique, so the
10+
warehouse id alone identifies the warehouse — even on a SPOG host shared by many
11+
workspaces (where only the ``?o=<workspace-id>`` path param distinguishes them),
12+
there is no cross-workspace collision. The host is kept in the key only as a
13+
cheap optimization (scoping lookups) and defense-in-depth, not for correctness.
14+
Entries expire after ``_TTL_SECONDS`` so a warehouse later reconfigured to accept
15+
Thrift is eventually retried.
1316
"""
1417

1518
import re

‎src/databricks/sql/client.py‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@
7272
from databricks.sql.session import Session
7373
from databricks.sql.backend.types import CommandId, BackendType, CommandState, SessionId
7474

75-
from databricks.sql.auth.common import ClientContext
75+
from databricks.sql.auth.common import AuthType, ClientContext
7676
from databricks.sql.common.unified_http_client import UnifiedHttpClient
7777
from databricks.sql.common.http import HttpMethod
7878

@@ -563,6 +563,27 @@ def build_session(session_kwargs: dict) -> Session:
563563
self.session.open()
564564
return self.session
565565

566+
def kernel_recovery_kwargs() -> dict:
567+
# Kwargs for re-opening on the kernel. The Thrift path treats an
568+
# unset auth_type as databricks-oauth (see get_auth_provider); the
569+
# kernel path has no such fallback and rejects auth_type=None unless
570+
# a credential shape (PAT / OAuth M2M) is present. Mirror the Thrift
571+
# default so a bare OAuth-U2M connection recovers instead of failing
572+
# with NotSupportedError. Skip the injection when a credential shape
573+
# is already present — the kernel routes on it regardless of
574+
# auth_type, and forcing databricks-oauth alongside an M2M secret or
575+
# a credentials_provider would change that routing.
576+
recovery_kwargs = {**kwargs, "use_kernel": True}
577+
has_credential_shape = (
578+
recovery_kwargs.get("access_token")
579+
or recovery_kwargs.get("oauth_client_secret")
580+
or recovery_kwargs.get("oauth_jwt_key_file")
581+
or recovery_kwargs.get("credentials_provider")
582+
)
583+
if recovery_kwargs.get("auth_type") is None and not has_credential_shape:
584+
recovery_kwargs["auth_type"] = AuthType.DATABRICKS_OAUTH.value
585+
return recovery_kwargs
586+
566587
# An explicit backend choice is always honored — auto-recovery engages
567588
# only on the default (Thrift) path.
568589
explicit_backend = kwargs.get("use_kernel", False) or kwargs.get(
@@ -582,7 +603,7 @@ def build_session(session_kwargs: dict) -> Session:
582603
warehouse_id,
583604
server_hostname,
584605
)
585-
return build_session({**kwargs, "use_kernel": True})
606+
return build_session(kernel_recovery_kwargs())
586607

587608
try:
588609
return build_session(kwargs)
@@ -597,7 +618,7 @@ def build_session(session_kwargs: dict) -> Session:
597618
if warehouse_id:
598619
mark_reyden(server_hostname, warehouse_id)
599620
try:
600-
return build_session({**kwargs, "use_kernel": True})
621+
return build_session(kernel_recovery_kwargs())
601622
except Exception as kernel_ex:
602623
# Surface the kernel failure (the actionable one) while keeping
603624
# the original Thrift rejection in the chain for diagnosis.

‎tests/unit/test_session.py‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -930,3 +930,38 @@ def test_recovered_kernel_failure_suppresses_wrapper_telemetry(self, mock_thrift
930930
self._connect(enable_telemetry=True)
931931
mock_fail_log.assert_called_once()
932932
assert mock_fail_log.call_args.kwargs["enable_telemetry"] is False
933+
934+
@patch("%s.session.get_python_sql_connector_auth_provider" % PACKAGE)
935+
@patch("%s.session.ThriftDatabricksClient" % PACKAGE)
936+
def test_oauth_default_recovery_injects_databricks_oauth_auth_type(
937+
self, mock_thrift, mock_provider
938+
):
939+
# A bare connection (no access_token, no auth_type) defaults to OAuth
940+
# U2M on the Thrift path. The kernel path has no such default and would
941+
# reject auth_type=None, so recovery must inject databricks-oauth to
942+
# mirror the Thrift default. (The provider builder is patched so the
943+
# token-less Thrift attempt doesn't build a real OAuth provider, which
944+
# would hit the network at construction.)
945+
mock_thrift.return_value.open_session.side_effect = self._reject()
946+
with self._fake_kernel() as mock_kernel:
947+
conn = self._connect(access_token=None)
948+
try:
949+
_, kwargs = mock_kernel.call_args
950+
assert kwargs["auth_options"]["auth_type"] == "databricks-oauth"
951+
finally:
952+
conn.close()
953+
954+
@patch("%s.session.ThriftDatabricksClient" % PACKAGE)
955+
def test_pat_recovery_does_not_inject_auth_type(self, mock_thrift):
956+
# With a credential shape present (here a PAT) the kernel routes on it
957+
# regardless of auth_type, so recovery must NOT inject databricks-oauth:
958+
# forcing it alongside other credentials could change kernel routing or
959+
# trip the auth bridge's ambiguity guards.
960+
mock_thrift.return_value.open_session.side_effect = self._reject()
961+
with self._fake_kernel() as mock_kernel:
962+
conn = self._connect() # access_token="tok"
963+
try:
964+
_, kwargs = mock_kernel.call_args
965+
assert kwargs["auth_options"].get("auth_type") is None
966+
finally:
967+
conn.close()

0 commit comments

Comments
 (0)