Repository navigation
Conversation
When an administrator disables port forwarding to serverless compute, the driver proxy answers every request to the tunnel server with 403 FEATURE_DISABLED. ssh connect treated that like a server that was still starting, kept polling /metadata for the full 5-minute timeout, and then failed with a generic "status code 403" error while the compute it had started stayed up. A 403 from /metadata is now final: connect stops on the first one with the proxy's own message, both in the wait loop and on the reuse path (where it would otherwise submit another server), and cancels the run it submitted. Every other non-200, such as 400 INVALID_STATE from a stale metadata.json, is still retried. The test server models the port_fwd_serverless_enabled setting so the acceptance test reaches the 403 through the setting rather than a stub. DECO-28889 Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The settings v2 backend decodes the PATCH body as a protobuf message, in
which false is the default, so {"boolean_val": {"value": false}} arrives
empty and is rejected with "must contain at least one subfield with a
non-default value" (seen on dogfood). The fake accepted it, so the
acceptance test turned the setting off with a request the real API
refuses. The fake now rejects it the same way, and the test sends the
name in the body, which is what the real API needs.
Co-authored-by: Isaac <no-reply@databricks.com>
…mpute A serverless ssh connect now reads the port_fwd_serverless_enabled workspace setting first, in proxy mode too. When an administrator has turned it off, connect stops before it creates the secret scope, uploads binaries or submits the server run, and says which setting is off and who can change it. The driver proxy's gate also depends on an enforcement flag and a port allow-list the CLI cannot see, and the setting is not served through the public API in every environment (production today). A failed read therefore lets the connection go ahead, and the 403 handling from the previous change stays the backstop. The acceptance test for that path now stubs the settings read the way production answers it. DECO-28889 Co-authored-by: Isaac <no-reply@databricks.com>
Both refusals from the previous changes were reported as SERVER_START_TIMEOUT (the driver proxy's 403) or UNKNOWN (the up-front setting check), so they looked like slow server starts or CLI bugs. They now report PORT_FORWARDING_DISABLED when the setting check stops the connection before any compute is started, and DRIVER_PROXY_FORBIDDEN when the driver proxy refuses the server after its run was submitted. Keeping them apart shows how often the setting can't be read and the proxy has to catch it. The values need the matching universe enum change to be stored. DECO-28889 Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: 3451727
Top 9 slowest tests (at least 2 minutes):
|
mihaimitrea-db
left a comment
There was a problem hiding this comment.
Not sure why my approval is needed? Could this signal something weird into our ownership system?
@anton-107 change LGTM I can approve if this was intentional.
1ea406f to
a84b403
Compare
rugpanov
left a comment
There was a problem hiding this comment.
LGTM. Before you merge, rebase this branch onto the updated base. It still contains the old commits from #7057 and #7063. Because of this, GitHub shows 18 files instead of 6 and reports a merge conflict. Run git rebase --onto origin/ssh-preflight-port-forwarding-setting a441f86cf. Your commit applies to a84b40362 without conflicts, and the SSH unit tests and the two acceptance tests pass there.
| // (port_fwd_serverless_enabled), read from the setting before any compute was started. | ||
| SshTunnelErrorCategoryPortForwardingDisabled SshTunnelErrorCategory = "PORT_FORWARDING_DISABLED" | ||
|
|
||
| // The driver proxy refused the SSH server with a 403 after its run was submitted. Usually |
There was a problem hiding this comment.
[nice to have] This comment says that the 403 comes "after its run was submitted". But ensureSSHServerIsRunning also returns errDriverProxyForbidden from its first getServerMetadata call, before it submits a run. This occurs when a metadata.json file from an earlier connection exists. Thus a person who reads the data can think that each DRIVER_PROXY_FORBIDDEN row started compute. Change the text to "refused a request to the SSH server with a 403, for a server that was already running or for one that this connection started". Make the same change in the enum.proto comment in databricks-eng/universe#2802257 and in the table in the PR description.
Changes
Two new
SshTunnelErrorCategoryvalues for the port forwarding refusals added in #7057 and #7063:PORT_FORWARDING_DISABLEDport_fwd_serverless_enabledreturnsfalse, before any compute is startedUNKNOWNDRIVER_PROXY_FORBIDDENSERVER_START_TIMEOUTBoth acceptance tests now also check the
error_categoryin the uploaded telemetry event.Why
Stacked on #7063, PR 3 of DECO-28889. Without these, a disabled setting shows up as slow server starts or as CLI bugs. Keeping the two apart shows how often the setting can't be read (production today) and the proxy's 403 has to catch it.
The backend enum needs the matching values: https://github.com/databricks-eng/universe/pull/2802257. Either PR can land first; until the universe one does, ingestion drops these values.
Tests
acceptance/ssh/connect-serverless-port-forwarding-disabledreportsPORT_FORWARDING_DISABLED.acceptance/ssh/connect-serverless-feature-disabledreportsDRIVER_PROXY_FORBIDDEN.acceptance/sshtests andexperimental/ssh/...unit tests pass.This pull request and its description were written by Isaac.