Skip to content

experimental/ssh: validate dedicated access mode on direct ssh connect#5768

Open
TanishqDatabricks wants to merge 1 commit into
mainfrom
ssh-connect-validate-dedicated-cluster
Open

experimental/ssh: validate dedicated access mode on direct ssh connect#5768
TanishqDatabricks wants to merge 1 commit into
mainfrom
ssh-connect-validate-dedicated-cluster

Conversation

@TanishqDatabricks

Copy link
Copy Markdown
Collaborator

Summary

databricks ssh connect --cluster <id> could be run without going through databricks ssh setup, which is where the cluster's access mode was validated. A user pointing the command directly at a non-dedicated cluster got no early error — the connection only failed later at runtime with an opaque message.

This change validates the cluster's access mode on the direct connect --cluster path as well:

  • The access-mode check (DataSecurityMode == SINGLE_USER) moves into the client package as the exported ValidateClusterAccess. setup now reuses it instead of its own private copy, so there is a single source of truth.
  • Run() calls it only on the direct, non-proxy connect --cluster path:
    • Proxy mode is skipped — its ProxyCommand was generated by setup, which already validated the cluster, so re-checking would add a Clusters.Get on every (re)connection.
    • Serverless is skipped — there is no cluster to inspect.
  • The error message now reports the access mode using the Databricks UI label ("Dedicated" / "Standard" / "No isolation") instead of the raw API enum (SINGLE_USER / USER_ISOLATION), so it matches what the user selected when creating the cluster. Legacy/auto/unknown modes fall back to the raw value.

Example

Before:

cluster '0605-...' does not have dedicated access mode. Current access mode: USER_ISOLATION. ...

After:

cluster '0605-...' does not have Dedicated access mode. Current access mode: Standard. Please ensure the cluster is configured with Dedicated (single user) access mode

Tests

  • Moved the three validateClusterAccess unit tests into client_internal_test.go (now testing the exported function) and updated assertions for the new wording.
  • Added TestAccessModeUILabel covering the enum → UI-label mapping, including the raw-value fallback for legacy modes.

This pull request and its description were written by Isaac.

`ssh connect --cluster` could be run without going through `ssh setup`,
which is where the cluster's access mode was validated. A user pointing
the command directly at a non-dedicated cluster got no early error and
the connection only failed later at runtime.

Move the access-mode check into the client package as the exported
ValidateClusterAccess and call it on the direct, non-proxy
`connect --cluster` path. Proxy mode is skipped (its ProxyCommand was
generated by `setup`, which already validated the cluster) as is
serverless (no cluster to inspect). `setup` now reuses the same function.

The error message now reports the access mode using the Databricks UI
label ("Dedicated"/"Standard"/"No isolation") instead of the raw API
enum (SINGLE_USER/USER_ISOLATION), so it matches what the user selected
when creating the cluster.

Co-authored-by: Isaac
@github-actions

Copy link
Copy Markdown
Contributor

Waiting for approval

Based on git history, these people are best suited to review:

  • @anton-107 -- recent work in experimental/ssh/internal/client/
  • @ilia-db -- recent work in experimental/ssh/internal/client/, experimental/ssh/internal/setup/

Eligible reviewers: @andrewnester, @denik, @pietern, @renaudhartert-db, @shreyas-goenka, @simonfaltum

Suggestions based on git history. See OWNERS for ownership rules.

@eng-dev-ecosystem-bot

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 1f087ba

Run: 28412350444

Env 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 8 13 230 1037 5:16
💚​ aws windows 8 13 232 1035 2:38
💚​ aws-ucws linux 8 13 314 955 4:15
💚​ aws-ucws windows 8 13 316 953 3:03
💚​ azure linux 2 15 230 1036 4:34
💚​ azure windows 2 15 232 1034 2:34
💚​ azure-ucws linux 2 15 316 952 5:12
💚​ azure-ucws windows 2 15 318 950 2:55
💚​ gcp linux 2 15 229 1038 4:50
💚​ gcp windows 2 15 231 1036 2:34
21 interesting tests: 13 SKIP, 8 RECOVERED
Test Name aws linux aws windows aws-ucws linux aws-ucws windows azure linux azure windows azure-ucws linux azure-ucws windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/permissions 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestAccept/bundle/resources/permissions/jobs/destroy_without_mgmtperms/with_permissions 💚​R 💚​R 💚​R 💚​R 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestAccept/bundle/resources/permissions/jobs/destroy_without_mgmtperms/with_permissions/DATABRICKS_BUNDLE_ENGINE=direct 💚​R 💚​R 💚​R 💚​R
💚​ TestAccept/bundle/resources/permissions/jobs/destroy_without_mgmtperms/with_permissions/DATABRICKS_BUNDLE_ENGINE=terraform 💚​R 💚​R 💚​R 💚​R
💚​ TestAccept/bundle/resources/permissions/jobs/destroy_without_mgmtperms/without_permissions 💚​R 💚​R 💚​R 💚​R 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestAccept/bundle/resources/permissions/jobs/destroy_without_mgmtperms/without_permissions/DATABRICKS_BUNDLE_ENGINE=direct 💚​R 💚​R 💚​R 💚​R
💚​ TestAccept/bundle/resources/permissions/jobs/destroy_without_mgmtperms/without_permissions/DATABRICKS_BUNDLE_ENGINE=terraform 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/resources/postgres_branches/basic 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/postgres_branches/recreate 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/postgres_branches/replace_existing 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/postgres_branches/update_protected 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/postgres_branches/without_branch_id 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/postgres_endpoints/basic 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/postgres_projects/update_display_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/synced_database_tables/basic 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
Top 3 slowest tests (at least 2 minutes):
duration env testname
3:27 gcp linux TestSecretsPutSecretStringValue
3:09 aws linux TestSecretsPutSecretStringValue
2:07 azure-ucws linux TestSecretsPutSecretStringValue

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.

2 participants