Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance pre/post cleanup by bypassing GitHub-hosted runners #431

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MoChilia
Copy link
Member

@MoChilia MoChilia commented Apr 9, 2024

The purpose of pre/post cleanup is to ensure no Azure account remains active before and after a job containing azure/login. This measure prevents incorrect operations on unexpected Azure accounts and protects against the disclosure of Azure accounts.

However, certain scenarios are ephemeral for only one job and don't need pre/post cleanup. Two main scenarios fall into this category:

  1. GitHub-hosted runners: A GitHub-hosted runner is on a VM that gets re-imaged for every job. Once the job finishes, the VM is automatically decommissioned (see Using a GitHub-hosted runner). Since the VM is ephemeral, we can bypass this scenario to save time for our users. This bypass will be implemented in this PR. To detect whether a runner is GitHub-hosted, I refer to the solution here.
  2. Running an ephemeral container on a self-hosted runner: This scenario, as described in (1.6.0 versions Pre login Fails on self hosted runners #403 (comment), involves running a container on a self-hosted runner where az is not pre-installed. We address this scenario by implementing PR Fix #403: Catch the error thrown in pre and post steps #407.

With this PR merged, it is assumed that pre/post cleanup will only take effect in scenarios where it is truly required.

@MoChilia MoChilia requested a review from YanaXu April 9, 2024 08:51
@MoChilia MoChilia self-assigned this Apr 9, 2024
@HowardvanRooijen
Copy link

Yes please... this would shave a few minutes off every single one of our builds...

@maskati
Copy link

maskati commented Apr 17, 2024

Thanks for this PR. A note for 2. Running an ephemeral container on a self-hosted runner, my understanding is that #407 will not fix the case of ephemeral self-hosted runners that already have Azure CLI installed.

It might be quite difficult to determine if a job is running on an ephemeral runner. This information is in the runner config, but does not seem to be exposed to the running job. The easiest option would probably be to allow control of this through an action input parameter e.g. disable-cleanup.

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.

Make pre and post action cleanup (az account clear) optional for better performance on ephemeral runners
3 participants