Skip to content

[iris] Drop implicit SA fallback from SSH impersonation#4382

Merged
rjpower merged 1 commit intomainfrom
iris/drop-ssh-impersonate-fallback
Apr 3, 2026
Merged

[iris] Drop implicit SA fallback from SSH impersonation#4382
rjpower merged 1 commit intomainfrom
iris/drop-ssh-impersonate-fallback

Conversation

@rjpower
Copy link
Copy Markdown
Collaborator

@rjpower rjpower commented Apr 3, 2026

Remove the service_account parameter from ssh_impersonate_service_account()
so impersonation only happens when explicitly configured in SshConfig. Previously
the function fell back to the worker/controller service_account, which caused
metadata-mode SSH commands to include --impersonate-service-account unnecessarily,
requiring the controller SA to hold iam.serviceAccountUser on every worker SA.
Now metadata mode uses the VM's ambient identity and OS Login setups must set
ssh.impersonate_service_account explicitly.

@rjpower rjpower added the agent-generated Created by automation/agent label Apr 3, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 3, 2026

Claude finished @rjpower's task in 1m 9s —— View job


PR Review: Drop implicit SA fallback from SSH impersonation

  • Read changed files and understand the diff
  • Check for correctness and edge cases
  • Post review feedback

Verdict: Looks good — clean, focused removal of the implicit fallback. The change is well-motivated and correctly applied across all call sites.

What this does

Removes the service_account parameter from ssh_impersonate_service_account() so it only returns an SA when ssh_config.impersonate_service_account is explicitly set. Previously, it fell back to the worker/controller SA, which caused metadata-mode SSH to include --impersonate-service-account unnecessarily.

Review notes

  1. All call sites updated consistentlyhandles.py, controller.py, workers.py all stop passing the now-removed parameter. No stale references remain.

  2. Tests updated correctlytest_gcp_vm_slice_os_login_uses_service_account_impersonation (line 478) and test_gcp_tpu_slice_os_login_resolves_user_from_service_account (line 860) now assert None instead of the old fallback SA, which matches the new behavior. The "prefers explicit" test variants are unchanged and still verify the explicit ssh_config.impersonate_service_account path works.

  3. Minor observation: _service_account field is now unused on the handlesGcpStandaloneWorkerHandle._service_account and GcpVmSliceHandle._service_account are still stored and passed through (e.g., handles.py:459) but no longer read for any purpose within the handle classes themselves. If nothing external reads these fields, they could be cleaned up in a follow-up. Not a blocker.

  4. _gcp_tunnel signature cleaned up — the service_account kwarg is removed from the private _gcp_tunnel function and its single caller in GcpControllerProvider.tunnel no longer passes it. Clean.

No issues found. Ship it.

@rjpower rjpower requested a review from ravwojdyla April 3, 2026 04:20
@rjpower rjpower enabled auto-merge (squash) April 3, 2026 04:20
@ravwojdyla
Copy link
Copy Markdown
Contributor

this still needed if #4381 was merged?

@rjpower
Copy link
Copy Markdown
Collaborator Author

rjpower commented Apr 3, 2026

this is more cleaning up the path that shouldn't have been there, i'll rebase

…rvice_account

Remove the service_account parameter from ssh_impersonate_service_account()
so it only returns a value when impersonate_service_account is explicitly set
in SshConfig. In metadata mode the controller's ambient VM identity is used
for gcloud SSH — no impersonation needed. OS Login setups that need
impersonation must set ssh.impersonate_service_account explicitly.
@rjpower rjpower force-pushed the iris/drop-ssh-impersonate-fallback branch from d15bac5 to 4bbf326 Compare April 3, 2026 15:27
@rjpower rjpower disabled auto-merge April 3, 2026 15:32
@rjpower rjpower merged commit f3f2c4d into main Apr 3, 2026
37 of 38 checks passed
@rjpower rjpower deleted the iris/drop-ssh-impersonate-fallback branch April 3, 2026 15:32
Helw150 pushed a commit that referenced this pull request Apr 8, 2026
Remove the service_account parameter from
ssh_impersonate_service_account()
so impersonation only happens when explicitly configured in SshConfig.
Previously
the function fell back to the worker/controller service_account, which
caused
metadata-mode SSH commands to include --impersonate-service-account
unnecessarily,
requiring the controller SA to hold iam.serviceAccountUser on every
worker SA.
Now metadata mode uses the VM's ambient identity and OS Login setups
must set
ssh.impersonate_service_account explicitly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-generated Created by automation/agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants