Repository navigation
fix: stop calling the ssh tunnel when removing ssh_tunnel - #796
Conversation
A connection update that leaves the tunnel disabled makes the server delete the tunnel, so the follow-up call to the tunnel answers 404. Treat that answer as the tunnel being already gone.
Only an already removed tunnel is accepted when removing ssh_tunnel; a 403 on removal and a 404 on a hostname change still fail the apply.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the change that tolerates already-deleted SSH tunnels during removal.
No open issues 🎉
What changed in this PR
This PR makes SSH tunnel removal resilient when dbt Cloud has already deleted the tunnel during a Postgres or Redshift global connection update.
Changes:
- Accepts
resource-not-founderrors when deleting a tunnel, without changing create or update error handling. - Adds mocked regression tests for successful removal and continued failure on other removal or hostname-change errors.
- Adds an unreleased changelog entry.
| File | Description |
|---|---|
pkg/framework/objects/global_connection/resource.go |
Allows deletion to succeed when the tunnel is already gone. |
pkg/framework/objects/global_connection/resource_ssh_tunnel_unit_test.go |
Tests removal for both adapters and preserves error-handling coverage. |
.changes/unreleased/Fixes-20261106-150000.yaml |
Documents the SSH tunnel removal fix. |
Review effort: Balanced
skapfer
left a comment
skapfer
left a comment
There was a problem hiding this comment.
Wondering whether we should just not update the tunnel config at all in the delete path. The connection update already does the job, and the extra encryption_info update seems redundant in every case?
Conceptually, encryption info should be in connection config, not its own resource at the API level. So I think it's OK to omit state updates that serve no purpose.
| @@ -1773,7 +1773,8 @@ func (r *globalConnectionResource) handleSSHTunnelUpdates( | |||
| State: dbt_cloud.STATE_DELETED, | |||
There was a problem hiding this comment.
Do we still need to POST to the encryptions endpoint? It looks like the connection update precedes the encryption write, so when the SSH tunnel goes away in a TF apply, this will always return 404.
There was a problem hiding this comment.
Good call, I think it makes to avoid the extra call. thanks!
|
Related PR: https://github.com/dbt-labs/dbt-cloud/pull/20030 |
The connection update that comes first already removes the tunnel, so the follow-up call to the tunnel is redundant and answers 404 once the server hard-deletes it. Drop the call and keep the guard on tunnel changes.
fernando-toledo
changed the title
TL/DR
Removing the
ssh_tunnelblock of a Postgres or Redshiftdbtcloud_global_connectionno longer calls the tunnel, because the connection update that comes first already removes it.Why
Since https://github.com/dbt-labs/dbt-cloud/pull/19697 (hard delete of
EncryptionInfo), the server deletes a connection's SSH tunnel as part of the connection update that leaves the tunnel disabled. The provider then called the tunnel to delete it and got404 resource-not-found, so the apply failed withError with the SSH Tunnel. This is whyTestAccDbtCloudGlobalConnectionPostgresResourceandTestAccDbtCloudGlobalConnectionRedshiftResourcefail in CI onmainand on other PRs.The call was redundant before that change too: the update already disabled the tunnel, and Read only shows a tunnel while
is_ssh_tunnel_enabledis true.Before
ssh_tunnelfails withError with the SSH Tunnel(404 onPOST /v2/accounts/<id>/encryptions/<id>/).After
ssh_tunnelmakes one request, the connection update, and the apply succeeds.Notes
state=2hard-delete the tunnel, which keeps older provider versions working.